Re: [PATCH 02/22] dt-bindings: Add bindings for Keem Bay IPC driver

[Jassi Brar]

On Tue, Dec 8, 2020 at 6:12 PM mark gross <mgross@xxxxxxxxxxxxxxx> wrote:
>
> On Mon, Dec 07, 2020 at 02:31:37PM -0600, Jassi Brar wrote:
> > On Mon, Dec 7, 2020 at 12:43 PM Daniele Alessandrelli
> > <daniele.alessandrelli@xxxxxxxxxxxxxxx> wrote:
> > >
> > > Hi Rob,
> > >
> > > Thanks for the feedback.
> > >
> > > On Mon, 2020-12-07 at 10:01 -0600, Rob Herring wrote:
> > > > On Tue, Dec 01, 2020 at 02:34:51PM -0800, mgross@xxxxxxxxxxxxxxx wrote:
> > > > > From: Daniele Alessandrelli <daniele.alessandrelli@xxxxxxxxx>
> > > > >
> > > > > Add DT binding documentation for the Intel Keem Bay IPC driver, which
> > > > > enables communication between the Computing Sub-System (CSS) and the
> > > > > Multimedia Sub-System (MSS) of the Intel Movidius SoC code named Keem
> > > > > Bay.
> > > > >
> > >
> > > [cut]
> > >
> > > > > +
> > > > > +description:
> > > > > +  The Keem Bay IPC driver enables Inter-Processor Communication (IPC) with the
> > > > > +  Visual Processor Unit (VPU) embedded in the Intel Movidius SoC code named
> > > > > +  Keem Bay.
> > > >
> > > > Sounds like a mailbox.
> > >
> > > We did consider using the mailbox framework, but eventually decided
> > > against it; mainly because of the following two reasons:
> > >
> > > 1. The channel concept in the Mailbox framework is different than the
> > >    channel concept in Keem Bay IPC:
> > >
> > >    a. My understanding is that Mailbox channels are meant to be SW
> > >       representation of physical HW channels, while in Keem Bay IPC
> > >       channels are software abstractions to achieve communication
> > >       multiplexing over a single HW link
> > >
> > In mailbox api, that would be a physical channel shared between various clients.
> >
> > >    b. Additionally, Keem Bay IPC has two different classes of channels
> > >       (high-speed channels and general-purpose channels) that need to
> > >       access the same HW link with different priorities.
> > >
> > If the priorities are hard (programmed into some register), you could
> > do that via dt during channel population.
> > If they are soft, that would be handled in the shared channel implementation.
> >
> > > 2. The blocking / non-blocking TX behavior of mailbox channels is
> > >    defined at channel creation time (by the tx_block value of the
> > >    mailbox client passed to mbox_request_channel();
> > >
> > No, that is checked at mbox_send_message()
> >
> > > my understanding
> > >    is that the tx_block value cannot be modified after the channel is
> > >    created),
> > >
> > Again no. If you don't queue more than one message at any time you can
> > change it between transfers. To be safe you can always change it
> > between channel release - request calls.
> >
> > >  while in Keem Bay IPC the same channel can be used for
> > >    both blocking and non-blocking TX (behavior is controlled by the
> > >    timeout argument passed to keembay_ipc_send()).
> > >
> > > Having said that, I guess that it could be possible to create a Mailbox
> > > driver implementing the core communication mechanism used by the Keem
> > > Bay IPC and then build our API around it (basically having two
> > > drivers). But I'm not sure that would make the code simpler or easier
> > > to maintain. Any thoughts on this?
> > >
> > I think so. Most of KeemBay specific behaviour would be implemented in
> > the shared channel above the mailbox api.
>
> Quick question.  By "I think so"
>
>From what I have read, it seems it should be doable as a mailbox.

> do you mean that you feel the keem bay IPC
> code will be simpler and easier to maintain if we make yet another driver at the
> Keem Bay IPC driver sits on top off?  Or, the current implementation would be
> simpler if we rework the implementation to use the mailbox api?
>
Most of the queue management code would be taken over my mailbox api.
And you'll have
separated out the pure controller driver from protocol assumptions.
So there shouldn't be extra code... just this code reorganised and some reduced.

cheers!