2023-10-07 15:48:58

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 00/10] Add multiport support for DWC3 controllers

Currently the DWC3 driver supports only single port controller which
requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
DWC3 controller with multiple ports that can operate in host mode.
Some of the port supports both SS+HS and other port supports only HS
mode.

This change primarily refactors the Phy logic in core driver to allow
multiport support with Generic Phy's.

Changes have been tested on QCOM SoC SA8295P which has 4 ports (2
are HS+SS capable and 2 are HS only capable).

Changes in v13:
This series is a subset of patches in v11 as the first 3 patches in v11
have been mereged into usb-next.
Moved dr_mode property from platform specific files to common sc8280xp DT.
Fixed function call wrapping, added comments and replaced #defines with
enum in dwc3-qcom for identifying IRQ index appropriately.
Fixed nitpicks pointed out in v11 for suspend-resume handling.
Added reported-by tag for phy refactoring patch as a compile error was
found by kernel test bot [1].
Removed reviewed-by tag of maintainer for phy refactoring patch as a minor
change of increasing phy-names array size by 2-bytes was done to fix
compilation issue mentioned in [1].

Changes in v12:
Pushed as a subset of acked but no-yet-merged patches of v11 with intent
of making rebase of other patches easy. Active reviewers from community
suggested that it would be better to push the whole series in one go as it
would give good clarity and context for all the patches in the series.
So pushed v13 for the same addressing comments received in v11.

Changes in v11:
Implemented port_count calculation by reading interrupt-names from DT.
Refactored IRQ handling in dwc3-qcom.
Moving of macros to xhci-ext-caps.h made as a separate patch.
Names of interrupts to be displayed on /proc/interrupts set to the ones
present in DT.

Changes in v10:
Refactored phy init/exit/power-on/off functions in dwc3 core
Refactored dwc3-qcom irq registration and handling
Implemented wakeup for multiport irq's
Moved few macros from xhci.h to xhci-ext-caps.h
Fixed nits pointed out in v9
Fixed Co-developed by and SOB tags in patches 5 and 11

Changes in v9:
Added IRQ support for DP/DM/SS MP Irq's of SC8280
Refactored code to read port count by accessing xhci registers

Changes in v8:
Reorganised code in patch-5
Fixed nitpicks in code according to comments received on v7
Fixed indentation in DT patches
Added drive strength for pinctrl nodes in SA8295 DT

Changes in v7:
Added power event irq's for Multiport controller.
Udpated commit text for patch-9 (adding DT changes for enabling first
port of multiport controller on sa8540-ride).
Fixed check-patch warnings for driver code.
Fixed DT binding errors for changes in snps,dwc3.yaml
Reabsed code on top of usb-next

Changes in v6:
Updated comments in code after.
Updated variables names appropriately as per review comments.
Updated commit text in patch-2 and added additional info as per review
comments.
The patch header in v5 doesn't have "PATHCH v5" notation present. Corrected
it in this version.

Changes in v5:
Added DT support for first port of Teritiary USB controller on SA8540-Ride
Added support for reading port info from XHCI Extended Params registers.

Changes in RFC v4:
Added DT support for SA8295p.

Changes in RFC v3:
Incase any PHY init fails, then clear/exit the PHYs that
are already initialized.

Changes in RFC v2:
Changed dwc3_count_phys to return the number of PHY Phandles in the node.
This will be used now in dwc3_extract_num_phys to increment num_usb2_phy
and num_usb3_phy.

Added new parameter "ss_idx" in dwc3_core_get_phy_ny_node and changed its
structure such that the first half is for HS-PHY and second half is for
SS-PHY.

In dwc3_core_get_phy, for multiport controller, only if SS-PHY phandle is
present, pass proper SS_IDX else pass -1.

Tests done on v13:

1) MP controller enumerting devices on different ports:

/ # lsusb
Bus 001 Device 001: ID 1d6b:0002
Bus 001 Device 003: ID 03f0:0024
Bus 001 Device 002: ID 17ef:6099
Bus 002 Device 001: ID 1d6b:0003

2) Interrupt registration for 3 controllers of sa8295p-adp

/ # cat /proc/interrupts | grep phy
171: 0 0 0 0 0 0 0 0 PDC 127 Level dp_hs_phy_1
172: 0 0 0 0 0 0 0 0 PDC 126 Level dm_hs_phy_1
173: 0 0 0 0 0 0 0 0 PDC 129 Level dp_hs_phy_2
174: 0 0 0 0 0 0 0 0 PDC 128 Level dm_hs_phy_2
175: 0 0 0 0 0 0 0 0 PDC 131 Level dp_hs_phy_3
176: 0 0 0 0 0 0 0 0 PDC 130 Level dm_hs_phy_3
177: 0 0 0 0 0 0 0 0 PDC 133 Level dp_hs_phy_4
178: 0 0 0 0 0 0 0 0 PDC 132 Level dm_hs_phy_4
179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1
180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2
182: 0 0 0 0 0 0 0 0 PDC 14 Level dp_hs_phy_irq
183: 0 0 0 0 0 0 0 0 PDC 15 Level dm_hs_phy_irq
184: 0 0 0 0 0 0 0 0 PDC 138 Level ss_phy_irq
190: 0 0 0 0 0 0 0 0 PDC 12 Level dp_hs_phy_irq
191: 0 0 0 0 0 0 0 0 PDC 13 Level dm_hs_phy_irq
192: 0 0 0 0 0 0 0 0 PDC 136 Level ss_phy_irq

3) Wakeup was tested on all 4 ports of tertiary controller by
a) Connecting peripheral after device is suspended and checking wakeup
b) Removing peripheral after device is suspended and checking wakeup
c) Connecting keyboard and checking if button press wakes up system

4) Testing interrupt registration on single port controller SM8550-MTP

/ # cat /proc/interrupts | grep phy
146: 0 0 0 0 0 0 0 0 GICv3 162 Level hs_phy_irq
147: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_irq
148: 0 0 0 0 0 0 0 0 PDC 15 Level dm_hs_phy_irq
149: 0 0 0 0 0 0 0 0 PDC 14 Level dp_hs_phy_irq

5) ADB enumeration and working on SM8550-MTP has been tested

[1]: https://lore.kernel.org/all/[email protected]/

Links to previous versions:
Link to v12: https://lore.kernel.org/all/[email protected]/
Link to v11: https://lore.kernel.org/all/[email protected]/
Link to v10: https://lore.kernel.org/all/[email protected]/
Link to v9: https://lore.kernel.org/all/[email protected]/
Link to v8: https://lore.kernel.org/all/[email protected]/
Link to v7: https://lore.kernel.org/all/[email protected]/
Link to v6: https://lore.kernel.org/all/[email protected]/
Link to v5: https://lore.kernel.org/all/[email protected]/
Link to RFC v4: https://lore.kernel.org/all/[email protected]/
Link to RFC v3: https://lore.kernel.org/all/[email protected]/#r
Link to RFC v2: https://lore.kernel.org/all/[email protected]/#r

Andrew Halaney (1):
arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb
controller

Harsh Agarwal (1):
usb: dwc3: core: Refactor PHY logic to support Multiport Controller

Krishna Kurapati (8):
usb: dwc3: core: Access XHCI address space temporarily to read port
info
usb: dwc3: core: Skip setting event buffers for host only controllers
usb: dwc3: qcom: Add helper function to request threaded IRQ
usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver
usb: dwc3: qcom: Enable wakeup for applicable ports of multiport
usb: dwc3: qcom: Add multiport suspend/resume support for wrapper
arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280
arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB
ports

arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++
arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 ++
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++
drivers/usb/dwc3/core.c | 324 +++++++++++++++++-----
drivers/usb/dwc3/core.h | 16 +-
drivers/usb/dwc3/drd.c | 15 +-
drivers/usb/dwc3/dwc3-qcom.c | 323 ++++++++++++++-------
7 files changed, 642 insertions(+), 190 deletions(-)

--
2.42.0


2023-10-07 15:49:20

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 01/10] usb: dwc3: core: Access XHCI address space temporarily to read port info

Currently host-only capable DWC3 controllers support Multiport.
Temporarily map XHCI address space for host-only controllers and parse
XHCI Extended Capabilities registers to read number of usb2 ports and
usb3 ports present on multiport controller. Each USB Port is at least HS
capable.

The port info for usb2 and usb3 phy are identified as num_usb2_ports
and num_usb3_ports. The intention is as follows:

Wherever we need to perform phy operations like:

LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS()
{
phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
}

If number of usb2 ports is 3, loop can go from index 0-2 for
usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure,
if the first 2 ports are SS capable or some other ports like (2 and 3)
are SS capable. So instead, num_usb2_ports is used to loop around all
phy's (both hs and ss) for performing phy operations. If any
usb3_generic_phy turns out to be NULL, phy operation just bails out.

num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up
phy's as we need to know how many SS capable ports are there for this.

Signed-off-by: Krishna Kurapati <[email protected]>
Acked-by: Thinh Nguyen <[email protected]>
---
drivers/usb/dwc3/core.c | 61 +++++++++++++++++++++++++++++++++++++++++
drivers/usb/dwc3/core.h | 5 ++++
2 files changed, 66 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 44ee8526dc28..d7c5669ebd06 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -39,6 +39,7 @@
#include "io.h"

#include "debug.h"
+#include "../host/xhci-ext-caps.h"

#define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */

@@ -1839,6 +1840,51 @@ static int dwc3_get_clocks(struct dwc3 *dwc)
return 0;
}

+static int dwc3_read_port_info(struct dwc3 *dwc)
+{
+ void __iomem *base;
+ u8 major_revision;
+ u32 offset = 0;
+ u32 val;
+
+ /*
+ * Remap xHCI address space to access XHCI ext cap regs,
+ * since it is needed to get port info.
+ */
+ base = ioremap(dwc->xhci_resources[0].start,
+ resource_size(&dwc->xhci_resources[0]));
+ if (IS_ERR(base))
+ return PTR_ERR(base);
+
+ do {
+ offset = xhci_find_next_ext_cap(base, offset,
+ XHCI_EXT_CAPS_PROTOCOL);
+ if (!offset)
+ break;
+
+ val = readl(base + offset);
+ major_revision = XHCI_EXT_PORT_MAJOR(val);
+
+ val = readl(base + offset + 0x08);
+ if (major_revision == 0x03) {
+ dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val);
+ } else if (major_revision <= 0x02) {
+ dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val);
+ } else {
+ dev_err(dwc->dev,
+ "Unrecognized port major revision %d\n",
+ major_revision);
+ }
+ } while (1);
+
+ dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
+ dwc->num_usb2_ports, dwc->num_usb3_ports);
+
+ iounmap(base);
+
+ return 0;
+}
+
static int dwc3_probe(struct platform_device *pdev)
{
struct device *dev = &pdev->dev;
@@ -1846,6 +1892,7 @@ static int dwc3_probe(struct platform_device *pdev)
void __iomem *regs;
struct dwc3 *dwc;
int ret;
+ unsigned int hw_mode;

dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
if (!dwc)
@@ -1926,6 +1973,20 @@ static int dwc3_probe(struct platform_device *pdev)
goto err_disable_clks;
}

+ /*
+ * Currently only DWC3 controllers that are host-only capable
+ * support Multiport.
+ */
+ hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+ if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
+ ret = dwc3_read_port_info(dwc);
+ if (ret)
+ goto err_disable_clks;
+ } else {
+ dwc->num_usb2_ports = 1;
+ dwc->num_usb3_ports = 1;
+ }
+
spin_lock_init(&dwc->lock);
mutex_init(&dwc->mutex);

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 6782ec8bfd64..2ea6df7e6571 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1031,6 +1031,8 @@ struct dwc3_scratchpad_array {
* @usb3_phy: pointer to USB3 PHY
* @usb2_generic_phy: pointer to USB2 PHY
* @usb3_generic_phy: pointer to USB3 PHY
+ * @num_usb2_ports: number of USB2 ports
+ * @num_usb3_ports: number of USB3 ports
* @phys_ready: flag to indicate that PHYs are ready
* @ulpi: pointer to ulpi interface
* @ulpi_ready: flag to indicate that ULPI is initialized
@@ -1174,6 +1176,9 @@ struct dwc3 {
struct phy *usb2_generic_phy;
struct phy *usb3_generic_phy;

+ u8 num_usb2_ports;
+ u8 num_usb3_ports;
+
bool phys_ready;

struct ulpi *ulpi;
--
2.42.0

2023-10-07 15:49:21

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 02/10] usb: dwc3: core: Skip setting event buffers for host only controllers

On some SoC's like SA8295P where the tertiary controller is host-only
capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible.
Trying to access them leads to a crash.

For DRD/Peripheral supported controllers, event buffer setup is done
again in gadget_pullup. Skip setup or cleanup of event buffers if
controller is host-only capable.

Suggested-by: Johan Hovold <[email protected]>
Signed-off-by: Krishna Kurapati <[email protected]>
Acked-by: Thinh Nguyen <[email protected]>
---
drivers/usb/dwc3/core.c | 13 +++++++++++++
1 file changed, 13 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d7c5669ebd06..3e507e4cc542 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -486,6 +486,13 @@ static void dwc3_free_event_buffers(struct dwc3 *dwc)
static int dwc3_alloc_event_buffers(struct dwc3 *dwc, unsigned int length)
{
struct dwc3_event_buffer *evt;
+ unsigned int hw_mode;
+
+ hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+ if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
+ dwc->ev_buf = NULL;
+ return 0;
+ }

evt = dwc3_alloc_one_event_buffer(dwc, length);
if (IS_ERR(evt)) {
@@ -507,6 +514,9 @@ int dwc3_event_buffers_setup(struct dwc3 *dwc)
{
struct dwc3_event_buffer *evt;

+ if (!dwc->ev_buf)
+ return 0;
+
evt = dwc->ev_buf;
evt->lpos = 0;
dwc3_writel(dwc->regs, DWC3_GEVNTADRLO(0),
@@ -524,6 +534,9 @@ void dwc3_event_buffers_cleanup(struct dwc3 *dwc)
{
struct dwc3_event_buffer *evt;

+ if (!dwc->ev_buf)
+ return;
+
evt = dwc->ev_buf;

evt->lpos = 0;
--
2.42.0

2023-10-07 15:49:25

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

Currently wakeup is supported by only single port controllers. Read speed
of each port and accordingly enable IRQ's for those ports.

Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++-----------------
1 file changed, 35 insertions(+), 30 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 863892284146..651b9775a0c2 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -90,7 +90,7 @@ struct dwc3_qcom {
*/
int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
int hs_phy_irq;
- enum usb_device_speed usb2_speed;
+ enum usb_device_speed usb2_speed[DWC3_MAX_PORTS];

struct extcon_dev *edev;
struct extcon_dev *host_edev;
@@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
return dwc->xhci;
}

-static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
+static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom,
+ int port_index)
{
struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
struct usb_device *udev;
@@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)

/*
* It is possible to query the speed of all children of
- * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
- * currently supports only 1 port per controller. So
- * this is sufficient.
+ * USB2.0 root hub via usb_hub_for_each_child().
*/
#ifdef CONFIG_USB
- udev = usb_hub_find_child(hcd->self.root_hub, 1);
+ udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
#else
udev = NULL;
#endif
@@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)

static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
{
+ int i;
+
dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

- if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
- } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
- (qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
- } else {
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
- }
+ for (i = 0; i < qcom->num_ports; i++) {
+ if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
+ } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
+ (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
+ } else {
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
+ }

- dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]);
+ }
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
{
+ int i;
+
dwc3_qcom_enable_wakeup_irq(qcom->hs_phy_irq, 0);

/*
@@ -413,22 +418,24 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
* disconnect and remote wakeup. When no device is connected, configure both
* DP and DM lines as rising edge to detect HS/HS/LS device connect scenario.
*/
-
- if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
+ for (i = 0; i < qcom->num_ports; i++) {
+ qcom->usb2_speed[i] = dwc3_qcom_read_usb2_speed(qcom, i);
+ if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i],
IRQ_TYPE_EDGE_FALLING);
- } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
- (qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
+ } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
+ (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i],
IRQ_TYPE_EDGE_FALLING);
- } else {
- dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
+ } else {
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i],
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i],
IRQ_TYPE_EDGE_RISING);
- }
+ }

- dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0);
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i], 0);
+ }
}

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -454,10 +461,8 @@ 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) {
- qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
+ if (dwc3_qcom_is_host(qcom) && wakeup)
dwc3_qcom_enable_interrupts(qcom);
- }

qcom->is_suspended = true;

--
2.42.0

2023-10-07 15:49:39

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 07/10] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
for all the ports during suspend/resume.

Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 35 ++++++++++++++++++++++++++++-------
1 file changed, 28 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 651b9775a0c2..dbd4239e61c9 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -37,7 +37,11 @@
#define PIPE3_PHYSTATUS_SW BIT(3)
#define PIPE_UTMI_CLK_DIS BIT(8)

-#define PWR_EVNT_IRQ_STAT_REG 0x58
+#define PWR_EVNT_IRQ1_STAT_REG 0x58
+#define PWR_EVNT_IRQ2_STAT_REG 0x1dc
+#define PWR_EVNT_IRQ3_STAT_REG 0x228
+#define PWR_EVNT_IRQ4_STAT_REG 0x238
+
#define PWR_EVNT_LPM_IN_L2_MASK BIT(4)
#define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)

@@ -107,6 +111,19 @@ struct dwc3_qcom {
int num_ports;
};

+/*
+ * Currently non-multiport controller have only one PWR_EVENT_IRQ register,
+ * but multiport controllers like SA8295 contain upto 4 of them.
+ */
+#define NUM_PWR_EVENT_STAT_REGS 4
+
+static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = {
+ PWR_EVNT_IRQ1_STAT_REG,
+ PWR_EVNT_IRQ2_STAT_REG,
+ PWR_EVNT_IRQ3_STAT_REG,
+ PWR_EVNT_IRQ4_STAT_REG,
+};
+
static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
{
u32 reg;
@@ -446,9 +463,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
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 < qcom->num_ports; i++) {
+ val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
+ if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
+ dev_err(qcom->dev, "HS-PHY not in L2\n");
+ }

for (i = qcom->num_clocks - 1; i >= 0; i--)
clk_disable_unprepare(qcom->clks[i]);
@@ -494,9 +513,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);

/* Clear existing events from PHY related to L2 in/out */
- dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
- PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
-
+ for (i = 0; i < qcom->num_ports; i++) {
+ dwc3_qcom_setbits(qcom->qscratch_base,
+ pwr_evnt_irq_stat_reg_offset[i],
+ PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
+ }
qcom->is_suspended = false;

return 0;
--
2.42.0

2023-10-07 15:49:48

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

Refactor setup_irq call to facilitate reading multiport IRQ's along
with non mulitport ones. Read through the interrupt-names property
to figure out, the type of interrupt (DP/DM/HS/SS) and to which port
it belongs. Also keep track of port index to calculate port count
based on interrupts provided as input in DT.

Signed-off-by: Krishna Kurapati <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++----------
1 file changed, 154 insertions(+), 56 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index ef2006db7601..863892284146 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -53,14 +53,25 @@
#define APPS_USB_AVG_BW 0
#define APPS_USB_PEAK_BW MBps_to_icc(40)

+#define NUM_PHY_IRQ 4
+
+enum dwc3_qcom_ph_index {
+ DP_HS_PHY_IRQ_INDEX = 0,
+ DM_HS_PHY_IRQ_INDEX,
+ SS_PHY_IRQ_INDEX,
+ HS_PHY_IRQ_INDEX,
+};
+
struct dwc3_acpi_pdata {
u32 qscratch_base_offset;
u32 qscratch_base_size;
u32 dwc3_core_base_size;
+ /*
+ * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
+ * IRQ's respectively.
+ */
+ int phy_irq_index[NUM_PHY_IRQ - 1];
int hs_phy_irq_index;
- int dp_hs_phy_irq_index;
- int dm_hs_phy_irq_index;
- int ss_phy_irq_index;
bool is_urs;
};

@@ -73,10 +84,12 @@ struct dwc3_qcom {
int num_clocks;
struct reset_control *resets;

+ /*
+ * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
+ * respectively.
+ */
+ int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
int hs_phy_irq;
- int dp_hs_phy_irq;
- int dm_hs_phy_irq;
- int ss_phy_irq;
enum usb_device_speed usb2_speed;

struct extcon_dev *edev;
@@ -91,6 +104,7 @@ struct dwc3_qcom {
bool pm_suspended;
struct icc_path *icc_path_ddr;
struct icc_path *icc_path_apps;
+ int num_ports;
};

static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
@@ -375,16 +389,16 @@ static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);

if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
} else {
- dwc3_qcom_disable_wakeup_irq(qcom->dp_hs_phy_irq);
- dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
}

- dwc3_qcom_disable_wakeup_irq(qcom->ss_phy_irq);
+ dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
}

static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
@@ -401,20 +415,20 @@ static void dwc3_qcom_enable_interrupts(struct dwc3_qcom *qcom)
*/

if (qcom->usb2_speed == USB_SPEED_LOW) {
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_FALLING);
} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
(qcom->usb2_speed == USB_SPEED_FULL)) {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_FALLING);
} else {
- dwc3_qcom_enable_wakeup_irq(qcom->dp_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_RISING);
- dwc3_qcom_enable_wakeup_irq(qcom->dm_hs_phy_irq,
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0],
IRQ_TYPE_EDGE_RISING);
}

- dwc3_qcom_enable_wakeup_irq(qcom->ss_phy_irq, 0);
+ dwc3_qcom_enable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0], 0);
}

static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
@@ -535,8 +549,8 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
return ret;
}

-static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
- char *disp_name, int irq)
+static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
+ const char *disp_name, int irq)
{
int ret;

@@ -553,47 +567,135 @@ static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
return ret;
}

+static int dwc3_qcom_get_irq_index(const char *irq_name)
+{
+ /*
+ * If we are reading IRQ not supported by the driver
+ * like pwr_event_irq, then return -1 indicating the next
+ * helper function to skip processing IRQ name further.
+ */
+ int irq_index = -1;
+
+ if (strncmp(irq_name, "dp_hs_phy", strlen("dp_hs_phy")) == 0)
+ irq_index = DP_HS_PHY_IRQ_INDEX;
+ else if (strncmp(irq_name, "dm_hs_phy", strlen("dm_hs_phy")) == 0)
+ irq_index = DM_HS_PHY_IRQ_INDEX;
+ else if (strncmp(irq_name, "ss_phy", strlen("ss_phy")) == 0)
+ irq_index = SS_PHY_IRQ_INDEX;
+ else if (strncmp(irq_name, "hs_phy", strlen("hs_phy")) == 0)
+ irq_index = HS_PHY_IRQ_INDEX;
+ return irq_index;
+}
+
+static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index)
+{
+ int port_index = -1;
+
+ switch (irq_index) {
+ case DP_HS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "dp_hs_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "dp_hs_phy_%d", &port_index);
+ break;
+
+ case DM_HS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "dm_hs_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "dm_hs_phy_%d", &port_index);
+ break;
+
+ case SS_PHY_IRQ_INDEX:
+ if (strcmp(irq_name, "ss_phy_irq") == 0)
+ port_index = 1;
+ else
+ sscanf(irq_name, "ss_phy_%d", &port_index);
+ break;
+
+ case HS_PHY_IRQ_INDEX:
+ port_index = 1;
+ break;
+ }
+
+ if (port_index > DWC3_MAX_PORTS)
+ port_index = -1;
+
+ return port_index;
+}
+
+static int dwc3_qcom_get_acpi_index(struct dwc3_qcom *qcom, int irq_index,
+ int port_index)
+{
+ const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+ int acpi_index = -1;
+
+ /*
+ * Currently multiport supported targets don't have an ACPI variant.
+ * So return -1 if we are not dealing with first port of the controller.
+ */
+ if ((pdata == NULL) || (port_index != 1))
+ goto done;
+
+ if (irq_index == HS_PHY_IRQ_INDEX)
+ acpi_index = pdata->hs_phy_irq_index;
+ else
+ acpi_index = pdata->phy_irq_index[irq_index];
+
+done:
+ return acpi_index;
+}
+
static int dwc3_qcom_setup_irq(struct platform_device *pdev)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
- const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
+ struct device_node *np = pdev->dev.of_node;
+ const char **irq_names;
+ int port_index;
+ int acpi_index;
+ int irq_count;
+ int irq_index;
int irq;
int ret;
+ int i;

- irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
- pdata ? pdata->hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq);
- if (ret)
- return ret;
- qcom->hs_phy_irq = irq;
- }
+ irq_count = of_property_count_strings(np, "interrupt-names");
+ irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);
+ if (!irq_names)
+ return -ENOMEM;

- irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
- pdata ? pdata->dp_hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq);
- if (ret)
- return ret;
- qcom->dp_hs_phy_irq = irq;
- }
+ ret = of_property_read_string_array(np, "interrupt-names",
+ irq_names, irq_count);
+ for (i = 0; i < irq_count; i++) {
+ irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
+ if (irq_index == -1) {
+ dev_dbg(&pdev->dev, "Invalid IRQ not handled");
+ continue;
+ }

- irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
- pdata ? pdata->dm_hs_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "dm_hs_phy_irq", "qcom_dwc3 DM_HS", irq);
- if (ret)
- return ret;
- qcom->dm_hs_phy_irq = irq;
- }
+ port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index);
+ if (port_index == -1) {
+ dev_dbg(&pdev->dev, "Port index invalid. IRQ not handled");
+ continue;
+ }

- irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
- pdata ? pdata->ss_phy_irq_index : -1);
- if (irq > 0) {
- ret = dwc3_qcom_prep_irq(qcom, "ss_phy_irq", "qcom_dwc3 SS", irq);
- if (ret)
- return ret;
- qcom->ss_phy_irq = irq;
+ acpi_index = dwc3_qcom_get_acpi_index(qcom, irq_index, port_index);
+
+ irq = dwc3_qcom_get_irq(pdev, irq_names[i], acpi_index);
+ if (irq > 0) {
+ ret = dwc3_qcom_prep_irq(qcom, irq_names[i],
+ irq_names[i], irq);
+ if (ret)
+ return ret;
+
+ if (irq_index == HS_PHY_IRQ_INDEX)
+ qcom->hs_phy_irq = irq;
+ else
+ qcom->phy_irq[irq_index][port_index-1] = irq;
+
+ if (qcom->num_ports < port_index)
+ qcom->num_ports = port_index;
+ }
}

return 0;
@@ -1026,20 +1128,16 @@ static const struct dwc3_acpi_pdata sdm845_acpi_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
+ .phy_irq_index = {4, 3, 2},
.hs_phy_irq_index = 1,
- .dp_hs_phy_irq_index = 4,
- .dm_hs_phy_irq_index = 3,
- .ss_phy_irq_index = 2
};

static const struct dwc3_acpi_pdata sdm845_acpi_urs_pdata = {
.qscratch_base_offset = SDM845_QSCRATCH_BASE_OFFSET,
.qscratch_base_size = SDM845_QSCRATCH_SIZE,
.dwc3_core_base_size = SDM845_DWC3_CORE_SIZE,
+ .phy_irq_index = {4, 3, 2},
.hs_phy_irq_index = 1,
- .dp_hs_phy_irq_index = 4,
- .dm_hs_phy_irq_index = 3,
- .ss_phy_irq_index = 2,
.is_urs = true,
};

--
2.42.0

2023-10-07 15:50:04

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

Enable tertiary controller for SA8295P (based on SC8280XP).
Add pinctrl support for usb ports to provide VBUS to connected peripherals.

Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
1 file changed, 49 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
index fd253942e5e5..271000163823 100644
--- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
+++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
@@ -9,6 +9,7 @@
#include <dt-bindings/gpio/gpio.h>
#include <dt-bindings/regulator/qcom,rpmh-regulator.h>
#include <dt-bindings/spmi/spmi.h>
+#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>

#include "sa8540p.dtsi"
#include "sa8540p-pmics.dtsi"
@@ -584,6 +585,16 @@ &usb_1_qmpphy {
status = "okay";
};

+&usb_2 {
+ pinctrl-0 = <&usb2_en_state>,
+ <&usb3_en_state>,
+ <&usb4_en_state>,
+ <&usb5_en_state>;
+ pinctrl-names = "default";
+
+ status = "okay";
+};
+
&usb_2_hsphy0 {
vdda-pll-supply = <&vreg_l5a>;
vdda18-supply = <&vreg_l7g>;
@@ -729,3 +740,41 @@ wake-n-pins {
};
};
};
+
+&pmm8540c_gpios {
+ usb2_en_state: usb2-en-state {
+ pins = "gpio9";
+ function = "normal";
+ qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+ output-high;
+ power-source = <0>;
+ };
+};
+
+&pmm8540e_gpios {
+ usb3_en_state: usb3-en-state {
+ pins = "gpio5";
+ function = "normal";
+ qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+ output-high;
+ power-source = <0>;
+ };
+};
+
+&pmm8540g_gpios {
+ usb4_en_state: usb4-en-state {
+ pins = "gpio5";
+ function = "normal";
+ qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+ output-high;
+ power-source = <0>;
+ };
+
+ usb5_en_state: usb5-en-state {
+ pins = "gpio9";
+ function = "normal";
+ qcom,drive-strength = <PMIC_GPIO_STRENGTH_HIGH>;
+ output-high;
+ power-source = <0>;
+ };
+};
--
2.42.0

2023-10-07 15:50:06

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 04/10] usb: dwc3: qcom: Add helper function to request threaded IRQ

Cleanup setup irq call by implementing a new prep_irq helper function
and using it to request threaded IRQ's.

Signed-off-by: Krishna Kurapati <[email protected]>
Reviewed-by: Bjorn Andersson <[email protected]>
---
drivers/usb/dwc3/dwc3-qcom.c | 59 ++++++++++++++++--------------------
1 file changed, 26 insertions(+), 33 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
index 3de43df6bbe8..ef2006db7601 100644
--- a/drivers/usb/dwc3/dwc3-qcom.c
+++ b/drivers/usb/dwc3/dwc3-qcom.c
@@ -535,6 +535,24 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
return ret;
}

+static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
+ char *disp_name, int irq)
+{
+ int ret;
+
+ /* Keep wakeup interrupts disabled until suspend */
+ irq_set_status_flags(irq, IRQ_NOAUTOEN);
+ ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
+ qcom_dwc3_resume_irq,
+ IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
+ disp_name, qcom);
+
+ if (ret)
+ dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);
+
+ return ret;
+}
+
static int dwc3_qcom_setup_irq(struct platform_device *pdev)
{
struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
@@ -545,61 +563,36 @@ static int dwc3_qcom_setup_irq(struct platform_device *pdev)
irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
pdata ? pdata->hs_phy_irq_index : -1);
if (irq > 0) {
- /* Keep wakeup interrupts disabled until suspend */
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq);
+ if (ret)
return ret;
- }
qcom->hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
pdata ? pdata->dp_hs_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 DP_HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "dp_hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq);
+ if (ret)
return ret;
- }
qcom->dp_hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
pdata ? pdata->dm_hs_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 DM_HS", qcom);
- if (ret) {
- dev_err(qcom->dev, "dm_hs_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "dm_hs_phy_irq", "qcom_dwc3 DM_HS", irq);
+ if (ret)
return ret;
- }
qcom->dm_hs_phy_irq = irq;
}

irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
pdata ? pdata->ss_phy_irq_index : -1);
if (irq > 0) {
- irq_set_status_flags(irq, IRQ_NOAUTOEN);
- ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
- qcom_dwc3_resume_irq,
- IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
- "qcom_dwc3 SS", qcom);
- if (ret) {
- dev_err(qcom->dev, "ss_phy_irq failed: %d\n", ret);
+ ret = dwc3_qcom_prep_irq(qcom, "ss_phy_irq", "qcom_dwc3 SS", irq);
+ if (ret)
return ret;
- }
qcom->ss_phy_irq = irq;
}

--
2.42.0

2023-10-07 15:50:10

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

Add USB and DWC3 node for tertiary port of SC8280 along with multiport
IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
platforms.

Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
1 file changed, 84 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
index cad59af7ccef..5f64f75b07db 100644
--- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
@@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
};

+ usb_2: usb@a4f8800 {
+ compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
+ reg = <0 0x0a4f8800 0 0x400>;
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ clocks = <&gcc GCC_CFG_NOC_USB3_MP_AXI_CLK>,
+ <&gcc GCC_USB30_MP_MASTER_CLK>,
+ <&gcc GCC_AGGRE_USB3_MP_AXI_CLK>,
+ <&gcc GCC_USB30_MP_SLEEP_CLK>,
+ <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+ <&gcc GCC_AGGRE_USB_NOC_AXI_CLK>,
+ <&gcc GCC_AGGRE_USB_NOC_NORTH_AXI_CLK>,
+ <&gcc GCC_AGGRE_USB_NOC_SOUTH_AXI_CLK>,
+ <&gcc GCC_SYS_NOC_USB_AXI_CLK>;
+ clock-names = "cfg_noc", "core", "iface", "sleep", "mock_utmi",
+ "noc_aggr", "noc_aggr_north", "noc_aggr_south", "noc_sys";
+
+ assigned-clocks = <&gcc GCC_USB30_MP_MOCK_UTMI_CLK>,
+ <&gcc GCC_USB30_MP_MASTER_CLK>;
+ assigned-clock-rates = <19200000>, <200000000>;
+
+ interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 126 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 129 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 128 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 131 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 130 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 133 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 132 IRQ_TYPE_EDGE_RISING>,
+ <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
+ <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
+ <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
+
+ interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
+ "dp_hs_phy_2", "dm_hs_phy_2",
+ "dp_hs_phy_3", "dm_hs_phy_3",
+ "dp_hs_phy_4", "dm_hs_phy_4",
+ "ss_phy_1", "ss_phy_2",
+ "pwr_event_1",
+ "pwr_event_2",
+ "pwr_event_3",
+ "pwr_event_4";
+
+ power-domains = <&gcc USB30_MP_GDSC>;
+ required-opps = <&rpmhpd_opp_nom>;
+
+ resets = <&gcc GCC_USB30_MP_BCR>;
+
+ interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>,
+ <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>;
+ interconnect-names = "usb-ddr", "apps-usb";
+
+ wakeup-source;
+
+ status = "disabled";
+
+ usb_2_dwc3: usb@a400000 {
+ compatible = "snps,dwc3";
+ reg = <0 0x0a400000 0 0xcd00>;
+ interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
+ iommus = <&apps_smmu 0x800 0x0>;
+ phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+ <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+ <&usb_2_hsphy2>,
+ <&usb_2_hsphy3>;
+ phy-names = "usb2-port0", "usb3-port0",
+ "usb2-port1", "usb3-port1",
+ "usb2-port2",
+ "usb2-port3";
+
+ /*
+ * Multiport controllers are host only contollers, so
+ * the dr_mode can be defaulted to host irrespective of
+ * the platform.
+ */
+ dr_mode = "host";
+ };
+ };
+
usb_0: usb@a6f8800 {
compatible = "qcom,sc8280xp-dwc3", "qcom,dwc3";
reg = <0 0x0a6f8800 0 0x400>;
--
2.42.0

2023-10-07 15:50:12

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

From: Harsh Agarwal <[email protected]>

Currently the DWC3 driver supports only single port controller
which requires at most one HS and one SS PHY.

But the DWC3 USB controller can be connected to multiple ports and
each port can have their own PHYs. Each port of the multiport
controller can either be HS+SS capable or HS only capable
Proper quantification of them is required to modify GUSB2PHYCFG
and GUSB3PIPECTL registers appropriately.

Add support for detecting, obtaining and configuring phy's supported
by a multiport controller and. Limit the max number of ports
supported to 4 as only SC8280 which is a quad port controller supports
Multiport currently.

Reported-by: kernel test robot <[email protected]>
Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
Co-developed-by: Harsh Agarwal <[email protected]>
Signed-off-by: Harsh Agarwal <[email protected]>
Co-developed-by:Krishna Kurapati <[email protected]>
Signed-off-by: Krishna Kurapati <[email protected]>
---
Changes in v13:
Compiler issues found by kernel test robot have been fixed and tags added.
So removing maintainers reviewed-by tag as we have made a minor change
in the patch.

drivers/usb/dwc3/core.c | 252 +++++++++++++++++++++++++++-------------
drivers/usb/dwc3/core.h | 11 +-
drivers/usb/dwc3/drd.c | 15 ++-
3 files changed, 190 insertions(+), 88 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 3e507e4cc542..f380cbf908a9 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -124,6 +124,7 @@ static void __dwc3_set_mode(struct work_struct *work)
int ret;
u32 reg;
u32 desired_dr_role;
+ int i;

mutex_lock(&dwc->mutex);
spin_lock_irqsave(&dwc->lock, flags);
@@ -201,8 +202,10 @@ static void __dwc3_set_mode(struct work_struct *work)
} else {
if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, true);
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
- phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+ phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
+ }
if (dwc->dis_split_quirk) {
reg = dwc3_readl(dwc->regs, DWC3_GUCTL3);
reg |= DWC3_GUCTL3_SPLITDISABLE;
@@ -217,8 +220,8 @@ static void __dwc3_set_mode(struct work_struct *work)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, false);
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
- phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);

ret = dwc3_gadget_init(dwc);
if (ret)
@@ -589,22 +592,14 @@ static int dwc3_core_ulpi_init(struct dwc3 *dwc)
return ret;
}

-/**
- * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
- * @dwc: Pointer to our controller context structure
- *
- * Returns 0 on success. The USB PHY interfaces are configured but not
- * initialized. The PHY interfaces and the PHYs get initialized together with
- * the core in dwc3_core_init.
- */
-static int dwc3_phy_setup(struct dwc3 *dwc)
+static int dwc3_ss_phy_setup(struct dwc3 *dwc, int index)
{
unsigned int hw_mode;
u32 reg;

hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);

- reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(index));

/*
* Make sure UX_EXIT_PX is cleared as that causes issues with some
@@ -659,9 +654,19 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
if (dwc->dis_del_phy_power_chg_quirk)
reg &= ~DWC3_GUSB3PIPECTL_DEPOCHANGE;

- dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+ dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(index), reg);

- reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
+ return 0;
+}
+
+static int dwc3_hs_phy_setup(struct dwc3 *dwc, int index)
+{
+ unsigned int hw_mode;
+ u32 reg;
+
+ hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(index));

/* Select the HS PHY interface */
switch (DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3)) {
@@ -673,7 +678,7 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
} else if (dwc->hsphy_interface &&
!strncmp(dwc->hsphy_interface, "ulpi", 4)) {
reg |= DWC3_GUSB2PHYCFG_ULPI_UTMI;
- dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
} else {
/* Relying on default value. */
if (!(reg & DWC3_GUSB2PHYCFG_ULPI_UTMI))
@@ -740,7 +745,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
if (dwc->ulpi_ext_vbus_drv)
reg |= DWC3_GUSB2PHYCFG_ULPIEXTVBUSDRV;

- dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(index), reg);
+
+ return 0;
+}
+
+/**
+ * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
+ * @dwc: Pointer to our controller context structure
+ *
+ * Returns 0 on success. The USB PHY interfaces are configured but not
+ * initialized. The PHY interfaces and the PHYs get initialized together with
+ * the core in dwc3_core_init.
+ */
+static int dwc3_phy_setup(struct dwc3 *dwc)
+{
+ int i;
+ int ret;
+
+ for (i = 0; i < dwc->num_usb3_ports; i++) {
+ ret = dwc3_ss_phy_setup(dwc, i);
+ if (ret)
+ return ret;
+ }
+
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ ret = dwc3_hs_phy_setup(dwc, i);
+ if (ret)
+ return ret;
+ }

return 0;
}
@@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
static int dwc3_phy_init(struct dwc3 *dwc)
{
int ret;
+ int i;
+ int j;

usb_phy_init(dwc->usb2_phy);
usb_phy_init(dwc->usb3_phy);

- ret = phy_init(dwc->usb2_generic_phy);
- if (ret < 0)
- goto err_shutdown_usb3_phy;
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ ret = phy_init(dwc->usb2_generic_phy[i]);
+ if (ret < 0)
+ goto err_exit_phy;

- ret = phy_init(dwc->usb3_generic_phy);
- if (ret < 0)
- goto err_exit_usb2_phy;
+ ret = phy_init(dwc->usb3_generic_phy[i]);
+ if (ret < 0) {
+ phy_exit(dwc->usb2_generic_phy[i]);
+ goto err_exit_phy;
+ }
+ }

return 0;

-err_exit_usb2_phy:
- phy_exit(dwc->usb2_generic_phy);
-err_shutdown_usb3_phy:
+err_exit_phy:
+ for (j = i - 1; j >= 0; j--) {
+ phy_exit(dwc->usb2_generic_phy[j]);
+ phy_exit(dwc->usb3_generic_phy[j]);
+ }
+
usb_phy_shutdown(dwc->usb3_phy);
usb_phy_shutdown(dwc->usb2_phy);

@@ -773,8 +815,12 @@ static int dwc3_phy_init(struct dwc3 *dwc)

static void dwc3_phy_exit(struct dwc3 *dwc)
{
- phy_exit(dwc->usb3_generic_phy);
- phy_exit(dwc->usb2_generic_phy);
+ int i;
+
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_exit(dwc->usb3_generic_phy[i]);
+ phy_exit(dwc->usb2_generic_phy[i]);
+ }

usb_phy_shutdown(dwc->usb3_phy);
usb_phy_shutdown(dwc->usb2_phy);
@@ -783,23 +829,32 @@ static void dwc3_phy_exit(struct dwc3 *dwc)
static int dwc3_phy_power_on(struct dwc3 *dwc)
{
int ret;
+ int i;
+ int j;

usb_phy_set_suspend(dwc->usb2_phy, 0);
usb_phy_set_suspend(dwc->usb3_phy, 0);

- ret = phy_power_on(dwc->usb2_generic_phy);
- if (ret < 0)
- goto err_suspend_usb3_phy;
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ ret = phy_power_on(dwc->usb2_generic_phy[i]);
+ if (ret < 0)
+ goto err_power_off_phy;

- ret = phy_power_on(dwc->usb3_generic_phy);
- if (ret < 0)
- goto err_power_off_usb2_phy;
+ ret = phy_power_on(dwc->usb3_generic_phy[i]);
+ if (ret < 0) {
+ phy_power_off(dwc->usb2_generic_phy[i]);
+ goto err_power_off_phy;
+ }
+ }

return 0;

-err_power_off_usb2_phy:
- phy_power_off(dwc->usb2_generic_phy);
-err_suspend_usb3_phy:
+err_power_off_phy:
+ for (j = i - 1; j >= 0; j--) {
+ phy_power_off(dwc->usb2_generic_phy[j]);
+ phy_power_off(dwc->usb3_generic_phy[j]);
+ }
+
usb_phy_set_suspend(dwc->usb3_phy, 1);
usb_phy_set_suspend(dwc->usb2_phy, 1);

@@ -808,8 +863,12 @@ static int dwc3_phy_power_on(struct dwc3 *dwc)

static void dwc3_phy_power_off(struct dwc3 *dwc)
{
- phy_power_off(dwc->usb3_generic_phy);
- phy_power_off(dwc->usb2_generic_phy);
+ int i;
+
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_power_off(dwc->usb3_generic_phy[i]);
+ phy_power_off(dwc->usb2_generic_phy[i]);
+ }

usb_phy_set_suspend(dwc->usb3_phy, 1);
usb_phy_set_suspend(dwc->usb2_phy, 1);
@@ -1187,6 +1246,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
unsigned int hw_mode;
u32 reg;
int ret;
+ int i;

hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);

@@ -1230,15 +1290,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
if (hw_mode == DWC3_GHWPARAMS0_MODE_DRD &&
!DWC3_VER_IS_WITHIN(DWC3, ANY, 194A)) {
if (!dwc->dis_u3_susphy_quirk) {
- reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0));
- reg |= DWC3_GUSB3PIPECTL_SUSPHY;
- dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg);
+ for (i = 0; i < dwc->num_usb3_ports; i++) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(i));
+ reg |= DWC3_GUSB3PIPECTL_SUSPHY;
+ dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(i), reg);
+ }
}

if (!dwc->dis_u2_susphy_quirk) {
- reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
- reg |= DWC3_GUSB2PHYCFG_SUSPHY;
- dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+ reg |= DWC3_GUSB2PHYCFG_SUSPHY;
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+ }
}
}

@@ -1346,7 +1410,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
{
struct device *dev = dwc->dev;
struct device_node *node = dev->of_node;
+ char phy_name[13];
int ret;
+ int i;

if (node) {
dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
@@ -1372,22 +1438,36 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
return dev_err_probe(dev, ret, "no usb3 phy configured\n");
}

- dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
- if (IS_ERR(dwc->usb2_generic_phy)) {
- ret = PTR_ERR(dwc->usb2_generic_phy);
- if (ret == -ENOSYS || ret == -ENODEV)
- dwc->usb2_generic_phy = NULL;
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ if (dwc->num_usb2_ports == 1)
+ sprintf(phy_name, "usb2-phy");
else
- return dev_err_probe(dev, ret, "no usb2 phy configured\n");
- }
+ sprintf(phy_name, "usb2-port%d", i);

- dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
- if (IS_ERR(dwc->usb3_generic_phy)) {
- ret = PTR_ERR(dwc->usb3_generic_phy);
- if (ret == -ENOSYS || ret == -ENODEV)
- dwc->usb3_generic_phy = NULL;
+ dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
+ if (IS_ERR(dwc->usb2_generic_phy[i])) {
+ ret = PTR_ERR(dwc->usb2_generic_phy[i]);
+ if (ret == -ENOSYS || ret == -ENODEV)
+ dwc->usb2_generic_phy[i] = NULL;
+ else
+ return dev_err_probe(dev, ret,
+ "failed to lookup phy %s\n", phy_name);
+ }
+
+ if (dwc->num_usb2_ports == 1)
+ sprintf(phy_name, "usb3-phy");
else
- return dev_err_probe(dev, ret, "no usb3 phy configured\n");
+ sprintf(phy_name, "usb3-port%d", i);
+
+ dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
+ if (IS_ERR(dwc->usb3_generic_phy[i])) {
+ ret = PTR_ERR(dwc->usb3_generic_phy[i]);
+ if (ret == -ENOSYS || ret == -ENODEV)
+ dwc->usb3_generic_phy[i] = NULL;
+ else
+ return dev_err_probe(dev, ret,
+ "failed to lookup phy %s\n", phy_name);
+ }
}

return 0;
@@ -1397,6 +1477,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
{
struct device *dev = dwc->dev;
int ret;
+ int i;

switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
@@ -1404,8 +1485,8 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, false);
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
- phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
+ phy_set_mode(dwc->usb3_generic_phy[0], PHY_MODE_USB_DEVICE);

ret = dwc3_gadget_init(dwc);
if (ret)
@@ -1416,8 +1497,10 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, true);
- phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST);
- phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST);
+ phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST);
+ }

ret = dwc3_host_init(dwc);
if (ret)
@@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)

dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
dwc->num_usb2_ports, dwc->num_usb3_ports);
-
iounmap(base);

+ if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
+ (dwc->num_usb3_ports > DWC3_MAX_PORTS))
+ return -ENOMEM;
+
return 0;
}

@@ -2130,6 +2216,7 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
{
unsigned long flags;
u32 reg;
+ int i;

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
@@ -2148,17 +2235,21 @@ static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
/* Let controller to suspend HSPHY before PHY driver suspends */
if (dwc->dis_u2_susphy_quirk ||
dwc->dis_enblslpm_quirk) {
- reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
- reg |= DWC3_GUSB2PHYCFG_ENBLSLPM |
- DWC3_GUSB2PHYCFG_SUSPHY;
- dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+ reg |= DWC3_GUSB2PHYCFG_ENBLSLPM |
+ DWC3_GUSB2PHYCFG_SUSPHY;
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+ }

/* Give some time for USB2 PHY to suspend */
usleep_range(5000, 6000);
}

- phy_pm_runtime_put_sync(dwc->usb2_generic_phy);
- phy_pm_runtime_put_sync(dwc->usb3_generic_phy);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_pm_runtime_put_sync(dwc->usb2_generic_phy[i]);
+ phy_pm_runtime_put_sync(dwc->usb3_generic_phy[i]);
+ }
break;
case DWC3_GCTL_PRTCAP_OTG:
/* do nothing during runtime_suspend */
@@ -2188,6 +2279,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
unsigned long flags;
int ret;
u32 reg;
+ int i;

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
@@ -2207,17 +2299,21 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
break;
}
/* Restore GUSB2PHYCFG bits that were modified in suspend */
- reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0));
- if (dwc->dis_u2_susphy_quirk)
- reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
+ if (dwc->dis_u2_susphy_quirk)
+ reg &= ~DWC3_GUSB2PHYCFG_SUSPHY;

- if (dwc->dis_enblslpm_quirk)
- reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;
+ if (dwc->dis_enblslpm_quirk)
+ reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM;

- dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg);
+ dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
+ }

- phy_pm_runtime_get_sync(dwc->usb2_generic_phy);
- phy_pm_runtime_get_sync(dwc->usb3_generic_phy);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ phy_pm_runtime_get_sync(dwc->usb2_generic_phy[i]);
+ phy_pm_runtime_get_sync(dwc->usb3_generic_phy[i]);
+ }
break;
case DWC3_GCTL_PRTCAP_OTG:
/* nothing to do on runtime_resume */
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2ea6df7e6571..fc5d15edab1c 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -33,6 +33,9 @@

#include <linux/power_supply.h>

+/* Number of ports supported by a multiport controller */
+#define DWC3_MAX_PORTS 4
+
#define DWC3_MSG_MAX 500

/* Global constants */
@@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array {
* @usb_psy: pointer to power supply interface.
* @usb2_phy: pointer to USB2 PHY
* @usb3_phy: pointer to USB3 PHY
- * @usb2_generic_phy: pointer to USB2 PHY
- * @usb3_generic_phy: pointer to USB3 PHY
+ * @usb2_generic_phy: pointer to array of USB2 PHY
+ * @usb3_generic_phy: pointer to array of USB3 PHY
* @num_usb2_ports: number of USB2 ports
* @num_usb3_ports: number of USB3 ports
* @phys_ready: flag to indicate that PHYs are ready
@@ -1173,8 +1176,8 @@ struct dwc3 {
struct usb_phy *usb2_phy;
struct usb_phy *usb3_phy;

- struct phy *usb2_generic_phy;
- struct phy *usb3_generic_phy;
+ struct phy *usb2_generic_phy[DWC3_MAX_PORTS];
+ struct phy *usb3_generic_phy[DWC3_MAX_PORTS];

u8 num_usb2_ports;
u8 num_usb3_ports;
diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
index 039bf241769a..9aec41f1ad43 100644
--- a/drivers/usb/dwc3/drd.c
+++ b/drivers/usb/dwc3/drd.c
@@ -331,6 +331,7 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
u32 reg;
int id;
unsigned long flags;
+ int i;

if (dwc->dr_mode != USB_DR_MODE_OTG)
return;
@@ -386,9 +387,12 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
} else {
if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, true);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy,
- PHY_MODE_USB_HOST);
+ for (i = 0; i < dwc->num_usb2_ports; i++) {
+ if (dwc->usb2_generic_phy[i]) {
+ phy_set_mode(dwc->usb2_generic_phy[i],
+ PHY_MODE_USB_HOST);
+ }
+ }
}
break;
case DWC3_OTG_ROLE_DEVICE:
@@ -400,9 +404,8 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)

if (dwc->usb2_phy)
otg_set_vbus(dwc->usb2_phy->otg, false);
- if (dwc->usb2_generic_phy)
- phy_set_mode(dwc->usb2_generic_phy,
- PHY_MODE_USB_DEVICE);
+ if (dwc->usb2_generic_phy[0])
+ phy_set_mode(dwc->usb2_generic_phy[0], PHY_MODE_USB_DEVICE);
ret = dwc3_gadget_init(dwc);
if (ret)
dev_err(dwc->dev, "failed to initialize peripheral\n");
--
2.42.0

2023-10-07 15:50:21

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: [PATCH v13 10/10] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

From: Andrew Halaney <[email protected]>

There is now support for the multiport USB controller this uses so
enable it.

The board only has a single port hooked up (despite it being wired up to
the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
which by default on boot is selected to mux properly. Grab the gpio
controlling that and ensure it stays in the right position so USB 2.0
continues to be routed from the external port to the SoC.

Co-developed-by: Andrew Halaney <[email protected]>
Signed-off-by: Andrew Halaney <[email protected]>
[Krishna: Rebased on top of usb-next]
Co-developed-by: Krishna Kurapati <[email protected]>
Signed-off-by: Krishna Kurapati <[email protected]>
---
arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
index b04f72ec097c..6904a4c201ff 100644
--- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
+++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
@@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
status = "okay";
};

