| Message ID | 20190702075513.17451-1-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND,V4,1/5] clocksource: timer-of: Support getting clock frequency from DT | expand |
Hi Anson, why did you resend the series? On 02/07/2019 09:55, Anson.Huang@nxp.com wrote: > From: Anson Huang <Anson.Huang@nxp.com> > > 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 <Anson.Huang@nxp.com> > --- > 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", &rate); > + 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, &to->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, &to->of_clk); > + if (ret) > + goto out_fail; > + flags |= TIMER_OF_CLOCK_FREQUENCY; > + } > + > if (to->flags & TIMER_OF_IRQ) { > ret = timer_of_irq_init(np, &to->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(&to->of_clk); > > + if (flags & TIMER_OF_CLOCK_FREQUENCY) > + to->of_clk.rate = 0; > + > if (flags & TIMER_OF_BASE) > timer_of_base_exit(&to->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 <linux/clockchips.h> > > -#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; >
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.Huang@nxp.com wrote: > > From: Anson Huang <Anson.Huang@nxp.com> > > > > 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 <Anson.Huang@nxp.com> > > --- > > 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", &rate); > > + 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, &to->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, &to->of_clk); > > + if (ret) > > + goto out_fail; > > + flags |= TIMER_OF_CLOCK_FREQUENCY; > > + } > > + > > if (to->flags & TIMER_OF_IRQ) { > > ret = timer_of_irq_init(np, &to->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(&to->of_clk); > > > > + if (flags & TIMER_OF_CLOCK_FREQUENCY) > > + to->of_clk.rate = 0; > > + > > if (flags & TIMER_OF_BASE) > > timer_of_base_exit(&to->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 <linux/clockchips.h> > > > > -#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;
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.Huang@nxp.com wrote: >>> From: Anson Huang <Anson.Huang@nxp.com> >>> >>> 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 <Anson.Huang@nxp.com> >>> --- >>> 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", &rate); >>> + 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, &to->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, &to->of_clk); >>> + if (ret) >>> + goto out_fail; >>> + flags |= TIMER_OF_CLOCK_FREQUENCY; >>> + } >>> + >>> if (to->flags & TIMER_OF_IRQ) { >>> ret = timer_of_irq_init(np, &to->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(&to->of_clk); >>> >>> + if (flags & TIMER_OF_CLOCK_FREQUENCY) >>> + to->of_clk.rate = 0; >>> + >>> if (flags & TIMER_OF_BASE) >>> timer_of_base_exit(&to->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 <linux/clockchips.h> >>> >>> -#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;
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
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", &rate); + 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, &to->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, &to->of_clk); + if (ret) + goto out_fail; + flags |= TIMER_OF_CLOCK_FREQUENCY; + } + if (to->flags & TIMER_OF_IRQ) { ret = timer_of_irq_init(np, &to->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(&to->of_clk); + if (flags & TIMER_OF_CLOCK_FREQUENCY) + to->of_clk.rate = 0; + if (flags & TIMER_OF_BASE) timer_of_base_exit(&to->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 <linux/clockchips.h> -#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;