[EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
Krzysztof Kozlowski
krzk at kernel.org
Mon Nov 16 11:13:38 EST 2020
On Mon, Nov 16, 2020 at 08:18:59AM +0000, Alice Guo wrote:
>
>
> > -----Original Message-----
> > From: Krzysztof Kozlowski <krzk at kernel.org>
> > Sent: 2020年11月15日 0:41
> > To: Alice Guo <alice.guo at nxp.com>
> > Cc: robh+dt at kernel.org; shawnguo at kernel.org; s.hauer at pengutronix.de;
> > dl-linux-imx <linux-imx at nxp.com>; Peng Fan <peng.fan at nxp.com>;
> > devicetree at vger.kernel.org; linux-kernel at vger.kernel.org;
> > linux-arm-kernel at lists.infradead.org
> > Subject: [EXT] Re: [PATCH v3 4/4] soc: imx8m: change to use platform driver
> >
> > Caution: EXT Email
> >
> > On Fri, Nov 13, 2020 at 07:04:09PM +0800, Alice Guo wrote:
> > > Directly reading ocotp register depends on that bootloader enables
> > > ocotp clk, which is not always effective, so change to use nvmem API.
> > > Using nvmem API requires to support driver defer probe and thus change
> > > soc-imx8m.c to use platform driver.
> > >
> > > The other reason is that directly reading ocotp register causes kexec
> > > kernel hang because the 1st kernel running will disable unused clks
> > > after kernel boots up, and then ocotp clk will be disabled even if
> > > bootloader enables it. When kexec kernel, ocotp clk needs to be
> > > enabled before reading ocotp registers, and nvmem API with platform
> > > driver supported can accomplish this.
> > >
> > > Old .dts files can also work.
> > >
> > > Signed-off-by: Alice Guo <alice.guo at nxp.com>
> > > ---
> > > drivers/soc/imx/soc-imx8m.c | 89
> > > ++++++++++++++++++++++++++++++++-----
> > > 1 file changed, 79 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> > > index cc57a384d74d..af2c0dbe8291 100644
> > > --- a/drivers/soc/imx/soc-imx8m.c
> > > +++ b/drivers/soc/imx/soc-imx8m.c
> > > @@ -5,6 +5,8 @@
> > >
> > > #include <linux/init.h>
> > > #include <linux/io.h>
> > > +#include <linux/module.h>
> > > +#include <linux/nvmem-consumer.h>
> > > #include <linux/of_address.h>
> > > #include <linux/slab.h>
> > > #include <linux/sys_soc.h>
> > > @@ -29,7 +31,7 @@
> > >
> > > struct imx8_soc_data {
> > > char *name;
> > > - u32 (*soc_revision)(void);
> > > + u32 (*soc_revision)(struct device *dev, int flag);
> > > };
> > >
> > > static u64 soc_uid;
> > > @@ -50,7 +52,7 @@ static u32 imx8mq_soc_revision_from_atf(void)
> > > static inline u32 imx8mq_soc_revision_from_atf(void) { return 0; };
> > > #endif
> > >
> > > -static u32 __init imx8mq_soc_revision(void)
> > > +static u32 __init imx8mq_soc_revision(struct device *dev, int flag)
> > > {
> > > struct device_node *np;
> > > void __iomem *ocotp_base;
> > > @@ -75,9 +77,17 @@ static u32 __init imx8mq_soc_revision(void)
> > > rev = REV_B1;
> > > }
> > >
> > > - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > > - soc_uid <<= 32;
> > > - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > > + if (flag) {
> > > + int ret = 0;
> > > +
> > > + ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > &soc_uid);
> > > + if (ret)
> > > + return ret;
> > > + } else {
> > > + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH);
> > > + soc_uid <<= 32;
> > > + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW);
> > > + }
> > >
> > > iounmap(ocotp_base);
> > > of_node_put(np);
> > > @@ -107,7 +117,7 @@ static void __init imx8mm_soc_uid(void)
> > > of_node_put(np);
> > > }
> > >
> > > -static u32 __init imx8mm_soc_revision(void)
> > > +static u32 __init imx8mm_soc_revision(struct device *dev, int flag)
> > > {
> > > struct device_node *np;
> > > void __iomem *anatop_base;
> > > @@ -125,7 +135,15 @@ static u32 __init imx8mm_soc_revision(void)
> > > iounmap(anatop_base);
> > > of_node_put(np);
> > >
> > > - imx8mm_soc_uid();
> > > + if (flag) {
> > > + int ret = 0;
> > > +
> > > + ret = nvmem_cell_read_u64(dev, "soc_unique_id",
> > &soc_uid);
> > > + if (ret)
> > > + return ret;
> > > + } else {
> > > + imx8mm_soc_uid();
> > > + }
> > >
> > > return rev;
> > > }
> > > @@ -158,12 +176,21 @@ static __maybe_unused const struct of_device_id
> > imx8_soc_match[] = {
> > > { }
> > > };
> > >
> > > +static __maybe_unused const struct of_device_id imx8m_soc_match[] = {
> >
> > Could this really be unused?
>
> [Alice Guo] I will delete "__maybe_unused".
>
> >
> > > + { .compatible = "fsl,imx8mq-soc", .data = &imx8mq_soc_data, },
> > > + { .compatible = "fsl,imx8mm-soc", .data = &imx8mm_soc_data, },
> > > + { .compatible = "fsl,imx8mn-soc", .data = &imx8mn_soc_data, },
> > > + { .compatible = "fsl,imx8mp-soc", .data = &imx8mp_soc_data, },
> > > + { }
> > > +};
> > > +MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> >
> > You already have "imx8_soc_match" which covers imx8m and now you add
> > "imx8m_soc_match" which also covers imx8m. Such naming is a pure
> > confusion.
> >
>
> [Alice Guo] device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
> whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. The purpose is to be compatible with the old DTS file which does not have
> "fsl,imx8mX-soc".
I got it, but it's not what I was pointing out. Let me make it simpler:
static const struct of_device_id imx8m_soc_match;
static const struct of_device_id imx8_soc_match;
This is pure confusion in naming.
Based on this naming:
1. imx8m_soc_match means "matching only i.MX 8M SoCs",
2. imx8_soc_match means "match all of i.MX 8".
Totally different than what you wrote here and what you intend....
>
> > > +
> > > #define imx8_revision(soc_rev) \
> > > soc_rev ? \
> > > kasprintf(GFP_KERNEL, "%d.%d", (soc_rev >> 4) & 0xf, soc_rev &
> > 0xf) : \
> > > "unknown"
> > >
> > > -static int __init imx8_soc_init(void)
> > > +static int imx8_soc_init_flag(struct platform_device *pdev, int flag)
> > > {
> > > struct soc_device_attribute *soc_dev_attr;
> > > struct soc_device *soc_dev;
> > > @@ -182,7 +209,10 @@ static int __init imx8_soc_init(void)
> > > if (ret)
> > > goto free_soc;
> > >
> > > - id = of_match_node(imx8_soc_match, of_root);
> > > + if (flag)
> > > + id = of_match_node(imx8m_soc_match,
> > pdev->dev.of_node);
> > > + else
> > > + id = of_match_node(imx8_soc_match, of_root);
> > > if (!id) {
> > > ret = -ENODEV;
> > > goto free_soc;
> > > @@ -192,7 +222,13 @@ static int __init imx8_soc_init(void)
> > > if (data) {
> > > soc_dev_attr->soc_id = data->name;
> > > if (data->soc_revision)
> > > - soc_rev = data->soc_revision();
> > > + soc_rev = data->soc_revision(&pdev->dev, flag);
> > > +
> > > + if (flag) {
> > > + ret = soc_rev;
> > > + if (ret < 0)
> > > + goto free_soc;
> > > + }
> > > }
> > >
> > > soc_dev_attr->revision = imx8_revision(soc_rev); @@ -230,4
> > > +266,37 @@ static int __init imx8_soc_init(void)
> > > kfree(soc_dev_attr);
> > > return ret;
> > > }
> > > +
> > > +static int __init imx8_soc_init(void) {
> > > + int ret = 0, flag = 0;
> > > +
> > > + if (of_find_compatible_node(NULL, NULL, "fsl,imx8mm-soc") ||
> > > + of_find_compatible_node(NULL, NULL, "fsl,imx8mn-soc") ||
> > > + of_find_compatible_node(NULL, NULL, "fsl,imx8mp-soc") ||
> > > + of_find_compatible_node(NULL, NULL, "fsl,imx8mq-soc"))
> >
> > Missing puts.
> >
> > Don't duplicate the compatibles, iterate over existing structure... or see
> > comments below. Maybe you could simplify it with something like
> > of_find_matching_node_and_match()... but check comments below.
>
> [Alice Guo] I check comments below.
>
> >
> > > + return 0;
> > > +
> > > + ret = imx8_soc_init_flag(NULL, flag);
> > > + return ret;
> > > +}
> > > device_initcall(imx8_soc_init);
> >
> > Where is the changelog? This was removed previously, now it stays...
> >
> > After more thoughs, it looks you have kept it for the purpose of supporting
> > existing DTB, but it is not explained. Neither in the source code (which after
> > applying this patch looks confusing) nor in commit message.
> >
> > In case of old DTB without fsl,imx8mm-soc-like compatibles, it would be better
> > to still register a platform driver and create a device
> > (of_platform_device_create())). However still this won't solve the problem of
> > actually missing device node... so maybe this double entry point is acceptable,
> > if properly explained.
>
> [Alice Guo] Sorry, I will add changelog next time. Actually I wrote "Old .dts files can also work." in the commit.
>
> device_initcall is executed earlier than module_platform_driver. imx8_soc_init will judge
> whether there is "fsl,imx8mX-soc" in DTS file. If there is "fsl,imx8mX-soc", it will exit device_initcall and use module_platform_driver. Can I keep double entry point?
If it is properly explained and there is no other way then yes, you
could. Here, for old DTBs, I would prefer to use
of_platform_device_create() and bind to "soc" node (child of root).
This way you would always have device and exactly one entry point for
the probe.
Best regards,
Krzysztof
More information about the linux-arm-kernel
mailing list