[PATCH 1/2] usb: dwc3: use local copy of resource to fix-up register offset

[PATCH 1/2] usb: dwc3: use local copy of resource to fix-up register offset

[Masahiro Yamada]

It is not a good idea to modify the resource from the platform device.
Modify its local copy to pass it to devm_ioremap_resource() so that we
do not need to restore it in the failure path and the remove hook.

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 drivers/usb/dwc3/core.c | 32 ++++++++------------------------
 1 file changed, 8 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index f1d838a..e9083a3 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1164,7 +1164,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
 static int dwc3_probe(struct platform_device *pdev)
 {
 	struct device		*dev = &pdev->dev;
-	struct resource		*res;
+	struct resource		*res, dwc_res;
 	struct dwc3		*dwc;
 
 	int			ret;
@@ -1189,20 +1189,19 @@ static int dwc3_probe(struct platform_device *pdev)
 	dwc->xhci_resources[0].flags = res->flags;
 	dwc->xhci_resources[0].name = res->name;
 
-	res->start += DWC3_GLOBALS_REGS_START;
-
 	/*
 	 * Request memory region but exclude xHCI regs,
 	 * since it will be requested by the xhci-plat driver.
 	 */
-	regs = devm_ioremap_resource(dev, res);
-	if (IS_ERR(regs)) {
-		ret = PTR_ERR(regs);
-		goto err0;
-	}
+	dwc_res = *res;
+	dwc_res.start += DWC3_GLOBALS_REGS_START;
+
+	regs = devm_ioremap_resource(dev, &dwc_res);
+	if (IS_ERR(regs))
+		return PTR_ERR(regs);
 
 	dwc->regs	= regs;
-	dwc->regs_size	= resource_size(res);
+	dwc->regs_size	= resource_size(&dwc_res);
 
 	dwc3_get_properties(dwc);
 
@@ -1269,29 +1268,14 @@ static int dwc3_probe(struct platform_device *pdev)
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
-err0:
-	/*
-	 * restore res->start back to its original value so that, in case the
-	 * probe is deferred, we don't end up getting error in request the
-	 * memory region the next time probe is called.
-	 */
-	res->start -= DWC3_GLOBALS_REGS_START;
-
 	return ret;
 }
 
 static int dwc3_remove(struct platform_device *pdev)
 {
 	struct dwc3	*dwc = platform_get_drvdata(pdev);
-	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 
 	pm_runtime_get_sync(&pdev->dev);
-	/*
-	 * restore res->start back to its original value so that, in case the
-	 * probe is deferred, we don't end up getting error in request the
-	 * memory region the next time probe is called.
-	 */
-	res->start -= DWC3_GLOBALS_REGS_START;
 
 	dwc3_debugfs_exit(dwc);
 	dwc3_core_exit_mode(dwc);
-- 
2.7.4

[PATCH 2/2] usb: dwc3: add clock and resets

[Masahiro Yamada]

dwc3-of-simple.c only handles arbitrary number of clocks and resets.
They are both generic enough to be put into the dwc3 core.  For simple
cases, a nested node structure like follows:

  dwc3-glue {
          compatible = "foo,dwc3";
          clocks = ...;
          resets = ...;
          ...

          dwc3 {
                  compatible = "snps,dwc3";
                  ...
          };
  }

would be turned into a single node:

  dwc3 {
          compatible = "foo,dwc3", "snps,dwc3";
          clocks = ...;
          resets = ...;
          ...
  }

I inserted reset_control_deassert() and clk_enable() before the first
register access, i.e. dwc3_cache_hwparams().

Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
---

 Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
 drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
 drivers/usb/dwc3/core.h                        |   5 +
 3 files changed, 132 insertions(+), 2 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
index 44e8bab..67e9cfb 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -9,12 +9,14 @@ Required properties:
  - interrupts: Interrupts used by the dwc3 controller.
 
 Optional properties:
+ - clocks: list of phandle and clock specifier pairs
  - usb-phy : array of phandle for the PHY device.  The first element
    in the array is expected to be a handle to the USB2/HS PHY and
    the second element is expected to be a handle to the USB3/SS PHY
  - phys: from the *Generic PHY* bindings
  - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
 	or "usb3-phy".
+ - resets: list of phandle and reset specifier pairs
  - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
  - snps,disable_scramble_quirk: true when SW should disable data scrambling.
 	Only really useful for FPGA builds.
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index e9083a3..f17e4a9 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -8,6 +8,7 @@
  *	    Sebastian Andrzej Siewior <bigeasy@linutronix.de>
  */
 
+#include <linux/clk.h>
 #include <linux/version.h>
 #include <linux/module.h>
 #include <linux/kernel.h>
@@ -24,6 +25,7 @@
 #include <linux/of.h>
 #include <linux/acpi.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/reset.h>
 
 #include <linux/usb/ch9.h>
 #include <linux/usb/gadget.h>
@@ -240,6 +242,74 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
 	return -ETIMEDOUT;
 }
 
