Messages in this thread | | | Date | Wed, 10 Apr 2024 10:10:54 +0530 | Subject | Re: [PATCH v20 4/9] usb: dwc3: core: Refactor PHY logic to support Multiport Controller | From | Krishna Kurapati PSSNV <> |
| |
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,
| |