[PATCH 1/7] clk: sunxi: Implement A31 USB clock

[Maxime Ripard]

The A31 USB clock slightly differ from its older counterparts, mostly because
it has a different gate for each PHY, while the older one had a single gate for
all the phy.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---
drivers/clk/sunxi/clk-sunxi.c | 6 ++++++
1 file changed, 6 insertions(+)

diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
index bd7dc733c1ca..d9bab75f128b 100644
--- a/drivers/clk/sunxi/clk-sunxi.c
+++ b/drivers/clk/sunxi/clk-sunxi.c
@@ -972,6 +972,11 @@ static const struct gates_data sun5i_a13_usb_gates_data __initconst = {
.reset_mask = 0x03,
};

+static const struct gates_data sun6i_a31_usb_gates_data __initconst = {
+ .mask = { BIT(18) | BIT(17) | BIT(16) | BIT(10) | BIT(9) | BIT(8) },
+ .reset_mask = BIT(2) | BIT(1) | BIT(0),
+};
+
static void __init sunxi_gates_clk_setup(struct device_node *node,
struct gates_data *data)
{
@@ -1267,6 +1272,7 @@ static const struct of_device_id clk_gates_match[] __initconst = {
{.compatible = "allwinner,sun6i-a31-apb2-gates-clk", .data = &sun6i_a31_apb2_gates_data,},
{.compatible = "allwinner,sun4i-a10-usb-clk", .data = &sun4i_a10_usb_gates_data,},
{.compatible = "allwinner,sun5i-a13-usb-clk", .data = &sun5i_a13_usb_gates_data,},
+ {.compatible = "allwinner,sun6i-a31-usb-clk", .data = &sun6i_a31_usb_gates_data,},
{}
};

--
1.9.1

[Maxime Ripard]

The USB clocks of the A31 seems to be parented to the 24MHz oscillator, and
handle the clocks for the USB phys and OHCI devices.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index eec1afa257a5..13aa56ed5858 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -269,6 +269,17 @@
clocks = <&osc24M>, <&pll6>;
clock-output-names = "spi3";
};
+
+ usb_clk: clk@01c200cc {
+ #clock-cells = <1>;
+ #reset-cells = <1>;
+ compatible = "allwinner,sun6i-a31-usb-clk";
+ reg = <0x01c200cc 0x4>;
+ clocks = <&osc24M>;
+ clock-output-names = "usb_phy0", "usb_phy1", "usb_phy2",
+ "usb_ohci0", "usb_ohci1",
+ "usb_ohci2";
+ };
};

soc@01c00000 {
--
1.9.1

[Maxime Ripard]

From: Boris BREZILLON <boris.b...@free-electrons.com>

On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to
be deasserted for the EHCI block to be usable.

Add support for an optional reset controller that will be deasserted on
power off and asserted on power on.

Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
drivers/usb/host/ehci-platform.c | 25 +++++++++++++++++++++-
2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index ff151ec084c4..43c1a4e06767 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt
@@ -15,6 +15,7 @@ Optional properties:
- clocks : a list of phandle + clock specifier pairs
- phys : phandle + phy specifier pair
- phy-names : "usb"
+ - resets : phandle + reset specifier pair

Example (Sequoia 440EPx):
ehci@e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c
index c7dd93aad20c..4ee67728e443 100644
--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
#include <linux/of.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
#include <linux/usb/ehci_pdriver.h>
@@ -41,6 +42,7 @@

struct ehci_platform_priv {
struct clk *clks[EHCI_MAX_CLKS];
+ struct reset_control *rst;
struct phy *phy;
};

@@ -84,10 +86,16 @@ static int ehci_platform_power_on(struct platform_device *dev)
goto err_disable_clks;
}

+ if (priv->rst) {
+ ret = reset_control_deassert(priv->rst);
+ if (ret)
+ goto err_disable_clks;
+ }
+
if (priv->phy) {
ret = phy_init(priv->phy);
if (ret)
- goto err_disable_clks;
+ goto err_assert_rst;

ret = phy_power_on(priv->phy);
if (ret)
@@ -98,6 +106,9 @@ static int ehci_platform_power_on(struct platform_device *dev)

err_exit_phy:
phy_exit(priv->phy);
+err_assert_rst:
+ if (priv->rst)
+ reset_control_assert(priv->rst);
err_disable_clks:
while (--clk >= 0)
clk_disable_unprepare(priv->clks[clk]);
@@ -119,6 +130,9 @@ static void ehci_platform_power_off(struct platform_device *dev)
for (clk = EHCI_MAX_CLKS - 1; clk >= 0; clk--)
if (priv->clks[clk])
clk_disable_unprepare(priv->clks[clk]);
+
+ if (priv->rst)
+ reset_control_assert(priv->rst);
}

static struct hc_driver __read_mostly ehci_platform_hc_driver;
@@ -206,6 +220,15 @@ static int ehci_platform_probe(struct platform_device *dev)
break;
}
}
+
+ priv->rst = devm_reset_control_get_optional(&dev->dev,
+ NULL);
+ if (IS_ERR(priv->rst)) {
+ err = PTR_ERR(priv->rst);
+ if (err == -EPROBE_DEFER)
+ goto err_put_clks;
+ priv->rst = NULL;
+ }
}

if (pdata->big_endian_desc)
--
1.9.1

[Maxime Ripard]

The OHCI controllers used in the Allwinner A31 are asserted in reset using a
global reset controller.

Add optional support for such a controller in the OHCI platform driver.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

Documentation/devicetree/bindings/usb/usb-ohci.txt | 1 +
drivers/usb/host/ohci-platform.c | 25 +++++++++++++++++++++-

2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-ohci.txt b/Documentation/devicetree/bindings/usb/usb-ohci.txt
index 45f67d91e888..b968a1aea995 100644
--- a/Documentation/devicetree/bindings/usb/usb-ohci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ohci.txt
@@ -12,6 +12,7 @@ Optional properties:

- clocks : a list of phandle + clock specifier pairs
- phys : phandle + phy specifier pair
- phy-names : "usb"

+- resets : phandle + reset specifier pair

Example:

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index b6002c951c5c..55f6c87628eb 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -24,6 +24,7 @@
#include <linux/err.h>

#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>

#include <linux/usb/ohci_pdriver.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -36,6 +37,7 @@

struct ohci_platform_priv {
struct clk *clks[OHCI_MAX_CLKS];

+ struct reset_control *rst;
struct phy *phy;
};

@@ -67,10 +69,16 @@ static int ohci_platform_power_on(struct platform_device *dev)

goto err_disable_clks;
}

+ if (priv->rst) {
+ ret = reset_control_deassert(priv->rst);
+ if (ret)
+ goto err_disable_clks;
+ }
+
if (priv->phy) {
ret = phy_init(priv->phy);
if (ret)
- goto err_disable_clks;
+ goto err_assert_rst;

ret = phy_power_on(priv->phy);
if (ret)

@@ -81,6 +89,9 @@ static int ohci_platform_power_on(struct platform_device *dev)

err_exit_phy:
phy_exit(priv->phy);
+err_assert_rst:
+ if (priv->rst)
+ reset_control_assert(priv->rst);
err_disable_clks:
while (--clk >= 0)
clk_disable_unprepare(priv->clks[clk]);

@@ -99,6 +110,9 @@ static void ohci_platform_power_off(struct platform_device *dev)
phy_exit(priv->phy);

}

+ if (priv->rst)
+ reset_control_assert(priv->rst);

+
for (clk = OHCI_MAX_CLKS - 1; clk >= 0; clk--)

if (priv->clks[clk])
clk_disable_unprepare(priv->clks[clk]);

@@ -191,6 +205,15 @@ static int ohci_platform_probe(struct platform_device *dev)

[Maxime Ripard]

The A31 has two ECHI/OHCI controllers, and one OHCI-only phy-less controller.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

arch/arm/boot/dts/sun6i-a31.dtsi | 77 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 13aa56ed5858..5e9f01af6d99 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi
@@ -341,6 +341,83 @@
status = "disabled";
};

+ usbphy: phy@01c19400 {
+ compatible = "allwinner,sun6i-a31-usb-phy";
+ reg = <0x01c19400 0x10>,
+ <0x01c1a800 0x4>,
+ <0x01c1b800 0x4>;
+ reg-names = "phy_ctrl",
+ "pmu1",
+ "pmu2";
+ clocks = <&usb_clk 8>,
+ <&usb_clk 9>,
+ <&usb_clk 10>;
+ clock-names = "usb0_phy",
+ "usb1_phy",
+ "usb2_phy";
+ resets = <&usb_clk 0>,
+ <&usb_clk 1>,
+ <&usb_clk 2>;
+ reset-names = "usb0_reset",
+ "usb1_reset",
+ "usb2_reset";
+ status = "disabled";
+ #phy-cells = <1>;
+ };
+
+ ehci0: usb@01c1a000 {
+ compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
+ reg = <0x01c1a000 0x100>;
+ interrupts = <0 72 4>;
+ clocks = <&ahb1_gates 26>;
+ resets = <&ahb1_rst 26>;
+ phys = <&usbphy 1>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ ohci0: usb@01c1a400 {
+ compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
+ reg = <0x01c1a400 0x100>;
+ interrupts = <0 73 4>;
+ clocks = <&ahb1_gates 29>, <&usb_clk 16>;
+ resets = <&ahb1_rst 29>;
+ phys = <&usbphy 1>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ ehci1: usb@01c1b000 {
+ compatible = "allwinner,sun6i-a31-ehci", "generic-ehci";
+ reg = <0x01c1b000 0x100>;
+ interrupts = <0 74 4>;
+ clocks = <&ahb1_gates 27>;
+ resets = <&ahb1_rst 27>;
+ phys = <&usbphy 2>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ ohci1: usb@01c1b400 {
+ compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
+ reg = <0x01c1b400 0x100>;
+ interrupts = <0 75 4>;
+ clocks = <&ahb1_gates 30>, <&usb_clk 17>;
+ resets = <&ahb1_rst 30>;
+ phys = <&usbphy 2>;
+ phy-names = "usb";
+ status = "disabled";
+ };
+
+ ohci2: usb@01c1c000 {
+ compatible = "allwinner,sun6i-a31-ohci", "generic-ohci";
+ reg = <0x01c1c400 0x100>;
+ interrupts = <0 77 4>;
+ clocks = <&ahb1_gates 31>, <&usb_clk 18>;
+ resets = <&ahb1_rst 31>;
+ status = "disabled";
+ };
+
pio: pinctrl@01c20800 {
compatible = "allwinner,sun6i-a31-pinctrl";
reg = <0x01c20800 0x400>;
--
1.9.1

[Maxime Ripard]

The USB phy controller in the A31 differs mostly from the older controllers
because it has a clock dedicated for each phy, while the older ones were having
a single clock for all the phys.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

drivers/phy/phy-sun4i-usb.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
index e6e6c4ba7145..1d83abe07a29 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -61,16 +61,17 @@
#define MAX_PHYS 3

struct sun4i_usb_phy_data {
- struct clk *clk;
void __iomem *base;
struct mutex mutex;
int num_phys;
u32 disc_thresh;
+ bool dedicated_clocks;
struct sun4i_usb_phy {
struct phy *phy;
void __iomem *pmu;
struct regulator *vbus;
struct reset_control *reset;
+ struct clk *clk;
int index;
} phys[MAX_PHYS];
};
@@ -146,13 +147,13 @@ static int sun4i_usb_phy_init(struct phy *_phy)
struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
int ret;

- ret = clk_prepare_enable(data->clk);
+ ret = clk_prepare_enable(phy->clk);
if (ret)
return ret;

ret = reset_control_deassert(phy->reset);
if (ret) {
- clk_disable_unprepare(data->clk);
+ clk_disable_unprepare(phy->clk);
return ret;
}

@@ -170,11 +171,10 @@ static int sun4i_usb_phy_init(struct phy *_phy)
static int sun4i_usb_phy_exit(struct phy *_phy)
{
struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
- struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);

sun4i_usb_phy_passby(phy, 0);
reset_control_assert(phy->reset);
- clk_disable_unprepare(data->clk);
+ clk_disable_unprepare(phy->clk);

return 0;
}
@@ -230,6 +230,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
struct regulator *vbus;
struct resource *res;
struct phy *phy;
+ struct clk *clk;
char name[16];
int i;

@@ -249,15 +250,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
else
data->disc_thresh = 2;

+ if (of_device_is_compatible(np, "allwinner,sun6i-a31-usb-phy"))
+ data->dedicated_clocks = true;
+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_ctrl");
data->base = devm_ioremap_resource(dev, res);
if (IS_ERR(data->base))
return PTR_ERR(data->base);

- data->clk = devm_clk_get(dev, "usb_phy");
- if (IS_ERR(data->clk)) {
- dev_err(dev, "could not get usb_phy clock\n");
- return PTR_ERR(data->clk);
+ if (!data->dedicated_clocks) {
+ clk = devm_clk_get(dev, "usb_phy");
+ if (IS_ERR(clk)) {
+ dev_err(dev, "could not get usb_phy clock\n");
+ return PTR_ERR(clk);
+ }
}

/* Skip 0, 0 is the phy for otg which is not yet supported. */
@@ -270,6 +276,15 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
vbus = NULL;
}

+ if (data->dedicated_clocks) {
+ snprintf(name, sizeof(name), "usb%d_phy", i);
+ clk = devm_clk_get(dev, name);
+ if (IS_ERR(clk)) {
+ dev_err(dev, "failed to get clock %s\n", name);
+ return PTR_ERR(clk);
+ }
+ }
+
snprintf(name, sizeof(name), "usb%d_reset", i);
reset = devm_reset_control_get(dev, name);
if (IS_ERR(reset)) {
@@ -296,6 +311,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
data->phys[i].pmu = pmu;
data->phys[i].vbus = vbus;
data->phys[i].reset = reset;
+ data->phys[i].clk = clk;
data->phys[i].index = i;
phy_set_drvdata(phy, &data->phys[i]);
}
@@ -311,6 +327,7 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
static const struct of_device_id sun4i_usb_phy_of_match[] = {
{ .compatible = "allwinner,sun4i-a10-usb-phy" },
{ .compatible = "allwinner,sun5i-a13-usb-phy" },
+ { .compatible = "allwinner,sun6i-a31-usb-phy" },
{ .compatible = "allwinner,sun7i-a20-usb-phy" },
{ },
};
--
1.9.1

[Maxime Ripard]

Hi everyone,

This patchset adds support for the USB controllers found in the
Allwinner A31.

While the design is similar to the earlier Allwinner SoCs that are
already supported, a few details here and there change, like the fact
that the PHYs now have one clock per phy, while it used to be only one
for all the PHYs.

Thanks,
Maxime

Boris BREZILLON (2):
usb: ehci-platform: add optional reset controller retrieval
ARM: sunxi: dt: add APP4-EVB1 board support

Maxime Ripard (5):
clk: sunxi: Implement A31 USB clock
ARM: sun6i: Add the USB clocks to the DTSI.
phy: usb: sunxi: Introduce Allwinner A31 USB PHY support
usb: ohci-platform: Enable optional use of reset controller
ARM: sun6i: dt: Add support for the USB controllers

Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
Documentation/devicetree/bindings/usb/usb-ohci.txt | 1 +
arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun6i-a31-app4-evb1.dts | 63 ++++++++++++++++
arch/arm/boot/dts/sun6i-a31.dtsi | 88 ++++++++++++++++++++++
drivers/clk/sunxi/clk-sunxi.c | 6 ++
drivers/phy/phy-sun4i-usb.c | 35 ++++++---
drivers/usb/host/ehci-platform.c | 25 +++++-
drivers/usb/host/ohci-platform.c | 25 +++++-
9 files changed, 234 insertions(+), 11 deletions(-)
create mode 100644 arch/arm/boot/dts/sun6i-a31-app4-evb1.dts

--
1.9.1

[Maxime Ripard]

From: Boris BREZILLON <boris.b...@free-electrons.com>

The APP4 EVB1 development boards embeds an A31, together with some NAND, one SD
card slot, and one SDIO + UART WiFi and Bluetooth chip, a few I2C buses, USB,
and a LCD display.

Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
---

arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun6i-a31-app4-evb1.dts | 63 +++++++++++++++++++++++++++++++
2 files changed, 64 insertions(+)
create mode 100644 arch/arm/boot/dts/sun6i-a31-app4-evb1.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index ffa3f5ef27d3..d50c0895a9d5 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -343,6 +343,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += \
sun5i-a10s-olinuxino-micro.dtb \
sun5i-a13-olinuxino.dtb \
sun5i-a13-olinuxino-micro.dtb \
+ sun6i-a31-app4-evb1.dtb \
sun6i-a31-colombus.dtb \
sun6i-a31-m9.dtb \
sun7i-a20-cubieboard2.dtb \
diff --git a/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts b/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts
new file mode 100644
index 000000000000..270ab978f858
--- /dev/null
+++ b/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts
@@ -0,0 +1,63 @@
+/*
+ * Copyright 2014 Boris Brezillon
+ *
+ * Boris Brezillon <boris.b...@free-electrons.com>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:
+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+/include/ "sun6i-a31.dtsi"
+
+/ {
+ model = "Allwinner A31 APP4 EVB1 Evaluation Board";
+ compatible = "allwinner,app4-evb1", "allwinner,sun6i-a31";
+
+ chosen {
+ bootargs = "earlyprintk console=ttyS0,115200";
+ };
+
+ soc@01c00000 {
+ pio: pinctrl@01c20800 {
+ usb1_vbus_pin_a: usb1_vbus_pin@0 {
+ allwinner,pins = "PH27";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
+ };
+
+ usbphy: phy@01c19400 {
+ usb1_vbus-supply = <&reg_usb1_vbus>;
+ status = "okay";

+ };
+
+ ehci0: usb@01c1a000 {

+ status = "okay";
+ };
+
+ uart0: serial@01c28000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pins_a>;
+ status = "okay";
+ };
+ };
+
+ reg_usb1_vbus: usb1-vbus {
+ compatible = "regulator-fixed";
+ pinctrl-names = "default";
+ pinctrl-0 = <&usb1_vbus_pin_a>;
+ regulator-name = "usb1-vbus";
+ regulator-min-microvolt = <5000000>;
+ regulator-max-microvolt = <5000000>;
+ enable-active-high;
+ gpio = <&pio 7 27 0>;
+ status = "okay";
+ };
+
+};
--
1.9.1

[Koen Kooi]

Doesn't the kernel try to avoid the 'or later' clause?

regards,

Koen

> --
> You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi...@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[Hans de Goede]

Hi,

The kernel as a whole is just "version 2", but for individual files /
drivers it is left up to the author whether to add "or later " or not.

Regards,

Hans

[Hans de Goede]

Hi,

On 05/07/2014 05:50 AM, Maxime Ripard wrote:
> Hi everyone,
>
> This patchset adds support for the USB controllers found in the
> Allwinner A31.
>
> While the design is similar to the earlier Allwinner SoCs that are
> already supported, a few details here and there change, like the fact
> that the PHYs now have one clock per phy, while it used to be only one
> for all the PHYs.
>
> Thanks,
> Maxime

Thanks for working on this.

Looks good, the entire series is:

Reviewed-by: Hans de Goede <hdeg...@redhat.com>

Regards,

Hans

[Kishon Vijay Abraham I]

Hi,

On Wednesday 07 May 2014 09:20 AM, Maxime Ripard wrote:
> The USB phy controller in the A31 differs mostly from the older controllers
> because it has a clock dedicated for each phy, while the older ones were having
> a single clock for all the phys.
>
> Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
> ---
> drivers/phy/phy-sun4i-usb.c | 35 ++++++++++++++++++++++++++---------
> 1 file changed, 26 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c
> index e6e6c4ba7145..1d83abe07a29 100644
> --- a/drivers/phy/phy-sun4i-usb.c
> +++ b/drivers/phy/phy-sun4i-usb.c
> @@ -61,16 +61,17 @@
> #define MAX_PHYS 3
>
> struct sun4i_usb_phy_data {
> - struct clk *clk;
> void __iomem *base;
> struct mutex mutex;
> int num_phys;
> u32 disc_thresh;
> + bool dedicated_clocks;

Don't think we need this structure member as it's been used only in probe.

Thanks
Kishon

[Alan Stern]

On Tue, 6 May 2014, Maxime Ripard wrote:

> From: Boris BREZILLON <boris.b...@free-electrons.com>
>
> On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to
> be deasserted for the EHCI block to be usable.
>
> Add support for an optional reset controller that will be deasserted on
> power off and asserted on power on.
>
> Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>

Is this really a _reset_ line? That is, when you assert the reset
line, does it actually reset the EHCI controller, or does it merely
leave the controller in a partially powered-down state?

The difference is important. During suspend, the controller is
supposed to remember the state of the port connections as well as other
settings. If it doesn't, the controller and all attached USB devices
will have to be reinitialized every time the controller resumes, which
will increase the latency.

Alan Stern

[Sergei Shtylyov]

Hello.

On 07-05-2014 7:50, Maxime Ripard wrote:

> The OHCI controllers used in the Allwinner A31 are asserted in reset using a

s/asserted/powered up/?

> global reset controller.

> Add optional support for such a controller in the OHCI platform driver.

> Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
> ---
> Documentation/devicetree/bindings/usb/usb-ohci.txt | 1 +
> drivers/usb/host/ohci-platform.c | 25 +++++++++++++++++++++-
> 2 files changed, 25 insertions(+), 1 deletion(-)

WBR, Sergei

[Maxime Ripard]

Ah, right.

I'll change this in the next version.

Thanks!
Maxime

--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

[Maxime Ripard]

On Wed, May 07, 2014 at 10:25:55AM -0400, Alan Stern wrote:
> On Tue, 6 May 2014, Maxime Ripard wrote:
>
> > From: Boris BREZILLON <boris.b...@free-electrons.com>
> >
> > On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to
> > be deasserted for the EHCI block to be usable.
> >
> > Add support for an optional reset controller that will be deasserted on
> > power off and asserted on power on.
> >
> > Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>
> > Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
>
> Is this really a _reset_ line? That is, when you assert the reset
> line, does it actually reset the EHCI controller, or does it merely
> leave the controller in a partially powered-down state?

It actually resets the whole controller.

> The difference is important. During suspend, the controller is
> supposed to remember the state of the port connections as well as other
> settings. If it doesn't, the controller and all attached USB devices
> will have to be reinitialized every time the controller resumes, which
> will increase the latency.

So you're saying that we should move this to the probe then?

Thanks,

[Maxime Ripard]

On Wed, May 07, 2014 at 08:26:39PM +0400, Sergei Shtylyov wrote:
> Hello.
>
> On 07-05-2014 7:50, Maxime Ripard wrote:
>
> >The OHCI controllers used in the Allwinner A31 are asserted in reset using a
>
> s/asserted/powered up/?

No. There's an external reset controller that maintains these devices
into reset. It's not only a power on thing, it can also be put back
into reset later in the life of the system.

[Hans de Goede]

Hi,

Yes.

Regards,

Hans

[Sergei Shtylyov]

Hello.

On 05/08/2014 02:03 AM, Maxime Ripard wrote:

>>> The OHCI controllers used in the Allwinner A31 are asserted in reset using a

>> s/asserted/powered up/?

> No. There's an external reset controller that maintains these devices
> into reset. It's not only a power on thing, it can also be put back
> into reset later in the life of the system.

However, the expression that "the OHCI controllers are asserted in reset"
still seems dubious to me...

> Maxime

WBR, Sergei

[Alan Stern]

That's right. The controller should not be reset during suspend, if
you can possibly avoid it.

There isn't any real benefit to asserting the reset signal during
suspend, is there? I mean, it won't use any less power, right?

Alan Stern

[Maxime Ripard]

I don't think it will. I'll update the patches and send a new version.

Thanks!

[Maxime Ripard]

The A31 USB clock slightly differ from its older counterparts, mostly because
it has a different gate for each PHY, while the older one had a single gate for
all the phy.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>

Reviewed-by: Hans de Goede <hdeg...@redhat.com>

[Maxime Ripard]

The USB clocks of the A31 seems to be parented to the 24MHz oscillator, and
handle the clocks for the USB phys and OHCI devices.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

arch/arm/boot/dts/sun6i-a31.dtsi | 11 +++++++++++
1 file changed, 11 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index eec1afa257a5..13aa56ed5858 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi

[Maxime Ripard]

The USB phy controller in the A31 differs mostly from the older controllers
because it has a clock dedicated for each phy, while the older ones were having
a single clock for all the phys.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

drivers/phy/phy-sun4i-usb.c | 35 ++++++++++++++++++++++++++---------
1 file changed, 26 insertions(+), 9 deletions(-)

diff --git a/drivers/phy/phy-sun4i-usb.c b/drivers/phy/phy-sun4i-usb.c

index e6e6c4ba7145..8bd89430d945 100644
--- a/drivers/phy/phy-sun4i-usb.c
+++ b/drivers/phy/phy-sun4i-usb.c
@@ -61,7 +61,6 @@

#define MAX_PHYS 3

struct sun4i_usb_phy_data {
- struct clk *clk;
void __iomem *base;
struct mutex mutex;
int num_phys;

@@ -71,6 +70,7 @@ struct sun4i_usb_phy_data {

void __iomem *pmu;
struct regulator *vbus;
struct reset_control *reset;
+ struct clk *clk;
int index;
} phys[MAX_PHYS];
};

@@ -146,13 +146,13 @@ static int sun4i_usb_phy_init(struct phy *_phy)

struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);
int ret;

- ret = clk_prepare_enable(data->clk);
+ ret = clk_prepare_enable(phy->clk);
if (ret)
return ret;

ret = reset_control_deassert(phy->reset);
if (ret) {
- clk_disable_unprepare(data->clk);
+ clk_disable_unprepare(phy->clk);
return ret;
}

@@ -170,11 +170,10 @@ static int sun4i_usb_phy_init(struct phy *_phy)

static int sun4i_usb_phy_exit(struct phy *_phy)
{
struct sun4i_usb_phy *phy = phy_get_drvdata(_phy);
- struct sun4i_usb_phy_data *data = to_sun4i_usb_phy_data(phy);

sun4i_usb_phy_passby(phy, 0);
reset_control_assert(phy->reset);
- clk_disable_unprepare(data->clk);
+ clk_disable_unprepare(phy->clk);

return 0;
}

@@ -228,8 +227,10 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
struct phy_provider *phy_provider;
struct reset_control *reset;
struct regulator *vbus;
+ bool dedicated_clocks;

struct resource *res;
struct phy *phy;
+ struct clk *clk;
char name[16];
int i;

@@ -249,15 +250,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
else
data->disc_thresh = 2;

+ if (of_device_is_compatible(np, "allwinner,sun6i-a31-usb-phy"))

+ dedicated_clocks = true;

+
res = platform_get_resource_byname(pdev, IORESOURCE_MEM, "phy_ctrl");
data->base = devm_ioremap_resource(dev, res);
if (IS_ERR(data->base))
return PTR_ERR(data->base);

- data->clk = devm_clk_get(dev, "usb_phy");
- if (IS_ERR(data->clk)) {
- dev_err(dev, "could not get usb_phy clock\n");
- return PTR_ERR(data->clk);

+ if (!dedicated_clocks) {

+ clk = devm_clk_get(dev, "usb_phy");
+ if (IS_ERR(clk)) {
+ dev_err(dev, "could not get usb_phy clock\n");
+ return PTR_ERR(clk);
+ }
}

/* Skip 0, 0 is the phy for otg which is not yet supported. */
@@ -270,6 +276,15 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
vbus = NULL;
}

+ if (dedicated_clocks) {

[Maxime Ripard]

Hi everyone,

This patchset adds support for the USB controllers found in the
Allwinner A31.

While the design is similar to the earlier Allwinner SoCs that are
already supported, a few details here and there change, like the fact
that the PHYs now have one clock per phy, while it used to be only one
for all the PHYs.

Thanks,
Maxime

Changes from v1:
- Moved the reset assertion/deassertion to probe/remove
- Moved the dedicated_clocks to the probe function instead of the
private structure since it was the only user

Boris BREZILLON (2):
usb: ehci-platform: add optional reset controller retrieval
ARM: sunxi: dt: add APP4-EVB1 board support

Maxime Ripard (5):
clk: sunxi: Implement A31 USB clock
ARM: sun6i: Add the USB clocks to the DTSI.
phy: usb: sunxi: Introduce Allwinner A31 USB PHY support
usb: ohci-platform: Enable optional use of reset controller
ARM: sun6i: dt: Add support for the USB controllers

Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
Documentation/devicetree/bindings/usb/usb-ohci.txt | 1 +
arch/arm/boot/dts/Makefile | 1 +

arch/arm/boot/dts/sun6i-a31-app4-evb1.dts | 57 ++++++++++++++

arch/arm/boot/dts/sun6i-a31.dtsi | 88 ++++++++++++++++++++++
drivers/clk/sunxi/clk-sunxi.c | 6 ++
drivers/phy/phy-sun4i-usb.c | 35 ++++++---

drivers/usb/host/ehci-platform.c | 18 +++++
drivers/usb/host/ohci-platform.c | 18 +++++
9 files changed, 216 insertions(+), 9 deletions(-)
create mode 100644 arch/arm/boot/dts/sun6i-a31-app4-evb1.dts

--
1.9.1

[Maxime Ripard]

The OHCI controllers used in the Allwinner A31 are asserted in reset using a

global reset controller.

Add optional support for such a controller in the OHCI platform driver.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

Documentation/devicetree/bindings/usb/usb-ohci.txt | 1 +
drivers/usb/host/ohci-platform.c | 18 ++++++++++++++++++
2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ohci.txt b/Documentation/devicetree/bindings/usb/usb-ohci.txt
index 45f67d91e888..b968a1aea995 100644
--- a/Documentation/devicetree/bindings/usb/usb-ohci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ohci.txt
@@ -12,6 +12,7 @@ Optional properties:
- clocks : a list of phandle + clock specifier pairs
- phys : phandle + phy specifier pair
- phy-names : "usb"
+- resets : phandle + reset specifier pair

Example:

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c

index b6002c951c5c..25073112abea 100644

--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -24,6 +24,7 @@
#include <linux/err.h>
#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>
#include <linux/usb/ohci_pdriver.h>
#include <linux/usb.h>
#include <linux/usb/hcd.h>
@@ -36,6 +37,7 @@

struct ohci_platform_priv {
struct clk *clks[OHCI_MAX_CLKS];
+ struct reset_control *rst;
struct phy *phy;
};

@@ -191,6 +193,19 @@ static int ohci_platform_probe(struct platform_device *dev)

break;
}
}
+
+ priv->rst = devm_reset_control_get_optional(&dev->dev,
+ NULL);
+ if (IS_ERR(priv->rst)) {
+ err = PTR_ERR(priv->rst);
+ if (err == -EPROBE_DEFER)
+ goto err_put_clks;
+ priv->rst = NULL;

+ } else {
+ err = reset_control_deassert(priv->rst);
+ if (err)
+ goto err_put_clks;
+ }
}

