| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20231106161453.tucbpn643cxub3ah@pengutronix.de>
Date: Mon, 6 Nov 2023 17:14:53 +0100
From: Marco Felsch <m.felsch@...gutronix.de>
To: "Peng Fan (OSS)" <peng.fan@....nxp.com>
Cc: shawnguo@...nel.org, s.hauer@...gutronix.de, kernel@...gutronix.de,
festevam@...il.com, linux-imx@....com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
Peng Fan <peng.fan@....com>
Subject: Re: [PATCH V2] soc: imx8mp: support 128 bits UID
Hi Peng,
thanks for your patch, please see my comments below.
On 23-10-29, Peng Fan (OSS) wrote:
> From: Peng Fan <peng.fan@....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@....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
Powered by blists - more mailing lists