| Message ID | 20191014080025.11245-4-patrick.delaunay@st.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
| Series | usb: host: dwc2: use driver model for PHY and CLOCK | expand |
On 10/14/19 10:00 AM, Patrick Delaunay wrote: > Assert reset before deassert in dwc2_reset; > It should be more safe for DWC2. Can you be more descriptive about this issue ? I have no idea what this patch does or fixes from the description. > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > --- > > drivers/usb/host/dwc2.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > index 51023b0c2c..3086411fc4 100644 > --- a/drivers/usb/host/dwc2.c > +++ b/drivers/usb/host/dwc2.c > @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) > return ret; > } > > + reset_assert_bulk(&priv->resets); > + udelay(2); Why is there a 2 uS delay ? > ret = reset_deassert_bulk(&priv->resets); > if (ret) { > reset_release_bulk(&priv->resets); > [...]
Hi, > From: Marek Vasut <marex@denx.de> > Sent: mardi 15 octobre 2019 01:30 > > On 10/14/19 10:00 AM, Patrick Delaunay wrote: > > Assert reset before deassert in dwc2_reset; It should be more safe for > > DWC2. > > Can you be more descriptive about this issue ? I have no idea what this patch does > or fixes from the description. Yes I will explain it in V2 commit message. The issue only occurs if the DWC2 OTG device switch between gadget mode and host mode. For example: some registers initialiaze by the command "ums" (device mode is forced for example), cause problem for the next command "usb start" and vice versa. Even the existing software reset in dwc_otg_core_reset is not enough; the added hardware reset solve all the issues. > > Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> > > --- > > > > drivers/usb/host/dwc2.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index > > 51023b0c2c..3086411fc4 100644 > > --- a/drivers/usb/host/dwc2.c > > +++ b/drivers/usb/host/dwc2.c > > @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) > > return ret; > > } > > > > + reset_assert_bulk(&priv->resets); > > + udelay(2); > > Why is there a 2 uS delay ? I think: no real reason to have 2 us.... It was jus a reasonable time to be sure that the device reset is correctly performed, the reset signal is propagated.... but perhaps that no delay is working... I can test without delay if you prefer... PS: I use the same value than DWC2 gadget driver: Added by my commit c2c74f97afff static int dwc2_udc_otg_reset_init(struct udevice *dev, struct reset_ctl_bulk *resets) { ..... ret = reset_assert_bulk(resets); if (!ret) { udelay(2); ret = reset_deassert_bulk(resets); } .... } > > ret = reset_deassert_bulk(&priv->resets); > > if (ret) { > > reset_release_bulk(&priv->resets); > > > > [...] Regards Patrick
On 11/6/19 7:27 PM, Patrick DELAUNAY wrote: > Hi, Hi, [...] >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index >>> 51023b0c2c..3086411fc4 100644 >>> --- a/drivers/usb/host/dwc2.c >>> +++ b/drivers/usb/host/dwc2.c >>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) >>> return ret; >>> } >>> >>> + reset_assert_bulk(&priv->resets); >>> + udelay(2); >> >> Why is there a 2 uS delay ? > > I think: no real reason to have 2 us.... > > It was jus a reasonable time to be sure that the device reset is correctly > performed, the reset signal is propagated.... > > but perhaps that no delay is working... > I can test without delay if you prefer... > > PS: I use the same value than DWC2 gadget driver: > Added by my commit c2c74f97afff Isn't there a way to poll the IP to determine whether the reset completed ? [...]
Hi, > From: Marek Vasut <marex@denx.de> > Sent: mercredi 6 novembre 2019 23:00 > > On 11/6/19 7:27 PM, Patrick DELAUNAY wrote: > > Hi, > > Hi, > > [...] > > >>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index > >>> 51023b0c2c..3086411fc4 100644 > >>> --- a/drivers/usb/host/dwc2.c > >>> +++ b/drivers/usb/host/dwc2.c > >>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) > >>> return ret; > >>> } > >>> > >>> + reset_assert_bulk(&priv->resets); > >>> + udelay(2); > >> > >> Why is there a 2 uS delay ? > > > > I think: no real reason to have 2 us.... > > > > It was jus a reasonable time to be sure that the device reset is > > correctly performed, the reset signal is propagated.... > > > > but perhaps that no delay is working... > > I can test without delay if you prefer... > > > > PS: I use the same value than DWC2 gadget driver: > > Added by my commit c2c74f97afff > > Isn't there a way to poll the IP to determine whether the reset completed ? It is HW IP reset, the complete state is not available for stm32mp1 reset controller (RCC). And the need reset duration of depends on each IP (can't be handle in reset u-class). I check with DWC2 OTG IP expert, and we found in DWC_otg_databook_3.30a.pdf t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap time (a minimum of 12 cycles of the slowest clock is recommended.) In our board, we have 209MHz for AHB frequency... USB phy clock is 48MHz So freq12 cycles is MIN(57ns, 250ns) < 1us. The 2us value seens a over protection. I will reduce it to 1us in V2 and I will add comments for. > [...] > > -- > Best regards, > Marek Vasut Regards
On 11/8/19 10:53 AM, Patrick DELAUNAY wrote: > Hi, Hi, [...] >>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index >>>>> 51023b0c2c..3086411fc4 100644 >>>>> --- a/drivers/usb/host/dwc2.c >>>>> +++ b/drivers/usb/host/dwc2.c >>>>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) >>>>> return ret; >>>>> } >>>>> >>>>> + reset_assert_bulk(&priv->resets); >>>>> + udelay(2); >>>> >>>> Why is there a 2 uS delay ? >>> >>> I think: no real reason to have 2 us.... >>> >>> It was jus a reasonable time to be sure that the device reset is >>> correctly performed, the reset signal is propagated.... >>> >>> but perhaps that no delay is working... >>> I can test without delay if you prefer... >>> >>> PS: I use the same value than DWC2 gadget driver: >>> Added by my commit c2c74f97afff >> >> Isn't there a way to poll the IP to determine whether the reset completed ? > > It is HW IP reset, the complete state is not available for stm32mp1 reset controller (RCC). > And the need reset duration of depends on each IP (can't be handle in reset u-class). If it's a SoC specific delay, it should be in the reset driver. > I check with DWC2 OTG IP expert, and we found in DWC_otg_databook_3.30a.pdf > > t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap time > (a minimum of 12 cycles of the slowest clock is recommended.) > > In our board, we have 209MHz for AHB frequency... USB phy clock is 48MHz > So freq12 cycles is MIN(57ns, 250ns) < 1us. > > The 2us value seens a over protection. > > I will reduce it to 1us in V2 and I will add comments for. Well, why don't you put this into the reset driver ? Seems to be a more fitting place for this. I don't think every single SoC has the same clock settings.
Hi, > > On 11/8/19 10:53 AM, Patrick DELAUNAY wrote: > > Hi, > > Hi, > > [...] > > >>>>> diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c > >>>>> index > >>>>> 51023b0c2c..3086411fc4 100644 > >>>>> --- a/drivers/usb/host/dwc2.c > >>>>> +++ b/drivers/usb/host/dwc2.c > >>>>> @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) > >>>>> return ret; > >>>>> } > >>>>> > >>>>> + reset_assert_bulk(&priv->resets); > >>>>> + udelay(2); > >>>> > >>>> Why is there a 2 uS delay ? > >>> > >>> I think: no real reason to have 2 us.... > >>> > >>> It was jus a reasonable time to be sure that the device reset is > >>> correctly performed, the reset signal is propagated.... > >>> > >>> but perhaps that no delay is working... > >>> I can test without delay if you prefer... > >>> > >>> PS: I use the same value than DWC2 gadget driver: > >>> Added by my commit c2c74f97afff > >> > >> Isn't there a way to poll the IP to determine whether the reset completed ? > > > > It is HW IP reset, the complete state is not available for stm32mp1 reset > controller (RCC). > > And the need reset duration of depends on each IP (can't be handle in reset u- > class). > > If it's a SoC specific delay, it should be in the reset driver. > > > I check with DWC2 OTG IP expert, and we found in > > DWC_otg_databook_3.30a.pdf > > > > t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap > time > > (a minimum of 12 cycles of the slowest clock is recommended.) > > > > In our board, we have 209MHz for AHB frequency... USB phy clock is > > 48MHz So freq12 cycles is MIN(57ns, 250ns) < 1us. > > > > The 2us value seens a over protection. > > > > I will reduce it to 1us in V2 and I will add comments for. > > Well, why don't you put this into the reset driver ? Seems to be a more fitting place > for this. I don't think every single SoC has the same clock settings. Ok, I will remove the delay in driver. Patrick
On 11/8/19 11:51 AM, Patrick DELAUNAY wrote: [...] >>>> Isn't there a way to poll the IP to determine whether the reset completed ? >>> >>> It is HW IP reset, the complete state is not available for stm32mp1 reset >> controller (RCC). >>> And the need reset duration of depends on each IP (can't be handle in reset u- >> class). >> >> If it's a SoC specific delay, it should be in the reset driver. >> >>> I check with DWC2 OTG IP expert, and we found in >>> DWC_otg_databook_3.30a.pdf >>> >>> t_rst: DWC_otg PHY clock domain reset and AHB hclk domain reset over lap >> time >>> (a minimum of 12 cycles of the slowest clock is recommended.) >>> >>> In our board, we have 209MHz for AHB frequency... USB phy clock is >>> 48MHz So freq12 cycles is MIN(57ns, 250ns) < 1us. >>> >>> The 2us value seens a over protection. >>> >>> I will reduce it to 1us in V2 and I will add comments for. >> >> Well, why don't you put this into the reset driver ? Seems to be a more fitting place >> for this. I don't think every single SoC has the same clock settings. > > Ok, I will remove the delay in driver. Does it make sense to put it into the reset driver though ?
diff --git a/drivers/usb/host/dwc2.c b/drivers/usb/host/dwc2.c index 51023b0c2c..3086411fc4 100644 --- a/drivers/usb/host/dwc2.c +++ b/drivers/usb/host/dwc2.c @@ -1149,6 +1149,8 @@ static int dwc2_reset(struct udevice *dev) return ret; } + reset_assert_bulk(&priv->resets); + udelay(2); ret = reset_deassert_bulk(&priv->resets); if (ret) { reset_release_bulk(&priv->resets);
Assert reset before deassert in dwc2_reset; It should be more safe for DWC2. Signed-off-by: Patrick Delaunay <patrick.delaunay@st.com> --- drivers/usb/host/dwc2.c | 2 ++ 1 file changed, 2 insertions(+)