Re: drivers/usb/musb/musb_io.h

Re: drivers/usb/musb/musb_io.h

[Andrew Morton]

(cc linux-arch)

On Fri, 15 Aug 2008 11:11:55 +0300 Felipe Balbi <me@felipebalbi.com> wrote:

> On Fri, Aug 15, 2008 at 01:02:27AM -0700, Andrew Morton wrote:
> > > SH also defines read/write friends.
> > 
> > That's an unilluminating changelog.
> > What's actually going on in here?
> 
> Those functions are used for fifo reading/writing operations. Some
> architectures provide them and some don't. For those who don't, we add
> the definition to the driver.
> 
> > Why is a driver implementing what appear to be core architecture helper
> > functions?
> 
> Just because the core architecture doesn't provide them.
> 
> > What is a proper fix?
> 
> The proper fix is that we always have, at least, stubs for read/write
> friends even if the arch doesn't provide them.

That is absolutely not the proper fix.  Far from it.  This way we end
up with multiple (and of course, different) implementations of these
things in an arbitrary number of drivers.

A proper fix would be to a) define the semantics of these operations,
b) document them then c) implement them on each architecture.

One possible implementation might be


--- a/include/linux/io.h~a
+++ a/include/linux/io.h
@@ -67,4 +67,41 @@ int check_signature(const volatile void 
 			const unsigned char *signature, int length);
 void devm_ioremap_release(struct device *dev, void *res);
 
+#ifndef readsl
+
+/*
+ * description goes here
+ */
+static inline void readsl(const void __iomem *addr, void *buf, int len)
+{
+	insl((unsigned long)addr, buf, len);
+}
+
+static inline void readsw(const void __iomem *addr, void *buf, int len)
+{
+	insw((unsigned long)addr, buf, len);
+}
+
+static inline void readsb(const void __iomem *addr, void *buf, int len)
+{
+	insb((unsigned long)addr, buf, len);
+}
+
+static inline void writesl(const void __iomem *addr, const void *buf, int len)
+{
+	outsl((unsigned long)addr, buf, len);
+}
+
+static inline void writesw(const void __iomem *addr, const void *buf, int len)
+{
+	outsw((unsigned long)addr, buf, len);
+}
+
+static inline void writesb(const void __iomem *addr, const void *buf, int len)
+{
+	outsb((unsigned long)addr, buf, len);
+}
+
+#endif		/* readsl */
+
 #endif /* _LINUX_IO_H */


But just saying "oh look, the arch layer is all screwed up, let's hack
around it in our driver" is plain irresponsible.

> > avr32 and powerpc might also have conflicts.  Now, or in the future.

Did you review these architectures?

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

On Fri, Aug 15, 2008 at 01:31:48AM -0700, Andrew Morton wrote:
> One possible implementation might be
> 
> 
> --- a/include/linux/io.h~a
> +++ a/include/linux/io.h
> @@ -67,4 +67,41 @@ int check_signature(const volatile void 
>  			const unsigned char *signature, int length);
>  void devm_ioremap_release(struct device *dev, void *res);
>  
> +#ifndef readsl
> +
> +/*
> + * description goes here
> + */
> +static inline void readsl(const void __iomem *addr, void *buf, int len)
> +{
> +	insl((unsigned long)addr, buf, len);
> +}
> +
> +static inline void readsw(const void __iomem *addr, void *buf, int len)
> +{
> +	insw((unsigned long)addr, buf, len);
> +}
> +
> +static inline void readsb(const void __iomem *addr, void *buf, int len)
> +{
> +	insb((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesl(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsl((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesw(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsw((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesb(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsb((unsigned long)addr, buf, len);
> +}
> +
> +#endif		/* readsl */
> +
>  #endif /* _LINUX_IO_H */

That's what I meant (or tried to). It should come from the kernel itself.
Unfortunately we didn't have that at that time and we "hacked" it in the
driver.

> But just saying "oh look, the arch layer is all screwed up, let's hack
> around it in our driver" is plain irresponsible.

I'd be glad to change if include/linux/io.h can provide those stubs.

> > > avr32 and powerpc might also have conflicts.  Now, or in the future.
> 
> Did you review these architectures?

Yeah. ppc and avr32 provide them.
Wonder if it's possible to have the above patch applied so I can get rid
of those stubs in musb_io.h

-- 
balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

On Fri, Aug 15, 2008 at 01:31:48AM -0700, Andrew Morton wrote:
> One possible implementation might be
> 
> 
> --- a/include/linux/io.h~a
> +++ a/include/linux/io.h
> @@ -67,4 +67,41 @@ int check_signature(const volatile void 
>  			const unsigned char *signature, int length);
>  void devm_ioremap_release(struct device *dev, void *res);
>  
> +#ifndef readsl
> +
> +/*
> + * description goes here
> + */
> +static inline void readsl(const void __iomem *addr, void *buf, int len)
> +{
> +	insl((unsigned long)addr, buf, len);
> +}
> +
> +static inline void readsw(const void __iomem *addr, void *buf, int len)
> +{
> +	insw((unsigned long)addr, buf, len);
> +}
> +
> +static inline void readsb(const void __iomem *addr, void *buf, int len)
> +{
> +	insb((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesl(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsl((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesw(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsw((unsigned long)addr, buf, len);
> +}
> +
> +static inline void writesb(const void __iomem *addr, const void *buf, int len)
> +{
> +	outsb((unsigned long)addr, buf, len);
> +}
> +
> +#endif		/* readsl */
> +
>  #endif /* _LINUX_IO_H */

That's what I meant (or tried to). It should come from the kernel itself.
Unfortunately we didn't have that at that time and we "hacked" it in the
driver.

> But just saying "oh look, the arch layer is all screwed up, let's hack
> around it in our driver" is plain irresponsible.

I'd be glad to change if include/linux/io.h can provide those stubs.

> > > avr32 and powerpc might also have conflicts.  Now, or in the future.
> 
> Did you review these architectures?

Yeah. ppc and avr32 provide them.
Wonder if it's possible to have the above patch applied so I can get rid
of those stubs in musb_io.h

-- 
balbi

Re: drivers/usb/musb/musb_io.h

[Andrew Morton]

On Fri, 15 Aug 2008 11:52:48 +0300 Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org> wrote:

> Wonder if it's possible to have the above patch applied so I can get rid
> of those stubs in musb_io.h

Someone would need to take care of it and document it and repair all
those driver which broke because they went and created private copies.

I could do some of that but I'm stuck at step #1.  What the heck _are_
these things?  Why do they exist?  What are their semantics?

The arm implementation says

 * Generic IO read/write.  These perform native-endian accesses.  Note
 * that some architectures will want to re-define __raw_{read,write}w.

which is somewhat clear as mud.  I'd guess that these are the MMIO
partners to insl and friends?

And then it has

extern void __raw_writesb(void __iomem *addr, const void *data, int bytelen);
extern void __raw_writesw(void __iomem *addr, const void *data, int wordlen);
extern void __raw_writesl(void __iomem *addr, const void *data, int longlen);

which is a bit odd.  I assume that "bytelen" should have been
"bytecount" or similar.


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drivers/usb/musb/musb_io.h

[Andrew Morton]

On Fri, 15 Aug 2008 11:52:48 +0300 Felipe Balbi <me@felipebalbi.com> wrote:

> Wonder if it's possible to have the above patch applied so I can get rid
> of those stubs in musb_io.h

Someone would need to take care of it and document it and repair all
those driver which broke because they went and created private copies.

I could do some of that but I'm stuck at step #1.  What the heck _are_
these things?  Why do they exist?  What are their semantics?

The arm implementation says

 * Generic IO read/write.  These perform native-endian accesses.  Note
 * that some architectures will want to re-define __raw_{read,write}w.

which is somewhat clear as mud.  I'd guess that these are the MMIO
partners to insl and friends?

And then it has

extern void __raw_writesb(void __iomem *addr, const void *data, int bytelen);
extern void __raw_writesw(void __iomem *addr, const void *data, int wordlen);
extern void __raw_writesl(void __iomem *addr, const void *data, int longlen);

which is a bit odd.  I assume that "bytelen" should have been
"bytecount" or similar.

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

On Fri, Aug 15, 2008 at 02:11:31AM -0700, Andrew Morton wrote:
> On Fri, 15 Aug 2008 11:52:48 +0300 Felipe Balbi <me@felipebalbi.com> wrote:
> 
> > Wonder if it's possible to have the above patch applied so I can get rid
> > of those stubs in musb_io.h
> 
> Someone would need to take care of it and document it and repair all
> those driver which broke because they went and created private copies.
> 
> I could do some of that but I'm stuck at step #1.  What the heck _are_
> these things?  Why do they exist?  What are their semantics?
> 
> The arm implementation says
> 
>  * Generic IO read/write.  These perform native-endian accesses.  Note
>  * that some architectures will want to re-define __raw_{read,write}w.
> 
> which is somewhat clear as mud.  I'd guess that these are the MMIO
> partners to insl and friends?
> 
> And then it has
> 
> extern void __raw_writesb(void __iomem *addr, const void *data, int bytelen);
> extern void __raw_writesw(void __iomem *addr, const void *data, int wordlen);
> extern void __raw_writesl(void __iomem *addr, const void *data, int longlen);
> 
> which is a bit odd.  I assume that "bytelen" should have been
> "bytecount" or similar.

Clearing that out is as easy as asking Russel King for more details.

Russel, do you have any comments to the above ?

-- 
balbi

Re: drivers/usb/musb/musb_io.h

[Russell King]

On Fri, Aug 15, 2008 at 02:11:31AM -0700, Andrew Morton wrote:
> On Fri, 15 Aug 2008 11:52:48 +0300 Felipe Balbi <me-uiRdBs8odbtmTBlB0Cgj/Q@public.gmane.org> wrote:
> 
> > Wonder if it's possible to have the above patch applied so I can get rid
> > of those stubs in musb_io.h
> 
> Someone would need to take care of it and document it and repair all
> those driver which broke because they went and created private copies.
> 
> I could do some of that but I'm stuck at step #1.  What the heck _are_
> these things?  Why do they exist?  What are their semantics?
> 
> The arm implementation says
> 
>  * Generic IO read/write.  These perform native-endian accesses.  Note
>  * that some architectures will want to re-define __raw_{read,write}w.
> 
> which is somewhat clear as mud.  I'd guess that these are the MMIO
> partners to insl and friends?

Yes.

> And then it has
> 
> extern void __raw_writesb(void __iomem *addr, const void *data, int bytelen);
> extern void __raw_writesw(void __iomem *addr, const void *data, int wordlen);
> extern void __raw_writesl(void __iomem *addr, const void *data, int longlen);
> 
> which is a bit odd.

Shrug.  Everything is not a PC.  I don't see anything odd about the above.

We basically implement a standard set of IO macros (the __raw_*) stuff
which is used to implement the standard set of IO support (inl, insl)
and provide a set of extensions for our platforms (readsl to complement
insl - named precisely the same way because that's what they are) because
the generic stuff doesn't cover all our needs.

> I assume that "bytelen" should have been "bytecount" or similar.

bytelen and bytecount mean the same thing to me.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drivers/usb/musb/musb_io.h

[Russell King]

On Fri, Aug 15, 2008 at 02:11:31AM -0700, Andrew Morton wrote:
> On Fri, 15 Aug 2008 11:52:48 +0300 Felipe Balbi <me@felipebalbi.com> wrote:
> 
> > Wonder if it's possible to have the above patch applied so I can get rid
> > of those stubs in musb_io.h
> 
> Someone would need to take care of it and document it and repair all
> those driver which broke because they went and created private copies.
> 
> I could do some of that but I'm stuck at step #1.  What the heck _are_
> these things?  Why do they exist?  What are their semantics?
> 
> The arm implementation says
> 
>  * Generic IO read/write.  These perform native-endian accesses.  Note
>  * that some architectures will want to re-define __raw_{read,write}w.
> 
> which is somewhat clear as mud.  I'd guess that these are the MMIO
> partners to insl and friends?

Yes.

> And then it has
> 
> extern void __raw_writesb(void __iomem *addr, const void *data, int bytelen);
> extern void __raw_writesw(void __iomem *addr, const void *data, int wordlen);
> extern void __raw_writesl(void __iomem *addr, const void *data, int longlen);
> 
> which is a bit odd.

Shrug.  Everything is not a PC.  I don't see anything odd about the above.

We basically implement a standard set of IO macros (the __raw_*) stuff
which is used to implement the standard set of IO support (inl, insl)
and provide a set of extensions for our platforms (readsl to complement
insl - named precisely the same way because that's what they are) because
the generic stuff doesn't cover all our needs.

> I assume that "bytelen" should have been "bytecount" or similar.

bytelen and bytecount mean the same thing to me.

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

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On Fri, Aug 15, 2008 at 12:53:08PM +0100, Russell King wrote:
> Shrug.  Everything is not a PC.  I don't see anything odd about the above.
> 
> We basically implement a standard set of IO macros (the __raw_*) stuff
> which is used to implement the standard set of IO support (inl, insl)
> and provide a set of extensions for our platforms (readsl to complement
> insl - named precisely the same way because that's what they are) because
> the generic stuff doesn't cover all our needs.
> 
> > I assume that "bytelen" should have been "bytecount" or similar.
> 
> bytelen and bytecount mean the same thing to me.

Thanks Russel for the clarification.

So I think until we have a better solution we'll need the attached patch
to musb_io.h

Paul, is it ok for you ?

-- 
balbi

[-- Attachment #2: 0001-musb-superh-also-provides-read-write-friends.diff --]
[-- Type: text/x-diff, Size: 1051 bytes --]

From f3cba688c0447504f3812c570cf37732e1f9493b Mon Sep 17 00:00:00 2001
From: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
Date: Fri, 15 Aug 2008 10:51:56 +0300
Subject: [PATCH] musb: superh also provides read/write friends

do not redefine them unless we're not building for
scuch archs.

Signed-off-by: Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org>
Signed-off-by: Felipe Balbi <felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/musb/musb_io.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
index 6bbedae..d0f812a 100644
--- a/drivers/usb/musb/musb_io.h
+++ b/drivers/usb/musb/musb_io.h
@@ -37,7 +37,7 @@
 
 #include <linux/io.h>
 
-#ifndef	CONFIG_ARM
+#if !defined(CONFIG_ARM) && !defined(CONFIG_SUPERH)
 static inline void readsl(const void __iomem *addr, void *buf, int len)
 	{ insl((unsigned long)addr, buf, len); }
 static inline void readsw(const void __iomem *addr, void *buf, int len)
-- 
1.6.0.rc3.6.ga0653

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 774 bytes --]

On Fri, Aug 15, 2008 at 12:53:08PM +0100, Russell King wrote:
> Shrug.  Everything is not a PC.  I don't see anything odd about the above.
> 
> We basically implement a standard set of IO macros (the __raw_*) stuff
> which is used to implement the standard set of IO support (inl, insl)
> and provide a set of extensions for our platforms (readsl to complement
> insl - named precisely the same way because that's what they are) because
> the generic stuff doesn't cover all our needs.
> 
> > I assume that "bytelen" should have been "bytecount" or similar.
> 
> bytelen and bytecount mean the same thing to me.

Thanks Russel for the clarification.

So I think until we have a better solution we'll need the attached patch
to musb_io.h

Paul, is it ok for you ?

-- 
balbi

[-- Attachment #2: 0001-musb-superh-also-provides-read-write-friends.diff --]
[-- Type: text/x-diff, Size: 967 bytes --]

From f3cba688c0447504f3812c570cf37732e1f9493b Mon Sep 17 00:00:00 2001
From: Paul Mundt <lethal@linux-sh.org>
Date: Fri, 15 Aug 2008 10:51:56 +0300
Subject: [PATCH] musb: superh also provides read/write friends

do not redefine them unless we're not building for
scuch archs.

Signed-off-by: Paul Mundt <lethal@linux-sh.org>
Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/musb_io.h |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
index 6bbedae..d0f812a 100644
--- a/drivers/usb/musb/musb_io.h
+++ b/drivers/usb/musb/musb_io.h
@@ -37,7 +37,7 @@
 
 #include <linux/io.h>
 
-#ifndef	CONFIG_ARM
+#if !defined(CONFIG_ARM) && !defined(CONFIG_SUPERH)
 static inline void readsl(const void __iomem *addr, void *buf, int len)
 	{ insl((unsigned long)addr, buf, len); }
 static inline void readsw(const void __iomem *addr, void *buf, int len)
-- 
1.6.0.rc3.6.ga0653

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

On Fri, Aug 15, 2008 at 03:38:49PM +0300, Felipe Balbi wrote:
> So I think until we have a better solution we'll need the attached patch
> to musb_io.h

Another possible solution would be the attached. I think this new
version is a bit better since we won't have to keep adding other
architectures when we find out they provide read/write friends.

=== cut here ===

From ee8f7e4150e54139613208043561be557f35e3b3 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
Date: Fri, 15 Aug 2008 10:51:56 +0300
Subject: [PATCH] musb: io: only define read/write stubs if they're not defined yet

For those archs which don't provide read/write friends we
provide our own implementation so musb driver won't break
compilation.

Signed-off-by: Felipe Balbi <felipe.balbi-xNZwKgViW5gAvxtiuMwx3w@public.gmane.org>
---
 drivers/usb/musb/musb_io.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
index 6bbedae..222f5ab 100644
--- a/drivers/usb/musb/musb_io.h
+++ b/drivers/usb/musb/musb_io.h
@@ -37,21 +37,30 @@
 
 #include <linux/io.h>
 
-#ifndef	CONFIG_ARM
+#ifndef readsl
 static inline void readsl(const void __iomem *addr, void *buf, int len)
 	{ insl((unsigned long)addr, buf, len); }
+#endif
+#ifndef readsw
 static inline void readsw(const void __iomem *addr, void *buf, int len)
 	{ insw((unsigned long)addr, buf, len); }
+#endif
+#ifndef readsb
 static inline void readsb(const void __iomem *addr, void *buf, int len)
 	{ insb((unsigned long)addr, buf, len); }
+#endif
 
+#ifndef writesl
 static inline void writesl(const void __iomem *addr, const void *buf, int len)
 	{ outsl((unsigned long)addr, buf, len); }
+#endif
+#ifndef writesw
 static inline void writesw(const void __iomem *addr, const void *buf, int len)
 	{ outsw((unsigned long)addr, buf, len); }
+#endif
+#ifndef writesb
 static inline void writesb(const void __iomem *addr, const void *buf, int len)
 	{ outsb((unsigned long)addr, buf, len); }

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

On Fri, Aug 15, 2008 at 03:38:49PM +0300, Felipe Balbi wrote:
> So I think until we have a better solution we'll need the attached patch
> to musb_io.h

Another possible solution would be the attached. I think this new
version is a bit better since we won't have to keep adding other
architectures when we find out they provide read/write friends.

=== cut here ===

From ee8f7e4150e54139613208043561be557f35e3b3 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Fri, 15 Aug 2008 10:51:56 +0300
Subject: [PATCH] musb: io: only define read/write stubs if they're not defined yet

For those archs which don't provide read/write friends we
provide our own implementation so musb driver won't break
compilation.

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/musb_io.h |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
index 6bbedae..222f5ab 100644
--- a/drivers/usb/musb/musb_io.h
+++ b/drivers/usb/musb/musb_io.h
@@ -37,21 +37,30 @@
 
 #include <linux/io.h>
 
-#ifndef	CONFIG_ARM
+#ifndef readsl
 static inline void readsl(const void __iomem *addr, void *buf, int len)
 	{ insl((unsigned long)addr, buf, len); }
+#endif
+#ifndef readsw
 static inline void readsw(const void __iomem *addr, void *buf, int len)
 	{ insw((unsigned long)addr, buf, len); }
+#endif
+#ifndef readsb
 static inline void readsb(const void __iomem *addr, void *buf, int len)
 	{ insb((unsigned long)addr, buf, len); }
+#endif
 
+#ifndef writesl
 static inline void writesl(const void __iomem *addr, const void *buf, int len)
 	{ outsl((unsigned long)addr, buf, len); }
+#endif
+#ifndef writesw
 static inline void writesw(const void __iomem *addr, const void *buf, int len)
 	{ outsw((unsigned long)addr, buf, len); }
+#endif
+#ifndef writesb
 static inline void writesb(const void __iomem *addr, const void *buf, int len)
 	{ outsb((unsigned long)addr, buf, len); }
-
 #endif
 
 /* NOTE:  these offsets are all in bytes */
-- 
1.6.0.rc3.6.ga0653

-- 
balbi

Re: drivers/usb/musb/musb_io.h

[David Brownell]

On Friday 15 August 2008, Felipe Balbi wrote:
> So I think until we have a better solution we'll need the attached patch
> to musb_io.h

Probably worth adding AVR32 to the list too.  I happened to
enable musb during avr32 test build; it kablooied there too,
but behaves given the CONFIG_AVR32 #ifdef.

The existing #ifdef was sufficient to get a clean x86 build,
given basic <linux/clk.h> support.  Someone wanting to see this
driver work on the PCI (FPGA?) incarnation of this IP will
have to solve that problem too.

- Dave

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1398 bytes --]

On Fri, Aug 15, 2008 at 02:46:45PM -0700, David Brownell wrote:
> On Friday 15 August 2008, Felipe Balbi wrote:
> > So I think until we have a better solution we'll need the attached patch
> > to musb_io.h
> 
> Probably worth adding AVR32 to the list too.  I happened to
> enable musb during avr32 test build; it kablooied there too,
> but behaves given the CONFIG_AVR32 #ifdef.

Good to know, thanks.

Hmm... but how about the other version?

+#ifndef readsl
 static inline void readsl(const void __iomem *addr, void *buf, int len)
        { insl((unsigned long)addr, buf, len); }
+#endif
+#ifndef readsw
 static inline void readsw(const void __iomem *addr, void *buf, int len)
        { insw((unsigned long)addr, buf, len); }
+#endif

I think this would be a bit better since we don't need to keep adding
several archs to the list and prevents anything from breaking...
although those ifdefs look a bit odd.

> The existing #ifdef was sufficient to get a clean x86 build,
> given basic <linux/clk.h> support.  Someone wanting to see this
> driver work on the PCI (FPGA?) incarnation of this IP will
> have to solve that problem too.

Yeah... I have one of those OPTs, was even wondering on starting the pci
bus glue, but I'm too busy with nokia internal stuff for playing around
now :-s

Hope someone ever implement that, would be fun to see :-)

-- 
balbi

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: drivers/usb/musb/musb_io.h

[David Brownell]

On Friday 15 August 2008, Felipe Balbi wrote:
> Hmm... but how about the other version?
> 
> +#ifndef readsl
>  static inline void readsl(const void __iomem *addr, void *buf, int len)
>         { insl((unsigned long)addr, buf, len); }
> +#endif
> +#ifndef readsw
>  static inline void readsw(const void __iomem *addr, void *buf, int len)
>         { insw((unsigned long)addr, buf, len); }
> +#endif

Any arch defining an inline readsl() etc will break.  And
inlines are the preferred style lately.  :)

Someone commented that this is what caused creation of
the mmio_insl() family of routines ...

... but I notice that at least on ARM (current primary home
for this driver!) those calls don't end up using optimized
code. Since those optimimzed loops are a *big* win in terms
of performance -- using LDM/STM to burst memory acccess to/from
registers -- I'd rather not go that way for now.

- Dave


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: drivers/usb/musb/musb_io.h

[David Brownell]

On Friday 15 August 2008, Felipe Balbi wrote:
> Hmm... but how about the other version?
> 
> +#ifndef readsl
>  static inline void readsl(const void __iomem *addr, void *buf, int len)
>         { insl((unsigned long)addr, buf, len); }
> +#endif
> +#ifndef readsw
>  static inline void readsw(const void __iomem *addr, void *buf, int len)
>         { insw((unsigned long)addr, buf, len); }
> +#endif

Any arch defining an inline readsl() etc will break.  And
inlines are the preferred style lately.  :)

Someone commented that this is what caused creation of
the mmio_insl() family of routines ...

... but I notice that at least on ARM (current primary home
for this driver!) those calls don't end up using optimized
code. Since those optimimzed loops are a *big* win in terms
of performance -- using LDM/STM to burst memory acccess to/from
registers -- I'd rather not go that way for now.

- Dave

Re: drivers/usb/musb/musb_io.h

[David Brownell]

On Friday 15 August 2008, David Brownell wrote:
> Someone commented that this is what caused creation of
> the mmio_insl() family of routines ...
> 
> ... but I notice that at least on ARM (current primary home
> for this driver!) those calls don't end up using optimized
> code. Since those optimimzed loops are a *big* win in terms
> of performance -- using LDM/STM to burst memory acccess to/from
> registers -- I'd rather not go that way for now.

Oh, and worth remembering:  *WHEN* a clean solution for this
is available, it should be usable in the NAND code.  And surely
in other places which sometimes need PIO access to a FIFO...

I was surprised to see that 16% improvement in read performance
with an 8-bit NAND device, just by switching from code looking
like generic mmio_insb() over to __raw_readsb().  Better IMO to
have the faster code be what the NAND core uses by default, than
to need to modify every platform's NAND driver to do that.

- Dave

Re: drivers/usb/musb/musb_io.h

[Felipe Balbi]

[-- Attachment #1.1: Type: text/plain, Size: 1254 bytes --]

On Fri, Aug 15, 2008 at 07:05:31PM -0700, David Brownell wrote:
> On Friday 15 August 2008, David Brownell wrote:
> > Someone commented that this is what caused creation of
> > the mmio_insl() family of routines ...
> > 
> > ... but I notice that at least on ARM (current primary home
> > for this driver!) those calls don't end up using optimized
> > code. Since those optimimzed loops are a *big* win in terms
> > of performance -- using LDM/STM to burst memory acccess to/from
> > registers -- I'd rather not go that way for now.
> 
> Oh, and worth remembering:  *WHEN* a clean solution for this
> is available, it should be usable in the NAND code.  And surely
> in other places which sometimes need PIO access to a FIFO...
> 
> I was surprised to see that 16% improvement in read performance
> with an 8-bit NAND device, just by switching from code looking
> like generic mmio_insb() over to __raw_readsb().  Better IMO to
> have the faster code be what the NAND core uses by default, than
> to need to modify every platform's NAND driver to do that.

Sure, I'll have to keep an eye on what's going on include/linux/io.h

Attached is a new patch, added also ppc since they also provide those
read/write calls.

-- 
balbi

[-- Attachment #1.2: 0001-musb-io-only-define-read-write-stubs-if-they-re-not.diff --]
[-- Type: text/plain, Size: 1365 bytes --]

From e6df3ab0b49e5a52c0b0b089d38efba97dd2f870 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <felipe.balbi@nokia.com>
Date: Fri, 15 Aug 2008 10:51:56 +0300
Subject: [PATCH] musb: io: only define read/write stubs if they're not defined yet

For those archs which don't provide read/write friends we
provide our own implementation so musb driver won't break
compilation.

This is temporary fix until a better solution comes from
upstream. Idealy, <linux/io.h> would provide those calls
if the architecture did not provide them yet. In that case
being possible to remove all those stubs from musb_io.h

Signed-off-by: Felipe Balbi <felipe.balbi@nokia.com>
---
 drivers/usb/musb/musb_io.h |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/drivers/usb/musb/musb_io.h b/drivers/usb/musb/musb_io.h
index 6bbedae..223f0a5 100644
--- a/drivers/usb/musb/musb_io.h
+++ b/drivers/usb/musb/musb_io.h
@@ -37,7 +37,9 @@
 
 #include <linux/io.h>
 
-#ifndef	CONFIG_ARM
+#if !defined(CONFIG_ARM) && !defined(CONFIG_SUPERH) \
+	&& !defined(CONFIG_AVR32) && !defined(CONFIG_PPC32) \
+	&& !defined(CONFIG_PPC64)
 static inline void readsl(const void __iomem *addr, void *buf, int len)
 	{ insl((unsigned long)addr, buf, len); }
 static inline void readsw(const void __iomem *addr, void *buf, int len)
-- 
1.6.0.rc3.10.g5a13c


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: drivers/usb/musb/musb_io.h

[Geert Uytterhoeven]

On Fri, 15 Aug 2008, Felipe Balbi wrote:
> On Fri, Aug 15, 2008 at 03:38:49PM +0300, Felipe Balbi wrote:
> +#ifndef writesl
>  static inline void writesl(const void __iomem *addr, const void *buf, int len)
>  	{ outsl((unsigned long)addr, buf, len); }
> +#endif
> +#ifndef writesw
>  static inline void writesw(const void __iomem *addr, const void *buf, int len)
>  	{ outsw((unsigned long)addr, buf, len); }
> +#endif
> +#ifndef writesb
>  static inline void writesb(const void __iomem *addr, const void *buf, int len)
>  	{ outsb((unsigned long)addr, buf, len); }
> -
>  #endif

... which may cause silent memory corruption when somebody enables this
driver on a platform where the above routines don't do what they should
do?

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