[PATCH v2 2/2] mmc: meson-gx: add SDIO interrupt support
Heiner Kallweit
hkallweit1 at gmail.com
Tue Aug 23 11:16:49 PDT 2022
On 22.08.2022 13:33, Ulf Hansson wrote:
> On Fri, 19 Aug 2022 at 23:14, Heiner Kallweit <hkallweit1 at 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 at 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
More information about the linux-amlogic
mailing list