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
