* [PATCH v3] mmc: mediatek: Add MMC_CAP_SDIO_IRQ support
@ 2018-12-10 11:43 Jjian Zhou
2018-12-11 9:29 ` Ulf Hansson
0 siblings, 1 reply; 3+ messages in thread
From: Jjian Zhou @ 2018-12-10 11:43 UTC (permalink / raw)
To: Ulf Hansson
Cc: Matthias Brugger, Chaotian Jing, Ryder Lee, jjian zhou,
Sean Wang, linux-mmc, linux-arm-kernel, linux-mediatek,
srv_heupstream, linux-kernel, Yong Mao
From: jjian zhou <jjian.zhou@mediatek.com>
This patch enables support SDIO IRQs. It enables
MMC_CAP_SDIO_IRQ & MMC_CAP2_SDIO_IRQ_NOTHREAD
and implement the ->ack_sdio_irq callback.
Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
Signed-off-by: Yong Mao <yong.mao@mediatek.com>
---
drivers/mmc/host/mtk-sd.c | 49 ++++++++++++++++++++++++++++++++++++++++++++---
1 file changed, 46 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 6334cc7..40a1848 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -1114,6 +1114,7 @@ static void msdc_start_command(struct msdc_host *host,
struct mmc_request *mrq, struct mmc_command *cmd)
{
u32 rawcmd;
+ unsigned long flags;
WARN_ON(host->cmd);
host->cmd = cmd;
@@ -1131,7 +1132,10 @@ static void msdc_start_command(struct msdc_host *host,
cmd->error = 0;
rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
+ spin_lock_irqsave(&host->lock, flags);
sdr_set_bits(host->base + MSDC_INTEN, cmd_ints_mask);
+ spin_unlock_irqrestore(&host->lock, flags);
+
writel(cmd->arg, host->base + SDC_ARG);
writel(rawcmd, host->base + SDC_CMD);
}
@@ -1351,6 +1355,27 @@ static void msdc_request_timeout(struct work_struct *work)
}
}
+static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
+{
+ unsigned long flags;
+ struct msdc_host *host = mmc_priv(mmc);
+
+ if (enb)
+ pm_runtime_get_sync(host->dev);
+
+ spin_lock_irqsave(&host->lock, flags);
+ if (enb)
+ sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
+ else
+ sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ if (!enb) {
+ pm_runtime_mark_last_busy(host->dev);
+ pm_runtime_put_autosuspend(host->dev);
+ }
+}
+
static irqreturn_t msdc_irq(int irq, void *dev_id)
{
struct msdc_host *host = (struct msdc_host *) dev_id;
@@ -1373,7 +1398,12 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
data = host->data;
spin_unlock_irqrestore(&host->lock, flags);
- if (!(events & event_mask))
+ if ((events & event_mask) & MSDC_INT_SDIOIRQ) {
+ msdc_enable_sdio_irq(host->mmc, 0);
+ sdio_signal_irq(host->mmc);
+ }
+
+ if (!(events & (event_mask & ~MSDC_INT_SDIOIRQ)))
break;
if (!mrq) {
@@ -1493,8 +1523,11 @@ static void msdc_init_hw(struct msdc_host *host)
*/
sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
- /* disable detect SDIO device interrupt function */
- sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+ /* Config SDIO device detect interrupt function */
+ if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
+ sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
+ else
+ sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
/* Configure to default data timeout */
sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
@@ -2013,6 +2046,11 @@ static void msdc_hw_reset(struct mmc_host *mmc)
sdr_clr_bits(host->base + EMMC_IOCON, 1);
}
+static void msdc_ack_sdio_irq(struct mmc_host *mmc)
+{
+ msdc_enable_sdio_irq(mmc, 1);
+}
+
static const struct mmc_host_ops mt_msdc_ops = {
.post_req = msdc_post_req,
.pre_req = msdc_pre_req,
@@ -2020,6 +2058,8 @@ static void msdc_hw_reset(struct mmc_host *mmc)
.set_ios = msdc_ops_set_ios,
.get_ro = mmc_gpio_get_ro,
.get_cd = mmc_gpio_get_cd,
+ .enable_sdio_irq = msdc_enable_sdio_irq,
+ .ack_sdio_irq = msdc_ack_sdio_irq,
.start_signal_voltage_switch = msdc_ops_switch_volt,
.card_busy = msdc_card_busy,
.execute_tuning = msdc_execute_tuning,
@@ -2147,6 +2187,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
else
mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 4095);
+ if (mmc->caps & MMC_CAP_SDIO_IRQ)
+ mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
+
mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
/* MMC core transfer sizes tunable parameters */
mmc->max_segs = MAX_BD_NUM;
--
1.9.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v3] mmc: mediatek: Add MMC_CAP_SDIO_IRQ support
2018-12-10 11:43 [PATCH v3] mmc: mediatek: Add MMC_CAP_SDIO_IRQ support Jjian Zhou
@ 2018-12-11 9:29 ` Ulf Hansson
2018-12-11 12:11 ` Jjian Zhou
0 siblings, 1 reply; 3+ messages in thread
From: Ulf Hansson @ 2018-12-11 9:29 UTC (permalink / raw)
To: jjian.zhou
Cc: Matthias Brugger, Chaotian Jing, Ryder Lee, Sean Wang, linux-mmc,
Linux ARM, linux-mediatek, srv_heupstream,
Linux Kernel Mailing List, Yong Mao
On Mon, 10 Dec 2018 at 12:44, Jjian Zhou <jjian.zhou@mediatek.com> wrote:
>
> From: jjian zhou <jjian.zhou@mediatek.com>
>
> This patch enables support SDIO IRQs. It enables
> MMC_CAP_SDIO_IRQ & MMC_CAP2_SDIO_IRQ_NOTHREAD
> and implement the ->ack_sdio_irq callback.
>
> Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> ---
> drivers/mmc/host/mtk-sd.c | 49 ++++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 46 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 6334cc7..40a1848 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -1114,6 +1114,7 @@ static void msdc_start_command(struct msdc_host *host,
> struct mmc_request *mrq, struct mmc_command *cmd)
> {
> u32 rawcmd;
> + unsigned long flags;
>
> WARN_ON(host->cmd);
> host->cmd = cmd;
> @@ -1131,7 +1132,10 @@ static void msdc_start_command(struct msdc_host *host,
> cmd->error = 0;
> rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
>
> + spin_lock_irqsave(&host->lock, flags);
> sdr_set_bits(host->base + MSDC_INTEN, cmd_ints_mask);
> + spin_unlock_irqrestore(&host->lock, flags);
> +
The above looks like a reasonable change, however does it really
belong in $subject patch. If so, can you explain why?
Maybe you should move this into a standalone separate patch?
> writel(cmd->arg, host->base + SDC_ARG);
> writel(rawcmd, host->base + SDC_CMD);
> }
> @@ -1351,6 +1355,27 @@ static void msdc_request_timeout(struct work_struct *work)
> }
> }
>
> +static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
> +{
> + unsigned long flags;
> + struct msdc_host *host = mmc_priv(mmc);
> +
> + if (enb)
> + pm_runtime_get_sync(host->dev);
I would convert to pm_runtime_get_noresume() as the host is already
been runtime resumed by the mmc core.
> +
> + spin_lock_irqsave(&host->lock, flags);
> + if (enb)
> + sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> + else
> + sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> + spin_unlock_irqrestore(&host->lock, flags);
You should move the code above into a separate function. Along the lines of:
static void __msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
You need this, because you will otherwise mess up the runtime PM usage
count when acking the SDIO irq. More comments about that below.
> +
> + if (!enb) {
> + pm_runtime_mark_last_busy(host->dev);
> + pm_runtime_put_autosuspend(host->dev);
Convert to pm_runtime_put_noidle(), as the above is already managed by
the mmc core.
> + }
> +}
> +
> static irqreturn_t msdc_irq(int irq, void *dev_id)
> {
> struct msdc_host *host = (struct msdc_host *) dev_id;
> @@ -1373,7 +1398,12 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
> data = host->data;
> spin_unlock_irqrestore(&host->lock, flags);
>
> - if (!(events & event_mask))
> + if ((events & event_mask) & MSDC_INT_SDIOIRQ) {
> + msdc_enable_sdio_irq(host->mmc, 0);
To not mess up the runtime PM reference count and to avoid invoking
runtime PM callbacks from atomic context, I suggest you call
__msdc_enable_sdio_irq(host->mmc, 0) (new proposed function according
to earlier comment).
> + sdio_signal_irq(host->mmc);
> + }
> +
> + if (!(events & (event_mask & ~MSDC_INT_SDIOIRQ)))
> break;
>
> if (!mrq) {
> @@ -1493,8 +1523,11 @@ static void msdc_init_hw(struct msdc_host *host)
> */
> sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
>
> - /* disable detect SDIO device interrupt function */
> - sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> + /* Config SDIO device detect interrupt function */
> + if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
> + sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> + else
> + sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
>
> /* Configure to default data timeout */
> sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
> @@ -2013,6 +2046,11 @@ static void msdc_hw_reset(struct mmc_host *mmc)
> sdr_clr_bits(host->base + EMMC_IOCON, 1);
> }
>
> +static void msdc_ack_sdio_irq(struct mmc_host *mmc)
> +{
> + msdc_enable_sdio_irq(mmc, 1);
To not mess up the runtime PM reference count, I suggest you call
__msdc_enable_sdio_irq(host->mmc, 1) (new proposed function according
to earlier comment).
> +}
> +
> static const struct mmc_host_ops mt_msdc_ops = {
> .post_req = msdc_post_req,
> .pre_req = msdc_pre_req,
> @@ -2020,6 +2058,8 @@ static void msdc_hw_reset(struct mmc_host *mmc)
> .set_ios = msdc_ops_set_ios,
> .get_ro = mmc_gpio_get_ro,
> .get_cd = mmc_gpio_get_cd,
> + .enable_sdio_irq = msdc_enable_sdio_irq,
> + .ack_sdio_irq = msdc_ack_sdio_irq,
> .start_signal_voltage_switch = msdc_ops_switch_volt,
> .card_busy = msdc_card_busy,
> .execute_tuning = msdc_execute_tuning,
> @@ -2147,6 +2187,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> else
> mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 4095);
>
> + if (mmc->caps & MMC_CAP_SDIO_IRQ)
> + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
> +
> mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> /* MMC core transfer sizes tunable parameters */
> mmc->max_segs = MAX_BD_NUM;
> --
> 1.9.1
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v3] mmc: mediatek: Add MMC_CAP_SDIO_IRQ support
2018-12-11 9:29 ` Ulf Hansson
@ 2018-12-11 12:11 ` Jjian Zhou
0 siblings, 0 replies; 3+ messages in thread
From: Jjian Zhou @ 2018-12-11 12:11 UTC (permalink / raw)
To: Ulf Hansson
Cc: Matthias Brugger, Chaotian Jing (井朝天),
Ryder Lee (李庚諺),
Sean Wang, linux-mmc, Linux ARM, linux-mediatek, srv_heupstream,
Linux Kernel Mailing List, Yong Mao (毛勇)
On Tue, 2018-12-11 at 17:29 +0800, Ulf Hansson wrote:
> On Mon, 10 Dec 2018 at 12:44, Jjian Zhou <jjian.zhou@mediatek.com> wrote:
> >
> > From: jjian zhou <jjian.zhou@mediatek.com>
> >
> > This patch enables support SDIO IRQs. It enables
> > MMC_CAP_SDIO_IRQ & MMC_CAP2_SDIO_IRQ_NOTHREAD
> > and implement the ->ack_sdio_irq callback.
> >
> > Signed-off-by: Jjian Zhou <jjian.zhou@mediatek.com>
> > Signed-off-by: Chaotian Jing <chaotian.jing@mediatek.com>
> > Signed-off-by: Yong Mao <yong.mao@mediatek.com>
> > ---
> > drivers/mmc/host/mtk-sd.c | 49 ++++++++++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 46 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 6334cc7..40a1848 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -1114,6 +1114,7 @@ static void msdc_start_command(struct msdc_host *host,
> > struct mmc_request *mrq, struct mmc_command *cmd)
> > {
> > u32 rawcmd;
> > + unsigned long flags;
> >
> > WARN_ON(host->cmd);
> > host->cmd = cmd;
> > @@ -1131,7 +1132,10 @@ static void msdc_start_command(struct msdc_host *host,
> > cmd->error = 0;
> > rawcmd = msdc_cmd_prepare_raw_cmd(host, mrq, cmd);
> >
> > + spin_lock_irqsave(&host->lock, flags);
> > sdr_set_bits(host->base + MSDC_INTEN, cmd_ints_mask);
> > + spin_unlock_irqrestore(&host->lock, flags);
> > +
>
> The above looks like a reasonable change, however does it really
> belong in $subject patch. If so, can you explain why?
>
> Maybe you should move this into a standalone separate patch?
>
Thanks for your suggestion.
The modification of MSDC_INTEN is non-atomic operation. If it enanbles
MMC_CAP_SDIO_IRQ, there is a race condition of access MSDC_INTEN in irq
and thread context. So need make modification of MSDC_INTEN by atomic.
> > writel(cmd->arg, host->base + SDC_ARG);
> > writel(rawcmd, host->base + SDC_CMD);
> > }
> > @@ -1351,6 +1355,27 @@ static void msdc_request_timeout(struct work_struct *work)
> > }
> > }
> >
> > +static void msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
> > +{
> > + unsigned long flags;
> > + struct msdc_host *host = mmc_priv(mmc);
> > +
> > + if (enb)
> > + pm_runtime_get_sync(host->dev);
>
> I would convert to pm_runtime_get_noresume() as the host is already
> been runtime resumed by the mmc core.
will do it in next version.
> > +
> > + spin_lock_irqsave(&host->lock, flags);
> > + if (enb)
> > + sdr_set_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> > + else
> > + sdr_clr_bits(host->base + MSDC_INTEN, MSDC_INTEN_SDIOIRQ);
> > + spin_unlock_irqrestore(&host->lock, flags);
>
> You should move the code above into a separate function. Along the lines of:
>
> static void __msdc_enable_sdio_irq(struct mmc_host *mmc, int enb)
>
> You need this, because you will otherwise mess up the runtime PM usage
> count when acking the SDIO irq. More comments about that below.
will do it in next version.
> > +
> > + if (!enb) {
> > + pm_runtime_mark_lastwill do it in next version._busy(host->dev);
> > + pm_runtime_put_autosuspend(host->dev);
>
> Convert to pm_runtime_put_noidle(), as the above is already managed by
> the mmc core.
>
will do it in next version.
> > + }
> > +}
> > +
> > static irqreturn_t msdc_irq(int irq, void *dev_id)
> > {
> > struct msdc_host *host = (struct msdc_host *) dev_id;
> > @@ -1373,7 +1398,12 @@ static irqreturn_t msdc_irq(int irq, void *dev_id)
> > data = host->data;
> > spin_unlock_irqrestore(&host->lock, flags);
> >
> > - if (!(events & event_mask))
> > + if ((events & event_mask) & MSDC_INT_SDIOIRQ) {
> > + msdc_enable_sdio_irq(host->mmc, 0);
>
> To not mess up the runtime PM reference count and to avoid invoking
> runtime PM callbacks from atomic context, I suggest you call
> __msdc_enable_sdio_irq(host->mmc, 0) (new proposed function according
> to earlier comment).
will do it in next version.
>
> > + sdio_signal_irq(host->mmc);
> > + }
> > +
> > + if (!(events & (event_mask & ~MSDC_INT_SDIOIRQ)))
> > break;
> >
> > if (!mrq) {
> > @@ -1493,8 +1523,11 @@ static void msdc_init_hw(struct msdc_host *host)
> > */
> > sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIO);
> >
> > - /* disable detect SDIO device interrupt function */
> > - sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> > + /* Config SDIO device detect interrupt function */
> > + if (host->mmc->caps & MMC_CAP_SDIO_IRQ)
> > + sdr_set_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> > + else
> > + sdr_clr_bits(host->base + SDC_CFG, SDC_CFG_SDIOIDE);
> >
> > /* Configure to default data timeout */
> > sdr_set_field(host->base + SDC_CFG, SDC_CFG_DTOC, 3);
> > @@ -2013,6 +2046,11 @@ static void msdc_hw_reset(struct mmc_host *mmc)
> > sdr_clr_bits(host->base + EMMC_IOCON, 1);
> > }
> >
> > +static void msdc_ack_sdio_irq(struct mmc_host *mmc)
> > +{
> > + msdc_enable_sdio_irq(mmc, 1);
>
> To not mess up the runtime PM reference count, I suggest you call
> __msdc_enable_sdio_irq(host->mmc, 1) (new proposed function according
> to earlier comment).
>
will do it in next version.
> > +}
> > +
> > static const struct mmc_host_ops mt_msdc_ops = {
> > .post_req = msdc_post_req,
> > .pre_req = msdc_pre_req,
> > @@ -2020,6 +2058,8 @@ static void msdc_hw_reset(struct mmc_host *mmc)
> > .set_ios = msdc_ops_set_ios,
> > .get_ro = mmc_gpio_get_ro,
> > .get_cd = mmc_gpio_get_cd,
> > + .enable_sdio_irq = msdc_enable_sdio_irq,
> > + .ack_sdio_irq = msdc_ack_sdio_irq,
> > .start_signal_voltage_switch = msdc_ops_switch_volt,
> > .card_busy = msdc_card_busy,
> > .execute_tuning = msdc_execute_tuning,
> > @@ -2147,6 +2187,9 @@ static int msdc_drv_probe(struct platform_device *pdev)
> > else
> > mmc->f_min = DIV_ROUND_UP(host->src_clk_freq, 4 * 4095);
> >
> > + if (mmc->caps & MMC_CAP_SDIO_IRQ)
> > + mmc->caps2 |= MMC_CAP2_SDIO_IRQ_NOTHREAD;
> > +
> > mmc->caps |= MMC_CAP_ERASE | MMC_CAP_CMD23;
> > /* MMC core transfer sizes tunable parameters */
> > mmc->max_segs = MAX_BD_NUM;
> > --
> > 1.9.1
> >
>
> Kind regards
> Uffe
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2018-12-11 12:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-10 11:43 [PATCH v3] mmc: mediatek: Add MMC_CAP_SDIO_IRQ support Jjian Zhou
2018-12-11 9:29 ` Ulf Hansson
2018-12-11 12:11 ` Jjian Zhou
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).