MIME-Version: 1.0
References: <20190410221237.160856-1-dianders@chromium.org>
 <CAD=FV=Wm+T+W=LWTSiwBT3UQWG7YKQdGuL2+BmAFQhk8CYY1mQ@mail.gmail.com> <6e5c9395-51b0-7f28-ec91-de95cb54f58d@rock-chips.com>
In-Reply-To: <6e5c9395-51b0-7f28-ec91-de95cb54f58d@rock-chips.com>
From:   Doug Anderson <dianders@chromium.org>
Date:   Tue, 23 Apr 2019 18:09:19 -0700
Message-ID: <CAD=FV=XuCrymazJDKBYqLezkYkikzGNiv6X0FdzNG0SaR8PuRw@mail.gmail.com>
Subject: Re: [PATCH] mmc: dw_mmc: Disable SDIO interrupts while suspended to
 fix suspend/resume
To:     Shawn Lin <shawn.lin@rock-chips.com>
Cc:     Jaehoon Chung <jh80.chung@samsung.com>,
        Ulf Hansson <ulf.hansson@linaro.org>,
        Kalle Valo <kvalo@codeaurora.org>,
        =?UTF-8?Q?Heiko_St=C3=BCbner?= <heiko@sntech.de>,
        "open list:ARM/Rockchip SoC..." <linux-rockchip@lists.infradead.org>,
        Brian Norris <briannorris@chromium.org>,
        linux-wireless <linux-wireless@vger.kernel.org>,
        Matthias Kaehlcke <mka@chromium.org>,
        Ryan Case <ryandcase@chromium.org>, stable@vger.kernel.org,
        Linux MMC List <linux-mmc@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-wireless-owner@vger.kernel.org
Precedence: bulk

Hi,

On Tue, Apr 23, 2019 at 5:57 PM Shawn Lin <shawn.lin@rock-chips.com> wrote:
>
> The intention seems reasonable to me, but just wonder if we need
> mask/unmask SDIO interrupt when it's never used?

I don't think we do.

Specifically "client_sdio_enb" starts out as false.  If nobody ever
calls dw_mci_enable_sdio_irq() then "client_sdio_enb" will always
continue to be false.

Now at suspend time we'll call "__dw_mci_enable_sdio_irq".  Because
"client_sdio_enb" is false then the local variable "enb" will always
be false.  Sure we'll clear the "SDMMC_INT_SDIO" from the "INTMASK"
register, but it should already have been cleared so this is a no-op.

...at resume time we'll have a similar situation where
"client_sdio_enb" is false and thus we'll (again) just clear the
"SDMMC_INT_SDIO" from the "INTMASK".

I could potentially optimize away the "mci_writel()" if we're not
changing anything if you're worried about that?


> It's the same
> situation for SDMMC_CLKEN_LOW_PWR that we couldn't stop providing
> clock for SDIO cards, so I guess we need to check MMC_CAP_SDIO_IRQ
> as well.

I think it might be a slightly different situation though.  In this
case I believe it's not just a problem with clock stoppage.  I believe
the problem is that the interrupt will be passed to the SDIO device
driver right away and that'll call back into dw_mmc.  dw_mmc is just
not in a state to handle it until we've more fully resumed things.

In any case with my patch the only way we'd ever end up unmasking the
SDIO IRQ here would be if dw_mci_enable_sdio_irq() was called.  That
will only happen if there's an SDIO card plugged in.


-Doug