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

From: Rongrong Zou <zourongrong@gmail.com>
To: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
	arnd@arndb.de, catalin.marinas@arm.com, will.deacon@arm.com
Cc: lijianhua@huawei.com, lixiancai@huawei.com, linuxarm@huawei.com,
	linux-kernel@vger.kernel.org, minyard@acm.org,
	gregkh@linuxfoundation.org
Subject: Re: [PATCH v1 3/3] ARM64 LPC: update binding doc
Date: Thu, 14 Jan 2016 10:03:15 +0800	[thread overview]
Message-ID: <569701E3.2090307@gmail.com> (raw)
In-Reply-To: <1452727756.2403.47.camel@kernel.crashing.org>

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