Re: [PATCH] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
Hi, On 08-01-19 07:25, m.bal...@intel.com wrote: From: M, Balaji This fix enables USB role feature on intel commercial nuc platform which is based on Kabylake chipset. Signed-off-by: M, Balaji Patch looks good to me: Reviewed-by: Hans de Goede Regards, Hans --- drivers/usb/host/xhci-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index a9ec7051f286..c2fe218e051f 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
[PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
From: M, Balaji This fix enables USB role feature on intel commercial nuc platform which is based on Kabylake chipset. Signed-off-by: M, Balaji Reviewed-by: Hans de Goede --- Changes from v1: Added Reviewed-by drivers/usb/host/xhci-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index a9ec7051f286..c2fe218e051f 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_INTEL && -- 2.17.1
Re: [PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
Hi, On 08-01-19 11:04, m.bal...@intel.com wrote: From: M, Balaji This fix enables USB role feature on intel commercial nuc platform which is based on Kabylake chipset. Signed-off-by: M, Balaji Reviewed-by: Hans de Goede --- Changes from v1: Added Reviewed-by There is no need to send out a v2 just to add a Reviewed-by, the subsys-maintainer will pick the Reviewed-by up when merging. If you need to do a v2 to add some other minor change adding the Reviewed-by is fine. Regards, Hans drivers/usb/host/xhci-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index a9ec7051f286..c2fe218e051f 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_INTEL &&
Re: [PATCH 02/19] usbnet: smsc95xx: Stop propagation of in_pm
On Do, 2019-01-03 at 02:10 +0100, Marek Vasut wrote: > The information whether the SMSC95xx is in_pm or not can be derived from > the usbdev->suspend_count. First thing called in smsc95xx_suspend() is > usbnet_suspend(), which increments the usbdev->suspend_count and since > then the driver only calls _nopm() functions and functions with in_pm > set to 1. The smsc95xx_resume() does the inverse, it calls functions > with _nopm() suffix and with in_pm set to 1 until usbnet_resume(), which > sets the usbdev->suspend_count to 0. Hi, unfortunately I am forced to disagree in the strongest terms. The logic behind this patch is wrong. The "in_pm" parameter exists because some function need to be called in the code paths needed to implement power management. Under those circumstances they must not take a pm reference to keep the device awake, lest the driver deadlock. (For example __smsc95xx_read_reg) However from other code paths precisely that reference must be taken in order to make sure that the driver do not try to communicate with a suspended device. If you check the suspend counter, you will omit the necessary getting of a reference precisely when it is needed. The driver will never dedalock, but you will cause numerous races. I must suggest to simply drop this part and redo the series. Regards Oliver
Re: [PATCH 07/19] usbnet: smsc95xx: Split the reset function
On Do, 2019-01-03 at 02:10 +0100, Marek Vasut wrote:
> The smsc95xx_reset() is called either during bind or later during
> the driver operation. However, the MII structure can be populated
> only once, when the smsc95xx_reset() is called from the drivers
> bind function.
>
> Split the reset function to allow filling the MII structure only
> once. This is done in preparation of phydev conversion, where the
> code will connect to PHY between those two halves of the reset
> function.
>
> Signed-off-by: Marek Vasut
> Cc: David S. Miller
> Cc: Nisar Sayed
> Cc: Woojung Huh
> Cc: Andrew Lunn
> Cc: Florian Fainelli
> Cc: linux-usb@vger.kernel.org
> To: net...@vger.kernel.org
> ---
> drivers/net/usb/smsc95xx.c | 36 +++-
> 1 file changed, 27 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 551d05eb258e..e40cde490a42 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -944,14 +944,6 @@ static int smsc95xx_phy_initialize(struct usbnet *dev)
> {
> int bmcr, ret, timeout = 0;
>
> - /* Initialize MII structure */
> - dev->mii.dev = dev->net;
> - dev->mii.mdio_read = smsc95xx_mdio_read;
> - dev->mii.mdio_write = smsc95xx_mdio_write;
> - dev->mii.phy_id_mask = 0x1f;
> - dev->mii.reg_num_mask = 0x1f;
> - dev->mii.phy_id = SMSC95XX_INTERNAL_PHY_ID;
> -
> /* reset phy and wait for reset to complete */
> smsc95xx_mdio_write(dev->net, dev->mii.phy_id, MII_BMCR, BMCR_RESET);
>
> @@ -985,7 +977,7 @@ static int smsc95xx_phy_initialize(struct usbnet *dev)
> return 0;
> }
>
> -static int smsc95xx_reset(struct usbnet *dev)
> +static int smsc95xx_reset_pre(struct usbnet *dev)
Hi,
may I request that you choose different names? These names suggest a
connection with the pre_reset() and post_reset() methods of a USB
driver.
Regards
Oliver
Re: [PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
On Tue, Jan 08, 2019 at 05:04:20AM -0500, m.bal...@intel.com wrote: > From: M, Balaji > > This fix enables USB role feature on intel commercial nuc > platform which is based on Kabylake chipset. > > Signed-off-by: M, Balaji > Reviewed-by: Hans de Goede OK by me: Reviewed-by: Heikki Krogerus Do not send an other version where you add just that tag! Mathias, as the maintainer, will take care of adding the tag to the commit. > --- > Changes from v1: Added Reviewed-by > drivers/usb/host/xhci-pci.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > index a9ec7051f286..c2fe218e051f 100644 > --- a/drivers/usb/host/xhci-pci.c > +++ b/drivers/usb/host/xhci-pci.c > @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct > xhci_hcd *xhci) > xhci->quirks |= XHCI_SSIC_PORT_UNUSED; > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || > + pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || >pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) > xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > -- > 2.17.1 thanks, -- heikki
Re: [PATCH 02/19] usbnet: smsc95xx: Stop propagation of in_pm
On 1/7/19 12:02 PM, Oliver Neukum wrote: > On Do, 2019-01-03 at 02:10 +0100, Marek Vasut wrote: >> The information whether the SMSC95xx is in_pm or not can be derived from >> the usbdev->suspend_count. First thing called in smsc95xx_suspend() is >> usbnet_suspend(), which increments the usbdev->suspend_count and since >> then the driver only calls _nopm() functions and functions with in_pm >> set to 1. The smsc95xx_resume() does the inverse, it calls functions >> with _nopm() suffix and with in_pm set to 1 until usbnet_resume(), which >> sets the usbdev->suspend_count to 0. > > Hi, Hello Oliver, > unfortunately I am forced to disagree in the strongest terms. > > The logic behind this patch is wrong. The "in_pm" parameter > exists because some function need to be called in the code paths > needed to implement power management. Under those circumstances > they must not take a pm reference to keep the device awake, lest > the driver deadlock. > (For example __smsc95xx_read_reg) > > However from other code paths precisely that reference must be taken > in order to make sure that the driver do not try to communicate with > a suspended device. > If you check the suspend counter, you will omit the necessary getting > of a reference precisely when it is needed. The driver will never > dedalock, but you will cause numerous races. > > I must suggest to simply drop this part and redo the series. OK, let's drop the whole series. -- Best regards, Marek Vasut
Re: [PATCH lora-next 3/5] net: lora: sx130x: Add PicoCell serdev driver
On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote:
>
> +struct picogw_device {
> + struct serdev_device *serdev;
> +
> + u8 rx_buf[1024];
No, you cannot do that. AFAICT this buffer can be used for DMA.
Thus putting it into another data structure violates the rules
of DMA coherency. You must allocate it separately.
> +static int picogw_send_cmd(struct picogw_device *picodev, char cmd,
> + u8 addr, const void *data, u16 data_len)
> +{
> + struct serdev_device *sdev = picodev->serdev;
> + u8 buf[4];
> + int i, ret;
> +
> + buf[0] = cmd;
> + buf[1] = (data_len >> 8) & 0xff;
> + buf[2] = (data_len >> 0) & 0xff;
We have macros for converting endianness and unaligned access.
> + buf[3] = addr;
> +
> + dev_dbg(&sdev->dev, "%s: %c %02x %02x %02x\n", __func__, buf[0],
> + (unsigned int)buf[1], (unsigned int)buf[2], (unsigned
> int)buf[3]);
> + for (i = 0; i < data_len; i++) {
> + dev_dbg(&sdev->dev, "%s: data %02x\n", __func__, (unsigned
> int)((const u8*)data)[i]);
> + }
> +
> + ret = serdev_device_write_buf(sdev, buf, 4);
> + if (ret < 0)
> + return ret;
> + if (ret != 4)
> + return -EIO;
> +
> + if (data_len) {
> + ret = serdev_device_write_buf(sdev, data, data_len);
> + if (ret < 0)
> + return ret;
> + if (ret != data_len)
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> +static int picogw_recv_answer(struct picogw_device *picodev,
> + char *cmd, bool *ack, void *buf, int buf_len,
> + unsigned long timeout)
> +{
> + int len;
> +
> + timeout = wait_for_completion_timeout(&picodev->answer_comp, timeout);
> + if (!timeout)
> + return -ETIMEDOUT;
And? The IO is still scheduled. Simply erroring out is problematic.
If you see a timeout you need to kill the scheduled IO.
> +static int picogw_handle_answer(struct picogw_device *picodev)
> +{
> + struct device *dev = &picodev->serdev->dev;
> + unsigned int data_len = ((u16)picodev->rx_buf[1] << 8) |
> picodev->rx_buf[2];
> + int cmd_len = 4 + data_len;
> + int i, ret;
> +
> + if (picodev->rx_len < 4)
> + return 0;
> +
> + if (cmd_len > sizeof(picodev->rx_buf)) {
> + dev_warn(dev, "answer too long (%u)\n", data_len);
> + return 0;
> + }
> +
> + if (picodev->rx_len < cmd_len) {
> + dev_dbg(dev, "got %u, need %u bytes\n", picodev->rx_len,
> cmd_len);
> + return 0;
> + }
> +
> + dev_dbg(dev, "Answer %c =%u %s (%u)\n", picodev->rx_buf[0],
> + (unsigned int)picodev->rx_buf[3],
> + (picodev->rx_buf[3] == 1) ? "OK" : "K0",
> + data_len);
> + for (i = 0; i < data_len; i++) {
> + //dev_dbg(dev, "%s: %02x\n", __func__, (unsigned
> int)picodev->rx_buf[4 + i]);
> + }
> +
> + complete(&picodev->answer_comp);
> + ret =
> wait_for_completion_interruptible_timeout(&picodev->answer_read_comp, HZ / 2);
Problematic. You have no idea when that complete() will have an effect.
You are operating with an undefined timeout here. Theoretically it
could be negative.
Regards
Oliver
Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote: > Switch from tty_port_register_device() to tty_port_register_device_serdev() > and from tty_unregister_device() to tty_port_unregister_device(). > > On removal of a serdev driver sometimes count mismatch warnings were seen: > > ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0)) > ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0 > > Note: The serdev drivers appear to probe asynchronously as soon as they > are registered. Should the USB quirks in probe be moved before registration? > No noticeable difference for the devices at hand. That is quite drastic a change. Johan, how complete in terms of features is serdev? Are you refering to CLEAR_HALT_CONDITIONS in terms of quirks? Regards Oliver
[PATCH] usb: core: Simplify return value of usb_get_configuration()
It is better to initialize the return value "result" to -ENOMEM
than to 0. And because "result" takes the return value of
usb_parse_configuration() which returns 0 for success, setting
"result" to 0 at before and after of the for loop is unnecessary.
Signed-off-by: Suwan Kim
---
drivers/usb/core/config.c | 5 +
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
index 7b5cb28ffb35..4a0945c04b4c 100644
--- a/drivers/usb/core/config.c
+++ b/drivers/usb/core/config.c
@@ -800,13 +800,12 @@ int usb_get_configuration(struct usb_device *dev)
{
struct device *ddev = &dev->dev;
int ncfg = dev->descriptor.bNumConfigurations;
- int result = 0;
+ int result = -ENOMEM;
unsigned int cfgno, length;
unsigned char *bigbuffer;
struct usb_config_descriptor *desc;
cfgno = 0;
- result = -ENOMEM;
if (ncfg > USB_MAXCONFIG) {
dev_warn(ddev, "too many configurations: %d, "
"using maximum allowed: %d\n", ncfg, USB_MAXCONFIG);
@@ -832,7 +831,6 @@ int usb_get_configuration(struct usb_device *dev)
if (!desc)
goto err2;
- result = 0;
for (; cfgno < ncfg; cfgno++) {
/* We grab just the first descriptor so we know how long
* the whole configuration is */
@@ -889,7 +887,6 @@ int usb_get_configuration(struct usb_device *dev)
goto err;
}
}
- result = 0;
err:
kfree(desc);
--
2.20.1
RE: [PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
> -Original Message- > From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb- > ow...@vger.kernel.org] On Behalf Of Hans de Goede > Sent: Monday, January 7, 2019 4:20 PM > To: Balaji, M ; linux-usb@vger.kernel.org > Cc: mathias.ny...@linux.intel.com; heikki.kroge...@linux.intel.com > Subject: Re: [PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on > INTEL_SUNRISEPOINT_LP_XHCI > > Hi, > > On 08-01-19 11:04, m.bal...@intel.com wrote: > > From: M, Balaji > > > > This fix enables USB role feature on intel commercial nuc > > platform which is based on Kabylake chipset. > > > > Signed-off-by: M, Balaji > > Reviewed-by: Hans de Goede > > --- > > Changes from v1: Added Reviewed-by > > There is no need to send out a v2 just to add a Reviewed-by, > the subsys-maintainer will pick the Reviewed-by up when merging. > > If you need to do a v2 to add some other minor change adding the > Reviewed-by is fine. > > Regards, > > Hans Hello Hans , Sure got it , thanks Will take care next time . Regards , Balaji > > > > drivers/usb/host/xhci-pci.c | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c > > index a9ec7051f286..c2fe218e051f 100644 > > --- a/drivers/usb/host/xhci-pci.c > > +++ b/drivers/usb/host/xhci-pci.c > > @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct > xhci_hcd *xhci) > > xhci->quirks |= XHCI_SSIC_PORT_UNUSED; > > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > > (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || > > +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || > > pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) > > xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; > > if (pdev->vendor == PCI_VENDOR_ID_INTEL && > >
Re: [PATCH RFC 4/5] usb: cdc-acm: Enable serdev support
On Mon, Jan 07, 2019 at 02:48:26PM +0100, Oliver Neukum wrote: > On Fr, 2019-01-04 at 12:21 +0100, Andreas Färber wrote: > > Switch from tty_port_register_device() to tty_port_register_device_serdev() > > and from tty_unregister_device() to tty_port_unregister_device(). > > > > On removal of a serdev driver sometimes count mismatch warnings were seen: > > > > ttyACM ttyACM0: tty_hangup: tty->count(1) != (#fd's(0) + #kopen's(0)) > > ttyACM ttyACM0: tty_port_close_start: tty->count = 1 port count = 0 > > > > Note: The serdev drivers appear to probe asynchronously as soon as they > > are registered. Should the USB quirks in probe be moved before registration? > > No noticeable difference for the devices at hand. > > That is quite drastic a change. > Johan, how complete in terms of features is serdev? serdev doesn't support hangups yet, and that's precisely why it's not enabled for hotpluggable buses. That would need to be fixed before accepting something like this. Johan
Re: [PATCH v4] USB: Don't enable LPM if it's already enabled
On Mon, 7 Jan 2019, Kai Heng Feng wrote: > Hi, > > > On Dec 3, 2018, at 18:26, Kai-Heng Feng wrote: > > > > USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working > > after S3: > > [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin > > [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110) > > > > After some experiments, I found that disabling LPM can workaround the > > issue. > > > > On some platforms, the USB power is cut during S3, so the driver uses > > reset-resume to resume the device. During port resume, LPM gets enabled > > twice, by usb_reset_and_verify_device() and usb_port_resume(). > > > > So let's enable LPM for just once, as this solves the issue for the > > device in question. > > > > Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm() > > and usb_disable_usb2_hardware_lpm(). > > Please review my new approach, hopefully this can be included in Linux v5.0. > > > > > Signed-off-by: Kai-Heng Feng > > --- > > v4: > > - Use usb_enable_usb2_hardware_lpm() and > > usb_disable_usb2_hardware_lpm() to control USB2 LPM. > > v3: > > - Consolidate udev->usb2_hw_lpm_capable and udev->usb2_hw_lpm_enabled > > check to usb_set_usb2_hardware_lpm(). > > v2: > > - Check udev->usb2_hw_lpm_enabled before calling usb_port_resume(). > > > > drivers/usb/core/driver.c | 23 +++ > > drivers/usb/core/hub.c | 16 ++-- > > drivers/usb/core/message.c | 3 +-- > > drivers/usb/core/sysfs.c | 5 - > > drivers/usb/core/usb.h | 10 -- > > 5 files changed, 38 insertions(+), 19 deletions(-) Reviewed-by: Alan Stern
Re: [PATCH] usb: core: Simplify return value of usb_get_configuration()
On Mon, 7 Jan 2019, Suwan Kim wrote:
> It is better to initialize the return value "result" to -ENOMEM
> than to 0. And because "result" takes the return value of
> usb_parse_configuration() which returns 0 for success, setting
> "result" to 0 at before and after of the for loop is unnecessary.
>
> Signed-off-by: Suwan Kim
> ---
> drivers/usb/core/config.c | 5 +
> 1 file changed, 1 insertion(+), 4 deletions(-)
>
> diff --git a/drivers/usb/core/config.c b/drivers/usb/core/config.c
> index 7b5cb28ffb35..4a0945c04b4c 100644
> --- a/drivers/usb/core/config.c
> +++ b/drivers/usb/core/config.c
> @@ -800,13 +800,12 @@ int usb_get_configuration(struct usb_device *dev)
> {
> struct device *ddev = &dev->dev;
> int ncfg = dev->descriptor.bNumConfigurations;
> - int result = 0;
> + int result = -ENOMEM;
> unsigned int cfgno, length;
> unsigned char *bigbuffer;
> struct usb_config_descriptor *desc;
>
> cfgno = 0;
> - result = -ENOMEM;
> if (ncfg > USB_MAXCONFIG) {
> dev_warn(ddev, "too many configurations: %d, "
> "using maximum allowed: %d\n", ncfg, USB_MAXCONFIG);
> @@ -832,7 +831,6 @@ int usb_get_configuration(struct usb_device *dev)
> if (!desc)
> goto err2;
>
> - result = 0;
> for (; cfgno < ncfg; cfgno++) {
> /* We grab just the first descriptor so we know how long
>* the whole configuration is */
> @@ -889,7 +887,6 @@ int usb_get_configuration(struct usb_device *dev)
> goto err;
> }
> }
> - result = 0;
>
> err:
> kfree(desc);
Acked-by: Alan Stern
Re: [RFC lora-next 5/5] HACK: net: lora: sx130x: Add PicoCell gateway shim for cdc-acm
On Sat, Jan 05, 2019 at 12:43:48AM +0100, Andreas Färber wrote: > Hi Rob, > > Am 04.01.19 um 18:07 schrieb Rob Herring: > > On Fri, Jan 4, 2019 at 5:21 AM Andreas Färber wrote: > >> > >> Ignore our device in cdc-acm probing and add a new driver for it, > >> dispatching to cdc-acm for the actual implementation. > >> > >> WARNING: It is likely that this VID/PID is in use for unrelated devices. > >> Only the Product string hints what this really is in current v0.2.1. > >> Previous code v0.2.0 was using a Semtech VID/PID, but no card shipping > >> with such firmware is known to me. > >> > >> While this may seem unorthodox, no internals of the driver are accessed, > >> just the set of driver callbacks is duplicated as shim. > >> > >> Use this shim construct to fake DT nodes for serdev on probe. > >> For testing purposes these nodes do not have a parent yet. > >> This results in two "Error -2 creating of_node link" warnings on probe. > > > > It looks like the DT is pretty static. Rather than creating the nodes > > at run-time, can't you create a dts file and build that into your > > module. > > Heh, if that were the only issue with this patch... ;) My thoughts exactly. ;) This clearly is too much of a hack, but maintaining serdev compatible information in the corresponding tty drivers is probably what we'll want to do. When the tty driver binds and registers its ports with tty core, we can could match again on the USB descriptors, but since the device has already been matched, we can just pass the equivalent of a compatible string, or more generally dt-fragment, instead. Without having had time to look into it myself yet, this sounds like something we should be using the new software nodes for (software generated fw nodes). That way, we don't depend on CONFIG_OF either. Johan
Re: [PATCH] USB: serial: simple: add Motorola Tetra driver for TPG2200
On Mon, Jan 07, 2019 at 08:31:49AM +0100, Max Schulze wrote: > Add new Motorola Tetra (simple) driver for Motorola Solutions TETRA PEI > device I rephrased the summary and comment above since you're adding a new device id to an existing driver (rather than a new driver). > T: Bus=02 Lev=01 Prnt=01 Port=01 Cnt=01 Dev#= 4 Spd=480 MxCh= 0 > D: Ver= 2.00 Cls=00(>ifc ) Sub=00 Prot=00 MxPS=64 #Cfgs= 1 > P: Vendor=0cad ProdID=9016 Rev=24.16 > S: Manufacturer=Motorola Solutions, Inc. > S: Product=TETRA PEI interface > C: #Ifs= 2 Cfg#= 1 Atr=80 MxPwr=500mA > I: If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 > Driver=usb_serial_simple > I: If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=00 Prot=00 > Driver=usb_serial_simple > > Signed-off-by: Max Schulze > Tested-by: Max Schulze And no need to add Tested-by for your own patches; submissions are always supposed to be tested. Applied for -rc2. Thanks, Johan
RE: r8152: data corruption in various scenarios
> -Original Message-
> From: Hayes Wang
> Sent: Sunday, January 6, 2019 9:54 PM
> To: Mark Lord; Kai Heng Feng
> Cc: Ansis Atteka; David Miller; g...@kroah.com; rom...@fr.zoreil.com;
> net...@vger.kernel.org; nic_swsd; linux-ker...@vger.kernel.org; linux-
> u...@vger.kernel.org; Limonciello, Mario; Ryankao
> Subject: RE: r8152: data corruption in various scenarios
>
>
> [EXTERNAL EMAIL]
>
> Monday, January 07, 2019 5:17 AM
> [...]
> >> This is probably an xHC bug. A similar issue is fixed by commit
> >> 9da5a1092b13
> >> ("xhci: Bad Ethernet performance plugged in ASM1042A host”).
> >>
> >>> I just got that exact message above, with the r8152 in my 1-day old WD15
> >>> dock,
> >>> with the TB16 "workaround" enabled in Linux kernel 4.20.0.
> >>
> >> Is the xHC WD15 connected an ASMedia one?
> >
> > I don't know. I *think* it identifies as a DSL6340 (see below).
> >
>
> According to our record, it is relative to the asmedia.
>
DSL6430 should be referring to the Alpine Ridge controller in the system.
TB16 contains ASMedia host controller. It's a Thunderbolt dock and all USB
devices
are connected to ASMedia host controller in the dock.
WD15 does not contain an ASMedia host controller, it connected to system's
USB host controller.
Re: [PATCH v4] USB: Don't enable LPM if it's already enabled
On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote: > USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working > after S3: > [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin > [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110) > > After some experiments, I found that disabling LPM can workaround the > issue. > > On some platforms, the USB power is cut during S3, so the driver uses > reset-resume to resume the device. During port resume, LPM gets enabled > twice, by usb_reset_and_verify_device() and usb_port_resume(). > > So let's enable LPM for just once, as this solves the issue for the > device in question. > > Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm() > and usb_disable_usb2_hardware_lpm(). > > Signed-off-by: Kai-Heng Feng What kernel patch does this one "fix"? Adding a "Fixes:" tag would be good to try to figure out how far back in the kernel releases this should be backported. thanks, greg k-h
Re: [PATCH] USB: host: ehci: allow tine of highspeed nak-count
On Wed, Nov 14, 2018 at 05:13:15PM +, 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. > > To allow the changing of this value, add a sysfs node to each of > the controllers to allow run-time changing. That's going to be a pain, why can you not just figure this out at runtime and adjust it that way? Also, you can't add a sysfs file and not also add a Documentation/API/ update :( Can you fix this up to work without needing manual adjustments? thanks, greg k-h
Re: [PATCH V2] roles: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
On Tue, Jan 08, 2019 at 05:04:20AM -0500, m.bal...@intel.com wrote: > From: M, Balaji > > This fix enables USB role feature on intel commercial nuc > platform which is based on Kabylake chipset. Your subject prefix is a bit odd "roles" is not a global thing, it should look something like: usb: xhci: fix for enabling... thanks, greg k-h
Re: [PATCH v3] usb: hub: add retry routine after intr URB submit error
On Tue, Nov 20, 2018 at 03:34:38PM +0100, Nicolas Saenz Julienne wrote:
> The hub sends hot-plug events to the host trough it's interrupt URB. The
> driver takes care of completing the URB and re-submitting it. Completion
> errors are handled in the hub_event() work, yet submission errors are
> ignored, rendering the device unresponsive. All further events are lost.
>
> It is fairly hard to find this issue in the wild, since you have to time
> the USB hot-plug event with the URB submission failure. For instance it
> could be the system running out of memory or some malfunction in the USB
> controller driver. Nevertheless, it's pretty reasonable to think it'll
> happen sometime. One can trigger this issue using eBPF's function
> override feature (see BCC's inject.py script).
>
> This patch adds a retry routine to the event of a submission error. The
> HUB driver will try to re-submit the URB once every second until it's
> successful or the HUB is disconnected.
>
> As some USB subsystems already take care of this issue, the
> implementation was inspired from usbhid/hid_core.c's.
>
> Signed-off-by: Nicolas Saenz Julienne
>
> ---
What ever happened to this patch? Is it still needed? Oliver and Alan,
any objections about it anymore?
thanks,
greg k-h
>
> v3: As per Oliver's request:
> - Take care of race condition between disconnect and irq
>
> v2: as per Alan's and Oliver's comments:
> - Rename timer
> - Delete the timer on disconnect
> - Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
> period
> - Check for -ESHUTDOWN prior kicking the timer
>
> drivers/usb/core/hub.c | 45 --
> drivers/usb/core/hub.h | 2 ++
> 2 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index c6077d582d29..630490a35301 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -607,6 +607,38 @@ static int hub_port_status(struct usb_hub *hub, int
> port1,
> status, change, NULL);
> }
>
> +static void hub_resubmit_irq_urb(struct usb_hub *hub)
> +{
> + unsigned long flags;
> + int status;
> +
> + spin_lock_irqsave(&hub->irq_urb_lock, flags);
> +
> + if (hub->quiescing) {
> + spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
> + return;
> + }
> +
> + status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> + if (status && status != -ENODEV && status != -EPERM &&
> + status != -ESHUTDOWN) {
> + dev_err(hub->intfdev, "resubmit --> %d\n", status);
> + mod_timer(&hub->irq_urb_retry,
> + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
> +
> + }
> +
> + spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
> +}
> +
> +static void hub_retry_irq_urb(struct timer_list *t)
> +{
> + struct usb_hub *hub = from_timer(hub, t, irq_urb_retry);
> +
> + hub_resubmit_irq_urb(hub);
> +}
> +
> +
> static void kick_hub_wq(struct usb_hub *hub)
> {
> struct usb_interface *intf;
> @@ -709,12 +741,7 @@ static void hub_irq(struct urb *urb)
> kick_hub_wq(hub);
>
> resubmit:
> - if (hub->quiescing)
> - return;
> -
> - status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> - if (status != 0 && status != -ENODEV && status != -EPERM)
> - dev_err(hub->intfdev, "resubmit --> %d\n", status);
> + hub_resubmit_irq_urb(hub);
> }
>
> /* USB 2.0 spec Section 11.24.2.3 */
> @@ -1254,10 +1281,13 @@ enum hub_quiescing_type {
> static void hub_quiesce(struct usb_hub *hub, enum hub_quiescing_type type)
> {
> struct usb_device *hdev = hub->hdev;
> + unsigned long flags;
> int i;
>
> /* hub_wq and related activity won't re-trigger */
> + spin_lock_irqsave(&hub->irq_urb_lock, flags);
> hub->quiescing = 1;
> + spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
>
> if (type != HUB_SUSPEND) {
> /* Disconnect all the children */
> @@ -1268,6 +1298,7 @@ static void hub_quiesce(struct usb_hub *hub, enum
> hub_quiescing_type type)
> }
>
> /* Stop hub_wq and related activity */
> + del_timer_sync(&hub->irq_urb_retry);
> usb_kill_urb(hub->urb);
> if (hub->has_indicators)
> cancel_delayed_work_sync(&hub->leds);
> @@ -1800,6 +1831,8 @@ static int hub_probe(struct usb_interface *intf, const
> struct usb_device_id *id)
> INIT_DELAYED_WORK(&hub->leds, led_work);
> INIT_DELAYED_WORK(&hub->init_work, NULL);
> INIT_WORK(&hub->events, hub_event);
> + spin_lock_init(&hub->irq_urb_lock);
> + timer_setup(&hub->irq_urb_retry, hub_retry_irq_urb, 0);
> usb_get_intf(intf);
> usb_get_dev(hdev);
>
> diff --git a/drivers/usb/core/hub.h b/drivers/usb/core/hub.h
> index 4accfb63f7dc..a9e24e4b8df1 100644
> --- a/drivers/usb/core/hub.h
> +++ b/drivers/usb/core/hub.h
> @@ -69,6 +69,8 @@ struct usb_hub {
>
Re: [PATCH] cdc_ether: trivial whitespace readability fix
From: Bjørn Mork Date: Sat, 5 Jan 2019 14:32:39 +0100 > This function is unreadable enough without indenting mismatches > and unnecessary line breaks. > > Signed-off-by: Bjørn Mork Applied, thanks.
[PATCH V2] usb:xhci: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
From: M, Balaji This fix enables USB role feature on intel commercial nuc platform which is based on Kabylake chipset. Signed-off-by: M, Balaji --- Changes from v1: changed patch subject from roles: to usb:xhci drivers/usb/host/xhci-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index a9ec7051f286..c2fe218e051f 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_INTEL && -- 2.17.1
Re: [PATCH V2] usb:xhci: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
On Tue, Jan 08, 2019 at 11:31:53AM -0500, m.bal...@intel.com wrote: > From: M, Balaji We need a "full" name, "M," is probably not your first name :) thanks, greg k-h
Re: r8152: data corruption in various scenarios
On 2019-01-07 11:01 a.m., mario.limoncie...@dell.com wrote: > > TB16 contains ASMedia host controller. It's a Thunderbolt dock and all USB > devices > are connected to ASMedia host controller in the dock. > > WD15 does not contain an ASMedia host controller, it connected to system's > USB host controller. Thank-you, Mario. So.. why are we enabling the r8153 (USB-ethernet) workaround on this WD15 dock? The discussion back in 2017 was that only the TB15/TB16 were affected by the XHCI overruns it produces? -- Mark Lord Real-Time Remedies Inc. ml...@pobox.com
Re: [PATCH v1 3/6] phy: qcom-qusb2: Add QUSB2 PHY support for msm8998
Hi Jeff, Spotted a typo below: On Fri, Jan 04, 2019 at 09:50:29AM -0700, Jeffrey Hugo wrote: > MSM8998 contains one QUSB2 PHY which is very similar to the existing > sdm845 support. > > Signed-off-by: Jeffrey Hugo > --- > .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 1 + > drivers/phy/qualcomm/phy-qcom-qusb2.c | 41 > ++ > 2 files changed, 42 insertions(+) > > diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > index 03025d9..3976847 100644 > --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt > @@ -6,6 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on > Qualcomm chipsets. > Required properties: > - compatible: compatible list, contains > "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996, > +"qcom,msm8998-qusb2-phy" for 10nm PHY on msm8996, should be 8998. Thanks, Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
RE: r8152: data corruption in various scenarios
> -Original Message- > From: Mark Lord > Sent: Monday, January 7, 2019 12:06 PM > To: Limonciello, Mario; hayesw...@realtek.com; kai.heng.f...@canonical.com > Cc: aatt...@nicira.com; da...@davemloft.net; g...@kroah.com; > rom...@fr.zoreil.com; net...@vger.kernel.org; nic_s...@realtek.com; linux- > ker...@vger.kernel.org; linux-usb@vger.kernel.org; ryan...@realtek.com > Subject: Re: r8152: data corruption in various scenarios > > > [EXTERNAL EMAIL] > > On 2019-01-07 11:01 a.m., mario.limoncie...@dell.com wrote: > > > > TB16 contains ASMedia host controller. It's a Thunderbolt dock and all USB > devices > > are connected to ASMedia host controller in the dock. > > > > WD15 does not contain an ASMedia host controller, it connected to system's > > USB host controller. > > > Thank-you, Mario. > > So.. why are we enabling the r8153 (USB-ethernet) workaround on this WD15 > dock? > The discussion back in 2017 was that only the TB15/TB16 were affected by > the XHCI overruns it produces? > > -- The xHCI overrun workaround should only be applied on TB16/TB16, correct. Can you double check the verbose information from lsusb for the r8153 device on your WD15? I just double checked on my on hand WD15 with an XPS 9380 and it's not activating the quirk (bcdDevice was different). If it's the same information as the TB16 (which it sounds like it is) Kai Heng and I will check around internally to find out why they're looking the same. I can hypothesize a few guesses of what happened. My first guess would be a comparison issue with the logic in 176eb614b. Looking at that commit, I guess I would ask on the compiler behavior of !strcmp(). Would that be matching the less than case as well as the zero case? If so, it might need to be changed to strcmp() == 0. My second guess would be maybe newer ethernet NVM in manufacturing. My third guess would be a manufacturing issue putting wrong NVM image on your WD15.
Re: [PATCH v1 3/6] phy: qcom-qusb2: Add QUSB2 PHY support for msm8998
On 1/7/2019 11:18 AM, Jack Pham wrote: Hi Jeff, Spotted a typo below: On Fri, Jan 04, 2019 at 09:50:29AM -0700, Jeffrey Hugo wrote: MSM8998 contains one QUSB2 PHY which is very similar to the existing sdm845 support. Signed-off-by: Jeffrey Hugo --- .../devicetree/bindings/phy/qcom-qusb2-phy.txt | 1 + drivers/phy/qualcomm/phy-qcom-qusb2.c | 41 ++ 2 files changed, 42 insertions(+) diff --git a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt index 03025d9..3976847 100644 --- a/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt +++ b/Documentation/devicetree/bindings/phy/qcom-qusb2-phy.txt @@ -6,6 +6,7 @@ QUSB2 controller supports LS/FS/HS usb connectivity on Qualcomm chipsets. Required properties: - compatible: compatible list, contains "qcom,msm8996-qusb2-phy" for 14nm PHY on msm8996, + "qcom,msm8998-qusb2-phy" for 10nm PHY on msm8996, should be 8998. Yes it should. Thanks for noticing. -- Jeffrey Hugo Qualcomm Datacenter Technologies as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
Re: [PATCH v3] usb: hub: add retry routine after intr URB submit error
On Mon, 7 Jan 2019, Greg KH wrote:
> On Tue, Nov 20, 2018 at 03:34:38PM +0100, Nicolas Saenz Julienne wrote:
> > The hub sends hot-plug events to the host trough it's interrupt URB. The
> > driver takes care of completing the URB and re-submitting it. Completion
> > errors are handled in the hub_event() work, yet submission errors are
> > ignored, rendering the device unresponsive. All further events are lost.
> >
> > It is fairly hard to find this issue in the wild, since you have to time
> > the USB hot-plug event with the URB submission failure. For instance it
> > could be the system running out of memory or some malfunction in the USB
> > controller driver. Nevertheless, it's pretty reasonable to think it'll
> > happen sometime. One can trigger this issue using eBPF's function
> > override feature (see BCC's inject.py script).
> >
> > This patch adds a retry routine to the event of a submission error. The
> > HUB driver will try to re-submit the URB once every second until it's
> > successful or the HUB is disconnected.
> >
> > As some USB subsystems already take care of this issue, the
> > implementation was inspired from usbhid/hid_core.c's.
> >
> > Signed-off-by: Nicolas Saenz Julienne
> >
> > ---
>
> What ever happened to this patch? Is it still needed? Oliver and Alan,
> any objections about it anymore?
I have just one very minor nit to pick (see below). Mostly I've been
waiting to hear from Oliver.
Alan Stern
>
> thanks,
>
> greg k-h
>
> >
> > v3: As per Oliver's request:
> > - Take care of race condition between disconnect and irq
> >
> > v2: as per Alan's and Oliver's comments:
> > - Rename timer
> > - Delete the timer on disconnect
> > - Don't reset HUB nor exponential slowdown the timer, 1s fixed retry
> > period
> > - Check for -ESHUTDOWN prior kicking the timer
> >
> > drivers/usb/core/hub.c | 45 --
> > drivers/usb/core/hub.h | 2 ++
> > 2 files changed, 41 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> > index c6077d582d29..630490a35301 100644
> > --- a/drivers/usb/core/hub.c
> > +++ b/drivers/usb/core/hub.c
> > @@ -607,6 +607,38 @@ static int hub_port_status(struct usb_hub *hub, int
> > port1,
> >status, change, NULL);
> > }
> >
> > +static void hub_resubmit_irq_urb(struct usb_hub *hub)
> > +{
> > + unsigned long flags;
> > + int status;
> > +
> > + spin_lock_irqsave(&hub->irq_urb_lock, flags);
> > +
> > + if (hub->quiescing) {
> > + spin_unlock_irqrestore(&hub->irq_urb_lock, flags);
> > + return;
> > + }
> > +
> > + status = usb_submit_urb(hub->urb, GFP_ATOMIC);
> > + if (status && status != -ENODEV && status != -EPERM &&
> > + status != -ESHUTDOWN) {
> > + dev_err(hub->intfdev, "resubmit --> %d\n", status);
> > + mod_timer(&hub->irq_urb_retry,
> > + jiffies + msecs_to_jiffies(MSEC_PER_SEC));
This can be written more simply as:
mod_timer(&hub->irq_urb_retry, jiffies + HZ);
Re: [PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
Hi Paul,
Sorry for the delay on reviewing it.
For the subject, can you please use
usb: musb: gadget: fix short isoc packets with inventra dma
On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote:
> Handling short packets (length < max packet size) in the Inventra DMA
> engine in the MUSB driver causes the MUSB DMA controller to hang. An
> example of a problem that is caused by this problem is when streaming
> video out of a UVC gadget, only the first video frame is transferred.
>
> For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> set manually by the driver. This was previously done in musb_g_tx
> (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
> some requests to be transferred correctly, but multiple requests were
> often put together in one USB packet, and caused problems if the packet
> size was not a multiple of 4.
>
> Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
> just like host mode transfers, then musb_g_tx forces the packet to be
> flushed, by setting MUSB_TXCSR_FLUSHFIFO.
>
> This topic was originally tackled by Nicolas Boichat [0] [1] and is discussed
this line is longer than 75 chars.
> further at [2] as part of his GSoC project [3].
>
> [0] https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> [1]
> https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> [2]
> http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
>
> I have forward-ported this patch from 2.6.34 to 4.19-rc1.
this line is irrelevant to the commit message, please move it down to
under '---'.
>
> Signed-off-by: Paul Elder
> ---
> drivers/usb/musb/musb_gadget.c | 21 ++---
> drivers/usb/musb/musbhsdma.c | 21 +++--
> 2 files changed, 25 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> index eae8b1b1b45b..d3f33f449445 100644
> --- a/drivers/usb/musb/musb_gadget.c
> +++ b/drivers/usb/musb/musb_gadget.c
> @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> && (request->actual == request->length))
> short_packet = true;
>
> - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
> - (is_dma && (!dma->desired_mode ||
> - (request->actual &
> - (musb_ep->packet_sz - 1)
> - short_packet = true;
> + if (is_dma && (musb_dma_inventra(musb) ||
> musb_dma_ux500(musb))) {
more than 80 chars.
> + if (!dma->desired_mode ||
I understand you forward-port Nicolas' patch, but do you have a specific
readon to re-write this 'if' condition? I'd like to see minimum code
change in bug fixes,
> + request->actual % musb_ep->packet_sz) {
but I like this version though, easier to read.
> + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
what is 'KPB'? the message could be more meaningful?
> + musb_ep->end_point.name);
> + musb_writew(epio, MUSB_TXCSR,
> + csr | MUSB_TXCSR_FLUSHFIFO);
What if without this line? The short packet doesn't send out? setting
TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is
only used for aborting cases.
> + return;
> + }
> + }
>
> if (short_packet) {
> /*
> @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> if (csr & MUSB_TXCSR_TXPKTRDY)
> return;
>
> - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
> - | MUSB_TXCSR_TXPKTRDY);
> + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d,
> actual=%d, dma->desired_mode=%d)\n",
> + request->zero, request->length,
> request->actual,
more than 80 chars.
> + dma->desired_mode);
> + musb_writew(epio, MUSB_TXCSR, csr |
> MUSB_TXCSR_TXPKTRDY);
more than 80 chars.
> request->zero = 0;
> }
>
> diff --git a/drivers/usb/musb/musbhsdma.c b/drivers/usb/musb/musbhsdma.c
> index a688f7f87829..e514d4700a6b 100644
> --- a/drivers/usb/musb/musbhsdma.c
> +++ b/drivers/usb/musb/musbhsdma.c
> @@ -346,12 +346,10 @@ static irqreturn_t dma_controller_irq(int ir
[PATCH] usb: storage: Remove outdated URL from MAINTAINERS
This website hasn't worked for quite some time. Signed-off-by: David Brown Cc: Matt Dharm --- MAINTAINERS | 1 - 1 file changed, 1 deletion(-) diff --git a/MAINTAINERS b/MAINTAINERS index 82ad0eabce4f..0dbba14128e4 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -14142,7 +14142,6 @@ M: Alan Stern L: linux-usb@vger.kernel.org L: usb-stor...@lists.one-eyed-alien.net S: Maintained -W: http://www.one-eyed-alien.net/~mdharm/linux-usb/ F: drivers/usb/storage/ USB MIDI DRIVER -- 2.19.2
Re: r8152: data corruption in various scenarios
On 2019-01-07 1:27 p.m., mario.limoncie...@dell.com wrote: .. > The xHCI overrun workaround should only be applied on TB16/TB16, correct. > > Can you double check the verbose information from lsusb for the r8153 device > on your WD15? Sure, see below for the full output. > If it's the same information as the TB16 (which it sounds like it is) Kai > Heng and I will check > around internally to find out why they're looking the same. Thanks. > My second guess would be maybe newer ethernet NVM in manufacturing. > My third guess would be a manufacturing issue putting wrong NVM image on your > WD15. It could be one of those two things. Let us know what you discover. Thanks Bus 004 Device 003: ID 0bda:8153 Realtek Semiconductor Corp. Device Descriptor: bLength18 bDescriptorType 1 bcdUSB 3.00 bDeviceClass0 (Defined at Interface level) bDeviceSubClass 0 bDeviceProtocol 0 bMaxPacketSize0 9 idVendor 0x0bda Realtek Semiconductor Corp. idProduct 0x8153 bcdDevice 30.11 iManufacturer 1 Realtek iProduct2 USB 10/100/1000 LAN iSerial 6 0200 bNumConfigurations 2 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 57 bNumInterfaces 1 bConfigurationValue 1 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 64mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 3 bInterfaceClass 255 Vendor Specific Class bInterfaceSubClass255 Vendor Specific Subclass bInterfaceProtocol 0 iInterface 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x81 EP 1 IN bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 3 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x02 EP 2 OUT bmAttributes2 Transfer TypeBulk Synch Type None Usage Type Data wMaxPacketSize 0x0400 1x 1024 bytes bInterval 0 bMaxBurst 3 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0002 1x 2 bytes bInterval 8 bMaxBurst 0 Configuration Descriptor: bLength 9 bDescriptorType 2 wTotalLength 98 bNumInterfaces 2 bConfigurationValue 2 iConfiguration 0 bmAttributes 0xa0 (Bus Powered) Remote Wakeup MaxPower 64mA Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber0 bAlternateSetting 0 bNumEndpoints 1 bInterfaceClass 2 Communications bInterfaceSubClass 6 Ethernet Networking bInterfaceProtocol 0 iInterface 5 CDC Communications Control CDC Header: bcdCDC 1.10 CDC Union: bMasterInterface0 bSlaveInterface 1 CDC Ethernet: iMacAddress 3 54BF6450FC4F bmEthernetStatistics0x wMaxSegmentSize 1514 wNumberMCFilters0x bNumberPowerFilters 0 Endpoint Descriptor: bLength 7 bDescriptorType 5 bEndpointAddress 0x83 EP 3 IN bmAttributes3 Transfer TypeInterrupt Synch Type None Usage Type Data wMaxPacketSize 0x0010 1x 16 bytes bInterval 8 bMaxBurst 0 Interface Descriptor: bLength 9 bDescriptorType 4 bInterfaceNumber1 bAlternateSetting 0 bNumEndpoints 0 bInterfaceClass10 CDC Data bInterfaceSubClass 0 Unused bInterfaceProtocol 0 iInterface 0 Interface Descriptor:
Re: [PATCH 2/4] usb: musb: jz4740: Add support for devicetree
Hi,
On Thu, Dec 13, 2018 at 03:45:53PM +0100, Paul Cercueil wrote:
> Add support for probing the driver from devicetree.
>
> Signed-off-by: Paul Cercueil
> ---
> drivers/usb/musb/jz4740.c | 7 +++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/usb/musb/jz4740.c b/drivers/usb/musb/jz4740.c
> index 04d8b2bc205a..9a2cebcac260 100644
> --- a/drivers/usb/musb/jz4740.c
> +++ b/drivers/usb/musb/jz4740.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -188,11 +189,17 @@ static int jz4740_remove(struct platform_device *pdev)
> return 0;
> }
>
> +static const struct of_device_id jz4740_musb_of_match[] = {
> + { .compatible = "ingenic,jz4740-musb" },
> + {},
> +};
should be wrapped by '#ifdef CONFIG_OF'?
miss MODULE_DEVICE_TABLE()
> +
> static struct platform_driver jz4740_driver = {
> .probe = jz4740_probe,
> .remove = jz4740_remove,
> .driver = {
> .name = "musb-jz4740",
> + .of_match_table = of_match_ptr(jz4740_musb_of_match),
> },
> };
>
Regards,
-Bin.
Re: [PATCH 3/4] usb: musb: jz4740: Drop dependency on MACH_JZ4740, use COMPILE_TEST
Hi, Please use the following subject instead. usb: musb: Kconfig: Drop dependency on MACH_JZ4740 and use COMPILE_TEST for jz4740 On Thu, Dec 13, 2018 at 03:45:54PM +0100, Paul Cercueil wrote: > Depending on MACH_INGENIC prevent us from creating a generic kernel that did you mean MACH_JZ4740 instead? Regards, -Bin.
Re: [PATCH 4/4] usb: musb: jz4740: Drop dependency on USB_OTG_BLACKLIST_HUB
Hi, Please use the following subject instead. usb: musb: Kconfig: Drop dependency on USB_OTG_BLACKLIST_HUB for jz4740 Regards, -Bin. On Thu, Dec 13, 2018 at 03:45:55PM +0100, Paul Cercueil wrote: > The USB IP in the JZ4740 SoC does not support host mode, only gadget > mode. > > Signed-off-by: Paul Cercueil > --- > drivers/usb/musb/Kconfig | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig > index 6f5b0ed6a507..a884179bc3c0 100644 > --- a/drivers/usb/musb/Kconfig > +++ b/drivers/usb/musb/Kconfig > @@ -113,7 +113,6 @@ config USB_MUSB_JZ4740 > depends on NOP_USB_XCEIV > depends on MIPS || COMPILE_TEST > depends on USB_MUSB_GADGET > - depends on USB_OTG_BLACKLIST_HUB > > config USB_MUSB_AM335X_CHILD > tristate > -- > 2.11.0 >
Re: [PATCH] USB: musb: fix indentation issue on a return statement
Hi, On Fri, Jan 04, 2019 at 05:59:41PM +, Colin King wrote: > From: Colin Ian King > > A return statement is indented one level too far, fix this by removing > a tab. > > Signed-off-by: Colin Ian King > --- > drivers/usb/musb/musb_host.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) Applied. Thanks. Regards, -Bin.
Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings
Hi Jorge, Sorry for the late reply as I was out during the holiday break. On Fri, Dec 28, 2018 at 01:38:59PM +0100, Jorge Ramirez wrote: > On 12/20/18 18:37, Jack Pham wrote: > >Hi Rob, Jorge, > > > >On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote: > >>On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote: > >>>Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY > >>>controller embedded in QCS404. > >>> > >>>Based on Sriharsha Allenki's original > >>>definitions. > >>> > >>>Signed-off-by: Jorge Ramirez-Ortiz > >>>Reviewed-by: Vinod Koul > >>>--- > >>> .../devicetree/bindings/usb/qcom,usb-ssphy.txt | 78 > >>> ++ > >>> 1 file changed, 78 insertions(+) > >>> create mode 100644 > >>> Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > >>> > >>>diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > >>>b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > >>>new file mode 100644 > >>>index 000..fcf4e01 > >>>--- /dev/null > >>>+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > >>>@@ -0,0 +1,78 @@ > >>>+Qualcomm Synopsys 1.0.0 SS phy controller > >>>+=== > >>>+ > >>>+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on Qualcomm > >>>+chipsets > >>>+ > >>>+Required properties: > >>>+ > >>>+- compatible: > >>>+Value type: > >>>+Definition: Should contain "qcom,usb-ssphy". > >> > >>What is "qcom,dwc3-ss-usb-phy" which already exists then? > > > >Uh, apparently only the bindings doc is there but the driver never > >landed. I guess it fell through the cracks nearly 4 years ago. > > > >https://lore.kernel.org/patchwork/patch/499502/ > > > >Jorge, does Andy's version of this driver at all resemble what can be > >used for QCS404? > > on close inspection I cant see any similitudes between the drivers. > Unfortunately I don't have access to documentation yet but the > control register offsets and the control bits in the drivers do not > match. > > because of the above I'd like to go ahead with our separate drivers > -already tested and validated- for HS (Shawn's) and SS (mine). > > if that is acceptable, should we reuse the upstream bindings for > our implementation? or perhaps Shawn Guo will do for his HS version > of the driver and I go ahead and create a new one? what would you > suggest? I'm not really sure. My understanding of the driver Andy submitted were for some of the older MSM and IPQ SoCs that implemented the PHY controls as part of the DWC3 controller's "QScratch" registers, which is why the bindings doc and the compatible string reference "dwc3" in both the compatible and the docs filename. Is the SNPS PHY on QCS404 architected similarly in this regard? Either way, the existing bindings doc for the non-existent driver looks incomplete for QCS404, so you'd have to update it anyway. My feeling is that there should just be one document describing all variants of SNPS PHYs on Qualcomm chips. Maybe we should also just delete the "qcom,dwc3-ss-usb-phy" binding unless there is a plan to resurrect Andy's driver. Jack -- The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project
Re: [PATCH 1/2] dt-bindings: Add Qualcomm USB Super-Speed PHY bindings
On Mon, 7 Jan 2019 at 14:26, Jack Pham wrote: > > Hi Jorge, > > Sorry for the late reply as I was out during the holiday break. > > On Fri, Dec 28, 2018 at 01:38:59PM +0100, Jorge Ramirez wrote: > > On 12/20/18 18:37, Jack Pham wrote: > > >Hi Rob, Jorge, > > > > > >On Thu, Dec 20, 2018 at 11:05:31AM -0600, Rob Herring wrote: > > >>On Fri, Dec 07, 2018 at 10:55:57AM +0100, Jorge Ramirez-Ortiz wrote: > > >>>Binding description for Qualcomm's Synopsys 1.0.0 super-speed PHY > > >>>controller embedded in QCS404. > > >>> > > >>>Based on Sriharsha Allenki's original > > >>>definitions. > > >>> > > >>>Signed-off-by: Jorge Ramirez-Ortiz > > >>>Reviewed-by: Vinod Koul > > >>>--- > > >>> .../devicetree/bindings/usb/qcom,usb-ssphy.txt | 78 > > >>> ++ > > >>> 1 file changed, 78 insertions(+) > > >>> create mode 100644 > > >>> Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > > >>> > > >>>diff --git a/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > > >>>b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > > >>>new file mode 100644 > > >>>index 000..fcf4e01 > > >>>--- /dev/null > > >>>+++ b/Documentation/devicetree/bindings/usb/qcom,usb-ssphy.txt > > >>>@@ -0,0 +1,78 @@ > > >>>+Qualcomm Synopsys 1.0.0 SS phy controller > > >>>+=== > > >>>+ > > >>>+Synopsys 1.0.0 ss phy controller supports SS usb connectivity on > > >>>Qualcomm > > >>>+chipsets > > >>>+ > > >>>+Required properties: > > >>>+ > > >>>+- compatible: > > >>>+Value type: > > >>>+Definition: Should contain "qcom,usb-ssphy". > > >> > > >>What is "qcom,dwc3-ss-usb-phy" which already exists then? > > > > > >Uh, apparently only the bindings doc is there but the driver never > > >landed. I guess it fell through the cracks nearly 4 years ago. > > > > > >https://lore.kernel.org/patchwork/patch/499502/ > > > > > >Jorge, does Andy's version of this driver at all resemble what can be > > >used for QCS404? > > > > on close inspection I cant see any similitudes between the drivers. > > Unfortunately I don't have access to documentation yet but the > > control register offsets and the control bits in the drivers do not > > match. > > > > because of the above I'd like to go ahead with our separate drivers > > -already tested and validated- for HS (Shawn's) and SS (mine). > > > > if that is acceptable, should we reuse the upstream bindings for > > our implementation? or perhaps Shawn Guo will do for his HS version > > of the driver and I go ahead and create a new one? what would you > > suggest? > > I'm not really sure. My understanding of the driver Andy submitted > were for some of the older MSM and IPQ SoCs that implemented the PHY > controls as part of the DWC3 controller's "QScratch" registers, which is > why the bindings doc and the compatible string reference "dwc3" in both > the compatible and the docs filename. Is the SNPS PHY on QCS404 > architected similarly in this regard? Either way, the existing bindings > doc for the non-existent driver looks incomplete for QCS404, so you'd > have to update it anyway. My feeling is that there should just be one > document describing all variants of SNPS PHYs on Qualcomm chips. Yeah the original driver was specifically for the IPQ8064 phys. The actual phy driver changed over time due to some comments from a few people, but it still used the qscratch memory for the phy control regs. Due to this never landing, you can change the phy binding to work for both of them or just for yours. If the control regs are totally different for the QCS404 phy, it should use a different compatible and driver. That's my 2 cents. > Maybe we should also just delete the "qcom,dwc3-ss-usb-phy" binding > unless there is a plan to resurrect Andy's driver. I have the hardware I just don't have the time in the short-mid term to resurrect this. Unless someone else wants to pick this up, it'll be a while. In the meantime, I'd suggest just changing the binding to apply to the QCS404 if that makes sense (completely different IP / register layout). Andy
Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
Hi,
On Thu, Dec 27, 2018 at 03:34:23PM +0800, min@mediatek.com wrote:
> From: Min Guo
>
> This adds support for MediaTek musb controller in
> host, peripheral and otg mode
>
> Signed-off-by: Min Guo
> ---
> .../devicetree/bindings/usb/mediatek,musb.txt | 49
> ++
> 1 file changed, 49 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> new file mode 100644
> index 000..e899c9b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> @@ -0,0 +1,49 @@
> +MediaTek musb DRC/OTG controller
s/DRC/DRD/
> +---
> +
> +Required properties:
[snip]
> +Example:
> +
> +usb2: usb@1120 {
> + compatible = "mediatek,mt2701-musb";
s/;/,/
> + "mediatek,mtk-musb";
Regards,
-Bin.
[PATCH V3] usb:xhci: Fix for Enabling USB ROLE SWITCH QUIRK on INTEL_SUNRISEPOINT_LP_XHCI
From: Balaji Manoharan This fix enables USB role feature on intel commercial nuc platform which is based on Kabylake chipset. Signed-off-by: Balaji Manoharan --- Changes from v2: changed initals to full name Changes from v1: changed patch subject from roles: to usb:xhci drivers/usb/host/xhci-pci.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c index a9ec7051f286..c2fe218e051f 100644 --- a/drivers/usb/host/xhci-pci.c +++ b/drivers/usb/host/xhci-pci.c @@ -194,6 +194,7 @@ static void xhci_pci_quirks(struct device *dev, struct xhci_hcd *xhci) xhci->quirks |= XHCI_SSIC_PORT_UNUSED; if (pdev->vendor == PCI_VENDOR_ID_INTEL && (pdev->device == PCI_DEVICE_ID_INTEL_CHERRYVIEW_XHCI || +pdev->device == PCI_DEVICE_ID_INTEL_SUNRISEPOINT_LP_XHCI || pdev->device == PCI_DEVICE_ID_INTEL_APL_XHCI)) xhci->quirks |= XHCI_INTEL_USB_ROLE_SW; if (pdev->vendor == PCI_VENDOR_ID_INTEL && -- 2.17.1
Re: [PATCH v3] usb: hub: add retry routine after intr URB submit error
On Mo, 2019-01-07 at 13:59 -0500, Alan Stern wrote: > On Mon, 7 Jan 2019, Greg KH wrote: > > > > What ever happened to this patch? Is it still needed? Oliver and Alan, > > any objections about it anymore? > > I have just one very minor nit to pick (see below). Mostly I've been > waiting to hear from Oliver. Sorry, I have been away over Christmas. It is looking good to me. Regards Oliver
Re: [PATCH 1/4] dt-bindings: usb: musb: Add support for MediaTek musb controller
On Mon, 2019-01-07 at 14:40 -0600, Bin Liu wrote:
> Hi,
>
> On Thu, Dec 27, 2018 at 03:34:23PM +0800, min@mediatek.com wrote:
> > From: Min Guo
> >
> > This adds support for MediaTek musb controller in
> > host, peripheral and otg mode
> >
> > Signed-off-by: Min Guo
> > ---
> > .../devicetree/bindings/usb/mediatek,musb.txt | 49
> > ++
> > 1 file changed, 49 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/usb/mediatek,musb.txt
> >
> > diff --git a/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > new file mode 100644
> > index 000..e899c9b
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/mediatek,musb.txt
> > @@ -0,0 +1,49 @@
> > +MediaTek musb DRC/OTG controller
>
> s/DRC/DRD/
I will modify it in next patch.
> > +---
> > +
> > +Required properties:
>
> [snip]
>
> > +Example:
> > +
> > +usb2: usb@1120 {
> > + compatible = "mediatek,mt2701-musb";
>
> s/;/,/
I will modify it in next patch.
> > + "mediatek,mtk-musb";
>
> Regards,
> -Bin.
Re: [PATCH] usb: gadget: musb: fix short isoc packets with inventra dma for pandaboard es
Hi Bin,
On Mon, Jan 07, 2019 at 01:11:57PM -0600, Bin Liu wrote:
> Hi Paul,
>
> Sorry for the delay on reviewing it.
Thanks for the review.
> For the subject, can you please use
>
> usb: musb: gadget: fix short isoc packets with inventra dma
ack
> On Tue, Oct 09, 2018 at 02:32:20AM -0400, Paul Elder wrote:
> > Handling short packets (length < max packet size) in the Inventra DMA
> > engine in the MUSB driver causes the MUSB DMA controller to hang. An
> > example of a problem that is caused by this problem is when streaming
> > video out of a UVC gadget, only the first video frame is transferred.
> >
> > For short packets (mode-0 or mode-1 DMA), MUSB_TXCSR_TXPKTRDY must be
> > set manually by the driver. This was previously done in musb_g_tx
> > (musb_gadget.c), but incorrectly (all csr flags were cleared, and only
> > MUSB_TXCSR_MODE and MUSB_TXCSR_TXPKTRDY). Fixing that problem allows
> > some requests to be transferred correctly, but multiple requests were
> > often put together in one USB packet, and caused problems if the packet
> > size was not a multiple of 4.
> >
> > Instead, MUSB_TXCSR_TXPKTRDY is set in dma_controller_irq (musbhsdma.c),
> > just like host mode transfers, then musb_g_tx forces the packet to be
> > flushed, by setting MUSB_TXCSR_FLUSHFIFO.
> >
> > This topic was originally tackled by Nicolas Boichat [0] [1] and is
> > discussed
>
> this line is longer than 75 chars.
ack
> > further at [2] as part of his GSoC project [3].
> >
> > [0]
> > https://groups.google.com/forum/?hl=en#!topic/beagleboard-gsoc/k8Azwfp75CU
> > [1]
> > https://gitorious.org/beagleboard-usbsniffer/beagleboard-usbsniffer-kernel/commit/b0be3b6cc195ba732189b04f1d43ec843c3e54c9?p=beagleboard-usbsniffer:beagleboard-usbsniffer-kernel.git;a=patch;h=b0be3b6cc195ba732189b04f1d43ec843c3e54c9
> > [2]
> > http://beagleboard-usbsniffer.blogspot.com/2010/07/musb-isochronous-transfers-fixed.html
> > [3] http://elinux.org/BeagleBoard/GSoC/USBSniffer
> >
> > I have forward-ported this patch from 2.6.34 to 4.19-rc1.
>
> this line is irrelevant to the commit message, please move it down to
> under '---'.
ack
> >
> > Signed-off-by: Paul Elder
> > ---
> > drivers/usb/musb/musb_gadget.c | 21 ++---
> > drivers/usb/musb/musbhsdma.c | 21 +++--
> > 2 files changed, 25 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
> > index eae8b1b1b45b..d3f33f449445 100644
> > --- a/drivers/usb/musb/musb_gadget.c
> > +++ b/drivers/usb/musb/musb_gadget.c
> > @@ -479,11 +479,16 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > && (request->actual == request->length))
> > short_packet = true;
> >
> > - if ((musb_dma_inventra(musb) || musb_dma_ux500(musb)) &&
> > - (is_dma && (!dma->desired_mode ||
> > - (request->actual &
> > - (musb_ep->packet_sz - 1)
> > - short_packet = true;
> > + if (is_dma && (musb_dma_inventra(musb) ||
> > musb_dma_ux500(musb))) {
>
> more than 80 chars.
>
> > + if (!dma->desired_mode ||
>
> I understand you forward-port Nicolas' patch, but do you have a specific
> readon to re-write this 'if' condition? I'd like to see minimum code
> change in bug fixes,
>
> > + request->actual % musb_ep->packet_sz) {
>
> but I like this version though, easier to read.
>
> > + musb_dbg(musb, "%s Flushing (FIFO) EP : KPB\n",
>
> what is 'KPB'? the message could be more meaningful?
>
> > + musb_ep->end_point.name);
> > + musb_writew(epio, MUSB_TXCSR,
> > + csr | MUSB_TXCSR_FLUSHFIFO);
>
> What if without this line? The short packet doesn't send out? setting
> TXSCR_TXPKTRDY in the dma driver is not sufficient? TXSCR_FLUSHFIFO is
> only used for aborting cases.
I just tested this and you are right, it does work without this line.
Since this is the only significant line in this very complex if block,
I'll just remove this entire block too.
> > + return;
> > + }
> > + }
> >
> > if (short_packet) {
> > /*
> > @@ -493,8 +498,10 @@ void musb_g_tx(struct musb *musb, u8 epnum)
> > if (csr & MUSB_TXCSR_TXPKTRDY)
> > return;
> >
> > - musb_writew(epio, MUSB_TXCSR, MUSB_TXCSR_MODE
> > - | MUSB_TXCSR_TXPKTRDY);
> > + musb_dbg(musb, "sending zero pkt (zero=%d, length=%d,
> > actual=%d, dma->desired_mode=%d)\n",
> > +request->zero, request->length,
> > request->actual,
>
> more than 80 chars.
The format string or the paramet
Re: [PATCH v4 5/6] usb: musb: gadget: implement optional explicit status stage
On Sun, Jan 06, 2019 at 03:03:09PM -0500, Alan Stern wrote: > On Sun, 6 Jan 2019, Paul Elder wrote: > > > Implement the mechanism for optional explicit status stage for the MUSB > > driver. This allows a function driver to specify what to reply for the > > status stage. The functionality for an implicit status stage is > > retained. > > > > Signed-off-by: Paul Elder > > v1 Reviewed-by: Laurent Pinchart > > v1 Acked-by: Bin Liu > > --- > > No change from v3 > > > > Changes from v2: > > - update call to usb_gadget_control_complete to include status > > - since sending STALL from the function driver is now done with > > usb_ep_set_halt, there is no need for the internal ep0_send_response to > > take a stall/ack parameter; remove the parameter and make the function > > only send ack, and remove checking for the status reply in the > > usb_request for the status stage > > > > Changes from v1: > > - obvious change to implement v2 mechanism laid out by 4/6 of this > > series (send_response, and musb_g_ep0_send_response function has > > been removed, call to usb_gadget_control_complete has been added) > > - ep0_send_response's ack argument has been changed from stall > > - last_packet flag in ep0_rxstate has been removed, since it is equal to > > req != NULL > > > > drivers/usb/musb/musb_gadget.c | 2 ++ > > drivers/usb/musb/musb_gadget_ep0.c | 23 +++ > > 2 files changed, 25 insertions(+) > > > > diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c > > index d3f33f449445..a7a992ab0c9d 100644 > > --- a/drivers/usb/musb/musb_gadget.c > > +++ b/drivers/usb/musb/musb_gadget.c > > @@ -145,6 +145,8 @@ __acquires(ep->musb->lock) > > > > trace_musb_req_gb(req); > > usb_gadget_giveback_request(&req->ep->end_point, &req->request); > > + usb_gadget_control_complete(&musb->g, request->explicit_status, > > + request->status); > > I haven't paid much attention to this part of the patch series, not > knowing much about musb. Still, it's clear that > usb_gadget_control_complete should be called only for transfers on > ep0. You need to test the endpoint value. Oh oops, yeah, you're right. > Another problem: the completion handler may deallocate the request. > Dereferencing request->expicit_status and request->status would then > cause errors. Would it be preferable to call > usb_gadget_control_complete before usb_gadget_giveback_request? If > it gets done that way then the arguments could be simplified: we could > pass a pointer to the request instead of the separate explicit_status > and status values. I thought that usb_gadget_control_complete needs to check the status of the request that was given back. Doesn't that mean that usb_gadget_giveback_request must be called first? I was thinking that maybe we could save explicit_status before calling usb_gadget_giveback_request, and if request is still valid then we can pull status from it otherwise use 0, but then would that be considered adding complexity to UDCs that want to implement optional status stage delay? Or add a wrapper function? On the other hand, if we do put usb_gadget_control_complete before usb_gadget_giveback_request, then the control transfer would complete before the function driver has a chance to complete, but if the function driver wanted to intervene/determine the status stage then it would have gone through the new mechanism that we're making here. So it could be fine to switch the order. My tests for it work too, so I guess we'll go with usb_gadget_control_complete before usb_gadget_giveback_request then. In that case usb_gadget_control_complete doesn't need to check the status of the request, since there isn't any, right? Thanks, Paul Elder
[PATCH 1/3] usb: kconfig: remove dependency FSL_SOC for ehci fsl driver
From: Rajesh Bhagat CONFIG_USB_EHCI_FSL is not dependent on FSL_SOC, it can be built on non-PPC platforms. Signed-off-by: Rajesh Bhagat Signed-off-by: Ran Wang --- drivers/usb/host/Kconfig |4 ++-- 1 files changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/usb/host/Kconfig b/drivers/usb/host/Kconfig index 16758b1..53cbc0c 100644 --- a/drivers/usb/host/Kconfig +++ b/drivers/usb/host/Kconfig @@ -179,8 +179,8 @@ config XPS_USB_HCD_XILINX devices only. config USB_EHCI_FSL - tristate "Support for Freescale PPC on-chip EHCI USB controller" - depends on FSL_SOC + tristate "Support for Freescale on-chip EHCI USB controller" + depends on USB_EHCI_HCD select USB_EHCI_ROOT_HUB_TT ---help--- Variation of ARC USB block used in some Freescale chips. -- 1.7.1
[PATCH 2/3] usb: ehci: fsl: Update register accessing for arm/arm64 platforms
arm/arm64's io.h doesn't define clrbits32() and clrsetbits_be32(), which
causing compile failure on some Layerscape Platforms (such as LS1021A and
LS2012A which also integrates FSL EHCI controller). So use
ioread32be()/iowrite32be() instead to make it workable on both
powerpc and arm.
Signed-off-by: Ran Wang
---
drivers/usb/host/ehci-fsl.c | 64 ---
1 files changed, 42 insertions(+), 22 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 0a9fd20..59ebe1b 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include "ehci.h"
#include "ehci-fsl.h"
@@ -50,6 +51,7 @@ static int fsl_ehci_drv_probe(struct platform_device *pdev)
struct resource *res;
int irq;
int retval;
+ u32 tmp;
pr_debug("initializing FSL-SOC USB Controller\n");
@@ -114,18 +116,23 @@ static int fsl_ehci_drv_probe(struct platform_device
*pdev)
}
/* Enable USB controller, 83xx or 8536 */
- if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6)
- clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
- CONTROL_REGISTER_W1C_MASK, 0x4);
-
+ if (pdata->have_sysif_regs && pdata->controller_ver < FSL_USB_VER_1_6) {
+ tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
+ tmp &= ~CONTROL_REGISTER_W1C_MASK;
+ tmp |= 0x4;
+ iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
+ }
/*
* Enable UTMI phy and program PTS field in UTMI mode before asserting
* controller reset for USB Controller version 2.5
*/
if (pdata->has_fsl_erratum_a007792) {
- clrsetbits_be32(hcd->regs + FSL_SOC_USB_CTRL,
- CONTROL_REGISTER_W1C_MASK, CTRL_UTMI_PHY_EN);
- writel(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
+ tmp = ioread32be(hcd->regs + FSL_SOC_USB_CTRL);
+ tmp &= ~CONTROL_REGISTER_W1C_MASK;
+ tmp |= CTRL_UTMI_PHY_EN;
+ iowrite32be(tmp, hcd->regs + FSL_SOC_USB_CTRL);
+
+ iowrite32be(PORT_PTS_UTMI, hcd->regs + FSL_SOC_USB_PORTSC1);
}
/* Don't need to set host mode here. It will be done by tdi_reset() */
@@ -174,7 +181,7 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
enum fsl_usb2_phy_modes phy_mode,
unsigned int port_offset)
{
- u32 portsc;
+ u32 portsc, tmp;
struct ehci_hcd *ehci = hcd_to_ehci(hcd);
void __iomem *non_ehci = hcd->regs;
struct device *dev = hcd->self.controller;
@@ -192,11 +199,16 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
case FSL_USB2_PHY_ULPI:
if (pdata->have_sysif_regs && pdata->controller_ver) {
/* controller version 1.6 or above */
- clrbits32(non_ehci + FSL_SOC_USB_CTRL,
- CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
- clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
- CONTROL_REGISTER_W1C_MASK,
- ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN);
+ /* turn off UTMI PHY first */
+ tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+ tmp &= ~(CONTROL_REGISTER_W1C_MASK | UTMI_PHY_EN);
+ iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+
+ /* then turn on ULPI and enable USB controller */
+ tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+ tmp &= ~(CONTROL_REGISTER_W1C_MASK);
+ tmp |= ULPI_PHY_CLK_SEL | USB_CTRL_USB_EN;
+ iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
}
portsc |= PORT_PTS_ULPI;
break;
@@ -210,16 +222,21 @@ static int ehci_fsl_setup_phy(struct usb_hcd *hcd,
case FSL_USB2_PHY_UTMI_DUAL:
if (pdata->have_sysif_regs && pdata->controller_ver) {
/* controller version 1.6 or above */
- clrsetbits_be32(non_ehci + FSL_SOC_USB_CTRL,
- CONTROL_REGISTER_W1C_MASK, UTMI_PHY_EN);
+ tmp = ioread32be(non_ehci + FSL_SOC_USB_CTRL);
+ tmp &= ~(CONTROL_REGISTER_W1C_MASK);
+ tmp |= UTMI_PHY_EN;
+ iowrite32be(tmp, non_ehci + FSL_SOC_USB_CTRL);
+
mdelay(FSL_UTMI_PHY_DLY); /* Delay for UTMI PHY CLK to
become stable - 10ms*/
}
/* enable UTMI PHY */
- if (pdata->have_sysif_regs)
-
[PATCH 3/3] drivers: usb :fsl: Remove USB Errata checking code
From: yinbo.zhu
Remove USB errata checking code from driver. Applicability of erratum
is retrieved by reading corresponding property in device tree.
This property is written during device tree fixup.
Signed-off-by: Ramneek Mehresh
Signed-off-by: Nikhil Badola
Signed-off-by: yinbo.zhu
Signed-off-by: Ran Wang
---
drivers/usb/host/ehci-fsl.c |7 +--
drivers/usb/host/fsl-mph-dr-of.c |6 ++
include/linux/fsl_devices.h |7 ---
3 files changed, 11 insertions(+), 9 deletions(-)
diff --git a/drivers/usb/host/ehci-fsl.c b/drivers/usb/host/ehci-fsl.c
index 59ebe1b..2aa408a 100644
--- a/drivers/usb/host/ehci-fsl.c
+++ b/drivers/usb/host/ehci-fsl.c
@@ -304,14 +304,9 @@ static int ehci_fsl_usb_setup(struct ehci_hcd *ehci)
return -EINVAL;
if (pdata->operating_mode == FSL_USB2_MPH_HOST) {
- unsigned int chip, rev, svr;
-
- svr = mfspr(SPRN_SVR);
- chip = svr >> 16;
- rev = (svr >> 4) & 0xf;
/* Deal with USB Erratum #14 on MPC834x Rev 1.0 & 1.1 chips */
- if ((rev == 1) && (chip >= 0x8050) && (chip <= 0x8055))
+ if (pdata->has_fsl_erratum_14 == 1)
ehci->has_fsl_port_bug = 1;
if (pdata->port_enables & FSL_USB2_PORT0_ENABLED)
diff --git a/drivers/usb/host/fsl-mph-dr-of.c b/drivers/usb/host/fsl-mph-dr-of.c
index 677f9d5..4f8b8a0 100644
--- a/drivers/usb/host/fsl-mph-dr-of.c
+++ b/drivers/usb/host/fsl-mph-dr-of.c
@@ -225,6 +225,12 @@ static int fsl_usb2_mph_dr_of_probe(struct platform_device
*ofdev)
pdata->has_fsl_erratum_a005697 =
of_property_read_bool(np, "fsl,usb_erratum-a005697");
+ if (of_get_property(np, "fsl,usb_erratum_14", NULL))
+ pdata->has_fsl_erratum_14 = 1;
+ else
+ pdata->has_fsl_erratum_14 = 0;
+
+
/*
* Determine whether phy_clk_valid needs to be checked
* by reading property in device tree
diff --git a/include/linux/fsl_devices.h b/include/linux/fsl_devices.h
index 60cef82..7aa51bc 100644
--- a/include/linux/fsl_devices.h
+++ b/include/linux/fsl_devices.h
@@ -98,10 +98,11 @@ struct fsl_usb2_platform_data {
unsignedsuspended:1;
unsignedalready_suspended:1;
- unsignedhas_fsl_erratum_a007792:1;
- unsignedhas_fsl_erratum_a005275:1;
+ unsignedhas_fsl_erratum_a007792:1;
+ unsignedhas_fsl_erratum_14:1;
+ unsignedhas_fsl_erratum_a005275:1;
unsignedhas_fsl_erratum_a005697:1;
- unsignedcheck_phy_clk_valid:1;
+ unsignedcheck_phy_clk_valid:1;
/* register save area for suspend/resume */
u32 pm_command;
--
1.7.1
Re: [PATCH v4] USB: Don't enable LPM if it's already enabled
> On Jan 8, 2019, at 00:16, Greg KH wrote:
>
> On Mon, Dec 03, 2018 at 06:26:43PM +0800, Kai-Heng Feng wrote:
>> USB Bluetooth controller QCA ROME (0cf3:e007) sometimes stops working
>> after S3:
>> [ 165.110742] Bluetooth: hci0: using NVM file: qca/nvm_usb_0302.bin
>> [ 168.432065] Bluetooth: hci0: Failed to send body at 4 of 1953 (-110)
>>
>> After some experiments, I found that disabling LPM can workaround the
>> issue.
>>
>> On some platforms, the USB power is cut during S3, so the driver uses
>> reset-resume to resume the device. During port resume, LPM gets enabled
>> twice, by usb_reset_and_verify_device() and usb_port_resume().
>>
>> So let's enable LPM for just once, as this solves the issue for the
>> device in question.
>>
>> Also consolidate USB2 LPM functions to usb_enable_usb2_hardware_lpm()
>> and usb_disable_usb2_hardware_lpm().
>>
>> Signed-off-by: Kai-Heng Feng
>
> What kernel patch does this one "fix"? Adding a "Fixes:" tag would be
> good to try to figure out how far back in the kernel releases this
> should be backported.
Fixes: de68bab4fa96 ("usb: Don't enable USB 2.0 Link PM by default.”)
The usb_set_usb2_hardware_lpm() was added to usb_reset_and_verify_device()
by this commit.
Kai-Heng
>
> thanks,
>
> greg k-h
