| Message ID | 20240404051229.3082902-3-quic_kriskura@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add multiport support for DWC3 controllers | expand |
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
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
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
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 :) "you" meaning Krishna, sorry for any confusion here.
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
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
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
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.
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 --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;
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(+)