mbox series

[v15,0/9] Add multiport support for DWC3 controllers

Message ID 20240216005756.762712-1-quic_kriskura@quicinc.com
Headers show
Series Add multiport support for DWC3 controllers | expand

Message

Krishna Kurapati Feb. 16, 2024, 12:57 a.m. UTC
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 v15:
Added minItems property in qcom,dwc3 bindings as suggested by Rob.
Retained all RB's/ACK's got in v14.

Changes in v14:
Moved wrapper binding update to 5th patch in the series as it deals
with only wakeup and not enumeration. The first part of the series
deals with enumeration and the next part deals with wakeup.
Updated commit text for wrapper driver patches.
Added error checks in get_port_index and setup_irq call which were
missing in v13.
Added SOB and CDB tags appropriately for the patches.
Rebased code on top of latest usb next.
DT changes have been removed and will be sent as a seperate series.

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.

Tested enumeration and wakeup on all ports:

/ # lsusb
Bus 001 Device 001: ID 1d6b:0002
Bus 001 Device 018: ID 0781:5567
Bus 001 Device 020: ID 03f0:134a
Bus 002 Device 002: ID 18d1:4ee1
Bus 002 Device 001: ID 1d6b:0003
/ #
/ # dmesg | grep ports
[    0.224450] hub 1-0:1.0: 4 ports detected
[    0.230479] hub 2-0:1.0: 2 ports detecte/ #
/ #
/ # cat /proc/interrupts  |grep phy
158:          1          0          0          0          0          0          0          0       PDC 127 Edge      dp_hs_phy_1
159:          2          0          0          0          0          0          0          0       PDC 126 Edge      dm_hs_phy_1
160:          6          0          0          0          0          0          0          0       PDC 129 Edge      dp_hs_phy_2
161:          3          0          0          0          0          0          0          0       PDC 128 Edge      dm_hs_phy_2
162:          1          0          0          0          0          0          0          0       PDC 131 Edge      dp_hs_phy_3
163:          2          0          0          0          0          0          0          0       PDC 130 Edge      dm_hs_phy_3
164:          2          0          0          0          0          0          0          0       PDC 133 Edge      dp_hs_phy_4
165:          3          0          0          0          0          0          0          0       PDC 132 Edge      dm_hs_phy_4
166:          0          0          0          0          0          0          0          0       PDC  16 Level     ss_phy_1
167:          0          0          0          0          0          0          0          0       PDC  17 Level     ss_phy_2

Links to previous versions:
Link to v14: https://lore.kernel.org/all/20240206051825.1038685-1-quic_kriskura@quicinc.com/
Link to v13: https://lore.kernel.org/all/20231007154806.605-1-quic_kriskura@quicinc.com/
Link to v12: https://lore.kernel.org/all/20231004165922.25642-1-quic_kriskura@quicinc.com/
Link to v11: https://lore.kernel.org/all/20230828133033.11988-1-quic_kriskura@quicinc.com/
Link to v10: https://lore.kernel.org/all/20230727223307.8096-1-quic_kriskura@quicinc.com/
Link to v9: https://lore.kernel.org/all/20230621043628.21485-1-quic_kriskura@quicinc.com/
Link to v8: https://lore.kernel.org/all/20230514054917.21318-1-quic_kriskura@quicinc.com/
Link to v7: https://lore.kernel.org/all/20230501143445.3851-1-quic_kriskura@quicinc.com/
Link to v6: https://lore.kernel.org/all/20230405125759.4201-1-quic_kriskura@quicinc.com/
Link to v5: https://lore.kernel.org/all/20230310163420.7582-1-quic_kriskura@quicinc.com/
Link to RFC v4: https://lore.kernel.org/all/20230115114146.12628-1-quic_kriskura@quicinc.com/
Link to RFC v3: https://lore.kernel.org/all/1654709787-23686-1-git-send-email-quic_harshq@quicinc.com/#r
Link to RFC v2: https://lore.kernel.org/all/1653560029-6937-1-git-send-email-quic_harshq@quicinc.com/#r

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

Krishna Kurapati (8):
  dt-bindings: usb: Add bindings for multiport properties on DWC3
    controller
  usb: dwc3: core: Access XHCI address space temporarily to read port
    info
  usb: dwc3: core: Skip setting event buffers for host only controllers
  dt-bindings: usb: qcom,dwc3: Add bindings for SC8280 Multiport
  usb: dwc3: qcom: Add helper function to request wakeup interrupts
  usb: dwc3: qcom: Refactor IRQ handling in glue driver
  usb: dwc3: qcom: Enable wakeup for applicable ports of multiport
  usb: dwc3: qcom: Add multiport suspend/resume support for wrapper

 .../devicetree/bindings/usb/qcom,dwc3.yaml    |  34 ++
 .../devicetree/bindings/usb/snps,dwc3.yaml    |  13 +-
 drivers/usb/dwc3/core.c                       | 326 +++++++++++++----
 drivers/usb/dwc3/core.h                       |  19 +-
 drivers/usb/dwc3/drd.c                        |  15 +-
 drivers/usb/dwc3/dwc3-qcom.c                  | 329 ++++++++++++------
 6 files changed, 535 insertions(+), 201 deletions(-)

Comments

Greg Kroah-Hartman Feb. 17, 2024, 5:39 p.m. UTC | #1
On Fri, Feb 16, 2024 at 06:27:47AM +0530, 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've dropped these all from my queue (see the other private email thread
on the commit response).  Please resend when you address the issues
Johan raised.

And note, please do NOT attempt to poke maintainers in private bug
reports to do reviews as that is not how any of this happens, you know
this quite well.

thanks,

greg k-h
Johan Hovold Feb. 29, 2024, 9:47 a.m. UTC | #2
On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote:
> Currently Multiport DWC3 controllers are host-only capable.

I already asked you to rephrase this so that it becomes clear that you
are describing a property of the current hardware (and similar
throughout the series):

	https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/

> +static int dwc3_read_port_info(struct dwc3 *dwc)
> +{
> +	void __iomem *base;
> +	u8 major_revision;
> +	u32 offset;
> +	u32 val;
> +
> +	/*
> +	 * Remap xHCI address space to access XHCI ext cap regs since it is
> +	 * needed to get information on number of ports present.
> +	 */
> +	base = ioremap(dwc->xhci_resources[0].start,
> +		       resource_size(&dwc->xhci_resources[0]));
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	offset = 0;
> +	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_warn(dwc->dev,
> +				 "unrecognized port major revision %d\n",

I still think you should merge this with the previous line even if you
end up with 83 chars.

> +							major_revision);
> +		}
> +	} while (1);
 
> +	/*
> +	 * Currently only DWC3 controllers that are host-only capable
> +	 * support Multiport.
> +	 */

So again, also here, rephrase the comment so that it is clear that you
are referring to a property of the current hardware.

> +	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;
> +	}

Johan
Krishna Kurapati Feb. 29, 2024, 11:53 a.m. UTC | #3
On 2/29/2024 3:17 PM, Johan Hovold wrote:
> On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote:
>> Currently Multiport DWC3 controllers are host-only capable.
> 
> I already asked you to rephrase this so that it becomes clear that you
> are describing a property of the current hardware (and similar
> throughout the series):
> 
> 	https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/

Hi Johan. Thanks for the review.

IMO, the statement is describing a property unique to current hardware, 
that "If it is a multiport controller, it is then host-only capable"

I used the word "Currently" to indicate that "Today, the multiport 
devices present...". Let me know if there is any ambiguity in the sentence.

In v13, I wrote:
"Currently host-only capable DWC3 controllers support Multiport."
You were right. It was ambiguous as it might refer to even single port 
controllers.

So I changed it saying all the DWC3 multiport controllers are host only 
capable.

How about:

"All the DWC3 Multi Port controllers that exist today only support host 
mode"

