Message-ID: <Y1u28wZc8DJDMXfR@orome>
Date:   Fri, 28 Oct 2022 13:03:15 +0200
From:   Thierry Reding <thierry.reding@...il.com>
To:     Jim Lin <jilin@...dia.com>
Cc:     mathias.nyman@...el.com, gregkh@...uxfoundation.org,
        jonathanh@...dia.com, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-tegra@...r.kernel.org,
        Petlozu Pravareshwar <petlozup@...dia.com>,
        JC Kuo <jckuo@...dia.com>
Subject: Re: [PATCH v5 3/3] xhci: tegra: USB2 pad power controls

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@...dia.com>
> Signed-off-by: JC Kuo <jckuo@...dia.com>
> Signed-off-by: Jim Lin <jilin@...dia.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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists