Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Felipe Balbi]

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

Hi,

On Mon, Sep 24, 2012 at 02:53:24PM +0300, philippedeswert@gmail.com wrote:
> From: Philippe De Swert <philippe.deswert@jollamobile.com>
> 
> Unless generic_interrupt is defined the irq setup
> in musb_init_controller fails for the omap2430 driver
> since that one does not set it's own interrupt handler.
> Which leaves usb non-functional.
> 
> Tested on N950/N9.
> 
> Signed-off-by: Philippe De Swert <philippedeswert@gmail.com>

SoB's mail doesn't From mail.

> ---
>  drivers/usb/musb/musb_core.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index db3dff8..ff9e482 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1499,7 +1499,8 @@ static int __devinit musb_core_init(u16 musb_type, struct musb *musb)
>  /*-------------------------------------------------------------------------*/
>  
>  #if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
> +	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500) || \
> +	defined(CONFIG_ARCH_OMAP2PLUS)

Weird, how can you build OMAP2PLUS without SOC_OMAPXXXX ??

BTW, this is also wrong as OMAP2PLUS is also used to enable AM3xxx and
TI8xxx, and those platforms don't use generic_interrupt().

-- 
balbi

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

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Felipe Balbi]

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

On Mon, Sep 24, 2012 at 03:15:39PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Sep 24, 2012 at 02:53:24PM +0300, philippedeswert@gmail.com wrote:
> > From: Philippe De Swert <philippe.deswert@jollamobile.com>
> > 
> > Unless generic_interrupt is defined the irq setup
> > in musb_init_controller fails for the omap2430 driver
> > since that one does not set it's own interrupt handler.
> > Which leaves usb non-functional.
> > 
> > Tested on N950/N9.
> > 
> > Signed-off-by: Philippe De Swert <philippedeswert@gmail.com>
> 
> SoB's mail doesn't From mail.

doesn't 'match'.

-- 
balbi

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

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Philippe De Swert]

Hi,

On 24/09/2012, Felipe Balbi <balbi@ti.com> wrote:
> SoB's mail doesn't From mail.

Well still in the progress of migrating of my personal to work laptop.
Since the patch does not seem correct the replacement will have
matching addresses.

>> /*-------------------------------------------------------------------------*/
>>
>>  #if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
>> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
>> +	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500) || \
>> +	defined(CONFIG_ARCH_OMAP2PLUS)
>
> Weird, how can you build OMAP2PLUS without SOC_OMAPXXXX ??

It seems entirely possible. I quickly tried to look if it got defined
somewhere and it does not seem to be set anywhere.  That is why I got
the impression it was replaced by CONFIG_ARCH_OMAP2PLUS. I'll dig
deeper to find out why SOC_OMAPXXXX is not set if it should be.

>From the .config I got (used menuconfig)

#
# TI OMAP2/3/4 Specific Features
#
CONFIG_ARCH_OMAP2PLUS_TYPICAL=y
CONFIG_SOC_HAS_OMAP2_SDRC=y
# CONFIG_ARCH_OMAP2 is not set
CONFIG_ARCH_OMAP3=y
# CONFIG_ARCH_OMAP4 is not set
# CONFIG_SOC_OMAP5 is not set
# CONFIG_SOC_OMAP3430 is not set
# CONFIG_SOC_TI81XX is not set
# CONFIG_SOC_AM33XX is not set
CONFIG_OMAP_PACKAGE_CBB=y

Not a mention of CONFIG_SOC_OMAP2430 and  CONFIG_SOC_OMAP3430 did not
get set (while it is not a 3430 but a 3630 I am using). Maybe
CONFIG_ARCH_OMAP3 would have been a better choice then?

> BTW, this is also wrong as OMAP2PLUS is also used to enable AM3xxx and
> TI8xxx, and those platforms don't use generic_interrupt().

It would not break them from what I could see in musb_core.c

        musb->isr = generic_interrupt;
        status = musb_platform_init(musb); <--- isr would be (re)set here
        if (status < 0)
                goto fail1;

        if (!musb->isr) {
                status = -ENODEV;
                goto fail2;
        }

The two you mention set their own interrupt routines.
drivers/usb/musb/da8xx.c:	musb->isr = da8xx_musb_interrupt;
drivers/usb/musb/am35x.c:	musb->isr = am35x_musb_interrupt;


Regards,

Philippe

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Felipe Balbi]

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

Hi,

