[PATCH 1/3] ARM: CSR: Adding CSR SiRFprimaII board support

[Barry Song]

2011/7/6 Arnd Bergmann <arnd at arndb.de>:
> On Wednesday 06 July 2011, Barry Song wrote:
>
>> > I would normally recommend defining the ranges so that addresses are local
>> > to the respective bus, like
>> >
>> >        axi {
>> >                ranges = <0 0x40000000 0x80000000>;
>> >
>> >                sys-iobg {
>> >                        ranges = <0 0x48000000 0x40000>;
>> >                        clock-controller at 0x88000000 {
>> >                               compatible = "sirf,prima2-clkc";
>> >                               reg = <0 0x1000>;
>> >                        }
>> >
>> >                        reset-controller at 0x88010000 {
>> >                               compatible = "sirf,prima2-rstc";
>> >                               reg = <0x10000 0x1000>;
>> >                        };
>> >                }
>> >        }
>>
>> i am not sure whether it make us a little more difficult to know the
>> real address at first glance.we will need to calculate.
>> all addresses are 1:1 mapped in this chip. bus map can work even
>> though we only give "ranges;" without real "ranges = <0x....>;".
>
> So each iobg still passes down the entire 32-bit address?

yes. each iobg is basically transparent for address transferring.
ranges = <0x40000000 0x40000000 0x80000000>; should be ok.

>
> Note that you never have to do the calculation in the driver
> source, of_iomap and the resource logic both take care of this.
>
> There are multiple ways to handle this, and an empty ranges property
> usually works fine, but I find that less readable.
>
> Another way to handle these is to have a separate range for
> each child bus, as in arch/powerpc/boot/dts/gef_ppc9a.dts
>
> To stay in the example, this would mean doing something like
>
>        axi {
>                #address-cells = <2>;
>                #size-cells = <1>;
>
>                ranges = <0 0 0x80000000 0x08000000 // axi devices
>                          1 0 0x88000000 0x08000000 // sys-iobg
>                          2 0 0x90000000 0x00010000 // mem-iobg
>                          3 0 0x90010000 0x07fe0000 // disp-iobg
>                          ... >;
>
>                l2-cache-controller at 80040000 {
>                        compatible = "arm,pl310-cache";
>                        reg = <0 0x40000 0x1000>;
>                        interrupts = <59>;
>                };
>
>                sys-iobg {
>                        #address-cells = <1>;
>                        #size-cells = <1>;
>                        ranges = <1 0 0 0x40000>;
>                        clock-controller at 88000000 {
>                               compatible = "sirf,prima2-clkc";
>                               reg = <0 0x1000>;
>                        }
>
>                        reset-controller at 88010000 {
>                               compatible = "sirf,prima2-rstc";
>                               reg = <0x10000 0x1000>;
>                        };
>                }
>        }
>
>
>
>> >> +
>> >> +                     graphics-iobg {
>> >> +                             compatible = "simple-bus";
>> >> +                             #address-cells = <1>;
>> >> +                             #size-cells = <1>;
>> >> +                             ranges = <0x98000000 0x98000000 0x8000000>;
>> >> +
>> >> +                             graphics at 0x98000000 {
>> >> +                                     compatible = "sirf,prima2-graphics";
>> >> +                                     reg = <0x98000000 0x8000000>;
>> >> +                                     interrupts = <6>;
>> >> +                             };
>> >> +                     };
>> >
>> > Are the display and graphics units CSR developments? If the GPU is
>> > in fact licensed from someone else (powervr, arm, ...), you should
>> > probably list the actual name of the device.
>>
>> GPU is powervr sgx 531, so could we define compatible as "powervr,sgx531"?
>
> Probably yes. You should have a look if there are already bindings for
> this that define other attributes. Also, if there is any customization
> inside of the chip, you should have another more specific identifier
> that makes it possible that this is the version that csr has modified.
>
>> >> +                     multimedia-iobg {
>> >> +                             compatible = "simple-bus";
>> >> +                             #address-cells = <1>;
>> >> +                             #size-cells = <1>;
>> >> +                             ranges = <0xa0000000 0xa0000000 0x8000000>;
>> >> +
>> >> +                             multimedia at 0xa0000000 {
>> >> +                                     compatible = "sirf,prima2-multimedia";
>> >> +                                     reg = <0xa0000000 0x8000000>;
>> >> +                                     interrupts = <5>;
>> >> +                             };
>> >> +                     };
>> >
>> > "multimedia" sounds like a too generic term. What does this do?
>>
>>  video decoding.
>
> sirf,prima2-video-codec is probably better than, but if anyone has other
> suggestions, you could use something else.
>
>> > Are these proprietary uarts, or are they compatible to 8250 and the
>> > like? You might want to set a clock-frequency property as of_serial.c
>> > uses.
>>
>> it is not compatible with 8250 .
>
> ok
>
>> > Are these rtc implementations related? From the register layout, I would
>> > guess that they are supposed to be used by the same driver, so it's
>> > probably a good idea to add a "compatible" property with a common name
>> > for all three.
>>
>> in fact, because they are slow, they can't be accessed by mapped
>> address directly, the only common point they have is we need to access
>> them through mapped address in rtc-iobg indirectly just like we access
>> i2c/spi/nand devices.
>>
>> they are three different devices with different purpose and register
>> layout in fact.
>
> Ok.
>
>        Arnd
>