RE: [PATCH 4/6] arm64/ls1043a: add DTS for Freescale LS1043A SoC

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Mark,

> -----Original Message-----
> From: Mark Rutland [mailto:mark.rutland@xxxxxxx]
> Sent: 2015年9月22日 9:24
> To: Hou Zhiqiang-B48286; marc.zyngier@xxxxxxx
> Cc: linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; Catalin Marinas; Will Deacon;
> linux-i2c@xxxxxxxxxxxxxxx; linux-watchdog@xxxxxxxxxxxxxxx; linux-
> doc@xxxxxxxxxxxxxxx; linux-clk@xxxxxxxxxxxxxxx; Xie Shaohui-B21989;
> corbet@xxxxxxx; Sharma Bhupesh-B45370; mturquette@xxxxxxxxxxxx; wsa@the-
> dreams.de; sboyd@xxxxxxxxxxxxxx; wim@xxxxxxxxx; Song Wenbin-B53747; Wood
> Scott-B07421; Hu Mingkai-B21284; Li Yang-Leo-R58472
> Subject: Re: [PATCH 4/6] arm64/ls1043a: add DTS for Freescale LS1043A SoC
> 
> Hi,
> 
> > +/memreserve/ 0x80000000 0x00010000;
> 
> Why is this necessary?
 
This memory region is pre-reserved for the spin-table/psci, although didn't add
Enable method of secondary cores.

> If this is necessary, please add a comment stating what this is for.
> 
> > +               cpu@3 {
> > +                       device_type = "cpu";
> > +                       compatible = "arm,cortex-a53";
> > +                       reg = <0x0 0x3>;
> > +                       clocks = <&clockgen 1 0>;
> > +               };
> 
> Missing enable-method properties on all the secondary CPUs.
> 

There are two methods (spin-table and psci) to bring up secondary cores, which one
do you think is better?

> [...]
> 
> > +       gic: interrupt-controller@1400000 {
> > +               compatible = "arm,gic-400";
> > +               #interrupt-cells = <3>;
> > +               interrupt-controller;
> > +               reg = <0x0 0x1401000 0 0x1000>, /* GICD */
> > +                     <0x0 0x1402000 0 0x1000>, /* GICC */
> > +                     <0x0 0x1404000 0 0x2000>, /* GICH */
> > +                     <0x0 0x1406000 0 0x2000>; /* GICV */
> > +               interrupts = <1 9 0xf08>;
> > +       };
> 
> These sizes don't look right.
> 
> To the best of my knowledge, GICC (and GICV) should be 0x2000 in size,
> while GICD and GICH should be 0x1000.

Thanks for your correction.

> 
> [...]
> 
> > +       ifc: ifc@1530000 {
> > +               compatible = "fsl,ifc", "simple-bus";
> > +               reg = <0x0 0x1530000 0x0 0x10000>;
> > +               interrupts = <0 43 0x4>;
> > +       };
> 
> Why simple-bus?

There are 3 child node located in dtsi file that should be created and added
to platform device list.

> 
> > +       clockgen: clocking@1ee1000 {
> > +               compatible = "fsl,ls1043a-clockgen";
> > +               reg = <0x0 0x1ee1000 0x0 0x1000>;
> > +               #clock-cells = <2>;
> > +               clocks = <&sysclk>;
> > +               sysclk: sysclk {
> > +                       compatible = "fixed-clock";
> > +                       #clock-cells = <0>;
> > +                       clock-frequency = <100000000>;
> > +                       clock-output-names = "sysclk";
> > +               };
> > +       };
> 
> Why does this fixed clock live under the clockgen? It should live
> directly under the root node.
> 

Thanks and will move it out in v2.

