Re: [PATCH v4 1/2] x86: Support APU5 & APU6 in PCEngines platform driver

[Ed W <lists@wildgooses.com>]

On 19/01/2023 10:22, Hans de Goede wrote:

>>  /* Order in which the GPIO lines are defined in the register list */
>>  #define APU2_GPIO_LINE_LED1		0
>>  #define APU2_GPIO_LINE_LED2		1
>>  #define APU2_GPIO_LINE_LED3		2
>>  #define APU2_GPIO_LINE_MODESW		3
>> -#define APU2_GPIO_LINE_SIMSWAP		4
>> -#define APU2_GPIO_LINE_MPCIE2		5
>> -#define APU2_GPIO_LINE_MPCIE3		6
>> +#define APU2_GPIO_LINE_RESETM1		4
>> +#define APU2_GPIO_LINE_RESETM2		5
>> +#define APU2_GPIO_LINE_SIMSWAP		6
> I don't think this changing of GPIO ordering, or
> for that part the changing of the gpio_names from 
> "mpcie2_reset" to "modem1-reset" is a good idea.
>
> I'm not entirely sure how these GPIOs are supposed to be
> consumed / used by userspace. But since they are not used
> directly in this driver I assume userspace is supposed to
> use either the (deprecated) sysfs GPIO API or the new ioctl
> based GPIO API to toggle say "simswap" if it needs to.
>
> The old sysfs API exclusively uses pin-indexes inside a GPIO
> chip to select the pin, so by changing the pin order you
> have just broken the userspace API.
>
> And the new ioctl API can use either pin-indexes or GPIO-line-names,
> so by changing the names you have also just potentially broken
> that.
>
> Please keep the order as is and only use the new names for
> the newly added models (so for APU6 I believe).


Hi, I'm not sure what the "correct" thing to do is, but just to add some background to the situation:

There are an increasing number of APU boards, which are *very* similar, and also through time the
pin allocations have muddled around, plus most recently, the BIOS can configure many things and has
started to use naming conventions different to the historic kernel naming

So I don't have a board in front of me to be definitive, but something like the following happened:

- APU2 used something like mpcie sockets 1&2 for USB stuff and hence LTE cards, socket 3 was msata

- Then another version APU3, I think moved these to sockets 2&3

- Then another version APU4, moved the USB to sockets 2&3 and wired up a second SIM slot in most
versions, including a SIM line swapper chip. Now you start to wonder if you should have labelled
things PCIE1, PCIE2, PCIE3, etc, when really they mean modem 1 and modem 2, etc?

- Then came APU5, which has 3x USB sockets, plus 3x mpcie sockets. These are wired to different pcie
numbers, and so the naming modem1, modem2, modem3 starts to make a lot more sense.

- APU6, which is mentioned in the original patch, is really just the same as one of the other
boards, but with different ethernet sockets (SFP vs copper)


- There is also a rare feature, which is likely not known to most users, or even wired up correctly
on many boards. You have a reset/enable line to some of the mpcie slots. This again makes more sense
to label logically vs than per slot. It's really not clear that this feature is properly supported
or functioning on all boards (you can order special order boards wired in various ways). So changes
here are unlikely to be noticed by all but a handful of specialist users.


Overall, if one could start again, the unifying feature would be label slots logically, ie modem1,
modem2, wifi1, wifi2, rather than numbering them based on how they are wired on a specific board rev.

Additionally, users who didn't load the APU driver, likely had ACPI named devices and these all have
different (and to my eye, more logical names). So whatever we decide to do here will cause some
breakage and inconsistency...


Note that I submitted this previous patch "years ago", and I've somewhat given up on ever getting
the APU driver up to date.. I think in 2020, Enrico shot me down because he was working on some
grand unification for modem GPIO handling? (Enrico, please correct me on the details?) Hans, I think
if you search back to 2020 on "APU", you will see that you arbitrated in that thread? For whatever
reason, we seem to be stuck that there are competing voices blocking progress here. Every route
leads to some level of incompatibility. Personally I am a fairly large consumer of these devices,
but I really don't care what we decide, because we ship a custom software, where userspace will
match kernel, so we will update both in lockstep, whatever happens. Changes aren't a problem for me
personally.

My vote would be for a one-of breakage, to at least get everyone using the same
terms/names/whatever. I would suspect OpenWRT is probably the biggest voice here, so suggest we go
with whatever they suggest, and then at least we are all in sync for the future? If its a one off,
then suggest taking into account the ACPI naming as well?

Note, there is a very big risk that I missed the point... Please be gentle. Quite possibly there is
a solution to just reorder some definitions and we land where we want to be? Is it that simple?


Hopefully the above helps understanding the bigger picture?

All the best

Ed W