[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Kever Yang]

Init the clock rate to max-frequency from dts with clock driver api.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v3:
- using dt for max-frequency
Series-changes: 2
- using the return value

 drivers/mmc/rockchip_sdhci.c | 19 +++++++++++++++++--
 1 file changed, 17 insertions(+), 2 deletions(-)

diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
index c56e1a3..e33e35e 100644
--- a/drivers/mmc/rockchip_sdhci.c
+++ b/drivers/mmc/rockchip_sdhci.c
@@ -12,7 +12,9 @@
 #include <libfdt.h>
 #include <malloc.h>
 #include <sdhci.h>
+#include <clk.h>
 
+DECLARE_GLOBAL_DATA_PTR;
 /* 400KHz is max freq for card ID etc. Use that as min */
 #define EMMC_MIN_FREQ	400000
 
@@ -32,11 +34,24 @@ static int arasan_sdhci_probe(struct udevice *dev)
 	struct rockchip_sdhc_plat *plat = dev_get_platdata(dev);
 	struct rockchip_sdhc *prv = dev_get_priv(dev);
 	struct sdhci_host *host = &prv->host;
-	int ret;
+	int max_frequency, ret;
+	struct clk clk;
+
+
+	max_frequency = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
+			"max-frequency", 0);
+	ret = clk_get_by_index(dev, 0, &clk);
+	if (!ret) {
+		ret = clk_set_rate(&clk, max_frequency);
+		if (IS_ERR_VALUE(ret))
+			printf("%s clk set rate fail!\n", __func__);
+	} else {
+		printf("%s fail to get clk\n", __func__);
+	}
 
 	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD;
 
-	ret = sdhci_setup_cfg(&plat->cfg, host, CONFIG_ROCKCHIP_SDHCI_MAX_FREQ,
+	ret = sdhci_setup_cfg(&plat->cfg, host, max_frequency,
 			EMMC_MIN_FREQ);
 
 	host->mmc = &plat->mmc;
-- 
1.9.1

[U-Boot] [RESEND PATCH v3 2/2] dts: arm64: rk3399: add max-frequency for sdhci

[Kever Yang]

Add 'max-frequency' for sdhci node for clock init.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

Changes in v3: None

 arch/arm/dts/rk3399.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/dts/rk3399.dtsi b/arch/arm/dts/rk3399.dtsi
index 179860c..22277ff 100644
--- a/arch/arm/dts/rk3399.dtsi
+++ b/arch/arm/dts/rk3399.dtsi
@@ -188,6 +188,7 @@
 		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
 		assigned-clocks = <&cru SCLK_EMMC>;
 		assigned-clock-rates = <200000000>;
+		max-frequency = <200000000>;
 		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
 		clock-names = "clk_xin", "clk_ahb";
 		phys = <&emmc_phy>;
-- 
1.9.1

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Jaehoon Chung]

On 12/28/2016 12:32 PM, Kever Yang wrote:
> Init the clock rate to max-frequency from dts with clock driver api.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

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

Best Regards,
Jaehoon Chung

> ---
> 
> Changes in v3:
> - using dt for max-frequency
> Series-changes: 2
> - using the return value
> 
>  drivers/mmc/rockchip_sdhci.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/rockchip_sdhci.c b/drivers/mmc/rockchip_sdhci.c
> index c56e1a3..e33e35e 100644
> --- a/drivers/mmc/rockchip_sdhci.c
> +++ b/drivers/mmc/rockchip_sdhci.c
> @@ -12,7 +12,9 @@
>  #include <libfdt.h>
>  #include <malloc.h>
>  #include <sdhci.h>
> +#include <clk.h>
>  
> +DECLARE_GLOBAL_DATA_PTR;
>  /* 400KHz is max freq for card ID etc. Use that as min */
>  #define EMMC_MIN_FREQ	400000
>  
> @@ -32,11 +34,24 @@ static int arasan_sdhci_probe(struct udevice *dev)
>  	struct rockchip_sdhc_plat *plat = dev_get_platdata(dev);
>  	struct rockchip_sdhc *prv = dev_get_priv(dev);
>  	struct sdhci_host *host = &prv->host;
> -	int ret;
> +	int max_frequency, ret;
> +	struct clk clk;
> +
> +
> +	max_frequency = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
> +			"max-frequency", 0);
> +	ret = clk_get_by_index(dev, 0, &clk);
> +	if (!ret) {
> +		ret = clk_set_rate(&clk, max_frequency);
> +		if (IS_ERR_VALUE(ret))
> +			printf("%s clk set rate fail!\n", __func__);
> +	} else {
> +		printf("%s fail to get clk\n", __func__);
> +	}
>  
>  	host->quirks = SDHCI_QUIRK_WAIT_SEND_CMD;
>  
> -	ret = sdhci_setup_cfg(&plat->cfg, host, CONFIG_ROCKCHIP_SDHCI_MAX_FREQ,
> +	ret = sdhci_setup_cfg(&plat->cfg, host, max_frequency,
>  			EMMC_MIN_FREQ);
>  
>  	host->mmc = &plat->mmc;
> 

[U-Boot] [RESEND PATCH v3 2/2] dts: arm64: rk3399: add max-frequency for sdhci

[Jaehoon Chung]

On 12/28/2016 12:32 PM, Kever Yang wrote:
> Add 'max-frequency' for sdhci node for clock init.
> 
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>

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

Best Regards,
Jaehoon Chung

> ---
> 
> Changes in v3: None
> 
>  arch/arm/dts/rk3399.dtsi | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/arch/arm/dts/rk3399.dtsi b/arch/arm/dts/rk3399.dtsi
> index 179860c..22277ff 100644
> --- a/arch/arm/dts/rk3399.dtsi
> +++ b/arch/arm/dts/rk3399.dtsi
> @@ -188,6 +188,7 @@
>  		interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>;
>  		assigned-clocks = <&cru SCLK_EMMC>;
>  		assigned-clock-rates = <200000000>;
> +		max-frequency = <200000000>;
>  		clocks = <&cru SCLK_EMMC>, <&cru ACLK_EMMC>;
>  		clock-names = "clk_xin", "clk_ahb";
>  		phys = <&emmc_phy>;
> 

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Stefan Herbrechtsmeier]

Hi,

Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
> On 12/28/2016 12:32 PM, Kever Yang wrote:
>> Init the clock rate to max-frequency from dts with clock driver api.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
This is an incorrect use of the max-frequency property.

The max-frequency value limit the output clock of the mmc interface and 
depends on the controller, circuit (level shifter), board and so on. It 
doesn't represents the clock frequency of the controller.

The clock setup inside the clock framework should use the 
assigned-clock-rates property. The mmc driver should only enable the 
clock and pass the clock rate together with the max-frequency to the mmc 
framework.

Regards,
   Stefan

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Kever Yang]

Hi Stefan,

     Thanks for your review comment.
On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
> Hi,
>
> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>
>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
> This is an incorrect use of the max-frequency property.
>
> The max-frequency value limit the output clock of the mmc interface 
> and depends on the controller, circuit (level shifter), board and so 
> on. It doesn't represents the clock frequency of the controller.
>
> The clock setup inside the clock framework should use the 
> assigned-clock-rates property. The mmc driver should only enable the 
> clock and pass the clock rate together with the max-frequency to the 
> mmc framework.

I'm not good at mmc controller and driver framework, but seems that the 
sdhci core treats the max-frequency as the clock input from clock 
module, right?
What if the mmc controller max-frequency is not equal to the clock 
module output which is possible? Does kernel deal with this, and how.

Thanks,
- Kever
>
> Regards,
>   Stefan
>
>
>
>

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Jaehoon Chung]

Hi

On 12/29/2016 09:53 AM, Kever Yang wrote:
> Hi Stefan,
> 
>     Thanks for your review comment.
> On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>>
>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>> This is an incorrect use of the max-frequency property.
>>
>> The max-frequency value limit the output clock of the mmc interface and depends on the controller, circuit (level shifter), board and so on. It doesn't represents the clock frequency of the controller.
>>
>> The clock setup inside the clock framework should use the assigned-clock-rates property. The mmc driver should only enable the clock and pass the clock rate together with the max-frequency to the mmc framework.
> 
> I'm not good at mmc controller and driver framework, but seems that the sdhci core treats the max-frequency as the clock input from clock module, right?
> What if the mmc controller max-frequency is not equal to the clock module output which is possible? Does kernel deal with this, and how.

