[PATCH 0/4] usb: Supporting patches for registering Tegra USB PHY as a platform driver

[PATCH 0/4] usb: Supporting patches for registering Tegra USB PHY as a platform driver

[Venu Byravarasu]

As part of registering tegra USB PHY as platform driver, prepared
patches to add separate DT nodes describing Tegra USB PHY properties.
Modified instance number based processing to make use of the added
DT properties. As PHY will be registered as separate driver, removed
ehci register access from PHY driver and added APIs to ehci driver for
this purpose.

Venu Byravarasu (4):
  arm: tegra: Add DT nodes for Tegra USB PHY
  USB: PHY: Get rid of instance number to differentiate legacy
    controller
  USB: PHY: Tegra: Get rid of instance number to differentiate PHY type
  usb: Add APIs to access host registers from Tegra PHY

 .../bindings/usb/nvidia,tegra20-ehci.txt           |    1 +
 .../bindings/usb/nvidia,tegra20-usb-phy.txt        |   14 +++
 arch/arm/boot/dts/tegra20.dtsi                     |   22 +++++
 drivers/usb/host/ehci-tegra.c                      |   71 ++++++++++++++-
 drivers/usb/phy/tegra_usb_phy.c                    |  100 +++++++-------------
 include/linux/usb/tegra_usb_phy.h                  |    8 ++
 6 files changed, 149 insertions(+), 67 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt

[PATCH 1/4] arm: tegra: Add DT nodes for Tegra USB PHY

[Venu Byravarasu]

Add DT nodes for Tegra USB PHY along with related documentation.
Also added a phandle property to controller DT node, for referring
to connected PHY instance.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---
 .../bindings/usb/nvidia,tegra20-ehci.txt           |    1 +
 .../bindings/usb/nvidia,tegra20-usb-phy.txt        |   14 ++++++++++++
 arch/arm/boot/dts/tegra20.dtsi                     |   22 ++++++++++++++++++++
 3 files changed, 37 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt

diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
index 6ea765a..34c9528 100644
--- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
@@ -11,6 +11,7 @@ Required properties :
  - phy_type : Should be one of "ulpi" or "utmi".
  - nvidia,vbus-gpio : If present, specifies a gpio that needs to be
    activated for the bus to be powered.
+ - nvidia,phy : phandle of the PHY instance, the controller is connected to.
 
 Required properties for phy_type == ulpi:
   - nvidia,phy-reset-gpio : The GPIO used to reset the PHY.
diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
new file mode 100644
index 0000000..84a4c12
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
@@ -0,0 +1,14 @@
+Tegra SOC USB PHY
+
+The device node for Tegra SOC USB PHY:
+
+Required properties :
+ - compatible : Should be "nvidia,tegra20-usb-phy".
+ - reg : Address and length of the register set for the USB PHY interface.
+ - phy_type : Should be one of "ulpi" or "utmi".
+
+Optional properties:
+  - nvidia,has-legacy-mode : boolean indicates whether this controller can
+    operate in legacy mode (as APX 2500 / 2600). In legacy mode some
+    registers are accessed through the APB_MISC base address instead of
+    the USB controller.
\ No newline at end of file
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index b4c13b6..96251b0 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -394,6 +394,25 @@
 		#size-cells = <0>;
 	};
 
+	phy1: usb-phy@c5000400 {
+		compatible = "nvidia,tegra20-usb-phy";
+		reg = <0xc5000400 0x3c00>;
+		phy_type = "utmi";
+		nvidia,has-legacy-mode;
+	};
+
+	phy2: usb-phy@c5004400 {
+		compatible = "nvidia,tegra20-usb-phy";
+		reg = <0xc5004400 0x3c00>;
+		phy_type = "ulpi";
+	};
+
+	phy3: usb-phy@c5008400 {
+		compatible = "nvidia,tegra20-usb-phy";
+		reg = <0xc5008400 0x3C00>;
+		phy_type = "utmi";
+	};
+
 	usb@c5000000 {
 		compatible = "nvidia,tegra20-ehci", "usb-ehci";
 		reg = <0xc5000000 0x4000>;
@@ -402,6 +421,7 @@
 		nvidia,has-legacy-mode;
 		status = "disabled";
 		nvidia,needs-double-reset;
+		nvidia,phy = <&phy1>;
 	};
 
 	usb@c5004000 {
@@ -410,6 +430,7 @@
 		interrupts = <0 21 0x04>;
 		phy_type = "ulpi";
 		status = "disabled";
+		nvidia,phy = <&phy2>;
 	};
 
 	usb@c5008000 {
@@ -418,6 +439,7 @@
 		interrupts = <0 97 0x04>;
 		phy_type = "utmi";
 		status = "disabled";
+		nvidia,phy = <&phy3>;
 	};
 
 	sdhci@c8000000 {
-- 
1.7.0.4

Re: [PATCH 1/4] arm: tegra: Add DT nodes for Tegra USB PHY

[Stephen Warren]

On 01/16/2013 06:30 AM, Venu Byravarasu wrote:
> Add DT nodes for Tegra USB PHY along with related documentation.
> Also added a phandle property to controller DT node, for referring
> to connected PHY instance.

Just a quick reminder to Greg, Felipe, Alan: Once this series passes
review, I'll need to take it through the Tegra tree. (Sorry if I'm
belabouring that point). This patch in particular will interact with the
common clock framework changes that will very soon be in the Tegra tree.

