Re: [PATCH] Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
On Thu, Nov 15, 2018 at 03:16:04PM +0100, Nikolaj Fogh wrote: > On 11/15/18 9:24 AM, Johan Hovold wrote: > > On Tue, Nov 13, 2018 at 08:19:44PM +0100, Nikolaj Fogh wrote: > >> On 11/12/18 10:54 AM, Johan Hovold wrote: > >>> On Wed, Oct 31, 2018 at 09:16:48PM +0100, Nikolaj Fogh wrote: > I have experienced that the ftdi_sio driver gives less-than-optimal > baud rates as the driver truncates instead of rounds to nearest > during baud rate divisor calculation. > This patch improves on the baud rate generation. The generated baud > rate corresponds to the optimal baud rate achievable with the chip. > This is what the windows driver gives as well. > >>> How did you verify this? Did you trace and compare the divisors > >>> actually requested by the Windows driver, or did you measure the > >>> resulting rates using a scope? > >> I verified it by scope. Granted, I only verified it for one baud rate > >> (961200). Whether it gives the same as the Windows driver in general, > >> I'm not sure. However, I would think that rounding instead of flooring > >> would always yield the most accurate result. > > I'm not so sure in this case. The driver uses "sub-integer" divisors and > > looks like it depends on truncation rather than rounding. Some > > background here: > > > > > > https://www.ftdichip.com/Support/Knowledgebase/index.html?whatbaudratesareachieveabl.htm > I have had a closer look at this (and the driver code), and it seemsthat > each bit in the divisorcorresponds to 1/8th (0.125) in the calculation. > > It is shuffled around a bit in the code (for legacy reasons I expect), and > put in the higher order bits, but prior to that, I see no reason that > rounding should not be used instead of truncating. I don't see how it > "depends" on truncation. As I mentioned in my follow-up mail, I agree with that; your proposed change looks correct. > > If you want to change these calculations you need to make a stronger > > case for it and verify that we don't mess up some other rate > > inadvertently. > I have done a calculation which compares the error of the baud rate > calculation going all the way from 1 to 3MBaud where it can be seen that > the rounding (as expected) halves the maximum error. Whereas the old method > went up to 12% baud rate error, the new method reaches 6%, so the range > of baud rates where communication will be successful should increase. Also, > the new method is always better or as-accurate as the old. Excellent, thanks for confirming. Just mention something about that in the commit message too. > I guess that image attachments are not welcome in the mailing list, so > I will refrain from attaching it. Let me know if I should send it to > you. Sure, if you want too that'd be great. > I am using it in a system which uses a baud rate of 961200 (and not > the standard 921600). Here the old calculation gave an error of 4.03% > and the new gave 0.12% error. Also good to have in the commit message. > I will try to verify the numbers I have calculated with a logic analyzer to > make sure that it corresponds with the real world. I can also try to compare > it to the windows driver outputs. That would be really good, at least for a few rates. > As I only have a FT232RT (232bm) to test with, the patch should probably be > limited to the changes in the ftdi_232bm_baud_base_to_divisor() function. No, as long as you mention which device you used for testing, and the numbers for the other other types looks similar, I think we can go ahead and round those divisions too. Thanks for doing this! Johan
Re: Query on usb/core/devio.c
On Thu, 2018-11-15 at 09:56 -0500, Alan Stern wrote: > On Thu, 15 Nov 2018, Oliver Neukum wrote: > > > > > On Do, 2018-11-15 at 12:45 +, Mayuresh Kulkarni wrote: > > > > > > > > > Understood this for remote-wake part. > > > > > > But still unclear about step (1) for host-wake as below (please note, I am > > > refering to host-wake and remote-wake as per my previous comment) - > > > Pre-condition: device in suspend and link in L2. > > > Use-case: end user wants to sends a control message to device. > > > Assumption: end user is NOT doing any activity that causes remote-wake. > > That is just an assumption you cannot make. > > Users are devious creatures. If it is physically > > possible, you have to be able to deal with it. > That's right. But let's suppose that the user hasn't interacted with > the device. In that case it's simple: The app should submit an URB to > the device (or do pretty much any other ioctl to the usbfs device file > descriptor). When usbfs gets an ioctl, it will automatically wake up > the device by incrementing the runtime PM usage counter. > Thanks for the comments. Based on the info so far, attempting to summarize the probable solution, to ensure that I understand it clearly - Facts - 1. USBFS driver grabs a PM ref-count in .open and drops it in .close which means USB device cannot suspend untill user-space closes it (even if all interface drivers report "idle" to usb-core). 2. Since the .ioctl "knows" that .open has ensured to keep device active, it does not call PM runtime APIs. Proposal - 1. Add new ioctl: suspend & wait-for-resume 2. suspend ioctl: decrements PM ref count and return 3. wait-for-resume ioctl: wait for resume or timeout or signal 4. Modify .ioctl implementation to do PM runtime calls except for above "new" ioctl calls (so pm_runtime_get_sync -> ioctl -> response -> pm_runtime_put_sync). This also means, pm runtime get/put will be in both .open/.close. Use-case analysis - 1. Remote-wake: Due to device's remote wake, wait-for-resume will return successfully. The user space caller then need to queue a request to "know" the reason of remote-wake. 2. Host-wake: The user-space caller issues any ioctl supported by .ioctl method. Due to (4) above, the device will be resumed and the ioctl will be performed. For (2) in use-case analysis, the user-space caller's wait-for-resume will also return, but since it knows that it has initiated the ioctl, it may or may not decide to queue a request. Instead, when ioctl returns it can call wait-for- resume again. Am I getting in sync with your comments? What issue(s) you anticipate in above proposal due to inherent race condition between host and remote-wake? Based on my meagre understanding of usb-core, it feels like usb_lock_device/usb_unlock_device calls around remote-wake and usbfs ioctl should help with race condition, right? > Alan Stern
Re: Query on usb/core/devio.c
On Fri, 16 Nov 2018, Mayuresh Kulkarni wrote:
> Thanks for the comments. Based on the info so far, attempting to summarize the
> probable solution, to ensure that I understand it clearly -
>
> Facts -
> 1. USBFS driver grabs a PM ref-count in .open and drops it in .close which
> means
> USB device cannot suspend untill user-space closes it (even if all interface
> drivers report "idle" to usb-core).
> 2. Since the .ioctl "knows" that .open has ensured to keep device active, it
> does not call PM runtime APIs.
>
> Proposal -
> 1. Add new ioctl: suspend & wait-for-resume
> 2. suspend ioctl: decrements PM ref count and return
> 3. wait-for-resume ioctl: wait for resume or timeout or signal
Do you really want to have a timeout for this ioctl? Maybe it isn't
important -- I don't know.
> 4. Modify .ioctl implementation to do PM runtime calls except for above "new"
> ioctl calls (so pm_runtime_get_sync -> ioctl -> response ->
> pm_runtime_put_sync). This also means, pm runtime get/put will be in both
> .open/.close.
That's not exactly what I had in mind. Open will do:
ps->runtime_active = true;
The new suspend ioctl will do this:
if (ps->runtime_active) {
usb_autosuspend_device(ps->dev);
ps->runtime_active = false;
}
and the old ioctls (and close) will do this at the start:
if (!ps->runtime_active) {
if (usb_autoresume_device(ps->dev))
return -EIO;/* Could not resume */
ps->runtime_active = true;
}
This means that after any interaction with the device, you will have to
call the suspend ioctl again if you want the device to go back to
sleep.
> Use-case analysis -
> 1. Remote-wake: Due to device's remote wake, wait-for-resume will return
> successfully. The user space caller then need to queue a request to "know" the
> reason of remote-wake.
> 2. Host-wake: The user-space caller issues any ioctl supported by .ioctl
> method.
> Due to (4) above, the device will be resumed and the ioctl will be performed.
Correct.
> For (2) in use-case analysis, the user-space caller's wait-for-resume will
> also
> return, but since it knows that it has initiated the ioctl, it may or may not
> decide to queue a request. Instead, when ioctl returns it can call wait-for-
> resume again.
Yes. Of course, your app will have some way to check for user
interaction with the device. Doing these checks while the device is
suspended would be counter-productive, since the check itself would
wake up the device. So you will probably want to do a check as soon as
you know the device has woken up, regardless of the cause. If you
don't, you run the risk of not noticing a user interaction.
> Am I getting in sync with your comments?
>
> What issue(s) you anticipate in above proposal due to inherent race condition
> between host and remote-wake?
Only what I mentioned above, that your program should check for user
interaction whenever it knows the device has woken up.
> Based on my meagre understanding of usb-core, it feels
> like usb_lock_device/usb_unlock_device calls around remote-wake and usbfs
> ioctl
> should help with race condition, right?
No, they will not help. This is not a race between two different parts
of the kernel both trying to communicate with the device; it is a race
between the kernel and the user. usb_lock_device doesn't prevent the
user from interacting with the device. :-)
Alan Stern
[PATCH v2] USB: serial: ftdi_sio: Improve the accuracy of the baud rate generator by using round-to-closest instead of truncating when calculating baud rate divisors.
Improve baud-rate generation by using rounding-to-closest instead of
truncation in divisor calculation.
Results have been verified by logic analyzer on an FT232RT (232BM) chip.
The following table shows the wanted baud rate, the baud rate obtained
with the old method (truncation), with the new method (rounding) and the
baud rate generated by the windows 10 driver. The numbers in parentheses
is the error.
+- Wanted --+-- Old ---+-- New ---+-- Win ---+
| 9600 | 9600 (0.00%) | 9604 (0.05%) | 9605 (0.05%) |
| 19200 | 19200 (0.00%) | 19199 (0.01%) | 19198 (0.01%) |
| 38400 | 38395 (0.01%) | 38431 (0.08%) | 38394 (0.02%) |
| 57600 | 57725 (0.22%) | 57540 (0.10%) | 57673 (0.13%) |
| 115200 | 115307 (0.09%) | 115330 (0.11%) | 115320 (0.10%) |
| 921600 | 919963 (0.18%) | 920386 (0.13%) | 920810 (0.09%) |
| 961200 | 996512 (3.67%) | 956480 (0.49%) | 956937 (0.44%) |
+---+--+--+--+
The error due to noise in the measurements is in the order of a few
tenths of a %. As can be seen, the baud rate for 961200 is significantly
improved for some rates, and corresponds to the output given by the
windows driver.
The theoretical baud rate has been calculated for all baud rates from 1
to 3M, and as expected, the error is centered around 0, with a triangle
shape instead of a sawtooth, so the maximum error is decreased to half.
Signed-off-by: Nikolaj Fogh
---
drivers/usb/serial/ftdi_sio.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/serial/ftdi_sio.c b/drivers/usb/serial/ftdi_sio.c
index 609198d9594c..0edbd3427548 100644
--- a/drivers/usb/serial/ftdi_sio.c
+++ b/drivers/usb/serial/ftdi_sio.c
@@ -1130,7 +1130,7 @@ static unsigned short int
ftdi_232am_baud_base_to_divisor(int baud, int base)
{
unsigned short int divisor;
/* divisor shifted 3 bits to the left */
- int divisor3 = base / 2 / baud;
+ int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
if ((divisor3 & 0x7) == 7)
divisor3++; /* round x.7/8 up to x+1 */
divisor = divisor3 >> 3;
@@ -1156,7 +1156,7 @@ static u32 ftdi_232bm_baud_base_to_divisor(int
baud, int base)
static const unsigned char divfrac[8] = { 0, 3, 2, 4, 1, 5, 6, 7 };
u32 divisor;
/* divisor shifted 3 bits to the left */
- int divisor3 = base / 2 / baud;
+ int divisor3 = DIV_ROUND_CLOSEST(base, 2 * baud);
divisor = divisor3 >> 3;
divisor |= (u32)divfrac[divisor3 & 0x7] << 14;
/* Deal with special cases for highest baud rates. */
@@ -1179,7 +1179,7 @@ static u32 ftdi_2232h_baud_base_to_divisor(int
baud, int base)
int divisor3;
/* hi-speed baud rate is 10-bit sampling instead of 16-bit */
- divisor3 = base * 8 / (baud * 10);
+ divisor3 = DIV_ROUND_CLOSEST(base * 8, baud * 10);
divisor = divisor3 >> 3;
divisor |= (u32)divfrac[divisor3 & 0x7] << 14;
--
2.19.1
Re: 答复: make a confirm for [usb: dwc3: gadget: skip Set/Clear Halt when invalid]
Hi Felipe,
On 6/28/2018 11:24 PM, Felipe Balbi wrote:
> (no top-posting!!)
>
> liangshengjun writes:
>
>> Hi balbi,
>>
>> It means that the mainline keep checking stall status first before
>> handle clear-halt request? as usb spec, it's actually okay to send
>> Clear Halt at any time. But dwc3 core hanging with macOS adb
>> application, so I think there is other rootcase why dwc3 hanging , and
>> current patch just for avoid this case. Right?
> that's correct. There's another problem happening that causes dwc3 to
> hang. I guess I'll just revert that patch since it causes more problems
> than solve.
>
>> If someday WindonwPC/LINUX PC meet this case again liked my case,
>> would you plan to revert it ? or other plan ?
> I'll just revert it.
>
Do you plan to revert the patch ffb80fc672c3 ("usb: dwc3: gadget: skip
Set/Clear Halt when invalid") for the next release?
BR,
Thinh
[PATCH] xhci: workaround CSS timeout on AMD SNPS 3.0 xHC.
From: Sandeep Singh
Occasionally AMD SNPS 3.0 xHC does not respond to
CSS when set, also it does not flag anything on SRE and HCE
to point the internal xHC errors on USBSTS register. This stalls
the entire system wide suspend and there is no point in stalling
just because of xHC CSS is not responding.
To work around this problem, if the xHC does not flag
anything on SRE and HCE, we can skip the CSS
timeout and allow the system to continue the suspend. Once the
system resume happens we can internally reset the controller
using XHCI_RESET_ON_RESUME quirk.
Signed-off-by: Shyam Sundar S K
Signed-off-by: Sandeep Singh
cc: Nehal Shah
---
drivers/usb/host/xhci-pci.c | 4
drivers/usb/host/xhci.c | 25 +
drivers/usb/host/xhci.h | 1 +
3 files changed, 30 insertions(+)
diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index 01c5705..72493c4 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -139,6 +139,10 @@ static void xhci_pci_quirks(struct device *dev, struct
xhci_hcd *xhci)
pdev->device == 0x43bb))
xhci->quirks |= XHCI_SUSPEND_DELAY;
+ if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+ (pdev->device == 0x15e0 || pdev->device == 0x15e1))
+ xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
+
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 0420eef..965b503 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -970,6 +970,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
unsigned intdelay = XHCI_MAX_HALT_USEC;
struct usb_hcd *hcd = xhci_to_hcd(xhci);
u32 command;
+ u32 res;
if (!hcd->state)
return 0;
@@ -1025,10 +1026,32 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
writel(command, &xhci->op_regs->command);
if (xhci_handshake(&xhci->op_regs->status,
STS_SAVE, 0, 10 * 1000)) {
+ if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) {
+ /*
+ * AMD SNPS xHC 3.0 occasionally does not clear the
+ * SSS bit of USBSTS and when driver tries to poll
+ * to see if the xHC clears BIT(8) which never happens
+ * and driver assumes that controller is not responding
+ * and times out. To workaround this, its good to check
+ * if SRE and HCE bits are not set (as per xhci
+ * Section 5.4.2) and bypass the timeout.
+ */
+
+ res = readl(&xhci->op_regs->status);
+ if (res & STS_SAVE) {
+ if (((res & STS_SRE) == 0) &&
+ ((res & STS_HCE) == 0)) {
+ xhci->quirks |= XHCI_RESET_ON_RESUME;
+ goto complete_suspend;
+ }
+ }
+ }
+
xhci_warn(xhci, "WARN: xHC save state timeout\n");
spin_unlock_irq(&xhci->lock);
return -ETIMEDOUT;
}
+ complete_suspend:
spin_unlock_irq(&xhci->lock);
/*
@@ -1213,6 +1236,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
usb_hcd_poll_rh_status(xhci->shared_hcd);
set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
usb_hcd_poll_rh_status(hcd);
+ if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)
+ xhci->quirks &= ~XHCI_RESET_ON_RESUME;
return retval;
}
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index bf0b369..eb99782 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1849,6 +1849,7 @@ struct xhci_hcd {
#define XHCI_INTEL_USB_ROLE_SW BIT_ULL(31)
#define XHCI_ZERO_64B_REGS BIT_ULL(32)
#define XHCI_DEFAULT_PM_RUNTIME_ALLOW BIT_ULL(33)
+#define XHCI_SNPS_BROKEN_SUSPEND BIT_ULL(34)
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
--
2.7.4
Re: [PATCH] xhci: workaround CSS timeout on AMD SNPS 3.0 xHC.
Hi Sandeep,
> On Nov 16, 2018, at 16:21, Singh, Sandeep wrote:
>
> From: Sandeep Singh
>
> Occasionally AMD SNPS 3.0 xHC does not respond to
> CSS when set, also it does not flag anything on SRE and HCE
> to point the internal xHC errors on USBSTS register. This stalls
> the entire system wide suspend and there is no point in stalling
> just because of xHC CSS is not responding.
>
> To work around this problem, if the xHC does not flag
> anything on SRE and HCE, we can skip the CSS
> timeout and allow the system to continue the suspend. Once the
> system resume happens we can internally reset the controller
> using XHCI_RESET_ON_RESUME quirk.
What happens to the connected and suspended USB devices?
Do USB devices lose remote wakeup functionality when this happens?
>
> Signed-off-by: Shyam Sundar S K
> Signed-off-by: Sandeep Singh
> cc: Nehal Shah
> ---
> drivers/usb/host/xhci-pci.c | 4
> drivers/usb/host/xhci.c | 25 +
> drivers/usb/host/xhci.h | 1 +
> 3 files changed, 30 insertions(+)
>
> diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
> index 01c5705..72493c4 100644
> --- a/drivers/usb/host/xhci-pci.c
> +++ b/drivers/usb/host/xhci-pci.c
> @@ -139,6 +139,10 @@ static void xhci_pci_quirks(struct device *dev, struct
> xhci_hcd *xhci)
>pdev->device == 0x43bb))
> xhci->quirks |= XHCI_SUSPEND_DELAY;
>
> + if (pdev->vendor == PCI_VENDOR_ID_AMD &&
> + (pdev->device == 0x15e0 || pdev->device == 0x15e1))
> + xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
> +
> if (pdev->vendor == PCI_VENDOR_ID_AMD)
> xhci->quirks |= XHCI_TRUST_TX_LENGTH;
>
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 0420eef..965b503 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -970,6 +970,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
> unsigned intdelay = XHCI_MAX_HALT_USEC;
> struct usb_hcd *hcd = xhci_to_hcd(xhci);
> u32 command;
> + u32 res;
>
> if (!hcd->state)
> return 0;
> @@ -1025,10 +1026,32 @@ int xhci_suspend(struct xhci_hcd *xhci, bool
> do_wakeup)
> writel(command, &xhci->op_regs->command);
> if (xhci_handshake(&xhci->op_regs->status,
> STS_SAVE, 0, 10 * 1000)) {
> + if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) {
> +/*
> + * AMD SNPS xHC 3.0 occasionally does not clear the
> + * SSS bit of USBSTS and when driver tries to poll
> + * to see if the xHC clears BIT(8) which never happens
> + * and driver assumes that controller is not responding
> + * and times out. To workaround this, its good to check
> + * if SRE and HCE bits are not set (as per xhci
> + * Section 5.4.2) and bypass the timeout.
> + */
> +
> + res = readl(&xhci->op_regs->status);
> + if (res & STS_SAVE) {
> + if (((res & STS_SRE) == 0) &&
> + ((res & STS_HCE) == 0)) {
> + xhci->quirks |= XHCI_RESET_ON_RESUME;
> + goto complete_suspend;
> + }
> + }
Maybe merge the two “ifs”? There are no other conditions to handle.
Kai-Heng
> + }
> +
> xhci_warn(xhci, "WARN: xHC save state timeout\n");
> spin_unlock_irq(&xhci->lock);
> return -ETIMEDOUT;
> }
> + complete_suspend:
> spin_unlock_irq(&xhci->lock);
>
> /*
> @@ -1213,6 +1236,8 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
> usb_hcd_poll_rh_status(xhci->shared_hcd);
> set_bit(HCD_FLAG_POLL_RH, &hcd->flags);
> usb_hcd_poll_rh_status(hcd);
> + if (xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND)
> + xhci->quirks &= ~XHCI_RESET_ON_RESUME;
>
> return retval;
> }
> diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
> index bf0b369..eb99782 100644
> --- a/drivers/usb/host/xhci.h
> +++ b/drivers/usb/host/xhci.h
> @@ -1849,6 +1849,7 @@ struct xhci_hcd {
> #define XHCI_INTEL_USB_ROLE_SWBIT_ULL(31)
> #define XHCI_ZERO_64B_REGSBIT_ULL(32)
> #define XHCI_DEFAULT_PM_RUNTIME_ALLOW BIT_ULL(33)
> +#define XHCI_SNPS_BROKEN_SUSPEND BIT_ULL(34)
>
> unsigned intnum_active_eps;
> unsigned intlimit_active_eps;
> --
> 2.7.4
>
Re: [PATCH v2 net-next 06/21] net: usb: aqc111: Introduce link management
Hi Andrew, Florian,
>>>
>>> So the point is that MAC firmware is managing SERDES and system interface
>>> link.
>>
>> Linux can manage that SERDES link between the MAC and the PHY. There
>> are two ways this can go:
>>
>> 1) You use phylib. When the PHY reports link, the adjust_link callback
>> in the MAC is called. The phydev structure contains information about
>> how you should configure the SERDES, SGMII, 2500Base-X, 5000Base-X. It
>> works, but it is not so nice.
>>
>> 2) phylink gives you a much nicer API to do the same. Again, the PHY
>> reports the link is up. phylink will then tell the MAC how to
>> configure its end of the SERDES. The problem with phylink is that it
>> expects a DT building. You don't have that, since this is a USB
>> device. But you also don't need a lot of the features of phylink like
>> SFPs, the i2c bus for the SFPs, GPIOs etc. So it should not be to hard
>> to make this work without device tree.
>>
>> By using core linux code, we avoid bugs in firmware which nobody can
>> fix. The Linux core code should be well tested and supported, but
>> phylink is rather new, so might still have some corner cases.
>>
>> I also cannot imaging parts of the PHY driver will not be re-usable
>> for other Aquantia PHYs. I have a board with an AQCS109 under my desk
>> waiting for me to do something with it. I really would like a better
>> PHY driver for it than the kernel currently has. Hopefully there is
>> some code reuse possibilities here.
>
> Even if the firmware is helping, there is still value in using PHYLINK
> to report things to Linux as Andrew is saying in that you get a lot of
> things for free: auto-negotiation results, link_ksetttings_{get,set} etc.
Thanks for the detailed explanation, I agree separating phy logic would
really improve things.
But just in time we've got a new updates from HW management/system teams,
It looks like they are deprecating direct phy access firmware mode,
so the only thing we have to support in this driver is FW based interface
for the link management.
Production dongles will always have firmware fully controlling all the phy.
Thus, I think in next series we'll just cut off all the direct phy
access code.
I agree with Andrew we eventually need a better generic Aquantia PHYs support
in linux, hope to look into this matter soon.
Regards,
Igor
Re: [PATCH] USB: serial: mos7840: Add a product ID for the new product
On Fri, Nov 16, 2018 at 02:36:56PM +0800, JackyChou wrote:
> From: JackyChou
>
> Add a new PID 0x7843 to the driver.
> Let the new products be able to set up 3 serial ports with the driver.
>
> Because the development of new product is based on 4 serial ports,
> but some users only need 3 serial ports. There is no way to set it from
> the hardware, so let the driver set 3 serial ports with PID 0x7843.
>
> Signed-off-by: JackyChou
> ---
Thanks for the update.
Always include a short changelog here so we know what has changed since
earlier versions.
Also include the patch revision in the Subject (i.e. "[PATCH v2] USB: ...").
> drivers/usb/serial/mos7840.c | 45 ++--
> 1 file changed, 22 insertions(+), 23 deletions(-)
>
> diff --git a/drivers/usb/serial/mos7840.c b/drivers/usb/serial/mos7840.c
> index b42bad85097a..57ef25a2f7bb 100644
> --- a/drivers/usb/serial/mos7840.c
> +++ b/drivers/usb/serial/mos7840.c
> @@ -94,6 +94,7 @@
> /* The native mos7840/7820 component */
> #define USB_VENDOR_ID_MOSCHIP 0x9710
> #define MOSCHIP_DEVICE_ID_7840 0x7840
> +#define MOSCHIP_DEVICE_ID_7843 0x7843
> #define MOSCHIP_DEVICE_ID_7820 0x7820
> #define MOSCHIP_DEVICE_ID_7810 0x7810
> /* The native component can have its vendor/device id's overridden
> @@ -176,6 +177,7 @@ enum mos7840_flag {
>
> static const struct usb_device_id id_table[] = {
> {USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7840)},
> + {USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7843)},
> {USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7820)},
> {USB_DEVICE(USB_VENDOR_ID_MOSCHIP, MOSCHIP_DEVICE_ID_7810)},
> {USB_DEVICE(USB_VENDOR_ID_BANDB, BANDB_DEVICE_ID_USO9ML2_2)},
> @@ -298,15 +300,10 @@ static int mos7840_set_uart_reg(struct usb_serial_port
> *port, __u16 reg,
> val = val & 0x00ff;
> /* For the UART control registers, the application number need
> to be Or'ed */
> - if (port->serial->num_ports == 4) {
> + if (port->serial->num_ports == 2 && port->port_number != 0)
> + val |= ((__u16)port->port_number + 2) << 8;
> + else
> val |= ((__u16)port->port_number + 1) << 8;
> - } else {
> - if (port->port_number == 0) {
> - val |= ((__u16)port->port_number + 1) << 8;
> - } else {
> - val |= ((__u16)port->port_number + 2) << 8;
> - }
> - }
This looks good, but please to this in a separate preparatory patch as
this is an independent change from adding support for you new device.
> dev_dbg(&port->dev, "%s application number is %x\n", __func__, val);
> return usb_control_msg(dev, usb_sndctrlpipe(dev, 0), MCS_WRREQ,
> MCS_WR_RTYPE, val, reg, NULL, 0,
> @@ -332,15 +329,10 @@ static int mos7840_get_uart_reg(struct usb_serial_port
> *port, __u16 reg,
> return -ENOMEM;
>
> /* Wval is same as application number */
> - if (port->serial->num_ports == 4) {
> + if (port->serial->num_ports == 2 && port->port_number != 0)
> + Wval = ((__u16)port->port_number + 2) << 8;
> + else
> Wval = ((__u16)port->port_number + 1) << 8;
> - } else {
> - if (port->port_number == 0) {
> - Wval = ((__u16)port->port_number + 1) << 8;
> - } else {
> - Wval = ((__u16)port->port_number + 2) << 8;
> - }
> - }
Same here.
> dev_dbg(&port->dev, "%s application number is %x\n", __func__,
> Wval);
> ret = usb_control_msg(dev, usb_rcvctrlpipe(dev, 0), MCS_RDREQ,
> MCS_RD_RTYPE, Wval, reg, buf,
> VENDOR_READ_LENGTH,
> @@ -2071,12 +2063,16 @@ static int mos7840_probe(struct usb_serial *serial,
> VENDOR_READ_LENGTH, MOS_WDR_TIMEOUT);
>
> /* For a MCS7840 device GPIO0 must be set to 1 */
> - if (buf[0] & 0x01)
> - device_type = MOSCHIP_DEVICE_ID_7840;
> - else if (mos7810_check(serial))
> + if (buf[0] & 0x01) {
> + if (product == MOSCHIP_DEVICE_ID_7843)
> + device_type = MOSCHIP_DEVICE_ID_7843;
And as I mentioned in my previous comments, if you're going to match in
PID, there's no need to check GPIO0. Just add code to handle 7843 to the
start of the function.
> + else
> + device_type = MOSCHIP_DEVICE_ID_7840;
> + } else if (mos7810_check(serial)) {
> device_type = MOSCHIP_DEVICE_ID_7810;
> - else
> + } else {
> device_type = MOSCHIP_DEVICE_ID_7820;
> + }
>
> kfree(buf);
> out:
> @@ -2091,7 +2087,10 @@ static int mos7840_calc_num_ports(struct usb_serial
> *serial,
> int device_type = (unsigned long)usb_get_serial_data(serial);
> int num_ports;
>
> - num_ports = (device_type >> 4) & 0x000F;
> + if (device_ty
Re: SMSC95xx driver updates (round 1)
On 15/11/18 18:06, woojung@microchip.com wrote: Hi Ben, -Original Message- From: netdev-ow...@vger.kernel.org On Behalf Of Ben Dooks Sent: Wednesday, November 14, 2018 6:50 AM To: net...@vger.kernel.org Cc: oneu...@suse.com; da...@davemloft.net; linux-usb@vger.kernel.org; linux- ker...@vger.kernel.org; steve.glendinn...@shawell.net; linux-ker...@lists.codethink.co.uk Subject: SMSC95xx driver updates (round 1) This is a series of a few driver cleanups and some fixups of the code for the SMSC95XX driver. There have been a few reviews, and the issues have been fixed so this should be ready for merging. I will work on the tx-alignment and the other bits of usbnet changes and produce at least two more patch series for this later. Some reason, maintainer email of unglinuxdri...@microchip.com is NOT included in CC. Please add it in following patch series you are creating to have a chance to review by proper reviewers. USB SMSC95XX ETHERNET DRIVER M: Steve Glendinning M: Microchip Linux Driver Support L: net...@vger.kernel.org S: Maintained F: drivers/net/usb/smsc95xx.* Sorry, must have missed this when rebasing from v4.18 to v4.19. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
Re: [PATCH] usb: hub: add I/O error retry & reset routine
Hi Alan,
thanks for the review.
On Thu, 2018-11-15 at 14:24 -0500, Alan Stern wrote:
> On Thu, 15 Nov 2018, Nicolas Saenz Julienne wrote:
>
> > An URB submission error in the HUB's endpoint completion function
> > renders the whole HUB device unresponsive. This patch introduces a
> > routine that retries the submission for 1s to then, as a last
> resort,
> > reset the whole device.
> >
> > The implementation is based on usbhid/hid_core.c's, which
> implements the
> > same functionality. It was tested with the help of BCC's error
> injection
> > tool (inject.py).
> >
> > Signed-off-by: Nicolas Saenz Julienne
>
> Why do you think this is needed? Are you experiencing these
> sorts of URB submission errors?
Sorry, I should have been more explicit on where I come from. I've been
playing around injecting atomic allocation errors on the USB stack. For
example any URB submission marked with GFP_ATOMIC that ends up into
xhci will allocate some memory with that flag.
Most subsystems, after facing a burst of memory allocation failures,
seem to recover well (usbnet/hid/uas/alsa/serial). But I found out that
it's not the case for USB hubs: In the event of detecting a new device
the hub will complete an int URB which was previously sent to it. The
event data is saved by the host and the URB is resubmitted for further
event passing. In the case that URB submission failed, we lose all
further events.
It is indeed pretty hard to find this issue in the wild, since you have
to time plugging or unplugging an USB device with the system running
out of memory. But I don't think it's unrealistic to think it might
happen.
As I comment in the patch description, I'm injecting the errors using
BCC and eBPF's function override capabilities.
>
> Why do you handle only errors during submission but not during
> completion? And if you keep on getting errors during submission, why
> would resetting the hub make any difference?
Well, as far as I know, errors during completion are handled. The error
is marked in hub->error, which later-on, in hub-event(), triggers a
device reset.
While implementing the solution I took into account the hub's
completion error processing behavior and HID's implementation of the
submission error handling (see hid_irq_in() in usbhid/hid-core.c). My
rationale was that since both HID and hub are USB devices with a
similar behavior there was no point in reinventing a mechanism. That
said I have no spec data to back the "1s retry window to then reset the
device".
One could argue that in the event of an error having a timer running
forever is not the best design. It has to stop sometime. If that's the
case, the HUB will be in a unknown state, i.e. a device might have
disappeared. Resetting the hub will at least unbind all the USB devices
attached to it and retry the enumeration. Regardless of the
enumeration's success we'll at least be in a "safe" state.
>
> The patch doesn't delete the io_retry timer when the hub is removed.
Right, that was silly of me...
Kind Regards,
Nicolas
>
> Alan Stern
>
>
> > ---
> > drivers/usb/core/hub.c | 43
> +-
> > drivers/usb/core/hub.h | 3 +++
> > 2 files changed, 45 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index d9bd7576786a..1447bdba59ec 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -607,6 +607,44 @@ static int hub_port_status(struct usb_hub
> *hub, int port1,
> > status, change, NULL);
> > }
> >
> > +static void hub_io_error(struct usb_hub *hub)
> > +{
> > + /*
> > + * If it has been a while since the last error, we'll assume
> > + * this a brand new error and reset the retry timeout.
> > + */
> > + if (time_after(jiffies, hub->stop_retry + HZ/2))
> > + hub->retry_delay = 0;
> > +
> > + /* When an error occurs, retry at increasing intervals */
> > + if (hub->retry_delay == 0) {
> > + hub->retry_delay = 13; /* Then 26, 52, 104, 104, ...
> */
> > + hub->stop_retry = jiffies + msecs_to_jiffies(1000);
> > + } else if (hub->retry_delay < 100)
> > + hub->retry_delay *= 2;
> > +
> > + if (time_after(jiffies, hub->stop_retry)) {
> > + /* Retries failed, so do a port reset */
> > + usb_queue_reset_device(to_usb_interface(hub-
> >intfdev));
> > + return;
> > + }
> > +
> > + mod_timer(&hub->io_retry,
> > + jiffies + msecs_to_jiffies(hub-
> >retry_delay));
> > +}
> > +
> > +static void hub_retry_timeout(struct timer_list *t)
> > +{
> > + struct usb_hub *hub = from_timer(hub, t, io_retry);
> > +
> > + if (hub->disconnected || hub->quiescing)
> > + return;
> > +
> > + dev_err(hub->intfdev, "retrying int urb\n");
> > + if (usb_submit_urb(hub->urb, GFP_ATOMIC))
> > + hub_io_error(hub);
> > +}
> > +
> > static
Re: [PATCH] USB: host: ehci: allow tine of highspeed nak-count
On 14/11/18 18:47, Alan Stern wrote: On Wed, 14 Nov 2018, Ben Dooks wrote: From: Ben Dooks At least some systems benefit with less scheduling if the NAK count value is set higher than the default 4. For instance a Tegra3 with an SMSC9512 showed less interrupt load when this was changed to 14. Interesting. Why do you think this is? In theory, increasing the NAK count RL value should cause a higher memory bus load and perhaps a slight rise in the interrupt load (transfers will complete a little more quickly because the controller tries harder to poll the endpoints and see if they are ready). I thought the NAK counter was decremented until the transfer is given up on. I'm going to have to go back and get some actual figures from a running system as this was originally done over a year ago with the SMSC9512 (IIRC) network driver. To allow the changing of this value, add a sysfs node to each of the controllers to allow run-time changing. Signed-off-by: Ben Dooks The patch looks okay to me. I'll look at getting some tracing from the SMSC driver to see what is going on. -- Ben Dooks http://www.codethink.co.uk/ Senior Engineer Codethink - Providing Genius https://www.codethink.co.uk/privacy.html
Re: [PATCH] USB: host: ehci: allow tine of highspeed nak-count
On Fri, 16 Nov 2018, Ben Dooks wrote: > On 14/11/18 18:47, Alan Stern wrote: > > On Wed, 14 Nov 2018, Ben Dooks wrote: > > > >> From: Ben Dooks > >> > >> At least some systems benefit with less scheduling if the NAK count > >> value is set higher than the default 4. For instance a Tegra3 with > >> an SMSC9512 showed less interrupt load when this was changed to 14. > > > > Interesting. Why do you think this is? In theory, increasing the NAK > > count RL value should cause a higher memory bus load and perhaps a > > slight rise in the interrupt load (transfers will complete a little > > more quickly because the controller tries harder to poll the endpoints > > and see if they are ready). > > I thought the NAK counter was decremented until the transfer is given > up on. That's right. So if the RL value is higher, there will be more polling attempts in quick succession before the NAK counter drops to 0 and the controller gives up. More polling attempts in quick succession means heavier memory bus usage. > I'm going to have to go back and get some actual figures from > a running system as this was originally done over a year ago with the > SMSC9512 (IIRC) network driver. > > >> To allow the changing of this value, add a sysfs node to each of > >> the controllers to allow run-time changing. > >> > >> Signed-off-by: Ben Dooks > > > > The patch looks okay to me. > > I'll look at getting some tracing from the SMSC driver to see what > is going on. Okay. Should we consider the patch to be held in suspense until then? Alan Stern
[PATCH] usbnet: use tasklet_init() for usbnet_bh handler
The tasklet initialisation would be better done by tasklet_init() instead of assuming all the fields are in an ok state by default. Note, this does not fix any known bug. Signed-off-by: Ben Dooks --- drivers/net/usb/usbnet.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/net/usb/usbnet.c b/drivers/net/usb/usbnet.c index 28d76c827e70..8f7db959d319 100644 --- a/drivers/net/usb/usbnet.c +++ b/drivers/net/usb/usbnet.c @@ -1704,8 +1704,7 @@ usbnet_probe (struct usb_interface *udev, const struct usb_device_id *prod) skb_queue_head_init (&dev->txq); skb_queue_head_init (&dev->done); skb_queue_head_init(&dev->rxq_pause); - dev->bh.func = (void (*)(unsigned long))usbnet_bh; - dev->bh.data = (unsigned long)&dev->delay; + tasklet_init(&dev->bh, (void (*)(unsigned long))usbnet_bh, (unsigned long)&dev->delay); INIT_WORK (&dev->kevent, usbnet_deferred_kevent); init_usb_anchor(&dev->deferred); timer_setup(&dev->delay, usbnet_bh, 0); -- 2.19.1
Re: [PATCH] usb: hub: add I/O error retry & reset routine
On Fri, 16 Nov 2018, Nicolas Saenz Julienne wrote: > Hi Alan, > thanks for the review. > > On Thu, 2018-11-15 at 14:24 -0500, Alan Stern wrote: > > On Thu, 15 Nov 2018, Nicolas Saenz Julienne wrote: > > > > > An URB submission error in the HUB's endpoint completion function > > > renders the whole HUB device unresponsive. This patch introduces a > > > routine that retries the submission for 1s to then, as a last > > resort, > > > reset the whole device. > > > > > > The implementation is based on usbhid/hid_core.c's, which > > implements the > > > same functionality. It was tested with the help of BCC's error > > injection > > > tool (inject.py). > > > > > > Signed-off-by: Nicolas Saenz Julienne > > > > Why do you think this is needed? Are you experiencing these > > sorts of URB submission errors? > > Sorry, I should have been more explicit on where I come from. I've been > playing around injecting atomic allocation errors on the USB stack. For > example any URB submission marked with GFP_ATOMIC that ends up into > xhci will allocate some memory with that flag. > > Most subsystems, after facing a burst of memory allocation failures, > seem to recover well (usbnet/hid/uas/alsa/serial). But I found out that > it's not the case for USB hubs: In the event of detecting a new device > the hub will complete an int URB which was previously sent to it. The > event data is saved by the host and the URB is resubmitted for further > event passing. In the case that URB submission failed, we lose all > further events. Ah, I see. > It is indeed pretty hard to find this issue in the wild, since you have > to time plugging or unplugging an USB device with the system running > out of memory. But I don't think it's unrealistic to think it might > happen. > > As I comment in the patch description, I'm injecting the errors using > BCC and eBPF's function override capabilities. > > > > > Why do you handle only errors during submission but not during > > completion? And if you keep on getting errors during submission, why > > would resetting the hub make any difference? > > Well, as far as I know, errors during completion are handled. The error > is marked in hub->error, which later-on, in hub-event(), triggers a > device reset. > > While implementing the solution I took into account the hub's > completion error processing behavior and HID's implementation of the > submission error handling (see hid_irq_in() in usbhid/hid-core.c). My > rationale was that since both HID and hub are USB devices with a > similar behavior there was no point in reinventing a mechanism. That > said I have no spec data to back the "1s retry window to then reset the > device". > > One could argue that in the event of an error having a timer running > forever is not the best design. It has to stop sometime. If that's the > case, the HUB will be in a unknown state, i.e. a device might have > disappeared. Resetting the hub will at least unbind all the USB devices > attached to it and retry the enumeration. Regardless of the > enumeration's success we'll at least be in a "safe" state. Well, the timer will get deleted when the hub is unplugged. If that doesn't happen, we can assume that the hub has retained its state and so a reset isn't necessary. I would just keep retrying at, say, 1-second intervals. Don't even bother with the exponential slowdown that the HID driver does. > > The patch doesn't delete the io_retry timer when the hub is removed. > > Right, that was silly of me... You could even rename the timer to something like irq_urb_retry; I think that would be a more accurate description of what it does. So fix those things up, simplify the retries, and expand the patch description -- then resubmit. :-) Alan Stern
Re: [PATCH v2 RESEND] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume
Hi, On 15.11.2018 10:09, Felipe Balbi wrote: > > Hi, > > Todor Tomov writes: > >> Hello, >> >> After I apply this patch on 4.14 (or receive it with 4.14.70) I see a >> regression with >> the Qualcomm QUSB2 phy driver. I'm testing on a Dragonboard 820c. >> In boot log I get: >> >> [4.525502] phy phy-7412000.phy.6: QUSB2PHY pll lock failed: status reg = >> 0 >> [4.529232] phy phy-7412000.phy.6: phy init failed --> -16 >> [4.536170] dwc3 760.dwc3: failed to initialize core >> [4.541743] dwc3: probe of 760.dwc3 failed with error -16 >> [4.549979] phy phy-7411000.phy.5: QUSB2PHY pll lock failed: status reg = >> 0 >> [4.552843] phy phy-7411000.phy.5: phy init failed --> -16 >> [4.559606] dwc3 6a0.dwc3: failed to initialize core >> [4.565181] dwc3: probe of 6a0.dwc3 failed with error -16 >> >> I can provide a full log if needed. > > please do. Also, try mainline and let us know if the problem This is the full log on 4.14.69 + this patch: https://paste.ubuntu.com/p/N5WdHw47QC/ I also managed to get a log from 4.20.0-rc2 and I see the same error: https://paste.ubuntu.com/p/jz6fvYyZh6/ > persists. Why do you get -EBUSY from the PHY driver? Maybe I could have proposed a fix if I knew but I don't know. Best regards, Todor > > - > balbi >
Re: [PATCH v2 net-next 06/21] net: usb: aqc111: Introduce link management
> Production dongles will always have firmware fully controlling all the phy. > Thus, I think in next series we'll just cut off all the direct phy > access code. O.K, but that is also a shame. The PHY i have has all sorts of nice things, MACSEC, temperature sensors, PTP, cable tests logic, etc. Without having a proper PHY driver which can access the registers, a lot of that is going to be very hard to use. Andrew
[PATCH 4/5] Bluetooth: btusb: Collect the common Intel assignments together
The BTUSB_INTEL and BTUSB_INTEL_NEW have common functions & quirks are
assigned to hdev structure. Lets collect them together instead of
repeating them in different code branches.
Signed-off-by: Rajat Jain
---
drivers/bluetooth/btusb.c | 27 ---
1 file changed, 12 insertions(+), 15 deletions(-)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index 7439a7eb50ac..e8e148480c91 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -3077,28 +3077,25 @@ static int btusb_probe(struct usb_interface *intf,
data->diag = usb_ifnum_to_if(data->udev, ifnum_base + 2);
}
#endif
+ if (id->driver_info & BTUSB_INTEL ||
+ id->driver_info & BTUSB_INTEL_NEW) {
- if (id->driver_info & BTUSB_INTEL) {
hdev->manufacturer = 2;
- hdev->setup = btusb_setup_intel;
- hdev->shutdown = btusb_shutdown_intel;
- hdev->set_diag = btintel_set_diag_mfg;
hdev->set_bdaddr = btintel_set_bdaddr;
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
- }
- if (id->driver_info & BTUSB_INTEL_NEW) {
- hdev->manufacturer = 2;
- hdev->send = btusb_send_frame_intel;
- hdev->setup = btusb_setup_intel_new;
- hdev->hw_error = btintel_hw_error;
- hdev->set_diag = btintel_set_diag;
- hdev->set_bdaddr = btintel_set_bdaddr;
- set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
- set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
- set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
+ if (id->driver_info & BTUSB_INTEL) {
+ hdev->setup = btusb_setup_intel;
+ hdev->shutdown = btusb_shutdown_intel;
+ hdev->set_diag = btintel_set_diag_mfg;
+ } else {
+ hdev->send = btusb_send_frame_intel;
+ hdev->setup = btusb_setup_intel_new;
+ hdev->hw_error = btintel_hw_error;
+ hdev->set_diag = btintel_set_diag;
+ }
}
if (id->driver_info & BTUSB_MARVELL)
--
2.19.1.1215.g8438c0b245-goog
[PATCH 2/5] usb: assign ACPI companions for embedded USB devices
From: Dmitry Torokhov
USB devices permanently connected to USB ports may be described in ACPI
tables and share ACPI devices with ports they are connected to. See [1]
for details.
This will allow us to describe sideband resources for devices, such as,
for example, hard reset line for BT USB controllers.
[1]
https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/other-acpi-namespace-objects#acpi-namespace-hierarchy-and-adr-for-embedded-usb-devices
Signed-off-by: Dmitry Torokhov
Signed-off-by: Rajat Jain (changed how we get the usb_port)
---
drivers/usb/core/usb-acpi.c | 44 +
1 file changed, 35 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index 8ff73c83e8e8..9043d7242d67 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -200,30 +200,56 @@ static struct acpi_device *
usb_acpi_find_companion_for_device(struct usb_device *udev)
{
struct acpi_device *adev;
+ struct usb_port *port_dev;
+ struct usb_hub *hub;
+
+ if (!udev->parent) {
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+ }
- if (!udev->parent)
+ hub = usb_hub_to_struct_hub(udev->parent);
+ if (!hub)
return NULL;
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(udev->dev.parent);
- return acpi_find_child_device(adev, 0, false);
+ /*
+* This is an embedded USB device connected to a port and such
+* devices share port's ACPI companion.
+*/
+ port_dev = hub->ports[udev->portnum - 1];
+ return usb_acpi_get_companion_for_port(port_dev);
}
-
static struct acpi_device *usb_acpi_find_companion(struct device *dev)
{
/*
-* In the ACPI DSDT table, only usb root hub and usb ports are
-* acpi device nodes. The hierarchy like following.
+* The USB hierarchy like following:
+*
* Device (EHC1)
* Device (HUBN)
* Device (PR01)
* Device (PR11)
* Device (PR12)
+* Device (FN12)
+* Device (FN13)
* Device (PR13)
* ...
-* So all binding process is divided into two parts. binding
-* root hub and usb ports.
+* where HUBN is root hub, and PRNN are USB ports and devices
+* connected to them, and FNNN are individualk functions for
+* connected composite USB devices. PRNN and FNNN may contain
+* _CRS and other methods describing sideband resources for
+* the connected device.
+*
+* On the kernel side both root hub and embedded USB devices are
+* represented as instances of usb_device structure, and ports
+* are represented as usb_port structures, so the whole process
+* is split into 2 parts: finding companions for devices and
+* finding companions for ports.
+*
+* Note that we do not handle individual functions of composite
+* devices yet, for that we would need to assign companions to
+* devices corresponding to USB interfaces.
*/
if (is_usb_device(dev))
return usb_acpi_find_companion_for_device(to_usb_device(dev));
--
2.19.1.1215.g8438c0b245-goog
[PATCH 5/5] Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip
If the platform provides it, use the reset gpio to reset the BT
chip (requested by the HCI core if needed). This has been found helpful
on some of Intel bluetooth controllers where the firmware gets stuck and
the only way out is a hard reset pin provided by the platform.
Signed-off-by: Rajat Jain
---
drivers/bluetooth/btusb.c | 36
1 file changed, 36 insertions(+)
diff --git a/drivers/bluetooth/btusb.c b/drivers/bluetooth/btusb.c
index e8e148480c91..8aad02d9e211 100644
--- a/drivers/bluetooth/btusb.c
+++ b/drivers/bluetooth/btusb.c
@@ -29,6 +29,7 @@
#include
#include
#include
+#include
#include
#include
@@ -475,6 +476,8 @@ struct btusb_data {
struct usb_endpoint_descriptor *diag_tx_ep;
struct usb_endpoint_descriptor *diag_rx_ep;
+ struct gpio_desc *reset_gpio;
+
__u8 cmdreq_type;
__u8 cmdreq;
@@ -490,6 +493,28 @@ struct btusb_data {
int oob_wake_irq; /* irq for out-of-band wake-on-bt */
};
+
+static void btusb_hw_reset(struct hci_dev *hdev)
+{
+ struct btusb_data *data = hci_get_drvdata(hdev);
+ struct gpio_desc *reset_gpio = data->reset_gpio;
+
+ /*
+* Toggle the hard reset line if the platform provides one. The reset
+* is going to yank the device off the USB and then replug. So doing
+* once is enough. The cleanup is handled correctly on the way out
+* (standard USB disconnect), and the new device is detected cleanly
+* and bound to the driver again like it should be.
+*/
+ if (reset_gpio) {
+ bt_dev_dbg(hdev, "%s: Initiating HW reset via gpio", __func__);
+ clear_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
+ gpiod_set_value(reset_gpio, 1);
+ mdelay(100);
+ gpiod_set_value(reset_gpio, 0);
+ }
+}
+
static inline void btusb_free_frags(struct btusb_data *data)
{
unsigned long flags;
@@ -3030,6 +3055,11 @@ static int btusb_probe(struct usb_interface *intf,
SET_HCIDEV_DEV(hdev, &intf->dev);
+ data->reset_gpio = gpiod_get_optional(&data->udev->dev, "reset",
+ GPIOD_OUT_LOW);
+ if (data->reset_gpio)
+ hdev->hw_reset = btusb_hw_reset;
+
hdev->open = btusb_open;
hdev->close = btusb_close;
hdev->flush = btusb_flush;
@@ -3085,6 +3115,7 @@ static int btusb_probe(struct usb_interface *intf,
set_bit(HCI_QUIRK_STRICT_DUPLICATE_FILTER, &hdev->quirks);
set_bit(HCI_QUIRK_SIMULTANEOUS_DISCOVERY, &hdev->quirks);
set_bit(HCI_QUIRK_NON_PERSISTENT_DIAG, &hdev->quirks);
+ set_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks);
if (id->driver_info & BTUSB_INTEL) {
hdev->setup = btusb_setup_intel;
@@ -3225,6 +3256,8 @@ static int btusb_probe(struct usb_interface *intf,
return 0;
out_free_dev:
+ if (data->reset_gpio)
+ gpiod_put(data->reset_gpio);
hci_free_dev(hdev);
return err;
}
@@ -3268,6 +3301,9 @@ static void btusb_disconnect(struct usb_interface *intf)
if (data->oob_wake_irq)
device_init_wakeup(&data->udev->dev, false);
+ if (data->reset_gpio)
+ gpiod_put(data->reset_gpio);
+
hci_free_dev(hdev);
}
--
2.19.1.1215.g8438c0b245-goog
[PATCH 1/5] usb: split code locating ACPI companion into port and device
From: Dmitry Torokhov
In preparation for handling embedded USB devices let's split
usb_acpi_find_companion() into usb_acpi_find_companion_for_device() and
usb_acpi_find_companion_for_port().
Signed-off-by: Dmitry Torokhov
Signed-off-by: Rajat Jain
---
drivers/usb/core/usb-acpi.c | 133 +++-
1 file changed, 72 insertions(+), 61 deletions(-)
diff --git a/drivers/usb/core/usb-acpi.c b/drivers/usb/core/usb-acpi.c
index e221861b3187..8ff73c83e8e8 100644
--- a/drivers/usb/core/usb-acpi.c
+++ b/drivers/usb/core/usb-acpi.c
@@ -139,12 +139,79 @@ static struct acpi_device *usb_acpi_find_port(struct
acpi_device *parent,
return acpi_find_child_device(parent, raw, false);
}
-static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+static struct acpi_device *
+usb_acpi_get_companion_for_port(struct usb_port *port_dev)
{
struct usb_device *udev;
struct acpi_device *adev;
acpi_handle *parent_handle;
+ int port1;
+
+ /* Get the struct usb_device point of port's hub */
+ udev = to_usb_device(port_dev->dev.parent->parent);
+
+ /*
+* The root hub ports' parent is the root hub. The non-root-hub
+* ports' parent is the parent hub port which the hub is
+* connected to.
+*/
+ if (!udev->parent) {
+ adev = ACPI_COMPANION(&udev->dev);
+ port1 = usb_hcd_find_raw_port_number(bus_to_hcd(udev->bus),
+port_dev->portnum);
+ } else {
+ parent_handle = usb_get_hub_port_acpi_handle(udev->parent,
+udev->portnum);
+ if (!parent_handle)
+ return NULL;
+
+ acpi_bus_get_device(parent_handle, &adev);
+ port1 = port_dev->portnum;
+ }
+
+ return usb_acpi_find_port(adev, port1);
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_port(struct usb_port *port_dev)
+{
+ struct acpi_device *adev;
+ struct acpi_pld_info *pld;
+ acpi_handle *handle;
+ acpi_status status;
+
+ adev = usb_acpi_get_companion_for_port(port_dev);
+ if (!adev)
+ return NULL;
+
+ handle = adev->handle;
+ status = acpi_get_physical_device_location(handle, &pld);
+ if (!ACPI_FAILURE(status) && pld) {
+ port_dev->location = USB_ACPI_LOCATION_VALID
+ | pld->group_token << 8 | pld->group_position;
+ port_dev->connect_type = usb_acpi_get_connect_type(handle, pld);
+ ACPI_FREE(pld);
+ }
+ return adev;
+}
+
+static struct acpi_device *
+usb_acpi_find_companion_for_device(struct usb_device *udev)
+{
+ struct acpi_device *adev;
+
+ if (!udev->parent)
+ return NULL;
+
+ /* root hub is only child (_ADR=0) under its parent, the HC */
+ adev = ACPI_COMPANION(udev->dev.parent);
+ return acpi_find_child_device(adev, 0, false);
+}
+
+
+static struct acpi_device *usb_acpi_find_companion(struct device *dev)
+{
/*
* In the ACPI DSDT table, only usb root hub and usb ports are
* acpi device nodes. The hierarchy like following.
@@ -158,66 +225,10 @@ static struct acpi_device *usb_acpi_find_companion(struct
device *dev)
* So all binding process is divided into two parts. binding
* root hub and usb ports.
*/
- if (is_usb_device(dev)) {
- udev = to_usb_device(dev);
- if (udev->parent)
- return NULL;
-
- /* root hub is only child (_ADR=0) under its parent, the HC */
- adev = ACPI_COMPANION(dev->parent);
- return acpi_find_child_device(adev, 0, false);
- } else if (is_usb_port(dev)) {
- struct usb_port *port_dev = to_usb_port(dev);
- int port1 = port_dev->portnum;
- struct acpi_pld_info *pld;
- acpi_handle *handle;
- acpi_status status;
-
- /* Get the struct usb_device point of port's hub */
- udev = to_usb_device(dev->parent->parent);
-
- /*
-* The root hub ports' parent is the root hub. The non-root-hub
-* ports' parent is the parent hub port which the hub is
-* connected to.
-*/
- if (!udev->parent) {
- struct usb_hcd *hcd = bus_to_hcd(udev->bus);
- int raw;
-
- raw = usb_hcd_find_raw_port_number(hcd, port1);
-
- adev = usb_acpi_find_port(ACPI_COMPANION(&udev->dev),
- raw);
-
- if (!adev)
- return NULL;
- } else {
- parent_handle =
- us
[PATCH 0/5] Reset Intel BT controller if it gets stuck
There can be error conditions within Intel BT firmware that can cause it to get stuck, with the only way out being toggle the reset pin to the device. (I do not have the details about the issues that lead to such conditions, Intel folks copied here can elaborate if needed). Thus, this is an effor to be able to toggle the reset line from the driver if it detects such a situation. It makes few enhancements to the common framework which I think may be useful for other unrelated problems. Dmitry Torokhov (2): usb: split code locating ACPI companion into port and device usb: assign ACPI companions for embedded USB devices (This basically allows ACPI nodes to be attached to the USB devices, thus useful for any onboard / embedded USB devices that wants to get some info from the ACPI). Rajat Jain (3): Bluetooth: Reset Bluetooth chip after multiple command timeouts Bluetooth: btusb: Collect the common Intel assignments together Bluetooth: btusb: Use the hw_reset method to allow resetting the BT chip drivers/bluetooth/btusb.c| 63 +--- drivers/usb/core/usb-acpi.c | 163 +++ include/net/bluetooth/hci.h | 8 ++ include/net/bluetooth/hci_core.h | 2 + net/bluetooth/hci_core.c | 15 ++- 5 files changed, 171 insertions(+), 80 deletions(-) -- 2.19.1.1215.g8438c0b245-goog
[PATCH 3/5] Bluetooth: Reset Bluetooth chip after multiple command timeouts
Add a quirk and a hook to allow the HCI core to reset the BT chip
if needed (after a number of timed out commands). Use that new hook to
initiate BT chip reset if the controller fails to respond to certain
number of commands (currently 5) including the HCI reset commands.
This is done based on a newly introduced quirk. This is done based
on some initial work by Intel.
Signed-off-by: Rajat Jain
---
include/net/bluetooth/hci.h | 8
include/net/bluetooth/hci_core.h | 2 ++
net/bluetooth/hci_core.c | 15 +--
3 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/include/net/bluetooth/hci.h b/include/net/bluetooth/hci.h
index c36dc1e20556..af02fa5ffe54 100644
--- a/include/net/bluetooth/hci.h
+++ b/include/net/bluetooth/hci.h
@@ -192,6 +192,14 @@ enum {
*
*/
HCI_QUIRK_NON_PERSISTENT_SETUP,
+
+ /* When this quirk is set, hw_reset() would be run to reset the
+* hardware, after a certain number of commands (currently 5)
+* time out because the device fails to respond.
+*
+* This quirk should be set before hci_register_dev is called.
+*/
+ HCI_QUIRK_HW_RESET_ON_TIMEOUT,
};
/* HCI device flags */
diff --git a/include/net/bluetooth/hci_core.h b/include/net/bluetooth/hci_core.h
index e5ea633ea368..b86218304b80 100644
--- a/include/net/bluetooth/hci_core.h
+++ b/include/net/bluetooth/hci_core.h
@@ -313,6 +313,7 @@ struct hci_dev {
unsigned intacl_cnt;
unsigned intsco_cnt;
unsigned intle_cnt;
+ unsigned inttimeout_cnt;
unsigned intacl_mtu;
unsigned intsco_mtu;
@@ -437,6 +438,7 @@ struct hci_dev {
int (*post_init)(struct hci_dev *hdev);
int (*set_diag)(struct hci_dev *hdev, bool enable);
int (*set_bdaddr)(struct hci_dev *hdev, const bdaddr_t *bdaddr);
+ void (*hw_reset)(struct hci_dev *hdev);
};
#define HCI_PHY_HANDLE(handle) (handle & 0xff)
diff --git a/net/bluetooth/hci_core.c b/net/bluetooth/hci_core.c
index 7352fe85674b..ab3a6a8b7ba6 100644
--- a/net/bluetooth/hci_core.c
+++ b/net/bluetooth/hci_core.c
@@ -2569,13 +2569,24 @@ static void hci_cmd_timeout(struct work_struct *work)
struct hci_dev *hdev = container_of(work, struct hci_dev,
cmd_timer.work);
+ hdev->timeout_cnt++;
if (hdev->sent_cmd) {
struct hci_command_hdr *sent = (void *) hdev->sent_cmd->data;
u16 opcode = __le16_to_cpu(sent->opcode);
- bt_dev_err(hdev, "command 0x%4.4x tx timeout", opcode);
+ bt_dev_err(hdev, "command 0x%4.4x tx timeout (cnt = %u)",
+ opcode, hdev->timeout_cnt);
} else {
- bt_dev_err(hdev, "command tx timeout");
+ bt_dev_err(hdev, "command tx timeout (cnt = %u)",
+ hdev->timeout_cnt);
+ }
+
+ if (test_bit(HCI_QUIRK_HW_RESET_ON_TIMEOUT, &hdev->quirks) &&
+ hdev->timeout_cnt >= 5) {
+ hdev->timeout_cnt = 0;
+ if (hdev->hw_reset)
+ hdev->hw_reset(hdev);
+ return;
}
atomic_set(&hdev->cmd_cnt, 1);
--
2.19.1.1215.g8438c0b245-goog
Re: [PATCH 01/10] dt-bindings: usb: add support for dwc3 controller on HiSilicon SoCs
Hi,
On 2018/11/13 0:02, Rob Herring wrote:
> On Sat, Oct 27, 2018 at 05:58:11PM +0800, Yu Chen wrote:
>> This patch adds binding descriptions to support the dwc3 controller
>> on HiSilicon SoCs and boards like the HiKey960.
>>
>> Cc: Greg Kroah-Hartman
>> Cc: Rob Herring
>> Cc: Mark Rutland
>> Cc: John Stultz
>> Signed-off-by: Yu Chen
>> ---
>> .../devicetree/bindings/usb/dwc3-hisi.txt | 53
>> ++
>> 1 file changed, 53 insertions(+)
>> create mode 100644 Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>>
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>> b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>> new file mode 100644
>> index ..e715e7b1c324
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/usb/dwc3-hisi.txt
>> @@ -0,0 +1,53 @@
>> +HiSilicon DWC3 USB SoC controller
>> +
>> +This file documents the parameters for the dwc3-hisi driver.
>> +
>> +Required properties:
>> +- compatible: should be "hisilicon,hi3660-dwc3"
>> +- clocks: A list of phandle + clock-specifier pairs for the
>> +clocks listed in clock-names
>> +- clock-names: Specify clock names
>> +- resets: list of phandle and reset specifier pairs.
>> +
>> +Sub-nodes:
>> +The dwc3 core should be added as subnode to HiSilicon DWC3 as shown in the
>> +example below. The DT binding details of dwc3 can be found in:
>> +Documentation/devicetree/bindings/usb/dwc3.txt
>
> If you only have clocks and resets and no glue registers, then you
> don't need a sub-node. Just make the controller one node.
>
In dwc3 glue driver，the controller driver usually probed by call
of_platform_populate
which will search the child node of glue driver.
The code of function of_platform_populate is as below:
for_each_child_of_node(root, child) {
rc = of_platform_bus_create(child, matches, lookup, parent,
true);
if (rc) {
of_node_put(child);
break;
}
}
So I think the controller node should be a sub-node.
>> +
>> +Example:
>> +usb3: hisi_dwc3 {
>> +compatible = "hisilicon,hi3660-dwc3";
>> +#address-cells = <2>;
>> +#size-cells = <2>;
>> +ranges;
>> +
>> +clocks = <&crg_ctrl HI3660_CLK_ABB_USB>,
>> + <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
>> +clock-names = "clk_usb3phy_ref", "aclk_usb3otg";
>> +assigned-clocks = <&crg_ctrl HI3660_ACLK_GATE_USB3OTG>;
>> +assigned-clock-rates = <22900>;
>> +resets = <&crg_rst 0x90 8>,
>> + <&crg_rst 0x90 7>,
>> + <&crg_rst 0x90 6>,
>> + <&crg_rst 0x90 5>;
>> +
>> +dwc3: dwc3@ff10 {
>
> usb@...
>
>> +compatible = "snps,dwc3";
>> +reg = <0x0 0xff10 0x0 0x10>;
>> +interrupts = <0 159 4>, <0 161 4>;
>> +phys = <&usb_phy>;
>> +phy-names = "usb3-phy";
>> +dr_mode = "otg";
>> +maximum-speed = "super-speed";
>> +phy_type = "utmi";
>> +snps,dis-del-phy-power-chg-quirk;
>> +snps,lfps_filter_quirk;
>> +snps,dis_u2_susphy_quirk;
>> +snps,dis_u3_susphy_quirk;
>> +snps,tx_de_emphasis_quirk;
>> +snps,tx_de_emphasis = <1>;
>> +snps,dis_enblslpm_quirk;
>> +snps,gctl-reset-quirk;
>> +extcon = <&hisi_hikey_usb>;
>> +};
>> +};
>> --
>> 2.15.0-rc2
>>
>
>
> .
>
Re: SMSC95xx driver updates (round 1)
From: Ben Dooks Date: Wed, 14 Nov 2018 11:50:18 + > This is a series of a few driver cleanups and some fixups of the code > for the SMSC95XX driver. There have been a few reviews, and the issues > have been fixed so this should be ready for merging. > > I will work on the tx-alignment and the other bits of usbnet changes > and produce at least two more patch series for this later. Series applied with the missing space in the comment of patch #4 fixed. Thanks.
