Skip to contentSkip to conversation

?hci-platform: Add support for controllers with more then one reset line

PATCH Superseded Hans de Goede 4 participants 8 messages Feb 24, 2016

Subsystems (4)
Device tree bindings USB USB ehci driver USB ohci driver

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Hans de Goede

author
#0 lore raw

v3 cover ?hci-platform: Add support for controllers with more then one reset line

Hi All,

Here is a new version of my patch-set to support usb controllers which have multiple resets. These patches apply on top of the related reset-controller patches which have just been merged here:

git://git.pengutronix.de/git/pza/linux.git reset/next

Changes in v2:
-Switch to now shared reset_[de]assert functions

Changes in v3:
-Adjust for changes to shared-reset reset-controller functions

Regards,

Hans

author
#1 lore raw

PATCH v3 1/2 ehci-platform: Add support for controllers with multiple reset lines

From: Reinder de Haan <[email protected]>

At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple reset lines, the controller will not initialize while the reset for its companion is still asserted, which means we need to de-assert 2 resets for the controller to work.

Signed-off-by Reinder de Haan <[email protected]>
Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Use the new reset_control_[de]assert_shared reset-controller functions Changes in v3:
-Adjust for changes to shared-reset reset-controller functions

---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  2 +-
 drivers/usb/host/ehci-platform.c                   | 41 ++++++++++++----------
 2 files changed, 24 insertions(+), 19 deletions(-)
Documentation/devicetree/bindings/usb/usb-ehci.txt +1 -1
index a12d601..0701812 100644
@@ -18,7 +18,7 @@ Optional properties:
  - clocks : a list of phandle + clock specifier pairs
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
- - resets : phandle + reset specifier pair
+ - resets : a list of phandle + reset specifier pairs
 
 Example (Sequoia 440EPx):
     ehci at e0000300 {
drivers/usb/host/ehci-platform.c +23 -18

82 lines not on this page · raw message

Signature · 1 line
2.7.1
build bot
#2 lore raw

[PATCH v3 1/2] ehci-platform: Add support for controllers with multiple reset lines

reproduce:

Hi Reinder,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc5 next-20160223]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/ehci-platform-Add-support-for-controllers-with-multiple-reset-lines/20160224-191812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: x86_64-randconfig-x004-201608 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

drivers/usb/host/ehci-platform.c: In function 'ehci_platform_probe':

