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

Rongrong Zou
2016-01-14T02:03:27Z

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