> 
>> +static int dwc3_read_port_info(struct dwc3 *dwc)
>> +{
>> +	void __iomem *base;
>> +	u8 major_revision;
>> +	u32 offset;
>> +	u32 val;
>> +
>> +	/*
>> +	 * Remap xHCI address space to access XHCI ext cap regs since it is
>> +	 * needed to get information on number of ports present.
>> +	 */
>> +	base = ioremap(dwc->xhci_resources[0].start,
>> +		       resource_size(&dwc->xhci_resources[0]));
>> +	if (IS_ERR(base))
>> +		return PTR_ERR(base);
>> +
>> +	offset = 0;
>> +	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_warn(dwc->dev,
>> +				 "unrecognized port major revision %d\n",
> 
> I still think you should merge this with the previous line even if you
> end up with 83 chars.
> 
>> +							major_revision);
>> +		}
>> +	} while (1);
>   
>> +	/*
>> +	 * Currently only DWC3 controllers that are host-only capable
>> +	 * support Multiport.
>> +	 */
> 
> So again, also here, rephrase the comment so that it is clear that you
> are referring to a property of the current hardware.

I put the comment this way to indicate that we don't want to check for 
existence of multiple ports if the controller is not "host-only" 
capable. We should only check for multport support only if we are 
host-only capable. I think the statement clearly tells that "check for 
host-only" configuration before proceeding to check for xhci register reads.

I replied the same on:
https://lore.kernel.org/all/279a54f2-7260-4270-83c7-d6f5c5ba0873@quicinc.com/

And since you didn't mention anything else at this part of code in your 
return reply in:
https://lore.kernel.org/all/ZTYyXhyZN3jBXEfm@hovoldconsulting.com/

I thought this statement was fine to go.

> 
>> +	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;
>> +	}
> 

Thanks for the review. Can you help let me know your review on the other 
patches as well.

Regards,
Krishna,
Johan Hovold March 1, 2024, 1:50 p.m. UTC | #4
On Fri, Feb 16, 2024 at 06:27:51AM +0530, Krishna Kurapati wrote:

> @@ -1398,22 +1464,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-%d", i);
> +
> +		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);

Please move the format string to the previous line as I already asked
you to do (e.g. as continuation lines should be substantially shorter).

> +		}
>  
> -	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;
> +		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-%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;

Johan
Johan Hovold March 1, 2024, 1:53 p.m. UTC | #5
On Fri, Feb 16, 2024 at 06:27:54AM +0530, Krishna Kurapati wrote:
> On multiport supported controllers, each port has its own DP/DM
> and SS (if super speed capable) interrupts. As per the bindings,
> their interrupt names differ from standard ones having "_x" added
> as suffix (x indicates port number). Refactor dwc3_qcom_setup_irq()
> call to parse multiport interrupts along with non-multiport ones.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 222 +++++++++++++++++++++++++----------
>  1 file changed, 161 insertions(+), 61 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 08df29584366..a20d63a791bd 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -53,17 +53,33 @@
>  #define APPS_USB_AVG_BW 0
>  #define APPS_USB_PEAK_BW MBps_to_icc(40)
>  
> +#define NUM_PHY_IRQ		4
> +
> +enum dwc3_qcom_phy_index {
> +	DP_HS_PHY_IRQ_INDEX,
> +	DM_HS_PHY_IRQ_INDEX,
> +	SS_PHY_IRQ_INDEX,
> +	QUSB2_PHY_IRQ_INDEX,
> +};
> +
>  struct dwc3_acpi_pdata {
>  	u32			qscratch_base_offset;
>  	u32			qscratch_base_size;
>  	u32			dwc3_core_base_size;
> -	int			qusb2_phy_irq_index;
> -	int			dp_hs_phy_irq_index;
> -	int			dm_hs_phy_irq_index;
> -	int			ss_phy_irq_index;
> +	/*
> +	 * The phy_irq_index corresponds to ACPI indexes of (in order)
> +	 * DP/DM/SS/QUSB2 IRQ's respectively.
> +	 */
> +	int			phy_irq_index[NUM_PHY_IRQ];
>  	bool			is_urs;
>  };

