diff mbox series

[v2,10/14] mmc: sdhci-esdhc-imx: handle 'sdhci,auto-cmd23-broken' from devicetree

Message ID 20191203130120.11511-11-haibo.chen@nxp.com (mailing list archive)
State New, archived
Headers show
Series [v2,01/14] mmc: sdhci: do not enable card detect interrupt for gpio cd type | expand

Commit Message

Bough Chen Dec. 3, 2019, 12:54 p.m. UTC
Since L4.15, community involve the commit 105819c8a545 ("mmc: core: use mrq->sbc
when sending CMD23 for RPMB"), let the usdhc to decide whether to use ACMD23 for
RPMB. This CMD23 for RPMB need to set the bit 31 to its argument, if not, the
RPMB write operation will return general fail.

According to the sdhci logic, SDMA mode will disable the ACMD23, and only in
ADMA mode, it will chose to use ACMD23 if the host support. But according to
debug, and confirm with IC, the imx6qpdl/imx6sx/imx6sl/imx7d do not support
the ACMD23 feature completely. These SoCs only use the 16 bit block count of
the register 0x4 (BLOCK_ATT) as the CMD23's argument in ACMD23 mode, which
means it will ignore the upper 16 bit of the CMD23's argument. This will block
the reliable write operation in RPMB, because RPMB reliable write need to set
the bit31 of the CMD23's argument. This is the hardware limitation. Due to
imx6sl use SDMA, so for imx6qpdl/imx6sx/imx7d, it need to broke the ACMD23 for
eMMC, SD card do not has this limitation, because SD card do not support reliable
write.

For imx6ul/imx6ull/imx6sll/imx7ulp/imx8, it support the ACMD23 completely, it
change to use the 0x0 register (DS_ADDR) to put the CMD23's argument in ADMA mode.

This patch handle 'sdhci,auto-cmd23-broken' from devicetree.

Signed-off-by: Haibo Chen <haibo.chen@nxp.com>
---
 drivers/mmc/host/sdhci-esdhc-imx.c | 5 +++--
 drivers/mmc/host/sdhci-pltfm.c     | 3 +++
 2 files changed, 6 insertions(+), 2 deletions(-)

Comments

Linus Walleij Dec. 10, 2019, 11:38 p.m. UTC | #1
On Tue, Dec 3, 2019 at 1:54 PM BOUGH CHEN <haibo.chen@nxp.com> wrote:

> Since L4.15, community involve the commit 105819c8a545 ("mmc: core: use mrq->sbc
> when sending CMD23 for RPMB"), let the usdhc to decide whether to use ACMD23 for
> RPMB. This CMD23 for RPMB need to set the bit 31 to its argument, if not, the
> RPMB write operation will return general fail.
>
> According to the sdhci logic, SDMA mode will disable the ACMD23, and only in
> ADMA mode, it will chose to use ACMD23 if the host support. But according to
> debug, and confirm with IC, the imx6qpdl/imx6sx/imx6sl/imx7d do not support
> the ACMD23 feature completely. These SoCs only use the 16 bit block count of
> the register 0x4 (BLOCK_ATT) as the CMD23's argument in ACMD23 mode, which
> means it will ignore the upper 16 bit of the CMD23's argument. This will block
> the reliable write operation in RPMB, because RPMB reliable write need to set
> the bit31 of the CMD23's argument. This is the hardware limitation. Due to
> imx6sl use SDMA, so for imx6qpdl/imx6sx/imx7d, it need to broke the ACMD23 for
> eMMC, SD card do not has this limitation, because SD card do not support reliable
> write.
>
> For imx6ul/imx6ull/imx6sll/imx7ulp/imx8, it support the ACMD23 completely, it
> change to use the 0x0 register (DS_ADDR) to put the CMD23's argument in ADMA mode.
>
> This patch handle 'sdhci,auto-cmd23-broken' from devicetree.
>
> Signed-off-by: Haibo Chen <haibo.chen@nxp.com>

So it does seem like this is a problem with the host controller, not
with the card?

> -       if (of_find_property(np, "no-1-8-v", NULL))
> -               host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;

Why are you deleting this? Seems unrelated.

> +       /* call to generic sdhci_get_property to support additional capabilities */
> +       sdhci_get_property(pdev);

Or does it get set by this call?
(Then explain that change in the commit message.)

> +       if (device_property_present(dev, "sdhci,auto-cmd23-broken"))
> +               host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN;

Even if this is a problem with the host controller I am not convinced.

Can't you just use the compatible-string of the host controller to
switch this flag?

They should be different compatibles because if this bug is present
in one version of the host controller and not another version then
by definition they are not compatible, right?

The idea with very specific compatible strings is to handle
exactly things like this.

Yours,
Linus Walleij
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
index 54ffb04ab1d1..0a8a7b748341 100644
--- a/drivers/mmc/host/sdhci-esdhc-imx.c
+++ b/drivers/mmc/host/sdhci-esdhc-imx.c
@@ -1371,8 +1371,6 @@  sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 
 	of_property_read_u32(np, "fsl,strobe-dll-delay-target",
 				&boarddata->strobe_dll_delay_target);
-	if (of_find_property(np, "no-1-8-v", NULL))
-		host->quirks2 |= SDHCI_QUIRK2_NO_1_8_V;
 
 	if (of_property_read_u32(np, "fsl,delay-line", &boarddata->delay_line))
 		boarddata->delay_line = 0;
@@ -1386,6 +1384,9 @@  sdhci_esdhc_imx_probe_dt(struct platform_device *pdev,
 						ESDHC_PINCTRL_STATE_200MHZ);
 	}
 
+	/* call to generic sdhci_get_property to support additional capabilities */
+	sdhci_get_property(pdev);
+
 	/* call to generic mmc_of_parse to support additional capabilities */
 	ret = mmc_of_parse(host->mmc);
 	if (ret)
diff --git a/drivers/mmc/host/sdhci-pltfm.c b/drivers/mmc/host/sdhci-pltfm.c
index 328b132bbe57..862d2bb9c2a8 100644
--- a/drivers/mmc/host/sdhci-pltfm.c
+++ b/drivers/mmc/host/sdhci-pltfm.c
@@ -86,6 +86,9 @@  void sdhci_get_property(struct platform_device *pdev)
 	if (device_property_present(dev, "sdhci,auto-cmd12"))
 		host->quirks |= SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12;
 
+	if (device_property_present(dev, "sdhci,auto-cmd23-broken"))
+		host->quirks2 |= SDHCI_QUIRK2_ACMD23_BROKEN;
+
 	if (device_property_present(dev, "sdhci,1-bit-only") ||
 	    (device_property_read_u32(dev, "bus-width", &bus_width) == 0 &&
 	    bus_width == 1))