Re: [PATCH 0/4] cpufreq support for the Raspberry Pi

[Stefan Wahren]

Hi Nicolas,

>> Am 05.06.19 um 13:00 schrieb Nicolas Saenz Julienne:
>>> Hi Stefan,
>>> thanks for the review, I took note of your code comments.
>>>
>>> On Wed, 2019-06-05 at 11:46 +0200, Stefan Wahren wrote:
>>>> Hi Nicolas,
>>>>
>>>> Am 04.06.19 um 19:32 schrieb Nicolas Saenz Julienne:
>>>>> Hi all,
>>>>> this series aims at adding cpufreq support to the Raspberry Pi family of
>>>>> boards.
>>>>>
>>>>> The previous revision can be found at: 
>>>>> https://lkml.org/lkml/2019/5/20/431
>>>>>
>>>>> The series first factors out 'pllb' from clk-bcm2385 and creates a new
>>>>> clk driver that operates it over RPi's firmware interface[1]. We are
>>>>> forced to do so as the firmware 'owns' the pll and we're not allowed to
>>>>> change through the register interface directly as we might race with the
>>>>> over-temperature and under-voltage protections provided by the firmware.
>>>> it would be nice to preserve such design decision in the driver as a
>>>> comment, because the cover letter usually get lost.
>>>>> Next it creates a minimal cpufreq driver that populates the CPU's opp
>>>>> table, and registers cpufreq-dt. Which is needed as the firmware
>>>>> controls the max and min frequencies available.
>>>> I tested your series on top of Linux 5.2-rc1 with multi_v7_defconfig and
>>>> manually enable this drivers. During boot with Raspbian rootfs i'm
>>>> getting the following:
>>>>
>>>> [    1.177009] cpu cpu0: failed to get clock: -2
>>>> [    1.183643] cpufreq-dt: probe of cpufreq-dt failed with error -2
>>> This is surprising, who could be creating a platform_device for cpufreq-dt
>>> apart from raspberrypi-cpufreq? Just to make things clear, you're using the
>>> device tree from v5.2-rc1 (as opposed to the Raspbian one)?
>> sorry my fault, i thought it already has been replaced. The behavior in
>> this unexpected case is fine, since it doesn't crash.
>>
>> I replaced the the DTB with the mainline one, but now i'm getting this:
>>
>> [    4.566068] cpufreq: cpufreq_online: CPU0: Running at unlisted freq:
>> 600000 KHz
>> [    4.580690] cpu cpu0: dev_pm_opp_set_rate: Invalid target frequency 0
>> [    4.594391] cpufreq: __target_index: Failed to change cpu frequency: -22
> For the record this fixes it:
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index aa51756fd4d6..edb71eefe9cf 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -1293,7 +1293,7 @@ static int clk_core_determine_round_nolock(struct clk_core
> *core,
>         } else if (core->ops->round_rate) {
>                 rate = core->ops->round_rate(core->hw, req->rate,
>                                              &req->best_parent_rate);
> -               if (rate < 0)
> +               if (IS_ERR_VALUE(rate))
>                         return rate;
>
>                 req->rate = rate;
>
> round_rate() returns a 'long' value, yet 'pllb' in rpi3b+ goes as high as
> 2.8GHz, which only fits in an 'unsigned long'. This explains why I didn't see
> this issue with RPI2b.

Okay, i understand the problem. But the patch doesn't look like the
proper fix to me.

Maybe the better way is to implement determine_rate instead of
round_rate in the clock driver. AFAICS this provides the necessary
unsigned long.

>
> I'll add the patch to the series.
>
> Regards,
> Nicolas