Date: Mon, 8 Jul 2013 13:20:52 +0300 From: Aleksandr Rybalko <ray@freebsd.org> To: Bruce Evans <brde@optusnet.com.au> Cc: svn-src-head@FreeBSD.org, Aleksandr Rybalko <ray@FreeBSD.org>, src-committers@FreeBSD.org, svn-src-all@FreeBSD.org Subject: Re: svn commit: r252425 - head/sys/arm/arm Message-ID: <20130708132052.f9757df88ede1b087de9da8c@freebsd.org> In-Reply-To: <20130701113808.F894@besplex.bde.org> References: <201306301952.r5UJqfwf010873@svn.freebsd.org> <20130701113808.F894@besplex.bde.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, 1 Jul 2013 11:56:25 +1000 (EST)
Bruce Evans <brde@optusnet.com.au> wrote:
> On Sun, 30 Jun 2013, Aleksandr Rybalko wrote:
>
> > Log:
> > Decrypt magic numbers - define names for fields of Generic Timer's CNTKCTL reg.
> >
> > Submitted by: Ruslan Bukin <br@bsdpad.com>
> >
> > Modified:
> > head/sys/arm/arm/generic_timer.c
> >
> > Modified: head/sys/arm/arm/generic_timer.c
> > ==============================================================================
> > --- head/sys/arm/arm/generic_timer.c Sun Jun 30 19:36:17 2013 (r252424)
> > +++ head/sys/arm/arm/generic_timer.c Sun Jun 30 19:52:41 2013 (r252425)
> > @@ -66,7 +66,22 @@ __FBSDID("$FreeBSD$");
> > #define GENERIC_TIMER_REG_CTRL 0
> > #define GENERIC_TIMER_REG_TVAL 1
> >
> > -#define CNTPSIRQ 29
> > +#define GENERIC_TIMER_CNTKCTL_PL0PTEN (1 << 9) /* Physical timer registers
> > + access from PL0 */
> > +#define GENERIC_TIMER_CNTKCTL_PL0VTEN (1 << 8) /* Virtual timer registers
>
> With names like these, the magic numbers are better. The prefix name
> GENERIC_TIMER is especially bad. GT would be good.
Changed in r252780.
>
> > ...
> > +#define GENERIC_TIMER_CNTPSIRQ 29
>
> Here the interesting part CNTPSIRQ is fairly abbreviated, but its prefix
> is not.
GENERIC_TIMER was original prefix before timer access bits was added,
but CNTPSIRQ is original name from documentation. Name was preserved to
avoid confusion.
>
> >
> > struct arm_tmr_softc {
> > struct resource *irq_res;
> > @@ -167,7 +182,11 @@ disable_user_access(void)
> > uint32_t cntkctl;
> >
> > __asm volatile("mrc p15, 0, %0, c14, c1, 0" : "=r" (cntkctl));
> > - cntkctl &= ~((3 << 8) | (7 << 0));
> > + cntkctl &= ~(GENERIC_TIMER_CNTKCTL_PL0PTEN |
> > + GENERIC_TIMER_CNTKCTL_PL0VTEN |
> > + GENERIC_TIMER_CNTKCTL_EVNTEN |
> > + GENERIC_TIMER_CNTKCTL_PL0VCTEN |
> > + GENERIC_TIMER_CNTKCTL_PL0PCTEN);
> > __asm volatile("mcr p15, 0, %0, c14, c1, 0" : : "r" (cntkctl));
> > isb();
> > }
>
> Using these verbose names takes about 15 times as much code as
> before (the statement is only 5 times longer, but the definitions
> macros are about 10 times longer), and looks like even more becayse
> the statement is misformatted with non-KNF indentation which then
> requires extra line spitting with only 1 name per line.
Now a lot of bytes saved! :)
>
> > @@ -270,7 +289,8 @@ arm_tmr_attach(device_t dev)
> >
> > rid = 0;
> > sc->irq_res = bus_alloc_resource(dev, SYS_RES_IRQ, &rid,
> > - CNTPSIRQ, CNTPSIRQ, 1, RF_SHAREABLE | RF_ACTIVE);
> > + GENERIC_TIMER_CNTPSIRQ, GENERIC_TIMER_CNTPSIRQ,
> > + 1, RF_SHAREABLE | RF_ACTIVE);
> >
> > arm_tmr_sc = sc;
>
> For full uglyness, expand all the prefixes and names:
I will! :-D
> - SYS -> SYSTEM
> - RES -> RESOURCE
> - IRQ -> INTERRUPT_REQUEST_NUMBER
> - SYS_RES_IRQ -> SYSTEM_RESOURCE_INTERRUPT_REQUEST_NUMBER
> - RF -> RESOURCE_FLAG
> - RF_SHAREABLE_RESOURCE_FLAG_SHAREABLE
> - RF_ACTIVE -> RESOURCE_FLAG_ACTIVE
> - CNTPSIRQ -> COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER
> (just guessing what PS means):
>
> sc->irq_res = bus_alloc_resource(dev, SYS_RES_INTERRUPT_REQUEST_NUMBER,
> &rid,
> GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER,
> GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER,
> 1, RESOURCE_FLAG_SHAREABLE | RESOURCE_FLAG_ACTIVE);
>
> Next, use non-KNF indentation:
>
> sc->irq_res = bus_alloc_resource(dev, SYS_RES_INTERRUPT_REQUEST_NUMBER,
> &rid,
> GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER,
> GENERIC_TIMER_COUNT_PRIVATELY_SOURCED_INTERRUPT_REQUEST_NUMBER,
> 1, RESOURCE_FLAG_SHAREABLE | RESOURCE_FLAG_ACTIVE);
>
> The names aren't even 80 characters long, so they actually fit on 1 line.
>
> Bruce
Thanks Bruce!
"Long words only upset me." (c) Vinny-Pooh (USSR version) :-D
--
Aleksandr Rybalko <ray@freebsd.org>
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20130708132052.f9757df88ede1b087de9da8c>