Re: [PATCH v8 6/9] usb: dwc3: qcom: Add multiport controller support for qcom wrapper
From: Krishna Kurapati PSSNV
Date: Fri May 26 2023 - 11:27:00 EST
On 5/26/2023 8:25 AM, Bjorn Andersson wrote:
+
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
{
u32 reg;
@@ -413,13 +423,16 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
{
u32 val;
int i, ret;
+ struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
if (qcom->is_suspended)
return 0;
- val = readl(qcom->qscratch_base + PWR_EVNT_IRQ_STAT_REG);
- if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
- dev_err(qcom->dev, "HS-PHY not in L2\n");
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
In the event that the dwc3 core fails to acquire or enable e.g. clocks
its drvdata will be NULL. If you then hit a runtime pm transition in the
dwc3-qcom glue you will dereference NULL here. (You can force this issue
by e.g. returning -EINVAL from dwc3_clk_enable()).
I looked at this once more, and realized that I missed the fact that
dwc3_qcom_is_host() will happily dereference the drvdata() just a few
lines further down...
So this is already broken.
So if you're peaking into qcom->dwc3 you need to handle the fact that
dwc might be NULL, here and in resume below.
We need to fix the dwc3 glue design, so that the glue and the core can
cooperate - and we have a few other use cases where this is needed (e.g.
usb_role_switch propagation to the glue code).
Hi Bjorn, Johan,
Thanks for the comments on this patch. I had some suggestions come in
from the team internally:
1. To use the notifier call available in drivers/usb/core/notify.c and
make sure that host mode is enabled. That way we can access dwc or xhci
without any issue.
2. For this particular case where we are trying to get info on number of
ports present (dwc->num_usb2_ports), we can add compatible data for
sc8280-mp and provide input to driver telling num ports is 4.
For the first idea, I tried it out as follows:
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..ce2f867d7c9a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -91,6 +91,9 @@ struct dwc3_qcom {
bool pm_suspended;
struct icc_path *icc_path_ddr;
struct icc_path *icc_path_apps;
+
+ bool in_host_mode;
+ struct notifier_block xhci_nb;
};
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset,
u32 val)
@@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct
dwc3_qcom *qcom)
icc_put(qcom->icc_path_apps);
}
-/* Only usable in contexts where the role can not change. */
-static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
-{
- struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
-
- return dwc->xhci;
-}
-
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct
dwc3_qcom *qcom)
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
@@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom,
bool wakeup)
* The role is stable during suspend as role switching is done
from a
* freezable workqueue.
*/
- if (dwc3_qcom_is_host(qcom) && wakeup) {
+ if (qcom->in_host_mode && wakeup) {
qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
dwc3_qcom_enable_interrupts(qcom);
}
@@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom,
bool wakeup)
if (!qcom->is_suspended)
return 0;
- if (dwc3_qcom_is_host(qcom) && wakeup)
+ if (qcom->in_host_mode && wakeup)
dwc3_qcom_disable_interrupts(qcom);
for (i = 0; i < qcom->num_clocks; i++) {
@@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq,
void *data)
* This is safe as role switching is done from a freezable
workqueue
* and the wakeup interrupts are disabled as part of resume.
*/
- if (dwc3_qcom_is_host(qcom))
+ if (qcom->in_host_mode)
pm_runtime_resume(&dwc->xhci->dev);
return IRQ_HANDLED;
@@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
return acpi_create_platform_device(adev, NULL);
}
+static int dwc3_xhci_event_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom,
xhci_nb);
+ struct usb_bus *ubus = ptr;
+ struct usb_hcd *hcd;
+
+ if (event != USB_BUS_ADD && event != USB_BUS_REMOVE)
+ return NOTIFY_DONE;
+
+ //TODO: Check whether event generated is for this qcom
controller or not
+
+ hcd = bus_to_hcd(ubus);
+ if (event == USB_BUS_ADD) {
+ /*
+ * Assuming both usb2 and usb3 roothubs wil
+ * be registered, wait for shared hcd to be
+ * registered to ensure that we are in host mode
+ * completely.
+ */
+ if (!usb_hcd_is_primary_hcd(hcd))
+ qcom->in_host_mode = true;
+ } else if (event == USB_BUS_REMOVE) {
+ /*
+ * While exiting host mode, primary hcd is
+ * removed at the end. Use it as indication
+ * that host stack has been removed successfully.
+ */
+ if (usb_hcd_is_primary_hcd(hcd))
+ qcom->in_host_mode = false;
+ }
+
+ return 0;
+}
+
static int dwc3_qcom_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
+ qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
+ usb_register_notify(&qcom->xhci_nb);
+
if (np)
ret = dwc3_qcom_of_register_core(pdev);
else
@@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
interconnect_exit:
dwc3_qcom_interconnect_exit(qcom);
depopulate:
+ usb_unregister_notify(&qcom->xhci_nb);
if (np)
of_platform_depopulate(&pdev->dev);
I tested the patch on sc7280 and see that wakeup from system suspend
works fine:
[ 3.807449] K: Before notify register
[ 3.810774] K: After notify register
[ 3.814006] K: calling dwc3_qcom_of_register_core
[ 3.818467] dwc3 a600000.usb: Adding to iommu group 8
[ 3.840031] K: before plat adde
[ 3.849151] K: before add hcd
[ 3.852219] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller
[ 3.857956] K: usb_notify_add_bus: Before notify add bus event: 3
[ 3.866481] K: dwc3_xhci_event_notifier recevied event: 3
[ 3.874007] K: dwc3_xhci_event_notifier: its a bus add/remove event
[ 3.880451] K: dwc3_xhci_event_notifier hcd added
[ 3.885301] K: in host mode: 0
[ 3.888441] K: usb_notify_add_bus: After notify add bus
[ 3.893815] xhci-hcd xhci-hcd.12.auto: new USB bus registered,
assigned bus number 1
[ 3.903799] xhci-hcd xhci-hcd.12.auto: hcc params 0x0230fe65 hci
version 0x110 quirks 0x0000000000010010
[ 3.913564] xhci-hcd xhci-hcd.12.auto: irq 214, io mem 0x0a600000
[ 3.919938] K: after add hcd
[ 3.922913] K: before add shared hcd
[ 3.926589] xhci-hcd xhci-hcd.12.auto: xHCI Host Controller
[ 3.932327] K: usb_notify_add_bus: Before notify add bus event: 3
[ 3.940973] K: dwc3_xhci_event_notifier recevied event: 3
[ 3.948492] K: dwc3_xhci_event_notifier: its a bus add/remove event
[ 3.954936] K: dwc3_xhci_event_notifier hcd added
[ 3.959770] K: dwc3_xhci_event_notifier: Its shared hcd
[ 3.965143] K: in host mode: 1
[ 3.968283] K: usb_notify_add_bus: After notify add bus
[ 3.973675] xhci-hcd xhci-hcd.12.auto: new USB bus registered,
assigned bus number 2
[ 3.981645] xhci-hcd xhci-hcd.12.auto: Host supports USB 3.0 SuperSpeed
[ 3.988537] usb usb1: New USB device found, idVendor=1d6b,
idProduct=0002, bcdDevice= 5.15
[ 3.997024] usb usb1: New USB device strings: Mfr=3, Product=2,
SerialNumber=1
[ 4.004438] usb usb1: Product: xHCI Host Controller
[ 4.009459] usb usb1: Manufacturer: Linux
5.15.90-25532-g92932f8e612f-dirty xhci-hcd
[ 4.017420] usb usb1: SerialNumber: xhci-hcd.12.auto
.....
[ 90.809188] Resume caused by IRQ 211, qcom_dwc3 DP_HS
Attached the patch file as well as in mail.
Let me know if this is a good enough way to fix the layering violations.
Regards,
Krishna,From 3a318ab38c4959b21df3cce6b933b6d0abe5eb4b Mon Sep 17 00:00:00 2001
From: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Date: Fri, 26 May 2023 15:03:07 +0530
Subject: [PATCH] usb: dwc3: qcom: Cleanup layering violations in dwc3 qcom
Implement host notifier in qcom to remove layering violations
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
---
drivers/usb/dwc3/dwc3-qcom.c | 56 +++++++++++++++++++++++++++++-------
1 file changed, 45 insertions(+), 11 deletions(-)
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 959fc925ca7c..ce2f867d7c9a 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -91,6 +91,9 @@ struct dwc3_qcom {
bool pm_suspended;
struct icc_path *icc_path_ddr;
struct icc_path *icc_path_apps;
+
+ bool in_host_mode;
+ struct notifier_block xhci_nb;
};
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -305,14 +308,6 @@ static void dwc3_qcom_interconnect_exit(struct dwc3_qcom *qcom)
icc_put(qcom->icc_path_apps);
}
-/* Only usable in contexts where the role can not change. */
-static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
-{
- struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
-
- return dwc->xhci;
-}
-
static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
@@ -432,7 +427,7 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
* The role is stable during suspend as role switching is done from a
* freezable workqueue.
*/
- if (dwc3_qcom_is_host(qcom) && wakeup) {
+ if (qcom->in_host_mode && wakeup) {
qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
dwc3_qcom_enable_interrupts(qcom);
}
@@ -450,7 +445,7 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
if (!qcom->is_suspended)
return 0;
- if (dwc3_qcom_is_host(qcom) && wakeup)
+ if (qcom->in_host_mode && wakeup)
dwc3_qcom_disable_interrupts(qcom);
for (i = 0; i < qcom->num_clocks; i++) {
@@ -488,7 +483,7 @@ static irqreturn_t qcom_dwc3_resume_irq(int irq, void *data)
* This is safe as role switching is done from a freezable workqueue
* and the wakeup interrupts are disabled as part of resume.
*/
- if (dwc3_qcom_is_host(qcom))
+ if (qcom->in_host_mode)
pm_runtime_resume(&dwc->xhci->dev);
return IRQ_HANDLED;
@@ -785,6 +780,41 @@ dwc3_qcom_create_urs_usb_platdev(struct device *dev)
return acpi_create_platform_device(adev, NULL);
}
+static int dwc3_xhci_event_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct dwc3_qcom *qcom = container_of(nb, struct dwc3_qcom, xhci_nb);
+ struct usb_bus *ubus = ptr;
+ struct usb_hcd *hcd;
+
+ if (event != USB_BUS_ADD && event != USB_BUS_REMOVE)
+ return NOTIFY_DONE;
+
+ //TODO: Check whether event generated is for this qcom controller or not
+
+ hcd = bus_to_hcd(ubus);
+ if (event == USB_BUS_ADD) {
+ /*
+ * Assuming both usb2 and usb3 roothubs wil
+ * be registered, wait for shared hcd to be
+ * registered to ensure that we are in host mode
+ * completely.
+ */
+ if (!usb_hcd_is_primary_hcd(hcd))
+ qcom->in_host_mode = true;
+ } else if (event == USB_BUS_REMOVE) {
+ /*
+ * While exiting host mode, primary hcd is
+ * removed at the end. Use it as indication
+ * that host stack has been removed successfully.
+ */
+ if (usb_hcd_is_primary_hcd(hcd))
+ qcom->in_host_mode = false;
+ }
+
+ return 0;
+}
+
static int dwc3_qcom_probe(struct platform_device *pdev)
{
struct device_node *np = pdev->dev.of_node;
@@ -884,6 +914,9 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
+ qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
+ usb_register_notify(&qcom->xhci_nb);
+
if (np)
ret = dwc3_qcom_of_register_core(pdev);
else
@@ -923,6 +956,7 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
interconnect_exit:
dwc3_qcom_interconnect_exit(qcom);
depopulate:
+ usb_unregister_notify(&qcom->xhci_nb);
if (np)
of_platform_depopulate(&pdev->dev);
else
--
2.40.0