Re: [PATCH 01/10] Documentation: dt-bindings: mailbox: tegra: Add binding for HSP mailbox

[Stephen Warren]

On 06/28/2016 03:15 AM, Joseph Lo wrote:
> On 06/27/2016 11:55 PM, Stephen Warren wrote:
> > On 06/27/2016 03:02 AM, Joseph Lo wrote:
> > > Add DT binding for the Hardware Synchronization Primitives (HSP). The
> > > HSP is designed for the processors to share resources and communicate
> > > together. It provides a set of hardware synchronization primitives for
> > > interprocessor communication. So the interprocessor communication (IPC)
> > > protocols can use hardware synchronization primitive, when operating
> > > between two processors not in an SMP relationship.
> >
> > This binding is quite different to the binding you sent to internal IP
> > review. I wonder why it changed? Specific comments below:
>
> Due to some enhancements for supporting multiple functions of HSP
> sub-modules in the same driver, I re-wrote some parts of the bindings
> and driver.
>
> > > diff --git
> > > a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
> > > b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt

> > > +- reg : Offset and length of the register set for the device
> > > +- interrupts : Should contain the HSP interrupts
> > > +- interrupt-names: Should contain the names of the HSP interrupts
> > > that the
> > > +           client are using.
> > > +           "doorbell"
> >
> > The binding should describe the HW, and not be affected by anything
> > "that the client(s) are using". If there are multiple interrupts, we
> > should list them all here, from the start.
> >
> > When revising this, I would consider the following wording canonical:
> >
> > - interrupt-names
> >      Array of strings.
> >      Contains a list of names for the interrupts described by the
> >      interrupts property. May contain the following entries, in any
> >      order:
> >      - "doorbell"
> >      - "..." (no doubt many more items will be listed here, e.g.
> >        for semaphores, etc.).
>
> I think I will just list "doorbell" for now. And adding more later once
> we add other HSP sub-module support.

That should be OK; adding more entries in interrupt-names is backwards 
compatible. Still, since the HW is fixed, it would be nice to just get 
the complete list documented up front if possible.

> > > +- nvidia,hsp-function : Specifies one of the HSP functions that the HSP unit
> > > +            will be supported. The function ID can be found in the
> > > +            header file <dt-bindings/mailbox/tegra-hsp.h>.
> >
> > This property wasn't in the internal patch.
> >
> > This doesn't make sense. The HW feature-set is fixed. This sounds like
> > some kind of software configuration option, or a way to allow different
> > drivers to handle different aspects of the HW? In general, the binding
> > shouldn't be influenced by software structure. Please delete this
> > property.
> >
> > Now, if you're attempting to set up a binding where each function
> > (semaphores, shared mailboxes, doorbells, etc.) has a different DT node,
> > then (a) splitting up HW modules into sub-blocks has usually turned out
> > to be a mistake in the past, and (b) the differences should likely be
> > represented by using a different compatible property for each
> > sub-component, rather than via a custom property.
>
> Currently the usage of HSP HW in the downstream kernel is something like
> the model below.
>
> remote_processor_A-\
> remote_processor_B--->hsp@1000 (doorbell func) <-> host CPU
> remote_processor_C-/
>
> remote_processor_D -> hsp@2000 (shared mailbox) <-> CPU
>
> remote_processor_E -> hsp@3000 (shared mailbox) <-> CPU
>
> I am thinking if we can just add the appropriate compatible strings for
> it to replace "nvidia,tegra186-hsp". e.g. "nvidia,tegra186-hsp-doorbell"
> and "nvidia,tegra186-hsp-sharedmailbox". So the driver can probe and
> initialize correctly depend on the compatible property. How do you think
> about it? Is this the same as the (b) you mentioned above?

Yes, that would be (b) above.

However, please do note (a): I expect that splitting things up will turn 
out to be a mistake, as it has for other HW modules in the past. I would 
far rather see a single hsp node in DT, since there is a single HSP 
block in HW. Sure that block has multiple sub-functions. However, there 
is common logic that affects all of those sub-functions and binds 
everything into a single HW module. If you represent the HW module using 
multiple different DT nodes, it will be hard to correctly represent that 
common logic. Conversely, I see no real advantage to splitting up the DT 
node. I strongly believe we should have a single "hsp" node in DT.

Internally, the SW driver for that node can be structured however you 
want; it could register with multiple subsystems (mailbox, ...) with 
just one struct device, or the HSP driver could be an MFD device with 
sub-drivers for each separate piece of functionality the HW implements. 
All this can easily be done even while using a single DT node. And 
furthermore, we can add this SW structure later if/when we actually need 
it; in other words, there's no need to change your current patches right 
now, except to remove the nvidia,hsp-function DT property.