[RFC,v4,2/5] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

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.

Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
---
 drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
 drivers/usb/dwc3/core.h |  15 +-
 drivers/usb/dwc3/drd.c  |  14 +-
 3 files changed, 244 insertions(+), 89 deletions(-)

Hi,

On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> Currently the DWC3 driver supports only single port controller
> which requires at most one HS and one SS PHY.

Add note here that multi-port is for host mode for clarity.

> 
> 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.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h |  15 +-
>  drivers/usb/dwc3/drd.c  |  14 +-
>  3 files changed, 244 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..7e0a9a598dfd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>  {
>  	struct dwc3 *dwc = work_to_dwc(work);
>  	unsigned long flags;
> -	int ret;
> +	int ret, i;

Can we declare variables in separate lines here and other places.

>  	u32 reg;
>  	u32 desired_dr_role;
>  
> @@ -200,8 +200,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_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;
> @@ -216,8 +218,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)
> @@ -659,22 +661,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
> @@ -729,9 +723,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)) {
> @@ -743,7 +747,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))
> @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>  	if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel)
>  		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>  
> -	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_ss_ports; i++) {
> +		ret = dwc3_ss_phy_setup(dwc, i);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = dwc3_hs_phy_setup(dwc, i);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	return 0;
>  }
> @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>  
>  static void dwc3_core_exit(struct dwc3 *dwc)
>  {
> +	int i;
> +
>  	dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -	phy_power_off(dwc->usb2_generic_phy);
> -	phy_power_off(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_power_off(dwc->usb2_generic_phy[i]);
> +		phy_power_off(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	dwc3_clk_disable(dwc);
>  	reset_control_assert(dwc->reset);
> @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	unsigned int		hw_mode;
>  	u32			reg;
>  	int			ret;
> +	int			i, j;
>  
>  	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>  
> @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	usb_phy_init(dwc->usb2_phy);
>  	usb_phy_init(dwc->usb3_phy);
> -	ret = phy_init(dwc->usb2_generic_phy);
> -	if (ret < 0)
> -		goto err0a;
>  
> -	ret = phy_init(dwc->usb3_generic_phy);
> -	if (ret < 0) {
> -		phy_exit(dwc->usb2_generic_phy);
> -		goto err0a;
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_init(dwc->usb2_generic_phy[i]);
> +		if (ret < 0) {
> +			/* clean up prior initialized HS PHYs */
> +			for (j = 0; j < i; j++)
> +				phy_exit(dwc->usb2_generic_phy[j]);
> +			goto err0a;
> +		}
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_init(dwc->usb3_generic_phy[i]);
> +		if (ret < 0) {
> +			/* clean up prior initialized SS PHYs */
> +			for (j = 0; j < i; j++)
> +				phy_exit(dwc->usb3_generic_phy[j]);
> +			for (j = 0; j < dwc->num_ports; j++)
> +				phy_exit(dwc->usb2_generic_phy[j]);
> +			goto err0a;
> +		}
>  	}
>  
>  	ret = dwc3_core_soft_reset(dwc);
> @@ -1136,15 +1190,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_ss_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_ports; i++) {
> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
> +				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
> +			}
>  		}
>  	}
>  
> @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  
>  	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 err2;
>  
> -	ret = phy_power_on(dwc->usb3_generic_phy);
> -	if (ret < 0)
> -		goto err3;
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_power_on(dwc->usb2_generic_phy[i]);
> +		if (ret < 0) {
> +			for (j = 0; j < i; j++)
> +				phy_power_off(dwc->usb2_generic_phy[j]);
> +			goto err2;
> +		}
> +	}
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		ret = phy_power_on(dwc->usb3_generic_phy[i]);
> +		if (ret < 0) {
> +			for (j = 0; j < i; j++)
> +				phy_power_off(dwc->usb3_generic_phy[j]);
> +			goto err3;
> +		}
> +	}
>  
>  	ret = dwc3_event_buffers_setup(dwc);
>  	if (ret) {
> @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return 0;
>  
>  err4:
> -	phy_power_off(dwc->usb3_generic_phy);
> +	for (i = 0; i < dwc->num_ports; i++)
> +		phy_power_off(dwc->usb3_generic_phy[i]);
>  
>  err3:
> -	phy_power_off(dwc->usb2_generic_phy);
> +	for (i = 0; i < dwc->num_ports; i++)
> +		phy_power_off(dwc->usb2_generic_phy[i]);
>  
>  err2:
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
> @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  err1:
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  err0a:
>  	dwc3_ulpi_exit(dwc);
> @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  	return ret;
>  }
>  
> +static int dwc3_get_multiport_phys(struct dwc3 *dwc)
> +{
> +	int ret;
> +	struct device *dev = dwc->dev;
> +	int i;
> +	char phy_name[15];
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		sprintf(phy_name, "usb2-phy_port%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, "usb2 phy: %s not configured\n", phy_name);
> +		}
> +
> +		sprintf(phy_name, "usb3-phy_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, "usb3 phy: %s not configured\n", phy_name);
> +		}
> +	}
> +
> +	return 0;
> +}
> +
>  static int dwc3_core_get_phy(struct dwc3 *dwc)
>  {
>  	struct device		*dev = dwc->dev;
> @@ -1349,31 +1456,37 @@ 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 (dwc->num_ports > 1)
> +		goto get_multiport_phys;

Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here?
It's easier to read IMO.

> +
> +	dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy");
> +	if (IS_ERR(dwc->usb2_generic_phy[0])) {
> +		ret = PTR_ERR(dwc->usb2_generic_phy[0]);
>  		if (ret == -ENOSYS || ret == -ENODEV)
> -			dwc->usb2_generic_phy = NULL;
> +			dwc->usb2_generic_phy[0] = NULL;
>  		else
>  			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>  	}
>  
> -	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
> -	if (IS_ERR(dwc->usb3_generic_phy)) {
> -		ret = PTR_ERR(dwc->usb3_generic_phy);
> +	dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy");
> +	if (IS_ERR(dwc->usb3_generic_phy[0])) {
> +		ret = PTR_ERR(dwc->usb3_generic_phy[0]);
>  		if (ret == -ENOSYS || ret == -ENODEV)
> -			dwc->usb3_generic_phy = NULL;
> +			dwc->usb3_generic_phy[0] = NULL;
>  		else
>  			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>  	}
>  
>  	return 0;
> +
> +get_multiport_phys:
> +	return dwc3_get_multiport_phys(dwc);
>  }
>  
>  static int dwc3_core_init_mode(struct dwc3 *dwc)
>  {
>  	struct device *dev = dwc->dev;
> -	int ret;
> +	int ret, i;
>  
>  	switch (dwc->dr_mode) {
>  	case USB_DR_MODE_PERIPHERAL:
> @@ -1381,8 +1494,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)
> @@ -1393,8 +1506,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_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)
> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +
> +	/*
> +	 * If no mulitport properties are defined, default

multi*

> +	 * the port count to '1'.
> +	 */

Can we initialize num_ports and num_ss_ports to 1 instead of setting it
on error (similar to how we handle other properties).

> +	ret = device_property_read_u32(dev, "num-ports",
> +				&dwc->num_ports);
> +	if (ret)
> +		dwc->num_ports = 1;
> +
> +	ret = device_property_read_u32(dev, "num-ss-ports",
> +				&dwc->num_ss_ports);
> +	if (ret)
> +		dwc->num_ss_ports = 1;
> +
>  	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>  	dwc->tx_de_emphasis = tx_de_emphasis;
>  
> @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev)
>  	struct device		*dev = &pdev->dev;
>  	struct resource		*res, dwc_res;
>  	struct dwc3		*dwc;
> -
> -	int			ret;
> +	int			ret, i;
>  
>  	void __iomem		*regs;
>  
> @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev)
>  
>  err5:
>  	dwc3_debugfs_exit(dwc);
> +
>  	dwc3_event_buffers_cleanup(dwc);
>  
>  	usb_phy_set_suspend(dwc->usb2_phy, 1);
>  	usb_phy_set_suspend(dwc->usb3_phy, 1);
> -	phy_power_off(dwc->usb2_generic_phy);
> -	phy_power_off(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_power_off(dwc->usb2_generic_phy[i]);
> +		phy_power_off(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	usb_phy_shutdown(dwc->usb2_phy);
>  	usb_phy_shutdown(dwc->usb3_phy);
> -	phy_exit(dwc->usb2_generic_phy);
> -	phy_exit(dwc->usb3_generic_phy);
> +
> +	for (i = 0; i < dwc->num_ports; i++) {
> +		phy_exit(dwc->usb2_generic_phy[i]);
> +		phy_exit(dwc->usb3_generic_phy[i]);
> +	}
>  
>  	dwc3_ulpi_exit(dwc);
>  
> @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>  
>  static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>  {
> +	int i;
>  	unsigned long	flags;
>  	u32 reg;
>  
> @@ -2045,17 +2182,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_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_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 */
> @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>  {
>  	unsigned long	flags;
>  	int		ret;
> +	int		i;
>  	u32		reg;
>  
>  	switch (dwc->current_dr_role) {
> @@ -2104,17 +2246,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_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_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 8f9959ba9fd4..2f82eda9d44f 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -35,6 +35,9 @@
>  
>  #define DWC3_MSG_MAX	500
>  
> +/* Number of ports supported by a multiport controller */
> +#define MAX_PORTS_SUPPORTED	4
> +
>  /* Global constants */
>  #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>  #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
> @@ -1023,8 +1026,10 @@ 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
> + * @num_ports: Indicates number of usb ports supported by the controller.

Note that this is the total number of usb ports including the SS capable
ones.

> + * @num_ss_ports: Indicates number of ss capable ports supported by controller
> + * @usb2_generic_phy: pointer to array of USB2 PHY's
> + * @usb3_generic_phy: pointer to array of USB3 PHY's
>   * @phys_ready: flag to indicate that PHYs are ready
>   * @ulpi: pointer to ulpi interface
>   * @ulpi_ready: flag to indicate that ULPI is initialized
> @@ -1157,8 +1162,10 @@ struct dwc3 {
>  	struct usb_phy		*usb2_phy;
>  	struct usb_phy		*usb3_phy;
>  
> -	struct phy		*usb2_generic_phy;
> -	struct phy		*usb3_generic_phy;
> +	u32			num_ports;
> +	u32			num_ss_ports;
> +	struct phy		*usb2_generic_phy[MAX_PORTS_SUPPORTED];
> +	struct phy		*usb3_generic_phy[MAX_PORTS_SUPPORTED];
>  
>  	bool			phys_ready;
>  
> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
> index 039bf241769a..ea86ff01433b 100644
> --- a/drivers/usb/dwc3/drd.c
> +++ b/drivers/usb/dwc3/drd.c
> @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
>  
>  void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>  {
> -	int ret;
> +	int ret, i;
>  	u32 reg;
>  	int id;
>  	unsigned long flags;
> @@ -386,9 +386,11 @@ 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_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,8 +402,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,
> +		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)
> -- 
> 2.39.0
> 

No major issue I see here. Just some minor nits. Once you feel it's
ready, please drop the RFC tags on resubmission.

Thanks,
Thinh

On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> Hi,
> 
> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
> 
> Add note here that multi-port is for host mode for clarity.
> 
>>
>> 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.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>   drivers/usb/dwc3/core.h |  15 +-
>>   drivers/usb/dwc3/drd.c  |  14 +-
>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..7e0a9a598dfd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>   {
>>   	struct dwc3 *dwc = work_to_dwc(work);
>>   	unsigned long flags;
>> -	int ret;
>> +	int ret, i;
> 
> Can we declare variables in separate lines here and other places.
> 
>>   	u32 reg;
>>   	u32 desired_dr_role;
>>   
>> @@ -200,8 +200,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_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;
>> @@ -216,8 +218,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)
>> @@ -659,22 +661,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
>> @@ -729,9 +723,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)) {
>> @@ -743,7 +747,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))
>> @@ -800,7 +804,35 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   	if (dwc->dis_u2_freeclk_exists_quirk || dwc->gfladj_refclk_lpm_sel)
>>   		reg &= ~DWC3_GUSB2PHYCFG_U2_FREECLK_EXISTS;
>>   
>> -	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_ss_ports; i++) {
>> +		ret = dwc3_ss_phy_setup(dwc, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = dwc3_hs_phy_setup(dwc, i);
>> +		if (ret)
>> +			return ret;
>> +	}
>>   
>>   	return 0;
>>   }
>> @@ -839,17 +871,25 @@ static void dwc3_clk_disable(struct dwc3 *dwc)
>>   
>>   static void dwc3_core_exit(struct dwc3 *dwc)
>>   {
>> +	int i;
>> +
>>   	dwc3_event_buffers_cleanup(dwc);
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 1);
>> -	phy_power_off(dwc->usb2_generic_phy);
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	dwc3_clk_disable(dwc);
>>   	reset_control_assert(dwc->reset);
>> @@ -1085,6 +1125,7 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	unsigned int		hw_mode;
>>   	u32			reg;
>>   	int			ret;
>> +	int			i, j;
>>   
>>   	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
>>   
>> @@ -1119,14 +1160,27 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	usb_phy_init(dwc->usb2_phy);
>>   	usb_phy_init(dwc->usb3_phy);
>> -	ret = phy_init(dwc->usb2_generic_phy);
>> -	if (ret < 0)
>> -		goto err0a;
>>   
>> -	ret = phy_init(dwc->usb3_generic_phy);
>> -	if (ret < 0) {
>> -		phy_exit(dwc->usb2_generic_phy);
>> -		goto err0a;
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_init(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0) {
>> +			/* clean up prior initialized HS PHYs */
>> +			for (j = 0; j < i; j++)
>> +				phy_exit(dwc->usb2_generic_phy[j]);
>> +			goto err0a;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_init(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0) {
>> +			/* clean up prior initialized SS PHYs */
>> +			for (j = 0; j < i; j++)
>> +				phy_exit(dwc->usb3_generic_phy[j]);
>> +			for (j = 0; j < dwc->num_ports; j++)
>> +				phy_exit(dwc->usb2_generic_phy[j]);
>> +			goto err0a;
>> +		}
>>   	}
>>   
>>   	ret = dwc3_core_soft_reset(dwc);
>> @@ -1136,15 +1190,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_ss_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_ports; i++) {
>> +				reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(i));
>> +				reg |= DWC3_GUSB2PHYCFG_SUSPHY;
>> +				dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(i), reg);
>> +			}
>>   		}
>>   	}
>>   
>> @@ -1168,13 +1226,25 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   
>>   	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 err2;
>>   
>> -	ret = phy_power_on(dwc->usb3_generic_phy);
>> -	if (ret < 0)
>> -		goto err3;
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_power_on(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0) {
>> +			for (j = 0; j < i; j++)
>> +				phy_power_off(dwc->usb2_generic_phy[j]);
>> +			goto err2;
>> +		}
>> +	}
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		ret = phy_power_on(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0) {
>> +			for (j = 0; j < i; j++)
>> +				phy_power_off(dwc->usb3_generic_phy[j]);
>> +			goto err3;
>> +		}
>> +	}
>>   
>>   	ret = dwc3_event_buffers_setup(dwc);
>>   	if (ret) {
>> @@ -1297,10 +1367,12 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	return 0;
>>   
>>   err4:
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +	for (i = 0; i < dwc->num_ports; i++)
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>>   
>>   err3:
>> -	phy_power_off(dwc->usb2_generic_phy);
>> +	for (i = 0; i < dwc->num_ports; i++)
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>>   
>>   err2:
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>> @@ -1309,8 +1381,11 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   err1:
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   err0a:
>>   	dwc3_ulpi_exit(dwc);
>> @@ -1319,6 +1394,38 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>   	return ret;
>>   }
>>   
>> +static int dwc3_get_multiport_phys(struct dwc3 *dwc)
>> +{
>> +	int ret;
>> +	struct device *dev = dwc->dev;
>> +	int i;
>> +	char phy_name[15];
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		sprintf(phy_name, "usb2-phy_port%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, "usb2 phy: %s not configured\n", phy_name);
>> +		}
>> +
>> +		sprintf(phy_name, "usb3-phy_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, "usb3 phy: %s not configured\n", phy_name);
>> +		}
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>>   static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   {
>>   	struct device		*dev = dwc->dev;
>> @@ -1349,31 +1456,37 @@ 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 (dwc->num_ports > 1)
>> +		goto get_multiport_phys;
> 
> Can we avoid goto and just return dwc3_get_multiport_phys(dwc) here?
> It's easier to read IMO.
> 
>> +
>> +	dwc->usb2_generic_phy[0] = devm_phy_get(dev, "usb2-phy");
>> +	if (IS_ERR(dwc->usb2_generic_phy[0])) {
>> +		ret = PTR_ERR(dwc->usb2_generic_phy[0]);
>>   		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb2_generic_phy = NULL;
>> +			dwc->usb2_generic_phy[0] = NULL;
>>   		else
>>   			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>>   	}
>>   
>> -	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> -	if (IS_ERR(dwc->usb3_generic_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_generic_phy);
>> +	dwc->usb3_generic_phy[0] = devm_phy_get(dev, "usb3-phy");
>> +	if (IS_ERR(dwc->usb3_generic_phy[0])) {
>> +		ret = PTR_ERR(dwc->usb3_generic_phy[0]);
>>   		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb3_generic_phy = NULL;
>> +			dwc->usb3_generic_phy[0] = NULL;
>>   		else
>>   			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>>   	}
>>   
>>   	return 0;
>> +
>> +get_multiport_phys:
>> +	return dwc3_get_multiport_phys(dwc);
>>   }
>>   
>>   static int dwc3_core_init_mode(struct dwc3 *dwc)
>>   {
>>   	struct device *dev = dwc->dev;
>> -	int ret;
>> +	int ret, i;
>>   
>>   	switch (dwc->dr_mode) {
>>   	case USB_DR_MODE_PERIPHERAL:
>> @@ -1381,8 +1494,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)
>> @@ -1393,8 +1506,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_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)
>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +
>> +	/*
>> +	 * If no mulitport properties are defined, default
> 
> multi*
> 
>> +	 * the port count to '1'.
>> +	 */
> 
> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> on error (similar to how we handle other properties).
> 
Hi Thinh,

   Thanks for the review. On the bindings, Rob and Krzysztof have 
suggested to get the num-ports and num-ss-ports by counting the 
Phy-names in DT.

Since there may be many cases where the user might skip giving any Phy's 
or even skip different ports in the DT if he doesn't want to use them, 
can we design/refactor the below logic as follows while mandating the 
fact that user must give the SS Phy's if any starting from Port-0.:

num-ss-ports = max_port_index (usb3-portX) + 1
num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1

Ex: If there are 3 ports and only 1 is SS capable and user decides to 
skip port-2 HS Phy.

case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"

In both cases, only one SS is present, just the order is changed. (Not 
sure if last few ports can be made SS Capable instead of the first ports 
on any HW) ?

But according to the above formula:

In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong

I believe this covers all cases and I can read this in get_properties 
function. Let me know your opinion if this design is good to proceed 
further.

Regards,
Krishna,

>> +	ret = device_property_read_u32(dev, "num-ports",
>> +				&dwc->num_ports);
>> +	if (ret)
>> +		dwc->num_ports = 1;
>> +
>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>> +				&dwc->num_ss_ports);
>> +	if (ret)
>> +		dwc->num_ss_ports = 1;
>> +
>>   	dwc->lpm_nyet_threshold = lpm_nyet_threshold;
>>   	dwc->tx_de_emphasis = tx_de_emphasis;
>>   
>> @@ -1755,8 +1885,7 @@ static int dwc3_probe(struct platform_device *pdev)
>>   	struct device		*dev = &pdev->dev;
>>   	struct resource		*res, dwc_res;
>>   	struct dwc3		*dwc;
>> -
>> -	int			ret;
>> +	int			ret, i;
>>   
>>   	void __iomem		*regs;
>>   
>> @@ -1933,17 +2062,24 @@ static int dwc3_probe(struct platform_device *pdev)
>>   
>>   err5:
>>   	dwc3_debugfs_exit(dwc);
>> +
>>   	dwc3_event_buffers_cleanup(dwc);
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 1);
>> -	phy_power_off(dwc->usb2_generic_phy);
>> -	phy_power_off(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	usb_phy_shutdown(dwc->usb2_phy);
>>   	usb_phy_shutdown(dwc->usb3_phy);
>> -	phy_exit(dwc->usb2_generic_phy);
>> -	phy_exit(dwc->usb3_generic_phy);
>> +
>> +	for (i = 0; i < dwc->num_ports; i++) {
>> +		phy_exit(dwc->usb2_generic_phy[i]);
>> +		phy_exit(dwc->usb3_generic_phy[i]);
>> +	}
>>   
>>   	dwc3_ulpi_exit(dwc);
>>   
>> @@ -2025,6 +2161,7 @@ static int dwc3_core_init_for_resume(struct dwc3 *dwc)
>>   
>>   static int dwc3_suspend_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>> +	int i;
>>   	unsigned long	flags;
>>   	u32 reg;
>>   
>> @@ -2045,17 +2182,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_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_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 */
>> @@ -2084,6 +2225,7 @@ static int dwc3_resume_common(struct dwc3 *dwc, pm_message_t msg)
>>   {
>>   	unsigned long	flags;
>>   	int		ret;
>> +	int		i;
>>   	u32		reg;
>>   
>>   	switch (dwc->current_dr_role) {
>> @@ -2104,17 +2246,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_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_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 8f9959ba9fd4..2f82eda9d44f 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -35,6 +35,9 @@
>>   
>>   #define DWC3_MSG_MAX	500
>>   
>> +/* Number of ports supported by a multiport controller */
>> +#define MAX_PORTS_SUPPORTED	4
>> +
>>   /* Global constants */
>>   #define DWC3_PULL_UP_TIMEOUT	500	/* ms */
>>   #define DWC3_BOUNCE_SIZE	1024	/* size of a superspeed bulk */
>> @@ -1023,8 +1026,10 @@ 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
>> + * @num_ports: Indicates number of usb ports supported by the controller.
> 
> Note that this is the total number of usb ports including the SS capable
> ones.
> 
>> + * @num_ss_ports: Indicates number of ss capable ports supported by controller
>> + * @usb2_generic_phy: pointer to array of USB2 PHY's
>> + * @usb3_generic_phy: pointer to array of USB3 PHY's
>>    * @phys_ready: flag to indicate that PHYs are ready
>>    * @ulpi: pointer to ulpi interface
>>    * @ulpi_ready: flag to indicate that ULPI is initialized
>> @@ -1157,8 +1162,10 @@ struct dwc3 {
>>   	struct usb_phy		*usb2_phy;
>>   	struct usb_phy		*usb3_phy;
>>   
>> -	struct phy		*usb2_generic_phy;
>> -	struct phy		*usb3_generic_phy;
>> +	u32			num_ports;
>> +	u32			num_ss_ports;
>> +	struct phy		*usb2_generic_phy[MAX_PORTS_SUPPORTED];
>> +	struct phy		*usb3_generic_phy[MAX_PORTS_SUPPORTED];
>>   
>>   	bool			phys_ready;
>>   
>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
>> index 039bf241769a..ea86ff01433b 100644
>> --- a/drivers/usb/dwc3/drd.c
>> +++ b/drivers/usb/dwc3/drd.c
>> @@ -327,7 +327,7 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
>>   
>>   void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>>   {
>> -	int ret;
>> +	int ret, i;
>>   	u32 reg;
>>   	int id;
>>   	unsigned long flags;
>> @@ -386,9 +386,11 @@ 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_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,8 +402,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,
>> +		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)
>> -- 
>> 2.39.0
>>
> 
> No major issue I see here. Just some minor nits. Once you feel it's
> ready, please drop the RFC tags on resubmission.

On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
> 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.
> 
> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> ---
>  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>  drivers/usb/dwc3/core.h |  15 +-
>  drivers/usb/dwc3/drd.c  |  14 +-
>  3 files changed, 244 insertions(+), 89 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 476b63618511..7e0a9a598dfd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c

<snip>

> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>  	dwc->dis_split_quirk = device_property_read_bool(dev,
>  				"snps,dis-split-quirk");
>  
> +
> +	/*
> +	 * If no mulitport properties are defined, default
> +	 * the port count to '1'.
> +	 */
> +	ret = device_property_read_u32(dev, "num-ports",
> +				&dwc->num_ports);
> +	if (ret)
> +		dwc->num_ports = 1;
> +
> +	ret = device_property_read_u32(dev, "num-ss-ports",
> +				&dwc->num_ss_ports);
> +	if (ret)
> +		dwc->num_ss_ports = 1;

By using this DT property instead of using the number of each phy type you
find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
when there's no phy to go along with it.

I ran into this when testing on sa8540p-ride, which only uses one of the
ports on the multiport controller. I didn't enable the other phys (not
sure if that was smart or not) and overrode phy-names/phys, but did not
override num-ports/num-ss-ports, which resulted in that. Nothing bad
happened on a quick test.. but I thought I'd highlight that as another
downside of decoupling this value from the number of phys you grab.

Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
series (probably needs clean up after review, and will definitely need
alteration after you update the dt-binding again). If not I'll continue
to test/review so please CC me!:

Hi,

On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > Currently the DWC3 driver supports only single port controller
> > > which requires at most one HS and one SS PHY.
> > 
> > Add note here that multi-port is for host mode for clarity.
> > 
> > > 
> > > 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.
> > > 
> > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > >   drivers/usb/dwc3/core.h |  15 +-
> > >   drivers/usb/dwc3/drd.c  |  14 +-
> > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > 

<snip>

> > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > >   	dwc->dis_split_quirk = device_property_read_bool(dev,
> > >   				"snps,dis-split-quirk");
> > > +
> > > +	/*
> > > +	 * If no mulitport properties are defined, default
> > 
> > multi*
> > 
> > > +	 * the port count to '1'.
> > > +	 */
> > 
> > Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> > on error (similar to how we handle other properties).
> > 
> Hi Thinh,
> 
>   Thanks for the review. On the bindings, Rob and Krzysztof have suggested
> to get the num-ports and num-ss-ports by counting the Phy-names in DT.

This may be a bit problematic for non-DT device. Currently pci devices
pass fake DT properties to send these kinds of info. But that's fine,
we can enhance dwc3 for non-DT devices later.

> 
> Since there may be many cases where the user might skip giving any Phy's or
> even skip different ports in the DT if he doesn't want to use them, can we
> design/refactor the below logic as follows while mandating the fact that
> user must give the SS Phy's if any starting from Port-0.:
> 
> num-ss-ports = max_port_index (usb3-portX) + 1
> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
> 
> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
> port-2 HS Phy.
> 
> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
> 
> In both cases, only one SS is present, just the order is changed. (Not sure
> if last few ports can be made SS Capable instead of the first ports on any
> HW) ?
> 
> But according to the above formula:
> 
> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
> 

Can't we just walk through all the phy names to figure that out? Let's
not require the user to specify Port-0 is SS capable if they can skip
it.

> I believe this covers all cases and I can read this in get_properties
> function. Let me know your opinion if this design is good to proceed
> further.
> 

Thanks,
Thinh

On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
> Hi,
> 
> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>> Currently the DWC3 driver supports only single port controller
>>>> which requires at most one HS and one SS PHY.
>>>
>>> Add note here that multi-port is for host mode for clarity.
>>>
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
> 
> <snip>
> 
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>    	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>    				"snps,dis-split-quirk");
>>>> +
>>>> +	/*
>>>> +	 * If no mulitport properties are defined, default
>>>
>>> multi*
>>>
>>>> +	 * the port count to '1'.
>>>> +	 */
>>>
>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>> on error (similar to how we handle other properties).
>>>
>> Hi Thinh,
>>
>>    Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
> 
> This may be a bit problematic for non-DT device. Currently pci devices
> pass fake DT properties to send these kinds of info. But that's fine,
> we can enhance dwc3 for non-DT devices later.
> 
>>
>> Since there may be many cases where the user might skip giving any Phy's or
>> even skip different ports in the DT if he doesn't want to use them, can we
>> design/refactor the below logic as follows while mandating the fact that
>> user must give the SS Phy's if any starting from Port-0.:
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>
>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>> port-2 HS Phy.
>>
>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>
>> In both cases, only one SS is present, just the order is changed. (Not sure
>> if last few ports can be made SS Capable instead of the first ports on any
>> HW) ?
>>
>> But according to the above formula:
>>
>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>
> 
> Can't we just walk through all the phy names to figure that out? Let's
> not require the user to specify Port-0 is SS capable if they can skip
> it.
> 
Hi Thinh,

Thanks for the review.

   May be I wasn't able to convey my intention properly in my previous 
thread. The above suggested method doesn't tell that user must not skip 
any phy.

Let us take the below case for 2 Port (HS+SS) capable controller.
If the user skips ss-phy 2, then:

phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"

We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If 
we parse phy-names and find it out, we see there are 2 hs-phy's and 
1-ssphy and num-ports = 2 and num-ss-ports = 1.

If the user skips ss-phy-1, then phy-names would be something like the 
below:

phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";

We need to handle two types of interpretations here while parsing the 
phy-names:

a) There are 2 SS Phy's and we just skipped the first one. In this 
scenario, if we consider (num-ss-ports = 2) and (num-ports = 2) by using 
the above formula as reference, we configure both GUSB3PIPECTL registers 
present in the address map although we never use SS Phy-1 but nothing 
must break. All ports would still work as the user intends with the 
exception of GUSB3PIPECTL1 (for-port1) still being modified.

b) The second interpretation is something like, port-1 is only HS 
capable and only Port-2 is SS Capable, and so in the phy-names only 
port-2 has been provided a SS Phy. Just by parsing through the 
phy-names, it would not be possible to get that info. So if we consider 
num-ss-ports as 2 in this scenario, we end up meddling with wrong 
registers (as there is only 1 GUSB3PIPECTL reg and we are assuing there 
are 2). I wanted to make sure that this scenario was not possible.

num-ss-ports = max_port_index (usb3-portX) + 1
num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1

Taking case of a quad port controller with all ports SS Capable, if the 
user wants to completely skip port-4. Then with above formula, we get 
(num-ports = 3) and (num-ss-ports = 3) by parsing the phy-names and we 
will configure exactly those dwc3-phy registers and not touch the port-4 
registers which is still fine.

Please let me know if the above idea helps us in this scenario.

Regards,
Krishna,

On 1/20/2023 3:39 AM, Andrew Halaney wrote:
> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>> 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.
>>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> ---
>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>   drivers/usb/dwc3/core.h |  15 +-
>>   drivers/usb/dwc3/drd.c  |  14 +-
>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 476b63618511..7e0a9a598dfd 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
> 
> <snip>
> 
>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>   	dwc->dis_split_quirk = device_property_read_bool(dev,
>>   				"snps,dis-split-quirk");
>>   
>> +
>> +	/*
>> +	 * If no mulitport properties are defined, default
>> +	 * the port count to '1'.
>> +	 */
>> +	ret = device_property_read_u32(dev, "num-ports",
>> +				&dwc->num_ports);
>> +	if (ret)
>> +		dwc->num_ports = 1;
>> +
>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>> +				&dwc->num_ss_ports);
>> +	if (ret)
>> +		dwc->num_ss_ports = 1;
> 
> By using this DT property instead of using the number of each phy type you
> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
> when there's no phy to go along with it.
> 
Hi Andrew,

  Thanks for the review. Yes, this decoupling is still there and its 
