From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754177AbcANDkV (ORCPT ); Wed, 13 Jan 2016 22:40:21 -0500 Received: from gate.crashing.org ([63.228.1.57]:60649 "EHLO gate.crashing.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752848AbcANDjt (ORCPT ); Wed, 13 Jan 2016 22:39:49 -0500 Message-ID: <1452742757.31558.8.camel@kernel.crashing.org> Subject: Re: [PATCH v1 3/3] ARM64 LPC: update binding doc From: Benjamin Herrenschmidt To: Rongrong Zou , 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 Date: Thu, 14 Jan 2016 14:39:17 +1100 In-Reply-To: <569701E3.2090307@gmail.com> References: <1451396032-23708-1-git-send-email-zourongrong@gmail.com> <1451396032-23708-4-git-send-email-zourongrong@gmail.com> <1452727756.2403.47.camel@kernel.crashing.org> <569701E3.2090307@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.18.3 (3.18.3-1.fc23) Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > > > --- > > >   .../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.