Re: [PATCH v7 10/19] imx9: scmi: add i.MX95 SoC and clock related code

Lothar Waßmann
2025-03-06T05:42:06Z

Hi,

On Wed, 05 Mar 2025 21:28:22 +0800 Alice Guo (OSS) wrote:
> From: Peng Fan <peng.fan@nxp.com>
>
> This patch adds i.MX95 SoC and clock related code. Because they are
> based on SCMI, put them in the scmi subfolder.
>
> Signed-off-by: Ye Li <ye.li@nxp.com>
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Alice Guo <alice.guo@nxp.com>
> Reviewed-by: Peng Fan <peng.fan@nxp.com>
> ---
> arch/arm/include/asm/arch-imx/cpu.h | 2 +
> arch/arm/include/asm/arch-imx9/clock.h | 10 +
> arch/arm/include/asm/arch-imx9/imx-regs.h | 5 +
> arch/arm/include/asm/arch-imx9/sys_proto.h | 1 +
> arch/arm/include/asm/mach-imx/sys_proto.h | 39 ++
> arch/arm/mach-imx/imx9/scmi/Makefile | 6 +
> arch/arm/mach-imx/imx9/scmi/clock.c | 105 ++++
> arch/arm/mach-imx/imx9/scmi/clock_scmi.c | 133 +++++
> arch/arm/mach-imx/imx9/scmi/soc.c | 788 +++++++++++++++++++++++++++++
> arch/sandbox/include/asm/scmi_test.h | 2 +-
> 10 files changed, 1090 insertions(+), 1 deletion(-)
>
[...]
> diff --git a/arch/arm/mach-imx/imx9/scmi/clock.c
b/arch/arm/mach-imx/imx9/scmi/clock.c
> new file mode 100644
> index 0000000000..9ebd380976
> --- /dev/null
> +++ b/arch/arm/mach-imx/imx9/scmi/clock.c
> @@ -0,0 +1,105 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright 2025 NXP
> + */
> +
> +#include <asm/arch/clock.h>
> +#include <dm/uclass.h>
> +#include <scmi_agent.h>
> +#include "../../../../../dts/upstream/src/arm64/freescale/imx95-clock.h"
>
"Interesting" include path...
Shouldn't this file be located under dts/upstream/include/dt-bindings/
like all the other imx*-clock.h files?
Then the file should be picked up via
#include <dt-bindings/imx95-clock.h>

> +#define UNLOCK_WORD 0xD928C520 /* unlock word */
> +#define REFRESH_WORD 0xB480A602 /* refresh word */
>
useless comments.

> +static void disable_wdog(void __iomem *wdog_base)
> +{
> + u32 val_cs = readl(wdog_base + 0x00);
> +
> + if (!(val_cs & 0x80))
> + return;
> +
> + /* default is 32bits cmd */
> + writel(REFRESH_WORD, (wdog_base + 0x04)); /* Refresh the CNT */
> +
> + if (!(val_cs & 0x800)) {
> + writel(UNLOCK_WORD, (wdog_base + 0x04));
> + while (!(readl(wdog_base + 0x00) & 0x800))
> + ;
> + }
> + writel(0x0, (wdog_base + 0x0C)); /* Set WIN to 0 */
> + writel(0x400, (wdog_base + 0x08)); /* Set timeout to default 0x400 */
> + writel(0x2120, (wdog_base + 0x00)); /* Disable it and set update */
> +
> + while (!(readl(wdog_base + 0x00) & 0x400))
> + ;
>
indefinite loops polling hardware bits will lead to silent hangs when
the hardware misbehaves.

[...]
> +void imx_get_mac_from_fuse(int dev_id, unsigned char *mac)
> +{
> + u32 val[2] = {};
> + int ret;
> +
> + if (dev_id == 0) {
> + ret = fuse_read(39, 3, &val[0]);
> + if (ret)
> + goto err;
> +
> + ret = fuse_read(39, 4, &val[1]);
> + if (ret)
> + goto err;
> +
> + mac[0] = val[1] >> 8;
> + mac[1] = val[1];
> + mac[2] = val[0] >> 24;
> + mac[3] = val[0] >> 16;
> + mac[4] = val[0] >> 8;
> + mac[5] = val[0];
> +
> + } else {
> + ret = fuse_read(39, 5, &val[0]);
> + if (ret)
> + goto err;
> +
> + ret = fuse_read(39, 4, &val[1]);
> + if (ret)
> + goto err;
> +
> + if (is_soc_rev(CHIP_REV_1_0)) {
> + mac[0] = val[1] >> 24;
> + mac[1] = val[1] >> 16;
> + mac[2] = val[0] >> 24;
> + mac[3] = val[0] >> 16;
> + mac[4] = val[0] >> 8;
> + mac[5] = val[0];
> + } else {
> + mac[0] = val[0] >> 24;
> + mac[1] = val[0] >> 16;
> + mac[2] = val[0] >> 8;
> + mac[3] = val[0];
> + mac[4] = val[1] >> 24;
> + mac[5] = val[1] >> 16;
> + }
> + }
> +
> + debug("%s: MAC%d: %02x.%02x.%02x.%02x.%02x.%02x\n",
> + __func__, dev_id, mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
>
The format "%pM" exists for printing MAC addresses.

[...]
> +int print_cpuinfo(void)
> +{
> + u32 cpurev, max_freq;
> + int minc, maxc;
> +
> + cpurev = get_cpu_rev();
> +
> + printf("CPU: i.MX%s rev%d.%d",
> + get_imx_type((cpurev & 0x1FF000) >> 12),
> + (cpurev & 0x000F0) >> 4, (cpurev & 0x0000F) >> 0);
> +
> + max_freq = get_cpu_speed_grade_hz();
> + if (!max_freq || max_freq == mxc_get_clock(MXC_ARM_CLK)) {
> + printf(" at %dMHz\n", mxc_get_clock(MXC_ARM_CLK) / 1000000);
> + } else {
> + printf(" %d MHz (running at %d MHz)\n", max_freq / 1000000,
> + mxc_get_clock(MXC_ARM_CLK) / 1000000);
>
"%u" to match the data types.

[...]
> +enum env_location env_get_location(enum env_operation op, int prio)
> +{
>
should be declared as __weak to give others the chance to override this
function in board specific code.

Lothar Waßmann