mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs

Commit Message

Eugeniu Rosca Nov. 12, 2019, 1:48 p.m. UTC

From: Harish Jenny K N <[e-mail]>

Enable MMC_CAP_ERASE capability in the driver to allow
erase/discard/trim requests.

Suggested-by: Andrew Gabbasov <[e-mail]>
Signed-off-by: Harish Jenny K N <[e-mail]>
[erosca: Forward-port and test on v5.4-rc7 using H3ULCB-KF:
         "blkdiscard /dev/mmcblk0" passes with this patch applied
         and complains otherwise:
	 "BLKDISCARD ioctl failed: Operation not supported"]
Signed-off-by: Eugeniu Rosca <[e-mail]>
---
 drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Wolfram Sang Nov. 12, 2019, 8:49 p.m. UTC | #1

On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> From: Harish Jenny K N <[e-mail]>
> 
> Enable MMC_CAP_ERASE capability in the driver to allow
> erase/discard/trim requests.
> 
> Suggested-by: Andrew Gabbasov <[e-mail]>
> Signed-off-by: Harish Jenny K N <[e-mail]>
> [erosca: Forward-port and test on v5.4-rc7 using H3ULCB-KF:
>          "blkdiscard /dev/mmcblk0" passes with this patch applied
>          and complains otherwise:
> 	 "BLKDISCARD ioctl failed: Operation not supported"]
> Signed-off-by: Eugeniu Rosca <[e-mail]>

Looks good to me. Just a generic question, probably more for Ulf:

Why does this CAP_ERASE exist? As I understand, the driver only needs to
set the flag and no further handling is required. Why would a driver not
set this flag and not support erase/trim commands?

Kind regards,

   Wolfram

Ulf Hansson Nov. 14, 2019, 10:56 a.m. UTC | #2

On Tue, 12 Nov 2019 at 21:49, Wolfram Sang <[e-mail]> wrote:
>
> On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > From: Harish Jenny K N <[e-mail]>
> >
> > Enable MMC_CAP_ERASE capability in the driver to allow
> > erase/discard/trim requests.
> >
> > Suggested-by: Andrew Gabbasov <[e-mail]>
> > Signed-off-by: Harish Jenny K N <[e-mail]>
> > [erosca: Forward-port and test on v5.4-rc7 using H3ULCB-KF:
> >          "blkdiscard /dev/mmcblk0" passes with this patch applied
> >          and complains otherwise:
> >        "BLKDISCARD ioctl failed: Operation not supported"]
> > Signed-off-by: Eugeniu Rosca <[e-mail]>
>
> Looks good to me. Just a generic question, probably more for Ulf:
>
> Why does this CAP_ERASE exist? As I understand, the driver only needs to
> set the flag and no further handling is required. Why would a driver not
> set this flag and not support erase/trim commands?

I am working on removing the cap, altogether. Step by step, this is
getting closer now.

The main problem has been about busy detect timeouts, as an erase
command may have a very long busy timeout. On the host side, they
typically need to respect the cmd->busy_timeout for the request, and
if it can't because of some HW limitation, it needs to set
mmc->max_busy_timeout.

Once that is fixed for all, we can drop CAP_ERASE.

Kind regards
Uffe

Eugeniu Rosca Nov. 14, 2019, 11:37 a.m. UTC | #3

Hi everyone,

On Thu, Nov 14, 2019 at 11:56:23AM +0100, Ulf Hansson wrote:
> On Tue, 12 Nov 2019 at 21:49, Wolfram Sang <[e-mail]> wrote:
> >
> > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > From: Harish Jenny K N <[e-mail]>
> > >
> > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > erase/discard/trim requests.
> > >
> > > Suggested-by: Andrew Gabbasov <[e-mail]>
> > > Signed-off-by: Harish Jenny K N <[e-mail]>
> > > [erosca: Forward-port and test on v5.4-rc7 using H3ULCB-KF:
> > >          "blkdiscard /dev/mmcblk0" passes with this patch applied
> > >          and complains otherwise:
> > >        "BLKDISCARD ioctl failed: Operation not supported"]
> > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> >
> > Looks good to me. Just a generic question, probably more for Ulf:
> >
> > Why does this CAP_ERASE exist? As I understand, the driver only needs to
> > set the flag and no further handling is required. Why would a driver not
> > set this flag and not support erase/trim commands?
> 
> I am working on removing the cap, altogether. Step by step, this is
> getting closer now.
> 
> The main problem has been about busy detect timeouts, as an erase
> command may have a very long busy timeout. On the host side, they
> typically need to respect the cmd->busy_timeout for the request, and
> if it can't because of some HW limitation, it needs to set
> mmc->max_busy_timeout.

FWIW we've discussed such concerns internally, based on past commits
which either disable [1-2] busy timeouts or increase their value [3].

To get a feeling if this is relevant for R-Car3, I've run blkdiscard on
a 64 GiB eMMC without noticing any issues on v5.4-rc7. Hopefully this
is sufficient as testing?

> 
> Once that is fixed for all, we can drop CAP_ERASE.
> 
> Kind regards
> Uffe

[1] 93caf8e69eac76 ("omap_hsmmc: add erase capability")
[2] b13d1f0f9ad64b ("mmc: omap: Add erase capability")
[3] ec30f11e821f2d ("mmc: rtsx_usb: Use the provided busy timeout from the mmc core")

Ulf Hansson Nov. 14, 2019, 12:48 p.m. UTC | #4

On Thu, 14 Nov 2019 at 12:37, Eugeniu Rosca <[e-mail]> wrote:
>
> Hi everyone,
>
> On Thu, Nov 14, 2019 at 11:56:23AM +0100, Ulf Hansson wrote:
> > On Tue, 12 Nov 2019 at 21:49, Wolfram Sang <[e-mail]> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > > From: Harish Jenny K N <[e-mail]>
> > > >
> > > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > > erase/discard/trim requests.
> > > >
> > > > Suggested-by: Andrew Gabbasov <[e-mail]>
> > > > Signed-off-by: Harish Jenny K N <[e-mail]>
> > > > [erosca: Forward-port and test on v5.4-rc7 using H3ULCB-KF:
> > > >          "blkdiscard /dev/mmcblk0" passes with this patch applied
> > > >          and complains otherwise:
> > > >        "BLKDISCARD ioctl failed: Operation not supported"]
> > > > Signed-off-by: Eugeniu Rosca <[e-mail]>
> > >
> > > Looks good to me. Just a generic question, probably more for Ulf:
> > >
> > > Why does this CAP_ERASE exist? As I understand, the driver only needs to
> > > set the flag and no further handling is required. Why would a driver not
> > > set this flag and not support erase/trim commands?
> >
> > I am working on removing the cap, altogether. Step by step, this is
> > getting closer now.
> >
> > The main problem has been about busy detect timeouts, as an erase
> > command may have a very long busy timeout. On the host side, they
> > typically need to respect the cmd->busy_timeout for the request, and
> > if it can't because of some HW limitation, it needs to set
> > mmc->max_busy_timeout.
>
> FWIW we've discussed such concerns internally, based on past commits
> which either disable [1-2] busy timeouts or increase their value [3].
>
> To get a feeling if this is relevant for R-Car3, I've run blkdiscard on
> a 64 GiB eMMC without noticing any issues on v5.4-rc7. Hopefully this
> is sufficient as testing?

Let's first take a step back, because I don't know how the HW busy
detection works for your controller.

I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
pre-defined number of loops/timeout. This looks scary, but I can't
tell if it's really a problem.

BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?

I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
the renesas/tmio variant hosts. Is that simply because the HW doesn't
support this? Or because implementation is missing?

If you want to run a test that stretches the behaviour on the timeout
path, I would rather use an SD-card (the older the better). For eMMCs
the erase likely translates to a trim/discard, which is far more
quicker than a real erase - as is what happens on an old SD card.

>
> >
> > Once that is fixed for all, we can drop CAP_ERASE.
> >
> > Kind regards
> > Uffe
>
> [1] 93caf8e69eac76 ("omap_hsmmc: add erase capability")
> [2] b13d1f0f9ad64b ("mmc: omap: Add erase capability")
> [3] ec30f11e821f2d ("mmc: rtsx_usb: Use the provided busy timeout from the mmc core")
>
> --
> Best Regards,
> Eugeniu

Kind regards
Uffe

Wolfram Sang Nov. 14, 2019, 8:15 p.m. UTC | #5

Hi Ulf,

thanks again for the heads up.

> Let's first take a step back, because I don't know how the HW busy
> detection works for your controller.
> 
> I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
> variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
> pre-defined number of loops/timeout. This looks scary, but I can't
> tell if it's really a problem.

