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

[Rongrong Zou]

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

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

Regards,
Rongrong