if (pdata->big_endian_desc)
@@ -266,6 +281,9 @@ static int ohci_platform_remove(struct platform_device *dev)
if (pdata->power_off)
pdata->power_off(dev);

+ if (priv->rst)
+ reset_control_assert(priv->rst);
+

for (clk = 0; clk < OHCI_MAX_CLKS && priv->clks[clk]; clk++)
clk_put(priv->clks[clk]);

--
1.9.1

[Maxime Ripard]

The A31 has two ECHI/OHCI controllers, and one OHCI-only phy-less controller.

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

arch/arm/boot/dts/sun6i-a31.dtsi | 77 ++++++++++++++++++++++++++++++++++++++++
1 file changed, 77 insertions(+)

diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
index 13aa56ed5858..5e9f01af6d99 100644
--- a/arch/arm/boot/dts/sun6i-a31.dtsi
+++ b/arch/arm/boot/dts/sun6i-a31.dtsi

@@ -341,6 +341,83 @@
status = "disabled";
};

+ usbphy: phy@01c19400 {
+ compatible = "allwinner,sun6i-a31-usb-phy";
+ reg = <0x01c19400 0x10>,
+ <0x01c1a800 0x4>,
+ <0x01c1b800 0x4>;
+ reg-names = "phy_ctrl",
+ "pmu1",
+ "pmu2";
+ clocks = <&usb_clk 8>,
+ <&usb_clk 9>,
+ <&usb_clk 10>;
+ clock-names = "usb0_phy",
+ "usb1_phy",
+ "usb2_phy";
+ resets = <&usb_clk 0>,
+ <&usb_clk 1>,
+ <&usb_clk 2>;
+ reset-names = "usb0_reset",
+ "usb1_reset",
+ "usb2_reset";
+ status = "disabled";
+ #phy-cells = <1>;

+ };
+
+ ehci0: usb@01c1a000 {

[Maxime Ripard]

From: Boris BREZILLON <boris.b...@free-electrons.com>

On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to
be deasserted for the EHCI block to be usable.

Add support for an optional reset controller that will be deasserted on
power off and asserted on power on.

Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>

Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

Documentation/devicetree/bindings/usb/usb-ehci.txt | 1 +
drivers/usb/host/ehci-platform.c | 18 ++++++++++++++++++

2 files changed, 19 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/usb-ehci.txt b/Documentation/devicetree/bindings/usb/usb-ehci.txt
index ff151ec084c4..43c1a4e06767 100644
--- a/Documentation/devicetree/bindings/usb/usb-ehci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-ehci.txt

@@ -15,6 +15,7 @@ Optional properties:

- clocks : a list of phandle + clock specifier pairs
- phys : phandle + phy specifier pair
- phy-names : "usb"

+ - resets : phandle + reset specifier pair

Example (Sequoia 440EPx):
ehci@e0000300 {
diff --git a/drivers/usb/host/ehci-platform.c b/drivers/usb/host/ehci-platform.c

index c7dd93aad20c..da89f274aa76 100644

--- a/drivers/usb/host/ehci-platform.c
+++ b/drivers/usb/host/ehci-platform.c
@@ -29,6 +29,7 @@
#include <linux/of.h>

#include <linux/phy/phy.h>
#include <linux/platform_device.h>
+#include <linux/reset.h>

#include <linux/usb.h>
#include <linux/usb/hcd.h>
#include <linux/usb/ehci_pdriver.h>
@@ -41,6 +42,7 @@

struct ehci_platform_priv {
struct clk *clks[EHCI_MAX_CLKS];

+ struct reset_control *rst;
struct phy *phy;
};

@@ -206,6 +208,19 @@ static int ehci_platform_probe(struct platform_device *dev)

break;
}
}
+
+ priv->rst = devm_reset_control_get_optional(&dev->dev,
+ NULL);
+ if (IS_ERR(priv->rst)) {
+ err = PTR_ERR(priv->rst);
+ if (err == -EPROBE_DEFER)
+ goto err_put_clks;
+ priv->rst = NULL;
+ } else {
+ err = reset_control_deassert(priv->rst);
+ if (err)
+ goto err_put_clks;
+ }
}

