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

Groups > linux.kernel > #1300292 > unrolled thread

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

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 Rongrong Zou <zourongrong@huawei.com> - 2016-01-03 13:30 +0100
    Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-04 12:20 +0100
      Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-04 17:00 +0100
        Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-04 17:40 +0100
          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-05 13:10 +0100
            Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-05 13:30 +0100
              Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-06 14:40 +0100
                Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-07 04:40 +0100
              Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rolland Chau <zourongrong@gmail.com> - 2016-01-10 10:40 +0100
                Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-10 14:40 +0100
      Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-11 17:20 +0100
        Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-12 03:40 +0100
          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 10:10 +0100
            Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 10:30 +0100
              Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 11:20 +0100
                Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 12:10 +0100
                  Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 12:30 +0100
                    Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@huawei.com> - 2016-01-12 13:00 +0100
                      Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-12 16:20 +0100
                        Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 00:00 +0100
                          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Benjamin Herrenschmidt <benh@kernel.crashing.org> - 2016-01-13 07:20 +0100
                            Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-13 07:40 +0100
                              Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 10:30 +0100
                          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 11:20 +0100
                            Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:40 +0100
                          Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:20 +0100
                Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 00:00 +0100
                  Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 11:10 +0100
                    Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Arnd Bergmann <arnd@arndb.de> - 2016-01-13 11:40 +0100
                    Re: [PATCH v1 3/3] ARM64 LPC: update binding doc Rongrong Zou <zourongrong@gmail.com> - 2016-01-13 12:10 +0100
                      Re: [PATCH v1 3/3] ARM64 LPC: update binding doc liviu.dudau@arm.com - 2016-01-13 12:30 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1308072

On Tue, 2016-01-12 at 23:52 +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 15:13:35 liviu.dudau@arm.com wrote:
> > > int of_address_to_resource(struct device_node *dev, int index,
> > >                          struct resource *r)
> > > {
> > >       ...
> > >       /* flags can be get here, without ranges property reqired.
> > >        * if the reg = <0x0 0xe4 4>, I can get flag of
> IORESOURCE_MEM,
> > >        * if the reg = <0x1 0xe4 4>, I can get flag of
> IORESOURCE_IO,
> > 
> > That is strange, the parent node has #address-cells = <2> so the
> first two numbers should be part
> > of the address and not influence the flags. Can you add some
> debugging in of_get_address() and
> > try to figure out what bus is used in  *flags = bus-
> >get_flags(prop) ?
> > 
> > 
> 
> This is the standard ISA binding. The first cell is the address space
> (IO or MEM), the second cell is the address within that space. This
> is similar to how PCI works.

Picking up that mid-way, I have LPC busses on power and am using a
similar binding. I'll try to grab some examples and review the
document tomorrow (only just noticed it while unpiling emails post-
vacation).

Cheers,
Ben.

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

#1308081

On 2016/1/13 13:53, Benjamin Herrenschmidt wrote:
> On Tue, 2016-01-12 at 23:52 +0100, Arnd Bergmann wrote:
>> On Tuesday 12 January 2016 15:13:35 liviu.dudau@arm.com wrote:
>>>> int of_address_to_resource(struct device_node *dev, int index,
>>>>                            struct resource *r)
>>>> {
>>>>         ...
>>>>         /* flags can be get here, without ranges property reqired.
>>>>          * if the reg = <0x0 0xe4 4>, I can get flag of
>> IORESOURCE_MEM,
>>>>          * if the reg = <0x1 0xe4 4>, I can get flag of
>> IORESOURCE_IO,
>>>
>>> That is strange, the parent node has #address-cells = <2> so the
>> first two numbers should be part
>>> of the address and not influence the flags. Can you add some
>> debugging in of_get_address() and
>>> try to figure out what bus is used in  *flags = bus-
>>> get_flags(prop) ?
>>>
>>>
>>
>> This is the standard ISA binding. The first cell is the address space
>> (IO or MEM), the second cell is the address within that space. This
>> is similar to how PCI works.
>
> Picking up that mid-way, I have LPC busses on power and am using a
> similar binding. I'll try to grab some examples and review the
> document tomorrow (only just noticed it while unpiling emails post-
> vacation).

Hi Ben,
Thanks for reviewing this, I found a similar implementation at arch/powerpc/
platform/powernv/opal-lpc.c and I had get some ideas from your work. It is
nice to me. I'm expecting your suggestion.Thanks in advance.

>
> Cheers,
> Ben.
>
Regards,
Rongrong

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

#1308207

On Wednesday 13 January 2016 14:34:47 Rongrong Zou wrote:
> On 2016/1/13 13:53, Benjamin Herrenschmidt wrote:
> > On Tue, 2016-01-12 at 23:52 +0100, Arnd Bergmann wrote:
> >> On Tuesday 12 January 2016 15:13:35 liviu.dudau@arm.com wrote:
> >>>> int of_address_to_resource(struct device_node *dev, int index,
> >>>>                            struct resource *r)
> >>>> {
> >>>>         ...
> >>>>         /* flags can be get here, without ranges property reqired.
> >>>>          * if the reg = <0x0 0xe4 4>, I can get flag of
> >> IORESOURCE_MEM,
> >>>>          * if the reg = <0x1 0xe4 4>, I can get flag of
> >> IORESOURCE_IO,
> >>>
> >>> That is strange, the parent node has #address-cells = <2> so the
> >> first two numbers should be part
> >>> of the address and not influence the flags. Can you add some
> >> debugging in of_get_address() and
> >>> try to figure out what bus is used in  *flags = bus-
> >>> get_flags(prop) ?
> >>>
> >>>
> >>
> >> This is the standard ISA binding. The first cell is the address space
> >> (IO or MEM), the second cell is the address within that space. This
> >> is similar to how PCI works.
> >
> > Picking up that mid-way, I have LPC busses on power and am using a
> > similar binding. I'll try to grab some examples and review the
> > document tomorrow (only just noticed it while unpiling emails post-
> > vacation).

I really should have thought of that, as you mentioned already that
there is an ast2400 on those machines, and no I/O space on the PCI
bus.

Too bad we have to keep the I/O workarounds alive on PowerPC now,
I was already hoping they could go away after spider-pci gets phased
out.

> Thanks for reviewing this, I found a similar implementation at arch/powerpc/
> platform/powernv/opal-lpc.c and I had get some ideas from your work. It is
> nice to me. I'm expecting your suggestion.Thanks in advance.

Unfortunately, the way that PCI host bridges on PowerPC are handled
is a bit different from what we do on ARM64, otherwise the obvious
solution would be to move the I/O workarounds to an architecture
independent location. Maybe it's still possible, but that also requires
some refactoring then.

	Arnd

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

#1308251

On Wednesday 13 January 2016 10:10:28 liviu.dudau@arm.com wrote:
> 
> OK, but from DT point of view and given the parent's #address-cells = <2> and #size-cells = <1>
> should the reg not be something like reg = <0x1 0x0 0xe4 4> ?

No: 2+1 = 3, not 4.

	Arnd

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

#1308269

On Wed, Jan 13, 2016 at 11:18:11AM +0100, Arnd Bergmann wrote:
> On Wednesday 13 January 2016 10:10:28 liviu.dudau@arm.com wrote:
> > 
> > OK, but from DT point of view and given the parent's #address-cells = <2> and #size-cells = <1>
> > should the reg not be something like reg = <0x1 0x0 0xe4 4> ?
> 
> No: 2+1 = 3, not 4.

Bah, not enough caffeine this morning, forgot the flags are part of the address space. Sorry for inept noise.

Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

#1308253

On Tue, Jan 12, 2016 at 11:52:48PM +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 15:13:35 liviu.dudau@arm.com wrote:
> > > int of_address_to_resource(struct device_node *dev, int index,
> > >                          struct resource *r)
> > > {
> > >       ...
> > >       /* flags can be get here, without ranges property reqired.
> > >        * if the reg = <0x0 0xe4 4>, I can get flag of IORESOURCE_MEM,
> > >        * if the reg = <0x1 0xe4 4>, I can get flag of IORESOURCE_IO,
> > 
> > That is strange, the parent node has #address-cells = <2> so the first two numbers should be part
> > of the address and not influence the flags. Can you add some debugging in of_get_address() and
> > try to figure out what bus is used in  *flags = bus->get_flags(prop) ?
> > 
> > 
> 
> This is the standard ISA binding. The first cell is the address space
> (IO or MEM), the second cell is the address within that space. This
> is similar to how PCI works.

OK, but from DT point of view and given the parent's #address-cells = <2> and #size-cells = <1>
should the reg not be something like reg = <0x1 0x0 0xe4 4> ?

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

#1307901

On Tuesday 12 January 2016 10:14:18 liviu.dudau@arm.com wrote:
> 
> OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
> 
> 

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.

	Arnd

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

#1308243

On Tue, Jan 12, 2016 at 11:54:59PM +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 10:14:18 liviu.dudau@arm.com wrote:
> > 
> > OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> > only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> > parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> > the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
> > 
> > 
> 
> 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.

Agree. However of_translate_one()'s behaviour doesn't match our expectations and I have no
useful suggestions on what the right behaviour should be.

Best regards,
Liviu

> 
> 	Arnd
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

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

#1308268

On Wednesday 13 January 2016 10:09:11 liviu.dudau@arm.com wrote:
> On Tue, Jan 12, 2016 at 11:54:59PM +0100, Arnd Bergmann wrote:
> > On Tuesday 12 January 2016 10:14:18 liviu.dudau@arm.com wrote:
> > > 
> > > OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> > > only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> > > parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> > > the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
> > > 
> > > 
> > 
> > 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.
> 
> Agree. However of_translate_one()'s behaviour doesn't match our expectations and I have no
> useful suggestions on what the right behaviour should be.

I believe of_get_address() already has the correct number (local to the
ISA/LPC bus here), an we just need to teach __of_address_to_resource
about ISA buses that have their own translation. We have the device
node of the ISA bus here, so we just need to stop translating further
using the ranges property and instead use the io_offset for that bus.

In fact we can use the same method for both ISA and PCI buses, if
we just remember which device node is the root for an I/O space
and what its offset is relative to the Linux I/O space. Going all
the way to a physical CPU address and then back to an I/O port number
through pci_address_to_pio() is awkward anyway, but here it's wrong
specifically because there is no physical address for it.

	Arnd

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

#1308288

On 2016/1/13 18:09, liviu.dudau@arm.com wrote:
> On Tue, Jan 12, 2016 at 11:54:59PM +0100, Arnd Bergmann wrote:
>> On Tuesday 12 January 2016 10:14:18 liviu.dudau@arm.com wrote:
>>>
>>> OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
>>> only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
>>> parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
>>> the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
>>>
>>>
>>
>> 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.
>
> Agree. However of_translate_one()'s behaviour doesn't match our expectations and I have no
> useful suggestions on what the right behaviour should be.
>

I had tried to modify the drivers/of/address.c to address this problem, but it looks
not so general. I'm not sure you have seen this: https://lkml.org/lkml/2016/1/10/89 .

Regards,
Rongrong

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

#1308302

On Wed, Jan 13, 2016 at 07:06:42PM +0800, Rongrong Zou wrote:
> On 2016/1/13 18:09, liviu.dudau@arm.com wrote:
> >On Tue, Jan 12, 2016 at 11:54:59PM +0100, Arnd Bergmann wrote:
> >>On Tuesday 12 January 2016 10:14:18 liviu.dudau@arm.com wrote:
> >>>
> >>>OK, looking at of_translate_one() comments it looks like a missing "ranges" property is
> >>>only accepted on PowerPC. I suggest you have an empty "ranges" property in your isa
> >>>parent node, that will signal to the OF parsing code that the mapping is 1:1. Then have
> >>>the IPMI node use the reg = <0x0 0xe4 4>; property values instead of reg = <0x1 0xe4 4>;
> >>>
> >>>
> >>
> >>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.
> >
> >Agree. However of_translate_one()'s behaviour doesn't match our expectations and I have no
> >useful suggestions on what the right behaviour should be.
> >
> 
> I had tried to modify the drivers/of/address.c to address this problem, but it looks
> not so general. I'm not sure you have seen this: https://lkml.org/lkml/2016/1/10/89 .

Yes, I have seen it. Based on Arnd's suggestion, it is probably the way to go.

Best regards,
Liviu

> 
> Regards,
> Rongrong
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web