RE: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
Hi,
> -Original Message-
> From: Yang Xiwen
> Sent: Wednesday, April 3, 2024 10:16 AM
> To: Jaehoon Chung ; Peng Fan
> 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
> >>
> >> 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
Reviewed-by: Jaehoon Chung
> >> ---
> >> 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
> >> +#include
> >> #include
> >> #include
> >> #include
> >> #include
> >> #include
> >> +#include
> >> #include
> >> +#include
> >>
> >> 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 = >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, >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 = >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(>rsts);
> >> + if (ret) {
> >> + dev_err(dev, "Failed to deassert resets(ret = %d).\n",
> >> ret);
> >> + return
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
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
---
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
+#include
#include
#include
#include
#include
#include
+#include
#include
+#include
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 = >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.
+ 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, >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 = >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(>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(>cfg, host, host->bus_hz, 40);
host->mmc = >mmc;
--
Regards,
Yang Xiwen
Re: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
Hi,
On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen
>
> 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
> ---
> 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
> +#include
> #include
> #include
> #include
> #include
> #include
> +#include
> #include
> +#include
>
> 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 = >host;
> + int ret;
If CONFIG_CLK and DM_RESET aren't enabled, this value is a dead code.
It also needs to initialize.
>
> + 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, >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 = >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(>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(>cfg, host, host->bus_hz, 40);
> host->mmc = >mmc;
[PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
From: Yang Xiwen
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
---
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
+#include
#include
#include
#include
#include
#include
+#include
#include
+#include
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 = >host;
+ int ret;
+ 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, >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 = >host;
struct hisi_mmc_data *mmc_data;
+ int ret;
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(>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(>cfg, host, host->bus_hz, 40);
host->mmc = >mmc;
--
2.43.0
