mmc: sdhci-of-dwcmshc: enhance framework

PATCH Superseded Chen Wang 3 participants 5 messages Apr 16, 2024 – Apr 22, 2024

Subsystems (2)

MMC, SD and SDIO Secure digital host controller interface (SDHCI) driver

lore

Share & tools

lore

Chen Wang

author

2024-04-16 09:43 UTC #0 lore raw

cover mmc: sdhci-of-dwcmshc: enhance framework

From: Chen Wang <[email protected]>

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.

Thanks,
Chen

---

Chen Wang (1):
mmc: sdhci-of-dwcmshc: add callback framework for expansion

 drivers/mmc/host/sdhci-of-dwcmshc.c | 185 ++++++++++++++++------------
 1 file changed, 107 insertions(+), 78 deletions(-)

base-commit: 4cece764965020c22cff7665b18a012006359095

Signature · 1 line

2.25.1

Chen Wang

author

2024-04-16 09:44 UTC #1 lore raw

PATCH 1/1 mmc: sdhci-of-dwcmshc: add callback framework for expansion

From: Chen Wang <[email protected]>

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 into virtual members of dwcmshc_priv. Each soc implements its own corresponding callback function and registers it in init function. dwcmshc framework is responsible for calling these callback functions in those dwcmshc_xxx functions.

Signed-off-by Chen Wang <[email protected]>

---
 drivers/mmc/host/sdhci-of-dwcmshc.c | 185 ++++++++++++++++------------
 1 file changed, 107 insertions(+), 78 deletions(-)

drivers/mmc/host/sdhci-of-dwcmshc.c +107 ~~-78~~

index ab4b964d4058..1ac8537361de 100644
@@ -200,6 +200,10 @@ struct dwcmshc_priv {
 	void *priv; /* pointer to SoC private stuff */
 	u16 delay_line;
 	u16 flags;
+
+	void (*soc_postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
+	int (*soc_clks_enable)(struct dwcmshc_priv *dwc_priv);
+	void (*soc_clks_disable)(struct dwcmshc_priv *dwc_priv);
 };
 
 /*

@@ -758,37 +762,81 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
 	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
 };
 
-static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+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 int dwcmshc_rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
+{
+	struct rk35xx_priv *soc = dwc_priv->priv;
+	int ret = 0;
+
+	if (soc)
+		ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, soc->rockchip_clks);
+	return ret;
+}
+
+static void dwcmshc_rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
+{
+	struct rk35xx_priv *rk_priv = dwc_priv->priv;
+
+	if (rk_priv)
+		clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
+					   rk_priv->rockchip_clks);
+}
+
+static int dwcmshc_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 *rk_priv;
 
-	priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
-	if (IS_ERR(priv->reset)) {
-		err = PTR_ERR(priv->reset);
+	rk_priv = devm_kzalloc(dev, sizeof(struct rk35xx_priv), GFP_KERNEL);
+	if (!rk_priv)
+		return -ENOMEM;
+
+	if (of_device_is_compatible(dev->of_node, "rockchip,rk3588-dwcmshc"))
+		rk_priv->devtype = DWCMSHC_RK3588;
+	else
+		rk_priv->devtype = DWCMSHC_RK3568;
+
+	rk_priv->reset = devm_reset_control_array_get_optional_exclusive(mmc_dev(host->mmc));
+	if (IS_ERR(rk_priv->reset)) {
+		err = PTR_ERR(rk_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";
+	rk_priv->rockchip_clks[0].id = "axi";
+	rk_priv->rockchip_clks[1].id = "block";
+	rk_priv->rockchip_clks[2].id = "timer";
 	err = devm_clk_bulk_get_optional(mmc_dev(host->mmc), RK35xx_MAX_CLKS,
-					 priv->rockchip_clks);
+					 rk_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);
+	err = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, rk_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;
+				&rk_priv->txclk_tapnum))
+		rk_priv->txclk_tapnum = DLL_TXCLK_TAPNUM_DEFAULT;
 
 	/* Disable cmd conflict check */
 	sdhci_writel(host, 0x0, dwc_priv->vendor_specific_area1 + DWCMSHC_HOST_CTRL3);

@@ -796,21 +844,41 @@ static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc
 	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_TXCLK);
 	sdhci_writel(host, 0, DWCMSHC_EMMC_DLL_STRBIN);
 
+	dwc_priv->priv = rk_priv;
+	dwc_priv->soc_postinit = dwcmshc_rk35xx_postinit;
+	dwc_priv->soc_clks_enable = dwcmshc_rk35xx_clks_enable;
+	dwc_priv->soc_clks_disable = dwcmshc_rk35xx_clks_disable;
+
 	return 0;
 }
 
