[PATCH 00/12] mci: add HS200 support for eMMCs

[PATCH 00/12] mci: add HS200 support for eMMCs

[Steffen Trumtrar]

This series adds support for HS200 mode to mci-core and sdhci.
As the host driver also needs to handle clock setup, pin control and
host specific tuning, this series only adds HS200 support to the Arasan
SDHCI driver.

Tested on: ZynqMP.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
Steffen Trumtrar (12):
      ARM: zynqmp: add sd_dll_reset call
      zynqmp: firmware: add functions to set tap delay
      mci: arasan: implement 25MHz quirk for zynqmp
      mci: arasan: read clk phases from DT
      mci: arasan: register sdcard/sampleclk
      include: mci: sync mci_timing with linux
      include: mci: add more EXT_CSD_CARD_TYPE_*
      mci: core: parse more host capabilities from DT
      mci: mci-core: add HS200 support
      mci: sdhci: add tuning support
      mci: arasan-sdhci: add HS200 tuning support on ZynqMP
      mci: arasan: use sdhci_wait_idle2

 arch/arm/mach-zynqmp/firmware-zynqmp.c |  59 ++++
 drivers/mci/Kconfig                    |   7 +
 drivers/mci/arasan-sdhci.c             | 516 ++++++++++++++++++++++++++++++++-
 drivers/mci/mci-core.c                 | 290 +++++++++++++++++-
 drivers/mci/sdhci.c                    | 269 ++++++++++++++++-
 drivers/mci/sdhci.h                    |  25 ++
 include/mach/zynqmp/firmware-zynqmp.h  |  23 ++
 include/mci.h                          | 114 +++++++-
 8 files changed, 1270 insertions(+), 33 deletions(-)
---
base-commit: ed7c14536d521793199abf0597164a46ba68e8e5
change-id: 20240308-v2024-02-0-topic-arasan-hs200-support-1bc3fb6b49aa

Best regards,
-- 
Steffen Trumtrar <s.trumtrar@pengutronix.de>

[PATCH 01/12] ARM: zynqmp: add sd_dll_reset call

[Steffen Trumtrar]

Add a function to reset DLL logic for SD devices.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 arch/arm/mach-zynqmp/firmware-zynqmp.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm/mach-zynqmp/firmware-zynqmp.c b/arch/arm/mach-zynqmp/firmware-zynqmp.c
index a2b61efff4..b383ed6f00 100644
--- a/arch/arm/mach-zynqmp/firmware-zynqmp.c
+++ b/arch/arm/mach-zynqmp/firmware-zynqmp.c
@@ -511,6 +511,23 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
 				   arg1, arg2, out);
 }
 
+/**
+ * zynqmp_pm_sd_dll_reset() - Reset DLL logic
+ *
+ * @node_id:	Node ID of the device
+ * @type:	Reset type
+ *
+ * This function resets DLL logic for the SD device.
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_sd_dll_reset(u32 node_id, u32 type)
+{
+	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, IOCTL_SD_DLL_RESET,
+				   type, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_sd_dll_reset);
+
 /*
  * zynqmp_pm_write_ggs() - PM API for writing global general storage (ggs)
  * @index:	GGS register index

-- 
2.40.1

Re: [PATCH 01/12] ARM: zynqmp: add sd_dll_reset call

[Ahmad Fatoum]

On 08.03.24 12:16, Steffen Trumtrar wrote:
> Add a function to reset DLL logic for SD devices.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm/mach-zynqmp/firmware-zynqmp.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/arch/arm/mach-zynqmp/firmware-zynqmp.c b/arch/arm/mach-zynqmp/firmware-zynqmp.c
> index a2b61efff4..b383ed6f00 100644
> --- a/arch/arm/mach-zynqmp/firmware-zynqmp.c
> +++ b/arch/arm/mach-zynqmp/firmware-zynqmp.c
> @@ -511,6 +511,23 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
>  				   arg1, arg2, out);
>  }
>  
> +/**
> + * zynqmp_pm_sd_dll_reset() - Reset DLL logic
> + *
> + * @node_id:	Node ID of the device
> + * @type:	Reset type
> + *
> + * This function resets DLL logic for the SD device.
> + *
> + * Return:	Returns status, either success or error+reason
> + */
> +int zynqmp_pm_sd_dll_reset(u32 node_id, u32 type)
> +{
> +	return zynqmp_pm_invoke_fn(PM_IOCTL, node_id, IOCTL_SD_DLL_RESET,
> +				   type, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_sd_dll_reset);
> +
>  /*
>   * zynqmp_pm_write_ggs() - PM API for writing global general storage (ggs)
>   * @index:	GGS register index
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 02/12] zynqmp: firmware: add functions to set tap delay

[Steffen Trumtrar]

Add a function to set the tap delay for the clk phase of the sd host
controller.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 arch/arm/mach-zynqmp/firmware-zynqmp.c | 42 ++++++++++++++++++++++++++++++++++
 include/mach/zynqmp/firmware-zynqmp.h  | 23 +++++++++++++++++++
 2 files changed, 65 insertions(+)

diff --git a/arch/arm/mach-zynqmp/firmware-zynqmp.c b/arch/arm/mach-zynqmp/firmware-zynqmp.c
index b383ed6f00..039a46e767 100644
--- a/arch/arm/mach-zynqmp/firmware-zynqmp.c
+++ b/arch/arm/mach-zynqmp/firmware-zynqmp.c
@@ -48,6 +48,7 @@ enum pm_ret_status {
 
 enum pm_api_id {
 	PM_GET_API_VERSION = 1,
+	PM_MMIO_WRITE = 19,
 	PM_FPGA_LOAD = 22,
 	PM_FPGA_GET_STATUS,
 	PM_IOCTL = 34,
@@ -511,6 +512,47 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
 				   arg1, arg2, out);
 }
 
+/**
+ * zynqmp_pm_set_sd_tapdelay() -  Set tap delay for the SD device
+ *
+ * @node_id:	Node ID of the device
+ * @type:	Type of tap delay to set (input/output)
+ * @value:	Value to set fot the tap delay
+ *
+ * This function sets input/output tap delay for the SD device.
+ *
+ * Return:	Returns status, either success or error+reason
+ */
+int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value)
+{
+	u32 reg = (type == PM_TAPDELAY_INPUT) ? SD_ITAPDLY : SD_OTAPDLYSEL;
+	u32 mask = (node_id == NODE_SD_0) ? GENMASK(15, 0) : GENMASK(31, 16);
+
+	if (value) {
+		return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
+					   IOCTL_SET_SD_TAPDELAY,
+					   type, value, NULL);
+	}
+
+	/*
+	 * Work around completely misdesigned firmware API on Xilinx ZynqMP.
+	 * The IOCTL_SET_SD_TAPDELAY firmware call allows the caller to only
+	 * ever set IOU_SLCR SD_ITAPDLY Register SD0_ITAPDLYENA/SD1_ITAPDLYENA
+	 * bits, but there is no matching call to clear those bits. If those
+	 * bits are not cleared, SDMMC tuning may fail.
+	 *
+	 * Luckily, there are PM_MMIO_READ/PM_MMIO_WRITE calls which seem to
+	 * allow complete unrestricted access to all address space, including
+	 * IOU_SLCR SD_ITAPDLY Register and all the other registers, access
+	 * to which was supposed to be protected by the current firmware API.
+	 *
+	 * Use PM_MMIO_READ/PM_MMIO_WRITE to re-implement the missing counter
+	 * part of IOCTL_SET_SD_TAPDELAY which clears SDx_ITAPDLYENA bits.
+	 */
+	return zynqmp_pm_invoke_fn(PM_MMIO_WRITE, reg, mask, 0, 0, NULL);
+}
+EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_tapdelay);
+
 /**
  * zynqmp_pm_sd_dll_reset() - Reset DLL logic
  *
diff --git a/include/mach/zynqmp/firmware-zynqmp.h b/include/mach/zynqmp/firmware-zynqmp.h
index 630677285f..00c63058f4 100644
--- a/include/mach/zynqmp/firmware-zynqmp.h
+++ b/include/mach/zynqmp/firmware-zynqmp.h
@@ -27,6 +27,10 @@
 
 #define ZYNQMP_PCAP_STATUS_FPGA_DONE	BIT(3)
 
+/* ZynqMP SD tap delay tuning */
+#define SD_ITAPDLY	0xFF180314
+#define SD_OTAPDLYSEL	0xFF180318
+
 enum pm_ioctl_id {
 	IOCTL_GET_RPU_OPER_MODE = 0,
 	IOCTL_SET_RPU_OPER_MODE = 1,
@@ -80,6 +84,22 @@ struct zynqmp_pm_query_data {
 	u32 arg3;
 };
 
+enum pm_node_id {
+	NODE_SD_0 = 39,
+	NODE_SD_1 = 40,
+};
+
+enum tap_delay_type {
+	PM_TAPDELAY_INPUT = 0,
+	PM_TAPDELAY_OUTPUT = 1,
+};
+
+enum dll_reset_type {
+	PM_DLL_RESET_ASSERT = 0,
+	PM_DLL_RESET_RELEASE = 1,
+	PM_DLL_RESET_PULSE = 2,
+};
+
 struct zynqmp_eemi_ops {
 	int (*get_api_version)(u32 *version);
 	int (*query_data)(struct zynqmp_pm_query_data qdata, u32 *out);
@@ -99,6 +119,9 @@ struct zynqmp_eemi_ops {
 
 const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
 
+int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value);
+int zynqmp_pm_sd_dll_reset(u32 node_id, u32 type);
+
 int zynqmp_pm_write_ggs(u32 index, u32 value);
 int zynqmp_pm_read_ggs(u32 index, u32 *value);
 int zynqmp_pm_write_pggs(u32 index, u32 value);

-- 
2.40.1

Re: [PATCH 02/12] zynqmp: firmware: add functions to set tap delay

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> Add a function to set the tap delay for the clk phase of the sd host
> controller.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  arch/arm/mach-zynqmp/firmware-zynqmp.c | 42 ++++++++++++++++++++++++++++++++++
>  include/mach/zynqmp/firmware-zynqmp.h  | 23 +++++++++++++++++++
>  2 files changed, 65 insertions(+)
> 
> diff --git a/arch/arm/mach-zynqmp/firmware-zynqmp.c b/arch/arm/mach-zynqmp/firmware-zynqmp.c
> index b383ed6f00..039a46e767 100644
> --- a/arch/arm/mach-zynqmp/firmware-zynqmp.c
> +++ b/arch/arm/mach-zynqmp/firmware-zynqmp.c
> @@ -48,6 +48,7 @@ enum pm_ret_status {
>  
>  enum pm_api_id {
>  	PM_GET_API_VERSION = 1,
> +	PM_MMIO_WRITE = 19,
>  	PM_FPGA_LOAD = 22,
>  	PM_FPGA_GET_STATUS,
>  	PM_IOCTL = 34,
> @@ -511,6 +512,47 @@ static int zynqmp_pm_ioctl(u32 node_id, u32 ioctl_id, u32 arg1, u32 arg2,
>  				   arg1, arg2, out);
>  }
>  
> +/**
> + * zynqmp_pm_set_sd_tapdelay() -  Set tap delay for the SD device
> + *
> + * @node_id:	Node ID of the device
> + * @type:	Type of tap delay to set (input/output)
> + * @value:	Value to set fot the tap delay
> + *
> + * This function sets input/output tap delay for the SD device.
> + *
> + * Return:	Returns status, either success or error+reason
> + */
> +int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value)
> +{
> +	u32 reg = (type == PM_TAPDELAY_INPUT) ? SD_ITAPDLY : SD_OTAPDLYSEL;
> +	u32 mask = (node_id == NODE_SD_0) ? GENMASK(15, 0) : GENMASK(31, 16);
> +
> +	if (value) {
> +		return zynqmp_pm_invoke_fn(PM_IOCTL, node_id,
> +					   IOCTL_SET_SD_TAPDELAY,
> +					   type, value, NULL);
> +	}
> +
> +	/*
> +	 * Work around completely misdesigned firmware API on Xilinx ZynqMP.
> +	 * The IOCTL_SET_SD_TAPDELAY firmware call allows the caller to only
> +	 * ever set IOU_SLCR SD_ITAPDLY Register SD0_ITAPDLYENA/SD1_ITAPDLYENA
> +	 * bits, but there is no matching call to clear those bits. If those
> +	 * bits are not cleared, SDMMC tuning may fail.
> +	 *
> +	 * Luckily, there are PM_MMIO_READ/PM_MMIO_WRITE calls which seem to
> +	 * allow complete unrestricted access to all address space, including
> +	 * IOU_SLCR SD_ITAPDLY Register and all the other registers, access
> +	 * to which was supposed to be protected by the current firmware API.
> +	 *
> +	 * Use PM_MMIO_READ/PM_MMIO_WRITE to re-implement the missing counter
> +	 * part of IOCTL_SET_SD_TAPDELAY which clears SDx_ITAPDLYENA bits.
> +	 */
> +	return zynqmp_pm_invoke_fn(PM_MMIO_WRITE, reg, mask, 0, 0, NULL);
> +}
> +EXPORT_SYMBOL_GPL(zynqmp_pm_set_sd_tapdelay);
> +
>  /**
>   * zynqmp_pm_sd_dll_reset() - Reset DLL logic
>   *
> diff --git a/include/mach/zynqmp/firmware-zynqmp.h b/include/mach/zynqmp/firmware-zynqmp.h
> index 630677285f..00c63058f4 100644
> --- a/include/mach/zynqmp/firmware-zynqmp.h
> +++ b/include/mach/zynqmp/firmware-zynqmp.h
> @@ -27,6 +27,10 @@
>  
>  #define ZYNQMP_PCAP_STATUS_FPGA_DONE	BIT(3)
>  
> +/* ZynqMP SD tap delay tuning */
> +#define SD_ITAPDLY	0xFF180314
> +#define SD_OTAPDLYSEL	0xFF180318
> +
>  enum pm_ioctl_id {
>  	IOCTL_GET_RPU_OPER_MODE = 0,
>  	IOCTL_SET_RPU_OPER_MODE = 1,
> @@ -80,6 +84,22 @@ struct zynqmp_pm_query_data {
>  	u32 arg3;
>  };
>  
> +enum pm_node_id {
> +	NODE_SD_0 = 39,
> +	NODE_SD_1 = 40,
> +};
> +
> +enum tap_delay_type {
> +	PM_TAPDELAY_INPUT = 0,
> +	PM_TAPDELAY_OUTPUT = 1,
> +};
> +
> +enum dll_reset_type {
> +	PM_DLL_RESET_ASSERT = 0,
> +	PM_DLL_RESET_RELEASE = 1,
> +	PM_DLL_RESET_PULSE = 2,
> +};
> +
>  struct zynqmp_eemi_ops {
>  	int (*get_api_version)(u32 *version);
>  	int (*query_data)(struct zynqmp_pm_query_data qdata, u32 *out);
> @@ -99,6 +119,9 @@ struct zynqmp_eemi_ops {
>  
>  const struct zynqmp_eemi_ops *zynqmp_pm_get_eemi_ops(void);
>  
> +int zynqmp_pm_set_sd_tapdelay(u32 node_id, u32 type, u32 value);
> +int zynqmp_pm_sd_dll_reset(u32 node_id, u32 type);
> +
>  int zynqmp_pm_write_ggs(u32 index, u32 value);
>  int zynqmp_pm_read_ggs(u32 index, u32 *value);
>  int zynqmp_pm_write_pggs(u32 index, u32 value);
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 03/12] mci: arasan: implement 25MHz quirk for zynqmp

[Steffen Trumtrar]

The Arasan on the zynqmp in version 8.9a doesn't meet the timing
requirements at 25MHz. It works at 19MHz instead.

Add the quirk from linux kernel v6.8-rc4.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/arasan-sdhci.c | 28 +++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 59afac7372..cc1bad8b62 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -37,6 +37,12 @@ struct arasan_sdhci_host {
 /* Controller does not have CD wired and will not function normally without */
 #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST		BIT(0)
 #define SDHCI_ARASAN_QUIRK_NO_1_8_V		BIT(1)
+/*
+ * Some of the Arasan variations might not have timing requirements
+ * met at 25MHz for Default Speed mode, those controllers work at
+ * 19MHz instead
+ */
+#define SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN	BIT(2)
 };
 
 static inline
@@ -110,13 +116,30 @@ static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
 	return 0;
 }
 
+static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
+{
+	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
+
+	if (host->quirks & SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN) {
+		/*
+		 * Some of the Arasan variations might not have timing
+		 * requirements met at 25MHz for Default Speed mode,
+		 * those controllers work at 19MHz instead.
+		 */
+		if (clock == 25000000)
+			clock = (25000000 * 19) / 25;
+	}
+
+	sdhci_set_clock(&host->sdhci, clock, host->sdhci.max_clk);
+}
+
 static void arasan_sdhci_set_ios(struct mci_host *mci, struct mci_ios *ios)
 {
 	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
 	u16 val;
 
 	if (ios->clock)
-		sdhci_set_clock(&host->sdhci, ios->clock, host->sdhci.max_clk);
+		arasan_sdhci_set_clock(mci, ios->clock);
 
 	sdhci_set_bus_width(&host->sdhci, ios->bus_width);
 
@@ -243,6 +266,9 @@ static int arasan_sdhci_probe(struct device *dev)
 	if (of_property_read_bool(np, "no-1-8-v"))
 		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_NO_1_8_V;
 
+	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
+		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN;
+
 	arasan_sdhci->sdhci.base = IOMEM(iores->start);
 	arasan_sdhci->sdhci.mci = mci;
 	mci->send_cmd = arasan_sdhci_send_cmd;

-- 
2.40.1

Re: [PATCH 03/12] mci: arasan: implement 25MHz quirk for zynqmp

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> The Arasan on the zynqmp in version 8.9a doesn't meet the timing
> requirements at 25MHz. It works at 19MHz instead.
> 
> Add the quirk from linux kernel v6.8-rc4.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/mci/arasan-sdhci.c | 28 +++++++++++++++++++++++++++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
> index 59afac7372..cc1bad8b62 100644
> --- a/drivers/mci/arasan-sdhci.c
> +++ b/drivers/mci/arasan-sdhci.c
> @@ -37,6 +37,12 @@ struct arasan_sdhci_host {
>  /* Controller does not have CD wired and will not function normally without */
>  #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST		BIT(0)
>  #define SDHCI_ARASAN_QUIRK_NO_1_8_V		BIT(1)
> +/*
> + * Some of the Arasan variations might not have timing requirements
> + * met at 25MHz for Default Speed mode, those controllers work at
> + * 19MHz instead
> + */
> +#define SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN	BIT(2)
>  };
>  
>  static inline
> @@ -110,13 +116,30 @@ static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
>  	return 0;
>  }
>  
> +static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
> +{
> +	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
> +
> +	if (host->quirks & SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN) {
> +		/*
> +		 * Some of the Arasan variations might not have timing
> +		 * requirements met at 25MHz for Default Speed mode,
> +		 * those controllers work at 19MHz instead.
> +		 */
> +		if (clock == 25000000)
> +			clock = (25000000 * 19) / 25;
> +	}
> +
> +	sdhci_set_clock(&host->sdhci, clock, host->sdhci.max_clk);
> +}
> +
>  static void arasan_sdhci_set_ios(struct mci_host *mci, struct mci_ios *ios)
>  {
>  	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
>  	u16 val;
>  
>  	if (ios->clock)
> -		sdhci_set_clock(&host->sdhci, ios->clock, host->sdhci.max_clk);
> +		arasan_sdhci_set_clock(mci, ios->clock);
>  
>  	sdhci_set_bus_width(&host->sdhci, ios->bus_width);
>  
> @@ -243,6 +266,9 @@ static int arasan_sdhci_probe(struct device *dev)
>  	if (of_property_read_bool(np, "no-1-8-v"))
>  		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_NO_1_8_V;
>  
> +	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
> +		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN;
> +
>  	arasan_sdhci->sdhci.base = IOMEM(iores->start);
>  	arasan_sdhci->sdhci.mci = mci;
>  	mci->send_cmd = arasan_sdhci_send_cmd;
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 04/12] mci: arasan: read clk phases from DT

[Steffen Trumtrar]

Depending on the used SDHCI mode the clock phases are different.
Import the helper function to get these values from the DT from linux v6.7.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/arasan-sdhci.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 121 insertions(+)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index cc1bad8b62..76022708cd 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -30,10 +30,33 @@
 #define SDHCI_ARASAN_BUS_WIDTH			4
 #define TIMEOUT_VAL				0xE
 
+#define ZYNQMP_CLK_PHASES			10
+#define ZYNQMP_CLK_PHASE_UHS_SDR104		6
+#define ZYNQMP_CLK_PHASE_HS200			9
+/* Default settings for ZynqMP Clock Phases */
+#define ZYNQMP_ICLK_PHASE {0, 63, 63, 0, 63,  0,   0, 183, 54,  0, 0}
+#define ZYNQMP_OCLK_PHASE {0, 72, 60, 0, 60, 72, 135, 48, 72, 135, 0}
+
+/**
+ * struct sdhci_arasan_clk_data - Arasan Controller Clock Data.
+ *
+ * @clk_phase_in:	Array of Input Clock Phase Delays for all speed modes
+ * @clk_phase_out:	Array of Output Clock Phase Delays for all speed modes
+ * @set_clk_delays:	Function pointer for setting Clock Delays
+ * @clk_of_data:	Platform specific runtime clock data storage pointer
+ */
+struct sdhci_arasan_clk_data {
+	int		clk_phase_in[ZYNQMP_CLK_PHASES + 1];
+	int		clk_phase_out[ZYNQMP_CLK_PHASES + 1];
+	void		(*set_clk_delays)(struct sdhci *host);
+	void		*clk_of_data;
+};
+
 struct arasan_sdhci_host {
 	struct mci_host		mci;
 	struct sdhci		sdhci;
 	unsigned int		quirks; /* Arasan deviations from spec */
+	struct sdhci_arasan_clk_data clk_data;
 /* Controller does not have CD wired and will not function normally without */
 #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST		BIT(0)
 #define SDHCI_ARASAN_QUIRK_NO_1_8_V		BIT(1)
@@ -119,6 +142,7 @@ static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
 static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
 {
 	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
+	struct sdhci_arasan_clk_data *clk_data = &host->clk_data;
 
 	if (host->quirks & SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN) {
 		/*
@@ -219,6 +243,101 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
 	return ret;
 }
 
+static void sdhci_arasan_set_clk_delays(struct sdhci *host)
+{
+	struct arasan_sdhci_host *arasan_sdhci = sdhci_to_arasan(host);
+	struct mci_host *mci = &arasan_sdhci->mci;
+	struct sdhci_arasan_clk_data *clk_data = &arasan_sdhci->clk_data;
+
+}
+
+static void arasan_dt_read_clk_phase(struct device *dev,
+				     struct sdhci_arasan_clk_data *clk_data,
+				     unsigned int timing, const char *prop)
+{
+	struct device_node *np = dev->of_node;
+
+	u32 clk_phase[2] = {0};
+	int ret;
+
+	/*
+	 * Read Tap Delay values from DT, if the DT does not contain the
+	 * Tap Values then use the pre-defined values.
+	 */
+	ret = of_property_read_u32_array(np, prop, &clk_phase[0], 2);
+	if (ret < 0) {
+		dev_dbg(dev, "Using predefined clock phase for %s = %d %d\n",
+			prop, clk_data->clk_phase_in[timing],
+			clk_data->clk_phase_out[timing]);
+		return;
+	}
+
+	/* The values read are Input and Output Clock Delays in order */
+	clk_data->clk_phase_in[timing] = clk_phase[0];
+	clk_data->clk_phase_out[timing] = clk_phase[1];
+}
+
+/**
+ * arasan_dt_parse_clk_phases - Read Clock Delay values from DT
+ *
+ * @dev:		Pointer to our struct device.
+ * @clk_data:		Pointer to the Clock Data structure
+ *
+ * Called at initialization to parse the values of Clock Delays.
+ */
+static void arasan_dt_parse_clk_phases(struct device *dev,
+				       struct sdhci_arasan_clk_data *clk_data)
+{
+	u32 mio_bank = 0;
+	int i;
+
+	/*
+	 * This has been kept as a pointer and is assigned a function here.
+	 * So that different controller variants can assign their own handling
+	 * function.
+	 */
+	clk_data->set_clk_delays = sdhci_arasan_set_clk_delays;
+
+	if (of_device_is_compatible(dev->of_node, "xlnx,zynqmp-8.9a")) {
+		u32 zynqmp_iclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_ICLK_PHASE;
+		u32 zynqmp_oclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_OCLK_PHASE;
+
+		of_property_read_u32(dev->of_node, "xlnx,mio-bank", &mio_bank);
+		if (mio_bank == 2) {
+			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_UHS_SDR104] = 90;
+			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_HS200] = 90;
+		}
+
+		for (i = 0; i <= ZYNQMP_CLK_PHASES; i++) {
+			clk_data->clk_phase_in[i] = zynqmp_iclk_phase[i];
+			clk_data->clk_phase_out[i] = zynqmp_oclk_phase[i];
+		}
+	}
+
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_LEGACY,
+				 "clk-phase-legacy");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS,
+				 "clk-phase-mmc-hs");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_SD_HS,
+				 "clk-phase-sd-hs");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR12,
+				 "clk-phase-uhs-sdr12");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR25,
+				 "clk-phase-uhs-sdr25");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR50,
+				 "clk-phase-uhs-sdr50");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR104,
+				 "clk-phase-uhs-sdr104");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_DDR50,
+				 "clk-phase-uhs-ddr50");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_DDR52,
+				 "clk-phase-mmc-ddr52");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS200,
+				 "clk-phase-mmc-hs200");
+	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS400,
+				 "clk-phase-mmc-hs400");
+}
+
 static int arasan_sdhci_probe(struct device *dev)
 {
 	struct device_node *np = dev->of_node;
@@ -281,6 +400,8 @@ static int arasan_sdhci_probe(struct device *dev)
 	mci->f_max = clk_get_rate(clk_xin);
 	mci->f_min = 50000000 / 256;
 
+	arasan_dt_parse_clk_phases(dev, &arasan_sdhci->clk_data);
+
 	/* parse board supported bus width capabilities */
 	mci_of_parse(&arasan_sdhci->mci);
 

-- 
2.40.1

Re: [PATCH 04/12] mci: arasan: read clk phases from DT

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> Depending on the used SDHCI mode the clock phases are different.
> Import the helper function to get these values from the DT from linux v6.7.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/mci/arasan-sdhci.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
> index cc1bad8b62..76022708cd 100644
> --- a/drivers/mci/arasan-sdhci.c
> +++ b/drivers/mci/arasan-sdhci.c
> @@ -30,10 +30,33 @@
>  #define SDHCI_ARASAN_BUS_WIDTH			4
>  #define TIMEOUT_VAL				0xE
>  
> +#define ZYNQMP_CLK_PHASES			10
> +#define ZYNQMP_CLK_PHASE_UHS_SDR104		6
> +#define ZYNQMP_CLK_PHASE_HS200			9
> +/* Default settings for ZynqMP Clock Phases */
> +#define ZYNQMP_ICLK_PHASE {0, 63, 63, 0, 63,  0,   0, 183, 54,  0, 0}
> +#define ZYNQMP_OCLK_PHASE {0, 72, 60, 0, 60, 72, 135, 48, 72, 135, 0}
> +
> +/**
> + * struct sdhci_arasan_clk_data - Arasan Controller Clock Data.
> + *
> + * @clk_phase_in:	Array of Input Clock Phase Delays for all speed modes
> + * @clk_phase_out:	Array of Output Clock Phase Delays for all speed modes
> + * @set_clk_delays:	Function pointer for setting Clock Delays
> + * @clk_of_data:	Platform specific runtime clock data storage pointer
> + */
> +struct sdhci_arasan_clk_data {
> +	int		clk_phase_in[ZYNQMP_CLK_PHASES + 1];
> +	int		clk_phase_out[ZYNQMP_CLK_PHASES + 1];
> +	void		(*set_clk_delays)(struct sdhci *host);
> +	void		*clk_of_data;
> +};
> +
>  struct arasan_sdhci_host {
>  	struct mci_host		mci;
>  	struct sdhci		sdhci;
>  	unsigned int		quirks; /* Arasan deviations from spec */
> +	struct sdhci_arasan_clk_data clk_data;
>  /* Controller does not have CD wired and will not function normally without */
>  #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST		BIT(0)
>  #define SDHCI_ARASAN_QUIRK_NO_1_8_V		BIT(1)
> @@ -119,6 +142,7 @@ static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
>  static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
>  {
>  	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
> +	struct sdhci_arasan_clk_data *clk_data = &host->clk_data;
>  
>  	if (host->quirks & SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN) {
>  		/*
> @@ -219,6 +243,101 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
>  	return ret;
>  }
>  
> +static void sdhci_arasan_set_clk_delays(struct sdhci *host)
> +{
> +	struct arasan_sdhci_host *arasan_sdhci = sdhci_to_arasan(host);
> +	struct mci_host *mci = &arasan_sdhci->mci;
> +	struct sdhci_arasan_clk_data *clk_data = &arasan_sdhci->clk_data;
> +
> +}
> +
> +static void arasan_dt_read_clk_phase(struct device *dev,
> +				     struct sdhci_arasan_clk_data *clk_data,
> +				     unsigned int timing, const char *prop)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	u32 clk_phase[2] = {0};
> +	int ret;
> +
> +	/*
> +	 * Read Tap Delay values from DT, if the DT does not contain the
> +	 * Tap Values then use the pre-defined values.
> +	 */
> +	ret = of_property_read_u32_array(np, prop, &clk_phase[0], 2);
> +	if (ret < 0) {
> +		dev_dbg(dev, "Using predefined clock phase for %s = %d %d\n",
> +			prop, clk_data->clk_phase_in[timing],
> +			clk_data->clk_phase_out[timing]);
> +		return;
> +	}
> +
> +	/* The values read are Input and Output Clock Delays in order */
> +	clk_data->clk_phase_in[timing] = clk_phase[0];
> +	clk_data->clk_phase_out[timing] = clk_phase[1];
> +}
> +
> +/**
> + * arasan_dt_parse_clk_phases - Read Clock Delay values from DT
> + *
> + * @dev:		Pointer to our struct device.
> + * @clk_data:		Pointer to the Clock Data structure
> + *
> + * Called at initialization to parse the values of Clock Delays.
> + */
> +static void arasan_dt_parse_clk_phases(struct device *dev,
> +				       struct sdhci_arasan_clk_data *clk_data)
> +{
> +	u32 mio_bank = 0;
> +	int i;
> +
> +	/*
> +	 * This has been kept as a pointer and is assigned a function here.
> +	 * So that different controller variants can assign their own handling
> +	 * function.
> +	 */
> +	clk_data->set_clk_delays = sdhci_arasan_set_clk_delays;
> +
> +	if (of_device_is_compatible(dev->of_node, "xlnx,zynqmp-8.9a")) {
> +		u32 zynqmp_iclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_ICLK_PHASE;
> +		u32 zynqmp_oclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_OCLK_PHASE;
> +
> +		of_property_read_u32(dev->of_node, "xlnx,mio-bank", &mio_bank);
> +		if (mio_bank == 2) {
> +			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_UHS_SDR104] = 90;
> +			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_HS200] = 90;
> +		}
> +
> +		for (i = 0; i <= ZYNQMP_CLK_PHASES; i++) {
> +			clk_data->clk_phase_in[i] = zynqmp_iclk_phase[i];
> +			clk_data->clk_phase_out[i] = zynqmp_oclk_phase[i];
> +		}
> +	}
> +
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_LEGACY,
> +				 "clk-phase-legacy");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS,
> +				 "clk-phase-mmc-hs");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_SD_HS,
> +				 "clk-phase-sd-hs");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR12,
> +				 "clk-phase-uhs-sdr12");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR25,
> +				 "clk-phase-uhs-sdr25");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR50,
> +				 "clk-phase-uhs-sdr50");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR104,
> +				 "clk-phase-uhs-sdr104");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_DDR50,
> +				 "clk-phase-uhs-ddr50");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_DDR52,
> +				 "clk-phase-mmc-ddr52");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS200,
> +				 "clk-phase-mmc-hs200");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS400,
> +				 "clk-phase-mmc-hs400");
> +}
> +
>  static int arasan_sdhci_probe(struct device *dev)
>  {
>  	struct device_node *np = dev->of_node;
> @@ -281,6 +400,8 @@ static int arasan_sdhci_probe(struct device *dev)
>  	mci->f_max = clk_get_rate(clk_xin);
>  	mci->f_min = 50000000 / 256;
>  
> +	arasan_dt_parse_clk_phases(dev, &arasan_sdhci->clk_data);
> +
>  	/* parse board supported bus width capabilities */
>  	mci_of_parse(&arasan_sdhci->mci);
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 04/12] mci: arasan: read clk phases from DT

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> Depending on the used SDHCI mode the clock phases are different.
> Import the helper function to get these values from the DT from linux v6.7.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> ---
>  drivers/mci/arasan-sdhci.c | 121 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 121 insertions(+)
> 
> diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
> index cc1bad8b62..76022708cd 100644
> --- a/drivers/mci/arasan-sdhci.c
> +++ b/drivers/mci/arasan-sdhci.c
> @@ -30,10 +30,33 @@
>  #define SDHCI_ARASAN_BUS_WIDTH			4
>  #define TIMEOUT_VAL				0xE
>  
> +#define ZYNQMP_CLK_PHASES			10
> +#define ZYNQMP_CLK_PHASE_UHS_SDR104		6
> +#define ZYNQMP_CLK_PHASE_HS200			9
> +/* Default settings for ZynqMP Clock Phases */
> +#define ZYNQMP_ICLK_PHASE {0, 63, 63, 0, 63,  0,   0, 183, 54,  0, 0}
> +#define ZYNQMP_OCLK_PHASE {0, 72, 60, 0, 60, 72, 135, 48, 72, 135, 0}

"include: mci: sync mci_timing with linux" should be ordered before this
patch, because the above two lines depend on it for correctness.

> +
> +/**
> + * struct sdhci_arasan_clk_data - Arasan Controller Clock Data.
> + *
> + * @clk_phase_in:	Array of Input Clock Phase Delays for all speed modes
> + * @clk_phase_out:	Array of Output Clock Phase Delays for all speed modes
> + * @set_clk_delays:	Function pointer for setting Clock Delays
> + * @clk_of_data:	Platform specific runtime clock data storage pointer
> + */
> +struct sdhci_arasan_clk_data {
> +	int		clk_phase_in[ZYNQMP_CLK_PHASES + 1];
> +	int		clk_phase_out[ZYNQMP_CLK_PHASES + 1];
> +	void		(*set_clk_delays)(struct sdhci *host);
> +	void		*clk_of_data;
> +};
> +
>  struct arasan_sdhci_host {
>  	struct mci_host		mci;
>  	struct sdhci		sdhci;
>  	unsigned int		quirks; /* Arasan deviations from spec */
> +	struct sdhci_arasan_clk_data clk_data;
>  /* Controller does not have CD wired and will not function normally without */
>  #define SDHCI_ARASAN_QUIRK_FORCE_CDTEST		BIT(0)
>  #define SDHCI_ARASAN_QUIRK_NO_1_8_V		BIT(1)
> @@ -119,6 +142,7 @@ static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
>  static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
>  {
>  	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
> +	struct sdhci_arasan_clk_data *clk_data = &host->clk_data;
>  
>  	if (host->quirks & SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN) {
>  		/*
> @@ -219,6 +243,101 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
>  	return ret;
>  }
>  
> +static void sdhci_arasan_set_clk_delays(struct sdhci *host)
> +{
> +	struct arasan_sdhci_host *arasan_sdhci = sdhci_to_arasan(host);
> +	struct mci_host *mci = &arasan_sdhci->mci;
> +	struct sdhci_arasan_clk_data *clk_data = &arasan_sdhci->clk_data;
> +
> +}
> +
> +static void arasan_dt_read_clk_phase(struct device *dev,
> +				     struct sdhci_arasan_clk_data *clk_data,
> +				     unsigned int timing, const char *prop)
> +{
> +	struct device_node *np = dev->of_node;
> +
> +	u32 clk_phase[2] = {0};
> +	int ret;
> +
> +	/*
> +	 * Read Tap Delay values from DT, if the DT does not contain the
> +	 * Tap Values then use the pre-defined values.
> +	 */
> +	ret = of_property_read_u32_array(np, prop, &clk_phase[0], 2);
> +	if (ret < 0) {
> +		dev_dbg(dev, "Using predefined clock phase for %s = %d %d\n",
> +			prop, clk_data->clk_phase_in[timing],
> +			clk_data->clk_phase_out[timing]);
> +		return;
> +	}
> +
> +	/* The values read are Input and Output Clock Delays in order */
> +	clk_data->clk_phase_in[timing] = clk_phase[0];
> +	clk_data->clk_phase_out[timing] = clk_phase[1];
> +}
> +
> +/**
> + * arasan_dt_parse_clk_phases - Read Clock Delay values from DT
> + *
> + * @dev:		Pointer to our struct device.
> + * @clk_data:		Pointer to the Clock Data structure
> + *
> + * Called at initialization to parse the values of Clock Delays.
> + */
> +static void arasan_dt_parse_clk_phases(struct device *dev,
> +				       struct sdhci_arasan_clk_data *clk_data)
> +{
> +	u32 mio_bank = 0;
> +	int i;
> +
> +	/*
> +	 * This has been kept as a pointer and is assigned a function here.
> +	 * So that different controller variants can assign their own handling
> +	 * function.
> +	 */
> +	clk_data->set_clk_delays = sdhci_arasan_set_clk_delays;
> +
> +	if (of_device_is_compatible(dev->of_node, "xlnx,zynqmp-8.9a")) {
> +		u32 zynqmp_iclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_ICLK_PHASE;
> +		u32 zynqmp_oclk_phase[ZYNQMP_CLK_PHASES + 1] = ZYNQMP_OCLK_PHASE;
> +
> +		of_property_read_u32(dev->of_node, "xlnx,mio-bank", &mio_bank);
> +		if (mio_bank == 2) {
> +			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_UHS_SDR104] = 90;
> +			zynqmp_oclk_phase[ZYNQMP_CLK_PHASE_HS200] = 90;
> +		}
> +
> +		for (i = 0; i <= ZYNQMP_CLK_PHASES; i++) {
> +			clk_data->clk_phase_in[i] = zynqmp_iclk_phase[i];
> +			clk_data->clk_phase_out[i] = zynqmp_oclk_phase[i];
> +		}
> +	}
> +
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_LEGACY,
> +				 "clk-phase-legacy");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS,
> +				 "clk-phase-mmc-hs");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_SD_HS,
> +				 "clk-phase-sd-hs");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR12,
> +				 "clk-phase-uhs-sdr12");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR25,
> +				 "clk-phase-uhs-sdr25");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR50,
> +				 "clk-phase-uhs-sdr50");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_SDR104,
> +				 "clk-phase-uhs-sdr104");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_UHS_DDR50,
> +				 "clk-phase-uhs-ddr50");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_DDR52,
> +				 "clk-phase-mmc-ddr52");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS200,
> +				 "clk-phase-mmc-hs200");
> +	arasan_dt_read_clk_phase(dev, clk_data, MMC_TIMING_MMC_HS400,
> +				 "clk-phase-mmc-hs400");
> +}
> +
>  static int arasan_sdhci_probe(struct device *dev)
>  {
>  	struct device_node *np = dev->of_node;
> @@ -281,6 +400,8 @@ static int arasan_sdhci_probe(struct device *dev)
>  	mci->f_max = clk_get_rate(clk_xin);
>  	mci->f_min = 50000000 / 256;
>  
> +	arasan_dt_parse_clk_phases(dev, &arasan_sdhci->clk_data);
> +
>  	/* parse board supported bus width capabilities */
>  	mci_of_parse(&arasan_sdhci->mci);
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 05/12] mci: arasan: register sdcard/sampleclk

