[PATCH] finally fix 53c700 to use the generic iomem infrastructure

[PATCH] finally fix 53c700 to use the generic iomem infrastructure

[James Bottomley]

This driver has had it's own different infrastructure for doing this for
ages, but it's time it used the common one.


James

===== drivers/scsi/53c700.c 1.64 vs edited =====
--- 1.64/drivers/scsi/53c700.c	2005-03-20 21:04:31 -06:00
+++ edited/drivers/scsi/53c700.c	2005-04-02 14:15:04 -06:00
@@ -389,8 +389,7 @@
 	host->max_lun = NCR_700_MAX_LUNS;
 	BUG_ON(NCR_700_transport_template == NULL);
 	host->transportt = NCR_700_transport_template;
-	host->unique_id = hostdata->base;
-	host->base = hostdata->base;
+	host->unique_id = (unsigned long)hostdata->base;
 	hostdata->eh_complete = NULL;
 	host->hostdata[0] = (unsigned long)hostdata;
 	/* kick the chip */
===== drivers/scsi/53c700.h 1.23 vs edited =====
--- 1.23/drivers/scsi/53c700.h	2005-03-20 21:04:31 -06:00
+++ edited/drivers/scsi/53c700.h	2005-04-02 14:23:33 -06:00
@@ -14,10 +14,6 @@
 #include <scsi/scsi_device.h>
 
 
-#if defined(CONFIG_53C700_MEM_MAPPED) && defined(CONFIG_53C700_IO_MAPPED)
-#define CONFIG_53C700_BOTH_MAPPED
-#endif
-
 /* Turn on for general debugging---too verbose for normal use */
 #undef	NCR_700_DEBUG
 /* Debug the tag queues, checking hash queue allocation and deallocation
@@ -49,13 +45,6 @@
 /* magic byte identifying an internally generated REQUEST_SENSE command */
 #define NCR_700_INTERNAL_SENSE_MAGIC	0x42
 
-/* WARNING: Leave this in for now: the dependency preprocessor doesn't
- * pick up file specific flags, so must define here if they are not
- * set */
-#if !defined(CONFIG_53C700_IO_MAPPED) && !defined(CONFIG_53C700_MEM_MAPPED)
-#error "Config.in must define either CONFIG_53C700_IO_MAPPED or CONFIG_53C700_MEM_MAPPED to use this scsi core."
-#endif
-
 struct NCR_700_Host_Parameters;
 
 /* These are the externally used routines */
