[PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Drew Fustini]

This series adds initial support for the eMMC on the BeagleV Ahead
board. This allows the kernel to boot with the root fs on eMMC.

I tested [1] on top of v6.5-rc3 with this config [2] along with the
prerequisite series [3] that adds the BeagleV Ahead dts file.

I am submitting this as an RFC for other people that want to test
booting from the eMMC with mainline. There are several issues that need
to be resolved before this code can progress beyond an RFC:

  - Only the MMC controller connected to the eMMC is enabled. I did
    not yet attempt to configure or use the microSD card slot.

  - The thead,th1520-dwcmshc compatible sets quirks which disable DMA
    and forces the use of inefficient PIO mode. I need to determine the
    correct configuration for the SDMA and ADMA modes.

  - th1520-specific code is needed in dwcmshc_set_uhs_signaling() for
    MMC_TIMING_MMC_HS400. I have not figured out add proper way to make
    that code conditional so that it only runs on th1520. One method
    could be to add a th1520 flag to dwcmshc_priv but that seems like a
    hack. Alternatively, set_uhs_signaling in sdhci_dwcmshc_th1520_ops
    could point to a th1520-specific function, but that new function
    would have to duplicate all the code in the current
    dwcmshc_set_uhs_signaling().

NOTE: I combined schema, dts and driver patches into this one series to
simplify review and testing of this RFC.

References:
[1] https://gist.github.com/pdp7/09995be1e30df0a04b9b9cd31420f9d5
[2] https://gist.github.com/pdp7/e4585311eb2cd27df7b50c87babc15fd
[3] https://lore.kernel.org/linux-riscv/20230722-upstream-beaglev-ahead-dts-v2-0-a470ab8fe806@baylibre.com/

Changes in v2:
- Expand dwcmshc_priv based on driver in the T-Head 5.10 kernel:
  delay_line, non_removable, pull_up_en, io_fixed_1v8
- New boolean property "thead,pull-up" indicates phy pull-up config
- New boolean property "thead,io-fixed-1v8" indicates that io voltage
  should be set to 1.8V during reset
- Add th1520_phy_1_8v_init() as voltage_switch op
- Add th1520_execute_tuning() as the platform_execute_tuning op
- Added th1520_sdhci_reset() as the .reset op. This function will set
  io voltage to 1.8V after calling the standard sdhci_reset() function.
- Modified dwcmshc_set_uhs_signaling() to enable SDHCI_CTRL_VDD_180 when
  io_fixed_1v8 is true
- Add many defines for register offsets and settings based on the mmc
  support in the T-Head downstream v5.10 kernel

v1 series:
https://lore.kernel.org/r/20230724-th1520-emmc-v1-0-cca1b2533da2@baylibre.com

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
Drew Fustini (4):
      dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support
      riscv: dts: thead: Add TH1520 mmc controller and sdhci clock
      riscv: dts: thead: Enable BeagleV Ahead eMMC controller
      mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520

 .../bindings/mmc/snps,dwcmshc-sdhci.yaml           |   9 +
 arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts |  17 ++
 arch/riscv/boot/dts/thead/th1520.dtsi              |  17 ++
 drivers/mmc/host/sdhci-of-dwcmshc.c                | 336 +++++++++++++++++++++
 4 files changed, 379 insertions(+)
---
base-commit: cb8c874afdc063290797ae1776a5d410fecb06cb
change-id: 20230724-th1520-emmc-73cde98805d6

Best regards,
-- 
Drew Fustini <dfustini@baylibre.com>

[PATCH RFC v2 1/4] dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support

[Drew Fustini]

Add compatible value for the T-Head TH1520 dwcmshc controller and
thead,io-fixed-1v8 and thead,pull-up properties.

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
index a43eb837f8da..57602c345cab 100644
--- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
+++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
@@ -19,6 +19,7 @@ properties:
       - rockchip,rk3568-dwcmshc
       - rockchip,rk3588-dwcmshc
       - snps,dwcmshc-sdhci
+      - thead,th1520-dwcmshc
 
   reg:
     maxItems: 1
@@ -60,6 +61,14 @@ properties:
     description: Specify the number of delay for tx sampling.
     $ref: /schemas/types.yaml#/definitions/uint8
 
+  thead,io-fixed-1v8:
+    description: SoC PHY pad is fixed 1.8V
+    type: boolean
+
+  thead,pull-up:
+    description: True if pull-up, false if pull-down
+    type: boolean
+
 
 required:
   - compatible

-- 
2.34.1

[PATCH RFC v2 2/4] riscv: dts: thead: Add TH1520 mmc controller and sdhci clock

[Drew Fustini]

Add nodes for the SDHCI fixed clock and the first mmc controller which
is typically connected to the eMMC device.

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 arch/riscv/boot/dts/thead/th1520.dtsi | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520.dtsi b/arch/riscv/boot/dts/thead/th1520.dtsi
index 56a73134b49e..b33bfb04c955 100644
--- a/arch/riscv/boot/dts/thead/th1520.dtsi
+++ b/arch/riscv/boot/dts/thead/th1520.dtsi
@@ -134,6 +134,13 @@ uart_sclk: uart-sclk-clock {
 		#clock-cells = <0>;
 	};
 
+	sdhci_clk: sdhci-clock {
+		compatible = "fixed-clock";
+		clock-frequency = <198000000>;
+		clock-output-names = "sdhci_clk";
+		#clock-cells = <0>;
+	};
+
 	soc {
 		compatible = "simple-bus";
 		interrupt-parent = <&plic>;
@@ -291,6 +298,16 @@ dmac0: dma-controller@ffefc00000 {
 			status = "disabled";
 		};
 
+		mmc0: mmc@ffe7080000 {
+			compatible = "thead,th1520-dwcmshc";
+			reg = <0xff 0xe7080000 0x0 0x10000
+			       0xff 0xef014060 0x0 0x4>;
+			interrupts = <62 IRQ_TYPE_LEVEL_HIGH>;
+			interrupt-names = "sdhciirq";
+			clocks = <&sdhci_clk>;
+			clock-names = "core";
+		};
+
 		timer0: timer@ffefc32000 {
 			compatible = "snps,dw-apb-timer";
 			reg = <0xff 0xefc32000 0x0 0x14>;

-- 
2.34.1

[PATCH RFC v2 3/4] riscv: dts: thead: Enable BeagleV Ahead eMMC controller

[Drew Fustini]

Add properties to the emmc node and enable it and set the frequency for
the sdhci clock.

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
index c315e5bd3d2d..f93c11754639 100644
--- a/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
+++ b/arch/riscv/boot/dts/thead/th1520-beaglev-ahead.dts
@@ -52,6 +52,10 @@ &uart_sclk {
 	clock-frequency = <100000000>;
 };
 
+&sdhci_clk {
+	clock-frequency = <198000000>;
+};
+
 &dmac0 {
 	status = "okay";
 };
@@ -59,3 +63,16 @@ &dmac0 {
 &uart0 {
 	status = "okay";
 };
+
+&mmc0 {
+	max-frequency = <198000000>;
+	non-removable;
+	mmc-hs400-1_8v;
+	thead,io-fixed-1v8;
+	no-sdio;
+	no-sd;
+	thead,pull-up;
+	bus-width = <8>;
+	status = "okay";
+
+};

-- 
2.34.1

[PATCH RFC v2 4/4] mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520

[Drew Fustini]

Add basic support for the T-Head TH1520 SoC mmc controller with the new
compatible "thead,th1520-dwcmshc".

However, quirks are currently set to disable DMA and use PIO. The proper
settings for DMA support still need to be determined.

Another issue is th1520-specific code for MMC_TIMING_MMC_HS400 in
dwcmshc_set_uhs_signaling() will run on all platforms which is not
correct. One solution could be to add a th1520 flag to dwcmshc_priv but
that is hacky. Another solution could be to set the set_uhs_signaling op
in sdhci_dwcmshc_th1520_ops to a th1520-specific function. However, that
new function would have to duplicate all the code in the current
dwcmshc_set_uhs_signaling().

Signed-off-by: Drew Fustini <dfustini@baylibre.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 336 ++++++++++++++++++++++++++++++++++++
 1 file changed, 336 insertions(+)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e68cd87998c8..d35e204cdb16 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -71,6 +71,63 @@
 	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
 #define RK35xx_MAX_CLKS 3
 
+/* T-Head specific registers */
+#define DWC_MSHC_PTR_PHY_R	0x300
+#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
+#define PHY_RSTN		0x0
+#define PAD_SP			0x10
+#define PAD_SN			0x14
+
+#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
+#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
+#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
+#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
+#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
+#define RXSEL			0x0
+#define WEAKPULL_EN		0x3
+#define TXSLEW_CTRL_P		0x5
+#define TXSLEW_CTRL_N		0x9
+
+#define PHY_PADTEST_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0e)
+#define PHY_PADTEST_OUT_R	(DWC_MSHC_PTR_PHY_R + 0x10)
+#define PHY_PADTEST_IN_R	(DWC_MSHC_PTR_PHY_R + 0x12)
+#define PHY_PRBS_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x18)
+#define PHY_PHYLBK_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1a)
+#define PHY_COMMDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1c)
+
+#define PHY_SDCLKDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1d)
+#define UPDATE_DC		0x4
+
+#define PHY_SDCLKDL_DC_R	(DWC_MSHC_PTR_PHY_R + 0x1e)
+#define PHY_SMPLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x20)
+#define PHY_ATDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x21)
+#define INPSEL_CNFG		2
+
+#define PHY_DLL_CTRL_R		(DWC_MSHC_PTR_PHY_R + 0x24)
+#define DLL_EN			0x0
+
+#define PHY_DLL_CNFG1_R		(DWC_MSHC_PTR_PHY_R + 0x25)
+#define PHY_DLL_CNFG2_R		(DWC_MSHC_PTR_PHY_R + 0x26)
+#define PHY_DLLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x28)
+#define SLV_INPSEL		0x5
+
+#define P_VENDOR_SPECIFIC_AREA	0x500
+#define EMMC_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x2c)
+#define AT_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x40)
+#define AT_EN			0x0
+#define CI_SEL			0x1
+#define SWIN_TH_EN		0x2
+#define RPT_TUNE_ERR		0x3
+#define SW_TUNE_EN		0x4
+#define WIN_EDGE_SEL		0x8
+#define TUNE_CLK_STOP_EN	0x10
+#define PRE_CHANGE_DLY		0x11
+#define POST_CHANGE_DLY		0x13
+#define SWIN_TH_VAL		0x18
+
+#define DELAY_LINE_HS400	24
+#define DELAY_LINE_DEFAULT	50
+
 #define BOUNDARY_OK(addr, len) \
 	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
 
@@ -91,6 +148,10 @@ struct dwcmshc_priv {
 	struct clk	*bus_clk;
 	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
 	void *priv; /* pointer to SoC private stuff */
+	uint32_t delay_line;
+	bool non_removable;
+	bool pull_up_en;
+	bool io_fixed_1v8;
 };
 
 /*
@@ -156,11 +217,171 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
 	sdhci_request(mmc, mrq);
 }
 
+static void sdhci_phy_1_8v_init_no_pull(struct sdhci_host *host)
+{
+	uint32_t val;
+	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
+	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
+	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
+
+	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
+	val &= ~(1 << 4);
+	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
+
+
+	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
+	sdhci_writew(host, val | 1, PHY_CMDPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
+	sdhci_writew(host, val | 1, PHY_DATAPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
+	sdhci_writew(host, val | 1, PHY_RSTNPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
+	sdhci_writew(host, val | 1, PHY_STBPAD_CNFG_R);
+
+	val = sdhci_readb(host, PHY_DLL_CTRL_R);
+	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
+}
+
+static void sdhci_phy_3_3v_init_no_pull(struct sdhci_host *host)
+{
+	uint32_t val;
+
+	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
+	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
+	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
+
+	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
+	val &= ~(1 << 4);
+	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
+
+	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
+	sdhci_writew(host, val | 2, PHY_CMDPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
+	sdhci_writew(host, val | 2, PHY_DATAPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
+	sdhci_writew(host, val | 2, PHY_RSTNPAD_CNFG_R);
+
+	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
+	sdhci_writew(host, val | 2, PHY_STBPAD_CNFG_R);
+
+	val = sdhci_readb(host, PHY_DLL_CTRL_R);
+	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
+}
+
+static void th1520_phy_1_8v_init(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	u32 val;
+
+	if (!priv)
+		return;
+
+	if (priv->pull_up_en == 0) {
+		sdhci_phy_1_8v_init_no_pull(host);
+		return;
+	}
+
+	/* set driving force */
+	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
+
+	/* disable delay lane */
+	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
+
+	/* set delay lane */
+	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
+	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
+
+	/* enable delay lane */
+	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
+	val &= ~(1 << UPDATE_DC);
+	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
+
+	val = (1 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
+	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
+	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
+
+	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
+
+	val = (1 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
+
+	/* enable data strobe mode */
+	sdhci_writeb(host, 3 << SLV_INPSEL, PHY_DLLDL_CNFG_R);
+	sdhci_writeb(host, (1 << DLL_EN),  PHY_DLL_CTRL_R);
+}
+
+static void th1520_phy_3_3v_init(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	u32 val;
+
+	if (priv->pull_up_en == 0) {
+		sdhci_phy_3_3v_init_no_pull(host);
+		return;
+	}
+
+	/* set driving force */
+	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
+
+	/* disable delay lane */
+	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
+
+	/* set delay lane */
+	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
+	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
+
+	/* enable delay lane */
+	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
+	val &= ~(1 << UPDATE_DC);
+	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
+
+	val = (2 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
+	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
+	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
+
+	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
+
+	val = (2 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
+	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
+}
+
+
+static void th1520_sdhci_set_phy(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	u8 emmc_ctl;
+
+	/* Before power on, set PHY configs */
+	emmc_ctl = sdhci_readw(host, EMMC_CTRL_R);
+	if (priv->non_removable) {
+		th1520_phy_1_8v_init(host);
+		emmc_ctl |= (1 << DWCMSHC_CARD_IS_EMMC);
+	} else {
+		th1520_phy_3_3v_init(host);
+		emmc_ctl &=~(1 << DWCMSHC_CARD_IS_EMMC);
+	}
+	sdhci_writeb(host, emmc_ctl, EMMC_CTRL_R);
+	sdhci_writeb(host, 0x25, PHY_DLL_CNFG1_R);
+}
+
 static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
 				      unsigned int timing)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+
 	u16 ctrl, ctrl_2;
 
 	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
@@ -188,7 +409,22 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
 		ctrl_2 |= DWCMSHC_CTRL_HS400;
 	}
 
+	if (priv->io_fixed_1v8)
+		ctrl_2 |= SDHCI_CTRL_VDD_180;
+
 	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+
+	/* TODO: add check so that this only runs on th1520  */
+	if (timing == MMC_TIMING_MMC_HS400) {
+		/* disable auto tuning */
+		u32 reg = sdhci_readl(host, AT_CTRL_R);
+		reg &= ~1;
+		sdhci_writel(host, reg, AT_CTRL_R);
+		priv->delay_line = DELAY_LINE_HS400;
+		th1520_sdhci_set_phy(host);
+	} else {
+		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
+	}
 }
 
 static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
@@ -337,6 +573,49 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
+static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
+{
+	u32 val = 0;
+
+	sdhci_writeb(host, 3 << INPSEL_CNFG, PHY_ATDL_CNFG_R);
+
+	val = sdhci_readl(host, AT_CTRL_R);
+	val &= ~((1 << CI_SEL) | (1 << RPT_TUNE_ERR) \
+	    | (1 << SW_TUNE_EN) |(0xf << WIN_EDGE_SEL));
+	val |= (1 << AT_EN) | (1 << SWIN_TH_EN) | (1 << TUNE_CLK_STOP_EN)\
+	    | (1 << PRE_CHANGE_DLY) | (3 << POST_CHANGE_DLY) | (9 << SWIN_TH_VAL);
+
+	sdhci_writel(host, val, AT_CTRL_R);
+
+	val = sdhci_readl(host, AT_CTRL_R);
+	if(!(val & (1 << AT_EN))) {
+		pr_warn("%s(): auto tuning is not enabled", __func__);
+		return -1;
+	}
+
+	val &= ~(1 << AT_EN);
+	sdhci_writel(host, val, AT_CTRL_R);
+
+	return 0;
+}
+
+static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	u16 ctrl_2;
+
+	sdhci_reset(host, mask);
+
+	if(priv->io_fixed_1v8){
+		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+		if(! (ctrl_2 & SDHCI_CTRL_VDD_180)){
+			ctrl_2 |= SDHCI_CTRL_VDD_180;
+			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
+		}
+	}
+}
+
 static const struct sdhci_ops sdhci_dwcmshc_ops = {
 	.set_clock		= sdhci_set_clock,
 	.set_bus_width		= sdhci_set_bus_width,
@@ -355,6 +634,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
 	.adma_write_desc	= dwcmshc_adma_write_desc,
 };
 
+static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
+	.set_clock		= sdhci_set_clock,
+	.set_bus_width		= sdhci_set_bus_width,
+	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
+	.get_max_clock		= dwcmshc_get_max_clock,
+	.reset			= th1520_sdhci_reset,
+	.adma_write_desc	= dwcmshc_adma_write_desc,
+	.voltage_switch		= th1520_phy_1_8v_init,
+	.platform_execute_tuning = &th1520_execute_tuning,
+};
+
 static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
 	.ops = &sdhci_dwcmshc_ops,
 	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
@@ -378,6 +668,15 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
 		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
 };
 