On Mon, Sep 24, 2012 at 03:54:22PM +0300, Philippe De Swert wrote:
> Hi,
> 
> On 24/09/2012, Felipe Balbi <balbi@ti.com> wrote:
> > SoB's mail doesn't From mail.
> 
> Well still in the progress of migrating of my personal to work laptop.
> Since the patch does not seem correct the replacement will have
> matching addresses.
> 
> >> /*-------------------------------------------------------------------------*/
> >>
> >>  #if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
> >> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
> >> +	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500) || \
> >> +	defined(CONFIG_ARCH_OMAP2PLUS)
> >
> > Weird, how can you build OMAP2PLUS without SOC_OMAPXXXX ??
> 
> It seems entirely possible. I quickly tried to look if it got defined
> somewhere and it does not seem to be set anywhere.  That is why I got
> the impression it was replaced by CONFIG_ARCH_OMAP2PLUS. I'll dig
> deeper to find out why SOC_OMAPXXXX is not set if it should be.
> 
> From the .config I got (used menuconfig)
> 
> #
> # TI OMAP2/3/4 Specific Features
> #
> CONFIG_ARCH_OMAP2PLUS_TYPICAL=y
> CONFIG_SOC_HAS_OMAP2_SDRC=y
> # CONFIG_ARCH_OMAP2 is not set
> CONFIG_ARCH_OMAP3=y
> # CONFIG_ARCH_OMAP4 is not set
> # CONFIG_SOC_OMAP5 is not set
> # CONFIG_SOC_OMAP3430 is not set
> # CONFIG_SOC_TI81XX is not set
> # CONFIG_SOC_AM33XX is not set
> CONFIG_OMAP_PACKAGE_CBB=y
> 
> Not a mention of CONFIG_SOC_OMAP2430 and  CONFIG_SOC_OMAP3430 did not
> get set (while it is not a 3430 but a 3630 I am using). Maybe
> CONFIG_ARCH_OMAP3 would have been a better choice then?

that's quite f**ked up. Tony, why doesn't SOC_OMAP3430 get set for all
OMAP3 boards ? And another question: why don't we have a matching
SOC_OMAP3530, SOC_OMAP3630 and so on ?

> > BTW, this is also wrong as OMAP2PLUS is also used to enable AM3xxx and
> > TI8xxx, and those platforms don't use generic_interrupt().
> 
> It would not break them from what I could see in musb_core.c
> 
>         musb->isr = generic_interrupt;
>         status = musb_platform_init(musb); <--- isr would be (re)set here

good point. Still that code would be hanging there with no users... Fair
enough, it's better to have extra code when it's not needed, then having
no code when it's needed :-)

That entire musb->isr crap is quite screwed up anyway and it's just
because TI's non-OMAP processors (daxxx, amxxx, ti8xxx, etc) decided
that the wrapper should read MUSB's address space (including IRQ
registers) and since MUSB's IRQ registers are clear-on-read we need to
handle the IRQ from wrapper drivers, which is pretty screwed up as well.

Ideally, we wouldn't have all of these exposed symbols flying around :-s

-- 
balbi

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

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Tony Lindgren]

* Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> [120924 06:08]:
> Hi,
> 
> On Mon, Sep 24, 2012 at 03:54:22PM +0300, Philippe De Swert wrote:
> > Hi,
> > 
> > On 24/09/2012, Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org> wrote:
> > > SoB's mail doesn't From mail.
> > 
> > Well still in the progress of migrating of my personal to work laptop.
> > Since the patch does not seem correct the replacement will have
> > matching addresses.
> > 
> > >> /*-------------------------------------------------------------------------*/
> > >>
> > >>  #if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
> > >> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
> > >> +	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500) || \
> > >> +	defined(CONFIG_ARCH_OMAP2PLUS)
> > >
> > > Weird, how can you build OMAP2PLUS without SOC_OMAPXXXX ??
> > 
> > It seems entirely possible. I quickly tried to look if it got defined
> > somewhere and it does not seem to be set anywhere.  That is why I got
> > the impression it was replaced by CONFIG_ARCH_OMAP2PLUS. I'll dig
> > deeper to find out why SOC_OMAPXXXX is not set if it should be.
> > 
> > From the .config I got (used menuconfig)
> > 
> > #
> > # TI OMAP2/3/4 Specific Features
> > #
> > CONFIG_ARCH_OMAP2PLUS_TYPICAL=y
> > CONFIG_SOC_HAS_OMAP2_SDRC=y
> > # CONFIG_ARCH_OMAP2 is not set
> > CONFIG_ARCH_OMAP3=y
> > # CONFIG_ARCH_OMAP4 is not set
> > # CONFIG_SOC_OMAP5 is not set
> > # CONFIG_SOC_OMAP3430 is not set
> > # CONFIG_SOC_TI81XX is not set
> > # CONFIG_SOC_AM33XX is not set
> > CONFIG_OMAP_PACKAGE_CBB=y
> > 
> > Not a mention of CONFIG_SOC_OMAP2430 and  CONFIG_SOC_OMAP3430 did not
> > get set (while it is not a 3430 but a 3630 I am using). Maybe
> > CONFIG_ARCH_OMAP3 would have been a better choice then?
> 
> that's quite f**ked up. Tony, why doesn't SOC_OMAP3430 get set for all
> OMAP3 boards ? And another question: why don't we have a matching
> SOC_OMAP3530, SOC_OMAP3630 and so on ?

ARCH_OMAP3 will probably eventually just disappear and
be replaced with ARCH_OMAP2PLUS + SOC_XXXX. But there's
no need to do it right now as most of that should be
internal to arch/arm/mach-omap2 anyways. I would just
set the driver to depend on ARCH_OMAP2PLUS, and rely on
the platform_data and DT to do something if specified.
That means that on 2420 musb should not probe unless
tusb6010 in specified.

It seems that things like fifo_mode and generic_interrupt
can all be set during the probe based on the platform_data
or DT passed information?
 
Regards,

Tony
--
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: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Felipe Balbi]

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

Hi,

On Mon, Sep 24, 2012 at 09:09:14AM -0700, Tony Lindgren wrote:
> * Felipe Balbi <balbi@ti.com> [120924 06:08]:
> > Hi,
> > 
> > On Mon, Sep 24, 2012 at 03:54:22PM +0300, Philippe De Swert wrote:
> > > Hi,
> > > 
> > > On 24/09/2012, Felipe Balbi <balbi@ti.com> wrote:
> > > > SoB's mail doesn't From mail.
> > > 
> > > Well still in the progress of migrating of my personal to work laptop.
> > > Since the patch does not seem correct the replacement will have
> > > matching addresses.
> > > 
> > > >> /*-------------------------------------------------------------------------*/
> > > >>
> > > >>  #if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
> > > >> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
> > > >> +	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500) || \
> > > >> +	defined(CONFIG_ARCH_OMAP2PLUS)
> > > >
> > > > Weird, how can you build OMAP2PLUS without SOC_OMAPXXXX ??
> > > 
> > > It seems entirely possible. I quickly tried to look if it got defined
> > > somewhere and it does not seem to be set anywhere.  That is why I got
> > > the impression it was replaced by CONFIG_ARCH_OMAP2PLUS. I'll dig
> > > deeper to find out why SOC_OMAPXXXX is not set if it should be.
> > > 
> > > From the .config I got (used menuconfig)
> > > 
> > > #
> > > # TI OMAP2/3/4 Specific Features
> > > #
> > > CONFIG_ARCH_OMAP2PLUS_TYPICAL=y
> > > CONFIG_SOC_HAS_OMAP2_SDRC=y
> > > # CONFIG_ARCH_OMAP2 is not set
> > > CONFIG_ARCH_OMAP3=y
> > > # CONFIG_ARCH_OMAP4 is not set
> > > # CONFIG_SOC_OMAP5 is not set
> > > # CONFIG_SOC_OMAP3430 is not set
> > > # CONFIG_SOC_TI81XX is not set
> > > # CONFIG_SOC_AM33XX is not set
> > > CONFIG_OMAP_PACKAGE_CBB=y
> > > 
> > > Not a mention of CONFIG_SOC_OMAP2430 and  CONFIG_SOC_OMAP3430 did not
> > > get set (while it is not a 3430 but a 3630 I am using). Maybe
> > > CONFIG_ARCH_OMAP3 would have been a better choice then?
> > 
> > that's quite f**ked up. Tony, why doesn't SOC_OMAP3430 get set for all
> > OMAP3 boards ? And another question: why don't we have a matching
> > SOC_OMAP3530, SOC_OMAP3630 and so on ?
> 
> ARCH_OMAP3 will probably eventually just disappear and
> be replaced with ARCH_OMAP2PLUS + SOC_XXXX. But there's
> no need to do it right now as most of that should be
> internal to arch/arm/mach-omap2 anyways. I would just
> set the driver to depend on ARCH_OMAP2PLUS, and rely on
> the platform_data and DT to do something if specified.
> That means that on 2420 musb should not probe unless
> tusb6010 in specified.
> 
> It seems that things like fifo_mode and generic_interrupt
> can all be set during the probe based on the platform_data
> or DT passed information?

Then maybe it's best to just remove the ifdefs and always provide
generic_interrupt() ?

Anyone against it ?

-- 
balbi

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

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Philippe De Swert]

Hi,
On 25/09/2012, Felipe Balbi <balbi@ti.com> wrote:
>> > > Not a mention of CONFIG_SOC_OMAP2430 and  CONFIG_SOC_OMAP3430 did not
>> > > get set (while it is not a 3430 but a 3630 I am using). Maybe
>> > > CONFIG_ARCH_OMAP3 would have been a better choice then?
>> >
>> > that's quite f**ked up. Tony, why doesn't SOC_OMAP3430 get set for all
>> > OMAP3 boards ? And another question: why don't we have a matching
>> > SOC_OMAP3530, SOC_OMAP3630 and so on ?
>>
>> ARCH_OMAP3 will probably eventually just disappear and
>> be replaced with ARCH_OMAP2PLUS + SOC_XXXX. But there's
>> no need to do it right now as most of that should be
>> internal to arch/arm/mach-omap2 anyways. I would just
>> set the driver to depend on ARCH_OMAP2PLUS, and rely on
>> the platform_data and DT to do something if specified.
>> That means that on 2420 musb should not probe unless
>> tusb6010 in specified.
>>
>> It seems that things like fifo_mode and generic_interrupt
>> can all be set during the probe based on the platform_data
>> or DT passed information?
>
> Then maybe it's best to just remove the ifdefs and always provide
> generic_interrupt() ?
>
> Anyone against it ?

Well it seems there are only two drivers that use it omap2430 and
ux500. Maybe we somehow link it to the drivers that need it? (I might
have missed other drivers but it looks like it is just those two)

Regards,

Philippe

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Praveena Nadahally]

On Tue, Sep 25, 2012 at 2:39 PM, Philippe De Swert
<philippedeswert@gmail.com> wrote:
> Hi,
> On 25/09/2012, Felipe Balbi <balbi@ti.com> wrote:

>> Then maybe it's best to just remove the ifdefs and always provide
>> generic_interrupt() ?
>>
>> Anyone against it ?

Providing generic_interrupt seems fine.

> Well it seems there are only two drivers that use it omap2430 and
> ux500. Maybe we somehow link it to the drivers that need it? (I might
> have missed other drivers but it looks like it is just those two)

One way I see is that omap2430 and ux500 implement the isr handler
in platform glue driver and then call the musb_interrupt() as done in
daxxx, amxxx, ti8xxx platforms. Then generic_interrupt can be removed.

RE: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Philippe De Swert]

Hi,
________________________________________
>On Tue, Sep 25, 2012 at 2:39 PM, Philippe De Swert
>>> Then maybe it's best to just remove the ifdefs and always provide
>>> generic_interrupt() ?
>>>
>>> Anyone against it ?
>
>Providing generic_interrupt seems fine.
>
>> Well it seems there are only two drivers that use it omap2430 and
>> ux500. Maybe we somehow link it to the drivers that need it? (I might
>> have missed other drivers but it looks like it is just those two)
>
>One way I see is that omap2430 and ux500 implement the isr handler
>in platform glue driver and then call the musb_interrupt() as done in
>daxxx, amxxx, ti8xxx platforms. Then generic_interrupt can be removed.

So something along those lines? (If this is the right way I will resend as a real patch)

>From deae78e1084749f340ae8b8aaeca51818d5bfc55 Mon Sep 17 00:00:00 2001
From: Philippe De Swert <philippe.deswert-4y2FMlU5MS8onNqTyK5kxQ@public.gmane.org>
Date: Wed, 26 Sep 2012 17:00:46 +0300
Subject: [PATCH 1/1] musb: Move generic_interrupt out of the way

Have all musb drivers define their own isr.

Signed-off-by: Philippe De Swert <philippe.deswert-4y2FMlU5MS8onNqTyK5kxQ@public.gmane.org>
---
 drivers/usb/musb/musb_core.c |   33 ++-------------------------------
 drivers/usb/musb/omap2430.c  |   22 ++++++++++++++++++++++
 drivers/usb/musb/ux500.c     |   21 +++++++++++++++++++++
 3 files changed, 45 insertions(+), 31 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 26f1bef..1d5ee34 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1496,35 +1496,6 @@ static int __devinit musb_core_init(u16 musb_type, struct musb *musb)
 	return 0;
 }
 