>> drivers/usb/host/ehci-platform.c:239:5: error: implicit declaration of function 'devm_reset_control_get_shared_by_index' [-Werror=implicit-function-declaration]
        devm_reset_control_get_shared_by_index(
        ^
>> drivers/usb/host/ehci-platform.c:238:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
       priv->resets[rst] =
                         ^

cc1: some warnings being treated as errors

vim +/devm_reset_control_get_shared_by_index +239 drivers/usb/host/ehci-platform.c

232 priv->clks[clk] = NULL;
233 break;
234 }
235 }
236
237 for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {

 > 238				priv->resets[rst] =
 > 239					devm_reset_control_get_shared_by_index(

240 &dev->dev, rst);
241 if (IS_ERR(priv->resets[rst])) {
242 err = PTR_ERR(priv->resets[rst]);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 24241 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160224/733e3be9/attachment-0001.obj>

#3 lore raw

PATCH v3 1/2 ehci-platform: Add support for controllers with multiple reset lines

In reply to Hans de Goede

Hi,

Quoted 80 lines · Hans de Goede

From: Reinder de Haan <[email protected]>

At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple reset lines, the controller will not initialize while the reset for its companion is still asserted, which means we need to de-assert 2 resets for the controller to work.

Signed-off-by Reinder de Haan <[email protected]>
Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Use the new reset_control_[de]assert_shared reset-controller functions Changes in v3:

(quoted diff) +0 -2
-Adjust for changes to shared-reset reset-controller functions
---
 Documentation/devicetree/bindings/usb/usb-ehci.txt |  2 +-
 drivers/usb/host/ehci-platform.c                   | 41 ++++++++++++----------
 2 files changed, 24 insertions(+), 19 deletions(-)
Documentation/devicetree/bindings/usb/usb-ehci.txt +1 -1
index a12d601..0701812 100644
@@ -18,7 +18,7 @@ Optional properties:
  - clocks : a list of phandle + clock specifier pairs
  - phys : phandle + phy specifier pair
  - phy-names : "usb"
- - resets : phandle + reset specifier pair
+ - resets : a list of phandle + reset specifier pairs
 
 Example (Sequoia 440EPx):
     ehci at e0000300 {
drivers/usb/host/ehci-platform.c +8 -13
index bd7082f2..3f195ba 100644
@@ -39,11 +39,12 @@
 
 #define DRIVER_DESC "EHCI generic platform driver"
 #define EHCI_MAX_CLKS 3
+#define EHCI_MAX_RESETS 2
 #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
 
 struct ehci_platform_priv {
 	struct clk *clks[EHCI_MAX_CLKS];
-	struct reset_control *rst;
+	struct reset_control *resets[EHCI_MAX_RESETS];
 	struct phy **phys;
 	int num_phys;
 	bool reset_on_resume;

@@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev)
 	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ehci_platform_priv *priv;
 	struct ehci_hcd *ehci;
-	int err, irq, phy_num, clk = 0;
+	int err, irq, phy_num, clk = 0, rst = 0;
 
 	if (usb_disabled())
 		return -ENODEV;

@@ -232,18 +233,22 @@ 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;
+		for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {
+			priv->resets[rst] =
+				devm_reset_control_get_shared_by_index(
+								&dev->dev, rst);
+			if (IS_ERR(priv->resets[rst])) {

drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 251

Quoted 10 lines
drivers/usb/host/ehci-platform.c +10 -0
+				err = PTR_ERR(priv->resets[rst]);
+				if (err == -EPROBE_DEFER)
+					goto err_reset;
+				priv->resets[rst] = NULL;
+				break;
+			}
+			err = reset_control_deassert(priv->resets[rst]);
+			if (err)
+				goto err_reset;
+		}

Why is this code within "if (dev->of.node)" ? Previous code looked like it worked for non-DT boots as well.

Quoted 24 lines
drivers/usb/host/ehci-platform.c +3 -3
 	}
 
 	if (pdata->big_endian_desc)

@@ -300,8 +305,8 @@ err_power:
 	if (pdata->power_off)
 		pdata->power_off(dev);
 err_reset:
-	if (priv->rst)
-		reset_control_assert(priv->rst);
+	while (--rst >= 0)
+		reset_control_assert(priv->resets[rst]);
 err_put_clks:
 	while (--clk >= 0)
 		clk_put(priv->clks[clk]);

@@ -319,15 +324,15 @@ static int ehci_platform_remove(struct platform_device *dev)
 	struct usb_hcd *hcd = platform_get_drvdata(dev);
 	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
 	struct ehci_platform_priv *priv = hcd_to_ehci_priv(hcd);
-	int clk;
+	int clk, rst;
 
 	usb_remove_hcd(hcd);

drivers/usb/host/ehci-platform.c static int ehci_platform_remove(struct platform_device *dev) · line 338

Quoted 10 lines
drivers/usb/host/ehci-platform.c +2 -2
 	if (pdata->power_off)
 		pdata->power_off(dev);
 
-	if (priv->rst)
-		reset_control_assert(priv->rst);
+	for (rst = 0; rst < EHCI_MAX_RESETS && priv->resets[rst]; rst++)
+		reset_control_assert(priv->resets[rst]);
 
 	for (clk = 0; clk < EHCI_MAX_CLKS && priv->clks[clk]; clk++)
 		clk_put(priv->clks[clk]);

cheers,
-roger

author
#4 lore raw

PATCH v3 1/2 ehci-platform: Add support for controllers with multiple reset lines

Hi,

Quoted 88 lines · Roger Quadros

Hi,

Quoted 85 lines · Hans de Goede

From: Reinder de Haan <[email protected]>

At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple reset lines, the controller will not initialize while the reset for its companion is still asserted, which means we need to de-assert 2 resets for the controller to work.

Signed-off-by Reinder de Haan <[email protected]>
Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-Use the new reset_control_[de]assert_shared reset-controller functions Changes in v3:

(quoted diff) +0 -2
-Adjust for changes to shared-reset reset-controller functions
---
  Documentation/devicetree/bindings/usb/usb-ehci.txt |  2 +-
  drivers/usb/host/ehci-platform.c                   | 41 ++++++++++++----------
  2 files changed, 24 insertions(+), 19 deletions(-)
Documentation/devicetree/bindings/usb/usb-ehci.txt +1 -1
index a12d601..0701812 100644
@@ -18,7 +18,7 @@ Optional properties:
   - clocks : a list of phandle + clock specifier pairs
   - phys : phandle + phy specifier pair
   - phy-names : "usb"
- - resets : phandle + reset specifier pair
+ - resets : a list of phandle + reset specifier pairs
 
  Example (Sequoia 440EPx):
      ehci at e0000300 {
drivers/usb/host/ehci-platform.c +13 -13
index bd7082f2..3f195ba 100644
@@ -39,11 +39,12 @@
 
  #define DRIVER_DESC "EHCI generic platform driver"
  #define EHCI_MAX_CLKS 3
+#define EHCI_MAX_RESETS 2
  #define hcd_to_ehci_priv(h) ((struct ehci_platform_priv *)hcd_to_ehci(h)->priv)
 
  struct ehci_platform_priv {
  	struct clk *clks[EHCI_MAX_CLKS];
-	struct reset_control *rst;
+	struct reset_control *resets[EHCI_MAX_RESETS];
  	struct phy **phys;
  	int num_phys;
  	bool reset_on_resume;

@@ -149,7 +150,7 @@ static int ehci_platform_probe(struct platform_device *dev)
  	struct usb_ehci_pdata *pdata = dev_get_platdata(&dev->dev);
  	struct ehci_platform_priv *priv;
  	struct ehci_hcd *ehci;
-	int err, irq, phy_num, clk = 0;
+	int err, irq, phy_num, clk = 0, rst = 0;
 
  	if (usb_disabled())
  		return -ENODEV;

@@ -232,18 +233,22 @@ 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;
+		for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {
+			priv->resets[rst] =
+				devm_reset_control_get_shared_by_index(
+								&dev->dev, rst);
+			if (IS_ERR(priv->resets[rst])) {
+				err = PTR_ERR(priv->resets[rst]);
+				if (err == -EPROBE_DEFER)
+					goto err_reset;
+				priv->resets[rst] = NULL;
+				break;
Quoted 8 lines

drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 251

Quoted 5 lines
drivers/usb/host/ehci-platform.c +5 -0
+			}
+			err = reset_control_deassert(priv->resets[rst]);
+			if (err)
+				goto err_reset;
+		}

Why is this code within "if (dev->of.node)" ? Previous code looked like it worked for non-DT boots as well.

Because devm_reset_control_get_shared_by_index() only works for DT platforms (it is safe to call on non DT platforms, it will just always return -EINVAL there) and the old reset paths were ever only used / only ever worked with DT too.

Regards,

Hans

maintainer
#5 lore raw

PATCH v3 1/2 ehci-platform: Add support for controllers with multiple reset lines

In reply to Roger Quadros
Quoted 13 lines · Roger Quadros

Hi,

Quoted 9 lines · Hans de Goede

From: Reinder de Haan <[email protected]>

At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple reset lines, the controller will not initialize while the reset for its companion is still asserted, which means we need to de-assert 2 resets for the controller to work.

Signed-off-by Reinder de Haan <[email protected]>
Signed-off-by Hans de Goede <[email protected]>
Quoted 8 lines

drivers/usb/host/ehci-platform.c static int ehci_platform_probe(struct platform_device *dev) · line 240

Quoted 8 lines
drivers/usb/host/ehci-platform.c +4 -4
-	} else {
-		err = reset_control_deassert(priv->rst);
-		if (err)
-			goto err_put_clks;
+		for (rst = 0; rst < EHCI_MAX_RESETS; rst++) {
+			priv->resets[rst] =
+				devm_reset_control_get_shared_by_index(
+								&dev->dev, rst);

Ugly continuation line. The convention in these files is to indent continuation lines by two tab stops beyond the original source line. Same comment applies to the 2/2 patch.

Once that is changed, you can add:

Acked-by Alan Stern <[email protected]>

to both.

Alan Stern

author
#6 lore raw

PATCH v3 2/2 ohci-platform: Add support for controllers with multiple reset lines

In reply to Hans de Goede

At least the EHCI/OHCI found on the Allwinnner H3 SoC needs multiple reset lines, the controller will not initialize while the reset for its companion is still asserted, which means we need to de-assert 2 resets for the controller to work.

Signed-off-by Hans de Goede <[email protected]>

---
Changes in v2:
-New patch in v2 of this patch-set, to complement the identical patch for
the ehci-platform code
Changes in v3:
-Adjust for changes to shared-reset reset-controller functions

---
 Documentation/devicetree/bindings/usb/usb-ohci.txt |  2 +-
 drivers/usb/host/ohci-platform.c                   | 43 ++++++++++++----------
 2 files changed, 24 insertions(+), 21 deletions(-)
Documentation/devicetree/bindings/usb/usb-ohci.txt +1 -1
index 19233b7..9df4569 100644
@@ -14,7 +14,7 @@ Optional properties:
 - clocks : a list of phandle + clock specifier pairs
 - phys : phandle + phy specifier pair
 - phy-names : "usb"
-- resets : phandle + reset specifier pair
+- resets : a list of phandle + reset specifier pairs
 
 Example:
drivers/usb/host/ohci-platform.c +23 -20

83 lines not on this page · raw message

Signature · 1 line
2.7.1
build bot
#7 lore raw

[PATCH v3 2/2] ohci-platform: Add support for controllers with multiple reset lines

reproduce:

Hi Hans,

[auto build test ERROR on robh/for-next]
[also build test ERROR on v4.5-rc5 next-20160223]
[if your patch is applied to the wrong git tree, please drop us a note to help improving the system]

url:    https://github.com/0day-ci/linux/commits/Hans-de-Goede/ehci-platform-Add-support-for-controllers-with-multiple-reset-lines/20160224-191812
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux for-next
config: x86_64-randconfig-x004-201608 (attached as .config)
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64

All error/warnings (new ones prefixed by >>):

drivers/usb/host/ohci-platform.c: In function 'ohci_platform_probe':

>> drivers/usb/host/ohci-platform.c:201:5: error: implicit declaration of function 'devm_reset_control_get_shared_by_index' [-Werror=implicit-function-declaration]
        devm_reset_control_get_shared_by_index(
        ^
>> drivers/usb/host/ohci-platform.c:200:22: warning: assignment makes pointer from integer without a cast [-Wint-conversion]
       priv->resets[rst] =
                         ^

cc1: some warnings being treated as errors

vim +/devm_reset_control_get_shared_by_index +201 drivers/usb/host/ohci-platform.c

194 goto err_put_clks;
195 priv->clks[clk] = NULL;
196 break;
197 }
198 }
199 for (rst = 0; rst < OHCI_MAX_RESETS; rst++) {

 > 200				priv->resets[rst] =
 > 201					devm_reset_control_get_shared_by_index(

202 &dev->dev, rst);
203 if (IS_ERR(priv->resets[rst])) {
204 err = PTR_ERR(priv->resets[rst]);

---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
-------------- next part --------------
A non-text attachment was scrubbed...
Name: .config.gz
Type: application/octet-stream
Size: 24241 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160224/c3f43aad/attachment-0001.obj>