On Fri, Jul 21, 2023, Johan Hovold wrote:
> On Mon, Jul 03, 2023 at 12:26:26AM +0530, Krishna Kurapati PSSNV wrote:
> > 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@xxxxxxxxxxx>
> > >> [Krishna: Modifed logic for generic phy and rebased the patch]
> > >> Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
> > >
> > > 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.
>
> > I tried to follow the following commit:
> >
> > 8030cb9a5568 ("soc: qcom: aoss: remove spurious IRQF_ONESHOT flags")
> >
> > Let me know if that is not acceptable.
>
> I don't see how that commit relevant to the discussion at hand.
>
> Please just fix your use of Signed-off-by and Co-developed-by tags that
> I've now pointed out repeatedly.
>
> If you can't figure it out by yourself after the feedback I've already
> given you need to ask someone inside Qualcomm. You work for a huge
> company that should provide resources for training it's developers in
> basic process issues like this.
>
> > >> @@ -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.
>
> That's generally good, but you should not change unrelated code as part
> of this patch. It's fine to leave this as is for now.
>
> > >> + 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]);
> > }
>
> Yeah, something like that, but you need to disable the usb3[i] phy when
> usb2[2] init fail above (and I'd also keep the order of initialising
> usb2 before usb3).
>
> > >> usb_phy_shutdown(dwc->usb3_phy);
> > >> usb_phy_shutdown(dwc->usb2_phy);
>
> > >> 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 ?
>
> Yes, and to clarify whether this was an arbitrary limit you chose
> because it was all that was needed for the hw you care about, or if it's
> a more general limitation.
>
Note: We can support many, but we set the current limit to 4 usb3 ports
and up to 15 usb2 ports.
BR,
Thinh
