| Message ID | 20231029034547.3039893-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [V2] soc: imx8mp: support 128 bits UID | expand |
Hi Peng, thanks for your patch, please see my comments below. On 23-10-29, Peng Fan (OSS) wrote: > From: Peng Fan <peng.fan@nxp.com> > > Current driver only supports 64bits UID for i.MX8MP, but > i.MX8MP UID is actually 128bits, the high 64bits is at 0xE00. > So update driver to support it. > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > > V2: > Address Shawn and Macro's comments > > drivers/soc/imx/soc-imx8m.c | 63 +++++++++++++++++++++++++++++++++---- > 1 file changed, 57 insertions(+), 6 deletions(-) > > diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c > index ec87d9d878f3..6ed7889e1902 100644 > --- a/drivers/soc/imx/soc-imx8m.c > +++ b/drivers/soc/imx/soc-imx8m.c > @@ -24,6 +24,7 @@ > #define OCOTP_UID_HIGH 0x420 > > #define IMX8MP_OCOTP_UID_OFFSET 0x10 > +#define IMX8MP_OCOTP_UID_HIGH 0xe00 > > /* Same as ANADIG_DIGPROG_IMX7D */ > #define ANADIG_DIGPROG_IMX8MM 0x800 > @@ -31,9 +32,11 @@ > struct imx8_soc_data { > char *name; > u32 (*soc_revision)(void); > + bool uid_128bit; Can we replace this by: void (*soc_uid)(void); and let the driver data set the correct soc_uid function within the driver data? Please see below for further comments on this. > }; > > static u64 soc_uid; > +static u64 soc_uid_h; > > #ifdef CONFIG_HAVE_ARM_SMCCC > static u32 imx8mq_soc_revision_from_atf(void) > @@ -101,8 +104,6 @@ static void __init imx8mm_soc_uid(void) > void __iomem *ocotp_base; > struct device_node *np; > struct clk *clk; > - 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) > @@ -118,12 +119,52 @@ static void __init imx8mm_soc_uid(void) > > clk_prepare_enable(clk); > > - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH + offset); > + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); > soc_uid <<= 32; > - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW + offset); > + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); > > clk_disable_unprepare(clk); > clk_put(clk); > + > + iounmap(ocotp_base); > + of_node_put(np); > +} > + > +static void __init imx8mp_soc_uid(void) > +{ > + void __iomem *ocotp_base; > + struct device_node *np; > + struct clk *clk; > + > + np = of_find_compatible_node(NULL, NULL, "fsl,imx8mp-ocotp"); > + if (!np) > + return; > + > + ocotp_base = of_iomap(np, 0); > + if (!ocotp_base) { > + WARN_ON(!ocotp_base); > + return; > + } > + > + clk = of_clk_get_by_name(np, NULL); > + if (IS_ERR(clk)) { > + WARN_ON(IS_ERR(clk)); > + return; > + } > + > + clk_prepare_enable(clk); > + > + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH + IMX8MP_OCOTP_UID_OFFSET); We can avoid this (base + old_reg + offset) pattern now and just do: soc_uid = readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_BITS_64_32); > + soc_uid <<= 32; > + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW + IMX8MP_OCOTP_UID_OFFSET); soc_uid |= readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_BITS_32_0); > + soc_uid_h = readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_HIGH + 0x10); soc_uid = readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_BITS_128_96); > + soc_uid_h <<= 32; > + soc_uid_h |= readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_HIGH); soc_uid |= readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_BITS_96_64); with: #define IMX8MP_OCOTP_UID_BITS_32_0 0x420 #define IMX8MP_OCOTP_UID_BITS_64_32 0x430 #define IMX8MP_OCOTP_UID_BITS_96_64 0xe00 #define IMX8MP_OCOTP_UID_BITS_128_96 0xe10 > + > + clk_disable_unprepare(clk); > + clk_put(clk); > + > iounmap(ocotp_base); > of_node_put(np); > } > @@ -146,7 +187,11 @@ static u32 __init imx8mm_soc_revision(void) > iounmap(anatop_base); > of_node_put(np); > > - imx8mm_soc_uid(); > + > + if (of_machine_is_compatible("fsl,imx8mp")) > + imx8mp_soc_uid(); > + else > + imx8mm_soc_uid(); Sorry for beeing a bit picky but we could improve this driver even further and drop this additional unnecessary of_machine_is_compatible() if we make the soc_uid() a function hook which can be filled by the driver data. Doing the UID within the imx8mm_soc_revision() seems wrong to too since the revisions is using the anatop and the uid the ocotp register space. So it made only sense for the i.MX8MQ. > return rev; > } > @@ -169,6 +214,7 @@ static const struct imx8_soc_data imx8mn_soc_data = { > static const struct imx8_soc_data imx8mp_soc_data = { > .name = "i.MX8MP", > .soc_revision = imx8mm_soc_revision, > + .uid_128bit = true, > }; > > static __maybe_unused const struct of_device_id imx8_soc_match[] = { > @@ -222,7 +268,12 @@ static int __init imx8_soc_init(void) > goto free_soc; > } The new hook should be called like it is done for the soc_revision hook, so within the: if (data) { soc_dev_attr->soc_id = data->name; if (data->soc_revision) soc_rev = data->soc_revision(); if (data->soc_uid) data->soc_uid(); } The split into soc_uid() hook for the i.MX8M platforms can be done within a seperate patch to make it more clear. The next patch should add the support to read the upper 64 bits. > - soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", soc_uid); > + if (data->uid_128bit) This can be checked via: if (soc_uid_h) > + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX%016llX", > + soc_uid_h, soc_uid); > + else > + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", soc_uid); > + > if (!soc_dev_attr->serial_number) { > ret = -ENOMEM; > goto free_rev; Regards, Marco
diff --git a/drivers/soc/imx/soc-imx8m.c b/drivers/soc/imx/soc-imx8m.c index ec87d9d878f3..6ed7889e1902 100644 --- a/drivers/soc/imx/soc-imx8m.c +++ b/drivers/soc/imx/soc-imx8m.c @@ -24,6 +24,7 @@ #define OCOTP_UID_HIGH 0x420 #define IMX8MP_OCOTP_UID_OFFSET 0x10 +#define IMX8MP_OCOTP_UID_HIGH 0xe00 /* Same as ANADIG_DIGPROG_IMX7D */ #define ANADIG_DIGPROG_IMX8MM 0x800 @@ -31,9 +32,11 @@ struct imx8_soc_data { char *name; u32 (*soc_revision)(void); + bool uid_128bit; }; static u64 soc_uid; +static u64 soc_uid_h; #ifdef CONFIG_HAVE_ARM_SMCCC static u32 imx8mq_soc_revision_from_atf(void) @@ -101,8 +104,6 @@ static void __init imx8mm_soc_uid(void) void __iomem *ocotp_base; struct device_node *np; struct clk *clk; - 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) @@ -118,12 +119,52 @@ static void __init imx8mm_soc_uid(void) clk_prepare_enable(clk); - soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH + offset); + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH); soc_uid <<= 32; - soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW + offset); + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW); clk_disable_unprepare(clk); clk_put(clk); + + iounmap(ocotp_base); + of_node_put(np); +} + +static void __init imx8mp_soc_uid(void) +{ + void __iomem *ocotp_base; + struct device_node *np; + struct clk *clk; + + np = of_find_compatible_node(NULL, NULL, "fsl,imx8mp-ocotp"); + if (!np) + return; + + ocotp_base = of_iomap(np, 0); + if (!ocotp_base) { + WARN_ON(!ocotp_base); + return; + } + + clk = of_clk_get_by_name(np, NULL); + if (IS_ERR(clk)) { + WARN_ON(IS_ERR(clk)); + return; + } + + clk_prepare_enable(clk); + + soc_uid = readl_relaxed(ocotp_base + OCOTP_UID_HIGH + IMX8MP_OCOTP_UID_OFFSET); + soc_uid <<= 32; + soc_uid |= readl_relaxed(ocotp_base + OCOTP_UID_LOW + IMX8MP_OCOTP_UID_OFFSET); + + soc_uid_h = readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_HIGH + 0x10); + soc_uid_h <<= 32; + soc_uid_h |= readl_relaxed(ocotp_base + IMX8MP_OCOTP_UID_HIGH); + + clk_disable_unprepare(clk); + clk_put(clk); + iounmap(ocotp_base); of_node_put(np); } @@ -146,7 +187,11 @@ static u32 __init imx8mm_soc_revision(void) iounmap(anatop_base); of_node_put(np); - imx8mm_soc_uid(); + + if (of_machine_is_compatible("fsl,imx8mp")) + imx8mp_soc_uid(); + else + imx8mm_soc_uid(); return rev; } @@ -169,6 +214,7 @@ static const struct imx8_soc_data imx8mn_soc_data = { static const struct imx8_soc_data imx8mp_soc_data = { .name = "i.MX8MP", .soc_revision = imx8mm_soc_revision, + .uid_128bit = true, }; static __maybe_unused const struct of_device_id imx8_soc_match[] = { @@ -222,7 +268,12 @@ static int __init imx8_soc_init(void) goto free_soc; } - soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", soc_uid); + if (data->uid_128bit) + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX%016llX", + soc_uid_h, soc_uid); + else + soc_dev_attr->serial_number = kasprintf(GFP_KERNEL, "%016llX", soc_uid); + if (!soc_dev_attr->serial_number) { ret = -ENOMEM; goto free_rev;