@@ -184,7 +173,7 @@
 struct NCR_700_Host_Parameters {
 	/* These must be filled in by the calling driver */
 	int	clock;			/* board clock speed in MHz */
-	unsigned long	base;		/* the base for the port (copied to host) */
+	void __iomem	*base;		/* the base for the port (copied to host) */
 	struct device	*dev;
 	__u32	dmode_extra;	/* adjustable bus settings */
 	__u32	differential:1;	/* if we are differential */
@@ -199,9 +188,6 @@
 	/* NOTHING BELOW HERE NEEDS ALTERING */
 	__u32	fast:1;		/* if we can alter the SCSI bus clock
                                    speed (so can negiotiate sync) */
-#ifdef CONFIG_53C700_BOTH_MAPPED
-	__u32	mem_mapped;	/* set if memory mapped */
-#endif
 	int	sync_clock;	/* The speed of the SYNC core */
 
 	__u32	*script;		/* pointer to script location */
@@ -246,12 +232,18 @@
 #ifdef CONFIG_53C700_LE_ON_BE
 #define bE	(hostdata->force_le_on_be ? 0 : 3)
 #define	bSWAP	(hostdata->force_le_on_be)
+/* This is terrible, but there's no raw version of ioread32.  That means
+ * that on a be board we swap twice (once in ioread32 and once again to 
+ * get the value correct) */
+#define bS_to_io(x)	((hostdata->force_le_on_be) ? (x) : cpu_to_le32(x))
 #elif defined(__BIG_ENDIAN)
 #define bE	3
 #define bSWAP	0
+#define bS_to_io(x)	(x)
 #elif defined(__LITTLE_ENDIAN)
 #define bE	0
 #define bSWAP	0
+#define bS_to_io(x)	(x)
 #else
 #error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined, did you include byteorder.h?"
 #endif
@@ -455,91 +447,42 @@
 
 
 static inline __u8
-NCR_700_mem_readb(struct Scsi_Host *host, __u32 reg)
-{
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	return readb(host->base + (reg^bE));
-}
-
-static inline __u32
-NCR_700_mem_readl(struct Scsi_Host *host, __u32 reg)
-{
-	__u32 value = __raw_readl(host->base + reg);
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-#if 1
-	/* sanity check the register */
-	if((reg & 0x3) != 0)
-		BUG();
-#endif
-
-	return bS_to_cpu(value);
-}
-
-static inline void
-NCR_700_mem_writeb(__u8 value, struct Scsi_Host *host, __u32 reg)
-{
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	writeb(value, host->base + (reg^bE));
-}
-
-static inline void
-NCR_700_mem_writel(__u32 value, struct Scsi_Host *host, __u32 reg)
-{
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-#if 1
-	/* sanity check the register */
-	if((reg & 0x3) != 0)
-		BUG();
-#endif
-
-	__raw_writel(bS_to_host(value), host->base + reg);
-}
-
-static inline __u8
-NCR_700_io_readb(struct Scsi_Host *host, __u32 reg)
+NCR_700_readb(struct Scsi_Host *host, __u32 reg)
 {
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
+	const struct NCR_700_Host_Parameters *hostdata
 		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
 
-	return inb(host->base + (reg^bE));
+	return ioread8(hostdata->base + (reg^bE));
 }
 
 static inline __u32
-NCR_700_io_readl(struct Scsi_Host *host, __u32 reg)
+NCR_700_readl(struct Scsi_Host *host, __u32 reg)
 {
-	__u32 value = inl(host->base + reg);
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
+	const struct NCR_700_Host_Parameters *hostdata
 		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
+	__u32 value = ioread32(hostdata->base + reg);
 #if 1
 	/* sanity check the register */
 	if((reg & 0x3) != 0)
 		BUG();
 #endif
 
-	return bS_to_cpu(value);
+	return bS_to_io(value);
 }
 
 static inline void
-NCR_700_io_writeb(__u8 value, struct Scsi_Host *host, __u32 reg)
+NCR_700_writeb(__u8 value, struct Scsi_Host *host, __u32 reg)
 {
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
+	const struct NCR_700_Host_Parameters *hostdata
 		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
 
-	outb(value, host->base + (reg^bE));
+	iowrite8(value, hostdata->base + (reg^bE));
 }
 
 static inline void
-NCR_700_io_writel(__u32 value, struct Scsi_Host *host, __u32 reg)
+NCR_700_writel(__u32 value, struct Scsi_Host *host, __u32 reg)
 {
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
+	const struct NCR_700_Host_Parameters *hostdata
 		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
 
 #if 1
@@ -548,102 +491,7 @@
 		BUG();
 #endif
 
-	outl(bS_to_host(value), host->base + reg);
-}
-
-#ifdef CONFIG_53C700_BOTH_MAPPED
-
-static inline __u8
-NCR_700_readb(struct Scsi_Host *host, __u32 reg)
-{
-	__u8 val;
-
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	if(hostdata->mem_mapped)
-		val = NCR_700_mem_readb(host, reg);
-	else
-		val = NCR_700_io_readb(host, reg);
-
-	return val;
-}
-
-static inline __u32
-NCR_700_readl(struct Scsi_Host *host, __u32 reg)
-{
-	__u32 val;
-
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	if(hostdata->mem_mapped)
-		val = NCR_700_mem_readl(host, reg);
-	else
-		val = NCR_700_io_readl(host, reg);
-
-	return val;
-}
-
-static inline void
-NCR_700_writeb(__u8 value, struct Scsi_Host *host, __u32 reg)
-{
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	if(hostdata->mem_mapped)
-		NCR_700_mem_writeb(value, host, reg);
-	else
-		NCR_700_io_writeb(value, host, reg);
-}
-
-static inline void
-NCR_700_writel(__u32 value, struct Scsi_Host *host, __u32 reg)
-{
-	const struct NCR_700_Host_Parameters *hostdata __attribute__((unused))
-		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-
-	if(hostdata->mem_mapped)
-		NCR_700_mem_writel(value, host, reg);
-	else
-		NCR_700_io_writel(value, host, reg);
-}
-
-static inline void
-NCR_700_set_mem_mapped(struct NCR_700_Host_Parameters *hostdata)
-{
-	hostdata->mem_mapped = 1;
-}
-
-static inline void
-NCR_700_set_io_mapped(struct NCR_700_Host_Parameters *hostdata)
-{
-	hostdata->mem_mapped = 0;
+	iowrite32(bS_to_io(value), hostdata->base + reg);
 }
-
-
-#elif defined(CONFIG_53C700_IO_MAPPED)
-
-#define NCR_700_readb NCR_700_io_readb
-#define NCR_700_readl NCR_700_io_readl
-#define NCR_700_writeb NCR_700_io_writeb
-#define NCR_700_writel NCR_700_io_writel
-
-#define NCR_700_set_io_mapped(x)
-#define NCR_700_set_mem_mapped(x)	error I/O mapped only
-
-#elif defined(CONFIG_53C700_MEM_MAPPED)
-
-#define NCR_700_readb NCR_700_mem_readb
-#define NCR_700_readl NCR_700_mem_readl
-#define NCR_700_writeb NCR_700_mem_writeb
-#define NCR_700_writel NCR_700_mem_writel
-
-#define NCR_700_set_io_mapped(x)	error MEM mapped only
-#define NCR_700_set_mem_mapped(x)
-
-#else
-#error neither CONFIG_53C700_MEM_MAPPED nor CONFIG_53C700_IO_MAPPED is set
-#endif
 
 #endif
===== drivers/scsi/NCR_D700.c 1.24 vs edited =====
--- 1.24/drivers/scsi/NCR_D700.c	2005-03-20 21:04:31 -06:00
+++ edited/drivers/scsi/NCR_D700.c	2005-04-02 14:16:55 -06:00
@@ -197,12 +197,10 @@
 	}
 		
 	/* Fill in the three required pieces of hostdata */
-	hostdata->base = region;
+	hostdata->base = ioport_map(region, 64);
 	hostdata->differential = (((1<<siop) & differential) != 0);
 	hostdata->clock = NCR_D700_CLOCK_MHZ;
 
-	NCR_700_set_io_mapped(hostdata);
-
 	/* and register the siop */
 	host = NCR_700_detect(&NCR_D700_driver_template, hostdata, p->dev);
 	if (!host) {
@@ -214,6 +212,7 @@
 	/* FIXME: read this from SUS */
 	host->this_id = id_array[slot * 2 + siop];
 	host->irq = irq;
+	host->base = region;
 	scsi_scan_host(host);
 
 	return 0;
===== drivers/scsi/lasi700.c 1.25 vs edited =====
--- 1.25/drivers/scsi/lasi700.c	2005-03-20 21:04:31 -06:00
+++ edited/drivers/scsi/lasi700.c	2005-04-02 14:38:36 -06:00
@@ -131,6 +131,7 @@
 	if (!host)
 		goto out_kfree;
 	host->this_id = 7;
+	host->base = base;
 	host->irq = dev->irq;
 	if(request_irq(dev->irq, NCR_700_intr, SA_SHIRQ, "lasi700", host)) {
 		printk(KERN_ERR "lasi700: request_irq failed!\n");
===== drivers/scsi/sim710.c 1.27 vs edited =====
--- 1.27/drivers/scsi/sim710.c	2005-03-20 21:04:31 -06:00
+++ edited/drivers/scsi/sim710.c	2005-04-02 14:21:06 -06:00
@@ -120,11 +120,10 @@
 	}
 
 	/* Fill in the three required pieces of hostdata */
-	hostdata->base = base_addr;
+	hostdata->base = ioport_map(base_addr, 64);
 	hostdata->differential = differential;
 	hostdata->clock = clock;
 	hostdata->chip710 = 1;
-	NCR_700_set_io_mapped(hostdata);
 
 	/* and register the chip */
 	if((host = NCR_700_detect(&sim710_driver_template, hostdata, dev))
@@ -133,6 +132,7 @@
 		goto out_release;
 	}
 	host->this_id = scsi_id;
+	host->base = base_addr;
 	host->irq = irq;
 	if (request_irq(irq, NCR_700_intr, SA_SHIRQ, "sim710", host)) {
 		printk(KERN_ERR "sim710: request_irq failed\n");
@@ -164,6 +164,7 @@
 	NCR_700_release(host);
 	kfree(hostdata);
 	free_irq(host->irq, host);
+	release_region(host->base, 64);
 	return 0;
 }
 

iomapping a big endian area

[Matthew Wilcox]

On Sat, Apr 02, 2005 at 02:52:14PM -0600, James Bottomley wrote:
> This driver has had it's own different infrastructure for doing this for
> ages, but it's time it used the common one.

Thanks.  I'd been looking at this for a while but hadn't got round tuit yet.

>  #ifdef CONFIG_53C700_LE_ON_BE
>  #define bE	(hostdata->force_le_on_be ? 0 : 3)
>  #define	bSWAP	(hostdata->force_le_on_be)
> +/* This is terrible, but there's no raw version of ioread32.  That means
> + * that on a be board we swap twice (once in ioread32 and once again to 
> + * get the value correct) */
> +#define bS_to_io(x)	((hostdata->force_le_on_be) ? (x) : cpu_to_le32(x))

I raised this with Linus back when he did the original iomap() stuff.
Unfortunately, I think he ignored the question ;-)

My thought on this is that we should encode the endianness of the
registers in the ioremap cookie.  Some architectures (sparc, I think?) can
do this in their PTEs.  The rest of us can do it in our ioread/writeN
methods.  I've planned for this in the parisc iomap implementation but
not actually implemented it.

It doesn't look too hard so I'll commit something to the parisc tree
later that'll let you iomap a BE area.  Do we have any cards that need to be
accessed in a BE way on a LE machine?  (ie x86)

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: iomapping a big endian area

[David S. Miller]

On Sun, 3 Apr 2005 02:37:57 +0100
Matthew Wilcox <matthew@wil.cx> wrote:

> My thought on this is that we should encode the endianness of the
> registers in the ioremap cookie.  Some architectures (sparc, I think?) can
> do this in their PTEs.  The rest of us can do it in our ioread/writeN
> methods.  I've planned for this in the parisc iomap implementation but
> not actually implemented it.

SPARC64 can do it in the PTEs, but we just use raw physical
addresses in our I/O accessors, and in those load/store instructions
we can specify the endianness.

Be careful though.  Endianness can be dealt with on a hardware
level.  Consider a byte access to a 32-bit word sized config space
datum, the PCI controller on a big-endian system will likely byte-twist
the data lanes in order for this to work properly.

It's a subtle issue, and it's explained pretty well in some of the
UltraSPARC PCI controller docs at:

	http://www.sun.com/processors/documentation.html

In particular, "U2P UPA to PCI User's Manual", chapter 10
"Little-Endian Support", has some informative diagrams.

Re: iomapping a big endian area

[Matthew Wilcox]

On Sat, Apr 02, 2005 at 06:38:05PM -0800, David S. Miller wrote:
> > My thought on this is that we should encode the endianness of the
> > registers in the ioremap cookie.  Some architectures (sparc, I think?) can
> > do this in their PTEs.  The rest of us can do it in our ioread/writeN
> > methods.  I've planned for this in the parisc iomap implementation but
> > not actually implemented it.
> 
> SPARC64 can do it in the PTEs, but we just use raw physical
> addresses in our I/O accessors, and in those load/store instructions
> we can specify the endianness.

Ah right.  So you'd prefer an ioread8be() interface?

> Be careful though.  Endianness can be dealt with on a hardware
> level.  Consider a byte access to a 32-bit word sized config space
> datum, the PCI controller on a big-endian system will likely byte-twist
> the data lanes in order for this to work properly.

Yup, PA-RISC PCI adapters (both Dino and Elroy) do the same thing.
The 53c700 driver handles this lack of skewing by xoring the address with 3.

-- 
"Next the statesmen will invent cheap lies, putting the blame upon 
the nation that is attacked, and every man will be glad of those
conscience-soothing falsities, and will diligently study them, and refuse
to examine any refutations of them; and thus he will by and by convince 
himself that the war is just, and will thank God for the better sleep 
he enjoys after this process of grotesque self-deception." -- Mark Twain

Re: iomapping a big endian area

[James Bottomley]

On Sun, 2005-04-03 at 04:10 +0100, Matthew Wilcox wrote:
> > SPARC64 can do it in the PTEs, but we just use raw physical
> > addresses in our I/O accessors, and in those load/store instructions
> > we can specify the endianness.
> 
> Ah right.  So you'd prefer an ioread8be() interface?

Actually, ioread8be is unnecessary, but I was planning to add
ioread16/ioread32 and iowritexx be on be variants (equivalent to
_raw_readw et al.)

After all, the driver must know the card is BE, so the routines that
make use of the feature are easily coded into the card, so there's no
real need to add it to the iomem cookie.

Did anyone have a preference for the API?  I was thinking
ioread32_native, but ioread32be is fine too.

James

Re: iomapping a big endian area

[David S. Miller]

On Sat, 02 Apr 2005 21:40:39 -0600
James Bottomley <James.Bottomley@SteelEye.com> wrote:

> After all, the driver must know the card is BE, so the routines that
> make use of the feature are easily coded into the card, so there's no
> real need to add it to the iomem cookie.

Yes, I don't believe it needs to be in the cookie either.

> Did anyone have a preference for the API?  I was thinking
> ioread32_native, but ioread32be is fine too.

I think doing foo{be,le}{8,16,32}() would be consistent with
our byteorder.h interface names.

Re: iomapping a big endian area

[James Bottomley]

On Sat, 2005-04-02 at 20:08 -0800, David S. Miller wrote:
> > Did anyone have a preference for the API?  I was thinking
> > ioread32_native, but ioread32be is fine too.
> 
> I think doing foo{be,le}{8,16,32}() would be consistent with
> our byteorder.h interface names.

Thinking about this some more, I know of no case of a BE bus connected
to a LE system, nor do I think anyone would ever create such a beast, so
our only missing interface is for a BE bus on a BE system.

Thus, I think io{read,write}{16,32}_native are better interfaces ...
they basically mean pass memory operations without byte swaps, so
they're well defined on both BE and LE systems and correspond exactly to
our existing _raw_{read,write}{w,l} calls (principle of least surprise).

James

Re: iomapping a big endian area

[Benjamin Herrenschmidt]

On Sat, 2005-04-02 at 21:40 -0600, James Bottomley wrote:

> Actually, ioread8be is unnecessary, but I was planning to add
> ioread16/ioread32 and iowritexx be on be variants (equivalent to
> _raw_readw et al.)
> 
> After all, the driver must know the card is BE, so the routines that
> make use of the feature are easily coded into the card, so there's no
> real need to add it to the iomem cookie.
> 
> Did anyone have a preference for the API?  I was thinking
> ioread32_native, but ioread32be is fine too.

I think we want "be" since obviously, the "ioread32" one is "le", that
makes sense to provide both and let the implementation bother with what
has to swap or not. With "native", x86 would do what ? swap or not
swap ? unclear ... with "be", at least it's clear.

The problem ? Hehe, well ... there is at least one little problem: The
way iomap "fixes" the issue of mem vs. io space in a driver could have
been used to also fix le vs. be issues. For example, an USB OHCI
controller is normally little endian. But some too-stupid-for-words HW
folks tried to be too smart on a number of embedded chips, and put a big
endian version of it. Thus the driver ends up having to support both.
Most embedded vendors just butcher the driver with #ifdef's which is
fine by me ... until you end up having _also_ a PCI bus with an
EHCI/OHCI pair on it on the same board... then you are toast.

But, I wouldn't bother too much about this case. The driver has other
issues than just IO to deal with (the DMA data structures in memory are
also endian- swapped) so I suppose the entire driver need to be somewhat
#included from a wrapper an compiled twice for different endians to get
that right...

Ben.

Re: iomapping a big endian area

[Benjamin Herrenschmidt]

On Sat, 2005-04-02 at 22:27 -0600, James Bottomley wrote:
> On Sat, 2005-04-02 at 20:08 -0800, David S. Miller wrote:
> > > Did anyone have a preference for the API?  I was thinking
> > > ioread32_native, but ioread32be is fine too.
> > 
> > I think doing foo{be,le}{8,16,32}() would be consistent with
> > our byteorder.h interface names.
> 
> Thinking about this some more, I know of no case of a BE bus connected
> to a LE system, nor do I think anyone would ever create such a beast, so
> our only missing interface is for a BE bus on a BE system.

It's more a matter of the device than the bus imho... 

> Thus, I think io{read,write}{16,32}_native are better interfaces ...

I disagree. The driver will never "know" ...

> they basically mean pass memory operations without byte swaps, so
> they're well defined on both BE and LE systems and correspond exactly to
> our existing _raw_{read,write}{w,l} calls (principle of least surprise).

I don't think it's sane. You know that your device is BE or LE and use
the appropriate interface. "native" doesn't make sense to me in this
context.

Ben.

Re: iomapping a big endian area

[James Bottomley]

On Mon, 2005-04-04 at 17:50 +1000, Benjamin Herrenschmidt wrote:
> I disagree. The driver will never "know" ...

? the driver has to know.  Look at the 53c700 to see exactly how awful
it is.  This beast has byte and word registers.  When used BE, all the
byte registers alter their position (to both inb and readb).

> I don't think it's sane. You know that your device is BE or LE and use
> the appropriate interface. "native" doesn't make sense to me in this
> context.

Well ... it's like this. Native means "pass through without swapping"
and has an easy implementation on both BE and LE platforms.  Logically
io{read,write}{16,32}be would have to do byte swaps on LE platforms.
Being lazy, I'm opposed to doing the work if there's no actual use for
it, so can you provide an example of a BE bus (or device) used on a LE
platform that would actually benefit from this abstraction?

James

Re: iomapping a big endian area

[Christoph Hellwig]

On Mon, Apr 04, 2005 at 08:59:03AM -0500, James Bottomley wrote:
> Well ... it's like this. Native means "pass through without swapping"
> and has an easy implementation on both BE and LE platforms.  Logically
> io{read,write}{16,32}be would have to do byte swaps on LE platforms.
> Being lazy, I'm opposed to doing the work if there's no actual use for
> it, so can you provide an example of a BE bus (or device) used on a LE
> platform that would actually benefit from this abstraction?

The IOC4 device that provides IDE, serial ports and external interrupts
on Altix systems has a big endian register layour, and the PCI-X bridge
in those Altix systems can do the swapping if a special bit is set.

In older kernels that bit was set from the driver through a special API,
but it seems the firmware does that automatically now.

Re: iomapping a big endian area

[Randy.Dunlap]

James Bottomley wrote:
> On Mon, 2005-04-04 at 17:50 +1000, Benjamin Herrenschmidt wrote:
> 
>>I disagree. The driver will never "know" ...
> 
> 
> ? the driver has to know.  Look at the 53c700 to see exactly how awful
> it is.  This beast has byte and word registers.  When used BE, all the
> byte registers alter their position (to both inb and readb).
> 
> 
>>I don't think it's sane. You know that your device is BE or LE and use
>>the appropriate interface. "native" doesn't make sense to me in this
>>context.
> 
> 
> Well ... it's like this. Native means "pass through without swapping"
> and has an easy implementation on both BE and LE platforms.  Logically
> io{read,write}{16,32}be would have to do byte swaps on LE platforms.
> Being lazy, I'm opposed to doing the work if there's no actual use for
> it, so can you provide an example of a BE bus (or device) used on a LE
> platform that would actually benefit from this abstraction?

I would probably spell "native" as "noswap".
"native" just doesn't convey enough specific meaning...

-- 
~Randy

Re: iomapping a big endian area

[James Bottomley]

On Mon, 2005-04-04 at 15:16 +0100, Christoph Hellwig wrote:
> The IOC4 device that provides IDE, serial ports and external interrupts
> on Altix systems has a big endian register layour, and the PCI-X bridge
> in those Altix systems can do the swapping if a special bit is set.
> 
> In older kernels that bit was set from the driver through a special API,
> but it seems the firmware does that automatically now.

We already have some unusual code in drivers to support other altix
design ... features ... do you regard this as something that's likely to
be replicated on other platforms, or is it more in the category of a one
off mistake that can be corrected in firmware?

James

Re: iomapping a big endian area

[James Bottomley]

OK, I sent the patch off to Andrew.  To complete the original problem,
the attached is the patch that uses it in the parisc lasi driver
(although, actually, it sets up 53c700 to work everywhere including BE
on a LE system).

I changed some of the flags around to reflect the fact that we now have
generic BE support in the driver (rather than the more limited
force_le_on_be flag).

James

===== drivers/scsi/53c700.h 1.25 vs edited =====
--- 1.25/drivers/scsi/53c700.h	2005-04-04 09:55:44 -05:00
+++ edited/drivers/scsi/53c700.h	2005-04-04 15:39:01 -05:00
@@ -177,10 +177,10 @@
 	struct device	*dev;
 	__u32	dmode_extra;	/* adjustable bus settings */
 	__u32	differential:1;	/* if we are differential */
-#ifdef CONFIG_53C700_LE_ON_BE
+#ifdef CONFIG_53C700_BE
 	/* This option is for HP only.  Set it if your chip is wired for
 	 * little endian on this platform (which is big endian) */
-	__u32	force_le_on_be:1;
+	__u32	chip_is_be:1;
 #endif
 	__u32	chip710:1;	/* set if really a 710 not 700 */
 	__u32	burst_disable:1;	/* set to 1 to disable 710 bursting */
@@ -229,24 +229,29 @@
 /*
  *	53C700 Register Interface - the offset from the Selected base
  *	I/O address */
-#ifdef CONFIG_53C700_LE_ON_BE
-#define bE	(hostdata->force_le_on_be ? 0 : 3)
-#define	bSWAP	(hostdata->force_le_on_be)
-/* This is terrible, but there's no raw version of ioread32.  That means
- * that on a be board we swap twice (once in ioread32 and once again to 
- * get the value correct) */
-#define bS_to_io(x)	((hostdata->force_le_on_be) ? (x) : cpu_to_le32(x))
-#elif defined(__BIG_ENDIAN)
+#ifdef CONFIG_53C700_BE
+#define bE	(hostdata->chip_is_be ? 3: 0)
+#ifdef __BIG_ENDIAN
+#define	bSWAP	(!hostdata->chip_is_be)
+#else
+#define bSWAP	(hostdata->chip_is_be);
+#endif
+#define NCR_ioread32(x)	((hostdata->chip_is_be) ? ioread32be(x) : ioread32(x))
+#define NCR_iowrite32(v, x) \
+	((hostdata->chip_is_be) ? iowrite32be((v), (x)) : iowrite32((v), (x)))
+#else
+#define NCR_ioread32(x)		ioread32(x)
+#define NCR_iowrite32(v, x)	iowrite32((v), (x))
+#if defined(__BIG_ENDIAN)
 #define bE	3
 #define bSWAP	0
-#define bS_to_io(x)	(x)
 #elif defined(__LITTLE_ENDIAN)
 #define bE	0
 #define bSWAP	0
-#define bS_to_io(x)	(x)
 #else
 #error "__BIG_ENDIAN or __LITTLE_ENDIAN must be defined, did you include byteorder.h?"
 #endif
+#endif
 #define bS_to_cpu(x)	(bSWAP ? le32_to_cpu(x) : (x))
 #define bS_to_host(x)	(bSWAP ? cpu_to_le32(x) : (x))
 
@@ -460,14 +465,13 @@
 {
 	const struct NCR_700_Host_Parameters *hostdata
 		= (struct NCR_700_Host_Parameters *)host->hostdata[0];
-	__u32 value = ioread32(hostdata->base + reg);
+	__u32 value = NCR_ioread32(hostdata->base + reg);
 #if 1
 	/* sanity check the register */
-	if((reg & 0x3) != 0)
-		BUG();
+	BUG_ON((reg & 0x3) != 0);
 #endif
 
-	return bS_to_io(value);
+	return value;
 }
 
 static inline void
@@ -487,11 +491,10 @@
 
 #if 1
 	/* sanity check the register */
-	if((reg & 0x3) != 0)
-		BUG();
+	BUG_ON((reg & 0x3) != 0);
 #endif
 
-	iowrite32(bS_to_io(value), hostdata->base + reg);
+	NCR_iowrite32(value, hostdata->base + reg);
 }
 
 #endif
===== drivers/scsi/Kconfig 1.88 vs edited =====
--- 1.88/drivers/scsi/Kconfig	2005-04-04 09:55:45 -05:00
+++ edited/drivers/scsi/Kconfig	2005-04-04 15:34:40 -05:00
@@ -951,7 +951,7 @@
 	  many PA-RISC workstations & servers.  If you do not know whether you
 	  have a Lasi chip, it is safe to say "Y" here.
 
-config 53C700_LE_ON_BE
+config 53C700_BE
 	bool
 	depends on SCSI_LASI700
 	default y
===== drivers/scsi/lasi700.c 1.27 vs edited =====
--- 1.27/drivers/scsi/lasi700.c	2005-04-04 09:55:45 -05:00
+++ edited/drivers/scsi/lasi700.c	2005-04-04 15:31:19 -05:00
@@ -117,15 +117,13 @@
 
 	if (dev->id.sversion == LASI_700_SVERSION) {
 		hostdata->clock = LASI700_CLOCK;
-		hostdata->force_le_on_be = 1;
+		hostdata->chip_is_be = 0;
 	} else {
 		hostdata->clock = LASI710_CLOCK;
-		hostdata->force_le_on_be = 0;
+		hostdata->chip_is_be = 1;
 		hostdata->chip710 = 1;
 		hostdata->dmode_extra = DMODE_FC2;
 	}
-
-	NCR_700_set_mem_mapped(hostdata);
 
 	host = NCR_700_detect(&lasi700_template, hostdata, &dev->dev);
 	if (!host)

Re: iomapping a big endian area

[Benjamin Herrenschmidt]

> > Well ... it's like this. Native means "pass through without swapping"
> > and has an easy implementation on both BE and LE platforms.  Logically
> > io{read,write}{16,32}be would have to do byte swaps on LE platforms.
> > Being lazy, I'm opposed to doing the work if there's no actual use for
> > it, so can you provide an example of a BE bus (or device) used on a LE
> > platform that would actually benefit from this abstraction?
> 
> I would probably spell "native" as "noswap".
> "native" just doesn't convey enough specific meaning...

But that implies that the driver has to know that the bus and the device
and the CPU are on the same byte endian etc.... that is rather specific,
and if they all know, then they can also just use the correct "be" or
"le" ... I really see no point in "native" abstraction.

Ben.

Re: iomapping a big endian area

[Benjamin Herrenschmidt]

On Mon, 2005-04-04 at 08:59 -0500, James Bottomley wrote:
> On Mon, 2005-04-04 at 17:50 +1000, Benjamin Herrenschmidt wrote:
> > I disagree. The driver will never "know" ...
> 
> ? the driver has to know.  Look at the 53c700 to see exactly how awful
> it is.  This beast has byte and word registers.  When used BE, all the
> byte registers alter their position (to both inb and readb).

What I mean is that the driver doesn't have "know" whatever CPU/bus
combo endianness will require it to use "native" or "swapped" access.
What the driver knows is wether the device it tries to access need BE or
LE accessors.

> > I don't think it's sane. You know that your device is BE or LE and use
> > the appropriate interface. "native" doesn't make sense to me in this
> > context.
> 
> Well ... it's like this. Native means "pass through without swapping"
> and has an easy implementation on both BE and LE platforms.  Logically
> io{read,write}{16,32}be would have to do byte swaps on LE platforms.
> Being lazy, I'm opposed to doing the work if there's no actual use for
> it, so can you provide an example of a BE bus (or device) used on a LE
> platform that would actually benefit from this abstraction?

I don't think drivers will benefit from "native" vs. "swapping". Taht
means that pretty much all drivers that care will end up with #ifdef
crap to pick up the right one based on the CPU endian, and some wil get
it wrong of course. No, I _do_ thing that this should be hidden in the
implementation, and we should provide "le" and "be" accessors (le beeing
the current ones, be new ones). Which one swap or not is in the
implementation of those accessors for the architecture, but the driver
shouldn't have to care.

Ben.

Re: iomapping a big endian area

[Christoph Hellwig]

On Mon, Apr 04, 2005 at 04:17:45PM -0500, James Bottomley wrote:
> OK, I sent the patch off to Andrew.  To complete the original problem,
> the attached is the patch that uses it in the parisc lasi driver
> (although, actually, it sets up 53c700 to work everywhere including BE
> on a LE system).
> 
> I changed some of the flags around to reflect the fact that we now have
> generic BE support in the driver (rather than the more limited
> force_le_on_be flag).

What I really don't like is that you still need ifdefs and wrappers to
support BE and LE wired chips in the same driver.  Shouldn't you set the
BE or LE flag at iomap() time and always use the same accessors?

Re: iomapping a big endian area

[Russell King]

On Sat, Apr 02, 2005 at 09:40:39PM -0600, James Bottomley wrote:
> On Sun, 2005-04-03 at 04:10 +0100, Matthew Wilcox wrote:
> > > SPARC64 can do it in the PTEs, but we just use raw physical
> > > addresses in our I/O accessors, and in those load/store instructions
> > > we can specify the endianness.
> > 
> > Ah right.  So you'd prefer an ioread8be() interface?
> 
> Actually, ioread8be is unnecessary, but I was planning to add
> ioread16/ioread32 and iowritexx be on be variants (equivalent to
> _raw_readw et al.)

Not so.  There are two different styles of big endian.  (Lets just face
it, BE is fucked in the head anyway...)

physical bus:	31...24	23...16	15...8	7...0

BE version 1 (word invariant)
  byte access	byte 0	byte 1	byte 2	byte 3
  word access	31-24	23-16	15-8	7-0

BE version 2 (byte invariant)
  byte access	byte 3	byte 2	byte 1	byte 0
  word access	7-0	15-8	23-16	31-24

Depending on this and how your devices are wired up to such a bus, you
may need to swap bytes in a word access, or munge the byte/half word
address itself.

And guess which architecture implements *both* of these...  Grumble.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: iomapping a big endian area

[James Bottomley]

