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.y...@rock-chips.com>
>>>>> Reviewed-by: Jaehoon Chung <jh80.ch...@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 mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
