[PATCH] pinctrl: zynqmp: Add SPL support

[PATCH] pinctrl: zynqmp: Add SPL support

[Sean Anderson]

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;
+		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);
+		strcpy((char *)ret_payload, pinctrl_functions[arg1]);
+		return 0;
+	case PM_QID_PINCTRL_GET_FUNCTION_GROUPS:
+	case PM_QID_PINCTRL_GET_PIN_GROUPS:
+		memset(ret_payload + 1, 0xff, 12);
+		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) &&
+		    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
-- 
2.35.1.1320.gc452695387.dirty

base-commit: 38ace442b630c5ddf049af83e8db229c012ed355
branch: pinctrl_zynqmp_spl

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

[Michal Simek]

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

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

[Sean Anderson]

On 1/16/26 03:17, Michal Simek wrote:
> 
> 
> 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.

Why? This value is used exactly once, and its semantics can be
directly inferred from the previous line.

>> +        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.

Yes. It comes from ATF. I can move MAX_FUNC_NAME_LEN to
include/zynqmp_firmware.h so we can use it here.

>> +        strcpy((char *)ret_payload, pinctrl_functions[arg1]);
> 
> you should check that arg1 is between 0 and array size not to read value behind.

This is guaranteed by the loop condition in zynqmp_pinctrl_probe. I can
add an assert, but we have one internal user so we don't really need
validation beyond what would be necessary for something like

	for (i = 0; i < ARRAY_SIZE(pinctrl_functions); i++)
		pinctrl_functions[i];

>> +        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.

NUM_GROUPS_PER_RESP * sizeof(u16)

>> +        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.

PINCTRL_ZYNQMP is not enabled in any defconfig for U-Boot proper, so it
will not normally be enabled in SPL either.

> 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.

I think it's reasonable to support this, which is why I left the
condition as-is.

--Sean

>> +            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

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

[Michal Simek]

On 1/16/26 16:59, Sean Anderson wrote:
> On 1/16/26 03:17, Michal Simek wrote:
>>
>>
>> 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.
> 
> Why? This value is used exactly once, and its semantics can be
> directly inferred from the previous line.

But it is not clear what this is referring to. I expect it is amount of MIO PINs 
but macro name would make it clear that it is not random number.

> 
>>> +        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.
> 
> Yes. It comes from ATF. I can move MAX_FUNC_NAME_LEN to
> include/zynqmp_firmware.h so we can use it here.
> 
>>> +        strcpy((char *)ret_payload, pinctrl_functions[arg1]);
>>
>> you should check that arg1 is between 0 and array size not to read value behind.
> 
> This is guaranteed by the loop condition in zynqmp_pinctrl_probe. I can
> add an assert, but we have one internal user so we don't really need
> validation beyond what would be necessary for something like

But you never know who use/call this code in future.

> 
> 	for (i = 0; i < ARRAY_SIZE(pinctrl_functions); i++)
> 		pinctrl_functions[i];
> 
>>> +        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.
> 
> NUM_GROUPS_PER_RESP * sizeof(u16)

much better.

> 
>>> +        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.
> 
> PINCTRL_ZYNQMP is not enabled in any defconfig for U-Boot proper, so it
> will not normally be enabled in SPL either.

It is in xilinx_zynqmp_kria_defconfig

Thanks,
Michal

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

[Sean Anderson]

On 1/19/26 03:08, Michal Simek wrote:
> 
> 
> On 1/16/26 16:59, Sean Anderson wrote:
>> On 1/16/26 03:17, Michal Simek wrote:
>>>
>>>
>>> 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.
>>
>> Why? This value is used exactly once, and its semantics can be
>> directly inferred from the previous line.
> 
> But it is not clear what this is referring to. I expect it is amount of MIO PINs but macro name would make it clear that it is not random number.

ret_payload[1] = 78; /* NUM_PINS */

>>
>>>> +        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.
>>
>> Yes. It comes from ATF. I can move MAX_FUNC_NAME_LEN to
>> include/zynqmp_firmware.h so we can use it here.
>>
>>>> +        strcpy((char *)ret_payload, pinctrl_functions[arg1]);
>>>
>>> you should check that arg1 is between 0 and array size not to read value behind.
>>
>> This is guaranteed by the loop condition in zynqmp_pinctrl_probe. I can
>> add an assert, but we have one internal user so we don't really need
>> validation beyond what would be necessary for something like
> 
> But you never know who use/call this code in future.

I don't think there will ever be any new users outside of the pinctrl driver.

>>
>>     for (i = 0; i < ARRAY_SIZE(pinctrl_functions); i++)
>>         pinctrl_functions[i];
>>
>>>> +        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.
>>
>> NUM_GROUPS_PER_RESP * sizeof(u16)
> 
> much better.
> 
>>
>>>> +        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.
>>
>> PINCTRL_ZYNQMP is not enabled in any defconfig for U-Boot proper, so it
>> will not normally be enabled in SPL either.
> 
> It is in xilinx_zynqmp_kria_defconfig

Ah, interesting. That is new since the U-Boot I've been working on.

I'll add a CONFIG_PINCTRL_ZYNQMP_SPL to test for.

--Sean