[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Uwe Kleine-König]

Commit

	09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)

introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
To make the driver compile again this use has to be protected by
an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
is #ifdefed out, too.

Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
---
Hello,

I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
a flush ARM probably does, too, no?
If so, what it the right thing to do? Implement flush_dcache_range for
ARM (just wrapping flush_dcache_page?)?

Best regards
Uwe

 drivers/usb/gadget/fsl_udc_core.c |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
index 999eafe..47152e0 100644
--- a/drivers/usb/gadget/fsl_udc_core.c
+++ b/drivers/usb/gadget/fsl_udc_core.c
@@ -1333,8 +1333,10 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
 	/* Fill in the reqest structure */
 	*((u16 *) req->req.buf) = cpu_to_le16(tmp);
 
+#ifdef CONFIG_PPC32
 	/* flush cache for the req buffer */
 	flush_dcache_range((u32)req->req.buf, (u32)req->req.buf + 8);
+#endif
 
 	req->ep = ep;
 	req->req.length = 2;
@@ -2455,6 +2457,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 	}
 
 	/* Set accessors only after pdata->init() ! */
+#ifdef CONFIG_PPC32
 	if (pdata->big_endian_mmio) {
 		_fsl_readl = _fsl_readl_be;
 		_fsl_writel = _fsl_writel_be;
@@ -2462,6 +2465,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
 		_fsl_readl = _fsl_readl_le;
 		_fsl_writel = _fsl_writel_le;
 	}
+#endif
 
 #ifndef CONFIG_ARCH_MXC
 	if (pdata->have_sysif_regs)
-- 
1.7.4.1

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Russell King - ARM Linux]

On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> a flush ARM probably does, too, no?
> If so, what it the right thing to do? Implement flush_dcache_range for
> ARM (just wrapping flush_dcache_page?)?

On ARM, we assume all new pages have dirty dcache, which allows us to
neatly end the ever-increasing quantity of drivers which need to be
patched to work on ARM.  PowerPC doesn't do this.

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Uwe Kleine-König]

Hi Russell,

On Fri, May 06, 2011 at 10:04:49AM +0100, Russell King - ARM Linux wrote:
> On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> > a flush ARM probably does, too, no?
> > If so, what it the right thing to do? Implement flush_dcache_range for
> > ARM (just wrapping flush_dcache_page?)?
> 
> On ARM, we assume all new pages have dirty dcache, which allows us to
> neatly end the ever-increasing quantity of drivers which need to be
> patched to work on ARM.  PowerPC doesn't do this.
Unless I'm missing something the cache flushed here isn't necessarily
for a new page. (And even if the flush isn't needed here for ARM, not
having flush_dcache_range results in #ifdefs for each user of it. So a
definition would be nice, wouldn't it?)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Russell King - ARM Linux]

On Fri, May 06, 2011 at 12:03:10PM +0200, Uwe Kleine-K?nig wrote:
> Hi Russell,
> 
> On Fri, May 06, 2011 at 10:04:49AM +0100, Russell King - ARM Linux wrote:
> > On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> > > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> > > a flush ARM probably does, too, no?
> > > If so, what it the right thing to do? Implement flush_dcache_range for
> > > ARM (just wrapping flush_dcache_page?)?
> > 
> > On ARM, we assume all new pages have dirty dcache, which allows us to
> > neatly end the ever-increasing quantity of drivers which need to be
> > patched to work on ARM.  PowerPC doesn't do this.
> Unless I'm missing something the cache flushed here isn't necessarily
> for a new page. (And even if the flush isn't needed here for ARM, not
> having flush_dcache_range results in #ifdefs for each user of it. So a
> definition would be nice, wouldn't it?)

Well, the folk introducing it should have added it to cachetlb.txt so
that other folk know what the intentions of it are.  At the moment it
seems to be an unofficial extension with unknown semantics.  Maybe the
PPC folk can clear it up and fix other arches if they wish to make it
an official arch cachetlb extension.

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Uwe Kleine-König]

Hello Greg,

On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> Commit
> 
> 	09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
> 
> introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
> CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
> To make the driver compile again this use has to be protected by
> an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
> is #ifdefed out, too.
> 
> Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> ---
> Hello,
> 
> I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> a flush ARM probably does, too, no?
> If so, what it the right thing to do? Implement flush_dcache_range for
> ARM (just wrapping flush_dcache_page?)?
As Russell seem to be OK with the #ifdef, can you please take this
patch?
 
Thanks
Uwe

>  drivers/usb/gadget/fsl_udc_core.c |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> index 999eafe..47152e0 100644
> --- a/drivers/usb/gadget/fsl_udc_core.c
> +++ b/drivers/usb/gadget/fsl_udc_core.c
> @@ -1333,8 +1333,10 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
>  	/* Fill in the reqest structure */
>  	*((u16 *) req->req.buf) = cpu_to_le16(tmp);
>  
> +#ifdef CONFIG_PPC32
>  	/* flush cache for the req buffer */
>  	flush_dcache_range((u32)req->req.buf, (u32)req->req.buf + 8);
> +#endif
>  
>  	req->ep = ep;
>  	req->req.length = 2;
> @@ -2455,6 +2457,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  	}
>  
>  	/* Set accessors only after pdata->init() ! */
> +#ifdef CONFIG_PPC32
>  	if (pdata->big_endian_mmio) {
>  		_fsl_readl = _fsl_readl_be;
>  		_fsl_writel = _fsl_writel_be;
> @@ -2462,6 +2465,7 @@ static int __init fsl_udc_probe(struct platform_device *pdev)
>  		_fsl_readl = _fsl_readl_le;
>  		_fsl_writel = _fsl_writel_le;
>  	}
> +#endif
>  
>  #ifndef CONFIG_ARCH_MXC
>  	if (pdata->have_sysif_regs)