+&usb_2 {
+ pinctrl-0 = <&usb2_en_state>;
+ pinctrl-names = "default";
+
+ status = "okay";
+};
+
+&usb_2_dwc3 {
+ phy-names = "usb2-port0", "usb3-port0";
+ phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
+};
+
&xo_board_clk {
clock-frequency = <38400000>;
};
@@ -655,4 +667,13 @@ wake-pins {
bias-pull-up;
};
};
+
+ usb2_en_state: usb2-en-state {
+ /* TS3USB221A USB2.0 mux select */
+ pins = "gpio24";
+ function = "gpio";
+ drive-strength = <2>;
+ bias-disable;
+ output-low;
+ };
};
--
2.42.0

2023-10-08 10:43:42

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers

On 07/10/2023 17:47, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller which
> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
> DWC3 controller with multiple ports that can operate in host mode.
> Some of the port supports both SS+HS and other port supports only HS
> mode.
>
> This change primarily refactors the Phy logic in core driver to allow
> multiport support with Generic Phy's.
>
> Changes have been tested on QCOM SoC SA8295P which has 4 ports (2
> are HS+SS capable and 2 are HS only capable).
>

I think I said it few times on the lists to Qualcomm folks, although I
cannot remember whether exactly in this patchset. Please split DTS from
USB, because Greg prefers to grab everything and DTS *should go* via
Qualcomm SoC.

Best regards,
Krzysztof

2023-10-08 11:02:07

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers



On 10/8/2023 4:13 PM, Krzysztof Kozlowski wrote:
> On 07/10/2023 17:47, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller which
>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>> DWC3 controller with multiple ports that can operate in host mode.
>> Some of the port supports both SS+HS and other port supports only HS
>> mode.
>>
>> This change primarily refactors the Phy logic in core driver to allow
>> multiport support with Generic Phy's.
>>
>> Changes have been tested on QCOM SoC SA8295P which has 4 ports (2
>> are HS+SS capable and 2 are HS only capable).
>>
>
> I think I said it few times on the lists to Qualcomm folks, although I
> cannot remember whether exactly in this patchset. Please split DTS from
> USB, because Greg prefers to grab everything and DTS *should go* via
> Qualcomm SoC.
>
Hi Krzyztof,

Apologies !

Do you mean to send the DTS just to linux-arm-msm list and not linux-usb
list ? I don't think it was posted on this series and I am not on
linux-arm-msm mailing list, so missed that comment. Sorry for that. I
saw some series where DT, Driver and bindings were sent as one set to
linux-usb list as well and so wanted to follow suite. Will make sure to
send DT just to linux-arm-msm list from now on. Thanks for the comments :-)

Regards,
Krishna,

2023-10-08 11:09:52

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers

On 08/10/2023 13:01, Krishna Kurapati PSSNV wrote:
>
>
> On 10/8/2023 4:13 PM, Krzysztof Kozlowski wrote:
>> On 07/10/2023 17:47, Krishna Kurapati wrote:
>>> Currently the DWC3 driver supports only single port controller which
>>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>>> DWC3 controller with multiple ports that can operate in host mode.
>>> Some of the port supports both SS+HS and other port supports only HS
>>> mode.
>>>
>>> This change primarily refactors the Phy logic in core driver to allow
>>> multiport support with Generic Phy's.
>>>
>>> Changes have been tested on QCOM SoC SA8295P which has 4 ports (2
>>> are HS+SS capable and 2 are HS only capable).
>>>
>>
>> I think I said it few times on the lists to Qualcomm folks, although I
>> cannot remember whether exactly in this patchset. Please split DTS from
>> USB, because Greg prefers to grab everything and DTS *should go* via
>> Qualcomm SoC.
>>
> Hi Krzyztof,
>
> Apologies !
>
> Do you mean to send the DTS just to linux-arm-msm list and not linux-usb
> list ?


Not entirely, I meant to create separate patchsets. One for USB drivers
and other one for Qualcomm SoC. The DTS patchset should have link to
lore to USB drivers patchset. You send each of the patchsets to
respective maintainer, lists and everyone necessary using b4 or
scripts/get_maintainers.pl.


I don't think it was posted on this series and I am not on
> linux-arm-msm mailing list, so missed that comment. Sorry for that. I
> saw some series where DT, Driver and bindings were sent as one set to
> linux-usb list as well and so wanted to follow suite. Will make sure to
> send DT just to linux-arm-msm list from now on. Thanks for the comments :-)


Best regards,
Krzysztof

2023-10-08 11:11:53

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

On 07/10/2023 17:48, Krishna Kurapati wrote:
> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
> platforms.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index cad59af7ccef..5f64f75b07db 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> + usb_2: usb@a4f8800 {
> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";

There is no such compatible. Scrolling through 3 pages of cover letter,
I did not find dependency listed. I did not scroll more - dependency is
the most important piece, so I would assume it is in the top-part.

Best regards,
Krzysztof

2023-10-08 11:22:10

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280



On 10/8/2023 4:41 PM, Krzysztof Kozlowski wrote:
> On 07/10/2023 17:48, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>> 1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index cad59af7ccef..5f64f75b07db 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> + usb_2: usb@a4f8800 {
>> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
>
> There is no such compatible. Scrolling through 3 pages of cover letter,
> I did not find dependency listed. I did not scroll more - dependency is
> the most important piece, so I would assume it is in the top-part.
>
Hi Krzysztof,

I rebased the series on top of latest usb-next. I mentioned in cover
letter that the first 3 patches have been merged on usb-next and this
series has the remaining 10 patches. Apologies, I must have mentioned
commit number of the merged patches as well for better context. The
compatible was merged in the patch [1].

[1]:
https://lore.kernel.org/all/[email protected]/

Regards,
Krishna,

2023-10-08 11:23:41

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

On 08/10/2023 13:21, Krishna Kurapati PSSNV wrote:
>
>
> On 10/8/2023 4:41 PM, Krzysztof Kozlowski wrote:
>> On 07/10/2023 17:48, Krishna Kurapati wrote:
>>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>>> platforms.
>>>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>> ---
>>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>>> 1 file changed, 84 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> index cad59af7ccef..5f64f75b07db 100644
>>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>>> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>>> };
>>>
>>> + usb_2: usb@a4f8800 {
>>> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
>>
>> There is no such compatible. Scrolling through 3 pages of cover letter,
>> I did not find dependency listed. I did not scroll more - dependency is
>> the most important piece, so I would assume it is in the top-part.
>>
> Hi Krzysztof,
>
> I rebased the series on top of latest usb-next. I mentioned in cover
> letter that the first 3 patches have been merged on usb-next and this
> series has the remaining 10 patches. Apologies, I must have mentioned
> commit number of the merged patches as well for better context. The
> compatible was merged in the patch [1].

OK, I see it now in latest next.

Best regards,
Krzysztof

2023-10-10 20:52:15

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers



On 10/7/23 17:47, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller which
> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
> DWC3 controller with multiple ports that can operate in host mode.
> Some of the port supports both SS+HS and other port supports only HS
> mode.
>
> This change primarily refactors the Phy logic in core driver to allow
> multiport support with Generic Phy's.
>
> Changes have been tested on QCOM SoC SA8295P which has 4 ports (2
> are HS+SS capable and 2 are HS only capable).
>
> Changes in v13:
> This series is a subset of patches in v11 as the first 3 patches in v11
> have been mereged into usb-next.
> Moved dr_mode property from platform specific files to common sc8280xp DT.
> Fixed function call wrapping, added comments and replaced #defines with
> enum in dwc3-qcom for identifying IRQ index appropriately.
> Fixed nitpicks pointed out in v11 for suspend-resume handling.
> Added reported-by tag for phy refactoring patch as a compile error was
> found by kernel test bot [1].
"If you fix the issue in a separate patch/commit (i.e. not just a new
version of
the same patch/commit), kindly add following tags"

the issue your patch resolves is not one that was reported by the kernel
testing robot, it just pointed out that you need to fix up the next revision

Konrad

2023-10-11 05:12:19

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers



On 10/11/2023 2:21 AM, Konrad Dybcio wrote:
>
>
> On 10/7/23 17:47, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller which
>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>> DWC3 controller with multiple ports that can operate in host mode.
>> Some of the port supports both SS+HS and other port supports only HS
>> mode.
>>
>> This change primarily refactors the Phy logic in core driver to allow
>> multiport support with Generic Phy's.
>>
>> Changes have been tested on  QCOM SoC SA8295P which has 4 ports (2
>> are HS+SS capable and 2 are HS only capable).
>>
>> Changes in v13:
>> This series is a subset of patches in v11 as the first 3 patches in v11
>> have been mereged into usb-next.
>> Moved dr_mode property from platform specific files to common sc8280xp
>> DT.
>> Fixed function call wrapping, added comments and replaced #defines with
>> enum in dwc3-qcom for identifying IRQ index appropriately.
>> Fixed nitpicks pointed out in v11 for suspend-resume handling.
>> Added reported-by tag for phy refactoring patch as a compile error was
>> found by kernel test bot [1].
> "If you fix the issue in a separate patch/commit (i.e. not just a new
> version of
> the same patch/commit), kindly add following tags"
>
> the issue your patch resolves is not one that was reported by the kernel
> testing robot, it just pointed out that you need to fix up the next
> revision
>

I Agree. It sounds wrong to add a reproted-by tag making it seem like a
bug instead of a feature we have written. But if we fix the compile
error mentioned and not add the "reported-by", its like not giving
credit for the reporter. So I put in the reproted by and closes tag to
give a view of what was reported and the feature implemented.

Regards,
Krishna,

2023-10-11 09:36:10

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers



On 10/11/23 07:11, Krishna Kurapati PSSNV wrote:
>
>
> On 10/11/2023 2:21 AM, Konrad Dybcio wrote:
>>
>>
>> On 10/7/23 17:47, Krishna Kurapati wrote:
>>> Currently the DWC3 driver supports only single port controller which
>>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>>> DWC3 controller with multiple ports that can operate in host mode.
>>> Some of the port supports both SS+HS and other port supports only HS
>>> mode.
>>>
>>> This change primarily refactors the Phy logic in core driver to allow
>>> multiport support with Generic Phy's.
>>>
>>> Changes have been tested on  QCOM SoC SA8295P which has 4 ports (2
>>> are HS+SS capable and 2 are HS only capable).
>>>
>>> Changes in v13:
>>> This series is a subset of patches in v11 as the first 3 patches in v11
>>> have been mereged into usb-next.
>>> Moved dr_mode property from platform specific files to common
>>> sc8280xp DT.
>>> Fixed function call wrapping, added comments and replaced #defines with
>>> enum in dwc3-qcom for identifying IRQ index appropriately.
>>> Fixed nitpicks pointed out in v11 for suspend-resume handling.
>>> Added reported-by tag for phy refactoring patch as a compile error was
>>> found by kernel test bot [1].
>> "If you fix the issue in a separate patch/commit (i.e. not just a new
>> version of
>> the same patch/commit), kindly add following tags"
>>
>> the issue your patch resolves is not one that was reported by the
>> kernel testing robot, it just pointed out that you need to fix up the
>> next revision
>>
>
> I Agree. It sounds wrong to add a reproted-by tag making it seem like a
> bug instead of a feature we have written. But if we fix the compile
> error mentioned and not add the "reported-by", its like not giving
> credit for the reporter. So I put in the reproted by and closes tag to
> give a view of what was reported and the feature implemented.
This is a normal thing in review, people spot mistakes, null ptrs, etc..

If I had a reported-by for each review where I pointed out e.g. device
tree changes that don't compile i'd be topping lwn charts

Konrad

2023-10-12 06:18:17

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 00/10] Add multiport support for DWC3 controllers



On 10/11/2023 3:04 PM, Konrad Dybcio wrote:
>
>
> On 10/11/23 07:11, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 10/11/2023 2:21 AM, Konrad Dybcio wrote:
>>>
>>>
>>> On 10/7/23 17:47, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller which
>>>> requires at most two PHYs ie HS and SS PHYs. There are SoCs that has
>>>> DWC3 controller with multiple ports that can operate in host mode.
>>>> Some of the port supports both SS+HS and other port supports only HS
>>>> mode.
>>>>
>>>> This change primarily refactors the Phy logic in core driver to allow
>>>> multiport support with Generic Phy's.
>>>>
>>>> Changes have been tested on  QCOM SoC SA8295P which has 4 ports (2
>>>> are HS+SS capable and 2 are HS only capable).
>>>>
>>>> Changes in v13:
>>>> This series is a subset of patches in v11 as the first 3 patches in v11
>>>> have been mereged into usb-next.
>>>> Moved dr_mode property from platform specific files to common
>>>> sc8280xp DT.
>>>> Fixed function call wrapping, added comments and replaced #defines with
>>>> enum in dwc3-qcom for identifying IRQ index appropriately.
>>>> Fixed nitpicks pointed out in v11 for suspend-resume handling.
>>>> Added reported-by tag for phy refactoring patch as a compile error was
>>>> found by kernel test bot [1].
>>> "If you fix the issue in a separate patch/commit (i.e. not just a new
>>> version of
>>> the same patch/commit), kindly add following tags"
>>>
>>> the issue your patch resolves is not one that was reported by the
>>> kernel testing robot, it just pointed out that you need to fix up the
>>> next revision
>>>
>>
>> I Agree. It sounds wrong to add a reproted-by tag making it seem like
>> a bug instead of a feature we have written. But if we fix the compile
>> error mentioned and not add the "reported-by", its like not giving
>> credit for the reporter. So I put in the reproted by and closes tag to
>> give a view of what was reported and the feature implemented.
> This is a normal thing in review, people spot mistakes, null ptrs, etc..
>
> If I had a reported-by for each review where I pointed out e.g. device
> tree changes that don't compile i'd be topping lwn charts
>

Sure. Will keep this in mind for future patches. And if revising this
again, will remove the above two tags.

Regards,
Krishna,

2023-10-12 16:41:21

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports



On 10/7/23 17:48, Krishna Kurapati wrote:
> Enable tertiary controller for SA8295P (based on SC8280XP).
> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
Reviewed-by: Konrad Dybcio <[email protected]>

Konrad

2023-10-12 16:41:38

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280



On 10/7/23 17:48, Krishna Kurapati wrote:
> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
> platforms.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
[...]

> +
> + interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt SLAVE_EBI1 0>,
> + <&gem_noc MASTER_APPSS_PROC 0 &config_noc SLAVE_USB3_MP 0>;
Please use QCOM_ICC_TAG_ALWAYS from
include/dt-bindings/interconnect/qcom,icc.h (like in sa8775p)

With that I think it's good to go :)

Konrad

2023-10-12 16:42:32

by Konrad Dybcio

[permalink] [raw]
Subject: Re: [PATCH v13 10/10] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller



On 10/7/23 17:48, Krishna Kurapati wrote:
> From: Andrew Halaney <[email protected]>
>
> There is now support for the multiport USB controller this uses so
> enable it.
>
> The board only has a single port hooked up (despite it being wired up to
> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> which by default on boot is selected to mux properly. Grab the gpio
> controlling that and ensure it stays in the right position so USB 2.0
> continues to be routed from the external port to the SoC.
>
> Co-developed-by: Andrew Halaney <[email protected]>
> Signed-off-by: Andrew Halaney <[email protected]>
> [Krishna: Rebased on top of usb-next]
> Co-developed-by: Krishna Kurapati <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index b04f72ec097c..6904a4c201ff 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> status = "okay";
> };
>
> +&usb_2 {
> + pinctrl-0 = <&usb2_en_state>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +};
> +
> +&usb_2_dwc3 {
These additions are not quite sorted alphabetically, it seems

> + phy-names = "usb2-port0", "usb3-port0";
> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
property
property-names

With that:

Reviewed-by: Konrad Dybcio <[email protected]>

Konrad

2023-10-12 17:03:32

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280



On 10/12/2023 10:10 PM, Konrad Dybcio wrote:
>
>
> On 10/7/23 17:48, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
> [...]
>
>> +
>> +            interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt
>> SLAVE_EBI1 0>,
>> +                    <&gem_noc MASTER_APPSS_PROC 0 &config_noc
>> SLAVE_USB3_MP 0>;
> Please use QCOM_ICC_TAG_ALWAYS from
> include/dt-bindings/interconnect/qcom,icc.h (like in sa8775p)
>
> With that I think it's good to go :)
>
Hi Konrad. Thanks for the review.

I see that the tags are used fr spi/i2c but not usb. So to maintain
uniformity, wanted to keep the same here.

Regards,
Krishna,

2023-10-12 17:38:05

by Thinh Nguyen

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

Hi,

On Sat, Oct 07, 2023, Krishna Kurapati wrote:
> From: Harsh Agarwal <[email protected]>
>
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.
>
> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
>
> Add support for detecting, obtaining and configuring phy's supported
> by a multiport controller and. Limit the max number of ports
> supported to 4 as only SC8280 which is a quad port controller supports
> Multiport currently.
>
> Reported-by: kernel test robot <[email protected]>
> Closes: https://urldefense.com/v3/__https://lore.kernel.org/oe-kbuild-all/[email protected]/__;!!A4F2R9G_pg!fTcK8QrbE_IGWMo0AY7E6Wi_MSBhyN_Q1jt8aTsB4s0YTY7ltK3pVHyEc4JlBBL3NClZD1vhKmKCWxXabuaXaSQvka5emA$
> Co-developed-by: Harsh Agarwal <[email protected]>
> Signed-off-by: Harsh Agarwal <[email protected]>
> Co-developed-by:Krishna Kurapati <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> Changes in v13:
> Compiler issues found by kernel test robot have been fixed and tags added.
> So removing maintainers reviewed-by tag as we have made a minor change
> in the patch.
>
> drivers/usb/dwc3/core.c | 252 +++++++++++++++++++++++++++-------------
> drivers/usb/dwc3/core.h | 11 +-
> drivers/usb/dwc3/drd.c | 15 ++-
> 3 files changed, 190 insertions(+), 88 deletions(-)
>

Please check and address checkpatch issues. You can ignore the ENOSYS
warnings.

WARNING:BAD_SIGN_OFF: Co-developed-by: should not be used to attribute nominal patch author 'Harsh Agarwal <[email protected]>'
#41:
Co-developed-by: Harsh Agarwal <[email protected]>

WARNING:BAD_SIGN_OFF: Use a single space after Co-developed-by:
#43:
Co-developed-by:Krishna Kurapati <[email protected]>

WARNING:ENOSYS: ENOSYS means 'invalid syscall nr' and nothing else
#368: FILE: drivers/usb/dwc3/core.c:1450:
+ if (ret == -ENOSYS || ret == -ENODEV)

WARNING:ENOSYS: ENOSYS means 'invalid syscall nr' and nothing else
#384: FILE: drivers/usb/dwc3/core.c:1465:
+ if (ret == -ENOSYS || ret == -ENODEV)

CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'dwc->num_usb2_ports > DWC3_MAX_PORTS'
#432: FILE: drivers/usb/dwc3/core.c:1980:
+ if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
+ (dwc->num_usb3_ports > DWC3_MAX_PORTS))

CHECK:UNNECESSARY_PARENTHESES: Unnecessary parentheses around 'dwc->num_usb3_ports > DWC3_MAX_PORTS'
#432: FILE: drivers/usb/dwc3/core.c:1980:
+ if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
+ (dwc->num_usb3_ports > DWC3_MAX_PORTS))

CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#433: FILE: drivers/usb/dwc3/core.c:1981:
+ if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
+ (dwc->num_usb3_ports > DWC3_MAX_PORTS))

WARNING:TABSTOP: Statements should start on a tabstop
#490: FILE: drivers/usb/dwc3/core.c:2302:
+ for (i = 0; i < dwc->num_usb2_ports; i++) {

total: 0 errors, 5 warnings, 3 checks, 492 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
mechanically convert to the typical style using --fix or --fix-inplace.

"[PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support" has style problems, please review.


BR,
Thinh

2023-10-18 11:57:57

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280



On 10/12/2023 10:32 PM, Krishna Kurapati PSSNV wrote:
>
>
> On 10/12/2023 10:10 PM, Konrad Dybcio wrote:
>>
>>
>> On 10/7/23 17:48, Krishna Kurapati wrote:
>>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>>> platforms.
>>>
>>> Signed-off-by: Krishna Kurapati <[email protected]>
>>> ---
>> [...]
>>
>>> +
>>> +            interconnects = <&aggre1_noc MASTER_USB3_MP 0 &mc_virt
>>> SLAVE_EBI1 0>,
>>> +                    <&gem_noc MASTER_APPSS_PROC 0 &config_noc
>>> SLAVE_USB3_MP 0>;
>> Please use QCOM_ICC_TAG_ALWAYS from
>> include/dt-bindings/interconnect/qcom,icc.h (like in sa8775p)
>>
>> With that I think it's good to go :)
>>
> Hi Konrad. Thanks for the review.
>
> I see that the tags are used fr spi/i2c but not usb. So to maintain
> uniformity, wanted to keep the same here.
>

Hi Konrad,

Even in sa8775p.dtsi, the interconnect nodes have 0 & 1 instead of
macros. So wouldn't it be disturbing the uniformity if we use ICC_TAG's
here. Let me know your thoughts on this.

Regards,
Krishna,

2023-10-20 08:32:13

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 01/10] usb: dwc3: core: Access XHCI address space temporarily to read port info

On Sat, Oct 07, 2023 at 09:17:57PM +0530, Krishna Kurapati wrote:
> Currently host-only capable DWC3 controllers support Multiport.

You use the word "currently" in a few places like this (e.g. in comments
in the code). What exactly do you mean? That all current multiport
controllers are host-only, or that this is all that the driver supports
after your changes?

Please rephrase accordingly throughout so that this becomes clear.

In any case it looks like the above sentence is at least missing an
"only".

> +static int dwc3_read_port_info(struct dwc3 *dwc)
> +{
> + void __iomem *base;
> + u8 major_revision;
> + u32 offset = 0;

I'd move the initialisation just before the loop.

> + u32 val;
> +
> + /*
> + * Remap xHCI address space to access XHCI ext cap regs,

Drop comma and merge with next line and break it closer to 80 chars
(instead of 65).

> + * since it is needed to get port info.

s/since it is needed to get/which hold the/?

> + */
> + base = ioremap(dwc->xhci_resources[0].start,
> + resource_size(&dwc->xhci_resources[0]));
> + if (IS_ERR(base))
> + return PTR_ERR(base);
> +
> + do {
> + offset = xhci_find_next_ext_cap(base, offset,
> + XHCI_EXT_CAPS_PROTOCOL);
> + if (!offset)
> + break;
> +
> + val = readl(base + offset);
> + major_revision = XHCI_EXT_PORT_MAJOR(val);
> +
> + val = readl(base + offset + 0x08);
> + if (major_revision == 0x03) {
> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val);
> + } else if (major_revision <= 0x02) {
> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val);
> + } else {
> + dev_err(dwc->dev,

This should be dev_warn() (as in the xhci driver) now that you no longer
treat it as a fatal error.

> + "Unrecognized port major revision %d\n",

Merge this with the previous line (even if it makes that line 83 chars).

Use a lower case 'U' for consistency with most of the error messages.

> + major_revision);
> + }
> + } while (1);
> +
> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> + dwc->num_usb2_ports, dwc->num_usb3_ports);
> +
> + iounmap(base);
> +
> + return 0;
> +}
> +
> static int dwc3_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> @@ -1846,6 +1892,7 @@ static int dwc3_probe(struct platform_device *pdev)
> void __iomem *regs;
> struct dwc3 *dwc;
> int ret;
> + unsigned int hw_mode;

Nit: I'd place this one before ret.

>
> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
> if (!dwc)
> @@ -1926,6 +1973,20 @@ static int dwc3_probe(struct platform_device *pdev)
> goto err_disable_clks;
> }
>
> + /*
> + * Currently only DWC3 controllers that are host-only capable
> + * support Multiport.
> + */

So is this is a limitation of the hardware or implementation?

> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
> + ret = dwc3_read_port_info(dwc);
> + if (ret)
> + goto err_disable_clks;
> + } else {
> + dwc->num_usb2_ports = 1;
> + dwc->num_usb3_ports = 1;
> + }
> +
> spin_lock_init(&dwc->lock);
> mutex_init(&dwc->mutex);

Johan

2023-10-20 08:38:30

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 02/10] usb: dwc3: core: Skip setting event buffers for host only controllers

On Sat, Oct 07, 2023 at 09:17:58PM +0530, Krishna Kurapati wrote:
> On some SoC's like SA8295P where the tertiary controller is host-only
> capable, GEVTADDRHI/LO, GEVTSIZ, GEVTCOUNT registers are not accessible.
> Trying to access them leads to a crash.
>
> For DRD/Peripheral supported controllers, event buffer setup is done
> again in gadget_pullup. Skip setup or cleanup of event buffers if
> controller is host-only capable.
>
> Suggested-by: Johan Hovold <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>
> Acked-by: Thinh Nguyen <[email protected]>

Reviewed-by: Johan Hovold <[email protected]>

2023-10-20 09:43:30

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 01/10] usb: dwc3: core: Access XHCI address space temporarily to read port info



On 10/20/2023 2:02 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:17:57PM +0530, Krishna Kurapati wrote:
>> Currently host-only capable DWC3 controllers support Multiport.
>
> You use the word "currently" in a few places like this (e.g. in comments
> in the code). What exactly do you mean? That all current multiport
> controllers are host-only, or that this is all that the driver supports
> after your changes?
>
This means that, today the capable multiport controllers are host-only
capable, not that the driver is designed that way.

