* [PATCH 0/2] arm: zynq: Enable global timer @ 2013-09-12 16:50 ` Soren Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: devicetree, linux-kernel, linux-arm-kernel, Soren Brinkmann Hi all, I'm sitting on this for a while and was waiting for Stephen's patch to get merged somewhere. Unfortunately that didn't happen yet. The patch to enable the global timer is pretty straight forward. Stephen's patch is the result of this thread: https://lkml.org/lkml/2013/7/22/649 and required to prevent the global_timer from becoming the broadcast device, since the system will hang otherwise. Soren Brinkmann (1): arm: zynq: Enable arm_global_timer Stephen Boyd (1): tick: broadcast: Deny per-cpu clockevents from being broadcast sources arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ arch/arm/mach-zynq/Kconfig | 1 + kernel/time/tick-broadcast.c | 3 +++ 3 files changed, 12 insertions(+) -- 1.8.4 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/2] arm: zynq: Enable global timer @ 2013-09-12 16:50 ` Soren Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: linux-arm-kernel Hi all, I'm sitting on this for a while and was waiting for Stephen's patch to get merged somewhere. Unfortunately that didn't happen yet. The patch to enable the global timer is pretty straight forward. Stephen's patch is the result of this thread: https://lkml.org/lkml/2013/7/22/649 and required to prevent the global_timer from becoming the broadcast device, since the system will hang otherwise. Soren Brinkmann (1): arm: zynq: Enable arm_global_timer Stephen Boyd (1): tick: broadcast: Deny per-cpu clockevents from being broadcast sources arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ arch/arm/mach-zynq/Kconfig | 1 + kernel/time/tick-broadcast.c | 3 +++ 3 files changed, 12 insertions(+) -- 1.8.4 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 0/2] arm: zynq: Enable global timer @ 2013-09-12 16:50 ` Soren Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: Soren Brinkmann, devicetree, linux-kernel, linux-arm-kernel Hi all, I'm sitting on this for a while and was waiting for Stephen's patch to get merged somewhere. Unfortunately that didn't happen yet. The patch to enable the global timer is pretty straight forward. Stephen's patch is the result of this thread: https://lkml.org/lkml/2013/7/22/649 and required to prevent the global_timer from becoming the broadcast device, since the system will hang otherwise. Soren Brinkmann (1): arm: zynq: Enable arm_global_timer Stephen Boyd (1): tick: broadcast: Deny per-cpu clockevents from being broadcast sources arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ arch/arm/mach-zynq/Kconfig | 1 + kernel/time/tick-broadcast.c | 3 +++ 3 files changed, 12 insertions(+) -- 1.8.4 ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources 2013-09-12 16:50 ` Soren Brinkmann @ 2013-09-12 16:50 ` Soren Brinkmann -1 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: devicetree, linux-kernel, linux-arm-kernel From: Stephen Boyd <sboyd@codeaurora.org> On most ARM systems the per-cpu clockevents are truly per-cpu in the sense that they can't be controlled on any other CPU besides the CPU that they interrupt. If one of these clockevents were to become a broadcast source we will run into a lot of trouble because the broadcast source is enabled on the first CPU to go into deep idle (if that CPU suffers from FEAT_C3_STOP) and that could be a different CPU than what the clockevent is interrupting (or even worse the CPU that the clockevent interrupts could be offline). Theoretically it's possible to support per-cpu clockevents as the broadcast source but so far we haven't needed this and supporting it is rather complicated. Let's just deny the possibility for now until this becomes a reality (let's hope it never does!). Reported-by: Sören Brinkmann <soren.brinkmann@xilinx.com> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- kernel/time/tick-broadcast.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 218bcb5..d3539e5 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) + return false; + return !curdev || newdev->rating > curdev->rating; } -- 1.8.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-12 16:50 ` Soren Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: linux-arm-kernel From: Stephen Boyd <sboyd@codeaurora.org> On most ARM systems the per-cpu clockevents are truly per-cpu in the sense that they can't be controlled on any other CPU besides the CPU that they interrupt. If one of these clockevents were to become a broadcast source we will run into a lot of trouble because the broadcast source is enabled on the first CPU to go into deep idle (if that CPU suffers from FEAT_C3_STOP) and that could be a different CPU than what the clockevent is interrupting (or even worse the CPU that the clockevent interrupts could be offline). Theoretically it's possible to support per-cpu clockevents as the broadcast source but so far we haven't needed this and supporting it is rather complicated. Let's just deny the possibility for now until this becomes a reality (let's hope it never does!). Reported-by: S?ren Brinkmann <soren.brinkmann@xilinx.com> Signed-off-by: Stephen Boyd <sboyd@codeaurora.org> --- kernel/time/tick-broadcast.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index 218bcb5..d3539e5 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -77,6 +77,9 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; + if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) + return false; + return !curdev || newdev->rating > curdev->rating; } -- 1.8.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources 2013-09-12 16:50 ` Soren Brinkmann @ 2013-09-12 20:30 ` Thomas Gleixner -1 siblings, 0 replies; 36+ messages in thread From: Thomas Gleixner @ 2013-09-12 20:30 UTC (permalink / raw) To: Soren Brinkmann Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Daniel Lezcano, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel On Thu, 12 Sep 2013, Soren Brinkmann wrote: > From: Stephen Boyd <sboyd@codeaurora.org> > > On most ARM systems the per-cpu clockevents are truly per-cpu in > the sense that they can't be controlled on any other CPU besides > the CPU that they interrupt. If one of these clockevents were to > become a broadcast source we will run into a lot of trouble > because the broadcast source is enabled on the first CPU to go > into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > could be a different CPU than what the clockevent is interrupting > (or even worse the CPU that the clockevent interrupts could be > offline). > > Theoretically it's possible to support per-cpu clockevents as the > broadcast source but so far we haven't needed this and supporting > it is rather complicated. Let's just deny the possibility for now > until this becomes a reality (let's hope it never does!). Well, we can't do it this way. There are globally accessible clock event devices which deliver only to cpu0. So the mask check might be causing failure here. Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event device and check for it. Thanks, tglx ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-12 20:30 ` Thomas Gleixner 0 siblings, 0 replies; 36+ messages in thread From: Thomas Gleixner @ 2013-09-12 20:30 UTC (permalink / raw) To: linux-arm-kernel On Thu, 12 Sep 2013, Soren Brinkmann wrote: > From: Stephen Boyd <sboyd@codeaurora.org> > > On most ARM systems the per-cpu clockevents are truly per-cpu in > the sense that they can't be controlled on any other CPU besides > the CPU that they interrupt. If one of these clockevents were to > become a broadcast source we will run into a lot of trouble > because the broadcast source is enabled on the first CPU to go > into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > could be a different CPU than what the clockevent is interrupting > (or even worse the CPU that the clockevent interrupts could be > offline). > > Theoretically it's possible to support per-cpu clockevents as the > broadcast source but so far we haven't needed this and supporting > it is rather complicated. Let's just deny the possibility for now > until this becomes a reality (let's hope it never does!). Well, we can't do it this way. There are globally accessible clock event devices which deliver only to cpu0. So the mask check might be causing failure here. Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event device and check for it. Thanks, tglx ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources 2013-09-12 20:30 ` Thomas Gleixner (?) @ 2013-09-12 23:48 ` Sören Brinkmann -1 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-12 23:48 UTC (permalink / raw) To: Thomas Gleixner Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Daniel Lezcano, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel Hi Thomas, On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: > > From: Stephen Boyd <sboyd@codeaurora.org> > > > > On most ARM systems the per-cpu clockevents are truly per-cpu in > > the sense that they can't be controlled on any other CPU besides > > the CPU that they interrupt. If one of these clockevents were to > > become a broadcast source we will run into a lot of trouble > > because the broadcast source is enabled on the first CPU to go > > into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > > could be a different CPU than what the clockevent is interrupting > > (or even worse the CPU that the clockevent interrupts could be > > offline). > > > > Theoretically it's possible to support per-cpu clockevents as the > > broadcast source but so far we haven't needed this and supporting > > it is rather complicated. Let's just deny the possibility for now > > until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. I gave it a shot. Is this what you imagine: diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index b66c1f3..c639b1a 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) int cpu = smp_processor_id(); clk->name = "arm_global_timer"; - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | + CLOCK_EVT_FEAT_PERCPU; clk->set_mode = gt_clockevent_set_mode; clk->set_next_event = gt_clockevent_set_next_event; clk->cpumask = cpumask_of(cpu); diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 0857922..493aa02 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -60,6 +60,7 @@ enum clock_event_mode { * Core shall set the interrupt affinity dynamically in broadcast mode */ #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 +#define CLOCK_EVT_FEAT_PERCPU 0x000040 /** * struct clock_event_device - clock event device descriptor diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index d3539e5..de4c5d8 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, struct clock_event_device *newdev) { if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) return false; if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) - return false; - return !curdev || newdev->rating > curdev->rating; } If this is the way to go, I can prepare this in a v2. Thanks, Sören ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-12 23:48 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-12 23:48 UTC (permalink / raw) To: linux-arm-kernel Hi Thomas, On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: > > From: Stephen Boyd <sboyd@codeaurora.org> > > > > On most ARM systems the per-cpu clockevents are truly per-cpu in > > the sense that they can't be controlled on any other CPU besides > > the CPU that they interrupt. If one of these clockevents were to > > become a broadcast source we will run into a lot of trouble > > because the broadcast source is enabled on the first CPU to go > > into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > > could be a different CPU than what the clockevent is interrupting > > (or even worse the CPU that the clockevent interrupts could be > > offline). > > > > Theoretically it's possible to support per-cpu clockevents as the > > broadcast source but so far we haven't needed this and supporting > > it is rather complicated. Let's just deny the possibility for now > > until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. I gave it a shot. Is this what you imagine: diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index b66c1f3..c639b1a 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) int cpu = smp_processor_id(); clk->name = "arm_global_timer"; - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | + CLOCK_EVT_FEAT_PERCPU; clk->set_mode = gt_clockevent_set_mode; clk->set_next_event = gt_clockevent_set_next_event; clk->cpumask = cpumask_of(cpu); diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 0857922..493aa02 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -60,6 +60,7 @@ enum clock_event_mode { * Core shall set the interrupt affinity dynamically in broadcast mode */ #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 +#define CLOCK_EVT_FEAT_PERCPU 0x000040 /** * struct clock_event_device - clock event device descriptor diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index d3539e5..de4c5d8 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, struct clock_event_device *newdev) { if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) return false; if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) - return false; - return !curdev || newdev->rating > curdev->rating; } If this is the way to go, I can prepare this in a v2. Thanks, S?ren ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-12 23:48 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-12 23:48 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Rutland, devicetree, Daniel Lezcano, Russell King, Pawel Moll, Ian Campbell, Stephen Warren, Stephen Boyd, Michal Simek, Rob Herring, linux-kernel, linux-arm-kernel Hi Thomas, On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: > > From: Stephen Boyd <sboyd@codeaurora.org> > > > > On most ARM systems the per-cpu clockevents are truly per-cpu in > > the sense that they can't be controlled on any other CPU besides > > the CPU that they interrupt. If one of these clockevents were to > > become a broadcast source we will run into a lot of trouble > > because the broadcast source is enabled on the first CPU to go > > into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > > could be a different CPU than what the clockevent is interrupting > > (or even worse the CPU that the clockevent interrupts could be > > offline). > > > > Theoretically it's possible to support per-cpu clockevents as the > > broadcast source but so far we haven't needed this and supporting > > it is rather complicated. Let's just deny the possibility for now > > until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. I gave it a shot. Is this what you imagine: diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c index b66c1f3..c639b1a 100644 --- a/drivers/clocksource/arm_global_timer.c +++ b/drivers/clocksource/arm_global_timer.c @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) int cpu = smp_processor_id(); clk->name = "arm_global_timer"; - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | + CLOCK_EVT_FEAT_PERCPU; clk->set_mode = gt_clockevent_set_mode; clk->set_next_event = gt_clockevent_set_next_event; clk->cpumask = cpumask_of(cpu); diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h index 0857922..493aa02 100644 --- a/include/linux/clockchips.h +++ b/include/linux/clockchips.h @@ -60,6 +60,7 @@ enum clock_event_mode { * Core shall set the interrupt affinity dynamically in broadcast mode */ #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 +#define CLOCK_EVT_FEAT_PERCPU 0x000040 /** * struct clock_event_device - clock event device descriptor diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c index d3539e5..de4c5d8 100644 --- a/kernel/time/tick-broadcast.c +++ b/kernel/time/tick-broadcast.c @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, struct clock_event_device *newdev) { if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) return false; if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) return false; - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) - return false; - return !curdev || newdev->rating > curdev->rating; } If this is the way to go, I can prepare this in a v2. Thanks, Sören _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 14:45 ` Thomas Gleixner 0 siblings, 0 replies; 36+ messages in thread From: Thomas Gleixner @ 2013-09-13 14:45 UTC (permalink / raw) To: Sören Brinkmann Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Daniel Lezcano, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel [-- Attachment #1: Type: TEXT/PLAIN, Size: 2531 bytes --] On Thu, 12 Sep 2013, Sören Brinkmann wrote: > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > I gave it a shot. Is this what you imagine: > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index b66c1f3..c639b1a 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > int cpu = smp_processor_id(); > > clk->name = "arm_global_timer"; > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > + CLOCK_EVT_FEAT_PERCPU; > clk->set_mode = gt_clockevent_set_mode; > clk->set_next_event = gt_clockevent_set_next_event; > clk->cpumask = cpumask_of(cpu); > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 0857922..493aa02 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -60,6 +60,7 @@ enum clock_event_mode { > * Core shall set the interrupt affinity dynamically in broadcast mode > */ > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > /** > * struct clock_event_device - clock event device descriptor > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index d3539e5..de4c5d8 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > struct clock_event_device *newdev) > { > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > return false; > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > return false; > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > - return false; > - > return !curdev || newdev->rating > curdev->rating; > } > > If this is the way to go, I can prepare this in a v2. Looks good to me. The last junk of the patch won't apply on mainline, but thats the least of my worries. :) Thanks, tglx ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 14:45 ` Thomas Gleixner 0 siblings, 0 replies; 36+ messages in thread From: Thomas Gleixner @ 2013-09-13 14:45 UTC (permalink / raw) To: linux-arm-kernel On Thu, 12 Sep 2013, S?ren Brinkmann wrote: > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > I gave it a shot. Is this what you imagine: > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index b66c1f3..c639b1a 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > int cpu = smp_processor_id(); > > clk->name = "arm_global_timer"; > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > + CLOCK_EVT_FEAT_PERCPU; > clk->set_mode = gt_clockevent_set_mode; > clk->set_next_event = gt_clockevent_set_next_event; > clk->cpumask = cpumask_of(cpu); > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 0857922..493aa02 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -60,6 +60,7 @@ enum clock_event_mode { > * Core shall set the interrupt affinity dynamically in broadcast mode > */ > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > /** > * struct clock_event_device - clock event device descriptor > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index d3539e5..de4c5d8 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > struct clock_event_device *newdev) > { > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > return false; > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > return false; > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > - return false; > - > return !curdev || newdev->rating > curdev->rating; > } > > If this is the way to go, I can prepare this in a v2. Looks good to me. The last junk of the patch won't apply on mainline, but thats the least of my worries. :) Thanks, tglx ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 14:45 ` Thomas Gleixner 0 siblings, 0 replies; 36+ messages in thread From: Thomas Gleixner @ 2013-09-13 14:45 UTC (permalink / raw) To: Sören Brinkmann Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Daniel Lezcano, Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r [-- Attachment #1: Type: TEXT/PLAIN, Size: 2531 bytes --] On Thu, 12 Sep 2013, Sören Brinkmann wrote: > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > I gave it a shot. Is this what you imagine: > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > index b66c1f3..c639b1a 100644 > --- a/drivers/clocksource/arm_global_timer.c > +++ b/drivers/clocksource/arm_global_timer.c > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > int cpu = smp_processor_id(); > > clk->name = "arm_global_timer"; > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > + CLOCK_EVT_FEAT_PERCPU; > clk->set_mode = gt_clockevent_set_mode; > clk->set_next_event = gt_clockevent_set_next_event; > clk->cpumask = cpumask_of(cpu); > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > index 0857922..493aa02 100644 > --- a/include/linux/clockchips.h > +++ b/include/linux/clockchips.h > @@ -60,6 +60,7 @@ enum clock_event_mode { > * Core shall set the interrupt affinity dynamically in broadcast mode > */ > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > /** > * struct clock_event_device - clock event device descriptor > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > index d3539e5..de4c5d8 100644 > --- a/kernel/time/tick-broadcast.c > +++ b/kernel/time/tick-broadcast.c > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > struct clock_event_device *newdev) > { > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > return false; > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > return false; > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > - return false; > - > return !curdev || newdev->rating > curdev->rating; > } > > If this is the way to go, I can prepare this in a v2. Looks good to me. The last junk of the patch won't apply on mainline, but thats the least of my worries. :) Thanks, tglx ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources 2013-09-13 14:45 ` Thomas Gleixner (?) @ 2013-09-13 15:25 ` Sören Brinkmann -1 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 15:25 UTC (permalink / raw) To: Thomas Gleixner Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Daniel Lezcano, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel On Fri, Sep 13, 2013 at 04:45:12PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Sören Brinkmann wrote: > > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > > I gave it a shot. Is this what you imagine: > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > > index b66c1f3..c639b1a 100644 > > --- a/drivers/clocksource/arm_global_timer.c > > +++ b/drivers/clocksource/arm_global_timer.c > > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > > int cpu = smp_processor_id(); > > > > clk->name = "arm_global_timer"; > > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > > + CLOCK_EVT_FEAT_PERCPU; > > clk->set_mode = gt_clockevent_set_mode; > > clk->set_next_event = gt_clockevent_set_next_event; > > clk->cpumask = cpumask_of(cpu); > > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > > index 0857922..493aa02 100644 > > --- a/include/linux/clockchips.h > > +++ b/include/linux/clockchips.h > > @@ -60,6 +60,7 @@ enum clock_event_mode { > > * Core shall set the interrupt affinity dynamically in broadcast mode > > */ > > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > > > /** > > * struct clock_event_device - clock event device descriptor > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > > index d3539e5..de4c5d8 100644 > > --- a/kernel/time/tick-broadcast.c > > +++ b/kernel/time/tick-broadcast.c > > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > > struct clock_event_device *newdev) > > { > > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > > return false; > > > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > > return false; > > > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > > - return false; > > - > > return !curdev || newdev->rating > curdev->rating; > > } > > > > If this is the way to go, I can prepare this in a v2. > > Looks good to me. The last junk of the patch won't apply on mainline, > but thats the least of my worries. :) Of course. I'll split this into smaller chunks and send out a v2. Thanks, Sören ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 15:25 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 15:25 UTC (permalink / raw) To: linux-arm-kernel On Fri, Sep 13, 2013 at 04:45:12PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, S?ren Brinkmann wrote: > > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > > I gave it a shot. Is this what you imagine: > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > > index b66c1f3..c639b1a 100644 > > --- a/drivers/clocksource/arm_global_timer.c > > +++ b/drivers/clocksource/arm_global_timer.c > > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > > int cpu = smp_processor_id(); > > > > clk->name = "arm_global_timer"; > > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > > + CLOCK_EVT_FEAT_PERCPU; > > clk->set_mode = gt_clockevent_set_mode; > > clk->set_next_event = gt_clockevent_set_next_event; > > clk->cpumask = cpumask_of(cpu); > > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > > index 0857922..493aa02 100644 > > --- a/include/linux/clockchips.h > > +++ b/include/linux/clockchips.h > > @@ -60,6 +60,7 @@ enum clock_event_mode { > > * Core shall set the interrupt affinity dynamically in broadcast mode > > */ > > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > > > /** > > * struct clock_event_device - clock event device descriptor > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > > index d3539e5..de4c5d8 100644 > > --- a/kernel/time/tick-broadcast.c > > +++ b/kernel/time/tick-broadcast.c > > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > > struct clock_event_device *newdev) > > { > > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > > return false; > > > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > > return false; > > > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > > - return false; > > - > > return !curdev || newdev->rating > curdev->rating; > > } > > > > If this is the way to go, I can prepare this in a v2. > > Looks good to me. The last junk of the patch won't apply on mainline, > but thats the least of my worries. :) Of course. I'll split this into smaller chunks and send out a v2. Thanks, S?ren ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 15:25 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 15:25 UTC (permalink / raw) To: Thomas Gleixner Cc: Mark Rutland, devicetree, Daniel Lezcano, Russell King, Pawel Moll, Ian Campbell, Stephen Warren, Stephen Boyd, Michal Simek, Rob Herring, linux-kernel, linux-arm-kernel On Fri, Sep 13, 2013 at 04:45:12PM +0200, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Sören Brinkmann wrote: > > On Thu, Sep 12, 2013 at 10:30:15PM +0200, Thomas Gleixner wrote: > > I gave it a shot. Is this what you imagine: > > diff --git a/drivers/clocksource/arm_global_timer.c b/drivers/clocksource/arm_global_timer.c > > index b66c1f3..c639b1a 100644 > > --- a/drivers/clocksource/arm_global_timer.c > > +++ b/drivers/clocksource/arm_global_timer.c > > @@ -169,7 +169,8 @@ static int gt_clockevents_init(struct clock_event_device *clk) > > int cpu = smp_processor_id(); > > > > clk->name = "arm_global_timer"; > > - clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > > + clk->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT | > > + CLOCK_EVT_FEAT_PERCPU; > > clk->set_mode = gt_clockevent_set_mode; > > clk->set_next_event = gt_clockevent_set_next_event; > > clk->cpumask = cpumask_of(cpu); > > diff --git a/include/linux/clockchips.h b/include/linux/clockchips.h > > index 0857922..493aa02 100644 > > --- a/include/linux/clockchips.h > > +++ b/include/linux/clockchips.h > > @@ -60,6 +60,7 @@ enum clock_event_mode { > > * Core shall set the interrupt affinity dynamically in broadcast mode > > */ > > #define CLOCK_EVT_FEAT_DYNIRQ 0x000020 > > +#define CLOCK_EVT_FEAT_PERCPU 0x000040 > > > > /** > > * struct clock_event_device - clock event device descriptor > > diff --git a/kernel/time/tick-broadcast.c b/kernel/time/tick-broadcast.c > > index d3539e5..de4c5d8 100644 > > --- a/kernel/time/tick-broadcast.c > > +++ b/kernel/time/tick-broadcast.c > > @@ -70,16 +70,14 @@ static bool tick_check_broadcast_device(struct clock_event_device *curdev, > > struct clock_event_device *newdev) > > { > > if ((newdev->features & CLOCK_EVT_FEAT_DUMMY) || > > - (newdev->features & CLOCK_EVT_FEAT_C3STOP)) > > + (newdev->features & CLOCK_EVT_FEAT_C3STOP) || > > + (newdev->features & CLOCK_EVT_FEAT_PERCPU)) > > return false; > > > > if (tick_broadcast_device.mode == TICKDEV_MODE_ONESHOT && > > !(newdev->features & CLOCK_EVT_FEAT_ONESHOT)) > > return false; > > > > - if (cpumask_equal(newdev->cpumask, cpumask_of(smp_processor_id()))) > > - return false; > > - > > return !curdev || newdev->rating > curdev->rating; > > } > > > > If this is the way to go, I can prepare this in a v2. > > Looks good to me. The last junk of the patch won't apply on mainline, > but thats the least of my worries. :) Of course. I'll split this into smaller chunks and send out a v2. Thanks, Sören _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 8:25 ` Daniel Lezcano 0 siblings, 0 replies; 36+ messages in thread From: Daniel Lezcano @ 2013-09-13 8:25 UTC (permalink / raw) To: Thomas Gleixner Cc: Soren Brinkmann, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: >> From: Stephen Boyd <sboyd@codeaurora.org> >> >> On most ARM systems the per-cpu clockevents are truly per-cpu in >> the sense that they can't be controlled on any other CPU besides >> the CPU that they interrupt. If one of these clockevents were to >> become a broadcast source we will run into a lot of trouble >> because the broadcast source is enabled on the first CPU to go >> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >> could be a different CPU than what the clockevent is interrupting >> (or even worse the CPU that the clockevent interrupts could be >> offline). >> >> Theoretically it's possible to support per-cpu clockevents as the >> broadcast source but so far we haven't needed this and supporting >> it is rather complicated. Let's just deny the possibility for now >> until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. It sounds probably more understandable than dealing with the cpumasks. I am wondering if this is semantically opposed to http://lwn.net/Articles/566270/ ? [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states -- Daniel -- <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 * English - detected * English * French * English * French <javascript:void(0);> ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 8:25 ` Daniel Lezcano 0 siblings, 0 replies; 36+ messages in thread From: Daniel Lezcano @ 2013-09-13 8:25 UTC (permalink / raw) To: linux-arm-kernel On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: >> From: Stephen Boyd <sboyd@codeaurora.org> >> >> On most ARM systems the per-cpu clockevents are truly per-cpu in >> the sense that they can't be controlled on any other CPU besides >> the CPU that they interrupt. If one of these clockevents were to >> become a broadcast source we will run into a lot of trouble >> because the broadcast source is enabled on the first CPU to go >> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >> could be a different CPU than what the clockevent is interrupting >> (or even worse the CPU that the clockevent interrupts could be >> offline). >> >> Theoretically it's possible to support per-cpu clockevents as the >> broadcast source but so far we haven't needed this and supporting >> it is rather complicated. Let's just deny the possibility for now >> until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. It sounds probably more understandable than dealing with the cpumasks. I am wondering if this is semantically opposed to http://lwn.net/Articles/566270/ ? [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states -- Daniel -- <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 * English - detected * English * French * English * French <javascript:void(0);> ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 8:25 ` Daniel Lezcano 0 siblings, 0 replies; 36+ messages in thread From: Daniel Lezcano @ 2013-09-13 8:25 UTC (permalink / raw) To: Thomas Gleixner Cc: Soren Brinkmann, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > On Thu, 12 Sep 2013, Soren Brinkmann wrote: >> From: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >> >> On most ARM systems the per-cpu clockevents are truly per-cpu in >> the sense that they can't be controlled on any other CPU besides >> the CPU that they interrupt. If one of these clockevents were to >> become a broadcast source we will run into a lot of trouble >> because the broadcast source is enabled on the first CPU to go >> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >> could be a different CPU than what the clockevent is interrupting >> (or even worse the CPU that the clockevent interrupts could be >> offline). >> >> Theoretically it's possible to support per-cpu clockevents as the >> broadcast source but so far we haven't needed this and supporting >> it is rather complicated. Let's just deny the possibility for now >> until this becomes a reality (let's hope it never does!). > > Well, we can't do it this way. There are globally accessible clock > event devices which deliver only to cpu0. So the mask check might be > causing failure here. > > Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > device and check for it. It sounds probably more understandable than dealing with the cpumasks. I am wondering if this is semantically opposed to http://lwn.net/Articles/566270/ ? [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states -- Daniel -- <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 * English - detected * English * French * English * French <javascript:void(0);> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 36+ messages in thread
[parent not found: <CAM4v1pPQwBfQ3V6M2VGsc-Fh+VhLkQE2JZeoVc=_3kniODNEhA@mail.gmail.com>]
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 10:39 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-13 10:39 UTC (permalink / raw) To: Preeti Murthy, Daniel Lezcano, Thomas Gleixner Cc: Soren Brinkmann, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel Hi Soren, On 09/13/2013 03:50 PM, Preeti Murthy wrote: > Hi, > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > enables broadcast functionality > without using an external global clock device. It uses one of the per cpu > clock devices to enable the broadcast functionality. > > The way it achieves this is by creating a pseudo clock device and > associating it with one of the cpus clock device and > by having a hrtimer queued on the same cpu. This pseudo clock device acts > as the broadcast device, and the > per cpu clock device that it is associated with acts as the broadcast > source. > > The disadvantages that Soren mentions in having a per cpu clock device as > the broadcast source can be overcome > by following the approach proposed in this patch n the way described below: > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > The solution that the above patch proposes is associate the pseudo clock > device with another cpu and move the hrtimer > whose function is explained in the next point to another cpu. The broadcast > functionality continues to remain active transparently. > > 2. The cpu that requires broadcast functionality is different from the cpu > whose clock device is the broadcast source. > So how will the former cpu program/control the clock device of the latter > cpu? > > The above patch queues a hrtimer on the cpu whose clock device is the > broadcast source, which expires at > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > is what we define and dev is the > pseudo device whose next event is set by the broadcast framework. > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > with same as above, > max(tick_broadcast_period, dev->next_event). > > This ensures that a cpu that requires broadcast function to be activated > need not program the broadcast source, > which also happens to be a per cpu clock device. The hrtimer queued on the > cpu whose clock device is the > broadcast source takes care of when to do broadcast handling. > tick_broadcast_period ensures that we do > not miss wakeups. This is introduced to overcome the constraint of a cpu > not being able to program the clock > device of another cpu. > > Soren, do let me know if the above approach described in the patch has not > addressed any of the challenges > that you see with having a per cpu clock device as the broadcast source. > > Regards > Preeti U Murthy > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > <daniel.lezcano@linaro.org>wrote: > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: >>>> From: Stephen Boyd <sboyd@codeaurora.org> >>>> >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in >>>> the sense that they can't be controlled on any other CPU besides >>>> the CPU that they interrupt. If one of these clockevents were to >>>> become a broadcast source we will run into a lot of trouble >>>> because the broadcast source is enabled on the first CPU to go >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >>>> could be a different CPU than what the clockevent is interrupting >>>> (or even worse the CPU that the clockevent interrupts could be >>>> offline). >>>> >>>> Theoretically it's possible to support per-cpu clockevents as the >>>> broadcast source but so far we haven't needed this and supporting >>>> it is rather complicated. Let's just deny the possibility for now >>>> until this becomes a reality (let's hope it never does!). >>> >>> Well, we can't do it this way. There are globally accessible clock >>> event devices which deliver only to cpu0. So the mask check might be >>> causing failure here. >>> >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event >>> device and check for it. >> >> It sounds probably more understandable than dealing with the cpumasks. >> >> I am wondering if this is semantically opposed to >> http://lwn.net/Articles/566270/ ? >> >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states >> >> -- Daniel So the point I am trying to make is that the fix that you have proposed on this thread is valid. It is difficult to ensure that a per cpu clock device doubles up as the broadcast source without significant code changes to the current broadcast code and the timer code. But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states, attempts to overcome the disadvantage on certain architectures of not having an external clock device to perform broadcast *without* significant code changes in broadcast or timer. This patch does not conflict with what you are proposing in this thread of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock device that the patch introduces will not have this flag set anyway. So ideally architectures, without having a planned infrastructure in them cannot nominate their per cpu clock device as the broadcast source. And if they do have some infrastructure to support a per cpu clock device as broadcast source, they should ensure that the device passes your test as is proposed in this patch. So your fix is valid IMHO. That said it is still possible to manage without an external clock device for performing broadcast. Regards Preeti U Murthy ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 10:39 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-13 10:39 UTC (permalink / raw) To: linux-arm-kernel Hi Soren, On 09/13/2013 03:50 PM, Preeti Murthy wrote: > Hi, > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > enables broadcast functionality > without using an external global clock device. It uses one of the per cpu > clock devices to enable the broadcast functionality. > > The way it achieves this is by creating a pseudo clock device and > associating it with one of the cpus clock device and > by having a hrtimer queued on the same cpu. This pseudo clock device acts > as the broadcast device, and the > per cpu clock device that it is associated with acts as the broadcast > source. > > The disadvantages that Soren mentions in having a per cpu clock device as > the broadcast source can be overcome > by following the approach proposed in this patch n the way described below: > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > The solution that the above patch proposes is associate the pseudo clock > device with another cpu and move the hrtimer > whose function is explained in the next point to another cpu. The broadcast > functionality continues to remain active transparently. > > 2. The cpu that requires broadcast functionality is different from the cpu > whose clock device is the broadcast source. > So how will the former cpu program/control the clock device of the latter > cpu? > > The above patch queues a hrtimer on the cpu whose clock device is the > broadcast source, which expires at > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > is what we define and dev is the > pseudo device whose next event is set by the broadcast framework. > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > with same as above, > max(tick_broadcast_period, dev->next_event). > > This ensures that a cpu that requires broadcast function to be activated > need not program the broadcast source, > which also happens to be a per cpu clock device. The hrtimer queued on the > cpu whose clock device is the > broadcast source takes care of when to do broadcast handling. > tick_broadcast_period ensures that we do > not miss wakeups. This is introduced to overcome the constraint of a cpu > not being able to program the clock > device of another cpu. > > Soren, do let me know if the above approach described in the patch has not > addressed any of the challenges > that you see with having a per cpu clock device as the broadcast source. > > Regards > Preeti U Murthy > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > <daniel.lezcano@linaro.org>wrote: > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: >>>> From: Stephen Boyd <sboyd@codeaurora.org> >>>> >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in >>>> the sense that they can't be controlled on any other CPU besides >>>> the CPU that they interrupt. If one of these clockevents were to >>>> become a broadcast source we will run into a lot of trouble >>>> because the broadcast source is enabled on the first CPU to go >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >>>> could be a different CPU than what the clockevent is interrupting >>>> (or even worse the CPU that the clockevent interrupts could be >>>> offline). >>>> >>>> Theoretically it's possible to support per-cpu clockevents as the >>>> broadcast source but so far we haven't needed this and supporting >>>> it is rather complicated. Let's just deny the possibility for now >>>> until this becomes a reality (let's hope it never does!). >>> >>> Well, we can't do it this way. There are globally accessible clock >>> event devices which deliver only to cpu0. So the mask check might be >>> causing failure here. >>> >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event >>> device and check for it. >> >> It sounds probably more understandable than dealing with the cpumasks. >> >> I am wondering if this is semantically opposed to >> http://lwn.net/Articles/566270/ ? >> >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states >> >> -- Daniel So the point I am trying to make is that the fix that you have proposed on this thread is valid. It is difficult to ensure that a per cpu clock device doubles up as the broadcast source without significant code changes to the current broadcast code and the timer code. But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states, attempts to overcome the disadvantage on certain architectures of not having an external clock device to perform broadcast *without* significant code changes in broadcast or timer. This patch does not conflict with what you are proposing in this thread of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock device that the patch introduces will not have this flag set anyway. So ideally architectures, without having a planned infrastructure in them cannot nominate their per cpu clock device as the broadcast source. And if they do have some infrastructure to support a per cpu clock device as broadcast source, they should ensure that the device passes your test as is proposed in this patch. So your fix is valid IMHO. That said it is still possible to manage without an external clock device for performing broadcast. Regards Preeti U Murthy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 10:39 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-13 10:39 UTC (permalink / raw) To: Preeti Murthy, Daniel Lezcano, Thomas Gleixner Cc: Soren Brinkmann, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r Hi Soren, On 09/13/2013 03:50 PM, Preeti Murthy wrote: > Hi, > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > enables broadcast functionality > without using an external global clock device. It uses one of the per cpu > clock devices to enable the broadcast functionality. > > The way it achieves this is by creating a pseudo clock device and > associating it with one of the cpus clock device and > by having a hrtimer queued on the same cpu. This pseudo clock device acts > as the broadcast device, and the > per cpu clock device that it is associated with acts as the broadcast > source. > > The disadvantages that Soren mentions in having a per cpu clock device as > the broadcast source can be overcome > by following the approach proposed in this patch n the way described below: > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > The solution that the above patch proposes is associate the pseudo clock > device with another cpu and move the hrtimer > whose function is explained in the next point to another cpu. The broadcast > functionality continues to remain active transparently. > > 2. The cpu that requires broadcast functionality is different from the cpu > whose clock device is the broadcast source. > So how will the former cpu program/control the clock device of the latter > cpu? > > The above patch queues a hrtimer on the cpu whose clock device is the > broadcast source, which expires at > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > is what we define and dev is the > pseudo device whose next event is set by the broadcast framework. > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > with same as above, > max(tick_broadcast_period, dev->next_event). > > This ensures that a cpu that requires broadcast function to be activated > need not program the broadcast source, > which also happens to be a per cpu clock device. The hrtimer queued on the > cpu whose clock device is the > broadcast source takes care of when to do broadcast handling. > tick_broadcast_period ensures that we do > not miss wakeups. This is introduced to overcome the constraint of a cpu > not being able to program the clock > device of another cpu. > > Soren, do let me know if the above approach described in the patch has not > addressed any of the challenges > that you see with having a per cpu clock device as the broadcast source. > > Regards > Preeti U Murthy > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > <daniel.lezcano-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>wrote: > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: >>>> From: Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >>>> >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in >>>> the sense that they can't be controlled on any other CPU besides >>>> the CPU that they interrupt. If one of these clockevents were to >>>> become a broadcast source we will run into a lot of trouble >>>> because the broadcast source is enabled on the first CPU to go >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that >>>> could be a different CPU than what the clockevent is interrupting >>>> (or even worse the CPU that the clockevent interrupts could be >>>> offline). >>>> >>>> Theoretically it's possible to support per-cpu clockevents as the >>>> broadcast source but so far we haven't needed this and supporting >>>> it is rather complicated. Let's just deny the possibility for now >>>> until this becomes a reality (let's hope it never does!). >>> >>> Well, we can't do it this way. There are globally accessible clock >>> event devices which deliver only to cpu0. So the mask check might be >>> causing failure here. >>> >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event >>> device and check for it. >> >> It sounds probably more understandable than dealing with the cpumasks. >> >> I am wondering if this is semantically opposed to >> http://lwn.net/Articles/566270/ ? >> >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states >> >> -- Daniel So the point I am trying to make is that the fix that you have proposed on this thread is valid. It is difficult to ensure that a per cpu clock device doubles up as the broadcast source without significant code changes to the current broadcast code and the timer code. But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states, attempts to overcome the disadvantage on certain architectures of not having an external clock device to perform broadcast *without* significant code changes in broadcast or timer. This patch does not conflict with what you are proposing in this thread of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock device that the patch introduces will not have this flag set anyway. So ideally architectures, without having a planned infrastructure in them cannot nominate their per cpu clock device as the broadcast source. And if they do have some infrastructure to support a per cpu clock device as broadcast source, they should ensure that the device passes your test as is proposed in this patch. So your fix is valid IMHO. That said it is still possible to manage without an external clock device for performing broadcast. Regards Preeti U Murthy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources 2013-09-13 10:39 ` Preeti U Murthy (?) @ 2013-09-13 16:23 ` Sören Brinkmann -1 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 16:23 UTC (permalink / raw) To: Preeti U Murthy Cc: Preeti Murthy, Daniel Lezcano, Thomas Gleixner, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel Hi Preeti, On Fri, Sep 13, 2013 at 04:09:46PM +0530, Preeti U Murthy wrote: > Hi Soren, > > On 09/13/2013 03:50 PM, Preeti Murthy wrote: > > Hi, > > > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > > enables broadcast functionality > > without using an external global clock device. It uses one of the per cpu > > clock devices to enable the broadcast functionality. > > > > The way it achieves this is by creating a pseudo clock device and > > associating it with one of the cpus clock device and > > by having a hrtimer queued on the same cpu. This pseudo clock device acts > > as the broadcast device, and the > > per cpu clock device that it is associated with acts as the broadcast > > source. > > > > The disadvantages that Soren mentions in having a per cpu clock device as > > the broadcast source can be overcome > > by following the approach proposed in this patch n the way described below: > > > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > > > The solution that the above patch proposes is associate the pseudo clock > > device with another cpu and move the hrtimer > > whose function is explained in the next point to another cpu. The broadcast > > functionality continues to remain active transparently. > > > > 2. The cpu that requires broadcast functionality is different from the cpu > > whose clock device is the broadcast source. > > So how will the former cpu program/control the clock device of the latter > > cpu? > > > > The above patch queues a hrtimer on the cpu whose clock device is the > > broadcast source, which expires at > > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > > is what we define and dev is the > > pseudo device whose next event is set by the broadcast framework. > > > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > > with same as above, > > max(tick_broadcast_period, dev->next_event). > > > > This ensures that a cpu that requires broadcast function to be activated > > need not program the broadcast source, > > which also happens to be a per cpu clock device. The hrtimer queued on the > > cpu whose clock device is the > > broadcast source takes care of when to do broadcast handling. > > tick_broadcast_period ensures that we do > > not miss wakeups. This is introduced to overcome the constraint of a cpu > > not being able to program the clock > > device of another cpu. > > > > Soren, do let me know if the above approach described in the patch has not > > addressed any of the challenges > > that you see with having a per cpu clock device as the broadcast source. > > > > Regards > > Preeti U Murthy > > > > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > > <daniel.lezcano@linaro.org>wrote: > > > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: > >>>> From: Stephen Boyd <sboyd@codeaurora.org> > >>>> > >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in > >>>> the sense that they can't be controlled on any other CPU besides > >>>> the CPU that they interrupt. If one of these clockevents were to > >>>> become a broadcast source we will run into a lot of trouble > >>>> because the broadcast source is enabled on the first CPU to go > >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > >>>> could be a different CPU than what the clockevent is interrupting > >>>> (or even worse the CPU that the clockevent interrupts could be > >>>> offline). > >>>> > >>>> Theoretically it's possible to support per-cpu clockevents as the > >>>> broadcast source but so far we haven't needed this and supporting > >>>> it is rather complicated. Let's just deny the possibility for now > >>>> until this becomes a reality (let's hope it never does!). > >>> > >>> Well, we can't do it this way. There are globally accessible clock > >>> event devices which deliver only to cpu0. So the mask check might be > >>> causing failure here. > >>> > >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > >>> device and check for it. > >> > >> It sounds probably more understandable than dealing with the cpumasks. > >> > >> I am wondering if this is semantically opposed to > >> http://lwn.net/Articles/566270/ ? > >> > >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states > >> > >> -- Daniel > > So the point I am trying to make is that the fix that you have proposed > on this thread is valid. It is difficult to ensure that a per cpu clock > device doubles up as the broadcast source without significant code > changes to the current broadcast code and the timer code. > > But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for > deep idle states, attempts to overcome the disadvantage on certain > architectures of not having an external clock device to perform > broadcast *without* significant code changes in broadcast or timer. > > This patch does not conflict with what you are proposing in this thread > of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock > device that the patch introduces will not have this flag set anyway. > > So ideally architectures, without having a planned infrastructure in > them cannot nominate their per cpu clock device as the broadcast source. > And if they do have some infrastructure to support a per cpu clock > device as broadcast source, they should ensure that the device passes > your test as is proposed in this patch. So your fix is valid IMHO. That > said it is still possible to manage without an external clock device for > performing broadcast. Thanks for the explanation but now I'm a little confused. That's a lot of details and I'm lacking the in depth knowledge to fully understand everything. Is it correct to say, that your patch series enables per cpu devices to be the broadcast device - for PPC? And that would mean, that even though you have a per cpu device, you'd deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu timer is a valid broadcast device? Assuming that is not going into an utterly wrong direction: How would we close on this one? AFAIK, ARM does not have this capability and I guess it won't be added. So, should I go forward with the fix proposed by Thomas? Should we rename the FEAT_PERCPU flag to something else, given that PPC may use per cpu devices for broadcasting and the sole usage of that flag is to prevent such a device from becoming the broadcast device? Thanks, Sören ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 16:23 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 16:23 UTC (permalink / raw) To: linux-arm-kernel Hi Preeti, On Fri, Sep 13, 2013 at 04:09:46PM +0530, Preeti U Murthy wrote: > Hi Soren, > > On 09/13/2013 03:50 PM, Preeti Murthy wrote: > > Hi, > > > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > > enables broadcast functionality > > without using an external global clock device. It uses one of the per cpu > > clock devices to enable the broadcast functionality. > > > > The way it achieves this is by creating a pseudo clock device and > > associating it with one of the cpus clock device and > > by having a hrtimer queued on the same cpu. This pseudo clock device acts > > as the broadcast device, and the > > per cpu clock device that it is associated with acts as the broadcast > > source. > > > > The disadvantages that Soren mentions in having a per cpu clock device as > > the broadcast source can be overcome > > by following the approach proposed in this patch n the way described below: > > > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > > > The solution that the above patch proposes is associate the pseudo clock > > device with another cpu and move the hrtimer > > whose function is explained in the next point to another cpu. The broadcast > > functionality continues to remain active transparently. > > > > 2. The cpu that requires broadcast functionality is different from the cpu > > whose clock device is the broadcast source. > > So how will the former cpu program/control the clock device of the latter > > cpu? > > > > The above patch queues a hrtimer on the cpu whose clock device is the > > broadcast source, which expires at > > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > > is what we define and dev is the > > pseudo device whose next event is set by the broadcast framework. > > > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > > with same as above, > > max(tick_broadcast_period, dev->next_event). > > > > This ensures that a cpu that requires broadcast function to be activated > > need not program the broadcast source, > > which also happens to be a per cpu clock device. The hrtimer queued on the > > cpu whose clock device is the > > broadcast source takes care of when to do broadcast handling. > > tick_broadcast_period ensures that we do > > not miss wakeups. This is introduced to overcome the constraint of a cpu > > not being able to program the clock > > device of another cpu. > > > > Soren, do let me know if the above approach described in the patch has not > > addressed any of the challenges > > that you see with having a per cpu clock device as the broadcast source. > > > > Regards > > Preeti U Murthy > > > > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > > <daniel.lezcano@linaro.org>wrote: > > > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: > >>>> From: Stephen Boyd <sboyd@codeaurora.org> > >>>> > >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in > >>>> the sense that they can't be controlled on any other CPU besides > >>>> the CPU that they interrupt. If one of these clockevents were to > >>>> become a broadcast source we will run into a lot of trouble > >>>> because the broadcast source is enabled on the first CPU to go > >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > >>>> could be a different CPU than what the clockevent is interrupting > >>>> (or even worse the CPU that the clockevent interrupts could be > >>>> offline). > >>>> > >>>> Theoretically it's possible to support per-cpu clockevents as the > >>>> broadcast source but so far we haven't needed this and supporting > >>>> it is rather complicated. Let's just deny the possibility for now > >>>> until this becomes a reality (let's hope it never does!). > >>> > >>> Well, we can't do it this way. There are globally accessible clock > >>> event devices which deliver only to cpu0. So the mask check might be > >>> causing failure here. > >>> > >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > >>> device and check for it. > >> > >> It sounds probably more understandable than dealing with the cpumasks. > >> > >> I am wondering if this is semantically opposed to > >> http://lwn.net/Articles/566270/ ? > >> > >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states > >> > >> -- Daniel > > So the point I am trying to make is that the fix that you have proposed > on this thread is valid. It is difficult to ensure that a per cpu clock > device doubles up as the broadcast source without significant code > changes to the current broadcast code and the timer code. > > But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for > deep idle states, attempts to overcome the disadvantage on certain > architectures of not having an external clock device to perform > broadcast *without* significant code changes in broadcast or timer. > > This patch does not conflict with what you are proposing in this thread > of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock > device that the patch introduces will not have this flag set anyway. > > So ideally architectures, without having a planned infrastructure in > them cannot nominate their per cpu clock device as the broadcast source. > And if they do have some infrastructure to support a per cpu clock > device as broadcast source, they should ensure that the device passes > your test as is proposed in this patch. So your fix is valid IMHO. That > said it is still possible to manage without an external clock device for > performing broadcast. Thanks for the explanation but now I'm a little confused. That's a lot of details and I'm lacking the in depth knowledge to fully understand everything. Is it correct to say, that your patch series enables per cpu devices to be the broadcast device - for PPC? And that would mean, that even though you have a per cpu device, you'd deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu timer is a valid broadcast device? Assuming that is not going into an utterly wrong direction: How would we close on this one? AFAIK, ARM does not have this capability and I guess it won't be added. So, should I go forward with the fix proposed by Thomas? Should we rename the FEAT_PERCPU flag to something else, given that PPC may use per cpu devices for broadcasting and the sole usage of that flag is to prevent such a device from becoming the broadcast device? Thanks, S?ren ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-13 16:23 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-13 16:23 UTC (permalink / raw) To: Preeti U Murthy Cc: Preeti Murthy, Daniel Lezcano, Thomas Gleixner, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel Hi Preeti, On Fri, Sep 13, 2013 at 04:09:46PM +0530, Preeti U Murthy wrote: > Hi Soren, > > On 09/13/2013 03:50 PM, Preeti Murthy wrote: > > Hi, > > > > So the patch that Daniel points out http://lwn.net/Articles/566270/ , > > enables broadcast functionality > > without using an external global clock device. It uses one of the per cpu > > clock devices to enable the broadcast functionality. > > > > The way it achieves this is by creating a pseudo clock device and > > associating it with one of the cpus clock device and > > by having a hrtimer queued on the same cpu. This pseudo clock device acts > > as the broadcast device, and the > > per cpu clock device that it is associated with acts as the broadcast > > source. > > > > The disadvantages that Soren mentions in having a per cpu clock device as > > the broadcast source can be overcome > > by following the approach proposed in this patch n the way described below: > > > > 1. What if the cpu, whose clock device is the broadcast source goes offline? > > > > The solution that the above patch proposes is associate the pseudo clock > > device with another cpu and move the hrtimer > > whose function is explained in the next point to another cpu. The broadcast > > functionality continues to remain active transparently. > > > > 2. The cpu that requires broadcast functionality is different from the cpu > > whose clock device is the broadcast source. > > So how will the former cpu program/control the clock device of the latter > > cpu? > > > > The above patch queues a hrtimer on the cpu whose clock device is the > > broadcast source, which expires at > > max(tick_broadcast_period, dev->next_event), where tick_broadcast_period > > is what we define and dev is the > > pseudo device whose next event is set by the broadcast framework. > > > > On expiry of this hrtimer, do broadcast handling and reprogram the hrtimer > > with same as above, > > max(tick_broadcast_period, dev->next_event). > > > > This ensures that a cpu that requires broadcast function to be activated > > need not program the broadcast source, > > which also happens to be a per cpu clock device. The hrtimer queued on the > > cpu whose clock device is the > > broadcast source takes care of when to do broadcast handling. > > tick_broadcast_period ensures that we do > > not miss wakeups. This is introduced to overcome the constraint of a cpu > > not being able to program the clock > > device of another cpu. > > > > Soren, do let me know if the above approach described in the patch has not > > addressed any of the challenges > > that you see with having a per cpu clock device as the broadcast source. > > > > Regards > > Preeti U Murthy > > > > > > On Fri, Sep 13, 2013 at 1:55 PM, Daniel Lezcano > > <daniel.lezcano@linaro.org>wrote: > > > >> On 09/12/2013 10:30 PM, Thomas Gleixner wrote: > >>> On Thu, 12 Sep 2013, Soren Brinkmann wrote: > >>>> From: Stephen Boyd <sboyd@codeaurora.org> > >>>> > >>>> On most ARM systems the per-cpu clockevents are truly per-cpu in > >>>> the sense that they can't be controlled on any other CPU besides > >>>> the CPU that they interrupt. If one of these clockevents were to > >>>> become a broadcast source we will run into a lot of trouble > >>>> because the broadcast source is enabled on the first CPU to go > >>>> into deep idle (if that CPU suffers from FEAT_C3_STOP) and that > >>>> could be a different CPU than what the clockevent is interrupting > >>>> (or even worse the CPU that the clockevent interrupts could be > >>>> offline). > >>>> > >>>> Theoretically it's possible to support per-cpu clockevents as the > >>>> broadcast source but so far we haven't needed this and supporting > >>>> it is rather complicated. Let's just deny the possibility for now > >>>> until this becomes a reality (let's hope it never does!). > >>> > >>> Well, we can't do it this way. There are globally accessible clock > >>> event devices which deliver only to cpu0. So the mask check might be > >>> causing failure here. > >>> > >>> Just add a feature flag CLOCK_EVT_FEAT_PERCPU to the clock event > >>> device and check for it. > >> > >> It sounds probably more understandable than dealing with the cpumasks. > >> > >> I am wondering if this is semantically opposed to > >> http://lwn.net/Articles/566270/ ? > >> > >> [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for deep idle states > >> > >> -- Daniel > > So the point I am trying to make is that the fix that you have proposed > on this thread is valid. It is difficult to ensure that a per cpu clock > device doubles up as the broadcast source without significant code > changes to the current broadcast code and the timer code. > > But the patch [PATCH V3 0/6] cpuidle/ppc: Enable broadcast support for > deep idle states, attempts to overcome the disadvantage on certain > architectures of not having an external clock device to perform > broadcast *without* significant code changes in broadcast or timer. > > This patch does not conflict with what you are proposing in this thread > of having a feature flag CLOCK_EVT_FEAT_PERCPU, since the pseudo clock > device that the patch introduces will not have this flag set anyway. > > So ideally architectures, without having a planned infrastructure in > them cannot nominate their per cpu clock device as the broadcast source. > And if they do have some infrastructure to support a per cpu clock > device as broadcast source, they should ensure that the device passes > your test as is proposed in this patch. So your fix is valid IMHO. That > said it is still possible to manage without an external clock device for > performing broadcast. Thanks for the explanation but now I'm a little confused. That's a lot of details and I'm lacking the in depth knowledge to fully understand everything. Is it correct to say, that your patch series enables per cpu devices to be the broadcast device - for PPC? And that would mean, that even though you have a per cpu device, you'd deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu timer is a valid broadcast device? Assuming that is not going into an utterly wrong direction: How would we close on this one? AFAIK, ARM does not have this capability and I guess it won't be added. So, should I go forward with the fix proposed by Thomas? Should we rename the FEAT_PERCPU flag to something else, given that PPC may use per cpu devices for broadcasting and the sole usage of that flag is to prevent such a device from becoming the broadcast device? Thanks, Sören ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-14 0:23 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-14 0:23 UTC (permalink / raw) To: Sören Brinkmann Cc: Preeti Murthy, Daniel Lezcano, Thomas Gleixner, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel, svaidy Hi Soren, On 09/13/2013 09:53 PM, Sören Brinkmann wrote: > Hi Preeti, > Thanks for the explanation but now I'm a little confused. That's a lot of > details and I'm lacking the in depth knowledge to fully understand > everything. > > Is it correct to say, that your patch series enables per cpu devices to > be the broadcast device - for PPC? Not really. We have a pseudo clock device, which is registered as the broadcast device. This clock device has all the features of an external clock device that the broadcast framework expects from a broadcast device like !CLOCK_FEAT_C3STOP & !FEAT_PERCPU that you introduce in your patch. It as though we trick the broadcast framework into believing that we have an external device, while in reality the pseudo device is just a dummy. So if this is a pseudo device, which gets registered as the broadcast device, how do we program it to handle broadcast events? That is where the per cpu device steps in. It serves as the clock source to this pseudo device. Meaning we program the per cpu device for the next broadcast event using a hrtimer framework that we introduce, which calls pseudo_dev->event_handler on expiry. This is nothing but the broadcast handler. Therefore we are able to manage broadcast without having to have an explicit clock device for the purpose. > And that would mean, that even though you have a per cpu device, you'd > deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu > timer is a valid broadcast device? No we would set the FEAT_PERCPU for the per cpu device on PPC. As I mentioned above this is not going to be registered as the broadcast device. We would however not set this flag for the pseudo device, that we register as the broadcast device. > > Assuming that is not going into an utterly wrong direction: How would we > close on this one? AFAIK, ARM does not have this capability and I guess > it won't be added. So, should I go forward with the fix proposed by > Thomas? Should we rename the FEAT_PERCPU flag to something else, given > that PPC may use per cpu devices for broadcasting and the sole usage of > that flag is to prevent such a device from becoming the broadcast device? You can go ahead with this fix because as explained above, when we register a broadcast device we use a pseudo device which has the features that the broadcast framework approves. The per cpu device does not register itself with the broadcast framework. It merely programs itself for the next broadcast event. Hence this fix will not hinder the broadcast support on PPC. > > Thanks, > Sören > > Regards Preeti U Murthy ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-14 0:23 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-14 0:23 UTC (permalink / raw) To: linux-arm-kernel Hi Soren, On 09/13/2013 09:53 PM, S?ren Brinkmann wrote: > Hi Preeti, > Thanks for the explanation but now I'm a little confused. That's a lot of > details and I'm lacking the in depth knowledge to fully understand > everything. > > Is it correct to say, that your patch series enables per cpu devices to > be the broadcast device - for PPC? Not really. We have a pseudo clock device, which is registered as the broadcast device. This clock device has all the features of an external clock device that the broadcast framework expects from a broadcast device like !CLOCK_FEAT_C3STOP & !FEAT_PERCPU that you introduce in your patch. It as though we trick the broadcast framework into believing that we have an external device, while in reality the pseudo device is just a dummy. So if this is a pseudo device, which gets registered as the broadcast device, how do we program it to handle broadcast events? That is where the per cpu device steps in. It serves as the clock source to this pseudo device. Meaning we program the per cpu device for the next broadcast event using a hrtimer framework that we introduce, which calls pseudo_dev->event_handler on expiry. This is nothing but the broadcast handler. Therefore we are able to manage broadcast without having to have an explicit clock device for the purpose. > And that would mean, that even though you have a per cpu device, you'd > deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu > timer is a valid broadcast device? No we would set the FEAT_PERCPU for the per cpu device on PPC. As I mentioned above this is not going to be registered as the broadcast device. We would however not set this flag for the pseudo device, that we register as the broadcast device. > > Assuming that is not going into an utterly wrong direction: How would we > close on this one? AFAIK, ARM does not have this capability and I guess > it won't be added. So, should I go forward with the fix proposed by > Thomas? Should we rename the FEAT_PERCPU flag to something else, given > that PPC may use per cpu devices for broadcasting and the sole usage of > that flag is to prevent such a device from becoming the broadcast device? You can go ahead with this fix because as explained above, when we register a broadcast device we use a pseudo device which has the features that the broadcast framework approves. The per cpu device does not register itself with the broadcast framework. It merely programs itself for the next broadcast event. Hence this fix will not hinder the broadcast support on PPC. > > Thanks, > S?ren > > Regards Preeti U Murthy ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources @ 2013-09-14 0:23 ` Preeti U Murthy 0 siblings, 0 replies; 36+ messages in thread From: Preeti U Murthy @ 2013-09-14 0:23 UTC (permalink / raw) To: Sören Brinkmann Cc: Preeti Murthy, Daniel Lezcano, Thomas Gleixner, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, svaidy-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8 Hi Soren, On 09/13/2013 09:53 PM, Sören Brinkmann wrote: > Hi Preeti, > Thanks for the explanation but now I'm a little confused. That's a lot of > details and I'm lacking the in depth knowledge to fully understand > everything. > > Is it correct to say, that your patch series enables per cpu devices to > be the broadcast device - for PPC? Not really. We have a pseudo clock device, which is registered as the broadcast device. This clock device has all the features of an external clock device that the broadcast framework expects from a broadcast device like !CLOCK_FEAT_C3STOP & !FEAT_PERCPU that you introduce in your patch. It as though we trick the broadcast framework into believing that we have an external device, while in reality the pseudo device is just a dummy. So if this is a pseudo device, which gets registered as the broadcast device, how do we program it to handle broadcast events? That is where the per cpu device steps in. It serves as the clock source to this pseudo device. Meaning we program the per cpu device for the next broadcast event using a hrtimer framework that we introduce, which calls pseudo_dev->event_handler on expiry. This is nothing but the broadcast handler. Therefore we are able to manage broadcast without having to have an explicit clock device for the purpose. > And that would mean, that even though you have a per cpu device, you'd > deliberately not set the FEAT_PERCPU flag, because on PPC a per cpu > timer is a valid broadcast device? No we would set the FEAT_PERCPU for the per cpu device on PPC. As I mentioned above this is not going to be registered as the broadcast device. We would however not set this flag for the pseudo device, that we register as the broadcast device. > > Assuming that is not going into an utterly wrong direction: How would we > close on this one? AFAIK, ARM does not have this capability and I guess > it won't be added. So, should I go forward with the fix proposed by > Thomas? Should we rename the FEAT_PERCPU flag to something else, given > that PPC may use per cpu devices for broadcasting and the sole usage of > that flag is to prevent such a device from becoming the broadcast device? You can go ahead with this fix because as explained above, when we register a broadcast device we use a pseudo device which has the features that the broadcast framework approves. The per cpu device does not register itself with the broadcast framework. It merely programs itself for the next broadcast event. Hence this fix will not hinder the broadcast support on PPC. > > Thanks, > Sören > > Regards Preeti U Murthy -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/2] arm: zynq: Enable arm_global_timer 2013-09-12 16:50 ` Soren Brinkmann @ 2013-09-12 16:50 ` Soren Brinkmann -1 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: devicetree, linux-kernel, linux-arm-kernel, Soren Brinkmann Zynq is based on an ARM Cortex-A9 MPCore, which features the arm_global_timer in its SCU. Therefore enable the timer for Zynq. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ arch/arm/mach-zynq/Kconfig | 1 + 2 files changed, 9 insertions(+) diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi index e32b92b..eaacb39 100644 --- a/arch/arm/boot/dts/zynq-7000.dtsi +++ b/arch/arm/boot/dts/zynq-7000.dtsi @@ -92,6 +92,14 @@ }; }; + global_timer: global_timer@f8f00200 { + compatible = "arm,cortex-a9-global-timer"; + reg = <0xf8f00200 0x20>; + interrupts = <1 11 0x301>; + interrupt-parent = <&intc>; + clocks = <&clkc 4>; + }; + ttc0: ttc0@f8001000 { interrupt-parent = <&intc>; interrupts = < 0 10 4 0 11 4 0 12 4 >; diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig index 04f8a4a..6b04260 100644 --- a/arch/arm/mach-zynq/Kconfig +++ b/arch/arm/mach-zynq/Kconfig @@ -13,5 +13,6 @@ config ARCH_ZYNQ select HAVE_SMP select SPARSE_IRQ select CADENCE_TTC_TIMER + select ARM_GLOBAL_TIMER help Support for Xilinx Zynq ARM Cortex A9 Platform -- 1.8.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-12 16:50 ` Soren Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Soren Brinkmann @ 2013-09-12 16:50 UTC (permalink / raw) To: linux-arm-kernel Zynq is based on an ARM Cortex-A9 MPCore, which features the arm_global_timer in its SCU. Therefore enable the timer for Zynq. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ arch/arm/mach-zynq/Kconfig | 1 + 2 files changed, 9 insertions(+) diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi index e32b92b..eaacb39 100644 --- a/arch/arm/boot/dts/zynq-7000.dtsi +++ b/arch/arm/boot/dts/zynq-7000.dtsi @@ -92,6 +92,14 @@ }; }; + global_timer: global_timer at f8f00200 { + compatible = "arm,cortex-a9-global-timer"; + reg = <0xf8f00200 0x20>; + interrupts = <1 11 0x301>; + interrupt-parent = <&intc>; + clocks = <&clkc 4>; + }; + ttc0: ttc0 at f8001000 { interrupt-parent = <&intc>; interrupts = < 0 10 4 0 11 4 0 12 4 >; diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig index 04f8a4a..6b04260 100644 --- a/arch/arm/mach-zynq/Kconfig +++ b/arch/arm/mach-zynq/Kconfig @@ -13,5 +13,6 @@ config ARCH_ZYNQ select HAVE_SMP select SPARSE_IRQ select CADENCE_TTC_TIMER + select ARM_GLOBAL_TIMER help Support for Xilinx Zynq ARM Cortex A9 Platform -- 1.8.4 ^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-15 12:40 ` Grant Likely 0 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2013-09-15 12:40 UTC (permalink / raw) To: Soren Brinkmann, Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: devicetree, linux-kernel, linux-arm-kernel, Soren Brinkmann On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > Zynq is based on an ARM Cortex-A9 MPCore, which features the > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > arch/arm/mach-zynq/Kconfig | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > index e32b92b..eaacb39 100644 > --- a/arch/arm/boot/dts/zynq-7000.dtsi > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > @@ -92,6 +92,14 @@ > }; > }; > > + global_timer: global_timer@f8f00200 { Nit: node names and property names use '-' not '_'. Plus the generic names principle suggests the node should be names 'timer' not 'global_timer'. The following would be fine: global_timer: timer@f8f00200 { (There's a distinction between node names and labels. '_' is fine in labels since it doesn't get output into the compiled .dtb. g. > + compatible = "arm,cortex-a9-global-timer"; > + reg = <0xf8f00200 0x20>; > + interrupts = <1 11 0x301>; > + interrupt-parent = <&intc>; > + clocks = <&clkc 4>; > + }; > + > ttc0: ttc0@f8001000 { > interrupt-parent = <&intc>; > interrupts = < 0 10 4 0 11 4 0 12 4 >; > diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig > index 04f8a4a..6b04260 100644 > --- a/arch/arm/mach-zynq/Kconfig > +++ b/arch/arm/mach-zynq/Kconfig > @@ -13,5 +13,6 @@ config ARCH_ZYNQ > select HAVE_SMP > select SPARSE_IRQ > select CADENCE_TTC_TIMER > + select ARM_GLOBAL_TIMER > help > Support for Xilinx Zynq ARM Cortex A9 Platform > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-15 12:40 ` Grant Likely 0 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2013-09-15 12:40 UTC (permalink / raw) To: linux-arm-kernel On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > Zynq is based on an ARM Cortex-A9 MPCore, which features the > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > arch/arm/mach-zynq/Kconfig | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > index e32b92b..eaacb39 100644 > --- a/arch/arm/boot/dts/zynq-7000.dtsi > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > @@ -92,6 +92,14 @@ > }; > }; > > + global_timer: global_timer at f8f00200 { Nit: node names and property names use '-' not '_'. Plus the generic names principle suggests the node should be names 'timer' not 'global_timer'. The following would be fine: global_timer: timer at f8f00200 { (There's a distinction between node names and labels. '_' is fine in labels since it doesn't get output into the compiled .dtb. g. > + compatible = "arm,cortex-a9-global-timer"; > + reg = <0xf8f00200 0x20>; > + interrupts = <1 11 0x301>; > + interrupt-parent = <&intc>; > + clocks = <&clkc 4>; > + }; > + > ttc0: ttc0 at f8001000 { > interrupt-parent = <&intc>; > interrupts = < 0 10 4 0 11 4 0 12 4 >; > diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig > index 04f8a4a..6b04260 100644 > --- a/arch/arm/mach-zynq/Kconfig > +++ b/arch/arm/mach-zynq/Kconfig > @@ -13,5 +13,6 @@ config ARCH_ZYNQ > select HAVE_SMP > select SPARSE_IRQ > select CADENCE_TTC_TIMER > + select ARM_GLOBAL_TIMER > help > Support for Xilinx Zynq ARM Cortex A9 Platform > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-15 12:40 ` Grant Likely 0 siblings, 0 replies; 36+ messages in thread From: Grant Likely @ 2013-09-15 12:40 UTC (permalink / raw) To: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd Cc: devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Soren Brinkmann On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> wrote: > Zynq is based on an ARM Cortex-A9 MPCore, which features the > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> > --- > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > arch/arm/mach-zynq/Kconfig | 1 + > 2 files changed, 9 insertions(+) > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > index e32b92b..eaacb39 100644 > --- a/arch/arm/boot/dts/zynq-7000.dtsi > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > @@ -92,6 +92,14 @@ > }; > }; > > + global_timer: global_timer@f8f00200 { Nit: node names and property names use '-' not '_'. Plus the generic names principle suggests the node should be names 'timer' not 'global_timer'. The following would be fine: global_timer: timer@f8f00200 { (There's a distinction between node names and labels. '_' is fine in labels since it doesn't get output into the compiled .dtb. g. > + compatible = "arm,cortex-a9-global-timer"; > + reg = <0xf8f00200 0x20>; > + interrupts = <1 11 0x301>; > + interrupt-parent = <&intc>; > + clocks = <&clkc 4>; > + }; > + > ttc0: ttc0@f8001000 { > interrupt-parent = <&intc>; > interrupts = < 0 10 4 0 11 4 0 12 4 >; > diff --git a/arch/arm/mach-zynq/Kconfig b/arch/arm/mach-zynq/Kconfig > index 04f8a4a..6b04260 100644 > --- a/arch/arm/mach-zynq/Kconfig > +++ b/arch/arm/mach-zynq/Kconfig > @@ -13,5 +13,6 @@ config ARCH_ZYNQ > select HAVE_SMP > select SPARSE_IRQ > select CADENCE_TTC_TIMER > + select ARM_GLOBAL_TIMER > help > Support for Xilinx Zynq ARM Cortex A9 Platform > -- > 1.8.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-18 17:05 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-18 17:05 UTC (permalink / raw) To: Grant Likely Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd, devicetree, linux-kernel, linux-arm-kernel On Sun, Sep 15, 2013 at 01:40:36PM +0100, Grant Likely wrote: > On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > > Zynq is based on an ARM Cortex-A9 MPCore, which features the > > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > > arch/arm/mach-zynq/Kconfig | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > > index e32b92b..eaacb39 100644 > > --- a/arch/arm/boot/dts/zynq-7000.dtsi > > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > > @@ -92,6 +92,14 @@ > > }; > > }; > > > > + global_timer: global_timer@f8f00200 { > > Nit: node names and property names use '-' not '_'. Plus the generic > names principle suggests the node should be names 'timer' not > 'global_timer'. The following would be fine: That is really good to know. When I wrote this line, I read through a couple of other dtses and was unable to identify a consistent system behind node/label names. > > global_timer: timer@f8f00200 { All right, I'll change it to this. Thanks, Sören ^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-18 17:05 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-18 17:05 UTC (permalink / raw) To: linux-arm-kernel On Sun, Sep 15, 2013 at 01:40:36PM +0100, Grant Likely wrote: > On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > > Zynq is based on an ARM Cortex-A9 MPCore, which features the > > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > --- > > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > > arch/arm/mach-zynq/Kconfig | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > > index e32b92b..eaacb39 100644 > > --- a/arch/arm/boot/dts/zynq-7000.dtsi > > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > > @@ -92,6 +92,14 @@ > > }; > > }; > > > > + global_timer: global_timer at f8f00200 { > > Nit: node names and property names use '-' not '_'. Plus the generic > names principle suggests the node should be names 'timer' not > 'global_timer'. The following would be fine: That is really good to know. When I wrote this line, I read through a couple of other dtses and was unable to identify a consistent system behind node/label names. > > global_timer: timer at f8f00200 { All right, I'll change it to this. Thanks, S?ren ^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH 2/2] arm: zynq: Enable arm_global_timer @ 2013-09-18 17:05 ` Sören Brinkmann 0 siblings, 0 replies; 36+ messages in thread From: Sören Brinkmann @ 2013-09-18 17:05 UTC (permalink / raw) To: Grant Likely Cc: Rob Herring, Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell, Russell King, Michal Simek, Thomas Gleixner, Daniel Lezcano, Stephen Boyd, devicetree-u79uwXL29TY76Z2rM5mHXA, linux-kernel-u79uwXL29TY76Z2rM5mHXA, linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r On Sun, Sep 15, 2013 at 01:40:36PM +0100, Grant Likely wrote: > On Thu, 12 Sep 2013 09:50:40 -0700, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > > Zynq is based on an ARM Cortex-A9 MPCore, which features the > > arm_global_timer in its SCU. Therefore enable the timer for Zynq. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann-gjFFaj9aHVfQT0dZR+AlfA@public.gmane.org> > > --- > > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++++++++ > > arch/arm/mach-zynq/Kconfig | 1 + > > 2 files changed, 9 insertions(+) > > > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > > index e32b92b..eaacb39 100644 > > --- a/arch/arm/boot/dts/zynq-7000.dtsi > > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > > @@ -92,6 +92,14 @@ > > }; > > }; > > > > + global_timer: global_timer@f8f00200 { > > Nit: node names and property names use '-' not '_'. Plus the generic > names principle suggests the node should be names 'timer' not > 'global_timer'. The following would be fine: That is really good to know. When I wrote this line, I read through a couple of other dtses and was unable to identify a consistent system behind node/label names. > > global_timer: timer@f8f00200 { All right, I'll change it to this. Thanks, Sören -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2013-09-18 17:05 UTC | newest] Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2013-09-12 16:50 [PATCH 0/2] arm: zynq: Enable global timer Soren Brinkmann 2013-09-12 16:50 ` Soren Brinkmann 2013-09-12 16:50 ` Soren Brinkmann 2013-09-12 16:50 ` [PATCH 1/2] tick: broadcast: Deny per-cpu clockevents from being broadcast sources Soren Brinkmann 2013-09-12 16:50 ` Soren Brinkmann 2013-09-12 20:30 ` Thomas Gleixner 2013-09-12 20:30 ` Thomas Gleixner 2013-09-12 23:48 ` Sören Brinkmann 2013-09-12 23:48 ` Sören Brinkmann 2013-09-12 23:48 ` Sören Brinkmann 2013-09-13 14:45 ` Thomas Gleixner 2013-09-13 14:45 ` Thomas Gleixner 2013-09-13 14:45 ` Thomas Gleixner 2013-09-13 15:25 ` Sören Brinkmann 2013-09-13 15:25 ` Sören Brinkmann 2013-09-13 15:25 ` Sören Brinkmann 2013-09-13 8:25 ` Daniel Lezcano 2013-09-13 8:25 ` Daniel Lezcano 2013-09-13 8:25 ` Daniel Lezcano [not found] ` <CAM4v1pPQwBfQ3V6M2VGsc-Fh+VhLkQE2JZeoVc=_3kniODNEhA@mail.gmail.com> 2013-09-13 10:39 ` Preeti U Murthy 2013-09-13 10:39 ` Preeti U Murthy 2013-09-13 10:39 ` Preeti U Murthy 2013-09-13 16:23 ` Sören Brinkmann 2013-09-13 16:23 ` Sören Brinkmann 2013-09-13 16:23 ` Sören Brinkmann 2013-09-14 0:23 ` Preeti U Murthy 2013-09-14 0:23 ` Preeti U Murthy 2013-09-14 0:23 ` Preeti U Murthy 2013-09-12 16:50 ` [PATCH 2/2] arm: zynq: Enable arm_global_timer Soren Brinkmann 2013-09-12 16:50 ` Soren Brinkmann 2013-09-15 12:40 ` Grant Likely 2013-09-15 12:40 ` Grant Likely 2013-09-15 12:40 ` Grant Likely 2013-09-18 17:05 ` Sören Brinkmann 2013-09-18 17:05 ` Sören Brinkmann 2013-09-18 17:05 ` Sören Brinkmann
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.