From owner-svn-src-head@FreeBSD.ORG Tue Jul 9 12:59:34 2013 Return-Path: Delivered-To: svn-src-head@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) by hub.freebsd.org (Postfix) with ESMTP id 3796ADC7; Tue, 9 Jul 2013 12:59:34 +0000 (UTC) (envelope-from ray@freebsd.org) Received: from smtp.dlink.ua (smtp.dlink.ua [193.138.187.146]) by mx1.freebsd.org (Postfix) with ESMTP id A3F241E73; Tue, 9 Jul 2013 12:59:33 +0000 (UTC) Received: from terran (unknown [192.168.99.1]) (Authenticated sender: ray) by smtp.dlink.ua (Postfix) with ESMTPA id 9976BC4963; Mon, 8 Jul 2013 13:21:09 +0300 (EEST) Date: Mon, 8 Jul 2013 13:20:52 +0300 From: Aleksandr Rybalko To: Bruce Evans 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> X-Mailer: Sylpheed 3.2.0 (GTK+ 2.24.6; amd64-portbld-freebsd9.0) Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: svn-src-head@FreeBSD.org, Aleksandr Rybalko , src-committers@FreeBSD.org, svn-src-all@FreeBSD.org X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 09 Jul 2013 12:59:34 -0000 On Mon, 1 Jul 2013 11:56:25 +1000 (EST) Bruce Evans 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 > > > > 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