LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Daniel Lezcano <daniel.lezcano@linaro.org>
To: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>,
	"Wolfram Sang" <wsa+renesas@sang-engineering.com>,
	linux-kernel@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org,
	Phong Hoang <phong.hoang.wz@renesas.com>
Subject: Re: [PATCH] clocksource: sh_cmt: Fix wrong setting if don't request IRQ for clock source channel
Date: Thu, 10 Jun 2021 14:45:05 +0200	[thread overview]
Message-ID: <7cf6444a-1dc4-a2bb-8fef-696e0b2b6f23@linaro.org> (raw)
In-Reply-To: <20210422123443.73334-1-niklas.soderlund+renesas@ragnatech.se>

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

  reply	other threads:[~2021-06-10 12:46 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-04-22 12:34 Niklas Söderlund
2021-06-10 12:45 ` Daniel Lezcano [this message]
2021-06-11  7:56   ` Niklas Söderlund
2021-08-11 14:06     ` Wolfram Sang
2021-08-26 16:25 ` [tip: timers/core] clocksource/drivers/sh_cmt: " tip-bot2 for Phong Hoang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7cf6444a-1dc4-a2bb-8fef-696e0b2b6f23@linaro.org \
    --to=daniel.lezcano@linaro.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-renesas-soc@vger.kernel.org \
    --cc=niklas.soderlund+renesas@ragnatech.se \
    --cc=phong.hoang.wz@renesas.com \
    --cc=wsa+renesas@sang-engineering.com \
    --subject='Re: [PATCH] clocksource: sh_cmt: Fix wrong setting if don'\''t request IRQ for clock source channel' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).