| Message ID | 20240216005756.762712-3-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote: > Currently Multiport DWC3 controllers are host-only capable. I already asked you to rephrase this so that it becomes clear that you are describing a property of the current hardware (and similar throughout the series): https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/ > +static int dwc3_read_port_info(struct dwc3 *dwc) > +{ > + void __iomem *base; > + u8 major_revision; > + u32 offset; > + u32 val; > + > + /* > + * Remap xHCI address space to access XHCI ext cap regs since it is > + * needed to get information on number of ports present. > + */ > + base = ioremap(dwc->xhci_resources[0].start, > + resource_size(&dwc->xhci_resources[0])); > + if (IS_ERR(base)) > + return PTR_ERR(base); > + > + offset = 0; > + do { > + offset = xhci_find_next_ext_cap(base, offset, > + XHCI_EXT_CAPS_PROTOCOL); > + if (!offset) > + break; > + > + val = readl(base + offset); > + major_revision = XHCI_EXT_PORT_MAJOR(val); > + > + val = readl(base + offset + 0x08); > + if (major_revision == 0x03) { > + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); > + } else if (major_revision <= 0x02) { > + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); > + } else { > + dev_warn(dwc->dev, > + "unrecognized port major revision %d\n", I still think you should merge this with the previous line even if you end up with 83 chars. > + major_revision); > + } > + } while (1); > + /* > + * Currently only DWC3 controllers that are host-only capable > + * support Multiport. > + */ So again, also here, rephrase the comment so that it is clear that you are referring to a property of the current hardware. > + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); > + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { > + ret = dwc3_read_port_info(dwc); > + if (ret) > + goto err_disable_clks; > + } else { > + dwc->num_usb2_ports = 1; > + dwc->num_usb3_ports = 1; > + } Johan
On 2/29/2024 3:17 PM, Johan Hovold wrote: > On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote: >> Currently Multiport DWC3 controllers are host-only capable. > > I already asked you to rephrase this so that it becomes clear that you > are describing a property of the current hardware (and similar > throughout the series): > > https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/ Hi Johan. Thanks for the review. IMO, the statement is describing a property unique to current hardware, that "If it is a multiport controller, it is then host-only capable" I used the word "Currently" to indicate that "Today, the multiport devices present...". Let me know if there is any ambiguity in the sentence. In v13, I wrote: "Currently host-only capable DWC3 controllers support Multiport." You were right. It was ambiguous as it might refer to even single port controllers. So I changed it saying all the DWC3 multiport controllers are host only capable. How about: "All the DWC3 Multi Port controllers that exist today only support host mode" > >> +static int dwc3_read_port_info(struct dwc3 *dwc) >> +{ >> + void __iomem *base; >> + u8 major_revision; >> + u32 offset; >> + u32 val; >> + >> + /* >> + * Remap xHCI address space to access XHCI ext cap regs since it is >> + * needed to get information on number of ports present. >> + */ >> + base = ioremap(dwc->xhci_resources[0].start, >> + resource_size(&dwc->xhci_resources[0])); >> + if (IS_ERR(base)) >> + return PTR_ERR(base); >> + >> + offset = 0; >> + do { >> + offset = xhci_find_next_ext_cap(base, offset, >> + XHCI_EXT_CAPS_PROTOCOL); >> + if (!offset) >> + break; >> + >> + val = readl(base + offset); >> + major_revision = XHCI_EXT_PORT_MAJOR(val); >> + >> + val = readl(base + offset + 0x08); >> + if (major_revision == 0x03) { >> + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); >> + } else if (major_revision <= 0x02) { >> + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); >> + } else { >> + dev_warn(dwc->dev, >> + "unrecognized port major revision %d\n", > > I still think you should merge this with the previous line even if you > end up with 83 chars. > >> + major_revision); >> + } >> + } while (1); > >> + /* >> + * Currently only DWC3 controllers that are host-only capable >> + * support Multiport. >> + */ > > So again, also here, rephrase the comment so that it is clear that you > are referring to a property of the current hardware. I put the comment this way to indicate that we don't want to check for existence of multiple ports if the controller is not "host-only" capable. We should only check for multport support only if we are host-only capable. I think the statement clearly tells that "check for host-only" configuration before proceeding to check for xhci register reads. I replied the same on: https://lore.kernel.org/all/279a54f2-7260-4270-83c7-d6f5c5ba0873@quicinc.com/ And since you didn't mention anything else at this part of code in your return reply in: https://lore.kernel.org/all/ZTYyXhyZN3jBXEfm@hovoldconsulting.com/ I thought this statement was fine to go. > >> + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); >> + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { >> + ret = dwc3_read_port_info(dwc); >> + if (ret) >> + goto err_disable_clks; >> + } else { >> + dwc->num_usb2_ports = 1; >> + dwc->num_usb3_ports = 1; >> + } > Thanks for the review. Can you help let me know your review on the other patches as well. Regards, Krishna,
On Thu, Feb 29, 2024 at 05:23:08PM +0530, Krishna Kurapati PSSNV wrote: > On 2/29/2024 3:17 PM, Johan Hovold wrote: > > On Fri, Feb 16, 2024 at 06:27:49AM +0530, Krishna Kurapati wrote: > >> Currently Multiport DWC3 controllers are host-only capable. > > > > I already asked you to rephrase this so that it becomes clear that you > > are describing a property of the current hardware (and similar > > throughout the series): > > > > https://lore.kernel.org/all/ZTI7AtCJWgAnACSh@hovoldconsulting.com/ > IMO, the statement is describing a property unique to current hardware, > that "If it is a multiport controller, it is then host-only capable" > > I used the word "Currently" to indicate that "Today, the multiport > devices present...". Let me know if there is any ambiguity in the sentence. > > In v13, I wrote: > "Currently host-only capable DWC3 controllers support Multiport." > You were right. It was ambiguous as it might refer to even single port > controllers. > > So I changed it saying all the DWC3 multiport controllers are host only > capable. > > How about: > > "All the DWC3 Multi Port controllers that exist today only support host > mode" That should be clear enough, thanks. > >> + /* > >> + * Currently only DWC3 controllers that are host-only capable > >> + * support Multiport. > >> + */ > > > > So again, also here, rephrase the comment so that it is clear that you > > are referring to a property of the current hardware. > > I put the comment this way to indicate that we don't want to check for > existence of multiple ports if the controller is not "host-only" > capable. We should only check for multport support only if we are > host-only capable. I think the statement clearly tells that "check for > host-only" configuration before proceeding to check for xhci register reads. Fair enough, this comment could be considered to apply only to the implementation. Perhaps the following would be more clear though: Currently only DWC3 controllers that are host-only capable can have more than one port. or simply Host-only capable controllers can have more than one port. Both of these also gives a hint that this is a property of the hardware. > I replied the same on: > https://lore.kernel.org/all/279a54f2-7260-4270-83c7-d6f5c5ba0873@quicinc.com/ > > And since you didn't mention anything else at this part of code in your > return reply in: > https://lore.kernel.org/all/ZTYyXhyZN3jBXEfm@hovoldconsulting.com/ I left in the following quote on purpose in that reply: > > Please rephrase accordingly throughout so that this becomes clear. Johan
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 3b68e8e45b8b..965eaad195fb 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -39,6 +39,7 @@ #include "io.h" #include "debug.h" +#include "../host/xhci-ext-caps.h" #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */ @@ -1882,10 +1883,57 @@ static int dwc3_get_clocks(struct dwc3 *dwc) return 0; } +static int dwc3_read_port_info(struct dwc3 *dwc) +{ + void __iomem *base; + u8 major_revision; + u32 offset; + u32 val; + + /* + * Remap xHCI address space to access XHCI ext cap regs since it is + * needed to get information on number of ports present. + */ + base = ioremap(dwc->xhci_resources[0].start, + resource_size(&dwc->xhci_resources[0])); + if (IS_ERR(base)) + return PTR_ERR(base); + + offset = 0; + do { + offset = xhci_find_next_ext_cap(base, offset, + XHCI_EXT_CAPS_PROTOCOL); + if (!offset) + break; + + val = readl(base + offset); + major_revision = XHCI_EXT_PORT_MAJOR(val); + + val = readl(base + offset + 0x08); + if (major_revision == 0x03) { + dwc->num_usb3_ports += XHCI_EXT_PORT_COUNT(val); + } else if (major_revision <= 0x02) { + dwc->num_usb2_ports += XHCI_EXT_PORT_COUNT(val); + } else { + dev_warn(dwc->dev, + "unrecognized port major revision %d\n", + major_revision); + } + } while (1); + + dev_dbg(dwc->dev, "hs-ports: %u ss-ports: %u\n", + dwc->num_usb2_ports, dwc->num_usb3_ports); + + iounmap(base); + + return 0; +} + static int dwc3_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; struct resource *res, dwc_res; + unsigned int hw_mode; void __iomem *regs; struct dwc3 *dwc; int ret; @@ -1969,6 +2017,20 @@ static int dwc3_probe(struct platform_device *pdev) goto err_disable_clks; } + /* + * Currently only DWC3 controllers that are host-only capable + * support Multiport. + */ + hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0); + if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) { + ret = dwc3_read_port_info(dwc); + if (ret) + goto err_disable_clks; + } else { + dwc->num_usb2_ports = 1; + dwc->num_usb3_ports = 1; + } + spin_lock_init(&dwc->lock); mutex_init(&dwc->mutex); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index df544ec730d2..4c52fef99838 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1039,6 +1039,8 @@ struct dwc3_scratchpad_array { * @usb3_phy: pointer to USB3 PHY * @usb2_generic_phy: pointer to USB2 PHY * @usb3_generic_phy: pointer to USB3 PHY + * @num_usb2_ports: number of USB2 ports + * @num_usb3_ports: number of USB3 ports * @phys_ready: flag to indicate that PHYs are ready * @ulpi: pointer to ulpi interface * @ulpi_ready: flag to indicate that ULPI is initialized @@ -1187,6 +1189,9 @@ struct dwc3 { struct phy *usb2_generic_phy; struct phy *usb3_generic_phy; + u8 num_usb2_ports; + u8 num_usb3_ports; + bool phys_ready; struct ulpi *ulpi;
Currently Multiport DWC3 controllers are host-only capable. Temporarily map XHCI address space for host-only controllers and parse XHCI Extended Capabilities registers to read number of usb2 ports and usb3 ports present on multiport controller. Each USB Port is at least HS capable. The port info for usb2 and usb3 phy are identified as num_usb2_ports and num_usb3_ports. The intention is as follows: Wherever we need to perform phy operations like: LOOP_OVER_NUMBER_OF_AVAILABLE_PORTS() { phy_set_mode(dwc->usb2_generic_phy[i], PHY_MODE_USB_HOST); phy_set_mode(dwc->usb3_generic_phy[i], PHY_MODE_USB_HOST); } If number of usb2 ports is 3, loop can go from index 0-2 for usb2_generic_phy. If number of usb3-ports is 2, we don't know for sure, if the first 2 ports are SS capable or some other ports like (2 and 3) are SS capable. So instead, num_usb2_ports is used to loop around all phy's (both hs and ss) for performing phy operations. If any usb3_generic_phy turns out to be NULL, phy operation just bails out. num_usb3_ports is used to modify GUSB3PIPECTL registers while setting up phy's as we need to know how many SS capable ports are there for this. Signed-off-by: Krishna Kurapati <quic_kriskura@quicinc.com> Reviewed-by: Bjorn Andersson <andersson@kernel.org> Acked-by: Thinh Nguyen <Thinh.Nguyen@synopsys.com> --- drivers/usb/dwc3/core.c | 62 +++++++++++++++++++++++++++++++++++++++++ drivers/usb/dwc3/core.h | 5 ++++ 2 files changed, 67 insertions(+)