Date: Wed, 5 Aug 2015 12:24:00 +0530
From: Gautham R Shenoy <ego@linux.vnet.ibm.com>
To: Segher Boessenkool <segher@kernel.crashing.org>
Cc: Michael Ellerman <mpe@ellerman.id.au>,
        "Gautham R. Shenoy" <ego@linux.vnet.ibm.com>,
        Paul Mackerras <paulus@samba.org>, linux-kernel@vger.kernel.org,
        linuxppc-dev@lists.ozlabs.org, mikey@neuling.org
Subject: Re: powerpc: Add an inline function to update HID0
Message-ID: <20150805065400.GA14701@in.ibm.com>
Reply-To: ego@linux.vnet.ibm.com
References: <1438677058-12883-1-git-send-email-ego@linux.vnet.ibm.com>
 <20150804100858.1F272140306@ozlabs.org>
 <20150805023057.GT11083@gate.crashing.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20150805023057.GT11083@gate.crashing.org>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 1601
Lines: 50

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.

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/