Linux-mmc Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
@ 2019-11-12 13:48 Eugeniu Rosca
  2019-11-12 20:49 ` Wolfram Sang
  0 siblings, 1 reply; 13+ messages in thread
From: Eugeniu Rosca @ 2019-11-12 13:48 UTC (permalink / raw)
  To: Wolfram Sang, Yoshihiro Shimoda, Niklas Söderlund, Ulf Hansson
  Cc: Geert Uytterhoeven, Simon Horman, linux-mmc, linux-kernel,
	linux-renesas-soc, Eugeniu Rosca, Eugeniu Rosca,
	Harish Jenny K N, Andrew Gabbasov

From: Harish Jenny K N <harish_kandiga@mentor.com>

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

Suggested-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
[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 <erosca@de.adit-jv.com>
---
 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-12 13:48 [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs Eugeniu Rosca
@ 2019-11-12 20:49 ` Wolfram Sang
  2019-11-14 10:56   ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2019-11-12 20:49 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Wolfram Sang, Yoshihiro Shimoda, Niklas Söderlund,
	Ulf Hansson, Geert Uytterhoeven, Simon Horman, linux-mmc,
	linux-kernel, linux-renesas-soc, Eugeniu Rosca, Harish Jenny K N,
	Andrew Gabbasov

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

On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> From: Harish Jenny K N <harish_kandiga@mentor.com>
> 
> Enable MMC_CAP_ERASE capability in the driver to allow
> erase/discard/trim requests.
> 
> Suggested-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> [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 <erosca@de.adit-jv.com>

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


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-12 20:49 ` Wolfram Sang
@ 2019-11-14 10:56   ` Ulf Hansson
  2019-11-14 11:37     ` Eugeniu Rosca
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2019-11-14 10:56 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

On Tue, 12 Nov 2019 at 21:49, Wolfram Sang <wsa@the-dreams.de> wrote:
>
> On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > From: Harish Jenny K N <harish_kandiga@mentor.com>
> >
> > Enable MMC_CAP_ERASE capability in the driver to allow
> > erase/discard/trim requests.
> >
> > Suggested-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> > [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 <erosca@de.adit-jv.com>
>
> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 10:56   ` Ulf Hansson
@ 2019-11-14 11:37     ` Eugeniu Rosca
  2019-11-14 12:48       ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Eugeniu Rosca @ 2019-11-14 11:37 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Wolfram Sang, Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

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 <wsa@the-dreams.de> wrote:
> >
> > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > From: Harish Jenny K N <harish_kandiga@mentor.com>
> > >
> > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > erase/discard/trim requests.
> > >
> > > Suggested-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> > > [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 <erosca@de.adit-jv.com>
> >
> > 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 11:37     ` Eugeniu Rosca
@ 2019-11-14 12:48       ` Ulf Hansson
  2019-11-14 20:15         ` Wolfram Sang
  2019-11-14 22:07         ` Eugeniu Rosca
  0 siblings, 2 replies; 13+ messages in thread
From: Ulf Hansson @ 2019-11-14 12:48 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Wolfram Sang, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

On Thu, 14 Nov 2019 at 12:37, Eugeniu Rosca <erosca@de.adit-jv.com> 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 <wsa@the-dreams.de> wrote:
> > >
> > > On Tue, Nov 12, 2019 at 02:48:08PM +0100, Eugeniu Rosca wrote:
> > > > From: Harish Jenny K N <harish_kandiga@mentor.com>
> > > >
> > > > Enable MMC_CAP_ERASE capability in the driver to allow
> > > > erase/discard/trim requests.
> > > >
> > > > Suggested-by: Andrew Gabbasov <andrew_gabbasov@mentor.com>
> > > > Signed-off-by: Harish Jenny K N <harish_kandiga@mentor.com>
> > > > [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 <erosca@de.adit-jv.com>
> > >
> > > 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 12:48       ` Ulf Hansson
@ 2019-11-14 20:15         ` Wolfram Sang
  2019-11-15  9:19           ` Ulf Hansson
  2019-11-14 22:07         ` Eugeniu Rosca
  1 sibling, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2019-11-14 20:15 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

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

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


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 12:48       ` Ulf Hansson
  2019-11-14 20:15         ` Wolfram Sang
@ 2019-11-14 22:07         ` Eugeniu Rosca
  2019-11-15  9:27           ` Ulf Hansson
  1 sibling, 1 reply; 13+ messages in thread
From: Eugeniu Rosca @ 2019-11-14 22:07 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Eugeniu Rosca, Wolfram Sang, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 20:15         ` Wolfram Sang
@ 2019-11-15  9:19           ` Ulf Hansson
  2019-11-15 10:12             ` Wolfram Sang
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2019-11-15  9:19 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

On Thu, 14 Nov 2019 at 21:15, Wolfram Sang <wsa@the-dreams.de> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-14 22:07         ` Eugeniu Rosca
@ 2019-11-15  9:27           ` Ulf Hansson
  2019-11-15 12:51             ` Eugeniu Rosca
  0 siblings, 1 reply; 13+ messages in thread
From: Ulf Hansson @ 2019-11-15  9:27 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Wolfram Sang, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

On Thu, 14 Nov 2019 at 23:07, Eugeniu Rosca <erosca@de.adit-jv.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-15  9:19           ` Ulf Hansson
@ 2019-11-15 10:12             ` Wolfram Sang
  2019-11-15 10:38               ` Ulf Hansson
  0 siblings, 1 reply; 13+ messages in thread
From: Wolfram Sang @ 2019-11-15 10:12 UTC (permalink / raw)
  To: Ulf Hansson
  Cc: Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

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


> 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?


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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-15 10:12             ` Wolfram Sang
@ 2019-11-15 10:38               ` Ulf Hansson
  0 siblings, 0 replies; 13+ messages in thread
From: Ulf Hansson @ 2019-11-15 10:38 UTC (permalink / raw)
  To: Wolfram Sang
  Cc: Eugeniu Rosca, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

On Fri, 15 Nov 2019 at 11:12, Wolfram Sang <wsa@the-dreams.de> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-15  9:27           ` Ulf Hansson
@ 2019-11-15 12:51             ` Eugeniu Rosca
  2019-11-15 13:54               ` Eugeniu Rosca
  0 siblings, 1 reply; 13+ messages in thread
From: Eugeniu Rosca @ 2019-11-15 12:51 UTC (permalink / raw)
  To: Masahiro Yamada, Ulf Hansson
  Cc: Eugeniu Rosca, Wolfram Sang, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

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 <erosca@de.adit-jv.com> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs
  2019-11-15 12:51             ` Eugeniu Rosca
@ 2019-11-15 13:54               ` Eugeniu Rosca
  0 siblings, 0 replies; 13+ messages in thread
From: Eugeniu Rosca @ 2019-11-15 13:54 UTC (permalink / raw)
  To: Masahiro Yamada, Ulf Hansson
  Cc: Eugeniu Rosca, Wolfram Sang, Wolfram Sang, Yoshihiro Shimoda,
	Niklas Söderlund, Geert Uytterhoeven, Simon Horman,
	linux-mmc, Linux Kernel Mailing List, Linux-Renesas,
	Eugeniu Rosca, Harish Jenny K N, Andrew Gabbasov

[to facilitate version tracking]

Superseded by:
https://lore.kernel.org/linux-renesas-soc/20191115134430.12621-1-erosca@de.adit-jv.com/
("[PATCH v2] mmc: tmio: Add MMC_CAP_ERASE to allow erase/discard/trim requests")

-- 
Best Regards,
Eugeniu

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, back to index

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-12 13:48 [PATCH] mmc: renesas_sdhi_internal_dmac: Add MMC_CAP_ERASE to Gen3 SoCs Eugeniu Rosca
2019-11-12 20:49 ` Wolfram Sang
2019-11-14 10:56   ` Ulf Hansson
2019-11-14 11:37     ` Eugeniu Rosca
2019-11-14 12:48       ` Ulf Hansson
2019-11-14 20:15         ` Wolfram Sang
2019-11-15  9:19           ` Ulf Hansson
2019-11-15 10:12             ` Wolfram Sang
2019-11-15 10:38               ` Ulf Hansson
2019-11-14 22:07         ` Eugeniu Rosca
2019-11-15  9:27           ` Ulf Hansson
2019-11-15 12:51             ` Eugeniu Rosca
2019-11-15 13:54               ` Eugeniu Rosca

Linux-mmc Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mmc/0 linux-mmc/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mmc linux-mmc/ https://lore.kernel.org/linux-mmc \
		linux-mmc@vger.kernel.org
	public-inbox-index linux-mmc

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-mmc


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git