Hi Simon, Thanks for your comments!
> -----Original Message----- > From: [email protected] [mailto:[email protected]] On Behalf Of Simon Glass > Sent: 2016年11月30日 5:41 > To: Z.Q. Hou <[email protected]> > Cc: U-Boot Mailing List <[email protected]>; Albert ARIBAUD > <[email protected]>; Prabhakar Kushwaha > <[email protected]>; Huan Wang-B18965 > <[email protected]>; Sumit Garg <[email protected]>; Ruchika > Gupta <[email protected]>; Saksham Jain > <[email protected]>; york sun <[email protected]>; M.H. Lian > <[email protected]>; Bin Meng <[email protected]>; Mingkai Hu > <[email protected]> > Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM > > Hi, > > On 27 November 2016 at 22:59, Z.Q. Hou <[email protected]> wrote: > > Hi Simon, > > > > Thanks for your comments! > > > >> -----Original Message----- > >> From: [email protected] [mailto:[email protected]] On Behalf Of Simon Glass > >> Sent: 2016年11月28日 1:02 > >> To: Z.Q. Hou <[email protected]> > >> Cc: U-Boot Mailing List <[email protected]>; Albert ARIBAUD > >> <[email protected]>; Prabhakar Kushwaha > >> <[email protected]>; Huan Wang-B18965 > >> <[email protected]>; Sumit Garg <[email protected]>; > Ruchika > >> Gupta <[email protected]>; Saksham Jain > >> <[email protected]>; york sun <[email protected]>; M.H. > >> Lian <[email protected]>; Bin Meng <[email protected]>; > Mingkai > >> Hu <[email protected]> > >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on > >> DM > >> > >> Hi, > >> > >> On 24 November 2016 at 02:28, Z.Q. Hou <[email protected]> wrote: > >> > Hi Simon, > >> > > >> > Thanks for your comments! > >> > > >> >> -----Original Message----- > >> >> From: [email protected] [mailto:[email protected]] On Behalf Of Simon > >> >> Glass > >> >> Sent: 2016年11月24日 10:21 > >> >> To: Z.Q. Hou <[email protected]> > >> >> Cc: U-Boot Mailing List <[email protected]>; Albert ARIBAUD > >> >> <[email protected]>; Prabhakar Kushwaha > >> >> <[email protected]>; Huan Wang-B18965 > >> >> <[email protected]>; Sumit Garg <[email protected]>; > >> Ruchika > >> >> Gupta <[email protected]>; Saksham Jain > >> >> <[email protected]>; york sun <[email protected]>; > M.H. > >> >> Lian <[email protected]>; Bin Meng <[email protected]>; > >> Mingkai > >> >> Hu <[email protected]> > >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based > >> >> on DM > >> >> > >> >> Hi, > >> >> > >> >> On 22 November 2016 at 02:25, Z.Q. Hou <[email protected]> > 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: [email protected] [mailto:[email protected]] On Behalf Of Simon > >> >> >> Glass > >> >> >> Sent: 2016年11月18日 9:15 > >> >> >> To: Z.Q. Hou <[email protected]> > >> >> >> Cc: U-Boot Mailing List <[email protected]>; Albert ARIBAUD > >> >> >> <[email protected]>; Prabhakar Kushwaha > >> >> >> <[email protected]>; Huan Wang-B18965 > >> >> >> <[email protected]>; Sumit Garg <[email protected]>; > >> >> Ruchika > >> >> >> Gupta <[email protected]>; Saksham Jain > >> >> >> <[email protected]>; york sun <[email protected]>; > >> M.H. > >> >> >> Lian <[email protected]>; Bin Meng <[email protected]>; > >> >> Mingkai > >> >> >> Hu <[email protected]> > >> >> >> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver > >> >> >> based on DM > >> >> >> > >> >> >> Hi, > >> >> >> > >> >> >> On 16 November 2016 at 02:48, Zhiqiang Hou > >> >> >> <[email protected]> > >> >> >> wrote: > >> >> >> > From: Minghuan Lian <[email protected]> > >> >> >> > > >> >> >> > 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 <[email protected]> > >> >> >> > Signed-off-by: Hou Zhiqiang <[email protected]> > >> >> >> > --- > >> >> >> > 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. > > > >> > >> > > >> >> > > >> >> >> > >> >> >> > +/* > >> >> >> > + * 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. > > > >> > >> > > >> >> > >> >> > > >> >> >> > >> >> >> > + 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. > > > >> > > >> >> > > >> >> >> > >> >> >> > + 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 [email protected] http://lists.denx.de/mailman/listinfo/u-boot
