[PATCH v2 0/2] Add mtk-sd support for MT8196

[PATCH v2 0/2] Add mtk-sd support for MT8196

[Andy-ld Lu]

There are some new features for Mediatek SoC MT8196, which include new
command/data transmitting and receiving path (abbreviated as tx/rx), and
two modified register settings.

The driver code has to be adapted to implement the above changes, and the
compatible string 'mediatek,mt8196-mmc' is added to driver and devicetree
bindings.

---
Changes in v2:
- Use compatible string 'mediatek,mt8196-mmc' to replace 'mediatek,msdc-v2';
- Remove the 'mediatek,stop-dly-sel', 'mediatek,pop-en-cnt' and 'mediatek,
  prohibit-gate-cg' in devicetree bindings, due to SoC dependent;
- Add 'stop_dly_sel' and 'pop_en_cnt' to the compatiblity structure for
  different register settings;
- The SoC's upgraded version would discard the bus design that detect source
  clock CG when the CPU access the IP registers, so drop the related control
  flow with 'prohibit_gate_cg' flag.

Link to v1:
https://patchwork.kernel.org/patch/13812924

---
Andy-ld Lu (2):
  mmc: mtk-sd: Add support for MT8196
  dt-bindings: mmc: mtk-sd: Add support for MT8196

 .../devicetree/bindings/mmc/mtk-sd.yaml       |   2 +
 drivers/mmc/host/mtk-sd.c                     | 162 +++++++++++++++---
 2 files changed, 143 insertions(+), 21 deletions(-)

-- 
2.46.0

[PATCH v2 1/2] mmc: mtk-sd: Add support for MT8196

[Andy-ld Lu]

Mediatek SoC MT8196 features a new design for tx/rx path. The new tx
path incorporates register settings that are closely associated with
bus timing. And the difference between new rx path and older versions
is the usage of distinct register bits when setting the data sampling
edge as part of the tuning process.

Besides, there are modified register settings for STOP_DLY_SEL and
POP_EN_CNT, with two new fields added to the compatibility structure
to reflect the modifications.

For the changes mentioned in relation to the MT8196, the new compatible
string 'mediatek,mt8196-mmc' is introduced. This is to accommodate
different settings and workflows specific to the MT8196.

Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
---
 drivers/mmc/host/mtk-sd.c | 162 +++++++++++++++++++++++++++++++++-----
 1 file changed, 141 insertions(+), 21 deletions(-)

diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
index 89018b6c97b9..4254b3012aeb 100644
--- a/drivers/mmc/host/mtk-sd.c
+++ b/drivers/mmc/host/mtk-sd.c
@@ -65,6 +65,7 @@
 #define SDC_RESP3        0x4c
 #define SDC_BLK_NUM      0x50
 #define SDC_ADV_CFG0     0x64
+#define MSDC_NEW_RX_CFG  0x68
 #define EMMC_IOCON       0x7c
 #define SDC_ACMD_RESP    0x80
 #define DMA_SA_H4BIT     0x8c
@@ -91,6 +92,7 @@
 #define EMMC_TOP_CONTROL	0x00
 #define EMMC_TOP_CMD		0x04
 #define EMMC50_PAD_DS_TUNE	0x0c
+#define LOOP_TEST_CONTROL	0x30
 
 /*--------------------------------------------------------------------------*/
 /* Register Mask                                                            */
@@ -202,9 +204,13 @@
 #define SDC_STS_CMDBUSY         BIT(1)	/* RW */
 #define SDC_STS_SWR_COMPL       BIT(31)	/* RW */
 
-#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
 /* SDC_ADV_CFG0 mask */
+#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
 #define SDC_RX_ENHANCE_EN	BIT(20)	/* RW */
+#define SDC_NEW_TX_EN		BIT(31)	/* RW */
+
+/* MSDC_NEW_RX_CFG mask */
+#define MSDC_NEW_RX_PATH_SEL	BIT(0)	/* RW */
 
 /* DMA_SA_H4BIT mask */
 #define DMA_ADDR_HIGH_4BIT      GENMASK(3, 0)	/* RW */
@@ -226,6 +232,7 @@
 
 /* MSDC_PATCH_BIT mask */
 #define MSDC_PATCH_BIT_ODDSUPP    BIT(1)	/* RW */
+#define MSDC_PATCH_BIT_RD_DAT_SEL BIT(3)	/* RW */
 #define MSDC_INT_DAT_LATCH_CK_SEL GENMASK(9, 7)
 #define MSDC_CKGEN_MSDC_DLY_SEL   GENMASK(14, 10)
 #define MSDC_PATCH_BIT_IODSSEL    BIT(16)	/* RW */
@@ -247,6 +254,8 @@
 #define MSDC_PB2_SUPPORT_64G      BIT(1)    /* RW */
 #define MSDC_PB2_RESPWAIT         GENMASK(3, 2)   /* RW */
 #define MSDC_PB2_RESPSTSENSEL     GENMASK(18, 16) /* RW */
+#define MSDC_PB2_POP_EN_CNT       GENMASK(23, 20) /* RW */
+#define MSDC_PB2_CFGCRCSTSEDGE    BIT(25)   /* RW */
 #define MSDC_PB2_CRCSTSENSEL      GENMASK(31, 29) /* RW */
 
 #define MSDC_PAD_TUNE_DATWRDLY	  GENMASK(4, 0)		/* RW */
@@ -311,6 +320,12 @@
 #define PAD_DS_DLY1		GENMASK(14, 10)	/* RW */
 #define PAD_DS_DLY3		GENMASK(4, 0)	/* RW */
 
+/* LOOP_TEST_CONTROL mask */
+#define TEST_LOOP_DSCLK_MUX_SEL        BIT(0)	/* RW */
+#define TEST_LOOP_LATCH_MUX_SEL        BIT(1)	/* RW */
+#define LOOP_EN_SEL_CLK                BIT(20)	/* RW */
+#define TEST_HS400_CMD_LOOP_MUX_SEL    BIT(31)	/* RW */
+
 #define REQ_CMD_EIO  BIT(0)
 #define REQ_CMD_TMO  BIT(1)
 #define REQ_DAT_ERR  BIT(2)
@@ -391,6 +406,7 @@ struct msdc_save_para {
 	u32 emmc_top_control;
 	u32 emmc_top_cmd;
 	u32 emmc50_pad_ds_tune;
+	u32 loop_test_control;
 };
 
 struct mtk_mmc_compatible {
@@ -402,9 +418,13 @@ struct mtk_mmc_compatible {
 	bool data_tune;
 	bool busy_check;
 	bool stop_clk_fix;
+	u8 stop_dly_sel;
+	u8 pop_en_cnt;
 	bool enhance_rx;
 	bool support_64g;
 	bool use_internal_cd;
+	bool support_new_tx;
+	bool support_new_rx;
 };
 
 struct msdc_tune_para {
@@ -621,6 +641,23 @@ static const struct mtk_mmc_compatible mt8516_compat = {
 	.stop_clk_fix = true,
 };
 
+static const struct mtk_mmc_compatible mt8196_compat = {
+	.clk_div_bits = 12,
+	.recheck_sdio_irq = false,
+	.hs400_tune = false,
+	.pad_tune_reg = MSDC_PAD_TUNE0,
+	.async_fifo = true,
+	.data_tune = true,
+	.busy_check = true,
+	.stop_clk_fix = true,
+	.stop_dly_sel = 1,
+	.pop_en_cnt = 2,
+	.enhance_rx = true,
+	.support_64g = true,
+	.support_new_tx = true,
+	.support_new_rx = true,
+};
+
 static const struct of_device_id msdc_of_ids[] = {
 	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
 	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
@@ -632,6 +669,7 @@ static const struct of_device_id msdc_of_ids[] = {
 	{ .compatible = "mediatek,mt8135-mmc", .data = &mt8135_compat},
 	{ .compatible = "mediatek,mt8173-mmc", .data = &mt8173_compat},
 	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
+	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
 	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
 
 	{}
@@ -872,6 +910,42 @@ static int msdc_ungate_clock(struct msdc_host *host)
 				  (val & MSDC_CFG_CKSTB), 1, 20000);
 }
 
+static void msdc_new_tx_setting(struct msdc_host *host)
+{
+	if (host->top_base) {
+		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
+			     TEST_LOOP_DSCLK_MUX_SEL);
+		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
+			     TEST_LOOP_LATCH_MUX_SEL);
+		sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
+			     TEST_HS400_CMD_LOOP_MUX_SEL);
+	}
+
+	switch (host->timing) {
+	case MMC_TIMING_LEGACY:
+	case MMC_TIMING_MMC_HS:
+	case MMC_TIMING_SD_HS:
+	case MMC_TIMING_UHS_SDR12:
+	case MMC_TIMING_UHS_SDR25:
+	case MMC_TIMING_UHS_DDR50:
+	case MMC_TIMING_MMC_DDR52:
+		if (host->top_base)
+			sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
+				     LOOP_EN_SEL_CLK);
+		break;
+	case MMC_TIMING_UHS_SDR50:
+	case MMC_TIMING_UHS_SDR104:
+	case MMC_TIMING_MMC_HS200:
+	case MMC_TIMING_MMC_HS400:
+		if (host->top_base)
+			sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
+				     LOOP_EN_SEL_CLK);
+		break;
+	default:
+		break;
+	}
+}
+
 static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
 {
 	struct mmc_host *mmc = mmc_from_priv(host);
@@ -881,6 +955,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
 	u32 sclk;
 	u32 tune_reg = host->dev_comp->pad_tune_reg;
 	u32 val;
+	bool timing_changed = false;
 
 	if (!hz) {
 		dev_dbg(host->dev, "set mclk to 0\n");
@@ -890,6 +965,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
 		return;
 	}
 
+	if (host->timing != timing)
+		timing_changed = true;
+
 	flags = readl(host->base + MSDC_INTEN);
 	sdr_clr_bits(host->base + MSDC_INTEN, flags);
 	if (host->dev_comp->clk_div_bits == 8)
@@ -996,6 +1074,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
 		sdr_set_field(host->base + tune_reg,
 			      MSDC_PAD_TUNE_CMDRRDLY,
 			      host->hs400_cmd_int_delay);
+	if (timing_changed && host->dev_comp->support_new_tx)
+		msdc_new_tx_setting(host);
+
 	dev_dbg(host->dev, "sclk: %d, timing: %d\n", mmc->actual_clock,
 		timing);
 }
@@ -1704,6 +1785,17 @@ static void msdc_init_hw(struct msdc_host *host)
 		reset_control_deassert(host->reset);
 	}
 
+	/* New tx/rx enable bit need to be 0->1 for hardware check */
+	if (host->dev_comp->support_new_tx) {
+		sdr_clr_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
+		sdr_set_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
+		msdc_new_tx_setting(host);
+	}
+	if (host->dev_comp->support_new_rx) {
+		sdr_clr_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
+		sdr_set_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
+	}
+
 	/* Configure to MMC/SD mode, clock free running */
 	sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE | MSDC_CFG_CKPDN);
 
@@ -1742,8 +1834,19 @@ static void msdc_init_hw(struct msdc_host *host)
 	sdr_set_bits(host->base + EMMC50_CFG0, EMMC50_CFG_CFCSTS_SEL);
 
 	if (host->dev_comp->stop_clk_fix) {
-		sdr_set_field(host->base + MSDC_PATCH_BIT1,
-			      MSDC_PATCH_BIT1_STOP_DLY, 3);
+		if (host->dev_comp->stop_dly_sel)
+			sdr_set_field(host->base + MSDC_PATCH_BIT1,
+				      MSDC_PATCH_BIT1_STOP_DLY,
+				      host->dev_comp->stop_dly_sel & 0xf);
+		else
+			sdr_set_field(host->base + MSDC_PATCH_BIT1,
+				      MSDC_PATCH_BIT1_STOP_DLY, 3);
+
+		if (host->dev_comp->pop_en_cnt)
+			sdr_set_field(host->base + MSDC_PATCH_BIT2,
+				      MSDC_PB2_POP_EN_CNT,
+				      host->dev_comp->pop_en_cnt & 0xf);
+
 		sdr_clr_bits(host->base + SDC_FIFO_CFG,
 			     SDC_FIFO_CFG_WRVALIDSEL);
 		sdr_clr_bits(host->base + SDC_FIFO_CFG,
@@ -2055,6 +2158,19 @@ static inline void msdc_set_data_delay(struct msdc_host *host, u32 value)
 	}
 }
 
+static inline void msdc_set_data_sample_edge(struct msdc_host *host, bool rising)
+{
+	u32 value = rising ? 0 : 1;
+
+	if (host->dev_comp->support_new_rx) {
+		sdr_set_field(host->base + MSDC_PATCH_BIT, MSDC_PATCH_BIT_RD_DAT_SEL, value);
+		sdr_set_field(host->base + MSDC_PATCH_BIT2, MSDC_PB2_CFGCRCSTSEDGE, value);
+	} else {
+		sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_DSPL, value);
+		sdr_set_field(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL, value);
+	}
+}
+
 static int msdc_tune_response(struct mmc_host *mmc, u32 opcode)
 {
 	struct msdc_host *host = mmc_priv(mmc);
@@ -2210,8 +2326,7 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode)
 
 	sdr_set_field(host->base + MSDC_PATCH_BIT, MSDC_INT_DAT_LATCH_CK_SEL,
 		      host->latch_ck);
-	sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL);
-	sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL);
+	msdc_set_data_sample_edge(host, true);
 	for (i = 0; i < host->tuning_step; i++) {
 		msdc_set_data_delay(host, i);
 		ret = mmc_send_tuning(mmc, opcode, NULL);
@@ -2224,8 +2339,7 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode)
 	    (final_rise_delay.start == 0 && final_rise_delay.maxlen >= 4))
 		goto skip_fall;
 
-	sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL);
-	sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL);
+	msdc_set_data_sample_edge(host, false);
 	for (i = 0; i < host->tuning_step; i++) {
 		msdc_set_data_delay(host, i);
 		ret = mmc_send_tuning(mmc, opcode, NULL);
@@ -2237,12 +2351,10 @@ static int msdc_tune_data(struct mmc_host *mmc, u32 opcode)
 skip_fall:
 	final_maxlen = max(final_rise_delay.maxlen, final_fall_delay.maxlen);
 	if (final_maxlen == final_rise_delay.maxlen) {
-		sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL);
-		sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL);
+		msdc_set_data_sample_edge(host, true);
 		final_delay = final_rise_delay.final_phase;
 	} else {
-		sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_DSPL);
-		sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_W_DSPL);
+		msdc_set_data_sample_edge(host, false);
 		final_delay = final_fall_delay.final_phase;
 	}
 	msdc_set_data_delay(host, final_delay);
@@ -2267,8 +2379,7 @@ static int msdc_tune_together(struct mmc_host *mmc, u32 opcode)
 		      host->latch_ck);
 
 	sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
-	sdr_clr_bits(host->base + MSDC_IOCON,
-		     MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL);
+	msdc_set_data_sample_edge(host, true);
 	for (i = 0; i < host->tuning_step; i++) {
 		msdc_set_cmd_delay(host, i);
 		msdc_set_data_delay(host, i);
@@ -2283,8 +2394,7 @@ static int msdc_tune_together(struct mmc_host *mmc, u32 opcode)
 		goto skip_fall;
 
 	sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
-	sdr_set_bits(host->base + MSDC_IOCON,
-		     MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL);
+	msdc_set_data_sample_edge(host, false);
 	for (i = 0; i < host->tuning_step; i++) {
 		msdc_set_cmd_delay(host, i);
 		msdc_set_data_delay(host, i);
@@ -2298,13 +2408,11 @@ static int msdc_tune_together(struct mmc_host *mmc, u32 opcode)
 	final_maxlen = max(final_rise_delay.maxlen, final_fall_delay.maxlen);
 	if (final_maxlen == final_rise_delay.maxlen) {
 		sdr_clr_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
-		sdr_clr_bits(host->base + MSDC_IOCON,
-			     MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL);
+		msdc_set_data_sample_edge(host, true);
 		final_delay = final_rise_delay.final_phase;
 	} else {
 		sdr_set_bits(host->base + MSDC_IOCON, MSDC_IOCON_RSPL);
-		sdr_set_bits(host->base + MSDC_IOCON,
-			     MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL);
+		msdc_set_data_sample_edge(host, false);
 		final_delay = final_fall_delay.final_phase;
 	}
 
@@ -2324,8 +2432,7 @@ static int msdc_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	if (host->dev_comp->data_tune && host->dev_comp->async_fifo) {
 		ret = msdc_tune_together(mmc, opcode);
 		if (host->hs400_mode) {
-			sdr_clr_bits(host->base + MSDC_IOCON,
-				     MSDC_IOCON_DSPL | MSDC_IOCON_W_DSPL);
+			msdc_set_data_sample_edge(host, true);
 			msdc_set_data_delay(host, 0);
 		}
 		goto tune_done;
@@ -2995,6 +3102,8 @@ static void msdc_save_reg(struct msdc_host *host)
 			readl(host->top_base + EMMC_TOP_CMD);
 		host->save_para.emmc50_pad_ds_tune =
 			readl(host->top_base + EMMC50_PAD_DS_TUNE);
+		host->save_para.loop_test_control =
+			readl(host->top_base + LOOP_TEST_CONTROL);
 	} else {
 		host->save_para.pad_tune = readl(host->base + tune_reg);
 	}
@@ -3005,6 +3114,15 @@ static void msdc_restore_reg(struct msdc_host *host)
 	struct mmc_host *mmc = mmc_from_priv(host);
 	u32 tune_reg = host->dev_comp->pad_tune_reg;
 
+	if (host->dev_comp->support_new_tx) {
+		sdr_clr_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
+		sdr_set_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
+	}
+	if (host->dev_comp->support_new_rx) {
+		sdr_clr_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
+		sdr_set_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
+	}
+
 	writel(host->save_para.msdc_cfg, host->base + MSDC_CFG);
 	writel(host->save_para.iocon, host->base + MSDC_IOCON);
 	writel(host->save_para.sdc_cfg, host->base + SDC_CFG);
@@ -3023,6 +3141,8 @@ static void msdc_restore_reg(struct msdc_host *host)
 		       host->top_base + EMMC_TOP_CMD);
 		writel(host->save_para.emmc50_pad_ds_tune,
 		       host->top_base + EMMC50_PAD_DS_TUNE);
+		writel(host->save_para.loop_test_control,
+		       host->top_base + LOOP_TEST_CONTROL);
 	} else {
 		writel(host->save_para.pad_tune, host->base + tune_reg);
 	}
-- 
2.46.0

Re: [PATCH v2 1/2] mmc: mtk-sd: Add support for MT8196

[AngeloGioacchino Del Regno]

Il 29/09/24 09:44, Andy-ld Lu ha scritto:
> Mediatek SoC MT8196 features a new design for tx/rx path. The new tx
> path incorporates register settings that are closely associated with
> bus timing. And the difference between new rx path and older versions
> is the usage of distinct register bits when setting the data sampling
> edge as part of the tuning process.
> 
> Besides, there are modified register settings for STOP_DLY_SEL and
> POP_EN_CNT, with two new fields added to the compatibility structure
> to reflect the modifications.
> 
> For the changes mentioned in relation to the MT8196, the new compatible
> string 'mediatek,mt8196-mmc' is introduced. This is to accommodate
> different settings and workflows specific to the MT8196.
> 
> Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
> ---
>   drivers/mmc/host/mtk-sd.c | 162 +++++++++++++++++++++++++++++++++-----
>   1 file changed, 141 insertions(+), 21 deletions(-)
> 
> diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> index 89018b6c97b9..4254b3012aeb 100644
> --- a/drivers/mmc/host/mtk-sd.c
> +++ b/drivers/mmc/host/mtk-sd.c
> @@ -65,6 +65,7 @@
>   #define SDC_RESP3        0x4c
>   #define SDC_BLK_NUM      0x50
>   #define SDC_ADV_CFG0     0x64
> +#define MSDC_NEW_RX_CFG  0x68
>   #define EMMC_IOCON       0x7c
>   #define SDC_ACMD_RESP    0x80
>   #define DMA_SA_H4BIT     0x8c
> @@ -91,6 +92,7 @@
>   #define EMMC_TOP_CONTROL	0x00
>   #define EMMC_TOP_CMD		0x04
>   #define EMMC50_PAD_DS_TUNE	0x0c
> +#define LOOP_TEST_CONTROL	0x30
>   
>   /*--------------------------------------------------------------------------*/
>   /* Register Mask                                                            */
> @@ -202,9 +204,13 @@
>   #define SDC_STS_CMDBUSY         BIT(1)	/* RW */
>   #define SDC_STS_SWR_COMPL       BIT(31)	/* RW */
>   
> -#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
>   /* SDC_ADV_CFG0 mask */
> +#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
>   #define SDC_RX_ENHANCE_EN	BIT(20)	/* RW */
> +#define SDC_NEW_TX_EN		BIT(31)	/* RW */
> +
> +/* MSDC_NEW_RX_CFG mask */
> +#define MSDC_NEW_RX_PATH_SEL	BIT(0)	/* RW */
>   
>   /* DMA_SA_H4BIT mask */
>   #define DMA_ADDR_HIGH_4BIT      GENMASK(3, 0)	/* RW */
> @@ -226,6 +232,7 @@
>   
>   /* MSDC_PATCH_BIT mask */
>   #define MSDC_PATCH_BIT_ODDSUPP    BIT(1)	/* RW */
> +#define MSDC_PATCH_BIT_RD_DAT_SEL BIT(3)	/* RW */
>   #define MSDC_INT_DAT_LATCH_CK_SEL GENMASK(9, 7)
>   #define MSDC_CKGEN_MSDC_DLY_SEL   GENMASK(14, 10)
>   #define MSDC_PATCH_BIT_IODSSEL    BIT(16)	/* RW */
> @@ -247,6 +254,8 @@
>   #define MSDC_PB2_SUPPORT_64G      BIT(1)    /* RW */
>   #define MSDC_PB2_RESPWAIT         GENMASK(3, 2)   /* RW */
>   #define MSDC_PB2_RESPSTSENSEL     GENMASK(18, 16) /* RW */
> +#define MSDC_PB2_POP_EN_CNT       GENMASK(23, 20) /* RW */
> +#define MSDC_PB2_CFGCRCSTSEDGE    BIT(25)   /* RW */
>   #define MSDC_PB2_CRCSTSENSEL      GENMASK(31, 29) /* RW */
>   
>   #define MSDC_PAD_TUNE_DATWRDLY	  GENMASK(4, 0)		/* RW */
> @@ -311,6 +320,12 @@
>   #define PAD_DS_DLY1		GENMASK(14, 10)	/* RW */
>   #define PAD_DS_DLY3		GENMASK(4, 0)	/* RW */
>   
> +/* LOOP_TEST_CONTROL mask */
> +#define TEST_LOOP_DSCLK_MUX_SEL        BIT(0)	/* RW */
> +#define TEST_LOOP_LATCH_MUX_SEL        BIT(1)	/* RW */
> +#define LOOP_EN_SEL_CLK                BIT(20)	/* RW */
> +#define TEST_HS400_CMD_LOOP_MUX_SEL    BIT(31)	/* RW */
> +
>   #define REQ_CMD_EIO  BIT(0)
>   #define REQ_CMD_TMO  BIT(1)
>   #define REQ_DAT_ERR  BIT(2)
> @@ -391,6 +406,7 @@ struct msdc_save_para {
>   	u32 emmc_top_control;
>   	u32 emmc_top_cmd;
>   	u32 emmc50_pad_ds_tune;
> +	u32 loop_test_control;
>   };
>   
>   struct mtk_mmc_compatible {
> @@ -402,9 +418,13 @@ struct mtk_mmc_compatible {
>   	bool data_tune;
>   	bool busy_check;
>   	bool stop_clk_fix;
> +	u8 stop_dly_sel;
> +	u8 pop_en_cnt;
>   	bool enhance_rx;
>   	bool support_64g;
>   	bool use_internal_cd;
> +	bool support_new_tx;
> +	bool support_new_rx;

Is there really any platform that supports new_tx but *not* new_rx, or vice-versa?

If not, you can add just one `bool support_new_rx_tx` member to this structure.

>   };
>   
>   struct msdc_tune_para {
> @@ -621,6 +641,23 @@ static const struct mtk_mmc_compatible mt8516_compat = {
>   	.stop_clk_fix = true,
>   };
>   
> +static const struct mtk_mmc_compatible mt8196_compat = {
> +	.clk_div_bits = 12,
> +	.recheck_sdio_irq = false,
> +	.hs400_tune = false,
> +	.pad_tune_reg = MSDC_PAD_TUNE0,
> +	.async_fifo = true,
> +	.data_tune = true,
> +	.busy_check = true,
> +	.stop_clk_fix = true,
> +	.stop_dly_sel = 1,
> +	.pop_en_cnt = 2,
> +	.enhance_rx = true,
> +	.support_64g = true,
> +	.support_new_tx = true,
> +	.support_new_rx = true,
> +};
> +
>   static const struct of_device_id msdc_of_ids[] = {
>   	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
>   	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> @@ -632,6 +669,7 @@ static const struct of_device_id msdc_of_ids[] = {
>   	{ .compatible = "mediatek,mt8135-mmc", .data = &mt8135_compat},
>   	{ .compatible = "mediatek,mt8173-mmc", .data = &mt8173_compat},
>   	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
> +	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
>   	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
>   
>   	{}
> @@ -872,6 +910,42 @@ static int msdc_ungate_clock(struct msdc_host *host)
>   				  (val & MSDC_CFG_CKSTB), 1, 20000);
>   }
>   
> +static void msdc_new_tx_setting(struct msdc_host *host)
> +{

That's simpler:

	if (!host->top_base)
		return;

> +	if (host->top_base) {
> +		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> +			     TEST_LOOP_DSCLK_MUX_SEL);
> +		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> +			     TEST_LOOP_LATCH_MUX_SEL);
> +		sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
> +			     TEST_HS400_CMD_LOOP_MUX_SEL);
> +	}
> +
> +	switch (host->timing) {
> +	case MMC_TIMING_LEGACY:
> +	case MMC_TIMING_MMC_HS:
> +	case MMC_TIMING_SD_HS:
> +	case MMC_TIMING_UHS_SDR12:
> +	case MMC_TIMING_UHS_SDR25:
> +	case MMC_TIMING_UHS_DDR50:
> +	case MMC_TIMING_MMC_DDR52:
> +		if (host->top_base)
> +			sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
> +				     LOOP_EN_SEL_CLK);
> +		break;
> +	case MMC_TIMING_UHS_SDR50:
> +	case MMC_TIMING_UHS_SDR104:
> +	case MMC_TIMING_MMC_HS200:
> +	case MMC_TIMING_MMC_HS400:
> +		if (host->top_base)
> +			sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> +				     LOOP_EN_SEL_CLK);
> +		break;
> +	default:
> +		break;
> +	}
> +}
> +
>   static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>   {
>   	struct mmc_host *mmc = mmc_from_priv(host);
> @@ -881,6 +955,7 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>   	u32 sclk;
>   	u32 tune_reg = host->dev_comp->pad_tune_reg;
>   	u32 val;
> +	bool timing_changed = false;

bool timing_changed;

>   
>   	if (!hz) {
>   		dev_dbg(host->dev, "set mclk to 0\n");
> @@ -890,6 +965,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>   		return;
>   	}
>   
> +	if (host->timing != timing)
> +		timing_changed = true;

	else
		timing_changed = false;

> +
>   	flags = readl(host->base + MSDC_INTEN);
>   	sdr_clr_bits(host->base + MSDC_INTEN, flags);
>   	if (host->dev_comp->clk_div_bits == 8)
> @@ -996,6 +1074,9 @@ static void msdc_set_mclk(struct msdc_host *host, unsigned char timing, u32 hz)
>   		sdr_set_field(host->base + tune_reg,
>   			      MSDC_PAD_TUNE_CMDRRDLY,
>   			      host->hs400_cmd_int_delay);
> +	if (timing_changed && host->dev_comp->support_new_tx)

I would invert this to (host->dev_comp->support_new_tx && timing_changed)
as that, at least to me, reads as "if this is new_tx - and the timing changed"
maning that the primary reason why we're checking is "if this is new_tx".

It's a personal preference though, so the final choice is yours.

> +		msdc_new_tx_setting(host);
> +
>   	dev_dbg(host->dev, "sclk: %d, timing: %d\n", mmc->actual_clock,
>   		timing);
>   }
> @@ -1704,6 +1785,17 @@ static void msdc_init_hw(struct msdc_host *host)
>   		reset_control_deassert(host->reset);
>   	}
>   
> +	/* New tx/rx enable bit need to be 0->1 for hardware check */
> +	if (host->dev_comp->support_new_tx) {
> +		sdr_clr_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> +		sdr_set_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> +		msdc_new_tx_setting(host);
> +	}
> +	if (host->dev_comp->support_new_rx) {
> +		sdr_clr_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
> +		sdr_set_bits(host->base + MSDC_NEW_RX_CFG, MSDC_NEW_RX_PATH_SEL);
> +	}
> +
>   	/* Configure to MMC/SD mode, clock free running */
>   	sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE | MSDC_CFG_CKPDN);
>   
> @@ -1742,8 +1834,19 @@ static void msdc_init_hw(struct msdc_host *host)
>   	sdr_set_bits(host->base + EMMC50_CFG0, EMMC50_CFG_CFCSTS_SEL);
>   
>   	if (host->dev_comp->stop_clk_fix) {
> -		sdr_set_field(host->base + MSDC_PATCH_BIT1,
> -			      MSDC_PATCH_BIT1_STOP_DLY, 3);
> +		if (host->dev_comp->stop_dly_sel)
> +			sdr_set_field(host->base + MSDC_PATCH_BIT1,
> +				      MSDC_PATCH_BIT1_STOP_DLY,
> +				      host->dev_comp->stop_dly_sel & 0xf);

This doesn't look like being something to support new_tx and new_rx, but rather
a way to specify a different STOP_DLY depending on the SoC and its platdata.

So this one goes to a different commit, and you won't need either the 0xf masking
nor the `else`, as you will have to add the value to all SoCs' platdata instead.

> +		else
> +			sdr_set_field(host->base + MSDC_PATCH_BIT1,
> +				      MSDC_PATCH_BIT1_STOP_DLY, 3);
> +
> +		if (host->dev_comp->pop_en_cnt)
> +			sdr_set_field(host->base + MSDC_PATCH_BIT2,
> +				      MSDC_PB2_POP_EN_CNT,
> +				      host->dev_comp->pop_en_cnt & 0xf);
> +
>   		sdr_clr_bits(host->base + SDC_FIFO_CFG,
>   			     SDC_FIFO_CFG_WRVALIDSEL);
>   		sdr_clr_bits(host->base + SDC_FIFO_CFG,
> @@ -2055,6 +2158,19 @@ static inline void msdc_set_data_delay(struct msdc_host *host, u32 value)
>   	}
>   }
>   

Regards,
Angelo

Re: [PATCH v2 1/2] mmc: mtk-sd: Add support for MT8196

[Andy-ld Lu (卢东)]

On Mon, 2024-09-30 at 11:13 +0200, AngeloGioacchino Del Regno wrote:
> Il 29/09/24 09:44, Andy-ld Lu ha scritto:
> > Mediatek SoC MT8196 features a new design for tx/rx path. The new
> > tx
> > path incorporates register settings that are closely associated
> > with
> > bus timing. And the difference between new rx path and older
> > versions
> > is the usage of distinct register bits when setting the data
> > sampling
> > edge as part of the tuning process.
> > 
> > Besides, there are modified register settings for STOP_DLY_SEL and
> > POP_EN_CNT, with two new fields added to the compatibility
> > structure
> > to reflect the modifications.
> > 
> > For the changes mentioned in relation to the MT8196, the new
> > compatible
> > string 'mediatek,mt8196-mmc' is introduced. This is to accommodate
> > different settings and workflows specific to the MT8196.
> > 
> > Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
> > ---
> >   drivers/mmc/host/mtk-sd.c | 162
> > +++++++++++++++++++++++++++++++++-----
> >   1 file changed, 141 insertions(+), 21 deletions(-)
> > 
> > diff --git a/drivers/mmc/host/mtk-sd.c b/drivers/mmc/host/mtk-sd.c
> > index 89018b6c97b9..4254b3012aeb 100644
> > --- a/drivers/mmc/host/mtk-sd.c
> > +++ b/drivers/mmc/host/mtk-sd.c
> > @@ -65,6 +65,7 @@
> >   #define SDC_RESP3        0x4c
> >   #define SDC_BLK_NUM      0x50
> >   #define SDC_ADV_CFG0     0x64
> > +#define MSDC_NEW_RX_CFG  0x68
> >   #define EMMC_IOCON       0x7c
> >   #define SDC_ACMD_RESP    0x80
> >   #define DMA_SA_H4BIT     0x8c
> > @@ -91,6 +92,7 @@
> >   #define EMMC_TOP_CONTROL	0x00
> >   #define EMMC_TOP_CMD		0x04
> >   #define EMMC50_PAD_DS_TUNE	0x0c
> > +#define LOOP_TEST_CONTROL	0x30
> >   
> >   /*---------------------------------------------------------------
> > -----------*/
> >   /* Register
> > Mask                                                            */
> > @@ -202,9 +204,13 @@
> >   #define SDC_STS_CMDBUSY         BIT(1)	/* RW */
> >   #define SDC_STS_SWR_COMPL       BIT(31)	/* RW */
> >   
> > -#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
> >   /* SDC_ADV_CFG0 mask */
> > +#define SDC_DAT1_IRQ_TRIGGER	BIT(19)	/* RW */
> >   #define SDC_RX_ENHANCE_EN	BIT(20)	/* RW */
> > +#define SDC_NEW_TX_EN		BIT(31)	/* RW */
> > +
> > +/* MSDC_NEW_RX_CFG mask */
> > +#define MSDC_NEW_RX_PATH_SEL	BIT(0)	/* RW */
> >   
> >   /* DMA_SA_H4BIT mask */
> >   #define DMA_ADDR_HIGH_4BIT      GENMASK(3, 0)	/* RW */
> > @@ -226,6 +232,7 @@
> >   
> >   /* MSDC_PATCH_BIT mask */
> >   #define MSDC_PATCH_BIT_ODDSUPP    BIT(1)	/* RW */
> > +#define MSDC_PATCH_BIT_RD_DAT_SEL BIT(3)	/* RW */
> >   #define MSDC_INT_DAT_LATCH_CK_SEL GENMASK(9, 7)
> >   #define MSDC_CKGEN_MSDC_DLY_SEL   GENMASK(14, 10)
> >   #define MSDC_PATCH_BIT_IODSSEL    BIT(16)	/* RW */
> > @@ -247,6 +254,8 @@
> >   #define MSDC_PB2_SUPPORT_64G      BIT(1)    /* RW */
> >   #define MSDC_PB2_RESPWAIT         GENMASK(3, 2)   /* RW */
> >   #define MSDC_PB2_RESPSTSENSEL     GENMASK(18, 16) /* RW */
> > +#define MSDC_PB2_POP_EN_CNT       GENMASK(23, 20) /* RW */
> > +#define MSDC_PB2_CFGCRCSTSEDGE    BIT(25)   /* RW */
> >   #define MSDC_PB2_CRCSTSENSEL      GENMASK(31, 29) /* RW */
> >   
> >   #define MSDC_PAD_TUNE_DATWRDLY	  GENMASK(4, 0)		/*
> > RW */
> > @@ -311,6 +320,12 @@
> >   #define PAD_DS_DLY1		GENMASK(14, 10)	/* RW */
> >   #define PAD_DS_DLY3		GENMASK(4, 0)	/* RW */
> >   
> > +/* LOOP_TEST_CONTROL mask */
> > +#define TEST_LOOP_DSCLK_MUX_SEL        BIT(0)	/* RW */
> > +#define TEST_LOOP_LATCH_MUX_SEL        BIT(1)	/* RW */
> > +#define LOOP_EN_SEL_CLK                BIT(20)	/* RW */
> > +#define TEST_HS400_CMD_LOOP_MUX_SEL    BIT(31)	/* RW */
> > +
> >   #define REQ_CMD_EIO  BIT(0)
> >   #define REQ_CMD_TMO  BIT(1)
> >   #define REQ_DAT_ERR  BIT(2)
> > @@ -391,6 +406,7 @@ struct msdc_save_para {
> >   	u32 emmc_top_control;
> >   	u32 emmc_top_cmd;
> >   	u32 emmc50_pad_ds_tune;
> > +	u32 loop_test_control;
> >   };
> >   
> >   struct mtk_mmc_compatible {
> > @@ -402,9 +418,13 @@ struct mtk_mmc_compatible {
> >   	bool data_tune;
> >   	bool busy_check;
> >   	bool stop_clk_fix;
> > +	u8 stop_dly_sel;
> > +	u8 pop_en_cnt;
> >   	bool enhance_rx;
> >   	bool support_64g;
> >   	bool use_internal_cd;
> > +	bool support_new_tx;
> > +	bool support_new_rx;
> 
> Is there really any platform that supports new_tx but *not* new_rx,
> or vice-versa?
> 
> If not, you can add just one `bool support_new_rx_tx` member to this
> structure.

Thanks for your review, and sorry for the late reply.

New tx and rx could be separately supported in our next platforms, so
we use two bool members to indicate.
> 
> >   };
> >   
> >   struct msdc_tune_para {
> > @@ -621,6 +641,23 @@ static const struct mtk_mmc_compatible
> > mt8516_compat = {
> >   	.stop_clk_fix = true,
> >   };
> >   
> > +static const struct mtk_mmc_compatible mt8196_compat = {
> > +	.clk_div_bits = 12,
> > +	.recheck_sdio_irq = false,
> > +	.hs400_tune = false,
> > +	.pad_tune_reg = MSDC_PAD_TUNE0,
> > +	.async_fifo = true,
> > +	.data_tune = true,
> > +	.busy_check = true,
> > +	.stop_clk_fix = true,
> > +	.stop_dly_sel = 1,
> > +	.pop_en_cnt = 2,
> > +	.enhance_rx = true,
> > +	.support_64g = true,
> > +	.support_new_tx = true,
> > +	.support_new_rx = true,
> > +};
> > +
> >   static const struct of_device_id msdc_of_ids[] = {
> >   	{ .compatible = "mediatek,mt2701-mmc", .data = &mt2701_compat},
> >   	{ .compatible = "mediatek,mt2712-mmc", .data = &mt2712_compat},
> > @@ -632,6 +669,7 @@ static const struct of_device_id msdc_of_ids[]
> > = {
> >   	{ .compatible = "mediatek,mt8135-mmc", .data = &mt8135_compat},
> >   	{ .compatible = "mediatek,mt8173-mmc", .data = &mt8173_compat},
> >   	{ .compatible = "mediatek,mt8183-mmc", .data = &mt8183_compat},
> > +	{ .compatible = "mediatek,mt8196-mmc", .data = &mt8196_compat},
> >   	{ .compatible = "mediatek,mt8516-mmc", .data = &mt8516_compat},
> >   
> >   	{}
> > @@ -872,6 +910,42 @@ static int msdc_ungate_clock(struct msdc_host
> > *host)
> >   				  (val & MSDC_CFG_CKSTB), 1, 20000);
> >   }
> >   
> > +static void msdc_new_tx_setting(struct msdc_host *host)
> > +{
> 
> That's simpler:
> 
> 	if (!host->top_base)
> 		return;

I will follow your comment in next change.
> 
> > +	if (host->top_base) {
> > +		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> > +			     TEST_LOOP_DSCLK_MUX_SEL);
> > +		sdr_set_bits(host->top_base + LOOP_TEST_CONTROL,
> > +			     TEST_LOOP_LATCH_MUX_SEL);
> > +		sdr_clr_bits(host->top_base + LOOP_TEST_CONTROL,
> > +			     TEST_HS400_CMD_LOOP_MUX_SEL);
> > +	}
> > +
> > +	switch (host->timing) {
> > +	case MMC_TIMING_LEGACY:
> > +	case MMC_TIMING_MMC_HS:
> > +	case MMC_TIMING_SD_HS:
> > +	case MMC_TIMING_UHS_SDR12:
> > +	case MMC_TIMING_UHS_SDR25:
> > +	case MMC_TIMING_UHS_DDR50:
> > +	case MMC_TIMING_MMC_DDR52:
> > +		if (host->top_base)
> > +			sdr_clr_bits(host->top_base +
> > LOOP_TEST_CONTROL,
> > +				     LOOP_EN_SEL_CLK);
> > +		break;
> > +	case MMC_TIMING_UHS_SDR50:
> > +	case MMC_TIMING_UHS_SDR104:
> > +	case MMC_TIMING_MMC_HS200:
> > +	case MMC_TIMING_MMC_HS400:
> > +		if (host->top_base)
> > +			sdr_set_bits(host->top_base +
> > LOOP_TEST_CONTROL,
> > +				     LOOP_EN_SEL_CLK);
> > +		break;
> > +	default:
> > +		break;
> > +	}
> > +}
> > +
> >   static void msdc_set_mclk(struct msdc_host *host, unsigned char
> > timing, u32 hz)
> >   {
> >   	struct mmc_host *mmc = mmc_from_priv(host);
> > @@ -881,6 +955,7 @@ static void msdc_set_mclk(struct msdc_host
> > *host, unsigned char timing, u32 hz)
> >   	u32 sclk;
> >   	u32 tune_reg = host->dev_comp->pad_tune_reg;
> >   	u32 val;
> > +	bool timing_changed = false;
> 
> bool timing_changed;

I will follow your comment in next change.
> 
> >   
> >   	if (!hz) {
> >   		dev_dbg(host->dev, "set mclk to 0\n");
> > @@ -890,6 +965,9 @@ static void msdc_set_mclk(struct msdc_host
> > *host, unsigned char timing, u32 hz)
> >   		return;
> >   	}
> >   
> > +	if (host->timing != timing)
> > +		timing_changed = true;
> 
> 	else
> 		timing_changed = false;

I will follow your comment in next change.
> 
> > +
> >   	flags = readl(host->base + MSDC_INTEN);
> >   	sdr_clr_bits(host->base + MSDC_INTEN, flags);
> >   	if (host->dev_comp->clk_div_bits == 8)
> > @@ -996,6 +1074,9 @@ static void msdc_set_mclk(struct msdc_host
> > *host, unsigned char timing, u32 hz)
> >   		sdr_set_field(host->base + tune_reg,
> >   			      MSDC_PAD_TUNE_CMDRRDLY,
> >   			      host->hs400_cmd_int_delay);
> > +	if (timing_changed && host->dev_comp->support_new_tx)
> 
> I would invert this to (host->dev_comp->support_new_tx &&
> timing_changed)
> as that, at least to me, reads as "if this is new_tx - and the timing
> changed"
> maning that the primary reason why we're checking is "if this is
> new_tx".
> 
> It's a personal preference though, so the final choice is yours.

Thanks for your wonderful suggestion, I will follow your comment in
next change.
> 
> > +		msdc_new_tx_setting(host);
> > +
> >   	dev_dbg(host->dev, "sclk: %d, timing: %d\n", mmc->actual_clock,
> >   		timing);
> >   }
> > @@ -1704,6 +1785,17 @@ static void msdc_init_hw(struct msdc_host
> > *host)
> >   		reset_control_deassert(host->reset);
> >   	}
> >   
> > +	/* New tx/rx enable bit need to be 0->1 for hardware check */
> > +	if (host->dev_comp->support_new_tx) {
> > +		sdr_clr_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> > +		sdr_set_bits(host->base + SDC_ADV_CFG0, SDC_NEW_TX_EN);
> > +		msdc_new_tx_setting(host);
> > +	}
> > +	if (host->dev_comp->support_new_rx) {
> > +		sdr_clr_bits(host->base + MSDC_NEW_RX_CFG,
> > MSDC_NEW_RX_PATH_SEL);
> > +		sdr_set_bits(host->base + MSDC_NEW_RX_CFG,
> > MSDC_NEW_RX_PATH_SEL);
> > +	}
> > +
> >   	/* Configure to MMC/SD mode, clock free running */
> >   	sdr_set_bits(host->base + MSDC_CFG, MSDC_CFG_MODE |
> > MSDC_CFG_CKPDN);
> >   
> > @@ -1742,8 +1834,19 @@ static void msdc_init_hw(struct msdc_host
> > *host)
> >   	sdr_set_bits(host->base + EMMC50_CFG0, EMMC50_CFG_CFCSTS_SEL);
> >   
> >   	if (host->dev_comp->stop_clk_fix) {
> > -		sdr_set_field(host->base + MSDC_PATCH_BIT1,
> > -			      MSDC_PATCH_BIT1_STOP_DLY, 3);
> > +		if (host->dev_comp->stop_dly_sel)
> > +			sdr_set_field(host->base + MSDC_PATCH_BIT1,
> > +				      MSDC_PATCH_BIT1_STOP_DLY,
> > +				      host->dev_comp->stop_dly_sel &
> > 0xf);
> 
> This doesn't look like being something to support new_tx and new_rx,
> but rather
> a way to specify a different STOP_DLY depending on the SoC and its
> platdata.
> 
> So this one goes to a different commit, and you won't need either the
> 0xf masking
> nor the `else`, as you will have to add the value to all SoCs'
> platdata instead.

I would move it to another commit, and add the value of 'stop_dly_sel'
to all the SoCs whose 'stop_clk_fix' is true.
> 
> > +		else
> > +			sdr_set_field(host->base + MSDC_PATCH_BIT1,
> > +				      MSDC_PATCH_BIT1_STOP_DLY, 3);
> > +
> > +		if (host->dev_comp->pop_en_cnt)
> > +			sdr_set_field(host->base + MSDC_PATCH_BIT2,
> > +				      MSDC_PB2_POP_EN_CNT,
> > +				      host->dev_comp->pop_en_cnt &
> > 0xf);
> > +
> >   		sdr_clr_bits(host->base + SDC_FIFO_CFG,
> >   			     SDC_FIFO_CFG_WRVALIDSEL);
> >   		sdr_clr_bits(host->base + SDC_FIFO_CFG,
> > @@ -2055,6 +2158,19 @@ static inline void
> > msdc_set_data_delay(struct msdc_host *host, u32 value)
> >   	}
> >   }
> >   
> 
> Regards,
> Angelo

[PATCH v2 2/2] dt-bindings: mmc: mtk-sd: Add support for MT8196

[Andy-ld Lu]

Extend the devicetree bindings to include the MT8196 mmc controller
by adding the compatible string 'mediatek,mt8196-mmc'.

Signed-off-by: Andy-ld Lu <andy-ld.lu@mediatek.com>
---
 Documentation/devicetree/bindings/mmc/mtk-sd.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
index c532ec92d2d9..9281a0326891 100644
--- a/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
+++ b/Documentation/devicetree/bindings/mmc/mtk-sd.yaml
@@ -24,6 +24,7 @@ properties:
           - mediatek,mt8135-mmc
           - mediatek,mt8173-mmc
           - mediatek,mt8183-mmc
+          - mediatek,mt8196-mmc
           - mediatek,mt8516-mmc
       - items:
           - const: mediatek,mt7623-mmc
@@ -190,6 +191,7 @@ allOf:
             - mediatek,mt8186-mmc
             - mediatek,mt8188-mmc
             - mediatek,mt8195-mmc
+            - mediatek,mt8196-mmc
             - mediatek,mt8516-mmc
     then:
       properties:
-- 
2.46.0

Re: [PATCH v2 2/2] dt-bindings: mmc: mtk-sd: Add support for MT8196

