Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.33 as permitted sender) client-ip=23.128.96.33;
Date:   Mon, 6 Nov 2023 17:14:53 +0100
From:   Marco Felsch <m.felsch@pengutronix.de>
To:     "Peng Fan (OSS)" <peng.fan@oss.nxp.com>
Cc:     shawnguo@kernel.org, s.hauer@pengutronix.de, kernel@pengutronix.de,
        festevam@gmail.com, linux-imx@nxp.com,
        linux-arm-kernel@lists.infradead.org, linux-kernel@vger.kernel.org,
        Peng Fan <peng.fan@nxp.com>
Subject: Re: [PATCH V2] soc: imx8mp: support 128 bits UID
Message-ID: <20231106161453.tucbpn643cxub3ah@pengutronix.de>
References: <20231029034547.3039893-1-peng.fan@oss.nxp.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20231029034547.3039893-1-peng.fan@oss.nxp.com>
User-Agent: NeoMutt/20180716
Precedence: bulk

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