RE: [EXT] Re: [PATCH v2 4/4] soc: imx8m: change to use platform driver

[Alice Guo <alice.guo@nxp.com>]

> -----Original Message-----
> From: Krzysztof Kozlowski <krzk@kernel.org>
> Sent: 2020年11月5日 17:06
> To: Alice Guo <alice.guo@nxp.com>
> Cc: robh+dt@kernel.org; shawnguo@kernel.org; s.hauer@pengutronix.de;
> dl-linux-imx <linux-imx@nxp.com>; Peng Fan <peng.fan@nxp.com>;
> devicetree@vger.kernel.org; linux-kernel@vger.kernel.org;
> linux-arm-kernel@lists.infradead.org
> Subject: [EXT] Re: [PATCH v2 4/4] soc: imx8m: change to use platform driver
> 
> Caution: EXT Email
> 
> On Thu, Nov 05, 2020 at 03:26:29PM +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.
> >
> > Signed-off-by: Alice Guo <alice.guo@nxp.com>
> > ---
> >  drivers/soc/imx/soc-imx8m.c | 75
> > +++++++++++++++++++++----------------
> >  1 file changed, 42 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> > index cc57a384d74d..83f3297509be 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);
> >  };
> >
> >  static u64 soc_uid;
> > @@ -50,12 +52,15 @@ 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 imx8mm_soc_uid(struct device *dev);
> > +
> > +static u32 __init imx8mq_soc_revision(struct device *dev)
> >  {
> >       struct device_node *np;
> >       void __iomem *ocotp_base;
> >       u32 magic;
> >       u32 rev;
> > +     int ret = 0;
> >
> >       np = of_find_compatible_node(NULL, NULL, "fsl,imx8mq-ocotp");
> >       if (!np)
> > @@ -75,9 +80,9 @@ 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);
> > +     ret = imx8mm_soc_uid(dev);
> > +     if (ret)
> > +             return ret;
> >
> >       iounmap(ocotp_base);
> >       of_node_put(np);
> > @@ -85,33 +90,20 @@ static u32 __init imx8mq_soc_revision(void)
> >       return rev;
> >  }
> >
> > -static void __init imx8mm_soc_uid(void)
> > +static u32 __init imx8mm_soc_uid(struct device *dev)
> >  {
> > -     void __iomem *ocotp_base;
> > -     struct device_node *np;
> > -     u32 offset = of_machine_is_compatible("fsl,imx8mp") ?
> > -                  IMX8MP_OCOTP_UID_OFFSET : 0;
> > -
> > -     np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-ocotp");
> > -     if (!np)
> > -             return;
> > -
> > -     ocotp_base = of_iomap(np, 0);
> > -     WARN_ON(!ocotp_base);
> > -
> > -     soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH + offset);
> > -     soc_uid <<= 32;
> > -     soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW + offset);
> > +     int ret = 0;
> >
> > -     iounmap(ocotp_base);
> > -     of_node_put(np);
> > +     ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> > +     return ret;
> >  }
> >
> > -static u32 __init imx8mm_soc_revision(void)
> > +static u32 __init imx8mm_soc_revision(struct device *dev)
> >  {
> >       struct device_node *np;
> >       void __iomem *anatop_base;
> >       u32 rev;
> > +     int ret;
> >
> >       np = of_find_compatible_node(NULL, NULL, "fsl,imx8mm-anatop");
> >       if (!np)
> > @@ -125,7 +117,9 @@ static u32 __init imx8mm_soc_revision(void)
> >       iounmap(anatop_base);
> >       of_node_put(np);
> >
> > -     imx8mm_soc_uid();
> > +     ret = imx8mm_soc_uid(dev);
> > +     if (ret)
> > +             return ret;
> 
> I think this breaks old existing DTBs. If applied on separate branch than DTS
> patches, it will cause bisect regressions. Regardless of it, all out-of-tree
> (customer DTBs) will start failing here as well.
> 
> Best regards,
> Krzysztof
> 
> 
[Alice Guo] Compatible with old existing DTBs, I did not find a good solution. I want to use soc device to match this driver. Do you have some suggestions? Thank you.

Best regards,
Alice
> >
> >       return rev;
> >  }
> > @@ -151,19 +145,20 @@ static const struct imx8_soc_data
> > imx8mp_soc_data = {  };
> >
> >  static __maybe_unused const struct of_device_id imx8_soc_match[] = {
> > -     { .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
> > -     { .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
> > -     { .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, },
> > -     { .compatible = "fsl,imx8mp", .data = &imx8mp_soc_data, },
> > +     { .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, imx8_soc_match);
> >
> >  #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_probe(struct platform_device *pdev)
> >  {
> >       struct soc_device_attribute *soc_dev_attr;
> >       struct soc_device *soc_dev;
> > @@ -182,7 +177,7 @@ static int __init imx8_soc_init(void)
> >       if (ret)
> >               goto free_soc;
> >
> > -     id = of_match_node(imx8_soc_match, of_root);
> > +     id = of_match_node(imx8_soc_match, pdev->dev.of_node);
> >       if (!id) {
> >               ret = -ENODEV;
> >               goto free_soc;
> > @@ -192,7 +187,11 @@ 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);
> > +
> > +             ret = soc_rev;
> > +             if (ret < 0)
> > +                     goto free_soc;
> >       }
> >
> >       soc_dev_attr->revision = imx8_revision(soc_rev); @@ -230,4
> > +229,14 @@ static int __init imx8_soc_init(void)
> >       kfree(soc_dev_attr);
> >       return ret;
> >  }
> > -device_initcall(imx8_soc_init);
> > +
> > +static struct platform_driver imx8_soc_init_driver = {
> > +     .probe = imx8_soc_init_probe,
> > +     .driver = {
> > +             .name = "imx8_soc_init",
> > +             .of_match_table = of_match_ptr(imx8_soc_match),
> > +     },
> > +};
> > +
> > +module_platform_driver(imx8_soc_init_driver);
> > +MODULE_LICENSE("GPL v2");
> > --
> > 2.17.1
> >