[PATCH v1 0/4] NVIDIA Tegra USB2 drivers clean up

[PATCH v1 0/4] NVIDIA Tegra USB2 drivers clean up

[Dmitry Osipenko]

Hello,

This small patch series brings the NVIDIA Tegra USB2 PHY driver into a better
shape by refactoring code to match upstream standards, the ChipIdea/Tegra UDC
driver also gets a minor update. Please review and apply, thanks in advance!

Dmitry Osipenko (4):
  dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support
  usb: phy: tegra: Hook up init/shutdown callbacks
  usb: phy: tegra: Perform general clean up of the code
  usb: phy: tegra: Use relaxed versions of readl/writel

 .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |   4 +
 drivers/usb/chipidea/ci_hdrc_tegra.c          |   9 -
 drivers/usb/phy/phy-tegra-usb.c               | 715 ++++++++++--------
 3 files changed, 395 insertions(+), 333 deletions(-)

-- 
2.24.0

[PATCH v1 1/4] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support

[Dmitry Osipenko]

NVIDIA Tegra SoCs use ChipIdea silicon IP for the USB controllers.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index cfc9f40ab641..51376cbe5f3d 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -15,6 +15,10 @@ Required properties:
 	"qcom,ci-hdrc"
 	"chipidea,usb2"
 	"xlnx,zynq-usb-2.20a"
+	"nvidia,tegra20-udc"
+	"nvidia,tegra30-udc"
+	"nvidia,tegra114-udc"
+	"nvidia,tegra124-udc"
 - reg: base address and length of the registers
 - interrupts: interrupt for the USB controller
 
-- 
2.24.0

Re: [PATCH v1 1/4] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support

[Peter Chen]

On 19-12-18 20:53:10, Dmitry Osipenko wrote:
> NVIDIA Tegra SoCs use ChipIdea silicon IP for the USB controllers.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index cfc9f40ab641..51376cbe5f3d 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -15,6 +15,10 @@ Required properties:
>  	"qcom,ci-hdrc"
>  	"chipidea,usb2"
>  	"xlnx,zynq-usb-2.20a"
> +	"nvidia,tegra20-udc"
> +	"nvidia,tegra30-udc"
> +	"nvidia,tegra114-udc"
> +	"nvidia,tegra124-udc"
>  - reg: base address and length of the registers
>  - interrupts: interrupt for the USB controller
>  

Acked-by: Peter Chen <peter.chen@nxp.com>

-- 

Thanks,
Peter Chen

Re: [PATCH v1 1/4] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support

[Thierry Reding]

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

On Wed, Dec 18, 2019 at 08:53:10PM +0300, Dmitry Osipenko wrote:
> NVIDIA Tegra SoCs use ChipIdea silicon IP for the USB controllers.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 4 ++++
>  1 file changed, 4 insertions(+)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks

[Dmitry Osipenko]

Generic PHY provides init/shutdown callbacks which allow USB-host drivers
to abstract PHY's hardware management in a common way. This change allows
to remove Tegra-specific PHY handling from the ChipIdea driver.

Note that ChipIdea's driver shall be changed at the same time because it
turns PHY ON without the PHY's initialization and this doesn't work now,
resulting in a NULL dereference of phy->freq because it's set during of
the PHY's initialization.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c |   9 --
 drivers/usb/phy/phy-tegra-usb.c      | 165 +++++++++++++++++----------
 2 files changed, 102 insertions(+), 72 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
index 0c9911d44ee5..7455df0ede49 100644
--- a/drivers/usb/chipidea/ci_hdrc_tegra.c
+++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
@@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
 		return err;
 	}
 
-	/*
-	 * Tegra's USB PHY driver doesn't implement optional phy_init()
-	 * hook, so we have to power on UDC controller before ChipIdea
-	 * driver initialization kicks in.
-	 */
-	usb_phy_set_suspend(udc->phy, 0);
-
 	/* setup and register ChipIdea HDRC device */
 	udc->data.name = "tegra-udc";
 	udc->data.flags = soc->flags;
@@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
 	return 0;
 
 fail_power_off:
-	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 	return err;
 }
@@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
 	struct tegra_udc *udc = platform_get_drvdata(pdev);
 
 	ci_hdrc_remove_device(udc->dev);
-	usb_phy_set_suspend(udc->phy, 1);
 	clk_disable_unprepare(udc->clk);
 
 	return 0;
diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index ea7ef1dc0b42..15bd253d53c9 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
 {
 	int ret;
 
-	phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
-	if (IS_ERR(phy->pad_clk)) {
-		ret = PTR_ERR(phy->pad_clk);
-		dev_err(phy->u_phy.dev,
-			"Failed to get UTMIP pad clock: %d\n", ret);
-		return ret;
-	}
-
-	phy->pad_rst = devm_reset_control_get_optional_shared(
-						phy->u_phy.dev, "utmi-pads");
-	if (IS_ERR(phy->pad_rst)) {
-		ret = PTR_ERR(phy->pad_rst);
-		dev_err(phy->u_phy.dev,
-			"Failed to get UTMI-pads reset: %d\n", ret);
-		return ret;
-	}
-
 	ret = clk_prepare_enable(phy->pad_clk);
 	if (ret) {
 		dev_err(phy->u_phy.dev,
@@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
 	return ret;
 }
 
+static void ulpi_close(struct tegra_usb_phy *phy)
+{
+	int err;
+
+	err = gpio_direction_output(phy->reset_gpio, 1);
+	if (err < 0) {
+		dev_err(phy->u_phy.dev,
+			"ULPI reset GPIO %d direction not asserted: %d\n",
+			phy->reset_gpio, err);
+	}
+}
+
 static void utmip_pad_power_on(struct tegra_usb_phy *phy)
 {
 	unsigned long val, flags;
@@ -761,14 +756,19 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
 	return gpio_direction_output(phy->reset_gpio, 0);
 }
 
-static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
+static void tegra_usb_phy_close(struct usb_phy *u_phy)
 {
-	if (!IS_ERR(phy->vbus))
-		regulator_disable(phy->vbus);
+	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
+						 u_phy);
 
-	if (!phy->is_ulpi_phy)
+	if (phy->is_ulpi_phy)
+		ulpi_close(phy);
+	else
 		utmip_pad_close(phy);
 
+	if (!IS_ERR(phy->vbus))
+		regulator_disable(phy->vbus);
+
 	clk_disable_unprepare(phy->pll_u);
 }
 
@@ -788,7 +788,7 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
 		return utmi_phy_power_off(phy);
 }
 
-static int	tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
+static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
 {
 	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
 	if (suspend)
@@ -801,54 +801,25 @@ static int ulpi_open(struct tegra_usb_phy *phy)
 {
 	int err;
 
-	phy->clk = devm_clk_get(phy->u_phy.dev, "ulpi-link");
-	if (IS_ERR(phy->clk)) {
-		err = PTR_ERR(phy->clk);
-		dev_err(phy->u_phy.dev, "Failed to get ULPI clock: %d\n", err);
-		return err;
-	}
-
-	err = devm_gpio_request(phy->u_phy.dev, phy->reset_gpio,
-		"ulpi_phy_reset_b");
-	if (err < 0) {
-		dev_err(phy->u_phy.dev, "Request failed for GPIO %d: %d\n",
-			phy->reset_gpio, err);
-		return err;
-	}
-
 	err = gpio_direction_output(phy->reset_gpio, 0);
 	if (err < 0) {
 		dev_err(phy->u_phy.dev,
-			"GPIO %d direction not set to output: %d\n",
+			"ULPI reset GPIO %d direction not deasserted: %d\n",
 			phy->reset_gpio, err);
 		return err;
 	}
 
-	phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
-	if (!phy->ulpi) {
-		dev_err(phy->u_phy.dev, "Failed to create ULPI OTG\n");
-		err = -ENOMEM;
-		return err;
-	}
-
-	phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
 	return 0;
 }
 
-static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
+static int tegra_usb_phy_init(struct usb_phy *u_phy)
 {
+	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
+						 u_phy);
 	unsigned long parent_rate;
 	int i;
 	int err;
 
-	phy->pll_u = devm_clk_get(phy->u_phy.dev, "pll_u");
-	if (IS_ERR(phy->pll_u)) {
-		err = PTR_ERR(phy->pll_u);
-		dev_err(phy->u_phy.dev,
-			"Failed to get pll_u clock: %d\n", err);
-		return err;
-	}
-
 	err = clk_prepare_enable(phy->pll_u);
 	if (err)
 		return err;
@@ -884,8 +855,17 @@ static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
 	if (err < 0)
 		goto fail;
 
+	err = tegra_usb_phy_power_on(phy);
+	if (err)
+		goto close_phy;
+
 	return 0;
 
+close_phy:
+	if (phy->is_ulpi_phy)
+		ulpi_close(phy);
+	else
+		utmip_pad_close(phy);
 fail:
 	clk_disable_unprepare(phy->pll_u);
 	return err;
@@ -1134,22 +1114,77 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 		tegra_phy->vbus = ERR_PTR(-ENODEV);
 	}
 
-	tegra_phy->u_phy.dev = &pdev->dev;
-	err = tegra_usb_phy_init(tegra_phy);
-	if (err < 0)
+	tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
+	err = PTR_ERR_OR_ZERO(tegra_phy);
+	if (err) {
+		dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err);
 		return err;
+	}
 
+	if (tegra_phy->is_ulpi_phy) {
+		tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link");
+		err = PTR_ERR_OR_ZERO(tegra_phy->clk);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to get ULPI clock: %d\n", err);
+			return err;
+		}
+
+		err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
+			"ulpi_phy_reset_b");
+		if (err < 0) {
+			dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
+				tegra_phy->reset_gpio, err);
+			return err;
+		}
+
+		tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
+		if (!tegra_phy->ulpi) {
+			dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
+			err = -ENOMEM;
+			return err;
+		}
+
+		tegra_phy->ulpi->io_priv = tegra_phy->regs + ULPI_VIEWPORT;
+	} else {
+		tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads");
+		err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to get UTMIP pad clock: %d\n", err);
+			return err;
+		}
+
+		tegra_phy->pad_rst = devm_reset_control_get_optional_shared(
+						&pdev->dev, "utmi-pads");
+		err = PTR_ERR_OR_ZERO(tegra_phy->pad_rst);
+		if (err) {
+			dev_err(&pdev->dev,
+				"Failed to get UTMI-pads reset: %d\n", err);
+			return err;
+		}
+	}
+
+	tegra_phy->u_phy.dev = &pdev->dev;
+	tegra_phy->u_phy.init = tegra_usb_phy_init;
+	tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
 	tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
 
 	platform_set_drvdata(pdev, tegra_phy);
 
 	err = usb_add_phy_dev(&tegra_phy->u_phy);
-	if (err < 0) {
-		tegra_usb_phy_close(tegra_phy);
-		return err;
-	}
+	if (err < 0)
+		goto free_ulpi;
 
 	return 0;
+
+free_ulpi:
+	if (tegra_phy->ulpi) {
+		kfree(tegra_phy->ulpi->otg);
+		kfree(tegra_phy->ulpi);
+	}
+
+	return err;
 }
 
 static int tegra_usb_phy_remove(struct platform_device *pdev)
@@ -1157,7 +1192,11 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
 	struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
 
 	usb_remove_phy(&tegra_phy->u_phy);
-	tegra_usb_phy_close(tegra_phy);
+
+	if (tegra_phy->ulpi) {
+		kfree(tegra_phy->ulpi->otg);
+		kfree(tegra_phy->ulpi);
+	}
 
 	return 0;
 }
-- 
2.24.0

