Re: pinctrl: zynqmp: Valid pin muxings cannot be configured

[Sean Anderson <sean.anderson@xxxxxxxxx>]

On 4/24/24 02:22, Michal Simek wrote:
> Hi Sean,
> 
> On 4/24/24 01:04, Sean Anderson wrote:
>> Hi Michal,
>>
>> I was looking to upstream one of our ZynqMP boards, and I ran into an
>> issue with the pinmuxing. We use almost all of the I/Os, so everything
>> is tightly packed into the MIO. For example, we have the QSPI on MIO0 to
>> MIO5, and MIO6 to MIO11 are used for SPI1. However, I cannot select this
>> configuration using the pinmux driver. I am using the following
>> configuration:
>>
>> pinctrl_qspi_default: qspi-default {
>>     mux {
>>         groups = "qspi0_0_grp";
>>         function = "qspi0";
>>     };
>>
>>     mux-cs {
>>         groups = "qspi_ss_0_grp";
>>         function = "qspi_ss";
>>     };
>> };
>>
>> pinctrl_spi1_default: spi1-default {
>>     mux {
>>         groups = "spi1_0_grp";
>>         function = "spi1";
>>     };
>>
>>     mux-cs {
>>         groups = "spi1_ss_0_grp", "spi1_ss_1_grp";
>>         function = "spi1_ss";
>>     };
>> };
>>
>> But I get the following errors on boot:
>>
>> [    4.261739] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: pin MIO8 already requested by ff050000.spi; cannot claim for ff0f0000.spi
>> [    4.274506] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: error -EINVAL: pin-8 (ff0f0000.spi)
>> [    4.283789] zynqmp-pinctrl firmware:zynqmp-firmware:pinctrl: error -EINVAL: could not request pin 8 (MIO8) from group qspi0_0_grp  on device zynqmp_pinctrl
>>
>> This is because the qspi0_0_grp and spi1_0_grp groups overlap:
>>
>> group: qspi0_0_grp
>> pin 0 (MIO0)
>> pin 1 (MIO1)
>> pin 2 (MIO2)
>> pin 3 (MIO3)
>> pin 4 (MIO4)
>> pin 8 (MIO8)
>> pin 9 (MIO9)
>> pin 10 (MIO10)
>> pin 11 (MIO11)
>> pin 12 (MIO12)
>>
>> group: qspi_ss_0_grp
>> pin 5 (MIO5)
>> pin 7 (MIO7)
>>
>> group: qspi_fbclk_0_grp
>> pin 6 (MIO6)
>>
>> group: spi1_0_grp
>> pin 6 (MIO6)
>> pin 10 (MIO10)
>> pin 11 (MIO11)
>>
>> group: spi1_ss_0_grp
>> pin 9 (MIO9)
>>
>> group: spi1_ss_1_grp
>> pin 8 (MIO8)
>>
>> group: spi1_ss_2_grp
>> pin 7 (MIO7)
>>
>> However, we are not using the "upper" pins of the QSPI device.
>> Therefore, these pins should not be included in the qspi0_0_grp. This
>> stems from the driver placing all possible pins into a function's group,
>> even though each pin can be muxed individially and it is not necessary
>> to mux all pins for full functionality.
> 
> Correct. These configurations were not consider at that time when code
> was written. The same issue is there if you want to combine pins from
> different groups. IIRC uart rx via MIOX and tx not from MIOX+1.
> 
>>
>> I think it would be better to have a single group for each pin:
>>
>> pinctrl_qspi_default: qspi-default {
>>     mux {
>>         groups = "mio0", "mio1", "mio2", "mio3", "mio4";
>>         function = "qspi0";
>>     };
>>
>>     mux-cs {
>>         groups = "mio5";
>>         function = "qspi_ss";
>>     };
>> };
>>
>> pinctrl_spi1_default: spi1-default {
>>     mux {
>>         groups = "mio6", "mio10", "mio11";
>>         function = "spi1";
>>     };
>>
>>     mux-cs {
>>         groups = "mio8", "mio9";
>>         function = "spi1_ss";
>>     };
>> };
>>
>> This allows the full functionality of this chip to be configured. Does
>> that sound good? I can send a patch to this effect if you agree.
> 
> The only question is if this can be done without changing TF-A code
> because we are running out of space in OCM for it.

I think this can be done just in the Linux driver. This would also be
convenient because then it will work regardless of the firmware.

> Just a generic question to your problem. It doesn't sound like a
> dynamic case. You have static assignment for pins which likely won't
> change over lifecycle. QSPI can be even boot device. Do you really
> need to describe pins via DT that it is not enough to have them
> configured via psu_init directly?
>
> Driver has been developed for i2c bus recovery via gpio which was the
> main application. Right now Kria SOM is using it for carrier card pins
> configuration.  And Kria is pretty much only platform where this is
> regularly tested.

This is just following the example of the ZCU102. But you're right,
these pin configurations are static (excepting I2C).

--Sean