I asked you to add a port structure and get rid of the PHY indexes in
v13, and so you did for the diver data below, but you still have an
array of indexes here for the ACPI data. 

I don't think ever got around to actually reviewing the ACPI hack (and
maybe I was hoping that we'd be able to drop ACPI support before merging
multi-port support), but removing these fields and replacing them with
an array is a step in the wrong direction (e.g. making the code harder
to read).

Why can't you just add a helper function which returns one of these
fields based on the interrupt name string?

> +struct dwc3_qcom_port {
> +	int			dp_hs_phy_irq;
> +	int			dm_hs_phy_irq;
> +	int			ss_phy_irq;
> +};

And as I've explicitly said before, you should include hs_phy_irq here.

It's a port interrupt and special casing just this one make no sense at
all even if there are no multi-port controller that use it.

> +
>  struct dwc3_qcom {
>  	struct device		*dev;
>  	void __iomem		*qscratch_base;
> @@ -74,9 +90,7 @@ struct dwc3_qcom {
>  	struct reset_control	*resets;
>  
>  	int			qusb2_phy_irq;
> -	int			dp_hs_phy_irq;
> -	int			dm_hs_phy_irq;
> -	int			ss_phy_irq;
> +	struct dwc3_qcom_port	port_info[DWC3_MAX_PORTS];

Just name the array 'ports' as I already suggested. It's more succinct
and makes the code that uses it easier to read.

>  	enum usb_device_speed	usb2_speed;
>  
>  	struct extcon_dev	*edev;
> @@ -91,6 +105,7 @@ struct dwc3_qcom {
>  	bool			pm_suspended;
>  	struct icc_path		*icc_path_ddr;
>  	struct icc_path		*icc_path_apps;
> +	u8			num_ports;

Any reason not to keep this one closer to the ports array?

>  };
 
> +static int dwc3_qcom_get_irq_index(const char *irq_name)
> +{
> +	/*
> +	 * Parse IRQ index based on prefixes from interrupt name.
> +	 * Return -1 incase of an invalid interrupt name.
> +	 */
> +	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, "qusb2_phy", strlen("qusb2_phy")) == 0)
> +		irq_index = QUSB2_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 QUSB2_PHY_IRQ_INDEX:
> +		port_index = 1;
> +		break;
> +	}
> +
> +	if (port_index <= 0 || 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;
> +
> +	/*
> +	 * 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 || port_index != 1)
> +		return -1;
> +
> +	return pdata->phy_irq_index[irq_index];
> +}

Yeah, you need to come some better solution than the above, which is
just unnecessarily convoluted.

>  static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
>  				 const char *name)
>  {
> @@ -554,44 +637,67 @@ static int dwc3_qcom_request_irq(struct dwc3_qcom *qcom, int irq,
>  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, "qusb2_phy",
> -				pdata ? pdata->qusb2_phy_irq_index : -1);
> -	if (irq > 0) {
> -		ret = dwc3_qcom_request_irq(qcom, irq, "hs_phy_irq");
> -		if (ret)
> -			return ret;
> -		qcom->qusb2_phy_irq = irq;
> -	}
> +	irq_count = of_property_count_strings(np, "interrupt-names");
> +	if (irq_count < 0)
> +		return -EINVAL;
>  
> -	irq = dwc3_qcom_get_irq(pdev, "dp_hs_phy_irq",
> -				pdata ? pdata->dp_hs_phy_irq_index : -1);
> -	if (irq > 0) {
> -		ret = dwc3_qcom_request_irq(qcom, irq, "dp_hs_phy_irq");
> -		if (ret)
> -			return ret;
> -		qcom->dp_hs_phy_irq = irq;
> -	}
> +	irq_names = devm_kcalloc(&pdev->dev, irq_count, sizeof(*irq_names), GFP_KERNEL);
> +	if (!irq_names)
> +		return -ENOMEM;
>  
> -	irq = dwc3_qcom_get_irq(pdev, "dm_hs_phy_irq",
> -				pdata ? pdata->dm_hs_phy_irq_index : -1);
> -	if (irq > 0) {
> -		ret = dwc3_qcom_request_irq(qcom, irq, "dm_hs_phy_irq");
> -		if (ret)
> -			return ret;
> -		qcom->dm_hs_phy_irq = irq;
> -	}
> +	ret = of_property_read_string_array(np, "interrupt-names",
> +					    irq_names, irq_count);
> +	if (!ret)
> +		return ret;
>  
> -	irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
> -				pdata ? pdata->ss_phy_irq_index : -1);
> -	if (irq > 0) {
> -		ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
> -		if (ret)
> -			return ret;
> -		qcom->ss_phy_irq = irq;
> +	for (i = 0; i < irq_count; i++) {
> +		irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
> +		if (irq_index == -1) {
> +			dev_err(&pdev->dev, "Unknown interrupt-name \"%s\" found\n", irq_names[i]);

This is now spamming the logs with errors like

	dwc3-qcom a6f8800.usb: Unknown interrupt-name "pwr_event" found

which is clearly just broken.

> +			continue;
> +		}
> +		port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index);
> +		if (port_index == -1) {
> +			dev_err(&pdev->dev, "Invalid interrupt-name suffix \"%s\"\n", irq_names[i]);
> +			continue;
> +		}
> +
> +		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_request_irq(qcom, irq, irq_names[i]);
> +			if (ret)
> +				return ret;
> +
> +			switch (irq_index) {
> +			case DP_HS_PHY_IRQ_INDEX:
> +				qcom->port_info[port_index - 1].dp_hs_phy_irq = irq;
> +				break;
> +			case DM_HS_PHY_IRQ_INDEX:
> +				qcom->port_info[port_index - 1].dm_hs_phy_irq = irq;
> +				break;
> +			case SS_PHY_IRQ_INDEX:
> +				qcom->port_info[port_index - 1].ss_phy_irq = irq;
> +				break;
> +			case QUSB2_PHY_IRQ_INDEX:
> +				qcom->qusb2_phy_irq = irq;
> +				break;
> +			}
> +
> +			if (qcom->num_ports < port_index)
> +				qcom->num_ports = port_index;
> +		}
>  	}

Why don't you add a port helper for fetching the interrupts instead?

There are multiple ways that you can use to determine if this is a
multiport controller or not; you can use OF match data, or simply look
at one of the interrupts that would always be there for a multiport
(or single port) controller (e.g. "dp_hs_phy_1").

You can even determine the number of ports first by parsing the
interrupts names and looking for the highest port number.

Then you can iterate over the ports and parse the interrupts for each
port in turn, which should allow for a much cleaner and less
error-prone implementation.

Johan
Johan Hovold March 1, 2024, 3:43 p.m. UTC | #6
On Fri, Feb 16, 2024 at 06:27:55AM +0530, Krishna Kurapati wrote:
> DWC3 Qcom wrapper currently supports only wakeup configuration
> for single port controllers. Read speed of each port connected
> to the controller and enable wakeup for each of them accordingly.
> 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 72 ++++++++++++++++++------------------
>  1 file changed, 37 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index a20d63a791bd..572dc3fdae12 100644
> --- a/drivers/usb/dwc3/dwc3-qcom.c
> +++ b/drivers/usb/dwc3/dwc3-qcom.c
> @@ -78,6 +78,7 @@ struct dwc3_qcom_port {
>  	int			dp_hs_phy_irq;
>  	int			dm_hs_phy_irq;
>  	int			ss_phy_irq;
> +	enum usb_device_speed	usb2_speed;

You need to remove the corresponding, and now unused, field from struct
dwc3_qcom as well.

>  };
>  
>  struct dwc3_qcom {
> @@ -336,7 +337,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)

As I mentioned, there's no need for a line break after the first
parameter as this is a function definition (e.g. Linus as expressed a
preference for this as it makes functions easier to grep for).

>  {
>  	struct dwc3 *dwc = platform_get_drvdata(qcom->dwc3);
>  	struct usb_device *udev;
> @@ -347,14 +349,8 @@ static enum usb_device_speed dwc3_qcom_read_usb2_speed(struct dwc3_qcom *qcom)
>  	 */
>  	hcd = platform_get_drvdata(dwc->xhci);
>  
> -	/*
> -	 * 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.
> -	 */
>  #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
> @@ -387,23 +383,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->qusb2_phy_irq);
>  
> -	if (qcom->usb2_speed == USB_SPEED_LOW) {
> -		dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq);
> -	} else if ((qcom->usb2_speed == USB_SPEED_HIGH) ||
> -			(qcom->usb2_speed == USB_SPEED_FULL)) {
> -		dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq);
> -	} else {
> -		dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dp_hs_phy_irq);
> -		dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].dm_hs_phy_irq);
> -	}
> +	for (i = 0; i < qcom->num_ports; i++) {
> +		if (qcom->port_info[i].usb2_speed == USB_SPEED_LOW) {
> +			dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq);
> +		} else if ((qcom->port_info[i].usb2_speed == USB_SPEED_HIGH) ||
> +				(qcom->port_info[i].usb2_speed == USB_SPEED_FULL)) {
> +			dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq);
> +		} else {
> +			dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dp_hs_phy_irq);
> +			dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].dm_hs_phy_irq);
> +		}
>  
> -	dwc3_qcom_disable_wakeup_irq(qcom->port_info[0].ss_phy_irq);
> +		dwc3_qcom_disable_wakeup_irq(qcom->port_info[i].ss_phy_irq);
> +	}

