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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3747f246650622ef65787159af5271a79401a855.camel@mediatek.com>
Date:   Mon, 25 Jul 2022 17:13:22 +0800
From:   Axe Yang <axe.yang@...iatek.com>
To:     Linus Walleij <linus.walleij@...aro.org>,
        Ulf Hansson <ulf.hansson@...aro.org>
CC:     Rob Herring <robh+dt@...nel.org>,
        Chaotian Jing <chaotian.jing@...iatek.com>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Adrian Hunter <adrian.hunter@...el.com>,
        Yoshihiro Shimoda <yoshihiro.shimoda.uh@...esas.com>,
        Satya Tangirala <satyat@...gle.com>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Wolfram Sang <wsa+renesas@...g-engineering.com>,
        Lucas Stach <dev@...xeye.de>,
        "Eric Biggers" <ebiggers@...gle.com>,
        Andrew Jeffery <andrew@...id.au>,
        "Stephen Boyd" <swboyd@...omium.org>,
        Kiwoong Kim <kwmad.kim@...sung.com>,
        Yue Hu <huyue2@...ong.com>, Tian Tao <tiantao6@...ilicon.com>,
        <angelogioacchino.delregno@...labora.com>,
        <linux-mmc@...r.kernel.org>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-mediatek@...ts.infradead.org>,
        <Project_Global_Chrome_Upstream_Group@...iatek.com>,
        Yong Mao <yong.mao@...iatek.com>
Subject: Re: [PATCH v13 3/3] mmc: mediatek: add support for SDIO eint wakup
 IRQ

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@...iatek.com>
> 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@...aro.org>
> 
> Yours,
> Linus Walleij

Regards,
Axe