* [PATCH v2 0/3] mmc: hi6220-dwmmc: handle resets and clocks
@ 2024-02-01 14:05 ` Yang Xiwen
0 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen via B4 Relay @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 0/3] mmc: hi6220-dwmmc: handle resets and clocks
@ 2024-02-01 14:05 ` Yang Xiwen
0 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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>
^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
2024-02-01 14:05 ` Yang Xiwen
@ 2024-02-01 14:05 ` Yang Xiwen
-1 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen via B4 Relay @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
@ 2024-02-01 14:05 ` Yang Xiwen
0 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, 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 <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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
2024-02-01 14:05 ` Yang Xiwen
@ 2024-02-01 14:05 ` Yang Xiwen
-1 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen via B4 Relay @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
@ 2024-02-01 14:05 ` Yang Xiwen
0 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
2024-02-01 14:05 ` Yang Xiwen
@ 2024-02-01 14:05 ` Yang Xiwen
-1 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen via B4 Relay @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
@ 2024-02-01 14:05 ` Yang Xiwen
0 siblings, 0 replies; 17+ messages in thread
From: Yang Xiwen @ 2024-02-01 14:05 UTC (permalink / raw)
To: Peng Fan, Jaehoon Chung; +Cc: u-boot, Yang Xiwen
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
^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
[not found] ` <CGME20240403003945epcas1p4bb700e4390a5c5d29dbe241cb621db7a@epcas1p4.samsung.com>
@ 2024-04-03 0:39 ` Jaehoon Chung
2024-04-03 1:16 ` Yang Xiwen
0 siblings, 1 reply; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-03 0:39 UTC (permalink / raw)
To: forbidden405, Peng Fan; +Cc: u-boot
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;
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
[not found] ` <CGME20240403004155epcas1p1fdb3680cde2a4dee4325e22b9b076e6a@epcas1p1.samsung.com>
@ 2024-04-03 0:41 ` Jaehoon Chung
2024-04-03 1:19 ` Yang Xiwen
0 siblings, 1 reply; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-03 0:41 UTC (permalink / raw)
To: forbidden405, Peng Fan; +Cc: u-boot
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;
> }
> }
>
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
[not found] ` <CGME20240403004345epcas1p3f668ec6887b8e74350e9840682a875c5@epcas1p3.samsung.com>
@ 2024-04-03 0:43 ` Jaehoon Chung
2024-04-03 1:22 ` Yang Xiwen
0 siblings, 1 reply; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-03 0:43 UTC (permalink / raw)
To: forbidden405, Peng Fan; +Cc: u-boot
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 },
> { }
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
2024-04-03 0:39 ` Jaehoon Chung
@ 2024-04-03 1:16 ` Yang Xiwen
2024-04-15 6:57 ` Jaehoon Chung
0 siblings, 1 reply; 17+ messages in thread
From: Yang Xiwen @ 2024-04-03 1:16 UTC (permalink / raw)
To: Jaehoon Chung, Peng Fan; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
2024-04-03 0:41 ` Jaehoon Chung
@ 2024-04-03 1:19 ` Yang Xiwen
2024-04-15 6:58 ` Jaehoon Chung
0 siblings, 1 reply; 17+ messages in thread
From: Yang Xiwen @ 2024-04-03 1:19 UTC (permalink / raw)
To: Jaehoon Chung, Peng Fan; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
2024-04-03 0:43 ` Jaehoon Chung
@ 2024-04-03 1:22 ` Yang Xiwen
2024-04-15 7:00 ` Jaehoon Chung
0 siblings, 1 reply; 17+ messages in thread
From: Yang Xiwen @ 2024-04-03 1:22 UTC (permalink / raw)
To: Jaehoon Chung, Peng Fan; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled
2024-04-03 1:16 ` Yang Xiwen
@ 2024-04-15 6:57 ` Jaehoon Chung
0 siblings, 0 replies; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-15 6:57 UTC (permalink / raw)
To: 'Yang Xiwen', 'Peng Fan'; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout
2024-04-03 1:19 ` Yang Xiwen
@ 2024-04-15 6:58 ` Jaehoon Chung
0 siblings, 0 replies; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-15 6:58 UTC (permalink / raw)
To: 'Yang Xiwen', 'Peng Fan'; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
* RE: [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe
2024-04-03 1:22 ` Yang Xiwen
@ 2024-04-15 7:00 ` Jaehoon Chung
0 siblings, 0 replies; 17+ messages in thread
From: Jaehoon Chung @ 2024-04-15 7:00 UTC (permalink / raw)
To: 'Yang Xiwen', 'Peng Fan'; +Cc: u-boot
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
^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-04-15 7:00 UTC | newest]
Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-02-01 14:05 [PATCH v2 0/3] mmc: hi6220-dwmmc: handle resets and clocks Yang Xiwen via B4 Relay
2024-02-01 14:05 ` Yang Xiwen
2024-02-01 14:05 ` [PATCH v2 1/3] mmc: hi6220-dwmmc: handle clocks and resets if CONFIG_CLK and CONFIG_DM_RESET enabled Yang Xiwen via B4 Relay
2024-02-01 14:05 ` Yang Xiwen
[not found] ` <CGME20240403003945epcas1p4bb700e4390a5c5d29dbe241cb621db7a@epcas1p4.samsung.com>
2024-04-03 0:39 ` Jaehoon Chung
2024-04-03 1:16 ` Yang Xiwen
2024-04-15 6:57 ` Jaehoon Chung
2024-02-01 14:05 ` [PATCH v2 2/3] mmc: dw_mmc: Don't return error if data busy timeout Yang Xiwen via B4 Relay
2024-02-01 14:05 ` Yang Xiwen
[not found] ` <CGME20240403004155epcas1p1fdb3680cde2a4dee4325e22b9b076e6a@epcas1p1.samsung.com>
2024-04-03 0:41 ` Jaehoon Chung
2024-04-03 1:19 ` Yang Xiwen
2024-04-15 6:58 ` Jaehoon Chung
2024-02-01 14:05 ` [PATCH v2 3/3] mmc: hi6220_dw_mmc: add fifoth_val to private data and set it in .probe Yang Xiwen via B4 Relay
2024-02-01 14:05 ` Yang Xiwen
[not found] ` <CGME20240403004345epcas1p3f668ec6887b8e74350e9840682a875c5@epcas1p3.samsung.com>
2024-04-03 0:43 ` Jaehoon Chung
2024-04-03 1:22 ` Yang Xiwen
2024-04-15 7:00 ` Jaehoon Chung
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.