[PATCH v2 0/3] mmc: hi6220-dwmmc: handle resets and clocks

[PATCH v2 0/3] mmc: hi6220-dwmmc: handle resets and clocks

[Yang Xiwen via B4 Relay]

Also allow setup fifoth_val in driver

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
Changes in v2:
- dw-mmc: proceed if data busy found
- hi6220-dw-mmc: add fifoth_val setup, separate hi3798mv200 data
- Link to v1: https://lore.kernel.org/r/20240119-mmc-v1-1-aee6b2cf6902@outlook.com

---
Yang Xiwen (3):
      mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
      mmc: dw_mmc: Don't return error if data busy timeout
      mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe

 drivers/mmc/dw_mmc.c        |  4 +--
 drivers/mmc/hi6220_dw_mmc.c | 72 +++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 72 insertions(+), 4 deletions(-)
---
base-commit: f7cca7ccc5117eaafcc2bde91ad1bed6fee7cfc3
change-id: 20240119-mmc-9cf7f3455cb4

Best regards,
-- 
Yang Xiwen <forbidden405@outlook.com>

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

[Yang Xiwen via B4 Relay]

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>
---
 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_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;
 
 	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;

-- 
2.43.0

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

[Jaehoon Chung]

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>
> ---
>  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.

>  
> +	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;

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

[Yang Xiwen]

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>
>> ---
>>   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.


>
>>   
>> +	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

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

[Jaehoon Chung]

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

[PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

As described in [1], some poor hardware or cards would fail to release
the bus and keep driving data lines low. Ignore it and send the next cmd
directly seems okay for most cases.

[1]: https://patchwork.kernel.org/project/linux-mmc/patch/1424458179-5456-1-git-send-email-dianders@chromium.org/

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/mmc/dw_mmc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
index 400066fa99..e103664145 100644
--- a/drivers/mmc/dw_mmc.c
+++ b/drivers/mmc/dw_mmc.c
@@ -262,8 +262,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
 
 	while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
 		if (get_timer(start) > timeout) {
-			debug("%s: Timeout on data busy\n", __func__);
-			return -ETIMEDOUT;
+			debug("%s: Timeout on data busy, continue anyway\n", __func__);
+			break;
 		}
 	}
 

-- 
2.43.0

Re: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout

[Jaehoon Chung]

Hi,

On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> As described in [1], some poor hardware or cards would fail to release
> the bus and keep driving data lines low. Ignore it and send the next cmd
> directly seems okay for most cases.
This patch seems to be same with previous patch, right?

Best Regards,
Jaehoon Chung

> 
> [1]: https://patchwork.kernel.org/project/linux-mmc/patch/1424458179-5456-1-git-send-email-dianders@chromium.org/
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/mmc/dw_mmc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> index 400066fa99..e103664145 100644
> --- a/drivers/mmc/dw_mmc.c
> +++ b/drivers/mmc/dw_mmc.c
> @@ -262,8 +262,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>  
>  	while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
>  		if (get_timer(start) > timeout) {
> -			debug("%s: Timeout on data busy\n", __func__);
> -			return -ETIMEDOUT;
> +			debug("%s: Timeout on data busy, continue anyway\n", __func__);
> +			break;
>  		}
>  	}
>  

Re: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout

[Yang Xiwen]

On 4/3/2024 8:41 AM, Jaehoon Chung wrote:
> Hi,
>
> On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> As described in [1], some poor hardware or cards would fail to release
>> the bus and keep driving data lines low. Ignore it and send the next cmd
>> directly seems okay for most cases.
> This patch seems to be same with previous patch, right?


 From my observation, this patch does fix some weird problems and is 
mostly okay for other dwmmc users. I can't say it is very well tested 
because of I can't come up of other tests i can do except some `mmc 
read` and `mmc write`.


>
> Best Regards,
> Jaehoon Chung
>
>> [1]: https://patchwork.kernel.org/project/linux-mmc/patch/1424458179-5456-1-git-send-email-dianders@chromium.org/
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   drivers/mmc/dw_mmc.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
>> index 400066fa99..e103664145 100644
>> --- a/drivers/mmc/dw_mmc.c
>> +++ b/drivers/mmc/dw_mmc.c
>> @@ -262,8 +262,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
>>   
>>   	while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
>>   		if (get_timer(start) > timeout) {
>> -			debug("%s: Timeout on data busy\n", __func__);
>> -			return -ETIMEDOUT;
>> +			debug("%s: Timeout on data busy, continue anyway\n", __func__);
>> +			break;
>>   		}
>>   	}
>>   


-- 
Regards,
Yang Xiwen

RE: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Yang Xiwen <forbidden405@outlook.com>
> Sent: Wednesday, April 3, 2024 10:20 AM
> To: Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan <peng.fan@nxp.com>
> Cc: u-boot@lists.denx.de
> Subject: Re: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
> 
> On 4/3/2024 8:41 AM, Jaehoon Chung wrote:
> > Hi,
> >
> > On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
> >> From: Yang Xiwen <forbidden405@outlook.com>
> >>
> >> As described in [1], some poor hardware or cards would fail to release
> >> the bus and keep driving data lines low. Ignore it and send the next cmd
> >> directly seems okay for most cases.
> > This patch seems to be same with previous patch, right?
> 
> 
>  From my observation, this patch does fix some weird problems and is
> mostly okay for other dwmmc users. I can't say it is very well tested
> because of I can't come up of other tests i can do except some `mmc
> read` and `mmc write`.
> 
> 
> >
> > Best Regards,
> > Jaehoon Chung
> >
> >> [1]: https://patchwork.kernel.org/project/linux-mmc/patch/1424458179-5456-1-git-send-email-
> dianders@chromium.org/
> >>
> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>

Tested-by: Jaehoon Chung <jh80.chung@samsung.com>

Best Regards,
Jaehoon Chung

> >> ---
> >>   drivers/mmc/dw_mmc.c | 4 ++--
> >>   1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c
> >> index 400066fa99..e103664145 100644
> >> --- a/drivers/mmc/dw_mmc.c
> >> +++ b/drivers/mmc/dw_mmc.c
> >> @@ -262,8 +262,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd,
> >>
> >>   	while (dwmci_readl(host, DWMCI_STATUS) & DWMCI_BUSY) {
> >>   		if (get_timer(start) > timeout) {
> >> -			debug("%s: Timeout on data busy\n", __func__);
> >> -			return -ETIMEDOUT;
> >> +			debug("%s: Timeout on data busy, continue anyway\n", __func__);
> >> +			break;
> >>   		}
> >>   	}
> >>
> 
> 
> --
> Regards,
> Yang Xiwen

[PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe

[Yang Xiwen via B4 Relay]

From: Yang Xiwen <forbidden405@outlook.com>

The value defaults to 0 and is ignored by dw_mmc code, so the other
users are not affected.

Setting this explicitly fixes some weird reading error found on Hi3798MV200.

Fixes: 8a5dc8140e62 ("mmc: hi6220_dw_mmc: add compatible for HC2910 support")

Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
---
 drivers/mmc/hi6220_dw_mmc.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/mmc/hi6220_dw_mmc.c b/drivers/mmc/hi6220_dw_mmc.c
index a4b8072976..dc0210402b 100644
--- a/drivers/mmc/hi6220_dw_mmc.c
+++ b/drivers/mmc/hi6220_dw_mmc.c
@@ -37,6 +37,7 @@ struct hi6220_dwmmc_priv_data {
 struct hisi_mmc_data {
 	unsigned int clock;
 	bool use_fifo;
+	u32 fifoth_val;
 };
 
 static int hi6220_dwmmc_of_to_plat(struct udevice *dev)
@@ -125,6 +126,7 @@ static int hi6220_dwmmc_probe(struct udevice *dev)
 	host->mmc = &plat->mmc;
 
 	host->fifo_mode = mmc_data->use_fifo;
+	host->fifoth_val = mmc_data->fifoth_val;
 	host->mmc->priv = &priv->host;
 	upriv->mmc = host->mmc;
 	host->mmc->dev = dev;
@@ -154,13 +156,20 @@ static const struct hisi_mmc_data hi6220_mmc_data = {
 	.use_fifo = false,
 };
 
+static const struct hisi_mmc_data hi3798mv2x_mmc_data = {
+	.clock = 50000000,
+	.use_fifo = false,
+	// FIFO depth is 256
+	.fifoth_val = MSIZE(4) | RX_WMARK(0x7f) | TX_WMARK(0x80),
+};
+
 static const struct udevice_id hi6220_dwmmc_ids[] = {
 	{ .compatible = "hisilicon,hi6220-dw-mshc",
 	  .data = (ulong)&hi6220_mmc_data },
 	{ .compatible = "hisilicon,hi3798cv200-dw-mshc",
 	  .data = (ulong)&hi6220_mmc_data },
 	{ .compatible = "hisilicon,hi3798mv200-dw-mshc",
-	  .data = (ulong)&hi6220_mmc_data },
+	  .data = (ulong)&hi3798mv2x_mmc_data },
 	{ .compatible = "hisilicon,hi3660-dw-mshc",
 	  .data = (ulong)&hi3660_mmc_data },
 	{ }

-- 
2.43.0

Re: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe

[Jaehoon Chung]

Hi

On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
> From: Yang Xiwen <forbidden405@outlook.com>
> 
> The value defaults to 0 and is ignored by dw_mmc code, so the other
> users are not affected.
> 
> Setting this explicitly fixes some weird reading error found on Hi3798MV200.
> 
> Fixes: 8a5dc8140e62 ("mmc: hi6220_dw_mmc: add compatible for HC2910 support")
> 
> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
> ---
>  drivers/mmc/hi6220_dw_mmc.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/hi6220_dw_mmc.c b/drivers/mmc/hi6220_dw_mmc.c
> index a4b8072976..dc0210402b 100644
> --- a/drivers/mmc/hi6220_dw_mmc.c
> +++ b/drivers/mmc/hi6220_dw_mmc.c
> @@ -37,6 +37,7 @@ struct hi6220_dwmmc_priv_data {
>  struct hisi_mmc_data {
>  	unsigned int clock;
>  	bool use_fifo;
> +	u32 fifoth_val;
>  };
>  
>  static int hi6220_dwmmc_of_to_plat(struct udevice *dev)
> @@ -125,6 +126,7 @@ static int hi6220_dwmmc_probe(struct udevice *dev)
>  	host->mmc = &plat->mmc;
>  
>  	host->fifo_mode = mmc_data->use_fifo;
> +	host->fifoth_val = mmc_data->fifoth_val;
>  	host->mmc->priv = &priv->host;
>  	upriv->mmc = host->mmc;
>  	host->mmc->dev = dev;
> @@ -154,13 +156,20 @@ static const struct hisi_mmc_data hi6220_mmc_data = {
>  	.use_fifo = false,
>  };
>  
> +static const struct hisi_mmc_data hi3798mv2x_mmc_data = {
> +	.clock = 50000000,
> +	.use_fifo = false,
> +	// FIFO depth is 256
Removed unnecessary comment.

Best Regards,
Jaehoon Chung

> +	.fifoth_val = MSIZE(4) | RX_WMARK(0x7f) | TX_WMARK(0x80),
> +};
> +
>  static const struct udevice_id hi6220_dwmmc_ids[] = {
>  	{ .compatible = "hisilicon,hi6220-dw-mshc",
>  	  .data = (ulong)&hi6220_mmc_data },
>  	{ .compatible = "hisilicon,hi3798cv200-dw-mshc",
>  	  .data = (ulong)&hi6220_mmc_data },
>  	{ .compatible = "hisilicon,hi3798mv200-dw-mshc",
> -	  .data = (ulong)&hi6220_mmc_data },
> +	  .data = (ulong)&hi3798mv2x_mmc_data },
>  	{ .compatible = "hisilicon,hi3660-dw-mshc",
>  	  .data = (ulong)&hi3660_mmc_data },
>  	{ }

Re: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe

[Yang Xiwen]

On 4/3/2024 8:43 AM, Jaehoon Chung wrote:
> Hi
>
> On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
>> From: Yang Xiwen <forbidden405@outlook.com>
>>
>> The value defaults to 0 and is ignored by dw_mmc code, so the other
>> users are not affected.
>>
>> Setting this explicitly fixes some weird reading error found on Hi3798MV200.
>>
>> Fixes: 8a5dc8140e62 ("mmc: hi6220_dw_mmc: add compatible for HC2910 support")
>>
>> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>
>> ---
>>   drivers/mmc/hi6220_dw_mmc.c | 11 ++++++++++-
>>   1 file changed, 10 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/hi6220_dw_mmc.c b/drivers/mmc/hi6220_dw_mmc.c
>> index a4b8072976..dc0210402b 100644
>> --- a/drivers/mmc/hi6220_dw_mmc.c
>> +++ b/drivers/mmc/hi6220_dw_mmc.c
>> @@ -37,6 +37,7 @@ struct hi6220_dwmmc_priv_data {
>>   struct hisi_mmc_data {
>>   	unsigned int clock;
>>   	bool use_fifo;
>> +	u32 fifoth_val;
>>   };
>>   
>>   static int hi6220_dwmmc_of_to_plat(struct udevice *dev)
>> @@ -125,6 +126,7 @@ static int hi6220_dwmmc_probe(struct udevice *dev)
>>   	host->mmc = &plat->mmc;
>>   
>>   	host->fifo_mode = mmc_data->use_fifo;
>> +	host->fifoth_val = mmc_data->fifoth_val;
>>   	host->mmc->priv = &priv->host;
>>   	upriv->mmc = host->mmc;
>>   	host->mmc->dev = dev;
>> @@ -154,13 +156,20 @@ static const struct hisi_mmc_data hi6220_mmc_data = {
>>   	.use_fifo = false,
>>   };
>>   
>> +static const struct hisi_mmc_data hi3798mv2x_mmc_data = {
>> +	.clock = 50000000,
>> +	.use_fifo = false,
>> +	// FIFO depth is 256
> Removed unnecessary comment.


Will do. It seems that this should also apply to hi3798cv200-dw-mshc. 
tests are needed from cv200 users.


>
> Best Regards,
> Jaehoon Chung
>
>> +	.fifoth_val = MSIZE(4) | RX_WMARK(0x7f) | TX_WMARK(0x80),
>> +};
>> +
>>   static const struct udevice_id hi6220_dwmmc_ids[] = {
>>   	{ .compatible = "hisilicon,hi6220-dw-mshc",
>>   	  .data = (ulong)&hi6220_mmc_data },
>>   	{ .compatible = "hisilicon,hi3798cv200-dw-mshc",
>>   	  .data = (ulong)&hi6220_mmc_data },
>>   	{ .compatible = "hisilicon,hi3798mv200-dw-mshc",
>> -	  .data = (ulong)&hi6220_mmc_data },
>> +	  .data = (ulong)&hi3798mv2x_mmc_data },
>>   	{ .compatible = "hisilicon,hi3660-dw-mshc",
>>   	  .data = (ulong)&hi3660_mmc_data },
>>   	{ }


-- 
Regards,
Yang Xiwen

RE: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe

[Jaehoon Chung]

Hi,

> -----Original Message-----
> From: Yang Xiwen <forbidden405@outlook.com>
> Sent: Wednesday, April 3, 2024 10:22 AM
> To: Jaehoon Chung <jh80.chung@samsung.com>; Peng Fan <peng.fan@nxp.com>
> Cc: u-boot@lists.denx.de
> Subject: Re: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
> 
> On 4/3/2024 8:43 AM, Jaehoon Chung wrote:
> > Hi
> >
> > On 2/1/24 23:05, Yang Xiwen via B4 Relay wrote:
> >> From: Yang Xiwen <forbidden405@outlook.com>
> >>
> >> The value defaults to 0 and is ignored by dw_mmc code, so the other
> >> users are not affected.
> >>
> >> Setting this explicitly fixes some weird reading error found on Hi3798MV200.
> >>
> >> Fixes: 8a5dc8140e62 ("mmc: hi6220_dw_mmc: add compatible for HC2910 support")
> >>
> >> Signed-off-by: Yang Xiwen <forbidden405@outlook.com>

Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

> >> ---
> >>   drivers/mmc/hi6220_dw_mmc.c | 11 ++++++++++-
> >>   1 file changed, 10 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/mmc/hi6220_dw_mmc.c b/drivers/mmc/hi6220_dw_mmc.c
> >> index a4b8072976..dc0210402b 100644
> >> --- a/drivers/mmc/hi6220_dw_mmc.c
> >> +++ b/drivers/mmc/hi6220_dw_mmc.c
> >> @@ -37,6 +37,7 @@ struct hi6220_dwmmc_priv_data {
> >>   struct hisi_mmc_data {
> >>   	unsigned int clock;
> >>   	bool use_fifo;
> >> +	u32 fifoth_val;
> >>   };
> >>
> >>   static int hi6220_dwmmc_of_to_plat(struct udevice *dev)
> >> @@ -125,6 +126,7 @@ static int hi6220_dwmmc_probe(struct udevice *dev)
> >>   	host->mmc = &plat->mmc;
> >>
> >>   	host->fifo_mode = mmc_data->use_fifo;
> >> +	host->fifoth_val = mmc_data->fifoth_val;
> >>   	host->mmc->priv = &priv->host;
> >>   	upriv->mmc = host->mmc;
> >>   	host->mmc->dev = dev;
> >> @@ -154,13 +156,20 @@ static const struct hisi_mmc_data hi6220_mmc_data = {
> >>   	.use_fifo = false,
> >>   };
> >>
> >> +static const struct hisi_mmc_data hi3798mv2x_mmc_data = {
> >> +	.clock = 50000000,
> >> +	.use_fifo = false,
> >> +	// FIFO depth is 256
> > Removed unnecessary comment.
> 
> 
> Will do. It seems that this should also apply to hi3798cv200-dw-mshc.
> tests are needed from cv200 users.

In future, It can be removed. Others looks good to me.

Best Regards,
Jaehoon Chung

> 
> 
> >
> > Best Regards,
> > Jaehoon Chung
> >
> >> +	.fifoth_val = MSIZE(4) | RX_WMARK(0x7f) | TX_WMARK(0x80),
> >> +};
> >> +
> >>   static const struct udevice_id hi6220_dwmmc_ids[] = {
> >>   	{ .compatible = "hisilicon,hi6220-dw-mshc",
> >>   	  .data = (ulong)&hi6220_mmc_data },
> >>   	{ .compatible = "hisilicon,hi3798cv200-dw-mshc",
> >>   	  .data = (ulong)&hi6220_mmc_data },
> >>   	{ .compatible = "hisilicon,hi3798mv200-dw-mshc",
> >> -	  .data = (ulong)&hi6220_mmc_data },
> >> +	  .data = (ulong)&hi3798mv2x_mmc_data },
> >>   	{ .compatible = "hisilicon,hi3660-dw-mshc",
> >>   	  .data = (ulong)&hi3660_mmc_data },
> >>   	{ }
> 
> 
> --
> Regards,
> Yang Xiwen