[U-Boot] [PATCH 1/2] usb: xhci: implement FEAT_POWER hook for switching regulators for ports

[Marek Vasut]

On 11/06/2017 11:04 PM, Philipp Tomsich wrote:
> When the FEAT_POWER flag is set/cleared for a port, power to this port
> should be enabled/disabled.  As many embedded xHCI controllers do not
> expose a signal to control this, extra effort may be required.
> 
> In order to link up setting/clearing FEAT_POWER with the regulator
> framework (so either a regulator or a GPIO modelled as a fixed
> regulator) can be switched, two callbacks are implemented in this
> change: if regulators are available an optional property
> 'xhci,port-power' can contain a stringlist with names of regulators
> that should be switched to control the power of ports (each entry in
> the stringlist corresponds to its respective regulator).
> 
> For some versions of the RK3399-Q7 (at least revisions v1.1 and v1.2
> are affected), we need to turn on the power for the port connected to
> the on-module USB hub only when FEAT_POWER is set to ensure that the
> hub does not enter a low-power mode that U-Boot's USB stack can't deal
> with.  Note that Linux eventually manages to attach the hub even when
> it's in its low-power state after a few seconds.
> 
> Signed-off-by: Philipp Tomsich <philipp.tomsich at theobroma-systems.com>
> ---
> 
>  drivers/usb/host/xhci.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 55 insertions(+)
> 
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 4673738..dabba18 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -30,6 +30,7 @@
>  #include <asm/unaligned.h>
>  #include <linux/errno.h>
>  #include "xhci.h"
> +#include <power/regulator.h>
>  
>  #ifndef CONFIG_USB_MAX_CONTROLLER_COUNT
>  #define CONFIG_USB_MAX_CONTROLLER_COUNT 1
> @@ -872,6 +873,58 @@ static u32 xhci_port_state_to_neutral(u32 state)
>  }
>  
>  /**
> + * Switch power at an external regulator each port at the root hub, when
> + * the FEAT_POWER feature is set/cleared.
> + *
> + * @param ctrl pointer to the xHCI controller
> + * @param req_index port number as in the control message (one-based)
> + * @param enable boolean indicating whether to enable or disable power
> + * @return returns 0 on success, an error-code on failure
> + */
> +static int xhci_board_port_powerset(struct xhci_ctrl *ctrl, int req_index,
> +				    bool enable)
> +{
> +#if CONFIG_IS_ENABLED(OF_CONTROL) && CONFIG_IS_ENABLED(DM_REGULATOR)
> +	/* We start counting ports at 0, while the request counts from 1. */
> +	int index = req_index - 1;
> +	struct udevice *dev = ctrl->dev;
> +	const char *regname = NULL;
> +	struct udevice *regulator;
> +	int ret;
> +
> +	debug("%s: ctrl '%s' port %d enable %s\n", __func__,
> +	      dev_read_name(dev), req_index, enable ? "true" : "false");
> +
> +	ret = dev_read_string_index(dev, "xhci,port-power", index, &regname);
> +	if (ret < 0) {
> +		debug("%s: ctrl '%s' port %d: no entry in 'xhci,port-power'\n",
> +		      __func__, dev_read_name(dev), req_index);
> +		return ret;
> +	}
> +
> +	ret = regulator_get_by_platname(regname, &regulator);
> +	if (ret) {
> +		debug("%s: ctrl '%s' port %d: could not get regulator '%s'\n",
> +		      __func__, dev_read_name(dev), req_index, regname);

Not being  able to get regulator is an error ? Why ?

> +		return ret;
> +	}
> +
> +	regulator_set_enable(regulator, enable);

This function returns integer, yet you ignore it. Please fix.

> +#endif
> +	return 0;
> +}
> +
> +static int xhci_board_port_poweron(struct xhci_ctrl *ctrl, int req_index)
> +{
> +	return xhci_board_port_powerset(ctrl, req_index, true);
> +}
> +
> +static int xhci_board_port_poweroff(struct xhci_ctrl *ctrl, int req_index)
> +{
> +	return xhci_board_port_powerset(ctrl, req_index, false);
> +}

Are these really needed ? IMO they're just useless wrappers.

> +/**
>   * Submits the Requests to the XHCI Host Controller
>   *
>   * @param udev pointer to the USB device structure
> @@ -1036,6 +1089,7 @@ static int xhci_submit_root(struct usb_device *udev, unsigned long pipe,
>  			xhci_writel(status_reg, reg);
>  			break;
>  		case USB_PORT_FEAT_POWER:
> +			xhci_board_port_poweron(ctrl, le16_to_cpu(req->index));
>  			reg |= PORT_POWER;
>  			xhci_writel(status_reg, reg);
>  			break;
> @@ -1056,6 +1110,7 @@ static int xhci_submit_root(struct usb_device *udev, unsigned long pipe,
>  			reg &= ~PORT_PE;
>  			break;
>  		case USB_PORT_FEAT_POWER:
> +			xhci_board_port_poweroff(ctrl, le16_to_cpu(req->index));
>  			reg &= ~PORT_POWER;
>  			break;
>  		case USB_PORT_FEAT_C_RESET:
> 


-- 
Best regards,
Marek Vasut