On Tue, 2005-04-05 at 08:42 +0100, Russell King wrote:
> Not so.  There are two different styles of big endian.  (Lets just face
> it, BE is fucked in the head anyway...)
> 
> physical bus:	31...24	23...16	15...8	7...0
> 
> BE version 1 (word invariant)
>   byte access	byte 0	byte 1	byte 2	byte 3
>   word access	31-24	23-16	15-8	7-0
> 
> BE version 2 (byte invariant)
>   byte access	byte 3	byte 2	byte 1	byte 0
>   word access	7-0	15-8	23-16	31-24

These are just representations of the same thing.  However, I did
deliberately elect not to try to solve this problem in the accessors.  I
know all about the register relayout, because 53c700 has to do that on
parisc.

However, I don't think it's the job of the io accessors to remap the
register locations (primarily because the remapping depends on the
documentation:  a chip that's documented as BE on BE has no remapping
required.  Hoever, the same chip on LE would need this).


> And guess which architecture implements *both* of these...  Grumble.

We have this in parisc too ...

James

Re: iomapping a big endian area

[James Bottomley]

On Tue, 2005-04-05 at 08:21 +0100, Christoph Hellwig wrote:
> What I really don't like is that you still need ifdefs and wrappers to
> support BE and LE wired chips in the same driver.  Shouldn't you set the
> BE or LE flag at iomap() time and always use the same accessors?

No, if you look how it works.  On parisc, it can be either BE or LE
depending on the chip wiring.  There would be a case for selecting BE or
LE by #define, but there's no case I know today where we have a driver
that would always be BE.

James

Re: iomapping a big endian area

[Russell King]

On Tue, Apr 05, 2005 at 09:05:15AM -0500, James Bottomley wrote:
> On Tue, 2005-04-05 at 08:42 +0100, Russell King wrote:
> > Not so.  There are two different styles of big endian.  (Lets just face
> > it, BE is fucked in the head anyway...)
> > 
> > physical bus:	31...24	23...16	15...8	7...0
> > 
> > BE version 1 (word invariant)
> >   byte access	byte 0	byte 1	byte 2	byte 3
> >   word access	31-24	23-16	15-8	7-0
> > 
> > BE version 2 (byte invariant)
> >   byte access	byte 3	byte 2	byte 1	byte 0
> >   word access	7-0	15-8	23-16	31-24
> 
> These are just representations of the same thing.  However, I did
> deliberately elect not to try to solve this problem in the accessors.  I
> know all about the register relayout, because 53c700 has to do that on
> parisc.

They aren't.  On some of our platforms, we have to exclusive-or the address
for byte accesses with 3 to convert to the right endian-ness.

Sure, from the point of view of which byte each byte of a word represents,
it's true that they're indentical.  But as far as the hardware is concerned,
they're definitely different.

See the Intel IXP platforms for an example.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: iomapping a big endian area

[Maciej W. Rozycki]

On Tue, 5 Apr 2005, Russell King wrote:

> > > physical bus:	31...24	23...16	15...8	7...0
> > > 
> > > BE version 1 (word invariant)
> > >   byte access	byte 0	byte 1	byte 2	byte 3
> > >   word access	31-24	23-16	15-8	7-0
> > > 
> > > BE version 2 (byte invariant)
> > >   byte access	byte 3	byte 2	byte 1	byte 0
> > >   word access	7-0	15-8	23-16	31-24
> > 
> > These are just representations of the same thing.  However, I did
> > deliberately elect not to try to solve this problem in the accessors.  I
> > know all about the register relayout, because 53c700 has to do that on
> > parisc.
> 
> They aren't.  On some of our platforms, we have to exclusive-or the address
> for byte accesses with 3 to convert to the right endian-ness.

 The same with certain MIPS configurations.  And likewise you need to xor 
addresses with 2 for halfword accesses.

> Sure, from the point of view of which byte each byte of a word represents,
> it's true that they're indentical.  But as far as the hardware is concerned,
> they're definitely different.

 To clarify it a bit: both big and little endian representations are 
always the same -- it's going to a domain of the reverse endianness that 
can be done in two different ways, i.e. by preserving either bit or byte 
ordering (as described above).  Depending on the interpretation of data 
being passed you want one or the other.  That's why some systems provide 
ways of doing both kinds of accesses in hardware (e.g. the host bus to PCI 
bridge) to save CPU cycles needed for bit shuffling otherwise.

  Maciej

Re: iomapping a big endian area

[Jesse Barnes]

On Monday, April 4, 2005 7:25 am, James Bottomley wrote:
> On Mon, 2005-04-04 at 15:16 +0100, Christoph Hellwig wrote:
> > The IOC4 device that provides IDE, serial ports and external interrupts
> > on Altix systems has a big endian register layour, and the PCI-X bridge
> > in those Altix systems can do the swapping if a special bit is set.
> >
> > In older kernels that bit was set from the driver through a special API,
> > but it seems the firmware does that automatically now.
>
> We already have some unusual code in drivers to support other altix
> design ... features ... do you regard this as something that's likely to
> be replicated on other platforms, or is it more in the category of a one
> off mistake that can be corrected in firmware?

The whole line of altix pci bridges can do byteswapping, so it's more than 
just one product that could benefit (however slightly).  Not sure about other 
bridges though.

Jesse