[PATCH 0/2] Allow xhci-plat using a second clock

[PATCH 0/2] Allow xhci-plat using a second clock

[Gregory CLEMENT]

Hello,

The purpose of this series is to allow xhci-plat using a second
clock. It is needed on the Armada 7K/8K but could be used by other
SoCs.

The first patch is just a fix found while I was working on this
feature.

Thanks,

Gregory


Gregory CLEMENT (2):
  usb: host: xhci-plat: Remove useless test before clk_disable_unprepare
  usb: host: xhci-plat: Fix clock resource by adding a register clock

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 ++-
 drivers/usb/host/xhci-plat.c                       | 39 ++++++++++++++++------
 drivers/usb/host/xhci.h                            |  3 +-
 3 files changed, 35 insertions(+), 12 deletions(-)

-- 
2.15.1

[PATCH 1/2] usb: host: xhci-plat: Remove useless test before clk_disable_unprepare

[Gregory CLEMENT]

clk_disable_unprepare() already checks that the clock pointer is valid.
No need to test it before calling it.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 drivers/usb/host/xhci-plat.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 6f038306c14d..79afaac57ef6 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -319,8 +319,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	usb_put_hcd(xhci->shared_hcd);
 
 disable_clk:
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 
 put_hcd:
 	usb_put_hcd(hcd);
@@ -346,8 +345,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	usb_remove_hcd(hcd);
 	usb_put_hcd(xhci->shared_hcd);
 
-	if (!IS_ERR(clk))
-		clk_disable_unprepare(clk);
+	clk_disable_unprepare(clk);
 	usb_put_hcd(hcd);
 
 	pm_runtime_set_suspended(&dev->dev);
-- 
2.15.1

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Gregory CLEMENT]

On Armada 7K/8K we need to explicitly enable the register clock. This
clock is optional because not all the SoCs using this IP need it but at
least for Armada 7K/8K it is actually mandatory.

The change was done at xhci-plat level and not at a xhci-mvebu.c because,
it is expected that other SoC would have this kind of constraint.

The binding documentation is updating accordingly.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
 drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
 drivers/usb/host/xhci.h                            |  3 +-
 3 files changed, 33 insertions(+), 8 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index e2ea59bbca93..e4b14511f4f8 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -27,7 +27,10 @@ Required properties:
   - interrupts: one XHCI interrupt should be described here.
 
 Optional properties:
-  - clocks: reference to a clock
+  - clocks: reference to the clocks
+  - clock-names: mandatory if there is a second clock, in this case
+    the name must be "core" for the first clock and "reg" for the
+    second one
   - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable mechanism
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 79afaac57ef6..fd0c399013a2 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	struct resource         *res;
 	struct usb_hcd		*hcd;
 	struct clk              *clk;
+	struct clk              *reg_clk;
 	int			ret;
 	int			irq;
 
@@ -226,17 +227,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	hcd->rsrc_len = resource_size(res);
 
 	/*
-	 * Not all platforms have a clk so it is not an error if the
-	 * clock does not exists.
+	 * Not all platforms have clks so it is not an error if the
+	 * clock do not exist.
 	 */
+	reg_clk = devm_clk_get(&pdev->dev, "reg");
+	if (!IS_ERR(reg_clk)) {
+		ret = clk_prepare_enable(reg_clk);
+		if (ret)
+			goto put_hcd;
+	} else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
+		ret = -EPROBE_DEFER;
+		goto put_hcd;
+	}
+
 	clk = devm_clk_get(&pdev->dev, NULL);
 	if (!IS_ERR(clk)) {
 		ret = clk_prepare_enable(clk);
 		if (ret)
-			goto put_hcd;
+			goto disable_reg_clk;
 	} else if (PTR_ERR(clk) == -EPROBE_DEFER) {
 		ret = -EPROBE_DEFER;
-		goto put_hcd;
+		goto disable_reg_clk;
 	}
 
 	xhci = hcd_to_xhci(hcd);
@@ -252,6 +263,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
 	device_wakeup_enable(hcd->self.controller);
 
 	xhci->clk = clk;
+	xhci->reg_clk = reg_clk;
 	xhci->main_hcd = hcd;
 	xhci->shared_hcd = __usb_create_hcd(driver, sysdev, &pdev->dev,
 			dev_name(&pdev->dev), hcd);
@@ -321,6 +333,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 disable_clk:
 	clk_disable_unprepare(clk);
 
+disable_reg_clk:
+	clk_disable_unprepare(reg_clk);
+
 put_hcd:
 	usb_put_hcd(hcd);
 
@@ -336,6 +351,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	struct usb_hcd	*hcd = platform_get_drvdata(dev);
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	struct clk *clk = xhci->clk;
+	struct clk *reg_clk = xhci->reg_clk;
 
 	xhci->xhc_state |= XHCI_STATE_REMOVING;
 
@@ -346,6 +362,7 @@ static int xhci_plat_remove(struct platform_device *dev)
 	usb_put_hcd(xhci->shared_hcd);
 
 	clk_disable_unprepare(clk);
+	clk_disable_unprepare(reg_clk);
 	usb_put_hcd(hcd);
 
 	pm_runtime_set_suspended(&dev->dev);
@@ -370,8 +387,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
 	 */
 	ret = xhci_suspend(xhci, device_may_wakeup(dev));
 
-	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
+	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
 		clk_disable_unprepare(xhci->clk);
+		clk_disable_unprepare(xhci->reg_clk);
+	}
 
 	return ret;
 }
@@ -382,8 +401,10 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 	int ret;
 
-	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
+	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
+		clk_prepare_enable(xhci->reg_clk);
 		clk_prepare_enable(xhci->clk);
+	}
 
 	ret = xhci_priv_resume_quirk(hcd);
 	if (ret)
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 96099a245c69..9884aaba1b40 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1726,8 +1726,9 @@ struct xhci_hcd {
 	int		page_shift;
 	/* msi-x vectors */
 	int		msix_count;
-	/* optional clock */
+	/* optional clocks */
 	struct clk		*clk;
+	struct clk		*reg_clk;
 	/* data structures */
 	struct xhci_device_context_array *dcbaa;
 	struct xhci_ring	*cmd_ring;
-- 
2.15.1

[PATCH 0/2] Allow xhci-plat using a second clock

[Gregory CLEMENT]

Hi Mathias,
 
 On mer., f?vr. 14 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hello,
>
> The purpose of this series is to allow xhci-plat using a second
> clock. It is needed on the Armada 7K/8K but could be used by other
> SoCs.

Do you have some comments on this series and especially on the second
patch?

Thanks,

Gregory

>
> The first patch is just a fix found while I was working on this
> feature.
>
> Thanks,
>
> Gregory
>
>
> Gregory CLEMENT (2):
>   usb: host: xhci-plat: Remove useless test before clk_disable_unprepare
>   usb: host: xhci-plat: Fix clock resource by adding a register clock
>
>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 ++-
>  drivers/usb/host/xhci-plat.c                       | 39 ++++++++++++++++------
>  drivers/usb/host/xhci.h                            |  3 +-
>  3 files changed, 35 insertions(+), 12 deletions(-)
>
> -- 
> 2.15.1
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Mathias Nyman]

On 14.02.2018 18:16, Gregory CLEMENT wrote:
> On Armada 7K/8K we need to explicitly enable the register clock. This
> clock is optional because not all the SoCs using this IP need it but at
> least for Armada 7K/8K it is actually mandatory.
> 
> The change was done at xhci-plat level and not at a xhci-mvebu.c because,
> it is expected that other SoC would have this kind of constraint.
> 
> The binding documentation is updating accordingly.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>   Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
>   drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
>   drivers/usb/host/xhci.h                            |  3 +-
>   3 files changed, 33 insertions(+), 8 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index e2ea59bbca93..e4b14511f4f8 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -27,7 +27,10 @@ Required properties:
>     - interrupts: one XHCI interrupt should be described here.
>   
>   Optional properties:
> -  - clocks: reference to a clock
> +  - clocks: reference to the clocks
> +  - clock-names: mandatory if there is a second clock, in this case
> +    the name must be "core" for the first clock and "reg" for the
> +    second one
>     - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
>     - usb3-lpm-capable: determines if platform is USB3 LPM capable
>     - quirk-broken-port-ped: set if the controller has broken port disable mechanism

Would be good to get a Ack or review by Rob Herring for the above

> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 79afaac57ef6..fd0c399013a2 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   	struct resource         *res;
>   	struct usb_hcd		*hcd;
>   	struct clk              *clk;
> +	struct clk              *reg_clk;
>   	int			ret;
>   	int			irq;
>   
> @@ -226,17 +227,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   	hcd->rsrc_len = resource_size(res);
>   
>   	/*
> -	 * Not all platforms have a clk so it is not an error if the
> -	 * clock does not exists.
> +	 * Not all platforms have clks so it is not an error if the
> +	 * clock do not exist.
>   	 */
> +	reg_clk = devm_clk_get(&pdev->dev, "reg");
> +	if (!IS_ERR(reg_clk)) {
> +		ret = clk_prepare_enable(reg_clk);
> +		if (ret)
> +			goto put_hcd;
> +	} else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto put_hcd;
> +	}
> +
>   	clk = devm_clk_get(&pdev->dev, NULL);
>   	if (!IS_ERR(clk)) {
>   		ret = clk_prepare_enable(clk);
>   		if (ret)
> -			goto put_hcd;
> +			goto disable_reg_clk;
>   	} else if (PTR_ERR(clk) == -EPROBE_DEFER) {
>   		ret = -EPROBE_DEFER;
> -		goto put_hcd;
> +		goto disable_reg_clk;
>   	}
>   
>   	xhci = hcd_to_xhci(hcd);
> @@ -252,6 +263,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   	device_wakeup_enable(hcd->self.controller);
>   
>   	xhci->clk = clk;
> +	xhci->reg_clk = reg_clk;
>   	xhci->main_hcd = hcd;
>   	xhci->shared_hcd = __usb_create_

hcd(driver, sysdev, &pdev->dev,
>   			dev_name(&pdev->dev), hcd);
> @@ -321,6 +333,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   disable_clk:
>   	clk_disable_unprepare(clk);
>   
> +disable_reg_clk:
> +	clk_disable_unprepare(reg_clk);
> +
>   put_hcd:
>   	usb_put_hcd(hcd);
>   
> @@ -336,6 +351,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>   	struct usb_hcd	*hcd = platform_get_drvdata(dev);
>   	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>   	struct clk *clk = xhci->clk;
> +	struct clk *reg_clk = xhci->reg_clk;
>   
>   	xhci->xhc_state |= XHCI_STATE_REMOVING;
>   
> @@ -346,6 +362,7 @@ static int xhci_plat_remove(struct platform_device *dev)
>   	usb_put_hcd(xhci->shared_hcd);
>   
>   	clk_disable_unprepare(clk);
> +	clk_disable_unprepare(reg_clk);
>   	usb_put_hcd(hcd);
>   
>   	pm_runtime_set_suspended(&dev->dev);
> @@ -370,8 +387,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>   	 */
>   	ret = xhci_suspend(xhci, device_may_wakeup(dev));
>   
> -	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
> +	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
>   		clk_disable_unprepare(xhci->clk);
> +		clk_disable_unprepare(xhci->reg_clk);
> +	}
>   
>   	return ret;
>   }
> @@ -382,8 +401,10 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
>   	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>   	int ret;
>   
> -	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
> +	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
> +		clk_prepare_enable(xhci->reg_clk);
>   		clk_prepare_enable(xhci->clk);
> +	}
>   
>   	ret = xhci_priv_resume_quirk(hcd);
>   	if (ret)
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index 96099a245c69..9884aaba1b40 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1726,8 +1726,9 @@ struct xhci_hcd {
>   	int		page_shift;
>   	/* msi-x vectors */
>   	int		msix_count;
> -	/* optional clock */
> +	/* optional clocks */
>   	struct clk		*clk;
> +	struct clk		*reg_clk;
>   	/* data structures */
>   	struct xhci_device_context_array *dcbaa;
>   	struct xhci_ring	*cmd_ring;
> 

Looks good to me, I'll queue it up if the devicetree bindings looks ok for Rob Herring

-Mathias

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Manu Gautam]

Hi,


On 2/14/2018 9:46 PM, Gregory CLEMENT wrote:
> On Armada 7K/8K we need to explicitly enable the register clock. This
> clock is optional because not all the SoCs using this IP need it but at
> least for Armada 7K/8K it is actually mandatory.
>
> The change was done at xhci-plat level and not at a xhci-mvebu.c because,
> it is expected that other SoC would have this kind of constraint.
>
> The binding documentation is updating accordingly.
>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
>  drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
>  drivers/usb/host/xhci.h                            |  3 +-
>  3 files changed, 33 insertions(+), 8 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> index e2ea59bbca93..e4b14511f4f8 100644
> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
> @@ -27,7 +27,10 @@ Required properties:
>    - interrupts: one XHCI interrupt should be described here.
>  
>  Optional properties:
> -  - clocks: reference to a clock
> +  - clocks: reference to the clocks
> +  - clock-names: mandatory if there is a second clock, in this case
> +    the name must be "core" for the first clock and "reg" for the
> +    second one
>    - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
>    - usb3-lpm-capable: determines if platform is USB3 LPM capable
>    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> index 79afaac57ef6..fd0c399013a2 100644
> --- a/drivers/usb/host/xhci-plat.c
> +++ b/drivers/usb/host/xhci-plat.c
> @@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	struct resource         *res;
>  	struct usb_hcd		*hcd;
>  	struct clk              *clk;
> +	struct clk              *reg_clk;
>  	int			ret;
>  	int			irq;
>  
> @@ -226,17 +227,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
>  	hcd->rsrc_len = resource_size(res);
>  
>  	/*
> -	 * Not all platforms have a clk so it is not an error if the
> -	 * clock does not exists.
> +	 * Not all platforms have clks so it is not an error if the
> +	 * clock do not exist.
>  	 */
> +	reg_clk = devm_clk_get(&pdev->dev, "reg");
> +	if (!IS_ERR(reg_clk)) {
> +		ret = clk_prepare_enable(reg_clk);
> +		if (ret)
> +			goto put_hcd;
> +	} else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
> +		ret = -EPROBE_DEFER;
> +		goto put_hcd;
> +	}
> +

How about using clk_bulk_ APIs?