-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Greg KH]

On Wed, May 11, 2011 at 08:39:54AM +0200, Uwe Kleine-K?nig wrote:
> Hello Greg,
> 
> On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> > Commit
> > 
> > 	09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
> > 
> > introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
> > CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
> > To make the driver compile again this use has to be protected by
> > an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
> > is #ifdefed out, too.
> > 
> > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > ---
> > Hello,
> > 
> > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> > a flush ARM probably does, too, no?
> > If so, what it the right thing to do? Implement flush_dcache_range for
> > ARM (just wrapping flush_dcache_page?)?
> As Russell seem to be OK with the #ifdef, can you please take this
> patch?

Ick, no.

> >  drivers/usb/gadget/fsl_udc_core.c |    4 ++++
> >  1 files changed, 4 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/usb/gadget/fsl_udc_core.c b/drivers/usb/gadget/fsl_udc_core.c
> > index 999eafe..47152e0 100644
> > --- a/drivers/usb/gadget/fsl_udc_core.c
> > +++ b/drivers/usb/gadget/fsl_udc_core.c
> > @@ -1333,8 +1333,10 @@ static void ch9getstatus(struct fsl_udc *udc, u8 request_type, u16 value,
> >  	/* Fill in the reqest structure */
> >  	*((u16 *) req->req.buf) = cpu_to_le16(tmp);
> >  
> > +#ifdef CONFIG_PPC32
> >  	/* flush cache for the req buffer */
> >  	flush_dcache_range((u32)req->req.buf, (u32)req->req.buf + 8);
> > +#endif

Come on, we don't have ifdefs in .c files for a reason, surely there is
a better fix for this some other way?

thanks,

greg k-h

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Russell King - ARM Linux]

On Wed, May 11, 2011 at 08:54:46AM -0700, Greg KH wrote:
> On Wed, May 11, 2011 at 08:39:54AM +0200, Uwe Kleine-K?nig wrote:
> > Hello Greg,
> > 
> > On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> > > Commit
> > > 
> > > 	09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
> > > 
> > > introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
> > > CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
> > > To make the driver compile again this use has to be protected by
> > > an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
> > > is #ifdefed out, too.
> > > 
> > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > ---
> > > Hello,
> > > 
> > > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> > > a flush ARM probably does, too, no?
> > > If so, what it the right thing to do? Implement flush_dcache_range for
> > > ARM (just wrapping flush_dcache_page?)?
> > As Russell seem to be OK with the #ifdef, can you please take this
> > patch?
> 
> Ick, no.

I never said I was OK with the #ifdef...

> Come on, we don't have ifdefs in .c files for a reason, surely there is
> a better fix for this some other way?

I suggested ways to fix it, which apparantly got ignored.  To repeat myself,
what I said was:

"Well, the folk introducing it should have added it to cachetlb.txt so
that other folk know what the intentions of it are.  At the moment it
seems to be an unofficial extension with unknown semantics.  Maybe the
PPC folk can clear it up and fix other arches if they wish to make it
an official arch cachetlb extension."

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Uwe Kleine-König]

On Wed, May 11, 2011 at 05:29:02PM +0100, Russell King - ARM Linux wrote:
> On Wed, May 11, 2011 at 08:54:46AM -0700, Greg KH wrote:
> > On Wed, May 11, 2011 at 08:39:54AM +0200, Uwe Kleine-K?nig wrote:
> > > Hello Greg,
> > > 
> > > On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> > > > Commit
> > > > 
> > > > 	09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
> > > > 
> > > > introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
> > > > CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
> > > > To make the driver compile again this use has to be protected by
> > > > an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
> > > > is #ifdefed out, too.
> > > > 
> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> > > > ---
> > > > Hello,
> > > > 
> > > > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> > > > a flush ARM probably does, too, no?
> > > > If so, what it the right thing to do? Implement flush_dcache_range for
> > > > ARM (just wrapping flush_dcache_page?)?
> > > As Russell seem to be OK with the #ifdef, can you please take this
> > > patch?
> > 
> > Ick, no.
> 
> I never said I was OK with the #ifdef...
Ah, right, sorry.

> > Come on, we don't have ifdefs in .c files for a reason, surely there is
> > a better fix for this some other way?
> 
> I suggested ways to fix it, which apparantly got ignored.  To repeat myself,
> what I said was:
> 
> "Well, the folk introducing it should have added it to cachetlb.txt so
> that other folk know what the intentions of it are.  At the moment it
> seems to be an unofficial extension with unknown semantics.  Maybe the
> PPC folk can clear it up and fix other arches if they wish to make it
> an official arch cachetlb extension."
So what should we do? The fsl_udc_core driver currently fails to build
because it uses flush_dcache_range unconditionally. So the obvious ways
to fix it are:

 1) define flush_dcache_range
   a) in arch/arm
   b) in the driver
 2) make the driver not use flush_dcache_range
   a) #ifdef 
   b) revert 09ba0de (and probably some commits that depend on it)
   c) use a different function to flush the cache


-- 
Pengutronix e.K.                           | Uwe Kleine-K?nig            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Eric Miao]

2011/5/22 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> On Wed, May 11, 2011 at 05:29:02PM +0100, Russell King - ARM Linux wrote:
>> On Wed, May 11, 2011 at 08:54:46AM -0700, Greg KH wrote:
>> > On Wed, May 11, 2011 at 08:39:54AM +0200, Uwe Kleine-K?nig wrote:
>> > > Hello Greg,
>> > >
>> > > On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
>> > > > Commit
>> > > >
>> > > > ? ? ? ? 09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
>> > > >
>> > > > introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
>> > > > CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
>> > > > To make the driver compile again this use has to be protected by
>> > > > an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
>> > > > is #ifdefed out, too.
>> > > >
>> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
>> > > > ---
>> > > > Hello,
>> > > >
>> > > > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
>> > > > a flush ARM probably does, too, no?
>> > > > If so, what it the right thing to do? Implement flush_dcache_range for
>> > > > ARM (just wrapping flush_dcache_page?)?
>> > > As Russell seem to be OK with the #ifdef, can you please take this
>> > > patch?
>> >
>> > Ick, no.
>>
>> I never said I was OK with the #ifdef...
> Ah, right, sorry.
>
>> > Come on, we don't have ifdefs in .c files for a reason, surely there is
>> > a better fix for this some other way?
>>
>> I suggested ways to fix it, which apparantly got ignored. ?To repeat myself,
>> what I said was:
>>
>> "Well, the folk introducing it should have added it to cachetlb.txt so
>> that other folk know what the intentions of it are. ?At the moment it
>> seems to be an unofficial extension with unknown semantics. ?Maybe the
>> PPC folk can clear it up and fix other arches if they wish to make it
>> an official arch cachetlb extension."
> So what should we do? The fsl_udc_core driver currently fails to build
> because it uses flush_dcache_range unconditionally. So the obvious ways
> to fix it are:
>
> ?1) define flush_dcache_range
> ? a) in arch/arm
> ? b) in the driver
> ?2) make the driver not use flush_dcache_range
> ? a) #ifdef
> ? b) revert 09ba0de (and probably some commits that depend on it)
> ? c) use a different function to flush the cache

I see no reason why a dma_map_*() function couldn't be used here?

This is preventing the driver from being built. And the _fsl_readl stuffs,
which was really introduced by PPC guys, yet it can be made generic
enough I believe. The real requirement is really for a single kernel binary
to run on different LE/BE configurations, which will hurt performance
a bit and is normally not true on the i.MX SoCs, so that can be made as
a config option in my POV, but not necessary #ifdef CONFIG_PPC32,
maybe some better name

PS: why PPC32 is using io_*() API instead of readl/writel(), ISTR PPC32
doesn't have a separate IO space.

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Russell King - ARM Linux]

On Tue, Jun 21, 2011 at 05:32:32PM +0800, Eric Miao wrote:
> 2011/5/22 Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>:
> > On Wed, May 11, 2011 at 05:29:02PM +0100, Russell King - ARM Linux wrote:
> >> On Wed, May 11, 2011 at 08:54:46AM -0700, Greg KH wrote:
> >> > On Wed, May 11, 2011 at 08:39:54AM +0200, Uwe Kleine-K?nig wrote:
> >> > > Hello Greg,
> >> > >
> >> > > On Fri, May 06, 2011 at 11:00:05AM +0200, Uwe Kleine-K?nig wrote:
> >> > > > Commit
> >> > > >
> >> > > > ? ? ? ? 09ba0de (USB: fsl_udc_core: prepare for SoCs with BE registers and descriptors)
> >> > > >
> >> > > > introduced two function pointers _fsl_readl and _fsl_writel in an #ifdef
> >> > > > CONFIG_PPC32 block and used then unconditionally in fsl_udc_probe.
> >> > > > To make the driver compile again this use has to be protected by
> >> > > > an #ifdef, too. Moreover ARM doesn't have flush_dcache_range so this
> >> > > > is #ifdefed out, too.
> >> > > >
> >> > > > Signed-off-by: Uwe Kleine-K?nig <u.kleine-koenig@pengutronix.de>
> >> > > > ---
> >> > > > Hello,
> >> > > >
> >> > > > I'm unsure about getting rid of the flush_dcache_range. If powerpc needs
> >> > > > a flush ARM probably does, too, no?
> >> > > > If so, what it the right thing to do? Implement flush_dcache_range for
> >> > > > ARM (just wrapping flush_dcache_page?)?
> >> > > As Russell seem to be OK with the #ifdef, can you please take this
> >> > > patch?
> >> >
> >> > Ick, no.
> >>
> >> I never said I was OK with the #ifdef...
> > Ah, right, sorry.
> >
> >> > Come on, we don't have ifdefs in .c files for a reason, surely there is
> >> > a better fix for this some other way?
> >>
> >> I suggested ways to fix it, which apparantly got ignored. ?To repeat myself,
> >> what I said was:
> >>
> >> "Well, the folk introducing it should have added it to cachetlb.txt so
> >> that other folk know what the intentions of it are. ?At the moment it
> >> seems to be an unofficial extension with unknown semantics. ?Maybe the
> >> PPC folk can clear it up and fix other arches if they wish to make it
> >> an official arch cachetlb extension."
> > So what should we do? The fsl_udc_core driver currently fails to build
> > because it uses flush_dcache_range unconditionally. So the obvious ways
> > to fix it are:
> >
> > ?1) define flush_dcache_range
> > ? a) in arch/arm
> > ? b) in the driver
> > ?2) make the driver not use flush_dcache_range
> > ? a) #ifdef
> > ? b) revert 09ba0de (and probably some commits that depend on it)
> > ? c) use a different function to flush the cache
> 
> I see no reason why a dma_map_*() function couldn't be used here?
> 
> This is preventing the driver from being built. And the _fsl_readl stuffs,
> which was really introduced by PPC guys, yet it can be made generic
> enough I believe. The real requirement is really for a single kernel binary
> to run on different LE/BE configurations, which will hurt performance
> a bit and is normally not true on the i.MX SoCs, so that can be made as
> a config option in my POV, but not necessary #ifdef CONFIG_PPC32,
> maybe some better name

Hang on, let me get this straight.

This is an iMX driver, which was written avoiding the DMA API despite
it clearly doing DMA related stuff.  Allocating memory from kmalloc,
translating it to physical addresses using virt_to_phys, etc.

The PPC people decided to reuse it, and ended up introducing some custom
cache flushing APIs to get around the lack of dma_map_* usage.

Given that, I'm not surprised that this has ended up being broken.  Had
it been written _properly_ from the start (iow, using the DMA API) then
maybe the driver wouldn't be broken now.

I think the iMX people have got their just deserts for this.  There's a
lesson to be learnt there - always use the correct APIs from day one.

So, I have no sympathy for this self-inflicted breakage.  The solution:
fix the driver to use the proper APIs.

[PATCH] [RFC] USB: fsl_udc_core: fix build-failure for ARM

[Russell King - ARM Linux]

On Tue, Jun 21, 2011 at 10:48:34AM +0100, Russell King - ARM Linux wrote:
> This is an iMX driver, which was written avoiding the DMA API despite
> it clearly doing DMA related stuff.  Allocating memory from kmalloc,
> translating it to physical addresses using virt_to_phys, etc.
> 
> The PPC people decided to reuse it, and ended up introducing some custom
> cache flushing APIs to get around the lack of dma_map_* usage.

Oops, no it wasn't - it was a freescale driver.  git log didn't go back
past the rename the imx people did.

But still, it needs converting to the DMA API rather than these private
arch bodges being introduced.