lkml.org 
[lkml]   [2011]   [Dec]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 0/9] XSHM: Shared Memory Driver for ST-E Thor M7400 LTE modem
    Date
    Hi Sjur,

    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.

    On Wednesday 07 December 2011, Sjur Brændeland wrote:
    > 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.

    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?

    > 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?

    > 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?

    > 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. If you cannot, that is something that I think
    needs to be fixed in the interface to the modem. If you can, you
    probably should not be using the platform_bus but instead create
    your own bus_type.

    Arnd
    --
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2011-12-07 13:33    [from the cache]
    ©2003-2011 Jasper Spaans