[PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Manikandan Pillai]

MUSB on OMAP3EVM uses ISP1504 phy and doesn't need twl4030.
OMAP35xx Beagle board MUSB uses twl4030 phy thus uses it and
it also sets xceiv global field using otg_set_transceiver().
As OMAP3EVM MUSB doesn't require twl4030 so otg_set_transceiver()
part is being done in this patch.

This is a temporary patch and the updated patch will come soon.

Signed-off-by: Manikandan Pillai <mani.pillai@ti.com>
---
 drivers/usb/musb/omap2430.c |   53 +++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 51 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index 03fc864..9aa4518 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -215,13 +215,60 @@ void musb_platform_set_mode(struct musb *musb, u8 musb_mode)
 	}
 }
 
+#ifdef CONFIG_MACH_OMAP3EVM
+static int omap3_evm_otg_set_host(struct otg_transceiver *xceiv,
+			 struct usb_bus *host)
+{
+	if (!xceiv)
+		return -ENODEV;
+
+	if (!host) {
+		xceiv->host = NULL;
+		return -ENODEV;
+	}
+	DBG(2, "xceiv in host\n");
+	xceiv->host = host;
+	return 0;
+}
+static int omap3_evm_otg_set_peripheral(struct otg_transceiver *xceiv,
+		struct usb_gadget *gadget)
+{
+	if (!xceiv)
+		return -ENODEV;
+
+	if (!gadget) {
+		xceiv->gadget = NULL;
+		return -ENODEV;
+	}
+	DBG(2, "xceiv in peripheral\n");
+	xceiv->gadget = gadget;
+	xceiv->state = OTG_STATE_B_IDLE;
+	return 0;
+
+}
+static int omap3_evm_otg_set_suspend(struct otg_transceiver *x, int suspend)
+{
+	DBG(2, "xceiv suspend\n");
+	return 0;
+}
+#endif
+
 int __init musb_platform_init(struct musb *musb)
 {
-	struct otg_transceiver *x = otg_get_transceiver();
+	struct otg_transceiver *x;
 	u32 l;
 
 #if defined(CONFIG_ARCH_OMAP2430)
 	omap_cfg_reg(AE5_2430_USB0HS_STP);
+	x = otg_get_transceiver();
+#elif defined(CONFIG_MACH_OMAP3EVM)
+	x = kzalloc(sizeof *x, GFP_KERNEL);
+	if (!x)
+		return 0;
+	x->set_host = omap3_evm_otg_set_host;
+	x->set_peripheral = omap3_evm_otg_set_peripheral;
+	x->set_suspend = omap3_evm_otg_set_suspend;
+	otg_set_transceiver(x);
 #endif
 
 	musb->xceiv = *x;
@@ -323,6 +370,8 @@ int musb_platform_exit(struct musb *musb)
 
 	clk_put(musb->clock);
 	musb->clock = 0;
-
+#if defined(CONFIG_MACH_OMAP3EVM)
+	kfree(&musb->xceiv);
+#endif
 	return 0;
 }
-- 
1.5.6

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Felipe Balbi]

On Fri, Nov 28, 2008 at 10:58:24AM +0530, ext Manikandan Pillai wrote:
> MUSB on OMAP3EVM uses ISP1504 phy and doesn't need twl4030.
> OMAP35xx Beagle board MUSB uses twl4030 phy thus uses it and
> it also sets xceiv global field using otg_set_transceiver().
> As OMAP3EVM MUSB doesn't require twl4030 so otg_set_transceiver()
> part is being done in this patch.
> 
> This is a temporary patch and the updated patch will come soon.

musb patches should go to linux-usb mailing list.

> Signed-off-by: Manikandan Pillai <mani.pillai@ti.com>
> ---
>  drivers/usb/musb/omap2430.c |   53 +++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 51 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
> index 03fc864..9aa4518 100644
> --- a/drivers/usb/musb/omap2430.c
> +++ b/drivers/usb/musb/omap2430.c
> @@ -215,13 +215,60 @@ void musb_platform_set_mode(struct musb *musb, u8 musb_mode)
>  	}
>  }
>  
> +#ifdef CONFIG_MACH_OMAP3EVM
> +static int omap3_evm_otg_set_host(struct otg_transceiver *xceiv,
> +			 struct usb_bus *host)
> +{
> +	if (!xceiv)
> +		return -ENODEV;
> +
> +	if (!host) {
> +		xceiv->host = NULL;
> +		return -ENODEV;
> +	}
> +	DBG(2, "xceiv in host\n");
> +	xceiv->host = host;
> +	return 0;
> +}
> +static int omap3_evm_otg_set_peripheral(struct otg_transceiver *xceiv,
> +		struct usb_gadget *gadget)
> +{
> +	if (!xceiv)
> +		return -ENODEV;
> +
> +	if (!gadget) {
> +		xceiv->gadget = NULL;
> +		return -ENODEV;
> +	}
> +	DBG(2, "xceiv in peripheral\n");
> +	xceiv->gadget = gadget;
> +	xceiv->state = OTG_STATE_B_IDLE;
> +	return 0;
> +
> +}
> +static int omap3_evm_otg_set_suspend(struct otg_transceiver *x, int suspend)
> +{
> +	DBG(2, "xceiv suspend\n");
> +	return 0;
> +}
> +#endif

sorry, no. I won't add ifdefs to any of those files since it breaks
multiomap support.

> +
>  int __init musb_platform_init(struct musb *musb)
>  {
> -	struct otg_transceiver *x = otg_get_transceiver();
> +	struct otg_transceiver *x;
>  	u32 l;
>  
>  #if defined(CONFIG_ARCH_OMAP2430)

what about 3430 ?

>  	omap_cfg_reg(AE5_2430_USB0HS_STP);
> +	x = otg_get_transceiver();
> +#elif defined(CONFIG_MACH_OMAP3EVM)
> +	x = kzalloc(sizeof *x, GFP_KERNEL);
> +	if (!x)
> +		return 0;
> +	x->set_host = omap3_evm_otg_set_host;
> +	x->set_peripheral = omap3_evm_otg_set_peripheral;
> +	x->set_suspend = omap3_evm_otg_set_suspend;
> +	otg_set_transceiver(x);
>  #endif
>  
>  	musb->xceiv = *x;
> @@ -323,6 +370,8 @@ int musb_platform_exit(struct musb *musb)
>  
>  	clk_put(musb->clock);
>  	musb->clock = 0;
> -
> +#if defined(CONFIG_MACH_OMAP3EVM)
> +	kfree(&musb->xceiv);
> +#endif

no.

I could even live with if (machine_is_xxx()) code, although I don't want
to. But ifdefs are an automatic NAK.

-- 
balbi

RE: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Gupta, Ajay Kumar]

> no.
> 
> I could even live with if (machine_is_xxx()) code, although I don't want
> to. But ifdefs are an automatic NAK.

Felipe, this is temporary patch as described. I will submit the final one
later. twl4030 dependency is bottleneck for TPS work on OMAP3EVM.  

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

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[David Brownell]

On Thursday 27 November 2008, Manikandan Pillai wrote:
>  #if defined(CONFIG_ARCH_OMAP2430)
>         omap_cfg_reg(AE5_2430_USB0HS_STP);
> +       x = otg_get_transceiver();
> +#elif defined(CONFIG_MACH_OMAP3EVM)
> +       x = kzalloc(sizeof *x, GFP_KERNEL);
> +       if (!x)
> +               return 0;
> +       x->set_host = omap3_evm_otg_set_host;
> +       x->set_peripheral = omap3_evm_otg_set_peripheral;
> +       x->set_suspend = omap3_evm_otg_set_suspend;
> +       otg_set_transceiver(x);
>  #endif

This is obviously wrong.  One does set_transceiver(),
the other does get_transceiver() ... 

It seems that some boards need some kind of basic
OTG transceiver stub.  The newish drivers/usb/otg
directory is the place to keep such stuff.

- Dave

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

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Felipe Balbi]

On Fri, Nov 28, 2008 at 12:21:05PM +0530, Ajay Kumar Gupta wrote:
> > no.
> > 
> > I could even live with if (machine_is_xxx()) code, although I don't want
> > to. But ifdefs are an automatic NAK.
> 
> Felipe, this is temporary patch as described. I will submit the final one
> later. twl4030 dependency is bottleneck for TPS work on OMAP3EVM.  

Good, but still, let's not break multiomap ;-)

-- 
balbi

RE: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Gupta, Ajay Kumar]

> -----Original Message-----
> From: linux-omap-owner@vger.kernel.org [mailto:linux-omap-owner@vger.kernel.org] On Behalf Of David
> Brownell
> Sent: Friday, November 28, 2008 12:33 PM
> To: Pillai, Manikandan
> Cc: linux-omap@vger.kernel.org
> Subject: Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB
> 
> On Thursday 27 November 2008, Manikandan Pillai wrote:
> >  #if defined(CONFIG_ARCH_OMAP2430)
> >         omap_cfg_reg(AE5_2430_USB0HS_STP);
> > +       x = otg_get_transceiver();
> > +#elif defined(CONFIG_MACH_OMAP3EVM)
> > +       x = kzalloc(sizeof *x, GFP_KERNEL);
> > +       if (!x)
> > +               return 0;
> > +       x->set_host = omap3_evm_otg_set_host;
> > +       x->set_peripheral = omap3_evm_otg_set_peripheral;
> > +       x->set_suspend = omap3_evm_otg_set_suspend;
> > +       otg_set_transceiver(x);
> >  #endif
> 
> This is obviously wrong.  One does set_transceiver(),
> the other does get_transceiver() ...

For OMAP3EVM we don't need twl4030 support for musb and thus otg_set_transceiver() which was done
in twl4030-usb.c, is now done here itself for OMAP3EVM.
Whereas for SDP and Beagle twol4030-usb would be enabled and thus otg_set_transceiver() would
have been done in twl4030-usb.c file itself.

> 
> It seems that some boards need some kind of basic
> OTG transceiver stub.  The newish drivers/usb/otg
> directory is the place to keep such stuff.
> 
> - Dave
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[David Brownell]

On Thursday 27 November 2008, Gupta, Ajay Kumar wrote:

> > This is obviously wrong.  One does set_transceiver(),
> > the other does get_transceiver() ...
> 
> For OMAP3EVM we don't need twl4030 support for musb and thus
> otg_set_transceiver() which was done 
> in twl4030-usb.c, is now done here itself for OMAP3EVM.

Which is obviously wrong.  Some other code should have
called otg_set_transceiver() ... *NEVER* the musb driver,
except with silicon that integrates an OTG transceiver
(like DaVinci and Blackfin).

Yes, the MUSB driver hasn't fully sorted this out yet;
it started with DaVinci EVMs, and only later started
to support discrete transceivers.  Patches are in the
works to fix that, I'm not sure of their status.

That's an example of why you must not discard your basic
intuition when updating drivers ... like noticing that
since two branches of an #if do entirely different things,
at least one branch must accordingly be wrong.


> Whereas for SDP and Beagle twol4030-usb would be enabled and
> thus otg_set_transceiver() would 
> have been done in twl4030-usb.c file itself.
> 
> > 
> > It seems that some boards need some kind of basic
> > OTG transceiver stub.  The newish drivers/usb/otg
> > directory is the place to keep such stuff.

So the answer for you is obviously to have something
other than the musb driver hold your otg_transceiver
driver.  Like ... an isp1504.c I2C driver, which will
eventually move to drivers/usb/otg and which calls
the otg_set_transceiver() utility.

Board-specific logic would presumably involve just
declaring the i2c board info for the isp1504 chip.

- Dave

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Felipe Balbi]

On Fri, Nov 28, 2008 at 09:25:26AM -0800, David Brownell wrote:
> So the answer for you is obviously to have something
> other than the musb driver hold your otg_transceiver
> driver.  Like ... an isp1504.c I2C driver, which will
> eventually move to drivers/usb/otg and which calls
> the otg_set_transceiver() utility.

There's one problem here. If this is similar to isp1704, it doesn't have
a control interface. And the only way to talk to the transceiver is via
ULPI. I have patches that add musb_ulpi_read/write functions using the
musb ulpi wrapper. BUT, isp1x04 is pretty much autonomous, there isn't
much you can/have to do, the problem with them is that they don't have
an interrupt line with omap (afaict) so it's difficult to be sure when
we have to e.g. switch to host mode unless we keep polling the INT_STS
register for changes. That's too bad.

But other than that, I agree with you completely. I won't queue up
patches adding such kind of ifdefs.

-- 
balbi

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[David Brownell]

On Friday 28 November 2008, Felipe Balbi wrote:
> On Fri, Nov 28, 2008 at 09:25:26AM -0800, David Brownell wrote:
> > So the answer for you is obviously to have something
> > other than the musb driver hold your otg_transceiver
> > driver.  Like ... an isp1504.c I2C driver, which will
> > eventually move to drivers/usb/otg and which calls
> > the otg_set_transceiver() utility.
> 
> There's one problem here. If this is similar to isp1704, it doesn't have
> a control interface. 

It does have an I2C interface; I looked before making
that particular comment ...

Whether it's hooked up on that board is a different
issue.  I didn't look at how for example it arranges
to supply VBUS power when running in A-Default mode.

- Dave

Re: [PATCH 3/3] musb: Remvoing twl4030 dependency for OMAP3EVM MUSB

[Felipe Balbi]

On Fri, Nov 28, 2008 at 10:37:47AM -0800, David Brownell wrote:
> On Friday 28 November 2008, Felipe Balbi wrote:
> > On Fri, Nov 28, 2008 at 09:25:26AM -0800, David Brownell wrote:
> > > So the answer for you is obviously to have something
> > > other than the musb driver hold your otg_transceiver
> > > driver.  Like ... an isp1504.c I2C driver, which will
> > > eventually move to drivers/usb/otg and which calls
> > > the otg_set_transceiver() utility.
> > 
> > There's one problem here. If this is similar to isp1704, it doesn't have
> > a control interface. 
> 
> It does have an I2C interface; I looked before making
> that particular comment ...

so it's better than isp1704. It's horrible only been able to talk to the
device via ulpi.

-- 
balbi