On 22/04/2021 14:34, Niklas Söderlund wrote:
> From: Phong Hoang <phong.hoang.wz@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Niklas Söderlund <niklas.soderlund+renesas@xxxxxxxxxxxx>
> ---
> 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