if (pdata->big_endian_desc)

@@ -280,6 +295,9 @@ static int ehci_platform_remove(struct platform_device *dev)

if (pdata->power_off)
pdata->power_off(dev);

+ if (priv->rst)
+ reset_control_assert(priv->rst);
+

for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++)

[Maxime Ripard]

From: Boris BREZILLON <boris.b...@free-electrons.com>

The APP4 EVB1 development boards embeds an A31, together with some NAND, one SD
card slot, and one SDIO + UART WiFi and Bluetooth chip, a few I2C buses, USB,
and a LCD display.

Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>
Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
Reviewed-by: Hans de Goede <hdeg...@redhat.com>
---

arch/arm/boot/dts/Makefile | 1 +
arch/arm/boot/dts/sun6i-a31-app4-evb1.dts | 57 +++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)

create mode 100644 arch/arm/boot/dts/sun6i-a31-app4-evb1.dts

diff --git a/arch/arm/boot/dts/Makefile b/arch/arm/boot/dts/Makefile
index ffa3f5ef27d3..d50c0895a9d5 100644
--- a/arch/arm/boot/dts/Makefile
+++ b/arch/arm/boot/dts/Makefile
@@ -343,6 +343,7 @@ dtb-$(CONFIG_ARCH_SUNXI) += \
sun5i-a10s-olinuxino-micro.dtb \
sun5i-a13-olinuxino.dtb \
sun5i-a13-olinuxino-micro.dtb \
+ sun6i-a31-app4-evb1.dtb \
sun6i-a31-colombus.dtb \
sun6i-a31-m9.dtb \
sun7i-a20-cubieboard2.dtb \
diff --git a/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts b/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts
new file mode 100644

index 000000000000..2bbf8867362b
--- /dev/null
+++ b/arch/arm/boot/dts/sun6i-a31-app4-evb1.dts
@@ -0,0 +1,57 @@

+/*
+ * Copyright 2014 Boris Brezillon
+ *
+ * Boris Brezillon <boris.b...@free-electrons.com>
+ *
+ * The code contained herein is licensed under the GNU General Public
+ * License. You may obtain a copy of the GNU General Public License
+ * Version 2 or later at the following locations:

+ *
+ * http://www.opensource.org/licenses/gpl-license.html
+ * http://www.gnu.org/copyleft/gpl.html
+ */
+
+/dts-v1/;
+/include/ "sun6i-a31.dtsi"

+/include/ "sunxi-common-regulators.dtsi"

+
+/ {
+ model = "Allwinner A31 APP4 EVB1 Evaluation Board";
+ compatible = "allwinner,app4-evb1", "allwinner,sun6i-a31";
+
+ chosen {
+ bootargs = "earlyprintk console=ttyS0,115200";
+ };
+
+ soc@01c00000 {
+ pio: pinctrl@01c20800 {
+ usb1_vbus_pin_a: usb1_vbus_pin@0 {
+ allwinner,pins = "PH27";
+ allwinner,function = "gpio_out";
+ allwinner,drive = <0>;
+ allwinner,pull = <0>;
+ };
+ };
+
+ usbphy: phy@01c19400 {
+ usb1_vbus-supply = <&reg_usb1_vbus>;
+ status = "okay";

+ };
+
+ ehci0: usb@01c1a000 {

+ status = "okay";
+ };
+
+ uart0: serial@01c28000 {
+ pinctrl-names = "default";
+ pinctrl-0 = <&uart0_pins_a>;
+ status = "okay";
+ };
+ };
+
+ reg_usb1_vbus: usb1-vbus {

+ pinctrl-0 = <&usb1_vbus_pin_a>;

+ gpio = <&pio 7 27 0>;
+ status = "okay";
+ };

+};
--
1.9.1

