Re: [PATCH v13 03/10] usb: dwc3: core: Refactor PHY logic to support Multiport Controller

[Krishna Kurapati PSSNV <quic_kriskura@quicinc.com>]

On 10/20/2023 3:27 PM, Johan Hovold wrote:
> On Sat, Oct 07, 2023 at 09:17:59PM +0530, Krishna Kurapati wrote:
>> From: Harsh Agarwal <quic_harshq@quicinc.com>
>>
>> Currently the DWC3 driver supports only single port controller
>> which requires at most one HS and one SS PHY.
> 
> Should that not be "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 phy's supported
> 
> "PHYs" for consistency, no apostrophe
> 
>> by a multiport controller and. Limit the max number of ports
> 
> "and." what? Looks like part of the sentence is missing? Or just drop
> " and"?
> 
>> supported to 4 as only SC8280 which is a quad port controller supports
> 
> s/4/four/
> 
> Just change this to
> 
> 	Limit support to multiport controllers with up to four ports for
> 	now (e.g. as needed for SC8280XP).
> 
>> Multiport currently.
> 
> You use capitalised "Multiport" in several places it seems. Is this an
> established term for these controllers or should it just be "multiport"
> or "multiple ports"?
> 
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202309200156.CxQ3yaLY-lkp@intel.com/
> 
> Drop these two lines, as people have already suggested.
> 
>> Co-developed-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Signed-off-by: Harsh Agarwal <quic_harshq@quicinc.com>
>> Co-developed-by:Krishna Kurapati <quic_kriskura@quicinc.com>
>> Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com>
> 
> Thinh pointed out the problems with the above which were also reported
> by checkpatch.pl.
> 

I see that removing Co-Developed by tag for Harsh is helping with one 
checkpatch issue. From what I know, although I made Harsh the Primary 
author for the patch, should we still mention his Co-Developed-by along 
with this Signed-Of by ?

>> ---
>> Changes in v13:
>> Compiler issues found by kernel test robot have been fixed and tags added.
>> So removing maintainers reviewed-by tag as we have made a minor change
>> in the patch.
> 
> In general this is the right thing to do when the change in question was
> non-trivial. I'm not sure that's the case here, but the robots tend to
> complain about smaller (but sometimes important) things.
> 

I too had this uncertainity, but I see that we must not add maintainers 
reviewed by tag if we even make one letter of change. Wanted to adhere 
to these rules and so removed Thinh's tag and asked for re-review.

>> @@ -748,23 +781,32 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
>>   static int dwc3_phy_init(struct dwc3 *dwc)
>>   {
>>   	int ret;
>> +	int i;
>> +	int j;
> 
> These could be declared on one line (same throughout).
> 

I did so in v7, but was asked to put them in separate lines:
https://lore.kernel.org/all/20230502221100.ecska23anlzv3iwq@synopsys.com/


>>   	usb_phy_init(dwc->usb2_phy);
>>   	usb_phy_init(dwc->usb3_phy);

				dwc->usb2_generic_phy[i] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret,
>> +					"failed to lookup phy %s\n", phy_name);
> 
> Continuation lines should be intented at least two tabs further.
> 
> I generally suggest adding brackets around blocks with multiline
> statements to improve readability but if you move the string to the
> previous line and intend the continuation line (phy_name) one tab more I
> guess that's fine.
> 
>> +		}
>> +
>> +		if (dwc->num_usb2_ports == 1)
>> +			sprintf(phy_name, "usb3-phy");
>>   		else
>> -			return dev_err_probe(dev, ret, "no usb3 phy configured\n");
>> +			sprintf(phy_name, "usb3-port%d", i);
>> +
>> +		dwc->usb3_generic_phy[i] = devm_phy_get(dev, phy_name);
>> +		if (IS_ERR(dwc->usb3_generic_phy[i])) {
>> +			ret = PTR_ERR(dwc->usb3_generic_phy[i]);
>> +			if (ret == -ENOSYS || ret == -ENODEV)
>> +				dwc->usb3_generic_phy[i] = NULL;
>> +			else
>> +				return dev_err_probe(dev, ret,
>> +					"failed to lookup phy %s\n", phy_name);
> 
> Same here.
> 
>> +		}
>>   	}
>>   
>>   	return 0;
> 
>> @@ -1892,9 +1975,12 @@ static int dwc3_read_port_info(struct dwc3 *dwc)
>>   
>>   	dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n",
>>   			dwc->num_usb2_ports, dwc->num_usb3_ports);
>> -
> 
> Drop this random change.

The removal of extra line here done you mean ?

> 
>>   	iounmap(base);
>>   
>> +	if ((dwc->num_usb2_ports > DWC3_MAX_PORTS) ||
>> +		(dwc->num_usb3_ports > DWC3_MAX_PORTS))
> 
> Again, continuation lines should be indented at least two tabs further.
> 
>> +		return -ENOMEM;
>> +
>>   	return 0;
>>   }
> 
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index 2ea6df7e6571..fc5d15edab1c 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -33,6 +33,9 @@
>>   
>>   #include <linux/power_supply.h>
>>   
>> +/* Number of ports supported by a multiport controller */
> 
> 	/*
> 	 * Maximum number of ports currently supported for multiport
> 	 * controllers.
> 	 */
> 
>> +#define DWC3_MAX_PORTS 4
>> +
>>   #define DWC3_MSG_MAX	500
>>   
>>   /* Global constants */
>> @@ -1029,8 +1032,8 @@ struct dwc3_scratchpad_array {
>>    * @usb_psy: pointer to power supply interface.
>>    * @usb2_phy: pointer to USB2 PHY
>>    * @usb3_phy: pointer to USB3 PHY
>> - * @usb2_generic_phy: pointer to USB2 PHY
>> - * @usb3_generic_phy: pointer to USB3 PHY
>> + * @usb2_generic_phy: pointer to array of USB2 PHY
>> + * @usb3_generic_phy: pointer to array of USB3 PHY
> 
> s/PHY/PHYs/
> 
>>    * @num_usb2_ports: number of USB2 ports
>>    * @num_usb3_ports: number of USB3 ports
>>    * @phys_ready: flag to indicate that PHYs are ready
> 
> Johan

Thanks for the review. Will fix these nits in v14.

Regards,
Krishna,