+static int dwc3_core_get_clks(struct dwc3 *dwc)
+{
+	struct device *dev = dwc->dev;
+	struct device_node *node = dev->of_node;
+	struct clk *clk;
+	int num_clks, i;
+
+	num_clks = of_count_phandle_with_args(node, "clocks", "#clock-cells");
+	if (num_clks <= 0)
+		return 0;
+
+	dwc->num_clks = num_clks;
+
+	dwc->clks = devm_kcalloc(dev, num_clks, sizeof(*dwc->clks), GFP_KERNEL);
+	if (!dwc->clks)
+		return -ENOMEM;
+
+	for (i = 0; i < num_clks; i++) {
+		clk = of_clk_get(node, i);
+		if (IS_ERR(clk))
+			goto put_clks;
+		dwc->clks[i] = clk;
+	}
+
+	return 0;
+
+put_clks:
+	while (--i >= 0)
+		clk_put(dwc->clks[i]);
+
+	return PTR_ERR(clk);
+}
+
+static void dwc3_core_put_clks(struct dwc3 *dwc)
+{
+	int i;
+
+	for (i = dwc->num_clks - 1; i >= 0; i--)
+		clk_put(dwc->clks[i]);
+}
+
+static int dwc3_core_enable_clks(struct dwc3 *dwc)
+{
+	int ret, i;
+
+	for (i = 0; i < dwc->num_clks; i++) {
+		ret = clk_prepare_enable(dwc->clks[i]);
+		if (ret)
+			goto disable_clks;
+	}
+
+	return 0;
+
+disable_clks:
+	while (--i >= 0)
+		clk_disable_unprepare(dwc->clks[i]);
+
+	return ret;
+}
+
+static void dwc3_core_disable_clks(struct dwc3 *dwc)
+{
+	int i;
+
+	for (i = dwc->num_clks - 1; i >= 0; i--)
+		clk_disable_unprepare(dwc->clks[i]);
+}
+
 /*
  * dwc3_frame_length_adjustment - Adjusts frame length if required
  * @dwc3: Pointer to our controller context structure
@@ -641,6 +711,8 @@ static void dwc3_core_exit(struct dwc3 *dwc)
 	usb_phy_set_suspend(dwc->usb3_phy, 1);
 	phy_power_off(dwc->usb2_generic_phy);
 	phy_power_off(dwc->usb3_generic_phy);
+	dwc3_core_disable_clks(dwc);
+	reset_control_assert(dwc->resets);
 }
 
 static bool dwc3_core_is_valid(struct dwc3 *dwc)
@@ -1205,6 +1277,22 @@ static int dwc3_probe(struct platform_device *pdev)
 
 	dwc3_get_properties(dwc);
 
+	dwc->resets = devm_reset_control_array_get_optional_shared(dev);
+	if (IS_ERR(dwc->resets))
+		return PTR_ERR(dwc->resets);
+
+	ret = dwc3_core_get_clks(dwc);
+	if (ret)
+		return ret;
+
+	ret = reset_control_deassert(dwc->resets);
+	if (ret)
+		goto put_clks;
+
+	ret = dwc3_core_enable_clks(dwc);
+	if (ret)
+		goto assert_resets;
+
 	platform_set_drvdata(pdev, dwc);
 	dwc3_cache_hwparams(dwc);
 
@@ -1268,6 +1356,14 @@ static int dwc3_probe(struct platform_device *pdev)
 	pm_runtime_put_sync(&pdev->dev);
 	pm_runtime_disable(&pdev->dev);
 
+	dwc3_core_disable_clks(dwc);
+
+assert_resets:
+	reset_control_assert(dwc->resets);
+
+put_clks:
+	dwc3_core_put_clks(dwc);
+
 	return ret;
 }
 
@@ -1289,11 +1385,38 @@ static int dwc3_remove(struct platform_device *pdev)
 
 	dwc3_free_event_buffers(dwc);
 	dwc3_free_scratch_buffers(dwc);
+	dwc3_core_put_clks(dwc);
 
 	return 0;
 }
 
 #ifdef CONFIG_PM
+static int dwc3_core_init_for_resume(struct dwc3 *dwc)
+{
+	int ret;
+
+	ret = reset_control_deassert(dwc->resets);
+	if (ret)
+		return ret;
+
+	ret = dwc3_core_enable_clks(dwc);
+	if (ret)
+		goto assert_resets;
+
+	ret = dwc3_core_init(dwc);
+	if (ret)
+		goto disable_clks;
+
+	return 0;
+
+disable_clks:
+	dwc3_core_disable_clks(dwc);
+assert_resets:
+	reset_control_assert(dwc->resets);
+
+	return ret;
+}
+
 static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
 {
 	unsigned long	flags;
@@ -1325,7 +1448,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 
 	switch (dwc->current_dr_role) {
 	case DWC3_GCTL_PRTCAP_DEVICE:
-		ret = dwc3_core_init(dwc);
+		ret = dwc3_core_init_for_resume(dwc);
 		if (ret)
 			return ret;
 
@@ -1336,7 +1459,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
 	case DWC3_GCTL_PRTCAP_HOST:
 		/* nothing to do on host runtime_resume */
 		if (!PMSG_IS_AUTO(msg)) {
-			ret = dwc3_core_init(dwc);
+			ret = dwc3_core_init_for_resume(dwc);
 			if (ret)
 				return ret;
 		}
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 860d2bc..14cd335 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -891,6 +891,11 @@ struct dwc3 {
 	struct usb_gadget	gadget;
 	struct usb_gadget_driver *gadget_driver;
 
+	struct clk		**clks;
+	int			num_clks;
+
+	struct reset_control	*resets;
+
 	struct usb_phy		*usb2_phy;
 	struct usb_phy		*usb3_phy;
 
-- 
2.7.4

Re: [PATCH 1/2] usb: dwc3: use local copy of resource to fix-up register offset

[Masami Hiramatsu]

On Thu, 15 Mar 2018 20:39:57 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> It is not a good idea to modify the resource from the platform device.
> Modify its local copy to pass it to devm_ioremap_resource() so that we
> do not need to restore it in the failure path and the remove hook.
> 

This looks good to me.

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks,

> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  drivers/usb/dwc3/core.c | 32 ++++++++------------------------
>  1 file changed, 8 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index f1d838a..e9083a3 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1164,7 +1164,7 @@ static void dwc3_check_params(struct dwc3 *dwc)
>  static int dwc3_probe(struct platform_device *pdev)
>  {
>  	struct device		*dev = &pdev->dev;
> -	struct resource		*res;
> +	struct resource		*res, dwc_res;
>  	struct dwc3		*dwc;
>  
>  	int			ret;
> @@ -1189,20 +1189,19 @@ static int dwc3_probe(struct platform_device *pdev)
>  	dwc->xhci_resources[0].flags = res->flags;
>  	dwc->xhci_resources[0].name = res->name;
>  
> -	res->start += DWC3_GLOBALS_REGS_START;
> -
>  	/*
>  	 * Request memory region but exclude xHCI regs,
>  	 * since it will be requested by the xhci-plat driver.
>  	 */
> -	regs = devm_ioremap_resource(dev, res);
> -	if (IS_ERR(regs)) {
> -		ret = PTR_ERR(regs);
> -		goto err0;
> -	}
> +	dwc_res = *res;
> +	dwc_res.start += DWC3_GLOBALS_REGS_START;
> +
> +	regs = devm_ioremap_resource(dev, &dwc_res);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
>  
>  	dwc->regs	= regs;
> -	dwc->regs_size	= resource_size(res);
> +	dwc->regs_size	= resource_size(&dwc_res);
>  
>  	dwc3_get_properties(dwc);
>  
> @@ -1269,29 +1268,14 @@ static int dwc3_probe(struct platform_device *pdev)
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> -err0:
> -	/*
> -	 * restore res->start back to its original value so that, in case the
> -	 * probe is deferred, we don't end up getting error in request the
> -	 * memory region the next time probe is called.
> -	 */
> -	res->start -= DWC3_GLOBALS_REGS_START;
> -
>  	return ret;
>  }
>  
>  static int dwc3_remove(struct platform_device *pdev)
>  {
>  	struct dwc3	*dwc = platform_get_drvdata(pdev);
> -	struct resource *res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>  
>  	pm_runtime_get_sync(&pdev->dev);
> -	/*
> -	 * restore res->start back to its original value so that, in case the
> -	 * probe is deferred, we don't end up getting error in request the
> -	 * memory region the next time probe is called.
> -	 */
> -	res->start -= DWC3_GLOBALS_REGS_START;
>  
>  	dwc3_debugfs_exit(dwc);
>  	dwc3_core_exit_mode(dwc);
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Masami Hiramatsu]

On Thu, 15 Mar 2018 20:39:58 +0900
Masahiro Yamada <yamada.masahiro@socionext.com> wrote:

> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
> They are both generic enough to be put into the dwc3 core.  For simple
> cases, a nested node structure like follows:
> 
>   dwc3-glue {
>           compatible = "foo,dwc3";
>           clocks = ...;
>           resets = ...;
>           ...
> 
>           dwc3 {
>                   compatible = "snps,dwc3";
>                   ...
>           };
>   }
> 
> would be turned into a single node:
> 
>   dwc3 {
>           compatible = "foo,dwc3", "snps,dwc3";
>           clocks = ...;
>           resets = ...;
>           ...
>   }
> 
> I inserted reset_control_deassert() and clk_enable() before the first
> register access, i.e. dwc3_cache_hwparams().

This looks good to me. 

Reviewed-by: Masami Hiramatsu <mhiramat@kernel.org>

Thanks!

> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h                        |   5 +
>  3 files changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 44e8bab..67e9cfb 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -9,12 +9,14 @@ Required properties:
>   - interrupts: Interrupts used by the dwc3 controller.
>  
>  Optional properties:
> + - clocks: list of phandle and clock specifier pairs
>   - usb-phy : array of phandle for the PHY device.  The first element
>     in the array is expected to be a handle to the USB2/HS PHY and
>     the second element is expected to be a handle to the USB3/SS PHY
>   - phys: from the *Generic PHY* bindings
>   - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
>  	or "usb3-phy".
> + - resets: list of phandle and reset specifier pairs
>   - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
>   - snps,disable_scramble_quirk: true when SW should disable data scrambling.
>  	Only really useful for FPGA builds.
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e9083a3..f17e4a9 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -8,6 +8,7 @@
>   *	    Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/version.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> @@ -24,6 +25,7 @@
>  #include <linux/of.h>
>  #include <linux/acpi.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/reset.h>
>  
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> @@ -240,6 +242,74 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	return -ETIMEDOUT;
>  }
>  
> +static int dwc3_core_get_clks(struct dwc3 *dwc)
> +{
> +	struct device *dev = dwc->dev;
> +	struct device_node *node = dev->of_node;
> +	struct clk *clk;
> +	int num_clks, i;
> +
> +	num_clks = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +	if (num_clks <= 0)
> +		return 0;
> +
> +	dwc->num_clks = num_clks;
> +
> +	dwc->clks = devm_kcalloc(dev, num_clks, sizeof(*dwc->clks), GFP_KERNEL);
> +	if (!dwc->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_clks; i++) {
> +		clk = of_clk_get(node, i);
> +		if (IS_ERR(clk))
> +			goto put_clks;
> +		dwc->clks[i] = clk;
> +	}
> +
> +	return 0;
> +
> +put_clks:
> +	while (--i >= 0)
> +		clk_put(dwc->clks[i]);
> +
> +	return PTR_ERR(clk);
> +}
> +
> +static void dwc3_core_put_clks(struct dwc3 *dwc)
> +{
> +	int i;
> +
> +	for (i = dwc->num_clks - 1; i >= 0; i--)
> +		clk_put(dwc->clks[i]);
> +}
> +
> +static int dwc3_core_enable_clks(struct dwc3 *dwc)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < dwc->num_clks; i++) {
> +		ret = clk_prepare_enable(dwc->clks[i]);
> +		if (ret)
> +			goto disable_clks;
> +	}
> +
> +	return 0;
> +
> +disable_clks:
> +	while (--i >= 0)
> +		clk_disable_unprepare(dwc->clks[i]);
> +
> +	return ret;
> +}
> +
> +static void dwc3_core_disable_clks(struct dwc3 *dwc)
> +{
> +	int i;
> +
> +	for (i = dwc->num_clks - 1; i >= 0; i--)
> +		clk_disable_unprepare(dwc->clks[i]);
> +}
> +
>  /*
>   * dwc3_frame_length_adjustment - Adjusts frame length if required
>   * @dwc3: Pointer to our controller context structure
> @@ -641,6 +711,8 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
> +	dwc3_core_disable_clks(dwc);
> +	reset_control_assert(dwc->resets);
>  }
>  
>  static bool dwc3_core_is_valid(struct dwc3 *dwc)
> @@ -1205,6 +1277,22 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	dwc3_get_properties(dwc);
>  
> +	dwc->resets = devm_reset_control_array_get_optional_shared(dev);
> +	if (IS_ERR(dwc->resets))
> +		return PTR_ERR(dwc->resets);
> +
> +	ret = dwc3_core_get_clks(dwc);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(dwc->resets);
> +	if (ret)
> +		goto put_clks;
> +
> +	ret = dwc3_core_enable_clks(dwc);
> +	if (ret)
> +		goto assert_resets;
> +
>  	platform_set_drvdata(pdev, dwc);
>  	dwc3_cache_hwparams(dwc);
>  
> @@ -1268,6 +1356,14 @@ static int dwc3_probe(struct platform_device *pdev)
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	dwc3_core_disable_clks(dwc);
> +
> +assert_resets:
> +	reset_control_assert(dwc->resets);
> +
> +put_clks:
> +	dwc3_core_put_clks(dwc);
> +
>  	return ret;
>  }
>  
> @@ -1289,11 +1385,38 @@ static int dwc3_remove(struct platform_device *pdev)
>  
>  	dwc3_free_event_buffers(dwc);
>  	dwc3_free_scratch_buffers(dwc);
> +	dwc3_core_put_clks(dwc);
>  
>  	return 0;
>  }
>  
>  #ifdef CONFIG_PM
> +static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(dwc->resets);
> +	if (ret)
> +		return ret;
> +
> +	ret = dwc3_core_enable_clks(dwc);
> +	if (ret)
> +		goto assert_resets;
> +
> +	ret = dwc3_core_init(dwc);
> +	if (ret)
> +		goto disable_clks;
> +
> +	return 0;
> +
> +disable_clks:
> +	dwc3_core_disable_clks(dwc);
> +assert_resets:
> +	reset_control_assert(dwc->resets);
> +
> +	return ret;
> +}
> +
>  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
> @@ -1325,7 +1448,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> -		ret = dwc3_core_init(dwc);
> +		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
>  
> @@ -1336,7 +1459,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		/* nothing to do on host runtime_resume */
>  		if (!PMSG_IS_AUTO(msg)) {
> -			ret = dwc3_core_init(dwc);
> +			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 860d2bc..14cd335 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -891,6 +891,11 @@ struct dwc3 {
>  	struct usb_gadget	gadget;
>  	struct usb_gadget_driver *gadget_driver;
>  
> +	struct clk		**clks;
> +	int			num_clks;
> +
> +	struct reset_control	*resets;
> +
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> -- 
> 2.7.4
> 


-- 
Masami Hiramatsu <mhiramat@kernel.org>

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[kbuild test robot]

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

Hi Masahiro,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180316]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Masahiro-Yamada/usb-dwc3-use-local-copy-of-resource-to-fix-up-register-offset/20180317-011715
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   WARNING: convert(1) not found, for SVG to PDF conversion install ImageMagick (https://www.imagemagick.org)
   include/linux/crypto.h:477: warning: Function parameter or member 'cra_u.ablkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:477: warning: Function parameter or member 'cra_u.blkcipher' not described in 'crypto_alg'
   include/linux/crypto.h:477: warning: Function parameter or member 'cra_u.cipher' not described in 'crypto_alg'
   include/linux/crypto.h:477: warning: Function parameter or member 'cra_u.compress' not described in 'crypto_alg'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.ibss' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.connect' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.keys' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.ie' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.ie_len' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.bssid' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.prev_bssid' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.ssid' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.default_key' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.default_mgmt_key' not described in 'wireless_dev'
   include/net/cfg80211.h:4129: warning: Function parameter or member 'wext.prev_bssid_valid' not described in 'wireless_dev'
   include/net/mac80211.h:2259: warning: Function parameter or member 'radiotap_timestamp.units_pos' not described in 'ieee80211_hw'
   include/net/mac80211.h:2259: warning: Function parameter or member 'radiotap_timestamp.accuracy' not described in 'ieee80211_hw'
   include/net/mac80211.h:950: warning: Function parameter or member 'rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.rts_cts_rate_idx' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.use_rts' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.use_cts_prot' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.short_preamble' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.skip_table' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.jiffies' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.vif' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.hw_key' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.flags' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'control.enqueue_time' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'ack' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'ack.cookie' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.ack_signal' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'status_driver_data' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:950: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'msdu_retries' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'msdu_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
   net/mac80211/sta_info.h:584: warning: Function parameter or member 'msdu' not described in 'sta_info'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:307: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
   include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
   include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.sign' not described in 'iio_chan_spec'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.realbits' not described in 'iio_chan_spec'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.storagebits' not described in 'iio_chan_spec'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.shift' not described in 'iio_chan_spec'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.repeat' not described in 'iio_chan_spec'
   include/linux/iio/iio.h:270: warning: Function parameter or member 'scan_type.endianness' not described in 'iio_chan_spec'
   include/linux/iio/hw-consumer.h:1: warning: no structured comments found
   include/linux/device.h:294: warning: Function parameter or member 'coredump' not described in 'device_driver'
   include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
   include/linux/mtd/rawnand.h:709: warning: Function parameter or member 'timings.sdr' not described in 'nand_data_interface'
   include/linux/mtd/rawnand.h:774: warning: Function parameter or member 'buf' not described in 'nand_op_data_instr'
   include/linux/mtd/rawnand.h:774: warning: Function parameter or member 'buf.in' not described in 'nand_op_data_instr'
   include/linux/mtd/rawnand.h:774: warning: Function parameter or member 'buf.out' not described in 'nand_op_data_instr'
   include/linux/mtd/rawnand.h:820: warning: Function parameter or member 'ctx' not described in 'nand_op_instr'
   include/linux/mtd/rawnand.h:820: warning: Function parameter or member 'ctx.cmd' not described in 'nand_op_instr'
   include/linux/mtd/rawnand.h:820: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_instr'
   include/linux/mtd/rawnand.h:820: warning: Function parameter or member 'ctx.data' not described in 'nand_op_instr'
   include/linux/mtd/rawnand.h:820: warning: Function parameter or member 'ctx.waitrdy' not described in 'nand_op_instr'
   include/linux/mtd/rawnand.h:967: warning: Function parameter or member 'ctx' not described in 'nand_op_parser_pattern_elem'
   include/linux/mtd/rawnand.h:967: warning: Function parameter or member 'ctx.addr' not described in 'nand_op_parser_pattern_elem'
   include/linux/mtd/rawnand.h:967: warning: Function parameter or member 'ctx.data' not described in 'nand_op_parser_pattern_elem'
   include/linux/mtd/rawnand.h:1281: warning: Function parameter or member 'manufacturer.desc' not described in 'nand_chip'
   include/linux/mtd/rawnand.h:1281: warning: Function parameter or member 'manufacturer.priv' not described in 'nand_chip'
   include/linux/regulator/driver.h:221: warning: Function parameter or member 'resume_early' not described in 'regulator_ops'
   drivers/regulator/core.c:4299: warning: Excess function parameter 'state' description in 'regulator_suspend_late'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
>> drivers/usb/dwc3/core.h:1028: warning: Function parameter or member 'clks' not described in 'dwc3'
>> drivers/usb/dwc3/core.h:1028: warning: Function parameter or member 'num_clks' not described in 'dwc3'
>> drivers/usb/dwc3/core.h:1028: warning: Function parameter or member 'resets' not described in 'dwc3'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
   include/drm/drm_connector.h:370: warning: Function parameter or member 'margins.left' not described in 'drm_tv_connector_state'
   include/drm/drm_connector.h:370: warning: Function parameter or member 'margins.right' not described in 'drm_tv_connector_state'
   include/drm/drm_connector.h:370: warning: Function parameter or member 'margins.top' not described in 'drm_tv_connector_state'
   include/drm/drm_connector.h:370: warning: Function parameter or member 'margins.bottom' not described in 'drm_tv_connector_state'
   include/drm/drm_vblank.h:63: warning: Function parameter or member 'event.base' not described in 'drm_pending_vblank_event'
   include/drm/drm_vblank.h:63: warning: Function parameter or member 'event.vbl' not described in 'drm_pending_vblank_event'
   include/drm/drm_vblank.h:63: warning: Function parameter or member 'event.seq' not described in 'drm_pending_vblank_event'
   include/linux/skbuff.h:846: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'skb_mstamp' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member '__unused' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'pfmemalloc' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:846: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:234: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:234: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:487: warning: Function parameter or member 'sk_backlog.rmem_alloc' not described in 'sock'
   include/net/sock.h:487: warning: Function parameter or member 'sk_backlog.len' not described in 'sock'
   include/net/sock.h:487: warning: Function parameter or member 'sk_backlog.head' not described in 'sock'
   include/net/sock.h:487: warning: Function parameter or member 'sk_backlog.tail' not described in 'sock'
   include/net/sock.h:487: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:487: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'adj_list.upper' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'adj_list.lower' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'switchdev_ops' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:1940: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   include/linux/rcupdate.h:570: ERROR: Unexpected indentation.
   include/linux/rcupdate.h:574: ERROR: Unexpected indentation.
   include/linux/rcupdate.h:578: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/rcupdate.h:580: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/rcupdate.h:580: WARNING: Inline literal start-string without end-string.
   kernel/time/timer.c:1259: ERROR: Unexpected indentation.
   kernel/time/timer.c:1261: ERROR: Unexpected indentation.
   kernel/time/timer.c:1262: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:110: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/wait.h:113: ERROR: Unexpected indentation.
   include/linux/wait.h:115: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/time/hrtimer.c:1113: WARNING: Block quote ends without a blank line; unexpected unindent.
   kernel/signal.c:327: WARNING: Inline literal start-string without end-string.
   drivers/video/fbdev/core/modedb.c:647: WARNING: Inline strong start-string without end-string.
   drivers/video/fbdev/core/modedb.c:647: WARNING: Inline strong start-string without end-string.
   drivers/video/fbdev/core/modedb.c:647: WARNING: Inline strong start-string without end-string.
   drivers/video/fbdev/core/modedb.c:647: WARNING: Inline strong start-string without end-string.
   include/linux/iio/iio.h:191: ERROR: Unexpected indentation.
   include/linux/iio/iio.h:192: WARNING: Block quote ends without a blank line; unexpected unindent.
   include/linux/iio/iio.h:198: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/ata/libata-core.c:5920: ERROR: Unknown target name: "hw".
   drivers/message/fusion/mptbase.c:5052: WARNING: Definition list ends without a blank line; unexpected unindent.
   drivers/tty/serial/serial_core.c:1901: WARNING: Definition list ends without a blank line; unexpected unindent.
   include/linux/mtd/rawnand.h:805: ERROR: Unexpected indentation.
   include/linux/mtd/rawnand.h:1391: WARNING: Inline strong start-string without end-string.
   include/linux/mtd/rawnand.h:1393: WARNING: Inline strong start-string without end-string.
   include/linux/regulator/driver.h:273: ERROR: Unknown target name: "regulator_regmap_x_voltage".
   Documentation/driver-api/slimbus.rst:93: WARNING: Title underline too short.

vim +1028 drivers/usb/dwc3/core.h

72246da4 Felipe Balbi 2011-08-19 @1028  

:::::: The code at line 1028 was first introduced by commit
:::::: 72246da40f3719af3bfd104a2365b32537c27d83 usb: Introduce DesignWare USB3 DRD Driver

:::::: TO: Felipe Balbi <balbi@ti.com>
:::::: CC: Greg Kroah-Hartman <gregkh@suse.de>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6801 bytes --]

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Rob Herring]

