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

From: Krishna Kurapati PSSNV
Date: Fri Oct 20 2023 - 07:44:48 EST



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@xxxxxxxxxxx>

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@xxxxxxxxx>
Closes: https://lore.kernel.org/oe-kbuild-all/202309200156.CxQ3yaLY-lkp@xxxxxxxxx/

Drop these two lines, as people have already suggested.

Co-developed-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx>
Signed-off-by: Harsh Agarwal <quic_harshq@xxxxxxxxxxx>
Co-developed-by:Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>
Signed-off-by: Krishna Kurapati <quic_kriskura@xxxxxxxxxxx>

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@xxxxxxxxxxxx/


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,