Re: [PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks

[Peter Chen]

On 19-12-18 20:53:11, Dmitry Osipenko wrote:
> Generic PHY provides init/shutdown callbacks which allow USB-host drivers
> to abstract PHY's hardware management in a common way. This change allows
> to remove Tegra-specific PHY handling from the ChipIdea driver.
> 
> Note that ChipIdea's driver shall be changed at the same time because it
> turns PHY ON without the PHY's initialization and this doesn't work now,
> resulting in a NULL dereference of phy->freq because it's set during of
> the PHY's initialization.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_tegra.c |   9 --
>  drivers/usb/phy/phy-tegra-usb.c      | 165 +++++++++++++++++----------
>  2 files changed, 102 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 0c9911d44ee5..7455df0ede49 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	/*
> -	 * Tegra's USB PHY driver doesn't implement optional phy_init()
> -	 * hook, so we have to power on UDC controller before ChipIdea
> -	 * driver initialization kicks in.
> -	 */
> -	usb_phy_set_suspend(udc->phy, 0);
> -
>  	/* setup and register ChipIdea HDRC device */
>  	udc->data.name = "tegra-udc";
>  	udc->data.flags = soc->flags;
> @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_power_off:
> -	usb_phy_set_suspend(udc->phy, 1);
>  	clk_disable_unprepare(udc->clk);
>  	return err;
>  }
> @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
>  	struct tegra_udc *udc = platform_get_drvdata(pdev);
>  
>  	ci_hdrc_remove_device(udc->dev);
> -	usb_phy_set_suspend(udc->phy, 1);
>  	clk_disable_unprepare(udc->clk);
>  
>  	return 0;
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index ea7ef1dc0b42..15bd253d53c9 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  {
>  	int ret;
>  
> -	phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
> -	if (IS_ERR(phy->pad_clk)) {
> -		ret = PTR_ERR(phy->pad_clk);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get UTMIP pad clock: %d\n", ret);
> -		return ret;
> -	}
> -
> -	phy->pad_rst = devm_reset_control_get_optional_shared(
> -						phy->u_phy.dev, "utmi-pads");
> -	if (IS_ERR(phy->pad_rst)) {
> -		ret = PTR_ERR(phy->pad_rst);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get UTMI-pads reset: %d\n", ret);
> -		return ret;
> -	}
> -
>  	ret = clk_prepare_enable(phy->pad_clk);
>  	if (ret) {
>  		dev_err(phy->u_phy.dev,
> @@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
>  	return ret;
>  }

Acked-by: Peter Chen <peter.chen@nxp.com>

Felipe, would you please queue this series after reviewing for USB PHY
changes? If not, Dmitry may need to split the patch.

Peter
>  
> +static void ulpi_close(struct tegra_usb_phy *phy)
> +{
> +	int err;
> +
> +	err = gpio_direction_output(phy->reset_gpio, 1);
> +	if (err < 0) {
> +		dev_err(phy->u_phy.dev,
> +			"ULPI reset GPIO %d direction not asserted: %d\n",
> +			phy->reset_gpio, err);
> +	}
> +}
> +
>  static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>  {
>  	unsigned long val, flags;
> @@ -761,14 +756,19 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
>  	return gpio_direction_output(phy->reset_gpio, 0);
>  }
>  
> -static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
> +static void tegra_usb_phy_close(struct usb_phy *u_phy)
>  {
> -	if (!IS_ERR(phy->vbus))
> -		regulator_disable(phy->vbus);
> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> +						 u_phy);
>  
> -	if (!phy->is_ulpi_phy)
> +	if (phy->is_ulpi_phy)
> +		ulpi_close(phy);
> +	else
>  		utmip_pad_close(phy);
>  
> +	if (!IS_ERR(phy->vbus))
> +		regulator_disable(phy->vbus);
> +
>  	clk_disable_unprepare(phy->pll_u);
>  }
>  
> @@ -788,7 +788,7 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
>  		return utmi_phy_power_off(phy);
>  }
>  
> -static int	tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
> +static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
>  {
>  	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
>  	if (suspend)
> @@ -801,54 +801,25 @@ static int ulpi_open(struct tegra_usb_phy *phy)
>  {
>  	int err;
>  
> -	phy->clk = devm_clk_get(phy->u_phy.dev, "ulpi-link");
> -	if (IS_ERR(phy->clk)) {
> -		err = PTR_ERR(phy->clk);
> -		dev_err(phy->u_phy.dev, "Failed to get ULPI clock: %d\n", err);
> -		return err;
> -	}
> -
> -	err = devm_gpio_request(phy->u_phy.dev, phy->reset_gpio,
> -		"ulpi_phy_reset_b");
> -	if (err < 0) {
> -		dev_err(phy->u_phy.dev, "Request failed for GPIO %d: %d\n",
> -			phy->reset_gpio, err);
> -		return err;
> -	}
> -
>  	err = gpio_direction_output(phy->reset_gpio, 0);
>  	if (err < 0) {
>  		dev_err(phy->u_phy.dev,
> -			"GPIO %d direction not set to output: %d\n",
> +			"ULPI reset GPIO %d direction not deasserted: %d\n",
>  			phy->reset_gpio, err);
>  		return err;
>  	}
>  
> -	phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> -	if (!phy->ulpi) {
> -		dev_err(phy->u_phy.dev, "Failed to create ULPI OTG\n");
> -		err = -ENOMEM;
> -		return err;
> -	}
> -
> -	phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
>  	return 0;
>  }
>  
> -static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
> +static int tegra_usb_phy_init(struct usb_phy *u_phy)
>  {
> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> +						 u_phy);
>  	unsigned long parent_rate;
>  	int i;
>  	int err;
>  
> -	phy->pll_u = devm_clk_get(phy->u_phy.dev, "pll_u");
> -	if (IS_ERR(phy->pll_u)) {
> -		err = PTR_ERR(phy->pll_u);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get pll_u clock: %d\n", err);
> -		return err;
> -	}
> -
>  	err = clk_prepare_enable(phy->pll_u);
>  	if (err)
>  		return err;
> @@ -884,8 +855,17 @@ static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
>  	if (err < 0)
>  		goto fail;
>  
> +	err = tegra_usb_phy_power_on(phy);
> +	if (err)
> +		goto close_phy;
> +
>  	return 0;
>  
> +close_phy:
> +	if (phy->is_ulpi_phy)
> +		ulpi_close(phy);
> +	else
> +		utmip_pad_close(phy);
>  fail:
>  	clk_disable_unprepare(phy->pll_u);
>  	return err;
> @@ -1134,22 +1114,77 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
>  		tegra_phy->vbus = ERR_PTR(-ENODEV);
>  	}
>  
> -	tegra_phy->u_phy.dev = &pdev->dev;
> -	err = tegra_usb_phy_init(tegra_phy);
> -	if (err < 0)
> +	tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
> +	err = PTR_ERR_OR_ZERO(tegra_phy);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err);
>  		return err;
> +	}
>  
> +	if (tegra_phy->is_ulpi_phy) {
> +		tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->clk);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get ULPI clock: %d\n", err);
> +			return err;
> +		}
> +
> +		err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
> +			"ulpi_phy_reset_b");
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
> +				tegra_phy->reset_gpio, err);
> +			return err;
> +		}
> +
> +		tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> +		if (!tegra_phy->ulpi) {
> +			dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
> +			err = -ENOMEM;
> +			return err;
> +		}
> +
> +		tegra_phy->ulpi->io_priv = tegra_phy->regs + ULPI_VIEWPORT;
> +	} else {
> +		tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get UTMIP pad clock: %d\n", err);
> +			return err;
> +		}
> +
> +		tegra_phy->pad_rst = devm_reset_control_get_optional_shared(
> +						&pdev->dev, "utmi-pads");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_rst);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get UTMI-pads reset: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	tegra_phy->u_phy.dev = &pdev->dev;
> +	tegra_phy->u_phy.init = tegra_usb_phy_init;
> +	tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
>  	tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
>  
>  	platform_set_drvdata(pdev, tegra_phy);
>  
>  	err = usb_add_phy_dev(&tegra_phy->u_phy);
> -	if (err < 0) {
> -		tegra_usb_phy_close(tegra_phy);
> -		return err;
> -	}
> +	if (err < 0)
> +		goto free_ulpi;
>  
>  	return 0;
> +
> +free_ulpi:
> +	if (tegra_phy->ulpi) {
> +		kfree(tegra_phy->ulpi->otg);
> +		kfree(tegra_phy->ulpi);
> +	}
> +
> +	return err;
>  }
>  
>  static int tegra_usb_phy_remove(struct platform_device *pdev)
> @@ -1157,7 +1192,11 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
>  	struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
>  
>  	usb_remove_phy(&tegra_phy->u_phy);
> -	tegra_usb_phy_close(tegra_phy);
> +
> +	if (tegra_phy->ulpi) {
> +		kfree(tegra_phy->ulpi->otg);
> +		kfree(tegra_phy->ulpi);
> +	}
>  
>  	return 0;
>  }
> -- 
> 2.24.0
> 

-- 

Thanks,
Peter Chen

Re: [PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks

[Dmitry Osipenko]

19.12.2019 09:56, Peter Chen пишет:
> On 19-12-18 20:53:11, Dmitry Osipenko wrote:
>> Generic PHY provides init/shutdown callbacks which allow USB-host drivers
>> to abstract PHY's hardware management in a common way. This change allows
>> to remove Tegra-specific PHY handling from the ChipIdea driver.
>>
>> Note that ChipIdea's driver shall be changed at the same time because it
>> turns PHY ON without the PHY's initialization and this doesn't work now,
>> resulting in a NULL dereference of phy->freq because it's set during of
>> the PHY's initialization.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/usb/chipidea/ci_hdrc_tegra.c |   9 --
>>  drivers/usb/phy/phy-tegra-usb.c      | 165 +++++++++++++++++----------
>>  2 files changed, 102 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> index 0c9911d44ee5..7455df0ede49 100644
>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> -	/*
>> -	 * Tegra's USB PHY driver doesn't implement optional phy_init()
>> -	 * hook, so we have to power on UDC controller before ChipIdea
>> -	 * driver initialization kicks in.
>> -	 */
>> -	usb_phy_set_suspend(udc->phy, 0);
>> -
>>  	/* setup and register ChipIdea HDRC device */
>>  	udc->data.name = "tegra-udc";
>>  	udc->data.flags = soc->flags;
>> @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>  	return 0;
>>  
>>  fail_power_off:
>> -	usb_phy_set_suspend(udc->phy, 1);
>>  	clk_disable_unprepare(udc->clk);
>>  	return err;
>>  }
>> @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
>>  	struct tegra_udc *udc = platform_get_drvdata(pdev);
>>  
>>  	ci_hdrc_remove_device(udc->dev);
>> -	usb_phy_set_suspend(udc->phy, 1);
>>  	clk_disable_unprepare(udc->clk);
>>  
>>  	return 0;
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index ea7ef1dc0b42..15bd253d53c9 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>> @@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>>  {
>>  	int ret;
>>  
>> -	phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
>> -	if (IS_ERR(phy->pad_clk)) {
>> -		ret = PTR_ERR(phy->pad_clk);
>> -		dev_err(phy->u_phy.dev,
>> -			"Failed to get UTMIP pad clock: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	phy->pad_rst = devm_reset_control_get_optional_shared(
>> -						phy->u_phy.dev, "utmi-pads");
>> -	if (IS_ERR(phy->pad_rst)) {
>> -		ret = PTR_ERR(phy->pad_rst);
>> -		dev_err(phy->u_phy.dev,
>> -			"Failed to get UTMI-pads reset: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>>  	ret = clk_prepare_enable(phy->pad_clk);
>>  	if (ret) {
>>  		dev_err(phy->u_phy.dev,
>> @@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
>>  	return ret;
>>  }
> 
> Acked-by: Peter Chen <peter.chen@nxp.com>
> 
> Felipe, would you please queue this series after reviewing for USB PHY
> changes? If not, Dmitry may need to split the patch.

I'll take a closer look whether it is possible to factor out ChipIdea's
driver change into a separate patch in a sensible way. Thanks!

Re: [PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks

[Thierry Reding]

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

On Wed, Dec 18, 2019 at 08:53:11PM +0300, Dmitry Osipenko wrote:
> Generic PHY provides init/shutdown callbacks which allow USB-host drivers
> to abstract PHY's hardware management in a common way. This change allows
> to remove Tegra-specific PHY handling from the ChipIdea driver.
> 
> Note that ChipIdea's driver shall be changed at the same time because it
> turns PHY ON without the PHY's initialization and this doesn't work now,
> resulting in a NULL dereference of phy->freq because it's set during of
> the PHY's initialization.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/chipidea/ci_hdrc_tegra.c |   9 --
>  drivers/usb/phy/phy-tegra-usb.c      | 165 +++++++++++++++++----------
>  2 files changed, 102 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 0c9911d44ee5..7455df0ede49 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  		return err;
>  	}
>  
> -	/*
> -	 * Tegra's USB PHY driver doesn't implement optional phy_init()
> -	 * hook, so we have to power on UDC controller before ChipIdea
> -	 * driver initialization kicks in.
> -	 */
> -	usb_phy_set_suspend(udc->phy, 0);
> -
>  	/* setup and register ChipIdea HDRC device */
>  	udc->data.name = "tegra-udc";
>  	udc->data.flags = soc->flags;
> @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  	return 0;
>  
>  fail_power_off:
> -	usb_phy_set_suspend(udc->phy, 1);
>  	clk_disable_unprepare(udc->clk);
>  	return err;
>  }
> @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
>  	struct tegra_udc *udc = platform_get_drvdata(pdev);
>  
>  	ci_hdrc_remove_device(udc->dev);
> -	usb_phy_set_suspend(udc->phy, 1);
>  	clk_disable_unprepare(udc->clk);
>  
>  	return 0;
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index ea7ef1dc0b42..15bd253d53c9 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  {
>  	int ret;
>  
> -	phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
> -	if (IS_ERR(phy->pad_clk)) {
> -		ret = PTR_ERR(phy->pad_clk);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get UTMIP pad clock: %d\n", ret);
> -		return ret;
> -	}
> -
> -	phy->pad_rst = devm_reset_control_get_optional_shared(
> -						phy->u_phy.dev, "utmi-pads");
> -	if (IS_ERR(phy->pad_rst)) {
> -		ret = PTR_ERR(phy->pad_rst);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get UTMI-pads reset: %d\n", ret);
> -		return ret;
> -	}
> -
>  	ret = clk_prepare_enable(phy->pad_clk);
>  	if (ret) {
>  		dev_err(phy->u_phy.dev,
> @@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
>  	return ret;
>  }
>  
> +static void ulpi_close(struct tegra_usb_phy *phy)
> +{
> +	int err;
> +
> +	err = gpio_direction_output(phy->reset_gpio, 1);
> +	if (err < 0) {
> +		dev_err(phy->u_phy.dev,
> +			"ULPI reset GPIO %d direction not asserted: %d\n",
> +			phy->reset_gpio, err);
> +	}
> +}
> +
>  static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>  {
>  	unsigned long val, flags;
> @@ -761,14 +756,19 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
>  	return gpio_direction_output(phy->reset_gpio, 0);
>  }
>  
> -static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
> +static void tegra_usb_phy_close(struct usb_phy *u_phy)
>  {
> -	if (!IS_ERR(phy->vbus))
> -		regulator_disable(phy->vbus);
> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> +						 u_phy);
>  
> -	if (!phy->is_ulpi_phy)
> +	if (phy->is_ulpi_phy)
> +		ulpi_close(phy);
> +	else
>  		utmip_pad_close(phy);
>  
> +	if (!IS_ERR(phy->vbus))
> +		regulator_disable(phy->vbus);
> +
>  	clk_disable_unprepare(phy->pll_u);
>  }
>  
> @@ -788,7 +788,7 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
>  		return utmi_phy_power_off(phy);
>  }
>  
> -static int	tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
> +static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
>  {
>  	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
>  	if (suspend)
> @@ -801,54 +801,25 @@ static int ulpi_open(struct tegra_usb_phy *phy)
>  {
>  	int err;
>  
> -	phy->clk = devm_clk_get(phy->u_phy.dev, "ulpi-link");
> -	if (IS_ERR(phy->clk)) {
> -		err = PTR_ERR(phy->clk);
> -		dev_err(phy->u_phy.dev, "Failed to get ULPI clock: %d\n", err);
> -		return err;
> -	}
> -
> -	err = devm_gpio_request(phy->u_phy.dev, phy->reset_gpio,
> -		"ulpi_phy_reset_b");
> -	if (err < 0) {
> -		dev_err(phy->u_phy.dev, "Request failed for GPIO %d: %d\n",
> -			phy->reset_gpio, err);
> -		return err;
> -	}
> -
>  	err = gpio_direction_output(phy->reset_gpio, 0);
>  	if (err < 0) {
>  		dev_err(phy->u_phy.dev,
> -			"GPIO %d direction not set to output: %d\n",
> +			"ULPI reset GPIO %d direction not deasserted: %d\n",
>  			phy->reset_gpio, err);
>  		return err;
>  	}
>  
> -	phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> -	if (!phy->ulpi) {
> -		dev_err(phy->u_phy.dev, "Failed to create ULPI OTG\n");
> -		err = -ENOMEM;
> -		return err;
> -	}
> -
> -	phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
>  	return 0;
>  }
>  
> -static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
> +static int tegra_usb_phy_init(struct usb_phy *u_phy)
>  {
> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
> +						 u_phy);
>  	unsigned long parent_rate;
>  	int i;
>  	int err;
>  
> -	phy->pll_u = devm_clk_get(phy->u_phy.dev, "pll_u");
> -	if (IS_ERR(phy->pll_u)) {
> -		err = PTR_ERR(phy->pll_u);
> -		dev_err(phy->u_phy.dev,
> -			"Failed to get pll_u clock: %d\n", err);
> -		return err;
> -	}
> -
>  	err = clk_prepare_enable(phy->pll_u);
>  	if (err)
>  		return err;
> @@ -884,8 +855,17 @@ static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
>  	if (err < 0)
>  		goto fail;
>  
> +	err = tegra_usb_phy_power_on(phy);
> +	if (err)
> +		goto close_phy;
> +
>  	return 0;
>  
> +close_phy:
> +	if (phy->is_ulpi_phy)
> +		ulpi_close(phy);
> +	else
> +		utmip_pad_close(phy);
>  fail:
>  	clk_disable_unprepare(phy->pll_u);
>  	return err;
> @@ -1134,22 +1114,77 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
>  		tegra_phy->vbus = ERR_PTR(-ENODEV);
>  	}
>  
> -	tegra_phy->u_phy.dev = &pdev->dev;
> -	err = tegra_usb_phy_init(tegra_phy);
> -	if (err < 0)
> +	tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
> +	err = PTR_ERR_OR_ZERO(tegra_phy);
> +	if (err) {
> +		dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err);
>  		return err;
> +	}
>  
> +	if (tegra_phy->is_ulpi_phy) {
> +		tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->clk);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get ULPI clock: %d\n", err);
> +			return err;
> +		}
> +
> +		err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
> +			"ulpi_phy_reset_b");
> +		if (err < 0) {
> +			dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
> +				tegra_phy->reset_gpio, err);
> +			return err;
> +		}
> +
> +		tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> +		if (!tegra_phy->ulpi) {
> +			dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
> +			err = -ENOMEM;
> +			return err;
> +		}
> +
> +		tegra_phy->ulpi->io_priv = tegra_phy->regs + ULPI_VIEWPORT;
> +	} else {
> +		tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get UTMIP pad clock: %d\n", err);
> +			return err;
> +		}
> +
> +		tegra_phy->pad_rst = devm_reset_control_get_optional_shared(
> +						&pdev->dev, "utmi-pads");
> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_rst);
> +		if (err) {
> +			dev_err(&pdev->dev,
> +				"Failed to get UTMI-pads reset: %d\n", err);
> +			return err;
> +		}
> +	}
> +
> +	tegra_phy->u_phy.dev = &pdev->dev;
> +	tegra_phy->u_phy.init = tegra_usb_phy_init;
> +	tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
>  	tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
>  
>  	platform_set_drvdata(pdev, tegra_phy);
>  
>  	err = usb_add_phy_dev(&tegra_phy->u_phy);
> -	if (err < 0) {
> -		tegra_usb_phy_close(tegra_phy);
> -		return err;
> -	}
> +	if (err < 0)
> +		goto free_ulpi;
>  
>  	return 0;
> +
> +free_ulpi:
> +	if (tegra_phy->ulpi) {
> +		kfree(tegra_phy->ulpi->otg);
> +		kfree(tegra_phy->ulpi);
> +	}
> +
> +	return err;
>  }
>  
>  static int tegra_usb_phy_remove(struct platform_device *pdev)
> @@ -1157,7 +1192,11 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
>  	struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
>  
>  	usb_remove_phy(&tegra_phy->u_phy);
> -	tegra_usb_phy_close(tegra_phy);
> +
> +	if (tegra_phy->ulpi) {
> +		kfree(tegra_phy->ulpi->otg);
> +		kfree(tegra_phy->ulpi);
> +	}

It might be nicer to add a new otg_ulpi_free() (or whatever) function to
do this. Manually cleaning up the resources allocated by a public API is
a bit asymmetric and easy to get wrong.

But it's correct in this case and the new function can be added in a
separate patch, so:

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 2/4] usb: phy: tegra: Hook up init/shutdown callbacks

[Dmitry Osipenko]

19.12.2019 16:01, Thierry Reding пишет:
> On Wed, Dec 18, 2019 at 08:53:11PM +0300, Dmitry Osipenko wrote:
>> Generic PHY provides init/shutdown callbacks which allow USB-host drivers
>> to abstract PHY's hardware management in a common way. This change allows
>> to remove Tegra-specific PHY handling from the ChipIdea driver.
>>
>> Note that ChipIdea's driver shall be changed at the same time because it
>> turns PHY ON without the PHY's initialization and this doesn't work now,
>> resulting in a NULL dereference of phy->freq because it's set during of
>> the PHY's initialization.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/usb/chipidea/ci_hdrc_tegra.c |   9 --
>>  drivers/usb/phy/phy-tegra-usb.c      | 165 +++++++++++++++++----------
>>  2 files changed, 102 insertions(+), 72 deletions(-)
>>
>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> index 0c9911d44ee5..7455df0ede49 100644
>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> @@ -83,13 +83,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>  		return err;
>>  	}
>>  
>> -	/*
>> -	 * Tegra's USB PHY driver doesn't implement optional phy_init()
>> -	 * hook, so we have to power on UDC controller before ChipIdea
>> -	 * driver initialization kicks in.
>> -	 */
>> -	usb_phy_set_suspend(udc->phy, 0);
>> -
>>  	/* setup and register ChipIdea HDRC device */
>>  	udc->data.name = "tegra-udc";
>>  	udc->data.flags = soc->flags;
>> @@ -109,7 +102,6 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>  	return 0;
>>  
>>  fail_power_off:
>> -	usb_phy_set_suspend(udc->phy, 1);
>>  	clk_disable_unprepare(udc->clk);
>>  	return err;
>>  }
>> @@ -119,7 +111,6 @@ static int tegra_udc_remove(struct platform_device *pdev)
>>  	struct tegra_udc *udc = platform_get_drvdata(pdev);
>>  
>>  	ci_hdrc_remove_device(udc->dev);
>> -	usb_phy_set_suspend(udc->phy, 1);
>>  	clk_disable_unprepare(udc->clk);
>>  
>>  	return 0;
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index ea7ef1dc0b42..15bd253d53c9 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>> @@ -238,23 +238,6 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>>  {
>>  	int ret;
>>  
>> -	phy->pad_clk = devm_clk_get(phy->u_phy.dev, "utmi-pads");
>> -	if (IS_ERR(phy->pad_clk)) {
>> -		ret = PTR_ERR(phy->pad_clk);
>> -		dev_err(phy->u_phy.dev,
>> -			"Failed to get UTMIP pad clock: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>> -	phy->pad_rst = devm_reset_control_get_optional_shared(
>> -						phy->u_phy.dev, "utmi-pads");
>> -	if (IS_ERR(phy->pad_rst)) {
>> -		ret = PTR_ERR(phy->pad_rst);
>> -		dev_err(phy->u_phy.dev,
>> -			"Failed to get UTMI-pads reset: %d\n", ret);
>> -		return ret;
>> -	}
>> -
>>  	ret = clk_prepare_enable(phy->pad_clk);
>>  	if (ret) {
>>  		dev_err(phy->u_phy.dev,
>> @@ -315,6 +298,18 @@ static int utmip_pad_close(struct tegra_usb_phy *phy)
>>  	return ret;
>>  }
>>  
>> +static void ulpi_close(struct tegra_usb_phy *phy)
>> +{
>> +	int err;
>> +
>> +	err = gpio_direction_output(phy->reset_gpio, 1);
>> +	if (err < 0) {
>> +		dev_err(phy->u_phy.dev,
>> +			"ULPI reset GPIO %d direction not asserted: %d\n",
>> +			phy->reset_gpio, err);
>> +	}
>> +}
>> +
>>  static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>>  {
>>  	unsigned long val, flags;
>> @@ -761,14 +756,19 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
>>  	return gpio_direction_output(phy->reset_gpio, 0);
>>  }
>>  
>> -static void tegra_usb_phy_close(struct tegra_usb_phy *phy)
>> +static void tegra_usb_phy_close(struct usb_phy *u_phy)
>>  {
>> -	if (!IS_ERR(phy->vbus))
>> -		regulator_disable(phy->vbus);
>> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
>> +						 u_phy);
>>  
>> -	if (!phy->is_ulpi_phy)
>> +	if (phy->is_ulpi_phy)
>> +		ulpi_close(phy);
>> +	else
>>  		utmip_pad_close(phy);
>>  
>> +	if (!IS_ERR(phy->vbus))
>> +		regulator_disable(phy->vbus);
>> +
>>  	clk_disable_unprepare(phy->pll_u);
>>  }
>>  
>> @@ -788,7 +788,7 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
>>  		return utmi_phy_power_off(phy);
>>  }
>>  
>> -static int	tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
>> +static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
>>  {
>>  	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
>>  	if (suspend)
>> @@ -801,54 +801,25 @@ static int ulpi_open(struct tegra_usb_phy *phy)
>>  {
>>  	int err;
>>  
>> -	phy->clk = devm_clk_get(phy->u_phy.dev, "ulpi-link");
>> -	if (IS_ERR(phy->clk)) {
>> -		err = PTR_ERR(phy->clk);
>> -		dev_err(phy->u_phy.dev, "Failed to get ULPI clock: %d\n", err);
>> -		return err;
>> -	}
>> -
>> -	err = devm_gpio_request(phy->u_phy.dev, phy->reset_gpio,
>> -		"ulpi_phy_reset_b");
>> -	if (err < 0) {
>> -		dev_err(phy->u_phy.dev, "Request failed for GPIO %d: %d\n",
>> -			phy->reset_gpio, err);
>> -		return err;
>> -	}
>> -
>>  	err = gpio_direction_output(phy->reset_gpio, 0);
>>  	if (err < 0) {
>>  		dev_err(phy->u_phy.dev,
>> -			"GPIO %d direction not set to output: %d\n",
>> +			"ULPI reset GPIO %d direction not deasserted: %d\n",
>>  			phy->reset_gpio, err);
>>  		return err;
>>  	}
>>  
>> -	phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
>> -	if (!phy->ulpi) {
>> -		dev_err(phy->u_phy.dev, "Failed to create ULPI OTG\n");
>> -		err = -ENOMEM;
>> -		return err;
>> -	}
>> -
>> -	phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
>>  	return 0;
>>  }
>>  
>> -static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
>> +static int tegra_usb_phy_init(struct usb_phy *u_phy)
>>  {
>> +	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
>> +						 u_phy);
>>  	unsigned long parent_rate;
>>  	int i;
>>  	int err;
>>  
>> -	phy->pll_u = devm_clk_get(phy->u_phy.dev, "pll_u");
>> -	if (IS_ERR(phy->pll_u)) {
>> -		err = PTR_ERR(phy->pll_u);
>> -		dev_err(phy->u_phy.dev,
>> -			"Failed to get pll_u clock: %d\n", err);
>> -		return err;
>> -	}
>> -
>>  	err = clk_prepare_enable(phy->pll_u);
>>  	if (err)
>>  		return err;
>> @@ -884,8 +855,17 @@ static int tegra_usb_phy_init(struct tegra_usb_phy *phy)
>>  	if (err < 0)
>>  		goto fail;
>>  
>> +	err = tegra_usb_phy_power_on(phy);
>> +	if (err)
>> +		goto close_phy;
>> +
>>  	return 0;
>>  
>> +close_phy:
>> +	if (phy->is_ulpi_phy)
>> +		ulpi_close(phy);
>> +	else
>> +		utmip_pad_close(phy);
>>  fail:
>>  	clk_disable_unprepare(phy->pll_u);
>>  	return err;
>> @@ -1134,22 +1114,77 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
>>  		tegra_phy->vbus = ERR_PTR(-ENODEV);
>>  	}
>>  
>> -	tegra_phy->u_phy.dev = &pdev->dev;
>> -	err = tegra_usb_phy_init(tegra_phy);
>> -	if (err < 0)
>> +	tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
>> +	err = PTR_ERR_OR_ZERO(tegra_phy);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "Failed to get pll_u clock: %d\n", err);
>>  		return err;
>> +	}
>>  
>> +	if (tegra_phy->is_ulpi_phy) {
>> +		tegra_phy->clk = devm_clk_get(&pdev->dev, "ulpi-link");
>> +		err = PTR_ERR_OR_ZERO(tegra_phy->clk);
>> +		if (err) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to get ULPI clock: %d\n", err);
>> +			return err;
>> +		}
>> +
>> +		err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
>> +			"ulpi_phy_reset_b");
>> +		if (err < 0) {
>> +			dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
>> +				tegra_phy->reset_gpio, err);
>> +			return err;
>> +		}
>> +
>> +		tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
>> +		if (!tegra_phy->ulpi) {
>> +			dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
>> +			err = -ENOMEM;
>> +			return err;
>> +		}
>> +
>> +		tegra_phy->ulpi->io_priv = tegra_phy->regs + ULPI_VIEWPORT;
>> +	} else {
>> +		tegra_phy->pad_clk = devm_clk_get(&pdev->dev, "utmi-pads");
>> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_clk);
>> +		if (err) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to get UTMIP pad clock: %d\n", err);
>> +			return err;
>> +		}
>> +
>> +		tegra_phy->pad_rst = devm_reset_control_get_optional_shared(
>> +						&pdev->dev, "utmi-pads");
>> +		err = PTR_ERR_OR_ZERO(tegra_phy->pad_rst);
>> +		if (err) {
>> +			dev_err(&pdev->dev,
>> +				"Failed to get UTMI-pads reset: %d\n", err);
>> +			return err;
>> +		}
>> +	}
>> +
>> +	tegra_phy->u_phy.dev = &pdev->dev;
>> +	tegra_phy->u_phy.init = tegra_usb_phy_init;
>> +	tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
>>  	tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
>>  
>>  	platform_set_drvdata(pdev, tegra_phy);
>>  
>>  	err = usb_add_phy_dev(&tegra_phy->u_phy);
>> -	if (err < 0) {
>> -		tegra_usb_phy_close(tegra_phy);
>> -		return err;
>> -	}
>> +	if (err < 0)
>> +		goto free_ulpi;
>>  
>>  	return 0;
>> +
>> +free_ulpi:
>> +	if (tegra_phy->ulpi) {
>> +		kfree(tegra_phy->ulpi->otg);
>> +		kfree(tegra_phy->ulpi);
>> +	}
>> +
>> +	return err;
>>  }
>>  
>>  static int tegra_usb_phy_remove(struct platform_device *pdev)
>> @@ -1157,7 +1192,11 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
>>  	struct tegra_usb_phy *tegra_phy = platform_get_drvdata(pdev);
>>  
>>  	usb_remove_phy(&tegra_phy->u_phy);
>> -	tegra_usb_phy_close(tegra_phy);
>> +
>> +	if (tegra_phy->ulpi) {
>> +		kfree(tegra_phy->ulpi->otg);
>> +		kfree(tegra_phy->ulpi);
>> +	}
> 
> It might be nicer to add a new otg_ulpi_free() (or whatever) function to
> do this. Manually cleaning up the resources allocated by a public API is
> a bit asymmetric and easy to get wrong.
> 
> But it's correct in this case and the new function can be added in a
> separate patch, so:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>
> 

Guess devm_otg_ulpi_create() could be even a better variant, I'll take a
look at implementing it. Thanks!

[PATCH v1 3/4] usb: phy: tegra: Perform general clean up of the code

[Dmitry Osipenko]

This patch fixes few dozens of legit checkpatch warnings, adds missed
handling of potential error-cases, fixes ULPI clk-prepare refcounting and
prettifies code where makes sense. All these clean-up changes are quite
minor and do not fix any problems.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 367 +++++++++++++++++---------------
 1 file changed, 197 insertions(+), 170 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 15bd253d53c9..76949dbbbdc2 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -28,35 +28,35 @@
 #include <linux/usb/tegra_usb_phy.h>
 #include <linux/regulator/consumer.h>
 
-#define ULPI_VIEWPORT		0x170
+#define ULPI_VIEWPORT				0x170
 
 /* PORTSC PTS/PHCD bits, Tegra20 only */
-#define TEGRA_USB_PORTSC1				0x184
-#define TEGRA_USB_PORTSC1_PTS(x)			(((x) & 0x3) << 30)
-#define TEGRA_USB_PORTSC1_PHCD				(1 << 23)
+#define TEGRA_USB_PORTSC1			0x184
+#define TEGRA_USB_PORTSC1_PTS(x)		(((x) & 0x3) << 30)
+#define TEGRA_USB_PORTSC1_PHCD			BIT(23)
 
 /* HOSTPC1 PTS/PHCD bits, Tegra30 and above */
-#define TEGRA_USB_HOSTPC1_DEVLC		0x1b4
-#define TEGRA_USB_HOSTPC1_DEVLC_PTS(x)	(((x) & 0x7) << 29)
-#define TEGRA_USB_HOSTPC1_DEVLC_PHCD	(1 << 22)
+#define TEGRA_USB_HOSTPC1_DEVLC			0x1b4
+#define TEGRA_USB_HOSTPC1_DEVLC_PTS(x)		(((x) & 0x7) << 29)
+#define TEGRA_USB_HOSTPC1_DEVLC_PHCD		BIT(22)
 
 /* Bits of PORTSC1, which will get cleared by writing 1 into them */
 #define TEGRA_PORTSC1_RWC_BITS	(PORT_CSC | PORT_PEC | PORT_OCC)
 
-#define USB_SUSP_CTRL		0x400
-#define   USB_WAKE_ON_CNNT_EN_DEV	(1 << 3)
-#define   USB_WAKE_ON_DISCON_EN_DEV	(1 << 4)
-#define   USB_SUSP_CLR		(1 << 5)
-#define   USB_PHY_CLK_VALID	(1 << 7)
-#define   UTMIP_RESET			(1 << 11)
-#define   UHSIC_RESET			(1 << 11)
-#define   UTMIP_PHY_ENABLE		(1 << 12)
-#define   ULPI_PHY_ENABLE	(1 << 13)
-#define   USB_SUSP_SET		(1 << 14)
-#define   USB_WAKEUP_DEBOUNCE_COUNT(x)	(((x) & 0x7) << 16)
-
-#define USB1_LEGACY_CTRL	0x410
-#define   USB1_NO_LEGACY_MODE			(1 << 0)
+#define USB_SUSP_CTRL				0x400
+#define   USB_WAKE_ON_CNNT_EN_DEV		BIT(3)
+#define   USB_WAKE_ON_DISCON_EN_DEV		BIT(4)
+#define   USB_SUSP_CLR				BIT(5)
+#define   USB_PHY_CLK_VALID			BIT(7)
+#define   UTMIP_RESET				BIT(11)
+#define   UHSIC_RESET				BIT(11)
+#define   UTMIP_PHY_ENABLE			BIT(12)
+#define   ULPI_PHY_ENABLE			BIT(13)
+#define   USB_SUSP_SET				BIT(14)
+#define   USB_WAKEUP_DEBOUNCE_COUNT(x)		(((x) & 0x7) << 16)
+
+#define USB1_LEGACY_CTRL			0x410
+#define   USB1_NO_LEGACY_MODE			BIT(0)
 #define   USB1_VBUS_SENSE_CTL_MASK		(3 << 1)
 #define   USB1_VBUS_SENSE_CTL_VBUS_WAKEUP	(0 << 1)
 #define   USB1_VBUS_SENSE_CTL_AB_SESS_VLD_OR_VBUS_WAKEUP \
@@ -64,88 +64,88 @@
 #define   USB1_VBUS_SENSE_CTL_AB_SESS_VLD	(2 << 1)
 #define   USB1_VBUS_SENSE_CTL_A_SESS_VLD	(3 << 1)
 
-#define ULPI_TIMING_CTRL_0	0x424
-#define   ULPI_OUTPUT_PINMUX_BYP	(1 << 10)
-#define   ULPI_CLKOUT_PINMUX_BYP	(1 << 11)
+#define ULPI_TIMING_CTRL_0			0x424
+#define   ULPI_OUTPUT_PINMUX_BYP		BIT(10)
+#define   ULPI_CLKOUT_PINMUX_BYP		BIT(11)
 
-#define ULPI_TIMING_CTRL_1	0x428
-#define   ULPI_DATA_TRIMMER_LOAD	(1 << 0)
-#define   ULPI_DATA_TRIMMER_SEL(x)	(((x) & 0x7) << 1)
-#define   ULPI_STPDIRNXT_TRIMMER_LOAD	(1 << 16)
-#define   ULPI_STPDIRNXT_TRIMMER_SEL(x)	(((x) & 0x7) << 17)
-#define   ULPI_DIR_TRIMMER_LOAD		(1 << 24)
-#define   ULPI_DIR_TRIMMER_SEL(x)	(((x) & 0x7) << 25)
+#define ULPI_TIMING_CTRL_1			0x428
+#define   ULPI_DATA_TRIMMER_LOAD		BIT(0)
+#define   ULPI_DATA_TRIMMER_SEL(x)		(((x) & 0x7) << 1)
+#define   ULPI_STPDIRNXT_TRIMMER_LOAD		BIT(16)
+#define   ULPI_STPDIRNXT_TRIMMER_SEL(x)		(((x) & 0x7) << 17)
+#define   ULPI_DIR_TRIMMER_LOAD			BIT(24)
+#define   ULPI_DIR_TRIMMER_SEL(x)		(((x) & 0x7) << 25)
 
