| Message ID | 20230914150904.155630-1-yann.gautier@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] mmc: mmci: stm32: add SDIO in-band interrupt mode | expand |
On Thu, Sep 14, 2023 at 5:09 PM Yann Gautier <yann.gautier@foss.st.com> wrote: > From: Christophe Kerello <christophe.kerello@foss.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@foss.st.com> > Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> v2 looks good to me, Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, 14 Sept 2023 at 17:09, Yann Gautier <yann.gautier@foss.st.com> wrote: > > From: Christophe Kerello <christophe.kerello@foss.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@foss.st.com> > Signed-off-by: Yann Gautier <yann.gautier@foss.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
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; + 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; } 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 && + 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; + + 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; + + 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); + } + /* 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 = {