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

[Rongrong Zou]

On 2016/1/14 11:39, Benjamin Herrenschmidt wrote:
> 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@xxxxxxxxx>
> > > > ---
> > > >    .../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.

It is not directly MMIO mapped actually.

> > > > +- 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.

Yes, it is a vendor-specific LPC controller. The compatible property here
 is too general :)

> 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.

The big problem is we do not want the "ranges" property, but we can't get
resource if the property is absent, you could see discussion at
https://lkml.org/lkml/2016/1/11/631.

> > 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.