> [...]
> 
> > +       pcie@3400000 {
> > +               compatible = "fsl,ls1043a-pcie", "snps,dw-pcie";
> > +               reg = <0x00 0x03400000 0x0 0x00100000   /* controller
> registers */
> > +                      0x40 0x00000000 0x0 0x00002000>; /*
> configuration space */
> > +               reg-names = "regs", "config";
> > +               interrupts = <0 118 0x4>, /* controller interrupt */
> > +                            <0 117 0x4>; /* PME interrupt */
> > +               interrupt-names = "intr", "pme";
> > +               #address-cells = <3>;
> > +               #size-cells = <2>;
> > +               device_type = "pci";
> > +               num-lanes = <4>;
> > +               bus-range = <0x0 0xff>;
> > +               ranges = <0x81000000 0x0 0x00000000 0x40 0x00010000 0x0
> 0x00010000   /* downstream I/O */
> > +                         0x82000000 0x0 0x40000000 0x40 0x40000000 0x0
> 0x40000000>; /* non-prefetchable memory */
> > +               msi-parent = <&msi1>;
> > +               #interrupt-cells = <1>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0000 0 0 1 &gic 0 110 0x4>,
> > +                               <0000 0 0 2 &gic 0 111 0x4>,
> > +                               <0000 0 0 3 &gic 0 112 0x4>,
> > +                               <0000 0 0 4 &gic 0 113 0x4>;
> > +       };
> > +
> > +       pcie@3500000 {
> > +               compatible = "fsl,ls1043a-pcie", "snps,dw-pcie";
> > +               reg = <0x00 0x03500000 0x0 0x00100000   /* controller
> registers */
> > +                      0x48 0x00000000 0x0 0x00002000>; /*
> configuration space */
> > +               reg-names = "regs", "config";
> > +               interrupts = <0 128 0x4>,
> > +                            <0 127 0x4>;
> > +               interrupt-names = "intr", "pme";
> > +               #address-cells = <3>;
> > +               #size-cells = <2>;
> > +               device_type = "pci";
> > +               num-lanes = <2>;
> > +               bus-range = <0x0 0xff>;
> > +               ranges = <0x81000000 0x0 0x00000000 0x48 0x00010000 0x0
> 0x00010000   /* downstream I/O */
> > +                         0x82000000 0x0 0x40000000 0x48 0x40000000 0x0
> 0x40000000>; /* non-prefetchable memory */
> > +               msi-parent = <&msi2>;
> > +               #interrupt-cells = <1>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0000 0 0 1 &gic 0 120  0x4>,
> > +                               <0000 0 0 2 &gic 0 121 0x4>,
> > +                               <0000 0 0 3 &gic 0 122 0x4>,
> > +                               <0000 0 0 4 &gic 0 123 0x4>;
> > +       };
> > +
> > +       pcie@3600000 {
> > +               compatible = "fsl,ls1043a-pcie", "snps,dw-pcie";
> > +               reg = <0x00 0x03600000 0x0 0x00100000   /* controller
> registers */
> > +                      0x50 0x00000000 0x0 0x00002000>; /*
> configuration space */
> > +               reg-names = "regs", "config";
> > +               interrupts = <0 162 0x4>,
> > +                            <0 161 0x4>;
> > +               interrupt-names = "intr", "pme";
> > +               #address-cells = <3>;
> > +               #size-cells = <2>;
> > +               device_type = "pci";
> > +               num-lanes = <2>;
> > +               bus-range = <0x0 0xff>;
> > +               ranges = <0x81000000 0x0 0x00000000 0x50 0x00010000 0x0
> 0x00010000   /* downstream I/O */
> > +                         0x82000000 0x0 0x40000000 0x50 0x40000000 0x0
> 0x40000000>; /* non-prefetchable memory */
> > +               msi-parent = <&msi3>;
> > +               #interrupt-cells = <1>;
> > +               interrupt-map-mask = <0 0 0 7>;
> > +               interrupt-map = <0000 0 0 1 &gic 0 154 0x4>,
> > +                               <0000 0 0 2 &gic 0 155 0x4>,
> > +                               <0000 0 0 3 &gic 0 156 0x4>,
> > +                               <0000 0 0 4 &gic 0 157 0x4>;
> > +       };
> 
> You wait ages for a bus, then three show up at once...
> 
> > +
> > +       memory@80000000 {
> > +               device_type = "memory";
> > +               reg = <0x0 0x80000000 0 0x80000000>;
> > +                     /* DRAM space 1 - 2 GB DRAM */
> 
> I don't understand the comment. This describes 2GB at 2GB.
> 

Yes, there is a 2GB space for DRAM from address 2G.
The DRAM address 0x0 will be remapped to SoC address 2G, this remap is done by hardware.

> Does the bootloader overwrite this?

No.
 
Thanks,
Zhiqiang
?韬{.n?????%??檩??w?{.n???{饼???骅w*jg????????G??⒏⒎?:+v????????????"??????



[Index of Archives]     [Kernel Newbies]     [Security]     [Netfilter]     [Bugtraq]     [Linux FS]     [Yosemite Forum]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Samba]     [Video 4 Linux]     [Device Mapper]     [Linux Resources]

  Powered by Linux