Re: [PATCH v3 09/17] imx9: scmi: add i.MX95 SoC and clock related code

On 1/3/25 7:45 AM, Alice Guo wrote:

[...]

diff --git a/arch/arm/include/asm/arch-imx9/clock.h

b/arch/arm/include/asm/arch-imx9/clock.h

index 60d48b13b11f274c9e4c8caf20954de0431d9d6a..a64c18b28291553d47725d83cb748031faf64488 100644 --- a/arch/arm/include/asm/arch-imx9/clock.h +++ b/arch/arm/include/asm/arch-imx9/clock.h @@ -1,8 +1,8 @@ /* SPDX-License-Identifier: GPL-2.0+ */ /*

• • Copyright 2022 NXP

• • Copyright 2024 NXP

2022-2024

• • Peng Fan <peng.fan at nxp.com>

• • Peng Fan peng.fan@nxp.com

Is the email update desirable ?

[...]

diff --git a/arch/arm/include/asm/mach-imx/sys_proto.h b/arch/arm/include/asm/mach-imx/sys_proto.h index 109a806852ab42d018ce45a4e96af5b57adb6a9c..bcf33769ae5f45125bbf9377eb64d96473dc4996 100644 --- a/arch/arm/include/asm/mach-imx/sys_proto.h +++ b/arch/arm/include/asm/mach-imx/sys_proto.h @@ -2,6 +2,7 @@ /*

• (C) Copyright 2009
• Stefano Babic, DENX Software Engineering, sbabic@denx.de.

• • Copyright 2024 NXP

  */

  #ifndef _SYS_PROTO_H_

@@ -97,6 +98,8 @@ struct bd_info; #define is_imx9302() (is_cpu_type(MXC_CPU_IMX9302)) #define is_imx9301() (is_cpu_type(MXC_CPU_IMX9301))

+#define is_imx95() (is_cpu_type(MXC_CPU_IMX95))

• #define is_imx9121() (is_cpu_type(MXC_CPU_IMX9121)) #define is_imx9111() (is_cpu_type(MXC_CPU_IMX9111)) #define is_imx9101() (is_cpu_type(MXC_CPU_IMX9101))

@@ -216,6 +219,43 @@ ulong spl_romapi_get_uboot_base(u32 image_offset, u32 rom_bt_dev); u32 rom_api_download_image(u8 *dest, u32 offset, u32 size); u32 rom_api_query_boot_infor(u32 info_type, u32 *info);

+#ifdef CONFIG_SCMI_FIRMWARE

Avoid typedefs

+typedef struct rom_passover {

Use uN types instead of uintN_t ...

• uint16_t tag; //!< Tag

u16

• uint8_t len; //!< Fixed value of 0x80

u8 and so on ...

• uint8_t ver; //!< Version
• uint32_t boot_mode; //!< Boot mode
• uint32_t card_addr_mode; //!< SD card address mode
• uint32_t bad_blks_of_img_set0; //!< NAND bad block count skipped 1
• uint32_t ap_mu_id; //!< AP MU ID
• uint32_t bad_blks_of_img_set1; //!< NAND bad block count skipped 1
• uint8_t boot_stage; //!< Boot stage
• uint8_t img_set_sel; //!< Image set booted from
• uint8_t rsv0[2]; //!< Reserved
• uint32_t img_set_end; //!< Offset of Image End
• uint32_t rom_version; //!< ROM version
• uint8_t boot_dev_state; //!< Boot device state
• uint8_t boot_dev_inst; //!< Boot device type
• uint8_t boot_dev_type; //!< Boot device instance
• uint8_t rsv1; //!< Reserved
• uint32_t dev_page_size; //!< Boot device page size
• uint32_t cnt_header_ofs; //!< Container header offset
• uint32_t img_ofs; //!< Image offset

What is that //!< in comments ^ ?

+} __attribute__ ((packed)) rom_passover_t;

__packed

[...]

+++ b/arch/arm/mach-imx/imx9/scmi/clock.c @@ -0,0 +1,328 @@ +// SPDX-License-Identifier: GPL-2.0+ +/*

