[PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Stephen Warren]

From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
HW devices, such as the I2C controller for the power management I2C bus.
Software running on other CPUs must perform IPC to the BPMP in order to
execute transactions on that I2C bus. This binding describes an I2C bus
that is accessed in such a fashion.

Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35 ++++++++++++++++++++++
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt

diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
new file mode 100644
index 000000000000..eb9f70723ab7
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
@@ -0,0 +1,35 @@
+NVIDIA Tegra186 BPMP I2C controller
+
+In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
+devices, such as the I2C controller for the power management I2C bus. Software
+running on other CPUs must perform IPC to the BPMP in order to execute
+transactions on that I2C bus. This binding describes an I2C bus that is
+accessed in such a fashion.
+
+Required properties:
+- compatible:
+    Array of strings.
+    One of:
+    - "nvidia,tegra186-bpmp-i2c".
+- address-cells: Address cells for I2C device address.
+    Single-cell integer.
+    Must be <1>.
+- size-cells:
+    Single-cell integer.
+    Must be <0>.
+- nvidia,bpmp:
+    The phandle to the BPMP device.
+- nvidia,bpmp-bus-id:
+    Single-cell integer.
+    Indicates the I2C bus number this DT node represent, as defined by the
+    BPMP firmware.
+
+Example:
+
+	bpmp-i2c {
+		compatible = "nvidia,tegra186-bpmp-i2c";
+		#address-cells = <1>;
+		#size-cells = <0>;
+		nvidia,bpmp = <&bpmp>;
+		nvidia,bpmp-bus-id = <5>;
+	};
-- 
2.9.0

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Rob Herring]

On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> 
> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
> HW devices, such as the I2C controller for the power management I2C bus.
> Software running on other CPUs must perform IPC to the BPMP in order to
> execute transactions on that I2C bus. This binding describes an I2C bus
> that is accessed in such a fashion.
> 
> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35 ++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> new file mode 100644
> index 000000000000..eb9f70723ab7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
> @@ -0,0 +1,35 @@
> +NVIDIA Tegra186 BPMP I2C controller
> +
> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
> +devices, such as the I2C controller for the power management I2C bus. Software
> +running on other CPUs must perform IPC to the BPMP in order to execute
> +transactions on that I2C bus. This binding describes an I2C bus that is
> +accessed in such a fashion.
> +
> +Required properties:
> +- compatible:
> +    Array of strings.
> +    One of:
> +    - "nvidia,tegra186-bpmp-i2c".
> +- address-cells: Address cells for I2C device address.
> +    Single-cell integer.
> +    Must be <1>.
> +- size-cells:
> +    Single-cell integer.
> +    Must be <0>.
> +- nvidia,bpmp:
> +    The phandle to the BPMP device.

Any reason to not make this a sub-node of the BPMP device?

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Stephen Warren]

On 07/16/2016 03:07 PM, Rob Herring wrote:
> On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>
>> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>> HW devices, such as the I2C controller for the power management I2C bus.
>> Software running on other CPUs must perform IPC to the BPMP in order to
>> execute transactions on that I2C bus. This binding describes an I2C bus
>> that is accessed in such a fashion.
>>
>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>   .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35 ++++++++++++++++++++++
>>   1 file changed, 35 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>
>> diff --git a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>> new file mode 100644
>> index 000000000000..eb9f70723ab7
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>> @@ -0,0 +1,35 @@
>> +NVIDIA Tegra186 BPMP I2C controller
>> +
>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain HW
>> +devices, such as the I2C controller for the power management I2C bus. Software
>> +running on other CPUs must perform IPC to the BPMP in order to execute
>> +transactions on that I2C bus. This binding describes an I2C bus that is
>> +accessed in such a fashion.
>> +
>> +Required properties:
>> +- compatible:
>> +    Array of strings.
>> +    One of:
>> +    - "nvidia,tegra186-bpmp-i2c".
>> +- address-cells: Address cells for I2C device address.
>> +    Single-cell integer.
>> +    Must be <1>.
>> +- size-cells:
>> +    Single-cell integer.
>> +    Must be <0>.
>> +- nvidia,bpmp:
>> +    The phandle to the BPMP device.
>
> Any reason to not make this a sub-node of the BPMP device?

That would be possible too.

