| Message ID | 20221027133127.27592-1-jilin@nvidia.com |
|---|---|
| Headers | show |
| Series | xhci: tegra: USB2 pad power controls | expand |
On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote: > External email: Use caution opening links or attachments > > > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > > Program USB2 pad PD controls during port connect/disconnect, port > > suspend/resume, and test mode, to reduce power consumption on > > disconnect or suspend. > > > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > > Signed-off-by: JC Kuo <jckuo@nvidia.com> > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > Who is the author here? These do not seem to be in the correct order > if > you are the author, right? > > This is an old patch. Each time went with some small modification. Petlozu is author for local Kernel 3.18 Then JC for local Kernel 4.4 Now my turn for Kernel 5.xx > > > > > --- > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > > v3: No change on copyright > > v4: Remove hcd_to_tegra_xusb() function which is used only once. > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > > Invoke xhci_hub_control() directly (xhci-tegra.c) > > > > drivers/usb/host/xhci-tegra.c | 131 > > +++++++++++++++++++++++++++++++++- > > 1 file changed, 130 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- > > tegra.c > > index c8af2cd2216d..f685bb7459ba 100644 > > --- a/drivers/usb/host/xhci-tegra.c > > +++ b/drivers/usb/host/xhci-tegra.c > > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { > > } fpci; > > }; > > > > +enum tegra_xhci_phy_type { > > + USB3_PHY, > > + USB2_PHY, > > + HSIC_PHY, > > + MAX_PHY_TYPES, > > +}; > > + > > struct tegra_xusb_soc { > > const char *firmware; > > const char * const *supply_names; > > @@ -274,6 +281,7 @@ struct tegra_xusb { > > > > bool suspended; > > struct tegra_xusb_context context; > > + u32 enable_utmi_pad_after_lp0_exit; > > This is a bitfield, how do we know it will fit in a u32? What is the > range you are putting in here? > > thanks, > > greg k-h static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra) { unsigned int i; for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { if (!is_host_mode_phy(tegra, USB2_PHY, i)) continue; /* USB2 */ if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) : How many bits to be used is based on tegra->soc- >phy_types[USB2_PHY].num which is defined like static const struct tegra_xusb_phy_type tegra210_phy_types[] = { : { .name = "usb2", .num = 4, }, : }; static const struct tegra_xusb_phy_type tegra194_phy_types[] = { : { .name = "usb2", .num = 4, }, }; , so far at most 4. Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough. --nvpublic
On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote: > External email: Use caution opening links or attachments > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote: > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module > > to invoke and avoid linking error. > > What other driver module? > > There is no user here :( > > confused, > In arch/arm/configs/multi_v7_defconfig It defines CONFIG_USB_XHCI_TEGRA=m If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control() , I will get compile/linking error like ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko] undefined! if patch "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls" https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/ is added in xhci-tegra.c to invoke xhci_hub_control() Should I integrate this patch with [PATCH v5,3/3] as one? --nvpublic
On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote: > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote: > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module > > > to invoke and avoid linking error. > > > > What other driver module? > > > > There is no user here :( > > > > confused, > > > > In arch/arm/configs/multi_v7_defconfig > It defines > CONFIG_USB_XHCI_TEGRA=m > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control() > , I will get compile/linking error like > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko] > undefined! > > if patch > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls" > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/ > is added in xhci-tegra.c to invoke xhci_hub_control() > > Should I integrate this patch with [PATCH v5,3/3] as one? Yes, do not add something that is not needed for that specific commit, otherwise it causes reviewers to be confused. thanks, greg k-h
On Fri, Oct 28, 2022 at 03:11:31AM +0000, Jim Lin wrote: > On Thu, 2022-10-27 at 16:00 +0200, Greg KH wrote: > > External email: Use caution opening links or attachments > > > > > > On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > > > Program USB2 pad PD controls during port connect/disconnect, port > > > suspend/resume, and test mode, to reduce power consumption on > > > disconnect or suspend. > > > > > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > > > Signed-off-by: JC Kuo <jckuo@nvidia.com> > > > Signed-off-by: Jim Lin <jilin@nvidia.com> > > > > Who is the author here? These do not seem to be in the correct order > > if > > you are the author, right? > > > This is an old patch. Each time went with some small modification. > > > Petlozu is author for local Kernel 3.18 > > Then JC for local Kernel 4.4 > Now my turn for Kernel 5.xx Then you all need to figure out how to proper document this (hint, read the documentation for how to do that...) > > > --- > > > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > > > v3: No change on copyright > > > v4: Remove hcd_to_tegra_xusb() function which is used only once. > > > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > > > Invoke xhci_hub_control() directly (xhci-tegra.c) > > > > > > drivers/usb/host/xhci-tegra.c | 131 > > > +++++++++++++++++++++++++++++++++- > > > 1 file changed, 130 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci- > > > tegra.c > > > index c8af2cd2216d..f685bb7459ba 100644 > > > --- a/drivers/usb/host/xhci-tegra.c > > > +++ b/drivers/usb/host/xhci-tegra.c > > > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { > > > } fpci; > > > }; > > > > > > +enum tegra_xhci_phy_type { > > > + USB3_PHY, > > > + USB2_PHY, > > > + HSIC_PHY, > > > + MAX_PHY_TYPES, > > > +}; > > > + > > > struct tegra_xusb_soc { > > > const char *firmware; > > > const char * const *supply_names; > > > @@ -274,6 +281,7 @@ struct tegra_xusb { > > > > > > bool suspended; > > > struct tegra_xusb_context context; > > > + u32 enable_utmi_pad_after_lp0_exit; > > > > This is a bitfield, how do we know it will fit in a u32? What is the > > range you are putting in here? > > > > thanks, > > > > greg k-h > static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb > *tegra) > { > unsigned int i; > > for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { > if (!is_host_mode_phy(tegra, USB2_PHY, i)) > continue; > /* USB2 */ > if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) > : > How many bits to be used is based on tegra->soc- > >phy_types[USB2_PHY].num which is defined like > > static const struct tegra_xusb_phy_type tegra210_phy_types[] = { > : > { .name = "usb2", .num = 4, }, > : > }; > > static const struct tegra_xusb_phy_type tegra194_phy_types[] = { > : > { .name = "usb2", .num = 4, }, > }; > , so far at most 4. > > Therefore u8 for enable_utmi_pad_after_lp0_exit is long enough. I am sorry, I do not understand. If you are needing to treat this as a bitfield, then properly define it that way so it is obvious what is happening. As it is, this is very confusing and I do not understand what is going on at all. What is the relationship to "num" and the bit being set? Why set a bit at all? thanks, greg k-h
On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote: > On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote: > > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote: > > > External email: Use caution opening links or attachments > > > > > > > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote: > > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module > > > > to invoke and avoid linking error. > > > > > > What other driver module? > > > > > > There is no user here :( > > > > > > confused, > > > > > > > In arch/arm/configs/multi_v7_defconfig > > It defines > > CONFIG_USB_XHCI_TEGRA=m > > > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control() > > , I will get compile/linking error like > > > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko] > > undefined! > > > > if patch > > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls" > > > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/ > > is added in xhci-tegra.c to invoke xhci_hub_control() > > > > Should I integrate this patch with [PATCH v5,3/3] as one? > > Yes, do not add something that is not needed for that specific commit, > otherwise it causes reviewers to be confused. Other subsystem maintainers prefer core changes to be split from driver changes, so this type of split is commonly encountered elsewhere. Obviously, since this is your turf you get to make the rules. I'm just trying to say that this kind of advice can be confusing for contributors because when they then sent driver and code changes mixed for their next submission, the subsystem maintainer might tell them otherwise. Thierry
On Fri, Oct 28, 2022 at 11:50:47AM +0200, Thierry Reding wrote: > On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote: > > On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote: > > > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote: > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote: > > > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module > > > > > to invoke and avoid linking error. > > > > > > > > What other driver module? > > > > > > > > There is no user here :( > > > > > > > > confused, > > > > > > > > > > In arch/arm/configs/multi_v7_defconfig > > > It defines > > > CONFIG_USB_XHCI_TEGRA=m > > > > > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control() > > > , I will get compile/linking error like > > > > > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko] > > > undefined! > > > > > > if patch > > > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls" > > > > > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/ > > > is added in xhci-tegra.c to invoke xhci_hub_control() > > > > > > Should I integrate this patch with [PATCH v5,3/3] as one? > > > > Yes, do not add something that is not needed for that specific commit, > > otherwise it causes reviewers to be confused. > > Other subsystem maintainers prefer core changes to be split from driver > changes, so this type of split is commonly encountered elsewhere. > > Obviously, since this is your turf you get to make the rules. I'm just > trying to say that this kind of advice can be confusing for contributors > because when they then sent driver and code changes mixed for their next > submission, the subsystem maintainer might tell them otherwise. Sure, but if you do split it up like this, DOCUMENT WHY THE EXPORT IS NEEDED. That didn't happen here so I had no idea why this was even an issue. And yes, I am very sensitive to this, we have had LOTS of people trying to export xhci symbols in the past few years for no in-kernel users, despite us constantly telling them that this is not allowed. It happened again, just yesterday: https://lore.kernel.org/r/20221027004050.4192111-1-albertccwang@google.com And at first glance, I assumed this was much the same as there was no description of why this was needed at all. thanks, greg k-h
On Thu, Oct 27, 2022 at 09:31:27PM +0800, Jim Lin wrote: > Program USB2 pad PD controls during port connect/disconnect, port > suspend/resume, and test mode, to reduce power consumption on > disconnect or suspend. > > Signed-off-by: Petlozu Pravareshwar <petlozup@nvidia.com> > Signed-off-by: JC Kuo <jckuo@nvidia.com> > Signed-off-by: Jim Lin <jilin@nvidia.com> As Greg mentioned, these Signed-off-by: lines should be paired up with Co-developed-by: lines to give additional information about what their respective role was in the patch submission. A Signed-off-by alone has no additional information about the actor, so the implication is that they simply forwarded the patches, which would be what a maintainer would typically do. Signed-off-by: lines should also be in chronological order, so So if Petlozu and JC did not author and simply forward the patches, then you as the author (taken from the From: field) would need to appear before both of them. Also in that case, you wouldn't typically be sending the patch. Co-developed-by: in this case provides additional information, indicating that Petlozu and JC co-authored the patch. A Signed-off-by from them is still required, though. Your From: line says that you also authored it (i.e. you become the primary author). Since you then send it upstream the Signed-off-by is then in the right place. Also, since the authorship is already implied from the From: header, no need to include a Co-developed-by: line for yourself. > --- > v2: Fix issue that wrong tegra->phys[] may be accessed on tegra124 > v3: No change on copyright > v4: Remove hcd_to_tegra_xusb() function which is used only once. > v5: Update .hub_control in tegra_xhci_overrides (xhci-tegra.c) > Invoke xhci_hub_control() directly (xhci-tegra.c) > > drivers/usb/host/xhci-tegra.c | 131 +++++++++++++++++++++++++++++++++- > 1 file changed, 130 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/host/xhci-tegra.c b/drivers/usb/host/xhci-tegra.c > index c8af2cd2216d..f685bb7459ba 100644 > --- a/drivers/usb/host/xhci-tegra.c > +++ b/drivers/usb/host/xhci-tegra.c > @@ -189,6 +189,13 @@ struct tegra_xusb_context_soc { > } fpci; > }; > > +enum tegra_xhci_phy_type { > + USB3_PHY, > + USB2_PHY, > + HSIC_PHY, > + MAX_PHY_TYPES, > +}; This is a little messy. This is the default order that we define, but there's no strict requirement for the entries in tegra_xusb_phy_type to be in this order. To avoid this, we've used string comparisions in other places, which isn't great either, but it's at least a bit more consistent. > + > struct tegra_xusb_soc { > const char *firmware; > const char * const *supply_names; > @@ -274,6 +281,7 @@ struct tegra_xusb { > > bool suspended; > struct tegra_xusb_context context; > + u32 enable_utmi_pad_after_lp0_exit; Perhaps call this lp0_utmi_pad_mask or something to make it clearer that this represents a mask of bits, one for each UTMI pad. > }; > > static struct hc_driver __read_mostly tegra_xhci_hc_driver; > @@ -1949,12 +1957,30 @@ static void tegra_xhci_enable_phy_sleepwalk_wake(struct tegra_xusb *tegra) > static void tegra_xhci_disable_phy_wake(struct tegra_xusb *tegra) > { > struct tegra_xusb_padctl *padctl = tegra->padctl; > - unsigned int i; > + unsigned int i, j; > > for (i = 0; i < tegra->num_phys; i++) { > if (!tegra->phys[i]) > continue; > + if (tegra_xusb_padctl_remote_wake_detected(padctl, tegra->phys[i])) { > + if (i < tegra->soc->phy_types[USB3_PHY].num) { > + /* USB3 */ > + j = i; > + } else if (i < (tegra->soc->phy_types[USB3_PHY].num + > + tegra->soc->phy_types[USB2_PHY].num)) { > + /* USB2 */ > + j = i - tegra->soc->phy_types[USB3_PHY].num; > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); > + } else { > + /* HSIC */ > + j = i - (tegra->soc->phy_types[USB3_PHY].num + > + tegra->soc->phy_types[USB2_PHY].num); > + } > + dev_dbg(tegra->dev, > + "%s port %u (0 based) remote wake detected\n", > + dev_name(&tegra->phys[i]->dev), j); It looks like the only reason we do all this is to print out a debug message. Can we not just stick with dev_name(&tegra->phys[i]->dev)? That should already be unique. I guess the only downside is that the index included in that string is not 0 based (PHY core uses a global index for some reason), but I'm not sure all this extra effort is warranted just for that. If it is, perhaps we need to find some other way to get at the actual index. Maybe the PHY driver can be modified to make the device name reflect a more meaningful name. Or perhaps yet another solution would be to print the PHY's OF node name: "%pOFn", phy->dev.of_node > This blank line was originally after the "continue;" above, to make things a bit more readable, so put it back there. > + } > tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]); Again, could use a blank line after the closing } for readability. > } > } > @@ -1972,6 +1998,23 @@ static void tegra_xhci_disable_phy_sleepwalk(struct tegra_xusb *tegra) > } > } > > +static void tegra_xhci_program_utmi_power_lp0_exit(struct tegra_xusb *tegra) > +{ > + unsigned int i; > + > + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { > + if (!is_host_mode_phy(tegra, USB2_PHY, i)) > + continue; > + /* USB2 */ > + if (tegra->enable_utmi_pad_after_lp0_exit & BIT(i)) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); > + else > + tegra_phy_xusb_utmi_pad_power_down( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); This is all very confusing. The following should get you the right PHY in a more straightforward way: struct phy *phy = tegra_xusb_get_phy(tegra, "usb2", i); > + } > +} > + > static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > { > struct xhci_hcd *xhci = hcd_to_xhci(tegra->hcd); > @@ -1980,6 +2023,7 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > unsigned int i; > int err; > u32 usbcmd; > + u32 portsc; > > dev_dbg(dev, "entering ELPG\n"); > > @@ -1993,6 +2037,15 @@ static int tegra_xusb_enter_elpg(struct tegra_xusb *tegra, bool runtime) > goto out; > } > > + for (i = 0; i < tegra->soc->phy_types[USB2_PHY].num; i++) { > + if (!xhci->usb2_rhub.ports[i]) > + continue; > + portsc = readl(xhci->usb2_rhub.ports[i]->addr); > + tegra->enable_utmi_pad_after_lp0_exit &= ~BIT(i); > + if (((portsc & PORT_PLS_MASK) == XDEV_U3) || ((portsc & DEV_SPEED_MASK) == XDEV_FS)) > + tegra->enable_utmi_pad_after_lp0_exit |= BIT(i); > + } > + > err = xhci_suspend(xhci, wakeup); > if (err < 0) { > dev_err(tegra->dev, "failed to suspend XHCI: %d\n", err); > @@ -2066,6 +2119,8 @@ static int tegra_xusb_exit_elpg(struct tegra_xusb *tegra, bool runtime) > > phy_power_on(tegra->phys[i]); > } > + if (tegra->suspended) > + tegra_xhci_program_utmi_power_lp0_exit(tegra); > > tegra_xusb_config(tegra); > tegra_xusb_restore_context(tegra); > @@ -2437,8 +2492,82 @@ static int tegra_xhci_setup(struct usb_hcd *hcd) > return xhci_gen_setup(hcd, tegra_xhci_quirks); > } > > +static int tegra_xhci_hub_control(struct usb_hcd *hcd, u16 type_req, u16 value, u16 index, > + char *buf, u16 length) > +{ > + struct tegra_xusb *tegra = dev_get_drvdata(hcd->self.controller); > + struct xhci_hcd *xhci = hcd_to_xhci(hcd); > + struct xhci_hub *rhub; > + struct xhci_bus_state *bus_state; > + int port = (index & 0xff) - 1; > + int i; "unsigned int i" to match rhub->num_ports; The variable name choice is a little confusing to me here. I wonder if perhaps this would be clearer if you didn't reuse "i" for the PHY index and instead used the tegra_xusb_get_phy() as before. See below... > + struct xhci_port **ports; > + u32 portsc; > + int ret; > + > + rhub = &xhci->usb2_rhub; > + bus_state = &rhub->bus_state; > + if (bus_state->resuming_ports && hcd->speed == HCD_USB2) { > + ports = rhub->ports; > + i = rhub->num_ports; > + while (i--) { > + if (!test_bit(i, &bus_state->resuming_ports)) > + continue; > + portsc = readl(ports[i]->addr); > + if ((portsc & PORT_PLS_MASK) == XDEV_RESUME) > + tegra_phy_xusb_utmi_pad_power_on( > + tegra->phys[tegra->soc->phy_types[USB3_PHY].num + i]); This could then be: phy = tegra_xusb_get_phy(tegra, "usb2", i); tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + } > + > + if (hcd->speed == HCD_USB2) { > + i = tegra->soc->phy_types[USB3_PHY].num + port; Similarly: phy = tegra_xusb_get_phy(tegra, "usb2", i); > + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) { > + if (!index || index > rhub->num_ports) > + return -EPIPE; > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); And: tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_RESET)) { > + if (!index || index > rhub->num_ports) > + return -EPIPE; > + ports = rhub->ports; > + portsc = readl(ports[port]->addr); > + if (portsc & PORT_CONNECT) > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); And, finally: tegra_phy_xusb_utmi_pad_power_on(phy); > + } > + } > + > + ret = xhci_hub_control(hcd, type_req, value, index, buf, length); > + if (ret < 0) > + return ret; > + > + if (hcd->speed == HCD_USB2) { It might be a good idea to add a comment here about where "i" (or "phy" in the alternative I proposed) has been set. I kept thinking that this was accidentally using an old value, whereas it's really doing so on purpose. Maybe that could be made a bit clearer by making this second check: if (phy) { ... } If you then declare phy = NULL at the top level of this function, it becomes clearer that it's reusing something that was previously obtained. That's less clear (to me) from looking at the current check. > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_SUSPEND)) > + /* We don't suspend the PAD while HNP role swap happens on the OTG port */ > + if (!((hcd->self.otg_port == (port + 1)) && hcd->self.b_hnp_enable)) > + tegra_phy_xusb_utmi_pad_power_down(tegra->phys[i]); > + > + if ((type_req == ClearPortFeature) && (value == USB_PORT_FEAT_C_CONNECTION)) { > + ports = rhub->ports; > + portsc = readl(ports[port]->addr); > + if (!(portsc & PORT_CONNECT)) { > + /* We don't suspend the PAD while HNP role swap happens on the OTG > + * port > + */ > + if (!((hcd->self.otg_port == (port + 1)) && hcd->self.b_hnp_enable)) > + tegra_phy_xusb_utmi_pad_power_down(tegra->phys[i]); > + } > + } > + if ((type_req == SetPortFeature) && (value == USB_PORT_FEAT_TEST)) > + tegra_phy_xusb_utmi_pad_power_on(tegra->phys[i]); > + } > + > + return ret; > +} Honestly, after going through all this I'm beginning to wonder if perhaps we should encapsulate all of this better. On one hand we've got a bunch of low-level stuff being done here in the XHCI driver that would fit better into the XUSB pad control driver (where the PHY implementation lives). We may be missing a bit of infrastructure here, but perhaps we can add something to improve this overall? Secondly, this whole .hub_control patching seems overly complicated to me. But maybe I'm being a bit naive and there's really no better way to do this currently. Thierry > + > static const struct xhci_driver_overrides tegra_xhci_overrides __initconst = { > .reset = tegra_xhci_setup, > + .hub_control = tegra_xhci_hub_control, > }; > > static int __init tegra_xusb_init(void) > -- > 2.17.1 >
On Fri, Oct 28, 2022 at 12:37:15PM +0200, gregkh@linuxfoundation.org wrote: > On Fri, Oct 28, 2022 at 11:50:47AM +0200, Thierry Reding wrote: > > On Fri, Oct 28, 2022 at 08:04:54AM +0200, gregkh@linuxfoundation.org wrote: > > > On Fri, Oct 28, 2022 at 05:36:41AM +0000, Jim Lin wrote: > > > > On Thu, 2022-10-27 at 15:58 +0200, Greg KH wrote: > > > > > External email: Use caution opening links or attachments > > > > > > > > > > > > > > > On Thu, Oct 27, 2022 at 09:31:25PM +0800, Jim Lin wrote: > > > > > > Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module > > > > > > to invoke and avoid linking error. > > > > > > > > > > What other driver module? > > > > > > > > > > There is no user here :( > > > > > > > > > > confused, > > > > > > > > > > > > > In arch/arm/configs/multi_v7_defconfig > > > > It defines > > > > CONFIG_USB_XHCI_TEGRA=m > > > > > > > > If I don't add EXPORT_SYMBOL_GPL on xhci_hub_control() > > > > , I will get compile/linking error like > > > > > > > > ERROR: modpost: "xhci_hub_control" [drivers/usb/host/xhci-tegra.ko] > > > > undefined! > > > > > > > > if patch > > > > "[PATCH v5,3/3] xhci: tegra: USB2 pad power controls" > > > > > > > > https://patchwork.kernel.org/project/linux-usb/patch/20221027133127.27592-4-jilin@nvidia.com/ > > > > is added in xhci-tegra.c to invoke xhci_hub_control() > > > > > > > > Should I integrate this patch with [PATCH v5,3/3] as one? > > > > > > Yes, do not add something that is not needed for that specific commit, > > > otherwise it causes reviewers to be confused. > > > > Other subsystem maintainers prefer core changes to be split from driver > > changes, so this type of split is commonly encountered elsewhere. > > > > Obviously, since this is your turf you get to make the rules. I'm just > > trying to say that this kind of advice can be confusing for contributors > > because when they then sent driver and code changes mixed for their next > > submission, the subsystem maintainer might tell them otherwise. > > Sure, but if you do split it up like this, DOCUMENT WHY THE EXPORT IS > NEEDED. That didn't happen here so I had no idea why this was even an > issue. > > And yes, I am very sensitive to this, we have had LOTS of people trying > to export xhci symbols in the past few years for no in-kernel users, > despite us constantly telling them that this is not allowed. It > happened again, just yesterday: > https://lore.kernel.org/r/20221027004050.4192111-1-albertccwang@google.com > > And at first glance, I assumed this was much the same as there was no > description of why this was needed at all. Agreed. I suppose this could've been spelled out more explicitly in the cover letter or in patch 1. Jim, please make sure to describe this dependency explicitly in v6. Thierry