• • Copyright 2024 NXP
• */

+#include <asm/arch/ccm_regs.h> +#include <asm/arch/clock.h> +#include <asm/arch/imx-regs.h> +#include <asm/arch/sys_proto.h> +#include <asm/global_data.h> +#include <asm/io.h> +#include <command.h> +#include <errno.h> +#ifdef CONFIG_CLK_SCMI +#include "../../../../../dts/upstream/src/arm64/freescale/imx95-clock.h" +#include <dm/uclass.h> +#include <dm/uclass-internal.h> +#include <linux/clk-provider.h> +#include <scmi_agent.h> +#include <scmi_protocols.h> +#include <dm/device.h> +#include <dm/device-internal.h> +#endif

+DECLARE_GLOBAL_DATA_PTR;

+u32 get_arm_core_clk(void) +{

• u32 val;
• /* TODO: */

To Do what ?

• val = imx_clk_scmi_get_rate(IMX95_CLK_SEL_A55C0);
• if (val)

• return val;
  

• return imx_clk_scmi_get_rate(IMX95_CLK_A55);

+}

+void set_arm_core_max_clk(void) +{

• int ret;
• u32 arm_domain_id = 8;
• struct scmi_perf_in in = {

• .domain_id = arm_domain_id,
  

• .perf_level = 3,
  

• };
• struct scmi_perf_out out;
• struct scmi_msg msg = SCMI_MSG_IN(SCMI_PROTOCOL_ID_PERF, SCMI_PERF_LEVEL_SET, in, out);
• struct udevice *dev;
• ret = uclass_get_device_by_name(UCLASS_CLK, "protocol@14", &dev);
• if (ret)

• printf("%s: %d\n", __func__, ret);
  

What happens on failure ?

• ret = devm_scmi_process_msg(dev, &msg);
• if (ret)

• printf("%s: %d\n", __func__, ret);
  

+}

+void enable_usboh3_clk(unsigned char enable)

Is this needed ?

+{ +}

+int clock_init_early(void) +{

• return 0;

+}

+/* Set bus and A55 core clock per voltage mode */ +int clock_init_late(void) +{

• set_arm_core_max_clk();
• return 0;

+}

+u32 get_lpuart_clk(void) +{

• return imx_clk_scmi_get_rate(IMX95_CLK_LPUART1);

+}

+void init_uart_clk(u32 index) +{

• u32 clock_id;
• switch (index) {
• case 0:

• clock_id = IMX95_CLK_LPUART1;
  

• break;
  

• case 1:

• clock_id = IMX95_CLK_LPUART2;
  

• break;
  

• case 2:

• clock_id = IMX95_CLK_LPUART3;
  

• break;
  

• default:

• return;
  

• }
• /* 24MHz */
• imx_clk_scmi_enable(clock_id, false);
• imx_clk_scmi_set_parent(clock_id, IMX95_CLK_24M);
• imx_clk_scmi_set_rate(clock_id, 24000000);
• imx_clk_scmi_enable(clock_id, true);

+}

