lkml.org 
[lkml]   [2016]   [Jun]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 01/10] Documentation: dt-bindings: mailbox: tegra: Add binding for HSP mailbox
From
Date
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

\
 
 \ /
  Last update: 2016-06-28 12:01    [W:0.053 / U:1.616 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site