> Please rephrase accordingly throughout so that this becomes clear.
>
> In any case it looks like the above sentence is at least missing an
> "only".
>
>> +static int dwc3_read_port_info(struct dwc3 *dwc)
>> +{
>> + void __iomem *base;
>> + u8 major_revision;
>> + u32 offset = 0;
>
> I'd move the initialisation just before the loop.
>
>> + u32 val;
>> +
>> + /*
>> + * Remap xHCI address space to access XHCI ext cap regs,
>
> Drop comma and merge with next line and break it closer to 80 chars
> (instead of 65).
>
>> + * since it is needed to get port info.
>
> s/since it is needed to get/which hold the/?
>
>> + */
>> + base = ioremap(dwc->xhci_resources[0].start,
>> + resource_size(&dwc->xhci_resources[0]));
>> + if (IS_ERR(base))
>> + return PTR_ERR(base);
>> +
>> + do {
>> + offset = xhci_find_next_ext_cap(base, offset,
>> + XHCI_EXT_CAPS_PROTOCOL);
>> + if (!offset)
>> + break;
>> +
>> + val = readl(base + offset);
>> + major_revision = XHCI_EXT_PORT_MAJOR(val);
>> +
>> + val = readl(base + offset + 0x08);
>> + if (major_revision == 0x03) {
>> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val);
>> + } else if (major_revision <= 0x02) {
>> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val);
>> + } else {
>> + dev_err(dwc->dev,
>
> This should be dev_warn() (as in the xhci driver) now that you no longer
> treat it as a fatal error.
>
>> + "Unrecognized port major revision %d\n",
>
> Merge this with the previous line (even if it makes that line 83 chars).
>
> Use a lower case 'U' for consistency with most of the error messages.
>
Sure, will change this to dev_warn and modify the "u".

>> + major_revision);
>> + }
>> + } while (1);
>> +
>> + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
>> + dwc->num_usb2_ports, dwc->num_usb3_ports);
>> +
>> + iounmap(base);
>> +
>> + return 0;
>> +}
>> +
>> static int dwc3_probe(struct platform_device *pdev)
>> {
>> struct device *dev = &pdev->dev;
>> @@ -1846,6 +1892,7 @@ static int dwc3_probe(struct platform_device *pdev)
>> void __iomem *regs;
>> struct dwc3 *dwc;
>> int ret;
>> + unsigned int hw_mode;
>
> Nit: I'd place this one before ret.
> >>
>> dwc = devm_kzalloc(dev, sizeof(*dwc), GFP_KERNEL);
>> if (!dwc)
>> @@ -1926,6 +1973,20 @@ static int dwc3_probe(struct platform_device *pdev)
>> goto err_disable_clks;
>> }
>>
>> + /*
>> + * Currently only DWC3 controllers that are host-only capable
>> + * support Multiport.
>> + */
>
> So is this is a limitation of the hardware or implementation?
>

This is how the hardware is implemented today. I wanted to convey that
"lets check for host-only condition before going for reading port info"

>> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
>> + ret = dwc3_read_port_info(dwc);
>> + if (ret)
>> + goto err_disable_clks;
>> + } else {
>> + dwc->num_usb2_ports = 1;
>> + dwc->num_usb3_ports = 1;
>> + }
>> +
>> spin_lock_init(&dwc->lock);
>> mutex_init(&dwc->mutex);
>
> Johan
>

2023-10-20 09:58:17

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
> From: Harsh Agarwal <[email protected]>
>
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.

Should that not be "at least one HS PHY and at most one SS PHY"?

> But the DWC3 USB controller can be connected to multiple ports and
> each port can have their own PHYs. Each port of the multiport
> controller can either be HS+SS capable or HS only capable
> Proper quantification of them is required to modify GUSB2PHYCFG
> and GUSB3PIPECTL registers appropriately.
>
> Add support for detecting, obtaining and configuring phy's supported

"PHYs" for consistency, no apostrophe

> by a multiport controller and. Limit the max number of ports

"and." what? Looks like part of the sentence is missing? Or just drop
" and"?

> supported to 4 as only SC8280 which is a quad port controller supports

s/4/four/

Just change this to

Limit support to multiport controllers with up to four ports for
now (e.g. as needed for SC8280XP).

> Multiport currently.

You use capitalised "Multiport" in several places it seems. Is this an
established term for these controllers or should it just be "multiport"
or "multiple ports"?

> Reported-by: kernel test robot <[email protected]>
> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/

Drop these two lines, as people have already suggested.

> Co-developed-by: Harsh Agarwal <[email protected]>
> Signed-off-by: Harsh Agarwal <[email protected]>
> Co-developed-by:Krishna Kurapati <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>

Thinh pointed out the problems with the above which were also reported
by checkpatch.pl.

> ---
> Changes in v13:
> Compiler issues found by kernel test robot have been fixed and tags added.
> So removing maintainers reviewed-by tag as we have made a minor change
> in the patch.

In general this is the right thing to do when the change in question was
non-trivial. I'm not sure that's the case here, but the robots tend to
complain about smaller (but sometimes important) things.

> @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> static int dwc3_phy_init(struct dwc3 *dwc)
> {
> int ret;
> + int i;
> + int j;

These could be declared on one line (same throughout).

> usb_phy_init(dwc->usb2_phy);
> usb_phy_init(dwc->usb3_phy);
>
> - ret = phy_init(dwc->usb2_generic_phy);
> - if (ret < 0)
> - goto err_shutdown_usb3_phy;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + ret = phy_init(dwc->usb2_generic_phy[i]);
> + if (ret < 0)
> + goto err_exit_phy;
>
> - ret = phy_init(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_exit_usb2_phy;
> + ret = phy_init(dwc->usb3_generic_phy[i]);
> + if (ret < 0) {
> + phy_exit(dwc->usb2_generic_phy[i]);
> + goto err_exit_phy;
> + }
> + }
>
> return 0;
>
> -err_exit_usb2_phy:
> - phy_exit(dwc->usb2_generic_phy);
> -err_shutdown_usb3_phy:
> +err_exit_phy:
> + for (j = i - 1; j >= 0; j--) {
> + phy_exit(dwc->usb2_generic_phy[j]);
> + phy_exit(dwc->usb3_generic_phy[j]);

Try to always unwind in reverse order so in this case move phy_exit()
for usb3 before usb2 here too.

> + }
> +
> usb_phy_shutdown(dwc->usb3_phy);
> usb_phy_shutdown(dwc->usb2_phy);

> @@ -783,23 +829,32 @@ static void dwc3_phy_exit(struct dwc3 *dwc)
> static int dwc3_phy_power_on(struct dwc3 *dwc)
> {
> int ret;
> + int i;
> + int j;
>
> usb_phy_set_suspend(dwc->usb2_phy, 0);
> usb_phy_set_suspend(dwc->usb3_phy, 0);
>
> - ret = phy_power_on(dwc->usb2_generic_phy);
> - if (ret < 0)
> - goto err_suspend_usb3_phy;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + ret = phy_power_on(dwc->usb2_generic_phy[i]);
> + if (ret < 0)
> + goto err_power_off_phy;
>
> - ret = phy_power_on(dwc->usb3_generic_phy);
> - if (ret < 0)
> - goto err_power_off_usb2_phy;
> + ret = phy_power_on(dwc->usb3_generic_phy[i]);
> + if (ret < 0) {
> + phy_power_off(dwc->usb2_generic_phy[i]);
> + goto err_power_off_phy;
> + }
> + }
>
> return 0;
>
> -err_power_off_usb2_phy:
> - phy_power_off(dwc->usb2_generic_phy);
> -err_suspend_usb3_phy:
> +err_power_off_phy:
> + for (j = i - 1; j >= 0; j--) {
> + phy_power_off(dwc->usb2_generic_phy[j]);
> + phy_power_off(dwc->usb3_generic_phy[j]);

Same here.

> + }
> +
> usb_phy_set_suspend(dwc->usb3_phy, 1);
> usb_phy_set_suspend(dwc->usb2_phy, 1);

> @@ -1346,7 +1410,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> {
> struct device *dev = dwc->dev;
> struct device_node *node = dev->of_node;
> + char phy_name[13];
> int ret;
> + int i;
>
> if (node) {
> dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
> @@ -1372,22 +1438,36 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
> return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> }
>
> - dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
> - if (IS_ERR(dwc->usb2_generic_phy)) {
> - ret = PTR_ERR(dwc->usb2_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb2_generic_phy = NULL;
> + for (i = 0; i < dwc->num_usb2_ports; i++) {
> + if (dwc->num_usb2_ports == 1)
> + sprintf(phy_name, "usb2-phy");
> else
> - return dev_err_probe(dev, ret, "no usb2 phy configured\n");
> - }
> + sprintf(phy_name, "usb2-port%d", i);
>
> - dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> - if (IS_ERR(dwc->usb3_generic_phy)) {
> - ret = PTR_ERR(dwc->usb3_generic_phy);
> - if (ret == -ENOSYS || ret == -ENODEV)
> - dwc->usb3_generic_phy = NULL;
> + dwc->usb2_generic_phy[i] = devm_phy_get(dev, phy_name);
> + if (IS_ERR(dwc->usb2_generic_phy[i])) {
> + ret = PTR_ERR(dwc->usb2_generic_phy[i]);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb2_generic_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret,
> + "failed to lookup phy %s\n", phy_name);

Continuation lines should be intented at least two tabs further.

I generally suggest adding brackets around blocks with multiline
statements to improve readability but if you move the string to the
previous line and intend the continuation line (phy_name) one tab more I
guess that's fine.

> + }
> +
> + if (dwc->num_usb2_ports == 1)
> + sprintf(phy_name, "usb3-phy");
> else
> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
> + sprintf(phy_name, "usb3-port%d", i);
> +
> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
> + if (IS_ERR(dwc->usb3_generic_phy[i])) {
> + ret = PTR_ERR(dwc->usb3_generic_phy[i]);
> + if (ret == -ENOSYS || ret == -ENODEV)
> + dwc->usb3_generic_phy[i] = NULL;
> + else
> + return dev_err_probe(dev, ret,
> + "failed to lookup phy %s\n", phy_name);

Same here.

> + }
> }
>
> return 0;

> @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
>
> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> dwc->num_usb2_ports, dwc->num_usb3_ports);
> -

Drop this random change.

> iounmap(base);
>
> + if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
> + (dwc->num_usb3_ports > DWC3_MAX_PORTS))

Again, continuation lines should be indented at least two tabs further.

> + return -ENOMEM;
> +
> return 0;
> }

> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2ea6df7e6571..fc5d15edab1c 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -33,6 +33,9 @@
>
> #include <linux/power_supply.h>
>
> +/* Number of ports supported by a multiport controller */

/*
* Maximum number of ports currently supported for multiport
* controllers.
*/

> +#define DWC3_MAX_PORTS 4
> +
> #define DWC3_MSG_MAX 500
>
> /* Global constants */
> @@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array {
> * @usb_psy: pointer to power supply interface.
> * @usb2_phy: pointer to USB2 PHY
> * @usb3_phy: pointer to USB3 PHY
> - * @usb2_generic_phy: pointer to USB2 PHY
> - * @usb3_generic_phy: pointer to USB3 PHY
> + * @usb2_generic_phy: pointer to array of USB2 PHY
> + * @usb3_generic_phy: pointer to array of USB3 PHY

s/PHY/PHYs/

> * @num_usb2_ports: number of USB2 ports
> * @num_usb3_ports: number of USB3 ports
> * @phys_ready: flag to indicate that PHYs are ready

Johan

2023-10-20 11:45:07

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller



On 10/20/2023 3:27 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
>> From: Harsh Agarwal <[email protected]>
>>
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
>
> Should that not be "at least one HS PHY and at most one SS PHY"?
>
>> But the DWC3 USB controller can be connected to multiple ports and
>> each port can have their own PHYs. Each port of the multiport
>> controller can either be HS+SS capable or HS only capable
>> Proper quantification of them is required to modify GUSB2PHYCFG
>> and GUSB3PIPECTL registers appropriately.
>>
>> Add support for detecting, obtaining and configuring phy's supported
>
> "PHYs" for consistency, no apostrophe
>
>> by a multiport controller and. Limit the max number of ports
>
> "and." what? Looks like part of the sentence is missing? Or just drop
> " and"?
>
>> supported to 4 as only SC8280 which is a quad port controller supports
>
> s/4/four/
>
> Just change this to
>
> Limit support to multiport controllers with up to four ports for
> now (e.g. as needed for SC8280XP).
>
>> Multiport currently.
>
> You use capitalised "Multiport" in several places it seems. Is this an
> established term for these controllers or should it just be "multiport"
> or "multiple ports"?
>
>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> Drop these two lines, as people have already suggested.
>
>> Co-developed-by: Harsh Agarwal <[email protected]>
>> Signed-off-by: Harsh Agarwal <[email protected]>
>> Co-developed-by:Krishna Kurapati <[email protected]>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>
> Thinh pointed out the problems with the above which were also reported
> by checkpatch.pl.
>

I see that removing Co-Developed by tag for Harsh is helping with one
checkpatch issue. From what I know, although I made Harsh the Primary
author for the patch, should we still mention his Co-Developed-by along
with this Signed-Of by ?

>> ---
>> Changes in v13:
>> Compiler issues found by kernel test robot have been fixed and tags added.
>> So removing maintainers reviewed-by tag as we have made a minor change
>> in the patch.
>
> In general this is the right thing to do when the change in question was
> non-trivial. I'm not sure that's the case here, but the robots tend to
> complain about smaller (but sometimes important) things.
>

I too had this uncertainity, but I see that we must not add maintainers
reviewed by tag if we even make one letter of change. Wanted to adhere
to these rules and so removed Thinh's tag and asked for re-review.

>> @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>> static int dwc3_phy_init(struct dwc3 *dwc)
>> {
>> int ret;
>> + int i;
>> + int j;
>
> These could be declared on one line (same throughout).
>

I did so in v7, but was asked to put them in separate lines:
https://lore.kernel.org/all/[email protected]/


>> usb_phy_init(dwc->usb2_phy);
>> usb_phy_init(dwc->usb3_phy);

dwc->usb2_generic_phy[i] = NULL;
>> + else
>> + return dev_err_probe(dev, ret,
>> + "failed to lookup phy %s\n", phy_name);
>
> Continuation lines should be intented at least two tabs further.
>
> I generally suggest adding brackets around blocks with multiline
> statements to improve readability but if you move the string to the
> previous line and intend the continuation line (phy_name) one tab more I
> guess that's fine.
>
>> + }
>> +
>> + if (dwc->num_usb2_ports == 1)
>> + sprintf(phy_name, "usb3-phy");
>> else
>> - return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> + sprintf(phy_name, "usb3-port%d", i);
>> +
>> + dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
>> + if (IS_ERR(dwc->usb3_generic_phy[i])) {
>> + ret = PTR_ERR(dwc->usb3_generic_phy[i]);
>> + if (ret == -ENOSYS || ret == -ENODEV)
>> + dwc->usb3_generic_phy[i] = NULL;
>> + else
>> + return dev_err_probe(dev, ret,
>> + "failed to lookup phy %s\n", phy_name);
>
> Same here.
>
>> + }
>> }
>>
>> return 0;
>
>> @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
>>
>> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
>> dwc->num_usb2_ports, dwc->num_usb3_ports);
>> -
>
> Drop this random change.

The removal of extra line here done you mean ?

>
>> iounmap(base);
>>
>> + if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
>> + (dwc->num_usb3_ports > DWC3_MAX_PORTS))
>
> Again, continuation lines should be indented at least two tabs further.
>
>> + return -ENOMEM;
>> +
>> return 0;
>> }
>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2ea6df7e6571..fc5d15edab1c 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -33,6 +33,9 @@
>>
>> #include <linux/power_supply.h>
>>
>> +/* Number of ports supported by a multiport controller */
>
> /*
> * Maximum number of ports currently supported for multiport
> * controllers.
> */
>
>> +#define DWC3_MAX_PORTS 4
>> +
>> #define DWC3_MSG_MAX 500
>>
>> /* Global constants */
>> @@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array {
>> * @usb_psy: pointer to power supply interface.
>> * @usb2_phy: pointer to USB2 PHY
>> * @usb3_phy: pointer to USB3 PHY
>> - * @usb2_generic_phy: pointer to USB2 PHY
>> - * @usb3_generic_phy: pointer to USB3 PHY
>> + * @usb2_generic_phy: pointer to array of USB2 PHY
>> + * @usb3_generic_phy: pointer to array of USB3 PHY
>
> s/PHY/PHYs/
>
>> * @num_usb2_ports: number of USB2 ports
>> * @num_usb3_ports: number of USB3 ports
>> * @phys_ready: flag to indicate that PHYs are ready
>
> Johan

Thanks for the review. Will fix these nits in v14.

Regards,
Krishna,

2023-10-20 12:30:48

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 04/10] usb: dwc3: qcom: Add helper function to request threaded IRQ

On Sat, Oct 07, 2023 at 09:18:00PM +0530, Krishna Kurapati wrote:
> Cleanup setup irq call by implementing a new prep_irq helper function
> and using it to request threaded IRQ's.

Please replace this with:

Refactor interrupt setup by adding a new helper function for
requesting the wakeup interrupts.

and similarly for Subject ("wakeup interrupts").

> Signed-off-by: Krishna Kurapati <[email protected]>
> Reviewed-by: Bjorn Andersson <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 59 ++++++++++++++++--------------------
> 1 file changed, 26 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 3de43df6bbe8..ef2006db7601 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -535,6 +535,24 @@ static int dwc3_qcom_get_irq(struct platform_device *pdev,
> return ret;
> }
>
> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> + char *disp_name, int irq)

Please rename this one dwc3_qcom_request_irq() so that is obvious what
it does without having to look at the implementation.

This series eventually makes the driver only call this with irq_name ==
disp_name so just drop the latter and rename the parameter as "name" and
mention that in the commit message.

Also move irq before name and add the missing const. That is:

static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq, const char *name);

> +{
> + int ret;
> +
> + /* Keep wakeup interrupts disabled until suspend */
> + irq_set_status_flags(irq, IRQ_NOAUTOEN);
> + ret = devm_request_threaded_irq(qcom->dev, irq, NULL,
> + qcom_dwc3_resume_irq,
> + IRQF_TRIGGER_HIGH | IRQF_ONESHOT,
> + disp_name, qcom);
> +

Drop stray newline.

> + if (ret)
> + dev_err(qcom->dev, "%s failed: %d\n", irq_name, ret);

Please spell out

"failed to request irq %s: %d\n"

> +
> + return ret;
> +}

Johan

2023-10-20 13:23:46

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

First, drop "QCOM Glue driver" from Subject, you already have the "qcom"
prefix.

On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
> Refactor setup_irq call to facilitate reading multiport IRQ's along

"IRQs" or just "interrupts"

> with non mulitport ones. Read through the interrupt-names property

"multiport"

Please spell check all your patches (commit messages and code) before
posting, it's not the reviewers job.

> to figure out, the type of interrupt (DP/DM/HS/SS) and to which port
> it belongs. Also keep track of port index to calculate port count
> based on interrupts provided as input in DT.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++----------
> 1 file changed, 154 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index ef2006db7601..863892284146 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -53,14 +53,25 @@
> #define APPS_USB_AVG_BW 0
> #define APPS_USB_PEAK_BW MBps_to_icc(40)
>
> +#define NUM_PHY_IRQ 4
> +
> +enum dwc3_qcom_ph_index {

"phy_index"

> + DP_HS_PHY_IRQ_INDEX = 0,
> + DM_HS_PHY_IRQ_INDEX,
> + SS_PHY_IRQ_INDEX,
> + HS_PHY_IRQ_INDEX,
> +};
> +
> struct dwc3_acpi_pdata {
> u32 qscratch_base_offset;
> u32 qscratch_base_size;
> u32 dwc3_core_base_size;
> + /*
> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
> + * IRQ's respectively.
> + */
> + int phy_irq_index[NUM_PHY_IRQ - 1];
> int hs_phy_irq_index;
> - int dp_hs_phy_irq_index;
> - int dm_hs_phy_irq_index;
> - int ss_phy_irq_index;
> bool is_urs;
> };
>
> @@ -73,10 +84,12 @@ struct dwc3_qcom {
> int num_clocks;
> struct reset_control *resets;
>
> + /*
> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
> + * respectively.
> + */
> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
> int hs_phy_irq;
> - int dp_hs_phy_irq;
> - int dm_hs_phy_irq;
> - int ss_phy_irq;

I'm not sure using arrays like this is a good idea (and haven't you
switched the indexes above?).

Why not add a port structure instead?

struct dwc3_qcom_port {
int hs_phy_irq;
int dp_hs_phy_irq;
int dm_hs_phy_irq;
int ss_phy_irq;
};

and then have

struct dwc3_qcom_port ports[DWC3_MAX_PORTS];

in dwc3_qcom. The port structure can the later also be amended with
whatever other additional per-port data there is need for.

This should make the implementation cleaner.

I also don't like the special handling of hs_phy_irq; if this is really
just another name for the pwr_event_irq then this should be cleaned up
before making the code more complicated than it needs to be.

Make sure to clarify this before posting a new revision.

> enum usb_device_speed usb2_speed;
>
> struct extcon_dev *edev;

> if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_disable_wakeup_irq(qcom->dm_hs_phy_irq);
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);

For example, this would become

dwc3_qcom_disable_wakeup_irq(qcom->ports[0].dm_hs_phy_irq);

which is much more readable.

> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> - char *disp_name, int irq)
> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
> + const char *disp_name, int irq)

Ok, here you did drop the second name parameter, but without renaming
the first and hidden in a long diff without any mention anywhere.

> +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index)
> +{
> + int port_index = -1;
> +
> + switch (irq_index) {
> + case DP_HS_PHY_IRQ_INDEX:
> + if (strcmp(irq_name, "dp_hs_phy_irq") == 0)
> + port_index = 1;
> + else
> + sscanf(irq_name, "dp_hs_phy_%d", &port_index);
> + break;
> +

No need for newlines after break.

> + case DM_HS_PHY_IRQ_INDEX:
> + if (strcmp(irq_name, "dm_hs_phy_irq") == 0)
> + port_index = 1;
> + else
> + sscanf(irq_name, "dm_hs_phy_%d", &port_index);
> + break;
> +
> + case SS_PHY_IRQ_INDEX:
> + if (strcmp(irq_name, "ss_phy_irq") == 0)
> + port_index = 1;
> + else
> + sscanf(irq_name, "ss_phy_%d", &port_index);
> + break;
> +
> + case HS_PHY_IRQ_INDEX:
> + port_index = 1;
> + break;
> + }
> +
> + if (port_index > DWC3_MAX_PORTS)
> + port_index = -1;
> +
> + return port_index;
> +}

> static int dwc3_qcom_setup_irq(struct platform_device *pdev)
> {
> struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
> - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
> + struct device_node *np = pdev->dev.of_node;
> + const char **irq_names;
> + int port_index;
> + int acpi_index;
> + int irq_count;
> + int irq_index;
> int irq;
> int ret;
> + int i;
>
> - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
> - pdata ? pdata->hs_phy_irq_index : -1);
> - if (irq > 0) {
> - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq);
> - if (ret)
> - return ret;
> - qcom->hs_phy_irq = irq;
> - }
> + irq_count = of_property_count_strings(np, "interrupt-names");

of_property_count_strings() can return negative errnos and you don't
have any sanity checks for the return value...

Please slow down, and also make sure to get your patches reviewed
internally before posting new revisions.

> + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);

devm_kcalloc()

> + if (!irq_names)
> + return -ENOMEM;
>
> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
> - pdata ? pdata->dp_hs_phy_irq_index : -1);
> - if (irq > 0) {
> - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq);
> - if (ret)
> - return ret;
> - qcom->dp_hs_phy_irq = irq;
> - }
> + ret = of_property_read_string_array(np, "interrupt-names",
> + irq_names, irq_count);

No sanity check here either?

> + for (i = 0; i < irq_count; i++) {
> + irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
> + if (irq_index == -1) {
> + dev_dbg(&pdev->dev, "Invalid IRQ not handled");
> + continue;
> + }

I'll just stop reviewing here. This is a waste of my time.

Johan

2023-10-22 18:04:49

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller



On 10/20/2023 3:27 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
>> From: Harsh Agarwal <[email protected]>
>>
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
>
> Should that not be "at least one HS PHY and at most one SS PHY"?
>

No, I think it would be appropriate to say "at least one phy (HS/SS)" as
even one phy is sufficient to get things working.

>> But the DWC3 USB controller can be connected to multiple ports and
>> each port can have their own PHYs. Each port of the multiport
>> controller can either be HS+SS capable or HS only capable
>> Proper quantification of them is required to modify GUSB2PHYCFG
>> and GUSB3PIPECTL registers appropriately.
>>
>> Add support for detecting, obtaining and configuring phy's supported
>
> "PHYs" for consistency, no apostrophe
>
>> by a multiport controller and. Limit the max number of ports
>
> "and." what? Looks like part of the sentence is missing? Or just drop
> " and"?
>
>> supported to 4 as only SC8280 which is a quad port controller supports
>
> s/4/four/
>
> Just change this to
>
> Limit support to multiport controllers with up to four ports for
> now (e.g. as needed for SC8280XP).
>
>> Multiport currently.
>
> You use capitalised "Multiport" in several places it seems. Is this an
> established term for these controllers or should it just be "multiport"
> or "multiple ports"?
>
This is an established term AFAIK. So I've been using it here like this.

>> Reported-by: kernel test robot <[email protected]>
>> Closes: https://lore.kernel.org/oe-kbuild-all/[email protected]/
>
> Drop these two lines, as people have already suggested.
>
ACK.

>> Co-developed-by: Harsh Agarwal <[email protected]>
>> Signed-off-by: Harsh Agarwal <[email protected]>
>> Co-developed-by:Krishna Kurapati <[email protected]>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>
> Thinh pointed out the problems with the above which were also reported
> by checkpatch.pl.
>
>> ---
>> Changes in v13:
>> Compiler issues found by kernel test robot have been fixed and tags added.
>> So removing maintainers reviewed-by tag as we have made a minor change
>> in the patch.
>
> In general this is the right thing to do when the change in question was
> non-trivial. I'm not sure that's the case here, but the robots tend to
> complain about smaller (but sometimes important) things.
>
Got it. Will be careful about such things next time.

Regards,
Krishna,

2023-10-22 18:42:32

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 10/20/2023 6:53 PM, Johan Hovold wrote:
> First, drop "QCOM Glue driver" from Subject, you already have the "qcom"
> prefix.
>
> On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
>> Refactor setup_irq call to facilitate reading multiport IRQ's along
>
> "IRQs" or just "interrupts"
>
>> with non mulitport ones. Read through the interrupt-names property
>
> "multiport"
>
> Please spell check all your patches (commit messages and code) before
> posting, it's not the reviewers job.
>
>> to figure out, the type of interrupt (DP/DM/HS/SS) and to which port
>> it belongs. Also keep track of port index to calculate port count
>> based on interrupts provided as input in DT.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 210 +++++++++++++++++++++++++----------
>> 1 file changed, 154 insertions(+), 56 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index ef2006db7601..863892284146 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -53,14 +53,25 @@
>> #define APPS_USB_AVG_BW 0
>> #define APPS_USB_PEAK_BW MBps_to_icc(40)
>>
>> +#define NUM_PHY_IRQ 4
>> +
>> +enum dwc3_qcom_ph_index {
>
> "phy_index"
>
>> + DP_HS_PHY_IRQ_INDEX = 0,
>> + DM_HS_PHY_IRQ_INDEX,
>> + SS_PHY_IRQ_INDEX,
>> + HS_PHY_IRQ_INDEX,
>> +};
>> +
>> struct dwc3_acpi_pdata {
>> u32 qscratch_base_offset;
>> u32 qscratch_base_size;
>> u32 dwc3_core_base_size;
>> + /*
>> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
>> + * IRQ's respectively.
>> + */
>> + int phy_irq_index[NUM_PHY_IRQ - 1];
>> int hs_phy_irq_index;
>> - int dp_hs_phy_irq_index;
>> - int dm_hs_phy_irq_index;
>> - int ss_phy_irq_index;
>> bool is_urs;
>> };
>>
>> @@ -73,10 +84,12 @@ struct dwc3_qcom {
>> int num_clocks;
>> struct reset_control *resets;
>>
>> + /*
>> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
>> + * respectively.
>> + */
>> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
>> int hs_phy_irq;
>> - int dp_hs_phy_irq;
>> - int dm_hs_phy_irq;
>> - int ss_phy_irq;
>
> I'm not sure using arrays like this is a good idea (and haven't you
> switched the indexes above?).
>
> Why not add a port structure instead?
>
> struct dwc3_qcom_port {
> int hs_phy_irq;
> int dp_hs_phy_irq;
> int dm_hs_phy_irq;
> int ss_phy_irq;
> };
>
> and then have
>
> struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
>
> in dwc3_qcom. The port structure can the later also be amended with
> whatever other additional per-port data there is need for.
>
> This should make the implementation cleaner.
>
> I also don't like the special handling of hs_phy_irq; if this is really
> just another name for the pwr_event_irq then this should be cleaned up
> before making the code more complicated than it needs to be.
>
> Make sure to clarify this before posting a new revision.
>

hs_phy_irq is different from pwr_event_irq.
AFAIK, there is only one of this per controller.

>> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
>> - char *disp_name, int irq)
>> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
>> + const char *disp_name, int irq)
>
> Ok, here you did drop the second name parameter, but without renaming
> the first and hidden in a long diff without any mention anywhere.
>
I didn't understand the comment. Can you please elaborate.
I didn't drop the second parameter. In the usage of this call, I passed
same value to both irq_name and disp_name:

"dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"

I mentioned in cover-letter that I am using same name as obtained from
DT to register the interrupts as well. Should've mentioned that in
commit text of this patch. Will do it in next version.

>> +static int dwc3_qcom_get_port_index(const char *irq_name, int irq_index)
>> +{
>> + int port_index = -1;
>> +
>> + switch (irq_index) {
>> + case DP_HS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "dp_hs_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "dp_hs_phy_%d", &port_index);
>> + break;
>> +
>
> No need for newlines after break.
>
>> + case DM_HS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "dm_hs_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "dm_hs_phy_%d", &port_index);
>> + break;
>> +
>> + case SS_PHY_IRQ_INDEX:
>> + if (strcmp(irq_name, "ss_phy_irq") == 0)
>> + port_index = 1;
>> + else
>> + sscanf(irq_name, "ss_phy_%d", &port_index);
>> + break;
>> +
>> + case HS_PHY_IRQ_INDEX:
>> + port_index = 1;
>> + break;
>> + }
>> +
>> + if (port_index > DWC3_MAX_PORTS)
>> + port_index = -1;
>> +
>> + return port_index;
>> +}
>
>> static int dwc3_qcom_setup_irq(struct platform_device *pdev)
>> {
>> struct dwc3_qcom *qcom = platform_get_drvdata(pdev);
>> - const struct dwc3_acpi_pdata *pdata = qcom->acpi_pdata;
>> + struct device_node *np = pdev->dev.of_node;
>> + const char **irq_names;
>> + int port_index;
>> + int acpi_index;
>> + int irq_count;
>> + int irq_index;
>> int irq;
>> int ret;
>> + int i;
>>
>> - irq = dwc3_qcom_get_irq(pdev, "hs_phy_irq",
>> - pdata ? pdata->hs_phy_irq_index : -1);
>> - if (irq > 0) {
>> - ret = dwc3_qcom_prep_irq(qcom, "hs_phy_irq", "qcom_dwc3 HS", irq);
>> - if (ret)
>> - return ret;
>> - qcom->hs_phy_irq = irq;
>> - }
>> + irq_count = of_property_count_strings(np, "interrupt-names");
>
> of_property_count_strings() can return negative errnos and you don't
> have any sanity checks for the return value...
>
> Please slow down, and also make sure to get your patches reviewed
> internally before posting new revisions.
>
It did go through internal review but probably went un-noticed. Will add
sanity checks here and below.

>> + irq_names = devm_kzalloc(&pdev->dev, sizeof(*irq_names) * irq_count, GFP_KERNEL);
>
> devm_kcalloc()
>
>> + if (!irq_names)
>> + return -ENOMEM;
>>
>> - irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
>> - pdata ? pdata->dp_hs_phy_irq_index : -1);
>> - if (irq > 0) {
>> - ret = dwc3_qcom_prep_irq(qcom, "dp_hs_phy_irq", "qcom_dwc3 DP_HS", irq);
>> - if (ret)
>> - return ret;
>> - qcom->dp_hs_phy_irq = irq;
>> - }
>> + ret = of_property_read_string_array(np, "interrupt-names",
>> + irq_names, irq_count);
>
> No sanity check here either?
>
>> + for (i = 0; i < irq_count; i++) {
>> + irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
>> + if (irq_index == -1) {
>> + dev_dbg(&pdev->dev, "Invalid IRQ not handled");
>> + continue;
>> + }
>
> I'll just stop reviewing here. This is a waste of my time.
>

Regards,
Krishna,

2023-10-23 08:44:13

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 01/10] usb: dwc3: core: Access XHCI address space temporarily to read port info

On Fri, Oct 20, 2023 at 03:12:44PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/20/2023 2:02 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:17:57PM +0530, Krishna Kurapati wrote:
> >> Currently host-only capable DWC3 controllers support Multiport.
> >
> > You use the word "currently" in a few places like this (e.g. in comments
> > in the code). What exactly do you mean? That all current multiport
> > controllers are host-only, or that this is all that the driver supports
> > after your changes?
> >
> This means that, today the capable multiport controllers are host-only
> capable, not that the driver is designed that way.

Ok.

> > Please rephrase accordingly throughout so that this becomes clear.

Johan

2023-10-23 08:51:54

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

On Fri, Oct 20, 2023 at 05:11:46PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/20/2023 3:27 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
> >> From: Harsh Agarwal <[email protected]>

> >> Co-developed-by: Harsh Agarwal <[email protected]>
> >> Signed-off-by: Harsh Agarwal <[email protected]>
> >> Co-developed-by:Krishna Kurapati <[email protected]>
> >> Signed-off-by: Krishna Kurapati <[email protected]>
> >
> > Thinh pointed out the problems with the above which were also reported
> > by checkpatch.pl.
>
> I see that removing Co-Developed by tag for Harsh is helping with one
> checkpatch issue. From what I know, although I made Harsh the Primary
> author for the patch, should we still mention his Co-Developed-by along
> with this Signed-Of by ?

This is all documented in the process document I pointed you at earlier.

You don't need a Co-Developed-by tag for the primary author.

> >> @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
> >> static int dwc3_phy_init(struct dwc3 *dwc)
> >> {
> >> int ret;
> >> + int i;
> >> + int j;
> >
> > These could be declared on one line (same throughout).
> >
>
> I did so in v7, but was asked to put them in separate lines:
> https://lore.kernel.org/all/[email protected]/

Ok, either is fine if Thinh prefers it this way.

The problem is usually the other way round where people try to squeeze
in too much (e.g. unrelated variables and declarations) on the same
line.

> >> @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
> >>
> >> dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
> >> dwc->num_usb2_ports, dwc->num_usb3_ports);
> >> -
> >
> > Drop this random change.
>
> The removal of extra line here done you mean ?

Yes.

> >
> >> iounmap(base);

Johan

2023-10-23 09:12:12

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

On Sun, Oct 22, 2023 at 11:33:52PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/20/2023 3:27 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
> >> From: Harsh Agarwal <[email protected]>
> >>
> >> Currently the DWC3 driver supports only single port controller
> >> which requires at most one HS and one SS PHY.
> >
> > Should that not be "at least one HS PHY and at most one SS PHY"?
> >
>
> No, I think it would be appropriate to say "at least one phy (HS/SS)" as
> even one phy is sufficient to get things working.

No, that would be a violation if the binding (even if the driver may not
currently enforce it for generic phys) as well as the USB spec.

Also note that your implementation (i.e. this very patch) assumes that
num_usb2_ports >= num_usb3_ports.

> >> Add support for detecting, obtaining and configuring phy's supported
> >
> > "PHYs" for consistency, no apostrophe
> >
> >> by a multiport controller and. Limit the max number of ports
> >
> > "and." what? Looks like part of the sentence is missing? Or just drop
> > " and"?
> >
> >> supported to 4 as only SC8280 which is a quad port controller supports
> >
> > s/4/four/
> >
> > Just change this to
> >
> > Limit support to multiport controllers with up to four ports for
> > now (e.g. as needed for SC8280XP).
> >
> >> Multiport currently.
> >
> > You use capitalised "Multiport" in several places it seems. Is this an
> > established term for these controllers or should it just be "multiport"
> > or "multiple ports"?
> >
> This is an established term AFAIK. So I've been using it here like this.

Do you have a pointer? A google search seems to mostly come up with
links to this patch series.

Johan

2023-10-23 09:22:17

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
> On 10/20/2023 6:53 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:

> >> +#define NUM_PHY_IRQ 4
> >> +
> >> +enum dwc3_qcom_ph_index {
> >
> > "phy_index"
> >
> >> + DP_HS_PHY_IRQ_INDEX = 0,
> >> + DM_HS_PHY_IRQ_INDEX,
> >> + SS_PHY_IRQ_INDEX,
> >> + HS_PHY_IRQ_INDEX,
> >> +};
> >> +
> >> struct dwc3_acpi_pdata {
> >> u32 qscratch_base_offset;
> >> u32 qscratch_base_size;
> >> u32 dwc3_core_base_size;
> >> + /*
> >> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
> >> + * IRQ's respectively.
> >> + */
> >> + int phy_irq_index[NUM_PHY_IRQ - 1];
> >> int hs_phy_irq_index;
> >> - int dp_hs_phy_irq_index;
> >> - int dm_hs_phy_irq_index;
> >> - int ss_phy_irq_index;
> >> bool is_urs;
> >> };
> >>
> >> @@ -73,10 +84,12 @@ struct dwc3_qcom {
> >> int num_clocks;
> >> struct reset_control *resets;
> >>
> >> + /*
> >> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
> >> + * respectively.
> >> + */
> >> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
> >> int hs_phy_irq;
> >> - int dp_hs_phy_irq;
> >> - int dm_hs_phy_irq;
> >> - int ss_phy_irq;
> >
> > I'm not sure using arrays like this is a good idea (and haven't you
> > switched the indexes above?).
> >
> > Why not add a port structure instead?
> >
> > struct dwc3_qcom_port {
> > int hs_phy_irq;
> > int dp_hs_phy_irq;
> > int dm_hs_phy_irq;
> > int ss_phy_irq;
> > };
> >
> > and then have
> >
> > struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
> >
> > in dwc3_qcom. The port structure can the later also be amended with
> > whatever other additional per-port data there is need for.
> >
> > This should make the implementation cleaner.
> >
> > I also don't like the special handling of hs_phy_irq; if this is really
> > just another name for the pwr_event_irq then this should be cleaned up
> > before making the code more complicated than it needs to be.
> >
> > Make sure to clarify this before posting a new revision.
>
> hs_phy_irq is different from pwr_event_irq.

How is it different and how are they used?

> AFAIK, there is only one of this per controller.

But previous controllers were all single port so this interrupt is
likely also per-port, even if your comment below seems to suggest even
SC8280XP has one, which is unexpected (and not described in the updated
binding):

Yes, all targets have the same IRQ's. Just that MP one's have
multiple IRQ's of each type. But hs-phy_irq is only one in
SC8280 as well.

https://lore.kernel.org/lkml/[email protected]/

Please clarify.

> >> -static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, char *irq_name,
> >> - char *disp_name, int irq)
> >> +static int dwc3_qcom_prep_irq(struct dwc3_qcom *qcom, const char *irq_name,
> >> + const char *disp_name, int irq)
> >
> > Ok, here you did drop the second name parameter, but without renaming
> > the first and hidden in a long diff without any mention anywhere.
> >
> I didn't understand the comment. Can you please elaborate.
> I didn't drop the second parameter. In the usage of this call, I passed
> same value to both irq_name and disp_name:
>
> "dwc3_qcom_prep_irq(qcom, irq_names[i], irq_names[i], irq);"
>
> I mentioned in cover-letter that I am using same name as obtained from
> DT to register the interrupts as well. Should've mentioned that in
> commit text of this patch. Will do it in next version.

Ah, sorry I misread the diff. You never drop the second name even though
it is now redundant as I pointed on in a comment to one of the earlier
patches.

Johan

2023-10-23 11:25:11

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 10/23/2023 2:51 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/20/2023 6:53 PM, Johan Hovold wrote:
>>> On Sat, Oct 07, 2023 at 09:18:01PM +0530, Krishna Kurapati wrote:
>
>>>> +#define NUM_PHY_IRQ 4
>>>> +
>>>> +enum dwc3_qcom_ph_index {
>>>
>>> "phy_index"
>>>
>>>> + DP_HS_PHY_IRQ_INDEX = 0,
>>>> + DM_HS_PHY_IRQ_INDEX,
>>>> + SS_PHY_IRQ_INDEX,
>>>> + HS_PHY_IRQ_INDEX,
>>>> +};
>>>> +
>>>> struct dwc3_acpi_pdata {
>>>> u32 qscratch_base_offset;
>>>> u32 qscratch_base_size;
>>>> u32 dwc3_core_base_size;
>>>> + /*
>>>> + * The phy_irq_index corresponds to ACPI indexes of (in order) DP/DM/SS
>>>> + * IRQ's respectively.
>>>> + */
>>>> + int phy_irq_index[NUM_PHY_IRQ - 1];
>>>> int hs_phy_irq_index;
>>>> - int dp_hs_phy_irq_index;
>>>> - int dm_hs_phy_irq_index;
>>>> - int ss_phy_irq_index;
>>>> bool is_urs;
>>>> };
>>>>
>>>> @@ -73,10 +84,12 @@ struct dwc3_qcom {
>>>> int num_clocks;
>>>> struct reset_control *resets;
>>>>
>>>> + /*
>>>> + * The phy_irq corresponds to IRQ's registered for (in order) DP/DM/SS
>>>> + * respectively.
>>>> + */
>>>> + int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
>>>> int hs_phy_irq;
>>>> - int dp_hs_phy_irq;
>>>> - int dm_hs_phy_irq;
>>>> - int ss_phy_irq;
>>>
>>> I'm not sure using arrays like this is a good idea (and haven't you
>>> switched the indexes above?).
>>>
>>> Why not add a port structure instead?
>>>
>>> struct dwc3_qcom_port {
>>> int hs_phy_irq;
>>> int dp_hs_phy_irq;
>>> int dm_hs_phy_irq;
>>> int ss_phy_irq;
>>> };
>>>
>>> and then have
>>>
>>> struct dwc3_qcom_port ports[DWC3_MAX_PORTS];
>>>
>>> in dwc3_qcom. The port structure can the later also be amended with
>>> whatever other additional per-port data there is need for.
>>>
>>> This should make the implementation cleaner.
>>>
>>> I also don't like the special handling of hs_phy_irq; if this is really
>>> just another name for the pwr_event_irq then this should be cleaned up
>>> before making the code more complicated than it needs to be.
>>>
>>> Make sure to clarify this before posting a new revision.
>>
>> hs_phy_irq is different from pwr_event_irq.
>
> How is it different and how are they used?
>
>> AFAIK, there is only one of this per controller.
>
> But previous controllers were all single port so this interrupt is
> likely also per-port, even if your comment below seems to suggest even
> SC8280XP has one, which is unexpected (and not described in the updated
> binding):
>
> Yes, all targets have the same IRQ's. Just that MP one's have
> multiple IRQ's of each type. But hs-phy_irq is only one in
> SC8280 as well.
>
> https://lore.kernel.org/lkml/[email protected]/
>
> Please clarify.
>

For sure pwr_event_irq and hs_phy_irq are different. I assumed it was
per-controller and not per-phy because I took reference from software
code we have on downstream and hs_phy for multiport is not used
anywhere. I don't see any functionality implemented in downstream for
that IRQ. And it is only one for single port controllers.

But I got the following info from HW page and these are all the
interrupts (on apss processor) for multiport (extra details removed):

u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_0 SYS_apcsQgicSPI[130]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_1 SYS_apcsQgicSPI[135]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_3 SYS_apcsQgicSPI[856]
u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_2 SYS_apcsQgicSPI[857]

u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[0] SYS_apcsQgicSPI[133]
u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[1] SYS_apcsQgicSPI[134]
u_cm_usb3_uni_wrapper_mp0_usb3phy_debug_irq SYS_apcsQgicSPI[668]
u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0] SYS_apcsQgicSPI[830]
u_cm_usb3_uni_wrapper_mp1_usb3phy_debug_irq SYS_apcsQgicSPI[855]

u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_0 SYS_apcsQgicSPI[131]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_1 SYS_apcsQgicSPI[136]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_3 SYS_apcsQgicSPI[859]
u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_2 SYS_apcsQgicSPI[860]


u_cm_dwc_usb2_hs0_usb2_dpse apps_pdc_irq_out[127]
u_cm_dwc_usb2_hs0_usb2_dmse apps_pdc_irq_out[126]
u_cm_dwc_usb2_hs1_usb2_dpse apps_pdc_irq_out[129]
u_cm_dwc_usb2_hs1_usb2_dmse apps_pdc_irq_out[128]
u_cm_dwc_usb2_hs2_usb2_dpse apps_pdc_irq_out[131]
u_cm_dwc_usb2_hs2_usb2_dmse apps_pdc_irq_out[130]
u_cm_dwc_usb2_hs3_usb2_dpse apps_pdc_irq_out[133]
u_cm_dwc_usb2_hs3_usb2_dmse apps_pdc_irq_out[132]
u_cm_usb3_uni_wrapper_mp0_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[16]
u_cm_usb3_uni_wrapper_mp1_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[17]

Seems like there are 4 IRQ's for HS.

Regards,
Krishna,

2023-10-23 12:34:14

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller



On 10/23/2023 2:41 PM, Johan Hovold wrote:

>>>> Multiport currently.
>>>
>>> You use capitalised "Multiport" in several places it seems. Is this an
>>> established term for these controllers or should it just be "multiport"
>>> or "multiple ports"?
>>>
>> This is an established term AFAIK. So I've been using it here like this.
>
> Do you have a pointer? A google search seems to mostly come up with
> links to this patch series.

Only pointer I had is the hardware programming guide internally. It
mentioned "Multiport" as an established term. I think that is self
explanatory in usb context. Isn't it ?

Regards,
Krishna,

2023-10-23 14:07:14

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Mon, Oct 23, 2023 at 04:54:11PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 2:51 PM, Johan Hovold wrote:
> > On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
> >> On 10/20/2023 6:53 PM, Johan Hovold wrote:

> >>> I also don't like the special handling of hs_phy_irq; if this is really
> >>> just another name for the pwr_event_irq then this should be cleaned up
> >>> before making the code more complicated than it needs to be.
> >>>
> >>> Make sure to clarify this before posting a new revision.
> >>
> >> hs_phy_irq is different from pwr_event_irq.
> >
> > How is it different and how are they used?
> >
> >> AFAIK, there is only one of this per controller.
> >
> > But previous controllers were all single port so this interrupt is
> > likely also per-port, even if your comment below seems to suggest even
> > SC8280XP has one, which is unexpected (and not described in the updated
> > binding):
> >
> > Yes, all targets have the same IRQ's. Just that MP one's have
> > multiple IRQ's of each type. But hs-phy_irq is only one in
> > SC8280 as well.
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > Please clarify.
> >
>
> For sure pwr_event_irq and hs_phy_irq are different. I assumed it was
> per-controller and not per-phy because I took reference from software
> code we have on downstream and hs_phy for multiport is not used
> anywhere. I don't see any functionality implemented in downstream for
> that IRQ. And it is only one for single port controllers.
>
> But I got the following info from HW page and these are all the
> interrupts (on apss processor) for multiport (extra details removed):
>
> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_0 SYS_apcsQgicSPI[130]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_1 SYS_apcsQgicSPI[135]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_3 SYS_apcsQgicSPI[856]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_2 SYS_apcsQgicSPI[857]
>
> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[0] SYS_apcsQgicSPI[133]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[1] SYS_apcsQgicSPI[134]

This second core interrupt is also missing in the updated binding... It
is defined in the ACPI tables so presumably it is needed for the
multiport controller.

Do you have any more details on this one?

> u_cm_usb3_uni_wrapper_mp0_usb3phy_debug_irq SYS_apcsQgicSPI[668]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0] SYS_apcsQgicSPI[830]
> u_cm_usb3_uni_wrapper_mp1_usb3phy_debug_irq SYS_apcsQgicSPI[855]
>
> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_0 SYS_apcsQgicSPI[131]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_1 SYS_apcsQgicSPI[136]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_3 SYS_apcsQgicSPI[859]
> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_2 SYS_apcsQgicSPI[860]

Ok, so at least we know hs_phy_irq and pwr_event_irq are distinct and
both per-port.

The ACPI tables do not seem to include these, but yeah, that doesn't say
much more than that the Windows implementation doesn't currently use
them either.

> u_cm_dwc_usb2_hs0_usb2_dpse apps_pdc_irq_out[127]
> u_cm_dwc_usb2_hs0_usb2_dmse apps_pdc_irq_out[126]
> u_cm_dwc_usb2_hs1_usb2_dpse apps_pdc_irq_out[129]
> u_cm_dwc_usb2_hs1_usb2_dmse apps_pdc_irq_out[128]
> u_cm_dwc_usb2_hs2_usb2_dpse apps_pdc_irq_out[131]
> u_cm_dwc_usb2_hs2_usb2_dmse apps_pdc_irq_out[130]
> u_cm_dwc_usb2_hs3_usb2_dpse apps_pdc_irq_out[133]
> u_cm_dwc_usb2_hs3_usb2_dmse apps_pdc_irq_out[132]
> u_cm_usb3_uni_wrapper_mp0_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[16]
> u_cm_usb3_uni_wrapper_mp1_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[17]
>
> Seems like there are 4 IRQ's for HS.

Right. And I assume there are hs_phy_irqs also for the first two USB
controllers on sc8280xp?

Can you find out anything more about what hs_phy_irq is used for? It
appears to be an HS wakeup interrupt like the dp/dm ones, but there are
not really any details on how it is supposed to be used.

Johan

2023-10-23 14:11:04

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

On Mon, Oct 23, 2023 at 06:03:32PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 2:41 PM, Johan Hovold wrote:
>
> >>>> Multiport currently.
> >>>
> >>> You use capitalised "Multiport" in several places it seems. Is this an
> >>> established term for these controllers or should it just be "multiport"
> >>> or "multiple ports"?
> >>>
> >> This is an established term AFAIK. So I've been using it here like this.
> >
> > Do you have a pointer? A google search seems to mostly come up with
> > links to this patch series.
>
> Only pointer I had is the hardware programming guide internally. It
> mentioned "Multiport" as an established term. I think that is self
> explanatory in usb context. Isn't it ?

Self-explanatory, yes, just not sure whether capitalising it as
"Multiport" is warranted. But thanks for clarifying where this comes
from.

Johan

2023-10-23 15:47:21

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote:
> Currently wakeup is supported by only single port controllers. Read speed
> of each port and accordingly enable IRQ's for those ports.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++-----------------
> 1 file changed, 35 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 863892284146..651b9775a0c2 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -90,7 +90,7 @@ struct dwc3_qcom {
> */
> int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
> int hs_phy_irq;
> - enum usb_device_speed usb2_speed;
> + enum usb_device_speed usb2_speed[DWC3_MAX_PORTS];

This also belongs in a new port structure.

> struct extcon_dev *edev;
> struct extcon_dev *host_edev;
> @@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
> return dwc->xhci;
> }
>
> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom,
> + int port_index)

No need for line break (since it's a function definition).

> {
> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> struct usb_device *udev;
> @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>
> /*
> * It is possible to query the speed of all children of
> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
> - * currently supports only 1 port per controller. So
> - * this is sufficient.
> + * USB2.0 root hub via usb_hub_for_each_child().

This comment no longer makes sense with your current implementation.

But perhaps this should be done using usb_hub_for_each_child() instead
as that may be more efficient. Then you use this function to read out
the speed for all the ports in go (and store it in the port structures I
mentioned). Please determine which alternative is best.

> */
> #ifdef CONFIG_USB
> - udev = usb_hub_find_child(hcd->self.root_hub, 1);
> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
> #else
> udev = NULL;
> #endif
> @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
>
> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> {
> + int i;
> +
> dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
>
> - if (qcom->usb2_speed == USB_SPEED_LOW) {
> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> - (qcom->usb2_speed == USB_SPEED_FULL)) {
> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
> - } else {
> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
> - }
> + for (i = 0; i < qcom->num_ports; i++) {
> + if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
> + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
> + (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
> + } else {
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
> + }
>
> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]);
> + }
> }

The above is hardly readable, partly because of the 2d array that I
think you should drop, and partly because you add the port loop here
instead of in the caller.

A lot of these functions should become port operation where you either
pass in a port structure directly or possibly a port index as I've
mentioned before.

[ I realise that the confusion around hs_phy_irq may be partly to blame
for this but since that one is also a per-port interrupt, that's no
longer an issue. ]

> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> @@ -454,10 +461,8 @@ 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) {
> - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);

So just let this function update the usb2 speed for all ports unless
there are reasons not to.

> + if (dwc3_qcom_is_host(qcom) && wakeup)
> dwc3_qcom_enable_interrupts(qcom);

And then iterate over the ports and enable the interrupts here as you
did above for the pwr_evnt_irqs.

> - }
>
> qcom->is_suspended = true;

Johan

2023-10-23 15:58:49

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 07/10] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

On Sat, Oct 07, 2023 at 09:18:03PM +0530, Krishna Kurapati wrote:
> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
> for all the ports during suspend/resume.

No need to mention SA8295P as this is needed for all multiport
controllers.

Say something about adding support for multiport controllers generally
instead and mention what the power event irqs are used for.

> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> drivers/usb/dwc3/dwc3-qcom.c | 35 ++++++++++++++++++++++++++++-------
> 1 file changed, 28 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 651b9775a0c2..dbd4239e61c9 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -37,7 +37,11 @@
> #define PIPE3_PHYSTATUS_SW BIT(3)
> #define PIPE_UTMI_CLK_DIS BIT(8)
>
> -#define PWR_EVNT_IRQ_STAT_REG 0x58
> +#define PWR_EVNT_IRQ1_STAT_REG 0x58
> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc
> +#define PWR_EVNT_IRQ3_STAT_REG 0x228
> +#define PWR_EVNT_IRQ4_STAT_REG 0x238

Not sure these defines makes sense on their own. You now only use them
via the array below.

I think I already asked you whether these offsets depend on SoC and you
said no, right?

> +
> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4)
> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)
>
> @@ -107,6 +111,19 @@ struct dwc3_qcom {
> int num_ports;
> };
>
> +/*
> + * Currently non-multiport controller have only one PWR_EVENT_IRQ register,
> + * but multiport controllers like SA8295 contain upto 4 of them.
> + */

Please try not talk about "currently" and as things are likely to
change or, in fact, even *are* changing with your very patch series.

Again, this is not SA8295 specific.

> +#define NUM_PWR_EVENT_STAT_REGS 4

You already have MAX_PORTS, why are you defining a new define that will
always have to be equal to MAX_PORTS?

> +
> +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = {

missing const

> + PWR_EVNT_IRQ1_STAT_REG,
> + PWR_EVNT_IRQ2_STAT_REG,
> + PWR_EVNT_IRQ3_STAT_REG,
> + PWR_EVNT_IRQ4_STAT_REG,
> +};
> +
> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
> {
> u32 reg;
> @@ -446,9 +463,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> 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 < qcom->num_ports; i++) {
> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
> + dev_err(qcom->dev, "HS-PHY not in L2\n");

Error message should contain the port number.

> + }
>
> for (i = qcom->num_clocks - 1; i >= 0; i--)
> clk_disable_unprepare(qcom->clks[i]);
> @@ -494,9 +513,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
> dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>
> /* Clear existing events from PHY related to L2 in/out */
> - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
> - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
> -
> + for (i = 0; i < qcom->num_ports; i++) {
> + dwc3_qcom_setbits(qcom->qscratch_base,
> + pwr_evnt_irq_stat_reg_offset[i],
> + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);

Again, continuation lines should be indented at least two tabs further.

> + }
> qcom->is_suspended = false;
>
> return 0;

Johan

2023-10-23 16:09:43

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
> platforms.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
> 1 file changed, 84 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> index cad59af7ccef..5f64f75b07db 100644
> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
> };
>
> + usb_2: usb@a4f8800 {
> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";

So you went with a dedicated compatible even though you are now
inferring the number of ports from the interrupts property.

Should we drop that compatible again or is there any other reason to
keep a separate one?

> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 126 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 129 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 128 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 131 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 130 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 133 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 132 IRQ_TYPE_EDGE_RISING>,
> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
> + <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
> +
> + interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
> + "dp_hs_phy_2", "dm_hs_phy_2",
> + "dp_hs_phy_3", "dm_hs_phy_3",
> + "dp_hs_phy_4", "dm_hs_phy_4",
> + "ss_phy_1", "ss_phy_2",
> + "pwr_event_1",
> + "pwr_event_2",
> + "pwr_event_3",
> + "pwr_event_4";

The interrupt order does not match the binding, where the power event
interrupts come first.

And we probably also want the hs_phy_irqs here after fixing the
incomplete binding.

> + usb_2_dwc3: usb@a400000 {
> + compatible = "snps,dwc3";
> + reg = <0 0x0a400000 0 0xcd00>;
> + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;

I'd also like to know what that second dwc3 interrupt is for and whether
it should be defined here as well.

> + iommus = <&apps_smmu 0x800 0x0>;
> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> + <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> + <&usb_2_hsphy2>,
> + <&usb_2_hsphy3>;
> + phy-names = "usb2-port0", "usb3-port0",
> + "usb2-port1", "usb3-port1",
> + "usb2-port2",
> + "usb2-port3";
> +
> + /*
> + * Multiport controllers are host only contollers, so

spelling again...

> + * the dr_mode can be defaulted to host irrespective of
> + * the platform.
> + */

I know someone asked you to add a comment, but I think you should drop
it again because it makes little sense in its current form.

This particular controller is always going to be host only so just set
dr_mode here. No one is going to be overriding that.

Any comment would need to be about this particular platform and not make
claims about future controllers.

> + dr_mode = "host";
> + };
> + };
> +

Johan

2023-10-23 16:23:45

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

On Sat, Oct 07, 2023 at 09:18:05PM +0530, Krishna Kurapati wrote:
> Enable tertiary controller for SA8295P (based on SC8280XP).
> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>
> Signed-off-by: Krishna Kurapati <[email protected]>
> ---
> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
> 1 file changed, 49 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> index fd253942e5e5..271000163823 100644
> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
> @@ -9,6 +9,7 @@
> #include <dt-bindings/gpio/gpio.h>
> #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
> #include <dt-bindings/spmi/spmi.h>
> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>

Sort order ('p' < 'r').

> +&usb_2 {
> + pinctrl-0 = <&usb2_en_state>,
> + <&usb3_en_state>,
> + <&usb4_en_state>,
> + <&usb5_en_state>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +};
> +
> &usb_2_hsphy0 {
> vdda-pll-supply = <&vreg_l5a>;
> vdda18-supply = <&vreg_l7g>;
> @@ -729,3 +740,41 @@ wake-n-pins {
> };
> };
> };
> +
> +&pmm8540c_gpios {

Sort order here too ('p' < 't' in "&tlmm").

> + usb2_en_state: usb2-en-state {

No need to include '_state' in the labels.

Johan

2023-10-23 16:30:48

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 10/10] arm64: dts: qcom: sa8540-ride: Enable first port of tertiary usb controller

On Sat, Oct 07, 2023 at 09:18:06PM +0530, Krishna Kurapati wrote:
> From: Andrew Halaney <[email protected]>
>
> There is now support for the multiport USB controller this uses so
> enable it.
>
> The board only has a single port hooked up (despite it being wired up to
> the multiport IP on the SoC). There's also a USB 2.0 mux hooked up,
> which by default on boot is selected to mux properly. Grab the gpio
> controlling that and ensure it stays in the right position so USB 2.0
> continues to be routed from the external port to the SoC.
>
> Co-developed-by: Andrew Halaney <[email protected]>

Checkpatch complains about this one too since Andrew is the primary
author.

> Signed-off-by: Andrew Halaney <[email protected]>
> [Krishna: Rebased on top of usb-next]
> Co-developed-by: Krishna Kurapati <[email protected]>
> Signed-off-by: Krishna Kurapati <[email protected]>

How much co-development did you actually do here? Just rebasing and
submitting a patch is not enough to warrant shared authorship.

> ---
> arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 21 +++++++++++++++++++++
> 1 file changed, 21 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index b04f72ec097c..6904a4c201ff 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -503,6 +503,18 @@ &usb_2_qmpphy0 {
> status = "okay";
> };
>
> +&usb_2 {
> + pinctrl-0 = <&usb2_en_state>;
> + pinctrl-names = "default";
> +
> + status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> + phy-names = "usb2-port0", "usb3-port0";
> + phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};

Sort order and what Konrad said.

> +
> &xo_board_clk {
> clock-frequency = <38400000>;
> };
> @@ -655,4 +667,13 @@ wake-pins {
> bias-pull-up;
> };
> };
> +
> + usb2_en_state: usb2-en-state {

Drop "_state" from label.

> + /* TS3USB221A USB2.0 mux select */
> + pins = "gpio24";
> + function = "gpio";
> + drive-strength = <2>;
> + bias-disable;
> + output-low;
> + };
> };

Johan

2023-10-23 17:13:12

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 10/23/2023 7:37 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 04:54:11PM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/23/2023 2:51 PM, Johan Hovold wrote:
>>> On Mon, Oct 23, 2023 at 12:11:45AM +0530, Krishna Kurapati PSSNV wrote:
>>>> On 10/20/2023 6:53 PM, Johan Hovold wrote:
>
>>>>> I also don't like the special handling of hs_phy_irq; if this is really
>>>>> just another name for the pwr_event_irq then this should be cleaned up
>>>>> before making the code more complicated than it needs to be.
>>>>>
>>>>> Make sure to clarify this before posting a new revision.
>>>>
>>>> hs_phy_irq is different from pwr_event_irq.
>>>
>>> How is it different and how are they used?
>>>
>>>> AFAIK, there is only one of this per controller.
>>>
>>> But previous controllers were all single port so this interrupt is
>>> likely also per-port, even if your comment below seems to suggest even
>>> SC8280XP has one, which is unexpected (and not described in the updated
>>> binding):
>>>
>>> Yes, all targets have the same IRQ's. Just that MP one's have
>>> multiple IRQ's of each type. But hs-phy_irq is only one in
>>> SC8280 as well.
>>>
>>> https://lore.kernel.org/lkml/[email protected]/
>>>
>>> Please clarify.
>>>
>>
>> For sure pwr_event_irq and hs_phy_irq are different. I assumed it was
>> per-controller and not per-phy because I took reference from software
>> code we have on downstream and hs_phy for multiport is not used
>> anywhere. I don't see any functionality implemented in downstream for
>> that IRQ. And it is only one for single port controllers.
>>
>> But I got the following info from HW page and these are all the
>> interrupts (on apss processor) for multiport (extra details removed):
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_0 SYS_apcsQgicSPI[130]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_1 SYS_apcsQgicSPI[135]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_3 SYS_apcsQgicSPI[856]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_power_event_irq_2 SYS_apcsQgicSPI[857]
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[0] SYS_apcsQgicSPI[133]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_ctrl_irq[1] SYS_apcsQgicSPI[134]
>
> This second core interrupt is also missing in the updated binding... It
> is defined in the ACPI tables so presumably it is needed for the
> multiport controller.
>
> Do you have any more details on this one?
>
>> u_cm_usb3_uni_wrapper_mp0_usb3phy_debug_irq SYS_apcsQgicSPI[668]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0] SYS_apcsQgicSPI[830]
>> u_cm_usb3_uni_wrapper_mp1_usb3phy_debug_irq SYS_apcsQgicSPI[855]
>>
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_0 SYS_apcsQgicSPI[131]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_1 SYS_apcsQgicSPI[136]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_3 SYS_apcsQgicSPI[859]
>> u_usb31_scnd_mvs_pipe_wrapper_usb31_hs_phy_irq_2 SYS_apcsQgicSPI[860]
>
> Ok, so at least we know hs_phy_irq and pwr_event_irq are distinct and
> both per-port.
>
> The ACPI tables do not seem to include these, but yeah, that doesn't say
> much more than that the Windows implementation doesn't currently use
> them either.
>
>> u_cm_dwc_usb2_hs0_usb2_dpse apps_pdc_irq_out[127]
>> u_cm_dwc_usb2_hs0_usb2_dmse apps_pdc_irq_out[126]
>> u_cm_dwc_usb2_hs1_usb2_dpse apps_pdc_irq_out[129]
>> u_cm_dwc_usb2_hs1_usb2_dmse apps_pdc_irq_out[128]
>> u_cm_dwc_usb2_hs2_usb2_dpse apps_pdc_irq_out[131]
>> u_cm_dwc_usb2_hs2_usb2_dmse apps_pdc_irq_out[130]
>> u_cm_dwc_usb2_hs3_usb2_dpse apps_pdc_irq_out[133]
>> u_cm_dwc_usb2_hs3_usb2_dmse apps_pdc_irq_out[132]
>> u_cm_usb3_uni_wrapper_mp0_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[16]
>> u_cm_usb3_uni_wrapper_mp1_qmp_usb3_lfps_rxterm_irq apps_pdc_irq_out[17]
>>
>> Seems like there are 4 IRQ's for HS.
>
> Right. And I assume there are hs_phy_irqs also for the first two USB
> controllers on sc8280xp?

Hi Johan,

There are, I can dig through and find out. Atleast in downstream I don't
see any use of them.

>
> Can you find out anything more about what hs_phy_irq is used for? It
> appears to be an HS wakeup interrupt like the dp/dm ones, but there are
> not really any details on how it is supposed to be used.
>

This IRQ is really not used in downstream controllers. Not sure if its
a good idea to add driver code for that. I did some digging and I got
the reason why I first said that there is only one hs_phy_irq for
tertiary port of controller. The hardware programming sequence doesn't
specify usage of these 4 IRQ's but the hw specifics mention that there
are 4 of them. Adding driver support for these IRQ's is not a good idea
(atleast at this point because they are not used in downstream and I am
not sure what would be the side effect). For now I suggest we can add
them in bindings and DT and not handle the 4 hs_phy_irq's in the driver
code (meaning not add the hs_phy_irq to port structure we plan on adding
to dwc3_qcom).

Also I plan on splitting the patchset into 4 parts (essentially 4 diff
series):

1. Bindings update for hs_phy_irq's
2. DT patches for MP controller and platform specific files
3. Core driver update for supporting multiport
4. QCOM driver update for supporting wakeup/suspend/resume

This is in accordance to [1] and that way qcom code won't block core
driver changes from getting merged. Core driver changes are independent
and are sufficient to get multiport working.

[1]:
https://lore.kernel.org/all/[email protected]/

Regards,
Krishna,

2023-10-23 17:16:29

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

On 23/10/2023 18:09, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>> 1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index cad59af7ccef..5f64f75b07db 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> + usb_2: usb@a4f8800 {
>> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
>
> So you went with a dedicated compatible even though you are now
> inferring the number of ports from the interrupts property.
>
> Should we drop that compatible again or is there any other reason to
> keep a separate one?

Please keep the dedicated compatible even if currently it is not used.
https://elixir.bootlin.com/linux/v6.1-rc1/source/Documentation/devicetree/bindings/writing-bindings.rst#L42

Best regards,
Krzysztof

2023-10-23 17:23:15

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 07/10] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper



On 10/23/2023 9:28 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:03PM +0530, Krishna Kurapati wrote:
>> QCOM SoC SA8295P's tertiary quad port controller supports 2 HS+SS
>> ports and 2 HS only ports. Add support for configuring PWR_EVENT_IRQ's
>> for all the ports during suspend/resume.
>
> No need to mention SA8295P as this is needed for all multiport
> controllers.
> > Say something about adding support for multiport controllers generally
> instead and mention what the power event irqs are used for.
>

ACK.

>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 35 ++++++++++++++++++++++++++++-------
>> 1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 651b9775a0c2..dbd4239e61c9 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -37,7 +37,11 @@
>> #define PIPE3_PHYSTATUS_SW BIT(3)
>> #define PIPE_UTMI_CLK_DIS BIT(8)
>>
>> -#define PWR_EVNT_IRQ_STAT_REG 0x58
>> +#define PWR_EVNT_IRQ1_STAT_REG 0x58
>> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc
>> +#define PWR_EVNT_IRQ3_STAT_REG 0x228
>> +#define PWR_EVNT_IRQ4_STAT_REG 0x238
>
> Not sure these defines makes sense on their own. You now only use them
> via the array below.
>
> I think I already asked you whether these offsets depend on SoC and you
> said no, right?
>
There are only 3 QC SoC's today that support multiport.
The offsets mentioned here are for SC8280 based platforms.

For Sc8180 based platforms, these are the offsets:
USB3_MP_PWR_EVNT_IRQ_STAT 0xA4F8858
USB3_MP_PWR_EVNT_IRQ_1_STAT 0xA4F89DC

These would translate to 0x58 and 0x1DC

And for SX8380 the values are as follows:

USB3_MP_PWR_EVNT_IRQ_STAT 0xA4F8858
USB3_MP_PWR_EVNT_IRQ_1_STAT 0xA4F89DC

So here also, the offsets are same. 0x58 and 0x1DC.
So these are not SoC specific (atleast looking at the controllers
present). But there is no mathematical pattern to denote this as in the
following form (x + (port_num) * y). So made an array like this.

>> +
>> #define PWR_EVNT_LPM_IN_L2_MASK BIT(4)
>> #define PWR_EVNT_LPM_OUT_L2_MASK BIT(5)
>>
>> @@ -107,6 +111,19 @@ struct dwc3_qcom {
>> int num_ports;
>> };
>>
>> +/*
>> + * Currently non-multiport controller have only one PWR_EVENT_IRQ register,
>> + * but multiport controllers like SA8295 contain upto 4 of them.
>> + */
>
> Please try not talk about "currently" and as things are likely to
> change or, in fact, even *are* changing with your very patch series.
>
> Again, this is not SA8295 specific.
>
>> +#define NUM_PWR_EVENT_STAT_REGS 4
>
> You already have MAX_PORTS, why are you defining a new define that will
> always have to be equal to MAX_PORTS?
>

Do you recommend using the same max_ports ? If so, I can remove this
macro altogether.

>> +
>> +static u32 pwr_evnt_irq_stat_reg_offset[NUM_PWR_EVENT_STAT_REGS] = {
>
> missing const
>
>> + PWR_EVNT_IRQ1_STAT_REG,
>> + PWR_EVNT_IRQ2_STAT_REG,
>> + PWR_EVNT_IRQ3_STAT_REG,
>> + PWR_EVNT_IRQ4_STAT_REG,
>> +};
>> +
>> static inline void dwc3_qcom_setbits(void __iomem *base, u32 offset, u32 val)
>> {
>> u32 reg;
>> @@ -446,9 +463,11 @@ static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>> 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 < qcom->num_ports; i++) {
>> + val = readl(qcom->qscratch_base + pwr_evnt_irq_stat_reg_offset[i]);
>> + if (!(val & PWR_EVNT_LPM_IN_L2_MASK))
>> + dev_err(qcom->dev, "HS-PHY not in L2\n");
>
> Error message should contain the port number.
>

ACK

>> + }
>>
>> for (i = qcom->num_clocks - 1; i >= 0; i--)
>> clk_disable_unprepare(qcom->clks[i]);
>> @@ -494,9 +513,11 @@ static int dwc3_qcom_resume(struct dwc3_qcom *qcom, bool wakeup)
>> dev_warn(qcom->dev, "failed to enable interconnect: %d\n", ret);
>>
>> /* Clear existing events from PHY related to L2 in/out */
>> - dwc3_qcom_setbits(qcom->qscratch_base, PWR_EVNT_IRQ_STAT_REG,
>> - PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
>> -
>> + for (i = 0; i < qcom->num_ports; i++) {
>> + dwc3_qcom_setbits(qcom->qscratch_base,
>> + pwr_evnt_irq_stat_reg_offset[i],
>> + PWR_EVNT_LPM_IN_L2_MASK | PWR_EVNT_LPM_OUT_L2_MASK);
>
> Again, continuation lines should be indented at least two tabs further.
>

ACK.

Thanks for the review.

Regards,
Krishna,

2023-10-23 17:27:49

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport



On 10/23/2023 9:17 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote:
>> Currently wakeup is supported by only single port controllers. Read speed
>> of each port and accordingly enable IRQ's for those ports.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> drivers/usb/dwc3/dwc3-qcom.c | 65 +++++++++++++++++++-----------------
>> 1 file changed, 35 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
>> index 863892284146..651b9775a0c2 100644
>> --- a/drivers/usb/dwc3/dwc3-qcom.c
>> +++ b/drivers/usb/dwc3/dwc3-qcom.c
>> @@ -90,7 +90,7 @@ struct dwc3_qcom {
>> */
>> int phy_irq[NUM_PHY_IRQ - 1][DWC3_MAX_PORTS];
>> int hs_phy_irq;
>> - enum usb_device_speed usb2_speed;
>> + enum usb_device_speed usb2_speed[DWC3_MAX_PORTS];
>
> This also belongs in a new port structure.
>
>> struct extcon_dev *edev;
>> struct extcon_dev *host_edev;
>> @@ -335,7 +335,8 @@ static bool dwc3_qcom_is_host(struct dwc3_qcom *qcom)
>> return dwc->xhci;
>> }
>>
>> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom,
>> + int port_index)
>
> No need for line break (since it's a function definition).
>
>> {
>> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>> struct usb_device *udev;
>> @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>>
>> /*
>> * It is possible to query the speed of all children of
>> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
>> - * currently supports only 1 port per controller. So
>> - * this is sufficient.
>> + * USB2.0 root hub via usb_hub_for_each_child().
>
> This comment no longer makes sense with your current implementation.
>
Can you help elaborate on your comment ? Do you mean that this API
doesn't get speed on all ports, but this has to be called in a loop to
get all the port speeds ? In that sense, I agree, I can change the
comments here.

> But perhaps this should be done using usb_hub_for_each_child() instead
> as that may be more efficient. Then you use this function to read out
> the speed for all the ports in go (and store it in the port structures I
> mentioned). Please determine which alternative is best.
>
Either ways is fine. We would have qcom->num_ports to determine how many
speeds we can read.

>> */
>> #ifdef CONFIG_USB
>> - udev = usb_hub_find_child(hcd->self.root_hub, 1);
>> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
>> #else
>> udev = NULL;
>> #endif
>> @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
>>
>> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
>> {
>> + int i;
>> +
>> dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
>>
>> - if (qcom->usb2_speed == USB_SPEED_LOW) {
>> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
>> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
>> - (qcom->usb2_speed == USB_SPEED_FULL)) {
>> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
>> - } else {
>> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
>> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
>> - }
>> + for (i = 0; i < qcom->num_ports; i++) {
>> + if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
>> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
>> + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
>> + (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
>> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
>> + } else {
>> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
>> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
>> + }
>>
>> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
>> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]);
>> + }
>> }
>
> The above is hardly readable, partly because of the 2d array that I
> think you should drop, and partly because you add the port loop here
> instead of in the caller.
>
> A lot of these functions should become port operation where you either
> pass in a port structure directly or possibly a port index as I've
> mentioned before.

With your suggestion, yes, this can be refactored to be readable.

>
> [ I realise that the confusion around hs_phy_irq may be partly to blame
> for this but since that one is also a per-port interrupt, that's no
> longer an issue. ]

I don't want to add support for this right away [1]. I would like to
keep hs_phy_irq outside the loop for now.

>
>> static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
>> @@ -454,10 +461,8 @@ 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) {
>> - qcom->usb2_speed = dwc3_qcom_read_usb2_speed(qcom);
>
> So just let this function update the usb2 speed for all ports unless
> there are reasons not to.

Either way is fine by me as mentioned above. Will udapte code accordingly.

>
>> + if (dwc3_qcom_is_host(qcom) && wakeup)
>> dwc3_qcom_enable_interrupts(qcom);
>
> And then iterate over the ports and enable the interrupts here as you
> did above for the pwr_evnt_irqs.
>
>> - }
>>
>> qcom->is_suspended = true;
[1]:
https://lore.kernel.org/all/[email protected]/

Regards,
Krishna,

2023-10-23 17:35:09

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280



On 10/23/2023 9:39 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:
>> Add USB and DWC3 node for tertiary port of SC8280 along with multiport
>> IRQ's and phy's. This will be used as a base for SA8295P and SA8295-Ride
>> platforms.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sc8280xp.dtsi | 84 ++++++++++++++++++++++++++
>> 1 file changed, 84 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> index cad59af7ccef..5f64f75b07db 100644
>> --- a/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> +++ b/arch/arm64/boot/dts/qcom/sc8280xp.dtsi
>> @@ -3330,6 +3330,90 @@ system-cache-controller@9200000 {
>> interrupts = <GIC_SPI 582 IRQ_TYPE_LEVEL_HIGH>;
>> };
>>
>> + usb_2: usb@a4f8800 {
>> + compatible = "qcom,sc8280xp-dwc3-mp", "qcom,dwc3";
>
> So you went with a dedicated compatible even though you are now
> inferring the number of ports from the interrupts property.
>
> Should we drop that compatible again or is there any other reason to
> keep a separate one?
> >> + interrupts-extended = <&pdc 127 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 126 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 129 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 128 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 131 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 130 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 133 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 132 IRQ_TYPE_EDGE_RISING>,
>> + <&pdc 16 IRQ_TYPE_LEVEL_HIGH>,
>> + <&pdc 17 IRQ_TYPE_LEVEL_HIGH>,
>> + <&intc GIC_SPI 130 IRQ_TYPE_LEVEL_HIGH>,
>> + <&intc GIC_SPI 135 IRQ_TYPE_LEVEL_HIGH>,
>> + <&intc GIC_SPI 857 IRQ_TYPE_LEVEL_HIGH>,
>> + <&intc GIC_SPI 856 IRQ_TYPE_LEVEL_HIGH>;
>> +
>> + interrupt-names = "dp_hs_phy_1", "dm_hs_phy_1",
>> + "dp_hs_phy_2", "dm_hs_phy_2",
>> + "dp_hs_phy_3", "dm_hs_phy_3",
>> + "dp_hs_phy_4", "dm_hs_phy_4",
>> + "ss_phy_1", "ss_phy_2",
>> + "pwr_event_1",
>> + "pwr_event_2",
>> + "pwr_event_3",
>> + "pwr_event_4";
>
> The interrupt order does not match the binding, where the power event
> interrupts come first.
>
> And we probably also want the hs_phy_irqs here after fixing the
> incomplete binding.

You want to update the driver code for this as well ? I have no problem
in adding it in DT and binding but not in driver.

>
>> + usb_2_dwc3: usb@a400000 {
>> + compatible = "snps,dwc3";
>> + reg = <0 0x0a400000 0 0xcd00>;
>> + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
>
> I'd also like to know what that second dwc3 interrupt is for and whether
> it should be defined here as well.

Second interrupts is for HW acceleration I believe for which support is
not there currently.

Regards,
Krishna,

2023-10-23 17:43:21

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports



On 10/23/2023 9:53 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:18:05PM +0530, Krishna Kurapati wrote:
>> Enable tertiary controller for SA8295P (based on SC8280XP).
>> Add pinctrl support for usb ports to provide VBUS to connected peripherals.
>>
>> Signed-off-by: Krishna Kurapati <[email protected]>
>> ---
>> arch/arm64/boot/dts/qcom/sa8295p-adp.dts | 49 ++++++++++++++++++++++++
>> 1 file changed, 49 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> index fd253942e5e5..271000163823 100644
>> --- a/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> +++ b/arch/arm64/boot/dts/qcom/sa8295p-adp.dts
>> @@ -9,6 +9,7 @@
>> #include <dt-bindings/gpio/gpio.h>
>> #include <dt-bindings/regulator/qcom,rpmh-regulator.h>
>> #include <dt-bindings/spmi/spmi.h>
>> +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
>
> Sort order ('p' < 'r').

ACK

>
>> +&usb_2 {
>> + pinctrl-0 = <&usb2_en_state>,
>> + <&usb3_en_state>,
>> + <&usb4_en_state>,
>> + <&usb5_en_state>;
>> + pinctrl-names = "default";
>> +
>> + status = "okay";
>> +};
>> +
>> &usb_2_hsphy0 {
>> vdda-pll-supply = <&vreg_l5a>;
>> vdda18-supply = <&vreg_l7g>;
>> @@ -729,3 +740,41 @@ wake-n-pins {
>> };
>> };
>> };
>> +
>> +&pmm8540c_gpios {
>
> Sort order here too ('p' < 't' in "&tlmm").
>

ACK.

>> + usb2_en_state: usb2-en-state {
>
> No need to include '_state' in the labels.
>
Any specific reason ? I have no problem if removing the suffix but just
wanted to know the reason.

Regards,
Krishna,

2023-10-24 06:56:25

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 7:37 PM, Johan Hovold wrote:

> > Right. And I assume there are hs_phy_irqs also for the first two USB
> > controllers on sc8280xp?

> There are, I can dig through and find out. Atleast in downstream I don't
> see any use of them.

Yes, please do post how these are wired as well for completeness.

> > Can you find out anything more about what hs_phy_irq is used for? It
> > appears to be an HS wakeup interrupt like the dp/dm ones, but there are
> > not really any details on how it is supposed to be used.
>
> This IRQ is really not used in downstream controllers. Not sure if its
> a good idea to add driver code for that. I did some digging and I got
> the reason why I first said that there is only one hs_phy_irq for
> tertiary port of controller. The hardware programming sequence doesn't
> specify usage of these 4 IRQ's but the hw specifics mention that there
> are 4 of them. Adding driver support for these IRQ's is not a good idea
> (atleast at this point because they are not used in downstream and I am
> not sure what would be the side effect). For now I suggest we can add
> them in bindings and DT and not handle the 4 hs_phy_irq's in the driver
> code (meaning not add the hs_phy_irq to port structure we plan on adding
> to dwc3_qcom).

But there is already support for these interrupts in the driver. You
work for Qualcomm who built the thing so surely you can figure how they
intended these to be used?

You need to provide this information so that we can determine what the
binding should look like. The implementation would also be simplified if
we don't have to add random hacks to it just because we don't know why
the vendor driver you refer does not use it currently on this particular
platform.

> Also I plan on splitting the patchset into 4 parts (essentially 4 diff
> series):
>
> 1. Bindings update for hs_phy_irq's
> 2. DT patches for MP controller and platform specific files
> 3. Core driver update for supporting multiport
> 4. QCOM driver update for supporting wakeup/suspend/resume
>
> This is in accordance to [1] and that way qcom code won't block core
> driver changes from getting merged. Core driver changes are independent
> and are sufficient to get multiport working.

No, you clearly did not understand [1] at all. And stop trying to game
the upstreaming process. Bindings and driver patches go together. The
devicetree changes can be sent separately in case of USB but only
*after* the first set has been merged.

If the code had been in good shape from the start it would have been
merged by now. Just learn from your mistakes and next time things will
be smoother.

> [1]:
> https://lore.kernel.org/all/[email protected]/

Johan

2023-10-24 07:03:11

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 07/10] usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

On Mon, Oct 23, 2023 at 10:52:38PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:28 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:03PM +0530, Krishna Kurapati wrote:

> >> -#define PWR_EVNT_IRQ_STAT_REG 0x58
> >> +#define PWR_EVNT_IRQ1_STAT_REG 0x58
> >> +#define PWR_EVNT_IRQ2_STAT_REG 0x1dc
> >> +#define PWR_EVNT_IRQ3_STAT_REG 0x228
> >> +#define PWR_EVNT_IRQ4_STAT_REG 0x238
> >
> > Not sure these defines makes sense on their own. You now only use them
> > via the array below.
> >
> > I think I already asked you whether these offsets depend on SoC and you
> > said no, right?
> >
> There are only 3 QC SoC's today that support multiport.
> The offsets mentioned here are for SC8280 based platforms.
>
> For Sc8180 based platforms, these are the offsets:
> USB3_MP_PWR_EVNT_IRQ_STAT 0xA4F8858
> USB3_MP_PWR_EVNT_IRQ_1_STAT 0xA4F89DC
>
> These would translate to 0x58 and 0x1DC
>
> And for SX8380 the values are as follows:
>
> USB3_MP_PWR_EVNT_IRQ_STAT 0xA4F8858
> USB3_MP_PWR_EVNT_IRQ_1_STAT 0xA4F89DC
>
> So here also, the offsets are same. 0x58 and 0x1DC.
> So these are not SoC specific (atleast looking at the controllers
> present). But there is no mathematical pattern to denote this as in the
> following form (x + (port_num) * y). So made an array like this.

Sounds good. Thanks for confirming.

> >> +#define NUM_PWR_EVENT_STAT_REGS 4
> >
> > You already have MAX_PORTS, why are you defining a new define that will
> > always have to be equal to MAX_PORTS?
> >
> Do you recommend using the same max_ports ? If so, I can remove this
> macro altogether.

Indeed, and perhaps also some (compile-time) assert as the driver breaks
if they ever get out of sync.

Johan

2023-10-24 07:11:10

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

On Mon, Oct 23, 2023 at 10:57:04PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:17 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:02PM +0530, Krishna Kurapati wrote:
> >> Currently wakeup is supported by only single port controllers. Read speed
> >> of each port and accordingly enable IRQ's for those ports.
> >>
> >> Signed-off-by: Krishna Kurapati <[email protected]>
> >> ---

> >> -static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> >> +static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom,
> >> + int port_index)
> >
> > No need for line break (since it's a function definition).
> >
> >> {
> >> struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
> >> struct usb_device *udev;
> >> @@ -348,12 +349,10 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
> >>
> >> /*
> >> * It is possible to query the speed of all children of
> >> - * USB2.0 root hub via usb_hub_for_each_child(). DWC3 code
> >> - * currently supports only 1 port per controller. So
> >> - * this is sufficient.
> >> + * USB2.0 root hub via usb_hub_for_each_child().
> >
> > This comment no longer makes sense with your current implementation.
> >
> Can you help elaborate on your comment ? Do you mean that this API
> doesn't get speed on all ports, but this has to be called in a loop to
> get all the port speeds ? In that sense, I agree, I can change the
> comments here.

It does not make sense to keep only half the comment after your update
as it is a suggestion for how one could go about and generalise this for
multiport, which is what you are now doing.

> > But perhaps this should be done using usb_hub_for_each_child() instead
> > as that may be more efficient. Then you use this function to read out
> > the speed for all the ports in go (and store it in the port structures I
> > mentioned). Please determine which alternative is best.
> >
> Either ways is fine. We would have qcom->num_ports to determine how many
> speeds we can read.

That's not the point. I'm referring to which alternative is less
computationally expensive and allows for a clean implementation.

Please do try to figure it out yourself.

> >> */
> >> #ifdef CONFIG_USB
> >> - udev = usb_hub_find_child(hcd->self.root_hub, 1);
> >> + udev = usb_hub_find_child(hcd->self.root_hub, port_index + 1);
> >> #else
> >> udev = NULL;
> >> #endif
> >> @@ -386,23 +385,29 @@ static void dwc3_qcom_disable_wakeup_irq(int irq)
> >>
> >> static void dwc3_qcom_disable_interrupts(struct dwc3_qcom *qcom)
> >> {
> >> + int i;
> >> +
> >> dwc3_qcom_disable_wakeup_irq(qcom->hs_phy_irq);
> >>
> >> - if (qcom->usb2_speed == USB_SPEED_LOW) {
> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
> >> - } else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> >> - (qcom->usb2_speed == USB_SPEED_FULL)) {
> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
> >> - } else {
> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][0]);
> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][0]);
> >> - }
> >> + for (i = 0; i < qcom->num_ports; i++) {
> >> + if (qcom->usb2_speed[i] == USB_SPEED_LOW) {
> >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
> >> + } else if ((qcom->usb2_speed[i] == USB_SPEED_HIGH) ||
> >> + (qcom->usb2_speed[i] == USB_SPEED_FULL)) {
> >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
> >> + } else {
> >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DP_HS_PHY_IRQ_INDEX][i]);
> >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[DM_HS_PHY_IRQ_INDEX][i]);
> >> + }
> >>
> >> - dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][0]);
> >> + dwc3_qcom_disable_wakeup_irq(qcom->phy_irq[SS_PHY_IRQ_INDEX][i]);
> >> + }
> >> }
> >
> > The above is hardly readable, partly because of the 2d array that I
> > think you should drop, and partly because you add the port loop here
> > instead of in the caller.
> >
> > A lot of these functions should become port operation where you either
> > pass in a port structure directly or possibly a port index as I've
> > mentioned before.
>
> With your suggestion, yes, this can be refactored to be readable.
>
> >
> > [ I realise that the confusion around hs_phy_irq may be partly to blame
> > for this but since that one is also a per-port interrupt, that's no
> > longer an issue. ]
>
> I don't want to add support for this right away [1]. I would like to
> keep hs_phy_irq outside the loop for now.

No. Stop trying to take shortcuts. Again, this is upstream, not
Qualcomm's vendor kernel.

Johan

2023-10-24 07:13:24

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 08/10] arm64: dts: qcom: sc8280xp: Add multiport controller node for SC8280

On Mon, Oct 23, 2023 at 11:04:38PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:39 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:04PM +0530, Krishna Kurapati wrote:

> > The interrupt order does not match the binding, where the power event
> > interrupts come first.
> >
> > And we probably also want the hs_phy_irqs here after fixing the
> > incomplete binding.
>
> You want to update the driver code for this as well ? I have no problem
> in adding it in DT and binding but not in driver.

Possibly both.

> >> + usb_2_dwc3: usb@a400000 {
> >> + compatible = "snps,dwc3";
> >> + reg = <0 0x0a400000 0 0xcd00>;
> >> + interrupts = <GIC_SPI 133 IRQ_TYPE_LEVEL_HIGH>;
> >
> > I'd also like to know what that second dwc3 interrupt is for and whether
> > it should be defined here as well.
>
> Second interrupts is for HW acceleration I believe for which support is
> not there currently.

But the interrupt is there. And devicetree describes the hardware, not
the implementation, as I've pointed out before.

Johan

2023-10-24 07:20:38

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports

On Mon, Oct 23, 2023 at 11:12:40PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/23/2023 9:53 PM, Johan Hovold wrote:
> > On Sat, Oct 07, 2023 at 09:18:05PM +0530, Krishna Kurapati wrote:

> >> + usb2_en_state: usb2-en-state {
> >
> > No need to include '_state' in the labels.
> >
> Any specific reason ? I have no problem if removing the suffix but just
> wanted to know the reason.

For consistency with the rest of the sc8280xp devicetree sources
(including this file) where we've used that pattern (e.g. as reproducing
"state" in the label is mostly redundant). For example:

pcie2a_default: pcie2a-default-state {}

and

nvme_reg_en: nvme-reg-en-state {}

Johan

2023-10-24 08:27:10

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 09/10] arm64: dts: qcom: sa8295p: Enable tertiary controller and its 4 USB ports



On 10/24/2023 12:50 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 11:12:40PM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/23/2023 9:53 PM, Johan Hovold wrote:
>>> On Sat, Oct 07, 2023 at 09:18:05PM +0530, Krishna Kurapati wrote:
>
>>>> + usb2_en_state: usb2-en-state {
>>>
>>> No need to include '_state' in the labels.
>>>
>> Any specific reason ? I have no problem if removing the suffix but just
>> wanted to know the reason.
>
> For consistency with the rest of the sc8280xp devicetree sources
> (including this file) where we've used that pattern (e.g. as reproducing
> "state" in the label is mostly redundant). For example:
>
> pcie2a_default: pcie2a-default-state {}
>
> and
>
> nvme_reg_en: nvme-reg-en-state {}
>

Thanks for the reference.

Regards,
Krishna,

2023-10-24 08:42:28

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport


On 10/24/2023 12:40 PM, Johan Hovold wrote:
>>>
>>> This comment no longer makes sense with your current implementation.
>>>
>> Can you help elaborate on your comment ? Do you mean that this API
>> doesn't get speed on all ports, but this has to be called in a loop to
>> get all the port speeds ? In that sense, I agree, I can change the
>> comments here.
>
> It does not make sense to keep only half the comment after your update
> as it is a suggestion for how one could go about and generalise this for
> multiport, which is what you are now doing.
>

Thanks for explanation. Will update the comments.

>>> But perhaps this should be done using usb_hub_for_each_child() instead
>>> as that may be more efficient. Then you use this function to read out
>>> the speed for all the ports in go (and store it in the port structures I
>>> mentioned). Please determine which alternative is best.
>>>
>> Either ways is fine. We would have qcom->num_ports to determine how many
>> speeds we can read.
>
> That's not the point. I'm referring to which alternative is less
> computationally expensive and allows for a clean implementation.
>
> Please do try to figure it out yourself.
>
I don't think its much of a difference:

while (loop over num_ports) {
read_usb2_speed()
}

read_usb2_speed() {
while (loop over num_ports) {
hub api to read speed.
}
}

The second one would avoid calling read_usb2_speed multiple times. Will
take that path.

>>>
>>> [ I realise that the confusion around hs_phy_irq may be partly to blame
>>> for this but since that one is also a per-port interrupt, that's no
>>> longer an issue. ]
>>
>> I don't want to add support for this right away [1]. I would like to
>> keep hs_phy_irq outside the loop for now.
>
> No. Stop trying to take shortcuts. Again, this is upstream, not
> Qualcomm's vendor kernel.
>

I don't think it is a shortcut.

The reason I said I would keep it out of loop is I know why we need
DP/DM/SS IRQ's during wakeup. The wakeup signals come in as
rising/falling edges in high speed on DP/DM lines and LFPS terminations
come on SS lines.

So we need these 3 interrupts for sure in wakeup context.
hs_phy_irq is not mandatory for wakeup. Any particular reason why it is
needed to add driver support for hs_phy_irq's of multiport now ? May be
I am missing something. If there is any reason why we need to add it
now, I would try to learn and see if it has any side effects (like
generating spurious wakeup's) and if nothing, I would add it back to
port structure.

Regards,
Krishna,

2023-10-24 08:55:22

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 10/24/2023 12:26 PM, Johan Hovold wrote:
>
> You need to provide this information so that we can determine what the
> binding should look like. The implementation would also be simplified if
> we don't have to add random hacks to it just because we don't know why
> the vendor driver you refer does not use it currently on this particular
> platform.
>
>> Also I plan on splitting the patchset into 4 parts (essentially 4 diff
>> series):
>>
>> 1. Bindings update for hs_phy_irq's
>> 2. DT patches for MP controller and platform specific files
>> 3. Core driver update for supporting multiport
>> 4. QCOM driver update for supporting wakeup/suspend/resume
>>
>> This is in accordance to [1] and that way qcom code won't block core
>> driver changes from getting merged. Core driver changes are independent
>> and are sufficient to get multiport working.
>
> No, you clearly did not understand [1] at all. And stop trying to game
> the upstreaming process. Bindings and driver patches go together. The
> devicetree changes can be sent separately in case of USB but only
> *after* the first set has been merged.
>
> If the code had been in good shape from the start it would have been
> merged by now. Just learn from your mistakes and next time things will
> be smoother.
>

I agree that bindings should go first. My point is core bindings are
already approved and merged and just wanted to check if core driver
changes can be merged without glue blocking them. Core driver changes
have nothing to do with interrupt handling in glue. If we get the core
changes merged separately after fixing the nits mentioned, we can take
up this interrupt handling in glue in parallel. I am just trying to see
if we can start merging independent portions of code. I agree that my
glue driver changes are still not upto mark. But that has nothing to do
with core driver changes.

Please let me know if that is appropriate because I think functionality
intended by changes in core is independent of glue driver and glue
bindings. If anything glue is partially dependent on core changes (like
the MAX_PORTS macro etc., but not the other way around).

Regards,
Krishna,

2023-10-24 09:06:32

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 06/10] usb: dwc3: qcom: Enable wakeup for applicable ports of multiport

On Tue, Oct 24, 2023 at 02:11:31PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/24/2023 12:40 PM, Johan Hovold wrote:

> >>> But perhaps this should be done using usb_hub_for_each_child() instead
> >>> as that may be more efficient. Then you use this function to read out
> >>> the speed for all the ports in go (and store it in the port structures I
> >>> mentioned). Please determine which alternative is best.
> >>>
> >> Either ways is fine. We would have qcom->num_ports to determine how many
> >> speeds we can read.
> >
> > That's not the point. I'm referring to which alternative is less
> > computationally expensive and allows for a clean implementation.
> >
> > Please do try to figure it out yourself.
> >
> I don't think its much of a difference:
>
> while (loop over num_ports) {
> read_usb2_speed()
> }
>
> read_usb2_speed() {
> while (loop over num_ports) {
> hub api to read speed.
> }
> }
>
> The second one would avoid calling read_usb2_speed multiple times. Will
> take that path.

You need to look at the implementation of usb_hub_for_each_child() and
usb_hub_find_child() to determine that, which you now forced me to
do; and yes, you're right, this shouldn't matter from an efficiency
standpoint.

> >>> [ I realise that the confusion around hs_phy_irq may be partly to blame
> >>> for this but since that one is also a per-port interrupt, that's no
> >>> longer an issue. ]
> >>
> >> I don't want to add support for this right away [1]. I would like to
> >> keep hs_phy_irq outside the loop for now.
> >
> > No. Stop trying to take shortcuts. Again, this is upstream, not
> > Qualcomm's vendor kernel.
>
> I don't think it is a shortcut.
>
> The reason I said I would keep it out of loop is I know why we need
> DP/DM/SS IRQ's during wakeup. The wakeup signals come in as
> rising/falling edges in high speed on DP/DM lines and LFPS terminations
> come on SS lines.

It is a shortcut as this interrupt is per-port and some SoC's already
use it. So you're making a mess of the implementation for no good
reason.

> So we need these 3 interrupts for sure in wakeup context.
> hs_phy_irq is not mandatory for wakeup. Any particular reason why it is
> needed to add driver support for hs_phy_irq's of multiport now ? May be
> I am missing something. If there is any reason why we need to add it
> now, I would try to learn and see if it has any side effects (like
> generating spurious wakeup's) and if nothing, I would add it back to
> port structure.

As I've mentioned a few times now, the hs_phy_irq is already used by a
few SoC's so you can't just pretend it doesn't exist and mess up the
implementation for no good reason.

Just find out how it is used and why only some Qualcomm SoC's use it
currently. It appears to be used in parallel with the DP/DM interrupts,
and it has been there from the start:

a4333c3a6ba9 ("usb: dwc3: Add Qualcomm DWC3 glue driver")

Sure, the wakeup implementation was incomplete and broken for a long
time, but I'm not going to let you continue this practise of pushing
incomplete hacks upstream which someone else will eventually be forced
to clean up. You have the documentation, just use it.

Johan

2023-10-24 09:18:27

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Tue, Oct 24, 2023 at 02:23:57PM +0530, Krishna Kurapati PSSNV wrote:
> On 10/24/2023 12:26 PM, Johan Hovold wrote:

> > No, you clearly did not understand [1] at all. And stop trying to game
> > the upstreaming process. Bindings and driver patches go together. The
> > devicetree changes can be sent separately in case of USB but only
> > *after* the first set has been merged.
> >
> > If the code had been in good shape from the start it would have been
> > merged by now. Just learn from your mistakes and next time things will
> > be smoother.
>
> I agree that bindings should go first. My point is core bindings are
> already approved and merged and just wanted to check if core driver
> changes can be merged without glue blocking them. Core driver changes
> have nothing to do with interrupt handling in glue. If we get the core
> changes merged separately after fixing the nits mentioned, we can take
> up this interrupt handling in glue in parallel. I am just trying to see
> if we can start merging independent portions of code. I agree that my
> glue driver changes are still not upto mark. But that has nothing to do
> with core driver changes.

Again, no. The dwc3 glue and core bits are not independent, and ideally
the bindings should not have been merged either before having the
implementation in a decent shape either (e.g. as the messy
implementation suggested that the bindings were incomplete).

You're again trying to sneak in an incomplete implementation. Qualcomm
has a terrible track record of doing just that and leaving others with
the task to clean up their mess.

This should go in as one series, when it's ready, and not before.

And we may even consider reverting the updated bindings as it appears
they are still not correct.

Johan

2023-10-24 09:23:35

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote:
> On Tue, Oct 24, 2023 at 02:23:57PM +0530, Krishna Kurapati PSSNV wrote:
> > On 10/24/2023 12:26 PM, Johan Hovold wrote:
>
> > > No, you clearly did not understand [1] at all. And stop trying to game
> > > the upstreaming process. Bindings and driver patches go together. The
> > > devicetree changes can be sent separately in case of USB but only
> > > *after* the first set has been merged.
> > >
> > > If the code had been in good shape from the start it would have been
> > > merged by now. Just learn from your mistakes and next time things will
> > > be smoother.
> >
> > I agree that bindings should go first. My point is core bindings are
> > already approved and merged and just wanted to check if core driver
> > changes can be merged without glue blocking them. Core driver changes
> > have nothing to do with interrupt handling in glue. If we get the core
> > changes merged separately after fixing the nits mentioned, we can take
> > up this interrupt handling in glue in parallel. I am just trying to see
> > if we can start merging independent portions of code. I agree that my
> > glue driver changes are still not upto mark. But that has nothing to do
> > with core driver changes.
>
> Again, no. The dwc3 glue and core bits are not independent, and ideally
> the bindings should not have been merged either before having the
> implementation in a decent shape either (e.g. as the messy
> implementation suggested that the bindings were incomplete).
>
> You're again trying to sneak in an incomplete implementation. Qualcomm
> has a terrible track record of doing just that and leaving others with
> the task to clean up their mess.
>
> This should go in as one series, when it's ready, and not before.
>
> And we may even consider reverting the updated bindings as it appears
> they are still not correct.

If you can tell me what the git ids of them are, I'll be glad to do so
right now, sorry for taking them "early".

thanks,

greg k-h

2023-10-24 09:31:12

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Tue, Oct 24, 2023 at 11:23:19AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote:

> > And we may even consider reverting the updated bindings as it appears
> > they are still not correct.
>
> If you can tell me what the git ids of them are, I'll be glad to do so
> right now, sorry for taking them "early".

That's

ca58c4ae75b6 ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport")

and

eb3f1d9e42b1 ("dt-bindings: usb: Add bindings for multiport properties on DWC3 controller")

It's probably best to just revert them now.

Thanks.

Johan

2023-10-24 09:54:40

by Greg KH

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Tue, Oct 24, 2023 at 11:29:17AM +0200, Johan Hovold wrote:
> On Tue, Oct 24, 2023 at 11:23:19AM +0200, Greg Kroah-Hartman wrote:
> > On Tue, Oct 24, 2023 at 11:18:26AM +0200, Johan Hovold wrote:
>
> > > And we may even consider reverting the updated bindings as it appears
> > > they are still not correct.
> >
> > If you can tell me what the git ids of them are, I'll be glad to do so
> > right now, sorry for taking them "early".
>
> That's
>
> ca58c4ae75b6 ("dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport")
>
> and
>
> eb3f1d9e42b1 ("dt-bindings: usb: Add bindings for multiport properties on DWC3 controller")
>
> It's probably best to just revert them now.

Now reverted, thanks.

greg k-h

2023-11-03 10:06:32

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 10/24/2023 12:26 PM, Johan Hovold wrote:
> On Mon, Oct 23, 2023 at 10:42:31PM +0530, Krishna Kurapati PSSNV wrote:
>> On 10/23/2023 7:37 PM, Johan Hovold wrote:
>
>>> Right. And I assume there are hs_phy_irqs also for the first two USB
>>> controllers on sc8280xp?
>
>> There are, I can dig through and find out. Atleast in downstream I don't
>> see any use of them.
>
> Yes, please do post how these are wired as well for completeness.
>
>>> Can you find out anything more about what hs_phy_irq is used for? It
>>> appears to be an HS wakeup interrupt like the dp/dm ones, but there are
>>> not really any details on how it is supposed to be used.
>>
>> This IRQ is really not used in downstream controllers. Not sure if its
>> a good idea to add driver code for that. I did some digging and I got
>> the reason why I first said that there is only one hs_phy_irq for
>> tertiary port of controller. The hardware programming sequence doesn't
>> specify usage of these 4 IRQ's but the hw specifics mention that there
>> are 4 of them. Adding driver support for these IRQ's is not a good idea
>> (atleast at this point because they are not used in downstream and I am
>> not sure what would be the side effect). For now I suggest we can add
>> them in bindings and DT and not handle the 4 hs_phy_irq's in the driver
>> code (meaning not add the hs_phy_irq to port structure we plan on adding
>> to dwc3_qcom).
>
> But there is already support for these interrupts in the driver. You
> work for Qualcomm who built the thing so surely you can figure how they
> intended these to be used?
>
> You need to provide this information so that we can determine what the
> binding should look like. The implementation would also be simplified if
> we don't have to add random hacks to it just because we don't know why
> the vendor driver you refer does not use it currently on this particular
> platform.
>

Hi Johan,

Regarding the points of discussion we had last week on [1], here are
some clarifications:

1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as
mentioned. Why do we need them and would we use it in multiport targets ?

DPSE and DMSE are single ended line state of DP and DM lines. The DP
line and DM line stay in steady High or Low during suspend and they flip
when there is a RESUME or REMOTE WAKE. This is what we do/check in
dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed.

Initially in QUSB2 targets, the interrupts were enabled and configured
in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2].
In that case, we modify DP/DM interrupts in phy registers, specifically
QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is
triggered. But in femto targets, this is done via DP/DM interrupts and
there is no use of hs_phy_irq. Even hw folks confirmed they dont use
hs_ph_irq in femto phy targets.

As an experiment, I tried to test wakeup by pressing buttons on
connected keyboard when in suspend state or connecting/disconnecting
keyboard in suspended state on different ports and only see dp/dm IRQ's
getting fired although we register for hs_phy_irq as well:

/ # cat /proc/interrupts |grep phy_
171: 1 0 0 0 0 0 0 0 PDC 127 Edge dp_hs_phy_1
172: 2 0 0 0 0 0 0 0 PDC 126 Edge dm_hs_phy_1
173: 3 0 0 0 0 0 0 0 PDC 129 Edge dp_hs_phy_2
174: 4 0 0 0 0 0 0 0 PDC 128 Edge dm_hs_phy_2
175: 0 0 0 0 0 0 0 0 PDC 131 Edge dp_hs_phy_3
176: 2 0 0 0 0 0 0 0 PDC 130 Edge dm_hs_phy_3
177: 2 0 0 0 0 0 0 0 PDC 133 Edge dp_hs_phy_4
178: 5 0 0 0 0 0 0 0 PDC 132 Edge dm_hs_phy_4
179: 0 0 0 0 0 0 0 0 PDC 16 Level ss_phy_1
180: 0 0 0 0 0 0 0 0 PDC 17 Level ss_phy_2
181: 0 0 0 0 0 0 0 0 GICv3 163 Level hs_phy_1
182: 0 0 0 0 0 0 0 0 GICv3 168 Level hs_phy_2
183: 0 0 0 0 0 0 0 0 GICv3 892 Level hs_phy_3
184: 0 0 0 0 0 0 0 0 GICv3 891 Level hs_phy_4

Since the hs_phy_irq is applicable only for qusb2 targets, do we still
need to add it to DT.

2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]):

BAM IRQ is not needed in host-only controller. It was just added in
process of porting/deriving code from DRD controllers and is
non-functional (confirmed by HW team here). We can skip this from DT of
multiport.

3. ctrl_irq[1] usage:

This is a feature of SNPS controller, not qcom glue wrapper, and is
present on all targets (non-QC as well probably). As mentioned before on
[3], this is used for HW acceleration.

In host mode, XHCI spec does allow for multiple interrupters when
multiple event rings are used. A possible usage is multiple execution
environments something like what we are doing on mobile with ADSP audio
offload [4]. Another possibility could be some of virtualization where
host/hyp would manage the first interrupter and could allow a guest to
operate only with the second (though current design does not go far
enough to offer true isolation for real VM type workloads). The
additional interrupts (ones other than ctrl_irq[0]) are either for
virtualization use cases, or for our various “hw offload” features. In
device mode, these are used for offloading tethering functionality to
IPA FW.

Since the DeviceTree passed to the OS, should describe the hardware to
the OS, and should represent the hardware from the point-of-view of the
OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux
would not use the other interrupts. Furthermore AFAIK even UEFI/Windows
also use only ctrl_irq[0] for host mode in their execution environment
today. Do we still need to add this to bindings and DT ?

[1]: https://lore.kernel.org/all/[email protected]/
[2]:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-qusb2.c?h=v6.6#n626
[3]: https://lore.kernel.org/all/[email protected]/
[4]:
https://lore.kernel.org/all/[email protected]/

Regards,
Krishna,

2023-11-07 08:31:12

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

>>
>> But there is already support for these interrupts in the driver. You
>> work for Qualcomm who built the thing so surely you can figure how they
>> intended these to be used?
>>
>> You need to provide this information so that we can determine what the
>> binding should look like. The implementation would also be simplified if
>> we don't have to add random hacks to it just because we don't know why
>> the vendor driver you refer does not use it currently on this particular
>> platform.
>>
>
> Hi Johan,
>
> Regarding the points of discussion we had last week on [1], here are
> some clarifications:
>
> 1. We do have hs_phy_irq 1/2/3/4 for tertiary port of Sc8280 as
> mentioned. Why do we need them and would we use it in multiport targets ?
>
> DPSE and DMSE are single ended line state of DP and DM lines. The DP
> line and DM line stay in steady High or Low during suspend and they flip
> when there is a RESUME or REMOTE WAKE. This is what we do/check in
> dwc3_qcom_enable_interrupts call for dp/dm irq's based on usb2_speed.
>
> Initially in QUSB2 targets, the interrupts were enabled and configured
> in phy and the wakeup was interrupt was read on hs_phy_irq vector - [2].
> In that case, we modify DP/DM interrupts in phy registers, specifically
> QUSB2PHY_INTR_CTRL and when wakeup signal comes in, hs_phy_irq is
> triggered. But in femto targets, this is done via DP/DM interrupts and
> there is no use of hs_phy_irq. Even hw folks confirmed they dont use
> hs_ph_irq in femto phy targets.
>
> As an experiment, I tried to test wakeup by pressing buttons on
> connected keyboard when in suspend state or connecting/disconnecting
> keyboard in suspended state on different ports and only see dp/dm IRQ's
> getting fired although we register for hs_phy_irq as well:
>
> / # cat /proc/interrupts  |grep phy_
> 171:   1  0   0   0  0  0  0  0       PDC 127 Edge      dp_hs_phy_1
> 172:   2  0   0   0  0  0  0  0       PDC 126 Edge      dm_hs_phy_1
> 173:   3  0   0   0  0  0  0  0       PDC 129 Edge      dp_hs_phy_2
> 174:   4  0   0   0  0  0  0  0       PDC 128 Edge      dm_hs_phy_2
> 175:   0  0   0   0  0  0  0  0       PDC 131 Edge      dp_hs_phy_3
> 176:   2  0   0   0  0  0  0  0       PDC 130 Edge      dm_hs_phy_3
> 177:   2  0   0   0  0  0  0  0       PDC 133 Edge      dp_hs_phy_4
> 178:   5  0   0   0  0  0  0  0       PDC 132 Edge      dm_hs_phy_4
> 179:   0  0   0   0  0  0  0  0       PDC  16 Level     ss_phy_1
> 180:   0  0   0   0  0  0  0  0       PDC  17 Level     ss_phy_2
> 181:   0  0   0   0  0  0  0  0     GICv3 163 Level     hs_phy_1
> 182:   0  0   0   0  0  0  0  0     GICv3 168 Level     hs_phy_2
> 183:   0  0   0   0  0  0  0  0     GICv3 892 Level     hs_phy_3
> 184:   0  0   0   0  0  0  0  0     GICv3 891 Level     hs_phy_4
>
> Since the hs_phy_irq is applicable only for qusb2 targets, do we still
> need to add it to DT.
>
> 2. BAM Irq usage (u_usb31_scnd_mvs_pipe_wrapper_usb31_bam_irq[0]):
>
> BAM IRQ is not needed in host-only controller. It was just added in
> process of porting/deriving code from DRD controllers and is
> non-functional (confirmed by HW team here). We can skip this from DT of
> multiport.
>
> 3. ctrl_irq[1] usage:
>
> This is a feature of SNPS controller, not qcom glue wrapper, and is
> present on all targets (non-QC as well probably). As mentioned before on
> [3], this is used for HW acceleration.
>
> In host mode, XHCI spec does allow for multiple interrupters when
> multiple event rings are used. A possible usage is multiple execution
> environments something like what we are doing on mobile with ADSP audio
> offload [4]. Another possibility could be some of virtualization where
> host/hyp would manage the first interrupter and could allow a guest to
> operate only with the second (though current design does not go far
> enough to offer true isolation for real VM type workloads). The
> additional interrupts (ones other than ctrl_irq[0]) are either for
> virtualization use cases, or for our various “hw offload” features. In
> device mode, these are used for offloading tethering functionality to
> IPA FW.
>
> Since the DeviceTree passed to the OS, should describe the hardware to
> the OS, and should represent the hardware from the point-of-view of the
> OS, adding one interrupt (ctrl_irq[0]) might be sufficient as Linux
> would not use the other interrupts. Furthermore AFAIK even UEFI/Windows
> also use only ctrl_irq[0] for host mode in their execution environment
> today. Do we still need to add this to bindings and DT ?
>
> [1]: https://lore.kernel.org/all/[email protected]/
> [2]:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/phy/qualcomm/phy-qcom-qusb2.c?h=v6.6#n626
> [3]: https://lore.kernel.org/all/[email protected]/
> [4]:
> https://lore.kernel.org/all/[email protected]/
>

Hi Johan,

Can you help provide your comments on the above mentioned points so
that we can take the discussion forward and finalize changes for v14
patches. And thanks for all the reviews so far on previous revisions.

Regards,
Krishna,

2023-11-15 17:43:20

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver


Hi Johan,

> Are you sure there's no support for hs_phy_irq also in the "femto" PHYs
> and that it's just that there is currently no driver support for using
> them?
>
> And why is it defined if there is truly no use for it?
>

We had an internal sync up with HW folks and here is some baseline
suggestions we received:

If DP/DM interrupts are defined, then that is the preferred path to
used, irrespective if HS Phy irq is defined or not / or whether it is
Femto / QUSB2 target. There is no target that has femto phy but misses
DP/DM today.

For cases like sdm660/msm8998/msm8953/msm8956, these targets use
hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding
in entirety.

> Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does
> the following Qualcomm SoCs define all three?
>

HS Phy Irq is redundant or functionality is mutually exclusive in this
case. If there are targets that define all three, then we need to update
those to only utilize DP/DM interrupts.

Regards,
Krishna,

2023-11-16 13:03:34

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Wed, Nov 15, 2023 at 11:12:16PM +0530, Krishna Kurapati PSSNV wrote:

> > Are you sure there's no support for hs_phy_irq also in the "femto" PHYs
> > and that it's just that there is currently no driver support for using
> > them?
> >
> > And why is it defined if there is truly no use for it?

> We had an internal sync up with HW folks and here is some baseline
> suggestions we received:
>
> If DP/DM interrupts are defined, then that is the preferred path to
> used, irrespective if HS Phy irq is defined or not / or whether it is
> Femto / QUSB2 target. There is no target that has femto phy but misses
> DP/DM today.

Ok, but just knowing that it is "preferred" does not in itself mean that
it should be removed from the binding.

We need to know that it's effectively useless (i.e. that the interrupts
are defined but cannot be triggered) for that.

We can still use the DP/DM interrupts in favour of HS in the driver
however.

> For cases like sdm660/msm8998/msm8953/msm8956, these targets use
> hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding
> in entirety.

I fixed the binding for those specific platforms last year:

dd566faebe9f ("dt-bindings: usb: qcom,dwc3: refine interrupt requirements")

But as I mentioned in that commit message the following platforms do not
have any wakeup interrupts specified in mainline currently:

- qcom,ipq4019-dwc3
- qcom,ipq6018-dwc3
- qcom,ipq8064-dwc3
- qcom,ipq8074-dwc3
- qcom,msm8994-dwc3
- qcom,qcs404-dwc3

It would be good to get that cleaned up too (i.e. add the missing
interrupt definitions and update the binding to match).

> > Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does
> > the following Qualcomm SoCs define all three?

> HS Phy Irq is redundant or functionality is mutually exclusive in this
> case. If there are targets that define all three, then we need to update
> those to only utilize DP/DM interrupts.

No, as I wrote above that depends on if the HS interrupt is truly
useless. Otherwise it still belongs in the binding, even if the driver
uses DP/DM in place of it.

Again, the binding should describe the hardware, not what a particular
OS chooses to use.

Johan

2023-11-22 19:33:26

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 11/16/2023 6:33 PM, Johan Hovold wrote:
> On Wed, Nov 15, 2023 at 11:12:16PM +0530, Krishna Kurapati PSSNV wrote:
>
>>> Are you sure there's no support for hs_phy_irq also in the "femto" PHYs
>>> and that it's just that there is currently no driver support for using
>>> them?
>>>
>>> And why is it defined if there is truly no use for it?
>
>> We had an internal sync up with HW folks and here is some baseline
>> suggestions we received:
>>
>> If DP/DM interrupts are defined, then that is the preferred path to
>> used, irrespective if HS Phy irq is defined or not / or whether it is
>> Femto / QUSB2 target. There is no target that has femto phy but misses
>> DP/DM today.
>
> Ok, but just knowing that it is "preferred" does not in itself mean that
> it should be removed from the binding.
>
> We need to know that it's effectively useless (i.e. that the interrupts
> are defined but cannot be triggered) for that.
>
> We can still use the DP/DM interrupts in favour of HS in the driver
> however.
>
>> For cases like sdm660/msm8998/msm8953/msm8956, these targets use
>> hs_phy_irq only and don't rely on DP/DM. So we cannot remove the binding
>> in entirety.
>
> I fixed the binding for those specific platforms last year:
>
> dd566faebe9f ("dt-bindings: usb: qcom,dwc3: refine interrupt requirements")
>
> But as I mentioned in that commit message the following platforms do not
> have any wakeup interrupts specified in mainline currently:
>
> - qcom,ipq4019-dwc3
> - qcom,ipq6018-dwc3
> - qcom,ipq8064-dwc3
> - qcom,ipq8074-dwc3
> - qcom,msm8994-dwc3
> - qcom,qcs404-dwc3
>
> It would be good to get that cleaned up too (i.e. add the missing
> interrupt definitions and update the binding to match).
>
>>> Also, if hs_phy_irq and dp/dm_phy_irq were mutually exclusive, why does
>>> the following Qualcomm SoCs define all three?
>
>> HS Phy Irq is redundant or functionality is mutually exclusive in this
>> case. If there are targets that define all three, then we need to update
>> those to only utilize DP/DM interrupts.
>
> No, as I wrote above that depends on if the HS interrupt is truly
> useless. Otherwise it still belongs in the binding, even if the driver
> uses DP/DM in place of it.
>
> Again, the binding should describe the hardware, not what a particular
> OS chooses to use.
>

Hi Johan,

Sorry for delayed response.

Pushed [1] to address all the queries and comments. I was initially
looking at only Femto phy targets, but when I looked at all targets in
general, seems there is one irq not defined in bindings. It is qubs2_phy
irq which is named as "hs_phy_irq" on QUSB target DT's (both downstream
and upstream).

There is one actual "hs_phy_irq" as well but it is not used either by hs
validation team or sw team on any target. It was put in for debug
purpose only and doesn't have code to trigger it (even downstream never
implemented it I suppose) Atleast 4.4 onwards I saw the code but I
didn't see the actual hs_phy_irq being used. It was the qusb2_phy irq
named as hs_phy_irq.

Even hw folks used it under the same name which is why they recommended
using it on qusb2 targets and dp/dm on femto targets.

As we moved from qusb2 to femto phys, since qusb2_phy irq was not
present, the actual hs_phy_irq was put in the interrupts although it is
never triggered. That is why when I tried to check on sa8295-adp
multiport, those 4 hs interrupts never got fired. Hope the explanation
clears the confusion present around the interrupts.

On some targets the hs_phy_irq was given vector number of pwr_event irq
also like sm8550/sm8450 etc., I tried to address those as well in the
series.

Also, per your question as to there are some qusb2 targets having dp/dm
interrupts defined... It is only for SDM845/SDM670/SM6350 which were
last in line of using qusb2 phy's and they started incorporating dp/dm
interrupts.

Also added missing interrupts for qcs404/ipq5332.
I didn't add missing interrupts on sc8280xp because I see that current
interrupts present are working fine (I see ADB working and wakeup
working as well), but the interrupt vector numbers are off by "1"
between hs specifics and DT (both upstream and downstream). Will sort it
out and clean that target up later.

[1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412

Regards,
Krishna,

2023-11-23 13:44:45

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Thu, Nov 23, 2023 at 01:02:24AM +0530, Krishna Kurapati PSSNV wrote:

> Pushed [1] to address all the queries and comments. I was initially
> looking at only Femto phy targets, but when I looked at all targets in
> general, seems there is one irq not defined in bindings. It is qubs2_phy
> irq which is named as "hs_phy_irq" on QUSB target DT's (both downstream
> and upstream).
>
> There is one actual "hs_phy_irq" as well but it is not used either by hs
> validation team or sw team on any target. It was put in for debug
> purpose only and doesn't have code to trigger it (even downstream never
> implemented it I suppose) Atleast 4.4 onwards I saw the code but I
> didn't see the actual hs_phy_irq being used. It was the qusb2_phy irq
> named as hs_phy_irq.
>
> Even hw folks used it under the same name which is why they recommended
> using it on qusb2 targets and dp/dm on femto targets.

Ah, thanks for getting to the bottom of this.

> On some targets the hs_phy_irq was given vector number of pwr_event irq
> also like sm8550/sm8450 etc., I tried to address those as well in the
> series.

I can imagine that we have a number of such issues.

> Also, per your question as to there are some qusb2 targets having dp/dm
> interrupts defined... It is only for SDM845/SDM670/SM6350 which were
> last in line of using qusb2 phy's and they started incorporating dp/dm
> interrupts.

Ok.

> Also added missing interrupts for qcs404/ipq5332.

Thanks.

> I didn't add missing interrupts on sc8280xp because I see that current
> interrupts present are working fine (I see ADB working and wakeup
> working as well), but the interrupt vector numbers are off by "1"
> between hs specifics and DT (both upstream and downstream). Will sort it
> out and clean that target up later.

Which interrupt numbers are off by one here?

> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412

I took a quick look at the series, and it looks like this will
eventually clean things up a lot. We should probably define a generic
order for the interrupts with the sometimes optional SS interrupts last.

Side note: It looks like the threading in that series is broken.
Consider using git-send-email for sending series as it takes care of
things like that.

Johan

2023-11-24 09:02:00

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

>
>> I didn't add missing interrupts on sc8280xp because I see that current
>> interrupts present are working fine (I see ADB working and wakeup
>> working as well), but the interrupt vector numbers are off by "1"
>> between hs specifics and DT (both upstream and downstream). Will sort it
>> out and clean that target up later.
>
> Which interrupt numbers are off by one here?
>

My bad, this might be the confusion. The HW specifics say:

Controller-2, power_event irq:

SYS_apcsQgicSPI[812] Vector-number: 843


Usually vector number = 32 + GIC number AFAIK.
By that logic, If vector number is 843, GIC_SPI number is 811 which is
same as DT. Probably the GIC_SPI number is printed wrong. The DT matches
(vector number - 32).

Sorry for mentioning that it is wrong. The DT entries are right and it
is working on upstream.

The missing hs_phy_irq's have been put on the mail thread on this list
before.

Regards,
Krishna,

>> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412
>
> I took a quick look at the series, and it looks like this will
> eventually clean things up a lot. We should probably define a generic
> order for the interrupts with the sometimes optional SS interrupts last.
>
> Side note: It looks like the threading in that series is broken.
> Consider using git-send-email for sending series as it takes care of
> things like that.
>

Usually I do git send-email for the whole out folder where the patches
are present, but linux-usb list is common to all the patches in that
case, even the DT ones. So to avoid that and to send patches to only
relavant mailing lists, I did git send email individually on each patch
which might have caused this issue.

Will make sure this won't happen again.

Regards,
Krishna,

2023-11-24 09:13:34

by Krzysztof Kozlowski

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On 24/11/2023 10:00, Krishna Kurapati PSSNV wrote:
>>
>>> I didn't add missing interrupts on sc8280xp because I see that current
>>> interrupts present are working fine (I see ADB working and wakeup
>>> working as well), but the interrupt vector numbers are off by "1"
>>> between hs specifics and DT (both upstream and downstream). Will sort it
>>> out and clean that target up later.
>>
>> Which interrupt numbers are off by one here?
>>
>
> My bad, this might be the confusion. The HW specifics say:
>
> Controller-2, power_event irq:
>
> SYS_apcsQgicSPI[812] Vector-number: 843
>
>
> Usually vector number = 32 + GIC number AFAIK.
> By that logic, If vector number is 843, GIC_SPI number is 811 which is
> same as DT. Probably the GIC_SPI number is printed wrong. The DT matches
> (vector number - 32).
>
> Sorry for mentioning that it is wrong. The DT entries are right and it
> is working on upstream.
>
> The missing hs_phy_irq's have been put on the mail thread on this list
> before.
>
> Regards,
> Krishna,
>
>>> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412
>>
>> I took a quick look at the series, and it looks like this will
>> eventually clean things up a lot. We should probably define a generic
>> order for the interrupts with the sometimes optional SS interrupts last.
>>
>> Side note: It looks like the threading in that series is broken.
>> Consider using git-send-email for sending series as it takes care of
>> things like that.
>>
>
> Usually I do git send-email for the whole out folder where the patches
> are present, but linux-usb list is common to all the patches in that
> case, even the DT ones. So to avoid that and to send patches to only
> relavant mailing lists, I did git send email individually on each patch
> which might have caused this issue.

I don't understand why. This is some weird workflow. If you do not use
b4, then it is simple:
git format-patch -10 -v13
get_maintainers v13*
git send-email v13*
And that's it. Last two steps can be even one command, like I am doing
(shared the macro multiple times).

Best regards,
Krzysztof

2023-11-24 10:13:27

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Fri, Nov 24, 2023 at 02:30:56PM +0530, Krishna Kurapati PSSNV wrote:
> >
> >> I didn't add missing interrupts on sc8280xp because I see that current
> >> interrupts present are working fine (I see ADB working and wakeup
> >> working as well), but the interrupt vector numbers are off by "1"
> >> between hs specifics and DT (both upstream and downstream). Will sort it
> >> out and clean that target up later.
> >
> > Which interrupt numbers are off by one here?

> Sorry for mentioning that it is wrong. The DT entries are right and it
> is working on upstream.

Thanks for clarifying.

> >> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412
> >
> > I took a quick look at the series, and it looks like this will
> > eventually clean things up a lot. We should probably define a generic
> > order for the interrupts with the sometimes optional SS interrupts last.
> >
> > Side note: It looks like the threading in that series is broken.
> > Consider using git-send-email for sending series as it takes care of
> > things like that.
>
> Usually I do git send-email for the whole out folder where the patches
> are present, but linux-usb list is common to all the patches in that
> case, even the DT ones. So to avoid that and to send patches to only
> relavant mailing lists, I did git send email individually on each patch
> which might have caused this issue.

I'd suggest that you just send two separate series, one with binding and
driver updates, which will eventually be merged by Greg, and one with
the devicetree changes, which goes through Bjorn's tree.

It's good if you could add a link to the binding series in the cover
letter of the devicetree changes as they are of course going to be quite
closely related and need to be reviewed in parallel.

Johan

2023-11-24 10:39:11

by Krishna Kurapati PSSNV

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver



On 11/24/2023 3:43 PM, Johan Hovold wrote:
> On Fri, Nov 24, 2023 at 02:30:56PM +0530, Krishna Kurapati PSSNV wrote:
>>>
>>>> I didn't add missing interrupts on sc8280xp because I see that current
>>>> interrupts present are working fine (I see ADB working and wakeup
>>>> working as well), but the interrupt vector numbers are off by "1"
>>>> between hs specifics and DT (both upstream and downstream). Will sort it
>>>> out and clean that target up later.
>>>
>>> Which interrupt numbers are off by one here?
>
>> Sorry for mentioning that it is wrong. The DT entries are right and it
>> is working on upstream.
>
> Thanks for clarifying.
>
>>>> [1]: https://patchwork.kernel.org/project/linux-arm-msm/list/?series=803412
>>>
>>> I took a quick look at the series, and it looks like this will
>>> eventually clean things up a lot. We should probably define a generic
>>> order for the interrupts with the sometimes optional SS interrupts last.
>>>
>>> Side note: It looks like the threading in that series is broken.
>>> Consider using git-send-email for sending series as it takes care of
>>> things like that.
>>
>> Usually I do git send-email for the whole out folder where the patches
>> are present, but linux-usb list is common to all the patches in that
>> case, even the DT ones. So to avoid that and to send patches to only
>> relavant mailing lists, I did git send email individually on each patch
>> which might have caused this issue.
>
> I'd suggest that you just send two separate series, one with binding and
> driver updates, which will eventually be merged by Greg, and one with
> the devicetree changes, which goes through Bjorn's tree.
>
> It's good if you could add a link to the binding series in the cover
> letter of the devicetree changes as they are of course going to be quite
> closely related and need to be reviewed in parallel.
>

Thanks for this pointer. So for Multiport, can I do it this way:

1. Core bindings and Core driver changes in one series. Now that we
finalized we don't be adding the ctrl_irq[1] as discussed on:
https://lore.kernel.org/all/[email protected]/.

2. QC bindings and QC driver changes for Multiport to be pushed after we
clean up the current driver and DT's (an effort which is going on
currently).

Regards,
Krishna,

2023-11-24 11:19:46

by Johan Hovold

[permalink] [raw]
Subject: Re: [PATCH v13 05/10] usb: dwc3: qcom: Refactor IRQ handling in QCOM Glue driver

On Fri, Nov 24, 2023 at 04:08:42PM +0530, Krishna Kurapati PSSNV wrote:
> On 11/24/2023 3:43 PM, Johan Hovold wrote:

> > I'd suggest that you just send two separate series, one with binding and
> > driver updates, which will eventually be merged by Greg, and one with
> > the devicetree changes, which goes through Bjorn's tree.
> >
> > It's good if you could add a link to the binding series in the cover
> > letter of the devicetree changes as they are of course going to be quite
> > closely related and need to be reviewed in parallel.
>
> Thanks for this pointer. So for Multiport, can I do it this way:
>
> 1. Core bindings and Core driver changes in one series. Now that we
> finalized we don't be adding the ctrl_irq[1] as discussed on:
> https://lore.kernel.org/all/[email protected]/.
>
> 2. QC bindings and QC driver changes for Multiport to be pushed after we
> clean up the current driver and DT's (an effort which is going on
> currently).

No, I was just referring to how to handle binding/driver vs devicetree
patches for USB where we send them separately (unlike for most other
subsystems).

The dwc3 core and Qualcomm glue parts should still go in the same series
for multiport support.

Whether to do the irq cleanup before or after adding multiport support
is a different question, but, yeah, it is probably best to do it before.

The question of whether we can drop ACPI support should also be
considered as that should also simplify your multiport series.

Johan