[RFC/RFT usb-next v1 0/6] remove driver-specific "multiple PHY" handling

[RFC/RFT usb-next v1 0/6] remove driver-specific "multiple PHY" handling

[Martin Blumenstingl]

(this is the follow-up to my "initialize (multiple) PHYs for a HCD"
series from [0])

Various USB controller drivers parse the "phys" device-tree property and
grab all listed PHYs.
There is a lot of duplicate code around this in the various drivers:
- parse the "phys" property during .probe and save all PHYs in an array
- call phy_init and phy_power_on on all PHYs
- (where supported by the driver) call phy_power_off during .suspend and
  phy_power_on during .resume
- call phy_power_off and phy_exit during .remove

With [0] this functionality was moved to the core HCD driver, meaning
that all USB controller drivers now support this without having to
implement custom code for it.

Thus this series removes this code duplication.

The last two patches are a bit special:
- after patches #1-4 the chipidea driver is the last driver which sets
  the "phy" field of struct usb_hcd. remove this logic as the PHY
  wrapper (which is now part of the core HCD driver) manages the state
  of all PHYs anyways.
- the last patch removes the "phy" field from struct usb_hcd because
  nothing uses it anymore after the previous patches. it also removes
  the parsing and managing of a single PHY with the name "usb" as the
  new PHY wrapper parses all PHYs from the device-tree "phys" property
  anyways (so the PHY wrapper has the same functionality, except that it
  doesn't need the phy-name "usb" and it can manage more than one PHY)

preconditions for testing this series: all patches from [0] need to be
applied.
disclaimer: I don't have any hardware that uses any of the affected
drivers!


[0] http://lists.infradead.org/pipermail/linux-amlogic/2018-January/006274.html

Martin Blumenstingl (6):
  usb: mtu3: remove custom USB PHY handling
  usb: host: xhci-mtk: remove custom USB PHY handling
  usb: host: ehci-platform: remove custom USB PHY handling
  usb: host: ohci-platform: remove custom USB PHY handling
  usb: chipidea: do not set the "phy" field in struct usb_hcd
  usb: core: hcd: remove support for initializing a single PHY

 drivers/usb/chipidea/host.c      |   4 +-
 drivers/usb/core/hcd.c           |  37 --------------
 drivers/usb/host/ehci-platform.c |  55 ++-------------------
 drivers/usb/host/ohci-platform.c |  56 ++--------------------
 drivers/usb/host/xhci-mtk.c      |  98 +------------------------------------
 drivers/usb/mtu3/mtu3_plat.c     | 101 ---------------------------------------
 include/linux/usb/hcd.h          |   1 -
 7 files changed, 12 insertions(+), 340 deletions(-)

-- 
2.16.1

[RFC/RFT usb-next v1 1/6] usb: mtu3: remove custom USB PHY handling

[Martin Blumenstingl]

The new PHY wrapper is now wired up in the core HCD code. This means
that PHYs are now controlled (initialized, enabled, disabled, exited)
without requiring any host-driver specific code.
Remove the custom USB PHY handling from the mtu3 driver as the core HCD
code now handles this.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/mtu3/mtu3_plat.c | 101 -------------------------------------------
 1 file changed, 101 deletions(-)

diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
index 628d5ce356ca..a894ddf25bcd 100644
--- a/drivers/usb/mtu3/mtu3_plat.c
+++ b/drivers/usb/mtu3/mtu3_plat.c
@@ -44,62 +44,6 @@ int ssusb_check_clocks(struct ssusb_mtk *ssusb, u32 ex_clks)
 	return 0;
 }
 
-static int ssusb_phy_init(struct ssusb_mtk *ssusb)
-{
-	int i;
-	int ret;
-
-	for (i = 0; i < ssusb->num_phys; i++) {
-		ret = phy_init(ssusb->phys[i]);
-		if (ret)
-			goto exit_phy;
-	}
-	return 0;
-
-exit_phy:
-	for (; i > 0; i--)
-		phy_exit(ssusb->phys[i - 1]);
-
-	return ret;
-}
-
-static int ssusb_phy_exit(struct ssusb_mtk *ssusb)
-{
-	int i;
-
-	for (i = 0; i < ssusb->num_phys; i++)
-		phy_exit(ssusb->phys[i]);
-
-	return 0;
-}
-
-static int ssusb_phy_power_on(struct ssusb_mtk *ssusb)
-{
-	int i;
-	int ret;
-
-	for (i = 0; i < ssusb->num_phys; i++) {
-		ret = phy_power_on(ssusb->phys[i]);
-		if (ret)
-			goto power_off_phy;
-	}
-	return 0;
-
-power_off_phy:
-	for (; i > 0; i--)
-		phy_power_off(ssusb->phys[i - 1]);
-
-	return ret;
-}
-
-static void ssusb_phy_power_off(struct ssusb_mtk *ssusb)
-{
-	unsigned int i;
-
-	for (i = 0; i < ssusb->num_phys; i++)
-		phy_power_off(ssusb->phys[i]);
-}
-
 static int ssusb_clks_enable(struct ssusb_mtk *ssusb)
 {
 	int ret;
@@ -162,24 +106,8 @@ static int ssusb_rscs_init(struct ssusb_mtk *ssusb)
 	if (ret)
 		goto clks_err;
 
-	ret = ssusb_phy_init(ssusb);
-	if (ret) {
-		dev_err(ssusb->dev, "failed to init phy\n");
-		goto phy_init_err;
-	}
-
-	ret = ssusb_phy_power_on(ssusb);
-	if (ret) {
-		dev_err(ssusb->dev, "failed to power on phy\n");
-		goto phy_err;
-	}
-
 	return 0;
 
-phy_err:
-	ssusb_phy_exit(ssusb);
-phy_init_err:
-	ssusb_clks_disable(ssusb);
 clks_err:
 	regulator_disable(ssusb->vusb33);
 vusb33_err:
@@ -190,8 +118,6 @@ static void ssusb_rscs_exit(struct ssusb_mtk *ssusb)
 {
 	ssusb_clks_disable(ssusb);
 	regulator_disable(ssusb->vusb33);
-	ssusb_phy_power_off(ssusb);
-	ssusb_phy_exit(ssusb);
 }
 
 static void ssusb_ip_sw_reset(struct ssusb_mtk *ssusb)
@@ -222,7 +148,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
 	struct device *dev = &pdev->dev;
 	struct regulator *vbus;
 	struct resource *res;
-	int i;
 	int ret;
 
 	ssusb->vusb33 = devm_regulator_get(&pdev->dev, "vusb33");
@@ -249,25 +174,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
 	if (IS_ERR(ssusb->dma_clk))
 		return PTR_ERR(ssusb->dma_clk);
 
-	ssusb->num_phys = of_count_phandle_with_args(node,
-			"phys", "#phy-cells");
-	if (ssusb->num_phys > 0) {
-		ssusb->phys = devm_kcalloc(dev, ssusb->num_phys,
-					sizeof(*ssusb->phys), GFP_KERNEL);
-		if (!ssusb->phys)
-			return -ENOMEM;
-	} else {
-		ssusb->num_phys = 0;
-	}
-
-	for (i = 0; i < ssusb->num_phys; i++) {
-		ssusb->phys[i] = devm_of_phy_get_by_index(dev, node, i);
-		if (IS_ERR(ssusb->phys[i])) {
-			dev_err(dev, "failed to get phy-%d\n", i);
-			return PTR_ERR(ssusb->phys[i]);
-		}
-	}
-
 	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ippc");
 	ssusb->ippc_base = devm_ioremap_resource(dev, res);
 	if (IS_ERR(ssusb->ippc_base))
@@ -457,7 +363,6 @@ static int __maybe_unused mtu3_suspend(struct device *dev)
 		return 0;
 
 	ssusb_host_disable(ssusb, true);
-	ssusb_phy_power_off(ssusb);
 	ssusb_clks_disable(ssusb);
 	ssusb_wakeup_set(ssusb, true);
 
@@ -480,16 +385,10 @@ static int __maybe_unused mtu3_resume(struct device *dev)
 	if (ret)
 		goto clks_err;
 
-	ret = ssusb_phy_power_on(ssusb);
-	if (ret)
-		goto phy_err;
-
 	ssusb_host_enable(ssusb);
 
 	return 0;
 
-phy_err:
-	ssusb_clks_disable(ssusb);
 clks_err:
 	return ret;
 }
