Re: [PATCH v9 05/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 6/27/2023 5:39 PM, Johan Hovold wrote:
> On Wed, Jun 21, 2023 at 10:06:23AM +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>
>> [Krishna: Modifed logic for generic phy and rebased the patch]
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> 
> As I already said:
> 
> 	If Harsh is the primary author you need to add a From: line at
> 	the beginning of the patch.
> 
> 	Either way, you need his SoB as well as your Co-developed-by tag.
> 
> 	All this is documented under Documentation/process/ somewhere.
> 
> The above is missing a From line and two Co-developed-by tags at least.
> 
Hi Johan,

  I tried to follow the following commit:

8030cb9a5568 ("soc: qcom: aoss: remove spurious IRQF_ONESHOT flags")

Let me know if that is not acceptable.

>> ---
>>   drivers/usb/dwc3/core.c | 252 ++++++++++++++++++++++++++++------------
>>   drivers/usb/dwc3/core.h |  11 +-
>>   drivers/usb/dwc3/drd.c  |  15 ++-
>>   3 files changed, 192 insertions(+), 86 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index e1ebae54454f..2ac72525de7d 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -120,10 +120,11 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode)
>>   static void __dwc3_set_mode(struct work_struct *work)
>>   {
>>   	struct dwc3 *dwc = work_to_dwc(work);
>> +	u32 desired_dr_role;
>>   	unsigned long flags;
>>   	int ret;
>>   	u32 reg;
>> -	u32 desired_dr_role;
> 
> This is an unrelated change. Just add int i at the end.
> 
I was trying to keep the reverse xmas order of variables.

>> +	int i;
>>   
>>   	mutex_lock(&dwc->mutex);
>>   	spin_lock_irqsave(&dwc->lock, flags);
> 
>> @@ -746,23 +779,34 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   static int dwc3_phy_init(struct dwc3 *dwc)
>>   {
>>   	int ret;
>> +	int i;
>> +	int j;
>>   
>>   	usb_phy_init(dwc->usb2_phy);
>>   	usb_phy_init(dwc->usb3_phy);
>>   
>> -	ret = phy_init(dwc->usb2_generic_phy);
>> -	if (ret < 0)
>> -		goto err_shutdown_usb3_phy;
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>> +		ret = phy_init(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0)
>> +			goto err_exit_usb2_phy;
>> +	}
>>   
>> -	ret = phy_init(dwc->usb3_generic_phy);
>> -	if (ret < 0)
>> -		goto err_exit_usb2_phy;
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>> +		ret = phy_init(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0)
>> +			goto err_exit_usb3_phy;
>> +	}
>>   
>>   	return 0;
>>   
>> +err_exit_usb3_phy:
>> +	for (j = i-1; j >= 0; j--)
> 
> Missing spaces around - here and below.
> 
>> +		phy_exit(dwc->usb3_generic_phy[j]);
>> +	i = dwc->num_usb2_ports;
>>   err_exit_usb2_phy:
>> -	phy_exit(dwc->usb2_generic_phy);
>> -err_shutdown_usb3_phy:
>> +	for (j = i-1; j >= 0; j--)
>> +		phy_exit(dwc->usb2_generic_phy[j]);
>> +
> 
> Again:
> 
> 	The above is probably better implemented as a *single* loop over
> 	num_usb2_ports where you enable each USB2 and USB3 PHY. On
> 	errors you use the loop index to disable the already enabled
> 	PHYs in reverse order below (after disabling the USB2 PHY if
> 	USB3 phy init fails).
> 
> with emphasis on "single" added.
> 
Oh, you mean something like this ?

for (loop over num_ports) {
	ret = phy_init(dwc->usb3_generic_phy[i]);
	if (ret != 0)
		goto err_exit_phy;

	ret = phy_init(dwc->usb2_generic_phy[i]);
	if (ret != 0)
		goto err_exit_phy;
}

err_exit_phy:
	for (j = i-1; j >= 0; j--) {
		phy_exit(dwc->usb3_generic_phy[j]);
		phy_exit(dwc->usb2_generic_phy[j]);
	}

