2014-12-16 09:46:15

by Magnus Damm

[permalink] [raw]
Subject: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

From: Magnus Damm <[email protected]>

Update the TMU driver to use cpu_possible_mask as cpumask to make
r8a7779 SMP work as expected with or without the ARM TWD timer.

Signed-off-by: Magnus Damm <[email protected]>
---

drivers/clocksource/sh_tmu.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/drivers/clocksource/sh_tmu.c
+++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
@@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
ced->features = CLOCK_EVT_FEAT_PERIODIC;
ced->features |= CLOCK_EVT_FEAT_ONESHOT;
ced->rating = 200;
- ced->cpumask = cpumask_of(0);
+ ced->cpumask = cpu_possible_mask;
ced->set_next_event = sh_tmu_clock_event_next;
ced->set_mode = sh_tmu_clock_event_mode;
ced->suspend = sh_tmu_clock_event_suspend;


2014-12-16 11:14:44

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

On 12/16/2014 10:48 AM, Magnus Damm wrote:
> From: Magnus Damm <[email protected]>
>
> Update the TMU driver to use cpu_possible_mask as cpumask to make
> r8a7779 SMP work as expected with or without the ARM TWD timer.
>
> Signed-off-by: Magnus Damm <[email protected]>

Applied as a 3.18 fix.

Thanks !

-- Daniel

ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?

> ---
>
> drivers/clocksource/sh_tmu.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> --- 0001/drivers/clocksource/sh_tmu.c
> +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
> @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
> ced->features = CLOCK_EVT_FEAT_PERIODIC;
> ced->features |= CLOCK_EVT_FEAT_ONESHOT;
> ced->rating = 200;
> - ced->cpumask = cpumask_of(0);
> + ced->cpumask = cpu_possible_mask;
> ced->set_next_event = sh_tmu_clock_event_next;
> ced->set_mode = sh_tmu_clock_event_mode;
> ced->suspend = sh_tmu_clock_event_suspend;
>


--
<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

2014-12-16 11:20:55

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Daniel,

On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
> On 12/16/2014 10:48 AM, Magnus Damm wrote:
> > From: Magnus Damm <[email protected]>
> >
> > Update the TMU driver to use cpu_possible_mask as cpumask to make
> > r8a7779 SMP work as expected with or without the ARM TWD timer.
> >
> > Signed-off-by: Magnus Damm <[email protected]>
>
> Applied as a 3.18 fix.

You're a bit too fast, I haven't had time to review the patch yet.

> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
>
> > ---
> >
> > drivers/clocksource/sh_tmu.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > --- 0001/drivers/clocksource/sh_tmu.c
> > +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
> > @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
> >
> > ced->features = CLOCK_EVT_FEAT_PERIODIC;
> > ced->features |= CLOCK_EVT_FEAT_ONESHOT;
> > ced->rating = 200;
> > - ced->cpumask = cpumask_of(0);
> > + ced->cpumask = cpu_possible_mask;

Magnus, how thoroughly have you tested this ? The TMU is indeed usable by all
CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but last time
I've tried that it broke the broadcast timer due to the heuristics used by the
clock events core code.

Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
userspace and tested timer broadcast on all CPUs ?

> > ced->set_next_event = sh_tmu_clock_event_next;
> > ced->set_mode = sh_tmu_clock_event_mode;
> > ced->suspend = sh_tmu_clock_event_suspend;

--
Regards,

Laurent Pinchart

2014-12-16 11:25:15

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus,

On Tuesday 16 December 2014 13:20:56 Laurent Pinchart wrote:
> On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
> > On 12/16/2014 10:48 AM, Magnus Damm wrote:
> > > From: Magnus Damm <[email protected]>
> > >
> > > Update the TMU driver to use cpu_possible_mask as cpumask to make
> > > r8a7779 SMP work as expected with or without the ARM TWD timer.
> > >
> > > Signed-off-by: Magnus Damm <[email protected]>
> >
> > Applied as a 3.18 fix.
>
> You're a bit too fast, I haven't had time to review the patch yet.
>
> > ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
> >
> > > ---
> > >
> > > drivers/clocksource/sh_tmu.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > --- 0001/drivers/clocksource/sh_tmu.c
> > > +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000
> > > +0900
> > > @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
> > >
> > > ced->features = CLOCK_EVT_FEAT_PERIODIC;
> > > ced->features |= CLOCK_EVT_FEAT_ONESHOT;
> > > ced->rating = 200;
> > >
> > > - ced->cpumask = cpumask_of(0);
> > > + ced->cpumask = cpu_possible_mask;
>
> Magnus, how thoroughly have you tested this ? The TMU is indeed usable by
> all CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but
> last time I've tried that it broke the broadcast timer due to the
> heuristics used by the clock events core code.
>
> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
> userspace and tested timer broadcast on all CPUs ?

I forgot to mention that this should be tested on R8A7740 with TMU only and
both TMU and CMT, and on r8A7779 with TMU only both TMU and TWD.

Without getting test results I'm afraid I must NACK the patch.

> > > ced->set_next_event = sh_tmu_clock_event_next;
> > > ced->set_mode = sh_tmu_clock_event_mode;
> > > ced->suspend = sh_tmu_clock_event_suspend;

--
Regards,

Laurent Pinchart

2014-12-16 11:29:15

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

On 12/16/2014 12:20 PM, Laurent Pinchart wrote:
> Hi Daniel,
>
> On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
>> On 12/16/2014 10:48 AM, Magnus Damm wrote:
>>> From: Magnus Damm <[email protected]>
>>>
>>> Update the TMU driver to use cpu_possible_mask as cpumask to make
>>> r8a7779 SMP work as expected with or without the ARM TWD timer.
>>>
>>> Signed-off-by: Magnus Damm <[email protected]>
>>
>> Applied as a 3.18 fix.
>
> You're a bit too fast, I haven't had time to review the patch yet.

Ah, ok. Hanging on then :)

