Re: [PATCH 0/2] pinctrl: zynqmp: Support muxing individual pins

[Sean Anderson <sean.anderson@xxxxxxxxx>]

On 5/29/24 04:38, Linus Walleij wrote:
> On Tue, May 28, 2024 at 4:28 PM Sean Anderson <sean.anderson@xxxxxxxxx> wrote:
> 
>> Well, perhaps you should have reviewed the original driver more
>> closely.
> 
> Do you want to push me down and increase my work related
> stress? Because that is the effect of such statements.
> 
> It looks like criticism of me as a person, so explain yourself.
> 
> Writing this kind of things looks to me like some kind of abusive way
> to express your desire and that is what burns maintainers out, so
> if that is what you are doing, stop doing that, adjust your behaviour
> and focus on technical issues.

The technical issue is that the driver does not match the hardware. We
must maintain the existing set of groups for backwards-compatibility.
But this should not prevent improvement.

Saying that we cannot have both group styles means that the driver is
permanently stuck with whatever was picked when it was submitted. Hence,
if you want to have only one style you had better review new drivers
very carefully.

>> > 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
> 
> Please share your calculations for figures like "thousands".
> 
> In my experience, groups are usually in the tens, perhaps
> hundreds, physically restricted by the number of pins
> underneath a BGA. A Micro-FCBGA has 479 balls and many
> are GND and power, so that sets a ballpark figure.

There are 78 muxable pins on this hardware, and around 40 groups, each
with signals that can be muxed to each pin. If we were to create groups
for each combination of signals and pins, there would literally be
thousands of groups.

>> for every combination of pin musings
> 
> It is clear from the documentation that the point if the pinmux
> groups and pins are not to present all possible options (known as
> a "phone exchange" solution) but those that are used in practice,
> i.e. these representing real world use cases. See below.
> 
>> 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.
> 
> Historically there are  ~2 camps:
> 
> - One camp want to use groups and
> functions to combine pins in groups with functions to form usecases.
> 
> In some cases (such as pinctrl-gemini.c or the very latest
> pinctrl-scmi.c merged for v6.10) this reflects how the hardware
> actually looks: it does not make individual pins available for muxing,
> but you poke bits or send messages to change entire
> groups-to-function mappings, so it is necessary for some hardware.
> 
> So when you write that "groups are a Linux-only concept" this
> is because you probably haven't seen this part of the world.
> Groups exist in hardware, and in the SCMI specification.

What I mean is that, for this hardware, groups are a Linux only concept.
Neither the firmware nor the hardware itself has a concept of groups.
While other hardware may have this concept, it does not apply here.

I do not object to groups where that is the hardware reality, but they
are unnecessarily constraining for this part.

> There are systems with individual control of the muxing
> of every pin, such that e.g. every pin has a muxing register.
> 
> These are again not really phone exchanges: I am yet to see
> a system where any function can be mapped to any pin. These
> just do not exist.

Canaan K210.

> What exists in practice is that each pin can be mapped to 2-4
> functions, in extreme cases some more. Often these functions are
> mapped to adjacent pins, and the "chessboard" picture in the
> documentation for the subsystem reflects this.
> 
> For this reason, it is often helpful for driver writers to group
> adjacent pins into groups, so an iterator can walk over the
> pins and poke their registers in order, instead of treating each
> pin as a unique entity.
> 
> - Then there is the camp that just by habit *want* to control
> each pin individually. The extreme example is pinctrl-single.c
> which is named like such because each pin is controlled by
> a single register. TI wanted this solution mainly because their
> hardware wasn't described in manuals, but in other HW
> description files, and they needed to process large volumes
> of data into DT-form.
> 
> I didn't like this solution initially because it makes it hard for
> people without datasheets to understand what is going on.
> But I was convinced to let this coexist with the group and function
> mapping, which is fine: maybe one size doesn't fit all.
> 
> i.MX and others also do this approach but with large sets of
> defines in the <dt-bindings/*> files.
> 
> Combining these two approaches is not something I recommend.

Well, the former approach is wrong for this hardware, but we must
support it for backwards-compatibility. A combination is the obvious
solution.

--Sean