| Message ID | 20190623123850.22584-1-Anson.Huang@nxp.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [RESEND,V2,1/3] clocksource/drivers/sysctr: Add optional clock-frequency property | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success |
On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > From: Anson Huang <Anson.Huang@nxp.com> > > Systems which use platform driver model for clock driver require the > clock frequency to be supplied via device tree when system counter > driver is enabled. > > This is necessary as in the platform driver model the of_clk operations > do not work correctly because system counter driver is initialized in > early phase of system boot up, and clock driver using platform driver > model is NOT ready at that time, it will cause system counter driver > initialization failed. > > Add the optinal clock-frequency to the device tree bindings of the NXP > system counter, so the frequency can be handed in and the of_clk > operations can be skipped. Isn't it possible to create a fixed-clock and refer to it? So no need to create a specific action before calling timer_of_init() ? > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V1: > - improve commit log, no content change. > --- > Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > index d576599..c9907a0 100644 > --- a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > @@ -14,6 +14,11 @@ Required properties: > - clocks : Specifies the counter clock. > - clock-names: Specifies the clock's name of this module > > +Optional properties: > + > +- clock-frequency : Specifies system counter clock frequency and indicates system > + counter driver to skip clock operations. > + > Example: > > system_counter: timer@306a0000 { > @@ -22,4 +27,5 @@ Example: > clocks = <&clk_8m>; > clock-names = "per"; > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > + clock-frequency = <8333333>; > }; >
Hi, Daniel > On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > > From: Anson Huang <Anson.Huang@nxp.com> > > > > Systems which use platform driver model for clock driver require the > > clock frequency to be supplied via device tree when system counter > > driver is enabled. > > > > This is necessary as in the platform driver model the of_clk > > operations do not work correctly because system counter driver is > > initialized in early phase of system boot up, and clock driver using > > platform driver model is NOT ready at that time, it will cause system > > counter driver initialization failed. > > > > Add the optinal clock-frequency to the device tree bindings of the NXP > > system counter, so the frequency can be handed in and the of_clk > > operations can be skipped. > > Isn't it possible to create a fixed-clock and refer to it? So no need to create a > specific action before calling timer_of_init() ? > As the clock must be ready before the TIMER_OF_DECLARE, so adding a CLK_OF_DECLARE_DRIVER in clock driver to ONLY register a fixed-clock? The system counter's frequency are different on different platforms, so adding fixed clock in system counter driver is NOT a good idea, ONLY the DT node or the clock driver can create this fixed clock according to platforms, can you advise where to create this fixed clock is better? Thanks, Anson
On 26/06/2019 03:42, Anson Huang wrote: > Hi, Daniel > >> On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: >>> From: Anson Huang <Anson.Huang@nxp.com> >>> >>> Systems which use platform driver model for clock driver require the >>> clock frequency to be supplied via device tree when system counter >>> driver is enabled. >>> >>> This is necessary as in the platform driver model the of_clk >>> operations do not work correctly because system counter driver is >>> initialized in early phase of system boot up, and clock driver using >>> platform driver model is NOT ready at that time, it will cause system >>> counter driver initialization failed. >>> >>> Add the optinal clock-frequency to the device tree bindings of the NXP >>> system counter, so the frequency can be handed in and the of_clk >>> operations can be skipped. >> >> Isn't it possible to create a fixed-clock and refer to it? So no need to create a >> specific action before calling timer_of_init() ? >> > > As the clock must be ready before the TIMER_OF_DECLARE, so adding a CLK_OF_DECLARE_DRIVER in > clock driver to ONLY register a fixed-clock? The system counter's frequency are different on different > platforms, so adding fixed clock in system counter driver is NOT a good idea, ONLY the DT node or the > clock driver can create this fixed clock according to platforms, can you advise where to create this fixed > clock is better? Can you point me to a DT with the "nxp,sysctr-timer" ?
Hi, Daniel > On 26/06/2019 03:42, Anson Huang wrote: > > Hi, Daniel > > > >> On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > >>> From: Anson Huang <Anson.Huang@nxp.com> > >>> > >>> Systems which use platform driver model for clock driver require the > >>> clock frequency to be supplied via device tree when system counter > >>> driver is enabled. > >>> > >>> This is necessary as in the platform driver model the of_clk > >>> operations do not work correctly because system counter driver is > >>> initialized in early phase of system boot up, and clock driver using > >>> platform driver model is NOT ready at that time, it will cause > >>> system counter driver initialization failed. > >>> > >>> Add the optinal clock-frequency to the device tree bindings of the > >>> NXP system counter, so the frequency can be handed in and the of_clk > >>> operations can be skipped. > >> > >> Isn't it possible to create a fixed-clock and refer to it? So no need > >> to create a specific action before calling timer_of_init() ? > >> > > > > As the clock must be ready before the TIMER_OF_DECLARE, so adding a > > CLK_OF_DECLARE_DRIVER in clock driver to ONLY register a fixed-clock? > > The system counter's frequency are different on different platforms, > > so adding fixed clock in system counter driver is NOT a good idea, > > ONLY the DT node or the clock driver can create this fixed clock according to > platforms, can you advise where to create this fixed clock is better? > > Can you point me to a DT with the "nxp,sysctr-timer" ? The DT node of system counter is new added in 3/3 of this patch series, also can be found from below link: https://patchwork.kernel.org/patch/11011703/ thanks, Anson
On 27/06/2019 02:43, Anson Huang wrote: > Hi, Daniel > >> On 26/06/2019 03:42, Anson Huang wrote: >>> Hi, Daniel >>> >>>> On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: >>>>> From: Anson Huang <Anson.Huang@nxp.com> >>>>> >>>>> Systems which use platform driver model for clock driver require the >>>>> clock frequency to be supplied via device tree when system counter >>>>> driver is enabled. >>>>> >>>>> This is necessary as in the platform driver model the of_clk >>>>> operations do not work correctly because system counter driver is >>>>> initialized in early phase of system boot up, and clock driver using >>>>> platform driver model is NOT ready at that time, it will cause >>>>> system counter driver initialization failed. >>>>> >>>>> Add the optinal clock-frequency to the device tree bindings of the >>>>> NXP system counter, so the frequency can be handed in and the of_clk >>>>> operations can be skipped. >>>> >>>> Isn't it possible to create a fixed-clock and refer to it? So no need >>>> to create a specific action before calling timer_of_init() ? >>>> >>> >>> As the clock must be ready before the TIMER_OF_DECLARE, so adding a >>> CLK_OF_DECLARE_DRIVER in clock driver to ONLY register a fixed-clock? >>> The system counter's frequency are different on different platforms, >>> so adding fixed clock in system counter driver is NOT a good idea, >>> ONLY the DT node or the clock driver can create this fixed clock according to >> platforms, can you advise where to create this fixed clock is better? >> >> Can you point me to a DT with the "nxp,sysctr-timer" ? > > The DT node of system counter is new added in 3/3 of this patch series, also can be found > from below link: > https://patchwork.kernel.org/patch/11011703/ Sorry, I was unclear. I meant a patch with the timer defined using a clock as defined currently in the binding (no clock-frequency).
Hi, Daniel > On 27/06/2019 02:43, Anson Huang wrote: > > Hi, Daniel > > > >> On 26/06/2019 03:42, Anson Huang wrote: > >>> Hi, Daniel > >>> > >>>> On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > >>>>> From: Anson Huang <Anson.Huang@nxp.com> > >>>>> > >>>>> Systems which use platform driver model for clock driver require > >>>>> the clock frequency to be supplied via device tree when system > >>>>> counter driver is enabled. > >>>>> > >>>>> This is necessary as in the platform driver model the of_clk > >>>>> operations do not work correctly because system counter driver is > >>>>> initialized in early phase of system boot up, and clock driver > >>>>> using platform driver model is NOT ready at that time, it will > >>>>> cause system counter driver initialization failed. > >>>>> > >>>>> Add the optinal clock-frequency to the device tree bindings of the > >>>>> NXP system counter, so the frequency can be handed in and the > >>>>> of_clk operations can be skipped. > >>>> > >>>> Isn't it possible to create a fixed-clock and refer to it? So no > >>>> need to create a specific action before calling timer_of_init() ? > >>>> > >>> > >>> As the clock must be ready before the TIMER_OF_DECLARE, so adding a > >>> CLK_OF_DECLARE_DRIVER in clock driver to ONLY register a fixed-clock? > >>> The system counter's frequency are different on different platforms, > >>> so adding fixed clock in system counter driver is NOT a good idea, > >>> ONLY the DT node or the clock driver can create this fixed clock > >>> according to > >> platforms, can you advise where to create this fixed clock is better? > >> > >> Can you point me to a DT with the "nxp,sysctr-timer" ? > > > > The DT node of system counter is new added in 3/3 of this patch > > series, also can be found from below link: > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc > > > hwork.kernel.org%2Fpatch%2F11011703%2F&data=02%7C01%7Canson. > huang% > > > 40nxp.com%7C8b9519ecceb346712be808d6fad675e4%7C686ea1d3bc2b4c6f > a92cd99 > > > c5c301635%7C0%7C0%7C636972196338405582&sdata=sOQQzDFxoCqe > VuHFuYPHh > > F8Bdj2Zu9WS7Go%2FV9lrWa8%3D&reserved=0 > > Sorry, I was unclear. I meant a patch with the timer defined using a clock as > defined currently in the binding (no clock-frequency). OK, for i.MX8MM, we use clocks, check below patch series: https://patchwork.kernel.org/patch/11008519/ code piece as below: + system_counter: timer@306a0000 { + compatible = "nxp,sysctr-timer"; + reg = <0x306a0000 0x30000>; + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>, + <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; + clocks = <&clk IMX8MM_CLK_SYS_CTR>; + clock-names = "per"; + }; Thanks, Anson.
Hi Anson, On 27/06/2019 10:11, Anson Huang wrote: > Hi, Daniel > >> On 27/06/2019 02:43, Anson Huang wrote: >>> Hi, Daniel >>> >>>> On 26/06/2019 03:42, Anson Huang wrote: >>>>> Hi, Daniel >>>>> >>>>>> On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: >>>>>>> From: Anson Huang <Anson.Huang@nxp.com> >>>>>>> >>>>>>> Systems which use platform driver model for clock driver require >>>>>>> the clock frequency to be supplied via device tree when system >>>>>>> counter driver is enabled. >>>>>>> >>>>>>> This is necessary as in the platform driver model the of_clk >>>>>>> operations do not work correctly because system counter driver is >>>>>>> initialized in early phase of system boot up, and clock driver >>>>>>> using platform driver model is NOT ready at that time, it will >>>>>>> cause system counter driver initialization failed. >>>>>>> >>>>>>> Add the optinal clock-frequency to the device tree bindings of the >>>>>>> NXP system counter, so the frequency can be handed in and the >>>>>>> of_clk operations can be skipped. >>>>>> >>>>>> Isn't it possible to create a fixed-clock and refer to it? So no >>>>>> need to create a specific action before calling timer_of_init() ? >>>>>> >>>>> >>>>> As the clock must be ready before the TIMER_OF_DECLARE, so adding a >>>>> CLK_OF_DECLARE_DRIVER in clock driver to ONLY register a fixed-clock? >>>>> The system counter's frequency are different on different platforms, >>>>> so adding fixed clock in system counter driver is NOT a good idea, >>>>> ONLY the DT node or the clock driver can create this fixed clock >>>>> according to >>>> platforms, can you advise where to create this fixed clock is better? >>>> >>>> Can you point me to a DT with the "nxp,sysctr-timer" ? >>> >>> The DT node of system counter is new added in 3/3 of this patch >>> series, also can be found from below link: >>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc >>> >> hwork.kernel.org%2Fpatch%2F11011703%2F&data=02%7C01%7Canson. >> huang% >>> >> 40nxp.com%7C8b9519ecceb346712be808d6fad675e4%7C686ea1d3bc2b4c6f >> a92cd99 >>> >> c5c301635%7C0%7C0%7C636972196338405582&sdata=sOQQzDFxoCqe >> VuHFuYPHh >>> F8Bdj2Zu9WS7Go%2FV9lrWa8%3D&reserved=0 >> >> Sorry, I was unclear. I meant a patch with the timer defined using a clock as >> defined currently in the binding (no clock-frequency). > > OK, for i.MX8MM, we use clocks, check below patch series: > > https://patchwork.kernel.org/patch/11008519/ > > code piece as below: > > + system_counter: timer@306a0000 { > + compatible = "nxp,sysctr-timer"; > + reg = <0x306a0000 0x30000>; > + interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>, > + <GIC_SPI 48 IRQ_TYPE_LEVEL_HIGH>; > + clocks = <&clk IMX8MM_CLK_SYS_CTR>; > + clock-names = "per"; > + }; Thanks, The fixed-clock can help to keep the code and the DT definition for the timer untouched as the 'clocks' above will refer to it. But that means we describe a fake clock. So it is up to you to decide if you want to stick the clock-frequency or use a fixed-clock.
On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > From: Anson Huang <Anson.Huang@nxp.com> > > Systems which use platform driver model for clock driver require the > clock frequency to be supplied via device tree when system counter > driver is enabled. > > This is necessary as in the platform driver model the of_clk operations > do not work correctly because system counter driver is initialized in > early phase of system boot up, and clock driver using platform driver > model is NOT ready at that time, it will cause system counter driver > initialization failed. > > Add the optinal clock-frequency to the device tree bindings of the NXP > system counter, so the frequency can be handed in and the of_clk > operations can be skipped. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V1: > - improve commit log, no content change. > --- > Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt | 6 ++++++ > 1 file changed, 6 insertions(+) > > diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > index d576599..c9907a0 100644 > --- a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > @@ -14,6 +14,11 @@ Required properties: > - clocks : Specifies the counter clock. > - clock-names: Specifies the clock's name of this module > > +Optional properties: > + > +- clock-frequency : Specifies system counter clock frequency and indicates system > + counter driver to skip clock operations. > + Shouldn't it be required and mutually exclusive with clocks/clock-names? > Example: > > system_counter: timer@306a0000 { > @@ -22,4 +27,5 @@ Example: > clocks = <&clk_8m>; > clock-names = "per"; > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > + clock-frequency = <8333333>; > }; >
On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > From: Anson Huang <Anson.Huang@nxp.com> > > On some i.MX8M platforms, clock driver uses platform driver > model and it is NOT ready during timer initialization phase, > the clock operations will fail and system counter driver will > fail too. As all the i.MX8M platforms' system counter clock > are from OSC which is always enabled, so it is no need to enable > clock for system counter driver, the ONLY thing is to pass > clock frequence to driver. > > To make system counter driver work for upper scenario, add an > option of skipping of_clk operation for system counter driver, > an optional property "clock-frequency" is introduced to pass > the frequency value to system counter driver and indicate driver > to skip of_clk operations. > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > --- > Changes since V1: > - improve commit log, no content change. > --- > drivers/clocksource/timer-imx-sysctr.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/drivers/clocksource/timer-imx-sysctr.c b/drivers/clocksource/timer-imx-sysctr.c > index fd7d680..8ff3d7e 100644 > --- a/drivers/clocksource/timer-imx-sysctr.c > +++ b/drivers/clocksource/timer-imx-sysctr.c > @@ -129,6 +129,14 @@ static void __init sysctr_clockevent_init(void) > static int __init sysctr_timer_init(struct device_node *np) > { > int ret = 0; > + u32 rate; > + > + if (!of_property_read_u32(np, "clock-frequency", > + &rate)) { > + to_sysctr.of_clk.rate = rate; > + to_sysctr.of_clk.period = DIV_ROUND_UP(rate, HZ); > + to_sysctr.flags &= ~TIMER_OF_CLOCK; > + } Please take the opportunity to add the TIMER_OF_CLOCK_FREQUENCY flag in timer-of and the corresponding code above. Then check the clock-frequency presence and add TIMER_OF_CLOCK or TIMER_OF_CLOCK_FREQUENCY flag to the timer-of structure. eg: to_sysctr.flags |= of_find_property(np, "clock-frequency", NULL) ? TIMER_OF_CLOCK_FREQUENCY : TIMER_OF_CLOCK; > ret = timer_of_init(np, &to_sysctr); > if (ret) >
Hi, Daniel > On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > > From: Anson Huang <Anson.Huang@nxp.com> > > > > On some i.MX8M platforms, clock driver uses platform driver model and > > it is NOT ready during timer initialization phase, the clock > > operations will fail and system counter driver will fail too. As all > > the i.MX8M platforms' system counter clock are from OSC which is > > always enabled, so it is no need to enable clock for system counter > > driver, the ONLY thing is to pass clock frequence to driver. > > > > To make system counter driver work for upper scenario, add an option > > of skipping of_clk operation for system counter driver, an optional > > property "clock-frequency" is introduced to pass the frequency value > > to system counter driver and indicate driver to skip of_clk > > operations. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > Changes since V1: > > - improve commit log, no content change. > > --- > > drivers/clocksource/timer-imx-sysctr.c | 8 ++++++++ > > 1 file changed, 8 insertions(+) > > > > diff --git a/drivers/clocksource/timer-imx-sysctr.c > > b/drivers/clocksource/timer-imx-sysctr.c > > index fd7d680..8ff3d7e 100644 > > --- a/drivers/clocksource/timer-imx-sysctr.c > > +++ b/drivers/clocksource/timer-imx-sysctr.c > > @@ -129,6 +129,14 @@ static void __init sysctr_clockevent_init(void) > > static int __init sysctr_timer_init(struct device_node *np) { > > int ret = 0; > > + u32 rate; > > + > > + if (!of_property_read_u32(np, "clock-frequency", > > + &rate)) { > > + to_sysctr.of_clk.rate = rate; > > + to_sysctr.of_clk.period = DIV_ROUND_UP(rate, HZ); > > + to_sysctr.flags &= ~TIMER_OF_CLOCK; > > + } > > Please take the opportunity to add the TIMER_OF_CLOCK_FREQUENCY flag in > timer-of and the corresponding code above. OK, so another patch for timer-of is necessary, if TIMER_OF_CLOCK_FREQUENCY flag is present, just read the "clock-frequency" from DT instead of getting clock rate from clock tree, right? I think this becomes a common case for timer driver, as more and more platforms will use platform driver model for clock driver, it would be good if timer-of can provide solution. > > Then check the clock-frequency presence and add TIMER_OF_CLOCK or > TIMER_OF_CLOCK_FREQUENCY flag to the timer-of structure. > > eg: > > to_sysctr.flags |= of_find_property(np, "clock-frequency", NULL) ? > TIMER_OF_CLOCK_FREQUENCY : TIMER_OF_CLOCK; > OK. Thanks, Anson.
Hi, Daniel > On 23/06/2019 14:38, Anson.Huang@nxp.com wrote: > > From: Anson Huang <Anson.Huang@nxp.com> > > > > Systems which use platform driver model for clock driver require the > > clock frequency to be supplied via device tree when system counter > > driver is enabled. > > > > This is necessary as in the platform driver model the of_clk > > operations do not work correctly because system counter driver is > > initialized in early phase of system boot up, and clock driver using > > platform driver model is NOT ready at that time, it will cause system > > counter driver initialization failed. > > > > Add the optinal clock-frequency to the device tree bindings of the NXP > > system counter, so the frequency can be handed in and the of_clk > > operations can be skipped. > > > > Signed-off-by: Anson Huang <Anson.Huang@nxp.com> > > --- > > Changes since V1: > > - improve commit log, no content change. > > --- > > Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt | 6 > > ++++++ > > 1 file changed, 6 insertions(+) > > > > diff --git > > a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > > b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > > index d576599..c9907a0 100644 > > --- a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > > +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt > > @@ -14,6 +14,11 @@ Required properties: > > - clocks : Specifies the counter clock. > > - clock-names: Specifies the clock's name of this module > > > > +Optional properties: > > + > > +- clock-frequency : Specifies system counter clock frequency and indicates > system > > + counter driver to skip clock operations. > > + > > Shouldn't it be required and mutually exclusive with clocks/clock-names? > Yes, make sense, I ever thought about it when doing this patch, but eventually I picked the optional...will fix it in next version. Thanks, Anson > > Example: > > > > system_counter: timer@306a0000 { > > @@ -22,4 +27,5 @@ Example: > > clocks = <&clk_8m>; > > clock-names = "per"; > > interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; > > + clock-frequency = <8333333>; > > }; > > > >
diff --git a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt index d576599..c9907a0 100644 --- a/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt +++ b/Documentation/devicetree/bindings/timer/nxp,sysctr-timer.txt @@ -14,6 +14,11 @@ Required properties: - clocks : Specifies the counter clock. - clock-names: Specifies the clock's name of this module +Optional properties: + +- clock-frequency : Specifies system counter clock frequency and indicates system + counter driver to skip clock operations. + Example: system_counter: timer@306a0000 { @@ -22,4 +27,5 @@ Example: clocks = <&clk_8m>; clock-names = "per"; interrupts = <GIC_SPI 47 IRQ_TYPE_LEVEL_HIGH>; + clock-frequency = <8333333>; };