RE: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled

15 Apr 2024

Hi,
...
-----Original Message-----
From: Yang Xiwen forbidden405@outlook.com
Sent: Wednesday, April 3, 2024 10:16 AM
To: Jaehoon Chung jh80.chung@samsung.com; Peng Fan peng.fan@nxp.com
Cc: u-boot@lists.denx.de
Subject: Re: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and
CONFIG_DM_RESET enabled
On 4/3/2024 8:39 AM, Jaehoon Chung wrote:
...
Hi,
On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
...
From: Yang Xiwen forbidden405@outlook.com
This can avoid hardcoding a clock rate in driver. Also can enable the
clocks and deassert the resets if the pre-bootloader does not do this
for us.
Currently only enabled for Hi3798MV200.
Signed-off-by: Yang Xiwen forbidden405@outlook.com
Reviewed-by: Jaehoon Chung jh80.chung@samsung.com
...
...
...

drivers/mmc/hi6220_dw_mmc.c | 61 ++++++++++++++++++++++++++++++++++++++++++++-
  1 file changed, 60 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/hi6220_dw_mmc.c b/drivers/mmc/hi6220_dw_mmc.c
index 71962cd47e..a4b8072976 100644
--- a/drivers/mmc/hi6220_dw_mmc.c
+++ b/drivers/mmc/hi6220_dw_mmc.c
@@ -5,15 +5,24 @@
   */
#include <common.h>
+#include <clk.h>
  #include <dm.h>
  #include <dwmmc.h>
  #include <errno.h>
  #include <fdtdec.h>
  #include <malloc.h>
+#include <reset.h>
  #include <asm/global_data.h>
+#include <dm/device_compat.h>
DECLARE_GLOBAL_DATA_PTR;
+enum hi6220_dwmmc_clk_type {

HI6220_DWMMC_CLK_BIU,
HI6220_DWMMC_CLK_CIU,
HI6220_DWMMC_CLK_CNT,

+};

struct hi6220_dwmmc_plat {
 struct mmc_config cfg;
 struct mmc mmc;

@@ -21,6 +30,8 @@ struct hi6220_dwmmc_plat {
struct hi6220_dwmmc_priv_data {
   struct dwmci_host host;

struct clk *clks[HI6220_DWMMC_CLK_CNT];

struct reset_ctl_bulk rsts;
};
struct hisi_mmc_data {


@@ -32,7 +43,29 @@ static int hi6220_dwmmc_of_to_plat(struct udevice *dev)
  {
   struct hi6220_dwmmc_priv_data *priv = dev_get_priv(dev);
   struct dwmci_host *host = &priv->host;

int ret;

If CONFIG_CLK and DM_RESET aren't enabled, this value is a dead code.
It also needs to initialize.
I think a alternative solution is replacing the if stmt below with some
`#ifdef`s just like some unittests code. So we can mask variable `ret'
out if it's not used However, this seems not favored by checkpatch.pl.
It's not a critical thing. If possible to change more generic, I will change them.
Thanks!
Best Regards,
Jaehoon Chung
...
...
...

if (CONFIG_IS_ENABLED(CLK) && CONFIG_IS_ENABLED(DM_RESET)) {
priv->clks[HI6220_DWMMC_CLK_BIU] = devm_clk_get(dev, "biu");


if (IS_ERR(priv->clks[HI6220_DWMMC_CLK_BIU])) {


	ret = PTR_ERR(priv->clks[HI6220_DWMMC_CLK_BIU]);


	dev_err(dev, "Failed to get BIU clock(ret = %d).\n", ret);


	return log_msg_ret("clk", ret);


}



priv->clks[HI6220_DWMMC_CLK_CIU] = devm_clk_get(dev, "ciu");


if (IS_ERR(priv->clks[HI6220_DWMMC_CLK_CIU])) {


	ret = PTR_ERR(priv->clks[HI6220_DWMMC_CLK_CIU]);


	dev_err(dev, "Failed to get CIU clock(ret = %d).\n", ret);


	return log_msg_ret("clk", ret);


}



ret = reset_get_bulk(dev, &priv->rsts);


if (ret) {


	dev_err(dev, "Failed to get resets(ret = %d)", ret);


	return log_msg_ret("rst", ret);


}


}
host->name = dev->name;
host->ioaddr = dev_read_addr_ptr(dev);
host->buswidth = fdtdec_get_int(gd->fdt_blob, dev_of_offset(dev),

@@ -56,11 +89,37 @@ static int hi6220_dwmmc_probe(struct udevice *dev)
   struct hi6220_dwmmc_priv_data *priv = dev_get_priv(dev);
   struct dwmci_host *host = &priv->host;
   struct hisi_mmc_data *mmc_data;

int ret;

Ditto.
Best Regards,
Jaehoon Chung
...
mmc_data = (struct hisi_mmc_data *)dev_get_driver_data(dev);

/* Use default bus speed due to absence of clk driver */
host->bus_hz = mmc_data->clock;


if (CONFIG_IS_ENABLED(CLK) && CONFIG_IS_ENABLED(DM_RESET)) {

ret = clk_prepare_enable(priv->clks[HI6220_DWMMC_CLK_BIU]);


if (ret) {


	dev_err(dev, "Failed to enable biu clock(ret = %d).\n", ret);


	return log_msg_ret("clk", ret);


}



ret = clk_prepare_enable(priv->clks[HI6220_DWMMC_CLK_CIU]);


if (ret) {


	dev_err(dev, "Failed to enable ciu clock(ret = %d).\n", ret);


	return log_msg_ret("clk", ret);


}



ret = reset_deassert_bulk(&priv->rsts);


if (ret) {


	dev_err(dev, "Failed to deassert resets(ret = %d).\n", ret);


	return log_msg_ret("rst", ret);


}



host->bus_hz = clk_get_rate(priv->clks[HI6220_DWMMC_CLK_CIU]);


if (host->bus_hz <= 0) {


	dev_err(dev, "Failed to get ciu clock rate(ret = %d).\n", ret);


	return log_msg_ret("clk", ret);


}


}

dev_dbg(dev, "bus clock rate: %d.\n", host->bus_hz);
dwmci_setup_cfg(&plat->cfg, host, host->bus_hz, 400000);
host->mmc = &plat->mmc;


--
Regards,
Yang Xiwen

    