All of lore.kernel.org
 help / color / mirror / Atom feed
From: Stafford Horne <shorne@gmail.com>
To: Joel Holdsworth <jholdsworth@nvidia.com>
Cc: Richard Henderson <richard.henderson@linaro.org>,
	"qemu-devel@nongnu.org" <qemu-devel@nongnu.org>
Subject: Re: [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode
Date: Thu, 19 Dec 2024 21:50:26 +0000	[thread overview]
Message-ID: <Z2SVIopc8Fgfajkk@antec> (raw)
In-Reply-To: <DM4PR12MB656535602DEDBA2D60321A54C8062@DM4PR12MB6565.namprd12.prod.outlook.com>

On Thu, Dec 19, 2024 at 08:08:14PM +0000, Joel Holdsworth wrote:
> > > > > +        /* Zero the count by applying a negative offset to the counter */
> > > > > +        or1k_timer->ttcr_offset += UINT32_MAX - (cpu->env.ttmr & TTMR_TP);
> > > >
> > > > Since UINT32_MAX is -1 in this context, this appears to be off-by-one.
> > > > I think -(ttmr & mask) alone is correct.
> > > 
> > > Thanks, I did send a mail to Joel asking about this bit.  He didn't respond for 2
> > > weeks to I just sent the patch as is as it appears to work.  As I understand,
> > > yes UINT32_MAX is just -1.  But why the -1?  I guess it's because after
> > > ttcr_offset is updated we call cpu_openrisc_timer_update() which calls
> > > cpu_openrisc_count_update() to update ttcr.  Since a few _ns would have passed
> > > and we are rounding up it will update ttcr to 0.
> > >
> > > But maybe I am reading too much into it.
> >
> > I think you're reading too much into it -- I just think it's a bug which isn't particularly noticeable because the clock is only off by 1ns.
> 
> Richard is correct. It should be:
> 
> or1k_timer->ttcr_offset += -(cpu->env.ttmr & TTMR_TP);
> 
> Stafford: sorry for not being responsive. I've been very busy lately, and it's been several months since I touched anything OpenRISC-related. Are you able to push this the rest of the way through the acceptance process? I had understood that you were looking for a more elaborate overhaul of the OpenRISC timer design which I haven't had time to work on. But if the patch can go forward in its current form, I think the improvement is worth having even it doesn't address other design issues.

Hi Joel,

Yes, I was able to sort out the issues and get everything tested and fixed up.
The patch is now committed upstream in QEMU for the 9.2.0 release.  I also
thought its better to have this upstream in its current form rather than wait
for further improvements.

Thanks for your help.  I look forward to you getting some free cycles and
helping more with OpenRISC when you get any chance.

I was also busy last year until November and didn't have much time to work on
this.

-Stafford


  reply	other threads:[~2024-12-19 21:51 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-12-19 20:08 [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode Joel Holdsworth
2024-12-19 21:50 ` Stafford Horne [this message]
  -- strict thread matches above, loose matches on Subject: below --
2024-11-23 10:38 [PATCH 0/2] Misc OpenRISC fixes for 9.2.0 Stafford Horne
2024-11-23 10:38 ` [PATCH 2/2] hw/openrisc: Fixed undercounting of TTCR in continuous mode Stafford Horne
2024-11-23 13:39   ` Richard Henderson
2024-11-23 17:11     ` Stafford Horne
2024-11-23 21:11       ` Richard Henderson

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=Z2SVIopc8Fgfajkk@antec \
    --to=shorne@gmail.com \
    --cc=jholdsworth@nvidia.com \
    --cc=qemu-devel@nongnu.org \
    --cc=richard.henderson@linaro.org \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.