>
>> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
>>
>>> ---
>>>
>>> drivers/clocksource/sh_tmu.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> --- 0001/drivers/clocksource/sh_tmu.c
>>> +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
>>> @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
>>>
>>> ced->features = CLOCK_EVT_FEAT_PERIODIC;
>>> ced->features |= CLOCK_EVT_FEAT_ONESHOT;
>>> ced->rating = 200;
>>> - ced->cpumask = cpumask_of(0);
>>> + ced->cpumask = cpu_possible_mask;
>
> Magnus, how thoroughly have you tested this ? The TMU is indeed usable by all
> CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but last time
> I've tried that it broke the broadcast timer due to the heuristics used by the
> clock events core code.
>
> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
> userspace and tested timer broadcast on all CPUs ?
>
>>> ced->set_next_event = sh_tmu_clock_event_next;
>>> ced->set_mode = sh_tmu_clock_event_mode;
>>> ced->suspend = sh_tmu_clock_event_suspend;
>


--
<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

2014-12-16 11:46:37

by Magnus Damm

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent,

On Tue, Dec 16, 2014 at 8:20 PM, Laurent Pinchart
<[email protected]> wrote:
> Hi Daniel,
>
> On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
>> On 12/16/2014 10:48 AM, Magnus Damm wrote:
>> > From: Magnus Damm <[email protected]>
>> >
>> > Update the TMU driver to use cpu_possible_mask as cpumask to make
>> > r8a7779 SMP work as expected with or without the ARM TWD timer.
>> >
>> > Signed-off-by: Magnus Damm <[email protected]>
>>
>> Applied as a 3.18 fix.
>
> You're a bit too fast, I haven't had time to review the patch yet.
>
>> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
>>
>> > ---
>> >
>> > drivers/clocksource/sh_tmu.c | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > --- 0001/drivers/clocksource/sh_tmu.c
>> > +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
>> > @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
>> >
>> > ced->features = CLOCK_EVT_FEAT_PERIODIC;
>> > ced->features |= CLOCK_EVT_FEAT_ONESHOT;
>> > ced->rating = 200;
>> > - ced->cpumask = cpumask_of(0);
>> > + ced->cpumask = cpu_possible_mask;
>
> Magnus, how thoroughly have you tested this ? The TMU is indeed usable by all
> CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but last time
> I've tried that it broke the broadcast timer due to the heuristics used by the
> clock events core code.

Uhm, so I've tested this particular patch on r8a7779 but I do agree
that the TMU is used on a bunch of SoCs if that's what you mean. I
don't see how it is different from any other of our timers though, and
those got fixed like this earlier.

I wonder if you may recall an earlier issue with incorrect clock event
priorities and code somehow working-by-accident without the mask set
as expected?

> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
> userspace and tested timer broadcast on all CPUs ?

No I have not. I've booted to user space in initramfs with DT-based
TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
broken on r8a7779 - both legacy and non-legacy. I have not gotten to
sh73a0 yet, but I assume it is busted too.

Can you please explain to me how the TMU is any different compared to
the CMT, MTU2 or STI? =)

And no, I don't have any r8a7740 board anymore. Can anyone else test?

Cheers,

/ magnus

2014-12-16 11:54:59

by Daniel Lezcano

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

On 12/16/2014 12:46 PM, Magnus Damm wrote:
> Hi Laurent,
>
> On Tue, Dec 16, 2014 at 8:20 PM, Laurent Pinchart
> <[email protected]> wrote:
>> Hi Daniel,
>>
>> On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
>>> On 12/16/2014 10:48 AM, Magnus Damm wrote:
>>>> From: Magnus Damm <[email protected]>
>>>>
>>>> Update the TMU driver to use cpu_possible_mask as cpumask to make
>>>> r8a7779 SMP work as expected with or without the ARM TWD timer.
>>>>
>>>> Signed-off-by: Magnus Damm <[email protected]>
>>>
>>> Applied as a 3.18 fix.
>>
>> You're a bit too fast, I haven't had time to review the patch yet.
>>
>>> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
>>>
>>>> ---
>>>>
>>>> drivers/clocksource/sh_tmu.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> --- 0001/drivers/clocksource/sh_tmu.c
>>>> +++ work/drivers/clocksource/sh_tmu.c 2014-12-16 17:49:49.000000000 +0900
>>>> @@ -428,7 +428,7 @@ static void sh_tmu_register_clockevent(s
>>>>
>>>> ced->features = CLOCK_EVT_FEAT_PERIODIC;
>>>> ced->features |= CLOCK_EVT_FEAT_ONESHOT;
>>>> ced->rating = 200;
>>>> - ced->cpumask = cpumask_of(0);
>>>> + ced->cpumask = cpu_possible_mask;
>>
>> Magnus, how thoroughly have you tested this ? The TMU is indeed usable by all
>> CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but last time
>> I've tried that it broke the broadcast timer due to the heuristics used by the
>> clock events core code.
>
> Uhm, so I've tested this particular patch on r8a7779 but I do agree
> that the TMU is used on a bunch of SoCs if that's what you mean. I
> don't see how it is different from any other of our timers though, and
> those got fixed like this earlier.
>
> I wonder if you may recall an earlier issue with incorrect clock event
> priorities and code somehow working-by-accident without the mask set
> as expected?

Could have been fixed with : ?

commit 70e5975d3a04be5479a28eec4a2fb10f98ad2785
Author: Stephen Boyd <[email protected]>
Date: Thu Jun 13 11:39:50 2013 -0700

clockevents: Prefer CPU local devices over global devices

On an SMP system with only one global clockevent and a dummy
clockevent per CPU we run into problems. We want the dummy
clockevents to be registered as the per CPU tick devices, but
we can only achieve that if we register the dummy clockevents
before the global clockevent or if we artificially inflate the
rating of the dummy clockevents to be higher than the rating
of the global clockevent. Failure to do so leads to boot
hangs when the dummy timers are registered on all other CPUs
besides the CPU that accepted the global clockevent as its tick
device and there is no broadcast timer to poke the dummy
devices.

If we're registering multiple clockevents and one clockevent is
global and the other is local to a particular CPU we should
choose to use the local clockevent regardless of the rating of
the device. This way, if the clockevent is a dummy it will take
the tick device duty as long as there isn't a higher rated tick
device and any global clockevent will be bumped out into
broadcast mode, fixing the problem described above.



>> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
>> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
>> userspace and tested timer broadcast on all CPUs ?
>
> No I have not. I've booted to user space in initramfs with DT-based
> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
> sh73a0 yet, but I assume it is busted too.
>
> Can you please explain to me how the TMU is any different compared to
> the CMT, MTU2 or STI? =)
>
> And no, I don't have any r8a7740 board anymore. Can anyone else test?
>
> Cheers,
>
> / magnus
>


