[PATCH v4 0/4] rockchip: sdhci: Fix reinit and add HS400 Enhanced Strobe support

[PATCH v4 0/4] rockchip: sdhci: Fix reinit and add HS400 Enhanced Strobe support

[Alper Nebi Yasak]

My rk3399-gru-kevin has some problems with the eMMC. The board can boot
to U-Boot proper with the eMMC working at a low speed, but trying to
reinitialize it with "mmc dev 0" or "mmc rescan" makes it unusable. If
the HS400 mode is enabled, it times out while executing tuning and
doesn't even start at a working state.

To work around these errors, I had implemented support for the HS400
Enhanced Strobe mode as the first version of this series. I have also
managed the fix the issue above (related to power-cycling the eMMC PHY),
which exposed another one with this series: reinitialization at lower
speeds fail if the ES bit is set. Since fixing that needed changes to
this series I decided to send the previous fix as part of this instead
of as an independent patch.

To test, I'm building with the following configs enabled:

    +CONFIG_MMC_SPEED_MODE_SET=y
     [...]
     CONFIG_MMC_PWRSEQ=y
    +CONFIG_MMC_IO_VOLTAGE=y
    +CONFIG_MMC_UHS_SUPPORT=y
    +CONFIG_MMC_HS400_ES_SUPPORT=y
    +CONFIG_MMC_HS400_SUPPORT=y
     CONFIG_MMC_DW=y
     CONFIG_MMC_DW_ROCKCHIP=y
     CONFIG_MMC_SDHCI=y
    +CONFIG_MMC_SDHCI_SDMA=y
     CONFIG_MMC_SDHCI_ROCKCHIP=y

and running roughly:

    $ mmc rescan [0|1|3|10|11|12]
    $ mmc info
    $ mmc part
    $ load mmc 0:1 0xd0000000 256MiB.bin
    $ load mmc 0:1 0xd0000000 16MiB.bin
    $ load mmc 0:1 0xd0000000 8MiB.bin

I used to test by loading different sizes from a very big file (~7GiB),
but that's slower than reading fixed-size files for some reason I don't
know. I thought loading full files would be a better test so I switched
to those. Here's the differences in info and speeds I get with this:

    Mode                   | Bus Speed    | Bus Width
    -----------------------+--------------+--------------
    MMC Legacy             | 25000000     | 8-bit
    MMC High Speed (26MHz) | 26000000     | 8-bit
    MMC High Speed (52MHz) | 52000000     | 8-bit
    HS200 (200MHz)         | 200000000    | 8-bit
    HS400 (200MHz)         | 200000000    | 8-bit DDR
    HS400ES (200MHz)       | 200000000    | 8-bit DDR

    Mode                   | 256 MiB Load | 16 MiB Load  | 8 MiB Load
    -----------------------+--------------+--------------+--------------
    MMC Legacy             | ~22.1  MiB/s | ~21.9  MiB/s | ~21.6  MiB/s
    MMC High Speed (26MHz) | ~22.1  MiB/s | ~21.9  MiB/s | ~21.6  MiB/s
    MMC High Speed (52MHz) | ~43.7  MiB/s | ~42.8  MiB/s | ~41.7  MiB/s
    HS200 (200MHz)         | ~161.2 MiB/s | ~149.5 MiB/s | ~137.9 MiB/s
    HS400 (200MHz)         | ~254.5 MiB/s | ~235.3 MiB/s | ~216.2 MiB/s
    HS400ES (200MHz)       | ~254.7 MiB/s | ~238.8 MiB/s | ~216.2 MiB/s

Hope I haven't missed anything. Enabling the configs above for each
board is left to board maintainers as I can't test on those boards.

