| Message ID | 1370516488-25860-11-git-send-email-chander.kashyap@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, I have a few comments. On Thu, Jun 06, 2013 at 12:01:25PM +0100, Chander Kashyap wrote: > Extend the local timer interrupt support for handling four local timers. Is this the maximum number of CPUs the MCT could theoretically support? > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > --- > drivers/clocksource/exynos_mct.c | 33 ++++++++++++++++++++++++++++++--- > 1 file changed, 30 insertions(+), 3 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index 662fcc0..6af17d4 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -412,6 +412,18 @@ static struct irqaction mct_tick1_event_irq = { > .handler = exynos4_mct_tick_isr, > }; > > +static struct irqaction mct_tick2_event_irq = { > + .name = "mct_tick2_irq", > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > + .handler = exynos4_mct_tick_isr, > +}; > + > +static struct irqaction mct_tick3_event_irq = { > + .name = "mct_tick3_irq", > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > + .handler = exynos4_mct_tick_isr, > +}; > + Is there any reason you can't use {request,free}_irq? > static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) > { > struct mct_clock_event_device *mevt; > @@ -439,11 +451,21 @@ static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) > mct_tick0_event_irq.dev_id = mevt; > evt->irq = mct_irqs[MCT_L0_IRQ]; > setup_irq(evt->irq, &mct_tick0_event_irq); > - } else { > + } else if (cpu == 1) { > mct_tick1_event_irq.dev_id = mevt; > evt->irq = mct_irqs[MCT_L1_IRQ]; > setup_irq(evt->irq, &mct_tick1_event_irq); > irq_set_affinity(evt->irq, cpumask_of(1)); > + } else if (cpu == 2) { > + mct_tick2_event_irq.dev_id = mevt; > + evt->irq = mct_irqs[MCT_L2_IRQ]; > + setup_irq(evt->irq, &mct_tick2_event_irq); > + irq_set_affinity(evt->irq, cpumask_of(2)); > + } else if (cpu == 3) { > + mct_tick3_event_irq.dev_id = mevt; > + evt->irq = mct_irqs[MCT_L3_IRQ]; > + setup_irq(evt->irq, &mct_tick3_event_irq); > + irq_set_affinity(evt->irq, cpumask_of(3)); This doesn't seem good to me. You're duplicating the logic for each CPU. Can you not figure out which values you need based on the smp_processor_id (or even better, the *evt) without requiring a separate branch for each CPU? > } > } else { > enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); > @@ -456,11 +478,16 @@ static void exynos4_local_timer_stop(struct clock_event_device *evt) > { > unsigned int cpu = smp_processor_id(); > evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); > - if (mct_int_type == MCT_INT_SPI) > + if (mct_int_type == MCT_INT_SPI) { > if (cpu == 0) > remove_irq(evt->irq, &mct_tick0_event_irq); > - else > + else if (cpu == 1) > remove_irq(evt->irq, &mct_tick1_event_irq); > + else if (cpu == 2) > + remove_irq(evt->irq, &mct_tick2_event_irq); > + else if (cpu == 3) > + remove_irq(evt->irq, &mct_tick3_event_irq); > + } Again, I don't think each CPU should be special-cased. If you used {request,free}_irq this would be simpler. Thanks, Mark.
On Thursday 06 of June 2013 17:50:40 Mark Rutland wrote: > Hi, > > I have a few comments. > > On Thu, Jun 06, 2013 at 12:01:25PM +0100, Chander Kashyap wrote: > > Extend the local timer interrupt support for handling four local > > timers. > Is this the maximum number of CPUs the MCT could theoretically support? > > > Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> > > --- > > > > drivers/clocksource/exynos_mct.c | 33 > > ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), > > 3 deletions(-) > > > > diff --git a/drivers/clocksource/exynos_mct.c > > b/drivers/clocksource/exynos_mct.c index 662fcc0..6af17d4 100644 > > --- a/drivers/clocksource/exynos_mct.c > > +++ b/drivers/clocksource/exynos_mct.c > > @@ -412,6 +412,18 @@ static struct irqaction mct_tick1_event_irq = { > > > > .handler = exynos4_mct_tick_isr, > > > > }; > > > > +static struct irqaction mct_tick2_event_irq = { > > + .name = "mct_tick2_irq", > > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > > + .handler = exynos4_mct_tick_isr, > > +}; > > + > > +static struct irqaction mct_tick3_event_irq = { > > + .name = "mct_tick3_irq", > > + .flags = IRQF_TIMER | IRQF_NOBALANCING, > > + .handler = exynos4_mct_tick_isr, > > +}; > > + > > Is there any reason you can't use {request,free}_irq? > > > static int __cpuinit exynos4_local_timer_setup(struct > > clock_event_device *evt) { > > > > struct mct_clock_event_device *mevt; > > > > @@ -439,11 +451,21 @@ static int __cpuinit > > exynos4_local_timer_setup(struct clock_event_device *evt)> > > mct_tick0_event_irq.dev_id = mevt; > > evt->irq = mct_irqs[MCT_L0_IRQ]; > > setup_irq(evt->irq, &mct_tick0_event_irq); > > > > - } else { > > + } else if (cpu == 1) { > > > > mct_tick1_event_irq.dev_id = mevt; > > evt->irq = mct_irqs[MCT_L1_IRQ]; > > setup_irq(evt->irq, &mct_tick1_event_irq); > > irq_set_affinity(evt->irq, cpumask_of(1)); > > > > + } else if (cpu == 2) { > > + mct_tick2_event_irq.dev_id = mevt; > > + evt->irq = mct_irqs[MCT_L2_IRQ]; > > + setup_irq(evt->irq, &mct_tick2_event_irq); > > + irq_set_affinity(evt->irq, cpumask_of(2)); > > + } else if (cpu == 3) { > > + mct_tick3_event_irq.dev_id = mevt; > > + evt->irq = mct_irqs[MCT_L3_IRQ]; > > + setup_irq(evt->irq, &mct_tick3_event_irq); > > + irq_set_affinity(evt->irq, cpumask_of(3)); > > This doesn't seem good to me. You're duplicating the logic for each CPU. > Can you not figure out which values you need based on the > smp_processor_id (or even better, the *evt) without requiring a > separate branch for each CPU? > > } > > > > } else { > > > > enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); > > > > @@ -456,11 +478,16 @@ static void exynos4_local_timer_stop(struct > > clock_event_device *evt)> > > { > > > > unsigned int cpu = smp_processor_id(); > > evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); > > > > - if (mct_int_type == MCT_INT_SPI) > > + if (mct_int_type == MCT_INT_SPI) { > > > > if (cpu == 0) > > > > remove_irq(evt->irq, &mct_tick0_event_irq); > > > > - else > > + else if (cpu == 1) > > > > remove_irq(evt->irq, &mct_tick1_event_irq); > > > > + else if (cpu == 2) > > + remove_irq(evt->irq, &mct_tick2_event_irq); > > + else if (cpu == 3) > > + remove_irq(evt->irq, &mct_tick3_event_irq); > > + } > > Again, I don't think each CPU should be special-cased. If you used > {request,free}_irq this would be simpler. > > Thanks, > Mark. +1 to all Mark's comments. Best regards, Tomasz
On 6 June 2013 22:20, Mark Rutland <mark.rutland@arm.com> wrote: > Hi, > > I have a few comments. > > On Thu, Jun 06, 2013 at 12:01:25PM +0100, Chander Kashyap wrote: >> Extend the local timer interrupt support for handling four local timers. > > Is this the maximum number of CPUs the MCT could theoretically support? > >> >> Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> >> --- >> drivers/clocksource/exynos_mct.c | 33 ++++++++++++++++++++++++++++++--- >> 1 file changed, 30 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >> index 662fcc0..6af17d4 100644 >> --- a/drivers/clocksource/exynos_mct.c >> +++ b/drivers/clocksource/exynos_mct.c >> @@ -412,6 +412,18 @@ static struct irqaction mct_tick1_event_irq = { >> .handler = exynos4_mct_tick_isr, >> }; >> >> +static struct irqaction mct_tick2_event_irq = { >> + .name = "mct_tick2_irq", >> + .flags = IRQF_TIMER | IRQF_NOBALANCING, >> + .handler = exynos4_mct_tick_isr, >> +}; >> + >> +static struct irqaction mct_tick3_event_irq = { >> + .name = "mct_tick3_irq", >> + .flags = IRQF_TIMER | IRQF_NOBALANCING, >> + .handler = exynos4_mct_tick_isr, >> +}; >> + > > Is there any reason you can't use {request,free}_irq? > >> static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) >> { >> struct mct_clock_event_device *mevt; >> @@ -439,11 +451,21 @@ static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) >> mct_tick0_event_irq.dev_id = mevt; >> evt->irq = mct_irqs[MCT_L0_IRQ]; >> setup_irq(evt->irq, &mct_tick0_event_irq); >> - } else { >> + } else if (cpu == 1) { >> mct_tick1_event_irq.dev_id = mevt; >> evt->irq = mct_irqs[MCT_L1_IRQ]; >> setup_irq(evt->irq, &mct_tick1_event_irq); >> irq_set_affinity(evt->irq, cpumask_of(1)); >> + } else if (cpu == 2) { >> + mct_tick2_event_irq.dev_id = mevt; >> + evt->irq = mct_irqs[MCT_L2_IRQ]; >> + setup_irq(evt->irq, &mct_tick2_event_irq); >> + irq_set_affinity(evt->irq, cpumask_of(2)); >> + } else if (cpu == 3) { >> + mct_tick3_event_irq.dev_id = mevt; >> + evt->irq = mct_irqs[MCT_L3_IRQ]; >> + setup_irq(evt->irq, &mct_tick3_event_irq); >> + irq_set_affinity(evt->irq, cpumask_of(3)); > > This doesn't seem good to me. You're duplicating the logic for each CPU. Can > you not figure out which values you need based on the smp_processor_id (or even > better, the *evt) without requiring a separate branch for each CPU? > >> } >> } else { >> enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); >> @@ -456,11 +478,16 @@ static void exynos4_local_timer_stop(struct clock_event_device *evt) >> { >> unsigned int cpu = smp_processor_id(); >> evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); >> - if (mct_int_type == MCT_INT_SPI) >> + if (mct_int_type == MCT_INT_SPI) { >> if (cpu == 0) >> remove_irq(evt->irq, &mct_tick0_event_irq); >> - else >> + else if (cpu == 1) >> remove_irq(evt->irq, &mct_tick1_event_irq); >> + else if (cpu == 2) >> + remove_irq(evt->irq, &mct_tick2_event_irq); >> + else if (cpu == 3) >> + remove_irq(evt->irq, &mct_tick3_event_irq); >> + } > > Again, I don't think each CPU should be special-cased. If you used > {request,free}_irq this would be simpler. I will convert the calls to {request,free}_irq. All problem will be taken care by that. Thanks for the review. > > Thanks, > Mark. -- with warm regards, Chander Kashyap
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index 662fcc0..6af17d4 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -412,6 +412,18 @@ static struct irqaction mct_tick1_event_irq = { .handler = exynos4_mct_tick_isr, }; +static struct irqaction mct_tick2_event_irq = { + .name = "mct_tick2_irq", + .flags = IRQF_TIMER | IRQF_NOBALANCING, + .handler = exynos4_mct_tick_isr, +}; + +static struct irqaction mct_tick3_event_irq = { + .name = "mct_tick3_irq", + .flags = IRQF_TIMER | IRQF_NOBALANCING, + .handler = exynos4_mct_tick_isr, +}; + static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) { struct mct_clock_event_device *mevt; @@ -439,11 +451,21 @@ static int __cpuinit exynos4_local_timer_setup(struct clock_event_device *evt) mct_tick0_event_irq.dev_id = mevt; evt->irq = mct_irqs[MCT_L0_IRQ]; setup_irq(evt->irq, &mct_tick0_event_irq); - } else { + } else if (cpu == 1) { mct_tick1_event_irq.dev_id = mevt; evt->irq = mct_irqs[MCT_L1_IRQ]; setup_irq(evt->irq, &mct_tick1_event_irq); irq_set_affinity(evt->irq, cpumask_of(1)); + } else if (cpu == 2) { + mct_tick2_event_irq.dev_id = mevt; + evt->irq = mct_irqs[MCT_L2_IRQ]; + setup_irq(evt->irq, &mct_tick2_event_irq); + irq_set_affinity(evt->irq, cpumask_of(2)); + } else if (cpu == 3) { + mct_tick3_event_irq.dev_id = mevt; + evt->irq = mct_irqs[MCT_L3_IRQ]; + setup_irq(evt->irq, &mct_tick3_event_irq); + irq_set_affinity(evt->irq, cpumask_of(3)); } } else { enable_percpu_irq(mct_irqs[MCT_L0_IRQ], 0); @@ -456,11 +478,16 @@ static void exynos4_local_timer_stop(struct clock_event_device *evt) { unsigned int cpu = smp_processor_id(); evt->set_mode(CLOCK_EVT_MODE_UNUSED, evt); - if (mct_int_type == MCT_INT_SPI) + if (mct_int_type == MCT_INT_SPI) { if (cpu == 0) remove_irq(evt->irq, &mct_tick0_event_irq); - else + else if (cpu == 1) remove_irq(evt->irq, &mct_tick1_event_irq); + else if (cpu == 2) + remove_irq(evt->irq, &mct_tick2_event_irq); + else if (cpu == 3) + remove_irq(evt->irq, &mct_tick3_event_irq); + } else disable_percpu_irq(mct_irqs[MCT_L0_IRQ]); }
Extend the local timer interrupt support for handling four local timers. Signed-off-by: Chander Kashyap <chander.kashyap@linaro.org> --- drivers/clocksource/exynos_mct.c | 33 ++++++++++++++++++++++++++++++--- 1 file changed, 30 insertions(+), 3 deletions(-)