Message-ID: <ZLo6MwbuKNL5xtPE@hovoldconsulting.com>
Date:   Fri, 21 Jul 2023 09:56:35 +0200
From:   Johan Hovold <johan@...nel.org>
To:     Krishna Kurapati PSSNV <quic_kriskura@...cinc.com>
Cc:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Andy Gross <agross@...nel.org>,
        Konrad Dybcio <konrad.dybcio@...aro.org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Felipe Balbi <balbi@...nel.org>,
        Wesley Cheng <quic_wcheng@...cinc.com>,
        linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arm-msm@...r.kernel.org, devicetree@...r.kernel.org,
        quic_pkondeti@...cinc.com, quic_ppratap@...cinc.com,
        quic_jackp@...cinc.com, quic_harshq@...cinc.com,
        ahalaney@...hat.com, quic_shazhuss@...cinc.com
Subject: Re: [PATCH v9 05/10] usb: dwc3: core: Refactor PHY logic to support
 Multiport Controller

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@...cinc.com>
> >> [Krishna: Modifed logic for generic phy and rebased the patch]
> >> Signed-off-by: Krishna Kurapati <quic_kriskura@...cinc.com>
> > 
> > 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.

Johan

Powered by blists - more mailing lists