* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:16 [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Simon Glass
@ 2015-08-07 2:16 ` Simon Glass
2015-08-07 2:51 ` Marek Vasut
2015-08-12 7:39 ` Pantelis Antoniou
2015-08-07 2:16 ` [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided Simon Glass
` (2 subsequent siblings)
3 siblings, 2 replies; 23+ messages in thread
From: Simon Glass @ 2015-08-07 2:16 UTC (permalink / raw)
To: u-boot
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;
};
--
2.5.0.rc2.392.g76e840b
^ permalink raw reply related [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:16 ` [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method Simon Glass
@ 2015-08-07 2:51 ` Marek Vasut
2015-08-07 2:54 ` Simon Glass
2015-08-12 7:39 ` Pantelis Antoniou
1 sibling, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2015-08-07 2:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:51 ` Marek Vasut
@ 2015-08-07 2:54 ` Simon Glass
2015-08-07 2:58 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-07 2:54 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:54 ` Simon Glass
@ 2015-08-07 2:58 ` Marek Vasut
2015-08-07 3:00 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2015-08-07 2:58 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:58 ` Marek Vasut
@ 2015-08-07 3:00 ` Simon Glass
2015-08-07 3:07 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-07 3:00 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 3:00 ` Simon Glass
@ 2015-08-07 3:07 ` Marek Vasut
2015-08-07 5:51 ` Jaehoon Chung
2015-08-12 13:04 ` Simon Glass
0 siblings, 2 replies; 23+ messages in thread
From: Marek Vasut @ 2015-08-07 3:07 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 3:07 ` Marek Vasut
@ 2015-08-07 5:51 ` Jaehoon Chung
2015-08-12 13:04 ` Simon Glass
1 sibling, 0 replies; 23+ messages in thread
From: Jaehoon Chung @ 2015-08-07 5:51 UTC (permalink / raw)
To: u-boot
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
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 3:07 ` Marek Vasut
2015-08-07 5:51 ` Jaehoon Chung
@ 2015-08-12 13:04 ` Simon Glass
2015-08-12 13:48 ` Marek Vasut
1 sibling, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-12 13:04 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 13:04 ` Simon Glass
@ 2015-08-12 13:48 ` Marek Vasut
2015-08-12 13:51 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2015-08-12 13:48 UTC (permalink / raw)
To: u-boot
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!
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 13:48 ` Marek Vasut
@ 2015-08-12 13:51 ` Simon Glass
2015-08-12 13:53 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-12 13:51 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 13:51 ` Simon Glass
@ 2015-08-12 13:53 ` Marek Vasut
2015-08-12 13:55 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2015-08-12 13:53 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 13:53 ` Marek Vasut
@ 2015-08-12 13:55 ` Simon Glass
2015-08-12 14:40 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-12 13:55 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 13:55 ` Simon Glass
@ 2015-08-12 14:40 ` Marek Vasut
2015-08-12 21:09 ` Simon Glass
0 siblings, 1 reply; 23+ messages in thread
From: Marek Vasut @ 2015-08-12 14:40 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 14:40 ` Marek Vasut
@ 2015-08-12 21:09 ` Simon Glass
2015-08-12 22:11 ` Marek Vasut
0 siblings, 1 reply; 23+ messages in thread
From: Simon Glass @ 2015-08-12 21:09 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 21:09 ` Simon Glass
@ 2015-08-12 22:11 ` Marek Vasut
0 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2015-08-12 22:11 UTC (permalink / raw)
To: u-boot
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 :)
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-07 2:16 ` [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method Simon Glass
2015-08-07 2:51 ` Marek Vasut
@ 2015-08-12 7:39 ` Pantelis Antoniou
2015-08-12 8:55 ` Marek Vasut
1 sibling, 1 reply; 23+ messages in thread
From: Pantelis Antoniou @ 2015-08-12 7:39 UTC (permalink / raw)
To: u-boot
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);
> 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;
> };
> --
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method
2015-08-12 7:39 ` Pantelis Antoniou
@ 2015-08-12 8:55 ` Marek Vasut
0 siblings, 0 replies; 23+ messages in thread
From: Marek Vasut @ 2015-08-12 8:55 UTC (permalink / raw)
To: u-boot
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.
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided
2015-08-07 2:16 [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Simon Glass
2015-08-07 2:16 ` [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method Simon Glass
@ 2015-08-07 2:16 ` Simon Glass
2015-08-07 4:26 ` Jaehoon Chung
2015-08-12 7:40 ` Pantelis Antoniou
2015-08-07 4:22 ` [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Jaehoon Chung
2015-08-12 7:37 ` Pantelis Antoniou
3 siblings, 2 replies; 23+ messages in thread
From: Simon Glass @ 2015-08-07 2:16 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply related [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided
2015-08-07 2:16 ` [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided Simon Glass
@ 2015-08-07 4:26 ` Jaehoon Chung
2015-08-12 7:40 ` Pantelis Antoniou
1 sibling, 0 replies; 23+ messages in thread
From: Jaehoon Chung @ 2015-08-07 4:26 UTC (permalink / raw)
To: u-boot
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);
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided
2015-08-07 2:16 ` [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided Simon Glass
2015-08-07 4:26 ` Jaehoon Chung
@ 2015-08-12 7:40 ` Pantelis Antoniou
1 sibling, 0 replies; 23+ messages in thread
From: Pantelis Antoniou @ 2015-08-12 7:40 UTC (permalink / raw)
To: u-boot
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors
2015-08-07 2:16 [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Simon Glass
2015-08-07 2:16 ` [U-Boot] [PATCH v4 2/3] mmc: dw_mmc: Support bypass mode with the get_mmc_clk() method Simon Glass
2015-08-07 2:16 ` [U-Boot] [PATCH v4 3/3] mmc: Calculate dwmmc FIFO threshold size if not provided Simon Glass
@ 2015-08-07 4:22 ` Jaehoon Chung
2015-08-12 7:37 ` Pantelis Antoniou
3 siblings, 0 replies; 23+ messages in thread
From: Jaehoon Chung @ 2015-08-07 4:22 UTC (permalink / raw)
To: u-boot
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__);
> 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 */
>
^ permalink raw reply [flat|nested] 23+ messages in thread* [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors
2015-08-07 2:16 [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Simon Glass
` (2 preceding siblings ...)
2015-08-07 4:22 ` [U-Boot] [PATCH v4 1/3] mmc: dw_mmc: Avoid using printf() for errors Jaehoon Chung
@ 2015-08-12 7:37 ` Pantelis Antoniou
3 siblings, 0 replies; 23+ messages in thread
From: Pantelis Antoniou @ 2015-08-12 7:37 UTC (permalink / raw)
To: u-boot
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__);
> 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 */
> --
> 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
^ permalink raw reply [flat|nested] 23+ messages in thread