--
<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

2014-12-16 12:40:51

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus,

On Tue, Dec 16, 2014 at 12:46 PM, Magnus Damm <[email protected]> wrote:
>> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
>> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
>> userspace and tested timer broadcast on all CPUs ?
>
> No I have not. I've booted to user space in initramfs with DT-based
> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
> sh73a0 yet, but I assume it is busted too.

FWIW, I applied this patch, and booted the result successfully and without
any user-visible changes on:
- koelsch
- armadillo-multiplatform
- kzm9g-legacy
- kzm9g-multiplatform

Armadillo-legacy is broken, and probably won't come back (cfr.
https://lkml.org/lkml/2014/12/2/339).

Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
at some point in the past).

Note that my local tree is based on renesas-drivers-2014-12-08-v3.18,
renesas-devel-20141212-v3.18, and yesterday's upstream, with CCF,
PM domain, TWD, and lots of WIP patches from the kitchen sink applied.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2014-12-17 00:43:41

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus,

On Tuesday 16 December 2014 20:46:33 Magnus Damm wrote:
> On Tue, Dec 16, 2014 at 8:20 PM, Laurent Pinchart wrote:
> > On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
> >> On 12/16/2014 10:48 AM, Magnus Damm wrote:
> >> > From: Magnus Damm <[email protected]>
> >> >
> >> > Update the TMU driver to use cpu_possible_mask as cpumask to make
> >> > r8a7779 SMP work as expected with or without the ARM TWD timer.
> >> >
> >> > Signed-off-by: Magnus Damm <[email protected]>
> >>
> >> Applied as a 3.18 fix.
> >
> > You're a bit too fast, I haven't had time to review the patch yet.
> >
> >> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver ?
> >>
> >> > ---
> >> >
> >> > drivers/clocksource/sh_tmu.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > --- 0001/drivers/clocksource/sh_tmu.c
> >> > +++ work/drivers/clocksource/sh_tmu.c 2014-12-16
> >> > 17:49:49.000000000 +0900 @@ -428,7 +428,7 @@ static void
> >> > sh_tmu_register_clockevent(s
> >> > ced->features = CLOCK_EVT_FEAT_PERIODIC;
> >> > ced->features |= CLOCK_EVT_FEAT_ONESHOT;
> >> > ced->rating = 200;
> >> > - ced->cpumask = cpumask_of(0);
> >> > + ced->cpumask = cpu_possible_mask;
> >
> > Magnus, how thoroughly have you tested this ? The TMU is indeed usable by
> > all CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but
> > last time I've tried that it broke the broadcast timer due to the
> > heuristics used by the clock events core code.
>
> Uhm, so I've tested this particular patch on r8a7779 but I do agree
> that the TMU is used on a bunch of SoCs if that's what you mean. I
> don't see how it is different from any other of our timers though, and
> those got fixed like this earlier.
>
> I wonder if you may recall an earlier issue with incorrect clock event
> priorities and code somehow working-by-accident without the mask set
> as expected?

As discussed privately today, I've had this exact same patch in one of my
private branches, and I haven't pushed it upstream at the same time as the
similar cmt and mtu2 patches due to a regression. Unfortunately I don't
remember the details :-/

I'll test the patch on Marzen and KZM9G. I unfortunately can't test it on
Armadillo as I don't have access to that board anymore.

> > Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> > CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted
> > to userspace and tested timer broadcast on all CPUs ?
>
> No I have not. I've booted to user space in initramfs with DT-based
> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
> sh73a0 yet, but I assume it is busted too.
>
> Can you please explain to me how the TMU is any different compared to
> the CMT, MTU2 or STI? =)

I don't know yet :-) I'll hopefully have more information after testing on
KZM9G.

> And no, I don't have any r8a7740 board anymore. Can anyone else test?

--
Regards,

Laurent Pinchart

2014-12-17 00:44:51

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Daniel,

On Tuesday 16 December 2014 12:54:56 Daniel Lezcano wrote:
> On 12/16/2014 12:46 PM, Magnus Damm wrote:
> > On Tue, Dec 16, 2014 at 8:20 PM, Laurent Pinchart wrote:
> >> On Tuesday 16 December 2014 12:14:40 Daniel Lezcano wrote:
> >>> On 12/16/2014 10:48 AM, Magnus Damm wrote:
> >>>> From: Magnus Damm <[email protected]>
> >>>>
> >>>> Update the TMU driver to use cpu_possible_mask as cpumask to make
> >>>> r8a7779 SMP work as expected with or without the ARM TWD timer.
> >>>>
> >>>> Signed-off-by: Magnus Damm <[email protected]>
> >>>
> >>> Applied as a 3.18 fix.
> >>
> >> You're a bit too fast, I haven't had time to review the patch yet.
> >>
> >>> ps: May I suggest to use the CLOCK_EVT_FEAT_DYNIRQ flag for this driver
> >>> ?
> >>>
> >>>> ---
> >>>> drivers/clocksource/sh_tmu.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> --- 0001/drivers/clocksource/sh_tmu.c
> >>>> +++ work/drivers/clocksource/sh_tmu.c 2014-12-16
> >>>> 17:49:49.000000000 +0900 @@ -428,7 +428,7 @@ static void
> >>>> sh_tmu_register_clockevent(s
> >>>> ced->features = CLOCK_EVT_FEAT_PERIODIC;
> >>>> ced->features |= CLOCK_EVT_FEAT_ONESHOT;
> >>>> ced->rating = 200;
> >>>> - ced->cpumask = cpumask_of(0);
> >>>> + ced->cpumask = cpu_possible_mask;
> >>
> >> Magnus, how thoroughly have you tested this ? The TMU is indeed usable by
> >> all CPUs, so setting the CPU mask to cpu_possible_mask makes sense, but
> >> last time I've tried that it broke the broadcast timer due to the
> >> heuristics used by the clock events core code.
> >
> > Uhm, so I've tested this particular patch on r8a7779 but I do agree
> > that the TMU is used on a bunch of SoCs if that's what you mean. I
> > don't see how it is different from any other of our timers though, and
> > those got fixed like this earlier.
> >
> > I wonder if you may recall an earlier issue with incorrect clock event
> > priorities and code somehow working-by-accident without the mask set
> > as expected?
>
> Could have been fixed with : ?
>
> commit 70e5975d3a04be5479a28eec4a2fb10f98ad2785
> Author: Stephen Boyd <[email protected]>
> Date: Thu Jun 13 11:39:50 2013 -0700
>
> clockevents: Prefer CPU local devices over global devices
>
> On an SMP system with only one global clockevent and a dummy
> clockevent per CPU we run into problems. We want the dummy
> clockevents to be registered as the per CPU tick devices, but
> we can only achieve that if we register the dummy clockevents
> before the global clockevent or if we artificially inflate the
> rating of the dummy clockevents to be higher than the rating
> of the global clockevent. Failure to do so leads to boot
> hangs when the dummy timers are registered on all other CPUs
> besides the CPU that accepted the global clockevent as its tick
> device and there is no broadcast timer to poke the dummy
> devices.
>
> If we're registering multiple clockevents and one clockevent is
> global and the other is local to a particular CPU we should
> choose to use the local clockevent regardless of the rating of
> the device. This way, if the clockevent is a dummy it will take
> the tick device duty as long as there isn't a higher rated tick
> device and any global clockevent will be bumped out into
> broadcast mode, fixing the problem described above.

I think I had that patch in my tree when I noticed the breakage, since my own
tmu patch dates from February 2014. I'll retest it though and will let you
know.

> >> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> >> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted
> >> to userspace and tested timer broadcast on all CPUs ?
> >
> > No I have not. I've booted to user space in initramfs with DT-based
> > TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
> > TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
> > broken on r8a7779 - both legacy and non-legacy. I have not gotten to
> > sh73a0 yet, but I assume it is busted too.
> >
> > Can you please explain to me how the TMU is any different compared to
> > the CMT, MTU2 or STI? =)
> >
> > And no, I don't have any r8a7740 board anymore. Can anyone else test?

--
Regards,

Laurent Pinchart

2014-12-17 01:30:36

by Magnus Damm

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Geert,

On Tue, Dec 16, 2014 at 9:40 PM, Geert Uytterhoeven
<[email protected]> wrote:
> Hi Magnus,
>
> On Tue, Dec 16, 2014 at 12:46 PM, Magnus Damm <[email protected]> wrote:
>>> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
>>> CONFIG_PREEMPT with and without the ARM TWD times, and that you've booted to
>>> userspace and tested timer broadcast on all CPUs ?
>>
>> No I have not. I've booted to user space in initramfs with DT-based
>> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
>> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
>> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
>> sh73a0 yet, but I assume it is busted too.
>
> FWIW, I applied this patch, and booted the result successfully and without
> any user-visible changes on:
> - koelsch
> - armadillo-multiplatform
> - kzm9g-legacy
> - kzm9g-multiplatform

Thanks for testing - now let us wait and see what Laurent says.

> Armadillo-legacy is broken, and probably won't come back (cfr.
> https://lkml.org/lkml/2014/12/2/339).
>
> Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
> at some point in the past).

When the time allows I'd like us to try to fix up these somehow. At
least I'll give it a go - I guess Armadillo is difficult without any
board.

> Note that my local tree is based on renesas-drivers-2014-12-08-v3.18,
> renesas-devel-20141212-v3.18, and yesterday's upstream, with CCF,
> PM domain, TWD, and lots of WIP patches from the kitchen sink applied.

The TWD portion above - is it driver code or integration stuff? If the
latter - is there any reason why these can't be picked up by Simon and
merged right away.

My plan is to dig into sh73a0 (maybe including TWD) later today.

Thanks for your help!

/ magnus

2014-12-17 02:08:56

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus and Geert,

On Wednesday 17 December 2014 10:30:33 Magnus Damm wrote:
> On Tue, Dec 16, 2014 at 9:40 PM, Geert Uytterhoeven wrote:
> > On Tue, Dec 16, 2014 at 12:46 PM, Magnus Damm wrote:
> >>> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
> >>> CONFIG_PREEMPT with and without the ARM TWD times, and that you've
> >>> booted to userspace and tested timer broadcast on all CPUs ?
> >>
> >> No I have not. I've booted to user space in initramfs with DT-based
> >> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
> >> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
> >> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
> >> sh73a0 yet, but I assume it is busted too.
> >
> > FWIW, I applied this patch, and booted the result successfully and without
> >
> > any user-visible changes on:
> > - koelsch
> > - armadillo-multiplatform
> > - kzm9g-legacy
> > - kzm9g-multiplatform
>
> Thanks for testing - now let us wait and see what Laurent says.
>
> > Armadillo-legacy is broken, and probably won't come back (cfr.
> > https://lkml.org/lkml/2014/12/2/339).
> >
> > Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
> > at some point in the past).

kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch with
CONFIG_PREEMPT_NONE but fails to boot with CONFIG_PREEMPT. The platform
doesn't instantiate the TMU at the moment anyway, so it's unrelated to this
patch.

Given that kzm9g-legacy works for me regardless of the preempt configuration
and on the TMU cpumask, that kzm9g-reference doesn't use TMU, and that you've
tested kzm9g-multiplaform successfully, I think we can consider that this
patch is safe from a kzm9g point of view.

(By the way, how have you tested kzm9g-multiplatform given that none of
renesas-drivers-2014-12-08-v3.18, renesas-devel-20141212-v3.18 or today's
upstream support multiplatform kernels for sh73a0 ?)

