From: Harish Jenny K N <[email protected]>
Enable MMC_CAP_ERASE capability in the driver to allow
erase/discard/trim requests.
Suggested-by: Andrew Gabbasov <[email protected]>
Signed-off-by: Harish Jenny K N <[email protected]>
[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 <[email protected]>
---
drivers/mmc/host/renesas_sdhi_internal_dmac.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
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,
--
2.24.0
On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> From: Harish Jenny K N <[email protected]>
>
> Enable MMC_CAP_ERASE capability in the driver to allow
> erase/discard/trim requests.
>
> Suggested-by: Andrew Gabbasov <[email protected]>
> Signed-off-by: Harish Jenny K N <[email protected]>
> [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 <[email protected]>
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
On Tue, 12 Nov 2019 at 21:49, Wolfram Sang <[email protected]> wrote:
>
> On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > From: Harish Jenny K N <[email protected]>
> >
> > Enable MMC_CAP_ERASE capability in the driver to allow
> > erase/discard/trim requests.
> >
> > Suggested-by: Andrew Gabbasov <[email protected]>
> > Signed-off-by: Harish Jenny K N <[email protected]>
> > [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 <[email protected]>
>
> 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
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 <[email protected]> wrote:
> >
> > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > From: Harish Jenny K N <[email protected]>
> > >
> > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > erase/discard/trim requests.
> > >
> > > Suggested-by: Andrew Gabbasov <[email protected]>
> > > Signed-off-by: Harish Jenny K N <[email protected]>
> > > [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 <[email protected]>
> >
> > 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")
--
Best Regards,
Eugeniu
On Thu, 14 Nov 2019 at 12:37, Eugeniu Rosca <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > > From: Harish Jenny K N <[email protected]>
> > > >
> > > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > > erase/discard/trim requests.
> > > >
> > > > Suggested-by: Andrew Gabbasov <[email protected]>
> > > > Signed-off-by: Harish Jenny K N <[email protected]>
> > > > [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 <[email protected]>
> > >
> > > 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
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
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
--
Best Regards,
Eugeniu
On Thu, 14 Nov 2019 at 21:15, Wolfram Sang <[email protected]> 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
On Thu, 14 Nov 2019 at 23:07, Eugeniu Rosca <[email protected]> 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
> 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?
On Fri, 15 Nov 2019 at 11:12, Wolfram Sang <[email protected]> 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
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 <[email protected]> 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
--
Best Regards,
Eugeniu
[to facilitate version tracking]
Superseded by:
https://lore.kernel.org/linux-renesas-soc/[email protected]/
("[PATCH v2] mmc: tmio: Add MMC_CAP_ERASE to allow erase/discard/trim requests")
--
Best Regards,
Eugeniu