| 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: <25910cae-b29e-49a5-86d2-6da571664b4a@intel.com>
Date: Fri, 14 Jun 2024 13:52:26 +0300
From: Adrian Hunter <adrian.hunter@...el.com>
To: Chen Wang <unicornxw@...il.com>, aou@...s.berkeley.edu,
conor+dt@...nel.org, guoren@...nel.org, inochiama@...look.com,
jszhang@...nel.org, krzysztof.kozlowski+dt@...aro.org, palmer@...belt.com,
paul.walmsley@...ive.com, robh@...nel.org, ulf.hansson@...aro.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-mmc@...r.kernel.org, linux-riscv@...ts.infradead.org,
chao.wei@...hgo.com, haijiao.liu@...hgo.com, xiaoguang.xing@...hgo.com,
tingzhu.wang@...hgo.com
Cc: Chen Wang <unicorn_wang@...look.com>
Subject: Re: [PATCH v3 4/4] mmc: sdhci-of-dwcmshc: add callback functions for
dwcmshc
On 13/06/24 04:43, 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
> as dwcmshc platform data. Each soc implements the corresponding callback
> function according to its own needs.
> dwcmshc framework is responsible for calling these callback functions
> in those dwcmshc_xxx functions at proper positions.
>
> Signed-off-by: Chen Wang <unicorn_wang@...look.com>
> ---
> drivers/mmc/host/sdhci-of-dwcmshc.c | 143 +++++++++++++++++++---------
> 1 file changed, 99 insertions(+), 44 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 38ab755aa044..ebae461019f9 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
> @@ -206,6 +206,7 @@ struct rk35xx_priv {
> u8 txclk_tapnum;
> };
>
> +struct dwcmshc_ops;
> struct dwcmshc_priv {
> struct clk *bus_clk;
> int vendor_specific_area1; /* P_VENDOR_SPECIFIC_AREA1 reg */
> @@ -214,6 +215,20 @@ struct dwcmshc_priv {
> void *priv; /* pointer to SoC private stuff */
> u16 delay_line;
> u16 flags;
> +
> + const struct dwcmshc_ops *ops;
const struct dwcmshc_data *data;
> +};
> +
> +struct dwcmshc_ops {
> + int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> + void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
> + int (*clks_enable)(struct dwcmshc_priv *dwc_priv);
> + void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
> +};
> +
> +struct dwcmshc_data {
> + const struct sdhci_pltfm_data *pdata;
> + const struct dwcmshc_ops *ops;
> };
Currently, ops and pdata values are unique to an individual
dwcmshc_data, so it is simpler to put it altogether i.e.
struct dwcmshc_data {
const struct sdhci_pltfm_data pdata;
int (*init)(struct device *dev, struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
void (*postinit)(struct sdhci_host *host, struct dwcmshc_priv *dwc_priv);
int (*clks_enable)(struct dwcmshc_priv *dwc_priv);
void (*clks_disable)(struct dwcmshc_priv *dwc_priv);
};
>
> /*******************************************************************************
> @@ -815,6 +830,25 @@ static void rk35xx_postinit(struct sdhci_host *host, struct dwcmshc_priv *dwc_pr
> }
> }
>
> +static int rk35xx_clks_enable(struct dwcmshc_priv *dwc_priv)
> +{
> + struct rk35xx_priv *priv = dwc_priv->priv;
> + int ret = 0;
> +
> + if (priv)
> + ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS, priv->rockchip_clks);
> + return ret;
> +}
> +
> +static void rk35xx_clks_disable(struct dwcmshc_priv *dwc_priv)
> +{
> + struct rk35xx_priv *priv = dwc_priv->priv;
> +
> + if (priv)
> + clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> + priv->rockchip_clks);
> +}
> +
> static void th1520_sdhci_set_phy(struct sdhci_host *host)
> {
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> @@ -1167,30 +1201,65 @@ static const struct sdhci_pltfm_data sdhci_dwcmshc_cv18xx_pdata = {
> .quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> };
>
> +static const struct dwcmshc_ops dwcmshc_rk35xx_ops = {
> + .init = rk35xx_init,
> + .postinit = rk35xx_postinit,
> + .clks_enable = rk35xx_clks_enable,
> + .clks_disable = rk35xx_clks_disable,
> +};
So this becomes:
static const struct dwcmshc_data sdhci_dwcmshc_rk35xx_pdata = {
.pdata = {
.ops = &sdhci_dwcmshc_rk35xx_ops,
.quirks = SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN |
SDHCI_QUIRK_BROKEN_TIMEOUT_VAL,
.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN |
SDHCI_QUIRK2_CLOCK_DIV_ZERO_BROKEN,
},
.init = rk35xx_init,
.postinit = rk35xx_postinit,
.clks_enable = rk35xx_clks_enable,
.clks_disable = rk35xx_clks_disable,
};
etc
> +
> +static const struct dwcmshc_ops dwcmshc_th1520_ops = {
> + .init = th1520_init,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_cv18xx_data = {
> + .pdata = &sdhci_dwcmshc_cv18xx_pdata,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_generic_data = {
> + .pdata = &sdhci_dwcmshc_pdata,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_rk35xx_data = {
> + .pdata = &sdhci_dwcmshc_rk35xx_pdata,
> + .ops = &dwcmshc_rk35xx_ops,
> +};
> +
> +static const struct dwcmshc_data dwcmshc_th1520_data = {
> + .pdata = &sdhci_dwcmshc_th1520_pdata,
> + .ops = &dwcmshc_th1520_ops,
> +};
> +
> +#ifdef CONFIG_ACPI
> +static const struct dwcmshc_data dwcmshc_bf3_data = {
> + .pdata = &sdhci_dwcmshc_bf3_pdata,
> +};
> +#endif
> +
> static const struct of_device_id sdhci_dwcmshc_dt_ids[] = {
> {
> .compatible = "rockchip,rk3588-dwcmshc",
> - .data = &sdhci_dwcmshc_rk35xx_pdata,
> + .data = &dwcmshc_rk35xx_data,
> },
> {
> .compatible = "rockchip,rk3568-dwcmshc",
> - .data = &sdhci_dwcmshc_rk35xx_pdata,
> + .data = &dwcmshc_rk35xx_data,
> },
> {
> .compatible = "snps,dwcmshc-sdhci",
> - .data = &sdhci_dwcmshc_pdata,
> + .data = &dwcmshc_generic_data,
> },
> {
> .compatible = "sophgo,cv1800b-dwcmshc",
> - .data = &sdhci_dwcmshc_cv18xx_pdata,
> + .data = &dwcmshc_cv18xx_data,
> },
> {
> .compatible = "sophgo,sg2002-dwcmshc",
> - .data = &sdhci_dwcmshc_cv18xx_pdata,
> + .data = &dwcmshc_cv18xx_data,
> },
> {
> .compatible = "thead,th1520-dwcmshc",
> - .data = &sdhci_dwcmshc_th1520_pdata,
> + .data = &dwcmshc_th1520_data,
> },
> {},
> };
> @@ -1200,7 +1269,7 @@ MODULE_DEVICE_TABLE(of, sdhci_dwcmshc_dt_ids);
> static const struct acpi_device_id sdhci_dwcmshc_acpi_ids[] = {
> {
> .id = "MLNXBF30",
> - .driver_data = (kernel_ulong_t)&sdhci_dwcmshc_bf3_pdata,
> + .driver_data = (kernel_ulong_t)&dwcmshc_bf3_data,
> },
> {}
> };
> @@ -1213,18 +1282,17 @@ static int dwcmshc_probe(struct platform_device *pdev)
> struct sdhci_pltfm_host *pltfm_host;
> struct sdhci_host *host;
> struct dwcmshc_priv *priv;
> - struct rk35xx_priv *rk_priv = NULL;
> - const struct sdhci_pltfm_data *pltfm_data;
> + const struct dwcmshc_data *data;
> int err;
> u32 extra, caps;
>
> - pltfm_data = device_get_match_data(&pdev->dev);
> - if (!pltfm_data) {
> + data = device_get_match_data(&pdev->dev);
> + if (!data) {
> dev_err(&pdev->dev, "Error: No device match data found\n");
> return -ENODEV;
> }
>
> - host = sdhci_pltfm_init(pdev, pltfm_data,
> + host = sdhci_pltfm_init(pdev, data->pdata,
> sizeof(struct dwcmshc_priv));
> if (IS_ERR(host))
> return PTR_ERR(host);
> @@ -1239,6 +1307,7 @@ static int dwcmshc_probe(struct platform_device *pdev)
>
> pltfm_host = sdhci_priv(host);
> priv = sdhci_pltfm_priv(pltfm_host);
> + priv->ops = data->ops;
Becomes:
priv->data = data;
>
> if (dev->of_node) {
> pltfm_host->clk = devm_clk_get(dev, "core");
> @@ -1269,20 +1338,14 @@ static int dwcmshc_probe(struct platform_device *pdev)
> host->mmc_host_ops.hs400_enhanced_strobe = dwcmshc_hs400_enhanced_strobe;
> host->mmc_host_ops.execute_tuning = dwcmshc_execute_tuning;
>
> - if (pltfm_data == &sdhci_dwcmshc_rk35xx_pdata) {
> - err = rk35xx_init(&pdev->dev, host, priv);
> - if (err)
> - goto err_clk;
> - }
> -
> - if (pltfm_data == &sdhci_dwcmshc_th1520_pdata) {
> - err = th1520_init(&pdev->dev, host, priv);
> + if (data->ops && data->ops->init) {
Becomes:
if (data->init) {
etc
> + err = data->ops->init(&pdev->dev, host, priv);
> if (err)
> goto err_clk;
> }
>
> #ifdef CONFIG_ACPI
> - if (pltfm_data == &sdhci_dwcmshc_bf3_pdata)
> + if (data == &dwcmshc_bf3_data)
> sdhci_enable_v4_mode(host);
> #endif
>
> @@ -1308,8 +1371,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
> dwcmshc_cqhci_init(host, pdev);
> }
>
> - if (rk_priv)
> - rk35xx_postinit(host, priv);
> + if (data->ops && data->ops->postinit)
> + data->ops->postinit(host, priv);
>
> err = __sdhci_add_host(host);
> if (err)
> @@ -1327,9 +1390,8 @@ static int dwcmshc_probe(struct platform_device *pdev)
> err_clk:
> clk_disable_unprepare(pltfm_host->clk);
> clk_disable_unprepare(priv->bus_clk);
> - if (rk_priv)
> - clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> - rk_priv->rockchip_clks);
> + if (data->ops && data->ops->clks_disable)
> + data->ops->clks_disable(priv);
> free_pltfm:
> sdhci_pltfm_free(pdev);
> return err;
> @@ -1340,7 +1402,6 @@ static void dwcmshc_remove(struct platform_device *pdev)
> struct sdhci_host *host = platform_get_drvdata(pdev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> - struct rk35xx_priv *rk_priv = priv->priv;
>
> pm_runtime_get_sync(&pdev->dev);
> pm_runtime_disable(&pdev->dev);
> @@ -1352,9 +1413,8 @@ static void dwcmshc_remove(struct platform_device *pdev)
>
> clk_disable_unprepare(pltfm_host->clk);
> clk_disable_unprepare(priv->bus_clk);
> - if (rk_priv)
> - clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> - rk_priv->rockchip_clks);
> + if (priv->ops && priv->ops->clks_disable)
> + priv->ops->clks_disable(priv);
> sdhci_pltfm_free(pdev);
> }
>
> @@ -1364,7 +1424,6 @@ static int dwcmshc_suspend(struct device *dev)
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> - struct rk35xx_priv *rk_priv = priv->priv;
> int ret;
>
> pm_runtime_resume(dev);
> @@ -1383,9 +1442,8 @@ static int dwcmshc_suspend(struct device *dev)
> if (!IS_ERR(priv->bus_clk))
> clk_disable_unprepare(priv->bus_clk);
>
> - if (rk_priv)
> - clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> - rk_priv->rockchip_clks);
> + if (priv->ops && priv->ops->clks_disable)
> + priv->ops->clks_disable(priv);
>
> return ret;
> }
> @@ -1395,7 +1453,6 @@ static int dwcmshc_resume(struct device *dev)
> struct sdhci_host *host = dev_get_drvdata(dev);
> struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> struct dwcmshc_priv *priv = sdhci_pltfm_priv(pltfm_host);
> - struct rk35xx_priv *rk_priv = priv->priv;
> int ret;
>
> ret = clk_prepare_enable(pltfm_host->clk);
> @@ -1408,29 +1465,27 @@ static int dwcmshc_resume(struct device *dev)
> goto disable_clk;
> }
>
> - if (rk_priv) {
> - ret = clk_bulk_prepare_enable(RK35xx_MAX_CLKS,
> - rk_priv->rockchip_clks);
> + if (priv->ops && priv->ops->clks_enable) {
> + ret = priv->ops->clks_enable(priv);
> if (ret)
> goto disable_bus_clk;
> }
>
> ret = sdhci_resume_host(host);
> if (ret)
> - goto disable_rockchip_clks;
> + goto disable_soc_clks;
>
> if (host->mmc->caps2 & MMC_CAP2_CQE) {
> ret = cqhci_resume(host->mmc);
> if (ret)
> - goto disable_rockchip_clks;
> + goto disable_soc_clks;
> }
>
> return 0;
>
> -disable_rockchip_clks:
> - if (rk_priv)
> - clk_bulk_disable_unprepare(RK35xx_MAX_CLKS,
> - rk_priv->rockchip_clks);
> +disable_soc_clks:
> + if (priv->ops && priv->ops->clks_disable)
> + priv->ops->clks_disable(priv);
> disable_bus_clk:
> if (!IS_ERR(priv->bus_clk))
> clk_disable_unprepare(priv->bus_clk);
Powered by blists - more mailing lists