Message-ID: <51d984f5-896e-469f-914d-2c902be91748@linux.dev>
Date: Tue, 28 May 2024 10:28:51 -0400
From: Sean Anderson <sean.anderson@...ux.dev>
To: Linus Walleij <linus.walleij@...aro.org>
Cc: Michal Simek <michal.simek@....com>, linux-gpio@...r.kernel.org,
 Krishna Potthuri <sai.krishna.potthuri@....com>,
 linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
 Conor Dooley <conor+dt@...nel.org>, Krzysztof Kozlowski
 <krzk+dt@...nel.org>, Rob Herring <robh@...nel.org>,
 devicetree@...r.kernel.org
Subject: Re: [PATCH 0/2] pinctrl: zynqmp: Support muxing individual pins

On 5/27/24 09:15, Linus Walleij wrote:
> On Mon, May 6, 2024 at 4:45 PM Sean Anderson <sean.anderson@...ux.dev> wrote:
> 
>> > Then we realize that not everyone need all the modem
>> > control signals provided. What to do. Well this:
>> >
>> > uart0_rxtx_grp = pin_rx, pin_tx:
>> > uart0_modem_grp = pin_cts, pin_dts, pin_dcd;
>> >
>> > mux0:
>> >     function = "uart0";
>> >     groups = "uart0_rxtx_grp";
>> >
>> > Now the CTS, DTS, DCD pins can be reused for something
>> > else such as GPIO.
>> >
>> > I *know* that this breaks ABI: the driver group definitions change
>> > and the device tree needs to be changed too.
> 
> Actually I didn't think that over, it is possible to add new groups
> and retain the old ones.
> 
> I.e. retain uart0_grp, but additionally add and use
> uart0_rxtx and uart0_modem_grp and use one or the
> other approach.

That is what this patch does.

>> Well, the pin groups are actually defined in the PMU firmware.
> 
> Is that firmware written in such an helpful way that the groups
> can be extracted from the firmware then, as with SCMI? Or is it
> a matter of duplicating the info from the PMU in the software-defined
> groups.

Fundamentally, the pin muxings are known a priori from the reference
manual. Because pinmuxing itself has been delegated to the PMU firmware,
we defer to it when determining what muxings are available. The PMU
firmware describes muxings in terms of pins and functions; groups are a
Linux-only concept.

>> And
>> frankly, I don't see the point of pin "groups" when there are not actual
>> pin groups at the hardware level. The pins can all be muxed
>> individually, so there's no point in adding artificial groups on top.
>> Just mux the pins like the hardware allows and everything is easy. Cuts
>> down on the absurd number of strings too.
> 
> So are you going to switch all of Xilinx devicetrees over to using exclusively
> the new method (muxing individual pins)?

No. We have to support it anyway for compatibility, so there is no point
in changing everything for no reason.

> I'm fine with one (string identified groups) which I encourage, but I
> let individual pin control pass as well on several occasions.
> 
> What I don't want to see is a Franken-solution that mixes the two
> approaches, even less so on the same system. Someone is going to
> have to maintain the resulting mess. And this looks like exactly that.

Well, perhaps you should have reviewed the original driver more
closely.

> If you want to mux individual pins instead of groups and functions, by
> all means, but please do not mix the two approaches in the same
> driver, I'm just trying to save Xilinx from themselves here.

I see no point in creating thousands of groups for every combination of
pin muxings when we could just switch to the solution in this (or v2 of)
patch. For compatibility we cannot be rid of the old situation, but we
can at least fix it. There is no technical problem with them coexisting.

--Sean

Powered by blists - more mailing lists