Messages in this thread | | | From | Sjur BRENDELAND <> | Date | Wed, 7 Dec 2011 16:44:49 +0100 | Subject | RE: [PATCH 0/9] XSHM: Shared Memory Driver for ST-E Thor M7400 LTE modem |
| |
Hi Arnd,
> I've tried to do a review of your code, but I really need to understand > more of the background first, to be able to tell if it's using the > right high-level approach. There are a lot of details I could > comment on, but let's first look at the overall design.
Ok great, looking forward to hearing your feedback.
> > Introduction: > > ~~~~~~~~~~~~~ > > This patch-set introduces the Shared Memory Driver for ST-Ericsson's > > Thor M7400 LTE modem. > > The shared memory model is implemented for Chip-to-chip and > > uses a reserved memory area and a number of bi-directional > > channels. Each channel has it's own designated data area where > payload > > data is copied into. > > Can you explain what the scope of this framework is? What I don't > understand from the code is whether this is meant to be very > broadly applicable to a lot of devices and competing with e.g. > rpmsg from Ohad Ben-Cohen, or whether this is really speciific > to one hardware implementation for caif.
This is primarily designed for supporting the ST-Ericsson's M7400 product and future modem platforms supporting shared memory. There are some parts with a rather tight coupling to the modem such as synchronization of the startup phases "ipc_ready" and "caif_ready" and how we store the 1st stage boot images in shared memory. However there are other concepts and parts of the implementation that probably is possible to reuse.
> Also, to what degree is the protocol design flexible? Is the modem > side fixed, so you have to live with any shortcomings of the interface, > or are you at this point able to improve both sides when something > is found to be lacking?
As I see it, this interface to the modem is pretty much set, and isn't going to change a lot for the M7400 product.
However for the long term perspective: we expect this interface to evolve for future products, so suggestions and input for improvements is welcome. rpmsg or at least the use of virtio-ring combined with a true end-to-end zero copy is something we definitely are interested to look into for the future.
> > Two different channel types are defined, one stream channel which is > > implemented as a traditional ring-buffer, and a packet channel which > > is a ring-buffer of fix sized buffers where each buffer contains > > an array of CAIF frames. > > > > The notification of read and write index updates are handled in a > > separate driver called c2c_genio. This driver will be contributed > separately, > > but the API is included in this patch-set. > > > > The channel configuration is stored in shared memory, each channel > > has a designated area in shared memory for configuration data, > > read and write indexes and a data area. > > How many channels will there be on of each type in a typical system, > and respectively in the maximum you can realistically expect over > the next 10 years?
Currently we have defined the following channels:
Ch# Type Used for RX size TX size ------------------------------------------------------------------ 1 Stream Modem Crash dump 64 kB 4 kB 2 Stream Flash-less Boot Protocol 4 kB 256 kB 3 Stream Boot Logging 8 kB 4kB 4 Packet CAIF high speed 8*16 kB 8*16kB 5 Packet CAIF low latency 4*1kB 4*1kB
The modem is doing a multi stage boot, where it initially reads boot images stored in shared memory. This will boot-strap the 2nd boot stage where the Flash-less Boot Protocol is used. In this phase channel #2 and #3 is used. At this stage only a very basic OS-less runtime system is available, so CAIF protocol cannot be running on the modem.
When the 2nd boot stage is complete, channel #4 and #5 will be used. CAIF is a multiplexing protocol allowing multiple connections, so the reason for two channels is to be able to separate high throughput traffic such as IP + Logging from traffic with low-latency requirements such as control (e.g. AT commands) and audio.
In case of modem crash-dump the modem is running in OS-less mode, and transfers dump over channel #1.
Initially I have implemented separate memory areas for the different channels and the initial boot images, however the channel specification and netlink interface supports overlapping channels. This is a possible future improvement.
> > Stream data > > ~~~~~~~~~~~ > > The driver for the stream channel is implemented as a character device > > interface to user space. The character device implements non-blocking open > > and non-blocking IO in general. The character device is implementing > > a traditional circular buffer directly in the shared memory region for > > the channel. > > It seems unusual to have both a socket interface and a character device > interface. What is the motivation behind this? Why can't you use the > socket for non-blocking I/O?
As mentioned above, the modem is in different run modes, stream device is only used when modem runs in OS-less mode, during boot or crash-dump. Except for boot and crash-dump the CAIF stack is used. A similar approach is used for HSI, where we use a bare HSI channel in OS-less mode, and CAIF over HSI otherwise.
> > Driver model > > ~~~~~~~~~~~~~~ > > Current implementation is using the platform bus for XSHM devices. > > The packet channels are named "xshmp", and stream channel "xshms". > > > > /sys/devices: > > |-- platform > > | |-- uevent > > | `-- xshm > > | |-- bootimg > > | |-- caif_ready > > | |-- ipc_ready > > | |-- subsystem -> ../../../bus/platform > > | |-- xshmp.1 > > | | |-- driver -> ../../../../bus/platform/drivers/xshmp > > | | |-- subsystem -> ../../../../bus/platform > > | `-- xshms.0 > > | |-- driver -> ../../../../bus/platform/drivers/xshms > > | |-- misc > > | | `-- xshm0 > > | | |-- device -> ../../../xshms.0 > > | | |-- subsystem -> ../../../../../../class/misc > > `-- virtual > > |-- net > > | |-- cfshm0 > > Why are the channels /platform/ devices? With all the infrastructure > you have in the xshm driver, I would think that you should be > able to probe the available channels yourself instead of relying the > board to define them.
OK point taken. Using the platform bus, was just share laziness as it provides a simple way of creating device and drivers. I will look into creating a separate bus.
Regards, Sjur
| |