[U-Boot] [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
Z.Q. Hou
zhiqiang.hou at nxp.com
Thu Nov 24 10:28:25 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年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?
> >
> >>
> >> > +/*
> >> > + * 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.
>
> >
> >>
> >> > + 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.
>
> >
> >> > + 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?
> >
> >>
> >> > + 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.
> >
> >> > +
> >> > + /* 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,
> Simon
More information about the U-Boot
mailing list