[RFC,linux,dev-4.19] clocksource: fttmr010: Detect back-pressure for Aspeed controllers

Message ID 20190111033628.18677-1-andrew@aj.id.au
State New
Headers show
Series
  • [RFC,linux,dev-4.19] clocksource: fttmr010: Detect back-pressure for Aspeed controllers
Related show

Commit Message

Andrew Jeffery Jan. 11, 2019, 3:36 a.m.
Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
---

See the cover letter on the corresponding qemu RFC series, to be sent after
this patch.

 arch/arm/mach-aspeed/Kconfig         | 1 +
 drivers/clocksource/timer-fttmr010.c | 7 +++++++
 2 files changed, 8 insertions(+)

Comments

Cédric Le Goater Jan. 11, 2019, 9:06 a.m. | #1
On 1/11/19 4:36 AM, Andrew Jeffery wrote:
> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>

This patch should not have any impact on a real system where l == cycles.
It might be difficult to merge though.

C. 

> ---
> 
> See the cover letter on the corresponding qemu RFC series, to be sent after
> this patch.
> 
>  arch/arm/mach-aspeed/Kconfig         | 1 +
>  drivers/clocksource/timer-fttmr010.c | 7 +++++++
>  2 files changed, 8 insertions(+)
> 
> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> index 2d5570e6e186..6b82d5416cf0 100644
> --- a/arch/arm/mach-aspeed/Kconfig
> +++ b/arch/arm/mach-aspeed/Kconfig
> @@ -7,6 +7,7 @@ menuconfig ARCH_ASPEED
>  	select FTTMR010_TIMER
>  	select MFD_SYSCON
>  	select PINCTRL
> +	select GENERIC_CLOCKEVENTS_MIN_ADJUST
>  	help
>  	  Say Y here if you want to run your kernel on an ASpeed BMC SoC.
>  
> diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
> index cf93f6419b51..f15386b98289 100644
> --- a/drivers/clocksource/timer-fttmr010.c
> +++ b/drivers/clocksource/timer-fttmr010.c
> @@ -124,6 +124,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>  {
>  	struct fttmr010 *fttmr010 = to_fttmr010(evt);
>  	u32 cr;
> +	u32 l;
>  
>  	/* Stop */
>  	cr = readl(fttmr010->base + TIMER_CR);
> @@ -136,6 +137,12 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>  		 * into TIMER1_COUNT register when the timer is re-enabled.
>  		 */
>  		writel(cycles, fttmr010->base + TIMER1_LOAD);
> +#if CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST> +		/* QEMU model may provide back-pressure on requested period */
> +		l = readl(fttmr010->base + TIMER1_LOAD);
> +		if (l > cycles)
> +			return -EINVAL;
> +#endif
>  	} else {
>  		/* Setup the match register forward in time */
>  		cr = readl(fttmr010->base + TIMER1_COUNT);
>
Andrew Jeffery Jan. 13, 2019, 11:04 p.m. | #2
On Fri, 11 Jan 2019, at 19:36, Cédric Le Goater wrote:
> On 1/11/19 4:36 AM, Andrew Jeffery wrote:
> > Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
> 
> This patch should not have any impact on a real system where l == cycles.
> It might be difficult to merge though.

I agree.

Note however that the qemu patches resolve the issue regardless of whether this
patch to Linux is applied, just if it's not Linux will not be aware that the timer isn't
behaving how Linux requested.

Maybe we live with that?

Andrew

> 
> C. 
> 
> > ---
> > 
> > See the cover letter on the corresponding qemu RFC series, to be sent after
> > this patch.
> > 
> >  arch/arm/mach-aspeed/Kconfig         | 1 +
> >  drivers/clocksource/timer-fttmr010.c | 7 +++++++
> >  2 files changed, 8 insertions(+)
> > 
> > diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
> > index 2d5570e6e186..6b82d5416cf0 100644
> > --- a/arch/arm/mach-aspeed/Kconfig
> > +++ b/arch/arm/mach-aspeed/Kconfig
> > @@ -7,6 +7,7 @@ menuconfig ARCH_ASPEED
> >  	select FTTMR010_TIMER
> >  	select MFD_SYSCON
> >  	select PINCTRL
> > +	select GENERIC_CLOCKEVENTS_MIN_ADJUST
> >  	help
> >  	  Say Y here if you want to run your kernel on an ASpeed BMC SoC.
> >  
> > diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
> > index cf93f6419b51..f15386b98289 100644
> > --- a/drivers/clocksource/timer-fttmr010.c
> > +++ b/drivers/clocksource/timer-fttmr010.c
> > @@ -124,6 +124,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
> >  {
> >  	struct fttmr010 *fttmr010 = to_fttmr010(evt);
> >  	u32 cr;
> > +	u32 l;
> >  
> >  	/* Stop */
> >  	cr = readl(fttmr010->base + TIMER_CR);
> > @@ -136,6 +137,12 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
> >  		 * into TIMER1_COUNT register when the timer is re-enabled.
> >  		 */
> >  		writel(cycles, fttmr010->base + TIMER1_LOAD);
> > +#if CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST> +		/* QEMU model may provide back-pressure on requested period */
> > +		l = readl(fttmr010->base + TIMER1_LOAD);
> > +		if (l > cycles)
> > +			return -EINVAL;
> > +#endif
> >  	} else {
> >  		/* Setup the match register forward in time */
> >  		cr = readl(fttmr010->base + TIMER1_COUNT);
> > 
>
Cédric Le Goater Jan. 14, 2019, 7:40 a.m. | #3
On 1/14/19 12:04 AM, Andrew Jeffery wrote:
> On Fri, 11 Jan 2019, at 19:36, Cédric Le Goater wrote:
>> On 1/11/19 4:36 AM, Andrew Jeffery wrote:
>>> Signed-off-by: Andrew Jeffery <andrew@aj.id.au>
>>
>> This patch should not have any impact on a real system where l == cycles.
>> It might be difficult to merge though.
> 
> I agree.
> 
> Note however that the qemu patches resolve the issue regardless of whether this
> patch to Linux is applied, just if it's not Linux will not be aware that the timer isn't
> behaving how Linux requested.

Indeed. I checked on a ppc64el.

> Maybe we live with that?

yes ! Could please send your QEMU patchset ?  
 
Thanks,

C. 


> Andrew
> 
>>
>> C. 
>>
>>> ---
>>>
>>> See the cover letter on the corresponding qemu RFC series, to be sent after
>>> this patch.
>>>
>>>  arch/arm/mach-aspeed/Kconfig         | 1 +
>>>  drivers/clocksource/timer-fttmr010.c | 7 +++++++
>>>  2 files changed, 8 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
>>> index 2d5570e6e186..6b82d5416cf0 100644
>>> --- a/arch/arm/mach-aspeed/Kconfig
>>> +++ b/arch/arm/mach-aspeed/Kconfig
>>> @@ -7,6 +7,7 @@ menuconfig ARCH_ASPEED
>>>  	select FTTMR010_TIMER
>>>  	select MFD_SYSCON
>>>  	select PINCTRL
>>> +	select GENERIC_CLOCKEVENTS_MIN_ADJUST
>>>  	help
>>>  	  Say Y here if you want to run your kernel on an ASpeed BMC SoC.
>>>  
>>> diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
>>> index cf93f6419b51..f15386b98289 100644
>>> --- a/drivers/clocksource/timer-fttmr010.c
>>> +++ b/drivers/clocksource/timer-fttmr010.c
>>> @@ -124,6 +124,7 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>>>  {
>>>  	struct fttmr010 *fttmr010 = to_fttmr010(evt);
>>>  	u32 cr;
>>> +	u32 l;
>>>  
>>>  	/* Stop */
>>>  	cr = readl(fttmr010->base + TIMER_CR);
>>> @@ -136,6 +137,12 @@ static int fttmr010_timer_set_next_event(unsigned long cycles,
>>>  		 * into TIMER1_COUNT register when the timer is re-enabled.
>>>  		 */
>>>  		writel(cycles, fttmr010->base + TIMER1_LOAD);
>>> +#if CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST> +		/* QEMU model may provide back-pressure on requested period */
>>> +		l = readl(fttmr010->base + TIMER1_LOAD);
>>> +		if (l > cycles)
>>> +			return -EINVAL;
>>> +#endif
>>>  	} else {
>>>  		/* Setup the match register forward in time */
>>>  		cr = readl(fttmr010->base + TIMER1_COUNT);
>>>
>>

Patch

diff --git a/arch/arm/mach-aspeed/Kconfig b/arch/arm/mach-aspeed/Kconfig
index 2d5570e6e186..6b82d5416cf0 100644
--- a/arch/arm/mach-aspeed/Kconfig
+++ b/arch/arm/mach-aspeed/Kconfig
@@ -7,6 +7,7 @@  menuconfig ARCH_ASPEED
 	select FTTMR010_TIMER
 	select MFD_SYSCON
 	select PINCTRL
+	select GENERIC_CLOCKEVENTS_MIN_ADJUST
 	help
 	  Say Y here if you want to run your kernel on an ASpeed BMC SoC.
 
diff --git a/drivers/clocksource/timer-fttmr010.c b/drivers/clocksource/timer-fttmr010.c
index cf93f6419b51..f15386b98289 100644
--- a/drivers/clocksource/timer-fttmr010.c
+++ b/drivers/clocksource/timer-fttmr010.c
@@ -124,6 +124,7 @@  static int fttmr010_timer_set_next_event(unsigned long cycles,
 {
 	struct fttmr010 *fttmr010 = to_fttmr010(evt);
 	u32 cr;
+	u32 l;
 
 	/* Stop */
 	cr = readl(fttmr010->base + TIMER_CR);
@@ -136,6 +137,12 @@  static int fttmr010_timer_set_next_event(unsigned long cycles,
 		 * into TIMER1_COUNT register when the timer is re-enabled.
 		 */
 		writel(cycles, fttmr010->base + TIMER1_LOAD);
+#if CONFIG_GENERIC_CLOCKEVENTS_MIN_ADJUST
+		/* QEMU model may provide back-pressure on requested period */
+		l = readl(fttmr010->base + TIMER1_LOAD);
+		if (l > cycles)
+			return -EINVAL;
+#endif
 	} else {
 		/* Setup the match register forward in time */
 		cr = readl(fttmr010->base + TIMER1_COUNT);