| Message ID | 20230901120836.1057900-1-yann.gautier@foss.st.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: mmci: stm32: add SDIO in-band interrupt mode | expand |
Hi Yann/Christophe, thanks for your patch! On Fri, Sep 1, 2023 at 2:08 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 variant. > It allows the SD I/O card to interrupt the host on SDMMC_D1 data line. > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> (...) > +++ b/drivers/mmc/host/mmci.h > @@ -332,6 +332,7 @@ enum mmci_busy_state { > * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register > * @dma_lli: true if variant has dma link list feature. > * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. > + * @use_sdio_irq: allow SD I/O card to interrupt the host The documentation tag should be one line up (compare to the members...) > @@ -376,6 +377,7 @@ struct variant_data { > u32 start_err; > u32 opendrain; > u8 dma_lli:1; > + u8 use_sdio_irq:1; 1. bool use_sdio_irq; 2. supports_sdio_irq is more to the point don't you think? Especially since it activates these two callbacks: > + void (*enable_sdio_irq)(struct mmci_host *host, int enable); > + void (*sdio_irq)(struct mmci_host *host, u32 status); Further: all the Ux500 variants support this (bit 22) as well, so enable those too in their vendor data. All I have is out-of-band signaling with an GPIO IRQ on my Broadcom chips but I think it works (maybe Ulf has tested it in the far past). Yours, Linus Walleij
On 9/1/23 16:10, Linus Walleij wrote: > Hi Yann/Christophe, > > thanks for your patch! Hi Linus Thanks for the review, I agree with the proposed changes. I'll prepare a new version and send it next week! Best regards, Yann > > On Fri, Sep 1, 2023 at 2:08 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 variant. >> It allows the SD I/O card to interrupt the host on SDMMC_D1 data line. >> >> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> >> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> > (...) >> +++ b/drivers/mmc/host/mmci.h >> @@ -332,6 +332,7 @@ enum mmci_busy_state { >> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register >> * @dma_lli: true if variant has dma link list feature. >> * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. >> + * @use_sdio_irq: allow SD I/O card to interrupt the host > > The documentation tag should be one line up (compare to the members...) > >> @@ -376,6 +377,7 @@ struct variant_data { >> u32 start_err; >> u32 opendrain; >> u8 dma_lli:1; >> + u8 use_sdio_irq:1; > > 1. bool use_sdio_irq; > > 2. supports_sdio_irq is more to the point don't you think? > Especially since it activates these two callbacks: > >> + void (*enable_sdio_irq)(struct mmci_host *host, int enable); >> + void (*sdio_irq)(struct mmci_host *host, u32 status); > > Further: all the Ux500 variants support this (bit 22) as well, so enable those > too in their vendor data. All I have is out-of-band signaling with an GPIO IRQ > on my Broadcom chips but I think it works (maybe Ulf has tested it in the > far past). > > Yours, > Linus Walleij
Hi Yann/Christophe, just a quick note: On Fri, Sep 1, 2023 at 2:08 PM Yann Gautier <yann.gautier@foss.st.com> wrote: > +static void sdmmc_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); > +} > + > +static void sdmmc_sdio_irq(struct mmci_host *host, u32 status) > +{ > + if (status & MCI_ST_SDIOIT) { > + sdmmc_enable_sdio_irq(host, 0); > + sdio_signal_irq(host->mmc); > + } > +} You need to move these to mmci and rename them since Ux500 will use the same callbacks. > static struct mmci_host_ops sdmmc_variant_ops = { > .validate_data = sdmmc_idma_validate_data, (...) > + .enable_sdio_irq = sdmmc_enable_sdio_irq, > + .sdio_irq = sdmmc_sdio_irq, > }; What about dropping the per-variant callbacks and just inline this into mmci_enable_sdio_irq()/mmci_ack_sdio_irq() since so many variants have the same scheme? I haven't looked at the Qualcomm variant though, maybe it is completely different... Yours, Linus Walleij
On 9/2/23 18:43, Linus Walleij wrote: > Hi Yann/Christophe, > > just a quick note: > > On Fri, Sep 1, 2023 at 2:08 PM Yann Gautier <yann.gautier@foss.st.com> wrote: > >> +static void sdmmc_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); >> +} >> + >> +static void sdmmc_sdio_irq(struct mmci_host *host, u32 status) >> +{ >> + if (status & MCI_ST_SDIOIT) { >> + sdmmc_enable_sdio_irq(host, 0); >> + sdio_signal_irq(host->mmc); >> + } >> +} > > You need to move these to mmci and rename them since Ux500 will use > the same callbacks. Hi Linus, Yes, that's what I was planning to do. > >> static struct mmci_host_ops sdmmc_variant_ops = { >> .validate_data = sdmmc_idma_validate_data, > (...) >> + .enable_sdio_irq = sdmmc_enable_sdio_irq, >> + .sdio_irq = sdmmc_sdio_irq, >> }; > > What about dropping the per-variant callbacks and just inline > this into mmci_enable_sdio_irq()/mmci_ack_sdio_irq() since > so many variants have the same scheme? I haven't looked > at the Qualcomm variant though, maybe it is completely > different... I'm not sure about this. Keeping the ops will make it easier for other variants to bring their own code if their scheme is different. Best regards, Yann > > Yours, > Linus Walleij
On Fri, 1 Sept 2023 at 16:10, Linus Walleij <linus.walleij@linaro.org> wrote: > > Hi Yann/Christophe, > > thanks for your patch! > > On Fri, Sep 1, 2023 at 2:08 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 variant. > > It allows the SD I/O card to interrupt the host on SDMMC_D1 data line. > > > > Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > > Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> > (...) > > +++ b/drivers/mmc/host/mmci.h > > @@ -332,6 +332,7 @@ enum mmci_busy_state { > > * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register > > * @dma_lli: true if variant has dma link list feature. > > * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. > > + * @use_sdio_irq: allow SD I/O card to interrupt the host > > The documentation tag should be one line up (compare to the members...) > > > @@ -376,6 +377,7 @@ struct variant_data { > > u32 start_err; > > u32 opendrain; > > u8 dma_lli:1; > > + u8 use_sdio_irq:1; > > 1. bool use_sdio_irq; > > 2. supports_sdio_irq is more to the point don't you think? > Especially since it activates these two callbacks: > > > + void (*enable_sdio_irq)(struct mmci_host *host, int enable); > > + void (*sdio_irq)(struct mmci_host *host, u32 status); > > Further: all the Ux500 variants support this (bit 22) as well, so enable those > too in their vendor data. All I have is out-of-band signaling with an GPIO IRQ > on my Broadcom chips but I think it works (maybe Ulf has tested it in the > far past). For the ux500 variant there is a HW problem. After running some stress tests, we may end up being stuck waiting for an SDIO IRQ to be delivered. Even if the SDIO irqs should be considered level triggered, it looked like it was implemented in the HW as an edge triggered IRQ. The downstream workaround consisted of re-routing the DAT1 to a GPIO at runtime suspend (we wanted that for optimal power save support anyway) - and manually checking if the DAT1 line was asserted, before enabling the GPIO line for an irq. This worked perfectly fine as a workaround, with the limitation that one may observe a little bit of hick-up in the traffic occasionally. That said, the out-of-band IRQs is what works best for the ux500 variants. Kind regards Uffe
On 9/4/23 14:21, Ulf Hansson wrote: > On Fri, 1 Sept 2023 at 16:10, Linus Walleij <linus.walleij@linaro.org> wrote: >> >> Hi Yann/Christophe, >> >> thanks for your patch! >> >> On Fri, Sep 1, 2023 at 2:08 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 variant. >>> It allows the SD I/O card to interrupt the host on SDMMC_D1 data line. >>> >>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> >>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> >> (...) >>> +++ b/drivers/mmc/host/mmci.h >>> @@ -332,6 +332,7 @@ enum mmci_busy_state { >>> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register >>> * @dma_lli: true if variant has dma link list feature. >>> * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. >>> + * @use_sdio_irq: allow SD I/O card to interrupt the host >> >> The documentation tag should be one line up (compare to the members...) >> >>> @@ -376,6 +377,7 @@ struct variant_data { >>> u32 start_err; >>> u32 opendrain; >>> u8 dma_lli:1; >>> + u8 use_sdio_irq:1; >> >> 1. bool use_sdio_irq; >> Hi, Should it really be changed to a bool? Other boolean likes in the structure are u8:1. >> 2. supports_sdio_irq is more to the point don't you think? >> Especially since it activates these two callbacks: >> >>> + void (*enable_sdio_irq)(struct mmci_host *host, int enable); >>> + void (*sdio_irq)(struct mmci_host *host, u32 status); >> >> Further: all the Ux500 variants support this (bit 22) as well, so enable those >> too in their vendor data. All I have is out-of-band signaling with an GPIO IRQ >> on my Broadcom chips but I think it works (maybe Ulf has tested it in the >> far past). > > For the ux500 variant there is a HW problem. After running some stress > tests, we may end up being stuck waiting for an SDIO IRQ to be > delivered. Even if the SDIO irqs should be considered level triggered, > it looked like it was implemented in the HW as an edge triggered IRQ. > > The downstream workaround consisted of re-routing the DAT1 to a GPIO > at runtime suspend (we wanted that for optimal power save support > anyway) - and manually checking if the DAT1 line was asserted, before > enabling the GPIO line for an irq. This worked perfectly fine as a > workaround, with the limitation that one may observe a little bit of > hick-up in the traffic occasionally. > > That said, the out-of-band IRQs is what works best for the ux500 variants. What I understand here is that in-band interrupts are not properly working on ux500, and then the feature shouldn't be enabled for this platform. Am I correct? If this is the case, the v2 will consist in changing the use_sdio_irq to use_sdio_irq, and update the comment of the struct. And depending on the answer, maybe change the field to bool. Best regards, Yann > > Kind regards > Uffe
On Thu, Sep 14, 2023 at 11:08 AM Yann Gautier <yann.gautier@foss.st.com> wrote: > On 9/4/23 14:21, Ulf Hansson wrote: > > On Fri, 1 Sept 2023 at 16:10, Linus Walleij <linus.walleij@linaro.org> wrote: > >> > >> Hi Yann/Christophe, > >> > >> thanks for your patch! > >> > >> On Fri, Sep 1, 2023 at 2:08 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 variant. > >>> It allows the SD I/O card to interrupt the host on SDMMC_D1 data line. > >>> > >>> Signed-off-by: Christophe Kerello <christophe.kerello@foss.st.com> > >>> Signed-off-by: Yann Gautier <yann.gautier@foss.st.com> > >> (...) > >>> +++ b/drivers/mmc/host/mmci.h > >>> @@ -332,6 +332,7 @@ enum mmci_busy_state { > >>> * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register > >>> * @dma_lli: true if variant has dma link list feature. > >>> * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. > >>> + * @use_sdio_irq: allow SD I/O card to interrupt the host > >> > >> The documentation tag should be one line up (compare to the members...) > >> > >>> @@ -376,6 +377,7 @@ struct variant_data { > >>> u32 start_err; > >>> u32 opendrain; > >>> u8 dma_lli:1; > >>> + u8 use_sdio_irq:1; > >> > >> 1. bool use_sdio_irq; > >> > Hi, > > Should it really be changed to a bool? > Other boolean likes in the structure are u8:1. Yes, two wrongs does not make one right. Using u8:1 is a way of trying to outsmart the compiler which is generally a bad idea. > > That said, the out-of-band IRQs is what works best for the ux500 variants. > > What I understand here is that in-band interrupts are not properly > working on ux500, and then the feature shouldn't be enabled for this > platform. > Am I correct? I think we can flag the feature as available and implement the handling but add a comment that this is unstable and that Ux500 users should prefer to use out-of-band IRQs. Yours, Linus Walleij
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c index dda756a563793..c4d3ffc5340f7 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), + .use_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), + .use_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), + .use_sdio_irq = true, .dma_lli = true, .busy_timeout = true, .busy_detect = true, @@ -423,6 +426,10 @@ 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->use_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); @@ -1805,6 +1812,11 @@ static irqreturn_t mmci_irq(int irq, void *dev_id) mmci_data_irq(host, host->data, status); } + if (host->variant->use_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 +2053,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->use_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->use_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 +2100,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 +2369,14 @@ static int mmci_probe(struct amba_device *dev, mmc->caps |= MMC_CAP_WAIT_WHILE_BUSY; } + if (variant->use_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..554473c01b8af 100644 --- a/drivers/mmc/host/mmci.h +++ b/drivers/mmc/host/mmci.h @@ -332,6 +332,7 @@ enum mmci_busy_state { * @opendrain: bitmask identifying the OPENDRAIN bit inside MMCIPOWER register * @dma_lli: true if variant has dma link list feature. * @stm32_idmabsize_mask: stm32 sdmmc idma buffer size. + * @use_sdio_irq: allow SD I/O card to interrupt the host */ struct variant_data { unsigned int clkreg; @@ -376,6 +377,7 @@ struct variant_data { u32 start_err; u32 opendrain; u8 dma_lli:1; + u8 use_sdio_irq:1; 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 { diff --git a/drivers/mmc/host/mmci_stm32_sdmmc.c b/drivers/mmc/host/mmci_stm32_sdmmc.c index 35067e1e6cd80..d66871f1a57ed 100644 --- a/drivers/mmc/host/mmci_stm32_sdmmc.c +++ b/drivers/mmc/host/mmci_stm32_sdmmc.c @@ -668,6 +668,25 @@ static int sdmmc_post_sig_volt_switch(struct mmci_host *host, return ret; } +static void sdmmc_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); +} + +static void sdmmc_sdio_irq(struct mmci_host *host, u32 status) +{ + if (status & MCI_ST_SDIOIT) { + sdmmc_enable_sdio_irq(host, 0); + sdio_signal_irq(host->mmc); + } +} + static struct mmci_host_ops sdmmc_variant_ops = { .validate_data = sdmmc_idma_validate_data, .prep_data = sdmmc_idma_prep_data, @@ -681,6 +700,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 = sdmmc_enable_sdio_irq, + .sdio_irq = sdmmc_sdio_irq, }; static struct sdmmc_tuning_ops dlyb_tuning_mp15_ops = {