On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
> They are both generic enough to be put into the dwc3 core.  For simple
> cases, a nested node structure like follows:
> 
>   dwc3-glue {
>           compatible = "foo,dwc3";
>           clocks = ...;
>           resets = ...;
>           ...
> 
>           dwc3 {
>                   compatible = "snps,dwc3";
>                   ...
>           };

I'm not a fan of how this was done.


>   }
> 
> would be turned into a single node:
> 
>   dwc3 {
>           compatible = "foo,dwc3", "snps,dwc3";
>           clocks = ...;
>           resets = ...;
>           ...
>   }

And yes, this is what I'd prefer.

> 
> I inserted reset_control_deassert() and clk_enable() before the first
> register access, i.e. dwc3_cache_hwparams().
> 
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> ---
> 
>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>  drivers/usb/dwc3/core.h                        |   5 +
>  3 files changed, 132 insertions(+), 2 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
> index 44e8bab..67e9cfb 100644
> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
> @@ -9,12 +9,14 @@ Required properties:
>   - interrupts: Interrupts used by the dwc3 controller.
>  
>  Optional properties:
> + - clocks: list of phandle and clock specifier pairs

However, this should be specific as to how many clocks and their 
function. This should be readily available to someone with access to 
Synopsys datasheet. The number of clocks should generally be the same 
across SoCs, it is just some SoCs either tie clocks together or don't 
provide s/w control of some of the clocks.