>  	clk = devm_clk_get(&pdev->dev, NULL);
>  	if (!IS_ERR(clk)) {
>  		ret = clk_prepare_enable(clk);
>

-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Manu,
 
 On mer., f?vr. 28 2018, Manu Gautam <mgautam@codeaurora.org> wrote:

> Hi,
>
>
> On 2/14/2018 9:46 PM, Gregory CLEMENT wrote:
>> On Armada 7K/8K we need to explicitly enable the register clock. This
>> clock is optional because not all the SoCs using this IP need it but at
>> least for Armada 7K/8K it is actually mandatory.
>>
>> The change was done at xhci-plat level and not at a xhci-mvebu.c because,
>> it is expected that other SoC would have this kind of constraint.
>>
>> The binding documentation is updating accordingly.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
>>  drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
>>  drivers/usb/host/xhci.h                            |  3 +-
>>  3 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> index e2ea59bbca93..e4b14511f4f8 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> @@ -27,7 +27,10 @@ Required properties:
>>    - interrupts: one XHCI interrupt should be described here.
>>  
>>  Optional properties:
>> -  - clocks: reference to a clock
>> +  - clocks: reference to the clocks
>> +  - clock-names: mandatory if there is a second clock, in this case
>> +    the name must be "core" for the first clock and "reg" for the
>> +    second one
>>    - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
>>    - usb3-lpm-capable: determines if platform is USB3 LPM capable
>>    - quirk-broken-port-ped: set if the controller has broken port disable mechanism
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 79afaac57ef6..fd0c399013a2 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>  	struct resource         *res;
>>  	struct usb_hcd		*hcd;
>>  	struct clk              *clk;
>> +	struct clk              *reg_clk;
>>  	int			ret;
>>  	int			irq;
>>  
>> @@ -226,17 +227,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>  	hcd->rsrc_len = resource_size(res);
>>  
>>  	/*
>> -	 * Not all platforms have a clk so it is not an error if the
>> -	 * clock does not exists.
>> +	 * Not all platforms have clks so it is not an error if the
>> +	 * clock do not exist.
>>  	 */
>> +	reg_clk = devm_clk_get(&pdev->dev, "reg");
>> +	if (!IS_ERR(reg_clk)) {
>> +		ret = clk_prepare_enable(reg_clk);
>> +		if (ret)
>> +			goto put_hcd;
>> +	} else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
>> +		ret = -EPROBE_DEFER;
>> +		goto put_hcd;
>> +	}
>> +
>
> How about using clk_bulk_ APIs?

I didn't know this API, but after having a look on it, it didn't match
what I need. Indeed the second clock is "optional" to handle the
backward compatibility. With the clk_bulk_ APIS all the clocks are
mandatory.

Thanks,

Gregory

