On 12/9/2015 4:46 PM, Doug Anderson wrote: > John, > > On Wed, Dec 2, 2015 at 11:14 AM, John Youn <John.Youn@xxxxxxxxxxxx> wrote: >> 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 <johnyoun@xxxxxxxxxxxx> >> --- >> 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html