public inbox for linux-arm-kernel@lists.infradead.org
 help / color / mirror / Atom feed
From: arnd@arndb.de (Arnd Bergmann)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 3/3] reset: socfpga: use readl/writel_relaxed
Date: Tue, 05 Jul 2016 14:59:49 +0200	[thread overview]
Message-ID: <3713699.k1gRikAJNK@wuerfel> (raw)
In-Reply-To: <1467718816.2978.41.camel@pengutronix.de>

On Tuesday, July 5, 2016 1:40:16 PM CEST Philipp Zabel wrote:
> Am Dienstag, den 05.07.2016, 13:20 +0200 schrieb Arnd Bergmann:
> > On Tuesday, July 5, 2016 12:17:52 PM CEST Philipp Zabel wrote:
> > > This just removes the rmb()/wmb() pair between register read and
> > > write. Since no relevant reads follow the rmb and no relevant writes
> > > precede the wmb, they should be safe to remove.
> > > 
> > > Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
> > 
> > We should only do this if you are fixing a bug (which you don't mention
> > in the changelog), or if you can show a relevant performance
> > improvement. Is this code ever used in a fast path? If it is,
> > wouldn't that indicate a problem in some driver?
> 
> It does not fix a bug, and it's not about performance either. I'd like
> to align code with the recently posted stm32 driver, to unify them in a
> future patch.
> Of course we can change the stm32 driver to use readl/writel instead of
> the relaxed variants, it just seemed useless to have those barriers
> between the read and write.

On stm32, there is no barrier because ARM_DMA_MEM_BUFFERABLE is not set.

I'd really prefer to just have readl/writel everywhere except in the
few places that are performance critical and have a comment explaining
why it's safe there, mainly to avoid having new developers blindly
add the relaxed accessors in drivers because they think it's the
normal coding style.

> If anything, we'd need to try to make sure that the writel in assert
> hits the hardware before the function returns, so that a
> assert-delay-deassert doesn't accidentally spend half its delay with the
> writel still in the store buffer, and we'd need a full barrier after the
> writel in deassert so that there can be no successive reads from still
> disabled IP cores.

In general, I think you need a readl() following the writel() to guarantee
that it has actually hit the hardware.

On ARM you often have just the CPU write buffer that needs to be flushed,
but if you have a PCI device or a more complex SoC, then a barriers doesn't
wait for a write to arrive at the device, it only ensures that a subsequent
write cannot arrive any earlier.

	Arnd

  reply	other threads:[~2016-07-05 12:59 UTC|newest]

Thread overview: 9+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-07-05 10:17 [PATCH 1/3] reset: socfpga: no need to store modrst_offset Philipp Zabel
2016-07-05 10:17 ` [PATCH 2/3] reset: sunxi: use readl/writel_relaxed Philipp Zabel
2016-07-05 10:17 ` [PATCH 3/3] reset: socfpga: " Philipp Zabel
2016-07-05 11:20   ` Arnd Bergmann
2016-07-05 11:40     ` Philipp Zabel
2016-07-05 12:59       ` Arnd Bergmann [this message]
2016-07-05 13:20         ` Philipp Zabel
2016-07-06 16:06 ` [PATCH 1/3] reset: socfpga: no need to store modrst_offset Dinh Nguyen
2016-07-06 16:26   ` Philipp Zabel

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=3713699.k1gRikAJNK@wuerfel \
    --to=arnd@arndb.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox