Re: powerpc: Add an inline function to update HID0

[Gautham R Shenoy <ego@linux.vnet.ibm.com>]

Hi Segher,

Thanks for the suggestions. I will rename the function to
update_power8_hid0() and use asm volatile.


On Tue, Aug 04, 2015 at 09:30:57PM -0500, Segher Boessenkool wrote:
> On Tue, Aug 04, 2015 at 08:08:58PM +1000, Michael Ellerman wrote:
> > > +static inline void update_hid0(unsigned long hid0)
> > > +{
> > > +	/*
> > > +	 *  The HID0 update should at the very least be preceded by a
> > > +	 *  a SYNC instruction followed by an ISYNC instruction
> > > +	 */
> > > +	mb();
> > > +	mtspr(SPRN_HID0, hid0);
> > > +	isync();
> > 
> > That's going to turn into three separate inline asm blocks, which is maybe a
> > bit unfortunate. Have you checked the generated code is what we want, ie. just
> > sync, mtspr, isync ?
> 
> The "mb()" is not such a great name anyway: you don't want a memory
> barrier, you want an actual sync instruction ("sync 0", "hwsync",
> whatever the currently preferred spelling is).
> 
> The function name should also say this is for POWER8 (the required
> sequences are different for some other processors; and some others
> might not even _have_ a HID0, or not at 1008).  power8_write_hid0
> or such?
> 
> For writing it as one asm, why not just
> 
>   asm volatile("sync ; mtspr %0,%1 ; isync" : : "i"(SPRN_HID0), "r"(hid0));
> 
> instead of the stringify stuff?
> 
> 
> Segher
> 

--
Thanks and Regards
gautham.