Hello Arnd, Thank you for reviewing! I'm seriously Sorry for the late response! I was on business travel on a project and had been very busy for a long time. Please see inline. Regards, Shaohui > -----Original Message----- > From: Arnd Bergmann [mailto:arnd@xxxxxxxx] > Sent: Friday, July 08, 2016 8:00 PM > To: shh.xie@xxxxxxxxx > Cc: devicetree@xxxxxxxxxxxxxxx; robh+dt@xxxxxxxxxx; mark.rutland@xxxxxxx; > pawel.moll@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx; > linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; catalin.marinas@xxxxxxx; > will.deacon@xxxxxxx; Mingkai Hu <mingkai.hu@xxxxxxx>; Mihai Emilian Bantea > <mihai.bantea@xxxxxxx>; Qianyu Gong <qianyu.gong@xxxxxxx>; Minghuan Lian > <minghuan.lian@xxxxxxx>; Zhiqiang Hou <zhiqiang.hou@xxxxxxx>; Shaohui Xie > <shaohui.xie@xxxxxxx> > Subject: Re: [PATCH 1/5] arm64: dts: add QorIQ LS1046A SoC support > > On Friday, July 8, 2016 6:15:40 PM CEST shh.xie@xxxxxxxxx wrote: > > + > > + memory@80000000 { > > + device_type = "memory"; > > + reg = <0x0 0x80000000 0 0x80000000>; > > + /* DRAM space 1, size: 2GiB DRAM */ > > + }; > > The memory size is usually in the .dts file, unless this is on-chip eDRAM. [S.H] The memory size will be fixup by U-boot, if it's not proper to have it in .dtsti, How about remove it? > > + clockgen: clocking@1ee1000 { > > + compatible = "fsl,ls1046a-clockgen"; > > > + scfg: scfg@1570000 { > > + compatible = "fsl,ls1046a-scfg", "syscon"; > > > + dcfg: dcfg@1ee0000 { > > + compatible = "fsl,ls1046a-dcfg", "syscon"; > > None of the fsl,ls1046a-* devices seem to have any binding documentation. [S.H] Will update binding for fsl,ls1046-* devices in next version. > > > + wdog0: wdog@2ad0000 { > > watchdog@2ad0000 [S.H] Will fix it. > > > + usb0: usb3@2f00000 { > > usb@2f00000 [S.H] Will fix it. > > > + pcie@3400000 { > > + compatible = "fsl,ls1046a-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 */ > > No prefetchable memory area? > > > + msi-parent = <&msi>; > > You seem to have a gic-400, could you use that as the MSI sink instead? > > > + #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 110 0x4>, > > + <0000 0 0 3 &gic 0 110 0x4>, > > + <0000 0 0 4 &gic 0 110 0x4>; > > + }; > > > > If the four interrupts are all the same, why do you have separate entries? [S.H] We are working on refining the MSI driver and the MSI node will also be changed, so the MSI and PCIe node will be added together with the MSI driver updates. In the next version, I will drop the MSI node and PCIe node. Thank you! -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
