| Message ID | 5612649C.5020401@sigmadesigns.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 05/10/2015 13:53, Marc Gonzalez wrote: > In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") > Russell noted that "the TWD local timers are unable to wake up the CPU > when it is placed into a low power mode". > > However, some platforms do not stop the TWD block in low-power mode, > and can thus use the TWD timer in one-shot mode, without setting up > a broadcast device. > > Make the driver check for the "twd-never-stops" boolean property, > and set the CLOCK_EVT_FEAT_C3STOP flag accordingly. > > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > --- > Documentation/devicetree/bindings/arm/twd.txt | 4 ++++ > arch/arm/kernel/smp_twd.c | 5 ++++- > 2 files changed, 8 insertions(+), 1 deletion(-) Sorry, I don't fully understand the submission process. Should I now submit my patch to the patch queue? http://www.arm.linux.org.uk/developer/patches/ Regards.
On 05/10/2015 18:37, Marc Gonzalez wrote: > On 05/10/2015 13:53, Marc Gonzalez wrote: >> In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") >> Russell noted that "the TWD local timers are unable to wake up the CPU >> when it is placed into a low power mode". >> >> However, some platforms do not stop the TWD block in low-power mode, >> and can thus use the TWD timer in one-shot mode, without setting up >> a broadcast device. >> >> Make the driver check for the "twd-never-stops" boolean property, >> and set the CLOCK_EVT_FEAT_C3STOP flag accordingly. >> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> >> --- >> Documentation/devicetree/bindings/arm/twd.txt | 4 ++++ >> arch/arm/kernel/smp_twd.c | 5 ++++- >> 2 files changed, 8 insertions(+), 1 deletion(-) > > Sorry, I don't fully understand the submission process. > > Should I now submit my patch to the patch queue? > http://www.arm.linux.org.uk/developer/patches/ Submitted as patch 8441/1 http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8441/1
On Mon, Oct 05, 2015 at 01:53:00PM +0200, Marc Gonzalez wrote: > In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") > Russell noted that "the TWD local timers are unable to wake up the CPU > when it is placed into a low power mode". > > However, some platforms do not stop the TWD block in low-power mode, > and can thus use the TWD timer in one-shot mode, without setting up > a broadcast device. > > Make the driver check for the "twd-never-stops" boolean property, > and set the CLOCK_EVT_FEAT_C3STOP flag accordingly. > > Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> > --- > Documentation/devicetree/bindings/arm/twd.txt | 4 ++++ > arch/arm/kernel/smp_twd.c | 5 ++++- > 2 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt > index 75b8610939fa..700a517b000e 100644 > --- a/Documentation/devicetree/bindings/arm/twd.txt > +++ b/Documentation/devicetree/bindings/arm/twd.txt > @@ -19,6 +19,10 @@ interrupts. > - reg : Specify the base address and the size of the TWD timer > register window. > > +Optional > +- twd-never-stops : boolean property. If present, TWD timers are expected > + to keep generating interrupts, even when the CPU is in low-power mode. Sorry for the last minute bikeshed, but it would be better if we could align this with the ARM generic timer binding, both in naming and description: - always-on : a boolean property. If present, the timer is powered through an always-on power domain, therefore it never loses context. Otherwise this looks ok. If you can respin with the above wording, and s/twd-never-stops/always-on/ in the patch, you can add: Acked-by: Mark Rutland <mark.rutland@arm.com> Thanks, Mark. > + > Example: > > twd-timer@2c000600 { > diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c > index e8f6d241881f..79298d4fb41f 100644 > --- a/arch/arm/kernel/smp_twd.c > +++ b/arch/arm/kernel/smp_twd.c > @@ -33,6 +33,7 @@ static unsigned long twd_timer_rate; > static DEFINE_PER_CPU(bool, percpu_setup_called); > > static struct clock_event_device __percpu *twd_evt; > +static int feat_c3stop; > static int twd_ppi; > > static void twd_set_mode(enum clock_event_mode mode, > @@ -293,7 +294,7 @@ static void twd_timer_setup(void) > > clk->name = "local_timer"; > clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > - CLOCK_EVT_FEAT_C3STOP; > + feat_c3stop; > clk->rating = 350; > clk->set_mode = twd_set_mode; > clk->set_next_event = twd_set_next_event; > @@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np) > goto out_irq; > > twd_get_clock(np); > + if (!of_property_read_bool(np, "twd-never-stops")) > + feat_c3stop = CLOCK_EVT_FEAT_C3STOP; > > /* > * Immediately configure the timer on the boot CPU, unless we need > -- > 2.4.5 >
On 08/10/2015 19:16, Mark Rutland wrote: > On Mon, Oct 05, 2015 at 01:53:00PM +0200, Marc Gonzalez wrote: >> In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") >> Russell noted that "the TWD local timers are unable to wake up the CPU >> when it is placed into a low power mode". >> >> However, some platforms do not stop the TWD block in low-power mode, >> and can thus use the TWD timer in one-shot mode, without setting up >> a broadcast device. >> >> Make the driver check for the "twd-never-stops" boolean property, >> and set the CLOCK_EVT_FEAT_C3STOP flag accordingly. >> >> Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> >> --- >> Documentation/devicetree/bindings/arm/twd.txt | 4 ++++ >> arch/arm/kernel/smp_twd.c | 5 ++++- >> 2 files changed, 8 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt >> index 75b8610939fa..700a517b000e 100644 >> --- a/Documentation/devicetree/bindings/arm/twd.txt >> +++ b/Documentation/devicetree/bindings/arm/twd.txt >> @@ -19,6 +19,10 @@ interrupts. >> - reg : Specify the base address and the size of the TWD timer >> register window. >> >> +Optional >> +- twd-never-stops : boolean property. If present, TWD timers are expected >> + to keep generating interrupts, even when the CPU is in low-power mode. > > Sorry for the last minute bikeshed, but it would be better if we could > align this with the ARM generic timer binding, both in naming and > description: > > - always-on : a boolean property. If present, the timer is powered through an > always-on power domain, therefore it never loses context. No problemo. Too bad I didn't think to copy that description in the first place! > Otherwise this looks ok. If you can respin with the above wording, and > s/twd-never-stops/always-on/ in the patch, you can add: > > Acked-by: Mark Rutland <mark.rutland@arm.com> Wouldn't you feel like going all-in and Signing-off? ;-) One last nit (see below). >> diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c >> index e8f6d241881f..79298d4fb41f 100644 >> --- a/arch/arm/kernel/smp_twd.c >> +++ b/arch/arm/kernel/smp_twd.c >> @@ -33,6 +33,7 @@ static unsigned long twd_timer_rate; >> static DEFINE_PER_CPU(bool, percpu_setup_called); >> >> static struct clock_event_device __percpu *twd_evt; >> +static int feat_c3stop; >> static int twd_ppi; >> >> static void twd_set_mode(enum clock_event_mode mode, >> @@ -293,7 +294,7 @@ static void twd_timer_setup(void) >> >> clk->name = "local_timer"; >> clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | >> - CLOCK_EVT_FEAT_C3STOP; >> + feat_c3stop; >> clk->rating = 350; >> clk->set_mode = twd_set_mode; >> clk->set_next_event = twd_set_next_event; >> @@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np) >> goto out_irq; >> >> twd_get_clock(np); >> + if (!of_property_read_bool(np, "twd-never-stops")) >> + feat_c3stop = CLOCK_EVT_FEAT_C3STOP; Is it possible that twd_local_timer_common_register() would be called more than once? twd_local_timer_register() guards against multiple invocations, but twd_local_timer_of_register() doesn't (but maybe there is some guarantee from OF, or it's invalid to have multiple "arm,cortex-a9-twd-timer" strings in the DT). Regards.
On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote: > On 08/10/2015 19:16, Mark Rutland wrote: > > Otherwise this looks ok. If you can respin with the above wording, and > > s/twd-never-stops/always-on/ in the patch, you can add: > > > > Acked-by: Mark Rutland <mark.rutland@arm.com> > > Wouldn't you feel like going all-in and Signing-off? ;-) Then I'd reject it, because it would be wrong. Documentation/SubmittingPatches: 12) When to use Acked-by: and Cc: --------------------------------- The Signed-off-by: tag indicates that the signer was involved in the development of the patch, or that he/she was in the patch's delivery path. If a person was not directly involved in the preparation or handling of a patch but wishes to signify and record their approval of it then they can ask to have an Acked-by: line added to the patch's changelog. Acked-by: is often used by the maintainer of the affected code when that maintainer neither contributed to nor forwarded the patch. Acked-by: is not as formal as Signed-off-by:. It is a record that the acker has at least reviewed the patch and has indicated acceptance. Hence patch mergers will sometimes manually convert an acker's "yep, looks good to me" into an Acked-by: (but note that it is usually better to ask for an explicit ack). Acked-by: does not necessarily indicate acknowledgement of the entire patch. For example, if a patch affects multiple subsystems and has an Acked-by: from one subsystem maintainer then this usually indicates acknowledgement of just the part which affects that maintainer's code. Judgement should be used here. When in doubt people should refer to the original discussion in the mailing list archives.
> Is it possible that twd_local_timer_common_register() would be > called more than once? > > twd_local_timer_register() guards against multiple invocations, > but twd_local_timer_of_register() doesn't (but maybe there is > some guarantee from OF, or it's invalid to have multiple > "arm,cortex-a9-twd-timer" strings in the DT). It does appear that were you to have have two or more "arm,cortex-a9-twd-timer" nodes in the DT, things would go wrong. As far as I am aware, that would result in multiple calls to twd_local_timer_of_register. I don't think it would ever be valid to have two nodes for this particular device, but it's probably worth having the same check as twd_local_timer_register does, with a warning that there was more than one such node logged to the console. Thanks, Mark
On 08/10/2015 19:43, Russell King - ARM Linux wrote: > On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote: >> On 08/10/2015 19:16, Mark Rutland wrote: >>> Otherwise this looks ok. If you can respin with the above wording, and >>> s/twd-never-stops/always-on/ in the patch, you can add: >>> >>> Acked-by: Mark Rutland <mark.rutland@arm.com> >> >> Wouldn't you feel like going all-in and Signing-off? ;-) > > Then I'd reject it, because it would be wrong. > > Documentation/SubmittingPatches: [snip] Could someone explain the difference between Acked-by and Reviewed-by? Regards.
On Thu, Oct 08, 2015 at 08:16:24PM +0200, Mason wrote: > On 08/10/2015 19:43, Russell King - ARM Linux wrote: > > On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote: > >> On 08/10/2015 19:16, Mark Rutland wrote: > >>> Otherwise this looks ok. If you can respin with the above wording, and > >>> s/twd-never-stops/always-on/ in the patch, you can add: > >>> > >>> Acked-by: Mark Rutland <mark.rutland@arm.com> > >> > >> Wouldn't you feel like going all-in and Signing-off? ;-) > > > > Then I'd reject it, because it would be wrong. > > > > Documentation/SubmittingPatches: [snip] > > Could someone explain the difference between Acked-by and > Reviewed-by? Again, I could quote from the above file, but you already have a copy of it locally, so you could just read that version instead. Section 13.
On 08/10/2015 19:57, Mark Rutland wrote: >> Is it possible that twd_local_timer_common_register() would be >> called more than once? >> >> twd_local_timer_register() guards against multiple invocations, >> but twd_local_timer_of_register() doesn't (but maybe there is >> some guarantee from OF, or it's invalid to have multiple >> "arm,cortex-a9-twd-timer" strings in the DT). > > It does appear that were you to have have two or more > "arm,cortex-a9-twd-timer" nodes in the DT, things would go wrong. As far > as I am aware, that would result in multiple calls to > twd_local_timer_of_register. > > I don't think it would ever be valid to have two nodes for this > particular device, but it's probably worth having the same check as > twd_local_timer_register does, with a warning that there was more than > one such node logged to the console. Hmmm, I think I'll do it in a follow-up patch, once Felipe's revert hits mainline. Regards.
On Thu, Oct 08, 2015 at 08:25:07PM +0200, Mason wrote: > On 08/10/2015 19:57, Mark Rutland wrote: > >> Is it possible that twd_local_timer_common_register() would be > >> called more than once? > >> > >> twd_local_timer_register() guards against multiple invocations, > >> but twd_local_timer_of_register() doesn't (but maybe there is > >> some guarantee from OF, or it's invalid to have multiple > >> "arm,cortex-a9-twd-timer" strings in the DT). > > > > It does appear that were you to have have two or more > > "arm,cortex-a9-twd-timer" nodes in the DT, things would go wrong. As far > > as I am aware, that would result in multiple calls to > > twd_local_timer_of_register. > > > > I don't think it would ever be valid to have two nodes for this > > particular device, but it's probably worth having the same check as > > twd_local_timer_register does, with a warning that there was more than > > one such node logged to the console. > > Hmmm, I think I'll do it in a follow-up patch, once Felipe's > revert hits mainline. Sure, this is certainly independent of the CLOCK_EVT_FEAT_C3STOP patch. Thanks, Mark.
On 08/10/2015 20:22, Russell King - ARM Linux wrote: > On Thu, Oct 08, 2015 at 08:16:24PM +0200, Mason wrote: >> On 08/10/2015 19:43, Russell King - ARM Linux wrote: >>> On Thu, Oct 08, 2015 at 07:38:53PM +0200, Mason wrote: >>>> On 08/10/2015 19:16, Mark Rutland wrote: >>>>> Otherwise this looks ok. If you can respin with the above wording, and >>>>> s/twd-never-stops/always-on/ in the patch, you can add: >>>>> >>>>> Acked-by: Mark Rutland <mark.rutland@arm.com> >>>> >>>> Wouldn't you feel like going all-in and Signing-off? ;-) >>> >>> Then I'd reject it, because it would be wrong. >>> >>> Documentation/SubmittingPatches: [snip] >> >> Could someone explain the difference between Acked-by and >> Reviewed-by? > > Again, I could quote from the above file, but you already have a copy > of it locally, so you could just read that version instead. Section 13. Acked-by: [...] is a record that the acker has at least reviewed the patch and has indicated acceptance. Reviewed-by: indicates that the patch has been reviewed and found acceptable according to the Reviewer's Statement IIUC Acked-by is stronger than Reviewed-by? Regards.
On 08/10/2015 19:16, Mark Rutland wrote: > Sorry for the last minute bikeshed, but it would be better if we could > align this with the ARM generic timer binding, both in naming and > description: Updated patch has been sent to Russell's patch mill. http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8441/2 Regards.
diff --git a/Documentation/devicetree/bindings/arm/twd.txt b/Documentation/devicetree/bindings/arm/twd.txt index 75b8610939fa..700a517b000e 100644 --- a/Documentation/devicetree/bindings/arm/twd.txt +++ b/Documentation/devicetree/bindings/arm/twd.txt @@ -19,6 +19,10 @@ interrupts. - reg : Specify the base address and the size of the TWD timer register window. +Optional +- twd-never-stops : boolean property. If present, TWD timers are expected + to keep generating interrupts, even when the CPU is in low-power mode. + Example: twd-timer@2c000600 { diff --git a/arch/arm/kernel/smp_twd.c b/arch/arm/kernel/smp_twd.c index e8f6d241881f..79298d4fb41f 100644 --- a/arch/arm/kernel/smp_twd.c +++ b/arch/arm/kernel/smp_twd.c @@ -33,6 +33,7 @@ static unsigned long twd_timer_rate; static DEFINE_PER_CPU(bool, percpu_setup_called); static struct clock_event_device __percpu *twd_evt; +static int feat_c3stop; static int twd_ppi; static void twd_set_mode(enum clock_event_mode mode, @@ -293,7 +294,7 @@ static void twd_timer_setup(void) clk->name = "local_timer"; clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | - CLOCK_EVT_FEAT_C3STOP; + feat_c3stop; clk->rating = 350; clk->set_mode = twd_set_mode; clk->set_next_event = twd_set_next_event; @@ -345,6 +346,8 @@ static int __init twd_local_timer_common_register(struct device_node *np) goto out_irq; twd_get_clock(np); + if (!of_property_read_bool(np, "twd-never-stops")) + feat_c3stop = CLOCK_EVT_FEAT_C3STOP; /* * Immediately configure the timer on the boot CPU, unless we need
In 5388a6b266 ("ARM: SMP: Always enable clock event broadcast support") Russell noted that "the TWD local timers are unable to wake up the CPU when it is placed into a low power mode". However, some platforms do not stop the TWD block in low-power mode, and can thus use the TWD timer in one-shot mode, without setting up a broadcast device. Make the driver check for the "twd-never-stops" boolean property, and set the CLOCK_EVT_FEAT_C3STOP flag accordingly. Signed-off-by: Marc Gonzalez <marc_gonzalez@sigmadesigns.com> --- Documentation/devicetree/bindings/arm/twd.txt | 4 ++++ arch/arm/kernel/smp_twd.c | 5 ++++- 2 files changed, 8 insertions(+), 1 deletion(-)