Re: [PATCH v1 3/3] ARM64 LPC: update binding doc

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Thu, 2016-01-14 at 10:03 +0800, Rongrong Zou wrote:
> On 2016/1/14 7:29, Benjamin Herrenschmidt wrote:
> > On Tue, 2015-12-29 at 21:33 +0800, Rongrong Zou wrote:
> > > Signed-off-by: Rongrong Zou <zourongrong@gmail.com>
> > > ---
> > >   .../devicetree/bindings/arm64/low-pin-count.txt      | 20
> > > ++++++++++++++++++++
> > >   1 file changed, 20 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/arm64/low-
> > > pin-count.txt
> > > 
> > > diff --git a/Documentation/devicetree/bindings/arm64/low-pin-
> > > count.txt b/Documentation/devicetree/bindings/arm64/low-pin-
> > > count.txt
> > > new file mode 100644
> > > index 0000000..215f2c4
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm64/low-pin-count.txt
> > > @@ -0,0 +1,20 @@
> > > +Low Pin Count bus driver
> > > +
> > > +Usually LPC controller is part of PCI host bridge, so the legacy
> > > ISA
> > > +port locate on LPC bus can be accessed directly. But some SoC
> > > have
> > > +independent LPC controller, and we can access the legacy port by
> > > specifying
> > > +LPC address cycle. Thus, LPC driver is introduced.
> > > +
> > > +Required properties:
> > > +- compatible: "low-pin-count"
> > 
> > I'm not sure about the above. I'd rather just make it "isa" or
> > maybe
> > isa-lpc. The LPC bus is fundamentally an ISA bus with the 3 cycle
> > types of ISA etc... I would also allow the node to be named "isa".
> 
> I had modified its name to "isa@****", otherwise, the kernel do not
> understand its children devices are on ISA bus.

Right, and the "compatible" property should be something like the
specific implementation of the LPC bridge. For example, ibm,power8-lpc
in my case. Not something generic.

Maybe we could allow for a generic one if the LPC is directly MMIO
mapped via the ranges property.

> > > +- reg: specifies low pin count address range
> > > +
> > > +
> > > +Example:
> > > +
> > > +        lpc_0: lpc@a01b0000 {
> > > +		#address-cells = <1>;
> > > +		#size-cells = <1>;
> > 
> > As discussed earlier, address-cells should be 2 with the first cell
> > indicating the address space type (0 = mem, 1 = IO, possibly 2 =
> > firmware but that remains somewhat TBD).
> > 
> > > +                compatible = "low-pin-count";
> > > +                reg = <0x0 0xa01b0000 0x0 0x10000>;
> > 
> > And also as discussed, this is the business of the "ranges"
> > property so
> > that children devices can be properly expressed.
> > 
> 
> As discussed before,
>  > A missing ranges property means that there is no translation,
> while an
>  > empty ranges means a 1:1 translation to the parent bus.
>  > We really want the former here, as I/O port addresses are not
> mapped into
>  > the MMIO space of the parent bus.

Right ok but then it's not a generic binding for "low-pin-count". It's
a specific binding for that specific vendor LPC controller. In that
case yes, reg contains the registers for it but your compatible
property should be more precise.

For a generic binding of LPC, you'd want a ranges property though.

> > > +        };
> > 
> > Also, this being a bus binding, it should describe the format for
> > children (for example, PNP related properties).
> > 
> > That leads to the obvious question: Why not just reference the
> > existing
> > Open Firmware ISA binding ?
> 
> Unfortunately, I found all these bindings are based on memory-mapped
> I/O.

You should still refer to it for the definition of the properties of
children of the LPC.

Basically, a generic LPC bus is an ISA bus and honors the ISA binding.
A specific LPC controller provides a method of generating the ISA bus
cycles. If it's memory mapped, it can just stay generic and use a
ranges property. If not, it's more specific, thus has no range and has
a reg and a *precise* compatible property.

But that shouldn't affect the definition of the children nodes.


> Such as below binding I found in
> arch/x86/platform/ce4100/falconfalls.dts
>                  pci@3fc {
>                          #address-cells = <3>;
>                          #size-cells = <2>;
>                          compatible = "intel,ce4100-pci", "pci";
>                          device_type = "pci";
>                          bus-range = <0 0>;
>                          ranges = <0x2000000 0 0xbffff000 0xbffff000
> 0 0x1000
>                                    0x2000000 0 0xdffe0000 0xdffe0000
> 0 0x1000
>                                    0x0000000 0
> 0x0        0x0        0 0x100>;
> 
>                          isa@1f,0 {
>                                  #address-cells = <2>;
>                                  #size-cells = <1>;
>                                  compatible = "isa";
>                                  reg = <0xf800 0x0 0x0 0x0 0x0>;
>                                  ranges = <1 0 0 0 0 0x100>;
> 
>                                  rtc@70 {
>                                          compatible = "intel,ce4100-
> rtc", "motorola,mc146818";
>                                          interrupts = <8 3>;
>                                          interrupt-parent =
> <&ioapic1>;
>                                          ctrl-reg = <2>;
>                                          freq-reg = <0x26>;
>                                          reg = <1 0x70 2>;
>                                  };
>                          };
>                  };

Yes that's a generic one.

Cheers,
Ben.