| Message ID | 20210608154341.10794-2-will@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Use Exynos-MCT timer as wakeup for Arm arch timer | expand |
On 08/06/2021 17:43, Will Deacon wrote: > All arm64 CPUs feature an architected timer, which offers a relatively > low-latency interface to a per-cpu clocksource and timer. For the most > part, using this interface is a no-brainer, with the exception of SoCs > where it cannot be used to wake up from deep idle state (i.e. > CLOCK_EVT_FEAT_C3STOP is set). > > On the contrary, the Exynos MCT is extremely slow to access yet can be > used as a wakeup source. In preparation for using the Exynos MCT as a > potential wakeup timer for the Arm architected timer, reduce its ratings > so that the architected timer is preferred. > > This effectively reverts the decision made in 6282edb72bed > ("clocksource/drivers/exynos_mct: Increase priority over ARM arch timer") > for arm64, as the reasoning for the original change was to work around > a 32-bit SoC design. > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Will Deacon <will@kernel.org> > --- > drivers/clocksource/exynos_mct.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index fabad79baafc..804d3e01c8f4 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -51,6 +51,15 @@ > > #define TICK_BASE_CNT 1 > > +#ifdef CONFIG_ARM > +/* Use values higher than ARM arch timer. See 6282edb72bed. */ > +#define MCT_CLKSOURCE_RATING 450 > +#define MCT_CLKEVENTS_RATING 500 > +#else > +#define MCT_CLKSOURCE_RATING 350 > +#define MCT_CLKEVENTS_RATING 350 > +#endif > + Tested on Exynos5422 (32-bit ARM) and it seems to work. I don't have ARMv8 board to check it. We want Exynos MCT on ARMv8 as well, because unfortunately it shares the block with Arch timer. This was investigated by Marek: https://lore.kernel.org/linux-samsung-soc/20181017123622eucas1p14654c89a8590fd094d638b60ab9af8f0~eZY3j27rs0422004220eucas1p1M@eucas1p1.samsung.com/ Best regards, Krzysztof
Hi, On 6/9/21 12:43 AM, Will Deacon wrote: > All arm64 CPUs feature an architected timer, which offers a relatively > low-latency interface to a per-cpu clocksource and timer. For the most > part, using this interface is a no-brainer, with the exception of SoCs > where it cannot be used to wake up from deep idle state (i.e. > CLOCK_EVT_FEAT_C3STOP is set). > > On the contrary, the Exynos MCT is extremely slow to access yet can be > used as a wakeup source. In preparation for using the Exynos MCT as a > potential wakeup timer for the Arm architected timer, reduce its ratings > so that the architected timer is preferred. > > This effectively reverts the decision made in 6282edb72bed > ("clocksource/drivers/exynos_mct: Increase priority over ARM arch timer") > for arm64, as the reasoning for the original change was to work around > a 32-bit SoC design. > > Cc: Marek Szyprowski <m.szyprowski@samsung.com> > Cc: Krzysztof Kozlowski <krzk@kernel.org> > Cc: Chanwoo Choi <cw00.choi@samsung.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Signed-off-by: Will Deacon <will@kernel.org> > --- > drivers/clocksource/exynos_mct.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > index fabad79baafc..804d3e01c8f4 100644 > --- a/drivers/clocksource/exynos_mct.c > +++ b/drivers/clocksource/exynos_mct.c > @@ -51,6 +51,15 @@ > > #define TICK_BASE_CNT 1 > > +#ifdef CONFIG_ARM > +/* Use values higher than ARM arch timer. See 6282edb72bed. */ > +#define MCT_CLKSOURCE_RATING 450 > +#define MCT_CLKEVENTS_RATING 500 > +#else > +#define MCT_CLKSOURCE_RATING 350 > +#define MCT_CLKEVENTS_RATING 350 > +#endif > + > enum { > MCT_INT_SPI, > MCT_INT_PPI > @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) > > static struct clocksource mct_frc = { > .name = "mct-frc", > - .rating = 450, /* use value higher than ARM arch timer */ > + .rating = MCT_CLKSOURCE_RATING, > .read = exynos4_frc_read, > .mask = CLOCKSOURCE_MASK(32), > .flags = CLOCK_SOURCE_IS_CONTINUOUS, > @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) > evt->set_state_oneshot_stopped = set_state_shutdown; > evt->tick_resume = set_state_shutdown; > evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > - evt->rating = 500; /* use value higher than ARM arch timer */ > + evt->rating = MCT_CLKEVENTS_RATING, > > exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); > > I'm not sure that exynos mct is working without problem such as the case of 6282edb72bed. As described on On ,6282edb72bed the arch timer on exynos SoC depends on Exynos MCT device. the arch timer is not able to work without Exynos MCT because of using the common module.
On 10/06/2021 03:03, Chanwoo Choi wrote: > Hi, > > On 6/9/21 12:43 AM, Will Deacon wrote: >> All arm64 CPUs feature an architected timer, which offers a relatively >> low-latency interface to a per-cpu clocksource and timer. For the most >> part, using this interface is a no-brainer, with the exception of SoCs >> where it cannot be used to wake up from deep idle state (i.e. >> CLOCK_EVT_FEAT_C3STOP is set). >> >> On the contrary, the Exynos MCT is extremely slow to access yet can be >> used as a wakeup source. In preparation for using the Exynos MCT as a >> potential wakeup timer for the Arm architected timer, reduce its ratings >> so that the architected timer is preferred. >> >> This effectively reverts the decision made in 6282edb72bed >> ("clocksource/drivers/exynos_mct: Increase priority over ARM arch timer") >> for arm64, as the reasoning for the original change was to work around >> a 32-bit SoC design. >> >> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >> Cc: Krzysztof Kozlowski <krzk@kernel.org> >> Cc: Chanwoo Choi <cw00.choi@samsung.com> >> Cc: Thomas Gleixner <tglx@linutronix.de> >> Signed-off-by: Will Deacon <will@kernel.org> >> --- >> drivers/clocksource/exynos_mct.c | 13 +++++++++++-- >> 1 file changed, 11 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >> index fabad79baafc..804d3e01c8f4 100644 >> --- a/drivers/clocksource/exynos_mct.c >> +++ b/drivers/clocksource/exynos_mct.c >> @@ -51,6 +51,15 @@ >> >> #define TICK_BASE_CNT 1 >> >> +#ifdef CONFIG_ARM >> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ >> +#define MCT_CLKSOURCE_RATING 450 >> +#define MCT_CLKEVENTS_RATING 500 >> +#else >> +#define MCT_CLKSOURCE_RATING 350 >> +#define MCT_CLKEVENTS_RATING 350 >> +#endif >> + >> enum { >> MCT_INT_SPI, >> MCT_INT_PPI >> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) >> >> static struct clocksource mct_frc = { >> .name = "mct-frc", >> - .rating = 450, /* use value higher than ARM arch timer */ >> + .rating = MCT_CLKSOURCE_RATING, >> .read = exynos4_frc_read, >> .mask = CLOCKSOURCE_MASK(32), >> .flags = CLOCK_SOURCE_IS_CONTINUOUS, >> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) >> evt->set_state_oneshot_stopped = set_state_shutdown; >> evt->tick_resume = set_state_shutdown; >> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; >> - evt->rating = 500; /* use value higher than ARM arch timer */ >> + evt->rating = MCT_CLKEVENTS_RATING, >> >> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); >> >> > > I'm not sure that exynos mct is working without problem > such as the case of 6282edb72bed. > As described on On ,6282edb72bed the arch timer on exynos SoC > depends on Exynos MCT device. the arch timer is not able to work > without Exynos MCT because of using the common module. Is it possible to change the DT to have a phandle to the exynos_mct, so it will be probed before the arch_arm_timer ?
On 6/17/21 12:25 AM, Daniel Lezcano wrote: > On 10/06/2021 03:03, Chanwoo Choi wrote: >> Hi, >> >> On 6/9/21 12:43 AM, Will Deacon wrote: >>> All arm64 CPUs feature an architected timer, which offers a relatively >>> low-latency interface to a per-cpu clocksource and timer. For the most >>> part, using this interface is a no-brainer, with the exception of SoCs >>> where it cannot be used to wake up from deep idle state (i.e. >>> CLOCK_EVT_FEAT_C3STOP is set). >>> >>> On the contrary, the Exynos MCT is extremely slow to access yet can be >>> used as a wakeup source. In preparation for using the Exynos MCT as a >>> potential wakeup timer for the Arm architected timer, reduce its ratings >>> so that the architected timer is preferred. >>> >>> This effectively reverts the decision made in 6282edb72bed >>> ("clocksource/drivers/exynos_mct: Increase priority over ARM arch timer") >>> for arm64, as the reasoning for the original change was to work around >>> a 32-bit SoC design. >>> >>> Cc: Marek Szyprowski <m.szyprowski@samsung.com> >>> Cc: Krzysztof Kozlowski <krzk@kernel.org> >>> Cc: Chanwoo Choi <cw00.choi@samsung.com> >>> Cc: Thomas Gleixner <tglx@linutronix.de> >>> Signed-off-by: Will Deacon <will@kernel.org> >>> --- >>> drivers/clocksource/exynos_mct.c | 13 +++++++++++-- >>> 1 file changed, 11 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >>> index fabad79baafc..804d3e01c8f4 100644 >>> --- a/drivers/clocksource/exynos_mct.c >>> +++ b/drivers/clocksource/exynos_mct.c >>> @@ -51,6 +51,15 @@ >>> >>> #define TICK_BASE_CNT 1 >>> >>> +#ifdef CONFIG_ARM >>> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ >>> +#define MCT_CLKSOURCE_RATING 450 >>> +#define MCT_CLKEVENTS_RATING 500 >>> +#else >>> +#define MCT_CLKSOURCE_RATING 350 >>> +#define MCT_CLKEVENTS_RATING 350 >>> +#endif >>> + >>> enum { >>> MCT_INT_SPI, >>> MCT_INT_PPI >>> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) >>> >>> static struct clocksource mct_frc = { >>> .name = "mct-frc", >>> - .rating = 450, /* use value higher than ARM arch timer */ >>> + .rating = MCT_CLKSOURCE_RATING, >>> .read = exynos4_frc_read, >>> .mask = CLOCKSOURCE_MASK(32), >>> .flags = CLOCK_SOURCE_IS_CONTINUOUS, >>> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) >>> evt->set_state_oneshot_stopped = set_state_shutdown; >>> evt->tick_resume = set_state_shutdown; >>> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; >>> - evt->rating = 500; /* use value higher than ARM arch timer */ >>> + evt->rating = MCT_CLKEVENTS_RATING, >>> >>> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); >>> >>> >> >> I'm not sure that exynos mct is working without problem >> such as the case of 6282edb72bed. >> As described on On ,6282edb72bed the arch timer on exynos SoC >> depends on Exynos MCT device. the arch timer is not able to work >> without Exynos MCT because of using the common module. > > Is it possible to change the DT to have a phandle to the exynos_mct, so > it will be probed before the arch_arm_timer ? I think that DT changes is not proper way to keep the order between exynos_mct and arch timer.
On Thu, Jun 17, 2021 at 09:58:35AM +0900, Chanwoo Choi wrote: > On 6/17/21 12:25 AM, Daniel Lezcano wrote: > > On 10/06/2021 03:03, Chanwoo Choi wrote: > >> On 6/9/21 12:43 AM, Will Deacon wrote: > >>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > >>> index fabad79baafc..804d3e01c8f4 100644 > >>> --- a/drivers/clocksource/exynos_mct.c > >>> +++ b/drivers/clocksource/exynos_mct.c > >>> @@ -51,6 +51,15 @@ > >>> > >>> #define TICK_BASE_CNT 1 > >>> > >>> +#ifdef CONFIG_ARM > >>> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ > >>> +#define MCT_CLKSOURCE_RATING 450 > >>> +#define MCT_CLKEVENTS_RATING 500 > >>> +#else > >>> +#define MCT_CLKSOURCE_RATING 350 > >>> +#define MCT_CLKEVENTS_RATING 350 > >>> +#endif > >>> + > >>> enum { > >>> MCT_INT_SPI, > >>> MCT_INT_PPI > >>> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) > >>> > >>> static struct clocksource mct_frc = { > >>> .name = "mct-frc", > >>> - .rating = 450, /* use value higher than ARM arch timer */ > >>> + .rating = MCT_CLKSOURCE_RATING, > >>> .read = exynos4_frc_read, > >>> .mask = CLOCKSOURCE_MASK(32), > >>> .flags = CLOCK_SOURCE_IS_CONTINUOUS, > >>> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) > >>> evt->set_state_oneshot_stopped = set_state_shutdown; > >>> evt->tick_resume = set_state_shutdown; > >>> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > >>> - evt->rating = 500; /* use value higher than ARM arch timer */ > >>> + evt->rating = MCT_CLKEVENTS_RATING, > >>> > >>> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); > >>> > >>> > >> > >> I'm not sure that exynos mct is working without problem > >> such as the case of 6282edb72bed. > >> As described on On ,6282edb72bed the arch timer on exynos SoC > >> depends on Exynos MCT device. the arch timer is not able to work > >> without Exynos MCT because of using the common module. > > > > Is it possible to change the DT to have a phandle to the exynos_mct, so > > it will be probed before the arch_arm_timer ? > > I think that DT changes is not proper way to keep the order between > exynos_mct and arch timer. exynos4_mct_frc_start() is called unconditionally from probe via exynos4_clocksource_init() so as long as the mct probes first, then the arch timer should work, no? The rating shouldn't affect that. Will
On 17/06/2021 23:47, Will Deacon wrote: > On Thu, Jun 17, 2021 at 09:58:35AM +0900, Chanwoo Choi wrote: >> On 6/17/21 12:25 AM, Daniel Lezcano wrote: >>> On 10/06/2021 03:03, Chanwoo Choi wrote: >>>> On 6/9/21 12:43 AM, Will Deacon wrote: >>>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >>>>> index fabad79baafc..804d3e01c8f4 100644 >>>>> --- a/drivers/clocksource/exynos_mct.c >>>>> +++ b/drivers/clocksource/exynos_mct.c >>>>> @@ -51,6 +51,15 @@ >>>>> >>>>> #define TICK_BASE_CNT 1 >>>>> >>>>> +#ifdef CONFIG_ARM >>>>> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ >>>>> +#define MCT_CLKSOURCE_RATING 450 >>>>> +#define MCT_CLKEVENTS_RATING 500 >>>>> +#else >>>>> +#define MCT_CLKSOURCE_RATING 350 >>>>> +#define MCT_CLKEVENTS_RATING 350 >>>>> +#endif >>>>> + >>>>> enum { >>>>> MCT_INT_SPI, >>>>> MCT_INT_PPI >>>>> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) >>>>> >>>>> static struct clocksource mct_frc = { >>>>> .name = "mct-frc", >>>>> - .rating = 450, /* use value higher than ARM arch timer */ >>>>> + .rating = MCT_CLKSOURCE_RATING, >>>>> .read = exynos4_frc_read, >>>>> .mask = CLOCKSOURCE_MASK(32), >>>>> .flags = CLOCK_SOURCE_IS_CONTINUOUS, >>>>> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) >>>>> evt->set_state_oneshot_stopped = set_state_shutdown; >>>>> evt->tick_resume = set_state_shutdown; >>>>> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; >>>>> - evt->rating = 500; /* use value higher than ARM arch timer */ >>>>> + evt->rating = MCT_CLKEVENTS_RATING, >>>>> >>>>> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); >>>>> >>>>> >>>> >>>> I'm not sure that exynos mct is working without problem >>>> such as the case of 6282edb72bed. >>>> As described on On ,6282edb72bed the arch timer on exynos SoC >>>> depends on Exynos MCT device. the arch timer is not able to work >>>> without Exynos MCT because of using the common module. >>> >>> Is it possible to change the DT to have a phandle to the exynos_mct, so >>> it will be probed before the arch_arm_timer ? >> >> I think that DT changes is not proper way to keep the order between >> exynos_mct and arch timer. > > exynos4_mct_frc_start() is called unconditionally from probe via > exynos4_clocksource_init() so as long as the mct probes first, then the > arch timer should work, no? The rating shouldn't affect that. How do you ensure the exynos mct is probed before the arch timer ? The Makefile provides the right order, but the dependency is implicit.
On Mon, Jun 21, 2021 at 11:25:47AM +0200, Daniel Lezcano wrote: > On 17/06/2021 23:47, Will Deacon wrote: > > On Thu, Jun 17, 2021 at 09:58:35AM +0900, Chanwoo Choi wrote: > >> On 6/17/21 12:25 AM, Daniel Lezcano wrote: > >>> On 10/06/2021 03:03, Chanwoo Choi wrote: > >>>> On 6/9/21 12:43 AM, Will Deacon wrote: > >>>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c > >>>>> index fabad79baafc..804d3e01c8f4 100644 > >>>>> --- a/drivers/clocksource/exynos_mct.c > >>>>> +++ b/drivers/clocksource/exynos_mct.c > >>>>> @@ -51,6 +51,15 @@ > >>>>> > >>>>> #define TICK_BASE_CNT 1 > >>>>> > >>>>> +#ifdef CONFIG_ARM > >>>>> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ > >>>>> +#define MCT_CLKSOURCE_RATING 450 > >>>>> +#define MCT_CLKEVENTS_RATING 500 > >>>>> +#else > >>>>> +#define MCT_CLKSOURCE_RATING 350 > >>>>> +#define MCT_CLKEVENTS_RATING 350 > >>>>> +#endif > >>>>> + > >>>>> enum { > >>>>> MCT_INT_SPI, > >>>>> MCT_INT_PPI > >>>>> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) > >>>>> > >>>>> static struct clocksource mct_frc = { > >>>>> .name = "mct-frc", > >>>>> - .rating = 450, /* use value higher than ARM arch timer */ > >>>>> + .rating = MCT_CLKSOURCE_RATING, > >>>>> .read = exynos4_frc_read, > >>>>> .mask = CLOCKSOURCE_MASK(32), > >>>>> .flags = CLOCK_SOURCE_IS_CONTINUOUS, > >>>>> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) > >>>>> evt->set_state_oneshot_stopped = set_state_shutdown; > >>>>> evt->tick_resume = set_state_shutdown; > >>>>> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; > >>>>> - evt->rating = 500; /* use value higher than ARM arch timer */ > >>>>> + evt->rating = MCT_CLKEVENTS_RATING, > >>>>> > >>>>> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); > >>>>> > >>>>> > >>>> > >>>> I'm not sure that exynos mct is working without problem > >>>> such as the case of 6282edb72bed. > >>>> As described on On ,6282edb72bed the arch timer on exynos SoC > >>>> depends on Exynos MCT device. the arch timer is not able to work > >>>> without Exynos MCT because of using the common module. > >>> > >>> Is it possible to change the DT to have a phandle to the exynos_mct, so > >>> it will be probed before the arch_arm_timer ? > >> > >> I think that DT changes is not proper way to keep the order between > >> exynos_mct and arch timer. > > > > exynos4_mct_frc_start() is called unconditionally from probe via > > exynos4_clocksource_init() so as long as the mct probes first, then the > > arch timer should work, no? The rating shouldn't affect that. > > How do you ensure the exynos mct is probed before the arch timer ? > > The Makefile provides the right order, but the dependency is implicit. Currently, I think it's done by the order of the CPU hotplug notifiers ( see the hunk of 6282edb72bed which touches cpuhotplug.h). Will
Hi Daniel, On 21.06.2021 11:25, Daniel Lezcano wrote: > On 17/06/2021 23:47, Will Deacon wrote: >> On Thu, Jun 17, 2021 at 09:58:35AM +0900, Chanwoo Choi wrote: >>> On 6/17/21 12:25 AM, Daniel Lezcano wrote: >>>> On 10/06/2021 03:03, Chanwoo Choi wrote: >>>>> On 6/9/21 12:43 AM, Will Deacon wrote: >>>>>> diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c >>>>>> index fabad79baafc..804d3e01c8f4 100644 >>>>>> --- a/drivers/clocksource/exynos_mct.c >>>>>> +++ b/drivers/clocksource/exynos_mct.c >>>>>> @@ -51,6 +51,15 @@ >>>>>> >>>>>> #define TICK_BASE_CNT 1 >>>>>> >>>>>> +#ifdef CONFIG_ARM >>>>>> +/* Use values higher than ARM arch timer. See 6282edb72bed. */ >>>>>> +#define MCT_CLKSOURCE_RATING 450 >>>>>> +#define MCT_CLKEVENTS_RATING 500 >>>>>> +#else >>>>>> +#define MCT_CLKSOURCE_RATING 350 >>>>>> +#define MCT_CLKEVENTS_RATING 350 >>>>>> +#endif >>>>>> + >>>>>> enum { >>>>>> MCT_INT_SPI, >>>>>> MCT_INT_PPI >>>>>> @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) >>>>>> >>>>>> static struct clocksource mct_frc = { >>>>>> .name = "mct-frc", >>>>>> - .rating = 450, /* use value higher than ARM arch timer */ >>>>>> + .rating = MCT_CLKSOURCE_RATING, >>>>>> .read = exynos4_frc_read, >>>>>> .mask = CLOCKSOURCE_MASK(32), >>>>>> .flags = CLOCK_SOURCE_IS_CONTINUOUS, >>>>>> @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) >>>>>> evt->set_state_oneshot_stopped = set_state_shutdown; >>>>>> evt->tick_resume = set_state_shutdown; >>>>>> evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; >>>>>> - evt->rating = 500; /* use value higher than ARM arch timer */ >>>>>> + evt->rating = MCT_CLKEVENTS_RATING, >>>>>> >>>>>> exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET); >>>>>> >>>>>> >>>>> I'm not sure that exynos mct is working without problem >>>>> such as the case of 6282edb72bed. >>>>> As described on On ,6282edb72bed the arch timer on exynos SoC >>>>> depends on Exynos MCT device. the arch timer is not able to work >>>>> without Exynos MCT because of using the common module. >>>> Is it possible to change the DT to have a phandle to the exynos_mct, so >>>> it will be probed before the arch_arm_timer ? >>> I think that DT changes is not proper way to keep the order between >>> exynos_mct and arch timer. >> exynos4_mct_frc_start() is called unconditionally from probe via >> exynos4_clocksource_init() so as long as the mct probes first, then the >> arch timer should work, no? The rating shouldn't affect that. > How do you ensure the exynos mct is probed before the arch timer ? > > The Makefile provides the right order, but the dependency is implicit. In my test systems based on Exynos5433, the bootloader enabled MCT, so kernel can also start with arch_timer then switch to MCT. The probe order also depend on the order of DT nodes, so with the current dts MCT is probed first. This is not the best approach, but so far it works. Best regards
On 21/06/2021 12:10, Will Deacon wrote: [ ... ] >>> exynos4_mct_frc_start() is called unconditionally from probe via >>> exynos4_clocksource_init() so as long as the mct probes first, then the >>> arch timer should work, no? The rating shouldn't affect that. >> >> How do you ensure the exynos mct is probed before the arch timer ? >> >> The Makefile provides the right order, but the dependency is implicit. > > Currently, I think it's done by the order of the CPU hotplug notifiers ( > see the hunk of 6282edb72bed which touches cpuhotplug.h). Ah, right. Indeed whatever the DT order, the cpuhotplug order solves the dependency. Chanwoo, are fine with this change ?
On 21/06/2021 12:17, Marek Szyprowski wrote: [ ... ] >>>>>> I'm not sure that exynos mct is working without problem >>>>>> such as the case of 6282edb72bed. >>>>>> As described on On ,6282edb72bed the arch timer on exynos SoC >>>>>> depends on Exynos MCT device. the arch timer is not able to work >>>>>> without Exynos MCT because of using the common module. >>>>> Is it possible to change the DT to have a phandle to the exynos_mct, so >>>>> it will be probed before the arch_arm_timer ? >>>> I think that DT changes is not proper way to keep the order between >>>> exynos_mct and arch timer. >>> exynos4_mct_frc_start() is called unconditionally from probe via >>> exynos4_clocksource_init() so as long as the mct probes first, then the >>> arch timer should work, no? The rating shouldn't affect that. >> How do you ensure the exynos mct is probed before the arch timer ? >> >> The Makefile provides the right order, but the dependency is implicit. > > In my test systems based on Exynos5433, the bootloader enabled MCT, so > kernel can also start with arch_timer then switch to MCT. The probe > order also depend on the order of DT nodes, so with the current dts MCT > is probed first. This is not the best approach, but so far it works. Right, generally assuming the firmware is doing the right thing from an initialization POV is not a good idea :/ But in our case, as pointed by Will, the cpuhotplug ordering gives the right sequence. So it should be fine.
On 6/21/21 7:18 PM, Daniel Lezcano wrote: > On 21/06/2021 12:10, Will Deacon wrote: > > [ ... ] > >>>> exynos4_mct_frc_start() is called unconditionally from probe via >>>> exynos4_clocksource_init() so as long as the mct probes first, then the >>>> arch timer should work, no? The rating shouldn't affect that. >>> >>> How do you ensure the exynos mct is probed before the arch timer ? >>> >>> The Makefile provides the right order, but the dependency is implicit. >> >> Currently, I think it's done by the order of the CPU hotplug notifiers ( >> see the hunk of 6282edb72bed which touches cpuhotplug.h). > > Ah, right. Indeed whatever the DT order, the cpuhotplug order solves the > dependency. > > Chanwoo, are fine with this change ? OK about the order. Actually, I have not fully tested the arch timer on Exynos5433 64bit because of the dependency between arch timer and MCT as we knew. If the Krzysztof and Marek have no any objection, I have no any objection anymore. Thanks.
On 22/06/2021 04:40, Chanwoo Choi wrote: > On 6/21/21 7:18 PM, Daniel Lezcano wrote: >> On 21/06/2021 12:10, Will Deacon wrote: >> >> [ ... ] >> >>>>> exynos4_mct_frc_start() is called unconditionally from probe via >>>>> exynos4_clocksource_init() so as long as the mct probes first, then the >>>>> arch timer should work, no? The rating shouldn't affect that. >>>> >>>> How do you ensure the exynos mct is probed before the arch timer ? >>>> >>>> The Makefile provides the right order, but the dependency is implicit. >>> >>> Currently, I think it's done by the order of the CPU hotplug notifiers ( >>> see the hunk of 6282edb72bed which touches cpuhotplug.h). >> >> Ah, right. Indeed whatever the DT order, the cpuhotplug order solves the >> dependency. >> >> Chanwoo, are fine with this change ? > > OK about the order. > > Actually, I have not fully tested the arch timer on Exynos5433 64bit > because of the dependency between arch timer and MCT as we knew. > > If the Krzysztof and Marek have no any objection, > I have no any objection anymore. Thanks. > Shall I consider it as an Acked-by ?
On 6/22/21 2:21 PM, Daniel Lezcano wrote: > On 22/06/2021 04:40, Chanwoo Choi wrote: >> On 6/21/21 7:18 PM, Daniel Lezcano wrote: >>> On 21/06/2021 12:10, Will Deacon wrote: >>> >>> [ ... ] >>> >>>>>> exynos4_mct_frc_start() is called unconditionally from probe via >>>>>> exynos4_clocksource_init() so as long as the mct probes first, then the >>>>>> arch timer should work, no? The rating shouldn't affect that. >>>>> >>>>> How do you ensure the exynos mct is probed before the arch timer ? >>>>> >>>>> The Makefile provides the right order, but the dependency is implicit. >>>> >>>> Currently, I think it's done by the order of the CPU hotplug notifiers ( >>>> see the hunk of 6282edb72bed which touches cpuhotplug.h). >>> >>> Ah, right. Indeed whatever the DT order, the cpuhotplug order solves the >>> dependency. >>> >>> Chanwoo, are fine with this change ? >> >> OK about the order. >> >> Actually, I have not fully tested the arch timer on Exynos5433 64bit >> because of the dependency between arch timer and MCT as we knew. >> >> If the Krzysztof and Marek have no any objection, >> I have no any objection anymore. Thanks. >> > > Shall I consider it as an Acked-by ? > Unfortunately, it is not acked. Just no objection. I'm not sure that all cases will be working when using arch timer because as I said, I have only used the exynos mct timer for all of cases.
diff --git a/drivers/clocksource/exynos_mct.c b/drivers/clocksource/exynos_mct.c index fabad79baafc..804d3e01c8f4 100644 --- a/drivers/clocksource/exynos_mct.c +++ b/drivers/clocksource/exynos_mct.c @@ -51,6 +51,15 @@ #define TICK_BASE_CNT 1 +#ifdef CONFIG_ARM +/* Use values higher than ARM arch timer. See 6282edb72bed. */ +#define MCT_CLKSOURCE_RATING 450 +#define MCT_CLKEVENTS_RATING 500 +#else +#define MCT_CLKSOURCE_RATING 350 +#define MCT_CLKEVENTS_RATING 350 +#endif + enum { MCT_INT_SPI, MCT_INT_PPI @@ -206,7 +215,7 @@ static void exynos4_frc_resume(struct clocksource *cs) static struct clocksource mct_frc = { .name = "mct-frc", - .rating = 450, /* use value higher than ARM arch timer */ + .rating = MCT_CLKSOURCE_RATING, .read = exynos4_frc_read, .mask = CLOCKSOURCE_MASK(32), .flags = CLOCK_SOURCE_IS_CONTINUOUS, @@ -457,7 +466,7 @@ static int exynos4_mct_starting_cpu(unsigned int cpu) evt->set_state_oneshot_stopped = set_state_shutdown; evt->tick_resume = set_state_shutdown; evt->features = CLOCK_EVT_FEAT_PERIODIC | CLOCK_EVT_FEAT_ONESHOT; - evt->rating = 500; /* use value higher than ARM arch timer */ + evt->rating = MCT_CLKEVENTS_RATING, exynos4_mct_write(TICK_BASE_CNT, mevt->base + MCT_L_TCNTB_OFFSET);
All arm64 CPUs feature an architected timer, which offers a relatively low-latency interface to a per-cpu clocksource and timer. For the most part, using this interface is a no-brainer, with the exception of SoCs where it cannot be used to wake up from deep idle state (i.e. CLOCK_EVT_FEAT_C3STOP is set). On the contrary, the Exynos MCT is extremely slow to access yet can be used as a wakeup source. In preparation for using the Exynos MCT as a potential wakeup timer for the Arm architected timer, reduce its ratings so that the architected timer is preferred. This effectively reverts the decision made in 6282edb72bed ("clocksource/drivers/exynos_mct: Increase priority over ARM arch timer") for arm64, as the reasoning for the original change was to work around a 32-bit SoC design. Cc: Marek Szyprowski <m.szyprowski@samsung.com> Cc: Krzysztof Kozlowski <krzk@kernel.org> Cc: Chanwoo Choi <cw00.choi@samsung.com> Cc: Thomas Gleixner <tglx@linutronix.de> Signed-off-by: Will Deacon <will@kernel.org> --- drivers/clocksource/exynos_mct.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)