[Steffen Trumtrar]

Read and register the sampleclk and sdcardclk. They are needed later for
HS200 tuning support.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/arasan-sdhci.c | 305 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 305 insertions(+)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 76022708cd..77ea1df7e7 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -6,6 +6,7 @@
 #include <init.h>
 #include <linux/clk.h>
 #include <mci.h>
+#include <mach/zynqmp/firmware-zynqmp.h>
 
 #include "sdhci.h"
 
@@ -40,12 +41,20 @@
 /**
  * struct sdhci_arasan_clk_data - Arasan Controller Clock Data.
  *
+ * @sdcardclk_hw:	Struct for the clock we might provide to a PHY.
+ * @sdcardclk:		Pointer to normal 'struct clock' for sdcardclk_hw.
+ * @sampleclk_hw:	Struct for the clock we might provide to a PHY.
+ * @sampleclk:		Pointer to normal 'struct clock' for sampleclk_hw.
  * @clk_phase_in:	Array of Input Clock Phase Delays for all speed modes
  * @clk_phase_out:	Array of Output Clock Phase Delays for all speed modes
  * @set_clk_delays:	Function pointer for setting Clock Delays
  * @clk_of_data:	Platform specific runtime clock data storage pointer
  */
 struct sdhci_arasan_clk_data {
+	struct clk_hw	sdcardclk_hw;
+	struct clk      *sdcardclk;
+	struct clk_hw	sampleclk_hw;
+	struct clk      *sampleclk;
 	int		clk_phase_in[ZYNQMP_CLK_PHASES + 1];
 	int		clk_phase_out[ZYNQMP_CLK_PHASES + 1];
 	void		(*set_clk_delays)(struct sdhci *host);
@@ -154,6 +163,11 @@ static void arasan_sdhci_set_clock(struct mci_host *mci, unsigned int clock)
 			clock = (25000000 * 19) / 25;
 	}
 
+	clk_set_phase(clk_data->sampleclk,
+		      clk_data->clk_phase_in[mci->mci->host->timing]);
+	clk_set_phase(clk_data->sdcardclk,
+		      clk_data->clk_phase_out[mci->mci->host->timing]);
+
 	sdhci_set_clock(&host->sdhci, clock, host->sdhci.max_clk);
 }
 
@@ -249,6 +263,10 @@ static void sdhci_arasan_set_clk_delays(struct sdhci *host)
 	struct mci_host *mci = &arasan_sdhci->mci;
 	struct sdhci_arasan_clk_data *clk_data = &arasan_sdhci->clk_data;
 
+	clk_set_phase(clk_data->sampleclk,
+		      clk_data->clk_phase_in[mci->timing]);
+	clk_set_phase(clk_data->sdcardclk,
+		      clk_data->clk_phase_out[mci->timing]);
 }
 
 static void arasan_dt_read_clk_phase(struct device *dev,
@@ -277,6 +295,291 @@ static void arasan_dt_read_clk_phase(struct device *dev,
 	clk_data->clk_phase_out[timing] = clk_phase[1];
 }
 
+/**
+ * sdhci_zynqmp_sampleclk_set_phase - Set the SD Input Clock Tap Delays
+ *
+ * @hw:			Pointer to the hardware clock structure.
+ * @degrees:		The clock phase shift between 0 - 359.
+ *
+ * Set the SD Input Clock Tap Delays for Input path
+ *
+ * Return: 0 on success and error value on error
+ */
+static int arasan_zynqmp_sampleclk_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct sdhci_arasan_clk_data *clk_data =
+		container_of(hw, struct sdhci_arasan_clk_data, sampleclk_hw);
+	struct arasan_sdhci_host *sdhci_arasan =
+		container_of(clk_data, struct arasan_sdhci_host, clk_data);
+	struct mci_host *host = &sdhci_arasan->mci;
+	const char *clk_name = clk_hw_get_name(hw);
+	u32 node_id = !strcmp(clk_name, "clk_in_sd0") ? NODE_SD_0 : NODE_SD_1;
+	u8 tap_delay, tap_max = 0;
+	int ret;
+
+	/* This is applicable for SDHCI_SPEC_300 and above */
+	if (sdhci_arasan->sdhci.version < SDHCI_SPEC_300)
+		return 0;
+
+	/* Assert DLL Reset */
+	zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_ASSERT);
+
+	switch (host->timing) {
+	case MMC_TIMING_MMC_HS:
+	case MMC_TIMING_SD_HS:
+	case MMC_TIMING_UHS_DDR50:
+	case MMC_TIMING_MMC_DDR52:
+		/* For 50MHz clock, 120 Taps are available */
+		tap_max = 120;
+		break;
+	case MMC_TIMING_UHS_SDR50:
+		/* For 100MHz clock, 60 Taps are available */
+		tap_max = 60;
+		break;
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_MMC_HS200:
+		/* For 200MHz clock, 30 Taps are available */
+		tap_max = 30;
+		break;
+	default:
+		break;
+	}
+
+	tap_delay = (degrees * tap_max) / 360;
+
+	/* Set the Clock Phase */
+	ret = zynqmp_pm_set_sd_tapdelay(node_id, PM_TAPDELAY_INPUT, tap_delay);
+	if (ret)
+		pr_err("Error setting Input Tap Delay\n");
+
+	return ret;
+}
+
+static unsigned long arasan_zynqmp_sampleclk_recalc_rate(struct clk_hw *hw,
+							 unsigned long parent_rate)
+{
+	return 0;
+};
+
+/**
+ * sdhci_zynqmp_sdcardclk_set_phase - Set the SD Output Clock Tap Delays
+ *
+ * @hw:			Pointer to the hardware clock structure.
+ * @degrees:		The clock phase shift between 0 - 359.
+ *
+ * Set the SD Output Clock Tap Delays for Output path
+ *
+ * Return: 0 on success and error value on error
+ */
+static int arasan_zynqmp_sdcardclk_set_phase(struct clk_hw *hw, int degrees)
+{
+	struct sdhci_arasan_clk_data *clk_data =
+		container_of(hw, struct sdhci_arasan_clk_data, sdcardclk_hw);
+	struct arasan_sdhci_host *sdhci_arasan =
+		container_of(clk_data, struct arasan_sdhci_host, clk_data);
+	struct mci_host *host = &sdhci_arasan->mci;
+	const char *clk_name = clk_hw_get_name(hw);
+	u32 node_id = !strcmp(clk_name, "clk_out_sd0") ? NODE_SD_0 : NODE_SD_1;
+	u8 tap_delay, tap_max = 0;
+	int ret;
+
+	/* This is applicable for SDHCI_SPEC_300 and above */
+	if (sdhci_arasan->sdhci.version < SDHCI_SPEC_300)
+		return 0;
+
+	switch (host->timing) {
+	case MMC_TIMING_MMC_HS:
+	case MMC_TIMING_SD_HS:
+	case MMC_TIMING_UHS_DDR50:
+	case MMC_TIMING_MMC_DDR52:
+		/* For 50MHz clock, 30 Taps are available */
+		tap_max = 30;
+		break;
+	case MMC_TIMING_UHS_SDR50:
+		/* For 100MHz clock, 15 Taps are available */
+		tap_max = 15;
+		break;
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_MMC_HS200:
+		/* For 200MHz clock, 8 Taps are available */
+		tap_max = 8;
+		break;
+	default:
+		break;
+	}
+
+	tap_delay = (degrees * tap_max) / 360;
+
+	/* Set the Clock Phase */
+	ret = zynqmp_pm_set_sd_tapdelay(node_id, PM_TAPDELAY_OUTPUT, tap_delay);
+	if (ret)
+		pr_err("Error setting Output Tap Delay\n");
+
+	/* Release DLL Reset */
+	zynqmp_pm_sd_dll_reset(node_id, PM_DLL_RESET_RELEASE);
+
+	return ret;
+}
+
+static unsigned long arasan_zynqmp_sdcardclk_recalc_rate(struct clk_hw *hw,
+							 unsigned long parent_rate)
+{
+	return 0;
+};
+
+static const struct clk_ops clk_sampleclk_ops = {
+	.recalc_rate = arasan_zynqmp_sampleclk_recalc_rate,
+	.set_phase = arasan_zynqmp_sampleclk_set_phase,
+};
+
+static const struct clk_ops clk_sdcardclk_ops = {
+	.recalc_rate = arasan_zynqmp_sdcardclk_recalc_rate,
+	.set_phase = arasan_zynqmp_sdcardclk_set_phase,
+};
+
+/**
+ * arasan_sdhci_register_sampleclk - Register the sampleclk for a PHY to use
+ *
+ * @sdhci_arasan:	Our private data structure.
+ * @clk_xin:		Pointer to the functional clock
+ * @dev:		Pointer to our struct device.
+ *
+ * Some PHY devices need to know what the actual card clock is.  In order for
+ * them to find out, we'll provide a clock through the common clock framework
+ * for them to query.
+ *
+ * Return: 0 on success and error value on error
+ */
+static int
+arasan_sdhci_register_sampleclk(struct sdhci_arasan_clk_data *clk_data,
+				struct clk *clk_xin,
+				struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct clk_init_data sampleclk_init = {};
+	const char *clk_name;
+	int ret;
+
+	ret = of_property_read_string_index(np, "clock-output-names", 1,
+					    &sampleclk_init.name);
+	if (ret) {
+		dev_err(dev, "DT has #clock-cells but no clock-output-names\n");
+		return ret;
+	}
+
+	clk_name = __clk_get_name(clk_xin);
+	sampleclk_init.parent_names = &clk_name;
+	sampleclk_init.num_parents = 1;
+	sampleclk_init.ops = &clk_sampleclk_ops;
+
+	clk_data->sampleclk_hw.init = &sampleclk_init;
+	clk_data->sampleclk = clk_register(dev, &clk_data->sampleclk_hw);
+	if (IS_ERR(clk_data->sampleclk))
+		return PTR_ERR(clk_data->sampleclk);
+	clk_data->sampleclk_hw.init = NULL;
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get,
+				  clk_data->sampleclk);
+	if (ret)
+		dev_err(dev, "Failed to add sample clock provider\n");
+
+	return ret;
+}
+
+/**
+ * sdhci_arasan_register_sdcardclk - Register the sdcardclk for a PHY to use
+ *
+ * @sdhci_arasan:	Our private data structure.
+ * @clk_xin:		Pointer to the functional clock
+ * @dev:		Pointer to our struct device.
+ *
+ * Some PHY devices need to know what the actual card clock is.  In order for
+ * them to find out, we'll provide a clock through the common clock framework
+ * for them to query.
+ *
+ * Return: 0 on success and error value on error
+ */
+static int
+arasan_sdhci_register_sdcardclk(struct sdhci_arasan_clk_data *clk_data,
+				struct clk *clk_xin,
+				struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	struct clk_init_data sdcardclk_init = {};
+	const char *clk_name;
+	int ret;
+
+	ret = of_property_read_string_index(np, "clock-output-names", 0,
+					    &sdcardclk_init.name);
+	if (ret) {
+		dev_err(dev, "DT has #clock-cells but no clock-output-names\n");
+		return ret;
+	}
+
+	clk_name = __clk_get_name(clk_xin);
+	sdcardclk_init.parent_names = &clk_name;
+	sdcardclk_init.ops = &clk_sdcardclk_ops;
+	sdcardclk_init.num_parents = 1;
+
+	clk_data->sdcardclk_hw.init = &sdcardclk_init;
+	clk_data->sdcardclk = clk_register(dev, &clk_data->sdcardclk_hw);
+	if (IS_ERR(clk_data->sdcardclk))
+		return PTR_ERR(clk_data->sdcardclk);
+	clk_data->sdcardclk_hw.init = NULL;
+
+	ret = of_clk_add_provider(np, of_clk_src_simple_get,
+				  clk_data->sdcardclk);
+	if (ret)
+		dev_err(dev, "Failed to add sdcard clock provider\n");
+
+	return ret;
+}
+
+/**
+ * arasan_sdhci_register_sdclk - Register the sdcardclk for a PHY to use
+ *
+ * @sdhci_arasan:	Our private data structure.
+ * @clk_xin:		Pointer to the functional clock
+ * @dev:		Pointer to our struct device.
+ *
+ * Some PHY devices need to know what the actual card clock is.  In order for
+ * them to find out, we'll provide a clock through the common clock framework
+ * for them to query.
+ *
+ * Note: without seriously re-architecting SDHCI's clock code and testing on
+ * all platforms, there's no way to create a totally beautiful clock here
+ * with all clock ops implemented.  Instead, we'll just create a clock that can
+ * be queried and set the CLK_GET_RATE_NOCACHE attribute to tell common clock
+ * framework that we're doing things behind its back.  This should be sufficient
+ * to create nice clean device tree bindings and later (if needed) we can try
+ * re-architecting SDHCI if we see some benefit to it.
+ *
+ * Return: 0 on success and error value on error
+ */
+static int arasan_sdhci_register_sdclk(struct sdhci_arasan_clk_data *sdhci_arasan,
+				       struct clk *clk_xin,
+				       struct device *dev)
+{
+	struct device_node *np = dev->of_node;
+	u32 num_clks = 0;
+	int ret;
+
+	/* Providing a clock to the PHY is optional; no error if missing */
+	if (of_property_read_u32(np, "#clock-cells", &num_clks) < 0)
+		return 0;
+
+	ret = arasan_sdhci_register_sdcardclk(sdhci_arasan, clk_xin, dev);
+	if (ret)
+		return ret;
+
+	if (num_clks) {
+		ret = arasan_sdhci_register_sampleclk(sdhci_arasan, clk_xin,
+						      dev);
+	}
+
+	return 0;
+}
+
 /**
  * arasan_dt_parse_clk_phases - Read Clock Delay values from DT
  *
@@ -400,6 +703,8 @@ static int arasan_sdhci_probe(struct device *dev)
 	mci->f_max = clk_get_rate(clk_xin);
 	mci->f_min = 50000000 / 256;
 
+	arasan_sdhci_register_sdclk(&arasan_sdhci->clk_data, clk_xin, dev);
+
 	arasan_dt_parse_clk_phases(dev, &arasan_sdhci->clk_data);
 
 	/* parse board supported bus width capabilities */

-- 
2.40.1

Re: [PATCH 05/12] mci: arasan: register sdcard/sampleclk

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> +static unsigned long arasan_zynqmp_sampleclk_recalc_rate(struct clk_hw *hw,
> +							 unsigned long parent_rate)
> +{
> +	return 0;

In HS200 mode, Linux reports this as 200MHz. Returning zero here is confusing,
even if no one depends on it. What we should be doing, I think, is set

  host->actual_clock = host->clock

at the end of mci_set_ios() and return host->sdhci.mci->actual_clock here.

> +static unsigned long arasan_zynqmp_sdcardclk_recalc_rate(struct clk_hw *hw,
> +							 unsigned long parent_rate)
> +{
> +	return 0;
> +};

Same here

> +static int arasan_sdhci_register_sdclk(struct sdhci_arasan_clk_data *sdhci_arasan,
> +				       struct clk *clk_xin,
> +				       struct device *dev)
> +{
> +	struct device_node *np = dev->of_node;
> +	u32 num_clks = 0;
> +	int ret;
> +
> +	/* Providing a clock to the PHY is optional; no error if missing */
> +	if (of_property_read_u32(np, "#clock-cells", &num_clks) < 0)
> +		return 0;
> +
> +	ret = arasan_sdhci_register_sdcardclk(sdhci_arasan, clk_xin, dev);
> +	if (ret)
> +		return ret;
> +
> +	if (num_clks) {
> +		ret = arasan_sdhci_register_sampleclk(sdhci_arasan, clk_xin,
> +						      dev);
		s/ret = /return/

While at it, you can drop the braces.


> +	}
> +
> +	return 0;
> +}
> +
>  /**
>   * arasan_dt_parse_clk_phases - Read Clock Delay values from DT
>   *
> @@ -400,6 +703,8 @@ static int arasan_sdhci_probe(struct device *dev)
>  	mci->f_max = clk_get_rate(clk_xin);
>  	mci->f_min = 50000000 / 256;
>  
> +	arasan_sdhci_register_sdclk(&arasan_sdhci->clk_data, clk_xin, dev);
> +
>  	arasan_dt_parse_clk_phases(dev, &arasan_sdhci->clk_data);
>  
>  	/* parse board supported bus width capabilities */
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 06/12] include: mci: sync mci_timing with linux

[Steffen Trumtrar]

The timings are used to check if mci_host->timing is equal to one of
this values. Linux uses a different numbering. Adapt that for compatability.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 include/mci.h | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/include/mci.h b/include/mci.h
index 9c9d44881d..91460b7ef6 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -411,14 +411,16 @@ enum mci_timing {
 	MMC_TIMING_LEGACY	= 0,
 	MMC_TIMING_MMC_HS	= 1,
 	MMC_TIMING_SD_HS	= 2,
-	MMC_TIMING_UHS_SDR12	= MMC_TIMING_LEGACY,
-	MMC_TIMING_UHS_SDR25	= MMC_TIMING_SD_HS,
-	MMC_TIMING_UHS_SDR50	= 3,
-	MMC_TIMING_UHS_SDR104	= 4,
-	MMC_TIMING_UHS_DDR50	= 5,
-	MMC_TIMING_MMC_HS200	= 6,
-	MMC_TIMING_MMC_DDR52	= 7,
-	MMC_TIMING_MMC_HS400	= 8,
+	MMC_TIMING_UHS_SDR12	= 3,
+	MMC_TIMING_UHS_SDR25	= 4,
+	MMC_TIMING_UHS_SDR50	= 5,
+	MMC_TIMING_UHS_SDR104	= 6,
+	MMC_TIMING_UHS_DDR50	= 7,
+	MMC_TIMING_MMC_DDR52	= 8,
+	MMC_TIMING_MMC_HS200	= 9,
+	MMC_TIMING_MMC_HS400	= 10,
+	MMC_TIMING_SD_EXP	= 11,
+	MMC_TIMING_SD_EXP_1_2V	= 12,
 };
 
 static inline bool mci_timing_is_ddr(enum mci_timing timing)

-- 
2.40.1

Re: [PATCH 06/12] include: mci: sync mci_timing with linux

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> The timings are used to check if mci_host->timing is equal to one of
> this values. Linux uses a different numbering. Adapt that for compatability.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  include/mci.h | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/include/mci.h b/include/mci.h
> index 9c9d44881d..91460b7ef6 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -411,14 +411,16 @@ enum mci_timing {
>  	MMC_TIMING_LEGACY	= 0,
>  	MMC_TIMING_MMC_HS	= 1,
>  	MMC_TIMING_SD_HS	= 2,
> -	MMC_TIMING_UHS_SDR12	= MMC_TIMING_LEGACY,
> -	MMC_TIMING_UHS_SDR25	= MMC_TIMING_SD_HS,
> -	MMC_TIMING_UHS_SDR50	= 3,
> -	MMC_TIMING_UHS_SDR104	= 4,
> -	MMC_TIMING_UHS_DDR50	= 5,
> -	MMC_TIMING_MMC_HS200	= 6,
> -	MMC_TIMING_MMC_DDR52	= 7,
> -	MMC_TIMING_MMC_HS400	= 8,
> +	MMC_TIMING_UHS_SDR12	= 3,
> +	MMC_TIMING_UHS_SDR25	= 4,
> +	MMC_TIMING_UHS_SDR50	= 5,
> +	MMC_TIMING_UHS_SDR104	= 6,
> +	MMC_TIMING_UHS_DDR50	= 7,
> +	MMC_TIMING_MMC_DDR52	= 8,
> +	MMC_TIMING_MMC_HS200	= 9,
> +	MMC_TIMING_MMC_HS400	= 10,
> +	MMC_TIMING_SD_EXP	= 11,
> +	MMC_TIMING_SD_EXP_1_2V	= 12,
>  };
>  
>  static inline bool mci_timing_is_ddr(enum mci_timing timing)
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 07/12] include: mci: add more EXT_CSD_CARD_TYPE_*

[Steffen Trumtrar]

Import missing EXT_CSD_CARD_TYPE_ defines from linux v6.7.

EXT_CSD_CARD_TYPE_SDR_1_8V/1_2V is unused in barebox. Replace with the
defines from linux.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 include/mci.h | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/include/mci.h b/include/mci.h
index 91460b7ef6..9a52e3f360 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -293,13 +293,24 @@
 #define EXT_CSD_CARD_TYPE_MASK		0x3f
 #define EXT_CSD_CARD_TYPE_26		(1<<0)	/* Card can run at 26MHz */
 #define EXT_CSD_CARD_TYPE_52		(1<<1)	/* Card can run at 52MHz */
+#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_HS_26 |	\
+					 EXT_CSD_CARD_TYPE_HS_52)
 #define EXT_CSD_CARD_TYPE_DDR_1_8V	(1<<2)	/* Card can run at 52MHz */
 						/* DDR mode @1.8V or 3V I/O */
 #define EXT_CSD_CARD_TYPE_DDR_1_2V	(1<<3)	/* Card can run at 52MHz */
 						/* DDR mode @1.2V I/O */
-#define EXT_CSD_CARD_TYPE_SDR_1_8V	(1<<4)	/* Card can run at 200MHz */
-#define EXT_CSD_CARD_TYPE_SDR_1_2V	(1<<5)	/* Card can run at 200MHz */
+#define EXT_CSD_CARD_TYPE_DDR_52	(EXT_CSD_CARD_TYPE_DDR_1_8V  \
+					 | EXT_CSD_CARD_TYPE_DDR_1_2V)
+#define EXT_CSD_CARD_TYPE_HS200_1_8V	(1<<4)	/* Card can run at 200MHz */
+#define EXT_CSD_CARD_TYPE_HS200_1_2V	(1<<5)	/* Card can run at 200MHz */
 						/* SDR mode @1.2V I/O */
+#define EXT_CSD_CARD_TYPE_HS200		(EXT_CSD_CARD_TYPE_HS200_1_8V | \
+					 EXT_CSD_CARD_TYPE_HS200_1_2V)
+#define EXT_CSD_CARD_TYPE_HS400_1_8V	(1<<6)	/* Card can run at 200MHz DDR, 1.8V */
+#define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
+#define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
+					 EXT_CSD_CARD_TYPE_HS400_1_2V)
+#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
 
 /* register PARTITIONS_ATTRIBUTE [156] */
 #define EXT_CSD_ENH_USR_MASK		(1 << 0)

-- 
2.40.1

Re: [PATCH 07/12] include: mci: add more EXT_CSD_CARD_TYPE_*

[Ahmad Fatoum]

On 08.03.24 12:17, Steffen Trumtrar wrote:
> Import missing EXT_CSD_CARD_TYPE_ defines from linux v6.7.
> 
> EXT_CSD_CARD_TYPE_SDR_1_8V/1_2V is unused in barebox. Replace with the
> defines from linux.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  include/mci.h | 15 +++++++++++++--
>  1 file changed, 13 insertions(+), 2 deletions(-)
> 
> diff --git a/include/mci.h b/include/mci.h
> index 91460b7ef6..9a52e3f360 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -293,13 +293,24 @@
>  #define EXT_CSD_CARD_TYPE_MASK		0x3f
>  #define EXT_CSD_CARD_TYPE_26		(1<<0)	/* Card can run at 26MHz */
>  #define EXT_CSD_CARD_TYPE_52		(1<<1)	/* Card can run at 52MHz */
> +#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_HS_26 |	\
> +					 EXT_CSD_CARD_TYPE_HS_52)
>  #define EXT_CSD_CARD_TYPE_DDR_1_8V	(1<<2)	/* Card can run at 52MHz */
>  						/* DDR mode @1.8V or 3V I/O */
>  #define EXT_CSD_CARD_TYPE_DDR_1_2V	(1<<3)	/* Card can run at 52MHz */
>  						/* DDR mode @1.2V I/O */
> -#define EXT_CSD_CARD_TYPE_SDR_1_8V	(1<<4)	/* Card can run at 200MHz */
> -#define EXT_CSD_CARD_TYPE_SDR_1_2V	(1<<5)	/* Card can run at 200MHz */
> +#define EXT_CSD_CARD_TYPE_DDR_52	(EXT_CSD_CARD_TYPE_DDR_1_8V  \
> +					 | EXT_CSD_CARD_TYPE_DDR_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS200_1_8V	(1<<4)	/* Card can run at 200MHz */
> +#define EXT_CSD_CARD_TYPE_HS200_1_2V	(1<<5)	/* Card can run at 200MHz */
>  						/* SDR mode @1.2V I/O */
> +#define EXT_CSD_CARD_TYPE_HS200		(EXT_CSD_CARD_TYPE_HS200_1_8V | \
> +					 EXT_CSD_CARD_TYPE_HS200_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400_1_8V	(1<<6)	/* Card can run at 200MHz DDR, 1.8V */
> +#define EXT_CSD_CARD_TYPE_HS400_1_2V	(1<<7)	/* Card can run at 200MHz DDR, 1.2V */
> +#define EXT_CSD_CARD_TYPE_HS400		(EXT_CSD_CARD_TYPE_HS400_1_8V | \
> +					 EXT_CSD_CARD_TYPE_HS400_1_2V)
> +#define EXT_CSD_CARD_TYPE_HS400ES	(1<<8)	/* Card can run at HS400ES */
>  
>  /* register PARTITIONS_ATTRIBUTE [156] */
>  #define EXT_CSD_ENH_USR_MASK		(1 << 0)
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 08/12] mci: core: parse more host capabilities from DT

[Steffen Trumtrar]

Port the linux v6.7 mmc host caps2 parsing.
While at it, remove the ->no_sd and ->no_sdio. These are bits in the
caps2 field.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/mci-core.c | 34 ++++++++++++++++++++++++++++------
 include/mci.h          | 32 ++++++++++++++++++++++++++++++--
 2 files changed, 58 insertions(+), 8 deletions(-)

diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index 2b39985d5e..cd046149b4 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -2029,7 +2029,7 @@ static int mci_card_probe(struct mci *mci)
 		goto on_error;
 	}
 
-	if (!host->no_sdio) {
+	if (!(host->caps2 & MMC_CAP2_NO_SDIO)) {
 		rc = sdio_send_op_cond(mci);
 		if (!rc) {
 			mci->ready_for_use = true;
@@ -2040,11 +2040,11 @@ static int mci_card_probe(struct mci *mci)
 	}
 
 	/* Check if this card can handle the "SD Card Physical Layer Specification 2.0" */
-	if (!host->no_sd) {
+	if (!(host->caps2 & MMC_CAP2_NO_SD)) {
 		rc = sd_send_if_cond(mci);
 		rc = sd_send_op_cond(mci);
 	}
-	if (host->no_sd || rc == -ETIMEDOUT) {
+	if ((host->caps2 & MMC_CAP2_NO_SD) || rc == -ETIMEDOUT) {
 		/* If SD card initialization was skipped or if it timed out,
 		 * we check for an MMC card */
 		dev_dbg(&mci->dev, "Card seems to be a MultiMediaCard\n");
@@ -2251,7 +2251,7 @@ int mci_register(struct mci_host *host)
 	if (IS_ENABLED(CONFIG_MCI_STARTUP))
 		mci_card_probe(mci);
 
-	if (!host->no_sd && dev_of_node(host->hw_dev))
+	if (!(host->caps2 & MMC_CAP2_NO_SD) && dev_of_node(host->hw_dev))
 		of_register_fixup(of_broken_cd_fixup, host);
 
 	list_add_tail(&mci->list, &mci_list);
@@ -2322,9 +2322,31 @@ void mci_of_parse_node(struct mci_host *host,
 
 	host->broken_cd = of_property_read_bool(np, "broken-cd");
 	host->non_removable = of_property_read_bool(np, "non-removable");
-	host->no_sd = of_property_read_bool(np, "no-sd");
-	host->no_sdio = of_property_read_bool(np, "no-sdio");
 	host->disable_wp = of_property_read_bool(np, "disable-wp");
+
+	if (of_property_read_bool(np, "full-pwr-cycle"))
+		host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE;
+	if (of_property_read_bool(np, "full-pwr-cycle-in-suspend"))
+		host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND;
+	if (of_property_read_bool(np, "mmc-hs200-1_8v"))
+		host->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
+	if (of_property_read_bool(np, "mmc-hs200-1_2v"))
+		host->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
+	if (of_property_read_bool(np, "mmc-hs400-1_8v"))
+		host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;
+	if (of_property_read_bool(np, "mmc-hs400-1_2v"))
+		host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR;
+	if (of_property_read_bool(np, "mmc-hs400-enhanced-strobe"))
+		host->caps2 |= MMC_CAP2_HS400_ES;
+	if (of_property_read_bool(np, "no-sdio"))
+		host->caps2 |= MMC_CAP2_NO_SDIO;
+	if (of_property_read_bool(np, "no-sd"))
+		host->caps2 |= MMC_CAP2_NO_SD;
+	if (of_property_read_bool(np, "no-mmc"))
+		host->caps2 |= MMC_CAP2_NO_MMC;
+	if (of_property_read_bool(np, "no-mmc-hs400"))
+		host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
+				 MMC_CAP2_HS400_ES);
 }
 
 void mci_of_parse(struct mci_host *host)
diff --git a/include/mci.h b/include/mci.h
index 9a52e3f360..ed2967c889 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -468,6 +468,36 @@ struct mci_host {
 	const char *devname;		/**< the devicename for the card, defaults to disk%d */
 	unsigned voltages;
 	unsigned host_caps;	/**< Host's interface capabilities, refer MMC_VDD_* */
+	unsigned caps2;		/* More host capabilities */
+#define MMC_CAP2_BOOTPART_NOACC	(1 << 0)	/* Boot partition no access */
+#define MMC_CAP2_FULL_PWR_CYCLE	(1 << 2)	/* Can do full power cycle */
+#define MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND (1 << 3) /* Can do full power cycle in suspend */
+#define MMC_CAP2_HS200_1_8V_SDR	(1 << 5)        /* can support */
+#define MMC_CAP2_HS200_1_2V_SDR	(1 << 6)        /* can support */
+#define MMC_CAP2_HS200		(MMC_CAP2_HS200_1_8V_SDR | \
+				 MMC_CAP2_HS200_1_2V_SDR)
+#define MMC_CAP2_SD_EXP		(1 << 7)	/* SD express via PCIe */
+#define MMC_CAP2_SD_EXP_1_2V	(1 << 8)	/* SD express 1.2V */
+#define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
+#define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
+#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
+#define MMC_CAP2_HS400_1_8V	(1 << 15)	/* Can support HS400 1.8V */
+#define MMC_CAP2_HS400_1_2V	(1 << 16)	/* Can support HS400 1.2V */
+#define MMC_CAP2_HS400		(MMC_CAP2_HS400_1_8V | \
+				 MMC_CAP2_HS400_1_2V)
+#define MMC_CAP2_HSX00_1_8V	(MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V)
+#define MMC_CAP2_HSX00_1_2V	(MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
+#define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
+#define MMC_CAP2_NO_WRITE_PROTECT (1 << 18)	/* No physical write protect pin, assume that card is always read-write */
+#define MMC_CAP2_NO_SDIO	(1 << 19)	/* Do not send SDIO commands during initialization */
+#define MMC_CAP2_HS400_ES	(1 << 20)	/* Host supports enhanced strobe */
+#define MMC_CAP2_NO_SD		(1 << 21)	/* Do not send SD commands during initialization */
+#define MMC_CAP2_NO_MMC		(1 << 22)	/* Do not send (e)MMC commands during initialization */
+#define MMC_CAP2_CQE		(1 << 23)	/* Has eMMC command queue engine */
+#define MMC_CAP2_CQE_DCMD	(1 << 24)	/* CQE can issue a direct command */
+#define MMC_CAP2_AVOID_3_3V	(1 << 25)	/* Host must negotiate down from 3.3V */
+#define MMC_CAP2_MERGE_CAPABLE	(1 << 26)	/* Host can merge a segment over the segment size */
+#define MMC_CAP2_CRYPTO		0
 	unsigned f_min;		/**< host interface lower limit */
 	unsigned f_max;		/**< host interface upper limit */
 	unsigned clock;		/**< Current clock used to talk to the card */
@@ -478,8 +508,6 @@ struct mci_host {
 	int use_dsr;		/**< optional dsr usage flag */
 	int broken_cd;		/**< card detect is broken */
 	bool non_removable;	/**< device is non removable */
-	bool no_sd;		/**< do not send SD commands during initialization */
-	bool no_sdio;		/**< do not send SDIO commands during initialization */
 	bool disable_wp;	/**< ignore write-protect detection logic */
 	struct regulator *supply;
 

-- 
2.40.1

Re: [PATCH 08/12] mci: core: parse more host capabilities from DT

[Ahmad Fatoum]

Hi Steffen,

On 08.03.24 12:17, Steffen Trumtrar wrote:
> @@ -2322,9 +2322,31 @@ void mci_of_parse_node(struct mci_host *host,
>  
>  	host->broken_cd = of_property_read_bool(np, "broken-cd");
>  	host->non_removable = of_property_read_bool(np, "non-removable");
> -	host->no_sd = of_property_read_bool(np, "no-sd");
> -	host->no_sdio = of_property_read_bool(np, "no-sdio");
>  	host->disable_wp = of_property_read_bool(np, "disable-wp");
> +
> +	if (of_property_read_bool(np, "full-pwr-cycle"))
> +		host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE;
> +	if (of_property_read_bool(np, "full-pwr-cycle-in-suspend"))
> +		host->caps2 |= MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND;
> +	if (of_property_read_bool(np, "mmc-hs200-1_8v"))
> +		host->caps2 |= MMC_CAP2_HS200_1_8V_SDR;
> +	if (of_property_read_bool(np, "mmc-hs200-1_2v"))
> +		host->caps2 |= MMC_CAP2_HS200_1_2V_SDR;
> +	if (of_property_read_bool(np, "mmc-hs400-1_8v"))
> +		host->caps2 |= MMC_CAP2_HS400_1_8V | MMC_CAP2_HS200_1_8V_SDR;
> +	if (of_property_read_bool(np, "mmc-hs400-1_2v"))
> +		host->caps2 |= MMC_CAP2_HS400_1_2V | MMC_CAP2_HS200_1_2V_SDR;
> +	if (of_property_read_bool(np, "mmc-hs400-enhanced-strobe"))
> +		host->caps2 |= MMC_CAP2_HS400_ES;
> +	if (of_property_read_bool(np, "no-sdio"))
> +		host->caps2 |= MMC_CAP2_NO_SDIO;
> +	if (of_property_read_bool(np, "no-sd"))
> +		host->caps2 |= MMC_CAP2_NO_SD;
> +	if (of_property_read_bool(np, "no-mmc"))
> +		host->caps2 |= MMC_CAP2_NO_MMC;
> +	if (of_property_read_bool(np, "no-mmc-hs400"))
> +		host->caps2 &= ~(MMC_CAP2_HS400_1_8V | MMC_CAP2_HS400_1_2V |
> +				 MMC_CAP2_HS400_ES);

Can you move all the stuff with hs200 and hs400 into it into
an IS_ENABLED(CONFIG_MCI_TUNING)?

With this done: Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

>  }
>  
>  void mci_of_parse(struct mci_host *host)
> diff --git a/include/mci.h b/include/mci.h
> index 9a52e3f360..ed2967c889 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -468,6 +468,36 @@ struct mci_host {
>  	const char *devname;		/**< the devicename for the card, defaults to disk%d */
>  	unsigned voltages;
>  	unsigned host_caps;	/**< Host's interface capabilities, refer MMC_VDD_* */
> +	unsigned caps2;		/* More host capabilities */
> +#define MMC_CAP2_BOOTPART_NOACC	(1 << 0)	/* Boot partition no access */
> +#define MMC_CAP2_FULL_PWR_CYCLE	(1 << 2)	/* Can do full power cycle */
> +#define MMC_CAP2_FULL_PWR_CYCLE_IN_SUSPEND (1 << 3) /* Can do full power cycle in suspend */
> +#define MMC_CAP2_HS200_1_8V_SDR	(1 << 5)        /* can support */
> +#define MMC_CAP2_HS200_1_2V_SDR	(1 << 6)        /* can support */
> +#define MMC_CAP2_HS200		(MMC_CAP2_HS200_1_8V_SDR | \
> +				 MMC_CAP2_HS200_1_2V_SDR)
> +#define MMC_CAP2_SD_EXP		(1 << 7)	/* SD express via PCIe */
> +#define MMC_CAP2_SD_EXP_1_2V	(1 << 8)	/* SD express 1.2V */
> +#define MMC_CAP2_CD_ACTIVE_HIGH	(1 << 10)	/* Card-detect signal active high */
> +#define MMC_CAP2_RO_ACTIVE_HIGH	(1 << 11)	/* Write-protect signal active high */
> +#define MMC_CAP2_NO_PRESCAN_POWERUP (1 << 14)	/* Don't power up before scan */
> +#define MMC_CAP2_HS400_1_8V	(1 << 15)	/* Can support HS400 1.8V */
> +#define MMC_CAP2_HS400_1_2V	(1 << 16)	/* Can support HS400 1.2V */
> +#define MMC_CAP2_HS400		(MMC_CAP2_HS400_1_8V | \
> +				 MMC_CAP2_HS400_1_2V)
> +#define MMC_CAP2_HSX00_1_8V	(MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V)
> +#define MMC_CAP2_HSX00_1_2V	(MMC_CAP2_HS200_1_2V_SDR | MMC_CAP2_HS400_1_2V)
> +#define MMC_CAP2_SDIO_IRQ_NOTHREAD (1 << 17)
> +#define MMC_CAP2_NO_WRITE_PROTECT (1 << 18)	/* No physical write protect pin, assume that card is always read-write */
> +#define MMC_CAP2_NO_SDIO	(1 << 19)	/* Do not send SDIO commands during initialization */
> +#define MMC_CAP2_HS400_ES	(1 << 20)	/* Host supports enhanced strobe */
> +#define MMC_CAP2_NO_SD		(1 << 21)	/* Do not send SD commands during initialization */
> +#define MMC_CAP2_NO_MMC		(1 << 22)	/* Do not send (e)MMC commands during initialization */
> +#define MMC_CAP2_CQE		(1 << 23)	/* Has eMMC command queue engine */
> +#define MMC_CAP2_CQE_DCMD	(1 << 24)	/* CQE can issue a direct command */
> +#define MMC_CAP2_AVOID_3_3V	(1 << 25)	/* Host must negotiate down from 3.3V */
> +#define MMC_CAP2_MERGE_CAPABLE	(1 << 26)	/* Host can merge a segment over the segment size */
> +#define MMC_CAP2_CRYPTO		0
>  	unsigned f_min;		/**< host interface lower limit */
>  	unsigned f_max;		/**< host interface upper limit */
>  	unsigned clock;		/**< Current clock used to talk to the card */
> @@ -478,8 +508,6 @@ struct mci_host {
>  	int use_dsr;		/**< optional dsr usage flag */
>  	int broken_cd;		/**< card detect is broken */
>  	bool non_removable;	/**< device is non removable */
> -	bool no_sd;		/**< do not send SD commands during initialization */
> -	bool no_sdio;		/**< do not send SDIO commands during initialization */
>  	bool disable_wp;	/**< ignore write-protect detection logic */
>  	struct regulator *supply;
>  
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 09/12] mci: mci-core: add HS200 support

[Steffen Trumtrar]

HS200 is a timing mode for eMMCs to work 8bit with 200MHz clocks.
To be used, drivers need to set the correct drive strength, clock phases
and then SDHCI can start tuning for HS200.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/Kconfig    |   7 ++
 drivers/mci/mci-core.c | 256 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/mci.h          |  53 +++++++++-
 3 files changed, 304 insertions(+), 12 deletions(-)

diff --git a/drivers/mci/Kconfig b/drivers/mci/Kconfig
index f569e24c0d..1e8c85643b 100644
--- a/drivers/mci/Kconfig
+++ b/drivers/mci/Kconfig
@@ -12,6 +12,13 @@ if MCI
 
 comment "--- Feature list ---"
 
+config MCI_TUNING
+	bool "EXPERIMENTAL - support MMC tuning for higher speeds"
+	help
+	  Say 'y' here if supporting drivers should do tuning to support
+	  higher clock speeds than 52 MHz SDR. MMC only; SD-Card max
+	  frequency is 50MHz SDR at present.
+
 config MCI_STARTUP
 	bool "Force probe on system start"
 	help
diff --git a/drivers/mci/mci-core.c b/drivers/mci/mci-core.c
index cd046149b4..b3a5884edd 100644
--- a/drivers/mci/mci-core.c
+++ b/drivers/mci/mci-core.c
@@ -1228,6 +1228,17 @@ static int mci_mmc_try_bus_width(struct mci *mci, enum mci_bus_width bus_width,
 	mci->host->timing = timing;
 	mci_set_bus_width(mci, bus_width);
 
+	switch (bus_width) {
+		case MMC_BUS_WIDTH_8:
+			mci->card_caps |= MMC_CAP_8_BIT_DATA;
+			break;
+		case MMC_BUS_WIDTH_4:
+			mci->card_caps |= MMC_CAP_4_BIT_DATA;
+			break;
+		default:
+			break;
+	}
+
 	err = mmc_compare_ext_csds(mci, bus_width);
 	if (err < 0)
 		goto out;
@@ -1301,10 +1312,220 @@ static int mci_mmc_select_hs_ddr(struct mci *mci)
 	return 0;
 }
 
+#ifdef CONFIG_MCI_TUNING
+/*
+ * Switch to the high-speed mode
+ */
+static int mmc_select_hs(struct mci *mci)
+{
+	int err;
+
+	err = mci_switch(mci, EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS);
+	if (err)
+		dev_warn(&mci->dev, "switch to high-speed failed, err:%d\n", err);
+
+	return err;
+}
+
+int mci_execute_tuning(struct mci *mci)
+{
+	struct mci_host *host = mci->host;
+	u32 opcode;
+	int err;
+
+	if (!host->execute_tuning)
+		return 0;
+
+	/* Tuning is only supported for MMC / HS200 */
+	if (mmc_card_hs200(mci))
+		opcode = MMC_SEND_TUNING_BLOCK_HS200;
+	else
+		return 0;
+
+	err = host->execute_tuning(host, opcode);
+	return err;
+}
+
+int mci_send_abort_tuning(struct mci *mci, u32 opcode)
+{
+	struct mci_cmd cmd = {};
+
+	/*
+	 * eMMC specification specifies that CMD12 can be used to stop a tuning
+	 * command, but SD specification does not, so do nothing unless it is
+	 * eMMC.
+	 */
+	if (opcode != MMC_SEND_TUNING_BLOCK_HS200)
+		return 0;
+
+	cmd.cmdidx = MMC_CMD_STOP_TRANSMISSION;
+	cmd.resp_type = MMC_RSP_SPI_R1 | MMC_RSP_R1 | MMC_CMD_AC;
+
+	return mci_send_cmd(mci, &cmd, NULL);
+}
+EXPORT_SYMBOL_GPL(mci_send_abort_tuning);
+
+static int mmc_select_max_dtr(struct mci *mci)
+{
+	u8 card_type = mci->ext_csd[EXT_CSD_DEVICE_TYPE];
+	u32 caps2 = mci->host->caps2;
+	u32 caps = mci->card_caps;
+	unsigned int hs_max_dtr = 0;
+	unsigned int hs200_max_dtr = 0;
+
+	if (caps & MMC_CAP_MMC_HIGHSPEED &&
+	    card_type & EXT_CSD_CARD_TYPE_26) {
+		hs_max_dtr = MMC_HIGH_26_MAX_DTR;
+	}
+
+	if (caps & MMC_CAP_MMC_HIGHSPEED &&
+	    card_type & EXT_CSD_CARD_TYPE_52) {
+		hs_max_dtr = MMC_HIGH_52_MAX_DTR;
+	}
+
+	if (caps2 & MMC_CAP2_HS200_1_8V_SDR &&
+	    card_type & EXT_CSD_CARD_TYPE_HS200_1_8V) {
+		hs200_max_dtr = MMC_HS200_MAX_DTR;
+	}
+
+	if (caps2 & MMC_CAP2_HS200_1_2V_SDR &&
+	    card_type & EXT_CSD_CARD_TYPE_HS200_1_2V) {
+		hs200_max_dtr = MMC_HS200_MAX_DTR;
+	}
+
+	mci->host->hs200_max_dtr = hs200_max_dtr;
+	mci->host->hs_max_dtr = hs_max_dtr;
+
+	return 0;
+}
+/*
+ * For device supporting HS200 mode, the following sequence
+ * should be done before executing the tuning process.
+ * 1. set the desired bus width(4-bit or 8-bit, 1-bit is not supported)
+ * 2. switch to HS200 mode
+ * 3. set the clock to > 52Mhz and <=200MHz
+ */
+static int mmc_select_hs200(struct mci *mci)
+{
+	unsigned int old_timing, old_clock;
+	int err = -EINVAL;
+	u8 val;
+
+	/*
+	 * Set the bus width(4 or 8) with host's support and
+	 * switch to HS200 mode if bus width is set successfully.
+	 */
+	/* find out maximum bus width and then try DDR if supported */
+	err = mci_mmc_select_bus_width(mci);
+	if (err > 0) {
+		u32 status;
+
+		/* TODO  actually set drive strength instead of 0. Currently unsupported. */
+		val = EXT_CSD_TIMING_HS200 | 0 << EXT_CSD_DRV_STR_SHIFT;
+		err = mci_switch(mci, EXT_CSD_HS_TIMING, val);
+		if (err)
+			goto err;
+
+		/*
+		 * Bump to HS timing and frequency. Some cards don't handle
+		 * SEND_STATUS reliably at the initial frequency.
+		 * NB: We can't move to full (HS200) speeds until after we've
+		 * successfully switched over.
+		 */
+		old_timing = mci->host->timing;
+		old_clock = mci->host->clock;
+
+		mci->host->timing = MMC_TIMING_MMC_HS200;
+		mci_set_ios(mci);
+		mci_set_clock(mci, mci->host->hs_max_dtr);
+
+		/*
+		 * For HS200, CRC errors are not a reliable way to know the
+		 * switch failed. If there really is a problem, we would expect
+		 * tuning will fail and the result ends up the same.
+		 */
+		err = mci_send_status(mci, &status);
+
+		/*
+		 * mmc_select_timing() assumes timing has not changed if
+		 * it is a switch error.
+		 */
+		if (err == -EBADMSG) {
+			mci->host->clock = old_clock;
+			mci->host->timing = old_timing;
+			mci_set_ios(mci);
+		}
+	}
+err:
+	if (err) {
+		dev_err(&mci->dev, "%s failed, error %d\n", __func__, err);
+	}
+	return err;
+}
+
+/*
+ * Set the bus speed for the selected speed mode.
+ */
+static void mmc_set_bus_speed(struct mci *mci)
+{
+	unsigned int max_dtr = (unsigned int)-1;
+
+	if (mmc_card_hs200(mci) &&
+		max_dtr > mci->host->hs200_max_dtr)
+		max_dtr = mci->host->hs200_max_dtr;
+	else if (mmc_card_hs(mci) && max_dtr > mci->host->hs_max_dtr)
+		max_dtr = mci->host->hs_max_dtr;
+	else if (max_dtr > 26000000)
+		max_dtr = 26000000;
+
+	mci_set_clock(mci, max_dtr);
+}
+
+/*
+ * Activate High Speed, HS200 or HS400ES mode if supported.
+ */
+int mmc_select_timing(struct mci *mci)
+{
+	unsigned int mmc_avail_type;
+	int err = 0;
+
+	mmc_select_max_dtr(mci);
+
+	mmc_avail_type = mci->ext_csd[EXT_CSD_DEVICE_TYPE] & EXT_CSD_CARD_TYPE_MASK;
+	if (mmc_avail_type & EXT_CSD_CARD_TYPE_HS200) {
+		err = mmc_select_hs200(mci);
+		if (err == -EBADMSG)
+			mmc_avail_type &= ~EXT_CSD_CARD_TYPE_HS200;
+		else
+			goto out;
+	}
+
+	if (mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
+		err = mmc_select_hs(mci);
+
+out:
+	if (err && err != -EBADMSG)
+		return err;
+
+	/*
+	 * Set the bus speed to the selected bus timing.
+	 * If timing is not selected, backward compatible is the default.
+	 */
+	mmc_set_bus_speed(mci);
+
+	return 0;
+}
+
+int mmc_hs200_tuning(struct mci *mci)
+{
+	return mci_execute_tuning(mci);
+}
+#endif
+
 static int mci_startup_mmc(struct mci *mci)
 {
 	struct mci_host *host = mci->host;
-	int ret;
+	int ret = 0;
 
 	/* if possible, speed up the transfer */
 	if (mci_caps(mci) & MMC_CAP_MMC_HIGHSPEED) {
@@ -1316,19 +1537,32 @@ static int mci_startup_mmc(struct mci *mci)
 		host->timing = MMC_TIMING_MMC_HS;
 	}
 
-	mci_set_clock(mci, mci->tran_speed);
+	if (IS_ENABLED(CONFIG_MCI_TUNING)) {
+		/*
+		 * Select timing interface
+		 */
+		ret = mmc_select_timing(mci);
+		if (ret)
+			return ret;
 
-	/* find out maximum bus width and then try DDR if supported */
-	ret = mci_mmc_select_bus_width(mci);
-	if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
-		ret = mci_mmc_select_hs_ddr(mci);
+		if (mmc_card_hs200(mci))
+			ret = mmc_hs200_tuning(mci);
+	}
 
-	if (ret < 0) {
-		dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);
-		return ret;
+	if (ret || !IS_ENABLED(CONFIG_MCI_TUNING)) {
+		mci_set_clock(mci, mci->tran_speed);
+
+		/* find out maximum bus width and then try DDR if supported */
+		ret = mci_mmc_select_bus_width(mci);
+		if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
+			ret = mci_mmc_select_hs_ddr(mci);
+
+		if (ret < 0) {
+			dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);
+		}
 	}
 
-	return 0;
+	return ret;
 }
 
 /**
@@ -1756,6 +1990,8 @@ static const char *mci_timing_tostr(unsigned timing)
 		return "SD HS";
 	case MMC_TIMING_MMC_DDR52:
 		return "MMC DDR52";
+	case MMC_TIMING_MMC_HS200:
+		return "HS200";
 	default:
 		return "unknown"; /* shouldn't happen */
 	}
diff --git a/include/mci.h b/include/mci.h
index ed2967c889..734e1239fb 100644
--- a/include/mci.h
+++ b/include/mci.h
@@ -82,6 +82,8 @@
 #define MMC_CMD_SET_BLOCKLEN		16
 #define MMC_CMD_READ_SINGLE_BLOCK	17
 #define MMC_CMD_READ_MULTIPLE_BLOCK	18
+#define MMC_SEND_TUNING_BLOCK		19   /* adtc R1  */
+#define MMC_SEND_TUNING_BLOCK_HS200	21   /* adtc R1  */
 #define MMC_CMD_WRITE_SINGLE_BLOCK	24
 #define MMC_CMD_WRITE_MULTIPLE_BLOCK	25
 #define MMC_CMD_APP_CMD			55
@@ -293,8 +295,8 @@
 #define EXT_CSD_CARD_TYPE_MASK		0x3f
 #define EXT_CSD_CARD_TYPE_26		(1<<0)	/* Card can run at 26MHz */
 #define EXT_CSD_CARD_TYPE_52		(1<<1)	/* Card can run at 52MHz */
-#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_HS_26 |	\
-					 EXT_CSD_CARD_TYPE_HS_52)
+#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_26 |	\
+					 EXT_CSD_CARD_TYPE_52)
 #define EXT_CSD_CARD_TYPE_DDR_1_8V	(1<<2)	/* Card can run at 52MHz */
 						/* DDR mode @1.8V or 3V I/O */
 #define EXT_CSD_CARD_TYPE_DDR_1_2V	(1<<3)	/* Card can run at 52MHz */
@@ -330,6 +332,12 @@
 #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
 #define EXT_CSD_DDR_FLAG	BIT(2)	/* Flag for DDR mode */
 
+#define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
+#define EXT_CSD_TIMING_HS	1	/* High speed */
+#define EXT_CSD_TIMING_HS200	2	/* HS200 */
+#define EXT_CSD_TIMING_HS400	3	/* HS400 */
+#define EXT_CSD_DRV_STR_SHIFT	4	/* Driver Strength shift */
+
 #define R1_ILLEGAL_COMMAND		(1 << 22)
 #define R1_STATUS(x)			(x & 0xFFF9A000)
 #define R1_CURRENT_STATE(x)		((x & 0x00001E00) >> 9)	/* sx, b (4 bits) */
@@ -503,6 +511,8 @@ struct mci_host {
 	unsigned clock;		/**< Current clock used to talk to the card */
 	enum mci_bus_width bus_width;	/**< used data bus width to the card */
 	enum mci_timing timing;	/**< used timing specification to the card */
+	unsigned hs_max_dtr;
+	unsigned hs200_max_dtr;
 	unsigned max_req_size;
 	unsigned dsr_val;	/**< optional dsr value */
 	int use_dsr;		/**< optional dsr usage flag */
@@ -521,6 +531,9 @@ struct mci_host {
 	int (*card_present)(struct mci_host *);
 	/** check if a card is write protected */
 	int (*card_write_protected)(struct mci_host *);
+	/* The tuning command opcode value is different for SD and eMMC cards */
+	int (*execute_tuning)(struct mci_host *, u32);
+	int (*platform_execute_tuning)(struct mci_host *host, u32 opcode);
 };
 
 #define MMC_NUM_BOOT_PARTITION	2
@@ -603,4 +616,40 @@ static inline struct mci *mci_get_device_by_devpath(const char *devpath)
 	return mci_get_device_by_name(devpath_to_name(devpath));
 }
 
+#define MMC_HIGH_26_MAX_DTR	26000000
+#define MMC_HIGH_52_MAX_DTR	52000000
+#define MMC_HIGH_DDR_MAX_DTR	52000000
+#define MMC_HS200_MAX_DTR	200000000
+
+static inline int mmc_card_hs(struct mci *mci)
+{
+	return mci->host->timing == MMC_TIMING_SD_HS ||
+		mci->host->timing == MMC_TIMING_MMC_HS;
+}
+
+#ifdef CONFIG_MCI_TUNING
+/*
+ * Execute tuning sequence to seek the proper bus operating
+ * conditions for HS200 and HS400, which sends CMD21 to the device.
+ */
+int mmc_hs200_tuning(struct mci *mci);
+int mci_execute_tuning(struct mci *mci);
+int mci_send_abort_tuning(struct mci *mci, u32 opcode);
+int mmc_select_timing(struct mci *mci);
+#else
+static inline int mmc_hs200_tuning(struct mci *mci)
+{
+	return -ENOSYS;
+}
+static inline int mmc_select_timing(struct mci *mci)
+{
+	return -ENOSYS;
+}
+#endif
+
+static inline bool mmc_card_hs200(struct mci *mci)
+{
+	return mci->host->timing == MMC_TIMING_MMC_HS200;
+}
+
 #endif /* _MCI_H_ */

-- 
2.40.1

Re: [PATCH 09/12] mci: mci-core: add HS200 support

[Ahmad Fatoum]

Hello Steffen,

On 08.03.24 12:17, Steffen Trumtrar wrote:
> +	switch (bus_width) {
> +		case MMC_BUS_WIDTH_8:
> +			mci->card_caps |= MMC_CAP_8_BIT_DATA;
> +			break;
> +		case MMC_BUS_WIDTH_4:
> +			mci->card_caps |= MMC_CAP_4_BIT_DATA;
> +			break;
> +		default:
> +			break;
> +	}

This is needed to make devinfo output less confusing, right?

> +#ifdef CONFIG_MCI_TUNING

For compile-time coverage, drop the #ifdef and reference the code
inside a if (IS_ENABLED(CONFIG_MCI_TUNING)) {}

> +/*
> + * Switch to the high-speed mode
> + */
> +static int mmc_select_hs(struct mci *mci)
> +{
> +	int err;
> +
> +	err = mci_switch(mci, EXT_CSD_HS_TIMING, EXT_CSD_TIMING_HS);
> +	if (err)
> +		dev_warn(&mci->dev, "switch to high-speed failed, err:%d\n", err);
> +
> +	return err;
> +}

This duplicate codes already existing in mmc_change_freq. 

> +
> +int mci_execute_tuning(struct mci *mci)
> +{
> +	struct mci_host *host = mci->host;
> +	u32 opcode;
> +	int err;
> +
> +	if (!host->execute_tuning)
> +		return 0;
> +
> +	/* Tuning is only supported for MMC / HS200 */
> +	if (mmc_card_hs200(mci))
> +		opcode = MMC_SEND_TUNING_BLOCK_HS200;
> +	else
> +		return 0;
> +
> +	err = host->execute_tuning(host, opcode);
> +	return err;

You can return host->execute_tuning(host, opcode); directly here.

> +static int mmc_select_max_dtr(struct mci *mci)
> +{
> +	u8 card_type = mci->ext_csd[EXT_CSD_DEVICE_TYPE];
> +	u32 caps2 = mci->host->caps2;
> +	u32 caps = mci->card_caps;
> +	unsigned int hs_max_dtr = 0;
> +	unsigned int hs200_max_dtr = 0;
> +
> +	if (caps & MMC_CAP_MMC_HIGHSPEED &&
> +	    card_type & EXT_CSD_CARD_TYPE_26) {

Nitpick: Parenthesis around the terms on both side of && for clarity.

> +		hs_max_dtr = MMC_HIGH_26_MAX_DTR;
> +	}
> +
> +	if (caps & MMC_CAP_MMC_HIGHSPEED &&
> +	    card_type & EXT_CSD_CARD_TYPE_52) {
> +		hs_max_dtr = MMC_HIGH_52_MAX_DTR;
> +	}
> +
> +	if (caps2 & MMC_CAP2_HS200_1_8V_SDR &&
> +	    card_type & EXT_CSD_CARD_TYPE_HS200_1_8V) {
> +		hs200_max_dtr = MMC_HS200_MAX_DTR;
> +	}
> +
> +	if (caps2 & MMC_CAP2_HS200_1_2V_SDR &&
> +	    card_type & EXT_CSD_CARD_TYPE_HS200_1_2V) {
> +		hs200_max_dtr = MMC_HS200_MAX_DTR;
> +	}
> +
> +	mci->host->hs200_max_dtr = hs200_max_dtr;
> +	mci->host->hs_max_dtr = hs_max_dtr;
> +
> +	return 0;

Can be void instead.

> +}
> +/*
> + * For device supporting HS200 mode, the following sequence
> + * should be done before executing the tuning process.
> + * 1. set the desired bus width(4-bit or 8-bit, 1-bit is not supported)
> + * 2. switch to HS200 mode
> + * 3. set the clock to > 52Mhz and <=200MHz
> + */
> +static int mmc_select_hs200(struct mci *mci)
> +{
> +	unsigned int old_timing, old_clock;
> +	int err = -EINVAL;
> +	u8 val;
> +
> +	/*
> +	 * Set the bus width(4 or 8) with host's support and
> +	 * switch to HS200 mode if bus width is set successfully.
> +	 */
> +	/* find out maximum bus width and then try DDR if supported */
> +	err = mci_mmc_select_bus_width(mci);
> +	if (err > 0) {
> +		u32 status;
> +
> +		/* TODO  actually set drive strength instead of 0. Currently unsupported. */
> +		val = EXT_CSD_TIMING_HS200 | 0 << EXT_CSD_DRV_STR_SHIFT;
> +		err = mci_switch(mci, EXT_CSD_HS_TIMING, val);
> +		if (err)
> +			goto err;
> +
> +		/*
> +		 * Bump to HS timing and frequency. Some cards don't handle
> +		 * SEND_STATUS reliably at the initial frequency.
> +		 * NB: We can't move to full (HS200) speeds until after we've
> +		 * successfully switched over.
> +		 */
> +		old_timing = mci->host->timing;
> +		old_clock = mci->host->clock;
> +
> +		mci->host->timing = MMC_TIMING_MMC_HS200;
> +		mci_set_ios(mci);
> +		mci_set_clock(mci, mci->host->hs_max_dtr);
> +
> +		/*
> +		 * For HS200, CRC errors are not a reliable way to know the
> +		 * switch failed. If there really is a problem, we would expect
> +		 * tuning will fail and the result ends up the same.
> +		 */

Linux comment no longer aligns with barebox code. The comment explains why the kernel
doesn't fail on CRC errors here, but mci_send_status returns -EILSEQ normally.

While not relevant to your Arasan use case, other drivers return -EILSEQ on
CRC errors and SDHCI drivers might start doing so too in future, so please
port mmc_switch_status() from Linux and use below.

> +		err = mci_send_status(mci, &status);
> +
> +		/*
> +		 * mmc_select_timing() assumes timing has not changed if
> +		 * it is a switch error.
> +		 */
> +		if (err == -EBADMSG) {
> +			mci->host->clock = old_clock;
> +			mci->host->timing = old_timing;
> +			mci_set_ios(mci);
> +		}
> +	}
> +err:
> +	if (err) {
> +		dev_err(&mci->dev, "%s failed, error %d\n", __func__, err);
> +	}
> +	return err;
> +}
> +
> +/*
> + * Set the bus speed for the selected speed mode.
> + */
> +static void mmc_set_bus_speed(struct mci *mci)
> +{
> +	unsigned int max_dtr = (unsigned int)-1;
> +
> +	if (mmc_card_hs200(mci) &&
> +		max_dtr > mci->host->hs200_max_dtr)
> +		max_dtr = mci->host->hs200_max_dtr;
> +	else if (mmc_card_hs(mci) && max_dtr > mci->host->hs_max_dtr)
> +		max_dtr = mci->host->hs_max_dtr;
> +	else if (max_dtr > 26000000)
> +		max_dtr = 26000000;

Why hardcode this when Linux uses card->csd.max_dtr?

> +
> +	mci_set_clock(mci, max_dtr);
> +}
> +
> +/*
> + * Activate High Speed, HS200 or HS400ES mode if supported.
> + */
> +int mmc_select_timing(struct mci *mci)
> +{
> +	unsigned int mmc_avail_type;
> +	int err = 0;
> +
> +	mmc_select_max_dtr(mci);
> +
> +	mmc_avail_type = mci->ext_csd[EXT_CSD_DEVICE_TYPE] & EXT_CSD_CARD_TYPE_MASK;
> +	if (mmc_avail_type & EXT_CSD_CARD_TYPE_HS200) {
> +		err = mmc_select_hs200(mci);
> +		if (err == -EBADMSG)
> +			mmc_avail_type &= ~EXT_CSD_CARD_TYPE_HS200;
> +		else
> +			goto out;
> +	}

If HS200 doesn't succeed, DDR52 should be tried next. This is what mci_startup_mmc
is already doing, so it seems to me you should incorporate this code and remove
the mmc_select_hs duplication. 

> +
> +	if (mmc_avail_type & EXT_CSD_CARD_TYPE_HS)
> +		err = mmc_select_hs(mci);
> +
> +out:
> +	if (err && err != -EBADMSG)
> +		return err;
> +
> +	/*
> +	 * Set the bus speed to the selected bus timing.
> +	 * If timing is not selected, backward compatible is the default.
> +	 */
> +	mmc_set_bus_speed(mci);
> +
> +	return 0;
> +}
> +
> +int mmc_hs200_tuning(struct mci *mci)
> +{
> +	return mci_execute_tuning(mci);
> +}
> +#endif
> +
>  static int mci_startup_mmc(struct mci *mci)
>  {
>  	struct mci_host *host = mci->host;
> -	int ret;
> +	int ret = 0;
>  
>  	/* if possible, speed up the transfer */
>  	if (mci_caps(mci) & MMC_CAP_MMC_HIGHSPEED) {
> @@ -1316,19 +1537,32 @@ static int mci_startup_mmc(struct mci *mci)
>  		host->timing = MMC_TIMING_MMC_HS;
>  	}
>  
> -	mci_set_clock(mci, mci->tran_speed);
> +	if (IS_ENABLED(CONFIG_MCI_TUNING)) {
> +		/*
> +		 * Select timing interface
> +		 */
> +		ret = mmc_select_timing(mci);
> +		if (ret)
> +			return ret;
>  
> -	/* find out maximum bus width and then try DDR if supported */
> -	ret = mci_mmc_select_bus_width(mci);
> -	if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
> -		ret = mci_mmc_select_hs_ddr(mci);
> +		if (mmc_card_hs200(mci))
> +			ret = mmc_hs200_tuning(mci);
> +	}
>  
> -	if (ret < 0) {
> -		dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);
> -		return ret;
> +	if (ret || !IS_ENABLED(CONFIG_MCI_TUNING)) {

As mentioned above, this duplicates code and precludes use of DDR52 when CONFIG_MCI_TUNING
is enabled.

> +		mci_set_clock(mci, mci->tran_speed);
> +
> +		/* find out maximum bus width and then try DDR if supported */
> +		ret = mci_mmc_select_bus_width(mci);
> +		if (ret > MMC_BUS_WIDTH_1 && mci->tran_speed == 52000000)
> +			ret = mci_mmc_select_hs_ddr(mci);
> +
> +		if (ret < 0) {
> +			dev_warn(&mci->dev, "Changing MMC bus width failed: %d\n", ret);
> +		}
>  	}
>  
> -	return 0;
> +	return ret;
>  }
>  
>  /**
> @@ -1756,6 +1990,8 @@ static const char *mci_timing_tostr(unsigned timing)
>  		return "SD HS";
>  	case MMC_TIMING_MMC_DDR52:
>  		return "MMC DDR52";
> +	case MMC_TIMING_MMC_HS200:
> +		return "HS200";
>  	default:
>  		return "unknown"; /* shouldn't happen */
>  	}
> diff --git a/include/mci.h b/include/mci.h
> index ed2967c889..734e1239fb 100644
> --- a/include/mci.h
> +++ b/include/mci.h
> @@ -82,6 +82,8 @@
>  #define MMC_CMD_SET_BLOCKLEN		16
>  #define MMC_CMD_READ_SINGLE_BLOCK	17
>  #define MMC_CMD_READ_MULTIPLE_BLOCK	18
> +#define MMC_SEND_TUNING_BLOCK		19   /* adtc R1  */
> +#define MMC_SEND_TUNING_BLOCK_HS200	21   /* adtc R1  */
>  #define MMC_CMD_WRITE_SINGLE_BLOCK	24
>  #define MMC_CMD_WRITE_MULTIPLE_BLOCK	25
>  #define MMC_CMD_APP_CMD			55
> @@ -293,8 +295,8 @@
>  #define EXT_CSD_CARD_TYPE_MASK		0x3f
>  #define EXT_CSD_CARD_TYPE_26		(1<<0)	/* Card can run at 26MHz */
>  #define EXT_CSD_CARD_TYPE_52		(1<<1)	/* Card can run at 52MHz */
> -#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_HS_26 |	\
> -					 EXT_CSD_CARD_TYPE_HS_52)
> +#define EXT_CSD_CARD_TYPE_HS		(EXT_CSD_CARD_TYPE_26 |	\
> +					 EXT_CSD_CARD_TYPE_52)
>  #define EXT_CSD_CARD_TYPE_DDR_1_8V	(1<<2)	/* Card can run at 52MHz */
>  						/* DDR mode @1.8V or 3V I/O */
>  #define EXT_CSD_CARD_TYPE_DDR_1_2V	(1<<3)	/* Card can run at 52MHz */
> @@ -330,6 +332,12 @@
>  #define EXT_CSD_DDR_BUS_WIDTH_8	6	/* Card is in 8 bit DDR mode */
>  #define EXT_CSD_DDR_FLAG	BIT(2)	/* Flag for DDR mode */
>  
> +#define EXT_CSD_TIMING_BC	0	/* Backwards compatility */
> +#define EXT_CSD_TIMING_HS	1	/* High speed */
> +#define EXT_CSD_TIMING_HS200	2	/* HS200 */
> +#define EXT_CSD_TIMING_HS400	3	/* HS400 */
> +#define EXT_CSD_DRV_STR_SHIFT	4	/* Driver Strength shift */
> +
>  #define R1_ILLEGAL_COMMAND		(1 << 22)
>  #define R1_STATUS(x)			(x & 0xFFF9A000)
>  #define R1_CURRENT_STATE(x)		((x & 0x00001E00) >> 9)	/* sx, b (4 bits) */
> @@ -503,6 +511,8 @@ struct mci_host {
>  	unsigned clock;		/**< Current clock used to talk to the card */
>  	enum mci_bus_width bus_width;	/**< used data bus width to the card */
>  	enum mci_timing timing;	/**< used timing specification to the card */
> +	unsigned hs_max_dtr;
> +	unsigned hs200_max_dtr;
>  	unsigned max_req_size;
>  	unsigned dsr_val;	/**< optional dsr value */
>  	int use_dsr;		/**< optional dsr usage flag */
> @@ -521,6 +531,9 @@ struct mci_host {
>  	int (*card_present)(struct mci_host *);
>  	/** check if a card is write protected */
>  	int (*card_write_protected)(struct mci_host *);
> +	/* The tuning command opcode value is different for SD and eMMC cards */
> +	int (*execute_tuning)(struct mci_host *, u32);
> +	int (*platform_execute_tuning)(struct mci_host *host, u32 opcode);

In Linux, this is a member of struct sdhci_ops, which in barebox would be struct sdhci.
Adding in here is out-of-place.

>  };
>  
>  #define MMC_NUM_BOOT_PARTITION	2
> @@ -603,4 +616,40 @@ static inline struct mci *mci_get_device_by_devpath(const char *devpath)
>  	return mci_get_device_by_name(devpath_to_name(devpath));
>  }
>  
> +#define MMC_HIGH_26_MAX_DTR	26000000
> +#define MMC_HIGH_52_MAX_DTR	52000000
> +#define MMC_HIGH_DDR_MAX_DTR	52000000
> +#define MMC_HS200_MAX_DTR	200000000
> +
> +static inline int mmc_card_hs(struct mci *mci)
> +{
> +	return mci->host->timing == MMC_TIMING_SD_HS ||
> +		mci->host->timing == MMC_TIMING_MMC_HS;
> +}
> +
> +#ifdef CONFIG_MCI_TUNING
> +/*
> + * Execute tuning sequence to seek the proper bus operating
> + * conditions for HS200 and HS400, which sends CMD21 to the device.
> + */
> +int mmc_hs200_tuning(struct mci *mci);
> +int mci_execute_tuning(struct mci *mci);
> +int mci_send_abort_tuning(struct mci *mci, u32 opcode);
> +int mmc_select_timing(struct mci *mci);
> +#else
> +static inline int mmc_hs200_tuning(struct mci *mci)
> +{
> +	return -ENOSYS;
> +}
> +static inline int mmc_select_timing(struct mci *mci)
> +{
> +	return -ENOSYS;
> +}
> +#endif
> +
> +static inline bool mmc_card_hs200(struct mci *mci)
> +{
> +	return mci->host->timing == MMC_TIMING_MMC_HS200;
> +}

You can drop this once the #ifdef in the source is removed.

Cheers,
Ahmad

> +
>  #endif /* _MCI_H_ */
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 10/12] mci: sdhci: add tuning support

[Steffen Trumtrar]

The arasan SDHCI driver doesn't use MMC tuning but SDHCI tuning. Both
are not supported yet in barebox.
Add SDHCI tuning support from linux v6.7.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/mci/sdhci.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/mci/sdhci.h |  24 ++++++
 2 files changed, 267 insertions(+), 1 deletion(-)

diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index edb819d66d..62e6443e64 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -9,6 +9,196 @@
 
 #include "sdhci.h"
 
+#define MAX_TUNING_LOOP 40
+#define  SDHCI_MAKE_BLKSZ(dma, blksz) (((dma & 0x7) << 12) | (blksz & 0xFFF))
+
+#ifdef CONFIG_MCI_TUNING
+static int sdhci_send_command_retry(struct sdhci *host, struct mci_cmd *cmd)
+{
+	int timeout = 10;
+
+	while ((sdhci_read32(host, SDHCI_PRESENT_STATE) & SDHCI_CMD_INHIBIT_CMD)) {
+		if (!timeout--)
+			return -ETIMEDOUT;
+
+		mdelay(1);
+	}
+
+	return host->mci->send_cmd(host->mci, cmd, NULL);
+}
+
+/*
+ * We use sdhci_send_tuning() because mmc_send_tuning() is not a good fit. SDHCI
+ * tuning command does not have a data payload (or rather the hardware does it
+ * automatically) so mmc_send_tuning() will return -EIO. Also the tuning command
+ * interrupt setup is different to other commands and there is no timeout
+ * interrupt so special handling is needed.
+ */
+static int sdhci_send_tuning(struct sdhci *host, u32 opcode)
+{
+	struct mci_cmd cmd = {};
+	int ret;
+
+	cmd.cmdidx = opcode;
+	cmd.resp_type = MMC_RSP_R1 | MMC_CMD_ADTC;
+
+	/*
+	 * In response to CMD19, the card sends 64 bytes of tuning
+	 * block to the Host Controller. So we set the block size
+	 * to 64 here.
+	 */
+	if (cmd.cmdidx == MMC_SEND_TUNING_BLOCK_HS200 &&
+	    host->mci->bus_width == MMC_BUS_WIDTH_8) {
+		sdhci_write16(host, SDHCI_BLOCK_SIZE, SDHCI_MAKE_BLKSZ(7, 128));
+	} else {
+		sdhci_write16(host, SDHCI_BLOCK_SIZE, SDHCI_MAKE_BLKSZ(7, 64));
+	}
+
+	ret = sdhci_send_command_retry(host, &cmd);
+
+	udelay(100);
+
+	return ret;
+}
+
+static void sdhci_end_tuning(struct sdhci *host)
+{
+	sdhci_write32(host, SDHCI_INT_ENABLE, host->tuning_old_ier);
+	sdhci_write32(host, SDHCI_SIGNAL_ENABLE, host->tuning_old_sig);
+}
+
+static void sdhci_start_tuning(struct sdhci *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
+	ctrl |= SDHCI_CTRL_EXEC_TUNING;
+	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl);
+
+	mdelay(1);
+
+	host->tuning_old_ier = sdhci_read32(host, SDHCI_INT_ENABLE);
+	host->tuning_old_sig = sdhci_read32(host, SDHCI_SIGNAL_ENABLE);
+
+	sdhci_write32(host, SDHCI_INT_ENABLE, SDHCI_INT_DATA_AVAIL);
+	sdhci_write32(host, SDHCI_SIGNAL_ENABLE, SDHCI_INT_DATA_AVAIL);
+}
+
+static void sdhci_reset_tuning(struct sdhci *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
+	ctrl &= ~SDHCI_CTRL_TUNED_CLK;
+	ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
+	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl);
+}
+
+static void sdhci_abort_tuning(struct sdhci *host, u32 opcode)
+{
+	sdhci_reset_tuning(host);
+
+	sdhci_reset(host, SDHCI_RESET_CMD);
+	sdhci_reset(host, SDHCI_RESET_DATA);
+
+	sdhci_end_tuning(host);
+
+	mci_send_abort_tuning(host->mci->mci, opcode);
+}
+
+static int __sdhci_execute_tuning(struct sdhci *host, u32 opcode)
+{
+	int i;
+	int ret;
+
+	/*
+	 * Issue opcode repeatedly till Execute Tuning is set to 0 or the number
+	 * of loops reaches tuning loop count.
+	 */
+	for (i = 0; i < host->tuning_loop_count; i++) {
+		u16 ctrl;
+
+		ret = sdhci_send_tuning(host, opcode);
+		if (ret) {
+			sdhci_abort_tuning(host, opcode);
+			return -ETIMEDOUT;
+		}
+
+		/* Spec does not require a delay between tuning cycles */
+		if (host->tuning_delay > 0)
+			mdelay(host->tuning_delay);
+
+		ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
+		if (!(ctrl & SDHCI_CTRL_EXEC_TUNING)) {
+			if (ctrl & SDHCI_CTRL_TUNED_CLK) {
+				return 0; /* Success! */
+			}
+			break;
+		}
+
+	}
+
+	dev_dbg(&host->mci->mci->dev, "Tuning timeout, falling back to fixed sampling clock\n");
+	sdhci_reset_tuning(host);
+	return -EAGAIN;
+}
+
+int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode)
+{
+	struct mci_host *host = sdhci->mci;
+	int err = 0;
+	unsigned int tuning_count = 0;
+
+	if (sdhci->tuning_mode == SDHCI_TUNING_MODE_1)
+		tuning_count = sdhci->tuning_count;
+
+	/*
+	 * The Host Controller needs tuning in case of SDR104 and DDR50
+	 * mode, and for SDR50 mode when Use Tuning for SDR50 is set in
+	 * the Capabilities register.
+	 * If the Host Controller supports the HS200 mode then the
+	 * tuning function has to be executed.
+	 */
+	switch (host->timing) {
+	/* HS400 tuning is done in HS200 mode */
+	case MMC_TIMING_MMC_HS400:
+		err = -EINVAL;
+		goto out;
+
+	case MMC_TIMING_MMC_HS200:
+		break;
+
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_UHS_DDR50:
+		break;
+
+	case MMC_TIMING_UHS_SDR50:
+		fallthrough;
+
+	default:
+		goto out;
+	}
+
+	if (host->platform_execute_tuning) {
+		err = host->platform_execute_tuning(host, opcode);
+		goto out;
+	}
+
+	if (sdhci->tuning_delay < 0)
+		sdhci->tuning_delay = opcode == MMC_SEND_TUNING_BLOCK;
+
+	sdhci_start_tuning(sdhci);
+
+	sdhci->tuning_err = __sdhci_execute_tuning(sdhci, opcode);
+
+	sdhci_end_tuning(sdhci);
+out:
+
+	return err;
+}
+EXPORT_SYMBOL_GPL(sdhci_execute_tuning);
+#endif
+
 void sdhci_read_response(struct sdhci *sdhci, struct mci_cmd *cmd)
 {
 	if (cmd->resp_type & MMC_RSP_136) {
@@ -50,7 +240,12 @@ void sdhci_set_cmd_xfer_mode(struct sdhci *host, struct mci_cmd *cmd,
 
 	*command |= SDHCI_CMD_INDEX(cmd->cmdidx);
 
-	if (data) {
+	if (cmd->cmdidx == MMC_SEND_TUNING_BLOCK ||
+	    cmd->cmdidx == MMC_SEND_TUNING_BLOCK_HS200) {
+		*command |= SDHCI_DATA_PRESENT;
+		*xfer = SDHCI_DATA_TO_HOST;
+
+	} else if (data) {
 		*command |= SDHCI_DATA_PRESENT;
 
 		*xfer |= SDHCI_BLOCK_COUNT_EN;
@@ -111,6 +306,31 @@ void sdhci_set_bus_width(struct sdhci *host, int width)
 	sdhci_write8(host, SDHCI_HOST_CONTROL, ctrl);
 }
 
+static void sdhci_set_uhs_signaling(struct sdhci *host, unsigned timing)
+{
+	u16 ctrl_2;
+
+	ctrl_2 = sdhci_read16(host, SDHCI_HOST_CONTROL2);
+	/* Select Bus Speed Mode for host */
+	ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
+	if ((timing == MMC_TIMING_MMC_HS200) ||
+	    (timing == MMC_TIMING_UHS_SDR104))
+		ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
+	else if (timing == MMC_TIMING_UHS_SDR12)
+		ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
+	else if (timing == MMC_TIMING_UHS_SDR25)
+		ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
+	else if (timing == MMC_TIMING_UHS_SDR50)
+		ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
+	else if ((timing == MMC_TIMING_UHS_DDR50) ||
+		 (timing == MMC_TIMING_MMC_DDR52))
+		ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
+	else if (timing == MMC_TIMING_MMC_HS400)
+		ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
+	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl_2);
+}
+EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
+
 static inline bool sdhci_can_64bit_dma(struct sdhci *host)
 {
 	/*
@@ -567,6 +787,8 @@ void sdhci_set_clock(struct sdhci *host, unsigned int clock, unsigned int input_
 
 	host->mci->clock = 0;
 
+	sdhci_set_uhs_signaling(host, host->mci->timing);
+
 	sdhci_wait_idle(host, NULL);
 
 	sdhci_write16(host, SDHCI_CLOCK_CONTROL, 0);
@@ -695,5 +917,25 @@ int sdhci_setup_host(struct sdhci *host)
 	if (sdhci_can_64bit_dma(host))
 		host->flags |= SDHCI_USE_64_BIT_DMA;
 
+	if ((mci->caps2 & (MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V)))
+		host->flags |= SDHCI_SIGNALING_180;
+
+	host->tuning_delay = -1;
+	host->tuning_loop_count = MAX_TUNING_LOOP;
+
+	/* Initial value for re-tuning timer count */
+	host->tuning_count = FIELD_GET(SDHCI_RETUNING_TIMER_COUNT_MASK,
+				       host->caps1);
+
+	/*
+	 * In case Re-tuning Timer is not disabled, the actual value of
+	 * re-tuning timer will be 2 ^ (n - 1).
+	 */
+	if (host->tuning_count)
+		host->tuning_count = 1 << (host->tuning_count - 1);
+
+	/* Re-tuning mode supported by the Host Controller */
+	host->tuning_mode = FIELD_GET(SDHCI_RETUNING_MODE_MASK, host->caps1);
+
 	return 0;
 }
diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
index 2139498855..80916e670f 100644
--- a/drivers/mci/sdhci.h
+++ b/drivers/mci/sdhci.h
@@ -28,9 +28,11 @@
 #define SDHCI_TRANSFER_MODE					0x0c
 #define  SDHCI_MULTIPLE_BLOCKS			BIT(5)
 #define  SDHCI_DATA_TO_HOST			BIT(4)
+#define  SDHCI_TRNS_AUTO_CMD12			BIT(3)
 #define  SDHCI_BLOCK_COUNT_EN			BIT(1)
 #define  SDHCI_DMA_EN				BIT(0)
 #define SDHCI_COMMAND						0x0e
+#define  SDHCI_MAKE_CMD(c, f) 			(((c & 0xff) << 8) | (f & 0xff))
 #define  SDHCI_CMD_INDEX(c)			(((c) & 0x3f) << 8)
 #define  SDHCI_COMMAND_CMDTYP_SUSPEND		(1 << 6)
 #define  SDHCI_COMMAND_CMDTYP_RESUME		(2 << 6)
@@ -121,6 +123,15 @@
 #define SDHCI_SIGNAL_ENABLE					0x38
 #define SDHCI_ACMD12_ERR__HOST_CONTROL2				0x3C
 #define SDHCI_HOST_CONTROL2					0x3E
+#define  SDHCI_CTRL_UHS_MASK			GENMASK(3, 0)
+#define   SDHCI_CTRL_UHS_SDR12			0x0
+#define   SDHCI_CTRL_UHS_SDR25			0x1
+#define   SDHCI_CTRL_UHS_SDR50			0x2
+#define   SDHCI_CTRL_UHS_SDR104			0x3
+#define   SDHCI_CTRL_UHS_DDR50			0x4
+#define   SDHCI_CTRL_HS400			0x5 /* Non-standard */
+#define  SDHCI_CTRL_EXEC_TUNING			BIT(6)
+#define  SDHCI_CTRL_TUNED_CLK			BIT(7)
 #define  SDHCI_CTRL_64BIT_ADDR			BIT(13)
 #define  SDHCI_CTRL_V4_MODE			BIT(12)
 #define SDHCI_CAPABILITIES					0x40
@@ -230,6 +241,18 @@ struct sdhci {
 	bool read_caps;	/* Capability flags have been read */
 	u32 sdma_boundary;
 
+	unsigned int		tuning_count;	/* Timer count for re-tuning */
+	unsigned int		tuning_mode;	/* Re-tuning mode supported by host */
+	unsigned int		tuning_err;	/* Error code for re-tuning */
+#define SDHCI_TUNING_MODE_1	0
+#define SDHCI_TUNING_MODE_2	1
+#define SDHCI_TUNING_MODE_3	2
+	/* Delay (ms) between tuning commands */
+	int			tuning_delay;
+	int			tuning_loop_count;
+	int			tuning_old_ier;
+	int			tuning_old_sig;
+
 	struct mci_host	*mci;
 };
 
@@ -282,6 +305,7 @@ static inline void sdhci_write8(struct sdhci *host, int reg, u32 val)
 }
 
 #define SDHCI_NO_DMA DMA_ERROR_CODE
+int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode);
 int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd);
 int sdhci_wait_for_done(struct sdhci *host, u32 mask);
 void sdhci_read_response(struct sdhci *host, struct mci_cmd *cmd);

-- 
2.40.1

Re: [PATCH 10/12] mci: sdhci: add tuning support

[Ahmad Fatoum]

Hello Steffen,

On 08.03.24 12:17, Steffen Trumtrar wrote:
> The arasan SDHCI driver doesn't use MMC tuning but SDHCI tuning. Both
> are not supported yet in barebox.
> Add SDHCI tuning support from linux v6.7.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>  drivers/mci/sdhci.c | 244 +++++++++++++++++++++++++++++++++++++++++++++++++++-
>  drivers/mci/sdhci.h |  24 ++++++
>  2 files changed, 267 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
> index edb819d66d..62e6443e64 100644
> --- a/drivers/mci/sdhci.c
> +++ b/drivers/mci/sdhci.c
> @@ -9,6 +9,196 @@
>  
>  #include "sdhci.h"
>  
> +#define MAX_TUNING_LOOP 40
> +#define  SDHCI_MAKE_BLKSZ(dma, blksz) (((dma & 0x7) << 12) | (blksz & 0xFFF))
> +
> +#ifdef CONFIG_MCI_TUNING

As with mci-core, use only IS_ENABLED() for compile coverage.


> +static int sdhci_send_command_retry(struct sdhci *host, struct mci_cmd *cmd)
> +{
> +	int timeout = 10;
> +
> +	while ((sdhci_read32(host, SDHCI_PRESENT_STATE) & SDHCI_CMD_INHIBIT_CMD)) {
> +		if (!timeout--)
> +			return -ETIMEDOUT;
> +
> +		mdelay(1);
> +	}
> +
> +	return host->mci->send_cmd(host->mci, cmd, NULL);
> +}
> +
> +/*
> + * We use sdhci_send_tuning() because mmc_send_tuning() is not a good fit. SDHCI
> + * tuning command does not have a data payload (or rather the hardware does it
> + * automatically) so mmc_send_tuning() will return -EIO. Also the tuning command
> + * interrupt setup is different to other commands and there is no timeout
> + * interrupt so special handling is needed.
> + */
> +static int sdhci_send_tuning(struct sdhci *host, u32 opcode)
> +{
> +	struct mci_cmd cmd = {};
> +	int ret;
> +
> +	cmd.cmdidx = opcode;
> +	cmd.resp_type = MMC_RSP_R1 | MMC_CMD_ADTC;
> +
> +	/*
> +	 * In response to CMD19, the card sends 64 bytes of tuning
> +	 * block to the Host Controller. So we set the block size
> +	 * to 64 here.
> +	 */
> +	if (cmd.cmdidx == MMC_SEND_TUNING_BLOCK_HS200 &&
> +	    host->mci->bus_width == MMC_BUS_WIDTH_8) {
> +		sdhci_write16(host, SDHCI_BLOCK_SIZE, SDHCI_MAKE_BLKSZ(7, 128));
> +	} else {
> +		sdhci_write16(host, SDHCI_BLOCK_SIZE, SDHCI_MAKE_BLKSZ(7, 64));
> +	}
> +
> +	ret = sdhci_send_command_retry(host, &cmd);
> +
> +	udelay(100);

IIRC, this was debugging left-over in case Linux context switch here influenced
the result. I think this can be dropped.

> +
> +	return ret;
> +}
> +
> +static void sdhci_end_tuning(struct sdhci *host)
> +{
> +	sdhci_write32(host, SDHCI_INT_ENABLE, host->tuning_old_ier);
> +	sdhci_write32(host, SDHCI_SIGNAL_ENABLE, host->tuning_old_sig);
> +}
> +
> +static void sdhci_start_tuning(struct sdhci *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
> +	ctrl |= SDHCI_CTRL_EXEC_TUNING;
> +	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl);
> +
> +	mdelay(1);
> +
> +	host->tuning_old_ier = sdhci_read32(host, SDHCI_INT_ENABLE);
> +	host->tuning_old_sig = sdhci_read32(host, SDHCI_SIGNAL_ENABLE);
> +
> +	sdhci_write32(host, SDHCI_INT_ENABLE, SDHCI_INT_DATA_AVAIL);
> +	sdhci_write32(host, SDHCI_SIGNAL_ENABLE, SDHCI_INT_DATA_AVAIL);
> +}
> +
> +static void sdhci_reset_tuning(struct sdhci *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
> +	ctrl &= ~SDHCI_CTRL_TUNED_CLK;
> +	ctrl &= ~SDHCI_CTRL_EXEC_TUNING;
> +	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl);
> +}
> +
> +static void sdhci_abort_tuning(struct sdhci *host, u32 opcode)
> +{
> +	sdhci_reset_tuning(host);
> +
> +	sdhci_reset(host, SDHCI_RESET_CMD);
> +	sdhci_reset(host, SDHCI_RESET_DATA);

I fear this might get "cleaned up" in future to be SDHCI_RESET_CMD | SDHCI_RESET_DATA,
which is wrong. Could you port sdhci_reset_for_reason over and use it?

> +
> +	sdhci_end_tuning(host);
> +
> +	mci_send_abort_tuning(host->mci->mci, opcode);
> +}
> +
> +static int __sdhci_execute_tuning(struct sdhci *host, u32 opcode)
> +{
> +	int i;
> +	int ret;
> +
> +	/*
> +	 * Issue opcode repeatedly till Execute Tuning is set to 0 or the number
> +	 * of loops reaches tuning loop count.

Please add to comment: Some controllers are known to always require 40 iterations.

> +	 */
> +	for (i = 0; i < host->tuning_loop_count; i++) {
> +		u16 ctrl;
> +
> +		ret = sdhci_send_tuning(host, opcode);
> +		if (ret) {
> +			sdhci_abort_tuning(host, opcode);
> +			return -ETIMEDOUT;
> +		}
> +
> +		/* Spec does not require a delay between tuning cycles */
> +		if (host->tuning_delay > 0)
> +			mdelay(host->tuning_delay);
> +
> +		ctrl = sdhci_read16(host, SDHCI_HOST_CONTROL2);
> +		if (!(ctrl & SDHCI_CTRL_EXEC_TUNING)) {
> +			if (ctrl & SDHCI_CTRL_TUNED_CLK) {
> +				return 0; /* Success! */
> +			}
> +			break;
> +		}
> +
> +	}
> +
> +	dev_dbg(&host->mci->mci->dev, "Tuning timeout, falling back to fixed sampling clock\n");
> +	sdhci_reset_tuning(host);
> +	return -EAGAIN;
> +}
> +
> +int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode)
> +{
> +	struct mci_host *host = sdhci->mci;
> +	int err = 0;
> +	unsigned int tuning_count = 0;
> +
> +	if (sdhci->tuning_mode == SDHCI_TUNING_MODE_1)
> +		tuning_count = sdhci->tuning_count;
> +
> +	/*
> +	 * The Host Controller needs tuning in case of SDR104 and DDR50
> +	 * mode, and for SDR50 mode when Use Tuning for SDR50 is set in
> +	 * the Capabilities register.
> +	 * If the Host Controller supports the HS200 mode then the
> +	 * tuning function has to be executed.
> +	 */
> +	switch (host->timing) {
> +	/* HS400 tuning is done in HS200 mode */
> +	case MMC_TIMING_MMC_HS400:
> +		err = -EINVAL;
> +		goto out;
> +
> +	case MMC_TIMING_MMC_HS200:
> +		break;
> +
> +	case MMC_TIMING_UHS_SDR104:
> +	case MMC_TIMING_UHS_DDR50:
> +		break;
> +
> +	case MMC_TIMING_UHS_SDR50:
> +		fallthrough;
> +
> +	default:
> +		goto out;
> +	}
> +
> +	if (host->platform_execute_tuning) {
> +		err = host->platform_execute_tuning(host, opcode);
> +		goto out;
> +	}

As mentioned in previous patch, this should be sdhci->platform_execute_tuning.

> +
> +	if (sdhci->tuning_delay < 0)
> +		sdhci->tuning_delay = opcode == MMC_SEND_TUNING_BLOCK;
> +
> +	sdhci_start_tuning(sdhci);
> +
> +	sdhci->tuning_err = __sdhci_execute_tuning(sdhci, opcode);
> +
> +	sdhci_end_tuning(sdhci);
> +out:
> +
> +	return err;
> +}
> +EXPORT_SYMBOL_GPL(sdhci_execute_tuning);
> +#endif
> +
>  void sdhci_read_response(struct sdhci *sdhci, struct mci_cmd *cmd)
>  {
>  	if (cmd->resp_type & MMC_RSP_136) {
> @@ -50,7 +240,12 @@ void sdhci_set_cmd_xfer_mode(struct sdhci *host, struct mci_cmd *cmd,
>  
>  	*command |= SDHCI_CMD_INDEX(cmd->cmdidx);
>  
> -	if (data) {
> +	if (cmd->cmdidx == MMC_SEND_TUNING_BLOCK ||
> +	    cmd->cmdidx == MMC_SEND_TUNING_BLOCK_HS200) {
> +		*command |= SDHCI_DATA_PRESENT;
> +		*xfer = SDHCI_DATA_TO_HOST;
> +
> +	} else if (data) {
>  		*command |= SDHCI_DATA_PRESENT;
>  
>  		*xfer |= SDHCI_BLOCK_COUNT_EN;
> @@ -111,6 +306,31 @@ void sdhci_set_bus_width(struct sdhci *host, int width)
>  	sdhci_write8(host, SDHCI_HOST_CONTROL, ctrl);
>  }
>  
> +static void sdhci_set_uhs_signaling(struct sdhci *host, unsigned timing)
> +{
> +	u16 ctrl_2;
> +
> +	ctrl_2 = sdhci_read16(host, SDHCI_HOST_CONTROL2);
> +	/* Select Bus Speed Mode for host */
> +	ctrl_2 &= ~SDHCI_CTRL_UHS_MASK;
> +	if ((timing == MMC_TIMING_MMC_HS200) ||
> +	    (timing == MMC_TIMING_UHS_SDR104))
> +		ctrl_2 |= SDHCI_CTRL_UHS_SDR104;
> +	else if (timing == MMC_TIMING_UHS_SDR12)
> +		ctrl_2 |= SDHCI_CTRL_UHS_SDR12;
> +	else if (timing == MMC_TIMING_UHS_SDR25)
> +		ctrl_2 |= SDHCI_CTRL_UHS_SDR25;
> +	else if (timing == MMC_TIMING_UHS_SDR50)
> +		ctrl_2 |= SDHCI_CTRL_UHS_SDR50;
> +	else if ((timing == MMC_TIMING_UHS_DDR50) ||
> +		 (timing == MMC_TIMING_MMC_DDR52))
> +		ctrl_2 |= SDHCI_CTRL_UHS_DDR50;
> +	else if (timing == MMC_TIMING_MMC_HS400)
> +		ctrl_2 |= SDHCI_CTRL_HS400; /* Non-standard */
> +	sdhci_write16(host, SDHCI_HOST_CONTROL2, ctrl_2);
> +}
> +EXPORT_SYMBOL_GPL(sdhci_set_uhs_signaling);
> +
>  static inline bool sdhci_can_64bit_dma(struct sdhci *host)
>  {
>  	/*
> @@ -567,6 +787,8 @@ void sdhci_set_clock(struct sdhci *host, unsigned int clock, unsigned int input_
>  
>  	host->mci->clock = 0;
>  
> +	sdhci_set_uhs_signaling(host, host->mci->timing);
> +
>  	sdhci_wait_idle(host, NULL);
>  
>  	sdhci_write16(host, SDHCI_CLOCK_CONTROL, 0);
> @@ -695,5 +917,25 @@ int sdhci_setup_host(struct sdhci *host)
>  	if (sdhci_can_64bit_dma(host))
>  		host->flags |= SDHCI_USE_64_BIT_DMA;
>  
> +	if ((mci->caps2 & (MMC_CAP2_HS200_1_8V_SDR | MMC_CAP2_HS400_1_8V)))
> +		host->flags |= SDHCI_SIGNALING_180;
> +
> +	host->tuning_delay = -1;
> +	host->tuning_loop_count = MAX_TUNING_LOOP;
> +
> +	/* Initial value for re-tuning timer count */
> +	host->tuning_count = FIELD_GET(SDHCI_RETUNING_TIMER_COUNT_MASK,
> +				       host->caps1);
> +
> +	/*
> +	 * In case Re-tuning Timer is not disabled, the actual value of
> +	 * re-tuning timer will be 2 ^ (n - 1).
> +	 */
> +	if (host->tuning_count)
> +		host->tuning_count = 1 << (host->tuning_count - 1);
> +
> +	/* Re-tuning mode supported by the Host Controller */
> +	host->tuning_mode = FIELD_GET(SDHCI_RETUNING_MODE_MASK, host->caps1);
> +
>  	return 0;
>  }
> diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
> index 2139498855..80916e670f 100644
> --- a/drivers/mci/sdhci.h
> +++ b/drivers/mci/sdhci.h
> @@ -28,9 +28,11 @@
>  #define SDHCI_TRANSFER_MODE					0x0c
>  #define  SDHCI_MULTIPLE_BLOCKS			BIT(5)
>  #define  SDHCI_DATA_TO_HOST			BIT(4)
> +#define  SDHCI_TRNS_AUTO_CMD12			BIT(3)
>  #define  SDHCI_BLOCK_COUNT_EN			BIT(1)
>  #define  SDHCI_DMA_EN				BIT(0)
>  #define SDHCI_COMMAND						0x0e
> +#define  SDHCI_MAKE_CMD(c, f) 			(((c & 0xff) << 8) | (f & 0xff))
>  #define  SDHCI_CMD_INDEX(c)			(((c) & 0x3f) << 8)
>  #define  SDHCI_COMMAND_CMDTYP_SUSPEND		(1 << 6)
>  #define  SDHCI_COMMAND_CMDTYP_RESUME		(2 << 6)
> @@ -121,6 +123,15 @@
>  #define SDHCI_SIGNAL_ENABLE					0x38
>  #define SDHCI_ACMD12_ERR__HOST_CONTROL2				0x3C
>  #define SDHCI_HOST_CONTROL2					0x3E
> +#define  SDHCI_CTRL_UHS_MASK			GENMASK(3, 0)
> +#define   SDHCI_CTRL_UHS_SDR12			0x0
> +#define   SDHCI_CTRL_UHS_SDR25			0x1
> +#define   SDHCI_CTRL_UHS_SDR50			0x2
> +#define   SDHCI_CTRL_UHS_SDR104			0x3
> +#define   SDHCI_CTRL_UHS_DDR50			0x4
> +#define   SDHCI_CTRL_HS400			0x5 /* Non-standard */
> +#define  SDHCI_CTRL_EXEC_TUNING			BIT(6)
> +#define  SDHCI_CTRL_TUNED_CLK			BIT(7)
>  #define  SDHCI_CTRL_64BIT_ADDR			BIT(13)
>  #define  SDHCI_CTRL_V4_MODE			BIT(12)
>  #define SDHCI_CAPABILITIES					0x40
> @@ -230,6 +241,18 @@ struct sdhci {
>  	bool read_caps;	/* Capability flags have been read */
>  	u32 sdma_boundary;
>  
> +	unsigned int		tuning_count;	/* Timer count for re-tuning */
> +	unsigned int		tuning_mode;	/* Re-tuning mode supported by host */
> +	unsigned int		tuning_err;	/* Error code for re-tuning */
> +#define SDHCI_TUNING_MODE_1	0
> +#define SDHCI_TUNING_MODE_2	1
> +#define SDHCI_TUNING_MODE_3	2
> +	/* Delay (ms) between tuning commands */
> +	int			tuning_delay;
> +	int			tuning_loop_count;
> +	int			tuning_old_ier;
> +	int			tuning_old_sig;
> +
>  	struct mci_host	*mci;
>  };
>  
> @@ -282,6 +305,7 @@ static inline void sdhci_write8(struct sdhci *host, int reg, u32 val)
>  }
>  
>  #define SDHCI_NO_DMA DMA_ERROR_CODE
> +int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode);
>  int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd);
>  int sdhci_wait_for_done(struct sdhci *host, u32 mask);
>  void sdhci_read_response(struct sdhci *host, struct mci_cmd *cmd);
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 11/12] mci: arasan-sdhci: add HS200 tuning support on ZynqMP

