As part of this series, apart from patch containing changes to register TEGRA
USB PHY driver as platform driver, prepared below patches:
1. Re-arranging & adding new DT properties.
2. Getting various params from DT properties added.
3. code clean up.
Venu Byravarasu (7):
ARM: tegra: finalize USB EHCI and PHY bindings
ARM: tegra: update device trees for USB binding rework
usb: phy: tegra: Get PHY mode using DT
usb: phy: tegra: Return correct error value provided by clk_get_sys
usb: phy: tegra: get ULPI reset GPIO info using DT.
usb: phy: tegra: Add error handling & clean up.
usb: phy: registering tegra USB PHY as platform driver
.../bindings/usb/nvidia,tegra20-ehci.txt | 27 +--
.../bindings/usb/nvidia,tegra20-usb-phy.txt | 27 ++-
arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +
arch/arm/boot/dts/tegra20-harmony.dts | 8 +-
arch/arm/boot/dts/tegra20-iris-512.dts | 4 +
arch/arm/boot/dts/tegra20-paz00.dts | 8 +-
arch/arm/boot/dts/tegra20-seaboard.dts | 13 +-
arch/arm/boot/dts/tegra20-trimslice.dts | 12 +-
arch/arm/boot/dts/tegra20-ventana.dts | 7 +-
arch/arm/boot/dts/tegra20.dtsi | 32 ++-
drivers/usb/host/ehci-tegra.c | 100 +++----
drivers/usb/phy/tegra_usb_phy.c | 301 ++++++++++++--------
include/linux/usb/tegra_usb_phy.h | 5 +-
13 files changed, 322 insertions(+), 226 deletions(-)
The existing Tegra USB bindings have a few issues:
1) Many properties are documented as being part of the EHCI controller
node, yet they apply more to the PHY device. They should be moved.
2) Some registers in PHY1 are shared with PHY3, and hence PHY3 needs a
reg entry to point at PHY1's register space. We can't assume the PHY1
driver is present, so the PHY3 driver will directly access those
registers.
3) The list of clocks required by the PHY was missing some required
entries.
This patch fixes the binding definition to resolve these issues.
Signed-off-by: Venu Byravarasu <[email protected]>
---
.../bindings/usb/nvidia,tegra20-ehci.txt | 27 +++----------------
.../bindings/usb/nvidia,tegra20-usb-phy.txt | 27 +++++++++++++++++--
2 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
index 34c9528..df09330 100644
--- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
@@ -6,27 +6,10 @@ Practice : Universal Serial Bus" with the following modifications
and additions :
Required properties :
- - compatible : Should be "nvidia,tegra20-ehci" for USB controllers
- used in host mode.
- - 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.
+ - compatible : Should be "nvidia,tegra20-ehci".
+ - nvidia,phy : phandle of the PHY that the controller is connected to.
+ - clocks : Contains a single entry which defines the USB controller's clock.
Optional properties:
- - dr_mode : dual role mode. Indicates the working mode for
- nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral",
- or "otg". Default to "host" if not defined for backward compatibility.
- host means this is a host controller
- peripheral means it is device controller
- otg means it can operate as either ("on the go")
- - 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. Since this is a legacy issue it probably does not
- warrant a compatible string of its own.
- - nvidia,needs-double-reset : boolean is to be set for some of the Tegra2
- USB ports, which need reset twice due to hardware issues.
+ - nvidia,needs-double-reset : boolean is to be set for some of the Tegra20
+ USB ports, which need reset twice due to hardware issues.
diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
index 6bdaba2..7ceccd3 100644
--- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
+++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
@@ -4,8 +4,24 @@ 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".
+ - reg : Defines the following set of registers, in the order listed:
+ - The PHY's own register set.
+ Always present.
+ - The register set of the PHY containing the UTMI pad control registers.
+ Present if-and-only-if phy_type == utmi.
+ - phy_type : Should be one of "utmi", "ulpi" or "hsic".
+ - clocks : Defines the clocks listed in the clock-names property.
+ - clock-names : The following clock names must be present:
+ - reg: The clock needed to access the PHY's own registers. This is the
+ associated EHCI controller's clock. Always present.
+ - pll_u: PLL_U. Always present.
+ - timer: The timeout clock (clk_m). Present if phy_type == utmi.
+ - utmi-pads: The clock needed to access the UTMI pad control registers.
+ Present if phy_type == utmi.
+ - ulpi-link: The clock Tegra provides to the ULPI PHY (cdev2).
+ Present if phy_type == ulpi, and ULPI link mode is in use.
+ - nvidia,vbus-gpio : If present, specifies a GPIO that needs to be
+ activated for the bus to be powered.
Required properties for phy_type == ulpi:
- nvidia,phy-reset-gpio : The GPIO used to reset the PHY.
@@ -14,4 +30,9 @@ 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
+ the USB controller.
+ - dr_mode : dual role mode. Indicates the working mode for the PHY. Can be
+ "host", "gadget", or "otg". Defaults to "host" if not defined.
+ host means this is a host controller
+ gadget means it is device controller
+ otg means it can operate as either ("on the go")
--
1.7.0.4
This patch updates all Tegra board files so that they contain all the
properties required by the updated USB DT binding. Note that this patch
only adds the new properties and does not yet remove the old properties,
in order to maintain bisectability. The old properties will be removed
once the driver has been updated to assume the new bindings.
Signed-off-by: Venu Byravarasu <[email protected]>
---
arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
8 files changed, 57 insertions(+), 31 deletions(-)
diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
index cb73e62..af5a7ae 100644
--- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
+++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
@@ -443,6 +443,10 @@
nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
+ };
+
sdhci@c8000600 {
cd-gpios = <&gpio 23 1>; /* gpio PC7 */
};
diff --git a/arch/arm/boot/dts/tegra20-harmony.dts b/arch/arm/boot/dts/tegra20-harmony.dts
index 1f79c0d..14dd6ed 100644
--- a/arch/arm/boot/dts/tegra20-harmony.dts
+++ b/arch/arm/boot/dts/tegra20-harmony.dts
@@ -427,12 +427,12 @@
nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
- usb@c5008000 {
- status = "okay";
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
- usb-phy@c5004400 {
- nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
+ usb@c5008000 {
+ status = "okay";
};
sdhci@c8000200 {
diff --git a/arch/arm/boot/dts/tegra20-iris-512.dts b/arch/arm/boot/dts/tegra20-iris-512.dts
index 52f1103..c99eccc 100644
--- a/arch/arm/boot/dts/tegra20-iris-512.dts
+++ b/arch/arm/boot/dts/tegra20-iris-512.dts
@@ -41,6 +41,10 @@
dr_mode = "otg";
};
+ usb-phy@c5000000 {
+ dr_mode = "otg";
+ };
+
usb@c5008000 {
status = "okay";
};
diff --git a/arch/arm/boot/dts/tegra20-paz00.dts b/arch/arm/boot/dts/tegra20-paz00.dts
index 9db36da..cc3c032 100644
--- a/arch/arm/boot/dts/tegra20-paz00.dts
+++ b/arch/arm/boot/dts/tegra20-paz00.dts
@@ -426,12 +426,12 @@
nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
};
- usb@c5008000 {
- status = "okay";
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
};
- usb-phy@c5004400 {
- nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
+ usb@c5008000 {
+ status = "okay";
};
sdhci@c8000000 {
diff --git a/arch/arm/boot/dts/tegra20-seaboard.dts b/arch/arm/boot/dts/tegra20-seaboard.dts
index 715a8b8..320964f 100644
--- a/arch/arm/boot/dts/tegra20-seaboard.dts
+++ b/arch/arm/boot/dts/tegra20-seaboard.dts
@@ -563,17 +563,22 @@
dr_mode = "otg";
};
+ usb-phy@c5000000 {
+ nvidia,vbus-gpio = <&gpio 24 0>; /* PD0 */
+ dr_mode = "otg";
+ };
+
usb@c5004000 {
status = "okay";
nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
- usb@c5008000 {
- status = "okay";
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
- usb-phy@c5004400 {
- nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
+ usb@c5008000 {
+ status = "okay";
};
sdhci@c8000000 {
diff --git a/arch/arm/boot/dts/tegra20-trimslice.dts b/arch/arm/boot/dts/tegra20-trimslice.dts
index 98f3e44..3a8d6ed 100644
--- a/arch/arm/boot/dts/tegra20-trimslice.dts
+++ b/arch/arm/boot/dts/tegra20-trimslice.dts
@@ -305,17 +305,21 @@
nvidia,vbus-gpio = <&gpio 170 0>; /* gpio PV2 */
};
+ usb-phy@c5000000 {
+ nvidia,vbus-gpio = <&gpio 170 0>; /* gpio PV2 */
+ };
+
usb@c5004000 {
status = "okay";
nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
};
- usb@c5008000 {
- status = "okay";
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
};
- usb-phy@c5004400 {
- nvidia,phy-reset-gpio = <&gpio 168 0>; /* gpio PV0 */
+ usb@c5008000 {
+ status = "okay";
};
sdhci@c8000000 {
diff --git a/arch/arm/boot/dts/tegra20-ventana.dts b/arch/arm/boot/dts/tegra20-ventana.dts
index 4aef56f..f566fd7 100644
--- a/arch/arm/boot/dts/tegra20-ventana.dts
+++ b/arch/arm/boot/dts/tegra20-ventana.dts
@@ -504,13 +504,14 @@
nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
};
+ usb-phy@c5004000 {
+ nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
+ };
+
usb@c5008000 {
status = "okay";
};
- usb-phy@c5004400 {
- nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
- };
sdhci@c8000000 {
status = "okay";
diff --git a/arch/arm/boot/dts/tegra20.dtsi b/arch/arm/boot/dts/tegra20.dtsi
index 3183581..9eb1085 100644
--- a/arch/arm/boot/dts/tegra20.dtsi
+++ b/arch/arm/boot/dts/tegra20.dtsi
@@ -453,13 +453,16 @@
status = "disabled";
};
- phy1: usb-phy@c5000400 {
+ phy1: usb-phy@c5000000 {
compatible = "nvidia,tegra20-usb-phy";
- reg = <0xc5000400 0x3c00>;
+ reg = <0xc5000000 0x4000 0xc5000000 0x4000>;
phy_type = "utmi";
+ clocks = <&tegra_car 22>,
+ <&tegra_car 127>,
+ <&tegra_car 106>,
+ <&tegra_car 22>;
+ clock-names = "reg", "pll_u", "timer", "utmi-pads";
nvidia,has-legacy-mode;
- clocks = <&tegra_car 22>, <&tegra_car 127>;
- clock-names = "phy", "pll_u";
};
usb@c5004000 {
@@ -472,12 +475,14 @@
status = "disabled";
};
- phy2: usb-phy@c5004400 {
+ phy2: usb-phy@c5004000 {
compatible = "nvidia,tegra20-usb-phy";
- reg = <0xc5004400 0x3c00>;
+ reg = <0xc5004000 0x4000>;
phy_type = "ulpi";
- clocks = <&tegra_car 94>, <&tegra_car 127>;
- clock-names = "phy", "pll_u";
+ clocks = <&tegra_car 58>,
+ <&tegra_car 127>,
+ <&tegra_car 94>;
+ clock-names = "reg", "pll_u", "ulpi-link";
};
usb@c5008000 {
@@ -490,12 +495,15 @@
status = "disabled";
};
- phy3: usb-phy@c5008400 {
+ phy3: usb-phy@c5008000 {
compatible = "nvidia,tegra20-usb-phy";
- reg = <0xc5008400 0x3c00>;
+ reg = <0xc5008000 0x4000 0xc5000000 0x4000>;
phy_type = "utmi";
- clocks = <&tegra_car 22>, <&tegra_car 127>;
- clock-names = "phy", "pll_u";
+ clocks = <&tegra_car 59>,
+ <&tegra_car 127>,
+ <&tegra_car 106>,
+ <&tegra_car 22>;
+ clock-names = "reg", "pll_u", "timer", "utmi-pads";
};
sdhci@c8000000 {
--
1.7.0.4
As GPIO information is avail through DT, used it to get Tegra ULPI
reset GPIO number. Added a new member to tegra_usb_phy structure to
store this number.
Signed-off-by: Venu Byravarasu <[email protected]>
---
drivers/usb/phy/tegra_usb_phy.c | 25 +++++++++++--------------
include/linux/usb/tegra_usb_phy.h | 1 +
2 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index b5b2037..29c5ac4 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -541,11 +541,10 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
int ret;
unsigned long val;
void __iomem *base = phy->regs;
- struct tegra_ulpi_config *config = phy->config;
- gpio_direction_output(config->reset_gpio, 0);
+ gpio_direction_output(phy->reset_gpio, 0);
msleep(5);
- gpio_direction_output(config->reset_gpio, 1);
+ gpio_direction_output(phy->reset_gpio, 1);
clk_prepare_enable(phy->clk);
msleep(1);
@@ -603,10 +602,8 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
{
- struct tegra_ulpi_config *config = phy->config;
-
clk_disable(phy->clk);
- return gpio_direction_output(config->reset_gpio, 0);
+ return gpio_direction_output(phy->reset_gpio, 0);
}
static int tegra_phy_init(struct usb_phy *x)
@@ -622,18 +619,18 @@ static int tegra_phy_init(struct usb_phy *x)
pr_err("%s: can't get ulpi clock\n", __func__);
return PTR_ERR(phy->clk);
}
- if (!gpio_is_valid(ulpi_config->reset_gpio))
- ulpi_config->reset_gpio =
- of_get_named_gpio(phy->dev->of_node,
- "nvidia,phy-reset-gpio", 0);
- if (!gpio_is_valid(ulpi_config->reset_gpio)) {
+
+ phy->reset_gpio =
+ of_get_named_gpio(phy->dev->of_node,
+ "nvidia,phy-reset-gpio", 0);
+ if (!gpio_is_valid(phy->reset_gpio)) {
pr_err("%s: invalid reset gpio: %d\n", __func__,
- ulpi_config->reset_gpio);
+ phy->reset_gpio);
err = -EINVAL;
goto err1;
}
- gpio_request(ulpi_config->reset_gpio, "ulpi_phy_reset_b");
- gpio_direction_output(ulpi_config->reset_gpio, 0);
+ gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
+ gpio_direction_output(phy->reset_gpio, 0);
phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
} else {
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index a7af923..6cfb8f1 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -62,6 +62,7 @@ struct tegra_usb_phy {
struct device *dev;
bool is_legacy_phy;
bool is_ulpi_phy;
+ int reset_gpio;
};
struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
--
1.7.0.4
Added a new PHY mode to support OTG.
Obtained Tegra USB PHY mode using DT property.
Signed-off-by: Venu Byravarasu <[email protected]>
---
drivers/usb/host/ehci-tegra.c | 3 +--
drivers/usb/phy/tegra_usb_phy.c | 13 +++++++++++--
include/linux/usb/tegra_usb_phy.h | 3 ++-
3 files changed, 14 insertions(+), 5 deletions(-)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 568aecc..7afb962 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -732,8 +732,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
}
tegra->phy = tegra_usb_phy_open(&pdev->dev, instance, hcd->regs,
- pdata->phy_config,
- TEGRA_USB_PHY_MODE_HOST);
+ pdata->phy_config);
if (IS_ERR(tegra->phy)) {
dev_err(&pdev->dev, "Failed to open USB phy\n");
err = -ENXIO;
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 5487d38..93abd68 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -688,7 +688,7 @@ static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
}
struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
- void __iomem *regs, void *config, enum tegra_usb_phy_mode phy_mode)
+ void __iomem *regs, void *config)
{
struct tegra_usb_phy *phy;
unsigned long parent_rate;
@@ -703,7 +703,6 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
phy->instance = instance;
phy->regs = regs;
phy->config = config;
- phy->mode = phy_mode;
phy->dev = dev;
phy->is_legacy_phy =
of_property_read_bool(np, "nvidia,has-legacy-mode");
@@ -713,6 +712,16 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
else
phy->is_ulpi_phy = true;
+ err = of_property_match_string(np, "dr_mode", "otg");
+ if (err < 0) {
+ err = of_property_match_string(np, "dr_mode", "gadget");
+ if (err < 0)
+ phy->mode = TEGRA_USB_PHY_MODE_HOST;
+ else
+ phy->mode = TEGRA_USB_PHY_MODE_DEVICE;
+ } else
+ phy->mode = TEGRA_USB_PHY_MODE_OTG;
+
if (!phy->config) {
if (phy->is_ulpi_phy) {
pr_err("%s: ulpi phy configuration missing", __func__);
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index 9ebebe9..a7af923 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -42,6 +42,7 @@ enum tegra_usb_phy_port_speed {
enum tegra_usb_phy_mode {
TEGRA_USB_PHY_MODE_DEVICE,
TEGRA_USB_PHY_MODE_HOST,
+ TEGRA_USB_PHY_MODE_OTG,
};
struct tegra_xtal_freq;
@@ -64,7 +65,7 @@ struct tegra_usb_phy {
};
struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
- void __iomem *regs, void *config, enum tegra_usb_phy_mode phy_mode);
+ void __iomem *regs, void *config);
void tegra_usb_phy_preresume(struct usb_phy *phy);
--
1.7.0.4
Registered tegra USB PHY as a separate platform driver.
To synchronize host controller and PHY initialization, used deferred
probe mechanism. As PHY should be initialized before EHCI starts running,
deferred probe of Tegra EHCI driver till PHY probe gets completed.
Got rid of instance number based handling in host driver.
Made use of DT params to get the PHY Pad registers.
Merged tegra_phy_init into tegra_usb_phy_init.
Signed-off-by: Venu Byravarasu <[email protected]>
---
drivers/usb/host/ehci-tegra.c | 99 ++++++------
drivers/usb/phy/tegra_usb_phy.c | 308 +++++++++++++++++++++----------------
include/linux/usb/tegra_usb_phy.h | 3 +-
3 files changed, 221 insertions(+), 189 deletions(-)
diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
index 7afb962..772fa29 100644
--- a/drivers/usb/host/ehci-tegra.c
+++ b/drivers/usb/host/ehci-tegra.c
@@ -648,7 +648,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
struct tegra_ehci_platform_data *pdata;
int err = 0;
int irq;
- int instance = pdev->id;
+ struct device_node *np_phy;
struct usb_phy *u_phy;
pdata = pdev->dev.platform_data;
@@ -671,34 +671,56 @@ static int tegra_ehci_probe(struct platform_device *pdev)
if (!tegra)
return -ENOMEM;
- hcd = usb_create_hcd(&tegra_ehci_hc_driver, &pdev->dev,
- dev_name(&pdev->dev));
- if (!hcd) {
- dev_err(&pdev->dev, "Unable to create HCD\n");
- return -ENOMEM;
- }
-
platform_set_drvdata(pdev, tegra);
tegra->clk = devm_clk_get(&pdev->dev, NULL);
if (IS_ERR(tegra->clk)) {
dev_err(&pdev->dev, "Can't get ehci clock\n");
- err = PTR_ERR(tegra->clk);
- goto fail_clk;
+ return PTR_ERR(tegra->clk);
}
err = clk_prepare_enable(tegra->clk);
if (err)
- goto fail_clk;
+ return err;
+
+ np_phy = of_parse_phandle(pdev->dev.of_node, "nvidia,phy", 0);
+ if (!np_phy) {
+ err = -ENODEV;
+ goto cleanup_clk;
+ }
+
+ u_phy = tegra_usb_get_phy(np_phy);
+ if (IS_ERR(u_phy)) {
+ err = PTR_ERR(u_phy);
+ goto cleanup_clk;
+ }
tegra->needs_double_reset = of_property_read_bool(pdev->dev.of_node,
"nvidia,needs-double-reset");
+ hcd = usb_create_hcd(&tegra_ehci_hc_driver, &pdev->dev,
+ dev_name(&pdev->dev));
+ if (!hcd) {
+ dev_err(&pdev->dev, "Unable to create HCD\n");
+ err = -ENOMEM;
+ goto cleanup_clk;
+ }
+ hcd->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 = -ENXIO;
+ goto cleanup_hcd_create;
+ }
+
+ u_phy->otg->host = hcd_to_bus(hcd);
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
dev_err(&pdev->dev, "Failed to get I/O memory\n");
err = -ENXIO;
- goto fail_io;
+ goto cleanup_hcd_create;
}
hcd->rsrc_start = res->start;
hcd->rsrc_len = resource_size(res);
@@ -706,55 +728,28 @@ static int tegra_ehci_probe(struct platform_device *pdev)
if (!hcd->regs) {
dev_err(&pdev->dev, "Failed to remap I/O memory\n");
err = -ENOMEM;
- goto fail_io;
- }
-
- /* This is pretty ugly and needs to be fixed when we do only
- * device-tree probing. Old code relies on the platform_device
- * numbering that we lack for device-tree-instantiated devices.
- */
- if (instance < 0) {
- switch (res->start) {
- case TEGRA_USB_BASE:
- instance = 0;
- break;
- case TEGRA_USB2_BASE:
- instance = 1;
- break;
- case TEGRA_USB3_BASE:
- instance = 2;
- break;
- default:
- err = -ENODEV;
- dev_err(&pdev->dev, "unknown usb instance\n");
- goto fail_io;
- }
+ goto cleanup_hcd_create;
}
- tegra->phy = tegra_usb_phy_open(&pdev->dev, instance, hcd->regs,
- pdata->phy_config);
- if (IS_ERR(tegra->phy)) {
- dev_err(&pdev->dev, "Failed to open USB phy\n");
- err = -ENXIO;
- goto fail_io;
+ err = usb_phy_init(hcd->phy);
+ if (err) {
+ dev_err(&pdev->dev, "Failed to initialize phy\n");
+ goto cleanup_hcd_create;
}
- hcd->phy = u_phy = &tegra->phy->u_phy;
- usb_phy_init(hcd->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;
+ goto cleanup_phy;
}
u_phy->otg->host = hcd_to_bus(hcd);
err = usb_phy_set_suspend(hcd->phy, 0);
if (err) {
dev_err(&pdev->dev, "Failed to power on the phy\n");
- goto fail;
+ goto cleanup_phy;
}
tegra->host_resumed = 1;
@@ -764,7 +759,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
if (!irq) {
dev_err(&pdev->dev, "Failed to get IRQ\n");
err = -ENODEV;
- goto fail;
+ goto cleanup_phy;
}
#ifdef CONFIG_USB_OTG_UTILS
@@ -779,7 +774,7 @@ static int tegra_ehci_probe(struct platform_device *pdev)
err = usb_add_hcd(hcd, irq, IRQF_SHARED);
if (err) {
dev_err(&pdev->dev, "Failed to add USB HCD\n");
- goto fail;
+ goto cleanup_phy;
}
pm_runtime_set_active(&pdev->dev);
@@ -792,16 +787,16 @@ static int tegra_ehci_probe(struct platform_device *pdev)
pm_runtime_put_sync(&pdev->dev);
return err;
-fail:
+cleanup_phy:
#ifdef CONFIG_USB_OTG_UTILS
if (!IS_ERR_OR_NULL(tegra->transceiver))
otg_set_host(tegra->transceiver->otg, NULL);
#endif
usb_phy_shutdown(hcd->phy);
-fail_io:
- clk_disable_unprepare(tegra->clk);
-fail_clk:
+cleanup_hcd_create:
usb_put_hcd(hcd);
+cleanup_clk:
+ clk_disable_unprepare(tegra->clk);
return err;
}
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 7ead114..63dfa3e 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -1,9 +1,11 @@
/*
* Copyright (C) 2010 Google, Inc.
+ * Copyright (C) 2011 - 2013 NVIDIA Corporation
*
* Author:
* Erik Gilling <[email protected]>
* Benoit Goby <[email protected]>
+ * Venu Byravarasu <[email protected]>
*
* This software is licensed under the terms of the GNU General Public
* License version 2, as published by the Free Software Foundation, and
@@ -30,9 +32,7 @@
#include <linux/usb/ulpi.h>
#include <asm/mach-types.h>
#include <linux/usb/tegra_usb_phy.h>
-
-#define TEGRA_USB_BASE 0xC5000000
-#define TEGRA_USB_SIZE SZ_16K
+#include <linux/module.h>
#define ULPI_VIEWPORT 0x170
@@ -198,32 +198,15 @@ static struct tegra_utmip_config utmip_default[] = {
static int utmip_pad_open(struct tegra_usb_phy *phy)
{
- phy->pad_clk = clk_get_sys("utmip-pad", NULL);
+ phy->pad_clk = devm_clk_get(phy->dev, "utmi-pads");
if (IS_ERR(phy->pad_clk)) {
pr_err("%s: can't get utmip pad clock\n", __func__);
return PTR_ERR(phy->pad_clk);
}
- if (phy->is_legacy_phy) {
- phy->pad_regs = phy->regs;
- } else {
- phy->pad_regs = ioremap(TEGRA_USB_BASE, TEGRA_USB_SIZE);
- if (!phy->pad_regs) {
- pr_err("%s: can't remap usb registers\n", __func__);
- clk_put(phy->pad_clk);
- return -ENOMEM;
- }
- }
return 0;
}
-static void utmip_pad_close(struct tegra_usb_phy *phy)
-{
- if (!phy->is_legacy_phy)
- iounmap(phy->pad_regs);
- clk_put(phy->pad_clk);
-}
-
static void utmip_pad_power_on(struct tegra_usb_phy *phy)
{
unsigned long val, flags;
@@ -614,76 +597,15 @@ static int ulpi_phy_power_off(struct tegra_usb_phy *phy)
return gpio_direction_output(phy->reset_gpio, 0);
}
-static int tegra_phy_init(struct usb_phy *x)
-{
- struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
- struct tegra_ulpi_config *ulpi_config;
- int err;
-
- if (phy->is_ulpi_phy) {
- ulpi_config = phy->config;
- phy->clk = clk_get_sys(NULL, ulpi_config->clk);
- if (IS_ERR(phy->clk)) {
- pr_err("%s: can't get ulpi clock\n", __func__);
- return PTR_ERR(phy->clk);
- }
-
- phy->reset_gpio =
- of_get_named_gpio(phy->dev->of_node,
- "nvidia,phy-reset-gpio", 0);
- if (!gpio_is_valid(phy->reset_gpio)) {
- dev_err(phy->dev, "invalid gpio: %d\n",
- phy->reset_gpio);
- err = phy->reset_gpio;
- goto cleanup_clk_get;
- }
-
- err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
- if (err < 0) {
- dev_err(phy->dev, "request failed for gpio: %d\n",
- phy->reset_gpio);
- goto cleanup_clk_get;
- }
-
- err = gpio_direction_output(phy->reset_gpio, 0);
- if (err < 0) {
- dev_err(phy->dev, "gpio %d direction not set to output\n",
- phy->reset_gpio);
- goto cleanup_gpio_req;
- }
-
- phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
- if (!phy->ulpi) {
- dev_err(phy->dev, "otg_ulpi_create returned err\n");
- err = -ENOMEM;
- goto cleanup_gpio_req;
- }
-
- phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
- } else {
- err = utmip_pad_open(phy);
- if (err < 0)
- return err;
- }
- return 0;
-cleanup_gpio_req:
- gpio_free(phy->reset_gpio);
-cleanup_clk_get:
- clk_put(phy->clk);
- return err;
-}
-
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)
clk_put(phy->clk);
- else
- utmip_pad_close(phy);
+
clk_disable_unprepare(phy->pll_u);
clk_put(phy->pll_u);
- kfree(phy);
}
static int tegra_usb_phy_power_on(struct tegra_usb_phy *phy)
@@ -711,58 +633,29 @@ static int tegra_usb_phy_suspend(struct usb_phy *x, int suspend)
return tegra_usb_phy_power_on(phy);
}
-struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
- void __iomem *regs, void *config)
+static int tegra_usb_phy_init(struct usb_phy *x)
{
- struct tegra_usb_phy *phy;
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)
- return ERR_PTR(-ENOMEM);
-
- phy->instance = instance;
- phy->regs = regs;
- phy->config = config;
- 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;
+ struct tegra_usb_phy *phy = container_of(x, struct tegra_usb_phy, u_phy);
- err = of_property_match_string(np, "dr_mode", "otg");
- if (err < 0) {
- err = of_property_match_string(np, "dr_mode", "gadget");
- if (err < 0)
- phy->mode = TEGRA_USB_PHY_MODE_HOST;
+ if (!phy->is_ulpi_phy) {
+ if (phy->is_legacy_phy)
+ phy->config = &utmip_default[0];
else
- phy->mode = TEGRA_USB_PHY_MODE_DEVICE;
- } else
- phy->mode = TEGRA_USB_PHY_MODE_OTG;
-
- if (!phy->config) {
- if (phy->is_ulpi_phy) {
- pr_err("%s: ulpi phy configuration missing", __func__);
- err = -EINVAL;
- goto err0;
- } else {
- phy->config = &utmip_default[instance];
- }
+ phy->config = &utmip_default[2];
}
- phy->pll_u = clk_get_sys(NULL, "pll_u");
+ phy->pll_u = devm_clk_get(phy->dev, "pll_u");
if (IS_ERR(phy->pll_u)) {
pr_err("Can't get pll_u clock\n");
- err = PTR_ERR(phy->pll_u);
- goto err0;
+ return PTR_ERR(phy->pll_u);
}
- clk_prepare_enable(phy->pll_u);
+
+ err = clk_prepare_enable(phy->pll_u);
+ if (err)
+ return err;
parent_rate = clk_get_rate(clk_get_parent(phy->pll_u));
for (i = 0; i < ARRAY_SIZE(tegra_freq_table); i++) {
@@ -774,23 +667,53 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
if (!phy->freq) {
pr_err("invalid pll_u parent rate %ld\n", parent_rate);
err = -EINVAL;
- goto err1;
+ goto fail;
}
- phy->u_phy.init = tegra_phy_init;
- phy->u_phy.shutdown = tegra_usb_phy_close;
- phy->u_phy.set_suspend = tegra_usb_phy_suspend;
+ if (phy->is_ulpi_phy) {
+ phy->clk = devm_clk_get(phy->dev, "ulpi-link");
+ if (IS_ERR(phy->clk)) {
+ pr_err("%s: can't get ulpi clock\n", __func__);
+ err = PTR_ERR(phy->clk);
+ goto fail;
+
+ }
+
+ err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
+ if (err < 0) {
+ dev_err(phy->dev, "request failed for gpio: %d\n",
+ phy->reset_gpio);
+ goto fail;
+ }
+
+ err = gpio_direction_output(phy->reset_gpio, 0);
+ if (err < 0) {
+ dev_err(phy->dev, "gpio %d direction not set to output\n",
+ phy->reset_gpio);
+ goto cleanup_gpio_req;
+ }
- return phy;
+ phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
+ if (!phy->ulpi) {
+ dev_err(phy->dev, "otg_ulpi_create returned err\n");
+ err = -ENOMEM;
+ goto cleanup_gpio_req;
+ }
-err1:
+ phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
+ } else {
+ err = utmip_pad_open(phy);
+ if (err < 0)
+ goto fail;
+ }
+ return 0;
+
+cleanup_gpio_req:
+ gpio_free(phy->reset_gpio);
+fail:
clk_disable_unprepare(phy->pll_u);
- clk_put(phy->pll_u);
-err0:
- kfree(phy);
- return ERR_PTR(err);
+ return err;
}
-EXPORT_SYMBOL_GPL(tegra_usb_phy_open);
void tegra_usb_phy_preresume(struct usb_phy *x)
{
@@ -829,3 +752,118 @@ void tegra_ehci_phy_restore_end(struct usb_phy *x)
}
EXPORT_SYMBOL_GPL(tegra_ehci_phy_restore_end);
+static int tegra_usb_phy_probe(struct platform_device *pdev)
+{
+ struct resource *res;
+ struct tegra_usb_phy *tegra_phy = NULL;
+ struct device_node *np = pdev->dev.of_node;
+ int err;
+
+ tegra_phy = devm_kzalloc(&pdev->dev, sizeof(*tegra_phy), GFP_KERNEL);
+ if (!tegra_phy) {
+ dev_err(&pdev->dev, "unable to allocate memory for USB2 PHY\n");
+ return -ENOMEM;
+ }
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get I/O memory\n");
+ return -ENXIO;
+ }
+
+ tegra_phy->regs = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
+ if (!tegra_phy->regs) {
+ dev_err(&pdev->dev, "Failed to remap I/O memory\n");
+ return -ENOMEM;
+ }
+
+ tegra_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) {
+ tegra_phy->is_ulpi_phy = false;
+
+ res = platform_get_resource(pdev, IORESOURCE_MEM, 1);
+ if (!res) {
+ dev_err(&pdev->dev, "Failed to get UTMI Pad regs\n");
+ return -ENXIO;
+ }
+
+ tegra_phy->pad_regs = devm_ioremap(&pdev->dev, res->start,
+ resource_size(res));
+ if (!tegra_phy->regs) {
+ dev_err(&pdev->dev, "Failed to remap UTMI Pad regs\n");
+ return -ENOMEM;
+ }
+ } else {
+ tegra_phy->is_ulpi_phy = true;
+
+ 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;
+ }
+ }
+
+ err = of_property_match_string(np, "dr_mode", "otg");
+ if (err < 0) {
+ err = of_property_match_string(np, "dr_mode", "gadget");
+ if (err < 0)
+ tegra_phy->mode = TEGRA_USB_PHY_MODE_HOST;
+ else
+ tegra_phy->mode = TEGRA_USB_PHY_MODE_DEVICE;
+ } else
+ tegra_phy->mode = TEGRA_USB_PHY_MODE_OTG;
+
+ tegra_phy->dev = &pdev->dev;
+ tegra_phy->u_phy.init = tegra_usb_phy_init;
+ tegra_phy->u_phy.shutdown = tegra_usb_phy_close;
+ tegra_phy->u_phy.set_suspend = tegra_usb_phy_suspend;
+
+ dev_set_drvdata(&pdev->dev, tegra_phy);
+ return 0;
+}
+
+static struct of_device_id tegra_usb_phy_id_table[] = {
+ { .compatible = "nvidia,tegra20-usb-phy", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, tegra_usb_phy_id_table);
+
+static struct platform_driver tegra_usb_phy_driver = {
+ .probe = tegra_usb_phy_probe,
+ .driver = {
+ .name = "tegra-phy",
+ .owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(tegra_usb_phy_id_table),
+ },
+};
+module_platform_driver(tegra_usb_phy_driver);
+
+static int tegra_usb_phy_match(struct device *dev, void *data)
+{
+ struct tegra_usb_phy *tegra_phy = dev_get_drvdata(dev);
+ struct device_node *dn = data;
+
+ return (tegra_phy->dev->of_node == dn) ? 1 : 0;
+}
+
+struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
+{
+ struct device *dev;
+ struct tegra_usb_phy *tegra_phy;
+
+ dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
+ tegra_usb_phy_match);
+ if (!dev)
+ return ERR_PTR(-EPROBE_DEFER);
+
+ tegra_phy = dev_get_drvdata(dev);
+
+ return &tegra_phy->u_phy;
+}
+EXPORT_SYMBOL_GPL(tegra_usb_get_phy);
diff --git a/include/linux/usb/tegra_usb_phy.h b/include/linux/usb/tegra_usb_phy.h
index 6cfb8f1..0cd15d2 100644
--- a/include/linux/usb/tegra_usb_phy.h
+++ b/include/linux/usb/tegra_usb_phy.h
@@ -65,8 +65,7 @@ struct tegra_usb_phy {
int reset_gpio;
};
-struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
- void __iomem *regs, void *config);
+struct usb_phy *tegra_usb_get_phy(struct device_node *dn);
void tegra_usb_phy_preresume(struct usb_phy *phy);
--
1.7.0.4
Check return values from all GPIO APIs and handle errors accordingly.
Remove clk_disable_unprepare which is no more needed.
Signed-off-by: Venu Byravarasu <[email protected]>
---
drivers/usb/phy/tegra_usb_phy.c | 50 ++++++++++++++++++++++++++++++--------
1 files changed, 39 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 29c5ac4..7ead114 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -542,9 +542,17 @@ static int ulpi_phy_power_on(struct tegra_usb_phy *phy)
unsigned long val;
void __iomem *base = phy->regs;
- gpio_direction_output(phy->reset_gpio, 0);
+ ret = gpio_direction_output(phy->reset_gpio, 0);
+ if (ret < 0) {
+ dev_err(phy->dev, "gpio %d direction not set\n", phy->reset_gpio);
+ return ret;
+ }
msleep(5);
- gpio_direction_output(phy->reset_gpio, 1);
+ ret = gpio_direction_output(phy->reset_gpio, 1);
+ if (ret < 0) {
+ dev_err(phy->dev, "gpio %d direction not set\n", phy->reset_gpio);
+ return ret;
+ }
clk_prepare_enable(phy->clk);
msleep(1);
@@ -624,24 +632,44 @@ static int tegra_phy_init(struct usb_phy *x)
of_get_named_gpio(phy->dev->of_node,
"nvidia,phy-reset-gpio", 0);
if (!gpio_is_valid(phy->reset_gpio)) {
- pr_err("%s: invalid reset gpio: %d\n", __func__,
+ dev_err(phy->dev, "invalid gpio: %d\n",
+ phy->reset_gpio);
+ err = phy->reset_gpio;
+ goto cleanup_clk_get;
+ }
+
+ err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
+ if (err < 0) {
+ dev_err(phy->dev, "request failed for gpio: %d\n",
phy->reset_gpio);
- err = -EINVAL;
- goto err1;
+ goto cleanup_clk_get;
+ }
+
+ err = gpio_direction_output(phy->reset_gpio, 0);
+ if (err < 0) {
+ dev_err(phy->dev, "gpio %d direction not set to output\n",
+ phy->reset_gpio);
+ goto cleanup_gpio_req;
}
- gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
- gpio_direction_output(phy->reset_gpio, 0);
+
phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
+ if (!phy->ulpi) {
+ dev_err(phy->dev, "otg_ulpi_create returned err\n");
+ err = -ENOMEM;
+ goto cleanup_gpio_req;
+ }
+
phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
} else {
err = utmip_pad_open(phy);
if (err < 0)
- goto err1;
+ return err;
}
return 0;
-err1:
- clk_disable_unprepare(phy->pll_u);
- clk_put(phy->pll_u);
+cleanup_gpio_req:
+ gpio_free(phy->reset_gpio);
+cleanup_clk_get:
+ clk_put(phy->clk);
return err;
}
--
1.7.0.4
In case if clk_get_sys fails, return correct error value provided by
the API.
Signed-off-by: Venu Byravarasu <[email protected]>
---
drivers/usb/phy/tegra_usb_phy.c | 3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
index 93abd68..b5b2037 100644
--- a/drivers/usb/phy/tegra_usb_phy.c
+++ b/drivers/usb/phy/tegra_usb_phy.c
@@ -620,8 +620,7 @@ static int tegra_phy_init(struct usb_phy *x)
phy->clk = clk_get_sys(NULL, ulpi_config->clk);
if (IS_ERR(phy->clk)) {
pr_err("%s: can't get ulpi clock\n", __func__);
- err = -ENXIO;
- goto err1;
+ return PTR_ERR(phy->clk);
}
if (!gpio_is_valid(ulpi_config->reset_gpio))
ulpi_config->reset_gpio =
--
1.7.0.4
Hello.
On 18-03-2013 16:29, Venu Byravarasu wrote:
> As GPIO information is avail through DT, used it to get Tegra ULPI
> reset GPIO number. Added a new member to tegra_usb_phy structure to
> store this number.
> Signed-off-by: Venu Byravarasu <[email protected]>
> ---
> drivers/usb/phy/tegra_usb_phy.c | 25 +++++++++++--------------
> include/linux/usb/tegra_usb_phy.h | 1 +
> 2 files changed, 12 insertions(+), 14 deletions(-)
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> index b5b2037..29c5ac4 100644
> --- a/drivers/usb/phy/tegra_usb_phy.c
> +++ b/drivers/usb/phy/tegra_usb_phy.c
[...]
> @@ -622,18 +619,18 @@ static int tegra_phy_init(struct usb_phy *x)
[...]
> - gpio_request(ulpi_config->reset_gpio, "ulpi_phy_reset_b");
> - gpio_direction_output(ulpi_config->reset_gpio, 0);
> + gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
> + gpio_direction_output(phy->reset_gpio, 0);
Why not use goio_request_one() instead of these two? Thought maybe it's a
material of another patch...
WBR, Sergei
> -----Original Message-----
> From: [email protected] [mailto:linux-usb-
> [email protected]] On Behalf Of Sergei Shtylyov
> Sent: Monday, March 18, 2013 6:32 PM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; devicetree-
> [email protected]
> Subject: Re: [PATCH 5/7] usb: phy: tegra: get ULPI reset GPIO info using DT.
>
> Hello.
>
> On 18-03-2013 16:29, Venu Byravarasu wrote:
>
> > As GPIO information is avail through DT, used it to get Tegra ULPI
> > reset GPIO number. Added a new member to tegra_usb_phy structure to
> > store this number.
>
> > Signed-off-by: Venu Byravarasu <[email protected]>
> > ---
> > drivers/usb/phy/tegra_usb_phy.c | 25 +++++++++++--------------
> > include/linux/usb/tegra_usb_phy.h | 1 +
> > 2 files changed, 12 insertions(+), 14 deletions(-)
>
> > diff --git a/drivers/usb/phy/tegra_usb_phy.c
> b/drivers/usb/phy/tegra_usb_phy.c
> > index b5b2037..29c5ac4 100644
> > --- a/drivers/usb/phy/tegra_usb_phy.c
> > +++ b/drivers/usb/phy/tegra_usb_phy.c
> [...]
> > @@ -622,18 +619,18 @@ static int tegra_phy_init(struct usb_phy *x)
> [...]
> > - gpio_request(ulpi_config->reset_gpio, "ulpi_phy_reset_b");
> > - gpio_direction_output(ulpi_config->reset_gpio, 0);
> > + gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
> > + gpio_direction_output(phy->reset_gpio, 0);
>
> Why not use goio_request_one() instead of these two? Thought maybe it's
> a
> material of another patch...
Sure, can take this up as part of next patch.
Thanks for the review.
>
> WBR, Sergei
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Check return values from all GPIO APIs and handle errors accordingly.
> Remove clk_disable_unprepare which is no more needed.
Patches 6 and 7 each fail checkpatch with "WARNING: line over 80
characters".
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> As part of this series, apart from patch containing changes to register TEGRA
> USB PHY driver as platform driver, prepared below patches:
> 1. Re-arranging & adding new DT properties.
> 2. Getting various params from DT properties added.
> 3. code clean up.
Venu, I'm curious whether these patches were tested at all. I have found
at least two significant problems with trivial testing:
1)
"reboot" or "shutdown -h now" both cause the following crash, with or
without any USB devices plugged in (or ever having been plugged in):
> [ 355.836288] Unable to handle kernel NULL pointer dereference at virtual address 00000028
> [ 355.847961] pgd = ed620000
> [ 355.854093] [00000028] *pgd=00000000
...
> [ 356.146728] [<c02e5978>] (tegra_ehci_hcd_shutdown+0x18/0x2c) from [<c0279edc>] (platform_drv_shutdown+0x18/0x1c)
> [ 356.160379] [<c0279edc>] (platform_drv_shutdown+0x18/0x1c) from [<c027703c>] (device_shutdown+0x34/0x188)
> [ 356.173464] [<c027703c>] (device_shutdown+0x34/0x188) from [<c0034650>] (kernel_restart_prepare+0x34/0x3c)
> [ 356.186668] [<c0034650>] (kernel_restart_prepare+0x34/0x3c) from [<c0034664>] (kernel_restart+0xc/0x4c)
> [ 356.199637] [<c0034664>] (kernel_restart+0xc/0x4c) from [<c0034858>] (sys_reboot+0x1ac/0x1d8)
> [ 356.211704] [<c0034858>] (sys_reboot+0x1ac/0x1d8) from [<c000e2c0>] (ret_fast_syscall+0x0/0x30)
> [ 356.223965] Code: ebfe4b27 e5903000 e24300e8 e5133044 (e5933028)
> [ 356.233896] ---[ end trace 088d89482b4af176 ]---
2)
The first time enumeration USB devices is attempted on a port fails. For
devices that are plugged in at boot, this means that to get them
working, they must be unplugged and re-plugged after boot. For devices
that are not plugged in at boot, this means they must be plugged,
unplugged, and then plugged in again.
This is obviously problematic in and of itself. This is especially true
for boards like Harmony that have a built-in USB hub and network chip. I
didn't actually test this, but I assume that they cannot be made to work
at all with this patch series, since they cannot be unplugged.
The failed enumeration is accompanied by the following message:
[ 2.451530] hub 3-0:1.0: unable to enumerate USB device on port 1
Both of these problems reproduce on at least boards Ventana and
Seaboard(Springbank), although I assume that all boards are affected.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> This patch updates all Tegra board files so that they contain all the
> properties required by the updated USB DT binding. Note that this patch
> only adds the new properties and does not yet remove the old properties,
> in order to maintain bisectability. The old properties will be removed
> once the driver has been updated to assume the new bindings.
>
> Signed-off-by: Venu Byravarasu <[email protected]>
> ---
> arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
> arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
> arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
> arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
> arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
> arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
> arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
> arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
> 8 files changed, 57 insertions(+), 31 deletions(-)
I think you forgot to update arch/arm/boot/dts/tegra20-whistler.dts in
this patch.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Added a new PHY mode to support OTG.
> Obtained Tegra USB PHY mode using DT property.
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> @@ -713,6 +712,16 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
> else
> phy->is_ulpi_phy = true;
>
> + err = of_property_match_string(np, "dr_mode", "otg");
> + if (err < 0) {
> + err = of_property_match_string(np, "dr_mode", "gadget");
> + if (err < 0)
The binding says the 3 legal values for this property are "host",
"peripheral", or "otg". This agrees with the usage in
Documentation/devicetree/bindings/usb/fsl-usb.txt and
drivers/usb/host/fsl-mph-dr-of.c. So, "gadget" is not something the code
should be checking for.
I'm sure I pointed this out before.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> As GPIO information is avail through DT, used it to get Tegra ULPI
> reset GPIO number. Added a new member to tegra_usb_phy structure to
> store this number.
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> - gpio_direction_output(config->reset_gpio, 0);
> + gpio_direction_output(phy->reset_gpio, 0);
> msleep(5);
> - gpio_direction_output(config->reset_gpio, 1);
> + gpio_direction_output(phy->reset_gpio, 1);
That implies that the PHY reset signal is active-low. This should be
represented in the GPIO flags in the device tree. In other words,
instead of e.g.:
nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
you want:
nvidia,phy-reset-gpio = <&gpio 169 1>; /* gpio PV1 */
Flag 1 means active-low. See
Documentation/devicetree/bindings/gpio/nvidia,tegra20-gpio.txt. This is
a bug in the current device tree content, although it has no effect
since no code currently uses the GPIO flags from DT. I suggest creating
a separate patch to fix this, and inserting it between patch 1 and 2 of
the series.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Check return values from all GPIO APIs and handle errors accordingly.
> Remove clk_disable_unprepare which is no more needed.
The call to clk_disable_unprepare is incorrect in the current code. The
way you worded that, it sounds like it's no longer needed because of the
changes made in this patch. I would re-write that last sentence as:
Remove the call to clk_disable_unprepare(); this function does not
prepare or enable the clock, so the error path should not disable or
unprepare it.
On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> Registered tegra USB PHY as a separate platform driver.
>
> To synchronize host controller and PHY initialization, used deferred
> probe mechanism. As PHY should be initialized before EHCI starts running,
> deferred probe of Tegra EHCI driver till PHY probe gets completed.
>
> Got rid of instance number based handling in host driver.
>
> Made use of DT params to get the PHY Pad registers.
>
> Merged tegra_phy_init into tegra_usb_phy_init.
> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
> 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)
> clk_put(phy->clk);
phy->clk is obtained using devm_clk_get(). This typically means you
never need to clk_put() it, and if for some reason you really have to,
you should use devm_clk_put() instead of plain clk_put().
> clk_put(phy->pll_u);
Same here.
> @@ -774,23 +667,53 @@ struct tegra_usb_phy *tegra_usb_phy_open(struct device *dev, int instance,
> + if (phy->is_ulpi_phy) {
> + phy->clk = devm_clk_get(phy->dev, "ulpi-link");
> + if (IS_ERR(phy->clk)) {
> + pr_err("%s: can't get ulpi clock\n", __func__);
> + err = PTR_ERR(phy->clk);
> + goto fail;
> +
> + }
> +
> + err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
I think you can use devm_gpio_request() here to simplify the error-handling.
> + if (err < 0) {
> + dev_err(phy->dev, "request failed for gpio: %d\n",
> + phy->reset_gpio);
> + goto fail;
> + }
> +
> + err = gpio_direction_output(phy->reset_gpio, 0);
> + if (err < 0) {
> + dev_err(phy->dev, "gpio %d direction not set to output\n",
> + phy->reset_gpio);
> + goto cleanup_gpio_req;
> + }
>
> - return phy;
> + phy->ulpi = otg_ulpi_create(&ulpi_viewport_access_ops, 0);
> + if (!phy->ulpi) {
> + dev_err(phy->dev, "otg_ulpi_create returned err\n");
> + err = -ENOMEM;
> + goto cleanup_gpio_req;
> + }
>
> -err1:
> + phy->ulpi->io_priv = phy->regs + ULPI_VIEWPORT;
> + } else {
> + err = utmip_pad_open(phy);
> + if (err < 0)
> + goto fail;
> + }
I wonder why in the ULPI case, all the code is inline here, whereas in
the UTMI case, this simply calls a function. Wouldn't it be more
consistent to have the following code here:
if (phy->is_ulpi_phy)
err = ulpi_open();
else
err = utmip_open();
if (err)
goto fail;
> +static int tegra_usb_phy_probe(struct platform_device *pdev)
Hmmm. Note that in order to make deferred probe work correctly, all the
gpio_request(), clk_get(), etc. calls that acquire resources from other
drivers must happen here in probe() and not in tegra_usb_phy_open().
> + err = of_property_match_string(np, "dr_mode", "otg");
> + if (err < 0) {
> + err = of_property_match_string(np, "dr_mode", "gadget");
Again, use "peripheral", not "gadget".
> +struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
> +{
> + struct device *dev;
> + struct tegra_usb_phy *tegra_phy;
> +
> + dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
> + tegra_usb_phy_match);
> + if (!dev)
> + return ERR_PTR(-EPROBE_DEFER);
> +
> + tegra_phy = dev_get_drvdata(dev);
> +
> + return &tegra_phy->u_phy;
> +}
I think you need a module_get() somewhere in there, and also need to add
a tegra_usb_put_phy() function too, so you can call module_put() from it.
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 1:22 AM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 0/7] USB: PHY: Tegra: registering TEGRA USB PHY as
> platform driver
>
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > As part of this series, apart from patch containing changes to register
> TEGRA
> > USB PHY driver as platform driver, prepared below patches:
> > 1. Re-arranging & adding new DT properties.
> > 2. Getting various params from DT properties added.
> > 3. code clean up.
>
> Venu, I'm curious whether these patches were tested at all. I have found
> at least two significant problems with trivial testing:
Stephen,
Initially started testing after applying each and every patch.
Like that tested till first 5 patches.
As did not see any issues till then, applied rest 2 patches at once and tested with that.
Though did not see mouse getting vbus on the 1st boot, Vbus was coming fine after disconnect and connect.
Hence did not test thereafter.
After checking your current mail, tried now and observed that there seems to be some real issue with patch#7 only. (As tried now after applying till patch# 6 and did not see this issue).
Will debug further on patch#7 and update with proper fix after addressing your other comments.
Thanks for the review & heads up,
venu
>
> 1)
>
> "reboot" or "shutdown -h now" both cause the following crash, with or
> without any USB devices plugged in (or ever having been plugged in):
>
> > [ 355.836288] Unable to handle kernel NULL pointer dereference at virtual
> address 00000028
> > [ 355.847961] pgd = ed620000
> > [ 355.854093] [00000028] *pgd=00000000
> ...
> > [ 356.146728] [<c02e5978>] (tegra_ehci_hcd_shutdown+0x18/0x2c) from
> [<c0279edc>] (platform_drv_shutdown+0x18/0x1c)
> > [ 356.160379] [<c0279edc>] (platform_drv_shutdown+0x18/0x1c) from
> [<c027703c>] (device_shutdown+0x34/0x188)
> > [ 356.173464] [<c027703c>] (device_shutdown+0x34/0x188) from
> [<c0034650>] (kernel_restart_prepare+0x34/0x3c)
> > [ 356.186668] [<c0034650>] (kernel_restart_prepare+0x34/0x3c) from
> [<c0034664>] (kernel_restart+0xc/0x4c)
> > [ 356.199637] [<c0034664>] (kernel_restart+0xc/0x4c) from [<c0034858>]
> (sys_reboot+0x1ac/0x1d8)
> > [ 356.211704] [<c0034858>] (sys_reboot+0x1ac/0x1d8) from [<c000e2c0>]
> (ret_fast_syscall+0x0/0x30)
> > [ 356.223965] Code: ebfe4b27 e5903000 e24300e8 e5133044 (e5933028)
> > [ 356.233896] ---[ end trace 088d89482b4af176 ]---
>
> 2)
>
> The first time enumeration USB devices is attempted on a port fails. For
> devices that are plugged in at boot, this means that to get them
> working, they must be unplugged and re-plugged after boot. For devices
> that are not plugged in at boot, this means they must be plugged,
> unplugged, and then plugged in again.
>
> This is obviously problematic in and of itself. This is especially true
> for boards like Harmony that have a built-in USB hub and network chip. I
> didn't actually test this, but I assume that they cannot be made to work
> at all with this patch series, since they cannot be unplugged.
>
> The failed enumeration is accompanied by the following message:
>
> [ 2.451530] hub 3-0:1.0: unable to enumerate USB device on port 1
>
> Both of these problems reproduce on at least boards Ventana and
> Seaboard(Springbank), although I assume that all boards are affected.
Hi,
On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
> The existing Tegra USB bindings have a few issues:
>
> 1) Many properties are documented as being part of the EHCI controller
> node, yet they apply more to the PHY device. They should be moved.
>
> 2) Some registers in PHY1 are shared with PHY3, and hence PHY3 needs a
> reg entry to point at PHY1's register space. We can't assume the PHY1
> driver is present, so the PHY3 driver will directly access those
> registers.
>
> 3) The list of clocks required by the PHY was missing some required
> entries.
>
> This patch fixes the binding definition to resolve these issues.
>
> Signed-off-by: Venu Byravarasu <[email protected]>
> ---
> .../bindings/usb/nvidia,tegra20-ehci.txt | 27 +++----------------
> .../bindings/usb/nvidia,tegra20-usb-phy.txt | 27 +++++++++++++++++--
> 2 files changed, 29 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> index 34c9528..df09330 100644
> --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> @@ -6,27 +6,10 @@ Practice : Universal Serial Bus" with the following modifications
> and additions :
>
> Required properties :
> - - compatible : Should be "nvidia,tegra20-ehci" for USB controllers
> - used in host mode.
> - - 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.
> + - compatible : Should be "nvidia,tegra20-ehci".
> + - nvidia,phy : phandle of the PHY that the controller is connected to.
> + - clocks : Contains a single entry which defines the USB controller's clock.
>
> Optional properties:
> - - dr_mode : dual role mode. Indicates the working mode for
> - nvidia,tegra20-ehci compatible controllers. Can be "host", "peripheral",
> - or "otg". Default to "host" if not defined for backward compatibility.
> - host means this is a host controller
> - peripheral means it is device controller
> - otg means it can operate as either ("on the go")
> - - 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. Since this is a legacy issue it probably does not
> - warrant a compatible string of its own.
> - - nvidia,needs-double-reset : boolean is to be set for some of the Tegra2
> - USB ports, which need reset twice due to hardware issues.
> + - nvidia,needs-double-reset : boolean is to be set for some of the Tegra20
> + USB ports, which need reset twice due to hardware issues.
> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
> index 6bdaba2..7ceccd3 100644
> --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
> @@ -4,8 +4,24 @@ 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".
> + - reg : Defines the following set of registers, in the order listed:
> + - The PHY's own register set.
> + Always present.
> + - The register set of the PHY containing the UTMI pad control registers.
> + Present if-and-only-if phy_type == utmi.
> + - phy_type : Should be one of "utmi", "ulpi" or "hsic".
dt property names generally dont have "_".
Thanks
Kishon
Hi,
On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
> This patch updates all Tegra board files so that they contain all the
> properties required by the updated USB DT binding. Note that this patch
> only adds the new properties and does not yet remove the old properties,
> in order to maintain bisectability. The old properties will be removed
> once the driver has been updated to assume the new bindings.
>
> Signed-off-by: Venu Byravarasu <[email protected]>
> ---
> arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
> arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
> arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
> arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
> arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
> arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
> arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
> arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
> 8 files changed, 57 insertions(+), 31 deletions(-)
>
> diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> index cb73e62..af5a7ae 100644
> --- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> +++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> @@ -443,6 +443,10 @@
> nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
> };
>
> + usb-phy@c5004000 {
This node doesn't have a *reg* property. So "@c5004000" is not needed.
This comment applies to all the nodes which doesn't have *reg* property.
Thanks
Kishon
> -----Original Message-----
> From: Venu Byravarasu
> Sent: Wednesday, March 20, 2013 11:30 AM
> To: 'Stephen Warren'
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: RE: [PATCH 0/7] USB: PHY: Tegra: registering TEGRA USB PHY as
> platform driver
>
> > -----Original Message-----
> > From: Stephen Warren [mailto:[email protected]]
> > Sent: Wednesday, March 20, 2013 1:22 AM
> > To: Venu Byravarasu
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]
> > Subject: Re: [PATCH 0/7] USB: PHY: Tegra: registering TEGRA USB PHY as
> > platform driver
> >
> > On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > > As part of this series, apart from patch containing changes to register
> > TEGRA
> > > USB PHY driver as platform driver, prepared below patches:
> > > 1. Re-arranging & adding new DT properties.
> > > 2. Getting various params from DT properties added.
> > > 3. code clean up.
> >
> > Venu, I'm curious whether these patches were tested at all. I have found
> > at least two significant problems with trivial testing:
>
> Stephen,
> Initially started testing after applying each and every patch.
> Like that tested till first 5 patches.
> As did not see any issues till then, applied rest 2 patches at once and tested
> with that.
> Though did not see mouse getting vbus on the 1st boot, Vbus was coming
> fine after disconnect and connect.
> Hence did not test thereafter.
>
> After checking your current mail, tried now and observed that there seems to
> be some real issue with patch#7 only. (As tried now after applying till patch#
> 6 and did not see this issue).
> Will debug further on patch#7 and update with proper fix after addressing
> your other comments.
Debugged further and found that the issue is because of http://marc.info/?l=linux-arm-kernel&m=135890098024987&w=2
On reverting that patch and applying it on top of patch#7, able to see enumeration working fine.
Anyhow, will take care of your other comments and merge this change with patch#7 and resend
for review.
>
> Thanks for the review & heads up,
> venu
>
> -----Original Message-----
> From: kishon [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 4:49 PM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; devicetree-
> [email protected]
> Subject: Re: [PATCH 1/7] ARM: tegra: finalize USB EHCI and PHY bindings
>
> Hi,
>
> On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
> > The existing Tegra USB bindings have a few issues:
> >
> > 1) Many properties are documented as being part of the EHCI controller
> > node, yet they apply more to the PHY device. They should be moved.
> >
> > 2) Some registers in PHY1 are shared with PHY3, and hence PHY3 needs a
> > reg entry to point at PHY1's register space. We can't assume the PHY1
> > driver is present, so the PHY3 driver will directly access those
> > registers.
> >
> > 3) The list of clocks required by the PHY was missing some required
> > entries.
> >
> > This patch fixes the binding definition to resolve these issues.
> >
> > Signed-off-by: Venu Byravarasu <[email protected]>
> > ---
> > .../bindings/usb/nvidia,tegra20-ehci.txt | 27 +++----------------
> > .../bindings/usb/nvidia,tegra20-usb-phy.txt | 27
> +++++++++++++++++--
> > 2 files changed, 29 insertions(+), 25 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> > index 34c9528..df09330 100644
> > --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> > +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
> > @@ -6,27 +6,10 @@ Practice : Universal Serial Bus" with the following
> modifications
> > and additions :
> >
> > Required properties :
> > - - compatible : Should be "nvidia,tegra20-ehci" for USB controllers
> > - used in host mode.
> > - - phy_type : Should be one of "ulpi" or "utmi".
> >
> > Optional properties:
> > - - dr_mode : dual role mode. Indicates the working mode for
> > index 6bdaba2..7ceccd3 100644
> > --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
> > +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
> > @@ -4,8 +4,24 @@ The device node for Tegra SOC USB PHY:
> >
> > Required properties :
> > + - phy_type : Should be one of "utmi", "ulpi" or "hsic".
>
> dt property names generally dont have "_".
Thanks Kishon, for your comments.
Is it mandatory or optional?
If it is mandatory, then I might have to change dr_mode as well along with phy_type.
>
> Thanks
> Kishon
> -----Original Message-----
> From: kishon [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 4:53 PM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; devicetree-
> [email protected]
> Subject: Re: [PATCH 2/7] ARM: tegra: update device trees for USB binding
> rework
>
> Hi,
>
> On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
> > This patch updates all Tegra board files so that they contain all the
> > properties required by the updated USB DT binding. Note that this patch
> > only adds the new properties and does not yet remove the old properties,
> > in order to maintain bisectability. The old properties will be removed
> > once the driver has been updated to assume the new bindings.
> >
> > Signed-off-by: Venu Byravarasu <[email protected]>
> > ---
> > arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
> > arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
> > arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
> > arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
> > arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
> > arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
> > arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
> > arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
> > 8 files changed, 57 insertions(+), 31 deletions(-)
> >
> > diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > index cb73e62..af5a7ae 100644
> > --- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > +++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > @@ -443,6 +443,10 @@
> > nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
> > };
> >
> > + usb-phy@c5004000 {
> This node doesn't have a *reg* property. So "@c5004000" is not needed.
> This comment applies to all the nodes which doesn't have *reg* property.
Thanks Kishon for the comments.
As I've 3 usb-phy DT nodes, how to differentiate between them if I remove this @Address ?
>
> Thanks
> Kishon
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 1:24 AM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 2/7] ARM: tegra: update device trees for USB binding
> rework
>
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > This patch updates all Tegra board files so that they contain all the
> > properties required by the updated USB DT binding. Note that this patch
> > only adds the new properties and does not yet remove the old properties,
> > in order to maintain bisectability. The old properties will be removed
> > once the driver has been updated to assume the new bindings.
> >
> > Signed-off-by: Venu Byravarasu <[email protected]>
> > ---
> > arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
> > arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
> > arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
> > arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
> > arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
> > arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
> > arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
> > arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
> > 8 files changed, 57 insertions(+), 31 deletions(-)
>
> I think you forgot to update arch/arm/boot/dts/tegra20-whistler.dts in
> this patch.
Thanks Stephen, will add that & send updated patch for review.
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 1:29 AM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 3/7] usb: phy: tegra: Get PHY mode using DT
>
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > Added a new PHY mode to support OTG.
> > Obtained Tegra USB PHY mode using DT property.
>
> > diff --git a/drivers/usb/phy/tegra_usb_phy.c
> b/drivers/usb/phy/tegra_usb_phy.c
>
> > @@ -713,6 +712,16 @@ struct tegra_usb_phy
> *tegra_usb_phy_open(struct device *dev, int instance,
> > else
> > phy->is_ulpi_phy = true;
> >
> > + err = of_property_match_string(np, "dr_mode", "otg");
> > + if (err < 0) {
> > + err = of_property_match_string(np, "dr_mode", "gadget");
> > + if (err < 0)
>
> The binding says the 3 legal values for this property are "host",
> "peripheral", or "otg". This agrees with the usage in
> Documentation/devicetree/bindings/usb/fsl-usb.txt and
> drivers/usb/host/fsl-mph-dr-of.c. So, "gadget" is not something the code
> should be checking for.
Agree, will correct it.
>
> I'm sure I pointed this out before.
On Wed, Mar 20, 2013 at 05:47:46PM +0530, Venu Byravarasu wrote:
> > -----Original Message-----
> > From: kishon [mailto:[email protected]]
> > Sent: Wednesday, March 20, 2013 4:53 PM
> > To: Venu Byravarasu
> > Cc: [email protected]; [email protected];
> > [email protected]; [email protected]; [email protected];
> > [email protected]; [email protected]; devicetree-
> > [email protected]
> > Subject: Re: [PATCH 2/7] ARM: tegra: update device trees for USB binding
> > rework
> >
> > Hi,
> >
> > On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
> > > This patch updates all Tegra board files so that they contain all the
> > > properties required by the updated USB DT binding. Note that this patch
> > > only adds the new properties and does not yet remove the old properties,
> > > in order to maintain bisectability. The old properties will be removed
> > > once the driver has been updated to assume the new bindings.
> > >
> > > Signed-off-by: Venu Byravarasu <[email protected]>
> > > ---
> > > arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
> > > arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
> > > arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
> > > arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
> > > arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
> > > arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
> > > arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
> > > arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
> > > 8 files changed, 57 insertions(+), 31 deletions(-)
> > >
> > > diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > > index cb73e62..af5a7ae 100644
> > > --- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > > +++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
> > > @@ -443,6 +443,10 @@
> > > nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
> > > };
> > >
> > > + usb-phy@c5004000 {
> > This node doesn't have a *reg* property. So "@c5004000" is not needed.
> > This comment applies to all the nodes which doesn't have *reg* property.
>
> Thanks Kishon for the comments.
> As I've 3 usb-phy DT nodes, how to differentiate between them if I remove this @Address ?
then add reg property :-)
--
balbi
> -----Original Message-----
> From: Felipe Balbi [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 5:55 PM
> To: Venu Byravarasu
> Cc: kishon; [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; devicetree-
> [email protected]
> Subject: Re: [PATCH 2/7] ARM: tegra: update device trees for USB binding
> rework
>
> * PGP Signed by an unknown key
>
> On Wed, Mar 20, 2013 at 05:47:46PM +0530, Venu Byravarasu wrote:
> > > -----Original Message-----
> > > From: kishon [mailto:[email protected]]
> > > Sent: Wednesday, March 20, 2013 4:53 PM
> > > To: Venu Byravarasu
> > > Cc: [email protected]; [email protected];
> > > [email protected]; [email protected]; [email protected];
> > > [email protected]; [email protected]; devicetree-
> > > [email protected]
> > > Subject: Re: [PATCH 2/7] ARM: tegra: update device trees for USB binding
> > > rework
> > >
> > > Hi,
> > > >
> > > > + usb-phy@c5004000 {
> > > This node doesn't have a *reg* property. So "@c5004000" is not needed.
> > > This comment applies to all the nodes which doesn't have *reg* property.
> >
> > Thanks Kishon for the comments.
> > As I've 3 usb-phy DT nodes, how to differentiate between them if I remove
> this @Address ?
>
> then add reg property :-)
Thanks Felipe, for confirming.
>
> --
> balbi
>
> * Unknown Key
> * 0x35CAA444
> -----Original Message-----
> From: Stephen Warren [mailto:[email protected]]
> Sent: Wednesday, March 20, 2013 1:51 AM
> To: Venu Byravarasu
> Cc: [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]
> Subject: Re: [PATCH 7/7] usb: phy: registering tegra USB PHY as platform
> driver
>
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
> > Registered tegra USB PHY as a separate platform driver.
> >
> > diff --git a/drivers/usb/phy/tegra_usb_phy.c
> b/drivers/usb/phy/tegra_usb_phy.c
>
> > static void tegra_usb_phy_close(struct usb_phy *x)
> > {
> > if (phy->is_ulpi_phy)
> > clk_put(phy->clk);
>
> phy->clk is obtained using devm_clk_get(). This typically means you
> never need to clk_put() it, and if for some reason you really have to,
> you should use devm_clk_put() instead of plain clk_put().
Agree, will remove clk_put.
>
> > @@ -774,23 +667,53 @@ struct tegra_usb_phy
> *tegra_usb_phy_open(struct device *dev, int instance,
>
> > + err = gpio_request(phy->reset_gpio, "ulpi_phy_reset_b");
>
> I think you can use devm_gpio_request() here to simplify the error-handling.
Sure, will do.
>
> I wonder why in the ULPI case, all the code is inline here, whereas in
> the UTMI case, this simply calls a function. Wouldn't it be more
> consistent to have the following code here:
>
> if (phy->is_ulpi_phy)
> err = ulpi_open();
> else
> err = utmip_open();
> if (err)
> goto fail;
Sure, will take care of this in next patch.
>
> > +static int tegra_usb_phy_probe(struct platform_device *pdev)
>
> Hmmm. Note that in order to make deferred probe work correctly, all the
> gpio_request(), clk_get(), etc. calls that acquire resources from other
> drivers must happen here in probe() and not in tegra_usb_phy_open().
In present code tegra_usb_phy_open() is indirectly called via usb_phy_init() from ehci-tegra.c.
Between obtaining PHY handle (and hence getting into deferred probe, when it is not available)
and calling usb_phy_init, no other USB registers were accessed. Hence I was doing this way.
Do you still think it would be better to move gpio and clk APIs to probe?
>
> > + err = of_property_match_string(np, "dr_mode", "otg");
> > + if (err < 0) {
> > + err = of_property_match_string(np, "dr_mode", "gadget");
>
> Again, use "peripheral", not "gadget".
Will do.
>
> > +struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
> > +{
> > + struct device *dev;
> > + struct tegra_usb_phy *tegra_phy;
> > +
> > + dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
> > + tegra_usb_phy_match);
> > + if (!dev)
> > + return ERR_PTR(-EPROBE_DEFER);
> > +
> > + tegra_phy = dev_get_drvdata(dev);
> > +
> > + return &tegra_phy->u_phy;
> > +}
>
> I think you need a module_get() somewhere in there, and also need to add
> a tegra_usb_put_phy() function too, so you can call module_put() from it.
Am not clear on why to add module_get here. Can you plz provide more details?
Thanks Stephen, for the review.
On 03/20/2013 06:15 AM, Venu Byravarasu wrote:
>> -----Original Message-----
>> From: kishon [mailto:[email protected]]
>> Sent: Wednesday, March 20, 2013 4:49 PM
>> To: Venu Byravarasu
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]; devicetree-
>> [email protected]
>> Subject: Re: [PATCH 1/7] ARM: tegra: finalize USB EHCI and PHY bindings
>>
>> Hi,
>>
>> On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
>>> The existing Tegra USB bindings have a few issues:
>>>
>>> 1) Many properties are documented as being part of the EHCI controller
>>> node, yet they apply more to the PHY device. They should be moved.
>>>
>>> 2) Some registers in PHY1 are shared with PHY3, and hence PHY3 needs a
>>> reg entry to point at PHY1's register space. We can't assume the PHY1
>>> driver is present, so the PHY3 driver will directly access those
>>> registers.
>>>
>>> 3) The list of clocks required by the PHY was missing some required
>>> entries.
>>>
>>> This patch fixes the binding definition to resolve these issues.
>>>
>>> Signed-off-by: Venu Byravarasu <[email protected]>
>>> ---
>>> .../bindings/usb/nvidia,tegra20-ehci.txt | 27 +++----------------
>>> .../bindings/usb/nvidia,tegra20-usb-phy.txt | 27
>> +++++++++++++++++--
>>> 2 files changed, 29 insertions(+), 25 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
>> b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
>>> index 34c9528..df09330 100644
>>> --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
>>> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-ehci.txt
>>> @@ -6,27 +6,10 @@ Practice : Universal Serial Bus" with the following
>> modifications
>>> and additions :
>>>
>>> Required properties :
>>> - - compatible : Should be "nvidia,tegra20-ehci" for USB controllers
>>> - used in host mode.
>>> - - phy_type : Should be one of "ulpi" or "utmi".
>>>
>>> Optional properties:
>>> - - dr_mode : dual role mode. Indicates the working mode for
>
>>> index 6bdaba2..7ceccd3 100644
>>> --- a/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
>>> +++ b/Documentation/devicetree/bindings/usb/nvidia,tegra20-usb-phy.txt
>>> @@ -4,8 +4,24 @@ The device node for Tegra SOC USB PHY:
>>>
>>> Required properties :
>>> + - phy_type : Should be one of "utmi", "ulpi" or "hsic".
>>
>> dt property names generally dont have "_".
>
> Thanks Kishon, for your comments.
> Is it mandatory or optional?
> If it is mandatory, then I might have to change dr_mode as well along with phy_type.
This rule is basically mandatory for *new* property definitions.
However, both phy_type and dr_mode are properties that already exist and
have historical precedence for their naming. So, I don't think we can
change them. In fact, IIRC, someone even posted some patches to provide
a common set of parsing functions for those properties, and I assume
those patches use the existing names with _ in them. The patches aren't
applied yet though, I think.
(Venu, we already discussed this rule downstream)
On 03/20/2013 05:23 AM, kishon wrote:
> Hi,
>
> On Monday 18 March 2013 05:59 PM, Venu Byravarasu wrote:
>> This patch updates all Tegra board files so that they contain all the
>> properties required by the updated USB DT binding. Note that this patch
>> only adds the new properties and does not yet remove the old properties,
>> in order to maintain bisectability. The old properties will be removed
>> once the driver has been updated to assume the new bindings.
>>
>> Signed-off-by: Venu Byravarasu <[email protected]>
>> ---
>> arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
>> arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
>> arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
>> arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
>> arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
>> arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
>> arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
>> arch/arm/boot/dts/tegra20.dtsi | 32
>> +++++++++++++++++----------
>> 8 files changed, 57 insertions(+), 31 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
>> b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
>> index cb73e62..af5a7ae 100644
>> --- a/arch/arm/boot/dts/tegra20-colibri-512.dtsi
>> +++ b/arch/arm/boot/dts/tegra20-colibri-512.dtsi
>> @@ -443,6 +443,10 @@
>> nvidia,phy-reset-gpio = <&gpio 169 0>; /* gpio PV1 */
>> };
>>
>> + usb-phy@c5004000 {
>
> This node doesn't have a *reg* property. So "@c5004000" is not needed.
> This comment applies to all the nodes which doesn't have *reg* property.
Yes it does have a reg property.
The node itself is first defined in tegra20.dtsi, and does contain a reg
property there.
tegra20-colibri-512.dtsi (and many other files in this patch) include
tegra20.dtsi, and simply add additional board-specific properties to the
existing node, and should not re-iterate properties that already exist.
On 03/20/2013 06:12 AM, Venu Byravarasu wrote:
>> -----Original Message-----
>> From: Venu Byravarasu
>> Sent: Wednesday, March 20, 2013 11:30 AM
>> To: 'Stephen Warren'
>> Cc: [email protected]; [email protected];
>> [email protected]; [email protected]; [email protected];
>> [email protected]; [email protected]
>> Subject: RE: [PATCH 0/7] USB: PHY: Tegra: registering TEGRA USB PHY as
>> platform driver
>>
>>> -----Original Message-----
>>> From: Stephen Warren [mailto:[email protected]]
>>> Sent: Wednesday, March 20, 2013 1:22 AM
>>> To: Venu Byravarasu
>>> Cc: [email protected]; [email protected];
>>> [email protected]; [email protected]; [email protected];
>>> [email protected]; [email protected]
>>> Subject: Re: [PATCH 0/7] USB: PHY: Tegra: registering TEGRA USB PHY as
>>> platform driver
>>>
>>> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>>>> As part of this series, apart from patch containing changes to register
>>> TEGRA
>>>> USB PHY driver as platform driver, prepared below patches:
>>>> 1. Re-arranging & adding new DT properties.
>>>> 2. Getting various params from DT properties added.
>>>> 3. code clean up.
>>>
>>> Venu, I'm curious whether these patches were tested at all. I have found
>>> at least two significant problems with trivial testing:
>>
>> Stephen,
>> Initially started testing after applying each and every patch.
>> Like that tested till first 5 patches.
>> As did not see any issues till then, applied rest 2 patches at once and tested
>> with that.
>> Though did not see mouse getting vbus on the 1st boot, Vbus was coming
>> fine after disconnect and connect.
>> Hence did not test thereafter.
>>
>> After checking your current mail, tried now and observed that there seems to
>> be some real issue with patch#7 only. (As tried now after applying till patch#
>> 6 and did not see this issue).
>> Will debug further on patch#7 and update with proper fix after addressing
>> your other comments.
>
> Debugged further and found that the issue is because of http://marc.info/?l=linux-arm-kernel&m=135890098024987&w=2
> On reverting that patch and applying it on top of patch#7, able to see enumeration working fine.
>
> Anyhow, will take care of your other comments and merge this change with patch#7 and resend
> for review.
Venu, we already discussed this downstream, and I pointed out that patch
is /extremely/ unlikely to cause any issue.
The clk_set_rate() call returns an error since the requested clock rate
is not supported. The clk_prepare_enable() shouldn't have any effect
because if it did, the only possible meaning is that the clock is
otherwise turned off because it has no users, and that would prevent far
more than USB from operating correctly.
Please fully debug this problem and root-cause it. I'm not reverting
that EMC clock patch without a complete explanation. There's little
point reposting the patches until you've found the problem.
If we did have to revert that patch, then we would need to redefine the
DT bindings for Tegra USB to add that clock into the list of required
clocks, since otherwise it could not clk_get(dev, "emc"). But the USB
driver shouldn't be touching non-USB clocks.
On 03/20/2013 06:43 AM, Venu Byravarasu wrote:
> Stephen Warren wrote at Wednesday, March 20, 2013 1:51 AM:
>> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>>> Registered tegra USB PHY as a separate platform driver.
>>> diff --git a/drivers/usb/phy/tegra_usb_phy.c b/drivers/usb/phy/tegra_usb_phy.c
>>> +static int tegra_usb_phy_probe(struct platform_device *pdev)
>>
>> Hmmm. Note that in order to make deferred probe work correctly, all the
>> gpio_request(), clk_get(), etc. calls that acquire resources from other
>> drivers must happen here in probe() and not in tegra_usb_phy_open().
>
> In present code tegra_usb_phy_open() is indirectly called via usb_phy_init() from ehci-tegra.c.
> Between obtaining PHY handle (and hence getting into deferred probe, when it is not available)
> and calling usb_phy_init, no other USB registers were accessed. Hence I was doing this way.
>
> Do you still think it would be better to move gpio and clk APIs to probe?
Yes.
What's happening right now is that the PHY driver potentially completes
probe() before its resources are available.
This just happens not to break anything because the EHCI driver's probe
ends up getting deferred until some other PHY driver function can
acquire those resources, so the USB port as a whole isn't registered
until the resources are available.
However, this still means that the PHY driver could be suspended after
e.g. a driver that provides a GPIO it uses, since the PHY's probe
completed before the GPIO driver's probe.
Hence, this could easily cause some form of suspend/resume or perhaps
even shutdown/reboot problem.
>>> +struct usb_phy *tegra_usb_get_phy(struct device_node *dn)
>>> +{
>>> + struct device *dev;
>>> + struct tegra_usb_phy *tegra_phy;
>>> +
>>> + dev = driver_find_device(&tegra_usb_phy_driver.driver, NULL, dn,
>>> + tegra_usb_phy_match);
>>> + if (!dev)
>>> + return ERR_PTR(-EPROBE_DEFER);
>>> +
>>> + tegra_phy = dev_get_drvdata(dev);
>>> +
>>> + return &tegra_phy->u_phy;
>>> +}
>>
>> I think you need a module_get() somewhere in there, and also need to add
>> a tegra_usb_put_phy() function too, so you can call module_put() from it.
>
> Am not clear on why to add module_get here. Can you plz provide more details?
Actually, I guess it isn't needed, although slightly by accident perhaps:
If ehci-tegra.c and phy-usb-tegra.c are built as modules, they'll be
separate modules. Since ehci-tegra.c calls into phy-usb-tegra.c, you
need to make sure that phy-usb-tegra.c stays loaded any time
ehci-tegra.c is loaded.
Right now, this is ensured because ehci-tegra.c references functions in
phy-usb-tegra.c by symbol name, so a dependency exists. So, I guess
everything actually works out without the module_get(). So, no changes
are needed.
After this series is applied, I believe that tegra_usb_get_phy() is the
last function that ehci-tegra.c references by symbol name. Eventually,
that function will be replaced by devm_of_phy_get_byname() (see Kishon's
generic PHY API patch series), so there won't be any symbol name
dependencies, so some other mechanism must be used to ensure the PHY
driver stays loaded while the EHCI driver is using it. At that point,
the implementation of devm_of_phy_get_byname() should be calling
module_get(), so again no changes should be required to your patch.
On 03/20/2013 11:36 AM, Stephen Warren wrote:
> On 03/20/2013 06:12 AM, Venu Byravarasu wrote:
>> Venu Byravarasu wrote at Wednesday, March 20, 2013 11:30 AM:
>>> Stephen Warren wrote at Wednesday, March 20, 2013 1:22 AM:
>>>> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>>>>> As part of this series, apart from patch containing changes to register
>>>> TEGRA
>>>>> USB PHY driver as platform driver, prepared below patches:
>>>>> 1. Re-arranging & adding new DT properties.
>>>>> 2. Getting various params from DT properties added.
>>>>> 3. code clean up.
>>>>
>>>> Venu, I'm curious whether these patches were tested at all. I have found
>>>> at least two significant problems with trivial testing:
>>>
>>> Stephen,
>>> Initially started testing after applying each and every patch.
>>> Like that tested till first 5 patches.
>>> As did not see any issues till then, applied rest 2 patches at once and tested
>>> with that.
>>> Though did not see mouse getting vbus on the 1st boot, Vbus was coming
>>> fine after disconnect and connect.
>>> Hence did not test thereafter.
>>>
>>> After checking your current mail, tried now and observed that there seems to
>>> be some real issue with patch#7 only. (As tried now after applying till patch#
>>> 6 and did not see this issue).
>>> Will debug further on patch#7 and update with proper fix after addressing
>>> your other comments.
>>
>> Debugged further and found that the issue is because of http://marc.info/?l=linux-arm-kernel&m=135890098024987&w=2
>> On reverting that patch and applying it on top of patch#7, able to see enumeration working fine.
>>
>> Anyhow, will take care of your other comments and merge this change with patch#7 and resend
>> for review.
>
> Venu, we already discussed this downstream, and I pointed out that patch
> is /extremely/ unlikely to cause any issue.
OK, so I found that reverting that patch does "solve" the issue, but
it's got nothing to do with that patch being wrong. There is some memory
corruption issue in your patches that is hidden if that patch is reverted.
In other words, on both Springbank (which you don't have, and has an EMC
scaling table so that the EMC frequency probably varies with CPU load)
and Ventana (which is what I assume you're testing on, and has no EMC
table, so the EMC clock frequency is fixed):
1) Tegra's for-next branch: Works fine.
2) (1) plus your patches: Broken as I described above.
3) (2) plus revert 9304512 "usb: host: tegra: don't touch EMC clock":
works fine on Ventana, somewhat works on Seaboard; lots of "can't
enumerate USB bus" messages, but eventually succeeds.
The really interesting thing is that if I take (2) above plus *just* the
following patch:
> diff --git a/drivers/usb/host/ehci-tegra.c b/drivers/usb/host/ehci-tegra.c
> index 772fa29..7499240 100644
> --- a/drivers/usb/host/ehci-tegra.c
> +++ b/drivers/usb/host/ehci-tegra.c
> @@ -44,6 +44,7 @@ struct tegra_ehci_hcd {
> struct ehci_hcd *ehci;
> struct tegra_usb_phy *phy;
> struct clk *clk;
> + struct clk *emc_clk;
> struct usb_phy *transceiver;
> int host_resumed;
> int port_resuming;
Then it works fine on both Ventana and Seaboard.
To me, this says that there's nothing at all wrong with 9304512 "usb:
host: tegra: don't touch EMC clock", but rather there is some memory
corruption going on in your patches series, and adding that extra field
in the struct just hides the effect of the memory corruption.
Please debug and fix that. Thanks.
On 03/19/2013 02:10 PM, Stephen Warren wrote:
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>> Check return values from all GPIO APIs and handle errors accordingly.
>
>> Remove clk_disable_unprepare which is no more needed.
>
> The call to clk_disable_unprepare is incorrect in the current code. The
> way you worded that, it sounds like it's no longer needed because of the
> changes made in this patch. I would re-write that last sentence as:
>
> Remove the call to clk_disable_unprepare(); this function does not
> prepare or enable the clock, so the error path should not disable or
> unprepare it.
V2 didn't address this review feedback:-(
On 03/19/2013 01:53 PM, Stephen Warren wrote:
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>> This patch updates all Tegra board files so that they contain all the
>> properties required by the updated USB DT binding. Note that this patch
>> only adds the new properties and does not yet remove the old properties,
>> in order to maintain bisectability. The old properties will be removed
>> once the driver has been updated to assume the new bindings.
>>
>> Signed-off-by: Venu Byravarasu <[email protected]>
>> ---
>> arch/arm/boot/dts/tegra20-colibri-512.dtsi | 4 +++
>> arch/arm/boot/dts/tegra20-harmony.dts | 8 +++---
>> arch/arm/boot/dts/tegra20-iris-512.dts | 4 +++
>> arch/arm/boot/dts/tegra20-paz00.dts | 8 +++---
>> arch/arm/boot/dts/tegra20-seaboard.dts | 13 +++++++---
>> arch/arm/boot/dts/tegra20-trimslice.dts | 12 +++++++---
>> arch/arm/boot/dts/tegra20-ventana.dts | 7 +++--
>> arch/arm/boot/dts/tegra20.dtsi | 32 +++++++++++++++++----------
>> 8 files changed, 57 insertions(+), 31 deletions(-)
>
> I think you forgot to update arch/arm/boot/dts/tegra20-whistler.dts in
> this patch.
This was not fixed in V2.
On 03/19/2013 01:51 PM, Stephen Warren wrote:
> On 03/18/2013 06:29 AM, Venu Byravarasu wrote:
>> As part of this series, apart from patch containing changes to register TEGRA
>> USB PHY driver as platform driver, prepared below patches:
>> 1. Re-arranging & adding new DT properties.
>> 2. Getting various params from DT properties added.
>> 3. code clean up.
>
> Venu, I'm curious whether these patches were tested at all. I have found
> at least two significant problems with trivial testing:
...
> "reboot" or "shutdown -h now" both cause the following crash
...
> The first time enumeration USB devices is attempted on a port fails.
For the record, V2 of this series fixes both of these problems.