[RFC PATCH 0/1] usb: dwc3: Extcon hotplug support to of-simple

[RFC PATCH 0/1] usb: dwc3: Extcon hotplug support to of-simple

[Prashant Malani]

Some platforms like rk3399 would like to power on the USB PHY layer only
when external devices are connected. This patch introduces optional
support for extcon USB_HOST events, so that child devices are
populated/depopulated when external devices are connected/disconnected,
respectively.

This is also useful since some PHY drivers like phy-rockchip-typec only
configure their Type C Phy on power on; if they are only powered on once
at boot by dwc3, these drivers will not be able to reconfigure their PHY
for peripherals plugged in later, like (Display Port) DP monitors.

I thought I’d send out an initial RFC patch, for comments and feedback
about the approach. Depending on feedback, we can refine this approach
and modify the bindings file.

Thanks,

Prashant Malani (1):
  usb: dwc3: of-simple: Add extcon support

 drivers/usb/dwc3/dwc3-of-simple.c | 149 +++++++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 3 deletions(-)

-- 
2.27.0.rc0.183.gde8f92d652-goog

[RFC PATCH 1/1] usb: dwc3: of-simple: Add extcon support

[Prashant Malani]

Add optional extcon notifier support to enable the hotplug / unplug of
the underlying PHY layer devices.

If supported, the Device Tree (DT) node for the device should include an
"extcon" property which is a phandle to an extcon DT node.

This patch is an effort to incorporate the equivalent support from the
Rockchip dwc3 driver implementation from Chrome OS [1] to the mainline.

[1] : https://chromium.googlesource.com/chromiumos/third_party/kernel/
+/refs/heads/chromeos-4.4/drivers/usb/dwc3/dwc3-rockchip.c

Cc: Benson Leung <bleung@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Signed-off-by: Prashant Malani <pmalani@chromium.org>
---
 drivers/usb/dwc3/dwc3-of-simple.c | 149 +++++++++++++++++++++++++++++-
 1 file changed, 146 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
index e64754be47b4..28bde27cd1f9 100644
--- a/drivers/usb/dwc3/dwc3-of-simple.c
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -11,6 +11,7 @@
  * by Subbaraya Sundeep Bhatta <subbaraya.sundeep.bhatta@xilinx.com>
  */
 
+#include <linux/extcon.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/slab.h>
@@ -29,8 +30,117 @@ struct dwc3_of_simple {
 	struct reset_control	*resets;
 	bool			pulse_resets;
 	bool			need_reset;
+	struct extcon_dev	*edev;
+	struct notifier_block	nb;
+	struct work_struct	work;
+	/* Denotes whether child devices have been populated. */
+	bool			populated;
+	bool			suspended;
+	spinlock_t		suspend_lock;
 };
 
+static int dwc3_of_simple_populate(struct dwc3_of_simple *simple)
+{
+	struct device *dev = simple->dev;
+	struct device_node *np = dev->of_node;
+	int ret;
+
+	ret = of_platform_populate(np, NULL, NULL, dev);
+	if (ret) {
+		dev_err(dev, "Failed to populate dwc3 devices.\n");
+		return ret;
+	}
+	simple->populated = true;
+	return 0;
+}
+
+static void dwc3_of_simple_depopulate(struct dwc3_of_simple *simple)
+{
+	if (simple->populated) {
+		of_platform_depopulate(simple->dev);
+		simple->populated = false;
+	}
+}
+
+static void dwc3_of_simple_work(struct work_struct *work)
+{
+	struct dwc3_of_simple *simple = container_of(work,
+					struct dwc3_of_simple, work);
+	struct extcon_dev *edev = simple->edev;
+
+	if (extcon_get_state(edev, EXTCON_USB_HOST) > 0) {
+		if (simple->populated)
+			return;
+
+		dwc3_of_simple_populate(simple);
+	} else {
+		if (!simple->populated)
+			return;
+
+		dwc3_of_simple_depopulate(simple);
+	}
+}
+
+static int dwc3_of_simple_notifier(struct notifier_block *nb,
+			      unsigned long event, void *ptr)
+{
+	struct dwc3_of_simple *simple = container_of(nb, struct dwc3_of_simple,
+						nb);
+	unsigned long flags;
+
+	spin_lock_irqsave(&simple->suspend_lock, flags);
+	if (!simple->suspended)
+		schedule_work(&simple->work);
+	spin_unlock_irqrestore(&simple->suspend_lock, flags);
+
+	return NOTIFY_OK;
+}
+
+static int dwc3_of_simple_extcon_register(struct dwc3_of_simple *simple)
+{
+	struct device		*dev = simple->dev;
+	struct extcon_dev	*edev;
+	int			ret;
+
+	edev = extcon_get_edev_by_phandle(dev, 0);
+	if (IS_ERR(edev)) {
+		/* The extcon property is optional. */
+		if (PTR_ERR(edev) == -ENODEV)
+			return 0;
+		if (PTR_ERR(edev) != -EPROBE_DEFER)
+			dev_err(dev, "Couldn't get extcon device.\n");
+		return PTR_ERR(edev);
+	}
+
+	INIT_WORK(&simple->work, dwc3_of_simple_work);
+
+	simple->nb.notifier_call = dwc3_of_simple_notifier;
+	ret = devm_extcon_register_notifier(dev, edev, EXTCON_USB_HOST,
+					&simple->nb);
+	if (ret < 0) {
+		dev_err(dev, "Failed to register notifier.\n");
+		return ret;
+	}
+
+	simple->edev = edev;
+
+	return 0;
+}
+
+static void dwc3_of_simple_extcon_unregister(struct dwc3_of_simple *simple)
+{
+	if (!simple->edev)
+		return;
+
+	/*
+	 * We explicitly unregister the notifier to prevent races with
+	 * the of_depopulate() call in remove().
+	 */
+	devm_extcon_unregister_notifier(simple->dev, simple->edev,
+					EXTCON_USB_HOST, &simple->nb);
+	cancel_work_sync(&simple->work);
+}
+
 static int dwc3_of_simple_probe(struct platform_device *pdev)
 {
 	struct dwc3_of_simple	*simple;
@@ -47,6 +157,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, simple);
 	simple->dev = dev;
 
+	spin_lock_init(&simple->suspend_lock);
+
 	/*
 	 * Some controllers need to toggle the usb3-otg reset before trying to
 	 * initialize the PHY, otherwise the PHY times out.
@@ -87,9 +199,24 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_resetc_assert;
 
-	ret = of_platform_populate(np, NULL, NULL, dev);
-	if (ret)
+	ret = dwc3_of_simple_extcon_register(simple);
+	if (ret < 0) {
+		dev_warn(dev, "No extcon device found, err: %d\n", ret);
 		goto err_clk_put;
+	}
+
+	if (!simple->edev) {
+		ret = dwc3_of_simple_populate(simple);
+		if (ret)
+			goto err_clk_put;
+	} else {
+		/*
+		 * Populate through worker to avoid race conditions against
+		 * action scheduled through notifier.
+		 */
+		if (extcon_get_state(simple->edev, EXTCON_USB_HOST) > 0)
+			schedule_work(&simple->work);
+	}
 
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
@@ -112,7 +239,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
 
 static void __dwc3_of_simple_teardown(struct dwc3_of_simple *simple)
 {
-	of_platform_depopulate(simple->dev);
+	dwc3_of_simple_extcon_unregister(simple);
+	dwc3_of_simple_depopulate(simple);
 
 	clk_bulk_disable_unprepare(simple->num_clocks, simple->clks);
 	clk_bulk_put_all(simple->num_clocks, simple->clks);
@@ -163,6 +291,13 @@ static int __maybe_unused dwc3_of_simple_runtime_resume(struct device *dev)
 static int __maybe_unused dwc3_of_simple_suspend(struct device *dev)
 {
 	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
+	unsigned long flags;
+
+	spin_lock_irqsave(&simple->suspend_lock, flags);
+	simple->suspended = true;
+	spin_unlock_irqrestore(&simple->suspend_lock, flags);
+
+	cancel_work_sync(&simple->work);
 
 	if (simple->need_reset)
 		reset_control_assert(simple->resets);
@@ -173,10 +308,18 @@ static int __maybe_unused dwc3_of_simple_suspend(struct device *dev)
 static int __maybe_unused dwc3_of_simple_resume(struct device *dev)
 {
 	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
+	unsigned long flags;
 
 	if (simple->need_reset)
 		reset_control_deassert(simple->resets);
 
+	spin_lock_irqsave(&simple->suspend_lock, flags);
+	simple->suspended = false;
+	spin_unlock_irqrestore(&simple->suspend_lock, flags);
+
+	if (simple->edev)
+		schedule_work(&simple->work);
+
 	return 0;
 }
 
-- 
2.27.0.rc0.183.gde8f92d652-goog

Re: [RFC PATCH 1/1] usb: dwc3: of-simple: Add extcon support

[Guenter Roeck]

On Fri, May 22, 2020 at 09:12:02PM -0700, Prashant Malani wrote:
> Add optional extcon notifier support to enable the hotplug / unplug of
> the underlying PHY layer devices.
> 
> If supported, the Device Tree (DT) node for the device should include an
> "extcon" property which is a phandle to an extcon DT node.
> 
> This patch is an effort to incorporate the equivalent support from the
> Rockchip dwc3 driver implementation from Chrome OS [1] to the mainline.
> 
> [1] : https://chromium.googlesource.com/chromiumos/third_party/kernel/
> +/refs/heads/chromeos-4.4/drivers/usb/dwc3/dwc3-rockchip.c
> 
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Signed-off-by: Prashant Malani <pmalani@chromium.org>

Couple of nitpicks, otherwise

Reviewed-by: Guenter Roeck <groeck@chromium.org>

> ---
>  drivers/usb/dwc3/dwc3-of-simple.c | 149 +++++++++++++++++++++++++++++-
>  1 file changed, 146 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-of-simple.c b/drivers/usb/dwc3/dwc3-of-simple.c
> index e64754be47b4..28bde27cd1f9 100644
> --- a/drivers/usb/dwc3/dwc3-of-simple.c
> +++ b/drivers/usb/dwc3/dwc3-of-simple.c
> @@ -11,6 +11,7 @@
>   * by Subbaraya Sundeep Bhatta <subbaraya.sundeep.bhatta@xilinx.com>
>   */
>  
> +#include <linux/extcon.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
>  #include <linux/slab.h>
> @@ -29,8 +30,117 @@ struct dwc3_of_simple {
>  	struct reset_control	*resets;
>  	bool			pulse_resets;
>  	bool			need_reset;
> +	struct extcon_dev	*edev;
> +	struct notifier_block	nb;
> +	struct work_struct	work;
> +	/* Denotes whether child devices have been populated. */
> +	bool			populated;
> +	bool			suspended;
> +	spinlock_t		suspend_lock;
>  };
>  
> +static int dwc3_of_simple_populate(struct dwc3_of_simple *simple)
> +{
> +	struct device *dev = simple->dev;
> +	struct device_node *np = dev->of_node;
> +	int ret;
> +
> +	ret = of_platform_populate(np, NULL, NULL, dev);
> +	if (ret) {
> +		dev_err(dev, "Failed to populate dwc3 devices.\n");
> +		return ret;
> +	}
> +	simple->populated = true;
> +	return 0;
> +}
> +
> +static void dwc3_of_simple_depopulate(struct dwc3_of_simple *simple)
> +{
> +	if (simple->populated) {
> +		of_platform_depopulate(simple->dev);
> +		simple->populated = false;
> +	}
> +}
> +
> +static void dwc3_of_simple_work(struct work_struct *work)
> +{
> +	struct dwc3_of_simple *simple = container_of(work,
> +					struct dwc3_of_simple, work);
> +	struct extcon_dev *edev = simple->edev;
> +
> +	if (extcon_get_state(edev, EXTCON_USB_HOST) > 0) {
> +		if (simple->populated)
> +			return;
> +
> +		dwc3_of_simple_populate(simple);
> +	} else {
> +		if (!simple->populated)
> +			return;
> +
> +		dwc3_of_simple_depopulate(simple);
> +	}
> +}
> +
> +static int dwc3_of_simple_notifier(struct notifier_block *nb,
> +			      unsigned long event, void *ptr)

Multi-line alignment is off.

> +{
> +	struct dwc3_of_simple *simple = container_of(nb, struct dwc3_of_simple,
> +						nb);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&simple->suspend_lock, flags);
> +	if (!simple->suspended)
> +		schedule_work(&simple->work);
> +	spin_unlock_irqrestore(&simple->suspend_lock, flags);
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int dwc3_of_simple_extcon_register(struct dwc3_of_simple *simple)
> +{
> +	struct device		*dev = simple->dev;
> +	struct extcon_dev	*edev;
> +	int			ret;
> +
> +	edev = extcon_get_edev_by_phandle(dev, 0);
> +	if (IS_ERR(edev)) {
> +		/* The extcon property is optional. */
> +		if (PTR_ERR(edev) == -ENODEV)
> +			return 0;
> +		if (PTR_ERR(edev) != -EPROBE_DEFER)
> +			dev_err(dev, "Couldn't get extcon device.\n");
> +		return PTR_ERR(edev);
> +	}
> +
> +	INIT_WORK(&simple->work, dwc3_of_simple_work);
> +
> +	simple->nb.notifier_call = dwc3_of_simple_notifier;
> +	ret = devm_extcon_register_notifier(dev, edev, EXTCON_USB_HOST,
> +					&simple->nb);

Same here.

> +	if (ret < 0) {
> +		dev_err(dev, "Failed to register notifier.\n");
> +		return ret;
> +	}
> +
> +	simple->edev = edev;
> +
> +	return 0;
> +}
> +
> +static void dwc3_of_simple_extcon_unregister(struct dwc3_of_simple *simple)
> +{
> +	if (!simple->edev)
> +		return;
> +
> +	/*
> +	 * We explicitly unregister the notifier to prevent races with
> +	 * the of_depopulate() call in remove().
> +	 */
> +	devm_extcon_unregister_notifier(simple->dev, simple->edev,
> +					EXTCON_USB_HOST, &simple->nb);
> +	cancel_work_sync(&simple->work);
> +}
> +
>  static int dwc3_of_simple_probe(struct platform_device *pdev)
>  {
>  	struct dwc3_of_simple	*simple;
> @@ -47,6 +157,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, simple);
>  	simple->dev = dev;
>  
> +	spin_lock_init(&simple->suspend_lock);
> +
>  	/*
>  	 * Some controllers need to toggle the usb3-otg reset before trying to
>  	 * initialize the PHY, otherwise the PHY times out.
> @@ -87,9 +199,24 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_resetc_assert;
>  
> -	ret = of_platform_populate(np, NULL, NULL, dev);
> -	if (ret)
> +	ret = dwc3_of_simple_extcon_register(simple);
> +	if (ret < 0) {
> +		dev_warn(dev, "No extcon device found, err: %d\n", ret);
>  		goto err_clk_put;
> +	}
> +
> +	if (!simple->edev) {
> +		ret = dwc3_of_simple_populate(simple);
> +		if (ret)
> +			goto err_clk_put;
> +	} else {
> +		/*
> +		 * Populate through worker to avoid race conditions against
> +		 * action scheduled through notifier.
> +		 */
> +		if (extcon_get_state(simple->edev, EXTCON_USB_HOST) > 0)
> +			schedule_work(&simple->work);
> +	}
>  
>  	pm_runtime_set_active(dev);
>  	pm_runtime_enable(dev);
> @@ -112,7 +239,8 @@ static int dwc3_of_simple_probe(struct platform_device *pdev)
>  
>  static void __dwc3_of_simple_teardown(struct dwc3_of_simple *simple)
>  {
> -	of_platform_depopulate(simple->dev);
> +	dwc3_of_simple_extcon_unregister(simple);
> +	dwc3_of_simple_depopulate(simple);
>  
>  	clk_bulk_disable_unprepare(simple->num_clocks, simple->clks);
>  	clk_bulk_put_all(simple->num_clocks, simple->clks);
> @@ -163,6 +291,13 @@ static int __maybe_unused dwc3_of_simple_runtime_resume(struct device *dev)
>  static int __maybe_unused dwc3_of_simple_suspend(struct device *dev)
>  {
>  	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&simple->suspend_lock, flags);
> +	simple->suspended = true;
> +	spin_unlock_irqrestore(&simple->suspend_lock, flags);
> +
> +	cancel_work_sync(&simple->work);
>  
>  	if (simple->need_reset)
>  		reset_control_assert(simple->resets);
> @@ -173,10 +308,18 @@ static int __maybe_unused dwc3_of_simple_suspend(struct device *dev)
>  static int __maybe_unused dwc3_of_simple_resume(struct device *dev)
>  {
>  	struct dwc3_of_simple *simple = dev_get_drvdata(dev);
> +	unsigned long flags;
>  
>  	if (simple->need_reset)
>  		reset_control_deassert(simple->resets);
>  
> +	spin_lock_irqsave(&simple->suspend_lock, flags);
> +	simple->suspended = false;
> +	spin_unlock_irqrestore(&simple->suspend_lock, flags);
> +
> +	if (simple->edev)
> +		schedule_work(&simple->work);
> +
>  	return 0;
>  }
>