linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's
@ 2018-11-05 21:31 Al Cooper
  2018-11-05 21:31 ` [PATCH V3 1/6] dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document Al Cooper
                   ` (5 more replies)
  0 siblings, 6 replies; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

V3 - Based on feedback
   - Patch 3/6
     In xhci-brcm.h, fixed incorrect function name when
     CONFIG_USB_XHCI_BRCM is not defined. Found by kbuild test robot.
   - Patch 4/6
     In ohci-platform.c, use devm_kmemdup() instead of demv_kzalloc().
   - Patch 5/6
     In ehci-platform.c, use dma_coerce_mask_and_coherent()
     instead of dma_coerce_mask_and_coherent(). Remove unneeded
     #ifdef CONFIG_OF.

V2 - Based on feedback, the functionality for XHCI and OHCI was
     moved from Broadcom platform drivers into the standard XHCI
     and OHCI platform drivers. The EHCI functionality still uses
     a Broadcom EHCI driver because of the workarounds needed for
     bugs in the EHCI controller.

This adds support for the XHCI, EHCI and OHCI host controllers found
in Broadcom STB SoC's. These drivers depend on getting access to the
new Broadcom STB USB PHY driver through a device-tree phandle and
will fail if the driver is not available.

Al Cooper (6):
  dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document
  usb: core: Add ability to skip phy exit on suspend and init on resume
  usb: xhci: xhci-plat: Add support for Broadcom STB SoC's
  usb: ohci-platform: Add support for Broadcom STB SoC's
  usb: ehci: Add new EHCI driver for Broadcom STB SoC's
  usb: host: Add ability to build new Broadcom STB USB drivers

 .../devicetree/bindings/usb/brcm,bcm7445-ehci.txt  |  22 ++
 .../devicetree/bindings/usb/brcm,bcm7445-ohci.txt  |  22 ++
 .../devicetree/bindings/usb/brcm,bcm7445-xhci.txt  |  23 ++
 MAINTAINERS                                        |   9 +
 drivers/usb/core/hcd.c                             |   8 +-
 drivers/usb/core/phy.c                             |  18 +-
 drivers/usb/core/phy.h                             |   9 +-
 drivers/usb/host/Kconfig                           |  29 +++
 drivers/usb/host/Makefile                          |  18 +-
 drivers/usb/host/ehci-brcm.c                       | 287 +++++++++++++++++++++
 drivers/usb/host/ohci-platform.c                   |  34 ++-
 drivers/usb/host/xhci-brcm.c                       |  17 ++
 drivers/usb/host/xhci-brcm.h                       |  16 ++
 drivers/usb/host/xhci-plat.c                       |   8 +
 include/linux/usb/hcd.h                            |   3 +
 include/linux/usb/ohci_pdriver.h                   |   1 +
 16 files changed, 499 insertions(+), 25 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.txt
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ohci.txt
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-xhci.txt
 create mode 100644 drivers/usb/host/ehci-brcm.c
 create mode 100644 drivers/usb/host/xhci-brcm.c
 create mode 100644 drivers/usb/host/xhci-brcm.h

-- 
1.9.0.138.g2de3478


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

* [PATCH V3 1/6] dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-05 21:31 ` [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume Al Cooper
                   ` (4 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add DT bindings document for Broadcom STB USB OHCI, EHCI and
XHCI drivers.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 .../devicetree/bindings/usb/brcm,bcm7445-ehci.txt  | 22 +++++++++++++++++++++
 .../devicetree/bindings/usb/brcm,bcm7445-ohci.txt  | 22 +++++++++++++++++++++
 .../devicetree/bindings/usb/brcm,bcm7445-xhci.txt  | 23 ++++++++++++++++++++++
 3 files changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.txt
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-ohci.txt
 create mode 100644 Documentation/devicetree/bindings/usb/brcm,bcm7445-xhci.txt

diff --git a/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.txt b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.txt
new file mode 100644
index 000000000000..824f4bf5d07c
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ehci.txt
@@ -0,0 +1,22 @@
+Broadcom STB USB EHCI controller
+
+Required properties:
+- compatible: should be "brcm,bcm7445-ehci"
+- reg: should contain one register range i.e. start and length
+- interrupts: description of the interrupt line
+- phys: phandle + phy specifier pair
+  The specifier should be 0 for the OHCI/EHCI PHY
+
+Optional properties:
+- clocks: A phandle for the EHCI clock
+
+Example:
+
+ehci@f0b00300 {
+	compatible = "brcm,bcm7445-ehci";
+	reg = <0xf0b00300 0xa8>;
+	interrupts = <0x0 0x5a 0x0>;
+	interrupt-names = "usb0_ehci_0";
+	phys = <&usbphy_0 0x0>;
+	clocks = <&usb20>
+};
diff --git a/Documentation/devicetree/bindings/usb/brcm,bcm7445-ohci.txt b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ohci.txt
new file mode 100644
index 000000000000..de5a22b3d138
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/brcm,bcm7445-ohci.txt
@@ -0,0 +1,22 @@
+Broadcom STB USB OHCI controller
+
+Required properties:
+- compatible: should be "brcm,bcm7445-ohci"
+- reg: should contain one register range i.e. start and length
+- interrupts: description of the interrupt line
+- phys: phandle + phy specifier pair
+  The specifier should be 0 for the OHCI/EHCI PHY
+
+Optional properties:
+- clocks: A phandle for the OHCI clock
+
+Example:
+
+ohci@f0b00400 {
+	compatible = "brcm,bcm7445-ohci";
+	reg = <0xf0b00400 0x58>;
+	interrupts = <0x0 0x5b 0x0>;
+	interrupt-names = "usb0_ohci_0";
+	phys = <&usbphy_0 0x0>;
+	clocks = <&usb20>;
+};
diff --git a/Documentation/devicetree/bindings/usb/brcm,bcm7445-xhci.txt b/Documentation/devicetree/bindings/usb/brcm,bcm7445-xhci.txt
new file mode 100644
index 000000000000..c41f3f8d836d
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/brcm,bcm7445-xhci.txt
@@ -0,0 +1,23 @@
+Broadcom STB USB XHCI controller
+
+Required properties:
+- compatible: should be "brcm,bcm7445-xhci"
+- reg: should contain one register range i.e. start and length
+- interrupts: description of the interrupt line
+- phys: phandle + phy specifier pair
+  The specifier should be 1 for the XHCI PHY
+
+Optional properties:
+- clocks: A phandle for the XHCI clock
+- usb3-lpm-capable: determines if platform is USB3 LPM capable
+
+Example:
+
+xhci_0_0: xhci@f0b01000 {
+	compatible = "brcm,bcm7445-xhci";
+	reg = <0xf0b01000 0x1000>;
+	interrupts = <0x0 0x5c 0x0>;
+	interrupt-names = "usb0_xhci_0";
+	phys = <&usbphy_0 0x1>;
+	clocks = <&usb30>;
+};
-- 
1.9.0.138.g2de3478


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

* [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
  2018-11-05 21:31 ` [PATCH V3 1/6] dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-05 21:31 ` [PATCH V3 3/6] usb: xhci: xhci-plat: Add support for Broadcom STB SoC's Al Cooper
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add the ability to skip calling the PHY's exit routine on suspend
and the PHY's init routine on resume. This is to handle a USB PHY
that should have it's power_off function called on suspend but cannot
have it's exit function called because on exit it will disable the
PHY to the point where register accesses to the Host Controllers
using the PHY will be disabled and the host drivers will crash.

This is enabled with the HCD flag "suspend_without_phy_exit" which
can be set from any HCD driver.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/core/hcd.c  |  8 ++++----
 drivers/usb/core/phy.c  | 18 ++++++++++++------
 drivers/usb/core/phy.h  |  9 ++++++---
 include/linux/usb/hcd.h |  3 +++
 4 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 487025d31d44..5ebc82a69403 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2249,7 +2249,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, pm_message_t msg)
 		hcd->state = HC_STATE_SUSPENDED;
 
 		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_suspend(hcd->self.sysdev,
+			usb_phy_roothub_suspend(hcd,
 						hcd->phy_roothub);
 
 		/* Did we race with a root-hub wakeup event? */
@@ -2290,7 +2290,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 	}
 
 	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_resume(hcd->self.sysdev,
+		status = usb_phy_roothub_resume(hcd,
 						hcd->phy_roothub);
 		if (status)
 			return status;
@@ -2333,7 +2333,7 @@ int hcd_bus_resume(struct usb_device *rhdev, pm_message_t msg)
 		}
 	} else {
 		hcd->state = old_state;
-		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd, hcd->phy_roothub);
 		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
 				"resume", status);
 		if (status != -ESHUTDOWN)
@@ -2730,7 +2730,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
 	struct usb_device *rhdev;
 
 	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd);
 		if (IS_ERR(hcd->phy_roothub))
 			return PTR_ERR(hcd->phy_roothub);
 
diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 38b2c776c4b4..308d716304da 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -46,10 +46,11 @@ static int usb_phy_roothub_add_phy(struct device *dev, int index,
 	return 0;
 }
 
-struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd)
 {
 	struct usb_phy_roothub *phy_roothub;
 	int i, num_phys, err;
+	struct device *dev = hcd->self.sysdev;
 
 	if (!IS_ENABLED(CONFIG_GENERIC_PHY))
 		return NULL;
@@ -162,26 +163,30 @@ void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub)
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
 
