Re: [PATCH v4 4/4] soc: imx8m: change to use platform driver
From: Krzysztof Kozlowski
Date: Fri Nov 20 2020 - 05:46:31 EST
On Fri, Nov 20, 2020 at 06:11:12PM +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.
>
> v2: remove the subject prefix "LF-2571-4"
> v3: Keep the original way which uses device_initcall to read soc unique
> ID, and add the other way which uses module_platform_driver and
> nvmem API, so that it will not break the old version DTBs.
> v4: delete "__maybe_unused"
> delete MODULE_DEVICE_TABLE(of, imx8m_soc_match);
> rename match table, "fsl,imx8mm/n/q/p" is actually a machine
> compabile and "fsl,imx8mm/n/q/p-soc" is a compabile of soc@0
> delete "flag" and change to determine whether the pointer is NULL
> ues of_find_matching_node_and_match()
> delete of_match_ptr()
Put all the patch version changelogs after --- separator, so they will
not go to the commit.
>
> Signed-off-by: Alice Guo <alice.guo@xxxxxxx>
> ---
> drivers/soc/imx/soc-imx8m.c | 85 +++++++++++++++++++++++++++++++------
> 1 file changed, 73 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c
> index cc57a384d74d..1b0a34e545ae 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,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)
> {
> struct device_node *np;
> void __iomem *ocotp_base;
> @@ -75,9 +77,19 @@ 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 (dev) {
> + int ret = 0;
> +
> + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> + if (ret) {
> + iounmap(ocotp_base);
What about other cleanup parts? of_node_put?
> + 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 +119,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)
> {
> struct device_node *np;
> void __iomem *anatop_base;
> @@ -125,7 +137,15 @@ static u32 __init imx8mm_soc_revision(void)
> iounmap(anatop_base);
> of_node_put(np);
>
> - imx8mm_soc_uid();
> + if (dev) {
> + int ret = 0;
> +
> + ret = nvmem_cell_read_u64(dev, "soc_unique_id", &soc_uid);
> + if (ret)
> + return ret;
> + } else {
> + imx8mm_soc_uid();
> + }
>
> return rev;
> }
> @@ -150,7 +170,7 @@ static const struct imx8_soc_data imx8mp_soc_data = {
> .soc_revision = imx8mm_soc_revision,
> };
>
> -static __maybe_unused const struct of_device_id imx8_soc_match[] = {
> +static const struct of_device_id imx8_machine_match[] = {
> { .compatible = "fsl,imx8mq", .data = &imx8mq_soc_data, },
> { .compatible = "fsl,imx8mm", .data = &imx8mm_soc_data, },
> { .compatible = "fsl,imx8mn", .data = &imx8mn_soc_data, },
> @@ -158,12 +178,20 @@ static __maybe_unused const struct of_device_id imx8_soc_match[] = {
> { }
> };
>
> +static const struct of_device_id imx8_soc_match[] = {
> + { .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, },
> + { }
> +};
> +
> #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_info(struct platform_device *pdev)
> {
> struct soc_device_attribute *soc_dev_attr;
> struct soc_device *soc_dev;
> @@ -182,7 +210,10 @@ static int __init imx8_soc_init(void)
> if (ret)
> goto free_soc;
>
> - id = of_match_node(imx8_soc_match, of_root);
> + if (pdev)
> + id = of_match_node(imx8_soc_match, pdev->dev.of_node);
> + else
> + id = of_match_node(imx8_machine_match, of_root);
> if (!id) {
> ret = -ENODEV;
> goto free_soc;
> @@ -191,8 +222,16 @@ static int __init imx8_soc_init(void)
> data = id->data;
> if (data) {
> soc_dev_attr->soc_id = data->name;
> - if (data->soc_revision)
> - soc_rev = data->soc_revision();
> + if (data->soc_revision) {
> + if (pdev) {
> + soc_rev = data->soc_revision(&pdev->dev);
> + ret = soc_rev;
> + if (ret < 0)
> + goto free_soc;
> + } else {
> + soc_rev = data->soc_revision(NULL);
> + }
> + }
> }
>
> soc_dev_attr->revision = imx8_revision(soc_rev);
> @@ -230,4 +269,26 @@ static int __init imx8_soc_init(void)
> kfree(soc_dev_attr);
> return ret;
> }
> +
> +static int __init imx8_soc_init(void)
> +{
> + int ret = 0;
Skip the initialization.
You need to document why the initcal is still there. Please write that
it is purely for backward compatibility.
Best regards,
Krzysztof