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 11:43 PM, Thinh Nguyen wrote:
> On Tue, Apr 09, 2024, Krishna Kurapati PSSNV wrote:
>>
>>
>> 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
> 
> That's not what you described in the comment right above the macro...
> 
>> 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
> 
> Those fields are clear. But for DWC3_MAX_PORTS, based on the name and
> comment of the macro, it's not clear.
> 
>> 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.
> 
> The error code -ENOMEM indicates out of memory failure. The check
> condition dwc->num_usb2_ports > DWC3_MAX_PORTS indicates invalid config.
> There's no allocation in that check.
> 
>>
>>> [*] https://urldefense.com/v3/__https://lore.kernel.org/linux-usb/20230801013031.ft3zpoatiyfegmh6@synopsys.com/__;!!A4F2R9G_pg!azHqgm92ENkFQrpv6Fhs6PCe210VGOAIrsuGFhrgmfaor8N_kWLu6rxkPpbeCBTLL4NbUpOWlQ0ufmP9DFwO9iFc0XdSEg$
>>>
>>>> + */
>>>> +#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).
>>
> 
> That is fine with me. Thanks for your effort pursuing and continue
> working on this series.
> 

Thanks Thinh. If there are no other issues, I will wait till Greg picks 
the series up. Thanks for the reviews throughout the series.

Regards,
Krishna,