diff mbox series

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

Message ID 20240404051229.3082902-3-quic_kriskura@quicinc.com
State Superseded
Headers show
Series Add multiport support for DWC3 controllers | expand

Commit Message

Krishna Kurapati April 4, 2024, 5:12 a.m. UTC
All DWC3 Multi Port controllers that exist today only support host mode.
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: Johan Hovold <johan+linaro@kernel.org>
---
 drivers/usb/dwc3/core.c | 61 +++++++++++++++++++++++++++++++++++++++++
 drivers/usb/dwc3/core.h |  5 ++++
 2 files changed, 66 insertions(+)

Comments

Johan Hovold April 4, 2024, 7:21 a.m. UTC | #1
On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
 
> +static int dwc3_get_num_ports(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 (!base)
> +		return PTR_ERR(base);

This is obviously still broken. You need to update the return value as
well.

Fix in v20.

Johan
Krzysztof Kozlowski April 4, 2024, 8:07 a.m. UTC | #2
On 04/04/2024 09:21, Johan Hovold wrote:
> On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
>  
>> +static int dwc3_get_num_ports(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 (!base)
>> +		return PTR_ERR(base);
> 
> This is obviously still broken. You need to update the return value as
> well.
> 
> Fix in v20.

If one patchset reaches 20 versions, I think it is time to stop and
really think from the beginning, why issues keep appearing and reviewers
are still not happy.

Maybe you did not perform extensive internal review, which you are
encouraged to by your own internal policies, AFAIR. Before posting next
version, please really get some internal review first.

Best regards,
Krzysztof
Greg KH April 4, 2024, 12:58 p.m. UTC | #3
On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> On 04/04/2024 09:21, Johan Hovold wrote:
> > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> >  
> >> +static int dwc3_get_num_ports(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 (!base)
> >> +		return PTR_ERR(base);
> > 
> > This is obviously still broken. You need to update the return value as
> > well.
> > 
> > Fix in v20.
> 
> If one patchset reaches 20 versions, I think it is time to stop and
> really think from the beginning, why issues keep appearing and reviewers
> are still not happy.
> 
> Maybe you did not perform extensive internal review, which you are
> encouraged to by your own internal policies, AFAIR. Before posting next
> version, please really get some internal review first.

Also get those internal reviewers to sign-off on the commits and have
that show up when you post them next.  That way they are also
responsible for this patchset, it's not fair that they are making you do
all the work here :)

thanks,

greg k-h
Bjorn Andersson April 5, 2024, 1:25 a.m. UTC | #4
On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > On 04/04/2024 09:21, Johan Hovold wrote:
> > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > >  
> > >> +static int dwc3_get_num_ports(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 (!base)
> > >> +		return PTR_ERR(base);
> > > 
> > > This is obviously still broken. You need to update the return value as
> > > well.
> > > 
> > > Fix in v20.
> > 
> > If one patchset reaches 20 versions, I think it is time to stop and
> > really think from the beginning, why issues keep appearing and reviewers
> > are still not happy.
> > 
> > Maybe you did not perform extensive internal review, which you are
> > encouraged to by your own internal policies, AFAIR. Before posting next
> > version, please really get some internal review first.
> 
> Also get those internal reviewers to sign-off on the commits and have
> that show up when you post them next.  That way they are also
> responsible for this patchset, it's not fair that they are making you do
> all the work here :)
> 

I like this idea and I'm open to us changing our way of handling this.

But unless such internal review brings significant input to the
development I'd say a s-o-b would take the credit from the actual
author.

We've discussed a few times about carrying Reviewed-by et al from the
internal reviews, but as maintainer I dislike this because I'd have no
way to know if a r-b on vN means the patch was reviewed, or if it was
just "accidentally" carried from v(N-1).
But it might be worth this risk, is this something you think would be
appropriate?

Regards,
Bjorn
Greg KH April 5, 2024, 4:43 a.m. UTC | #5
On Thu, Apr 04, 2024 at 06:25:48PM -0700, Bjorn Andersson wrote:
> On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > > On 04/04/2024 09:21, Johan Hovold wrote:
> > > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > > >  
> > > >> +static int dwc3_get_num_ports(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 (!base)
> > > >> +		return PTR_ERR(base);
> > > > 
> > > > This is obviously still broken. You need to update the return value as
> > > > well.
> > > > 
> > > > Fix in v20.
> > > 
> > > If one patchset reaches 20 versions, I think it is time to stop and
> > > really think from the beginning, why issues keep appearing and reviewers
> > > are still not happy.
> > > 
> > > Maybe you did not perform extensive internal review, which you are
> > > encouraged to by your own internal policies, AFAIR. Before posting next
> > > version, please really get some internal review first.
> > 
> > Also get those internal reviewers to sign-off on the commits and have
> > that show up when you post them next.  That way they are also
> > responsible for this patchset, it's not fair that they are making you do
> > all the work here :)
> > 
> 
> I like this idea and I'm open to us changing our way of handling this.
> 
> But unless such internal review brings significant input to the
> development I'd say a s-o-b would take the credit from the actual
> author.

It does not do that at all.  It provides proof that someone else has
reviewed it and agrees with it.  Think of it as a "path of blame" for
when things go bad (i.e. there is a bug in the submission.)  Putting
your name on it makes you take responsibility if that happens.

> We've discussed a few times about carrying Reviewed-by et al from the
> internal reviews, but as maintainer I dislike this because I'd have no
> way to know if a r-b on vN means the patch was reviewed, or if it was
> just "accidentally" carried from v(N-1).
> But it might be worth this risk, is this something you think would be
> appropriate?

For some companies we REQUIRE this to happen due to low-quality
submissions and waste of reviewer's time.  Based on the track record
here for some of these patchsets, hopefully it doesn't become a
requirement for this company as well :)

thanks,

greg k-h
Bjorn Andersson April 5, 2024, 7:27 p.m. UTC | #6
On Fri, Apr 05, 2024 at 06:43:56AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Apr 04, 2024 at 06:25:48PM -0700, Bjorn Andersson wrote:
> > On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > > > On 04/04/2024 09:21, Johan Hovold wrote:
> > > > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > > > >  
> > > > >> +static int dwc3_get_num_ports(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 (!base)
> > > > >> +		return PTR_ERR(base);
> > > > > 
> > > > > This is obviously still broken. You need to update the return value as
> > > > > well.
> > > > > 
> > > > > Fix in v20.
> > > > 
> > > > If one patchset reaches 20 versions, I think it is time to stop and
> > > > really think from the beginning, why issues keep appearing and reviewers
> > > > are still not happy.
> > > > 
> > > > Maybe you did not perform extensive internal review, which you are
> > > > encouraged to by your own internal policies, AFAIR. Before posting next
> > > > version, please really get some internal review first.
> > > 
> > > Also get those internal reviewers to sign-off on the commits and have
> > > that show up when you post them next.  That way they are also
> > > responsible for this patchset, it's not fair that they are making you do
> > > all the work here :)
> > > 
> > 
> > I like this idea and I'm open to us changing our way of handling this.
> > 
> > But unless such internal review brings significant input to the
> > development I'd say a s-o-b would take the credit from the actual
> > author.
> 
> It does not do that at all.  It provides proof that someone else has
> reviewed it and agrees with it.  Think of it as a "path of blame" for
> when things go bad (i.e. there is a bug in the submission.)  Putting
> your name on it makes you take responsibility if that happens.
> 

Right, this is why I like your idea.

But as s-o-b either builds a trail of who handled the patch, or reflects
that it was co-authored by multiple people, I don't think either one
properly reflects reality.

> > We've discussed a few times about carrying Reviewed-by et al from the
> > internal reviews, but as maintainer I dislike this because I'd have no
> > way to know if a r-b on vN means the patch was reviewed, or if it was
> > just "accidentally" carried from v(N-1).
> > But it might be worth this risk, is this something you think would be
> > appropriate?
> 
> For some companies we REQUIRE this to happen due to low-quality
> submissions and waste of reviewer's time.  Based on the track record
> here for some of these patchsets, hopefully it doesn't become a
> requirement for this company as well :)
> 

