[PATCH] Allow MUSB DSPS to use "force host" mode

[PATCH] Allow MUSB DSPS to use "force host" mode

[Mark Jackson]

The IDDIG input pin is normally used to determine the USB mode
(i.e. HOST or DEVICE).

On some systems (e.g. AM335x) leaving this pin floating allows
the USB mode to be set via software.

This patch adds support for this via the device tree.

Signed-off-by: Mark Jackson <mpfj@newflow.co.uk>
---
 .../devicetree/bindings/usb/am33xx-usb.txt         |    2 ++
 drivers/usb/musb/musb_dsps.c                       |   14 ++++++++++++++
 include/linux/usb/musb.h                           |    1 +
 3 files changed, 17 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/am33xx-usb.txt b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
index 20c2ff2..560b7ff 100644
--- a/Documentation/devicetree/bindings/usb/am33xx-usb.txt
+++ b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
@@ -47,6 +47,8 @@ USB
 - dmas: specifies the dma channels
 - dma-names: specifies the names of the channels. Use "rxN" for receive
   and "txN" for transmit endpoints. N specifies the endpoint number.
+- ti,force-host: specifies that the IDDIG input be ignored and the device be
+  put into host mode regardless.
 
 The controller should have an "usb" alias numbered properly in the alias
 node.
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 1901f6f..6439809 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -105,6 +105,7 @@ struct dsps_musb_wrapper {
 	unsigned	otg_disable:5;
 
 	/* bit positions for mode */
+	unsigned	iddig_mux:5;
 	unsigned	iddig:5;
 	/* miscellaneous stuff */
 	u8		poll_seconds;
@@ -387,6 +388,15 @@ static int dsps_musb_init(struct musb *musb)
 
 	musb->isr = dsps_interrupt;
 
+	/* Force host mode, rather than relying on IDDIG input */
+	if (musb->config->force_host) {
+		val = dsps_readl(reg_base, wrp->mode);
+		/* clear IDDIG bit, set IDDIG_MUX bit */
+		val &= ~(1 << wrp->iddig);
+		val |= (1 << wrp->iddig_mux);
+		dsps_writel(musb->ctrl_base, wrp->mode, val);
+	}
+
 	/* reset the otgdisable bit, needed for host mode to work */
 	val = dsps_readl(reg_base, wrp->phy_utmi);
 	val &= ~(1 << wrp->otg_disable);
@@ -512,6 +522,9 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
 	pdata.power = get_int_prop(dn, "mentor,power") / 2;
 	config->multipoint = of_property_read_bool(dn, "mentor,multipoint");
 
+	if (of_property_read_bool(dn, "ti,force-host"))
+		config->force_host = true;
+
 	ret = platform_device_add_data(musb, &pdata, sizeof(pdata));
 	if (ret) {
 		dev_err(dev, "failed to add platform_data\n");
@@ -607,6 +620,7 @@ static const struct dsps_musb_wrapper am33xx_driver_data = {
 	.mode			= 0xe8,
 	.reset			= 0,
 	.otg_disable		= 21,
+	.iddig_mux		= 7,
 	.iddig			= 8,
 	.usb_shift		= 0,
 	.usb_mask		= 0x1ff,
diff --git a/include/linux/usb/musb.h b/include/linux/usb/musb.h
index eb50525..a0a81c5 100644
--- a/include/linux/usb/musb.h
+++ b/include/linux/usb/musb.h
@@ -75,6 +75,7 @@ struct musb_hdrc_config {
 	unsigned	high_iso_rx:1;	/* Rx ep required for HD iso */
 	unsigned	dma:1 __deprecated; /* supports DMA */
 	unsigned	vendor_req:1 __deprecated; /* vendor registers required */
+	unsigned	force_host:1;	/* force host mode */
 
 	u8		num_eps;	/* number of endpoints _with_ ep0 */
 	u8		dma_channels __deprecated; /* number of dma channels */
-- 
1.7.9.5

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Sebastian Andrzej Siewior]

On 11/22/2013 04:55 PM, Mark Jackson wrote:
> The IDDIG input pin is normally used to determine the USB mode
> (i.e. HOST or DEVICE).
> 
> On some systems (e.g. AM335x) leaving this pin floating allows
> the USB mode to be set via software.

So you have a board where musb is used only as host or only as device
and the ID pin not on ground or 3.3V?
What are the side effects? I remember correctly Bin wanted to avoid
settings this if it could be avoided.

Sebastian

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Mark Jackson]

On 22/11/13 16:33, Sebastian Andrzej Siewior wrote:
> On 11/22/2013 04:55 PM, Mark Jackson wrote:
>> The IDDIG input pin is normally used to determine the USB mode
>> (i.e. HOST or DEVICE).
>>
>> On some systems (e.g. AM335x) leaving this pin floating allows
>> the USB mode to be set via software.
> 
> So you have a board where musb is used only as host or only as device
> and the ID pin not on ground or 3.3V?
> What are the side effects? I remember correctly Bin wanted to avoid
> settings this if it could be avoided.

Yes ... we have a host only USB port and an unconnected ID pin.

AFAIK it defaults to device mode so I can't see any devices that get
plugged into the USB port.

If I tweak the s/w to "force" host mode on, then everything appears to
work okay.

I guess it's more of a hardware oversight that we left the pin floating
but in the real world, I guess someone may want this feature to they
can change the usb port type ?

Either way, I need to fix the current h/w (which can be done via s/w)
hence the patch.

Mark J.

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Sebastian Andrzej Siewior]

On 11/22/2013 05:49 PM, Mark Jackson wrote:
>> and the ID pin not on ground or 3.3V?
>> What are the side effects? I remember correctly Bin wanted to avoid
>> settings this if it could be avoided.
> 
> Yes ... we have a host only USB port and an unconnected ID pin.

Is it too late to connect that pin?

> AFAIK it defaults to device mode so I can't see any devices that get
> plugged into the USB port.
> 
> If I tweak the s/w to "force" host mode on, then everything appears to
> work okay.
> 
> I guess it's more of a hardware oversight that we left the pin floating
> but in the real world, I guess someone may want this feature to they
> can change the usb port type ?

This is something I would prefer to avoid if possible. We have the
dr_mode attribute in DT. Based on that one we act as a device or host.
Now if you decide (via dr_mode) either for host or device that means we
have to set that bit. Where I feel a little uncomfortable is when
someone having OTG runs in hostmode and attaches a host.

> 
> Either way, I need to fix the current h/w (which can be done via s/w)
> hence the patch.
I've seen many projects where this pin has been forgotten and it could
not be changed in SW and they patched the HW. Usually this is noticed
in the early phase and a wire is just soldered and the redesign has it
fixed. So I don't think that this is a big issue.
Do you insists on having this change merged upstream?

> 
> Mark J.
> 

Sebastian

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Mark Jackson]

On 22/11/13 17:01, Sebastian Andrzej Siewior wrote:
> On 11/22/2013 05:49 PM, Mark Jackson wrote:
>>> and the ID pin not on ground or 3.3V?
>>> What are the side effects? I remember correctly Bin wanted to avoid
>>> settings this if it could be avoided.
>>
>> Yes ... we have a host only USB port and an unconnected ID pin.
> 
> Is it too late to connect that pin?

Unfortunately, yes.

The USB portion of the CPU board has not been needed until now, and
although I did know about the unconnected pin, I also knew it could be
fixed in s/w, so I wasn't too concerned.

>> AFAIK it defaults to device mode so I can't see any devices that get
>> plugged into the USB port.
>>
>> If I tweak the s/w to "force" host mode on, then everything appears to
>> work okay.
>>
>> I guess it's more of a hardware oversight that we left the pin floating
>> but in the real world, I guess someone may want this feature to they
>> can change the usb port type ?
> 
> This is something I would prefer to avoid if possible. We have the
> dr_mode attribute in DT. Based on that one we act as a device or host.
> Now if you decide (via dr_mode) either for host or device that means we
> have to set that bit. Where I feel a little uncomfortable is when
> someone having OTG runs in hostmode and attaches a host.
> 
>>
>> Either way, I need to fix the current h/w (which can be done via s/w)
>> hence the patch.
> I've seen many projects where this pin has been forgotten and it could
> not be changed in SW and they patched the HW. Usually this is noticed
> in the early phase and a wire is just soldered and the redesign has it
> fixed. So I don't think that this is a big issue.
> Do you insists on having this change merged upstream?

No ... I didn't think it would be such an issue, so I'm happy to keep this
as a local patch if that's any better.

Mark J.

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Sebastian Andrzej Siewior]

On 11/22/2013 06:07 PM, Mark Jackson wrote:
>> Do you insists on having this change merged upstream?
> 
> No ... I didn't think it would be such an issue, so I'm happy to keep this
> as a local patch if that's any better.

Felipe, Bin: Second opinion on that?

> 
> Mark J.

Sebastian

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Michael Grzeschik]

Hallo,

On Fri, Nov 22, 2013 at 03:55:59PM +0000, Mark Jackson wrote:
> The IDDIG input pin is normally used to determine the USB mode
> (i.e. HOST or DEVICE).
> 
> On some systems (e.g. AM335x) leaving this pin floating allows
> the USB mode to be set via software.
> 
> This patch adds support for this via the device tree.
> 
> Signed-off-by: Mark Jackson <mpfj-2FZW7xY0fHgqdlJmJB21zg@public.gmane.org>
> ---
>  .../devicetree/bindings/usb/am33xx-usb.txt         |    2 ++
>  drivers/usb/musb/musb_dsps.c                       |   14 ++++++++++++++
>  include/linux/usb/musb.h                           |    1 +
>  3 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/am33xx-usb.txt b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> index 20c2ff2..560b7ff 100644
> --- a/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> @@ -47,6 +47,8 @@ USB
>  - dmas: specifies the dma channels
>  - dma-names: specifies the names of the channels. Use "rxN" for receive
>    and "txN" for transmit endpoints. N specifies the endpoint number.
> +- ti,force-host: specifies that the IDDIG input be ignored and the device be
> +  put into host mode regardless.

You should always CC devicetree-discuss if adding new bindings. Why
another binding anyway? We have the common binding dr_mode already.
Please use this and of_usb_get_dr_mode from drivers/usb/usb-common.c
instead.

Thanks,
Michael
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
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] Allow MUSB DSPS to use "force host" mode

[Mark Jackson]

On 22/11/13 16:38, Michael Grzeschik wrote:
> Hallo,
> 
> On Fri, Nov 22, 2013 at 03:55:59PM +0000, Mark Jackson wrote:
>> The IDDIG input pin is normally used to determine the USB mode
>> (i.e. HOST or DEVICE).
>>
>> On some systems (e.g. AM335x) leaving this pin floating allows
>> the USB mode to be set via software.
>>
>> This patch adds support for this via the device tree.
>>
>> Signed-off-by: Mark Jackson <mpfj@newflow.co.uk>
>> ---
>>  .../devicetree/bindings/usb/am33xx-usb.txt         |    2 ++
>>  drivers/usb/musb/musb_dsps.c                       |   14 ++++++++++++++
>>  include/linux/usb/musb.h                           |    1 +
>>  3 files changed, 17 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/am33xx-usb.txt b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
>> index 20c2ff2..560b7ff 100644
>> --- a/Documentation/devicetree/bindings/usb/am33xx-usb.txt
>> +++ b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
>> @@ -47,6 +47,8 @@ USB
>>  - dmas: specifies the dma channels
>>  - dma-names: specifies the names of the channels. Use "rxN" for receive
>>    and "txN" for transmit endpoints. N specifies the endpoint number.
>> +- ti,force-host: specifies that the IDDIG input be ignored and the device be
>> +  put into host mode regardless.
> 
> You should always CC devicetree-discuss if adding new bindings. Why
> another binding anyway? We have the common binding dr_mode already.
> Please use this and of_usb_get_dr_mode from drivers/usb/usb-common.c
> instead.

Sure ... that's a nicer solution.

I'll do that for v2

Mark J.

Re: [PATCH] Allow MUSB DSPS to use "force host" mode

[Felipe Balbi]

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

Hi,

On Fri, Nov 22, 2013 at 03:55:59PM +0000, Mark Jackson wrote:
> The IDDIG input pin is normally used to determine the USB mode
> (i.e. HOST or DEVICE).
> 
> On some systems (e.g. AM335x) leaving this pin floating allows
> the USB mode to be set via software.
> 
> This patch adds support for this via the device tree.
> 
> Signed-off-by: Mark Jackson <mpfj@newflow.co.uk>
> ---
>  .../devicetree/bindings/usb/am33xx-usb.txt         |    2 ++
>  drivers/usb/musb/musb_dsps.c                       |   14 ++++++++++++++
>  include/linux/usb/musb.h                           |    1 +
>  3 files changed, 17 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/am33xx-usb.txt b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> index 20c2ff2..560b7ff 100644
> --- a/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> +++ b/Documentation/devicetree/bindings/usb/am33xx-usb.txt
> @@ -47,6 +47,8 @@ USB
>  - dmas: specifies the dma channels
>  - dma-names: specifies the names of the channels. Use "rxN" for receive
>    and "txN" for transmit endpoints. N specifies the endpoint number.
> +- ti,force-host: specifies that the IDDIG input be ignored and the device be
> +  put into host mode regardless.
>  
>  The controller should have an "usb" alias numbered properly in the alias
>  node.
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 1901f6f..6439809 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -105,6 +105,7 @@ struct dsps_musb_wrapper {
>  	unsigned	otg_disable:5;
>  
>  	/* bit positions for mode */
> +	unsigned	iddig_mux:5;
>  	unsigned	iddig:5;
>  	/* miscellaneous stuff */
>  	u8		poll_seconds;
> @@ -387,6 +388,15 @@ static int dsps_musb_init(struct musb *musb)
>  
>  	musb->isr = dsps_interrupt;
>  
> +	/* Force host mode, rather than relying on IDDIG input */
> +	if (musb->config->force_host) {
> +		val = dsps_readl(reg_base, wrp->mode);
> +		/* clear IDDIG bit, set IDDIG_MUX bit */
> +		val &= ~(1 << wrp->iddig);
> +		val |= (1 << wrp->iddig_mux);
> +		dsps_writel(musb->ctrl_base, wrp->mode, val);
> +	}
> +
>  	/* reset the otgdisable bit, needed for host mode to work */
>  	val = dsps_readl(reg_base, wrp->phy_utmi);
>  	val &= ~(1 << wrp->otg_disable);
> @@ -512,6 +522,9 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>  	pdata.power = get_int_prop(dn, "mentor,power") / 2;
>  	config->multipoint = of_property_read_bool(dn, "mentor,multipoint");
>  
> +	if (of_property_read_bool(dn, "ti,force-host"))
> +		config->force_host = true;
> +
>  	ret = platform_device_add_data(musb, &pdata, sizeof(pdata));
>  	if (ret) {
>  		dev_err(dev, "failed to add platform_data\n");
> @@ -607,6 +620,7 @@ static const struct dsps_musb_wrapper am33xx_driver_data = {
>  	.mode			= 0xe8,
>  	.reset			= 0,
>  	.otg_disable		= 21,
> +	.iddig_mux		= 7,

I recently sent a patch to make this work, I'm attaching it here in case
it didn't reach mailing list.

-- 
balbi

[-- Attachment #1.2: 0001-usb-musb-dsps-implement-set_mode.diff --]
[-- Type: text/x-diff, Size: 2864 bytes --]

From 5edc4b78bbfdc5fcff2cba46c7dbeebd2efddb76 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Tue, 29 Oct 2013 12:17:16 -0500
Subject: [PATCH 1/2] usb: musb: dsps: implement ->set_mode()

this will let us support broken designs such
as AM335x EVM SK where ID pin isn't routed
anywhere on a host port.

With this we can toggle internal IDDIG signal
and make sure MUSB goes into host mode as
necessary.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/musb/musb_dsps.c | 51 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 51 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 1901f6f..ce7ec01 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -106,6 +106,7 @@ struct dsps_musb_wrapper {
 
 	/* bit positions for mode */
 	unsigned	iddig:5;
+	unsigned	iddig_mux:5;
 	/* miscellaneous stuff */
 	u8		poll_seconds;
 };
@@ -406,6 +407,54 @@ static int dsps_musb_exit(struct musb *musb)
 	return 0;
 }
 
+static int dsps_musb_set_mode(struct musb *musb, u8 mode)
+{
+	struct device *dev = musb->controller;
+	struct dsps_glue *glue = dev_get_drvdata(dev->parent);
+	const struct dsps_musb_wrapper *wrp = glue->wrp;
+	void __iomem *ctrl_base = musb->ctrl_base;
+	void __iomem *base = musb->mregs;
+	u32 reg;
+
+	reg = dsps_readl(base, wrp->mode);
+
+	switch (mode) {
+	case MUSB_HOST:
+		reg &= ~(1 << wrp->iddig);
+
+		/*
+		 * if we're setting mode to host-only or device-only, we're
+		 * going to ignore whatever the PHY sends us and just force
+		 * ID pin status by SW
+		 */
+		reg |= (1 << wrp->iddig_mux);
+
+		dsps_writel(base, wrp->mode, reg);
+		dsps_writel(ctrl_base, wrp->phy_utmi, 0x02);
+		break;
+	case MUSB_PERIPHERAL:
+		reg |= (1 << wrp->iddig);
+
+		/*
+		 * if we're setting mode to host-only or device-only, we're
+		 * going to ignore whatever the PHY sends us and just force
+		 * ID pin status by SW
+		 */
+		reg |= (1 << wrp->iddig_mux);
+
+		dsps_writel(base, wrp->mode, reg);
+		break;
+	case MUSB_OTG:
+		dsps_writel(base, wrp->phy_utmi, 0x02);
+		break;
+	default:
+		dev_err(glue->dev, "unsupported mode %d\n", mode);
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static struct musb_platform_ops dsps_ops = {
 	.init		= dsps_musb_init,
 	.exit		= dsps_musb_exit,
@@ -414,6 +463,7 @@ static struct musb_platform_ops dsps_ops = {
 	.disable	= dsps_musb_disable,
 
 	.try_idle	= dsps_musb_try_idle,
+	.set_mode	= dsps_musb_set_mode,
 };
 
 static u64 musb_dmamask = DMA_BIT_MASK(32);
@@ -608,6 +658,7 @@ static const struct dsps_musb_wrapper am33xx_driver_data = {
 	.reset			= 0,
 	.otg_disable		= 21,
 	.iddig			= 8,
+	.iddig_mux		= 7,
 	.usb_shift		= 0,
 	.usb_mask		= 0x1ff,
 	.usb_bitmap		= (0x1ff << 0),
-- 
1.8.4.GIT


[-- Attachment #1.3: 0002-usb-musb-core-call-musb_platform_set_mode-during-prob.diff --]
[-- Type: text/x-diff, Size: 1509 bytes --]

From c9dfe3d2b4c3f00951839eb17835daa0c5c19d71 Mon Sep 17 00:00:00 2001
From: Felipe Balbi <balbi@ti.com>
Date: Tue, 29 Oct 2013 12:17:17 -0500
Subject: [PATCH 2/2] usb: musb: core: call musb_platform_set_mode() during
 probe

This will tell glue layer which mode we want port
to be in.

Signed-off-by: Felipe Balbi <balbi@ti.com>
---
 drivers/usb/musb/musb_core.c | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 0a43329..377ef9b 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1941,17 +1941,26 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	switch (musb->port_mode) {
 	case MUSB_PORT_MODE_HOST:
 		status = musb_host_setup(musb, plat->power);
+		if (status < 0)
+			goto fail3;
+		status = musb_platform_set_mode(musb, MUSB_HOST);
 		break;
 	case MUSB_PORT_MODE_GADGET:
 		status = musb_gadget_setup(musb);
+		if (status < 0)
+			goto fail3;
+		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
 		break;
 	case MUSB_PORT_MODE_DUAL_ROLE:
 		status = musb_host_setup(musb, plat->power);
 		if (status < 0)
 			goto fail3;
 		status = musb_gadget_setup(musb);
-		if (status)
+		if (status) {
 			musb_host_cleanup(musb);
+			goto fail3;
+		}
+		status = musb_platform_set_mode(musb, MUSB_OTG);
 		break;
 	default:
 		dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
-- 
1.8.4.GIT


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