-#define UTMIP_PLL_CFG1		0x804
+#define UTMIP_PLL_CFG1				0x804
 #define   UTMIP_XTAL_FREQ_COUNT(x)		(((x) & 0xfff) << 0)
 #define   UTMIP_PLLU_ENABLE_DLY_COUNT(x)	(((x) & 0x1f) << 27)
 
-#define UTMIP_XCVR_CFG0		0x808
+#define UTMIP_XCVR_CFG0				0x808
 #define   UTMIP_XCVR_SETUP(x)			(((x) & 0xf) << 0)
 #define   UTMIP_XCVR_SETUP_MSB(x)		((((x) & 0x70) >> 4) << 22)
 #define   UTMIP_XCVR_LSRSLEW(x)			(((x) & 0x3) << 8)
 #define   UTMIP_XCVR_LSFSLEW(x)			(((x) & 0x3) << 10)
-#define   UTMIP_FORCE_PD_POWERDOWN		(1 << 14)
-#define   UTMIP_FORCE_PD2_POWERDOWN		(1 << 16)
-#define   UTMIP_FORCE_PDZI_POWERDOWN		(1 << 18)
-#define   UTMIP_XCVR_LSBIAS_SEL			(1 << 21)
+#define   UTMIP_FORCE_PD_POWERDOWN		BIT(14)
+#define   UTMIP_FORCE_PD2_POWERDOWN		BIT(16)
+#define   UTMIP_FORCE_PDZI_POWERDOWN		BIT(18)
+#define   UTMIP_XCVR_LSBIAS_SEL			BIT(21)
 #define   UTMIP_XCVR_HSSLEW(x)			(((x) & 0x3) << 4)
 #define   UTMIP_XCVR_HSSLEW_MSB(x)		((((x) & 0x1fc) >> 2) << 25)
 
-#define UTMIP_BIAS_CFG0		0x80c
-#define   UTMIP_OTGPD			(1 << 11)
-#define   UTMIP_BIASPD			(1 << 10)
-#define   UTMIP_HSSQUELCH_LEVEL(x)	(((x) & 0x3) << 0)
-#define   UTMIP_HSDISCON_LEVEL(x)	(((x) & 0x3) << 2)
-#define   UTMIP_HSDISCON_LEVEL_MSB(x)	((((x) & 0x4) >> 2) << 24)
+#define UTMIP_BIAS_CFG0				0x80c
+#define   UTMIP_OTGPD				BIT(11)
+#define   UTMIP_BIASPD				BIT(10)
+#define   UTMIP_HSSQUELCH_LEVEL(x)		(((x) & 0x3) << 0)
+#define   UTMIP_HSDISCON_LEVEL(x)		(((x) & 0x3) << 2)
+#define   UTMIP_HSDISCON_LEVEL_MSB(x)		((((x) & 0x4) >> 2) << 24)
 
-#define UTMIP_HSRX_CFG0		0x810
-#define   UTMIP_ELASTIC_LIMIT(x)	(((x) & 0x1f) << 10)
-#define   UTMIP_IDLE_WAIT(x)		(((x) & 0x1f) << 15)
+#define UTMIP_HSRX_CFG0				0x810
+#define   UTMIP_ELASTIC_LIMIT(x)		(((x) & 0x1f) << 10)
+#define   UTMIP_IDLE_WAIT(x)			(((x) & 0x1f) << 15)
 
-#define UTMIP_HSRX_CFG1		0x814
-#define   UTMIP_HS_SYNC_START_DLY(x)	(((x) & 0x1f) << 1)
+#define UTMIP_HSRX_CFG1				0x814
+#define   UTMIP_HS_SYNC_START_DLY(x)		(((x) & 0x1f) << 1)
 
-#define UTMIP_TX_CFG0		0x820
-#define   UTMIP_FS_PREABMLE_J		(1 << 19)
-#define   UTMIP_HS_DISCON_DISABLE	(1 << 8)
+#define UTMIP_TX_CFG0				0x820
+#define   UTMIP_FS_PREABMLE_J			BIT(19)
+#define   UTMIP_HS_DISCON_DISABLE		BIT(8)
 
-#define UTMIP_MISC_CFG0		0x824
-#define   UTMIP_DPDM_OBSERVE		(1 << 26)
-#define   UTMIP_DPDM_OBSERVE_SEL(x)	(((x) & 0xf) << 27)
-#define   UTMIP_DPDM_OBSERVE_SEL_FS_J	UTMIP_DPDM_OBSERVE_SEL(0xf)
-#define   UTMIP_DPDM_OBSERVE_SEL_FS_K	UTMIP_DPDM_OBSERVE_SEL(0xe)
-#define   UTMIP_DPDM_OBSERVE_SEL_FS_SE1 UTMIP_DPDM_OBSERVE_SEL(0xd)
-#define   UTMIP_DPDM_OBSERVE_SEL_FS_SE0 UTMIP_DPDM_OBSERVE_SEL(0xc)
-#define   UTMIP_SUSPEND_EXIT_ON_EDGE	(1 << 22)
+#define UTMIP_MISC_CFG0				0x824
+#define   UTMIP_DPDM_OBSERVE			BIT(26)
+#define   UTMIP_DPDM_OBSERVE_SEL(x)		(((x) & 0xf) << 27)
+#define   UTMIP_DPDM_OBSERVE_SEL_FS_J		UTMIP_DPDM_OBSERVE_SEL(0xf)
+#define   UTMIP_DPDM_OBSERVE_SEL_FS_K		UTMIP_DPDM_OBSERVE_SEL(0xe)
+#define   UTMIP_DPDM_OBSERVE_SEL_FS_SE1		UTMIP_DPDM_OBSERVE_SEL(0xd)
+#define   UTMIP_DPDM_OBSERVE_SEL_FS_SE0		UTMIP_DPDM_OBSERVE_SEL(0xc)
+#define   UTMIP_SUSPEND_EXIT_ON_EDGE		BIT(22)
 
-#define UTMIP_MISC_CFG1		0x828
-#define   UTMIP_PLL_ACTIVE_DLY_COUNT(x)	(((x) & 0x1f) << 18)
-#define   UTMIP_PLLU_STABLE_COUNT(x)	(((x) & 0xfff) << 6)
+#define UTMIP_MISC_CFG1				0x828
+#define   UTMIP_PLL_ACTIVE_DLY_COUNT(x)		(((x) & 0x1f) << 18)
+#define   UTMIP_PLLU_STABLE_COUNT(x)		(((x) & 0xfff) << 6)
 
-#define UTMIP_DEBOUNCE_CFG0	0x82c
-#define   UTMIP_BIAS_DEBOUNCE_A(x)	(((x) & 0xffff) << 0)
+#define UTMIP_DEBOUNCE_CFG0			0x82c
+#define   UTMIP_BIAS_DEBOUNCE_A(x)		(((x) & 0xffff) << 0)
 
-#define UTMIP_BAT_CHRG_CFG0	0x830
-#define   UTMIP_PD_CHRG			(1 << 0)
+#define UTMIP_BAT_CHRG_CFG0			0x830
+#define   UTMIP_PD_CHRG				BIT(0)
 
-#define UTMIP_SPARE_CFG0	0x834
-#define   FUSE_SETUP_SEL		(1 << 3)
+#define UTMIP_SPARE_CFG0			0x834
+#define   FUSE_SETUP_SEL			BIT(3)
 
-#define UTMIP_XCVR_CFG1		0x838
-#define   UTMIP_FORCE_PDDISC_POWERDOWN	(1 << 0)
-#define   UTMIP_FORCE_PDCHRP_POWERDOWN	(1 << 2)
-#define   UTMIP_FORCE_PDDR_POWERDOWN	(1 << 4)
-#define   UTMIP_XCVR_TERM_RANGE_ADJ(x)	(((x) & 0xf) << 18)
+#define UTMIP_XCVR_CFG1				0x838
+#define   UTMIP_FORCE_PDDISC_POWERDOWN		BIT(0)
+#define   UTMIP_FORCE_PDCHRP_POWERDOWN		BIT(2)
+#define   UTMIP_FORCE_PDDR_POWERDOWN		BIT(4)
+#define   UTMIP_XCVR_TERM_RANGE_ADJ(x)		(((x) & 0xf) << 18)
 
-#define UTMIP_BIAS_CFG1		0x83c
-#define   UTMIP_BIAS_PDTRK_COUNT(x)	(((x) & 0x1f) << 3)
+#define UTMIP_BIAS_CFG1				0x83c
+#define   UTMIP_BIAS_PDTRK_COUNT(x)		(((x) & 0x1f) << 3)
 
 /* For Tegra30 and above only, the address is different in Tegra20 */
-#define USB_USBMODE		0x1f8
-#define   USB_USBMODE_MASK		(3 << 0)
-#define   USB_USBMODE_HOST		(3 << 0)
-#define   USB_USBMODE_DEVICE		(2 << 0)
+#define USB_USBMODE				0x1f8
+#define   USB_USBMODE_MASK			(3 << 0)
+#define   USB_USBMODE_HOST			(3 << 0)
+#define   USB_USBMODE_DEVICE			(2 << 0)
 
 static DEFINE_SPINLOCK(utmip_pad_lock);
 static int utmip_pad_count;
@@ -194,6 +194,11 @@ static const struct tegra_xtal_freq tegra_freq_table[] = {
 	},
 };
 
+static inline struct tegra_usb_phy *to_tegra_usb_phy(struct usb_phy *u_phy)
+{
+	return container_of(u_phy, struct tegra_usb_phy, u_phy);
+}
+
 static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
 {
 	void __iomem *base = phy->regs;
@@ -310,13 +315,16 @@ static void ulpi_close(struct tegra_usb_phy *phy)
 	}
 }
 
