| Message ID | 1510005881-5519-1-git-send-email-philipp.tomsich@theobroma-systems.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Bin Meng |
| Headers | show |
| Series | [U-Boot,1/2] usb: xhci: implement FEAT_POWER hook for switching regulators for ports | expand |
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@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, ®name); > + 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, ®ulator); > + 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: >
Hi Philipp, On Tue, Nov 7, 2017 at 6:04 AM, Philipp Tomsich <philipp.tomsich@theobroma-systems.com> 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. > Please help me understand the problem. At first glance, I don't think such change should be put in the xHCI core codes. To me, this patch sounds like we use the regulator to enable/disable the power to an external on-board USB hub. For the xHC root port, this port power on/off is already configured by the xHC registers. > Signed-off-by: Philipp Tomsich <philipp.tomsich@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, ®name); > + 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, ®ulator); > + if (ret) { > + debug("%s: ctrl '%s' port %d: could not get regulator '%s'\n", > + __func__, dev_read_name(dev), req_index, regname); > + return ret; > + } > + > + regulator_set_enable(regulator, enable); > +#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); > +} > + > +/** > * 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: > -- Regards, Bin
Bin, > On 17 Nov 2017, at 10:27, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Philipp, > > On Tue, Nov 7, 2017 at 6:04 AM, Philipp Tomsich > <philipp.tomsich@theobroma-systems.com <mailto:philipp.tomsich@theobroma-systems.com>> 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. >> > > Please help me understand the problem. At first glance, I don't think > such change should be put in the xHCI core codes. > To me, this patch sounds like we use the regulator to enable/disable > the power to an external on-board USB hub. For the xHC root port, this > port power on/off is already configured by the xHC registers. The problem is that we have only a limited window of time between enabling the power to the USB hub and the USB controller starting its link-up procedure (i.e. reset of the port and waiting for a port-change), as the hub will otherwise go into a low-power mode and only come up for a short period of time every [I don’t remember the amount of time] milliseconds. While this is no problem on Linux (the USB hub eventually gets detected after a delay of 2-3 seconds), the U-Boot USB stack (as it’s not interrupt driven) usually misses the port-change event (i.e. the probing has already moved on by the time the USB hub tries again). By having a callback directly from within the xHCI root-hub code, we can ensure that we always catch the USB hub. We can’t fix this from the board code, as the time between us enabling the regulator and the probing from within ‘usb start’ will be unpredictable. My first instinct was to have a call to a board-specific weak-function from xhci, but I finally settled on using the regulator-framework to have some that can be reused in the general case and also allows for on-demand power-on/off of on-board USB devices (if anybody has the need for it). > >> Signed-off-by: Philipp Tomsich <philipp.tomsich@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, ®name); >> + 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, ®ulator); >> + if (ret) { >> + debug("%s: ctrl '%s' port %d: could not get regulator '%s'\n", >> + __func__, dev_read_name(dev), req_index, regname); >> + return ret; >> + } >> + >> + regulator_set_enable(regulator, enable); >> +#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); >> +} >> + >> +/** >> * 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: >> -- > > Regards, > Bin
Hi Philipp, On Fri, Nov 17, 2017 at 5:43 PM, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > Bin, > > On 17 Nov 2017, at 10:27, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Philipp, > > On Tue, Nov 7, 2017 at 6:04 AM, Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> 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. > > > Please help me understand the problem. At first glance, I don't think > such change should be put in the xHCI core codes. > To me, this patch sounds like we use the regulator to enable/disable > the power to an external on-board USB hub. For the xHC root port, this > port power on/off is already configured by the xHC registers. > > > The problem is that we have only a limited window of time between enabling > the power to the USB hub and the USB controller starting its link-up > procedure > (i.e. reset of the port and waiting for a port-change), as the hub will > otherwise > go into a low-power mode and only come up for a short period of time every > [I don’t remember the amount of time] milliseconds. > Does the USB hub keep online all the time when the power is supplied by the regulator? Is this entering low-power mode automatic? Can it be turned off? > While this is no problem on Linux (the USB hub eventually gets detected > after > a delay of 2-3 seconds), the U-Boot USB stack (as it’s not interrupt driven) > usually misses the port-change event (i.e. the probing has already moved on > by the time the USB hub tries again). > > By having a callback directly from within the xHCI root-hub code, we can > ensure that we always catch the USB hub. We can’t fix this from the board > code, as the time between us enabling the regulator and the probing from > within ‘usb start’ will be unpredictable. My first instinct was to have a > call > to a board-specific weak-function from xhci, but I finally settled on using > the regulator-framework to have some that can be reused in the general > case and also allows for on-demand power-on/off of on-board USB devices > (if anybody has the need for it). > If turning off the low-power mode is not possible, there is already a weak function usb_hub_reset_devices() in current U-Boot USB stack. I think you can use that existing one? Regards, Bin
> On 17 Nov 2017, at 14:19, Bin Meng <bmeng.cn@gmail.com> wrote: > > Hi Philipp, > > On Fri, Nov 17, 2017 at 5:43 PM, Dr. Philipp Tomsich > <philipp.tomsich@theobroma-systems.com> wrote: >> Bin, >> >> On 17 Nov 2017, at 10:27, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Hi Philipp, >> >> On Tue, Nov 7, 2017 at 6:04 AM, Philipp Tomsich >> <philipp.tomsich@theobroma-systems.com> 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. >> >> >> Please help me understand the problem. At first glance, I don't think >> such change should be put in the xHCI core codes. >> To me, this patch sounds like we use the regulator to enable/disable >> the power to an external on-board USB hub. For the xHC root port, this >> port power on/off is already configured by the xHC registers. >> >> >> The problem is that we have only a limited window of time between enabling >> the power to the USB hub and the USB controller starting its link-up >> procedure >> (i.e. reset of the port and waiting for a port-change), as the hub will >> otherwise >> go into a low-power mode and only come up for a short period of time every >> [I don’t remember the amount of time] milliseconds. >> > > Does the USB hub keep online all the time when the power is supplied > by the regulator? Is this entering low-power mode automatic? Can it be > turned off? Once power starts probing for a link and (if it misses the link establishment) then goes into this low power mode where it wakes up every [x] milliseconds. So if we miss the initial active period for the attachment, then we’re pretty much lost within the constraints (i.e. w/o asynchronous port-change notifications) of the U-Boot USB stack. Turning this behaviour off requires an additional SPI flash for the USB hub, which may (as in “no new version has been scheduled yet”) be added in a future revision of the board. However, there are affected revisions of this product in the field that would benefit from fixing this (w/o this fix, only the dual-role port can be used from within U-Boot). > >> While this is no problem on Linux (the USB hub eventually gets detected >> after >> a delay of 2-3 seconds), the U-Boot USB stack (as it’s not interrupt driven) >> usually misses the port-change event (i.e. the probing has already moved on >> by the time the USB hub tries again). >> >> By having a callback directly from within the xHCI root-hub code, we can >> ensure that we always catch the USB hub. We can’t fix this from the board >> code, as the time between us enabling the regulator and the probing from >> within ‘usb start’ will be unpredictable. My first instinct was to have a >> call >> to a board-specific weak-function from xhci, but I finally settled on using >> the regulator-framework to have some that can be reused in the general >> case and also allows for on-demand power-on/off of on-board USB devices >> (if anybody has the need for it). >> > > If turning off the low-power mode is not possible, there is already a > weak function usb_hub_reset_devices() in current U-Boot USB stack. I > think you can use that existing one? I don’t think so, as the on-module hub won’t even show up on the bus if it’s in its low-power mode and usb_hub_configure (which in turn calls the usb_hub_reset_devices-hook) wouldn’t even be invoked for this case. Furthermore, usb_hub_reset_devices(int) only passes a port-number and we need to be selective on a specific root-port of a specific controller (we have 2 xHCIs active in our default DTS). > Regards, > Bin
Hi Philipp, On Fri, Nov 17, 2017 at 9:50 PM, Dr. Philipp Tomsich <philipp.tomsich@theobroma-systems.com> wrote: > >> On 17 Nov 2017, at 14:19, Bin Meng <bmeng.cn@gmail.com> wrote: >> >> Hi Philipp, >> >> On Fri, Nov 17, 2017 at 5:43 PM, Dr. Philipp Tomsich >> <philipp.tomsich@theobroma-systems.com> wrote: >>> Bin, >>> >>> On 17 Nov 2017, at 10:27, Bin Meng <bmeng.cn@gmail.com> wrote: >>> >>> Hi Philipp, >>> >>> On Tue, Nov 7, 2017 at 6:04 AM, Philipp Tomsich >>> <philipp.tomsich@theobroma-systems.com> 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. >>> >>> >>> Please help me understand the problem. At first glance, I don't think >>> such change should be put in the xHCI core codes. >>> To me, this patch sounds like we use the regulator to enable/disable >>> the power to an external on-board USB hub. For the xHC root port, this >>> port power on/off is already configured by the xHC registers. >>> >>> >>> The problem is that we have only a limited window of time between enabling >>> the power to the USB hub and the USB controller starting its link-up >>> procedure >>> (i.e. reset of the port and waiting for a port-change), as the hub will >>> otherwise >>> go into a low-power mode and only come up for a short period of time every >>> [I don’t remember the amount of time] milliseconds. >>> >> >> Does the USB hub keep online all the time when the power is supplied >> by the regulator? Is this entering low-power mode automatic? Can it be >> turned off? > > Once power starts probing for a link and (if it misses the link establishment) > then goes into this low power mode where it wakes up every [x] milliseconds. > So if we miss the initial active period for the attachment, then we’re pretty > much lost within the constraints (i.e. w/o asynchronous port-change notifications) > of the U-Boot USB stack. > Yes, I understand the lower-power mode. > Turning this behaviour off requires an additional SPI flash for the USB hub, > which may (as in “no new version has been scheduled yet”) be added in a future > revision of the board. However, there are affected revisions of this product in > the field that would benefit from fixing this (w/o this fix, only the dual-role port > can be used from within U-Boot). > So from your description, this automatic low-power mode entering is designed to be the default behavior. Turning it off requires an SPI flash which I believe is for the hub to load the configuration data to turn it off. OK, we have to endure this. >> >>> While this is no problem on Linux (the USB hub eventually gets detected >>> after >>> a delay of 2-3 seconds), the U-Boot USB stack (as it’s not interrupt driven) >>> usually misses the port-change event (i.e. the probing has already moved on >>> by the time the USB hub tries again). >>> >>> By having a callback directly from within the xHCI root-hub code, we can >>> ensure that we always catch the USB hub. We can’t fix this from the board >>> code, as the time between us enabling the regulator and the probing from >>> within ‘usb start’ will be unpredictable. My first instinct was to have a >>> call >>> to a board-specific weak-function from xhci, but I finally settled on using >>> the regulator-framework to have some that can be reused in the general >>> case and also allows for on-demand power-on/off of on-board USB devices >>> (if anybody has the need for it). >>> >> >> If turning off the low-power mode is not possible, there is already a >> weak function usb_hub_reset_devices() in current U-Boot USB stack. I >> think you can use that existing one? > > I don’t think so, as the on-module hub won’t even show up on the bus if > it’s in its low-power mode and usb_hub_configure (which in turn calls the > usb_hub_reset_devices-hook) wouldn’t even be invoked for this case. > usb_hub_configure() calls usb_hub_power_on() which in turn set the port feature (USB_PORT_FEAT_POWER). And usb_hub_reset_devices() is called right after usb_hub_power_on(). > Furthermore, usb_hub_reset_devices(int) only passes a port-number and > we need to be selective on a specific root-port of a specific controller (we > have 2 xHCIs active in our default DTS). Then this usb_hub_reset_devices() needs to be enhanced to provide an additional argument for the xHC controller. It's quite odd for the xHCI codes to handle any external power regulator requirements. The names xhci_board_port_poweron()/xhci_board_port_poweroff() mislead people to believe they are something related to the root port power, but in fact they are not. It's just for such low-power mode USB hub devices. Regards, Bin
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, ®name); + 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, ®ulator); + if (ret) { + debug("%s: ctrl '%s' port %d: could not get regulator '%s'\n", + __func__, dev_read_name(dev), req_index, regname); + return ret; + } + + regulator_set_enable(regulator, enable); +#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); +} + +/** * 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:
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@theobroma-systems.com> --- drivers/usb/host/xhci.c | 55 +++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 55 insertions(+)