Re: [PATCH v13 3/3] mmc: mediatek: add support for SDIO eint wakup IRQ

[Axe Yang]

Hi Linus,

Thanks for the comments, but Ulf and I have some question on pinctrl.

On Fri, 2022-07-22 at 13:21 +0200, Linus Walleij wrote:
> On Thu, Jun 23, 2022 at 11:10 AM Axe Yang <axe.yang@xxxxxxxxxxxx>
> wrote:
> 
> > MSDC driver will time-share the SDIO DAT1 pin. During suspend, MSDC
> > turn off clock and switch SDIO DAT1 pin to GPIO mode. And during
> > resume, switch GPIO function back to DAT1 mode then turn on clock.
> 
> So what I see is that you have what the electronics chip people call
> an armed pad ring, such that an asynchronous event detector is
> connected
> to the pin when it is in GPIO mode, and in this mode the system
> can enter a lower sleep state (like powering off the MMC block
> silicon...)
> 
> That seems fine, this is definitely how the chip people expected
> it to work.
> 
> > +       if (mmc_card_enable_async_irq(mmc->card) && host-
> > >pins_eint) {
> > +               if (enb) {
> > +                       /*
> > +                        * In
> > dev_pm_set_dedicated_wake_irq_reverse(), eint pin will be set to
> > +                        * GPIO mode. We need to restore it to SDIO
> > DAT1 mode after that.
> > +                        * Since the current pinstate is pins_uhs,
> > to ensure pinctrl select take
> > +                        * affect successfully, we change the
> > pinstate to pins_eint firstly.
> > +                        */
> > +                       pinctrl_select_state(host->pinctrl, host-
> > >pins_eint);
> > +                       ret =
> > dev_pm_set_dedicated_wake_irq_reverse(host->dev, host->eint_irq);
> > +
> > +                       if (ret) {
> > +                               dev_err(host->dev, "Failed to
> > register SDIO wakeup irq!\n");
> > +                               host->pins_eint = NULL;
> > +                               pm_runtime_get_noresume(host->dev);
> > +                       } else {
> > +                               dev_dbg(host->dev, "SDIO eint irq:
> > %d!\n", host->eint_irq);
> > +                       }
> > +
> > +                       pinctrl_select_state(host->pinctrl, host-
> > >pins_uhs);
> > +               } else {
> > +                       dev_pm_clear_wake_irq(host->dev);
> > +               }
> > +       } else {
> > +               if (enb) {
> > +                       /* Ensure host->pins_eint is NULL */
> > +                       host->pins_eint = NULL;
> > +                       pm_runtime_get_noresume(host->dev);
> > +               } else {
> > +                       pm_runtime_put_noidle(host->dev);
> > +               }
> > +       }
> 
> This looks right.

SDIO DAT1 pin mode is changed to GPIO mode in
dev_pm_set_dedicated_wake_irq_reverse():

https://elixir.bootlin.com/linux/latest/source/drivers/pinctrl/mediatek/pinctrl-mtk-common-v2.c#L339

dev_pm_set_dedicated_wake_irq_reverse() -> ... ->request_threaded_irq()
-> __setup_irq() -> irq_request_resources() ->
mtk_eint_irq_request_resources()-> mtk_xt_set_gpio_as_eint()


To restore SDIO DAT1 pin to uhs mode. I have to call
pinctrl_select_state() twice(change pinctrl to another state, then
change back to uhs mode). Ulf worried we might be doing something at
the mmc driver level, which should really be managed at the pinctrl
layer.

Do you have any comment or suggestion on this?


> 
> > +       /* Support for SDIO eint irq ? */
> > +       if ((mmc->pm_caps & MMC_PM_WAKE_SDIO_IRQ) && (mmc->pm_caps
> > & MMC_PM_KEEP_POWER)) {
> > +               host->eint_irq = platform_get_irq_byname(pdev,
> > "sdio_wakeup");
> > +               if (host->eint_irq > 0) {
> > +                       host->pins_eint =
> > pinctrl_lookup_state(host->pinctrl, "state_eint");
> 
> I guess one of the other patches adds this to the DT binding?

Yes. The property related change is in patch 1/3

> 
> With that:
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> 
> Yours,
> Linus Walleij

Regards,
Axe