[U-Boot] [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
Z.Q. Hou
zhiqiang.hou at nxp.com
Thu Dec 1 11:48:13 CET 2016
Hi Simon,
Thanks for your comments!
> -----Original Message-----
> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> Sent: 2016年12月1日 10:20
> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert ARIBAUD
> <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> <alison.wang at freescale.com>; Sumit Garg <sumit.garg at nxp.com>; Ruchika
> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> <saksham.jain at nxp.freescale.com>; york sun <york.sun at nxp.com>; M.H. Lian
> <minghuan.lian at nxp.com>; Bin Meng <bmeng.cn at gmail.com>; Mingkai Hu
> <mingkai.hu at nxp.com>
> Subject: Re: [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
>
> Hi,
>
> On 30 November 2016 at 01:14, Z.Q. Hou <zhiqiang.hou at nxp.com> wrote:
> > Hi Simon,
> >
> > Thanks for your comments!
> >
> >> -----Original Message-----
> >> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon Glass
> >> Sent: 2016年11月30日 5:41
> >> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> >> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert ARIBAUD
> >> <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> >> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> >> <alison.wang at freescale.com>; Sumit Garg <sumit.garg at nxp.com>;
> Ruchika
> >> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> >> <saksham.jain at nxp.freescale.com>; york sun <york.sun at nxp.com>; M.H.
> >> Lian <minghuan.lian at nxp.com>; Bin Meng <bmeng.cn at gmail.com>;
> Mingkai
> >> Hu <mingkai.hu at 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.hou at nxp.com> wrote:
> >> > Hi Simon,
> >> >
> >> > Thanks for your comments!
> >> >
> >> >> -----Original Message-----
> >> >> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon
> >> >> Glass
> >> >> Sent: 2016年11月28日 1:02
> >> >> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> >> >> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert ARIBAUD
> >> >> <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> >> >> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> >> >> <alison.wang at freescale.com>; Sumit Garg <sumit.garg at nxp.com>;
> >> Ruchika
> >> >> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> >> >> <saksham.jain at nxp.freescale.com>; york sun <york.sun at nxp.com>;
> M.H.
> >> >> Lian <minghuan.lian at nxp.com>; Bin Meng <bmeng.cn at gmail.com>;
> >> Mingkai
> >> >> Hu <mingkai.hu at 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.hou at nxp.com>
> wrote:
> >> >> > Hi Simon,
> >> >> >
> >> >> > Thanks for your comments!
> >> >> >
> >> >> >> -----Original Message-----
> >> >> >> From: sjg at google.com [mailto:sjg at google.com] On Behalf Of Simon
> >> >> >> Glass
> >> >> >> Sent: 2016年11月24日 10:21
> >> >> >> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> >> >> >> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert ARIBAUD
> >> >> >> <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> >> >> >> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> >> >> >> <alison.wang at freescale.com>; Sumit Garg <sumit.garg at nxp.com>;
> >> >> Ruchika
> >> >> >> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> >> >> >> <saksham.jain at nxp.freescale.com>; york sun <york.sun at nxp.com>;
> >> M.H.
> >> >> >> Lian <minghuan.lian at nxp.com>; Bin Meng <bmeng.cn at gmail.com>;
> >> >> Mingkai
> >> >> >> Hu <mingkai.hu at 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.hou at 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: sjg at google.com [mailto:sjg at google.com] On Behalf Of
> >> >> >> >> Simon Glass
> >> >> >> >> Sent: 2016年11月18日 9:15
> >> >> >> >> To: Z.Q. Hou <zhiqiang.hou at nxp.com>
> >> >> >> >> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Albert
> >> >> >> >> ARIBAUD <albert.u.boot at aribaud.net>; Prabhakar Kushwaha
> >> >> >> >> <prabhakar.kushwaha at nxp.com>; Huan Wang-B18965
> >> >> >> >> <alison.wang at freescale.com>; Sumit Garg
> >> >> >> >> <sumit.garg at nxp.com>;
> >> >> >> Ruchika
> >> >> >> >> Gupta <ruchika.gupta at nxp.com>; Saksham Jain
> >> >> >> >> <saksham.jain at nxp.freescale.com>; york sun
> >> >> >> >> <york.sun at nxp.com>;
> >> >> M.H.
> >> >> >> >> Lian <minghuan.lian at nxp.com>; Bin Meng
> <bmeng.cn at gmail.com>;
> >> >> >> Mingkai
> >> >> >> >> Hu <mingkai.hu at 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.Hou at nxp.com>
> >> >> >> >> wrote:
> >> >> >> >> > From: Minghuan Lian <Minghuan.Lian at 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.Lian at nxp.com>
> >> >> >> >> > Signed-off-by: Hou Zhiqiang <Zhiqiang.Hou at 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?
Ok, will add a new file for fixup.
>
> >
> >> >
> >> >>
> >> >> >
> >> >> >> >
> >> >> >> >>
> >> >> >> >> > +/*
> >> >> >> >> > + * 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.
Ok, thanks for your tips.
>
> >
> >> >
> >> >> >
> >> >> >> >
> >> >> >> >>
> >> >> >> >> > + 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
More information about the U-Boot
mailing list