by Krishna Kurapati PSSNV

[permalink] [raw]

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

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 <[email protected]>
>>>> Reviewed-by: Johan Hovold <[email protected]>
>>>> ---
>>>> 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/[email protected]/__;!!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,

2024-04-11 12:20:35

by Greg KH

[permalink] [raw]

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

On Wed, Apr 10, 2024 at 10:10:54AM +0530, Krishna Kurapati PSSNV wrote:
>
>
> 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 <[email protected]>
> > > > > Reviewed-by: Johan Hovold <[email protected]>
> > > > > ---
> > > > > 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/[email protected]/__;!!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.

I can't take it yet, based on the review of this patch, so I'll wait for
a new version of the series.

thanks,

greg k-h