| Message ID | 20240119213130.3147517-2-Frank.Li@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] dt-bindings: usb: dwc3: Add snps,host-vbus-glitches avoiding vbus glitch | expand |
On Fri, Jan 19, 2024, Frank Li wrote: > From: Ran Wang <ran.wang_1@nxp.com> > > When DWC3 is set to host mode by programming register DWC3_GCTL, VBUS > (or its control signal) will be turned on immediately on related Root Hub > ports. Then, the VBUS is turned off for a little while(15us) when do xhci > reset (conducted by xhci driver) and back to normal finally, we can > observe a negative glitch of related signal happen. > > This VBUS glitch might cause some USB devices enumeration fail if kernel > boot with them connected. Such as LS1012AFWRY/LS1043ARDB/LX2160AQDS > /LS1088ARDB with Kingston 16GB USB2.0/Kingston USB3.0/JetFlash Transcend > 4GB USB2.0 drives. The fail cases include enumerated as full-speed device > or report wrong device descriptor, etc. > > One SW workaround which can fix this is by programing all xhci PORTSC[PP] > to 0 to turn off VBUS immediately after setting host mode in DWC3 driver > (per signal measurement result, it will be too late to do it in > xhci-plat.c or xhci.c). Then, after xhci reset complete in xhci driver, > PORTSC[PP]s' value will back to 1 automatically and VBUS on at that time, > no glitch happen and normal enumeration process has no impact. > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com> > Reviewed-by: Peter Chen <peter.chen@nxp.com> > Signed-off-by: Frank Li <Frank.Li@nxp.com> > --- > > Notes: > Last review at June 06, 2019. Fixed all review comments and sent again. > > https://urldefense.com/v3/__https://lore.kernel.org/linux-kernel/AM5PR0402MB2865979E26017BC5937DBBA5F1170@AM5PR0402MB2865.eurprd04.prod.outlook.com/__;!!A4F2R9G_pg!bdutJWi1Tcz8SYscB7Mr96SWYMKIo8ElUKgEILFJfK3_60EbECQHXPBmJYAMMhNwQ4YrjxqMZWHdokXhHB6a$ > > drivers/usb/dwc3/core.c | 2 ++ > drivers/usb/dwc3/core.h | 2 ++ > drivers/usb/dwc3/host.c | 46 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 50 insertions(+) > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > index 3e55838c00014..a57adf0c11dd1 100644 > --- a/drivers/usb/dwc3/core.c > +++ b/drivers/usb/dwc3/core.c > @@ -1626,6 +1626,8 @@ static void dwc3_get_properties(struct dwc3 *dwc) > dwc->dis_split_quirk = device_property_read_bool(dev, > "snps,dis-split-quirk"); > > + dwc->host_vbus_glitches = device_property_read_bool(dev, "snps,host-vbus-glitches"); This is a quirk. The property should be named with "quirk" subfix. But do we need a new quirk? How many different platforms are affected? If it's just 1 or 2, then just use compatible string. > + > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > dwc->tx_de_emphasis = tx_de_emphasis; > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > index e3eea965e57bf..0269bacbbf6bd 100644 > --- a/drivers/usb/dwc3/core.h > +++ b/drivers/usb/dwc3/core.h > @@ -1135,6 +1135,7 @@ struct dwc3_scratchpad_array { > * @dis_split_quirk: set to disable split boundary. > * @wakeup_configured: set if the device is configured for remote wakeup. > * @suspended: set to track suspend event due to U3/L2. > + * @host_vbus_glitches: set to avoid vbus glitch during xhci reset. This is only applicable to the first xhci reset in its initialization and not every xhci reset right? If so, please reword. Also, please place it correspond to the order of the field. > * @imod_interval: set the interrupt moderation interval in 250ns > * increments or 0 to disable. > * @max_cfg_eps: current max number of IN eps used across all USB configs. > @@ -1353,6 +1354,7 @@ struct dwc3 { > unsigned tx_de_emphasis:2; > > unsigned dis_metastability_quirk:1; > + unsigned host_vbus_glitches:1; > > unsigned dis_split_quirk:1; > unsigned async_callbacks:1; > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > index 61f57fe5bb783..af8903ee37c20 100644 > --- a/drivers/usb/dwc3/host.c > +++ b/drivers/usb/dwc3/host.c > @@ -11,6 +11,7 @@ > #include <linux/of.h> > #include <linux/platform_device.h> > > +#include "../host/xhci.h" Let's not import the entire xhci.h. If we're going to share some macros from xhci.h, please split them from xhci.h and perhaps create xhci-ports.h for dwc3 to share. > #include "core.h" > > static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, > @@ -28,6 +29,44 @@ static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, > dwc->xhci_resources[1].name = name; > } > > +#define XHCI_HCSPARAMS1 0x4 > +#define XHCI_PORTSC_BASE 0x400 > + > +/* > + * dwc3_power_off_all_roothub_ports - Power off all Root hub ports > + * @dwc3: Pointer to our controller context structure > + */ > +static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc) > +{ > + int i, port_num; > + u32 reg, op_regs_base, offset; > + void __iomem *xhci_regs; > + > + /* xhci regs is not mapped yet, do it temperary here */ > + if (dwc->xhci_resources[0].start) { > + xhci_regs = ioremap(dwc->xhci_resources[0].start, > + DWC3_XHCI_REGS_END); > + if (IS_ERR(xhci_regs)) { > + dev_err(dwc->dev, "Failed to ioremap xhci_regs\n"); > + return; > + } > + > + op_regs_base = HC_LENGTH(readl(xhci_regs)); > + reg = readl(xhci_regs + XHCI_HCSPARAMS1); > + port_num = HCS_MAX_PORTS(reg); > + > + for (i = 1; i <= port_num; i++) { > + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); > + reg = readl(xhci_regs + offset); > + reg &= ~PORT_POWER; > + writel(reg, xhci_regs + offset); > + } > + > + iounmap(xhci_regs); > + } else > + dev_err(dwc->dev, "xhci base reg invalid\n"); > +} > + > static int dwc3_host_get_irq(struct dwc3 *dwc) > { > struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > @@ -66,6 +105,13 @@ int dwc3_host_init(struct dwc3 *dwc) > int ret, irq; > int prop_idx = 0; > > + /* > + * We have to power off all Root hub ports immediately after DWC3 set > + * to host mode to avoid VBUS glitch happen when xhci get reset later. > + */ > + if (dwc->host_vbus_glitches) > + dwc3_power_off_all_roothub_ports(dwc); > + It's part of the dwc3_host_init(), but don't do this in dwc3_host_get_irq(). Place it where it makes sense. > irq = dwc3_host_get_irq(dwc); > if (irq < 0) > return irq; > -- > 2.34.1 > Please run checkpatch.pl and fix them next time. Regarding PARENTHESIS_ALIGNMENT or line continuation, it can be two indentations or parenthesis aligned, whichever one makes it easier to read. From checkpatch: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis #119: FILE: drivers/usb/dwc3/host.c:48: + xhci_regs = ioremap(dwc->xhci_resources[0].start, + DWC3_XHCI_REGS_END); CHECK:SPACING: spaces preferred around that '*' (ctx:VxV) #130: FILE: drivers/usb/dwc3/host.c:59: + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); ^ CHECK:SPACING: spaces preferred around that '-' (ctx:VxV) #130: FILE: drivers/usb/dwc3/host.c:59: + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); ^ CHECK:BRACES: Unbalanced braces around else statement #137: FILE: drivers/usb/dwc3/host.c:66: + } else total: 0 errors, 0 warnings, 4 checks, 86 lines checked Thanks, Thinh
On Sat, Jan 20, 2024, Thinh Nguyen wrote: > > + > > static int dwc3_host_get_irq(struct dwc3 *dwc) > > { > > struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > > @@ -66,6 +105,13 @@ int dwc3_host_init(struct dwc3 *dwc) > > int ret, irq; > > int prop_idx = 0; > > > > + /* > > + * We have to power off all Root hub ports immediately after DWC3 set > > + * to host mode to avoid VBUS glitch happen when xhci get reset later. > > + */ > > + if (dwc->host_vbus_glitches) > > + dwc3_power_off_all_roothub_ports(dwc); > > + > > It's part of the dwc3_host_init(), but don't do this in > dwc3_host_get_irq(). Place it where it makes sense. Ignore this comment, I thought it's called from dwc3_host_get_irq(), but it's not. BR, Thinh > > > irq = dwc3_host_get_irq(dwc); > > if (irq < 0) > > return irq; > > -- > > 2.34.1 > > >
On Sat, Jan 20, 2024 at 12:51:07AM +0000, Thinh Nguyen wrote: > On Fri, Jan 19, 2024, Frank Li wrote: > > From: Ran Wang <ran.wang_1@nxp.com> > > > > When DWC3 is set to host mode by programming register DWC3_GCTL, VBUS > > (or its control signal) will be turned on immediately on related Root Hub > > ports. Then, the VBUS is turned off for a little while(15us) when do xhci > > reset (conducted by xhci driver) and back to normal finally, we can > > observe a negative glitch of related signal happen. > > > > This VBUS glitch might cause some USB devices enumeration fail if kernel > > boot with them connected. Such as LS1012AFWRY/LS1043ARDB/LX2160AQDS > > /LS1088ARDB with Kingston 16GB USB2.0/Kingston USB3.0/JetFlash Transcend > > 4GB USB2.0 drives. The fail cases include enumerated as full-speed device > > or report wrong device descriptor, etc. > > > > One SW workaround which can fix this is by programing all xhci PORTSC[PP] > > to 0 to turn off VBUS immediately after setting host mode in DWC3 driver > > (per signal measurement result, it will be too late to do it in > > xhci-plat.c or xhci.c). Then, after xhci reset complete in xhci driver, > > PORTSC[PP]s' value will back to 1 automatically and VBUS on at that time, > > no glitch happen and normal enumeration process has no impact. > > > > Signed-off-by: Ran Wang <ran.wang_1@nxp.com> > > Reviewed-by: Peter Chen <peter.chen@nxp.com> > > Signed-off-by: Frank Li <Frank.Li@nxp.com> > > --- > > > > Notes: > > Last review at June 06, 2019. Fixed all review comments and sent again. > > > > https://urldefense.com/v3/__https://lore.kernel.org/linux-kernel/AM5PR0402MB2865979E26017BC5937DBBA5F1170@AM5PR0402MB2865.eurprd04.prod.outlook.com/__;!!A4F2R9G_pg!bdutJWi1Tcz8SYscB7Mr96SWYMKIo8ElUKgEILFJfK3_60EbECQHXPBmJYAMMhNwQ4YrjxqMZWHdokXhHB6a$ > > > > drivers/usb/dwc3/core.c | 2 ++ > > drivers/usb/dwc3/core.h | 2 ++ > > drivers/usb/dwc3/host.c | 46 +++++++++++++++++++++++++++++++++++++++++ > > 3 files changed, 50 insertions(+) > > > > diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c > > index 3e55838c00014..a57adf0c11dd1 100644 > > --- a/drivers/usb/dwc3/core.c > > +++ b/drivers/usb/dwc3/core.c > > @@ -1626,6 +1626,8 @@ static void dwc3_get_properties(struct dwc3 *dwc) > > dwc->dis_split_quirk = device_property_read_bool(dev, > > "snps,dis-split-quirk"); > > > > + dwc->host_vbus_glitches = device_property_read_bool(dev, "snps,host-vbus-glitches"); > > This is a quirk. The property should be named with "quirk" subfix. > But do we need a new quirk? How many different platforms are affected? > If it's just 1 or 2, then just use compatible string. more than 2 platform. I think quirk is more flexible. Frank > > > + > > dwc->lpm_nyet_threshold = lpm_nyet_threshold; > > dwc->tx_de_emphasis = tx_de_emphasis; > > > > diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h > > index e3eea965e57bf..0269bacbbf6bd 100644 > > --- a/drivers/usb/dwc3/core.h > > +++ b/drivers/usb/dwc3/core.h > > @@ -1135,6 +1135,7 @@ struct dwc3_scratchpad_array { > > * @dis_split_quirk: set to disable split boundary. > > * @wakeup_configured: set if the device is configured for remote wakeup. > > * @suspended: set to track suspend event due to U3/L2. > > + * @host_vbus_glitches: set to avoid vbus glitch during xhci reset. > > This is only applicable to the first xhci reset in its initialization > and not every xhci reset right? If so, please reword. > > Also, please place it correspond to the order of the field. > > > * @imod_interval: set the interrupt moderation interval in 250ns > > * increments or 0 to disable. > > * @max_cfg_eps: current max number of IN eps used across all USB configs. > > @@ -1353,6 +1354,7 @@ struct dwc3 { > > unsigned tx_de_emphasis:2; > > > > unsigned dis_metastability_quirk:1; > > + unsigned host_vbus_glitches:1; > > > > unsigned dis_split_quirk:1; > > unsigned async_callbacks:1; > > diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c > > index 61f57fe5bb783..af8903ee37c20 100644 > > --- a/drivers/usb/dwc3/host.c > > +++ b/drivers/usb/dwc3/host.c > > @@ -11,6 +11,7 @@ > > #include <linux/of.h> > > #include <linux/platform_device.h> > > > > +#include "../host/xhci.h" > > Let's not import the entire xhci.h. If we're going to share some macros > from xhci.h, please split them from xhci.h and perhaps create > xhci-ports.h for dwc3 to share. > > > #include "core.h" > > > > static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, > > @@ -28,6 +29,44 @@ static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, > > dwc->xhci_resources[1].name = name; > > } > > > > +#define XHCI_HCSPARAMS1 0x4 > > +#define XHCI_PORTSC_BASE 0x400 > > + > > +/* > > + * dwc3_power_off_all_roothub_ports - Power off all Root hub ports > > + * @dwc3: Pointer to our controller context structure > > + */ > > +static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc) > > +{ > > + int i, port_num; > > + u32 reg, op_regs_base, offset; > > + void __iomem *xhci_regs; > > + > > + /* xhci regs is not mapped yet, do it temperary here */ > > + if (dwc->xhci_resources[0].start) { > > + xhci_regs = ioremap(dwc->xhci_resources[0].start, > > + DWC3_XHCI_REGS_END); > > + if (IS_ERR(xhci_regs)) { > > + dev_err(dwc->dev, "Failed to ioremap xhci_regs\n"); > > + return; > > + } > > + > > + op_regs_base = HC_LENGTH(readl(xhci_regs)); > > + reg = readl(xhci_regs + XHCI_HCSPARAMS1); > > + port_num = HCS_MAX_PORTS(reg); > > + > > + for (i = 1; i <= port_num; i++) { > > + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); > > + reg = readl(xhci_regs + offset); > > + reg &= ~PORT_POWER; > > + writel(reg, xhci_regs + offset); > > + } > > + > > + iounmap(xhci_regs); > > + } else > > + dev_err(dwc->dev, "xhci base reg invalid\n"); > > +} > > + > > static int dwc3_host_get_irq(struct dwc3 *dwc) > > { > > struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); > > @@ -66,6 +105,13 @@ int dwc3_host_init(struct dwc3 *dwc) > > int ret, irq; > > int prop_idx = 0; > > > > + /* > > + * We have to power off all Root hub ports immediately after DWC3 set > > + * to host mode to avoid VBUS glitch happen when xhci get reset later. > > + */ > > + if (dwc->host_vbus_glitches) > > + dwc3_power_off_all_roothub_ports(dwc); > > + > > It's part of the dwc3_host_init(), but don't do this in > dwc3_host_get_irq(). Place it where it makes sense. > > > irq = dwc3_host_get_irq(dwc); > > if (irq < 0) > > return irq; > > -- > > 2.34.1 > > > > Please run checkpatch.pl and fix them next time. Regarding > PARENTHESIS_ALIGNMENT or line continuation, it can be two indentations > or parenthesis aligned, whichever one makes it easier to read. > > > From checkpatch: > > CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis > #119: FILE: drivers/usb/dwc3/host.c:48: > + xhci_regs = ioremap(dwc->xhci_resources[0].start, > + DWC3_XHCI_REGS_END); > > CHECK:SPACING: spaces preferred around that '*' (ctx:VxV) > #130: FILE: drivers/usb/dwc3/host.c:59: > + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); > ^ > > CHECK:SPACING: spaces preferred around that '-' (ctx:VxV) > #130: FILE: drivers/usb/dwc3/host.c:59: > + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); > ^ > > CHECK:BRACES: Unbalanced braces around else statement > #137: FILE: drivers/usb/dwc3/host.c:66: > + } else > Sorry, I too trust original patch author. Will fix next version Frank > total: 0 errors, 0 warnings, 4 checks, 86 lines checked > > > Thanks, > Thinh
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 3e55838c00014..a57adf0c11dd1 100644 --- a/drivers/usb/dwc3/core.c +++ b/drivers/usb/dwc3/core.c @@ -1626,6 +1626,8 @@ static void dwc3_get_properties(struct dwc3 *dwc) dwc->dis_split_quirk = device_property_read_bool(dev, "snps,dis-split-quirk"); + dwc->host_vbus_glitches = device_property_read_bool(dev, "snps,host-vbus-glitches"); + dwc->lpm_nyet_threshold = lpm_nyet_threshold; dwc->tx_de_emphasis = tx_de_emphasis; diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index e3eea965e57bf..0269bacbbf6bd 100644 --- a/drivers/usb/dwc3/core.h +++ b/drivers/usb/dwc3/core.h @@ -1135,6 +1135,7 @@ struct dwc3_scratchpad_array { * @dis_split_quirk: set to disable split boundary. * @wakeup_configured: set if the device is configured for remote wakeup. * @suspended: set to track suspend event due to U3/L2. + * @host_vbus_glitches: set to avoid vbus glitch during xhci reset. * @imod_interval: set the interrupt moderation interval in 250ns * increments or 0 to disable. * @max_cfg_eps: current max number of IN eps used across all USB configs. @@ -1353,6 +1354,7 @@ struct dwc3 { unsigned tx_de_emphasis:2; unsigned dis_metastability_quirk:1; + unsigned host_vbus_glitches:1; unsigned dis_split_quirk:1; unsigned async_callbacks:1; diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c index 61f57fe5bb783..af8903ee37c20 100644 --- a/drivers/usb/dwc3/host.c +++ b/drivers/usb/dwc3/host.c @@ -11,6 +11,7 @@ #include <linux/of.h> #include <linux/platform_device.h> +#include "../host/xhci.h" #include "core.h" static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, @@ -28,6 +29,44 @@ static void dwc3_host_fill_xhci_irq_res(struct dwc3 *dwc, dwc->xhci_resources[1].name = name; } +#define XHCI_HCSPARAMS1 0x4 +#define XHCI_PORTSC_BASE 0x400 + +/* + * dwc3_power_off_all_roothub_ports - Power off all Root hub ports + * @dwc3: Pointer to our controller context structure + */ +static void dwc3_power_off_all_roothub_ports(struct dwc3 *dwc) +{ + int i, port_num; + u32 reg, op_regs_base, offset; + void __iomem *xhci_regs; + + /* xhci regs is not mapped yet, do it temperary here */ + if (dwc->xhci_resources[0].start) { + xhci_regs = ioremap(dwc->xhci_resources[0].start, + DWC3_XHCI_REGS_END); + if (IS_ERR(xhci_regs)) { + dev_err(dwc->dev, "Failed to ioremap xhci_regs\n"); + return; + } + + op_regs_base = HC_LENGTH(readl(xhci_regs)); + reg = readl(xhci_regs + XHCI_HCSPARAMS1); + port_num = HCS_MAX_PORTS(reg); + + for (i = 1; i <= port_num; i++) { + offset = op_regs_base + XHCI_PORTSC_BASE + 0x10*(i-1); + reg = readl(xhci_regs + offset); + reg &= ~PORT_POWER; + writel(reg, xhci_regs + offset); + } + + iounmap(xhci_regs); + } else + dev_err(dwc->dev, "xhci base reg invalid\n"); +} + static int dwc3_host_get_irq(struct dwc3 *dwc) { struct platform_device *dwc3_pdev = to_platform_device(dwc->dev); @@ -66,6 +105,13 @@ int dwc3_host_init(struct dwc3 *dwc) int ret, irq; int prop_idx = 0; + /* + * We have to power off all Root hub ports immediately after DWC3 set + * to host mode to avoid VBUS glitch happen when xhci get reset later. + */ + if (dwc->host_vbus_glitches) + dwc3_power_off_all_roothub_ports(dwc); + irq = dwc3_host_get_irq(dwc); if (irq < 0) return irq;