If my understanding is right, some controller should be broken the CLOCK_BASE capability. (Refer to Linux kernel)
And then they needs to get value from CMU.

host->max_clk should be used the card's maximum value.

In Linux Kernel's case
if max_frequency property is defined, assigned to mmc->f_max
and host->f_max is assigned to clk_get_rate() value. (If Broken clock_base capability)
And check "mmc->f_max > host->f_max" or "mmc->f_max == 0"
	if true
	then mmc->_f_max = f_max;
	else
	then mmc->f_max is used to "max_frequency" value.

In Conclusion,
	host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)

Kever's patch is not problem.

Best Regards,
Jaehoon Chung

> 
> Thanks,
> - Kever
>>
>> Regards,
>>   Stefan
>>
>>
>>
>>
> 
> 
> 
> 
> 

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Stefan Herbrechtsmeier]

Hi,

Am 29.12.2016 um 08:44 schrieb Jaehoon Chung:
> Hi
>
> On 12/29/2016 09:53 AM, Kever Yang wrote:
>> Hi Stefan,
>>
>>      Thanks for your review comment.
>> On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
>>> Hi,
>>>
>>> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>>>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>>>
>>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>> This is an incorrect use of the max-frequency property.
>>>
>>> The max-frequency value limit the output clock of the mmc interface and depends on the controller, circuit (level shifter), board and so on. It doesn't represents the clock frequency of the controller.
>>>
>>> The clock setup inside the clock framework should use the assigned-clock-rates property. The mmc driver should only enable the clock and pass the clock rate together with the max-frequency to the mmc framework.
>> I'm not good at mmc controller and driver framework, but seems that the sdhci core treats the max-frequency as the clock input from clock module, right?
This is true for the current u-boot implementation. But this code is 
wrong and differs from the kernel. The u-boot mmc framework doesn't 
distinguish between f_max of the mmc interface and max_clk of the host 
controller. I have already post a patch to fix this.

>> What if the mmc controller max-frequency is not equal to the clock module output which is possible? Does kernel deal with this, and how.
The kernel distinguish between clock module output frequency 
(host->max_clk) and max-frequency of the mmc interface (mmc->f_max).

> If my understanding is right, some controller should be broken the CLOCK_BASE capability. (Refer to Linux kernel)
> And then they needs to get value from CMU.
>
> host->max_clk should be used the card's maximum value.
It represents the (input) base clock of the mmc controller and not the 
card. A divider of one leads to maximum value.

> In Linux Kernel's case
> if max_frequency property is defined, assigned to mmc->f_max
> and host->f_max is assigned to clk_get_rate() value. (If Broken clock_base capability)
host->max_clk not host->f_max

> And check "mmc->f_max > host->f_max" or "mmc->f_max == 0"
> 	if true
> 	then mmc->_f_max = f_max;
> 	else
> 	then mmc->f_max is used to "max_frequency" value.
>
> In Conclusion,
> 	host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)
The conclusion is wrong. The host->max_clk isn't influenced by the 
max-frequency. The mmc drivers supplies the host->max_clk via the 
get_max_clock function if QUIRK_BROKEN_CAP_CLOCK_BASE is set. The 
mmc->f_max is equal to host->max_clk or max-frequency if set. This means 
you only need max-frequency if it is lower than the host->max_clk.

The host->max_clk is used for the calculation of the divider and 
multiplier. It represents the clock rate of the controller.
The mmc->f_max limits the clock rate of the card.

> Kever's patch is not problem.
The problem is that the patch "init the clock rate to max-frequency" and 
this is wrong and differs from the kernel which use the 
assigned-clock-rates. What happens if somebody sets the max-frequency to 
400000? Does the clock controller supports such a low frequency? What 
happens if the clock controller use a different clock as requested and 
the mmc framework assume the requested clock rate?

The mmc drivers shouldn't use the max-frequency to request a clock rate. 
It should only request the current clock rate or set a default clock 
rate independent of the max-frequency.

Regards
   Stefan

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Jaehoon Chung]

Hi Stefan,

On 12/30/2016 12:41 AM, Stefan Herbrechtsmeier wrote:
> Hi,
> 
> Am 29.12.2016 um 08:44 schrieb Jaehoon Chung:
>> Hi
>>
>> On 12/29/2016 09:53 AM, Kever Yang wrote:
>>> Hi Stefan,
>>>
>>>      Thanks for your review comment.
>>> On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
>>>> Hi,
>>>>
>>>> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>>>>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>>>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>>>>
>>>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>> This is an incorrect use of the max-frequency property.
>>>>
>>>> The max-frequency value limit the output clock of the mmc interface and depends on the controller, circuit (level shifter), board and so on. It doesn't represents the clock frequency of the controller.
>>>>
>>>> The clock setup inside the clock framework should use the assigned-clock-rates property. The mmc driver should only enable the clock and pass the clock rate together with the max-frequency to the mmc framework.
>>> I'm not good at mmc controller and driver framework, but seems that the sdhci core treats the max-frequency as the clock input from clock module, right?
> This is true for the current u-boot implementation. But this code is wrong and differs from the kernel. The u-boot mmc framework doesn't distinguish between f_max of the mmc interface and max_clk of the host controller. I have already post a patch to fix this.
> 
>>> What if the mmc controller max-frequency is not equal to the clock module output which is possible? Does kernel deal with this, and how.
> The kernel distinguish between clock module output frequency (host->max_clk) and max-frequency of the mmc interface (mmc->f_max).
> 
>> If my understanding is right, some controller should be broken the CLOCK_BASE capability. (Refer to Linux kernel)
>> And then they needs to get value from CMU.
>>
>> host->max_clk should be used the card's maximum value.
> It represents the (input) base clock of the mmc controller and not the card. A divider of one leads to maximum value.
> 
>> In Linux Kernel's case
>> if max_frequency property is defined, assigned to mmc->f_max
>> and host->f_max is assigned to clk_get_rate() value. (If Broken clock_base capability)
> host->max_clk not host->f_max
> 
>> And check "mmc->f_max > host->f_max" or "mmc->f_max == 0"
>>     if true
>>     then mmc->_f_max = f_max;
>>     else
>>     then mmc->f_max is used to "max_frequency" value.
>>
>> In Conclusion,
>>     host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)
> The conclusion is wrong. The host->max_clk isn't influenced by the max-frequency. The mmc drivers supplies the host->max_clk via the get_max_clock function if QUIRK_BROKEN_CAP_CLOCK_BASE is set. The mmc->f_max is equal to host->max_clk or max-frequency if set. This means you only need max-frequency if it is lower than the host->max_clk.

host->max_clk is influenced by max-frequency.
get_max_clock function? where is get_max_clock() function used?
Your patches didn't apply yet. waiting for Michal's review.

Did you know what means the quirk for broken clock base?
It means host->max_clk can be 0 or other. Then it should be lower than min_clk likes your mentions.
To prevent this, getting from max_frequency property.

> 
> The host->max_clk is used for the calculation of the divider and multiplier. It represents the clock rate of the controller.
> The mmc->f_max limits the clock rate of the card.

Yes, mmc->f_max is card's maximum frequency.
You can see how to control the clock in drivers/mmc/mmc.c

Kernel and u-boot have to check the card and host's clock values.
And needs to choose the one of them.. f_max is not getting from card. Also it's assigned from host controller.

Really ideal approach is "Using the clk_set_rate()/clk_get_rate() from CMU"..but some controllers don't fully support yet the CMU.
Of_course, it needs to consider the base clock broken case.

> 
>> Kever's patch is not problem.
> The problem is that the patch "init the clock rate to max-frequency" and this is wrong and differs from the kernel which use the assigned-clock-rates. What happens if somebody sets the max-frequency to 400000? Does the clock controller supports such a low frequency? What happens if the clock controller use a different clock as requested and the mmc framework assume the requested clock rate?

Agreed this point, It needs to implement the clk_set_rate() in rockchip_sdhci.c. with value passed by set_ios().

> 
> The mmc drivers shouldn't use the max-frequency to request a clock rate. It should only request the current clock rate or set a default clock rate independent of the max-frequency.
> 
> Regards
>   Stefan
> 
> 
> 

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Stefan Herbrechtsmeier]

