[PATCH 0/9] usb: musb: several bugfixes for the musb driver

[PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Lothar Waßmann]

The first three patches do some source code cleanup in the files that
are modified in the subsequent patches.

Patch 4 carries the proper fix reported in commit:
        7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")

Patch 6 makes the USBOTG_ID pin override via the USB_MODE register
        really work.

Patch 5 makes the error message upon driver probe failure visible
        without having to recompile the driver with DEBUG enabled.

Patch 7 makes sure the parameters for the usb_gen_phy are properly set
        up before registering it.

Patch 8 makes it possible to use the driver with HW that requires
        external regulators or clocks.

Patch 9 reinstates module unloading support for the musb_am335x driver
        which was disabled due to a false failure analysis

[PATCH 1/9] usb: musb: core: cleanup - remove some useless 'break's from switch statements

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/musb/musb_core.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index b841ee0..8623112 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -680,7 +680,6 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
 		default:
 			/* "should not happen" */
 			musb->is_active = 0;
-			break;
 		}
 	}
 
@@ -737,7 +736,6 @@ b_host:
 				if (hcd)
 					hcd->self.is_b_host = 0;
 			}
-			break;
 		}
 
 		musb_host_poke_root_hub(musb);
@@ -787,7 +785,6 @@ b_host:
 		default:
 			WARNING("unhandled DISCONNECT transition (%s)\n",
 				usb_otg_state_string(musb->xceiv->state));
-			break;
 		}
 	}
 
@@ -2015,7 +2012,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 		break;
 	default:
 		dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
-		break;
 	}
 
 	if (status < 0)
-- 
1.7.10.4

[PATCH 2/9] usb: phy: am335x: group the #includes by subdirs

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/phy/phy-am335x.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
index b70e055..91c71ab 100644
--- a/drivers/usb/phy/phy-am335x.c
+++ b/drivers/usb/phy/phy-am335x.c
@@ -1,13 +1,13 @@
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/dma-mapping.h>
-#include <linux/usb/otg.h>
-#include <linux/usb/usb_phy_generic.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
-#include <linux/regulator/consumer.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
+#include <linux/regulator/consumer.h>
+#include <linux/usb/otg.h>
+#include <linux/usb/usb_phy_generic.h>
 
 #include "am35x-phy-control.h"
 #include "phy-generic.h"
-- 
1.7.10.4

[PATCH 3/9] usb: musb_am335x: source cleanup

[Lothar Waßmann]

- remove comma after end of list delimiter
  The empty entry must always be the last item in the list, thus there
  is no point in having a trailing comma to facilitate adding
  succeding entries.
  Remove the comma, so that inadvertedly adding an entry after the end
  of list sentinel will produce a compile time error rather than an
  unreachable entry in the list.
- consistently use tabs for indentation

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/musb/musb_am335x.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
index 1e58ed2..164c868 100644
--- a/drivers/usb/musb/musb_am335x.c
+++ b/drivers/usb/musb/musb_am335x.c
@@ -21,14 +21,14 @@ err:
 
 static const struct of_device_id am335x_child_of_match[] = {
 	{ .compatible = "ti,am33xx-usb" },
-	{  },
+	{  }
 };
 MODULE_DEVICE_TABLE(of, am335x_child_of_match);
 
 static struct platform_driver am335x_child_driver = {
 	.probe		= am335x_child_probe,
-	.driver         = {
-		.name   = "am335x-usb-childs",
+	.driver		= {
+		.name	= "am335x-usb-childs",
 		.of_match_table	= am335x_child_of_match,
 	},
 };
-- 
1.7.10.4

[PATCH 4/9] usb: phy: am335x-control: prevent module from being unloaded when in use

[Lothar Waßmann]

This patch fixes the real cause of the crash that was "fixed" by
commit 7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/phy/phy-am335x-control.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/usb/phy/phy-am335x-control.c b/drivers/usb/phy/phy-am335x-control.c
index 35b6083..df3e1ba 100644
--- a/drivers/usb/phy/phy-am335x-control.c
+++ b/drivers/usb/phy/phy-am335x-control.c
@@ -140,6 +140,9 @@ static int am335x_control_usb_probe(struct platform_device *pdev)
 	const struct of_device_id *of_id;
 	const struct phy_control *phy_ctrl;
 
+	if (!try_module_get(pdev->dev.parent->driver->owner))
+		return -EPROBE_DEFER;
+
 	of_id = of_match_node(omap_control_usb_id_table, pdev->dev.of_node);
 	if (!of_id)
 		return -EINVAL;
@@ -171,8 +174,15 @@ static int am335x_control_usb_probe(struct platform_device *pdev)
 	return 0;
 }
 
