| Message ID | a185b4e9-f238-c2e6-0847-79cd8265844a@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mmc: meson-gx: add SDIO interrupt support | expand |
On Fri, 19 Aug 2022 at 23:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > Add SDIO interrupt support. Successfully tested on a S905X4-based > system (V3 register layout) with a BRCM4334 SDIO wifi module > (brcmfmac driver). The implementation also considers the potential > race discussed in [0]. > > [0] https://lore.kernel.org/linux-arm-kernel/CAPDyKFoJDhjLkajBHgW3zHasvYYri77NQoDpiW-BpKgkdjtOyg@mail.gmail.com/ > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 76 ++++++++++++++++++++++++++++----- > 1 file changed, 66 insertions(+), 10 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 9a4da2544..58b7836a5 100644 > --- a/drivers/mmc/host/meson-gx-mmc.c > +++ b/drivers/mmc/host/meson-gx-mmc.c > @@ -41,14 +41,17 @@ > #define CLK_V2_TX_DELAY_MASK GENMASK(19, 16) > #define CLK_V2_RX_DELAY_MASK GENMASK(23, 20) > #define CLK_V2_ALWAYS_ON BIT(24) > +#define CLK_V2_IRQ_SDIO_SLEEP BIT(25) > > #define CLK_V3_TX_DELAY_MASK GENMASK(21, 16) > #define CLK_V3_RX_DELAY_MASK GENMASK(27, 22) > #define CLK_V3_ALWAYS_ON BIT(28) > +#define CLK_V3_IRQ_SDIO_SLEEP BIT(29) > > #define CLK_TX_DELAY_MASK(h) (h->data->tx_delay_mask) > #define CLK_RX_DELAY_MASK(h) (h->data->rx_delay_mask) > #define CLK_ALWAYS_ON(h) (h->data->always_on) > +#define CLK_IRQ_SDIO_SLEEP(h) (h->data->irq_sdio_sleep) > > #define SD_EMMC_DELAY 0x4 > #define SD_EMMC_ADJUST 0x8 > @@ -135,6 +138,7 @@ struct meson_mmc_data { > unsigned int rx_delay_mask; > unsigned int always_on; > unsigned int adjust; > + unsigned int irq_sdio_sleep; > }; > > struct sd_emmc_desc { > @@ -174,6 +178,7 @@ struct meson_host { > bool vqmmc_enabled; > bool needs_pre_post_req; > > + spinlock_t lock; > }; > > #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > @@ -430,6 +435,7 @@ static int meson_mmc_clk_init(struct meson_host *host) > clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180); > clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0); > clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); > + clk_reg |= CLK_IRQ_SDIO_SLEEP(host); > writel(clk_reg, host->regs + SD_EMMC_CLOCK); > > /* get the mux parents */ > @@ -934,32 +940,54 @@ static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) > } > } > > +static void __meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > +{ > + struct meson_host *host = mmc_priv(mmc); > + u32 reg_irqen = IRQ_EN_MASK; > + > + if (enable) > + reg_irqen |= IRQ_SDIO; > + writel(reg_irqen, host->regs + SD_EMMC_IRQ_EN); > +} > + > static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > { > struct meson_host *host = dev_id; > struct mmc_command *cmd; > - struct mmc_data *data; > u32 status, raw_status; > irqreturn_t ret = IRQ_NONE; > > raw_status = readl(host->regs + SD_EMMC_STATUS); > - status = raw_status & IRQ_EN_MASK; > + status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); > > if (!status) { > dev_dbg(host->dev, > "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", > - IRQ_EN_MASK, raw_status); > + IRQ_EN_MASK | IRQ_SDIO, raw_status); > return IRQ_NONE; > } > > - if (WARN_ON(!host) || WARN_ON(!host->cmd)) > + if (WARN_ON(!host)) > return IRQ_NONE; > > /* ack all raised interrupts */ > writel(status, host->regs + SD_EMMC_STATUS); > > cmd = host->cmd; > - data = cmd->data; > + > + if (status & IRQ_SDIO) { > + spin_lock(&host->lock); > + __meson_mmc_enable_sdio_irq(host->mmc, 0); > + sdio_signal_irq(host->mmc); > + spin_unlock(&host->lock); > + status &= ~IRQ_SDIO; > + if (!status) > + return IRQ_HANDLED; > + } > + > + if (WARN_ON(!cmd)) > + return IRQ_NONE; > + > cmd->error = 0; > if (status & IRQ_CRC_ERR) { > dev_dbg(host->dev, "CRC Error - status 0x%08x\n", status); > @@ -977,12 +1005,9 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > > meson_mmc_read_resp(host->mmc, cmd); > > - if (status & IRQ_SDIO) { > - dev_dbg(host->dev, "IRQ: SDIO TODO.\n"); > - ret = IRQ_HANDLED; > - } > - > if (status & (IRQ_END_OF_CHAIN | IRQ_RESP_STATUS)) { > + struct mmc_data *data = cmd->data; > + > if (data && !cmd->error) > data->bytes_xfered = data->blksz * data->blocks; > if (meson_mmc_bounce_buf_read(data) || > @@ -1125,6 +1150,27 @@ static int meson_mmc_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) > return -EINVAL; > } > > +static void meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > +{ > + struct meson_host *host = mmc_priv(mmc); > + unsigned long flags; > + > + spin_lock_irqsave(&host->lock, flags); > + __meson_mmc_enable_sdio_irq(mmc, enable); > + spin_unlock_irqrestore(&host->lock, flags); > +} > + > +static void meson_mmc_ack_sdio_irq(struct mmc_host *mmc) > +{ > + struct meson_host *host = mmc_priv(mmc); > + unsigned long flags; > + > + spin_lock_irqsave(&host->lock, flags); > + if (!mmc->sdio_irq_pending) I am not quite sure I understand why you need to check this flag here. The point is, in meson_mmc_irq() you are doing things in the correct order, which means disabling the SDIO irq by calling __meson_mmc_enable_sdio_irq(host->mmc, 0) prior calling sdio_signal_irq(host->mmc) (which sets the flag). In this way, the host driver should be prevented from signaling another new SDIO irq, until it has acked (which means re-enabling the SDIO irqs) the current one to be processed. Or did I miss something? > + __meson_mmc_enable_sdio_irq(mmc, 1); > + spin_unlock_irqrestore(&host->lock, flags); > +} > [...] Other than the comment above, the patch looks good to me! Kind regards Uffe
On 22.08.2022 13:33, Ulf Hansson wrote: > On Fri, 19 Aug 2022 at 23:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> Add SDIO interrupt support. Successfully tested on a S905X4-based >> system (V3 register layout) with a BRCM4334 SDIO wifi module >> (brcmfmac driver). The implementation also considers the potential >> race discussed in [0]. >> >> [0] https://lore.kernel.org/linux-arm-kernel/CAPDyKFoJDhjLkajBHgW3zHasvYYri77NQoDpiW-BpKgkdjtOyg@mail.gmail.com/ >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/mmc/host/meson-gx-mmc.c | 76 ++++++++++++++++++++++++++++----- >> 1 file changed, 66 insertions(+), 10 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index 9a4da2544..58b7836a5 100644 >> --- a/drivers/mmc/host/meson-gx-mmc.c >> +++ b/drivers/mmc/host/meson-gx-mmc.c >> @@ -41,14 +41,17 @@ >> #define CLK_V2_TX_DELAY_MASK GENMASK(19, 16) >> #define CLK_V2_RX_DELAY_MASK GENMASK(23, 20) >> #define CLK_V2_ALWAYS_ON BIT(24) >> +#define CLK_V2_IRQ_SDIO_SLEEP BIT(25) >> >> #define CLK_V3_TX_DELAY_MASK GENMASK(21, 16) >> #define CLK_V3_RX_DELAY_MASK GENMASK(27, 22) >> #define CLK_V3_ALWAYS_ON BIT(28) >> +#define CLK_V3_IRQ_SDIO_SLEEP BIT(29) >> >> #define CLK_TX_DELAY_MASK(h) (h->data->tx_delay_mask) >> #define CLK_RX_DELAY_MASK(h) (h->data->rx_delay_mask) >> #define CLK_ALWAYS_ON(h) (h->data->always_on) >> +#define CLK_IRQ_SDIO_SLEEP(h) (h->data->irq_sdio_sleep) >> >> #define SD_EMMC_DELAY 0x4 >> #define SD_EMMC_ADJUST 0x8 >> @@ -135,6 +138,7 @@ struct meson_mmc_data { >> unsigned int rx_delay_mask; >> unsigned int always_on; >> unsigned int adjust; >> + unsigned int irq_sdio_sleep; >> }; >> >> struct sd_emmc_desc { >> @@ -174,6 +178,7 @@ struct meson_host { >> bool vqmmc_enabled; >> bool needs_pre_post_req; >> >> + spinlock_t lock; >> }; >> >> #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) >> @@ -430,6 +435,7 @@ static int meson_mmc_clk_init(struct meson_host *host) >> clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180); >> clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0); >> clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); >> + clk_reg |= CLK_IRQ_SDIO_SLEEP(host); >> writel(clk_reg, host->regs + SD_EMMC_CLOCK); >> >> /* get the mux parents */ >> @@ -934,32 +940,54 @@ static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) >> } >> } >> >> +static void __meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) >> +{ >> + struct meson_host *host = mmc_priv(mmc); >> + u32 reg_irqen = IRQ_EN_MASK; >> + >> + if (enable) >> + reg_irqen |= IRQ_SDIO; >> + writel(reg_irqen, host->regs + SD_EMMC_IRQ_EN); >> +} >> + >> static irqreturn_t meson_mmc_irq(int irq, void *dev_id) >> { >> struct meson_host *host = dev_id; >> struct mmc_command *cmd; >> - struct mmc_data *data; >> u32 status, raw_status; >> irqreturn_t ret = IRQ_NONE; >> >> raw_status = readl(host->regs + SD_EMMC_STATUS); >> - status = raw_status & IRQ_EN_MASK; >> + status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); >> >> if (!status) { >> dev_dbg(host->dev, >> "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", >> - IRQ_EN_MASK, raw_status); >> + IRQ_EN_MASK | IRQ_SDIO, raw_status); >> return IRQ_NONE; >> } >> >> - if (WARN_ON(!host) || WARN_ON(!host->cmd)) >> + if (WARN_ON(!host)) >> return IRQ_NONE; >> >> /* ack all raised interrupts */ >> writel(status, host->regs + SD_EMMC_STATUS); >> >> cmd = host->cmd; >> - data = cmd->data; >> + >> + if (status & IRQ_SDIO) { >> + spin_lock(&host->lock); >> + __meson_mmc_enable_sdio_irq(host->mmc, 0); >> + sdio_signal_irq(host->mmc); >> + spin_unlock(&host->lock); >> + status &= ~IRQ_SDIO; >> + if (!status) >> + return IRQ_HANDLED; >> + } >> + >> + if (WARN_ON(!cmd)) >> + return IRQ_NONE; >> + >> cmd->error = 0; >> if (status & IRQ_CRC_ERR) { >> dev_dbg(host->dev, "CRC Error - status 0x%08x\n", status); >> @@ -977,12 +1005,9 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) >> >> meson_mmc_read_resp(host->mmc, cmd); >> >> - if (status & IRQ_SDIO) { >> - dev_dbg(host->dev, "IRQ: SDIO TODO.\n"); >> - ret = IRQ_HANDLED; >> - } >> - >> if (status & (IRQ_END_OF_CHAIN | IRQ_RESP_STATUS)) { >> + struct mmc_data *data = cmd->data; >> + >> if (data && !cmd->error) >> data->bytes_xfered = data->blksz * data->blocks; >> if (meson_mmc_bounce_buf_read(data) || >> @@ -1125,6 +1150,27 @@ static int meson_mmc_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) >> return -EINVAL; >> } >> >> +static void meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) >> +{ >> + struct meson_host *host = mmc_priv(mmc); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&host->lock, flags); >> + __meson_mmc_enable_sdio_irq(mmc, enable); >> + spin_unlock_irqrestore(&host->lock, flags); >> +} >> + >> +static void meson_mmc_ack_sdio_irq(struct mmc_host *mmc) >> +{ >> + struct meson_host *host = mmc_priv(mmc); >> + unsigned long flags; >> + >> + spin_lock_irqsave(&host->lock, flags); >> + if (!mmc->sdio_irq_pending) > > I am not quite sure I understand why you need to check this flag here. > > The point is, in meson_mmc_irq() you are doing things in the correct > order, which means disabling the SDIO irq by calling > __meson_mmc_enable_sdio_irq(host->mmc, 0) prior calling > sdio_signal_irq(host->mmc) (which sets the flag). > > In this way, the host driver should be prevented from signaling > another new SDIO irq, until it has acked (which means re-enabling the > SDIO irqs) the current one to be processed. > > Or did I miss something? > What could happen (at least based on the code): Even though SDIO IRQ signalling is disabled we may receive another interrupt that by chance has also the SDIO IRQ status bit set. And if the hard irq handler interrupts sdio_run_irqs() here then we may have a problem. if (!host->sdio_irq_pending) host->ops->ack_sdio_irq(host); Therefore the additional check (because checking the flag and calling __meson_mmc_enable_sdio_irq() is protected by the spinlock). However that's a theoretical worst case scenario. It don't know enough about SDIO interrupts to state whether it actually can happen. By the way: I'd also be reluctant to omit checking the SDIO IRQ status bit if host->sdio_irq_pending is true because then we might miss a SDIO interrupt. But if you say it's not needed I'll remove the duplicated flag check. >> + __meson_mmc_enable_sdio_irq(mmc, 1); >> + spin_unlock_irqrestore(&host->lock, flags); >> +} >> > > [...] > > Other than the comment above, the patch looks good to me! > > Kind regards > Uffe
On Tue, 23 Aug 2022 at 20:16, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 22.08.2022 13:33, Ulf Hansson wrote: > > On Fri, 19 Aug 2022 at 23:14, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> Add SDIO interrupt support. Successfully tested on a S905X4-based > >> system (V3 register layout) with a BRCM4334 SDIO wifi module > >> (brcmfmac driver). The implementation also considers the potential > >> race discussed in [0]. > >> > >> [0] https://lore.kernel.org/linux-arm-kernel/CAPDyKFoJDhjLkajBHgW3zHasvYYri77NQoDpiW-BpKgkdjtOyg@mail.gmail.com/ > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/mmc/host/meson-gx-mmc.c | 76 ++++++++++++++++++++++++++++----- > >> 1 file changed, 66 insertions(+), 10 deletions(-) > >> > >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > >> index 9a4da2544..58b7836a5 100644 > >> --- a/drivers/mmc/host/meson-gx-mmc.c > >> +++ b/drivers/mmc/host/meson-gx-mmc.c > >> @@ -41,14 +41,17 @@ > >> #define CLK_V2_TX_DELAY_MASK GENMASK(19, 16) > >> #define CLK_V2_RX_DELAY_MASK GENMASK(23, 20) > >> #define CLK_V2_ALWAYS_ON BIT(24) > >> +#define CLK_V2_IRQ_SDIO_SLEEP BIT(25) > >> > >> #define CLK_V3_TX_DELAY_MASK GENMASK(21, 16) > >> #define CLK_V3_RX_DELAY_MASK GENMASK(27, 22) > >> #define CLK_V3_ALWAYS_ON BIT(28) > >> +#define CLK_V3_IRQ_SDIO_SLEEP BIT(29) > >> > >> #define CLK_TX_DELAY_MASK(h) (h->data->tx_delay_mask) > >> #define CLK_RX_DELAY_MASK(h) (h->data->rx_delay_mask) > >> #define CLK_ALWAYS_ON(h) (h->data->always_on) > >> +#define CLK_IRQ_SDIO_SLEEP(h) (h->data->irq_sdio_sleep) > >> > >> #define SD_EMMC_DELAY 0x4 > >> #define SD_EMMC_ADJUST 0x8 > >> @@ -135,6 +138,7 @@ struct meson_mmc_data { > >> unsigned int rx_delay_mask; > >> unsigned int always_on; > >> unsigned int adjust; > >> + unsigned int irq_sdio_sleep; > >> }; > >> > >> struct sd_emmc_desc { > >> @@ -174,6 +178,7 @@ struct meson_host { > >> bool vqmmc_enabled; > >> bool needs_pre_post_req; > >> > >> + spinlock_t lock; > >> }; > >> > >> #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) > >> @@ -430,6 +435,7 @@ static int meson_mmc_clk_init(struct meson_host *host) > >> clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180); > >> clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0); > >> clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); > >> + clk_reg |= CLK_IRQ_SDIO_SLEEP(host); > >> writel(clk_reg, host->regs + SD_EMMC_CLOCK); > >> > >> /* get the mux parents */ > >> @@ -934,32 +940,54 @@ static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) > >> } > >> } > >> > >> +static void __meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > >> +{ > >> + struct meson_host *host = mmc_priv(mmc); > >> + u32 reg_irqen = IRQ_EN_MASK; > >> + > >> + if (enable) > >> + reg_irqen |= IRQ_SDIO; > >> + writel(reg_irqen, host->regs + SD_EMMC_IRQ_EN); > >> +} > >> + > >> static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > >> { > >> struct meson_host *host = dev_id; > >> struct mmc_command *cmd; > >> - struct mmc_data *data; > >> u32 status, raw_status; > >> irqreturn_t ret = IRQ_NONE; > >> > >> raw_status = readl(host->regs + SD_EMMC_STATUS); > >> - status = raw_status & IRQ_EN_MASK; > >> + status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); > >> > >> if (!status) { > >> dev_dbg(host->dev, > >> "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", > >> - IRQ_EN_MASK, raw_status); > >> + IRQ_EN_MASK | IRQ_SDIO, raw_status); > >> return IRQ_NONE; > >> } > >> > >> - if (WARN_ON(!host) || WARN_ON(!host->cmd)) > >> + if (WARN_ON(!host)) > >> return IRQ_NONE; > >> > >> /* ack all raised interrupts */ > >> writel(status, host->regs + SD_EMMC_STATUS); > >> > >> cmd = host->cmd; > >> - data = cmd->data; > >> + > >> + if (status & IRQ_SDIO) { > >> + spin_lock(&host->lock); > >> + __meson_mmc_enable_sdio_irq(host->mmc, 0); > >> + sdio_signal_irq(host->mmc); > >> + spin_unlock(&host->lock); > >> + status &= ~IRQ_SDIO; > >> + if (!status) > >> + return IRQ_HANDLED; > >> + } > >> + > >> + if (WARN_ON(!cmd)) > >> + return IRQ_NONE; > >> + > >> cmd->error = 0; > >> if (status & IRQ_CRC_ERR) { > >> dev_dbg(host->dev, "CRC Error - status 0x%08x\n", status); > >> @@ -977,12 +1005,9 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > >> > >> meson_mmc_read_resp(host->mmc, cmd); > >> > >> - if (status & IRQ_SDIO) { > >> - dev_dbg(host->dev, "IRQ: SDIO TODO.\n"); > >> - ret = IRQ_HANDLED; > >> - } > >> - > >> if (status & (IRQ_END_OF_CHAIN | IRQ_RESP_STATUS)) { > >> + struct mmc_data *data = cmd->data; > >> + > >> if (data && !cmd->error) > >> data->bytes_xfered = data->blksz * data->blocks; > >> if (meson_mmc_bounce_buf_read(data) || > >> @@ -1125,6 +1150,27 @@ static int meson_mmc_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) > >> return -EINVAL; > >> } > >> > >> +static void meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) > >> +{ > >> + struct meson_host *host = mmc_priv(mmc); > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&host->lock, flags); > >> + __meson_mmc_enable_sdio_irq(mmc, enable); > >> + spin_unlock_irqrestore(&host->lock, flags); > >> +} > >> + > >> +static void meson_mmc_ack_sdio_irq(struct mmc_host *mmc) > >> +{ > >> + struct meson_host *host = mmc_priv(mmc); > >> + unsigned long flags; > >> + > >> + spin_lock_irqsave(&host->lock, flags); > >> + if (!mmc->sdio_irq_pending) > > > > I am not quite sure I understand why you need to check this flag here. > > > > The point is, in meson_mmc_irq() you are doing things in the correct > > order, which means disabling the SDIO irq by calling > > __meson_mmc_enable_sdio_irq(host->mmc, 0) prior calling > > sdio_signal_irq(host->mmc) (which sets the flag). > > > > In this way, the host driver should be prevented from signaling > > another new SDIO irq, until it has acked (which means re-enabling the > > SDIO irqs) the current one to be processed. > > > > Or did I miss something? > > > > What could happen (at least based on the code): > Even though SDIO IRQ signalling is disabled we may receive another > interrupt that by chance has also the SDIO IRQ status bit set. That sounds like a broken HW to me, but I get your point. > And if the hard irq handler interrupts sdio_run_irqs() here then > we may have a problem. > > if (!host->sdio_irq_pending) > host->ops->ack_sdio_irq(host); > > Therefore the additional check (because checking the flag and calling > __meson_mmc_enable_sdio_irq() is protected by the spinlock). > However that's a theoretical worst case scenario. It don't know enough > about SDIO interrupts to state whether it actually can happen. If you are worried about getting spurious IRQs with the SDIO IRQ status bit set, then I prefer that you use a meson host driver specific flag. The flag could indicate whether the IRQ_SDIO bit has been set in the IRQ_EN_MASK (__meson_mmc_enable_sdio_irq()) and then simply prevent (or postpone) signaling a new SDIO IRQ (sdio_signal_irq()). > > By the way: I'd also be reluctant to omit checking the SDIO IRQ status bit > if host->sdio_irq_pending is true because then we might miss a SDIO interrupt. Again, that sounds like a broken HW to me. If you are worried that we may miss an SDIO irq, then it's seems better to "cache" that additional SDIO IRQ in a state variable (from the irq handler) and then check that variable in ->ack_sdio_irq() to immediately signal a new SDIO irq (sdio_signal_irq()). > > But if you say it's not needed I'll remove the duplicated flag check. Let me put it like this, if the HW works as expected, the additional check/protection should not be needed. So in the end, this is your call. > > >> + __meson_mmc_enable_sdio_irq(mmc, 1); > >> + spin_unlock_irqrestore(&host->lock, flags); > >> +} > >> [...] Kind regards Uffe
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 9a4da2544..58b7836a5 100644 --- a/drivers/mmc/host/meson-gx-mmc.c +++ b/drivers/mmc/host/meson-gx-mmc.c @@ -41,14 +41,17 @@ #define CLK_V2_TX_DELAY_MASK GENMASK(19, 16) #define CLK_V2_RX_DELAY_MASK GENMASK(23, 20) #define CLK_V2_ALWAYS_ON BIT(24) +#define CLK_V2_IRQ_SDIO_SLEEP BIT(25) #define CLK_V3_TX_DELAY_MASK GENMASK(21, 16) #define CLK_V3_RX_DELAY_MASK GENMASK(27, 22) #define CLK_V3_ALWAYS_ON BIT(28) +#define CLK_V3_IRQ_SDIO_SLEEP BIT(29) #define CLK_TX_DELAY_MASK(h) (h->data->tx_delay_mask) #define CLK_RX_DELAY_MASK(h) (h->data->rx_delay_mask) #define CLK_ALWAYS_ON(h) (h->data->always_on) +#define CLK_IRQ_SDIO_SLEEP(h) (h->data->irq_sdio_sleep) #define SD_EMMC_DELAY 0x4 #define SD_EMMC_ADJUST 0x8 @@ -135,6 +138,7 @@ struct meson_mmc_data { unsigned int rx_delay_mask; unsigned int always_on; unsigned int adjust; + unsigned int irq_sdio_sleep; }; struct sd_emmc_desc { @@ -174,6 +178,7 @@ struct meson_host { bool vqmmc_enabled; bool needs_pre_post_req; + spinlock_t lock; }; #define CMD_CFG_LENGTH_MASK GENMASK(8, 0) @@ -430,6 +435,7 @@ static int meson_mmc_clk_init(struct meson_host *host) clk_reg |= FIELD_PREP(CLK_CORE_PHASE_MASK, CLK_PHASE_180); clk_reg |= FIELD_PREP(CLK_TX_PHASE_MASK, CLK_PHASE_0); clk_reg |= FIELD_PREP(CLK_RX_PHASE_MASK, CLK_PHASE_0); + clk_reg |= CLK_IRQ_SDIO_SLEEP(host); writel(clk_reg, host->regs + SD_EMMC_CLOCK); /* get the mux parents */ @@ -934,32 +940,54 @@ static void meson_mmc_read_resp(struct mmc_host *mmc, struct mmc_command *cmd) } } +static void __meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) +{ + struct meson_host *host = mmc_priv(mmc); + u32 reg_irqen = IRQ_EN_MASK; + + if (enable) + reg_irqen |= IRQ_SDIO; + writel(reg_irqen, host->regs + SD_EMMC_IRQ_EN); +} + static irqreturn_t meson_mmc_irq(int irq, void *dev_id) { struct meson_host *host = dev_id; struct mmc_command *cmd; - struct mmc_data *data; u32 status, raw_status; irqreturn_t ret = IRQ_NONE; raw_status = readl(host->regs + SD_EMMC_STATUS); - status = raw_status & IRQ_EN_MASK; + status = raw_status & (IRQ_EN_MASK | IRQ_SDIO); if (!status) { dev_dbg(host->dev, "Unexpected IRQ! irq_en 0x%08lx - status 0x%08x\n", - IRQ_EN_MASK, raw_status); + IRQ_EN_MASK | IRQ_SDIO, raw_status); return IRQ_NONE; } - if (WARN_ON(!host) || WARN_ON(!host->cmd)) + if (WARN_ON(!host)) return IRQ_NONE; /* ack all raised interrupts */ writel(status, host->regs + SD_EMMC_STATUS); cmd = host->cmd; - data = cmd->data; + + if (status & IRQ_SDIO) { + spin_lock(&host->lock); + __meson_mmc_enable_sdio_irq(host->mmc, 0); + sdio_signal_irq(host->mmc); + spin_unlock(&host->lock); + status &= ~IRQ_SDIO; + if (!status) + return IRQ_HANDLED; + } + + if (WARN_ON(!cmd)) + return IRQ_NONE; + cmd->error = 0; if (status & IRQ_CRC_ERR) { dev_dbg(host->dev, "CRC Error - status 0x%08x\n", status); @@ -977,12 +1005,9 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) meson_mmc_read_resp(host->mmc, cmd); - if (status & IRQ_SDIO) { - dev_dbg(host->dev, "IRQ: SDIO TODO.\n"); - ret = IRQ_HANDLED; - } - if (status & (IRQ_END_OF_CHAIN | IRQ_RESP_STATUS)) { + struct mmc_data *data = cmd->data; + if (data && !cmd->error) data->bytes_xfered = data->blksz * data->blocks; if (meson_mmc_bounce_buf_read(data) || @@ -1125,6 +1150,27 @@ static int meson_mmc_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios) return -EINVAL; } +static void meson_mmc_enable_sdio_irq(struct mmc_host *mmc, int enable) +{ + struct meson_host *host = mmc_priv(mmc); + unsigned long flags; + + spin_lock_irqsave(&host->lock, flags); + __meson_mmc_enable_sdio_irq(mmc, enable); + spin_unlock_irqrestore(&host->lock, flags); +} + +static void meson_mmc_ack_sdio_irq(struct mmc_host *mmc) +{ + struct meson_host *host = mmc_priv(mmc); + unsigned long flags; + + spin_lock_irqsave(&host->lock, flags); + if (!mmc->sdio_irq_pending) + __meson_mmc_enable_sdio_irq(mmc, 1); + spin_unlock_irqrestore(&host->lock, flags); +} + static const struct mmc_host_ops meson_mmc_ops = { .request = meson_mmc_request, .set_ios = meson_mmc_set_ios, @@ -1134,6 +1180,8 @@ static const struct mmc_host_ops meson_mmc_ops = { .execute_tuning = meson_mmc_resampling_tuning, .card_busy = meson_mmc_card_busy, .start_signal_voltage_switch = meson_mmc_voltage_switch, + .enable_sdio_irq = meson_mmc_enable_sdio_irq, + .ack_sdio_irq = meson_mmc_ack_sdio_irq, }; static int meson_mmc_probe(struct platform_device *pdev) @@ -1237,7 +1285,13 @@ static int meson_mmc_probe(struct platform_device *pdev) if (ret) goto err_init_clk; + spin_lock_init(&host->lock); + mmc->caps |= MMC_CAP_CMD23; + + if (mmc->caps & MMC_CAP_SDIO_IRQ) + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD; + if (host->dram_access_quirk) { /* Limit segments to 1 due to low available sram memory */ mmc->max_segs = 1; @@ -1328,6 +1382,7 @@ static const struct meson_mmc_data meson_gx_data = { .rx_delay_mask = CLK_V2_RX_DELAY_MASK, .always_on = CLK_V2_ALWAYS_ON, .adjust = SD_EMMC_ADJUST, + .irq_sdio_sleep = CLK_V2_IRQ_SDIO_SLEEP, }; static const struct meson_mmc_data meson_axg_data = { @@ -1335,6 +1390,7 @@ static const struct meson_mmc_data meson_axg_data = { .rx_delay_mask = CLK_V3_RX_DELAY_MASK, .always_on = CLK_V3_ALWAYS_ON, .adjust = SD_EMMC_V3_ADJUST, + .irq_sdio_sleep = CLK_V3_IRQ_SDIO_SLEEP, }; static const struct of_device_id meson_mmc_of_match[] = {
Add SDIO interrupt support. Successfully tested on a S905X4-based system (V3 register layout) with a BRCM4334 SDIO wifi module (brcmfmac driver). The implementation also considers the potential race discussed in [0]. [0] https://lore.kernel.org/linux-arm-kernel/CAPDyKFoJDhjLkajBHgW3zHasvYYri77NQoDpiW-BpKgkdjtOyg@mail.gmail.com/ Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/mmc/host/meson-gx-mmc.c | 76 ++++++++++++++++++++++++++++----- 1 file changed, 66 insertions(+), 10 deletions(-)