| Message ID | 20200603120915.14001-1-mike.looijmans@topic.nl |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | [v2] usb: dwc3: Add support for VBUS power control | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 0 errors, 1 warnings, 117 lines checked |
On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: > Support VBUS power control using regulator framework. Enables the regulator > while the port is in host mode. > > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- > v2: Add missing devm_regulator_get call which got lost during rebase > > .../devicetree/bindings/usb/dwc3.txt | 1 + > drivers/usb/dwc3/core.c | 34 ++++++++++++++----- > drivers/usb/dwc3/core.h | 4 +++ > drivers/usb/dwc3/drd.c | 6 ++-- > 4 files changed, 33 insertions(+), 12 deletions(-) > > diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt > index 9946ff9ba735..56bc3f238e2d 100644 > --- a/Documentation/devicetree/bindings/usb/dwc3.txt > +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > @@ -37,6 +37,7 @@ Optional properties: > - phys: from the *Generic PHY* bindings > - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" > or "usb3-phy". > + - vbus-supply: Regulator handle that provides the VBUS power. Does the DWC3 block require Vbus to power itself? Doubtful. This belongs in a usb-connector node. If the DWC3 driver wants to get the Vbus supply, it can fetch it from that node. Rob
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topicproducts.com Please consider the environment before printing this e-mail On 10-06-2020 22:22, Rob Herring wrote: > On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: >> Support VBUS power control using regulator framework. Enables the regulator >> while the port is in host mode. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> v2: Add missing devm_regulator_get call which got lost during rebase >> >> .../devicetree/bindings/usb/dwc3.txt | 1 + >> drivers/usb/dwc3/core.c | 34 ++++++++++++++----- >> drivers/usb/dwc3/core.h | 4 +++ >> drivers/usb/dwc3/drd.c | 6 ++-- >> 4 files changed, 33 insertions(+), 12 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 9946ff9ba735..56bc3f238e2d 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -37,6 +37,7 @@ Optional properties: >> - phys: from the *Generic PHY* bindings >> - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" >> or "usb3-phy". >> + - vbus-supply: Regulator handle that provides the VBUS power. > Does the DWC3 block require Vbus to power itself? Doubtful. This > belongs in a usb-connector node. If the DWC3 driver wants to get the > Vbus supply, it can fetch it from that node. > > Rob Ah, now I understand. There's a vbus-supply in the connector in newer kernel versions than the one I have to test with. It'll have to wait until I've been able to upgrade the kernel version for this board.
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topicproducts.com Please consider the environment before printing this e-mail On 10-06-2020 22:22, Rob Herring wrote: > On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: >> Support VBUS power control using regulator framework. Enables the regulator >> while the port is in host mode. >> >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> v2: Add missing devm_regulator_get call which got lost during rebase >> >> .../devicetree/bindings/usb/dwc3.txt | 1 + >> drivers/usb/dwc3/core.c | 34 ++++++++++++++----- >> drivers/usb/dwc3/core.h | 4 +++ >> drivers/usb/dwc3/drd.c | 6 ++-- >> 4 files changed, 33 insertions(+), 12 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >> index 9946ff9ba735..56bc3f238e2d 100644 >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >> @@ -37,6 +37,7 @@ Optional properties: >> - phys: from the *Generic PHY* bindings >> - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" >> or "usb3-phy". >> + - vbus-supply: Regulator handle that provides the VBUS power. > Does the DWC3 block require Vbus to power itself? Doubtful. This > belongs in a usb-connector node. If the DWC3 driver wants to get the > Vbus supply, it can fetch it from that node. > > Rob Okay, I've been digging into that. But there's no actual driver that binds to a "usb-b-connector" compatible, so how do we get to the vbus-supply from there? [devm_]regulator_get only accepts a device as argument, and will not look into child nodes. The only way to get at the vbus of a child node (or a node linked through a port) would be to hand-code the equivalent of of_regulator_get(), which will not be acceptable. Or is it the intention that I write a usb-X-connector device driver first that handles the vbus?
On Wed, Jun 17, 2020 at 8:38 AM Mike Looijmans <mike.looijmans@topic.nl> wrote: > > > Met vriendelijke groet / kind regards, > > Mike Looijmans > System Expert > > > TOPIC Embedded Products B.V. > Materiaalweg 4, 5681 RJ Best > The Netherlands > > T: +31 (0) 499 33 69 69 > E: mike.looijmans@topicproducts.com > W: www.topicproducts.com > > Please consider the environment before printing this e-mail > On 10-06-2020 22:22, Rob Herring wrote: > > On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: > >> Support VBUS power control using regulator framework. Enables the regulator > >> while the port is in host mode. > >> > >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > >> --- > >> v2: Add missing devm_regulator_get call which got lost during rebase > >> > >> .../devicetree/bindings/usb/dwc3.txt | 1 + > >> drivers/usb/dwc3/core.c | 34 ++++++++++++++----- > >> drivers/usb/dwc3/core.h | 4 +++ > >> drivers/usb/dwc3/drd.c | 6 ++-- > >> 4 files changed, 33 insertions(+), 12 deletions(-) > >> > >> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt > >> index 9946ff9ba735..56bc3f238e2d 100644 > >> --- a/Documentation/devicetree/bindings/usb/dwc3.txt > >> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt > >> @@ -37,6 +37,7 @@ Optional properties: > >> - phys: from the *Generic PHY* bindings > >> - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" > >> or "usb3-phy". > >> + - vbus-supply: Regulator handle that provides the VBUS power. > > Does the DWC3 block require Vbus to power itself? Doubtful. This > > belongs in a usb-connector node. If the DWC3 driver wants to get the > > Vbus supply, it can fetch it from that node. > > > > Rob > > Okay, I've been digging into that. But there's no actual driver that > binds to a "usb-b-connector" compatible, so how do we get to the > vbus-supply from there? > > [devm_]regulator_get only accepts a device as argument, and will not > look into child nodes. The only way to get at the vbus of a child node > (or a node linked through a port) would be to hand-code the equivalent > of of_regulator_get(), which will not be acceptable. Doesn't it look into child nodes calling of_get_child_regulator()? You're right that it wouldn't work if graph is used. The connector has to be either a child of a controller for the connector or the USB controller. I'd expect you'd have the former for Type-C, but for "usb-b-connector" the parent is more likely just the USB controller. In any case, having a struct device shouldn't be a requirement and most subsystems expose a get function only needing the DT node. > Or is it the intention that I write a usb-X-connector device driver > first that handles the vbus? That's a kernel implementation detail that is independent of the binding, but yes we'll probably need a driver or library helper functions eventually. Note that it is possible to have a struct device without a driver. Rob
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topicproducts.com Please consider the environment before printing this e-mail On 17-06-2020 18:13, Rob Herring wrote: > On Wed, Jun 17, 2020 at 8:38 AM Mike Looijmans <mike.looijmans@topic.nl> wrote: >> >> Met vriendelijke groet / kind regards, >> >> Mike Looijmans >> System Expert >> >> >> TOPIC Embedded Products B.V. >> Materiaalweg 4, 5681 RJ Best >> The Netherlands >> >> T: +31 (0) 499 33 69 69 >> E: mike.looijmans@topicproducts.com >> W: www.topicproducts.com >> >> Please consider the environment before printing this e-mail >> On 10-06-2020 22:22, Rob Herring wrote: >>> On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: >>>> Support VBUS power control using regulator framework. Enables the regulator >>>> while the port is in host mode. >>>> >>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >>>> --- >>>> v2: Add missing devm_regulator_get call which got lost during rebase >>>> >>>> .../devicetree/bindings/usb/dwc3.txt | 1 + >>>> drivers/usb/dwc3/core.c | 34 ++++++++++++++----- >>>> drivers/usb/dwc3/core.h | 4 +++ >>>> drivers/usb/dwc3/drd.c | 6 ++-- >>>> 4 files changed, 33 insertions(+), 12 deletions(-) >>>> >>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> index 9946ff9ba735..56bc3f238e2d 100644 >>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>> @@ -37,6 +37,7 @@ Optional properties: >>>> - phys: from the *Generic PHY* bindings >>>> - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" >>>> or "usb3-phy". >>>> + - vbus-supply: Regulator handle that provides the VBUS power. >>> Does the DWC3 block require Vbus to power itself? Doubtful. This >>> belongs in a usb-connector node. If the DWC3 driver wants to get the >>> Vbus supply, it can fetch it from that node. >>> >>> Rob >> Okay, I've been digging into that. But there's no actual driver that >> binds to a "usb-b-connector" compatible, so how do we get to the >> vbus-supply from there? >> >> [devm_]regulator_get only accepts a device as argument, and will not >> look into child nodes. The only way to get at the vbus of a child node >> (or a node linked through a port) would be to hand-code the equivalent >> of of_regulator_get(), which will not be acceptable. > Doesn't it look into child nodes calling of_get_child_regulator()? Looking at the code in regulator/core.c, yeah, it should. I'll have to add some debugging lines and look into why it didn't work in my test. > You're right that it wouldn't work if graph is used. The connector has > to be either a child of a controller for the connector or the USB > controller. I'd expect you'd have the former for Type-C, but for > "usb-b-connector" the parent is more likely just the USB controller. For my current case, using a direct child would be fine, there's nothing else connected. But I expect that we'll produce a board with some USB-C connector some day yeah. > > In any case, having a struct device shouldn't be a requirement and > most subsystems expose a get function only needing the DT node. > >> Or is it the intention that I write a usb-X-connector device driver >> first that handles the vbus? > That's a kernel implementation detail that is independent of the > binding, but yes we'll probably need a driver or library helper > functions eventually. Note that it is possible to have a struct device > without a driver. > > Rob
Met vriendelijke groet / kind regards, Mike Looijmans System Expert TOPIC Embedded Products B.V. Materiaalweg 4, 5681 RJ Best The Netherlands T: +31 (0) 499 33 69 69 E: mike.looijmans@topicproducts.com W: www.topicproducts.com Please consider the environment before printing this e-mail On 17-06-2020 19:28, Mike Looijmans wrote: > On 17-06-2020 18:13, Rob Herring wrote: >> On Wed, Jun 17, 2020 at 8:38 AM Mike Looijmans >> <mike.looijmans@topic.nl> wrote: >>> On 10-06-2020 22:22, Rob Herring wrote: >>>> On Wed, Jun 03, 2020 at 02:09:15PM +0200, Mike Looijmans wrote: >>>>> Support VBUS power control using regulator framework. Enables the >>>>> regulator >>>>> while the port is in host mode. >>>>> >>>>> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >>>>> --- >>>>> v2: Add missing devm_regulator_get call which got lost during rebase >>>>> >>>>> .../devicetree/bindings/usb/dwc3.txt | 1 + >>>>> drivers/usb/dwc3/core.c | 34 >>>>> ++++++++++++++----- >>>>> drivers/usb/dwc3/core.h | 4 +++ >>>>> drivers/usb/dwc3/drd.c | 6 ++-- >>>>> 4 files changed, 33 insertions(+), 12 deletions(-) >>>>> >>>>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> b/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> index 9946ff9ba735..56bc3f238e2d 100644 >>>>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt >>>>> @@ -37,6 +37,7 @@ Optional properties: >>>>> - phys: from the *Generic PHY* bindings >>>>> - phy-names: from the *Generic PHY* bindings; supported names >>>>> are "usb2-phy" >>>>> or "usb3-phy". >>>>> + - vbus-supply: Regulator handle that provides the VBUS power. >>>> Does the DWC3 block require Vbus to power itself? Doubtful. This >>>> belongs in a usb-connector node. If the DWC3 driver wants to get the >>>> Vbus supply, it can fetch it from that node. >>>> >>>> Rob >>> Okay, I've been digging into that. But there's no actual driver that >>> binds to a "usb-b-connector" compatible, so how do we get to the >>> vbus-supply from there? >>> >>> [devm_]regulator_get only accepts a device as argument, and will not >>> look into child nodes. The only way to get at the vbus of a child node >>> (or a node linked through a port) would be to hand-code the equivalent >>> of of_regulator_get(), which will not be acceptable. >> Doesn't it look into child nodes calling of_get_child_regulator()? > Looking at the code in regulator/core.c, yeah, it should. I'll have to > add some debugging lines and look into why it didn't work in my test. Ah, I had put my "connector" child in the wrong node. If I add the following fragment to the dwc3 node, the vbus patch works: connector { compatible = "usb-b-connector"; label = "micro-USB-otg"; type = "micro"; vbus-supply = <®_usb0_vbus>; }; The driver indeed picks up the vbus supply from a child node. This would mean there's no change to the devicetree bindings, it's using already existing bindings. >> You're right that it wouldn't work if graph is used. The connector has >> to be either a child of a controller for the connector or the USB >> controller. I'd expect you'd have the former for Type-C, but for >> "usb-b-connector" the parent is more likely just the USB controller. > For my current case, using a direct child would be fine, there's > nothing else connected. But I expect that we'll produce a board with > some USB-C connector some day yeah. >> >> In any case, having a struct device shouldn't be a requirement and >> most subsystems expose a get function only needing the DT node. >> >>> Or is it the intention that I write a usb-X-connector device driver >>> first that handles the vbus? >> That's a kernel implementation detail that is independent of the >> binding, but yes we'll probably need a driver or library helper >> functions eventually. Note that it is possible to have a struct device >> without a driver. >> >> Rob > >
diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt b/Documentation/devicetree/bindings/usb/dwc3.txt index 9946ff9ba735..56bc3f238e2d 100644 --- a/Documentation/devicetree/bindings/usb/dwc3.txt +++ b/Documentation/devicetree/bindings/usb/dwc3.txt @@ -37,6 +37,7 @@ Optional properties: - phys: from the *Generic PHY* bindings - phy-names: from the *Generic PHY* bindings; supported names are "usb2-phy" or "usb3-phy". + - vbus-supply: Regulator handle that provides the VBUS power. - resets: set of phandle and reset specifier pairs - snps,usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM - snps,usb3_lpm_capable: determines if platform is USB3 LPM capable diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index edc17155cb2b..ad341a182999 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -25,6 +25,7 @@ #include <linux/of.h> #include <linux/acpi.h> #include <linux/pinctrl/consumer.h> +#include <linux/regulator/consumer.h> #include <linux/reset.h> #include <linux/usb/ch9.h> @@ -112,6 +113,23 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) dwc->current_dr_role = mode; } +void dwc3_set_vbus(struct dwc3 *dwc, bool enable) +{ + int ret; + + if (enable != dwc->vbus_reg_enabled) { + if (enable) + ret = regulator_enable(dwc->vbus_reg); + else + ret = regulator_disable(dwc->vbus_reg); + if (!ret) + dwc->vbus_reg_enabled = enable; + } + + if (dwc->usb2_phy) + otg_set_vbus(dwc->usb2_phy->otg, enable); +} + static void __dwc3_set_mode(struct work_struct *work) { struct dwc3 *dwc = work_to_dwc(work); @@ -164,8 +182,7 @@ static void __dwc3_set_mode(struct work_struct *work) if (ret) { dev_err(dwc->dev, "failed to initialize host\n"); } else { - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, true); + dwc3_set_vbus(dwc, true); phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); } @@ -173,8 +190,7 @@ static void __dwc3_set_mode(struct work_struct *work) case DWC3_GCTL_PRTCAP_DEVICE: dwc3_event_buffers_setup(dwc); - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, false); + dwc3_set_vbus(dwc, false); phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); @@ -1183,8 +1199,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) case USB_DR_MODE_PERIPHERAL: dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_DEVICE); - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, false); + dwc3_set_vbus(dwc, false); phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE); phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_DEVICE); @@ -1198,8 +1213,7 @@ static int dwc3_core_init_mode(struct dwc3 *dwc) case USB_DR_MODE_HOST: dwc3_set_prtcap(dwc, DWC3_GCTL_PRTCAP_HOST); - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, true); + dwc3_set_vbus(dwc, true); phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); phy_set_mode(dwc->usb3_generic_phy, PHY_MODE_USB_HOST); @@ -1470,6 +1484,10 @@ static int dwc3_probe(struct platform_device *pdev) dwc3_get_properties(dwc); + dwc->vbus_reg = devm_regulator_get_optional(dev, "vbus"); + if (IS_ERR(dwc->vbus_reg)) + return PTR_ERR(dwc->vbus_reg); + dwc->reset = devm_reset_control_array_get(dev, true, true); if (IS_ERR(dwc->reset)) return PTR_ERR(dwc->reset); diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index 4c171a8e215f..cee2574d7bf4 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1085,6 +1085,9 @@ struct dwc3 { bool phys_ready; + struct regulator *vbus_reg; + bool vbus_reg_enabled; + struct ulpi *ulpi; bool ulpi_ready; @@ -1397,6 +1400,7 @@ struct dwc3_gadget_ep_cmd_params { /* prototypes */ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode); +void dwc3_set_vbus(struct dwc3 *dwc, bool enable); void dwc3_set_mode(struct dwc3 *dwc, u32 mode); u32 dwc3_core_fifo_space(struct dwc3_ep *dep, u8 type); diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c index 7db1ffc92bbd..45fdec2d128d 100644 --- a/drivers/usb/dwc3/drd.c +++ b/drivers/usb/dwc3/drd.c @@ -384,8 +384,7 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) if (ret) { dev_err(dwc->dev, "failed to initialize host\n"); } else { - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, true); + dwc3_set_vbus(dwc, true); if (dwc->usb2_generic_phy) phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_HOST); @@ -398,8 +397,7 @@ void dwc3_otg_update(struct dwc3 *dwc, bool ignore_idstatus) dwc3_event_buffers_setup(dwc); spin_unlock_irqrestore(&dwc->lock, flags); - if (dwc->usb2_phy) - otg_set_vbus(dwc->usb2_phy->otg, false); + dwc3_set_vbus(dwc, false); if (dwc->usb2_generic_phy) phy_set_mode(dwc->usb2_generic_phy, PHY_MODE_USB_DEVICE);
Support VBUS power control using regulator framework. Enables the regulator while the port is in host mode. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- v2: Add missing devm_regulator_get call which got lost during rebase .../devicetree/bindings/usb/dwc3.txt | 1 + drivers/usb/dwc3/core.c | 34 ++++++++++++++----- drivers/usb/dwc3/core.h | 4 +++ drivers/usb/dwc3/drd.c | 6 ++-- 4 files changed, 33 insertions(+), 12 deletions(-)