Received-SPF: pass (google.com: domain of linux-kernel+bounces-155987-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Message-ID: <5bb0dc7e-4c89-4f3d-abc6-41ae9ded5ae9@linux.dev>
Date: Tue, 23 Apr 2024 19:04:59 -0400
Precedence: bulk
MIME-Version: 1.0
Content-Language: en-US
To: Michal Simek <michal.simek@amd.com>,
 "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>
Cc: Linus Walleij <linus.walleij@linaro.org>,
 "linux-arm-kernel@lists.infradead.org"
 <linux-arm-kernel@lists.infradead.org>,
 "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
From: Sean Anderson <sean.anderson@linux.dev>
Subject: pinctrl: zynqmp: Valid pin muxings cannot be configured
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

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.

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.

--Sean