diff mbox series

mmc: meson-gx: add SDIO interrupt support

Message ID ecc229ad-c15a-2092-6568-f92e4507553e@gmail.com (mailing list archive)
State Superseded
Headers show
Series mmc: meson-gx: add SDIO interrupt support | expand

Commit Message

Heiner Kallweit Aug. 14, 2022, 9:43 p.m. UTC
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(-)

Comments

Ulf Hansson Aug. 15, 2022, 6:20 p.m. UTC | #1
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
Heiner Kallweit Aug. 18, 2022, 5:54 a.m. UTC | #2
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
Heiner Kallweit Aug. 18, 2022, 6:20 a.m. UTC | #3
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
Ulf Hansson Aug. 18, 2022, 10 a.m. UTC | #4
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
Ulf Hansson Aug. 18, 2022, 10:11 a.m. UTC | #5
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 mbox series

Patch

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[] = {