| Message ID | 20200526145051.31520-1-mike.looijmans@topic.nl (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | usb/phy-generic: Add support for OTG VBUS supply control | expand |
Hi Mike,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on usb/usb-testing peter.chen-usb/ci-for-usb-next v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Mike-Looijmans/usb-phy-generic-Add-support-for-OTG-VBUS-supply-control/20200526-225304
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arm64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
drivers/usb/phy/phy-generic.c: In function 'nop_set_vbus':
>> drivers/usb/phy/phy-generic.c:234:1: warning: control reaches end of non-void function [-Wreturn-type]
234 | }
| ^
vim +234 drivers/usb/phy/phy-generic.c
205
206 static int nop_set_vbus(struct usb_otg *otg, bool enabled)
207 {
208 struct usb_phy_generic *nop;
209 int ret;
210
211 if (!otg)
212 return -ENODEV;
213
214 nop = container_of(otg->usb_phy, struct usb_phy_generic, phy);
215
216 if (!nop->vbus_reg)
217 return 0;
218
219 if (enabled) {
220 if (nop->vbus_reg_enabled)
221 return 0;
222 ret = regulator_enable(nop->vbus_reg);
223 if (ret < 0)
224 return ret;
225 nop->vbus_reg_enabled = true;
226 } else {
227 if (!nop->vbus_reg_enabled)
228 return 0;
229 ret = regulator_disable(nop->vbus_reg);
230 if (ret < 0)
231 return ret;
232 nop->vbus_reg_enabled = false;
233 }
> 234 }
235
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
Hi Mike,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on balbi-usb/testing/next]
[also build test WARNING on usb/usb-testing v5.7-rc7 next-20200526]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]
url: https://github.com/0day-ci/linux/commits/Mike-Looijmans/usb-phy-generic-Add-support-for-OTG-VBUS-supply-control/20200526-225304
base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git testing/next
config: x86_64-randconfig-a016-20200527 (attached as .config)
compiler: clang version 11.0.0 (https://github.com/llvm/llvm-project 3393cc4cebf9969db94dc424b7a2b6195589c33b)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# install x86_64 cross compiling tool for clang build
# apt-get install binutils-x86-64-linux-gnu
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=x86_64
If you fix the issue, kindly add following tag as appropriate
Reported-by: kbuild test robot <lkp@intel.com>
All warnings (new ones prefixed by >>, old ones prefixed by <<):
>> drivers/usb/phy/phy-generic.c:234:1: warning: non-void function does not return a value in all control paths [-Wreturn-type]
}
^
1 warning generated.
vim +234 drivers/usb/phy/phy-generic.c
205
206 static int nop_set_vbus(struct usb_otg *otg, bool enabled)
207 {
208 struct usb_phy_generic *nop;
209 int ret;
210
211 if (!otg)
212 return -ENODEV;
213
214 nop = container_of(otg->usb_phy, struct usb_phy_generic, phy);
215
216 if (!nop->vbus_reg)
217 return 0;
218
219 if (enabled) {
220 if (nop->vbus_reg_enabled)
221 return 0;
222 ret = regulator_enable(nop->vbus_reg);
223 if (ret < 0)
224 return ret;
225 nop->vbus_reg_enabled = true;
226 } else {
227 if (!nop->vbus_reg_enabled)
228 return 0;
229 ret = regulator_disable(nop->vbus_reg);
230 if (ret < 0)
231 return ret;
232 nop->vbus_reg_enabled = false;
233 }
> 234 }
235
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
On 20-05-26 16:50:51, Mike Looijmans wrote: > This enables support for VBUS on boards where the power is supplied > by a regulator. The regulator is enabled when the USB port enters > HOST mode. > Why you don't do this at your host controller driver? Peter > Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> > --- > .../devicetree/bindings/usb/usb-nop-xceiv.txt | 3 ++ > drivers/usb/phy/phy-generic.c | 44 ++++++++++++++++++- > drivers/usb/phy/phy-generic.h | 2 + > 3 files changed, 48 insertions(+), 1 deletion(-) > > diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt > index 4dc6a8ee3071..775a19fdb613 100644 > --- a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt > +++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt > @@ -16,6 +16,9 @@ Optional properties: > > - vcc-supply: phandle to the regulator that provides power to the PHY. > > +- vbus-supply: phandle to the regulator that provides the VBUS power for when > + the device is in HOST mode. > + > - reset-gpios: Should specify the GPIO for reset. > > - vbus-detect-gpio: should specify the GPIO detecting a VBus insertion > diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c > index 661a229c105d..ebfb90764511 100644 > --- a/drivers/usb/phy/phy-generic.c > +++ b/drivers/usb/phy/phy-generic.c > @@ -203,13 +203,43 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) > return 0; > } > > +static int nop_set_vbus(struct usb_otg *otg, bool enabled) > +{ > + struct usb_phy_generic *nop; > + int ret; > + > + if (!otg) > + return -ENODEV; > + > + nop = container_of(otg->usb_phy, struct usb_phy_generic, phy); > + > + if (!nop->vbus_reg) > + return 0; > + > + if (enabled) { > + if (nop->vbus_reg_enabled) > + return 0; > + ret = regulator_enable(nop->vbus_reg); > + if (ret < 0) > + return ret; > + nop->vbus_reg_enabled = true; > + } else { > + if (!nop->vbus_reg_enabled) > + return 0; > + ret = regulator_disable(nop->vbus_reg); > + if (ret < 0) > + return ret; > + nop->vbus_reg_enabled = false; > + } > +} > + > int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) > { > enum usb_phy_type type = USB_PHY_TYPE_USB2; > int err = 0; > > u32 clk_rate = 0; > - bool needs_vcc = false, needs_clk = false; > + bool needs_vcc = false, needs_clk = false, needs_vbus = false; > > if (dev->of_node) { > struct device_node *node = dev->of_node; > @@ -219,6 +249,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) > > needs_vcc = of_property_read_bool(node, "vcc-supply"); > needs_clk = of_property_read_bool(node, "clocks"); > + needs_vbus = of_property_read_bool(node, "vbus-supply"); > } > nop->gpiod_reset = devm_gpiod_get_optional(dev, "reset", > GPIOD_ASIS); > @@ -268,6 +299,16 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) > return -EPROBE_DEFER; > } > > + nop->vbus_reg = devm_regulator_get(dev, "vbus"); > + if (IS_ERR(nop->vbus_reg)) { > + dev_dbg(dev, "Error getting vbus regulator: %ld\n", > + PTR_ERR(nop->vbus_reg)); > + if (needs_vbus) > + return -EPROBE_DEFER; > + > + nop->vbus_reg = NULL; > + } > + > nop->dev = dev; > nop->phy.dev = nop->dev; > nop->phy.label = "nop-xceiv"; > @@ -278,6 +319,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) > nop->phy.otg->usb_phy = &nop->phy; > nop->phy.otg->set_host = nop_set_host; > nop->phy.otg->set_peripheral = nop_set_peripheral; > + nop->phy.otg->set_vbus = nop_set_vbus; > > return 0; > } > diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h > index 7ee80211a688..a3663639ea1e 100644 > --- a/drivers/usb/phy/phy-generic.h > +++ b/drivers/usb/phy/phy-generic.h > @@ -14,7 +14,9 @@ struct usb_phy_generic { > struct gpio_desc *gpiod_reset; > struct gpio_desc *gpiod_vbus; > struct regulator *vbus_draw; > + struct regulator *vbus_reg; > bool vbus_draw_enabled; > + bool vbus_reg_enabled; > unsigned long mA; > unsigned int vbus; > }; > -- > 2.17.1 >
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 28-05-2020 13:20, Peter Chen wrote: > On 20-05-26 16:50:51, Mike Looijmans wrote: >> This enables support for VBUS on boards where the power is supplied >> by a regulator. The regulator is enabled when the USB port enters >> HOST mode. >> > Why you don't do this at your host controller driver? That was my first thought too, but it made more sense to do it here. It's about how things are connected on the board, and not about the controller. Also makes for a consistent devicetree when using different SOCs on the same carrier board. I already needed this driver to properly reset the USB phy, which the controller driver also did not do. So it made sense to add the vbus power control to this driver too. If you have a strong preference for the controller driver, I can move this to the DWC3 driver which happens to be the controller for the latest batch of SOMs. > > Peter >> Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> >> --- >> .../devicetree/bindings/usb/usb-nop-xceiv.txt | 3 ++ >> drivers/usb/phy/phy-generic.c | 44 ++++++++++++++++++- >> drivers/usb/phy/phy-generic.h | 2 + >> 3 files changed, 48 insertions(+), 1 deletion(-) >> >> diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt >> index 4dc6a8ee3071..775a19fdb613 100644 >> --- a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt >> +++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt >> @@ -16,6 +16,9 @@ Optional properties: >> >> - vcc-supply: phandle to the regulator that provides power to the PHY. >> >> +- vbus-supply: phandle to the regulator that provides the VBUS power for when >> + the device is in HOST mode. >> + >> - reset-gpios: Should specify the GPIO for reset. >> >> - vbus-detect-gpio: should specify the GPIO detecting a VBus insertion >> diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c >> index 661a229c105d..ebfb90764511 100644 >> --- a/drivers/usb/phy/phy-generic.c >> +++ b/drivers/usb/phy/phy-generic.c >> @@ -203,13 +203,43 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) >> return 0; >> } >> >> +static int nop_set_vbus(struct usb_otg *otg, bool enabled) >> +{ >> + struct usb_phy_generic *nop; >> + int ret; >> + >> + if (!otg) >> + return -ENODEV; >> + >> + nop = container_of(otg->usb_phy, struct usb_phy_generic, phy); >> + >> + if (!nop->vbus_reg) >> + return 0; >> + >> + if (enabled) { >> + if (nop->vbus_reg_enabled) >> + return 0; >> + ret = regulator_enable(nop->vbus_reg); >> + if (ret < 0) >> + return ret; >> + nop->vbus_reg_enabled = true; >> + } else { >> + if (!nop->vbus_reg_enabled) >> + return 0; >> + ret = regulator_disable(nop->vbus_reg); >> + if (ret < 0) >> + return ret; >> + nop->vbus_reg_enabled = false; >> + } There's a "return 0" missing here, will fix that in v2 >> +} >> + >> int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) >> { >> enum usb_phy_type type = USB_PHY_TYPE_USB2; >> int err = 0; >> >> u32 clk_rate = 0; >> - bool needs_vcc = false, needs_clk = false; >> + bool needs_vcc = false, needs_clk = false, needs_vbus = false; >> >> if (dev->of_node) { >> struct device_node *node = dev->of_node; >> @@ -219,6 +249,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) >> >> needs_vcc = of_property_read_bool(node, "vcc-supply"); >> needs_clk = of_property_read_bool(node, "clocks"); >> + needs_vbus = of_property_read_bool(node, "vbus-supply"); >> } >> nop->gpiod_reset = devm_gpiod_get_optional(dev, "reset", >> GPIOD_ASIS); >> @@ -268,6 +299,16 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) >> return -EPROBE_DEFER; >> } >> >> + nop->vbus_reg = devm_regulator_get(dev, "vbus"); >> + if (IS_ERR(nop->vbus_reg)) { >> + dev_dbg(dev, "Error getting vbus regulator: %ld\n", >> + PTR_ERR(nop->vbus_reg)); >> + if (needs_vbus) >> + return -EPROBE_DEFER; >> + >> + nop->vbus_reg = NULL; >> + } >> + >> nop->dev = dev; >> nop->phy.dev = nop->dev; >> nop->phy.label = "nop-xceiv"; >> @@ -278,6 +319,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) >> nop->phy.otg->usb_phy = &nop->phy; >> nop->phy.otg->set_host = nop_set_host; >> nop->phy.otg->set_peripheral = nop_set_peripheral; >> + nop->phy.otg->set_vbus = nop_set_vbus; >> >> return 0; >> } >> diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h >> index 7ee80211a688..a3663639ea1e 100644 >> --- a/drivers/usb/phy/phy-generic.h >> +++ b/drivers/usb/phy/phy-generic.h >> @@ -14,7 +14,9 @@ struct usb_phy_generic { >> struct gpio_desc *gpiod_reset; >> struct gpio_desc *gpiod_vbus; >> struct regulator *vbus_draw; >> + struct regulator *vbus_reg; >> bool vbus_draw_enabled; >> + bool vbus_reg_enabled; >> unsigned long mA; >> unsigned int vbus; >> }; >> -- >> 2.17.1 >>
On 20-05-29 07:47:37, Mike Looijmans 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: https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fwww.topicproducts.com%2F&data=02%7C01%7Cpeter.chen%40nxp.com%7Cc8a5e33bbd3141b57d5408d80393cff2%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637263280668758097&sdata=e5yJuVtOJ9%2BUqmRl6HNk14Fd0%2FPR2jQAhOl1vHtYJKg%3D&reserved=0 > > Please consider the environment before printing this e-mail > On 28-05-2020 13:20, Peter Chen wrote: > > On 20-05-26 16:50:51, Mike Looijmans wrote: > > > This enables support for VBUS on boards where the power is supplied > > > by a regulator. The regulator is enabled when the USB port enters > > > HOST mode. > > > > > Why you don't do this at your host controller driver? > > That was my first thought too, but it made more sense to do it here. It's > about how things are connected on the board, and not about the controller. > Also makes for a consistent devicetree when using different SOCs on the same > carrier board. A standard interface is: port_power for hc_driver, but seems the xHCI doesn't implement it. > > I already needed this driver to properly reset the USB phy, which the > controller driver also did not do. So it made sense to add the vbus power > control to this driver too. > > If you have a strong preference for the controller driver, I can move this > to the DWC3 driver which happens to be the controller for the latest batch > of SOMs. > You may ask Mathias or Felipe for comments where is suitable for vbus regulator. > > > + > > > nop->dev = dev; > > > nop->phy.dev = nop->dev; > > > nop->phy.label = "nop-xceiv"; > > > @@ -278,6 +319,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) > > > nop->phy.otg->usb_phy = &nop->phy; > > > nop->phy.otg->set_host = nop_set_host; > > > nop->phy.otg->set_peripheral = nop_set_peripheral; > > > + nop->phy.otg->set_vbus = nop_set_vbus; OTG is dead, even USB-IF has refused OTG test since 2019-09, you may avoid to use OTG structure. Peter > > > return 0; > > > } > > > diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h > > > index 7ee80211a688..a3663639ea1e 100644 > > > --- a/drivers/usb/phy/phy-generic.h > > > +++ b/drivers/usb/phy/phy-generic.h > > > @@ -14,7 +14,9 @@ struct usb_phy_generic { > > > struct gpio_desc *gpiod_reset; > > > struct gpio_desc *gpiod_vbus; > > > struct regulator *vbus_draw; > > > + struct regulator *vbus_reg; > > > bool vbus_draw_enabled; > > > + bool vbus_reg_enabled; > > > unsigned long mA; > > > unsigned int vbus; > > > }; > > > -- > > > 2.17.1 > > > > > -- > Mike Looijmans >
diff --git a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt index 4dc6a8ee3071..775a19fdb613 100644 --- a/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt +++ b/Documentation/devicetree/bindings/usb/usb-nop-xceiv.txt @@ -16,6 +16,9 @@ Optional properties: - vcc-supply: phandle to the regulator that provides power to the PHY. +- vbus-supply: phandle to the regulator that provides the VBUS power for when + the device is in HOST mode. + - reset-gpios: Should specify the GPIO for reset. - vbus-detect-gpio: should specify the GPIO detecting a VBus insertion diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c index 661a229c105d..ebfb90764511 100644 --- a/drivers/usb/phy/phy-generic.c +++ b/drivers/usb/phy/phy-generic.c @@ -203,13 +203,43 @@ static int nop_set_host(struct usb_otg *otg, struct usb_bus *host) return 0; } +static int nop_set_vbus(struct usb_otg *otg, bool enabled) +{ + struct usb_phy_generic *nop; + int ret; + + if (!otg) + return -ENODEV; + + nop = container_of(otg->usb_phy, struct usb_phy_generic, phy); + + if (!nop->vbus_reg) + return 0; + + if (enabled) { + if (nop->vbus_reg_enabled) + return 0; + ret = regulator_enable(nop->vbus_reg); + if (ret < 0) + return ret; + nop->vbus_reg_enabled = true; + } else { + if (!nop->vbus_reg_enabled) + return 0; + ret = regulator_disable(nop->vbus_reg); + if (ret < 0) + return ret; + nop->vbus_reg_enabled = false; + } +} + int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) { enum usb_phy_type type = USB_PHY_TYPE_USB2; int err = 0; u32 clk_rate = 0; - bool needs_vcc = false, needs_clk = false; + bool needs_vcc = false, needs_clk = false, needs_vbus = false; if (dev->of_node) { struct device_node *node = dev->of_node; @@ -219,6 +249,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) needs_vcc = of_property_read_bool(node, "vcc-supply"); needs_clk = of_property_read_bool(node, "clocks"); + needs_vbus = of_property_read_bool(node, "vbus-supply"); } nop->gpiod_reset = devm_gpiod_get_optional(dev, "reset", GPIOD_ASIS); @@ -268,6 +299,16 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) return -EPROBE_DEFER; } + nop->vbus_reg = devm_regulator_get(dev, "vbus"); + if (IS_ERR(nop->vbus_reg)) { + dev_dbg(dev, "Error getting vbus regulator: %ld\n", + PTR_ERR(nop->vbus_reg)); + if (needs_vbus) + return -EPROBE_DEFER; + + nop->vbus_reg = NULL; + } + nop->dev = dev; nop->phy.dev = nop->dev; nop->phy.label = "nop-xceiv"; @@ -278,6 +319,7 @@ int usb_phy_gen_create_phy(struct device *dev, struct usb_phy_generic *nop) nop->phy.otg->usb_phy = &nop->phy; nop->phy.otg->set_host = nop_set_host; nop->phy.otg->set_peripheral = nop_set_peripheral; + nop->phy.otg->set_vbus = nop_set_vbus; return 0; } diff --git a/drivers/usb/phy/phy-generic.h b/drivers/usb/phy/phy-generic.h index 7ee80211a688..a3663639ea1e 100644 --- a/drivers/usb/phy/phy-generic.h +++ b/drivers/usb/phy/phy-generic.h @@ -14,7 +14,9 @@ struct usb_phy_generic { struct gpio_desc *gpiod_reset; struct gpio_desc *gpiod_vbus; struct regulator *vbus_draw; + struct regulator *vbus_reg; bool vbus_draw_enabled; + bool vbus_reg_enabled; unsigned long mA; unsigned int vbus; };
This enables support for VBUS on boards where the power is supplied by a regulator. The regulator is enabled when the USB port enters HOST mode. Signed-off-by: Mike Looijmans <mike.looijmans@topic.nl> --- .../devicetree/bindings/usb/usb-nop-xceiv.txt | 3 ++ drivers/usb/phy/phy-generic.c | 44 ++++++++++++++++++- drivers/usb/phy/phy-generic.h | 2 + 3 files changed, 48 insertions(+), 1 deletion(-)