| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ed900af1-f090-49a9-bc7e-363a28a4ac2b@intel.com>
Date: Mon, 29 Apr 2024 10:08:04 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Chen Wang <unicornxw@...il.com>, ulf.hansson@...aro.org,
linux-mmc@...r.kernel.org, linux-kernel@...r.kernel.org, jszhang@...nel.org,
dfustini@...libre.com, yifeng.zhao@...k-chips.com, shawn.lin@...k-chips.com,
chao.wei@...hgo.com, haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com,
tingzhu.wang@...hgo.com, guoren@...nel.org, inochiama@...look.com,
unicorn_wang@...look.com
Subject: Re: [PATCH v2 1/1] mmc: sdhci-of-dwcmshc: add callback functions for
dwcmshc_priv
On 28/04/24 05:32, Chen Wang wrote:
> From: Chen Wang <unicorn_wang@...look.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@...look.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);
Powered by blists - more mailing lists