>   - usb-phy : array of phandle for the PHY device.  The first element
>     in the array is expected to be a handle to the USB2/HS PHY and
>     the second element is expected to be a handle to the USB3/SS PHY
>   - phys: from the *Generic PHY* bindings
>   - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
>  	or "usb3-phy".
> + - resets: list of phandle and reset specifier pairs

ditto

>   - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable
>   - snps,disable_scramble_quirk: true when SW should disable data scrambling.
>  	Only really useful for FPGA builds.
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index e9083a3..f17e4a9 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -8,6 +8,7 @@
>   *	    Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>   */
>  
> +#include <linux/clk.h>
>  #include <linux/version.h>
>  #include <linux/module.h>
>  #include <linux/kernel.h>
> @@ -24,6 +25,7 @@
>  #include <linux/of.h>
>  #include <linux/acpi.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/reset.h>
>  
>  #include <linux/usb/ch9.h>
>  #include <linux/usb/gadget.h>
> @@ -240,6 +242,74 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>  	return -ETIMEDOUT;
>  }
>  
> +static int dwc3_core_get_clks(struct dwc3 *dwc)
> +{
> +	struct device *dev = dwc->dev;
> +	struct device_node *node = dev->of_node;
> +	struct clk *clk;
> +	int num_clks, i;
> +
> +	num_clks = of_count_phandle_with_args(node, "clocks", "#clock-cells");
> +	if (num_clks <= 0)
> +		return 0;
> +
> +	dwc->num_clks = num_clks;
> +
> +	dwc->clks = devm_kcalloc(dev, num_clks, sizeof(*dwc->clks), GFP_KERNEL);
> +	if (!dwc->clks)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < num_clks; i++) {
> +		clk = of_clk_get(node, i);
> +		if (IS_ERR(clk))
> +			goto put_clks;
> +		dwc->clks[i] = clk;
> +	}
> +
> +	return 0;
> +
> +put_clks:
> +	while (--i >= 0)
> +		clk_put(dwc->clks[i]);
> +
> +	return PTR_ERR(clk);
> +}
> +
> +static void dwc3_core_put_clks(struct dwc3 *dwc)
> +{
> +	int i;
> +
> +	for (i = dwc->num_clks - 1; i >= 0; i--)
> +		clk_put(dwc->clks[i]);
> +}
> +
> +static int dwc3_core_enable_clks(struct dwc3 *dwc)
> +{
> +	int ret, i;
> +
> +	for (i = 0; i < dwc->num_clks; i++) {
> +		ret = clk_prepare_enable(dwc->clks[i]);
> +		if (ret)
> +			goto disable_clks;
> +	}
> +
> +	return 0;
> +
> +disable_clks:
> +	while (--i >= 0)
> +		clk_disable_unprepare(dwc->clks[i]);
> +
> +	return ret;
> +}
> +
> +static void dwc3_core_disable_clks(struct dwc3 *dwc)
> +{
> +	int i;
> +
> +	for (i = dwc->num_clks - 1; i >= 0; i--)
> +		clk_disable_unprepare(dwc->clks[i]);
> +}
> +
>  /*
>   * dwc3_frame_length_adjustment - Adjusts frame length if required
>   * @dwc3: Pointer to our controller context structure
> @@ -641,6 +711,8 @@ static void dwc3_core_exit(struct dwc3 *dwc)
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
>  	phy_power_off(dwc->usb2_generic_phy);
>  	phy_power_off(dwc->usb3_generic_phy);
> +	dwc3_core_disable_clks(dwc);
> +	reset_control_assert(dwc->resets);
>  }
>  
>  static bool dwc3_core_is_valid(struct dwc3 *dwc)
> @@ -1205,6 +1277,22 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  	dwc3_get_properties(dwc);
>  
> +	dwc->resets = devm_reset_control_array_get_optional_shared(dev);
> +	if (IS_ERR(dwc->resets))
> +		return PTR_ERR(dwc->resets);
> +
> +	ret = dwc3_core_get_clks(dwc);
> +	if (ret)
> +		return ret;
> +
> +	ret = reset_control_deassert(dwc->resets);
> +	if (ret)
> +		goto put_clks;
> +
> +	ret = dwc3_core_enable_clks(dwc);
> +	if (ret)
> +		goto assert_resets;
> +
>  	platform_set_drvdata(pdev, dwc);
>  	dwc3_cache_hwparams(dwc);
>  
> @@ -1268,6 +1356,14 @@ static int dwc3_probe(struct platform_device *pdev)
>  	pm_runtime_put_sync(&pdev->dev);
>  	pm_runtime_disable(&pdev->dev);
>  
> +	dwc3_core_disable_clks(dwc);
> +
> +assert_resets:
> +	reset_control_assert(dwc->resets);
> +
> +put_clks:
> +	dwc3_core_put_clks(dwc);
> +
>  	return ret;
>  }
>  
> @@ -1289,11 +1385,38 @@ static int dwc3_remove(struct platform_device *pdev)
>  
>  	dwc3_free_event_buffers(dwc);
>  	dwc3_free_scratch_buffers(dwc);
> +	dwc3_core_put_clks(dwc);
>  
>  	return 0;
>  }
>  
>  #ifdef CONFIG_PM
> +static int dwc3_core_init_for_resume(struct dwc3 *dwc)
> +{
> +	int ret;
> +
> +	ret = reset_control_deassert(dwc->resets);
> +	if (ret)
> +		return ret;
> +
> +	ret = dwc3_core_enable_clks(dwc);
> +	if (ret)
> +		goto assert_resets;
> +
> +	ret = dwc3_core_init(dwc);
> +	if (ret)
> +		goto disable_clks;
> +
> +	return 0;
> +
> +disable_clks:
> +	dwc3_core_disable_clks(dwc);
> +assert_resets:
> +	reset_control_assert(dwc->resets);
> +
> +	return ret;
> +}
> +
>  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
> @@ -1325,7 +1448,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  
>  	switch (dwc->current_dr_role) {
>  	case DWC3_GCTL_PRTCAP_DEVICE:
> -		ret = dwc3_core_init(dwc);
> +		ret = dwc3_core_init_for_resume(dwc);
>  		if (ret)
>  			return ret;
>  
> @@ -1336,7 +1459,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  	case DWC3_GCTL_PRTCAP_HOST:
>  		/* nothing to do on host runtime_resume */
>  		if (!PMSG_IS_AUTO(msg)) {
> -			ret = dwc3_core_init(dwc);
> +			ret = dwc3_core_init_for_resume(dwc);
>  			if (ret)
>  				return ret;
>  		}
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 860d2bc..14cd335 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -891,6 +891,11 @@ struct dwc3 {
>  	struct usb_gadget	gadget;
>  	struct usb_gadget_driver *gadget_driver;
>  
> +	struct clk		**clks;
> +	int			num_clks;
> +
> +	struct reset_control	*resets;
> +
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> -- 
> 2.7.4
> 

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Masahiro Yamada]

