| Message ID | 1534398342-5200-1-git-send-email-ley.foon.tan@intel.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | [U-Boot,v2] usb: dwc2: Add reset ctrl to driver | expand |
On 08/16/2018 07:45 AM, Ley Foon Tan wrote: > Add code to reset all reset signals as in usb DT node. A reset property > is an optional feature, so only print out a warning and do not fail if a > reset property is not present. > > If a reset property is discovered, then use it to deassert, thus > bringing the IP out of reset. > > Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> > > --- > v2: > - Assert reset when .remove. > --- > drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index cbe065b..bee3b33 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -15,6 +15,7 @@ > #include <wait_bit.h> > #include <asm/io.h> > #include <power/regulator.h> > +#include <reset.h> > > #include "dwc2.h" > > @@ -49,6 +50,8 @@ struct dwc2_priv { > */ > bool hnp_srp_disable; > bool oc_disable; > + > + struct reset_ctl_bulk resets; > }; > > #ifndef CONFIG_DM_USB > @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, > } > } > > +static int dwc2_reset(struct udevice *dev) > +{ > + int ret; > + struct dwc2_priv *priv = dev_get_priv(dev); > + > + ret = reset_get_bulk(dev, &priv->resets); > + if (ret) { > + dev_warn(dev, "Can't get reset: %d\n", ret); So this now generates a ton of warnings on Gen5 , right ? Did you test it on any Gen5 board recently ? > + return ret; > + } > + > + ret = reset_deassert_bulk(&priv->resets); > + if (ret) { > + reset_release_bulk(&priv->resets); > + dev_err(dev, "Failed to reset: %d\n", ret); > + return ret; > + } > + > + return 0; > +} > + > static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) > { > struct dwc2_core_regs *regs = priv->regs; > uint32_t snpsid; > int i, j; > + int ret; > + > + ret = dwc2_reset(dev); > + if (ret) > + return ret; > > snpsid = readl(®s->gsnpsid); > dev_info(dev, "Core Release: %x.%03x\n", > @@ -1303,6 +1332,8 @@ static int dwc2_usb_remove(struct udevice *dev) > > dwc2_uninit_common(priv->regs); > > + reset_release_bulk(&priv->resets); Dont you need to assert the resets too ? > return 0; > } > >
On Thu, Aug 16, 2018 at 4:26 PM, Marek Vasut <marex@denx.de> wrote: > On 08/16/2018 07:45 AM, Ley Foon Tan wrote: >> Add code to reset all reset signals as in usb DT node. A reset property >> is an optional feature, so only print out a warning and do not fail if a >> reset property is not present. >> >> If a reset property is discovered, then use it to deassert, thus >> bringing the IP out of reset. >> >> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> >> >> --- >> v2: >> - Assert reset when .remove. >> --- >> drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >> index cbe065b..bee3b33 100644 >> --- a/drivers/usb/host/dwc2.c >> +++ b/drivers/usb/host/dwc2.c >> @@ -15,6 +15,7 @@ >> #include <wait_bit.h> >> #include <asm/io.h> >> #include <power/regulator.h> >> +#include <reset.h> >> >> #include "dwc2.h" >> >> @@ -49,6 +50,8 @@ struct dwc2_priv { >> */ >> bool hnp_srp_disable; >> bool oc_disable; >> + >> + struct reset_ctl_bulk resets; >> }; >> >> #ifndef CONFIG_DM_USB >> @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, >> } >> } >> >> +static int dwc2_reset(struct udevice *dev) >> +{ >> + int ret; >> + struct dwc2_priv *priv = dev_get_priv(dev); >> + >> + ret = reset_get_bulk(dev, &priv->resets); >> + if (ret) { >> + dev_warn(dev, "Can't get reset: %d\n", ret); > > So this now generates a ton of warnings on Gen5 , right ? > Did you test it on any Gen5 board recently ? Will change to "return 0" if can't find reset in dts, so it will not fail the probe. It will not print warning by default. I don't have Gen5 board, but I tested with Stratix 10 by removing reset from USB DT node. We will not see the warning. > >> + return ret; >> + } >> + >> + ret = reset_deassert_bulk(&priv->resets); >> + if (ret) { >> + reset_release_bulk(&priv->resets); >> + dev_err(dev, "Failed to reset: %d\n", ret); >> + return ret; >> + } >> + >> + return 0; >> +} >> + >> static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) >> { >> struct dwc2_core_regs *regs = priv->regs; >> uint32_t snpsid; >> int i, j; >> + int ret; >> + >> + ret = dwc2_reset(dev); >> + if (ret) >> + return ret; >> >> snpsid = readl(®s->gsnpsid); >> dev_info(dev, "Core Release: %x.%03x\n", >> @@ -1303,6 +1332,8 @@ static int dwc2_usb_remove(struct udevice *dev) >> >> dwc2_uninit_common(priv->regs); >> >> + reset_release_bulk(&priv->resets); > > Dont you need to assert the resets too ? reset_release_bulk will assert the reset if it is requested before. > >> return 0; >> } >> >> > Regards Ley Foon
On 08/16/2018 11:18 AM, Ley Foon Tan wrote: > On Thu, Aug 16, 2018 at 4:26 PM, Marek Vasut <marex@denx.de> wrote: >> On 08/16/2018 07:45 AM, Ley Foon Tan wrote: >>> Add code to reset all reset signals as in usb DT node. A reset property >>> is an optional feature, so only print out a warning and do not fail if a >>> reset property is not present. >>> >>> If a reset property is discovered, then use it to deassert, thus >>> bringing the IP out of reset. >>> >>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> >>> >>> --- >>> v2: >>> - Assert reset when .remove. >>> --- >>> drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ >>> 1 file changed, 31 insertions(+) >>> >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >>> index cbe065b..bee3b33 100644 >>> --- a/drivers/usb/host/dwc2.c >>> +++ b/drivers/usb/host/dwc2.c >>> @@ -15,6 +15,7 @@ >>> #include <wait_bit.h> >>> #include <asm/io.h> >>> #include <power/regulator.h> >>> +#include <reset.h> >>> >>> #include "dwc2.h" >>> >>> @@ -49,6 +50,8 @@ struct dwc2_priv { >>> */ >>> bool hnp_srp_disable; >>> bool oc_disable; >>> + >>> + struct reset_ctl_bulk resets; >>> }; >>> >>> #ifndef CONFIG_DM_USB >>> @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, >>> } >>> } >>> >>> +static int dwc2_reset(struct udevice *dev) >>> +{ >>> + int ret; >>> + struct dwc2_priv *priv = dev_get_priv(dev); >>> + >>> + ret = reset_get_bulk(dev, &priv->resets); >>> + if (ret) { >>> + dev_warn(dev, "Can't get reset: %d\n", ret); >> >> So this now generates a ton of warnings on Gen5 , right ? >> Did you test it on any Gen5 board recently ? > > Will change to "return 0" if can't find reset in dts, so it will not > fail the probe. But that's a valid error. Maybe some ifdef CONFIG_RESET would be more appropriate ? Look at what the other drivers do. > It will not print warning by default. I don't have Gen5 board, but I > tested with Stratix 10 by removing reset from USB DT node. We will not > see the warning. Paging Simon, he's been doing a lot of good work on Gen5 recently. You should CC him on changes too :) >>> + return ret; >>> + } >>> + >>> + ret = reset_deassert_bulk(&priv->resets); >>> + if (ret) { >>> + reset_release_bulk(&priv->resets); >>> + dev_err(dev, "Failed to reset: %d\n", ret); >>> + return ret; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) >>> { >>> struct dwc2_core_regs *regs = priv->regs; >>> uint32_t snpsid; >>> int i, j; >>> + int ret; >>> + >>> + ret = dwc2_reset(dev); >>> + if (ret) >>> + return ret; >>> >>> snpsid = readl(®s->gsnpsid); >>> dev_info(dev, "Core Release: %x.%03x\n", >>> @@ -1303,6 +1332,8 @@ static int dwc2_usb_remove(struct udevice *dev) >>> >>> dwc2_uninit_common(priv->regs); >>> >>> + reset_release_bulk(&priv->resets); >> >> Dont you need to assert the resets too ? > reset_release_bulk will assert the reset if it is requested before. >> >>> return 0; >>> } >>> >>> >> > Regards > Ley Foon >
On Thu, Aug 16, 2018 at 5:21 PM, Marek Vasut <marex@denx.de> wrote: > On 08/16/2018 11:18 AM, Ley Foon Tan wrote: >> On Thu, Aug 16, 2018 at 4:26 PM, Marek Vasut <marex@denx.de> wrote: >>> On 08/16/2018 07:45 AM, Ley Foon Tan wrote: >>>> Add code to reset all reset signals as in usb DT node. A reset property >>>> is an optional feature, so only print out a warning and do not fail if a >>>> reset property is not present. >>>> >>>> If a reset property is discovered, then use it to deassert, thus >>>> bringing the IP out of reset. >>>> >>>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> >>>> >>>> --- >>>> v2: >>>> - Assert reset when .remove. >>>> --- >>>> drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ >>>> 1 file changed, 31 insertions(+) >>>> >>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >>>> index cbe065b..bee3b33 100644 >>>> --- a/drivers/usb/host/dwc2.c >>>> +++ b/drivers/usb/host/dwc2.c >>>> @@ -15,6 +15,7 @@ >>>> #include <wait_bit.h> >>>> #include <asm/io.h> >>>> #include <power/regulator.h> >>>> +#include <reset.h> >>>> >>>> #include "dwc2.h" >>>> >>>> @@ -49,6 +50,8 @@ struct dwc2_priv { >>>> */ >>>> bool hnp_srp_disable; >>>> bool oc_disable; >>>> + >>>> + struct reset_ctl_bulk resets; >>>> }; >>>> >>>> #ifndef CONFIG_DM_USB >>>> @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, >>>> } >>>> } >>>> >>>> +static int dwc2_reset(struct udevice *dev) >>>> +{ >>>> + int ret; >>>> + struct dwc2_priv *priv = dev_get_priv(dev); >>>> + >>>> + ret = reset_get_bulk(dev, &priv->resets); >>>> + if (ret) { >>>> + dev_warn(dev, "Can't get reset: %d\n", ret); >>> >>> So this now generates a ton of warnings on Gen5 , right ? >>> Did you test it on any Gen5 board recently ? >> >> Will change to "return 0" if can't find reset in dts, so it will not >> fail the probe. > > But that's a valid error. Maybe some ifdef CONFIG_RESET would be more > appropriate ? Look at what the other drivers do. Platforms that doesn't support reset framework shouldn't treat it as error. Other drivers don't have CONFIG_RESET too. > >> It will not print warning by default. I don't have Gen5 board, but I >> tested with Stratix 10 by removing reset from USB DT node. We will not >> see the warning. > > Paging Simon, he's been doing a lot of good work on Gen5 recently. You > should CC him on changes too :) > >>>> + return ret; >>>> + } >>>> + >>>> + ret = reset_deassert_bulk(&priv->resets); >>>> + if (ret) { >>>> + reset_release_bulk(&priv->resets); >>>> + dev_err(dev, "Failed to reset: %d\n", ret); >>>> + return ret; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) >>>> { >>>> struct dwc2_core_regs *regs = priv->regs; >>>> uint32_t snpsid; >>>> int i, j; >>>> + int ret; >>>> + >>>> + ret = dwc2_reset(dev); >>>> + if (ret) >>>> + return ret; >>>> >>>> snpsid = readl(®s->gsnpsid); >>>> dev_info(dev, "Core Release: %x.%03x\n", >>>> @@ -1303,6 +1332,8 @@ static int dwc2_usb_remove(struct udevice *dev) >>>> >>>> dwc2_uninit_common(priv->regs); >>>> >>>> + reset_release_bulk(&priv->resets); >>> >>> Dont you need to assert the resets too ? >> reset_release_bulk will assert the reset if it is requested before. >>> >>>> return 0; >>>> } >>>> >>>> >>> >> Regards >> Ley Foon Regards Ley Foon
On 08/16/2018 11:48 AM, Ley Foon Tan wrote: > On Thu, Aug 16, 2018 at 5:21 PM, Marek Vasut <marex@denx.de> wrote: >> On 08/16/2018 11:18 AM, Ley Foon Tan wrote: >>> On Thu, Aug 16, 2018 at 4:26 PM, Marek Vasut <marex@denx.de> wrote: >>>> On 08/16/2018 07:45 AM, Ley Foon Tan wrote: >>>>> Add code to reset all reset signals as in usb DT node. A reset property >>>>> is an optional feature, so only print out a warning and do not fail if a >>>>> reset property is not present. >>>>> >>>>> If a reset property is discovered, then use it to deassert, thus >>>>> bringing the IP out of reset. >>>>> >>>>> Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> >>>>> >>>>> --- >>>>> v2: >>>>> - Assert reset when .remove. >>>>> --- >>>>> drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ >>>>> 1 file changed, 31 insertions(+) >>>>> >>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c >>>>> index cbe065b..bee3b33 100644 >>>>> --- a/drivers/usb/host/dwc2.c >>>>> +++ b/drivers/usb/host/dwc2.c >>>>> @@ -15,6 +15,7 @@ >>>>> #include <wait_bit.h> >>>>> #include <asm/io.h> >>>>> #include <power/regulator.h> >>>>> +#include <reset.h> >>>>> >>>>> #include "dwc2.h" >>>>> >>>>> @@ -49,6 +50,8 @@ struct dwc2_priv { >>>>> */ >>>>> bool hnp_srp_disable; >>>>> bool oc_disable; >>>>> + >>>>> + struct reset_ctl_bulk resets; >>>>> }; >>>>> >>>>> #ifndef CONFIG_DM_USB >>>>> @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, >>>>> } >>>>> } >>>>> >>>>> +static int dwc2_reset(struct udevice *dev) >>>>> +{ >>>>> + int ret; >>>>> + struct dwc2_priv *priv = dev_get_priv(dev); >>>>> + >>>>> + ret = reset_get_bulk(dev, &priv->resets); >>>>> + if (ret) { >>>>> + dev_warn(dev, "Can't get reset: %d\n", ret); >>>> >>>> So this now generates a ton of warnings on Gen5 , right ? >>>> Did you test it on any Gen5 board recently ? >>> >>> Will change to "return 0" if can't find reset in dts, so it will not >>> fail the probe. >> >> But that's a valid error. Maybe some ifdef CONFIG_RESET would be more >> appropriate ? Look at what the other drivers do. > Platforms that doesn't support reset framework shouldn't treat it as error. > Other drivers don't have CONFIG_RESET too. What about platforms that do have reset and fail requesting it ? The error would be ignored on those ?
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index cbe065b..bee3b33 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -15,6 +15,7 @@ #include <wait_bit.h> #include <asm/io.h> #include <power/regulator.h> +#include <reset.h> #include "dwc2.h" @@ -49,6 +50,8 @@ struct dwc2_priv { */ bool hnp_srp_disable; bool oc_disable; + + struct reset_ctl_bulk resets; }; #ifndef CONFIG_DM_USB @@ -1124,11 +1127,37 @@ int _submit_int_msg(struct dwc2_priv *priv, struct usb_device *dev, } } +static int dwc2_reset(struct udevice *dev) +{ + int ret; + struct dwc2_priv *priv = dev_get_priv(dev); + + ret = reset_get_bulk(dev, &priv->resets); + if (ret) { + dev_warn(dev, "Can't get reset: %d\n", ret); + return ret; + } + + ret = reset_deassert_bulk(&priv->resets); + if (ret) { + reset_release_bulk(&priv->resets); + dev_err(dev, "Failed to reset: %d\n", ret); + return ret; + } + + return 0; +} + static int dwc2_init_common(struct udevice *dev, struct dwc2_priv *priv) { struct dwc2_core_regs *regs = priv->regs; uint32_t snpsid; int i, j; + int ret; + + ret = dwc2_reset(dev); + if (ret) + return ret; snpsid = readl(®s->gsnpsid); dev_info(dev, "Core Release: %x.%03x\n", @@ -1303,6 +1332,8 @@ static int dwc2_usb_remove(struct udevice *dev) dwc2_uninit_common(priv->regs); + reset_release_bulk(&priv->resets); + return 0; }
Add code to reset all reset signals as in usb DT node. A reset property is an optional feature, so only print out a warning and do not fail if a reset property is not present. If a reset property is discovered, then use it to deassert, thus bringing the IP out of reset. Signed-off-by: Ley Foon Tan <ley.foon.tan@intel.com> --- v2: - Assert reset when .remove. --- drivers/usb/host/dwc2.c | 31 +++++++++++++++++++++++++++++++ 1 file changed, 31 insertions(+)