-int usb_phy_roothub_suspend(struct device *controller_dev,
+int usb_phy_roothub_suspend(struct usb_hcd *hcd,
 			    struct usb_phy_roothub *phy_roothub)
 {
+	struct device *controller_dev = hcd->self.sysdev;
+
 	usb_phy_roothub_power_off(phy_roothub);
 
 	/* keep the PHYs initialized so the device can wake up the system */
-	if (device_may_wakeup(controller_dev))
+	if (device_may_wakeup(controller_dev) || hcd->suspend_without_phy_exit)
 		return 0;
 
 	return usb_phy_roothub_exit(phy_roothub);
 }
 EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
 
-int usb_phy_roothub_resume(struct device *controller_dev,
+int usb_phy_roothub_resume(struct usb_hcd *hcd,
 			   struct usb_phy_roothub *phy_roothub)
 {
+	struct device *controller_dev = hcd->self.sysdev;
 	int err;
 
 	/* if the device can't wake up the system _exit was called */
-	if (!device_may_wakeup(controller_dev)) {
+	if (!device_may_wakeup(controller_dev) &&
+	    !hcd->suspend_without_phy_exit) {
 		err = usb_phy_roothub_init(phy_roothub);
 		if (err)
 			return err;
@@ -190,7 +195,8 @@ int usb_phy_roothub_resume(struct device *controller_dev,
 	err = usb_phy_roothub_power_on(phy_roothub);
 
 	/* undo _init if _power_on failed */
-	if (err && !device_may_wakeup(controller_dev))
+	if (err && !device_may_wakeup(controller_dev)
+	    && !hcd->suspend_without_phy_exit)
 		usb_phy_roothub_exit(phy_roothub);
 
 	return err;
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 88a3c037e9df..34293e11a917 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -5,13 +5,16 @@
  * Copyright (C) 2018 Martin Blumenstingl <martin.blumenstingl@googlemail.com>
  */
 
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
 #ifndef __USB_CORE_PHY_H_
 #define __USB_CORE_PHY_H_
 
 struct device;
 struct usb_phy_roothub;
 
-struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd);
 
 int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
 int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
@@ -19,9 +22,9 @@
 int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
 void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
 
-int usb_phy_roothub_suspend(struct device *controller_dev,
+int usb_phy_roothub_suspend(struct usb_hcd *hcd,
 			    struct usb_phy_roothub *phy_roothub);
-int usb_phy_roothub_resume(struct device *controller_dev,
+int usb_phy_roothub_resume(struct usb_hcd *hcd,
 			   struct usb_phy_roothub *phy_roothub);
 
 #endif /* __USB_CORE_PHY_H_ */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 97e2ddec18b1..87a104055b5e 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -157,6 +157,9 @@ struct usb_hcd {
 	 */
 	unsigned		skip_phy_initialization:1;
 
+	/* Some phys don't want the phy's exit/init called on suspend/resume */
+	unsigned		suspend_without_phy_exit:1;
+
 	/* The next flag is a stopgap, to be removed when all the HCDs
 	 * support the new root-hub polling mechanism. */
 	unsigned		uses_new_polling:1;
-- 
1.9.0.138.g2de3478


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

* [PATCH V3 3/6] usb: xhci: xhci-plat: Add support for Broadcom STB SoC's
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
  2018-11-05 21:31 ` [PATCH V3 1/6] dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document Al Cooper
  2018-11-05 21:31 ` [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-05 21:31 ` [PATCH V3 4/6] usb: ohci-platform: " Al Cooper
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add support for Broadcom STB SoC's to the xhci platform driver

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/host/xhci-brcm.c | 17 +++++++++++++++++
 drivers/usb/host/xhci-brcm.h | 16 ++++++++++++++++
 drivers/usb/host/xhci-plat.c |  8 ++++++++
 3 files changed, 41 insertions(+)
 create mode 100644 drivers/usb/host/xhci-brcm.c
 create mode 100644 drivers/usb/host/xhci-brcm.h

diff --git a/drivers/usb/host/xhci-brcm.c b/drivers/usb/host/xhci-brcm.c
new file mode 100644
index 000000000000..a7220126f6af
--- /dev/null
+++ b/drivers/usb/host/xhci-brcm.c
@@ -0,0 +1,17 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Broadcom */
+
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+#include "xhci.h"
+
+int xhci_plat_brcm_init_quirk(struct usb_hcd *hcd)
+{
+	struct xhci_hcd	*xhci = hcd_to_xhci(hcd);
+
+	xhci->quirks |= XHCI_RESET_ON_RESUME;
+	hcd->suspend_without_phy_exit = 1;
+	return 0;
+}
+
diff --git a/drivers/usb/host/xhci-brcm.h b/drivers/usb/host/xhci-brcm.h
new file mode 100644
index 000000000000..e64800fae4d5
--- /dev/null
+++ b/drivers/usb/host/xhci-brcm.h
@@ -0,0 +1,16 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/* Copyright (c) 2018, Broadcom */
+
+#ifndef _XHCI_BRCM_H
+#define _XHCI_BRCM_H
+
+#if IS_ENABLED(CONFIG_USB_XHCI_BRCM)
+int xhci_plat_brcm_init_quirk(struct usb_hcd *hcd);
+#else
+static inline int xhci_plat_brcm_init_quirk(struct usb_hcd *hcd)
+{
+	return 0;
+}
+#endif
+#endif /* _XHCI_BRCM_H */
+
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index 32b5574ad5c5..5372d4040be6 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -24,6 +24,7 @@
 #include "xhci-plat.h"
 #include "xhci-mvebu.h"
 #include "xhci-rcar.h"
+#include "xhci-brcm.h"
 
 static struct hc_driver __read_mostly xhci_plat_hc_driver;
 
@@ -112,6 +113,10 @@ static int xhci_plat_start(struct usb_hcd *hcd)
 	.resume_quirk = xhci_rcar_resume_quirk,
 };
 
+static const struct xhci_plat_priv xhci_plat_brcm = {
+	.init_quirk = xhci_plat_brcm_init_quirk
+};
+
 static const struct of_device_id usb_xhci_of_match[] = {
 	{
 		.compatible = "generic-xhci",
@@ -144,6 +149,9 @@ static int xhci_plat_start(struct usb_hcd *hcd)
 	}, {
 		.compatible = "renesas,rcar-gen3-xhci",
 		.data = &xhci_plat_renesas_rcar_gen3,
+	}, {
+		.compatible = "brcm,bcm7445-xhci",
+		.data = &xhci_plat_brcm,
 	},
 	{},
 };
-- 
1.9.0.138.g2de3478


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

* [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
                   ` (2 preceding siblings ...)
  2018-11-05 21:31 ` [PATCH V3 3/6] usb: xhci: xhci-plat: Add support for Broadcom STB SoC's Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-06 16:08   ` Alan Stern
  2018-11-05 21:31 ` [PATCH V3 5/6] usb: ehci: Add new EHCI driver " Al Cooper
  2018-11-05 21:31 ` [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers Al Cooper
  5 siblings, 1 reply; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add support for Broadcom STB SoC's to the ohci platform driver.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/host/ohci-platform.c | 34 ++++++++++++++++++++++++++++------
 include/linux/usb/ohci_pdriver.h |  1 +
 2 files changed, 29 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/host/ohci-platform.c b/drivers/usb/host/ohci-platform.c
index 65a1c3fdc88c..f425168238e6 100644
--- a/drivers/usb/host/ohci-platform.c
+++ b/drivers/usb/host/ohci-platform.c
@@ -22,6 +22,7 @@
 #include <linux/module.h>
 #include <linux/err.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/pm_runtime.h>
 #include <linux/reset.h>
@@ -99,12 +100,23 @@ static int ohci_platform_probe(struct platform_device *dev)
 	if (usb_disabled())
 		return -ENODEV;
 
-	/*
-	 * Use reasonable defaults so platforms don't have to provide these
-	 * with DT probing on ARM.
-	 */
-	if (!pdata)
-		pdata = &ohci_platform_defaults;
+	if (!pdata) {
+		const struct usb_ohci_pdata *match_pdata;
+
+		match_pdata = of_device_get_match_data(&dev->dev);
+		if (match_pdata) {
+			pdata = devm_kmemdup(&dev->dev, match_pdata,
+					     sizeof(*pdata), GFP_KERNEL);
+			if (!pdata)
+				return -ENOMEM;
+		} else {
+			/*
+			 * Use reasonable defaults so platforms don't have
+			 * to provide these with DT probing on ARM.
+			 */
+			pdata = &ohci_platform_defaults;
+		}
+	}
 
 	err = dma_coerce_mask_and_coherent(&dev->dev, DMA_BIT_MASK(32));
 	if (err)
@@ -177,6 +189,8 @@ static int ohci_platform_probe(struct platform_device *dev)
 		ohci->flags |= OHCI_QUIRK_FRAME_NO;
 	if (pdata->num_ports)
 		ohci->num_ports = pdata->num_ports;
+	if (pdata->suspend_without_phy_exit)
+		hcd->suspend_without_phy_exit = 1;
 
 #ifndef CONFIG_USB_OHCI_BIG_ENDIAN_MMIO
 	if (ohci->flags & OHCI_QUIRK_BE_MMIO) {
@@ -305,10 +319,18 @@ static int ohci_platform_resume(struct device *dev)
 }
 #endif /* CONFIG_PM_SLEEP */
 
+static const struct usb_ohci_pdata ohci_plat_brcm_bcm7445_ohci = {
+	.power_on =		ohci_platform_power_on,
+	.power_suspend =	ohci_platform_power_off,
+	.power_off =		ohci_platform_power_off,
+	.suspend_without_phy_exit = 1,
+};
+
 static const struct of_device_id ohci_platform_ids[] = {
 	{ .compatible = "generic-ohci", },
 	{ .compatible = "cavium,octeon-6335-ohci", },
 	{ .compatible = "ti,ohci-omap3", },
+	{ .compatible = "brcm,bcm7445-ohci", &ohci_plat_brcm_bcm7445_ohci},
 	{ }
 };
 MODULE_DEVICE_TABLE(of, ohci_platform_ids);
diff --git a/include/linux/usb/ohci_pdriver.h b/include/linux/usb/ohci_pdriver.h
index 7eb16cf587ee..16b24ea1e3bb 100644
--- a/include/linux/usb/ohci_pdriver.h
+++ b/include/linux/usb/ohci_pdriver.h
@@ -35,6 +35,7 @@ struct usb_ohci_pdata {
 	unsigned	big_endian_desc:1;
 	unsigned	big_endian_mmio:1;
 	unsigned	no_big_frame_no:1;
+	unsigned	suspend_without_phy_exit:1;
 	unsigned int	num_ports;
 
 	/* Turn on all power and clocks */
-- 
1.9.0.138.g2de3478


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

* [PATCH V3 5/6] usb: ehci: Add new EHCI driver for Broadcom STB SoC's
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
                   ` (3 preceding siblings ...)
  2018-11-05 21:31 ` [PATCH V3 4/6] usb: ohci-platform: " Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-05 21:31 ` [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers Al Cooper
  5 siblings, 0 replies; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add a new EHCI driver for Broadcom STB SoC's. A new EHCI driver
was created instead of adding support to the existing ehci platform
driver because of the code required to workaround bugs in the EHCI
controller.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 drivers/usb/host/ehci-brcm.c | 287 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 287 insertions(+)
 create mode 100644 drivers/usb/host/ehci-brcm.c

diff --git a/drivers/usb/host/ehci-brcm.c b/drivers/usb/host/ehci-brcm.c
new file mode 100644
index 000000000000..f36ce240538b
--- /dev/null
+++ b/drivers/usb/host/ehci-brcm.c
@@ -0,0 +1,287 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2018, Broadcom */
+
+#include <linux/acpi.h>
+#include <linux/clk.h>
+#include <linux/dma-mapping.h>
+#include <linux/err.h>
+#include <linux/kernel.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
+#include "ehci.h"
+
+#define BRCM_DRIVER_DESC "EHCI Broadcom STB driver"
+
+#define hcd_to_ehci_priv(h) ((struct brcm_priv *)hcd_to_ehci(h)->priv)
+
+struct brcm_priv {
+	struct clk *clk;
+};
+
+static const char brcm_hcd_name[] = "ehci-brcm";
+
+static int (*org_hub_control)(struct usb_hcd *hcd,
+			u16 typeReq, u16 wValue, u16 wIndex,
+			char *buf, u16 wLength);
+
+/* ehci_brcm_wait_for_sof
+ * Wait for start of next microframe, then wait extra delay microseconds
+ */
+static inline void ehci_brcm_wait_for_sof(struct ehci_hcd *ehci, u32 delay)
+{
+	int frame_idx = ehci_readl(ehci, &ehci->regs->frame_index);
+
+	while (frame_idx == ehci_readl(ehci, &ehci->regs->frame_index))
+		;
+	udelay(delay);
+}
+
+/*
+ * ehci_brcm_hub_control
+ * Intercept echi-hcd request to complete RESUME and align it to the start
+ * of the next microframe.
+ * If RESUME is complete too late in the microframe, host controller
+ * detects babble on suspended port and resets the port afterwards.
+ * This s/w workaround allows to avoid this problem.
+ * See SWLINUX-1909 for more details
+ */
+static int ehci_brcm_hub_control(
+	struct usb_hcd	*hcd,
+	u16		typeReq,
+	u16		wValue,
+	u16		wIndex,
+	char		*buf,
+	u16		wLength)
+{
+	struct ehci_hcd	*ehci = hcd_to_ehci(hcd);
+	int		ports = HCS_N_PORTS(ehci->hcs_params);
+	u32 __iomem	*status_reg = &ehci->regs->port_status[
+				(wIndex & 0xff) - 1];
+	unsigned long flags;
+	int retval, irq_disabled = 0;
+
+	/*
+	 * RESUME is cleared when GetPortStatus() is called 20ms after start
+	 * of RESUME
+	 */
+	if ((typeReq == GetPortStatus) &&
+	    (wIndex && wIndex <= ports) &&
+	    ehci->reset_done[wIndex-1] &&
+	    time_after_eq(jiffies, ehci->reset_done[wIndex-1]) &&
+	    (ehci_readl(ehci, status_reg) & PORT_RESUME)) {
+
+		/*
+		 * to make sure we are not interrupted until RESUME bit
+		 * is cleared, disable interrupts on current CPU
+		 */
+		ehci_dbg(ehci, "SOF alignment workaround\n");
+		irq_disabled = 1;
+		local_irq_save(flags);
+		ehci_brcm_wait_for_sof(ehci, 5);
+	}
+	retval = (*org_hub_control)(hcd, typeReq, wValue, wIndex, buf, wLength);
+	if (irq_disabled)
+		local_irq_restore(flags);
+	return retval;
+}
+
+static int ehci_brcm_reset(struct usb_hcd *hcd)
+{
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+
+	ehci->big_endian_mmio = 1;
+	hcd->suspend_without_phy_exit = 1;
+
+	ehci->caps = (struct ehci_caps *) hcd->regs;
+	ehci->regs = (struct ehci_regs *) (hcd->regs +
+		HC_LENGTH(ehci, ehci_readl(ehci, &ehci->caps->hc_capbase)));
+
+	/* This fixes the lockup during reboot due to prior interrupts */
+	ehci_writel(ehci, CMD_RESET, &ehci->regs->command);
+	mdelay(10);
+
+	/*
+	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
+	 *   bus usage
+	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00 @ 0x90
+	 */
+	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
+	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+
+	return ehci_setup(hcd);
+}
+
+static struct hc_driver __read_mostly ehci_brcm_hc_driver;
+
+static const struct ehci_driver_overrides brcm_overrides __initconst = {
+
+	.reset =	ehci_brcm_reset,
+	.extra_priv_size = sizeof(struct brcm_priv),
+};
+
+static int ehci_brcm_probe(struct platform_device *pdev)
+{
+	struct usb_hcd *hcd;
+	struct resource *res_mem;
+	struct brcm_priv *priv;
+	int irq;
+	int err;
+
+	if (usb_disabled())
+		return -ENODEV;
+
+	err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(32));
+	if (err)
+		return err;
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		dev_err(&pdev->dev, "no irq provided");
+		return irq;
+	}
+
+	/* Hook the hub control routine to work around a bug */
+	if (org_hub_control == NULL)
+		org_hub_control = ehci_brcm_hc_driver.hub_control;
+	ehci_brcm_hc_driver.hub_control = ehci_brcm_hub_control;
+
+	/* initialize hcd */
+	hcd = usb_create_hcd(&ehci_brcm_hc_driver,
+			&pdev->dev, dev_name(&pdev->dev));
+	if (!hcd)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, hcd);
+	priv = hcd_to_ehci_priv(hcd);
+
+	priv->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(priv->clk)) {
+		dev_err(&pdev->dev, "Clock not found in Device Tree\n");
+		priv->clk = NULL;
+	}
+	err = clk_prepare_enable(priv->clk);
+	if (err)
+		goto err_hcd;
+
+	res_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	hcd->regs = devm_ioremap_resource(&pdev->dev, res_mem);
+	if (IS_ERR(hcd->regs)) {
+		err = PTR_ERR(hcd->regs);
+		goto err_clk;
+	}
+	hcd->rsrc_start = res_mem->start;
+	hcd->rsrc_len = resource_size(res_mem);
+	err = usb_add_hcd(hcd, irq, IRQF_SHARED);
+	if (err)
+		goto err_clk;
+
+	device_wakeup_enable(hcd->self.controller);
+	device_enable_async_suspend(hcd->self.controller);
+	platform_set_drvdata(pdev, hcd);
+
+	return err;
+
+err_clk:
+	clk_disable_unprepare(priv->clk);
+err_hcd:
+	usb_put_hcd(hcd);
+
+	return err;
+}
+
+static int ehci_brcm_remove(struct platform_device *dev)
+{
+	struct usb_hcd *hcd = platform_get_drvdata(dev);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+
+	usb_remove_hcd(hcd);
+	clk_disable_unprepare(priv->clk);
+	usb_put_hcd(hcd);
+	return 0;
+}
+
+#ifdef CONFIG_PM_SLEEP
+
+static int ehci_brcm_suspend(struct device *dev)
+{
+	int ret;
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+	bool do_wakeup = device_may_wakeup(dev);
+
+	ret = ehci_suspend(hcd, do_wakeup);
+	clk_disable_unprepare(priv->clk);
+	return ret;
+}
+
+static int ehci_brcm_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	struct brcm_priv *priv = hcd_to_ehci_priv(hcd);
+	int err;
+
+	err = clk_prepare_enable(priv->clk);
+	if (err)
+		return err;
+	/*
+	 * SWLINUX-1705: Avoid OUT packet underflows during high memory
+	 *   bus usage
+	 * port_status[0x0f] = Broadcom-proprietary USB_EHCI_INSNREG00
+	 * @ 0x90
+	 */
+	ehci_writel(ehci, 0x00800040, &ehci->regs->port_status[0x10]);
+	ehci_writel(ehci, 0x00000001, &ehci->regs->port_status[0x12]);
+
+	ehci_resume(hcd, false);
+	return 0;
+}
+#endif /* CONFIG_PM_SLEEP */
+
+static SIMPLE_DEV_PM_OPS(ehci_brcm_pm_ops, ehci_brcm_suspend,
+		ehci_brcm_resume);
+
+static const struct of_device_id brcm_ehci_of_match[] = {
+	{ .compatible = "brcm,bcm7445-ehci", },
+	{}
+};
+
+static struct platform_driver ehci_brcm_driver = {
+	.probe		= ehci_brcm_probe,
+	.remove		= ehci_brcm_remove,
+	.shutdown	= usb_hcd_platform_shutdown,
+	.driver		= {
+		.owner	= THIS_MODULE,
+		.name	= "ehci-brcm",
+		.pm	= &ehci_brcm_pm_ops,
+		.of_match_table = brcm_ehci_of_match,
+	}
+};
+
+static int __init ehci_brcm_init(void)
+{
+	if (usb_disabled())
+		return -ENODEV;
+
+	pr_info("%s: " BRCM_DRIVER_DESC "\n", brcm_hcd_name);
+
+	ehci_init_driver(&ehci_brcm_hc_driver, &brcm_overrides);
+	return platform_driver_register(&ehci_brcm_driver);
+}
+module_init(ehci_brcm_init);
+
+static void __exit ehci_brcm_exit(void)
+{
+	platform_driver_unregister(&ehci_brcm_driver);
+}
+module_exit(ehci_brcm_exit);
+
+MODULE_ALIAS("platform:ehci-brcm");
+MODULE_DESCRIPTION(BRCM_DRIVER_DESC);
+MODULE_AUTHOR("Al Cooper");
+MODULE_LICENSE("GPL");
-- 
1.9.0.138.g2de3478


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

* [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers
  2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
                   ` (4 preceding siblings ...)
  2018-11-05 21:31 ` [PATCH V3 5/6] usb: ehci: Add new EHCI driver " Al Cooper
@ 2018-11-05 21:31 ` Al Cooper
  2018-11-06 11:09   ` Arnd Bergmann
  5 siblings, 1 reply; 21+ messages in thread
From: Al Cooper @ 2018-11-05 21:31 UTC (permalink / raw)
  To: linux-kernel
  Cc: Al Cooper, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

Add the build system changes needed to get the Broadcom STB XHCI,
EHCI and OHCI functionality working. The link order for XHCI was
changed in the Makefile because of the way STB XHCI, EHCI and OHCI
controllers share a port which requires that the XHCI driver be
initialized first. Also update MAINTAINERS.

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
 MAINTAINERS               |  9 +++++++++
 drivers/usb/host/Kconfig  | 29 +++++++++++++++++++++++++++++
 drivers/usb/host/Makefile | 18 ++++++++++++------
 3 files changed, 50 insertions(+), 6 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index e8e1d86bd505..c4e7bcb7f992 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -2991,6 +2991,15 @@ S:	Supported
 F:	drivers/i2c/busses/i2c-brcmstb.c
 F:	Documentation/devicetree/bindings/i2c/i2c-brcmstb.txt
 
+BROADCOM BRCMSTB USB XHCI, EHCI and OHCI DRIVERS
+M:	Al Cooper <alcooperx@gmail.com>
+L:	linux-usb@vger.kernel.org
+L:	bcm-kernel-feedback-list@broadcom.com
+S:	Maintained
+F:	drivers/usb/host/xhci-brcm.*
+F:	drivers/usb/host/ohci-brcm.c
+F:	Documentation/devicetree/bindings/usb/brcm,bcm7445-*.txt
+
 BROADCOM BRCMSTB USB2 and USB3 PHY DRIVER
 M:	Al Cooper <alcooperx@gmail.com>
 L:	linux-kernel@vger.kernel.org
diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig
index 16758b12a5e9..ec6b4582ab12 100644
--- a/drivers/usb/host/Kconfig
+++ b/drivers/usb/host/Kconfig
@@ -94,6 +94,35 @@ config USB_XHCI_TEGRA
 	  Say 'Y' to enable the support for the xHCI host controller
 	  found in NVIDIA Tegra124 and later SoCs.
 
+config USB_OHCI_BRCM
+       tristate
+
+config USB_EHCI_BRCM
+       tristate
+
+config USB_XHCI_BRCM
+       tristate
+
+config BRCM_USB_PHY
+       tristate
+
+config USB_BRCM
+	tristate "Broadcom STB USB support"
+	depends on ARCH_BRCMSTB
+	select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
+	select USB_EHCI_BRCM if USB_EHCI_HCD
+	select USB_XHCI_BRCM if USB_XHCI_HCD
+	select USB_XHCI_PLATFORM if USB_XHCI_HCD
+	select BRCM_USB_PHY if USB_OHCI_HCD || USB_EHCI_HCD || USB_XHCI_HCD
+	select GENERIC_PHY if BRCM_USB_PHY
+	default ARCH_BRCMSTB
+	help
+	  Say Y to enable support for XHCI, EHCI and OHCI host controllers
+	  found in Broadcom STB SoC's.
+
+	  Disabling this will keep the controllers and corresponding
+	  PHYs powered down.
+
 endif # USB_XHCI_HCD
 
 config USB_EHCI_HCD
diff --git a/drivers/usb/host/Makefile b/drivers/usb/host/Makefile
index 84514f71ae44..9bdd64b710ee 100644
--- a/drivers/usb/host/Makefile
+++ b/drivers/usb/host/Makefile
@@ -31,6 +31,10 @@ ifneq ($(CONFIG_USB_XHCI_RCAR), )
 	xhci-plat-hcd-y		+= xhci-rcar.o
 endif
 
+ifneq ($(CONFIG_USB_XHCI_BRCM), )
+	xhci-plat-hcd-y		+= xhci-brcm.o
+endif
+
 ifneq ($(CONFIG_DEBUG_FS),)
 	xhci-hcd-y		+= xhci-debugfs.o
 endif
@@ -39,6 +43,13 @@ obj-$(CONFIG_USB_WHCI_HCD)	+= whci/
 
 obj-$(CONFIG_USB_PCI)	+= pci-quirks.o
 
+obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
+obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
+obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
+obj-$(CONFIG_USB_XHCI_HISTB)	+= xhci-histb.o
+obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
+obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
+
 obj-$(CONFIG_USB_EHCI_HCD)	+= ehci-hcd.o
 obj-$(CONFIG_USB_EHCI_PCI)	+= ehci-pci.o
 obj-$(CONFIG_USB_EHCI_HCD_PLATFORM)	+= ehci-platform.o
@@ -52,6 +63,7 @@ obj-$(CONFIG_USB_EHCI_EXYNOS)	+= ehci-exynos.o
 obj-$(CONFIG_USB_EHCI_HCD_AT91) += ehci-atmel.o
 obj-$(CONFIG_USB_EHCI_TEGRA)	+= ehci-tegra.o
 obj-$(CONFIG_USB_W90X900_EHCI)	+= ehci-w90x900.o
+obj-$(CONFIG_USB_EHCI_BRCM)	+= ehci-brcm.o
 
 obj-$(CONFIG_USB_OXU210HP_HCD)	+= oxu210hp-hcd.o
 obj-$(CONFIG_USB_ISP116X_HCD)	+= isp116x-hcd.o
@@ -72,12 +84,6 @@ obj-$(CONFIG_USB_OHCI_HCD_DAVINCI)	+= ohci-da8xx.o
 
 obj-$(CONFIG_USB_UHCI_HCD)	+= uhci-hcd.o
 obj-$(CONFIG_USB_FHCI_HCD)	+= fhci.o
-obj-$(CONFIG_USB_XHCI_HCD)	+= xhci-hcd.o
-obj-$(CONFIG_USB_XHCI_PCI)	+= xhci-pci.o
-obj-$(CONFIG_USB_XHCI_PLATFORM) += xhci-plat-hcd.o
-obj-$(CONFIG_USB_XHCI_HISTB)	+= xhci-histb.o
-obj-$(CONFIG_USB_XHCI_MTK)	+= xhci-mtk.o
-obj-$(CONFIG_USB_XHCI_TEGRA)	+= xhci-tegra.o
 obj-$(CONFIG_USB_SL811_HCD)	+= sl811-hcd.o
 obj-$(CONFIG_USB_SL811_CS)	+= sl811_cs.o
 obj-$(CONFIG_USB_U132_HCD)	+= u132-hcd.o
-- 
1.9.0.138.g2de3478


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

* Re: [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers
  2018-11-05 21:31 ` [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers Al Cooper
@ 2018-11-06 11:09   ` Arnd Bergmann
  2018-11-06 15:55     ` Alan Cooper
  0 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2018-11-06 11:09 UTC (permalink / raw)
  To: Al Cooper
  Cc: linux-kernel, Alan Stern, Alban Bedel, Alex Elder, Andrew Morton,
	Avi Fishman, bcm-kernel-feedback-list, Bjorn Andersson,
	Chunfeng Yun, David S. Miller, devicetree, Dmitry Osipenko,
	Greg Kroah-Hartman, Gustavo A. R. Silva, Hans de Goede,
	James Hogan, Jianguo Sun, Johan Hovold, Kees Cook, linux-usb,
	Lu Baolu, Mark Rutland, Martin Blumenstingl, Mathias Nyman,
	Mathias Nyman, Mauro Carvalho Chehab, Rishabh Bhatnagar,
	Rob Herring, Roger Quadros

On 11/5/18, Al Cooper <alcooperx@gmail.com> wrote:

> +
> +config USB_BRCM
> +	tristate "Broadcom STB USB support"
> +	depends on ARCH_BRCMSTB
> +	select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> +	select USB_EHCI_BRCM if USB_EHCI_HCD
> +	select USB_XHCI_BRCM if USB_XHCI_HCD
> +	select USB_XHCI_PLATFORM if USB_XHCI_HCD
> +	select BRCM_USB_PHY if USB_OHCI_HCD || USB_EHCI_HCD || USB_XHCI_HCD
> +	select GENERIC_PHY if BRCM_USB_PHY
> +	default ARCH_BRCMSTB

Is there a build time dependency on the PHY layer? If not, I would suggest
dropping the last two 'select' statements. I would also suggest
making the dependency 'depends on ARCH_BRCMSTB || COMPILE_TEST'
to allow building it on other architecture for better build testing, and
dropping the 'default' line. Just add this to the defconfig like the other
drivers you have.

      Arnd

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

* Re: [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers
  2018-11-06 11:09   ` Arnd Bergmann
@ 2018-11-06 15:55     ` Alan Cooper
  2018-11-06 18:22       ` Arnd Bergmann
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Cooper @ 2018-11-06 15:55 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: : Linux Kernel Mailing List, Alan Stern, Alban Bedel, Alex Elder,
	Andrew Morton, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, chunfeng yun, David S. Miller, DTML,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	USB list, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On Tue, Nov 6, 2018 at 6:09 AM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On 11/5/18, Al Cooper <alcooperx@gmail.com> wrote:
>
> > +
> > +config USB_BRCM
> > +     tristate "Broadcom STB USB support"
> > +     depends on ARCH_BRCMSTB
> > +     select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> > +     select USB_EHCI_BRCM if USB_EHCI_HCD
> > +     select USB_XHCI_BRCM if USB_XHCI_HCD
> > +     select USB_XHCI_PLATFORM if USB_XHCI_HCD
> > +     select BRCM_USB_PHY if USB_OHCI_HCD || USB_EHCI_HCD || USB_XHCI_HCD
> > +     select GENERIC_PHY if BRCM_USB_PHY
> > +     default ARCH_BRCMSTB
>
> Is there a build time dependency on the PHY layer? If not, I would suggest
> dropping the last two 'select' statements. I would also suggest
> making the dependency 'depends on ARCH_BRCMSTB || COMPILE_TEST'
> to allow building it on other architecture for better build testing, and
> dropping the 'default' line. Just add this to the defconfig like the other
> drivers you have.

I added COMPILE_TEST and dropped the last 2 select lines.
Most of our other drivers default to "y" for "ARCH_BRCMSTB", would it
be better if I changed it to "default y"?

Thanks
Al

>
>       Arnd

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-05 21:31 ` [PATCH V3 4/6] usb: ohci-platform: " Al Cooper
@ 2018-11-06 16:08   ` Alan Stern
  2018-11-06 21:40     ` Al Cooper
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2018-11-06 16:08 UTC (permalink / raw)
  To: Al Cooper
  Cc: linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On Mon, 5 Nov 2018, Al Cooper wrote:

> Add support for Broadcom STB SoC's to the ohci platform driver.
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---

> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct platform_device *dev)
>  		ohci->flags |= OHCI_QUIRK_FRAME_NO;
>  	if (pdata->num_ports)
>  		ohci->num_ports = pdata->num_ports;
> +	if (pdata->suspend_without_phy_exit)
> +		hcd->suspend_without_phy_exit = 1;

Sorry if I missed this in the earlier discussions...  Is there any 
possibility of adding a DT binding that could express this requirement, 
instead of putting it in the platform data?

Alan Stern


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

* Re: [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers
  2018-11-06 15:55     ` Alan Cooper
@ 2018-11-06 18:22       ` Arnd Bergmann
  2018-11-06 19:33         ` Alan Cooper
  0 siblings, 1 reply; 21+ messages in thread
From: Arnd Bergmann @ 2018-11-06 18:22 UTC (permalink / raw)
  To: Alan Cooper
  Cc: : Linux Kernel Mailing List, Alan Stern, Alban Bedel, Alex Elder,
	Andrew Morton, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, chunfeng yun, David S. Miller, DTML,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	USB list, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/6/18, Alan Cooper <alcooperx@gmail.com> wrote:
> On Tue, Nov 6, 2018 at 6:09 AM Arnd Bergmann <arnd@arndb.de> wrote:
>>
>> On 11/5/18, Al Cooper <alcooperx@gmail.com> wrote:
>>
>> > +
>> > +config USB_BRCM
>> > +     tristate "Broadcom STB USB support"
>> > +     depends on ARCH_BRCMSTB
>> > +     select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
>> > +     select USB_EHCI_BRCM if USB_EHCI_HCD
>> > +     select USB_XHCI_BRCM if USB_XHCI_HCD
>> > +     select USB_XHCI_PLATFORM if USB_XHCI_HCD
>> > +     select BRCM_USB_PHY if USB_OHCI_HCD || USB_EHCI_HCD ||
>> > USB_XHCI_HCD
>> > +     select GENERIC_PHY if BRCM_USB_PHY
>> > +     default ARCH_BRCMSTB
>>
>> Is there a build time dependency on the PHY layer? If not, I would
>> suggest
>> dropping the last two 'select' statements. I would also suggest
>> making the dependency 'depends on ARCH_BRCMSTB || COMPILE_TEST'
>> to allow building it on other architecture for better build testing, and
>> dropping the 'default' line. Just add this to the defconfig like the
>> other
>> drivers you have.
>
> I added COMPILE_TEST and dropped the last 2 select lines.
> Most of our other drivers default to "y" for "ARCH_BRCMSTB", would it
> be better if I changed it to "default y"?

No, "default ARCH_BRCMSTB" is better than "default y", but I'd recommend
to just not have non-essential drivers default-on.

      Arnd

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

* Re: [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers
  2018-11-06 18:22       ` Arnd Bergmann
@ 2018-11-06 19:33         ` Alan Cooper
  0 siblings, 0 replies; 21+ messages in thread
From: Alan Cooper @ 2018-11-06 19:33 UTC (permalink / raw)
  To: Arnd Bergmann
  Cc: : Linux Kernel Mailing List, Alan Stern, Alban Bedel, Alex Elder,
	Andrew Morton, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, chunfeng yun, David S. Miller, DTML,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Johan Hovold, Kees Cook, USB list,
	Lu Baolu, Mark Rutland, Martin Blumenstingl, Mathias Nyman,
	Mathias Nyman, Mauro Carvalho Chehab, Rishabh Bhatnagar,
	Rob Herring, Roger Quadros

On Tue, Nov 6, 2018 at 1:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On 11/6/18, Alan Cooper <alcooperx@gmail.com> wrote:
> > On Tue, Nov 6, 2018 at 6:09 AM Arnd Bergmann <arnd@arndb.de> wrote:
> >>
> >> On 11/5/18, Al Cooper <alcooperx@gmail.com> wrote:
> >>
> >> > +
> >> > +config USB_BRCM
> >> > +     tristate "Broadcom STB USB support"
> >> > +     depends on ARCH_BRCMSTB
> >> > +     select USB_OHCI_HCD_PLATFORM if USB_OHCI_HCD
> >> > +     select USB_EHCI_BRCM if USB_EHCI_HCD
> >> > +     select USB_XHCI_BRCM if USB_XHCI_HCD
> >> > +     select USB_XHCI_PLATFORM if USB_XHCI_HCD
> >> > +     select BRCM_USB_PHY if USB_OHCI_HCD || USB_EHCI_HCD ||
> >> > USB_XHCI_HCD
> >> > +     select GENERIC_PHY if BRCM_USB_PHY
> >> > +     default ARCH_BRCMSTB
> >>
> >> Is there a build time dependency on the PHY layer? If not, I would
> >> suggest
> >> dropping the last two 'select' statements. I would also suggest
> >> making the dependency 'depends on ARCH_BRCMSTB || COMPILE_TEST'
> >> to allow building it on other architecture for better build testing, and
> >> dropping the 'default' line. Just add this to the defconfig like the
> >> other
> >> drivers you have.
> >
> > I added COMPILE_TEST and dropped the last 2 select lines.
> > Most of our other drivers default to "y" for "ARCH_BRCMSTB", would it
> > be better if I changed it to "default y"?
>
> No, "default ARCH_BRCMSTB" is better than "default y", but I'd recommend
> to just not have non-essential drivers default-on.
>
>       Arnd

Okay.

Thanks
Al

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-06 16:08   ` Alan Stern
@ 2018-11-06 21:40     ` Al Cooper
  2018-11-06 23:44       ` Florian Fainelli
  0 siblings, 1 reply; 21+ messages in thread
From: Al Cooper @ 2018-11-06 21:40 UTC (permalink / raw)
  To: Alan Stern, Al Cooper
  Cc: linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/6/18 11:08 AM, Alan Stern wrote:
> On Mon, 5 Nov 2018, Al Cooper wrote:
> 
>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>
>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>> ---
> 
>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct platform_device *dev)
>>   		ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>   	if (pdata->num_ports)
>>   		ohci->num_ports = pdata->num_ports;
>> +	if (pdata->suspend_without_phy_exit)
>> +		hcd->suspend_without_phy_exit = 1;
> 
> Sorry if I missed this in the earlier discussions...  Is there any
> possibility of adding a DT binding that could express this requirement,
> instead of putting it in the platform data?
> 
> Alan Stern
> 

Alan,

That was my original approach but internal review suggested that I use 
pdata instead. Below is my original patch for:
[PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and 
init on resume
With this patch I can then use ohci_platform.c without any 
modifications. Could you let me know what you think?

Thanks
Al

Add the ability to skip calling the PHY's exit routine on suspend
and the PHY's init routine on resume. This is to handle a USB PHY
that should have it's power_off function called on suspend but cannot
have it's exit function called because on exit it will disable the
PHY to the point where register accesses to the Host Controllers
using the PHY will be disabled and the host drivers will crash.

This is enabled with the HCD flag "suspend_without_phy_exit" which
can be set from any HCD driver or from the device-tree property
"suspend-without-phy-exit".

Signed-off-by: Al Cooper <alcooperx@gmail.com>
---
  drivers/usb/core/hcd.c  |  8 ++++----
  drivers/usb/core/phy.c  | 21 +++++++++++++++------
  drivers/usb/core/phy.h  |  9 ++++++---
  include/linux/usb/hcd.h |  3 +++
  4 files changed, 28 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 1c21955fe7c0..e67e4d6b3d21 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2263,7 +2263,7 @@ int hcd_bus_suspend(struct usb_device *rhdev, 
pm_message_t msg)
  		hcd->state = HC_STATE_SUSPENDED;

  		if (!PMSG_IS_AUTO(msg))
-			usb_phy_roothub_suspend(hcd->self.sysdev,
+			usb_phy_roothub_suspend(hcd,
  						hcd->phy_roothub);

  		/* Did we race with a root-hub wakeup event? */
@@ -2304,7 +2304,7 @@ int hcd_bus_resume(struct usb_device *rhdev, 
pm_message_t msg)
  	}

  	if (!PMSG_IS_AUTO(msg)) {
-		status = usb_phy_roothub_resume(hcd->self.sysdev,
+		status = usb_phy_roothub_resume(hcd,
  						hcd->phy_roothub);
  		if (status)
  			return status;
@@ -2347,7 +2347,7 @@ int hcd_bus_resume(struct usb_device *rhdev, 
pm_message_t msg)
  		}
  	} else {
  		hcd->state = old_state;
-		usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
+		usb_phy_roothub_suspend(hcd, hcd->phy_roothub);
  		dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
  				"resume", status);
  		if (status != -ESHUTDOWN)
@@ -2744,7 +2744,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
  	struct usb_device *rhdev;

  	if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
-		hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
+		hcd->phy_roothub = usb_phy_roothub_alloc(hcd);
  		if (IS_ERR(hcd->phy_roothub))
  			return PTR_ERR(hcd->phy_roothub);

diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
index 9879767452a2..0eb12566f1c3 100644
--- a/drivers/usb/core/phy.c
+++ b/drivers/usb/core/phy.c
@@ -45,10 +45,11 @@ static int usb_phy_roothub_add_phy(struct device 
*dev, int index,
  	return 0;
  }

-struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd)
  {
  	struct usb_phy_roothub *phy_roothub;
  	int i, num_phys, err;
+	struct device *dev = hcd->self.sysdev;

  	if (!IS_ENABLED(CONFIG_GENERIC_PHY))
  		return NULL;
@@ -58,6 +59,9 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct 
device *dev)
  	if (num_phys <= 0)
  		return NULL;

+	if (device_property_read_bool(dev, "suspend-without-phy-exit"))
+		hcd->suspend_without_phy_exit = 1;
+
  	phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
  	if (!phy_roothub)
  		return ERR_PTR(-ENOMEM);
@@ -161,26 +165,30 @@ void usb_phy_roothub_power_off(struct 
usb_phy_roothub *phy_roothub)
  }
  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);

-int usb_phy_roothub_suspend(struct device *controller_dev,
+int usb_phy_roothub_suspend(struct usb_hcd *hcd,
  			    struct usb_phy_roothub *phy_roothub)
  {
+	struct device *controller_dev = hcd->self.sysdev;
+
  	usb_phy_roothub_power_off(phy_roothub);

  	/* keep the PHYs initialized so the device can wake up the system */
-	if (device_may_wakeup(controller_dev))
+	if (device_may_wakeup(controller_dev) || hcd->suspend_without_phy_exit)
  		return 0;

  	return usb_phy_roothub_exit(phy_roothub);
  }
  EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);

-int usb_phy_roothub_resume(struct device *controller_dev,
+int usb_phy_roothub_resume(struct usb_hcd *hcd,
  			   struct usb_phy_roothub *phy_roothub)
  {
+	struct device *controller_dev = hcd->self.sysdev;
  	int err;

  	/* if the device can't wake up the system _exit was called */
-	if (!device_may_wakeup(controller_dev)) {
+	if (!device_may_wakeup(controller_dev) &&
+	    !hcd->suspend_without_phy_exit) {
  		err = usb_phy_roothub_init(phy_roothub);
  		if (err)
  			return err;
@@ -189,7 +197,8 @@ int usb_phy_roothub_resume(struct device 
*controller_dev,
  	err = usb_phy_roothub_power_on(phy_roothub);

  	/* undo _init if _power_on failed */
-	if (err && !device_may_wakeup(controller_dev))
+	if (err && !device_may_wakeup(controller_dev)
+	    && !hcd->suspend_without_phy_exit)
  		usb_phy_roothub_exit(phy_roothub);

  	return err;
diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
index 88a3c037e9df..34293e11a917 100644
--- a/drivers/usb/core/phy.h
+++ b/drivers/usb/core/phy.h
@@ -5,13 +5,16 @@
   * Copyright (C) 2018 Martin Blumenstingl 
<martin.blumenstingl@googlemail.com>
   */

+#include <linux/usb.h>
+#include <linux/usb/hcd.h>
+
  #ifndef __USB_CORE_PHY_H_
  #define __USB_CORE_PHY_H_

  struct device;
  struct usb_phy_roothub;

-struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
+struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd);

  int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
@@ -19,9 +22,9 @@
  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);

-int usb_phy_roothub_suspend(struct device *controller_dev,
+int usb_phy_roothub_suspend(struct usb_hcd *hcd,
  			    struct usb_phy_roothub *phy_roothub);
-int usb_phy_roothub_resume(struct device *controller_dev,
+int usb_phy_roothub_resume(struct usb_hcd *hcd,
  			   struct usb_phy_roothub *phy_roothub);

  #endif /* __USB_CORE_PHY_H_ */
diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
index 97e2ddec18b1..87a104055b5e 100644
--- a/include/linux/usb/hcd.h
+++ b/include/linux/usb/hcd.h
@@ -157,6 +157,9 @@ struct usb_hcd {
  	 */
  	unsigned		skip_phy_initialization:1;

+	/* Some phys don't want the phy's exit/init called on suspend/resume */
+	unsigned		suspend_without_phy_exit:1;
+
  	/* The next flag is a stopgap, to be removed when all the HCDs
  	 * support the new root-hub polling mechanism. */
  	unsigned		uses_new_polling:1;
-- 
1.9.0.138.g2de3478


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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-06 21:40     ` Al Cooper
@ 2018-11-06 23:44       ` Florian Fainelli
  2018-11-07 15:23         ` Alan Stern
  0 siblings, 1 reply; 21+ messages in thread
From: Florian Fainelli @ 2018-11-06 23:44 UTC (permalink / raw)
  To: Al Cooper, Alan Stern, Al Cooper
  Cc: linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/6/18 1:40 PM, Al Cooper wrote:
> On 11/6/18 11:08 AM, Alan Stern wrote:
>> On Mon, 5 Nov 2018, Al Cooper wrote:
>>
>>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>>
>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>> ---
>>
>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
>>> platform_device *dev)
>>>           ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>>       if (pdata->num_ports)
>>>           ohci->num_ports = pdata->num_ports;
>>> +    if (pdata->suspend_without_phy_exit)
>>> +        hcd->suspend_without_phy_exit = 1;
>>
>> Sorry if I missed this in the earlier discussions...  Is there any
>> possibility of adding a DT binding that could express this requirement,
>> instead of putting it in the platform data?
>>
>> Alan Stern
>>
> 
> Alan,
> 
> That was my original approach but internal review suggested that I use
> pdata instead. Below is my original patch for:

And the reason for that suggestion was really because it was percevied
as encoding a driver behavior as a Device Tree property as opposed to
describing something that was inherently and strictly a hardware
behavior (therefore suitable for Device Tree).

> [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and
> init on resume
> With this patch I can then use ohci_platform.c without any
> modifications. Could you let me know what you think?
> 
> Thanks
> Al
> 
> Add the ability to skip calling the PHY's exit routine on suspend
> and the PHY's init routine on resume. This is to handle a USB PHY
> that should have it's power_off function called on suspend but cannot
> have it's exit function called because on exit it will disable the
> PHY to the point where register accesses to the Host Controllers
> using the PHY will be disabled and the host drivers will crash.
> 
> This is enabled with the HCD flag "suspend_without_phy_exit" which
> can be set from any HCD driver or from the device-tree property
> "suspend-without-phy-exit".
> 
> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> ---
>  drivers/usb/core/hcd.c  |  8 ++++----
>  drivers/usb/core/phy.c  | 21 +++++++++++++++------
>  drivers/usb/core/phy.h  |  9 ++++++---
>  include/linux/usb/hcd.h |  3 +++
>  4 files changed, 28 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 1c21955fe7c0..e67e4d6b3d21 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2263,7 +2263,7 @@ int hcd_bus_suspend(struct usb_device *rhdev,
> pm_message_t msg)
>          hcd->state = HC_STATE_SUSPENDED;
> 
>          if (!PMSG_IS_AUTO(msg))
> -            usb_phy_roothub_suspend(hcd->self.sysdev,
> +            usb_phy_roothub_suspend(hcd,
>                          hcd->phy_roothub);
> 
>          /* Did we race with a root-hub wakeup event? */
> @@ -2304,7 +2304,7 @@ int hcd_bus_resume(struct usb_device *rhdev,
> pm_message_t msg)
>      }
> 
>      if (!PMSG_IS_AUTO(msg)) {
> -        status = usb_phy_roothub_resume(hcd->self.sysdev,
> +        status = usb_phy_roothub_resume(hcd,
>                          hcd->phy_roothub);
>          if (status)
>              return status;
> @@ -2347,7 +2347,7 @@ int hcd_bus_resume(struct usb_device *rhdev,
> pm_message_t msg)
>          }
>      } else {
>          hcd->state = old_state;
> -        usb_phy_roothub_suspend(hcd->self.sysdev, hcd->phy_roothub);
> +        usb_phy_roothub_suspend(hcd, hcd->phy_roothub);
>          dev_dbg(&rhdev->dev, "bus %s fail, err %d\n",
>                  "resume", status);
>          if (status != -ESHUTDOWN)
> @@ -2744,7 +2744,7 @@ int usb_add_hcd(struct usb_hcd *hcd,
>      struct usb_device *rhdev;
> 
>      if (!hcd->skip_phy_initialization && usb_hcd_is_primary_hcd(hcd)) {
> -        hcd->phy_roothub = usb_phy_roothub_alloc(hcd->self.sysdev);
> +        hcd->phy_roothub = usb_phy_roothub_alloc(hcd);
>          if (IS_ERR(hcd->phy_roothub))
>              return PTR_ERR(hcd->phy_roothub);
> 
> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
> index 9879767452a2..0eb12566f1c3 100644
> --- a/drivers/usb/core/phy.c
> +++ b/drivers/usb/core/phy.c
> @@ -45,10 +45,11 @@ static int usb_phy_roothub_add_phy(struct device
> *dev, int index,
>      return 0;
>  }
> 
> -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd)
>  {
>      struct usb_phy_roothub *phy_roothub;
>      int i, num_phys, err;
> +    struct device *dev = hcd->self.sysdev;
> 
>      if (!IS_ENABLED(CONFIG_GENERIC_PHY))
>          return NULL;
> @@ -58,6 +59,9 @@ struct usb_phy_roothub *usb_phy_roothub_alloc(struct
> device *dev)
>      if (num_phys <= 0)
>          return NULL;
> 
> +    if (device_property_read_bool(dev, "suspend-without-phy-exit"))
> +        hcd->suspend_without_phy_exit = 1;
> +
>      phy_roothub = devm_kzalloc(dev, sizeof(*phy_roothub), GFP_KERNEL);
>      if (!phy_roothub)
>          return ERR_PTR(-ENOMEM);
> @@ -161,26 +165,30 @@ void usb_phy_roothub_power_off(struct
> usb_phy_roothub *phy_roothub)
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_power_off);
> 
> -int usb_phy_roothub_suspend(struct device *controller_dev,
> +int usb_phy_roothub_suspend(struct usb_hcd *hcd,
>                  struct usb_phy_roothub *phy_roothub)
>  {
> +    struct device *controller_dev = hcd->self.sysdev;
> +
>      usb_phy_roothub_power_off(phy_roothub);
> 
>      /* keep the PHYs initialized so the device can wake up the system */
> -    if (device_may_wakeup(controller_dev))
> +    if (device_may_wakeup(controller_dev) ||
> hcd->suspend_without_phy_exit)
>          return 0;
> 
>      return usb_phy_roothub_exit(phy_roothub);
>  }
>  EXPORT_SYMBOL_GPL(usb_phy_roothub_suspend);
> 
> -int usb_phy_roothub_resume(struct device *controller_dev,
> +int usb_phy_roothub_resume(struct usb_hcd *hcd,
>                 struct usb_phy_roothub *phy_roothub)
>  {
> +    struct device *controller_dev = hcd->self.sysdev;
>      int err;
> 
>      /* if the device can't wake up the system _exit was called */
> -    if (!device_may_wakeup(controller_dev)) {
> +    if (!device_may_wakeup(controller_dev) &&
> +        !hcd->suspend_without_phy_exit) {
>          err = usb_phy_roothub_init(phy_roothub);
>          if (err)
>              return err;
> @@ -189,7 +197,8 @@ int usb_phy_roothub_resume(struct device
> *controller_dev,
>      err = usb_phy_roothub_power_on(phy_roothub);
> 
>      /* undo _init if _power_on failed */
> -    if (err && !device_may_wakeup(controller_dev))
> +    if (err && !device_may_wakeup(controller_dev)
> +        && !hcd->suspend_without_phy_exit)
>          usb_phy_roothub_exit(phy_roothub);
> 
>      return err;
> diff --git a/drivers/usb/core/phy.h b/drivers/usb/core/phy.h
> index 88a3c037e9df..34293e11a917 100644
> --- a/drivers/usb/core/phy.h
> +++ b/drivers/usb/core/phy.h
> @@ -5,13 +5,16 @@
>   * Copyright (C) 2018 Martin Blumenstingl
> <martin.blumenstingl@googlemail.com>
>   */
> 
> +#include <linux/usb.h>
> +#include <linux/usb/hcd.h>
> +
>  #ifndef __USB_CORE_PHY_H_
>  #define __USB_CORE_PHY_H_
> 
>  struct device;
>  struct usb_phy_roothub;
> 
> -struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev);
> +struct usb_phy_roothub *usb_phy_roothub_alloc(struct usb_hcd *hcd);
> 
>  int usb_phy_roothub_init(struct usb_phy_roothub *phy_roothub);
>  int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub);
> @@ -19,9 +22,9 @@
>  int usb_phy_roothub_power_on(struct usb_phy_roothub *phy_roothub);
>  void usb_phy_roothub_power_off(struct usb_phy_roothub *phy_roothub);
> 
> -int usb_phy_roothub_suspend(struct device *controller_dev,
> +int usb_phy_roothub_suspend(struct usb_hcd *hcd,
>                  struct usb_phy_roothub *phy_roothub);
> -int usb_phy_roothub_resume(struct device *controller_dev,
> +int usb_phy_roothub_resume(struct usb_hcd *hcd,
>                 struct usb_phy_roothub *phy_roothub);
> 
>  #endif /* __USB_CORE_PHY_H_ */
> diff --git a/include/linux/usb/hcd.h b/include/linux/usb/hcd.h
> index 97e2ddec18b1..87a104055b5e 100644
> --- a/include/linux/usb/hcd.h
> +++ b/include/linux/usb/hcd.h
> @@ -157,6 +157,9 @@ struct usb_hcd {
>       */
>      unsigned        skip_phy_initialization:1;
> 
> +    /* Some phys don't want the phy's exit/init called on
> suspend/resume */
> +    unsigned        suspend_without_phy_exit:1;
> +
>      /* The next flag is a stopgap, to be removed when all the HCDs
>       * support the new root-hub polling mechanism. */
>      unsigned        uses_new_polling:1;


-- 
Florian

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-06 23:44       ` Florian Fainelli
@ 2018-11-07 15:23         ` Alan Stern
  2018-11-07 15:58           ` Al Cooper
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2018-11-07 15:23 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Al Cooper, Al Cooper, linux-kernel, Alban Bedel, Alex Elder,
	Andrew Morton, Arnd Bergmann, Avi Fishman,
	bcm-kernel-feedback-list, Bjorn Andersson, Chunfeng Yun,
	David S. Miller, devicetree, Dmitry Osipenko, Greg Kroah-Hartman,
	Gustavo A. R. Silva, Hans de Goede, James Hogan, Jianguo Sun,
	Johan Hovold, Kees Cook, linux-usb, Lu Baolu, Mark Rutland,
	Martin Blumenstingl, Mathias Nyman, Mathias Nyman,
	Mauro Carvalho Chehab, Rishabh Bhatnagar, Rob Herring,
	Roger Quadros

On Tue, 6 Nov 2018, Florian Fainelli wrote:

> On 11/6/18 1:40 PM, Al Cooper wrote:
> > On 11/6/18 11:08 AM, Alan Stern wrote:
> >> On Mon, 5 Nov 2018, Al Cooper wrote:
> >>
> >>> Add support for Broadcom STB SoC's to the ohci platform driver.
> >>>
> >>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> >>> ---
> >>
> >>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
> >>> platform_device *dev)
> >>>           ohci->flags |= OHCI_QUIRK_FRAME_NO;
> >>>       if (pdata->num_ports)
> >>>           ohci->num_ports = pdata->num_ports;
> >>> +    if (pdata->suspend_without_phy_exit)
> >>> +        hcd->suspend_without_phy_exit = 1;
> >>
> >> Sorry if I missed this in the earlier discussions...  Is there any
> >> possibility of adding a DT binding that could express this requirement,
> >> instead of putting it in the platform data?
> >>
> >> Alan Stern
> >>
> > 
> > Alan,
> > 
> > That was my original approach but internal review suggested that I use
> > pdata instead. Below is my original patch for:
> 
> And the reason for that suggestion was really because it was percevied
> as encoding a driver behavior as a Device Tree property as opposed to
> describing something that was inherently and strictly a hardware
> behavior (therefore suitable for Device Tree).

Right.  The best way to approach this problem is to identify and 
characterize the hardware behavior which makes this override necessary.  
Then _that_ can be added to DT, since it will be a property of the 
hardware rather than of the driver.

> > Add the ability to skip calling the PHY's exit routine on suspend
> > and the PHY's init routine on resume. This is to handle a USB PHY
> > that should have it's power_off function called on suspend but cannot
> > have it's exit function called because on exit it will disable the
> > PHY to the point where register accesses to the Host Controllers
> > using the PHY will be disabled and the host drivers will crash.

What's special about this PHY?  Why does the exit function mess the PHY 
up?  Or to put it another way, why doesn't the exit function mess up 
other PHYs in the same way?

For that matter, can we change the code so that suspend doesn't call 
the exit function for _any_ PHY?  Will just calling the power_off 
function be good enough?  If not, then why not?

Alan Stern


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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-07 15:23         ` Alan Stern
@ 2018-11-07 15:58           ` Al Cooper
  2018-11-07 16:27             ` Alan Stern
  0 siblings, 1 reply; 21+ messages in thread
From: Al Cooper @ 2018-11-07 15:58 UTC (permalink / raw)
  To: Alan Stern, Florian Fainelli
  Cc: Al Cooper, linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/7/18 10:23 AM, Alan Stern wrote:
> On Tue, 6 Nov 2018, Florian Fainelli wrote:
> 
>> On 11/6/18 1:40 PM, Al Cooper wrote:
>>> On 11/6/18 11:08 AM, Alan Stern wrote:
>>>> On Mon, 5 Nov 2018, Al Cooper wrote:
>>>>
>>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>>>>
>>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>>> ---
>>>>
>>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
>>>>> platform_device *dev)
>>>>>            ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>>>>        if (pdata->num_ports)
>>>>>            ohci->num_ports = pdata->num_ports;
>>>>> +    if (pdata->suspend_without_phy_exit)
>>>>> +        hcd->suspend_without_phy_exit = 1;
>>>>
>>>> Sorry if I missed this in the earlier discussions...  Is there any
>>>> possibility of adding a DT binding that could express this requirement,
>>>> instead of putting it in the platform data?
>>>>
>>>> Alan Stern
>>>>
>>>
>>> Alan,
>>>
>>> That was my original approach but internal review suggested that I use
>>> pdata instead. Below is my original patch for:
>>
>> And the reason for that suggestion was really because it was percevied
>> as encoding a driver behavior as a Device Tree property as opposed to
>> describing something that was inherently and strictly a hardware
>> behavior (therefore suitable for Device Tree).
> 
> Right.  The best way to approach this problem is to identify and
> characterize the hardware behavior which makes this override necessary.
> Then _that_ can be added to DT, since it will be a property of the
> hardware rather than of the driver.
> 
>>> Add the ability to skip calling the PHY's exit routine on suspend
>>> and the PHY's init routine on resume. This is to handle a USB PHY
>>> that should have it's power_off function called on suspend but cannot
>>> have it's exit function called because on exit it will disable the
>>> PHY to the point where register accesses to the Host Controllers
>>> using the PHY will be disabled and the host drivers will crash.
> 
> What's special about this PHY?  Why does the exit function mess the PHY
> up?  Or to put it another way, why doesn't the exit function mess up
> other PHYs in the same way?
> 
> For that matter, can we change the code so that suspend doesn't call
> the exit function for _any_ PHY?  Will just calling the power_off
> function be good enough?  If not, then why not?
> 
> Alan Stern
> 

In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and 
XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI 
and XHCI registers will cause an exception if accessed and cause the 
EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing 
this in the hardware by adding an aux clock that will takeover when the 
PHY clock is shut down, but this hasn't happened yet. It seems like 
"exit on suspend" still makes sense on systems that don't have this 
problem (additional power savings?) so removing the exit on suspend for 
all systems is not a good idea.

Al

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-07 15:58           ` Al Cooper
@ 2018-11-07 16:27             ` Alan Stern
  2018-11-07 17:29               ` Florian Fainelli
  0 siblings, 1 reply; 21+ messages in thread
From: Alan Stern @ 2018-11-07 16:27 UTC (permalink / raw)
  To: Al Cooper
  Cc: Florian Fainelli, Al Cooper, linux-kernel, Alban Bedel,
	Alex Elder, Andrew Morton, Arnd Bergmann, Avi Fishman,
	bcm-kernel-feedback-list, Bjorn Andersson, Chunfeng Yun,
	David S. Miller, devicetree, Dmitry Osipenko, Greg Kroah-Hartman,
	Gustavo A. R. Silva, Hans de Goede, James Hogan, Jianguo Sun,
	Johan Hovold, Kees Cook, linux-usb, Lu Baolu, Mark Rutland,
	Martin Blumenstingl, Mathias Nyman, Mathias Nyman,
	Mauro Carvalho Chehab, Rishabh Bhatnagar, Rob Herring,
	Roger Quadros

On Wed, 7 Nov 2018, Al Cooper wrote:

> On 11/7/18 10:23 AM, Alan Stern wrote:
> > On Tue, 6 Nov 2018, Florian Fainelli wrote:
> > 
> >> On 11/6/18 1:40 PM, Al Cooper wrote:
> >>> On 11/6/18 11:08 AM, Alan Stern wrote:
> >>>> On Mon, 5 Nov 2018, Al Cooper wrote:
> >>>>
> >>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
> >>>>>
> >>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> >>>>> ---
> >>>>
> >>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
> >>>>> platform_device *dev)
> >>>>>            ohci->flags |= OHCI_QUIRK_FRAME_NO;
> >>>>>        if (pdata->num_ports)
> >>>>>            ohci->num_ports = pdata->num_ports;
> >>>>> +    if (pdata->suspend_without_phy_exit)
> >>>>> +        hcd->suspend_without_phy_exit = 1;
> >>>>
> >>>> Sorry if I missed this in the earlier discussions...  Is there any
> >>>> possibility of adding a DT binding that could express this requirement,
> >>>> instead of putting it in the platform data?
> >>>>
> >>>> Alan Stern
> >>>>
> >>>
> >>> Alan,
> >>>
> >>> That was my original approach but internal review suggested that I use
> >>> pdata instead. Below is my original patch for:
> >>
> >> And the reason for that suggestion was really because it was percevied
> >> as encoding a driver behavior as a Device Tree property as opposed to
> >> describing something that was inherently and strictly a hardware
> >> behavior (therefore suitable for Device Tree).
> > 
> > Right.  The best way to approach this problem is to identify and
> > characterize the hardware behavior which makes this override necessary.
> > Then _that_ can be added to DT, since it will be a property of the
> > hardware rather than of the driver.
> > 
> >>> Add the ability to skip calling the PHY's exit routine on suspend
> >>> and the PHY's init routine on resume. This is to handle a USB PHY
> >>> that should have it's power_off function called on suspend but cannot
> >>> have it's exit function called because on exit it will disable the
> >>> PHY to the point where register accesses to the Host Controllers
> >>> using the PHY will be disabled and the host drivers will crash.
> > 
> > What's special about this PHY?  Why does the exit function mess the PHY
> > up?  Or to put it another way, why doesn't the exit function mess up
> > other PHYs in the same way?
> > 
> > For that matter, can we change the code so that suspend doesn't call
> > the exit function for _any_ PHY?  Will just calling the power_off
> > function be good enough?  If not, then why not?
> > 
> > Alan Stern
> > 
> 
> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and 
> XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI 
> and XHCI registers will cause an exception if accessed and cause the 
> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing 
> this in the hardware by adding an aux clock that will takeover when the 
> PHY clock is shut down, but this hasn't happened yet. It seems like 
> "exit on suspend" still makes sense on systems that don't have this 
> problem (additional power savings?) so removing the exit on suspend for 
> all systems is not a good idea.

Then in theory you should be able to add a Device Tree property which
says that the PHY provides a clock for the USB host controller.  That
is strictly a property of the hardware; it has nothing to do with the
driver.  Therefore it is appropriate for DT.

Wouldn't this solve your issue?

Alan Stern


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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-07 16:27             ` Alan Stern
@ 2018-11-07 17:29               ` Florian Fainelli
  2018-11-07 17:40                 ` Al Cooper
  0 siblings, 1 reply; 21+ messages in thread
From: Florian Fainelli @ 2018-11-07 17:29 UTC (permalink / raw)
  To: Alan Stern, Al Cooper
  Cc: Florian Fainelli, Al Cooper, linux-kernel, Alban Bedel,
	Alex Elder, Andrew Morton, Arnd Bergmann, Avi Fishman,
	bcm-kernel-feedback-list, Bjorn Andersson, Chunfeng Yun,
	David S. Miller, devicetree, Dmitry Osipenko, Greg Kroah-Hartman,
	Gustavo A. R. Silva, Hans de Goede, James Hogan, Jianguo Sun,
	Johan Hovold, Kees Cook, linux-usb, Lu Baolu, Mark Rutland,
	Martin Blumenstingl, Mathias Nyman, Mathias Nyman,
	Mauro Carvalho Chehab, Rishabh Bhatnagar, Rob Herring,
	Roger Quadros

On 11/7/18 8:27 AM, Alan Stern wrote:
> On Wed, 7 Nov 2018, Al Cooper wrote:
> 
>> On 11/7/18 10:23 AM, Alan Stern wrote:
>>> On Tue, 6 Nov 2018, Florian Fainelli wrote:
>>>
>>>> On 11/6/18 1:40 PM, Al Cooper wrote:
>>>>> On 11/6/18 11:08 AM, Alan Stern wrote:
>>>>>> On Mon, 5 Nov 2018, Al Cooper wrote:
>>>>>>
>>>>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>>>>>>
>>>>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>>>>> ---
>>>>>>
>>>>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
>>>>>>> platform_device *dev)
>>>>>>>            ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>>>>>>        if (pdata->num_ports)
>>>>>>>            ohci->num_ports = pdata->num_ports;
>>>>>>> +    if (pdata->suspend_without_phy_exit)
>>>>>>> +        hcd->suspend_without_phy_exit = 1;
>>>>>>
>>>>>> Sorry if I missed this in the earlier discussions...  Is there any
>>>>>> possibility of adding a DT binding that could express this requirement,
>>>>>> instead of putting it in the platform data?
>>>>>>
>>>>>> Alan Stern
>>>>>>
>>>>>
>>>>> Alan,
>>>>>
>>>>> That was my original approach but internal review suggested that I use
>>>>> pdata instead. Below is my original patch for:
>>>>
>>>> And the reason for that suggestion was really because it was percevied
>>>> as encoding a driver behavior as a Device Tree property as opposed to
>>>> describing something that was inherently and strictly a hardware
>>>> behavior (therefore suitable for Device Tree).
>>>
>>> Right.  The best way to approach this problem is to identify and
>>> characterize the hardware behavior which makes this override necessary.
>>> Then _that_ can be added to DT, since it will be a property of the
>>> hardware rather than of the driver.
>>>
>>>>> Add the ability to skip calling the PHY's exit routine on suspend
>>>>> and the PHY's init routine on resume. This is to handle a USB PHY
>>>>> that should have it's power_off function called on suspend but cannot
>>>>> have it's exit function called because on exit it will disable the
>>>>> PHY to the point where register accesses to the Host Controllers
>>>>> using the PHY will be disabled and the host drivers will crash.
>>>
>>> What's special about this PHY?  Why does the exit function mess the PHY
>>> up?  Or to put it another way, why doesn't the exit function mess up
>>> other PHYs in the same way?
>>>
>>> For that matter, can we change the code so that suspend doesn't call
>>> the exit function for _any_ PHY?  Will just calling the power_off
>>> function be good enough?  If not, then why not?
>>>
>>> Alan Stern
>>>
>>
>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and 
>> XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI 
>> and XHCI registers will cause an exception if accessed and cause the 
>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing 
>> this in the hardware by adding an aux clock that will takeover when the 
>> PHY clock is shut down, but this hasn't happened yet. It seems like 
>> "exit on suspend" still makes sense on systems that don't have this 
>> problem (additional power savings?) so removing the exit on suspend for 
>> all systems is not a good idea.
> 
> Then in theory you should be able to add a Device Tree property which
> says that the PHY provides a clock for the USB host controller.  That
> is strictly a property of the hardware; it has nothing to do with the
> driver.  Therefore it is appropriate for DT.

The very compatible string that is being allocated/defined for this
controller carries that information already, that is, if you probe a
"brcm,bcm7445-ohci" compatible then that means the controller has a
dependency on the PHY to supply its clock.

Adding a property vs. keying on the compatible string makes sense if you
know there is at least a second consumer of that property (unless we
make it a broadcom specific property, in which case, it really is
redundant with the compatible string).

Anyway, my grudge with that property was the name chosen initially,
which included an action to be performed by an implementation as opposed
to something purely descriptive. E.g: 'phy-supplies-clock' might be okay.

> 
> Wouldn't this solve your issue?

It would not change much except that there is no need to much with
ohci-platform.c anymore, but ultimately that property needs to be read
by ohci-hcd.c and acted on, which would likely lead to the same amount
of changes as those present in patch #2 currently.
-- 
Florian

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-07 17:29               ` Florian Fainelli
@ 2018-11-07 17:40                 ` Al Cooper
  2018-11-07 18:11                   ` Florian Fainelli
  0 siblings, 1 reply; 21+ messages in thread
From: Al Cooper @ 2018-11-07 17:40 UTC (permalink / raw)
  To: Florian Fainelli, Alan Stern
  Cc: Al Cooper, linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/7/18 12:29 PM, Florian Fainelli wrote:
> On 11/7/18 8:27 AM, Alan Stern wrote:
>> On Wed, 7 Nov 2018, Al Cooper wrote:
>>
>>> On 11/7/18 10:23 AM, Alan Stern wrote:
>>>> On Tue, 6 Nov 2018, Florian Fainelli wrote:
>>>>
>>>>> On 11/6/18 1:40 PM, Al Cooper wrote:
>>>>>> On 11/6/18 11:08 AM, Alan Stern wrote:
>>>>>>> On Mon, 5 Nov 2018, Al Cooper wrote:
>>>>>>>
>>>>>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>>>>>>>
>>>>>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>>>>>> ---
>>>>>>>
>>>>>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
>>>>>>>> platform_device *dev)
>>>>>>>>             ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>>>>>>>         if (pdata->num_ports)
>>>>>>>>             ohci->num_ports = pdata->num_ports;
>>>>>>>> +    if (pdata->suspend_without_phy_exit)
>>>>>>>> +        hcd->suspend_without_phy_exit = 1;
>>>>>>>
>>>>>>> Sorry if I missed this in the earlier discussions...  Is there any
>>>>>>> possibility of adding a DT binding that could express this requirement,
>>>>>>> instead of putting it in the platform data?
>>>>>>>
>>>>>>> Alan Stern
>>>>>>>
>>>>>>
>>>>>> Alan,
>>>>>>
>>>>>> That was my original approach but internal review suggested that I use
>>>>>> pdata instead. Below is my original patch for:
>>>>>
>>>>> And the reason for that suggestion was really because it was percevied
>>>>> as encoding a driver behavior as a Device Tree property as opposed to
>>>>> describing something that was inherently and strictly a hardware
>>>>> behavior (therefore suitable for Device Tree).
>>>>
>>>> Right.  The best way to approach this problem is to identify and
>>>> characterize the hardware behavior which makes this override necessary.
>>>> Then _that_ can be added to DT, since it will be a property of the
>>>> hardware rather than of the driver.
>>>>
>>>>>> Add the ability to skip calling the PHY's exit routine on suspend
>>>>>> and the PHY's init routine on resume. This is to handle a USB PHY
>>>>>> that should have it's power_off function called on suspend but cannot
>>>>>> have it's exit function called because on exit it will disable the
>>>>>> PHY to the point where register accesses to the Host Controllers
>>>>>> using the PHY will be disabled and the host drivers will crash.
>>>>
>>>> What's special about this PHY?  Why does the exit function mess the PHY
>>>> up?  Or to put it another way, why doesn't the exit function mess up
>>>> other PHYs in the same way?
>>>>
>>>> For that matter, can we change the code so that suspend doesn't call
>>>> the exit function for _any_ PHY?  Will just calling the power_off
>>>> function be good enough?  If not, then why not?
>>>>
>>>> Alan Stern
>>>>
>>>
>>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and
>>> XHCI host controllers and if the PHY is totally shut down the EHCI, OHCI
>>> and XHCI registers will cause an exception if accessed and cause the
>>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing
>>> this in the hardware by adding an aux clock that will takeover when the
>>> PHY clock is shut down, but this hasn't happened yet. It seems like
>>> "exit on suspend" still makes sense on systems that don't have this
>>> problem (additional power savings?) so removing the exit on suspend for
>>> all systems is not a good idea.
>>
>> Then in theory you should be able to add a Device Tree property which
>> says that the PHY provides a clock for the USB host controller.  That
>> is strictly a property of the hardware; it has nothing to do with the
>> driver.  Therefore it is appropriate for DT.
> 
> The very compatible string that is being allocated/defined for this
> controller carries that information already, that is, if you probe a
> "brcm,bcm7445-ohci" compatible then that means the controller has a
> dependency on the PHY to supply its clock.
> 
> Adding a property vs. keying on the compatible string makes sense if you
> know there is at least a second consumer of that property (unless we
> make it a broadcom specific property, in which case, it really is
> redundant with the compatible string).
> 
> Anyway, my grudge with that property was the name chosen initially,
> which included an action to be performed by an implementation as opposed
> to something purely descriptive. E.g: 'phy-supplies-clock' might be okay.
> 
>>
>> Wouldn't this solve your issue?
> 
> It would not change much except that there is no need to much with
> ohci-platform.c anymore, but ultimately that property needs to be read
> by ohci-hcd.c and acted on, which would likely lead to the same amount
> of changes as those present in patch #2 currently.
> 
We also need this functionality in the EHCI and XHCI drivers and it's 
not the ohci-hcd.c module that needs to know, it's the core/phy.c module 
called from core/hcd.c.

Al

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
  2018-11-07 17:40                 ` Al Cooper
@ 2018-11-07 18:11                   ` Florian Fainelli
       [not found]                     ` <20181112174532.GA14682@bogus>
  0 siblings, 1 reply; 21+ messages in thread
From: Florian Fainelli @ 2018-11-07 18:11 UTC (permalink / raw)
  To: Al Cooper, Florian Fainelli, Alan Stern
  Cc: Al Cooper, linux-kernel, Alban Bedel, Alex Elder, Andrew Morton,
	Arnd Bergmann, Avi Fishman, bcm-kernel-feedback-list,
	Bjorn Andersson, Chunfeng Yun, David S. Miller, devicetree,
	Dmitry Osipenko, Greg Kroah-Hartman, Gustavo A. R. Silva,
	Hans de Goede, James Hogan, Jianguo Sun, Johan Hovold, Kees Cook,
	linux-usb, Lu Baolu, Mark Rutland, Martin Blumenstingl,
	Mathias Nyman, Mathias Nyman, Mauro Carvalho Chehab,
	Rishabh Bhatnagar, Rob Herring, Roger Quadros

On 11/7/2018 9:40 AM, Al Cooper wrote:
> On 11/7/18 12:29 PM, Florian Fainelli wrote:
>> On 11/7/18 8:27 AM, Alan Stern wrote:
>>> On Wed, 7 Nov 2018, Al Cooper wrote:
>>>
>>>> On 11/7/18 10:23 AM, Alan Stern wrote:
>>>>> On Tue, 6 Nov 2018, Florian Fainelli wrote:
>>>>>
>>>>>> On 11/6/18 1:40 PM, Al Cooper wrote:
>>>>>>> On 11/6/18 11:08 AM, Alan Stern wrote:
>>>>>>>> On Mon, 5 Nov 2018, Al Cooper wrote:
>>>>>>>>
>>>>>>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
>>>>>>>>> ---
>>>>>>>>
>>>>>>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
>>>>>>>>> platform_device *dev)
>>>>>>>>>             ohci->flags |= OHCI_QUIRK_FRAME_NO;
>>>>>>>>>         if (pdata->num_ports)
>>>>>>>>>             ohci->num_ports = pdata->num_ports;
>>>>>>>>> +    if (pdata->suspend_without_phy_exit)
>>>>>>>>> +        hcd->suspend_without_phy_exit = 1;
>>>>>>>>
>>>>>>>> Sorry if I missed this in the earlier discussions...  Is there any
>>>>>>>> possibility of adding a DT binding that could express this
>>>>>>>> requirement,
>>>>>>>> instead of putting it in the platform data?
>>>>>>>>
>>>>>>>> Alan Stern
>>>>>>>>
>>>>>>>
>>>>>>> Alan,
>>>>>>>
>>>>>>> That was my original approach but internal review suggested that
>>>>>>> I use
>>>>>>> pdata instead. Below is my original patch for:
>>>>>>
>>>>>> And the reason for that suggestion was really because it was
>>>>>> percevied
>>>>>> as encoding a driver behavior as a Device Tree property as opposed to
>>>>>> describing something that was inherently and strictly a hardware
>>>>>> behavior (therefore suitable for Device Tree).
>>>>>
>>>>> Right.  The best way to approach this problem is to identify and
>>>>> characterize the hardware behavior which makes this override
>>>>> necessary.
>>>>> Then _that_ can be added to DT, since it will be a property of the
>>>>> hardware rather than of the driver.
>>>>>
>>>>>>> Add the ability to skip calling the PHY's exit routine on suspend
>>>>>>> and the PHY's init routine on resume. This is to handle a USB PHY
>>>>>>> that should have it's power_off function called on suspend but
>>>>>>> cannot
>>>>>>> have it's exit function called because on exit it will disable the
>>>>>>> PHY to the point where register accesses to the Host Controllers
>>>>>>> using the PHY will be disabled and the host drivers will crash.
>>>>>
>>>>> What's special about this PHY?  Why does the exit function mess the
>>>>> PHY
>>>>> up?  Or to put it another way, why doesn't the exit function mess up
>>>>> other PHYs in the same way?
>>>>>
>>>>> For that matter, can we change the code so that suspend doesn't call
>>>>> the exit function for _any_ PHY?  Will just calling the power_off
>>>>> function be good enough?  If not, then why not?
>>>>>
>>>>> Alan Stern
>>>>>
>>>>
>>>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and
>>>> XHCI host controllers and if the PHY is totally shut down the EHCI,
>>>> OHCI
>>>> and XHCI registers will cause an exception if accessed and cause the
>>>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing
>>>> this in the hardware by adding an aux clock that will takeover when the
>>>> PHY clock is shut down, but this hasn't happened yet. It seems like
>>>> "exit on suspend" still makes sense on systems that don't have this
>>>> problem (additional power savings?) so removing the exit on suspend for
>>>> all systems is not a good idea.
>>>
>>> Then in theory you should be able to add a Device Tree property which
>>> says that the PHY provides a clock for the USB host controller.  That
>>> is strictly a property of the hardware; it has nothing to do with the
>>> driver.  Therefore it is appropriate for DT.
>>
>> The very compatible string that is being allocated/defined for this
>> controller carries that information already, that is, if you probe a
>> "brcm,bcm7445-ohci" compatible then that means the controller has a
>> dependency on the PHY to supply its clock.
>>
>> Adding a property vs. keying on the compatible string makes sense if you
>> know there is at least a second consumer of that property (unless we
>> make it a broadcom specific property, in which case, it really is
>> redundant with the compatible string).
>>
>> Anyway, my grudge with that property was the name chosen initially,
>> which included an action to be performed by an implementation as opposed
>> to something purely descriptive. E.g: 'phy-supplies-clock' might be okay.
>>
>>>
>>> Wouldn't this solve your issue?
>>
>> It would not change much except that there is no need to much with
>> ohci-platform.c anymore, but ultimately that property needs to be read
>> by ohci-hcd.c and acted on, which would likely lead to the same amount
>> of changes as those present in patch #2 currently.
>>
> We also need this functionality in the EHCI and XHCI drivers and it's
> not the ohci-hcd.c module that needs to know, it's the core/phy.c module
> called from core/hcd.c.

So in that regard the Device Tree property would actually scale a bit
better in that you would no longer need to modify the various
*hci-plat*.c files, if that is the way to go, then sure.
-- 
Florian

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

* Re: [PATCH V3 4/6] usb: ohci-platform: Add support for Broadcom STB SoC's
       [not found]                     ` <20181112174532.GA14682@bogus>
@ 2018-11-13 21:54                       ` Alan Cooper
  0 siblings, 0 replies; 21+ messages in thread
From: Alan Cooper @ 2018-11-13 21:54 UTC (permalink / raw)
  To: Rob Herring
  Cc: Florian Fainelli, al.cooper, Alan Stern,
	: Linux Kernel Mailing List, Alban Bedel, Alex Elder,
	Andrew Morton, Arnd Bergmann, Avi Fishman,
	bcm-kernel-feedback-list, Bjorn Andersson, chunfeng yun,
	David S. Miller, DTML, Dmitry Osipenko, Greg Kroah-Hartman,
	Gustavo A. R. Silva, Hans de Goede, James Hogan, Jianguo Sun,
	Johan Hovold, Kees Cook, USB list, Lu Baolu, Mark Rutland,
	Martin Blumenstingl, Mathias Nyman, Mathias Nyman,
	Mauro Carvalho Chehab, Rishabh Bhatnagar, Roger Quadros

On Mon, Nov 12, 2018 at 6:37 PM Rob Herring <robh@kernel.org> wrote:
>
> On Wed, Nov 07, 2018 at 10:11:59AM -0800, Florian Fainelli wrote:
> > On 11/7/2018 9:40 AM, Al Cooper wrote:
> > > On 11/7/18 12:29 PM, Florian Fainelli wrote:
> > >> On 11/7/18 8:27 AM, Alan Stern wrote:
> > >>> On Wed, 7 Nov 2018, Al Cooper wrote:
> > >>>
> > >>>> On 11/7/18 10:23 AM, Alan Stern wrote:
> > >>>>> On Tue, 6 Nov 2018, Florian Fainelli wrote:
> > >>>>>
> > >>>>>> On 11/6/18 1:40 PM, Al Cooper wrote:
> > >>>>>>> On 11/6/18 11:08 AM, Alan Stern wrote:
> > >>>>>>>> On Mon, 5 Nov 2018, Al Cooper wrote:
> > >>>>>>>>
> > >>>>>>>>> Add support for Broadcom STB SoC's to the ohci platform driver.
> > >>>>>>>>>
> > >>>>>>>>> Signed-off-by: Al Cooper <alcooperx@gmail.com>
> > >>>>>>>>> ---
> > >>>>>>>>
> > >>>>>>>>> @@ -177,6 +189,8 @@ static int ohci_platform_probe(struct
> > >>>>>>>>> platform_device *dev)
> > >>>>>>>>>             ohci->flags |= OHCI_QUIRK_FRAME_NO;
> > >>>>>>>>>         if (pdata->num_ports)
> > >>>>>>>>>             ohci->num_ports = pdata->num_ports;
> > >>>>>>>>> +    if (pdata->suspend_without_phy_exit)
> > >>>>>>>>> +        hcd->suspend_without_phy_exit = 1;
> > >>>>>>>>
> > >>>>>>>> Sorry if I missed this in the earlier discussions...  Is there any
> > >>>>>>>> possibility of adding a DT binding that could express this
> > >>>>>>>> requirement,
> > >>>>>>>> instead of putting it in the platform data?
> > >>>>>>>>
> > >>>>>>>> Alan Stern
> > >>>>>>>>
> > >>>>>>>
> > >>>>>>> Alan,
> > >>>>>>>
> > >>>>>>> That was my original approach but internal review suggested that
> > >>>>>>> I use
> > >>>>>>> pdata instead. Below is my original patch for:
> > >>>>>>
> > >>>>>> And the reason for that suggestion was really because it was
> > >>>>>> percevied
> > >>>>>> as encoding a driver behavior as a Device Tree property as opposed to
> > >>>>>> describing something that was inherently and strictly a hardware
> > >>>>>> behavior (therefore suitable for Device Tree).
> > >>>>>
> > >>>>> Right.  The best way to approach this problem is to identify and
> > >>>>> characterize the hardware behavior which makes this override
> > >>>>> necessary.
> > >>>>> Then _that_ can be added to DT, since it will be a property of the
> > >>>>> hardware rather than of the driver.
> > >>>>>
> > >>>>>>> Add the ability to skip calling the PHY's exit routine on suspend
> > >>>>>>> and the PHY's init routine on resume. This is to handle a USB PHY
> > >>>>>>> that should have it's power_off function called on suspend but
> > >>>>>>> cannot
> > >>>>>>> have it's exit function called because on exit it will disable the
> > >>>>>>> PHY to the point where register accesses to the Host Controllers
> > >>>>>>> using the PHY will be disabled and the host drivers will crash.
> > >>>>>
> > >>>>> What's special about this PHY?  Why does the exit function mess the
> > >>>>> PHY
> > >>>>> up?  Or to put it another way, why doesn't the exit function mess up
> > >>>>> other PHYs in the same way?
> > >>>>>
> > >>>>> For that matter, can we change the code so that suspend doesn't call
> > >>>>> the exit function for _any_ PHY?  Will just calling the power_off
> > >>>>> function be good enough?  If not, then why not?
> > >>>>>
> > >>>>> Alan Stern
> > >>>>>
> > >>>>
> > >>>> In our USB hardware the USB PHY supplies a clock for the EHCI/OHCI and
> > >>>> XHCI host controllers and if the PHY is totally shut down the EHCI,
> > >>>> OHCI
> > >>>> and XHCI registers will cause an exception if accessed and cause the
> > >>>> EHCI, OHCI and XHCI drivers to crash. There is always talk of fixing
> > >>>> this in the hardware by adding an aux clock that will takeover when the
> > >>>> PHY clock is shut down, but this hasn't happened yet. It seems like
> > >>>> "exit on suspend" still makes sense on systems that don't have this
> > >>>> problem (additional power savings?) so removing the exit on suspend for
> > >>>> all systems is not a good idea.
> > >>>
> > >>> Then in theory you should be able to add a Device Tree property which
> > >>> says that the PHY provides a clock for the USB host controller.  That
> > >>> is strictly a property of the hardware; it has nothing to do with the
> > >>> driver.  Therefore it is appropriate for DT.
> > >>
> > >> The very compatible string that is being allocated/defined for this
> > >> controller carries that information already, that is, if you probe a
> > >> "brcm,bcm7445-ohci" compatible then that means the controller has a
> > >> dependency on the PHY to supply its clock.
> > >>
> > >> Adding a property vs. keying on the compatible string makes sense if you
> > >> know there is at least a second consumer of that property (unless we
> > >> make it a broadcom specific property, in which case, it really is
> > >> redundant with the compatible string).
> > >>
> > >> Anyway, my grudge with that property was the name chosen initially,
> > >> which included an action to be performed by an implementation as opposed
> > >> to something purely descriptive. E.g: 'phy-supplies-clock' might be okay.
> > >>
> > >>>
> > >>> Wouldn't this solve your issue?
> > >>
> > >> It would not change much except that there is no need to much with
> > >> ohci-platform.c anymore, but ultimately that property needs to be read
> > >> by ohci-hcd.c and acted on, which would likely lead to the same amount
> > >> of changes as those present in patch #2 currently.
> > >>
> > > We also need this functionality in the EHCI and XHCI drivers and it's
> > > not the ohci-hcd.c module that needs to know, it's the core/phy.c module
> > > called from core/hcd.c.
> >
> > So in that regard the Device Tree property would actually scale a bit
> > better in that you would no longer need to modify the various
> > *hci-plat*.c files, if that is the way to go, then sure.
>
> Sounds like the phy needs to be a clock provider to the USB controller.
> Maybe that's a bit of overkill, but would be the most accurate.
>
> Otherwise, my preference is using the compatible string. IOW, we already
> have properties to handle this. If you don't want to use them, then use
> compatible rather than inventing something new and custom.

My previous commit used the compatible string to set the HCD flag
instead of a device tree property, but Alan Stern ask me to use a DT
binding instead. This has the advantage of having the code in one
place in the phy.c module instead of having to add it to the OHCI,
EHCI and XHCI platform drivers.

>
> At least last time I looked, there's a lack of support in the PHY API to
> handle various handshakes needed between phys and controllers like this.
> It's fairly easy for the controller to fetch info from a phy node, but
> the opposite is not so easy. It seems to me some API for controllers to
> set flags in the phy driver is needed.

The Generic PHY subsystem allows the PHY provider to export
phy_power_on/phy_power_off and phy_init/phy_exit functions for use by
the PHY consumer. The exact functionality of these routines seems to
vary among the different PHY provider drivers. The Broadcom USB PHY
driver expects the phy_power_on/phy_power_off to be used on
suspend/resume and the phy_init/phy_exit to be used by the consumer
driver's probe/remove routines. The new USB PHY consumer code does not
do this, instead it calls both power_off AND exit for both suspend
(not wakable) and remove. If you look at other PHY provider/consumer
implementations in the kernel tree you'll find examples of both these
methods but there are more examples that behave like the Broadcom PHY.
What I was trying to do was to use a DT property to tell the USB PHY
consumer code which method to use.

I guess the problem is that the current generic PHY api only allows
the consumer to put the PHY in 1 of 3 states, EXIT, OFF or ON. The
current USB PHY consumer code is using OFF for "suspend with wakeup"
and EXIT for "suspend without wakeup". The Broadcom PHY driver wants
OFF for either "suspend with wakeup" or "suspend without wakeup" and
EXIT when there are no consumers using the PHY. If the PHY API allowed
for 4 states, EXIT, OFF_NOT_WAKABLE, OFF_WAKABLE and ON then the
individual PHY provider could hide the differences, but this seems
hard to retrofit into the PHY subsystem.

Al

>
> Rob
>

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

end of thread, other threads:[~2018-11-13 21:54 UTC | newest]

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-05 21:31 [PATCH V3 0/6] Add XHCI, EHCI and OHCI support for Broadcom STB SoS's Al Cooper
2018-11-05 21:31 ` [PATCH V3 1/6] dt-bindings: Add Broadcom STB OHCI, EHCI and XHCI binding document Al Cooper
2018-11-05 21:31 ` [PATCH V3 2/6] usb: core: Add ability to skip phy exit on suspend and init on resume Al Cooper
2018-11-05 21:31 ` [PATCH V3 3/6] usb: xhci: xhci-plat: Add support for Broadcom STB SoC's Al Cooper
2018-11-05 21:31 ` [PATCH V3 4/6] usb: ohci-platform: " Al Cooper
2018-11-06 16:08   ` Alan Stern
2018-11-06 21:40     ` Al Cooper
2018-11-06 23:44       ` Florian Fainelli
2018-11-07 15:23         ` Alan Stern
2018-11-07 15:58           ` Al Cooper
2018-11-07 16:27             ` Alan Stern
2018-11-07 17:29               ` Florian Fainelli
2018-11-07 17:40                 ` Al Cooper
2018-11-07 18:11                   ` Florian Fainelli
     [not found]                     ` <20181112174532.GA14682@bogus>
2018-11-13 21:54                       ` Alan Cooper
2018-11-05 21:31 ` [PATCH V3 5/6] usb: ehci: Add new EHCI driver " Al Cooper
2018-11-05 21:31 ` [PATCH V3 6/6] usb: host: Add ability to build new Broadcom STB USB drivers Al Cooper
2018-11-06 11:09   ` Arnd Bergmann
2018-11-06 15:55     ` Alan Cooper
2018-11-06 18:22       ` Arnd Bergmann
2018-11-06 19:33         ` Alan Cooper

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