That should be okay. The datasheet mentions that some registers can only
be accessed when either CBSY or SCLKDIVEN bits signal non-busyness.
renesas_sdhi_wait_idle() is for that.

> BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?

0: A command sequence has been completed.
1: A command sequence is being executed.

> I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
> the renesas/tmio variant hosts. Is that simply because the HW doesn't
> support this? Or because implementation is missing?

Good thing we use public development. I recalled we discussed this
before but I needed a search engine to find it again:

https://patchwork.kernel.org/patch/8114821/

Summary: The HW (at least since Gen2) has HW support for busy timeout
detection but I never came around to implement it (and even forgot about
it :( ). So, we still use a workqueue for it.

Kind regards,

   Wolfram

Eugeniu Rosca Nov. 14, 2019, 10:07 p.m. UTC | #6

Hi Ulf,

On Thu, Nov 14, 2019 at 01:48:41PM +0100, Ulf Hansson wrote:

[..]
> 
> Let's first take a step back, because I don't know how the HW busy
> detection works for your controller.
> 
> I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
> variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
> pre-defined number of loops/timeout. This looks scary, but I can't
> tell if it's really a problem.
> 
> BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?
> 
> I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
> the renesas/tmio variant hosts. Is that simply because the HW doesn't
> support this? Or because implementation is missing?

Hopefully Wolfram just addressed that?

> If you want to run a test that stretches the behaviour on the timeout
> path, I would rather use an SD-card (the older the better). For eMMCs
> the erase likely translates to a trim/discard, which is far more
> quicker than a real erase - as is what happens on an old SD card.

Running 'blkdiscard' with different SD cards on H3ULCB, I don't see any
signs of misbehavior:

root@rcar-gen3:~# blkdiscard -V
blkdiscard from util-linux 2.32.1

root@rcar-gen3:~# lsblk
NAME         MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
mmcblk0      179:0    0 59.2G  0 disk
mmcblk0boot0 179:8    0    4M  1 disk
mmcblk0boot1 179:16   0    4M  1 disk
mmcblk1      179:24   0   30G  0 disk

# Erasing 32 GiB uSD Card
root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk1
/dev/mmcblk1: Discarded 32227983360 bytes from the offset 0

real    0m1.198s
user    0m0.001s
sys     0m0.122s

# Erasing 64 GiB eMMC
root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk0
/dev/mmcblk0: Discarded 63585648640 bytes from the offset 0

real    0m8.703s
user    0m0.002s
sys     0m1.909s

I guess that by decreasing below erase sizes, I could further increase
the execution time, but these sysfs properties are read-only:

cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/preferred_erase_size
4194304
cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/erase_size
512

Ulf Hansson Nov. 15, 2019, 9:19 a.m. UTC | #7

On Thu, 14 Nov 2019 at 21:15, Wolfram Sang <[e-mail]> wrote:
>
> Hi Ulf,
>
> thanks again for the heads up.
>
> > Let's first take a step back, because I don't know how the HW busy
> > detection works for your controller.
> >
> > I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
> > variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
> > pre-defined number of loops/timeout. This looks scary, but I can't
> > tell if it's really a problem.
>
> That should be okay. The datasheet mentions that some registers can only
> be accessed when either CBSY or SCLKDIVEN bits signal non-busyness.
> renesas_sdhi_wait_idle() is for that.
>
> > BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?
>
> 0: A command sequence has been completed.
> 1: A command sequence is being executed.

Alright, thanks for clarifying!

>
> > I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
> > the renesas/tmio variant hosts. Is that simply because the HW doesn't
> > support this? Or because implementation is missing?
>
> Good thing we use public development. I recalled we discussed this
> before but I needed a search engine to find it again:
>
> https://patchwork.kernel.org/patch/8114821/
>
> Summary: The HW (at least since Gen2) has HW support for busy timeout
> detection but I never came around to implement it (and even forgot about
> it :( ). So, we still use a workqueue for it.

I had a vague memory about this discussion as well, thanks for giving
the pointers to it.

I think using a workqueue for scheduling a reset work with a timeout
of 5 s, as in your case.

However, as a heads up, if/when implementing support for busy
detection and adding MMC_CAP_WAIT_WHILE_BUSY, needs to update that
timeout according to cmd->busy_timeout, which is provided by the core.

Kind regards
Uffe

Ulf Hansson Nov. 15, 2019, 9:27 a.m. UTC | #8

On Thu, 14 Nov 2019 at 23:07, Eugeniu Rosca <[e-mail]> wrote:
>
> Hi Ulf,
>
> On Thu, Nov 14, 2019 at 01:48:41PM +0100, Ulf Hansson wrote:
>
> [..]
> >
> > Let's first take a step back, because I don't know how the HW busy
> > detection works for your controller.
> >
> > I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
> > variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
> > pre-defined number of loops/timeout. This looks scary, but I can't
> > tell if it's really a problem.
> >
> > BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?
> >
> > I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
> > the renesas/tmio variant hosts. Is that simply because the HW doesn't
> > support this? Or because implementation is missing?
>
> Hopefully Wolfram just addressed that?
>
> > If you want to run a test that stretches the behaviour on the timeout
> > path, I would rather use an SD-card (the older the better). For eMMCs
> > the erase likely translates to a trim/discard, which is far more
> > quicker than a real erase - as is what happens on an old SD card.
>
> Running 'blkdiscard' with different SD cards on H3ULCB, I don't see any
> signs of misbehavior:
>
> root@rcar-gen3:~# blkdiscard -V
> blkdiscard from util-linux 2.32.1
>
> root@rcar-gen3:~# lsblk
> NAME         MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
> mmcblk0      179:0    0 59.2G  0 disk
> mmcblk0boot0 179:8    0    4M  1 disk
> mmcblk0boot1 179:16   0    4M  1 disk
> mmcblk1      179:24   0   30G  0 disk
>
> # Erasing 32 GiB uSD Card
> root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk1
> /dev/mmcblk1: Discarded 32227983360 bytes from the offset 0
>
> real    0m1.198s
> user    0m0.001s
> sys     0m0.122s
>
> # Erasing 64 GiB eMMC
> root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk0
> /dev/mmcblk0: Discarded 63585648640 bytes from the offset 0
>
> real    0m8.703s
> user    0m0.002s
> sys     0m1.909s
>
> I guess that by decreasing below erase sizes, I could further increase
> the execution time, but these sysfs properties are read-only:
>
> cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/preferred_erase_size
> 4194304
> cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/erase_size
> 512
>

This test and due to the discussions with Wolfram and you in this
thread, I would actually suggest that you enable MMC_CAP_ERASE for all
tmio variants, rather than just for this particular one.

In other words, set the cap in tmio_mmc_host_probe() should be fine,
as it seems none of the tmio variants supports HW busy detection at
this point.

Kind regards
Uffe

Wolfram Sang Nov. 15, 2019, 10:12 a.m. UTC | #9

> I think using a workqueue for scheduling a reset work with a timeout
> of 5 s, as in your case.

Sorry, I didn't get it. You think what exactly? Is it good / bad / ok
for now / ok in general?

Ulf Hansson Nov. 15, 2019, 10:38 a.m. UTC | #10

On Fri, 15 Nov 2019 at 11:12, Wolfram Sang <[e-mail]> wrote:
>
>
> > I think using a workqueue for scheduling a reset work with a timeout
> > of 5 s, as in your case.
>
> Sorry, I didn't get it. You think what exactly? Is it good / bad / ok
> for now / ok in general?

Sorry.

It's good for now!

If/when you start implementing support for HW busy detection, then you
need to adjust to the timeout value according to cmd->busy_timeout
from the core.

Kind regards
Uffe

Eugeniu Rosca Nov. 15, 2019, 12:51 p.m. UTC | #11

Hello Yamada-san,

On Fri, Nov 15, 2019 at 10:27:25AM +0100, Ulf Hansson wrote:
> On Thu, 14 Nov 2019 at 23:07, Eugeniu Rosca <[e-mail]> wrote:
> >
> > Hi Ulf,
> >
> > On Thu, Nov 14, 2019 at 01:48:41PM +0100, Ulf Hansson wrote:
> >
> > [..]
> > >
> > > Let's first take a step back, because I don't know how the HW busy
> > > detection works for your controller.
> > >
> > > I have noticed there is TMIO_STAT_CMD_BUSY bit being set for some
> > > variants, which seems to cause renesas_sdhi_wait_idle() to loop for a
> > > pre-defined number of loops/timeout. This looks scary, but I can't
> > > tell if it's really a problem.
> > >
> > > BTW, do you know what TMIO_STAT_CMD_BUSY actually is monitoring?
> > >
> > > I have also noticed that MMC_CAP_WAIT_WHILE_BUSY isn't set for any of
> > > the renesas/tmio variant hosts. Is that simply because the HW doesn't
> > > support this? Or because implementation is missing?
> >
> > Hopefully Wolfram just addressed that?
> >
> > > If you want to run a test that stretches the behaviour on the timeout
> > > path, I would rather use an SD-card (the older the better). For eMMCs
> > > the erase likely translates to a trim/discard, which is far more
> > > quicker than a real erase - as is what happens on an old SD card.
> >
> > Running 'blkdiscard' with different SD cards on H3ULCB, I don't see any
> > signs of misbehavior:
> >
> > root@rcar-gen3:~# blkdiscard -V
> > blkdiscard from util-linux 2.32.1
> >
> > root@rcar-gen3:~# lsblk
> > NAME         MAJ:MIN RM  SIZE RO TYPE MOUNTPOINT
> > mmcblk0      179:0    0 59.2G  0 disk
> > mmcblk0boot0 179:8    0    4M  1 disk
> > mmcblk0boot1 179:16   0    4M  1 disk
> > mmcblk1      179:24   0   30G  0 disk
> >
> > # Erasing 32 GiB uSD Card
> > root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk1
> > /dev/mmcblk1: Discarded 32227983360 bytes from the offset 0
> >
> > real    0m1.198s
> > user    0m0.001s
> > sys     0m0.122s
> >
> > # Erasing 64 GiB eMMC
> > root@rcar-gen3:~# time blkdiscard -v /dev/mmcblk0
> > /dev/mmcblk0: Discarded 63585648640 bytes from the offset 0
> >
> > real    0m8.703s
> > user    0m0.002s
> > sys     0m1.909s
> >
> > I guess that by decreasing below erase sizes, I could further increase
> > the execution time, but these sysfs properties are read-only:
> >
> > cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/preferred_erase_size
> > 4194304
> > cat /sys/devices/platform/soc/ee100000.sd/mmc_host/mmc1/mmc1:59b4/erase_size
> > 512
> >
> 
> This test and due to the discussions with Wolfram and you in this
> thread, I would actually suggest that you enable MMC_CAP_ERASE for all
> tmio variants, rather than just for this particular one.
> 
> In other words, set the cap in tmio_mmc_host_probe() should be fine,
> as it seems none of the tmio variants supports HW busy detection at
> this point.

Just for your information, following Ulf's suggestion, we are going to
enable MMC_CAP_ERASE in the TMIO mmc core driver, affecting UniPhier
SD/eMMC Host Controller. Hope to see your Ack/NAK on this in the
upcoming patch. TIA.

> 
> Kind regards
> Uffe

Eugeniu Rosca Nov. 15, 2019, 1:54 p.m. UTC | #12

[to facilitate version tracking]

Superseded by:
https://lore.kernel.org/linux-renesas-soc/[e-mail]/
("[PATCH v2] mmc: tmio: Add MMC_CAP_ERASE to allow erase/discard/trim requests")

Patch

diff --git a/drivers/mmc/host/renesas_sdhi_internal_dmac.c b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
index a66f8d6d61d1..61fcbf51c947 100644
--- a/drivers/mmc/host/renesas_sdhi_internal_dmac.c
+++ b/drivers/mmc/host/renesas_sdhi_internal_dmac.c
@@ -105,7 +105,7 @@  static const struct renesas_sdhi_of_data of_rcar_gen3_compatible = {
 	.tmio_flags	= TMIO_MMC_HAS_IDLE_WAIT | TMIO_MMC_CLK_ACTUAL |
 			  TMIO_MMC_HAVE_CBSY | TMIO_MMC_MIN_RCAR2,
 	.capabilities	= MMC_CAP_SD_HIGHSPEED | MMC_CAP_SDIO_IRQ |
-			  MMC_CAP_CMD23,
+			  MMC_CAP_ERASE | MMC_CAP_CMD23,
 	.capabilities2	= MMC_CAP2_NO_WRITE_PROTECT | MMC_CAP2_MERGE_CAPABLE,
 	.bus_shift	= 2,
 	.scc_offset	= 0x1000,