-/*-------------------------------------------------------------------------*/
-
-#if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
-	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
-
-static irqreturn_t generic_interrupt(int irq, void *__hci)
-{
-	unsigned long	flags;
-	irqreturn_t	retval = IRQ_NONE;
-	struct musb	*musb = __hci;
-
-	spin_lock_irqsave(&musb->lock, flags);
-
-	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
-	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
-	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
-
-	if (musb->int_usb || musb->int_tx || musb->int_rx)
-		retval = musb_interrupt(musb);
-
-	spin_unlock_irqrestore(&musb->lock, flags);
-
-	return retval;
-}
-
-#else
-#define generic_interrupt	NULL
-#endif
-
 /*
  * handle all the irqs defined by the HDRC core. for now we expect:  other
  * irq sources (phy, dma, etc) will be handled first, musb->int_* values
@@ -1907,7 +1878,8 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	musb->ops = plat->platform_ops;
 
 	/* The musb_platform_init() call:
-	 *   - adjusts musb->mregs and musb->isr if needed,
+	 *   - adjusts musb->mregs if needed 
+	 *   - sets the musb->isr 
 	 *   - may initialize an integrated tranceiver
 	 *   - initializes musb->xceiv, usually by otg_get_phy()
 	 *   - stops powering VBUS
@@ -1917,7 +1889,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	 * external/discrete ones in various flavors (twl4030 family,
 	 * isp1504, non-OTG, etc) mostly hooking up through ULPI.
 	 */
-	musb->isr = generic_interrupt;
 	status = musb_platform_init(musb);
 	if (status < 0)
 		goto fail1;
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 5fdb9da..5461619d 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -306,6 +306,26 @@ static void omap_musb_mailbox_work(struct work_struct *mailbox_work)
 	omap_musb_set_mailbox(glue);
 }
 
+static irqreturn_t omap2430_musb_interrupt(int irq, void *__hci)
+{
+        unsigned long   flags;
+        irqreturn_t     retval = IRQ_NONE;
+        struct musb     *musb = __hci;
+
+        spin_lock_irqsave(&musb->lock, flags);
+
+        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
+        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
+        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
+
+        if (musb->int_usb || musb->int_tx || musb->int_rx)
+                retval = musb_interrupt(musb);
+
+        spin_unlock_irqrestore(&musb->lock, flags);
+
+        return retval;
+}
+
 static int omap2430_musb_init(struct musb *musb)
 {
 	u32 l;
@@ -325,6 +345,8 @@ static int omap2430_musb_init(struct musb *musb)
 		return -ENODEV;
 	}
 
+	musb->isr = omap2430_musb_interrupt;
+
 	status = pm_runtime_get_sync(dev);
 	if (status < 0) {
 		dev_err(dev, "pm_runtime_get_sync FAILED %d\n", status);
diff --git a/drivers/usb/musb/ux500.c b/drivers/usb/musb/ux500.c
index a8c0fad..ec9aaec 100644
--- a/drivers/usb/musb/ux500.c
+++ b/drivers/usb/musb/ux500.c
@@ -36,6 +36,26 @@ struct ux500_glue {
 };
 #define glue_to_musb(g)	platform_get_drvdata(g->musb)
 
+static irqreturn_t ux500_musb_interrupt(int irq, void *__hci)
+{
+        unsigned long   flags;
+        irqreturn_t     retval = IRQ_NONE;
+        struct musb     *musb = __hci;
+
+        spin_lock_irqsave(&musb->lock, flags);
+
+        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
+        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
+        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
+
+        if (musb->int_usb || musb->int_tx || musb->int_rx)
+                retval = musb_interrupt(musb);
+
+        spin_unlock_irqrestore(&musb->lock, flags);
+
+        return retval;
+}
+
 static int ux500_musb_init(struct musb *musb)
 {
 	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
@@ -43,6 +63,7 @@ static int ux500_musb_init(struct musb *musb)
 		pr_err("HS USB OTG: no transceiver configured\n");
 		return -ENODEV;
 	}
+	musb->isr = ux500_musb_interrupt;
 
 	return 0;
 }
-- 
1.7.10.4


Cheers,

Philippe--
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: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Philippe De Swert]

Hi all,

On 26/09/2012, Philippe De Swert <philippe.deswert@jollamobile.com> wrote:
> Hi,
> ________________________________________
>>On Tue, Sep 25, 2012 at 2:39 PM, Philippe De Swert
>>>> Then maybe it's best to just remove the ifdefs and always provide
>>>> generic_interrupt() ?
>>>>
>>>> Anyone against it ?
>>
>>Providing generic_interrupt seems fine.
>>
>>> Well it seems there are only two drivers that use it omap2430 and
>>> ux500. Maybe we somehow link it to the drivers that need it? (I might
>>> have missed other drivers but it looks like it is just those two)
>>
>>One way I see is that omap2430 and ux500 implement the isr handler
>>in platform glue driver and then call the musb_interrupt() as done in
>>daxxx, amxxx, ti8xxx platforms. Then generic_interrupt can be removed.
> 
> So something along those lines? (If this is the right way I will resend as a
> real patch)

So any comments on the approach here (see patch kept below)? Or should I immediately send it as a new patch to get the comments? I sent it in this thread as it also solves the issue I have. 

BTW: CONFIG_SOC_OMAP3430 could be easily removed as it only changes minor things in the musb stack. It would clean up the code and get rid of this very misleading option as it has nothing to do with any OMAP3430 soc specific handling.

Regards,

Philippe

 
> From deae78e1084749f340ae8b8aaeca51818d5bfc55 Mon Sep 17 00:00:00 2001
> From: Philippe De Swert <philippe.deswert@jollamobile.com>
> Date: Wed, 26 Sep 2012 17:00:46 +0300
> Subject: [PATCH 1/1] musb: Move generic_interrupt out of the way
> 
> Have all musb drivers define their own isr.
> 
> Signed-off-by: Philippe De Swert <philippe.deswert@jollamobile.com>
> ---
>  drivers/usb/musb/musb_core.c |   33 ++-------------------------------
>  drivers/usb/musb/omap2430.c  |   22 ++++++++++++++++++++++
>  drivers/usb/musb/ux500.c     |   21 +++++++++++++++++++++
>  3 files changed, 45 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 26f1bef..1d5ee34 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1496,35 +1496,6 @@ static int __devinit musb_core_init(u16 musb_type,
> struct musb *musb)
>  	return 0;
>  }
>  
> -/*-------------------------------------------------------------------------*/
> -
> -#if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
> -	defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
> -
> -static irqreturn_t generic_interrupt(int irq, void *__hci)
> -{
> -	unsigned long	flags;
> -	irqreturn_t	retval = IRQ_NONE;
> -	struct musb	*musb = __hci;
> -
> -	spin_lock_irqsave(&musb->lock, flags);
> -
> -	musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
> -	musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
> -	musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
> -
> -	if (musb->int_usb || musb->int_tx || musb->int_rx)
> -		retval = musb_interrupt(musb);
> -
> -	spin_unlock_irqrestore(&musb->lock, flags);
> -
> -	return retval;
> -}
> -
> -#else
> -#define generic_interrupt	NULL
> -#endif
> -
>  /*
>   * handle all the irqs defined by the HDRC core. for now we expect:  other
>   * irq sources (phy, dma, etc) will be handled first, musb->int_* values
> @@ -1907,7 +1878,8 @@ musb_init_controller(struct device *dev, int nIrq,
> void __iomem *ctrl)
>  	musb->ops = plat->platform_ops;
>  
>  	/* The musb_platform_init() call:
> -	 *   - adjusts musb->mregs and musb->isr if needed,
> +	 *   - adjusts musb->mregs if needed 
> +	 *   - sets the musb->isr 
>  	 *   - may initialize an integrated tranceiver
>  	 *   - initializes musb->xceiv, usually by otg_get_phy()
>  	 *   - stops powering VBUS
> @@ -1917,7 +1889,6 @@ musb_init_controller(struct device *dev, int nIrq,
> void __iomem *ctrl)
>  	 * external/discrete ones in various flavors (twl4030 family,
>  	 * isp1504, non-OTG, etc) mostly hooking up through ULPI.
>  	 */
> -	musb->isr = generic_interrupt;
>  	status = musb_platform_init(musb);
>  	if (status < 0)
>  		goto fail1;
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 5fdb9da..5461619d 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -306,6 +306,26 @@ static void omap_musb_mailbox_work(struct work_struct
> *mailbox_work)
>  	omap_musb_set_mailbox(glue);
>  }
>  
> +static irqreturn_t omap2430_musb_interrupt(int irq, void *__hci)
> +{
> +        unsigned long   flags;
> +        irqreturn_t     retval = IRQ_NONE;
> +        struct musb     *musb = __hci;
> +
> +        spin_lock_irqsave(&musb->lock, flags);
> +
> +        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
> +        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
> +        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
> +
> +        if (musb->int_usb || musb->int_tx || musb->int_rx)
> +                retval = musb_interrupt(musb);
> +
> +        spin_unlock_irqrestore(&musb->lock, flags);
> +
> +        return retval;
> +}
> +
>  static int omap2430_musb_init(struct musb *musb)
>  {
>  	u32 l;
> @@ -325,6 +345,8 @@ static int omap2430_musb_init(struct musb *musb)
>  		return -ENODEV;
>  	}
>  
> +	musb->isr = omap2430_musb_interrupt;
> +
>  	status = pm_runtime_get_sync(dev);
>  	if (status < 0) {
>  		dev_err(dev, "pm_runtime_get_sync FAILED %d\n", status);
> diff --git a/drivers/usb/musb/ux500.c b/drivers/usb/musb/ux500.c
> index a8c0fad..ec9aaec 100644
> --- a/drivers/usb/musb/ux500.c
> +++ b/drivers/usb/musb/ux500.c
> @@ -36,6 +36,26 @@ struct ux500_glue {
>  };
>  #define glue_to_musb(g)	platform_get_drvdata(g->musb)
>  
> +static irqreturn_t ux500_musb_interrupt(int irq, void *__hci)
> +{
> +        unsigned long   flags;
> +        irqreturn_t     retval = IRQ_NONE;
> +        struct musb     *musb = __hci;
> +
> +        spin_lock_irqsave(&musb->lock, flags);
> +
> +        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
> +        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
> +        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
> +
> +        if (musb->int_usb || musb->int_tx || musb->int_rx)
> +                retval = musb_interrupt(musb);
> +
> +        spin_unlock_irqrestore(&musb->lock, flags);
> +
> +        return retval;
> +}
> +
>  static int ux500_musb_init(struct musb *musb)
>  {
>  	musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
> @@ -43,6 +63,7 @@ static int ux500_musb_init(struct musb *musb)
>  		pr_err("HS USB OTG: no transceiver configured\n");
>  		return -ENODEV;
>  	}
> +	musb->isr = ux500_musb_interrupt;
>  
>  	return 0;
>  }
> -- 
> 1.7.10.4

Re: [PATCH 1/1] usb: Include generic_interrupt for OMAP2_PLUS

[Praveena Nadahally]

On Wed, Oct 3, 2012 at 7:15 PM, Philippe De Swert
<philippe.deswert@jollamobile.com> wrote:

> So any comments on the approach here (see patch kept below)? Or should I immediately send it as a new patch to get the comments? I sent it in this thread as it also solves the issue I have.

Patch is fine for me. Not sure if Felipe has some comments.

> BTW: CONFIG_SOC_OMAP3430 could be easily removed as it only changes minor things in the musb stack. It would clean up the code and get rid of this very misleading option as it has nothing to do with any OMAP3430 soc specific handling.

It would be better if some OMAP3430 users can comment on this.


>> From deae78e1084749f340ae8b8aaeca51818d5bfc55 Mon Sep 17 00:00:00 2001
>> From: Philippe De Swert <philippe.deswert@jollamobile.com>
>> Date: Wed, 26 Sep 2012 17:00:46 +0300
>> Subject: [PATCH 1/1] musb: Move generic_interrupt out of the way
>>
>> Have all musb drivers define their own isr.
>>
>> Signed-off-by: Philippe De Swert <philippe.deswert@jollamobile.com>
>> ---
>>  drivers/usb/musb/musb_core.c |   33 ++-------------------------------
>>  drivers/usb/musb/omap2430.c  |   22 ++++++++++++++++++++++
>>  drivers/usb/musb/ux500.c     |   21 +++++++++++++++++++++
>>  3 files changed, 45 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 26f1bef..1d5ee34 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -1496,35 +1496,6 @@ static int __devinit musb_core_init(u16 musb_type,
>> struct musb *musb)
>>       return 0;
>>  }
>>
>> -/*-------------------------------------------------------------------------*/
>> -
>> -#if defined(CONFIG_SOC_OMAP2430) || defined(CONFIG_SOC_OMAP3430) || \
>> -     defined(CONFIG_ARCH_OMAP4) || defined(CONFIG_ARCH_U8500)
>> -
>> -static irqreturn_t generic_interrupt(int irq, void *__hci)
>> -{
>> -     unsigned long   flags;
>> -     irqreturn_t     retval = IRQ_NONE;
>> -     struct musb     *musb = __hci;
>> -
>> -     spin_lock_irqsave(&musb->lock, flags);
>> -
>> -     musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>> -     musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
>> -     musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
>> -
>> -     if (musb->int_usb || musb->int_tx || musb->int_rx)
>> -             retval = musb_interrupt(musb);
>> -
>> -     spin_unlock_irqrestore(&musb->lock, flags);
>> -
>> -     return retval;
>> -}
>> -
>> -#else
>> -#define generic_interrupt    NULL
>> -#endif
>> -
>>  /*
>>   * handle all the irqs defined by the HDRC core. for now we expect:  other
>>   * irq sources (phy, dma, etc) will be handled first, musb->int_* values
>> @@ -1907,7 +1878,8 @@ musb_init_controller(struct device *dev, int nIrq,
>> void __iomem *ctrl)
>>       musb->ops = plat->platform_ops;
>>
>>       /* The musb_platform_init() call:
>> -      *   - adjusts musb->mregs and musb->isr if needed,
>> +      *   - adjusts musb->mregs if needed
>> +      *   - sets the musb->isr
>>        *   - may initialize an integrated tranceiver
>>        *   - initializes musb->xceiv, usually by otg_get_phy()
>>        *   - stops powering VBUS
>> @@ -1917,7 +1889,6 @@ musb_init_controller(struct device *dev, int nIrq,
>> void __iomem *ctrl)
>>        * external/discrete ones in various flavors (twl4030 family,
>>        * isp1504, non-OTG, etc) mostly hooking up through ULPI.
>>        */
>> -     musb->isr = generic_interrupt;
>>       status = musb_platform_init(musb);
>>       if (status < 0)
>>               goto fail1;
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index 5fdb9da..5461619d 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -306,6 +306,26 @@ static void omap_musb_mailbox_work(struct work_struct
>> *mailbox_work)
>>       omap_musb_set_mailbox(glue);
>>  }
>>
>> +static irqreturn_t omap2430_musb_interrupt(int irq, void *__hci)
>> +{
>> +        unsigned long   flags;
>> +        irqreturn_t     retval = IRQ_NONE;
>> +        struct musb     *musb = __hci;
>> +
>> +        spin_lock_irqsave(&musb->lock, flags);
>> +
>> +        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>> +        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
>> +        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
>> +
>> +        if (musb->int_usb || musb->int_tx || musb->int_rx)
>> +                retval = musb_interrupt(musb);
>> +
>> +        spin_unlock_irqrestore(&musb->lock, flags);
>> +
>> +        return retval;
>> +}
>> +
>>  static int omap2430_musb_init(struct musb *musb)
>>  {
>>       u32 l;
>> @@ -325,6 +345,8 @@ static int omap2430_musb_init(struct musb *musb)
>>               return -ENODEV;
>>       }
>>
>> +     musb->isr = omap2430_musb_interrupt;
>> +
>>       status = pm_runtime_get_sync(dev);
>>       if (status < 0) {
>>               dev_err(dev, "pm_runtime_get_sync FAILED %d\n", status);
>> diff --git a/drivers/usb/musb/ux500.c b/drivers/usb/musb/ux500.c
>> index a8c0fad..ec9aaec 100644
>> --- a/drivers/usb/musb/ux500.c
>> +++ b/drivers/usb/musb/ux500.c
>> @@ -36,6 +36,26 @@ struct ux500_glue {
>>  };
>>  #define glue_to_musb(g)      platform_get_drvdata(g->musb)
>>
>> +static irqreturn_t ux500_musb_interrupt(int irq, void *__hci)
>> +{
>> +        unsigned long   flags;
>> +        irqreturn_t     retval = IRQ_NONE;
>> +        struct musb     *musb = __hci;
>> +
>> +        spin_lock_irqsave(&musb->lock, flags);
>> +
>> +        musb->int_usb = musb_readb(musb->mregs, MUSB_INTRUSB);
>> +        musb->int_tx = musb_readw(musb->mregs, MUSB_INTRTX);
>> +        musb->int_rx = musb_readw(musb->mregs, MUSB_INTRRX);
>> +
>> +        if (musb->int_usb || musb->int_tx || musb->int_rx)
>> +                retval = musb_interrupt(musb);
>> +
>> +        spin_unlock_irqrestore(&musb->lock, flags);
>> +
>> +        return retval;
>> +}
>> +
>>  static int ux500_musb_init(struct musb *musb)
>>  {
>>       musb->xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
>> @@ -43,6 +63,7 @@ static int ux500_musb_init(struct musb *musb)
>>               pr_err("HS USB OTG: no transceiver configured\n");
>>               return -ENODEV;
>>       }
>> +     musb->isr = ux500_musb_interrupt;
>>
>>       return 0;
>>  }
>> --
>> 1.7.10.4