-- 
2.16.1

[RFC/RFT usb-next v1 1/6] usb: mtu3: remove custom USB PHY handling

[Chunfeng Yun]

Hi,

On Thu, 2018-01-25 at 01:16 +0100, Martin Blumenstingl wrote:
> The new PHY wrapper is now wired up in the core HCD code. This means
> that PHYs are now controlled (initialized, enabled, disabled, exited)
> without requiring any host-driver specific code.
> Remove the custom USB PHY handling from the mtu3 driver as the core HCD
> code now handles this.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/mtu3/mtu3_plat.c | 101 -------------------------------------------
>  1 file changed, 101 deletions(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
> index 628d5ce356ca..a894ddf25bcd 100644
> --- a/drivers/usb/mtu3/mtu3_plat.c
> +++ b/drivers/usb/mtu3/mtu3_plat.c
> @@ -44,62 +44,6 @@ int ssusb_check_clocks(struct ssusb_mtk *ssusb, u32 ex_clks)
>  	return 0;
>  }
>  
> -static int ssusb_phy_init(struct ssusb_mtk *ssusb)
> -{
> -	int i;
> -	int ret;
> -
> -	for (i = 0; i < ssusb->num_phys; i++) {
> -		ret = phy_init(ssusb->phys[i]);
> -		if (ret)
> -			goto exit_phy;
> -	}
> -	return 0;
> -
> -exit_phy:
> -	for (; i > 0; i--)
> -		phy_exit(ssusb->phys[i - 1]);
> -
> -	return ret;
> -}
> -
> -static int ssusb_phy_exit(struct ssusb_mtk *ssusb)
> -{
> -	int i;
> -
> -	for (i = 0; i < ssusb->num_phys; i++)
> -		phy_exit(ssusb->phys[i]);
> -
> -	return 0;
> -}
> -
> -static int ssusb_phy_power_on(struct ssusb_mtk *ssusb)
> -{
> -	int i;
> -	int ret;
> -
> -	for (i = 0; i < ssusb->num_phys; i++) {
> -		ret = phy_power_on(ssusb->phys[i]);
> -		if (ret)
> -			goto power_off_phy;
> -	}
> -	return 0;
> -
> -power_off_phy:
> -	for (; i > 0; i--)
> -		phy_power_off(ssusb->phys[i - 1]);
> -
> -	return ret;
> -}
> -
> -static void ssusb_phy_power_off(struct ssusb_mtk *ssusb)
> -{
> -	unsigned int i;
> -
> -	for (i = 0; i < ssusb->num_phys; i++)
> -		phy_power_off(ssusb->phys[i]);
> -}
> -
>  static int ssusb_clks_enable(struct ssusb_mtk *ssusb)
>  {
>  	int ret;
> @@ -162,24 +106,8 @@ static int ssusb_rscs_init(struct ssusb_mtk *ssusb)
>  	if (ret)
>  		goto clks_err;
>  
> -	ret = ssusb_phy_init(ssusb);
> -	if (ret) {
> -		dev_err(ssusb->dev, "failed to init phy\n");
> -		goto phy_init_err;
> -	}
> -
> -	ret = ssusb_phy_power_on(ssusb);
> -	if (ret) {
> -		dev_err(ssusb->dev, "failed to power on phy\n");
> -		goto phy_err;
> -	}
> -
>  	return 0;
>  
> -phy_err:
> -	ssusb_phy_exit(ssusb);
> -phy_init_err:
> -	ssusb_clks_disable(ssusb);
>  clks_err:
>  	regulator_disable(ssusb->vusb33);
>  vusb33_err:
> @@ -190,8 +118,6 @@ static void ssusb_rscs_exit(struct ssusb_mtk *ssusb)
>  {
>  	ssusb_clks_disable(ssusb);
>  	regulator_disable(ssusb->vusb33);
> -	ssusb_phy_power_off(ssusb);
> -	ssusb_phy_exit(ssusb);
>  }
>  
>  static void ssusb_ip_sw_reset(struct ssusb_mtk *ssusb)
> @@ -222,7 +148,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
>  	struct device *dev = &pdev->dev;
>  	struct regulator *vbus;
>  	struct resource *res;
> -	int i;
>  	int ret;
>  
>  	ssusb->vusb33 = devm_regulator_get(&pdev->dev, "vusb33");
> @@ -249,25 +174,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
>  	if (IS_ERR(ssusb->dma_clk))
>  		return PTR_ERR(ssusb->dma_clk);
>  
> -	ssusb->num_phys = of_count_phandle_with_args(node,
> -			"phys", "#phy-cells");
> -	if (ssusb->num_phys > 0) {
> -		ssusb->phys = devm_kcalloc(dev, ssusb->num_phys,
> -					sizeof(*ssusb->phys), GFP_KERNEL);
> -		if (!ssusb->phys)
> -			return -ENOMEM;
> -	} else {
> -		ssusb->num_phys = 0;
> -	}
> -
> -	for (i = 0; i < ssusb->num_phys; i++) {
> -		ssusb->phys[i] = devm_of_phy_get_by_index(dev, node, i);
> -		if (IS_ERR(ssusb->phys[i])) {
> -			dev_err(dev, "failed to get phy-%d\n", i);
> -			return PTR_ERR(ssusb->phys[i]);
> -		}
> -	}
> -
>  	res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ippc");
>  	ssusb->ippc_base = devm_ioremap_resource(dev, res);
>  	if (IS_ERR(ssusb->ippc_base))
> @@ -457,7 +363,6 @@ static int __maybe_unused mtu3_suspend(struct device *dev)
>  		return 0;
>  
>  	ssusb_host_disable(ssusb, true);
> -	ssusb_phy_power_off(ssusb);
>  	ssusb_clks_disable(ssusb);
>  	ssusb_wakeup_set(ssusb, true);
>  
> @@ -480,16 +385,10 @@ static int __maybe_unused mtu3_resume(struct device *dev)
>  	if (ret)
>  		goto clks_err;
>  
> -	ret = ssusb_phy_power_on(ssusb);
> -	if (ret)
> -		goto phy_err;
> -
>  	ssusb_host_enable(ssusb);
>  
>  	return 0;
>  
> -phy_err:
> -	ssusb_clks_disable(ssusb);
>  clks_err:
>  	return ret;
>  }
This patch will affect device function when works as device only mode.
Please abandon it, and I will modify it if need after the patch series
of "initialize (multiple) PHYs for a HCD" are picked up.