-static void utmip_pad_power_on(struct tegra_usb_phy *phy)
+static int utmip_pad_power_on(struct tegra_usb_phy *phy)
 {
-	unsigned long val, flags;
-	void __iomem *base = phy->pad_regs;
 	struct tegra_utmip_config *config = phy->config;
+	void __iomem *base = phy->pad_regs;
+	unsigned long val, flags;
+	int err;
 
-	clk_prepare_enable(phy->pad_clk);
+	err = clk_prepare_enable(phy->pad_clk);
+	if (err)
+		return err;
 
 	spin_lock_irqsave(&utmip_pad_lock, flags);
 
@@ -339,19 +347,24 @@ static void utmip_pad_power_on(struct tegra_usb_phy *phy)
 	spin_unlock_irqrestore(&utmip_pad_lock, flags);
 
 	clk_disable_unprepare(phy->pad_clk);
+
+	return 0;
 }
 
 static int utmip_pad_power_off(struct tegra_usb_phy *phy)
 {
-	unsigned long val, flags;
 	void __iomem *base = phy->pad_regs;
+	unsigned long val, flags;
+	int err;
 
 	if (!utmip_pad_count) {
 		dev_err(phy->u_phy.dev, "UTMIP pad already powered off\n");
 		return -EINVAL;
 	}
 
-	clk_prepare_enable(phy->pad_clk);
+	err = clk_prepare_enable(phy->pad_clk);
+	if (err)
+		return err;
 
 	spin_lock_irqsave(&utmip_pad_lock, flags);
 
@@ -378,8 +391,8 @@ static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
 
 static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	/*
 	 * The USB driver may have already initiated the phy clock
@@ -394,13 +407,14 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		val |= USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
 
-		udelay(10);
+		usleep_range(10, 100);
 
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
-	} else
+	} else {
 		set_phcd(phy, true);
+	}
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) < 0)
 		dev_err(phy->u_phy.dev,
@@ -409,8 +423,8 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 
 static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	/*
 	 * The USB driver may have already initiated the phy clock
@@ -426,25 +440,27 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		val |= USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
 
-		udelay(10);
+		usleep_range(10, 100);
 
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
-	} else
+	} else {
 		set_phcd(phy, false);
+	}
 
-	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
-						     USB_PHY_CLK_VALID))
+	if (utmi_wait_register(base + USB_SUSP_CTRL,
+			       USB_PHY_CLK_VALID, USB_PHY_CLK_VALID))
 		dev_err(phy->u_phy.dev,
 			"Timeout waiting for PHY to stabilize on enable\n");
 }
 
 static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
-	void __iomem *base = phy->regs;
 	struct tegra_utmip_config *config = phy->config;
+	void __iomem *base = phy->regs;
+	unsigned long val;
+	int err;
 
 	val = readl(base + USB_SUSP_CTRL);
 	val |= UTMIP_RESET;
@@ -510,7 +526,9 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 		writel(val, base + UTMIP_BAT_CHRG_CFG0);
 	}
 
-	utmip_pad_power_on(phy);
+	err = utmip_pad_power_on(phy);
+	if (err)
+		return err;
 
 	val = readl(base + UTMIP_XCVR_CFG0);
 	val &= ~(UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN |
@@ -591,8 +609,8 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 
 static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	utmi_phy_clk_disable(phy);
 
@@ -626,8 +644,8 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 
 static void utmi_phy_preresume(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	val = readl(base + UTMIP_TX_CFG0);
 	val |= UTMIP_HS_DISCON_DISABLE;
@@ -636,8 +654,8 @@ static void utmi_phy_preresume(struct tegra_usb_phy *phy)
 
 static void utmi_phy_postresume(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	val = readl(base + UTMIP_TX_CFG0);
 	val &= ~UTMIP_HS_DISCON_DISABLE;
@@ -647,8 +665,8 @@ static void utmi_phy_postresume(struct tegra_usb_phy *phy)
 static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
 				   enum tegra_usb_phy_port_speed port_speed)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	val = readl(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE_SEL(~0);
@@ -657,47 +675,52 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
 	else
 		val |= UTMIP_DPDM_OBSERVE_SEL_FS_J;
 	writel(val, base + UTMIP_MISC_CFG0);
-	udelay(1);
+	usleep_range(1, 10);
 
 	val = readl(base + UTMIP_MISC_CFG0);
 	val |= UTMIP_DPDM_OBSERVE;
 	writel(val, base + UTMIP_MISC_CFG0);
-	udelay(10);
+	usleep_range(10, 100);
 }
 
 static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
 
 	val = readl(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE;
 	writel(val, base + UTMIP_MISC_CFG0);
-	udelay(10);
+	usleep_range(10, 100);
 }
 
 static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 {
-	int ret;
-	unsigned long val;
 	void __iomem *base = phy->regs;
+	unsigned long val;
+	int err;
 
-	ret = gpio_direction_output(phy->reset_gpio, 0);
-	if (ret < 0) {
+	err = gpio_direction_output(phy->reset_gpio, 0);
+	if (err < 0) {
 		dev_err(phy->u_phy.dev, "GPIO %d not set to 0: %d\n",
-			phy->reset_gpio, ret);
-		return ret;
+			phy->reset_gpio, err);
+		return err;
 	}
-	msleep(5);
-	ret = gpio_direction_output(phy->reset_gpio, 1);
-	if (ret < 0) {
+
+	err = clk_prepare_enable(phy->clk);
+	if (err)
+		return err;
+
+	usleep_range(5000, 10000);
+
+	err = gpio_direction_output(phy->reset_gpio, 1);
+	if (err < 0) {
 		dev_err(phy->u_phy.dev, "GPIO %d not set to 1: %d\n",
-			phy->reset_gpio, ret);
-		return ret;
+			phy->reset_gpio, err);
+		goto disable_clk;
 	}
 
-	clk_prepare_enable(phy->clk);
-	msleep(1);
+	usleep_range(1000, 2000);
 
 	val = readl(base + USB_SUSP_CTRL);
 	val |= UHSIC_RESET;
@@ -718,7 +741,7 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 	val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
 	val |= ULPI_DIR_TRIMMER_SEL(4);
 	writel(val, base + ULPI_TIMING_CTRL_1);
-	udelay(10);
+	usleep_range(10, 100);
 
 	val |= ULPI_DATA_TRIMMER_LOAD;
 	val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
@@ -726,40 +749,45 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 	writel(val, base + ULPI_TIMING_CTRL_1);
 
 	/* Fix VbusInvalid due to floating VBUS */
-	ret = usb_phy_io_write(phy->ulpi, 0x40, 0x08);
-	if (ret) {
-		dev_err(phy->u_phy.dev, "ULPI write failed: %d\n", ret);
-		return ret;
+	err = usb_phy_io_write(phy->ulpi, 0x40, 0x08);
+	if (err) {
+		dev_err(phy->u_phy.dev, "ULPI write failed: %d\n", err);
+		goto disable_clk;
 	}
 
-	ret = usb_phy_io_write(phy->ulpi, 0x80, 0x0B);
-	if (ret) {
-		dev_err(phy->u_phy.dev, "ULPI write failed: %d\n", ret);
-		return ret;
+	err = usb_phy_io_write(phy->ulpi, 0x80, 0x0B);
+	if (err) {
+		dev_err(phy->u_phy.dev, "ULPI write failed: %d\n", err);
+		goto disable_clk;
 	}
 
 	val = readl(base + USB_SUSP_CTRL);
 	val |= USB_SUSP_CLR;
 	writel(val, base + USB_SUSP_CTRL);
-	udelay(100);
+	usleep_range(100, 1000);
 
 	val = readl(base + USB_SUSP_CTRL);
 	val &= ~USB_SUSP_CLR;
 	writel(val, base + USB_SUSP_CTRL);
 
 	return 0;
+
+disable_clk:
+	clk_disable_unprepare(phy->clk);
+
+	return err;
 }
 
 static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
 {
-	clk_disable(phy->clk);
+	clk_disable_unprepare(phy->clk);
+
 	return gpio_direction_output(phy->reset_gpio, 0);
 }
 
 static void tegra_usb_phy_close(struct usb_phy *u_phy)
 {
-	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
-						 u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 
 	if (phy->is_ulpi_phy)
 		ulpi_close(phy);
@@ -788,9 +816,10 @@ static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
 		return utmi_phy_power_off(phy);
 }
 
-static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
+static int tegra_usb_phy_suspend(struct usb_phy *u_phy, int suspend)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
+
 	if (suspend)
 		return tegra_usb_phy_power_off(phy);
 	else
@@ -814,10 +843,9 @@ static int ulpi_open(struct tegra_usb_phy *phy)
 
 static int tegra_usb_phy_init(struct usb_phy *u_phy)
 {
-	struct tegra_usb_phy *phy = container_of(u_phy, struct tegra_usb_phy,
-						 u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 	unsigned long parent_rate;
-	int i;
+	unsigned int i;
 	int err;
 
 	err = clk_prepare_enable(phy->pll_u);
@@ -868,40 +896,41 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy)
 		utmip_pad_close(phy);
 fail:
 	clk_disable_unprepare(phy->pll_u);
+
 	return err;
 }
 
-void tegra_usb_phy_preresume(struct usb_phy *x)
+void tegra_usb_phy_preresume(struct usb_phy *u_phy)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 
 	if (!phy->is_ulpi_phy)
 		utmi_phy_preresume(phy);
 }
 EXPORT_SYMBOL_GPL(tegra_usb_phy_preresume);
 
-void tegra_usb_phy_postresume(struct usb_phy *x)
+void tegra_usb_phy_postresume(struct usb_phy *u_phy)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 
 	if (!phy->is_ulpi_phy)
 		utmi_phy_postresume(phy);
 }
 EXPORT_SYMBOL_GPL(tegra_usb_phy_postresume);
 
-void tegra_ehci_phy_restore_start(struct usb_phy *x,
-				 enum tegra_usb_phy_port_speed port_speed)
+void tegra_ehci_phy_restore_start(struct usb_phy *u_phy,
+				  enum tegra_usb_phy_port_speed port_speed)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 
 	if (!phy->is_ulpi_phy)
 		utmi_phy_restore_start(phy, port_speed);
 }
 EXPORT_SYMBOL_GPL(tegra_ehci_phy_restore_start);
 
-void tegra_ehci_phy_restore_end(struct usb_phy *x)
+void tegra_ehci_phy_restore_end(struct usb_phy *u_phy)
 {
-	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
+	struct tegra_usb_phy *phy = to_tegra_usb_phy(u_phy);
 
 	if (!phy->is_ulpi_phy)
 		utmi_phy_restore_end(phy);
@@ -912,21 +941,25 @@ static int read_utmi_param(struct platform_device *pdev, const char *param,
 			   u8 *dest)
 {
 	u32 value;
-	int err = of_property_read_u32(pdev->dev.of_node, param, &value);
-	*dest = (u8)value;
+	int err;
+
+	err = of_property_read_u32(pdev->dev.of_node, param, &value);
 	if (err < 0)
 		dev_err(&pdev->dev,
 			"Failed to read USB UTMI parameter %s: %d\n",
 			param, err);
+	else
+		*dest = (u8)value;
+
 	return err;
 }
 
 static int utmi_phy_probe(struct tegra_usb_phy *tegra_phy,
 			  struct platform_device *pdev)
 {
+	struct tegra_utmip_config *config;
 	struct resource *res;
 	int err;
-	struct tegra_utmip_config *config;
 
 	tegra_phy->is_ulpi_phy = false;
 
@@ -937,7 +970,7 @@ static int utmi_phy_probe(struct tegra_usb_phy *tegra_phy,
 	}
 
 	tegra_phy->pad_regs = devm_ioremap(&pdev->dev, res->start,
-		resource_size(res));
+					   resource_size(res));
 	if (!tegra_phy->pad_regs) {
 		dev_err(&pdev->dev, "Failed to remap UTMI pad regs\n");
 		return -ENOMEM;
@@ -951,48 +984,48 @@ static int utmi_phy_probe(struct tegra_usb_phy *tegra_phy,
 	config = tegra_phy->config;
 
 	err = read_utmi_param(pdev, "nvidia,hssync-start-delay",
-		&config->hssync_start_delay);
+			      &config->hssync_start_delay);
 	if (err < 0)
 		return err;
 
 	err = read_utmi_param(pdev, "nvidia,elastic-limit",
-		&config->elastic_limit);
+			      &config->elastic_limit);
 	if (err < 0)
 		return err;
 
 	err = read_utmi_param(pdev, "nvidia,idle-wait-delay",
-		&config->idle_wait_delay);
+			      &config->idle_wait_delay);
 	if (err < 0)
 		return err;
 
 	err = read_utmi_param(pdev, "nvidia,term-range-adj",
-		&config->term_range_adj);
+			      &config->term_range_adj);
 	if (err < 0)
 		return err;
 
 	err = read_utmi_param(pdev, "nvidia,xcvr-lsfslew",
-		&config->xcvr_lsfslew);
+			      &config->xcvr_lsfslew);
 	if (err < 0)
 		return err;
 
 	err = read_utmi_param(pdev, "nvidia,xcvr-lsrslew",
-		&config->xcvr_lsrslew);
+			      &config->xcvr_lsrslew);
 	if (err < 0)
 		return err;
 
 	if (tegra_phy->soc_config->requires_extra_tuning_parameters) {
 		err = read_utmi_param(pdev, "nvidia,xcvr-hsslew",
-			&config->xcvr_hsslew);
+				      &config->xcvr_hsslew);
 		if (err < 0)
 			return err;
 
 		err = read_utmi_param(pdev, "nvidia,hssquelch-level",
-			&config->hssquelch_level);
+				      &config->hssquelch_level);
 		if (err < 0)
 			return err;
 
 		err = read_utmi_param(pdev, "nvidia,hsdiscon-level",
-			&config->hsdiscon_level);
+				      &config->hsdiscon_level);
 		if (err < 0)
 			return err;
 	}
@@ -1002,7 +1035,7 @@ static int utmi_phy_probe(struct tegra_usb_phy *tegra_phy,
 
 	if (!config->xcvr_setup_use_fuses) {
 		err = read_utmi_param(pdev, "nvidia,xcvr-setup",
-			&config->xcvr_setup);
+				      &config->xcvr_setup);
 		if (err < 0)
 			return err;
 	}
