* [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
@ 2019-11-13 17:25 Ludovic Barre
2019-11-14 15:09 ` Ulf Hansson
2019-12-10 12:30 ` Ulf Hansson
0 siblings, 2 replies; 7+ messages in thread
From: Ludovic Barre @ 2019-11-13 17:25 UTC (permalink / raw)
To: Ulf Hansson, Rob Herring
Cc: devicetree, Alexandre Torgue, linux-mmc, linux-kernel,
srinivas.kandagatla, Ludovic Barre, Maxime Coquelin, linux-stm32,
linux-arm-kernel
From: Ludovic Barre <ludovic.barre@st.com>
If datatimeout occurs on R1B request, the Data Path State Machine stays
in busy and is non-functional. Only a reset aborts the DPSM.
Like a reset must be outside of critical section, this patch adds
threaded irq function to release state machine. In this case,
the mmc_request_done is called at the end of threaded irq and
skipped into irq handler.
Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
---
drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
drivers/mmc/host/mmci.h | 1 +
2 files changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
index 40e72c30ea84..ec6e249c87ca 100644
--- a/drivers/mmc/host/mmci.c
+++ b/drivers/mmc/host/mmci.c
@@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
static void
mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
{
+ if (host->irq_action == IRQ_WAKE_THREAD)
+ return;
+
writel(0, host->base + MMCICOMMAND);
BUG_ON(host->data);
@@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
} else if (host->variant->busy_timeout && busy_resp &&
status & MCI_DATATIMEOUT) {
cmd->error = -ETIMEDOUT;
+ host->irq_action = IRQ_WAKE_THREAD;
} else {
cmd->resp[0] = readl(base + MMCIRESPONSE0);
cmd->resp[1] = readl(base + MMCIRESPONSE1);
@@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
{
struct mmci_host *host = dev_id;
u32 status;
- int ret = 0;
spin_lock(&host->lock);
+ host->irq_action = IRQ_HANDLED;
do {
status = readl(host->base + MMCISTATUS);
@@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
if (host->variant->busy_detect_flag)
status &= ~host->variant->busy_detect_flag;
- ret = 1;
} while (status);
spin_unlock(&host->lock);
- return IRQ_RETVAL(ret);
+ return host->irq_action;
+}
+
+/*
+ * mmci_irq_threaded is call if the mmci host need to release state machines
+ * before to terminate the request.
+ * If datatimeout occurs on R1B request, the Data Path State Machine stays
+ * in busy and is non-functional. Only a reset can to abort the DPSM.
+ */
+static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
+{
+ struct mmci_host *host = dev_id;
+ unsigned long flags;
+
+ if (host->rst) {
+ reset_control_assert(host->rst);
+ udelay(2);
+ reset_control_deassert(host->rst);
+ }
+
+ spin_lock_irqsave(&host->lock, flags);
+ writel(host->clk_reg, host->base + MMCICLOCK);
+ writel(host->pwr_reg, host->base + MMCIPOWER);
+ writel(MCI_IRQENABLE | host->variant->start_err,
+ host->base + MMCIMASK0);
+
+ host->irq_action = IRQ_HANDLED;
+ mmci_request_end(host, host->mrq);
+ spin_unlock_irqrestore(&host->lock, flags);
+
+ return host->irq_action;
}
static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
@@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
goto clk_disable;
}
- ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
- DRIVER_NAME " (cmd)", host);
+ ret = devm_request_threaded_irq(&dev->dev, dev->irq[0], mmci_irq,
+ mmci_irq_threaded, IRQF_SHARED,
+ DRIVER_NAME " (cmd)", host);
if (ret)
goto clk_disable;
diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
index 158e1231aa23..5e63c0596364 100644
--- a/drivers/mmc/host/mmci.h
+++ b/drivers/mmc/host/mmci.h
@@ -412,6 +412,7 @@ struct mmci_host {
struct timer_list timer;
unsigned int oldstat;
+ u32 irq_action;
/* pio stuff */
struct sg_mapping_iter sg_miter;
--
2.17.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-11-13 17:25 [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state Ludovic Barre
@ 2019-11-14 15:09 ` Ulf Hansson
2019-11-15 14:59 ` Ludovic BARRE
2019-12-10 12:30 ` Ulf Hansson
1 sibling, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2019-11-14 15:09 UTC (permalink / raw)
To: Ludovic Barre
Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin,
Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML,
linux-mmc, linux-stm32
On Wed, 13 Nov 2019 at 18:25, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> If datatimeout occurs on R1B request, the Data Path State Machine stays
> in busy and is non-functional. Only a reset aborts the DPSM.
Please clarify/extend this information to tell that this is for the
variant, that keeps DPSM enabled and uses the data timer while sending
a CMD12. Or something along those lines.
>
> Like a reset must be outside of critical section, this patch adds
/s/critical section/atomic context
> threaded irq function to release state machine. In this case,
> the mmc_request_done is called at the end of threaded irq and
> skipped into irq handler.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
> drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
> drivers/mmc/host/mmci.h | 1 +
> 2 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 40e72c30ea84..ec6e249c87ca 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
> static void
> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
> {
> + if (host->irq_action == IRQ_WAKE_THREAD)
> + return;
> +
> writel(0, host->base + MMCICOMMAND);
>
> BUG_ON(host->data);
> @@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> } else if (host->variant->busy_timeout && busy_resp &&
> status & MCI_DATATIMEOUT) {
> cmd->error = -ETIMEDOUT;
> + host->irq_action = IRQ_WAKE_THREAD;
> } else {
> cmd->resp[0] = readl(base + MMCIRESPONSE0);
> cmd->resp[1] = readl(base + MMCIRESPONSE1);
> @@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> {
> struct mmci_host *host = dev_id;
> u32 status;
> - int ret = 0;
>
> spin_lock(&host->lock);
> + host->irq_action = IRQ_HANDLED;
>
> do {
> status = readl(host->base + MMCISTATUS);
> @@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> if (host->variant->busy_detect_flag)
> status &= ~host->variant->busy_detect_flag;
>
> - ret = 1;
> } while (status);
>
> spin_unlock(&host->lock);
>
> - return IRQ_RETVAL(ret);
> + return host->irq_action;
> +}
> +
> +/*
> + * mmci_irq_threaded is call if the mmci host need to release state machines
> + * before to terminate the request.
> + * If datatimeout occurs on R1B request, the Data Path State Machine stays
> + * in busy and is non-functional. Only a reset can to abort the DPSM.
> + */
> +static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
> +{
> + struct mmci_host *host = dev_id;
> + unsigned long flags;
> +
> + if (host->rst) {
> + reset_control_assert(host->rst);
> + udelay(2);
> + reset_control_deassert(host->rst);
> + }
> +
> + spin_lock_irqsave(&host->lock, flags);
> + writel(host->clk_reg, host->base + MMCICLOCK);
> + writel(host->pwr_reg, host->base + MMCIPOWER);
> + writel(MCI_IRQENABLE | host->variant->start_err,
> + host->base + MMCIMASK0);
> +
> + host->irq_action = IRQ_HANDLED;
> + mmci_request_end(host, host->mrq);
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return host->irq_action;
> }
>
> static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> @@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
> goto clk_disable;
> }
>
> - ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
> - DRIVER_NAME " (cmd)", host);
> + ret = devm_request_threaded_irq(&dev->dev, dev->irq[0], mmci_irq,
> + mmci_irq_threaded, IRQF_SHARED,
> + DRIVER_NAME " (cmd)", host);
In general it's a good idea to move drivers into using a threaded IRQ handler.
However, the reason this hasn't been done for mmci before, is because
there are some legacy variants, that doesn't support HW flow control.
Unless I am mistaken, that means when the fifo gets full during data
transfers - it's too late to act. In other words, running the handler
in hard IRQ context, should increase the probability of not missing
the deadline.
If a threaded IRQ handler also is sufficient for these legacy
variants, only tests can tell.
An option, would be to use a threaded handler for those variants that
supports HW flow control. Not sure how messy the code would be with
this option, perhaps you can give this a try?
> if (ret)
> goto clk_disable;
>
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 158e1231aa23..5e63c0596364 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -412,6 +412,7 @@ struct mmci_host {
>
> struct timer_list timer;
> unsigned int oldstat;
> + u32 irq_action;
>
> /* pio stuff */
> struct sg_mapping_iter sg_miter;
> --
> 2.17.1
>
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-11-14 15:09 ` Ulf Hansson
@ 2019-11-15 14:59 ` Ludovic BARRE
2019-11-28 14:06 ` [Linux-stm32] " Ludovic BARRE
0 siblings, 1 reply; 7+ messages in thread
From: Ludovic BARRE @ 2019-11-15 14:59 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin,
Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML,
linux-mmc, linux-stm32
hi Ulf
Le 11/14/19 à 4:09 PM, Ulf Hansson a écrit :
> On Wed, 13 Nov 2019 at 18:25, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> If datatimeout occurs on R1B request, the Data Path State Machine stays
>> in busy and is non-functional. Only a reset aborts the DPSM.
>
> Please clarify/extend this information to tell that this is for the
> variant, that keeps DPSM enabled and uses the data timer while sending
> a CMD12. Or something along those lines.
>
yes, of course.
>>
>> Like a reset must be outside of critical section, this patch adds
>
> /s/critical section/atomic context
OK, thanks
>
>> threaded irq function to release state machine. In this case,
>> the mmc_request_done is called at the end of threaded irq and
>> skipped into irq handler.
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> ---
>> drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
>> drivers/mmc/host/mmci.h | 1 +
>> 2 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 40e72c30ea84..ec6e249c87ca 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
>> static void
>> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
>> {
>> + if (host->irq_action == IRQ_WAKE_THREAD)
>> + return;
>> +
>> writel(0, host->base + MMCICOMMAND);
>>
>> BUG_ON(host->data);
>> @@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>> } else if (host->variant->busy_timeout && busy_resp &&
>> status & MCI_DATATIMEOUT) {
>> cmd->error = -ETIMEDOUT;
>> + host->irq_action = IRQ_WAKE_THREAD;
>> } else {
>> cmd->resp[0] = readl(base + MMCIRESPONSE0);
>> cmd->resp[1] = readl(base + MMCIRESPONSE1);
>> @@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>> {
>> struct mmci_host *host = dev_id;
>> u32 status;
>> - int ret = 0;
>>
>> spin_lock(&host->lock);
>> + host->irq_action = IRQ_HANDLED;
>>
>> do {
>> status = readl(host->base + MMCISTATUS);
>> @@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>> if (host->variant->busy_detect_flag)
>> status &= ~host->variant->busy_detect_flag;
>>
>> - ret = 1;
>> } while (status);
>>
>> spin_unlock(&host->lock);
>>
>> - return IRQ_RETVAL(ret);
>> + return host->irq_action;
>> +}
>> +
>> +/*
>> + * mmci_irq_threaded is call if the mmci host need to release state machines
>> + * before to terminate the request.
>> + * If datatimeout occurs on R1B request, the Data Path State Machine stays
>> + * in busy and is non-functional. Only a reset can to abort the DPSM.
>> + */
>> +static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
>> +{
>> + struct mmci_host *host = dev_id;
>> + unsigned long flags;
>> +
>> + if (host->rst) {
>> + reset_control_assert(host->rst);
>> + udelay(2);
>> + reset_control_deassert(host->rst);
>> + }
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + writel(host->clk_reg, host->base + MMCICLOCK);
>> + writel(host->pwr_reg, host->base + MMCIPOWER);
>> + writel(MCI_IRQENABLE | host->variant->start_err,
>> + host->base + MMCIMASK0);
>> +
>> + host->irq_action = IRQ_HANDLED;
>> + mmci_request_end(host, host->mrq);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + return host->irq_action;
>> }
>>
>> static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> @@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
>> goto clk_disable;
>> }
>>
>> - ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
>> - DRIVER_NAME " (cmd)", host);
>> + ret = devm_request_threaded_irq(&dev->dev, dev->irq[0], mmci_irq,
>> + mmci_irq_threaded, IRQF_SHARED,
>> + DRIVER_NAME " (cmd)", host);
>
> In general it's a good idea to move drivers into using a threaded IRQ handler.
>
> However, the reason this hasn't been done for mmci before, is because
> there are some legacy variants, that doesn't support HW flow control.
With this patch, the mmci_irq_threaded is called only when
a datatimeout occurs on R1B request else the mmci_irq return IRQ_HANDLED
and the threaded irq is not called.
So normally, there is no impact for legacy variants.
>
> Unless I am mistaken, that means when the fifo gets full during data
> transfers - it's too late to act. In other words, running the handler
> in hard IRQ context, should increase the probability of not missing
> the deadline.
>
> If a threaded IRQ handler also is sufficient for these legacy
> variants, only tests can tell.
>
> An option, would be to use a threaded handler for those variants that
> supports HW flow control. Not sure how messy the code would be with
> this option, perhaps you can give this a try?
For all these reasons you mentioned above, I'm not sure it's safe to
extend the thread manager to anything other than "aborting the DPSM" on
R1B datatiemout ? what do you think about that?
I prefer just used the irq threaded for specific errors case which need
be outside of atomic context.
>
>
>> if (ret)
>> goto clk_disable;
>>
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index 158e1231aa23..5e63c0596364 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -412,6 +412,7 @@ struct mmci_host {
>>
>> struct timer_list timer;
>> unsigned int oldstat;
>> + u32 irq_action;
>>
>> /* pio stuff */
>> struct sg_mapping_iter sg_miter;
>> --
>> 2.17.1
>>
>
> Kind regards
> Uffe
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-stm32] [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-11-15 14:59 ` Ludovic BARRE
@ 2019-11-28 14:06 ` Ludovic BARRE
2019-12-10 12:20 ` Ulf Hansson
0 siblings, 1 reply; 7+ messages in thread
From: Ludovic BARRE @ 2019-11-28 14:06 UTC (permalink / raw)
To: Ulf Hansson
Cc: DTML, linux-mmc, Linux Kernel Mailing List, Rob Herring,
Srinivas Kandagatla, Maxime Coquelin, linux-stm32, Linux ARM
hi Ulf
just a gentleman ping about this thread.
small summarize:
This patch return an IRQ_WAKE_THREAD only when the variant is
busy_timeout capable and a datatimeout occurs on R1B request.
So the threaded irq is called only to treat this specific error.
Normally, there is no impact on HW flow control or for legacy variants.
In your previous message, you seem to suggest using threaded irq to
manage HW flow control (pio mode). But Like you mention below, the mmci
legacy could timing sensitive.
For the moment, I prefer to use the threaded irq just to manage this
error. If needed, the irq threade could be extended later.
What do you think about that?
Kind regards
Ludo
Le 11/15/19 à 3:59 PM, Ludovic BARRE a écrit :
> hi Ulf
>
> Le 11/14/19 à 4:09 PM, Ulf Hansson a écrit :
>> On Wed, 13 Nov 2019 at 18:25, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>>
>>> From: Ludovic Barre <ludovic.barre@st.com>
>>>
>>> If datatimeout occurs on R1B request, the Data Path State Machine stays
>>> in busy and is non-functional. Only a reset aborts the DPSM.
>>
>> Please clarify/extend this information to tell that this is for the
>> variant, that keeps DPSM enabled and uses the data timer while sending
>> a CMD12. Or something along those lines.
>>
>
> yes, of course.
>
>>>
>>> Like a reset must be outside of critical section, this patch adds
>>
>> /s/critical section/atomic context
>
> OK, thanks
>
>>
>>> threaded irq function to release state machine. In this case,
>>> the mmc_request_done is called at the end of threaded irq and
>>> skipped into irq handler.
>>>
>>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>>> ---
>>> drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
>>> drivers/mmc/host/mmci.h | 1 +
>>> 2 files changed, 40 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>>> index 40e72c30ea84..ec6e249c87ca 100644
>>> --- a/drivers/mmc/host/mmci.c
>>> +++ b/drivers/mmc/host/mmci.c
>>> @@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
>>> static void
>>> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
>>> {
>>> + if (host->irq_action == IRQ_WAKE_THREAD)
>>> + return;
>>> +
>>> writel(0, host->base + MMCICOMMAND);
>>>
>>> BUG_ON(host->data);
>>> @@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct
>>> mmc_command *cmd,
>>> } else if (host->variant->busy_timeout && busy_resp &&
>>> status & MCI_DATATIMEOUT) {
>>> cmd->error = -ETIMEDOUT;
>>> + host->irq_action = IRQ_WAKE_THREAD;
>>> } else {
>>> cmd->resp[0] = readl(base + MMCIRESPONSE0);
>>> cmd->resp[1] = readl(base + MMCIRESPONSE1);
>>> @@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>>> {
>>> struct mmci_host *host = dev_id;
>>> u32 status;
>>> - int ret = 0;
>>>
>>> spin_lock(&host->lock);
>>> + host->irq_action = IRQ_HANDLED;
>>>
>>> do {
>>> status = readl(host->base + MMCISTATUS);
>>> @@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void
>>> *dev_id)
>>> if (host->variant->busy_detect_flag)
>>> status &= ~host->variant->busy_detect_flag;
>>>
>>> - ret = 1;
>>> } while (status);
>>>
>>> spin_unlock(&host->lock);
>>>
>>> - return IRQ_RETVAL(ret);
>>> + return host->irq_action;
>>> +}
>>> +
>>> +/*
>>> + * mmci_irq_threaded is call if the mmci host need to release state
>>> machines
>>> + * before to terminate the request.
>>> + * If datatimeout occurs on R1B request, the Data Path State Machine
>>> stays
>>> + * in busy and is non-functional. Only a reset can to abort the DPSM.
>>> + */
>>> +static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
>>> +{
>>> + struct mmci_host *host = dev_id;
>>> + unsigned long flags;
>>> +
>>> + if (host->rst) {
>>> + reset_control_assert(host->rst);
>>> + udelay(2);
>>> + reset_control_deassert(host->rst);
>>> + }
>>> +
>>> + spin_lock_irqsave(&host->lock, flags);
>>> + writel(host->clk_reg, host->base + MMCICLOCK);
>>> + writel(host->pwr_reg, host->base + MMCIPOWER);
>>> + writel(MCI_IRQENABLE | host->variant->start_err,
>>> + host->base + MMCIMASK0);
>>> +
>>> + host->irq_action = IRQ_HANDLED;
>>> + mmci_request_end(host, host->mrq);
>>> + spin_unlock_irqrestore(&host->lock, flags);
>>> +
>>> + return host->irq_action;
>>> }
>>>
>>> static void mmci_request(struct mmc_host *mmc, struct mmc_request
>>> *mrq)
>>> @@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
>>> goto clk_disable;
>>> }
>>>
>>> - ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq,
>>> IRQF_SHARED,
>>> - DRIVER_NAME " (cmd)", host);
>>> + ret = devm_request_threaded_irq(&dev->dev, dev->irq[0],
>>> mmci_irq,
>>> + mmci_irq_threaded, IRQF_SHARED,
>>> + DRIVER_NAME " (cmd)", host);
>>
>> In general it's a good idea to move drivers into using a threaded IRQ
>> handler.
>>
>> However, the reason this hasn't been done for mmci before, is because
>> there are some legacy variants, that doesn't support HW flow control.
>
> With this patch, the mmci_irq_threaded is called only when
> a datatimeout occurs on R1B request else the mmci_irq return IRQ_HANDLED
> and the threaded irq is not called.
> So normally, there is no impact for legacy variants.
>
>>
>> Unless I am mistaken, that means when the fifo gets full during data
>> transfers - it's too late to act. In other words, running the handler
>> in hard IRQ context, should increase the probability of not missing
>> the deadline.
>>
>> If a threaded IRQ handler also is sufficient for these legacy
>> variants, only tests can tell.
>>
>> An option, would be to use a threaded handler for those variants that
>> supports HW flow control. Not sure how messy the code would be with
>> this option, perhaps you can give this a try?
> For all these reasons you mentioned above, I'm not sure it's safe to
> extend the thread manager to anything other than "aborting the DPSM" on
> R1B datatiemout ? what do you think about that?
>
> I prefer just used the irq threaded for specific errors case which need
> be outside of atomic context.
>
>>
>>
>>> if (ret)
>>> goto clk_disable;
>>>
>>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>>> index 158e1231aa23..5e63c0596364 100644
>>> --- a/drivers/mmc/host/mmci.h
>>> +++ b/drivers/mmc/host/mmci.h
>>> @@ -412,6 +412,7 @@ struct mmci_host {
>>>
>>> struct timer_list timer;
>>> unsigned int oldstat;
>>> + u32 irq_action;
>>>
>>> /* pio stuff */
>>> struct sg_mapping_iter sg_miter;
>>> --
>>> 2.17.1
>>>
>>
>> Kind regards
>> Uffe
>>
> _______________________________________________
> Linux-stm32 mailing list
> Linux-stm32@st-md-mailman.stormreply.com
> https://st-md-mailman.stormreply.com/mailman/listinfo/linux-stm32
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Linux-stm32] [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-11-28 14:06 ` [Linux-stm32] " Ludovic BARRE
@ 2019-12-10 12:20 ` Ulf Hansson
0 siblings, 0 replies; 7+ messages in thread
From: Ulf Hansson @ 2019-12-10 12:20 UTC (permalink / raw)
To: Ludovic BARRE
Cc: DTML, linux-mmc, Linux Kernel Mailing List, Rob Herring,
Srinivas Kandagatla, Maxime Coquelin, linux-stm32, Linux ARM
Hi Ludovic,
On Thu, 28 Nov 2019 at 15:06, Ludovic BARRE <ludovic.barre@st.com> wrote:
>
> hi Ulf
>
> just a gentleman ping about this thread.
>
> small summarize:
> This patch return an IRQ_WAKE_THREAD only when the variant is
> busy_timeout capable and a datatimeout occurs on R1B request.
>
> So the threaded irq is called only to treat this specific error.
> Normally, there is no impact on HW flow control or for legacy variants.
Yes, this should work.
>
> In your previous message, you seem to suggest using threaded irq to
> manage HW flow control (pio mode). But Like you mention below, the mmci
> legacy could timing sensitive.
>
> For the moment, I prefer to use the threaded irq just to manage this
> error. If needed, the irq threade could be extended later.
>
> What do you think about that?
Yes, that's fine!
I have another minor comment on the code, though, but posting that separately.
[...]
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-11-13 17:25 [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state Ludovic Barre
2019-11-14 15:09 ` Ulf Hansson
@ 2019-12-10 12:30 ` Ulf Hansson
2019-12-10 14:03 ` Ludovic BARRE
1 sibling, 1 reply; 7+ messages in thread
From: Ulf Hansson @ 2019-12-10 12:30 UTC (permalink / raw)
To: Ludovic Barre
Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin,
Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML,
linux-mmc, linux-stm32
On Wed, 13 Nov 2019 at 18:25, Ludovic Barre <ludovic.Barre@st.com> wrote:
>
> From: Ludovic Barre <ludovic.barre@st.com>
>
> If datatimeout occurs on R1B request, the Data Path State Machine stays
> in busy and is non-functional. Only a reset aborts the DPSM.
>
> Like a reset must be outside of critical section, this patch adds
> threaded irq function to release state machine. In this case,
> the mmc_request_done is called at the end of threaded irq and
> skipped into irq handler.
>
> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
> ---
> drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
> drivers/mmc/host/mmci.h | 1 +
> 2 files changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
> index 40e72c30ea84..ec6e249c87ca 100644
> --- a/drivers/mmc/host/mmci.c
> +++ b/drivers/mmc/host/mmci.c
> @@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
> static void
> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
> {
> + if (host->irq_action == IRQ_WAKE_THREAD)
> + return;
> +
It seems a bit unnecessary to check this every time mmci_request_end()
is called.
How about avoiding to call mmci_request_end() for the one specific
condition instead? See more below.
> writel(0, host->base + MMCICOMMAND);
>
> BUG_ON(host->data);
> @@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
> } else if (host->variant->busy_timeout && busy_resp &&
> status & MCI_DATATIMEOUT) {
> cmd->error = -ETIMEDOUT;
> + host->irq_action = IRQ_WAKE_THREAD;
You could check this flag a few lines below and if it's set to
IRQ_WAKE_THREAD, avoid to call mmci_request_end().
> } else {
> cmd->resp[0] = readl(base + MMCIRESPONSE0);
> cmd->resp[1] = readl(base + MMCIRESPONSE1);
> @@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> {
> struct mmci_host *host = dev_id;
> u32 status;
> - int ret = 0;
>
> spin_lock(&host->lock);
> + host->irq_action = IRQ_HANDLED;
>
> do {
> status = readl(host->base + MMCISTATUS);
> @@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
> if (host->variant->busy_detect_flag)
> status &= ~host->variant->busy_detect_flag;
>
> - ret = 1;
> } while (status);
>
> spin_unlock(&host->lock);
>
> - return IRQ_RETVAL(ret);
> + return host->irq_action;
> +}
> +
> +/*
> + * mmci_irq_threaded is call if the mmci host need to release state machines
> + * before to terminate the request.
> + * If datatimeout occurs on R1B request, the Data Path State Machine stays
> + * in busy and is non-functional. Only a reset can to abort the DPSM.
> + */
> +static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
> +{
> + struct mmci_host *host = dev_id;
> + unsigned long flags;
> +
> + if (host->rst) {
> + reset_control_assert(host->rst);
> + udelay(2);
> + reset_control_deassert(host->rst);
> + }
> +
> + spin_lock_irqsave(&host->lock, flags);
> + writel(host->clk_reg, host->base + MMCICLOCK);
> + writel(host->pwr_reg, host->base + MMCIPOWER);
> + writel(MCI_IRQENABLE | host->variant->start_err,
> + host->base + MMCIMASK0);
> +
> + host->irq_action = IRQ_HANDLED;
> + mmci_request_end(host, host->mrq);
> + spin_unlock_irqrestore(&host->lock, flags);
> +
> + return host->irq_action;
> }
>
> static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
> @@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
> goto clk_disable;
> }
>
> - ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
> - DRIVER_NAME " (cmd)", host);
> + ret = devm_request_threaded_irq(&dev->dev, dev->irq[0], mmci_irq,
> + mmci_irq_threaded, IRQF_SHARED,
> + DRIVER_NAME " (cmd)", host);
> if (ret)
> goto clk_disable;
>
> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
> index 158e1231aa23..5e63c0596364 100644
> --- a/drivers/mmc/host/mmci.h
> +++ b/drivers/mmc/host/mmci.h
> @@ -412,6 +412,7 @@ struct mmci_host {
>
> struct timer_list timer;
> unsigned int oldstat;
> + u32 irq_action;
>
> /* pio stuff */
> struct sg_mapping_iter sg_miter;
> --
> 2.17.1
>
Otherwise this looks good, besides my other earlier comments, of course.
Kind regards
Uffe
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state
2019-12-10 12:30 ` Ulf Hansson
@ 2019-12-10 14:03 ` Ludovic BARRE
0 siblings, 0 replies; 7+ messages in thread
From: Ludovic BARRE @ 2019-12-10 14:03 UTC (permalink / raw)
To: Ulf Hansson
Cc: Rob Herring, Srinivas Kandagatla, Maxime Coquelin,
Alexandre Torgue, Linux ARM, Linux Kernel Mailing List, DTML,
linux-mmc, linux-stm32
hi Ulf
Le 12/10/19 à 1:30 PM, Ulf Hansson a écrit :
> On Wed, 13 Nov 2019 at 18:25, Ludovic Barre <ludovic.Barre@st.com> wrote:
>>
>> From: Ludovic Barre <ludovic.barre@st.com>
>>
>> If datatimeout occurs on R1B request, the Data Path State Machine stays
>> in busy and is non-functional. Only a reset aborts the DPSM.
>>
>> Like a reset must be outside of critical section, this patch adds
>> threaded irq function to release state machine. In this case,
>> the mmc_request_done is called at the end of threaded irq and
>> skipped into irq handler.
>>
>> Signed-off-by: Ludovic Barre <ludovic.barre@st.com>
>> ---
>> drivers/mmc/host/mmci.c | 44 ++++++++++++++++++++++++++++++++++++-----
>> drivers/mmc/host/mmci.h | 1 +
>> 2 files changed, 40 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/mmc/host/mmci.c b/drivers/mmc/host/mmci.c
>> index 40e72c30ea84..ec6e249c87ca 100644
>> --- a/drivers/mmc/host/mmci.c
>> +++ b/drivers/mmc/host/mmci.c
>> @@ -556,6 +556,9 @@ static void mmci_dma_error(struct mmci_host *host)
>> static void
>> mmci_request_end(struct mmci_host *host, struct mmc_request *mrq)
>> {
>> + if (host->irq_action == IRQ_WAKE_THREAD)
>> + return;
>> +
>
> It seems a bit unnecessary to check this every time mmci_request_end()
> is called.
>
> How about avoiding to call mmci_request_end() for the one specific
> condition instead? See more below.
>
>> writel(0, host->base + MMCICOMMAND);
>>
>> BUG_ON(host->data);
>> @@ -1321,6 +1324,7 @@ mmci_cmd_irq(struct mmci_host *host, struct mmc_command *cmd,
>> } else if (host->variant->busy_timeout && busy_resp &&
>> status & MCI_DATATIMEOUT) {
>> cmd->error = -ETIMEDOUT;
>> + host->irq_action = IRQ_WAKE_THREAD;
>
> You could check this flag a few lines below and if it's set to
> IRQ_WAKE_THREAD, avoid to call mmci_request_end().
yes, it was my first implementation. after, I wanted to centralize this,
if the irq threaded would be extend.
But you are right, it's not the goal of this commit.
I resend a v2
>
>> } else {
>> cmd->resp[0] = readl(base + MMCIRESPONSE0);
>> cmd->resp[1] = readl(base + MMCIRESPONSE1);
>> @@ -1532,9 +1536,9 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>> {
>> struct mmci_host *host = dev_id;
>> u32 status;
>> - int ret = 0;
>>
>> spin_lock(&host->lock);
>> + host->irq_action = IRQ_HANDLED;
>>
>> do {
>> status = readl(host->base + MMCISTATUS);
>> @@ -1574,12 +1578,41 @@ static irqreturn_t mmci_irq(int irq, void *dev_id)
>> if (host->variant->busy_detect_flag)
>> status &= ~host->variant->busy_detect_flag;
>>
>> - ret = 1;
>> } while (status);
>>
>> spin_unlock(&host->lock);
>>
>> - return IRQ_RETVAL(ret);
>> + return host->irq_action;
>> +}
>> +
>> +/*
>> + * mmci_irq_threaded is call if the mmci host need to release state machines
>> + * before to terminate the request.
>> + * If datatimeout occurs on R1B request, the Data Path State Machine stays
>> + * in busy and is non-functional. Only a reset can to abort the DPSM.
>> + */
>> +static irqreturn_t mmci_irq_threaded(int irq, void *dev_id)
>> +{
>> + struct mmci_host *host = dev_id;
>> + unsigned long flags;
>> +
>> + if (host->rst) {
>> + reset_control_assert(host->rst);
>> + udelay(2);
>> + reset_control_deassert(host->rst);
>> + }
>> +
>> + spin_lock_irqsave(&host->lock, flags);
>> + writel(host->clk_reg, host->base + MMCICLOCK);
>> + writel(host->pwr_reg, host->base + MMCIPOWER);
>> + writel(MCI_IRQENABLE | host->variant->start_err,
>> + host->base + MMCIMASK0);
>> +
>> + host->irq_action = IRQ_HANDLED;
>> + mmci_request_end(host, host->mrq);
>> + spin_unlock_irqrestore(&host->lock, flags);
>> +
>> + return host->irq_action;
>> }
>>
>> static void mmci_request(struct mmc_host *mmc, struct mmc_request *mrq)
>> @@ -2071,8 +2104,9 @@ static int mmci_probe(struct amba_device *dev,
>> goto clk_disable;
>> }
>>
>> - ret = devm_request_irq(&dev->dev, dev->irq[0], mmci_irq, IRQF_SHARED,
>> - DRIVER_NAME " (cmd)", host);
>> + ret = devm_request_threaded_irq(&dev->dev, dev->irq[0], mmci_irq,
>> + mmci_irq_threaded, IRQF_SHARED,
>> + DRIVER_NAME " (cmd)", host);
>> if (ret)
>> goto clk_disable;
>>
>> diff --git a/drivers/mmc/host/mmci.h b/drivers/mmc/host/mmci.h
>> index 158e1231aa23..5e63c0596364 100644
>> --- a/drivers/mmc/host/mmci.h
>> +++ b/drivers/mmc/host/mmci.h
>> @@ -412,6 +412,7 @@ struct mmci_host {
>>
>> struct timer_list timer;
>> unsigned int oldstat;
>> + u32 irq_action;
>>
>> /* pio stuff */
>> struct sg_mapping_iter sg_miter;
>> --
>> 2.17.1
>>
>
> Otherwise this looks good, besides my other earlier comments, of course.
>
> Kind regards
> Uffe
>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-12-10 14:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-13 17:25 [PATCH 1/1] mmc: mmci: add threaded irq to abort DPSM of non-functional state Ludovic Barre
2019-11-14 15:09 ` Ulf Hansson
2019-11-15 14:59 ` Ludovic BARRE
2019-11-28 14:06 ` [Linux-stm32] " Ludovic BARRE
2019-12-10 12:20 ` Ulf Hansson
2019-12-10 12:30 ` Ulf Hansson
2019-12-10 14:03 ` Ludovic BARRE
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).