diff mbox series

[v15,2/9] usb: dwc3: core: Access XHCI address space temporarily to read port info

Message ID 20240216005756.762712-3-quic_kriskura@quicinc.com
State New
Headers
Series Add multiport support for DWC3 controllers |

Commit Message

Krishna Kurapati Feb. 16, 2024, 12:57 a.m. UTC 
  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(+)

Comments

Johan Hovold Feb. 29, 2024, 9:47 a.m. UTC | #1 
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
Krishna Kurapati Feb. 29, 2024, 11:53 a.m. UTC | #2 
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,
Johan Hovold March 4, 2024, 8:40 a.m. UTC | #3 
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 mbox series

Patch

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;