[PATCH v2 0/2] mmc: renesas_sdhi: improve performance by using IOMMU

[PATCH v2 0/2] mmc: renesas_sdhi: improve performance by using IOMMU

[Yoshihiro Shimoda]

This patch set is based on renesas-drivers.git /
renesas-drivers-2019-05-07-v5.1 tag.

Since SDHI host internal DMAC of the R-Car Gen3 cannot handle two or more
segments, the performance rate (especially, eMMC HS400 reading) is not good.
However, if IOMMU is enabled on the DMAC, since IOMMU will map multiple
scatter gather buffers as one contignous iova, the DMAC can handle the iova
as well and then the performance rate is possible to improve. In fact,
I have measured the performance by using bonnie++, "Sequential Input - block"
rate was improved on all platforms (r8a7795, r8a77965 and r8a77990).
Please refer to the following URL about the performance.
(I beleive if the performance is improved, the CPU load is also increased.)

https://marc.info/?l=linux-renesas-soc&m=155625619320165&w=2

However, in case of a sdio card (especiialy some WiFi cards/drivers),
scatter gather buffers are possible to be not contiguous iova because
each scatter gather buffer has only about 1500 bytes, the DMAC cannot
handle it. So, this patch set adds init_card() ops to detect the card
type, and then the driver changes the max_segs if the DMAC is under
IOMMU environment and an sd card/mmc is detected.

Changes from v1 [1]:
 - Remove adding init_card ops into struct tmio_mmc_dma_ops and
   tmio_mmc_host and just set init_card on renesas_sdhi_core.c.
 - Revise typos on "mmc: tmio: No memory size limitation if runs on IOMMU".
 - Add Simon-san's Reviewed-by on a tmio patch.

[1]
https://patchwork.kernel.org/project/linux-renesas-soc/list/?series=110485


Yoshihiro Shimoda (2):
  mmc: tmio: No memory size limitation if runs on IOMMU
  mmc: renesas_sdhi: use multiple segments if possible

 drivers/mmc/host/renesas_sdhi_core.c | 13 +++++++++++++
 drivers/mmc/host/tmio_mmc_core.c     |  5 +++--
 2 files changed, 16 insertions(+), 2 deletions(-)

-- 
2.7.4

[PATCH v2 1/2] mmc: tmio: No memory size limitation if runs on IOMMU

[Yoshihiro Shimoda]

This patch adds a condition to avoid a memory size limitation of
swiotlb if the driver runs on IOMMU.

Tested-by: Takeshi Saito <takeshi.saito.xv@renesas.com>
Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
---
 drivers/mmc/host/tmio_mmc_core.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_core.c b/drivers/mmc/host/tmio_mmc_core.c
index 130b91c..7c76ab0 100644
--- a/drivers/mmc/host/tmio_mmc_core.c
+++ b/drivers/mmc/host/tmio_mmc_core.c
@@ -1194,9 +1194,10 @@ int tmio_mmc_host_probe(struct tmio_mmc_host *_host)
 	 * Since swiotlb has memory size limitation, this will calculate
 	 * the maximum size locally (because we don't have any APIs for it now)
 	 * and check the current max_req_size. And then, this will update
-	 * the max_req_size if needed as a workaround.
+	 * the max_req_size if needed as a workaround. However, if the driver
+	 * runs on IOMMU, this workaround isn't needed.
 	 */
-	if (swiotlb_max_segment()) {
+	if (swiotlb_max_segment() && !pdev->dev.iommu_group) {
 		unsigned int max_size = (1 << IO_TLB_SHIFT) * IO_TLB_SEGSIZE;
 
 		if (mmc->max_req_size > max_size)
-- 
2.7.4

[PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Yoshihiro Shimoda]

In IOMMU environment, since it's possible to merge scatter gather
buffers of memory requests to one iova, this patch changes the max_segs
value when init_card of mmc_host timing to improve the transfer
performance on renesas_sdhi_internal_dmac.

Notes that an sdio card may be possible to use scatter gather buffers
with non page aligned size, so that this driver will not use multiple
segments to avoid any trouble. Also, on renesas_sdhi_sys_dmac,
the max_segs value will change from 32 to 512, but the sys_dmac
can handle 512 segments, so that this init_card ops is added on
"TMIO_MMC_MIN_RCAR2" environment.

Signed-off-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
---
 drivers/mmc/host/renesas_sdhi_core.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
index 5e9e36e..29a7d66 100644
--- a/drivers/mmc/host/renesas_sdhi_core.c
+++ b/drivers/mmc/host/renesas_sdhi_core.c
@@ -203,6 +203,17 @@ static void renesas_sdhi_clk_disable(struct tmio_mmc_host *host)
 	clk_disable_unprepare(priv->clk_cd);
 }
 
+static void renesas_sdhi_init_card(struct mmc_host *mmc, struct mmc_card *card)
+{
+	struct tmio_mmc_host *host = mmc_priv(mmc);
+
+	if (host->pdev->dev.iommu_group &&
+	    (mmc_card_mmc(card) || mmc_card_sd(card)))
+		host->mmc->max_segs = 512;
+	else
+		host->mmc->max_segs = host->pdata->max_segs;
+}
+
 static int renesas_sdhi_card_busy(struct mmc_host *mmc)
 {
 	struct tmio_mmc_host *host = mmc_priv(mmc);
@@ -726,6 +737,8 @@ int renesas_sdhi_probe(struct platform_device *pdev,
 
 	/* SDR speeds are only available on Gen2+ */
 	if (mmc_data->flags & TMIO_MMC_MIN_RCAR2) {
+		host->ops.init_card = renesas_sdhi_init_card;
+
 		/* card_busy caused issues on r8a73a4 (pre-Gen2) CD-less SDHI */
 		host->ops.card_busy = renesas_sdhi_card_busy;
 		host->ops.start_signal_voltage_switch =
-- 
2.7.4

Re: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 965 bytes --]

Hi Shimoda-san,

thank you for this update!

> +static void renesas_sdhi_init_card(struct mmc_host *mmc, struct mmc_card *card)
> +{
> +	struct tmio_mmc_host *host = mmc_priv(mmc);
> +
> +	if (host->pdev->dev.iommu_group &&

I wonder if I am too cautious, but maybe we should have another
condition here to be checked first, namely "host->mmc->max_segs < 512"?

> +	    (mmc_card_mmc(card) || mmc_card_sd(card)))
> +		host->mmc->max_segs = 512;
> +	else
> +		host->mmc->max_segs = host->pdata->max_segs;

max_segs can be 0, so we should probably have:

 +		host->mmc->max_segs = host->pdata->max_segs ?: 32;

That also means, for the sys-dmac and Gen2, we then use 512 for the
IOMMU case and 32 (default TMIO value) for the non IOMMU case. My
understanding is that SYS DMAC can handle 512 in both cases. Maybe it
makes sense then to make an incremental patch setting the max_segs value
explicitly to 512 in the sys-dmac driver for Gen2?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Monday, May 13, 2019 6:01 PM
> 
> Hi Shimoda-san,
> 
> thank you for this update!
> 
> > +static void renesas_sdhi_init_card(struct mmc_host *mmc, struct mmc_card *card)
> > +{
> > +	struct tmio_mmc_host *host = mmc_priv(mmc);
> > +
> > +	if (host->pdev->dev.iommu_group &&
> 
> I wonder if I am too cautious, but maybe we should have another
> condition here to be checked first, namely "host->mmc->max_segs < 512"?

I got it. I'll fix it on v3 patch.

> > +	    (mmc_card_mmc(card) || mmc_card_sd(card)))
> > +		host->mmc->max_segs = 512;
> > +	else
> > +		host->mmc->max_segs = host->pdata->max_segs;
> 
> max_segs can be 0, so we should probably have:
> 
>  +		host->mmc->max_segs = host->pdata->max_segs ?: 32;

Thank you for the point! I'll fix it on v3 patch.

> That also means, for the sys-dmac and Gen2, we then use 512 for the
> IOMMU case and 32 (default TMIO value) for the non IOMMU case. My
> understanding is that SYS DMAC can handle 512 in both cases. Maybe it
> makes sense then to make an incremental patch setting the max_segs value
> explicitly to 512 in the sys-dmac driver for Gen2?

I also think SYS DMAC can handle 512 segments. However, I'm not sure
it can improve the performance or not though. Anyway, an incremental patch
makes sense if needed, I think.

Best regards,
Yoshihiro Shimoda

> Kind regards,
> 
>    Wolfram

RE: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Yoshihiro Shimoda, Sent: Monday, May 13, 2019 6:46 PM
<snip>
> > That also means, for the sys-dmac and Gen2, we then use 512 for the
> > IOMMU case and 32 (default TMIO value) for the non IOMMU case. My
> > understanding is that SYS DMAC can handle 512 in both cases. Maybe it
> > makes sense then to make an incremental patch setting the max_segs value
> > explicitly to 512 in the sys-dmac driver for Gen2?
> 
> I also think SYS DMAC can handle 512 segments. However, I'm not sure
> it can improve the performance or not though. Anyway, an incremental patch
> makes sense if needed, I think.

I measured the performance on R-Car H2 Lager. It seems 512 segments improve
the sequential input to 5%. May I make an incremental patch on the patch series?
What do you think?

Best regards,
Yoshihiro Shimoda

--
kernel v5.1-rc7 + local patches + sdr104,,,,,,,,,,,,,,,,,,,,,,,,,,
Buildroot 2019.02.1,,,,,,,,,,,,,,,,,,,,,,,,,,
Bonnie++ 1.03e : bonnie\+\+ -d ./ -s 2048 -r 1024 -b -u root,,,,,,,,,,,,,,,,,,,,,,,,,,
,,,,,,,,,,,,,,,,,,,,,,,,,,
environment,Size,Sequential Output - per char (K/sec),<- (CPU %),Sequential Output - block (K/sec),<- (CPU %),Sequential Output - rewrite (K/sec),<- (CPU %),Sequential Input - per char (K/sec),<- (CPU %),Sequential Input ? block (K/sec),<- (CPU %),Random seeks,<- (CPU %),files,Sequential Create,<- (CPU %),Sequential Read,<- (CPU %),Sequential Delete,<- (CPU %),Random Create,<- (CPU %),Random Read,<- (CPU %),Random Delete,<- (CPU %)
max_segs_32_sys_dmac,2G,19651,45,18122,7,11612,5,31417,56,34344,6,20.9,0,16,183,1,+++++,+++,195,1,198,2,+++++,+++,195,2
max_segs_512_sys_dmac,2G,18728,43,18273,8,12405,5,33524,61,34158,6,21.0,0,16,184,1,+++++,+++,189,1,198,2,+++++,+++,194,2

Re: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1096 bytes --]

Hi Shimoda-san,

> > > That also means, for the sys-dmac and Gen2, we then use 512 for the
> > > IOMMU case and 32 (default TMIO value) for the non IOMMU case. My
> > > understanding is that SYS DMAC can handle 512 in both cases. Maybe it
> > > makes sense then to make an incremental patch setting the max_segs value
> > > explicitly to 512 in the sys-dmac driver for Gen2?
> > 
> > I also think SYS DMAC can handle 512 segments. However, I'm not sure
> > it can improve the performance or not though. Anyway, an incremental patch
> > makes sense if needed, I think.
> 
> I measured the performance on R-Car H2 Lager. It seems 512 segments improve
> the sequential input to 5%. May I make an incremental patch on the patch series?
> What do you think?

Cool! I didn't expect that much of a performance improvement. My main
concern was understandable code because there was no "real reason" that
we once use 32 and once 512.

But if it causes even a performance improvement, even better :)

Yes, an incremental patch is a good idea.

Thanks for the work,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Yoshihiro Shimoda]

Hi Wolfram-san again,

> From: Yoshihiro Shimoda, Sent: Monday, May 13, 2019 6:46 PM
> 
> Hi Wolfram-san,
> 
> > From: Wolfram Sang, Sent: Monday, May 13, 2019 6:01 PM
> >
> > Hi Shimoda-san,
> >
> > thank you for this update!
> >
> > > +static void renesas_sdhi_init_card(struct mmc_host *mmc, struct mmc_card *card)
> > > +{
> > > +	struct tmio_mmc_host *host = mmc_priv(mmc);
> > > +
> > > +	if (host->pdev->dev.iommu_group &&
> >
> > I wonder if I am too cautious, but maybe we should have another
> > condition here to be checked first, namely "host->mmc->max_segs < 512"?
> 
> I got it. I'll fix it on v3 patch.

I'm afraid but I misunderstood this condition is "host->pdata->max_segs", not "host->mmc->max_segs",
to avoid small max_segs value than pdata->max_segs? (No one has such max_segs value at the moment though.)

If we use "host->mmc->max_segs", the max_segments value will be toggled by connecting/disconnecting a card like below:

(a card is connected)
# cat /sys/block/mmcblk0/queue/max_segments
512
(a card is disconnected and connected again)
# cat /sys/block/mmcblk0/queue/max_segments
1
(a card is disconnected and connected again)
# cat /sys/block/mmcblk0/queue/max_segments
512
...

Best regards,
Yoshihiro Shimoda

> > > +	    (mmc_card_mmc(card) || mmc_card_sd(card)))
> > > +		host->mmc->max_segs = 512;
> > > +	else
> > > +		host->mmc->max_segs = host->pdata->max_segs;
> >
> > max_segs can be 0, so we should probably have:
> >
> >  +		host->mmc->max_segs = host->pdata->max_segs ?: 32;
> 
> Thank you for the point! I'll fix it on v3 patch.
> 
> > That also means, for the sys-dmac and Gen2, we then use 512 for the
> > IOMMU case and 32 (default TMIO value) for the non IOMMU case. My
> > understanding is that SYS DMAC can handle 512 in both cases. Maybe it
> > makes sense then to make an incremental patch setting the max_segs value
> > explicitly to 512 in the sys-dmac driver for Gen2?
> 
> I also think SYS DMAC can handle 512 segments. However, I'm not sure
> it can improve the performance or not though. Anyway, an incremental patch
> makes sense if needed, I think.
> 
> Best regards,
> Yoshihiro Shimoda
> 
> > Kind regards,
> >
> >    Wolfram

Re: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1249 bytes --]

Hi Shimoda-san,


> > > > +	if (host->pdev->dev.iommu_group &&
> > >
> > > I wonder if I am too cautious, but maybe we should have another
> > > condition here to be checked first, namely "host->mmc->max_segs < 512"?
> > 
> > I got it. I'll fix it on v3 patch.
> 
> I'm afraid but I misunderstood this condition is
> "host->pdata->max_segs", not "host->mmc->max_segs", to avoid small
> max_segs value than pdata->max_segs?

You are right. I was in deed thinking mmc->max_segs because it will be
set at probe time, so it would work with values > 512. But I missed the
case you described, I am sorry. But using pdata->max_segs should work.

> (No one has such max_segs value at the moment though.)

Yes. I want to be future-proof here. Just to avoid that the value might
be "magically" decreased if the value might be bigger than 512. It would
be hard to find then because it is kinda deep in the driver.

Two more remarks:

* We should probably use a define for the magic value 512.

* Maybe you could add a comment to the init_card function describing why
  we can increase max_segs in that case. Basically, a short summary of
  your patch description.

Does this make sense to you?

Kind regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH v2 2/2] mmc: renesas_sdhi: use multiple segments if possible

[Yoshihiro Shimoda]

Hi Wolfram-san,

> From: Wolfram Sang, Sent: Wednesday, May 22, 2019 6:57 AM
> 
> Hi Shimoda-san,
> 
> > > > > +	if (host->pdev->dev.iommu_group &&
> > > >
> > > > I wonder if I am too cautious, but maybe we should have another
> > > > condition here to be checked first, namely "host->mmc->max_segs < 512"?
> > >
> > > I got it. I'll fix it on v3 patch.
> >
> > I'm afraid but I misunderstood this condition is
> > "host->pdata->max_segs", not "host->mmc->max_segs", to avoid small
> > max_segs value than pdata->max_segs?
> 
> You are right. I was in deed thinking mmc->max_segs because it will be
> set at probe time, so it would work with values > 512. But I missed the
> case you described, I am sorry. But using pdata->max_segs should work.

Thank you for the reply. I got it!

> > (No one has such max_segs value at the moment though.)
> 
> Yes. I want to be future-proof here. Just to avoid that the value might
> be "magically" decreased if the value might be bigger than 512. It would
> be hard to find then because it is kinda deep in the driver.

I got it.

> Two more remarks:
> 
> * We should probably use a define for the magic value 512.

I think so. I also would like to use a define for a magic value 32.

> * Maybe you could add a comment to the init_card function describing why
>   we can increase max_segs in that case. Basically, a short summary of
>   your patch description.

It's a good idea! I'll add such a short summary.

> Does this make sense to you?

Yes. Thank you for your comments. I'll make v3 patch later.

Best regards,
Yoshihiro Shimoda

> Kind regards,
> 
>    Wolfram