+static int am335x_control_usb_remove(struct platform_device *pdev)
+{
+	module_put(pdev->dev.parent->driver->owner);
+	return 0;
+}
+
 static struct platform_driver am335x_control_driver = {
 	.probe		= am335x_control_usb_probe,
+	.remove		= am335x_control_usb_remove,
 	.driver		= {
 		.name	= "am335x-control-usb",
 		.owner	= THIS_MODULE,
-- 
1.7.10.4

[PATCH 5/9] usb: musb: print error message with dev_err() instead of dev_dbg()

[Lothar Waßmann]

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/musb/musb_core.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 8623112..f867b44 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1878,7 +1878,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 	 * Fail when the board needs a feature that's not enabled.
 	 */
 	if (!plat) {
-		dev_dbg(dev, "no platform_data?\n");
+		dev_err(dev, "no platform_data?\n");
 		status = -ENODEV;
 		goto fail0;
 	}
-- 
1.7.10.4

[PATCH 6/9] usb: musb: core: properly setup the HW before registering it to the USB core

[Lothar Waßmann]

Without this patch overriding the USBOTG_ID pin by setting the iddig
bit in the USB_MODE register doesn't work because it happens too late
to be recognized correctly.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/musb/musb_core.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index f867b44..bbf2aefb 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1988,18 +1988,21 @@ 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);
+		status = musb_platform_set_mode(musb, MUSB_HOST);
 		if (status < 0)
 			goto fail3;
-		status = musb_platform_set_mode(musb, MUSB_HOST);
+		status = musb_host_setup(musb, plat->power);
 		break;
 	case MUSB_PORT_MODE_GADGET:
-		status = musb_gadget_setup(musb);
+		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
 		if (status < 0)
 			goto fail3;
-		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
+		status = musb_gadget_setup(musb);
 		break;
 	case MUSB_PORT_MODE_DUAL_ROLE:
+		status = musb_platform_set_mode(musb, MUSB_OTG);
+		if (status < 0)
+			goto fail3;
 		status = musb_host_setup(musb, plat->power);
 		if (status < 0)
 			goto fail3;
@@ -2008,7 +2011,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
 			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.7.10.4

[PATCH 7/9] usb: phy: am335x: setup the gen_phy function pointers _before_ adding the phy

[Lothar Waßmann]

Make sure all parameters are correctly set up before registering the
PHY with the USB framework.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/phy/phy-am335x.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
index 91c71ab..6522fa7 100644
--- a/drivers/usb/phy/phy-am335x.c
+++ b/drivers/usb/phy/phy-am335x.c
@@ -56,11 +56,12 @@ static int am335x_phy_probe(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
+	am_phy->usb_phy_gen.phy.init = am335x_init;
+	am_phy->usb_phy_gen.phy.shutdown = am335x_shutdown;
+
 	ret = usb_add_phy_dev(&am_phy->usb_phy_gen.phy);
 	if (ret)
 		return ret;
-	am_phy->usb_phy_gen.phy.init = am335x_init;
-	am_phy->usb_phy_gen.phy.shutdown = am335x_shutdown;
 
 	platform_set_drvdata(pdev, am_phy);
 	device_init_wakeup(dev, true);
-- 
1.7.10.4

[PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Lothar Waßmann]

This patch makes it possible to use the musb driver with HW that
requires external regulators or clocks.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/phy/phy-am335x.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
index 6522fa7..de25674 100644
--- a/drivers/usb/phy/phy-am335x.c
+++ b/drivers/usb/phy/phy-am335x.c
@@ -22,6 +22,7 @@ static int am335x_init(struct usb_phy *phy)
 {
 	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
 
+	usb_gen_phy_init(phy);
 	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, true);
 	return 0;
 }
@@ -31,6 +32,7 @@ static void am335x_shutdown(struct usb_phy *phy)
 	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
 
 	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, false);
+	usb_gen_phy_shutdown(phy);
 }
 
 static int am335x_phy_probe(struct platform_device *pdev)
-- 
1.7.10.4

[PATCH 9/9] usb: musb: musb_am335x: reinstate module loading/unloading support

[Lothar Waßmann]

There is no need to throw the baby out with the bath due to a bad
failure analysis. The commit:
7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal
came to a wrong conclusion about the cause of the crash it was
"fixing". The real culprit was the phy-am335x module that was removed
from underneath its users that were still referencing data from it.
After having fixed this in a previous patch, module unloading can be
reinstated.

Another bug with module loading/unloading was the fact, that after
removing the devices instantiated from DT their 'OF_POPULATED' flag
was still set, so that re-loading the module after an unload had no
effect. This is also fixed in this patch.

Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
---
 drivers/usb/musb/musb_am335x.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
index 164c868..152a6f5 100644
--- a/drivers/usb/musb/musb_am335x.c
+++ b/drivers/usb/musb/musb_am335x.c
@@ -19,6 +19,22 @@ err:
 	return ret;
 }
 
+static int of_remove_populated_child(struct device *dev, void *d)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+
+	of_device_unregister(pdev);
+	of_node_clear_flag(pdev->dev.of_node, OF_POPULATED);
+	return 0;
+}
+
+static int am335x_child_remove(struct platform_device *pdev)
+{
+	device_for_each_child(&pdev->dev, NULL, of_remove_populated_child);
+	pm_runtime_disable(&pdev->dev);
+	return 0;
+}
+
 static const struct of_device_id am335x_child_of_match[] = {
 	{ .compatible = "ti,am33xx-usb" },
 	{  }
@@ -27,17 +43,14 @@ MODULE_DEVICE_TABLE(of, am335x_child_of_match);
 
 static struct platform_driver am335x_child_driver = {
 	.probe		= am335x_child_probe,
+	.remove		= am335x_child_remove,
 	.driver		= {
 		.name	= "am335x-usb-childs",
 		.of_match_table	= am335x_child_of_match,
 	},
 };
 
-static int __init am335x_child_init(void)
-{
-	return platform_driver_register(&am335x_child_driver);
-}
-module_init(am335x_child_init);
+module_platform_driver(am335x_child_driver);
 
 MODULE_DESCRIPTION("AM33xx child devices");
 MODULE_LICENSE("GPL v2");
-- 
1.7.10.4

Re: [PATCH 1/9] usb: musb: core: cleanup - remove some useless 'break's from switch statements

[Felipe Balbi]

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

HI,

On Fri, Jul 18, 2014 at 11:31:22AM +0200, Lothar Waßmann wrote:
> 

no commit log == no commit, also the worst thing you can do is have your
bug fixes depend on cleanups. This is *not* a bug fix by any stretch of
the imagination.

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/musb/musb_core.c |    4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index b841ee0..8623112 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -680,7 +680,6 @@ static irqreturn_t musb_stage0_irq(struct musb *musb, u8 int_usb,
>  		default:
>  			/* "should not happen" */
>  			musb->is_active = 0;
> -			break;
>  		}
>  	}
>  
> @@ -737,7 +736,6 @@ b_host:
>  				if (hcd)
>  					hcd->self.is_b_host = 0;
>  			}
> -			break;
>  		}
>  
>  		musb_host_poke_root_hub(musb);
> @@ -787,7 +785,6 @@ b_host:
>  		default:
>  			WARNING("unhandled DISCONNECT transition (%s)\n",
>  				usb_otg_state_string(musb->xceiv->state));
> -			break;
>  		}
>  	}
>  
> @@ -2015,7 +2012,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  		break;
>  	default:
>  		dev_err(dev, "unsupported port mode %d\n", musb->port_mode);
> -		break;
>  	}
>  
>  	if (status < 0)
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

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

Re: [PATCH 2/9] usb: phy: am335x: group the #includes by subdirs

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:23AM +0200, Lothar Waßmann wrote:
> 

no commit log. Not a bug fix.

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/phy/phy-am335x.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
> index b70e055..91c71ab 100644
> --- a/drivers/usb/phy/phy-am335x.c
> +++ b/drivers/usb/phy/phy-am335x.c
> @@ -1,13 +1,13 @@
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
>  #include <linux/dma-mapping.h>
> -#include <linux/usb/otg.h>
> -#include <linux/usb/usb_phy_generic.h>
>  #include <linux/slab.h>
>  #include <linux/clk.h>
> -#include <linux/regulator/consumer.h>
>  #include <linux/of.h>
>  #include <linux/of_address.h>
> +#include <linux/regulator/consumer.h>
> +#include <linux/usb/otg.h>
> +#include <linux/usb/usb_phy_generic.h>
>  
>  #include "am35x-phy-control.h"
>  #include "phy-generic.h"
> -- 
> 1.7.10.4
> 

-- 
balbi

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

Re: [PATCH 3/9] usb: musb_am335x: source cleanup

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:24AM +0200, Lothar Waßmann wrote:
> - remove comma after end of list delimiter
>   The empty entry must always be the last item in the list, thus there
>   is no point in having a trailing comma to facilitate adding
>   succeding entries.
>   Remove the comma, so that inadvertedly adding an entry after the end
>   of list sentinel will produce a compile time error rather than an
>   unreachable entry in the list.
> - consistently use tabs for indentation
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/musb/musb_am335x.c |    6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
> index 1e58ed2..164c868 100644
> --- a/drivers/usb/musb/musb_am335x.c
> +++ b/drivers/usb/musb/musb_am335x.c
> @@ -21,14 +21,14 @@ err:
>  
>  static const struct of_device_id am335x_child_of_match[] = {
>  	{ .compatible = "ti,am33xx-usb" },
> -	{  },
> +	{  }

makes not difference, but fine.

>  };
>  MODULE_DEVICE_TABLE(of, am335x_child_of_match);
>  
>  static struct platform_driver am335x_child_driver = {
>  	.probe		= am335x_child_probe,
> -	.driver         = {
> -		.name   = "am335x-usb-childs",
> +	.driver		= {
> +		.name	= "am335x-usb-childs",

thanks, still not a bug fix.

>  		.of_match_table	= am335x_child_of_match,
>  	},
>  };
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

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

Re: [PATCH 4/9] usb: phy: am335x-control: prevent module from being unloaded when in use

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:25AM +0200, Lothar Waßmann wrote:
> This patch fixes the real cause of the crash that was "fixed" by
> commit 7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>

what is the "real cause of the crash" ? You don't explain that here.

> ---
>  drivers/usb/phy/phy-am335x-control.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/drivers/usb/phy/phy-am335x-control.c b/drivers/usb/phy/phy-am335x-control.c
> index 35b6083..df3e1ba 100644
> --- a/drivers/usb/phy/phy-am335x-control.c
> +++ b/drivers/usb/phy/phy-am335x-control.c
> @@ -140,6 +140,9 @@ static int am335x_control_usb_probe(struct platform_device *pdev)
>  	const struct of_device_id *of_id;
>  	const struct phy_control *phy_ctrl;
>  
> +	if (!try_module_get(pdev->dev.parent->driver->owner))
> +		return -EPROBE_DEFER;
> +
>  	of_id = of_match_node(omap_control_usb_id_table, pdev->dev.of_node);
>  	if (!of_id)
>  		return -EINVAL;
> @@ -171,8 +174,15 @@ static int am335x_control_usb_probe(struct platform_device *pdev)
>  	return 0;
>  }
>  
> +static int am335x_control_usb_remove(struct platform_device *pdev)
> +{
> +	module_put(pdev->dev.parent->driver->owner);
> +	return 0;
> +}

I can't see how this can make any difference. Care to explain ?

-- 
balbi

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

Re: [PATCH 5/9] usb: musb: print error message with dev_err() instead of dev_dbg()

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:26AM +0200, Lothar Waßmann wrote:
> 

no commit log. Not a bug fix.

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/musb/musb_core.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index 8623112..f867b44 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1878,7 +1878,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  	 * Fail when the board needs a feature that's not enabled.
>  	 */
>  	if (!plat) {
> -		dev_dbg(dev, "no platform_data?\n");
> +		dev_err(dev, "no platform_data?\n");
>  		status = -ENODEV;
>  		goto fail0;
>  	}
> -- 
> 1.7.10.4
> 

-- 
balbi

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

Re: [PATCH 6/9] usb: musb: core: properly setup the HW before registering it to the USB core

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:27AM +0200, Lothar Waßmann wrote:
> Without this patch overriding the USBOTG_ID pin by setting the iddig
> bit in the USB_MODE register doesn't work because it happens too late
> to be recognized correctly.

and how did you test this ? Why is it too late ? What was your setup
when testing this ?

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/musb/musb_core.c |   12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
> index f867b44..bbf2aefb 100644
> --- a/drivers/usb/musb/musb_core.c
> +++ b/drivers/usb/musb/musb_core.c
> @@ -1988,18 +1988,21 @@ 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);
> +		status = musb_platform_set_mode(musb, MUSB_HOST);
>  		if (status < 0)
>  			goto fail3;
> -		status = musb_platform_set_mode(musb, MUSB_HOST);
> +		status = musb_host_setup(musb, plat->power);
>  		break;
>  	case MUSB_PORT_MODE_GADGET:
> -		status = musb_gadget_setup(musb);
> +		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
>  		if (status < 0)
>  			goto fail3;
> -		status = musb_platform_set_mode(musb, MUSB_PERIPHERAL);
> +		status = musb_gadget_setup(musb);
>  		break;
>  	case MUSB_PORT_MODE_DUAL_ROLE:
> +		status = musb_platform_set_mode(musb, MUSB_OTG);
> +		if (status < 0)
> +			goto fail3;
>  		status = musb_host_setup(musb, plat->power);
>  		if (status < 0)
>  			goto fail3;
> @@ -2008,7 +2011,6 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>  			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.7.10.4
> 

-- 
balbi

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

Re: [PATCH 7/9] usb: phy: am335x: setup the gen_phy function pointers _before_ adding the phy

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:28AM +0200, Lothar Waßmann wrote:
> Make sure all parameters are correctly set up before registering the
> PHY with the USB framework.
> 
> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/phy/phy-am335x.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
> index 91c71ab..6522fa7 100644
> --- a/drivers/usb/phy/phy-am335x.c
> +++ b/drivers/usb/phy/phy-am335x.c
> @@ -56,11 +56,12 @@ static int am335x_phy_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	am_phy->usb_phy_gen.phy.init = am335x_init;
> +	am_phy->usb_phy_gen.phy.shutdown = am335x_shutdown;
> +
>  	ret = usb_add_phy_dev(&am_phy->usb_phy_gen.phy);
>  	if (ret)
>  		return ret;
> -	am_phy->usb_phy_gen.phy.init = am335x_init;
> -	am_phy->usb_phy_gen.phy.shutdown = am335x_shutdown;
>  
>  	platform_set_drvdata(pdev, am_phy);
>  	device_init_wakeup(dev, true);

both of these need to be done before registering the PHY too.

-- 
balbi

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

Re: [PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Felipe Balbi]

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

Hi,

On Fri, Jul 18, 2014 at 11:31:29AM +0200, Lothar Waßmann wrote:
> This patch makes it possible to use the musb driver with HW that
> requires external regulators or clocks.

can you provide an example of such HW ? Are you not using the internal
PHYs ?

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/phy/phy-am335x.c |    2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
> index 6522fa7..de25674 100644
> --- a/drivers/usb/phy/phy-am335x.c
> +++ b/drivers/usb/phy/phy-am335x.c
> @@ -22,6 +22,7 @@ static int am335x_init(struct usb_phy *phy)
>  {
>  	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
>  
> +	usb_gen_phy_init(phy);
>  	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, true);
>  	return 0;
>  }
> @@ -31,6 +32,7 @@ static void am335x_shutdown(struct usb_phy *phy)
>  	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
>  
>  	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, false);
> +	usb_gen_phy_shutdown(phy);
>  }
>  
>  static int am335x_phy_probe(struct platform_device *pdev)
> -- 
> 1.7.10.4
> 

-- 
balbi

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

Re: [PATCH 9/9] usb: musb: musb_am335x: reinstate module loading/unloading support

[Felipe Balbi]

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

On Fri, Jul 18, 2014 at 11:31:30AM +0200, Lothar Waßmann wrote:
> There is no need to throw the baby out with the bath due to a bad
> failure analysis. The commit:
> 7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal
> came to a wrong conclusion about the cause of the crash it was
> "fixing". The real culprit was the phy-am335x module that was removed
> from underneath its users that were still referencing data from it.
> After having fixed this in a previous patch, module unloading can be
> reinstated.
> 
> Another bug with module loading/unloading was the fact, that after
> removing the devices instantiated from DT their 'OF_POPULATED' flag
> was still set, so that re-loading the module after an unload had no
> effect. This is also fixed in this patch.

now this is a good commit log. I still can't see the need for the other
patch adding try_module_get(), though. Another thing, this needs to be
reviewed by DT folks too.

> Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> ---
>  drivers/usb/musb/musb_am335x.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
> index 164c868..152a6f5 100644
> --- a/drivers/usb/musb/musb_am335x.c
> +++ b/drivers/usb/musb/musb_am335x.c
> @@ -19,6 +19,22 @@ err:
>  	return ret;
>  }
>  
> +static int of_remove_populated_child(struct device *dev, void *d)
> +{
> +	struct platform_device *pdev = to_platform_device(dev);
> +
> +	of_device_unregister(pdev);
> +	of_node_clear_flag(pdev->dev.of_node, OF_POPULATED);

I don't think this should be called by drivers; rather by the bus.

> +	return 0;
> +}
> +
> +static int am335x_child_remove(struct platform_device *pdev)
> +{
> +	device_for_each_child(&pdev->dev, NULL, of_remove_populated_child);
> +	pm_runtime_disable(&pdev->dev);
> +	return 0;
> +}
> +
>  static const struct of_device_id am335x_child_of_match[] = {
>  	{ .compatible = "ti,am33xx-usb" },
>  	{  }
> @@ -27,17 +43,14 @@ MODULE_DEVICE_TABLE(of, am335x_child_of_match);
>  
>  static struct platform_driver am335x_child_driver = {
>  	.probe		= am335x_child_probe,
> +	.remove		= am335x_child_remove,
>  	.driver		= {
>  		.name	= "am335x-usb-childs",
>  		.of_match_table	= am335x_child_of_match,
>  	},
>  };
>  
> -static int __init am335x_child_init(void)
> -{
> -	return platform_driver_register(&am335x_child_driver);
> -}
> -module_init(am335x_child_init);
> +module_platform_driver(am335x_child_driver);
>  
>  MODULE_DESCRIPTION("AM33xx child devices");
>  MODULE_LICENSE("GPL v2");
> -- 
> 1.7.10.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
balbi

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

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Ezequiel Garcia]

Hi Lothar,

On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> The first three patches do some source code cleanup in the files that
> are modified in the subsequent patches.
> 

I've applied patches 4 and 9 on a recent -next, after fixing a conflict
due to patch 3 ("usb: musb_am335x: source cleanup"):

> Patch 4 carries the proper fix reported in commit:
>         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> 
> Patch 9 reinstates module unloading support for the musb_am335x driver
>         which was disabled due to a false failure analysis
> 

For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>

Tested on a beaglebone with a mass storage USB device, module load/unload
works without issues. The module_get/put in the phy is now preventing the
musb_am335x driver unload, which seems to be the real cause of the issue
I reported. Thanks for providing a proper fix!
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Felipe Balbi]

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

Hi,

On Fri, Jul 18, 2014 at 01:16:36PM -0300, Ezequiel Garcia wrote:
> Hi Lothar,
> 
> On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> > The first three patches do some source code cleanup in the files that
> > are modified in the subsequent patches.
> > 
> 
> I've applied patches 4 and 9 on a recent -next, after fixing a conflict
> due to patch 3 ("usb: musb_am335x: source cleanup"):
> 
> > Patch 4 carries the proper fix reported in commit:
> >         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> > 
> > Patch 9 reinstates module unloading support for the musb_am335x driver
> >         which was disabled due to a false failure analysis
> > 
> 
> For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> 
> Tested on a beaglebone with a mass storage USB device, module load/unload
> works without issues. The module_get/put in the phy is now preventing the
> musb_am335x driver unload, which seems to be the real cause of the issue
> I reported. Thanks for providing a proper fix!

I don't that's a good fix. The problem is that even after am335x
removing all its child, someone else tried to release the PHY. That
ordering is the one that needs to be fixed. Doing a module_get on the
parent device looks like a hack to me.

-- 
balbi

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

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Lothar Waßmann]

Hi,

