| Message ID | 1616513066-62025-1-git-send-email-zhouyanjie@wanyeetech.com |
|---|---|
| State | New |
| Headers | show |
| Series | USB: DWC2: Add VBUS overcurrent detection control. | expand |
On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) wrote: > Introduce configurable option for enabling GOTGCTL register > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables > VBUS overcurrent detection. > > This patch is derived from Dragan Čečavac (in the kernel 3.18 > tree of CI20). It is very useful for the MIPS Creator CI20(r1). > Without this patch, CI20's OTG port has a great probability to > face overcurrent warning, which breaks the OTG functionality. > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> > --- > drivers/usb/dwc2/Kconfig | 6 ++++++ > drivers/usb/dwc2/core.c | 9 +++++++++ > 2 files changed, 15 insertions(+) > > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig > index c131719..e40d187 100644 > --- a/drivers/usb/dwc2/Kconfig > +++ b/drivers/usb/dwc2/Kconfig > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC > non-periodic transfers, but of course the debug logs will be > incomplete. Note that this also disables some debug messages > for which the transfer type cannot be deduced. > + > +config USB_DWC2_DISABLE_VOD > + bool "Disable VBUS overcurrent detection" > + help > + Say Y here to switch off VBUS overcurrent detection. It enables USB > + functionality blocked by overcurrent detection. Why would this be a configuration option? Shouldn't this be dynamic and just work properly automatically? You should not have to do this on a build-time basis, it should be able to be detected and handled properly at run-time for all devices. If you know this is needed for a specific type of device, detect it and make the change then, otherwise this could break working systems, right? thanks, greg k-h
Hi Greg, Sorry for taking so long to reply. 于 Tue, 23 Mar 2021 16:31:29 +0100 Greg KH <gregkh@linuxfoundation.org> 写道: > On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) wrote: > > Introduce configurable option for enabling GOTGCTL register > > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables > > VBUS overcurrent detection. > > > > This patch is derived from Dragan Čečavac (in the kernel 3.18 > > tree of CI20). It is very useful for the MIPS Creator CI20(r1). > > Without this patch, CI20's OTG port has a great probability to > > face overcurrent warning, which breaks the OTG functionality. > > > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> > > --- > > drivers/usb/dwc2/Kconfig | 6 ++++++ > > drivers/usb/dwc2/core.c | 9 +++++++++ > > 2 files changed, 15 insertions(+) > > > > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig > > index c131719..e40d187 100644 > > --- a/drivers/usb/dwc2/Kconfig > > +++ b/drivers/usb/dwc2/Kconfig > > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC > > non-periodic transfers, but of course the debug logs > > will be incomplete. Note that this also disables some debug messages > > for which the transfer type cannot be deduced. > > + > > +config USB_DWC2_DISABLE_VOD > > + bool "Disable VBUS overcurrent detection" > > + help > > + Say Y here to switch off VBUS overcurrent detection. It > > enables USB > > + functionality blocked by overcurrent detection. > > Why would this be a configuration option? Shouldn't this be dynamic > and just work properly automatically? > > You should not have to do this on a build-time basis, it should be > able to be detected and handled properly at run-time for all devices. > I consulted the original author Dragan Čečavac, he think since this is a feature which disables overcurrent detection, so we are not sure if it could be harmful for some devices. Therefore he advise against enabling it in runtime, and in favor that user explicitely has to enable it. > If you know this is needed for a specific type of device, detect it > and make the change then, otherwise this could break working systems, > right? According to the information provided by Dragan Čečavac, this function (select whether to enable over-current detection through the otgctl register) don't seem to be available for all dwc2 controllers, so it might make sense to add MACH_INGENIC dependency to USB_DWC2_DISABLE_VOD, which could provide additional protection from unwanted usage. Thanks and best regards! > > thanks, > > greg k-h
Hi Zhou, Le mar., juin 15 2021 at 16:16:39 +0800, 周琰杰 <zhouyanjie@wanyeetech.com> a écrit : > Hi Greg, > > Sorry for taking so long to reply. > > 于 Tue, 23 Mar 2021 16:31:29 +0100 > Greg KH <gregkh@linuxfoundation.org> 写道: > >> On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) >> wrote: >> > Introduce configurable option for enabling GOTGCTL register >> > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables >> > VBUS overcurrent detection. >> > >> > This patch is derived from Dragan Čečavac (in the kernel 3.18 >> > tree of CI20). It is very useful for the MIPS Creator CI20(r1). >> > Without this patch, CI20's OTG port has a great probability to >> > face overcurrent warning, which breaks the OTG functionality. >> > >> > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> >> > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> >> > --- >> > drivers/usb/dwc2/Kconfig | 6 ++++++ >> > drivers/usb/dwc2/core.c | 9 +++++++++ >> > 2 files changed, 15 insertions(+) >> > >> > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig >> > index c131719..e40d187 100644 >> > --- a/drivers/usb/dwc2/Kconfig >> > +++ b/drivers/usb/dwc2/Kconfig >> > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC >> > non-periodic transfers, but of course the debug logs >> > will be incomplete. Note that this also disables some debug >> messages >> > for which the transfer type cannot be deduced. >> > + >> > +config USB_DWC2_DISABLE_VOD >> > + bool "Disable VBUS overcurrent detection" >> > + help >> > + Say Y here to switch off VBUS overcurrent detection. It >> > enables USB >> > + functionality blocked by overcurrent detection. >> >> Why would this be a configuration option? Shouldn't this be dynamic >> and just work properly automatically? >> >> You should not have to do this on a build-time basis, it should be >> able to be detected and handled properly at run-time for all >> devices. >> > > I consulted the original author Dragan Čečavac, he think since this > is > a feature which disables overcurrent detection, so we are not sure if > it could be harmful for some devices. Therefore he advise against > enabling it in runtime, and in favor that user explicitely has to > enable it. This could still be enabled at runtime, though, via a module parameter. Leave it enabled by default, and those who want to disable it can do it. Also, overcurrent detection is just "detection", so enabling or disabling it won't change the fact that you can get overcurrent conditions, right? -Paul >> If you know this is needed for a specific type of device, detect it >> and make the change then, otherwise this could break working >> systems, >> right? > > According to the information provided by Dragan Čečavac, this > function > (select whether to enable over-current detection through the otgctl > register) don't seem to be available for all dwc2 controllers, so it > might make sense to add MACH_INGENIC dependency to > USB_DWC2_DISABLE_VOD, which could provide additional protection from > unwanted usage. > > Thanks and best regards! > >> >> thanks, >> >> greg k-h
于 Tue, 15 Jun 2021 09:52:20 +0100 Paul Cercueil <paul@opendingux.net> 写道: > Hi Zhou, > > Le mar., juin 15 2021 at 16:16:39 +0800, 周琰杰 > <zhouyanjie@wanyeetech.com> a écrit : > > Hi Greg, > > > > Sorry for taking so long to reply. > > > > 于 Tue, 23 Mar 2021 16:31:29 +0100 > > Greg KH <gregkh@linuxfoundation.org> 写道: > > > >> On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) > >> wrote: > >> > Introduce configurable option for enabling GOTGCTL register > >> > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables > >> > VBUS overcurrent detection. > >> > > >> > This patch is derived from Dragan Čečavac (in the kernel 3.18 > >> > tree of CI20). It is very useful for the MIPS Creator CI20(r1). > >> > Without this patch, CI20's OTG port has a great probability to > >> > face overcurrent warning, which breaks the OTG functionality. > >> > > >> > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > >> > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> > >> > --- > >> > drivers/usb/dwc2/Kconfig | 6 ++++++ > >> > drivers/usb/dwc2/core.c | 9 +++++++++ > >> > 2 files changed, 15 insertions(+) > >> > > >> > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig > >> > index c131719..e40d187 100644 > >> > --- a/drivers/usb/dwc2/Kconfig > >> > +++ b/drivers/usb/dwc2/Kconfig > >> > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC > >> > non-periodic transfers, but of course the debug logs > >> > will be incomplete. Note that this also disables some debug > >> messages > >> > for which the transfer type cannot be deduced. > >> > + > >> > +config USB_DWC2_DISABLE_VOD > >> > + bool "Disable VBUS overcurrent detection" > >> > + help > >> > + Say Y here to switch off VBUS overcurrent detection. > >> > It enables USB > >> > + functionality blocked by overcurrent detection. > >> > >> Why would this be a configuration option? Shouldn't this be > >> dynamic and just work properly automatically? > >> > >> You should not have to do this on a build-time basis, it should be > >> able to be detected and handled properly at run-time for all > >> devices. > >> > > > > I consulted the original author Dragan Čečavac, he think since this > > is > > a feature which disables overcurrent detection, so we are not sure > > if it could be harmful for some devices. Therefore he advise against > > enabling it in runtime, and in favor that user explicitely has to > > enable it. > > This could still be enabled at runtime, though, via a module > parameter. Leave it enabled by default, and those who want to disable > it can do it. > > Also, overcurrent detection is just "detection", so enabling or > disabling it won't change the fact that you can get overcurrent > conditions, right? emmm, the main problem now is that there is a phenomenon on CI20 r1 (JZ4780) and CU1000 (X1000) that even if there is no overcurrent (nothing is connected), there is still a high probability (about every 10 times it will be 6 to 7 times) an overcurrent warning appears (even when just finish detect the OTG driver during the kernel startup), which then causes the OTG function to not work normally before the system restarts. Thanks and best regards! > > -Paul > > >> If you know this is needed for a specific type of device, detect > >> it and make the change then, otherwise this could break working > >> systems, > >> right? > > > > According to the information provided by Dragan Čečavac, this > > function > > (select whether to enable over-current detection through the otgctl > > register) don't seem to be available for all dwc2 controllers, so it > > might make sense to add MACH_INGENIC dependency to > > USB_DWC2_DISABLE_VOD, which could provide additional protection from > > unwanted usage. > > > > Thanks and best regards! > > > >> > >> thanks, > >> > >> greg k-h >
On Tue, Jun 15, 2021 at 04:16:39PM +0800, 周琰杰 wrote: > Hi Greg, > > Sorry for taking so long to reply. > > 于 Tue, 23 Mar 2021 16:31:29 +0100 > Greg KH <gregkh@linuxfoundation.org> 写道: > > > On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) wrote: > > > Introduce configurable option for enabling GOTGCTL register > > > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables > > > VBUS overcurrent detection. > > > > > > This patch is derived from Dragan Čečavac (in the kernel 3.18 > > > tree of CI20). It is very useful for the MIPS Creator CI20(r1). > > > Without this patch, CI20's OTG port has a great probability to > > > face overcurrent warning, which breaks the OTG functionality. > > > > > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > > > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> > > > --- > > > drivers/usb/dwc2/Kconfig | 6 ++++++ > > > drivers/usb/dwc2/core.c | 9 +++++++++ > > > 2 files changed, 15 insertions(+) > > > > > > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig > > > index c131719..e40d187 100644 > > > --- a/drivers/usb/dwc2/Kconfig > > > +++ b/drivers/usb/dwc2/Kconfig > > > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC > > > non-periodic transfers, but of course the debug logs > > > will be incomplete. Note that this also disables some debug messages > > > for which the transfer type cannot be deduced. > > > + > > > +config USB_DWC2_DISABLE_VOD > > > + bool "Disable VBUS overcurrent detection" > > > + help > > > + Say Y here to switch off VBUS overcurrent detection. It > > > enables USB > > > + functionality blocked by overcurrent detection. > > > > Why would this be a configuration option? Shouldn't this be dynamic > > and just work properly automatically? > > > > You should not have to do this on a build-time basis, it should be > > able to be detected and handled properly at run-time for all devices. > > > > I consulted the original author Dragan Čečavac, he think since this is > a feature which disables overcurrent detection, so we are not sure if > it could be harmful for some devices. Therefore he advise against > enabling it in runtime, and in favor that user explicitely has to > enable it. That will not work AT ALL for systems that build a generic kernel for multiple devices (i.e. Android kernels), so please make this a runtime determination based on the hardware capabilities. > > If you know this is needed for a specific type of device, detect it > > and make the change then, otherwise this could break working systems, > > right? > > According to the information provided by Dragan Čečavac, this function > (select whether to enable over-current detection through the otgctl > register) don't seem to be available for all dwc2 controllers, so it > might make sense to add MACH_INGENIC dependency to > USB_DWC2_DISABLE_VOD, which could provide additional protection from > unwanted usage. Again, make this dynamic and only happen for devices that know it will work on, based on some hardware information (device type of DT value). Do not make this happen based on a Kconfig option. thanks, greg k-h
On Tue, Jun 15, 2021 at 09:52:20AM +0100, Paul Cercueil wrote: > Hi Zhou, > > Le mar., juin 15 2021 at 16:16:39 +0800, 周琰杰 <zhouyanjie@wanyeetech.com> > a écrit : > > Hi Greg, > > > > Sorry for taking so long to reply. > > > > 于 Tue, 23 Mar 2021 16:31:29 +0100 > > Greg KH <gregkh@linuxfoundation.org> 写道: > > > > > On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou Yanjie) > > > wrote: > > > > Introduce configurable option for enabling GOTGCTL register > > > > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables > > > > VBUS overcurrent detection. > > > > > > > > This patch is derived from Dragan Čečavac (in the kernel 3.18 > > > > tree of CI20). It is very useful for the MIPS Creator CI20(r1). > > > > Without this patch, CI20's OTG port has a great probability to > > > > face overcurrent warning, which breaks the OTG functionality. > > > > > > > > Signed-off-by: 周琰杰 (Zhou Yanjie) <zhouyanjie@wanyeetech.com> > > > > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> > > > > --- > > > > drivers/usb/dwc2/Kconfig | 6 ++++++ > > > > drivers/usb/dwc2/core.c | 9 +++++++++ > > > > 2 files changed, 15 insertions(+) > > > > > > > > diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig > > > > index c131719..e40d187 100644 > > > > --- a/drivers/usb/dwc2/Kconfig > > > > +++ b/drivers/usb/dwc2/Kconfig > > > > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC > > > > non-periodic transfers, but of course the debug logs > > > > will be incomplete. Note that this also disables some debug > > > messages > > > > for which the transfer type cannot be deduced. > > > > + > > > > +config USB_DWC2_DISABLE_VOD > > > > + bool "Disable VBUS overcurrent detection" > > > > + help > > > > + Say Y here to switch off VBUS overcurrent detection. It > > > > enables USB > > > > + functionality blocked by overcurrent detection. > > > > > > Why would this be a configuration option? Shouldn't this be dynamic > > > and just work properly automatically? > > > > > > You should not have to do this on a build-time basis, it should be > > > able to be detected and handled properly at run-time for all > > > devices. > > > > > > > I consulted the original author Dragan Čečavac, he think since this is > > a feature which disables overcurrent detection, so we are not sure if > > it could be harmful for some devices. Therefore he advise against > > enabling it in runtime, and in favor that user explicitely has to > > enable it. > > This could still be enabled at runtime, though, via a module parameter. > Leave it enabled by default, and those who want to disable it can do it. This is not the 1990's, please NEVER add new module parameters, especially ones that are somehow supposed to be device-specific. Remember, module options are code-wide, not device-specific. Just do this based on the device type, or something else dynamic, do NOT make this be forced to be selected by a kernel configuration option or a random module option at runtime. thanks, greg k-h
Hi Greg, Le mar., juin 15 2021 at 11:48:30 +0200, Greg KH <gregkh@linuxfoundation.org> a écrit : > On Tue, Jun 15, 2021 at 09:52:20AM +0100, Paul Cercueil wrote: >> Hi Zhou, >> >> Le mar., juin 15 2021 at 16:16:39 +0800, 周琰杰 >> <zhouyanjie@wanyeetech.com> >> a écrit : >> > Hi Greg, >> > >> > Sorry for taking so long to reply. >> > >> > 于 Tue, 23 Mar 2021 16:31:29 +0100 >> > Greg KH <gregkh@linuxfoundation.org> 写道: >> > >> > > On Tue, Mar 23, 2021 at 11:24:26PM +0800, 周琰杰 (Zhou >> Yanjie) >> > > wrote: >> > > > Introduce configurable option for enabling GOTGCTL register >> > > > bits VbvalidOvEn and VbvalidOvVal. Once selected it disables >> > > > VBUS overcurrent detection. >> > > > >> > > > This patch is derived from Dragan Čečavac (in the kernel >> 3.18 >> > > > tree of CI20). It is very useful for the MIPS Creator >> CI20(r1). >> > > > Without this patch, CI20's OTG port has a great probability >> to >> > > > face overcurrent warning, which breaks the OTG functionality. >> > > > >> > > > Signed-off-by: 周琰杰 (Zhou Yanjie) >> <zhouyanjie@wanyeetech.com> >> > > > Signed-off-by: Dragan Čečavac <dragancecavac@yahoo.com> >> > > > --- >> > > > drivers/usb/dwc2/Kconfig | 6 ++++++ >> > > > drivers/usb/dwc2/core.c | 9 +++++++++ >> > > > 2 files changed, 15 insertions(+) >> > > > >> > > > diff --git a/drivers/usb/dwc2/Kconfig >> b/drivers/usb/dwc2/Kconfig >> > > > index c131719..e40d187 100644 >> > > > --- a/drivers/usb/dwc2/Kconfig >> > > > +++ b/drivers/usb/dwc2/Kconfig >> > > > @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC >> > > > non-periodic transfers, but of course the debug logs >> > > > will be incomplete. Note that this also disables some debug >> > > messages >> > > > for which the transfer type cannot be deduced. >> > > > + >> > > > +config USB_DWC2_DISABLE_VOD >> > > > + bool "Disable VBUS overcurrent detection" >> > > > + help >> > > > + Say Y here to switch off VBUS overcurrent detection. It >> > > > enables USB >> > > > + functionality blocked by overcurrent detection. >> > > >> > > Why would this be a configuration option? Shouldn't this be >> dynamic >> > > and just work properly automatically? >> > > >> > > You should not have to do this on a build-time basis, it >> should be >> > > able to be detected and handled properly at run-time for all >> > > devices. >> > > >> > >> > I consulted the original author Dragan Čečavac, he think since >> this is >> > a feature which disables overcurrent detection, so we are not >> sure if >> > it could be harmful for some devices. Therefore he advise against >> > enabling it in runtime, and in favor that user explicitely has to >> > enable it. >> >> This could still be enabled at runtime, though, via a module >> parameter. >> Leave it enabled by default, and those who want to disable it can >> do it. > > This is not the 1990's, please NEVER add new module parameters, First time I hear this. > especially ones that are somehow supposed to be device-specific. > > Remember, module options are code-wide, not device-specific. Right. I thought "just make the option available on devices that support it" but that's not how it works. -Paul > Just do this based on the device type, or something else dynamic, do > NOT > make this be forced to be selected by a kernel configuration option > or a > random module option at runtime. > > thanks, > > greg k-h
diff --git a/drivers/usb/dwc2/Kconfig b/drivers/usb/dwc2/Kconfig index c131719..e40d187 100644 --- a/drivers/usb/dwc2/Kconfig +++ b/drivers/usb/dwc2/Kconfig @@ -94,4 +94,10 @@ config USB_DWC2_DEBUG_PERIODIC non-periodic transfers, but of course the debug logs will be incomplete. Note that this also disables some debug messages for which the transfer type cannot be deduced. + +config USB_DWC2_DISABLE_VOD + bool "Disable VBUS overcurrent detection" + help + Say Y here to switch off VBUS overcurrent detection. It enables USB + functionality blocked by overcurrent detection. endif diff --git a/drivers/usb/dwc2/core.c b/drivers/usb/dwc2/core.c index fec17a2..c629dc97 100644 --- a/drivers/usb/dwc2/core.c +++ b/drivers/usb/dwc2/core.c @@ -1200,6 +1200,7 @@ static int dwc2_hs_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) { u32 usbcfg; + u32 otgctl; int retval = 0; if ((hsotg->params.speed == DWC2_SPEED_PARAM_FULL || @@ -1231,6 +1232,14 @@ int dwc2_phy_init(struct dwc2_hsotg *hsotg, bool select_phy) dwc2_writel(hsotg, usbcfg, GUSBCFG); } + if (IS_ENABLED(CONFIG_USB_DWC2_DISABLE_VOD)) { + if (dwc2_is_host_mode(hsotg)) { + otgctl = readl(hsotg->regs + GOTGCTL); + otgctl |= GOTGCTL_VBVALOEN | GOTGCTL_VBVALOVAL; + writel(otgctl, hsotg->regs + GOTGCTL); + } + } + return retval; }