diff mbox series

mmc: host: renesas_internal_dmac: add pre_req and post_req support

Message ID 1607087853-6570-1-git-send-email-yoshihiro.shimoda.uh@renesas.com
State Superseded
Headers show
Series mmc: host: renesas_internal_dmac: add pre_req and post_req support | expand

Commit Message

Yoshihiro Shimoda Dec. 4, 2020, 1:17 p.m. UTC
Add pre_req and post_req support to improve performance.

Inspired by a patch in the BSP by Masaharu Hayakawa.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 When I applied this patch, read performance on R-Car H3 with HS400 mode
 was greatly improved:
 - without the patch : about 200MB/sec
 - with the patch :    about 250MB/sec

 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 88 ++++++++++++++++++++++++---
 1 file changed, 80 insertions(+), 8 deletions(-)

Comments

Wolfram Sang Dec. 14, 2020, 3:50 p.m. UTC | #1
Hi Shimoda-san,

On Fri, Dec 04, 2020 at 10:17:33PM +0900, Yoshihiro Shimoda wrote:
> Add pre_req and post_req support to improve performance.

> 

> Inspired by a patch in the BSP by Masaharu Hayakawa.


Thank you for upporting this!

>  /*

>   * Specification of this driver:

>   * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma

> @@ -172,6 +178,47 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {

>  	tasklet_schedule(&priv->dma_priv.dma_complete);

>  }

>  

> +/* Should not use host->sg_ptr/sg_len in the following function */


Maybe a short explanation why we shouldn't use the functions?

> +static void

> +renesas_sdhi_internal_dmac_unmap(struct tmio_mmc_host *host,

> +				 struct mmc_data *data,

> +				 enum renesas_sdhi_dma_cookie cookie,

> +				 bool expected_unmatch)


Can we maybe skip "expected_unmatch"? It is always true for
COOKIE_UNMAPPED and always false for the COOKIE_*MAPPED values, or?

> +{

> +	bool unmap = expected_unmatch ? (data->host_cookie != cookie) :

> +					(data->host_cookie == cookie);


Then, we could do:
 +	bool unmap = cookie == COOKIE_UNMAPPED ? (data->host_cookie != cookie) :
 +					(data->host_cookie == cookie);

> +

> +	if (unmap) {

> +		dma_unmap_sg(&host->pdev->dev, data->sg, data->sg_len,

> +			     mmc_get_dma_dir(data));

> +		data->host_cookie = COOKIE_UNMAPPED;

> +	}


Is it maybe worth a warning if the expected condition was not found?

Rest looks good!

All the best,

   Wolfram
Yoshihiro Shimoda Dec. 15, 2020, 1:32 a.m. UTC | #2
Hi Wolfram-san,

Thank you for your review!

> From: Wolfram Sang, Sent: Tuesday, December 15, 2020 12:50 AM

> 

> Hi Shimoda-san,

> 

> On Fri, Dec 04, 2020 at 10:17:33PM +0900, Yoshihiro Shimoda wrote:

> > Add pre_req and post_req support to improve performance.

> >

> > Inspired by a patch in the BSP by Masaharu Hayakawa.

> 

> Thank you for upporting this!

> 

> >  /*

> >   * Specification of this driver:

> >   * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma

> > @@ -172,6 +178,47 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {

> >  	tasklet_schedule(&priv->dma_priv.dma_complete);

> >  }

> >

> > +/* Should not use host->sg_ptr/sg_len in the following function */

> 

> Maybe a short explanation why we shouldn't use the functions?


I tried to update the comment as below:
/*
 * tmio_mmc_request() only sets host->sg_{ptr,len} and
 * renesas_sdhi_internal_dmac_pre_req() doesn't set host->sg_{ptr,len} so that
 * we should not use the values in the following function.
 */

Hmm... Perhaps, I should modify the code to use host->sg_{ptr,len}
in both paths (.request() and .pre_req()) and remove this comments.
So, I'll try to modify. I guess tmio_mmc_init_sg() is called in pre_req(),
we can use host->sg_{ptr,len}.

> > +static void

> > +renesas_sdhi_internal_dmac_unmap(struct tmio_mmc_host *host,

> > +				 struct mmc_data *data,

> > +				 enum renesas_sdhi_dma_cookie cookie,

> > +				 bool expected_unmatch)

> 

> Can we maybe skip "expected_unmatch"? It is always true for

> COOKIE_UNMAPPED and always false for the COOKIE_*MAPPED values, or?

> 

> > +{

> > +	bool unmap = expected_unmatch ? (data->host_cookie != cookie) :

> > +					(data->host_cookie == cookie);

> 

> Then, we could do:

>  +	bool unmap = cookie == COOKIE_UNMAPPED ? (data->host_cookie != cookie) :

>  +					(data->host_cookie == cookie);


Thank you for your suggestion! You're correct. I'll fix this.

> > +

> > +	if (unmap) {

> > +		dma_unmap_sg(&host->pdev->dev, data->sg, data->sg_len,

> > +			     mmc_get_dma_dir(data));

> > +		data->host_cookie = COOKIE_UNMAPPED;

> > +	}

> 

> Is it maybe worth a warning if the expected condition was not found?


If we could add such a warning, it's helpful. However,
I have no idea how to implement a condition for it because
we cannot use "else" here. For example, when this driver mapped
as PRE_MAPPED and then renesas_sdhi_internal_dmac_complete() is called,
the "unmap" value of renesas_sdhi_internal_dmac_unmap()
from renesas_sdhi_internal_dmac_complete() is false:

	data->host_cookie = PRE_MAPPED,
  and
	cookie = MAPPED

This is because this driver should unmap in .post_req() so that
this is the expected condition in such a case. But, what do you think?

> Rest looks good!


Thanks!

Best regards,
Yoshihiro Shimoda
Yoshihiro Shimoda Dec. 15, 2020, 3 a.m. UTC | #3
Hi Wolfram-san again,

> From: Yoshihiro Shimoda, Sent: Tuesday, December 15, 2020 10:32 AM

> > From: Wolfram Sang, Sent: Tuesday, December 15, 2020 12:50 AM

> > On Fri, Dec 04, 2020 at 10:17:33PM +0900, Yoshihiro Shimoda wrote:

> > >  /*

> > >   * Specification of this driver:

> > >   * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma

> > > @@ -172,6 +178,47 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {

> > >  	tasklet_schedule(&priv->dma_priv.dma_complete);

> > >  }

> > >

> > > +/* Should not use host->sg_ptr/sg_len in the following function */

> >

> > Maybe a short explanation why we shouldn't use the functions?

> 

> I tried to update the comment as below:

> /*

>  * tmio_mmc_request() only sets host->sg_{ptr,len} and

>  * renesas_sdhi_internal_dmac_pre_req() doesn't set host->sg_{ptr,len} so that

>  * we should not use the values in the following function.

>  */

> 

> Hmm... Perhaps, I should modify the code to use host->sg_{ptr,len}

> in both paths (.request() and .pre_req()) and remove this comments.

> So, I'll try to modify. I guess tmio_mmc_init_sg() is called in pre_req(),

> we can use host->sg_{ptr,len}.


I'm sorry. I was completely wrong. If we use {pre,post}_req,
the MMC core will call pre_req() twice with each mmc_data before
pre_req() is called. So that, second pre_req() will overwrite
the host->sg_ptr. So, we should not use host->sg_ptr here.
So, I'll update the comments like below.

/*
 * Since pre_req() will be called twice before post_req() is called,
 * host->sg_ptr will be overwritten by second pre_req(). So, to use
 * suitable sg pointer, should use data->sg/sg_len instead of
 * host->sg_ptr/sg_len.
 */

Best regards,
Yoshihiro Shimoda
Yoshihiro Shimoda Dec. 15, 2020, 5:03 a.m. UTC | #4
Hi Wolfram-san again,

> From: Yoshihiro Shimoda, Sent: Tuesday, December 15, 2020 12:00 PM

> > From: Yoshihiro Shimoda, Sent: Tuesday, December 15, 2020 10:32 AM

> > > From: Wolfram Sang, Sent: Tuesday, December 15, 2020 12:50 AM

> > > On Fri, Dec 04, 2020 at 10:17:33PM +0900, Yoshihiro Shimoda wrote:

> > > >  /*

> > > >   * Specification of this driver:

> > > >   * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma

> > > > @@ -172,6 +178,47 @@ renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {

> > > >  	tasklet_schedule(&priv->dma_priv.dma_complete);

> > > >  }

> > > >

> > > > +/* Should not use host->sg_ptr/sg_len in the following function */

> > >

> > > Maybe a short explanation why we shouldn't use the functions?

> >

> > I tried to update the comment as below:

> > /*

> >  * tmio_mmc_request() only sets host->sg_{ptr,len} and

> >  * renesas_sdhi_internal_dmac_pre_req() doesn't set host->sg_{ptr,len} so that

> >  * we should not use the values in the following function.

> >  */

> >

> > Hmm... Perhaps, I should modify the code to use host->sg_{ptr,len}

> > in both paths (.request() and .pre_req()) and remove this comments.

> > So, I'll try to modify. I guess tmio_mmc_init_sg() is called in pre_req(),

> > we can use host->sg_{ptr,len}.

> 

> I'm sorry. I was completely wrong. If we use {pre,post}_req,

> the MMC core will call pre_req() twice with each mmc_data before

> pre_req() is called. So that, second pre_req() will overwrite

> the host->sg_ptr. So, we should not use host->sg_ptr here.

> So, I'll update the comments like below.


I'm sorry again and again. But, I realized the current patch breaks
"force_pio" mode because tmio_mmc_pio_irq() doesn't take care of {pre,post}_req.
So, I'll try to refactor tmio core to support {pre,post}_req().

Best regards,
Yoshihiro Shimoda
Wolfram Sang Dec. 15, 2020, 10:40 a.m. UTC | #5
> I'm sorry again and again. But, I realized the current patch breaks

> "force_pio" mode because tmio_mmc_pio_irq() doesn't take care of {pre,post}_req.

> So, I'll try to refactor tmio core to support {pre,post}_req().


How did you test this BTW? Just checksumming a large file doesn't show
improvements here on 5.10 + your patch with H3 ES2.0 and M3-N for me.
Did I miss something?
Yoshihiro Shimoda Dec. 16, 2020, 8 a.m. UTC | #6
Hi Wolfram-san,

> From: Yoshihiro Shimoda, Sent: Tuesday, December 15, 2020 2:03 PM

<snip>
> I'm sorry again and again. But, I realized the current patch breaks

> "force_pio" mode because tmio_mmc_pio_irq() doesn't take care of {pre,post}_req.

> So, I'll try to refactor tmio core to support {pre,post}_req().


I'm sorry. My fault injection code is bad. In other words, we don't need
to refactor tmio core.

I had added "goto force_pio;" in renesas_sdhi_internal_dmac_start_dma() yesterday.
However, I should have added "goto force_pio_with_unmap;" instead.
Otherwise, the driver unmaps the buffer after pio finished so that reading data
by pio was invalidated by dma_unmap_sg().

So, I'll submit v2 patch later.

Best regards,
Yoshihiro Shimoda
diff mbox series

Patch

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index d8b811c..ce3c447 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -56,6 +56,12 @@ 
 #define INFO2_DTRANERR1		BIT(17)
 #define INFO2_DTRANERR0		BIT(16)
 
+enum renesas_sdhi_dma_cookie {
+	COOKIE_UNMAPPED,
+	COOKIE_PRE_MAPPED,
+	COOKIE_MAPPED,
+};
+
 /*
  * Specification of this driver:
  * - host->chan_{rx,tx} will be used as a flag of enabling/disabling the dma
@@ -172,6 +178,47 @@  renesas_sdhi_internal_dmac_dataend_dma(struct tmio_mmc_host *host) {
 	tasklet_schedule(&priv->dma_priv.dma_complete);
 }
 
+/* Should not use host->sg_ptr/sg_len in the following function */
+static void
+renesas_sdhi_internal_dmac_unmap(struct tmio_mmc_host *host,
+				 struct mmc_data *data,
+				 enum renesas_sdhi_dma_cookie cookie,
+				 bool expected_unmatch)
+{
+	bool unmap = expected_unmatch ? (data->host_cookie != cookie) :
+					(data->host_cookie == cookie);
+
+	if (unmap) {
+		dma_unmap_sg(&host->pdev->dev, data->sg, data->sg_len,
+			     mmc_get_dma_dir(data));
+		data->host_cookie = COOKIE_UNMAPPED;
+	}
+}
+
+/* Should not use host->sg_ptr/sg_len in the following function */
+static bool
+renesas_sdhi_internal_dmac_map(struct tmio_mmc_host *host,
+			       struct mmc_data *data,
+			       enum renesas_sdhi_dma_cookie cookie)
+{
+	if (data->host_cookie == COOKIE_PRE_MAPPED)
+		return true;
+
+	if (!dma_map_sg(&host->pdev->dev, data->sg, data->sg_len,
+			    mmc_get_dma_dir(data)))
+		return false;
+
+	data->host_cookie = cookie;
+
+	/* This DMAC cannot handle if buffer is not 8-bytes alignment */
+	if (!IS_ALIGNED(sg_dma_address(data->sg), 8)) {
+		renesas_sdhi_internal_dmac_unmap(host, data, cookie, false);
+		return false;
+	}
+
+	return true;
+}
+
 static void
 renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 				     struct mmc_data *data)
@@ -182,14 +229,9 @@  renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	if (!test_bit(SDHI_INTERNAL_DMAC_ADDR_MODE_FIXED_ONLY, &global_flags))
 		dtran_mode |= DTRAN_MODE_ADDR_MODE;
 
-	if (!dma_map_sg(&host->pdev->dev, sg, host->sg_len,
-			mmc_get_dma_dir(data)))
+	if (!renesas_sdhi_internal_dmac_map(host, data, COOKIE_MAPPED))
 		goto force_pio;
 
-	/* This DMAC cannot handle if buffer is not 8-bytes alignment */
-	if (!IS_ALIGNED(sg_dma_address(sg), 8))
-		goto force_pio_with_unmap;
-
 	if (data->flags & MMC_DATA_READ) {
 		dtran_mode |= DTRAN_MODE_CH_NUM_CH1;
 		if (test_bit(SDHI_INTERNAL_DMAC_ONE_RX_ONLY, &global_flags) &&
@@ -212,7 +254,7 @@  renesas_sdhi_internal_dmac_start_dma(struct tmio_mmc_host *host,
 	return;
 
 force_pio_with_unmap:
-	dma_unmap_sg(&host->pdev->dev, sg, host->sg_len, mmc_get_dma_dir(data));
+	renesas_sdhi_internal_dmac_unmap(host, data, COOKIE_UNMAPPED, true);
 
 force_pio:
 	renesas_sdhi_internal_dmac_enable_dma(host, false);
@@ -245,7 +287,7 @@  static bool renesas_sdhi_internal_dmac_complete(struct tmio_mmc_host *host)
 		dir = DMA_TO_DEVICE;
 
 	renesas_sdhi_internal_dmac_enable_dma(host, false);
-	dma_unmap_sg(&host->pdev->dev, host->sg_ptr, host->sg_len, dir);
+	renesas_sdhi_internal_dmac_unmap(host, host->data, COOKIE_MAPPED, false);
 
 	if (dir == DMA_FROM_DEVICE)
 		clear_bit(SDHI_INTERNAL_DMAC_RX_IN_USE, &global_flags);
@@ -274,6 +316,32 @@  static void renesas_sdhi_internal_dmac_end_dma(struct tmio_mmc_host *host)
 		renesas_sdhi_internal_dmac_complete(host);
 }
 
+static void renesas_sdhi_internal_dmac_post_req(struct mmc_host *mmc,
+						struct mmc_request *req,
+						int err)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct mmc_data *data = req->data;
+
+	if (!data)
+		return;
+
+	renesas_sdhi_internal_dmac_unmap(host, data, COOKIE_UNMAPPED, true);
+}
+
+static void renesas_sdhi_internal_dmac_pre_req(struct mmc_host *mmc,
+					       struct mmc_request *req)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+	struct mmc_data *data = req->data;
+
+	if (!data)
+		return;
+
+	data->host_cookie = COOKIE_UNMAPPED;
+	renesas_sdhi_internal_dmac_map(host, data, COOKIE_PRE_MAPPED);
+}
+
 static void
 renesas_sdhi_internal_dmac_request_dma(struct tmio_mmc_host *host,
 				       struct tmio_mmc_data *pdata)
@@ -295,6 +363,10 @@  renesas_sdhi_internal_dmac_request_dma(struct tmio_mmc_host *host,
 	tasklet_init(&host->dma_issue,
 		     renesas_sdhi_internal_dmac_issue_tasklet_fn,
 		     (unsigned long)host);
+
+	/* Add pre_req and post_req */
+	host->ops.pre_req = renesas_sdhi_internal_dmac_pre_req;
+	host->ops.post_req = renesas_sdhi_internal_dmac_post_req;
 }
 
 static void