Thanks a lot

[RFC/RFT usb-next v1 1/6] usb: mtu3: remove custom USB PHY handling

[Martin Blumenstingl]

Hi,

On Thu, Jan 25, 2018 at 3:47 AM, Chunfeng Yun <chunfeng.yun@mediatek.com> wrote:
> Hi,
>
> On Thu, 2018-01-25 at 01:16 +0100, Martin Blumenstingl wrote:
>> The new PHY wrapper is now wired up in the core HCD code. This means
>> that PHYs are now controlled (initialized, enabled, disabled, exited)
>> without requiring any host-driver specific code.
>> Remove the custom USB PHY handling from the mtu3 driver as the core HCD
>> code now handles this.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/usb/mtu3/mtu3_plat.c | 101 -------------------------------------------
>>  1 file changed, 101 deletions(-)
>>
>> diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
>> index 628d5ce356ca..a894ddf25bcd 100644
>> --- a/drivers/usb/mtu3/mtu3_plat.c
>> +++ b/drivers/usb/mtu3/mtu3_plat.c
>> @@ -44,62 +44,6 @@ int ssusb_check_clocks(struct ssusb_mtk *ssusb, u32 ex_clks)
>>       return 0;
>>  }
>>
>> -static int ssusb_phy_init(struct ssusb_mtk *ssusb)
>> -{
>> -     int i;
>> -     int ret;
>> -
>> -     for (i = 0; i < ssusb->num_phys; i++) {
>> -             ret = phy_init(ssusb->phys[i]);
>> -             if (ret)
>> -                     goto exit_phy;
>> -     }
>> -     return 0;
>> -
>> -exit_phy:
>> -     for (; i > 0; i--)
>> -             phy_exit(ssusb->phys[i - 1]);
>> -
>> -     return ret;
>> -}
>> -
>> -static int ssusb_phy_exit(struct ssusb_mtk *ssusb)
>> -{
>> -     int i;
>> -
>> -     for (i = 0; i < ssusb->num_phys; i++)
>> -             phy_exit(ssusb->phys[i]);
>> -
>> -     return 0;
>> -}
>> -
>> -static int ssusb_phy_power_on(struct ssusb_mtk *ssusb)
>> -{
>> -     int i;
>> -     int ret;
>> -
>> -     for (i = 0; i < ssusb->num_phys; i++) {
>> -             ret = phy_power_on(ssusb->phys[i]);
>> -             if (ret)
>> -                     goto power_off_phy;
>> -     }
>> -     return 0;
>> -
>> -power_off_phy:
>> -     for (; i > 0; i--)
>> -             phy_power_off(ssusb->phys[i - 1]);
>> -
>> -     return ret;
>> -}
>> -
>> -static void ssusb_phy_power_off(struct ssusb_mtk *ssusb)
>> -{
>> -     unsigned int i;
>> -
>> -     for (i = 0; i < ssusb->num_phys; i++)
>> -             phy_power_off(ssusb->phys[i]);
>> -}
>> -
>>  static int ssusb_clks_enable(struct ssusb_mtk *ssusb)
>>  {
>>       int ret;
>> @@ -162,24 +106,8 @@ static int ssusb_rscs_init(struct ssusb_mtk *ssusb)
>>       if (ret)
>>               goto clks_err;
>>
>> -     ret = ssusb_phy_init(ssusb);
>> -     if (ret) {
>> -             dev_err(ssusb->dev, "failed to init phy\n");
>> -             goto phy_init_err;
>> -     }
>> -
>> -     ret = ssusb_phy_power_on(ssusb);
>> -     if (ret) {
>> -             dev_err(ssusb->dev, "failed to power on phy\n");
>> -             goto phy_err;
>> -     }
>> -
>>       return 0;
>>
>> -phy_err:
>> -     ssusb_phy_exit(ssusb);
>> -phy_init_err:
>> -     ssusb_clks_disable(ssusb);
>>  clks_err:
>>       regulator_disable(ssusb->vusb33);
>>  vusb33_err:
>> @@ -190,8 +118,6 @@ static void ssusb_rscs_exit(struct ssusb_mtk *ssusb)
>>  {
>>       ssusb_clks_disable(ssusb);
>>       regulator_disable(ssusb->vusb33);
>> -     ssusb_phy_power_off(ssusb);
>> -     ssusb_phy_exit(ssusb);
>>  }
>>
>>  static void ssusb_ip_sw_reset(struct ssusb_mtk *ssusb)
>> @@ -222,7 +148,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
>>       struct device *dev = &pdev->dev;
>>       struct regulator *vbus;
>>       struct resource *res;
>> -     int i;
>>       int ret;
>>
>>       ssusb->vusb33 = devm_regulator_get(&pdev->dev, "vusb33");
>> @@ -249,25 +174,6 @@ static int get_ssusb_rscs(struct platform_device *pdev, struct ssusb_mtk *ssusb)
>>       if (IS_ERR(ssusb->dma_clk))
>>               return PTR_ERR(ssusb->dma_clk);
>>
>> -     ssusb->num_phys = of_count_phandle_with_args(node,
>> -                     "phys", "#phy-cells");
>> -     if (ssusb->num_phys > 0) {
>> -             ssusb->phys = devm_kcalloc(dev, ssusb->num_phys,
>> -                                     sizeof(*ssusb->phys), GFP_KERNEL);
>> -             if (!ssusb->phys)
>> -                     return -ENOMEM;
>> -     } else {
>> -             ssusb->num_phys = 0;
>> -     }
>> -
>> -     for (i = 0; i < ssusb->num_phys; i++) {
>> -             ssusb->phys[i] = devm_of_phy_get_by_index(dev, node, i);
>> -             if (IS_ERR(ssusb->phys[i])) {
>> -                     dev_err(dev, "failed to get phy-%d\n", i);
>> -                     return PTR_ERR(ssusb->phys[i]);
>> -             }
>> -     }
>> -
>>       res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "ippc");
>>       ssusb->ippc_base = devm_ioremap_resource(dev, res);
>>       if (IS_ERR(ssusb->ippc_base))
>> @@ -457,7 +363,6 @@ static int __maybe_unused mtu3_suspend(struct device *dev)
>>               return 0;
>>
>>       ssusb_host_disable(ssusb, true);
>> -     ssusb_phy_power_off(ssusb);
>>       ssusb_clks_disable(ssusb);
>>       ssusb_wakeup_set(ssusb, true);
>>
>> @@ -480,16 +385,10 @@ static int __maybe_unused mtu3_resume(struct device *dev)
>>       if (ret)
>>               goto clks_err;
>>
>> -     ret = ssusb_phy_power_on(ssusb);
>> -     if (ret)
>> -             goto phy_err;
>> -
>>       ssusb_host_enable(ssusb);
>>
>>       return 0;
>>
>> -phy_err:
>> -     ssusb_clks_disable(ssusb);
>>  clks_err:
>>       return ret;
>>  }
> This patch will affect device function when works as device only mode.
> Please abandon it, and I will modify it if need after the patch series
> of "initialize (multiple) PHYs for a HCD" are picked up.
thank you for reviewing this
it seems that I indeed missed the device only mode functionality
I'll drop the patch from my next version and let you handle this
(thank you in advance)!


