Message ID | 20191028215919.83697-1-john.stultz@linaro.org |
---|---|
Headers | show |
Series | Prereqs for HiKey960 USB support | expand |
Hi, John Stultz <john.stultz@linaro.org> writes: > From: Yu Chen <chenyu56@huawei.com> > > On the HiKey960, we need to do a GCTL soft reset when > switching modes. > > Jack Pham also noted that in the Synopsys databook it > mentions performing a GCTL CoreSoftReset when changing the > PrtCapDir between device & host modes. > > So this patch always does a GCTL Core Soft Reset when > changing the mode. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > CC: ShuFan Lee <shufan_lee@richtek.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: Jack Pham <jackp@codeaurora.org> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Signed-off-by: Yu Chen <chenyu56@huawei.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v3: Remove quirk conditional, as Jack Pham noted the > Synopsis databook states this should be done generally. > Also, at Jacks' suggestion, make the reset call before > changing the prtcap direction. > --- > drivers/usb/dwc3/core.c | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 999ce5e84d3c..a039e35ec7ad 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > dwc->current_dr_role = mode; > } > > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc) > +{ > + u32 reg; > + > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > + reg |= DWC3_GCTL_CORESOFTRESET; > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > + > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > + reg &= ~DWC3_GCTL_CORESOFTRESET; > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > +} > + > static void __dwc3_set_mode(struct work_struct *work) > { > struct dwc3 *dwc = work_to_dwc(work); > @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work) > > spin_lock_irqsave(&dwc->lock, flags); > > + /* Execute a GCTL Core Soft Reset when switch mode */ > + dwc3_gctl_core_soft_reset(dwc); > + This is totally unnecessary. We have several platforms supporting dual role *without* this trick. The only reason why the databook mentions a reset is because some registers are shadowed, meaning that they share the same physical space and just appear as different things for SW. The reason being that Synopsys wanted to reduce the area of the IP and decided to shadow registers which are mutually exclusive.
Hi, John Stultz <john.stultz@linaro.org> writes: > From: Yu Chen <chenyu56@huawei.com> > > It needs more time for the device controller to clear the CmdAct of > DEPCMD on Hisilicon Kirin Soc. Why does it need more time? Why is it so that no other platform needs more time, only this one? And which command, specifically, causes problem?
Hi, John Stultz <john.stultz@linaro.org> writes: > The dwc3 core binding specifies three clocks: > ref, bus_early, and suspend > > which are all controlled in the driver together. > > However some variants of the hardware my not have all three clks ^^ may In fact *all* platforms have all three clocks. It's just that in some cases clock pins are shorted together (or take input from same clock). > So this patch reworks the reading of the clks from the dts to > use devm_clk_bulk_get_all() will will fetch all the clocks ^^^^ which? > specified in the dts together. > > This patch was reccomended by Rob Herring <robh@kernel.org> > as an alternative to creating multiple bindings for each variant > of hardware when the only unique bits were clocks and resets. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > CC: ShuFan Lee <shufan_lee@richtek.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: Jack Pham <jackp@codeaurora.org> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Suggested-by: Rob Herring <robh@kernel.org> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v3: Rework dwc3 core rather then adding another dwc-of-simple > binding. > --- > drivers/usb/dwc3/core.c | 20 +++++--------------- > 1 file changed, 5 insertions(+), 15 deletions(-) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index a039e35ec7ad..4d4f1836b62c 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) > return 0; > } > > -static const struct clk_bulk_data dwc3_core_clks[] = { > - { .id = "ref" }, > - { .id = "bus_early" }, > - { .id = "suspend" }, > -}; another option would be to pass three clocks with the same phandle. That would even make sure that clock usage counts are correct, no?
Hi, John Stultz <john.stultz@linaro.org> writes: > The dwc3 core binding specifies one reset. > > However some variants of the hardware my not have more. ^^ may According to synopsys databook, there's a single *input* reset signal on this IP. What is this extra reset you have? Is this, perhaps, specific to your glue layer around the synopsys ip? Should, perhaps, your extra reset be managed by the glue layer?
Hi, John Stultz <john.stultz@linaro.org> writes: > From: Yu Chen <chenyu56@huawei.com> > > The Type-C drivers use USB role switch API to inform the > system about the negotiated data role, so registering a role > switch in the DRD code in order to support platforms with > USB Type-C connectors. > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > CC: ShuFan Lee <shufan_lee@richtek.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: Jack Pham <jackp@codeaurora.org> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Signed-off-by: Yu Chen <chenyu56@huawei.com> > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v2: Fix role_sw and role_switch_default_mode descriptions as > reported by kbuild test robot <lkp@intel.com> > > v3: Split out the role-switch-default-host logic into its own > patch > --- > drivers/usb/dwc3/Kconfig | 1 + > drivers/usb/dwc3/core.h | 3 ++ > drivers/usb/dwc3/drd.c | 66 +++++++++++++++++++++++++++++++++++++++- > 3 files changed, 69 insertions(+), 1 deletion(-) > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > index 89abc6078703..1104745c41a9 100644 > --- a/drivers/usb/dwc3/Kconfig > +++ b/drivers/usb/dwc3/Kconfig > @@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE > bool "Dual Role mode" > depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3)) > depends on (EXTCON=y || EXTCON=USB_DWC3) > + select USB_ROLE_SWITCH so even those using DWC3 as a peripheral-only or host-only driver will need role switch? > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 1c8b349379af..6f19e9891767 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -25,6 +25,7 @@ > #include <linux/usb/ch9.h> > #include <linux/usb/gadget.h> > #include <linux/usb/otg.h> > +#include <linux/usb/role.h> > #include <linux/ulpi/interface.h> > > #include <linux/phy/phy.h> > @@ -951,6 +952,7 @@ struct dwc3_scratchpad_array { > * @hsphy_mode: UTMI phy mode, one of following: > * - USBPHY_INTERFACE_MODE_UTMI > * - USBPHY_INTERFACE_MODE_UTMIW > + * @role_sw: usb_role_switch handle > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > * @usb2_generic_phy: pointer to USB2 PHY > @@ -1084,6 +1086,7 @@ struct dwc3 { > struct extcon_dev *edev; > struct notifier_block edev_nb; > enum usb_phy_interface hsphy_mode; > + struct usb_role_switch *role_sw; > > u32 fladj; > u32 irq_gadget; > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index c946d64142ad..61d4fd8aead4 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -476,6 +476,52 @@ static struct extcon_dev *dwc3_get_extcon(struct dwc3 *dwc) > return edev; > } > > +static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role) > +{ > + struct dwc3 *dwc = dev_get_drvdata(dev); > + u32 mode; > + > + switch (role) { > + case USB_ROLE_HOST: > + mode = DWC3_GCTL_PRTCAP_HOST; > + break; > + case USB_ROLE_DEVICE: > + mode = DWC3_GCTL_PRTCAP_DEVICE; > + break; > + default: > + mode = DWC3_GCTL_PRTCAP_DEVICE; > + break; > + } > + > + dwc3_set_mode(dwc, mode); > + return 0; > +} role switching is starting to get way too complicated in DWC3. We now have a function that queues a work on the system_freezable_wq that will configure PHY and change PRTCAP. Is there a way we can simplify some of this a little?
Hi, John Stultz <john.stultz@linaro.org> writes: > Support configuring the default role the controller assumes as > host mode when the usb role is USB_ROLE_NONE > > This patch was split out from a larger patch originally by > Yu Chen <chenyu56@huawei.com> > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > Cc: Rob Herring <robh+dt@kernel.org> > Cc: Mark Rutland <mark.rutland@arm.com> > CC: ShuFan Lee <shufan_lee@richtek.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > Cc: Yu Chen <chenyu56@huawei.com> > Cc: Felipe Balbi <balbi@kernel.org> > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Jun Li <lijun.kernel@gmail.com> > Cc: Valentin Schneider <valentin.schneider@arm.com> > Cc: Jack Pham <jackp@codeaurora.org> > Cc: linux-usb@vger.kernel.org > Cc: devicetree@vger.kernel.org > Signed-off-by: John Stultz <john.stultz@linaro.org> > --- > v3: Split this patch out from addition of usb-role-switch > handling > --- > drivers/usb/dwc3/core.h | 3 +++ > drivers/usb/dwc3/drd.c | 20 ++++++++++++++++---- > 2 files changed, 19 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index 6f19e9891767..3c879c9ab1aa 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -953,6 +953,8 @@ struct dwc3_scratchpad_array { > * - USBPHY_INTERFACE_MODE_UTMI > * - USBPHY_INTERFACE_MODE_UTMIW > * @role_sw: usb_role_switch handle > + * @role_switch_default_mode: default operation mode of controller while > + * usb role is USB_ROLE_NONE. > * @usb2_phy: pointer to USB2 PHY > * @usb3_phy: pointer to USB3 PHY > * @usb2_generic_phy: pointer to USB2 PHY > @@ -1087,6 +1089,7 @@ struct dwc3 { > struct notifier_block edev_nb; > enum usb_phy_interface hsphy_mode; > struct usb_role_switch *role_sw; > + enum usb_dr_mode role_switch_default_mode; > > u32 fladj; > u32 irq_gadget; > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > index 61d4fd8aead4..0e3466fe5ac4 100644 > --- a/drivers/usb/dwc3/drd.c > +++ b/drivers/usb/dwc3/drd.c > @@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role) > mode = DWC3_GCTL_PRTCAP_DEVICE; > break; > default: > - mode = DWC3_GCTL_PRTCAP_DEVICE; > + if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) > + mode = DWC3_GCTL_PRTCAP_HOST; > + else > + mode = DWC3_GCTL_PRTCAP_DEVICE; > break; > } > > @@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev) > role = dwc->current_otg_role; > break; > default: > - role = USB_ROLE_DEVICE; > + if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) > + role = USB_ROLE_HOST; look at this, we now have 3 different encodings for role which DWC3 needs to understand. One is its own PRTCAP_DIR, then there USB_DR_MODE_* and now USB_ROLE_*, can we make it so that we only have one private encoding and one generic encoding?
On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > > The dwc3 core binding specifies three clocks: > > ref, bus_early, and suspend > > > > which are all controlled in the driver together. > > > > However some variants of the hardware my not have all three clks > ^^ > may > > In fact *all* platforms have all three clocks. It's just that in some > cases clock pins are shorted together (or take input from same clock). > ... > another option would be to pass three clocks with the same phandle. That > would even make sure that clock usage counts are correct, no? Hey Felipe! So I actually had done that initially (and it seemed to work), but Rob suggested this way instead. I'm fine with either, as long as having multiple references to the same clk in the enable/disable paths doesn't cause trouble. Thanks so much for the review here! -john
On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > The dwc3 core binding specifies one reset. > > > > However some variants of the hardware my not have more. > ^^ > may > > According to synopsys databook, there's a single *input* reset signal on > this IP. What is this extra reset you have? > > Is this, perhaps, specific to your glue layer around the synopsys ip? Likely (again, I unfortunately don't have a ton of detail on the hardware). > Should, perhaps, your extra reset be managed by the glue layer? So yes the dwc3-of-simple does much of this already (it handles multiple resets, and variable clocks), but unfortunately we seem to need new bindings for each device added? I think the suggestion from Rob was due to the sprawl of bindings for the glue code, and the extra complexity of the parent node. So I believe Rob just thought it made sense to collapse this down into the core? I'm not really passionate about either approach, and am happy to rework (as long as there is eventual progress :). Just let me know what you'd prefer. thanks -john
On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > From: Yu Chen <chenyu56@huawei.com> > > > > It needs more time for the device controller to clear the CmdAct of > > DEPCMD on Hisilicon Kirin Soc. > > Why does it need more time? Why is it so that no other platform needs > more time, only this one? And which command, specifically, causes > problem? Hrm. Sadly I don't have that context (again I'm picking up a semi-abandoned patchset here), which is unfortunate, as I'm sure someone spent a number of hours debugging things to come up with this. :) But alas, I've dropped this for now in my stack, and things seem to be working ok so far. I suspect there's some edge case I'll run into, but hopefully I'll be able to debug and get more details when that happens. I do appreciate the review and pushback here! thanks -john
On Tue, Oct 29, 2019 at 2:09 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > From: Yu Chen <chenyu56@huawei.com> > > > > On the HiKey960, we need to do a GCTL soft reset when > > switching modes. > > > > Jack Pham also noted that in the Synopsys databook it > > mentions performing a GCTL CoreSoftReset when changing the > > PrtCapDir between device & host modes. > > > > So this patch always does a GCTL Core Soft Reset when > > changing the mode. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > CC: ShuFan Lee <shufan_lee@richtek.com> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > > Cc: Yu Chen <chenyu56@huawei.com> > > Cc: Felipe Balbi <balbi@kernel.org> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Jun Li <lijun.kernel@gmail.com> > > Cc: Valentin Schneider <valentin.schneider@arm.com> > > Cc: Jack Pham <jackp@codeaurora.org> > > Cc: linux-usb@vger.kernel.org > > Cc: devicetree@vger.kernel.org > > Signed-off-by: Yu Chen <chenyu56@huawei.com> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > v3: Remove quirk conditional, as Jack Pham noted the > > Synopsis databook states this should be done generally. > > Also, at Jacks' suggestion, make the reset call before > > changing the prtcap direction. > > --- > > drivers/usb/dwc3/core.c | 16 ++++++++++++++++ > > 1 file changed, 16 insertions(+) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 999ce5e84d3c..a039e35ec7ad 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -112,6 +112,19 @@ void dwc3_set_prtcap(struct dwc3 *dwc, u32 mode) > > dwc->current_dr_role = mode; > > } > > > > +static void dwc3_gctl_core_soft_reset(struct dwc3 *dwc) > > +{ > > + u32 reg; > > + > > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > + reg |= DWC3_GCTL_CORESOFTRESET; > > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > > + > > + reg = dwc3_readl(dwc->regs, DWC3_GCTL); > > + reg &= ~DWC3_GCTL_CORESOFTRESET; > > + dwc3_writel(dwc->regs, DWC3_GCTL, reg); > > +} > > + > > static void __dwc3_set_mode(struct work_struct *work) > > { > > struct dwc3 *dwc = work_to_dwc(work); > > @@ -154,6 +167,9 @@ static void __dwc3_set_mode(struct work_struct *work) > > > > spin_lock_irqsave(&dwc->lock, flags); > > > > + /* Execute a GCTL Core Soft Reset when switch mode */ > > + dwc3_gctl_core_soft_reset(dwc); > > + > > This is totally unnecessary. We have several platforms supporting dual > role *without* this trick. The only reason why the databook mentions a > reset is because some registers are shadowed, meaning that they share > the same physical space and just appear as different things for SW. The > reason being that Synopsys wanted to reduce the area of the IP and > decided to shadow registers which are mutually exclusive. Ok. I've dropped this for now. Without this I do see an occasional issues seemingly more frequently where he board seems to initialize improperly on boot (usb-c is connected, but it doesn't seem to detect until I unplug and replug), but it also trips (though seemingly less frequently) without this, so this may be just affecting the timing of a initialization race issue. I'll watch this for more info and follow up on it later. Thanks for the review! -john
Hi, John Stultz <john.stultz@linaro.org> writes: > On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote: >> John Stultz <john.stultz@linaro.org> writes: >> > The dwc3 core binding specifies one reset. >> > >> > However some variants of the hardware my not have more. >> ^^ >> may >> >> According to synopsys databook, there's a single *input* reset signal on >> this IP. What is this extra reset you have? >> >> Is this, perhaps, specific to your glue layer around the synopsys ip? > > Likely (again, I unfortunately don't have a ton of detail on the hardware). > >> Should, perhaps, your extra reset be managed by the glue layer? > > So yes the dwc3-of-simple does much of this already (it handles > multiple resets, and variable clocks), but unfortunately we seem to > need new bindings for each device added? I think the suggestion from > Rob was due to the sprawl of bindings for the glue code, and the extra > complexity of the parent node. So I believe Rob just thought it made > sense to collapse this down into the core? > > I'm not really passionate about either approach, and am happy to > rework (as long as there is eventual progress :). > Just let me know what you'd prefer. Well, I was under the impression we were supposed to describe the HW. Synopsys IP has a single reset input :-p
Hi, John Stultz <john.stultz@linaro.org> writes: > On Tue, Oct 29, 2019 at 2:14 AM Felipe Balbi <balbi@kernel.org> wrote: >> John Stultz <john.stultz@linaro.org> writes: >> >> > The dwc3 core binding specifies three clocks: >> > ref, bus_early, and suspend >> > >> > which are all controlled in the driver together. >> > >> > However some variants of the hardware my not have all three clks >> ^^ >> may >> >> In fact *all* platforms have all three clocks. It's just that in some >> cases clock pins are shorted together (or take input from same clock). >> > ... >> another option would be to pass three clocks with the same phandle. That >> would even make sure that clock usage counts are correct, no? > > Hey Felipe! > > So I actually had done that initially (and it seemed to work), but Rob > suggested this way instead. > I'm fine with either, as long as having multiple references to the > same clk in the enable/disable paths doesn't cause trouble. > > Thanks so much for the review here! same as the other patch, if we're supposed to describe the HW, then we should describe what's actually happening.
On Wed, Oct 30, 2019 at 4:01 AM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi, > > John Stultz <john.stultz@linaro.org> writes: > > > On Tue, Oct 29, 2019 at 2:17 AM Felipe Balbi <balbi@kernel.org> wrote: > >> John Stultz <john.stultz@linaro.org> writes: > >> > The dwc3 core binding specifies one reset. > >> > > >> > However some variants of the hardware my not have more. > >> ^^ > >> may > >> > >> According to synopsys databook, there's a single *input* reset signal on > >> this IP. What is this extra reset you have? > >> > >> Is this, perhaps, specific to your glue layer around the synopsys ip? > > > > Likely (again, I unfortunately don't have a ton of detail on the hardware). > > > >> Should, perhaps, your extra reset be managed by the glue layer? An extra clock or reset is a silly reason to have a whole other node and driver. If there's additional blocks and registers, then yes a glue node makes sense. > > So yes the dwc3-of-simple does much of this already (it handles > > multiple resets, and variable clocks), but unfortunately we seem to > > need new bindings for each device added? I think the suggestion from > > Rob was due to the sprawl of bindings for the glue code, and the extra > > complexity of the parent node. So I believe Rob just thought it made > > sense to collapse this down into the core? > > > > I'm not really passionate about either approach, and am happy to > > rework (as long as there is eventual progress :). > > Just let me know what you'd prefer. > > Well, I was under the impression we were supposed to describe the > HW. Synopsys IP has a single reset input :-p John is. His chip requires 2 resets to use the USB block and the compatible provides that distinction. Maybe HiSilicon has a newer or customized IP version that has 2 resets. The block could have external RAMs (because every process has its own) which may have their own reset. With NDA specifications and little knowledge of the full revision history, we can really never know. Also, omitting clocks and resets from the dwc3 node entirely is just as much not describing the h/w (only the glue needs clocks?). This block is the oddball. I think there's 1 or 2 other blocks where this glue node was done, but please stop. If we did this every time there's a variation in clocks or resets, we'd pretty much have glue nodes everywhere. Rob
On Tue, Oct 29, 2019 at 4:14 AM Felipe Balbi <balbi@kernel.org> wrote: > > > Hi, > > John Stultz <john.stultz@linaro.org> writes: > > > The dwc3 core binding specifies three clocks: > > ref, bus_early, and suspend > > > > which are all controlled in the driver together. > > > > However some variants of the hardware my not have all three clks > ^^ > may > > In fact *all* platforms have all three clocks. It's just that in some > cases clock pins are shorted together (or take input from same clock). > > > So this patch reworks the reading of the clks from the dts to > > use devm_clk_bulk_get_all() will will fetch all the clocks > ^^^^ > which? > > > specified in the dts together. > > > > This patch was reccomended by Rob Herring <robh@kernel.org> > > as an alternative to creating multiple bindings for each variant > > of hardware when the only unique bits were clocks and resets. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > CC: ShuFan Lee <shufan_lee@richtek.com> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > > Cc: Yu Chen <chenyu56@huawei.com> > > Cc: Felipe Balbi <balbi@kernel.org> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Jun Li <lijun.kernel@gmail.com> > > Cc: Valentin Schneider <valentin.schneider@arm.com> > > Cc: Jack Pham <jackp@codeaurora.org> > > Cc: linux-usb@vger.kernel.org > > Cc: devicetree@vger.kernel.org > > Suggested-by: Rob Herring <robh@kernel.org> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > v3: Rework dwc3 core rather then adding another dwc-of-simple > > binding. > > --- > > drivers/usb/dwc3/core.c | 20 +++++--------------- > > 1 file changed, 5 insertions(+), 15 deletions(-) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index a039e35ec7ad..4d4f1836b62c 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -305,12 +305,6 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc) > > return 0; > > } > > > > -static const struct clk_bulk_data dwc3_core_clks[] = { > > - { .id = "ref" }, > > - { .id = "bus_early" }, > > - { .id = "suspend" }, > > -}; > > another option would be to pass three clocks with the same phandle. That > would even make sure that clock usage counts are correct, no? If you have the datasheet for the block, then perhaps some suggestion of which clocks code be the same. My guess would be ref and suspend. Maybe suspend is a fixed clock which is unmanaged on HiSilicon platforms. If we allow for no clocks on some platforms, then why does it have to be all or none? Rob
On Tue, Oct 29, 2019 at 2:25 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > diff --git a/drivers/usb/dwc3/drd.c b/drivers/usb/dwc3/drd.c > > index 61d4fd8aead4..0e3466fe5ac4 100644 > > --- a/drivers/usb/dwc3/drd.c > > +++ b/drivers/usb/dwc3/drd.c > > @@ -489,7 +489,10 @@ static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role) > > mode = DWC3_GCTL_PRTCAP_DEVICE; > > break; > > default: > > - mode = DWC3_GCTL_PRTCAP_DEVICE; > > + if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) > > + mode = DWC3_GCTL_PRTCAP_HOST; > > + else > > + mode = DWC3_GCTL_PRTCAP_DEVICE; > > break; > > } > > > > @@ -515,7 +518,10 @@ static enum usb_role dwc3_usb_role_switch_get(struct device *dev) > > role = dwc->current_otg_role; > > break; > > default: > > - role = USB_ROLE_DEVICE; > > + if (dwc->role_switch_default_mode == USB_DR_MODE_HOST) > > + role = USB_ROLE_HOST; > > look at this, we now have 3 different encodings for role which DWC3 > needs to understand. One is its own PRTCAP_DIR, then there USB_DR_MODE_* > and now USB_ROLE_*, can we make it so that we only have one private > encoding and one generic encoding? And you left out the DWC3_OTG_ROLE_* set too! So I agree it can be easy to muddle up. The enums are *almost* equivalent: include/linux/usb/role.h: enum usb_role { USB_ROLE_NONE, USB_ROLE_HOST, USB_ROLE_DEVICE, }; include/linux/usb/otg.h: enum usb_dr_mode { USB_DR_MODE_UNKNOWN, USB_DR_MODE_HOST, USB_DR_MODE_PERIPHERAL, USB_DR_MODE_OTG, }; But both are widely used: $ git grep USB_ROLE_ | wc -l 123 $ git grep USB_DR_MODE_ | wc -l 190 So I'm not sure how easy it will be to condense down, since the usage is coming from different usb subsystems (otg and role switching) and I worry assuming them equivalent in just one driver may run into trouble eventually if the values diverge (ie someone adds USB_ROLE_BRICK or something). Heikki/Greg: Any thoughts on this? Does it make sense to try to drop the usb_role enum and users and replace it with usb_dr_mode? thanks -john
On Tue, Oct 29, 2019 at 2:21 AM Felipe Balbi <balbi@kernel.org> wrote: > John Stultz <john.stultz@linaro.org> writes: > > From: Yu Chen <chenyu56@huawei.com> > > > > The Type-C drivers use USB role switch API to inform the > > system about the negotiated data role, so registering a role > > switch in the DRD code in order to support platforms with > > USB Type-C connectors. > > > > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org> > > Cc: Rob Herring <robh+dt@kernel.org> > > Cc: Mark Rutland <mark.rutland@arm.com> > > CC: ShuFan Lee <shufan_lee@richtek.com> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Cc: Suzuki K Poulose <suzuki.poulose@arm.com> > > Cc: Chunfeng Yun <chunfeng.yun@mediatek.com> > > Cc: Yu Chen <chenyu56@huawei.com> > > Cc: Felipe Balbi <balbi@kernel.org> > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Andy Shevchenko <andy.shevchenko@gmail.com> > > Cc: Jun Li <lijun.kernel@gmail.com> > > Cc: Valentin Schneider <valentin.schneider@arm.com> > > Cc: Jack Pham <jackp@codeaurora.org> > > Cc: linux-usb@vger.kernel.org > > Cc: devicetree@vger.kernel.org > > Suggested-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Signed-off-by: Yu Chen <chenyu56@huawei.com> > > Signed-off-by: John Stultz <john.stultz@linaro.org> > > --- > > v2: Fix role_sw and role_switch_default_mode descriptions as > > reported by kbuild test robot <lkp@intel.com> > > > > v3: Split out the role-switch-default-host logic into its own > > patch > > --- > > drivers/usb/dwc3/Kconfig | 1 + > > drivers/usb/dwc3/core.h | 3 ++ > > drivers/usb/dwc3/drd.c | 66 +++++++++++++++++++++++++++++++++++++++- > > 3 files changed, 69 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig > > index 89abc6078703..1104745c41a9 100644 > > --- a/drivers/usb/dwc3/Kconfig > > +++ b/drivers/usb/dwc3/Kconfig > > @@ -44,6 +44,7 @@ config USB_DWC3_DUAL_ROLE > > bool "Dual Role mode" > > depends on ((USB=y || USB=USB_DWC3) && (USB_GADGET=y || USB_GADGET=USB_DWC3)) > > depends on (EXTCON=y || EXTCON=USB_DWC3) > > + select USB_ROLE_SWITCH > > so even those using DWC3 as a peripheral-only or host-only driver will > need role switch? So, just to clarify, the select is added to the CONFIG_USB_DWC3_DUAL_ROLE, wouldn't peripheral-only or host-only drivers select USB_DWC3_GADGET or USB_DWC3_HOST instead? Even so, if you'd prefer I can avoid the select, and add more #ifdef CONFIG_USB_ROLE_SWITCH around the logic added in this patch. I just worry it makes getting a valid config for some devices more complex and clutters the logic a touch. > > +static int dwc3_usb_role_switch_set(struct device *dev, enum usb_role role) > > +{ > > + struct dwc3 *dwc = dev_get_drvdata(dev); > > + u32 mode; > > + > > + switch (role) { > > + case USB_ROLE_HOST: > > + mode = DWC3_GCTL_PRTCAP_HOST; > > + break; > > + case USB_ROLE_DEVICE: > > + mode = DWC3_GCTL_PRTCAP_DEVICE; > > + break; > > + default: > > + mode = DWC3_GCTL_PRTCAP_DEVICE; > > + break; > > + } > > + > > + dwc3_set_mode(dwc, mode); > > + return 0; > > +} > > role switching is starting to get way too complicated in DWC3. We now > have a function that queues a work on the system_freezable_wq that will > configure PHY and change PRTCAP. Is there a way we can simplify some of > this a little? I'm sorry, could you expand a bit on this point? I'm not sure I quite see what you are envisioning as a simpler role_switch set handler? Is the objection that I'm calling dwc3_set_mode() and instead should be calling some non-static variant of __dwc3_set_mode() directly?
John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > John Stultz <john.stultz@linaro.org> writes: > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > It needs more time for the device controller to clear the CmdAct of > > > DEPCMD on Hisilicon Kirin Soc. > > > > Why does it need more time? Why is it so that no other platform needs > > more time, only this one? And which command, specifically, causes > > problem? Sorry for my back to this so late. This change is required on my dwc3 based HW too, I gave a check and the reason is suspend_clk is used in case the PIPE phy is at P3, this slow clock makes my EP command below timeout. dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 00000500 00000000 --> status: Timed Out Success case takes about 400us to complete, see below trace(44.286278 - 44.285897 = 0.000381): configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr 000000006d59aae1 value 00000401 configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr 000000006d59aae1 value 00000401 ... ... configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr 000000006d59aae1 value 00000001 configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 00000500 00000000 --> status: Successful Hi John, Do you still have this problem? if yes, What's the value of USBLNKST[21:18] when the timeout happens? thanks Li Jun > > Hrm. Sadly I don't have that context (again I'm picking up a > semi-abandoned patchset here), which is unfortunate, as I'm sure > someone spent a number of hours debugging things to come up with this. > :) > > But alas, I've dropped this for now in my stack, and things seem to be > working ok so far. I suspect there's some edge case I'll run into, but > hopefully I'll be able to debug and get more details when that > happens. > > I do appreciate the review and pushback here! > > thanks > -john
On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > John Stultz <john.stultz@linaro.org> writes: > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > Why does it need more time? Why is it so that no other platform needs > > > more time, only this one? And which command, specifically, causes > > > problem? > > Sorry for my back to this so late. > > This change is required on my dwc3 based HW too, I gave a check > and the reason is suspend_clk is used in case the PIPE phy is at P3, > this slow clock makes my EP command below timeout. > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > params 00001000 00000500 00000000 --> status: Timed Out > > Success case takes about 400us to complete, see below trace(44.286278 > - 44.285897 = 0.000381): > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > 000000006d59aae1 value 00000401 > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > 000000006d59aae1 value 00000401 > ... ... > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > 000000006d59aae1 value 00000001 > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > 00000500 00000000 --> status: Successful > > Hi John, > > Do you still have this problem? if yes, What's the value of > USBLNKST[21:18] when the timeout happens? Sorry. As I mentioned, I was working to upstream a patchset that I hadn't created, so the context I had was limited. As I couldn't reproduce an issue without the change on the device I had, I figured it would be best to drop it. However, as you have some analysis and rational for why such a change would be needed, I don't have an objection to it. Do you want to resubmit the patch with your explanation and detailed log above in the commit message? thanks -john
John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道: > > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > > John Stultz <john.stultz@linaro.org> writes: > > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > > > Why does it need more time? Why is it so that no other platform needs > > > > more time, only this one? And which command, specifically, causes > > > > problem? > > > > Sorry for my back to this so late. > > > > This change is required on my dwc3 based HW too, I gave a check > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > this slow clock makes my EP command below timeout. > > > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > > params 00001000 00000500 00000000 --> status: Timed Out > > > > Success case takes about 400us to complete, see below trace(44.286278 > > - 44.285897 = 0.000381): > > > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > > 000000006d59aae1 value 00000401 > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > > 000000006d59aae1 value 00000401 > > ... ... > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > > 000000006d59aae1 value 00000001 > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > > 00000500 00000000 --> status: Successful > > > > Hi John, > > > > Do you still have this problem? if yes, What's the value of > > USBLNKST[21:18] when the timeout happens? > > Sorry. As I mentioned, I was working to upstream a patchset that I > hadn't created, so the context I had was limited. As I couldn't > reproduce an issue without the change on the device I had, I figured > it would be best to drop it. That was fine. > > However, as you have some analysis and rational for why such a change > would be needed, I don't have an objection to it. Do you want to > resubmit the patch with your explanation and detailed log above in the > commit message? Sure, I will resubmit the patch with my explanation added in commit message. thanks Li Jun > > thanks > -john
Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道: > > John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道: > > > > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > > > > > John Stultz <john.stultz@linaro.org> writes: > > > > > > From: Yu Chen <chenyu56@huawei.com> > > > > > > > > > > > > It needs more time for the device controller to clear the CmdAct of > > > > > > DEPCMD on Hisilicon Kirin Soc. > > > > > > > > > > Why does it need more time? Why is it so that no other platform needs > > > > > more time, only this one? And which command, specifically, causes > > > > > problem? > > > > > > Sorry for my back to this so late. > > > > > > This change is required on my dwc3 based HW too, I gave a check > > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > > this slow clock makes my EP command below timeout. > > > > > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > > > params 00001000 00000500 00000000 --> status: Timed Out > > > > > > Success case takes about 400us to complete, see below trace(44.286278 > > > - 44.285897 = 0.000381): > > > > > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > > > 000000006d59aae1 value 00000401 > > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > > > 000000006d59aae1 value 00000401 > > > ... ... > > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > > > 000000006d59aae1 value 00000001 > > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > > > 00000500 00000000 --> status: Successful > > > > > > Hi John, > > > > > > Do you still have this problem? if yes, What's the value of > > > USBLNKST[21:18] when the timeout happens? > > > > Sorry. As I mentioned, I was working to upstream a patchset that I > > hadn't created, so the context I had was limited. As I couldn't > > reproduce an issue without the change on the device I had, I figured > > it would be best to drop it. > > That was fine. > > > > However, as you have some analysis and rational for why such a change > > would be needed, I don't have an objection to it. Do you want to > > resubmit the patch with your explanation and detailed log above in the > > commit message? > > Sure, I will resubmit the patch with my explanation added in commit message. Hi John A second think of this, I feel use readl_poll_timeout_atomic() to wait by time is more proper here, so I create a new patch to address this also other registers polling, see below patch with you CCed: https://patchwork.kernel.org/patch/11536081/ thanks Li Jun > > thanks > Li Jun > > > > thanks > > -john
Hi, Jun Li <lijun.kernel@gmail.com> writes: > Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道: >> >> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道: >> > >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: >> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: >> > > > > John Stultz <john.stultz@linaro.org> writes: >> > > > > > From: Yu Chen <chenyu56@huawei.com> >> > > > > > >> > > > > > It needs more time for the device controller to clear the CmdAct of >> > > > > > DEPCMD on Hisilicon Kirin Soc. >> > > > > >> > > > > Why does it need more time? Why is it so that no other platform needs >> > > > > more time, only this one? And which command, specifically, causes >> > > > > problem? >> > > >> > > Sorry for my back to this so late. >> > > >> > > This change is required on my dwc3 based HW too, I gave a check >> > > and the reason is suspend_clk is used in case the PIPE phy is at P3, >> > > this slow clock makes my EP command below timeout. >> > > >> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] >> > > params 00001000 00000500 00000000 --> status: Timed Out >> > > >> > > Success case takes about 400us to complete, see below trace(44.286278 >> > > - 44.285897 = 0.000381): >> > > >> > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr >> > > 000000006d59aae1 value 00000401 >> > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr >> > > 000000006d59aae1 value 00000401 >> > > ... ... >> > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr >> > > 000000006d59aae1 value 00000001 >> > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: >> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 >> > > 00000500 00000000 --> status: Successful >> > > >> > > Hi John, >> > > >> > > Do you still have this problem? if yes, What's the value of >> > > USBLNKST[21:18] when the timeout happens? >> > >> > Sorry. As I mentioned, I was working to upstream a patchset that I >> > hadn't created, so the context I had was limited. As I couldn't >> > reproduce an issue without the change on the device I had, I figured >> > it would be best to drop it. >> >> That was fine. >> > >> > However, as you have some analysis and rational for why such a change >> > would be needed, I don't have an objection to it. Do you want to >> > resubmit the patch with your explanation and detailed log above in the >> > commit message? >> >> Sure, I will resubmit the patch with my explanation added in commit message. > > Hi John > > A second think of this, I feel use readl_poll_timeout_atomic() to wait by time > is more proper here, so I create a new patch to address this also other > registers polling, see below patch with you CCed: > > https://patchwork.kernel.org/patch/11536081/ Fixing a bug has nothing to do with using readl_poll_timeout_atomic(). Please don't mix things as it just makes review time consuming. Let's find out what the bug is all about, only then should we consider moving over to readl_poll_timeout_atomic().
Hi, Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:33写道: > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > >> > >> On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > >> > John Stultz <john.stultz@linaro.org> writes: > >> > > From: Yu Chen <chenyu56@huawei.com> > >> > > > >> > > It needs more time for the device controller to clear the CmdAct of > >> > > DEPCMD on Hisilicon Kirin Soc. > >> > > >> > Why does it need more time? Why is it so that no other platform needs > >> > more time, only this one? And which command, specifically, causes > >> > problem? > > > > Sorry for my back to this so late. > > > > This change is required on my dwc3 based HW too, I gave a check > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > > this slow clock makes my EP command below timeout. > > The phy needs to woken up before the command is triggered. Currently we > only wake up the HS PHY. Does it help you if we wake up the SS phy as > well? > > Something like below ought to do it: > > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c > index a0555252dee0..ee46c2dacaeb 100644 > --- a/drivers/usb/dwc3/gadget.c > +++ b/drivers/usb/dwc3/gadget.c > @@ -274,7 +274,8 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > const struct usb_endpoint_descriptor *desc = dep->endpoint.desc; > struct dwc3 *dwc = dep->dwc; > u32 timeout = 1000; > - u32 saved_config = 0; > + u32 saved_hs_config = 0; > + u32 saved_ss_config = 0; > u32 reg; > > int cmd_status = 0; > @@ -293,19 +294,28 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > if (dwc->gadget.speed <= USB_SPEED_HIGH) { > reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > if (unlikely(reg & DWC3_GUSB2PHYCFG_SUSPHY)) { > - saved_config |= DWC3_GUSB2PHYCFG_SUSPHY; > + saved_hs_config |= DWC3_GUSB2PHYCFG_SUSPHY; > reg &= ~DWC3_GUSB2PHYCFG_SUSPHY; > } > > if (reg & DWC3_GUSB2PHYCFG_ENBLSLPM) { > - saved_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; > + saved_hs_config |= DWC3_GUSB2PHYCFG_ENBLSLPM; > reg &= ~DWC3_GUSB2PHYCFG_ENBLSLPM; > } > > - if (saved_config) > + if (saved_hs_config) > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > } > > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > + if (unlikely(reg & DWC3_GUSB3PIPECTL_SUSPHY)) { > + saved_ss_config |= DWC3_GUSB3PIPECTL_SUSPHY; > + reg &= ~DWC3_GUSB3PIPECTL_SUSPHY; > + } > + > + if (saved_ss_config) > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + > if (DWC3_DEPCMD_CMD(cmd) == DWC3_DEPCMD_STARTTRANSFER) { > int needs_wakeup; > > @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, > dwc3_gadget_ep_get_transfer_index(dep); > } > > - if (saved_config) { > + if (saved_hs_config) { > reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > - reg |= saved_config; > + reg |= saved_hs_config; > dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > } > > + if (saved_ss_config) { > + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > + reg |= saved_ss_config; > + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > + } > + > return ret; > } Unfortunately this way can't work, once the SS PHY enters P3, disable suspend_en can't force SS PHY exit P3, unless do this at the very beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test). thanks Li Jun > > -- > balbi
Felipe Balbi <balbi@kernel.org> 于2020年5月8日周五 下午8:35写道: > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > > Jun Li <lijun.kernel@gmail.com> 于2020年5月7日周四 上午11:08写道: > >> > >> John Stultz <john.stultz@linaro.org> 于2020年5月7日周四 上午6:27写道: > >> > > >> > On Wed, May 6, 2020 at 2:00 AM Jun Li <lijun.kernel@gmail.com> wrote: > >> > > John Stultz <john.stultz@linaro.org> 于2019年10月30日周三 上午5:18写道: > >> > > > On Tue, Oct 29, 2019 at 2:11 AM Felipe Balbi <balbi@kernel.org> wrote: > >> > > > > John Stultz <john.stultz@linaro.org> writes: > >> > > > > > From: Yu Chen <chenyu56@huawei.com> > >> > > > > > > >> > > > > > It needs more time for the device controller to clear the CmdAct of > >> > > > > > DEPCMD on Hisilicon Kirin Soc. > >> > > > > > >> > > > > Why does it need more time? Why is it so that no other platform needs > >> > > > > more time, only this one? And which command, specifically, causes > >> > > > > problem? > >> > > > >> > > Sorry for my back to this so late. > >> > > > >> > > This change is required on my dwc3 based HW too, I gave a check > >> > > and the reason is suspend_clk is used in case the PIPE phy is at P3, > >> > > this slow clock makes my EP command below timeout. > >> > > > >> > > dwc3_gadget_ep_cmd: ep0out: cmd 'Set Endpoint Configuration' [401] > >> > > params 00001000 00000500 00000000 --> status: Timed Out > >> > > > >> > > Success case takes about 400us to complete, see below trace(44.286278 > >> > > - 44.285897 = 0.000381): > >> > > > >> > > configfs_acm.sh-822 [000] d..1 44.285896: dwc3_writel: addr > >> > > 000000006d59aae1 value 00000401 > >> > > configfs_acm.sh-822 [000] d..1 44.285897: dwc3_readl: addr > >> > > 000000006d59aae1 value 00000401 > >> > > ... ... > >> > > configfs_acm.sh-822 [000] d..1 44.286278: dwc3_readl: addr > >> > > 000000006d59aae1 value 00000001 > >> > > configfs_acm.sh-822 [000] d..1 44.286279: dwc3_gadget_ep_cmd: > >> > > ep0out: cmd 'Set Endpoint Configuration' [401] params 00001000 > >> > > 00000500 00000000 --> status: Successful > >> > > > >> > > Hi John, > >> > > > >> > > Do you still have this problem? if yes, What's the value of > >> > > USBLNKST[21:18] when the timeout happens? > >> > > >> > Sorry. As I mentioned, I was working to upstream a patchset that I > >> > hadn't created, so the context I had was limited. As I couldn't > >> > reproduce an issue without the change on the device I had, I figured > >> > it would be best to drop it. > >> > >> That was fine. > >> > > >> > However, as you have some analysis and rational for why such a change > >> > would be needed, I don't have an objection to it. Do you want to > >> > resubmit the patch with your explanation and detailed log above in the > >> > commit message? > >> > >> Sure, I will resubmit the patch with my explanation added in commit message. > > > > Hi John > > > > A second think of this, I feel use readl_poll_timeout_atomic() to wait by time > > is more proper here, so I create a new patch to address this also other > > registers polling, see below patch with you CCed: > > > > https://patchwork.kernel.org/patch/11536081/ > > Fixing a bug has nothing to do with using > readl_poll_timeout_atomic(). Please don't mix things as it just makes > review time consuming. > > Let's find out what the bug is all about, only then should we consider > moving over to readl_poll_timeout_atomic(). Agreed, sorry about that, I will hold on my readl_poll_timeout_atomic() changes until we have a conclusion on this issue fix. thanks Li Jun > > -- > balbi
Hi, Jun Li <lijun.kernel@gmail.com> writes: >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned cmd, >> dwc3_gadget_ep_get_transfer_index(dep); >> } >> >> - if (saved_config) { >> + if (saved_hs_config) { >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> - reg |= saved_config; >> + reg |= saved_hs_config; >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> } >> >> + if (saved_ss_config) { >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> + reg |= saved_ss_config; >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> + } >> + >> return ret; >> } > > Unfortunately this way can't work, once the SS PHY enters P3, disable > suspend_en can't force SS PHY exit P3, unless do this at the very beginning > to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for test). It sounds like you have a quirky PHY. If that's the case, then you probably need to use the flag you mentioned above. Please verify with that.
> -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月15日 17:31 > To: Jun Li <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen > <Thinh.Nguyen@synopsys.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Jun Li <lijun.kernel@gmail.com> writes: > >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned > cmd, > >> dwc3_gadget_ep_get_transfer_index(dep); > >> } > >> > >> - if (saved_config) { > >> + if (saved_hs_config) { > >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > >> - reg |= saved_config; > >> + reg |= saved_hs_config; > >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >> } > >> > >> + if (saved_ss_config) { > >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > >> + reg |= saved_ss_config; > >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > >> + } > >> + > >> return ret; > >> } > > > > Unfortunately this way can't work, once the SS PHY enters P3, disable > > suspend_en can't force SS PHY exit P3, unless do this at the very > > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for > test). > > It sounds like you have a quirky PHY. From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY bit should be as what I said, not a quirky. Hi Thinh, could you comment this? > If that's the case, then you probably need > to use the flag you mentioned above. Please verify with that. With quirk of "snps,dis_u3_susphy_quirk", I had verified it can resolve the problem, but this will make USB3 Super Speed PHY never enter P3, this is a huge impact on USB power consumption. The timeout increase has no impact on those platforms which have no this problem, but can give chance for platform with very low supspend clk(like my case 32k) to work. Thanks Li Jun > > -- > balbi
Hi, Jun Li <jun.li@nxp.com> writes: >> Jun Li <lijun.kernel@gmail.com> writes: >> >> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >> cmd, >> >> dwc3_gadget_ep_get_transfer_index(dep); >> >> } >> >> >> >> - if (saved_config) { >> >> + if (saved_hs_config) { >> >> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >> >> - reg |= saved_config; >> >> + reg |= saved_hs_config; >> >> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >> >> } >> >> >> >> + if (saved_ss_config) { >> >> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >> >> + reg |= saved_ss_config; >> >> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >> >> + } >> >> + >> >> return ret; >> >> } >> > >> > Unfortunately this way can't work, once the SS PHY enters P3, disable >> > suspend_en can't force SS PHY exit P3, unless do this at the very >> > beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >> test). >> >> It sounds like you have a quirky PHY. > > From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY > bit should be as what I said, not a quirky. > > Hi Thinh, could you comment this? > >> If that's the case, then you probably need >> to use the flag you mentioned above. Please verify with that. > > With quirk of "snps,dis_u3_susphy_quirk", I had verified it can > resolve the problem, but this will make USB3 Super Speed PHY > never enter P3, this is a huge impact on USB power consumption. > > The timeout increase has no impact on those platforms which have > no this problem, but can give chance for platform with very low > supspend clk(like my case 32k) to work. I was under the impression that issuing a command would wake the PHY up. I don't have access to DWC3 documentation to verify, but that's as I remember. Is that not the case?
Hi, Jun Li wrote: >> -----Original Message----- >> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >> Sent: 2020年5月15日 17:31 >> To: Jun Li <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen >> <Thinh.Nguyen@synopsys.com> >> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >> controller >> >> >> Hi, >> >> Jun Li <lijun.kernel@gmail.com> writes: >>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >> cmd, >>>> dwc3_gadget_ep_get_transfer_index(dep); >>>> } >>>> >>>> - if (saved_config) { >>>> + if (saved_hs_config) { >>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >>>> - reg |= saved_config; >>>> + reg |= saved_hs_config; >>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>>> } >>>> >>>> + if (saved_ss_config) { >>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >>>> + reg |= saved_ss_config; >>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >>>> + } >>>> + >>>> return ret; >>>> } >>> Unfortunately this way can't work, once the SS PHY enters P3, disable >>> suspend_en can't force SS PHY exit P3, unless do this at the very >>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >> test). >> >> It sounds like you have a quirky PHY. > From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY > bit should be as what I said, not a quirky. > > Hi Thinh, could you comment this? You only need to wake up the usb2 phy when issuing the command while running in highspeed or below. If you're running in SS or higher, internally the controller does it for you for usb3 phy. In Jun's case, it seems like it takes longer for his phy to wake up. IMO, in this case, I think it's fine to increase the command timeout. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > Jun Li wrote: >>> -----Original Message----- >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>> Sent: 2020年5月15日 17:31 >>> To: Jun Li <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh Nguyen >>> <Thinh.Nguyen@synopsys.com> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >>> controller >>> >>> >>> Hi, >>> >>> Jun Li <lijun.kernel@gmail.com> writes: >>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep *dep, unsigned >>> cmd, >>>>> dwc3_gadget_ep_get_transfer_index(dep); >>>>> } >>>>> >>>>> - if (saved_config) { >>>>> + if (saved_hs_config) { >>>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); >>>>> - reg |= saved_config; >>>>> + reg |= saved_hs_config; >>>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); >>>>> } >>>>> >>>>> + if (saved_ss_config) { >>>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); >>>>> + reg |= saved_ss_config; >>>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); >>>>> + } >>>>> + >>>>> return ret; >>>>> } >>>> Unfortunately this way can't work, once the SS PHY enters P3, disable >>>> suspend_en can't force SS PHY exit P3, unless do this at the very >>>> beginning to prevent SS PHY entering P3(e.g. add "snps,dis_u3_susphy_quirk" for >>> test). >>> >>> It sounds like you have a quirky PHY. >> From what I got from the IC design, the behavior of DWC3_GUSB3PIPECTL_SUSPHY >> bit should be as what I said, not a quirky. >> >> Hi Thinh, could you comment this? > > You only need to wake up the usb2 phy when issuing the command while > running in highspeed or below. If you're running in SS or higher, > internally the controller does it for you for usb3 phy. In Jun's case, > it seems like it takes longer for his phy to wake up. > > IMO, in this case, I think it's fine to increase the command timeout. Is there an upper limit to this? Is 32k clock the slowest that can be fed to the PHY as a suspend clock?
Hi, > -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月16日 15:13 > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: > > Jun Li wrote: > >>> -----Original Message----- > >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > >>> Sent: 2020年5月15日 17:31 > >>> To: Jun Li <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@nxp.com>; Jun Li <jun.li@nxp.com>; Thinh > >>> Nguyen <Thinh.Nguyen@synopsys.com> > >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct > >>> cleared by device controller > >>> > >>> > >>> Hi, > >>> > >>> Jun Li <lijun.kernel@gmail.com> writes: > >>>>> @@ -397,12 +407,18 @@ int dwc3_send_gadget_ep_cmd(struct dwc3_ep > >>>>> *dep, unsigned > >>> cmd, > >>>>> dwc3_gadget_ep_get_transfer_index(dep); > >>>>> } > >>>>> > >>>>> - if (saved_config) { > >>>>> + if (saved_hs_config) { > >>>>> reg = dwc3_readl(dwc->regs, DWC3_GUSB2PHYCFG(0)); > >>>>> - reg |= saved_config; > >>>>> + reg |= saved_hs_config; > >>>>> dwc3_writel(dwc->regs, DWC3_GUSB2PHYCFG(0), reg); > >>>>> } > >>>>> > >>>>> + if (saved_ss_config) { > >>>>> + reg = dwc3_readl(dwc->regs, DWC3_GUSB3PIPECTL(0)); > >>>>> + reg |= saved_ss_config; > >>>>> + dwc3_writel(dwc->regs, DWC3_GUSB3PIPECTL(0), reg); > >>>>> + } > >>>>> + > >>>>> return ret; > >>>>> } > >>>> Unfortunately this way can't work, once the SS PHY enters P3, > >>>> disable suspend_en can't force SS PHY exit P3, unless do this at > >>>> the very beginning to prevent SS PHY entering P3(e.g. add > >>>> "snps,dis_u3_susphy_quirk" for > >>> test). > >>> > >>> It sounds like you have a quirky PHY. > >> From what I got from the IC design, the behavior of > >> DWC3_GUSB3PIPECTL_SUSPHY bit should be as what I said, not a quirky. > >> > >> Hi Thinh, could you comment this? > > > > You only need to wake up the usb2 phy when issuing the command while > > running in highspeed or below. If you're running in SS or higher, > > internally the controller does it for you for usb3 phy. In Jun's case, > > it seems like it takes longer for his phy to wake up. > > > > IMO, in this case, I think it's fine to increase the command timeout. > > Is there an upper limit to this? Is 32k clock the slowest that can be fed to the > PHY as a suspend clock? Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale (bits 31:19 of GCTL): "Power Down Scale (PwrDnScale) The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to a small part of the USB3 controller that operates when the SS PHY is in its lowest power (P3) state, and therefore does not provide a clock. The Power Down Scale field specifies how many suspend_clk periods fit into a 16 kHz clock period. When performing the division, round up the remainder. For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) Note: - Minimum Suspend clock frequency is 32 kHz - Maximum Suspend clock frequency is 125 MHz" Li Jun > > -- > balbi
Hi, Jun Li <jun.li@nxp.com> writes: >> >> Hi Thinh, could you comment this? >> > >> > You only need to wake up the usb2 phy when issuing the command while >> > running in highspeed or below. If you're running in SS or higher, >> > internally the controller does it for you for usb3 phy. In Jun's case, >> > it seems like it takes longer for his phy to wake up. >> > >> > IMO, in this case, I think it's fine to increase the command timeout. >> >> Is there an upper limit to this? Is 32k clock the slowest that can be fed to the >> PHY as a suspend clock? > > Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale > (bits 31:19 of GCTL): > > "Power Down Scale (PwrDnScale) > The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source > to a small part of the USB3 controller that operates when the SS PHY > is in its lowest power (P3) state, and therefore does not provide a clock. > The Power Down Scale field specifies how many suspend_clk periods > fit into a 16 kHz clock period. When performing the division, round up > the remainder. > For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend clock, > Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) > Note: > - Minimum Suspend clock frequency is 32 kHz > - Maximum Suspend clock frequency is 125 MHz" Cool, now do we have an upper bound for how many clock cycles it takes to wake up the PHY? Then we can just set the time to that upper bound.
> -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月16日 19:57 > To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi, > > Jun Li <jun.li@nxp.com> writes: > >> >> Hi Thinh, could you comment this? > >> > > >> > You only need to wake up the usb2 phy when issuing the command > >> > while running in highspeed or below. If you're running in SS or > >> > higher, internally the controller does it for you for usb3 phy. In > >> > Jun's case, it seems like it takes longer for his phy to wake up. > >> > > >> > IMO, in this case, I think it's fine to increase the command timeout. > >> > >> Is there an upper limit to this? Is 32k clock the slowest that can be > >> fed to the PHY as a suspend clock? > > > > Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale > > (bits 31:19 of GCTL): > > > > "Power Down Scale (PwrDnScale) > > The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to > > a small part of the USB3 controller that operates when the SS PHY is > > in its lowest power (P3) state, and therefore does not provide a clock. > > The Power Down Scale field specifies how many suspend_clk periods fit > > into a 16 kHz clock period. When performing the division, round up the > > remainder. > > For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend > > clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) > > Note: > > - Minimum Suspend clock frequency is 32 kHz > > - Maximum Suspend clock frequency is 125 MHz" > > Cool, now do we have an upper bound for how many clock cycles it takes to wake up > the PHY? My understanding is this ep command does not wake up the SS PHY, the SS PHY still stays at P3 when execute this ep command. The time required here is to wait controller complete something for this ep command with 32K clock. > Then we can just set the time to that upper bound. Per my test with trace, the time is about 400us(~13 cycles). Thanks Li Jun > > -- > balbi
Jun Li wrote: >> -----Original Message----- >> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >> Sent: 2020年5月16日 19:57 >> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li >> <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >> controller >> >> >> Hi, >> >> Jun Li <jun.li@nxp.com> writes: >>>>>> Hi Thinh, could you comment this? >>>>> You only need to wake up the usb2 phy when issuing the command >>>>> while running in highspeed or below. If you're running in SS or >>>>> higher, internally the controller does it for you for usb3 phy. In >>>>> Jun's case, it seems like it takes longer for his phy to wake up. >>>>> >>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>> Is there an upper limit to this? Is 32k clock the slowest that can be >>>> fed to the PHY as a suspend clock? >>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale >>> (bits 31:19 of GCTL): >>> >>> "Power Down Scale (PwrDnScale) >>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to >>> a small part of the USB3 controller that operates when the SS PHY is >>> in its lowest power (P3) state, and therefore does not provide a clock. >>> The Power Down Scale field specifies how many suspend_clk periods fit >>> into a 16 kHz clock period. When performing the division, round up the >>> remainder. >>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend >>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) >>> Note: >>> - Minimum Suspend clock frequency is 32 kHz >>> - Maximum Suspend clock frequency is 125 MHz" >> Cool, now do we have an upper bound for how many clock cycles it takes to wake up >> the PHY? > My understanding is this ep command does not wake up the SS PHY, > the SS PHY still stays at P3 when execute this ep command. The time > required here is to wait controller complete something for this ep > command with 32K clock. Sorry I made a mistake. You're right. Just checked with one of the RTL engineers, and it doesn't need to wake up the phy. However, if it is eSS speed, it may take longer time as the command may be completing with the suspend clock. BR, Thinh > >> Then we can just set the time to that upper bound. > Per my test with trace, the time is about 400us(~13 cycles). > > Thanks > Li Jun >> -- >> balbi
Thinh Nguyen wrote: > Jun Li wrote: >>> -----Original Message----- >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>> Sent: 2020年5月16日 19:57 >>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li >>> <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >>> controller >>> >>> >>> Hi, >>> >>> Jun Li <jun.li@nxp.com> writes: >>>>>>> Hi Thinh, could you comment this? >>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>> while running in highspeed or below. If you're running in SS or >>>>>> higher, internally the controller does it for you for usb3 phy. In >>>>>> Jun's case, it seems like it takes longer for his phy to wake up. >>>>>> >>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>> Is there an upper limit to this? Is 32k clock the slowest that can be >>>>> fed to the PHY as a suspend clock? >>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down Scale >>>> (bits 31:19 of GCTL): >>>> >>>> "Power Down Scale (PwrDnScale) >>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source to >>>> a small part of the USB3 controller that operates when the SS PHY is >>>> in its lowest power (P3) state, and therefore does not provide a clock. >>>> The Power Down Scale field specifies how many suspend_clk periods fit >>>> into a 16 kHz clock period. When performing the division, round up the >>>> remainder. >>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz Suspend >>>> clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 (rounder up) >>>> Note: >>>> - Minimum Suspend clock frequency is 32 kHz >>>> - Maximum Suspend clock frequency is 125 MHz" >>> Cool, now do we have an upper bound for how many clock cycles it takes to wake up >>> the PHY? >> My understanding is this ep command does not wake up the SS PHY, >> the SS PHY still stays at P3 when execute this ep command. The time >> required here is to wait controller complete something for this ep >> command with 32K clock. > Sorry I made a mistake. You're right. Just checked with one of the RTL > engineers, and it doesn't need to wake up the phy. However, if it is eSS > speed, it may take longer time as the command may be completing with the > suspend clock. > What's the value for GCTL[7:6]? BR, Thinh
Hi > -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: 2020年5月19日 14:46 > To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > Thinh Nguyen wrote: > > Jun Li wrote: > >>> -----Original Message----- > >>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > >>> Sent: 2020年5月16日 19:57 > >>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen > >>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen > >>> <Thinh.Nguyen@synopsys.com> > >>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct > >>> cleared by device controller > >>> > >>> > >>> Hi, > >>> > >>> Jun Li <jun.li@nxp.com> writes: > >>>>>>> Hi Thinh, could you comment this? > >>>>>> You only need to wake up the usb2 phy when issuing the command > >>>>>> while running in highspeed or below. If you're running in SS or > >>>>>> higher, internally the controller does it for you for usb3 phy. > >>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. > >>>>>> > >>>>>> IMO, in this case, I think it's fine to increase the command timeout. > >>>>> Is there an upper limit to this? Is 32k clock the slowest that can > >>>>> be fed to the PHY as a suspend clock? > >>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down > >>>> Scale (bits 31:19 of GCTL): > >>>> > >>>> "Power Down Scale (PwrDnScale) > >>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source > >>>> to a small part of the USB3 controller that operates when the SS > >>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock. > >>>> The Power Down Scale field specifies how many suspend_clk periods > >>>> fit into a 16 kHz clock period. When performing the division, round > >>>> up the remainder. > >>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz > >>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 > >>>> (rounder up) > >>>> Note: > >>>> - Minimum Suspend clock frequency is 32 kHz > >>>> - Maximum Suspend clock frequency is 125 MHz" > >>> Cool, now do we have an upper bound for how many clock cycles it > >>> takes to wake up the PHY? > >> My understanding is this ep command does not wake up the SS PHY, the > >> SS PHY still stays at P3 when execute this ep command. The time > >> required here is to wait controller complete something for this ep > >> command with 32K clock. > > Sorry I made a mistake. You're right. Just checked with one of the RTL > > engineers, and it doesn't need to wake up the phy. However, if it is > > eSS speed, it may take longer time as the command may be completing > > with the suspend clock. > > > > What's the value for GCTL[7:6]? 2'b00 Thanks Li Jun > > BR, > Thinh
Jun Li wrote: > Hi > >> -----Original Message----- >> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >> Sent: 2020年5月19日 14:46 >> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li >> <lijun.kernel@gmail.com> >> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >> Peter Chen <peter.chen@nxp.com> >> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >> controller >> >> Thinh Nguyen wrote: >>> Jun Li wrote: >>>>> -----Original Message----- >>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>>>> Sent: 2020年5月16日 19:57 >>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen >>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> >>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml >>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg >>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring >>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan >>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus >>>>> <heikki.krogerus@linux.intel.com>; >>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; >>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider >>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; >>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE >>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen >>>>> <Thinh.Nguyen@synopsys.com> >>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>> cleared by device controller >>>>> >>>>> >>>>> Hi, >>>>> >>>>> Jun Li <jun.li@nxp.com> writes: >>>>>>>>> Hi Thinh, could you comment this? >>>>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>>>> while running in highspeed or below. If you're running in SS or >>>>>>>> higher, internally the controller does it for you for usb3 phy. >>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. >>>>>>>> >>>>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can >>>>>>> be fed to the PHY as a suspend clock? >>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down >>>>>> Scale (bits 31:19 of GCTL): >>>>>> >>>>>> "Power Down Scale (PwrDnScale) >>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source >>>>>> to a small part of the USB3 controller that operates when the SS >>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock. >>>>>> The Power Down Scale field specifies how many suspend_clk periods >>>>>> fit into a 16 kHz clock period. When performing the division, round >>>>>> up the remainder. >>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz >>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 >>>>>> (rounder up) >>>>>> Note: >>>>>> - Minimum Suspend clock frequency is 32 kHz >>>>>> - Maximum Suspend clock frequency is 125 MHz" >>>>> Cool, now do we have an upper bound for how many clock cycles it >>>>> takes to wake up the PHY? >>>> My understanding is this ep command does not wake up the SS PHY, the >>>> SS PHY still stays at P3 when execute this ep command. The time >>>> required here is to wait controller complete something for this ep >>>> command with 32K clock. >>> Sorry I made a mistake. You're right. Just checked with one of the RTL >>> engineers, and it doesn't need to wake up the phy. However, if it is >>> eSS speed, it may take longer time as the command may be completing >>> with the suspend clock. >>> >> What's the value for GCTL[7:6]? > 2'b00 > > Thanks > Li Jun (Sorry for the delay reply) If it's 0, then the ram clock should be the same as the bus_clk, which is odd since you mentioned that the suspend_clk is used instead while in P3. Anyway, I was looking for a way maybe to improve the speed during issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should wakeup the phy anytime. I think Felipe suggested it. It's odd that it doesn't work for you. I don't have other ideas beside increasing the command timeout. Thanks, Thinh
Thinh Nguyen wrote: > Jun Li wrote: >> Hi >> >>> -----Original Message----- >>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> >>> Sent: 2020年5月19日 14:46 >>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li >>> <lijun.kernel@gmail.com> >>> Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu >>> Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob >>> Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee >>> <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko >>> <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; >>> Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open >>> list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>> Peter Chen <peter.chen@nxp.com> >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device >>> controller >>> >>> Thinh Nguyen wrote: >>>> Jun Li wrote: >>>>>> -----Original Message----- >>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi >>>>>> Sent: 2020年5月16日 19:57 >>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen >>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> >>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg >>>>>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring >>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan >>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus >>>>>> <heikki.krogerus@linux.intel.com>; >>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun >>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; >>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider >>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; >>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE >>>>>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; >>>>>> Peter Chen <peter.chen@nxp.com>; Thinh Nguyen >>>>>> <Thinh.Nguyen@synopsys.com> >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct >>>>>> cleared by device controller >>>>>> >>>>>> >>>>>> Hi, >>>>>> >>>>>> Jun Li <jun.li@nxp.com> writes: >>>>>>>>>> Hi Thinh, could you comment this? >>>>>>>>> You only need to wake up the usb2 phy when issuing the command >>>>>>>>> while running in highspeed or below. If you're running in SS or >>>>>>>>> higher, internally the controller does it for you for usb3 phy. >>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. >>>>>>>>> >>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout. >>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that can >>>>>>>> be fed to the PHY as a suspend clock? >>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down >>>>>>> Scale (bits 31:19 of GCTL): >>>>>>> >>>>>>> "Power Down Scale (PwrDnScale) >>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source >>>>>>> to a small part of the USB3 controller that operates when the SS >>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock. >>>>>>> The Power Down Scale field specifies how many suspend_clk periods >>>>>>> fit into a 16 kHz clock period. When performing the division, round >>>>>>> up the remainder. >>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz >>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 >>>>>>> (rounder up) >>>>>>> Note: >>>>>>> - Minimum Suspend clock frequency is 32 kHz >>>>>>> - Maximum Suspend clock frequency is 125 MHz" >>>>>> Cool, now do we have an upper bound for how many clock cycles it >>>>>> takes to wake up the PHY? >>>>> My understanding is this ep command does not wake up the SS PHY, the >>>>> SS PHY still stays at P3 when execute this ep command. The time >>>>> required here is to wait controller complete something for this ep >>>>> command with 32K clock. >>>> Sorry I made a mistake. You're right. Just checked with one of the RTL >>>> engineers, and it doesn't need to wake up the phy. However, if it is >>>> eSS speed, it may take longer time as the command may be completing >>>> with the suspend clock. >>>> >>> What's the value for GCTL[7:6]? >> 2'b00 >> >> Thanks >> Li Jun > (Sorry for the delay reply) > > If it's 0, then the ram clock should be the same as the bus_clk, which > is odd since you mentioned that the suspend_clk is used instead while in P3. Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, internally it can still run with suspend clock during P3. > Anyway, I was looking for a way maybe to improve the speed during > issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should > wakeup the phy anytime. I think Felipe suggested it. It's odd that it > doesn't work for you. I don't have other ideas beside increasing the > command timeout. > In any case, increasing the timeout should be fine with me. It maybe difficult to determine the max timeout base on the slowest clock rate and number of cycles. Different controller and controller versions behave differently and may have different number of clock cycles to complete a command. The RTL engineer recommended timeout to be at least 1ms (which maybe more than the polling rate of this patch). I'm fine with either the rate provided by this tested patch or higher. BR, Thinh
Hi, Thinh Nguyen <Thinh.Nguyen@synopsys.com> writes: >>>>>>>> "Power Down Scale (PwrDnScale) >>>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock source >>>>>>>> to a small part of the USB3 controller that operates when the SS >>>>>>>> PHY is in its lowest power (P3) state, and therefore does not provide a clock. >>>>>>>> The Power Down Scale field specifies how many suspend_clk periods >>>>>>>> fit into a 16 kHz clock period. When performing the division, round >>>>>>>> up the remainder. >>>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz >>>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 >>>>>>>> (rounder up) >>>>>>>> Note: >>>>>>>> - Minimum Suspend clock frequency is 32 kHz >>>>>>>> - Maximum Suspend clock frequency is 125 MHz" >>>>>>> Cool, now do we have an upper bound for how many clock cycles it >>>>>>> takes to wake up the PHY? >>>>>> My understanding is this ep command does not wake up the SS PHY, the >>>>>> SS PHY still stays at P3 when execute this ep command. The time >>>>>> required here is to wait controller complete something for this ep >>>>>> command with 32K clock. >>>>> Sorry I made a mistake. You're right. Just checked with one of the RTL >>>>> engineers, and it doesn't need to wake up the phy. However, if it is >>>>> eSS speed, it may take longer time as the command may be completing >>>>> with the suspend clock. >>>>> >>>> What's the value for GCTL[7:6]? >>> 2'b00 >>> >>> Thanks >>> Li Jun >> (Sorry for the delay reply) >> >> If it's 0, then the ram clock should be the same as the bus_clk, which >> is odd since you mentioned that the suspend_clk is used instead while in P3. > > Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, > internally it can still run with suspend clock during P3. > >> Anyway, I was looking for a way maybe to improve the speed during >> issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should >> wakeup the phy anytime. I think Felipe suggested it. It's odd that it >> doesn't work for you. I don't have other ideas beside increasing the >> command timeout. >> > > In any case, increasing the timeout should be fine with me. It maybe > difficult to determine the max timeout base on the slowest clock rate > and number of cycles. Different controller and controller versions > behave differently and may have different number of clock cycles to > complete a command. > > The RTL engineer recommended timeout to be at least 1ms (which maybe > more than the polling rate of this patch). I'm fine with either the rate > provided by this tested patch or higher. A whole ms waiting for a command to complete? Wow, that's a lot of time blocking the CPU. It looks like, perhaps, we should move to command completion interrupts. The difficulty here is that we issue commands from within the interrupt handler and, as such, can't wait_for_completion(). Meanwhile, we will take the timeout increase I guess, otherwise NXP won't have a working setup.
Hi Jun, Felipe Balbi <balbi@kernel.org> writes: >> In any case, increasing the timeout should be fine with me. It maybe >> difficult to determine the max timeout base on the slowest clock rate >> and number of cycles. Different controller and controller versions >> behave differently and may have different number of clock cycles to >> complete a command. >> >> The RTL engineer recommended timeout to be at least 1ms (which maybe >> more than the polling rate of this patch). I'm fine with either the rate >> provided by this tested patch or higher. > > A whole ms waiting for a command to complete? Wow, that's a lot of time > blocking the CPU. It looks like, perhaps, we should move to command > completion interrupts. The difficulty here is that we issue commands > from within the interrupt handler and, as such, can't > wait_for_completion(). > > Meanwhile, we will take the timeout increase I guess, otherwise NXP > won't have a working setup. patch 1 in this series doesn't apply to testing/next. Care to rebase and resend? Thank you
Hi Thinh, > -----Original Message----- > From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > Sent: 2020年5月21日 9:56 > To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > Thinh Nguyen wrote: > > Jun Li wrote: > >> Hi > >> > >>> -----Original Message----- > >>> From: Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >>> Sent: 2020年5月19日 14:46 > >>> To: Jun Li <jun.li@nxp.com>; Felipe Balbi <balbi@kernel.org>; Jun Li > >>> <lijun.kernel@gmail.com> > >>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; Greg > >>> Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>> <heikki.krogerus@linux.intel.com>; > >>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN FIRMWARE > >>> AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > >>> Peter Chen <peter.chen@nxp.com> > >>> Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct > >>> cleared by device controller > >>> > >>> Thinh Nguyen wrote: > >>>> Jun Li wrote: > >>>>>> -----Original Message----- > >>>>>> From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > >>>>>> Sent: 2020年5月16日 19:57 > >>>>>> To: Jun Li <jun.li@nxp.com>; Thinh Nguyen > >>>>>> <Thinh.Nguyen@synopsys.com>; Jun Li <lijun.kernel@gmail.com> > >>>>>> Cc: John Stultz <john.stultz@linaro.org>; lkml > >>>>>> <linux-kernel@vger.kernel.org>; Yu Chen <chenyu56@huawei.com>; > >>>>>> Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob Herring > >>>>>> <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan > >>>>>> Lee <shufan_lee@richtek.com>; Heikki Krogerus > >>>>>> <heikki.krogerus@linux.intel.com>; > >>>>>> Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > >>>>>> <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; > >>>>>> Andy Shevchenko <andy.shevchenko@gmail.com>; Valentin Schneider > >>>>>> <valentin.schneider@arm.com>; Jack Pham <jackp@codeaurora.org>; > >>>>>> Linux USB List <linux-usb@vger.kernel.org>; open list:OPEN > >>>>>> FIRMWARE AND FLATTENED DEVICE TREE BINDINGS > >>>>>> <devicetree@vger.kernel.org>; Peter Chen <peter.chen@nxp.com>; > >>>>>> Thinh Nguyen <Thinh.Nguyen@synopsys.com> > >>>>>> Subject: RE: [PATCH v4 3/9] usb: dwc3: Increase timeout for > >>>>>> CmdAct cleared by device controller > >>>>>> > >>>>>> > >>>>>> Hi, > >>>>>> > >>>>>> Jun Li <jun.li@nxp.com> writes: > >>>>>>>>>> Hi Thinh, could you comment this? > >>>>>>>>> You only need to wake up the usb2 phy when issuing the command > >>>>>>>>> while running in highspeed or below. If you're running in SS > >>>>>>>>> or higher, internally the controller does it for you for usb3 phy. > >>>>>>>>> In Jun's case, it seems like it takes longer for his phy to wake up. > >>>>>>>>> > >>>>>>>>> IMO, in this case, I think it's fine to increase the command timeout. > >>>>>>>> Is there an upper limit to this? Is 32k clock the slowest that > >>>>>>>> can be fed to the PHY as a suspend clock? > >>>>>>> Yes, 32K clock is the slowest, Per DWC3 document on Power Down > >>>>>>> Scale (bits 31:19 of GCTL): > >>>>>>> > >>>>>>> "Power Down Scale (PwrDnScale) > >>>>>>> The USB3 suspend_clk input replaces pipe3_rx_pclk as a clock > >>>>>>> source to a small part of the USB3 controller that operates when > >>>>>>> the SS PHY is in its lowest power (P3) state, and therefore does not provide > a clock. > >>>>>>> The Power Down Scale field specifies how many suspend_clk > >>>>>>> periods fit into a 16 kHz clock period. When performing the > >>>>>>> division, round up the remainder. > >>>>>>> For example, when using an 8-bit/16-bit/32-bit PHY and 25-MHz > >>>>>>> Suspend clock, Power Down Scale = 25000 kHz/16 kHz = 13'd1563 > >>>>>>> (rounder up) > >>>>>>> Note: > >>>>>>> - Minimum Suspend clock frequency is 32 kHz > >>>>>>> - Maximum Suspend clock frequency is 125 MHz" > >>>>>> Cool, now do we have an upper bound for how many clock cycles it > >>>>>> takes to wake up the PHY? > >>>>> My understanding is this ep command does not wake up the SS PHY, > >>>>> the SS PHY still stays at P3 when execute this ep command. The > >>>>> time required here is to wait controller complete something for > >>>>> this ep command with 32K clock. > >>>> Sorry I made a mistake. You're right. Just checked with one of the > >>>> RTL engineers, and it doesn't need to wake up the phy. However, if > >>>> it is eSS speed, it may take longer time as the command may be > >>>> completing with the suspend clock. > >>>> > >>> What's the value for GCTL[7:6]? > >> 2'b00 > >> > >> Thanks > >> Li Jun > > (Sorry for the delay reply) > > > > If it's 0, then the ram clock should be the same as the bus_clk, which > > is odd since you mentioned that the suspend_clk is used instead while in P3. > > Just checked with the RTL engineer, even if GCTL[7:6] is set to 0, internally it > can still run with suspend clock during P3. Thanks for your check. > > > Anyway, I was looking for a way maybe to improve the speed during > > issuing a command. One way is to set GUSB3PIPECTL[17]=0, and it should > > wakeup the phy anytime. I think Felipe suggested it. It's odd that it > > doesn't work for you. I don't have other ideas beside increasing the > > command timeout. > > > > In any case, increasing the timeout should be fine with me. It maybe difficult to > determine the max timeout base on the slowest clock rate and number of cycles. > Different controller and controller versions behave differently and may have > different number of clock cycles to complete a command. > > The RTL engineer recommended timeout to be at least 1ms (which maybe more than the > polling rate of this patch). I'm fine with either the rate provided by this tested > patch or higher. OK, I will change the timeout to be 1ms if no object from Felipe. thanks Li Jun > > BR, > Thinh
Hi Felipe, > -----Original Message----- > From: Felipe Balbi <balbif@gmail.com> On Behalf Of Felipe Balbi > Sent: 2020年5月21日 14:23 > To: Thinh Nguyen <Thinh.Nguyen@synopsys.com>; Jun Li <jun.li@nxp.com>; Jun Li > <lijun.kernel@gmail.com> > Cc: John Stultz <john.stultz@linaro.org>; lkml <linux-kernel@vger.kernel.org>; Yu > Chen <chenyu56@huawei.com>; Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Rob > Herring <robh+dt@kernel.org>; Mark Rutland <mark.rutland@arm.com>; ShuFan Lee > <shufan_lee@richtek.com>; Heikki Krogerus <heikki.krogerus@linux.intel.com>; > Suzuki K Poulose <suzuki.poulose@arm.com>; Chunfeng Yun > <chunfeng.yun@mediatek.com>; Hans de Goede <hdegoede@redhat.com>; Andy Shevchenko > <andy.shevchenko@gmail.com>; Valentin Schneider <valentin.schneider@arm.com>; > Jack Pham <jackp@codeaurora.org>; Linux USB List <linux-usb@vger.kernel.org>; open > list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS <devicetree@vger.kernel.org>; > Peter Chen <peter.chen@nxp.com> > Subject: Re: [PATCH v4 3/9] usb: dwc3: Increase timeout for CmdAct cleared by device > controller > > > Hi Jun, > > Felipe Balbi <balbi@kernel.org> writes: > >> In any case, increasing the timeout should be fine with me. It maybe > >> difficult to determine the max timeout base on the slowest clock rate > >> and number of cycles. Different controller and controller versions > >> behave differently and may have different number of clock cycles to > >> complete a command. > >> > >> The RTL engineer recommended timeout to be at least 1ms (which maybe > >> more than the polling rate of this patch). I'm fine with either the > >> rate provided by this tested patch or higher. > > > > A whole ms waiting for a command to complete? Wow, that's a lot of > > time blocking the CPU. It looks like, perhaps, we should move to > > command completion interrupts. The difficulty here is that we issue > > commands from within the interrupt handler and, as such, can't > > wait_for_completion(). > > > > Meanwhile, we will take the timeout increase I guess, otherwise NXP > > won't have a working setup. > > patch 1 in this series doesn't apply to testing/next. Care to rebase and resend? Sure, I will rebase and resend this patch with timeout loop 5000. Thanks Li Jun > > Thank you > > -- > balbi