Interesting, I was under the impression that we (maintainers) didn't
want such internally originating tags.

If that's not the case, then I'd be happy to adjust our internal
guidelines.

Regards,
Bjorn
Dmitry Baryshkov April 5, 2024, 8:36 p.m. UTC | #7
On Fri, 5 Apr 2024 at 22:27, Bjorn Andersson <quic_bjorande@quicinc.com> wrote:
>
> On Fri, Apr 05, 2024 at 06:43:56AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Apr 04, 2024 at 06:25:48PM -0700, Bjorn Andersson wrote:
> > > On Thu, Apr 04, 2024 at 02:58:29PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Apr 04, 2024 at 10:07:27AM +0200, Krzysztof Kozlowski wrote:
> > > > > On 04/04/2024 09:21, Johan Hovold wrote:
> > > > > > On Thu, Apr 04, 2024 at 10:42:22AM +0530, Krishna Kurapati wrote:
> > > > > >
> > > > > >> +static int dwc3_get_num_ports(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 (!base)
> > > > > >> +            return PTR_ERR(base);
> > > > > >
> > > > > > This is obviously still broken. You need to update the return value as
> > > > > > well.
> > > > > >
> > > > > > Fix in v20.
> > > > >
> > > > > If one patchset reaches 20 versions, I think it is time to stop and
> > > > > really think from the beginning, why issues keep appearing and reviewers
> > > > > are still not happy.
> > > > >
> > > > > Maybe you did not perform extensive internal review, which you are
> > > > > encouraged to by your own internal policies, AFAIR. Before posting next
> > > > > version, please really get some internal review first.
> > > >
> > > > Also get those internal reviewers to sign-off on the commits and have
> > > > that show up when you post them next.  That way they are also
> > > > responsible for this patchset, it's not fair that they are making you do
> > > > all the work here :)
> > > >
> > >
> > > I like this idea and I'm open to us changing our way of handling this.
> > >
> > > But unless such internal review brings significant input to the
> > > development I'd say a s-o-b would take the credit from the actual
> > > author.
> >
> > It does not do that at all.  It provides proof that someone else has
> > reviewed it and agrees with it.  Think of it as a "path of blame" for
> > when things go bad (i.e. there is a bug in the submission.)  Putting
> > your name on it makes you take responsibility if that happens.
> >
>
> Right, this is why I like your idea.
>
> But as s-o-b either builds a trail of who handled the patch, or reflects
> that it was co-authored by multiple people, I don't think either one
> properly reflects reality.
>
> > > We've discussed a few times about carrying Reviewed-by et al from the
> > > internal reviews, but as maintainer I dislike this because I'd have no
> > > way to know if a r-b on vN means the patch was reviewed, or if it was
> > > just "accidentally" carried from v(N-1).
> > > But it might be worth this risk, is this something you think would be
> > > appropriate?
> >
> > For some companies we REQUIRE this to happen due to low-quality
> > submissions and waste of reviewer's time.  Based on the track record
> > here for some of these patchsets, hopefully it doesn't become a
> > requirement for this company as well :)
> >
>
> Interesting, I was under the impression that we (maintainers) didn't
> want such internally originating tags.

But why? It just means that the patch has been reviewed. In some rare
cases we explicitly ask a developer to have all the patches reviewed
before sending them upstream. In such a case having an R-B tag
fulfills the expectation of the maintainer: it shows that another
engineer has reviewed the patch.

> If that's not the case, then I'd be happy to adjust our internal
> guidelines.
Krzysztof Kozlowski April 6, 2024, 11:14 a.m. UTC | #8
On 05/04/2024 22:36, Dmitry Baryshkov wrote:
>>>>>
>>>>> Also get those internal reviewers to sign-off on the commits and have
>>>>> that show up when you post them next.  That way they are also
>>>>> responsible for this patchset, it's not fair that they are making you do
>>>>> all the work here :)
>>>>>
>>>>
>>>> I like this idea and I'm open to us changing our way of handling this.
>>>>
>>>> But unless such internal review brings significant input to the
>>>> development I'd say a s-o-b would take the credit from the actual
>>>> author.
>>>
>>> It does not do that at all.  It provides proof that someone else has
>>> reviewed it and agrees with it.  Think of it as a "path of blame" for
>>> when things go bad (i.e. there is a bug in the submission.)  Putting
>>> your name on it makes you take responsibility if that happens.
>>>
>>
>> Right, this is why I like your idea.
>>
>> But as s-o-b either builds a trail of who handled the patch, or reflects
>> that it was co-authored by multiple people, I don't think either one
>> properly reflects reality.
>>
>>>> We've discussed a few times about carrying Reviewed-by et al from the
>>>> internal reviews, but as maintainer I dislike this because I'd have no
>>>> way to know if a r-b on vN means the patch was reviewed, or if it was
>>>> just "accidentally" carried from v(N-1).
>>>> But it might be worth this risk, is this something you think would be
>>>> appropriate?
>>>
>>> For some companies we REQUIRE this to happen due to low-quality
>>> submissions and waste of reviewer's time.  Based on the track record
>>> here for some of these patchsets, hopefully it doesn't become a
>>> requirement for this company as well :)
>>>
>>
>> Interesting, I was under the impression that we (maintainers) didn't
>> want such internally originating tags.
> 
> But why? It just means that the patch has been reviewed. In some rare
> cases we explicitly ask a developer to have all the patches reviewed
> before sending them upstream. In such a case having an R-B tag
> fulfills the expectation of the maintainer: it shows that another
> engineer has reviewed the patch.

Wait, there are two types of internal reviews.

Automatic, +1 from Gerrit or from whatever internal processes require,
which are not useful because these internal reviewers do not actually
review. I have seen a lot of such and I complain. It's easy to spot them
- a patchset consisting of few patches, including trivial ones, all of
them carrying one more more review tags. Even fixing a typo: reviewed
tag. Plus then you see that quality of the patchset is actually poor.

Another are real reviews done internally. If they are real, I find them
useful.

Best regards,
Krzysztof
diff mbox series

Patch

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 31684cdaaae3..d4765d93693f 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 */
 
@@ -1881,10 +1882,56 @@  static int dwc3_get_clocks(struct dwc3 *dwc)
 	return 0;
 }
 
+static int dwc3_get_num_ports(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 (!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;
@@ -1968,6 +2015,20 @@  static int dwc3_probe(struct platform_device *pdev)
 			goto err_disable_clks;
 	}
 
+	/*
+	 * Currently only DWC3 controllers that are host-only capable
+	 * can have more than one port.
+	 */
+	hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+	if (hw_mode == DWC3_GHWPARAMS0_MODE_HOST) {
+		ret = dwc3_get_num_ports(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 7e80dd3d466b..341e4c73cb2e 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;