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

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.

Signed-off-by: Joseph Lo <josephl@nvidia.com>
---
 .../bindings/mailbox/nvidia,tegra186-hsp.txt       | 42 ++++++++++++++++++++++
 include/dt-bindings/mailbox/tegra-hsp.h            | 20 +++++++++++
 2 files changed, 62 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt
 create mode 100644 include/dt-bindings/mailbox/tegra-hsp.h

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:

> diff --git a/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt b/Documentation/devicetree/bindings/mailbox/nvidia,tegra186-hsp.txt

> +NVIDIA Tegra Hardware Synchronization Primitives (HSP)
> +
> +The HSP modules are used 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 primitives, when operating between
> +two processors not in an SMP relationship.
> +
> +The features that HSP supported are shared mailboxes, shared semaphores,
> +arbitrated semaphores and doorbells.
> +
> +Required properties:
> +- name : Should be hsp
> +- compatible : Should be "nvidia,tegra<chip>-hsp"

I think this should explicitly list the value values of the compatible 
property, rather than being a generic/wildcard description:

- compatible
     Array of strings.
     One of:
   - "nvidia,tegra186-hsp"

If/when this binding supports other SoCs in the future, we'll add more 
entries into that list.

> +- 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.).
     Users of this binding MUST look up entries in the interrupts
     property by name, using this interrupts-names property to do so.
- interrupts
     Array of interrupt specifiers.
     Must contain one entry per entry in the interrupt-names property,
     in a matching order.

> +- 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.


The following properties were included in the internal patch:

                 nvidia,num-SM = <0x8>;
                 nvidia,num-AS = <0x2>;
                 nvidia,num-SS = <0x2>;
                 nvidia,num-DB = <0x7>;
                 nvidia,num-SI = <0x8>;

... yet aren't here. True the compatible value implies those values; was 
that why the properties were removed?

