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