>
>>  	clk = devm_clk_get(&pdev->dev, NULL);
>>  	if (!IS_ERR(clk)) {
>>  		ret = clk_prepare_enable(clk);
>>
>
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Rob,
 
 On mer., f?vr. 28 2018, Mathias Nyman <mathias.nyman@intel.com> wrote:

> On 14.02.2018 18:16, Gregory CLEMENT wrote:
>> On Armada 7K/8K we need to explicitly enable the register clock. This
>> clock is optional because not all the SoCs using this IP need it but at
>> least for Armada 7K/8K it is actually mandatory.
>>
>> The change was done at xhci-plat level and not at a xhci-mvebu.c because,
>> it is expected that other SoC would have this kind of constraint.
>>
>> The binding documentation is updating accordingly.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>   Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
>>   drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
>>   drivers/usb/host/xhci.h                            |  3 +-
>>   3 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> index e2ea59bbca93..e4b14511f4f8 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> @@ -27,7 +27,10 @@ Required properties:
>>     - interrupts: one XHCI interrupt should be described here.
>>     Optional properties:
>> -  - clocks: reference to a clock
>> +  - clocks: reference to the clocks
>> +  - clock-names: mandatory if there is a second clock, in this case
>> +    the name must be "core" for the first clock and "reg" for the
>> +    second one
>>     - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
>>     - usb3-lpm-capable: determines if platform is USB3 LPM capable
>>     - quirk-broken-port-ped: set if the controller has broken port disable mechanism
>
> Would be good to get a Ack or review by Rob Herring for the above

Would you mind to have a look on this binding update ?

Initially I didn't copy you trying to not overflow you because this
change is only about adding a new clock, which is pretty common. But I
think Mathias would be more confident with a Ack or review by you.

Thanks,

Gregory

>
>> diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
>> index 79afaac57ef6..fd0c399013a2 100644
>> --- a/drivers/usb/host/xhci-plat.c
>> +++ b/drivers/usb/host/xhci-plat.c
>> @@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>   	struct resource         *res;
>>   	struct usb_hcd		*hcd;
>>   	struct clk              *clk;
>> +	struct clk              *reg_clk;
>>   	int			ret;
>>   	int			irq;
>>   @@ -226,17 +227,27 @@ static int xhci_plat_probe(struct
>> platform_device *pdev)
>>   	hcd->rsrc_len = resource_size(res);
>>     	/*
>> -	 * Not all platforms have a clk so it is not an error if the
>> -	 * clock does not exists.
>> +	 * Not all platforms have clks so it is not an error if the
>> +	 * clock do not exist.
>>   	 */
>> +	reg_clk = devm_clk_get(&pdev->dev, "reg");
>> +	if (!IS_ERR(reg_clk)) {
>> +		ret = clk_prepare_enable(reg_clk);
>> +		if (ret)
>> +			goto put_hcd;
>> +	} else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
>> +		ret = -EPROBE_DEFER;
>> +		goto put_hcd;
>> +	}
>> +
>>   	clk = devm_clk_get(&pdev->dev, NULL);
>>   	if (!IS_ERR(clk)) {
>>   		ret = clk_prepare_enable(clk);
>>   		if (ret)
>> -			goto put_hcd;
>> +			goto disable_reg_clk;
>>   	} else if (PTR_ERR(clk) == -EPROBE_DEFER) {
>>   		ret = -EPROBE_DEFER;
>> -		goto put_hcd;
>> +		goto disable_reg_clk;
>>   	}
>>     	xhci = hcd_to_xhci(hcd);
>> @@ -252,6 +263,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>   	device_wakeup_enable(hcd->self.controller);
>>     	xhci->clk = clk;
>> +	xhci->reg_clk = reg_clk;
>>   	xhci->main_hcd = hcd;
>>   	xhci->shared_hcd = __usb_create_
>
> hcd(driver, sysdev, &pdev->dev,
>>   			dev_name(&pdev->dev), hcd);
>> @@ -321,6 +333,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
>>   disable_clk:
>>   	clk_disable_unprepare(clk);
>>   +disable_reg_clk:
>> +	clk_disable_unprepare(reg_clk);
>> +
>>   put_hcd:
>>   	usb_put_hcd(hcd);
>>   @@ -336,6 +351,7 @@ static int xhci_plat_remove(struct
>> platform_device *dev)
>>   	struct usb_hcd	*hcd = platform_get_drvdata(dev);
>>   	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>>   	struct clk *clk = xhci->clk;
>> +	struct clk *reg_clk = xhci->reg_clk;
>>     	xhci->xhc_state |= XHCI_STATE_REMOVING;
>>   @@ -346,6 +362,7 @@ static int xhci_plat_remove(struct
>> platform_device *dev)
>>   	usb_put_hcd(xhci->shared_hcd);
>>     	clk_disable_unprepare(clk);
>> +	clk_disable_unprepare(reg_clk);
>>   	usb_put_hcd(hcd);
>>     	pm_runtime_set_suspended(&dev->dev);
>> @@ -370,8 +387,10 @@ static int __maybe_unused xhci_plat_suspend(struct device *dev)
>>   	 */
>>   	ret = xhci_suspend(xhci, device_may_wakeup(dev));
>>   -	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
>> +	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
>>   		clk_disable_unprepare(xhci->clk);
>> +		clk_disable_unprepare(xhci->reg_clk);
>> +	}
>>     	return ret;
>>   }
>> @@ -382,8 +401,10 @@ static int __maybe_unused xhci_plat_resume(struct device *dev)
>>   	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
>>   	int ret;
>>   -	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk))
>> +	if (!device_may_wakeup(dev) && !IS_ERR(xhci->clk)) {
>> +		clk_prepare_enable(xhci->reg_clk);
>>   		clk_prepare_enable(xhci->clk);
>> +	}
>>     	ret = xhci_priv_resume_quirk(hcd);
>>   	if (ret)
>> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
>> index 96099a245c69..9884aaba1b40 100644
>> --- a/drivers/usb/host/xhci.h
>> +++ b/drivers/usb/host/xhci.h
>> @@ -1726,8 +1726,9 @@ struct xhci_hcd {
>>   	int		page_shift;
>>   	/* msi-x vectors */
>>   	int		msix_count;
>> -	/* optional clock */
>> +	/* optional clocks */
>>   	struct clk		*clk;
>> +	struct clk		*reg_clk;
>>   	/* data structures */
>>   	struct xhci_device_context_array *dcbaa;
>>   	struct xhci_ring	*cmd_ring;
>>
>
> Looks good to me, I'll queue it up if the devicetree bindings looks ok for Rob Herring
>
> -Mathias
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 2/2] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Gregory CLEMENT]

