[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 at 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