Felipe Balbi wrote:
> Hi,
> 
> On Fri, Jul 18, 2014 at 01:16:36PM -0300, Ezequiel Garcia wrote:
> > Hi Lothar,
> > 
> > On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> > > The first three patches do some source code cleanup in the files that
> > > are modified in the subsequent patches.
> > > 
> > 
> > I've applied patches 4 and 9 on a recent -next, after fixing a conflict
> > due to patch 3 ("usb: musb_am335x: source cleanup"):
> > 
> > > Patch 4 carries the proper fix reported in commit:
> > >         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> > > 
> > > Patch 9 reinstates module unloading support for the musb_am335x driver
> > >         which was disabled due to a false failure analysis
> > > 
> > 
> > For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > 
> > Tested on a beaglebone with a mass storage USB device, module load/unload
> > works without issues. The module_get/put in the phy is now preventing the
> > musb_am335x driver unload, which seems to be the real cause of the issue
> > I reported. Thanks for providing a proper fix!
> 
> I don't that's a good fix. The problem is that even after am335x
> removing all its child, someone else tried to release the PHY. That
> ordering is the one that needs to be fixed. Doing a module_get on the
> parent device looks like a hack to me.
> 
No. It guarantees that the module cannot be unloaded when its resources
are still in use!


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Lothar Waßmann]

Hi,

> On Fri, Jul 18, 2014 at 11:31:29AM +0200, Lothar Waßmann wrote:
> > This patch makes it possible to use the musb driver with HW that
> > requires external regulators or clocks.
> 
> can you provide an example of such HW ? Are you not using the internal
> PHYs ?
> 
The Ka-Ro electronics TX48 module uses the mmc0_clk pin as VBUSEN
rathern than usb0_drvvbus. This patch makes it possible to use an
external regulator to handle the VBUS switch through the 'vcc-supply'
property of the underlying generic_phy device.

> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/usb/phy/phy-am335x.c |    2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/usb/phy/phy-am335x.c b/drivers/usb/phy/phy-am335x.c
> > index 6522fa7..de25674 100644
> > --- a/drivers/usb/phy/phy-am335x.c
> > +++ b/drivers/usb/phy/phy-am335x.c
> > @@ -22,6 +22,7 @@ static int am335x_init(struct usb_phy *phy)
> >  {
> >  	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
> >  
> > +	usb_gen_phy_init(phy);
> >  	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, true);
> >  	return 0;
> >  }
> > @@ -31,6 +32,7 @@ static void am335x_shutdown(struct usb_phy *phy)
> >  	struct am335x_phy *am_phy = dev_get_drvdata(phy->dev);
> >  
> >  	phy_ctrl_power(am_phy->phy_ctrl, am_phy->id, false);
> > +	usb_gen_phy_shutdown(phy);
> >  }
> >  
> >  static int am335x_phy_probe(struct platform_device *pdev)
> > -- 
> > 1.7.10.4
> > 
> 


-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Felipe Balbi]

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

Hi,,

On Mon, Jul 21, 2014 at 10:03:07AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> > On Fri, Jul 18, 2014 at 11:31:29AM +0200, Lothar Waßmann wrote:
> > > This patch makes it possible to use the musb driver with HW that
> > > requires external regulators or clocks.
> > 
> > can you provide an example of such HW ? Are you not using the internal
> > PHYs ?
> > 
> The Ka-Ro electronics TX48 module uses the mmc0_clk pin as VBUSEN
> rathern than usb0_drvvbus. This patch makes it possible to use an
> external regulator to handle the VBUS switch through the 'vcc-supply'
> property of the underlying generic_phy device.

OK, I get it now. But why would not use usb0_drvvbus ? You could still
route usb0_drvvbus to the regulator enable pin and the regulator would
be enabled for you once correct values are written to the IP's mailbox.

I suppose this has something to do with layout constraints ?

cheers

-- 
balbi

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

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Felipe Balbi]

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

On Mon, Jul 21, 2014 at 09:34:30AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Felipe Balbi wrote:
> > Hi,
> > 
> > On Fri, Jul 18, 2014 at 01:16:36PM -0300, Ezequiel Garcia wrote:
> > > Hi Lothar,
> > > 
> > > On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> > > > The first three patches do some source code cleanup in the files that
> > > > are modified in the subsequent patches.
> > > > 
> > > 
> > > I've applied patches 4 and 9 on a recent -next, after fixing a conflict
> > > due to patch 3 ("usb: musb_am335x: source cleanup"):
> > > 
> > > > Patch 4 carries the proper fix reported in commit:
> > > >         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> > > > 
> > > > Patch 9 reinstates module unloading support for the musb_am335x driver
> > > >         which was disabled due to a false failure analysis
> > > > 
> > > 
> > > For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > 
> > > Tested on a beaglebone with a mass storage USB device, module load/unload
> > > works without issues. The module_get/put in the phy is now preventing the
> > > musb_am335x driver unload, which seems to be the real cause of the issue
> > > I reported. Thanks for providing a proper fix!
> > 
> > I don't that's a good fix. The problem is that even after am335x
> > removing all its child, someone else tried to release the PHY. That
> > ordering is the one that needs to be fixed. Doing a module_get on the
> > parent device looks like a hack to me.
> > 
> No. It guarantees that the module cannot be unloaded when its resources
> are still in use!

it's still a hack. You have a child incrementing the usage count on its
parent just because a sibbling isn't doing the right thing.

-- 
balbi

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

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Ezequiel Garcia]

On 21 Jul 10:11 AM, Felipe Balbi wrote:
> On Mon, Jul 21, 2014 at 09:34:30AM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > Felipe Balbi wrote:
> > > Hi,
> > > 
> > > On Fri, Jul 18, 2014 at 01:16:36PM -0300, Ezequiel Garcia wrote:
> > > > Hi Lothar,
> > > > 
> > > > On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> > > > > The first three patches do some source code cleanup in the files that
> > > > > are modified in the subsequent patches.
> > > > > 
> > > > 
> > > > I've applied patches 4 and 9 on a recent -next, after fixing a conflict
> > > > due to patch 3 ("usb: musb_am335x: source cleanup"):
> > > > 
> > > > > Patch 4 carries the proper fix reported in commit:
> > > > >         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> > > > > 
> > > > > Patch 9 reinstates module unloading support for the musb_am335x driver
> > > > >         which was disabled due to a false failure analysis
> > > > > 
> > > > 
> > > > For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > > 
> > > > Tested on a beaglebone with a mass storage USB device, module load/unload
> > > > works without issues. The module_get/put in the phy is now preventing the
> > > > musb_am335x driver unload, which seems to be the real cause of the issue
> > > > I reported. Thanks for providing a proper fix!
> > > 
> > > I don't that's a good fix. The problem is that even after am335x
> > > removing all its child, someone else tried to release the PHY. That
> > > ordering is the one that needs to be fixed. Doing a module_get on the
> > > parent device looks like a hack to me.
> > > 
> > No. It guarantees that the module cannot be unloaded when its resources
> > are still in use!
> 
> it's still a hack. You have a child incrementing the usage count on its
> parent just because a sibbling isn't doing the right thing.
> 