Hi Rob,

2018-03-18 21:52 GMT+09:00 Rob Herring <robh@kernel.org>:
> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>> They are both generic enough to be put into the dwc3 core.  For simple
>> cases, a nested node structure like follows:
>>
>>   dwc3-glue {
>>           compatible = "foo,dwc3";
>>           clocks = ...;
>>           resets = ...;
>>           ...
>>
>>           dwc3 {
>>                   compatible = "snps,dwc3";
>>                   ...
>>           };
>
> I'm not a fan of how this was done.
>
>
>>   }
>>
>> would be turned into a single node:
>>
>>   dwc3 {
>>           compatible = "foo,dwc3", "snps,dwc3";
>>           clocks = ...;
>>           resets = ...;
>>           ...
>>   }
>
> And yes, this is what I'd prefer.



Not only dwc3-of-simple.c, but all dwc3 nodes are
written like this.


omap_dwc3_1: omap_dwc3_1@48880000 {
        compatible = "ti,dwc3";
        reg = <0x48880000 0x10000>;
        #address-cells = <1>;
        #size-cells = <1>;
        ranges;
        ...

        usb1: usb@48890000 {
                compatible = "snps,dwc3";
                reg = <0x48890000 0x17000>;
                ...
        };
};


The glue layer initializes SoC-specific things,
then populates the child "snps,dwc3".


I think the following structure should work
by handling EPROBE_DEFER properly.

omap_dwc3_1: omap_dwc3_1@48880000 {
        compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
        reg = <0x48880000 0x10000>;
        ...
};

usb1: usb@48890000 {
        compatible = "snps,dwc3";
        reg = <0x48890000 0x17000>;
        ...
};



>>
>> I inserted reset_control_deassert() and clk_enable() before the first
>> register access, i.e. dwc3_cache_hwparams().
>>
>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>> ---
>>
>>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>>  drivers/usb/dwc3/core.h                        |   5 +
>>  3 files changed, 132 insertions(+), 2 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 44e8bab..67e9cfb 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -9,12 +9,14 @@ Required properties:
>>   - interrupts: Interrupts used by the dwc3 controller.
>>
>>  Optional properties:
>> + - clocks: list of phandle and clock specifier pairs
>
> However, this should be specific as to how many clocks and their
> function. This should be readily available to someone with access to
> Synopsys datasheet. The number of clocks should generally be the same
> across SoCs, it is just some SoCs either tie clocks together or don't
> provide s/w control of some of the clocks.


Make sense.
You also implies this property is mandatory.
The number of clocks should be available in the datasheet
and no hardware can work with zero clock.

However, making it mandatory breaks the binding
since the existing DT files do not specify clocks at all
in the "snps,dwc3" node.



Anyway, our current situation:

- We have the dwc3-core under the glue layer node
  despite they are independent in the CPU address view
- We add all sorts of clocks and resets in the glue layer node,
  and nothing in the dwc3-core node.

If these are design mistake, what should we do?

Continue development based on it?
If we fix it, how to change the course?





>>   - usb-phy : array of phandle for the PHY device.  The first element
>>     in the array is expected to be a handle to the USB2/HS PHY and
>>     the second element is expected to be a handle to the USB3/SS PHY
>>   - phys: from the *Generic PHY* bindings
>>   - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy"
>>       or "usb3-phy".
>> + - resets: list of phandle and reset specifier pairs
>
> ditto
>

Same issue as the clocks.



-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Masahiro Yamada]

2018-03-19 7:37 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
> Hi Rob,
>
> 2018-03-18 21:52 GMT+09:00 Rob Herring <robh@kernel.org>:
>> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>>> They are both generic enough to be put into the dwc3 core.  For simple
>>> cases, a nested node structure like follows:
>>>
>>>   dwc3-glue {
>>>           compatible = "foo,dwc3";
>>>           clocks = ...;
>>>           resets = ...;
>>>           ...
>>>
>>>           dwc3 {
>>>                   compatible = "snps,dwc3";
>>>                   ...
>>>           };
>>
>> I'm not a fan of how this was done.
>>
>>
>>>   }
>>>
>>> would be turned into a single node:
>>>
>>>   dwc3 {
>>>           compatible = "foo,dwc3", "snps,dwc3";
>>>           clocks = ...;
>>>           resets = ...;
>>>           ...
>>>   }
>>
>> And yes, this is what I'd prefer.
>
>
>
> Not only dwc3-of-simple.c, but all dwc3 nodes are
> written like this.
>
>
> omap_dwc3_1: omap_dwc3_1@48880000 {
>         compatible = "ti,dwc3";
>         reg = <0x48880000 0x10000>;
>         #address-cells = <1>;
>         #size-cells = <1>;
>         ranges;
>         ...
>
>         usb1: usb@48890000 {
>                 compatible = "snps,dwc3";
>                 reg = <0x48890000 0x17000>;
>                 ...
>         };
> };
>
>
> The glue layer initializes SoC-specific things,
> then populates the child "snps,dwc3".
>
>
> I think the following structure should work
> by handling EPROBE_DEFER properly.
>
> omap_dwc3_1: omap_dwc3_1@48880000 {
>         compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
>         reg = <0x48880000 0x10000>;
>         ...
> };
>
> usb1: usb@48890000 {
>         compatible = "snps,dwc3";
>         reg = <0x48890000 0x17000>;
>         ...
> };
>
>
>
>>>
>>> I inserted reset_control_deassert() and clk_enable() before the first
>>> register access, i.e. dwc3_cache_hwparams().
>>>
>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>> ---
>>>
>>>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>>>  drivers/usb/dwc3/core.h                        |   5 +
>>>  3 files changed, 132 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> index 44e8bab..67e9cfb 100644
>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>> @@ -9,12 +9,14 @@ Required properties:
>>>   - interrupts: Interrupts used by the dwc3 controller.
>>>
>>>  Optional properties:
>>> + - clocks: list of phandle and clock specifier pairs
>>
>> However, this should be specific as to how many clocks and their
>> function. This should be readily available to someone with access to
>> Synopsys datasheet. The number of clocks should generally be the same
>> across SoCs, it is just some SoCs either tie clocks together or don't
>> provide s/w control of some of the clocks.
>
>
> Make sense.
> You also implies this property is mandatory.
> The number of clocks should be available in the datasheet
> and no hardware can work with zero clock.
>
> However, making it mandatory breaks the binding
> since the existing DT files do not specify clocks at all
> in the "snps,dwc3" node.
>
>
>
> Anyway, our current situation:
>
> - We have the dwc3-core under the glue layer node
>   despite they are independent in the CPU address view
> - We add all sorts of clocks and resets in the glue layer node,
>   and nothing in the dwc3-core node.
>
> If these are design mistake, what should we do?
>
> Continue development based on it?
> If we fix it, how to change the course?
>