My thought process was along the lines of: The system has an I2C bus, 
which deserves a DT node. That fact seemed more important than the 
access mechanism; the fact it's accessed via BPMP rather than direct 
register access felt a bit more like an implementation detail. Still, I 
suppose we could flip it around and store the node underneath the BPMP 
if you want; let me know.

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Rob Herring]

On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/16/2016 03:07 PM, Rob Herring wrote:
>>
>> On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
>>>
>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>
>>> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>>> HW devices, such as the I2C controller for the power management I2C bus.
>>> Software running on other CPUs must perform IPC to the BPMP in order to
>>> execute transactions on that I2C bus. This binding describes an I2C bus
>>> that is accessed in such a fashion.
>>>
>>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>> ---
>>>   .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35
>>> ++++++++++++++++++++++
>>>   1 file changed, 35 insertions(+)
>>>   create mode 100644
>>> Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>> new file mode 100644
>>> index 000000000000..eb9f70723ab7
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>> @@ -0,0 +1,35 @@
>>> +NVIDIA Tegra186 BPMP I2C controller
>>> +
>>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>>> HW
>>> +devices, such as the I2C controller for the power management I2C bus.
>>> Software
>>> +running on other CPUs must perform IPC to the BPMP in order to execute
>>> +transactions on that I2C bus. This binding describes an I2C bus that is
>>> +accessed in such a fashion.
>>> +
>>> +Required properties:
>>> +- compatible:
>>> +    Array of strings.
>>> +    One of:
>>> +    - "nvidia,tegra186-bpmp-i2c".
>>> +- address-cells: Address cells for I2C device address.
>>> +    Single-cell integer.
>>> +    Must be <1>.
>>> +- size-cells:
>>> +    Single-cell integer.
>>> +    Must be <0>.
>>> +- nvidia,bpmp:
>>> +    The phandle to the BPMP device.
>>
>>
>> Any reason to not make this a sub-node of the BPMP device?
>
>
> That would be possible too.
>
> My thought process was along the lines of: The system has an I2C bus, which
> deserves a DT node. That fact seemed more important than the access
> mechanism; the fact it's accessed via BPMP rather than direct register
> access felt a bit more like an implementation detail. Still, I suppose we
> could flip it around and store the node underneath the BPMP if you want; let
> me know.

I prefer to utilize the hierarchy unless there are reasons it can't be
which doesn't seem to be the case here.

Rob

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Stephen Warren]

On 07/18/2016 03:00 PM, Rob Herring wrote:
> On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/16/2016 03:07 PM, Rob Herring wrote:
>>>
>>> On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
>>>>
>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>
>>>> In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>>>> HW devices, such as the I2C controller for the power management I2C bus.
>>>> Software running on other CPUs must perform IPC to the BPMP in order to
>>>> execute transactions on that I2C bus. This binding describes an I2C bus
>>>> that is accessed in such a fashion.
>>>>
>>>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>> ---
>>>>    .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35
>>>> ++++++++++++++++++++++
>>>>    1 file changed, 35 insertions(+)
>>>>    create mode 100644
>>>> Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>> new file mode 100644
>>>> index 000000000000..eb9f70723ab7
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>> @@ -0,0 +1,35 @@
>>>> +NVIDIA Tegra186 BPMP I2C controller
>>>> +
>>>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns certain
>>>> HW
>>>> +devices, such as the I2C controller for the power management I2C bus.
>>>> Software
>>>> +running on other CPUs must perform IPC to the BPMP in order to execute
>>>> +transactions on that I2C bus. This binding describes an I2C bus that is
>>>> +accessed in such a fashion.
>>>> +
>>>> +Required properties:
>>>> +- compatible:
>>>> +    Array of strings.
>>>> +    One of:
>>>> +    - "nvidia,tegra186-bpmp-i2c".
>>>> +- address-cells: Address cells for I2C device address.
>>>> +    Single-cell integer.
>>>> +    Must be <1>.
>>>> +- size-cells:
>>>> +    Single-cell integer.
>>>> +    Must be <0>.
>>>> +- nvidia,bpmp:
>>>> +    The phandle to the BPMP device.
>>>
>>>
>>> Any reason to not make this a sub-node of the BPMP device?
>>
>>
>> That would be possible too.
>>
>> My thought process was along the lines of: The system has an I2C bus, which
>> deserves a DT node. That fact seemed more important than the access
>> mechanism; the fact it's accessed via BPMP rather than direct register
>> access felt a bit more like an implementation detail. Still, I suppose we
>> could flip it around and store the node underneath the BPMP if you want; let
>> me know.
>
> I prefer to utilize the hierarchy unless there are reasons it can't be
> which doesn't seem to be the case here.

OK, how does the following look? If it seems OK, I'll write up the 
binding changes:

> 	bpmp: bpmp {
> 		compatible = "nvidia,tegra186-bpmp";
...
> 		#clock-cells = <1>;
> 		#power-domain-cells = <1>;
> 		#reset-cells = <1>;
>
> 		sub-devices {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
// There actually aren't any registers, so perhaps those two aren't
// needed. Perhaps those two properties should be dropped?
>
> 			bpmp-i2c {
> 				compatible = "nvidia,tegra186-bpmp-i2c";
> 				nvidia,bpmp-bus-id = <5>;
> 				#address-cells = <1>;
> 				#size-cells = <0>;
> 				status = "disabled";
> 			};
> 		};
> 	};

Rationale:

There may be more than 1 I2C bus, so we need a structure that can house 
n of them.

It's plausible the BPMP could support other sub-device-types in the 
future that each need their own node, so it's useful to have a scheme 
that supports arbitrary sub-devices, each using compatible values to 
define what they are.

We house the sub-devices in an explicit "sub-devices" node, rather than 
directly as children of the top-level bpmp node, so that the BPMP can 
separate any sub-nodes it uses for its own top-level configuration from 
sub-nodes that represent child-/sub-devices. This keeps things clean, 
and allows additions to the bpmp binding without complicating how 
sub-devices are enumerated. This is rather like the recent optinonal 
i2c-bus child node of I2C controllers.

Sound good?

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Rob Herring]

On Mon, Jul 18, 2016 at 6:31 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
> On 07/18/2016 03:00 PM, Rob Herring wrote:
>>
>> On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>> wrote:
>>>
>>> On 07/16/2016 03:07 PM, Rob Herring wrote:
>>>>
>>>>
>>>> On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
>>>>>
>>>>>
>>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>
>>>>> In Tegra186, the BPMP (Boot and Power Management Processor) owns
>>>>> certain
>>>>> HW devices, such as the I2C controller for the power management I2C
>>>>> bus.
>>>>> Software running on other CPUs must perform IPC to the BPMP in order to
>>>>> execute transactions on that I2C bus. This binding describes an I2C bus
>>>>> that is accessed in such a fashion.
>>>>>
>>>>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>> ---
>>>>>    .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35
>>>>> ++++++++++++++++++++++
>>>>>    1 file changed, 35 insertions(+)
>>>>>    create mode 100644
>>>>> Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>>
>>>>> diff --git
>>>>> a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>> new file mode 100644
>>>>> index 000000000000..eb9f70723ab7
>>>>> --- /dev/null
>>>>> +++
>>>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>> @@ -0,0 +1,35 @@
>>>>> +NVIDIA Tegra186 BPMP I2C controller
>>>>> +
>>>>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns
>>>>> certain
>>>>> HW
>>>>> +devices, such as the I2C controller for the power management I2C bus.
>>>>> Software
>>>>> +running on other CPUs must perform IPC to the BPMP in order to execute
>>>>> +transactions on that I2C bus. This binding describes an I2C bus that
>>>>> is
>>>>> +accessed in such a fashion.
>>>>> +
>>>>> +Required properties:
>>>>> +- compatible:
>>>>> +    Array of strings.
>>>>> +    One of:
>>>>> +    - "nvidia,tegra186-bpmp-i2c".
>>>>> +- address-cells: Address cells for I2C device address.
>>>>> +    Single-cell integer.
>>>>> +    Must be <1>.
>>>>> +- size-cells:
>>>>> +    Single-cell integer.
>>>>> +    Must be <0>.
>>>>> +- nvidia,bpmp:
>>>>> +    The phandle to the BPMP device.
>>>>
>>>>
>>>>
>>>> Any reason to not make this a sub-node of the BPMP device?
>>>
>>>
>>>
>>> That would be possible too.
>>>
>>> My thought process was along the lines of: The system has an I2C bus,
>>> which
>>> deserves a DT node. That fact seemed more important than the access
>>> mechanism; the fact it's accessed via BPMP rather than direct register
>>> access felt a bit more like an implementation detail. Still, I suppose we
>>> could flip it around and store the node underneath the BPMP if you want;
>>> let
>>> me know.
>>
>>
>> I prefer to utilize the hierarchy unless there are reasons it can't be
>> which doesn't seem to be the case here.
>
>
> OK, how does the following look? If it seems OK, I'll write up the binding
> changes:
>
>>         bpmp: bpmp {
>>                 compatible = "nvidia,tegra186-bpmp";
>
> ...
>>
>>                 #clock-cells = <1>;
>>                 #power-domain-cells = <1>;
>>                 #reset-cells = <1>;
>>
>>                 sub-devices {
>>                         #address-cells = <1>;
>>                         #size-cells = <0>;
>
> // There actually aren't any registers, so perhaps those two aren't
> // needed. Perhaps those two properties should be dropped?
>>
>>
>>                         bpmp-i2c {
>>                                 compatible = "nvidia,tegra186-bpmp-i2c";
>>                                 nvidia,bpmp-bus-id = <5>;

Given you need this bus number, what about doing "i2c@5" for the node
and using reg property. After all, that is how you "address" the bus.

>>                                 #address-cells = <1>;
>>                                 #size-cells = <0>;
>>                                 status = "disabled";
>>                         };
>>                 };
>>         };
>
>
> Rationale:
>
> There may be more than 1 I2C bus, so we need a structure that can house n of
> them.
>
> It's plausible the BPMP could support other sub-device-types in the future
> that each need their own node, so it's useful to have a scheme that supports
> arbitrary sub-devices, each using compatible values to define what they are.
>
> We house the sub-devices in an explicit "sub-devices" node, rather than
> directly as children of the top-level bpmp node, so that the BPMP can
> separate any sub-nodes it uses for its own top-level configuration from
> sub-nodes that represent child-/sub-devices. This keeps things clean, and
> allows additions to the bpmp binding without complicating how sub-devices
> are enumerated. This is rather like the recent optinonal i2c-bus child node
> of I2C controllers.
>
> Sound good?