[PATCH 2/4] USB: PHY: Get rid of instance number to differentiate legacy controller

[Venu Byravarasu]

Tegra20 USB has 3 PHY instances. Instance 0 is based on
legacy PHY interface and other two are standard interfaces.

As instance number was used to differentiate legacy from
standard interfaces, used DT param to get this info and
processed accordingly.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---
 drivers/usb/phy/tegra_usb_phy.c   |   32 +++++++++++++++-----------------
 include/linux/usb/tegra_usb_phy.h |    1 +
 2 files changed, 16 insertions(+), 17 deletions(-)

diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 3059384..79280fe 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -24,6 +24,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/gpio.h>
+#include <linux/of.h>
 #include <linux/of_gpio.h>
 #include <linux/usb/otg.h>
 #include <linux/usb/ulpi.h>
@@ -221,7 +222,7 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
 		return PTR_ERR(phy->pad_clk);
 	}
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		phy->pad_regs = phy->regs;
 	} else {
 		phy->pad_regs = ioremap(TEGRA_USB_BASE, TEGRA_USB_SIZE);
@@ -236,7 +237,7 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
 
 static void utmip_pad_close(struct tegra_usb_phy *phy)
 {
-	if (phy->instance != 0)
+	if (!phy->is_legacy_phy)
 		iounmap(phy->pad_regs);
 	clk_put(phy->pad_clk);
 }
@@ -305,7 +306,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 	unsigned long val;
 	void __iomem *base = phy->regs;
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		val = readl(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
@@ -315,9 +316,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
-	}
-
-	if (phy->instance == 2) {
+	} else {
 		val = readl(base + USB_PORTSC1);
 		val |= USB_PORTSC1_PHCD;
 		writel(val, base + USB_PORTSC1);
@@ -332,7 +331,7 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 	unsigned long val;
 	void __iomem *base = phy->regs;
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		val = readl(base + USB_SUSP_CTRL);
 		val |= USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
@@ -342,9 +341,7 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
-	}
-
-	if (phy->instance == 2) {
+	} else {
 		val = readl(base + USB_PORTSC1);
 		val &= ~USB_PORTSC1_PHCD;
 		writel(val, base + USB_PORTSC1);
@@ -365,7 +362,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 	val |= UTMIP_RESET;
 	writel(val, base + USB_SUSP_CTRL);
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		val = readl(base + USB1_LEGACY_CTRL);
 		val |= USB1_NO_LEGACY_MODE;
 		writel(val, base + USB1_LEGACY_CTRL);
@@ -440,16 +437,14 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 	val |= UTMIP_BIAS_PDTRK_COUNT(0x5);
 	writel(val, base + UTMIP_BIAS_CFG1);
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		val = readl(base + UTMIP_SPARE_CFG0);
 		if (phy->mode == TEGRA_USB_PHY_MODE_DEVICE)
 			val &= ~FUSE_SETUP_SEL;
 		else
 			val |= FUSE_SETUP_SEL;
 		writel(val, base + UTMIP_SPARE_CFG0);
-	}
-
-	if (phy->instance == 2) {
+	} else {
 		val = readl(base + USB_SUSP_CTRL);
 		val |= UTMIP_PHY_ENABLE;
 		writel(val, base + USB_SUSP_CTRL);
@@ -459,7 +454,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 	val &= ~UTMIP_RESET;
 	writel(val, base + USB_SUSP_CTRL);
 
-	if (phy->instance == 0) {
+	if (phy->is_legacy_phy) {
 		val = readl(base + USB1_LEGACY_CTRL);
 		val &= ~USB1_VBUS_SENSE_CTL_MASK;
 		val |= USB1_VBUS_SENSE_CTL_A_SESS_VLD;
@@ -472,7 +467,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 
 	utmi_phy_clk_enable(phy);
 
-	if (phy->instance == 2) {
+	if (!phy->is_legacy_phy) {
 		val = readl(base + USB_PORTSC1);
 		val &= ~USB_PORTSC1_PTS(~0);
 		writel(val, base + USB_PORTSC1);
@@ -739,6 +734,7 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
 	unsigned long parent_rate;
 	int i;
 	int err;
+	struct device_node *np = dev->of_node;
 
 	phy = kzalloc(sizeof(struct tegra_usb_phy), GFP_KERNEL);
 	if (!phy)
@@ -749,6 +745,8 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
 	phy->config = config;
 	phy->mode = phy_mode;
 	phy->dev = dev;
+	phy->is_legacy_phy =
+		of_property_read_bool(np, "nvidia,has-legacy-mode");
 
 	if (!phy->config) {
 		if (phy_is_ulpi(phy)) {
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index 34e6355..f03e157 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -59,6 +59,7 @@ struct tegra_usb_phy {
 	struct usb_phy *ulpi;
 	struct usb_phy u_phy;
 	struct device *dev;
+	bool is_legacy_phy;
 };
 
 struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
-- 
1.7.0.4

Re: [PATCH 2/4] USB: PHY: Get rid of instance number to differentiate legacy controller

[Felipe Balbi]

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

On Wed, Jan 16, 2013 at 07:00:20PM +0530, Venu Byravarasu wrote:
> Tegra20 USB has 3 PHY instances. Instance 0 is based on
> legacy PHY interface and other two are standard interfaces.
> 
> As instance number was used to differentiate legacy from
> standard interfaces, used DT param to get this info and
> processed accordingly.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

> ---
>  drivers/usb/phy/tegra_usb_phy.c   |   32 +++++++++++++++-----------------
>  include/linux/usb/tegra_usb_phy.h |    1 +
>  2 files changed, 16 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> index 3059384..79280fe 100644
> --- a/drivers/usb/phy/tegra_usb_phy.c
> +++ b/drivers/usb/phy/tegra_usb_phy.c
> @@ -24,6 +24,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/gpio.h>
> +#include <linux/of.h>
>  #include <linux/of_gpio.h>
>  #include <linux/usb/otg.h>
>  #include <linux/usb/ulpi.h>
> @@ -221,7 +222,7 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  		return PTR_ERR(phy->pad_clk);
>  	}
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		phy->pad_regs = phy->regs;
>  	} else {
>  		phy->pad_regs = ioremap(TEGRA_USB_BASE, TEGRA_USB_SIZE);
> @@ -236,7 +237,7 @@ static int utmip_pad_open(struct tegra_usb_phy *phy)
>  
>  static void utmip_pad_close(struct tegra_usb_phy *phy)
>  {
> -	if (phy->instance != 0)
> +	if (!phy->is_legacy_phy)
>  		iounmap(phy->pad_regs);
>  	clk_put(phy->pad_clk);
>  }
> @@ -305,7 +306,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  	unsigned long val;
>  	void __iomem *base = phy->regs;
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		val = readl(base + USB_SUSP_CTRL);
>  		val |= USB_SUSP_SET;
>  		writel(val, base + USB_SUSP_CTRL);
> @@ -315,9 +316,7 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
>  		val = readl(base + USB_SUSP_CTRL);
>  		val &= ~USB_SUSP_SET;
>  		writel(val, base + USB_SUSP_CTRL);
> -	}
> -
> -	if (phy->instance == 2) {
> +	} else {
>  		val = readl(base + USB_PORTSC1);
>  		val |= USB_PORTSC1_PHCD;
>  		writel(val, base + USB_PORTSC1);
> @@ -332,7 +331,7 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
>  	unsigned long val;
>  	void __iomem *base = phy->regs;
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		val = readl(base + USB_SUSP_CTRL);
>  		val |= USB_SUSP_CLR;
>  		writel(val, base + USB_SUSP_CTRL);
> @@ -342,9 +341,7 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
>  		val = readl(base + USB_SUSP_CTRL);
>  		val &= ~USB_SUSP_CLR;
>  		writel(val, base + USB_SUSP_CTRL);
> -	}
> -
> -	if (phy->instance == 2) {
> +	} else {
>  		val = readl(base + USB_PORTSC1);
>  		val &= ~USB_PORTSC1_PHCD;
>  		writel(val, base + USB_PORTSC1);
> @@ -365,7 +362,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  	val |= UTMIP_RESET;
>  	writel(val, base + USB_SUSP_CTRL);
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		val = readl(base + USB1_LEGACY_CTRL);
>  		val |= USB1_NO_LEGACY_MODE;
>  		writel(val, base + USB1_LEGACY_CTRL);
> @@ -440,16 +437,14 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  	val |= UTMIP_BIAS_PDTRK_COUNT(0x5);
>  	writel(val, base + UTMIP_BIAS_CFG1);
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		val = readl(base + UTMIP_SPARE_CFG0);
>  		if (phy->mode == TEGRA_USB_PHY_MODE_DEVICE)
>  			val &= ~FUSE_SETUP_SEL;
>  		else
>  			val |= FUSE_SETUP_SEL;
>  		writel(val, base + UTMIP_SPARE_CFG0);
> -	}
> -
> -	if (phy->instance == 2) {
> +	} else {
>  		val = readl(base + USB_SUSP_CTRL);
>  		val |= UTMIP_PHY_ENABLE;
>  		writel(val, base + USB_SUSP_CTRL);
> @@ -459,7 +454,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  	val &= ~UTMIP_RESET;
>  	writel(val, base + USB_SUSP_CTRL);
>  
> -	if (phy->instance == 0) {
> +	if (phy->is_legacy_phy) {
>  		val = readl(base + USB1_LEGACY_CTRL);
>  		val &= ~USB1_VBUS_SENSE_CTL_MASK;
>  		val |= USB1_VBUS_SENSE_CTL_A_SESS_VLD;
> @@ -472,7 +467,7 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
>  
>  	utmi_phy_clk_enable(phy);
>  
> -	if (phy->instance == 2) {
> +	if (!phy->is_legacy_phy) {
>  		val = readl(base + USB_PORTSC1);
>  		val &= ~USB_PORTSC1_PTS(~0);
>  		writel(val, base + USB_PORTSC1);
> @@ -739,6 +734,7 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
>  	unsigned long parent_rate;
>  	int i;
>  	int err;
> +	struct device_node *np = dev->of_node;
>  
>  	phy = kzalloc(sizeof(struct tegra_usb_phy), GFP_KERNEL);
>  	if (!phy)
> @@ -749,6 +745,8 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
>  	phy->config = config;
>  	phy->mode = phy_mode;
>  	phy->dev = dev;
> +	phy->is_legacy_phy =
> +		of_property_read_bool(np, "nvidia,has-legacy-mode");
>  
>  	if (!phy->config) {
>  		if (phy_is_ulpi(phy)) {
> diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
> index 34e6355..f03e157 100644
> --- a/include/linux/usb/tegra_usb_phy.h
> +++ b/include/linux/usb/tegra_usb_phy.h
> @@ -59,6 +59,7 @@ struct tegra_usb_phy {
>  	struct usb_phy *ulpi;
>  	struct usb_phy u_phy;
>  	struct device *dev;
> +	bool is_legacy_phy;
>  };
>  
>  struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
> -- 
> 1.7.0.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 3/4] USB: PHY: Tegra: Get rid of instance number to differentiate PHY type

[Venu Byravarasu]

Tegra20 USB has 3 PHY instances:
Instance 1 and 3 are UTMI. Instance 2 is ULPI.

As instance number was used to differentiate ULPI from UTMI,
used DT param to get this info and processed accordingly.

Signed-off-by: Venu Byravarasu <vbyravarasu@nvidia.com>
---
 drivers/usb/phy/tegra_usb_phy.c   |   23 +++++++++--------------
 include/linux/usb/tegra_usb_phy.h |    1 +
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 79280fe..ce1ff2a 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -209,11 +209,6 @@ static struct tegra_utmip_config utmip_default[] = {
 	},
 };
 
-static inline bool phy_is_ulpi(struct tegra_usb_phy *phy)
-{
-	return (phy->instance == 1);
-}
-
 static int utmip_pad_open(struct tegra_usb_phy *phy)
 {
 	phy->pad_clk = clk_get_sys("utmip-pad", NULL);
@@ -655,7 +650,7 @@ static int	tegra_phy_init(struct usb_phy *x)
 	struct tegra_ulpi_config *ulpi_config;
 	int err;
 
-	if (phy_is_ulpi(phy)) {
+	if (phy->is_ulpi_phy) {
 		ulpi_config = phy->config;
 		phy->clk = clk_get_sys(NULL, ulpi_config->clk);
 		if (IS_ERR(phy->clk)) {
@@ -693,7 +688,7 @@ static void tegra_usb_phy_close(struct usb_phy *x)
 {
 	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
 
-	if (phy_is_ulpi(phy))
+	if (phy->is_ulpi_phy)
 		clk_put(phy->clk);
 	else
 		utmip_pad_close(phy);
@@ -704,7 +699,7 @@ static void tegra_usb_phy_close(struct usb_phy *x)
 
 static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
 {
-	if (phy_is_ulpi(phy))
+	if (phy->is_ulpi_phy)
 		return ulpi_phy_power_on(phy);
 	else
 		return utmi_phy_power_on(phy);
@@ -712,7 +707,7 @@ static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
 
 static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
 {
-	if (phy_is_ulpi(phy))
+	if (phy->is_ulpi_phy)
 		return ulpi_phy_power_off(phy);
 	else
 		return utmi_phy_power_off(phy);
@@ -749,7 +744,7 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
 		of_property_read_bool(np, "nvidia,has-legacy-mode");
 
 	if (!phy->config) {
-		if (phy_is_ulpi(phy)) {
+		if (phy->is_ulpi_phy) {
 			pr_err("%s: ulpi phy configuration missing", __func__);
 			err = -EINVAL;
 			goto err0;
@@ -796,14 +791,14 @@ EXPORT_SYMBOL_GPL(tegra_usb_phy_open);
 
 void tegra_usb_phy_preresume(struct tegra_usb_phy *phy)
 {
-	if (!phy_is_ulpi(phy))
+	if (!phy->is_ulpi_phy)
 		utmi_phy_preresume(phy);
 }
 EXPORT_SYMBOL_GPL(tegra_usb_phy_preresume);
 
 void tegra_usb_phy_postresume(struct tegra_usb_phy *phy)
 {
-	if (!phy_is_ulpi(phy))
+	if (!phy->is_ulpi_phy)
 		utmi_phy_postresume(phy);
 }
 EXPORT_SYMBOL_GPL(tegra_usb_phy_postresume);
@@ -811,14 +806,14 @@ EXPORT_SYMBOL_GPL(tegra_usb_phy_postresume);
 void tegra_ehci_phy_restore_start(struct tegra_usb_phy *phy,
 				 enum tegra_usb_phy_port_speed port_speed)
 {
-	if (!phy_is_ulpi(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 tegra_usb_phy *phy)
 {
-	if (!phy_is_ulpi(phy))
+	if (!phy->is_ulpi_phy)
 		utmi_phy_restore_end(phy);
 }
 EXPORT_SYMBOL_GPL(tegra_ehci_phy_restore_end);
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index f03e157..a6a89d4 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -60,6 +60,7 @@ struct tegra_usb_phy {
 	struct usb_phy u_phy;
 	struct device *dev;
 	bool is_legacy_phy;
+	bool is_ulpi_phy;
 };
 
 struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
-- 
1.7.0.4

Re: [PATCH 3/4] USB: PHY: Tegra: Get rid of instance number to differentiate PHY type

[Stephen Warren]

On 01/16/2013 06:30 AM, Venu Byravarasu wrote:
> Tegra20 USB has 3 PHY instances:
> Instance 1 and 3 are UTMI. Instance 2 is ULPI.
> 
> As instance number was used to differentiate ULPI from UTMI,
> used DT param to get this info and processed accordingly.

> diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
> index f03e157..a6a89d4 100644
> --- a/include/linux/usb/tegra_usb_phy.h
> +++ b/include/linux/usb/tegra_usb_phy.h
> @@ -60,6 +60,7 @@ struct tegra_usb_phy {
>  	struct usb_phy u_phy;
>  	struct device *dev;
>  	bool is_legacy_phy;
> +	bool is_ulpi_phy;
>  };

I see the code that uses this new field, but it doesn't appear to be set
anywhere...

Re: [PATCH 3/4] USB: PHY: Tegra: Get rid of instance number to differentiate PHY type

[Stephen Warren]

On 01/18/2013 10:04 AM, Stephen Warren wrote:
> On 01/16/2013 06:30 AM, Venu Byravarasu wrote:
>> Tegra20 USB has 3 PHY instances:
>> Instance 1 and 3 are UTMI. Instance 2 is ULPI.
>>
>> As instance number was used to differentiate ULPI from UTMI,
>> used DT param to get this info and processed accordingly.
> 
>> diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
>> index f03e157..a6a89d4 100644
>> --- a/include/linux/usb/tegra_usb_phy.h
>> +++ b/include/linux/usb/tegra_usb_phy.h
>> @@ -60,6 +60,7 @@ struct tegra_usb_phy {
>>  	struct usb_phy u_phy;
>>  	struct device *dev;
>>  	bool is_legacy_phy;
>> +	bool is_ulpi_phy;
>>  };
> 
> I see the code that uses this new field, but it doesn't appear to be set
> anywhere...

Oh, I see that code ended up being part of patch 4. Can you please
repost the series with that chunk moved into patch 3 where it belongs?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] USB: PHY: Tegra: Get rid of instance number to differentiate PHY type

[Felipe Balbi]

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

On Wed, Jan 16, 2013 at 07:00:21PM +0530, Venu Byravarasu wrote:
> Tegra20 USB has 3 PHY instances:
> Instance 1 and 3 are UTMI. Instance 2 is ULPI.
> 
> As instance number was used to differentiate ULPI from UTMI,
> used DT param to get this info and processed accordingly.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

Acked-by: Felipe Balbi <balbi-l0cyMroinI0@public.gmane.org>

> ---
>  drivers/usb/phy/tegra_usb_phy.c   |   23 +++++++++--------------
>  include/linux/usb/tegra_usb_phy.h |    1 +
>  2 files changed, 10 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> index 79280fe..ce1ff2a 100644
> --- a/drivers/usb/phy/tegra_usb_phy.c
> +++ b/drivers/usb/phy/tegra_usb_phy.c
> @@ -209,11 +209,6 @@ static struct tegra_utmip_config utmip_default[] = {
>  	},
>  };
>  
> -static inline bool phy_is_ulpi(struct tegra_usb_phy *phy)
> -{
> -	return (phy->instance == 1);
> -}
> -
>  static int utmip_pad_open(struct tegra_usb_phy *phy)
>  {
>  	phy->pad_clk = clk_get_sys("utmip-pad", NULL);
> @@ -655,7 +650,7 @@ static int	tegra_phy_init(struct usb_phy *x)
>  	struct tegra_ulpi_config *ulpi_config;
>  	int err;
>  
> -	if (phy_is_ulpi(phy)) {
> +	if (phy->is_ulpi_phy) {
>  		ulpi_config = phy->config;
>  		phy->clk = clk_get_sys(NULL, ulpi_config->clk);
>  		if (IS_ERR(phy->clk)) {
> @@ -693,7 +688,7 @@ static void tegra_usb_phy_close(struct usb_phy *x)
>  {
>  	struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
>  
> -	if (phy_is_ulpi(phy))
> +	if (phy->is_ulpi_phy)
>  		clk_put(phy->clk);
>  	else
>  		utmip_pad_close(phy);
> @@ -704,7 +699,7 @@ static void tegra_usb_phy_close(struct usb_phy *x)
>  
>  static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
>  {
> -	if (phy_is_ulpi(phy))
> +	if (phy->is_ulpi_phy)
>  		return ulpi_phy_power_on(phy);
>  	else
>  		return utmi_phy_power_on(phy);
> @@ -712,7 +707,7 @@ static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
>  
>  static int tegra_usb_phy_power_off(struct tegra_usb_phy *phy)
>  {
> -	if (phy_is_ulpi(phy))
> +	if (phy->is_ulpi_phy)
>  		return ulpi_phy_power_off(phy);
>  	else
>  		return utmi_phy_power_off(phy);
> @@ -749,7 +744,7 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
>  		of_property_read_bool(np, "nvidia,has-legacy-mode");
>  
>  	if (!phy->config) {
> -		if (phy_is_ulpi(phy)) {
> +		if (phy->is_ulpi_phy) {
>  			pr_err("%s: ulpi phy configuration missing", __func__);
>  			err = -EINVAL;
>  			goto err0;
> @@ -796,14 +791,14 @@ EXPORT_SYMBOL_GPL(tegra_usb_phy_open);
>  
>  void tegra_usb_phy_preresume(struct tegra_usb_phy *phy)
>  {
> -	if (!phy_is_ulpi(phy))
> +	if (!phy->is_ulpi_phy)
>  		utmi_phy_preresume(phy);
>  }
>  EXPORT_SYMBOL_GPL(tegra_usb_phy_preresume);
>  
>  void tegra_usb_phy_postresume(struct tegra_usb_phy *phy)
>  {
> -	if (!phy_is_ulpi(phy))
> +	if (!phy->is_ulpi_phy)
>  		utmi_phy_postresume(phy);
>  }
>  EXPORT_SYMBOL_GPL(tegra_usb_phy_postresume);
> @@ -811,14 +806,14 @@ EXPORT_SYMBOL_GPL(tegra_usb_phy_postresume);
>  void tegra_ehci_phy_restore_start(struct tegra_usb_phy *phy,
>  				 enum tegra_usb_phy_port_speed port_speed)
>  {
> -	if (!phy_is_ulpi(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 tegra_usb_phy *phy)
>  {
> -	if (!phy_is_ulpi(phy))
> +	if (!phy->is_ulpi_phy)
>  		utmi_phy_restore_end(phy);
>  }
>  EXPORT_SYMBOL_GPL(tegra_ehci_phy_restore_end);
> diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
> index f03e157..a6a89d4 100644
> --- a/include/linux/usb/tegra_usb_phy.h
> +++ b/include/linux/usb/tegra_usb_phy.h
> @@ -60,6 +60,7 @@ struct tegra_usb_phy {
>  	struct usb_phy u_phy;
>  	struct device *dev;
>  	bool is_legacy_phy;
> +	bool is_ulpi_phy;
>  };
>  
>  struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
> -- 
> 1.7.0.4
> 

-- 
balbi

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 4/4] usb: Add APIs to access host registers from Tegra PHY

[Venu Byravarasu]

As Tegra PHY driver needs to access one of the Host registers,
added few APIs.

Signed-off-by: Venu Byravarasu <vbyravarasu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/usb/host/ehci-tegra.c     |   71 ++++++++++++++++++++++++++++++++++++-
 drivers/usb/phy/tegra_usb_phy.c   |   51 +++++++--------------------
 include/linux/usb/tegra_usb_phy.h |    6 +++
 3 files changed, 89 insertions(+), 39 deletions(-)

diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 55a9cde..5299b01 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -2,7 +2,7 @@
  * EHCI-compliant USB host controller driver for NVIDIA Tegra SoCs
  *
  * Copyright (C) 2010 Google, Inc.
- * Copyright (C) 2009 NVIDIA Corporation
+ * Copyright (C) 2009 - 2013 NVIDIA Corporation
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms of the GNU General Public License as published by the
@@ -33,6 +33,20 @@
 #define TEGRA_USB2_BASE			0xC5004000
 #define TEGRA_USB3_BASE			0xC5008000
 
+/* PORTSC registers */
+#define USB_PORTSC1			0x184
+#define USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
+#define USB_PORTSC1_PSPD(x)	(((x) & 0x3) << 26)
+#define USB_PORTSC1_PHCD	(1 << 23)
+#define USB_PORTSC1_WKOC	(1 << 22)
+#define USB_PORTSC1_WKDS	(1 << 21)
+#define USB_PORTSC1_WKCN	(1 << 20)
+#define USB_PORTSC1_PTC(x)	(((x) & 0xf) << 16)
+#define USB_PORTSC1_PP		(1 << 12)
+#define USB_PORTSC1_SUSP	(1 << 7)
+#define USB_PORTSC1_PE		(1 << 2)
+#define USB_PORTSC1_CCS		(1 << 0)
+
 #define TEGRA_USB_DMA_ALIGN 32
 
 struct tegra_ehci_hcd {
@@ -605,6 +619,50 @@ static const struct dev_pm_ops tegra_ehci_pm_ops = {
 
 #endif
 
+void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
+
+	val = readl(base + USB_PORTSC1);
+	if (enable)
+		val |= wake;
+	else
+		val &= ~wake;
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_wakeon_events);
+
+void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+
+	val = readl(base + USB_PORTSC1);
+	val &= ~USB_PORTSC1_PTS(3);
+	val |= USB_PORTSC1_PTS(pts_val & 3);
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_pts);
+
+void tegra_ehci_set_phcd(struct usb_phy *x, bool enable)
+{
+	unsigned long val;
+	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
+	void __iomem *base = hcd->regs;
+
+	val = readl(base + USB_PORTSC1);
+	if (enable)
+		val |= USB_PORTSC1_PHCD;
+	else
+		val &= ~USB_PORTSC1_PHCD;
+	writel(val, base + USB_PORTSC1);
+}
+EXPORT_SYMBOL_GPL(tegra_ehci_set_phcd);
+
 static u64 tegra_ehci_dma_mask = DMA_BIT_MASK(32);
 
 static int tegra_ehci_probe(struct platform_device *pdev)
@@ -616,6 +674,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 	int err = 0;
 	int irq;
 	int instance = pdev->id;
+	struct usb_phy *u_phy;
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata) {
@@ -718,6 +777,16 @@ static int tegra_ehci_probe(struct platform_device *pdev)
 
 	usb_phy_init(&tegra->phy->u_phy);
 
+	hcd->phy = u_phy = &tegra->phy->u_phy;
+	u_phy->otg = devm_kzalloc(&pdev->dev, sizeof(struct usb_otg),
+			     GFP_KERNEL);
+	if (!u_phy->otg) {
+		dev_err(&pdev->dev, "Failed to alloc memory for otg\n");
+		err = -ENOMEM;
+		goto fail_io;
+	}
+	u_phy->otg->host = hcd_to_bus(hcd);
+
 	err = usb_phy_set_suspend(&tegra->phy->u_phy, 0);
 	if (err) {
 		dev_err(&pdev->dev, "Failed to power on the phy\n");
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index ce1ff2a..f3b73b3 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -36,19 +36,6 @@
 
 #define ULPI_VIEWPORT		0x170
 
-#define USB_PORTSC1		0x184
-#define   USB_PORTSC1_PTS(x)	(((x) & 0x3) << 30)
-#define   USB_PORTSC1_PSPD(x)	(((x) & 0x3) << 26)
-#define   USB_PORTSC1_PHCD	(1 << 23)
-#define   USB_PORTSC1_WKOC	(1 << 22)
-#define   USB_PORTSC1_WKDS	(1 << 21)
-#define   USB_PORTSC1_WKCN	(1 << 20)
-#define   USB_PORTSC1_PTC(x)	(((x) & 0xf) << 16)
-#define   USB_PORTSC1_PP	(1 << 12)
-#define   USB_PORTSC1_SUSP	(1 << 7)
-#define   USB_PORTSC1_PE	(1 << 2)
-#define   USB_PORTSC1_CCS	(1 << 0)
-
 #define USB_SUSP_CTRL		0x400
 #define   USB_WAKE_ON_CNNT_EN_DEV	(1 << 3)
 #define   USB_WAKE_ON_DISCON_EN_DEV	(1 << 4)
@@ -311,11 +298,8 @@ static void utmi_phy_clk_disable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_SET;
 		writel(val, base + USB_SUSP_CTRL);
-	} else {
-		val = readl(base + USB_PORTSC1);
-		val |= USB_PORTSC1_PHCD;
-		writel(val, base + USB_PORTSC1);
-	}
+	} else
+		tegra_ehci_set_phcd(&phy->u_phy, true);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID, 0) < 0)
 		pr_err("%s: timeout waiting for phy to stabilize\n", __func__);
@@ -336,11 +320,8 @@ static void utmi_phy_clk_enable(struct tegra_usb_phy *phy)
 		val = readl(base + USB_SUSP_CTRL);
 		val &= ~USB_SUSP_CLR;
 		writel(val, base + USB_SUSP_CTRL);
-	} else {
-		val = readl(base + USB_PORTSC1);
-		val &= ~USB_PORTSC1_PHCD;
-		writel(val, base + USB_PORTSC1);
-	}
+	} else
+		tegra_ehci_set_phcd(&phy->u_phy, false);
 
 	if (utmi_wait_register(base + USB_SUSP_CTRL, USB_PHY_CLK_VALID,
 						     USB_PHY_CLK_VALID))
@@ -462,11 +443,8 @@ static int utmi_phy_power_on(struct tegra_usb_phy *phy)
 
 	utmi_phy_clk_enable(phy);
 
-	if (!phy->is_legacy_phy) {
-		val = readl(base + USB_PORTSC1);
-		val &= ~USB_PORTSC1_PTS(~0);
-		writel(val, base + USB_PORTSC1);
-	}
+	if (!phy->is_legacy_phy)
+		tegra_ehci_set_pts(&phy->u_phy, 0);
 
 	return 0;
 }
@@ -611,10 +589,7 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 		return ret;
 	}
 
-	val = readl(base + USB_PORTSC1);
-	val |= USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
-	writel(val, base + USB_PORTSC1);
-
+	tegra_ehci_set_wakeon_events(&phy->u_phy, true);
 	val = readl(base + USB_SUSP_CTRL);
 	val |= USB_SUSP_CLR;
 	writel(val, base + USB_SUSP_CTRL);
@@ -629,17 +604,12 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
 
 static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
 {
-	unsigned long val;
-	void __iomem *base = phy->regs;
 	struct tegra_ulpi_config *config = phy->config;
 
 	/* Clear WKCN/WKDS/WKOC wake-on events that can cause the USB
 	 * Controller to immediately bring the ULPI PHY out of low power
 	 */
-	val = readl(base + USB_PORTSC1);
-	val &= ~(USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN);
-	writel(val, base + USB_PORTSC1);
-
+	tegra_ehci_set_wakeon_events(&phy->u_phy, false);
 	clk_disable(phy->clk);
 	return gpio_direction_output(config->reset_gpio, 0);
 }
@@ -742,6 +712,11 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
 	phy->dev = dev;
 	phy->is_legacy_phy =
 		of_property_read_bool(np, "nvidia,has-legacy-mode");
+	err = of_property_match_string(np, "phy_type", "ulpi");
+	if (err < 0)
+		phy->is_ulpi_phy = false;
+	else
+		phy->is_ulpi_phy = true;
 
 	if (!phy->config) {
 		if (phy->is_ulpi_phy) {
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index a6a89d4..d5f695b 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -75,4 +75,10 @@ void tegra_ehci_phy_restore_start(struct tegra_usb_phy *phy,
 
 void tegra_ehci_phy_restore_end(struct tegra_usb_phy *phy);
 
+void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable);
+
+void tegra_ehci_set_pts(struct usb_phy *x, u8 pts_val);
+
+void tegra_ehci_set_phcd(struct usb_phy *x, bool enable);
+
 #endif /* __TEGRA_USB_PHY_H */
-- 
1.7.0.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] usb: Add APIs to access host registers from Tegra PHY

[Alan Stern]

On Wed, 16 Jan 2013, Venu Byravarasu wrote:

> As Tegra PHY driver needs to access one of the Host registers,
> added few APIs.
> 
> Signed-off-by: Venu Byravarasu <vbyravarasu-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>

> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c

> @@ -605,6 +619,50 @@ static const struct dev_pm_ops tegra_ehci_pm_ops = {
>  
>  #endif
>  
> +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
> +{
> +	unsigned long val;
> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> +	void __iomem *base = hcd->regs;
> +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
> +
> +	val = readl(base + USB_PORTSC1);
> +	if (enable)
> +		val |= wake;
> +	else
> +		val &= ~wake;
> +	writel(val, base + USB_PORTSC1);
> +}

Here and below, this sort of code is highly questionable.  You
evidently don't realize that some of the bits in the PORTSC registers
are R/WC.  This means writing a 1 to these bits will clear them.  

Consequently it is almost always wrong to read a PORTSC register and
then write back the same (or a slightly modified) value.

Alan Stern

Re: [PATCH 4/4] usb: Add APIs to access host registers from Tegra PHY

[Stephen Warren]

On 01/16/2013 08:08 AM, Alan Stern wrote:
> On Wed, 16 Jan 2013, Venu Byravarasu wrote:
> 
>> As Tegra PHY driver needs to access one of the Host registers,
>> added few APIs.

>> --- a/drivers/usb/host/ehci-tegra.c
>> +++ b/drivers/usb/host/ehci-tegra.c

>> +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
>> +{
>> +	unsigned long val;
>> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
>> +	void __iomem *base = hcd->regs;
>> +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
>> +
>> +	val = readl(base + USB_PORTSC1);
>> +	if (enable)
>> +		val |= wake;
>> +	else
>> +		val &= ~wake;
>> +	writel(val, base + USB_PORTSC1);
>> +}
> 
> Here and below, this sort of code is highly questionable.  You
> evidently don't realize that some of the bits in the PORTSC registers
> are R/WC.  This means writing a 1 to these bits will clear them.  
> 
> Consequently it is almost always wrong to read a PORTSC register and
> then write back the same (or a slightly modified) value.

Sorry I'm not familiar with USB... Are the bits being manipulated here
(i.e. USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN)
standardized USBisms, or some custom Tegra stuff?

Anyway, is the solution here to do:

val = readl(addr)

// i.e. add the following line:
val &= ~(all write-to-clear bits);

if (enable) val |= wake; else val &= ~wake;

writel(val, addr)

... or is there more broken than just that?

Also note that the driver is already doing exactly what is in these new
functions; the code is just being split out so that only the EHCI driver
touches EHCI registers, and the PHY driver only touches PHY registers.
Still, I'll admit it's a good time to fix any mistakes in this part of
the code.

Re: [PATCH 4/4] usb: Add APIs to access host registers from Tegra PHY

[Alan Stern]

On Wed, 16 Jan 2013, Stephen Warren wrote:

> On 01/16/2013 08:08 AM, Alan Stern wrote:
> > On Wed, 16 Jan 2013, Venu Byravarasu wrote:
> > 
> >> As Tegra PHY driver needs to access one of the Host registers,
> >> added few APIs.
> 
> >> --- a/drivers/usb/host/ehci-tegra.c
> >> +++ b/drivers/usb/host/ehci-tegra.c
> 
> >> +void tegra_ehci_set_wakeon_events(struct usb_phy *x, bool enable)
> >> +{
> >> +	unsigned long val;
> >> +	struct usb_hcd *hcd = bus_to_hcd(x->otg->host);
> >> +	void __iomem *base = hcd->regs;
> >> +	u32 wake = USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN;
> >> +
> >> +	val = readl(base + USB_PORTSC1);
> >> +	if (enable)
> >> +		val |= wake;
> >> +	else
> >> +		val &= ~wake;
> >> +	writel(val, base + USB_PORTSC1);
> >> +}
> > 
> > Here and below, this sort of code is highly questionable.  You
> > evidently don't realize that some of the bits in the PORTSC registers
> > are R/WC.  This means writing a 1 to these bits will clear them.  
> > 
> > Consequently it is almost always wrong to read a PORTSC register and
> > then write back the same (or a slightly modified) value.
> 
> Sorry I'm not familiar with USB... Are the bits being manipulated here
> (i.e. USB_PORTSC1_WKOC | USB_PORTSC1_WKDS | USB_PORTSC1_WKCN)
> standardized USBisms, or some custom Tegra stuff?

The _symbols_ are not standard (i.e., not used in the regular ehci-hcd
driver) but the _values_ are: compare with PORT_WKOC_E etc. in
include/linux/usb/ehci_defs.h.  I'm not sure about the register 
addresses, however.

> Anyway, is the solution here to do:
> 
> val = readl(addr)
> 
> // i.e. add the following line:
> val &= ~(all write-to-clear bits);
> 
> if (enable) val |= wake; else val &= ~wake;
> 
> writel(val, addr)
> 
> ... or is there more broken than just that?

Nope, that's the right way to do it.  There are loads of examples in 
drivers/usb/host/ehci-hub.c; look at usages of PORT_RWC_BITS.

> Also note that the driver is already doing exactly what is in these new
> functions; the code is just being split out so that only the EHCI driver
> touches EHCI registers, and the PHY driver only touches PHY registers.
> Still, I'll admit it's a good time to fix any mistakes in this part of
> the code.

Agreed.

Alan Stern