> +Example:
> +
> +hsp_top: hsp@3c00000 {
...
> +bpmp@d0000000 {
> +	...
> +	mboxes = <&hsp_top HSP_DB_MASTER_BPMP>;
> +	...
> +};

I'd suggest not including the bpmp node in the example, since it's not 
part of the HSP node. If you do, recall that bpmp has no reg property 
and hence the node name shouldn't include a unit address.

> diff --git a/include/dt-bindings/mailbox/tegra-hsp.h b/include/dt-bindings/mailbox/tegra-hsp.h

This file should probably be named tegra186-hsp, since I doubt the 
master ID values will be stable between chips.

> +/*
> + * This header provides constants for binding nvidia,tegra<chip>-hsp.

That should say "186" not "<chip>"

> +#ifndef _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
> +#define _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H

The two changes mentioned above would be consistent with that include 
guard's name including the text "186".

> +#define HSP_SHARED_MAILBOX		0
> +#define HSP_SHARED_SEMAPHORE		1
> +#define HSP_ARBITRATED_SEMAPHORE	2
> +#define HSP_DOORBELL			3

I think those should be removed, along with the nvidia,hsp-function 
property.

--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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
>
>> +NVIDIA Tegra Hardware Synchronization Primitives (HSP)
>> +
>> +The HSP modules are used 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 primitives, when operating
>> between
>> +two processors not in an SMP relationship.
>> +
>> +The features that HSP supported are shared mailboxes, shared semaphores,
>> +arbitrated semaphores and doorbells.
>> +
>> +Required properties:
>> +- name : Should be hsp
>> +- compatible : Should be "nvidia,tegra<chip>-hsp"
>
> I think this should explicitly list the value values of the compatible
> property, rather than being a generic/wildcard description:
>
> - compatible
>      Array of strings.
>      One of:
>    - "nvidia,tegra186-hsp"
>
> If/when this binding supports other SoCs in the future, we'll add more
> entries into that list.
>
>> +- 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:
Okay.
>
> - 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.

>      Users of this binding MUST look up entries in the interrupts
>      property by name, using this interrupts-names property to do so.
> - interrupts
>      Array of interrupt specifiers.
>      Must contain one entry per entry in the interrupt-names property,
>      in a matching order.
>
>> +- 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?

>
>
> The following properties were included in the internal patch:
>
>                  nvidia,num-SM = <0x8>;
>                  nvidia,num-AS = <0x2>;
>                  nvidia,num-SS = <0x2>;
>                  nvidia,num-DB = <0x7>;
>                  nvidia,num-SI = <0x8>;
>
> ... yet aren't here. True the compatible value implies those values; was
> that why the properties were removed?
Because these values are available in the HSP_INT_DIMENSIONING register, 
so remove them.

>
>> +Example:
>> +
>> +hsp_top: hsp@3c00000 {
> ...
>> +bpmp@d0000000 {
>> +    ...
>> +    mboxes = <&hsp_top HSP_DB_MASTER_BPMP>;
>> +    ...
>> +};
>
> I'd suggest not including the bpmp node in the example, since it's not
> part of the HSP node. If you do, recall that bpmp has no reg property
> and hence the node name shouldn't include a unit address.
Okay.
>
>> diff --git a/include/dt-bindings/mailbox/tegra-hsp.h
>> b/include/dt-bindings/mailbox/tegra-hsp.h
>
> This file should probably be named tegra186-hsp, since I doubt the
> master ID values will be stable between chips.
Yes, true. Will fix.
>
>> +/*
>> + * This header provides constants for binding nvidia,tegra<chip>-hsp.
>
> That should say "186" not "<chip>"
>
>> +#ifndef _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
>> +#define _DT_BINDINGS_MAILBOX_TEGRA186_HSP_H
>
> The two changes mentioned above would be consistent with that include
> guard's name including the text "186".
>
>> +#define HSP_SHARED_MAILBOX        0
>> +#define HSP_SHARED_SEMAPHORE        1
>> +#define HSP_ARBITRATED_SEMAPHORE    2
>> +#define HSP_DOORBELL            3
>
> I think those should be removed, along with the nvidia,hsp-function
> property.
>
Okay.

Thanks,
-Joseph
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/29/2016 03:08 AM, Stephen Warren wrote:
> 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:
snip.
>>
>> 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.

We have 6 HSP block in HW. FYI.

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

Thanks,
-Joseph
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/28/2016 11:56 PM, Joseph Lo wrote:
> On 06/29/2016 03:08 AM, Stephen Warren wrote:
>> 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:
> snip.
>>>
>>> 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.
>
> We have 6 HSP block in HW. FYI.

Yes, we have 6 /instances/ of the overall HSP block. Those should each 
have their own node, since they're entirely separate modules, all 
instances of the same configurable IP block.

Above, I was talking about the sub-blocks within each HSP instance, 
which should all be represented into a single node per instance, for a 
total of 6 DT nodes overall.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/29/2016 11:28 PM, Stephen Warren wrote:
> On 06/28/2016 11:56 PM, Joseph Lo wrote:
>> On 06/29/2016 03:08 AM, Stephen Warren wrote:
>>> 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:
>> snip.
>>>>
>>>> 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.
>>
>> We have 6 HSP block in HW. FYI.
>
> Yes, we have 6 /instances/ of the overall HSP block. Those should each
> have their own node, since they're entirely separate modules, all
> instances of the same configurable IP block.
>
> Above, I was talking about the sub-blocks within each HSP instance,
> which should all be represented into a single node per instance, for a
> total of 6 DT nodes overall.
Yes.

So, one thing still concerns me is that the binding and driver still 
can't work with multiple HSP sub-modules per HSP block. It only supports 
one HSP module per HSP block right how. Although, I said it matches the 
model that we are using in the downstream kernel. But I still concern if 
we need to enable and work with multiple HSP modules per HSP block at 
sometime in future, then the binding and driver need lots of change to 
achieve that. And the binding is not back-ward compatible obviously.

So I want to revise it again.

#mbox-cells: should be 2.

The mobxes property in the client node should contain the phandle of the 
HSP block, HSP sub-module ID and the specifier of the module.

Ex.
hsp_top0: hsp@1000 {
     ...
     #mbox-cells = <2>;
};

clientA {
     ....
     mboxes = <&hsp_top0 HSP_DOORBELL DB_MASTER_XXX>;
};

clientB {
     ...
     mboxes = <&hsp_top0 HSP_SHARED_MAILBOX SM_MASTER_XXX>;
};

Stephen, How do you think of this change?

Thanks,
-Joseph
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 06/30/2016 03:25 AM, Joseph Lo wrote:
> On 06/29/2016 11:28 PM, Stephen Warren wrote:
>> On 06/28/2016 11:56 PM, Joseph Lo wrote:
>>> On 06/29/2016 03:08 AM, Stephen Warren wrote:
>>>> 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:
>>> snip.
>>>>>
>>>>> 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.
>>>
>>> We have 6 HSP block in HW. FYI.
>>
>> Yes, we have 6 /instances/ of the overall HSP block. Those should each
>> have their own node, since they're entirely separate modules, all
>> instances of the same configurable IP block.
>>
>> Above, I was talking about the sub-blocks within each HSP instance,
>> which should all be represented into a single node per instance, for a
>> total of 6 DT nodes overall.
> Yes.
>
> So, one thing still concerns me is that the binding and driver still
> can't work with multiple HSP sub-modules per HSP block. It only supports
> one HSP module per HSP block right how.

The driver can be enhanced without affecting the DT binding, providing 
the binding is reasonably designed, as I believe it is.

I believe the existing binding can work fine for multiple HSP 
sub-modules, or at least be extended in a backwards-compatible way. 
Aside from the mailbox cells issue you mention below, is there any other 
reason you believe the binding can't be extended in a 
backwards-compatible way? Interrupts are already accessed solely by 
name, so we can add more later without issue. The node can become a 
provider for any other resource type besides mailboxes in a 
backwards-compatible way without issue.

> Although, I said it matches the
> model that we are using in the downstream kernel. But I still concern if
> we need to enable and work with multiple HSP modules per HSP block at
> sometime in future, then the binding and driver need lots of change to
> achieve that. And the binding is not back-ward compatible obviously.
>
> So I want to revise it again.
>
> #mbox-cells: should be 2.
>
> The mobxes property in the client node should contain the phandle of the
> HSP block, HSP sub-module ID and the specifier of the module.
>
> Ex.
> hsp_top0: hsp@1000 {
>      ...
>      #mbox-cells = <2>;
> };
>
> clientA {
>      ....
>      mboxes = <&hsp_top0 HSP_DOORBELL DB_MASTER_XXX>;
> };
>
> clientB {
>      ...
>      mboxes = <&hsp_top0 HSP_SHARED_MAILBOX SM_MASTER_XXX>;
> };
>
> Stephen, How do you think of this change?

Well, we could do that. Or, since we won't have 2^32 instances of 
doorbells, we could also have #mbox-cells=<1> as we do now, and encode 
mailbox IDs as "(type << 16) | id" where TEGRA186_HSP_MAILBOX_TYPE_DB is 
0. That would be backwards-compatible with no change to the binding. I 
think either way is fine. I have a slight preference for keeping 
#mbox-cells=<1> to avoid revising the U-Boot driver code I wrote, but I 
can deal with changing it if I have to.
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On 07/01/2016 12:02 AM, Stephen Warren wrote:
> On 06/30/2016 03:25 AM, Joseph Lo wrote:
>> On 06/29/2016 11:28 PM, Stephen Warren wrote:
>>> On 06/28/2016 11:56 PM, Joseph Lo wrote:
>>>> On 06/29/2016 03:08 AM, Stephen Warren wrote:
>>>>> 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:
>>>> snip.
>>>>>>
>>>>>> 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.
>>>>
>>>> We have 6 HSP block in HW. FYI.
>>>
>>> Yes, we have 6 /instances/ of the overall HSP block. Those should each
>>> have their own node, since they're entirely separate modules, all
>>> instances of the same configurable IP block.
>>>
>>> Above, I was talking about the sub-blocks within each HSP instance,
>>> which should all be represented into a single node per instance, for a
>>> total of 6 DT nodes overall.
>> Yes.
>>
>> So, one thing still concerns me is that the binding and driver still
>> can't work with multiple HSP sub-modules per HSP block. It only supports
>> one HSP module per HSP block right how.
>
> The driver can be enhanced without affecting the DT binding, providing
> the binding is reasonably designed, as I believe it is.
>
> I believe the existing binding can work fine for multiple HSP
> sub-modules, or at least be extended in a backwards-compatible way.
> Aside from the mailbox cells issue you mention below, is there any other
> reason you believe the binding can't be extended in a
> backwards-compatible way? Interrupts are already accessed solely by
> name, so we can add more later without issue. The node can become a
> provider for any other resource type besides mailboxes in a
> backwards-compatible way without issue.

Because the mbox client has no idea to know which hsb sub-module to bind 
with. However, the way you suggested below should solve my concern and 
back-ward compatible indeed.

>
>> Although, I said it matches the
>> model that we are using in the downstream kernel. But I still concern if
>> we need to enable and work with multiple HSP modules per HSP block at
>> sometime in future, then the binding and driver need lots of change to
>> achieve that. And the binding is not back-ward compatible obviously.
>>
>> So I want to revise it again.
>>
>> #mbox-cells: should be 2.
>>
>> The mobxes property in the client node should contain the phandle of the
>> HSP block, HSP sub-module ID and the specifier of the module.
>>
>> Ex.
>> hsp_top0: hsp@1000 {
>>      ...
>>      #mbox-cells = <2>;
>> };
>>
>> clientA {
>>      ....
>>      mboxes = <&hsp_top0 HSP_DOORBELL DB_MASTER_XXX>;
>> };
>>
>> clientB {
>>      ...
>>      mboxes = <&hsp_top0 HSP_SHARED_MAILBOX SM_MASTER_XXX>;
>> };
>>
>> Stephen, How do you think of this change?
>
> Well, we could do that. Or, since we won't have 2^32 instances of
> doorbells, we could also have #mbox-cells=<1> as we do now, and encode
> mailbox IDs as "(type << 16) | id" where TEGRA186_HSP_MAILBOX_TYPE_DB is
> 0. That would be backwards-compatible with no change to the binding. I
> think either way is fine. I have a slight preference for keeping
> #mbox-cells=<1> to avoid revising the U-Boot driver code I wrote, but I
> can deal with changing it if I have to.

Ah, yes. I think the U-Boot doesn't need to deal with the multiple HSP 
sub-module supporting issue, and the binding I purposed was more 
complicate for that. The idea with "#mbox-cells=<1>" and 
"mboxes=<(type<<16)|id>" is fine with me. I will revise the hsp driver 
for this.

Thanks,
-Joseph
--
To unsubscribe from this list: send the line "unsubscribe linux-tegra" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html