[U-Boot] [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
Simon Glass
sjg at chromium.org
Sun Nov 27 18:02:29 CET 2016
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?
>
>> >
>> >>
>> >> > +/*
>> >> > + * 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.
>
>>
>> >
>> >>
>> >> > + 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?
>
>>
>> >
>> >> > + 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().
>
>> >
>> >>
>> >> > + 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,
Simon
More information about the U-Boot
mailing list