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: Tuesday, October 18, 2022 3:21 AM
> > > > +       {  2, IMX8QM_SIM0_IO},
> > > > +       {  3, IMX8QM_SIM0_PD},
> > >
> > > this is pin control.
> > >
> > > I think you need to think about adding this to the i.MX pin control
> > > driver instead, possibly cooperaring with the GPIO driver.
> > >
> > > I'm not entirely certain about the relationship between MXC and i.MX
> > > so correct me if I get this wrong...
> >
> > This is not pin control.
> 
> So when something in a driver starts to enumerate all the pads on a system and
> assign them properties, I tend to think it is some kind of pin control.
> 
> You have things like this:
> 
> +const struct mxc_gpio_pad_map imx8qm_pad_map[] = {
> +       /* GPIO0 */
> +       {  0, IMX8QM_SIM0_CLK},
> +       {  1, IMX8QM_SIM0_RST},
> +       {  2, IMX8QM_SIM0_IO},
> +       {  3, IMX8QM_SIM0_PD},
> +       {  4, IMX8QM_SIM0_POWER_EN},
> +       {  5, IMX8QM_SIM0_GPIO0_00},
> +       {  6, IMX8QM_M40_I2C0_SCL},
> +       {  7, IMX8QM_M40_I2C0_SDA},
> +       {  8, IMX8QM_M40_GPIO0_00},
> +       {  9, IMX8QM_M40_GPIO0_01},
> +       { 10, IMX8QM_M41_I2C0_SCL},
> (...)
> 
> 
> Which is duplicating the same defines in
> drivers/pinctrl/freescale/pinctrl-imx8qm.c:
> 
> static const struct pinctrl_pin_desc imx8qm_pinctrl_pads[] = {
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_CLK),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_RST),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_IO),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_PD),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_POWER_EN),
>         IMX_PINCTRL_PIN(IMX8QM_SIM0_GPIO0_00),
>         IMX_PINCTRL_PIN(IMX8QM_COMP_CTL_GPIO_1V8_3V3_SIM),
>         IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SCL),
>         IMX_PINCTRL_PIN(IMX8QM_M40_I2C0_SDA),
>         IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_00),
>         IMX_PINCTRL_PIN(IMX8QM_M40_GPIO0_01),
>         IMX_PINCTRL_PIN(IMX8QM_M41_I2C0_SCL),
> (...)
> 

Thank you very much for the detailed review.

#define PINCTRL_PIN(a, b) { .number = a, .name = b }
#define IMX_PINCTRL_PIN(pin) PINCTRL_PIN(pin, #pin)

The imx8qm_pinctrl_pads array gives each PIN a string type of name.  The array I defined in
this patch adds another dimension information of the PIN, which is the GPIO index. These
two are not duplicated information. 

When I started to implement this feature, the first thing I thought is to use the existing
information to  calculate the GPIO index, but sadly I couldn't find a solution in the end.
The purpose for the array here is to record the relationship of a GPIO line with a PIN.
>From the PINCTRL driver, you may get the information regarding a PIN's Mux status
and something relating to the signal settings like pullup/pulldown/drive strength. So
you can know if a PIN is a GPIO function PIN or not, but you can't know which GPIO port and
which GPIO line the PIN belongs to.  

> This is blatant code duplication, at the very least we need to find a way to share
> this pin library, because it is irresponsible to put the same information into the
> kernel twice. You can easily imagine what happens if one of these pin tables
> contain an error and it only gets fixed in one of these two places.
> 

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.

> > The wakeup function doesn't change any PINCtrl settings.  On i.MX8
> > platform, there is an extra pad wakeup logic and each pin can be
> > configured as the wakeup source during system suspend.
> 
> So pads, pins, fingers - all these funny names for things like that - those are
> handled by the pin control subsystem.
> 

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.

> > Because GPIO module is powered off on i.MX8 platform in suspend state,
> > the GPIO's native wakeup won't work anymore. This patch just maps a
> > GPIO wakeup to the corresponding PAD wakeup, and this mapping doesn't
> impact or change any PINCtrl settings.
> > Also the pad wakeup feature here has a great power consumption
> > advantage comparing with the GPIO module's native wakeup.
> >
> > >
> > > To me this looks like two drivers could end up fighting about the
> > > control of the same hardware if you don't.
> > >
> >
> > It won't. This patch doesn't interact with any pinctrl relating registers at all.
> 
> It interacts with the i.MX SCU and the i.MX pin control does exactly that.
> 
> What it does at the end is to call this RPC to set up the asynchronous pad ring:
> 
> ret = imx_scu_call_rpc(ipc_handle, &msg, true);
> 
> Consider:
> drivers/pinctrl/freescale/pinctrl-scu.c
> 
> Which also issues a while bunch of imx_scu_call_rpc().
> 
> What I think you should do is call into the pin control back-end and have pinctrl-
> scu.c handle this for you.
> 

Normally there are multiple information behind a resource for the scu API calls. 
Each information may determine its own functionality. Let take the 
resource IMX_SC_R_UART_0 as an example, it has Power domain and Clock functions 
associated.  
You will call imx_scu_call_rpc in the power domain driver to turn on/off its power, and call
the function imx_scu_call_rpc again in the clock driver to turn on/off its clock. However,
you don't need to synchronize the power domain driver and the clock driver, because they
are controlling two different resources. The pad wakeup function is the same concept here 
comparing with the other PINCTRL information.

> An ordinary vanilla pin controller would use the configs from
> include/linux/pinctrl/pinconf-generic.h
> but I think the i.MX pin config is custom different, so you need to figure out
> what config to pass and how.
> 
> drivers/gpio/gpio-mxc.c should however implement gpio line configuration,
> somewhere along the lines of:
> 
> port->gc.set_config = gpiochip_generic_config;
> 
> Then if you look into gpiochip_generic_config you can see that this will call into
> the pin controller back-end:
> 
> int gpiochip_generic_config(struct gpio_chip *gc, unsigned int offset,
>                             unsigned long config) {
>         return pinctrl_gpio_set_config(gc->gpiodev->base + offset, config); }
> 
> At this point there must be a mechanism in place to cross-reference between
> GPIO lines and pins, the most common way to solve this is to use gpio-ranges,
> see Documentation/devicetree/bindings/gpio/gpio.txt.
> 

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.

> This will hopefully end up in .pin_config_set()->imx_pinconf_set, in
> drivers/pinctrl/freescale/pinctrl-imx.c which looks like this:
> 
> static int imx_pinconf_set(struct pinctrl_dev *pctldev,
>                            unsigned pin_id, unsigned long *configs,
>                            unsigned num_configs) {
>         struct imx_pinctrl *ipctl = pinctrl_dev_get_drvdata(pctldev);
>         const struct imx_pinctrl_soc_info *info = ipctl->info;
> 
>         if (info->flags & IMX_USE_SCU)
>                 return info->imx_pinconf_set(pctldev, pin_id,
>                                            configs, num_configs);
>         else
>                 return imx_pinconf_set_mmio(pctldev, pin_id,
>                                             configs, num_configs); }
> 
> Here one path goes down into the SCU, and there your custom wakeup config
> will be handled.
> 
> At least this is how I imagine it, the i.MX maintainers need to say how this should
> work in their opinion, but code duplication and two unsyncronized clients
> sending random messages to the SCU surely is not an option.

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.

Regards,
Shenwei

> 
> Yours,
> Linus Walleij