Hi,

Am 30.12.2016 um 01:13 schrieb Jaehoon Chung:
> Hi Stefan,
>
> On 12/30/2016 12:41 AM, Stefan Herbrechtsmeier wrote:
>> Hi,
>>
>> Am 29.12.2016 um 08:44 schrieb Jaehoon Chung:
>>> Hi
>>>
>>> On 12/29/2016 09:53 AM, Kever Yang wrote:
>>>> Hi Stefan,
>>>>
>>>>       Thanks for your review comment.
>>>> On 12/29/2016 02:35 AM, Stefan Herbrechtsmeier wrote:
>>>>> Hi,
>>>>>
>>>>> Am 28.12.2016 um 12:01 schrieb Jaehoon Chung:
>>>>>> On 12/28/2016 12:32 PM, Kever Yang wrote:
>>>>>>> Init the clock rate to max-frequency from dts with clock driver api.
>>>>>>>
>>>>>>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>>>>>> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>
>>>>> This is an incorrect use of the max-frequency property.
>>>>>
>>>>> The max-frequency value limit the output clock of the mmc interface and depends on the controller, circuit (level shifter), board and so on. It doesn't represents the clock frequency of the controller.
>>>>>
>>>>> The clock setup inside the clock framework should use the assigned-clock-rates property. The mmc driver should only enable the clock and pass the clock rate together with the max-frequency to the mmc framework.
>>>> I'm not good at mmc controller and driver framework, but seems that the sdhci core treats the max-frequency as the clock input from clock module, right?
>> This is true for the current u-boot implementation. But this code is wrong and differs from the kernel. The u-boot mmc framework doesn't distinguish between f_max of the mmc interface and max_clk of the host controller. I have already post a patch to fix this.
>>
>>>> What if the mmc controller max-frequency is not equal to the clock module output which is possible? Does kernel deal with this, and how.
>> The kernel distinguish between clock module output frequency (host->max_clk) and max-frequency of the mmc interface (mmc->f_max).
>>
>>> If my understanding is right, some controller should be broken the CLOCK_BASE capability. (Refer to Linux kernel)
>>> And then they needs to get value from CMU.
>>>
>>> host->max_clk should be used the card's maximum value.
>> It represents the (input) base clock of the mmc controller and not the card. A divider of one leads to maximum value.
>>
>>> In Linux Kernel's case
>>> if max_frequency property is defined, assigned to mmc->f_max
>>> and host->f_max is assigned to clk_get_rate() value. (If Broken clock_base capability)
>> host->max_clk not host->f_max
>>
>>> And check "mmc->f_max > host->f_max" or "mmc->f_max == 0"
>>>      if true
>>>      then mmc->_f_max = f_max;
>>>      else
>>>      then mmc->f_max is used to "max_frequency" value.
>>>
>>> In Conclusion,
>>>      host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)
>> The conclusion is wrong. The host->max_clk isn't influenced by the max-frequency. The mmc drivers supplies the host->max_clk via the get_max_clock function if QUIRK_BROKEN_CAP_CLOCK_BASE is set. The mmc->f_max is equal to host->max_clk or max-frequency if set. This means you only need max-frequency if it is lower than the host->max_clk.
My comments refer to the linux kernel sdhci implementation.

> host->max_clk is influenced by max-frequency.
Where is the host->max_clk influenced by the max-frequency?

> get_max_clock function? where is get_max_clock() function used?
It is used in the kernel to set host->max_clk if the 
SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN is set.

> Your patches didn't apply yet. waiting for Michal's review.
Only one patch is waiting Michal's review and the mmc clock separation 
could be applied without the other patches.

> Did you know what means the quirk for broken clock base?
> It means host->max_clk can be 0 or other.
It means that the host->max_clk could not be extracted from the 
SDHCI_CAPABILITIES and need to be provided by the driver.

>   Then it should be lower than min_clk likes your mentions.
> To prevent this, getting from max_frequency property.
Why do you think that max-frequency influences the host->max_clk?
The host->max_clk could be read from the SDHCI_CAPABILITIES or need to 
be provided by the driver. The host->max_clk is a fixed clock rate and 
the mmc->f_max limits the generated frequencies of the sdhci controller. 
The host->max_clk depends on the processor and the mmc->f_max depends on 
the board, card and so on.

