[PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk frequency

[Sagar Kadam]

Hello Sean,

> -----Original Message-----
> From: Sean Anderson <seanga2 at gmail.com>
> Sent: Friday, February 21, 2020 11:48 AM
> To: Sagar Kadam <sagar.kadam at sifive.com>; Bin Meng
> <bmeng.cn at gmail.com>
> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Lukasz Majewski
> <lukma at denx.de>; Anup Patel <Anup.Patel at wdc.com>; Paul Walmsley (
> Sifive) <paul.walmsley at sifive.com>; Vincent Chen
> <vincent.chen at sifive.com>
> Subject: Re: [PATCH v1 2/2] cpu: clk: riscv: populate proper CPU core clk
> frequency
> 
> On 2/21/20 12:59 AM, Sagar Kadam wrote:
> >> What you were trying to do in this patch, I believe the following 2
> >> patches already did it.
> >>
> >> http://patchwork.ozlabs.org/patch/1236177/
> >> http://patchwork.ozlabs.org/patch/1236180/
> >>
> >
> > Thanks for sharing the links. Unfortunately I didn’t come across it.
> > I applied these two patches within my repo  (assuming there are not
> > extra dependencies) and would like to share my observation:
> > The implementation in the patch links shared here read's clock dt
> > property in clk_get_by_index. If the cpu dt node doesn't contain clock
> > property it return's negative value and so the clk_get_rate here won't be be
> executed.
> >
> > +	ret = clk_get_by_index(dev, 0, &clk);
> > +	if (!ret) {
> > +		ret = clk_get_rate(&clk);
> 
> This is working as designed. The idea is to use the clocks property if it exists
> and to fall back on clock-frequency otherwise.

Thanks for clarifying. 
> 
> > Thus when I tested this on hifive unleashed the "cpu detail" showed
> frequency as 0 Hz.
> 
> This is because the cpu nodes in the hifive/fu540 device tree have neither a
> clock-frequency property or a clocks property.
> 
Yes, I will add clocks dt property.
> > Please correct me if I am wrong, but IMHO here we can check for
> > negative return code [if (ret < 0)] instead of (!ret) and if "clocks"
> > is missing in dt property then get the clock driver using
> > uclass_get_device_by_driver->request the clock -> and get clock rate,
> > just like below
> >
> > -       if (!ret) {
> > +       if (ret < 0) {
> > +               ret = uclass_get_device_by_driver(UCLASS_CLK,
> > +                                                 DM_GET_DRIVER(sifive_fu540_prci),
> > +                                                 &dev);
> 
> This is again adding board-specific code to a generic driver. Add a
> UCLASS_CLOCK driver if you want to support clocks. That way there is no
> need for code like this.
Thanks for suggestion.
I will remove board-specific code from generic driver.
> 
> > +               clk.id = 0;
> > +               ret = clk_request(dev, &clk);
> > +               if (ret < 0) {
> > +                       pr_err("%s: request to clock device
> > + failed...\n", __func__);
> 
> I belive pr_err is supported for linux compatibility. New code should use
> log_*. This is also probably debug-level and not err-level.
Ok. I will replace pr_err with log_debug.
> 
> > +                       return ret;
> > +               }
> > +
> >
> > Also there is missing "include linux/err.h" which is needed by
> > IS_ERR_VALUE
> 
> Yes, I noticed that when rebasing. It will be fixed in the next version of the
> series.
Thanks for updating.

BR,
Sagar Kadam

> 
> > Please let me know if I can rebase and update my patches above the two
> > patch's you pointed to.
> >
> >> Regards,
> >> Bin
> 
> --Sean