Currently for all dwc3 devices, runtime is forbidden in kernel code.
Although it can be configured from userspace, qualcomm devices using
role switching with dr_mode "otg" would need to modify glue specific
qscratch registers without which connection done/ disconnect events
wouldn't be generated in device mode thus blocking suspend entry.
Also when in host mode, xhci and roothubs needs to be configured
to use_autosuspend accrordingly. More information regarding disconnect
event generation on QC targets is in [1].
The series introduces vendor hooks for passing on notifications
from core to glue layers. Since these hooks are to be registered
by glue to core, glue needs to have some sort of indication that
core probe hasn't been deferred or core probe hasn't failed. So
this series banks upon Bjorn's flattened device tree implementation [2]
for dwc3 core and dwc3-qcom. Since these patches ensure that we use
core layer as a library and probe is invoked in a controlled fashion
from qcom glue layer, we can register vendor hooks in this probe call.
While in host mode, usb-core notifiers have been utilised to ensure
we use_autosuspend for connected devices and roothubs.
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
Tests done:
1. Enumeration in device mode:
After creating symlinks to ffs.adb and writing to UDC node, ADB is up and
working in a stable way.
2. When none is written to UDC, device enters suspend.
3. When cable is removed, cable disconnect notification comes and when
qscratch registers are cleared properly, it is generating disconnect event
4. Device enters suspend upon removing cable (host and device mode)
5. In host mode, when autosuspend and wakeup are enabled from userspace
via the following commands, device enters runtime suspend:
echo enabled > /sys/bus/platform/devices/xhci-hcd.0.auto/power/wakeup
echo auto > /sys/bus/platform/devices/xhci-hcd.0.auto/power/control
echo 5000 > /sys/bus/platform/devices/xhci-hcd.0.auto/usb1/1-1/power/autosuspend_delay_ms
echo enabled > /sys/bus/platform/devices/xhci-hcd.0.auto/usb1/1-1/power/wakeup
echo auto > /sys/bus/platform/devices/xhci-hcd.0.auto/usb1/1-1/power/control
6. Upon removing cable in host mode, setmode brings back usb to device
mode (which is default setting), it enters suspend as cable is still
disconnected
7. When in host mode, if we enter runtime suspend with wakeup enabled,
clicking on buttons of headset are resuming the controller.
Issues still present:
During bootup if UDC is not written before dwc3 is suspended, then device
enters suspend and although writing to UDC is resuming the device and
setting run stop, controller is not generating events. At this stage if
a simple plug-in/plug-out is done, everything works fine.
Krishna Kurapati (8):
dt-bindings: usb: qcom,dwc3: Add bindings to enable runtime
usb: dwc3: core: Register vendor hooks for dwc3-qcom
usb: dwc3: qcom: Enable autosuspend for host mode
clk: qcom: gcc-sm8450: Keep usb30 prim gdsc on during runtime suspend
arm64: dts: qcom: Flatten sm8450 usb device node
arm: dts: qcom: Add pmic glink support for sm8450-qrd
arm: dts: qcom: Enable runtime for SM8450 QRD
usb: dwc3: core: Skip set_mode notification if cable is disconnected
.../devicetree/bindings/usb/qcom,dwc3.yaml | 5 +
arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 3 -
arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 48 ++++++-
.../dts/qcom/sm8450-sony-xperia-nagara.dtsi | 3 -
arch/arm64/boot/dts/qcom/sm8450.dtsi | 44 +++----
drivers/clk/qcom/gcc-sm8450.c | 1 +
drivers/usb/dwc3/core.c | 19 ++-
drivers/usb/dwc3/core.h | 50 ++++++-
drivers/usb/dwc3/drd.c | 13 ++
drivers/usb/dwc3/dwc3-qcom.c | 122 +++++++++++++++++-
10 files changed, 271 insertions(+), 37 deletions(-)
--
2.42.0
Add Pmic Glink support for sm8450-qrd to facilitate passing
of roe switch notifications generated by ADSP to dwc3 core
via ucsi and pmic glink's.
Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 46 ++++++++++++++++++++++++-
1 file changed, 45 insertions(+), 1 deletion(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
index fb800d24b00b..aec47e45284e 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
@@ -27,6 +27,40 @@ chosen {
stdout-path = "serial0:115200n8";
};
+ pmic-glink {
+ compatible = "qcom,sm8450-pmic-glink", "qcom,pmic-glink";
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ connector@0 {
+ compatible = "usb-c-connector";
+ reg = <0>;
+ power-role = "dual";
+ data-role = "dual";
+
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ port@0 {
+ reg = <0>;
+
+ pmic_glink_hs_in: endpoint {
+ remote-endpoint = <&usb_1_dwc3_hs>;
+ };
+ };
+
+ port@1 {
+ reg = <1>;
+
+ pmic_glink_ss_in: endpoint {
+ remote-endpoint = <&usb_1_dwc3_ss>;
+ };
+ };
+ };
+ };
+ };
+
vph_pwr: vph-pwr-regulator {
compatible = "regulator-fixed";
regulator-name = "vph_pwr";
@@ -453,7 +487,17 @@ &ufs_mem_phy {
&usb_1 {
status = "okay";
- dr_mode = "peripheral";
+
+ dr_mode = "otg";
+ usb-role-switch;
+};
+
+&usb_1_dwc3_hs {
+ remote-endpoint = <&pmic_glink_hs_in>;
+};
+
+&usb_1_dwc3_ss {
+ remote-endpoint = <&pmic_glink_ss_in>;
};
&usb_1_hsphy {
--
2.42.0
Currently on QC targets, the conndone/disconnect events in device mode are
generated by controller when software writes to QSCRATCH registers in qcom
glue layer rather than the vbus line being routed to dwc3 core IP for it
to recognize and generate these events.
We need to write '1' to UTMI_OTG_VBUS_VALID bit of QSCRATCH_HS_PHY_CTRL
register to generate a connection done event and "0" if we need to
generate a disconnect event during cable removal or mode switch. Exactly
what is done by "dwc3_qcom_vbus_override_enable" call in dwc3-qcom.
When the disconnect is not generated upon cable removal, the connected
flag of dwc3 is left marked as "true" and it blocks runtime suspend.
The goal of these vendor hooks is to let the mode change and cable removal
notifications from core reach the glue layers so that glue can take
necessary action.
Before flattening the device tree, glue driver is not sure when the core
probe was completed as core probe can be deferred. In this case, glue is
not sure when to register vendor hooks. So mandate enabling runtime only
for flattened device node platforms so that glue can know when to register
vendor hooks.
The following are the requirements aimed in this implementation:
1. When enum in device mode, Glue/core must stay active.
2. When cable is connected but UDC is not written yet, then glue/core
must be suspended.
3. Upon removing cable in device mode, the disconnect event must be
generated and unblock runtime suspend for dwc3 core.
Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/core.c | 15 ++++++-
drivers/usb/dwc3/core.h | 39 +++++++++++++++++-
drivers/usb/dwc3/drd.c | 9 ++++
drivers/usb/dwc3/dwc3-qcom.c | 79 ++++++++++++++++++++++++++++++++++--
4 files changed, 136 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 923a6c623850..53a8d92ad663 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -137,6 +137,8 @@ static void __dwc3_set_mode(struct work_struct *work)
if (!desired_dr_role)
goto out;
+ dwc3_notify_set_mode(dwc, desired_dr_role);
+
if (desired_dr_role == dwc->current_dr_role)
goto out;
@@ -1751,7 +1753,8 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
return 0;
}
-struct dwc3 *dwc3_probe(struct platform_device *pdev)
+struct dwc3 *dwc3_probe(struct platform_device *pdev,
+ struct dwc3_glue_data *glue_data)
{
struct device *dev = &pdev->dev;
struct resource *res, dwc_res;
@@ -1765,6 +1768,11 @@ struct dwc3 *dwc3_probe(struct platform_device *pdev)
dwc->dev = dev;
+ if (glue_data) {
+ dwc->glue_data = glue_data->glue_data;
+ dwc->glue_ops = glue_data->ops;
+ }
+
res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
if (!res) {
dev_err(dev, "missing memory resource\n");
@@ -1914,7 +1922,7 @@ static int dwc3_plat_probe(struct platform_device *pdev)
{
struct dwc3 *dwc;
- dwc = dwc3_probe(pdev);
+ dwc = dwc3_probe(pdev, NULL);
if (IS_ERR(dwc))
return PTR_ERR(dwc);
@@ -1925,6 +1933,9 @@ static int dwc3_plat_probe(struct platform_device *pdev)
void dwc3_remove(struct dwc3 *dwc)
{
+ dwc->glue_data = NULL;
+ dwc->glue_ops = NULL;
+
pm_runtime_get_sync(dwc->dev);
dwc3_core_exit_mode(dwc);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 3ec4a961befe..aefcb0d388b7 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -968,6 +968,23 @@ struct dwc3_scratchpad_array {
__le64 dma_adr[DWC3_MAX_HIBER_SCRATCHBUFS];
};
+/*
+ * struct dwc3_glue_ops - The ops indicate the notifications that
+ * need to be passed on to glue layer.
+ * @notify_cable_disconnect: Notify glue of cable removal
+ * irrespective of host or device mode.
+ * @set_mode: Notify glue before mode change is about to happen.
+ */
+struct dwc3_glue_ops {
+ void (*notify_cable_disconnect)(void *glue_data);
+ void (*set_mode)(void *glue_data, u32 desired_dr_role);
+};
+
+struct dwc3_glue_data {
+ void *glue_data;
+ struct dwc3_glue_ops *ops;
+};
+
/**
* struct dwc3 - representation of our controller
* @drd_work: workqueue used for role swapping
@@ -1124,6 +1141,9 @@ struct dwc3_scratchpad_array {
* @num_ep_resized: carries the current number endpoints which have had its tx
* fifo resized.
* @debug_root: root debugfs directory for this device to put its files in.
+ * @glue_data: Private data stored by core to be passed on to glue during
+ * role switch notifications.
+ * @glue_ops: Store pointers to glue notifcation callbacks.
*/
struct dwc3 {
struct work_struct drd_work;
@@ -1340,6 +1360,9 @@ struct dwc3 {
int last_fifo_depth;
int num_ep_resized;
struct dentry *debug_root;
+
+ void *glue_data;
+ const struct dwc3_glue_ops *glue_ops;
};
#define INCRX_BURST_MODE 0
@@ -1553,7 +1576,8 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc);
int dwc3_core_soft_reset(struct dwc3 *dwc);
-struct dwc3 *dwc3_probe(struct platform_device *pdev);
+struct dwc3 *dwc3_probe(struct platform_device *pdev,
+ struct dwc3_glue_data *glue_data);
void dwc3_remove(struct dwc3 *dwc);
int dwc3_runtime_suspend(struct dwc3 *dwc);
@@ -1563,6 +1587,19 @@ int dwc3_suspend(struct dwc3 *dwc);
int dwc3_resume(struct dwc3 *dwc);
void dwc3_complete(struct dwc3 *dwc);
+static inline void dwc3_notify_cable_disconnect(struct dwc3 *dwc)
+{
+ if (dwc->glue_ops && dwc->glue_ops->notify_cable_disconnect)
+ dwc->glue_ops->notify_cable_disconnect(dwc->glue_data);
+}
+
+static inline void dwc3_notify_set_mode(struct dwc3 *dwc,
+ u32 desired_dr_role)
+{
+ if (dwc->glue_ops && dwc->glue_ops->set_mode)
+ dwc->glue_ops->set_mode(dwc->glue_data, desired_dr_role);
+}
+
#if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
int dwc3_host_init(struct dwc3 *dwc);
void dwc3_host_exit(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf241769a..947faeef0e4d 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -461,6 +461,15 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
break;
}
+ /*
+ * When cable is removed, the role changes to default mode.
+ * In the case we are in device mode and removed the cable, the
+ * glue needs to know that we are disconnected. It must not notify
+ * the change of mode to default mode.
+ */
+ if (role == USB_ROLE_NONE)
+ dwc3_notify_cable_disconnect(dwc);
+
dwc3_set_mode(dwc, mode);
return 0;
}
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3c9a2b5cd559..4013a5e6c6c0 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -88,6 +88,9 @@ struct dwc3_qcom {
bool pm_suspended;
struct icc_path *icc_path_ddr;
struct icc_path *icc_path_apps;
+
+ bool enable_rt;
+ enum usb_role current_role;
};
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -673,16 +676,71 @@ static const struct software_node dwc3_qcom_swnode = {
.properties = dwc3_qcom_acpi_properties,
};
+static void dwc3_qcom_handle_cable_disconnect(void *data)
+{
+ struct dwc3_qcom *qcom = (struct dwc3_qcom *)data;
+ /*
+ * If we are in device mode and get a cable disconnect,
+ * handle it by clearing OTG_VBUS_VALID bit in wrapper.
+ * The next set_mode to default role can be ignored.
+ */
+ if (qcom->current_role == USB_ROLE_DEVICE) {
+ pm_runtime_get_sync(qcom->dev);
+ dwc3_qcom_vbus_override_enable(qcom, false);
+ pm_runtime_put_autosuspend(qcom->dev);
+ }
+
+ pm_runtime_mark_last_busy(qcom->dev);
+ qcom->current_role = USB_ROLE_NONE;
+}
+
+static void dwc3_qcom_handle_set_mode(void *data, u32 desired_dr_role)
+{
+ struct dwc3_qcom *qcom = (struct dwc3_qcom *)data;
+
+ /*
+ * If we are in device mode and get a cable disconnect,
+ * handle it by clearing OTG_VBUS_VALID bit in wrapper.
+ * The next set_mode to default role can be ignored and
+ * so the OTG_VBUS_VALID should be set iff the current role
+ * is NONE and we need to enter DEVICE mode.
+ */
+ if ((qcom->current_role == USB_ROLE_NONE) &&
+ (desired_dr_role == DWC3_GCTL_PRTCAP_DEVICE)) {
+ dwc3_qcom_vbus_override_enable(qcom, true);
+ qcom->current_role = USB_ROLE_DEVICE;
+ } else if ((desired_dr_role == DWC3_GCTL_PRTCAP_HOST) &&
+ (qcom->current_role != USB_ROLE_HOST)) {
+ qcom->current_role = USB_ROLE_HOST;
+ }
+
+ pm_runtime_mark_last_busy(qcom->dev);
+}
+
+struct dwc3_glue_ops dwc3_qcom_glue_hooks = {
+ .notify_cable_disconnect = dwc3_qcom_handle_cable_disconnect,
+ .set_mode = dwc3_qcom_handle_set_mode,
+};
+
static int dwc3_qcom_probe_core(struct platform_device *pdev, struct dwc3_qcom *qcom)
{
struct dwc3 *dwc;
- dwc = dwc3_probe(pdev);
+ struct dwc3_glue_data qcom_glue_data = {
+ .glue_data = qcom,
+ .ops = &dwc3_qcom_glue_hooks,
+ };
+
+ dwc = dwc3_probe(pdev,
+ qcom->enable_rt ? &qcom_glue_data : NULL);
if (IS_ERR(dwc))
return PTR_ERR(dwc);
qcom->dwc = dwc;
+ if (qcom->enable_rt)
+ pm_runtime_allow(qcom->dev);
+
return 0;
}
@@ -897,6 +955,23 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (ignore_pipe_clk)
dwc3_qcom_select_utmi_clk(qcom);
+ qcom->enable_rt = device_property_read_bool(dev,
+ "qcom,enable-rt");
+ if (!legacy_binding) {
+ /*
+ * If we are enabling runtime, then we are using flattened
+ * device implementation.
+ */
+ qcom->mode = usb_get_dr_mode(dev);
+
+ if (qcom->mode == USB_DR_MODE_HOST)
+ qcom->current_role = USB_ROLE_HOST;
+ else if (qcom->mode == USB_DR_MODE_PERIPHERAL)
+ qcom->current_role = USB_ROLE_DEVICE;
+ else
+ qcom->current_role = USB_ROLE_NONE;
+ }
+
if (legacy_binding)
ret = dwc3_qcom_of_register_core(pdev);
else
@@ -913,8 +988,6 @@ static int dwc3_qcom_probe(struct platform_device *pdev)
if (qcom->dwc_dev)
qcom->mode = usb_get_dr_mode(&qcom->dwc_dev->dev);
- else
- qcom->mode = usb_get_dr_mode(dev);
/* enable vbus override for device mode */
if (qcom->mode != USB_DR_MODE_HOST)
--
2.42.0
When in host mode, enable autosuspend for xhci and root hubs.
a) Register a vendor call back to get information of successful role
change by core. When xhci is enumerated successfully, configure it to
use_autosuspend. The decision of whether or not to do runtime_allow for
xhci node is left to userspace:
(echo auto > */xhci-auto/power/control).
b) Register to usb-core notifications in set_mode vendor callback to
identify when root hubs are being created. Configure them to
use_autosuspend accordingly.
c) Configure any connected device to use_autosuspend. In general for
mobile use cases, autosuspend is enabled and wakeup is enabled only
for hubs and audio devices. So let userspace choose to configure
autosuspend_delay and wakeup capability of connected devices.
Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/core.c | 3 +++
drivers/usb/dwc3/core.h | 9 ++++++++
drivers/usb/dwc3/dwc3-qcom.c | 42 ++++++++++++++++++++++++++++++++++++
3 files changed, 54 insertions(+)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 53a8d92ad663..b4d1d1c98dd5 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -233,6 +233,9 @@ static void __dwc3_set_mode(struct work_struct *work)
break;
}
+ if (!ret)
+ dwc3_notify_mode_changed(dwc, dwc->current_dr_role);
+
out:
pm_runtime_mark_last_busy(dwc->dev);
pm_runtime_put_autosuspend(dwc->dev);
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index aefcb0d388b7..5ed7fd5eb776 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -974,10 +974,12 @@ struct dwc3_scratchpad_array {
* @notify_cable_disconnect: Notify glue of cable removal
* irrespective of host or device mode.
* @set_mode: Notify glue before mode change is about to happen.
+ * @mode_changed: Notify glue that mode change was done successfully
*/
struct dwc3_glue_ops {
void (*notify_cable_disconnect)(void *glue_data);
void (*set_mode)(void *glue_data, u32 desired_dr_role);
+ void (*mode_changed)(void *glue_data, u32 current_dr_role);
};
struct dwc3_glue_data {
@@ -1600,6 +1602,13 @@ static inline void dwc3_notify_set_mode(struct dwc3 *dwc,
dwc->glue_ops->set_mode(dwc->glue_data, desired_dr_role);
}
+static inline void dwc3_notify_mode_changed(struct dwc3 *dwc,
+ u32 current_dr_role)
+{
+ if (dwc->glue_ops && dwc->glue_ops->mode_changed)
+ dwc->glue_ops->mode_changed(dwc->glue_data, current_dr_role);
+}
+
#if IS_ENABLED(CONFIG_USB_DWC3_HOST) || IS_ENABLED(CONFIG_USB_DWC3_DUAL_ROLE)
int dwc3_host_init(struct dwc3 *dwc);
void dwc3_host_exit(struct dwc3 *dwc);
diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 4013a5e6c6c0..9c7b23888f11 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -91,6 +91,7 @@ struct dwc3_qcom {
bool enable_rt;
enum usb_role current_role;
+ struct notifier_block xhci_nb;
};
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -676,6 +677,27 @@ static const struct software_node dwc3_qcom_swnode = {
.properties = dwc3_qcom_acpi_properties,
};
+static int dwc3_xhci_event_notifier(struct notifier_block *nb,
+ unsigned long event, void *ptr)
+{
+ struct usb_device *udev = ptr;
+
+ if (event != USB_DEVICE_ADD)
+ return NOTIFY_DONE;
+
+ /*
+ * If this is a roothub corresponding to this controller, enable autosuspend
+ */
+ if (!udev->parent) {
+ pm_runtime_use_autosuspend(&udev->dev);
+ pm_runtime_set_autosuspend_delay(&udev->dev, 1000);
+ }
+
+ usb_mark_last_busy(udev);
+
+ return NOTIFY_DONE;
+}
+
static void dwc3_qcom_handle_cable_disconnect(void *data)
{
struct dwc3_qcom *qcom = (struct dwc3_qcom *)data;
@@ -688,6 +710,8 @@ static void dwc3_qcom_handle_cable_disconnect(void *data)
pm_runtime_get_sync(qcom->dev);
dwc3_qcom_vbus_override_enable(qcom, false);
pm_runtime_put_autosuspend(qcom->dev);
+ } else if (qcom->current_role == USB_ROLE_HOST) {
+ usb_unregister_notify(&qcom->xhci_nb);
}
pm_runtime_mark_last_busy(qcom->dev);
@@ -711,15 +735,33 @@ static void dwc3_qcom_handle_set_mode(void *data, u32 desired_dr_role)
qcom->current_role = USB_ROLE_DEVICE;
} else if ((desired_dr_role == DWC3_GCTL_PRTCAP_HOST) &&
(qcom->current_role != USB_ROLE_HOST)) {
+ qcom->xhci_nb.notifier_call = dwc3_xhci_event_notifier;
+ usb_register_notify(&qcom->xhci_nb);
qcom->current_role = USB_ROLE_HOST;
}
pm_runtime_mark_last_busy(qcom->dev);
}
+static void dwc3_qcom_handle_mode_changed(void *data, u32 current_dr_role)
+{
+ struct dwc3_qcom *qcom = (struct dwc3_qcom *)data;
+
+ /*
+ * XHCI platform device is allocated upon host init.
+ * So ensure we are in host mode before enabling autosuspend.
+ */
+ if ((current_dr_role == DWC3_GCTL_PRTCAP_HOST) &&
+ (qcom->current_role == USB_ROLE_HOST)) {
+ pm_runtime_use_autosuspend(&qcom->dwc->xhci->dev);
+ pm_runtime_set_autosuspend_delay(&qcom->dwc->xhci->dev, 0);
+ }
+}
+
struct dwc3_glue_ops dwc3_qcom_glue_hooks = {
.notify_cable_disconnect = dwc3_qcom_handle_cable_disconnect,
.set_mode = dwc3_qcom_handle_set_mode,
+ .mode_changed = dwc3_qcom_handle_mode_changed,
};
static int dwc3_qcom_probe_core(struct platform_device *pdev, struct dwc3_qcom *qcom)
--
2.42.0
Add enable-rt binding to let the device register vendor hooks to
core and facilitate runtime suspend and resume.
Signed-off-by: Krishna Kurapati <[email protected]>
---
Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
index cb50261c6a36..788d9c510abc 100644
--- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
+++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
@@ -151,6 +151,11 @@ properties:
HS/FS/LS modes are supported.
type: boolean
+ qcom,enable-rt:
+ description:
+ If present, register vendor hooks to facilitate runtime suspend/resume
+ type: boolean
+
wakeup-source: true
# Required child node:
--
2.42.0
Flatten usb device node by removing usb_1_dwc3 node.
Verified working of ADB in device mode and enumeration of headset
in host mode.
Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450-hdk.dts | 3 --
arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 3 --
.../dts/qcom/sm8450-sony-xperia-nagara.dtsi | 3 --
arch/arm64/boot/dts/qcom/sm8450.dtsi | 44 +++++++++----------
4 files changed, 21 insertions(+), 32 deletions(-)
diff --git a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
index bc4c125d1832..73b7c4f5276c 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-hdk.dts
@@ -805,9 +805,6 @@ &ufs_mem_phy {
&usb_1 {
status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "otg";
usb-role-switch;
};
diff --git a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
index 65a94dfaf5ae..fb800d24b00b 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
@@ -453,9 +453,6 @@ &ufs_mem_phy {
&usb_1 {
status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "peripheral";
};
diff --git a/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi b/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
index 001fb2723fbb..9ba6db3d54f8 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450-sony-xperia-nagara.dtsi
@@ -778,9 +778,6 @@ &uart7 {
&usb_1 {
status = "okay";
-};
-
-&usb_1_dwc3 {
dr_mode = "peripheral";
};
diff --git a/arch/arm64/boot/dts/qcom/sm8450.dtsi b/arch/arm64/boot/dts/qcom/sm8450.dtsi
index 5cd7296c7660..3785a4625aff 100644
--- a/arch/arm64/boot/dts/qcom/sm8450.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8450.dtsi
@@ -4270,7 +4270,7 @@ opp-202000000 {
usb_1: usb@a6f8800 {
compatible = "qcom,sm8450-dwc3", "qcom,dwc3";
- reg = <0 0x0a6f8800 0 0x400>;
+ reg = <0 0x0a600000 0 0x200000>;
status = "disabled";
#address-cells = <2>;
#size-cells = <2>;
@@ -4293,11 +4293,13 @@ usb_1: usb@a6f8800 {
<&gcc GCC_USB30_PRIM_MASTER_CLK>;
assigned-clock-rates = <19200000>, <200000000>;
- interrupts-extended = <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+ interrupts-extended = <&intc GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
<&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
<&pdc 15 IRQ_TYPE_EDGE_BOTH>,
<&pdc 14 IRQ_TYPE_EDGE_BOTH>;
- interrupt-names = "hs_phy_irq",
+ interrupt-names = "dwc_usb3",
+ "hs_phy_irq",
"ss_phy_irq",
"dm_hs_phy_irq",
"dp_hs_phy_irq";
@@ -4310,32 +4312,28 @@ usb_1: usb@a6f8800 {
<&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_0 0>;
interconnect-names = "usb-ddr", "apps-usb";
- usb_1_dwc3: usb@a600000 {
- compatible = "snps,dwc3";
- reg = <0 0x0a600000 0 0xcd00>;
- interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
- iommus = <&apps_smmu 0x0 0x0>;
- snps,dis_u2_susphy_quirk;
- snps,dis_enblslpm_quirk;
- phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
- phy-names = "usb2-phy", "usb3-phy";
+ iommus = <&apps_smmu 0x0 0x0>;
+ snps,dis_u2_susphy_quirk;
+ snps,dis_enblslpm_quirk;
- ports {
- #address-cells = <1>;
- #size-cells = <0>;
+ phys = <&usb_1_hsphy>, <&usb_1_qmpphy QMP_USB43DP_USB3_PHY>;
+ phy-names = "usb2-phy", "usb3-phy";
- port@0 {
- reg = <0>;
+ ports {
+ #address-cells = <1>;
+ #size-cells = <0>;
- usb_1_dwc3_hs: endpoint {
- };
+ port@0 {
+ reg = <0>;
+
+ usb_1_dwc3_hs: endpoint {
};
+ };
- port@1 {
- reg = <1>;
+ port@1 {
+ reg = <1>;
- usb_1_dwc3_ss: endpoint {
- };
+ usb_1_dwc3_ss: endpoint {
};
};
};
--
2.42.0
Enable runtime and wakeup source for SM8450 QRD platform.
Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 3 +++
1 file changed, 3 insertions(+)
diff --git a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
index aec47e45284e..d3e8fe7a37ec 100644
--- a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
+++ b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
@@ -490,6 +490,9 @@ &usb_1 {
dr_mode = "otg";
usb-role-switch;
+
+ qcom,enable-rt;
+ wakeup-source;
};
&usb_1_dwc3_hs {
--
2.42.0
In device mode use cases, the following sequence of actions are observed:
1. Cable disconnect happens and clears qscratch HS_PHY_CTRL_REG properly
2. Disconnect event is generated and "connected" flag turns false.
3. Then the setmode notification from core goes to glue
4. Glue will set back the qscratch HS_PHY_CTRL_REG bits again.
At this point, since the cable is removed, setting qscratch bits shouldn't
affect anything. But it is observed that after setting this bits, the
controller generated Event-0x101 and Event-0x30601 (bus reset and suspend)
in order. In bus reset, we set back the "connected" flag and this blocks
suspend again.
So send set_mode call only if the cable is connected, else skip it.
Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/core.c | 3 ++-
drivers/usb/dwc3/core.h | 2 ++
drivers/usb/dwc3/drd.c | 6 +++++-
3 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index b4d1d1c98dd5..6ef1e3558384 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -137,7 +137,8 @@ static void __dwc3_set_mode(struct work_struct *work)
if (!desired_dr_role)
goto out;
- dwc3_notify_set_mode(dwc, desired_dr_role);
+ if (dwc->cable_disconnected == false)
+ dwc3_notify_set_mode(dwc, desired_dr_role);
if (desired_dr_role == dwc->current_dr_role)
goto out;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 5ed7fd5eb776..1b79c407a798 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1365,6 +1365,8 @@ struct dwc3 {
void *glue_data;
const struct dwc3_glue_ops *glue_ops;
+
+ bool cable_disconnected;
};
#define INCRX_BURST_MODE 0
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 947faeef0e4d..b3a87c40c4f1 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -446,6 +446,8 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
struct dwc3 *dwc = usb_role_switch_get_drvdata(sw);
u32 mode;
+ dwc->cable_disconnected = false;
+
switch (role) {
case USB_ROLE_HOST:
mode = DWC3_GCTL_PRTCAP_HOST;
@@ -467,8 +469,10 @@ static int dwc3_usb_role_switch_set(struct usb_role_switch *sw,
* glue needs to know that we are disconnected. It must not notify
* the change of mode to default mode.
*/
- if (role == USB_ROLE_NONE)
+ if (role == USB_ROLE_NONE) {
+ dwc->cable_disconnected = true;
dwc3_notify_cable_disconnect(dwc);
+ }
dwc3_set_mode(dwc, mode);
return 0;
--
2.42.0
On 17/10/2023 15:18, Krishna Kurapati wrote:
> Add enable-rt binding to let the device register vendor hooks to
> core and facilitate runtime suspend and resume.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index cb50261c6a36..788d9c510abc 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -151,6 +151,11 @@ properties:
> HS/FS/LS modes are supported.
> type: boolean
>
> + qcom,enable-rt:
> + description:
> + If present, register vendor hooks to facilitate runtime suspend/resume
You described the desired Linux feature or behavior, not the actual
hardware. The bindings are about the latter, so instead you need to
rephrase the property and its description to match actual hardware
capabilities/features/configuration etc.
Best regards,
Krzysztof
On 17/10/2023 15:18, Krishna Kurapati wrote:
> Add Pmic Glink support for sm8450-qrd to facilitate passing
> of roe switch notifications generated by ADSP to dwc3 core
> via ucsi and pmic glink's.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
Please use subject prefixes matching the subsystem. You can get them for
example with `git log --oneline -- DIRECTORY_OR_FILE` on the directory
your patch is touching.
> ---
> arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 46 ++++++++++++++++++++++++-
> 1 file changed, 45 insertions(+), 1 deletion(-)
>
With subject fixes:
Reviewed-by: Krzysztof Kozlowski <[email protected]>
Best regards,
Krzysztof
On 17/10/2023 15:18, Krishna Kurapati wrote:
> Enable runtime and wakeup source for SM8450 QRD platform.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sm8450-qrd.dts | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
> index aec47e45284e..d3e8fe7a37ec 100644
> --- a/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
> +++ b/arch/arm64/boot/dts/qcom/sm8450-qrd.dts
> @@ -490,6 +490,9 @@ &usb_1 {
>
> dr_mode = "otg";
> usb-role-switch;
> +
> + qcom,enable-rt;
NAK. Not a HW property.
Best regards,
Krzysztof
On 10/17/2023 10:49 PM, Krzysztof Kozlowski wrote:
> On 17/10/2023 15:18, Krishna Kurapati wrote:
>> Add enable-rt binding to let the device register vendor hooks to
>> core and facilitate runtime suspend and resume.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> index cb50261c6a36..788d9c510abc 100644
>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> @@ -151,6 +151,11 @@ properties:
>> HS/FS/LS modes are supported.
>> type: boolean
>>
>> + qcom,enable-rt:
>> + description:
>> + If present, register vendor hooks to facilitate runtime suspend/resume
>
> You described the desired Linux feature or behavior, not the actual
> hardware. The bindings are about the latter, so instead you need to
> rephrase the property and its description to match actual hardware
> capabilities/features/configuration etc.
>
Hi Krzysztof,
Thanks for the review. Although it sounds like its a Linux property,
internally what it does is configuring qscratch registers properly when
(dr_mode == OTG)
Would it be fine to rephrase the property name to
"qcom,config-qscratch" and to make it dependent on dr_mode and
usb-role-switch properties ? Would it be possible to make such a
dependency in bindings ?
Regards,
Krishna,
On 17/10/2023 19:46, Krishna Kurapati PSSNV wrote:
>
>
> On 10/17/2023 10:49 PM, Krzysztof Kozlowski wrote:
>> On 17/10/2023 15:18, Krishna Kurapati wrote:
>>> Add enable-rt binding to let the device register vendor hooks to
>>> core and facilitate runtime suspend and resume.
>>>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>> ---
>>> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
>>> 1 file changed, 5 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> index cb50261c6a36..788d9c510abc 100644
>>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>>> @@ -151,6 +151,11 @@ properties:
>>> HS/FS/LS modes are supported.
>>> type: boolean
>>>
>>> + qcom,enable-rt:
>>> + description:
>>> + If present, register vendor hooks to facilitate runtime suspend/resume
>>
>> You described the desired Linux feature or behavior, not the actual
>> hardware. The bindings are about the latter, so instead you need to
>> rephrase the property and its description to match actual hardware
>> capabilities/features/configuration etc.
>>
>
> Hi Krzysztof,
>
> Thanks for the review. Although it sounds like its a Linux property,
> internally what it does is configuring qscratch registers properly when
> (dr_mode == OTG)
That's not what you wrote. You wrote "register vendor hooks". Really,
this is nothing to do with hardware.
>
> Would it be fine to rephrase the property name to
> "qcom,config-qscratch" and to make it dependent on dr_mode and
> usb-role-switch properties ? Would it be possible to make such a
> dependency in bindings ?
Examples are in: example-schema or my talk.
Best regards,
Krzysztof
On 10/17/23 15:18, Krishna Kurapati wrote:
> Add Pmic Glink support for sm8450-qrd to facilitate passing
> of roe switch notifications generated by ADSP to dwc3 core
> via ucsi and pmic glink's.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
No phy+redriver+dp configuration?
Konrad
On 10/27/2023 1:11 AM, Konrad Dybcio wrote:
>
>
> On 10/17/23 15:18, Krishna Kurapati wrote:
>> Add Pmic Glink support for sm8450-qrd to facilitate passing
>> of roe switch notifications generated by ADSP to dwc3 core
>> via ucsi and pmic glink's.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
> No phy+redriver+dp configuration?
>
Hi Konrad,
Did you mean adding the following node:
typec-mux@42 {
compatible = "fcs,fsa4480";
reg = <0x42>;
interrupts-extended = <&tlmm 2 IRQ_TYPE_LEVEL_LOW>;
vcc-supply = <&vreg_bob>;
mode-switch;
orientation-switch;
port {
fsa4480_sbu_mux: endpoint {
remote-endpoint = <&pmic_glink_sbu>;
};
};
};
and then adding port-2 for pmic_glink ?
Usually for role-switch the port-0/1 defined in this patch are
sufficient. Also if I added it, I don't have a way to currently test it.
So skipped this node. I will try and see if I can test it and add it if
possible.
Regards,
Krishna,
On 17/10/2023 14:18, Krishna Kurapati wrote:
> Add enable-rt binding to let the device register vendor hooks to
> core and facilitate runtime suspend and resume.
Hi Krishna,
From reading through these patches, it's not clear to me why this
behaviour should be conditional on a new devicetree property. Are there
some platforms where this behaviour would be undesirable? And if so then
would it be possible to determine this based on the QSCRATCH registers?
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> index cb50261c6a36..788d9c510abc 100644
> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
> @@ -151,6 +151,11 @@ properties:
> HS/FS/LS modes are supported.
> type: boolean
>
> + qcom,enable-rt:
> + description:
> + If present, register vendor hooks to facilitate runtime suspend/resume
> + type: boolean
A Krzysztof pointed out, properties should define the hardware
behaviour, not tot the implementation details. For this case the
hardware isn't wired up to vbus, so maybe something like "qcom,no-vbus"?
> +
> wakeup-source: true
>
> # Required child node:
--
// Caleb (they/them)
On 11/2/2023 10:03 PM, Caleb Connolly wrote:
>
>
> On 17/10/2023 14:18, Krishna Kurapati wrote:
>> Add enable-rt binding to let the device register vendor hooks to
>> core and facilitate runtime suspend and resume.
>
> Hi Krishna,
>
> From reading through these patches, it's not clear to me why this
> behaviour should be conditional on a new devicetree property. Are there
> some platforms where this behaviour would be undesirable? And if so then
> would it be possible to determine this based on the QSCRATCH registers?
>>
Hi Caleb,
There are two types of platforms, some use extcon and some use
role-switch to deliver vbus/id notifications. Extcon targets already
have this qscratch modifications present today in vbus and id handlers.
But for role-switch based targets we don't have any way to get this
notification to dwc3-qcom. In this implementation, I wanted to get those
notications from core to glue and for this we implenented vendor hooks.
The property added has been used to do two things:
1. Register glue's vendor hooks to core driver
2. Do runtime_allow for glue (and by default for core as the dt is not
flattened)
In case of extcon, we don't want to register vendor hooks as
notifications are not necessary.
For xhci, we opted to enable runtime from userspace.
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> Documentation/devicetree/bindings/usb/qcom,dwc3.yaml | 5 +++++
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> index cb50261c6a36..788d9c510abc 100644
>> --- a/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> +++ b/Documentation/devicetree/bindings/usb/qcom,dwc3.yaml
>> @@ -151,6 +151,11 @@ properties:
>> HS/FS/LS modes are supported.
>> type: boolean
>> + qcom,enable-rt:
>> + description:
>> + If present, register vendor hooks to facilitate runtime
>> suspend/resume
>> + type: boolean
>
> A Krzysztof pointed out, properties should define the hardware
> behaviour, not tot the implementation details. For this case the
> hardware isn't wired up to vbus, so maybe something like "qcom,no-vbus"?
>> +
On all targets, vbus is not routed to hardware. This vbus toggle
indication is given to controller via qscratch only.
Regards,
Krishna,
> Hi Caleb,
>
> There are two types of platforms, some use extcon and some use
> role-switch to deliver vbus/id notifications. Extcon targets already
> have this qscratch modifications present today in vbus and id handlers.
> But for role-switch based targets we don't have any way to get this
> notification to dwc3-qcom. In this implementation, I wanted to get those
> notications from core to glue and for this we implenented vendor hooks.
>
> The property added has been used to do two things:
>
> 1. Register glue's vendor hooks to core driver
> 2. Do runtime_allow for glue (and by default for core as the dt is not
> flattened)
>
> In case of extcon, we don't want to register vendor hooks as
> notifications are not necessary.
Could it just be enabled when role_switch is present then?
>
> For xhci, we opted to enable runtime from userspace.
>>> HS/FS/LS modes are supported.
>>> type: boolean
>>> + qcom,enable-rt:
>>> + description:
>>> + If present, register vendor hooks to facilitate runtime
>>> suspend/resume
>>> + type: boolean
>>
>> A Krzysztof pointed out, properties should define the hardware
>> behaviour, not tot the implementation details. For this case the
>> hardware isn't wired up to vbus, so maybe something like "qcom,no-vbus"?
>>> +
>
> On all targets, vbus is not routed to hardware. This vbus toggle
> indication is given to controller via qscratch only.
>
> Regards,
> Krishna,
--
// Caleb (they/them)
On 11/3/2023 12:10 AM, Caleb Connolly wrote:
>> Hi Caleb,
>>
>> There are two types of platforms, some use extcon and some use
>> role-switch to deliver vbus/id notifications. Extcon targets already
>> have this qscratch modifications present today in vbus and id
>> handlers. But for role-switch based targets we don't have any way to
>> get this notification to dwc3-qcom. In this implementation, I wanted
>> to get those notications from core to glue and for this we implenented
>> vendor hooks.
>>
>> The property added has been used to do two things:
>>
>> 1. Register glue's vendor hooks to core driver
>> 2. Do runtime_allow for glue (and by default for core as the dt is not
>> flattened)
>>
>> In case of extcon, we don't want to register vendor hooks as
>> notifications are not necessary.
>
> Could it just be enabled when role_switch is present then?
>>
So we would register vendor hooks when usb-role-switch is present but
don't do runtime allow, and leave that option to user space right ?
I think it would work and we can do away with the binding completely.
Will wait for comments from other folks as well on this approach.
Thanks for the review,
Krishna,
On 03/11/2023 05:34, Krishna Kurapati PSSNV wrote:
>
>
> On 11/3/2023 12:10 AM, Caleb Connolly wrote:
>>> Hi Caleb,
>>>
>>> There are two types of platforms, some use extcon and some use
>>> role-switch to deliver vbus/id notifications. Extcon targets already
>>> have this qscratch modifications present today in vbus and id
>>> handlers. But for role-switch based targets we don't have any way to
>>> get this notification to dwc3-qcom. In this implementation, I wanted
>>> to get those notications from core to glue and for this we
>>> implenented vendor hooks.
>>>
>>> The property added has been used to do two things:
>>>
>>> 1. Register glue's vendor hooks to core driver
>>> 2. Do runtime_allow for glue (and by default for core as the dt is
>>> not flattened)
>>>
>>> In case of extcon, we don't want to register vendor hooks as
>>> notifications are not necessary.
>>
>> Could it just be enabled when role_switch is present then?
>>>
>
> So we would register vendor hooks when usb-role-switch is present but
> don't do runtime allow, and leave that option to user space right ?
> I think it would work and we can do away with the binding completely.
Can we still enable runtime suspend? Maybe someone else wants to chime
in here, but I'd guess that it's preferable to have it enabled by
default, particularly for devices like phones. Or are there side effects
from this?
>
> Will wait for comments from other folks as well on this approach.
Sounds good, thanks!
>
> Thanks for the review,
> Krishna,
--
// Caleb (they/them)
On 17/10/2023 14:18, Krishna Kurapati wrote:
> Currently on QC targets, the conndone/disconnect events in device mode are
> generated by controller when software writes to QSCRATCH registers in qcom
> glue layer rather than the vbus line being routed to dwc3 core IP for it
> to recognize and generate these events.
>
> We need to write '1' to UTMI_OTG_VBUS_VALID bit of QSCRATCH_HS_PHY_CTRL
> register to generate a connection done event and "0" if we need to
> generate a disconnect event during cable removal or mode switch. Exactly
> what is done by "dwc3_qcom_vbus_override_enable" call in dwc3-qcom.
>
> When the disconnect is not generated upon cable removal, the connected
> flag of dwc3 is left marked as "true" and it blocks runtime suspend.
>
> The goal of these vendor hooks is to let the mode change and cable removal
> notifications from core reach the glue layers so that glue can take
> necessary action.
>
> Before flattening the device tree, glue driver is not sure when the core
> probe was completed as core probe can be deferred. In this case, glue is
> not sure when to register vendor hooks. So mandate enabling runtime only
> for flattened device node platforms so that glue can know when to register
> vendor hooks.
>
> The following are the requirements aimed in this implementation:
>
> 1. When enum in device mode, Glue/core must stay active.
>
> 2. When cable is connected but UDC is not written yet, then glue/core
> must be suspended.
>
> 3. Upon removing cable in device mode, the disconnect event must be
> generated and unblock runtime suspend for dwc3 core.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
What happens to this code if you
static int count;
1. sleep in dwc3_probe for 10 milliseconds
2. return -EPROBE_DEFER
3. if count++ < 5 goto 1
i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
and what happens if we introduce a 100 millsecond sleep into
dwc3_qcom_probe() - and run a fake disconnect event from
dwc3_qcom_probe_core() directly ?
In other words if make it that dwc3_probe() completes and struct
dwc3_glue_ops->notify_cable_disconnect() fires prior to
dwc3_qcom_probe_core() completing ?
i.e. I don't immediately see how you've solved the probe() completion
race condition here.
---
bod
On 11/3/2023 8:26 PM, Caleb Connolly wrote:
>
>
> On 03/11/2023 05:34, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 11/3/2023 12:10 AM, Caleb Connolly wrote:
>>>> Hi Caleb,
>>>>
>>>> There are two types of platforms, some use extcon and some use
>>>> role-switch to deliver vbus/id notifications. Extcon targets already
>>>> have this qscratch modifications present today in vbus and id
>>>> handlers. But for role-switch based targets we don't have any way to
>>>> get this notification to dwc3-qcom. In this implementation, I wanted
>>>> to get those notications from core to glue and for this we
>>>> implenented vendor hooks.
>>>>
>>>> The property added has been used to do two things:
>>>>
>>>> 1. Register glue's vendor hooks to core driver
>>>> 2. Do runtime_allow for glue (and by default for core as the dt is
>>>> not flattened)
>>>>
>>>> In case of extcon, we don't want to register vendor hooks as
>>>> notifications are not necessary.
>>>
>>> Could it just be enabled when role_switch is present then?
>>>>
>>
>> So we would register vendor hooks when usb-role-switch is present but
>> don't do runtime allow, and leave that option to user space right ?
>> I think it would work and we can do away with the binding completely.
>
> Can we still enable runtime suspend? Maybe someone else wants to chime
> in here, but I'd guess that it's preferable to have it enabled by
> default, particularly for devices like phones. Or are there side effects
> from this?
>>
AFAIK, I don't see any side effects whether we enable runtime from user
space or do runtime_allow() here in kernel itself and leave qscratch
config to vendor hooks.
But leaving it enabled by default, we do this for almost all targets in
downstream today. So I guess there would be no side effects.
Regards,
Krishna,
On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote:
> On 17/10/2023 14:18, Krishna Kurapati wrote:
>>
>> The following are the requirements aimed in this implementation:
>>
>> 1. When enum in device mode, Glue/core must stay active.
>>
>> 2. When cable is connected but UDC is not written yet, then glue/core
>> must be suspended.
>>
>> 3. Upon removing cable in device mode, the disconnect event must be
>> generated and unblock runtime suspend for dwc3 core.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>
Hi Bryan,
> What happens to this code if you
>
> static int count;
>
> 1. sleep in dwc3_probe for 10 milliseconds
> 2. return -EPROBE_DEFER
> 3. if count++ < 5 goto 1
>
> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
>
The vendor hooks are used in __dwc3_set_mode and role_switch_set calls
in core and drd files respectively. These are invoked only if we are OTG
capable. The drd_work is initialized in core_init_mode which is called
at the end of dwc3_probe. If dwc3_probe fails and gets deferred before
that, none of the vendor hooks will be fired and dwc3_qcom_probe is also
deferred.
However I see that if core_init_mode fails (the cleanup is already done
in drd to prevent set_role from getting invoked already), I need to
cleanup vendor hooks in error path of dwc3_probe().
> and what happens if we introduce a 100 millsecond sleep into
> dwc3_qcom_probe() - and run a fake disconnect event from
> dwc3_qcom_probe_core() directly ?
>
> In other words if make it that dwc3_probe() completes and struct
> dwc3_glue_ops->notify_cable_disconnect() fires prior to
> dwc3_qcom_probe_core() completing ?
>
> i.e. I don't immediately see how you've solved the probe() completion
> race condition here.
>
Just wanted to understand the situation clearly. Is this the sequence
you are referring to ?
1. dwc3_probe is successful and role switch is registered properly.
2. added delay after dwc3_qcom_probe_core and before interconnect_init
3. Between this delay, we got a disconnect notificiation from glink
4. We are clearing the qscratch reg in case of device mode and
un-registering notifier in case of host mode.
If so, firstly I don't see any issue if we process disconnect event
before qcom probe is complete. If we reached this stage, the clocks/gdsc
is definitely ON and register accesses are good to go.
If we are in host mode at this point, we would just unregister to
usb-core notifier and mark last busy. If we are in device mode, we would
just clear the hs_phy_ctrl reg of qscratch. After the 100ms delay you
mentioned we would call dwc3_remove anyways and cleanup the vendor
hooks. But is the concern here that, what if we enter runtime_suspend at
this point ?
Regards,
Krishna,
On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote:
>
>
> On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote:
>> On 17/10/2023 14:18, Krishna Kurapati wrote:
>>>
>>> The following are the requirements aimed in this implementation:
>>>
>>> 1. When enum in device mode, Glue/core must stay active.
>>>
>>> 2. When cable is connected but UDC is not written yet, then glue/core
>>> must be suspended.
>>>
>>> 3. Upon removing cable in device mode, the disconnect event must be
>>> generated and unblock runtime suspend for dwc3 core.
>>>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>
>
> Hi Bryan,
>
>> What happens to this code if you
>>
>> static int count;
>>
>> 1. sleep in dwc3_probe for 10 milliseconds
>> 2. return -EPROBE_DEFER
>> 3. if count++ < 5 goto 1
>>
>> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
>>
> The vendor hooks are used in __dwc3_set_mode and role_switch_set calls
> in core and drd files respectively. These are invoked only if we are OTG
> capable. The drd_work is initialized in core_init_mode which is called
> at the end of dwc3_probe. If dwc3_probe fails and gets deferred before
> that, none of the vendor hooks will be fired and dwc3_qcom_probe is also
> deferred.
>
> However I see that if core_init_mode fails (the cleanup is already done
> in drd to prevent set_role from getting invoked already), I need to
> cleanup vendor hooks in error path of dwc3_probe().
>
>> and what happens if we introduce a 100 millsecond sleep into
>> dwc3_qcom_probe() - and run a fake disconnect event from
>> dwc3_qcom_probe_core() directly ?
>>
>> In other words if make it that dwc3_probe() completes and struct
>> dwc3_glue_ops->notify_cable_disconnect() fires prior to
>> dwc3_qcom_probe_core() completing ?
>>
>> i.e. I don't immediately see how you've solved the probe() completion
>> race condition here.
>>
> Just wanted to understand the situation clearly. Is this the sequence
> you are referring to ?
>
> 1. dwc3_probe is successful and role switch is registered properly.
> 2. added delay after dwc3_qcom_probe_core and before interconnect_init
> 3. Between this delay, we got a disconnect notificiation from glink
> 4. We are clearing the qscratch reg in case of device mode and
> un-registering notifier in case of host mode.
>
> If so, firstly I don't see any issue if we process disconnect event
> before qcom probe is complete. If we reached this stage, the clocks/gdsc
> is definitely ON and register accesses are good to go.
>
> If we are in host mode at this point, we would just unregister to
> usb-core notifier and mark last busy. If we are in device mode, we would
> just clear the hs_phy_ctrl reg of qscratch. After the 100ms delay you
> mentioned we would call dwc3_remove anyways and cleanup the vendor
> hooks. But is the concern here that, what if we enter runtime_suspend at
> this point ?
>
Just to clarify one more thing. The probe completion requirement came in
because, before the device tree was flattened, dwc3-qcom and core are
two different platform devices. And if the dwc3 core device probe got
deferred, dwc3-qcom probe still gets successfully completed. The glue
would never know when to register vendor hook callbacks to dwc3-core as
it would never know when the core probe was completed.
That is the reason we wanted to find out accurate point where core probe
is done to ensure we can properly register these callbacks.
Regards,
Krishna,
On 03/11/2023 18:49, Krishna Kurapati PSSNV wrote:
>
>
> On 11/4/2023 12:15 AM, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 11/3/2023 8:44 PM, Bryan O'Donoghue wrote:
>>> On 17/10/2023 14:18, Krishna Kurapati wrote:
>>>>
>>>> The following are the requirements aimed in this implementation:
>>>>
>>>> 1. When enum in device mode, Glue/core must stay active.
>>>>
>>>> 2. When cable is connected but UDC is not written yet, then glue/core
>>>> must be suspended.
>>>>
>>>> 3. Upon removing cable in device mode, the disconnect event must be
>>>> generated and unblock runtime suspend for dwc3 core.
>>>>
>>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>>
>>
>> Hi Bryan,
>>
>>> What happens to this code if you
>>>
>>> static int count;
>>>
>>> 1. sleep in dwc3_probe for 10 milliseconds
>>> 2. return -EPROBE_DEFER
>>> 3. if count++ < 5 goto 1
>>>
>>> i.e. if we simulate say waiting on a PHY driver to probe in dwc3_probe()
>>>
>> The vendor hooks are used in __dwc3_set_mode and role_switch_set calls
>> in core and drd files respectively. These are invoked only if we are
>> OTG capable. The drd_work is initialized in core_init_mode which is
>> called at the end of dwc3_probe. If dwc3_probe fails and gets deferred
>> before that, none of the vendor hooks will be fired and
>> dwc3_qcom_probe is also deferred.
>>
>> However I see that if core_init_mode fails (the cleanup is already
>> done in drd to prevent set_role from getting invoked already), I need
>> to cleanup vendor hooks in error path of dwc3_probe().
>>
>>> and what happens if we introduce a 100 millsecond sleep into
>>> dwc3_qcom_probe() - and run a fake disconnect event from
>>> dwc3_qcom_probe_core() directly ?
>>>
>>> In other words if make it that dwc3_probe() completes and struct
>>> dwc3_glue_ops->notify_cable_disconnect() fires prior to
>>> dwc3_qcom_probe_core() completing ?
>>>
>>> i.e. I don't immediately see how you've solved the probe() completion
>>> race condition here.
>>>
>> Just wanted to understand the situation clearly. Is this the sequence
>> you are referring to ?
>>
>> 1. dwc3_probe is successful and role switch is registered properly.
>> 2. added delay after dwc3_qcom_probe_core and before interconnect_init
>> 3. Between this delay, we got a disconnect notificiation from glink
>> 4. We are clearing the qscratch reg in case of device mode and
>> un-registering notifier in case of host mode.
>>
>> If so, firstly I don't see any issue if we process disconnect event
>> before qcom probe is complete. If we reached this stage, the
>> clocks/gdsc is definitely ON and register accesses are good to go.
>>
>> If we are in host mode at this point, we would just unregister to
>> usb-core notifier and mark last busy. If we are in device mode, we
>> would just clear the hs_phy_ctrl reg of qscratch. After the 100ms
>> delay you mentioned we would call dwc3_remove anyways and cleanup the
>> vendor hooks. But is the concern here that, what if we enter
>> runtime_suspend at this point ?
>>
>
> Just to clarify one more thing. The probe completion requirement came in
> because, before the device tree was flattened, dwc3-qcom and core are
> two different platform devices. And if the dwc3 core device probe got
> deferred, dwc3-qcom probe still gets successfully completed. The glue
> would never know when to register vendor hook callbacks to dwc3-core as
> it would never know when the core probe was completed.
>
> That is the reason we wanted to find out accurate point where core probe
> is done to ensure we can properly register these callbacks.
Are you saying to you require/rely on both of these series being applied
first ?
[1]:
https://lore.kernel.org/all/[email protected]/
[2]:
https://lore.kernel.org/all/[email protected]/
Must be, nothing applies for me in this series.
>>> Hi Bryan,
>>>
>>>> What happens to this code if you
>>>>
>>>> static int count;
>>>>
>>>> 1. sleep in dwc3_probe for 10 milliseconds
>>>> 2. return -EPROBE_DEFER
>>>> 3. if count++ < 5 goto 1
>>>>
>>>> i.e. if we simulate say waiting on a PHY driver to probe in
>>>> dwc3_probe()
>>>>
>>> The vendor hooks are used in __dwc3_set_mode and role_switch_set
>>> calls in core and drd files respectively. These are invoked only if
>>> we are OTG capable. The drd_work is initialized in core_init_mode
>>> which is called at the end of dwc3_probe. If dwc3_probe fails and
>>> gets deferred before that, none of the vendor hooks will be fired and
>>> dwc3_qcom_probe is also deferred.
>>>
>>> However I see that if core_init_mode fails (the cleanup is already
>>> done in drd to prevent set_role from getting invoked already), I
>>> need to cleanup vendor hooks in error path of dwc3_probe().
>>>
>>>> and what happens if we introduce a 100 millsecond sleep into
>>>> dwc3_qcom_probe() - and run a fake disconnect event from
>>>> dwc3_qcom_probe_core() directly ?
>>>>
>>>> In other words if make it that dwc3_probe() completes and struct
>>>> dwc3_glue_ops->notify_cable_disconnect() fires prior to
>>>> dwc3_qcom_probe_core() completing ?
>>>>
>>>> i.e. I don't immediately see how you've solved the probe()
>>>> completion race condition here.
>>>>
>>> Just wanted to understand the situation clearly. Is this the sequence
>>> you are referring to ?
>>>
>>> 1. dwc3_probe is successful and role switch is registered properly.
>>> 2. added delay after dwc3_qcom_probe_core and before interconnect_init
>>> 3. Between this delay, we got a disconnect notificiation from glink
>>> 4. We are clearing the qscratch reg in case of device mode and
>>> un-registering notifier in case of host mode.
>>>
>>> If so, firstly I don't see any issue if we process disconnect event
>>> before qcom probe is complete. If we reached this stage, the
>>> clocks/gdsc is definitely ON and register accesses are good to go.
>>>
>>> If we are in host mode at this point, we would just unregister to
>>> usb-core notifier and mark last busy. If we are in device mode, we
>>> would just clear the hs_phy_ctrl reg of qscratch. After the 100ms
>>> delay you mentioned we would call dwc3_remove anyways and cleanup the
>>> vendor hooks. But is the concern here that, what if we enter
>>> runtime_suspend at this point ?
>>>
>>
>> Just to clarify one more thing. The probe completion requirement came
>> in because, before the device tree was flattened, dwc3-qcom and core
>> are two different platform devices. And if the dwc3 core device probe
>> got deferred, dwc3-qcom probe still gets successfully completed. The
>> glue would never know when to register vendor hook callbacks to
>> dwc3-core as it would never know when the core probe was completed.
>>
>> That is the reason we wanted to find out accurate point where core
>> probe is done to ensure we can properly register these callbacks.
>
> Are you saying to you require/rely on both of these series being applied
> first ?
>
> [1]:
> https://lore.kernel.org/all/[email protected]/
> [2]:
> https://lore.kernel.org/all/[email protected]/
>
> Must be, nothing applies for me in this series.
The first one is not a patch. It is just a discussion thread I started
to get community's opinion before on disconnect interrupt handling. The
current series is based on top of [2] made by Bjorn (as you already
found out) and as I mentioned in cover letter of my series.
Regards,
Krishna,
On 11/4/2023 10:32 PM, Krishna Kurapati PSSNV wrote:
>>
>> Are you saying to you require/rely on both of these series being
>> applied first ?
>>
>> [1]:
>> https://lore.kernel.org/all/[email protected]/
>> [2]:
>> https://lore.kernel.org/all/[email protected]/
>>
>> Must be, nothing applies for me in this series.
>
> The first one is not a patch. It is just a discussion thread I started
> to get community's opinion before on disconnect interrupt handling. The
> current series is based on top of [2] made by Bjorn (as you already
> found out) and as I mentioned in cover letter of my series.
>
Hi Bryan,
Are you able to apply the series after including Bjorn's patches ?
Also can you confirm if the comments provided to your queries on [1] are
proper and if you have any other comments w.r.t probe deferral.
[1]:
https://lore.kernel.org/all/[email protected]/
Regards,
Krishna,