[PATCH 1/2] asm-generic: io: remove {read,write} string functions

[PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Will Deacon]

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

[PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions

[Will Deacon]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Arnd Bergmann]

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

Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions

[Arnd Bergmann]

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>

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[H. Peter Anvin]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Will Deacon]

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

Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions

[Will Deacon]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Will Deacon]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH 2/2] asm-generic: io: don't perform swab during {in,out} string functions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[H. Peter Anvin]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[H. Peter Anvin]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Arnd Bergmann]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Mike Frysinger]

[-- 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 --]

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Geert Uytterhoeven]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Will Deacon]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Arnd Bergmann]

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

Re: [PATCH 1/2] asm-generic: io: remove {read,write} string functions

[Mike Frysinger]

[-- 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 --]