Re: 回复: [EXT] Re: [PATCH v5] soc: imx: Add SoC device register for i.MX9

[Stefan Wahren]

Hi Alice,

Am 15.11.24 um 08:24 schrieb Alice Guo (OSS):
> Hi Stefan,
>
> > -----邮件原件-----
> > 发件人: Stefan Wahren <wahrenst@xxxxxxx>
> > 发送时间: 2024年11月15日 1:40
> > 收件人: Alice Guo (OSS) <alice.guo@xxxxxxxxxxx>;
> > alexander.stein@xxxxxxxxxxxxxxx; shawnguo@xxxxxxxxxx;
> > s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx; festevam@xxxxxxxxx
> > 抄送: imx@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
> > linux-kernel@xxxxxxxxxxxxxxx; Alice Guo <alice.guo@xxxxxxx>
> > 主题: [EXT] Re: [PATCH v5] soc: imx: Add SoC device register for i.MX9
> >
> > Caution: This is an external email. Please take care when clicking links or
> > opening attachments. When in doubt, report the message using the 'Report this
> > email' button
> >
> >
> > Hi Alice,
> >
> > these are the new comments
> >
> > Am 11.11.24 um 04:23 schrieb alice.guo@xxxxxxxxxxx:
> > > From: "alice.guo" <alice.guo@xxxxxxx>
> > >
> > > i.MX9 SoCs have SoC ID, SoC revision number and chip unique identifier
> > > which are provided by the corresponding ARM trusted firmware API. This
> > > patch intends to use SMC call to obtain these information and then
> > > register i.MX9 SoC as a device.
> > >
> > > Signed-off-by: Alice Guo <alice.guo@xxxxxxx>
> > > Tested-by: Alexander Stein <alexander.stein@xxxxxxxxxxxxxxx>
> > > Reviewed-by: Stefan Wahren <wahrenst@xxxxxxx>
> > > ---
> > >
> > > Changes for v2:
> > >    - refine error log print
> > > Changes for v3:
> > >    - return -EINVAL when arm_smccc_smc failed
> > >    - fix the build warning caused by pr_err("%s: SMC failed: %d\n", __func__,
> > res.a0);
> > >    - drop the pr_err in imx9_soc_init
> > >    - free the memory in the reverse order of allocation
> > >    - use of_match_node instead of of_machine_is_compatible Changes for
> > > v4:
> > >    - fix the build warning: 'imx9_soc_match' defined but not used
> > [-Wunused-const-variable=]
> > >    - add Tested-by and Reviewed-by
> > > Changes for v5:
> > >    - probe imx9 soc driver as a platform driver
> > >
> > >    drivers/soc/imx/Makefile   |   2 +-
> > >    drivers/soc/imx/soc-imx9.c | 124
> > +++++++++++++++++++++++++++++++++++++
> > >    2 files changed, 125 insertions(+), 1 deletion(-)
> > >    create mode 100644 drivers/soc/imx/soc-imx9.c
> > >
> > > diff --git a/drivers/soc/imx/Makefile b/drivers/soc/imx/Makefile index
> > > 3ad321ca608a..ca6a5fa1618f 100644
> > > --- a/drivers/soc/imx/Makefile
> > > +++ b/drivers/soc/imx/Makefile
> > > @@ -3,4 +3,4 @@ ifeq ($(CONFIG_ARM),y)
> > >    obj-$(CONFIG_ARCH_MXC) += soc-imx.o
> > >    endif
> > >    obj-$(CONFIG_SOC_IMX8M) += soc-imx8m.o
> > > -obj-$(CONFIG_SOC_IMX9) += imx93-src.o
> > > +obj-$(CONFIG_SOC_IMX9) += imx93-src.o soc-imx9.o
> > > diff --git a/drivers/soc/imx/soc-imx9.c b/drivers/soc/imx/soc-imx9.c
> > > new file mode 100644 index 000000000000..4ef92260e8f9
> > > --- /dev/null
> > > +++ b/drivers/soc/imx/soc-imx9.c
> > > @@ -0,0 +1,124 @@
> > > +// SPDX-License-Identifier: GPL-2.0+
> > > +/*
> > > + * Copyright 2024 NXP
> > > + */
> > > +
> > > +#include <linux/arm-smccc.h>
> > > +#include <linux/init.h>
> > > +#include <linux/module.h>
> > > +#include <linux/of.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/slab.h>
> > > +#include <linux/sys_soc.h>
> > > +
> > > +#define IMX_SIP_GET_SOC_INFO 0xc2000006
> > > +#define SOC_ID(x)            (((x) & 0xFFFF) >> 8)
> > > +#define SOC_REV_MAJOR(x)     ((((x) >> 28) & 0xF) - 0x9)
> > > +#define SOC_REV_MINOR(x)     (((x) >> 24) & 0xF)
> > > +
> > > +static int imx9_soc_probe(struct platform_device *pdev) {
> > > +     struct soc_device_attribute *attr;
> > > +     struct arm_smccc_res res;
> > > +     struct soc_device *sdev;
> > > +     u32 soc_id, rev_major, rev_minor;
> > > +     u64 uid127_64, uid63_0;
> > > +     int err;
> > > +
> > > +     attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> > > +     if (!attr)
> > > +             return -ENOMEM;
> > > +
> > > +     err = of_property_read_string(of_root, "model", &attr->machine);
> > > +     if (err) {
> > > +             pr_err("%s: missing model property: %d\n", __func__, err);
> > > +             goto attr;
> > > +     }
> > > +
> > > +     attr->family = kasprintf(GFP_KERNEL, "Freescale i.MX");
> > > +
> > > +     /*
> > > +      * Retrieve the soc id, rev & uid info:
> > > +      * res.a1[31:16]: soc revision;
> > > +      * res.a1[15:0]: soc id;
> > > +      * res.a2: uid[127:64];
> > > +      * res.a3: uid[63:0];
> > > +      */
> > > +     arm_smccc_smc(IMX_SIP_GET_SOC_INFO, 0, 0, 0, 0, 0, 0, 0, &res);
> > > +     if (res.a0 != SMCCC_RET_SUCCESS) {
> > > +             pr_err("%s: SMC failed: 0x%lx\n", __func__, res.a0);
> > > +             err = -EINVAL;
> > > +             goto family;
> > > +     }
> > I like it that soc-imx8m.c handles the corner case
> > CONFIG_HAVE_ARM_SMCCC=n in order to increase build coverage.
> >
> > Is it possible to adapt this driver accordingly?
> Currently, i.MX93 and i.MX95 obtain soc_id, revision, and serial_number using SMC call. We do not provide a second method like imx8mq.
>
> 	/*
> 	 * SOC revision on older imx8mq is not available in fuses so query
> 	 * the value from ATF instead.
> 	 */
> 	rev = imx8mq_soc_revision_from_atf();
> 	if (!rev) {
> 		magic = readl_relaxed(ocotp_base + IMX8MQ_SW_INFO_B1);
> 		if (magic == IMX8MQ_SW_MAGIC_B1)
> 			rev = REV_B1;
> 	}
This wasn't the corner case which I had in my mind. It was about
building this driver with COMPILE_TEST, but the

#ifdef CONFIG_HAVE_ARM_SMCCC

in soc-imx8m confused me and let me think there is no definition of
arm_smccc_smc for the other case. So it should be fine without the ugly
ifdef stuff.

So please ignore this comment and sorry for the noise

Regards
> Best Regards,
> Alice Guo