As an aside, I want to further clean up this driver when I have the time
(it's a weird combination of what could be three different drivers), but
wanted to send this as it at least gets the driver to a working state.

Changes in v4:
- Add comment for SDHCI set_enhanced_strobe() operation
- Add comment for Rockchip SDHCI set_control_reg() driver data op
- Add comment for Rockchip SDHCI set_ios_post() driver data op
- Add comment for Rockchip SDHCI set_enhanced_strobe() driver data op

v3: https://patchwork.ozlabs.org/project/uboot/list/?series=281327&state=*

Changes in v3:
- Set DWCMSHC_CARD_IS_EMMC bit in rk3568_emmc_phy_init()

v2: https://patchwork.ozlabs.org/project/uboot/list/?series=280494&state=*

Changes in v2:
- Add patch to fix PHY power cycling at higher speeds
- Unset ES bit in rk3399 set_control_reg() to fix a reinit issue
- Don't use unnecessary & for function pointer in ops struct
- Rename rk3399_set_enhanced_strobe -> rk3399_sdhci_set_enhanced_strobe
- Rename rk3568_set_enhanced_strobe -> rk3568_sdhci_set_enhanced_strobe
- Let set_enhanced_strobe() unset the ES bit if mode is not HS400_ES
- Rewrote cover letter

v1: https://patchwork.ozlabs.org/project/uboot/list/?series=269768&state=*

Alper Nebi Yasak (4):
  mmc: sdhci: Add HS400 Enhanced Strobe support
  rockchip: sdhci: Fix RK3399 eMMC PHY power cycling
  rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3399
  rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3568

 drivers/mmc/rockchip_sdhci.c | 171 +++++++++++++++++++++++++++++++++--
 drivers/mmc/sdhci.c          |  18 ++++
 include/sdhci.h              |  12 +++
 3 files changed, 191 insertions(+), 10 deletions(-)

-- 
2.34.1

[PATCH v4 1/4] mmc: sdhci: Add HS400 Enhanced Strobe support

[Alper Nebi Yasak]

Delegate setting the Enhanced Strobe configuration to individual drivers
if they set a function for it. Return -ENOTSUPP if they do not, like
what the MMC uclass does.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
---

Changes in v4:
- Add comment for SDHCI set_enhanced_strobe() operation

Changes in v2:
- Add tag: "Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>"

 drivers/mmc/sdhci.c | 18 ++++++++++++++++++
 include/sdhci.h     | 12 ++++++++++++
 2 files changed, 30 insertions(+)

diff --git a/drivers/mmc/sdhci.c b/drivers/mmc/sdhci.c
index 766e4a6b0c5e..bf989a594f7e 100644
--- a/drivers/mmc/sdhci.c
+++ b/drivers/mmc/sdhci.c
@@ -513,6 +513,7 @@ void sdhci_set_uhs_timing(struct sdhci_host *host)
 		reg |= SDHCI_CTRL_UHS_SDR104;
 		break;
 	case MMC_HS_400:
+	case MMC_HS_400_ES:
 		reg |= SDHCI_CTRL_HS400;
 		break;
 	default:
@@ -666,6 +667,7 @@ static int sdhci_set_ios(struct mmc *mmc)
 		    mmc->selected_mode == MMC_DDR_52 ||
 		    mmc->selected_mode == MMC_HS_200 ||
 		    mmc->selected_mode == MMC_HS_400 ||
+		    mmc->selected_mode == MMC_HS_400_ES ||
 		    mmc->selected_mode == UHS_SDR25 ||
 		    mmc->selected_mode == UHS_SDR50 ||
 		    mmc->selected_mode == UHS_SDR104 ||
@@ -799,6 +801,19 @@ static int sdhci_wait_dat0(struct udevice *dev, int state,
 	return -ETIMEDOUT;
 }
 
+#if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
+static int sdhci_set_enhanced_strobe(struct udevice *dev)
+{
+	struct mmc *mmc = mmc_get_mmc_dev(dev);
+	struct sdhci_host *host = mmc->priv;
+
+	if (host->ops && host->ops->set_enhanced_strobe)
+		return host->ops->set_enhanced_strobe(host);
+
+	return -ENOTSUPP;
+}
+#endif
+
 const struct dm_mmc_ops sdhci_ops = {
 	.send_cmd	= sdhci_send_command,
 	.set_ios	= sdhci_set_ios,
@@ -808,6 +823,9 @@ const struct dm_mmc_ops sdhci_ops = {
 	.execute_tuning	= sdhci_execute_tuning,
 #endif
 	.wait_dat0	= sdhci_wait_dat0,
+#if CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT)
+	.set_enhanced_strobe = sdhci_set_enhanced_strobe,
+#endif
 };
 #else
 static const struct mmc_ops sdhci_ops = {
diff --git a/include/sdhci.h b/include/sdhci.h
index c8d69f5a63f7..88f1917480b6 100644
--- a/include/sdhci.h
+++ b/include/sdhci.h
@@ -272,6 +272,18 @@ struct sdhci_ops {
 	int (*platform_execute_tuning)(struct mmc *host, u8 opcode);
 	int (*set_delay)(struct sdhci_host *host);
 	int	(*deferred_probe)(struct sdhci_host *host);
+
+	/**
+	 * set_enhanced_strobe() - Set HS400 Enhanced Strobe config
+	 *
+	 * This is called after setting the card speed and mode to
+	 * HS400 ES, and should set any host-specific configuration
+	 * necessary for it.
+	 *
+	 * @host: SDHCI host structure
+	 * Return: 0 if successful, -ve on error
+	 */
+	int	(*set_enhanced_strobe)(struct sdhci_host *host);
 };
 
 #define ADMA_MAX_LEN	65532
-- 
2.34.1

[PATCH v4 2/4] rockchip: sdhci: Fix RK3399 eMMC PHY power cycling

[Alper Nebi Yasak]

The Rockchip RK3399 eMMC PHY has to be power-cycled while changing its
clock speed to some higher speeds. This is dependent on the desired
SDHCI clock speed, and it looks like the PHY should be powered off while
setting the SDHCI clock in these cases.

Commit ac804143cfd1 ("mmc: rockchip_sdhci: add phy and clock config for
rk3399") attempts to do this in the set_ios_post() hook by setting the
SDHCI clock once more while the PHY is turned off/on as necessary, as
the SDHCI framework does not provide a way to override how it sets its
clock. However, the commit breaks reinitializing the eMMC on a few
boards including chromebook_kevin and reportedly ROCKPro64.

This patch reworks the power cycling to utilize the SDHCI framework
slightly better (using the set_control_reg() hook to power off the PHY
and set_ios_post() hook to power it back on) which happens to fix the
issue, at least on a chromebook_kevin.

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
RK3568 parts only build-tested as I don't have a RK3568 board.

Changes in v4:
- Add comment for Rockchip SDHCI set_control_reg() driver data op
- Add comment for Rockchip SDHCI set_ios_post() driver data op

Changes in v2:
- Add this patch

 drivers/mmc/rockchip_sdhci.c | 76 +++++++++++++++++++++++++++++++-----
 1 file changed, 66 insertions(+), 10 deletions(-)

diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index 278473899c7c..b91df05de4ff 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -90,9 +90,33 @@ struct rockchip_sdhc {
 };
 
 struct sdhci_data {
-	int (*emmc_set_clock)(struct sdhci_host *host, unsigned int clock);
 	int (*emmc_phy_init)(struct udevice *dev);
 	int (*get_phy)(struct udevice *dev);
+
+	/**
+	 * set_control_reg() - Set SDHCI control registers
+	 *
+	 * This is the set_control_reg() SDHCI operation that should be
+	 * used for the hardware this driver data is associated with.
+	 * Normally, this is used to set up control registers for
+	 * voltage level and UHS speed mode.
+	 *
+	 * @host: SDHCI host structure
+	 */
+	void (*set_control_reg)(struct sdhci_host *host);
+
+	/**
+	 * set_ios_post() - Host specific hook after set_ios() calls
+	 *
+	 * This is the set_ios_post() SDHCI operation that should be
+	 * used for the hardware this driver data is associated with.
+	 * Normally, this is a hook that is called after sdhci_set_ios()
+	 * that does any necessary host-specific configuration.
+	 *
+	 * @host: SDHCI host structure
+	 * Return: 0 if successful, -ve on error
+	 */
+	int (*set_ios_post)(struct sdhci_host *host);
 };
 
 static int rk3399_emmc_phy_init(struct udevice *dev)
@@ -182,15 +206,28 @@ static int rk3399_emmc_get_phy(struct udevice *dev)
 	return 0;
 }
 
-static int rk3399_sdhci_emmc_set_clock(struct sdhci_host *host, unsigned int clock)
+static void rk3399_sdhci_set_control_reg(struct sdhci_host *host)
 {
 	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
+	struct mmc *mmc = host->mmc;
+	uint clock = mmc->tran_speed;
 	int cycle_phy = host->clock != clock && clock > EMMC_MIN_FREQ;
 
 	if (cycle_phy)
 		rk3399_emmc_phy_power_off(priv->phy);
 
-	sdhci_set_clock(host->mmc, clock);
+	sdhci_set_control_reg(host);
+};
+
+static int rk3399_sdhci_set_ios_post(struct sdhci_host *host)
+{
+	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
+	struct mmc *mmc = host->mmc;
+	uint clock = mmc->tran_speed;
+	int cycle_phy = host->clock != clock && clock > EMMC_MIN_FREQ;
+
+	if (!clock)
+		clock = mmc->clock;
 
 	if (cycle_phy)
 		rk3399_emmc_phy_power_on(priv->phy, clock);
@@ -269,10 +306,8 @@ static int rk3568_emmc_get_phy(struct udevice *dev)
 	return 0;
 }
 
-static int rockchip_sdhci_set_ios_post(struct sdhci_host *host)
+static int rk3568_sdhci_set_ios_post(struct sdhci_host *host)
 {
-	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
-	struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
 	struct mmc *mmc = host->mmc;
 	uint clock = mmc->tran_speed;
 	u32 reg;
@@ -280,8 +315,7 @@ static int rockchip_sdhci_set_ios_post(struct sdhci_host *host)
 	if (!clock)
 		clock = mmc->clock;
 
-	if (data->emmc_set_clock)
-		data->emmc_set_clock(host, clock);
+	rk3568_sdhci_emmc_set_clock(host, clock);
 
 	if (mmc->selected_mode == MMC_HS_400 || mmc->selected_mode == MMC_HS_400_ES) {
 		reg = sdhci_readw(host, SDHCI_HOST_CONTROL2);
@@ -295,6 +329,26 @@ static int rockchip_sdhci_set_ios_post(struct sdhci_host *host)
 	return 0;
 }
 
+static void rockchip_sdhci_set_control_reg(struct sdhci_host *host)
+{
+	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
+	struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
+
+	if (data->set_control_reg)
+		data->set_control_reg(host);
+}
+
+static int rockchip_sdhci_set_ios_post(struct sdhci_host *host)
+{
+	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
+	struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
+
+	if (data->set_ios_post)
+		return data->set_ios_post(host);
+
+	return 0;
+}
+
 static int rockchip_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 {
 	struct sdhci_host *host = dev_get_priv(mmc->dev);
@@ -358,6 +412,7 @@ static int rockchip_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 static struct sdhci_ops rockchip_sdhci_ops = {
 	.set_ios_post	= rockchip_sdhci_set_ios_post,
 	.platform_execute_tuning = &rockchip_sdhci_execute_tuning,
+	.set_control_reg = rockchip_sdhci_set_control_reg,
 };
 
 static int rockchip_sdhci_probe(struct udevice *dev)
@@ -436,15 +491,16 @@ static int rockchip_sdhci_bind(struct udevice *dev)
 }
 
 static const struct sdhci_data rk3399_data = {
-	.emmc_set_clock = rk3399_sdhci_emmc_set_clock,
 	.get_phy = rk3399_emmc_get_phy,
 	.emmc_phy_init = rk3399_emmc_phy_init,
+	.set_control_reg = rk3399_sdhci_set_control_reg,
+	.set_ios_post = rk3399_sdhci_set_ios_post,
 };
 
 static const struct sdhci_data rk3568_data = {
-	.emmc_set_clock = rk3568_sdhci_emmc_set_clock,
 	.get_phy = rk3568_emmc_get_phy,
 	.emmc_phy_init = rk3568_emmc_phy_init,
+	.set_ios_post = rk3568_sdhci_set_ios_post,
 };
 
 static const struct udevice_id sdhci_ids[] = {
-- 
2.34.1

[PATCH v4 3/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3399

[Alper Nebi Yasak]

On RK3399, a register bit must be set to enable Enhanced Strobe.
Let the Rockchip SDHCI driver set it when Enhanced Strobe configuration
is requested. However, having it set makes the lower-speed modes stop
working and makes reinitialization fail, so let it be unset as needed in
set_control_reg().

This is mostly ported from Linux's Arasan SDHCI driver which happens
to be the underlying IP. (drivers/mmc/host/sdhci-of-arasan.c in Linux
tree).

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---

Changes in v4:
- Add comment for Rockchip SDHCI set_enhanced_strobe() driver data op

Changes in v2:
- Unset ES bit in rk3399 set_control_reg() to fix a reinit issue
- Don't use unnecessary & for function pointer in ops struct
- Rename rk3399_set_enhanced_strobe -> rk3399_sdhci_set_enhanced_strobe
- Let set_enhanced_strobe() unset the ES bit if mode is not HS400_ES

 drivers/mmc/rockchip_sdhci.c | 53 ++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index b91df05de4ff..f4d5a59036a2 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -42,6 +42,9 @@
 	((((x) >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK) ==\
 	PHYCTRL_DLLRDY_DONE)
 
+#define ARASAN_VENDOR_REGISTER		0x78
+#define ARASAN_VENDOR_ENHANCED_STROBE	BIT(0)
+
 /* Rockchip specific Registers */
 #define DWCMSHC_EMMC_DLL_CTRL		0x800
 #define DWCMSHC_EMMC_DLL_CTRL_RESET	BIT(1)
@@ -117,6 +120,19 @@ struct sdhci_data {
 	 * Return: 0 if successful, -ve on error
 	 */
 	int (*set_ios_post)(struct sdhci_host *host);
+
+	/**
+	 * set_enhanced_strobe() - Set HS400 Enhanced Strobe config
+	 *
+	 * This is the set_enhanced_strobe() SDHCI operation that should
+	 * be used for the hardware this driver data is associated with.
+	 * Normally, this is used to set any host-specific configuration
+	 * necessary for HS400 ES.
+	 *
+	 * @host: SDHCI host structure
+	 * Return: 0 if successful, -ve on error
+	 */
+	int (*set_enhanced_strobe)(struct sdhci_host *host);
 };
 
 static int rk3399_emmc_phy_init(struct udevice *dev)
@@ -206,6 +222,21 @@ static int rk3399_emmc_get_phy(struct udevice *dev)
 	return 0;
 }
 
+static int rk3399_sdhci_set_enhanced_strobe(struct sdhci_host *host)
+{
+	struct mmc *mmc = host->mmc;
+	u32 vendor;
+
+	vendor = sdhci_readl(host, ARASAN_VENDOR_REGISTER);
+	if (mmc->selected_mode == MMC_HS_400_ES)
+		vendor |= ARASAN_VENDOR_ENHANCED_STROBE;
+	else
+		vendor &= ~ARASAN_VENDOR_ENHANCED_STROBE;
+	sdhci_writel(host, vendor, ARASAN_VENDOR_REGISTER);
+
+	return 0;
+}
+
 static void rk3399_sdhci_set_control_reg(struct sdhci_host *host)
 {
 	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
@@ -217,6 +248,15 @@ static void rk3399_sdhci_set_control_reg(struct sdhci_host *host)
 		rk3399_emmc_phy_power_off(priv->phy);
 
 	sdhci_set_control_reg(host);
+
+	/*
+	 * Reinitializing the device tries to set it to lower-speed modes
+	 * first, which fails if the Enhanced Strobe bit is set, making
+	 * the device impossible to use. Set the correct value here to
+	 * let reinitialization attempts succeed.
+	 */
+	if (CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT))
+		rk3399_sdhci_set_enhanced_strobe(host);
 };
 
 static int rk3399_sdhci_set_ios_post(struct sdhci_host *host)
@@ -409,10 +449,22 @@ static int rockchip_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
 	return ret;
 }
 
+static int rockchip_sdhci_set_enhanced_strobe(struct sdhci_host *host)
+{
+	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
+	struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
+
+	if (data->set_enhanced_strobe)
+		return data->set_enhanced_strobe(host);
+
+	return -ENOTSUPP;
+}
+
 static struct sdhci_ops rockchip_sdhci_ops = {
 	.set_ios_post	= rockchip_sdhci_set_ios_post,
 	.platform_execute_tuning = &rockchip_sdhci_execute_tuning,
 	.set_control_reg = rockchip_sdhci_set_control_reg,
+	.set_enhanced_strobe = rockchip_sdhci_set_enhanced_strobe,
 };
 
 static int rockchip_sdhci_probe(struct udevice *dev)
@@ -495,6 +547,7 @@ static const struct sdhci_data rk3399_data = {
 	.emmc_phy_init = rk3399_emmc_phy_init,
 	.set_control_reg = rk3399_sdhci_set_control_reg,
 	.set_ios_post = rk3399_sdhci_set_ios_post,
+	.set_enhanced_strobe = rk3399_sdhci_set_enhanced_strobe,
 };
 
 static const struct sdhci_data rk3568_data = {
-- 
2.34.1

[PATCH v4 4/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3568

[Alper Nebi Yasak]

On RK3568, a register bit must be set to enable Enhanced Strobe.
However, it appears that the address of this register may differ from
vendor to vendor and should be read from the underlying MMC IP. Let the
Rockchip SDHCI driver read this address and set the relevant bit when
Enhanced Strobe configuration is requested.

Additionally, a bit signifying that the connected hardware is an eMMC
chip must be set to enable Data Strobe for HS400 and HS400ES modes. Also
make the driver set this bit as appropriate.

This is partly ported from Linux's Synopsys DWC MSHC driver which
happens to be the underlying IP. (drivers/mmc/host/sdhci-of-dwcmshc.c in
Linux tree).

Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
---
Only build-tested as I don't have a RK3568 board.

(no changes since v3)

Changes in v3:
- Set DWCMSHC_CARD_IS_EMMC bit in rk3568_emmc_phy_init()

Changes in v2:
- Rename rk3568_set_enhanced_strobe -> rk3568_sdhci_set_enhanced_strobe
- Let set_enhanced_strobe() unset the ES bit if mode is not HS400_ES

 drivers/mmc/rockchip_sdhci.c | 42 ++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index f4d5a59036a2..1d96c4696b6f 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -45,6 +45,14 @@
 #define ARASAN_VENDOR_REGISTER		0x78
 #define ARASAN_VENDOR_ENHANCED_STROBE	BIT(0)
 
+/* DWC IP vendor area 1 pointer */
+#define DWCMSHC_P_VENDOR_AREA1		0xe8
+#define DWCMSHC_AREA1_MASK		GENMASK(11, 0)
+/* Offset inside the vendor area 1 */
+#define DWCMSHC_EMMC_CONTROL		0x2c
+#define DWCMSHC_CARD_IS_EMMC		BIT(0)
+#define DWCMSHC_ENHANCED_STROBE		BIT(8)
+
 /* Rockchip specific Registers */
 #define DWCMSHC_EMMC_DLL_CTRL		0x800
 #define DWCMSHC_EMMC_DLL_CTRL_RESET	BIT(1)
@@ -279,11 +287,25 @@ static int rk3568_emmc_phy_init(struct udevice *dev)
 {
 	struct rockchip_sdhc *prv = dev_get_priv(dev);
 	struct sdhci_host *host = &prv->host;
+	struct mmc *mmc = host->mmc;
 	u32 extra;
+	u32 vendor;
+	int reg;
 
 	extra = DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL;
 	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK);
 
+	/* set CARD_IS_EMMC bit to enable Data Strobe for HS400 and HS400ES */
+	reg = (sdhci_readl(host, DWCMSHC_P_VENDOR_AREA1) & DWCMSHC_AREA1_MASK)
+	      + DWCMSHC_EMMC_CONTROL;
+
+	vendor = sdhci_readw(host, reg);
+	if (IS_MMC(mmc))
+		vendor |= DWCMSHC_CARD_IS_EMMC;
+	else
+		vendor &= ~DWCMSHC_CARD_IS_EMMC;
+	sdhci_writew(host, vendor, reg);
+
 	return 0;
 }
 
@@ -346,6 +368,25 @@ static int rk3568_emmc_get_phy(struct udevice *dev)
 	return 0;
 }
 
+static int rk3568_sdhci_set_enhanced_strobe(struct sdhci_host *host)
+{
+	struct mmc *mmc = host->mmc;
+	u32 vendor;
+	int reg;
+
+	reg = (sdhci_readl(host, DWCMSHC_P_VENDOR_AREA1) & DWCMSHC_AREA1_MASK)
+	      + DWCMSHC_EMMC_CONTROL;
+
+	vendor = sdhci_readl(host, reg);
+	if (mmc->selected_mode == MMC_HS_400_ES)
+		vendor |= DWCMSHC_ENHANCED_STROBE;
+	else
+		vendor &= ~DWCMSHC_ENHANCED_STROBE;
+	sdhci_writel(host, vendor, reg);
+
+	return 0;
+}
+
 static int rk3568_sdhci_set_ios_post(struct sdhci_host *host)
 {
 	struct mmc *mmc = host->mmc;
@@ -554,6 +595,7 @@ static const struct sdhci_data rk3568_data = {
 	.get_phy = rk3568_emmc_get_phy,
 	.emmc_phy_init = rk3568_emmc_phy_init,
 	.set_ios_post = rk3568_sdhci_set_ios_post,
+	.set_enhanced_strobe = rk3568_sdhci_set_enhanced_strobe,
 };
 
 static const struct udevice_id sdhci_ids[] = {
-- 
2.34.1

Re: [PATCH v4 3/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3399

[Jaehoon Chung]

On 1/29/22 07:42, Alper Nebi Yasak wrote:
> On RK3399, a register bit must be set to enable Enhanced Strobe.
> Let the Rockchip SDHCI driver set it when Enhanced Strobe configuration
> is requested. However, having it set makes the lower-speed modes stop
> working and makes reinitialization fail, so let it be unset as needed in
> set_control_reg().
> 
> This is mostly ported from Linux's Arasan SDHCI driver which happens
> to be the underlying IP. (drivers/mmc/host/sdhci-of-arasan.c in Linux
> tree).
> 
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> ---
> 
> Changes in v4:
> - Add comment for Rockchip SDHCI set_enhanced_strobe() driver data op
> 
> Changes in v2:
> - Unset ES bit in rk3399 set_control_reg() to fix a reinit issue
> - Don't use unnecessary & for function pointer in ops struct
> - Rename rk3399_set_enhanced_strobe -> rk3399_sdhci_set_enhanced_strobe
> - Let set_enhanced_strobe() unset the ES bit if mode is not HS400_ES
> 
>  drivers/mmc/rockchip_sdhci.c | 53 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 53 insertions(+)
> 
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index b91df05de4ff..f4d5a59036a2 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -42,6 +42,9 @@
>  	((((x) >> PHYCTRL_DLLRDY_SHIFT) & PHYCTRL_DLLRDY_MASK) ==\
>  	PHYCTRL_DLLRDY_DONE)
>  
> +#define ARASAN_VENDOR_REGISTER		0x78
> +#define ARASAN_VENDOR_ENHANCED_STROBE	BIT(0)
> +
>  /* Rockchip specific Registers */
>  #define DWCMSHC_EMMC_DLL_CTRL		0x800
>  #define DWCMSHC_EMMC_DLL_CTRL_RESET	BIT(1)
> @@ -117,6 +120,19 @@ struct sdhci_data {
>  	 * Return: 0 if successful, -ve on error
>  	 */
>  	int (*set_ios_post)(struct sdhci_host *host);
> +
> +	/**
> +	 * set_enhanced_strobe() - Set HS400 Enhanced Strobe config
> +	 *
> +	 * This is the set_enhanced_strobe() SDHCI operation that should
> +	 * be used for the hardware this driver data is associated with.
> +	 * Normally, this is used to set any host-specific configuration
> +	 * necessary for HS400 ES.
> +	 *
> +	 * @host: SDHCI host structure
> +	 * Return: 0 if successful, -ve on error
> +	 */
> +	int (*set_enhanced_strobe)(struct sdhci_host *host);
>  };
>  
>  static int rk3399_emmc_phy_init(struct udevice *dev)
> @@ -206,6 +222,21 @@ static int rk3399_emmc_get_phy(struct udevice *dev)
>  	return 0;
>  }
>  
> +static int rk3399_sdhci_set_enhanced_strobe(struct sdhci_host *host)
> +{
> +	struct mmc *mmc = host->mmc;
> +	u32 vendor;
> +
> +	vendor = sdhci_readl(host, ARASAN_VENDOR_REGISTER);
> +	if (mmc->selected_mode == MMC_HS_400_ES)
> +		vendor |= ARASAN_VENDOR_ENHANCED_STROBE;
> +	else
> +		vendor &= ~ARASAN_VENDOR_ENHANCED_STROBE;
> +	sdhci_writel(host, vendor, ARASAN_VENDOR_REGISTER);
> +
> +	return 0;
> +}
> +
>  static void rk3399_sdhci_set_control_reg(struct sdhci_host *host)
>  {
>  	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
> @@ -217,6 +248,15 @@ static void rk3399_sdhci_set_control_reg(struct sdhci_host *host)
>  		rk3399_emmc_phy_power_off(priv->phy);
>  
>  	sdhci_set_control_reg(host);
> +
> +	/*
> +	 * Reinitializing the device tries to set it to lower-speed modes
> +	 * first, which fails if the Enhanced Strobe bit is set, making
> +	 * the device impossible to use. Set the correct value here to
> +	 * let reinitialization attempts succeed.
> +	 */
> +	if (CONFIG_IS_ENABLED(MMC_HS400_ES_SUPPORT))
> +		rk3399_sdhci_set_enhanced_strobe(host);
>  };
>  
>  static int rk3399_sdhci_set_ios_post(struct sdhci_host *host)
> @@ -409,10 +449,22 @@ static int rockchip_sdhci_execute_tuning(struct mmc *mmc, u8 opcode)
>  	return ret;
>  }
>  
> +static int rockchip_sdhci_set_enhanced_strobe(struct sdhci_host *host)
> +{
> +	struct rockchip_sdhc *priv = container_of(host, struct rockchip_sdhc, host);
> +	struct sdhci_data *data = (struct sdhci_data *)dev_get_driver_data(priv->dev);
> +
> +	if (data->set_enhanced_strobe)
> +		return data->set_enhanced_strobe(host);
> +
> +	return -ENOTSUPP;
> +}
> +
>  static struct sdhci_ops rockchip_sdhci_ops = {
>  	.set_ios_post	= rockchip_sdhci_set_ios_post,
>  	.platform_execute_tuning = &rockchip_sdhci_execute_tuning,
>  	.set_control_reg = rockchip_sdhci_set_control_reg,
> +	.set_enhanced_strobe = rockchip_sdhci_set_enhanced_strobe,
>  };
>  
>  static int rockchip_sdhci_probe(struct udevice *dev)
> @@ -495,6 +547,7 @@ static const struct sdhci_data rk3399_data = {
>  	.emmc_phy_init = rk3399_emmc_phy_init,
>  	.set_control_reg = rk3399_sdhci_set_control_reg,
>  	.set_ios_post = rk3399_sdhci_set_ios_post,
> +	.set_enhanced_strobe = rk3399_sdhci_set_enhanced_strobe,
>  };
>  
>  static const struct sdhci_data rk3568_data = {

Re: [PATCH v4 4/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3568

[Jaehoon Chung]

On 1/29/22 07:42, Alper Nebi Yasak wrote:
> On RK3568, a register bit must be set to enable Enhanced Strobe.
> However, it appears that the address of this register may differ from
> vendor to vendor and should be read from the underlying MMC IP. Let the
> Rockchip SDHCI driver read this address and set the relevant bit when
> Enhanced Strobe configuration is requested.
> 
> Additionally, a bit signifying that the connected hardware is an eMMC
> chip must be set to enable Data Strobe for HS400 and HS400ES modes. Also
> make the driver set this bit as appropriate.
> 
> This is partly ported from Linux's Synopsys DWC MSHC driver which
> happens to be the underlying IP. (drivers/mmc/host/sdhci-of-dwcmshc.c in
> Linux tree).
> 
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

As you mentioned, if someone test this on RK3568 board, it will be more better.

Best Regards,
Jaehoon Chung

> ---
> Only build-tested as I don't have a RK3568 board.
> 
> (no changes since v3)
> 
> Changes in v3:
> - Set DWCMSHC_CARD_IS_EMMC bit in rk3568_emmc_phy_init()
> 
> Changes in v2:
> - Rename rk3568_set_enhanced_strobe -> rk3568_sdhci_set_enhanced_strobe
> - Let set_enhanced_strobe() unset the ES bit if mode is not HS400_ES
> 
>  drivers/mmc/rockchip_sdhci.c | 42 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 42 insertions(+)
> 
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index f4d5a59036a2..1d96c4696b6f 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -45,6 +45,14 @@
>  #define ARASAN_VENDOR_REGISTER		0x78
>  #define ARASAN_VENDOR_ENHANCED_STROBE	BIT(0)
>  
> +/* DWC IP vendor area 1 pointer */
> +#define DWCMSHC_P_VENDOR_AREA1		0xe8
> +#define DWCMSHC_AREA1_MASK		GENMASK(11, 0)
> +/* Offset inside the vendor area 1 */
> +#define DWCMSHC_EMMC_CONTROL		0x2c
> +#define DWCMSHC_CARD_IS_EMMC		BIT(0)
> +#define DWCMSHC_ENHANCED_STROBE		BIT(8)
> +
>  /* Rockchip specific Registers */
>  #define DWCMSHC_EMMC_DLL_CTRL		0x800
>  #define DWCMSHC_EMMC_DLL_CTRL_RESET	BIT(1)
> @@ -279,11 +287,25 @@ static int rk3568_emmc_phy_init(struct udevice *dev)
>  {
>  	struct rockchip_sdhc *prv = dev_get_priv(dev);
>  	struct sdhci_host *host = &prv->host;
> +	struct mmc *mmc = host->mmc;
>  	u32 extra;
> +	u32 vendor;
> +	int reg;
>  
>  	extra = DLL_RXCLK_NO_INVERTER << DWCMSHC_EMMC_DLL_RXCLK_SRCSEL;
>  	sdhci_writel(host, extra, DWCMSHC_EMMC_DLL_RXCLK);
>  
> +	/* set CARD_IS_EMMC bit to enable Data Strobe for HS400 and HS400ES */
> +	reg = (sdhci_readl(host, DWCMSHC_P_VENDOR_AREA1) & DWCMSHC_AREA1_MASK)
> +	      + DWCMSHC_EMMC_CONTROL;
> +
> +	vendor = sdhci_readw(host, reg);
> +	if (IS_MMC(mmc))
> +		vendor |= DWCMSHC_CARD_IS_EMMC;
> +	else
> +		vendor &= ~DWCMSHC_CARD_IS_EMMC;
> +	sdhci_writew(host, vendor, reg);
> +
>  	return 0;
>  }
>  
> @@ -346,6 +368,25 @@ static int rk3568_emmc_get_phy(struct udevice *dev)
>  	return 0;
>  }
>  
> +static int rk3568_sdhci_set_enhanced_strobe(struct sdhci_host *host)
> +{
> +	struct mmc *mmc = host->mmc;
> +	u32 vendor;
> +	int reg;
> +
> +	reg = (sdhci_readl(host, DWCMSHC_P_VENDOR_AREA1) & DWCMSHC_AREA1_MASK)
> +	      + DWCMSHC_EMMC_CONTROL;
> +
> +	vendor = sdhci_readl(host, reg);
> +	if (mmc->selected_mode == MMC_HS_400_ES)
> +		vendor |= DWCMSHC_ENHANCED_STROBE;
> +	else
> +		vendor &= ~DWCMSHC_ENHANCED_STROBE;
> +	sdhci_writel(host, vendor, reg);
> +
> +	return 0;
> +}
> +
>  static int rk3568_sdhci_set_ios_post(struct sdhci_host *host)
>  {
>  	struct mmc *mmc = host->mmc;
> @@ -554,6 +595,7 @@ static const struct sdhci_data rk3568_data = {
>  	.get_phy = rk3568_emmc_get_phy,
>  	.emmc_phy_init = rk3568_emmc_phy_init,
>  	.set_ios_post = rk3568_sdhci_set_ios_post,
> +	.set_enhanced_strobe = rk3568_sdhci_set_enhanced_strobe,
>  };
>  
>  static const struct udevice_id sdhci_ids[] = {

Re: [PATCH v4 4/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3568

[Simon Glass]

Hi Jaehoon,

On Tue, 8 Feb 2022 at 16:42, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>
> On 1/29/22 07:42, Alper Nebi Yasak wrote:
> > On RK3568, a register bit must be set to enable Enhanced Strobe.
> > However, it appears that the address of this register may differ from
> > vendor to vendor and should be read from the underlying MMC IP. Let the
> > Rockchip SDHCI driver read this address and set the relevant bit when
> > Enhanced Strobe configuration is requested.
> >
> > Additionally, a bit signifying that the connected hardware is an eMMC
> > chip must be set to enable Data Strobe for HS400 and HS400ES modes. Also
> > make the driver set this bit as appropriate.
> >
> > This is partly ported from Linux's Synopsys DWC MSHC driver which
> > happens to be the underlying IP. (drivers/mmc/host/sdhci-of-dwcmshc.c in
> > Linux tree).
> >
> > Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>
> As you mentioned, if someone test this on RK3568 board, it will be more better.

That does not seem to have happened, but it will likely happen when it
is applied.

Regards,
Simon

Re: [PATCH v4 2/4] rockchip: sdhci: Fix RK3399 eMMC PHY power cycling

[Simon Glass]

On Fri, 28 Jan 2022 at 15:43, Alper Nebi Yasak <alpernebiyasak@gmail.com> wrote:
>
> The Rockchip RK3399 eMMC PHY has to be power-cycled while changing its
> clock speed to some higher speeds. This is dependent on the desired
> SDHCI clock speed, and it looks like the PHY should be powered off while
> setting the SDHCI clock in these cases.
>
> Commit ac804143cfd1 ("mmc: rockchip_sdhci: add phy and clock config for
> rk3399") attempts to do this in the set_ios_post() hook by setting the
> SDHCI clock once more while the PHY is turned off/on as necessary, as
> the SDHCI framework does not provide a way to override how it sets its
> clock. However, the commit breaks reinitializing the eMMC on a few
> boards including chromebook_kevin and reportedly ROCKPro64.
>
> This patch reworks the power cycling to utilize the SDHCI framework
> slightly better (using the set_control_reg() hook to power off the PHY
> and set_ios_post() hook to power it back on) which happens to fix the
> issue, at least on a chromebook_kevin.
>
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> ---
> RK3568 parts only build-tested as I don't have a RK3568 board.
>
> Changes in v4:
> - Add comment for Rockchip SDHCI set_control_reg() driver data op
> - Add comment for Rockchip SDHCI set_ios_post() driver data op
>
> Changes in v2:
> - Add this patch
>
>  drivers/mmc/rockchip_sdhci.c | 76 +++++++++++++++++++++++++++++++-----
>  1 file changed, 66 insertions(+), 10 deletions(-)

Reviewed-by: Simon Glass <sjg@chromium.org>

Re: [PATCH v4 4/4] rockchip: sdhci: Add HS400 Enhanced Strobe support for RK3568

[Jaehoon Chung]

Hi Simon,

On 2/27/22 03:36, Simon Glass wrote:
> Hi Jaehoon,
>
> On Tue, 8 Feb 2022 at 16:42, Jaehoon Chung <jh80.chung@samsung.com> wrote:
>> On 1/29/22 07:42, Alper Nebi Yasak wrote:
>>> On RK3568, a register bit must be set to enable Enhanced Strobe.
>>> However, it appears that the address of this register may differ from
>>> vendor to vendor and should be read from the underlying MMC IP. Let the
>>> Rockchip SDHCI driver read this address and set the relevant bit when
>>> Enhanced Strobe configuration is requested.
>>>
>>> Additionally, a bit signifying that the connected hardware is an eMMC
>>> chip must be set to enable Data Strobe for HS400 and HS400ES modes. Also
>>> make the driver set this bit as appropriate.
>>>
>>> This is partly ported from Linux's Synopsys DWC MSHC driver which
>>> happens to be the underlying IP. (drivers/mmc/host/sdhci-of-dwcmshc.c in
>>> Linux tree).
>>>
>>> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>
>> As you mentioned, if someone test this on RK3568 board, it will be more better.
> That does not seem to have happened, but it will likely happen when it
> is applied.

Agreed. I will apply this on u-boot-mmc.

Best Regards,
Jaehoon Chung

>
> Regards,
> Simon

Re: [PATCH v4 2/4] rockchip: sdhci: Fix RK3399 eMMC PHY power cycling

[Tom Rini]

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

On Sat, Jan 29, 2022 at 01:42:37AM +0300, Alper Nebi Yasak wrote:

> The Rockchip RK3399 eMMC PHY has to be power-cycled while changing its
> clock speed to some higher speeds. This is dependent on the desired
> SDHCI clock speed, and it looks like the PHY should be powered off while
> setting the SDHCI clock in these cases.
> 
> Commit ac804143cfd1 ("mmc: rockchip_sdhci: add phy and clock config for
> rk3399") attempts to do this in the set_ios_post() hook by setting the
> SDHCI clock once more while the PHY is turned off/on as necessary, as
> the SDHCI framework does not provide a way to override how it sets its
> clock. However, the commit breaks reinitializing the eMMC on a few
> boards including chromebook_kevin and reportedly ROCKPro64.
> 
> This patch reworks the power cycling to utilize the SDHCI framework
> slightly better (using the set_control_reg() hook to power off the PHY
> and set_ios_post() hook to power it back on) which happens to fix the
> issue, at least on a chromebook_kevin.
> 
> Signed-off-by: Alper Nebi Yasak <alpernebiyasak@gmail.com>
> Reviewed-by: Simon Glass <sjg@chromium.org>

Applied to u-boot/master, thanks!

-- 
Tom

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