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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CACRpkdZ5G2fMCqvkXANVEmZjNcF4U4mSDzZk6aXbqFjYVN3hcA@mail.gmail.com>
Date:   Fri, 22 Jul 2022 13:21:43 +0200
From:   Linus Walleij <linus.walleij@...aro.org>
To:     Axe Yang <axe.yang@...iatek.com>
Cc:     Ulf Hansson <ulf.hansson@...aro.org>,
        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

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.

> +       /* 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?

With that:
Reviewed-by: Linus Walleij <linus.walleij@...aro.org>

Yours,
Linus Walleij