+/* I2C check */ +u32 imx_get_i2cclk(u32 i2c_num) +{

• if (i2c_num > 7)

• return -EINVAL;
  

• switch (i2c_num) {
• case 0:

• return imx_clk_scmi_get_rate(IMX95_CLK_LPI2C1);
  

Can this switch-case statement be turned into some

return imx_clk_scmi_get_rate(IMX95_CLK_LPI2C1 + i2c_num);

?

+int enable_i2c_clk(unsigned char enable, u32 i2c_num) +{

• u32 clock_id;
• if (i2c_num > 7)

• return -EINVAL;
  

• switch (i2c_num) {
• case 0:

• clock_id = IMX95_CLK_LPI2C1;
  

Can this switch-case statement be turned into some

clock_id = IMX95_CLK_LPI2C1 + i2c_num;

?

• break;
  

• case 1:

• clock_id = IMX95_CLK_LPI2C2;
  

• break;
  

DTTO for all the other switch-case statements.

+void dram_enable_bypass(ulong clk_val) +{

• u64 rate;
• switch (clk_val) {
• case MHZ(625):

• imx_clk_scmi_set_parent(IMX95_CLK_DRAMALT, IMX95_CLK_SYSPLL1_PFD2);
  

• rate = imx_clk_scmi_set_rate(IMX95_CLK_DRAMALT, clk_val);
  

• break;
  

• case MHZ(400):

• imx_clk_scmi_set_parent(IMX95_CLK_DRAMALT, IMX95_CLK_SYSPLL1_PFD1);
  

• rate = imx_clk_scmi_set_rate(IMX95_CLK_DRAMALT, clk_val);
  

• break;
  

• case MHZ(333):

• imx_clk_scmi_set_parent(IMX95_CLK_DRAMALT, IMX95_CLK_SYSPLL1_PFD0);
  

• rate = imx_clk_scmi_set_rate(IMX95_CLK_DRAMALT, 333333333);
  

Why is this rate special with hard-coded 333333333 value , while the other rates use clk_val ?

[...]

+void dram_disable_bypass(void) +{

• u64 rate;
• /* Set DRAM APB to 133Mhz */
• imx_clk_scmi_set_parent(IMX95_CLK_DRAMAPB, IMX95_CLK_SYSPLL1_PFD1_DIV2);
• rate = imx_clk_scmi_set_rate(IMX95_CLK_DRAMAPB, 133333333);
• /*Set the DRAM_GPR_SEL to be sourced from DRAM_PLL.*/

/*[+SPACE+]Set the ...

Add the missing spaces into comments globally .

• imx_clk_scmi_set_parent(IMX95_CLK_SEL_DRAM, IMX95_CLK_DRAMPLL);
• rate = imx_clk_scmi_get_rate(IMX95_CLK_SEL_DRAM);
• printf("%s:SEL_DRAM: %llu\n", __func__, rate);

+}

+#endif

+unsigned int mxc_get_clock(enum mxc_clock clk) +{

• switch (clk) {
• case MXC_ARM_CLK:

• return get_arm_core_clk();
  

• case MXC_IPG_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_BUSWAKEUP);
  

• case MXC_CSPI_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_LPSPI1);
  

• case MXC_ESDHC_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_USDHC1);
  

• case MXC_ESDHC2_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_USDHC2);
  

• case MXC_ESDHC3_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_USDHC3);
  

• case MXC_UART_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_LPUART1);
  

• case MXC_FLEXSPI_CLK:

• return imx_clk_scmi_get_rate(IMX95_CLK_FLEXSPI1);
  

• default:

• return -1;
  

• };
• return -1;

+};

Can this clock stuff be finally moved into drivers/clk/ ?

diff --git a/arch/arm/mach-imx/imx9/scmi/clock_scmi.c b/arch/arm/mach-imx/imx9/scmi/clock_scmi.c new file mode 100644 index 0000000000000000000000000000000000000000..543abe9fed481c796602b2b2eeeec6c8f9a94862 --- /dev/null +++ b/arch/arm/mach-imx/imx9/scmi/clock_scmi.c

[...]

+int imx_clk_scmi_enable(u32 clock_id, bool enable) +{

• struct scmi_clk_state_in in = {

• .clock_id = clock_id,
  

• .attributes = (enable) ? 1 : 0,
  

• };
• struct scmi_clk_state_out out;
• struct scmi_msg msg = SCMI_MSG_IN(SCMI_PROTOCOL_ID_CLOCK,

• 			  SCMI_CLOCK_CONFIG_SET,
  

• 			  in, out);
  

• int ret;
• struct udevice *dev;
• ret = uclass_get_device_by_name(UCLASS_CLK, "protocol@14", &dev);

It seems this pattern of

uclass_get_device_by_name(UCLASS_CLK, "protocol@14", &dev); devm_scmi_process_msg(dev, &msg); scmi_to_linux_errno(out.status);

is repeated in multiple functions here. Can this be deduplicated ?

Also, should devm_scmi_process_msg(dev, &msg); be used here ? Who is deallocating the devm_ allocated memory ?

[...]

diff --git a/arch/arm/mach-imx/imx9/scmi/soc.c b/arch/arm/mach-imx/imx9/scmi/soc.c

[...]

+DECLARE_GLOBAL_DATA_PTR;

+rom_passover_t rom_passover_data = {0};

Should this variable be static ?

+uint32_t scmi_get_rom_data(rom_passover_t *rom_data) +{

• /* Read ROM passover data */
• struct scmi_rom_passover_get_out out;
• struct scmi_msg msg = SCMI_MSG(SCMI_PROTOCOL_ID_IMX_MISC, SCMI_MISC_ROM_PASSOVER_GET, out);
• int ret;
• struct udevice *dev;
• ret = uclass_get_device_by_name(UCLASS_CLK, "protocol@14", &dev);
• if (ret)

• return ret;
  

Seems like another duplicate pattern (see above).

• ret = devm_scmi_process_msg(dev, &msg);
• if (ret == 0 && out.status == 0) {

• memcpy(rom_data, (struct rom_passover_t *)out.passover, sizeof(rom_passover_t));
  

• } else {

Invert the conditional here, check for error and bail on error.

• printf("Failed to get ROM passover data, scmi_err = %d, size_of(out) = %ld\n",
  

•        out.status, sizeof(out));
  

• return -EINVAL;
  

• }
• return 0;

+}

+#ifdef CONFIG_ENV_IS_IN_MMC +__weak int board_mmc_get_env_dev(int devno) +{

• return devno;

+}

+int mmc_get_env_dev(void) +{

• int ret;
• u16 boot_type;
• u8 boot_instance;
• volatile gd_t *pgd = gd;
• rom_passover_t *rdata;

+#ifdef CONFIG_SPL_BUILD

That would be XPL now . Also use if IS_ENABLED or CONFIG_IS_ENABLED .

• rdata = &rom_passover_data;

+#else

• rom_passover_t rom_data = {0};
• if (!pgd->reloc_off)

• rdata = &rom_data;
  

• else

• rdata = &rom_passover_data;
  

+#endif

• if (rdata->tag == 0) {

• ret = scmi_get_rom_data(rdata);
  

• if (ret != 0) {
  

• 	puts("SCMI: failure at rom_boot_info\n");
  

• 	return CONFIG_SYS_MMC_ENV_DEV;
  

• }
  

• }
• boot_type = rdata->boot_dev_type;
• boot_instance = rdata->boot_dev_inst;
• set_gd(pgd);
• debug("boot_type %d, instance %d\n", boot_type, boot_instance);
• /* If not boot from sd/mmc, use default value */
• if (boot_type != BOOT_TYPE_SD && boot_type != BOOT_TYPE_MMC)

• return env_get_ulong("mmcdev", 10, CONFIG_SYS_MMC_ENV_DEV);
  

• return board_mmc_get_env_dev(boot_instance);

+} +#endif

+u32 get_cpu_speed_grade_hz(void) +{

• u32 speed, max_speed;
• int ret;
• u32 val, word, offset;
• word = 17;
• offset = 14;
• ret = fuse_read((word / 8), (word % 8), &val);

No need for the parenthesis inside fuse_read() , fix globally.

• if (ret)

• val = 0; /* If read fuse failed, return as blank fuse */
  

• val >>= offset;
• val &= 0xf;
• max_speed = 2300000000;
• speed = max_speed - val * 100000000;
• if (is_imx95())

• max_speed = 2000000000;
  

• /* In case the fuse of speed grade not programmed */
• if (speed > max_speed)

• speed = max_speed;
  

• return speed;

+}

+u32 get_cpu_temp_grade(int *minc, int *maxc) +{

• int ret;
• u32 val, word, offset;
• word = 17;
• offset = 12;
• ret = fuse_read((word / 8), (word % 8), &val);
• if (ret)

• val = 0; /* If read fuse failed, return as blank fuse */
  

• val >>= offset;
• val &= 0x3;
• if (minc && maxc) {

• if (val == TEMP_AUTOMOTIVE) {
  

• 	*minc = -40;
  

• 	*maxc = 125;
  

• } else if (val == TEMP_INDUSTRIAL) {
  

• 	*minc = -40;
  

• 	*maxc = 105;
  

• } else if (val == TEMP_EXTCOMMERCIAL) {
  

• 	*minc = -20;
  

• 	*maxc = 105;
  

• } else {
  

• 	*minc = 0;
  

• 	*maxc = 95;
  

• }
  

• }
• return val;

+}

Can at least some of this be moved into some thermal driver in drivers/thermal/ ?

+static void set_cpu_info(struct ele_get_info_data *info) +{

• gd->arch.soc_rev = info->soc;
• gd->arch.lifecycle = info->lc;
• memcpy((void *)&gd->arch.uid, &info->uid, 4 * sizeof(u32));

+}

+u32 get_cpu_rev(void) +{

• u32 rev = (gd->arch.soc_rev >> 24) - 0xa0;
• return (MXC_CPU_IMX95 << 12) | (CHIP_REV_1_0 + rev);

+}

+#define UNLOCK_WORD 0xD928C520 /* unlock word */ +#define REFRESH_WORD 0xB480A602 /* refresh word */

+static void disable_wdog(void __iomem *wdog_base) +{

• u32 val_cs = readl(wdog_base + 0x00);

Can this be moved to drivers/watchdog/ ?

• 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))

• ;
  

+}

[...]

+int ft_system_setup(void *blob, struct bd_info *bd) +{

• return 0;

+}

+#if defined(CONFIG_ENV_VARS_UBOOT_RUNTIME_CONFIG) +void get_board_serial(struct tag_serialnr *serialnr) +{

• printf("UID: 0x%x 0x%x 0x%x 0x%x\n",

•       gd->arch.uid[0], gd->arch.uid[1], gd->arch.uid[2], gd->arch.uid[3]);
  

• serialnr->low = gd->arch.uid[0];
• serialnr->high = gd->arch.uid[3];

+} +#endif

+static void gpio_reset(ulong gpio_base) +{

• writel(0, gpio_base + 0x10);
• writel(0, gpio_base + 0x14);
• writel(0, gpio_base + 0x18);
• writel(0, gpio_base + 0x1c);

This should be in drivers/gpio in the GPIO driver probe, if needed at all.

+}

+int arch_cpu_init(void) +{

• if (IS_ENABLED(CONFIG_SPL_BUILD)) {

if (!IS_ENABLED(CONFIG_SPL_BUILD)) return 0;

• disable_wdog((void __iomem *)WDG3_BASE_ADDR);
  

• disable_wdog((void __iomem *)WDG4_BASE_ADDR);
  

• clock_init_early();
  

• gpio_reset(GPIO2_BASE_ADDR);
  

• gpio_reset(GPIO3_BASE_ADDR);
  

• gpio_reset(GPIO4_BASE_ADDR);
  

• gpio_reset(GPIO5_BASE_ADDR);
  

• }
• return 0;

+}

+int imx9_probe_mu(void) +{

• struct udevice *dev;
• int node, ret;
• u32 res;
• struct ele_get_info_data info;
• ret = uclass_get_device_by_driver(UCLASS_SCMI_AGENT, DM_DRIVER_GET(scmi_mbox), &dev);
• if (ret)

• return ret;
  

• ret = uclass_get_device_by_name(UCLASS_CLK, "protocol@14", &dev);
• if (ret)

• return ret;
  

• ret = devm_scmi_of_get_channel(dev);
• if (ret)

• return ret;
  

• ret = uclass_get_device_by_name(UCLASS_PINCTRL, "protocol@19", &dev);
• if (ret)

• return ret;
  

Is this really needed ? MU would be probed on first use automatically somewhere else.

[...]