Hi, On 30 November 2016 at 01:14, Z.Q. Hou <zhiqiang....@nxp.com> wrote: > Hi Simon, > > Thanks for your comments! > >> -----Original Message----- >> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass >> Sent: 2016年11月30日 5:41 >> To: Z.Q. Hou <zhiqiang....@nxp.com> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD >> <albert.u.b...@aribaud.net>; Prabhakar Kushwaha >> <prabhakar.kushw...@nxp.com>; Huan Wang-B18965 >> <alison.w...@freescale.com>; Sumit Garg <sumit.g...@nxp.com>; Ruchika >> Gupta <ruchika.gu...@nxp.com>; Saksham Jain >> <saksham.j...@nxp.freescale.com>; york sun <york....@nxp.com>; M.H. Lian >> <minghuan.l...@nxp.com>; Bin Meng <bmeng...@gmail.com>; Mingkai Hu >> <mingkai...@nxp.com> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM >> >> Hi, >> >> On 27 November 2016 at 22:59, Z.Q. Hou <zhiqiang....@nxp.com> wrote: >> > Hi Simon, >> > >> > Thanks for your comments! >> > >> >> -----Original Message----- >> >> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon Glass >> >> Sent: 2016年11月28日 1:02 >> >> To: Z.Q. Hou <zhiqiang....@nxp.com> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD >> >> <albert.u.b...@aribaud.net>; Prabhakar Kushwaha >> >> <prabhakar.kushw...@nxp.com>; Huan Wang-B18965 >> >> <alison.w...@freescale.com>; Sumit Garg <sumit.g...@nxp.com>; >> Ruchika >> >> Gupta <ruchika.gu...@nxp.com>; Saksham Jain >> >> <saksham.j...@nxp.freescale.com>; york sun <york....@nxp.com>; M.H. >> >> Lian <minghuan.l...@nxp.com>; Bin Meng <bmeng...@gmail.com>; >> Mingkai >> >> Hu <mingkai...@nxp.com> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on >> >> DM >> >> >> >> Hi, >> >> >> >> On 24 November 2016 at 02:28, Z.Q. Hou <zhiqiang....@nxp.com> wrote: >> >> > Hi Simon, >> >> > >> >> > Thanks for your comments! >> >> > >> >> >> -----Original Message----- >> >> >> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon >> >> >> Glass >> >> >> Sent: 2016年11月24日 10:21 >> >> >> To: Z.Q. Hou <zhiqiang....@nxp.com> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD >> >> >> <albert.u.b...@aribaud.net>; Prabhakar Kushwaha >> >> >> <prabhakar.kushw...@nxp.com>; Huan Wang-B18965 >> >> >> <alison.w...@freescale.com>; Sumit Garg <sumit.g...@nxp.com>; >> >> Ruchika >> >> >> Gupta <ruchika.gu...@nxp.com>; Saksham Jain >> >> >> <saksham.j...@nxp.freescale.com>; york sun <york....@nxp.com>; >> M.H. >> >> >> Lian <minghuan.l...@nxp.com>; Bin Meng <bmeng...@gmail.com>; >> >> Mingkai >> >> >> Hu <mingkai...@nxp.com> >> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based >> >> >> on DM >> >> >> >> >> >> Hi, >> >> >> >> >> >> On 22 November 2016 at 02:25, Z.Q. Hou <zhiqiang....@nxp.com> >> wrote: >> >> >> > Hi Simon, >> >> >> > >> >> >> > Sorry for my delay respond due to out of the office several >> >> >> > days, and thanks >> >> >> a lot for your comments! >> >> >> > >> >> >> >> -----Original Message----- >> >> >> >> From: s...@google.com [mailto:s...@google.com] On Behalf Of Simon >> >> >> >> Glass >> >> >> >> Sent: 2016年11月18日 9:15 >> >> >> >> To: Z.Q. Hou <zhiqiang....@nxp.com> >> >> >> >> Cc: U-Boot Mailing List <u-boot@lists.denx.de>; Albert ARIBAUD >> >> >> >> <albert.u.b...@aribaud.net>; Prabhakar Kushwaha >> >> >> >> <prabhakar.kushw...@nxp.com>; Huan Wang-B18965 >> >> >> >> <alison.w...@freescale.com>; Sumit Garg <sumit.g...@nxp.com>; >> >> >> Ruchika >> >> >> >> Gupta <ruchika.gu...@nxp.com>; Saksham Jain >> >> >> >> <saksham.j...@nxp.freescale.com>; york sun <york....@nxp.com>; >> >> M.H. >> >> >> >> Lian <minghuan.l...@nxp.com>; Bin Meng <bmeng...@gmail.com>; >> >> >> Mingkai >> >> >> >> Hu <mingkai...@nxp.com> >> >> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver >> >> >> >> based on DM >> >> >> >> >> >> >> >> Hi, >> >> >> >> >> >> >> >> On 16 November 2016 at 02:48, Zhiqiang Hou >> >> >> >> <zhiqiang....@nxp.com> >> >> >> >> wrote: >> >> >> >> > From: Minghuan Lian <minghuan.l...@nxp.com> >> >> >> >> > >> >> >> >> > There are more than five kinds of Layerscape SoCs. >> >> >> >> > unfortunately, PCIe controller of each SoC is a little bit >> >> >> >> > different. In order to avoid too many macro definitions, the >> >> >> >> > patch addes a new implementation of PCIe driver based on DM. >> >> >> >> > PCIe dts node is used to describe the difference. >> >> >> >> > >> >> >> >> > Signed-off-by: Minghuan Lian <minghuan.l...@nxp.com> >> >> >> >> > Signed-off-by: Hou Zhiqiang <zhiqiang....@nxp.com> >> >> >> >> > --- >> >> >> >> > V3: >> >> >> >> > - No change >> >> >> >> > >> >> >> >> > drivers/pci/Kconfig | 8 + >> >> >> >> > drivers/pci/pcie_layerscape.c | 761 >> >> >> >> > ++++++++++++++++++++++++++++++++++++++++++ >> >> >> >> > 2 files changed, 769 insertions(+) >> >> >> >> > >> >> >> >> >> >> >> > +#ifdef CONFIG_FSL_LSCH3 >> >> >> >> >> >> >> >> Can this be a run-time check? >> >> >> > >> >> >> > No, it is for Linux DT fixup and these functions is needed only >> >> >> > by >> >> >> > FSL_LSCH3 >> >> >> SoCs. >> >> >> >> >> >> I mean that you cannot have an #ifdef in a driver - it should be >> >> >> done at run-time by looking at the compatible strings. >> >> > >> >> > This driver work for many platforms, but this fixup is only used by >> >> > FSL_LSCH3 SoCs, if check the compatible string at run-time, the >> >> > fixup will be >> >> still compiled for the platform which doesn't need it. >> >> > Why compile it into the binary for the platform which doesn't need it? >> >> >> >> Because that's how it works. Drivers are drivers for their hardware. >> >> We cannot compile them differently depending on who might use them... >> >> >> >> If this is a big problem you could split the driver into multiple >> >> parts perhaps. But what exactly is the problem here? >> > >> > It isn't a big problem, actually it is just kernel DT fixup function, and >> > it doesn't >> affect the u-boot pcie driver. >> > But the fixup is LSCH3 SoC special, and some macros are only defined in >> header file of LSCH3, e.g. FSL_PEX_STREAM_ID_*. >> > So cannot removed the #ifdef CONFIG_FSL_LSCH3. >> >> Really there should be two separate drivers, with a shared common file for >> common code. >> >> Failing that, is it really impossible to include the extra macros regardless? >> >> If we start putting board-specific #ifdefs in drivers, we have lost the DM >> battle. > > Is it necessary to separate two drivers just for a fixup function? > The fixup is functionally independent with pcie driver, and it works for > kernel pcie driver, if removed the fixup, u-boot pcie driver is still > unabridged and works well but kernel pcie driver won't. > The #ifdefs isn't introduced by Minghuan's refactor based on DM, actually > this refactor removed many #ifdefs. So we do not lost the DM battle.
OK so the #ifdef is only used for the fix-up function? In that case can we move this into a separate file like pcie_layerscape_fixup.c? Then we don't have #ifdef CONFIGs in the driver. Would that work? > >> > >> >> >> >> > >> >> >> > >> >> >> >> >> >> >> >> > +/* >> >> >> >> > + * Return next available LUT index. >> >> >> >> > + */ >> >> >> >> > +static int ls_pcie_next_lut_index(struct ls_pcie *pcie) { >> >> >> >> > + if (pcie->next_lut_index < PCIE_LUT_ENTRY_COUNT) >> >> >> >> > + return pcie->next_lut_index++; >> >> >> >> > + else >> >> >> >> > + return -1; /* LUT is full */ >> >> >> >> >> >> >> >> -ENOSPC? >> >> >> > >> >> >> > Yes, ENOSPC is more reasonable. >> >> >> > >> >> >> >> >> >> >> >> > +} >> >> >> >> > + >> >> >> >> > +/* >> >> >> >> > + * Program a single LUT entry */ static void >> >> >> >> > +ls_pcie_lut_set_mapping(struct ls_pcie *pcie, int index, u32 >> >> >> >> devid, >> >> >> >> > + u32 streamid) { >> >> >> >> > + /* leave mask as all zeroes, want to match all bits */ >> >> >> >> > + lut_writel(pcie, devid << 16, PCIE_LUT_UDR(index)); >> >> >> >> > + lut_writel(pcie, streamid | PCIE_LUT_ENABLE, >> >> >> >> > +PCIE_LUT_LDR(index)); } >> >> >> >> > + >> >> >> >> > +/* returns the next available streamid */ static u32 >> >> >> >> > +ls_pcie_next_streamid(void) { >> >> >> >> > + static int next_stream_id = FSL_PEX_STREAM_ID_START; >> >> >> >> > + >> >> >> >> > + if (next_stream_id > FSL_PEX_STREAM_ID_END) >> >> >> >> > + return 0xffffffff; >> >> >> >> >> >> >> >> Is FSL_PEX_STREAM_ID_END the maximum value, or the number of >> >> values? >> >> >> > >> >> >> > The maximum value for PCIe. >> >> >> > >> >> >> >> > + >> >> >> >> > + return next_stream_id++; } >> >> >> >> > + >> >> >> >> > +/* >> >> >> >> > + * An msi-map is a property to be added to the pci >> >> >> >> > +controller >> >> >> >> > + * node. It is a table, where each entry consists of 4 >> >> >> >> > +fields >> >> >> >> > + * e.g.: >> >> >> >> > + * >> >> >> >> > + * msi-map = <[devid] [phandle-to-msi-ctrl] [stream-id] >> [count] >> >> >> >> > + * [devid] [phandle-to-msi-ctrl] [stream-id] >> >> [count]>; >> >> >> >> > + */ >> >> >> >> > +static void fdt_pcie_set_msi_map_entry(void *blob, struct >> >> >> >> > +ls_pcie >> >> *pcie, >> >> >> >> > + u32 devid, u32 >> >> streamid) { >> >> >> >> > + u32 *prop; >> >> >> >> > + u32 phandle; >> >> >> >> > + int nodeoffset; >> >> >> >> > + >> >> >> >> > + /* find pci controller node */ >> >> >> >> > + nodeoffset = fdt_node_offset_by_compat_reg(blob, >> >> >> >> > + "fsl,ls-pcie", >> >> >> >> > + >> >> >> >> > + pcie->dbi_res.start); >> >> >> >> >> >> >> >> At this point I'm a bit lost, but if this is using driver >> >> >> >> model, you can use >> >> >> >> dev->of_offset >> >> >> > >> >> >> > This function is used to fixup Linux Kernel DT instead of u-boot DT. >> >> >> >> >> >> They should use the same DT. >> >> > >> >> > Yes, Ideally they should, but up to now actually Kernel does not >> >> > use the one u-boot used, so we cannot make sure the offset of the >> >> > nodes are the >> >> same. >> >> > So to ensure the fixup work, get the node offset from kernel DT. >> >> >> >> Is it not possible to change U-Boot to use the kernel DT? It might be less >> work. >> > >> > Since this is used to fixup Kernel DT, and u-boot and Kernel use two >> > copies of >> DT, until the u-boot and kernel use one copy of DT, we must fixup the one >> works for Kernel. >> >> OK. Please add a TODO(email) prominently. > > I'm afraid you're confused. > U-boot and kernel use two copies of DT whether they are the same or not, they > locate in different addresses, and let's name the u-boot used A and kernel > used B. > This function is used to fixup B, so the node-offset must be get from B > instead of A. Because we cannot ensure A and B always are the same. OK I think I am slowly understanding this. So you have a kernel DT fix-up function that you are putting in this driver. The only calls into drivers should be via driver model. As above I suggest putting this in its own file. > >> > >> >> >> >> > >> >> >> >> >> >> > >> >> >> >> >> >> >> >> > + if (nodeoffset < 0) { >> >> >> >> > + #ifdef FSL_PCIE_COMPAT /* Compatible with older >> >> >> >> > + version of dts node */ >> >> >> >> >> >> >> >> Eek! Can't you detect this at run-time? >> >> >> >> >> >> >> > >> >> >> > No, it's Kernel DT fixup, we plan to refactor Layerscape PCIe >> >> >> > Linux driver using the compatible "fsl,ls-pcie", but for now the >> >> >> > macro >> >> >> FSL_PCIE_COMPAT must be defined to fixup Linux DT. >> >> >> >> >> >> I'm still confused by this. I don't see it defined anywhere and it >> >> >> is not a >> >> CONFIG. >> >> >> Can you not detect at run-time when you need to do the fix-up? >> >> > >> >> > Ok, the process is find the node offset by "fsl,ls-pcie" first, if >> >> > failed, find it >> >> again by FSL_PCIE_COMPAT. >> >> > But in the current kernel DT the name of PCIe controller node is >> >> > NOT the "fsl,ls-pcie" which we will refactor layerscape pcie kernel >> >> > driver to use, so far it is the FSL_PCIE_COMPAT which is defined >> >> > according to the >> >> current kernel DT in header file include/configs/ls*.h. >> >> > So it is unable to be detected at run-time, but it will be removed >> >> > when the >> >> kernel driver refactored. >> >> >> >> OK, so how about making this a new CONFIG which you can turn on/off? >> > >> > Yes, will move it to CONFIG_ FSL_PCIE_COMPAT. >> > >> >> > >> >> >> >> >> >> > >> >> >> >> > + nodeoffset = >> >> fdt_node_offset_by_compat_reg(blob, >> >> >> >> > + >> >> >> >> FSL_PCIE_COMPAT, >> >> >> >> > + >> >> >> >> pcie->dbi_res.start); >> >> >> >> > + if (nodeoffset < 0) >> >> >> >> > + return; >> >> >> >> > + #else >> >> >> >> > + return; >> >> >> >> > + #endif >> >> >> >> > + } >> >> >> >> > + >> >> >> >> > + /* get phandle to MSI controller */ >> >> >> >> > + prop = (u32 *)fdt_getprop(blob, nodeoffset, >> >> >> >> > + "msi-parent", 0); >> >> >> >> >> >> >> >> fdtdec_getint() >> >> >> > >> >> >> > The fdtdec_get_int() is not suit for this case, because the >> >> >> > value of >> >> >> "msi-parent" is an index of gic-its, so there isn't a default value. >> >> >> >> >> >> Try: >> >> >> >> >> >> val = fdtdec_get_int(blob, nodeoffset, "msi-parent", -1) >> >> >> if (val == -1) { >> >> >> debug(...); >> >> >> return -EINVAL; >> >> >> } >> >> >> >> >> > >> >> > Any benefit compared with fdt_getprop? I'm confused by this >> >> > function, what >> >> if the correct value equal to the given default value? >> >> >> >> You choose an invalid default. If there isn't one then you cannot use >> >> this function. The benefit is that it avoids the be32_to_cpu(). >> > >> > The value of this property is a reference of other node and don't know >> > which >> is the invalid value. >> > Do you have any suggestion about this case? >> >> Well, phandles cannot be < 0, so how about -1? > > No, it can be < 0. > Made an experiment that added "test = <0xffffffff>;" to DT then the > fdtdec_get_int() return -1. > So, avoid to use it when didn't know an invalid value. Yes but a phandle will never be -1. My point is that if the phandle is missing, the function will return -1, so you can detect that case. All valid phandles are >= 0. Anyway if you like the code as is, it's fine with me. It just seems unnecessarily complicated. > >> > >> >> > >> >> >> > >> >> >> >> >> >> >> >> > + if (prop == NULL) { >> >> >> >> > + printf("\n%s: ERROR: missing msi-parent: >> >> PCIe%d\n", >> >> >> >> > + __func__, pcie->idx); >> >> >> >> > + return; >> >> >> >> >> >> >> >> Return an error error and check it. >> >> >> > >> >> >> > This function is used to fixup Linux DT, so this error won't >> >> >> > block the u-boot >> >> >> process, and I think an error message is enough. >> >> >> >> >> >> If it is an error it should return an error. If it is just a >> >> >> warning it should say so, ideally using debug(). As it is, it is >> >> >> very confusing for the user to get this message. >> >> > >> >> > Will replace with debug(). >> >> > >> >> >> > >> >> >> >> > + } >> >> >> >> > + phandle = be32_to_cpu(*prop); >> >> >> >> >> >> >> >> fdt32_to_cpu() >> >> >> >> >> >> >> > >> >> >> > Yes, better to use fdt32_to_cpu. >> >> >> >> >> >> But where do you use that value? Also. consider >> fdtdec_lookup_phandle(). >> >> > >> >> > Thanks for your tip, just the value of this phandle is used, see the >> >> > lines >> below. >> >> >> >> OK I see. >> >> >> >> > >> >> >> > >> >> >> >> > + >> >> >> >> > + /* set one msi-map row */ >> >> >> >> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", devid); >> >> >> >> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", >> phandle); >> >> >> >> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", >> >> streamid); >> >> >> >> > + fdt_appendprop_u32(blob, nodeoffset, "msi-map", 1); } >> >> >> >> > + >> >> >> >> > +static void fdt_fixup_pcie(void *blob) >> >> >> >> >> >> >> >> This is a pretty horrible function. What is it for? >> >> >> > >> >> >> > Kernel DT fixup. >> >> >> >> >> >> OK, well please add some comments! >> >> > >> >> > Will comment it. >> > > > Regards, > Zhiqiang _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot