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

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

[Rongrong Zou]

? 2015/12/31 23:00, Rongrong Zou ??:
>
>
> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>  >
>  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>  > > >> +Example:
>  > > >> +
>  > > >> +        lpc_0: lpc at a01b0000 {
>  > > >> +          #address-cells = <1>;
>  > > >> +          #size-cells = <1>;
>  > > >> +                compatible = "low-pin-count";
>  > > >> +                reg = <0x0 0xa01b0000 0x0 0x10000>;
>  > > >> +        };
>  > > >
>  > > > One more thought: please try to stick as closely as possible to the existing
>  > > > ISA binding that is documented at
>  > > >
>  > > > http://www.firmware.org/1275/bindings/isa/isa0_4d.ps
>  > >  From the specification, I think I should use 2 32bit integer to describe the isa addr in dts.
>  > > >
>  > > > In particular, this should cover the possibility of describing both memory
>  > > > and I/O spaces in child devices.
>  > > >
>  > >
>  > > I found below config in powerpc dts "arch/powerpc/boot/dts/mpc8544ds.dts"
>  > >
>  > > isa at 1e {
>  > >                                  device_type = "isa";
>  > >                                  #interrupt-cells = <2>;
>  > >                                  #size-cells = <1>;
>  > >                                  #address-cells = <2>;
>  > >                                  reg = <0xf000 0x0 0x0 0x0 0x0>;
>  > >                                  ranges = <0x1 0x0 0x1000000 0x0 0x0
>  > >                                            0x1000>;
>  > >                                  interrupt-parent = <&i8259>;
>  > >
>  > >
>  > >
>  > >                                  rtc at 70 {
>  > >                                          compatible = "pnpPNP,b00";
>  > >                                          reg = <0x1 0x70 0x2>;
>  > >                                  };
>  > >   the isa space in child-node: reg = <0x1 0x70 0x2>;
>  > >   0x1 means IO space, 70 means addr, 0x2 is size.
>  > >   but when i config the following in dts, the ipmi_0 node can't be probed,
>  > >   I think there may be some problems.
>  > >
>  > > lpc_0: lpc at a01b0000 {
>  > >       compatible = "low-pin-count";
>  > >       device_type = "isa";
>  > >       #address-cells = <2>;
>  > >       #size-cells = <1>;
>  > >       reg = <0x0 0xa01b0000 0x0 0x10000>;
>  > >
>  > >       ipmi_0:ipmi at 000000e4{
>  > >               device_type = "ipmi";
>  > >               compatible = "ipmi-bt";
>  > >               reg = <0x1 0x000000e4 0x4>;
>  > > };
>  >
>  > The DT sample above looks good in principle. I believe what you are missing
>  > here is code in your driver to scan the child nodes to create the platform
>  > devices. of_bus_isa_translate() should work with your definition here
>  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>  > resources, so the absence of a ranges property is ok. Maybe all you
>  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>  >
>
> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> will be sent later. By the way, it's my another email account use when i at home.

I tried, and there need some additional changes.

isa at a01b0000 {

/*the node name should start with "isa", because of below definition
* static int of_bus_isa_match(struct device_node *np)
* {
*	return !strcmp(np->name, "isa");
* }
*/
	compatible = "low-pin-count";
	device_type = "isa";
	#address-cells = <2>;
	#size-cells = <1>;
	reg = <0x0 0xa01b0000 0x0 0x10000>;
	ranges = <0x1 0x0 0x0 0x0 0x1000>;
/*
*  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
*
*/
	ipmi_0:ipmi at 000000e4{
		device_type = "ipmi";
		compatible = "ipmi-bt";
		reg = <0x1 0x000000e4 0x4>;
};

drivers\of\address.c
static int __of_address_to_resource(struct device_node *dev,
                 const __be32 *addrp, u64 size, unsigned int flags,
                 const char *name, struct resource *r)
{
         u64 taddr;

         if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
                 return -EINVAL;
         taddr = of_translate_address(dev, addrp);
         if (taddr == OF_BAD_ADDR)
                 return -EINVAL;
         memset(r, 0, sizeof(struct resource));
         if (flags & IORESOURCE_IO) {
                 unsigned long port;

/*****************************************************************/
/*legacy port(< 0x1000) is reserved, and need no translation here*/
/*****************************************************************/
                 if(taddr + size < PCIBIOS_MIN_IO){
                         r->start = taddr;
                         r->end = taddr + size - 1;
                 }


                 else{
                         port = pci_address_to_pio(taddr);
                         if (port == (unsigned long)-1)
                                 return -EINVAL;
                         r->start = port;
                         r->end = port + size - 1;
                 }
         } else {
                 r->start = taddr;
                 r->end = taddr + size - 1;
         }
         r->flags = flags;
         r->name = name ? name : dev->full_name;

         return 0;
}



>
>  >         Arnd

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

[Arnd Bergmann]

On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> ? 2015/12/31 23:00, Rongrong Zou ??:
> > 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> >  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> >  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >  >
> >  > The DT sample above looks good in principle. I believe what you are missing
> >  > here is code in your driver to scan the child nodes to create the platform
> >  > devices. of_bus_isa_translate() should work with your definition here
> >  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >  > resources, so the absence of a ranges property is ok. Maybe all you
> >  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >  >
> >
> > You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> > will be sent later. By the way, it's my another email account use when i at home.
> 
> I tried, and there need some additional changes.
> 
> isa at a01b0000 {
> 
> /*the node name should start with "isa", because of below definition
> * static int of_bus_isa_match(struct device_node *np)
> * {
> *	return !strcmp(np->name, "isa");
> * }

Looks good. It would be nicer to match on device_type than on name,
but this is ancient code and it's probably best not to touch it
so we don't accidentally break some old SPARC or PPC system.

> */
> 	compatible = "low-pin-count";
> 	device_type = "isa";
> 	#address-cells = <2>;
> 	#size-cells = <1>;
> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> /*
> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> *
> */
> 	ipmi_0:ipmi at 000000e4{
> 		device_type = "ipmi";
> 		compatible = "ipmi-bt";
> 		reg = <0x1 0x000000e4 0x4>;
> };
> 

This looks wrong: the property above says that the I/O port range is
translated to MMIO address 0x00000000 to 0x00010000, which is not
true on your hardware. I think this needs to be changed in the code
so the ranges property is not required for I/O ports.

> drivers\of\address.c
> static int __of_address_to_resource(struct device_node *dev,
>                  const __be32 *addrp, u64 size, unsigned int flags,
>                  const char *name, struct resource *r)
> {
>          u64 taddr;
> 
>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>                  return -EINVAL;
>          taddr = of_translate_address(dev, addrp);
>          if (taddr == OF_BAD_ADDR)
>                  return -EINVAL;
>          memset(r, 0, sizeof(struct resource));
>          if (flags & IORESOURCE_IO) {
>                  unsigned long port;
> 
> /*****************************************************************/
> /*legacy port(< 0x1000) is reserved, and need no translation here*/
> /*****************************************************************/
>                  if(taddr + size < PCIBIOS_MIN_IO){
>                          r->start = taddr;
>                          r->end = taddr + size - 1;
>                  }

I don't like having a special case based on the address here,
the same kind of hack might be needed for PCI I/O spaces in
hardware that uses an indirect method like your LPC bus
does, and the code above will not work on any LPC implementation
that correctly multiplexes its I/O ports with the first PCI domain.

I think it would be better to avoid translating the port into
a physical address to start with just to translate it back into
a port number, what we need instead is the offset between the
bus specific port number and the linux port number. I've added
Liviu to Cc, he wrote this code originally and may have some idea
of how we could do that.

	Arnd

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

[Rongrong Zou]

? 2016/1/4 19:13, Arnd Bergmann ??:
> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>>>   > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>   > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>>>   > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>   >
>>>   > The DT sample above looks good in principle. I believe what you are missing
>>>   > here is code in your driver to scan the child nodes to create the platform
>>>   > devices. of_bus_isa_translate() should work with your definition here
>>>   > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>   > resources, so the absence of a ranges property is ok. Maybe all you
>>>   > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>   >
>>>
>>> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
>>> will be sent later. By the way, it's my another email account use when i at home.
>>
>> I tried, and there need some additional changes.
>>
>> isa at a01b0000 {
>>
>> /*the node name should start with "isa", because of below definition
>> * static int of_bus_isa_match(struct device_node *np)
>> * {
>> *	return !strcmp(np->name, "isa");
>> * }
>
> Looks good. It would be nicer to match on device_type than on name,
> but this is ancient code and it's probably best not to touch it
> so we don't accidentally break some old SPARC or PPC system.
>
>> */
>> 	compatible = "low-pin-count";
>> 	device_type = "isa";
>> 	#address-cells = <2>;
>> 	#size-cells = <1>;
>> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
>> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
>> /*
>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>> *
>> */
>> 	ipmi_0:ipmi at 000000e4{
>> 		device_type = "ipmi";
>> 		compatible = "ipmi-bt";
>> 		reg = <0x1 0x000000e4 0x4>;
>> };
>>
>
> This looks wrong: the property above says that the I/O port range is
> translated to MMIO address 0x00000000 to 0x00010000, which is not
> true on your hardware. I think this needs to be changed in the code
> so the ranges property is not required for I/O ports.

Ranges property can set empty, but this means 1:1 translation. the I/O 
port range is translated to MMIO address 0x00000001 00000000 to 
0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy 
I/O port resource from dts.

For ipmi driver, I can get I/O port resource by DMI rather than dts.

>
>> drivers\of\address.c
>> static int __of_address_to_resource(struct device_node *dev,
>>                   const __be32 *addrp, u64 size, unsigned int flags,
>>                   const char *name, struct resource *r)
>> {
>>           u64 taddr;
>>
>>           if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>                   return -EINVAL;
>>           taddr = of_translate_address(dev, addrp);
>>           if (taddr == OF_BAD_ADDR)
>>                   return -EINVAL;
>>           memset(r, 0, sizeof(struct resource));
>>           if (flags & IORESOURCE_IO) {
>>                   unsigned long port;
>>
>> /*****************************************************************/
>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>> /*****************************************************************/
>>                   if(taddr + size < PCIBIOS_MIN_IO){
>>                           r->start = taddr;
>>                           r->end = taddr + size - 1;
>>                   }
>
> I don't like having a special case based on the address here,
> the same kind of hack might be needed for PCI I/O spaces in
> hardware that uses an indirect method like your LPC bus
> does, and the code above will not work on any LPC implementation
> that correctly multiplexes its I/O ports with the first PCI domain.
>
> I think it would be better to avoid translating the port into
> a physical address to start with just to translate it back into
> a port number, what we need instead is the offset between the
> bus specific port number and the linux port number. I've added
> Liviu to Cc, he wrote this code originally and may have some idea
> of how we could do that.
>
> 	Arnd
>

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

[Arnd Bergmann]

On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
> ? 2016/1/4 19:13, Arnd Bergmann ??:
> > On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >> ? 2015/12/31 23:00, Rongrong Zou ??:
> >> */
> >>      compatible = "low-pin-count";
> >>      device_type = "isa";
> >>      #address-cells = <2>;
> >>      #size-cells = <1>;
> >>      reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>      ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >> /*
> >> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >> *
> >> */
> >>      ipmi_0:ipmi at 000000e4{
> >>              device_type = "ipmi";
> >>              compatible = "ipmi-bt";
> >>              reg = <0x1 0x000000e4 0x4>;
> >> };
> >>
> >
> > This looks wrong: the property above says that the I/O port range is
> > translated to MMIO address 0x00000000 to 0x00010000, which is not
> > true on your hardware. I think this needs to be changed in the code
> > so the ranges property is not required for I/O ports.
> 
> Ranges property can set empty, but this means 1:1 translation. the I/O 
> port range is translated to MMIO address 0x00000001 00000000 to 
> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy 
> I/O port resource from dts.

As I said, nothing should really require the ranges property here, unless
you have a valid IORESOURCE_MEM translation. The code that requires
the ranges to be present is wrong.

> For ipmi driver, I can get I/O port resource by DMI rather than dts.

No, the ipmi driver uses the resource that belongs to the platform
device already, you can't rely on DMI data to be present there.

	Arnd

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

[Rongrong Zou]

? 2016/1/5 0:34, Arnd Bergmann ??:
> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>> ? 2016/1/4 19:13, Arnd Bergmann ??:
>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>> */
>>>>       compatible = "low-pin-count";
>>>>       device_type = "isa";
>>>>       #address-cells = <2>;
>>>>       #size-cells = <1>;
>>>>       reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>       ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>> /*
>>>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>> *
>>>> */
>>>>       ipmi_0:ipmi at 000000e4{
>>>>               device_type = "ipmi";
>>>>               compatible = "ipmi-bt";
>>>>               reg = <0x1 0x000000e4 0x4>;
>>>> };
>>>>
>>>
>>> This looks wrong: the property above says that the I/O port range is
>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>> true on your hardware. I think this needs to be changed in the code
>>> so the ranges property is not required for I/O ports.
>>
>> Ranges property can set empty, but this means 1:1 translation. the I/O
>> port range is translated to MMIO address 0x00000001 00000000 to
>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>> I/O port resource from dts.
>
> As I said, nothing should really require the ranges property here, unless
> you have a valid IORESOURCE_MEM translation. The code that requires
> the ranges to be present is wrong.
>

I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
must get resource by calling of_address_to_resource(), which have to call
pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
better idea for this now. Maybe liviu can give me some opinions.

/**
  * of_address_to_resource - Translate device tree address and return as resource
  *
  * Note that if your address is a PIO address, the conversion will fail if
  * the physical address can't be internally converted to an IO token with
  * pci_address_to_pio(), that is because it's either called to early or it
  * can't be matched to any host bridge IO space
  */
int of_address_to_resource(struct device_node *dev, int index,
			   struct resource *r)

>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>
> No, the ipmi driver uses the resource that belongs to the platform
> device already, you can't rely on DMI data to be present there.

Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
openfirmware and a few other), I think we just use one of them, not all of them.
It depend on vendor's hardware solution actually.


>
> 	Arnd
>
> .
>

Thanks,

Rongrong

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

[Arnd Bergmann]

On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
> ? 2016/1/5 0:34, Arnd Bergmann ??:
> > On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
> >> ? 2016/1/4 19:13, Arnd Bergmann ??:
> >>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>> ? 2015/12/31 23:00, Rongrong Zou ??:
> >> Ranges property can set empty, but this means 1:1 translation. the I/O
> >> port range is translated to MMIO address 0x00000001 00000000 to
> >> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
> >> I/O port resource from dts.
> >
> > As I said, nothing should really require the ranges property here, unless
> > you have a valid IORESOURCE_MEM translation. The code that requires
> > the ranges to be present is wrong.
> >
> 
> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
> must get resource by calling of_address_to_resource(), which have to call
> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
> better idea for this now. Maybe liviu can give me some opinions.

I think on x86 it works (or used to work, few people use open firmware on
x86 these days, and it may be broken), and the pci_address_to_pio() call
behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
memory mapped I/O addresses, they have their own way of accessing them
just like your platform.

> /**
>   * of_address_to_resource - Translate device tree address and return as resource
>   *
>   * Note that if your address is a PIO address, the conversion will fail if
>   * the physical address can't be internally converted to an IO token with
>   * pci_address_to_pio(), that is because it's either called to early or it
>   * can't be matched to any host bridge IO space
>   */
> int of_address_to_resource(struct device_node *dev, int index,
> 			   struct resource *r)

The problem here seems to be that the code assumes that either the I/O ports
are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
it because now we can have the combination of the two.

> >> For ipmi driver, I can get I/O port resource by DMI rather than dts.
> >
> > No, the ipmi driver uses the resource that belongs to the platform
> > device already, you can't rely on DMI data to be present there.
> 
> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
> openfirmware and a few other), I think we just use one of them, not all of them.
> It depend on vendor's hardware solution actually.

I don't think we should mix multiple methods here: if the bus is described
in DT, all its children should be there as well. Otherwise you get into problems
e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
for one or more of them have an offset to the bus specific addresses.

The bus probe code decides what the Linux I/O port numbers are, but DMI
and other methods have no idea of the mapping. As long as there is only
one instance, using the first 0x1000 addresses with a 1:1 mapping saves
us a bit of trouble, but I'd be worried about relying on that assumption
too much.

	Arnd

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

[Rongrong Zou]

? 2016/1/5 20:19, Arnd Bergmann ??:
> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>> ? 2016/1/5 0:34, Arnd Bergmann ??:
>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>> ? 2016/1/4 19:13, Arnd Bergmann ??:
>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>> I/O port resource from dts.
>>>
>>> As I said, nothing should really require the ranges property here, unless
>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>> the ranges to be present is wrong.
>>>
>>
>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>> must get resource by calling of_address_to_resource(), which have to call
>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>> better idea for this now. Maybe liviu can give me some opinions.
>
> I think on x86 it works (or used to work, few people use open firmware on
> x86 these days, and it may be broken), and the pci_address_to_pio() call
> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
> memory mapped I/O addresses, they have their own way of accessing them
> just like your platform.
>
>> /**
>>    * of_address_to_resource - Translate device tree address and return as resource
>>    *
>>    * Note that if your address is a PIO address, the conversion will fail if
>>    * the physical address can't be internally converted to an IO token with
>>    * pci_address_to_pio(), that is because it's either called to early or it
>>    * can't be matched to any host bridge IO space
>>    */
>> int of_address_to_resource(struct device_node *dev, int index,
>> 			   struct resource *r)
>
> The problem here seems to be that the code assumes that either the I/O ports
> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
> it because now we can have the combination of the two.


I am considering the following solution:

Adding unmapped isa io functions in

drivers/of/address.c,

static LIST_HEAD(legacy_io_range_list);
int isa_register_io_range(phys_addr_t addr, resource_size_t size);

/* before I call isa(LPC) bus driver, the input I/O port must be translated to phys_addr_t
(the least 16bit means port addr on bus, the second 16bit means bus id)*/

phys_addr_t isa_pio_to_bus_addr(unsigned long pio);

/* the returned PIO do not conflict with PIO get from pci_address_to_pio*/
unsigned long isa_bus_addr_to_pio(phys_addr_t address);

drivers/bus/lpc.c

lpc_bus_probe()
{
	isa_register_io_range(phys_addr_t addr, resource_size_t size);
}

inb(unsigned long port)
{
	unsigned short bus;
	phys_addr_t addr;
	/*hit isa port range*/
	if(addr = isa_pio_to_bus_addr(port))
	{
		bus = (addr >> 16) & 0xffff;

		call lpc driver with addr;
		return lpc_read_byte(bus, addr);
	}
	else /*not hit*/
	{
		return readb(PCI_IOBASE + port);
	}
}



>
>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>
>>> No, the ipmi driver uses the resource that belongs to the platform
>>> device already, you can't rely on DMI data to be present there.
>>
>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>> openfirmware and a few other), I think we just use one of them, not all of them.
>> It depend on vendor's hardware solution actually.
>
> I don't think we should mix multiple methods here: if the bus is described
> in DT, all its children should be there as well. Otherwise you get into problems
> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
> for one or more of them have an offset to the bus specific addresses.
>
> The bus probe code decides what the Linux I/O port numbers are, but DMI
> and other methods have no idea of the mapping. As long as there is only
> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
> us a bit of trouble, but I'd be worried about relying on that assumption
> too much.
>
> 	Arnd
>
>
> .
>

Thanks,

Rongrong

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

[Rongrong Zou]

? 2016/1/6 21:36, Rongrong Zou ??:
> ? 2016/1/5 20:19, Arnd Bergmann ??:
>> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>>> ? 2016/1/5 0:34, Arnd Bergmann ??:
>>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>>> ? 2016/1/4 19:13, Arnd Bergmann ??:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>>> I/O port resource from dts.
>>>>
>>>> As I said, nothing should really require the ranges property here, unless
>>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>>> the ranges to be present is wrong.
>>>>
>>>
>>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>>> must get resource by calling of_address_to_resource(), which have to call
>>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>>> better idea for this now. Maybe liviu can give me some opinions.
>>
>> I think on x86 it works (or used to work, few people use open firmware on
>> x86 these days, and it may be broken), and the pci_address_to_pio() call
>> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
>> memory mapped I/O addresses, they have their own way of accessing them
>> just like your platform.
>>

The big problem is:

The return value of_translate_address can be only an cpu addr,
there is no map from cpuaddr to I/O port in x86 as you said,
we still can't get io resource.

static int __of_address_to_resource(struct device_node *dev,
const __be32 *addrp, u64 size, unsigned int flags,
const char *name, struct resource *r)
{
...
taddr = of_translate_address(dev, addrp);
...
}


>>> /**
>>>    * of_address_to_resource - Translate device tree address and return as resource
>>>    *
>>>    * Note that if your address is a PIO address, the conversion will fail if
>>>    * the physical address can't be internally converted to an IO token with
>>>    * pci_address_to_pio(), that is because it's either called to early or it
>>>    * can't be matched to any host bridge IO space
>>>    */
>>> int of_address_to_resource(struct device_node *dev, int index,
>>>                struct resource *r)
>>
>> The problem here seems to be that the code assumes that either the I/O ports
>> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
>> it because now we can have the combination of the two.
>
>
> I am considering the following solution:
>
> Adding unmapped isa io functions in
>
> drivers/of/address.c,
>
> static LIST_HEAD(legacy_io_range_list);
> int isa_register_io_range(phys_addr_t addr, resource_size_t size);
>
> /* before I call isa(LPC) bus driver, the input I/O port must be translated to phys_addr_t
> (the least 16bit means port addr on bus, the second 16bit means bus id)*/
>
> phys_addr_t isa_pio_to_bus_addr(unsigned long pio);
>
> /* the returned PIO do not conflict with PIO get from pci_address_to_pio*/
> unsigned long isa_bus_addr_to_pio(phys_addr_t address);
>
> drivers/bus/lpc.c
>
> lpc_bus_probe()
> {
>      isa_register_io_range(phys_addr_t addr, resource_size_t size);
> }
>
> inb(unsigned long port)
> {
>      unsigned short bus;
>      phys_addr_t addr;
>      /*hit isa port range*/
>      if(addr = isa_pio_to_bus_addr(port))
>      {
>          bus = (addr >> 16) & 0xffff;
>
>          call lpc driver with addr;
>          return lpc_read_byte(bus, addr);
>      }
>      else /*not hit*/
>      {
>          return readb(PCI_IOBASE + port);
>      }
> }
>
>
>
>>
>>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>>
>>>> No, the ipmi driver uses the resource that belongs to the platform
>>>> device already, you can't rely on DMI data to be present there.
>>>
>>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>>> openfirmware and a few other), I think we just use one of them, not all of them.
>>> It depend on vendor's hardware solution actually.
>>
>> I don't think we should mix multiple methods here: if the bus is described
>> in DT, all its children should be there as well. Otherwise you get into problems
>> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
>> for one or more of them have an offset to the bus specific addresses.
>>
>> The bus probe code decides what the Linux I/O port numbers are, but DMI
>> and other methods have no idea of the mapping. As long as there is only
>> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
>> us a bit of trouble, but I'd be worried about relying on that assumption
>> too much.
>>
>>     Arnd
>>
>>
>> .

-- 
Thanks,

Rongrong

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

[Rolland Chau]

On 2016/1/5 20:19, Arnd Bergmann wrote:
> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>> ? 2016/1/5 0:34, Arnd Bergmann ??:
>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>> ? 2016/1/4 19:13, Arnd Bergmann ??:
>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get legacy
>>>> I/O port resource from dts.
>>>
>>> As I said, nothing should really require the ranges property here, unless
>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>> the ranges to be present is wrong.
>>>
>>
>> I think the openfirmware(DT) do not support for those unmapped I/O ports, because I
>> must get resource by calling of_address_to_resource(), which have to call
>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I have no
>> better idea for this now. Maybe liviu can give me some opinions.
>
> I think on x86 it works (or used to work, few people use open firmware on
> x86 these days, and it may be broken), and the pci_address_to_pio() call
> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
> memory mapped I/O addresses, they have their own way of accessing them
> just like your platform.
>
>> /**
>>    * of_address_to_resource - Translate device tree address and return as resource
>>    *
>>    * Note that if your address is a PIO address, the conversion will fail if
>>    * the physical address can't be internally converted to an IO token with
>>    * pci_address_to_pio(), that is because it's either called to early or it
>>    * can't be matched to any host bridge IO space
>>    */
>> int of_address_to_resource(struct device_node *dev, int index,
>> 			   struct resource *r)
>
> The problem here seems to be that the code assumes that either the I/O ports
> are always mapped or they are never mapped (no PCI_IOBASE). We need to extend
> it because now we can have the combination of the two.


Arnd , please take a look at the new solution when you have a moment,thanks.
I modify the "drivers/of/address.c" to address the problem above.

There are some assumptions:

1) ISA(LPC) Like bus must be scanned before PCI.
2) a property "unmapped-size" is introduced to ISA bus DT config, it means the address
  range of the bus. if bus-A have a port range(0-99), bus-B have a port range(0-199), then
the cpu port range (0-99) is reserved for bus-A and cpu port range is reserved for bus-B.
3) It it
---
  drivers/of/address.c | 76 +++++++++++++++++++++++++++++++++++++++++++++++-----
  1 file changed, 70 insertions(+), 6 deletions(-)

diff --git a/drivers/of/address.c b/drivers/of/address.c
index 9582c57..d3c71e5 100644
--- a/drivers/of/address.c
+++ b/drivers/of/address.c
@@ -682,8 +682,16 @@ struct io_range {

  static LIST_HEAD(io_range_list);
  static DEFINE_SPINLOCK(io_range_lock);
+phy_addr_t pci_pio_start = 0;
+EXPORT_SYMBOL(pci_pio_start)
  #endif

+int isa_register_unmapped_io_range(resource_size_t size)
+{
+       pci_pio_start += size;
+}
+
+
  /*
   * Record the PCI IO range (expressed as CPU physical address + size).
   * Return a negative value if an error has occured, zero otherwise
@@ -694,7 +702,7 @@ int __weak pci_register_io_range(phys_addr_t addr, resource_size_t size)

  #ifdef PCI_IOBASE
         struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = pci_pio_start;

         /* check if the range hasn't been previously recorded */
         spin_lock(&io_range_lock);
@@ -743,7 +751,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)

  #ifdef PCI_IOBASE
         struct io_range *range;
-       resource_size_t allocated_size = 0;
+       resource_size_t allocated_size = pci_pio_start;

         if (pio > IO_SPACE_LIMIT)
                 return address;
@@ -766,7 +774,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
  {
  #ifdef PCI_IOBASE
         struct io_range *res;
-       resource_size_t offset = 0;
+       resource_size_t offset = pci_pio_start;
         unsigned long addr = -1;

         spin_lock(&io_range_lock);
@@ -788,21 +796,77 @@ unsigned long __weak pci_address_to_pio(phys_addr_t address)
  #endif
  }

+static u64 of_get_unmapped_pio(struct device_node *dev, const __be32 *inaddr)
+{
+       struct device_node *np, *parent = NULL;
+       struct of_bus * bus, *pbus;
+       struct property *p_property;
+       int rlen;
+       u32 size = 0;
+       u32 port_base = 0;
+
+       /*IF NOT I/O port*/
+       if(*(in_addr) != 0x01)
+               return OF_BAD_ADDR;
+       else
+               *in_addr = 0;
+
+       parent = of_get_parent(dev);
+       if(parent == NULL)
+               return result;
+       bus = of_match_bus(parent);
+       if(!strcmp(bus->name, "isa")) {
+               p_property = of_find_property(parent, "ranges", &rlen);
+               /*no ranges property, this is a non-bridged isa bus, and the port on it is unmapped*/
+               if(p_property == NULL) {
+                       for_each_node_by_name(np, "isa") {
+                               if((np != parent) &&
+                                  !of_find_property(parent, "ranges", &rlen) &&
+                                  !of_property_read_u32(parent, "unmapped-size", &size)) {
+                                       port_base += size;
+                               }
+                               else if (np == parent) {
+                                       of_bus_default_translate(in_addr + 1, port_base, 1);
+                                       break;
+                               }
+                               else {
+                                       continue;
+                               }
+                       }
+                       return of_read_number(in_addr, 2);
+
+               }
+
+       }
+       return OF_BAD_ADDR;
+}
+
  static int __of_address_to_resource(struct device_node *dev,
                 const __be32 *addrp, u64 size, unsigned int flags,
                 const char *name, struct resource *r)
  {
         u64 taddr;
+       u8 addr_type = 0;

         if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
                 return -EINVAL;
         taddr = of_translate_address(dev, addrp);
-       if (taddr == OF_BAD_ADDR)
-               return -EINVAL;
+       if (taddr == OF_BAD_ADDR) {
+
+               taddr == of_get_unmapped_pio(dev, addrp);
+               if(taddr == OF_BAD_ADDR)
+                       return -EINVAL
+               else    /*we get unmapped I/O port successfully*/
+                       addr_type = 1;
+       }
+
         memset(r, 0, sizeof(struct resource));
         if (flags & IORESOURCE_IO) {
                 unsigned long port;
-               port = pci_address_to_pio(taddr);
+               if(!addr_type)
+                       port = pci_address_to_pio(taddr);
+               else
+                       port = (unsigned long)taddr;
                 if (port == (unsigned long)-1)
                         return -EINVAL;
                 r->start = port;
-- 


In bus device probe code
static int lpc_probe(struct platform_device *pdev)
{
	...
	if(ret = of_property_read_u32((pdev->dev).of_node, "unmapped-size", &io_ranges))
		return ret;
	
	lpc_dev->bus_size = io_ranges;
	isa_register_io_range(io_ranges);
	ret = of_platform_populate(pdev->dev.of_node, NULL, NULL, &pdev->dev);
	...
	
	return 0;
}

>
>>>> For ipmi driver, I can get I/O port resource by DMI rather than dts.
>>>
>>> No, the ipmi driver uses the resource that belongs to the platform
>>> device already, you can't rely on DMI data to be present there.
>>
>> Ipmi has a lot of way to be discovered(ACPI, DMI, hardcoded, hot-add,
>> openfirmware and a few other), I think we just use one of them, not all of them.
>> It depend on vendor's hardware solution actually.
>
> I don't think we should mix multiple methods here: if the bus is described
> in DT, all its children should be there as well. Otherwise you get into problems
> e.g. if you have multiple instances of the LPC bus and the Linux I/O addresses
> for one or more of them have an offset to the bus specific addresses.
>
> The bus probe code decides what the Linux I/O port numbers are, but DMI
> and other methods have no idea of the mapping. As long as there is only
> one instance, using the first 0x1000 addresses with a 1:1 mapping saves
> us a bit of trouble, but I'd be worried about relying on that assumption
> too much.
>
> 	Arnd
>

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

[Rongrong Zou]

? 2016/1/10 17:29, Rolland Chau ??:
> On 2016/1/5 20:19, Arnd Bergmann wrote:
>> On Tuesday 05 January 2016 19:59:49 Rongrong Zou wrote:
>>> ? 2016/1/5 0:34, Arnd Bergmann ??:
>>>> On Tuesday 05 January 2016 00:04:19 Rongrong Zou wrote:
>>>>> ? 2016/1/4 19:13, Arnd Bergmann ??:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>>> Ranges property can set empty, but this means 1:1 translation. the I/O
>>>>> port range is translated to MMIO address 0x00000001 00000000 to
>>>>> 0x00000001 00000004, it looks wrong else. I wonder if anyone get
>>>>> legacy
>>>>> I/O port resource from dts.
>>>>
>>>> As I said, nothing should really require the ranges property here,
>>>> unless
>>>> you have a valid IORESOURCE_MEM translation. The code that requires
>>>> the ranges to be present is wrong.
>>>>
>>>
>>> I think the openfirmware(DT) do not support for those unmapped I/O
>>> ports, because I
>>> must get resource by calling of_address_to_resource(), which have to
>>> call
>>> pci_address_to_pio() when resource type is IORESOURCE_IO. I'm sorry I
>>> have no
>>> better idea for this now. Maybe liviu can give me some opinions.
>>
>> I think on x86 it works (or used to work, few people use open firmware on
>> x86 these days, and it may be broken), and the pci_address_to_pio() call
>> behaves differently when PCI_IOBASE is set. x86 never maps I/O ports into
>> memory mapped I/O addresses, they have their own way of accessing them
>> just like your platform.
>>
>>> /**
>>>    * of_address_to_resource - Translate device tree address and
>>> return as resource
>>>    *
>>>    * Note that if your address is a PIO address, the conversion will
>>> fail if
>>>    * the physical address can't be internally converted to an IO
>>> token with
>>>    * pci_address_to_pio(), that is because it's either called to
>>> early or it
>>>    * can't be matched to any host bridge IO space
>>>    */
>>> int of_address_to_resource(struct device_node *dev, int index,
>>>                struct resource *r)
>>
>> The problem here seems to be that the code assumes that either the I/O
>> ports
>> are always mapped or they are never mapped (no PCI_IOBASE). We need to
>> extend
>> it because now we can have the combination of the two.
>
>
> Arnd , please take a look at the new solution when you have a
> moment,thanks.
> I modify the "drivers/of/address.c" to address the problem above.
>
> There are some assumptions:
>
> 1) ISA(LPC) Like bus must be scanned before PCI.
> 2) a property "unmapped-size" is introduced to ISA bus DT config, it
> means the address
>   range of the bus. if bus-A have a port range(0-99), bus-B have a port
> range(0-199), then
> the cpu port range (0-99) is reserved for bus-A and cpu port range (100, 299) is
> reserved for bus-B.
> 3) It it

I sorry, I made a mistake, please ignore 3). And I added the cpu port 
range(100, 299) for bus-B.

> ---
>   drivers/of/address.c | 76
> +++++++++++++++++++++++++++++++++++++++++++++++-----
>   1 file changed, 70 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/of/address.c b/drivers/of/address.c
> index 9582c57..d3c71e5 100644
> --- a/drivers/of/address.c
> +++ b/drivers/of/address.c
> @@ -682,8 +682,16 @@ struct io_range {
>
>   static LIST_HEAD(io_range_list);
>   static DEFINE_SPINLOCK(io_range_lock);
> +phy_addr_t pci_pio_start = 0;
> +EXPORT_SYMBOL(pci_pio_start)
>   #endif
>
> +int isa_register_unmapped_io_range(resource_size_t size)
> +{
> +       pci_pio_start += size;
> +}
> +
> +
>   /*
>    * Record the PCI IO range (expressed as CPU physical address + size).
>    * Return a negative value if an error has occured, zero otherwise
> @@ -694,7 +702,7 @@ int __weak pci_register_io_range(phys_addr_t addr,
> resource_size_t size)
>
>   #ifdef PCI_IOBASE
>          struct io_range *range;
> -       resource_size_t allocated_size = 0;
> +       resource_size_t allocated_size = pci_pio_start;
>
>          /* check if the range hasn't been previously recorded */
>          spin_lock(&io_range_lock);
> @@ -743,7 +751,7 @@ phys_addr_t pci_pio_to_address(unsigned long pio)
>
>   #ifdef PCI_IOBASE
>          struct io_range *range;
> -       resource_size_t allocated_size = 0;
> +       resource_size_t allocated_size = pci_pio_start;
>
>          if (pio > IO_SPACE_LIMIT)
>                  return address;
> @@ -766,7 +774,7 @@ unsigned long __weak pci_address_to_pio(phys_addr_t
> address)
>   {
>   #ifdef PCI_IOBASE
>          struct io_range *res;
> -       resource_size_t offset = 0;
> +       resource_size_t offset = pci_pio_start;
>          unsigned long addr = -1;
>
>          spin_lock(&io_range_lock);
> @@ -788,21 +796,77 @@ unsigned long __weak
> pci_address_to_pio(phys_addr_t address)
>   #endif
>   }
>
> +static u64 of_get_unmapped_pio(struct device_node *dev, const __be32
> *inaddr)
> +{
> +       struct device_node *np, *parent = NULL;
> +       struct of_bus * bus, *pbus;
> +       struct property *p_property;
> +       int rlen;
> +       u32 size = 0;
> +       u32 port_base = 0;
> +
> +       /*IF NOT I/O port*/
> +       if(*(in_addr) != 0x01)
> +               return OF_BAD_ADDR;
> +       else
> +               *in_addr = 0;
> +
> +       parent = of_get_parent(dev);
> +       if(parent == NULL)
> +               return result;
> +       bus = of_match_bus(parent);
> +       if(!strcmp(bus->name, "isa")) {
> +               p_property = of_find_property(parent, "ranges", &rlen);
> +               /*no ranges property, this is a non-bridged isa bus, and
> the port on it is unmapped*/
> +               if(p_property == NULL) {
> +                       for_each_node_by_name(np, "isa") {
> +                               if((np != parent) &&
> +                                  !of_find_property(parent, "ranges",
> &rlen) &&
> +                                  !of_property_read_u32(parent,
> "unmapped-size", &size)) {
> +                                       port_base += size;
> +                               }
> +                               else if (np == parent) {
> +                                       of_bus_default_translate(in_addr
> + 1, port_base, 1);
> +                                       break;
> +                               }
> +                               else {
> +                                       continue;
> +                               }
> +                       }
> +                       return of_read_number(in_addr, 2);
> +
> +               }
> +
> +       }
> +       return OF_BAD_ADDR;
> +}
> +
>   static int __of_address_to_resource(struct device_node *dev,
>                  const __be32 *addrp, u64 size, unsigned int flags,
>                  const char *name, struct resource *r)
>   {
>          u64 taddr;
> +       u8 addr_type = 0;
>
>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>                  return -EINVAL;
>          taddr = of_translate_address(dev, addrp);
> -       if (taddr == OF_BAD_ADDR)
> -               return -EINVAL;
> +       if (taddr == OF_BAD_ADDR) {
> +
> +               taddr == of_get_unmapped_pio(dev, addrp);
> +               if(taddr == OF_BAD_ADDR)
> +                       return -EINVAL
> +               else    /*we get unmapped I/O port successfully*/
> +                       addr_type = 1;
> +       }
> +
>          memset(r, 0, sizeof(struct resource));
>          if (flags & IORESOURCE_IO) {
>                  unsigned long port;
> -               port = pci_address_to_pio(taddr);
> +               if(!addr_type)
> +                       port = pci_address_to_pio(taddr);
> +               else
> +                       port = (unsigned long)taddr;
>                  if (port == (unsigned long)-1)
>                          return -EINVAL;
>                  r->start = port;

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

[liviu.dudau at arm.com]

On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> > ? 2015/12/31 23:00, Rongrong Zou ??:
> > > 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> > >  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> > >  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> > >  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> > >  >
> > >  > The DT sample above looks good in principle. I believe what you are missing
> > >  > here is code in your driver to scan the child nodes to create the platform
> > >  > devices. of_bus_isa_translate() should work with your definition here
> > >  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> > >  > resources, so the absence of a ranges property is ok. Maybe all you
> > >  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> > >  >
> > >
> > > You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> > > will be sent later. By the way, it's my another email account use when i at home.
> > 
> > I tried, and there need some additional changes.
> > 
> > isa at a01b0000 {
> > 
> > /*the node name should start with "isa", because of below definition
> > * static int of_bus_isa_match(struct device_node *np)
> > * {
> > *	return !strcmp(np->name, "isa");
> > * }
> 
> Looks good. It would be nicer to match on device_type than on name,
> but this is ancient code and it's probably best not to touch it
> so we don't accidentally break some old SPARC or PPC system.
> 
> > */
> > 	compatible = "low-pin-count";
> > 	device_type = "isa";
> > 	#address-cells = <2>;
> > 	#size-cells = <1>;
> > 	reg = <0x0 0xa01b0000 0x0 0x10000>;
> > 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> > /*
> > *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> > *
> > */
> > 	ipmi_0:ipmi at 000000e4{
> > 		device_type = "ipmi";
> > 		compatible = "ipmi-bt";
> > 		reg = <0x1 0x000000e4 0x4>;
> > };
> > 
> 
> This looks wrong: the property above says that the I/O port range is
> translated to MMIO address 0x00000000 to 0x00010000, which is not
> true on your hardware. I think this needs to be changed in the code
> so the ranges property is not required for I/O ports.
> 
> > drivers\of\address.c
> > static int __of_address_to_resource(struct device_node *dev,
> >                  const __be32 *addrp, u64 size, unsigned int flags,
> >                  const char *name, struct resource *r)
> > {
> >          u64 taddr;
> > 
> >          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >                  return -EINVAL;
> >          taddr = of_translate_address(dev, addrp);
> >          if (taddr == OF_BAD_ADDR)
> >                  return -EINVAL;
> >          memset(r, 0, sizeof(struct resource));
> >          if (flags & IORESOURCE_IO) {
> >                  unsigned long port;
> > 
> > /*****************************************************************/
> > /*legacy port(< 0x1000) is reserved, and need no translation here*/
> > /*****************************************************************/
> >                  if(taddr + size < PCIBIOS_MIN_IO){
> >                          r->start = taddr;
> >                          r->end = taddr + size - 1;
> >                  }
> 
> I don't like having a special case based on the address here,
> the same kind of hack might be needed for PCI I/O spaces in
> hardware that uses an indirect method like your LPC bus
> does, and the code above will not work on any LPC implementation
> that correctly multiplexes its I/O ports with the first PCI domain.
> 
> I think it would be better to avoid translating the port into
> a physical address to start with just to translate it back into
> a port number, what we need instead is the offset between the
> bus specific port number and the linux port number. I've added
> Liviu to Cc, he wrote this code originally and may have some idea
> of how we could do that.

Hi,      
         
Getting back to work after a longer holiday, my brain might not be running
at full speed here, so I'm trying to clarify things a bit here.

It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
added to arm64 by patch 1/3 and redirect those operations to the memory
mapped LPC driver. I think the whole redirection and registration of inb/outb
ops can be made cleaner, so that the general concept resembles the DMA ops
registration? (I have this mental picture that what Rongrong is trying to do
is similar to what a DMA engine does, except this is slowing down things to
byte level). If that is done properly in the parent node, then we should not
care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
go through the redirection for the children.

As for the ranges property: does he wants the ipmi-bt driver to see in the
reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
then I agree that the range property should not be required, but also the
reg values need to be changed (drop the top bit). If the later, then the
ranges property is required to do the proper translation.

Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
the of_address_to_resource() translation?

Best regards,
Liviu


> 
> 	Arnd
> 

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

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

[Rongrong Zou]

On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>>>>   > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>   > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>>>>   > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>   >
>>>>   > The DT sample above looks good in principle. I believe what you are missing
>>>>   > here is code in your driver to scan the child nodes to create the platform
>>>>   > devices. of_bus_isa_translate() should work with your definition here
>>>>   > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>   > resources, so the absence of a ranges property is ok. Maybe all you
>>>>   > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>   >
>>>>
>>>> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>
>>> I tried, and there need some additional changes.
>>>
>>> isa at a01b0000 {
>>>
>>> /*the node name should start with "isa", because of below definition
>>> * static int of_bus_isa_match(struct device_node *np)
>>> * {
>>> *	return !strcmp(np->name, "isa");
>>> * }
>>
>> Looks good. It would be nicer to match on device_type than on name,
>> but this is ancient code and it's probably best not to touch it
>> so we don't accidentally break some old SPARC or PPC system.
>>
>>> */
>>> 	compatible = "low-pin-count";
>>> 	device_type = "isa";
>>> 	#address-cells = <2>;
>>> 	#size-cells = <1>;
>>> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
>>> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>> /*
>>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>> *
>>> */
>>> 	ipmi_0:ipmi at 000000e4{
>>> 		device_type = "ipmi";
>>> 		compatible = "ipmi-bt";
>>> 		reg = <0x1 0x000000e4 0x4>;
>>> };
>>>
>>
>> This looks wrong: the property above says that the I/O port range is
>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>> true on your hardware. I think this needs to be changed in the code
>> so the ranges property is not required for I/O ports.
>>
>>> drivers\of\address.c
>>> static int __of_address_to_resource(struct device_node *dev,
>>>                   const __be32 *addrp, u64 size, unsigned int flags,
>>>                   const char *name, struct resource *r)
>>> {
>>>           u64 taddr;
>>>
>>>           if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>                   return -EINVAL;
>>>           taddr = of_translate_address(dev, addrp);
>>>           if (taddr == OF_BAD_ADDR)
>>>                   return -EINVAL;
>>>           memset(r, 0, sizeof(struct resource));
>>>           if (flags & IORESOURCE_IO) {
>>>                   unsigned long port;
>>>
>>> /*****************************************************************/
>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>> /*****************************************************************/
>>>                   if(taddr + size < PCIBIOS_MIN_IO){
>>>                           r->start = taddr;
>>>                           r->end = taddr + size - 1;
>>>                   }
>>
>> I don't like having a special case based on the address here,
>> the same kind of hack might be needed for PCI I/O spaces in
>> hardware that uses an indirect method like your LPC bus
>> does, and the code above will not work on any LPC implementation
>> that correctly multiplexes its I/O ports with the first PCI domain.
>>
>> I think it would be better to avoid translating the port into
>> a physical address to start with just to translate it back into
>> a port number, what we need instead is the offset between the
>> bus specific port number and the linux port number. I've added
>> Liviu to Cc, he wrote this code originally and may have some idea
>> of how we could do that.
>
> Hi,

Hi Liviu,

Thanks for reviewing this.

>
> Getting back to work after a longer holiday, my brain might not be running
> at full speed here, so I'm trying to clarify things a bit here.
>
> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> added to arm64 by patch 1/3 and redirect those operations to the memory
> mapped LPC driver. I think the whole redirection and registration of inb/outb
> ops can be made cleaner, so that the general concept resembles the DMA ops
> registration? (I have this mental picture that what Rongrong is trying to do
> is similar to what a DMA engine does, except this is slowing down things to
> byte level). If that is done properly in the parent node, then we should not
> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> go through the redirection for the children.
>
> As for the ranges property: does he wants the ipmi-bt driver to see in the
> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> then I agree that the range property should not be required, but also the
> reg values need to be changed (drop the top bit). If the later, then the
> ranges property is required to do the proper translation.

The former, thanks.

>
> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> the of_address_to_resource() translation?

I want to get IORESOURCE_IO type resource, but if the parent node drop the
"rangs" property, the of_address_to_resource() translation will return with -EINVAL.

>
> Best regards,
> Liviu
>
>
>>
>> 	Arnd
>>
>
Regards,
Rongrong

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
> >On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>? 2015/12/31 23:00, Rongrong Zou ??:
> >>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> >>>>  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> >>>>  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>  >
> >>>>  > The DT sample above looks good in principle. I believe what you are missing
> >>>>  > here is code in your driver to scan the child nodes to create the platform
> >>>>  > devices. of_bus_isa_translate() should work with your definition here
> >>>>  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>  > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>  >
> >>>>
> >>>>You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> >>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>
> >>>I tried, and there need some additional changes.
> >>>
> >>>isa at a01b0000 {
> >>>
> >>>/*the node name should start with "isa", because of below definition
> >>>* static int of_bus_isa_match(struct device_node *np)
> >>>* {
> >>>*	return !strcmp(np->name, "isa");
> >>>* }
> >>
> >>Looks good. It would be nicer to match on device_type than on name,
> >>but this is ancient code and it's probably best not to touch it
> >>so we don't accidentally break some old SPARC or PPC system.
> >>
> >>>*/
> >>>	compatible = "low-pin-count";
> >>>	device_type = "isa";
> >>>	#address-cells = <2>;
> >>>	#size-cells = <1>;
> >>>	reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>/*
> >>>*  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>*
> >>>*/
> >>>	ipmi_0:ipmi at 000000e4{
> >>>		device_type = "ipmi";
> >>>		compatible = "ipmi-bt";
> >>>		reg = <0x1 0x000000e4 0x4>;
> >>>};
> >>>
> >>
> >>This looks wrong: the property above says that the I/O port range is
> >>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>true on your hardware. I think this needs to be changed in the code
> >>so the ranges property is not required for I/O ports.
> >>
> >>>drivers\of\address.c
> >>>static int __of_address_to_resource(struct device_node *dev,
> >>>                  const __be32 *addrp, u64 size, unsigned int flags,
> >>>                  const char *name, struct resource *r)
> >>>{
> >>>          u64 taddr;
> >>>
> >>>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>                  return -EINVAL;
> >>>          taddr = of_translate_address(dev, addrp);
> >>>          if (taddr == OF_BAD_ADDR)
> >>>                  return -EINVAL;
> >>>          memset(r, 0, sizeof(struct resource));
> >>>          if (flags & IORESOURCE_IO) {
> >>>                  unsigned long port;
> >>>
> >>>/*****************************************************************/
> >>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>/*****************************************************************/
> >>>                  if(taddr + size < PCIBIOS_MIN_IO){
> >>>                          r->start = taddr;
> >>>                          r->end = taddr + size - 1;
> >>>                  }
> >>
> >>I don't like having a special case based on the address here,
> >>the same kind of hack might be needed for PCI I/O spaces in
> >>hardware that uses an indirect method like your LPC bus
> >>does, and the code above will not work on any LPC implementation
> >>that correctly multiplexes its I/O ports with the first PCI domain.
> >>
> >>I think it would be better to avoid translating the port into
> >>a physical address to start with just to translate it back into
> >>a port number, what we need instead is the offset between the
> >>bus specific port number and the linux port number. I've added
> >>Liviu to Cc, he wrote this code originally and may have some idea
> >>of how we could do that.
> >
> >Hi,
> 
> Hi Liviu,
> 
> Thanks for reviewing this.
> 
> >
> >Getting back to work after a longer holiday, my brain might not be running
> >at full speed here, so I'm trying to clarify things a bit here.
> >
> >It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >added to arm64 by patch 1/3 and redirect those operations to the memory
> >mapped LPC driver. I think the whole redirection and registration of inb/outb
> >ops can be made cleaner, so that the general concept resembles the DMA ops
> >registration? (I have this mental picture that what Rongrong is trying to do
> >is similar to what a DMA engine does, except this is slowing down things to
> >byte level). If that is done properly in the parent node, then we should not
> >care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >go through the redirection for the children.
> >
> >As for the ranges property: does he wants the ipmi-bt driver to see in the
> >reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >then I agree that the range property should not be required, but also the
> >reg values need to be changed (drop the top bit). If the later, then the
> >ranges property is required to do the proper translation.
> 
> The former, thanks.
> 
> >
> >Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >the of_address_to_resource() translation?
> 
> I want to get IORESOURCE_IO type resource, but if the parent node drop the
> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.

Have you tracked what part of the code is sensitive to the presence of "ranges"
property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?

Best regards,
Liviu

> 
> >
> >Best regards,
> >Liviu
> >
> >
> >>
> >>	Arnd
> >>
> >
> Regards,
> Rongrong
> 

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

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

[Rongrong Zou]

? 2016/1/12 17:07, liviu.dudau at arm.com ??:
> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>> On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>>>>>>   > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>>   > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>>>>>>   > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>>   >
>>>>>>   > The DT sample above looks good in principle. I believe what you are missing
>>>>>>   > here is code in your driver to scan the child nodes to create the platform
>>>>>>   > devices. of_bus_isa_translate() should work with your definition here
>>>>>>   > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>>   > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>>   > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>>   >
>>>>>>
>>>>>> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>
>>>>> I tried, and there need some additional changes.
>>>>>
>>>>> isa at a01b0000 {
>>>>>
>>>>> /*the node name should start with "isa", because of below definition
>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>> * {
>>>>> *	return !strcmp(np->name, "isa");
>>>>> * }
>>>>
>>>> Looks good. It would be nicer to match on device_type than on name,
>>>> but this is ancient code and it's probably best not to touch it
>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>
>>>>> */
>>>>> 	compatible = "low-pin-count";
>>>>> 	device_type = "isa";
>>>>> 	#address-cells = <2>;
>>>>> 	#size-cells = <1>;
>>>>> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>> /*
>>>>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>> *
>>>>> */
>>>>> 	ipmi_0:ipmi at 000000e4{
>>>>> 		device_type = "ipmi";
>>>>> 		compatible = "ipmi-bt";
>>>>> 		reg = <0x1 0x000000e4 0x4>;
>>>>> };
>>>>>
>>>>
>>>> This looks wrong: the property above says that the I/O port range is
>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>> true on your hardware. I think this needs to be changed in the code
>>>> so the ranges property is not required for I/O ports.
>>>>
>>>>> drivers\of\address.c
>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>>                   const __be32 *addrp, u64 size, unsigned int flags,
>>>>>                   const char *name, struct resource *r)
>>>>> {
>>>>>           u64 taddr;
>>>>>
>>>>>           if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>>                   return -EINVAL;
>>>>>           taddr = of_translate_address(dev, addrp);
>>>>>           if (taddr == OF_BAD_ADDR)
>>>>>                   return -EINVAL;
>>>>>           memset(r, 0, sizeof(struct resource));
>>>>>           if (flags & IORESOURCE_IO) {
>>>>>                   unsigned long port;
>>>>>
>>>>> /*****************************************************************/
>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>> /*****************************************************************/
>>>>>                   if(taddr + size < PCIBIOS_MIN_IO){
>>>>>                           r->start = taddr;
>>>>>                           r->end = taddr + size - 1;
>>>>>                   }
>>>>
>>>> I don't like having a special case based on the address here,
>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>> hardware that uses an indirect method like your LPC bus
>>>> does, and the code above will not work on any LPC implementation
>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>
>>>> I think it would be better to avoid translating the port into
>>>> a physical address to start with just to translate it back into
>>>> a port number, what we need instead is the offset between the
>>>> bus specific port number and the linux port number. I've added
>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>> of how we could do that.
>>>
>>> Hi,
>>
>> Hi Liviu,
>>
>> Thanks for reviewing this.
>>
>>>
>>> Getting back to work after a longer holiday, my brain might not be running
>>> at full speed here, so I'm trying to clarify things a bit here.
>>>
>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>> registration? (I have this mental picture that what Rongrong is trying to do
>>> is similar to what a DMA engine does, except this is slowing down things to
>>> byte level). If that is done properly in the parent node, then we should not
>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>> go through the redirection for the children.
>>>
>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>> then I agree that the range property should not be required, but also the
>>> reg values need to be changed (drop the top bit). If the later, then the
>>> ranges property is required to do the proper translation.
>>
>> The former, thanks.
>>
>>>
>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>> the of_address_to_resource() translation?
>>
>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>
> Have you tracked what part of the code is sensitive to the presence of "ranges"
> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>


Yes, IO_RESOURCE flag can be get without "ranges".
I tracked the code, it is at of_translate_one(), Below is the calling infomation.

of_address_to_resource-> __of_address_to_resource ->of_translate_address->
__of_translate_address(dev, in_addr, "ranges")->of_translate_one()


static int of_translate_one(struct device_node *parent, struct of_bus *bus,
			    struct of_bus *pbus, __be32 *addr,
			    int na, int ns, int pna, const char *rprop)
{
	const __be32 *ranges;
	unsigned int rlen;
	int rone;
	u64 offset = OF_BAD_ADDR;

	ranges = of_get_property(parent, rprop, &rlen);
	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
		pr_debug("OF: no ranges; cannot translate\n");
		return 1;
	}
	...
}

> Best regards,
> Liviu
>
>>
>>>
>>> Best regards,
>>> Liviu
>>>
>>>
>>>>
>>>> 	Arnd
>>>>
>>>
>> Regards,
>> Rongrong
>>
>
-- 
Regards,
Rongrong

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> ? 2016/1/12 17:07, liviu.dudau at arm.com ??:
> >On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
> >>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>? 2015/12/31 23:00, Rongrong Zou ??:
> >>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> >>>>>>  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>>  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> >>>>>>  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>>  >
> >>>>>>  > The DT sample above looks good in principle. I believe what you are missing
> >>>>>>  > here is code in your driver to scan the child nodes to create the platform
> >>>>>>  > devices. of_bus_isa_translate() should work with your definition here
> >>>>>>  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>>  > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>>  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>>  >
> >>>>>>
> >>>>>>You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> >>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>
> >>>>>I tried, and there need some additional changes.
> >>>>>
> >>>>>isa at a01b0000 {
> >>>>>
> >>>>>/*the node name should start with "isa", because of below definition
> >>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>* {
> >>>>>*	return !strcmp(np->name, "isa");
> >>>>>* }
> >>>>
> >>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>but this is ancient code and it's probably best not to touch it
> >>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>
> >>>>>*/
> >>>>>	compatible = "low-pin-count";
> >>>>>	device_type = "isa";
> >>>>>	#address-cells = <2>;
> >>>>>	#size-cells = <1>;
> >>>>>	reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>>	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>/*
> >>>>>*  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>*
> >>>>>*/
> >>>>>	ipmi_0:ipmi at 000000e4{
> >>>>>		device_type = "ipmi";
> >>>>>		compatible = "ipmi-bt";
> >>>>>		reg = <0x1 0x000000e4 0x4>;
> >>>>>};
> >>>>>
> >>>>
> >>>>This looks wrong: the property above says that the I/O port range is
> >>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>true on your hardware. I think this needs to be changed in the code
> >>>>so the ranges property is not required for I/O ports.
> >>>>
> >>>>>drivers\of\address.c
> >>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>>                  const __be32 *addrp, u64 size, unsigned int flags,
> >>>>>                  const char *name, struct resource *r)
> >>>>>{
> >>>>>          u64 taddr;
> >>>>>
> >>>>>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>>                  return -EINVAL;
> >>>>>          taddr = of_translate_address(dev, addrp);
> >>>>>          if (taddr == OF_BAD_ADDR)
> >>>>>                  return -EINVAL;
> >>>>>          memset(r, 0, sizeof(struct resource));
> >>>>>          if (flags & IORESOURCE_IO) {
> >>>>>                  unsigned long port;
> >>>>>
> >>>>>/*****************************************************************/
> >>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>/*****************************************************************/
> >>>>>                  if(taddr + size < PCIBIOS_MIN_IO){
> >>>>>                          r->start = taddr;
> >>>>>                          r->end = taddr + size - 1;
> >>>>>                  }
> >>>>
> >>>>I don't like having a special case based on the address here,
> >>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>hardware that uses an indirect method like your LPC bus
> >>>>does, and the code above will not work on any LPC implementation
> >>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>
> >>>>I think it would be better to avoid translating the port into
> >>>>a physical address to start with just to translate it back into
> >>>>a port number, what we need instead is the offset between the
> >>>>bus specific port number and the linux port number. I've added
> >>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>of how we could do that.
> >>>
> >>>Hi,
> >>
> >>Hi Liviu,
> >>
> >>Thanks for reviewing this.
> >>
> >>>
> >>>Getting back to work after a longer holiday, my brain might not be running
> >>>at full speed here, so I'm trying to clarify things a bit here.
> >>>
> >>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>is similar to what a DMA engine does, except this is slowing down things to
> >>>byte level). If that is done properly in the parent node, then we should not
> >>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>go through the redirection for the children.
> >>>
> >>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>then I agree that the range property should not be required, but also the
> >>>reg values need to be changed (drop the top bit). If the later, then the
> >>>ranges property is required to do the proper translation.
> >>
> >>The former, thanks.
> >>
> >>>
> >>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>the of_address_to_resource() translation?
> >>
> >>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >
> >Have you tracked what part of the code is sensitive to the presence of "ranges"
> >property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >
> 
> 
> Yes, IO_RESOURCE flag can be get without "ranges".
> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
> 
> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
> 
> 
> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> 			    struct of_bus *pbus, __be32 *addr,
> 			    int na, int ns, int pna, const char *rprop)
> {
> 	const __be32 *ranges;
> 	unsigned int rlen;
> 	int rone;
> 	u64 offset = OF_BAD_ADDR;
> 
> 	ranges = of_get_property(parent, rprop, &rlen);
> 	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> 		pr_debug("OF: no ranges; cannot translate\n");
> 		return 1;
> 	}
> 	...
> }

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

Best regards,
Liviu

> 
> >Best regards,
> >Liviu
> >
> >>
> >>>
> >>>Best regards,
> >>>Liviu
> >>>
> >>>
> >>>>
> >>>>	Arnd
> >>>>
> >>>
> >>Regards,
> >>Rongrong
> >>
> >
> -- 
> Regards,
> Rongrong
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

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

[Rongrong Zou]

? 2016/1/12 18:14, liviu.dudau at arm.com ??:
> On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
>> ? 2016/1/12 17:07, liviu.dudau at arm.com ??:
>>> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>>>> On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
>>>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>>>>>>>>   > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>>>>   > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>>>>>>>>   > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>>>>   >
>>>>>>>>   > The DT sample above looks good in principle. I believe what you are missing
>>>>>>>>   > here is code in your driver to scan the child nodes to create the platform
>>>>>>>>   > devices. of_bus_isa_translate() should work with your definition here
>>>>>>>>   > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>>>>   > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>>>>   > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>>>>   >
>>>>>>>>
>>>>>>>> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
>>>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>>>
>>>>>>> I tried, and there need some additional changes.
>>>>>>>
>>>>>>> isa at a01b0000 {
>>>>>>>
>>>>>>> /*the node name should start with "isa", because of below definition
>>>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>>>> * {
>>>>>>> *	return !strcmp(np->name, "isa");
>>>>>>> * }
>>>>>>
>>>>>> Looks good. It would be nicer to match on device_type than on name,
>>>>>> but this is ancient code and it's probably best not to touch it
>>>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>>>
>>>>>>> */
>>>>>>> 	compatible = "low-pin-count";
>>>>>>> 	device_type = "isa";
>>>>>>> 	#address-cells = <2>;
>>>>>>> 	#size-cells = <1>;
>>>>>>> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>>>> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>>>> /*
>>>>>>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>>>> *
>>>>>>> */
>>>>>>> 	ipmi_0:ipmi at 000000e4{
>>>>>>> 		device_type = "ipmi";
>>>>>>> 		compatible = "ipmi-bt";
>>>>>>> 		reg = <0x1 0x000000e4 0x4>;
>>>>>>> };
>>>>>>>
>>>>>>
>>>>>> This looks wrong: the property above says that the I/O port range is
>>>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>>>> true on your hardware. I think this needs to be changed in the code
>>>>>> so the ranges property is not required for I/O ports.
>>>>>>
>>>>>>> drivers\of\address.c
>>>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>>>>                   const __be32 *addrp, u64 size, unsigned int flags,
>>>>>>>                   const char *name, struct resource *r)
>>>>>>> {
>>>>>>>           u64 taddr;
>>>>>>>
>>>>>>>           if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>>>>                   return -EINVAL;
>>>>>>>           taddr = of_translate_address(dev, addrp);
>>>>>>>           if (taddr == OF_BAD_ADDR)
>>>>>>>                   return -EINVAL;
>>>>>>>           memset(r, 0, sizeof(struct resource));
>>>>>>>           if (flags & IORESOURCE_IO) {
>>>>>>>                   unsigned long port;
>>>>>>>
>>>>>>> /*****************************************************************/
>>>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>>>> /*****************************************************************/
>>>>>>>                   if(taddr + size < PCIBIOS_MIN_IO){
>>>>>>>                           r->start = taddr;
>>>>>>>                           r->end = taddr + size - 1;
>>>>>>>                   }
>>>>>>
>>>>>> I don't like having a special case based on the address here,
>>>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>>>> hardware that uses an indirect method like your LPC bus
>>>>>> does, and the code above will not work on any LPC implementation
>>>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>>>
>>>>>> I think it would be better to avoid translating the port into
>>>>>> a physical address to start with just to translate it back into
>>>>>> a port number, what we need instead is the offset between the
>>>>>> bus specific port number and the linux port number. I've added
>>>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>>>> of how we could do that.
>>>>>
>>>>> Hi,
>>>>
>>>> Hi Liviu,
>>>>
>>>> Thanks for reviewing this.
>>>>
>>>>>
>>>>> Getting back to work after a longer holiday, my brain might not be running
>>>>> at full speed here, so I'm trying to clarify things a bit here.
>>>>>
>>>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>>>> registration? (I have this mental picture that what Rongrong is trying to do
>>>>> is similar to what a DMA engine does, except this is slowing down things to
>>>>> byte level). If that is done properly in the parent node, then we should not
>>>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>>>> go through the redirection for the children.
>>>>>
>>>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>>>> then I agree that the range property should not be required, but also the
>>>>> reg values need to be changed (drop the top bit). If the later, then the
>>>>> ranges property is required to do the proper translation.
>>>>
>>>> The former, thanks.
>>>>
>>>>>
>>>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>>>> the of_address_to_resource() translation?
>>>>
>>>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>>>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>>>
>>> Have you tracked what part of the code is sensitive to the presence of "ranges"
>>> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>>>
>>
>>
>> Yes, IO_RESOURCE flag can be get without "ranges".
>> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
>>
>> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
>> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
>>
>>
>> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>> 			    struct of_bus *pbus, __be32 *addr,
>> 			    int na, int ns, int pna, const char *rprop)
>> {
>> 	const __be32 *ranges;
>> 	unsigned int rlen;
>> 	int rone;
>> 	u64 offset = OF_BAD_ADDR;
>>
>> 	ranges = of_get_property(parent, rprop, &rlen);
>> 	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>> 		pr_debug("OF: no ranges; cannot translate\n");
>> 		return 1;
>> 	}
>> 	...
>> }
>
> 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>;

But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
ranges has been discussed.


>
> Best regards,
> Liviu
>
>>
>>> Best regards,
>>> Liviu
>>>
>>>>
>>>>>
>>>>> Best regards,
>>>>> Liviu
>>>>>
>>>>>
>>>>>>
>>>>>> 	Arnd
>>>>>>
>>>>>
>>>> Regards,
>>>> Rongrong
>>>>
>>>
>> --
>> Regards,
>> Rongrong
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel at lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>


-- 
Regards,
Rongrong

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
> ? 2016/1/12 18:14, liviu.dudau at arm.com ??:
> >On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> >>? 2016/1/12 17:07, liviu.dudau at arm.com ??:
> >>>On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>>>On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
> >>>>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>>>? 2015/12/31 23:00, Rongrong Zou ??:
> >>>>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> >>>>>>>>  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>>>>  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> >>>>>>>>  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>>>>  >
> >>>>>>>>  > The DT sample above looks good in principle. I believe what you are missing
> >>>>>>>>  > here is code in your driver to scan the child nodes to create the platform
> >>>>>>>>  > devices. of_bus_isa_translate() should work with your definition here
> >>>>>>>>  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>>>>  > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>>>>  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>>>>  >
> >>>>>>>>
> >>>>>>>>You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> >>>>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>>>
> >>>>>>>I tried, and there need some additional changes.
> >>>>>>>
> >>>>>>>isa at a01b0000 {
> >>>>>>>
> >>>>>>>/*the node name should start with "isa", because of below definition
> >>>>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>>>* {
> >>>>>>>*	return !strcmp(np->name, "isa");
> >>>>>>>* }
> >>>>>>
> >>>>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>>>but this is ancient code and it's probably best not to touch it
> >>>>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>>>
> >>>>>>>*/
> >>>>>>>	compatible = "low-pin-count";
> >>>>>>>	device_type = "isa";
> >>>>>>>	#address-cells = <2>;
> >>>>>>>	#size-cells = <1>;
> >>>>>>>	reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>>>>	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>>>/*
> >>>>>>>*  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>>>*
> >>>>>>>*/
> >>>>>>>	ipmi_0:ipmi at 000000e4{
> >>>>>>>		device_type = "ipmi";
> >>>>>>>		compatible = "ipmi-bt";
> >>>>>>>		reg = <0x1 0x000000e4 0x4>;
> >>>>>>>};
> >>>>>>>
> >>>>>>
> >>>>>>This looks wrong: the property above says that the I/O port range is
> >>>>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>>>true on your hardware. I think this needs to be changed in the code
> >>>>>>so the ranges property is not required for I/O ports.
> >>>>>>
> >>>>>>>drivers\of\address.c
> >>>>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>>>>                  const __be32 *addrp, u64 size, unsigned int flags,
> >>>>>>>                  const char *name, struct resource *r)
> >>>>>>>{
> >>>>>>>          u64 taddr;
> >>>>>>>
> >>>>>>>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>>>>                  return -EINVAL;
> >>>>>>>          taddr = of_translate_address(dev, addrp);
> >>>>>>>          if (taddr == OF_BAD_ADDR)
> >>>>>>>                  return -EINVAL;
> >>>>>>>          memset(r, 0, sizeof(struct resource));
> >>>>>>>          if (flags & IORESOURCE_IO) {
> >>>>>>>                  unsigned long port;
> >>>>>>>
> >>>>>>>/*****************************************************************/
> >>>>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>>>/*****************************************************************/
> >>>>>>>                  if(taddr + size < PCIBIOS_MIN_IO){
> >>>>>>>                          r->start = taddr;
> >>>>>>>                          r->end = taddr + size - 1;
> >>>>>>>                  }
> >>>>>>
> >>>>>>I don't like having a special case based on the address here,
> >>>>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>>>hardware that uses an indirect method like your LPC bus
> >>>>>>does, and the code above will not work on any LPC implementation
> >>>>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>>>
> >>>>>>I think it would be better to avoid translating the port into
> >>>>>>a physical address to start with just to translate it back into
> >>>>>>a port number, what we need instead is the offset between the
> >>>>>>bus specific port number and the linux port number. I've added
> >>>>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>>>of how we could do that.
> >>>>>
> >>>>>Hi,
> >>>>
> >>>>Hi Liviu,
> >>>>
> >>>>Thanks for reviewing this.
> >>>>
> >>>>>
> >>>>>Getting back to work after a longer holiday, my brain might not be running
> >>>>>at full speed here, so I'm trying to clarify things a bit here.
> >>>>>
> >>>>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>>>is similar to what a DMA engine does, except this is slowing down things to
> >>>>>byte level). If that is done properly in the parent node, then we should not
> >>>>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>>>go through the redirection for the children.
> >>>>>
> >>>>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>>>then I agree that the range property should not be required, but also the
> >>>>>reg values need to be changed (drop the top bit). If the later, then the
> >>>>>ranges property is required to do the proper translation.
> >>>>
> >>>>The former, thanks.
> >>>>
> >>>>>
> >>>>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>>>the of_address_to_resource() translation?
> >>>>
> >>>>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>>>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >>>
> >>>Have you tracked what part of the code is sensitive to the presence of "ranges"
> >>>property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >>>
> >>
> >>
> >>Yes, IO_RESOURCE flag can be get without "ranges".

Earlier, you said this ^

> >>I tracked the code, it is at of_translate_one(), Below is the calling infomation.
> >>
> >>of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> >>__of_translate_address(dev, in_addr, "ranges")->of_translate_one()
> >>
> >>
> >>static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >>			    struct of_bus *pbus, __be32 *addr,
> >>			    int na, int ns, int pna, const char *rprop)
> >>{
> >>	const __be32 *ranges;
> >>	unsigned int rlen;
> >>	int rone;
> >>	u64 offset = OF_BAD_ADDR;
> >>
> >>	ranges = of_get_property(parent, rprop, &rlen);
> >>	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >>		pr_debug("OF: no ranges; cannot translate\n");
> >>		return 1;
> >>	}
> >>	...
> >>}
> >
> >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>;
> 
> But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
> the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
> ranges has been discussed.

So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
do you actually get, MMIO is not a valid value.

Liviu

> 
> -- 
> Regards,
> Rongrong
> 

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

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

[Rongrong Zou]

? 2016/1/12 19:27, liviu.dudau at arm.com ??:
> On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
>> ? 2016/1/12 18:14, liviu.dudau at arm.com ??:
>>> On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
>>>> ? 2016/1/12 17:07, liviu.dudau at arm.com ??:
>>>>> On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
>>>>>> On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
>>>>>>> On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
>>>>>>>> On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
>>>>>>>>> ? 2015/12/31 23:00, Rongrong Zou ??:
>>>>>>>>>> 2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
>>>>>>>>>>   > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
>>>>>>>>>>   > > ? 2015/12/30 17:06, Arnd Bergmann ??:
>>>>>>>>>>   > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
>>>>>>>>>>   >
>>>>>>>>>>   > The DT sample above looks good in principle. I believe what you are missing
>>>>>>>>>>   > here is code in your driver to scan the child nodes to create the platform
>>>>>>>>>>   > devices. of_bus_isa_translate() should work with your definition here
>>>>>>>>>>   > and create the correct IORESOURCE_IO resources. You don't have any MMIO
>>>>>>>>>>   > resources, so the absence of a ranges property is ok. Maybe all you
>>>>>>>>>>   > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
>>>>>>>>>>   >
>>>>>>>>>>
>>>>>>>>>> You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
>>>>>>>>>> will be sent later. By the way, it's my another email account use when i at home.
>>>>>>>>>
>>>>>>>>> I tried, and there need some additional changes.
>>>>>>>>>
>>>>>>>>> isa at a01b0000 {
>>>>>>>>>
>>>>>>>>> /*the node name should start with "isa", because of below definition
>>>>>>>>> * static int of_bus_isa_match(struct device_node *np)
>>>>>>>>> * {
>>>>>>>>> *	return !strcmp(np->name, "isa");
>>>>>>>>> * }
>>>>>>>>
>>>>>>>> Looks good. It would be nicer to match on device_type than on name,
>>>>>>>> but this is ancient code and it's probably best not to touch it
>>>>>>>> so we don't accidentally break some old SPARC or PPC system.
>>>>>>>>
>>>>>>>>> */
>>>>>>>>> 	compatible = "low-pin-count";
>>>>>>>>> 	device_type = "isa";
>>>>>>>>> 	#address-cells = <2>;
>>>>>>>>> 	#size-cells = <1>;
>>>>>>>>> 	reg = <0x0 0xa01b0000 0x0 0x10000>;
>>>>>>>>> 	ranges = <0x1 0x0 0x0 0x0 0x1000>;
>>>>>>>>> /*
>>>>>>>>> *  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
>>>>>>>>> *
>>>>>>>>> */
>>>>>>>>> 	ipmi_0:ipmi at 000000e4{
>>>>>>>>> 		device_type = "ipmi";
>>>>>>>>> 		compatible = "ipmi-bt";
>>>>>>>>> 		reg = <0x1 0x000000e4 0x4>;
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>
>>>>>>>> This looks wrong: the property above says that the I/O port range is
>>>>>>>> translated to MMIO address 0x00000000 to 0x00010000, which is not
>>>>>>>> true on your hardware. I think this needs to be changed in the code
>>>>>>>> so the ranges property is not required for I/O ports.
>>>>>>>>
>>>>>>>>> drivers\of\address.c
>>>>>>>>> static int __of_address_to_resource(struct device_node *dev,
>>>>>>>>>                   const __be32 *addrp, u64 size, unsigned int flags,
>>>>>>>>>                   const char *name, struct resource *r)
>>>>>>>>> {
>>>>>>>>>           u64 taddr;
>>>>>>>>>
>>>>>>>>>           if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
>>>>>>>>>                   return -EINVAL;
>>>>>>>>>           taddr = of_translate_address(dev, addrp);
>>>>>>>>>           if (taddr == OF_BAD_ADDR)
>>>>>>>>>                   return -EINVAL;
>>>>>>>>>           memset(r, 0, sizeof(struct resource));
>>>>>>>>>           if (flags & IORESOURCE_IO) {
>>>>>>>>>                   unsigned long port;
>>>>>>>>>
>>>>>>>>> /*****************************************************************/
>>>>>>>>> /*legacy port(< 0x1000) is reserved, and need no translation here*/
>>>>>>>>> /*****************************************************************/
>>>>>>>>>                   if(taddr + size < PCIBIOS_MIN_IO){
>>>>>>>>>                           r->start = taddr;
>>>>>>>>>                           r->end = taddr + size - 1;
>>>>>>>>>                   }
>>>>>>>>
>>>>>>>> I don't like having a special case based on the address here,
>>>>>>>> the same kind of hack might be needed for PCI I/O spaces in
>>>>>>>> hardware that uses an indirect method like your LPC bus
>>>>>>>> does, and the code above will not work on any LPC implementation
>>>>>>>> that correctly multiplexes its I/O ports with the first PCI domain.
>>>>>>>>
>>>>>>>> I think it would be better to avoid translating the port into
>>>>>>>> a physical address to start with just to translate it back into
>>>>>>>> a port number, what we need instead is the offset between the
>>>>>>>> bus specific port number and the linux port number. I've added
>>>>>>>> Liviu to Cc, he wrote this code originally and may have some idea
>>>>>>>> of how we could do that.
>>>>>>>
>>>>>>> Hi,
>>>>>>
>>>>>> Hi Liviu,
>>>>>>
>>>>>> Thanks for reviewing this.
>>>>>>
>>>>>>>
>>>>>>> Getting back to work after a longer holiday, my brain might not be running
>>>>>>> at full speed here, so I'm trying to clarify things a bit here.
>>>>>>>
>>>>>>> It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
>>>>>>> added to arm64 by patch 1/3 and redirect those operations to the memory
>>>>>>> mapped LPC driver. I think the whole redirection and registration of inb/outb
>>>>>>> ops can be made cleaner, so that the general concept resembles the DMA ops
>>>>>>> registration? (I have this mental picture that what Rongrong is trying to do
>>>>>>> is similar to what a DMA engine does, except this is slowing down things to
>>>>>>> byte level). If that is done properly in the parent node, then we should not
>>>>>>> care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
>>>>>>> go through the redirection for the children.
>>>>>>>
>>>>>>> As for the ranges property: does he wants the ipmi-bt driver to see in the
>>>>>>> reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
>>>>>>> then I agree that the range property should not be required, but also the
>>>>>>> reg values need to be changed (drop the top bit). If the later, then the
>>>>>>> ranges property is required to do the proper translation.
>>>>>>
>>>>>> The former, thanks.
>>>>>>
>>>>>>>
>>>>>>> Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
>>>>>>> in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
>>>>>>> the of_address_to_resource() translation?
>>>>>>
>>>>>> I want to get IORESOURCE_IO type resource, but if the parent node drop the
>>>>>> "rangs" property, the of_address_to_resource() translation will return with -EINVAL.
>>>>>
>>>>> Have you tracked what part of the code is sensitive to the presence of "ranges"
>>>>> property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
>>>>>
>>>>
>>>>
>>>> Yes, IO_RESOURCE flag can be get without "ranges".
>
> Earlier, you said this ^
>
>>>> I tracked the code, it is at of_translate_one(), Below is the calling infomation.
>>>>
>>>> of_address_to_resource-> __of_address_to_resource ->of_translate_address->
>>>> __of_translate_address(dev, in_addr, "ranges")->of_translate_one()
>>>>
>>>>
>>>> static int of_translate_one(struct device_node *parent, struct of_bus *bus,
>>>> 			    struct of_bus *pbus, __be32 *addr,
>>>> 			    int na, int ns, int pna, const char *rprop)
>>>> {
>>>> 	const __be32 *ranges;
>>>> 	unsigned int rlen;
>>>> 	int rone;
>>>> 	u64 offset = OF_BAD_ADDR;
>>>>
>>>> 	ranges = of_get_property(parent, rprop, &rlen);
>>>> 	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
>>>> 		pr_debug("OF: no ranges; cannot translate\n");
>>>> 		return 1;
>>>> 	}
>>>> 	...
>>>> }
>>>
>>> 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>;
>>
>> But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
>> the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
>> ranges has been discussed.
>
> So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
> do you actually get, MMIO is not a valid value.



I'm sorry I did not describe clearly.
Without "ranges", of_get_address() will return with valid value, but of_address_to_resource()
will return with -EINVAL;

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,
	 */
	addrp = of_get_address(dev, index, &size, &flags);

	if (addrp == NULL)
		return -EINVAL;

	/* Get optional "reg-names" property to add a name to a resource */
	of_property_read_string_index(dev, "reg-names",	index, &name);


	/* If it is empty ranges, and the flag is IORESOURCE_MEM then the below  __of_address_to_resource return with valid addr,
	 * if it is empty ranges, and the flag is IORESOURCE_IO then return with -EINVAL, because
	 *    pci_address_to_pio() will be called when flag is IORESOURCE_IO.
	 * if the ranges is absent, then return with -EINVAL.
	 */
	return __of_address_to_resource(dev, addrp, size, flags, name, r);
}

I want to get resource with flag = IORESOURCE_IO, resource.start=0XE4, resource.size=0X4,

>
> Liviu
>
>>
>> --
>> Regards,
>> Rongrong
>>
>


-- 
Regards,
Rongrong

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 07:56:47PM +0800, Rongrong Zou wrote:
> ? 2016/1/12 19:27, liviu.dudau at arm.com ??:
> >On Tue, Jan 12, 2016 at 07:05:29PM +0800, Rongrong Zou wrote:
> >>? 2016/1/12 18:14, liviu.dudau at arm.com ??:
> >>>On Tue, Jan 12, 2016 at 05:25:56PM +0800, Rongrong Zou wrote:
> >>>>? 2016/1/12 17:07, liviu.dudau at arm.com ??:
> >>>>>On Tue, Jan 12, 2016 at 10:39:36AM +0800, Rongrong Zou wrote:
> >>>>>>On 2016/1/12 0:14, liviu.dudau at arm.com wrote:
> >>>>>>>On Mon, Jan 04, 2016 at 12:13:05PM +0100, Arnd Bergmann wrote:
> >>>>>>>>On Sunday 03 January 2016 20:24:14 Rongrong Zou wrote:
> >>>>>>>>>? 2015/12/31 23:00, Rongrong Zou ??:
> >>>>>>>>>>2015-12-31 22:40 GMT+08:00 Arnd Bergmann <arnd at arndb.de <mailto:arnd@arndb.de>>:
> >>>>>>>>>>  > On Thursday 31 December 2015 22:12:19 Rongrong Zou wrote:
> >>>>>>>>>>  > > ? 2015/12/30 17:06, Arnd Bergmann ??:
> >>>>>>>>>>  > > > On Tuesday 29 December 2015 21:33:52 Rongrong Zou wrote:
> >>>>>>>>>>  >
> >>>>>>>>>>  > The DT sample above looks good in principle. I believe what you are missing
> >>>>>>>>>>  > here is code in your driver to scan the child nodes to create the platform
> >>>>>>>>>>  > devices. of_bus_isa_translate() should work with your definition here
> >>>>>>>>>>  > and create the correct IORESOURCE_IO resources. You don't have any MMIO
> >>>>>>>>>>  > resources, so the absence of a ranges property is ok. Maybe all you
> >>>>>>>>>>  > are missing is a call to of_platform_populate() or of_platform_bus_probe()?
> >>>>>>>>>>  >
> >>>>>>>>>>
> >>>>>>>>>>You are right. thanks, i'll try on test board .  if i get the correct result , the new patch
> >>>>>>>>>>will be sent later. By the way, it's my another email account use when i at home.
> >>>>>>>>>
> >>>>>>>>>I tried, and there need some additional changes.
> >>>>>>>>>
> >>>>>>>>>isa at a01b0000 {
> >>>>>>>>>
> >>>>>>>>>/*the node name should start with "isa", because of below definition
> >>>>>>>>>* static int of_bus_isa_match(struct device_node *np)
> >>>>>>>>>* {
> >>>>>>>>>*	return !strcmp(np->name, "isa");
> >>>>>>>>>* }
> >>>>>>>>
> >>>>>>>>Looks good. It would be nicer to match on device_type than on name,
> >>>>>>>>but this is ancient code and it's probably best not to touch it
> >>>>>>>>so we don't accidentally break some old SPARC or PPC system.
> >>>>>>>>
> >>>>>>>>>*/
> >>>>>>>>>	compatible = "low-pin-count";
> >>>>>>>>>	device_type = "isa";
> >>>>>>>>>	#address-cells = <2>;
> >>>>>>>>>	#size-cells = <1>;
> >>>>>>>>>	reg = <0x0 0xa01b0000 0x0 0x10000>;
> >>>>>>>>>	ranges = <0x1 0x0 0x0 0x0 0x1000>;
> >>>>>>>>>/*
> >>>>>>>>>*  ranges is required, then i can get the IORESOURCE_IO <0xe4,4> from "reg = <0x1, 0x000000e4, 4>".
> >>>>>>>>>*
> >>>>>>>>>*/
> >>>>>>>>>	ipmi_0:ipmi at 000000e4{
> >>>>>>>>>		device_type = "ipmi";
> >>>>>>>>>		compatible = "ipmi-bt";
> >>>>>>>>>		reg = <0x1 0x000000e4 0x4>;
> >>>>>>>>>};
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>>This looks wrong: the property above says that the I/O port range is
> >>>>>>>>translated to MMIO address 0x00000000 to 0x00010000, which is not
> >>>>>>>>true on your hardware. I think this needs to be changed in the code
> >>>>>>>>so the ranges property is not required for I/O ports.
> >>>>>>>>
> >>>>>>>>>drivers\of\address.c
> >>>>>>>>>static int __of_address_to_resource(struct device_node *dev,
> >>>>>>>>>                  const __be32 *addrp, u64 size, unsigned int flags,
> >>>>>>>>>                  const char *name, struct resource *r)
> >>>>>>>>>{
> >>>>>>>>>          u64 taddr;
> >>>>>>>>>
> >>>>>>>>>          if ((flags & (IORESOURCE_IO | IORESOURCE_MEM)) == 0)
> >>>>>>>>>                  return -EINVAL;
> >>>>>>>>>          taddr = of_translate_address(dev, addrp);
> >>>>>>>>>          if (taddr == OF_BAD_ADDR)
> >>>>>>>>>                  return -EINVAL;
> >>>>>>>>>          memset(r, 0, sizeof(struct resource));
> >>>>>>>>>          if (flags & IORESOURCE_IO) {
> >>>>>>>>>                  unsigned long port;
> >>>>>>>>>
> >>>>>>>>>/*****************************************************************/
> >>>>>>>>>/*legacy port(< 0x1000) is reserved, and need no translation here*/
> >>>>>>>>>/*****************************************************************/
> >>>>>>>>>                  if(taddr + size < PCIBIOS_MIN_IO){
> >>>>>>>>>                          r->start = taddr;
> >>>>>>>>>                          r->end = taddr + size - 1;
> >>>>>>>>>                  }
> >>>>>>>>
> >>>>>>>>I don't like having a special case based on the address here,
> >>>>>>>>the same kind of hack might be needed for PCI I/O spaces in
> >>>>>>>>hardware that uses an indirect method like your LPC bus
> >>>>>>>>does, and the code above will not work on any LPC implementation
> >>>>>>>>that correctly multiplexes its I/O ports with the first PCI domain.
> >>>>>>>>
> >>>>>>>>I think it would be better to avoid translating the port into
> >>>>>>>>a physical address to start with just to translate it back into
> >>>>>>>>a port number, what we need instead is the offset between the
> >>>>>>>>bus specific port number and the linux port number. I've added
> >>>>>>>>Liviu to Cc, he wrote this code originally and may have some idea
> >>>>>>>>of how we could do that.
> >>>>>>>
> >>>>>>>Hi,
> >>>>>>
> >>>>>>Hi Liviu,
> >>>>>>
> >>>>>>Thanks for reviewing this.
> >>>>>>
> >>>>>>>
> >>>>>>>Getting back to work after a longer holiday, my brain might not be running
> >>>>>>>at full speed here, so I'm trying to clarify things a bit here.
> >>>>>>>
> >>>>>>>It looks to me like Rongrong is trying to trap the inb()/outb() calls that he
> >>>>>>>added to arm64 by patch 1/3 and redirect those operations to the memory
> >>>>>>>mapped LPC driver. I think the whole redirection and registration of inb/outb
> >>>>>>>ops can be made cleaner, so that the general concept resembles the DMA ops
> >>>>>>>registration? (I have this mental picture that what Rongrong is trying to do
> >>>>>>>is similar to what a DMA engine does, except this is slowing down things to
> >>>>>>>byte level). If that is done properly in the parent node, then we should not
> >>>>>>>care what the PCIBIOS_MIN_IO value is as the inb()/outb() calls will always
> >>>>>>>go through the redirection for the children.
> >>>>>>>
> >>>>>>>As for the ranges property: does he wants the ipmi-bt driver to see in the
> >>>>>>>reg property the legacy ISA I/O ports values or the CPU addresses? If the former,
> >>>>>>>then I agree that the range property should not be required, but also the
> >>>>>>>reg values need to be changed (drop the top bit). If the later, then the
> >>>>>>>ranges property is required to do the proper translation.
> >>>>>>
> >>>>>>The former, thanks.
> >>>>>>
> >>>>>>>
> >>>>>>>Rongrong, removing the ranges property and with a reg = <0xe4 0x4> property
> >>>>>>>in the ipmi-bt node, what IO_RESOURCE type resources do you get back from
> >>>>>>>the of_address_to_resource() translation?
> >>>>>>
> >>>>>>I want to get IORESOURCE_IO type resource, but if the parent node drop the
> >>>>>>"rangs" property, the of_address_to_resource() translation will return with -EINVAL.
> >>>>>
> >>>>>Have you tracked what part of the code is sensitive to the presence of "ranges"
> >>>>>property? Does of_get_address() call returns the IO_RESOURCE flag set without "ranges"?
> >>>>>
> >>>>
> >>>>
> >>>>Yes, IO_RESOURCE flag can be get without "ranges".
> >
> >Earlier, you said this ^
> >
> >>>>I tracked the code, it is at of_translate_one(), Below is the calling infomation.
> >>>>
> >>>>of_address_to_resource-> __of_address_to_resource ->of_translate_address->
> >>>>__of_translate_address(dev, in_addr, "ranges")->of_translate_one()
> >>>>
> >>>>
> >>>>static int of_translate_one(struct device_node *parent, struct of_bus *bus,
> >>>>			    struct of_bus *pbus, __be32 *addr,
> >>>>			    int na, int ns, int pna, const char *rprop)
> >>>>{
> >>>>	const __be32 *ranges;
> >>>>	unsigned int rlen;
> >>>>	int rone;
> >>>>	u64 offset = OF_BAD_ADDR;
> >>>>
> >>>>	ranges = of_get_property(parent, rprop, &rlen);
> >>>>	if (ranges == NULL && !of_empty_ranges_quirk(parent)) {
> >>>>		pr_debug("OF: no ranges; cannot translate\n");
> >>>>		return 1;
> >>>>	}
> >>>>	...
> >>>>}
> >>>
> >>>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>;
> >>
> >>But in this condition, I still can't get the right resource type IORESOURCE_IO, I just get
> >>the MMIO resource E4:E7. Please see the url at https://lkml.org/lkml/2016/1/5/199, the empty
> >>ranges has been discussed.
> >
> >So, when you use an empty "ranges" of_get_address() doesn't return the right flags? What resource
> >do you actually get, MMIO is not a valid value.
> 
> 
> 
> I'm sorry I did not describe clearly.
> Without "ranges", of_get_address() will return with valid value, but of_address_to_resource()
> will return with -EINVAL;
> 
> 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) ?

> 	 */
> 	addrp = of_get_address(dev, index, &size, &flags);
> 
> 	if (addrp == NULL)
> 		return -EINVAL;
> 
> 	/* Get optional "reg-names" property to add a name to a resource */
> 	of_property_read_string_index(dev, "reg-names",	index, &name);
> 
> 
> 	/* If it is empty ranges, and the flag is IORESOURCE_MEM then the below  __of_address_to_resource return with valid addr,
> 	 * if it is empty ranges, and the flag is IORESOURCE_IO then return with -EINVAL, because
> 	 *    pci_address_to_pio() will be called when flag is IORESOURCE_IO.
> 	 * if the ranges is absent, then return with -EINVAL.
> 	 */
> 	return __of_address_to_resource(dev, addrp, size, flags, name, r);
> }
> 
> I want to get resource with flag = IORESOURCE_IO, resource.start=0XE4, resource.size=0X4,

Yes, that is what I expect you should get as well.

Liviu

> 
> >
> >Liviu
> >
> >>
> >>--
> >>Regards,
> >>Rongrong
> >>
> >
> 
> 
> -- 
> Regards,
> Rongrong
> 

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

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

[Arnd Bergmann]

On Tuesday 12 January 2016 15:13:35 liviu.dudau at 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.

	Arnd

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

[Benjamin Herrenschmidt]

On Tue, 2016-01-12 at 23:52 +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 15:13:35 liviu.dudau at 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.

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

[Rongrong Zou]

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

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

[Arnd Bergmann]

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

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 11:52:48PM +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 15:13:35 liviu.dudau at 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!  /
  ---------------
    ?\_(?)_/?

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

[Arnd Bergmann]

On Wednesday 13 January 2016 10:10:28 liviu.dudau at 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

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

[liviu.dudau at arm.com]

On Wed, Jan 13, 2016 at 11:18:11AM +0100, Arnd Bergmann wrote:
> On Wednesday 13 January 2016 10:10:28 liviu.dudau at 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!  /
  ---------------
    ?\_(?)_/?

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

[Arnd Bergmann]

On Tuesday 12 January 2016 10:14:18 liviu.dudau at 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

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

[liviu.dudau at arm.com]

On Tue, Jan 12, 2016 at 11:54:59PM +0100, Arnd Bergmann wrote:
> On Tuesday 12 January 2016 10:14:18 liviu.dudau at 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!  /
  ---------------
    ?\_(?)_/?

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

[Arnd Bergmann]

On Wednesday 13 January 2016 10:09:11 liviu.dudau at 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 at 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

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

[Rongrong Zou]

On 2016/1/13 18:09, liviu.dudau at 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 at 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

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

[liviu.dudau at arm.com]

On Wed, Jan 13, 2016 at 07:06:42PM +0800, Rongrong Zou wrote:
> On 2016/1/13 18:09, liviu.dudau at 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 at 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!  /
  ---------------
    ?\_(?)_/?

[PATCH v1 1/3] ARM64 LPC: indirect ISA PORT IO introduced

[Rongrong Zou]

? 2016/1/4 18:11, Will Deacon ??:
> On Wed, Dec 30, 2015 at 10:42:50AM +0100, Arnd Bergmann wrote:
>> On Wednesday 30 December 2015 17:28:20 Rongrong Zou wrote:
>>>>>
>>>>
>>>> config ARM64_INDIRECT_PIO
>>>>        bool
>>>>        help
>>>>          Any driver that provides indirect ISA I/O port access should select
>>>>          this symbol
>>>
>>>
>>>>
>>>> config HISILICON_HI123456_LPC
>>>>        bool "Workaround for nonstandard ISA I/O space on Hisilicon Hi123456 SoC"
>>>>        depends on ARCH_HISI || COMPILE_TEST
>>>>        select ARM64_INDIRECT_PIO
>>>>        help
>>>>          ...
>>> Can it be a submenu of "Platform selection--->Hisilicon SoC Family"?
>>
>> The first should be in the main Kconfig file for arm64, the second one depends
>> on where we put the respective code. I'd say both the Kconfig entry and the file
>> can go into drivers/bus, but arch/arm64/kernel/ is also fine with me.
>
> Unsurprisingly, I'd *much* prefer drivers/bus/ !

I'll move it to drivers/bus on next version of patch, thanks!

>
> Will
>
> .
>