+static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
+	.ops = &sdhci_dwcmshc_th1520_ops,
+
+	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
+		  SDHCI_QUIRK_BROKEN_DMA |
+		  SDHCI_QUIRK_BROKEN_ADMA,
+	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
+};
+
 static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
 {
 	int err;
@@ -434,6 +733,10 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv
 }
 
 static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
+	{
+		.compatible = "thead,th1520-dwcmshc",
+		.data = &sdhci_dwcmshc_th1520_pdata,
+	},
 	{
 		.compatible = "rockchip,rk3588-dwcmshc",
 		.data = &sdhci_dwcmshc_rk35xx_pdata,
@@ -541,6 +844,39 @@ static int dwcmshc_probe(struct platform_device *pdev)
 			goto err_clk;
 	}
 
+	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
+
+		priv->delay_line = DELAY_LINE_DEFAULT;
+
+		if (device_property_present(&pdev->dev, "non-removable"))
+			priv->non_removable = 1;
+		else
+			priv->non_removable = 0;
+
+		if (device_property_present(&pdev->dev, "thead,pull-up"))
+			priv->pull_up_en = 1;
+		else
+			priv->pull_up_en = 0;
+
+		if (device_property_present(&pdev->dev, "thead,io-fixed-1v8"))
+			priv->io_fixed_1v8 = true;
+		else
+			priv->io_fixed_1v8 = false;
+
+		/*
+		 * start_signal_voltage_switch() will try 3.3V first
+		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
+		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
+		 * in sdhci_start_signal_voltage_switch().
+		 */
+		if(priv->io_fixed_1v8){
+			host->flags &=~SDHCI_SIGNALING_330;
+			host->flags |= SDHCI_SIGNALING_180;
+		}
+
+		sdhci_enable_v4_mode(host);
+	}
+
 #ifdef CONFIG_ACPI
 	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
 		sdhci_enable_v4_mode(host);

-- 
2.34.1

Re: [PATCH RFC v2 4/4] mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520

[Jisheng Zhang]

On Fri, Aug 04, 2023 at 08:14:48PM -0700, Drew Fustini wrote:
> Add basic support for the T-Head TH1520 SoC mmc controller with the new
> compatible "thead,th1520-dwcmshc".
> 
> However, quirks are currently set to disable DMA and use PIO. The proper
> settings for DMA support still need to be determined.

Hi Drew,

Thank you for this patch. I think we need to make DMA work even in the
first version.

Several comments below.

> 
> Another issue is th1520-specific code for MMC_TIMING_MMC_HS400 in
> dwcmshc_set_uhs_signaling() will run on all platforms which is not
> correct. One solution could be to add a th1520 flag to dwcmshc_priv but
> that is hacky. Another solution could be to set the set_uhs_signaling op
> in sdhci_dwcmshc_th1520_ops to a th1520-specific function. However, that
> new function would have to duplicate all the code in the current
> dwcmshc_set_uhs_signaling().
> 
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 336 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 336 insertions(+)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e68cd87998c8..d35e204cdb16 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -71,6 +71,63 @@
>  	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
>  #define RK35xx_MAX_CLKS 3
>  
> +/* T-Head specific registers */

I guess this may not be T-HEAD specific but dwc phy, could you please
check?
> +#define DWC_MSHC_PTR_PHY_R	0x300
> +#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
> +#define PHY_RSTN		0x0
> +#define PAD_SP			0x10
> +#define PAD_SN			0x14
> +
> +#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
> +#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
> +#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
> +#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
> +#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
> +#define RXSEL			0x0
> +#define WEAKPULL_EN		0x3
> +#define TXSLEW_CTRL_P		0x5
> +#define TXSLEW_CTRL_N		0x9
> +
> +#define PHY_PADTEST_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0e)
> +#define PHY_PADTEST_OUT_R	(DWC_MSHC_PTR_PHY_R + 0x10)
> +#define PHY_PADTEST_IN_R	(DWC_MSHC_PTR_PHY_R + 0x12)
> +#define PHY_PRBS_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x18)
> +#define PHY_PHYLBK_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1a)
> +#define PHY_COMMDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1c)
> +
> +#define PHY_SDCLKDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1d)
> +#define UPDATE_DC		0x4
> +
> +#define PHY_SDCLKDL_DC_R	(DWC_MSHC_PTR_PHY_R + 0x1e)
> +#define PHY_SMPLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x20)
> +#define PHY_ATDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x21)
> +#define INPSEL_CNFG		2
> +
> +#define PHY_DLL_CTRL_R		(DWC_MSHC_PTR_PHY_R + 0x24)
> +#define DLL_EN			0x0
> +
> +#define PHY_DLL_CNFG1_R		(DWC_MSHC_PTR_PHY_R + 0x25)
> +#define PHY_DLL_CNFG2_R		(DWC_MSHC_PTR_PHY_R + 0x26)
> +#define PHY_DLLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x28)
> +#define SLV_INPSEL		0x5
> +
> +#define P_VENDOR_SPECIFIC_AREA	0x500
> +#define EMMC_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x2c)
> +#define AT_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x40)
> +#define AT_EN			0x0
> +#define CI_SEL			0x1
> +#define SWIN_TH_EN		0x2
> +#define RPT_TUNE_ERR		0x3
> +#define SW_TUNE_EN		0x4
> +#define WIN_EDGE_SEL		0x8
> +#define TUNE_CLK_STOP_EN	0x10
> +#define PRE_CHANGE_DLY		0x11
> +#define POST_CHANGE_DLY		0x13
> +#define SWIN_TH_VAL		0x18
> +
> +#define DELAY_LINE_HS400	24
> +#define DELAY_LINE_DEFAULT	50
> +
>  #define BOUNDARY_OK(addr, len) \
>  	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
>  
> @@ -91,6 +148,10 @@ struct dwcmshc_priv {
>  	struct clk	*bus_clk;
>  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
>  	void *priv; /* pointer to SoC private stuff */
> +	uint32_t delay_line;
> +	bool non_removable;

can be replaced with mmc_host.caps & MMC_CAP_NONREMOVABLE

> +	bool pull_up_en;
> +	bool io_fixed_1v8;

replace them with flag?

>  };
>  
>  /*
> @@ -156,11 +217,171 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
>  	sdhci_request(mmc, mrq);
>  }
>  
> +static void sdhci_phy_1_8v_init_no_pull(struct sdhci_host *host)
> +{
> +	uint32_t val;
> +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);

These magic numbers need a proper macro definitions.

> +
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(1 << 4);

ditto

> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +
> +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val | 1, PHY_CMDPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val | 1, PHY_DATAPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> +	sdhci_writew(host, val | 1, PHY_RSTNPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> +	sdhci_writew(host, val | 1, PHY_STBPAD_CNFG_R);
> +
> +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> +}
> +
> +static void sdhci_phy_3_3v_init_no_pull(struct sdhci_host *host)
> +{
> +	uint32_t val;
> +
> +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
> +
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(1 << 4);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val | 2, PHY_CMDPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val | 2, PHY_DATAPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> +	sdhci_writew(host, val | 2, PHY_RSTNPAD_CNFG_R);
> +
> +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> +	sdhci_writew(host, val | 2, PHY_STBPAD_CNFG_R);
> +
> +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> +}
> +
> +static void th1520_phy_1_8v_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 val;
> +
> +	if (!priv)
> +		return;
> +
> +	if (priv->pull_up_en == 0) {
> +		sdhci_phy_1_8v_init_no_pull(host);
> +		return;
> +	}
> +
> +	/* set driving force */
> +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> +
> +	/* disable delay lane */
> +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay lane */
> +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(1 << UPDATE_DC);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	val = (1 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> +
> +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> +
> +	val = (1 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> +
> +	/* enable data strobe mode */
> +	sdhci_writeb(host, 3 << SLV_INPSEL, PHY_DLLDL_CNFG_R);
> +	sdhci_writeb(host, (1 << DLL_EN),  PHY_DLL_CTRL_R);
> +}
> +
> +static void th1520_phy_3_3v_init(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u32 val;
> +
> +	if (priv->pull_up_en == 0) {
> +		sdhci_phy_3_3v_init_no_pull(host);
> +		return;
> +	}
> +
> +	/* set driving force */
> +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> +
> +	/* disable delay lane */
> +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> +
> +	/* set delay lane */
> +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> +
> +	/* enable delay lane */
> +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> +	val &= ~(1 << UPDATE_DC);
> +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> +
> +	val = (2 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> +
> +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> +
> +	val = (2 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> +}
> +
> +
> +static void th1520_sdhci_set_phy(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u8 emmc_ctl;
> +
> +	/* Before power on, set PHY configs */
> +	emmc_ctl = sdhci_readw(host, EMMC_CTRL_R);
> +	if (priv->non_removable) {
> +		th1520_phy_1_8v_init(host);
> +		emmc_ctl |= (1 << DWCMSHC_CARD_IS_EMMC);
> +	} else {
> +		th1520_phy_3_3v_init(host);
> +		emmc_ctl &=~(1 << DWCMSHC_CARD_IS_EMMC);
> +	}
> +	sdhci_writeb(host, emmc_ctl, EMMC_CTRL_R);
> +	sdhci_writeb(host, 0x25, PHY_DLL_CNFG1_R);

magic 0x25 needs a meaningful MACRO definition.

> +}
> +
>  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  				      unsigned int timing)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +

we don't need this extra blank line

>  	u16 ctrl, ctrl_2;
>  
>  	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> @@ -188,7 +409,22 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  		ctrl_2 |= DWCMSHC_CTRL_HS400;
>  	}
>  
> +	if (priv->io_fixed_1v8)
> +		ctrl_2 |= SDHCI_CTRL_VDD_180;
> +
>  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +
> +	/* TODO: add check so that this only runs on th1520  */

this TODO needs to be solved since it affects other platforms such as NV
and RK

> +	if (timing == MMC_TIMING_MMC_HS400) {
> +		/* disable auto tuning */
> +		u32 reg = sdhci_readl(host, AT_CTRL_R);
> +		reg &= ~1;
> +		sdhci_writel(host, reg, AT_CTRL_R);
> +		priv->delay_line = DELAY_LINE_HS400;
> +		th1520_sdhci_set_phy(host);
> +	} else {
> +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> +	}
>  }
>  
>  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
> @@ -337,6 +573,49 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> +static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
> +{
> +	u32 val = 0;
> +
> +	sdhci_writeb(host, 3 << INPSEL_CNFG, PHY_ATDL_CNFG_R);
> +
> +	val = sdhci_readl(host, AT_CTRL_R);
> +	val &= ~((1 << CI_SEL) | (1 << RPT_TUNE_ERR) \
> +	    | (1 << SW_TUNE_EN) |(0xf << WIN_EDGE_SEL));
> +	val |= (1 << AT_EN) | (1 << SWIN_TH_EN) | (1 << TUNE_CLK_STOP_EN)\
> +	    | (1 << PRE_CHANGE_DLY) | (3 << POST_CHANGE_DLY) | (9 << SWIN_TH_VAL);
> +
> +	sdhci_writel(host, val, AT_CTRL_R);
> +
> +	val = sdhci_readl(host, AT_CTRL_R);
> +	if(!(val & (1 << AT_EN))) {
> +		pr_warn("%s(): auto tuning is not enabled", __func__);

AFAIK, the controller supports both auto tuning and sw tuning.

> +		return -1;

can we replace -1 with meaningful error number?

> +	}
> +
> +	val &= ~(1 << AT_EN);
> +	sdhci_writel(host, val, AT_CTRL_R);
> +
> +	return 0;
> +}
> +
> +static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	u16 ctrl_2;
> +
> +	sdhci_reset(host, mask);
> +
> +	if(priv->io_fixed_1v8){
> +		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> +		if(! (ctrl_2 & SDHCI_CTRL_VDD_180)){
> +			ctrl_2 |= SDHCI_CTRL_VDD_180;
> +			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> +		}
> +	}
> +}
> +
>  static const struct sdhci_ops sdhci_dwcmshc_ops = {
>  	.set_clock		= sdhci_set_clock,
>  	.set_bus_width		= sdhci_set_bus_width,
> @@ -355,6 +634,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
>  	.adma_write_desc	= dwcmshc_adma_write_desc,
>  };
>  
> +static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> +	.set_clock		= sdhci_set_clock,
> +	.set_bus_width		= sdhci_set_bus_width,
> +	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
> +	.get_max_clock		= dwcmshc_get_max_clock,
> +	.reset			= th1520_sdhci_reset,
> +	.adma_write_desc	= dwcmshc_adma_write_desc,
> +	.voltage_switch		= th1520_phy_1_8v_init,
> +	.platform_execute_tuning = &th1520_execute_tuning,
> +};
> +
>  static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
>  	.ops = &sdhci_dwcmshc_ops,
>  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> @@ -378,6 +668,15 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
>  		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
>  };
>  
> +static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
> +	.ops = &sdhci_dwcmshc_th1520_ops,
> +
> +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> +		  SDHCI_QUIRK_BROKEN_DMA |
> +		  SDHCI_QUIRK_BROKEN_ADMA,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> +};
> +
>  static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  {
>  	int err;
> @@ -434,6 +733,10 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv
>  }
>  
>  static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> +	{
> +		.compatible = "thead,th1520-dwcmshc",
> +		.data = &sdhci_dwcmshc_th1520_pdata,
> +	},
>  	{
>  		.compatible = "rockchip,rk3588-dwcmshc",
>  		.data = &sdhci_dwcmshc_rk35xx_pdata,
> @@ -541,6 +844,39 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  			goto err_clk;
>  	}
>  
> +	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> +
> +		priv->delay_line = DELAY_LINE_DEFAULT;
> +
> +		if (device_property_present(&pdev->dev, "non-removable"))
> +			priv->non_removable = 1;
> +		else
> +			priv->non_removable = 0;
> +
> +		if (device_property_present(&pdev->dev, "thead,pull-up"))
> +			priv->pull_up_en = 1;
> +		else
> +			priv->pull_up_en = 0;
> +
> +		if (device_property_present(&pdev->dev, "thead,io-fixed-1v8"))
> +			priv->io_fixed_1v8 = true;
> +		else
> +			priv->io_fixed_1v8 = false;
> +
> +		/*
> +		 * start_signal_voltage_switch() will try 3.3V first
> +		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
> +		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> +		 * in sdhci_start_signal_voltage_switch().
> +		 */
> +		if(priv->io_fixed_1v8){
> +			host->flags &=~SDHCI_SIGNALING_330;
> +			host->flags |= SDHCI_SIGNALING_180;
> +		}
> +
> +		sdhci_enable_v4_mode(host);
> +	}
> +
>  #ifdef CONFIG_ACPI
>  	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
>  		sdhci_enable_v4_mode(host);
> 
> -- 
> 2.34.1
> 

Re: [PATCH RFC v2 1/4] dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support

[Krzysztof Kozlowski]

On 05/08/2023 05:14, Drew Fustini wrote:
> Add compatible value for the T-Head TH1520 dwcmshc controller and
> thead,io-fixed-1v8 and thead,pull-up properties.
> 
> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> ---
>  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> index a43eb837f8da..57602c345cab 100644
> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> @@ -19,6 +19,7 @@ properties:
>        - rockchip,rk3568-dwcmshc
>        - rockchip,rk3588-dwcmshc
>        - snps,dwcmshc-sdhci
> +      - thead,th1520-dwcmshc
>  
>    reg:
>      maxItems: 1
> @@ -60,6 +61,14 @@ properties:
>      description: Specify the number of delay for tx sampling.
>      $ref: /schemas/types.yaml#/definitions/uint8
>  
> +  thead,io-fixed-1v8:
> +    description: SoC PHY pad is fixed 1.8V
> +    type: boolean

Isn't this duplicating existing properties for MMC modes with 1.8 V?

> +
> +  thead,pull-up:
> +    description: True if pull-up, false if pull-down

This explains me nothing. No clue what you are pulling and why do you
need it. Pin pulls should be done via pin controller, not MMC.

Anyway you should have here allOf:if:then (move the allOf: from top to
behind "required:") which will disallow these properties for other variants.

> +    type: boolean
> +
>  
>  required:
>    - compatible
> 

Best regards,
Krzysztof

Re: [PATCH RFC v2 4/4] mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520

[Drew Fustini]

On Sun, Aug 06, 2023 at 06:09:39PM +0800, Jisheng Zhang wrote:
> On Fri, Aug 04, 2023 at 08:14:48PM -0700, Drew Fustini wrote:
> > Add basic support for the T-Head TH1520 SoC mmc controller with the new
> > compatible "thead,th1520-dwcmshc".
> > 
> > However, quirks are currently set to disable DMA and use PIO. The proper
> > settings for DMA support still need to be determined.
> 
> Hi Drew,
> 
> Thank you for this patch. I think we need to make DMA work even in the
> first version.

Thanks for your review. I agree that DMA should be made to work.
When I remove the SDHCI_QUIRK_BROKEN_ADMA quirk, I see the following in
the boot log [1]:

[    3.633611] mmc0: SDHCI controller on ffe7080000.mmc [ffe7080000.mmc] using ADMA 64-bit
[    3.723080] mmc0: ADMA error: 0x02000000
[    4.327491] mmc0: error -5 whilst initialising MMC card

I'm going to add some more debug output so I can better understand what
exactly is failing.

> 
> Several comments below.
> 
> > 
> > Another issue is th1520-specific code for MMC_TIMING_MMC_HS400 in
> > dwcmshc_set_uhs_signaling() will run on all platforms which is not
> > correct. One solution could be to add a th1520 flag to dwcmshc_priv but
> > that is hacky. Another solution could be to set the set_uhs_signaling op
> > in sdhci_dwcmshc_th1520_ops to a th1520-specific function. However, that
> > new function would have to duplicate all the code in the current
> > dwcmshc_set_uhs_signaling().
> > 
> > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> > ---
> >  drivers/mmc/host/sdhci-of-dwcmshc.c | 336 ++++++++++++++++++++++++++++++++++++
> >  1 file changed, 336 insertions(+)
> > 
> > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > index e68cd87998c8..d35e204cdb16 100644
> > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > @@ -71,6 +71,63 @@
> >  	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
> >  #define RK35xx_MAX_CLKS 3
> >  
> > +/* T-Head specific registers */
> 
> I guess this may not be T-HEAD specific but dwc phy, could you please
> check?

Yes, I will try to find out if if this really is t-head specific or just
standard for the DWC PHY.

> > +#define DWC_MSHC_PTR_PHY_R	0x300
> > +#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
> > +#define PHY_RSTN		0x0
> > +#define PAD_SP			0x10
> > +#define PAD_SN			0x14
> > +
> > +#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
> > +#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
> > +#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
> > +#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
> > +#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
> > +#define RXSEL			0x0
> > +#define WEAKPULL_EN		0x3
> > +#define TXSLEW_CTRL_P		0x5
> > +#define TXSLEW_CTRL_N		0x9
> > +
> > +#define PHY_PADTEST_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0e)
> > +#define PHY_PADTEST_OUT_R	(DWC_MSHC_PTR_PHY_R + 0x10)
> > +#define PHY_PADTEST_IN_R	(DWC_MSHC_PTR_PHY_R + 0x12)
> > +#define PHY_PRBS_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x18)
> > +#define PHY_PHYLBK_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1a)
> > +#define PHY_COMMDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1c)
> > +
> > +#define PHY_SDCLKDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1d)
> > +#define UPDATE_DC		0x4
> > +
> > +#define PHY_SDCLKDL_DC_R	(DWC_MSHC_PTR_PHY_R + 0x1e)
> > +#define PHY_SMPLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x20)
> > +#define PHY_ATDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x21)
> > +#define INPSEL_CNFG		2
> > +
> > +#define PHY_DLL_CTRL_R		(DWC_MSHC_PTR_PHY_R + 0x24)
> > +#define DLL_EN			0x0
> > +
> > +#define PHY_DLL_CNFG1_R		(DWC_MSHC_PTR_PHY_R + 0x25)
> > +#define PHY_DLL_CNFG2_R		(DWC_MSHC_PTR_PHY_R + 0x26)
> > +#define PHY_DLLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x28)
> > +#define SLV_INPSEL		0x5
> > +
> > +#define P_VENDOR_SPECIFIC_AREA	0x500
> > +#define EMMC_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x2c)
> > +#define AT_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x40)
> > +#define AT_EN			0x0
> > +#define CI_SEL			0x1
> > +#define SWIN_TH_EN		0x2
> > +#define RPT_TUNE_ERR		0x3
> > +#define SW_TUNE_EN		0x4
> > +#define WIN_EDGE_SEL		0x8
> > +#define TUNE_CLK_STOP_EN	0x10
> > +#define PRE_CHANGE_DLY		0x11
> > +#define POST_CHANGE_DLY		0x13
> > +#define SWIN_TH_VAL		0x18
> > +
> > +#define DELAY_LINE_HS400	24
> > +#define DELAY_LINE_DEFAULT	50
> > +
> >  #define BOUNDARY_OK(addr, len) \
> >  	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> >  
> > @@ -91,6 +148,10 @@ struct dwcmshc_priv {
> >  	struct clk	*bus_clk;
> >  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
> >  	void *priv; /* pointer to SoC private stuff */
> > +	uint32_t delay_line;
> > +	bool non_removable;
> 
> can be replaced with mmc_host.caps & MMC_CAP_NONREMOVABLE

Thank you, I'll change in the next version.

> 
> > +	bool pull_up_en;
> > +	bool io_fixed_1v8;
> 
> replace them with flag?

Do you mean that I should replace the above bool's with a single flags
field in the struct where the bool's would become bit fields in flags?

> 
> >  };
> >  
> >  /*
> > @@ -156,11 +217,171 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> >  	sdhci_request(mmc, mrq);
> >  }
> >  
> > +static void sdhci_phy_1_8v_init_no_pull(struct sdhci_host *host)
> > +{
> > +	uint32_t val;
> > +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> > +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> > +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
> 
> These magic numbers need a proper macro definitions.

I'll try to find out their meaning and document it.

> 
> > +
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(1 << 4);
> 
> ditto
> 
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +
> > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val | 1, PHY_CMDPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val | 1, PHY_DATAPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > +	sdhci_writew(host, val | 1, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > +	sdhci_writew(host, val | 1, PHY_STBPAD_CNFG_R);
> > +
> > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void sdhci_phy_3_3v_init_no_pull(struct sdhci_host *host)
> > +{
> > +	uint32_t val;
> > +
> > +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> > +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> > +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
> > +
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(1 << 4);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val | 2, PHY_CMDPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val | 2, PHY_DATAPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > +	sdhci_writew(host, val | 2, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > +	sdhci_writew(host, val | 2, PHY_STBPAD_CNFG_R);
> > +
> > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void th1520_phy_1_8v_init(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val;
> > +
> > +	if (!priv)
> > +		return;
> > +
> > +	if (priv->pull_up_en == 0) {
> > +		sdhci_phy_1_8v_init_no_pull(host);
> > +		return;
> > +	}
> > +
> > +	/* set driving force */
> > +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> > +
> > +	/* disable delay lane */
> > +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay lane */
> > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(1 << UPDATE_DC);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	val = (1 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > +	val = (1 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +
> > +	/* enable data strobe mode */
> > +	sdhci_writeb(host, 3 << SLV_INPSEL, PHY_DLLDL_CNFG_R);
> > +	sdhci_writeb(host, (1 << DLL_EN),  PHY_DLL_CTRL_R);
> > +}
> > +
> > +static void th1520_phy_3_3v_init(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u32 val;
> > +
> > +	if (priv->pull_up_en == 0) {
> > +		sdhci_phy_3_3v_init_no_pull(host);
> > +		return;
> > +	}
> > +
> > +	/* set driving force */
> > +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> > +
> > +	/* disable delay lane */
> > +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> > +
> > +	/* set delay lane */
> > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> > +
> > +	/* enable delay lane */
> > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > +	val &= ~(1 << UPDATE_DC);
> > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > +
> > +	val = (2 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > +
> > +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > +
> > +	val = (2 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > +}
> > +
> > +
> > +static void th1520_sdhci_set_phy(struct sdhci_host *host)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u8 emmc_ctl;
> > +
> > +	/* Before power on, set PHY configs */
> > +	emmc_ctl = sdhci_readw(host, EMMC_CTRL_R);
> > +	if (priv->non_removable) {
> > +		th1520_phy_1_8v_init(host);
> > +		emmc_ctl |= (1 << DWCMSHC_CARD_IS_EMMC);
> > +	} else {
> > +		th1520_phy_3_3v_init(host);
> > +		emmc_ctl &=~(1 << DWCMSHC_CARD_IS_EMMC);
> > +	}
> > +	sdhci_writeb(host, emmc_ctl, EMMC_CTRL_R);
> > +	sdhci_writeb(host, 0x25, PHY_DLL_CNFG1_R);
> 
> magic 0x25 needs a meaningful MACRO definition.

Okay, I'll investigate the meaning of that value and define it.

> 
> > +}
> > +
> >  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> >  				      unsigned int timing)
> >  {
> >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> >  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +
> 
> we don't need this extra blank line

Okay

> 
> >  	u16 ctrl, ctrl_2;
> >  
> >  	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > @@ -188,7 +409,22 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> >  		ctrl_2 |= DWCMSHC_CTRL_HS400;
> >  	}
> >  
> > +	if (priv->io_fixed_1v8)
> > +		ctrl_2 |= SDHCI_CTRL_VDD_180;
> > +
> >  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > +
> > +	/* TODO: add check so that this only runs on th1520  */
> 
> this TODO needs to be solved since it affects other platforms such as NV
> and RK

Yes, I need to fix this. For the next version, I am considering adding
a flag to dwcmshc_priv that would gate the following code block.


> > +	if (timing == MMC_TIMING_MMC_HS400) {
> > +		/* disable auto tuning */
> > +		u32 reg = sdhci_readl(host, AT_CTRL_R);
> > +		reg &= ~1;
> > +		sdhci_writel(host, reg, AT_CTRL_R);
> > +		priv->delay_line = DELAY_LINE_HS400;
> > +		th1520_sdhci_set_phy(host);
> > +	} else {
> > +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> > +	}
> >  }
> >  
> >  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
> > @@ -337,6 +573,49 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> >  	sdhci_reset(host, mask);
> >  }
> >  
> > +static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
> > +{
> > +	u32 val = 0;
> > +
> > +	sdhci_writeb(host, 3 << INPSEL_CNFG, PHY_ATDL_CNFG_R);
> > +
> > +	val = sdhci_readl(host, AT_CTRL_R);
> > +	val &= ~((1 << CI_SEL) | (1 << RPT_TUNE_ERR) \
> > +	    | (1 << SW_TUNE_EN) |(0xf << WIN_EDGE_SEL));
> > +	val |= (1 << AT_EN) | (1 << SWIN_TH_EN) | (1 << TUNE_CLK_STOP_EN)\
> > +	    | (1 << PRE_CHANGE_DLY) | (3 << POST_CHANGE_DLY) | (9 << SWIN_TH_VAL);
> > +
> > +	sdhci_writel(host, val, AT_CTRL_R);
> > +
> > +	val = sdhci_readl(host, AT_CTRL_R);
> > +	if(!(val & (1 << AT_EN))) {
> > +		pr_warn("%s(): auto tuning is not enabled", __func__);
> 
> AFAIK, the controller supports both auto tuning and sw tuning.

Okay, I'll have to investigate further why it is like this in the vendor
kernel [2].

> 
> > +		return -1;
> 
> can we replace -1 with meaningful error number?
> 

Yes, will do for next version.

> > +	}
> > +
> > +	val &= ~(1 << AT_EN);
> > +	sdhci_writel(host, val, AT_CTRL_R);
> > +
> > +	return 0;
> > +}
> > +
> > +static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> > +{
> > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > +	u16 ctrl_2;
> > +
> > +	sdhci_reset(host, mask);
> > +
> > +	if(priv->io_fixed_1v8){
> > +		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > +		if(! (ctrl_2 & SDHCI_CTRL_VDD_180)){
> > +			ctrl_2 |= SDHCI_CTRL_VDD_180;
> > +			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > +		}
> > +	}
> > +}
> > +
> >  static const struct sdhci_ops sdhci_dwcmshc_ops = {
> >  	.set_clock		= sdhci_set_clock,
> >  	.set_bus_width		= sdhci_set_bus_width,
> > @@ -355,6 +634,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> >  	.adma_write_desc	= dwcmshc_adma_write_desc,
> >  };
> >  
> > +static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> > +	.set_clock		= sdhci_set_clock,
> > +	.set_bus_width		= sdhci_set_bus_width,
> > +	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
> > +	.get_max_clock		= dwcmshc_get_max_clock,
> > +	.reset			= th1520_sdhci_reset,
> > +	.adma_write_desc	= dwcmshc_adma_write_desc,
> > +	.voltage_switch		= th1520_phy_1_8v_init,
> > +	.platform_execute_tuning = &th1520_execute_tuning,
> > +};
> > +
> >  static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
> >  	.ops = &sdhci_dwcmshc_ops,
> >  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > @@ -378,6 +668,15 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
> >  		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> >  };
> >  
> > +static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
> > +	.ops = &sdhci_dwcmshc_th1520_ops,
> > +
> > +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> > +		  SDHCI_QUIRK_BROKEN_DMA |
> > +		  SDHCI_QUIRK_BROKEN_ADMA,
> > +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> > +};
> > +
> >  static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> >  {
> >  	int err;
> > @@ -434,6 +733,10 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv
> >  }
> >  
> >  static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> > +	{
> > +		.compatible = "thead,th1520-dwcmshc",
> > +		.data = &sdhci_dwcmshc_th1520_pdata,
> > +	},
> >  	{
> >  		.compatible = "rockchip,rk3588-dwcmshc",
> >  		.data = &sdhci_dwcmshc_rk35xx_pdata,
> > @@ -541,6 +844,39 @@ static int dwcmshc_probe(struct platform_device *pdev)
> >  			goto err_clk;
> >  	}
> >  
> > +	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> > +
> > +		priv->delay_line = DELAY_LINE_DEFAULT;
> > +
> > +		if (device_property_present(&pdev->dev, "non-removable"))
> > +			priv->non_removable = 1;
> > +		else
> > +			priv->non_removable = 0;
> > +
> > +		if (device_property_present(&pdev->dev, "thead,pull-up"))
> > +			priv->pull_up_en = 1;
> > +		else
> > +			priv->pull_up_en = 0;
> > +
> > +		if (device_property_present(&pdev->dev, "thead,io-fixed-1v8"))
> > +			priv->io_fixed_1v8 = true;
> > +		else
> > +			priv->io_fixed_1v8 = false;
> > +
> > +		/*
> > +		 * start_signal_voltage_switch() will try 3.3V first
> > +		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
> > +		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> > +		 * in sdhci_start_signal_voltage_switch().
> > +		 */
> > +		if(priv->io_fixed_1v8){
> > +			host->flags &=~SDHCI_SIGNALING_330;
> > +			host->flags |= SDHCI_SIGNALING_180;
> > +		}
> > +
> > +		sdhci_enable_v4_mode(host);
> > +	}
> > +
> >  #ifdef CONFIG_ACPI
> >  	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> >  		sdhci_enable_v4_mode(host);
> > 
> > -- 
> > 2.34.1
> > 

Thank you,
Drew

[1] https://gist.github.com/pdp7/a8301b895c2dc293f2e0210e77e45e01
[2] https://git.beagleboard.org/beaglev-ahead/beaglev-ahead-linux/-/blob/beaglev-v5.10.113-1.1.2/drivers/mmc/host/sdhci-of-dwcmshc.c#L238

Re: [PATCH RFC v2 1/4] dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support

[Drew Fustini]

On Mon, Aug 07, 2023 at 08:29:21AM +0200, Krzysztof Kozlowski wrote:
> On 05/08/2023 05:14, Drew Fustini wrote:
> > Add compatible value for the T-Head TH1520 dwcmshc controller and
> > thead,io-fixed-1v8 and thead,pull-up properties.
> > 
> > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> > ---
> >  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> > index a43eb837f8da..57602c345cab 100644
> > --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> > +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
> > @@ -19,6 +19,7 @@ properties:
> >        - rockchip,rk3568-dwcmshc
> >        - rockchip,rk3588-dwcmshc
> >        - snps,dwcmshc-sdhci
> > +      - thead,th1520-dwcmshc
> >  
> >    reg:
> >      maxItems: 1
> > @@ -60,6 +61,14 @@ properties:
> >      description: Specify the number of delay for tx sampling.
> >      $ref: /schemas/types.yaml#/definitions/uint8
> >  
> > +  thead,io-fixed-1v8:
> > +    description: SoC PHY pad is fixed 1.8V
> > +    type: boolean
> 
> Isn't this duplicating existing properties for MMC modes with 1.8 V?

Thank you for reviewing. Yes, now that you mention it, I do see those
properties now in mmc-controller.yaml. It seems like the existing
mmc-ddr-1_8v property would be appropriate.

> 
> > +
> > +  thead,pull-up:
> > +    description: True if pull-up, false if pull-down
> 
> This explains me nothing. No clue what you are pulling and why do you
> need it. Pin pulls should be done via pin controller, not MMC.

Good point that my description is not helpful. The pull-up property
determines whether certain phy registers are written to. I need to try
to can get documentation on the phy so that I can better understand the
details of the pull-up configuration in the phy registers.

> 
> Anyway you should have here allOf:if:then (move the allOf: from top to
> behind "required:") which will disallow these properties for other variants.

I noticed that nvidia,tegra20-sdhci.yaml has several lines related to
pull-up/down configuration:

218   - if:
219       properties:
220         compatible:
221           contains:
222             const: nvidia,tegra210-sdhci
223     then:
224       properties:
225         pinctrl-names:
226           oneOf:
227             - items:
228                 - const: sdmmc-3v3
229                   description: pad configuration for 3.3 V
230                 - const: sdmmc-1v8
231                   description: pad configuration for 1.8 V
232                 - const: sdmmc-3v3-drv
233                   description: pull-up/down configuration for 3.3 V
234                 - const: sdmmc-1v8-drv
235                   description: pull-up/down configuration for 1.8 V
236             - items:
237                 - const: sdmmc-3v3-drv
238                   description: pull-up/down configuration for 3.3 V
239                 - const: sdmmc-1v8-drv
240                   description: pull-up/down configuration for 1.8 V
241             - items:
242                 - const: sdmmc-1v8-drv
243                   description: pull-up/down configuration for 1.8 V

Do you think creating something like that would be a good approach?

Thank you,
Drew

Re: [PATCH RFC v2 4/4] mmc: sdhci-of-dwcmshc: Add support for T-Head TH1520

[Guo Ren]

On Wed, Aug 16, 2023 at 02:17:16PM -0700, Drew Fustini wrote:
> On Sun, Aug 06, 2023 at 06:09:39PM +0800, Jisheng Zhang wrote:
> > On Fri, Aug 04, 2023 at 08:14:48PM -0700, Drew Fustini wrote:
> > > Add basic support for the T-Head TH1520 SoC mmc controller with the new
> > > compatible "thead,th1520-dwcmshc".
> > > 
> > > However, quirks are currently set to disable DMA and use PIO. The proper
> > > settings for DMA support still need to be determined.
> > 
> > Hi Drew,
> > 
> > Thank you for this patch. I think we need to make DMA work even in the
> > first version.
> 
> Thanks for your review. I agree that DMA should be made to work.
> When I remove the SDHCI_QUIRK_BROKEN_ADMA quirk, I see the following in
> the boot log [1]:
> 
> [    3.633611] mmc0: SDHCI controller on ffe7080000.mmc [ffe7080000.mmc] using ADMA 64-bit
> [    3.723080] mmc0: ADMA error: 0x02000000
> [    4.327491] mmc0: error -5 whilst initialising MMC card
> 
> I'm going to add some more debug output so I can better understand what
> exactly is failing.
> 
> > 
> > Several comments below.
> > 
> > > 
> > > Another issue is th1520-specific code for MMC_TIMING_MMC_HS400 in
> > > dwcmshc_set_uhs_signaling() will run on all platforms which is not
> > > correct. One solution could be to add a th1520 flag to dwcmshc_priv but
> > > that is hacky. Another solution could be to set the set_uhs_signaling op
> > > in sdhci_dwcmshc_th1520_ops to a th1520-specific function. However, that
> > > new function would have to duplicate all the code in the current
> > > dwcmshc_set_uhs_signaling().
> > > 
> > > Signed-off-by: Drew Fustini <dfustini@baylibre.com>
> > > ---
> > >  drivers/mmc/host/sdhci-of-dwcmshc.c | 336 ++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 336 insertions(+)
> > > 
> > > diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > index e68cd87998c8..d35e204cdb16 100644
> > > --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> > > @@ -71,6 +71,63 @@
> > >  	(((x) & DWCMSHC_EMMC_DLL_TIMEOUT) == 0))
> > >  #define RK35xx_MAX_CLKS 3
> > >  
> > > +/* T-Head specific registers */
> > 
> > I guess this may not be T-HEAD specific but dwc phy, could you please
> > check?
It's not a standard DesginWare MMC PHY registers, and we modiified
something. We could keep this code in custom first to make it work around
and see how to merge it into standard DesginWare MMC PHY registers next.

> 
> Yes, I will try to find out if if this really is t-head specific or just
> standard for the DWC PHY.
> 
> > > +#define DWC_MSHC_PTR_PHY_R	0x300
> > > +#define PHY_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x00)
> > > +#define PHY_RSTN		0x0
> > > +#define PAD_SP			0x10
> > > +#define PAD_SN			0x14
> > > +
> > > +#define PHY_CMDPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x04)
> > > +#define PHY_DATAPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x06)
> > > +#define PHY_CLKPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x08)
> > > +#define PHY_STBPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0a)
> > > +#define PHY_RSTNPAD_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0c)
> > > +#define RXSEL			0x0
> > > +#define WEAKPULL_EN		0x3
> > > +#define TXSLEW_CTRL_P		0x5
> > > +#define TXSLEW_CTRL_N		0x9
> > > +
> > > +#define PHY_PADTEST_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x0e)
> > > +#define PHY_PADTEST_OUT_R	(DWC_MSHC_PTR_PHY_R + 0x10)
> > > +#define PHY_PADTEST_IN_R	(DWC_MSHC_PTR_PHY_R + 0x12)
> > > +#define PHY_PRBS_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x18)
> > > +#define PHY_PHYLBK_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1a)
> > > +#define PHY_COMMDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1c)
> > > +
> > > +#define PHY_SDCLKDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x1d)
> > > +#define UPDATE_DC		0x4
> > > +
> > > +#define PHY_SDCLKDL_DC_R	(DWC_MSHC_PTR_PHY_R + 0x1e)
> > > +#define PHY_SMPLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x20)
> > > +#define PHY_ATDL_CNFG_R		(DWC_MSHC_PTR_PHY_R + 0x21)
> > > +#define INPSEL_CNFG		2
> > > +
> > > +#define PHY_DLL_CTRL_R		(DWC_MSHC_PTR_PHY_R + 0x24)
> > > +#define DLL_EN			0x0
> > > +
> > > +#define PHY_DLL_CNFG1_R		(DWC_MSHC_PTR_PHY_R + 0x25)
> > > +#define PHY_DLL_CNFG2_R		(DWC_MSHC_PTR_PHY_R + 0x26)
> > > +#define PHY_DLLDL_CNFG_R	(DWC_MSHC_PTR_PHY_R + 0x28)
> > > +#define SLV_INPSEL		0x5
> > > +
> > > +#define P_VENDOR_SPECIFIC_AREA	0x500
> > > +#define EMMC_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x2c)
> > > +#define AT_CTRL_R		(P_VENDOR_SPECIFIC_AREA + 0x40)
> > > +#define AT_EN			0x0
> > > +#define CI_SEL			0x1
> > > +#define SWIN_TH_EN		0x2
> > > +#define RPT_TUNE_ERR		0x3
> > > +#define SW_TUNE_EN		0x4
> > > +#define WIN_EDGE_SEL		0x8
> > > +#define TUNE_CLK_STOP_EN	0x10
> > > +#define PRE_CHANGE_DLY		0x11
> > > +#define POST_CHANGE_DLY		0x13
> > > +#define SWIN_TH_VAL		0x18
> > > +
> > > +#define DELAY_LINE_HS400	24
> > > +#define DELAY_LINE_DEFAULT	50
> > > +
> > >  #define BOUNDARY_OK(addr, len) \
> > >  	((addr | (SZ_128M - 1)) == ((addr + len - 1) | (SZ_128M - 1)))
> > >  
> > > @@ -91,6 +148,10 @@ struct dwcmshc_priv {
> > >  	struct clk	*bus_clk;
> > >  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA reg */
> > >  	void *priv; /* pointer to SoC private stuff */
> > > +	uint32_t delay_line;
> > > +	bool non_removable;
> > 
> > can be replaced with mmc_host.caps & MMC_CAP_NONREMOVABLE
> 
> Thank you, I'll change in the next version.
> 
> > 
> > > +	bool pull_up_en;
> > > +	bool io_fixed_1v8;
> > 
> > replace them with flag?
> 
> Do you mean that I should replace the above bool's with a single flags
> field in the struct where the bool's would become bit fields in flags?
> 
> > 
> > >  };
> > >  
> > >  /*
> > > @@ -156,11 +217,171 @@ static void dwcmshc_request(struct mmc_host *mmc, struct mmc_request *mrq)
> > >  	sdhci_request(mmc, mrq);
> > >  }
> > >  
> > > +static void sdhci_phy_1_8v_init_no_pull(struct sdhci_host *host)
> > > +{
> > > +	uint32_t val;
> > > +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> > > +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> > > +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
> > 
> > These magic numbers need a proper macro definitions.
> 
> I'll try to find out their meaning and document it.
> 
> > 
> > > +
> > > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > > +	val &= ~(1 << 4);
> > 
> > ditto
> > 
> > > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +
> > > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 1, PHY_CMDPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 1, PHY_DATAPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 1, PHY_RSTNPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 1, PHY_STBPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > > +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> > > +}
> > > +
> > > +static void sdhci_phy_3_3v_init_no_pull(struct sdhci_host *host)
> > > +{
> > > +	uint32_t val;
> > > +
> > > +	sdhci_writel(host, 1, DWC_MSHC_PTR_PHY_R);
> > > +	sdhci_writeb(host, 1 << 4, PHY_SDCLKDL_CNFG_R);
> > > +	sdhci_writeb(host, 0x40, PHY_SDCLKDL_DC_R);
> > > +
> > > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > > +	val &= ~(1 << 4);
> > > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_CMDPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 2, PHY_CMDPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_DATAPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 2, PHY_DATAPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_RSTNPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 2, PHY_RSTNPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readw(host, PHY_STBPAD_CNFG_R);
> > > +	sdhci_writew(host, val | 2, PHY_STBPAD_CNFG_R);
> > > +
> > > +	val = sdhci_readb(host, PHY_DLL_CTRL_R);
> > > +	sdhci_writeb(host, val | 1, PHY_DLL_CTRL_R);
> > > +}
> > > +
> > > +static void th1520_phy_1_8v_init(struct sdhci_host *host)
> > > +{
> > > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > > +	u32 val;
> > > +
> > > +	if (!priv)
> > > +		return;
> > > +
> > > +	if (priv->pull_up_en == 0) {
> > > +		sdhci_phy_1_8v_init_no_pull(host);
> > > +		return;
> > > +	}
> > > +
> > > +	/* set driving force */
> > > +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> > > +
> > > +	/* disable delay lane */
> > > +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +	/* set delay lane */
> > > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > > +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> > > +
> > > +	/* enable delay lane */
> > > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > > +	val &= ~(1 << UPDATE_DC);
> > > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +	val = (1 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > > +
> > > +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > > +
> > > +	val = (1 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > > +
> > > +	/* enable data strobe mode */
> > > +	sdhci_writeb(host, 3 << SLV_INPSEL, PHY_DLLDL_CNFG_R);
> > > +	sdhci_writeb(host, (1 << DLL_EN),  PHY_DLL_CTRL_R);
> > > +}
> > > +
> > > +static void th1520_phy_3_3v_init(struct sdhci_host *host)
> > > +{
> > > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > > +	u32 val;
> > > +
> > > +	if (priv->pull_up_en == 0) {
> > > +		sdhci_phy_3_3v_init_no_pull(host);
> > > +		return;
> > > +	}
> > > +
> > > +	/* set driving force */
> > > +	sdhci_writel(host, (1 << PHY_RSTN) | (0xc << PAD_SP) | (0xc << PAD_SN), PHY_CNFG_R);
> > > +
> > > +	/* disable delay lane */
> > > +	sdhci_writeb(host, 1 << UPDATE_DC, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +	/* set delay lane */
> > > +	sdhci_writeb(host, priv->delay_line, PHY_SDCLKDL_DC_R);
> > > +	sdhci_writeb(host, 0xa, PHY_DLL_CNFG2_R);
> > > +
> > > +	/* enable delay lane */
> > > +	val = sdhci_readb(host, PHY_SDCLKDL_CNFG_R);
> > > +	val &= ~(1 << UPDATE_DC);
> > > +	sdhci_writeb(host, val, PHY_SDCLKDL_CNFG_R);
> > > +
> > > +	val = (2 << RXSEL) | (1 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_CMDPAD_CNFG_R);
> > > +	sdhci_writew(host, val, PHY_DATAPAD_CNFG_R);
> > > +	sdhci_writew(host, val, PHY_RSTNPAD_CNFG_R);
> > > +
> > > +	val = (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_CLKPAD_CNFG_R);
> > > +
> > > +	val = (2 << RXSEL) | (2 << WEAKPULL_EN) | (3 << TXSLEW_CTRL_P) | (3 << TXSLEW_CTRL_N);
> > > +	sdhci_writew(host, val, PHY_STBPAD_CNFG_R);
> > > +}
> > > +
> > > +
> > > +static void th1520_sdhci_set_phy(struct sdhci_host *host)
> > > +{
> > > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > > +	u8 emmc_ctl;
> > > +
> > > +	/* Before power on, set PHY configs */
> > > +	emmc_ctl = sdhci_readw(host, EMMC_CTRL_R);
> > > +	if (priv->non_removable) {
> > > +		th1520_phy_1_8v_init(host);
> > > +		emmc_ctl |= (1 << DWCMSHC_CARD_IS_EMMC);
> > > +	} else {
> > > +		th1520_phy_3_3v_init(host);
> > > +		emmc_ctl &=~(1 << DWCMSHC_CARD_IS_EMMC);
> > > +	}
> > > +	sdhci_writeb(host, emmc_ctl, EMMC_CTRL_R);
> > > +	sdhci_writeb(host, 0x25, PHY_DLL_CNFG1_R);
> > 
> > magic 0x25 needs a meaningful MACRO definition.
> 
> Okay, I'll investigate the meaning of that value and define it.
> 
> > 
> > > +}
> > > +
> > >  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> > >  				      unsigned int timing)
> > >  {
> > >  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > >  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > > +
> > 
> > we don't need this extra blank line
> 
> Okay
> 
> > 
> > >  	u16 ctrl, ctrl_2;
> > >  
> > >  	ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > > @@ -188,7 +409,22 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
> > >  		ctrl_2 |= DWCMSHC_CTRL_HS400;
> > >  	}
> > >  
> > > +	if (priv->io_fixed_1v8)
> > > +		ctrl_2 |= SDHCI_CTRL_VDD_180;
> > > +
> > >  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > > +
> > > +	/* TODO: add check so that this only runs on th1520  */
> > 
> > this TODO needs to be solved since it affects other platforms such as NV
> > and RK
> 
> Yes, I need to fix this. For the next version, I am considering adding
> a flag to dwcmshc_priv that would gate the following code block.
> 
> 
> > > +	if (timing == MMC_TIMING_MMC_HS400) {
> > > +		/* disable auto tuning */
> > > +		u32 reg = sdhci_readl(host, AT_CTRL_R);
> > > +		reg &= ~1;
> > > +		sdhci_writel(host, reg, AT_CTRL_R);
> > > +		priv->delay_line = DELAY_LINE_HS400;
> > > +		th1520_sdhci_set_phy(host);
> > > +	} else {
> > > +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> > > +	}
> > >  }
> > >  
> > >  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
> > > @@ -337,6 +573,49 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
> > >  	sdhci_reset(host, mask);
> > >  }
> > >  
> > > +static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
> > > +{
> > > +	u32 val = 0;
> > > +
> > > +	sdhci_writeb(host, 3 << INPSEL_CNFG, PHY_ATDL_CNFG_R);
> > > +
> > > +	val = sdhci_readl(host, AT_CTRL_R);
> > > +	val &= ~((1 << CI_SEL) | (1 << RPT_TUNE_ERR) \
> > > +	    | (1 << SW_TUNE_EN) |(0xf << WIN_EDGE_SEL));
> > > +	val |= (1 << AT_EN) | (1 << SWIN_TH_EN) | (1 << TUNE_CLK_STOP_EN)\
> > > +	    | (1 << PRE_CHANGE_DLY) | (3 << POST_CHANGE_DLY) | (9 << SWIN_TH_VAL);
> > > +
> > > +	sdhci_writel(host, val, AT_CTRL_R);
> > > +
> > > +	val = sdhci_readl(host, AT_CTRL_R);
> > > +	if(!(val & (1 << AT_EN))) {
> > > +		pr_warn("%s(): auto tuning is not enabled", __func__);
> > 
> > AFAIK, the controller supports both auto tuning and sw tuning.
> 
> Okay, I'll have to investigate further why it is like this in the vendor
> kernel [2].
> 
> > 
> > > +		return -1;
> > 
> > can we replace -1 with meaningful error number?
> > 
> 
> Yes, will do for next version.
> 
> > > +	}
> > > +
> > > +	val &= ~(1 << AT_EN);
> > > +	sdhci_writel(host, val, AT_CTRL_R);
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
> > > +{
> > > +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> > > +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> > > +	u16 ctrl_2;
> > > +
> > > +	sdhci_reset(host, mask);
> > > +
> > > +	if(priv->io_fixed_1v8){
> > > +		ctrl_2 = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> > > +		if(! (ctrl_2 & SDHCI_CTRL_VDD_180)){
> > > +			ctrl_2 |= SDHCI_CTRL_VDD_180;
> > > +			sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
> > > +		}
> > > +	}
> > > +}
> > > +
> > >  static const struct sdhci_ops sdhci_dwcmshc_ops = {
> > >  	.set_clock		= sdhci_set_clock,
> > >  	.set_bus_width		= sdhci_set_bus_width,
> > > @@ -355,6 +634,17 @@ static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> > >  	.adma_write_desc	= dwcmshc_adma_write_desc,
> > >  };
> > >  
> > > +static const struct sdhci_ops sdhci_dwcmshc_th1520_ops = {
> > > +	.set_clock		= sdhci_set_clock,
> > > +	.set_bus_width		= sdhci_set_bus_width,
> > > +	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
> > > +	.get_max_clock		= dwcmshc_get_max_clock,
> > > +	.reset			= th1520_sdhci_reset,
> > > +	.adma_write_desc	= dwcmshc_adma_write_desc,
> > > +	.voltage_switch		= th1520_phy_1_8v_init,
> > > +	.platform_execute_tuning = &th1520_execute_tuning,
> > > +};
> > > +
> > >  static const struct sdhci_pltfm_data sdhci_dwcmshc_pdata = {
> > >  	.ops = &sdhci_dwcmshc_ops,
> > >  	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN,
> > > @@ -378,6 +668,15 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_rk35xx_pdata = {
> > >  		   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> > >  };
> > >  
> > > +static const struct sdhci_pltfm_data sdhci_dwcmshc_th1520_pdata = {
> > > +	.ops = &sdhci_dwcmshc_th1520_ops,
> > > +
> > > +	.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> > > +		  SDHCI_QUIRK_BROKEN_DMA |
> > > +		  SDHCI_QUIRK_BROKEN_ADMA,
> > > +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> > > +};
> > > +
> > >  static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> > >  {
> > >  	int err;
> > > @@ -434,6 +733,10 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv
> > >  }
> > >  
> > >  static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> > > +	{
> > > +		.compatible = "thead,th1520-dwcmshc",
> > > +		.data = &sdhci_dwcmshc_th1520_pdata,
> > > +	},
> > >  	{
> > >  		.compatible = "rockchip,rk3588-dwcmshc",
> > >  		.data = &sdhci_dwcmshc_rk35xx_pdata,
> > > @@ -541,6 +844,39 @@ static int dwcmshc_probe(struct platform_device *pdev)
> > >  			goto err_clk;
> > >  	}
> > >  
> > > +	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> > > +
> > > +		priv->delay_line = DELAY_LINE_DEFAULT;
> > > +
> > > +		if (device_property_present(&pdev->dev, "non-removable"))
> > > +			priv->non_removable = 1;
> > > +		else
> > > +			priv->non_removable = 0;
> > > +
> > > +		if (device_property_present(&pdev->dev, "thead,pull-up"))
> > > +			priv->pull_up_en = 1;
> > > +		else
> > > +			priv->pull_up_en = 0;
> > > +
> > > +		if (device_property_present(&pdev->dev, "thead,io-fixed-1v8"))
> > > +			priv->io_fixed_1v8 = true;
> > > +		else
> > > +			priv->io_fixed_1v8 = false;
> > > +
> > > +		/*
> > > +		 * start_signal_voltage_switch() will try 3.3V first
> > > +		 * then 1.8V. Use SDHCI_SIGNALING_180 ranther than
> > > +		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> > > +		 * in sdhci_start_signal_voltage_switch().
> > > +		 */
> > > +		if(priv->io_fixed_1v8){
> > > +			host->flags &=~SDHCI_SIGNALING_330;
> > > +			host->flags |= SDHCI_SIGNALING_180;
> > > +		}
> > > +
> > > +		sdhci_enable_v4_mode(host);
> > > +	}
> > > +
> > >  #ifdef CONFIG_ACPI
> > >  	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> > >  		sdhci_enable_v4_mode(host);
> > > 
> > > -- 
> > > 2.34.1
> > > 
> 
> Thank you,
> Drew
> 
> [1] https://gist.github.com/pdp7/a8301b895c2dc293f2e0210e77e45e01
> [2] https://git.beagleboard.org/beaglev-ahead/beaglev-ahead-linux/-/blob/beaglev-v5.10.113-1.1.2/drivers/mmc/host/sdhci-of-dwcmshc.c#L238
> 

Re: [PATCH RFC v2 1/4] dt-bindings: mmc: sdhci-of-dwcmhsc: Add T-Head TH1520 support

[Krzysztof Kozlowski]

On 17/08/2023 00:26, Drew Fustini wrote:
> On Mon, Aug 07, 2023 at 08:29:21AM +0200, Krzysztof Kozlowski wrote:
>> On 05/08/2023 05:14, Drew Fustini wrote:
>>> Add compatible value for the T-Head TH1520 dwcmshc controller and
>>> thead,io-fixed-1v8 and thead,pull-up properties.
>>>
>>> Signed-off-by: Drew Fustini <dfustini@baylibre.com>
>>> ---
>>>  Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> index a43eb837f8da..57602c345cab 100644
>>> --- a/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> +++ b/Documentation/devicetree/bindings/mmc/snps,dwcmshc-sdhci.yaml
>>> @@ -19,6 +19,7 @@ properties:
>>>        - rockchip,rk3568-dwcmshc
>>>        - rockchip,rk3588-dwcmshc
>>>        - snps,dwcmshc-sdhci
>>> +      - thead,th1520-dwcmshc
>>>  
>>>    reg:
>>>      maxItems: 1
>>> @@ -60,6 +61,14 @@ properties:
>>>      description: Specify the number of delay for tx sampling.
>>>      $ref: /schemas/types.yaml#/definitions/uint8
>>>  
>>> +  thead,io-fixed-1v8:
>>> +    description: SoC PHY pad is fixed 1.8V
>>> +    type: boolean
>>
>> Isn't this duplicating existing properties for MMC modes with 1.8 V?
> 
> Thank you for reviewing. Yes, now that you mention it, I do see those
> properties now in mmc-controller.yaml. It seems like the existing
> mmc-ddr-1_8v property would be appropriate.
> 
>>
>>> +
>>> +  thead,pull-up:
>>> +    description: True if pull-up, false if pull-down
>>
>> This explains me nothing. No clue what you are pulling and why do you
>> need it. Pin pulls should be done via pin controller, not MMC.
> 
> Good point that my description is not helpful. The pull-up property
> determines whether certain phy registers are written to. I need to try
> to can get documentation on the phy so that I can better understand the
> details of the pull-up configuration in the phy registers.
> 
>>
>> Anyway you should have here allOf:if:then (move the allOf: from top to
>> behind "required:") which will disallow these properties for other variants.
> 
> I noticed that nvidia,tegra20-sdhci.yaml has several lines related to
> pull-up/down configuration:
> 
> 218   - if:
> 219       properties:
> 220         compatible:
> 221           contains:
> 222             const: nvidia,tegra210-sdhci
> 223     then:
> 224       properties:
> 225         pinctrl-names:
> 226           oneOf:
> 227             - items:
> 228                 - const: sdmmc-3v3
> 229                   description: pad configuration for 3.3 V
> 230                 - const: sdmmc-1v8
> 231                   description: pad configuration for 1.8 V
> 232                 - const: sdmmc-3v3-drv
> 233                   description: pull-up/down configuration for 3.3 V
> 234                 - const: sdmmc-1v8-drv
> 235                   description: pull-up/down configuration for 1.8 V
> 236             - items:
> 237                 - const: sdmmc-3v3-drv
> 238                   description: pull-up/down configuration for 3.3 V
> 239                 - const: sdmmc-1v8-drv
> 240                   description: pull-up/down configuration for 1.8 V
> 241             - items:
> 242                 - const: sdmmc-1v8-drv
> 243                   description: pull-up/down configuration for 1.8 V
> 
> Do you think creating something like that would be a good approach?

This depends. Does your driver implementation will make use of it? If
yes, then it makes sense.

Best regards,
Krzysztof

Re: [PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Jiexun Wang]

Hello,
I tested the patch on my LicheePi 4A board.
It can successfully boot with eMMC, but when I use the eMMC more frequently - for instance:

$ while true; do /bin/dd if=/dev/zero of=bigfile bs=1024000 count=1024; done &

I encounter the following error:

sbi_trap_error: hart1: illegal instruction handler failed (error -2)
sbi_trap_error: hart1: mcause=0x0000000000000002 mtval=0x0000000060e2de4f
sbi_trap_error: hart1: mepc=0x000000000001897c mstatus=0x0000000a00001820
sbi_trap_error: hart1: ra=0x00000000000170f8 sp=0x000000000004adc8
sbi_trap_error: hart1: gp=0xffffffff8136ea90 tp=0xffffffd900228000
sbi_trap_error: hart1: s0=0x0000000000000000 s1=0x000000000004ae08
sbi_trap_error: hart1: a0=0x000000003f9aa9bc a1=0x0000000000000004
sbi_trap_error: hart1: a2=0x0000000000000000 a3=0x0000000000000000
sbi_trap_error: hart1: a4=0x0000000000042248 a5=0x00000000000170e5
sbi_trap_error: hart1: a6=0x0000000000000000 a7=0x0000000054494d45
sbi_trap_error: hart1: s2=0x000000000004aee8 s3=0x0000000000000000
sbi_trap_error: hart1: s4=0x000000000004ae08 s5=0x0000000000000000
sbi_trap_error: hart1: s6=0xffffffff813aa240 s7=0x0000000000000080
sbi_trap_error: hart1: s8=0xffffffff80a1b5f0 s9=0x0000000000000000
sbi_trap_error: hart1: s10=0xffffffd9fef5d380 s11=0xffffffff81290a80
sbi_trap_error: hart1: t0=0x0000000a00000820 t1=0x0000000000000000
sbi_trap_error: hart1: t2=0xffffffff80c00318 t3=0x0000000000000001
sbi_trap_error: hart1: t4=0x0000000000000330 t5=0x0000000000000001
sbi_trap_error: hart1: t6=0x0000000000040000

My kernel version is v6.5-rc3.
My OpenSBI version is 1.3.
I tried to use other versions of OpenSBI, yet the problem persists. 
Is there a possibility of any underlying bug? Your insights into this would be greatly appreciated.

Thanks,
Jiexun Wang

Re: [PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Jisheng Zhang]

On Mon, Aug 28, 2023 at 12:40:16PM +0800, Jiexun Wang wrote:
> Hello,
> I tested the patch on my LicheePi 4A board.
> It can successfully boot with eMMC, but when I use the eMMC more frequently - for instance:
> 
> $ while true; do /bin/dd if=/dev/zero of=bigfile bs=1024000 count=1024; done &
> 
> I encounter the following error:
> 
> sbi_trap_error: hart1: illegal instruction handler failed (error -2)

> sbi_trap_error: hart1: mcause=0x0000000000000002 mtval=0x0000000060e2de4f
> sbi_trap_error: hart1: mepc=0x000000000001897c mstatus=0x0000000a00001820
> sbi_trap_error: hart1: ra=0x00000000000170f8 sp=0x000000000004adc8
> sbi_trap_error: hart1: gp=0xffffffff8136ea90 tp=0xffffffd900228000
> sbi_trap_error: hart1: s0=0x0000000000000000 s1=0x000000000004ae08
> sbi_trap_error: hart1: a0=0x000000003f9aa9bc a1=0x0000000000000004
> sbi_trap_error: hart1: a2=0x0000000000000000 a3=0x0000000000000000
> sbi_trap_error: hart1: a4=0x0000000000042248 a5=0x00000000000170e5
> sbi_trap_error: hart1: a6=0x0000000000000000 a7=0x0000000054494d45
> sbi_trap_error: hart1: s2=0x000000000004aee8 s3=0x0000000000000000
> sbi_trap_error: hart1: s4=0x000000000004ae08 s5=0x0000000000000000
> sbi_trap_error: hart1: s6=0xffffffff813aa240 s7=0x0000000000000080
> sbi_trap_error: hart1: s8=0xffffffff80a1b5f0 s9=0x0000000000000000
> sbi_trap_error: hart1: s10=0xffffffd9fef5d380 s11=0xffffffff81290a80
> sbi_trap_error: hart1: t0=0x0000000a00000820 t1=0x0000000000000000
> sbi_trap_error: hart1: t2=0xffffffff80c00318 t3=0x0000000000000001
> sbi_trap_error: hart1: t4=0x0000000000000330 t5=0x0000000000000001
> sbi_trap_error: hart1: t6=0x0000000000040000
> 
> My kernel version is v6.5-rc3.
> My OpenSBI version is 1.3.
> I tried to use other versions of OpenSBI, yet the problem persists. 
> Is there a possibility of any underlying bug? Your insights into this would be greatly appreciated.


Can you plz try below opensbi?
https://github.com/xhackerustc/thead-opensbi.git

Re: [PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Jiexun Wang]

Date: Mon, 28 Aug 2023 23:05:35 +0800, Jisheng Zhang wrote:
>On Mon, Aug 28, 2023 at 12:40:16PM +0800, Jiexun Wang wrote:
>> Hello,
>> I tested the patch on my LicheePi 4A board.
>> It can successfully boot with eMMC, but when I use the eMMC more frequently - for instance:
>> 
>> $ while true; do /bin/dd if=/dev/zero of=bigfile bs=1024000 count=1024; done &
>> 
>> I encounter the following error:
>> 
>> sbi_trap_error: hart1: illegal instruction handler failed (error -2)
>
>> sbi_trap_error: hart1: mcause=0x0000000000000002 mtval=0x0000000060e2de4f
>> sbi_trap_error: hart1: mepc=0x000000000001897c mstatus=0x0000000a00001820
>> sbi_trap_error: hart1: ra=0x00000000000170f8 sp=0x000000000004adc8
>> sbi_trap_error: hart1: gp=0xffffffff8136ea90 tp=0xffffffd900228000
>> sbi_trap_error: hart1: s0=0x0000000000000000 s1=0x000000000004ae08
>> sbi_trap_error: hart1: a0=0x000000003f9aa9bc a1=0x0000000000000004
>> sbi_trap_error: hart1: a2=0x0000000000000000 a3=0x0000000000000000
>> sbi_trap_error: hart1: a4=0x0000000000042248 a5=0x00000000000170e5
>> sbi_trap_error: hart1: a6=0x0000000000000000 a7=0x0000000054494d45
>> sbi_trap_error: hart1: s2=0x000000000004aee8 s3=0x0000000000000000
>> sbi_trap_error: hart1: s4=0x000000000004ae08 s5=0x0000000000000000
>> sbi_trap_error: hart1: s6=0xffffffff813aa240 s7=0x0000000000000080
>> sbi_trap_error: hart1: s8=0xffffffff80a1b5f0 s9=0x0000000000000000
>> sbi_trap_error: hart1: s10=0xffffffd9fef5d380 s11=0xffffffff81290a80
>> sbi_trap_error: hart1: t0=0x0000000a00000820 t1=0x0000000000000000
>> sbi_trap_error: hart1: t2=0xffffffff80c00318 t3=0x0000000000000001
>> sbi_trap_error: hart1: t4=0x0000000000000330 t5=0x0000000000000001
>> sbi_trap_error: hart1: t6=0x0000000000040000
>> 
>> My kernel version is v6.5-rc3.
>> My OpenSBI version is 1.3.
>> I tried to use other versions of OpenSBI, yet the problem persists. 
>> Is there a possibility of any underlying bug? Your insights into this would be greatly appreciated.
>
>
>Can you plz try below opensbi?

I tried the OpenSBI you provided and the issue didn't recur.
I conducted stress test about 30 minutes and the system appears to be functioning very well.
Thank you so much for helping me resolve this problem.

Best regards,
Jiexun Wang

Re: [PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Drew Fustini]

On Tue, Aug 29, 2023 at 09:56:47AM +0800, Jiexun Wang wrote:
> Date: Mon, 28 Aug 2023 23:05:35 +0800, Jisheng Zhang wrote:
> >On Mon, Aug 28, 2023 at 12:40:16PM +0800, Jiexun Wang wrote:
> >> Hello,
> >> I tested the patch on my LicheePi 4A board.
> >> It can successfully boot with eMMC, but when I use the eMMC more frequently - for instance:
> >> 
> >> $ while true; do /bin/dd if=/dev/zero of=bigfile bs=1024000 count=1024; done &
> >> 
> >> I encounter the following error:
> >> 
> >> sbi_trap_error: hart1: illegal instruction handler failed (error -2)
> >
> >> sbi_trap_error: hart1: mcause=0x0000000000000002 mtval=0x0000000060e2de4f
> >> sbi_trap_error: hart1: mepc=0x000000000001897c mstatus=0x0000000a00001820
> >> sbi_trap_error: hart1: ra=0x00000000000170f8 sp=0x000000000004adc8
> >> sbi_trap_error: hart1: gp=0xffffffff8136ea90 tp=0xffffffd900228000
> >> sbi_trap_error: hart1: s0=0x0000000000000000 s1=0x000000000004ae08
> >> sbi_trap_error: hart1: a0=0x000000003f9aa9bc a1=0x0000000000000004
> >> sbi_trap_error: hart1: a2=0x0000000000000000 a3=0x0000000000000000
> >> sbi_trap_error: hart1: a4=0x0000000000042248 a5=0x00000000000170e5
> >> sbi_trap_error: hart1: a6=0x0000000000000000 a7=0x0000000054494d45
> >> sbi_trap_error: hart1: s2=0x000000000004aee8 s3=0x0000000000000000
> >> sbi_trap_error: hart1: s4=0x000000000004ae08 s5=0x0000000000000000
> >> sbi_trap_error: hart1: s6=0xffffffff813aa240 s7=0x0000000000000080
> >> sbi_trap_error: hart1: s8=0xffffffff80a1b5f0 s9=0x0000000000000000
> >> sbi_trap_error: hart1: s10=0xffffffd9fef5d380 s11=0xffffffff81290a80
> >> sbi_trap_error: hart1: t0=0x0000000a00000820 t1=0x0000000000000000
> >> sbi_trap_error: hart1: t2=0xffffffff80c00318 t3=0x0000000000000001
> >> sbi_trap_error: hart1: t4=0x0000000000000330 t5=0x0000000000000001
> >> sbi_trap_error: hart1: t6=0x0000000000040000
> >> 
> >> My kernel version is v6.5-rc3.
> >> My OpenSBI version is 1.3.
> >> I tried to use other versions of OpenSBI, yet the problem persists. 
> >> Is there a possibility of any underlying bug? Your insights into this would be greatly appreciated.
> >
> >
> >Can you plz try below opensbi?
> 
> I tried the OpenSBI you provided and the issue didn't recur.
> I conducted stress test about 30 minutes and the system appears to be functioning very well.
> Thank you so much for helping me resolve this problem.

That's great!

Jisheng - are these the commits that fix the error?

d98da90a19b5 ("lib: sbi_illegal_insn: Fix FENCE.TSO emulation infinite trap loop")
39d1e698c975 ("lib: sbi_illegal_insn: Add emulation for fence.tso")

thanks,
drew

Re: [PATCH RFC v2 0/4] RISC-V: Add basic eMMC support for BeagleV Ahead

[Jisheng Zhang]

On Tue, Aug 29, 2023 at 08:19:53AM -0700, Drew Fustini wrote:
> On Tue, Aug 29, 2023 at 09:56:47AM +0800, Jiexun Wang wrote:
> > Date: Mon, 28 Aug 2023 23:05:35 +0800, Jisheng Zhang wrote:
> > >On Mon, Aug 28, 2023 at 12:40:16PM +0800, Jiexun Wang wrote:
> > >> Hello,
> > >> I tested the patch on my LicheePi 4A board.
> > >> It can successfully boot with eMMC, but when I use the eMMC more frequently - for instance:
> > >> 
> > >> $ while true; do /bin/dd if=/dev/zero of=bigfile bs=1024000 count=1024; done &
> > >> 
> > >> I encounter the following error:
> > >> 
> > >> sbi_trap_error: hart1: illegal instruction handler failed (error -2)
> > >
> > >> sbi_trap_error: hart1: mcause=0x0000000000000002 mtval=0x0000000060e2de4f
> > >> sbi_trap_error: hart1: mepc=0x000000000001897c mstatus=0x0000000a00001820
> > >> sbi_trap_error: hart1: ra=0x00000000000170f8 sp=0x000000000004adc8
> > >> sbi_trap_error: hart1: gp=0xffffffff8136ea90 tp=0xffffffd900228000
> > >> sbi_trap_error: hart1: s0=0x0000000000000000 s1=0x000000000004ae08
> > >> sbi_trap_error: hart1: a0=0x000000003f9aa9bc a1=0x0000000000000004
> > >> sbi_trap_error: hart1: a2=0x0000000000000000 a3=0x0000000000000000
> > >> sbi_trap_error: hart1: a4=0x0000000000042248 a5=0x00000000000170e5
> > >> sbi_trap_error: hart1: a6=0x0000000000000000 a7=0x0000000054494d45
> > >> sbi_trap_error: hart1: s2=0x000000000004aee8 s3=0x0000000000000000
> > >> sbi_trap_error: hart1: s4=0x000000000004ae08 s5=0x0000000000000000
> > >> sbi_trap_error: hart1: s6=0xffffffff813aa240 s7=0x0000000000000080
> > >> sbi_trap_error: hart1: s8=0xffffffff80a1b5f0 s9=0x0000000000000000
> > >> sbi_trap_error: hart1: s10=0xffffffd9fef5d380 s11=0xffffffff81290a80
> > >> sbi_trap_error: hart1: t0=0x0000000a00000820 t1=0x0000000000000000
> > >> sbi_trap_error: hart1: t2=0xffffffff80c00318 t3=0x0000000000000001
> > >> sbi_trap_error: hart1: t4=0x0000000000000330 t5=0x0000000000000001
> > >> sbi_trap_error: hart1: t6=0x0000000000040000
> > >> 
> > >> My kernel version is v6.5-rc3.
> > >> My OpenSBI version is 1.3.
> > >> I tried to use other versions of OpenSBI, yet the problem persists. 
> > >> Is there a possibility of any underlying bug? Your insights into this would be greatly appreciated.
> > >
> > >
> > >Can you plz try below opensbi?
> > 
> > I tried the OpenSBI you provided and the issue didn't recur.
> > I conducted stress test about 30 minutes and the system appears to be functioning very well.
> > Thank you so much for helping me resolve this problem.
> 
> That's great!
> 
> Jisheng - are these the commits that fix the error?
> 
> d98da90a19b5 ("lib: sbi_illegal_insn: Fix FENCE.TSO emulation infinite trap loop")
> 39d1e698c975 ("lib: sbi_illegal_insn: Add emulation for fence.tso")
> 

These two commits have been in upstream OpenSBI, so there
must be something to be patched/fixed in the upstream OpenSBI, but I have not
yet bisected the upstream OpenSBI to find the root cause.
This issue is on my TODO list.