[Alan Stern]

On Sat, 10 May 2014, Maxime Ripard wrote:

> From: Boris BREZILLON <boris.b...@free-electrons.com>
>
> On the Allwinner's A31 SoC the reset line connected to the EHCI IP has to
> be deasserted for the EHCI block to be usable.
>
> Add support for an optional reset controller that will be deasserted on
> power off and asserted on power on.
>
> Signed-off-by: Boris BREZILLON <boris.b...@free-electrons.com>
> Signed-off-by: Maxime Ripard <maxime...@free-electrons.com>
> Reviewed-by: Hans de Goede <hdeg...@redhat.com>

This basically is good. fine. I have only two comments, and one of
them is a matter of taste rather than substance.

> @@ -206,6 +208,19 @@ static int ehci_platform_probe(struct platform_device *dev)
> break;
> }
> }
> +
> + priv->rst = devm_reset_control_get_optional(&dev->dev,
> + NULL);

I hate the style that matches arguments on a continuation line with the
opening paren of the function call, for a couple of reasons. Instead I
simply indent continuation lines by two or more tab stops. But some
people seem to be incurably attached to it.

> + if (IS_ERR(priv->rst)) {
> + err = PTR_ERR(priv->rst);
> + if (err == -EPROBE_DEFER)
> + goto err_put_clks;
> + priv->rst = NULL;
> + } else {
> + err = reset_control_deassert(priv->rst);
> + if (err)
> + goto err_put_clks;
> + }
> }
>
> if (pdata->big_endian_desc)

