devicetree.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up
@ 2019-12-20  1:52 Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support Dmitry Osipenko
                   ` (9 more replies)
  0 siblings, 10 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

Hello,

This 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!

Changelog:

v2: - The "usb: phy: tegra: Hook up init/shutdown callbacks" patch was
      updated and now it does a better job in regards to checking whether
      PHY is initialized before it is started to be used and whether there
      is a double init/shutdown. This allows to factor out the ChipIdea's
      driver change into a separate patch, which was requested by Peter Chen
      in a review comment to v1. In a result there is this new patch:

        usb: chipidea: tegra: Stop managing PHY's power

    - Added few more new patches:

        usb: phy: tegra: Use generic stub for a missing VBUS regulator

      I noticed that VBUS regulator usage could be cleaned up a tad as well.

        usb: ulpi: Add resource-managed variant of otg_ulpi_create()
        usb: phy: tegra: Use devm_otg_ulpi_create()
        usb: phy: tegra: Use u32 for hardware register variables

      These patches are made in response to review comments that were made
      by Thierry Reding to v1.

      I also noticed that phy_tegra_usb isn't getting auto-loaded while it
      should be. This is fixed in this new patch:

        usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies

Dmitry Osipenko (10):
  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
  usb: phy: tegra: Use generic stub for a missing VBUS regulator
  usb: ulpi: Add resource-managed variant of otg_ulpi_create()
  usb: phy: tegra: Use devm_otg_ulpi_create()
  usb: phy: tegra: Use u32 for hardware register variables
  usb: chipidea: tegra: Stop managing PHY's power
  usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's
    dependencies

 .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |   4 +
 drivers/usb/chipidea/Kconfig                  |   1 +
 drivers/usb/chipidea/ci_hdrc_tegra.c          |  15 +-
 drivers/usb/phy/phy-tegra-usb.c               | 740 ++++++++++--------
 drivers/usb/phy/phy-ulpi.c                    |  48 +-
 include/linux/usb/ulpi.h                      |  11 +
 6 files changed, 462 insertions(+), 357 deletions(-)

-- 
2.24.0


^ permalink raw reply	[flat|nested] 28+ messages in thread

* [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 02/10] usb: phy: tegra: Hook up init/shutdown callbacks Dmitry Osipenko
                   ` (8 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

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

Acked-by: Peter Chen <peter.chen@nxp.com>
Acked-by: Thierry Reding <treding@nvidia.com>
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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 02/10] usb: phy: tegra: Hook up init/shutdown callbacks
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 03/10] usb: phy: tegra: Perform general clean up of the code Dmitry Osipenko
                   ` (7 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

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.

Acked-by: Peter Chen <peter.chen@nxp.com>
Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 178 +++++++++++++++++++++-----------
 1 file changed, 116 insertions(+), 62 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index ea7ef1dc0b42..12d6f6433365 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,15 +756,25 @@ 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_shutdown(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 (WARN_ON(!phy->freq))
+		return;
+
+	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);
+
+	phy->freq = NULL;
 }
 
 static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
@@ -788,9 +793,13 @@ 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 (WARN_ON(!phy->freq))
+		return -EINVAL;
+
 	if (suspend)
 		return tegra_usb_phy_power_off(phy);
 	else
@@ -801,53 +810,27 @@ 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;
-	}
+	if (WARN_ON(phy->freq))
+		return 0;
 
 	err = clk_prepare_enable(phy->pll_u);
 	if (err)
@@ -884,10 +867,22 @@ 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);
+
+	phy->freq = NULL;
+
 	return err;
 }
 
@@ -1134,22 +1129,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_shutdown;
 	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 +1207,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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 03/10] usb: phy: tegra: Perform general clean up of the code
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 02/10] usb: phy: tegra: Hook up init/shutdown callbacks Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 04/10] usb: phy: tegra: Use relaxed versions of readl/writel Dmitry Osipenko
                   ` (6 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

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 real problems.

Acked-by: Thierry Reding <treding@nvidia.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 365 +++++++++++++++++---------------
 1 file changed, 195 insertions(+), 170 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 12d6f6433365..50b9d4574608 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_shutdown(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 (WARN_ON(!phy->freq))
 		return;
@@ -793,9 +821,9 @@ 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 (WARN_ON(!phy->freq))
 		return -EINVAL;
@@ -823,10 +851,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;
 
 	if (WARN_ON(phy->freq))
@@ -886,37 +913,37 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy)
 	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);
@@ -927,21 +954,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;
 
@@ -952,7 +983,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;
@@ -966,48 +997,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;
 	}
@@ -1017,7 +1048,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;
 	}
@@ -1048,23 +1079,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;
 	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) {
@@ -1073,7 +1098,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;
@@ -1095,12 +1120,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:
@@ -1146,7 +1171,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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 04/10] usb: phy: tegra: Use relaxed versions of readl/writel
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (2 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 03/10] usb: phy: tegra: Perform general clean up of the code Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 05/10] usb: phy: tegra: Use generic stub for a missing VBUS regulator Dmitry Osipenko
                   ` (5 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

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.

Acked-by: Thierry Reding <treding@nvidia.com>
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 50b9d4574608..7456479099ce 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


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 05/10] usb: phy: tegra: Use generic stub for a missing VBUS regulator
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (3 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 04/10] usb: phy: tegra: Use relaxed versions of readl/writel Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 06/10] usb: ulpi: Add resource-managed variant of otg_ulpi_create() Dmitry Osipenko
                   ` (4 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

Regulator core provides dummy regulator if device-tree doesn't define VBUS
regulator.

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

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 7456479099ce..9ce6699f40e7 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -798,8 +798,7 @@ static void tegra_usb_phy_shutdown(struct usb_phy *u_phy)
 	else
 		utmip_pad_close(phy);
 
-	if (!IS_ERR(phy->vbus))
-		regulator_disable(phy->vbus);
+	regulator_disable(phy->vbus);
 
 	clk_disable_unprepare(phy->pll_u);
 
@@ -878,14 +877,11 @@ static int tegra_usb_phy_init(struct usb_phy *u_phy)
 		goto fail;
 	}
 
-	if (!IS_ERR(phy->vbus)) {
-		err = regulator_enable(phy->vbus);
-		if (err) {
-			dev_err(phy->u_phy.dev,
-				"Failed to enable USB VBUS regulator: %d\n",
-				err);
-			goto fail;
-		}
+	err = regulator_enable(phy->vbus);
+	if (err) {
+		dev_err(phy->u_phy.dev,
+			"Failed to enable USB VBUS regulator: %d\n", err);
+		goto fail;
 	}
 
 	if (phy->is_ulpi_phy)
@@ -1146,14 +1142,9 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 	}
 
 	/* On some boards, the VBUS regulator doesn't need to be controlled */
-	if (of_find_property(np, "vbus-supply", NULL)) {
-		tegra_phy->vbus = devm_regulator_get(&pdev->dev, "vbus");
-		if (IS_ERR(tegra_phy->vbus))
-			return PTR_ERR(tegra_phy->vbus);
-	} else {
-		dev_notice(&pdev->dev, "no vbus regulator");
-		tegra_phy->vbus = ERR_PTR(-ENODEV);
-	}
+	tegra_phy->vbus = devm_regulator_get(&pdev->dev, "vbus");
+	if (IS_ERR(tegra_phy->vbus))
+		return PTR_ERR(tegra_phy->vbus);
 
 	tegra_phy->pll_u = devm_clk_get(&pdev->dev, "pll_u");
 	err = PTR_ERR_OR_ZERO(tegra_phy);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 06/10] usb: ulpi: Add resource-managed variant of otg_ulpi_create()
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (4 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 05/10] usb: phy: tegra: Use generic stub for a missing VBUS regulator Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 07/10] usb: phy: tegra: Use devm_otg_ulpi_create() Dmitry Osipenko
                   ` (3 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

Now drivers (like NVIDIA Tegra USB PHY for example) will be able to
benefit from the resource-managed variant, making driver's code a bit
cleaner.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-ulpi.c | 48 +++++++++++++++++++++++++++++++-------
 include/linux/usb/ulpi.h   | 11 +++++++++
 2 files changed, 50 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/phy/phy-ulpi.c b/drivers/usb/phy/phy-ulpi.c
index a43c49369a60..e683a37e3a7a 100644
--- a/drivers/usb/phy/phy-ulpi.c
+++ b/drivers/usb/phy/phy-ulpi.c
@@ -240,6 +240,21 @@ static int ulpi_set_vbus(struct usb_otg *otg, bool on)
 	return usb_phy_io_write(phy, flags, ULPI_OTG_CTRL);
 }
 
+static void otg_ulpi_init(struct usb_phy *phy, struct usb_otg *otg,
+			  struct usb_phy_io_ops *ops,
+			  unsigned int flags)
+{
+	phy->label	= "ULPI";
+	phy->flags	= flags;
+	phy->io_ops	= ops;
+	phy->otg	= otg;
+	phy->init	= ulpi_init;
+
+	otg->usb_phy	= phy;
+	otg->set_host	= ulpi_set_host;
+	otg->set_vbus	= ulpi_set_vbus;
+}
+
 struct usb_phy *
 otg_ulpi_create(struct usb_phy_io_ops *ops,
 		unsigned int flags)
@@ -257,17 +272,32 @@ otg_ulpi_create(struct usb_phy_io_ops *ops,
 		return NULL;
 	}
 
-	phy->label	= "ULPI";
-	phy->flags	= flags;
-	phy->io_ops	= ops;
-	phy->otg	= otg;
-	phy->init	= ulpi_init;
-
-	otg->usb_phy	= phy;
-	otg->set_host	= ulpi_set_host;
-	otg->set_vbus	= ulpi_set_vbus;
+	otg_ulpi_init(phy, otg, ops, flags);
 
 	return phy;
 }
 EXPORT_SYMBOL_GPL(otg_ulpi_create);
 
+struct usb_phy *
+devm_otg_ulpi_create(struct device *dev,
+		     struct usb_phy_io_ops *ops,
+		     unsigned int flags)
+{
+	struct usb_phy *phy;
+	struct usb_otg *otg;
+
+	phy = devm_kzalloc(dev, sizeof(*phy), GFP_KERNEL);
+	if (!phy)
+		return NULL;
+
+	otg = devm_kzalloc(dev, sizeof(*otg), GFP_KERNEL);
+	if (!otg) {
+		devm_kfree(dev, phy);
+		return NULL;
+	}
+
+	otg_ulpi_init(phy, otg, ops, flags);
+
+	return phy;
+}
+EXPORT_SYMBOL_GPL(devm_otg_ulpi_create);
diff --git a/include/linux/usb/ulpi.h b/include/linux/usb/ulpi.h
index c515765adab7..36c2982780ad 100644
--- a/include/linux/usb/ulpi.h
+++ b/include/linux/usb/ulpi.h
@@ -55,12 +55,23 @@
 #if IS_ENABLED(CONFIG_USB_ULPI)
 struct usb_phy *otg_ulpi_create(struct usb_phy_io_ops *ops,
 					unsigned int flags);
+
+struct usb_phy *devm_otg_ulpi_create(struct device *dev,
+				     struct usb_phy_io_ops *ops,
+				     unsigned int flags);
 #else
 static inline struct usb_phy *otg_ulpi_create(struct usb_phy_io_ops *ops,
 					      unsigned int flags)
 {
 	return NULL;
 }
+
+static inline struct usb_phy *devm_otg_ulpi_create(struct device *dev,
+						   struct usb_phy_io_ops *ops,
+						   unsigned int flags)
+{
+	return NULL;
+}
 #endif
 
 #ifdef CONFIG_USB_ULPI_VIEWPORT
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 07/10] usb: phy: tegra: Use devm_otg_ulpi_create()
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (5 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 06/10] usb: ulpi: Add resource-managed variant of otg_ulpi_create() Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables Dmitry Osipenko
                   ` (2 subsequent siblings)
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

The resource-managed variant removes the necessity for the driver to care
about freeing ULPI resources.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index 9ce6699f40e7..d5739b6e0b6c 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -1170,7 +1170,9 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 			return err;
 		}
 
-		tegra_phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
+		tegra_phy->ulpi = devm_otg_ulpi_create(&pdev->dev,
+						       &ulpi_viewport_access_ops,
+						       0);
 		if (!tegra_phy->ulpi) {
 			dev_err(&pdev->dev, "Failed to create ULPI OTG\n");
 			err = -ENOMEM;
@@ -1205,18 +1207,10 @@ static int tegra_usb_phy_probe(struct platform_device *pdev)
 	platform_set_drvdata(pdev, tegra_phy);
 
 	err = usb_add_phy_dev(&tegra_phy->u_phy);
-	if (err < 0)
-		goto free_ulpi;
+	if (err)
+		return err;
 
 	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)
@@ -1225,11 +1219,6 @@ static int tegra_usb_phy_remove(struct platform_device *pdev)
 
 	usb_remove_phy(&tegra_phy->u_phy);
 
-	if (tegra_phy->ulpi) {
-		kfree(tegra_phy->ulpi->otg);
-		kfree(tegra_phy->ulpi);
-	}
-
 	return 0;
 }
 
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (6 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 07/10] usb: phy: tegra: Use devm_otg_ulpi_create() Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-22 13:24   ` Dejin Zheng
  2019-12-20  1:52 ` [PATCH v2 09/10] usb: chipidea: tegra: Stop managing PHY's power Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies Dmitry Osipenko
  9 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

There is a mix of u32/ULONG usage in the driver's code. Let's switch to
u32 uniformly, for consistency.