[Steffen Trumtrar]

The ZynpMP 8.9a has an Arasan IP core that supports HS200 tuning.
Register the callback with the mci-core.

Arasan uses the SDHCI tuning method.

Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/arasan-sdhci.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index 77ea1df7e7..c20d901c3f 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -128,6 +128,46 @@ static int arasan_sdhci_reset(struct arasan_sdhci_host *host, u8 mask)
 	return 0;
 }
 
+static void arasan_zynqmp_dll_reset(struct arasan_sdhci_host *host, u32 deviceid)
+{
+	u16 clk;
+
+	clk = sdhci_read16(&host->sdhci, SDHCI_CLOCK_CONTROL);
+	clk &= ~(SDHCI_CLOCK_CARD_EN | SDHCI_CLOCK_INT_EN);
+	sdhci_write16(&host->sdhci, SDHCI_CLOCK_CONTROL, clk);
+
+	/* Issue DLL Reset */
+	zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_PULSE);
+
+	clk = sdhci_read16(&host->sdhci, SDHCI_CLOCK_CONTROL);
+
+	sdhci_enable_clk(&host->sdhci, clk);
+}
+
+static int arasan_zynqmp_execute_tuning(struct mci_host *mci, u32 opcode)
+{
+	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
+	struct clk_hw *hw = &host->clk_data.sdcardclk_hw;
+	const char *clk_name = clk_hw_get_name(hw);
+	u32 device_id = !strcmp(clk_name, "clk_out_sd0") ? NODE_SD_0 :
+							   NODE_SD_1;
+	int err;
+
+	/* ZynqMP SD controller does not perform auto tuning in DDR50 mode */
+	if (mci->timing == MMC_TIMING_UHS_DDR50)
+		return 0;
+
+	arasan_zynqmp_dll_reset(host, device_id);
+
+	err = sdhci_execute_tuning(&host->sdhci, opcode);
+	if (err)
+		return err;
+
+	arasan_zynqmp_dll_reset(host, device_id);
+
+	return 0;
+}
+
 static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
 {
 	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
@@ -226,8 +266,9 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
 				cmd, data, false, &command, &xfer);
 
 	sdhci_write8(&host->sdhci, SDHCI_TIMEOUT_CONTROL, TIMEOUT_VAL);
-	if (data) {
+	if (xfer)
 		sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
+	if (data) {
 		sdhci_write16(&host->sdhci, SDHCI_BLOCK_SIZE,
 			      SDHCI_DMA_BOUNDARY_512K | SDHCI_TRANSFER_BLOCK_SIZE(data->blocksize));
 		sdhci_write16(&host->sdhci, SDHCI_BLOCK_COUNT, data->blocks);
@@ -235,6 +276,10 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
 	sdhci_write32(&host->sdhci, SDHCI_ARGUMENT, cmd->cmdarg);
 	sdhci_write16(&host->sdhci, SDHCI_COMMAND, command);
 
+	/* CMD19/21 generate _only_ Buffer Read Ready interrupt */
+	if (cmd->cmdidx == MMC_SEND_TUNING_BLOCK || cmd->cmdidx == MMC_SEND_TUNING_BLOCK_HS200)
+		mask = SDHCI_INT_DATA_AVAIL;
+
 	ret = sdhci_wait_for_done(&host->sdhci, mask);
 	if (ret)
 		goto error;
@@ -688,8 +733,11 @@ static int arasan_sdhci_probe(struct device *dev)
 	if (of_property_read_bool(np, "no-1-8-v"))
 		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_NO_1_8_V;
 
-	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
+	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a")) {
+		if (IS_ENABLED(CONFIG_MCI_TUNING))
+			mci->execute_tuning = arasan_zynqmp_execute_tuning;
 		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN;
+	}
 
 	arasan_sdhci->sdhci.base = IOMEM(iores->start);
 	arasan_sdhci->sdhci.mci = mci;
@@ -700,7 +748,15 @@ static int arasan_sdhci_probe(struct device *dev)
 	mci->card_write_protected = arasan_sdhci_card_write_protected;
 	mci->hw_dev = dev;
 
-	mci->f_max = clk_get_rate(clk_xin);
+	/*
+	 * clk_rates on ZynqMP are rounded wrong. For HS200 clk_get_rate retunrs
+	 * 199999998 instead of 200000000
+	 */
+	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
+		mci->f_max = 200000000;
+	else
+		mci->f_max = clk_get_rate(clk_xin);
+
 	mci->f_min = 50000000 / 256;
 
 	arasan_sdhci_register_sdclk(&arasan_sdhci->clk_data, clk_xin, dev);

-- 
2.40.1

Re: [PATCH 11/12] mci: arasan-sdhci: add HS200 tuning support on ZynqMP

[Ahmad Fatoum]

Hello Steffen,

On 08.03.24 12:17, Steffen Trumtrar wrote:
> The ZynpMP 8.9a has an Arasan IP core that supports HS200 tuning.
> Register the callback with the mci-core.
> 
> Arasan uses the SDHCI tuning method.
> 
> Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  drivers/mci/arasan-sdhci.c | 62 +++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
> index 77ea1df7e7..c20d901c3f 100644
> --- a/drivers/mci/arasan-sdhci.c
> +++ b/drivers/mci/arasan-sdhci.c
> @@ -128,6 +128,46 @@ static int arasan_sdhci_reset(struct arasan_sdhci_host *host, u8 mask)
>  	return 0;
>  }
>  
> +static void arasan_zynqmp_dll_reset(struct arasan_sdhci_host *host, u32 deviceid)
> +{
> +	u16 clk;
> +
> +	clk = sdhci_read16(&host->sdhci, SDHCI_CLOCK_CONTROL);
> +	clk &= ~(SDHCI_CLOCK_CARD_EN | SDHCI_CLOCK_INT_EN);
> +	sdhci_write16(&host->sdhci, SDHCI_CLOCK_CONTROL, clk);
> +
> +	/* Issue DLL Reset */
> +	zynqmp_pm_sd_dll_reset(deviceid, PM_DLL_RESET_PULSE);
> +
> +	clk = sdhci_read16(&host->sdhci, SDHCI_CLOCK_CONTROL);
> +
> +	sdhci_enable_clk(&host->sdhci, clk);
> +}
> +
> +static int arasan_zynqmp_execute_tuning(struct mci_host *mci, u32 opcode)
> +{
> +	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
> +	struct clk_hw *hw = &host->clk_data.sdcardclk_hw;
> +	const char *clk_name = clk_hw_get_name(hw);
> +	u32 device_id = !strcmp(clk_name, "clk_out_sd0") ? NODE_SD_0 :
> +							   NODE_SD_1;
> +	int err;
> +
> +	/* ZynqMP SD controller does not perform auto tuning in DDR50 mode */
> +	if (mci->timing == MMC_TIMING_UHS_DDR50)
> +		return 0;
> +
> +	arasan_zynqmp_dll_reset(host, device_id);
> +
> +	err = sdhci_execute_tuning(&host->sdhci, opcode);
> +	if (err)
> +		return err;
> +
> +	arasan_zynqmp_dll_reset(host, device_id);
> +
> +	return 0;
> +}
> +
>  static int arasan_sdhci_init(struct mci_host *mci, struct device *dev)
>  {
>  	struct arasan_sdhci_host *host = to_arasan_sdhci_host(mci);
> @@ -226,8 +266,9 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
>  				cmd, data, false, &command, &xfer);
>  
>  	sdhci_write8(&host->sdhci, SDHCI_TIMEOUT_CONTROL, TIMEOUT_VAL);
> -	if (data) {
> +	if (xfer)
>  		sdhci_write16(&host->sdhci, SDHCI_TRANSFER_MODE, xfer);
> +	if (data) {
>  		sdhci_write16(&host->sdhci, SDHCI_BLOCK_SIZE,
>  			      SDHCI_DMA_BOUNDARY_512K | SDHCI_TRANSFER_BLOCK_SIZE(data->blocksize));
>  		sdhci_write16(&host->sdhci, SDHCI_BLOCK_COUNT, data->blocks);
> @@ -235,6 +276,10 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
>  	sdhci_write32(&host->sdhci, SDHCI_ARGUMENT, cmd->cmdarg);
>  	sdhci_write16(&host->sdhci, SDHCI_COMMAND, command);
>  
> +	/* CMD19/21 generate _only_ Buffer Read Ready interrupt */
> +	if (cmd->cmdidx == MMC_SEND_TUNING_BLOCK || cmd->cmdidx == MMC_SEND_TUNING_BLOCK_HS200)
> +		mask = SDHCI_INT_DATA_AVAIL;
> +
>  	ret = sdhci_wait_for_done(&host->sdhci, mask);
>  	if (ret)
>  		goto error;
> @@ -688,8 +733,11 @@ static int arasan_sdhci_probe(struct device *dev)
>  	if (of_property_read_bool(np, "no-1-8-v"))
>  		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_NO_1_8_V;
>  
> -	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
> +	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a")) {
> +		if (IS_ENABLED(CONFIG_MCI_TUNING))
> +			mci->execute_tuning = arasan_zynqmp_execute_tuning;
>  		arasan_sdhci->quirks |= SDHCI_ARASAN_QUIRK_CLOCK_25_BROKEN;
> +	}
>  
>  	arasan_sdhci->sdhci.base = IOMEM(iores->start);
>  	arasan_sdhci->sdhci.mci = mci;
> @@ -700,7 +748,15 @@ static int arasan_sdhci_probe(struct device *dev)
>  	mci->card_write_protected = arasan_sdhci_card_write_protected;
>  	mci->hw_dev = dev;
>  
> -	mci->f_max = clk_get_rate(clk_xin);
> +	/*
> +	 * clk_rates on ZynqMP are rounded wrong. For HS200 clk_get_rate retunrs
> +	 * 199999998 instead of 200000000
> +	 */
> +	if (of_device_is_compatible(np, "xlnx,zynqmp-8.9a"))
> +		mci->f_max = 200000000;
> +	else
> +		mci->f_max = clk_get_rate(clk_xin);
> +
>  	mci->f_min = 50000000 / 256;
>  
>  	arasan_sdhci_register_sdclk(&arasan_sdhci->clk_data, clk_xin, dev);
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

[PATCH 12/12] mci: arasan: use sdhci_wait_idle2

[Steffen Trumtrar]

To support HS200 mode, the arasan needs a differernt sdhci_wait_idle
function. Remove the default CMD_INHIBIT_DATA, otherwise the sdhci hs200
tuning will timeout.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
Signed-off-by: Steffen Trumtrar <s.trumtrar@pengutronix.de>
---
 drivers/mci/arasan-sdhci.c |  2 +-
 drivers/mci/sdhci.c        | 25 +++++++++++++++++++++++++
 drivers/mci/sdhci.h        |  1 +
 3 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/drivers/mci/arasan-sdhci.c b/drivers/mci/arasan-sdhci.c
index c20d901c3f..afddf19de7 100644
--- a/drivers/mci/arasan-sdhci.c
+++ b/drivers/mci/arasan-sdhci.c
@@ -250,7 +250,7 @@ static int arasan_sdhci_send_cmd(struct mci_host *mci, struct mci_cmd *cmd,
 	u32 mask, command, xfer;
 	int ret;
 
-	ret = sdhci_wait_idle(&host->sdhci, cmd);
+	ret = sdhci_wait_idle2(&host->sdhci, cmd, data);
 	if (ret)
 		return ret;
 
diff --git a/drivers/mci/sdhci.c b/drivers/mci/sdhci.c
index 62e6443e64..0f7e3cc193 100644
--- a/drivers/mci/sdhci.c
+++ b/drivers/mci/sdhci.c
@@ -757,6 +757,31 @@ void sdhci_enable_clk(struct sdhci *host, u16 clk)
 	sdhci_write16(host, SDHCI_CLOCK_CONTROL, clk);
 }
 
+int sdhci_wait_idle2(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data)
+{
+	u32 mask;
+	int ret;
+
+	mask = SDHCI_CMD_INHIBIT_CMD;
+
+	if (data || (cmd->resp_type & MMC_RSP_BUSY))
+		mask |= SDHCI_CMD_INHIBIT_DATA;
+
+	if (cmd && cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
+		mask &= ~SDHCI_CMD_INHIBIT_DATA;
+
+	ret = wait_on_timeout(10 * MSECOND,
+			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
+
+	if (ret) {
+		dev_err(host->mci->hw_dev,
+				"SDHCI timeout while waiting for idle\n");
+		return -EBUSY;
+	}
+
+	return 0;
+}
+
 int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd)
 {
 	u32 mask;
diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
index 80916e670f..80c3f98c9c 100644
--- a/drivers/mci/sdhci.h
+++ b/drivers/mci/sdhci.h
@@ -307,6 +307,7 @@ static inline void sdhci_write8(struct sdhci *host, int reg, u32 val)
 #define SDHCI_NO_DMA DMA_ERROR_CODE
 int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode);
 int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd);
+int sdhci_wait_idle2(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data);
 int sdhci_wait_for_done(struct sdhci *host, u32 mask);
 void sdhci_read_response(struct sdhci *host, struct mci_cmd *cmd);
 void sdhci_set_cmd_xfer_mode(struct sdhci *host, struct mci_cmd *cmd,

-- 
2.40.1

Re: [PATCH 12/12] mci: arasan: use sdhci_wait_idle2

[Ahmad Fatoum]

Hello Steffen,

On 08.03.24 12:17, Steffen Trumtrar wrote:
> To support HS200 mode, the arasan needs a differernt sdhci_wait_idle
> function. Remove the default CMD_INHIBIT_DATA, otherwise the sdhci hs200
> tuning will timeout.

AFAICS, this new sdhci_wait_idle2 is what Linux does by default. Given
that we will want to replace existing sdhci_wait_idle with the new
version, I think you should rename the old one to sdhci_wait_idle_data()
or similar and add sdhci_wait_idle2 without the 2.

> +	mask = SDHCI_CMD_INHIBIT_CMD;
> +
> +	if (data || (cmd->resp_type & MMC_RSP_BUSY))
> +		mask |= SDHCI_CMD_INHIBIT_DATA;

Here cmd is de-referenced unconditionally.

> +
> +	if (cmd && cmd->cmdidx == MMC_CMD_STOP_TRANSMISSION)
> +		mask &= ~SDHCI_CMD_INHIBIT_DATA;

Here, it's checked after the fact. Can you add a cmd && check in the hunk
before too?

Cheers,
Ahmad

> +
> +	ret = wait_on_timeout(10 * MSECOND,
> +			!(sdhci_read32(host, SDHCI_PRESENT_STATE) & mask));
> +
> +	if (ret) {
> +		dev_err(host->mci->hw_dev,
> +				"SDHCI timeout while waiting for idle\n");
> +		return -EBUSY;
> +	}
> +
> +	return 0;
> +}
> +
>  int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd)
>  {
>  	u32 mask;
> diff --git a/drivers/mci/sdhci.h b/drivers/mci/sdhci.h
> index 80916e670f..80c3f98c9c 100644
> --- a/drivers/mci/sdhci.h
> +++ b/drivers/mci/sdhci.h
> @@ -307,6 +307,7 @@ static inline void sdhci_write8(struct sdhci *host, int reg, u32 val)
>  #define SDHCI_NO_DMA DMA_ERROR_CODE
>  int sdhci_execute_tuning(struct sdhci *sdhci, u32 opcode);
>  int sdhci_wait_idle(struct sdhci *host, struct mci_cmd *cmd);
> +int sdhci_wait_idle2(struct sdhci *host, struct mci_cmd *cmd, struct mci_data *data);
>  int sdhci_wait_for_done(struct sdhci *host, u32 mask);
>  void sdhci_read_response(struct sdhci *host, struct mci_cmd *cmd);
>  void sdhci_set_cmd_xfer_mode(struct sdhci *host, struct mci_cmd *cmd,
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |