[PATCH v4 1/2] ARM: shmobile: r8a7790: add internal PCI bridge nodes

[arnd@arndb.de (Arnd Bergmann)]

On Saturday 21 June 2014 01:00:21 Sergei Shtylyov wrote:
> 
> On 06/21/2014 12:51 AM, Arnd Bergmann wrote:
> 
> >> +       pci0: pci at ee090000 {
> >> +               compatible = "renesas,pci-r8a7790";
> >> +               clocks = <&mstp7_clks R8A7790_CLK_EHCI>;
> >> +               reg = <0x0 0xee090000 0x0 0xc00>,
> >> +                     <0x0 0xee080000 0x0 0x1100>;
> >> +               interrupts = <0 108 IRQ_TYPE_LEVEL_HIGH>;
> >> +               status = "disabled";
> >> +
> >> +               bus-range = <0 0>;
> >> +               #address-cells = >;
> >> +               #size-cells = <2>;
> >> +               #interrupt-cells = <1>;
> >> +               interrupt-map-mask = <0xff00 0 0 0x7>;
> >> +               interrupt-map = <0x0000 0 0 1 &gic 0 108 IRQ_TYPE_LEVEL_HIGH
> >> +                                0x0800 0 0 1 &gic 0 108 IRQ_TYPE_LEVEL_HIGH
> >> +                                0x1000 0 0 2 &gic 0 108 IRQ_TYPE_LEVEL_HIGH>;
> >> +       };
> 
> > Hmm, this device node is not actually compliant to the PCI binding,
> > it needs a "ranges" property that can be used to look up the memory
> > and I/O space windows.
> 
>     The PCI driver doesn't support I/O space.

Well, whichever windows are registered by the driver should come
from the ranges property.

> > It also needs a device_type property.
> 
>     Hm, are you sure about that? I thought only PCI devices should have it...

Yes, pretty sure it's needed in both the host controller and the
devices.

> > I realize that the driver doesn't currently use them, but you should
> > adhere to the binding anyway, so we can fix the driver at some point.
> 
>     Sigh, agreed about the need to fix the driver. Too bad you've spoken up 
> only now though. And you've ACKed the DT bindings without those properties 
> documented or even present in an example... 

Yes, I realized that too late as well, sorry about it. For some reason
I only looked at the interrupt-map being done right and forgot to
check the ranges.

We definitely need to move the code handling the ranges into a common
location to avoid this mistake in the future.

	Arnd