-static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static int dwcmshc_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;
+
 	/*
-	 * Don't support highspeed bus mode with low clk speed as we
-	 * cannot use DLL for this condition.
+	 * 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 (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);
+	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 const struct of_device_id sdhci_dwcmshc_dt_ids[] = {

@@ -859,7 +927,6 @@ 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;
 	int err;
 	u32 extra;

@@ -915,46 +982,15 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
 
 	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 = dwcmshc_rk35xx_init(host, priv);
+		err = dwcmshc_rk35xx_init(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 = dwcmshc_th1520_init(dev, host, priv);
+		if (err)
+			goto err_clk;
 	}
 
 #ifdef CONFIG_ACPI

@@ -972,8 +1008,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
 	if (err)
 		goto err_rpm;
 
-	if (rk_priv)
-		dwcmshc_rk35xx_postinit(host, priv);
+	if (priv->soc_postinit)
+		priv->soc_postinit(host, priv);
 
 	err = __sdhci_add_host(host);
 	if (err)

@@ -991,9 +1027,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 (priv->soc_clks_disable)
+		priv->soc_clks_disable(priv);
 free_pltfm:
 	sdhci_pltfm_free(pdev);
 	return err;

@@ -1004,15 +1039,13 @@ 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;
 
 	sdhci_remove_host(host, 0);
 
 	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->soc_clks_disable)
+		priv->soc_clks_disable(priv);
 	sdhci_pltfm_free(pdev);
 }

@@ -1022,7 +1055,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);

@@ -1035,9 +1067,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->soc_clks_disable)
+		priv->soc_clks_disable(priv);
 
 	return ret;
 }

@@ -1047,7 +1078,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);

@@ -1060,23 +1090,22 @@ 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->soc_clks_enable) {
+		ret = priv->soc_clks_enable(priv);
 		if (ret)
 			goto disable_bus_clk;
 	}
 
 	ret = sdhci_resume_host(host);
 	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->soc_clks_disable)
+		priv->soc_clks_disable(priv);
+
 disable_bus_clk:
 	if (!IS_ERR(priv->bus_clk))
 		clk_disable_unprepare(priv->bus_clk);

Signature · 1 line

2.25.1

Chen Wang

2024-04-22 09:02 UTC #2 lore raw

In reply to Chen Wang

hi， any comments on this patch?

Another patch [1] has dependency on this one, so I would hope someone can review and comment this one first, thanks.

Link：https://lore.kernel.org/linux-riscv/[email protected]/ [1]

Quoted 19 lines · Chen Wang

From: Chen Wang <[email protected]>
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.
Thanks,
Chen
---
Chen Wang (1):
mmc: sdhci-of-dwcmshc: add callback framework for expansion
  drivers/mmc/host/sdhci-of-dwcmshc.c | 185 ++++++++++++++++------------
  1 file changed, 107 insertions(+), 78 deletions(-)
base-commit: 4cece764965020c22cff7665b18a012006359095

Adrian Hunter

maintainer

2024-04-22 10:44 UTC #3 lore raw

Quoted 3 lines · Chen Wang

hi， any comments on this patch?
Another patch [1] has dependency on this one, so I would hope someone can review and comment this one first, thanks.

Does not apply. Please re-base on mmc 'next' branch of:

git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git

Quoted 23 lines

Link：https://lore.kernel.org/linux-riscv/[email protected]/ [1]
Quoted 19 lines · Chen Wang
From: Chen Wang <[email protected]>
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.
Thanks,
Chen
---
Chen Wang (1):
mmc: sdhci-of-dwcmshc: add callback framework for expansion
drivers/mmc/host/sdhci-of-dwcmshc.c | 185 ++++++++++++++++------------
  1 file changed, 107 insertions(+), 78 deletions(-)
base-commit: 4cece764965020c22cff7665b18a012006359095

Chen Wang

2024-04-22 12:53 UTC #4 lore raw

Quoted 7 lines · Adrian Hunter

Quoted 3 lines · Chen Wang
hi， any comments on this patch?
Another patch [1] has dependency on this one, so I would hope someone can review and comment this one first, thanks.
Does not apply. Please re-base on mmc 'next' branch of:
git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git

Quoted 22 lines

Quoted 22 lines
Link：https://lore.kernel.org/linux-riscv/[email protected]/ [1]
Quoted 19 lines · Chen Wang
From: Chen Wang <[email protected]>
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.
Thanks,
Chen
---
Chen Wang (1):
mmc: sdhci-of-dwcmshc: add callback framework for expansion
   drivers/mmc/host/sdhci-of-dwcmshc.c | 185 ++++++++++++++++------------
   1 file changed, 107 insertions(+), 78 deletions(-)
base-commit: 4cece764965020c22cff7665b18a012006359095