Re: [PATCH] clocksource: sh_cmt: Fix wrong setting if don't request IRQ for clock source channel

[Daniel Lezcano <daniel.lezcano@linaro.org>]

On 22/04/2021 14:34, Niklas Söderlund wrote:
> From: Phong Hoang <phong.hoang.wz@renesas.com>
> 
> If CMT instance has at least two channels, one channel will be used
> as a clock source and another one used as a clock event device.
> In that case, IRQ is not requested for clock source channel so
> sh_cmt_clock_event_program_verify() might work incorrectly.
> Besides, when a channel is only used for clock source, don't need to
> re-set the next match_value since it should be maximum timeout as
> it still is.
> 
> On the other hand, due to no IRQ, total_cycles is not counted up
> when reaches compare match time (timer counter resets to zero),
> so sh_cmt_clocksource_read() returns unexpected value.
> Therefore, use 64-bit clocksoure's mask for 32-bit or 16-bit variants
> will also lead to wrong delta calculation. Hence, this mask should
> correspond to timer counter width, and above function just returns
> the raw value of timer counter register.

I'm not getting the 'ch->cmt->num_channels == 1' change, can you explain?

> Fixes: bfa76bb12f23 ("clocksource: sh_cmt: Request IRQ for clock event device only")
> Fixes: 37e7742c55ba ("clocksource/drivers/sh_cmt: Fix clocksource width for 32-bit machines")
> Signed-off-by: Phong Hoang <phong.hoang.wz@renesas.com>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@ragnatech.se>
> ---
>  drivers/clocksource/sh_cmt.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/clocksource/sh_cmt.c b/drivers/clocksource/sh_cmt.c
> index c98f8851fd680454..cadd09ad1a0946b9 100644
> --- a/drivers/clocksource/sh_cmt.c
> +++ b/drivers/clocksource/sh_cmt.c
> @@ -578,7 +578,8 @@ static int sh_cmt_start(struct sh_cmt_channel *ch, unsigned long flag)
>  	ch->flags |= flag;
>  
>  	/* setup timeout if no clockevent */
> -	if ((flag == FLAG_CLOCKSOURCE) && (!(ch->flags & FLAG_CLOCKEVENT)))
> +	if (ch->cmt->num_channels == 1 &&
> +	    flag == FLAG_CLOCKSOURCE && (!(ch->flags & FLAG_CLOCKEVENT)))
>  		__sh_cmt_set_next(ch, ch->max_match_value);
>   out:
>  	raw_spin_unlock_irqrestore(&ch->lock, flags);
> @@ -620,20 +621,25 @@ static struct sh_cmt_channel *cs_to_sh_cmt(struct clocksource *cs)
>  static u64 sh_cmt_clocksource_read(struct clocksource *cs)
>  {
>  	struct sh_cmt_channel *ch = cs_to_sh_cmt(cs);
> -	unsigned long flags;
>  	u32 has_wrapped;
> -	u64 value;
> -	u32 raw;
>  
> -	raw_spin_lock_irqsave(&ch->lock, flags);
> -	value = ch->total_cycles;
> -	raw = sh_cmt_get_counter(ch, &has_wrapped);
> +	if (ch->cmt->num_channels == 1) {



> +		unsigned long flags;
> +		u64 value;
> +		u32 raw;
>  
> -	if (unlikely(has_wrapped))
> -		raw += ch->match_value + 1;
> -	raw_spin_unlock_irqrestore(&ch->lock, flags);
> +		raw_spin_lock_irqsave(&ch->lock, flags);
> +		value = ch->total_cycles;
> +		raw = sh_cmt_get_counter(ch, &has_wrapped);
>  
> -	return value + raw;
> +		if (unlikely(has_wrapped))
> +			raw += ch->match_value + 1;
> +		raw_spin_unlock_irqrestore(&ch->lock, flags);
> +
> +		return value + raw;
> +	}
> +
> +	return sh_cmt_get_counter(ch, &has_wrapped);
>  }
>  
>  static int sh_cmt_clocksource_enable(struct clocksource *cs)
> @@ -696,7 +702,7 @@ static int sh_cmt_register_clocksource(struct sh_cmt_channel *ch,
>  	cs->disable = sh_cmt_clocksource_disable;
>  	cs->suspend = sh_cmt_clocksource_suspend;
>  	cs->resume = sh_cmt_clocksource_resume;
> -	cs->mask = CLOCKSOURCE_MASK(sizeof(u64) * 8);
> +	cs->mask = CLOCKSOURCE_MASK(ch->cmt->info->width);
>  	cs->flags = CLOCK_SOURCE_IS_CONTINUOUS;
>  
>  	dev_info(&ch->cmt->pdev->dev, "ch%u: used as clock source\n",
> 


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