* [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
@ 2020-12-04 13:17 Yoshihiro Shimoda
2020-12-14 15:50 ` Wolfram Sang
0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2020-12-04 13:17 UTC (permalink / raw)
To: ulf.hansson, wsa+renesas; +Cc: linux-mmc, linux-renesas-soc, Yoshihiro Shimoda
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(-)
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
--
2.7.4
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-04 13:17 [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support Yoshihiro Shimoda
@ 2020-12-14 15:50 ` Wolfram Sang
2020-12-15 1:32 ` Yoshihiro Shimoda
0 siblings, 1 reply; 7+ messages in thread
From: Wolfram Sang @ 2020-12-14 15:50 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 1537 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-14 15:50 ` Wolfram Sang
@ 2020-12-15 1:32 ` Yoshihiro Shimoda
2020-12-15 3:00 ` Yoshihiro Shimoda
0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2020-12-15 1:32 UTC (permalink / raw)
To: Wolfram Sang; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-15 1:32 ` Yoshihiro Shimoda
@ 2020-12-15 3:00 ` Yoshihiro Shimoda
2020-12-15 5:03 ` Yoshihiro Shimoda
0 siblings, 1 reply; 7+ messages in thread
From: Yoshihiro Shimoda @ 2020-12-15 3:00 UTC (permalink / raw)
To: Wolfram Sang; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-15 3:00 ` Yoshihiro Shimoda
@ 2020-12-15 5:03 ` Yoshihiro Shimoda
2020-12-15 10:40 ` Wolfram Sang
2020-12-16 8:00 ` Yoshihiro Shimoda
0 siblings, 2 replies; 7+ messages in thread
From: Yoshihiro Shimoda @ 2020-12-15 5:03 UTC (permalink / raw)
To: Wolfram Sang; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-15 5:03 ` Yoshihiro Shimoda
@ 2020-12-15 10:40 ` Wolfram Sang
2020-12-16 8:00 ` Yoshihiro Shimoda
1 sibling, 0 replies; 7+ messages in thread
From: Wolfram Sang @ 2020-12-15 10:40 UTC (permalink / raw)
To: Yoshihiro Shimoda; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
[-- Attachment #1: Type: text/plain, Size: 385 bytes --]
> 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?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support
2020-12-15 5:03 ` Yoshihiro Shimoda
2020-12-15 10:40 ` Wolfram Sang
@ 2020-12-16 8:00 ` Yoshihiro Shimoda
1 sibling, 0 replies; 7+ messages in thread
From: Yoshihiro Shimoda @ 2020-12-16 8:00 UTC (permalink / raw)
To: Wolfram Sang; +Cc: ulf.hansson, linux-mmc, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-12-16 8:01 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-12-04 13:17 [PATCH] mmc: host: renesas_internal_dmac: add pre_req and post_req support Yoshihiro Shimoda
2020-12-14 15:50 ` Wolfram Sang
2020-12-15 1:32 ` Yoshihiro Shimoda
2020-12-15 3:00 ` Yoshihiro Shimoda
2020-12-15 5:03 ` Yoshihiro Shimoda
2020-12-15 10:40 ` Wolfram Sang
2020-12-16 8:00 ` Yoshihiro Shimoda
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.