[Krzysztof Kozlowski]

On Sun, Sep 29, 2024 at 03:44:06PM +0800, Andy-ld Lu wrote:
> Extend the devicetree bindings to include the MT8196 mmc controller
> by adding the compatible string 'mediatek,mt8196-mmc'.

Your entire two-line sentence, says the same as commit msg and is pretty
obvious from the diff. Say something useful instead, like describe the
hardware differences.

Acked-by: Krzysztof Kozlowski <krzk@kernel.org>

Best regards,
Krzysztof

Re: [PATCH v2 2/2] dt-bindings: mmc: mtk-sd: Add support for MT8196

[Andy-ld Lu (卢东)]

On Sun, 2024-09-29 at 21:43 +0200, Krzysztof Kozlowski wrote:
>  	 
> External email : Please do not click links or open attachments until
> you have verified the sender or the content.
>  On Sun, Sep 29, 2024 at 03:44:06PM +0800, Andy-ld Lu wrote:
> > Extend the devicetree bindings to include the MT8196 mmc controller
> > by adding the compatible string 'mediatek,mt8196-mmc'.
> 
> Your entire two-line sentence, says the same as commit msg and is
> pretty
> obvious from the diff. Say something useful instead, like describe
> the
> hardware differences.
> 
> Acked-by: Krzysztof Kozlowski <krzk@kernel.org>
> 
> Best regards,
> Krzysztof

Thanks for your suggestion, I would update in next change.
>