>> The host->max_clk is used for the calculation of the divider and multiplier. It represents the clock rate of the controller.
>> The mmc->f_max limits the clock rate of the card.
> Yes, mmc->f_max is card's maximum frequency.
> You can see how to control the clock in drivers/mmc/mmc.c
>
> Kernel and u-boot have to check the card and host's clock values.
> And needs to choose the one of them.. f_max is not getting from card. Also it's assigned from host controller.
The mmc->f_max and host->max_clk have different purposes. The mmc->f_max 
limits the requested frequency. The host->max_clk represents the clock 
frequency in front of the divider and optional multiplier and is used to 
calculate the divider and multiplier. The host->max_clk depends on the 
CMU and is never changed. The mmc->f_max depends on the board.

> Really ideal approach is "Using the clk_set_rate()/clk_get_rate() from CMU"..but some controllers don't fully support yet the CMU.
You only need the clk_get_rate() to get the host->max_clk or let the 
driver set this value as my patch does.

> Of_course, it needs to consider the base clock broken case.
The whole discussion is about the base clock broken case. Otherwise the 
host->max_clk is extracted from the SDHCI_CAPABILITIES.
The linux kernel use a callback to request the host->max_clk from the 
driver in the base clock broken case. The current u-boot implementation 
only supports the host->max_clk but call it unfortunately 
mmc->cfg->f_max which could be mistaken as mmc->f_max from the kernel 
which represents max-frequency.

>>> Kever's patch is not problem.
>> The problem is that the patch "init the clock rate to max-frequency" and this is wrong and differs from the kernel which use the assigned-clock-rates. What happens if somebody sets the max-frequency to 400000? Does the clock controller supports such a low frequency? What happens if the clock controller use a different clock as requested and the mmc framework assume the requested clock rate?
> Agreed this point, It needs to implement the clk_set_rate() in rockchip_sdhci.c. with value passed by set_ios().
Does we speak about the sdhci or dw_mmc controller? The sdhci don't 
change the host->max_clk and don't need the clk_set_rate(). It have its 
own divider and optional multiplier and doesn't change the base clock.

>> The mmc drivers shouldn't use the max-frequency to request a clock rate. It should only request the current clock rate or set a default clock rate independent of the max-frequency.

Back to this patch. It should use the CONFIG_ROCKCHIP_SDHCI_MAX_FREQ in 
the clk_set_rate() or use the default rate and only request it with 
clk_get_rate().

Maybe my last patch could be generalized and the max-frequency support 
could be moved inside the sdhci driver.

Regards
   Stefan

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Jaehoon Chung]

Hi Stefan,

On 12/31/2016 12:07 AM, Stefan Herbrechtsmeier wrote:
> Hi,

[snip]

>>>> In Conclusion,
>>>>      host's maximum value is used. ("max_frequency" property is used to QUIRK_BROKEN_CAP_CLOCK_BASE in Linux kernel.)
>>> The conclusion is wrong. The host->max_clk isn't influenced by the max-frequency. The mmc drivers supplies the host->max_clk via the get_max_clock function if QUIRK_BROKEN_CAP_CLOCK_BASE is set. The mmc->f_max is equal to host->max_clk or max-frequency if set. This means you only need max-frequency if it is lower than the host->max_clk.
> My comments refer to the linux kernel sdhci implementation.

I knew it's referred to Linux kernel. 

> 
>> host->max_clk is influenced by max-frequency.
> Where is the host->max_clk influenced by the max-frequency?
> 
>> get_max_clock function? where is get_max_clock() function used?
> It is used in the kernel to set host->max_clk if the SDHCI_QUIRK_CAP_CLOCK_BASE_BROKEN is set.
> 
>> Your patches didn't apply yet. waiting for Michal's review.
> Only one patch is waiting Michal's review and the mmc clock separation could be applied without the other patches.

Yep, If there are no issue, i will pick the patch relevant to MMC.
My meaning isn't "your patch is wrong". Your patch is right way.
but if u-boot didn't implement everything like kernel, I'm believing that current u-boot is changing to status likes Linux kernel

> 
>> Did you know what means the quirk for broken clock base?
>> It means host->max_clk can be 0 or other.
> It means that the host->max_clk could not be extracted from the SDHCI_CAPABILITIES and need to be provided by the driver.
> 
>>   Then it should be lower than min_clk likes your mentions.
>> To prevent this, getting from max_frequency property.
> Why do you think that max-frequency influences the host->max_clk?
> The host->max_clk could be read from the SDHCI_CAPABILITIES or need to be provided by the driver. The host->max_clk is a fixed clock rate and the mmc->f_max limits the generated frequencies of the sdhci controller. The host->max_clk depends on the processor and the mmc->f_max depends on the board, card and so on.
> 
>>> The host->max_clk is used for the calculation of the divider and multiplier. It represents the clock rate of the controller.
>>> The mmc->f_max limits the clock rate of the card.
>> Yes, mmc->f_max is card's maximum frequency.
>> You can see how to control the clock in drivers/mmc/mmc.c
>>
>> Kernel and u-boot have to check the card and host's clock values.
>> And needs to choose the one of them.. f_max is not getting from card. Also it's assigned from host controller.
> The mmc->f_max and host->max_clk have different purposes. The mmc->f_max limits the requested frequency. The host->max_clk represents the clock frequency in front of the divider and optional multiplier and is used to calculate the divider and multiplier. The host->max_clk depends on the CMU and is never changed. The mmc->f_max depends on the board.
> 
>> Really ideal approach is "Using the clk_set_rate()/clk_get_rate() from CMU"..but some controllers don't fully support yet the CMU.
> You only need the clk_get_rate() to get the host->max_clk or let the driver set this value as my patch does.

No. we needs to have both "clk_get_rate() and clk_set_rate()".

> 
>> Of_course, it needs to consider the base clock broken case.
> The whole discussion is about the base clock broken case. Otherwise the host->max_clk is extracted from the SDHCI_CAPABILITIES.
> The linux kernel use a callback to request the host->max_clk from the driver in the base clock broken case. The current u-boot implementation only supports the host->max_clk but call it unfortunately mmc->cfg->f_max which could be mistaken as mmc->f_max from the kernel which represents max-frequency.
> 

Linux kernel has two options. One is get_max_clock(), the other is "using 'max-frequency' property".

>>>> Kever's patch is not problem.
>>> The problem is that the patch "init the clock rate to max-frequency" and this is wrong and differs from the kernel which use the assigned-clock-rates. What happens if somebody sets the max-frequency to 400000? Does the clock controller supports such a low frequency? What happens if the clock controller use a different clock as requested and the mmc framework assume the requested clock rate?
>> Agreed this point, It needs to implement the clk_set_rate() in rockchip_sdhci.c. with value passed by set_ios().
> Does we speak about the sdhci or dw_mmc controller? The sdhci don't change the host->max_clk and don't need the clk_set_rate(). It have its own divider and optional multiplier and doesn't change the base clock.
> 

sdhci and dwmmc controller are using the clk_set_rate() in each SoC's drivers.
Since driver's divider has a limitation. So i think it needs to use the "clk_set_rate()".

>>> The mmc drivers shouldn't use the max-frequency to request a clock rate. It should only request the current clock rate or set a default clock rate independent of the max-frequency.
> 
> Back to this patch. It should use the CONFIG_ROCKCHIP_SDHCI_MAX_FREQ in the clk_set_rate() or use the default rate and only request it with clk_get_rate().
> 
> Maybe my last patch could be generalized and the max-frequency support could be moved inside the sdhci driver.

I don't want to use CONFIG_ROCKCHIP_SDHCI_MAX_FREQ...i will remove the all CONFIG_xxxx for using value.

Best Regards,
Jaehoon Chung

> 
> Regards
>   Stefan
> 
> 
> 
> 

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Stefan Herbrechtsmeier]

Hi,

Am 02.01.2017 um 02:29 schrieb Jaehoon Chung:
> Hi Stefan,
[snip]

>>> Of_course, it needs to consider the base clock broken case.
>> The whole discussion is about the base clock broken case. Otherwise the host->max_clk is extracted from the SDHCI_CAPABILITIES.
>> The linux kernel use a callback to request the host->max_clk from the driver in the base clock broken case. The current u-boot implementation only supports the host->max_clk but call it unfortunately mmc->cfg->f_max which could be mistaken as mmc->f_max from the kernel which represents max-frequency.
>>
> Linux kernel has two options. One is get_max_clock(), the other is "using 'max-frequency' property".
The 'max-frequency' is read in host.c into f_max and used to limit the 
maximum clock in mmc_set_clock() in core.c.

In sdhci.c the f_max is set to max_clk if f_max is zero or higher then 
max_clk.

The whole sdhci.c use only the max_clk to calculate the divider and 
multiplier for the requested clock.

Most sdhci-*.c drivers use a preset clock rate or set a fixed clock rate.

Only  the  sdhci-bcm-kona.c and sdhci-st.c use the f_max for 
clk_set_rate(). They should instead use the 'clock-frequency' or 
'assigned-clock-rates'. Especially the last assume three specific clock 
rates and set the clock to a minimum of 50 MHz even for a lower 
'max-frequency'.

You always need to values. One to set the clock and one to limit the 
max-frequency independent of the base clock.

>>>>> Kever's patch is not problem.
>>>> The problem is that the patch "init the clock rate to max-frequency" and this is wrong and differs from the kernel which use the assigned-clock-rates. What happens if somebody sets the max-frequency to 400000? Does the clock controller supports such a low frequency? What happens if the clock controller use a different clock as requested and the mmc framework assume the requested clock rate?
>>> Agreed this point, It needs to implement the clk_set_rate() in rockchip_sdhci.c. with value passed by set_ios().
>> Does we speak about the sdhci or dw_mmc controller? The sdhci don't change the host->max_clk and don't need the clk_set_rate(). It have its own divider and optional multiplier and doesn't change the base clock.
>>
> sdhci and dwmmc controller are using the clk_set_rate() in each SoC's drivers.
Where can I find this code? Even in u-boot some drivers use the preset 
clock values.

> Since driver's divider has a limitation. So i think it needs to use the "clk_set_rate()".
Do you plan to change all Linux drivers?

>
>>>> The mmc drivers shouldn't use the max-frequency to request a clock rate. It should only request the current clock rate or set a default clock rate independent of the max-frequency.
>> Back to this patch. It should use the CONFIG_ROCKCHIP_SDHCI_MAX_FREQ in the clk_set_rate() or use the default rate and only request it with clk_get_rate().
>>
>> Maybe my last patch could be generalized and the max-frequency support could be moved inside the sdhci driver.
> I don't want to use CONFIG_ROCKCHIP_SDHCI_MAX_FREQ...i will remove the all CONFIG_xxxx for using value.
But then you need to encode this specific values inside the device tree 
and don't misuse the 'max-frequency' property. You need to support a 
fixed clock frequency for sdhci controller with a lower 'max-frequency' 
for the external mmc interface.

Regards
   Stefan

[U-Boot] [RESEND PATCH v3 1/2] mmc: rockchip_sdhci: add clock init for mmc

[Simon Glass]

Hi,

On 27 December 2016 at 20:32, Kever Yang <kever.yang@rock-chips.com> wrote:
> Init the clock rate to max-frequency from dts with clock driver api.
>
> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
> ---
>
> Changes in v3:
> - using dt for max-frequency
> Series-changes: 2
> - using the return value
>
>  drivers/mmc/rockchip_sdhci.c | 19 +++++++++++++++++--
>  1 file changed, 17 insertions(+), 2 deletions(-)

I know there is ongoing discussion here. But this has been in my queue
for a while so I'd like to apply this and deal with tiny-ups with
follow-on patches.

Applied to u-boot-rockchip, thanks!

Regards,
Simon

[U-Boot] [RESEND PATCH v3 2/2] dts: arm64: rk3399: add max-frequency for sdhci

[Simon Glass]

On 28 December 2016 at 04:01, Jaehoon Chung <jh80.chung@samsung.com> wrote:
> On 12/28/2016 12:32 PM, Kever Yang wrote:
>> Add 'max-frequency' for sdhci node for clock init.
>>
>> Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
>
> Reviewed-by: Jaehoon Chung <jh80.chung@samsung.com>

Applied to u-boot-rockchip, thanks!