Magnus, you have told me that you've performed tests on Marzen with
CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
perform the same tests with CONFIG_PREEMPT instead without noticing any
regression I think we can merge your patch. Whatever breakage it has caused in
the past is likely hidden somewhere beyond eyesight.

> When the time allows I'd like us to try to fix up these somehow. At
> least I'll give it a go - I guess Armadillo is difficult without any
> board.

I'd like that very much. Let's get rid of r8a73a4 legacy (Ulrich's patches
should be ready) first, and possible sh73a0 and r8a7740 as well, and then
retest our various timers configurations. We should test both CONFIG_PREEMPT
and CONFIG_PREEMPT none, with all combination of the CMT, TMU, MTU2, TWD and
ARM architected timer enabled.

> > Note that my local tree is based on renesas-drivers-2014-12-08-v3.18,
> > renesas-devel-20141212-v3.18, and yesterday's upstream, with CCF,
> > PM domain, TWD, and lots of WIP patches from the kitchen sink applied.
>
> The TWD portion above - is it driver code or integration stuff? If the
> latter - is there any reason why these can't be picked up by Simon and
> merged right away.
>
> My plan is to dig into sh73a0 (maybe including TWD) later today.

--
Regards,

Laurent Pinchart

2014-12-17 08:25:23

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus,

On Wed, Dec 17, 2014 at 2:30 AM, Magnus Damm <[email protected]> wrote:
>> Armadillo-legacy is broken, and probably won't come back (cfr.
>> https://lkml.org/lkml/2014/12/2/339).
>>
>> Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
>> at some point in the past).
>
> When the time allows I'd like us to try to fix up these somehow. At
> least I'll give it a go - I guess Armadillo is difficult without any
> board.

To fix Armadillo, we have to instantiate the GIC from C code, which is
a step backwards.

>> Note that my local tree is based on renesas-drivers-2014-12-08-v3.18,
>> renesas-devel-20141212-v3.18, and yesterday's upstream, with CCF,
>> PM domain, TWD, and lots of WIP patches from the kitchen sink applied.
>
> The TWD portion above - is it driver code or integration stuff? If the
> latter - is there any reason why these can't be picked up by Simon and
> merged right away.

Integration stuff. Basically it's
http://www.spinics.net/lists/arm-kernel/msg383413.html

In the mean time, I found the right clock, but I still have to repost that
(sorry, recovering from being ill --- merge windows don't help much ;-)

> My plan is to dig into sh73a0 (maybe including TWD) later today.

Okay, thanks!

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2014-12-17 08:31:01

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent,

On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart
<[email protected]> wrote:
>> > Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
>> > at some point in the past).
>
> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch with

OK.

I had expected the breakage to be something in my tree, either an issue in
a -next branch I'm using, an interaction with the CCF patches or so, or a
config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD is
not in DT).

> (By the way, how have you tested kzm9g-multiplatform given that none of
> renesas-drivers-2014-12-08-v3.18, renesas-devel-20141212-v3.18 or today's
> upstream support multiplatform kernels for sh73a0 ?)

As I said, my local tree contains lots of extra patches (erhm... 233).

> Magnus, you have told me that you've performed tests on Marzen with
> CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
> perform the same tests with CONFIG_PREEMPT instead without noticing any
> regression I think we can merge your patch. Whatever breakage it has caused in
> the past is likely hidden somewhere beyond eyesight.
>
>> When the time allows I'd like us to try to fix up these somehow. At
>> least I'll give it a go - I guess Armadillo is difficult without any
>> board.

I do have an Armadillo.

> I'd like that very much. Let's get rid of r8a73a4 legacy (Ulrich's patches
> should be ready) first, and possible sh73a0 and r8a7740 as well, and then
> retest our various timers configurations. We should test both CONFIG_PREEMPT
> and CONFIG_PREEMPT none, with all combination of the CMT, TMU, MTU2, TWD and
> ARM architected timer enabled.

Yes, getting rid of more legacy is good. It's just a pity for the Penguin on the
Armadillo's LCD, which doesn't want to visit armadillo-multiplatform.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2014-12-17 09:42:56

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent, Magnus,

On Wed, Dec 17, 2014 at 9:30 AM, Geert Uytterhoeven
<[email protected]> wrote:
> On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart
> <[email protected]> wrote:
>>> > Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
>>> > at some point in the past).
>>
>> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch with
>
> OK.
>
> I had expected the breakage to be something in my tree, either an issue in
> a -next branch I'm using, an interaction with the CCF patches or so, or a
> config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD is
> not in DT).

Kzm9g-reference hangs at "Calibrating local timer..." because I added the
TWD to sh73a0.dtsi (http://www.spinics.net/lists/arm-kernel/msg383413.html).

As twd_get_clock() does

if (np)
twd_clk = of_clk_get(np, 0);
else
twd_clk = clk_get_sys("smp_twd", NULL);

it's not DT-without-CCF proof, and kzm9g-reference cannot get its clock :-(

So the TWD node should be in sh73a0-kzm9g-multiplatform.dts, and we need
more dts files instead of less...

Note that I added the TWD node to DT to fix a hang on kzm9g-multiplatform
with CONFIG_CPU_IDLE=y (recently introduced "regression" in core code) after:

DMA: preallocated 256 KiB pool for atomic coherent allocations

On kzm9g-legacy the TWD is instantiated from C board code
(machine_desc.init_time = sh73a0_earlytimer_init), so CONFIG_CPU_IDLE=y
does not hang. Despite https://lkml.org/lkml/2014/12/2/339.

On kzm9g-reference, the TWD is not instantiated, causing the same hang.
If I instantiate the TWD from C board code there, it fails with

twd: can't register interrupt 29 (-22)

which looks like a symptom of https://lkml.org/lkml/2014/12/2/339

So no cookies for users of several -legacy and -reference platforms in v3.19-rc1
this Monday...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2014-12-17 11:31:11

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Geert,

On Wednesday 17 December 2014 09:30:57 Geert Uytterhoeven wrote:
> On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart wrote:
> >> > Kzm9g-reference still hangs at "Calibrating local timer..." (it did
> >> > work at some point in the past).
> >
> > kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch
> > with
>
> OK.
>
> I had expected the breakage to be something in my tree, either an issue in
> a -next branch I'm using, an interaction with the CCF patches or so, or a
> config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD is
> not in DT).
>
> > (By the way, how have you tested kzm9g-multiplatform given that none of
> > renesas-drivers-2014-12-08-v3.18, renesas-devel-20141212-v3.18 or today's
> > upstream support multiplatform kernels for sh73a0 ?)
>
> As I said, my local tree contains lots of extra patches (erhm... 233).