Regards
Martin

[RFC/RFT usb-next v1 2/6] usb: host: xhci-mtk: remove custom USB PHY handling

[Martin Blumenstingl]

The new PHY wrapper is now wired up in the core HCD code. This means
that PHYs are now controlled (initialized, enabled, disabled, exited)
without requiring any host-driver specific code.
Remove the custom USB PHY handling from the xhci-mtk driver as the core
HCD code now handles this.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/host/xhci-mtk.c | 98 +--------------------------------------------
 1 file changed, 2 insertions(+), 96 deletions(-)

diff --git a/drivers/usb/host/xhci-mtk.c b/drivers/usb/host/xhci-mtk.c
index b0ab4d5e2751..7334da9e9779 100644
--- a/drivers/usb/host/xhci-mtk.c
+++ b/drivers/usb/host/xhci-mtk.c
@@ -14,7 +14,6 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/regmap.h>
@@ -352,62 +351,6 @@ static const struct xhci_driver_overrides xhci_mtk_overrides __initconst = {
 
 static struct hc_driver __read_mostly xhci_mtk_hc_driver;
 
-static int xhci_mtk_phy_init(struct xhci_hcd_mtk *mtk)
-{
-	int i;
-	int ret;
-
-	for (i = 0; i < mtk->num_phys; i++) {
-		ret = phy_init(mtk->phys[i]);
-		if (ret)
-			goto exit_phy;
-	}
-	return 0;
-
-exit_phy:
-	for (; i > 0; i--)
-		phy_exit(mtk->phys[i - 1]);
-
-	return ret;
-}
-
-static int xhci_mtk_phy_exit(struct xhci_hcd_mtk *mtk)
-{
-	int i;
-
-	for (i = 0; i < mtk->num_phys; i++)
-		phy_exit(mtk->phys[i]);
-
-	return 0;
-}
-
-static int xhci_mtk_phy_power_on(struct xhci_hcd_mtk *mtk)
-{
-	int i;
-	int ret;
-
-	for (i = 0; i < mtk->num_phys; i++) {
-		ret = phy_power_on(mtk->phys[i]);
-		if (ret)
-			goto power_off_phy;
-	}
-	return 0;
-
-power_off_phy:
-	for (; i > 0; i--)
-		phy_power_off(mtk->phys[i - 1]);
-
-	return ret;
-}
-
-static void xhci_mtk_phy_power_off(struct xhci_hcd_mtk *mtk)
-{
-	unsigned int i;
-
-	for (i = 0; i < mtk->num_phys; i++)
-		phy_power_off(mtk->phys[i]);
-}
-
 static int xhci_mtk_ldos_enable(struct xhci_hcd_mtk *mtk)
 {
 	int ret;
@@ -488,8 +431,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	struct xhci_hcd *xhci;
 	struct resource *res;
 	struct usb_hcd *hcd;
-	struct phy *phy;
-	int phy_num;
 	int ret = -ENODEV;
 	int irq;
 
@@ -529,16 +470,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 		return ret;
 	}
 
-	mtk->num_phys = of_count_phandle_with_args(node,
-			"phys", "#phy-cells");
-	if (mtk->num_phys > 0) {
-		mtk->phys = devm_kcalloc(dev, mtk->num_phys,
-					sizeof(*mtk->phys), GFP_KERNEL);
-		if (!mtk->phys)
-			return -ENOMEM;
-	} else {
-		mtk->num_phys = 0;
-	}
 	pm_runtime_enable(dev);
 	pm_runtime_get_sync(dev);
 	device_enable_async_suspend(dev);
@@ -596,23 +527,6 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 		mtk->has_ippc = false;
 	}
 
-	for (phy_num = 0; phy_num < mtk->num_phys; phy_num++) {
-		phy = devm_of_phy_get_by_index(dev, node, phy_num);
-		if (IS_ERR(phy)) {
-			ret = PTR_ERR(phy);
-			goto put_usb2_hcd;
-		}
-		mtk->phys[phy_num] = phy;
-	}
-
-	ret = xhci_mtk_phy_init(mtk);
-	if (ret)
-		goto put_usb2_hcd;
-
-	ret = xhci_mtk_phy_power_on(mtk);
-	if (ret)
-		goto exit_phys;
-
 	device_init_wakeup(dev, true);
 
 	xhci = hcd_to_xhci(hcd);
@@ -630,7 +544,7 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 			dev_name(dev), hcd);
 	if (!xhci->shared_hcd) {
 		ret = -ENOMEM;
-		goto power_off_phys;
+		goto disable_device_wakeup;
 	}
 
 	ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
