RE: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support

[Sanil, Shruthi]

> -----Original Message-----
> From: Daniel Lezcano <daniel.lezcano@xxxxxxxxxx>
> Sent: Thursday, March 3, 2022 3:48 PM
> To: Sanil, Shruthi <shruthi.sanil@xxxxxxxxx>; tglx@xxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> devicetree@xxxxxxxxxxxxxxx
> Cc: andriy.shevchenko@xxxxxxxxxxxxxxx; mgross@xxxxxxxxxxxxxxx; Thokala,
> Srikanth <srikanth.thokala@xxxxxxxxx>; Raja Subramanian, Lakshmi Bai
> <lakshmi.bai.raja.subramanian@xxxxxxxxx>; Sangannavar, Mallikarjunappa
> <mallikarjunappa.sangannavar@xxxxxxxxx>
> Subject: Re: [PATCH v8 2/2] clocksource: Add Intel Keem Bay timer support
> 
> On 03/03/2022 07:18, Sanil, Shruthi wrote:
> 
> [ ... ]
> 
> >>>>>>> +	if (!(val & TIM_CONFIG_PRESCALER_ENABLE)) { +
> >>>>>>> pr_err("%pOF: Prescaler is not enabled\n", np); +		ret =
> >>>>>>> -ENODEV; +	}
> >>>>>>
> >>>>>> Why bail out instead of enabling the prescalar ?
> >>>>>
> >>>>> Because it is a secure register and it would be updated by the
> >>>>> bootloader.
> >>>> Should it be considered as a firmware bug ?
> >>>
> >>> No. This is a common driver across products in the series and
> >>> enablement of this bit depends on the project requirements. Hence to
> >>> be sure from driver, we added this check to avoid initialization of
> >>> the driver in the case where it cannot be functional.
> >>
> >> I'm not sure to get the meaning of 'project requirements' but (for my
> >> understanding) why not describe the timer in the DT for such
> >> projects?
> >>
> >
> > OK, I understand your point now. We can control the driver
> > initialization from device tree binding rather than add a check in the
> > driver. But isn't it good to have a check, if enabling of the bit is
> > missed out in the FW? This can help in debugging.
> 
> So if the description is in the DT but the prescaler bit is not enabled then the
> firmware is buggy, IIUC. Yeah, this check would help, may be add more
Yes, right. It would mean the FW is buggy.

> context in the error message, eg. "Firmware has not enabled the prescaler
> bit" or something like that
> 
> Thanks for the clarification

Yes, Sure. I'll update the comment accordingly.
Thank you 😊

> 
>    -- D.
> 
> 
> 
> --
> <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs
> 
> Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-
> blog/> Blog