| Message ID | ecc229ad-c15a-2092-6568-f92e4507553e@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: meson-gx: add SDIO interrupt support | expand |
On Sun, 14 Aug 2022 at 23:44, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > This adds SDIO interrupt support. > Successfully tested on a S905X4-based system with a BRCM4334 > SDIO wifi module (brcmfmac driver). > > Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > --- > drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- > 1 file changed, 34 insertions(+), 11 deletions(-) > > diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > index 2f08d442e..e8d53fcdd 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(29) > > #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 > @@ -100,9 +103,6 @@ > #define IRQ_END_OF_CHAIN BIT(13) > #define IRQ_RESP_STATUS BIT(14) > #define IRQ_SDIO BIT(15) > -#define IRQ_EN_MASK \ > - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ > - IRQ_SDIO) > > #define SD_EMMC_CMD_CFG 0x50 > #define SD_EMMC_CMD_ARG 0x54 > @@ -136,6 +136,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 { > @@ -431,6 +432,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 */ > @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; > irqreturn_t ret = IRQ_NONE; > > @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > 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) { > + mmc_signal_sdio_irq(host->mmc); This is the legacy interface for supporting SDIO irqs. I am planning to remove it, sooner or later. Please convert into using sdio_signal_irq() instead. Note that, using sdio_signal_irq() means you need to implement support for MMC_CAP2_SDIO_IRQ_NOTHREAD, which also includes to implement the ->ack_sdio_irq() callback. There are other host drivers to be inspired from, but don't hesitate to ask if there is something unclear. [...] Kind regards Uffe
On 15.08.2022 20:20, Ulf Hansson wrote: > On Sun, 14 Aug 2022 at 23:44, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> This adds SDIO interrupt support. >> Successfully tested on a S905X4-based system with a BRCM4334 >> SDIO wifi module (brcmfmac driver). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- >> 1 file changed, 34 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index 2f08d442e..e8d53fcdd 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(29) >> >> #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 >> @@ -100,9 +103,6 @@ >> #define IRQ_END_OF_CHAIN BIT(13) >> #define IRQ_RESP_STATUS BIT(14) >> #define IRQ_SDIO BIT(15) >> -#define IRQ_EN_MASK \ >> - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ >> - IRQ_SDIO) >> >> #define SD_EMMC_CMD_CFG 0x50 >> #define SD_EMMC_CMD_ARG 0x54 >> @@ -136,6 +136,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 { >> @@ -431,6 +432,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 */ >> @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; >> irqreturn_t ret = IRQ_NONE; >> >> @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) >> 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) { >> + mmc_signal_sdio_irq(host->mmc); > > This is the legacy interface for supporting SDIO irqs. I am planning > to remove it, sooner or later. > > Please convert into using sdio_signal_irq() instead. Note that, using > sdio_signal_irq() means you need to implement support for > MMC_CAP2_SDIO_IRQ_NOTHREAD, which also includes to implement the > ->ack_sdio_irq() callback. > > There are other host drivers to be inspired from, but don't hesitate > to ask if there is something unclear. > I switched to the new API and it works for me, will submit a v2. Just one question regarding core code: The core defines sdio_irq_work as delayed work, but no caller sets a delay. Then why not use a simple workqueue and schedule_work() instead of queue_delayed_work()? > [...] > > Kind regards > Uffe
On 15.08.2022 20:20, Ulf Hansson wrote: > On Sun, 14 Aug 2022 at 23:44, Heiner Kallweit <hkallweit1@gmail.com> wrote: >> >> This adds SDIO interrupt support. >> Successfully tested on a S905X4-based system with a BRCM4334 >> SDIO wifi module (brcmfmac driver). >> >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> >> --- >> drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- >> 1 file changed, 34 insertions(+), 11 deletions(-) >> >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c >> index 2f08d442e..e8d53fcdd 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(29) >> >> #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 >> @@ -100,9 +103,6 @@ >> #define IRQ_END_OF_CHAIN BIT(13) >> #define IRQ_RESP_STATUS BIT(14) >> #define IRQ_SDIO BIT(15) >> -#define IRQ_EN_MASK \ >> - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ >> - IRQ_SDIO) >> >> #define SD_EMMC_CMD_CFG 0x50 >> #define SD_EMMC_CMD_ARG 0x54 >> @@ -136,6 +136,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 { >> @@ -431,6 +432,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 */ >> @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; >> irqreturn_t ret = IRQ_NONE; >> >> @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) >> 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) { >> + mmc_signal_sdio_irq(host->mmc); > > This is the legacy interface for supporting SDIO irqs. I am planning > to remove it, sooner or later. > > Please convert into using sdio_signal_irq() instead. Note that, using > sdio_signal_irq() means you need to implement support for > MMC_CAP2_SDIO_IRQ_NOTHREAD, which also includes to implement the > ->ack_sdio_irq() callback. > > There are other host drivers to be inspired from, but don't hesitate > to ask if there is something unclear. > One more question came to my mind: Typically host drivers disable the SDIO interrupt source before calling sdio_signal_irq(), and re-enable it in ->ack_sdio_irq(). In sdio_run_irqs() we have the following: if (!host->sdio_irq_pending) host->ops->ack_sdio_irq(host); In the middle of this code the host can't actively trigger a SDIO interrupt because the interrupt source is still disabled. But some other host interrupt could fire with also the SDIO interrupt source bit set. Then the hard irq handler would disable the SDIO interrupt source, and directly after this ->ack_sdio_irq() would re-enable it. This looks racy to me and we may need some protection. Do you share this view or do I miss something? > [...] > > Kind regards > Uffe
On Thu, 18 Aug 2022 at 07:54, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 15.08.2022 20:20, Ulf Hansson wrote: > > On Sun, 14 Aug 2022 at 23:44, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> This adds SDIO interrupt support. > >> Successfully tested on a S905X4-based system with a BRCM4334 > >> SDIO wifi module (brcmfmac driver). > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- > >> 1 file changed, 34 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > >> index 2f08d442e..e8d53fcdd 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(29) > >> > >> #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 > >> @@ -100,9 +103,6 @@ > >> #define IRQ_END_OF_CHAIN BIT(13) > >> #define IRQ_RESP_STATUS BIT(14) > >> #define IRQ_SDIO BIT(15) > >> -#define IRQ_EN_MASK \ > >> - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ > >> - IRQ_SDIO) > >> > >> #define SD_EMMC_CMD_CFG 0x50 > >> #define SD_EMMC_CMD_ARG 0x54 > >> @@ -136,6 +136,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 { > >> @@ -431,6 +432,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 */ > >> @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; > >> irqreturn_t ret = IRQ_NONE; > >> > >> @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > >> 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) { > >> + mmc_signal_sdio_irq(host->mmc); > > > > This is the legacy interface for supporting SDIO irqs. I am planning > > to remove it, sooner or later. > > > > Please convert into using sdio_signal_irq() instead. Note that, using > > sdio_signal_irq() means you need to implement support for > > MMC_CAP2_SDIO_IRQ_NOTHREAD, which also includes to implement the > > ->ack_sdio_irq() callback. > > > > There are other host drivers to be inspired from, but don't hesitate > > to ask if there is something unclear. > > > > I switched to the new API and it works for me, will submit a v2. > > Just one question regarding core code: > The core defines sdio_irq_work as delayed work, but no caller sets a delay. > Then why not use a simple workqueue and schedule_work() instead of > queue_delayed_work()? That should work, I think. I don't quite recall why that wasn't made in the original solution, probably just a mistake. One thing that I do recall, is that we may want to consider running our own workqueue in the future, if it turns out that we are overloading the system_wq. But so far, none have reported problems. Kind regards Uffe
On Thu, 18 Aug 2022 at 08:20, Heiner Kallweit <hkallweit1@gmail.com> wrote: > > On 15.08.2022 20:20, Ulf Hansson wrote: > > On Sun, 14 Aug 2022 at 23:44, Heiner Kallweit <hkallweit1@gmail.com> wrote: > >> > >> This adds SDIO interrupt support. > >> Successfully tested on a S905X4-based system with a BRCM4334 > >> SDIO wifi module (brcmfmac driver). > >> > >> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> > >> --- > >> drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- > >> 1 file changed, 34 insertions(+), 11 deletions(-) > >> > >> diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c > >> index 2f08d442e..e8d53fcdd 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(29) > >> > >> #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 > >> @@ -100,9 +103,6 @@ > >> #define IRQ_END_OF_CHAIN BIT(13) > >> #define IRQ_RESP_STATUS BIT(14) > >> #define IRQ_SDIO BIT(15) > >> -#define IRQ_EN_MASK \ > >> - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ > >> - IRQ_SDIO) > >> > >> #define SD_EMMC_CMD_CFG 0x50 > >> #define SD_EMMC_CMD_ARG 0x54 > >> @@ -136,6 +136,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 { > >> @@ -431,6 +432,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 */ > >> @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; > >> irqreturn_t ret = IRQ_NONE; > >> > >> @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) > >> 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) { > >> + mmc_signal_sdio_irq(host->mmc); > > > > This is the legacy interface for supporting SDIO irqs. I am planning > > to remove it, sooner or later. > > > > Please convert into using sdio_signal_irq() instead. Note that, using > > sdio_signal_irq() means you need to implement support for > > MMC_CAP2_SDIO_IRQ_NOTHREAD, which also includes to implement the > > ->ack_sdio_irq() callback. > > > > There are other host drivers to be inspired from, but don't hesitate > > to ask if there is something unclear. > > > > One more question came to my mind: > > Typically host drivers disable the SDIO interrupt source before calling > sdio_signal_irq(), and re-enable it in ->ack_sdio_irq(). > > In sdio_run_irqs() we have the following: > > if (!host->sdio_irq_pending) > host->ops->ack_sdio_irq(host); > > In the middle of this code the host can't actively trigger a SDIO interrupt > because the interrupt source is still disabled. But some other host interrupt > could fire with also the SDIO interrupt source bit set. It's the responsibility of the host driver to deal with this situation. > Then the hard irq handler would disable the SDIO interrupt source, and directly > after this ->ack_sdio_irq() would re-enable it. The host driver shouldn't signal a new irq, before it has acked the previous one via ->ack_sdio_irq(). > This looks racy to me and we may need some protection. > Do you share this view or do I miss something? The protection the mmc core provides around this, includes the general host protection (mmc_claim|release_host()) in sdio_run_irqs() and by using the workqueue of course. Did that make sense? Kind regards Uffe
diff --git a/drivers/mmc/host/meson-gx-mmc.c b/drivers/mmc/host/meson-gx-mmc.c index 2f08d442e..e8d53fcdd 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(29) #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 @@ -100,9 +103,6 @@ #define IRQ_END_OF_CHAIN BIT(13) #define IRQ_RESP_STATUS BIT(14) #define IRQ_SDIO BIT(15) -#define IRQ_EN_MASK \ - (IRQ_CRC_ERR | IRQ_TIMEOUTS | IRQ_END_OF_CHAIN | IRQ_RESP_STATUS |\ - IRQ_SDIO) #define SD_EMMC_CMD_CFG 0x50 #define SD_EMMC_CMD_ARG 0x54 @@ -136,6 +136,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 { @@ -431,6 +432,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 */ @@ -933,7 +935,6 @@ 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 irq_en, status, raw_status; irqreturn_t ret = IRQ_NONE; @@ -948,14 +949,24 @@ static irqreturn_t meson_mmc_irq(int irq, void *dev_id) 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) { + mmc_signal_sdio_irq(host->mmc); + 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); @@ -973,12 +984,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) || @@ -1121,6 +1129,18 @@ 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); + u32 reg_irqen; + + reg_irqen = readl(host->regs + SD_EMMC_IRQ_EN); + reg_irqen &= ~IRQ_SDIO; + if (enable) + reg_irqen |= IRQ_SDIO; + writel(reg_irqen, host->regs + SD_EMMC_IRQ_EN); +} + static const struct mmc_host_ops meson_mmc_ops = { .request = meson_mmc_request, .set_ios = meson_mmc_set_ios, @@ -1130,6 +1150,7 @@ 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, }; static int meson_mmc_probe(struct platform_device *pdev) @@ -1326,6 +1347,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 = { @@ -1333,6 +1355,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[] = {
This adds SDIO interrupt support. Successfully tested on a S905X4-based system with a BRCM4334 SDIO wifi module (brcmfmac driver). Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com> --- drivers/mmc/host/meson-gx-mmc.c | 45 +++++++++++++++++++++++++-------- 1 file changed, 34 insertions(+), 11 deletions(-)