mmc: dw_mmc: Avoid using printf() for errors

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 <[email protected]>

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644
@@ -248,7 +248,7 @@ static int dwmci_setup_bus(struct dwmci_host *host, u32

freq) * host->bus_hz should be set by user.

Quoted 5 lines · Simon Glass

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 <[email protected]>

Hi Simon,

Quoted 10 lines

---

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(-)

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) · line 251

Quoted 10 lines

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644
@@ -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)

drivers/mmc/dw_mmc.c +1 -1

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

Hi Marek,

Quoted 30 lines · Simon Glass

Quoted 5 lines

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 <[email protected]>

Hi Simon,

Quoted 21 lines

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644
@@ -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)

Quoted 7 lines

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) · line 251

Quoted 2 lines

drivers/mmc/dw_mmc.c +1 -1

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

Quoted 1 line · Simon Glass

Hi Marek,

Hi Simon,

Quoted 39 lines · Marek Vasut

Quoted 37 lines · Simon Glass

Quoted 5 lines

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 <[email protected]>

Hi Simon,

Quoted 21 lines

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644
@@ -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)

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host *host, u32 freq) · line 251

Quoted 2 lines

drivers/mmc/dw_mmc.c +1 -1

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

Quoted 2 lines

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 ?

Hi Marek,

Quoted 45 lines · Simon Glass

Quoted 1 line

Hi Marek,

Hi Simon,

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Simon Glass

Quoted 5 lines

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 <[email protected]>

Hi Simon,

Quoted 16 lines

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host · line 254

Quoted 8 lines

drivers/mmc/dw_mmc.c +1 -1

@@ -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 ?

Quoted 6 lines

Quoted 2 lines

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.

Quoted 1 line · Simon Glass

Hi Marek,

Hi Simon,

Quoted 53 lines · Marek Vasut

Quoted 45 lines · Simon Glass

Quoted 1 line

Hi Marek,

Hi Simon,

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Simon Glass

Quoted 5 lines

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 <[email protected]>

Hi Simon,

Quoted 16 lines

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host · line 254

Quoted 8 lines

drivers/mmc/dw_mmc.c +1 -1

@@ -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 ?

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 5 lines

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 ?

*the .get_mmc_clk() is specific to a chip, see for example exynos_dw_mmc.c

Best regards,
Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi Simon,

Quoted 53 lines · Marek Vasut

Quoted 45 lines · Simon Glass

Quoted 1 line

Hi Marek,

Hi Simon,

Quoted 40 lines · Marek Vasut

Quoted 38 lines · Simon Glass

Quoted 5 lines

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 <[email protected]>

Hi Simon,

Quoted 16 lines

---

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(-)

drivers/mmc/dw_mmc.c +0 -0

index 8f28d7e..a034c3f 100644

drivers/mmc/dw_mmc.c static int dwmci_setup_bus(struct dwmci_host · line 254

Quoted 8 lines

drivers/mmc/dw_mmc.c +1 -1

@@ -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 ?

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 5 lines

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

Hi Marek,

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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 ?

Quoted 7 lines

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.

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 35 lines

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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 ?

Quoted 7 lines

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.

Quoted 6 lines

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 ?

Quoted 6 lines

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,

Quoted 49 lines · Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 41 lines

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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 ?

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 56 lines · Simon Glass

Hi Marek,

Quoted 52 lines · Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 35 lines

Quoted 22 lines

Quoted 12 lines

Quoted 11 lines

Quoted 7 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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 ?

Quoted 7 lines

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.

Quoted 6 lines

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 ?

Quoted 4 lines

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,

Quoted 65 lines · Marek Vasut

Quoted 63 lines · Simon Glass

Hi Marek,

Quoted 52 lines · Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 37 lines

Quoted 23 lines

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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 ?

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 5 lines

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 ?

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 3 lines

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.

Quoted 68 lines · Simon Glass

Hi Marek,

Quoted 65 lines · Marek Vasut

Quoted 63 lines · Simon Glass

Hi Marek,

Quoted 52 lines · Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 37 lines

Quoted 23 lines

Quoted 13 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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 ?

Quoted 6 lines

Quoted 2 lines

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 ?

Quoted 5 lines

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 ?

Quoted 8 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines

Quoted 3 lines

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,

Quoted 84 lines · Marek Vasut

Quoted 71 lines · Simon Glass

Hi Marek,

Quoted 68 lines · Marek Vasut

Quoted 66 lines · Simon Glass

Hi Marek,

Quoted 55 lines · Marek Vasut

Quoted 1 line · Simon Glass

Hi Marek,

Hi!

[...]

Quoted 40 lines

Quoted 32 lines

Quoted 15 lines

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

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 ?

Quoted 8 lines

Quoted 4 lines

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 ?

Quoted 5 lines

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 ?

Quoted 2 lines

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

Quoted 7 lines

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.

Quoted 3 lines

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

Quoted 7 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

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 <[email protected]>

---

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(-)

drivers/mmc/dw_mmc.c +1 -1

index 8f28d7e..a034c3f 100644
@@ -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 {

drivers/mmc/exynos_dw_mmc.c +1 -1

index e083745..3f702ba 100644
@@ -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;

include/dwmmc.h +15 -1

index 7a7555a..25cf42c 100644
@@ -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;
 };

Signature · 1 line

2.5.0.rc2.392.g76e840b

Hi Simon,

Quoted 82 lines · Simon Glass

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 <[email protected]>

---

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(-)

drivers/mmc/dw_mmc.c +1 -1

index 8f28d7e..a034c3f 100644
@@ -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 {

drivers/mmc/exynos_dw_mmc.c +1 -1

index e083745..3f702ba 100644
@@ -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;

include/dwmmc.h +15 -1

index 7a7555a..25cf42c 100644
@@ -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);
 

Quoted 3 lines

include/dwmmc.h +0 -0

 	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.

We can calculate this. Add code to do this if it is not provided.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None

 drivers/mmc/dw_mmc.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

drivers/mmc/dw_mmc.c +8 -2

index a034c3f..cce2a5d 100644
@@ -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);

We can calculate this. Add code to do this if it is not provided.

Signed-off-by Simon Glass <[email protected]>

---

Changes in v4: None

drivers/mmc/dw_mmc.c | 10 ++++++++--

1 file changed, 8 insertions(+), 2 deletions(-)

drivers/mmc/dw_mmc.c +8 -2

index a034c3f..cce2a5d 100644
@@ -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);

Signature · 1 line

2.5.0.rc2.392.g76e840b

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 <[email protected]>

---

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(-)

drivers/mmc/dw_mmc.c +11 -10

index 53a8aca..8f28d7e 100644
@@ -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 */

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 <[email protected]>

---

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(-)

drivers/mmc/dw_mmc.c +11 -10

index 53a8aca..8f28d7e 100644
@@ -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 */

Signature · 1 line

2.5.0.rc2.392.g76e840b