Any insight about this?


I think this is rather a general question.

If somebody upstreams a driver without clocks,
then later it turns out clocks are necessary,
adding required clocks would break existing platforms
since clk_get() fails.


-- 
Best Regards
Masahiro Yamada

Re: [PATCH 2/2] usb: dwc3: add clock and resets

[Rob Herring]

On Wed, Mar 28, 2018 at 11:32 PM, Masahiro Yamada
<yamada.masahiro@socionext.com> wrote:
> 2018-03-19 7:37 GMT+09:00 Masahiro Yamada <yamada.masahiro@socionext.com>:
>> Hi Rob,
>>
>> 2018-03-18 21:52 GMT+09:00 Rob Herring <robh@kernel.org>:
>>> On Thu, Mar 15, 2018 at 08:39:58PM +0900, Masahiro Yamada wrote:
>>>> dwc3-of-simple.c only handles arbitrary number of clocks and resets.
>>>> They are both generic enough to be put into the dwc3 core.  For simple
>>>> cases, a nested node structure like follows:
>>>>
>>>>   dwc3-glue {
>>>>           compatible = "foo,dwc3";
>>>>           clocks = ...;
>>>>           resets = ...;
>>>>           ...
>>>>
>>>>           dwc3 {
>>>>                   compatible = "snps,dwc3";
>>>>                   ...
>>>>           };
>>>
>>> I'm not a fan of how this was done.
>>>
>>>
>>>>   }
>>>>
>>>> would be turned into a single node:
>>>>
>>>>   dwc3 {
>>>>           compatible = "foo,dwc3", "snps,dwc3";
>>>>           clocks = ...;
>>>>           resets = ...;
>>>>           ...
>>>>   }
>>>
>>> And yes, this is what I'd prefer.
>>
>>
>>
>> Not only dwc3-of-simple.c, but all dwc3 nodes are
>> written like this.
>>
>>
>> omap_dwc3_1: omap_dwc3_1@48880000 {
>>         compatible = "ti,dwc3";
>>         reg = <0x48880000 0x10000>;
>>         #address-cells = <1>;
>>         #size-cells = <1>;
>>         ranges;
>>         ...
>>
>>         usb1: usb@48890000 {
>>                 compatible = "snps,dwc3";
>>                 reg = <0x48890000 0x17000>;
>>                 ...
>>         };
>> };
>>
>>
>> The glue layer initializes SoC-specific things,
>> then populates the child "snps,dwc3".
>>
>>
>> I think the following structure should work
>> by handling EPROBE_DEFER properly.
>>
>> omap_dwc3_1: omap_dwc3_1@48880000 {
>>         compatible = "ti,dwc3"; (should be "ti,dwc3-glue" or something)
>>         reg = <0x48880000 0x10000>;
>>         ...
>> };
>>
>> usb1: usb@48890000 {
>>         compatible = "snps,dwc3";
>>         reg = <0x48890000 0x17000>;
>>         ...
>> };
>>
>>
>>
>>>>
>>>> I inserted reset_control_deassert() and clk_enable() before the first
>>>> register access, i.e. dwc3_cache_hwparams().
>>>>
>>>> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
>>>> ---
>>>>
>>>>  Documentation/devicetree/bindings/usb/dwc3.txt |   2 +
>>>>  drivers/usb/dwc3/core.c                        | 127 ++++++++++++++++++++++++-
>>>>  drivers/usb/dwc3/core.h                        |   5 +
>>>>  3 files changed, 132 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> index 44e8bab..67e9cfb 100644
>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>>>> @@ -9,12 +9,14 @@ Required properties:
>>>>   - interrupts: Interrupts used by the dwc3 controller.
>>>>
>>>>  Optional properties:
>>>> + - clocks: list of phandle and clock specifier pairs
>>>
>>> However, this should be specific as to how many clocks and their
>>> function. This should be readily available to someone with access to
>>> Synopsys datasheet. The number of clocks should generally be the same
>>> across SoCs, it is just some SoCs either tie clocks together or don't
>>> provide s/w control of some of the clocks.
>>
>>
>> Make sense.
>> You also implies this property is mandatory.
>> The number of clocks should be available in the datasheet
>> and no hardware can work with zero clock.
>>
>> However, making it mandatory breaks the binding
>> since the existing DT files do not specify clocks at all
>> in the "snps,dwc3" node.

Because of this it has to be optional. We could list what compatibles
it is optional for and that it is mandatory for new users.

>>
>> Anyway, our current situation:
>>
>> - We have the dwc3-core under the glue layer node
>>   despite they are independent in the CPU address view
>> - We add all sorts of clocks and resets in the glue layer node,
>>   and nothing in the dwc3-core node.
>>
>> If these are design mistake, what should we do?
>>
>> Continue development based on it?
>> If we fix it, how to change the course?
>>
>
> Any insight about this?

We just need to reject new bindings that do things the old way. The
driver will have to support both cases for some time period.


> I think this is rather a general question.
>
> If somebody upstreams a driver without clocks,
> then later it turns out clocks are necessary,
> adding required clocks would break existing platforms
> since clk_get() fails.

We should try to prevent that, but of course that is not always
possible. Unless that platform is deemed unstable and we break
compatibility, we'd have to handle both cases in the driver.

Rob