[PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

When I tried to add a new soc to sdhci-of-dwcmshc, I found that the
existing driver code could be optimized to facilitate expansion for
the new soc. You can see another patch [sg2042-dwcmshc], which I am
working on to add SG2042 to sdhci-of-dwcmshc.

By the way, although I believe this patch only optimizes the framework
of the code and does not change the specific logic, simple verification
is certainly better. Since I don't have rk35xx/th1520 related hardware,
it would be greatly appreciated if someone could help verify it.

---

Changes in v3:
  
  The patch series is based on latest 'next' branch of [mmc-git].

  Improved the dirvier code as per comments from Adrian Hunter.
  Define new structure for dwcmshc platform data/ops. In addition, I organized
  the code and classified the helper functions.

  Since the file changes were relatively large (though the functional logic did
  not change much), I split the original patch into four for the convenience of
  review.

Changes in v2:

  Rebased on latest 'next' branch of [mmc-git]. You can simply review or test the
  patches at the link [2].

Changes in v1:

  The patch series is based on v6.9-rc1. You can simply review or test the
  patches at the link [1].

Link: https://lore.kernel.org/linux-mmc/cover.1713258948.git.unicorn_wang@outlook.com/ [sg2042-dwcmshc]
Link: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git [mmc-git]
Link: https://lore.kernel.org/linux-mmc/cover.1713257181.git.unicorn_wang@outlook.com/ [1]
Link: https://lore.kernel.org/linux-mmc/cover.1714270290.git.unicorn_wang@outlook.com/ [2]

---

Chen Wang (4):
  mmc: sdhci-of-dwcmshc: adjust positions of helper routines
  mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions
  mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520
  mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc

 drivers/mmc/host/sdhci-of-dwcmshc.c | 598 ++++++++++++++++------------
 1 file changed, 339 insertions(+), 259 deletions(-)


base-commit: d6cd1206ffaaa890e81f5d1134856d9edd406ec6
-- 
2.25.1

[PATCH v3 1/4] mmc: sdhci-of-dwcmshc: adjust positions of helper routines

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

This patch does not change the logic of the code, but only adjusts
the positions of some helper functions in the file according to
categories to facilitate future function search and maintenance.

Category: helper functions (except for driver callback functions
such as probe/remove/suspend/resume) are divided into two categories:

- dwcmshc level helpers
- soc level helpers

After the adjustment, these functions will be put together according
to category.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 392 +++++++++++++++-------------
 1 file changed, 204 insertions(+), 188 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index e79aa4b3b6c3..a68818f53786 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -216,6 +216,12 @@ struct dwcmshc_priv {
 	u16 flags;
 };
 
+/*******************************************************************************
+ *
+ * dwcmshc level helper routines begin
+ *
+ ******************************************************************************/
+
 /*
  * If DMA addr spans 128MB boundary, we split the DMA transfer into two
  * so that each DMA transfer doesn't exceed the boundary.
@@ -249,13 +255,6 @@ static unsigned int dwcmshc_get_max_clock(struct sdhci_host *host)
 		return pltfm_host->clock;
 }
 
-static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
-{
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-
-	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
-}
-
 static void dwcmshc_check_auto_cmd23(struct mmc_host *mmc,
 				     struct mmc_request *mrq)
 {
@@ -377,29 +376,6 @@ static void dwcmshc_phy_3_3v_init(struct sdhci_host *host)
 	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_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);
-	u32 emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
-	u16 emmc_ctrl;
-
-	/* Before power on, set PHY configs */
-	if (priv->flags & FLAG_IO_FIXED_1V8)
-		dwcmshc_phy_1_8v_init(host);
-	else
-		dwcmshc_phy_3_3v_init(host);
-
-	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
-		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
-		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;
-		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
-	}
-
-	sdhci_writeb(host, FIELD_PREP(PHY_DLL_CNFG1_SLVDLY_MASK, PHY_DLL_CNFG1_SLVDLY) |
-		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
-}
-
 static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
 				      unsigned int timing)
 {
@@ -437,20 +413,6 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
 	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
 }
 
-static void th1520_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);
-
-	dwcmshc_set_uhs_signaling(host, timing);
-	if (timing == MMC_TIMING_MMC_HS400)
-		priv->delay_line = PHY_SDCLKDL_DC_HS400;
-	else
-		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
-	th1520_sdhci_set_phy(host);
-}
-
 static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
 					  struct mmc_ios *ios)
 {
@@ -553,6 +515,112 @@ static void dwcmshc_cqhci_dumpregs(struct mmc_host *mmc)
 	sdhci_dumpregs(mmc_priv(mmc));
 }
 
+static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
+	.enable		= dwcmshc_sdhci_cqe_enable,
+	.disable	= sdhci_cqe_disable,
+	.dumpregs	= dwcmshc_cqhci_dumpregs,
+	.set_tran_desc	= dwcmshc_set_tran_desc,
+};
+
+static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
+{
+	struct cqhci_host *cq_host;
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
+	bool dma64 = false;
+	u16 clk;
+	int err;
+
+	host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
+	cq_host = devm_kzalloc(&pdev->dev, sizeof(*cq_host), GFP_KERNEL);
+	if (!cq_host) {
+		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: not enough memory\n");
+		goto dsbl_cqe_caps;
+	}
+
+	/*
+	 * For dwcmshc host controller we have to enable internal clock
+	 * before access to some registers from Vendor Specific Area 2.
+	 */
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	clk |= SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (!(clk & SDHCI_CLOCK_INT_EN)) {
+		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: internal clock enable error\n");
+		goto free_cq_host;
+	}
+
+	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
+	cq_host->ops = &dwcmshc_cqhci_ops;
+
+	/* Enable using of 128-bit task descriptors */
+	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
+	if (dma64) {
+		dev_dbg(mmc_dev(host->mmc), "128-bit task descriptors\n");
+		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
+	}
+	err = cqhci_init(cq_host, host->mmc, dma64);
+	if (err) {
+		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: error %d\n", err);
+		goto int_clock_disable;
+	}
+
+	dev_dbg(mmc_dev(host->mmc), "CQE init done\n");
+
+	return;
+
+int_clock_disable:
+	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	clk &= ~SDHCI_CLOCK_INT_EN;
+	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
+
+free_cq_host:
+	devm_kfree(&pdev->dev, cq_host);
+
+dsbl_cqe_caps:
+	host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
+}
+
+static void dwcmshc_disable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if (ctrl & SDHCI_CLOCK_CARD_EN) {
+		ctrl &= ~SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+#ifdef CONFIG_PM
+
+static void dwcmshc_enable_card_clk(struct sdhci_host *host)
+{
+	u16 ctrl;
+
+	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
+	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
+		ctrl |= SDHCI_CLOCK_CARD_EN;
+		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
+	}
+}
+
+#endif
+
+/*******************************************************************************
+ *
+ * SoC level helper routines begin
+ *
+ ******************************************************************************/
+
+static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
+{
+	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
+
+	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
+}
+
 static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -681,6 +749,98 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
+static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+{
+	int err;
+	struct rk35xx_priv *priv = dwc_priv->priv;
+
+	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
+	if (IS_ERR(priv->reset)) {
+		err = PTR_ERR(priv->reset);
+		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
+		return err;
+	}
+
+	priv->rockchip_clks[0].id = "axi";
+	priv->rockchip_clks[1].id = "block";
+	priv->rockchip_clks[2].id = "timer";
+	err = devm_clk_bulk_get_optional(mmc_dev(host->mmc), RK35xx_MAX_CLKS,
+					 priv->rockchip_clks);
+	if (err) {
+		dev_err(mmc_dev(host->mmc), "failed to get clocks %d\n", err);
+		return err;
+	}
+
+	err = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
+	if (err) {
+		dev_err(mmc_dev(host->mmc), "failed to enable clocks %d\n", err);
+		return err;
+	}
+
+	if (of_property_read_u8(mmc_dev(host->mmc)->of_node, "rockchip,txclk-tapnum",
+				&priv->txclk_tapnum))
+		priv->txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
+
+	/* Disable cmd conflict check */
+	sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3);
+	/* Reset previous settings */
+	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
+	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
+
+	return 0;
+}
+
+static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+{
+	/*
+	 * Don't support highspeed bus mode with low clk speed as we
+	 * cannot use DLL for this condition.
+	 */
+	if (host->mmc->f_max <= 52000000) {
+		dev_info(mmc_dev(host->mmc), "Disabling HS200/HS400, frequency too low (%d)\n",
+			 host->mmc->f_max);
+		host->mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
+		host->mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR);
+	}
+}
+
+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);
+	u32 emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
+	u16 emmc_ctrl;
+
+	/* Before power on, set PHY configs */
+	if (priv->flags & FLAG_IO_FIXED_1V8)
+		dwcmshc_phy_1_8v_init(host);
+	else
+		dwcmshc_phy_3_3v_init(host);
+
+	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
+		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
+		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;
+		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
+	}
+
+	sdhci_writeb(host, FIELD_PREP(PHY_DLL_CNFG1_SLVDLY_MASK, PHY_DLL_CNFG1_SLVDLY) |
+		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
+}
+
+static void th1520_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);
+
+	dwcmshc_set_uhs_signaling(host, timing);
+	if (timing == MMC_TIMING_MMC_HS400)
+		priv->delay_line = PHY_SDCLKDL_DC_HS400;
+	else
+		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
+	th1520_sdhci_set_phy(host);
+}
+
 static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -967,128 +1127,6 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
 	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 };
 
-static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
-	.enable		= dwcmshc_sdhci_cqe_enable,
-	.disable	= sdhci_cqe_disable,
-	.dumpregs	= dwcmshc_cqhci_dumpregs,
-	.set_tran_desc	= dwcmshc_set_tran_desc,
-};
-
-static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
-{
-	struct cqhci_host *cq_host;
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	bool dma64 = false;
-	u16 clk;
-	int err;
-
-	host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
-	cq_host = devm_kzalloc(&pdev->dev, sizeof(*cq_host), GFP_KERNEL);
-	if (!cq_host) {
-		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: not enough memory\n");
-		goto dsbl_cqe_caps;
-	}
-
-	/*
-	 * For dwcmshc host controller we have to enable internal clock
-	 * before access to some registers from Vendor Specific Area 2.
-	 */
-	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	clk |= SDHCI_CLOCK_INT_EN;
-	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	if (!(clk & SDHCI_CLOCK_INT_EN)) {
-		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: internal clock enable error\n");
-		goto free_cq_host;
-	}
-
-	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
-	cq_host->ops = &dwcmshc_cqhci_ops;
-
-	/* Enable using of 128-bit task descriptors */
-	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
-	if (dma64) {
-		dev_dbg(mmc_dev(host->mmc), "128-bit task descriptors\n");
-		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
-	}
-	err = cqhci_init(cq_host, host->mmc, dma64);
-	if (err) {
-		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: error %d\n", err);
-		goto int_clock_disable;
-	}
-
-	dev_dbg(mmc_dev(host->mmc), "CQE init done\n");
-
-	return;
-
-int_clock_disable:
-	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	clk &= ~SDHCI_CLOCK_INT_EN;
-	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
-
-free_cq_host:
-	devm_kfree(&pdev->dev, cq_host);
-
-dsbl_cqe_caps:
-	host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
-}
-
-static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
-{
-	int err;
-	struct rk35xx_priv *priv = dwc_priv->priv;
-
-	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
-	if (IS_ERR(priv->reset)) {
-		err = PTR_ERR(priv->reset);
-		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
-		return err;
-	}
-
-	priv->rockchip_clks[0].id = "axi";
-	priv->rockchip_clks[1].id = "block";
-	priv->rockchip_clks[2].id = "timer";
-	err = devm_clk_bulk_get_optional(mmc_dev(host->mmc), RK35xx_MAX_CLKS,
-					 priv->rockchip_clks);
-	if (err) {
-		dev_err(mmc_dev(host->mmc), "failed to get clocks %d\n", err);
-		return err;
-	}
-
-	err = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
-	if (err) {
-		dev_err(mmc_dev(host->mmc), "failed to enable clocks %d\n", err);
-		return err;
-	}
-
-	if (of_property_read_u8(mmc_dev(host->mmc)->of_node, "rockchip,txclk-tapnum",
-				&priv->txclk_tapnum))
-		priv->txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
-
-	/* Disable cmd conflict check */
-	sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3);
-	/* Reset previous settings */
-	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
-	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
-
-	return 0;
-}
-
-static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
-{
-	/*
-	 * Don't support highspeed bus mode with low clk speed as we
-	 * cannot use DLL for this condition.
-	 */
-	if (host->mmc->f_max <= 52000000) {
-		dev_info(mmc_dev(host->mmc), "Disabling HS200/HS400, frequency too low (%d)\n",
-			 host->mmc->f_max);
-		host->mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
-		host->mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR);
-	}
-}
-
 static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
 	{
 		.compatible = "rockchip,rk3588-dwcmshc",
@@ -1288,17 +1326,6 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	return err;
 }
 
-static void dwcmshc_disable_card_clk(struct sdhci_host *host)
-{
-	u16 ctrl;
-
-	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	if (ctrl & SDHCI_CLOCK_CARD_EN) {
-		ctrl &= ~SDHCI_CLOCK_CARD_EN;
-		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
-	}
-}
-
 static void dwcmshc_remove(struct platform_device *pdev)
 {
 	struct sdhci_host *host = platform_get_drvdata(pdev);
@@ -1406,17 +1433,6 @@ static int dwcmshc_resume(struct device *dev)
 
 #ifdef CONFIG_PM
 
-static void dwcmshc_enable_card_clk(struct sdhci_host *host)
-{
-	u16 ctrl;
-
-	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
-	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
-		ctrl |= SDHCI_CLOCK_CARD_EN;
-		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
-	}
-}
-
 static int dwcmshc_runtime_suspend(struct device *dev)
 {
 	struct sdhci_host *host = dev_get_drvdata(dev);
-- 
2.25.1

Re: [PATCH v3 1/4] mmc: sdhci-of-dwcmshc: adjust positions of helper routines

[Adrian Hunter]

On 13/06/24 04:42, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> This patch does not change the logic of the code, but only adjusts
> the positions of some helper functions in the file according to
> categories to facilitate future function search and maintenance.
> 
> Category: helper functions (except for driver callback functions
> such as probe/remove/suspend/resume) are divided into two categories:
> 
> - dwcmshc level helpers
> - soc level helpers
> 
> After the adjustment, these functions will be put together according
> to category.

Please do not move any functions unless it is needed to avoid forward
declaration.

Unnecessarily churning the code makes backports more difficult and
complicates the code history, so it should be avoided in general.

> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 392 +++++++++++++++-------------
>  1 file changed, 204 insertions(+), 188 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index e79aa4b3b6c3..a68818f53786 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -216,6 +216,12 @@ struct dwcmshc_priv {
>  	u16 flags;
>  };
>  
> +/*******************************************************************************
> + *
> + * dwcmshc level helper routines begin
> + *
> + ******************************************************************************/
> +
>  /*
>   * If DMA addr spans 128MB boundary, we split the DMA transfer into two
>   * so that each DMA transfer doesn't exceed the boundary.
> @@ -249,13 +255,6 @@ static unsigned int dwcmshc_get_max_clock(struct sdhci_host *host)
>  		return pltfm_host->clock;
>  }
>  
> -static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
> -{
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -
> -	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
> -}
> -
>  static void dwcmshc_check_auto_cmd23(struct mmc_host *mmc,
>  				     struct mmc_request *mrq)
>  {
> @@ -377,29 +376,6 @@ static void dwcmshc_phy_3_3v_init(struct sdhci_host *host)
>  	sdhci_writeb(host, PHY_DLL_CTRL_ENABLE, PHY_DLL_CTRL_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);
> -	u32 emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
> -	u16 emmc_ctrl;
> -
> -	/* Before power on, set PHY configs */
> -	if (priv->flags & FLAG_IO_FIXED_1V8)
> -		dwcmshc_phy_1_8v_init(host);
> -	else
> -		dwcmshc_phy_3_3v_init(host);
> -
> -	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
> -		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> -		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;
> -		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> -	}
> -
> -	sdhci_writeb(host, FIELD_PREP(PHY_DLL_CNFG1_SLVDLY_MASK, PHY_DLL_CNFG1_SLVDLY) |
> -		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
> -}
> -
>  static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  				      unsigned int timing)
>  {
> @@ -437,20 +413,6 @@ static void dwcmshc_set_uhs_signaling(struct sdhci_host *host,
>  	sdhci_writew(host, ctrl_2, SDHCI_HOST_CONTROL2);
>  }
>  
> -static void th1520_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);
> -
> -	dwcmshc_set_uhs_signaling(host, timing);
> -	if (timing == MMC_TIMING_MMC_HS400)
> -		priv->delay_line = PHY_SDCLKDL_DC_HS400;
> -	else
> -		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> -	th1520_sdhci_set_phy(host);
> -}
> -
>  static void dwcmshc_hs400_enhanced_strobe(struct mmc_host *mmc,
>  					  struct mmc_ios *ios)
>  {
> @@ -553,6 +515,112 @@ static void dwcmshc_cqhci_dumpregs(struct mmc_host *mmc)
>  	sdhci_dumpregs(mmc_priv(mmc));
>  }
>  
> +static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
> +	.enable		= dwcmshc_sdhci_cqe_enable,
> +	.disable	= sdhci_cqe_disable,
> +	.dumpregs	= dwcmshc_cqhci_dumpregs,
> +	.set_tran_desc	= dwcmshc_set_tran_desc,
> +};
> +
> +static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
> +{
> +	struct cqhci_host *cq_host;
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> +	bool dma64 = false;
> +	u16 clk;
> +	int err;
> +
> +	host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
> +	cq_host = devm_kzalloc(&pdev->dev, sizeof(*cq_host), GFP_KERNEL);
> +	if (!cq_host) {
> +		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: not enough memory\n");
> +		goto dsbl_cqe_caps;
> +	}
> +
> +	/*
> +	 * For dwcmshc host controller we have to enable internal clock
> +	 * before access to some registers from Vendor Specific Area 2.
> +	 */
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	clk |= SDHCI_CLOCK_INT_EN;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (!(clk & SDHCI_CLOCK_INT_EN)) {
> +		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: internal clock enable error\n");
> +		goto free_cq_host;
> +	}
> +
> +	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
> +	cq_host->ops = &dwcmshc_cqhci_ops;
> +
> +	/* Enable using of 128-bit task descriptors */
> +	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> +	if (dma64) {
> +		dev_dbg(mmc_dev(host->mmc), "128-bit task descriptors\n");
> +		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
> +	}
> +	err = cqhci_init(cq_host, host->mmc, dma64);
> +	if (err) {
> +		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: error %d\n", err);
> +		goto int_clock_disable;
> +	}
> +
> +	dev_dbg(mmc_dev(host->mmc), "CQE init done\n");
> +
> +	return;
> +
> +int_clock_disable:
> +	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	clk &= ~SDHCI_CLOCK_INT_EN;
> +	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> +
> +free_cq_host:
> +	devm_kfree(&pdev->dev, cq_host);
> +
> +dsbl_cqe_caps:
> +	host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
> +}
> +
> +static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> +		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +#ifdef CONFIG_PM
> +
> +static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> +{
> +	u16 ctrl;
> +
> +	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> +	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> +		ctrl |= SDHCI_CLOCK_CARD_EN;
> +		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> +	}
> +}
> +
> +#endif
> +
> +/*******************************************************************************
> + *
> + * SoC level helper routines begin
> + *
> + ******************************************************************************/
> +
> +static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +
> +	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
> +}
> +
>  static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -681,6 +749,98 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> +static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +{
> +	int err;
> +	struct rk35xx_priv *priv = dwc_priv->priv;
> +
> +	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> +	if (IS_ERR(priv->reset)) {
> +		err = PTR_ERR(priv->reset);
> +		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> +		return err;
> +	}
> +
> +	priv->rockchip_clks[0].id = "axi";
> +	priv->rockchip_clks[1].id = "block";
> +	priv->rockchip_clks[2].id = "timer";
> +	err = devm_clk_bulk_get_optional(mmc_dev(host->mmc), RK35xx_MAX_CLKS,
> +					 priv->rockchip_clks);
> +	if (err) {
> +		dev_err(mmc_dev(host->mmc), "failed to get clocks %d\n", err);
> +		return err;
> +	}
> +
> +	err = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
> +	if (err) {
> +		dev_err(mmc_dev(host->mmc), "failed to enable clocks %d\n", err);
> +		return err;
> +	}
> +
> +	if (of_property_read_u8(mmc_dev(host->mmc)->of_node, "rockchip,txclk-tapnum",
> +				&priv->txclk_tapnum))
> +		priv->txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
> +
> +	/* Disable cmd conflict check */
> +	sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3);
> +	/* Reset previous settings */
> +	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> +	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
> +
> +	return 0;
> +}
> +
> +static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +{
> +	/*
> +	 * Don't support highspeed bus mode with low clk speed as we
> +	 * cannot use DLL for this condition.
> +	 */
> +	if (host->mmc->f_max <= 52000000) {
> +		dev_info(mmc_dev(host->mmc), "Disabling HS200/HS400, frequency too low (%d)\n",
> +			 host->mmc->f_max);
> +		host->mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
> +		host->mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR);
> +	}
> +}
> +
> +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);
> +	u32 emmc_caps = MMC_CAP2_NO_SD | MMC_CAP2_NO_SDIO;
> +	u16 emmc_ctrl;
> +
> +	/* Before power on, set PHY configs */
> +	if (priv->flags & FLAG_IO_FIXED_1V8)
> +		dwcmshc_phy_1_8v_init(host);
> +	else
> +		dwcmshc_phy_3_3v_init(host);
> +
> +	if ((host->mmc->caps2 & emmc_caps) == emmc_caps) {
> +		emmc_ctrl = sdhci_readw(host, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> +		emmc_ctrl |= DWCMSHC_CARD_IS_EMMC;
> +		sdhci_writew(host, emmc_ctrl, priv->vendor_specific_area1 + DWCMSHC_EMMC_CONTROL);
> +	}
> +
> +	sdhci_writeb(host, FIELD_PREP(PHY_DLL_CNFG1_SLVDLY_MASK, PHY_DLL_CNFG1_SLVDLY) |
> +		     PHY_DLL_CNFG1_WAITCYCLE, PHY_DLL_CNFG1_R);
> +}
> +
> +static void th1520_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);
> +
> +	dwcmshc_set_uhs_signaling(host, timing);
> +	if (timing == MMC_TIMING_MMC_HS400)
> +		priv->delay_line = PHY_SDCLKDL_DC_HS400;
> +	else
> +		sdhci_writeb(host, 0, PHY_DLLDL_CNFG_R);
> +	th1520_sdhci_set_phy(host);
> +}
> +
>  static int th1520_execute_tuning(struct sdhci_host *host, u32 opcode)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -967,128 +1127,6 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
>  	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>  };
>  
> -static const struct cqhci_host_ops dwcmshc_cqhci_ops = {
> -	.enable		= dwcmshc_sdhci_cqe_enable,
> -	.disable	= sdhci_cqe_disable,
> -	.dumpregs	= dwcmshc_cqhci_dumpregs,
> -	.set_tran_desc	= dwcmshc_set_tran_desc,
> -};
> -
> -static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *pdev)
> -{
> -	struct cqhci_host *cq_host;
> -	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -	bool dma64 = false;
> -	u16 clk;
> -	int err;
> -
> -	host->mmc->caps2 |= MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD;
> -	cq_host = devm_kzalloc(&pdev->dev, sizeof(*cq_host), GFP_KERNEL);
> -	if (!cq_host) {
> -		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: not enough memory\n");
> -		goto dsbl_cqe_caps;
> -	}
> -
> -	/*
> -	 * For dwcmshc host controller we have to enable internal clock
> -	 * before access to some registers from Vendor Specific Area 2.
> -	 */
> -	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> -	clk |= SDHCI_CLOCK_INT_EN;
> -	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> -	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> -	if (!(clk & SDHCI_CLOCK_INT_EN)) {
> -		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: internal clock enable error\n");
> -		goto free_cq_host;
> -	}
> -
> -	cq_host->mmio = host->ioaddr + priv->vendor_specific_area2;
> -	cq_host->ops = &dwcmshc_cqhci_ops;
> -
> -	/* Enable using of 128-bit task descriptors */
> -	dma64 = host->flags & SDHCI_USE_64_BIT_DMA;
> -	if (dma64) {
> -		dev_dbg(mmc_dev(host->mmc), "128-bit task descriptors\n");
> -		cq_host->caps |= CQHCI_TASK_DESC_SZ_128;
> -	}
> -	err = cqhci_init(cq_host, host->mmc, dma64);
> -	if (err) {
> -		dev_err(mmc_dev(host->mmc), "Unable to setup CQE: error %d\n", err);
> -		goto int_clock_disable;
> -	}
> -
> -	dev_dbg(mmc_dev(host->mmc), "CQE init done\n");
> -
> -	return;
> -
> -int_clock_disable:
> -	clk = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> -	clk &= ~SDHCI_CLOCK_INT_EN;
> -	sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
> -
> -free_cq_host:
> -	devm_kfree(&pdev->dev, cq_host);
> -
> -dsbl_cqe_caps:
> -	host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
> -}
> -
> -static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> -{
> -	int err;
> -	struct rk35xx_priv *priv = dwc_priv->priv;
> -
> -	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
> -	if (IS_ERR(priv->reset)) {
> -		err = PTR_ERR(priv->reset);
> -		dev_err(mmc_dev(host->mmc), "failed to get reset control %d\n", err);
> -		return err;
> -	}
> -
> -	priv->rockchip_clks[0].id = "axi";
> -	priv->rockchip_clks[1].id = "block";
> -	priv->rockchip_clks[2].id = "timer";
> -	err = devm_clk_bulk_get_optional(mmc_dev(host->mmc), RK35xx_MAX_CLKS,
> -					 priv->rockchip_clks);
> -	if (err) {
> -		dev_err(mmc_dev(host->mmc), "failed to get clocks %d\n", err);
> -		return err;
> -	}
> -
> -	err = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
> -	if (err) {
> -		dev_err(mmc_dev(host->mmc), "failed to enable clocks %d\n", err);
> -		return err;
> -	}
> -
> -	if (of_property_read_u8(mmc_dev(host->mmc)->of_node, "rockchip,txclk-tapnum",
> -				&priv->txclk_tapnum))
> -		priv->txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
> -
> -	/* Disable cmd conflict check */
> -	sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3);
> -	/* Reset previous settings */
> -	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
> -	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
> -
> -	return 0;
> -}
> -
> -static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> -{
> -	/*
> -	 * Don't support highspeed bus mode with low clk speed as we
> -	 * cannot use DLL for this condition.
> -	 */
> -	if (host->mmc->f_max <= 52000000) {
> -		dev_info(mmc_dev(host->mmc), "Disabling HS200/HS400, frequency too low (%d)\n",
> -			 host->mmc->f_max);
> -		host->mmc->caps2 &= ~(MMC_CAP2_HS200 | MMC_CAP2_HS400);
> -		host->mmc->caps &= ~(MMC_CAP_3_3V_DDR | MMC_CAP_1_8V_DDR);
> -	}
> -}
> -
>  static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
>  	{
>  		.compatible = "rockchip,rk3588-dwcmshc",
> @@ -1288,17 +1326,6 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	return err;
>  }
>  
> -static void dwcmshc_disable_card_clk(struct sdhci_host *host)
> -{
> -	u16 ctrl;
> -
> -	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> -	if (ctrl & SDHCI_CLOCK_CARD_EN) {
> -		ctrl &= ~SDHCI_CLOCK_CARD_EN;
> -		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> -	}
> -}
> -
>  static void dwcmshc_remove(struct platform_device *pdev)
>  {
>  	struct sdhci_host *host = platform_get_drvdata(pdev);
> @@ -1406,17 +1433,6 @@ static int dwcmshc_resume(struct device *dev)
>  
>  #ifdef CONFIG_PM
>  
> -static void dwcmshc_enable_card_clk(struct sdhci_host *host)
> -{
> -	u16 ctrl;
> -
> -	ctrl = sdhci_readw(host, SDHCI_CLOCK_CONTROL);
> -	if ((ctrl & SDHCI_CLOCK_INT_EN) && !(ctrl & SDHCI_CLOCK_CARD_EN)) {
> -		ctrl |= SDHCI_CLOCK_CARD_EN;
> -		sdhci_writew(host, ctrl, SDHCI_CLOCK_CONTROL);
> -	}
> -}
> -
>  static int dwcmshc_runtime_suspend(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);

Re: [PATCH v3 1/4] mmc: sdhci-of-dwcmshc: adjust positions of helper routines

[Chen Wang]

On 2024/6/14 18:16, Adrian Hunter wrote:
> On 13/06/24 04:42, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> This patch does not change the logic of the code, but only adjusts
>> the positions of some helper functions in the file according to
>> categories to facilitate future function search and maintenance.
>>
>> Category: helper functions (except for driver callback functions
>> such as probe/remove/suspend/resume) are divided into two categories:
>>
>> - dwcmshc level helpers
>> - soc level helpers
>>
>> After the adjustment, these functions will be put together according
>> to category.
> Please do not move any functions unless it is needed to avoid forward
> declaration.
>
> Unnecessarily churning the code makes backports more difficult and
> complicates the code history, so it should be avoided in general.

Accepted.

[......]

[PATCH v3 2/4] mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Continue another patch: "mmc: sdhci-of-dwcmshc: adjust positions
of helper routines".

The helper functions at the dwcmshc level are all prefixed with
"dwcmshc_", which is easier to identify, while the functions at
the soc level are more confusing. Now they are uniformly prefixed
with the soc type string, such as "rk35xx_", "th1520_", etc.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index a68818f53786..346d2d323a05 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -193,7 +193,7 @@
 					 SDHCI_TRNS_BLK_CNT_EN | \
 					 SDHCI_TRNS_DMA)
 
-enum dwcmshc_rk_type {
+enum rk35xx_type {
 	DWCMSHC_RK3568,
 	DWCMSHC_RK3588,
 };
@@ -202,7 +202,7 @@ struct rk35xx_priv {
 	/* Rockchip specified optional clocks */
 	struct clk_bulk_data rockchip_clks[RK35xx_MAX_CLKS];
 	struct reset_control *reset;
-	enum dwcmshc_rk_type devtype;
+	enum rk35xx_type devtype;
 	u8 txclk_tapnum;
 };
 
@@ -621,7 +621,7 @@ static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
 	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
 }
 
-static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
+static void rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
@@ -749,7 +749,7 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
-static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
 {
 	int err;
 	struct rk35xx_priv *priv = dwc_priv->priv;
@@ -790,7 +790,7 @@ static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
 	return 0;
 }
 
-static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
 {
 	/*
 	 * Don't support highspeed bus mode with low clk speed as we
@@ -1062,7 +1062,7 @@ static const struct sdhci_ops sdhci_dwcmshc_ops = {
 };
 
 static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
-	.set_clock		= dwcmshc_rk3568_set_clock,
+	.set_clock		= rk3568_set_clock,
 	.set_bus_width		= sdhci_set_bus_width,
 	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
 	.get_max_clock		= rk35xx_get_max_clock,
@@ -1243,7 +1243,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
 
 		priv->priv = rk_priv;
 
-		err = dwcmshc_rk35xx_init(host, priv);
+		err = rk35xx_init(host, priv);
 		if (err)
 			goto err_clk;
 	}
@@ -1300,7 +1300,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	}
 
 	if (rk_priv)
-		dwcmshc_rk35xx_postinit(host, priv);
+		rk35xx_postinit(host, priv);
 
 	err = __sdhci_add_host(host);
 	if (err)
-- 
2.25.1

Re: [PATCH v3 2/4] mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions

[Adrian Hunter]

On 13/06/24 04:42, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Continue another patch: "mmc: sdhci-of-dwcmshc: adjust positions
> of helper routines".
> 
> The helper functions at the dwcmshc level are all prefixed with
> "dwcmshc_", which is easier to identify, while the functions at
> the soc level are more confusing. Now they are uniformly prefixed
> with the soc type string, such as "rk35xx_", "th1520_", etc.

This does not seem to be necessary.

Unnecessarily churning the code makes backports more difficult and
complicates the code history, so it should be avoided in general.

> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index a68818f53786..346d2d323a05 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -193,7 +193,7 @@
>  					 SDHCI_TRNS_BLK_CNT_EN | \
>  					 SDHCI_TRNS_DMA)
>  
> -enum dwcmshc_rk_type {
> +enum rk35xx_type {
>  	DWCMSHC_RK3568,
>  	DWCMSHC_RK3588,
>  };
> @@ -202,7 +202,7 @@ struct rk35xx_priv {
>  	/* Rockchip specified optional clocks */
>  	struct clk_bulk_data rockchip_clks[RK35xx_MAX_CLKS];
>  	struct reset_control *reset;
> -	enum dwcmshc_rk_type devtype;
> +	enum rk35xx_type devtype;
>  	u8 txclk_tapnum;
>  };
>  
> @@ -621,7 +621,7 @@ static unsigned int rk35xx_get_max_clock(struct sdhci_host *host)
>  	return clk_round_rate(pltfm_host->clk, ULONG_MAX);
>  }
>  
> -static void dwcmshc_rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
> +static void rk3568_set_clock(struct sdhci_host *host, unsigned int clock)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> @@ -749,7 +749,7 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> -static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  {
>  	int err;
>  	struct rk35xx_priv *priv = dwc_priv->priv;
> @@ -790,7 +790,7 @@ static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
>  	return 0;
>  }
>  
> -static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  {
>  	/*
>  	 * Don't support highspeed bus mode with low clk speed as we
> @@ -1062,7 +1062,7 @@ static const struct sdhci_ops sdhci_dwcmshc_ops = {
>  };
>  
>  static const struct sdhci_ops sdhci_dwcmshc_rk35xx_ops = {
> -	.set_clock		= dwcmshc_rk3568_set_clock,
> +	.set_clock		= rk3568_set_clock,
>  	.set_bus_width		= sdhci_set_bus_width,
>  	.set_uhs_signaling	= dwcmshc_set_uhs_signaling,
>  	.get_max_clock		= rk35xx_get_max_clock,
> @@ -1243,7 +1243,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  
>  		priv->priv = rk_priv;
>  
> -		err = dwcmshc_rk35xx_init(host, priv);
> +		err = rk35xx_init(host, priv);
>  		if (err)
>  			goto err_clk;
>  	}
> @@ -1300,7 +1300,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	}
>  
>  	if (rk_priv)
> -		dwcmshc_rk35xx_postinit(host, priv);
> +		rk35xx_postinit(host, priv);
>  
>  	err = __sdhci_add_host(host);
>  	if (err)

Re: [PATCH v3 2/4] mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions

[Chen Wang]

On 2024/6/14 18:18, Adrian Hunter wrote:
> On 13/06/24 04:42, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> Continue another patch: "mmc: sdhci-of-dwcmshc: adjust positions
>> of helper routines".
>>
>> The helper functions at the dwcmshc level are all prefixed with
>> "dwcmshc_", which is easier to identify, while the functions at
>> the soc level are more confusing. Now they are uniformly prefixed
>> with the soc type string, such as "rk35xx_", "th1520_", etc.
> This does not seem to be necessary.
>
> Unnecessarily churning the code makes backports more difficult and
> complicates the code history, so it should be avoided in general.

Accepted.

[......]

[PATCH v3 3/4] mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

Extract init function for rk35xx/th1520, which is an intermediate
process before further optimization.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 83 ++++++++++++++++-------------
 1 file changed, 46 insertions(+), 37 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 346d2d323a05..38ab755aa044 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -749,10 +749,19 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 	sdhci_reset(host, mask);
 }
 
-static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static int rk35xx_init(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
 {
 	int err;
-	struct rk35xx_priv *priv = dwc_priv->priv;
+	struct rk35xx_priv *priv;
+
+	priv = devm_kzalloc(dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc"))
+		priv->devtype = DWCMSHC_RK3588;
+	else
+		priv->devtype = DWCMSHC_RK3568;
 
 	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
 	if (IS_ERR(priv->reset)) {
@@ -787,6 +796,8 @@ static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
 	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
 	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
 
+	dwc_priv->priv = priv;
+
 	return 0;
 }
 
@@ -915,6 +926,35 @@ static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
 	}
 }
 
+static int th1520_init(struct device *dev,
+		       struct sdhci_host *host,
+		       struct dwcmshc_priv *dwc_priv)
+{
+	dwc_priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;
+
+	if (device_property_read_bool(dev, "mmc-ddr-1_8v") ||
+	    device_property_read_bool(dev, "mmc-hs200-1_8v") ||
+	    device_property_read_bool(dev, "mmc-hs400-1_8v"))
+		dwc_priv->flags |= FLAG_IO_FIXED_1V8;
+	else
+		dwc_priv->flags &= ~FLAG_IO_FIXED_1V8;
+
+	/*
+	 * start_signal_voltage_switch() will try 3.3V first
+	 * then 1.8V. Use SDHCI_SIGNALING_180 rather than
+	 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
+	 * in sdhci_start_signal_voltage_switch().
+	 */
+	if (dwc_priv->flags & FLAG_IO_FIXED_1V8) {
+		host->flags &= ~SDHCI_SIGNALING_330;
+		host->flags |=  SDHCI_SIGNALING_180;
+	}
+
+	sdhci_enable_v4_mode(host);
+
+	return 0;
+}
+
 static void cv18xx_sdhci_reset(struct sdhci_host *host, u8 mask)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1230,46 +1270,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
 
 	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
-		rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
-		if (!rk_priv) {
-			err = -ENOMEM;
-			goto err_clk;
-		}
-
-		if (of_device_is_compatible(pdev->dev.of_node, "rockchip,rk3588-dwcmshc"))
-			rk_priv->devtype = DWCMSHC_RK3588;
-		else
-			rk_priv->devtype = DWCMSHC_RK3568;
-
-		priv->priv = rk_priv;
-
-		err = rk35xx_init(host, priv);
+		err = rk35xx_init(&pdev->dev, host, priv);
 		if (err)
 			goto err_clk;
 	}
 
 	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
-		priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;
-
-		if (device_property_read_bool(dev, "mmc-ddr-1_8v") ||
-		    device_property_read_bool(dev, "mmc-hs200-1_8v") ||
-		    device_property_read_bool(dev, "mmc-hs400-1_8v"))
-			priv->flags |= FLAG_IO_FIXED_1V8;
-		else
-			priv->flags &= ~FLAG_IO_FIXED_1V8;
-
-		/*
-		 * start_signal_voltage_switch() will try 3.3V first
-		 * then 1.8V. Use SDHCI_SIGNALING_180 rather than
-		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
-		 * in sdhci_start_signal_voltage_switch().
-		 */
-		if (priv->flags & FLAG_IO_FIXED_1V8) {
-			host->flags &= ~SDHCI_SIGNALING_330;
-			host->flags |=  SDHCI_SIGNALING_180;
-		}
-
-		sdhci_enable_v4_mode(host);
+		err = th1520_init(&pdev->dev, host, priv);
+		if (err)
+			goto err_clk;
 	}
 
 #ifdef CONFIG_ACPI
-- 
2.25.1

Re: [PATCH v3 3/4] mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520

[Adrian Hunter]

On 13/06/24 04:43, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> Extract init function for rk35xx/th1520, which is an intermediate
> process before further optimization.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 83 ++++++++++++++++-------------
>  1 file changed, 46 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 346d2d323a05..38ab755aa044 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -749,10 +749,19 @@ static void rk35xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	sdhci_reset(host, mask);
>  }
>  
> -static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +static int rk35xx_init(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  {
>  	int err;
> -	struct rk35xx_priv *priv = dwc_priv->priv;
> +	struct rk35xx_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc"))
> +		priv->devtype = DWCMSHC_RK3588;
> +	else
> +		priv->devtype = DWCMSHC_RK3568;
>  
>  	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
>  	if (IS_ERR(priv->reset)) {
> @@ -787,6 +796,8 @@ static int rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>  	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
>  	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
>  
> +	dwc_priv->priv = priv;
> +
>  	return 0;
>  }
>  
> @@ -915,6 +926,35 @@ static void th1520_sdhci_reset(struct sdhci_host *host, u8 mask)
>  	}
>  }
>  
> +static int th1520_init(struct device *dev,
> +		       struct sdhci_host *host,
> +		       struct dwcmshc_priv *dwc_priv)
> +{
> +	dwc_priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;
> +
> +	if (device_property_read_bool(dev, "mmc-ddr-1_8v") ||
> +	    device_property_read_bool(dev, "mmc-hs200-1_8v") ||
> +	    device_property_read_bool(dev, "mmc-hs400-1_8v"))
> +		dwc_priv->flags |= FLAG_IO_FIXED_1V8;
> +	else
> +		dwc_priv->flags &= ~FLAG_IO_FIXED_1V8;
> +
> +	/*
> +	 * start_signal_voltage_switch() will try 3.3V first
> +	 * then 1.8V. Use SDHCI_SIGNALING_180 rather than
> +	 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> +	 * in sdhci_start_signal_voltage_switch().
> +	 */
> +	if (dwc_priv->flags & FLAG_IO_FIXED_1V8) {
> +		host->flags &= ~SDHCI_SIGNALING_330;
> +		host->flags |=  SDHCI_SIGNALING_180;
> +	}
> +
> +	sdhci_enable_v4_mode(host);
> +
> +	return 0;
> +}
> +
>  static void cv18xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1230,46 +1270,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
>  
>  	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
> -		rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
> -		if (!rk_priv) {
> -			err = -ENOMEM;
> -			goto err_clk;
> -		}
> -
> -		if (of_device_is_compatible(pdev->dev.of_node, "rockchip,rk3588-dwcmshc"))
> -			rk_priv->devtype = DWCMSHC_RK3588;
> -		else
> -			rk_priv->devtype = DWCMSHC_RK3568;
> -
> -		priv->priv = rk_priv;
> -
> -		err = rk35xx_init(host, priv);
> +		err = rk35xx_init(&pdev->dev, host, priv);

rk_priv is used further on, but it is not assigned anymore.

>  		if (err)
>  			goto err_clk;
>  	}
>  
>  	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> -		priv->delay_line = PHY_SDCLKDL_DC_DEFAULT;
> -
> -		if (device_property_read_bool(dev, "mmc-ddr-1_8v") ||
> -		    device_property_read_bool(dev, "mmc-hs200-1_8v") ||
> -		    device_property_read_bool(dev, "mmc-hs400-1_8v"))
> -			priv->flags |= FLAG_IO_FIXED_1V8;
> -		else
> -			priv->flags &= ~FLAG_IO_FIXED_1V8;
> -
> -		/*
> -		 * start_signal_voltage_switch() will try 3.3V first
> -		 * then 1.8V. Use SDHCI_SIGNALING_180 rather than
> -		 * SDHCI_SIGNALING_330 to avoid setting voltage to 3.3V
> -		 * in sdhci_start_signal_voltage_switch().
> -		 */
> -		if (priv->flags & FLAG_IO_FIXED_1V8) {
> -			host->flags &= ~SDHCI_SIGNALING_330;
> -			host->flags |=  SDHCI_SIGNALING_180;
> -		}
> -
> -		sdhci_enable_v4_mode(host);
> +		err = th1520_init(&pdev->dev, host, priv);
> +		if (err)
> +			goto err_clk;
>  	}
>  
>  #ifdef CONFIG_ACPI

Re: [PATCH v3 3/4] mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520

[Chen Wang]

On 2024/6/14 18:33, Adrian Hunter wrote:
> On 13/06/24 04:43, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
[......]
>>
>>
>>   static void cv18xx_sdhci_reset(struct sdhci_host *host, u8 mask)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1230,46 +1270,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
>>   
>>   	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
>> -		rk_priv = devm_kzalloc(&pdev->dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
>> -		if (!rk_priv) {
>> -			err = -ENOMEM;
>> -			goto err_clk;
>> -		}
>> -
>> -		if (of_device_is_compatible(pdev->dev.of_node, "rockchip,rk3588-dwcmshc"))
>> -			rk_priv->devtype = DWCMSHC_RK3588;
>> -		else
>> -			rk_priv->devtype = DWCMSHC_RK3568;
>> -
>> -		priv->priv = rk_priv;
>> -
>> -		err = rk35xx_init(host, priv);
>> +		err = rk35xx_init(&pdev->dev, host, priv);
> rk_priv is used further on, but it is not assigned anymore.

You are right. It seems unnecessary to provide this patch separately 
just to extract the init function, and it also violates the principle of 
atomicity of patch modification. I will merge this patch with the next one.

[......]

[PATCH v3 4/4] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc

[Chen Wang]

From: Chen Wang <unicorn_wang@outlook.com>

The current framework is not easily extended to support new SOCs.
For example, in the current code we see that the SOC-level
structure `rk35xx_priv` and related logic are distributed in
functions such as dwcmshc_probe/dwcmshc_remove/dwcmshc_suspend/......,
which is inappropriate.

The solution is to abstract some possible common operations of soc
as dwcmshc platform data. Each soc implements the corresponding callback
function according to its own needs.
dwcmshc framework is responsible for calling these callback functions
in those dwcmshc_xxx functions at proper positions.

Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 143 +++++++++++++++++++---------
 1 file changed, 99 insertions(+), 44 deletions(-)

diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 38ab755aa044..ebae461019f9 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -206,6 +206,7 @@ struct rk35xx_priv {
 	u8 txclk_tapnum;
 };
 
+struct dwcmshc_ops;
 struct dwcmshc_priv {
 	struct clk	*bus_clk;
 	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
@@ -214,6 +215,20 @@ struct dwcmshc_priv {
 	void *priv; /* pointer to SoC private stuff */
 	u16 delay_line;
 	u16 flags;
+
+	const struct dwcmshc_ops *ops;
+};
+
+struct dwcmshc_ops {
+	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
+	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
+	int  (*clks_enable)(struct dwcmshc_priv *dwc_priv);
+	void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
+};
+
+struct dwcmshc_data {
+	const struct sdhci_pltfm_data *pdata;
+	const struct dwcmshc_ops *ops;
 };
 
 /*******************************************************************************
@@ -815,6 +830,25 @@ static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_pr
 	}
 }
 
+static int rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
+{
+	struct rk35xx_priv *priv = dwc_priv->priv;
+	int ret = 0;
+
+	if (priv)
+		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
+	return ret;
+}
+
+static void rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
+{
+	struct rk35xx_priv *priv = dwc_priv->priv;
+
+	if (priv)
+		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
+					   priv->rockchip_clks);
+}
+
 static void th1520_sdhci_set_phy(struct sdhci_host *host)
 {
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
@@ -1167,30 +1201,65 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
 	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 };
 
+static const struct dwcmshc_ops dwcmshc_rk35xx_ops = {
+	.init = rk35xx_init,
+	.postinit = rk35xx_postinit,
+	.clks_enable = rk35xx_clks_enable,
+	.clks_disable = rk35xx_clks_disable,
+};
+
+static const struct dwcmshc_ops dwcmshc_th1520_ops = {
+	.init = th1520_init,
+};
+
+static const struct dwcmshc_data dwcmshc_cv18xx_data = {
+	.pdata = &sdhci_dwcmshc_cv18xx_pdata,
+};
+
+static const struct dwcmshc_data dwcmshc_generic_data = {
+	.pdata = &sdhci_dwcmshc_pdata,
+};
+
+static const struct dwcmshc_data dwcmshc_rk35xx_data = {
+	.pdata = &sdhci_dwcmshc_rk35xx_pdata,
+	.ops = &dwcmshc_rk35xx_ops,
+};
+
+static const struct dwcmshc_data dwcmshc_th1520_data = {
+	.pdata = &sdhci_dwcmshc_th1520_pdata,
+	.ops = &dwcmshc_th1520_ops,
+};
+
+#ifdef CONFIG_ACPI
+static const struct dwcmshc_data dwcmshc_bf3_data = {
+	.pdata = &sdhci_dwcmshc_bf3_pdata,
+};
+#endif
+
 static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
 	{
 		.compatible = "rockchip,rk3588-dwcmshc",
-		.data = &sdhci_dwcmshc_rk35xx_pdata,
+		.data = &dwcmshc_rk35xx_data,
 	},
 	{
 		.compatible = "rockchip,rk3568-dwcmshc",
-		.data = &sdhci_dwcmshc_rk35xx_pdata,
+		.data = &dwcmshc_rk35xx_data,
 	},
 	{
 		.compatible = "snps,dwcmshc-sdhci",
-		.data = &sdhci_dwcmshc_pdata,
+		.data = &dwcmshc_generic_data,
 	},
 	{
 		.compatible = "sophgo,cv1800b-dwcmshc",
-		.data = &sdhci_dwcmshc_cv18xx_pdata,
+		.data = &dwcmshc_cv18xx_data,
 	},
 	{
 		.compatible = "sophgo,sg2002-dwcmshc",
-		.data = &sdhci_dwcmshc_cv18xx_pdata,
+		.data = &dwcmshc_cv18xx_data,
 	},
 	{
 		.compatible = "thead,th1520-dwcmshc",
-		.data = &sdhci_dwcmshc_th1520_pdata,
+		.data = &dwcmshc_th1520_data,
 	},
 	{},
 };
@@ -1200,7 +1269,7 @@ MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
 static const struct acpi_device_id sdhci_dwcmshc_acpi_ids[] = {
 	{
 		.id = "MLNXBF30",
-		.driver_data = (kernel_ulong_t)&sdhci_dwcmshc_bf3_pdata,
+		.driver_data = (kernel_ulong_t)&dwcmshc_bf3_data,
 	},
 	{}
 };
@@ -1213,18 +1282,17 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	struct sdhci_pltfm_host *pltfm_host;
 	struct sdhci_host *host;
 	struct dwcmshc_priv *priv;
-	struct rk35xx_priv *rk_priv = NULL;
-	const struct sdhci_pltfm_data *pltfm_data;
+	const struct dwcmshc_data *data;
 	int err;
 	u32 extra, caps;
 
-	pltfm_data = device_get_match_data(&pdev->dev);
-	if (!pltfm_data) {
+	data = device_get_match_data(&pdev->dev);
+	if (!data) {
 		dev_err(&pdev->dev, "Error: No device match data found\n");
 		return -ENODEV;
 	}
 
-	host = sdhci_pltfm_init(pdev, pltfm_data,
+	host = sdhci_pltfm_init(pdev, data->pdata,
 				sizeof(struct dwcmshc_priv));
 	if (IS_ERR(host))
 		return PTR_ERR(host);
@@ -1239,6 +1307,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
 
 	pltfm_host = sdhci_priv(host);
 	priv = sdhci_pltfm_priv(pltfm_host);
+	priv->ops = data->ops;
 
 	if (dev->of_node) {
 		pltfm_host->clk = devm_clk_get(dev, "core");
@@ -1269,20 +1338,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
 	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
 
-	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
-		err = rk35xx_init(&pdev->dev, host, priv);
-		if (err)
-			goto err_clk;
-	}
-
-	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
-		err = th1520_init(&pdev->dev, host, priv);
+	if (data->ops && data->ops->init) {
+		err = data->ops->init(&pdev->dev, host, priv);
 		if (err)
 			goto err_clk;
 	}
 
 #ifdef CONFIG_ACPI
-	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
+	if (data == &dwcmshc_bf3_data)
 		sdhci_enable_v4_mode(host);
 #endif
 
@@ -1308,8 +1371,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 		dwcmshc_cqhci_init(host, pdev);
 	}
 
-	if (rk_priv)
-		rk35xx_postinit(host, priv);
+	if (data->ops && data->ops->postinit)
+		data->ops->postinit(host, priv);
 
 	err = __sdhci_add_host(host);
 	if (err)
@@ -1327,9 +1390,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 err_clk:
 	clk_disable_unprepare(pltfm_host->clk);
 	clk_disable_unprepare(priv->bus_clk);
-	if (rk_priv)
-		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
-					   rk_priv->rockchip_clks);
+	if (data->ops && data->ops->clks_disable)
+		data->ops->clks_disable(priv);
 free_pltfm:
 	sdhci_pltfm_free(pdev);
 	return err;
@@ -1340,7 +1402,6 @@ static void dwcmshc_remove(struct platform_device *pdev)
 	struct sdhci_host *host = platform_get_drvdata(pdev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	struct rk35xx_priv *rk_priv = priv->priv;
 
 	pm_runtime_get_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
@@ -1352,9 +1413,8 @@ static void dwcmshc_remove(struct platform_device *pdev)
 
 	clk_disable_unprepare(pltfm_host->clk);
 	clk_disable_unprepare(priv->bus_clk);
-	if (rk_priv)
-		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
-					   rk_priv->rockchip_clks);
+	if (priv->ops && priv->ops->clks_disable)
+		priv->ops->clks_disable(priv);
 	sdhci_pltfm_free(pdev);
 }
 
@@ -1364,7 +1424,6 @@ static int dwcmshc_suspend(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
 	pm_runtime_resume(dev);
@@ -1383,9 +1442,8 @@ static int dwcmshc_suspend(struct device *dev)
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);
 
-	if (rk_priv)
-		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
-					   rk_priv->rockchip_clks);
+	if (priv->ops && priv->ops->clks_disable)
+		priv->ops->clks_disable(priv);
 
 	return ret;
 }
@@ -1395,7 +1453,6 @@ static int dwcmshc_resume(struct device *dev)
 	struct sdhci_host *host = dev_get_drvdata(dev);
 	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
 	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
-	struct rk35xx_priv *rk_priv = priv->priv;
 	int ret;
 
 	ret = clk_prepare_enable(pltfm_host->clk);
@@ -1408,29 +1465,27 @@ static int dwcmshc_resume(struct device *dev)
 			goto disable_clk;
 	}
 
-	if (rk_priv) {
-		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
-					      rk_priv->rockchip_clks);
+	if (priv->ops && priv->ops->clks_enable) {
+		ret = priv->ops->clks_enable(priv);
 		if (ret)
 			goto disable_bus_clk;
 	}
 
 	ret = sdhci_resume_host(host);
 	if (ret)
-		goto disable_rockchip_clks;
+		goto disable_soc_clks;
 
 	if (host->mmc->caps2 & MMC_CAP2_CQE) {
 		ret = cqhci_resume(host->mmc);
 		if (ret)
-			goto disable_rockchip_clks;
+			goto disable_soc_clks;
 	}
 
 	return 0;
 
-disable_rockchip_clks:
-	if (rk_priv)
-		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
-					   rk_priv->rockchip_clks);
+disable_soc_clks:
+	if (priv->ops && priv->ops->clks_disable)
+		priv->ops->clks_disable(priv);
 disable_bus_clk:
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);
-- 
2.25.1

Re: [PATCH v3 4/4] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc

[Adrian Hunter]

On 13/06/24 04:43, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> The current framework is not easily extended to support new SOCs.
> For example, in the current code we see that the SOC-level
> structure `rk35xx_priv` and related logic are distributed in
> functions such as dwcmshc_probe/dwcmshc_remove/dwcmshc_suspend/......,
> which is inappropriate.
> 
> The solution is to abstract some possible common operations of soc
> as dwcmshc platform data. Each soc implements the corresponding callback
> function according to its own needs.
> dwcmshc framework is responsible for calling these callback functions
> in those dwcmshc_xxx functions at proper positions.
> 
> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
> ---
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 143 +++++++++++++++++++---------
>  1 file changed, 99 insertions(+), 44 deletions(-)
> 
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 38ab755aa044..ebae461019f9 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -206,6 +206,7 @@ struct rk35xx_priv {
>  	u8 txclk_tapnum;
>  };
>  
> +struct dwcmshc_ops;
>  struct dwcmshc_priv {
>  	struct clk	*bus_clk;
>  	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
> @@ -214,6 +215,20 @@ struct dwcmshc_priv {
>  	void *priv; /* pointer to SoC private stuff */
>  	u16 delay_line;
>  	u16 flags;
> +
> +	const struct dwcmshc_ops *ops;

	const struct dwcmshc_data *data;

> +};
> +
> +struct dwcmshc_ops {
> +	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> +	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> +	int  (*clks_enable)(struct dwcmshc_priv *dwc_priv);
> +	void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
> +};
> +
> +struct dwcmshc_data {
> +	const struct sdhci_pltfm_data *pdata;
> +	const struct dwcmshc_ops *ops;
>  };

Currently, ops and pdata values are unique to an individual
dwcmshc_data, so it is simpler to put it altogether i.e.

struct dwcmshc_data {
	const struct sdhci_pltfm_data pdata;
	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
	int  (*clks_enable)(struct dwcmshc_priv *dwc_priv);
	void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
};

>  
>  /*******************************************************************************
> @@ -815,6 +830,25 @@ static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_pr
>  	}
>  }
>  
> +static int rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
> +{
> +	struct rk35xx_priv *priv = dwc_priv->priv;
> +	int ret = 0;
> +
> +	if (priv)
> +		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
> +	return ret;
> +}
> +
> +static void rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
> +{
> +	struct rk35xx_priv *priv = dwc_priv->priv;
> +
> +	if (priv)
> +		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> +					   priv->rockchip_clks);
> +}
> +
>  static void th1520_sdhci_set_phy(struct sdhci_host *host)
>  {
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1167,30 +1201,65 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
>  	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>  };
>  
> +static const struct dwcmshc_ops dwcmshc_rk35xx_ops = {
> +	.init = rk35xx_init,
> +	.postinit = rk35xx_postinit,
> +	.clks_enable = rk35xx_clks_enable,
> +	.clks_disable = rk35xx_clks_disable,
> +};

So this becomes:

static const struct dwcmshc_data sdhci_dwcmshc_rk35xx_pdata = {
	.pdata = {
		.ops = &sdhci_dwcmshc_rk35xx_ops,
		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
			  SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
			   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
	},
	.init = rk35xx_init,
	.postinit = rk35xx_postinit,
	.clks_enable = rk35xx_clks_enable,
	.clks_disable = rk35xx_clks_disable,
};

etc

> +
> +static const struct dwcmshc_ops dwcmshc_th1520_ops = {
> +	.init = th1520_init,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_cv18xx_data = {
> +	.pdata = &sdhci_dwcmshc_cv18xx_pdata,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_generic_data = {
> +	.pdata = &sdhci_dwcmshc_pdata,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_rk35xx_data = {
> +	.pdata = &sdhci_dwcmshc_rk35xx_pdata,
> +	.ops = &dwcmshc_rk35xx_ops,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_th1520_data = {
> +	.pdata = &sdhci_dwcmshc_th1520_pdata,
> +	.ops = &dwcmshc_th1520_ops,
> +};
> +
> +#ifdef CONFIG_ACPI
> +static const struct dwcmshc_data dwcmshc_bf3_data = {
> +	.pdata = &sdhci_dwcmshc_bf3_pdata,
> +};
> +#endif
> +
>  static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
>  	{
>  		.compatible = "rockchip,rk3588-dwcmshc",
> -		.data = &sdhci_dwcmshc_rk35xx_pdata,
> +		.data = &dwcmshc_rk35xx_data,
>  	},
>  	{
>  		.compatible = "rockchip,rk3568-dwcmshc",
> -		.data = &sdhci_dwcmshc_rk35xx_pdata,
> +		.data = &dwcmshc_rk35xx_data,
>  	},
>  	{
>  		.compatible = "snps,dwcmshc-sdhci",
> -		.data = &sdhci_dwcmshc_pdata,
> +		.data = &dwcmshc_generic_data,
>  	},
>  	{
>  		.compatible = "sophgo,cv1800b-dwcmshc",
> -		.data = &sdhci_dwcmshc_cv18xx_pdata,
> +		.data = &dwcmshc_cv18xx_data,
>  	},
>  	{
>  		.compatible = "sophgo,sg2002-dwcmshc",
> -		.data = &sdhci_dwcmshc_cv18xx_pdata,
> +		.data = &dwcmshc_cv18xx_data,
>  	},
>  	{
>  		.compatible = "thead,th1520-dwcmshc",
> -		.data = &sdhci_dwcmshc_th1520_pdata,
> +		.data = &dwcmshc_th1520_data,
>  	},
>  	{},
>  };
> @@ -1200,7 +1269,7 @@ MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
>  static const struct acpi_device_id sdhci_dwcmshc_acpi_ids[] = {
>  	{
>  		.id = "MLNXBF30",
> -		.driver_data = (kernel_ulong_t)&sdhci_dwcmshc_bf3_pdata,
> +		.driver_data = (kernel_ulong_t)&dwcmshc_bf3_data,
>  	},
>  	{}
>  };
> @@ -1213,18 +1282,17 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	struct sdhci_pltfm_host *pltfm_host;
>  	struct sdhci_host *host;
>  	struct dwcmshc_priv *priv;
> -	struct rk35xx_priv *rk_priv = NULL;
> -	const struct sdhci_pltfm_data *pltfm_data;
> +	const struct dwcmshc_data *data;
>  	int err;
>  	u32 extra, caps;
>  
> -	pltfm_data = device_get_match_data(&pdev->dev);
> -	if (!pltfm_data) {
> +	data = device_get_match_data(&pdev->dev);
> +	if (!data) {
>  		dev_err(&pdev->dev, "Error: No device match data found\n");
>  		return -ENODEV;
>  	}
>  
> -	host = sdhci_pltfm_init(pdev, pltfm_data,
> +	host = sdhci_pltfm_init(pdev, data->pdata,
>  				sizeof(struct dwcmshc_priv));
>  	if (IS_ERR(host))
>  		return PTR_ERR(host);
> @@ -1239,6 +1307,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  
>  	pltfm_host = sdhci_priv(host);
>  	priv = sdhci_pltfm_priv(pltfm_host);
> +	priv->ops = data->ops;

Becomes:

	priv->data = data;

>  
>  	if (dev->of_node) {
>  		pltfm_host->clk = devm_clk_get(dev, "core");
> @@ -1269,20 +1338,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  	host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
>  	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
>  
> -	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
> -		err = rk35xx_init(&pdev->dev, host, priv);
> -		if (err)
> -			goto err_clk;
> -	}
> -
> -	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> -		err = th1520_init(&pdev->dev, host, priv);
> +	if (data->ops && data->ops->init) {

Becomes:

	if (data->init) {

etc

> +		err = data->ops->init(&pdev->dev, host, priv);
>  		if (err)
>  			goto err_clk;
>  	}
>  
>  #ifdef CONFIG_ACPI
> -	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> +	if (data == &dwcmshc_bf3_data)
>  		sdhci_enable_v4_mode(host);
>  #endif
>  
> @@ -1308,8 +1371,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  		dwcmshc_cqhci_init(host, pdev);
>  	}
>  
> -	if (rk_priv)
> -		rk35xx_postinit(host, priv);
> +	if (data->ops && data->ops->postinit)
> +		data->ops->postinit(host, priv);
>  
>  	err = __sdhci_add_host(host);
>  	if (err)
> @@ -1327,9 +1390,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>  err_clk:
>  	clk_disable_unprepare(pltfm_host->clk);
>  	clk_disable_unprepare(priv->bus_clk);
> -	if (rk_priv)
> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> -					   rk_priv->rockchip_clks);
> +	if (data->ops && data->ops->clks_disable)
> +		data->ops->clks_disable(priv);
>  free_pltfm:
>  	sdhci_pltfm_free(pdev);
>  	return err;
> @@ -1340,7 +1402,6 @@ static void dwcmshc_remove(struct platform_device *pdev)
>  	struct sdhci_host *host = platform_get_drvdata(pdev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -	struct rk35xx_priv *rk_priv = priv->priv;
>  
>  	pm_runtime_get_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
> @@ -1352,9 +1413,8 @@ static void dwcmshc_remove(struct platform_device *pdev)
>  
>  	clk_disable_unprepare(pltfm_host->clk);
>  	clk_disable_unprepare(priv->bus_clk);
> -	if (rk_priv)
> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> -					   rk_priv->rockchip_clks);
> +	if (priv->ops && priv->ops->clks_disable)
> +		priv->ops->clks_disable(priv);
>  	sdhci_pltfm_free(pdev);
>  }
>  
> @@ -1364,7 +1424,6 @@ static int dwcmshc_suspend(struct device *dev)
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
>  	pm_runtime_resume(dev);
> @@ -1383,9 +1442,8 @@ static int dwcmshc_suspend(struct device *dev)
>  	if (!IS_ERR(priv->bus_clk))
>  		clk_disable_unprepare(priv->bus_clk);
>  
> -	if (rk_priv)
> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> -					   rk_priv->rockchip_clks);
> +	if (priv->ops && priv->ops->clks_disable)
> +		priv->ops->clks_disable(priv);
>  
>  	return ret;
>  }
> @@ -1395,7 +1453,6 @@ static int dwcmshc_resume(struct device *dev)
>  	struct sdhci_host *host = dev_get_drvdata(dev);
>  	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>  	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> -	struct rk35xx_priv *rk_priv = priv->priv;
>  	int ret;
>  
>  	ret = clk_prepare_enable(pltfm_host->clk);
> @@ -1408,29 +1465,27 @@ static int dwcmshc_resume(struct device *dev)
>  			goto disable_clk;
>  	}
>  
> -	if (rk_priv) {
> -		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
> -					      rk_priv->rockchip_clks);
> +	if (priv->ops && priv->ops->clks_enable) {
> +		ret = priv->ops->clks_enable(priv);
>  		if (ret)
>  			goto disable_bus_clk;
>  	}
>  
>  	ret = sdhci_resume_host(host);
>  	if (ret)
> -		goto disable_rockchip_clks;
> +		goto disable_soc_clks;
>  
>  	if (host->mmc->caps2 & MMC_CAP2_CQE) {
>  		ret = cqhci_resume(host->mmc);
>  		if (ret)
> -			goto disable_rockchip_clks;
> +			goto disable_soc_clks;
>  	}
>  
>  	return 0;
>  
> -disable_rockchip_clks:
> -	if (rk_priv)
> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> -					   rk_priv->rockchip_clks);
> +disable_soc_clks:
> +	if (priv->ops && priv->ops->clks_disable)
> +		priv->ops->clks_disable(priv);
>  disable_bus_clk:
>  	if (!IS_ERR(priv->bus_clk))
>  		clk_disable_unprepare(priv->bus_clk);

Re: [PATCH v3 4/4] mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc

[Chen Wang]

On 2024/6/14 18:52, Adrian Hunter wrote:
> On 13/06/24 04:43, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> The current framework is not easily extended to support new SOCs.
>> For example, in the current code we see that the SOC-level
>> structure `rk35xx_priv` and related logic are distributed in
>> functions such as dwcmshc_probe/dwcmshc_remove/dwcmshc_suspend/......,
>> which is inappropriate.
>>
>> The solution is to abstract some possible common operations of soc
>> as dwcmshc platform data. Each soc implements the corresponding callback
>> function according to its own needs.
>> dwcmshc framework is responsible for calling these callback functions
>> in those dwcmshc_xxx functions at proper positions.
>>
>> Signed-off-by: Chen Wang <unicorn_wang@outlook.com>
>> ---
>>   drivers/mmc/host/sdhci-of-dwcmshc.c | 143 +++++++++++++++++++---------
>>   1 file changed, 99 insertions(+), 44 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 38ab755aa044..ebae461019f9 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -206,6 +206,7 @@ struct rk35xx_priv {
>>   	u8 txclk_tapnum;
>>   };
>>   
>> +struct dwcmshc_ops;
>>   struct dwcmshc_priv {
>>   	struct clk	*bus_clk;
>>   	int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
>> @@ -214,6 +215,20 @@ struct dwcmshc_priv {
>>   	void *priv; /* pointer to SoC private stuff */
>>   	u16 delay_line;
>>   	u16 flags;
>> +
>> +	const struct dwcmshc_ops *ops;
> 	const struct dwcmshc_data *data;
>
>> +};
>> +
>> +struct dwcmshc_ops {
>> +	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
>> +	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
>> +	int  (*clks_enable)(struct dwcmshc_priv *dwc_priv);
>> +	void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
>> +};
>> +
>> +struct dwcmshc_data {
>> +	const struct sdhci_pltfm_data *pdata;
>> +	const struct dwcmshc_ops *ops;
>>   };
> Currently, ops and pdata values are unique to an individual
> dwcmshc_data, so it is simpler to put it altogether i.e.
>
> struct dwcmshc_data {
> 	const struct sdhci_pltfm_data pdata;
> 	int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> 	void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> 	int  (*clks_enable)(struct dwcmshc_priv *dwc_priv);
> 	void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
> };

Agree, I will change the code like this.

Thanks,

Chen

>>   /*******************************************************************************
>> @@ -815,6 +830,25 @@ static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_pr
>>   	}
>>   }
>>   
>> +static int rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
>> +{
>> +	struct rk35xx_priv *priv = dwc_priv->priv;
>> +	int ret = 0;
>> +
>> +	if (priv)
>> +		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
>> +	return ret;
>> +}
>> +
>> +static void rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
>> +{
>> +	struct rk35xx_priv *priv = dwc_priv->priv;
>> +
>> +	if (priv)
>> +		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> +					   priv->rockchip_clks);
>> +}
>> +
>>   static void th1520_sdhci_set_phy(struct sdhci_host *host)
>>   {
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>> @@ -1167,30 +1201,65 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
>>   	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
>>   };
>>   
>> +static const struct dwcmshc_ops dwcmshc_rk35xx_ops = {
>> +	.init = rk35xx_init,
>> +	.postinit = rk35xx_postinit,
>> +	.clks_enable = rk35xx_clks_enable,
>> +	.clks_disable = rk35xx_clks_disable,
>> +};
> So this becomes:
>
> static const struct dwcmshc_data sdhci_dwcmshc_rk35xx_pdata = {
> 	.pdata = {
> 		.ops = &sdhci_dwcmshc_rk35xx_ops,
> 		.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
> 			  SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
> 		.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
> 			   SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
> 	},
> 	.init = rk35xx_init,
> 	.postinit = rk35xx_postinit,
> 	.clks_enable = rk35xx_clks_enable,
> 	.clks_disable = rk35xx_clks_disable,
> };
>
> etc
>
>> +
>> +static const struct dwcmshc_ops dwcmshc_th1520_ops = {
>> +	.init = th1520_init,
>> +};
>> +
>> +static const struct dwcmshc_data dwcmshc_cv18xx_data = {
>> +	.pdata = &sdhci_dwcmshc_cv18xx_pdata,
>> +};
>> +
>> +static const struct dwcmshc_data dwcmshc_generic_data = {
>> +	.pdata = &sdhci_dwcmshc_pdata,
>> +};
>> +
>> +static const struct dwcmshc_data dwcmshc_rk35xx_data = {
>> +	.pdata = &sdhci_dwcmshc_rk35xx_pdata,
>> +	.ops = &dwcmshc_rk35xx_ops,
>> +};
>> +
>> +static const struct dwcmshc_data dwcmshc_th1520_data = {
>> +	.pdata = &sdhci_dwcmshc_th1520_pdata,
>> +	.ops = &dwcmshc_th1520_ops,
>> +};
>> +
>> +#ifdef CONFIG_ACPI
>> +static const struct dwcmshc_data dwcmshc_bf3_data = {
>> +	.pdata = &sdhci_dwcmshc_bf3_pdata,
>> +};
>> +#endif
>> +
>>   static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
>>   	{
>>   		.compatible = "rockchip,rk3588-dwcmshc",
>> -		.data = &sdhci_dwcmshc_rk35xx_pdata,
>> +		.data = &dwcmshc_rk35xx_data,
>>   	},
>>   	{
>>   		.compatible = "rockchip,rk3568-dwcmshc",
>> -		.data = &sdhci_dwcmshc_rk35xx_pdata,
>> +		.data = &dwcmshc_rk35xx_data,
>>   	},
>>   	{
>>   		.compatible = "snps,dwcmshc-sdhci",
>> -		.data = &sdhci_dwcmshc_pdata,
>> +		.data = &dwcmshc_generic_data,
>>   	},
>>   	{
>>   		.compatible = "sophgo,cv1800b-dwcmshc",
>> -		.data = &sdhci_dwcmshc_cv18xx_pdata,
>> +		.data = &dwcmshc_cv18xx_data,
>>   	},
>>   	{
>>   		.compatible = "sophgo,sg2002-dwcmshc",
>> -		.data = &sdhci_dwcmshc_cv18xx_pdata,
>> +		.data = &dwcmshc_cv18xx_data,
>>   	},
>>   	{
>>   		.compatible = "thead,th1520-dwcmshc",
>> -		.data = &sdhci_dwcmshc_th1520_pdata,
>> +		.data = &dwcmshc_th1520_data,
>>   	},
>>   	{},
>>   };
>> @@ -1200,7 +1269,7 @@ MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
>>   static const struct acpi_device_id sdhci_dwcmshc_acpi_ids[] = {
>>   	{
>>   		.id = "MLNXBF30",
>> -		.driver_data = (kernel_ulong_t)&sdhci_dwcmshc_bf3_pdata,
>> +		.driver_data = (kernel_ulong_t)&dwcmshc_bf3_data,
>>   	},
>>   	{}
>>   };
>> @@ -1213,18 +1282,17 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   	struct sdhci_pltfm_host *pltfm_host;
>>   	struct sdhci_host *host;
>>   	struct dwcmshc_priv *priv;
>> -	struct rk35xx_priv *rk_priv = NULL;
>> -	const struct sdhci_pltfm_data *pltfm_data;
>> +	const struct dwcmshc_data *data;
>>   	int err;
>>   	u32 extra, caps;
>>   
>> -	pltfm_data = device_get_match_data(&pdev->dev);
>> -	if (!pltfm_data) {
>> +	data = device_get_match_data(&pdev->dev);
>> +	if (!data) {
>>   		dev_err(&pdev->dev, "Error: No device match data found\n");
>>   		return -ENODEV;
>>   	}
>>   
>> -	host = sdhci_pltfm_init(pdev, pltfm_data,
>> +	host = sdhci_pltfm_init(pdev, data->pdata,
>>   				sizeof(struct dwcmshc_priv));
>>   	if (IS_ERR(host))
>>   		return PTR_ERR(host);
>> @@ -1239,6 +1307,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   
>>   	pltfm_host = sdhci_priv(host);
>>   	priv = sdhci_pltfm_priv(pltfm_host);
>> +	priv->ops = data->ops;
> Becomes:
>
> 	priv->data = data;
>
>>   
>>   	if (dev->of_node) {
>>   		pltfm_host->clk = devm_clk_get(dev, "core");
>> @@ -1269,20 +1338,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   	host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
>>   	host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
>>   
>> -	if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
>> -		err = rk35xx_init(&pdev->dev, host, priv);
>> -		if (err)
>> -			goto err_clk;
>> -	}
>> -
>> -	if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
>> -		err = th1520_init(&pdev->dev, host, priv);
>> +	if (data->ops && data->ops->init) {
> Becomes:
>
> 	if (data->init) {
>
> etc
>
>> +		err = data->ops->init(&pdev->dev, host, priv);
>>   		if (err)
>>   			goto err_clk;
>>   	}
>>   
>>   #ifdef CONFIG_ACPI
>> -	if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
>> +	if (data == &dwcmshc_bf3_data)
>>   		sdhci_enable_v4_mode(host);
>>   #endif
>>   
>> @@ -1308,8 +1371,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   		dwcmshc_cqhci_init(host, pdev);
>>   	}
>>   
>> -	if (rk_priv)
>> -		rk35xx_postinit(host, priv);
>> +	if (data->ops && data->ops->postinit)
>> +		data->ops->postinit(host, priv);
>>   
>>   	err = __sdhci_add_host(host);
>>   	if (err)
>> @@ -1327,9 +1390,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
>>   err_clk:
>>   	clk_disable_unprepare(pltfm_host->clk);
>>   	clk_disable_unprepare(priv->bus_clk);
>> -	if (rk_priv)
>> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> -					   rk_priv->rockchip_clks);
>> +	if (data->ops && data->ops->clks_disable)
>> +		data->ops->clks_disable(priv);
>>   free_pltfm:
>>   	sdhci_pltfm_free(pdev);
>>   	return err;
>> @@ -1340,7 +1402,6 @@ static void dwcmshc_remove(struct platform_device *pdev)
>>   	struct sdhci_host *host = platform_get_drvdata(pdev);
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>   	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> -	struct rk35xx_priv *rk_priv = priv->priv;
>>   
>>   	pm_runtime_get_sync(&pdev->dev);
>>   	pm_runtime_disable(&pdev->dev);
>> @@ -1352,9 +1413,8 @@ static void dwcmshc_remove(struct platform_device *pdev)
>>   
>>   	clk_disable_unprepare(pltfm_host->clk);
>>   	clk_disable_unprepare(priv->bus_clk);
>> -	if (rk_priv)
>> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> -					   rk_priv->rockchip_clks);
>> +	if (priv->ops && priv->ops->clks_disable)
>> +		priv->ops->clks_disable(priv);
>>   	sdhci_pltfm_free(pdev);
>>   }
>>   
>> @@ -1364,7 +1424,6 @@ static int dwcmshc_suspend(struct device *dev)
>>   	struct sdhci_host *host = dev_get_drvdata(dev);
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>   	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> -	struct rk35xx_priv *rk_priv = priv->priv;
>>   	int ret;
>>   
>>   	pm_runtime_resume(dev);
>> @@ -1383,9 +1442,8 @@ static int dwcmshc_suspend(struct device *dev)
>>   	if (!IS_ERR(priv->bus_clk))
>>   		clk_disable_unprepare(priv->bus_clk);
>>   
>> -	if (rk_priv)
>> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> -					   rk_priv->rockchip_clks);
>> +	if (priv->ops && priv->ops->clks_disable)
>> +		priv->ops->clks_disable(priv);
>>   
>>   	return ret;
>>   }
>> @@ -1395,7 +1453,6 @@ static int dwcmshc_resume(struct device *dev)
>>   	struct sdhci_host *host = dev_get_drvdata(dev);
>>   	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
>>   	struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
>> -	struct rk35xx_priv *rk_priv = priv->priv;
>>   	int ret;
>>   
>>   	ret = clk_prepare_enable(pltfm_host->clk);
>> @@ -1408,29 +1465,27 @@ static int dwcmshc_resume(struct device *dev)
>>   			goto disable_clk;
>>   	}
>>   
>> -	if (rk_priv) {
>> -		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
>> -					      rk_priv->rockchip_clks);
>> +	if (priv->ops && priv->ops->clks_enable) {
>> +		ret = priv->ops->clks_enable(priv);
>>   		if (ret)
>>   			goto disable_bus_clk;
>>   	}
>>   
>>   	ret = sdhci_resume_host(host);
>>   	if (ret)
>> -		goto disable_rockchip_clks;
>> +		goto disable_soc_clks;
>>   
>>   	if (host->mmc->caps2 & MMC_CAP2_CQE) {
>>   		ret = cqhci_resume(host->mmc);
>>   		if (ret)
>> -			goto disable_rockchip_clks;
>> +			goto disable_soc_clks;
>>   	}
>>   
>>   	return 0;
>>   
>> -disable_rockchip_clks:
>> -	if (rk_priv)
>> -		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
>> -					   rk_priv->rockchip_clks);
>> +disable_soc_clks:
>> +	if (priv->ops && priv->ops->clks_disable)
>> +		priv->ops->clks_disable(priv);
>>   disable_bus_clk:
>>   	if (!IS_ERR(priv->bus_clk))
>>   		clk_disable_unprepare(priv->bus_clk);

Re: [PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[Drew Fustini]

On Thu, Jun 13, 2024 at 09:42:03AM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> When I tried to add a new soc to sdhci-of-dwcmshc, I found that the
> existing driver code could be optimized to facilitate expansion for
> the new soc. You can see another patch [sg2042-dwcmshc], which I am
> working on to add SG2042 to sdhci-of-dwcmshc.
> 
> By the way, although I believe this patch only optimizes the framework
> of the code and does not change the specific logic, simple verification
> is certainly better. Since I don't have rk35xx/th1520 related hardware,
> it would be greatly appreciated if someone could help verify it.
> 
> ---
> 
> Changes in v3:
>   
>   The patch series is based on latest 'next' branch of [mmc-git].
> 
>   Improved the dirvier code as per comments from Adrian Hunter.
>   Define new structure for dwcmshc platform data/ops. In addition, I organized
>   the code and classified the helper functions.
> 
>   Since the file changes were relatively large (though the functional logic did
>   not change much), I split the original patch into four for the convenience of
>   review.
> 
> Changes in v2:
> 
>   Rebased on latest 'next' branch of [mmc-git]. You can simply review or test the
>   patches at the link [2].
> 
> Changes in v1:
> 
>   The patch series is based on v6.9-rc1. You can simply review or test the
>   patches at the link [1].
> 
> Link: https://lore.kernel.org/linux-mmc/cover.1713258948.git.unicorn_wang@outlook.com/ [sg2042-dwcmshc]
> Link: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git [mmc-git]
> Link: https://lore.kernel.org/linux-mmc/cover.1713257181.git.unicorn_wang@outlook.com/ [1]
> Link: https://lore.kernel.org/linux-mmc/cover.1714270290.git.unicorn_wang@outlook.com/ [2]
> 
> ---
> 
> Chen Wang (4):
>   mmc: sdhci-of-dwcmshc: adjust positions of helper routines
>   mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions
>   mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520
>   mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc
> 
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 598 ++++++++++++++++------------
>  1 file changed, 339 insertions(+), 259 deletions(-)
> 
> 
> base-commit: d6cd1206ffaaa890e81f5d1134856d9edd406ec6
> -- 
> 2.25.1
> 

I have tested successfully on top of 6.10-rc3 with the Lichee Pi 4a:

Tested-by: Drew Fustini <dfustini@tenstorrent.com> # TH1520

Re: [PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[Chen Wang]

On 2024/6/16 10:30, Drew Fustini wrote:
> On Thu, Jun 13, 2024 at 09:42:03AM +0800, Chen Wang wrote:
[......]
> I have tested successfully on top of 6.10-rc3 with the Lichee Pi 4a:
>
> Tested-by: Drew Fustini <dfustini@tenstorrent.com> # TH1520

Thank you for your testing effort. I will continue to revise it 
according to other reviewers' suggestions.

Regards,

Chen

Re: [PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[Jisheng Zhang]

On Thu, Jun 13, 2024 at 09:42:03AM +0800, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@outlook.com>
> 
> When I tried to add a new soc to sdhci-of-dwcmshc, I found that the
> existing driver code could be optimized to facilitate expansion for
> the new soc. You can see another patch [sg2042-dwcmshc], which I am
> working on to add SG2042 to sdhci-of-dwcmshc.

Hi Chen,

IMHO, you'd better put the sg2042 support as the last patch of this
series, we want to see why the enhancement is necessary and how does
it help sg2042 upstreaming.

thanks

> 
> By the way, although I believe this patch only optimizes the framework
> of the code and does not change the specific logic, simple verification
> is certainly better. Since I don't have rk35xx/th1520 related hardware,
> it would be greatly appreciated if someone could help verify it.
> 
> ---
> 
> Changes in v3:
>   
>   The patch series is based on latest 'next' branch of [mmc-git].
> 
>   Improved the dirvier code as per comments from Adrian Hunter.
>   Define new structure for dwcmshc platform data/ops. In addition, I organized
>   the code and classified the helper functions.
> 
>   Since the file changes were relatively large (though the functional logic did
>   not change much), I split the original patch into four for the convenience of
>   review.
> 
> Changes in v2:
> 
>   Rebased on latest 'next' branch of [mmc-git]. You can simply review or test the
>   patches at the link [2].
> 
> Changes in v1:
> 
>   The patch series is based on v6.9-rc1. You can simply review or test the
>   patches at the link [1].
> 
> Link: https://lore.kernel.org/linux-mmc/cover.1713258948.git.unicorn_wang@outlook.com/ [sg2042-dwcmshc]
> Link: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git [mmc-git]
> Link: https://lore.kernel.org/linux-mmc/cover.1713257181.git.unicorn_wang@outlook.com/ [1]
> Link: https://lore.kernel.org/linux-mmc/cover.1714270290.git.unicorn_wang@outlook.com/ [2]
> 
> ---
> 
> Chen Wang (4):
>   mmc: sdhci-of-dwcmshc: adjust positions of helper routines
>   mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions
>   mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520
>   mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc
> 
>  drivers/mmc/host/sdhci-of-dwcmshc.c | 598 ++++++++++++++++------------
>  1 file changed, 339 insertions(+), 259 deletions(-)
> 
> 
> base-commit: d6cd1206ffaaa890e81f5d1134856d9edd406ec6
> -- 
> 2.25.1
> 

Re: [PATCH v3 0/4] mmc: sdhci-of-dwcmshc: enhance framework

[Chen Wang]

On 2024/6/17 22:18, Jisheng Zhang wrote:
> On Thu, Jun 13, 2024 at 09:42:03AM +0800, Chen Wang wrote:
>> From: Chen Wang <unicorn_wang@outlook.com>
>>
>> When I tried to add a new soc to sdhci-of-dwcmshc, I found that the
>> existing driver code could be optimized to facilitate expansion for
>> the new soc. You can see another patch [sg2042-dwcmshc], which I am
>> working on to add SG2042 to sdhci-of-dwcmshc.
> Hi Chen,
>
> IMHO, you'd better put the sg2042 support as the last patch of this
> series, we want to see why the enhancement is necessary and how does
> it help sg2042 upstreaming.
>
> thanks

OK, I will consider this in next revision.

Thanks,

Chen

>> By the way, although I believe this patch only optimizes the framework
>> of the code and does not change the specific logic, simple verification
>> is certainly better. Since I don't have rk35xx/th1520 related hardware,
>> it would be greatly appreciated if someone could help verify it.
>>
>> ---
>>
>> Changes in v3:
>>    
>>    The patch series is based on latest 'next' branch of [mmc-git].
>>
>>    Improved the dirvier code as per comments from Adrian Hunter.
>>    Define new structure for dwcmshc platform data/ops. In addition, I organized
>>    the code and classified the helper functions.
>>
>>    Since the file changes were relatively large (though the functional logic did
>>    not change much), I split the original patch into four for the convenience of
>>    review.
>>
>> Changes in v2:
>>
>>    Rebased on latest 'next' branch of [mmc-git]. You can simply review or test the
>>    patches at the link [2].
>>
>> Changes in v1:
>>
>>    The patch series is based on v6.9-rc1. You can simply review or test the
>>    patches at the link [1].
>>
>> Link: https://lore.kernel.org/linux-mmc/cover.1713258948.git.unicorn_wang@outlook.com/ [sg2042-dwcmshc]
>> Link: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git [mmc-git]
>> Link: https://lore.kernel.org/linux-mmc/cover.1713257181.git.unicorn_wang@outlook.com/ [1]
>> Link: https://lore.kernel.org/linux-mmc/cover.1714270290.git.unicorn_wang@outlook.com/ [2]
>>
>> ---
>>
>> Chen Wang (4):
>>    mmc: sdhci-of-dwcmshc: adjust positions of helper routines
>>    mmc: sdhci-of-dwcmshc: unify the naming of soc helper functions
>>    mmc: sdhci-of-dwcmshc: extract init function for rk35xx/th1520
>>    mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc
>>
>>   drivers/mmc/host/sdhci-of-dwcmshc.c | 598 ++++++++++++++++------------
>>   1 file changed, 339 insertions(+), 259 deletions(-)
>>
>>
>> base-commit: d6cd1206ffaaa890e81f5d1134856d9edd406ec6
>> -- 
>> 2.25.1
>>