Hi Rob,

(resent because of malformed cc list)
 
 On mer., f?vr. 28 2018, Mathias Nyman <mathias.nyman@intel.com> wrote:

> On 14.02.2018 18:16, Gregory CLEMENT wrote:
>> On Armada 7K/8K we need to explicitly enable the register clock. This
>> clock is optional because not all the SoCs using this IP need it but at
>> least for Armada 7K/8K it is actually mandatory.
>>
>> The change was done at xhci-plat level and not at a xhci-mvebu.c because,
>> it is expected that other SoC would have this kind of constraint.
>>
>> The binding documentation is updating accordingly.
>>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
>> ---
>>   Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
>>   drivers/usb/host/xhci-plat.c                       | 33 ++++++++++++++++++----
>>   drivers/usb/host/xhci.h                            |  3 +-
>>   3 files changed, 33 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> index e2ea59bbca93..e4b14511f4f8 100644
>> --- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
>> @@ -27,7 +27,10 @@ Required properties:
>>     - interrupts: one XHCI interrupt should be described here.
>>     Optional properties:
>> -  - clocks: reference to a clock
>> +  - clocks: reference to the clocks
>> +  - clock-names: mandatory if there is a second clock, in this case
>> +    the name must be "core" for the first clock and "reg" for the
>> +    second one
>>     - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
>>     - usb3-lpm-capable: determines if platform is USB3 LPM capable
>>     - quirk-broken-port-ped: set if the controller has broken port disable mechanism
>
> Would be good to get a Ack or review by Rob Herring for the above

Would you mind to have a look on this binding update ?

Initially I didn't copy you trying to not overflow you because this
change is only about adding a new clock, which is pretty common. But I
think Mathias would be more confident with a Ack or review by you.

Thanks,

Gregory

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 0/2] Allow xhci-plat using a second clock

[Gregory CLEMENT]

Hi Mathias,
 
 On mer., f?vr. 14 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:

> Hello,
>
> The purpose of this series is to allow xhci-plat using a second
> clock. It is needed on the Armada 7K/8K but could be used by other
> SoCs.
>
> The first patch is just a fix found while I was working on this
> feature.

I've just realized that this series sent 2 months ago was not merged in
v4.17. The issue is that now the USB support on the Armada 7K/8K is
broken, because the clock support part was already merged.

You already had a look on this series one month ago. The issue you had
was about getting an approval for the extension of the binding [1]. I
pinged Rob about it [2] one month ago but we didn't get any
feedback. However, Rob already approved the similar changes I introduced
in an other for mv_xor_v2: [3].

So would it be possible to apply this series on v4.17-rc ?

Thanks,

Gregory

[1]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-February/562739.html

[2]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/566076.html

[3]:
http://lists.infradead.org/pipermail/linux-arm-kernel/2018-March/564594.html




>
> Thanks,
>
> Gregory
>
>
> Gregory CLEMENT (2):
>   usb: host: xhci-plat: Remove useless test before clk_disable_unprepare
>   usb: host: xhci-plat: Fix clock resource by adding a register clock
>
>  Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 ++-
>  drivers/usb/host/xhci-plat.c                       | 39 ++++++++++++++++------
>  drivers/usb/host/xhci.h                            |  3 +-
>  3 files changed, 35 insertions(+), 12 deletions(-)
>
> -- 
> 2.15.1
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com

[PATCH 0/2] Allow xhci-plat using a second clock

[Mathias Nyman]

On 18.04.2018 17:20, Gregory CLEMENT wrote:
> Hi Mathias,
>   
>   On mer., f?vr. 14 2018, Gregory CLEMENT <gregory.clement@bootlin.com> wrote:
> 
>> Hello,
>>
>> The purpose of this series is to allow xhci-plat using a second
>> clock. It is needed on the Armada 7K/8K but could be used by other
>> SoCs.
>>
>> The first patch is just a fix found while I was working on this
>> feature.
> 
> I've just realized that this series sent 2 months ago was not merged in
> v4.17. The issue is that now the USB support on the Armada 7K/8K is
> broken, because the clock support part was already merged.
> 
> You already had a look on this series one month ago. The issue you had
> was about getting an approval for the extension of the binding [1]. I
> pinged Rob about it [2] one month ago but we didn't get any
> feedback. However, Rob already approved the similar changes I introduced
> in an other for mv_xor_v2: [3].
> 
> So would it be possible to apply this series on v4.17-rc ?

Patch 2/2 no longer applies due to clk suspend/resume changes in:

commit d56e57ca030c8b4296944a2ae61ac167bf979c07
     usb: host: xhci-plat: revert "usb: host: xhci-plat: enable clk in resume timing"

Not sure how that change affects Armada 7K/8K
Can you rebase and resend the series?

-Mathias

[PATCH 0/2] Allow xhci-plat using a second clock

[Gregory CLEMENT]

Hi Mathias,
 
 On jeu., avril 19 2018, Mathias Nyman <mathias.nyman@linux.intel.com> wrote:

> On 18.04.2018 17:20, Gregory CLEMENT wrote:
>> Hi Mathias,
>>     On mer., f?vr. 14 2018, Gregory CLEMENT
>> <gregory.clement@bootlin.com> wrote:
>>
>>> Hello,
>>>
>>> The purpose of this series is to allow xhci-plat using a second
>>> clock. It is needed on the Armada 7K/8K but could be used by other
>>> SoCs.
>>>
>>> The first patch is just a fix found while I was working on this
>>> feature.
>>
>> I've just realized that this series sent 2 months ago was not merged in
>> v4.17. The issue is that now the USB support on the Armada 7K/8K is
>> broken, because the clock support part was already merged.
>>
>> You already had a look on this series one month ago. The issue you had
>> was about getting an approval for the extension of the binding [1]. I
>> pinged Rob about it [2] one month ago but we didn't get any
>> feedback. However, Rob already approved the similar changes I introduced
>> in an other for mv_xor_v2: [3].
>>
>> So would it be possible to apply this series on v4.17-rc ?
>
> Patch 2/2 no longer applies due to clk suspend/resume changes in:
>
> commit d56e57ca030c8b4296944a2ae61ac167bf979c07
>     usb: host: xhci-plat: revert "usb: host: xhci-plat: enable clk in resume timing"
>
> Not sure how that change affects Armada 7K/8K
> Can you rebase and resend the series?

It's done!

About the suspend/resume part it does not affect Armada 7K/8K because we
don't support it yet.

However I am a bit surprised by the commit log because reverting a
common part because of a specific implementation looks wrong. I expect
issues when we will add PM support but we will see that later.

Gregory

>
> -Mathias
>

-- 
Gregory Clement, Bootlin (formerly Free Electrons)
Embedded Linux and Kernel engineering
http://bootlin.com