[PATCH v2 0/3] Introduce the request_atomic() for the host

[PATCH v2 0/3] Introduce the request_atomic() for the host

[Baolin Wang]

This patch set introduces a new request_atomic() interface for the
MMC host controller, which is used to submit a request to host in
the atomic context, such as in the irq hard handler, to reduce the
request latency.

Any comments are welcome. Thanks.

Note: Adrian pointed out that it is not good if moving the polling of
inhibit bits in sdhci_send_command() into the interrupt context, but
now I have not found a better way to address Adrian's concern. Moveover
this is an unusual abnormal case and the original code has the same
problem, so I plan to create another patch set to talk about and fix
this issue.

Changes from v1:
 - Re-split the changes to make them more clear suggested by Ulf.
 - Factor out the auto CMD23 checking into a separate function.

Baolin Wang (3):
  mmc: host: Introduce the request_atomic() for the host
  mmc: host: sdhci: Implement the request_atomic() API
  mmc: host: sdhci-sprd: Implement the request_atomic() API

 drivers/mmc/host/mmc_hsq.c    |  5 ++++-
 drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
 drivers/mmc/host/sdhci.c      | 27 +++++++++++++++++++--------
 drivers/mmc/host/sdhci.h      |  1 +
 include/linux/mmc/host.h      |  3 +++
 5 files changed, 47 insertions(+), 12 deletions(-)

-- 
1.9.1

[PATCH v2 1/3] mmc: host: Introduce the request_atomic() for the host

[Baolin Wang]

The SD host controller can process one request in the atomic context if
the card is nonremovable, which means we can submit next request in the
irq hard handler when using the MMC host software queue to reduce the
latency. Thus this patch adds a new API request_atomic() for the host
controller, as well as adding support for host software queue to submit
a request by the new request_atomic() API.

Suggested-by: Adrian Hunter <adrian.hunter@intel.com>
Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mmc/host/mmc_hsq.c | 5 ++++-
 include/linux/mmc/host.h   | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/host/mmc_hsq.c b/drivers/mmc/host/mmc_hsq.c
index 59d2776..fdbaa98 100644
--- a/drivers/mmc/host/mmc_hsq.c
+++ b/drivers/mmc/host/mmc_hsq.c
@@ -41,7 +41,10 @@ static void mmc_hsq_pump_requests(struct mmc_hsq *hsq)
 
 	spin_unlock_irqrestore(&hsq->lock, flags);
 
-	mmc->ops->request(mmc, hsq->mrq);
+	if (mmc->ops->request_atomic)
+		mmc->ops->request_atomic(mmc, hsq->mrq);
+	else
+		mmc->ops->request(mmc, hsq->mrq);
 }
 
 static void mmc_hsq_update_next_tag(struct mmc_hsq *hsq, int remains)
diff --git a/include/linux/mmc/host.h b/include/linux/mmc/host.h
index 562ed06..db5e59c 100644
--- a/include/linux/mmc/host.h
+++ b/include/linux/mmc/host.h
@@ -92,6 +92,9 @@ struct mmc_host_ops {
 			    int err);
 	void	(*pre_req)(struct mmc_host *host, struct mmc_request *req);
 	void	(*request)(struct mmc_host *host, struct mmc_request *req);
+	/* Submit one request to host in atomic context. */
+	void	(*request_atomic)(struct mmc_host *host,
+				  struct mmc_request *req);
 
 	/*
 	 * Avoid calling the next three functions too often or in a "fast
-- 
1.9.1

[PATCH v2 2/3] mmc: host: sdhci: Implement the request_atomic() API

[Baolin Wang]

Implement the request_atomic() ops for the sdhci driver to process
one request in the atomic context if the card is nonremovable.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mmc/host/sdhci.c | 27 +++++++++++++++++++--------
 drivers/mmc/host/sdhci.h |  1 +
 2 files changed, 20 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 9c37451..4febbcb 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2016,17 +2016,12 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
  *                                                                           *
 \*****************************************************************************/
 
-void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
+static void sdhci_start_request(struct mmc_host *mmc, struct mmc_request *mrq,
+				int present)
 {
-	struct sdhci_host *host;
-	int present;
+	struct sdhci_host *host = mmc_priv(mmc);
 	unsigned long flags;
 
-	host = mmc_priv(mmc);
-
-	/* Firstly check card presence */
-	present = mmc->ops->get_cd(mmc);
-
 	spin_lock_irqsave(&host->lock, flags);
 
 	sdhci_led_activate(host);
@@ -2043,6 +2038,22 @@ void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
 
 	spin_unlock_irqrestore(&host->lock, flags);
 }
+
+void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	sdhci_start_request(mmc, mrq, 1);
+}
+EXPORT_SYMBOL_GPL(sdhci_request_atomic);
+
+void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	int present;
+
+	/* Firstly check card presence */
+	present = mmc->ops->get_cd(mmc);
+
+	sdhci_start_request(mmc, mrq, present);
+}
 EXPORT_SYMBOL_GPL(sdhci_request);
 
 void sdhci_set_bus_width(struct sdhci_host *host, int width)
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cac2d97..5507a73 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -775,6 +775,7 @@ void sdhci_set_power(struct sdhci_host *host, unsigned char mode,
 void sdhci_set_power_noreg(struct sdhci_host *host, unsigned char mode,
 			   unsigned short vdd);
 void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
+void sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
 void sdhci_set_bus_width(struct sdhci_host *host, int width);
 void sdhci_reset(struct sdhci_host *host, u8 mask);
 void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
-- 
1.9.1

[PATCH v2 3/3] mmc: host: sdhci-sprd: Implement the request_atomic() API

[Baolin Wang]

Implement the request_atomic() API for nonremovable cards, that means
we can submit next request in the irq hard handler context to reduce
latency.

Moreover factor out the AUTO CMD23 checking into a separate function
to reduce duplicate code.

Signed-off-by: Baolin Wang <baolin.wang7@gmail.com>
---
 drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/mmc/host/sdhci-sprd.c b/drivers/mmc/host/sdhci-sprd.c
index 2ab42c5..9a08f70 100644
--- a/drivers/mmc/host/sdhci-sprd.c
+++ b/drivers/mmc/host/sdhci-sprd.c
@@ -406,7 +406,8 @@ static void sdhci_sprd_request_done(struct sdhci_host *host,
 	.request_done = sdhci_sprd_request_done,
 };
 
-static void sdhci_sprd_request(struct mmc_host *mmc, struct mmc_request *mrq)
+static void sdhci_sprd_check_auto_cmd23(struct mmc_host *mmc,
+					struct mmc_request *mrq)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
 	struct sdhci_sprd_host *sprd_host = TO_SPRD_HOST(host);
@@ -422,10 +423,23 @@ static void sdhci_sprd_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	    mrq->sbc && (mrq->sbc->arg & SDHCI_SPRD_ARG2_STUFF) &&
 	    (host->flags & SDHCI_AUTO_CMD23))
 		host->flags &= ~SDHCI_AUTO_CMD23;
+}
+
+static void sdhci_sprd_request(struct mmc_host *mmc, struct mmc_request *mrq)
+{
+	sdhci_sprd_check_auto_cmd23(mmc, mrq);
 
 	sdhci_request(mmc, mrq);
 }
 
+static void sdhci_sprd_request_atomic(struct mmc_host *mmc,
+				      struct mmc_request *mrq)
+{
+	sdhci_sprd_check_auto_cmd23(mmc, mrq);
+
+	sdhci_request_atomic(mmc, mrq);
+}
+
 static int sdhci_sprd_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -561,6 +575,11 @@ static int sdhci_sprd_probe(struct platform_device *pdev)
 	if (ret)
 		goto pltfm_free;
 
+	if (!mmc_card_is_removable(host->mmc))
+		host->mmc_host_ops.request_atomic = sdhci_sprd_request_atomic;
+	else
+		host->always_defer_done = true;
+
 	sprd_host = TO_SPRD_HOST(host);
 	sdhci_sprd_phy_param_parse(sprd_host, pdev->dev.of_node);
 
@@ -654,8 +673,6 @@ static int sdhci_sprd_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_cleanup_host;
 
-	host->always_defer_done = true;
-
 	ret = __sdhci_add_host(host);
 	if (ret)
 		goto err_cleanup_host;
-- 
1.9.1

Re: [PATCH v2 0/3] Introduce the request_atomic() for the host

[Adrian Hunter]

On 17/03/20 12:14 pm, Baolin Wang wrote:
> This patch set introduces a new request_atomic() interface for the
> MMC host controller, which is used to submit a request to host in
> the atomic context, such as in the irq hard handler, to reduce the
> request latency.
> 
> Any comments are welcome. Thanks.
> 
> Note: Adrian pointed out that it is not good if moving the polling of
> inhibit bits in sdhci_send_command() into the interrupt context, but
> now I have not found a better way to address Adrian's concern. Moveover
> this is an unusual abnormal case and the original code has the same
> problem, so I plan to create another patch set to talk about and fix
> this issue.

I tend to think the API requires the possibility for host controllers to
return "busy", so that should be sorted out first.

> 
> Changes from v1:
>  - Re-split the changes to make them more clear suggested by Ulf.
>  - Factor out the auto CMD23 checking into a separate function.
> 
> Baolin Wang (3):
>   mmc: host: Introduce the request_atomic() for the host
>   mmc: host: sdhci: Implement the request_atomic() API
>   mmc: host: sdhci-sprd: Implement the request_atomic() API
> 
>  drivers/mmc/host/mmc_hsq.c    |  5 ++++-
>  drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
>  drivers/mmc/host/sdhci.c      | 27 +++++++++++++++++++--------
>  drivers/mmc/host/sdhci.h      |  1 +
>  include/linux/mmc/host.h      |  3 +++
>  5 files changed, 47 insertions(+), 12 deletions(-)
> 

Re: [PATCH v2 0/3] Introduce the request_atomic() for the host

[Baolin Wang]

On Tue, Mar 17, 2020 at 9:25 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 17/03/20 12:14 pm, Baolin Wang wrote:
> > This patch set introduces a new request_atomic() interface for the
> > MMC host controller, which is used to submit a request to host in
> > the atomic context, such as in the irq hard handler, to reduce the
> > request latency.
> >
> > Any comments are welcome. Thanks.
> >
> > Note: Adrian pointed out that it is not good if moving the polling of
> > inhibit bits in sdhci_send_command() into the interrupt context, but
> > now I have not found a better way to address Adrian's concern. Moveover
> > this is an unusual abnormal case and the original code has the same
> > problem, so I plan to create another patch set to talk about and fix
> > this issue.
>
> I tend to think the API requires the possibility for host controllers to
> return "busy", so that should be sorted out first.

If request_atomic() can return 'busy', the HSQ need queue a work to
dispatch this request to host again?

I am thinking if I can introduce a new flag to avoid polling the
status before sending commands, cause from the datasheet, I did not
see we should need do this if the command complete and transfer
complete interrupts are processed normally. At least on my platfrom, I
did not see the inhibit bits are set. If we meet this issue, I think
some abormal things are happened, we should give out errors. How do
you think?

> >
> > Changes from v1:
> >  - Re-split the changes to make them more clear suggested by Ulf.
> >  - Factor out the auto CMD23 checking into a separate function.
> >
> > Baolin Wang (3):
> >   mmc: host: Introduce the request_atomic() for the host
> >   mmc: host: sdhci: Implement the request_atomic() API
> >   mmc: host: sdhci-sprd: Implement the request_atomic() API
> >
> >  drivers/mmc/host/mmc_hsq.c    |  5 ++++-
> >  drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
> >  drivers/mmc/host/sdhci.c      | 27 +++++++++++++++++++--------
> >  drivers/mmc/host/sdhci.h      |  1 +
> >  include/linux/mmc/host.h      |  3 +++
> >  5 files changed, 47 insertions(+), 12 deletions(-)
> >
>


-- 
Baolin Wang

Re: [PATCH v2 0/3] Introduce the request_atomic() for the host

[Adrian Hunter]

On 17/03/20 3:49 pm, Baolin Wang wrote:
> On Tue, Mar 17, 2020 at 9:25 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>>
>> On 17/03/20 12:14 pm, Baolin Wang wrote:
>>> This patch set introduces a new request_atomic() interface for the
>>> MMC host controller, which is used to submit a request to host in
>>> the atomic context, such as in the irq hard handler, to reduce the
>>> request latency.
>>>
>>> Any comments are welcome. Thanks.
>>>
>>> Note: Adrian pointed out that it is not good if moving the polling of
>>> inhibit bits in sdhci_send_command() into the interrupt context, but
>>> now I have not found a better way to address Adrian's concern. Moveover
>>> this is an unusual abnormal case and the original code has the same
>>> problem, so I plan to create another patch set to talk about and fix
>>> this issue.
>>
>> I tend to think the API requires the possibility for host controllers to
>> return "busy", so that should be sorted out first.
> 
> If request_atomic() can return 'busy', the HSQ need queue a work to
> dispatch this request to host again?

Sounds reasonable

> 
> I am thinking if I can introduce a new flag to avoid polling the
> status before sending commands, cause from the datasheet, I did not
> see we should need do this if the command complete and transfer
> complete interrupts are processed normally. At least on my platfrom, I
> did not see the inhibit bits are set. If we meet this issue, I think
> some abormal things are happened, we should give out errors. How do
> you think?

For the atomic path, some kind of warning would be ok.

> 
>>>
>>> Changes from v1:
>>>  - Re-split the changes to make them more clear suggested by Ulf.
>>>  - Factor out the auto CMD23 checking into a separate function.
>>>
>>> Baolin Wang (3):
>>>   mmc: host: Introduce the request_atomic() for the host
>>>   mmc: host: sdhci: Implement the request_atomic() API
>>>   mmc: host: sdhci-sprd: Implement the request_atomic() API
>>>
>>>  drivers/mmc/host/mmc_hsq.c    |  5 ++++-
>>>  drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
>>>  drivers/mmc/host/sdhci.c      | 27 +++++++++++++++++++--------
>>>  drivers/mmc/host/sdhci.h      |  1 +
>>>  include/linux/mmc/host.h      |  3 +++
>>>  5 files changed, 47 insertions(+), 12 deletions(-)
>>>
>>
> 
> 

Re: [PATCH v2 0/3] Introduce the request_atomic() for the host

[Baolin Wang]

On Tue, Mar 17, 2020 at 11:07 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
>
> On 17/03/20 3:49 pm, Baolin Wang wrote:
> > On Tue, Mar 17, 2020 at 9:25 PM Adrian Hunter <adrian.hunter@intel.com> wrote:
> >>
> >> On 17/03/20 12:14 pm, Baolin Wang wrote:
> >>> This patch set introduces a new request_atomic() interface for the
> >>> MMC host controller, which is used to submit a request to host in
> >>> the atomic context, such as in the irq hard handler, to reduce the
> >>> request latency.
> >>>
> >>> Any comments are welcome. Thanks.
> >>>
> >>> Note: Adrian pointed out that it is not good if moving the polling of
> >>> inhibit bits in sdhci_send_command() into the interrupt context, but
> >>> now I have not found a better way to address Adrian's concern. Moveover
> >>> this is an unusual abnormal case and the original code has the same
> >>> problem, so I plan to create another patch set to talk about and fix
> >>> this issue.
> >>
> >> I tend to think the API requires the possibility for host controllers to
> >> return "busy", so that should be sorted out first.
> >
> > If request_atomic() can return 'busy', the HSQ need queue a work to
> > dispatch this request to host again?
>
> Sounds reasonable
>
> >
> > I am thinking if I can introduce a new flag to avoid polling the
> > status before sending commands, cause from the datasheet, I did not
> > see we should need do this if the command complete and transfer
> > complete interrupts are processed normally. At least on my platfrom, I
> > did not see the inhibit bits are set. If we meet this issue, I think
> > some abormal things are happened, we should give out errors. How do
> > you think?
>
> For the atomic path, some kind of warning would be ok.

OK. I will try in next version. Thanks.

> >
> >>>
> >>> Changes from v1:
> >>>  - Re-split the changes to make them more clear suggested by Ulf.
> >>>  - Factor out the auto CMD23 checking into a separate function.
> >>>
> >>> Baolin Wang (3):
> >>>   mmc: host: Introduce the request_atomic() for the host
> >>>   mmc: host: sdhci: Implement the request_atomic() API
> >>>   mmc: host: sdhci-sprd: Implement the request_atomic() API
> >>>
> >>>  drivers/mmc/host/mmc_hsq.c    |  5 ++++-
> >>>  drivers/mmc/host/sdhci-sprd.c | 23 ++++++++++++++++++++---
> >>>  drivers/mmc/host/sdhci.c      | 27 +++++++++++++++++++--------
> >>>  drivers/mmc/host/sdhci.h      |  1 +
> >>>  include/linux/mmc/host.h      |  3 +++
> >>>  5 files changed, 47 insertions(+), 12 deletions(-)
> >>>
> >>
> >
> >
>


-- 
Baolin Wang