fine I believe.

> I ran into this when testing on sa8540p-ride, which only uses one of the
> ports on the multiport controller. I didn't enable the other phys (not
> sure if that was smart or not) and overrode phy-names/phys, but did not
> override num-ports/num-ss-ports, which resulted in that. Nothing bad
> happened on a quick test.. but I thought I'd highlight that as another
> downside of decoupling this value from the number of phys you grab.
> 
If we do not override phy-names or num-ports/num-ss-ports info in DT, 
they are just defaulted to '1' and as per the current logic only port-1 
registers must be configured. Isn't that the case happening ?

> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
> series (probably needs clean up after review, and will definitely need
> alteration after you update the dt-binding again). If not I'll continue
> to test/review so please CC me!:
> 
> 
Sure, I can add this patch (probably will add the other phy's too) 
during the final submission.

>  From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
> From: Andrew Halaney <ahalaney@redhat.com>
> Date: Thu, 19 Jan 2023 14:53:38 -0600
> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
> Content-type: text/plain
> 
> 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.
> 
> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> ---
>   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>   1 file changed, 24 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> index 97957f3baa64..56d4f43faa1e 100644
> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>   	status = "okay";
>   };
>   
> +&usb_2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&usb2_en_state>;
> +
> +	status = "okay";
> +};
> +
> +&usb_2_dwc3 {
> +	dr_mode = "host";
> +	num-ports = <1>;
> +	num-ss-ports = <1>;

More over, if this is a multiport controller and you are using only 
port-1, it is as good as a single port controller I believe and the 
normal DT convention must work. Adding these properties as "1" is not 
required as the driver logic defaults them to "1" if they are not found.

Just to add a point here (as I was not clear in DT Binding description, 
My bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
present on HW whether they are used in DT or not. Just to cover all 
cases which user can use [1].

[]1: 
https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/

Regards,
Krishna,

> +	phy-names = "usb2-phy", "usb3-phy";
> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> +};
> +
>   &usb_2_hsphy0 {
>   	vdda-pll-supply = <&vreg_l5a>;
>   	vdda18-supply = <&vreg_l7g>;
> @@ -313,4 +328,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;
> +	};
>   };

On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
> > On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
> > > 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.
> > > 
> > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > ---
> > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > >   drivers/usb/dwc3/core.h |  15 +-
> > >   drivers/usb/dwc3/drd.c  |  14 +-
> > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > 
> > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > index 476b63618511..7e0a9a598dfd 100644
> > > --- a/drivers/usb/dwc3/core.c
> > > +++ b/drivers/usb/dwc3/core.c
> > 
> > <snip>
> > 
> > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > >   	dwc->dis_split_quirk = device_property_read_bool(dev,
> > >   				"snps,dis-split-quirk");
> > > +
> > > +	/*
> > > +	 * If no mulitport properties are defined, default
> > > +	 * the port count to '1'.
> > > +	 */
> > > +	ret = device_property_read_u32(dev, "num-ports",
> > > +				&dwc->num_ports);
> > > +	if (ret)
> > > +		dwc->num_ports = 1;
> > > +
> > > +	ret = device_property_read_u32(dev, "num-ss-ports",
> > > +				&dwc->num_ss_ports);
> > > +	if (ret)
> > > +		dwc->num_ss_ports = 1;
> > 
> > By using this DT property instead of using the number of each phy type you
> > find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
> > when there's no phy to go along with it.
> > 
> Hi Andrew,
> 
>  Thanks for the review. Yes, this decoupling is still there and its fine I
> believe.
> 
> > I ran into this when testing on sa8540p-ride, which only uses one of the
> > ports on the multiport controller. I didn't enable the other phys (not
> > sure if that was smart or not) and overrode phy-names/phys, but did not
> > override num-ports/num-ss-ports, which resulted in that. Nothing bad
> > happened on a quick test.. but I thought I'd highlight that as another
> > downside of decoupling this value from the number of phys you grab.
> > 
> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
> are just defaulted to '1' and as per the current logic only port-1 registers
> must be configured. Isn't that the case happening ?
> 

In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
So unless I override them I get this from your sc8280xp.dtsi:

+                       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>;
+                               num-ports = <4>;
+                               num-ss-ports = <2>;
+                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
+                                       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
+                                       <&usb_2_hsphy2>,
+                                       <&usb_2_hsphy3>;
+                               phy-names = "usb2-phy_port0", "usb3-phy_port0",
+                                               "usb2-phy_port1", "usb3-phy_port1",
+                                               "usb2-phy_port2",
+                                               "usb2-phy_port3";
+                       };

Since this board only uses one port of the multiport controller, I
redefined phys/phy-names to indicate that. I figured that was more
desireable than enabling unnecessary phys. Without overriding
num-ports/num-ss-ports all the for loops in this patch would act like
the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
multiple times etc as well as look for the multiport phy-names and fail
to actually get any phys. Hope that makes sense!

> > Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
> > series (probably needs clean up after review, and will definitely need
> > alteration after you update the dt-binding again). If not I'll continue
> > to test/review so please CC me!:
> > 
> > 
> Sure, I can add this patch (probably will add the other phy's too) during
> the final submission.

I don't have a great understanding of the mapping of the phys to
physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
so if it makes more sense to enable all the relevant SoC phys, write
those registers in the DWC3 IP, etc, and only use one of the actual
board outputs then feel free. I think this is a good example of "what if
a board designer only uses a single port of the multiport IP" imo.

> 
> >  From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
> > From: Andrew Halaney <ahalaney@redhat.com>
> > Date: Thu, 19 Jan 2023 14:53:38 -0600
> > Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
> > Content-type: text/plain
> > 
> > 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.
> > 
> > Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
> > ---
> >   arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
> >   1 file changed, 24 insertions(+)
> > 
> > diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > index 97957f3baa64..56d4f43faa1e 100644
> > --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
> > @@ -246,6 +246,21 @@ &usb_0_qmpphy {
> >   	status = "okay";
> >   };
> > +&usb_2 {
> > +	pinctrl-names = "default";
> > +	pinctrl-0 = <&usb2_en_state>;
> > +
> > +	status = "okay";
> > +};
> > +
> > +&usb_2_dwc3 {
> > +	dr_mode = "host";
> > +	num-ports = <1>;
> > +	num-ss-ports = <1>;
> 
> More over, if this is a multiport controller and you are using only port-1,
> it is as good as a single port controller I believe and the normal DT
> convention must work. Adding these properties as "1" is not required as the
> driver logic defaults them to "1" if they are not found.

See above comment about inheriting from sc8280xp.dtsi and needing to
override their values.

> 
> Just to add a point here (as I was not clear in DT Binding description, My
> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
> HW whether they are used in DT or not. Just to cover all cases which user
> can use [1].
> 
> []1:
> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/

Ok, if you're going with that approach of "must indicate the HS/SS Phys
present on HW whether they are used in the DT or not" (/me assumes DT
here means on the board and not an incorrect coding of the DT) then I
suppose I should not have overridden anything but phys/phy-names to
indicate that I'm only using the first port (and used the multiport
phy-names convention). It looks like in that link you also mention that
it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
defined, which was my concern and reasoning above for overriding
num-ports/num-ss-ports.

Thanks,
Andrew

> 
> Regards,
> Krishna,
> 
> > +	phy-names = "usb2-phy", "usb3-phy";
> > +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
> > +};
> > +
> >   &usb_2_hsphy0 {
> >   	vdda-pll-supply = <&vreg_l5a>;
> >   	vdda18-supply = <&vreg_l7g>;
> > @@ -313,4 +328,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;
> > +	};
> >   };
>

On 1/20/2023 8:07 PM, Andrew Halaney wrote:
> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>> 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.
>>>>
>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>> ---
>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>
>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>> index 476b63618511..7e0a9a598dfd 100644
>>>> --- a/drivers/usb/dwc3/core.c
>>>> +++ b/drivers/usb/dwc3/core.c
>>>
>>> <snip>
>>>
>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>    	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>    				"snps,dis-split-quirk");
>>>> +
>>>> +	/*
>>>> +	 * If no mulitport properties are defined, default
>>>> +	 * the port count to '1'.
>>>> +	 */
>>>> +	ret = device_property_read_u32(dev, "num-ports",
>>>> +				&dwc->num_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ports = 1;
>>>> +
>>>> +	ret = device_property_read_u32(dev, "num-ss-ports",
>>>> +				&dwc->num_ss_ports);
>>>> +	if (ret)
>>>> +		dwc->num_ss_ports = 1;
>>>
>>> By using this DT property instead of using the number of each phy type you
>>> find you can get into situations where you're writing DWC3_GUSB2PHYCFG, etc,
>>> when there's no phy to go along with it.
>>>
>> Hi Andrew,
>>
>>   Thanks for the review. Yes, this decoupling is still there and its fine I
>> believe.
>>
>>> I ran into this when testing on sa8540p-ride, which only uses one of the
>>> ports on the multiport controller. I didn't enable the other phys (not
>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>> happened on a quick test.. but I thought I'd highlight that as another
>>> downside of decoupling this value from the number of phys you grab.
>>>
>> If we do not override phy-names or num-ports/num-ss-ports info in DT, they
>> are just defaulted to '1' and as per the current logic only port-1 registers
>> must be configured. Isn't that the case happening ?
>>
> 
> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've created!
> So unless I override them I get this from your sc8280xp.dtsi:
> 
> +                       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>;
> +                               num-ports = <4>;
> +                               num-ss-ports = <2>;
> +                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
> +                                       <&usb_2_hsphy1>, <&usb_2_qmpphy1>,
> +                                       <&usb_2_hsphy2>,
> +                                       <&usb_2_hsphy3>;
> +                               phy-names = "usb2-phy_port0", "usb3-phy_port0",
> +                                               "usb2-phy_port1", "usb3-phy_port1",
> +                                               "usb2-phy_port2",
> +                                               "usb2-phy_port3";
> +                       };
> 
> Since this board only uses one port of the multiport controller, I
> redefined phys/phy-names to indicate that. I figured that was more
> desireable than enabling unnecessary phys. Without overriding
> num-ports/num-ss-ports all the for loops in this patch would act like
> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
> multiple times etc as well as look for the multiport phy-names and fail
> to actually get any phys. Hope that makes sense!
> 
Hi Andrew,

  My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that 
case it makes complete sense to override the num-ports and num-ss-ports 
to "1" and the usb phy-names.
>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>> series (probably needs clean up after review, and will definitely need
>>> alteration after you update the dt-binding again). If not I'll continue
>>> to test/review so please CC me!:
>>>
>>>
>> Sure, I can add this patch (probably will add the other phy's too) during
>> the final submission.
> 
> I don't have a great understanding of the mapping of the phys to
> physical connections (as well as what registers like DWC3_GUSB2PHYCFG do),
> so if it makes more sense to enable all the relevant SoC phys, write
> those registers in the DWC3 IP, etc, and only use one of the actual
> board outputs then feel free. I think this is a good example of "what if
> a board designer only uses a single port of the multiport IP" imo.
> Agreed. This could be a good example of multi port with only single port 
working.

>>
>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 2001
>>> From: Andrew Halaney <ahalaney@redhat.com>
>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>> Content-type: text/plain
>>>
>>> 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.
>>>
>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>> ---
>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 +++++++++++++++++++++++
>>>    1 file changed, 24 insertions(+)
>>>
>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> index 97957f3baa64..56d4f43faa1e 100644
>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>    	status = "okay";
>>>    };
>>> +&usb_2 {
>>> +	pinctrl-names = "default";
>>> +	pinctrl-0 = <&usb2_en_state>;
>>> +
>>> +	status = "okay";
>>> +};
>>> +
>>> +&usb_2_dwc3 {
>>> +	dr_mode = "host";
>>> +	num-ports = <1>;
>>> +	num-ss-ports = <1>;
>>
>> More over, if this is a multiport controller and you are using only port-1,
>> it is as good as a single port controller I believe and the normal DT
>> convention must work. Adding these properties as "1" is not required as the
>> driver logic defaults them to "1" if they are not found.
> 
> See above comment about inheriting from sc8280xp.dtsi and needing to
> override their values.
> 
>>
>> Just to add a point here (as I was not clear in DT Binding description, My
>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys present on
>> HW whether they are used in DT or not. Just to cover all cases which user
>> can use [1].
>>
>> []1:
>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
> 
> Ok, if you're going with that approach of "must indicate the HS/SS Phys
> present on HW whether they are used in the DT or not" (/me assumes DT
> here means on the board and not an incorrect coding of the DT) then I
> suppose I should not have overridden anything but phys/phy-names to
> indicate that I'm only using the first port (and used the multiport
> phy-names convention). It looks like in that link you also mention that
> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
> defined, which was my concern and reasoning above for overriding
> num-ports/num-ss-ports.
> 
> Thanks,
> Andrew
> 
Actually, I was trying to mandate that rule to take care of cases where 
the phy's for say port2 or port3 are missing for a quad port controller 
in dtsi and we don't want to end up configuring wrong dwc3-phy regs.

For just the first port, the changes you have mentioned must be 
sufficient. (Furthermore, thanks for the review and testing it on 
sa8295-ride and confirming nothing breaks while the first port is enabled)

Regards,
Krishna,
>>
>> Regards,
>> Krishna,
>>
>>> +	phy-names = "usb2-phy", "usb3-phy";
>>> +	phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>> +};
>>> +
>>>    &usb_2_hsphy0 {
>>>    	vdda-pll-supply = <&vreg_l5a>;
>>>    	vdda18-supply = <&vreg_l7g>;
>>> @@ -313,4 +328,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;
>>> +	};
>>>    };
>>
>

On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 8:07 PM, Andrew Halaney wrote:
>> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>>
>>>
>>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 304 
>>>>> +++++++++++++++++++++++++++++-----------
>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>
>>>> <snip>
>>>>
>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 
>>>>> *dwc)
>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>                    "snps,dis-split-quirk");
>>>>> +
>>>>> +    /*
>>>>> +     * If no mulitport properties are defined, default
>>>>> +     * the port count to '1'.
>>>>> +     */
>>>>> +    ret = device_property_read_u32(dev, "num-ports",
>>>>> +                &dwc->num_ports);
>>>>> +    if (ret)
>>>>> +        dwc->num_ports = 1;
>>>>> +
>>>>> +    ret = device_property_read_u32(dev, "num-ss-ports",
>>>>> +                &dwc->num_ss_ports);
>>>>> +    if (ret)
>>>>> +        dwc->num_ss_ports = 1;
>>>>
>>>> By using this DT property instead of using the number of each phy 
>>>> type you
>>>> find you can get into situations where you're writing 
>>>> DWC3_GUSB2PHYCFG, etc,
>>>> when there's no phy to go along with it.
>>>>
>>> Hi Andrew,
>>>
>>>   Thanks for the review. Yes, this decoupling is still there and its 
>>> fine I
>>> believe.
>>>
>>>> I ran into this when testing on sa8540p-ride, which only uses one of 
>>>> the
>>>> ports on the multiport controller. I didn't enable the other phys (not
>>>> sure if that was smart or not) and overrode phy-names/phys, but did not
>>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>>> happened on a quick test.. but I thought I'd highlight that as another
>>>> downside of decoupling this value from the number of phys you grab.
>>>>
>>> If we do not override phy-names or num-ports/num-ss-ports info in DT, 
>>> they
>>> are just defaulted to '1' and as per the current logic only port-1 
>>> registers
>>> must be configured. Isn't that the case happening ?
>>>
>>
>> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've 
>> created!
>> So unless I override them I get this from your sc8280xp.dtsi:
>>
>> +                       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>;
>> +                               num-ports = <4>;
>> +                               num-ss-ports = <2>;
>> +                               phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>,
>> +                                       <&usb_2_hsphy1>, 
>> <&usb_2_qmpphy1>,
>> +                                       <&usb_2_hsphy2>,
>> +                                       <&usb_2_hsphy3>;
>> +                               phy-names = "usb2-phy_port0", 
>> "usb3-phy_port0",
>> +                                               "usb2-phy_port1", 
>> "usb3-phy_port1",
>> +                                               "usb2-phy_port2",
>> +                                               "usb2-phy_port3";
>> +                       };
>>
>> Since this board only uses one port of the multiport controller, I
>> redefined phys/phy-names to indicate that. I figured that was more
>> desireable than enabling unnecessary phys. Without overriding
>> num-ports/num-ss-ports all the for loops in this patch would act like
>> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
>> multiple times etc as well as look for the multiport phy-names and fail
>> to actually get any phys. Hope that makes sense!
>>
> Hi Andrew,
> 
>   My Bad. I missed the fact that it was based on sc8280xp.dtsi. In that 
> case it makes complete sense to override the num-ports and num-ss-ports 
> to "1" and the usb phy-names.
>>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>>> series (probably needs clean up after review, and will definitely need
>>>> alteration after you update the dt-binding again). If not I'll continue
>>>> to test/review so please CC me!:
>>>>
>>>>
>>> Sure, I can add this patch (probably will add the other phy's too) 
>>> during
>>> the final submission.
>>
>> I don't have a great understanding of the mapping of the phys to
>> physical connections (as well as what registers like DWC3_GUSB2PHYCFG 
>> do),
>> so if it makes more sense to enable all the relevant SoC phys, write
>> those registers in the DWC3 IP, etc, and only use one of the actual
>> board outputs then feel free. I think this is a good example of "what if
>> a board designer only uses a single port of the multiport IP" imo.
>> Agreed. This could be a good example of multi port with only single port 

Typo in the previous mail. Correcting it here.

> working.
Agreed, The dt-patch you provided will be a good working example of 
getting just a single port working for a multiport controller.

Regards,
Krishna,

>>>
>>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 
>>>> 2001
>>>> From: Andrew Halaney <ahalaney@redhat.com>
>>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>>> Content-type: text/plain
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>>> ---
>>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 
>>>> +++++++++++++++++++++++
>>>>    1 file changed, 24 insertions(+)
>>>>
>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts 
>>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> index 97957f3baa64..56d4f43faa1e 100644
>>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>>        status = "okay";
>>>>    };
>>>> +&usb_2 {
>>>> +    pinctrl-names = "default";
>>>> +    pinctrl-0 = <&usb2_en_state>;
>>>> +
>>>> +    status = "okay";
>>>> +};
>>>> +
>>>> +&usb_2_dwc3 {
>>>> +    dr_mode = "host";
>>>> +    num-ports = <1>;
>>>> +    num-ss-ports = <1>;
>>>
>>> More over, if this is a multiport controller and you are using only 
>>> port-1,
>>> it is as good as a single port controller I believe and the normal DT
>>> convention must work. Adding these properties as "1" is not required 
>>> as the
>>> driver logic defaults them to "1" if they are not found.
>>
>> See above comment about inheriting from sc8280xp.dtsi and needing to
>> override their values.
>>
>>>
>>> Just to add a point here (as I was not clear in DT Binding 
>>> description, My
>>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
>>> present on
>>> HW whether they are used in DT or not. Just to cover all cases which 
>>> user
>>> can use [1].
>>>
>>> []1:
>>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/ 
>>>
>>
>> Ok, if you're going with that approach of "must indicate the HS/SS Phys
>> present on HW whether they are used in the DT or not" (/me assumes DT
>> here means on the board and not an incorrect coding of the DT) then I
>> suppose I should not have overridden anything but phys/phy-names to
>> indicate that I'm only using the first port (and used the multiport
>> phy-names convention). It looks like in that link you also mention that
>> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
>> defined, which was my concern and reasoning above for overriding
>> num-ports/num-ss-ports.
>>
>> Thanks,
>> Andrew
>>
> Actually, I was trying to mandate that rule to take care of cases where 
> the phy's for say port2 or port3 are missing for a quad port controller 
> in dtsi and we don't want to end up configuring wrong dwc3-phy regs.
> 
> For just the first port, the changes you have mentioned must be 
> sufficient. (Furthermore, thanks for the review and testing it on 
> sa8295-ride and confirming nothing breaks while the first port is enabled)
> 
> Regards,
> Krishna,
>>>
>>> Regards,
>>> Krishna,
>>>
>>>> +    phy-names = "usb2-phy", "usb3-phy";
>>>> +    phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>>> +};
>>>> +
>>>>    &usb_2_hsphy0 {
>>>>        vdda-pll-supply = <&vreg_l5a>;
>>>>        vdda18-supply = <&vreg_l7g>;
>>>> @@ -313,4 +328,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;
>>>> +    };
>>>>    };
>>>
>>

