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

From: Linus Walleij <linus.walleij@linaro.org>
To: BOUGH CHEN <haibo.chen@nxp.com>
Cc: "ulf.hansson@linaro.org" <ulf.hansson@linaro.org>,
	"adrian.hunter@intel.com" <adrian.hunter@intel.com>,
	"shawnguo@kernel.org" <shawnguo@kernel.org>,
	"kernel@pengutronix.de" <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>
Subject: Re: [PATCH v2 10/14] mmc: sdhci-esdhc-imx: handle 'sdhci,auto-cmd23-broken' from devicetree
Date: Wed, 11 Dec 2019 00:38:38 +0100	[thread overview]
Message-ID: <CACRpkdbm75KX3bBH0gaGhiPTd=zD996qgu7vSHr1H3u_uSRo=g@mail.gmail.com> (raw)
In-Reply-To: <20191203130120.11511-11-haibo.chen@nxp.com>

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