RE: [EXT] Re: [PATCH v2 1/1] gpio: mxc: add suspend/resume support for i.mx8x SoCs

[Shenwei Wang <shenwei.wang@xxxxxxx>]

> -----Original Message-----
> From: Linus Walleij <linus.walleij@xxxxxxxxxx>
> Sent: Friday, October 21, 2022 3:27 AM
> To: Shenwei Wang <shenwei.wang@xxxxxxx>
> > function PIN or not, but you can't know which GPIO port and which GPIO line
> the PIN belongs to.
> 
> Cross-referencing GPIO line numbers to pins/pads is literally the job of gpio-
> ranges (the DT property) and we have excellent support in the GPIO library to do
> exactly this.
> 
> It can even be made using static data in the driver if gpio-ranges for some
> reason cannot be encoded in the device tree.
> 

The current implementation is using the static data inside the gpio driver, although it
is not used the gpio-range data structure. 

> (...)
> > In order to build up the cross-reference between GPIO lines and the
> > PINs, you need have a pre-prepared gpio-ranges mapping in advance.
> > Because the relationship between the PIN and the GPIO line is not a
> > linear, you end up to build up the gpio-ranges in the same way like the array in
> this patch. This turns to a chicken-egg problem.
> 
> I don't get it. gpio-ranges can contain any number of "holes" and whatever, the
> name "rangeS" (plural) implies it can be an array of ranges. If you want you can
> have a list of single-pin ranges, no problem.
> 

 No matter you prepare the mapping by using the gpio-ranges or the static array in the 
driver,  they serve the same purpose. The only user to use the mapping is the gpio driver,
and the mapping is constant for a SoC family. That's why I chose to define the array inside
the driver.

> > As I explained above, this is not the duplicated information. It adds
> > another dimension of a information regarding a PIN. As these
> > information is standalone and self-contained,  no matter how you
> > change the pin tables, it won't impact the function here.
> 
> Since the information/setting pertains to an electronic or similar property of a
> pin it falls under the pin config umbrella.
> 
> > This is a kind of GPIO wakeup function, and it happened to be given a
> > name called pad wakeup. From the user point of view, it works the same
> > way as the native GPIO wakeup. Except the name itself, it has nothing to do
> with the PINCTRL.
> 
> Hardly.
> 
> The MMC/SD card bus has ways of waking up devices by pulling a line low, as
> does things such as UARTs. And then the pin isn't even used as GPIO.
> 

That's another kind of wakup method. Currently we are talking about the pad
wakeup. The examples you gave like SD/UART, they are not pad wakeup feature,
we can call them as IP native wakeup. For example, both GPIO and SD ( or UART) have their
own IP native wakeup function. The difference here on i.MX8 platform is that
if you use UART's native wakeup, you have to keep power/clock to the UART IP block during the 
sleep state. The same requirement for the GPIO and SD's native wakeup too. 

> I bet a million to one that the exact same setting is used inside the SCU for
> waking up on such events on the pins.
> 

For those IP blocks' native wakeup, they are not managed by SCU. It is managed by
the block's driver directly. For example, the UART's native wakeup function is
enabled or disabled via the driver itself, and no communication required for SCU.

> I don't believe that just because a pin is muxed to something else than GPIO it
> cannot be programmed to wake the system up.
> 
> If you look into my presentation "building GPIO and pins from the ground up":
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.df.lt
> h.se%2F~triad%2Fpapers%2Fpincontrol.pdf&amp;data=05%7C01%7Cshenwei.w
> ang%40nxp.com%7C5f49f959229e452da06408dab33e0e38%7C686ea1d3bc2b4
> c6fa92cd99c5c301635%7C0%7C1%7C638019376349152251%7CUnknown%7CT
> WFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXV
> CI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=Cqud0jkMikeKz1Ok%2Bgm55mx
> kCeK3HQlqRU5n5RxU8VU%3D&amp;reserved=0
> see the picture on page 13 and page 18. The asynchronous edge detector is
> what takes the system online in sleep. That is a property of the pin cell, it has
> nothing to do with the GPIO block, which is further in.
> 
> It belongs in pin control because it is a property of the pin hardware.

That's a very good presentation, but it is away from the topic. The GPIO block has
its own edge detector logic for its native wakeup function. Here the pad wakeup
feature is implemented via an extra hardware logic, and it is an independent IP block.

> 
> > As I explained in the above, this is not the problem of two clients
> > accessing the same resource, so there is no conflict between the two
> > drivers. It works in the same way like SCU power domain driver and SCU
> > clock driver. The communication integrity is guaranteed by the call of
> imx_scu_call_rpc itself.
> 
> I understand that it makes your life simpler to just implement this as a hack in
> the MXC GPIO driver like this, but it is not a proper solution, and I ask you to do
> the more complicated and work consuming task instead.
> 

This is not a hack because the imx_scu_call_rpc function is designed to serve multiple threads
use case. As the pad wakeup function is an independent feature accompany 
with the gpio function, I prefer to put the implementation inside the gpio driver.

Regards,
Shenwei

> This is because it will have partitioned the problem in a clear way that is
> recognizable by the maintainers and will scale to other SoCs in the future.
> 
> I also wonder where all the other i.MX maintainers are. I need their input in this
> discussion.
> 
> Yours,
> Linus Walleij