RE: [PATCH RESEND V4 1/5] clocksource: timer-of: Support getting clock frequency from DT

[Anson Huang]

Hi, Daniel

> On 02/07/2019 11:03, Anson Huang wrote:
> > Hi, Daniel
> >
> >> Hi Anson,
> >>
> >> why did you resend the series?
> >
> > Previous patch series has build warning and I did NOT notice, sorry
> > for that,
> >
> > drivers/clocksource/timer-of.c: In function ‘timer_of_init’:
> > drivers/clocksource/timer-of.c:185:30: warning: suggest parentheses
> around comparison in operand of ‘&’ [-Wparentheses]
> >   if (to->flags & clock_flags == clock_flags)
> >   ^
> >
> > so I resend the patch series with below, sorry for missing mention of the
> changes in resent patch series.
> >
> >  +  if ((to->flags & clock_flags) == clock_flags)
> >
> > Sorry for mail storm...
> 
> No problem at all, I prefer this caught and fixed early :)
> 
> Next time just send a V5 because 'resend' means there is no change but
> there was a problem with the email (could be also interpreted as a gentle
> ping).

OK, will keep it in mind, thanks for sharing useful experience!

Thanks,
Anson

Re: [PATCH RESEND V4 1/5] clocksource: timer-of: Support getting clock frequency from DT

[Daniel Lezcano]

On 02/07/2019 11:03, Anson Huang wrote:
> Hi, Daniel
> 
>> Hi Anson,
>>
>> why did you resend the series?
> 
> Previous patch series has build warning and I did NOT notice, sorry for that,
> 
> drivers/clocksource/timer-of.c: In function ‘timer_of_init’:
> drivers/clocksource/timer-of.c:185:30: warning: suggest parentheses around 
> comparison in operand of ‘&’ [-Wparentheses]
>   if (to->flags & clock_flags == clock_flags)
>   ^
> 
> so I resend the patch series with below, sorry for missing mention of the 
> changes in resent patch series.
> 
>  +if ((to->flags & clock_flags) == clock_flags)
> 
> Sorry for mail storm...

No problem at all, I prefer this caught and fixed early :)

Next time just send a V5 because 'resend' means there is no change but
there was a problem with the email (could be also interpreted as a
gentle ping).

