Re: [PATCH] pinctrl: zynqmp: Add SPL support

[Michal Simek <michal.simek@amd.com>]

On 1/6/26 23:42, Sean Anderson wrote:
> Although the pinctrl pm requests are implemented in the PMU firmware,
> PM_QUERY_DATA is actually implemented in ATF. In SPL (or when running in
> EL3), ATF is not yet running, so we need to implement this API
> ourselves. Do the bare minimum, allowing SPL to enumerate functions, but
> don't bother with groups. Groups take up a lot of space, and can be
> emulated with pins. For example, a node like
> 
> 	display-port {
> 		mux {
> 			groups = "dpaux0_1";
> 			function = "dpaux0";
> 		};
> 	};
> 
> can be replaced by
> 
> 	display-port {
> 		mux {
> 			pins = "MIO34", "MIO35", "MIO36", "MIO37";
> 			function = "dpaux0";
> 		};
> 	};
> 
> While this isn't backwards-compatible with existing devicetrees, it's
> more than enough for SPL where we may only need to mux one or two pins.
> 
> Signed-off-by: Sean Anderson <sean.anderson@linux.dev>
> ---
> 
>   drivers/firmware/firmware-zynqmp.c | 100 +++++++++++++++++++++++++++++
>   1 file changed, 100 insertions(+)
> 
> diff --git a/drivers/firmware/firmware-zynqmp.c b/drivers/firmware/firmware-zynqmp.c
> index f8a9945c1da..77911757177 100644
> --- a/drivers/firmware/firmware-zynqmp.c
> +++ b/drivers/firmware/firmware-zynqmp.c
> @@ -427,6 +427,102 @@ U_BOOT_DRIVER(zynqmp_power) = {
>   };
>   #endif
>   
> +static const char *const pinctrl_functions[] = {
> +	"can0",
> +	"can1",
> +	"ethernet0",
> +	"ethernet1",
> +	"ethernet2",
> +	"ethernet3",
> +	"gemtsu0",
> +	"gpio0",
> +	"i2c0",
> +	"i2c1",
> +	"mdio0",
> +	"mdio1",
> +	"mdio2",
> +	"mdio3",
> +	"qspi0",
> +	"qspi_fbclk",
> +	"qspi_ss",
> +	"spi0",
> +	"spi1",
> +	"spi0_ss",
> +	"spi1_ss",
> +	"sdio0",
> +	"sdio0_pc",
> +	"sdio0_cd",
> +	"sdio0_wp",
> +	"sdio1",
> +	"sdio1_pc",
> +	"sdio1_cd",
> +	"sdio1_wp",
> +	"nand0",
> +	"nand0_ce",
> +	"nand0_rb",
> +	"nand0_dqs",
> +	"ttc0_clk",
> +	"ttc0_wav",
> +	"ttc1_clk",
> +	"ttc1_wav",
> +	"ttc2_clk",
> +	"ttc2_wav",
> +	"ttc3_clk",
> +	"ttc3_wav",
> +	"uart0",
> +	"uart1",
> +	"usb0",
> +	"usb1",
> +	"swdt0_clk",
> +	"swdt0_rst",
> +	"swdt1_clk",
> +	"swdt1_rst",
> +	"pmu0",
> +	"pcie0",
> +	"csu0",
> +	"dpaux0",
> +	"pjtag0",
> +	"trace0",
> +	"trace0_clk",
> +	"testscan0",
> +};
> +
> +/*
> + * PM_QUERY_DATA is implemented by ATF and not the PMU firmware, so we have to
> + * emulate it in SPL. Just implement functions/pins since the groups take up a
> + * lot of rodata and are mostly superfluous.
> + */
> +static int zynqmp_pm_query_data(enum pm_query_id qid, u32 arg1, u32 arg2,
> +				u32 *ret_payload)
> +{
> +	switch (qid) {
> +	case PM_QID_PINCTRL_GET_NUM_PINS:
> +		ret_payload[1] = 78;

Macro for this value please.

> +		ret_payload[0] = 0;
> +		return 0;
> +	case PM_QID_PINCTRL_GET_NUM_FUNCTIONS:
> +		ret_payload[1] = ARRAY_SIZE(pinctrl_functions);
> +		ret_payload[0] = 0;
> +		return 0;
> +	case PM_QID_PINCTRL_GET_NUM_FUNCTION_GROUPS:
> +		ret_payload[1] = 0;
> +		ret_payload[0] = 0;
> +		return 0;
> +	case PM_QID_PINCTRL_GET_FUNCTION_NAME:
> +		memset(ret_payload, 0, 16);

Where is this 16 coming from? I expect this is max number of chars of function.


> +		strcpy((char *)ret_payload, pinctrl_functions[arg1]);

you should check that arg1 is between 0 and array size not to read value behind.


> +		return 0;
> +	case PM_QID_PINCTRL_GET_FUNCTION_GROUPS:
> +	case PM_QID_PINCTRL_GET_PIN_GROUPS:
> +		memset(ret_payload + 1, 0xff, 12);

Where is this 12 coming from? Macro for it.


> +		ret_payload[0] = 0;
> +		return 0;
> +	default:
> +		ret_payload[0] = 1;
> +		return 1;
> +	}
> +}
> +
>   smc_call_handler_t __data smc_call_handler;
>   
>   static int smc_call_legacy(u32 api_id, u32 arg0, u32 arg1, u32 arg2,
> @@ -493,6 +589,10 @@ int __maybe_unused xilinx_pm_request(u32 api_id, u32 arg0, u32 arg1, u32 arg2,
>   	      __func__, current_el(), api_id, arg0, arg1, arg2, arg3, arg4, arg5);
>   
>   	if (IS_ENABLED(CONFIG_XPL_BUILD) || current_el() == 3) {
> +		if (IS_ENABLED(CONFIG_ARCH_ZYNQMP) &&
> +		    IS_ENABLED(CONFIG_PINCTRL_ZYNQMP) &&

This logic have to change a little bit.
There is also CONFIG_SPL_PINCTRL symbol which likely needs to be enabled. But if 
it is not still this code ends in SPL and just extend binary.

Don't think you want to support running U-Boot out of EL3 but if yes that logic 
should be adjusted in connection to CONFIG_SPL_PINCTRL.




> +		    api_id == PM_QUERY_DATA)
> +			return zynqmp_pm_query_data(arg0, arg1, arg2, ret_payload);
>   #if defined(CONFIG_ZYNQMP_IPI)
>   		/*
>   		 * Use fixed payload and arg size as the EL2 call. The firmware

M