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

[Benjamin Herrenschmidt]

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.

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