On Mon, Jul 18, 2016 at 6:31 PM, Stephen Warren <swarren@xxxxxxxxxxxxx> wrote:
> On 07/18/2016 03:00 PM, Rob Herring wrote:
>>
>> On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren <swarren@xxxxxxxxxxxxx>
>> 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@xxxxxxxxxx>
>>>>>
>>>>> 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@xxxxxxxxxx>
>>>>> ---
>>>>> .../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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
