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

Stephen Warren
2016-07-18T23:31:38Z

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?