Re: [PATCH 13/18] usb: dwc2: Move reset into dwc2_get_hwparams()
John, On Wed, Dec 2, 2015 at 11:14 AM, John Younwrote: > The reset is required to get reset values of the hardware parameters but > the force mode is not. Move the base reset into dwc2_get_hwparams() and > do the reset and force mode afterwards. > > Signed-off-by: John Youn > --- > drivers/usb/dwc2/core.c | 5 + > drivers/usb/dwc2/platform.c | 8 ++-- > 2 files changed, 7 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c > index 382f563..7090050 100644 > --- a/drivers/usb/dwc2/core.c > +++ b/drivers/usb/dwc2/core.c > @@ -3116,6 +3116,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) > u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4; > u32 hptxfsiz, grxfsiz, gnptxfsiz; > u32 gusbcfg = 0; > + int retval; > > /* > * Attempt to ensure this device is really a DWC_otg Controller. nit: technically the comment from commit aa63719d0b31 ("usb: dwc2: Improve handling of host and device hwparams") adding that this function now does a reset should have been added in this patch. > @@ -3135,6 +3136,10 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) > hw->snpsid >> 12 & 0xf, hw->snpsid >> 8 & 0xf, > hw->snpsid >> 4 & 0xf, hw->snpsid & 0xf, hw->snpsid); > > + retval = dwc2_core_reset(hsotg); > + if (retval) > + return retval; > + > hwcfg1 = dwc2_readl(hsotg->regs + GHWCFG1); > hwcfg2 = dwc2_readl(hsotg->regs + GHWCFG2); > hwcfg3 = dwc2_readl(hsotg->regs + GHWCFG3); > diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c > index f18d50f..12826a1 100644 > --- a/drivers/usb/dwc2/platform.c > +++ b/drivers/usb/dwc2/platform.c > @@ -510,12 +510,6 @@ static int dwc2_driver_probe(struct platform_device *dev) > if (retval) > return retval; > > - /* > -* Reset before dwc2_get_hwparams() then it could get power-on real > -* reset value form registers. > -*/ > - dwc2_core_reset_and_force_mode(hsotg); > - > /* Detect config values from hardware */ > retval = dwc2_get_hwparams(hsotg); > if (retval) > @@ -524,6 +518,8 @@ static int dwc2_driver_probe(struct platform_device *dev) > /* Validate parameter values */ > dwc2_set_parameters(hsotg, params); > > + dwc2_core_reset_and_force_mode(hsotg); > + Isn't this introducing a 2nd reset that's not necessary? You already know that dwc2_get_hwparams() did a reset and now you're doing a 2nd reset. Maybe I'm misunderstanding... -Doug -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 13/18] usb: dwc2: Move reset into dwc2_get_hwparams()
On 12/9/2015 4:46 PM, Doug Anderson wrote: > John, > > On Wed, Dec 2, 2015 at 11:14 AM, John Younwrote: >> The reset is required to get reset values of the hardware parameters but >> the force mode is not. Move the base reset into dwc2_get_hwparams() and >> do the reset and force mode afterwards. >> >> Signed-off-by: John Youn >> --- >> drivers/usb/dwc2/core.c | 5 + >> drivers/usb/dwc2/platform.c | 8 ++-- >> 2 files changed, 7 insertions(+), 6 deletions(-) >> >> diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c >> index 382f563..7090050 100644 >> --- a/drivers/usb/dwc2/core.c >> +++ b/drivers/usb/dwc2/core.c >> @@ -3116,6 +3116,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) >> u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4; >> u32 hptxfsiz, grxfsiz, gnptxfsiz; >> u32 gusbcfg = 0; >> + int retval; >> >> /* >> * Attempt to ensure this device is really a DWC_otg Controller. > > nit: technically the comment from commit aa63719d0b31 ("usb: dwc2: > Improve handling of host and device hwparams") adding that this > function now does a reset should have been added in this patch. Ahh thanks. I'll fix it if I have to respin this series. > > >> @@ -3135,6 +3136,10 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) >> hw->snpsid >> 12 & 0xf, hw->snpsid >> 8 & 0xf, >> hw->snpsid >> 4 & 0xf, hw->snpsid & 0xf, hw->snpsid); >> >> + retval = dwc2_core_reset(hsotg); >> + if (retval) >> + return retval; >> + >> hwcfg1 = dwc2_readl(hsotg->regs + GHWCFG1); >> hwcfg2 = dwc2_readl(hsotg->regs + GHWCFG2); >> hwcfg3 = dwc2_readl(hsotg->regs + GHWCFG3); >> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c >> index f18d50f..12826a1 100644 >> --- a/drivers/usb/dwc2/platform.c >> +++ b/drivers/usb/dwc2/platform.c >> @@ -510,12 +510,6 @@ static int dwc2_driver_probe(struct platform_device >> *dev) >> if (retval) >> return retval; >> >> - /* >> -* Reset before dwc2_get_hwparams() then it could get power-on real >> -* reset value form registers. >> -*/ >> - dwc2_core_reset_and_force_mode(hsotg); >> - >> /* Detect config values from hardware */ >> retval = dwc2_get_hwparams(hsotg); >> if (retval) >> @@ -524,6 +518,8 @@ static int dwc2_driver_probe(struct platform_device *dev) >> /* Validate parameter values */ >> dwc2_set_parameters(hsotg, params); >> >> + dwc2_core_reset_and_force_mode(hsotg); >> + > > Isn't this introducing a 2nd reset that's not necessary? You already > know that dwc2_get_hwparams() did a reset and now you're doing a 2nd > reset. Maybe I'm misunderstanding... I wanted to preserve setting force mode just before calling the gadget/hcd init. Only a plain (fast) reset is required to read the HW params. But it is redundant. I can split out the force mode to avoid the extra reset. I'll send an add-on patch to this series that does that. Regards, John -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 13/18] usb: dwc2: Move reset into dwc2_get_hwparams()
The reset is required to get reset values of the hardware parameters but the force mode is not. Move the base reset into dwc2_get_hwparams() and do the reset and force mode afterwards. Signed-off-by: John Youn--- drivers/usb/dwc2/core.c | 5 + drivers/usb/dwc2/platform.c | 8 ++-- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index 382f563..7090050 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -3116,6 +3116,7 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) u32 hwcfg1, hwcfg2, hwcfg3, hwcfg4; u32 hptxfsiz, grxfsiz, gnptxfsiz; u32 gusbcfg = 0; + int retval; /* * Attempt to ensure this device is really a DWC_otg Controller. @@ -3135,6 +3136,10 @@ int dwc2_get_hwparams(struct dwc2_hsotg *hsotg) hw->snpsid >> 12 & 0xf, hw->snpsid >> 8 & 0xf, hw->snpsid >> 4 & 0xf, hw->snpsid & 0xf, hw->snpsid); + retval = dwc2_core_reset(hsotg); + if (retval) + return retval; + hwcfg1 = dwc2_readl(hsotg->regs + GHWCFG1); hwcfg2 = dwc2_readl(hsotg->regs + GHWCFG2); hwcfg3 = dwc2_readl(hsotg->regs + GHWCFG3); diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c index f18d50f..12826a1 100644 --- a/drivers/usb/dwc2/platform.c +++ b/drivers/usb/dwc2/platform.c @@ -510,12 +510,6 @@ static int dwc2_driver_probe(struct platform_device *dev) if (retval) return retval; - /* -* Reset before dwc2_get_hwparams() then it could get power-on real -* reset value form registers. -*/ - dwc2_core_reset_and_force_mode(hsotg); - /* Detect config values from hardware */ retval = dwc2_get_hwparams(hsotg); if (retval) @@ -524,6 +518,8 @@ static int dwc2_driver_probe(struct platform_device *dev) /* Validate parameter values */ dwc2_set_parameters(hsotg, params); + dwc2_core_reset_and_force_mode(hsotg); + if (hsotg->dr_mode != USB_DR_MODE_HOST) { retval = dwc2_gadget_init(hsotg, irq); if (retval) -- 2.6.3 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html