Re: [PATCH v20 4/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

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

On 4/9/2024 6:41 AM, Thinh Nguyen wrote:
> On Mon, Apr 08, 2024, Krishna Kurapati wrote:
>> Currently the DWC3 driver supports only single port controller
>> which requires at least one HS PHY and at most one SS PHY.
>>
>> But the DWC3 USB controller can be connected to multiple ports and
>> each port can have their own PHYs. Each port of the multiport
>> controller can either be HS+SS capable or HS only capable
>> Proper quantification of them is required to modify GUSB2PHYCFG
>> and GUSB3PIPECTL registers appropriately.
>>
>> Add support for detecting, obtaining and configuring PHYs supported
>> by a multiport controller. Limit support to multiport controllers
>> with up to four ports for now (e.g. as needed for SC8280XP).
>>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
>> Reviewed-by: Johan Hovold <johan+linaro@kernel.org>
>> ---
>>   drivers/usb/dwc3/core.c | 251 ++++++++++++++++++++++++++++------------
>>   drivers/usb/dwc3/core.h |  14 ++-
>>   drivers/usb/dwc3/drd.c  |  15 ++-
>>   3 files changed, 193 insertions(+), 87 deletions(-)
>>
> 
> <snip>
> 
>> @@ -1937,6 +2020,10 @@ static int dwc3_get_num_ports(struct dwc3 *dwc)
>>   
>>   	iounmap(base);
>>   
>> +	if (dwc->num_usb2_ports > DWC3_MAX_PORTS ||
>> +	    dwc->num_usb3_ports > DWC3_MAX_PORTS)
>> +		return -ENOMEM;
> 
> This should be -EINVAL.
> 
>> +
>>   	return 0;
>>   }
> 
> <snip>
> 
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 341e4c73cb2e..df2e111aa848 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -33,6 +33,12 @@
>>   
>>   #include <linux/power_supply.h>
>>   
>> +/*
>> + * Maximum number of ports currently supported for multiport
>> + * controllers.
> 
> This macro here is being used per USB2 vs USB3 ports rather than USB2 +
> USB3, unlike the xHCI MAXPORTS. You can clarify in the comment and
> rename the macro to avoid any confusion. You can also create 2 separate
> macros for number of USB2 and USB3 ports even if they share the same
> value.
> 
> As noted[*], we support have different max number of usb2 ports vs usb3
> ports. I would suggest splitting the macros.
> 

Hi Thinh,

  This macro was intended only to identify how many USB2 (or USB3) Phy's 
were serviced/operated by this driver, not how many logical ports 
present (like in xHCI). I don't think it would be confusing currently 
given that it is only used to identify number of generic phy instances 
to allocate and not used for any other purpose. Once the num_usb2_ports 
and num_usb3_ports are read by get_num_ports(...) call, they directly 
indicate how many ports are HS and SS respectively. Keeping the same in 
mind, I returned ENOMEM above (as you mentioned) because we don't 
allocate more than DWC3_MAX_PORTS and if the number of hs or ss ports is 
more than that, we simply return ENOMEM saying the driver doesn't 
support operating those many phy's.

> [*] https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@synopsys.com/
> 
>> + */
>> +#define DWC3_MAX_PORTS 4
>> +
>>
> 
> But it's not a big issue whether you decided to push a new version or a
> create a separate patch for the comments above. Here's my Ack:
> 

Since this is not a bug, I would prefer to make a separate patch to 
rename the macros. (If that is fine).

> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com>
> 
> Thanks,
> Thinh