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. > >> >> > +/* >> > + * 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. > >> >> > + 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? > >> > + 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; } > >> >> > + 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. > >> > + } >> > + 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(). > >> > + >> > + /* 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! [...] Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot