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

["Lothar Waßmann" <LW@KARO-electronics.de>]

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