Hi Arnd,
thanks.
2011/7/6 Arnd Bergmann <[email protected]>:
> On Wednesday 06 July 2011, Barry Song wrote:
>> From: Binghua Duan <[email protected]>
>>
>> SiRFprimaII is the latest generation application processor from CSR’s
>> Multifunction SoC product family. Designed around an ARM cortex A9 core,
>> high-speed memory bus, advanced 3D accelerator and full-HD multi-format
>> video decoder, SiRFprimaII is able to meet the needs of complicated
>> applications for modern multifunction devices that require heavy concurrent
>> applications and fluid user experience. Integrated with GPS baseband,
>> analog and PMU, this new platform is designed to provide a cost effective
>> solution for Automotive and Consumer markets.
>>
>> This patch adds the basic support for this SoC and EVB board based on device
>> tree. It is following the ZYNQ of Grant Likely in some degree.
>>
>> Signed-off-by: Binghua Duan <[email protected]>
>> Signed-off-by: Rongjun Ying <[email protected]>
>> Signed-off-by: Zhiwu Song <[email protected]>
>> Signed-off-by: Yuping Luo <[email protected]>
>> Signed-off-by: Bin Shi <[email protected]>
>> Signed-off-by: Huayi Li <[email protected]>
>> Signed-off-by: Barry Song <[email protected]>
>
> Reviewed-by: Arnd Bergmann <[email protected]>
>
> I think this is good for 3.1, but there are still a few things about
> the device tree file could be improved.
>
>> + axi {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x40000000 0x40000000 0x80000000>;
>> +
>> + sirfsoc-iobus {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x40000000 0x40000000 0x80000000>;
>> +
>> + l2-cache-controller@0x80040000 {
>> + compatible = "arm,pl310-cache";
>> + reg = <0x80040000 0x1000>;
>> + interrupts = <59>;
>> + };
>> +
>> + intc: interrupt-controller@0x80020000 {
>> + #interrupt-cells = <1>;
>> + interrupt-controller;
>> + compatible = "sirf,prima2-intc";
>> + reg = <0x80020000 0x1000>;
>> + };
>> +
>> + sys-iobg {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x88000000 0x88000000 0x40000>;
>> +
>> + clock-controller@0x88000000 {
>> + compatible = "sirf,prima2-clkc";
>> + reg = <0x88000000 0x1000>;
>> + interrupts = <3>;
>> + };
>
>
> The axi bus and the sirfsoc-iobus seem to be identical in their scope,
> so it's probably enough to model one of them.
ok.
>
> 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@0x88000000 {
> compatible = "sirf,prima2-clkc";
> reg = <0 0x1000>;
> }
>
> reset-controller@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....>;".
i also find i missed grant's comment about deleting "0x" after "@" in
this patch.
>
>
>> + disp-iobg {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x90010000 0x90010000 0x30000>;
>> +
>> + display@0x90010000 {
>> + compatible = "sirf,prima2-lcd";
>> + reg = <0x90010000 0x20000>;
>> + interrupts = <30>;
>> + };
>> +
>> + vpp@0x90020000 {
>> + compatible = "sirf,prima2-vpp";
>> + reg = <0x90020000 0x10000>;
>> + interrupts = <31>;
>> + };
>> + };
>> +
>> + graphics-iobg {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0x98000000 0x98000000 0x8000000>;
>> +
>> + graphics@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"?
>
>> + multimedia-iobg {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0xa0000000 0xa0000000 0x8000000>;
>> +
>> + multimedia@0xa0000000 {
>> + compatible = "sirf,prima2-multimedia";
>> + reg = <0xa0000000 0x8000000>;
>> + interrupts = <5>;
>> + };
>> + };
>
> "multimedia" sounds like a too generic term. What does this do?
video decoding.
>
>> + uart0: uart@0xb0050000 {
>> + cell-index = <0>;
>> + compatible = "sirf,prima2-uart";
>> + reg = <0xb0050000 0x10000>;
>> + interrupts = <17>;
>> + };
>> +
>> + uart1: uart@0xb0060000 {
>> + cell-index = <1>;
>> + compatible = "sirf,prima2-uart";
>> + reg = <0xb0060000 0x10000>;
>> + interrupts = <18>;
>> + };
>> +
>> + uart2: uart@0xb0070000 {
>> + cell-index = <2>;
>> + compatible = "sirf,prima2-uart";
>> + reg = <0xb0070000 0x10000>;
>> + interrupts = <19>;
>> + };
>
> 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 .
>
>> + rtc-iobg {
>> + compatible = "sirf,prima2-rtciobg",
>> "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + reg = <0x80030000 0x10000>;
>> +
>> + gpsrtc@0x1000 {
>> + compatible = "sirf,prima2-gpsrtc";
>> + reg = <0x1000 0x1000>;
>> + interrupts = <55 56 57>;
>> + };
>> +
>> + sysrtc@0x2000 {
>> + compatible = "sirf,prima2-sysrtc";
>> + reg = <0x2000 0x1000>;
>> + interrupts = <52 53 54>;
>> + };
>> +
>> + pwrc@0x3000 {
>> + compatible = "sirf,prima2-pwrc";
>> + reg = <0x3000 0x1000>;
>> + interrupts = <32>;
>> + };
>> + };
>
> 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.
>
>> + uus-iobg {
>> + compatible = "simple-bus";
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>> + ranges = <0xb8000000 0xb8000000 0x40000>;
>> +
>> + usb0: usb@0xb00E0000 {
>> + compatible = "sirf,prima2-usb";
>> + reg = <0xb8000000 0x10000>;
>> + interrupts = <10>;
>> + };
>> +
>> + usb1: usb@0xb00f0000 {
>> + compatible = "sirf,prima2-usb";
>> + reg = <0xb8010000 0x10000>;
>> + interrupts = <11>;
>> + };
>
> Is the usb implementation compatible to an existing one? Many SoCs
> use one of ehci, ohci or musb. If that's the case, you should look
> at the respective bindings.
>
>> + sata@0xb00f0000 {
>> + compatible = "sirf,prima2-sata";
>> + reg = <0xb8020000 0x10000>;
>> + interrupts = <37>;
>> + };
>
> Same thing here. Most sata controllers are compatible to some
> standard implementation.
ok. i see :-). let me have some check with ic guys and send v4 with these fixes.
>
>> + security@0xb00f0000 {
>> + compatible = "sirf,prima2-security";
>> + reg = <0xb8030000 0x10000>;
>> + interrupts = <42>;
>> + };
>> + };
>> + };
>> + };
>> +};
>
> Arnd
> _______________________________________________
> devicetree-discuss mailing list
> [email protected]
> https://lists.ozlabs.org/listinfo/devicetree-discuss
>
Thanks
barry
_______________________________________________
devicetree-discuss mailing list
[email protected]
https://lists.ozlabs.org/listinfo/devicetree-discuss