How about having the musb_am335x (glue driver) call request_module and
module_get on the phy-am335x? Wouldn't this be a cleaner approach?

I haven't checked if this possible, though.
-- 
Ezequiel Garcia, VanguardiaSur
www.vanguardiasur.com.ar

Re: [PATCH 0/9] usb: musb: several bugfixes for the musb driver

[Felipe Balbi]

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

On Mon, Jul 21, 2014 at 12:53:52PM -0300, Ezequiel Garcia wrote:
> On 21 Jul 10:11 AM, Felipe Balbi wrote:
> > On Mon, Jul 21, 2014 at 09:34:30AM +0200, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > Felipe Balbi wrote:
> > > > Hi,
> > > > 
> > > > On Fri, Jul 18, 2014 at 01:16:36PM -0300, Ezequiel Garcia wrote:
> > > > > Hi Lothar,
> > > > > 
> > > > > On 18 Jul 11:31 AM, Lothar Waßmann wrote:
> > > > > > The first three patches do some source code cleanup in the files that
> > > > > > are modified in the subsequent patches.
> > > > > > 
> > > > > 
> > > > > I've applied patches 4 and 9 on a recent -next, after fixing a conflict
> > > > > due to patch 3 ("usb: musb_am335x: source cleanup"):
> > > > > 
> > > > > > Patch 4 carries the proper fix reported in commit:
> > > > > >         7adb5c876e9c ("usb: musb: Fix panic upon musb_am335x module removal")
> > > > > > 
> > > > > > Patch 9 reinstates module unloading support for the musb_am335x driver
> > > > > >         which was disabled due to a false failure analysis
> > > > > > 
> > > > > 
> > > > > For these two patches, Tested-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> > > > > 
> > > > > Tested on a beaglebone with a mass storage USB device, module load/unload
> > > > > works without issues. The module_get/put in the phy is now preventing the
> > > > > musb_am335x driver unload, which seems to be the real cause of the issue
> > > > > I reported. Thanks for providing a proper fix!
> > > > 
> > > > I don't that's a good fix. The problem is that even after am335x
> > > > removing all its child, someone else tried to release the PHY. That
> > > > ordering is the one that needs to be fixed. Doing a module_get on the
> > > > parent device looks like a hack to me.
> > > > 
> > > No. It guarantees that the module cannot be unloaded when its resources
> > > are still in use!
> > 
> > it's still a hack. You have a child incrementing the usage count on its
> > parent just because a sibbling isn't doing the right thing.
> > 
> 
> How about having the musb_am335x (glue driver) call request_module and
> module_get on the phy-am335x? Wouldn't this be a cleaner approach?
> 
> I haven't checked if this possible, though.

at most, you would have the phy layer do that so that all PHYs get usage
counter incremented when they're in use. We can't have this 'fixed' for
MUSB only.

-- 
balbi

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

Re: [PATCH 9/9] usb: musb: musb_am335x: reinstate module loading/unloading support

[Lothar Waßmann]

Hi,

Felipe Balbi wrote:
> On Fri, Jul 18, 2014 at 11:31:30AM +0200, Lothar Waßmann wrote:
> > There is no need to throw the baby out with the bath due to a bad
> > failure analysis. The commit:
> > 7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal
> > came to a wrong conclusion about the cause of the crash it was
> > "fixing". The real culprit was the phy-am335x module that was removed
> > from underneath its users that were still referencing data from it.
> > After having fixed this in a previous patch, module unloading can be
> > reinstated.
> > 
> > Another bug with module loading/unloading was the fact, that after
> > removing the devices instantiated from DT their 'OF_POPULATED' flag
> > was still set, so that re-loading the module after an unload had no
> > effect. This is also fixed in this patch.
> 
> now this is a good commit log. I still can't see the need for the other
> patch adding try_module_get(), though. Another thing, this needs to be
> reviewed by DT folks too.
> 
Without holding a reference to the phy module, the module may be
unloaded when its resources are still in use which may lead to the
crash observed in the above stated commit.

> > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > ---
> >  drivers/usb/musb/musb_am335x.c |   23 ++++++++++++++++++-----
> >  1 file changed, 18 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
> > index 164c868..152a6f5 100644
> > --- a/drivers/usb/musb/musb_am335x.c
> > +++ b/drivers/usb/musb/musb_am335x.c
> > @@ -19,6 +19,22 @@ err:
> >  	return ret;
> >  }
> >  
> > +static int of_remove_populated_child(struct device *dev, void *d)
> > +{
> > +	struct platform_device *pdev = to_platform_device(dev);
> > +
> > +	of_device_unregister(pdev);
> > +	of_node_clear_flag(pdev->dev.of_node, OF_POPULATED);
> 
> I don't think this should be called by drivers; rather by the bus.
> 
Possibly the right thing to do would be to use of_platform_depopulate()
in the remove function to pair up with op_platform_populate() in the
probe function, but doing this results in a crash in
platform_device_del() (called from platform_device_unregister()) when
release_resource() is being called on resources that were never
properly registered with the device:
Unable to handle kernel NULL pointer dereference at virtual address 00000018
pgd = 8dd64000
[00000018] *pgd=8ddc2831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT ARM
Modules linked in: musb_am335x(-) [last unloaded: snd]
CPU: 0 PID: 1435 Comm: modprobe Not tainted 3.16.0-rc5-next-20140717-dbg+ #13
task: 8da00880 ti: 8dda0000 task.ti: 8dda0000
PC is at release_resource+0x14/0x7c
LR is at release_resource+0x10/0x7c
pc : [<8003165c>]    lr : [<80031658>]    psr: a0000013
sp : 8dda1ec0  ip : 8dda0000  fp : 00000000
r10: 00000000  r9 : 8dda0000  r8 : 8deb7c10
r7 : 8deb7c00  r6 : 00000200  r5 : 00000001  r4 : 8deed380
r3 : 00000000  r2 : 00000000  r1 : 00000011  r0 : 806772a0
Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 10c5387d  Table: 8dd64019  DAC: 00000015
Process modprobe (pid: 1435, stack limit = 0x8dda0238)
Stack: (0x8dda1ec0 to 0x8dda2000)
1ec0: 8da00880 8deed380 00000001 802f850c 00000000 8deed380 44e10620 44e1062f
1ee0: 8deb7c00 00000000 80398c5c 00000081 8000e904 802f8848 8deb7c10 80398d0c
1f00: 00000000 802f3470 8d8d7200 8ddc84b0 8d92e810 7f0f9014 8d92e844 7f0f7010
1f20: 8d92e810 802f8110 802f80f8 802f68f8 7f0f9014 8d92e810 7f0f9014 802f7238
1f40: 7f0f9014 00000000 20000013 802f6764 7f0f9058 8007ec94 00000000 6273756d
1f60: 336d615f 00783533 8da00880 8000e764 00000001 80053ae4 00000058 76f41000
1f80: 7eb299e8 01290838 00000011 00000081 60000010 0000e854 7eb299e8 01290838
1fa0: 00000011 8000e740 7eb299e8 01290838 012908a0 00000080 00000000 00000001
1fc0: 7eb299e8 01290838 00000011 00000081 012908a0 00000000 01290844 00000000
1fe0: 76eb76f0 7eb2995c 0000a534 76eb76fc 60000010 012908a0 aaaaaaaa aaaaaaaa
[<8003165c>] (release_resource) from [<802f850c>] (platform_device_del+0xb4/0xf4)
[<802f850c>] (platform_device_del) from [<802f8848>] (platform_device_unregister+0xc/0x18)
[<802f8848>] (platform_device_unregister) from [<80398d0c>] (of_platform_device_destroy+0xb0/0xc8)
[<80398d0c>] (of_platform_device_destroy) from [<802f3470>] (device_for_each_child+0x34/0x74)
[<802f3470>] (device_for_each_child) from [<7f0f7010>] (am335x_child_remove+0x10/0x24 [musb_am335x])
[<7f0f7010>] (am335x_child_remove [musb_am335x]) from [<802f8110>] (platform_drv_remove+0x18/0x1c)
[<802f8110>] (platform_drv_remove) from [<802f68f8>] (__device_release_driver+0x70/0xc4)
[<802f68f8>] (__device_release_driver) from [<802f7238>] (driver_detach+0xb4/0xb8)
[<802f7238>] (driver_detach) from [<802f6764>] (bus_remove_driver+0x5c/0xa4)
[<802f6764>] (bus_remove_driver) from [<8007ec94>] (SyS_delete_module+0x120/0x18c)
[<8007ec94>] (SyS_delete_module) from [<8000e740>] (ret_fast_syscall+0x0/0x48)
Code: e1a04000 e59f0068 eb10bac4 e5943010 (e5932018) 
---[ end trace 24ca43dfc1a677d6 ]---

The cause for this seems to be calling platform_device_unregister() on
a device that was created with device_add() (rather than
platform_device_add()).

The call chain for registering a device from of_platform_populate() is:
of_platform_bus_create()
of_platform_device_create_pdata()
of_device_add()
device_add()

The call chain for the of_platform_depopulate() is:
of_platform_device_destroy()
platform_device_unregister()
platform_device_del()
release_resource()

The resources that are being released here have the 'parent' pointer
set to NULL which provokes the crash. This pointer would have been set
up by insert_resource() which is called by platform_device_add() but
not by device_add().

So, the conclusion to me is that of_platform_populate() /
of_platform_depopulate() are broken, because one uses device_*
functions while the other uses the platform_device_* counterparts.

Since there are no current users of of_platform_depopulate() this
obviously has gone unnoticed so far.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Lothar Waßmann]

Hi,

Felipe Balbi wrote:
> Hi,,
> 
> On Mon, Jul 21, 2014 at 10:03:07AM +0200, Lothar Waßmann wrote:
> > Hi,
> > 
> > > On Fri, Jul 18, 2014 at 11:31:29AM +0200, Lothar Waßmann wrote:
> > > > This patch makes it possible to use the musb driver with HW that
> > > > requires external regulators or clocks.
> > > 
> > > can you provide an example of such HW ? Are you not using the internal
> > > PHYs ?
> > > 
> > The Ka-Ro electronics TX48 module uses the mmc0_clk pin as VBUSEN
> > rathern than usb0_drvvbus. This patch makes it possible to use an
> > external regulator to handle the VBUS switch through the 'vcc-supply'
> > property of the underlying generic_phy device.
> 
> OK, I get it now. But why would not use usb0_drvvbus ? You could still
> route usb0_drvvbus to the regulator enable pin and the regulator would
> be enabled for you once correct values are written to the IP's mailbox.
> 
> I suppose this has something to do with layout constraints ?
> 
Yes. The usb0_drvvbus is used for a different purpose.


Lothar Waßmann
-- 
___________________________________________________________

Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen
Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10
Geschäftsführer: Matthias Kaussen
Handelsregistereintrag: Amtsgericht Aachen, HRB 4996

www.karo-electronics.de | info@karo-electronics.de
___________________________________________________________

Re: [PATCH 9/9] usb: musb: musb_am335x: reinstate module loading/unloading support

[Felipe Balbi]

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

