From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [RFC PATCH] drivers: bus: add ARM CCI support Date: Mon, 22 Apr 2013 15:24:57 +0100 Message-ID: <20130422142457.GM14496@n2100.arm.linux.org.uk> References: <1365691679-28674-1-git-send-email-lorenzo.pieralisi@arm.com> <1365691679-28674-2-git-send-email-lorenzo.pieralisi@arm.com> <51702B70.9000908@codeaurora.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: devicetree-discuss-bounces+gldd-devicetree-discuss=m.gmane.org-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org Sender: "devicetree-discuss" To: Nicolas Pitre Cc: Jon Medhurst , devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org, Stephen Boyd , Rob Herring , Santosh Shilimkar , Amit Kucheria , linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org List-Id: devicetree@vger.kernel.org On Thu, Apr 18, 2013 at 01:54:04PM -0400, Nicolas Pitre wrote: > On Thu, 18 Apr 2013, Stephen Boyd wrote: > > > On 04/11/13 07:47, Lorenzo Pieralisi wrote: > > > diff --git a/drivers/bus/arm-cci.c b/drivers/bus/arm-cci.c > > > new file mode 100644 > > > index 0000000..81953de > > > --- /dev/null > > > +++ b/drivers/bus/arm-cci.c > > [...] > > > +static void notrace cci_port_control(unsigned int port, bool enable) > > > +{ > > > + void __iomem *base = ports[port].base; > > > + > > > + if (!base) > > > + return; > > > + > > > + writel_relaxed(enable, base + CCI_PORT_CTRL); > > > + while (readl_relaxed(cci_ctrl_base + CCI_CTRL_STATUS) & 0x1) > > > + ; > > > > cpu_relax()? > > In some cases there is no more cache coherence when this is called and > the hardware might not be in a good state to cope with whatever action > people might be tempted to insert into cpu_relax(). After the CCI is > disabled it is important to keep a low profile and not touch anything > global. With some early hardware revision, even a DMB here was harmful. It would be useful if there was a comment in the code explaining this. As it stands, you _will_ get a stream of patches from kernel janitors itching to add cpu_relax() there. If you're going to do something different from the norm, it _needs_ to definitely be commented and explained.