@@ -653,13 +567,9 @@ static int xhci_mtk_probe(struct platform_device *pdev)
 	xhci_mtk_sch_exit(mtk);
 	usb_put_hcd(xhci->shared_hcd);
 
-power_off_phys:
-	xhci_mtk_phy_power_off(mtk);
+disable_device_wakeup:
 	device_init_wakeup(dev, false);
 
-exit_phys:
-	xhci_mtk_phy_exit(mtk);
-
 put_usb2_hcd:
 	usb_put_hcd(hcd);
 
@@ -682,8 +592,6 @@ static int xhci_mtk_remove(struct platform_device *dev)
 	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
 
 	usb_remove_hcd(xhci->shared_hcd);
-	xhci_mtk_phy_power_off(mtk);
-	xhci_mtk_phy_exit(mtk);
 	device_init_wakeup(&dev->dev, false);
 
 	usb_remove_hcd(hcd);
@@ -718,7 +626,6 @@ static int __maybe_unused xhci_mtk_suspend(struct device *dev)
 	del_timer_sync(&xhci->shared_hcd->rh_timer);
 
 	xhci_mtk_host_disable(mtk);
-	xhci_mtk_phy_power_off(mtk);
 	xhci_mtk_clks_disable(mtk);
 	usb_wakeup_set(mtk, true);
 	return 0;
@@ -732,7 +639,6 @@ static int __maybe_unused xhci_mtk_resume(struct device *dev)
 
 	usb_wakeup_set(mtk, false);
 	xhci_mtk_clks_enable(mtk);
-	xhci_mtk_phy_power_on(mtk);
 	xhci_mtk_host_enable(mtk);
 
 	xhci_dbg(xhci, "%s: restart port polling\n", __func__);
-- 
2.16.1

[RFC/RFT usb-next v1 3/6] usb: host: ehci-platform: remove custom USB PHY handling

[Martin Blumenstingl]

The new PHY wrapper is now wired up in the core HCD code. This means
that PHYs are now controlled (initialized, enabled, disabled, exited)
without requiring any host-driver specific code.
Remove the custom USB PHY handling from the ehci-platform driver as the
core HCD code now handles this.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/host/ehci-platform.c | 55 +++-------------------------------------
 1 file changed, 4 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index b065a960adc2..4c306fb6b069 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -27,7 +27,6 @@
 #include <linux/io.h>
 #include <linux/module.h>
 #include <linux/of.h>
-#include <linux/phy/phy.h>
 #include <linux/platform_device.h>
 #include <linux/reset.h>
 #include <linux/usb.h>
@@ -44,8 +43,6 @@
 struct ehci_platform_priv {
 	struct clk *clks[EHCI_MAX_CLKS];
 	struct reset_control *rsts;
-	struct phy **phys;
-	int num_phys;
 	bool reset_on_resume;
 };
 
@@ -80,7 +77,7 @@ static int ehci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
-	int clk, ret, phy_num;
+	int clk, ret;
 
 	for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++) {
 		ret = clk_prepare_enable(priv->clks[clk]);
@@ -88,24 +85,8 @@ static int ehci_platform_power_on(struct platform_device *dev)
 			goto err_disable_clks;
 	}
 
-	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		ret = phy_init(priv->phys[phy_num]);
-		if (ret)
-			goto err_exit_phy;
-		ret = phy_power_on(priv->phys[phy_num]);
-		if (ret) {
-			phy_exit(priv->phys[phy_num]);
-			goto err_exit_phy;
-		}
-	}
-
 	return 0;
 
-err_exit_phy:
-	while (--phy_num >= 0) {
-		phy_power_off(priv->phys[phy_num]);
-		phy_exit(priv->phys[phy_num]);
-	}
 err_disable_clks:
 	while (--clk >= 0)
 		clk_disable_unprepare(priv->clks[clk]);
@@ -117,12 +98,7 @@ static void ehci_platform_power_off(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
-	int clk, phy_num;
-
-	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		phy_power_off(priv->phys[phy_num]);
-		phy_exit(priv->phys[phy_num]);
-	}
+	int clk;
 
 	for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
 		if (priv->clks[clk])
@@ -149,7 +125,7 @@ static int ehci_platform_probe(struct platform_device *dev)
 	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ehci_platform_priv *priv;
 	struct ehci_hcd *ehci;
-	int err, irq, phy_num, clk = 0;
+	int err, irq, clk = 0;
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -202,29 +178,6 @@ static int ehci_platform_probe(struct platform_device *dev)
 					  "has-transaction-translator"))
 			hcd->has_tt = 1;
 
-		priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
-				"phys", "#phy-cells");
-
-		if (priv->num_phys > 0) {
-			priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
-					    sizeof(struct phy *), GFP_KERNEL);
-			if (!priv->phys)
-				return -ENOMEM;
-		} else
-			priv->num_phys = 0;
-
-		for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-			priv->phys[phy_num] = devm_of_phy_get_by_index(
-					&dev->dev, dev->dev.of_node, phy_num);
-			if (IS_ERR(priv->phys[phy_num])) {
-				err = PTR_ERR(priv->phys[phy_num]);
-					goto err_put_hcd;
-			} else if (!hcd->phy) {
-				/* Avoiding phy_get() in usb_add_hcd() */
-				hcd->phy = priv->phys[phy_num];
-			}
-		}
-
 		for (clk = 0; clk < EHCI_MAX_CLKS; clk++) {
 			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
 			if (IS_ERR(priv->clks[clk])) {
@@ -306,7 +259,7 @@ static int ehci_platform_probe(struct platform_device *dev)
 err_put_clks:
 	while (--clk >= 0)
 		clk_put(priv->clks[clk]);
-err_put_hcd:
+
 	if (pdata == &ehci_platform_defaults)
 		dev->dev.platform_data = NULL;
 
-- 
2.16.1

[RFC/RFT usb-next v1 3/6] usb: host: ehci-platform: remove custom USB PHY handling

[Alan Stern]

On Thu, 25 Jan 2018, Martin Blumenstingl wrote:

> The new PHY wrapper is now wired up in the core HCD code. This means
> that PHYs are now controlled (initialized, enabled, disabled, exited)
> without requiring any host-driver specific code.
> Remove the custom USB PHY handling from the ehci-platform driver as the
> core HCD code now handles this.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---

For patches 3/6 and 4/6:

Acked-by: Alan Stern <stern@rowland.harvard.edu>

[RFC/RFT usb-next v1 4/6] usb: host: ohci-platform: remove custom USB PHY handling

[Martin Blumenstingl]

The new PHY wrapper is now wired up in the core HCD code. This means
that PHYs are now controlled (initialized, enabled, disabled, exited)
without requiring any host-driver specific code.
Remove the custom USB PHY handling from the ohci-platform driver as the
core HCD code now handles this.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/host/ohci-platform.c | 56 ++++------------------------------------
 1 file changed, 5 insertions(+), 51 deletions(-)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 1e6c954f4b3f..65a1c3fdc88c 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -21,7 +21,7 @@
 #include <linux/kernel.h>
 #include <linux/module.h>
 #include <linux/err.h>
-#include <linux/phy/phy.h>
+#include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
@@ -38,8 +38,6 @@
 struct ohci_platform_priv {
 	struct clk *clks[OHCI_MAX_CLKS];
 	struct reset_control *resets;
-	struct phy **phys;
-	int num_phys;
 };
 
 static const char hcd_name[] = "ohci-platform";
@@ -48,7 +46,7 @@ static int ohci_platform_power_on(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
-	int clk, ret, phy_num;
+	int clk, ret;
 
 	for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++) {
 		ret = clk_prepare_enable(priv->clks[clk]);
@@ -56,24 +54,8 @@ static int ohci_platform_power_on(struct platform_device *dev)
 			goto err_disable_clks;
 	}
 
-	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		ret = phy_init(priv->phys[phy_num]);
-		if (ret)
-			goto err_exit_phy;
-		ret = phy_power_on(priv->phys[phy_num]);
-		if (ret) {
-			phy_exit(priv->phys[phy_num]);
-			goto err_exit_phy;
-		}
-	}
-
 	return 0;
 
-err_exit_phy:
-	while (--phy_num >= 0) {
-		phy_power_off(priv->phys[phy_num]);
-		phy_exit(priv->phys[phy_num]);
-	}
 err_disable_clks:
 	while (--clk >= 0)
 		clk_disable_unprepare(priv->clks[clk]);
@@ -85,12 +67,7 @@ static void ohci_platform_power_off(struct platform_device *dev)
 {
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct ohci_platform_priv *priv = hcd_to_ohci_priv(hcd);
-	int clk, phy_num;
-
-	for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-		phy_power_off(priv->phys[phy_num]);
-		phy_exit(priv->phys[phy_num]);
-	}
+	int clk;
 
 	for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)
 		if (priv->clks[clk])
@@ -117,7 +94,7 @@ static int ohci_platform_probe(struct platform_device *dev)
 	struct usb_ohci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ohci_platform_priv *priv;
 	struct ohci_hcd *ohci;
-	int err, irq, phy_num, clk = 0;
+	int err, irq, clk = 0;
 
 	if (usb_disabled())
 		return -ENODEV;
@@ -169,29 +146,6 @@ static int ohci_platform_probe(struct platform_device *dev)
 		of_property_read_u32(dev->dev.of_node, "num-ports",
 				     &ohci->num_ports);
 
-		priv->num_phys = of_count_phandle_with_args(dev->dev.of_node,
-				"phys", "#phy-cells");
-
-		if (priv->num_phys > 0) {
-			priv->phys = devm_kcalloc(&dev->dev, priv->num_phys,
-					    sizeof(struct phy *), GFP_KERNEL);
-			if (!priv->phys)
-				return -ENOMEM;
-		} else
-			priv->num_phys = 0;
-
-		for (phy_num = 0; phy_num < priv->num_phys; phy_num++) {
-			priv->phys[phy_num] = devm_of_phy_get_by_index(
-					&dev->dev, dev->dev.of_node, phy_num);
-			if (IS_ERR(priv->phys[phy_num])) {
-				err = PTR_ERR(priv->phys[phy_num]);
-				goto err_put_hcd;
-			} else if (!hcd->phy) {
-				/* Avoiding phy_get() in usb_add_hcd() */
-				hcd->phy = priv->phys[phy_num];
-			}
-		}
-
 		for (clk = 0; clk < OHCI_MAX_CLKS; clk++) {
 			priv->clks[clk] = of_clk_get(dev->dev.of_node, clk);
 			if (IS_ERR(priv->clks[clk])) {
@@ -277,7 +231,7 @@ static int ohci_platform_probe(struct platform_device *dev)
 err_put_clks:
 	while (--clk >= 0)
 		clk_put(priv->clks[clk]);
-err_put_hcd:
+
 	if (pdata == &ohci_platform_defaults)
 		dev->dev.platform_data = NULL;
 
-- 
2.16.1

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Martin Blumenstingl]

Now that usb_add_hcd parses all generic PHYs anyways the code which
skips initialization of a single PHY will go away.
Remove the code which sets struct usb_hcd's phy field from the chipidea
driver as this field will go away soon.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/chipidea/host.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
index 19d60ed7e41f..fc324767cb0f 100644
--- a/drivers/usb/chipidea/host.c
+++ b/drivers/usb/chipidea/host.c
@@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
 
 	hcd->power_budget = ci->platdata->power_budget;
 	hcd->tpl_support = ci->platdata->tpl_support;
-	if (ci->phy)
-		hcd->phy = ci->phy;
-	else
+	if (!ci->phy)
 		hcd->usb_phy = ci->usb_phy;
 
 	ehci = hcd_to_ehci(hcd);
-- 
2.16.1

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> 
> Now that usb_add_hcd parses all generic PHYs anyways the code which skips
> initialization of a single PHY will go away.
> Remove the code which sets struct usb_hcd's phy field from the chipidea driver as
> this field will go away soon.
> 
> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
> ---
>  drivers/usb/chipidea/host.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index
> 19d60ed7e41f..fc324767cb0f 100644
> --- a/drivers/usb/chipidea/host.c
> +++ b/drivers/usb/chipidea/host.c
> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
> 
>  	hcd->power_budget = ci->platdata->power_budget;
>  	hcd->tpl_support = ci->platdata->tpl_support;
> -	if (ci->phy)
> -		hcd->phy = ci->phy;
> -	else
> +	if (!ci->phy)
>  		hcd->usb_phy = ci->usb_phy;
> 

The reason hcd->phy is initialized by chipidea core is we do not need HCD core to
touch PHY, and PHY operation is shared for both device and host mode for chipidea.

If I understand correct, your HCD core PHY wrapper patch set will do PHY operation if
there is a "phy" node under controller's? If it is correct, you may supply one way to let
the HCD core bypass phy operations for some USB controllers, eg dual-role controllers.
Thanks.

Peter

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Martin Blumenstingl]

Hi Peter,

On Fri, Jan 26, 2018 at 10:06 AM, Peter Chen <peter.chen@nxp.com> wrote:
>
>>
>> Now that usb_add_hcd parses all generic PHYs anyways the code which skips
>> initialization of a single PHY will go away.
>> Remove the code which sets struct usb_hcd's phy field from the chipidea driver as
>> this field will go away soon.
>>
>> Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
>> ---
>>  drivers/usb/chipidea/host.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index
>> 19d60ed7e41f..fc324767cb0f 100644
>> --- a/drivers/usb/chipidea/host.c
>> +++ b/drivers/usb/chipidea/host.c
>> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
>>
>>       hcd->power_budget = ci->platdata->power_budget;
>>       hcd->tpl_support = ci->platdata->tpl_support;
>> -     if (ci->phy)
>> -             hcd->phy = ci->phy;
>> -     else
>> +     if (!ci->phy)
>>               hcd->usb_phy = ci->usb_phy;
>>
>
> The reason hcd->phy is initialized by chipidea core is we do not need HCD core to
> touch PHY, and PHY operation is shared for both device and host mode for chipidea.
Chunfeng wanted me to drop the mtu3 patch because I forgot about
device mode in the mtu3 driver.
however, the chipidea driver seems to be different because I'm not
dropping the whole phy_{init,power_on,power_off,exit} code from it,
but only a "flag" that tells the HCD core to skip managing the USB PHY

> If I understand correct, your HCD core PHY wrapper patch set will do PHY operation if
> there is a "phy" node under controller's? If it is correct, you may supply one way to let
> the HCD core bypass phy operations for some USB controllers, eg dual-role controllers.
> Thanks.
could you please explain why the HCD core should not manage the the
PHYs when the chipidea driver is used?

my understanding is that all phy_{init,power_on,power_off,exit}
operations are ref-counted internally in the PHY framework
this means that if the chipidea driver calls phy_{init,power_on} first
then the same functions called from within the HCD core are no-ops
(except for the ref-counting)
so I think it should not change anything - however, I have no hardware
to actually prove that.
it would be great if you could explain the issue behind this (and
thereby answer the question: why we would not want the HCD core to
manage the PHY states)!


Regards
Martin

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> >
> >>
> >> Now that usb_add_hcd parses all generic PHYs anyways the code which
> >> skips initialization of a single PHY will go away.
> >> Remove the code which sets struct usb_hcd's phy field from the
> >> chipidea driver as this field will go away soon.
> >>
> >> Signed-off-by: Martin Blumenstingl
> >> <martin.blumenstingl@googlemail.com>
> >> ---
> >>  drivers/usb/chipidea/host.c | 4 +---
> >>  1 file changed, 1 insertion(+), 3 deletions(-)
> >>
> >> diff --git a/drivers/usb/chipidea/host.c
> >> b/drivers/usb/chipidea/host.c index 19d60ed7e41f..fc324767cb0f 100644
> >> --- a/drivers/usb/chipidea/host.c
> >> +++ b/drivers/usb/chipidea/host.c
> >> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
> >>
> >>       hcd->power_budget = ci->platdata->power_budget;
> >>       hcd->tpl_support = ci->platdata->tpl_support;
> >> -     if (ci->phy)
> >> -             hcd->phy = ci->phy;
> >> -     else
> >> +     if (!ci->phy)
> >>               hcd->usb_phy = ci->usb_phy;
> >>
> >
> > The reason hcd->phy is initialized by chipidea core is we do not need
> > HCD core to touch PHY, and PHY operation is shared for both device and host
> mode for chipidea.
> Chunfeng wanted me to drop the mtu3 patch because I forgot about device mode in
> the mtu3 driver.
> however, the chipidea driver seems to be different because I'm not dropping the
> whole phy_{init,power_on,power_off,exit} code from it, but only a "flag" that tells the
> HCD core to skip managing the USB PHY
> 
> > If I understand correct, your HCD core PHY wrapper patch set will do
> > PHY operation if there is a "phy" node under controller's? If it is
> > correct, you may supply one way to let the HCD core bypass phy operations for
> some USB controllers, eg dual-role controllers.
> > Thanks.
> could you please explain why the HCD core should not manage the the PHYs when
> the chipidea driver is used?
> 
> my understanding is that all phy_{init,power_on,power_off,exit}
> operations are ref-counted internally in the PHY framework this means that if the
> chipidea driver calls phy_{init,power_on} first then the same functions called from
> within the HCD core are no-ops (except for the ref-counting) so I think it should not
> change anything - however, I have no hardware to actually prove that.

Martin, you design has no problem for most of cases, but some hardware needs special
sequence for phy control. I will give an example below.

> it would be great if you could explain the issue behind this (and thereby answer the
> question: why we would not want the HCD core to manage the PHY states)!
> 
> 
Eg, taking Qualcomm USB2 controller as an example, it even does not allow chipidea core
to manage its power operation, see CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea driver
(usb/chipidea/core.c). Its phy_power_on is called after ehci controller reset has finished. 
(usb/chipidea/ci_hdrc_msm.c).

Peter

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Martin Blumenstingl]

Hi Peter,

On Mon, Jan 29, 2018 at 4:30 AM, Peter Chen <peter.chen@nxp.com> wrote:
>
>> >
>> >>
>> >> Now that usb_add_hcd parses all generic PHYs anyways the code which
>> >> skips initialization of a single PHY will go away.
>> >> Remove the code which sets struct usb_hcd's phy field from the
>> >> chipidea driver as this field will go away soon.
>> >>
>> >> Signed-off-by: Martin Blumenstingl
>> >> <martin.blumenstingl@googlemail.com>
>> >> ---
>> >>  drivers/usb/chipidea/host.c | 4 +---
>> >>  1 file changed, 1 insertion(+), 3 deletions(-)
>> >>
>> >> diff --git a/drivers/usb/chipidea/host.c
>> >> b/drivers/usb/chipidea/host.c index 19d60ed7e41f..fc324767cb0f 100644
>> >> --- a/drivers/usb/chipidea/host.c
>> >> +++ b/drivers/usb/chipidea/host.c
>> >> @@ -124,9 +124,7 @@ static int host_start(struct ci_hdrc *ci)
>> >>
>> >>       hcd->power_budget = ci->platdata->power_budget;
>> >>       hcd->tpl_support = ci->platdata->tpl_support;
>> >> -     if (ci->phy)
>> >> -             hcd->phy = ci->phy;
>> >> -     else
>> >> +     if (!ci->phy)
>> >>               hcd->usb_phy = ci->usb_phy;
>> >>
>> >
>> > The reason hcd->phy is initialized by chipidea core is we do not need
>> > HCD core to touch PHY, and PHY operation is shared for both device and host
>> mode for chipidea.
>> Chunfeng wanted me to drop the mtu3 patch because I forgot about device mode in
>> the mtu3 driver.
>> however, the chipidea driver seems to be different because I'm not dropping the
>> whole phy_{init,power_on,power_off,exit} code from it, but only a "flag" that tells the
>> HCD core to skip managing the USB PHY
>>
>> > If I understand correct, your HCD core PHY wrapper patch set will do
>> > PHY operation if there is a "phy" node under controller's? If it is
>> > correct, you may supply one way to let the HCD core bypass phy operations for
>> some USB controllers, eg dual-role controllers.
>> > Thanks.
>> could you please explain why the HCD core should not manage the the PHYs when
>> the chipidea driver is used?
>>
>> my understanding is that all phy_{init,power_on,power_off,exit}
>> operations are ref-counted internally in the PHY framework this means that if the
>> chipidea driver calls phy_{init,power_on} first then the same functions called from
>> within the HCD core are no-ops (except for the ref-counting) so I think it should not
>> change anything - however, I have no hardware to actually prove that.
>
> Martin, you design has no problem for most of cases, but some hardware needs special
> sequence for phy control. I will give an example below.
great to hear that this should work for most devices!

>> it would be great if you could explain the issue behind this (and thereby answer the
>> question: why we would not want the HCD core to manage the PHY states)!
>>
>>
> Eg, taking Qualcomm USB2 controller as an example, it even does not allow chipidea core
> to manage its power operation, see CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea driver
> (usb/chipidea/core.c). Its phy_power_on is called after ehci controller reset has finished.
> (usb/chipidea/ci_hdrc_msm.c).
I see, thank you for explaining this!

what do you think about replacing the two following fields from struct usb_hcd:
  struct usb_phy *usb_phy;
  struct phy *phy;
with:
  /*
   * do not manage the PHY state in the HCD core, instead let the driver handle
   * this (for example if the PHY can only be turned on after a specific event)
   */
  bool skip_phy_initialization;

maybe I should also do this together with my other series which adds
the PHY wrapper to the HCD core (or even as a separate series, which
would be merged before this and the PHY wrapper series). what do you
think?


Regards
Martin

[RFC/RFT usb-next v1 5/6] usb: chipidea: do not set the "phy" field in struct usb_hcd

[Peter Chen]

 
> > Martin, you design has no problem for most of cases, but some hardware
> > needs special sequence for phy control. I will give an example below.
> great to hear that this should work for most devices!
> 
> >> it would be great if you could explain the issue behind this (and
> >> thereby answer the
> >> question: why we would not want the HCD core to manage the PHY states)!
> >>
> >>
> > Eg, taking Qualcomm USB2 controller as an example, it even does not
> > allow chipidea core to manage its power operation, see
> > CI_HDRC_OVERRIDE_PHY_CONTROL at chipidea driver (usb/chipidea/core.c).
> Its phy_power_on is called after ehci controller reset has finished.
> > (usb/chipidea/ci_hdrc_msm.c).
> I see, thank you for explaining this!
> 
> what do you think about replacing the two following fields from struct usb_hcd:
>   struct usb_phy *usb_phy;
>   struct phy *phy;
> with:
>   /*
>    * do not manage the PHY state in the HCD core, instead let the driver handle
>    * this (for example if the PHY can only be turned on after a specific event)
>    */
>   bool skip_phy_initialization;
> 
> maybe I should also do this together with my other series which adds the PHY
> wrapper to the HCD core (or even as a separate series, which would be merged
> before this and the PHY wrapper series). what do you think?
> 
> 
 
Hi Martin,

I think it is better to do this in one series.

Peter

[RFC/RFT usb-next v1 6/6] usb: core: hcd: remove support for initializing a single PHY

[Martin Blumenstingl]

With the new PHY wrapper in place we can now handle multiple PHYs.
Remove the code which handles only one generic PHY as this is now
covered (with support for multiple PHYs as well as suspend/resume
support) by the new PHY wrapper.

Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
 drivers/usb/core/hcd.c  | 37 -------------------------------------
 include/linux/usb/hcd.h |  1 -
 2 files changed, 38 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index fc99cddc117e..5d59e0b4d463 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2757,30 +2757,6 @@ int usb_add_hcd(struct usb_hcd *hcd,
 		}
 	}
 
-	if (IS_ENABLED(CONFIG_GENERIC_PHY) && !hcd->phy) {
-		struct phy *phy = phy_get(hcd->self.sysdev, "usb");
-
-		if (IS_ERR(phy)) {
-			retval = PTR_ERR(phy);
-			if (retval == -EPROBE_DEFER)
-				goto err_phy;
-		} else {
-			retval = phy_init(phy);
-			if (retval) {
-				phy_put(phy);
-				goto err_phy;
-			}
-			retval = phy_power_on(phy);
-			if (retval) {
-				phy_exit(phy);
-				phy_put(phy);
-				goto err_phy;
-			}
-			hcd->phy = phy;
-			hcd->remove_phy = 1;
-		}
-	}
-
 	hcd->phy_roothub = usb_phy_roothub_init(hcd->self.sysdev);
 	if (IS_ERR(hcd->phy_roothub)) {
 		retval = PTR_ERR(hcd->phy_roothub);
@@ -2959,13 +2935,6 @@ int usb_add_hcd(struct usb_hcd *hcd,
 err_usb_phy_roothub_power_on:
 	usb_phy_roothub_exit(hcd->phy_roothub);
 err_phy_roothub_init:
-	if (IS_ENABLED(CONFIG_GENERIC_PHY) && hcd->remove_phy && hcd->phy) {
-		phy_power_off(hcd->phy);
-		phy_exit(hcd->phy);
-		phy_put(hcd->phy);
-		hcd->phy = NULL;
-	}
-err_phy:
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);
@@ -3046,12 +3015,6 @@ void usb_remove_hcd(struct usb_hcd *hcd)
 	usb_phy_roothub_power_off(hcd->phy_roothub);
 	usb_phy_roothub_exit(hcd->phy_roothub);
 
-	if (IS_ENABLED(CONFIG_GENERIC_PHY) && hcd->remove_phy && hcd->phy) {
-		phy_power_off(hcd->phy);
-		phy_exit(hcd->phy);
-		phy_put(hcd->phy);
-		hcd->phy = NULL;
-	}
 	if (hcd->remove_phy && hcd->usb_phy) {
 		usb_phy_shutdown(hcd->usb_phy);
 		usb_put_phy(hcd->usb_phy);
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 9e8fc9c5f394..e464c7384bd5 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -103,7 +103,6 @@ struct usb_hcd {
 	 * other external phys should be software-transparent
 	 */
 	struct usb_phy		*usb_phy;
-	struct phy		*phy;
 	struct usb_phy_roothub	*phy_roothub;
 
 	/* Flags that need to be manipulated atomically because they can
-- 
2.16.1