On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
> > Hi,
> > 
> > On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
> > > > Hi,
> > > > 
> > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > > Currently the DWC3 driver supports only single port controller
> > > > > which requires at most one HS and one SS PHY.
> > > > 
> > > > Add note here that multi-port is for host mode for clarity.
> > > > 
> > > > > 
> > > > > 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.
> > > > > 
> > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > ---
> > > > >    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > > >    drivers/usb/dwc3/core.h |  15 +-
> > > > >    drivers/usb/dwc3/drd.c  |  14 +-
> > > > >    3 files changed, 244 insertions(+), 89 deletions(-)
> > > > > 
> > 
> > <snip>
> > 
> > > > > @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
> > > > >    	dwc->dis_split_quirk = device_property_read_bool(dev,
> > > > >    				"snps,dis-split-quirk");
> > > > > +
> > > > > +	/*
> > > > > +	 * If no mulitport properties are defined, default
> > > > 
> > > > multi*
> > > > 
> > > > > +	 * the port count to '1'.
> > > > > +	 */
> > > > 
> > > > Can we initialize num_ports and num_ss_ports to 1 instead of setting it
> > > > on error (similar to how we handle other properties).
> > > > 
> > > Hi Thinh,
> > > 
> > >    Thanks for the review. On the bindings, Rob and Krzysztof have suggested
> > > to get the num-ports and num-ss-ports by counting the Phy-names in DT.
> > 
> > This may be a bit problematic for non-DT device. Currently pci devices
> > pass fake DT properties to send these kinds of info. But that's fine,
> > we can enhance dwc3 for non-DT devices later.
> > 
> > > 
> > > Since there may be many cases where the user might skip giving any Phy's or
> > > even skip different ports in the DT if he doesn't want to use them, can we
> > > design/refactor the below logic as follows while mandating the fact that
> > > user must give the SS Phy's if any starting from Port-0.:
> > > 
> > > num-ss-ports = max_port_index (usb3-portX) + 1
> > > num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
> > > 
> > > Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
> > > port-2 HS Phy.
> > > 
> > > case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> > > case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
> > > 
> > > In both cases, only one SS is present, just the order is changed. (Not sure
> > > if last few ports can be made SS Capable instead of the first ports on any
> > > HW) ?
> > > 
> > > But according to the above formula:
> > > 
> > > In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
> > > In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
> > > 
> > 
> > Can't we just walk through all the phy names to figure that out? Let's
> > not require the user to specify Port-0 is SS capable if they can skip
> > it.
> > 
> Hi Thinh,
> 
> Thanks for the review.
> 
>   May be I wasn't able to convey my intention properly in my previous
> thread. The above suggested method doesn't tell that user must not skip any
> phy.
> 
> Let us take the below case for 2 Port (HS+SS) capable controller.
> If the user skips ss-phy 2, then:
> 
> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
> 
> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
> num-ports = 2 and num-ss-ports = 1.
> 
> If the user skips ss-phy-1, then phy-names would be something like the
> below:
> 
> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
> 
> We need to handle two types of interpretations here while parsing the
> phy-names:
> 
> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
> formula as reference, we configure both GUSB3PIPECTL registers present in
> the address map although we never use SS Phy-1 but nothing must break. All
> ports would still work as the user intends with the exception of
> GUSB3PIPECTL1 (for-port1) still being modified.
> 
> b) The second interpretation is something like, port-1 is only HS capable
> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
> provided a SS Phy. Just by parsing through the phy-names, it would not be
> possible to get that info. So if we consider num-ss-ports as 2 in this
> scenario, we end up meddling with wrong registers (as there is only 1
> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
> this scenario was not possible.
> 
> num-ss-ports = max_port_index (usb3-portX) + 1
> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
> 
> Taking case of a quad port controller with all ports SS Capable, if the user
> wants to completely skip port-4. Then with above formula, we get (num-ports
> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
> exactly those dwc3-phy registers and not touch the port-4 registers which is
> still fine.
> 
> Please let me know if the above idea helps us in this scenario.
> 

This becomes rather more complicated because the user can skip certain
port in the DT. We have access to the host registers. Can we just
temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
capability before handing control over to the xHCI driver. We would be
able to get the num_ports and num_ss_ports then.

Similarly, the xhci driver doesn't care whether the user skips certain
port in the DT, it only checks and operates based on the capability
registers.

If we have the exact num_ports and num_ss_ports, we can be sure the
setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.

Thanks,
Thinh

On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> Hi,
> 
> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > Currently the DWC3 driver supports only single port controller
> > which requires at most one HS and one SS PHY.
> 
> Add note here that multi-port is for host mode for clarity.
> 
> > 
> > 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.
> > 
> > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > ---
> >  drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> >  drivers/usb/dwc3/core.h |  15 +-
> >  drivers/usb/dwc3/drd.c  |  14 +-
> >  3 files changed, 244 insertions(+), 89 deletions(-)
> > 
> > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > index 476b63618511..7e0a9a598dfd 100644
> > --- a/drivers/usb/dwc3/core.c
> > +++ b/drivers/usb/dwc3/core.c
> > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> >  {
> >  	struct dwc3 *dwc = work_to_dwc(work);
> >  	unsigned long flags;
> > -	int ret;
> > +	int ret, i;
> 
> Can we declare variables in separate lines here and other places.
> 
> >  	u32 reg;
> >  	u32 desired_dr_role;
> >  
> > @@ -200,8 +200,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_ports; i++) {

BTW, is num_ports the total of usb2 + usb3 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 (dwc->dis_split_quirk) {

Thanks,
Thinh

On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> On Thu, Jan 19, 2023, Thinh Nguyen wrote:
>> Hi,
>>
>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>> Currently the DWC3 driver supports only single port controller
>>> which requires at most one HS and one SS PHY.
>>
>> Add note here that multi-port is for host mode for clarity.
>>
>>>
>>> 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.
>>>
>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>> ---
>>>   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>   drivers/usb/dwc3/core.h |  15 +-
>>>   drivers/usb/dwc3/drd.c  |  14 +-
>>>   3 files changed, 244 insertions(+), 89 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>> index 476b63618511..7e0a9a598dfd 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>   {
>>>   	struct dwc3 *dwc = work_to_dwc(work);
>>>   	unsigned long flags;
>>> -	int ret;
>>> +	int ret, i;
>>
>> Can we declare variables in separate lines here and other places.
>>
>>>   	u32 reg;
>>>   	u32 desired_dr_role;
>>>   
>>> @@ -200,8 +200,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_ports; i++) {
> 
> BTW, is num_ports the total of usb2 + usb3 ports?
Hi Thinh,

   No, num_ports is just the total number of hw usb ports present 
(presuming each port is hs capable, this is just the number of HS Phy's 
available).

Regards,
KRishna,
> 
>>> +				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) {
> 
> Thanks,
> Thinh

On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>>>> Hi,
>>>>>
>>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>>> Currently the DWC3 driver supports only single port controller
>>>>>> which requires at most one HS and one SS PHY.
>>>>>
>>>>> Add note here that multi-port is for host mode for clarity.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>>     drivers/usb/dwc3/core.h |  15 +-
>>>>>>     drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>     3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>
>>> <snip>
>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>>     	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>     				"snps,dis-split-quirk");
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If no mulitport properties are defined, default
>>>>>
>>>>> multi*
>>>>>
>>>>>> +	 * the port count to '1'.
>>>>>> +	 */
>>>>>
>>>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>>>> on error (similar to how we handle other properties).
>>>>>
>>>> Hi Thinh,
>>>>
>>>>     Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>>>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
>>>
>>> This may be a bit problematic for non-DT device. Currently pci devices
>>> pass fake DT properties to send these kinds of info. But that's fine,
>>> we can enhance dwc3 for non-DT devices later.
>>>
>>>>
>>>> Since there may be many cases where the user might skip giving any Phy's or
>>>> even skip different ports in the DT if he doesn't want to use them, can we
>>>> design/refactor the below logic as follows while mandating the fact that
>>>> user must give the SS Phy's if any starting from Port-0.:
>>>>
>>>> num-ss-ports = max_port_index (usb3-portX) + 1
>>>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>>>
>>>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>>>> port-2 HS Phy.
>>>>
>>>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>>>
>>>> In both cases, only one SS is present, just the order is changed. (Not sure
>>>> if last few ports can be made SS Capable instead of the first ports on any
>>>> HW) ?
>>>>
>>>> But according to the above formula:
>>>>
>>>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>>>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>>>
>>>
>>> Can't we just walk through all the phy names to figure that out? Let's
>>> not require the user to specify Port-0 is SS capable if they can skip
>>> it.
>>>
>> Hi Thinh,
>>
>> Thanks for the review.
>>
>>    May be I wasn't able to convey my intention properly in my previous
>> thread. The above suggested method doesn't tell that user must not skip any
>> phy.
>>
>> Let us take the below case for 2 Port (HS+SS) capable controller.
>> If the user skips ss-phy 2, then:
>>
>> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>
>> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
>> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
>> num-ports = 2 and num-ss-ports = 1.
>>
>> If the user skips ss-phy-1, then phy-names would be something like the
>> below:
>>
>> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
>>
>> We need to handle two types of interpretations here while parsing the
>> phy-names:
>>
>> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
>> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
>> formula as reference, we configure both GUSB3PIPECTL registers present in
>> the address map although we never use SS Phy-1 but nothing must break. All
>> ports would still work as the user intends with the exception of
>> GUSB3PIPECTL1 (for-port1) still being modified.
>>
>> b) The second interpretation is something like, port-1 is only HS capable
>> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
>> provided a SS Phy. Just by parsing through the phy-names, it would not be
>> possible to get that info. So if we consider num-ss-ports as 2 in this
>> scenario, we end up meddling with wrong registers (as there is only 1
>> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
>> this scenario was not possible.
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
>>
>> Taking case of a quad port controller with all ports SS Capable, if the user
>> wants to completely skip port-4. Then with above formula, we get (num-ports
>> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
>> exactly those dwc3-phy registers and not touch the port-4 registers which is
>> still fine.
>>
>> Please let me know if the above idea helps us in this scenario.
>>
> 
> This becomes rather more complicated because the user can skip certain
> port in the DT. We have access to the host registers. Can we just
> temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> capability before handing control over to the xHCI driver. We would be
> able to get the num_ports and num_ss_ports then.
> 
> Similarly, the xhci driver doesn't care whether the user skips certain
> port in the DT, it only checks and operates based on the capability
> registers.
> 
> If we have the exact num_ports and num_ss_ports, we can be sure the
> setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> 
Hi Thinh,

Thanks for the review.
Sure, I can explore this option and get the port info. This must make 
things easier.

Regards,
Krishna,

On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> > On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> > > Hi,
> > > 
> > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > Currently the DWC3 driver supports only single port controller
> > > > which requires at most one HS and one SS PHY.
> > > 
> > > Add note here that multi-port is for host mode for clarity.
> > > 
> > > > 
> > > > 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.
> > > > 
> > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > ---
> > > >   drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > >   drivers/usb/dwc3/core.h |  15 +-
> > > >   drivers/usb/dwc3/drd.c  |  14 +-
> > > >   3 files changed, 244 insertions(+), 89 deletions(-)
> > > > 
> > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > index 476b63618511..7e0a9a598dfd 100644
> > > > --- a/drivers/usb/dwc3/core.c
> > > > +++ b/drivers/usb/dwc3/core.c
> > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > >   {
> > > >   	struct dwc3 *dwc = work_to_dwc(work);
> > > >   	unsigned long flags;
> > > > -	int ret;
> > > > +	int ret, i;
> > > 
> > > Can we declare variables in separate lines here and other places.
> > > 
> > > >   	u32 reg;
> > > >   	u32 desired_dr_role;
> > > > @@ -200,8 +200,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_ports; i++) {
> > 
> > BTW, is num_ports the total of usb2 + usb3 ports?
> Hi Thinh,
> 
>   No, num_ports is just the total number of hw usb ports present (presuming
> each port is hs capable, this is just the number of HS Phy's available).
> 

I see, thanks. Can you also make this clear in its description. I got
mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.

Thanks,
Thinh

On 1/21/2023 7:49 AM, Thinh Nguyen wrote:
> On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
>>> On Thu, Jan 19, 2023, Thinh Nguyen wrote:
>>>> Hi,
>>>>
>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>> Currently the DWC3 driver supports only single port controller
>>>>> which requires at most one HS and one SS PHY.
>>>>
>>>> Add note here that multi-port is for host mode for clarity.
>>>>
>>>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>> ---
>>>>>    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>
>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>> --- a/drivers/usb/dwc3/core.c
>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>> @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
>>>>>    {
>>>>>    	struct dwc3 *dwc = work_to_dwc(work);
>>>>>    	unsigned long flags;
>>>>> -	int ret;
>>>>> +	int ret, i;
>>>>
>>>> Can we declare variables in separate lines here and other places.
>>>>
>>>>>    	u32 reg;
>>>>>    	u32 desired_dr_role;
>>>>> @@ -200,8 +200,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_ports; i++) {
>>>
>>> BTW, is num_ports the total of usb2 + usb3 ports?
>> Hi Thinh,
>>
>>    No, num_ports is just the total number of hw usb ports present (presuming
>> each port is hs capable, this is just the number of HS Phy's available).
>>
> 
> I see, thanks. Can you also make this clear in its description. I got
> mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.
> 
> Thanks,
> Thinh

Sure, Will add this to commit text.
But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS 
Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this 
value (to just number of hs and ss).

Regards,
Krishna,

On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 7:49 AM, Thinh Nguyen wrote:
> > On Sat, Jan 21, 2023, Krishna Kurapati PSSNV wrote:
> > > 
> > > 
> > > On 1/21/2023 4:27 AM, Thinh Nguyen wrote:
> > > > On Thu, Jan 19, 2023, Thinh Nguyen wrote:
> > > > > Hi,
> > > > > 
> > > > > On Sun, Jan 15, 2023, Krishna Kurapati wrote:
> > > > > > Currently the DWC3 driver supports only single port controller
> > > > > > which requires at most one HS and one SS PHY.
> > > > > 
> > > > > Add note here that multi-port is for host mode for clarity.
> > > > > 
> > > > > > 
> > > > > > 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.
> > > > > > 
> > > > > > Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
> > > > > > Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> > > > > > ---
> > > > > >    drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
> > > > > >    drivers/usb/dwc3/core.h |  15 +-
> > > > > >    drivers/usb/dwc3/drd.c  |  14 +-
> > > > > >    3 files changed, 244 insertions(+), 89 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> > > > > > index 476b63618511..7e0a9a598dfd 100644
> > > > > > --- a/drivers/usb/dwc3/core.c
> > > > > > +++ b/drivers/usb/dwc3/core.c
> > > > > > @@ -120,7 +120,7 @@ static void __dwc3_set_mode(struct work_struct *work)
> > > > > >    {
> > > > > >    	struct dwc3 *dwc = work_to_dwc(work);
> > > > > >    	unsigned long flags;
> > > > > > -	int ret;
> > > > > > +	int ret, i;
> > > > > 
> > > > > Can we declare variables in separate lines here and other places.
> > > > > 
> > > > > >    	u32 reg;
> > > > > >    	u32 desired_dr_role;
> > > > > > @@ -200,8 +200,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_ports; i++) {
> > > > 
> > > > BTW, is num_ports the total of usb2 + usb3 ports?
> > > Hi Thinh,
> > > 
> > >    No, num_ports is just the total number of hw usb ports present (presuming
> > > each port is hs capable, this is just the number of HS Phy's available).
> > > 
> > 
> > I see, thanks. Can you also make this clear in its description. I got
> > mixed up at some point for the equivalent of HCPARAMS1.MAXPORTS.
> > 
> > Thanks,
> > Thinh
> 
> Sure, Will add this to commit text.
> But as you rightly mentioned, HCSPARAMS1 gives the total number of HS+SS
> Phy's available (HCSPARAMS1.MAXPORTS). Is there a way to segregate this
> value (to just number of hs and ss).
> 

We need to walk through each port and check its capability, and we can
check the port's major/minor revision to determine whether it's SS
capable. See xhci driver's logic and how it calls xhci_add_in_port().

Thanks,
Thinh

On 1/20/2023 8:48 PM, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/20/2023 8:43 PM, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 8:07 PM, Andrew Halaney wrote:
>>> On Fri, Jan 20, 2023 at 07:25:57AM +0530, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/20/2023 3:39 AM, Andrew Halaney wrote:
>>>>> On Sun, Jan 15, 2023 at 05:11:43PM +0530, Krishna Kurapati wrote:
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>    drivers/usb/dwc3/core.c | 304 
>>>>>> +++++++++++++++++++++++++++++-----------
>>>>>>    drivers/usb/dwc3/core.h |  15 +-
>>>>>>    drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>    3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>>>>>> index 476b63618511..7e0a9a598dfd 100644
>>>>>> --- a/drivers/usb/dwc3/core.c
>>>>>> +++ b/drivers/usb/dwc3/core.c
>>>>>
>>>>> <snip>
>>>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 
>>>>>> *dwc)
>>>>>>        dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>                    "snps,dis-split-quirk");
>>>>>> +
>>>>>> +    /*
>>>>>> +     * If no mulitport properties are defined, default
>>>>>> +     * the port count to '1'.
>>>>>> +     */
>>>>>> +    ret = device_property_read_u32(dev, "num-ports",
>>>>>> +                &dwc->num_ports);
>>>>>> +    if (ret)
>>>>>> +        dwc->num_ports = 1;
>>>>>> +
>>>>>> +    ret = device_property_read_u32(dev, "num-ss-ports",
>>>>>> +                &dwc->num_ss_ports);
>>>>>> +    if (ret)
>>>>>> +        dwc->num_ss_ports = 1;
>>>>>
>>>>> By using this DT property instead of using the number of each phy 
>>>>> type you
>>>>> find you can get into situations where you're writing 
>>>>> DWC3_GUSB2PHYCFG, etc,
>>>>> when there's no phy to go along with it.
>>>>>
>>>> Hi Andrew,
>>>>
>>>>   Thanks for the review. Yes, this decoupling is still there and its 
>>>> fine I
>>>> believe.
>>>>
>>>>> I ran into this when testing on sa8540p-ride, which only uses one 
>>>>> of the
>>>>> ports on the multiport controller. I didn't enable the other phys (not
>>>>> sure if that was smart or not) and overrode phy-names/phys, but did 
>>>>> not
>>>>> override num-ports/num-ss-ports, which resulted in that. Nothing bad
>>>>> happened on a quick test.. but I thought I'd highlight that as another
>>>>> downside of decoupling this value from the number of phys you grab.
>>>>>
>>>> If we do not override phy-names or num-ports/num-ss-ports info in 
>>>> DT, they
>>>> are just defaulted to '1' and as per the current logic only port-1 
>>>> registers
>>>> must be configured. Isn't that the case happening ?
>>>>
>>>
>>> In my dts I'm inheriting from the sc8280xp.dtsi usb_2 phandle you've 
>>> created!
>>> So unless I override them I get this from your sc8280xp.dtsi:
>>>
>>> +                       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>;
>>> +                               num-ports = <4>;
>>> +                               num-ss-ports = <2>;
>>> +                               phys = <&usb_2_hsphy0>, 
>>> <&usb_2_qmpphy0>,
>>> +                                       <&usb_2_hsphy1>, 
>>> <&usb_2_qmpphy1>,
>>> +                                       <&usb_2_hsphy2>,
>>> +                                       <&usb_2_hsphy3>;
>>> +                               phy-names = "usb2-phy_port0", 
>>> "usb3-phy_port0",
>>> +                                               "usb2-phy_port1", 
>>> "usb3-phy_port1",
>>> +                                               "usb2-phy_port2",
>>> +                                               "usb2-phy_port3";
>>> +                       };
>>>
>>> Since this board only uses one port of the multiport controller, I
>>> redefined phys/phy-names to indicate that. I figured that was more
>>> desireable than enabling unnecessary phys. Without overriding
>>> num-ports/num-ss-ports all the for loops in this patch would act like
>>> the values were 4 and 2 respectively, writing to DWC3_GUSB2PHYCFG
>>> multiple times etc as well as look for the multiport phy-names and fail
>>> to actually get any phys. Hope that makes sense!
>>>
>> Hi Andrew,
>>
>>   My Bad. I missed the fact that it was based on sc8280xp.dtsi. In 
>> that case it makes complete sense to override the num-ports and 
>> num-ss-ports to "1" and the usb phy-names.
>>>>> Here's a patch enabling sa8540p-ride, I'd love if you'd add it to the
>>>>> series (probably needs clean up after review, and will definitely need
>>>>> alteration after you update the dt-binding again). If not I'll 
>>>>> continue
>>>>> to test/review so please CC me!:
>>>>>
>>>>>
>>>> Sure, I can add this patch (probably will add the other phy's too) 
>>>> during
>>>> the final submission.
>>>
>>> I don't have a great understanding of the mapping of the phys to
>>> physical connections (as well as what registers like DWC3_GUSB2PHYCFG 
>>> do),
>>> so if it makes more sense to enable all the relevant SoC phys, write
>>> those registers in the DWC3 IP, etc, and only use one of the actual
>>> board outputs then feel free. I think this is a good example of "what if
>>> a board designer only uses a single port of the multiport IP" imo.
>>> Agreed. This could be a good example of multi port with only single port 
> 
> Typo in the previous mail. Correcting it here.
> 
>> working.
> Agreed, The dt-patch you provided will be a good working example of 
> getting just a single port working for a multiport controller.
> 
> Regards,
> Krishna,
> 
>>>>
>>>>>   From dcb27d07f079194ebd7efe1c9bec64da78beb290 Mon Sep 17 00:00:00 
>>>>> 2001
>>>>> From: Andrew Halaney <ahalaney@redhat.com>
>>>>> Date: Thu, 19 Jan 2023 14:53:38 -0600
>>>>> Subject: [PATCH] arm64: dts: qcom: sa8540p-ride: Enable usb_2
>>>>> Content-type: text/plain
>>>>>
>>>>> 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.
>>>>>
>>>>> Signed-off-by: Andrew Halaney <ahalaney@redhat.com>
>>>>> ---
>>>>>    arch/arm64/boot/dts/qcom/sa8540p-ride.dts | 24 
>>>>> +++++++++++++++++++++++
>>>>>    1 file changed, 24 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts 
>>>>> b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> index 97957f3baa64..56d4f43faa1e 100644
>>>>> --- a/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> +++ b/arch/arm64/boot/dts/qcom/sa8540p-ride.dts
>>>>> @@ -246,6 +246,21 @@ &usb_0_qmpphy {
>>>>>        status = "okay";
>>>>>    };
>>>>> +&usb_2 {
>>>>> +    pinctrl-names = "default";
>>>>> +    pinctrl-0 = <&usb2_en_state>;
>>>>> +
>>>>> +    status = "okay";
>>>>> +};
>>>>> +
>>>>> +&usb_2_dwc3 {
>>>>> +    dr_mode = "host";
>>>>> +    num-ports = <1>;
>>>>> +    num-ss-ports = <1>;
>>>>
>>>> More over, if this is a multiport controller and you are using only 
>>>> port-1,
>>>> it is as good as a single port controller I believe and the normal DT
>>>> convention must work. Adding these properties as "1" is not required 
>>>> as the
>>>> driver logic defaults them to "1" if they are not found.
>>>
>>> See above comment about inheriting from sc8280xp.dtsi and needing to
>>> override their values.
>>>
>>>>
>>>> Just to add a point here (as I was not clear in DT Binding 
>>>> description, My
>>>> bad), the num-ports and num-ss-ports must indicate the HS/SS Phys 
>>>> present on
>>>> HW whether they are used in DT or not. Just to cover all cases which 
>>>> user
>>>> can use [1].
>>>>
>>>> []1:
>>>> https://lore.kernel.org/all/4eb26a54-148b-942f-01c6-64e66541de8b@quicinc.com/
>>>
>>> Ok, if you're going with that approach of "must indicate the HS/SS Phys
>>> present on HW whether they are used in the DT or not" (/me assumes DT
>>> here means on the board and not an incorrect coding of the DT) then I
>>> suppose I should not have overridden anything but phys/phy-names to
>>> indicate that I'm only using the first port (and used the multiport
>>> phy-names convention). It looks like in that link you also mention that
>>> it is ok to write to DWC3_GUSB2PHYCFG and friends even if the phy isn't
>>> defined, which was my concern and reasoning above for overriding
>>> num-ports/num-ss-ports.
>>>
>>> Thanks,
>>> Andrew
>>>
>> Actually, I was trying to mandate that rule to take care of cases 
>> where the phy's for say port2 or port3 are missing for a quad port 
>> controller in dtsi and we don't want to end up configuring wrong 
>> dwc3-phy regs.
>>
>> For just the first port, the changes you have mentioned must be 
>> sufficient. (Furthermore, thanks for the review and testing it on 
>> sa8295-ride and confirming nothing breaks while the first port is 
>> enabled)

I agree that for ride platform the changes provided with overridden
phys in DT for the sa8540-ride.dts makes more sense with only having
port0 of multi port on this platform.
Looking forward to validate the next version on ride platform.

Krishna, please keep me in cc in the next version :).

-Shazad

>>
>> Regards,
>> Krishna,
>>>>
>>>> Regards,
>>>> Krishna,
>>>>
>>>>> +    phy-names = "usb2-phy", "usb3-phy";
>>>>> +    phys = <&usb_2_hsphy0>, <&usb_2_qmpphy0>;
>>>>> +};
>>>>> +
>>>>>    &usb_2_hsphy0 {
>>>>>        vdda-pll-supply = <&vreg_l5a>;
>>>>>        vdda18-supply = <&vreg_l7g>;
>>>>> @@ -313,4 +328,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;
>>>>> +    };
>>>>>    };
>>>>
>>>

On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> On Fri, Jan 20, 2023, Krishna Kurapati PSSNV wrote:
>>
>>
>> On 1/20/2023 6:32 AM, Thinh Nguyen wrote:
>>> Hi,
>>>
>>> On Thu, Jan 19, 2023, Krishna Kurapati PSSNV wrote:
>>>>
>>>>
>>>> On 1/19/2023 6:06 AM, Thinh Nguyen wrote:
>>>>> Hi,
>>>>>
>>>>> On Sun, Jan 15, 2023, Krishna Kurapati wrote:
>>>>>> Currently the DWC3 driver supports only single port controller
>>>>>> which requires at most one HS and one SS PHY.
>>>>>
>>>>> Add note here that multi-port is for host mode for clarity.
>>>>>
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>>>>>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>>>>>> ---
>>>>>>     drivers/usb/dwc3/core.c | 304 +++++++++++++++++++++++++++++-----------
>>>>>>     drivers/usb/dwc3/core.h |  15 +-
>>>>>>     drivers/usb/dwc3/drd.c  |  14 +-
>>>>>>     3 files changed, 244 insertions(+), 89 deletions(-)
>>>>>>
>>>
>>> <snip>
>>>
>>>>>> @@ -1575,6 +1690,21 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>>>>>     	dwc->dis_split_quirk = device_property_read_bool(dev,
>>>>>>     				"snps,dis-split-quirk");
>>>>>> +
>>>>>> +	/*
>>>>>> +	 * If no mulitport properties are defined, default
>>>>>
>>>>> multi*
>>>>>
>>>>>> +	 * the port count to '1'.
>>>>>> +	 */
>>>>>
>>>>> Can we initialize num_ports and num_ss_ports to 1 instead of setting it
>>>>> on error (similar to how we handle other properties).
>>>>>
>>>> Hi Thinh,
>>>>
>>>>     Thanks for the review. On the bindings, Rob and Krzysztof have suggested
>>>> to get the num-ports and num-ss-ports by counting the Phy-names in DT.
>>>
>>> This may be a bit problematic for non-DT device. Currently pci devices
>>> pass fake DT properties to send these kinds of info. But that's fine,
>>> we can enhance dwc3 for non-DT devices later.
>>>
>>>>
>>>> Since there may be many cases where the user might skip giving any Phy's or
>>>> even skip different ports in the DT if he doesn't want to use them, can we
>>>> design/refactor the below logic as follows while mandating the fact that
>>>> user must give the SS Phy's if any starting from Port-0.:
>>>>
>>>> num-ss-ports = max_port_index (usb3-portX) + 1
>>>> num-ports = max (max_port_index(usb2-portX), num-ss-ports) + 1
>>>>
>>>> Ex: If there are 3 ports and only 1 is SS capable and user decides to skip
>>>> port-2 HS Phy.
>>>>
>>>> case-1: phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>>> case-2: phy-names = "usb2-port0", "usb2-port-1", "usb3-port1"
>>>>
>>>> In both cases, only one SS is present, just the order is changed. (Not sure
>>>> if last few ports can be made SS Capable instead of the first ports on any
>>>> HW) ?
>>>>
>>>> But according to the above formula:
>>>>
>>>> In case-1 : (num-ports = 2, num-ss-ports = 1) - This is correct
>>>> In case-2: (num-ports = 2, num-ss-ports = 2) - This is wrong
>>>>
>>>
>>> Can't we just walk through all the phy names to figure that out? Let's
>>> not require the user to specify Port-0 is SS capable if they can skip
>>> it.
>>>
>> Hi Thinh,
>>
>> Thanks for the review.
>>
>>    May be I wasn't able to convey my intention properly in my previous
>> thread. The above suggested method doesn't tell that user must not skip any
>> phy.
>>
>> Let us take the below case for 2 Port (HS+SS) capable controller.
>> If the user skips ss-phy 2, then:
>>
>> phy-names = "usb2-port0", "usb3-port0", "usb2-port-1"
>>
>> We don't need to configure GUSB3PIPECTL2 (for port-2) register ere. If we
>> parse phy-names and find it out, we see there are 2 hs-phy's and 1-ssphy and
>> num-ports = 2 and num-ss-ports = 1.
>>
>> If the user skips ss-phy-1, then phy-names would be something like the
>> below:
>>
>> phy-names = "usb2-port0", "usb2-port-1",  "usb3-port1";
>>
>> We need to handle two types of interpretations here while parsing the
>> phy-names:
>>
>> a) There are 2 SS Phy's and we just skipped the first one. In this scenario,
>> if we consider (num-ss-ports = 2) and (num-ports = 2) by using the above
>> formula as reference, we configure both GUSB3PIPECTL registers present in
>> the address map although we never use SS Phy-1 but nothing must break. All
>> ports would still work as the user intends with the exception of
>> GUSB3PIPECTL1 (for-port1) still being modified.
>>
>> b) The second interpretation is something like, port-1 is only HS capable
>> and only Port-2 is SS Capable, and so in the phy-names only port-2 has been
>> provided a SS Phy. Just by parsing through the phy-names, it would not be
>> possible to get that info. So if we consider num-ss-ports as 2 in this
>> scenario, we end up meddling with wrong registers (as there is only 1
>> GUSB3PIPECTL reg and we are assuing there are 2). I wanted to make sure that
>> this scenario was not possible.
>>
>> num-ss-ports = max_port_index (usb3-portX) + 1
>> num-ports = max (max_port_index(usb2-portX), max_port_index(usb2-portX)) + 1
>>
>> Taking case of a quad port controller with all ports SS Capable, if the user
>> wants to completely skip port-4. Then with above formula, we get (num-ports
>> = 3) and (num-ss-ports = 3) by parsing the phy-names and we will configure
>> exactly those dwc3-phy registers and not touch the port-4 registers which is
>> still fine.
>>
>> Please let me know if the above idea helps us in this scenario.
>>
> 
> This becomes rather more complicated because the user can skip certain
> port in the DT. We have access to the host registers. Can we just
> temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> capability before handing control over to the xHCI driver. We would be
> able to get the num_ports and num_ss_ports then.
> 
> Similarly, the xhci driver doesn't care whether the user skips certain
> port in the DT, it only checks and operates based on the capability
> registers.
> 
> If we have the exact num_ports and num_ss_ports, we can be sure the
> setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> 

Hi Thinh,

   Thanks for the suggestion. Is the following diff / implementation 
good enough ? I Wanted to get it clarified from upstream as I am using 
*ioremap/iounmap* directly instead of *devm_* API's

I tested it and it works fine on SA8295P. Will do some further testing 
on other devices as well.


+static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
+{
+       void __iomem            *regs;
+       struct resource         dwc_res;
+       unsigned int            hw_mode;
+       u32                     offset;
+       u32                     temp;
+       u8                      major_revision;
+       u8                      minor_revision;
+
+       /*
+        * Request memory region including xHCI regs,
+        * since it is needed to get port info
+        */
+       dwc_res = *res;
+       dwc_res.start += 0;
+
+       regs = ioremap(dwc_res.start, resource_size(&dwc_res));
+       if (IS_ERR(regs)) {
+               return PTR_ERR(regs);
+       }
+
+       /*
+        * If the controller is not host-only, then it must be a
+        * single port controller.
+        */
+       temp = readl(regs + DWC3_GHWPARAMS0);
+       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
+       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
+               dwc->num_ports = 1;
+               dwc->num_ss_ports = 1;
+               return 0;
+       }
+
+       offset = xhci_find_next_ext_cap(regs, 0,
+                                       XHCI_EXT_CAPS_PROTOCOL);
+       while (offset) {
+               temp = readl(regs + offset);
+               major_revision = XHCI_EXT_PORT_MAJOR(temp);;
+               minor_revision = XHCI_EXT_PORT_MINOR(temp);
+
+               temp = readl(regs + offset + 0x08);
+               if (major_revision == 0x03) {
+                       dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
+               } else if (major_revision <= 0x02) {
+                       dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
+               } else {
+                       dev_err(dwc->dev, "revision gone wrong\n");
+                       return -EINVAL;
+               }
+
+               offset = xhci_find_next_ext_cap(regs, offset,
+                                               XHCI_EXT_CAPS_PROTOCOL);
+       }
+
+       temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET);
+       if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
+               dev_err(dwc->dev, "inconsistency in port info\n");
+               return -EINVAL;
+       }
+
+       dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n", 
dwc->num_ports, dwc->num_ss_ports);
+       iounmap(regs);
+       return 0;
+}
+
  static int dwc3_probe(struct platform_device *pdev)
  {
         struct device           *dev = &pdev->dev;
@@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev)
         dwc->xhci_resources[0].flags = res->flags;
         dwc->xhci_resources[0].name = res->name;

+       ret = dwc3_read_port_info(dwc, res);
+       if (ret)
+               return ret;
+
         /*
          * Request memory region but exclude xHCI regs,
          * since it will be requested by the xhci-plat driver.
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 2f82eda9d44f..8535425b81d4 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -38,6 +38,9 @@
  /* Numer of ports supported by a multiport controller */
  #define MAX_PORTS_SUPPORTED    4

+/* XHCI Reg constants */
+#define DWC3_XHCI_HCSPARAMS1_OFFSET    0x04
+
  /* Global constants */
  #define DWC3_PULL_UP_TIMEOUT   500     /* ms */
  #define DWC3_BOUNCE_SIZE       1024    /* size of a superspeed bulk */



Please let me know if this would be acceptable.

BR,
Krishna,

On Wed, Jan 25, 2023, Krishna Kurapati PSSNV wrote:
> 
> 
> On 1/21/2023 4:14 AM, Thinh Nguyen wrote:
> > 
> > This becomes rather more complicated because the user can skip certain
> > port in the DT. We have access to the host registers. Can we just
> > temporarily map and access HCSPARAMS1 to get the MAXPORTS and each port
> > capability before handing control over to the xHCI driver. We would be
> > able to get the num_ports and num_ss_ports then.
> > 
> > Similarly, the xhci driver doesn't care whether the user skips certain
> > port in the DT, it only checks and operates based on the capability
> > registers.
> > 
> > If we have the exact num_ports and num_ss_ports, we can be sure the
> > setting to GUSB3PIPECTLn and GUSB2PHYCFGn are valid.
> > 
> 
> Hi Thinh,
> 
>   Thanks for the suggestion. Is the following diff / implementation good
> enough ? I Wanted to get it clarified from upstream as I am using
> *ioremap/iounmap* directly instead of *devm_* API's
> 
> I tested it and it works fine on SA8295P. Will do some further testing on
> other devices as well.
> 
> 
> +static int dwc3_read_port_info(struct dwc3 *dwc, struct resource *res)
> +{
> +       void __iomem            *regs;
> +       struct resource         dwc_res;
> +       unsigned int            hw_mode;
> +       u32                     offset;
> +       u32                     temp;
> +       u8                      major_revision;
> +       u8                      minor_revision;
> +
> +       /*
> +        * Request memory region including xHCI regs,
> +        * since it is needed to get port info
> +        */
> +       dwc_res = *res;
> +       dwc_res.start += 0;
> +
> +       regs = ioremap(dwc_res.start, resource_size(&dwc_res));
> +       if (IS_ERR(regs)) {
> +               return PTR_ERR(regs);
> +       }

We don't need to ioremap the whole region. Just do it for
the xhci_resources[0]

> +
> +       /*
> +        * If the controller is not host-only, then it must be a
> +        * single port controller.
> +        */
> +       temp = readl(regs + DWC3_GHWPARAMS0);
> +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> +               dwc->num_ports = 1;
> +               dwc->num_ss_ports = 1;
> +               return 0;
> +       }

This check should be done before we get into this function.

> +
> +       offset = xhci_find_next_ext_cap(regs, 0,
> +                                       XHCI_EXT_CAPS_PROTOCOL);
> +       while (offset) {
> +               temp = readl(regs + offset);
> +               major_revision = XHCI_EXT_PORT_MAJOR(temp);;
> +               minor_revision = XHCI_EXT_PORT_MINOR(temp);

We probably don't need minor revision.

> +
> +               temp = readl(regs + offset + 0x08);
> +               if (major_revision == 0x03) {
> +                       dwc->num_ss_ports += XHCI_EXT_PORT_COUNT(temp);
> +               } else if (major_revision <= 0x02) {
> +                       dwc->num_ports += XHCI_EXT_PORT_COUNT(temp);
> +               } else {
> +                       dev_err(dwc->dev, "revision gone wrong\n");
> +                       return -EINVAL;
> +               }
> +
> +               offset = xhci_find_next_ext_cap(regs, offset,
> +                                               XHCI_EXT_CAPS_PROTOCOL);
> +       }
> +
> +       temp = readl(regs + DWC3_XHCI_HCSPARAMS1_OFFSET);
> +       if (HCS_MAX_PORTS(temp) != (dwc->num_ss_ports + dwc->num_ports)) {
> +               dev_err(dwc->dev, "inconsistency in port info\n");
> +               return -EINVAL;
> +       }
> +
> +       dev_info(dwc->dev, "num_ports: %d, num_ss_ports: %d\n",
> dwc->num_ports, dwc->num_ss_ports);
> +       iounmap(regs);

Make sure to iounmap on all the early return/error cases.

> +       return 0;
> +}
> +
>  static int dwc3_probe(struct platform_device *pdev)
>  {
>         struct device           *dev = &pdev->dev;
> @@ -1912,6 +1964,10 @@ static int dwc3_probe(struct platform_device *pdev)
>         dwc->xhci_resources[0].flags = res->flags;
>         dwc->xhci_resources[0].name = res->name;
> 
> +       ret = dwc3_read_port_info(dwc, res);

This should be called after some initializations to make sure some
clocks are enabled. Otherwise some devices may not able to access the
registers. Preferably after dwc3_cache_hwparams() but before
dwc3_core_init().

> +       if (ret)
> +               return ret;
> +
>         /*
>          * Request memory region but exclude xHCI regs,
>          * since it will be requested by the xhci-plat driver.
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 2f82eda9d44f..8535425b81d4 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -38,6 +38,9 @@
>  /* Numer of ports supported by a multiport controller */
>  #define MAX_PORTS_SUPPORTED    4
> 
> +/* XHCI Reg constants */
> +#define DWC3_XHCI_HCSPARAMS1_OFFSET    0x04

Change to DWC3_XHCI_HCSPARAMS1

> +
>  /* Global constants */
>  #define DWC3_PULL_UP_TIMEOUT   500     /* ms */
>  #define DWC3_BOUNCE_SIZE       1024    /* size of a superspeed bulk */
> 
> 
> 
> Please let me know if this would be acceptable.
> 

It looks fine to me. Please review the comments and remmove debug
prints and any other cleanup for a proper patch.

Thanks,
Thinh

On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote:

<snip>

> > +       /*
> > +        * If the controller is not host-only, then it must be a
> > +        * single port controller.
> > +        */

Thinh, is this a correct assumption?  Is it possible for the IP to be
synthesized to support both dual-role and multiple ports?  We know that
when operating in device mode only the first port can be used but the
additional ports would be usable when in host.

Thanks,
Jack

> > +       temp = readl(regs + DWC3_GHWPARAMS0);
> > +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> > +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> > +               dwc->num_ports = 1;
> > +               dwc->num_ss_ports = 1;
> > +               return 0;
> > +       }
> 
> This check should be done before we get into this function.

On Wed, Jan 25, 2023, Jack Pham wrote:
> On Wed, Jan 25, 2023 at 07:08:10PM +0000, Thinh Nguyen wrote:
> 
> <snip>
> 
> > > +       /*
> > > +        * If the controller is not host-only, then it must be a
> > > +        * single port controller.
> > > +        */
> 
> Thinh, is this a correct assumption?  Is it possible for the IP to be
> synthesized to support both dual-role and multiple ports?  We know that
> when operating in device mode only the first port can be used but the
> additional ports would be usable when in host.
> 

Yes. Maybe we should rephrase it a bit. Perhaps something like this:
"Currently, only host mode supports multi-port"

In case this may change in the future.

BR,
Thinh


> 
> > > +       temp = readl(regs + DWC3_GHWPARAMS0);
> > > +       hw_mode = DWC3_GHWPARAMS0_MODE(temp);
> > > +       if (hw_mode != DWC3_GHWPARAMS0_MODE_HOST) {
> > > +               dwc->num_ports = 1;
> > > +               dwc->num_ss_ports = 1;
> > > +               return 0;
> > > +       }
> > 
> > This check should be done before we get into this function.