| Message ID | 5bb708cc830684676dede5f44ee22c7fd03300b7.1714270290.git.unicorn_wang@outlook.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mmc: sdhci-of-dwcmshc: enhance framework | expand |
On Sun, Apr 28, 2024 at 10:32:41AM +0800, 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 > 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 <unicorn_wang@outlook.com> 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 <dfustini@tenstorrent.com> # TH1520 Thanks, Drew
On 28/04/24 05:32, 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 > 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 <unicorn_wang@outlook.com> > --- > 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 <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 >> 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 <unicorn_wang@outlook.com> > 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 <dfustini@tenstorrent.com> # TH1520 > > Thanks, > Drew
On 2024/4/29 15:08, Adrian Hunter wrote: > On 28/04/24 05:32, 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 >> 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 <unicorn_wang@outlook.com> >> --- >> 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 <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 >> 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 <unicorn_wang@outlook.com> >> --- >> 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 <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 >>> 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 <unicorn_wang@outlook.com> >>> --- >>> 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.
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);