[U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors
The dw_mmc driver uses printf() in various places.
These bloat the code and cause problems for SPL. Use debug() where possible and try to return a useful error code instead.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 53a8aca..8f28d7e 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -8,6 +8,7 @@
#include <bouncebuf.h> #include <common.h> +#include <errno.h> #include <malloc.h> #include <mmc.h> #include <dwmmc.h> @@ -119,7 +120,7 @@ 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) { - printf("%s: Timeout on data busy\n", __func__); + debug("%s: Timeout on data busy\n", __func__); return TIMEOUT; } } @@ -178,7 +179,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, }
if (i == retry) { - printf("%s: Timeout.\n", __func__); + debug("%s: Timeout.\n", __func__); return TIMEOUT; }
@@ -194,8 +195,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, debug("%s: Response Timeout.\n", __func__); return TIMEOUT; } else if (mask & DWMCI_INTMSK_RE) { - printf("%s: Response Error.\n", __func__); - return -1; + debug("%s: Response Error.\n", __func__); + return -EIO; }
@@ -214,7 +215,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, do { mask = dwmci_readl(host, DWMCI_RINTSTS); if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) { - printf("%s: DATA ERROR!\n", __func__); + debug("%s: DATA ERROR!\n", __func__); return -1; } } while (!(mask & DWMCI_INTMSK_DTO)); @@ -251,7 +252,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) else if (host->bus_hz) sclk = host->bus_hz; else { - printf("%s: Didn't get source clock value.\n", __func__); + debug("%s: Didn't get source clock value.\n", __func__); return -EINVAL; }
@@ -270,7 +271,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) { - printf("%s: Timeout!\n", __func__); + debug("%s: Timeout!\n", __func__); return -ETIMEDOUT; } } while (status & DWMCI_CMD_START); @@ -285,7 +286,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) { - printf("%s: Timeout!\n", __func__); + debug("%s: Timeout!\n", __func__); return -ETIMEDOUT; } } while (status & DWMCI_CMD_START); @@ -339,8 +340,8 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_PWREN, 1);
if (!dwmci_wait_reset(host, DWMCI_RESET_ALL)) { - printf("%s[%d] Fail-reset!!\n", __func__, __LINE__); - return -1; + debug("%s[%d] Fail-reset!!\n", __func__, __LINE__); + return -EIO; }
/* Enumerate at 400KHz */
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: - Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user. */ if (host->get_mmc_clk) - sclk = host->get_mmc_clk(host); + sclk = host->get_mmc_clk(host, freq); else if (host->bus_hz) sclk = host->bus_hz; else { diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c index e083745..3f702ba 100644 --- a/drivers/mmc/exynos_dw_mmc.c +++ b/drivers/mmc/exynos_dw_mmc.c @@ -39,7 +39,7 @@ static void exynos_dwmci_clksel(struct dwmci_host *host) dwmci_writel(host, DWMCI_CLKSEL, priv->sdr_timing); }
-unsigned int exynos_dwmci_get_clk(struct dwmci_host *host) +unsigned int exynos_dwmci_get_clk(struct dwmci_host *host, uint freq) { unsigned long sclk; int8_t clk_div; diff --git a/include/dwmmc.h b/include/dwmmc.h index 7a7555a..25cf42c 100644 --- a/include/dwmmc.h +++ b/include/dwmmc.h @@ -163,7 +163,21 @@ struct dwmci_host {
void (*clksel)(struct dwmci_host *host); void (*board_init)(struct dwmci_host *host); - unsigned int (*get_mmc_clk)(struct dwmci_host *host); + + /** + * Get / set a particular MMC clock frequency + * + * This is used to request the current clock frequency of the clock + * that drives the DWMMC peripheral. The caller will then use this + * information to work out the divider it needs to achieve the + * required MMC bus clock frequency. If you want to handle the + * clock external to DWMMC, use @freq to select the frequency and + * return that value too. Then DWMMC will put itself in bypass mode. + * + * @host: DWMMC host + * @freq: Frequency the host is trying to achieve + */ + unsigned int (*get_mmc_clk)(struct dwmci_host *host, uint freq);
struct mmc_config cfg; };
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user. */ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);
Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Best regards, Marek Vasut
Hi Marek,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user. */ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Regards, Simon
On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user.
*/ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
Best regards, Marek Vasut
Hi Marek,
On 6 August 2015 at 20:58, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user.
*/ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
Regards, Simon
On Friday, August 07, 2015 at 05:00:24 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:58, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc
driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user.
*/ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
Best regards, Marek Vasut
Hi,
On 08/07/2015 12:07 PM, Marek Vasut wrote:
On Friday, August 07, 2015 at 05:00:24 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:58, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc
driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user.
*/ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand wha,t you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
Hmm. I don't know what purpose @freq is...just bypass? @freq doesn't use wherever..I'm checking with u-boot-dm repository(mmc-working branch) I wonder i'm also missing something like Marek.
Best Regards, Jaehoon Chung
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
Best regards, Marek Vasut
Hi Marek,
On 6 August 2015 at 21:07, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 05:00:24 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:58, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:54:42 AM, Simon Glass wrote:
Hi Marek,
Hi Simon,
On 6 August 2015 at 20:51, Marek Vasut marex@denx.de wrote:
On Friday, August 07, 2015 at 04:16:28 AM, Simon Glass wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Hi Simon,
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc
driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user.
*/ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
Unless we pass the bus frequency to get_mmc_clk() it has no way of knowing what bus clock is required and thus cannot implement this feature. The feature implementation is entirely within the implementation of get_mmc_clk() - it just needs one more piece of information to do its job.
Regards, Simon
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
Why are you passing the @freq into get_mmc_clk() ? Shouldn't you call some clock framework function to determine the input frequency of the DWMMC block from within the get_mmc_clk() implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
Unless we pass the bus frequency to get_mmc_clk() it has no way of knowing what bus clock is required and thus cannot implement this feature. The feature implementation is entirely within the implementation of get_mmc_clk() - it just needs one more piece of information to do its job.
I see, thanks for clearing this up!
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
> Why are you passing the @freq into get_mmc_clk() ? Shouldn't you > call some clock framework function to determine the input > frequency of the DWMMC block from within the get_mmc_clk() > implementation instead ? What do you think please ?
Well, yes. If such a clock frame work existed I would call it :-) We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
Unless we pass the bus frequency to get_mmc_clk() it has no way of knowing what bus clock is required and thus cannot implement this feature. The feature implementation is entirely within the implementation of get_mmc_clk() - it just needs one more piece of information to do its job.
I see, thanks for clearing this up!
Regards, Simon
On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
> > Why are you passing the @freq into get_mmc_clk() ? Shouldn't you > > call some clock framework function to determine the input > > frequency of the DWMMC block from within the get_mmc_clk() > > implementation instead ? What do you think please ? > > Well, yes. If such a clock frame work existed I would call it :-) > We do have a uclass now so we are getting there.
Excellent, so do you really need this kind of patch ? :) Why don't you make just some kind of function -- get_dwmmc_clock() -- and call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
OK, but making a .get_mmc_clk() function actually configure something is a behavior I wouldn't expect from a getter function. It's a bit odd and illogical in my opinion.
Hi Marek,
On 12 August 2015 at 07:53, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
>> > Why are you passing the @freq into get_mmc_clk() ? Shouldn't you >> > call some clock framework function to determine the input >> > frequency of the DWMMC block from within the get_mmc_clk() >> > implementation instead ? What do you think please ? >> >> Well, yes. If such a clock frame work existed I would call it :-) >> We do have a uclass now so we are getting there. > > Excellent, so do you really need this kind of patch ? :) Why don't > you make just some kind of function -- get_dwmmc_clock() -- and > call it instead ?
This is sort-of what is happening. It is calling a function in the host controller - i.e. the SoC's MMC controller. It is one step closer to knowing the input clock to the dwmmc input clock. Note that it is not the clock of the MMC bus itself, but the input clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
OK, but making a .get_mmc_clk() function actually configure something is a behavior I wouldn't expect from a getter function. It's a bit odd and illogical in my opinion.
Yes fair enough, it is odd. I did start an MMC uclass so perhaps that will lead to a better solution. It's unfortunately that dw_mmc need its own callback infrastructure.
Regards, Simon
On Wednesday, August 12, 2015 at 03:55:59 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:53, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
> >> > Why are you passing the @freq into get_mmc_clk() ? Shouldn't > >> > you call some clock framework function to determine the > >> > input frequency of the DWMMC block from within the > >> > get_mmc_clk() implementation instead ? What do you think > >> > please ? > >> > >> Well, yes. If such a clock frame work existed I would call it > >> :-) We do have a uclass now so we are getting there. > > > > Excellent, so do you really need this kind of patch ? :) Why > > don't you make just some kind of function -- get_dwmmc_clock() > > -- and call it instead ? > > This is sort-of what is happening. It is calling a function in the > host controller - i.e. the SoC's MMC controller. It is one step > closer to knowing the input clock to the dwmmc input clock. Note > that it is not the clock of the MMC bus itself, but the input > clock to the dwmmc logic block.
I don't think I quite understand what you mean here. We're talking about obtaining the frequency of the clock which go into the DWMMC IP block, right ?
So, if you implement a function, say -- dwmmc_get_upstream_clock() -- and call it from within the implementation of the .get_mmc_clk(), which is specific for that particular chip of yours*, you don't need this patch. Or am I really missing something fundamental ?
*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
OK, but making a .get_mmc_clk() function actually configure something is a behavior I wouldn't expect from a getter function. It's a bit odd and illogical in my opinion.
Yes fair enough, it is odd. I did start an MMC uclass so perhaps that will lead to a better solution. It's unfortunately that dw_mmc need its own callback infrastructure.
I hope we can iron that out shortly. The good thing is that you now have a board with the DWMMC and SoCFPGA also has one, so we have at least two pairs of eyes on it.
Also, what do you prefer to do about this patch ? Shall we go with the .set_mmc_clock() callback and be done with it or do you want to stick with the current approach ? I'm inclined to the former as it's less confusing in my opinion.
Hi Marek,
On 12 August 2015 at 08:40, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:55:59 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:53, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote:
Hi Marek,
Hi!
[...]
>> >> > Why are you passing the @freq into get_mmc_clk() ? Shouldn't >> >> > you call some clock framework function to determine the >> >> > input frequency of the DWMMC block from within the >> >> > get_mmc_clk() implementation instead ? What do you think >> >> > please ? >> >> >> >> Well, yes. If such a clock frame work existed I would call it >> >> :-) We do have a uclass now so we are getting there. >> > >> > Excellent, so do you really need this kind of patch ? :) Why >> > don't you make just some kind of function -- get_dwmmc_clock() >> > -- and call it instead ? >> >> This is sort-of what is happening. It is calling a function in the >> host controller - i.e. the SoC's MMC controller. It is one step >> closer to knowing the input clock to the dwmmc input clock. Note >> that it is not the clock of the MMC bus itself, but the input >> clock to the dwmmc logic block. > > I don't think I quite understand what you mean here. We're talking > about obtaining the frequency of the clock which go into the DWMMC > IP block, right ? > > So, if you implement a function, say -- dwmmc_get_upstream_clock() > -- and call it from within the implementation of the > .get_mmc_clk(), which is specific for that particular chip of > yours*, you don't need this patch. Or am I really missing > something fundamental ? > > *the .get_mmc_clk() is specific to a chip, see for example > exynos_dw_mmc.c
The purpose of the existing code (before my change) is to find out the input frequency of the IP block. By knowing this, the dw_mmc driver can work out what divisor it needs to achieve a particular MMC bus clock.
The implementation of get_mmc_clk() (which will be in the SoC-specific MMC driver) is indeed the place where the clock is figured out. My only change is to add a parameter which is the desired bus clock. This parameter can be ignored, but for implementations which can select the source clock such that it matches this bus clock, then they can do this and dw_mmc can just use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
OK, but making a .get_mmc_clk() function actually configure something is a behavior I wouldn't expect from a getter function. It's a bit odd and illogical in my opinion.
Yes fair enough, it is odd. I did start an MMC uclass so perhaps that will lead to a better solution. It's unfortunately that dw_mmc need its own callback infrastructure.
I hope we can iron that out shortly. The good thing is that you now have a board with the DWMMC and SoCFPGA also has one, so we have at least two pairs of eyes on it.
Also, what do you prefer to do about this patch ? Shall we go with the .set_mmc_clock() callback and be done with it or do you want to stick with the current approach ? I'm inclined to the former as it's less confusing in my opinion.
Let's revisit it when I get back to the rockchip series.
Regards, Simon
On Wednesday, August 12, 2015 at 11:09:15 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 08:40, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:55:59 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:53, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:51:07 PM, Simon Glass wrote:
Hi Marek,
On 12 August 2015 at 07:48, Marek Vasut marex@denx.de wrote:
On Wednesday, August 12, 2015 at 03:04:15 PM, Simon Glass wrote: > Hi Marek,
Hi!
[...]
> >> >> > Why are you passing the @freq into get_mmc_clk() ? > >> >> > Shouldn't you call some clock framework function to > >> >> > determine the input frequency of the DWMMC block from > >> >> > within the get_mmc_clk() implementation instead ? What do > >> >> > you think please ? > >> >> > >> >> Well, yes. If such a clock frame work existed I would call > >> >> it > >> >> > >> >> :-) We do have a uclass now so we are getting there. > >> > > >> > Excellent, so do you really need this kind of patch ? :) Why > >> > don't you make just some kind of function -- > >> > get_dwmmc_clock() -- and call it instead ? > >> > >> This is sort-of what is happening. It is calling a function in > >> the host controller - i.e. the SoC's MMC controller. It is one > >> step closer to knowing the input clock to the dwmmc input > >> clock. Note that it is not the clock of the MMC bus itself, > >> but the input clock to the dwmmc logic block. > > > > I don't think I quite understand what you mean here. We're > > talking about obtaining the frequency of the clock which go > > into the DWMMC IP block, right ? > > > > So, if you implement a function, say -- > > dwmmc_get_upstream_clock() -- and call it from within the > > implementation of the > > .get_mmc_clk(), which is specific for that particular chip of > > yours*, you don't need this patch. Or am I really missing > > something fundamental ? > > > > *the .get_mmc_clk() is specific to a chip, see for example > > exynos_dw_mmc.c > > The purpose of the existing code (before my change) is to find out > the input frequency of the IP block. By knowing this, the dw_mmc > driver can work out what divisor it needs to achieve a particular > MMC bus clock. > > The implementation of get_mmc_clk() (which will be in the > SoC-specific MMC driver) is indeed the place where the clock is > figured out. My only change is to add a parameter which is the > desired bus clock. This parameter can be ignored, but for > implementations which can select the source clock such that it > matches this bus clock, then they can do this and dw_mmc can just > use bypass mode.
I see now, this wasn't really clear from the patch description. Shouldn't you introduce another callback for this purpose then, like .set_mmc_clk() instead ?
We could do, but I don't like introducing another interface for one client. Also I think the right solution is to move it to use the generic clock infrastructure, when it exists (well we have it, but nothing uses it yet).
OK, but making a .get_mmc_clk() function actually configure something is a behavior I wouldn't expect from a getter function. It's a bit odd and illogical in my opinion.
Yes fair enough, it is odd. I did start an MMC uclass so perhaps that will lead to a better solution. It's unfortunately that dw_mmc need its own callback infrastructure.
I hope we can iron that out shortly. The good thing is that you now have a board with the DWMMC and SoCFPGA also has one, so we have at least two pairs of eyes on it.
Also, what do you prefer to do about this patch ? Shall we go with the .set_mmc_clock() callback and be done with it or do you want to stick with the current approach ? I'm inclined to the former as it's less confusing in my opinion.
Let's revisit it when I get back to the rockchip series.
I just checked the socfpga and I see I can use this functionality too :)
Hi Simon,
On Aug 7, 2015, at 05:16 , Simon Glass sjg@chromium.org wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) * host->bus_hz should be set by user. */ if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c index e083745..3f702ba 100644 --- a/drivers/mmc/exynos_dw_mmc.c +++ b/drivers/mmc/exynos_dw_mmc.c @@ -39,7 +39,7 @@ static void exynos_dwmci_clksel(struct dwmci_host *host) dwmci_writel(host, DWMCI_CLKSEL, priv->sdr_timing); }
-unsigned int exynos_dwmci_get_clk(struct dwmci_host *host) +unsigned int exynos_dwmci_get_clk(struct dwmci_host *host, uint freq) { unsigned long sclk; int8_t clk_div; diff --git a/include/dwmmc.h b/include/dwmmc.h index 7a7555a..25cf42c 100644 --- a/include/dwmmc.h +++ b/include/dwmmc.h @@ -163,7 +163,21 @@ struct dwmci_host {
void (*clksel)(struct dwmci_host *host); void (*board_init)(struct dwmci_host *host);
- unsigned int (*get_mmc_clk)(struct dwmci_host *host);
/**
* Get / set a particular MMC clock frequency** This is used to request the current clock frequency of the clock* that drives the DWMMC peripheral. The caller will then use this* information to work out the divider it needs to achieve the* required MMC bus clock frequency. If you want to handle the* clock external to DWMMC, use @freq to select the frequency and* return that value too. Then DWMMC will put itself in bypass mode.** @host: DWMMC host* @freq: Frequency the host is trying to achieve*/unsigned int (*get_mmc_clk)(struct dwmci_host *host, uint freq);
struct mmc_config cfg;
};
2.5.0.rc2.392.g76e840b
I’m applying this now. It makes sense. No, we don’t have a clock framework for now so this will do.
Thanks
— Pantelis
On Wednesday, August 12, 2015 at 09:39:03 AM, Pantelis Antoniou wrote:
Hi Simon,
Hi,
On Aug 7, 2015, at 05:16 , Simon Glass sjg@chromium.org wrote:
Some SoCs want to adjust the input clock to the DWMMC block as a way of controlling the MMC bus clock. Update the get_mmc_clk() method to support this.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 2 +- drivers/mmc/exynos_dw_mmc.c | 2 +- include/dwmmc.h | 16 +++++++++++++++- 3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 8f28d7e..a034c3f 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq)
* host->bus_hz should be set by user. */if (host->get_mmc_clk)
sclk = host->get_mmc_clk(host);
sclk = host->get_mmc_clk(host, freq);else if (host->bus_hz)
sclk = host->bus_hz;
else {
diff --git a/drivers/mmc/exynos_dw_mmc.c b/drivers/mmc/exynos_dw_mmc.c index e083745..3f702ba 100644 --- a/drivers/mmc/exynos_dw_mmc.c +++ b/drivers/mmc/exynos_dw_mmc.c @@ -39,7 +39,7 @@ static void exynos_dwmci_clksel(struct dwmci_host *host)
dwmci_writel(host, DWMCI_CLKSEL, priv->sdr_timing);
}
-unsigned int exynos_dwmci_get_clk(struct dwmci_host *host) +unsigned int exynos_dwmci_get_clk(struct dwmci_host *host, uint freq) {
unsigned long sclk; int8_t clk_div;
diff --git a/include/dwmmc.h b/include/dwmmc.h index 7a7555a..25cf42c 100644 --- a/include/dwmmc.h +++ b/include/dwmmc.h @@ -163,7 +163,21 @@ struct dwmci_host {
void (*clksel)(struct dwmci_host *host); void (*board_init)(struct dwmci_host *host);
- unsigned int (*get_mmc_clk)(struct dwmci_host *host);
/**
* Get / set a particular MMC clock frequency** This is used to request the current clock frequency of the clock* that drives the DWMMC peripheral. The caller will then use this* information to work out the divider it needs to achieve the* required MMC bus clock frequency. If you want to handle the* clock external to DWMMC, use @freq to select the frequency and* return that value too. Then DWMMC will put itself in bypass mode.** @host: DWMMC host* @freq: Frequency the host is trying to achieve*/unsigned int (*get_mmc_clk)(struct dwmci_host *host, uint freq);
struct mmc_config cfg;
};
I’m applying this now. It makes sense. No, we don’t have a clock framework for now so this will do.
Please don't apply patches which still have open questions, besides I still disagree with the patch.
We can calculate this. Add code to do this if it is not provided.
Signed-off-by: Simon Glass sjg@chromium.org ---
Changes in v4: None
drivers/mmc/dw_mmc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index a034c3f..cce2a5d 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -355,9 +355,15 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_IDINTEN, 0); dwmci_writel(host, DWMCI_BMOD, 1);
- if (host->fifoth_val) { - dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val); + if (!host->fifoth_val) { + uint32_t fifo_size; + + fifo_size = dwmci_readl(host, DWMCI_FIFOTH); + fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1; + host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size / 2 - 1) | + TX_WMARK(fifo_size / 2); } + dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
dwmci_writel(host, DWMCI_CLKENA, 0); dwmci_writel(host, DWMCI_CLKSRC, 0);
Hi,
Could you fix the prefix of subject with dw_mmc? :) Anyway..looks good to me.
Acked-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
On 08/07/2015 11:16 AM, Simon Glass wrote:
We can calculate this. Add code to do this if it is not provided.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None
drivers/mmc/dw_mmc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index a034c3f..cce2a5d 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -355,9 +355,15 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_IDINTEN, 0); dwmci_writel(host, DWMCI_BMOD, 1);
- if (host->fifoth_val) {
dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
if (!host->fifoth_val) {
uint32_t fifo_size;fifo_size = dwmci_readl(host, DWMCI_FIFOTH);fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size / 2 - 1) |TX_WMARK(fifo_size / 2);}
dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
dwmci_writel(host, DWMCI_CLKENA, 0); dwmci_writel(host, DWMCI_CLKSRC, 0);
Hi Simin,
On Aug 7, 2015, at 05:16 , Simon Glass sjg@chromium.org wrote:
We can calculate this. Add code to do this if it is not provided.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4: None
drivers/mmc/dw_mmc.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index a034c3f..cce2a5d 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -355,9 +355,15 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_IDINTEN, 0); dwmci_writel(host, DWMCI_BMOD, 1);
- if (host->fifoth_val) {
dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
if (!host->fifoth_val) {
uint32_t fifo_size;fifo_size = dwmci_readl(host, DWMCI_FIFOTH);fifo_size = ((fifo_size & RX_WMARK_MASK) >> RX_WMARK_SHIFT) + 1;host->fifoth_val = MSIZE(0x2) | RX_WMARK(fifo_size / 2 - 1) |TX_WMARK(fifo_size / 2);}
dwmci_writel(host, DWMCI_FIFOTH, host->fifoth_val);
dwmci_writel(host, DWMCI_CLKENA, 0); dwmci_writel(host, DWMCI_CLKSRC, 0);
-- 2.5.0.rc2.392.g76e840b
Thanks, applied (with prefix changed to dw_mmc :) )
— Pantelis
Acked-by: Jaehoon Chung jh80.chung@samsung.com
Best Regards, Jaehoon Chung
On 08/07/2015 11:16 AM, Simon Glass wrote:
The dw_mmc driver uses printf() in various places.
These bloat the code and cause problems for SPL. Use debug() where possible and try to return a useful error code instead.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 53a8aca..8f28d7e 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -8,6 +8,7 @@
#include <bouncebuf.h> #include <common.h> +#include <errno.h> #include <malloc.h> #include <mmc.h> #include <dwmmc.h> @@ -119,7 +120,7 @@ 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) {
printf("%s: Timeout on data busy\n", __func__);
debug("%s: Timeout on data busy\n", __func__); return TIMEOUT;@@ -178,7 +179,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, }
if (i == retry) {
printf("%s: Timeout.\n", __func__);
debug("%s: Timeout.\n", __func__);@@ -194,8 +195,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, debug("%s: Response Timeout.\n", __func__); return TIMEOUT; } else if (mask & DWMCI_INTMSK_RE) {
printf("%s: Response Error.\n", __func__);return -1;
debug("%s: Response Error.\n", __func__);return -EIO;@@ -214,7 +215,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, do { mask = dwmci_readl(host, DWMCI_RINTSTS); if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) {
printf("%s: DATA ERROR!\n", __func__);
debug("%s: DATA ERROR!\n", __func__); return -1; }@@ -251,7 +252,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) else if (host->bus_hz) sclk = host->bus_hz; else {
printf("%s: Didn't get source clock value.\n", __func__);
debug("%s: Didn't get source clock value.\n", __func__);@@ -270,7 +271,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) {
printf("%s: Timeout!\n", __func__);
debug("%s: Timeout!\n", __func__); return -ETIMEDOUT;@@ -285,7 +286,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) {
printf("%s: Timeout!\n", __func__);
debug("%s: Timeout!\n", __func__); return -ETIMEDOUT;@@ -339,8 +340,8 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_PWREN, 1);
if (!dwmci_wait_reset(host, DWMCI_RESET_ALL)) {
printf("%s[%d] Fail-reset!!\n", __func__, __LINE__);return -1;
debug("%s[%d] Fail-reset!!\n", __func__, __LINE__);return -EIO;}
/* Enumerate at 400KHz */
Hi Simon,
On Aug 7, 2015, at 05:16 , Simon Glass sjg@chromium.org wrote:
The dw_mmc driver uses printf() in various places.
These bloat the code and cause problems for SPL. Use debug() where possible and try to return a useful error code instead.
Signed-off-by: Simon Glass sjg@chromium.org
Changes in v4:
- Update commit message to indicate this patch is for the dw_mmc driver
drivers/mmc/dw_mmc.c | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-)
diff --git a/drivers/mmc/dw_mmc.c b/drivers/mmc/dw_mmc.c index 53a8aca..8f28d7e 100644 --- a/drivers/mmc/dw_mmc.c +++ b/drivers/mmc/dw_mmc.c @@ -8,6 +8,7 @@
#include <bouncebuf.h> #include <common.h> +#include <errno.h> #include <malloc.h> #include <mmc.h> #include <dwmmc.h> @@ -119,7 +120,7 @@ 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) {
printf("%s: Timeout on data busy\n", __func__);
debug("%s: Timeout on data busy\n", __func__); return TIMEOUT;@@ -178,7 +179,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, }
if (i == retry) {
printf("%s: Timeout.\n", __func__);
debug("%s: Timeout.\n", __func__);@@ -194,8 +195,8 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, debug("%s: Response Timeout.\n", __func__); return TIMEOUT; } else if (mask & DWMCI_INTMSK_RE) {
printf("%s: Response Error.\n", __func__);return -1;
debug("%s: Response Error.\n", __func__);return -EIO;@@ -214,7 +215,7 @@ static int dwmci_send_cmd(struct mmc *mmc, struct mmc_cmd *cmd, do { mask = dwmci_readl(host, DWMCI_RINTSTS); if (mask & (DWMCI_DATA_ERR | DWMCI_DATA_TOUT)) {
printf("%s: DATA ERROR!\n", __func__);
debug("%s: DATA ERROR!\n", __func__); return -1; }@@ -251,7 +252,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) else if (host->bus_hz) sclk = host->bus_hz; else {
printf("%s: Didn't get source clock value.\n", __func__);
debug("%s: Didn't get source clock value.\n", __func__);@@ -270,7 +271,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) {
printf("%s: Timeout!\n", __func__);
debug("%s: Timeout!\n", __func__); return -ETIMEDOUT;@@ -285,7 +286,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) do { status = dwmci_readl(host, DWMCI_CMD); if (timeout-- < 0) {
printf("%s: Timeout!\n", __func__);
debug("%s: Timeout!\n", __func__); return -ETIMEDOUT;@@ -339,8 +340,8 @@ static int dwmci_init(struct mmc *mmc) dwmci_writel(host, DWMCI_PWREN, 1);
if (!dwmci_wait_reset(host, DWMCI_RESET_ALL)) {
printf("%s[%d] Fail-reset!!\n", __func__, __LINE__);return -1;
debug("%s[%d] Fail-reset!!\n", __func__, __LINE__);return -EIO;}
/* Enumerate at 400KHz */
-- 2.5.0.rc2.392.g76e840b
Applied, with a small rework to account for a couple of printfs that Marek’s patch introduced.
Thanks
— Pantelis
participants (4)
-
Jaehoon Chung -
Marek Vasut -
Pantelis Antoniou -
Simon Glass