We should probably synchronize at some point and get pending patches merged,
or you'll loose sanity :-)

What multiplatform support patches do you plan to submit for v3.20 ?

> > Magnus, you have told me that you've performed tests on Marzen with
> > CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
> > perform the same tests with CONFIG_PREEMPT instead without noticing any
> > regression I think we can merge your patch. Whatever breakage it has
> > caused in the past is likely hidden somewhere beyond eyesight.
> >
> >> When the time allows I'd like us to try to fix up these somehow. At
> >> least I'll give it a go - I guess Armadillo is difficult without any
> >> board.
>
> I do have an Armadillo.
>
> > I'd like that very much. Let's get rid of r8a73a4 legacy (Ulrich's patches
> > should be ready) first, and possible sh73a0 and r8a7740 as well, and then
> > retest our various timers configurations. We should test both
> > CONFIG_PREEMPT and CONFIG_PREEMPT none, with all combination of the CMT,
> > TMU, MTU2, TWD and ARM architected timer enabled.
>
> Yes, getting rid of more legacy is good. It's just a pity for the Penguin on
> the Armadillo's LCD, which doesn't want to visit armadillo-multiplatform.

--
Regards,

Laurent Pinchart

2014-12-17 12:04:53

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Geert,

On Wednesday 17 December 2014 10:42:52 Geert Uytterhoeven wrote:
> On Wed, Dec 17, 2014 at 9:30 AM, Geert Uytterhoeven wrote:
> > On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart wrote:
> >>>> Kzm9g-reference still hangs at "Calibrating local timer..." (it did
> >>>> work at some point in the past).
> >>
> >> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch
> >> with
> >>
> > OK.
> >
> > I had expected the breakage to be something in my tree, either an issue in
> > a -next branch I'm using, an interaction with the CCF patches or so, or a
> > config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD is
> > not in DT).
>
> Kzm9g-reference hangs at "Calibrating local timer..." because I added the
> TWD to sh73a0.dtsi (http://www.spinics.net/lists/arm-kernel/msg383413.html).
>
> As twd_get_clock() does
>
> if (np)
> twd_clk = of_clk_get(np, 0);
> else
> twd_clk = clk_get_sys("smp_twd", NULL);
>
> it's not DT-without-CCF proof, and kzm9g-reference cannot get its clock :-(
>
> So the TWD node should be in sh73a0-kzm9g-multiplatform.dts, and we need
> more dts files instead of less...
>
> Note that I added the TWD node to DT to fix a hang on kzm9g-multiplatform
> with CONFIG_CPU_IDLE=y (recently introduced "regression" in core code)
> after:
>
> DMA: preallocated 256 KiB pool for atomic coherent allocations
>
> On kzm9g-legacy the TWD is instantiated from C board code
> (machine_desc.init_time = sh73a0_earlytimer_init), so CONFIG_CPU_IDLE=y
> does not hang. Despite https://lkml.org/lkml/2014/12/2/339.
>
> On kzm9g-reference, the TWD is not instantiated, causing the same hang.
> If I instantiate the TWD from C board code there, it fails with
>
> twd: can't register interrupt 29 (-22)
>
> which looks like a symptom of https://lkml.org/lkml/2014/12/2/339
>
> So no cookies for users of several -legacy and -reference platforms in
> v3.19-rc1 this Monday...

I'm not sure if fixing that would be worth it. Should we instead try to
replace kzm9g-legacy and kzm9g-reference by kzm9g-multiplatform in v3.20 ?
What are we missing ?

- The DIV6 multiparent series has been merged by Mike, and the sh73a0 CCF is
nearly ready.

- We need proper BSC support to get LAN working, although it could be worked
around temporarily by specifying the BSC clock in the LAN node if I recall
correctly.

- MMCIF and SDHI are present.

- Sound support is there as far as I can tell, but possibly without DMA.

- Touchscreen and magnetometer have DT bindings but are not instantiated in
the dts at the moment. That should be easy to fix.

- Accelerometer and RTC have no DT bindings, but they should work with the I2C
core OF match support.

- We're loosing USBHS (driver supports OF but doesn't cover sh73a0), USB host
(no OF support in driver) and LCDC (no OF support in driver).

All but USBHS, USB host and LCDC should be possible to support in v3.20
without too much issues, depending on the effort we can put in this.

--
Regards,

Laurent Pinchart

2014-12-17 12:11:10

by Geert Uytterhoeven

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent,

