Re: [PATCH] mmc: meson-gx: add SDIO interrupt support

[Heiner Kallweit <hkallweit1@gmail.com>]

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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel