Date: Wed, 21 Dec 2011 13:52:04 -0800 From: Adrian Chadd <adrian@freebsd.org> To: Dimitry Andric <dim@freebsd.org> Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org Subject: Re: svn commit: r228785 - in head/sys/dev/ath/ath_hal: ar5210 ar5211 Message-ID: <CAJ-Vmont2wOF=TkT1CmZt4Gp0iOc7ZYmfQpjW=8j1=F5DQGNwQ@mail.gmail.com> In-Reply-To: <201112211716.pBLHGhDH078507@svn.freebsd.org> References: <201112211716.pBLHGhDH078507@svn.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Erm, why did you do this without first getting clearance from someone who has the hardware to test it? Just because it looks obviously wrong to you, doesn't at all mean that it's "wrong". It's quite possible that the driver _requires_ those bits to be written to the hardware as 0. I'd appreciate it if would please revert this and other ath/hal changes until I've had time to research them and test them out. Thanks, Adrian On 21 December 2011 09:16, Dimitry Andric <dim@freebsd.org> wrote: > Author: dim > Date: Wed Dec 21 17:16:43 2011 > New Revision: 228785 > URL: http://svn.freebsd.org/changeset/base/228785 > > Log: > =A0Fix shift overflow problem in sys/dev/ath/ath_hal/ar5210/ar5210_power.= c > =A0and sys/dev/ath/ath_hal/ar5211/ar5211_power.c: > > =A0sys/dev/ath/ath_hal/ar5210/ar5210_power.c:36:3: warning: signed shift = result (0x200000000) requires 35 bits to represent, but 'int' only has 32 b= its [-Wshift-overflow] > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0OS_REG_RMW_FIELD(ah, AR_SCR, AR_SCR_SL= E, AR_SCR_SLE_ALLOW); > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~= ~~~~~~~~~~~~~~~~~~~~ > =A0sys/dev/ath/ath_hal/ah_internal.h:472:42: note: expanded from: > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0(OS_REG_READ(_a, _r) &~ (_f)) | (((_v)= << _f##_S) & (_f))) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^ > =A0sys/dev/ath/ah_osdep.h:127:49: note: expanded from: > =A0 =A0 =A0 =A0 =A0 =A0 =A0(bus_space_handle_t)(_ah)->ah_sh, (_reg), (_va= l)) > =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 ^~~~ > > =A0The AR_SCR_SLE_{WAKE,SLP,NORM} values are pre-shifted in ar5210reg.h a= nd > =A0ar5211reg.h, while they should be unshifted, like in ar5212reg.h. =A0T= hen, > =A0when the OS_REG_RMW_FIELD() macro shifts them again, the values will > =A0overflow, becoming effectively zero. > > =A0MFC after: 1 week > > Modified: > =A0head/sys/dev/ath/ath_hal/ar5210/ar5210reg.h > =A0head/sys/dev/ath/ath_hal/ar5211/ar5211reg.h > > Modified: head/sys/dev/ath/ath_hal/ar5210/ar5210reg.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/dev/ath/ath_hal/ar5210/ar5210reg.h Wed Dec 21 17:03:30 2011 = =A0 =A0 =A0 =A0(r228784) > +++ head/sys/dev/ath/ath_hal/ar5210/ar5210reg.h Wed Dec 21 17:16:43 2011 = =A0 =A0 =A0 =A0(r228785) > @@ -245,9 +245,9 @@ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLDUR =A0 =A0 =A0 =A0 =A0 =A00x0000ffff = =A0 =A0 =A0/* sleep duration */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLE =A0 =A0 =A0 =A0 =A0 =A0 =A00x0003000= 0 =A0 =A0 =A0/* sleep enable */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLE_S =A0 =A0 =A0 =A0 =A0 =A016 > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_WAKE =A0 =A0 =A0 =A0 0x00000000 =A0 = =A0 =A0/* force wake */ > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_SLP =A0 =A0 =A0 =A0 =A00x00010000 =A0 = =A0 =A0/* force sleep */ > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_ALLOW =A0 =A0 =A0 =A00x00020000 =A0 = =A0 =A0/* allow to control sleep */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_WAKE =A0 =A0 =A0 =A0 0 =A0 =A0 =A0 =A0= =A0 =A0 =A0 /* force wake */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_SLP =A0 =A0 =A0 =A0 =A01 =A0 =A0 =A0 = =A0 =A0 =A0 =A0 /* force sleep */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_ALLOW =A0 =A0 =A0 =A02 =A0 =A0 =A0 =A0= =A0 =A0 =A0 /* allow to control sleep */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_BITS =A0 =A0 "\20\20SLE_SLP\21SLE_ALLOW" > > =A0#define =A0 =A0 =A0 =A0AR_INTPEND_IP =A0 =A0 =A0 =A0 =A0 0x00000001 = =A0 =A0 =A0/* interrupt pending */ > > Modified: head/sys/dev/ath/ath_hal/ar5211/ar5211reg.h > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D > --- head/sys/dev/ath/ath_hal/ar5211/ar5211reg.h Wed Dec 21 17:03:30 2011 = =A0 =A0 =A0 =A0(r228784) > +++ head/sys/dev/ath/ath_hal/ar5211/ar5211reg.h Wed Dec 21 17:16:43 2011 = =A0 =A0 =A0 =A0(r228785) > @@ -618,9 +618,9 @@ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLDUR_S =A00 > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLE =A0 =A0 =A00x00030000 =A0 =A0 =A0/* = sleep enable mask */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLE_S =A0 =A016 =A0 =A0 =A0 =A0 =A0 =A0 = =A0/* sleep enable bits shift */ > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_WAKE 0x00000000 =A0 =A0 =A0/* force wa= ke */ > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_SLP =A00x00010000 =A0 =A0 =A0/* force = sleep */ > -#define =A0 =A0 =A0 =A0AR_SCR_SLE_NORM 0x00020000 =A0 =A0 =A0/* sleep lo= gic normal operation */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_WAKE 0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* = force wake */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_SLP =A01 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /= * force sleep */ > +#define =A0 =A0 =A0 =A0AR_SCR_SLE_NORM 2 =A0 =A0 =A0 =A0 =A0 =A0 =A0 /* = sleep logic normal operation */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_SLE_UNITS =A0 =A0 =A0 =A00x00000008 =A0 = =A0 =A0/* SCR units/TU */ > =A0#define =A0 =A0 =A0 =A0AR_SCR_BITS =A0 =A0 "\20\20SLE_SLP\21SLE" >
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-Vmont2wOF=TkT1CmZt4Gp0iOc7ZYmfQpjW=8j1=F5DQGNwQ>