| 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
| ||
|
Message-ID: <CAPDyKFrBUcqwuwVJsVi1JrG+0S=vWJ7D-BjQhjg-AU0GsbKO1Q@mail.gmail.com>
Date: Tue, 26 Sep 2023 12:29:45 +0200
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Yann Gautier <yann.gautier@...s.st.com>
Cc: Russell King <linux@...linux.org.uk>,
Linus Walleij <linus.walleij@...aro.org>,
Alexandre Torgue <alexandre.torgue@...s.st.com>,
Maxime Coquelin <mcoquelin.stm32@...il.com>,
Christophe Kerello <christophe.kerello@...s.st.com>,
Yang Yingliang <yangyingliang@...wei.com>,
Rob Herring <robh@...nel.org>, linux-mmc@...r.kernel.org,
linux-kernel@...r.kernel.org,
linux-stm32@...md-mailman.stormreply.com,
linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH v2] mmc: mmci: stm32: add SDIO in-band interrupt mode
On Thu, 14 Sept 2023 at 17:09, Yann Gautier <yann.gautier@...s.st.com> wrote:
>
> From: Christophe Kerello <christophe.kerello@...s.st.com>
>
> Add the support of SDIO in-band interrupt mode for STM32 and Ux500
> variants.
> It allows the SD I/O card to interrupt the host on SDMMC_D1 data line.
> It is not enabled by default on Ux500 variant as this is unstable and
> Ux500 users should use out-of-band IRQs.
>
> Signed-off-by: Christophe Kerello <christophe.kerello@...s.st.com>
> Signed-off-by: Yann Gautier <yann.gautier@...s.st.com>
> ---
> Updates on v2:
> * rename use_sdio_irq to supports_sdio_irq and change it to bool
> * use common code for ux500 and stm32 variants
>
> ---
> drivers/mmc/host/mmci.c | 85 +++++++++++++++++++++++++++++
> drivers/mmc/host/mmci.h | 7 +++
> drivers/mmc/host/mmci_stm32_sdmmc.c | 2 +
> 3 files changed, 94 insertions(+)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index dda756a563793..65cc03ee7f23b 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -272,6 +272,7 @@ static struct variant_data variant_stm32_sdmmc = {
> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
> .stm32_idmabsize_mask = GENMASK(12, 5),
> .stm32_idmabsize_align = BIT(5),
> + .supports_sdio_irq = true,
> .busy_timeout = true,
> .busy_detect = true,
> .busy_detect_flag = MCI_STM32_BUSYD0,
> @@ -299,6 +300,7 @@ static struct variant_data variant_stm32_sdmmcv2 = {
> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
> .stm32_idmabsize_mask = GENMASK(16, 5),
> .stm32_idmabsize_align = BIT(5),
> + .supports_sdio_irq = true,
> .dma_lli = true,
> .busy_timeout = true,
> .busy_detect = true,
> @@ -327,6 +329,7 @@ static struct variant_data variant_stm32_sdmmcv3 = {
> .datactrl_mask_sdio = MCI_DPSM_ST_SDIOEN,
> .stm32_idmabsize_mask = GENMASK(16, 6),
> .stm32_idmabsize_align = BIT(6),
> + .supports_sdio_irq = true,
> .dma_lli = true,
> .busy_timeout = true,
> .busy_detect = true,
> @@ -423,6 +426,11 @@ static void mmci_write_datactrlreg(struct mmci_host *host, u32 datactrl)
> /* Keep busy mode in DPSM if enabled */
> datactrl |= host->datactrl_reg & host->variant->busy_dpsm_flag;
>
> + /* Keep SD I/O interrupt mode enabled */
> + if (host->variant->supports_sdio_irq &&
> + host->mmc->caps & MMC_CAP_SDIO_IRQ)
> + datactrl |= host->variant->datactrl_mask_sdio;
> +
This doesn't look entirely correct to me, as it will make the
->datactrl_mask_sdio bit to be set even when it shouldn't. If I
understand correctly, we really want the bit to be set if the SDIO
irqs has been enabled, but otherwise leave it for mmci_start_data() to
manage it, right?
That said, perhaps the comment a few lines above, deserves some
clarification too. Would rephrasing it into "Keep the SDIO mode bit if
SDIO irqs are enabled" make it more clear?
>From an implementation point of view, an idea is to add a
"host->datactrl_reg_add" variable (we have this for the clk and pwr
registers already). mmci_write_datactrlreg() should then OR these bits
when writing to the register. In this way, mmci_enable_sdio_irq() can
update the host->datactrl_reg_add with ->datactrl_mask_sdio, when
needed. This should also work for the ->busy_dpsm_flag a few lines
above, I think.
> if (host->datactrl_reg != datactrl) {
> host->datactrl_reg = datactrl;
> writel(datactrl, host->base + MMCIDATACTRL);
> @@ -817,6 +825,25 @@ static bool ux500_busy_complete(struct mmci_host *host, struct mmc_command *cmd,
> return (host->busy_state == MMCI_BUSY_DONE);
> }
>
> +void ux500_and_stm32_enable_sdio_irq(struct mmci_host *host, int enable)
> +{
> + void __iomem *base = host->base;
> + u32 mask = readl_relaxed(base + MMCIMASK0);
> +
> + if (enable)
> + writel_relaxed(mask | MCI_ST_SDIOITMASK, base + MMCIMASK0);
> + else
> + writel_relaxed(mask & ~MCI_ST_SDIOITMASK, base + MMCIMASK0);
> +}
> +
> +void ux500_and_stm32_sdio_irq(struct mmci_host *host, u32 status)
> +{
> + if (status & MCI_ST_SDIOIT) {
> + ux500_and_stm32_enable_sdio_irq(host, 0);
> + sdio_signal_irq(host->mmc);
> + }
> +}
> +
> /*
> * All the DMA operation mode stuff goes inside this ifdef.
> * This assumes that you have a generic DMA device interface,
> @@ -1191,6 +1218,8 @@ static void ux500_variant_init(struct mmci_host *host)
> {
> host->ops = &mmci_variant_ops;
> host->ops->busy_complete = ux500_busy_complete;
> + host->ops->enable_sdio_irq = ux500_and_stm32_enable_sdio_irq;
> + host->ops->sdio_irq = ux500_and_stm32_sdio_irq;
> }
>
> static void ux500v2_variant_init(struct mmci_host *host)
> @@ -1198,6 +1227,8 @@ static void ux500v2_variant_init(struct mmci_host *host)
> host->ops = &mmci_variant_ops;
> host->ops->busy_complete = ux500_busy_complete;
> host->ops->get_datactrl_cfg = ux500v2_get_dctrl_cfg;
> + host->ops->enable_sdio_irq = ux500_and_stm32_enable_sdio_irq;
> + host->ops->sdio_irq = ux500_and_stm32_sdio_irq;
> }
It looks to me that the extra layer of mmci variant callbacks is a bit
"heavy" at this point. ux500 and the st variants seem to work very
similarly in this regard. So maybe just the mmci_ops callbacks
directly and stick to the mmci* prefix of the function names. At least
until we see a better reason to have the extra layer of callbacks.
Of course, this also means that we need to assign
mmci_ops->enable_sdio_irq|ack_sdio_irq() conditionally during probe,
based upon the variant->supports_sdio_irq bit.
>
> static void mmci_pre_request(struct mmc_host *mmc, struct mmc_request *mrq)
> @@ -1805,6 +1836,11 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> mmci_data_irq(host, host->data, status);
> }
>
> + if (host->variant->supports_sdio_irq &&
> + host->mmc->caps & MMC_CAP_SDIO_IRQ &&
Checking the caps seems superfluous. The SDIO irqs must not be
enabled, unless MMC_CAP_SDIO_IRQ is supported, right?
> + host->ops && host->ops->sdio_irq)
> + host->ops->sdio_irq(host, status);
> +
> /*
> * Busy detection has been handled by mmci_cmd_irq() above.
> * Clear the status bit to prevent polling in IRQ context.
> @@ -2041,6 +2077,45 @@ static int mmci_sig_volt_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> return ret;
> }
>
> +static void mmci_enable_sdio_irq(struct mmc_host *mmc, int enable)
> +{
> + struct mmci_host *host = mmc_priv(mmc);
> + unsigned long flags;
> +
> + if (!host->variant->supports_sdio_irq)
> + return;
According to the earlier comment above about the extra layers of
callbacks, this can then be checked during probe instead and dropped
from here.
> +
> + if (host->ops && host->ops->enable_sdio_irq) {
> + if (enable)
> + /* Keep device active while SDIO IRQ is enabled */
> + pm_runtime_get_sync(mmc_dev(mmc));
> +
> + spin_lock_irqsave(&host->lock, flags);
> + host->ops->enable_sdio_irq(host, enable);
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + if (!enable) {
> + pm_runtime_mark_last_busy(mmc_dev(mmc));
> + pm_runtime_put_autosuspend(mmc_dev(mmc));
> + }
> + }
> +}
> +
> +static void mmci_ack_sdio_irq(struct mmc_host *mmc)
> +{
> + struct mmci_host *host = mmc_priv(mmc);
> + unsigned long flags;
> +
> + if (!host->variant->supports_sdio_irq)
> + return;
Ditto.
> +
> + if (host->ops && host->ops->enable_sdio_irq) {
> + spin_lock_irqsave(&host->lock, flags);
> + host->ops->enable_sdio_irq(host, 1);
> + spin_unlock_irqrestore(&host->lock, flags);
> + }
> +}
> +
> static struct mmc_host_ops mmci_ops = {
> .request = mmci_request,
> .pre_req = mmci_pre_request,
> @@ -2049,6 +2124,8 @@ static struct mmc_host_ops mmci_ops = {
> .get_ro = mmc_gpio_get_ro,
> .get_cd = mmci_get_cd,
> .start_signal_voltage_switch = mmci_sig_volt_switch,
> + .enable_sdio_irq = mmci_enable_sdio_irq,
> + .ack_sdio_irq = mmci_ack_sdio_irq,
> };
>
> static void mmci_probe_level_translator(struct mmc_host *mmc)
> @@ -2316,6 +2393,14 @@ static int mmci_probe(struct amba_device *dev,
> mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY;
> }
>
> + if (variant->supports_sdio_irq && host->mmc->caps & MMC_CAP_SDIO_IRQ) {
> + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
> +
> + if (variant->datactrl_mask_sdio)
> + mmci_write_datactrlreg(host,
> + host->variant->datactrl_mask_sdio);
As I stated earlier, it looks to me that this should be managed when
enabling/disabling the SDIO irqs and not during probe. No?
> + }
> +
> /* Variants with mandatory busy timeout in HW needs R1B responses. */
> if (variant->busy_timeout)
> mmc->caps |= MMC_CAP_NEED_RSP_BUSY;
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 253197f132fca..5ea4975c18ec5 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -331,6 +331,7 @@ enum mmci_busy_state {
> * register.
> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register
> * @dma_lli: true if variant has dma link list feature.
> + * @supports_sdio_irq: allow SD I/O card to interrupt the host
> * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size.
> */
> struct variant_data {
> @@ -376,6 +377,7 @@ struct variant_data {
> u32 start_err;
> u32 opendrain;
> u8 dma_lli:1;
> + bool supports_sdio_irq;
> u32 stm32_idmabsize_mask;
> u32 stm32_idmabsize_align;
> void (*init)(struct mmci_host *host);
> @@ -400,6 +402,8 @@ struct mmci_host_ops {
> bool (*busy_complete)(struct mmci_host *host, struct mmc_command *cmd, u32 status, u32 err_msk);
> void (*pre_sig_volt_switch)(struct mmci_host *host);
> int (*post_sig_volt_switch)(struct mmci_host *host, struct mmc_ios *ios);
> + void (*enable_sdio_irq)(struct mmci_host *host, int enable);
> + void (*sdio_irq)(struct mmci_host *host, u32 status);
> };
>
> struct mmci_host {
> @@ -481,6 +485,9 @@ void mmci_dmae_finalize(struct mmci_host *host, struct mmc_data *data);
> void mmci_dmae_error(struct mmci_host *host);
> #endif
>
> +void ux500_and_stm32_enable_sdio_irq(struct mmci_host *host, int enable);
> +void ux500_and_stm32_sdio_irq(struct mmci_host *host, u32 status);
> +
> #ifdef CONFIG_MMC_QCOM_DML
> void qcom_variant_init(struct mmci_host *host);
> #else
> diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c
> index 35067e1e6cd80..fbfaa0bcec51e 100644
> --- a/drivers/mmc/host/mmci_stm32_sdmmc.c
> +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c
> @@ -681,6 +681,8 @@ static struct mmci_host_ops sdmmc_variant_ops = {
> .busy_complete = sdmmc_busy_complete,
> .pre_sig_volt_switch = sdmmc_pre_sig_volt_vswitch,
> .post_sig_volt_switch = sdmmc_post_sig_volt_switch,
> + .enable_sdio_irq = ux500_and_stm32_enable_sdio_irq,
> + .sdio_irq = ux500_and_stm32_sdio_irq,
> };
>
> static struct sdmmc_tuning_ops dlyb_tuning_mp15_ops = {
> --
> 2.34.1
>
Kind regards
Uffe
Powered by blists - more mailing lists