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. 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 v2:
Rebased on latest 'next' branch of [mmc-git].
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/[email protected]/ [sg2042-dwcmshc]
Link: git://git.kernel.org/pub/scm/linux/kernel/git/ulfh/mmc.git [mmc-git]
Link: https://lore.kernel.org/linux-mmc/[email protected]/ [1]
---
Chen Wang (1):
mmc: sdhci-of-dwcmshc: add callback functions for dwcmshc_priv
drivers/mmc/host/sdhci-of-dwcmshc.c | 152 +++++++++++++++++-----------
1 file changed, 91 insertions(+), 61 deletions(-)
base-commit: e38063b94324bc1409a29d699c73938c3d008126
--
2.25.1
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 | 152 +++++++++++++++++-----------
1 file changed, 91 insertions(+), 61 deletions(-)
diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
index 39edf04fedcf..525f954bcb65 100644
--- a/drivers/mmc/host/sdhci-of-dwcmshc.c
+++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
@@ -214,6 +214,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);
};
/*
@@ -1033,10 +1037,40 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
}
-static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+static int dwcmshc_rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
{
- int err;
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 dwcmshc_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 dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
+static int dwcmshc_rk35xx_init(struct device *dev,
+ struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
+{
+ int err;
+ 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)) {
@@ -1071,6 +1105,11 @@ 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 = 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;
}
@@ -1088,6 +1127,35 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_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;
+
+ /*
+ * 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 const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
{
.compatible = "rockchip,rk3588-dwcmshc",
@@ -1134,7 +1202,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, caps;
@@ -1191,46 +1258,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 = 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
@@ -1260,8 +1296,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
dwcmshc_cqhci_init(host, pdev);
}
- if (rk_priv)
- dwcmshc_rk35xx_postinit(host, priv);
+ if (priv->soc_postinit)
+ priv->soc_postinit(host, priv);
err = __sdhci_add_host(host);
if (err)
@@ -1279,9 +1315,9 @@ 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;
@@ -1303,7 +1339,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);
@@ -1315,9 +1350,9 @@ 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->soc_clks_disable)
+ priv->soc_clks_disable(priv);
+
sdhci_pltfm_free(pdev);
}
@@ -1327,7 +1362,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);
@@ -1346,9 +1380,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;
}
@@ -1358,7 +1391,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);
@@ -1371,29 +1403,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->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;
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->soc_clks_disable)
+ priv->soc_clks_disable(priv);
disable_bus_clk:
if (!IS_ERR(priv->bus_clk))
clk_disable_unprepare(priv->bus_clk);
--
2.25.1
On Sun, Apr 28, 2024 at 10:32:41AM +0800, Chen Wang wrote:
> 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]>
I have tested this with the eMMC and microSD on the Lichee Pi 4a which
has the T-Head TH1520 SoC.
Tested-by: Drew Fustini <[email protected]> # TH1520
Thanks,
Drew
On 28/04/24 05:32, Chen Wang wrote:
> 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 | 152 +++++++++++++++++-----------
> 1 file changed, 91 insertions(+), 61 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 39edf04fedcf..525f954bcb65 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -214,6 +214,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);
Normally the ops would be part of platform data. For example,
sdhci-of-arasan.c has:
struct sdhci_arasan_of_data {
const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
const struct sdhci_pltfm_data *pdata;
const struct sdhci_arasan_clk_ops *clk_ops;
};
And then:
static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
.soc_ctl_map = &rk3399_soc_ctl_map,
.pdata = &sdhci_arasan_cqe_pdata,
.clk_ops = &arasan_clk_ops,
};
etc
static const struct of_device_id sdhci_arasan_of_match[] = {
/* SoC-specific compatible strings w/ soc_ctl_map */
{
.compatible = "rockchip,rk3399-sdhci-5.1",
.data = &sdhci_arasan_rk3399_data,
},
etc
So, say:
struct dwcmshc_pltfm_data {
const struct sdhci_pltfm_data *pltfm_data;
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);
}
Or if the ops are mostly the same, it might be more convenient to
have them in their own structure:
struct dwcmshc_pltfm_data {
const struct sdhci_pltfm_data *pltfm_data;
const struct dwcmshc_ops *ops;
}
> };
>
> /*
> @@ -1033,10 +1037,40 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
> host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
> }
>
> -static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> +static int dwcmshc_rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
> {
> - int err;
> 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 dwcmshc_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 dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
Avoid forward declarations if possible. If necessary, it is
preferable to move the function definition.
> +static int dwcmshc_rk35xx_init(struct device *dev,
> + struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
This patch looks like it might be doing too much. Please consider
splitting it so reorganising the code is separate from adding the
callbacks.
> +{
> + int err;
> + 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)) {
> @@ -1071,6 +1105,11 @@ 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 = 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;
> }
>
> @@ -1088,6 +1127,35 @@ static void dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_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;
> +
> + /*
> + * 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 const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> {
> .compatible = "rockchip,rk3588-dwcmshc",
> @@ -1134,7 +1202,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, caps;
> @@ -1191,46 +1258,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 = 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
> @@ -1260,8 +1296,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
> dwcmshc_cqhci_init(host, pdev);
> }
>
> - if (rk_priv)
> - dwcmshc_rk35xx_postinit(host, priv);
> + if (priv->soc_postinit)
> + priv->soc_postinit(host, priv);
>
> err = __sdhci_add_host(host);
> if (err)
> @@ -1279,9 +1315,9 @@ 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;
> @@ -1303,7 +1339,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);
> @@ -1315,9 +1350,9 @@ 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->soc_clks_disable)
> + priv->soc_clks_disable(priv);
> +
> sdhci_pltfm_free(pdev);
> }
>
> @@ -1327,7 +1362,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);
> @@ -1346,9 +1380,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;
> }
> @@ -1358,7 +1391,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);
> @@ -1371,29 +1403,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->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;
>
> 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->soc_clks_disable)
> + priv->soc_clks_disable(priv);
> disable_bus_clk:
> if (!IS_ERR(priv->bus_clk))
> clk_disable_unprepare(priv->bus_clk);
Thank you Drew.
FYI, as per some inputs from Adrian, I will try to imrove the code and
send a new patch.
On 2024/4/29 9:40, Drew Fustini wrote:
> On Sun, Apr 28, 2024 at 10:32:41AM +0800, Chen Wang wrote:
>> 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]>
> I have tested this with the eMMC and microSD on the Lichee Pi 4a which
> has the T-Head TH1520 SoC.
>
> Tested-by: Drew Fustini <[email protected]> # TH1520
>
> Thanks,
> Drew
On 2024/4/29 15:08, Adrian Hunter wrote:
> On 28/04/24 05:32, Chen Wang wrote:
>> 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 | 152 +++++++++++++++++-----------
>> 1 file changed, 91 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 39edf04fedcf..525f954bcb65 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -214,6 +214,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);
> Normally the ops would be part of platform data. For example,
> sdhci-of-arasan.c has:
>
> struct sdhci_arasan_of_data {
> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> const struct sdhci_pltfm_data *pdata;
> const struct sdhci_arasan_clk_ops *clk_ops;
> };
>
> And then:
>
> static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
> .soc_ctl_map = &rk3399_soc_ctl_map,
> .pdata = &sdhci_arasan_cqe_pdata,
> .clk_ops = &arasan_clk_ops,
> };
> etc
>
> static const struct of_device_id sdhci_arasan_of_match[] = {
> /* SoC-specific compatible strings w/ soc_ctl_map */
> {
> .compatible = "rockchip,rk3399-sdhci-5.1",
> .data = &sdhci_arasan_rk3399_data,
> },
> etc
>
> So, say:
>
> struct dwcmshc_pltfm_data {
> const struct sdhci_pltfm_data *pltfm_data;
> 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);
> }
>
> Or if the ops are mostly the same, it might be more convenient to
> have them in their own structure:
>
> struct dwcmshc_pltfm_data {
> const struct sdhci_pltfm_data *pltfm_data;
> const struct dwcmshc_ops *ops;
> }
Thanks for your suggestion and it looks more formal, I will investigate
and provide a new revision.
>> };
>>
>> /*
>> @@ -1033,10 +1037,40 @@ static void dwcmshc_cqhci_init(struct sdhci_host *host, struct platform_device *
>> host->mmc->caps2 &= ~(MMC_CAP2_CQE | MMC_CAP2_CQE_DCMD);
>> }
>>
>> -static int dwcmshc_rk35xx_init(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
>> +static int dwcmshc_rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
>> {
>> - int err;
>> 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 dwcmshc_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 dwcmshc_rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> Avoid forward declarations if possible. If necessary, it is
> preferable to move the function definition.
Yes, I add this declaration just want to make diff look clearer :).
Anyway, move this postinit to the front should be better.
>> +static int dwcmshc_rk35xx_init(struct device *dev,
>> + struct sdhci_host *host, struct dwcmshc_priv *dwc_priv)
> This patch looks like it might be doing too much. Please consider
> splitting it so reorganising the code is separate from adding the
> callbacks.
Sure, will do like this. Thanks.
[......]
On 2024/4/29 15:08, Adrian Hunter wrote:
> On 28/04/24 05:32, Chen Wang wrote:
>> 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 | 152 +++++++++++++++++-----------
>> 1 file changed, 91 insertions(+), 61 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> index 39edf04fedcf..525f954bcb65 100644
>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>> @@ -214,6 +214,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);
> Normally the ops would be part of platform data. For example,
> sdhci-of-arasan.c has:
>
> struct sdhci_arasan_of_data {
> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
> const struct sdhci_pltfm_data *pdata;
> const struct sdhci_arasan_clk_ops *clk_ops;
> };
>
> And then:
>
> static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
> .soc_ctl_map = &rk3399_soc_ctl_map,
> .pdata = &sdhci_arasan_cqe_pdata,
> .clk_ops = &arasan_clk_ops,
> };
> etc
>
> static const struct of_device_id sdhci_arasan_of_match[] = {
> /* SoC-specific compatible strings w/ soc_ctl_map */
> {
> .compatible = "rockchip,rk3399-sdhci-5.1",
> .data = &sdhci_arasan_rk3399_data,
> },
> etc
>
> So, say:
>
> struct dwcmshc_pltfm_data {
> const struct sdhci_pltfm_data *pltfm_data;
> 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);
> }
>
> Or if the ops are mostly the same, it might be more convenient to
> have them in their own structure:
>
> struct dwcmshc_pltfm_data {
> const struct sdhci_pltfm_data *pltfm_data;
> const struct dwcmshc_ops *ops;
> }
hi, Adrian,
I thought about it for a while, and I would like to continue discussing
this issue as follows.
I feel like it would be simpler to put it at the dwcmshc_priv level
based on the ops involved in the code so far. Judging from the SOCs
currently supported by dwcmshc, the ops I abstracted only operate data
below the dwcmshc_priv level, and these ops are not used by most SOCs.
- postinit: only required by rk35xx
- init: involves rk35xx and th1520, and the new soc(sg2042) I want to add.
- clks_enable/clks_disable: only rk35xx and the sg2042 I want to add
In particular, for dwcmshc_suspend/dwcmshc_resume, we have already
obtained dwcmshc_priv. If ops is to be placed at the platformdata level,
we have to use device_get_match_data to obtain data again, which feels a
bit unnecessary.
What do you think?
Thanks,
Chen
[......]
On 9/05/24 05:17, Chen Wang wrote:
>
> On 2024/4/29 15:08, Adrian Hunter wrote:
>> On 28/04/24 05:32, Chen Wang wrote:
>>> 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 | 152 +++++++++++++++++-----------
>>> 1 file changed, 91 insertions(+), 61 deletions(-)
>>>
>>> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> index 39edf04fedcf..525f954bcb65 100644
>>> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
>>> @@ -214,6 +214,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);
>> Normally the ops would be part of platform data. For example,
>> sdhci-of-arasan.c has:
>>
>> struct sdhci_arasan_of_data {
>> const struct sdhci_arasan_soc_ctl_map *soc_ctl_map;
>> const struct sdhci_pltfm_data *pdata;
>> const struct sdhci_arasan_clk_ops *clk_ops;
>> };
>>
>> And then:
>>
>> static struct sdhci_arasan_of_data sdhci_arasan_rk3399_data = {
>> .soc_ctl_map = &rk3399_soc_ctl_map,
>> .pdata = &sdhci_arasan_cqe_pdata,
>> .clk_ops = &arasan_clk_ops,
>> };
>> etc
>>
>> static const struct of_device_id sdhci_arasan_of_match[] = {
>> /* SoC-specific compatible strings w/ soc_ctl_map */
>> {
>> .compatible = "rockchip,rk3399-sdhci-5.1",
>> .data = &sdhci_arasan_rk3399_data,
>> },
>> etc
>>
>> So, say:
>>
>> struct dwcmshc_pltfm_data {
>> const struct sdhci_pltfm_data *pltfm_data;
>> 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);
>> }
>>
>> Or if the ops are mostly the same, it might be more convenient to
>> have them in their own structure:
>>
>> struct dwcmshc_pltfm_data {
>> const struct sdhci_pltfm_data *pltfm_data;
>> const struct dwcmshc_ops *ops;
>> }
>
> hi, Adrian,
>
> I thought about it for a while, and I would like to continue discussing this issue as follows.
>
> I feel like it would be simpler to put it at the dwcmshc_priv level based on the ops involved in the code so far. Judging from the SOCs currently supported by dwcmshc, the ops I abstracted only operate data below the dwcmshc_priv level, and these ops are not used by most SOCs.
> - postinit: only required by rk35xx
> - init: involves rk35xx and th1520, and the new soc(sg2042) I want to add.
> - clks_enable/clks_disable: only rk35xx and the sg2042 I want to add
>
> In particular, for dwcmshc_suspend/dwcmshc_resume, we have already obtained dwcmshc_priv. If ops is to be placed at the platformdata level, we have to use device_get_match_data to obtain data again, which feels a bit unnecessary.
>
> What do you think?
In sdhci-of-arasan.c, ops are copied from platform data to
driver private data e.g.
static int sdhci_arasan_probe(struct platform_device *pdev)
{
...
struct sdhci_arasan_data *sdhci_arasan;
const struct sdhci_arasan_of_data *data;
data = of_device_get_match_data(dev);
if (!data)
return -EINVAL;
...
sdhci_arasan = sdhci_pltfm_priv(pltfm_host);
...
sdhci_arasan->clk_ops = data->clk_ops;
Alternatively, a pointer could be put in driver private data
to point to platform data.