| Message ID | 1497259182-3568-5-git-send-email-daniel.meng@rock-chips.com |
|---|---|
| State | Superseded |
| Delegated to: | Philipp Tomsich |
| Headers | show |
On 06/12/2017 11:19 AM, Meng Dongyang wrote: > Use fixed regulator to control the voltage of vbus and turn off > vbus when usb stop. > > Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> > --- > > Changes in v5: > - Propagate return value and print error message when failed > > Changes in v4: > - Splited from patch [Uboot,v3,04/10] > - Define set vbus as empty function if the macros aren't set > > Changes in v3: None > Changes in v2: > - Use fixed regulator to control vbus instead of gpio > > drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- > 1 file changed, 42 insertions(+), 13 deletions(-) > > diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c > index f559830..15df6ef 100644 > --- a/drivers/usb/host/xhci-rockchip.c > +++ b/drivers/usb/host/xhci-rockchip.c > @@ -11,10 +11,10 @@ > #include <malloc.h> > #include <usb.h> > #include <watchdog.h> > -#include <asm/gpio.h> > #include <linux/errno.h> > #include <linux/compat.h> > #include <linux/usb/dwc3.h> > +#include <power/regulator.h> > > #include "xhci.h" > > @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; > struct rockchip_xhci_platdata { > fdt_addr_t hcd_base; > fdt_addr_t phy_base; > - struct gpio_desc vbus_gpio; > + struct udevice *vbus_supply; > }; > > /* > @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) > */ > plat->hcd_base = dev_get_addr(dev); > if (plat->hcd_base == FDT_ADDR_T_NONE) { > - debug("Can't get the XHCI register base address\n"); > + error("Can't get the XHCI register base address\n"); > return -ENXIO; > } > > @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) > } > > if (plat->phy_base == FDT_ADDR_T_NONE) { > - debug("Can't get the usbphy register address\n"); > + error("Can't get the usbphy register address\n"); > return -ENXIO; > } > > - /* Vbus gpio */ > - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, > - &plat->vbus_gpio, GPIOD_IS_OUT); > +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) I don't think you need the CONFIG_DM_USB , the driver depends on it (or should) already anyway. > + /* Vbus regulator */ > + ret = device_get_supply_regulator(dev, "vbus-supply", > + &plat->vbus_supply); So I was curious, does this expand to empty function or is this not defined if CONFIG_DM_REGULATOR is not defined ? > if (ret) > - debug("rockchip,vbus-gpio node missing!"); > + debug("Can't get VBus regulator!\n"); > +#endif > > return 0; > } > > +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) > +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) > +{ > + int ret; > + > + ret = regulator_set_enable(vbus_supply, value); > + if (ret) > + error("XHCI: Failed to set vbus supply\n"); Please be consistent with the VBus usage. You use VBus above and vbus here. > + > + return ret; > +} > +#else > +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) > +{ > + return 0; > +} > +#endif > + > /* > * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core > * @dwc: Pointer to our controller context structure > @@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci, > > ret = dwc3_core_init(rkxhci->dwc3_reg); > if (ret) { > - debug("failed to initialize core\n"); > + error("failed to initialize core\n"); > return ret; > } > > @@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) > hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + > HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase))); > > - /* setup the Vbus gpio here */ > - if (dm_gpio_is_valid(&plat->vbus_gpio)) > - dm_gpio_set_value(&plat->vbus_gpio, 1); > + if (plat->vbus_supply) { > + ret = rockchip_xhci_set_vbus(plat->vbus_supply, true); > + if (ret) > + return ret; > + } > > ret = rockchip_xhci_core_init(ctx, dev); > if (ret) { > - debug("XHCI: failed to initialize controller\n"); > + error("XHCI: failed to initialize controller\n"); > return ret; > } > > @@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev) > > static int xhci_usb_remove(struct udevice *dev) > { > + struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); > struct rockchip_xhci *ctx = dev_get_priv(dev); > int ret; > > @@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) > if (ret) > return ret; > > + if (plat->vbus_supply) { > + ret = rockchip_xhci_set_vbus(plat->vbus_supply, false); > + if (ret) > + return ret; > + } > + > return 0; Just do return ret here, then you don't need the if (ret) above. > } > >
On 2017/6/13 17:11, Marek Vasut wrote: > On 06/12/2017 11:19 AM, Meng Dongyang wrote: >> Use fixed regulator to control the voltage of vbus and turn off >> vbus when usb stop. >> >> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >> --- >> >> Changes in v5: >> - Propagate return value and print error message when failed >> >> Changes in v4: >> - Splited from patch [Uboot,v3,04/10] >> - Define set vbus as empty function if the macros aren't set >> >> Changes in v3: None >> Changes in v2: >> - Use fixed regulator to control vbus instead of gpio >> >> drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- >> 1 file changed, 42 insertions(+), 13 deletions(-) >> >> diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c >> index f559830..15df6ef 100644 >> --- a/drivers/usb/host/xhci-rockchip.c >> +++ b/drivers/usb/host/xhci-rockchip.c >> @@ -11,10 +11,10 @@ >> #include <malloc.h> >> #include <usb.h> >> #include <watchdog.h> >> -#include <asm/gpio.h> >> #include <linux/errno.h> >> #include <linux/compat.h> >> #include <linux/usb/dwc3.h> >> +#include <power/regulator.h> >> >> #include "xhci.h" >> >> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >> struct rockchip_xhci_platdata { >> fdt_addr_t hcd_base; >> fdt_addr_t phy_base; >> - struct gpio_desc vbus_gpio; >> + struct udevice *vbus_supply; >> }; >> >> /* >> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) >> */ >> plat->hcd_base = dev_get_addr(dev); >> if (plat->hcd_base == FDT_ADDR_T_NONE) { >> - debug("Can't get the XHCI register base address\n"); >> + error("Can't get the XHCI register base address\n"); >> return -ENXIO; >> } >> >> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) >> } >> >> if (plat->phy_base == FDT_ADDR_T_NONE) { >> - debug("Can't get the usbphy register address\n"); >> + error("Can't get the usbphy register address\n"); >> return -ENXIO; >> } >> >> - /* Vbus gpio */ >> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >> - &plat->vbus_gpio, GPIOD_IS_OUT); >> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) > I don't think you need the CONFIG_DM_USB , the driver depends on it (or > should) already anyway. Yes, I will remove it in v6. > >> + /* Vbus regulator */ >> + ret = device_get_supply_regulator(dev, "vbus-supply", >> + &plat->vbus_supply); > So I was curious, does this expand to empty function or is this not > defined if CONFIG_DM_REGULATOR is not defined ? This is not defined if CONFIG_DM_REGULATOR is not defined. > >> if (ret) >> - debug("rockchip,vbus-gpio node missing!"); >> + debug("Can't get VBus regulator!\n"); >> +#endif >> >> return 0; >> } >> >> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >> +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) >> +{ >> + int ret; >> + >> + ret = regulator_set_enable(vbus_supply, value); >> + if (ret) >> + error("XHCI: Failed to set vbus supply\n"); > Please be consistent with the VBus usage. You use VBus above and vbus here. OK > >> + >> + return ret; >> +} >> +#else >> +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) >> +{ >> + return 0; >> +} >> +#endif >> + >> /* >> * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core >> * @dwc: Pointer to our controller context structure >> @@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci, >> >> ret = dwc3_core_init(rkxhci->dwc3_reg); >> if (ret) { >> - debug("failed to initialize core\n"); >> + error("failed to initialize core\n"); >> return ret; >> } >> >> @@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) >> hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + >> HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase))); >> >> - /* setup the Vbus gpio here */ >> - if (dm_gpio_is_valid(&plat->vbus_gpio)) >> - dm_gpio_set_value(&plat->vbus_gpio, 1); >> + if (plat->vbus_supply) { >> + ret = rockchip_xhci_set_vbus(plat->vbus_supply, true); >> + if (ret) >> + return ret; >> + } >> >> ret = rockchip_xhci_core_init(ctx, dev); >> if (ret) { >> - debug("XHCI: failed to initialize controller\n"); >> + error("XHCI: failed to initialize controller\n"); >> return ret; >> } >> >> @@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev) >> >> static int xhci_usb_remove(struct udevice *dev) >> { >> + struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); >> struct rockchip_xhci *ctx = dev_get_priv(dev); >> int ret; >> >> @@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) >> if (ret) >> return ret; >> >> + if (plat->vbus_supply) { >> + ret = rockchip_xhci_set_vbus(plat->vbus_supply, false); >> + if (ret) >> + return ret; >> + } >> + >> return 0; > Just do return ret here, then you don't need the if (ret) above. OK > >> } >> >> >
On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: > > > On 2017/6/13 17:11, Marek Vasut wrote: >> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>> Use fixed regulator to control the voltage of vbus and turn off >>> vbus when usb stop. >>> >>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>> --- >>> >>> Changes in v5: >>> - Propagate return value and print error message when failed >>> >>> Changes in v4: >>> - Splited from patch [Uboot,v3,04/10] >>> - Define set vbus as empty function if the macros aren't set >>> >>> Changes in v3: None >>> Changes in v2: >>> - Use fixed regulator to control vbus instead of gpio >>> >>> drivers/usb/host/xhci-rockchip.c | 55 >>> ++++++++++++++++++++++++++++++---------- >>> 1 file changed, 42 insertions(+), 13 deletions(-) >>> >>> diff --git a/drivers/usb/host/xhci-rockchip.c >>> b/drivers/usb/host/xhci-rockchip.c >>> index f559830..15df6ef 100644 >>> --- a/drivers/usb/host/xhci-rockchip.c >>> +++ b/drivers/usb/host/xhci-rockchip.c >>> @@ -11,10 +11,10 @@ >>> #include <malloc.h> >>> #include <usb.h> >>> #include <watchdog.h> >>> -#include <asm/gpio.h> >>> #include <linux/errno.h> >>> #include <linux/compat.h> >>> #include <linux/usb/dwc3.h> >>> +#include <power/regulator.h> >>> #include "xhci.h" >>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>> struct rockchip_xhci_platdata { >>> fdt_addr_t hcd_base; >>> fdt_addr_t phy_base; >>> - struct gpio_desc vbus_gpio; >>> + struct udevice *vbus_supply; >>> }; >>> /* >>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>> udevice *dev) >>> */ >>> plat->hcd_base = dev_get_addr(dev); >>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>> - debug("Can't get the XHCI register base address\n"); >>> + error("Can't get the XHCI register base address\n"); >>> return -ENXIO; >>> } >>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>> udevice *dev) >>> } >>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>> - debug("Can't get the usbphy register address\n"); >>> + error("Can't get the usbphy register address\n"); >>> return -ENXIO; >>> } >>> - /* Vbus gpio */ >>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >> should) already anyway. > > Yes, I will remove it in v6. >> >>> + /* Vbus regulator */ >>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>> + &plat->vbus_supply); >> So I was curious, does this expand to empty function or is this not >> defined if CONFIG_DM_REGULATOR is not defined ? > This is not defined if CONFIG_DM_REGULATOR is not defined. Simon, can you comment on this ? Daniel, I recommend you fix your mailer to insert at least one newline before the reply text.
Hi Marek, On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: > On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >> >> >> On 2017/6/13 17:11, Marek Vasut wrote: >>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>> Use fixed regulator to control the voltage of vbus and turn off >>>> vbus when usb stop. >>>> >>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>> --- >>>> >>>> Changes in v5: >>>> - Propagate return value and print error message when failed >>>> >>>> Changes in v4: >>>> - Splited from patch [Uboot,v3,04/10] >>>> - Define set vbus as empty function if the macros aren't set >>>> >>>> Changes in v3: None >>>> Changes in v2: >>>> - Use fixed regulator to control vbus instead of gpio >>>> >>>> drivers/usb/host/xhci-rockchip.c | 55 >>>> ++++++++++++++++++++++++++++++---------- >>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>> >>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>> b/drivers/usb/host/xhci-rockchip.c >>>> index f559830..15df6ef 100644 >>>> --- a/drivers/usb/host/xhci-rockchip.c >>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>> @@ -11,10 +11,10 @@ >>>> #include <malloc.h> >>>> #include <usb.h> >>>> #include <watchdog.h> >>>> -#include <asm/gpio.h> >>>> #include <linux/errno.h> >>>> #include <linux/compat.h> >>>> #include <linux/usb/dwc3.h> >>>> +#include <power/regulator.h> >>>> #include "xhci.h" >>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>> struct rockchip_xhci_platdata { >>>> fdt_addr_t hcd_base; >>>> fdt_addr_t phy_base; >>>> - struct gpio_desc vbus_gpio; >>>> + struct udevice *vbus_supply; >>>> }; >>>> /* >>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>> udevice *dev) >>>> */ >>>> plat->hcd_base = dev_get_addr(dev); >>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>> - debug("Can't get the XHCI register base address\n"); >>>> + error("Can't get the XHCI register base address\n"); >>>> return -ENXIO; >>>> } >>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>> udevice *dev) >>>> } >>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>> - debug("Can't get the usbphy register address\n"); >>>> + error("Can't get the usbphy register address\n"); >>>> return -ENXIO; >>>> } >>>> - /* Vbus gpio */ >>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>> should) already anyway. >> >> Yes, I will remove it in v6. >>> >>>> + /* Vbus regulator */ >>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>> + &plat->vbus_supply); >>> So I was curious, does this expand to empty function or is this not >>> defined if CONFIG_DM_REGULATOR is not defined ? >> This is not defined if CONFIG_DM_REGULATOR is not defined. > > Simon, can you comment on this ? It looks OK to me. > > Daniel, I recommend you fix your mailer to insert at least one newline > before the reply text. > > -- > Best regards, > Marek Vasut
On 06/17/2017 05:41 AM, Simon Glass wrote: > Hi Marek, > > On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>> >>> >>> On 2017/6/13 17:11, Marek Vasut wrote: >>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>> vbus when usb stop. >>>>> >>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>> --- >>>>> >>>>> Changes in v5: >>>>> - Propagate return value and print error message when failed >>>>> >>>>> Changes in v4: >>>>> - Splited from patch [Uboot,v3,04/10] >>>>> - Define set vbus as empty function if the macros aren't set >>>>> >>>>> Changes in v3: None >>>>> Changes in v2: >>>>> - Use fixed regulator to control vbus instead of gpio >>>>> >>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>> ++++++++++++++++++++++++++++++---------- >>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>> >>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>> b/drivers/usb/host/xhci-rockchip.c >>>>> index f559830..15df6ef 100644 >>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>> @@ -11,10 +11,10 @@ >>>>> #include <malloc.h> >>>>> #include <usb.h> >>>>> #include <watchdog.h> >>>>> -#include <asm/gpio.h> >>>>> #include <linux/errno.h> >>>>> #include <linux/compat.h> >>>>> #include <linux/usb/dwc3.h> >>>>> +#include <power/regulator.h> >>>>> #include "xhci.h" >>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>> struct rockchip_xhci_platdata { >>>>> fdt_addr_t hcd_base; >>>>> fdt_addr_t phy_base; >>>>> - struct gpio_desc vbus_gpio; >>>>> + struct udevice *vbus_supply; >>>>> }; >>>>> /* >>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>> udevice *dev) >>>>> */ >>>>> plat->hcd_base = dev_get_addr(dev); >>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>> - debug("Can't get the XHCI register base address\n"); >>>>> + error("Can't get the XHCI register base address\n"); >>>>> return -ENXIO; >>>>> } >>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>> udevice *dev) >>>>> } >>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>> - debug("Can't get the usbphy register address\n"); >>>>> + error("Can't get the usbphy register address\n"); >>>>> return -ENXIO; >>>>> } >>>>> - /* Vbus gpio */ >>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>> should) already anyway. >>> >>> Yes, I will remove it in v6. >>>> >>>>> + /* Vbus regulator */ >>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>> + &plat->vbus_supply); >>>> So I was curious, does this expand to empty function or is this not >>>> defined if CONFIG_DM_REGULATOR is not defined ? >>> This is not defined if CONFIG_DM_REGULATOR is not defined. >> >> Simon, can you comment on this ? > > It looks OK to me. Shouldn't you have empty stub functions instead to avoid proliferation of adhoc #ifdef CONFIG_FOO throughout the codebase ?
Hi Marek, On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: > On 06/17/2017 05:41 AM, Simon Glass wrote: >> Hi Marek, >> >> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>> >>>> >>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>> vbus when usb stop. >>>>>> >>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>> --- >>>>>> >>>>>> Changes in v5: >>>>>> - Propagate return value and print error message when failed >>>>>> >>>>>> Changes in v4: >>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>> - Define set vbus as empty function if the macros aren't set >>>>>> >>>>>> Changes in v3: None >>>>>> Changes in v2: >>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>> >>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>> ++++++++++++++++++++++++++++++---------- >>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>> >>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>> index f559830..15df6ef 100644 >>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>> @@ -11,10 +11,10 @@ >>>>>> #include <malloc.h> >>>>>> #include <usb.h> >>>>>> #include <watchdog.h> >>>>>> -#include <asm/gpio.h> >>>>>> #include <linux/errno.h> >>>>>> #include <linux/compat.h> >>>>>> #include <linux/usb/dwc3.h> >>>>>> +#include <power/regulator.h> >>>>>> #include "xhci.h" >>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>> struct rockchip_xhci_platdata { >>>>>> fdt_addr_t hcd_base; >>>>>> fdt_addr_t phy_base; >>>>>> - struct gpio_desc vbus_gpio; >>>>>> + struct udevice *vbus_supply; >>>>>> }; >>>>>> /* >>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>> udevice *dev) >>>>>> */ >>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>> + error("Can't get the XHCI register base address\n"); >>>>>> return -ENXIO; >>>>>> } >>>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>> udevice *dev) >>>>>> } >>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>> - debug("Can't get the usbphy register address\n"); >>>>>> + error("Can't get the usbphy register address\n"); >>>>>> return -ENXIO; >>>>>> } >>>>>> - /* Vbus gpio */ >>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>>> should) already anyway. >>>> >>>> Yes, I will remove it in v6. >>>>> >>>>>> + /* Vbus regulator */ >>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>> + &plat->vbus_supply); >>>>> So I was curious, does this expand to empty function or is this not >>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>> >>> Simon, can you comment on this ? >> >> It looks OK to me. > > Shouldn't you have empty stub functions instead to avoid proliferation > of adhoc #ifdef CONFIG_FOO throughout the codebase ? You could, but this is just a temporary state while apparently some rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what those bad boards are, actually. Regards, Simon
On 06/17/2017 07:28 PM, Simon Glass wrote: > Hi Marek, > > On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >> On 06/17/2017 05:41 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>> >>>>> >>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>> vbus when usb stop. >>>>>>> >>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>> --- >>>>>>> >>>>>>> Changes in v5: >>>>>>> - Propagate return value and print error message when failed >>>>>>> >>>>>>> Changes in v4: >>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>> >>>>>>> Changes in v3: None >>>>>>> Changes in v2: >>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>> >>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>> >>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>> index f559830..15df6ef 100644 >>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>> @@ -11,10 +11,10 @@ >>>>>>> #include <malloc.h> >>>>>>> #include <usb.h> >>>>>>> #include <watchdog.h> >>>>>>> -#include <asm/gpio.h> >>>>>>> #include <linux/errno.h> >>>>>>> #include <linux/compat.h> >>>>>>> #include <linux/usb/dwc3.h> >>>>>>> +#include <power/regulator.h> >>>>>>> #include "xhci.h" >>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>> struct rockchip_xhci_platdata { >>>>>>> fdt_addr_t hcd_base; >>>>>>> fdt_addr_t phy_base; >>>>>>> - struct gpio_desc vbus_gpio; >>>>>>> + struct udevice *vbus_supply; >>>>>>> }; >>>>>>> /* >>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>> udevice *dev) >>>>>>> */ >>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>> return -ENXIO; >>>>>>> } >>>>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>> udevice *dev) >>>>>>> } >>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>> return -ENXIO; >>>>>>> } >>>>>>> - /* Vbus gpio */ >>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>>>> should) already anyway. >>>>> >>>>> Yes, I will remove it in v6. >>>>>> >>>>>>> + /* Vbus regulator */ >>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>> + &plat->vbus_supply); >>>>>> So I was curious, does this expand to empty function or is this not >>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>> >>>> Simon, can you comment on this ? >>> >>> It looks OK to me. >> >> Shouldn't you have empty stub functions instead to avoid proliferation >> of adhoc #ifdef CONFIG_FOO throughout the codebase ? > > You could, but this is just a temporary state while apparently some > rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what > those bad boards are, actually. Temporary state ? What's the final state then ? Anyway, what if you just disable regulator support (because you don't need it for whatever reason), but want to keep USB ? Wouldn't it make more sense to have empty stub functions instead of swamping the drivers with ifdefs ?
Hi Marek, On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de> wrote: > On 06/17/2017 07:28 PM, Simon Glass wrote: >> Hi Marek, >> >> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>> >>>>>> >>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>> vbus when usb stop. >>>>>>>> >>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>>> --- >>>>>>>> >>>>>>>> Changes in v5: >>>>>>>> - Propagate return value and print error message when failed >>>>>>>> >>>>>>>> Changes in v4: >>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>> >>>>>>>> Changes in v3: None >>>>>>>> Changes in v2: >>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>> >>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>> >>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>> index f559830..15df6ef 100644 >>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>> #include <malloc.h> >>>>>>>> #include <usb.h> >>>>>>>> #include <watchdog.h> >>>>>>>> -#include <asm/gpio.h> >>>>>>>> #include <linux/errno.h> >>>>>>>> #include <linux/compat.h> >>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>> +#include <power/regulator.h> >>>>>>>> #include "xhci.h" >>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>> struct rockchip_xhci_platdata { >>>>>>>> fdt_addr_t hcd_base; >>>>>>>> fdt_addr_t phy_base; >>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>> + struct udevice *vbus_supply; >>>>>>>> }; >>>>>>>> /* >>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>> udevice *dev) >>>>>>>> */ >>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>> return -ENXIO; >>>>>>>> } >>>>>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>> udevice *dev) >>>>>>>> } >>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>> return -ENXIO; >>>>>>>> } >>>>>>>> - /* Vbus gpio */ >>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>>>>> should) already anyway. >>>>>> >>>>>> Yes, I will remove it in v6. >>>>>>> >>>>>>>> + /* Vbus regulator */ >>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>> + &plat->vbus_supply); >>>>>>> So I was curious, does this expand to empty function or is this not >>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>> >>>>> Simon, can you comment on this ? >>>> >>>> It looks OK to me. >>> >>> Shouldn't you have empty stub functions instead to avoid proliferation >>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >> >> You could, but this is just a temporary state while apparently some >> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >> those bad boards are, actually. > > Temporary state ? What's the final state then ? Well I wasn't aware that any rockchip boards didn't use regulators. Presumably this #ifdef is because of a board that does not. > > Anyway, what if you just disable regulator support (because you don't > need it for whatever reason), but want to keep USB ? Wouldn't it make > more sense to have empty stub functions instead of swamping the drivers > with ifdefs ? USB would not work without VBUS, which is handled with regulators, so there is something I don't understand here. Anyway I don't see any point in adding stub functions here. Meng can you please explain why the #ifdef is needed? Regards, Simon
On 06/18/2017 12:10 AM, Simon Glass wrote: > Hi Marek, > > On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de> wrote: >> On 06/17/2017 07:28 PM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >>>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>>> >>>>>>> >>>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>>> vbus when usb stop. >>>>>>>>> >>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>>>> --- >>>>>>>>> >>>>>>>>> Changes in v5: >>>>>>>>> - Propagate return value and print error message when failed >>>>>>>>> >>>>>>>>> Changes in v4: >>>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>>> >>>>>>>>> Changes in v3: None >>>>>>>>> Changes in v2: >>>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>>> >>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>>> >>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>>> index f559830..15df6ef 100644 >>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>>> #include <malloc.h> >>>>>>>>> #include <usb.h> >>>>>>>>> #include <watchdog.h> >>>>>>>>> -#include <asm/gpio.h> >>>>>>>>> #include <linux/errno.h> >>>>>>>>> #include <linux/compat.h> >>>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>>> +#include <power/regulator.h> >>>>>>>>> #include "xhci.h" >>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>>> struct rockchip_xhci_platdata { >>>>>>>>> fdt_addr_t hcd_base; >>>>>>>>> fdt_addr_t phy_base; >>>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>>> + struct udevice *vbus_supply; >>>>>>>>> }; >>>>>>>>> /* >>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>> udevice *dev) >>>>>>>>> */ >>>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>>> return -ENXIO; >>>>>>>>> } >>>>>>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>> udevice *dev) >>>>>>>>> } >>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>>> return -ENXIO; >>>>>>>>> } >>>>>>>>> - /* Vbus gpio */ >>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>>>>>> should) already anyway. >>>>>>> >>>>>>> Yes, I will remove it in v6. >>>>>>>> >>>>>>>>> + /* Vbus regulator */ >>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>>> + &plat->vbus_supply); >>>>>>>> So I was curious, does this expand to empty function or is this not >>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>>> >>>>>> Simon, can you comment on this ? >>>>> >>>>> It looks OK to me. >>>> >>>> Shouldn't you have empty stub functions instead to avoid proliferation >>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >>> >>> You could, but this is just a temporary state while apparently some >>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >>> those bad boards are, actually. >> >> Temporary state ? What's the final state then ? > > Well I wasn't aware that any rockchip boards didn't use regulators. > Presumably this #ifdef is because of a board that does not. This is IMO unrelated to rockchip. >> Anyway, what if you just disable regulator support (because you don't >> need it for whatever reason), but want to keep USB ? Wouldn't it make >> more sense to have empty stub functions instead of swamping the drivers >> with ifdefs ? > > USB would not work without VBUS, which is handled with regulators The VBUS can be directly tied to 5V rail without power switching. >, so > there is something I don't understand here. Anyway I don't see any > point in adding stub functions here. Not here, into the regulator framework, so you don't have to pollute drivers which use the regulators with ifdefs if the regulator framework is not enabled in the config. > Meng can you please explain why the #ifdef is needed? > > Regards, > Simon >
On 2017/6/18 13:11, Marek Vasut wrote: > On 06/18/2017 12:10 AM, Simon Glass wrote: >> Hi Marek, >> >> On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de> wrote: >>> On 06/17/2017 07:28 PM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >>>>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>>>> >>>>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>>>> vbus when usb stop. >>>>>>>>>> >>>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>>>>> --- >>>>>>>>>> >>>>>>>>>> Changes in v5: >>>>>>>>>> - Propagate return value and print error message when failed >>>>>>>>>> >>>>>>>>>> Changes in v4: >>>>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>>>> >>>>>>>>>> Changes in v3: None >>>>>>>>>> Changes in v2: >>>>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>>>> >>>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>>>> >>>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>> index f559830..15df6ef 100644 >>>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>>>> #include <malloc.h> >>>>>>>>>> #include <usb.h> >>>>>>>>>> #include <watchdog.h> >>>>>>>>>> -#include <asm/gpio.h> >>>>>>>>>> #include <linux/errno.h> >>>>>>>>>> #include <linux/compat.h> >>>>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>>>> +#include <power/regulator.h> >>>>>>>>>> #include "xhci.h" >>>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>>>> struct rockchip_xhci_platdata { >>>>>>>>>> fdt_addr_t hcd_base; >>>>>>>>>> fdt_addr_t phy_base; >>>>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>>>> + struct udevice *vbus_supply; >>>>>>>>>> }; >>>>>>>>>> /* >>>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>>> udevice *dev) >>>>>>>>>> */ >>>>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>>>> return -ENXIO; >>>>>>>>>> } >>>>>>>>>> @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>>> udevice *dev) >>>>>>>>>> } >>>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>>>> return -ENXIO; >>>>>>>>>> } >>>>>>>>>> - /* Vbus gpio */ >>>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends on it (or >>>>>>>>> should) already anyway. >>>>>>>> Yes, I will remove it in v6. >>>>>>>>>> + /* Vbus regulator */ >>>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>>>> + &plat->vbus_supply); >>>>>>>>> So I was curious, does this expand to empty function or is this not >>>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>>>> Simon, can you comment on this ? >>>>>> It looks OK to me. >>>>> Shouldn't you have empty stub functions instead to avoid proliferation >>>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >>>> You could, but this is just a temporary state while apparently some >>>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >>>> those bad boards are, actually. >>> Temporary state ? What's the final state then ? >> Well I wasn't aware that any rockchip boards didn't use regulators. >> Presumably this #ifdef is because of a board that does not. > This is IMO unrelated to rockchip. > >>> Anyway, what if you just disable regulator support (because you don't >>> need it for whatever reason), but want to keep USB ? Wouldn't it make >>> more sense to have empty stub functions instead of swamping the drivers >>> with ifdefs ? >> USB would not work without VBUS, which is handled with regulators > The VBUS can be directly tied to 5V rail without power switching. > >> , so >> there is something I don't understand here. Anyway I don't see any >> point in adding stub functions here. > Not here, into the regulator framework, so you don't have to pollute > drivers which use the regulators with ifdefs if the regulator framework > is not enabled in the config. > >> Meng can you please explain why the #ifdef is needed? Because the function "device_get_supply_regulator" is undefined if undefined CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the regulator framework can be optimized and make it compiled successful whether define CONFIG_DM_REGULATOR. So is this #ifdef still needed here? >> >> Regards, >> Simon >> >
On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote: > > > On 2017/6/18 13:11, Marek Vasut wrote: >> On 06/18/2017 12:10 AM, Simon Glass wrote: >>> Hi Marek, >>> >>> On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de> wrote: >>>> On 06/17/2017 07:28 PM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >>>>>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>>>>> Hi Marek, >>>>>>> >>>>>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>>>>> >>>>>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>>>>> vbus when usb stop. >>>>>>>>>>> >>>>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>>>>>> --- >>>>>>>>>>> >>>>>>>>>>> Changes in v5: >>>>>>>>>>> - Propagate return value and print error message when failed >>>>>>>>>>> >>>>>>>>>>> Changes in v4: >>>>>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>>>>> >>>>>>>>>>> Changes in v3: None >>>>>>>>>>> Changes in v2: >>>>>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>>>>> >>>>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>>>>> >>>>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>> index f559830..15df6ef 100644 >>>>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>>>>> #include <malloc.h> >>>>>>>>>>> #include <usb.h> >>>>>>>>>>> #include <watchdog.h> >>>>>>>>>>> -#include <asm/gpio.h> >>>>>>>>>>> #include <linux/errno.h> >>>>>>>>>>> #include <linux/compat.h> >>>>>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>>>>> +#include <power/regulator.h> >>>>>>>>>>> #include "xhci.h" >>>>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>>>>> struct rockchip_xhci_platdata { >>>>>>>>>>> fdt_addr_t hcd_base; >>>>>>>>>>> fdt_addr_t phy_base; >>>>>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>>>>> + struct udevice *vbus_supply; >>>>>>>>>>> }; >>>>>>>>>>> /* >>>>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>> udevice *dev) >>>>>>>>>>> */ >>>>>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>>>>> return -ENXIO; >>>>>>>>>>> } >>>>>>>>>>> @@ -62,19 +62,39 @@ static int >>>>>>>>>>> xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>> udevice *dev) >>>>>>>>>>> } >>>>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>>>>> return -ENXIO; >>>>>>>>>>> } >>>>>>>>>>> - /* Vbus gpio */ >>>>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>>>>>>>> on it (or >>>>>>>>>> should) already anyway. >>>>>>>>> Yes, I will remove it in v6. >>>>>>>>>>> + /* Vbus regulator */ >>>>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>>>>> + &plat->vbus_supply); >>>>>>>>>> So I was curious, does this expand to empty function or is >>>>>>>>>> this not >>>>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>>>>> Simon, can you comment on this ? >>>>>>> It looks OK to me. >>>>>> Shouldn't you have empty stub functions instead to avoid >>>>>> proliferation >>>>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >>>>> You could, but this is just a temporary state while apparently some >>>>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >>>>> those bad boards are, actually. >>>> Temporary state ? What's the final state then ? >>> Well I wasn't aware that any rockchip boards didn't use regulators. >>> Presumably this #ifdef is because of a board that does not. >> This is IMO unrelated to rockchip. >> >>>> Anyway, what if you just disable regulator support (because you don't >>>> need it for whatever reason), but want to keep USB ? Wouldn't it make >>>> more sense to have empty stub functions instead of swamping the drivers >>>> with ifdefs ? >>> USB would not work without VBUS, which is handled with regulators >> The VBUS can be directly tied to 5V rail without power switching. >> >>> , so >>> there is something I don't understand here. Anyway I don't see any >>> point in adding stub functions here. >> Not here, into the regulator framework, so you don't have to pollute >> drivers which use the regulators with ifdefs if the regulator framework >> is not enabled in the config. >> >>> Meng can you please explain why the #ifdef is needed? > > Because the function "device_get_supply_regulator" is undefined if > undefined > CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the > regulator > framework can be optimized and make it compiled successful whether define > CONFIG_DM_REGULATOR. > So is this #ifdef still needed here? Thus my discussion with Simon. Linux does keep the ifdefs in framework header files , so I think we should do the same.
On 19 June 2017 at 04:15, Marek Vasut <marex@denx.de> wrote: > On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote: >> >> >> On 2017/6/18 13:11, Marek Vasut wrote: >>> On 06/18/2017 12:10 AM, Simon Glass wrote: >>>> Hi Marek, >>>> >>>> On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de> wrote: >>>>> On 06/17/2017 07:28 PM, Simon Glass wrote: >>>>>> Hi Marek, >>>>>> >>>>>> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de> wrote: >>>>>>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>>>>>> Hi Marek, >>>>>>>> >>>>>>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de> wrote: >>>>>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>>>>>> >>>>>>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>>>>>> vbus when usb stop. >>>>>>>>>>>> >>>>>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> >>>>>>>>>>>> --- >>>>>>>>>>>> >>>>>>>>>>>> Changes in v5: >>>>>>>>>>>> - Propagate return value and print error message when failed >>>>>>>>>>>> >>>>>>>>>>>> Changes in v4: >>>>>>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>>>>>> >>>>>>>>>>>> Changes in v3: None >>>>>>>>>>>> Changes in v2: >>>>>>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>>>>>> >>>>>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>>>>>> >>>>>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>> index f559830..15df6ef 100644 >>>>>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>>>>>> #include <malloc.h> >>>>>>>>>>>> #include <usb.h> >>>>>>>>>>>> #include <watchdog.h> >>>>>>>>>>>> -#include <asm/gpio.h> >>>>>>>>>>>> #include <linux/errno.h> >>>>>>>>>>>> #include <linux/compat.h> >>>>>>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>>>>>> +#include <power/regulator.h> >>>>>>>>>>>> #include "xhci.h" >>>>>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>>>>>> struct rockchip_xhci_platdata { >>>>>>>>>>>> fdt_addr_t hcd_base; >>>>>>>>>>>> fdt_addr_t phy_base; >>>>>>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>>>>>> + struct udevice *vbus_supply; >>>>>>>>>>>> }; >>>>>>>>>>>> /* >>>>>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>>> udevice *dev) >>>>>>>>>>>> */ >>>>>>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>>>>>> return -ENXIO; >>>>>>>>>>>> } >>>>>>>>>>>> @@ -62,19 +62,39 @@ static int >>>>>>>>>>>> xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>>> udevice *dev) >>>>>>>>>>>> } >>>>>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>>>>>> return -ENXIO; >>>>>>>>>>>> } >>>>>>>>>>>> - /* Vbus gpio */ >>>>>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>>>>>>>>> on it (or >>>>>>>>>>> should) already anyway. >>>>>>>>>> Yes, I will remove it in v6. >>>>>>>>>>>> + /* Vbus regulator */ >>>>>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>>>>>> + &plat->vbus_supply); >>>>>>>>>>> So I was curious, does this expand to empty function or is >>>>>>>>>>> this not >>>>>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>>>>>> Simon, can you comment on this ? >>>>>>>> It looks OK to me. >>>>>>> Shouldn't you have empty stub functions instead to avoid >>>>>>> proliferation >>>>>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >>>>>> You could, but this is just a temporary state while apparently some >>>>>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >>>>>> those bad boards are, actually. >>>>> Temporary state ? What's the final state then ? >>>> Well I wasn't aware that any rockchip boards didn't use regulators. >>>> Presumably this #ifdef is because of a board that does not. >>> This is IMO unrelated to rockchip. >>> >>>>> Anyway, what if you just disable regulator support (because you don't >>>>> need it for whatever reason), but want to keep USB ? Wouldn't it make >>>>> more sense to have empty stub functions instead of swamping the drivers >>>>> with ifdefs ? >>>> USB would not work without VBUS, which is handled with regulators >>> The VBUS can be directly tied to 5V rail without power switching. >>> >>>> , so >>>> there is something I don't understand here. Anyway I don't see any >>>> point in adding stub functions here. >>> Not here, into the regulator framework, so you don't have to pollute >>> drivers which use the regulators with ifdefs if the regulator framework >>> is not enabled in the config. >>> >>>> Meng can you please explain why the #ifdef is needed? >> >> Because the function "device_get_supply_regulator" is undefined if >> undefined >> CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the >> regulator >> framework can be optimized and make it compiled successful whether define >> CONFIG_DM_REGULATOR. >> So is this #ifdef still needed here? > > Thus my discussion with Simon. Linux does keep the ifdefs in framework > header files , so I think we should do the same. OK, but arguably that is a separate issue from this patch. Also I hope we can always enable DM_REGULATOR for rockchip and drop the #ifdefs. Regards, Simon
On 06/20/2017 05:22 AM, Simon Glass wrote: > On 19 June 2017 at 04:15, Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: >> On 06/19/2017 11:50 AM, rock-chips(daniel.meng) wrote: >>> >>> >>> On 2017/6/18 13:11, Marek Vasut wrote: >>>> On 06/18/2017 12:10 AM, Simon Glass wrote: >>>>> Hi Marek, >>>>> >>>>> On 17 June 2017 at 13:33, Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: >>>>>> On 06/17/2017 07:28 PM, Simon Glass wrote: >>>>>>> Hi Marek, >>>>>>> >>>>>>> On 17 June 2017 at 00:22, Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: >>>>>>>> On 06/17/2017 05:41 AM, Simon Glass wrote: >>>>>>>>> Hi Marek, >>>>>>>>> >>>>>>>>> On 15 June 2017 at 10:53, Marek Vasut <marex@denx.de > <mailto:marex@denx.de>> wrote: >>>>>>>>>> On 06/15/2017 05:51 AM, rock-chips(daniel.meng) wrote: >>>>>>>>>>> >>>>>>>>>>> On 2017/6/13 17:11, Marek Vasut wrote: >>>>>>>>>>>> On 06/12/2017 11:19 AM, Meng Dongyang wrote: >>>>>>>>>>>>> Use fixed regulator to control the voltage of vbus and turn off >>>>>>>>>>>>> vbus when usb stop. >>>>>>>>>>>>> >>>>>>>>>>>>> Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com > <mailto:daniel.meng@rock-chips.com>> >>>>>>>>>>>>> --- >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v5: >>>>>>>>>>>>> - Propagate return value and print error message when failed >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v4: >>>>>>>>>>>>> - Splited from patch [Uboot,v3,04/10] >>>>>>>>>>>>> - Define set vbus as empty function if the macros aren't set >>>>>>>>>>>>> >>>>>>>>>>>>> Changes in v3: None >>>>>>>>>>>>> Changes in v2: >>>>>>>>>>>>> - Use fixed regulator to control vbus instead of gpio >>>>>>>>>>>>> >>>>>>>>>>>>> drivers/usb/host/xhci-rockchip.c | 55 >>>>>>>>>>>>> ++++++++++++++++++++++++++++++---------- >>>>>>>>>>>>> 1 file changed, 42 insertions(+), 13 deletions(-) >>>>>>>>>>>>> >>>>>>>>>>>>> diff --git a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>>> b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>>> index f559830..15df6ef 100644 >>>>>>>>>>>>> --- a/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>>> +++ b/drivers/usb/host/xhci-rockchip.c >>>>>>>>>>>>> @@ -11,10 +11,10 @@ >>>>>>>>>>>>> #include <malloc.h> >>>>>>>>>>>>> #include <usb.h> >>>>>>>>>>>>> #include <watchdog.h> >>>>>>>>>>>>> -#include <asm/gpio.h> >>>>>>>>>>>>> #include <linux/errno.h> >>>>>>>>>>>>> #include <linux/compat.h> >>>>>>>>>>>>> #include <linux/usb/dwc3.h> >>>>>>>>>>>>> +#include <power/regulator.h> >>>>>>>>>>>>> #include "xhci.h" >>>>>>>>>>>>> @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; >>>>>>>>>>>>> struct rockchip_xhci_platdata { >>>>>>>>>>>>> fdt_addr_t hcd_base; >>>>>>>>>>>>> fdt_addr_t phy_base; >>>>>>>>>>>>> - struct gpio_desc vbus_gpio; >>>>>>>>>>>>> + struct udevice *vbus_supply; >>>>>>>>>>>>> }; >>>>>>>>>>>>> /* >>>>>>>>>>>>> @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>>>> udevice *dev) >>>>>>>>>>>>> */ >>>>>>>>>>>>> plat->hcd_base = dev_get_addr(dev); >>>>>>>>>>>>> if (plat->hcd_base == FDT_ADDR_T_NONE) { >>>>>>>>>>>>> - debug("Can't get the XHCI register base address\n"); >>>>>>>>>>>>> + error("Can't get the XHCI register base address\n"); >>>>>>>>>>>>> return -ENXIO; >>>>>>>>>>>>> } >>>>>>>>>>>>> @@ -62,19 +62,39 @@ static int >>>>>>>>>>>>> xhci_usb_ofdata_to_platdata(struct >>>>>>>>>>>>> udevice *dev) >>>>>>>>>>>>> } >>>>>>>>>>>>> if (plat->phy_base == FDT_ADDR_T_NONE) { >>>>>>>>>>>>> - debug("Can't get the usbphy register address\n"); >>>>>>>>>>>>> + error("Can't get the usbphy register address\n"); >>>>>>>>>>>>> return -ENXIO; >>>>>>>>>>>>> } >>>>>>>>>>>>> - /* Vbus gpio */ >>>>>>>>>>>>> - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, >>>>>>>>>>>>> - &plat->vbus_gpio, GPIOD_IS_OUT); >>>>>>>>>>>>> +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) >>>>>>>>>>>> I don't think you need the CONFIG_DM_USB , the driver depends >>>>>>>>>>>> on it (or >>>>>>>>>>>> should) already anyway. >>>>>>>>>>> Yes, I will remove it in v6. >>>>>>>>>>>>> + /* Vbus regulator */ >>>>>>>>>>>>> + ret = device_get_supply_regulator(dev, "vbus-supply", >>>>>>>>>>>>> + &plat->vbus_supply); >>>>>>>>>>>> So I was curious, does this expand to empty function or is >>>>>>>>>>>> this not >>>>>>>>>>>> defined if CONFIG_DM_REGULATOR is not defined ? >>>>>>>>>>> This is not defined if CONFIG_DM_REGULATOR is not defined. >>>>>>>>>> Simon, can you comment on this ? >>>>>>>>> It looks OK to me. >>>>>>>> Shouldn't you have empty stub functions instead to avoid >>>>>>>> proliferation >>>>>>>> of adhoc #ifdef CONFIG_FOO throughout the codebase ? >>>>>>> You could, but this is just a temporary state while apparently some >>>>>>> rockchip boards don't use DM_USB and DM_REGULATOR. I'm not sure what >>>>>>> those bad boards are, actually. >>>>>> Temporary state ? What's the final state then ? >>>>> Well I wasn't aware that any rockchip boards didn't use regulators. >>>>> Presumably this #ifdef is because of a board that does not. >>>> This is IMO unrelated to rockchip. >>>> >>>>>> Anyway, what if you just disable regulator support (because you don't >>>>>> need it for whatever reason), but want to keep USB ? Wouldn't it make >>>>>> more sense to have empty stub functions instead of swamping the > drivers >>>>>> with ifdefs ? >>>>> USB would not work without VBUS, which is handled with regulators >>>> The VBUS can be directly tied to 5V rail without power switching. >>>> >>>>> , so >>>>> there is something I don't understand here. Anyway I don't see any >>>>> point in adding stub functions here. >>>> Not here, into the regulator framework, so you don't have to pollute >>>> drivers which use the regulators with ifdefs if the regulator framework >>>> is not enabled in the config. >>>> >>>>> Meng can you please explain why the #ifdef is needed? >>> >>> Because the function "device_get_supply_regulator" is undefined if >>> undefined >>> CONFIG_DM_REGULATOR and this will result in a compile error. Maybe the >>> regulator >>> framework can be optimized and make it compiled successful whether define >>> CONFIG_DM_REGULATOR. >>> So is this #ifdef still needed here? >> >> Thus my discussion with Simon. Linux does keep the ifdefs in framework >> header files , so I think we should do the same. > > OK, but arguably that is a separate issue from this patch. It is, and I believe it should be discussed and possibly fixed. > Also I hope we can always enable DM_REGULATOR for rockchip and drop the > #ifdefs. If the driver explicitly depends on DM_REGULATOR in Kconfig , then yes.
diff --git a/drivers/usb/host/xhci-rockchip.c b/drivers/usb/host/xhci-rockchip.c index f559830..15df6ef 100644 --- a/drivers/usb/host/xhci-rockchip.c +++ b/drivers/usb/host/xhci-rockchip.c @@ -11,10 +11,10 @@ #include <malloc.h> #include <usb.h> #include <watchdog.h> -#include <asm/gpio.h> #include <linux/errno.h> #include <linux/compat.h> #include <linux/usb/dwc3.h> +#include <power/regulator.h> #include "xhci.h" @@ -23,7 +23,7 @@ DECLARE_GLOBAL_DATA_PTR; struct rockchip_xhci_platdata { fdt_addr_t hcd_base; fdt_addr_t phy_base; - struct gpio_desc vbus_gpio; + struct udevice *vbus_supply; }; /* @@ -48,7 +48,7 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) */ plat->hcd_base = dev_get_addr(dev); if (plat->hcd_base == FDT_ADDR_T_NONE) { - debug("Can't get the XHCI register base address\n"); + error("Can't get the XHCI register base address\n"); return -ENXIO; } @@ -62,19 +62,39 @@ static int xhci_usb_ofdata_to_platdata(struct udevice *dev) } if (plat->phy_base == FDT_ADDR_T_NONE) { - debug("Can't get the usbphy register address\n"); + error("Can't get the usbphy register address\n"); return -ENXIO; } - /* Vbus gpio */ - ret = gpio_request_by_name(dev, "rockchip,vbus-gpio", 0, - &plat->vbus_gpio, GPIOD_IS_OUT); +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) + /* Vbus regulator */ + ret = device_get_supply_regulator(dev, "vbus-supply", + &plat->vbus_supply); if (ret) - debug("rockchip,vbus-gpio node missing!"); + debug("Can't get VBus regulator!\n"); +#endif return 0; } +#if defined(CONFIG_DM_USB) && defined(CONFIG_DM_REGULATOR) +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{ + int ret; + + ret = regulator_set_enable(vbus_supply, value); + if (ret) + error("XHCI: Failed to set vbus supply\n"); + + return ret; +} +#else +static int rockchip_xhci_set_vbus(struct udevice *vbus_supply, bool value) +{ + return 0; +} +#endif + /* * rockchip_dwc3_phy_setup() - Configure USB PHY Interface of DWC3 Core * @dwc: Pointer to our controller context structure @@ -124,7 +144,7 @@ static int rockchip_xhci_core_init(struct rockchip_xhci *rkxhci, ret = dwc3_core_init(rkxhci->dwc3_reg); if (ret) { - debug("failed to initialize core\n"); + error("failed to initialize core\n"); return ret; } @@ -153,13 +173,15 @@ static int xhci_usb_probe(struct udevice *dev) hcor = (struct xhci_hcor *)((uint64_t)ctx->hcd + HC_LENGTH(xhci_readl(&ctx->hcd->cr_capbase))); - /* setup the Vbus gpio here */ - if (dm_gpio_is_valid(&plat->vbus_gpio)) - dm_gpio_set_value(&plat->vbus_gpio, 1); + if (plat->vbus_supply) { + ret = rockchip_xhci_set_vbus(plat->vbus_supply, true); + if (ret) + return ret; + } ret = rockchip_xhci_core_init(ctx, dev); if (ret) { - debug("XHCI: failed to initialize controller\n"); + error("XHCI: failed to initialize controller\n"); return ret; } @@ -168,6 +190,7 @@ static int xhci_usb_probe(struct udevice *dev) static int xhci_usb_remove(struct udevice *dev) { + struct rockchip_xhci_platdata *plat = dev_get_platdata(dev); struct rockchip_xhci *ctx = dev_get_priv(dev); int ret; @@ -178,6 +201,12 @@ static int xhci_usb_remove(struct udevice *dev) if (ret) return ret; + if (plat->vbus_supply) { + ret = rockchip_xhci_set_vbus(plat->vbus_supply, false); + if (ret) + return ret; + } + return 0; }
Use fixed regulator to control the voltage of vbus and turn off vbus when usb stop. Signed-off-by: Meng Dongyang <daniel.meng@rock-chips.com> --- Changes in v5: - Propagate return value and print error message when failed Changes in v4: - Splited from patch [Uboot,v3,04/10] - Define set vbus as empty function if the macros aren't set Changes in v3: None Changes in v2: - Use fixed regulator to control vbus instead of gpio drivers/usb/host/xhci-rockchip.c | 55 ++++++++++++++++++++++++++++++---------- 1 file changed, 42 insertions(+), 13 deletions(-)