As I already commented on v13, this should be a per-port helper rather
than special casing qusb2_phy_irq and a for loop for the other
interrupts:

	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.

>  }
 
>  static int dwc3_qcom_suspend(struct dwc3_qcom *qcom, bool wakeup)
> @@ -455,10 +459,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);

And again, as I said for v13:

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

rather than hide it away in an odd for loop in
dwc3_qcom_enable_interrupts().

> +	if (dwc3_qcom_is_host(qcom) && wakeup)
>  		dwc3_qcom_enable_interrupts(qcom);
> -	}
>  
>  	qcom->is_suspended = true;

Johan
Johan Hovold March 1, 2024, 3:55 p.m. UTC | #7
On Fri, Feb 16, 2024 at 06:27:56AM +0530, Krishna Kurapati wrote:
> Power event IRQ stat registers are present for each port
> connected to controller. Add support for modifying all power event
> irq stat registers present in wrapper.

Could you please say about what the power-event irqs are used for here
in the commit message as I asked you before?
 
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> Reviewed-by: Bjorn Andersson <andersson@kernel.org>
> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> ---
>  drivers/usb/dwc3/dwc3-qcom.c | 30 +++++++++++++++++++++++-------
>  1 file changed, 23 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-qcom.c b/drivers/usb/dwc3/dwc3-qcom.c
> index 572dc3fdae12..e789745a9468 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

Again, not sure it makes any defines too keep these defines when you
only access them through the array.

> +
>  #define PWR_EVNT_LPM_IN_L2_MASK			BIT(4)
>  #define PWR_EVNT_LPM_OUT_L2_MASK		BIT(5)
>  
> @@ -109,6 +113,13 @@ struct dwc3_qcom {
>  	u8			num_ports;
>  };
>  
> +static const u32 pwr_evnt_irq_stat_reg_offset[DWC3_MAX_PORTS] = {

Seems "_offset" is redundant here, 'pwr_evnt_irq_stat_reg' should be
enough.

> +	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;
> @@ -444,9 +455,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, "Port-%d HS-PHY not in L2\n", i + 1);

Please use lower case "port-%d" for consistency.

Johan
Johan Hovold March 4, 2024, 8:40 a.m. UTC | #8
On Thu, Feb 29, 2024 at 05:23:08PM +0530, Krishna Kurapati PSSNV wrote:
> On 2/29/2024 3:17 PM, Johan Hovold wrote:
> > On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote:
> >> Currently Multiport DWC3 controllers are host-only capable.
> > 
> > I already asked you to rephrase this so that it becomes clear that you
> > are describing a property of the current hardware (and similar
> > throughout the series):
> > 
> > 	https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/

> IMO, the statement is describing a property unique to current hardware, 
> that "If it is a multiport controller, it is then host-only capable"
> 
> I used the word "Currently" to indicate that "Today, the multiport 
> devices present...". Let me know if there is any ambiguity in the sentence.
> 
> In v13, I wrote:
> "Currently host-only capable DWC3 controllers support Multiport."
> You were right. It was ambiguous as it might refer to even single port 
> controllers.
> 
> So I changed it saying all the DWC3 multiport controllers are host only 
> capable.
> 
> How about:
> 
> "All the DWC3 Multi Port controllers that exist today only support host 
> mode"

That should be clear enough, thanks.

> >> +	/*
> >> +	 * Currently only DWC3 controllers that are host-only capable
> >> +	 * support Multiport.
> >> +	 */
> > 
> > So again, also here, rephrase the comment so that it is clear that you
> > are referring to a property of the current hardware.
> 
> I put the comment this way to indicate that we don't want to check for 
> existence of multiple ports if the controller is not "host-only" 
> capable. We should only check for multport support only if we are 
> host-only capable. I think the statement clearly tells that "check for 
> host-only" configuration before proceeding to check for xhci register reads.

Fair enough, this comment could be considered to apply only to the
implementation. Perhaps the following would be more clear though:

	Currently only DWC3 controllers that are host-only capable
	can have more than one port.

or simply

	Host-only capable controllers can have more than one port.

Both of these also gives a hint that this is a property of the hardware.

> I replied the same on:
> https://lore.kernel.org/all/279a54f2-7260-4270-83c7-d6f5c5ba0873@quicinc.com/
> 
> And since you didn't mention anything else at this part of code in your 
> return reply in:
> https://lore.kernel.org/all/ZTYyXhyZN3jBXEfm@hovoldconsulting.com/

I left in the following quote on purpose in that reply:

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

Johan
Krishna Kurapati March 5, 2024, 3:39 p.m. UTC | #9
On 3/1/2024 7:23 PM, Johan Hovold wrote:

[...]

>> +
>>   struct dwc3_acpi_pdata {
>>   	u32			qscratch_base_offset;
>>   	u32			qscratch_base_size;
>>   	u32			dwc3_core_base_size;
>> -	int			qusb2_phy_irq_index;
>> -	int			dp_hs_phy_irq_index;
>> -	int			dm_hs_phy_irq_index;
>> -	int			ss_phy_irq_index;
>> +	/*
>> +	 * The phy_irq_index corresponds to ACPI indexes of (in order)
>> +	 * DP/DM/SS/QUSB2 IRQ's respectively.
>> +	 */
>> +	int			phy_irq_index[NUM_PHY_IRQ];
>>   	bool			is_urs;
>>   };
> 
> I asked you to add a port structure and get rid of the PHY indexes in
> v13, and so you did for the diver data below, but you still have an
> array of indexes here for the ACPI data.
> 
> I don't think ever got around to actually reviewing the ACPI hack (and
> maybe I was hoping that we'd be able to drop ACPI support before merging
> multi-port support), but removing these fields and replacing them with
> an array is a step in the wrong direction (e.g. making the code harder
> to read).
> 
> Why can't you just add a helper function which returns one of these
> fields based on the interrupt name string?

I think since [1] has been accepted, this comment has been taken care of.

> 
>> +struct dwc3_qcom_port {
>> +	int			dp_hs_phy_irq;
>> +	int			dm_hs_phy_irq;
>> +	int			ss_phy_irq;
>> +};
> 
> And as I've explicitly said before, you should include hs_phy_irq here.
> 
> It's a port interrupt and special casing just this one make no sense at
> all even if there are no multi-port controller that use it.
> 

Okay. Will add it to port structure.
I only kept it outside because there are no real devices which has 
multiple ports and qusb2_phy_irq in them.

>> +
>>   struct dwc3_qcom {
>>   	struct device		*dev;
>>   	void __iomem		*qscratch_base;
>> @@ -74,9 +90,7 @@ struct dwc3_qcom {
>>   	struct reset_control	*resets;
>>   
>>   	int			qusb2_phy_irq;
>> -	int			dp_hs_phy_irq;
>> -	int			dm_hs_phy_irq;
>> -	int			ss_phy_irq;
>> +	struct dwc3_qcom_port	port_info[DWC3_MAX_PORTS];
> 
> Just name the array 'ports' as I already suggested. It's more succinct
> and makes the code that uses it easier to read.
> 
>>   	enum usb_device_speed	usb2_speed;
>>   
>>   	struct extcon_dev	*edev;
>> @@ -91,6 +105,7 @@ struct dwc3_qcom {
>>   	bool			pm_suspended;
>>   	struct icc_path		*icc_path_ddr;
>>   	struct icc_path		*icc_path_apps;
>> +	u8			num_ports;
> 
> Any reason not to keep this one closer to the ports array?
> 
>>   };
>   

[...]

>> -	irq = dwc3_qcom_get_irq(pdev, "ss_phy_irq",
>> -				pdata ? pdata->ss_phy_irq_index : -1);
>> -	if (irq > 0) {
>> -		ret = dwc3_qcom_request_irq(qcom, irq, "ss_phy_irq");
>> -		if (ret)
>> -			return ret;
>> -		qcom->ss_phy_irq = irq;
>> +	for (i = 0; i < irq_count; i++) {
>> +		irq_index = dwc3_qcom_get_irq_index(irq_names[i]);
>> +		if (irq_index == -1) {
>> +			dev_err(&pdev->dev, "Unknown interrupt-name \"%s\" found\n", irq_names[i]);
> 
> This is now spamming the logs with errors like
> 
> 	dwc3-qcom a6f8800.usb: Unknown interrupt-name "pwr_event" found
> 
> which is clearly just broken.
> 
>> +			continue;
>> +		}
>> +		port_index = dwc3_qcom_get_port_index(irq_names[i], irq_index);
>> +		if (port_index == -1) {
>> +			dev_err(&pdev->dev, "Invalid interrupt-name suffix \"%s\"\n", irq_names[i]);
>> +			continue;
>> +		}
>> +
>> +		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_request_irq(qcom, irq, irq_names[i]);
>> +			if (ret)
>> +				return ret;
>> +
>> +			switch (irq_index) {
>> +			case DP_HS_PHY_IRQ_INDEX:
>> +				qcom->port_info[port_index - 1].dp_hs_phy_irq = irq;
>> +				break;
>> +			case DM_HS_PHY_IRQ_INDEX:
>> +				qcom->port_info[port_index - 1].dm_hs_phy_irq = irq;
>> +				break;
>> +			case SS_PHY_IRQ_INDEX:
>> +				qcom->port_info[port_index - 1].ss_phy_irq = irq;
>> +				break;
>> +			case QUSB2_PHY_IRQ_INDEX:
>> +				qcom->qusb2_phy_irq = irq;
>> +				break;
>> +			}
>> +
>> +			if (qcom->num_ports < port_index)
>> +				qcom->num_ports = port_index;
>> +		}
>>   	}
> 
> Why don't you add a port helper for fetching the interrupts instead?
> 
> There are multiple ways that you can use to determine if this is a
> multiport controller or not; you can use OF match data, or simply look
> at one of the interrupts that would always be there for a multiport
> (or single port) controller (e.g. "dp_hs_phy_1").
> 
> You can even determine the number of ports first by parsing the
> interrupts names and looking for the highest port number.
> 
> Then you can iterate over the ports and parse the interrupts for each
> port in turn, which should allow for a much cleaner and less
> error-prone implementation.
> 

With [1] merged, I think I can use your suggestion of going through and 
checking if dp_hs_phy_X is present or not and if present, it is 
multiport and go through dp_hs_phy_{1/2/3/4} to see the num of ports 
present.

That must simplify the code and make it clean as well.

[1]: 
https://lore.kernel.org/all/20240305093216.3814787-1-quic_kriskura@quicinc.com/

Regards,
Krishna,