>>   	usb_phy_shutdown(dwc->usb3_phy);
>>   	usb_phy_shutdown(dwc->usb2_phy);
> 
>> @@ -781,23 +829,34 @@ static void dwc3_phy_exit(struct dwc3 *dwc)
>>   static int dwc3_phy_power_on(struct dwc3 *dwc)
>>   {
>>   	int ret;
>> +	int i;
>> +	int j;
>>   
>>   	usb_phy_set_suspend(dwc->usb2_phy, 0);
>>   	usb_phy_set_suspend(dwc->usb3_phy, 0);
>>   
>> -	ret = phy_power_on(dwc->usb2_generic_phy);
>> -	if (ret < 0)
>> -		goto err_suspend_usb3_phy;
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>> +		ret = phy_power_on(dwc->usb2_generic_phy[i]);
>> +		if (ret < 0)
>> +			goto err_power_off_usb2_phy;
>> +	}
>>   
>> -	ret = phy_power_on(dwc->usb3_generic_phy);
>> -	if (ret < 0)
>> -		goto err_power_off_usb2_phy;
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>> +		ret = phy_power_on(dwc->usb3_generic_phy[i]);
>> +		if (ret < 0)
>> +			goto err_power_off_usb3_phy;
>> +	}
> 
> Again: These loops should be merged too as for phy_init.
> 
>>   	return 0;
>>   
>> +err_power_off_usb3_phy:
>> +	for (j = i-1; j >= 0; j--)
>> +		phy_power_off(dwc->usb3_generic_phy[i]);
>> +	i = dwc->num_usb2_ports;
>>   err_power_off_usb2_phy:
>> -	phy_power_off(dwc->usb2_generic_phy);
>> -err_suspend_usb3_phy:
>> +	for (j = i-1; j >= 0; j--)
>> +		phy_power_off(dwc->usb2_generic_phy[i]);
>> +
>>   	usb_phy_set_suspend(dwc->usb3_phy, 1);
>>   	usb_phy_set_suspend(dwc->usb2_phy, 1);
>>   
> 
>> @@ -1290,7 +1358,9 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   {
>>   	struct device		*dev = dwc->dev;
>>   	struct device_node	*node = dev->of_node;
>> +	char phy_name[11];
>>   	int ret;
>> +	int i;
>>   
>>   	if (node) {
>>   		dwc->usb2_phy = devm_usb_get_phy_by_phandle(dev, "usb-phy", 0);
>> @@ -1316,22 +1386,36 @@ static int dwc3_core_get_phy(struct dwc3 *dwc)
>>   			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>>   	}
>>   
>> -	dwc->usb2_generic_phy = devm_phy_get(dev, "usb2-phy");
>> -	if (IS_ERR(dwc->usb2_generic_phy)) {
>> -		ret = PTR_ERR(dwc->usb2_generic_phy);
>> -		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb2_generic_phy = NULL;
>> +	for (i = 0; i < dwc->num_usb2_ports; i++) {
>> +		if (dwc->num_usb2_ports == 1)
>> +			sprintf(phy_name, "usb2-phy");
>>   		else
>> -			return dev_err_probe(dev, ret, "no usb2 phy configured\n");
>> -	}
>> +			sprintf(phy_name, "usb2-port%d", i);
>> +
>> +		dwc->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,
>> +					"no %s phy configured\n", phy_name);
> 
> I still believe
> 
> 	"failed to lookup phy %s"
> 
> is better.
> 
>> +		}
>>   
>> -	dwc->usb3_generic_phy = devm_phy_get(dev, "usb3-phy");
>> -	if (IS_ERR(dwc->usb3_generic_phy)) {
>> -		ret = PTR_ERR(dwc->usb3_generic_phy);
>> -		if (ret == -ENOSYS || ret == -ENODEV)
>> -			dwc->usb3_generic_phy = NULL;
>> +		if (dwc->num_usb2_ports == 1)
>> +			sprintf(phy_name, "usb3-phy");
>>   		else
>> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +			sprintf(phy_name, "usb3-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,
>> +					"no %s phy configured\n", phy_name);
> 
> Same here.
> 
>> +		}
>>   	}
>>   
>>   	return 0;
> 
>> @@ -1821,6 +1908,9 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
>>   	dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
>>   			dwc->num_usb2_ports, dwc->num_usb3_ports);
>>   
>> +	if (dwc->num_usb2_ports > DWC3_MAX_PORTS)
>> +		ret = -ENOMEM;
> 
> You also need to add a check for num_usb3_ports as I already mentioned.
> 
>> +
>>   	iounmap(base);
>>   	return ret;
>>   }
> 
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 42fb17aa66fa..b2bab23ca22b 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -37,6 +37,9 @@
>>   #define XHCI_EXT_PORT_MINOR(x)	(((x) >> 16) & 0xff)
>>   #define XHCI_EXT_PORT_COUNT(x)	(((x) >> 8) & 0xff)
>>   
>> +/* Number of ports supported by a multiport controller */
>> +#define DWC3_MAX_PORTS 4
> 
> You did not answer my question about whether this was an arbitrary
> implementation limit (i.e. just reflecting the only currently supported
> multiport controller)?
> 
I mentioned in commit text that it is limited to 4. Are you referring to 
state the reason why I chose the value 4 ?

>> +
>>   #define DWC3_MSG_MAX	500
>>   
>>   /* Global constants */
>> @@ -1031,8 +1034,8 @@ struct dwc3_scratchpad_array {
>>    * @usb_psy: pointer to power supply interface.
>>    * @usb2_phy: pointer to USB2 PHY
>>    * @usb3_phy: pointer to USB3 PHY
>> - * @usb2_generic_phy: pointer to USB2 PHY
>> - * @usb3_generic_phy: pointer to USB3 PHY
>> + * @usb2_generic_phy: pointer to array of USB2 PHY
>> + * @usb3_generic_phy: pointer to array of USB3 PHY
>>    * @num_usb2_ports: number of USB2 ports.
>>    * @num_usb3_ports: number of USB3 ports.
>>    * @phys_ready: flag to indicate that PHYs are ready
>> @@ -1171,8 +1174,8 @@ struct dwc3 {
>>   	struct usb_phy		*usb2_phy;
>>   	struct usb_phy		*usb3_phy;
>>   
>> -	struct phy		*usb2_generic_phy;
>> -	struct phy		*usb3_generic_phy;
>> +	struct phy		*usb2_generic_phy[DWC3_MAX_PORTS];
>> +	struct phy		*usb3_generic_phy[DWC3_MAX_PORTS];
>>   
>>   	u8			num_usb2_ports;
>>   	u8			num_usb3_ports;
>> diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c
>> index 039bf241769a..18a247ff75ac 100644
>> --- a/drivers/usb/dwc3/drd.c
>> +++ b/drivers/usb/dwc3/drd.c
>> @@ -327,10 +327,11 @@ static void dwc3_otg_device_exit(struct dwc3 *dwc)
>>   
>>   void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus)
>>   {
>> +	unsigned long flags;
>>   	int ret;
>>   	u32 reg;
>>   	int id;
>> -	unsigned long flags;
>> +	int i;
>>   
>>   	if (dwc->dr_mode != USB_DR_MODE_OTG)
>>   		return;
>> @@ -386,9 +387,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_usb2_ports; i++) {
>> +				if (dwc->usb2_generic_phy[i])
>> +					phy_set_mode(dwc->usb2_generic_phy[i],
>> +						     PHY_MODE_USB_HOST);
> 
> While not strictly necessary, adding bracket around multiline statements
> is usually preferred as it improves readability.
> 
>> +			}
>>   		}
>>   		break;
>>   	case DWC3_OTG_ROLE_DEVICE:
>> @@ -400,8 +403,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);
> 
> Same here, but this is probably better just left at 85 columns after
> removing the line break.
> 
>>   		ret = dwc3_gadget_init(dwc);
>>   		if (ret)
> 
> Johan

Thanks,
Krishna,