On 07/18/2016 07:52 PM, Rob Herring wrote:

On Mon, Jul 18, 2016 at 6:31 PM, Stephen Warren [off-list ref] wrote:

quoted

On 07/18/2016 03:00 PM, Rob Herring wrote:

quoted

On Mon, Jul 18, 2016 at 11:16 AM, Stephen Warren [off-list ref]
wrote:

quoted

On 07/16/2016 03:07 PM, Rob Herring wrote:

quoted

On Thu, Jul 07, 2016 at 01:37:11PM -0600, Stephen Warren wrote:

quoted

From: Stephen Warren <redacted>

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 <redacted>
---
    .../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:

quoted

         bpmp: bpmp {
                 compatible = "nvidia,tegra186-bpmp";

...

quoted

                 #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?

quoted

                         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.

quoted

quoted

                                 #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:-(