From: Arnd Bergmann <arnd@arndb.de>
To: "Russell King - ARM Linux" <linux@arm.linux.org.uk>
Cc: linux-arm-kernel@lists.infradead.org,
Sascha Hauer <s.hauer@pengutronix.de>,
"Zhang Lily-R58066" <r58066@freescale.com>,
linux-fbdev@vger.kernel.org,
Arnaud Patard <arnaud.patard@rtp-net.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/9] ARM i.MX51: Add ipu clock support
Date: Wed, 15 Dec 2010 17:49:59 +0100 [thread overview]
Message-ID: <201012151749.59488.arnd@arndb.de> (raw)
In-Reply-To: <20101215163445.GE9937@n2100.arm.linux.org.uk>
On Wednesday 15 December 2010, Russell King - ARM Linux wrote:
> > The regular accessor function for I/O registers is readl, which handles
> > the access correctly with regard to atomicity, I/O ordering and byteorder.
>
> There's no possibility of those two being mis-ordered - they will be in
> program order whatever.
>
> What isn't guaranteed is the ordering between I/O accesses (accesses to
> device memory) and SDRAM accesses (normal memory) which can pass each other
> without additional barriers. Memory accesses can pass I/O accesses.
Yes, that's what I meant.
> If you don't need normal vs device access ordering, using readl_relaxed()/
> writel_relaxed() is preferred, and avoids the (apparantly rather high)
> performance overhead of having to issue barriers all the way down to the
> L2 cache.
Well, my point was that the authors should choose their I/O accessors
carefully. Using __raw_writel() without any explanations is a rather
bad default, it's not designed for that. Using writel() as a default
is usually a good choice, as we can assume it to do the right thing.
writel_relaxed() is also good where appropriate, because it tells
the reader that the driver author has thought about the I/O (vs. code)
ordering and concluded that it's safe to do.
> Lastly, I don't see where atomicity comes into it - __raw_writel vs writel
> have the same atomicity. Both are single access atomic provided they're
> naturally aligned. Misaligned device accesses are not predictable.
This is just what gcc turns it into today. In theory, a future gcc or
a future cpu might change that. If you mark a pointer as
'__attribute__((packed))', it probably already does, even for aligned
pointers, while it does not when using writel_{,relaxed}. The point is
that __raw_* means just that -- we don't give any guarantees on what
happens on the bus, so people should not use it.
Arnd
WARNING: multiple messages have this Message-ID (diff)
From: Arnd Bergmann <arnd@arndb.de>
To: linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH 1/9] ARM i.MX51: Add ipu clock support
Date: Wed, 15 Dec 2010 16:49:59 +0000 [thread overview]
Message-ID: <201012151749.59488.arnd@arndb.de> (raw)
In-Reply-To: <20101215163445.GE9937@n2100.arm.linux.org.uk>
On Wednesday 15 December 2010, Russell King - ARM Linux wrote:
> > The regular accessor function for I/O registers is readl, which handles
> > the access correctly with regard to atomicity, I/O ordering and byteorder.
>
> There's no possibility of those two being mis-ordered - they will be in
> program order whatever.
>
> What isn't guaranteed is the ordering between I/O accesses (accesses to
> device memory) and SDRAM accesses (normal memory) which can pass each other
> without additional barriers. Memory accesses can pass I/O accesses.
Yes, that's what I meant.
> If you don't need normal vs device access ordering, using readl_relaxed()/
> writel_relaxed() is preferred, and avoids the (apparantly rather high)
> performance overhead of having to issue barriers all the way down to the
> L2 cache.
Well, my point was that the authors should choose their I/O accessors
carefully. Using __raw_writel() without any explanations is a rather
bad default, it's not designed for that. Using writel() as a default
is usually a good choice, as we can assume it to do the right thing.
writel_relaxed() is also good where appropriate, because it tells
the reader that the driver author has thought about the I/O (vs. code)
ordering and concluded that it's safe to do.
> Lastly, I don't see where atomicity comes into it - __raw_writel vs writel
> have the same atomicity. Both are single access atomic provided they're
> naturally aligned. Misaligned device accesses are not predictable.
This is just what gcc turns it into today. In theory, a future gcc or
a future cpu might change that. If you mark a pointer as
'__attribute__((packed))', it probably already does, even for aligned
pointers, while it does not when using writel_{,relaxed}. The point is
that __raw_* means just that -- we don't give any guarantees on what
happens on the bus, so people should not use it.
Arnd
WARNING: multiple messages have this Message-ID (diff)
From: arnd@arndb.de (Arnd Bergmann)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 1/9] ARM i.MX51: Add ipu clock support
Date: Wed, 15 Dec 2010 17:49:59 +0100 [thread overview]
Message-ID: <201012151749.59488.arnd@arndb.de> (raw)
In-Reply-To: <20101215163445.GE9937@n2100.arm.linux.org.uk>
On Wednesday 15 December 2010, Russell King - ARM Linux wrote:
> > The regular accessor function for I/O registers is readl, which handles
> > the access correctly with regard to atomicity, I/O ordering and byteorder.
>
> There's no possibility of those two being mis-ordered - they will be in
> program order whatever.
>
> What isn't guaranteed is the ordering between I/O accesses (accesses to
> device memory) and SDRAM accesses (normal memory) which can pass each other
> without additional barriers. Memory accesses can pass I/O accesses.
Yes, that's what I meant.
> If you don't need normal vs device access ordering, using readl_relaxed()/
> writel_relaxed() is preferred, and avoids the (apparantly rather high)
> performance overhead of having to issue barriers all the way down to the
> L2 cache.
Well, my point was that the authors should choose their I/O accessors
carefully. Using __raw_writel() without any explanations is a rather
bad default, it's not designed for that. Using writel() as a default
is usually a good choice, as we can assume it to do the right thing.
writel_relaxed() is also good where appropriate, because it tells
the reader that the driver author has thought about the I/O (vs. code)
ordering and concluded that it's safe to do.
> Lastly, I don't see where atomicity comes into it - __raw_writel vs writel
> have the same atomicity. Both are single access atomic provided they're
> naturally aligned. Misaligned device accesses are not predictable.
This is just what gcc turns it into today. In theory, a future gcc or
a future cpu might change that. If you mark a pointer as
'__attribute__((packed))', it probably already does, even for aligned
pointers, while it does not when using writel_{,relaxed}. The point is
that __raw_* means just that -- we don't give any guarantees on what
happens on the bus, so people should not use it.
Arnd
next prev parent reply other threads:[~2010-12-15 16:50 UTC|newest]
Thread overview: 110+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-12-09 13:47 [PATCH RFC] i.MX51 Framebuffer support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 1/9] ARM i.MX51: Add ipu clock support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-15 15:40 ` Arnd Bergmann
2010-12-15 15:40 ` Arnd Bergmann
2010-12-15 15:40 ` Arnd Bergmann
2010-12-15 16:34 ` Russell King - ARM Linux
2010-12-15 16:34 ` Russell King - ARM Linux
2010-12-15 16:34 ` Russell King - ARM Linux
2010-12-15 16:49 ` Arnd Bergmann [this message]
2010-12-15 16:49 ` Arnd Bergmann
2010-12-15 16:49 ` Arnd Bergmann
2010-12-15 17:12 ` Russell King - ARM Linux
2010-12-15 17:12 ` Russell King - ARM Linux
2010-12-15 17:12 ` Russell King - ARM Linux
2010-12-09 13:47 ` [PATCH 2/9] ARM i.MX51: rename IPU irqs Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 14:34 ` Uwe Kleine-König
2010-12-09 14:34 ` Uwe Kleine-König
2010-12-09 14:34 `
2010-12-09 13:47 ` [PATCH 3/9] Add a mfd IPUv3 driver Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-12 5:21 ` Liu Ying
2010-12-12 5:21 ` Liu Ying
2010-12-13 11:23 ` Sascha Hauer
2010-12-13 11:23 ` Sascha Hauer
2010-12-13 11:23 ` Sascha Hauer
2010-12-14 4:05 ` Liu Ying
2010-12-14 4:05 ` Liu Ying
2010-12-14 4:05 ` Liu Ying
2010-12-14 8:40 ` Sascha Hauer
2010-12-14 8:40 ` Sascha Hauer
2010-12-14 8:40 ` Sascha Hauer
2010-12-14 13:13 ` Liu Ying
2010-12-14 13:13 ` Liu Ying
2010-12-14 13:13 ` Liu Ying
2010-12-09 13:47 ` [PATCH 4/9] fb: export fb mode db table Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2011-01-06 7:26 ` Paul Mundt
2011-01-06 7:26 ` Paul Mundt
2011-01-06 7:26 ` Paul Mundt
2011-01-06 10:04 ` Sascha Hauer
2011-01-06 10:04 ` Sascha Hauer
2011-01-06 10:04 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 5/9] Add i.MX5 framebuffer driver Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-12 6:13 ` Liu Ying
2010-12-12 6:13 ` Liu Ying
2010-12-12 6:13 ` Liu Ying
2010-12-13 7:23 ` Lothar Waßmann
2010-12-13 7:23 ` Lothar Waßmann
2010-12-13 7:23 ` Lothar Waßmann
2010-12-13 11:35 ` Liu Ying
2010-12-13 11:35 ` Liu Ying
2010-12-13 11:35 ` Liu Ying
2010-12-13 11:38 ` Sascha Hauer
2010-12-13 11:38 ` Sascha Hauer
2010-12-13 11:38 ` Sascha Hauer
2010-12-14 6:40 ` Liu Ying
2010-12-14 6:40 ` Liu Ying
2010-12-14 6:40 ` Liu Ying
2010-12-14 8:45 ` Sascha Hauer
2010-12-14 8:45 ` Sascha Hauer
2010-12-14 8:45 ` Sascha Hauer
2010-12-14 13:23 ` Liu Ying
2010-12-14 13:23 ` Liu Ying
2010-12-14 13:23 ` Liu Ying
2010-12-15 11:17 ` Sascha Hauer
2010-12-15 11:17 ` Sascha Hauer
2010-12-15 11:17 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 6/9] ARM i.MX51: Add IPU device support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-15 15:49 ` Arnd Bergmann
2010-12-15 15:49 ` Arnd Bergmann
2010-12-15 15:49 ` Arnd Bergmann
2010-12-15 16:26 ` Arnaud Patard
2010-12-15 16:26 ` Arnaud Patard (Rtp)
2010-12-15 16:26 ` Arnaud Patard
2010-12-15 16:29 ` Arnd Bergmann
2010-12-15 16:29 ` Arnd Bergmann
2010-12-15 16:29 ` Arnd Bergmann
2010-12-09 13:47 ` [PATCH 7/9] ARM i.MX5: Allow to increase max zone order Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 8/9] ARM i.MX5: increase dma consistent size for IPU support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` [PATCH 9/9] ARM i.MX51 babbage: Add framebuffer support Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-09 13:47 ` Sascha Hauer
2010-12-12 1:37 ` Liu Ying
2010-12-12 1:37 ` Liu Ying
2010-12-12 1:37 ` Liu Ying
2010-12-13 11:43 ` Sascha Hauer
2010-12-13 11:43 ` Sascha Hauer
2010-12-13 11:43 ` Sascha Hauer
2010-12-14 6:47 ` Liu Ying
2010-12-14 6:47 ` Liu Ying
2010-12-14 6:47 ` Liu Ying
2010-12-15 9:35 ` [PATCH RFC] i.MX51 Framebuffer support Peter Horton
2010-12-15 11:24 ` Sascha Hauer
2010-12-20 10:48 [PATCH v2] " Sascha Hauer
2010-12-20 10:48 ` [PATCH 1/9] ARM i.MX51: Add ipu clock support Sascha Hauer
2010-12-20 10:48 ` Sascha Hauer
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=201012151749.59488.arnd@arndb.de \
--to=arnd@arndb.de \
--cc=arnaud.patard@rtp-net.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux@arm.linux.org.uk \
--cc=r58066@freescale.com \
--cc=s.hauer@pengutronix.de \
/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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.