[U-Boot] [PATCHv3 09/15] pci: layerscape: add pci driver based on DM
Simon Glass
sjg at chromium.org
Thu Nov 24 03:20:53 CET 2016
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.
>
>>
>> > +/*
>> > + * 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.
>
>>
>> > + 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?
>
>> > + 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;
}
>
>>
>> > + 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.
>
>> > + }
>> > + 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().
>
>> > +
>> > + /* 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!
[...]
Regards,
Simon
More information about the U-Boot
mailing list