On Wed, Dec 17, 2014 at 1:04 PM, Laurent Pinchart
<[email protected]> wrote:
> On Wednesday 17 December 2014 10:42:52 Geert Uytterhoeven wrote:
>> On Wed, Dec 17, 2014 at 9:30 AM, Geert Uytterhoeven wrote:
>> > On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart wrote:
>> >>>> Kzm9g-reference still hangs at "Calibrating local timer..." (it did
>> >>>> work at some point in the past).
>> >>
>> >> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch
>> >> with
>> >>
>> > OK.
>> >
>> > I had expected the breakage to be something in my tree, either an issue in
>> > a -next branch I'm using, an interaction with the CCF patches or so, or a
>> > config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD is
>> > not in DT).
>>
>> Kzm9g-reference hangs at "Calibrating local timer..." because I added the
>> TWD to sh73a0.dtsi (http://www.spinics.net/lists/arm-kernel/msg383413.html).
>>
>> As twd_get_clock() does
>>
>> if (np)
>> twd_clk = of_clk_get(np, 0);
>> else
>> twd_clk = clk_get_sys("smp_twd", NULL);
>>
>> it's not DT-without-CCF proof, and kzm9g-reference cannot get its clock :-(
>>
>> So the TWD node should be in sh73a0-kzm9g-multiplatform.dts, and we need
>> more dts files instead of less...
>>
>> Note that I added the TWD node to DT to fix a hang on kzm9g-multiplatform
>> with CONFIG_CPU_IDLE=y (recently introduced "regression" in core code)
>> after:
>>
>> DMA: preallocated 256 KiB pool for atomic coherent allocations
>>
>> On kzm9g-legacy the TWD is instantiated from C board code
>> (machine_desc.init_time = sh73a0_earlytimer_init), so CONFIG_CPU_IDLE=y
>> does not hang. Despite https://lkml.org/lkml/2014/12/2/339.
>>
>> On kzm9g-reference, the TWD is not instantiated, causing the same hang.
>> If I instantiate the TWD from C board code there, it fails with
>>
>> twd: can't register interrupt 29 (-22)
>>
>> which looks like a symptom of https://lkml.org/lkml/2014/12/2/339
>>
>> So no cookies for users of several -legacy and -reference platforms in
>> v3.19-rc1 this Monday...
>
> I'm not sure if fixing that would be worth it. Should we instead try to
> replace kzm9g-legacy and kzm9g-reference by kzm9g-multiplatform in v3.20 ?

v3.20 is reasonable. Except that some breakage will arrive in v3.19-rc1.
So there will be a brokenness window for the kzm9g platform between
v3.18 and v3.20.

> What are we missing ?
>
> - The DIV6 multiparent series has been merged by Mike, and the sh73a0 CCF is
> nearly ready.

Yep.

> - We need proper BSC support to get LAN working, although it could be worked
> around temporarily by specifying the BSC clock in the LAN node if I recall
> correctly.

Working on that. I hope to send out v2 later today...

> - Accelerometer and RTC have no DT bindings, but they should work with the I2C
> core OF match support.

I have RTC in my DTS, and it works.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

2014-12-17 12:14:42

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Geert,

On Wednesday 17 December 2014 13:11:05 Geert Uytterhoeven wrote:
> On Wed, Dec 17, 2014 at 1:04 PM, Laurent Pinchart wrote:
> > On Wednesday 17 December 2014 10:42:52 Geert Uytterhoeven wrote:
> >> On Wed, Dec 17, 2014 at 9:30 AM, Geert Uytterhoeven wrote:
> >> > On Wed, Dec 17, 2014 at 3:08 AM, Laurent Pinchart wrote:
> >> >>>> Kzm9g-reference still hangs at "Calibrating local timer..." (it did
> >> >>>> work at some point in the past).
> >> >>
> >> >> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel
> >> >> branch with
> >> >
> >> > OK.
> >> >
> >> > I had expected the breakage to be something in my tree, either an issue
> >> > in a -next branch I'm using, an interaction with the CCF patches or so,
> >> > or a config issue (e.g. CONFIG_CPU_IDLE became broken lately if the TWD
> >> > is not in DT).
> >>
> >> Kzm9g-reference hangs at "Calibrating local timer..." because I added the
> >> TWD to sh73a0.dtsi
> >> (http://www.spinics.net/lists/arm-kernel/msg383413.html).
> >>
> >> As twd_get_clock() does
> >>
> >> if (np)
> >> twd_clk = of_clk_get(np, 0);
> >> else
> >> twd_clk = clk_get_sys("smp_twd", NULL);
> >>
> >> it's not DT-without-CCF proof, and kzm9g-reference cannot get its clock
> >> :-(
> >>
> >> So the TWD node should be in sh73a0-kzm9g-multiplatform.dts, and we need
> >> more dts files instead of less...
> >>
> >> Note that I added the TWD node to DT to fix a hang on kzm9g-multiplatform
> >> with CONFIG_CPU_IDLE=y (recently introduced "regression" in core code)
> >> after:
> >>
> >> DMA: preallocated 256 KiB pool for atomic coherent allocations
> >>
> >> On kzm9g-legacy the TWD is instantiated from C board code
> >> (machine_desc.init_time = sh73a0_earlytimer_init), so CONFIG_CPU_IDLE=y
> >> does not hang. Despite https://lkml.org/lkml/2014/12/2/339.
> >>
> >> On kzm9g-reference, the TWD is not instantiated, causing the same hang.
> >> If I instantiate the TWD from C board code there, it fails with
> >>
> >> twd: can't register interrupt 29 (-22)
> >>
> >> which looks like a symptom of https://lkml.org/lkml/2014/12/2/339
> >>
> >> So no cookies for users of several -legacy and -reference platforms in
> >> v3.19-rc1 this Monday...
> >
> > I'm not sure if fixing that would be worth it. Should we instead try to
> > replace kzm9g-legacy and kzm9g-reference by kzm9g-multiplatform in v3.20 ?
>
> v3.20 is reasonable. Except that some breakage will arrive in v3.19-rc1.
> So there will be a brokenness window for the kzm9g platform between
> v3.18 and v3.20.

I know, and that's unfortunate, but I wonder if we should really spend time on
fixing that if we can get multiplatform support merged in v3.20. We could
consider multiplatform to be a regression fix and get it merged in v3.19, but
that might be a bit far-fetched :-)

> > What are we missing ?
> >
> > - The DIV6 multiparent series has been merged by Mike, and the sh73a0 CCF
> > is nearly ready.
>
> Yep.
>
> > - We need proper BSC support to get LAN working, although it could be
> > worked around temporarily by specifying the BSC clock in the LAN node if
> > I recall correctly.
>
> Working on that. I hope to send out v2 later today...
>
> > - Accelerometer and RTC have no DT bindings, but they should work with the
> > I2C core OF match support.
>
> I have RTC in my DTS, and it works.

Great.

I can help with at least some of the remaining pieces if you share your
working branch.

--
Regards,

Laurent Pinchart

2014-12-17 13:23:18

by Magnus Damm

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent,

On Wed, Dec 17, 2014 at 11:08 AM, Laurent Pinchart
<[email protected]> wrote:
> Hi Magnus and Geert,
>
> On Wednesday 17 December 2014 10:30:33 Magnus Damm wrote:
>> On Tue, Dec 16, 2014 at 9:40 PM, Geert Uytterhoeven wrote:
>> > On Tue, Dec 16, 2014 at 12:46 PM, Magnus Damm wrote:
>> >>> Could you please confirm that you've tested both CONFIG_PREEMPT_NONE and
>> >>> CONFIG_PREEMPT with and without the ARM TWD times, and that you've
>> >>> booted to userspace and tested timer broadcast on all CPUs ?
>> >>
>> >> No I have not. I've booted to user space in initramfs with DT-based
>> >> TWD on Multiplatform for r8a7779. Without this fix (and other r8a7779
>> >> TWD bits) I see a lot of breakage. For instance, TWD and SMP boot is
>> >> broken on r8a7779 - both legacy and non-legacy. I have not gotten to
>> >> sh73a0 yet, but I assume it is busted too.
>> >
>> > FWIW, I applied this patch, and booted the result successfully and without
>> >
>> > any user-visible changes on:
>> > - koelsch
>> > - armadillo-multiplatform
>> > - kzm9g-legacy
>> > - kzm9g-multiplatform
>>
>> Thanks for testing - now let us wait and see what Laurent says.
>>
>> > Armadillo-legacy is broken, and probably won't come back (cfr.
>> > https://lkml.org/lkml/2014/12/2/339).
>> >
>> > Kzm9g-reference still hangs at "Calibrating local timer..." (it did work
>> > at some point in the past).
>
> kzm9g-reference boots for me with kzm9g_defconfig on Simon's devel branch with
> CONFIG_PREEMPT_NONE but fails to boot with CONFIG_PREEMPT. The platform
> doesn't instantiate the TMU at the moment anyway, so it's unrelated to this
> patch.

Ok, thanks for testing. That makes sense.

> Given that kzm9g-legacy works for me regardless of the preempt configuration
> and on the TMU cpumask, that kzm9g-reference doesn't use TMU, and that you've
> tested kzm9g-multiplaform successfully, I think we can consider that this
> patch is safe from a kzm9g point of view.

I think so too.

> (By the way, how have you tested kzm9g-multiplatform given that none of
> renesas-drivers-2014-12-08-v3.18, renesas-devel-20141212-v3.18 or today's
> upstream support multiplatform kernels for sh73a0 ?)

My recently posted patch set:
[PATCH v2 00/05] ARM: shmobile: sh73a0 and kzm9g Multiplatform revisit

> Magnus, you have told me that you've performed tests on Marzen with
> CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
> perform the same tests with CONFIG_PREEMPT instead without noticing any
> regression I think we can merge your patch. Whatever breakage it has caused in
> the past is likely hidden somewhere beyond eyesight.

Sorry, but I have not had any time to test this today. What I can tell
from yesterday is that legacy Marzen seemed busted and without my
patch(es) (including this one) multiplatform Marzen was busted as
well.

I'll most likely be buried in paper work tomorrow, but I'll see if I
can find time dealing with Marzen legacy tomorrow. In general I find
legacy upkeep rather time consuming since it seems to degrade rather
quickly, so I much rather spend time fixing up multiplatform.

>> When the time allows I'd like us to try to fix up these somehow. At
>> least I'll give it a go - I guess Armadillo is difficult without any
>> board.
>
> I'd like that very much. Let's get rid of r8a73a4 legacy (Ulrich's patches
> should be ready) first, and possible sh73a0 and r8a7740 as well, and then
> retest our various timers configurations. We should test both CONFIG_PREEMPT
> and CONFIG_PREEMPT none, with all combination of the CMT, TMU, MTU2, TWD and
> ARM architected timer enabled.

Sure, those are the ones I usually care about. From my side I find the
TWD and arch timer upkeep to be heavy enough as-is without considering
the preempt case. But you're right - we should cover all of them.

Thanks,

/ magnus

2014-12-19 00:03:04

by Magnus Damm

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Laurent,

On Wed, Dec 17, 2014 at 11:08 AM, Laurent Pinchart
<[email protected]> wrote:
> Magnus, you have told me that you've performed tests on Marzen with
> CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
> perform the same tests with CONFIG_PREEMPT instead without noticing any
> regression I think we can merge your patch. Whatever breakage it has caused in
> the past is likely hidden somewhere beyond eyesight.

I've now successfully boot tested the different PREEMPT settings on
r8a7779 Marzen multiplatform. Everything seems to work as expected.

Compared to before I've been a bit more lazy, so I've kept the timer
config to default which means TWD and TMU enabled. The kernel tree is
renesas-devel-20141218-v3.18 which is v3.18 plus local integration
stuff including TWD DT. I've applied this patch too.

Thanks,

/ magnus

2014-12-19 00:46:36

by Laurent Pinchart

[permalink] [raw]
Subject: Re: [PATCH] clocksource: sh_tmu: Set cpu_possible_mask to fix SMP broadcast

Hi Magnus,

On Friday 19 December 2014 09:03:00 Magnus Damm wrote:
> On Wed, Dec 17, 2014 at 11:08 AM, Laurent Pinchart wrote:
> > Magnus, you have told me that you've performed tests on Marzen with
> > CONFIG_PREEMPT_NONE and with both TWD enabled and disabled. If you could
> > perform the same tests with CONFIG_PREEMPT instead without noticing any
> > regression I think we can merge your patch. Whatever breakage it has
> > caused in the past is likely hidden somewhere beyond eyesight.
>
> I've now successfully boot tested the different PREEMPT settings on
> r8a7779 Marzen multiplatform. Everything seems to work as expected.
>
> Compared to before I've been a bit more lazy, so I've kept the timer
> config to default which means TWD and TMU enabled.

If your test setup is still around could you quickly test PREEMPT + TMU
without TWD ?

> The kernel tree is renesas-devel-20141218-v3.18 which is v3.18 plus local
> integration stuff including TWD DT. I've applied this patch too.

--
Regards,

Laurent Pinchart