>> On 02/07/2019 09:55, anson.hu...@nxp.com wrote:
>>> From: Anson Huang 
>>>
>>> More and more platforms use platform driver model for clock driver, so
>>> the clock driver is NOT ready during timer initialization phase, it
>>> will cause timer initialization failed.
>>>
>>> To support those platforms with upper scenario, introducing a new flag
>>> TIMER_OF_CLOCK_FREQUENCY which is mutually exclusive with
>>> TIMER_OF_CLOCK flag to support getting timer clock frequency from DT's
>>> timer node, the property name should be "clock-frequency", then of_clk
>>> operations can be skipped.
>>>
>>> User needs to select either TIMER_OF_CLOCK_FREQUENCY or
>> TIMER_OF_CLOCK
>>> flag if want to use timer-of driver to initialize the clock rate.
>>>
>>> Signed-off-by: Anson Huang 
>>> ---
>>> Changes since V3:
>>> - use hardcoded "clock-frequency" instead of adding new variable
>> prop_name;
>>> - add pre-condition check for TIMER_OF_CLOCK and
>> TIMER_OF_CLOCK_FREQUENCY, they MUST be exclusive.
>>> ---
>>>  drivers/clocksource/timer-of.c | 29 +
>>> drivers/clocksource/timer-of.h |  7 ---
>>>  2 files changed, 33 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/clocksource/timer-of.c
>>> b/drivers/clocksource/timer-of.c index 8054228..858f684 100644
>>> --- a/drivers/clocksource/timer-of.c
>>> +++ b/drivers/clocksource/timer-of.c
>>> @@ -161,11 +161,30 @@ static __init int timer_of_base_init(struct
>> device_node *np,
>>> return 0;
>>>  }
>>>
>>> +static __init int timer_of_clk_frequency_init(struct device_node *np,
>>> + struct of_timer_clk *of_clk) {
>>> +   int ret;
>>> +   u32 rate;
>>> +
>>> +   ret = of_property_read_u32(np, "clock-frequency", );
>>> +   if (!ret) {
>>> +   of_clk->rate = rate;
>>> +   of_clk->period = DIV_ROUND_UP(rate, HZ);
>>> +   }
>>> +
>>> +   return ret;
>>> +}
>>> +
>>>  int __init timer_of_init(struct device_node *np, struct timer_of *to)
>>> {
>>> +   unsigned long clock_flags = TIMER_OF_CLOCK |
>>> +TIMER_OF_CLOCK_FREQUENCY;
>>> int ret = -EINVAL;
>>> int flags = 0;
>>>
>>> +   if ((to->flags & clock_flags) == clock_flags)
>>> +   return ret;
>>> +
>>> if (to->flags & TIMER_OF_BASE) {
>>> ret = timer_of_base_init(np, >of_base);
>>> if (ret)
>>> @@ -180,6 +199,13 @@ int __init timer_of_init(struct device_node *np,
>> struct timer_of *to)
>>> flags |= TIMER_OF_CLOCK;
>>> }
>>>
>>> +   if (to->flags & TIMER_OF_CLOCK_FREQUENCY) {
>>> +   ret = timer_of_clk_frequency_init(np, >of_clk);
>>> +   if (ret)
>>> +   goto out_fail;
>>> +   flags |= TIMER_OF_CLOCK_FREQUENCY;
>>> +   }
>>> +
>>> if (to->flags & TIMER_OF_IRQ) {
>>> ret = timer_of_irq_init(np, >of_irq);
>>> if (ret)
>>> @@ -201,6 +227,9 @@ int __init timer_of_init(struct device_node *np,
>> struct timer_of *to)
>>> if (flags & TIMER_OF_CLOCK)
>>> timer_of_clk_exit(>of_clk);
>>>
>>> +   if (flags & TIMER_OF_CLOCK_FREQUENCY)
>>> +   to->of_clk.rate = 0;
>>> +
>>> if (flags & TIMER_OF_BASE)
>>> timer_of_base_exit(>of_base);
>>> return ret;
>>> diff --git a/drivers/clocksource/timer-of.h
>>> b/drivers/clocksource/timer-of.h index a5478f3..a08e108 100644
>>> --- a/drivers/clocksource/timer-of.h
>>> +++ b/drivers/clocksource/timer-of.h
>>> @@ -4,9 +4,10 @@
>>>
>>>  #include 
>>>
>>> -#define TIMER_OF_BASE  0x1
>>> -#define TIMER_OF_CLOCK 0x2
>>> -#define TIMER_OF_IRQ   0x4
>>> +#define TIMER_OF_BASE  0x1
>>> +#define TIMER_OF_CLOCK 0x2
>>> +#define TIMER_OF_IRQ   0x4
>>> +#define TIMER_OF_CLOCK_FREQUENCY   0x8
>>>
>>>  struct of_timer_irq {
>>> int irq;


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

RE: [PATCH RESEND V4 1/5] clocksource: timer-of: Support getting clock frequency from DT

[Anson Huang]

Hi, Daniel

> Hi Anson,
> 
> why did you resend the series?

Previous patch series has build warning and I did NOT notice, sorry for that,

drivers/clocksource/timer-of.c: In function ‘timer_of_init’:
drivers/clocksource/timer-of.c:185:30: warning: suggest parentheses around 
comparison in operand of ‘&’ [-Wparentheses]
  if (to->flags & clock_flags == clock_flags)
  ^

so I resend the patch series with below, sorry for missing mention of the 
changes in resent patch series.

 +  if ((to->flags & clock_flags) == clock_flags)

Sorry for mail storm...

Thanks,
Anson

> 
> 
> On 02/07/2019 09:55, anson.hu...@nxp.com wrote:
> > From: Anson Huang 
> >
> > More and more platforms use platform driver model for clock driver, so
> > the clock driver is NOT ready during timer initialization phase, it
> > will cause timer initialization failed.
> >
> > To support those platforms with upper scenario, introducing a new flag
> > TIMER_OF_CLOCK_FREQUENCY which is mutually exclusive with
> > TIMER_OF_CLOCK flag to support getting timer clock frequency from DT's
> > timer node, the property name should be "clock-frequency", then of_clk
> > operations can be skipped.
> >
> > User needs to select either TIMER_OF_CLOCK_FREQUENCY or
> TIMER_OF_CLOCK
> > flag if want to use timer-of driver to initialize the clock rate.
> >
> > Signed-off-by: Anson Huang 
> > ---
> > Changes since V3:
> > - use hardcoded "clock-frequency" instead of adding new variable
> prop_name;
> > - add pre-condition check for TIMER_OF_CLOCK and
> TIMER_OF_CLOCK_FREQUENCY, they MUST be exclusive.
> > ---
> >  drivers/clocksource/timer-of.c | 29 +
> > drivers/clocksource/timer-of.h |  7 ---
> >  2 files changed, 33 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/clocksource/timer-of.c
> > b/drivers/clocksource/timer-of.c index 8054228..858f684 100644
> > --- a/drivers/clocksource/timer-of.c
> > +++ b/drivers/clocksource/timer-of.c
> > @@ -161,11 +161,30 @@ static __init int timer_of_base_init(struct
> device_node *np,
> > return 0;
> >  }
> >
> > +static __init int timer_of_clk_frequency_init(struct device_node *np,
> > + struct of_timer_clk *of_clk) {
> > +   int ret;
> > +   u32 rate;
> > +
> > +   ret = of_property_read_u32(np, "clock-frequency", );
> > +   if (!ret) {
> > +   of_clk->rate = rate;
> > +   of_clk->period = DIV_ROUND_UP(rate, HZ);
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> >  int __init timer_of_init(struct device_node *np, struct timer_of *to)
> > {
> > +   unsigned long clock_flags = TIMER_OF_CLOCK |
> > +TIMER_OF_CLOCK_FREQUENCY;
> > int ret = -EINVAL;
> > int flags = 0;
> >
> > +   if ((to->flags & clock_flags) == clock_flags)
> > +   return ret;
> > +
> > if (to->flags & TIMER_OF_BASE) {
> > ret = timer_of_base_init(np, >of_base);
> > if (ret)
> > @@ -180,6 +199,13 @@ int __init timer_of_init(struct device_node *np,
> struct timer_of *to)
> > flags |= TIMER_OF_CLOCK;
> > }
> >
> > +   if (to->flags & TIMER_OF_CLOCK_FREQUENCY) {
> > +   ret = timer_of_clk_frequency_init(np, >of_clk);
> > +   if (ret)
> > +   goto out_fail;
> > +   flags |= TIMER_OF_CLOCK_FREQUENCY;
> > +   }
> > +
> > if (to->flags & TIMER_OF_IRQ) {
> > ret = timer_of_irq_init(np, >of_irq);
> > if (ret)
> > @@ -201,6 +227,9 @@ int __init timer_of_init(struct device_node *np,
> struct timer_of *to)
> > if (flags & TIMER_OF_CLOCK)
> > timer_of_clk_exit(>of_clk);
> >
> > +   if (flags & TIMER_OF_CLOCK_FREQUENCY)
> > +   to->of_clk.rate = 0;
> > +
> > if (flags & TIMER_OF_BASE)
> > timer_of_base_exit(>of_base);
> > return ret;
> > diff --git a/drivers/clocksource/timer-of.h
> > b/drivers/clocksource/timer-of.h index a5478f3..a08e108 100644
> > --- a/drivers/clocksource/timer-of.h
> > +++ b/drivers/clocksource/timer-of.h
> > @@ -4,9 +4,10 @@
> >
> >  #include 
> >
> > -#define TIMER_OF_BASE  0x1
> > -#define TIMER_OF_CLOCK 0x2
> > -#define TIMER_OF_IRQ   0x4
> > +#define TIMER_OF_BASE  0x1
> > +#define TIMER_OF_CLOCK 0x2
> > +#define TIMER_OF_IRQ   0x4
> > +#define TIMER_OF_CLOCK_FREQUENCY   0x8
> >
> >  struct of_timer_irq {
> > int irq;

[PATCH RESEND V4 1/5] clocksource: timer-of: Support getting clock frequency from DT

[Anson . Huang]

From: Anson Huang 

More and more platforms use platform driver model for clock driver,
so the clock driver is NOT ready during timer initialization phase,
it will cause timer initialization failed.

To support those platforms with upper scenario, introducing a new
flag TIMER_OF_CLOCK_FREQUENCY which is mutually exclusive with
TIMER_OF_CLOCK flag to support getting timer clock frequency from
DT's timer node, the property name should be "clock-frequency",
then of_clk operations can be skipped.

User needs to select either TIMER_OF_CLOCK_FREQUENCY or TIMER_OF_CLOCK
flag if want to use timer-of driver to initialize the clock rate.

Signed-off-by: Anson Huang 
---
Changes since V3:
- use hardcoded "clock-frequency" instead of adding new variable 
prop_name;
- add pre-condition check for TIMER_OF_CLOCK and 
TIMER_OF_CLOCK_FREQUENCY, they MUST be exclusive.
---
 drivers/clocksource/timer-of.c | 29 +
 drivers/clocksource/timer-of.h |  7 ---
 2 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
index 8054228..858f684 100644
--- a/drivers/clocksource/timer-of.c
+++ b/drivers/clocksource/timer-of.c
@@ -161,11 +161,30 @@ static __init int timer_of_base_init(struct device_node 
*np,
return 0;
 }
 
+static __init int timer_of_clk_frequency_init(struct device_node *np,
+ struct of_timer_clk *of_clk)
+{
+   int ret;
+   u32 rate;
+
+   ret = of_property_read_u32(np, "clock-frequency", );
+   if (!ret) {
+   of_clk->rate = rate;
+   of_clk->period = DIV_ROUND_UP(rate, HZ);
+   }
+
+   return ret;
+}
+
 int __init timer_of_init(struct device_node *np, struct timer_of *to)
 {
+   unsigned long clock_flags = TIMER_OF_CLOCK | TIMER_OF_CLOCK_FREQUENCY;
int ret = -EINVAL;
int flags = 0;
 
+   if ((to->flags & clock_flags) == clock_flags)
+   return ret;
+
if (to->flags & TIMER_OF_BASE) {
ret = timer_of_base_init(np, >of_base);
if (ret)
@@ -180,6 +199,13 @@ int __init timer_of_init(struct device_node *np, struct 
timer_of *to)
flags |= TIMER_OF_CLOCK;
}
 
+   if (to->flags & TIMER_OF_CLOCK_FREQUENCY) {
+   ret = timer_of_clk_frequency_init(np, >of_clk);
+   if (ret)
+   goto out_fail;
+   flags |= TIMER_OF_CLOCK_FREQUENCY;
+   }
+
if (to->flags & TIMER_OF_IRQ) {
ret = timer_of_irq_init(np, >of_irq);
if (ret)
@@ -201,6 +227,9 @@ int __init timer_of_init(struct device_node *np, struct 
timer_of *to)
if (flags & TIMER_OF_CLOCK)
timer_of_clk_exit(>of_clk);
 
+   if (flags & TIMER_OF_CLOCK_FREQUENCY)
+   to->of_clk.rate = 0;
+
if (flags & TIMER_OF_BASE)
timer_of_base_exit(>of_base);
return ret;
diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
index a5478f3..a08e108 100644
--- a/drivers/clocksource/timer-of.h
+++ b/drivers/clocksource/timer-of.h
@@ -4,9 +4,10 @@
 
 #include 
 
-#define TIMER_OF_BASE  0x1
-#define TIMER_OF_CLOCK 0x2
-#define TIMER_OF_IRQ   0x4
+#define TIMER_OF_BASE  0x1
+#define TIMER_OF_CLOCK 0x2
+#define TIMER_OF_IRQ   0x4
+#define TIMER_OF_CLOCK_FREQUENCY   0x8
 
 struct of_timer_irq {
int irq;
-- 
2.7.4

Re: [PATCH RESEND V4 1/5] clocksource: timer-of: Support getting clock frequency from DT

[Daniel Lezcano]

Hi Anson,

why did you resend the series?


On 02/07/2019 09:55, anson.hu...@nxp.com wrote:
> From: Anson Huang 
> 
> More and more platforms use platform driver model for clock driver,
> so the clock driver is NOT ready during timer initialization phase,
> it will cause timer initialization failed.
> 
> To support those platforms with upper scenario, introducing a new
> flag TIMER_OF_CLOCK_FREQUENCY which is mutually exclusive with
> TIMER_OF_CLOCK flag to support getting timer clock frequency from
> DT's timer node, the property name should be "clock-frequency",
> then of_clk operations can be skipped.
> 
> User needs to select either TIMER_OF_CLOCK_FREQUENCY or TIMER_OF_CLOCK
> flag if want to use timer-of driver to initialize the clock rate.
> 
> Signed-off-by: Anson Huang 
> ---
> Changes since V3:
>   - use hardcoded "clock-frequency" instead of adding new variable 
> prop_name;
>   - add pre-condition check for TIMER_OF_CLOCK and 
> TIMER_OF_CLOCK_FREQUENCY, they MUST be exclusive.
> ---
>  drivers/clocksource/timer-of.c | 29 +
>  drivers/clocksource/timer-of.h |  7 ---
>  2 files changed, 33 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c
> index 8054228..858f684 100644
> --- a/drivers/clocksource/timer-of.c
> +++ b/drivers/clocksource/timer-of.c
> @@ -161,11 +161,30 @@ static __init int timer_of_base_init(struct device_node 
> *np,
>   return 0;
>  }
>  
> +static __init int timer_of_clk_frequency_init(struct device_node *np,
> +   struct of_timer_clk *of_clk)
> +{
> + int ret;
> + u32 rate;
> +
> + ret = of_property_read_u32(np, "clock-frequency", );
> + if (!ret) {
> + of_clk->rate = rate;
> + of_clk->period = DIV_ROUND_UP(rate, HZ);
> + }
> +
> + return ret;
> +}
> +
>  int __init timer_of_init(struct device_node *np, struct timer_of *to)
>  {
> + unsigned long clock_flags = TIMER_OF_CLOCK | TIMER_OF_CLOCK_FREQUENCY;
>   int ret = -EINVAL;
>   int flags = 0;
>  
> + if ((to->flags & clock_flags) == clock_flags)
> + return ret;
> +
>   if (to->flags & TIMER_OF_BASE) {
>   ret = timer_of_base_init(np, >of_base);
>   if (ret)
> @@ -180,6 +199,13 @@ int __init timer_of_init(struct device_node *np, struct 
> timer_of *to)
>   flags |= TIMER_OF_CLOCK;
>   }
>  
> + if (to->flags & TIMER_OF_CLOCK_FREQUENCY) {
> + ret = timer_of_clk_frequency_init(np, >of_clk);
> + if (ret)
> + goto out_fail;
> + flags |= TIMER_OF_CLOCK_FREQUENCY;
> + }
> +
>   if (to->flags & TIMER_OF_IRQ) {
>   ret = timer_of_irq_init(np, >of_irq);
>   if (ret)
> @@ -201,6 +227,9 @@ int __init timer_of_init(struct device_node *np, struct 
> timer_of *to)
>   if (flags & TIMER_OF_CLOCK)
>   timer_of_clk_exit(>of_clk);
>  
> + if (flags & TIMER_OF_CLOCK_FREQUENCY)
> + to->of_clk.rate = 0;
> +
>   if (flags & TIMER_OF_BASE)
>   timer_of_base_exit(>of_base);
>   return ret;
> diff --git a/drivers/clocksource/timer-of.h b/drivers/clocksource/timer-of.h
> index a5478f3..a08e108 100644
> --- a/drivers/clocksource/timer-of.h
> +++ b/drivers/clocksource/timer-of.h
> @@ -4,9 +4,10 @@
>  
>  #include 
>  
> -#define TIMER_OF_BASE0x1
> -#define TIMER_OF_CLOCK   0x2
> -#define TIMER_OF_IRQ 0x4
> +#define TIMER_OF_BASE0x1
> +#define TIMER_OF_CLOCK   0x2
> +#define TIMER_OF_IRQ 0x4
> +#define TIMER_OF_CLOCK_FREQUENCY 0x8
>  
>  struct of_timer_irq {
>   int irq;
> 


-- 
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog