Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1308909 > unrolled thread

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

Started byBenjamin Herrenschmidt <benh@kernel.crashing.org>
First post2016-01-14 01:20 +0100
Last post2016-01-14 14:20 +0100
Articles 6 — 2 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 01:20 +0100
    Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 03:10 +0100
      Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 04:50 +0100
        Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 05:50 +0100
          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-14 12:30 +0100
            Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-14 14:20 +0100

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

FromBenjamin Herrenschmidt <benh@kernel.crashing.org>
Date2016-01-14 01:20 +0100
SubjectRe: [PATCH v1 3/3] ARM64 LPC: update binding doc
Message-ID<qQDNU-7lu-3@gated-at.bofh.it>
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".

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

> +        };

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 ?

Cheers,
Ben.

[toc] | [next] | [standalone]

#1308963

FromRongrong Zou <zourongrong@gmail.com>
Date2016-01-14 03:10 +0100
Message-ID<qQFwl-bb-9@gated-at.bofh.it>
In reply to#1308909
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

[toc] | [prev] | [next] | [standalone]

#1308982

FromBenjamin Herrenschmidt <benh@kernel.crashing.org>
Date2016-01-14 04:50 +0100
Message-ID<qQH57-1bp-1@gated-at.bofh.it>
In reply to#1308963
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@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.

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.

[toc] | [prev] | [next] | [standalone]

#1308989

FromRongrong Zou <zourongrong@gmail.com>
Date2016-01-14 05:50 +0100
Message-ID<qQI1b-1LZ-3@gated-at.bofh.it>
In reply to#1308982
On 2016/1/14 11:39, Benjamin Herrenschmidt wrote:
> 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@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.
>
> 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.

It is not directly MMIO mapped actually.

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

Yes, it is a vendor-specific LPC controller. The compatible property here
  is too general :)

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

The big problem is we do not want the "ranges" property, but we can't get
resource if the property is absent, you could see discussion at
https://lkml.org/lkml/2016/1/11/631.

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

[toc] | [prev] | [next] | [standalone]

#1309191

FromBenjamin Herrenschmidt <benh@kernel.crashing.org>
Date2016-01-14 12:30 +0100
Message-ID<qQOgi-6eV-5@gated-at.bofh.it>
In reply to#1308989
On Thu, 2016-01-14 at 12:42 +0800, Rongrong Zou wrote:

> > 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.
> 
> It is not directly MMIO mapped actually.

I know yours is not. But some are. My point is that we should have a
binding that is either completely specific to your ARM64 LPC or we
should have a generic LPC binding with provisions for implementation
specific stuff such as ARM64 or POWER8 which are both not MMIO mapped.

I go for the latter.

So "ranges" if you are mapped, otherwise "reg", and in the latter case,
the compatible property should be much more specific like it is for P8,

  .../...

> The big problem is we do not want the "ranges" property, but we can't
> get resource if the property is absent, you could see discussion at
> https://lkml.org/lkml/2016/1/11/631.

That's fixable. I missed the discussion but I'll have a look tomorrow.

Cheers
Ben.

[toc] | [prev] | [next] | [standalone]

#1309269

FromRongrong Zou <zourongrong@gmail.com>
Date2016-01-14 14:20 +0100
Message-ID<qQPYK-7yP-13@gated-at.bofh.it>
In reply to#1309191
On 2016/1/14 19:25, Benjamin Herrenschmidt wrote:
> On Thu, 2016-01-14 at 12:42 +0800, Rongrong Zou wrote:
>
>>> 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.
>>
>> It is not directly MMIO mapped actually.
>
> I know yours is not. But some are. My point is that we should have a
> binding that is either completely specific to your ARM64 LPC or we
> should have a generic LPC binding with provisions for implementation
> specific stuff such as ARM64 or POWER8 which are both not MMIO mapped.

Yes, that is what I want, both mapped and unmapped IO should
better be supported.

>
> I go for the latter.
>
> So "ranges" if you are mapped, otherwise "reg", and in the latter case,
> the compatible property should be much more specific like it is for P8,
>
>    .../...
>
>> The big problem is we do not want the "ranges" property, but we can't
>> get resource if the property is absent, you could see discussion at
>> https://lkml.org/lkml/2016/1/11/631.
>
> That's fixable. I missed the discussion but I'll have a look tomorrow.
>
> Cheers
> Ben.
>

Regars,
Rongrong

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web