From: oss@buserror•net (Scott Wood)
To: linux-arm-kernel@lists•infradead.org
Subject: [RFC PATCH 2/2] ARM64: arch_timer: Work around QorIQ Erratum A-009971
Date: Sat, 16 Apr 2016 20:32:21 -0500 [thread overview]
Message-ID: <1460856741.32510.161.camel@buserror.net> (raw)
In-Reply-To: <570CB04B.3090406@arm.com>
On Tue, 2016-04-12 at 09:22 +0100, Marc Zyngier wrote:
> On 12/04/16 06:54, Scott Wood wrote:
> > On Mon, 2016-04-11 at 10:55 +0100, Marc Zyngier wrote:
> > > On 11/04/16 03:22, Scott Wood wrote:
> > > > @@ -52,6 +53,20 @@ extern bool arm_arch_timer_reread;
> > > > _val; \
> > > > })
> > > >
> > > > +#define ARCH_TIMER_TVAL_REWRITE(pv, val) do { \
> > > > + u64 _cnt_old, _cnt_new; \
> > > > + int _timeout = 200; \
> > > > + do { \
> > > > + asm volatile("mrs %0, cntvct_el0;" \
> > > > + "msr cnt" pv "_tval_el0, %2;" \
> > > > + "mrs %1, cntvct_el0" \
> > > > + : "=&r" (_cnt_old), "=r" (_cnt_new) \
> > > > + : "r" (val)); \
> > > > + _timeout--; \
> > > > + } while (_cnt_old != _cnt_new && _timeout); \
> > > > + WARN_ON_ONCE(!_timeout); \
> > > > +} while (0)
> > > > +
> > >
> > > Given how many times you've written that loop, I'm sure you can have a
> > > preprocessor macro that will do the right thing.
> >
> > I did use a preprocessor macro. Are you asking me to consolidate the read
> > and
> > write macros? That seems like it would create a mess that's worse than an
> > extra instance of a simple loop.
>
> From patch 1:
>
> +static __always_inline
> +u32 arch_timer_reg_tval_reread(int access, enum arch_timer_reg reg)
> +{
> + u32 val, val_new;
> + int timeout = 200;
> +
> + do {
> + if (access == ARCH_TIMER_PHYS_ACCESS) {
> + asm volatile("mrc p15, 0, %0, c14, c2, 0;"
> + "mrc p15, 0, %1, c14, c2, 0"
> + : "=r" (val), "=r" (val_new));
> + } else if (access == ARCH_TIMER_VIRT_ACCESS) {
> + asm volatile("mrc p15, 0, %0, c14, c3, 0;"
> + "mrc p15, 0, %1, c14, c3, 0"
> + : "=r" (val), "=r" (val_new));
> + }
> + timeout--;
> + } while (val != val_new && timeout);
> +
> + WARN_ON_ONCE(!timeout);
> + return val;
> +}
>
> [...]
>
> +static __always_inline u64 arch_counter_get_cnt(int opcode, bool reread)
> {
> - u64 cval;
> + u64 val, val_new;
> + int timeout = 200;
>
> isb();
> - asm volatile("mrrc p15, 0, %Q0, %R0, c14" : "=r" (cval));
> - return cval;
> +
> + if (reread) {
> + do {
> + asm volatile("mrrc p15, %2, %Q0, %R0, c14;"
> + "mrrc p15, %2, %Q1, %R1, c14"
> + : "=r" (val), "=r" (val_new)
> + : "i" (opcode));
> + timeout--;
> + } while (val != val_new && timeout);
> +
> + BUG_ON(!timeout);
> + } else {
> + asm volatile("mrrc p15, %1, %Q0, %R0, c14" : "=r" (val)
> + : "i" (opcode));
> + }
> +
> + return val;
> }
>
> [...]
>
> +/* QorIQ errata workarounds */
> +#define ARCH_TIMER_REREAD(reg) ({ \
> + u64 _val_old, _val_new; \
> + int _timeout = 200; \
> + do { \
> + asm volatile("mrs %0, " reg ";" \
> + "mrs %1, " reg \
> + : "=r" (_val_old), "=r" (_val_new)); \
> + _timeout--; \
> + } while (_val_old != _val_new && _timeout); \
> + WARN_ON_ONCE(!_timeout); \
> + _val_old; \
> +})
>
> Do you notice a pattern? You are expressing the same loop in various
> ways (sometimes in a function, sometimes in a macro). I'm looking for a
> loop template that encapsulates the read access. You can have a similar
> macro for writes if you have more than a single instance.
One that covers arm and arm64? Where would it go?
If you mean one per arch, that's already the case on 64-bit (and you
complained in response to the write macro, hence my inferring that you wanted
read and write combined). Two instances on 32-bit (of a fairly simple loop)
didn't seem enough to warrant using ugly macros, but I can if you want (with
the entire asm statement passed as a macro parameter).
-Scott
next prev parent reply other threads:[~2016-04-17 1:32 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-11 2:22 [RFC PATCH 0/2] ARM/ARM64: arch_timer: QorIQ errata Scott Wood
2016-04-11 2:22 ` [RFC PATCH 1/2] ARM/ARM64: arch_timer: Work around QorIQ Erratum A-008585 Scott Wood
2016-04-11 9:19 ` Will Deacon
2016-04-11 9:52 ` Marc Zyngier
2016-04-12 5:48 ` Scott Wood
2016-04-12 9:07 ` Marc Zyngier
2016-04-13 5:41 ` Scott Wood
2016-04-13 7:36 ` Marc Zyngier
2016-04-13 13:22 ` Rob Herring
2016-04-17 0:58 ` Scott Wood
2016-04-11 2:22 ` [RFC PATCH 2/2] ARM64: arch_timer: Work around QorIQ Erratum A-009971 Scott Wood
2016-04-11 9:55 ` Marc Zyngier
2016-04-12 5:54 ` Scott Wood
2016-04-12 8:22 ` Marc Zyngier
2016-04-17 1:32 ` Scott Wood [this message]
2016-04-18 9:28 ` Marc Zyngier
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=1460856741.32510.161.camel@buserror.net \
--to=oss@buserror$(echo .)net \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox