[RESEND PATCH v3 0/4] usb: musb bugfixes

[RESEND PATCH v3 0/4] usb: musb bugfixes

[Markus Pargmann]

Hi,

I rebased this series onto Felipe's next branch. There are no other changes.

The series contains two bugfixes and some debugfs file operations for dsps
similar to the musb core regdump debugfs file.

Regards,

Markus Pargmann

Changes in v3:
- Using debugfs_reg32 for regdump debug file
- Added a patch to replace kzalloc with devm_kzalloc
- otg->gadget_driver NULL pointer handling was replaced by a musb_g_reset
  patch to force IDLE states when no gadget driver is loaded.

Markus Pargmann (4):
  usb: musb: gadget, stay IDLE without gadget driver
  usb: musb: Bugfix of_node assignment
  usb: musb: dsps, debugfs files
  usb: musb: dsps, use devm_kzalloc

 drivers/usb/musb/musb_core.c   | 15 ++++++++---
 drivers/usb/musb/musb_dsps.c   | 60 +++++++++++++++++++++++++++++++++++++++---
 drivers/usb/musb/musb_gadget.c | 14 ++++++++--
 3 files changed, 79 insertions(+), 10 deletions(-)

-- 
1.8.4.2

[RESEND PATCH v3 1/4] usb: musb: gadget, stay IDLE without gadget driver

[Markus Pargmann]

If there is no gadget driver musb should stay in B_IDLE state.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/usb/musb/musb_core.c   |  3 ---
 drivers/usb/musb/musb_gadget.c | 14 ++++++++++++--
 2 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 4db987f..8b7d903 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -831,12 +831,9 @@ b_host:
 			case OTG_STATE_B_WAIT_ACON:
 				dev_dbg(musb->controller, "HNP: RESET (%s), to b_peripheral\n",
 					usb_otg_state_string(musb->xceiv->state));
-				musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
 				musb_g_reset(musb);
 				break;
 			case OTG_STATE_B_IDLE:
-				musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
-				/* FALLTHROUGH */
 			case OTG_STATE_B_PERIPHERAL:
 				musb_g_reset(musb);
 				break;
diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index 9a08679..c352f73 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -2110,10 +2110,20 @@ __acquires(musb->lock)
 	 * or else after HNP, as A-Device
 	 */
 	if (devctl & MUSB_DEVCTL_BDEVICE) {
-		musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
+		if (!musb->gadget_driver) {
+			musb->is_active = 0;
+			musb->xceiv->state = OTG_STATE_B_IDLE;
+		} else {
+			musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
+		}
 		musb->g.is_a_peripheral = 0;
 	} else {
-		musb->xceiv->state = OTG_STATE_A_PERIPHERAL;
+		if (!musb->gadget_driver) {
+			musb->is_active = 0;
+			musb->xceiv->state = OTG_STATE_A_IDLE;
+		} else {
+			musb->xceiv->state = OTG_STATE_A_PERIPHERAL;
+		}
 		musb->g.is_a_peripheral = 1;
 	}
 
-- 
1.8.4.2

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Markus Pargmann]

It is not safe to assign the of_node to a device without driver. The
device is matched against a list of drivers and the of_node could lead
to a DT match with the parent driver.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/usb/musb/musb_core.c | 12 +++++++++++-
 drivers/usb/musb/musb_dsps.c |  1 -
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 8b7d903..d5ad9db 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1966,6 +1966,7 @@ static int musb_probe(struct platform_device *pdev)
 	int		irq = platform_get_irq_byname(pdev, "mc");
 	struct resource	*iomem;
 	void __iomem	*base;
+	int ret;
 
 	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!iomem || irq <= 0)
@@ -1975,7 +1976,16 @@ static int musb_probe(struct platform_device *pdev)
 	if (IS_ERR(base))
 		return PTR_ERR(base);
 
-	return musb_init_controller(dev, irq, base);
+	if (dev->parent)
+		dev->of_node = dev->parent->of_node;
+
+	ret = musb_init_controller(dev, irq, base);
+	if (ret) {
+		dev->of_node = NULL;
+		return ret;
+	}
+
+	return 0;
 }
 
 static int musb_remove(struct platform_device *pdev)
diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 82e1da0..7b36d32 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -485,7 +485,6 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
 	musb->dev.parent		= dev;
 	musb->dev.dma_mask		= &musb_dmamask;
 	musb->dev.coherent_dma_mask	= musb_dmamask;
-	musb->dev.of_node		= of_node_get(dn);
 
 	glue->musb = musb;
 
-- 
1.8.4.2

[RESEND PATCH v3 3/4] usb: musb: dsps, debugfs files

[Markus Pargmann]

debugfs files to show the contents of important dsps registers.

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/usb/musb/musb_dsps.c | 55 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 55 insertions(+)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 7b36d32..06debf8 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -46,6 +46,8 @@
 #include <linux/of_irq.h>
 #include <linux/usb/of.h>
 
+#include <linux/debugfs.h>
+
 #include "musb_core.h"
 
 static const struct of_device_id musb_dsps_of_match[];
@@ -119,6 +121,27 @@ struct dsps_glue {
 	const struct dsps_musb_wrapper *wrp; /* wrapper register offsets */
 	struct timer_list timer;	/* otg_workaround timer */
 	unsigned long last_timer;    /* last timer data for each instance */
+
+	struct debugfs_regset32 regset;
+	struct dentry *dbgfs_root;
+};
+
+static const struct debugfs_reg32 dsps_musb_regs[] = {
+	{ "revision",		0x00 },
+	{ "control",		0x14 },
+	{ "status",		0x18 },
+	{ "eoi",		0x24 },
+	{ "intr0_stat",		0x30 },
+	{ "intr1_stat",		0x34 },
+	{ "intr0_set",		0x38 },
+	{ "intr1_set",		0x3c },
+	{ "txmode",		0x70 },
+	{ "rxmode",		0x74 },
+	{ "autoreq",		0xd0 },
+	{ "srpfixtime",		0xd4 },
+	{ "tdown",		0xd8 },
+	{ "phy_utmi",		0xe0 },
+	{ "mode",		0xe8 },
 };
 
 static void dsps_musb_try_idle(struct musb *musb, unsigned long timeout)
@@ -350,6 +373,30 @@ out:
 	return ret;
 }
 
+static int dsps_musb_dbg_init(struct musb *musb, struct dsps_glue *glue)
+{
+	struct dentry *root;
+	struct dentry *file;
+	char buf[128];
+
+	sprintf(buf, "%s.dsps", dev_name(musb->controller));
+	root = debugfs_create_dir(buf, NULL);
+	if (!root)
+		return -ENOMEM;
+	glue->dbgfs_root = root;
+
+	glue->regset.regs = dsps_musb_regs;
+	glue->regset.nregs = ARRAY_SIZE(dsps_musb_regs);
+	glue->regset.base = musb->ctrl_base;
+
+	file = debugfs_create_regset32("regdump", S_IRUGO, root, &glue->regset);
+	if (!file) {
+		debugfs_remove_recursive(root);
+		return -ENOMEM;
+	}
+	return 0;
+}
+
 static int dsps_musb_init(struct musb *musb)
 {
 	struct device *dev = musb->controller;
@@ -359,6 +406,7 @@ static int dsps_musb_init(struct musb *musb)
 	void __iomem *reg_base;
 	struct resource *r;
 	u32 rev, val;
+	int ret;
 
 	r = platform_get_resource_byname(parent, IORESOURCE_MEM, "control");
 	if (!r)
@@ -392,6 +440,10 @@ static int dsps_musb_init(struct musb *musb)
 	val &= ~(1 << wrp->otg_disable);
 	dsps_writel(musb->ctrl_base, wrp->phy_utmi, val);
 
+	ret = dsps_musb_dbg_init(musb, glue);
+	if (ret)
+		return ret;
+
 	return 0;
 }
 
@@ -586,6 +638,9 @@ static int dsps_remove(struct platform_device *pdev)
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 	kfree(glue);
+
+	debugfs_remove_recursive(glue->dbgfs_root);
+
 	return 0;
 }
 
-- 
1.8.4.2

[RESEND PATCH v3 4/4] usb: musb: dsps, use devm_kzalloc

[Markus Pargmann]

Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
---
 drivers/usb/musb/musb_dsps.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 06debf8..058d611 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -596,7 +596,7 @@ static int dsps_probe(struct platform_device *pdev)
 	wrp = match->data;
 
 	/* allocate glue */
-	glue = kzalloc(sizeof(*glue), GFP_KERNEL);
+	glue = devm_kzalloc(&pdev->dev, sizeof(*glue), GFP_KERNEL);
 	if (!glue) {
 		dev_err(&pdev->dev, "unable to allocate glue memory\n");
 		return -ENOMEM;
@@ -624,7 +624,6 @@ err3:
 	pm_runtime_put(&pdev->dev);
 err2:
 	pm_runtime_disable(&pdev->dev);
-	kfree(glue);
 	return ret;
 }
 
@@ -637,7 +636,6 @@ static int dsps_remove(struct platform_device *pdev)
 	/* disable usbss clocks */
 	pm_runtime_put(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
-	kfree(glue);
 
 	debugfs_remove_recursive(glue->dbgfs_root);
 
-- 
1.8.4.2

[RESEND PATCH v3 1/4] usb: musb: gadget, stay IDLE without gadget driver

[Felipe Balbi]

Hi,

On Mon, Nov 18, 2013 at 04:54:35PM +0100, Markus Pargmann wrote:
> If there is no gadget driver musb should stay in B_IDLE state.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/usb/musb/musb_core.c   |  3 ---
>  drivers/usb/musb/musb_gadget.c | 14 ++++++++++++--
>  2 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 4db987f..8b7d903 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -831,12 +831,9 @@ b_host:
>  			case OTG_STATE_B_WAIT_ACON:
>  				dev_dbg(musb->controller, "HNP: RESET (%s), to b_peripheral\n",
>  					usb_otg_state_string(musb->xceiv->state));
> -				musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
>  				musb_g_reset(musb);
>  				break;
>  			case OTG_STATE_B_IDLE:
> -				musb->xceiv->state = OTG_STATE_B_PERIPHERAL;
> -				/* FALLTHROUGH */
>  			case OTG_STATE_B_PERIPHERAL:
>  				musb_g_reset(musb);
>  				break;

this is not the right way to fix it. We in OTG or host-only builds of
this driver, we should never show up in the USB bus until a gadget
driver has been probed. That's what we have ->pullup() for.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/bd0c4529/attachment.sig>

[RESEND PATCH v3 3/4] usb: musb: dsps, debugfs files

[Felipe Balbi]

Hi,

On Mon, Nov 18, 2013 at 04:54:37PM +0100, Markus Pargmann wrote:
> @@ -350,6 +373,30 @@ out:
>  	return ret;
>  }
>  
> +static int dsps_musb_dbg_init(struct musb *musb, struct dsps_glue *glue)
> +{
> +	struct dentry *root;
> +	struct dentry *file;
> +	char buf[128];
> +
> +	sprintf(buf, "%s.dsps", dev_name(musb->controller));

why ? just use the glue's device

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/630ad6d5/attachment.sig>

[RESEND PATCH v3 4/4] usb: musb: dsps, use devm_kzalloc

[Felipe Balbi]

On Mon, Nov 18, 2013 at 04:54:38PM +0100, Markus Pargmann wrote:
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>

no commit log == no commit. Sorry, but I'll be very pedantic about it.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/2c30f025/attachment.sig>

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Felipe Balbi]

Hi,

On Mon, Nov 18, 2013 at 04:54:36PM +0100, Markus Pargmann wrote:
> It is not safe to assign the of_node to a device without driver. The
> device is matched against a list of drivers and the of_node could lead
> to a DT match with the parent driver.
> 
> Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> ---
>  drivers/usb/musb/musb_core.c | 12 +++++++++++-
>  drivers/usb/musb/musb_dsps.c |  1 -
>  2 files changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 8b7d903..d5ad9db 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1966,6 +1966,7 @@ static int musb_probe(struct platform_device *pdev)
>  	int		irq = platform_get_irq_byname(pdev, "mc");
>  	struct resource	*iomem;
>  	void __iomem	*base;
> +	int ret;
>  
>  	iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  	if (!iomem || irq <= 0)
> @@ -1975,7 +1976,16 @@ static int musb_probe(struct platform_device *pdev)
>  	if (IS_ERR(base))
>  		return PTR_ERR(base);
>  
> -	return musb_init_controller(dev, irq, base);
> +	if (dev->parent)
> +		dev->of_node = dev->parent->of_node;
> +
> +	ret = musb_init_controller(dev, irq, base);
> +	if (ret) {
> +		dev->of_node = NULL;
> +		return ret;
> +	}
> +
> +	return 0;
>  }
>  
>  static int musb_remove(struct platform_device *pdev)
> diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
> index 82e1da0..7b36d32 100644
> --- a/drivers/usb/musb/musb_dsps.c
> +++ b/drivers/usb/musb/musb_dsps.c
> @@ -485,7 +485,6 @@ static int dsps_create_musb_pdev(struct dsps_glue *glue,
>  	musb->dev.parent		= dev;
>  	musb->dev.dma_mask		= &musb_dmamask;
>  	musb->dev.coherent_dma_mask	= musb_dmamask;
> -	musb->dev.of_node		= of_node_get(dn);

Sebastian, does this look correct to you ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131125/02ca2fd7/attachment.sig>

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Sebastian Andrzej Siewior]

On 11/25/2013 05:14 PM, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 18, 2013 at 04:54:36PM +0100, Markus Pargmann wrote:
>> It is not safe to assign the of_node to a device without driver.
>> The device is matched against a list of drivers and the of_node
>> could lead to a DT match with the parent driver.
>> 
>> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- 
>> drivers/usb/musb/musb_core.c | 12 +++++++++++- 
>> drivers/usb/musb/musb_dsps.c |  1 - 2 files changed, 11
>> insertions(+), 2 deletions(-)
>> 
>> diff --git a/drivers/usb/musb/musb_core.c
>> b/drivers/usb/musb/musb_core.c index 8b7d903..d5ad9db 100644 ---
>> a/drivers/usb/musb/musb_core.c +++
>> b/drivers/usb/musb/musb_core.c @@ -1966,6 +1966,7 @@ static int
>> musb_probe(struct platform_device *pdev) int		irq =
>> platform_get_irq_byname(pdev, "mc"); struct resource	*iomem; void
>> __iomem	*base; +	int ret;
>> 
>> iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if
>> (!iomem || irq <= 0) @@ -1975,7 +1976,16 @@ static int
>> musb_probe(struct platform_device *pdev) if (IS_ERR(base)) return
>> PTR_ERR(base);
>> 
>> -	return musb_init_controller(dev, irq, base); +	if
>> (dev->parent)

don't we always have a parent?

>> +		dev->of_node = dev->parent->of_node; + +	ret =
>> musb_init_controller(dev, irq, base); +	if (ret) { +
>> dev->of_node = NULL; +		return ret; +	} + +	return 0; }
>> 
>> static int musb_remove(struct platform_device *pdev) diff --git
>> a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c 
>> index 82e1da0..7b36d32 100644 --- a/drivers/usb/musb/musb_dsps.c 
>> +++ b/drivers/usb/musb/musb_dsps.c @@ -485,7 +485,6 @@ static int
>> dsps_create_musb_pdev(struct dsps_glue *glue, musb->dev.parent		=
>> dev; musb->dev.dma_mask		= &musb_dmamask; 
>> musb->dev.coherent_dma_mask	= musb_dmamask; -	musb->dev.of_node
>> = of_node_get(dn);
> 
> Sebastian, does this look correct to you ?

Is this fixing a bug? Commit 4fc4b2 ("usb: musb: dsps: do not bind to
"musb-hdrc") [0] should fix the problem that is described here.
ux500 is affected by the same problem as dsps but it is only visible on
dsps because we DEFER probe for few reasons there test the fail path.

So by just looking at it should fix the same problem as the change I
cited _but_ it would also cover ux500. Currently I can't think of any
side effects by assigning of_node if the musb_init_*() did not do it.
If we take this in I would suggest to remove the check I added (because
it does no longer serve any purpose) and the description (why we do
this, what is the problem exactly) is could be taken from my patch
since I think it is better described (it safe to assign a node to a
driver, it becomes unsafe if after platform_probe _also_ the DT probe
is executed which was not to designed to work like this).

In the long term I would suggest to move the DT probe over to the musb
core code and we wouldn't need the node assignment anymore?

[0] http://www.spinics.net/lists/linux-usb/msg94701.html

Sebastian

[RESEND PATCH v3 3/4] usb: musb: dsps, debugfs files

[Markus Pargmann]

Hi,

On Mon, Nov 25, 2013 at 09:57:39AM -0600, Felipe Balbi wrote:
> Hi,
> 
> On Mon, Nov 18, 2013 at 04:54:37PM +0100, Markus Pargmann wrote:
> > @@ -350,6 +373,30 @@ out:
> >  	return ret;
> >  }
> >  
> > +static int dsps_musb_dbg_init(struct musb *musb, struct dsps_glue *glue)
> > +{
> > +	struct dentry *root;
> > +	struct dentry *file;
> > +	char buf[128];
> > +
> > +	sprintf(buf, "%s.dsps", dev_name(musb->controller));
> 
> why ? just use the glue's device

I used this to have a more obvious relationship between musb-core
debugfs and musb-dsps debugfs. Otherwise we would have '47401400.usb'
for dsps and 'musb-hdrc.0.auto' for musb core.

But I can change it if you want.

Regards,

Markus

-- 
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 |

[RESEND PATCH v3 4/4] usb: musb: dsps, use devm_kzalloc

[Markus Pargmann]

On Mon, Nov 25, 2013 at 09:58:04AM -0600, Felipe Balbi wrote:
> On Mon, Nov 18, 2013 at 04:54:38PM +0100, Markus Pargmann wrote:
> > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> 
> no commit log == no commit. Sorry, but I'll be very pedantic about it.

Okay I will add a commit message.

Regards,

Markus

-- 
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 |

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Markus Pargmann]

Hi,

On Mon, Nov 25, 2013 at 05:48:54PM +0100, Sebastian Andrzej Siewior wrote:
> On 11/25/2013 05:14 PM, Felipe Balbi wrote:
> > Hi,
> > 
> > On Mon, Nov 18, 2013 at 04:54:36PM +0100, Markus Pargmann wrote:
> >> It is not safe to assign the of_node to a device without driver.
> >> The device is matched against a list of drivers and the of_node
> >> could lead to a DT match with the parent driver.
> >> 
> >> Signed-off-by: Markus Pargmann <mpa@pengutronix.de> --- 
> >> drivers/usb/musb/musb_core.c | 12 +++++++++++- 
> >> drivers/usb/musb/musb_dsps.c |  1 - 2 files changed, 11
> >> insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/usb/musb/musb_core.c
> >> b/drivers/usb/musb/musb_core.c index 8b7d903..d5ad9db 100644 ---
> >> a/drivers/usb/musb/musb_core.c +++
> >> b/drivers/usb/musb/musb_core.c @@ -1966,6 +1966,7 @@ static int
> >> musb_probe(struct platform_device *pdev) int		irq =
> >> platform_get_irq_byname(pdev, "mc"); struct resource	*iomem; void
> >> __iomem	*base; +	int ret;
> >> 
> >> iomem = platform_get_resource(pdev, IORESOURCE_MEM, 0); if
> >> (!iomem || irq <= 0) @@ -1975,7 +1976,16 @@ static int
> >> musb_probe(struct platform_device *pdev) if (IS_ERR(base)) return
> >> PTR_ERR(base);
> >> 
> >> -	return musb_init_controller(dev, irq, base); +	if
> >> (dev->parent)
> 
> don't we always have a parent?
> 
> >> +		dev->of_node = dev->parent->of_node; + +	ret =
> >> musb_init_controller(dev, irq, base); +	if (ret) { +
> >> dev->of_node = NULL; +		return ret; +	} + +	return 0; }
> >> 
> >> static int musb_remove(struct platform_device *pdev) diff --git
> >> a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c 
> >> index 82e1da0..7b36d32 100644 --- a/drivers/usb/musb/musb_dsps.c 
> >> +++ b/drivers/usb/musb/musb_dsps.c @@ -485,7 +485,6 @@ static int
> >> dsps_create_musb_pdev(struct dsps_glue *glue, musb->dev.parent		=
> >> dev; musb->dev.dma_mask		= &musb_dmamask; 
> >> musb->dev.coherent_dma_mask	= musb_dmamask; -	musb->dev.of_node
> >> = of_node_get(dn);
> > 
> > Sebastian, does this look correct to you ?
> 
> Is this fixing a bug? Commit 4fc4b2 ("usb: musb: dsps: do not bind to
> "musb-hdrc") [0] should fix the problem that is described here.
> ux500 is affected by the same problem as dsps but it is only visible on
> dsps because we DEFER probe for few reasons there test the fail path.

Sorry, I didn't check again if this is fixed by some applied patches. I
first discovered this bug in september and send this patch in october.
It is the same bug.

> So by just looking at it should fix the same problem as the change I
> cited _but_ it would also cover ux500. Currently I can't think of any
> side effects by assigning of_node if the musb_init_*() did not do it.
> If we take this in I would suggest to remove the check I added (because
> it does no longer serve any purpose) and the description (why we do
> this, what is the problem exactly) is could be taken from my patch
> since I think it is better described (it safe to assign a node to a
> driver, it becomes unsafe if after platform_probe _also_ the DT probe
> is executed which was not to designed to work like this).

Your patch is already applied, so I could simply drop this one.

Regards,

Markus

> 
> In the long term I would suggest to move the DT probe over to the musb
> core code and we wouldn't need the node assignment anymore?
> 
> [0] http://www.spinics.net/lists/linux-usb/msg94701.html
> 
> Sebastian
> 

-- 
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 |

[RESEND PATCH v3 3/4] usb: musb: dsps, debugfs files

[Felipe Balbi]

Hi,

On Tue, Nov 26, 2013 at 10:25:26AM +0100, Markus Pargmann wrote:
> > On Mon, Nov 18, 2013 at 04:54:37PM +0100, Markus Pargmann wrote:
> > > @@ -350,6 +373,30 @@ out:
> > >  	return ret;
> > >  }
> > >  
> > > +static int dsps_musb_dbg_init(struct musb *musb, struct dsps_glue *glue)
> > > +{
> > > +	struct dentry *root;
> > > +	struct dentry *file;
> > > +	char buf[128];
> > > +
> > > +	sprintf(buf, "%s.dsps", dev_name(musb->controller));
> > 
> > why ? just use the glue's device
> 
> I used this to have a more obvious relationship between musb-core
> debugfs and musb-dsps debugfs. Otherwise we would have '47401400.usb'
> for dsps and 'musb-hdrc.0.auto' for musb core.
> 
> But I can change it if you want.

yeah, I guess it's best to use the device name on this one, even though
it'll look at bit weird. At least it'll be guaranteed to be unique.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131126/2f2112b6/attachment.sig>

[RESEND PATCH v3 4/4] usb: musb: dsps, use devm_kzalloc

[Felipe Balbi]

On Tue, Nov 26, 2013 at 10:26:17AM +0100, Markus Pargmann wrote:
> On Mon, Nov 25, 2013 at 09:58:04AM -0600, Felipe Balbi wrote:
> > On Mon, Nov 18, 2013 at 04:54:38PM +0100, Markus Pargmann wrote:
> > > Signed-off-by: Markus Pargmann <mpa@pengutronix.de>
> > 
> > no commit log == no commit. Sorry, but I'll be very pedantic about it.
> 
> Okay I will add a commit message.

yeah, even if obvious.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20131126/ec4b5c0d/attachment-0001.sig>

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Markus Pargmann]

Hi Sebastian,

This thread was about the of_node assignment for the child device
musb-hdrc. Mainline commit:
	4fc4b274f9b3 (usb: musb: dsps: do not bind to "musb-hdrc")

On Mon, Nov 25, 2013 at 05:48:54PM +0100, Sebastian Andrzej Siewior wrote:
> 
> Is this fixing a bug? Commit 4fc4b2 ("usb: musb: dsps: do not bind to
> "musb-hdrc") [0] should fix the problem that is described here.
> ux500 is affected by the same problem as dsps but it is only visible on
> dsps because we DEFER probe for few reasons there test the fail path.
> 
> So by just looking at it should fix the same problem as the change I
> cited _but_ it would also cover ux500. Currently I can't think of any
> side effects by assigning of_node if the musb_init_*() did not do it.
> If we take this in I would suggest to remove the check I added (because
> it does no longer serve any purpose) and the description (why we do
> this, what is the problem exactly) is could be taken from my patch
> since I think it is better described (it safe to assign a node to a
> driver, it becomes unsafe if after platform_probe _also_ the DT probe
> is executed which was not to designed to work like this).
> 
> In the long term I would suggest to move the DT probe over to the musb
> core code and we wouldn't need the node assignment anymore?
> 
> [0] http://www.spinics.net/lists/linux-usb/msg94701.html

Really old thread but I just noticed that this solution does not work
for automatically parsed properties like 'default' pinctrl. I added
'default' pinctrl settings for the musb device node. They are
automatically setup by the driver core infrastracture as expected.

The problem is that the 'default' pincontrol settings will be applied
again for the musb-hdrc because they are extracted from the of_ndoe
data. This fails because they are already claimed by the 'musb-dsps'
driver. musb-hdrc continues probing, but there is a big error message
that it failed:

[    1.181912] pinctrl-single 44e10800.pinmux: pin 44e10a20.0 already requested by 47401c00.usb; cannot clai
m for musb-hdrc.1.auto
[    1.194053] pinctrl-single 44e10800.pinmux: pin-136 (musb-hdrc.1.auto) status -22
[    1.201930] pinctrl-single 44e10800.pinmux: could not request pin 136 (44e10a20.0) from group pinmux_usb1
_pins  on device pinctrl-single
[    1.214790] musb-hdrc musb-hdrc.1.auto: Error applying setting, reverse things back

Any ideas how to solve it for mainline?

Best regards,

Markus

-- 
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 |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140804/ed1f3e6a/attachment.sig>

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Sebastian Andrzej Siewior]

On 08/04/2014 12:13 PM, Markus Pargmann wrote:

>> In the long term I would suggest to move the DT probe over to the musb
>> core code and we wouldn't need the node assignment anymore?
>>
>> [0] http://www.spinics.net/lists/linux-usb/msg94701.html
> 
> Really old thread but I just noticed that this solution does not work
> for automatically parsed properties like 'default' pinctrl. I added
> 'default' pinctrl settings for the musb device node. They are
> automatically setup by the driver core infrastracture as expected.
> 
> The problem is that the 'default' pincontrol settings will be applied
> again for the musb-hdrc because they are extracted from the of_ndoe
> data. This fails because they are already claimed by the 'musb-dsps'
> driver. musb-hdrc continues probing, but there is a big error message
> that it failed:
> 
> [    1.181912] pinctrl-single 44e10800.pinmux: pin 44e10a20.0 already requested by 47401c00.usb; cannot clai
> m for musb-hdrc.1.auto
> [    1.194053] pinctrl-single 44e10800.pinmux: pin-136 (musb-hdrc.1.auto) status -22
> [    1.201930] pinctrl-single 44e10800.pinmux: could not request pin 136 (44e10a20.0) from group pinmux_usb1
> _pins  on device pinctrl-single
> [    1.214790] musb-hdrc musb-hdrc.1.auto: Error applying setting, reverse things back
> 
> Any ideas how to solve it for mainline?

Just what I said last time and I hope Felipe has the same opinion:
Teach musb-hdrc DT. Which means "ti,musb-am33xx" would be listed for
for musb-hdrc itself and we would lose that child device. The
additional ops that we have would be passed via the data field next to
the id.

> Best regards,
> 
> Markus

Sebastian

[RESEND PATCH v3 2/4] usb: musb: Bugfix of_node assignment

[Felipe Balbi]

On Tue, Aug 05, 2014 at 02:33:25PM +0200, Sebastian Andrzej Siewior wrote:
> On 08/04/2014 12:13 PM, Markus Pargmann wrote:
> 
> >> In the long term I would suggest to move the DT probe over to the musb
> >> core code and we wouldn't need the node assignment anymore?
> >>
> >> [0] http://www.spinics.net/lists/linux-usb/msg94701.html
> > 
> > Really old thread but I just noticed that this solution does not work
> > for automatically parsed properties like 'default' pinctrl. I added
> > 'default' pinctrl settings for the musb device node. They are
> > automatically setup by the driver core infrastracture as expected.
> > 
> > The problem is that the 'default' pincontrol settings will be applied
> > again for the musb-hdrc because they are extracted from the of_ndoe
> > data. This fails because they are already claimed by the 'musb-dsps'
> > driver. musb-hdrc continues probing, but there is a big error message
> > that it failed:
> > 
> > [    1.181912] pinctrl-single 44e10800.pinmux: pin 44e10a20.0 already requested by 47401c00.usb; cannot clai
> > m for musb-hdrc.1.auto
> > [    1.194053] pinctrl-single 44e10800.pinmux: pin-136 (musb-hdrc.1.auto) status -22
> > [    1.201930] pinctrl-single 44e10800.pinmux: could not request pin 136 (44e10a20.0) from group pinmux_usb1
> > _pins  on device pinctrl-single
> > [    1.214790] musb-hdrc musb-hdrc.1.auto: Error applying setting, reverse things back
> > 
> > Any ideas how to solve it for mainline?
> 
> Just what I said last time and I hope Felipe has the same opinion:
> Teach musb-hdrc DT. Which means "ti,musb-am33xx" would be listed for
> for musb-hdrc itself and we would lose that child device. The
> additional ops that we have would be passed via the data field next to
> the id.

sounds good to me.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140805/21431553/attachment-0001.sig>