I'm not crazy about having the extra layer, but I'm not going to
object. I'd argue that i2c@X is unique enough.

Rob

Re: [PATCH] ARM: tegra: add DT binding for Tegra186 BPMP I2C

[Stephen Warren]

On 07/18/2016 07:52 PM, Rob Herring wrote:
> On Mon, Jul 18, 2016 at 6:31 PM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org> wrote:
>> On 07/18/2016 03:00 PM, Rob Herring wrote:
>>>
>>> On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren <swarren-3lzwWm7+Weoh9ZMKESR00Q@public.gmane.org>
>>> wrote:
>>>>
>>>> On 07/16/2016 03:07 PM, Rob Herring wrote:
>>>>>
>>>>>
>>>>> On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:
>>>>>>
>>>>>>
>>>>>> From: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>>
>>>>>> In Tegra186, the BPMP (Boot and Power Management Processor) owns
>>>>>> certain
>>>>>> HW devices, such as the I2C controller for the power management I2C
>>>>>> bus.
>>>>>> Software running on other CPUs must perform IPC to the BPMP in order to
>>>>>> execute transactions on that I2C bus. This binding describes an I2C bus
>>>>>> that is accessed in such a fashion.
>>>>>>
>>>>>> Signed-off-by: Stephen Warren <swarren-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>>>>>> ---
>>>>>>     .../bindings/i2c/nvidia,tegra186-bpmp-i2c.txt      | 35
>>>>>> ++++++++++++++++++++++
>>>>>>     1 file changed, 35 insertions(+)
>>>>>>     create mode 100644
>>>>>> Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>>>
>>>>>> diff --git
>>>>>> a/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>>> new file mode 100644
>>>>>> index 000000000000..eb9f70723ab7
>>>>>> --- /dev/null
>>>>>> +++
>>>>>> b/Documentation/devicetree/bindings/i2c/nvidia,tegra186-bpmp-i2c.txt
>>>>>> @@ -0,0 +1,35 @@
>>>>>> +NVIDIA Tegra186 BPMP I2C controller
>>>>>> +
>>>>>> +In Tegra186, the BPMP (Boot and Power Management Processor) owns
>>>>>> certain
>>>>>> HW
>>>>>> +devices, such as the I2C controller for the power management I2C bus.
>>>>>> Software
>>>>>> +running on other CPUs must perform IPC to the BPMP in order to execute
>>>>>> +transactions on that I2C bus. This binding describes an I2C bus that
>>>>>> is
>>>>>> +accessed in such a fashion.
>>>>>> +
>>>>>> +Required properties:
>>>>>> +- compatible:
>>>>>> +    Array of strings.
>>>>>> +    One of:
>>>>>> +    - "nvidia,tegra186-bpmp-i2c".
>>>>>> +- address-cells: Address cells for I2C device address.
>>>>>> +    Single-cell integer.
>>>>>> +    Must be <1>.
>>>>>> +- size-cells:
>>>>>> +    Single-cell integer.
>>>>>> +    Must be <0>.
>>>>>> +- nvidia,bpmp:
>>>>>> +    The phandle to the BPMP device.
>>>>>
>>>>>
>>>>>
>>>>> Any reason to not make this a sub-node of the BPMP device?
>>>>
>>>>
>>>>
>>>> That would be possible too.
>>>>
>>>> My thought process was along the lines of: The system has an I2C bus,
>>>> which
>>>> deserves a DT node. That fact seemed more important than the access
>>>> mechanism; the fact it's accessed via BPMP rather than direct register
>>>> access felt a bit more like an implementation detail. Still, I suppose we
>>>> could flip it around and store the node underneath the BPMP if you want;
>>>> let
>>>> me know.
>>>
>>>
>>> I prefer to utilize the hierarchy unless there are reasons it can't be
>>> which doesn't seem to be the case here.
>>
>>
>> OK, how does the following look? If it seems OK, I'll write up the binding
>> changes:
>>
>>>          bpmp: bpmp {
>>>                  compatible = "nvidia,tegra186-bpmp";
>>
>> ...
>>>
>>>                  #clock-cells = <1>;
>>>                  #power-domain-cells = <1>;
>>>                  #reset-cells = <1>;
>>>
>>>                  sub-devices {
>>>                          #address-cells = <1>;
>>>                          #size-cells = <0>;
>>
>> // There actually aren't any registers, so perhaps those two aren't
>> // needed. Perhaps those two properties should be dropped?
>>>
>>>
>>>                          bpmp-i2c {
>>>                                  compatible = "nvidia,tegra186-bpmp-i2c";
>>>                                  nvidia,bpmp-bus-id = <5>;
>
> Given you need this bus number, what about doing "i2c@5" for the node
> and using reg property. After all, that is how you "address" the bus.

That implies we need a separate address space for every type of 
sub-device, instead of "sub-devices", we might need "i2c-buses", 
"spi-buses", "foo-bar-resources", etc., each with n child nodes.

>>>                                  #address-cells = <1>;
>>>                                  #size-cells = <0>;
>>>                                  status = "disabled";
>>>                          };
>>>                  };
>>>          };
>>
>>
>> Rationale:
>>
>> There may be more than 1 I2C bus, so we need a structure that can house n of
>> them.
>>
>> It's plausible the BPMP could support other sub-device-types in the future
>> that each need their own node, so it's useful to have a scheme that supports
>> arbitrary sub-devices, each using compatible values to define what they are.
>>
>> We house the sub-devices in an explicit "sub-devices" node, rather than
>> directly as children of the top-level bpmp node, so that the BPMP can
>> separate any sub-nodes it uses for its own top-level configuration from
>> sub-nodes that represent child-/sub-devices. This keeps things clean, and
>> allows additions to the bpmp binding without complicating how sub-devices
>> are enumerated. This is rather like the recent optinonal i2c-bus child node
>> of I2C controllers.
>>
>> Sound good?
>
> I'm not crazy about having the extra layer, but I'm not going to
> object. I'd argue that i2c@X is unique enough.

How would the core BPMP driver know which child nodes to process (and 
which to ignore) if they were direct children of the main bpmp node? I 
really don't like the idea of mixing (at the same level) sub-nodes that 
are pure configuration for the BPMP itself and sub-nodes that describe 
other devices to instantiate. I suppose the driver could just enumerate 
every child node and switch based on the name "i2c" vs. "spi" vs "foo", 
but then the enumeration algorithm is quite BPMP-specific and non-generic:-(