The new code was added inside an "if" statement, which will cause it to
apply only to OF devices. Is there any reason not to put the new code
outside the "if" statement, so it applies to all devices?

Alan Stern

[Chen-Yu Tsai]

Hi,

No default?

> struct resource *res;
> struct phy *phy;
> + struct clk *clk;
> char name[16];
> int i;
>
> @@ -249,15 +250,20 @@ static int sun4i_usb_phy_probe(struct platform_device *pdev)
> else
> data->disc_thresh = 2;
>
> + if (of_device_is_compatible(np, "allwinner,sun6i-a31-usb-phy"))
> + dedicated_clocks = true;
> +

dedicated_clocks is more than likely to be true from leftover data
on the stack. This results in the usb phy driver probe failing, and
the usb host drivers left in perpetual probe deferral on my Cubietruck.

Adding an else section fixes this.

Cheers
ChenYu

[Hans de Goede]

Hi,

Good catch, fixed this in the sunxi-devel branch for now (until Maxime sends a new version).

Regards,

Hans

[Kishon Vijay Abraham I]

Hi,

so I wont be taking this patch in my tree. Anyway FWIW
Acked-by: Kishon Vijay Abraham I <kis...@ti.com>
>
> Regards,
>
> Hans
>

[Hans de Goede]

Hi,

To be clear, the sunxi-devel branch is a testing branch where I collect all the
pending sunxi related patches, it is not a way for patches to go upstream.

I don't know what the exact upstreaming plan for this patch-set is,
but logically this patch (once a fixed version is resend) should be going
upstream through you.

Regards,

Hans

[Maxime Ripard]

Hi Alan,

On Sat, May 10, 2014 at 10:35:49AM -0400, Alan Stern wrote:
> > @@ -206,6 +208,19 @@ static int ehci_platform_probe(struct platform_device *dev)
> > break;
> > }
> > }
> > +
> > + priv->rst = devm_reset_control_get_optional(&dev->dev,
> > + NULL);
>
> I hate the style that matches arguments on a continuation line with the
> opening paren of the function call, for a couple of reasons. Instead I
> simply indent continuation lines by two or more tab stops. But some
> people seem to be incurably attached to it.

Ok, I'll change it.

> > + if (IS_ERR(priv->rst)) {
> > + err = PTR_ERR(priv->rst);
> > + if (err == -EPROBE_DEFER)
> > + goto err_put_clks;
> > + priv->rst = NULL;
> > + } else {
> > + err = reset_control_deassert(priv->rst);
> > + if (err)
> > + goto err_put_clks;
> > + }
> > }
> >
> > if (pdata->big_endian_desc)
>
> The new code was added inside an "if" statement, which will cause it to
> apply only to OF devices. Is there any reason not to put the new code
> outside the "if" statement, so it applies to all devices?

Hmmm, I did this because so far, the reset framework only handles the
DT case. But that's right that it can be extended in the future, I'll
update it.

[Maxime Ripard]

Oh, yes. Stupid me. It used to be in a kzalloc'd section, so I didn't
have the error, and didn't properly test it because it was trivial.

Thanks for catching this, I'll submit a v3.