* [PATCH 1/2] asm-generic: io: remove {read,write} string functions
@ 2012-04-27 10:42 Will Deacon
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
` (3 more replies)
0 siblings, 4 replies; 20+ messages in thread
From: Will Deacon @ 2012-04-27 10:42 UTC (permalink / raw)
To: linux-kernel
Cc: uclinux-dist-devel, Will Deacon, Arnd Bergmann, Mike Frysinger
The {read,write}s{b,w,l} functions are not defined across all
architectures and therefore shouldn't be used by portable drivers. We
should encourage driver writers to use the io{read,write}{8,16,32}_rep
functions instead.
This patch removes the {read,write} string functions for the generic IO
header as they have no place in a new architecture port.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
include/asm-generic/io.h | 30 ------------------------------
1 files changed, 0 insertions(+), 30 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 448303b..3607921 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -217,36 +217,6 @@ static inline void outsl(unsigned long addr, const void *buffer, int count)
}
#endif
-static inline void readsl(const void __iomem *addr, void *buf, int len)
-{
- insl(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void readsw(const void __iomem *addr, void *buf, int len)
-{
- insw(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void readsb(const void __iomem *addr, void *buf, int len)
-{
- insb(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesl(const void __iomem *addr, const void *buf, int len)
-{
- outsl(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesw(const void __iomem *addr, const void *buf, int len)
-{
- outsw(addr - PCI_IOBASE, buf, len);
-}
-
-static inline void writesb(const void __iomem *addr, const void *buf, int len)
-{
- outsb(addr - PCI_IOBASE, buf, len);
-}
-
#ifndef CONFIG_GENERIC_IOMAP
#define ioread8(addr) readb(addr)
#define ioread16(addr) readw(addr)
--
1.7.4.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions
2012-04-27 10:42 [PATCH 1/2] asm-generic: io: remove {read,write} string functions Will Deacon
@ 2012-04-27 10:42 ` Will Deacon
2012-04-27 12:12 ` Arnd Bergmann
2012-04-27 16:18 ` Mike Frysinger
2012-04-27 12:12 ` [PATCH 1/2] asm-generic: io: remove {read,write} " Arnd Bergmann
` (2 subsequent siblings)
3 siblings, 2 replies; 20+ messages in thread
From: Will Deacon @ 2012-04-27 10:42 UTC (permalink / raw)
To: linux-kernel
Cc: uclinux-dist-devel, Will Deacon, Arnd Bergmann, Mike Frysinger
The {in,out}s{b,w,l} functions are designed to operate on a stream of
bytes and therefore should not perform any byte-swapping, regardless of
the CPU byte order.
This patch fixes the generic IO header so that {in,out}s{b,w,l} call
the __raw_{read,write} functions directly rather than going via the
endian-correcting accessors.
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Will Deacon <will.deacon@arm.com>
---
include/asm-generic/io.h | 12 ++++++------
1 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index 3607921..403b861 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -148,7 +148,7 @@ static inline void insb(unsigned long addr, void *buffer, int count)
if (count) {
u8 *buf = buffer;
do {
- u8 x = inb(addr);
+ u8 x = __raw_readb(addr + PCI_IOBASE);
*buf++ = x;
} while (--count);
}
@@ -161,7 +161,7 @@ static inline void insw(unsigned long addr, void *buffer, int count)
if (count) {
u16 *buf = buffer;
do {
- u16 x = inw(addr);
+ u16 x = __raw_readw(addr + PCI_IOBASE);
*buf++ = x;
} while (--count);
}
@@ -174,7 +174,7 @@ static inline void insl(unsigned long addr, void *buffer, int count)
if (count) {
u32 *buf = buffer;
do {
- u32 x = inl(addr);
+ u32 x = __raw_readl(addr + PCI_IOBASE);
*buf++ = x;
} while (--count);
}
@@ -187,7 +187,7 @@ static inline void outsb(unsigned long addr, const void *buffer, int count)
if (count) {
const u8 *buf = buffer;
do {
- outb(*buf++, addr);
+ __raw_writeb(*buf++, addr + PCI_IOBASE);
} while (--count);
}
}
@@ -199,7 +199,7 @@ static inline void outsw(unsigned long addr, const void *buffer, int count)
if (count) {
const u16 *buf = buffer;
do {
- outw(*buf++, addr);
+ __raw_writew(*buf++, addr + PCI_IOBASE);
} while (--count);
}
}
@@ -211,7 +211,7 @@ static inline void outsl(unsigned long addr, const void *buffer, int count)
if (count) {
const u32 *buf = buffer;
do {
- outl(*buf++, addr);
+ __raw_writel(*buf++, addr + PCI_IOBASE);
} while (--count);
}
}
--
1.7.4.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 10:42 [PATCH 1/2] asm-generic: io: remove {read,write} string functions Will Deacon
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
@ 2012-04-27 12:12 ` Arnd Bergmann
2012-04-27 16:17 ` Mike Frysinger
2012-04-27 16:23 ` H. Peter Anvin
3 siblings, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2012-04-27 12:12 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Mike Frysinger
On Friday 27 April 2012, Will Deacon wrote:
> The {read,write}s{b,w,l} functions are not defined across all
> architectures and therefore shouldn't be used by portable drivers. We
> should encourage driver writers to use the io{read,write}{8,16,32}_rep
> functions instead.
>
> This patch removes the {read,write} string functions for the generic IO
> header as they have no place in a new architecture port.
I think we need to put these functions back into the blackfin version,
basically reverting efb2d31c1c, but otherwise the patch looks good
to me.
Arnd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
@ 2012-04-27 12:12 ` Arnd Bergmann
2012-04-27 16:18 ` Mike Frysinger
1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2012-04-27 12:12 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Mike Frysinger
On Friday 27 April 2012, Will Deacon wrote:
> The {in,out}s{b,w,l} functions are designed to operate on a stream of
> bytes and therefore should not perform any byte-swapping, regardless of
> the CPU byte order.
>
> This patch fixes the generic IO header so that {in,out}s{b,w,l} call
> the __raw_{read,write} functions directly rather than going via the
> endian-correcting accessors.
>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Will Deacon <will.deacon@arm.com>
Acked-by: Arnd Bergmann <arnd@arndb.de>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 10:42 [PATCH 1/2] asm-generic: io: remove {read,write} string functions Will Deacon
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
2012-04-27 12:12 ` [PATCH 1/2] asm-generic: io: remove {read,write} " Arnd Bergmann
@ 2012-04-27 16:17 ` Mike Frysinger
2012-04-27 16:53 ` Will Deacon
2012-04-27 20:29 ` H. Peter Anvin
2012-04-27 16:23 ` H. Peter Anvin
3 siblings, 2 replies; 20+ messages in thread
From: Mike Frysinger @ 2012-04-27 16:17 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
[-- Attachment #1: Type: Text/Plain, Size: 933 bytes --]
On Friday 27 April 2012 06:42:55 Will Deacon wrote:
> The {read,write}s{b,w,l} functions are not defined across all
> architectures and therefore shouldn't be used by portable drivers. We
> should encourage driver writers to use the io{read,write}{8,16,32}_rep
> functions instead.
well, that isn't true today as a grep of the drivers/ tree shows. perhaps we
should fix that first ? quite a number of architectures do implement these.
> This patch removes the {read,write} string functions for the generic IO
> header as they have no place in a new architecture port.
i don't see any file anywhere that describes what the baseline API is supposed
to be, and what each set of funcs are for. there is just the random ugliness
in each arch's asm/io.h cobbled together until things work. i think that also
needs to be addressed before we go extending/contracting the API provides by
asm-generic/io.h.
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
2012-04-27 12:12 ` Arnd Bergmann
@ 2012-04-27 16:18 ` Mike Frysinger
2012-04-27 16:59 ` Will Deacon
1 sibling, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-04-27 16:18 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
[-- Attachment #1: Type: Text/Plain, Size: 511 bytes --]
On Friday 27 April 2012 06:42:56 Will Deacon wrote:
> The {in,out}s{b,w,l} functions are designed to operate on a stream of
> bytes and therefore should not perform any byte-swapping, regardless of
> the CPU byte order.
says who ? where's the agreed upon documentation for this ?
(i'm not disagreeing with you, just that there lacks justification here, and
someone can just as easily post another patch saying the opposite of what you
said and there's nothing stopping us from merging it)
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 10:42 [PATCH 1/2] asm-generic: io: remove {read,write} string functions Will Deacon
` (2 preceding siblings ...)
2012-04-27 16:17 ` Mike Frysinger
@ 2012-04-27 16:23 ` H. Peter Anvin
2012-04-27 17:14 ` Will Deacon
3 siblings, 1 reply; 20+ messages in thread
From: H. Peter Anvin @ 2012-04-27 16:23 UTC (permalink / raw)
To: Will Deacon
Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann, Mike Frysinger
On 04/27/2012 03:42 AM, Will Deacon wrote:
> The {read,write}s{b,w,l} functions are not defined across all
> architectures and therefore shouldn't be used by portable drivers. We
> should encourage driver writers to use the io{read,write}{8,16,32}_rep
> functions instead.
Why is that? ioread/write are great if the address space is undefined,
but do we need the extra overhead in any other cases?
-hpa
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 16:17 ` Mike Frysinger
@ 2012-04-27 16:53 ` Will Deacon
2012-04-27 17:18 ` Mike Frysinger
2012-04-28 8:46 ` Geert Uytterhoeven
2012-04-27 20:29 ` H. Peter Anvin
1 sibling, 2 replies; 20+ messages in thread
From: Will Deacon @ 2012-04-27 16:53 UTC (permalink / raw)
To: Mike Frysinger; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
Hi Mike,
On Fri, Apr 27, 2012 at 05:17:47PM +0100, Mike Frysinger wrote:
> On Friday 27 April 2012 06:42:55 Will Deacon wrote:
> > The {read,write}s{b,w,l} functions are not defined across all
> > architectures and therefore shouldn't be used by portable drivers. We
> > should encourage driver writers to use the io{read,write}{8,16,32}_rep
> > functions instead.
>
> well, that isn't true today as a grep of the drivers/ tree shows. perhaps we
> should fix that first ? quite a number of architectures do implement these.
Sure, and architectures can continue to implement these if they're using
drivers that require them (I'll reintroduce them for blackfin). They
shouldn't be used for new architectures though, so any drivers that are
required by folks using asm-generic/io.h will need converting.
> > This patch removes the {read,write} string functions for the generic IO
> > header as they have no place in a new architecture port.
>
> i don't see any file anywhere that describes what the baseline API is supposed
> to be, and what each set of funcs are for. there is just the random ugliness
> in each arch's asm/io.h cobbled together until things work. i think that also
> needs to be addressed before we go extending/contracting the API provides by
> asm-generic/io.h.
I agree that it's a bit of a mess, but if we can keep the asm-generic
interfaces clean then it can help driver writers see what we have as a
common base. If a driver is not portable (maybe for some
architecture-specific device) then they could use extra accessors if they
really need to.
The point of removing these functions is to make it clear that they're not
part of the baseline API and instead encourage people to use other accessors
instead if they want to write portable code.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions
2012-04-27 16:18 ` Mike Frysinger
@ 2012-04-27 16:59 ` Will Deacon
2012-04-27 17:26 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Will Deacon @ 2012-04-27 16:59 UTC (permalink / raw)
To: Mike Frysinger; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
On Fri, Apr 27, 2012 at 05:18:58PM +0100, Mike Frysinger wrote:
> On Friday 27 April 2012 06:42:56 Will Deacon wrote:
> > The {in,out}s{b,w,l} functions are designed to operate on a stream of
> > bytes and therefore should not perform any byte-swapping, regardless of
> > the CPU byte order.
>
> says who ? where's the agreed upon documentation for this ?
This specific case is actually documented in Linux Device Drivers, but I
appreciate that it's not especially clear. I had some offline discussion
with Arnd where we agreed on this -- it also means that asm-generic/io.h
matches what is done by bi-endian architectures providing their own
accessors.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 16:23 ` H. Peter Anvin
@ 2012-04-27 17:14 ` Will Deacon
2012-04-27 17:53 ` H. Peter Anvin
0 siblings, 1 reply; 20+ messages in thread
From: Will Deacon @ 2012-04-27 17:14 UTC (permalink / raw)
To: H. Peter Anvin
Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann, Mike Frysinger
On Fri, Apr 27, 2012 at 05:23:34PM +0100, H. Peter Anvin wrote:
> On 04/27/2012 03:42 AM, Will Deacon wrote:
> > The {read,write}s{b,w,l} functions are not defined across all
> > architectures and therefore shouldn't be used by portable drivers. We
> > should encourage driver writers to use the io{read,write}{8,16,32}_rep
> > functions instead.
>
> Why is that? ioread/write are great if the address space is undefined,
> but do we need the extra overhead in any other cases?
I think it's because in reality not many drivers are using
the {read,write}s{b,w,l} accessors:
drivers/bluetooth/btmrvl_sdio.c
drivers/bluetooth/btsdio.c
drivers/i2c/busses/i2c-tegra.c
drivers/mmc/core/sdio_io.c
drivers/mmc/host/msm_sdcc.c
drivers/mmc/host/mvsdio.c
drivers/mmc/host/omap.c
drivers/mmc/host/tmio_mmc.h
drivers/mtd/nand/atmel_nand.c
drivers/mtd/nand/gpio.c
drivers/mtd/nand/omap2.c
drivers/mtd/nand/pxa3xx_nand.c
drivers/mtd/nand/s3c2410.c
drivers/net/ethernet/8390/ax88796.c
drivers/net/ethernet/8390/etherh.c
drivers/net/ethernet/davicom/dm9000.c
drivers/net/ethernet/seeq/ether3.c
drivers/net/ethernet/smsc/smc911x.h
drivers/net/ethernet/smsc/smc91x.h
drivers/net/ethernet/smsc/smsc911x.c
drivers/net/wireless/ath/ath6kl/sdio.c
drivers/net/wireless/brcm80211/brcmfmac/bcmsdh_sdmmc.c
drivers/net/wireless/libertas/if_sdio.c
drivers/net/wireless/mwifiex/sdio.c
drivers/net/wireless/wl12xx/sdio.c
drivers/parport/parport_ip32.c
drivers/pci/hotplug/ibmphp_core.c
drivers/pci/hotplug/ibmphp_ebda.c
drivers/pci/hotplug/ibmphp.h
drivers/pci/hotplug/ibmphp_hpc.c
drivers/scsi/arm/arxescsi.c
drivers/scsi/arm/cumana_2.c
drivers/scsi/arm/oak.c
drivers/ssb/sdio.c
drivers/usb/gadget/at91_udc.c
drivers/usb/gadget/s3c2410_udc.c
drivers/usb/gadget/s3c-hsotg.c
drivers/usb/host/isp1362.h
drivers/usb/musb/am35x.c
drivers/usb/musb/musb_core.c
drivers/usb/musb/musb_io.h
drivers/usb/musb/tusb6010.c
If you remove the architecture-specific drivers, there's really not a lot left
and, even then, we only need to convert those drivers which are intended to
be portable between architectures where the string functions are not
consistently available.
By overheads, I assume you're referring to the IO_COND check on the address
space? I wouldn't expect this to be noticeable compared to the cost of the
I/O access and I'm not sure it's worth worrying about for the sake of the
small number of drivers affected.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 16:53 ` Will Deacon
@ 2012-04-27 17:18 ` Mike Frysinger
2012-04-28 8:46 ` Geert Uytterhoeven
1 sibling, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2012-04-27 17:18 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
[-- Attachment #1: Type: Text/Plain, Size: 2709 bytes --]
On Friday 27 April 2012 12:53:06 Will Deacon wrote:
> On Fri, Apr 27, 2012 at 05:17:47PM +0100, Mike Frysinger wrote:
> > On Friday 27 April 2012 06:42:55 Will Deacon wrote:
> > > The {read,write}s{b,w,l} functions are not defined across all
> > > architectures and therefore shouldn't be used by portable drivers. We
> > > should encourage driver writers to use the io{read,write}{8,16,32}_rep
> > > functions instead.
> >
> > well, that isn't true today as a grep of the drivers/ tree shows.
> > perhaps we should fix that first ? quite a number of architectures do
> > implement these.
>
> Sure, and architectures can continue to implement these if they're using
> drivers that require them (I'll reintroduce them for blackfin). They
> shouldn't be used for new architectures though, so any drivers that are
> required by folks using asm-generic/io.h will need converting.
will they though ? or without documentation, will they simply copy & paste
the interfaces from another arch to build the driver ? most people doing arch
ports (especially new ones) don't have the experience or confidence to push
back and update the driver rather than squirreling away 6 #define's into their
arch-specific asm/io.h.
> > > This patch removes the {read,write} string functions for the generic IO
> > > header as they have no place in a new architecture port.
> >
> > i don't see any file anywhere that describes what the baseline API is
> > supposed to be, and what each set of funcs are for. there is just the
> > random ugliness in each arch's asm/io.h cobbled together until things
> > work. i think that also needs to be addressed before we go
> > extending/contracting the API provides by asm-generic/io.h.
>
> I agree that it's a bit of a mess, but if we can keep the asm-generic
> interfaces clean then it can help driver writers see what we have as a
> common base. If a driver is not portable (maybe for some
> architecture-specific device) then they could use extra accessors if they
> really need to.
>
> The point of removing these functions is to make it clear that they're not
> part of the baseline API and instead encourage people to use other
> accessors instead if they want to write portable code.
maybe i'm pessimistic, but without addressing the underlying problem (writing
clear documentation on what should exist, what they're for, and specifically
what functions *shouldn't* exist), we're just playing a losing game of whack-
a-mole. you remove an interface that appears to be mostly common, then
someone else posts a patch to add some other set of interfaces and people
don't notice, and we just keep flip-flopping.
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions
2012-04-27 16:59 ` Will Deacon
@ 2012-04-27 17:26 ` Mike Frysinger
0 siblings, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2012-04-27 17:26 UTC (permalink / raw)
To: Will Deacon; +Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann
[-- Attachment #1: Type: Text/Plain, Size: 1018 bytes --]
On Friday 27 April 2012 12:59:19 Will Deacon wrote:
> On Fri, Apr 27, 2012 at 05:18:58PM +0100, Mike Frysinger wrote:
> > On Friday 27 April 2012 06:42:56 Will Deacon wrote:
> > > The {in,out}s{b,w,l} functions are designed to operate on a stream of
> > > bytes and therefore should not perform any byte-swapping, regardless of
> > > the CPU byte order.
> >
> > says who ? where's the agreed upon documentation for this ?
>
> This specific case is actually documented in Linux Device Drivers
if it isn't in the tree (either Documentation/ or in asm-generic/io.h itself),
i don't think it's acceptable to refer to it as guiding documentation
> appreciate that it's not especially clear. I had some offline discussion
> with Arnd where we agreed on this -- it also means that asm-generic/io.h
> matches what is done by bi-endian architectures providing their own
> accessors.
please add start adding comments above these functions that clearly spell out
the expectations and behavior
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 17:14 ` Will Deacon
@ 2012-04-27 17:53 ` H. Peter Anvin
2012-04-27 20:52 ` Arnd Bergmann
0 siblings, 1 reply; 20+ messages in thread
From: H. Peter Anvin @ 2012-04-27 17:53 UTC (permalink / raw)
To: Will Deacon
Cc: linux-kernel, uclinux-dist-devel, Arnd Bergmann, Mike Frysinger
On 04/27/2012 10:14 AM, Will Deacon wrote:
>
> If you remove the architecture-specific drivers, there's really not a lot left
> and, even then, we only need to convert those drivers which are intended to
> be portable between architectures where the string functions are not
> consistently available.
>
> By overheads, I assume you're referring to the IO_COND check on the address
> space? I wouldn't expect this to be noticeable compared to the cost of the
> I/O access and I'm not sure it's worth worrying about for the sake of the
> small number of drivers affected.
>
It's not in time, but it adds bulk to the code. The point is that what
is the benefit of not making these part of the general API? For
architectures where the address space doesn't matter they could just
alias it to the same functions, or use the generic versions.
-hpa
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 16:17 ` Mike Frysinger
2012-04-27 16:53 ` Will Deacon
@ 2012-04-27 20:29 ` H. Peter Anvin
1 sibling, 0 replies; 20+ messages in thread
From: H. Peter Anvin @ 2012-04-27 20:29 UTC (permalink / raw)
To: Mike Frysinger
Cc: Will Deacon, linux-kernel, uclinux-dist-devel, Arnd Bergmann
On 04/27/2012 09:17 AM, Mike Frysinger wrote:
> On Friday 27 April 2012 06:42:55 Will Deacon wrote:
>> The {read,write}s{b,w,l} functions are not defined across all
>> architectures and therefore shouldn't be used by portable
>> drivers. We should encourage driver writers to use the
>> io{read,write}{8,16,32}_rep functions instead.
>
> well, that isn't true today as a grep of the drivers/ tree shows.
> perhaps we should fix that first ? quite a number of architectures
> do implement these.
>
>> This patch removes the {read,write} string functions for the
>> generic IO header as they have no place in a new architecture
>> port.
>
> i don't see any file anywhere that describes what the baseline API
> is supposed to be, and what each set of funcs are for. there is
> just the random ugliness in each arch's asm/io.h cobbled together
> until things work. i think that also needs to be addressed before
> we go extending/contracting the API provides by asm-generic/io.h.
And it's is much bigger than asm-generic/io.h, as evidenced by some
other recent events.
For example, we need streaming I/O (memory and I/O) with specific
access sizes, for example.
-hpa
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 17:53 ` H. Peter Anvin
@ 2012-04-27 20:52 ` Arnd Bergmann
2012-04-27 23:26 ` Mike Frysinger
0 siblings, 1 reply; 20+ messages in thread
From: Arnd Bergmann @ 2012-04-27 20:52 UTC (permalink / raw)
To: H. Peter Anvin
Cc: Will Deacon, linux-kernel, uclinux-dist-devel, Mike Frysinger
On Friday 27 April 2012, H. Peter Anvin wrote:
> On 04/27/2012 10:14 AM, Will Deacon wrote:
> >
> > If you remove the architecture-specific drivers, there's really not a lot left
> > and, even then, we only need to convert those drivers which are intended to
> > be portable between architectures where the string functions are not
> > consistently available.
> >
> > By overheads, I assume you're referring to the IO_COND check on the address
> > space? I wouldn't expect this to be noticeable compared to the cost of the
> > I/O access and I'm not sure it's worth worrying about for the sake of the
> > small number of drivers affected.
> >
>
> It's not in time, but it adds bulk to the code. The point is that what
> is the benefit of not making these part of the general API? For
> architectures where the address space doesn't matter they could just
> alias it to the same functions, or use the generic versions.
>
The main reasons I can see for not making it a general purpose API are:
* It's a very confusing interface, because the endianess rules are different
from the non-string variants and counterintuitive.
* Almost all the users are ancient ARM specific drivers, the others are
either new ARM specific drivers or drivers that started out as ARM-only
and were ported later to other architectures (sh, avr32, mips, mn10300
and blackfin)
* On all these architectures, the PCI I/O space is memory mapped (or
non-existent), so the ioread* functions are trivial wrappers without
additional overhead.
* Most architectures don't implement them today, but all architectures
that support MMIO also implement the ioread string operations.
Arnd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 20:52 ` Arnd Bergmann
@ 2012-04-27 23:26 ` Mike Frysinger
2012-05-01 13:34 ` Will Deacon
0 siblings, 1 reply; 20+ messages in thread
From: Mike Frysinger @ 2012-04-27 23:26 UTC (permalink / raw)
To: Arnd Bergmann
Cc: H. Peter Anvin, Will Deacon, linux-kernel, uclinux-dist-devel
[-- Attachment #1: Type: Text/Plain, Size: 2310 bytes --]
On Friday 27 April 2012 16:52:34 Arnd Bergmann wrote:
> On Friday 27 April 2012, H. Peter Anvin wrote:
> > On 04/27/2012 10:14 AM, Will Deacon wrote:
> > > If you remove the architecture-specific drivers, there's really not a
> > > lot left and, even then, we only need to convert those drivers which
> > > are intended to be portable between architectures where the string
> > > functions are not consistently available.
> > >
> > > By overheads, I assume you're referring to the IO_COND check on the
> > > address space? I wouldn't expect this to be noticeable compared to the
> > > cost of the I/O access and I'm not sure it's worth worrying about for
> > > the sake of the small number of drivers affected.
> >
> > It's not in time, but it adds bulk to the code. The point is that what
> > is the benefit of not making these part of the general API? For
> > architectures where the address space doesn't matter they could just
> > alias it to the same functions, or use the generic versions.
>
> The main reasons I can see for not making it a general purpose API are:
>
> * It's a very confusing interface, because the endianess rules are
> different from the non-string variants and counterintuitive.
>
> * Almost all the users are ancient ARM specific drivers, the others are
> either new ARM specific drivers or drivers that started out as ARM-only
> and were ported later to other architectures (sh, avr32, mips, mn10300
> and blackfin)
>
> * On all these architectures, the PCI I/O space is memory mapped (or
> non-existent), so the ioread* functions are trivial wrappers without
> additional overhead.
>
> * Most architectures don't implement them today, but all architectures
> that support MMIO also implement the ioread string operations.
i'm ambivalent with keeping or tossing these funcs. Blackfin (afaik) picked
them up really only for the reasons Arnd cited -- drivers using memory mapped
registers originally for ARM (or m68k/coldfire i think) used this API, and it
was easier for us to implement these defines in our asm/io.h and get the
drivers "for free".
i am strongly in favor though of agreeing on & documenting the baseline API
first before attempting to clean anything up. sorry to keep harping on this.
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 16:53 ` Will Deacon
2012-04-27 17:18 ` Mike Frysinger
@ 2012-04-28 8:46 ` Geert Uytterhoeven
1 sibling, 0 replies; 20+ messages in thread
From: Geert Uytterhoeven @ 2012-04-28 8:46 UTC (permalink / raw)
To: Will Deacon
Cc: Mike Frysinger, linux-kernel, uclinux-dist-devel, Arnd Bergmann
On Fri, Apr 27, 2012 at 18:53, Will Deacon <will.deacon@arm.com> wrote:
> On Fri, Apr 27, 2012 at 05:17:47PM +0100, Mike Frysinger wrote:
>> On Friday 27 April 2012 06:42:55 Will Deacon wrote:
>> > The {read,write}s{b,w,l} functions are not defined across all
>> > architectures and therefore shouldn't be used by portable drivers. We
>> > should encourage driver writers to use the io{read,write}{8,16,32}_rep
>> > functions instead.
>>
>> well, that isn't true today as a grep of the drivers/ tree shows. perhaps we
>> should fix that first ? quite a number of architectures do implement these.
>
> Sure, and architectures can continue to implement these if they're using
> drivers that require them (I'll reintroduce them for blackfin). They
> shouldn't be used for new architectures though, so any drivers that are
> required by folks using asm-generic/io.h will need converting.
Doh, I'm just about to introduce them for m68k for 3.5...
https://lkml.org/lkml/2012/4/21/161
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-04-27 23:26 ` Mike Frysinger
@ 2012-05-01 13:34 ` Will Deacon
2012-05-01 14:40 ` Arnd Bergmann
2012-05-01 18:28 ` Mike Frysinger
0 siblings, 2 replies; 20+ messages in thread
From: Will Deacon @ 2012-05-01 13:34 UTC (permalink / raw)
To: Mike Frysinger
Cc: Arnd Bergmann, H. Peter Anvin, linux-kernel, uclinux-dist-devel
On Sat, Apr 28, 2012 at 12:26:11AM +0100, Mike Frysinger wrote:
> On Friday 27 April 2012 16:52:34 Arnd Bergmann wrote:
> > The main reasons I can see for not making it a general purpose API are:
> >
> > * It's a very confusing interface, because the endianess rules are
> > different from the non-string variants and counterintuitive.
> >
> > * Almost all the users are ancient ARM specific drivers, the others are
> > either new ARM specific drivers or drivers that started out as ARM-only
> > and were ported later to other architectures (sh, avr32, mips, mn10300
> > and blackfin)
> >
> > * On all these architectures, the PCI I/O space is memory mapped (or
> > non-existent), so the ioread* functions are trivial wrappers without
> > additional overhead.
> >
> > * Most architectures don't implement them today, but all architectures
> > that support MMIO also implement the ioread string operations.
>
> i'm ambivalent with keeping or tossing these funcs. Blackfin (afaik) picked
> them up really only for the reasons Arnd cited -- drivers using memory mapped
> registers originally for ARM (or m68k/coldfire i think) used this API, and it
> was easier for us to implement these defines in our asm/io.h and get the
> drivers "for free".
Makes sense and there's no compelling reason to convert drivers if they're
not being used by architectures without these functions.
> i am strongly in favor though of agreeing on & documenting the baseline API
> first before attempting to clean anything up. sorry to keep harping on this.
It would be nice if we could just use asm-generic/io.h as the documention,
i.e. if you want to write a portable driver, just stick to the functions
implemented there. Apart from the string accessors, I think it's already
fairly accurate. The __raw_* functions seem to be missing on s390 but I
don't think driver portability is a huge concern for that architecture.
Will
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-05-01 13:34 ` Will Deacon
@ 2012-05-01 14:40 ` Arnd Bergmann
2012-05-01 18:28 ` Mike Frysinger
1 sibling, 0 replies; 20+ messages in thread
From: Arnd Bergmann @ 2012-05-01 14:40 UTC (permalink / raw)
To: Will Deacon
Cc: Mike Frysinger, H. Peter Anvin, linux-kernel, uclinux-dist-devel
On Tuesday 01 May 2012, Will Deacon wrote:
> It would be nice if we could just use asm-generic/io.h as the documention,
> i.e. if you want to write a portable driver, just stick to the functions
> implemented there. Apart from the string accessors, I think it's already
> fairly accurate. The __raw_* functions seem to be missing on s390 but I
> don't think driver portability is a huge concern for that architecture.
Actually, s390 has none of the mmio accessors right now, because there
is no PCI or other memory mapped hardware.
Arnd
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions
2012-05-01 13:34 ` Will Deacon
2012-05-01 14:40 ` Arnd Bergmann
@ 2012-05-01 18:28 ` Mike Frysinger
1 sibling, 0 replies; 20+ messages in thread
From: Mike Frysinger @ 2012-05-01 18:28 UTC (permalink / raw)
To: Will Deacon
Cc: Arnd Bergmann, H. Peter Anvin, linux-kernel, uclinux-dist-devel
[-- Attachment #1: Type: Text/Plain, Size: 1915 bytes --]
On Tuesday 01 May 2012 09:34:29 Will Deacon wrote:
> On Sat, Apr 28, 2012 at 12:26:11AM +0100, Mike Frysinger wrote:
> > i am strongly in favor though of agreeing on & documenting the baseline
> > API first before attempting to clean anything up. sorry to keep harping
> > on this.
>
> It would be nice if we could just use asm-generic/io.h as the documention,
> i.e. if you want to write a portable driver, just stick to the functions
> implemented there.
we have Documentation/DocBook/deviceiobook.tmpl where we can document what
should be used in device drivers for portability, and what should be avoided.
then we can refer to asm-generic/io.h for just the API level stuff (i.e.
inputs/outputs/return values).
then we update asm-generic/io.h to:
- refer device driver authors to deviceiobook for the higher level
documentation
- explicitly state that the API here is the baseline and any new functions
should be discussed first on linux-arch/lkml
- explicitly state what API functions have been omitted, and what should be
used instead of those
how's that sound as a plan ?
> It would be nice if we could just use asm-generic/io.h as the documention,
> i.e. if you want to write a portable driver, just stick to the functions
> implemented there. Apart from the string accessors, I think it's already
> fairly accurate. The __raw_* functions seem to be missing on s390 but I
> don't think driver portability is a huge concern for that architecture.
as long as we document expectations at the top of the file, that's fine. this
implicit assumption doesn't fly in general for asm-generic/ because it's been a
constantly increasing code base as it merges more arches. there were a bunch
of files/apis i extended in asm-generic/ as i converted Blackfin over to it.
this is what we wanted from the beginning (afaik; i'm sure Arnd can correct
me).
-mike
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2012-05-01 18:27 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-04-27 10:42 [PATCH 1/2] asm-generic: io: remove {read,write} string functions Will Deacon
2012-04-27 10:42 ` [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} " Will Deacon
2012-04-27 12:12 ` Arnd Bergmann
2012-04-27 16:18 ` Mike Frysinger
2012-04-27 16:59 ` Will Deacon
2012-04-27 17:26 ` Mike Frysinger
2012-04-27 12:12 ` [PATCH 1/2] asm-generic: io: remove {read,write} " Arnd Bergmann
2012-04-27 16:17 ` Mike Frysinger
2012-04-27 16:53 ` Will Deacon
2012-04-27 17:18 ` Mike Frysinger
2012-04-28 8:46 ` Geert Uytterhoeven
2012-04-27 20:29 ` H. Peter Anvin
2012-04-27 16:23 ` H. Peter Anvin
2012-04-27 17:14 ` Will Deacon
2012-04-27 17:53 ` H. Peter Anvin
2012-04-27 20:52 ` Arnd Bergmann
2012-04-27 23:26 ` Mike Frysinger
2012-05-01 13:34 ` Will Deacon
2012-05-01 14:40 ` Arnd Bergmann
2012-05-01 18:28 ` Mike Frysinger
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.