[PATCH v5 0/3] xhci: tegra: USB2 pad power controls

[PATCH v5 0/3] xhci: tegra: USB2 pad power controls

[Jim Lin]

1. Export symbol on xhci_hub_control
2. Add hub_control to xhci_driver_overrides
3. Program USB2 pad PD controls during port connect/disconnect, port
suspend/resume, and test mode, to reduce power consumption on
disconnect or suspend.

Jim Lin (3):
  xhci: hub: export symbol on xhci_hub_control
  xhci: Add hub_control to xhci_driver_overrides
  xhci: tegra: USB2 pad power controls

 drivers/usb/host/xhci-hub.c   |   1 +
 drivers/usb/host/xhci-tegra.c | 131 +++++++++++++++++++++++++++++++++-
 drivers/usb/host/xhci.c       |   2 +
 drivers/usb/host/xhci.h       |   2 +
 4 files changed, 135 insertions(+), 1 deletion(-)

-- 
2.17.1

[PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[Jim Lin]

Add EXPORT_SYMBOL_GPL on xhci_hub_control() for other driver module
to invoke and avoid linking error.

Signed-off-by: Jim Lin <jilin@nvidia.com>

---
v5: new change

 drivers/usb/host/xhci-hub.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index af946c42b6f0..4f20cdae2a89 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1604,6 +1604,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
 	spin_unlock_irqrestore(&xhci->lock, flags);
 	return retval;
 }
+EXPORT_SYMBOL_GPL(xhci_hub_control);
 
 /*
  * Returns 0 if the status hasn't changed, or the number of bytes in buf.
-- 
2.17.1

[PATCH v5 2/3] xhci: Add hub_control to xhci_driver_overrides

[Jim Lin]

Add hub_control to "struct xhci_driver_overrides".
Add hub_control to xhci_init_driver() for platform xhci driver to
override it for local feature.

Signed-off-by: Jim Lin <jilin@nvidia.com>

---
v5: new change

 drivers/usb/host/xhci.c | 2 ++
 drivers/usb/host/xhci.h | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 902f410874e8..3c7bf0a0e0b4 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -5461,6 +5461,8 @@ void xhci_init_driver(struct hc_driver *drv,
 			drv->check_bandwidth = over->check_bandwidth;
 		if (over->reset_bandwidth)
 			drv->reset_bandwidth = over->reset_bandwidth;
+		if (over->hub_control)
+			drv->hub_control = over->hub_control;
 	}
 }
 EXPORT_SYMBOL_GPL(xhci_init_driver);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 5a75fe563123..f5a17c75c144 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1943,6 +1943,8 @@ struct xhci_driver_overrides {
 			     struct usb_host_endpoint *ep);
 	int (*check_bandwidth)(struct usb_hcd *, struct usb_device *);
 	void (*reset_bandwidth)(struct usb_hcd *, struct usb_device *);
+	int (*hub_control)(struct usb_hcd *hcd, u16 typeReq, u16 wValue,
+		u16 wIndex, char *buf, u16 wLength);
 };
 
 #define	XHCI_CFC_DELAY		10
-- 
2.17.1

[PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[Jim Lin]

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>

---
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;
 };
 
 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);
 
+		}
 		tegra_xusb_padctl_disable_phy_wake(padctl, tegra->phys[i]);
 	}
 }
@@ -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]);
+	}
+}
+
 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;
+	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]);
+		}
+	}
+
+	if (hcd->speed == HCD_USB2) {
+		i = tegra->soc->phy_types[USB3_PHY].num + port;
+		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]);
+		}
+		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]);
+		}
+	}
+
+	ret = xhci_hub_control(hcd, type_req, value, index, buf, length);
+	if (ret < 0)
+		return ret;
+
+	if (hcd->speed == HCD_USB2) {
+		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;
+}
+
 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

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[Greg KH]

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,

greg k-h

Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[Greg KH]

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?

> 
> ---
> 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

Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[Jim Lin]

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

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[Jim Lin]

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

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[gregkh]

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

Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[gregkh]

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

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1751 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[gregkh]

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

Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 11908 bytes --]

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
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v5 1/3] xhci: hub: export symbol on xhci_hub_control

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 2867 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]