Suggested-by: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/phy/phy-tegra-usb.c | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
index d5739b6e0b6c..551c94e3877a 100644
--- a/drivers/usb/phy/phy-tegra-usb.c
+++ b/drivers/usb/phy/phy-tegra-usb.c
@@ -202,7 +202,7 @@ static inline struct tegra_usb_phy *to_tegra_usb_phy(struct usb_phy *u_phy)
 static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	if (phy->soc_config->has_hostpc) {
 		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
@@ -221,7 +221,7 @@ static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
 static void set_phcd(struct tegra_usb_phy *phy, bool enable)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	if (phy->soc_config->has_hostpc) {
 		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
@@ -320,7 +320,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
 {
 	struct tegra_utmip_config *config = phy->config;
 	void __iomem *base = phy->pad_regs;
-	unsigned long val, flags;
+	unsigned long flags;
+	u32 val;
 	int err;
 
 	err = clk_prepare_enable(phy->pad_clk);
@@ -355,7 +356,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
 static int utmip_pad_power_off(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->pad_regs;
-	unsigned long val, flags;
+	unsigned long flags;
+	u32 val;
 	int err;
 
 	if (!utmip_pad_count) {
@@ -393,7 +395,7 @@ static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
 static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	/*
 	 * The USB driver may have already initiated the phy clock
@@ -425,7 +427,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	/*
 	 * The USB driver may have already initiated the phy clock
@@ -460,7 +462,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 {
 	struct tegra_utmip_config *config = phy->config;
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 	int err;
 
 	val = readl_relaxed(base + USB_SUSP_CTRL);
@@ -611,7 +613,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	utmi_phy_clk_disable(phy);
 
@@ -646,7 +648,7 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
 static void utmi_phy_preresume(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	val = readl_relaxed(base + UTMIP_TX_CFG0);
 	val |= UTMIP_HS_DISCON_DISABLE;
@@ -656,7 +658,7 @@ static void utmi_phy_preresume(struct tegra_usb_phy *phy)
 static void utmi_phy_postresume(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	val = readl_relaxed(base + UTMIP_TX_CFG0);
 	val &= ~UTMIP_HS_DISCON_DISABLE;
@@ -667,7 +669,7 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
 				   enum tegra_usb_phy_port_speed port_speed)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE_SEL(~0);
@@ -687,7 +689,7 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
 static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 
 	val = readl_relaxed(base + UTMIP_MISC_CFG0);
 	val &= ~UTMIP_DPDM_OBSERVE;
@@ -698,7 +700,7 @@ static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
 static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 {
 	void __iomem *base = phy->regs;
-	unsigned long val;
+	u32 val;
 	int err;
 
 	err = gpio_direction_output(phy->reset_gpio, 0);
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 09/10] usb: chipidea: tegra: Stop managing PHY's power
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (7 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  1:52 ` [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies Dmitry Osipenko
  9 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

Tegra's USB PHY driver now provides generic PHY init/shutdown callbacks
and thus the custom PHY management could be removed from Tegra-specific
part of the ChipIdea driver.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/chipidea/ci_hdrc_tegra.c | 9 ---------
 1 file changed, 9 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;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
                   ` (8 preceding siblings ...)
  2019-12-20  1:52 ` [PATCH v2 09/10] usb: chipidea: tegra: Stop managing PHY's power Dmitry Osipenko
@ 2019-12-20  1:52 ` Dmitry Osipenko
  2019-12-20  3:56   ` Peter Chen
  9 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  1:52 UTC (permalink / raw)
  To: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi
  Cc: devicetree, linux-usb, linux-tegra, linux-kernel

Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module
is loaded too regardless of kernel's configuration. Previously this
problem was masked because Tegra's EHCI driver is usually enabled in
kernel's config and thus PHY driver was getting loaded because of it, but
now I was making some more thorough testing and noticed that PHY's module
isn't getting auto-loaded without the host driver.

Note that ChipIdea's driver doesn't use any of the exported functions of
phy_tegra_usb module and thus the module needs to be requested explicitly.

Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
---
 drivers/usb/chipidea/Kconfig         | 1 +
 drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
index ae850b3fddf2..d53db520e209 100644
--- a/drivers/usb/chipidea/Kconfig
+++ b/drivers/usb/chipidea/Kconfig
@@ -7,6 +7,7 @@ config USB_CHIPIDEA
 	select RESET_CONTROLLER
 	select USB_ULPI_BUS
 	select USB_ROLE_SWITCH
+	select USB_TEGRA_PHY if ARCH_TEGRA
 	help
 	  Say Y here if your system has a dual role high speed USB
 	  controller based on ChipIdea silicon IP. It supports:
diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
index 7455df0ede49..8bc11100245d 100644
--- a/drivers/usb/chipidea/ci_hdrc_tegra.c
+++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
@@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
 	struct tegra_udc *udc;
 	int err;
 
+	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
+		err = request_module("phy_tegra_usb");
+		if (err)
+			return err;
+	}
+
 	udc = devm_kzalloc(&pdev->dev, sizeof(*udc), GFP_KERNEL);
 	if (!udc)
 		return -ENOMEM;
-- 
2.24.0


^ permalink raw reply related	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-20  1:52 ` [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies Dmitry Osipenko
@ 2019-12-20  3:56   ` Peter Chen
  2019-12-20  4:31     ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Peter Chen @ 2019-12-20  3:56 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel

On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module
> is loaded too regardless of kernel's configuration. Previously this
> problem was masked because Tegra's EHCI driver is usually enabled in
> kernel's config and thus PHY driver was getting loaded because of it, but
> now I was making some more thorough testing and noticed that PHY's module
> isn't getting auto-loaded without the host driver.
> 
> Note that ChipIdea's driver doesn't use any of the exported functions of
> phy_tegra_usb module and thus the module needs to be requested explicitly.
> 
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/chipidea/Kconfig         | 1 +
>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> index ae850b3fddf2..d53db520e209 100644
> --- a/drivers/usb/chipidea/Kconfig
> +++ b/drivers/usb/chipidea/Kconfig
> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
>  	select RESET_CONTROLLER
>  	select USB_ULPI_BUS
>  	select USB_ROLE_SWITCH
> +	select USB_TEGRA_PHY if ARCH_TEGRA
>  	help
>  	  Say Y here if your system has a dual role high speed USB
>  	  controller based on ChipIdea silicon IP. It supports:
> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> index 7455df0ede49..8bc11100245d 100644
> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>  	struct tegra_udc *udc;
>  	int err;
>  
> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> +		err = request_module("phy_tegra_usb");
> +		if (err)
> +			return err;
> +	}
> +

Why you do this dependency, if this controller driver can't
get USB PHY, it should return error. What's the return value
after calling below:

	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);

Peter
>  	udc = devm_kzalloc(&pdev->dev, sizeof(*udc), GFP_KERNEL);
>  	if (!udc)
>  		return -ENOMEM;
> -- 
> 2.24.0
> 

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-20  3:56   ` Peter Chen
@ 2019-12-20  4:31     ` Dmitry Osipenko
  2019-12-23  6:40       ` Peter Chen
  2019-12-23 21:32       ` Michał Mirosław
  0 siblings, 2 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-20  4:31 UTC (permalink / raw)
  To: Peter Chen
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel

20.12.2019 06:56, Peter Chen пишет:
> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module
>> is loaded too regardless of kernel's configuration. Previously this
>> problem was masked because Tegra's EHCI driver is usually enabled in
>> kernel's config and thus PHY driver was getting loaded because of it, but
>> now I was making some more thorough testing and noticed that PHY's module
>> isn't getting auto-loaded without the host driver.
>>
>> Note that ChipIdea's driver doesn't use any of the exported functions of
>> phy_tegra_usb module and thus the module needs to be requested explicitly.
>>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/usb/chipidea/Kconfig         | 1 +
>>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
>>  2 files changed, 7 insertions(+)
>>
>> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
>> index ae850b3fddf2..d53db520e209 100644
>> --- a/drivers/usb/chipidea/Kconfig
>> +++ b/drivers/usb/chipidea/Kconfig
>> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
>>  	select RESET_CONTROLLER
>>  	select USB_ULPI_BUS
>>  	select USB_ROLE_SWITCH
>> +	select USB_TEGRA_PHY if ARCH_TEGRA
>>  	help
>>  	  Say Y here if your system has a dual role high speed USB
>>  	  controller based on ChipIdea silicon IP. It supports:
>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> index 7455df0ede49..8bc11100245d 100644
>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>  	struct tegra_udc *udc;
>>  	int err;
>>  
>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>> +		err = request_module("phy_tegra_usb");
>> +		if (err)
>> +			return err;
>> +	}
>> +
> 
> Why you do this dependency, if this controller driver can't
> get USB PHY, it should return error. What's the return value
> after calling below:
> 
> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);

It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.

So if you'll do:

# rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
# modprobe ci_hdrc_tegra
# lsmod
Module                  Size  Used by
ci_hdrc_tegra          16384  0
ci_hdrc                45056  1 ci_hdrc_tegra

After this patch:

# rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
# modprobe ci_hdrc_tegra
# lsmod
Module                  Size  Used by
Module                  Size  Used by
phy_tegra_usb          20480  1
ci_hdrc_tegra          16384  0
ci_hdrc                45056  1 ci_hdrc_tegra

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables
  2019-12-20  1:52 ` [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables Dmitry Osipenko
@ 2019-12-22 13:24   ` Dejin Zheng
  2019-12-22 21:48     ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Dejin Zheng @ 2019-12-22 13:24 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Fri, Dec 20, 2019 at 04:52:36AM +0300, Dmitry Osipenko wrote:
> There is a mix of u32/ULONG usage in the driver's code. Let's switch to
> u32 uniformly, for consistency.
> 
> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> ---
>  drivers/usb/phy/phy-tegra-usb.c | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> index d5739b6e0b6c..551c94e3877a 100644
> --- a/drivers/usb/phy/phy-tegra-usb.c
> +++ b/drivers/usb/phy/phy-tegra-usb.c
> @@ -202,7 +202,7 @@ static inline struct tegra_usb_phy *to_tegra_usb_phy(struct usb_phy *u_phy)
>  static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	if (phy->soc_config->has_hostpc) {
>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
> @@ -221,7 +221,7 @@ static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
>  static void set_phcd(struct tegra_usb_phy *phy, bool enable)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	if (phy->soc_config->has_hostpc) {
>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
> @@ -320,7 +320,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
>  {
>  	struct tegra_utmip_config *config = phy->config;
>  	void __iomem *base = phy->pad_regs;
> -	unsigned long val, flags;
> +	unsigned long flags;
> +	u32 val;
Why are you still using unsigned long here?

>  	int err;
>  
>  	err = clk_prepare_enable(phy->pad_clk);
> @@ -355,7 +356,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
>  static int utmip_pad_power_off(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->pad_regs;
> -	unsigned long val, flags;
> +	unsigned long flags;
> +	u32 val;
ditto

>  	int err;
>  
>  	if (!utmip_pad_count) {
> @@ -393,7 +395,7 @@ static int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
>  static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	/*
>  	 * The USB driver may have already initiated the phy clock
> @@ -425,7 +427,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	/*
>  	 * The USB driver may have already initiated the phy clock
> @@ -460,7 +462,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  {
>  	struct tegra_utmip_config *config = phy->config;
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  	int err;
>  
>  	val = readl_relaxed(base + USB_SUSP_CTRL);
> @@ -611,7 +613,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  static int utmi_phy_power_off(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	utmi_phy_clk_disable(phy);
>  
> @@ -646,7 +648,7 @@ static int utmi_phy_power_off(struct tegra_usb_phy *phy)
>  static void utmi_phy_preresume(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	val = readl_relaxed(base + UTMIP_TX_CFG0);
>  	val |= UTMIP_HS_DISCON_DISABLE;
> @@ -656,7 +658,7 @@ static void utmi_phy_preresume(struct tegra_usb_phy *phy)
>  static void utmi_phy_postresume(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	val = readl_relaxed(base + UTMIP_TX_CFG0);
>  	val &= ~UTMIP_HS_DISCON_DISABLE;
> @@ -667,7 +669,7 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
>  				   enum tegra_usb_phy_port_speed port_speed)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	val = readl_relaxed(base + UTMIP_MISC_CFG0);
>  	val &= ~UTMIP_DPDM_OBSERVE_SEL(~0);
> @@ -687,7 +689,7 @@ static void utmi_phy_restore_start(struct tegra_usb_phy *phy,
>  static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  
>  	val = readl_relaxed(base + UTMIP_MISC_CFG0);
>  	val &= ~UTMIP_DPDM_OBSERVE;
> @@ -698,7 +700,7 @@ static void utmi_phy_restore_end(struct tegra_usb_phy *phy)
>  static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
>  {
>  	void __iomem *base = phy->regs;
> -	unsigned long val;
> +	u32 val;
>  	int err;
>  
>  	err = gpio_direction_output(phy->reset_gpio, 0);
> -- 
> 2.24.0
> 

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables
  2019-12-22 13:24   ` Dejin Zheng
@ 2019-12-22 21:48     ` Dmitry Osipenko
  2019-12-23 14:53       ` Dejin Zheng
  0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-22 21:48 UTC (permalink / raw)
  To: Dejin Zheng
  Cc: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

22.12.2019 16:24, Dejin Zheng пишет:
> On Fri, Dec 20, 2019 at 04:52:36AM +0300, Dmitry Osipenko wrote:
>> There is a mix of u32/ULONG usage in the driver's code. Let's switch to
>> u32 uniformly, for consistency.
>>
>> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>> ---
>>  drivers/usb/phy/phy-tegra-usb.c | 28 +++++++++++++++-------------
>>  1 file changed, 15 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
>> index d5739b6e0b6c..551c94e3877a 100644
>> --- a/drivers/usb/phy/phy-tegra-usb.c
>> +++ b/drivers/usb/phy/phy-tegra-usb.c
>> @@ -202,7 +202,7 @@ static inline struct tegra_usb_phy *to_tegra_usb_phy(struct usb_phy *u_phy)
>>  static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
>>  {
>>  	void __iomem *base = phy->regs;
>> -	unsigned long val;
>> +	u32 val;
>>  
>>  	if (phy->soc_config->has_hostpc) {
>>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
>> @@ -221,7 +221,7 @@ static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
>>  static void set_phcd(struct tegra_usb_phy *phy, bool enable)
>>  {
>>  	void __iomem *base = phy->regs;
>> -	unsigned long val;
>> +	u32 val;
>>  
>>  	if (phy->soc_config->has_hostpc) {
>>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
>> @@ -320,7 +320,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
>>  {
>>  	struct tegra_utmip_config *config = phy->config;
>>  	void __iomem *base = phy->pad_regs;
>> -	unsigned long val, flags;
>> +	unsigned long flags;
>> +	u32 val;
> Why are you still using unsigned long here?

Please take a look at [1][2], the types are matching callees.

[1]
https://elixir.bootlin.com/linux/v5.5-rc2/source/include/linux/spinlock.h#L249

[2]
https://elixir.bootlin.com/linux/v5.5-rc2/source/include/asm-generic/io.h#L297

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-20  4:31     ` Dmitry Osipenko
@ 2019-12-23  6:40       ` Peter Chen
  2019-12-23 17:23         ` Dmitry Osipenko
  2019-12-23 21:32       ` Michał Mirosław
  1 sibling, 1 reply; 28+ messages in thread
From: Peter Chen @ 2019-12-23  6:40 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel

On 19-12-20 07:31:08, Dmitry Osipenko wrote:
> 20.12.2019 06:56, Peter Chen пишет:
> > On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> >> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module
> >> is loaded too regardless of kernel's configuration. Previously this
> >> problem was masked because Tegra's EHCI driver is usually enabled in
> >> kernel's config and thus PHY driver was getting loaded because of it, but
> >> now I was making some more thorough testing and noticed that PHY's module
> >> isn't getting auto-loaded without the host driver.
> >>
> >> Note that ChipIdea's driver doesn't use any of the exported functions of
> >> phy_tegra_usb module and thus the module needs to be requested explicitly.
> >>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/usb/chipidea/Kconfig         | 1 +
> >>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
> >>  2 files changed, 7 insertions(+)
> >>
> >> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
> >> index ae850b3fddf2..d53db520e209 100644
> >> --- a/drivers/usb/chipidea/Kconfig
> >> +++ b/drivers/usb/chipidea/Kconfig
> >> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
> >>  	select RESET_CONTROLLER
> >>  	select USB_ULPI_BUS
> >>  	select USB_ROLE_SWITCH
> >> +	select USB_TEGRA_PHY if ARCH_TEGRA
> >>  	help
> >>  	  Say Y here if your system has a dual role high speed USB
> >>  	  controller based on ChipIdea silicon IP. It supports:
> >> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >> index 7455df0ede49..8bc11100245d 100644
> >> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>  	struct tegra_udc *udc;
> >>  	int err;
> >>  
> >> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >> +		err = request_module("phy_tegra_usb");
> >> +		if (err)
> >> +			return err;
> >> +	}
> >> +
> > 
> > Why you do this dependency, if this controller driver can't
> > get USB PHY, it should return error. What's the return value
> > after calling below:
> > 
> > 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
> 
> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> 
> So if you'll do:
> 
> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
> # modprobe ci_hdrc_tegra
> # lsmod
> Module                  Size  Used by
> ci_hdrc_tegra          16384  0
> ci_hdrc                45056  1 ci_hdrc_tegra
> 
> After this patch:
> 
> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
> # modprobe ci_hdrc_tegra
> # lsmod
> Module                  Size  Used by
> Module                  Size  Used by
> phy_tegra_usb          20480  1
> ci_hdrc_tegra          16384  0
> ci_hdrc                45056  1 ci_hdrc_tegra

I wonder why the driver needs such dependency? If there are two phy
drivers could work with this controller driver, you may request two
modules. Doesn't such dependency should be done by the board
level script? Do you know are there any other drivers do such things?

-- 

Thanks,
Peter Chen

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables
  2019-12-22 21:48     ` Dmitry Osipenko
@ 2019-12-23 14:53       ` Dejin Zheng
  0 siblings, 0 replies; 28+ messages in thread
From: Dejin Zheng @ 2019-12-23 14:53 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Rob Herring, Greg Kroah-Hartman, Peter Chen, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Mon, Dec 23, 2019 at 12:48:09AM +0300, Dmitry Osipenko wrote:
> 22.12.2019 16:24, Dejin Zheng пишет:
> > On Fri, Dec 20, 2019 at 04:52:36AM +0300, Dmitry Osipenko wrote:
> >> There is a mix of u32/ULONG usage in the driver's code. Let's switch to
> >> u32 uniformly, for consistency.
> >>
> >> Suggested-by: Thierry Reding <thierry.reding@gmail.com>
> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >> ---
> >>  drivers/usb/phy/phy-tegra-usb.c | 28 +++++++++++++++-------------
> >>  1 file changed, 15 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/usb/phy/phy-tegra-usb.c b/drivers/usb/phy/phy-tegra-usb.c
> >> index d5739b6e0b6c..551c94e3877a 100644
> >> --- a/drivers/usb/phy/phy-tegra-usb.c
> >> +++ b/drivers/usb/phy/phy-tegra-usb.c
> >> @@ -202,7 +202,7 @@ static inline struct tegra_usb_phy *to_tegra_usb_phy(struct usb_phy *u_phy)
> >>  static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
> >>  {
> >>  	void __iomem *base = phy->regs;
> >> -	unsigned long val;
> >> +	u32 val;
> >>  
> >>  	if (phy->soc_config->has_hostpc) {
> >>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
> >> @@ -221,7 +221,7 @@ static void set_pts(struct tegra_usb_phy *phy, u8 pts_val)
> >>  static void set_phcd(struct tegra_usb_phy *phy, bool enable)
> >>  {
> >>  	void __iomem *base = phy->regs;
> >> -	unsigned long val;
> >> +	u32 val;
> >>  
> >>  	if (phy->soc_config->has_hostpc) {
> >>  		val = readl_relaxed(base + TEGRA_USB_HOSTPC1_DEVLC);
> >> @@ -320,7 +320,8 @@ static int utmip_pad_power_on(struct tegra_usb_phy *phy)
> >>  {
> >>  	struct tegra_utmip_config *config = phy->config;
> >>  	void __iomem *base = phy->pad_regs;
> >> -	unsigned long val, flags;
> >> +	unsigned long flags;
> >> +	u32 val;
> > Why are you still using unsigned long here?
> 
> Please take a look at [1][2], the types are matching callees.
> 
> [1]
> https://elixir.bootlin.com/linux/v5.5-rc2/source/include/linux/spinlock.h#L249
> 
> [2]
> https://elixir.bootlin.com/linux/v5.5-rc2/source/include/asm-generic/io.h#L297

Okay, thanks for your explanation.

Dejin

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-23  6:40       ` Peter Chen
@ 2019-12-23 17:23         ` Dmitry Osipenko
  2019-12-24  2:54           ` Peter Chen
  0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-23 17:23 UTC (permalink / raw)
  To: Peter Chen
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel,
	Peter Geis

23.12.2019 09:40, Peter Chen пишет:
> On 19-12-20 07:31:08, Dmitry Osipenko wrote:
>> 20.12.2019 06:56, Peter Chen пишет:
>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb module
>>>> is loaded too regardless of kernel's configuration. Previously this
>>>> problem was masked because Tegra's EHCI driver is usually enabled in
>>>> kernel's config and thus PHY driver was getting loaded because of it, but
>>>> now I was making some more thorough testing and noticed that PHY's module
>>>> isn't getting auto-loaded without the host driver.
>>>>
>>>> Note that ChipIdea's driver doesn't use any of the exported functions of
>>>> phy_tegra_usb module and thus the module needs to be requested explicitly.
>>>>
>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>> ---
>>>>  drivers/usb/chipidea/Kconfig         | 1 +
>>>>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
>>>>  2 files changed, 7 insertions(+)
>>>>
>>>> diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig
>>>> index ae850b3fddf2..d53db520e209 100644
>>>> --- a/drivers/usb/chipidea/Kconfig
>>>> +++ b/drivers/usb/chipidea/Kconfig
>>>> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
>>>>  	select RESET_CONTROLLER
>>>>  	select USB_ULPI_BUS
>>>>  	select USB_ROLE_SWITCH
>>>> +	select USB_TEGRA_PHY if ARCH_TEGRA
>>>>  	help
>>>>  	  Say Y here if your system has a dual role high speed USB
>>>>  	  controller based on ChipIdea silicon IP. It supports:
>>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>> index 7455df0ede49..8bc11100245d 100644
>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>  	struct tegra_udc *udc;
>>>>  	int err;
>>>>  
>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>> +		err = request_module("phy_tegra_usb");
>>>> +		if (err)
>>>> +			return err;
>>>> +	}
>>>> +
>>>
>>> Why you do this dependency, if this controller driver can't
>>> get USB PHY, it should return error. What's the return value
>>> after calling below:
>>>
>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>
>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>
>> So if you'll do:
>>
>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
>> # modprobe ci_hdrc_tegra
>> # lsmod
>> Module                  Size  Used by
>> ci_hdrc_tegra          16384  0
>> ci_hdrc                45056  1 ci_hdrc_tegra
>>
>> After this patch:
>>
>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb;
>> # modprobe ci_hdrc_tegra
>> # lsmod
>> Module                  Size  Used by
>> Module                  Size  Used by
>> phy_tegra_usb          20480  1
>> ci_hdrc_tegra          16384  0
>> ci_hdrc                45056  1 ci_hdrc_tegra
> 
> I wonder why the driver needs such dependency? If there are two phy
> drivers could work with this controller driver, you may request two
> modules.

Well, if somebody wants to use some PHY driver other than the upstream's
standard one, then that person could simply load the custom driver
module first, such that it will bind to the PHY's device first.

It is also possible to manually unbind the standard driver from PHY's
device and then bind whatever driver you want.

> Doesn't such dependency should be done by the board
> level script?

This patch only improves the default behaviour that is common for all
NVIDIA Tegra boards, it doesn't prevent from doing any special
customizations.

Perhaps the Kconfig change could be dropped from this patch in order to
provide a bit more flexibility in regards to kernel's configuration, but
I'm very doubtful that realistically anyone would want to replace the
default driver with anything else on Tegra. The Kconfig change also puts
ChipIdea's UDC driver in line with the Tegra's EHCI driver that selects
USB_TEGRA_PHY, please see drivers/usb/host/Kconfig.

> Do you know are there any other drivers do such things?

I don't think that any of the USB host drivers are currently doing such
things, but in general there are quite a lot of drivers in kernel that
are using request_module [1].

[1] https://elixir.bootlin.com/linux/latest/ident/request_module

Please note that drivers/usb/host/ehci-tegra.c uses exported symbols
from usb/phy/phy-tegra-usb.c and that is why the EHCI driver doesn't
need to explicitly load the phy_tegra_usb module, the load happens
automatically because of the missing symbols.

Also, please note that it is possible to squash the Tegra EHCI driver
into ci_hdrc_tegra.c and then the explicit dependency on the
phy_tegra_usb won't be needed anymore since it will be replaced with an
implicit dependency. We (me and Peter Geis) already had some
experimental patches that do the successful squashing of the drivers,
but looks like Peter got sidetracked for a more important things for
now, we'll probably return to that work later on.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-20  4:31     ` Dmitry Osipenko
  2019-12-23  6:40       ` Peter Chen
@ 2019-12-23 21:32       ` Michał Mirosław
  2019-12-24  4:21         ` Dmitry Osipenko
  1 sibling, 1 reply; 28+ messages in thread
From: Michał Mirosław @ 2019-12-23 21:32 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
> 20.12.2019 06:56, Peter Chen пишет:
> > On 19-12-20 04:52:38, Dmitry Osipenko wrote:
[...]
> >> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>  	struct tegra_udc *udc;
> >>  	int err;
> >>  
> >> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >> +		err = request_module("phy_tegra_usb");
> >> +		if (err)
> >> +			return err;
> >> +	}
> >> +
> > 
> > Why you do this dependency, if this controller driver can't
> > get USB PHY, it should return error. What's the return value
> > after calling below:
> > 
> > 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
> 
> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.

How are other driver modules autoloaded? Isn't there an appropriate
MODALIAS or MODULE_DEVICE_TABLE in there?

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 28+ messages in thread

* RE: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-23 17:23         ` Dmitry Osipenko
@ 2019-12-24  2:54           ` Peter Chen
  2019-12-24  4:21             ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Peter Chen @ 2019-12-24  2:54 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel,
	Peter Geis

 
> 
> 23.12.2019 09:40, Peter Chen пишет:
> > On 19-12-20 07:31:08, Dmitry Osipenko wrote:
> >> 20.12.2019 06:56, Peter Chen пишет:
> >>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> >>>> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb
> >>>> module is loaded too regardless of kernel's configuration.
> >>>> Previously this problem was masked because Tegra's EHCI driver is
> >>>> usually enabled in kernel's config and thus PHY driver was getting
> >>>> loaded because of it, but now I was making some more thorough
> >>>> testing and noticed that PHY's module isn't getting auto-loaded without the
> host driver.
> >>>>
> >>>> Note that ChipIdea's driver doesn't use any of the exported
> >>>> functions of phy_tegra_usb module and thus the module needs to be
> requested explicitly.
> >>>>
> >>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
> >>>> ---
> >>>>  drivers/usb/chipidea/Kconfig         | 1 +
> >>>>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
> >>>>  2 files changed, 7 insertions(+)
> >>>>
> >>>> diff --git a/drivers/usb/chipidea/Kconfig
> >>>> b/drivers/usb/chipidea/Kconfig index ae850b3fddf2..d53db520e209
> >>>> 100644
> >>>> --- a/drivers/usb/chipidea/Kconfig
> >>>> +++ b/drivers/usb/chipidea/Kconfig
> >>>> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
> >>>>  	select RESET_CONTROLLER
> >>>>  	select USB_ULPI_BUS
> >>>>  	select USB_ROLE_SWITCH
> >>>> +	select USB_TEGRA_PHY if ARCH_TEGRA
> >>>>  	help
> >>>>  	  Say Y here if your system has a dual role high speed USB
> >>>>  	  controller based on ChipIdea silicon IP. It supports:
> >>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> index 7455df0ede49..8bc11100245d 100644
> >>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device
> *pdev)
> >>>>  	struct tegra_udc *udc;
> >>>>  	int err;
> >>>>
> >>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >>>> +		err = request_module("phy_tegra_usb");
> >>>> +		if (err)
> >>>> +			return err;
> >>>> +	}
> >>>> +
> >>>
> >>> Why you do this dependency, if this controller driver can't get USB
> >>> PHY, it should return error. What's the return value after calling
> >>> below:
> >>>
> >>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy",
> >>> 0);
> >>
> >> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> >>
> >> So if you'll do:
> >>
> >> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
> >> ci_hdrc_tegra # lsmod
> >> Module                  Size  Used by
> >> ci_hdrc_tegra          16384  0
> >> ci_hdrc                45056  1 ci_hdrc_tegra
> >>
> >> After this patch:
> >>
> >> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
> >> ci_hdrc_tegra # lsmod
> >> Module                  Size  Used by
> >> Module                  Size  Used by
> >> phy_tegra_usb          20480  1
> >> ci_hdrc_tegra          16384  0
> >> ci_hdrc                45056  1 ci_hdrc_tegra
> >
> > I wonder why the driver needs such dependency? If there are two phy
> > drivers could work with this controller driver, you may request two
> > modules.
> 
> Well, if somebody wants to use some PHY driver other than the upstream's
> standard one, then that person could simply load the custom driver module first,
> such that it will bind to the PHY's device first.
> 
> It is also possible to manually unbind the standard driver from PHY's device and
> then bind whatever driver you want.
> 
> > Doesn't such dependency should be done by the board level script?
> 
> This patch only improves the default behaviour that is common for all NVIDIA Tegra
> boards, it doesn't prevent from doing any special customizations.
> 
> Perhaps the Kconfig change could be dropped from this patch in order to provide a
> bit more flexibility in regards to kernel's configuration, but I'm very doubtful that
> realistically anyone would want to replace the default driver with anything else on
> Tegra. The Kconfig change also puts ChipIdea's UDC driver in line with the Tegra's
> EHCI driver that selects USB_TEGRA_PHY, please see drivers/usb/host/Kconfig.
> 
> > Do you know are there any other drivers do such things?
> 
> I don't think that any of the USB host drivers are currently doing such things, but in
> general there are quite a lot of drivers in kernel that are using request_module [1].
> 
> [1]
> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.c
> om%2Flinux%2Flatest%2Fident%2Frequest_module&amp;data=02%7C01%7Cpete
> r.chen%40nxp.com%7Ca153b9e4d81044cde3c108d787ccdcb9%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C637127186257612269&amp;sdata=xOVnn
> bVGRVhypbiMWpt2MUfcYayAl4ywpCa7xOAQ1vk%3D&amp;reserved=0
> 
> Please note that drivers/usb/host/ehci-tegra.c uses exported symbols from
> usb/phy/phy-tegra-usb.c and that is why the EHCI driver doesn't need to explicitly
> load the phy_tegra_usb module, the load happens automatically because of the
> missing symbols.
> 
> Also, please note that it is possible to squash the Tegra EHCI driver into
> ci_hdrc_tegra.c and then the explicit dependency on the phy_tegra_usb won't be
> needed anymore since it will be replaced with an implicit dependency. We (me and
> Peter Geis) already had some experimental patches that do the successful
> squashing of the drivers, but looks like Peter got sidetracked for a more important
> things for now, we'll probably return to that work later on.

Hi Dmitry,

Thanks for explaining it.  In fact, your case is very common for USB since PHY driver
and controller driver are two independent drivers.  If you have no other ways
to fix this dependency issue, it is ok to add it at driver.

Peter

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-23 21:32       ` Michał Mirosław
@ 2019-12-24  4:21         ` Dmitry Osipenko
  2019-12-30 21:02           ` Michał Mirosław
  0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-24  4:21 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

24.12.2019 00:32, Michał Mirosław пишет:
> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
>> 20.12.2019 06:56, Peter Chen пишет:
>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> [...]
>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>  	struct tegra_udc *udc;
>>>>  	int err;
>>>>  
>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>> +		err = request_module("phy_tegra_usb");
>>>> +		if (err)
>>>> +			return err;
>>>> +	}
>>>> +
>>>
>>> Why you do this dependency, if this controller driver can't
>>> get USB PHY, it should return error. What's the return value
>>> after calling below:
>>>
>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>
>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> 
> How are other driver modules autoloaded? Isn't there an appropriate
> MODALIAS or MODULE_DEVICE_TABLE in there?

Hello Michał,

The phy_tegra_usb module is fine by itself, it's getting autoloaded.

The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
module and thus the PHY module should be loaded before the CI module,
otherwise CI driver fails with the EPROBE_DEFER.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-24  2:54           ` Peter Chen
@ 2019-12-24  4:21             ` Dmitry Osipenko
  0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2019-12-24  4:21 UTC (permalink / raw)
  To: Peter Chen
  Cc: Rob Herring, Greg Kroah-Hartman, Thierry Reding, Jonathan Hunter,
	Felipe Balbi, devicetree, linux-usb, linux-tegra, linux-kernel,
	Peter Geis

24.12.2019 05:54, Peter Chen пишет:
>  
>>
>> 23.12.2019 09:40, Peter Chen пишет:
>>> On 19-12-20 07:31:08, Dmitry Osipenko wrote:
>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>>>> Now, when ci_hdrc_tegra kernel module is loaded, the phy_tegra_usb
>>>>>> module is loaded too regardless of kernel's configuration.
>>>>>> Previously this problem was masked because Tegra's EHCI driver is
>>>>>> usually enabled in kernel's config and thus PHY driver was getting
>>>>>> loaded because of it, but now I was making some more thorough
>>>>>> testing and noticed that PHY's module isn't getting auto-loaded without the
>> host driver.
>>>>>>
>>>>>> Note that ChipIdea's driver doesn't use any of the exported
>>>>>> functions of phy_tegra_usb module and thus the module needs to be
>> requested explicitly.
>>>>>>
>>>>>> Signed-off-by: Dmitry Osipenko <digetx@gmail.com>
>>>>>> ---
>>>>>>  drivers/usb/chipidea/Kconfig         | 1 +
>>>>>>  drivers/usb/chipidea/ci_hdrc_tegra.c | 6 ++++++
>>>>>>  2 files changed, 7 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/usb/chipidea/Kconfig
>>>>>> b/drivers/usb/chipidea/Kconfig index ae850b3fddf2..d53db520e209
>>>>>> 100644
>>>>>> --- a/drivers/usb/chipidea/Kconfig
>>>>>> +++ b/drivers/usb/chipidea/Kconfig
>>>>>> @@ -7,6 +7,7 @@ config USB_CHIPIDEA
>>>>>>  	select RESET_CONTROLLER
>>>>>>  	select USB_ULPI_BUS
>>>>>>  	select USB_ROLE_SWITCH
>>>>>> +	select USB_TEGRA_PHY if ARCH_TEGRA
>>>>>>  	help
>>>>>>  	  Say Y here if your system has a dual role high speed USB
>>>>>>  	  controller based on ChipIdea silicon IP. It supports:
>>>>>> diff --git a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> index 7455df0ede49..8bc11100245d 100644
>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device
>> *pdev)
>>>>>>  	struct tegra_udc *udc;
>>>>>>  	int err;
>>>>>>
>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>> +		err = request_module("phy_tegra_usb");
>>>>>> +		if (err)
>>>>>> +			return err;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> Why you do this dependency, if this controller driver can't get USB
>>>>> PHY, it should return error. What's the return value after calling
>>>>> below:
>>>>>
>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy",
>>>>> 0);
>>>>
>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>>
>>>> So if you'll do:
>>>>
>>>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
>>>> ci_hdrc_tegra # lsmod
>>>> Module                  Size  Used by
>>>> ci_hdrc_tegra          16384  0
>>>> ci_hdrc                45056  1 ci_hdrc_tegra
>>>>
>>>> After this patch:
>>>>
>>>> # rmmod ci_hdrc_tegra; rmmod ci_hdrc; rmmod phy_tegra_usb; # modprobe
>>>> ci_hdrc_tegra # lsmod
>>>> Module                  Size  Used by
>>>> Module                  Size  Used by
>>>> phy_tegra_usb          20480  1
>>>> ci_hdrc_tegra          16384  0
>>>> ci_hdrc                45056  1 ci_hdrc_tegra
>>>
>>> I wonder why the driver needs such dependency? If there are two phy
>>> drivers could work with this controller driver, you may request two
>>> modules.
>>
>> Well, if somebody wants to use some PHY driver other than the upstream's
>> standard one, then that person could simply load the custom driver module first,
>> such that it will bind to the PHY's device first.
>>
>> It is also possible to manually unbind the standard driver from PHY's device and
>> then bind whatever driver you want.
>>
>>> Doesn't such dependency should be done by the board level script?
>>
>> This patch only improves the default behaviour that is common for all NVIDIA Tegra
>> boards, it doesn't prevent from doing any special customizations.
>>
>> Perhaps the Kconfig change could be dropped from this patch in order to provide a
>> bit more flexibility in regards to kernel's configuration, but I'm very doubtful that
>> realistically anyone would want to replace the default driver with anything else on
>> Tegra. The Kconfig change also puts ChipIdea's UDC driver in line with the Tegra's
>> EHCI driver that selects USB_TEGRA_PHY, please see drivers/usb/host/Kconfig.
>>
>>> Do you know are there any other drivers do such things?
>>
>> I don't think that any of the USB host drivers are currently doing such things, but in
>> general there are quite a lot of drivers in kernel that are using request_module [1].
>>
>> [1]
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Felixir.bootlin.c
>> om%2Flinux%2Flatest%2Fident%2Frequest_module&amp;data=02%7C01%7Cpete
>> r.chen%40nxp.com%7Ca153b9e4d81044cde3c108d787ccdcb9%7C686ea1d3bc2b
>> 4c6fa92cd99c5c301635%7C0%7C0%7C637127186257612269&amp;sdata=xOVnn
>> bVGRVhypbiMWpt2MUfcYayAl4ywpCa7xOAQ1vk%3D&amp;reserved=0
>>
>> Please note that drivers/usb/host/ehci-tegra.c uses exported symbols from
>> usb/phy/phy-tegra-usb.c and that is why the EHCI driver doesn't need to explicitly
>> load the phy_tegra_usb module, the load happens automatically because of the
>> missing symbols.
>>
>> Also, please note that it is possible to squash the Tegra EHCI driver into
>> ci_hdrc_tegra.c and then the explicit dependency on the phy_tegra_usb won't be
>> needed anymore since it will be replaced with an implicit dependency. We (me and
>> Peter Geis) already had some experimental patches that do the successful
>> squashing of the drivers, but looks like Peter got sidetracked for a more important
>> things for now, we'll probably return to that work later on.
> 
> Hi Dmitry,
> 
> Thanks for explaining it.  In fact, your case is very common for USB since PHY driver
> and controller driver are two independent drivers.  If you have no other ways
> to fix this dependency issue, it is ok to add it at driver.

Hello Peter,

For now I'm not aware of any alternatives to this patch, thanks.

BTW, I'll make a v3 of this series because found more things that could
be improved in the Tegra's PHY driver.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-24  4:21         ` Dmitry Osipenko
@ 2019-12-30 21:02           ` Michał Mirosław
  2020-01-02 15:17             ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Michał Mirosław @ 2019-12-30 21:02 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
> 24.12.2019 00:32, Michał Mirosław пишет:
> > On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
> >> 20.12.2019 06:56, Peter Chen пишет:
> >>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> > [...]
> >>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>>>  	struct tegra_udc *udc;
> >>>>  	int err;
> >>>>  
> >>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >>>> +		err = request_module("phy_tegra_usb");
> >>>> +		if (err)
> >>>> +			return err;
> >>>> +	}
> >>>> +
> >>>
> >>> Why you do this dependency, if this controller driver can't
> >>> get USB PHY, it should return error. What's the return value
> >>> after calling below:
> >>>
> >>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
> >>
> >> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> > 
> > How are other driver modules autoloaded? Isn't there an appropriate
> > MODALIAS or MODULE_DEVICE_TABLE in there?
> 
> Hello Michał,
> 
> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
> 
> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
> module and thus the PHY module should be loaded before the CI module,
> otherwise CI driver fails with the EPROBE_DEFER.

Why, then, is CI driver not being probed again after PHY driver loads?
EPROBE_DEFER is what should cause driver core to re-probe a device after
other devices appear (PHY in this case).

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2019-12-30 21:02           ` Michał Mirosław
@ 2020-01-02 15:17             ` Dmitry Osipenko
  2020-01-03  7:25               ` Michał Mirosław
  0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2020-01-02 15:17 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

31.12.2019 00:02, Michał Mirosław пишет:
> On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
>> 24.12.2019 00:32, Michał Mirosław пишет:
>>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>> [...]
>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>>>  	struct tegra_udc *udc;
>>>>>>  	int err;
>>>>>>  
>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>> +		err = request_module("phy_tegra_usb");
>>>>>> +		if (err)
>>>>>> +			return err;
>>>>>> +	}
>>>>>> +
>>>>>
>>>>> Why you do this dependency, if this controller driver can't
>>>>> get USB PHY, it should return error. What's the return value
>>>>> after calling below:
>>>>>
>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>>>
>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>
>>> How are other driver modules autoloaded? Isn't there an appropriate
>>> MODALIAS or MODULE_DEVICE_TABLE in there?
>>
>> Hello Michał,
>>
>> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
>>
>> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
>> module and thus the PHY module should be loaded before the CI module,
>> otherwise CI driver fails with the EPROBE_DEFER.
> 
> Why, then, is CI driver not being probed again after PHY driver loads?
> EPROBE_DEFER is what should cause driver core to re-probe a device after
> other devices appear (PHY in this case).

CI driver is getting re-probed just fine if PHY's driver module is
loaded manually after loading the CI's module. This patch removes this
necessity to manually load PHY's module.

This is just a minor convenience change that brings the CI's driver
loading behaviour on par with the behaviour of loading Tegra's EHCI
driver module.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2020-01-02 15:17             ` Dmitry Osipenko
@ 2020-01-03  7:25               ` Michał Mirosław
  2020-01-03 23:19                 ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Michał Mirosław @ 2020-01-03  7:25 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Thu, Jan 02, 2020 at 06:17:47PM +0300, Dmitry Osipenko wrote:
> 31.12.2019 00:02, Michał Mirosław пишет:
> > On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
> >> 24.12.2019 00:32, Michał Mirosław пишет:
> >>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
> >>>> 20.12.2019 06:56, Peter Chen пишет:
> >>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> >>> [...]
> >>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>>>>>  	struct tegra_udc *udc;
> >>>>>>  	int err;
> >>>>>>  
> >>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >>>>>> +		err = request_module("phy_tegra_usb");
> >>>>>> +		if (err)
> >>>>>> +			return err;
> >>>>>> +	}
> >>>>>> +
> >>>>>
> >>>>> Why you do this dependency, if this controller driver can't
> >>>>> get USB PHY, it should return error. What's the return value
> >>>>> after calling below:
> >>>>>
> >>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
> >>>>
> >>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> >>>
> >>> How are other driver modules autoloaded? Isn't there an appropriate
> >>> MODALIAS or MODULE_DEVICE_TABLE in there?
> >>
> >> Hello Michał,
> >>
> >> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
> >>
> >> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
> >> module and thus the PHY module should be loaded before the CI module,
> >> otherwise CI driver fails with the EPROBE_DEFER.
> > 
> > Why, then, is CI driver not being probed again after PHY driver loads?
> > EPROBE_DEFER is what should cause driver core to re-probe a device after
> > other devices appear (PHY in this case).
> 
> CI driver is getting re-probed just fine if PHY's driver module is
> loaded manually after loading the CI's module. This patch removes this
> necessity to manually load PHY's module.
> 
> This is just a minor convenience change that brings the CI's driver
> loading behaviour on par with the behaviour of loading Tegra's EHCI
> driver module.

I fully understand the goal, but what I'm missing is that why this
doesn't work out of the box? If the PHY module is autoloaded, and so is
CI driver, and (as I understand) the driver's probe() correctly returns
EPROBE_DEFER when PHY is not probed yet, then I guess that means bug
somewhere else and the patch just covers it up.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2020-01-03  7:25               ` Michał Mirosław
@ 2020-01-03 23:19                 ` Dmitry Osipenko
  2020-01-04 11:01                   ` Michał Mirosław
  0 siblings, 1 reply; 28+ messages in thread
From: Dmitry Osipenko @ 2020-01-03 23:19 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

03.01.2020 10:25, Michał Mirosław пишет:
> On Thu, Jan 02, 2020 at 06:17:47PM +0300, Dmitry Osipenko wrote:
>> 31.12.2019 00:02, Michał Mirosław пишет:
>>> On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
>>>> 24.12.2019 00:32, Michał Mirosław пишет:
>>>>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
>>>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>>> [...]
>>>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>>>>>  	struct tegra_udc *udc;
>>>>>>>>  	int err;
>>>>>>>>  
>>>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>>>> +		err = request_module("phy_tegra_usb");
>>>>>>>> +		if (err)
>>>>>>>> +			return err;
>>>>>>>> +	}
>>>>>>>> +
>>>>>>>
>>>>>>> Why you do this dependency, if this controller driver can't
>>>>>>> get USB PHY, it should return error. What's the return value
>>>>>>> after calling below:
>>>>>>>
>>>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>>>>>
>>>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>>>
>>>>> How are other driver modules autoloaded? Isn't there an appropriate
>>>>> MODALIAS or MODULE_DEVICE_TABLE in there?
>>>>
>>>> Hello Michał,
>>>>
>>>> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
>>>>
>>>> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
>>>> module and thus the PHY module should be loaded before the CI module,
>>>> otherwise CI driver fails with the EPROBE_DEFER.
>>>
>>> Why, then, is CI driver not being probed again after PHY driver loads?
>>> EPROBE_DEFER is what should cause driver core to re-probe a device after
>>> other devices appear (PHY in this case).
>>
>> CI driver is getting re-probed just fine if PHY's driver module is
>> loaded manually after loading the CI's module. This patch removes this
>> necessity to manually load PHY's module.
>>
>> This is just a minor convenience change that brings the CI's driver
>> loading behaviour on par with the behaviour of loading Tegra's EHCI
>> driver module.
> 
> I fully understand the goal, but what I'm missing is that why this
> doesn't work out of the box? If the PHY module is autoloaded, and so is
> CI driver, and (as I understand) the driver's probe() correctly returns
> EPROBE_DEFER when PHY is not probed yet, then I guess that means bug
> somewhere else and the patch just covers it up.

It works out of the box, but it also could work a bit better in a case
of manually reloading modules. Perhaps it should be possible to derive
module dependencies from the Kconfig dependencies, apparently kernel
doesn't support it yet or maybe there is some reason why it can't be done.

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2020-01-03 23:19                 ` Dmitry Osipenko
@ 2020-01-04 11:01                   ` Michał Mirosław
  2020-01-05  0:42                     ` Dmitry Osipenko
  0 siblings, 1 reply; 28+ messages in thread
From: Michał Mirosław @ 2020-01-04 11:01 UTC (permalink / raw)
  To: Dmitry Osipenko
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

On Sat, Jan 04, 2020 at 02:19:01AM +0300, Dmitry Osipenko wrote:
> 03.01.2020 10:25, Michał Mirosław пишет:
> > On Thu, Jan 02, 2020 at 06:17:47PM +0300, Dmitry Osipenko wrote:
> >> 31.12.2019 00:02, Michał Mirosław пишет:
> >>> On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
> >>>> 24.12.2019 00:32, Michał Mirosław пишет:
> >>>>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
> >>>>>> 20.12.2019 06:56, Peter Chen пишет:
> >>>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
> >>>>> [...]
> >>>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
> >>>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
> >>>>>>>>  	struct tegra_udc *udc;
> >>>>>>>>  	int err;
> >>>>>>>>  
> >>>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
> >>>>>>>> +		err = request_module("phy_tegra_usb");
> >>>>>>>> +		if (err)
> >>>>>>>> +			return err;
> >>>>>>>> +	}
> >>>>>>>> +
> >>>>>>>
> >>>>>>> Why you do this dependency, if this controller driver can't
> >>>>>>> get USB PHY, it should return error. What's the return value
> >>>>>>> after calling below:
> >>>>>>>
> >>>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
> >>>>>>
> >>>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
> >>>>>
> >>>>> How are other driver modules autoloaded? Isn't there an appropriate
> >>>>> MODALIAS or MODULE_DEVICE_TABLE in there?
> >>>>
> >>>> Hello Michał,
> >>>>
> >>>> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
> >>>>
> >>>> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
> >>>> module and thus the PHY module should be loaded before the CI module,
> >>>> otherwise CI driver fails with the EPROBE_DEFER.
> >>>
> >>> Why, then, is CI driver not being probed again after PHY driver loads?
> >>> EPROBE_DEFER is what should cause driver core to re-probe a device after
> >>> other devices appear (PHY in this case).
> >>
> >> CI driver is getting re-probed just fine if PHY's driver module is
> >> loaded manually after loading the CI's module. This patch removes this
> >> necessity to manually load PHY's module.
> >>
> >> This is just a minor convenience change that brings the CI's driver
> >> loading behaviour on par with the behaviour of loading Tegra's EHCI
> >> driver module.
> > 
> > I fully understand the goal, but what I'm missing is that why this
> > doesn't work out of the box? If the PHY module is autoloaded, and so is
> > CI driver, and (as I understand) the driver's probe() correctly returns
> > EPROBE_DEFER when PHY is not probed yet, then I guess that means bug
> > somewhere else and the patch just covers it up.
> 
> It works out of the box, but it also could work a bit better in a case
> of manually reloading modules. Perhaps it should be possible to derive
> module dependencies from the Kconfig dependencies, apparently kernel
> doesn't support it yet or maybe there is some reason why it can't be done.

Kconfig change I'm ok with as it simplifies kernel configuration.
The request_module() is something I would advise against, because if I
do manual module loading, I usually don't want modules loaded
automatically.

Best Regards,
Michał Mirosław

^ permalink raw reply	[flat|nested] 28+ messages in thread

* Re: [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies
  2020-01-04 11:01                   ` Michał Mirosław
@ 2020-01-05  0:42                     ` Dmitry Osipenko
  0 siblings, 0 replies; 28+ messages in thread
From: Dmitry Osipenko @ 2020-01-05  0:42 UTC (permalink / raw)
  To: Michał Mirosław
  Cc: Peter Chen, Rob Herring, Greg Kroah-Hartman, Thierry Reding,
	Jonathan Hunter, Felipe Balbi, devicetree, linux-usb,
	linux-tegra, linux-kernel

04.01.2020 14:01, Michał Mirosław пишет:
> On Sat, Jan 04, 2020 at 02:19:01AM +0300, Dmitry Osipenko wrote:
>> 03.01.2020 10:25, Michał Mirosław пишет:
>>> On Thu, Jan 02, 2020 at 06:17:47PM +0300, Dmitry Osipenko wrote:
>>>> 31.12.2019 00:02, Michał Mirosław пишет:
>>>>> On Tue, Dec 24, 2019 at 07:21:05AM +0300, Dmitry Osipenko wrote:
>>>>>> 24.12.2019 00:32, Michał Mirosław пишет:
>>>>>>> On Fri, Dec 20, 2019 at 07:31:08AM +0300, Dmitry Osipenko wrote:
>>>>>>>> 20.12.2019 06:56, Peter Chen пишет:
>>>>>>>>> On 19-12-20 04:52:38, Dmitry Osipenko wrote:
>>>>>>> [...]
>>>>>>>>>> --- a/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>>>> +++ b/drivers/usb/chipidea/ci_hdrc_tegra.c
>>>>>>>>>> @@ -53,6 +53,12 @@ static int tegra_udc_probe(struct platform_device *pdev)
>>>>>>>>>>  	struct tegra_udc *udc;
>>>>>>>>>>  	int err;
>>>>>>>>>>  
>>>>>>>>>> +	if (IS_MODULE(CONFIG_USB_TEGRA_PHY)) {
>>>>>>>>>> +		err = request_module("phy_tegra_usb");
>>>>>>>>>> +		if (err)
>>>>>>>>>> +			return err;
>>>>>>>>>> +	}
>>>>>>>>>> +
>>>>>>>>>
>>>>>>>>> Why you do this dependency, if this controller driver can't
>>>>>>>>> get USB PHY, it should return error. What's the return value
>>>>>>>>> after calling below:
>>>>>>>>>
>>>>>>>>> 	udc->phy = devm_usb_get_phy_by_phandle(&pdev->dev, "nvidia,phy", 0);
>>>>>>>>
>>>>>>>> It returns -EPROBE_DEFER when phy_tegra_usb isn't loaded.
>>>>>>>
>>>>>>> How are other driver modules autoloaded? Isn't there an appropriate
>>>>>>> MODALIAS or MODULE_DEVICE_TABLE in there?
>>>>>>
>>>>>> Hello Michał,
>>>>>>
>>>>>> The phy_tegra_usb module is fine by itself, it's getting autoloaded.
>>>>>>
>>>>>> The problem is that ci_hdrc_tegra module depends on the phy_tegra_usb
>>>>>> module and thus the PHY module should be loaded before the CI module,
>>>>>> otherwise CI driver fails with the EPROBE_DEFER.
>>>>>
>>>>> Why, then, is CI driver not being probed again after PHY driver loads?
>>>>> EPROBE_DEFER is what should cause driver core to re-probe a device after
>>>>> other devices appear (PHY in this case).
>>>>
>>>> CI driver is getting re-probed just fine if PHY's driver module is
>>>> loaded manually after loading the CI's module. This patch removes this
>>>> necessity to manually load PHY's module.
>>>>
>>>> This is just a minor convenience change that brings the CI's driver
>>>> loading behaviour on par with the behaviour of loading Tegra's EHCI
>>>> driver module.
>>>
>>> I fully understand the goal, but what I'm missing is that why this
>>> doesn't work out of the box? If the PHY module is autoloaded, and so is
>>> CI driver, and (as I understand) the driver's probe() correctly returns
>>> EPROBE_DEFER when PHY is not probed yet, then I guess that means bug
>>> somewhere else and the patch just covers it up.
>>
>> It works out of the box, but it also could work a bit better in a case
>> of manually reloading modules. Perhaps it should be possible to derive
>> module dependencies from the Kconfig dependencies, apparently kernel
>> doesn't support it yet or maybe there is some reason why it can't be done.
> 
> Kconfig change I'm ok with as it simplifies kernel configuration.
> The request_module() is something I would advise against, because if I
> do manual module loading, I usually don't want modules loaded
> automatically.

Okay, I'll drop the request_module since it raises a bit too many
questions and since it's an optional change that won't be needed once
Tegra EHCI driver will be squashed into CI.

^ permalink raw reply	[flat|nested] 28+ messages in thread

end of thread, other threads:[~2020-01-05  0:42 UTC | newest]

Thread overview: 28+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-12-20  1:52 [PATCH v2 00/10] NVIDIA Tegra USB2 drivers clean up Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 01/10] dt-binding: usb: ci-hdrc-usb2: Document NVIDIA Tegra support Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 02/10] usb: phy: tegra: Hook up init/shutdown callbacks Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 03/10] usb: phy: tegra: Perform general clean up of the code Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 04/10] usb: phy: tegra: Use relaxed versions of readl/writel Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 05/10] usb: phy: tegra: Use generic stub for a missing VBUS regulator Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 06/10] usb: ulpi: Add resource-managed variant of otg_ulpi_create() Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 07/10] usb: phy: tegra: Use devm_otg_ulpi_create() Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 08/10] usb: phy: tegra: Use u32 for hardware register variables Dmitry Osipenko
2019-12-22 13:24   ` Dejin Zheng
2019-12-22 21:48     ` Dmitry Osipenko
2019-12-23 14:53       ` Dejin Zheng
2019-12-20  1:52 ` [PATCH v2 09/10] usb: chipidea: tegra: Stop managing PHY's power Dmitry Osipenko
2019-12-20  1:52 ` [PATCH v2 10/10] usb: chipidea: tegra: Add USB_TEGRA_PHY module to driver's dependencies Dmitry Osipenko
2019-12-20  3:56   ` Peter Chen
2019-12-20  4:31     ` Dmitry Osipenko
2019-12-23  6:40       ` Peter Chen
2019-12-23 17:23         ` Dmitry Osipenko
2019-12-24  2:54           ` Peter Chen
2019-12-24  4:21             ` Dmitry Osipenko
2019-12-23 21:32       ` Michał Mirosław
2019-12-24  4:21         ` Dmitry Osipenko
2019-12-30 21:02           ` Michał Mirosław
2020-01-02 15:17             ` Dmitry Osipenko
2020-01-03  7:25               ` Michał Mirosław
2020-01-03 23:19                 ` Dmitry Osipenko
2020-01-04 11:01                   ` Michał Mirosław
2020-01-05  0:42                     ` Dmitry Osipenko

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).