@@ -1033,23 +1066,17 @@ MODULE_DEVICE_TABLE(of, tegra_usb_phy_id_table);
 
 static int tegra_usb_phy_probe(struct platform_device *pdev)
 {
-	const struct of_device_id *match;
-	struct resource *res;
-	struct tegra_usb_phy *tegra_phy = NULL;
 	struct device_node *np = pdev->dev.of_node;
+	struct tegra_usb_phy *tegra_phy = NULL;
 	enum usb_phy_interface phy_type;
+	struct resource *res;
 	int err;
 
 	tegra_phy = devm_kzalloc(&pdev->dev, sizeof(*tegra_phy), GFP_KERNEL);
 	if (!tegra_phy)
 		return -ENOMEM;
 
-	match = of_match_device(tegra_usb_phy_id_table, &pdev->dev);
-	if (!match) {
-		dev_err(&pdev->dev, "Error: No device match found\n");
-		return -ENODEV;
-	}
-	tegra_phy->soc_config = match->data;
+	tegra_phy->soc_config = of_device_get_match_data(&pdev->dev);
 
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
 	if (!res) {
@@ -1058,7 +1085,7 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 	}
 
 	tegra_phy->regs = devm_ioremap(&pdev->dev, res->start,
-		resource_size(res));
+				       resource_size(res));
 	if (!tegra_phy->regs) {
 		dev_err(&pdev->dev, "Failed to remap I/O memory\n");
 		return -ENOMEM;
@@ -1080,12 +1107,12 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 
 		tegra_phy->reset_gpio =
 			of_get_named_gpio(np, "nvidia,phy-reset-gpio", 0);
+
 		if (!gpio_is_valid(tegra_phy->reset_gpio)) {
 			dev_err(&pdev->dev,
 				"Invalid GPIO: %d\n", tegra_phy->reset_gpio);
 			return tegra_phy->reset_gpio;
 		}
-		tegra_phy->config = NULL;
 		break;
 
 	default:
@@ -1131,7 +1158,7 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 		}
 
 		err = devm_gpio_request(&pdev->dev, tegra_phy->reset_gpio,
-			"ulpi_phy_reset_b");
+					"ulpi_phy_reset_b");
 		if (err < 0) {
 			dev_err(&pdev->dev, "Request failed for GPIO %d: %d\n",
 				tegra_phy->reset_gpio, err);
-- 
2.24.0

Re: [PATCH v1 3/4] usb: phy: tegra: Perform general clean up of the code

[Thierry Reding]

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

On Wed, Dec 18, 2019 at 08:53:12PM +0300, Dmitry Osipenko wrote:
> This patch fixes few dozens of legit checkpatch warnings, adds missed
> handling of potential error-cases, fixes ULPI clk-prepare refcounting and
> prettifies code where makes sense. All these clean-up changes are quite
> minor and do not fix any problems.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/phy/phy-tegra-usb.c | 367 +++++++++++++++++---------------
>  1 file changed, 197 insertions(+), 170 deletions(-)

This could've been multiple patches to make it easier to review, but
either way:

Acked-by: Thierry Reding <treding@nvidia.com>

One minor comment below...

> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index 15bd253d53c9..76949dbbbdc2 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
[...]
> @@ -310,13 +315,16 @@ static void ulpi_close(struct tegra_usb_phy *phy)
>  	}
>  }
>  
> -static void utmip_pad_power_on(struct tegra_usb_phy *phy)
> +static int utmip_pad_power_on(struct tegra_usb_phy *phy)
>  {
> -	unsigned long val, flags;
> -	void __iomem *base = phy->pad_regs;
>  	struct tegra_utmip_config *config = phy->config;
> +	void __iomem *base = phy->pad_regs;
> +	unsigned long val, flags;

I think technically the "val" variable would have to be u32 because
that's what readl() and writel() operate on. That could be a separate
patch, though and isn't really a big problem.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v1 3/4] usb: phy: tegra: Perform general clean up of the code

[Dmitry Osipenko]

19.12.2019 15:54, Thierry Reding пишет:
> On Wed, Dec 18, 2019 at 08:53:12PM +0300, Dmitry Osipenko wrote:
>> This patch fixes few dozens of legit checkpatch warnings, adds missed
>> handling of potential error-cases, fixes ULPI clk-prepare refcounting and
>> prettifies code where makes sense. All these clean-up changes are quite
>> minor and do not fix any problems.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/usb/phy/phy-tegra-usb.c | 367 +++++++++++++++++---------------
>>  1 file changed, 197 insertions(+), 170 deletions(-)
> 
> This could've been multiple patches to make it easier to review, but
> either way:
> 
> Acked-by: Thierry Reding <treding@nvidia.com>

Yes, it probably could be separated into 10 patches, but then there is a
chance that I won't do it because sometimes it takes more time for me to
write commit message than to make a code change + extra time to re-check
every patch before sending it out :)

> One minor comment below...
> 
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index 15bd253d53c9..76949dbbbdc2 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
> [...]
>> @@ -310,13 +315,16 @@ static void ulpi_close(struct tegra_usb_phy *phy)
>>  	}
>>  }
>>  
>> -static void utmip_pad_power_on(struct tegra_usb_phy *phy)
>> +static int utmip_pad_power_on(struct tegra_usb_phy *phy)
>>  {
>> -	unsigned long val, flags;
>> -	void __iomem *base = phy->pad_regs;
>>  	struct tegra_utmip_config *config = phy->config;
>> +	void __iomem *base = phy->pad_regs;
>> +	unsigned long val, flags;
> 
> I think technically the "val" variable would have to be u32 because
> that's what readl() and writel() operate on.

I had the same thought and decided it's not worth the extra effort.

> That could be a separate
> patch, though and isn't really a big problem.

But I'll do it now for complicity, since you're asking about it (in a v2
perhaps).

Thank you very much for the reviews!

[PATCH v1 4/4] usb: phy: tegra: Use relaxed versions of readl/writel

[Dmitry Osipenko]

There is nothing to synchronize in regards to memory stores, thus all
readl/writel occurrences in the code could be replaced with a relaxed
versions, for consistency.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 195 ++++++++++++++++----------------
 1 file changed, 98 insertions(+), 97 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 76949dbbbdc2..a1cc09c29695 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -205,15 +205,16 @@ static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
 	unsigned long val;
 
 	if (phy->soc_config->has_hostpc) {
-		val = readl(base + TEGRA_USB_HOSTPC1_DEVLC);
+		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
 		val &= ~TEGRA_USB_HOSTPC1_DEVLC_PTS(~0);
 		val |= TEGRA_USB_HOSTPC1_DEVLC_PTS(pts_val);
-		writel(val, base + TEGRA_USB_HOSTPC1_DEVLC);
+		writel_relaxed(val, base + TEGRA_USB_HOSTPC1_DEVLC);
 	} else {
-		val = readl(base + TEGRA_USB_PORTSC1) & ~TEGRA_PORTSC1_RWC_BITS;
+		val = readl_relaxed(base + TEGRA_USB_PORTSC1);
+		val &= ~TEGRA_PORTSC1_RWC_BITS;
 		val &= ~TEGRA_USB_PORTSC1_PTS(~0);
 		val |= TEGRA_USB_PORTSC1_PTS(pts_val);
-		writel(val, base + TEGRA_USB_PORTSC1);
+		writel_relaxed(val, base + TEGRA_USB_PORTSC1);
 	}
 }
 
@@ -223,19 +224,19 @@ static void set_phcd(struct tegra_usb_phy *phy, bool enable)
 	unsigned long val;
 
 	if (phy->soc_config->has_hostpc) {
-		val = readl(base + TEGRA_USB_HOSTPC1_DEVLC);
+		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
 		if (enable)
 			val |= TEGRA_USB_HOSTPC1_DEVLC_PHCD;
 		else
 			val &= ~TEGRA_USB_HOSTPC1_DEVLC_PHCD;
-		writel(val, base + TEGRA_USB_HOSTPC1_DEVLC);
+		writel_relaxed(val, base + TEGRA_USB_HOSTPC1_DEVLC);
 	} else {
-		val = readl(base + TEGRA_USB_PORTSC1) & ~PORT_RWC_BITS;
+		val = readl_relaxed(base + TEGRA_USB_PORTSC1) & ~PORT_RWC_BITS;
 		if (enable)
 			val |= TEGRA_USB_PORTSC1_PHCD;
 		else
 			val &= ~TEGRA_USB_PORTSC1_PHCD;
-		writel(val, base + TEGRA_USB_PORTSC1);
+		writel_relaxed(val, base + TEGRA_USB_PORTSC1);
 	}
 }
 
@@ -329,7 +330,7 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
 	spin_lock_irqsave(&utmip_pad_lock, flags);
 
 	if (utmip_pad_count++ == 0) {
-		val = readl(base + UTMIP_BIAS_CFG0);
+		val = readl_relaxed(base + UTMIP_BIAS_CFG0);
 		val &= ~(UTMIP_OTGPD | UTMIP_BIASPD);
 
 		if (phy->soc_config->requires_extra_tuning_parameters) {
@@ -341,7 +342,7 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
 			val |= UTMIP_HSDISCON_LEVEL(config->hsdiscon_level);
 			val |= UTMIP_HSDISCON_LEVEL_MSB(config->hsdiscon_level);
 		}
-		writel(val, base + UTMIP_BIAS_CFG0);
+		writel_relaxed(val, base + UTMIP_BIAS_CFG0);
 	}
 
 	spin_unlock_irqrestore(&utmip_pad_lock, flags);
@@ -369,9 +370,9 @@ static int utmip_pad_power_off(struct tegra_usb_phy *phy)
 	spin_lock_irqsave(&utmip_pad_lock, flags);
 
 	if (--utmip_pad_count == 0) {
-		val = readl(base + UTMIP_BIAS_CFG0);
+		val = readl_relaxed(base + UTMIP_BIAS_CFG0);
 		val |= UTMIP_OTGPD | UTMIP_BIASPD;
-		writel(val, base + UTMIP_BIAS_CFG0);
+		writel_relaxed(val, base + UTMIP_BIAS_CFG0);
 	}
 
 	spin_unlock_irqrestore(&utmip_pad_lock, flags);
@@ -385,8 +386,8 @@ static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
 {
 	u32 tmp;
 
-	return readl_poll_timeout(reg, tmp, (tmp & mask) == result,
-				  2000, 6000);
+	return readl_relaxed_poll_timeout(reg, tmp, (tmp & mask) == result,
+					  2000, 6000);
 }
 
 static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
@@ -403,15 +404,15 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		return;
 
 	if (phy->is_legacy_phy) {
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_SET;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 
 		usleep_range(10, 100);
 
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 	} else {
 		set_phcd(phy, true);
 	}
@@ -436,15 +437,15 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		return;
 
 	if (phy->is_legacy_phy) {
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_CLR;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 
 		usleep_range(10, 100);
 
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_CLR;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 	} else {
 		set_phcd(phy, false);
 	}
@@ -462,75 +463,75 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 	unsigned long val;
 	int err;
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val |= UTMIP_RESET;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
 	if (phy->is_legacy_phy) {
-		val = readl(base + USB1_LEGACY_CTRL);
+		val = readl_relaxed(base + USB1_LEGACY_CTRL);
 		val |= USB1_NO_LEGACY_MODE;
-		writel(val, base + USB1_LEGACY_CTRL);
+		writel_relaxed(val, base + USB1_LEGACY_CTRL);
 	}
 
-	val = readl(base + UTMIP_TX_CFG0);
+	val = readl_relaxed(base + UTMIP_TX_CFG0);
 	val |= UTMIP_FS_PREABMLE_J;
-	writel(val, base + UTMIP_TX_CFG0);
+	writel_relaxed(val, base + UTMIP_TX_CFG0);
 
-	val = readl(base + UTMIP_HSRX_CFG0);
+	val = readl_relaxed(base + UTMIP_HSRX_CFG0);
 	val &= ~(UTMIP_IDLE_WAIT(~0) | UTMIP_ELASTIC_LIMIT(~0));
 	val |= UTMIP_IDLE_WAIT(config->idle_wait_delay);
 	val |= UTMIP_ELASTIC_LIMIT(config->elastic_limit);
-	writel(val, base + UTMIP_HSRX_CFG0);
+	writel_relaxed(val, base + UTMIP_HSRX_CFG0);
 
-	val = readl(base + UTMIP_HSRX_CFG1);
+	val = readl_relaxed(base + UTMIP_HSRX_CFG1);
 	val &= ~UTMIP_HS_SYNC_START_DLY(~0);
 	val |= UTMIP_HS_SYNC_START_DLY(config->hssync_start_delay);
-	writel(val, base + UTMIP_HSRX_CFG1);
+	writel_relaxed(val, base + UTMIP_HSRX_CFG1);
 
-	val = readl(base + UTMIP_DEBOUNCE_CFG0);
+	val = readl_relaxed(base + UTMIP_DEBOUNCE_CFG0);
 	val &= ~UTMIP_BIAS_DEBOUNCE_A(~0);
 	val |= UTMIP_BIAS_DEBOUNCE_A(phy->freq->debounce);
-	writel(val, base + UTMIP_DEBOUNCE_CFG0);
+	writel_relaxed(val, base + UTMIP_DEBOUNCE_CFG0);
 
-	val = readl(base + UTMIP_MISC_CFG0);
+	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_SUSPEND_EXIT_ON_EDGE;
-	writel(val, base + UTMIP_MISC_CFG0);
+	writel_relaxed(val, base + UTMIP_MISC_CFG0);
 
 	if (!phy->soc_config->utmi_pll_config_in_car_module) {
-		val = readl(base + UTMIP_MISC_CFG1);
+		val = readl_relaxed(base + UTMIP_MISC_CFG1);
 		val &= ~(UTMIP_PLL_ACTIVE_DLY_COUNT(~0) |
 			UTMIP_PLLU_STABLE_COUNT(~0));
 		val |= UTMIP_PLL_ACTIVE_DLY_COUNT(phy->freq->active_delay) |
 			UTMIP_PLLU_STABLE_COUNT(phy->freq->stable_count);
-		writel(val, base + UTMIP_MISC_CFG1);
+		writel_relaxed(val, base + UTMIP_MISC_CFG1);
 
-		val = readl(base + UTMIP_PLL_CFG1);
+		val = readl_relaxed(base + UTMIP_PLL_CFG1);
 		val &= ~(UTMIP_XTAL_FREQ_COUNT(~0) |
 			UTMIP_PLLU_ENABLE_DLY_COUNT(~0));
 		val |= UTMIP_XTAL_FREQ_COUNT(phy->freq->xtal_freq_count) |
 			UTMIP_PLLU_ENABLE_DLY_COUNT(phy->freq->enable_delay);
-		writel(val, base + UTMIP_PLL_CFG1);
+		writel_relaxed(val, base + UTMIP_PLL_CFG1);
 	}
 
 	if (phy->mode == USB_DR_MODE_PERIPHERAL) {
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val &= ~(USB_WAKE_ON_CNNT_EN_DEV | USB_WAKE_ON_DISCON_EN_DEV);
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 
-		val = readl(base + UTMIP_BAT_CHRG_CFG0);
+		val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0);
 		val &= ~UTMIP_PD_CHRG;
-		writel(val, base + UTMIP_BAT_CHRG_CFG0);
+		writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0);
 	} else {
-		val = readl(base + UTMIP_BAT_CHRG_CFG0);
+		val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0);
 		val |= UTMIP_PD_CHRG;
-		writel(val, base + UTMIP_BAT_CHRG_CFG0);
+		writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0);
 	}
 
 	err = utmip_pad_power_on(phy);
 	if (err)
 		return err;
 
-	val = readl(base + UTMIP_XCVR_CFG0);
+	val = readl_relaxed(base + UTMIP_XCVR_CFG0);
 	val &= ~(UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN |
 		 UTMIP_FORCE_PDZI_POWERDOWN | UTMIP_XCVR_LSBIAS_SEL |
 		 UTMIP_XCVR_SETUP(~0) | UTMIP_XCVR_SETUP_MSB(~0) |
@@ -548,57 +549,57 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 		val |= UTMIP_XCVR_HSSLEW(config->xcvr_hsslew);
 		val |= UTMIP_XCVR_HSSLEW_MSB(config->xcvr_hsslew);
 	}
-	writel(val, base + UTMIP_XCVR_CFG0);
+	writel_relaxed(val, base + UTMIP_XCVR_CFG0);
 
-	val = readl(base + UTMIP_XCVR_CFG1);
+	val = readl_relaxed(base + UTMIP_XCVR_CFG1);
 	val &= ~(UTMIP_FORCE_PDDISC_POWERDOWN | UTMIP_FORCE_PDCHRP_POWERDOWN |
 		 UTMIP_FORCE_PDDR_POWERDOWN | UTMIP_XCVR_TERM_RANGE_ADJ(~0));
 	val |= UTMIP_XCVR_TERM_RANGE_ADJ(config->term_range_adj);
-	writel(val, base + UTMIP_XCVR_CFG1);
+	writel_relaxed(val, base + UTMIP_XCVR_CFG1);
 
-	val = readl(base + UTMIP_BIAS_CFG1);
+	val = readl_relaxed(base + UTMIP_BIAS_CFG1);
 	val &= ~UTMIP_BIAS_PDTRK_COUNT(~0);
 	val |= UTMIP_BIAS_PDTRK_COUNT(0x5);
-	writel(val, base + UTMIP_BIAS_CFG1);
+	writel_relaxed(val, base + UTMIP_BIAS_CFG1);
 
-	val = readl(base + UTMIP_SPARE_CFG0);
+	val = readl_relaxed(base + UTMIP_SPARE_CFG0);
 	if (config->xcvr_setup_use_fuses)
 		val |= FUSE_SETUP_SEL;
 	else
 		val &= ~FUSE_SETUP_SEL;
-	writel(val, base + UTMIP_SPARE_CFG0);
+	writel_relaxed(val, base + UTMIP_SPARE_CFG0);
 
 	if (!phy->is_legacy_phy) {
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val |= UTMIP_PHY_ENABLE;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 	}
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val &= ~UTMIP_RESET;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
 	if (phy->is_legacy_phy) {
-		val = readl(base + USB1_LEGACY_CTRL);
+		val = readl_relaxed(base + USB1_LEGACY_CTRL);
 		val &= ~USB1_VBUS_SENSE_CTL_MASK;
 		val |= USB1_VBUS_SENSE_CTL_A_SESS_VLD;
-		writel(val, base + USB1_LEGACY_CTRL);
+		writel_relaxed(val, base + USB1_LEGACY_CTRL);
 
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 	}
 
 	utmi_phy_clk_enable(phy);
 
 	if (phy->soc_config->requires_usbmode_setup) {
-		val = readl(base + USB_USBMODE);
+		val = readl_relaxed(base + USB_USBMODE);
 		val &= ~USB_USBMODE_MASK;
 		if (phy->mode == USB_DR_MODE_HOST)
 			val |= USB_USBMODE_HOST;
 		else
 			val |= USB_USBMODE_DEVICE;
-		writel(val, base + USB_USBMODE);
+		writel_relaxed(val, base + USB_USBMODE);
 	}
 
 	if (!phy->is_legacy_phy)
@@ -615,29 +616,29 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 	utmi_phy_clk_disable(phy);
 
 	if (phy->mode == USB_DR_MODE_PERIPHERAL) {
-		val = readl(base + USB_SUSP_CTRL);
+		val = readl_relaxed(base + USB_SUSP_CTRL);
 		val &= ~USB_WAKEUP_DEBOUNCE_COUNT(~0);
 		val |= USB_WAKE_ON_CNNT_EN_DEV | USB_WAKEUP_DEBOUNCE_COUNT(5);
-		writel(val, base + USB_SUSP_CTRL);
+		writel_relaxed(val, base + USB_SUSP_CTRL);
 	}
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val |= UTMIP_RESET;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
-	val = readl(base + UTMIP_BAT_CHRG_CFG0);
+	val = readl_relaxed(base + UTMIP_BAT_CHRG_CFG0);
 	val |= UTMIP_PD_CHRG;
-	writel(val, base + UTMIP_BAT_CHRG_CFG0);
+	writel_relaxed(val, base + UTMIP_BAT_CHRG_CFG0);
 
-	val = readl(base + UTMIP_XCVR_CFG0);
+	val = readl_relaxed(base + UTMIP_XCVR_CFG0);
 	val |= UTMIP_FORCE_PD_POWERDOWN | UTMIP_FORCE_PD2_POWERDOWN |
 	       UTMIP_FORCE_PDZI_POWERDOWN;
-	writel(val, base + UTMIP_XCVR_CFG0);
+	writel_relaxed(val, base + UTMIP_XCVR_CFG0);
 
-	val = readl(base + UTMIP_XCVR_CFG1);
+	val = readl_relaxed(base + UTMIP_XCVR_CFG1);
 	val |= UTMIP_FORCE_PDDISC_POWERDOWN | UTMIP_FORCE_PDCHRP_POWERDOWN |
 	       UTMIP_FORCE_PDDR_POWERDOWN;
-	writel(val, base + UTMIP_XCVR_CFG1);
+	writel_relaxed(val, base + UTMIP_XCVR_CFG1);
 
 	return utmip_pad_power_off(phy);
 }
@@ -647,9 +648,9 @@ static void utmi_phy_preresume(struct tegra_usb_phy *phy)
 	void __iomem *base = phy->regs;
 	unsigned long val;
 
-	val = readl(base + UTMIP_TX_CFG0);
+	val = readl_relaxed(base + UTMIP_TX_CFG0);
 	val |= UTMIP_HS_DISCON_DISABLE;
-	writel(val, base + UTMIP_TX_CFG0);
+	writel_relaxed(val, base + UTMIP_TX_CFG0);
 }
 
 static void utmi_phy_postresume(struct tegra_usb_phy *phy)
@@ -657,9 +658,9 @@ static void utmi_phy_postresume(struct tegra_usb_phy *phy)
 	void __iomem *base = phy->regs;
 	unsigned long val;
 
-	val = readl(base + UTMIP_TX_CFG0);
+	val = readl_relaxed(base + UTMIP_TX_CFG0);
 	val &= ~UTMIP_HS_DISCON_DISABLE;
-	writel(val, base + UTMIP_TX_CFG0);
+	writel_relaxed(val, base + UTMIP_TX_CFG0);
 }
 
 static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
@@ -668,18 +669,18 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
 	void __iomem *base = phy->regs;
 	unsigned long val;
 
-	val = readl(base + UTMIP_MISC_CFG0);
+	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE_SEL(~0);
 	if (port_speed == TEGRA_USB_PHY_PORT_SPEED_LOW)
 		val |= UTMIP_DPDM_OBSERVE_SEL_FS_K;
 	else
 		val |= UTMIP_DPDM_OBSERVE_SEL_FS_J;
-	writel(val, base + UTMIP_MISC_CFG0);
+	writel_relaxed(val, base + UTMIP_MISC_CFG0);
 	usleep_range(1, 10);
 
-	val = readl(base + UTMIP_MISC_CFG0);
+	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val |= UTMIP_DPDM_OBSERVE;
-	writel(val, base + UTMIP_MISC_CFG0);
+	writel_relaxed(val, base + UTMIP_MISC_CFG0);
 	usleep_range(10, 100);
 }
 
@@ -688,9 +689,9 @@ static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
 	void __iomem *base = phy->regs;
 	unsigned long val;
 
-	val = readl(base + UTMIP_MISC_CFG0);
+	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE;
-	writel(val, base + UTMIP_MISC_CFG0);
+	writel_relaxed(val, base + UTMIP_MISC_CFG0);
 	usleep_range(10, 100);
 }
 
@@ -722,31 +723,31 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 
 	usleep_range(1000, 2000);
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val |= UHSIC_RESET;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
-	val = readl(base + ULPI_TIMING_CTRL_0);
+	val = readl_relaxed(base + ULPI_TIMING_CTRL_0);
 	val |= ULPI_OUTPUT_PINMUX_BYP | ULPI_CLKOUT_PINMUX_BYP;
-	writel(val, base + ULPI_TIMING_CTRL_0);
+	writel_relaxed(val, base + ULPI_TIMING_CTRL_0);
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val |= ULPI_PHY_ENABLE;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
 	val = 0;
-	writel(val, base + ULPI_TIMING_CTRL_1);
+	writel_relaxed(val, base + ULPI_TIMING_CTRL_1);
 
 	val |= ULPI_DATA_TRIMMER_SEL(4);
 	val |= ULPI_STPDIRNXT_TRIMMER_SEL(4);
 	val |= ULPI_DIR_TRIMMER_SEL(4);
-	writel(val, base + ULPI_TIMING_CTRL_1);
+	writel_relaxed(val, base + ULPI_TIMING_CTRL_1);
 	usleep_range(10, 100);
 
 	val |= ULPI_DATA_TRIMMER_LOAD;
 	val |= ULPI_STPDIRNXT_TRIMMER_LOAD;
 	val |= ULPI_DIR_TRIMMER_LOAD;
-	writel(val, base + ULPI_TIMING_CTRL_1);
+	writel_relaxed(val, base + ULPI_TIMING_CTRL_1);
 
 	/* Fix VbusInvalid due to floating VBUS */
 	err = usb_phy_io_write(phy->ulpi, 0x40, 0x08);
@@ -761,14 +762,14 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 		goto disable_clk;
 	}
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val |= USB_SUSP_CLR;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 	usleep_range(100, 1000);
 
-	val = readl(base + USB_SUSP_CTRL);
+	val = readl_relaxed(base + USB_SUSP_CTRL);
 	val &= ~USB_SUSP_CLR;
-	writel(val, base + USB_SUSP_CTRL);
+	writel_relaxed(val, base + USB_SUSP_CTRL);
 
 	return 0;
 
-- 
2.24.0

Re: [PATCH v1 4/4] usb: phy: tegra: Use relaxed versions of readl/writel

[Thierry Reding]

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

On Wed, Dec 18, 2019 at 08:53:13PM +0300, Dmitry Osipenko wrote:
> There is nothing to synchronize in regards to memory stores, thus all
> readl/writel occurrences in the code could be replaced with a relaxed
> versions, for consistency.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/phy/phy-tegra-usb.c | 195 ++++++++++++++++----------------
>  1 file changed, 98 insertions(+), 97 deletions(-)

Acked-by: Thierry Reding <treding@nvidia.com>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]