On Tue, Jul 22, 2014 at 09:49:30AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Felipe Balbi wrote:
> > On Fri, Jul 18, 2014 at 11:31:30AM +0200, Lothar Waßmann wrote:
> > > There is no need to throw the baby out with the bath due to a bad
> > > failure analysis. The commit:
> > > 7adb5c876e9c usb: musb: Fix panic upon musb_am335x module removal
> > > came to a wrong conclusion about the cause of the crash it was
> > > "fixing". The real culprit was the phy-am335x module that was removed
> > > from underneath its users that were still referencing data from it.
> > > After having fixed this in a previous patch, module unloading can be
> > > reinstated.
> > > 
> > > Another bug with module loading/unloading was the fact, that after
> > > removing the devices instantiated from DT their 'OF_POPULATED' flag
> > > was still set, so that re-loading the module after an unload had no
> > > effect. This is also fixed in this patch.
> > 
> > now this is a good commit log. I still can't see the need for the other
> > patch adding try_module_get(), though. Another thing, this needs to be
> > reviewed by DT folks too.
> > 
> Without holding a reference to the phy module, the module may be
> unloaded when its resources are still in use which may lead to the
> crash observed in the above stated commit.
> 
> > > Signed-off-by: Lothar Waßmann <LW@KARO-electronics.de>
> > > ---
> > >  drivers/usb/musb/musb_am335x.c |   23 ++++++++++++++++++-----
> > >  1 file changed, 18 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/drivers/usb/musb/musb_am335x.c b/drivers/usb/musb/musb_am335x.c
> > > index 164c868..152a6f5 100644
> > > --- a/drivers/usb/musb/musb_am335x.c
> > > +++ b/drivers/usb/musb/musb_am335x.c
> > > @@ -19,6 +19,22 @@ err:
> > >  	return ret;
> > >  }
> > >  
> > > +static int of_remove_populated_child(struct device *dev, void *d)
> > > +{
> > > +	struct platform_device *pdev = to_platform_device(dev);
> > > +
> > > +	of_device_unregister(pdev);
> > > +	of_node_clear_flag(pdev->dev.of_node, OF_POPULATED);
> > 
> > I don't think this should be called by drivers; rather by the bus.
> > 
> Possibly the right thing to do would be to use of_platform_depopulate()
> in the remove function to pair up with op_platform_populate() in the
> probe function, but doing this results in a crash in
> platform_device_del() (called from platform_device_unregister()) when
> release_resource() is being called on resources that were never
> properly registered with the device:
> Unable to handle kernel NULL pointer dereference at virtual address 00000018
> pgd = 8dd64000
> [00000018] *pgd=8ddc2831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] PREEMPT ARM
> Modules linked in: musb_am335x(-) [last unloaded: snd]
> CPU: 0 PID: 1435 Comm: modprobe Not tainted 3.16.0-rc5-next-20140717-dbg+ #13
> task: 8da00880 ti: 8dda0000 task.ti: 8dda0000
> PC is at release_resource+0x14/0x7c
> LR is at release_resource+0x10/0x7c
> pc : [<8003165c>]    lr : [<80031658>]    psr: a0000013
> sp : 8dda1ec0  ip : 8dda0000  fp : 00000000
> r10: 00000000  r9 : 8dda0000  r8 : 8deb7c10
> r7 : 8deb7c00  r6 : 00000200  r5 : 00000001  r4 : 8deed380
> r3 : 00000000  r2 : 00000000  r1 : 00000011  r0 : 806772a0
> Flags: NzCv  IRQs on  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 10c5387d  Table: 8dd64019  DAC: 00000015
> Process modprobe (pid: 1435, stack limit = 0x8dda0238)
> Stack: (0x8dda1ec0 to 0x8dda2000)
> 1ec0: 8da00880 8deed380 00000001 802f850c 00000000 8deed380 44e10620 44e1062f
> 1ee0: 8deb7c00 00000000 80398c5c 00000081 8000e904 802f8848 8deb7c10 80398d0c
> 1f00: 00000000 802f3470 8d8d7200 8ddc84b0 8d92e810 7f0f9014 8d92e844 7f0f7010
> 1f20: 8d92e810 802f8110 802f80f8 802f68f8 7f0f9014 8d92e810 7f0f9014 802f7238
> 1f40: 7f0f9014 00000000 20000013 802f6764 7f0f9058 8007ec94 00000000 6273756d
> 1f60: 336d615f 00783533 8da00880 8000e764 00000001 80053ae4 00000058 76f41000
> 1f80: 7eb299e8 01290838 00000011 00000081 60000010 0000e854 7eb299e8 01290838
> 1fa0: 00000011 8000e740 7eb299e8 01290838 012908a0 00000080 00000000 00000001
> 1fc0: 7eb299e8 01290838 00000011 00000081 012908a0 00000000 01290844 00000000
> 1fe0: 76eb76f0 7eb2995c 0000a534 76eb76fc 60000010 012908a0 aaaaaaaa aaaaaaaa
> [<8003165c>] (release_resource) from [<802f850c>] (platform_device_del+0xb4/0xf4)
> [<802f850c>] (platform_device_del) from [<802f8848>] (platform_device_unregister+0xc/0x18)
> [<802f8848>] (platform_device_unregister) from [<80398d0c>] (of_platform_device_destroy+0xb0/0xc8)
> [<80398d0c>] (of_platform_device_destroy) from [<802f3470>] (device_for_each_child+0x34/0x74)
> [<802f3470>] (device_for_each_child) from [<7f0f7010>] (am335x_child_remove+0x10/0x24 [musb_am335x])
> [<7f0f7010>] (am335x_child_remove [musb_am335x]) from [<802f8110>] (platform_drv_remove+0x18/0x1c)
> [<802f8110>] (platform_drv_remove) from [<802f68f8>] (__device_release_driver+0x70/0xc4)
> [<802f68f8>] (__device_release_driver) from [<802f7238>] (driver_detach+0xb4/0xb8)
> [<802f7238>] (driver_detach) from [<802f6764>] (bus_remove_driver+0x5c/0xa4)
> [<802f6764>] (bus_remove_driver) from [<8007ec94>] (SyS_delete_module+0x120/0x18c)
> [<8007ec94>] (SyS_delete_module) from [<8000e740>] (ret_fast_syscall+0x0/0x48)
> Code: e1a04000 e59f0068 eb10bac4 e5943010 (e5932018) 
> ---[ end trace 24ca43dfc1a677d6 ]---
> 
> The cause for this seems to be calling platform_device_unregister() on
> a device that was created with device_add() (rather than
> platform_device_add()).

good, then you found another bug. Let's fix that and use
of_platform_depopulate().

-- 
balbi

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

Re: [PATCH 8/9] usb: phy: am335x: call usb_gen_phy_init()/usb_gen_phy_shutdown() in am335x_init()/am335x_shutdown()

[Felipe Balbi]

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

On Tue, Jul 22, 2014 at 10:06:13AM +0200, Lothar Waßmann wrote:
> Hi,
> 
> Felipe Balbi wrote:
> > Hi,,
> > 
> > On Mon, Jul 21, 2014 at 10:03:07AM +0200, Lothar Waßmann wrote:
> > > Hi,
> > > 
> > > > On Fri, Jul 18, 2014 at 11:31:29AM +0200, Lothar Waßmann wrote:
> > > > > This patch makes it possible to use the musb driver with HW that
> > > > > requires external regulators or clocks.
> > > > 
> > > > can you provide an example of such HW ? Are you not using the internal
> > > > PHYs ?
> > > > 
> > > The Ka-Ro electronics TX48 module uses the mmc0_clk pin as VBUSEN
> > > rathern than usb0_drvvbus. This patch makes it possible to use an
> > > external regulator to handle the VBUS switch through the 'vcc-supply'
> > > property of the underlying generic_phy device.
> > 
> > OK, I get it now. But why would not use usb0_drvvbus ? You could still
> > route usb0_drvvbus to the regulator enable pin and the regulator would
> > be enabled for you once correct values are written to the IP's mailbox.
> > 
> > I suppose this has something to do with layout constraints ?
> > 
> Yes. The usb0_drvvbus is used for a different purpose.

alright, thanks for the info. I'll revisit this in a few days, quite
busy right now and my tree is closed for v3.17 anyway.

cheers

-- 
balbi

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