Re: [PATCH v4 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Hi Andy,
On Sun, Nov 15, 2015 at 10:04:10PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 15, 2015 at 8:39 PM, Dave Penkler wrote:
snip
> > +
>
> Redundant empty line.
>
ok
>
> > + data->iin_bTag = 2;
>
> Hmm??? Why 2?
> A-ha, below I found a comment. Something might be good to have here as well.
>
Added comment
> > +
>
> Redundant empty line.
>
ok
> > +
> > + if (data->iin_buffer[0] & 0x80) {
> > + /* check for valid STB notification */
> > + if ((data->iin_buffer[0] & 0x7f) > 1) {
>
> It's the same as
> if (data->iin_buffer[0] & 0x7e) {
>
Yes but when reading the spec and the code it is more obvious that here
we are testing for the value in bits D6..D0 to be a valid iin_bTag return.
(See Table 7 in the USBTMC-USB488 spec.)
> > + dev_dbg(&data->intf->dev,
> > + "%s - urb terminated, status: %d\n",
>
> I heard that dynamic debug adds function name.
>
> > + __func__, status);
I checked in device.h and could not find any trace of it. I'm on 4.4.0-rc1.
> > +static void usbtmc_free_int(struct usbtmc_device_data *data)
> > +{
> > + if (data->iin_ep_present) {
> > + if (data->iin_urb) {
>
> Why not
>
> if (!data->iin_ep_present || !data->iin_urb)
> return;
>
> ?
>
OK
Thanks,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 5/5] Add ioctls to enable and disable local controls on an instrument
On Sun, Nov 15, 2015 at 10:10:35PM +0200, Andy Shevchenko wrote:
> On Sun, Nov 15, 2015 at 8:40 PM, Dave Penkler wrote:
> > These ioctls provide support for the USBTMC-USB488 control requests
> > for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
> >
> > +
> > + dev = &data->intf->dev;
>
> Could be assigned above.
>
ok
> > + wValue = (val) ? 1 : 0;
>
> Redundant parens.
>
ok
> > + rv = -EIO;
>
> Hmm??? Does usb_control_msg() return a proper error code? If it does,
> perhaps better to propagate it.
>
You are right.
> > + rv = -EIO;
> > + goto exit;
>
> > + } else {
>
> Redundant else.
>
OK
>
Thanks,
-Dave
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 0/5] usb: usbtmc: Add support for missing functions in USBTMC-USB488 spec
Implement support for the USB488 defined READ_STATUS_BYTE ioctl (1/5) and SRQ notifications with fasync (2/5) and poll/select (3/5) in order to be able to synchronize with variable duration instrument operations. Add ioctls for other USB488 requests: REN_CONTROL, GOTO_LOCAL and LOCAL_LOCKOUT. (4/5) Add convenience ioctl to return all device capabilities (5/5) PATCH Changelog: v5 - Remove excess newlines and parens - Move dev variable initialisations to declaration - Add comment on interrupt btag wrap - simplify test in usbtmc_free_int() v4 - Remove excess newlines and parens - simplify some expressions v3 - Split into multiple patches as per gregkh request V2 - Fix V1 bug: not waking sleepers on disconnect. - Correct sparse warnings. V1 - Original patch Testing: All functions tested ok on an USBTMC-USB488 compliant oscilloscope Dave Penkler (5): Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation. Add support for USBTMC USB488 SRQ notification with fasync Add support for receiving USBTMC USB488 SRQ notifications via poll/select Add ioctl to retrieve USBTMC-USB488 capabilities Add ioctls to enable and disable local controls on an instrument drivers/usb/class/usbtmc.c | 342 +++ include/uapi/linux/usb/tmc.h | 29 +++- 2 files changed, 368 insertions(+), 3 deletions(-) -- 2.5.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/5] Add support for receiving USBTMC USB488 SRQ notifications via poll/select
Background:
In many situations operations on multiple instruments need to be
synchronized. poll/select provide a convenient way of waiting on a
number of different instruments and other peripherals
simultaneously.
Signed-off-by: Dave Penkler
---
drivers/usb/class/usbtmc.c | 23 +++
1 file changed, 23 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index a95a63d..6294a3c 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -27,6 +27,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1186,6 +1187,27 @@ static int usbtmc_fasync(int fd, struct file *file, int
on)
return fasync_helper(fd, file, on, &data->fasync);
}
+static unsigned int usbtmc_poll(struct file *file, poll_table *wait)
+{
+ struct usbtmc_device_data *data = file->private_data;
+ unsigned int mask;
+
+ mutex_lock(&data->io_mutex);
+
+ if (data->zombie) {
+ mask = POLLHUP | POLLERR;
+ goto no_poll;
+ }
+
+ poll_wait(file, &data->waitq, wait);
+
+ mask = (atomic_read(&data->srq_asserted)) ? POLLIN | POLLRDNORM : 0;
+
+no_poll:
+ mutex_unlock(&data->io_mutex);
+ return mask;
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1194,6 +1216,7 @@ static const struct file_operations fops = {
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
.fasync = usbtmc_fasync,
+ .poll = usbtmc_poll,
.llseek = default_llseek,
};
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 4/5] Add ioctl to retrieve USBTMC-USB488 capabilities
This is a convenience function to obtain an instrument's
capabilities from its file descriptor without having to access sysfs
from the user program.
Signed-off-by: Dave Penkler
---
drivers/usb/class/usbtmc.c | 12
include/uapi/linux/usb/tmc.h | 21 ++---
2 files changed, 30 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 6294a3c..2358991 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -102,6 +102,9 @@ struct usbtmc_device_data {
u16iin_wMaxPacketSize;
atomic_t srq_asserted;
+ /* coalesced usb488_caps from usbtmc_dev_capabilities */
+ u8 usb488_caps;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -995,6 +998,7 @@ static int get_capabilities(struct usbtmc_device_data *data)
data->capabilities.device_capabilities = buffer[5];
data->capabilities.usb488_interface_capabilities = buffer[14];
data->capabilities.usb488_device_capabilities = buffer[15];
+ data->usb488_caps = (buffer[14] & 0x07) | ((buffer[15] & 0x0f) << 4);
rv = 0;
err_out:
@@ -1170,6 +1174,14 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+ case USBTMC488_IOCTL_GET_CAPS:
+ retval = copy_to_user((void __user *)arg,
+ &data->usb488_caps,
+ sizeof(data->usb488_caps));
+ if (retval)
+ retval = -EFAULT;
+ break;
+
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 7e5ced8..1dc3af1 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -2,12 +2,14 @@
* Copyright (C) 2007 Stefan Kopp, Gechingen, Germany
* Copyright (C) 2008 Novell, Inc.
* Copyright (C) 2008 Greg Kroah-Hartman
+ * Copyright (C) 2015 Dave Penkler
*
* This file holds USB constants defined by the USB Device Class
- * Definition for Test and Measurement devices published by the USB-IF.
+ * and USB488 Subclass Definitions for Test and Measurement devices
+ * published by the USB-IF.
*
- * It also has the ioctl definitions for the usbtmc kernel driver that
- * userspace needs to know about.
+ * It also has the ioctl and capability definitions for the
+ * usbtmc kernel driver that userspace needs to know about.
*/
#ifndef __LINUX_USB_TMC_H
@@ -40,6 +42,19 @@
#define USBTMC_IOCTL_ABORT_BULK_IN _IO(USBTMC_IOC_NR, 4)
#define USBTMC_IOCTL_CLEAR_OUT_HALT_IO(USBTMC_IOC_NR, 6)
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
+#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+/* Driver encoded usb488 capabilities */
+#define USBTMC488_CAPABILITY_TRIGGER 1
+#define USBTMC488_CAPABILITY_SIMPLE 2
+#define USBTMC488_CAPABILITY_REN_CONTROL 2
+#define USBTMC488_CAPABILITY_GOTO_LOCAL 2
+#define USBTMC488_CAPABILITY_LOCAL_LOCKOUT 2
+#define USBTMC488_CAPABILITY_488_DOT_2 4
+#define USBTMC488_CAPABILITY_DT1 16
+#define USBTMC488_CAPABILITY_RL1 32
+#define USBTMC488_CAPABILITY_SR1 64
+#define USBTMC488_CAPABILITY_FULL_SCPI 128
+
#endif
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: phy: mxs: add "fsl,imx6ul-usbphy" compatible string
On Wed, Sep 16, 2015 at 03:52:33PM +0800, Li Jun wrote:
> From: Peter Chen
>
> Add "fsl,imx6ul-usbphy" compatible string for iMX6ul usb phy
>
> Signed-off-by: Peter Chen
> Signed-off-by: Li Jun
> ---
> drivers/usb/phy/phy-mxs-usb.c | 5 +
> 1 file changed, 5 insertions(+)
>
> diff --git a/drivers/usb/phy/phy-mxs-usb.c b/drivers/usb/phy/phy-mxs-usb.c
> index 4d863eb..630f643 100644
> --- a/drivers/usb/phy/phy-mxs-usb.c
> +++ b/drivers/usb/phy/phy-mxs-usb.c
> @@ -143,12 +143,17 @@ static const struct mxs_phy_data imx6sx_phy_data = {
> .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> };
>
> +static const struct mxs_phy_data imx6ul_phy_data = {
> + .flags = MXS_PHY_DISCONNECT_LINE_WITHOUT_VBUS,
> +};
> +
> static const struct of_device_id mxs_phy_dt_ids[] = {
> { .compatible = "fsl,imx6sx-usbphy", .data = &imx6sx_phy_data, },
> { .compatible = "fsl,imx6sl-usbphy", .data = &imx6sl_phy_data, },
> { .compatible = "fsl,imx6q-usbphy", .data = &imx6q_phy_data, },
> { .compatible = "fsl,imx23-usbphy", .data = &imx23_phy_data, },
> { .compatible = "fsl,vf610-usbphy", .data = &vf610_phy_data, },
> + { .compatible = "fsl,imx6ul-usbphy", .data = &imx6ul_phy_data, },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, mxs_phy_dt_ids);
> --
> 1.9.1
>
ping...
--
Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument
These ioctls provide support for the USBTMC-USB488 control requests
for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Signed-off-by: Dave Penkler
---
drivers/usb/class/usbtmc.c | 71
include/uapi/linux/usb/tmc.h | 6
2 files changed, 77 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 2358991..d416a5f 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
return rv;
}
+static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
+ unsigned long arg,
+ unsigned int cmd)
+{
+ u8 *buffer;
+ struct device *dev = &data->intf->dev;
+ int rv;
+ unsigned int val;
+ u16 wValue;
+
+ if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
+ return -EINVAL;
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
+ rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
+ if (rv) {
+ rv = -EFAULT;
+ goto exit;
+ }
+ wValue = val ? 1 : 0;
+ } else {
+ wValue = 0;
+ }
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ cmd,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ wValue,
+ data->ifnum,
+ buffer, 0x01, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "simple usb_control_msg failed %d\n", rv);
+ goto exit;
+ } else if (rv != 1) {
+ dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "simple control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+ rv = 0;
+
+ exit:
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1185,6 +1241,21 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC488_IOCTL_READ_STB:
retval = usbtmc488_ioctl_read_stb(data, arg);
break;
+
+ case USBTMC488_IOCTL_REN_CONTROL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_REN_CONTROL);
+ break;
+
+ case USBTMC488_IOCTL_GOTO_LOCAL:
+ retval = usbtmc488_ioctl_simple(data, arg,
+ USBTMC488_REQUEST_GOTO_LOCAL);
+ break;
+
+ case USBTMC488_IOCTL_LOCAL_LOCKOUT:
+ retval = usbtmc488_ioctl_simple(data, arg,
+
USBTMC488_REQUEST_LOCAL_LOCKOUT);
+ break;
}
skip_io_on_zombie:
diff --git a/include/uapi/linux/usb/tmc.h b/include/uapi/linux/usb/tmc.h
index 1dc3af1..0d852c9 100644
--- a/include/uapi/linux/usb/tmc.h
+++ b/include/uapi/linux/usb/tmc.h
@@ -33,6 +33,9 @@
#define USBTMC_REQUEST_GET_CAPABILITIES7
#define USBTMC_REQUEST_INDICATOR_PULSE 64
#define USBTMC488_REQUEST_READ_STATUS_BYTE 128
+#define USBTMC488_REQUEST_REN_CONTROL 160
+#define USBTMC488_REQUEST_GOTO_LOCAL 161
+#define USBTMC488_REQUEST_LOCAL_LOCKOUT162
/* Request values for USBTMC driver's ioctl entry point */
#define USBTMC_IOC_NR 91
@@ -44,6 +47,9 @@
#define USBTMC_IOCTL_CLEAR_IN_HALT _IO(USBTMC_IOC_NR, 7)
#define USBTMC488_IOCTL_GET_CAPS _IO(USBTMC_IOC_NR, 17)
#define USBTMC488_IOCTL_READ_STB _IOR(USBTMC_IOC_NR, 18, unsigned char)
+#define USBTMC488_IOCTL_REN_CONTROL_IOW(USBTMC_IOC_NR, 19, unsigned char)
+#define USBTMC488_IOCTL_GOTO_LOCAL _IO(USBTMC_IOC_NR, 20)
+#define USBTMC488_IOCTL_LOCAL_LOCKOUT _IO(USBTMC_IOC_NR, 21)
/* Driver encoded usb488 capabilities */
#define USBTMC488_CAPABILITY_TRIGGER 1
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 2/5] Add support for USBTMC USB488 SRQ notification with fasync
Background:
By configuring an instrument's event status register various
conditions can be reported via an SRQ notification. This complements
the synchronous polling approach using the READ_STATUS_BYTE ioctl
with an asynchronous notification.
Signed-off-by: Dave Penkler
---
drivers/usb/class/usbtmc.c | 24
1 file changed, 24 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 60a71cb..a95a63d 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -99,6 +99,7 @@ struct usbtmc_device_data {
intiin_interval;
struct urb*iin_urb;
u16iin_wMaxPacketSize;
+ atomic_t srq_asserted;
u8 rigol_quirk;
@@ -113,6 +114,7 @@ struct usbtmc_device_data {
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
wait_queue_head_t waitq;
+ struct fasync_struct *fasync;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -404,6 +406,9 @@ static int usbtmc488_ioctl_read_stb(struct
usbtmc_device_data *data,
atomic_set(&data->iin_data_valid, 0);
+ /* must issue read_stb before using poll or select */
+ atomic_set(&data->srq_asserted, 0);
+
rv = usb_control_msg(data->usb_dev,
usb_rcvctrlpipe(data->usb_dev, 0),
USBTMC488_REQUEST_READ_STATUS_BYTE,
@@ -1174,6 +1179,13 @@ skip_io_on_zombie:
return retval;
}
+static int usbtmc_fasync(int fd, struct file *file, int on)
+{
+ struct usbtmc_device_data *data = file->private_data;
+
+ return fasync_helper(fd, file, on, &data->fasync);
+}
+
static const struct file_operations fops = {
.owner = THIS_MODULE,
.read = usbtmc_read,
@@ -1181,6 +1193,7 @@ static const struct file_operations fops = {
.open = usbtmc_open,
.release= usbtmc_release,
.unlocked_ioctl = usbtmc_ioctl,
+ .fasync = usbtmc_fasync,
.llseek = default_llseek,
};
@@ -1210,6 +1223,16 @@ static void usbtmc_interrupt(struct urb *urb)
wake_up_interruptible(&data->waitq);
goto exit;
}
+ /* check for SRQ notification */
+ if ((data->iin_buffer[0] & 0x7f) == 1) {
+ if (data->fasync)
+ kill_fasync(&data->fasync,
+ SIGIO, POLL_IN);
+
+ atomic_set(&data->srq_asserted, 1);
+ wake_up_interruptible(&data->waitq);
+ goto exit;
+ }
}
dev_warn(&data->intf->dev, "invalid notification: %x\n",
data->iin_buffer[0]);
@@ -1274,6 +1297,7 @@ static int usbtmc_probe(struct usb_interface *intf,
mutex_init(&data->io_mutex);
init_waitqueue_head(&data->waitq);
atomic_set(&data->iin_data_valid, 0);
+ atomic_set(&data->srq_asserted, 0);
data->zombie = 0;
/* Determine if it is a Rigol or not */
--
2.5.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
Background:
When performing a read on an instrument that is executing a function
that runs longer than the USB timeout the instrument may hang and
require a device reset to recover. The READ_STATUS_BYTE operation
always returns even when the instrument is busy permitting to poll
for the appropriate condition. This capability is referred to in
instrument application notes on synchronizing acquisitions for other
platforms.
Signed-off-by: Dave Penkler
---
drivers/usb/class/usbtmc.c | 212 +++
include/uapi/linux/usb/tmc.h | 2 +
2 files changed, 214 insertions(+)
diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
index 7a11a82..60a71cb 100644
--- a/drivers/usb/class/usbtmc.c
+++ b/drivers/usb/class/usbtmc.c
@@ -87,6 +87,19 @@ struct usbtmc_device_data {
u8 bTag_last_write; /* needed for abort */
u8 bTag_last_read; /* needed for abort */
+ /* data for interrupt in endpoint handling */
+ u8 bNotify1;
+ u8 bNotify2;
+ u16ifnum;
+ u8 iin_bTag;
+ u8*iin_buffer;
+ atomic_t iin_data_valid;
+ unsigned int iin_ep;
+ intiin_ep_present;
+ intiin_interval;
+ struct urb*iin_urb;
+ u16iin_wMaxPacketSize;
+
u8 rigol_quirk;
/* attributes from the USB TMC spec for this device */
@@ -99,6 +112,7 @@ struct usbtmc_device_data {
struct usbtmc_dev_capabilities capabilities;
struct kref kref;
struct mutex io_mutex; /* only one i/o function running at a time */
+ wait_queue_head_t waitq;
};
#define to_usbtmc_data(d) container_of(d, struct usbtmc_device_data, kref)
@@ -373,6 +387,86 @@ exit:
return rv;
}
+static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
+ unsigned long arg)
+{
+ u8 *buffer;
+ struct device *dev = &data->intf->dev;
+ int rv;
+ u8 tag, stb;
+
+ dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
+ data->iin_ep_present);
+
+ buffer = kmalloc(8, GFP_KERNEL);
+ if (!buffer)
+ return -ENOMEM;
+
+ atomic_set(&data->iin_data_valid, 0);
+
+ rv = usb_control_msg(data->usb_dev,
+ usb_rcvctrlpipe(data->usb_dev, 0),
+ USBTMC488_REQUEST_READ_STATUS_BYTE,
+ USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
+ data->iin_bTag,
+ data->ifnum,
+ buffer, 0x03, USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_err(dev, "stb usb_control_msg returned %d\n", rv);
+ goto exit;
+ }
+
+ if (buffer[0] != USBTMC_STATUS_SUCCESS) {
+ dev_err(dev, "control status returned %x\n", buffer[0]);
+ rv = -EIO;
+ goto exit;
+ }
+
+ if (data->iin_ep_present) {
+ rv = wait_event_interruptible_timeout(
+ data->waitq,
+ atomic_read(&data->iin_data_valid) != 0,
+ USBTMC_TIMEOUT);
+
+ if (rv < 0) {
+ dev_dbg(dev, "wait interrupted %d\n", rv);
+ goto exit;
+ }
+
+ if (rv == 0) {
+ dev_dbg(dev, "wait timed out\n");
+ rv = -ETIME;
+ goto exit;
+ }
+
+ tag = data->bNotify1 & 0x7f;
+
+ if (tag != data->iin_bTag) {
+ dev_err(dev, "expected bTag %x got %x\n",
+ data->iin_bTag, tag);
+ }
+
+ stb = data->bNotify2;
+ } else {
+ stb = buffer[2];
+ }
+
+ rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
+ if (rv)
+ rv = -EFAULT;
+
+ exit:
+ /* bump interrupt bTag */
+ data->iin_bTag += 1;
+ if (data->iin_bTag > 127)
+ /* 1 is for SRQ see USBTMC-USB488 subclass specification 4.3.1*/
+ data->iin_bTag = 2;
+
+ kfree(buffer);
+ return rv;
+}
+
/*
* Sends a REQUEST_DEV_DEP_MSG_IN message on the Bulk-IN endpoint.
* @transfer_size: number of bytes to request from the device.
@@ -1069,6 +1163,10 @@ static long usbtmc_ioctl(struct file *file, unsigned int
cmd, unsigned long arg)
case USBTMC_IOCTL_ABORT_BULK_IN:
retval = usbtmc_ioctl_abort_bulk_in(data);
break;
+
+ case USBTMC488_IOCTL_READ_STB:
+ retval = usbtmc488_ioctl_read_stb(data, arg);
+ break;
}
skip_io_on_zombie:
@@ -1092,6 +1190,66 @@ static struct usb_class_driver usbtmc_class = {
.minor_base = USBTMC_MINOR_BASE,
};
+static void usbtmc_interrupt(struct urb *urb)
+{
+
[PATCH 0/3] xhci fixes for usb-linus
Hi Greg These xhci fixes for usb-linus resolve a link power management race preventing USB 2 devices from suspending again. The CFC fix is for the newly added Contiguous Frame ID (CFC) capability, fixing an audio noise issue with Isoch transfers on CFC capable hosts. The last fix is a grace period needed by Intel xhci controllers after controller reset. Lu Baolu (1): usb: xhci: fix checking ep busy for CFC Mathias Nyman (1): xhci: Fix a race in usb2 LPM resume, blocking U3 for usb2 devices Rajmohan Mani (1): xhci: Workaround to get Intel xHCI reset working more reliably drivers/usb/host/xhci-hub.c | 15 +-- drivers/usb/host/xhci-ring.c | 32 ++-- drivers/usb/host/xhci.c | 10 ++ 3 files changed, 25 insertions(+), 32 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] xhci: Workaround to get Intel xHCI reset working more reliably
From: Rajmohan Mani Existing Intel xHCI controllers require a delay of 1 mS, after setting the CMD_RESET bit in command register, before accessing any HC registers. This allows the HC to complete the reset operation and be ready for HC register access. Without this delay, the subsequent HC register access, may result in a system hang, very rarely. Verified CherryView / Braswell platforms go through over 5000 warm reboot cycles (which was not possible without this patch), without any xHCI reset hang. Signed-off-by: Rajmohan Mani Tested-by: Joe Lawrence Cc: stable Signed-off-by: Mathias Nyman --- drivers/usb/host/xhci.c | 10 ++ 1 file changed, 10 insertions(+) diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c index 6e7dc6f..dfa44d3 100644 --- a/drivers/usb/host/xhci.c +++ b/drivers/usb/host/xhci.c @@ -175,6 +175,16 @@ int xhci_reset(struct xhci_hcd *xhci) command |= CMD_RESET; writel(command, &xhci->op_regs->command); + /* Existing Intel xHCI controllers require a delay of 1 mS, +* after setting the CMD_RESET bit, and before accessing any +* HC registers. This allows the HC to complete the +* reset operation and be ready for HC register access. +* Without this delay, the subsequent HC register access, +* may result in a system hang very rarely. +*/ + if (xhci->quirks & XHCI_INTEL_HOST) + udelay(1000); + ret = xhci_handshake(&xhci->op_regs->command, CMD_RESET, 0, 10 * 1000 * 1000); if (ret) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] xhci: Fix a race in usb2 LPM resume, blocking U3 for usb2 devices
Clear device initiated resume variables once device is fully up and running
in U0 state.
Resume needs to be signaled for 20ms for usb2 devices before they can be
moved to U0 state.
An interrupt is triggered if a device initiates resume. As we handle the
event in interrupt context we can not sleep for 20ms, so we instead set
a resume flag, a timestamp, and start the roothub polling.
The roothub code will later move the port to U0 when it finds a port in
resume state with the resume flag set, and timestamp passed by 20ms.
A host initiated resume is however not done in interrupt context, and
host initiated resume code will directly signal resume, wait 20ms and then
move the port to U0.
These two codepaths can race, if we are in the middle of a host initated
resume, while sleeping for 20ms, we may handle a port event and find the
port in resume state. The port event handling code will assume the resume
was device initiated and set the resume flag and timestamp.
Root hub code will however not catch the port in resume state again as the
host initated resume code has already moved the port to U0.
The resume flag and timestamp will remain set for this port preventing port
from suspending again (LPM setting port to U3)
Fix this for now by always clearing the device initated resume parameters
once port is in U0
Cc: stable
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-hub.c | 15 +--
1 file changed, 9 insertions(+), 6 deletions(-)
diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 5d2d7e9..0230965 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -782,12 +782,15 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
status |= USB_PORT_STAT_SUSPEND;
}
}
- if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0
- && (raw_port_status & PORT_POWER)
- && (bus_state->suspended_ports & (1 << wIndex))) {
- bus_state->suspended_ports &= ~(1 << wIndex);
- if (hcd->speed < HCD_USB3)
- bus_state->port_c_suspend |= 1 << wIndex;
+ if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0 &&
+ (raw_port_status & PORT_POWER)) {
+ if (bus_state->suspended_ports & (1 << wIndex)) {
+ bus_state->suspended_ports &= ~(1 << wIndex);
+ if (hcd->speed < HCD_USB3)
+ bus_state->port_c_suspend |= 1 << wIndex;
+ }
+ bus_state->resume_done[wIndex] = 0;
+ clear_bit(wIndex, &bus_state->resuming_ports);
}
if (raw_port_status & PORT_CONNECT) {
status |= USB_PORT_STAT_CONNECTION;
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] usb: xhci: fix checking ep busy for CFC
From: Lu Baolu
Function ep_ring_is_processing() checks the dequeue pointer
in endpoint context to know whether an endpoint is busy with
processing TRBs. This is not correct since dequeue pointer
field in an endpoint context is only valid when the endpoint
is in Halted or Stopped states. This buggy code causes audio
noise when playing sound with USB headset connected to host
controllers which support CFC (one of xhci 1.1 features).
This patch should exist in stable kernel since v4.3.
Reported-and-tested-by: YD Tseng
Signed-off-by: Lu Baolu
Cc: stable # v4.3
Signed-off-by: Mathias Nyman
---
drivers/usb/host/xhci-ring.c | 32 ++--
1 file changed, 6 insertions(+), 26 deletions(-)
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index fa83625..6c5e813 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -3896,28 +3896,6 @@ cleanup:
return ret;
}
-static int ep_ring_is_processing(struct xhci_hcd *xhci,
- int slot_id, unsigned int ep_index)
-{
- struct xhci_virt_device *xdev;
- struct xhci_ring *ep_ring;
- struct xhci_ep_ctx *ep_ctx;
- struct xhci_virt_ep *xep;
- dma_addr_t hw_deq;
-
- xdev = xhci->devs[slot_id];
- xep = &xhci->devs[slot_id]->eps[ep_index];
- ep_ring = xep->ring;
- ep_ctx = xhci_get_ep_ctx(xhci, xdev->out_ctx, ep_index);
-
- if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) != EP_STATE_RUNNING)
- return 0;
-
- hw_deq = le64_to_cpu(ep_ctx->deq) & ~EP_CTX_CYCLE_MASK;
- return (hw_deq !=
- xhci_trb_virt_to_dma(ep_ring->enq_seg, ep_ring->enqueue));
-}
-
/*
* Check transfer ring to guarantee there is enough room for the urb.
* Update ISO URB start_frame and interval.
@@ -3983,10 +3961,12 @@ int xhci_queue_isoc_tx_prepare(struct xhci_hcd *xhci,
gfp_t mem_flags,
}
/* Calculate the start frame and put it in urb->start_frame. */
- if (HCC_CFC(xhci->hcc_params) &&
- ep_ring_is_processing(xhci, slot_id, ep_index)) {
- urb->start_frame = xep->next_frame_id;
- goto skip_start_over;
+ if (HCC_CFC(xhci->hcc_params) && !list_empty(&ep_ring->td_list)) {
+ if ((le32_to_cpu(ep_ctx->ep_info) & EP_STATE_MASK) ==
+ EP_STATE_RUNNING) {
+ urb->start_frame = xep->next_frame_id;
+ goto skip_start_over;
+ }
}
start_frame = readl(&xhci->run_regs->microframe_index);
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang wrote:
> On 17 November 2015 at 21:34, Andy Shevchenko
> wrote:
>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang wrote:
>>> It dose not work when we want to use the usb-to-serial port based
>>> on one usb gadget as a console. Thus this patch adds the console
>>> initialization to support this request.
>>
>>> +#define GS_BUFFER_SIZE (4096)
>> Redundant parens
> OK. I'll remove it.
>
>>> +#define GS_CONSOLE_BUF_SIZE(2 * GS_BUFFER_SIZE)
>>> +
>>> +struct gscons_info {
>>> + struct gs_port *port;
>>> + struct tty_driver *tty_driver;
>>> + struct work_struct work;
>>> + int buf_tail;
>>> + charbuf[GS_CONSOLE_BUF_SIZE];
>>
>> Can't be malloced once?
> The 'gscons_info' structure is malloced once.
In state of high fragmentation is quite hard to find big memory chunks.
I would split it to two allocations, though if maintainers are okay
with your code, then I'm also okay.
>>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int
>>> buffer_size)
>>> +{
>>> + struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>>> +
>>> + if (!req)
>>
>> For sake of readability it's better to have assignment explicitly before
>> 'if'.
>
> But I think it is very easy to understand the assignment here with
> saving code lines.
It's not a function of couple of lines, so, for me makes sense to
explicitly put the assignment here. Especially that one that does
allocations (for pointer arithmetic I could agree to place the
assignment in the definition block).
>>> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
>>> +{
>>> + if (req->status != 0 && req->status != -ECONNRESET)
>>> + return;
>>
>> Something missed here. Currently it's no-op.
>>
>
> Yeah. I didn't realize what need to do in the callback here, so just
> leave a callback without anything. But maybe something will be added
> if there are some requirements in future.
if ()
..
will be optimized away, why not to remove it?
>>> + port = ports[port_num].port;
>>> + if (!port) {
>>> + pr_err("%s: serial line [%d] not allocated.\n",
>>> + __func__, port_num);
>>> + return -ENODEV;
>>> + }
>>> +
>>> + if (!port->port_usb) {
>>> + pr_err("%s: no port usb.\n", __func__);
>>
>> Starting from here could it be dev_err and so on?
>
> There are no dev_err things and device things in this file, so pr_xxx
> is more reasonable.
This is understandable, but if in case you have device in place why
not to use its name?
>>> + pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
>>
>> Dynamic debug will add function name if asked.
>
> Sorry, I didn't get your point, you mean print the function name is
> redundant here?
Right.
Just pr_debug("port[%d] …", …);
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 5/5] Add ioctls to enable and disable local controls on an instrument
On Wed, Nov 18, 2015 at 10:38 AM, Dave Penkler wrote:
> These ioctls provide support for the USBTMC-USB488 control requests
> for REN_CONTROL, GO_TO_LOCAL and LOCAL_LOCKOUT
Couple of comments below.
> diff --git a/drivers/usb/class/usbtmc.c b/drivers/usb/class/usbtmc.c
> index 2358991..d416a5f 100644
> --- a/drivers/usb/class/usbtmc.c
> +++ b/drivers/usb/class/usbtmc.c
> @@ -476,6 +476,62 @@ static int usbtmc488_ioctl_read_stb(struct
> usbtmc_device_data *data,
> return rv;
> }
>
> +static int usbtmc488_ioctl_simple(struct usbtmc_device_data *data,
> + unsigned long arg,
> + unsigned int cmd)
> +{
> + u8 *buffer;
> + struct device *dev = &data->intf->dev;
> + int rv;
> + unsigned int val;
> + u16 wValue;
> +
> + if (!(data->usb488_caps & USBTMC488_CAPABILITY_SIMPLE))
> + return -EINVAL;
> +
> + buffer = kmalloc(8, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + if (cmd == USBTMC488_REQUEST_REN_CONTROL) {
> + rv = copy_from_user(&val, (void __user *)arg, sizeof(val));
> + if (rv) {
> + rv = -EFAULT;
> + goto exit;
> + }
> + wValue = val ? 1 : 0;
> + } else {
> + wValue = 0;
> + }
> +
> + rv = usb_control_msg(data->usb_dev,
> + usb_rcvctrlpipe(data->usb_dev, 0),
> + cmd,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + wValue,
> + data->ifnum,
> + buffer, 0x01, USBTMC_TIMEOUT);
> +
Redundant empty line
> + if (rv < 0) {
> + dev_err(dev, "simple usb_control_msg failed %d\n", rv);
> + goto exit;
> + } else if (rv != 1) {
> + dev_warn(dev, "simple usb_control_msg returned %d\n", rv);
Actually here what king of results could be? 0? 2+? In all cases of
error you have to provide an error code.
> + goto exit;
> + }
> +
> + if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> + dev_err(dev, "simple control status returned %x\n",
> buffer[0]);
> + rv = -EIO;
> + goto exit;
> + }
> + rv = 0;
> +
> + exit:
> + kfree(buffer);
> + return rv;
> +}
> +
--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 1/1] usb: misc: usbtest: improve the description for error message
Now the function of complicated_callback is not only used for iso
transfer, improve the error message to reflect it.
Signed-off-by: Peter Chen
---
Changes for v2:
- Typo for "fo" -> "for" at subject
drivers/usb/misc/usbtest.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 637f3f7..c1bd1eb 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -1849,7 +1849,7 @@ static void complicated_callback(struct urb *urb)
goto done;
default:
dev_err(&ctx->dev->intf->dev,
- "iso resubmit err %d\n",
+ "resubmit err %d\n",
status);
/* FALLTHROUGH */
case -ENODEV: /* disconnected */
@@ -1863,7 +1863,7 @@ static void complicated_callback(struct urb *urb)
if (ctx->pending == 0) {
if (ctx->errors)
dev_err(&ctx->dev->intf->dev,
- "iso test, %lu errors out of %lu\n",
+ "during the test, %lu errors out of %lu\n",
ctx->errors, ctx->packet_count);
complete(&ctx->done);
}
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: USB_OTG does not depend on PM
On Wed, Nov 18, 2015 at 10:38:18AM +0100, Arnd Bergmann wrote: > On Wednesday 18 November 2015 11:17:50 Peter Chen wrote: > > From 3a6918dae038aadc200dcf0263f4440acc2353d4 Mon Sep 17 00:00:00 2001 > > From: Peter Chen > > Date: Wed, 18 Nov 2015 11:06:34 +0800 > > Subject: [PATCH 1/1] usb: kconfig: fix warning of select USB_OTG > > > > When choose randconfig for kernel build, it reports below warning: > > "warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG > > which has unmet direct dependencies (USB_SUPPORT && USB && PM)" > > > > In fact, USB_OTG is visual symbol and depends on PM, so the driver > > visible ? Yes, will change this typo > > > needs to depend on it to reduce dependency problem. > > > > Signed-off-by: Peter Chen > > Reported-by: Arnd Bergmann > > Cc: Felipe Balbi > > Acked-by: Arnd Bergmann > > I was a bit worried that this might break defconfig files that now > no longer automatically get OSB_OTG enabled, but I have checked all > defconfig files we have in the kernel and none of them uses > USB_OTG_FSM, FSL_USB2_OTG or USB_MV_OTG, so we are fine. Thanks. > > Thanks! > > Arnd -- Best Regards, Peter Chen -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] USB: USB_OTG does not depend on PM
On Wednesday 18 November 2015 11:17:50 Peter Chen wrote: > From 3a6918dae038aadc200dcf0263f4440acc2353d4 Mon Sep 17 00:00:00 2001 > From: Peter Chen > Date: Wed, 18 Nov 2015 11:06:34 +0800 > Subject: [PATCH 1/1] usb: kconfig: fix warning of select USB_OTG > > When choose randconfig for kernel build, it reports below warning: > "warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG > which has unmet direct dependencies (USB_SUPPORT && USB && PM)" > > In fact, USB_OTG is visual symbol and depends on PM, so the driver visible ? > needs to depend on it to reduce dependency problem. > > Signed-off-by: Peter Chen > Reported-by: Arnd Bergmann > Cc: Felipe Balbi Acked-by: Arnd Bergmann I was a bit worried that this might break defconfig files that now no longer automatically get OSB_OTG enabled, but I have checked all defconfig files we have in the kernel and none of them uses USB_OTG_FSM, FSL_USB2_OTG or USB_MV_OTG, so we are fine. Thanks! Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: > On 16-11-15 18:01, Philipp Zabel wrote: > > If there are two devices sharing the same reset line that is initially > > held asserted, do the two drivers somehow have to synchronize before > > releasing the reset together? > > Not to my knowledge, I suggest that we simply treat this same as > regulators / clocks where the first one to enable it actually gets > it enabled (de-asserted in case of a reset), and the last one > to disable (assert) it (so dropping the usage counter back to 0) leads > to it being asserted again. > > This seems to work well enough for clocks / regulators and phys, and > at least for my use-case it should work fine for the shared reset too > (I will test to make sure of course). > > So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. I fear the deassertion count maps well to the case where resets are used just like clocks (when inactive modules are kept in reset), but I'm not sure this is useful in the case of resets that are kept deasserted most of the time, only to be asserted for a short pulse. Maybe we have to differentiate between the two cases? regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 1/5] Implement an ioctl to support the USMTMC-USB488 READ_STATUS_BYTE operation.
On Wed, Nov 18, 2015 at 10:37 AM, Dave Penkler wrote:
> Background:
> When performing a read on an instrument that is executing a function
> that runs longer than the USB timeout the instrument may hang and
> require a device reset to recover. The READ_STATUS_BYTE operation
> always returns even when the instrument is busy permitting to poll
> for the appropriate condition. This capability is referred to in
> instrument application notes on synchronizing acquisitions for other
> platforms.
>
First of all, please be patient and do not send patches immediately
when you answered to someone's review. It increases redundant noise
and burden on reviewer. Wait at least for 24h especially if there are
topics still to discuss.
[]
> +static int usbtmc488_ioctl_read_stb(struct usbtmc_device_data *data,
> + unsigned long arg)
> +{
> + u8 *buffer;
> + struct device *dev = &data->intf->dev;
> + int rv;
> + u8 tag, stb;
If you rearrange this like
+ struct device *dev = &data->intf->dev;
+ u8 *buffer;
+ u8 tag, stb;
+ int rv;
it will look better. Similar to check in all your patches.
> +
> + dev_dbg(dev, "Enter ioctl_read_stb iin_ep_present: %d\n",
> + data->iin_ep_present);
> +
> + buffer = kmalloc(8, GFP_KERNEL);
> + if (!buffer)
> + return -ENOMEM;
> +
> + atomic_set(&data->iin_data_valid, 0);
> +
> + rv = usb_control_msg(data->usb_dev,
> + usb_rcvctrlpipe(data->usb_dev, 0),
> + USBTMC488_REQUEST_READ_STATUS_BYTE,
> + USB_DIR_IN | USB_TYPE_CLASS | USB_RECIP_INTERFACE,
> + data->iin_bTag,
> + data->ifnum,
> + buffer, 0x03, USBTMC_TIMEOUT);
> +
Redundant empty line.
> + if (rv < 0) {
> + dev_err(dev, "stb usb_control_msg returned %d\n", rv);
> + goto exit;
> + }
> +
> + if (buffer[0] != USBTMC_STATUS_SUCCESS) {
> + dev_err(dev, "control status returned %x\n", buffer[0]);
> + rv = -EIO;
> + goto exit;
> + }
> +
> + if (data->iin_ep_present) {
> + rv = wait_event_interruptible_timeout(
> + data->waitq,
> + atomic_read(&data->iin_data_valid) != 0,
> + USBTMC_TIMEOUT);
> +
Ditto.
> + if (rv < 0) {
> + dev_dbg(dev, "wait interrupted %d\n", rv);
> + goto exit;
> + }
> +
> + if (rv == 0) {
> + dev_dbg(dev, "wait timed out\n");
> + rv = -ETIME;
> + goto exit;
> + }
> +
> + tag = data->bNotify1 & 0x7f;
> +
You may remove empty line, though it's minor nitpick here.
> + if (tag != data->iin_bTag) {
> + dev_err(dev, "expected bTag %x got %x\n",
> + data->iin_bTag, tag);
> + }
> +
> + stb = data->bNotify2;
> + } else {
> + stb = buffer[2];
> + }
> +
> + rv = copy_to_user((void __user *)arg, &stb, sizeof(stb));
> + if (rv)
> + rv = -EFAULT;
> +
> + exit:
> + /* bump interrupt bTag */
> + data->iin_bTag += 1;
> + if (data->iin_bTag > 127)
> + /* 1 is for SRQ see USBTMC-USB488 subclass specification
> 4.3.1*/
Missed space before asterisk.
> + data->iin_bTag = 2;
> +
> + kfree(buffer);
> + return rv;
> +}
[]
> +static void usbtmc_interrupt(struct urb *urb)
> +{
> + struct usbtmc_device_data *data = urb->context;
> + int status = urb->status;
> + int rv;
> +
> + dev_dbg(&data->intf->dev, "int status: %d len %d\n",
> + status, urb->actual_length);
> +
> + switch (status) {
> + case 0: /* SUCCESS */
> + if (data->iin_buffer[0] & 0x80) {
> + /* check for valid STB notification */
> + if ((data->iin_buffer[0] & 0x7f) > 1) {
Despite your answer to my comment code is quite understandable even with & 0x7e.
You already put comment line about this, you may add that you validate
the value to be 127 >= value >= 2.
> + data->bNotify1 = data->iin_buffer[0];
> + data->bNotify2 = data->iin_buffer[1];
> + atomic_set(&data->iin_data_valid, 1);
> + wake_up_interruptible(&data->waitq);
> + goto exit;
> + }
> + }
> + dev_warn(&data->intf->dev, "invalid notification: %x\n",
> + data->iin_buffer[0]);
> + break;
> + case -EOVERFLOW:
> + dev_err(&data->intf->dev,
> +
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
On Wed, Nov 18, 2015 at 10:46:52AM +0100, Philipp Zabel wrote: > Hi Hans, > > Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: > > On 16-11-15 18:01, Philipp Zabel wrote: > > > If there are two devices sharing the same reset line that is initially > > > held asserted, do the two drivers somehow have to synchronize before > > > releasing the reset together? > > > > Not to my knowledge, I suggest that we simply treat this same as > > regulators / clocks where the first one to enable it actually gets > > it enabled (de-asserted in case of a reset), and the last one > > to disable (assert) it (so dropping the usage counter back to 0) leads > > to it being asserted again. > > > > This seems to work well enough for clocks / regulators and phys, and > > at least for my use-case it should work fine for the shared reset too > > (I will test to make sure of course). > > > > So from my pov a simple counter should suffice, does that work for you ? > > I don't quite understand what counting will help with, then. The first > driver to call reset_control_deassert will deassert the reset, whether > you count or not. > But if the two drivers have deasserted an initially asserted reset, a > reset_control_assert for one of them will silently fail. Then maybe we can just make it return an error when someone calls _assert or _reset on a reset line that has more than one user? Maxime -- Maxime Ripard, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com signature.asc Description: Digital signature
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi All, On Wed, Nov 18, 2015 at 9:18 PM, Maxime Ripard wrote: > On Wed, Nov 18, 2015 at 10:46:52AM +0100, Philipp Zabel wrote: >> Hi Hans, >> >> Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: >> > On 16-11-15 18:01, Philipp Zabel wrote: >> > > If there are two devices sharing the same reset line that is initially >> > > held asserted, do the two drivers somehow have to synchronize before >> > > releasing the reset together? >> > >> > Not to my knowledge, I suggest that we simply treat this same as >> > regulators / clocks where the first one to enable it actually gets >> > it enabled (de-asserted in case of a reset), and the last one >> > to disable (assert) it (so dropping the usage counter back to 0) leads >> > to it being asserted again. >> > >> > This seems to work well enough for clocks / regulators and phys, and >> > at least for my use-case it should work fine for the shared reset too >> > (I will test to make sure of course). >> > >> > So from my pov a simple counter should suffice, does that work for you ? >> >> I don't quite understand what counting will help with, then. The first >> driver to call reset_control_deassert will deassert the reset, whether >> you count or not. >> But if the two drivers have deasserted an initially asserted reset, a >> reset_control_assert for one of them will silently fail. > > Then maybe we can just make it return an error when someone calls > _assert or _reset on a reset line that has more than one user? Just to set another cat amongst the pigeons: What if a driver needs to toggle the shared reset line of some IP block to get it out of some broken state? Thanks, -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On 18-11-15 10:46, Philipp Zabel wrote: Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? Not to my knowledge, I suggest that we simply treat this same as regulators / clocks where the first one to enable it actually gets it enabled (de-asserted in case of a reset), and the last one to disable (assert) it (so dropping the usage counter back to 0) leads to it being asserted again. This seems to work well enough for clocks / regulators and phys, and at least for my use-case it should work fine for the shared reset too (I will test to make sure of course). So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. Correct, which is what we want, although I would not call it silently fail I would call it a nop as it is intended. I fear the deassertion count maps well to the case where resets are used just like clocks (when inactive modules are kept in reset), but I'm not sure this is useful in the case of resets that are kept deasserted most of the time, only to be asserted for a short pulse. Maybe we have to differentiate between the two cases? Ack. I think that the "just like clocks" case is the more common one, and it seems to me that the short-pulse case should use reset_control_reset. Maybe we need to provide a default implementation of reset_control_reset which does the pulse when no implementation is provided by the driver ? Although that brings the question with it what to do with the deassert_count in that case, as some drivers may also use that for the initial deassert... I guess we could document to not do that if you want to assure that no other drivers muck with the reset-line ... Hmm, this is getting messy pretty quickly. New proposal: 1) Add a concept of shared resets, adding: reset_control_get_shared and devm_reset_control_get_shared functions, which set a shared bool in struct reset_control 2) Add int refcnt to struct reset_controller_dev, which gets incremented/decremented on reset_control_get/reset_control_put do a BUG_ON on refcnt == 1 in the get functions when the non-shared variant gets called (this is optional but probably a good extra check) 3) Do the whole deassert_count thingy only when the shared bool is true 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Am Mittwoch, den 18.11.2015, 21:20 +1100 schrieb Julian Calaby: > Hi All, > > On Wed, Nov 18, 2015 at 9:18 PM, Maxime Ripard > wrote: > > On Wed, Nov 18, 2015 at 10:46:52AM +0100, Philipp Zabel wrote: > >> Hi Hans, > >> > >> Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: > >> > On 16-11-15 18:01, Philipp Zabel wrote: > >> > > If there are two devices sharing the same reset line that is initially > >> > > held asserted, do the two drivers somehow have to synchronize before > >> > > releasing the reset together? > >> > > >> > Not to my knowledge, I suggest that we simply treat this same as > >> > regulators / clocks where the first one to enable it actually gets > >> > it enabled (de-asserted in case of a reset), and the last one > >> > to disable (assert) it (so dropping the usage counter back to 0) leads > >> > to it being asserted again. > >> > > >> > This seems to work well enough for clocks / regulators and phys, and > >> > at least for my use-case it should work fine for the shared reset too > >> > (I will test to make sure of course). > >> > > >> > So from my pov a simple counter should suffice, does that work for you ? > >> > >> I don't quite understand what counting will help with, then. The first > >> driver to call reset_control_deassert will deassert the reset, whether > >> you count or not. > >> But if the two drivers have deasserted an initially asserted reset, a > >> reset_control_assert for one of them will silently fail. > > > > Then maybe we can just make it return an error when someone calls > > _assert or _reset on a reset line that has more than one user? > > Just to set another cat amongst the pigeons: What if a driver needs to > toggle the shared reset line of some IP block to get it out of some > broken state? We'd need driver support for that. Possibly using notifications? In the hypothetical case of two GPUs sharing the same reset line, one currently currently busy processing a command stream, and the other one stuck, the driver for the stuck one would request a reset, the framework would have to give notice to the other driver, allow it to veto or block the reset until it has parked the hardware and stored state. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On 18-11-15 11:38, Philipp Zabel wrote: Am Mittwoch, den 18.11.2015, 21:20 +1100 schrieb Julian Calaby: Hi All, On Wed, Nov 18, 2015 at 9:18 PM, Maxime Ripard wrote: On Wed, Nov 18, 2015 at 10:46:52AM +0100, Philipp Zabel wrote: Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? Not to my knowledge, I suggest that we simply treat this same as regulators / clocks where the first one to enable it actually gets it enabled (de-asserted in case of a reset), and the last one to disable (assert) it (so dropping the usage counter back to 0) leads to it being asserted again. This seems to work well enough for clocks / regulators and phys, and at least for my use-case it should work fine for the shared reset too (I will test to make sure of course). So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. Then maybe we can just make it return an error when someone calls _assert or _reset on a reset line that has more than one user? Just to set another cat amongst the pigeons: What if a driver needs to toggle the shared reset line of some IP block to get it out of some broken state? We'd need driver support for that. Possibly using notifications? In the hypothetical case of two GPUs sharing the same reset line, one currently currently busy processing a command stream, and the other one stuck, the driver for the stuck one would request a reset, the framework would have to give notice to the other driver, allow it to veto or block the reset until it has parked the hardware and stored state. Right, see the proposal I've just sent which introduces a (light-weight) concept of shared reset lines, and forbids calling reset_control_reset on reset lines marked shared. This does not solve this case, but it makes it clear this will not work. Then we can cross this theoretical bridge if it ever becomes non theoretical. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 18 November 2015 at 17:32, Andy Shevchenko wrote:
> On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang wrote:
>> On 17 November 2015 at 21:34, Andy Shevchenko
>> wrote:
>>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang wrote:
It dose not work when we want to use the usb-to-serial port based
on one usb gadget as a console. Thus this patch adds the console
initialization to support this request.
>>>
>
+#define GS_BUFFER_SIZE (4096)
>>> Redundant parens
>> OK. I'll remove it.
>>
+#define GS_CONSOLE_BUF_SIZE(2 * GS_BUFFER_SIZE)
+
+struct gscons_info {
+ struct gs_port *port;
+ struct tty_driver *tty_driver;
+ struct work_struct work;
+ int buf_tail;
+ charbuf[GS_CONSOLE_BUF_SIZE];
>>>
>>> Can't be malloced once?
>> The 'gscons_info' structure is malloced once.
>
> In state of high fragmentation is quite hard to find big memory chunks.
> I would split it to two allocations, though if maintainers are okay
> with your code, then I'm also okay.
>
Make sense. But I think the major memory of the 'struct gscons_info'
is for the 'buf' member, so I still think no need to allocate it 2
times.
+static struct usb_request *gs_request_new(struct usb_ep *ep, int
buffer_size)
+{
+ struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
+
+ if (!req)
>>>
>>> For sake of readability it's better to have assignment explicitly before
>>> 'if'.
>>
>> But I think it is very easy to understand the assignment here with
>> saving code lines.
>
> It's not a function of couple of lines, so, for me makes sense to
> explicitly put the assignment here. Especially that one that does
> allocations (for pointer arithmetic I could agree to place the
> assignment in the definition block).
>
OK. Sounds reasonable.
+static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
+{
+ if (req->status != 0 && req->status != -ECONNRESET)
+ return;
>>>
>>> Something missed here. Currently it's no-op.
>>>
>>
>> Yeah. I didn't realize what need to do in the callback here, so just
>> leave a callback without anything. But maybe something will be added
>> if there are some requirements in future.
>
> if ()
> ..
>
> will be optimized away, why not to remove it?
OK. I'll remove it.
>
+ port = ports[port_num].port;
+ if (!port) {
+ pr_err("%s: serial line [%d] not allocated.\n",
+ __func__, port_num);
+ return -ENODEV;
+ }
+
+ if (!port->port_usb) {
+ pr_err("%s: no port usb.\n", __func__);
>>>
>>> Starting from here could it be dev_err and so on?
>>
>> There are no dev_err things and device things in this file, so pr_xxx
>> is more reasonable.
>
> This is understandable, but if in case you have device in place why
> not to use its name?
Yes, that's right.
>
+ pr_debug("%s: port[%d] console connect!\n", __func__, port_num);
>>>
>>> Dynamic debug will add function name if asked.
>>
>> Sorry, I didn't get your point, you mean print the function name is
>> redundant here?
>
> Right.
>
> Just pr_debug("port[%d] …", …);
>
OK. Very thanks for your suggestions.
> --
> With Best Regards,
> Andy Shevchenko
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 04/12] usb: xhci: dbc: add support for Intel xHCI dbc quirk
Hi, On 11/17/2015 06:28 PM, Dmitry Malkin wrote: On Mon, 16 Nov 2015 10:18:42 +0800, Lu Baolu wrote: This quirk works as well if debug host doesn't have DBC. I didn't try a DBC-capable debug host yet. Hi, I went through it again, with your v3 patch series on top of vanilla v4.3.0. Two targets, one host, all with Intel chipset (XHCI device 0:14.0 with VID:8086). Targets DIDs are 9c31 (8-series chipset) and 9d2f (100-series chipset). Host DID is 8c31 (8-series). I can only further confirm my previous observations. The host cannot see the target (there is no debug device) at all, until I manually re-plug a debug cable. Cable plugged in, prior to starting the target. Thank you for your time. Are you using a "USB3 debugging cable"? The internal wiring of the debug cable is crossed over. I think that trying a Hot Reset for all disconnected USB 3.0 ports on the debug target *just before* setting DCE bit is inherently racy. What if the number of disconnected ports changes or if someone will insert a print statement or refactors your code? That might be problematic. I will put a comment there. Also sending TS2 to the debug host port will either: - get dropped by its hub as unsupported upstream request, or - get ignored due to SS.Inactive port state Could you explain what exactly you workaround does at the low level? What I though was that reset USB 3.0 ports on debug target before setting DCE bit will cause debug host to warm reset the port for enumeration purpose and then setting the DCE bit will take effect. This just works on my development machine, not only connecting debug target to root port of host, but also under external hubs. I realized that this quirk isn't a universal solution and it might not work with some silicons. But I thought it shouldn't do any harmless. In bad case, users could plug out and plug in the debug cable, or reset the port through the sysfs node for workaround. If this is acceptable, I will add this in the user guide and increase the timeout value in code. Thanks, -Baolu -- with best regards, Dmitry Malkin -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On 18-11-15 11:38, Hans de Goede wrote: Hi, On 18-11-15 10:46, Philipp Zabel wrote: Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? Not to my knowledge, I suggest that we simply treat this same as regulators / clocks where the first one to enable it actually gets it enabled (de-asserted in case of a reset), and the last one to disable (assert) it (so dropping the usage counter back to 0) leads to it being asserted again. This seems to work well enough for clocks / regulators and phys, and at least for my use-case it should work fine for the shared reset too (I will test to make sure of course). So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. Correct, which is what we want, although I would not call it silently fail I would call it a nop as it is intended. I fear the deassertion count maps well to the case where resets are used just like clocks (when inactive modules are kept in reset), but I'm not sure this is useful in the case of resets that are kept deasserted most of the time, only to be asserted for a short pulse. Maybe we have to differentiate between the two cases? Ack. I think that the "just like clocks" case is the more common one, and it seems to me that the short-pulse case should use reset_control_reset. Maybe we need to provide a default implementation of reset_control_reset which does the pulse when no implementation is provided by the driver ? Although that brings the question with it what to do with the deassert_count in that case, as some drivers may also use that for the initial deassert... I guess we could document to not do that if you want to assure that no other drivers muck with the reset-line ... Hmm, this is getting messy pretty quickly. New proposal: 1) Add a concept of shared resets, adding: reset_control_get_shared and devm_reset_control_get_shared functions, which set a shared bool in struct reset_control 2) Add int refcnt to struct reset_controller_dev, which gets incremented/decremented on reset_control_get/reset_control_put do a BUG_ON on refcnt == 1 in the get functions when the non-shared variant gets called (this is optional but probably a good extra check) 3) Do the whole deassert_count thingy only when the shared bool is true 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true p.s. In case it was not clear, the above is a RFC, I will happily implement this, but if people have comments on the general concept I would like to get' those comments before writing a patch for this :) Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi Hans, Am Mittwoch, den 18.11.2015, 11:38 +0100 schrieb Hans de Goede: > Hi, > > On 18-11-15 10:46, Philipp Zabel wrote: > > Hi Hans, > > > > Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: > >> On 16-11-15 18:01, Philipp Zabel wrote: > >>> If there are two devices sharing the same reset line that is initially > >>> held asserted, do the two drivers somehow have to synchronize before > >>> releasing the reset together? > >> > >> Not to my knowledge, I suggest that we simply treat this same as > >> regulators / clocks where the first one to enable it actually gets > >> it enabled (de-asserted in case of a reset), and the last one > >> to disable (assert) it (so dropping the usage counter back to 0) leads > >> to it being asserted again. > >> > >> This seems to work well enough for clocks / regulators and phys, and > >> at least for my use-case it should work fine for the shared reset too > >> (I will test to make sure of course). > >> > >> So from my pov a simple counter should suffice, does that work for you ? > > > > I don't quite understand what counting will help with, then. The first > > driver to call reset_control_deassert will deassert the reset, whether > > you count or not. > > But if the two drivers have deasserted an initially asserted reset, a > > reset_control_assert for one of them will silently fail. > > Correct, which is what we want, although I would not call it silently > fail I would call it a nop as it is intended. > > > I fear the deassertion count maps well to the case where resets are used > > just like clocks (when inactive modules are kept in reset), but I'm not > > sure this is useful in the case of resets that are kept deasserted most > > of the time, only to be asserted for a short pulse. Maybe we have to > > differentiate between the two cases? > > Ack. I think that the "just like clocks" case is the more common one, We have to accommodate both. > and it seems to me that the short-pulse case should use reset_control_reset. reset_control_reset can only be used if the reset controller knows the necessary reset pulse duration itself. > Maybe we need to provide a default implementation of reset_control_reset which > does the pulse when no implementation is provided by the driver ? I have thought about adding a version that takes a delay parameter, but then you'd end up with udelay and msleep variants. > Although that brings the question with it what to do with the deassert_count > in > that case, as some drivers may also use that for the initial deassert... I > guess > we could document to not do that if you want to assure that no other drivers > muck with the reset-line ... > > Hmm, this is getting messy pretty quickly. New proposal: > > 1) Add a concept of shared resets, adding: reset_control_get_shared and > devm_reset_control_get_shared functions, which set a shared bool > in struct reset_control Yes, that is a good idea. And disallow reset_control_get (non-shared) on an already in-use control. > 2) Add int refcnt to struct reset_controller_dev, which gets > incremented/decremented on reset_control_get/reset_control_put > do a BUG_ON on refcnt == 1 in the get functions when the non-shared > variant gets called (this is optional but probably a good extra > check) Maybe add a new structure between reset_controller_dev and reset_control, or keep the reset_control structures themselves on a list and hand them out again for reset_control_get_shared and do the reference counting there? Otherwise the reset_controller_dev would have to allocate a (possibly huge, sparse) array of reference counters. > 3) Do the whole deassert_count thingy only when the shared bool is true > > 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true Sounds good, though I'd prefer WARN_ON over BUG_ON. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On 18-11-15 12:59, Philipp Zabel wrote: Hi Hans, Am Mittwoch, den 18.11.2015, 11:38 +0100 schrieb Hans de Goede: Hi, On 18-11-15 10:46, Philipp Zabel wrote: Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? Not to my knowledge, I suggest that we simply treat this same as regulators / clocks where the first one to enable it actually gets it enabled (de-asserted in case of a reset), and the last one to disable (assert) it (so dropping the usage counter back to 0) leads to it being asserted again. This seems to work well enough for clocks / regulators and phys, and at least for my use-case it should work fine for the shared reset too (I will test to make sure of course). So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. Correct, which is what we want, although I would not call it silently fail I would call it a nop as it is intended. I fear the deassertion count maps well to the case where resets are used just like clocks (when inactive modules are kept in reset), but I'm not sure this is useful in the case of resets that are kept deasserted most of the time, only to be asserted for a short pulse. Maybe we have to differentiate between the two cases? Ack. I think that the "just like clocks" case is the more common one, We have to accommodate both. and it seems to me that the short-pulse case should use reset_control_reset. reset_control_reset can only be used if the reset controller knows the necessary reset pulse duration itself. Maybe we need to provide a default implementation of reset_control_reset which does the pulse when no implementation is provided by the driver ? I have thought about adding a version that takes a delay parameter, but then you'd end up with udelay and msleep variants. Although that brings the question with it what to do with the deassert_count in that case, as some drivers may also use that for the initial deassert... I guess we could document to not do that if you want to assure that no other drivers muck with the reset-line ... Hmm, this is getting messy pretty quickly. New proposal: 1) Add a concept of shared resets, adding: reset_control_get_shared and devm_reset_control_get_shared functions, which set a shared bool in struct reset_control Yes, that is a good idea. And disallow reset_control_get (non-shared) on an already in-use control. 2) Add int refcnt to struct reset_controller_dev, which gets incremented/decremented on reset_control_get/reset_control_put do a BUG_ON on refcnt == 1 in the get functions when the non-shared variant gets called (this is optional but probably a good extra check) Maybe add a new structure between reset_controller_dev and reset_control, or keep the reset_control structures themselves on a list and hand them out again for reset_control_get_shared and do the reference counting there? Otherwise the reset_controller_dev would have to allocate a (possibly huge, sparse) array of reference counters. I see the problem here. I will see what I can come up with. Not sure when exactly I will have time to work on this, may be a bit before you see an actual patch for this from me. 3) Do the whole deassert_count thingy only when the shared bool is true 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true Sounds good, though I'd prefer WARN_ON over BUG_ON. Ack, I always mix the 2 up I meant WARN_ON. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH] usb: gadget: Add the console support for usb-to-serial port
From: Baolin Wang
> Sent: 18 November 2015 10:45
> On 18 November 2015 at 17:32, Andy Shevchenko
> wrote:
> > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang wrote:
> >> On 17 November 2015 at 21:34, Andy Shevchenko
> >> wrote:
> >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang
> >>> wrote:
> It dose not work when we want to use the usb-to-serial port based
> on one usb gadget as a console. Thus this patch adds the console
> initialization to support this request.
> >>>
> >
> +#define GS_BUFFER_SIZE (4096)
> >>> Redundant parens
> >> OK. I'll remove it.
> >>
> +#define GS_CONSOLE_BUF_SIZE(2 * GS_BUFFER_SIZE)
> +
> +struct gscons_info {
> + struct gs_port *port;
> + struct tty_driver *tty_driver;
> + struct work_struct work;
> + int buf_tail;
> + charbuf[GS_CONSOLE_BUF_SIZE];
> >>>
> >>> Can't be malloced once?
> >> The 'gscons_info' structure is malloced once.
> >
> > In state of high fragmentation is quite hard to find big memory chunks.
> > I would split it to two allocations, though if maintainers are okay
> > with your code, then I'm also okay.
> >
>
> Make sense. But I think the major memory of the 'struct gscons_info'
> is for the 'buf' member, so I still think no need to allocate it 2
> times.
It may be worth just reducing GS_BUFFER_SIZE slightly so that the gscons_info
structure itself is less than 8k.
If you can't get 2 adjacent pages then a lot of things will fail.
David
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On Wed, Nov 18, 2015 at 9:38 PM, Hans de Goede wrote: > Hi, > > On 18-11-15 10:46, Philipp Zabel wrote: >> >> Hi Hans, >> >> Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: >>> >>> On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? >>> >>> >>> Not to my knowledge, I suggest that we simply treat this same as >>> regulators / clocks where the first one to enable it actually gets >>> it enabled (de-asserted in case of a reset), and the last one >>> to disable (assert) it (so dropping the usage counter back to 0) leads >>> to it being asserted again. >>> >>> This seems to work well enough for clocks / regulators and phys, and >>> at least for my use-case it should work fine for the shared reset too >>> (I will test to make sure of course). >>> >>> So from my pov a simple counter should suffice, does that work for you ? >> >> >> I don't quite understand what counting will help with, then. The first >> driver to call reset_control_deassert will deassert the reset, whether >> you count or not. >> But if the two drivers have deasserted an initially asserted reset, a >> reset_control_assert for one of them will silently fail. > > > Correct, which is what we want, although I would not call it silently > fail I would call it a nop as it is intended. > >> I fear the deassertion count maps well to the case where resets are used >> just like clocks (when inactive modules are kept in reset), but I'm not >> sure this is useful in the case of resets that are kept deasserted most >> of the time, only to be asserted for a short pulse. Maybe we have to >> differentiate between the two cases? > > > Ack. I think that the "just like clocks" case is the more common one, and > it seems to me that the short-pulse case should use reset_control_reset. > > Maybe we need to provide a default implementation of reset_control_reset > which > does the pulse when no implementation is provided by the driver ? > > Although that brings the question with it what to do with the deassert_count > in > that case, as some drivers may also use that for the initial deassert... I > guess > we could document to not do that if you want to assure that no other drivers > muck with the reset-line ... > > Hmm, this is getting messy pretty quickly. New proposal: > > 1) Add a concept of shared resets, adding: reset_control_get_shared and >devm_reset_control_get_shared functions, which set a shared bool >in struct reset_control Assuming board designers are sufficiently ... stupid, won't most drivers eventually need to use the _shared variants? Wouldn't it be (horrifically more complicated and) better to just build this into the generic code and switch on the shared reset line handling if multiple devices take references to the same reset line? (Can we run through the device tree and mark them in advance? What happens with overlays?) That said, I suspect that to deal sensibly with shared reset lines we're also going to need some way to init devices in lockstep with others, i.e. we can't deassert device A until it's clocks are running, however the reset line is shared with device B who also can't be deasserted until it's clocks are running. This seems like it'll get rather complicated quickly, to say the least. It almost seems that, unless some other board has a similarly broken design, that it might almost be easier to just make variants of generic-ehci and generic-ohci to handle this one particular case. > 2) Add int refcnt to struct reset_controller_dev, which gets >incremented/decremented on reset_control_get/reset_control_put >do a BUG_ON on refcnt == 1 in the get functions when the non-shared >variant gets called (this is optional but probably a good extra >check) > > 3) Do the whole deassert_count thingy only when the shared bool is true > > 4) Make reset_control_reset fail (BUG_ON) if the shared bool is true > > Regards, > > Hans > > > -- > You received this message because you are subscribed to the Google Groups > "linux-sunxi" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to linux-sunxi+unsubscr...@googlegroups.com. > For more options, visit https://groups.google.com/d/optout. -- Julian Calaby Email: julian.cal...@gmail.com Profile: http://www.google.com/profiles/julian.calaby/ -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 18 November 2015 at 20:05, David Laight wrote:
> From: Baolin Wang
>> Sent: 18 November 2015 10:45
>> On 18 November 2015 at 17:32, Andy Shevchenko
>> wrote:
>> > On Wed, Nov 18, 2015 at 4:15 AM, Baolin Wang
>> > wrote:
>> >> On 17 November 2015 at 21:34, Andy Shevchenko
>> >> wrote:
>> >>> On Mon, Nov 16, 2015 at 9:05 AM, Baolin Wang
>> >>> wrote:
>> It dose not work when we want to use the usb-to-serial port based
>> on one usb gadget as a console. Thus this patch adds the console
>> initialization to support this request.
>> >>>
>> >
>> +#define GS_BUFFER_SIZE (4096)
>> >>> Redundant parens
>> >> OK. I'll remove it.
>> >>
>> +#define GS_CONSOLE_BUF_SIZE(2 * GS_BUFFER_SIZE)
>> +
>> +struct gscons_info {
>> + struct gs_port *port;
>> + struct tty_driver *tty_driver;
>> + struct work_struct work;
>> + int buf_tail;
>> + charbuf[GS_CONSOLE_BUF_SIZE];
>> >>>
>> >>> Can't be malloced once?
>> >> The 'gscons_info' structure is malloced once.
>> >
>> > In state of high fragmentation is quite hard to find big memory chunks.
>> > I would split it to two allocations, though if maintainers are okay
>> > with your code, then I'm also okay.
>> >
>>
>> Make sense. But I think the major memory of the 'struct gscons_info'
>> is for the 'buf' member, so I still think no need to allocate it 2
>> times.
>
> It may be worth just reducing GS_BUFFER_SIZE slightly so that the gscons_info
> structure itself is less than 8k.
> If you can't get 2 adjacent pages then a lot of things will fail.
>
But its allocation is called in early booting time, I think there are
not many memory fragments now.
> David
>
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi Julian, Am Mittwoch, den 18.11.2015, 23:32 +1100 schrieb Julian Calaby: [...] > Assuming board designers are sufficiently ... stupid, won't most > drivers eventually need to use the _shared variants? Wouldn't it be > (horrifically more complicated and) better to just build this into the > generic code and switch on the shared reset line handling if multiple > devices take references to the same reset line? (Can we run through > the device tree and mark them in advance? What happens with overlays?) Either way, I think we need some way to differentiate between two different meanings of reset_control_assert - reset_control_may_assert (the clock-like case) and reset_control_must_assert (the fix broken hardware state case). > That said, I suspect that to deal sensibly with shared reset lines > we're also going to need some way to init devices in lockstep with > others, i.e. we can't deassert device A until it's clocks are running, > however the reset line is shared with device B who also can't be > deasserted until it's clocks are running. This seems like it'll get > rather complicated quickly, to say the least. > > It almost seems that, unless some other board has a similarly broken > design, that it might almost be easier to just make variants of > generic-ehci and generic-ohci to handle this one particular case. On i.MX6Q the 2D rasterizer and 2D vector graphics cores share a reset line, but there we have the luxury of a single driver handling both (and furthermore the cores have their own soft reset bits). regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] usb: dwc3: Add Xilinx ZynqMP platform specific glue layer
This patch adds glue driver for DWC3 core in Xilinx ZynqMP SOC.
DWC3 glue layer is hardware layer around Synopsys DesignWare
USB3 core. Its purpose is to supply Synopsys IP with required clocks
and interface it with the rest of the SoC.
Signed-off-by: Subbaraya Sundeep Bhatta
---
drivers/usb/dwc3/Kconfig | 8 +++
drivers/usb/dwc3/Makefile | 1 +
drivers/usb/dwc3/dwc3-xilinx.c | 131 +
3 files changed, 140 insertions(+)
create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 5a42c45..a447879 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -104,4 +104,12 @@ config USB_DWC3_QCOM
Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside,
say 'Y' or 'M' if you have one such device.
+config USB_DWC3_XILINX
+ tristate "Xilinx ZynqMP Platform"
+ depends on ARCH_ZYNQMP || COMPILE_TEST
+ default USB_DWC3
+ help
+ Xilinx ZynqMP SOC ship with two DesignWare Core USB3 IPs inside,
+ say 'Y' or 'M' if you have one such device.
+
endif
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index acc951d..50cb626 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -39,3 +39,4 @@ obj-$(CONFIG_USB_DWC3_PCI)+= dwc3-pci.o
obj-$(CONFIG_USB_DWC3_KEYSTONE)+= dwc3-keystone.o
obj-$(CONFIG_USB_DWC3_QCOM)+= dwc3-qcom.o
obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o
+obj-$(CONFIG_USB_DWC3_XILINX) += dwc3-xilinx.o
diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
new file mode 100644
index 000..a0dce3e
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-xilinx.c
@@ -0,0 +1,131 @@
+/**
+ * dwc3-xilinx.c - Xilinx ZynqMP specific Glue layer
+ *
+ * Copyright (C) 2015 Xilinx Inc.
+ *
+ * Author: Subbaraya Sundeep
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+
+/**
+ * struct xilinx_dwc3 - dwc3 xilinx glue structure
+ * @dev: device pointer
+ * @ref_clk: clock input to core during PHY power down
+ * @bus_clk: bus clock input to core
+ */
+struct xilinx_dwc3 {
+ struct device *dev;
+ struct clk *ref_clk;
+ struct clk *bus_clk;
+};
+
+/**
+ * xilinx_dwc3_probe - The device probe function for driver initialization.
+ * @pdev: pointer to the platform device structure.
+ *
+ * Return: 0 for success and error value on failure
+ */
+static int xilinx_dwc3_probe(struct platform_device *pdev)
+{
+ struct device_node *node = pdev->dev.of_node;
+ struct xilinx_dwc3 *xdwc3;
+ int ret;
+
+ xdwc3 = devm_kzalloc(&pdev->dev, sizeof(*xdwc3), GFP_KERNEL);
+ if (!xdwc3)
+ return -ENOMEM;
+
+ xdwc3->dev = &pdev->dev;
+
+ xdwc3->bus_clk = devm_clk_get(xdwc3->dev, "bus_clk");
+ if (IS_ERR(xdwc3->bus_clk)) {
+ dev_err(xdwc3->dev, "unable to get usb bus clock");
+ return PTR_ERR(xdwc3->bus_clk);
+ }
+
+ xdwc3->ref_clk = devm_clk_get(xdwc3->dev, "ref_clk");
+ if (IS_ERR(xdwc3->ref_clk)) {
+ dev_err(xdwc3->dev, "unable to get usb ref clock");
+ return PTR_ERR(xdwc3->ref_clk);
+ }
+
+ ret = clk_prepare_enable(xdwc3->bus_clk);
+ if (ret)
+ goto err_bus_clk;
+ ret = clk_prepare_enable(xdwc3->ref_clk);
+ if (ret)
+ goto err_ref_clk;
+
+ platform_set_drvdata(pdev, xdwc3);
+
+ ret = of_platform_populate(node, NULL, NULL, xdwc3->dev);
+ if (ret) {
+ dev_err(xdwc3->dev, "failed to create dwc3 core\n");
+ goto err_dwc3_core;
+ }
+
+ return 0;
+
+err_dwc3_core:
+ clk_disable_unprepare(xdwc3->ref_clk);
+err_ref_clk:
+ clk_disable_unprepare(xdwc3->bus_clk);
+err_bus_clk:
+ return ret;
+}
+
+/**
+ * xilinx_dwc3_remove - Releases the resources allocated during initialization.
+ * @pdev: pointer to the platform device structure.
+ *
+ * Return: 0 always
+ */
+static int xilinx_dwc3_remove(struct platform_device *pdev)
+{
+ struct xilinx_dwc3 *xdwc3 = platform_get_drvdata(pdev);
+
+ of_platform_depopulate(xdwc3->dev);
+
+ clk_disable_unprepare(xdwc3->bus_clk);
+ clk_disable_unprepare(xdwc3->ref_clk);
+ platform_set_drvdata(pdev, NULL);
+
+ return 0;
+}
+
+/* Match table for of_platform binding */
+static const struct of_device_id xilinx_dwc3_o
[PATCH 1/2] usb: doc: dwc3-xilinx: Add devicetree bindings
This patch adds binding doc for Xilinx DWC3 glue driver.
Signed-off-by: Subbaraya Sundeep Bhatta
---
.../devicetree/bindings/usb/dwc3-xilinx.txt| 33 ++
1 file changed, 33 insertions(+)
create mode 100644 Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
new file mode 100644
index 000..30361b3
--- /dev/null
+++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
@@ -0,0 +1,33 @@
+Xilinx SuperSpeed DWC3 USB SoC controller
+
+Required properties:
+- compatible: Should contain "xlnx,zynqmp-dwc3"
+- clocks: A list of phandles for the clocks listed in clock-names
+- clock-names: Should contain the following:
+ "bus_clk" Master/Core clock, have to be >= 125 MHz for SS
+operation and >= 60MHz for HS operation
+
+ "ref_clk" Clock source to core during PHY power down
+
+Required child node:
+A child node must exist to represent the core DWC3 IP block. The name of
+the node is not important. The content of the node is defined in dwc3.txt.
+
+Example device node:
+
+ usb@0 {
+ #address-cells = <0x2>;
+ #size-cells = <0x1>;
+ status = "okay";
+ compatible = "xlnx,zynqmp-dwc3";
+ clock-names = "bus_clk" "ref_clk";
+ clocks = <&clk125>, <&clk125>;
+ ranges;
+
+ dwc3@fe20 {
+ compatible = "snps,dwc3";
+ reg = <0x0 0xfe20 0x4>;
+ interrupts = <0x0 0x41 0x4>;
+ dr_mode = "host";
+ };
+ };
--
2.1.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Tue, Nov 17, 2015 at 02:07:58PM -0500, Alan Stern wrote: > If we really want to do zerocopy I/O then we should not use a bounce > buffer. But the only way to avoid bounce buffers may be to give user > programs a way of allocating memory pages below 4 GB, because lots of > USB hardware can only do 32-bit DMA. But any system worth it's money these days has an IOMMU. > Is there an API for allocating user memory below 4 GB? Would a new > MMAP flag be acceptable? You'll have to ask the MM folks. But I doubt they will be excited about it. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Wed, Nov 18, 2015 at 3:02 PM, Christoph Hellwig wrote: > On Tue, Nov 17, 2015 at 02:07:58PM -0500, Alan Stern wrote: >> If we really want to do zerocopy I/O then we should not use a bounce >> buffer. But the only way to avoid bounce buffers may be to give user >> programs a way of allocating memory pages below 4 GB, because lots of >> USB hardware can only do 32-bit DMA. > > But any system worth it's money these days has an IOMMU. > you're definitely wrong with that routers, smartphones, settopboxes, NAS systems, etc. seems like you're sticking with X86 platform. USB is the most versatile bus system out there you'll find it in the range from embedded microprocessors up to high end systems. You will always have to expect that some system will not or some architecture will never support IOMMU due cut down design limitations out there. >> Is there an API for allocating user memory below 4 GB? Would a new >> MMAP flag be acceptable? > > You'll have to ask the MM folks. But I doubt they will be excited about it. Markus -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [linux-sunxi] Re: [PATCH 2/2] ehci-platform: Add support for controllers with multiple reset lines
Hi, On 18-11-15 13:32, Julian Calaby wrote: Hi, On Wed, Nov 18, 2015 at 9:38 PM, Hans de Goede wrote: Hi, On 18-11-15 10:46, Philipp Zabel wrote: Hi Hans, Am Montag, den 16.11.2015, 18:13 +0100 schrieb Hans de Goede: On 16-11-15 18:01, Philipp Zabel wrote: If there are two devices sharing the same reset line that is initially held asserted, do the two drivers somehow have to synchronize before releasing the reset together? Not to my knowledge, I suggest that we simply treat this same as regulators / clocks where the first one to enable it actually gets it enabled (de-asserted in case of a reset), and the last one to disable (assert) it (so dropping the usage counter back to 0) leads to it being asserted again. This seems to work well enough for clocks / regulators and phys, and at least for my use-case it should work fine for the shared reset too (I will test to make sure of course). So from my pov a simple counter should suffice, does that work for you ? I don't quite understand what counting will help with, then. The first driver to call reset_control_deassert will deassert the reset, whether you count or not. But if the two drivers have deasserted an initially asserted reset, a reset_control_assert for one of them will silently fail. Correct, which is what we want, although I would not call it silently fail I would call it a nop as it is intended. I fear the deassertion count maps well to the case where resets are used just like clocks (when inactive modules are kept in reset), but I'm not sure this is useful in the case of resets that are kept deasserted most of the time, only to be asserted for a short pulse. Maybe we have to differentiate between the two cases? Ack. I think that the "just like clocks" case is the more common one, and it seems to me that the short-pulse case should use reset_control_reset. Maybe we need to provide a default implementation of reset_control_reset which does the pulse when no implementation is provided by the driver ? Although that brings the question with it what to do with the deassert_count in that case, as some drivers may also use that for the initial deassert... I guess we could document to not do that if you want to assure that no other drivers muck with the reset-line ... Hmm, this is getting messy pretty quickly. New proposal: 1) Add a concept of shared resets, adding: reset_control_get_shared and devm_reset_control_get_shared functions, which set a shared bool in struct reset_control Assuming board designers are sufficiently ... stupid, won't most drivers eventually need to use the _shared variants? Wouldn't it be (horrifically more complicated and) better to just build this into the generic code and switch on the shared reset line handling if multiple devices take references to the same reset line? (Can we run through the device tree and mark them in advance? What happens with overlays?) That said, I suspect that to deal sensibly with shared reset lines we're also going to need some way to init devices in lockstep with others, i.e. we can't deassert device A until it's clocks are running, however the reset line is shared with device B who also can't be deasserted until it's clocks are running. This seems like it'll get rather complicated quickly, to say the least. It almost seems that, unless some other board has a similarly broken design, that it might almost be easier to just make variants of generic-ehci and generic-ohci to handle this one particular case. Having a specific version of the ehci / ohci driver is not going to help, unless a lot of magic is added to these drivers to coordinate between themselves. Magic which is not strictly necessary, as a simple usage-counter in the reset-core suffices and is much more KISS. I believe that the solution is to add a usage-counting variant of reset_control_[de]assert and switch to that in the case where the only needed coordination is to not assert the reset when one of the 2 drivers is still active. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: Infrastructure for zerocopy I/O
From: Christoph Hellwig > Sent: 18 November 2015 14:02 > On Tue, Nov 17, 2015 at 02:07:58PM -0500, Alan Stern wrote: > > If we really want to do zerocopy I/O then we should not use a bounce > > buffer. But the only way to avoid bounce buffers may be to give user > > programs a way of allocating memory pages below 4 GB, because lots of > > USB hardware can only do 32-bit DMA. > > But any system worth it's money these days has an IOMMU. If a system has an iommu, then the cost of updating the iommu itself becomes significant. This is on top of any TLB and cache operations that are required for typical page loaning schemes. One scheme that might work (if you have an open fd into the usb driver) is to do an mmap() request on the usb driver that will make it allocate DMA-able memory and map it into the applications address space. The USB transfer buffers can then be arranged to be in this area. David -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
Hi Baolin,
On 11/16/2015 02:05 AM, Baolin Wang wrote:
> It dose not work when we want to use the usb-to-serial port based
> on one usb gadget as a console. Thus this patch adds the console
> initialization to support this request.
I have some high level concerns.
1. I would defer registering the console until the port has at least been
allocated in gserial_alloc_line(), and unregister when the line is freed.
That also reduces many of the conditions that you shouldn't need to
check, like port number range and so on.
Further, consider deferring the console registration until gserial_connect();
that would further simplify things. In this case, unregistration would
happen at disconnect.
2. Why are you using a kworker for actual device i/o when all of the i/o
is done with interrupts disabled anyway?
Getting rid of the work would eliminate using the 8K intermediate buffer
as well.
3. If for some reason i/o deferral is really necessary, consider using a kthread
kworker instead of the pooled kworker. The scheduler response will be _way_
better.
4. If for some reason i/o deferral is really necessary, use a circular buffer
for the intermediate buffer, preferably lockless since there is only
one producer and one consumer.
Some other review comments below; please ignore anything other reviewers
have already noted.
Regards,
Peter Hurley
> Signed-off-by: Baolin Wang
> ---
> drivers/usb/gadget/Kconfig |6 +
> drivers/usb/gadget/function/u_serial.c | 238
>
> 2 files changed, 244 insertions(+)
>
> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
> index 33834aa..be5aab9 100644
> --- a/drivers/usb/gadget/Kconfig
> +++ b/drivers/usb/gadget/Kconfig
> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
> a module parameter as well.
> If unsure, say 2.
>
> +config U_SERIAL_CONSOLE
> + bool "Serial gadget console support"
> + depends on USB_G_SERIAL
> + help
> +It supports the serial gadget can be used as a console.
> +
> source "drivers/usb/gadget/udc/Kconfig"
>
> #
> diff --git a/drivers/usb/gadget/function/u_serial.c
> b/drivers/usb/gadget/function/u_serial.c
> index f7771d8..4ade527 100644
> --- a/drivers/usb/gadget/function/u_serial.c
> +++ b/drivers/usb/gadget/function/u_serial.c
> @@ -27,6 +27,7 @@
> #include
> #include
> #include
> +#include
>
> #include "u_serial.h"
>
> @@ -79,6 +80,16 @@
> */
> #define QUEUE_SIZE 16
> #define WRITE_BUF_SIZE 8192/* TX only */
> +#define GS_BUFFER_SIZE (4096)
> +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE)
> +
> +struct gscons_info {
> + struct gs_port *port;
> + struct tty_driver *tty_driver;
> + struct work_struct work;
> + int buf_tail;
> + charbuf[GS_CONSOLE_BUF_SIZE];
> +};
Make struct gscons_info a static declaration instead of
allocating it.
>
> /* circular buffer */
> struct gs_buf {
> @@ -118,6 +129,7 @@ struct gs_port {
>
> /* REVISIT this state ... */
> struct usb_cdc_line_coding port_line_coding;/* 8-N-1 etc */
> + struct usb_request *console_req;
> };
>
> static struct portmaster {
> @@ -1054,6 +1066,7 @@ gs_port_alloc(unsigned port_num, struct
> usb_cdc_line_coding *coding)
>
> port->port_num = port_num;
> port->port_line_coding = *coding;
> + port->console_req = NULL;
>
> ports[port_num].port = port;
> out:
> @@ -1143,6 +1156,227 @@ err:
> }
> EXPORT_SYMBOL_GPL(gserial_alloc_line);
>
> +#ifdef CONFIG_U_SERIAL_CONSOLE
> +
> +static struct usb_request *gs_request_new(struct usb_ep *ep, int buffer_size)
^^^
With only a single caller that uses a symbolic constant, is the
'buffer_size' parameter necessary?
> +{
> + struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
> +
Remove this newline.
> + if (!req)
> + return NULL;
> +
> + /* now allocate buffers for the requests */
Unnecessary comment.
> + req->buf = kmalloc(buffer_size, GFP_ATOMIC);
> + if (!req->buf) {
> + usb_ep_free_request(ep, req);
> + return NULL;
> + }
> +
> + return req;
> +}
> +
> +static void gs_request_free(struct usb_request *req, struct usb_ep *ep)
> +{
> + if (req) {
> + kfree(req->buf);
> + usb_ep_free_request(ep, req);
> + }
> +}
> +
> +static void gs_complete_out(struct usb_ep *ep, struct usb_request *req)
> +{
> + if (req->status != 0 && req->status != -ECONNRESET)
> + return;
> +}
> +
> +static struct console gserial_cons;
> +static int gs_console_connect(void)
Pass the console * as parameter, instead of forward declaring the console.
Or initialize info directly from the static struct gscons_info address.
> +
Re: Help needed for EHCI problem: removing an active bulk-in QH
On Tue, Nov 17, 2015 at 12:21 PM, Alan Stern wrote: > On Tue, 17 Nov 2015, Michael Reutman wrote: >> Ran 1 million operations of launch+cancel usb transfers last night >> without getting into the stuck state. I'll bump up the iterations to >> 10 million or so and run it again tonight just to be certain, but I >> think the last patch has the fix needed for this particular hardware. > > Okay, that sounds good. If everything works out, I'll write a patch > that does all this properly and ask you to test it. I ended up setting the usb test to run until the user decided to cancel it and ran it overnight for a total of 18 hours. Appears to have avoided the "stuck" state! > PS: Assuming it does work out, I would appreciate seeing the usb_snoop > output for a run containing just 20 iterations or so. Do you want me to grab usb_snoop for the last patch you provided or with the formal patch? Also, do you want usb_snoop_max enabled as well? Glad to be reaching a definite conclusion to this problem. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb-gadget-functionfs: fix missing access_ok checks
use safe copy_*_user instead of unsafe __copy_*_user
functions when accessing userland memory.
Signed-off-by: Daniel Walter
---
drivers/usb/gadget/function/f_fs.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/gadget/function/f_fs.c
b/drivers/usb/gadget/function/f_fs.c
index adc6d52..cf43e9e 100644
--- a/drivers/usb/gadget/function/f_fs.c
+++ b/drivers/usb/gadget/function/f_fs.c
@@ -423,7 +423,7 @@ static ssize_t __ffs_ep0_read_events(struct ffs_data *ffs,
char __user *buf,
spin_unlock_irq(&ffs->ev.waitq.lock);
mutex_unlock(&ffs->mutex);
- return unlikely(__copy_to_user(buf, events, size)) ? -EFAULT : size;
+ return unlikely(copy_to_user(buf, events, size)) ? -EFAULT : size;
}
static ssize_t ffs_ep0_read(struct file *file, char __user *buf,
@@ -513,7 +513,7 @@ static ssize_t ffs_ep0_read(struct file *file, char __user
*buf,
/* unlocks spinlock */
ret = __ffs_ep0_queue_wait(ffs, data, len);
- if (likely(ret > 0) && unlikely(__copy_to_user(buf, data, len)))
+ if (likely(ret > 0) && unlikely(copy_to_user(buf, data, len)))
ret = -EFAULT;
goto done_mutex;
@@ -3493,7 +3493,7 @@ static char *ffs_prepare_buffer(const char __user *buf,
size_t len)
if (unlikely(!data))
return ERR_PTR(-ENOMEM);
- if (unlikely(__copy_from_user(data, buf, len))) {
+ if (unlikely(copy_from_user(data, buf, len))) {
kfree(data);
return ERR_PTR(-EFAULT);
}
--
2.6.2
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
On Wed, 18 Nov 2015, Peter Stuge wrote: > Alan Stern wrote: > > if we can directly map the user-provided buffer for DMA. > > Given a memory buffer either kernel or userspace has to apply the > hardware constraints to find out what part if any is usable for DMA. > > I think it's fine to push this task to userspace - as long as > userspace has a way to take care of it. That means userspace must be > able to perform an allocation according to the constraints. I don't > think this is currently possible. Is that correct? As far as I know, it's not possible. In fact, the situation is worse than I thought at first. Memory allocated by userspace generally will not be located in physically contiguous pages. This means that using the memory as an I/O buffer will require scatter-gather. But the USB host controller drivers support scatter-gather only for bulk transfers, not isochronous. Of course, isochronous transfers already involve a weak sort of scatter-gather in the form of usbdevfs_iso_packet_desc lists. But at the driver level, the packets described by these lists must all sit in a single contiguous buffer. In theory a user program can work around this by submitting lots of small isochronous transfers instead of one big transfer. But that wastes resources in the form of excessive system calls. > The kernel could certainly do the allocation and hand the buffer to > userspace. Wiping the buffer once upon allocation is a reasonable > cost, because this allocation should be long-lived. For isochronous especially, this seems like the only way to go. > > On the other hand, adding an API that allows users to allocate low > > memory and then hiding it in the USB subsystem doesn't seem like a > > good idea. > > Unless another generic API already supports the neccessary constraints > then what other options are there? Adding a new generic API instead of hiding it in the USB subsystem. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
driver and that may not be available when CONFIG_DMADEVICES
is not set:
warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
dependencies (DMADEVICES && ARCH_OMAP)
This adds an extra dependency to avoid generating warnings in randconfig
builds. Ideally we'd remove the 'select' statement, but that has the
potential to break defconfig files.
Signed-off-by: Arnd Bergmann
Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode of
MUSB is enabled")
diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 1f2037bbeb0d..45c83baf675d 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
config USB_TI_CPPI41_DMA
bool 'TI CPPI 4.1 (AM335x)'
- depends on ARCH_OMAP
+ depends on ARCH_OMAP && DMADEVICES
select TI_CPPI41
config USB_TUSB_OMAP_DMA
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Help needed for EHCI problem: removing an active bulk-in QH
On Wed, 18 Nov 2015, Michael Reutman wrote: > On Tue, Nov 17, 2015 at 12:21 PM, Alan Stern > wrote: > > On Tue, 17 Nov 2015, Michael Reutman wrote: > >> Ran 1 million operations of launch+cancel usb transfers last night > >> without getting into the stuck state. I'll bump up the iterations to > >> 10 million or so and run it again tonight just to be certain, but I > >> think the last patch has the fix needed for this particular hardware. > > > > Okay, that sounds good. If everything works out, I'll write a patch > > that does all this properly and ask you to test it. > > I ended up setting the usb test to run until the user decided to > cancel it and ran it overnight for a total of 18 hours. Appears to > have avoided the "stuck" state! Great! > > PS: Assuming it does work out, I would appreciate seeing the usb_snoop > > output for a run containing just 20 iterations or so. > > Do you want me to grab usb_snoop for the last patch you provided or > with the formal patch? Also, do you want usb_snoop_max enabled as > well? For the last patch. And yes, do set usb_snoop_max. > Glad to be reaching a definite conclusion to this problem. I hope so. There is one thing I'm still undecided about: Should this workaround be applied only to AMD/ATI controllers or should it apply to everything? It does have a small probability of slowing down transfers under certain circumstances, but on the other hand, it's quite possible that other controller types will have the same sort of weakness as the AMD ones. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Allocating DMA-able memory for user programs
Memory management folk: People have been complaining about memory-related problems with their userspace USB drivers. There are two basic issues: Memory fragmentation eventually prevents the kernel from allocating contiguous buffers large enough to hold the I/O data. Such buffers currently have to be allocated for each individual I/O transfer. Copying data back and forth between the userspace and kernel buffers wastes a lot of time. The ideal solution, of course, is to use some form of zerocopy I/O, telling the hardware to DMA to/from the userspace buffer directly. However, we are under some constraints that make this difficult. Mapping a userspace buffer for DMA implies using some form of scatter-gather, because pages with adjacent virtual addresses generally are not physically adjacent. But the USB kernel drivers do not support scatter-gather for isochronous transfers, only for bulk transfers (and the complaints here are concerned with isochronous). Even if scatter-gather weren't an issue, user memory pages are not always usable for hardware DMA. Lots of USB controllers do only 32-bit DMA, so the pages containing the user buffer would have to be located physically below 4 GB. (If an IOMMU is present this may not matter, but plenty of lower-end systems don't have an IOMMU.) We want to avoid using automatic bounce buffers, for two reasons. First, they obviously defeat the purpose of zerocopy I/O. Second, isochronous READ transfers often leave gaps in the data buffer. (For example, a buffer might be set up to hold two 32-byte transfers, but the first transfer might only receive 20 bytes of data. The buffer would end up containing 20 bytes of data read in, followed by a 12-byte gap holding stale data -- whatever happened to be there before -- followed by 32 bytes of data read in.) If we use a bounce buffer automatically allocated in the kernel, we have no way to prevent the stale data in the gaps from being copied back to userspace, which would be a security leak. The only solution we have come up with is to create a device-specific mmap(2) implementation that would allocate contiguous pages within the device's DMA mask and map them into the user's virtual address space. The user program could then use these pages as a buffer to get true zerocopy I/O. There's the potential issue of exhausting a limited resource (memory below 4 GB), but we can take care of that by placing on overall cap on the amount of memory allocated using this mechanism. Does this seem reasonable? I'm not certain about the wisdom of creating an API for allocating and locking pages below 4 GB and then hiding it away in the USB subsystem. But if you folks say it's okay, we'll go ahead and do it. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [GIT PULL] USB Chipidea fixes for v4.4-rc2
On Wed, Nov 18, 2015 at 03:43:58PM +0800, Peter Chen wrote: > The following changes since commit aa05cfa95f686be5d1485094402ebc7b03729e0e: > > Merge tag 'fixes-for-v4.4-rc2' of > git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb into usb-linus > (2015-11-17 14:48:24 -0800) > > are available in the git repository at: > > > git://git.kernel.org/pub/scm/linux/kernel/git/peter.chen/usb.git/ > tags/usb-ci-v4.4-rc2 Pulled and pushed out, thanks. greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] usb: dwc3: Add Xilinx ZynqMP platform specific glue layer
Hi,
Subbaraya Sundeep Bhatta writes:
> This patch adds glue driver for DWC3 core in Xilinx ZynqMP SOC.
> DWC3 glue layer is hardware layer around Synopsys DesignWare
> USB3 core. Its purpose is to supply Synopsys IP with required clocks
> and interface it with the rest of the SoC.
>
> Signed-off-by: Subbaraya Sundeep Bhatta
> ---
> drivers/usb/dwc3/Kconfig | 8 +++
> drivers/usb/dwc3/Makefile | 1 +
> drivers/usb/dwc3/dwc3-xilinx.c | 131
> +
> 3 files changed, 140 insertions(+)
> create mode 100644 drivers/usb/dwc3/dwc3-xilinx.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index 5a42c45..a447879 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -104,4 +104,12 @@ config USB_DWC3_QCOM
> Recent Qualcomm SoCs ship with one DesignWare Core USB3 IP inside,
> say 'Y' or 'M' if you have one such device.
>
> +config USB_DWC3_XILINX
> + tristate "Xilinx ZynqMP Platform"
> + depends on ARCH_ZYNQMP || COMPILE_TEST
> + default USB_DWC3
> + help
> + Xilinx ZynqMP SOC ship with two DesignWare Core USB3 IPs inside,
> + say 'Y' or 'M' if you have one such device.
> +
> endif
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index acc951d..50cb626 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -39,3 +39,4 @@ obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o
> obj-$(CONFIG_USB_DWC3_KEYSTONE) += dwc3-keystone.o
> obj-$(CONFIG_USB_DWC3_QCOM) += dwc3-qcom.o
> obj-$(CONFIG_USB_DWC3_ST)+= dwc3-st.o
> +obj-$(CONFIG_USB_DWC3_XILINX)+= dwc3-xilinx.o
> diff --git a/drivers/usb/dwc3/dwc3-xilinx.c b/drivers/usb/dwc3/dwc3-xilinx.c
> new file mode 100644
> index 000..a0dce3e
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-xilinx.c
> @@ -0,0 +1,131 @@
> +/**
> + * dwc3-xilinx.c - Xilinx ZynqMP specific Glue layer
> + *
> + * Copyright (C) 2015 Xilinx Inc.
> + *
> + * Author: Subbaraya Sundeep
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 of
> + * the License as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +/**
> + * struct xilinx_dwc3 - dwc3 xilinx glue structure
> + * @dev: device pointer
> + * @ref_clk: clock input to core during PHY power down
> + * @bus_clk: bus clock input to core
> + */
> +struct xilinx_dwc3 {
> + struct device *dev;
> + struct clk *ref_clk;
> + struct clk *bus_clk;
> +};
> +
> +/**
> + * xilinx_dwc3_probe - The device probe function for driver initialization.
> + * @pdev: pointer to the platform device structure.
> + *
> + * Return: 0 for success and error value on failure
> + */
> +static int xilinx_dwc3_probe(struct platform_device *pdev)
> +{
> + struct device_node *node = pdev->dev.of_node;
> + struct xilinx_dwc3 *xdwc3;
> + int ret;
> +
> + xdwc3 = devm_kzalloc(&pdev->dev, sizeof(*xdwc3), GFP_KERNEL);
> + if (!xdwc3)
> + return -ENOMEM;
> +
> + xdwc3->dev = &pdev->dev;
> +
> + xdwc3->bus_clk = devm_clk_get(xdwc3->dev, "bus_clk");
> + if (IS_ERR(xdwc3->bus_clk)) {
> + dev_err(xdwc3->dev, "unable to get usb bus clock");
> + return PTR_ERR(xdwc3->bus_clk);
> + }
> +
> + xdwc3->ref_clk = devm_clk_get(xdwc3->dev, "ref_clk");
> + if (IS_ERR(xdwc3->ref_clk)) {
> + dev_err(xdwc3->dev, "unable to get usb ref clock");
> + return PTR_ERR(xdwc3->ref_clk);
> + }
> +
> + ret = clk_prepare_enable(xdwc3->bus_clk);
> + if (ret)
> + goto err_bus_clk;
> + ret = clk_prepare_enable(xdwc3->ref_clk);
> + if (ret)
> + goto err_ref_clk;
> +
> + platform_set_drvdata(pdev, xdwc3);
> +
> + ret = of_platform_populate(node, NULL, NULL, xdwc3->dev);
> + if (ret) {
> + dev_err(xdwc3->dev, "failed to create dwc3 core\n");
> + goto err_dwc3_core;
> + }
> +
> + return 0;
> +
> +err_dwc3_core:
> + clk_disable_unprepare(xdwc3->ref_clk);
> +err_ref_clk:
> + clk_disable_unprepare(xdwc3->bus_clk);
> +err_bus_clk:
> + return ret;
> +}
> +
> +/**
> + * xilinx_dwc3_remove - Releases the resources allocated during
> initialization.
> + * @pdev: pointer to the platform device structure.
> + *
> + * Return: 0 always
> + */
> +static int xilinx_dwc3_remove(struct platform_device *pdev)
> +{
> + struct xilinx_dwc3 *xdwc3 = platform_get_drvdata(pdev);
> +
> + of_platform_depopulate(xdwc3->dev);
> +
> +
Re: Infrastructure for zerocopy I/O
"Steinar H. Gunderson" writes: > On Tue, Nov 17, 2015 at 03:16:55PM -0500, Alan Stern wrote: >> xHCI always uses 64-bit addresses. But many EHCI controllers don't, >> and only a few of the EHCI platform drivers support 64-bit DMA. > > OK, sure. But are systems with USB2 only and more than 4GB of RAM common? Hmpff. They are common in my house at least :) bjorn@nemi:~$ lspci -nn|grep USB 00:1a.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #4 [8086:2937] (rev 03) 00:1a.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #5 [8086:2938] (rev 03) 00:1a.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #6 [8086:2939] (rev 03) 00:1a.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #2 [8086:293c] (rev 03) 00:1d.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #1 [8086:2934] (rev 03) 00:1d.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #2 [8086:2935] (rev 03) 00:1d.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #3 [8086:2936] (rev 03) 00:1d.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #1 [8086:293a] (rev 03) bjorn@nemi:~$ grep MemTotal /proc/meminfo MemTotal:8051536 kB bjorn@canardo:~$ lspci -nn|grep USB 00:1a.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #4 [8086:2937] (rev 02) 00:1a.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #5 [8086:2938] (rev 02) 00:1a.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #6 [8086:2939] (rev 02) 00:1a.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #2 [8086:293c] (rev 02) 00:1d.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #1 [8086:2934] (rev 02) 00:1d.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #2 [8086:2935] (rev 02) 00:1d.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB UHCI Controller #3 [8086:2936] (rev 02) 00:1d.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 EHCI Controller #1 [8086:293a] (rev 02) bjorn@canardo:~$ grep MemTotal /proc/meminfo MemTotal:8195224 kB Most systems of that generation can take 8GB RAM, and there isn't really any reason not to max that out, is there? > (And will they not increasingly die out, if nothing else as USB3 becomes > commonplace?) Can you wait 10 years for that to happen, or do you want a solution earlier? Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi,
On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann wrote:
> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
> driver and that may not be available when CONFIG_DMADEVICES
> is not set:
>
> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
> dependencies (DMADEVICES && ARCH_OMAP)
>
> This adds an extra dependency to avoid generating warnings in randconfig
> builds. Ideally we'd remove the 'select' statement, but that has the
> potential to break defconfig files.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode
> of MUSB is enabled")
>
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 1f2037bbeb0d..45c83baf675d 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>
> config USB_TI_CPPI41_DMA
> bool 'TI CPPI 4.1 (AM335x)'
> - depends on ARCH_OMAP
> + depends on ARCH_OMAP && DMADEVICES
> select TI_CPPI41
I am not sure what the generic policy is, but instead of hiding
USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
perspective.
Thanks,
-Bin.
>
> config USB_TUSB_OMAP_DMA
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] hid: usbhid: hid-core: fix recursive deadlock
The critical section protected by usbhid->lock in hid_ctrl() is too
big and in rare cases causes a recursive deadlock because of its call
to hid_input_report().
This deadlock reproduces on newer wacom tablets like 056a:033c because
the wacom driver in its irq handler ends up calling hid_hw_request()
from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means
is that it submits a report to reschedule a proximity read through a
sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb)
before calling hid_input_report(). When the irq kicks in on the same
cpu, it also tries to grab the lock resulting in a recursive deadlock.
The proper fix is to shrink the critical section in hid_ctrl() to
protect only the instructions which modify usbhid, thus move the lock
after the hid_input_report() call and the deadlock dissapears.
Signed-off-by: Ioan-Adrian Ratiu
---
drivers/hid/usbhid/hid-core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 36712e9..5dd426f 100644
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -477,8 +477,6 @@ static void hid_ctrl(struct urb *urb)
struct usbhid_device *usbhid = hid->driver_data;
int unplug = 0, status = urb->status;
- spin_lock(&usbhid->lock);
-
switch (status) {
case 0: /* success */
if (usbhid->ctrl[usbhid->ctrltail].dir == USB_DIR_IN)
@@ -498,6 +496,8 @@ static void hid_ctrl(struct urb *urb)
hid_warn(urb->dev, "ctrl urb status %d received\n", status);
}
+ spin_lock(&usbhid->lock);
+
if (unplug) {
usbhid->ctrltail = usbhid->ctrlhead;
} else {
--
2.6.3
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] usb: dwc3: add generic OF glue layer
For simple platforms which merely enable some clocks
and populate its children, we can use this generic
glue layer to avoid boilerplate code duplication.
For now this supports Qcom and Xilinx, but if we
find a way to add generic handling of regulators and
optional PHYs, we can absorb exynos as well.
Signed-off-by: Felipe Balbi
---
Can you guys check if this works for your respective platforms ? If it does we
can some code duplication going forward.
drivers/usb/dwc3/Kconfig | 9 ++
drivers/usb/dwc3/Makefile | 1 +
drivers/usb/dwc3/dwc3-of-simple.c | 174 ++
3 files changed, 184 insertions(+)
create mode 100644 drivers/usb/dwc3/dwc3-of-simple.c
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index 5a42c4590402..070e704829e5 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -87,6 +87,15 @@ config USB_DWC3_KEYSTONE
Support of USB2/3 functionality in TI Keystone2 platforms.
Say 'Y' or 'M' here if you have one such device
+config USB_DWC3_OF_SIMPLE
+ tristate "Generic OF Simple Glue Layer"
+ depends on OF && COMMON_CLK
+ default USB_DWC3
+ help
+ Support USB2/3 functionality in simple SoC integrations.
+Currently supports Xilinx and Qualcomm DWC USB3 IP.
+Say 'Y' or 'M' if you have one such device.
+
config USB_DWC3_ST
tristate "STMicroelectronics Platforms"
depends on ARCH_STI && OF
diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
index acc951d46c27..6491f9b474d4 100644
--- a/drivers/usb/dwc3/Makefile
+++ b/drivers/usb/dwc3/Makefile
@@ -37,5 +37,6 @@ obj-$(CONFIG_USB_DWC3_OMAP) += dwc3-omap.o
obj-$(CONFIG_USB_DWC3_EXYNOS) += dwc3-exynos.o
obj-$(CONFIG_USB_DWC3_PCI) += dwc3-pci.o
obj-$(CONFIG_USB_DWC3_KEYSTONE)+= dwc3-keystone.o
+obj-$(CONFIG_USB_DWC3_OF_SIMPLE) += dwc3-of-simple.o
obj-$(CONFIG_USB_DWC3_QCOM)+= dwc3-qcom.o
obj-$(CONFIG_USB_DWC3_ST) += dwc3-st.o
diff --git a/drivers/usb/dwc3/dwc3-of-simple.c
b/drivers/usb/dwc3/dwc3-of-simple.c
new file mode 100644
index ..00edd1195123
--- /dev/null
+++ b/drivers/usb/dwc3/dwc3-of-simple.c
@@ -0,0 +1,174 @@
+/**
+ * dwc3-of-simple.c - OF glue layer for simple integrations
+ *
+ * Copyright (c) 2015 Texas Instruments Incorporated - http://www.ti.com
+ *
+ * Author: Felipe Balbi
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 of
+ * the License as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+struct dwc3_of_simple {
+ struct device *dev;
+ struct clk **clks;
+ int num_clocks;
+};
+
+static int dwc3_of_simple_probe(struct platform_device *pdev)
+{
+ struct dwc3_of_simple *simple;
+ struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+
+ int ret;
+ int i;
+
+ simple = devm_kzalloc(dev, sizeof(*simple), GFP_KERNEL);
+ if (!simple)
+ return -ENOMEM;
+
+ ret = of_clk_get_parent_count(np);
+ if (ret < 0)
+ return ret;
+
+ simple->num_clocks = ret;
+
+ simple->clks = devm_kcalloc(dev, simple->num_clocks,
+ sizeof(struct clk *), GFP_KERNEL);
+ if (!simple->clks)
+ return -ENOMEM;
+
+ simple->dev = dev;
+
+ for (i = 0; i < simple->num_clocks; i++) {
+ struct clk *clk;
+
+ clk = of_clk_get(np, i);
+ if (IS_ERR(clk)) {
+ while (--i >= 0)
+ clk_put(simple->clks[i]);
+ return PTR_ERR(clk);
+ }
+
+ ret = clk_prepare_enable(clk);
+ if (ret < 0) {
+ while (--i >= 0) {
+ clk_disable_unprepare(simple->clks[i]);
+ clk_put(simple->clks[i]);
+ }
+ clk_put(clk);
+
+ return ret;
+ }
+
+ simple->clks[i] = clk;
+ }
+
+ ret = of_platform_populate(np, NULL, NULL, dev);
+ if (ret) {
+ for (i = 0; i < simple->num_clocks; i++) {
+ clk_disable_unprepare(simple->clks[i]);
+ clk_put(simple->clks[i]);
+ }
+
+ return ret;
+
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi,
Bin Liu writes:
> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann wrote:
>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>> driver and that may not be available when CONFIG_DMADEVICES
>> is not set:
>>
>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
>> dependencies (DMADEVICES && ARCH_OMAP)
>>
>> This adds an extra dependency to avoid generating warnings in randconfig
>> builds. Ideally we'd remove the 'select' statement, but that has the
>> potential to break defconfig files.
>>
>> Signed-off-by: Arnd Bergmann
>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode
>> of MUSB is enabled")
>>
>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>> index 1f2037bbeb0d..45c83baf675d 100644
>> --- a/drivers/usb/musb/Kconfig
>> +++ b/drivers/usb/musb/Kconfig
>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>
>> config USB_TI_CPPI41_DMA
>> bool 'TI CPPI 4.1 (AM335x)'
>> - depends on ARCH_OMAP
>> + depends on ARCH_OMAP && DMADEVICES
>> select TI_CPPI41
>
> I am not sure what the generic policy is, but instead of hiding
> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
> perspective.
that would mean "select DMADEVICES" and that's frowned upon.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
Here are some images with more information on this deadlock, might be helpful. First part of lockdep report: http://imgur.com/clLsCWe Second part: http://imgur.com/Wa2PzRl Here are some printk's of mine while reproducing + debugging the issue: http://imgur.com/SETOHT7 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] USB: option: add XS Stick W100-2 from 4G Systems
Thomas reports
"
4gsystems sells two total different LTE-surfsticks under the same name.
..
The newer version of XS Stick W100 is from "omega"
..
Under windows the driver switches to the same ID, and uses MI03\6 for
network and MI01\6 for modem.
..
echo "1c9e 9b01" > /sys/bus/usb/drivers/qmi_wwan/new_id
echo "1c9e 9b01" > /sys/bus/usb-serial/drivers/option1/new_id
T: Bus=01 Lev=01 Prnt=01 Port=03 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=1c9e ProdID=9b01 Rev=02.32
S: Manufacturer=USB Modem
S: Product=USB Modem
S: SerialNumber=
C: #Ifs= 5 Cfg#= 1 Atr=80 MxPwr=500mA
I: If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan
I: If#= 4 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=usb-storage
Now all important things are there:
wwp0s29f7u2i3 (net), ttyUSB2 (at), cdc-wdm0 (qmi), ttyUSB1 (at)
There is also ttyUSB0, but it is not usable, at least not for at.
The device works well with qmi and ModemManager-NetworkManager.
"
Reported-by: Thomas Schäfer
Cc:
Signed-off-by: Bjørn Mork
---
drivers/usb/serial/option.c | 9 +
1 file changed, 9 insertions(+)
diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 685fef71d3d1..232018b0b060 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -354,6 +354,7 @@ static void option_instat_callback(struct urb *urb);
/* This is the 4G XS Stick W14 a.k.a. Mobilcom Debitel Surf-Stick *
* It seems to contain a Qualcomm QSC6240/6290 chipset*/
#define FOUR_G_SYSTEMS_PRODUCT_W14 0x9603
+#define FOUR_G_SYSTEMS_PRODUCT_W1000x9b01
/* iBall 3.5G connect wireless modem */
#define IBALL_3_5G_CONNECT 0x9605
@@ -519,6 +520,11 @@ static const struct option_blacklist_info
four_g_w14_blacklist = {
.sendsetup = BIT(0) | BIT(1),
};
+static const struct option_blacklist_info four_g_w100_blacklist = {
+ .sendsetup = BIT(1) | BIT(2),
+ .reserved = BIT(3),
+};
+
static const struct option_blacklist_info alcatel_x200_blacklist = {
.sendsetup = BIT(0) | BIT(1),
.reserved = BIT(4),
@@ -1641,6 +1647,9 @@ static const struct usb_device_id option_ids[] = {
{ USB_DEVICE(LONGCHEER_VENDOR_ID, FOUR_G_SYSTEMS_PRODUCT_W14),
.driver_info = (kernel_ulong_t)&four_g_w14_blacklist
},
+ { USB_DEVICE(LONGCHEER_VENDOR_ID, FOUR_G_SYSTEMS_PRODUCT_W100),
+ .driver_info = (kernel_ulong_t)&four_g_w100_blacklist
+ },
{ USB_DEVICE_INTERFACE_CLASS(LONGCHEER_VENDOR_ID,
SPEEDUP_PRODUCT_SU9800, 0xff) },
{ USB_DEVICE(LONGCHEER_VENDOR_ID, ZOOM_PRODUCT_4597) },
{ USB_DEVICE(LONGCHEER_VENDOR_ID, IBALL_3_5G_CONNECT) },
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH net,stable] net: qmi_wwan: add XS Stick W100-2 from 4G Systems
Thomas reports
"
4gsystems sells two total different LTE-surfsticks under the same name.
..
The newer version of XS Stick W100 is from "omega"
..
Under windows the driver switches to the same ID, and uses MI03\6 for
network and MI01\6 for modem.
..
echo "1c9e 9b01" > /sys/bus/usb/drivers/qmi_wwan/new_id
echo "1c9e 9b01" > /sys/bus/usb-serial/drivers/option1/new_id
T: Bus=01 Lev=01 Prnt=01 Port=03 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=1c9e ProdID=9b01 Rev=02.32
S: Manufacturer=USB Modem
S: Product=USB Modem
S: SerialNumber=
C: #Ifs= 5 Cfg#= 1 Atr=80 MxPwr=500mA
I: If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 1 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 2 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I: If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=qmi_wwan
I: If#= 4 Alt= 0 #EPs= 2 Cls=08(stor.) Sub=06 Prot=50 Driver=usb-storage
Now all important things are there:
wwp0s29f7u2i3 (net), ttyUSB2 (at), cdc-wdm0 (qmi), ttyUSB1 (at)
There is also ttyUSB0, but it is not usable, at least not for at.
The device works well with qmi and ModemManager-NetworkManager.
"
Reported-by: Thomas Schäfer
Signed-off-by: Bjørn Mork
---
drivers/net/usb/qmi_wwan.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/usb/qmi_wwan.c b/drivers/net/usb/qmi_wwan.c
index 34799eaace41..9a5be8b85186 100644
--- a/drivers/net/usb/qmi_wwan.c
+++ b/drivers/net/usb/qmi_wwan.c
@@ -725,6 +725,7 @@ static const struct usb_device_id products[] = {
{QMI_FIXED_INTF(0x2357, 0x9000, 4)},/* TP-LINK MA260 */
{QMI_FIXED_INTF(0x1bc7, 0x1200, 5)},/* Telit LE920 */
{QMI_FIXED_INTF(0x1bc7, 0x1201, 2)},/* Telit LE920 */
+ {QMI_FIXED_INTF(0x1c9e, 0x9b01, 3)},/* XS Stick W100-2 from 4G
Systems */
{QMI_FIXED_INTF(0x0b3c, 0xc000, 4)},/* Olivetti Olicard 100 */
{QMI_FIXED_INTF(0x0b3c, 0xc001, 4)},/* Olivetti Olicard 120 */
{QMI_FIXED_INTF(0x0b3c, 0xc002, 4)},/* Olivetti Olicard 140 */
--
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi,
On Wed, Nov 18, 2015 at 1:27 PM, Felipe Balbi wrote:
>
> Hi,
>
> Bin Liu writes:
>> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann wrote:
>>> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
>>> driver and that may not be available when CONFIG_DMADEVICES
>>> is not set:
>>>
>>> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
>>> dependencies (DMADEVICES && ARCH_OMAP)
>>>
>>> This adds an extra dependency to avoid generating warnings in randconfig
>>> builds. Ideally we'd remove the 'select' statement, but that has the
>>> potential to break defconfig files.
>>>
>>> Signed-off-by: Arnd Bergmann
>>> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA mode
>>> of MUSB is enabled")
>>>
>>> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
>>> index 1f2037bbeb0d..45c83baf675d 100644
>>> --- a/drivers/usb/musb/Kconfig
>>> +++ b/drivers/usb/musb/Kconfig
>>> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>>>
>>> config USB_TI_CPPI41_DMA
>>> bool 'TI CPPI 4.1 (AM335x)'
>>> - depends on ARCH_OMAP
>>> + depends on ARCH_OMAP && DMADEVICES
>>> select TI_CPPI41
>>
>> I am not sure what the generic policy is, but instead of hiding
>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>> perspective.
>
> that would mean "select DMADEVICES" and that's frowned upon.
Currently 'select DMADEVICES' is not in there. Will adding it fix the
dependency warning in randconfig? Sorry for the question, but I don't
know enough about Kconfig to get the answer.
Regards,
-Bin.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
On Wednesday 18 November 2015 12:29:27 Bin Liu wrote: > > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig > > index 1f2037bbeb0d..45c83baf675d 100644 > > --- a/drivers/usb/musb/Kconfig > > +++ b/drivers/usb/musb/Kconfig > > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA > > > > config USB_TI_CPPI41_DMA > > bool 'TI CPPI 4.1 (AM335x)' > > - depends on ARCH_OMAP > > + depends on ARCH_OMAP && DMADEVICES > > select TI_CPPI41 > > I am not sure what the generic policy is, but instead of hiding > USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable > DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience > perspective. General policy is that you should not 'select' a symbol that is also user-visible, as that tends to cause dependency loops and other problems when something is enabled without the user being aware of that. Ideally we should remove the 'select TI_CPPI41' here as well, but what we could do instead is to make that a silent symbol and remove the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA and DMADEVICES are both enabled. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote: > The critical section protected by usbhid->lock in hid_ctrl() is too > big and in rare cases causes a recursive deadlock because of its call > to hid_input_report(). > > This deadlock reproduces on newer wacom tablets like 056a:033c because > the wacom driver in its irq handler ends up calling hid_hw_request() > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means > is that it submits a report to reschedule a proximity read through a > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb) > before calling hid_input_report(). When the irq kicks in on the same > cpu, it also tries to grab the lock resulting in a recursive deadlock. > > The proper fix is to shrink the critical section in hid_ctrl() to > protect only the instructions which modify usbhid, thus move the lock > after the hid_input_report() call and the deadlock dissapears. I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), isn't it? -- Jiri Kosina SUSE Labs -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi,
Bin Liu writes:
>> Bin Liu writes:
>>> On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann wrote:
The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
driver and that may not be available when CONFIG_DMADEVICES
is not set:
warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
dependencies (DMADEVICES && ARCH_OMAP)
This adds an extra dependency to avoid generating warnings in randconfig
builds. Ideally we'd remove the 'select' statement, but that has the
potential to break defconfig files.
Signed-off-by: Arnd Bergmann
Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA
mode of MUSB is enabled")
diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
index 1f2037bbeb0d..45c83baf675d 100644
--- a/drivers/usb/musb/Kconfig
+++ b/drivers/usb/musb/Kconfig
@@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
config USB_TI_CPPI41_DMA
bool 'TI CPPI 4.1 (AM335x)'
- depends on ARCH_OMAP
+ depends on ARCH_OMAP && DMADEVICES
select TI_CPPI41
>>>
>>> I am not sure what the generic policy is, but instead of hiding
>>> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
>>> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
>>> perspective.
>>
>> that would mean "select DMADEVICES" and that's frowned upon.
>
> Currently 'select DMADEVICES' is not in there. Will adding it fix the
> dependency warning in randconfig? Sorry for the question, but I don't
> know enough about Kconfig to get the answer.
it certainly would, but we don't like to add "select XYZ" to Kconfig
because a select bypasses the dependency tree. Let me explain:
config A
tristate "A"
depends on B
config B
tristate "B"
config C
tristate "C"
select A
C can select A without B being enabled.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi, On Wed, Nov 18, 2015 at 2:27 PM, Arnd Bergmann wrote: > On Wednesday 18 November 2015 12:29:27 Bin Liu wrote: >> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig >> > index 1f2037bbeb0d..45c83baf675d 100644 >> > --- a/drivers/usb/musb/Kconfig >> > +++ b/drivers/usb/musb/Kconfig >> > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA >> > >> > config USB_TI_CPPI41_DMA >> > bool 'TI CPPI 4.1 (AM335x)' >> > - depends on ARCH_OMAP >> > + depends on ARCH_OMAP && DMADEVICES >> > select TI_CPPI41 >> >> I am not sure what the generic policy is, but instead of hiding >> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable >> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience >> perspective. > > General policy is that you should not 'select' a symbol that is > also user-visible, as that tends to cause dependency loops and > other problems when something is enabled without the user being > aware of that. Understood. Thanks. I am okay with this patch. > > Ideally we should remove the 'select TI_CPPI41' here as well, but > what we could do instead is to make that a silent symbol and remove > the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA > and DMADEVICES are both enabled. But what if DMADEVICES was disabled and USB_TI_CPPI41_DMA was enabled? I would think I had CPPI fully enabled for MUSB, but it didn't because TI_CPPI41 was disabled. I would think this patch is the test option so far, we might have to document somewhere that to dmaengine has to be enabled to use MUSB CPPI, but I am not sure where the best place is to document... Regards, -Bin. > > Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
Hi, Arnd Bergmann writes: > On Wednesday 18 November 2015 12:29:27 Bin Liu wrote: >> > diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig >> > index 1f2037bbeb0d..45c83baf675d 100644 >> > --- a/drivers/usb/musb/Kconfig >> > +++ b/drivers/usb/musb/Kconfig >> > @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA >> > >> > config USB_TI_CPPI41_DMA >> > bool 'TI CPPI 4.1 (AM335x)' >> > - depends on ARCH_OMAP >> > + depends on ARCH_OMAP && DMADEVICES >> > select TI_CPPI41 >> >> I am not sure what the generic policy is, but instead of hiding >> USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable >> DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience >> perspective. > > General policy is that you should not 'select' a symbol that is > also user-visible, as that tends to cause dependency loops and > other problems when something is enabled without the user being > aware of that. > > Ideally we should remove the 'select TI_CPPI41' here as well, but > what we could do instead is to make that a silent symbol and remove > the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA > and DMADEVICES are both enabled. that should be perfect now that Tony L fixed this up so we can enable all MUSB DMA Engines in a single zImage. -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
On Wed, Nov 18, 2015 at 2:38 PM, Felipe Balbi wrote:
>
> Hi,
>
> Bin Liu writes:
>>> Bin Liu writes:
On Wed, Nov 18, 2015 at 10:18 AM, Arnd Bergmann wrote:
> The CPPI-4.1 driver selects TI_CPPI41, which is a dmaengine
> driver and that may not be available when CONFIG_DMADEVICES
> is not set:
>
> warning: (USB_TI_CPPI41_DMA) selects TI_CPPI41 which has unmet direct
> dependencies (DMADEVICES && ARCH_OMAP)
>
> This adds an extra dependency to avoid generating warnings in randconfig
> builds. Ideally we'd remove the 'select' statement, but that has the
> potential to break defconfig files.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 411dd19c682d ("usb: musb: Kconfig: Select the DMA driver if DMA
> mode of MUSB is enabled")
>
> diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
> index 1f2037bbeb0d..45c83baf675d 100644
> --- a/drivers/usb/musb/Kconfig
> +++ b/drivers/usb/musb/Kconfig
> @@ -159,7 +159,7 @@ config USB_TI_CPPI_DMA
>
> config USB_TI_CPPI41_DMA
> bool 'TI CPPI 4.1 (AM335x)'
> - depends on ARCH_OMAP
> + depends on ARCH_OMAP && DMADEVICES
> select TI_CPPI41
I am not sure what the generic policy is, but instead of hiding
USB_TI_CPPI41_DMA if DMADEVICES is disabled, I'd like to enable
DMADEVICES if USB_TI_CPPI41_DMA is enabled, from user experience
perspective.
>>>
>>> that would mean "select DMADEVICES" and that's frowned upon.
>>
>> Currently 'select DMADEVICES' is not in there. Will adding it fix the
>> dependency warning in randconfig? Sorry for the question, but I don't
>> know enough about Kconfig to get the answer.
>
> it certainly would, but we don't like to add "select XYZ" to Kconfig
> because a select bypasses the dependency tree. Let me explain:
>
> config A
>tristate "A"
>depends on B
>
> config B
>tristate "B"
>
>
> config C
>tristate "C"
>select A
>
>
> C can select A without B being enabled.
Thanks for the explanation, very clear.
>
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] staging/emxx_udc: fix 64-bit warnings
ARCH_SHMOBILE is coming to arm64, which creates new warnings in allmodconfig:
drivers/staging/emxx_udc/emxx_udc.c: In function '_nbu2ss_out_dma':
drivers/staging/emxx_udc/emxx_udc.c:843:45: warning: cast from pointer to
integer of different size [-Wpointer-to-int-cast]
_nbu2ss_writel(&preg->EP_DCR[num].EP_TADR, (u32)pBuffer);
This is clearly a mistake from confusing a dma_addr_t with a pointer,
so the fix is to use the correct types in two places.
The third warning of this kind is a check for an unaligned pointer,
which should be done by casting the pointer to uintptr_t, not int.
Signed-off-by: Arnd Bergmann
diff --git a/drivers/staging/emxx_udc/emxx_udc.c
b/drivers/staging/emxx_udc/emxx_udc.c
index 4e6c16af40fc..c168845cbb91 100644
--- a/drivers/staging/emxx_udc/emxx_udc.c
+++ b/drivers/staging/emxx_udc/emxx_udc.c
@@ -823,7 +823,7 @@ static int _nbu2ss_out_dma(
u32 length
)
{
- u8 *pBuffer;
+ dma_addr_t pBuffer;
u32 mpkt;
u32 lmpkt;
u32 dmacnt;
@@ -836,7 +836,7 @@ static int _nbu2ss_out_dma(
return 1; /* DMA is forwarded */
req->dma_flag = TRUE;
- pBuffer = (u8 *)req->req.dma;
+ pBuffer = req->req.dma;
pBuffer += req->req.actual;
/* DMA Address */
@@ -1034,7 +1034,7 @@ static int _nbu2ss_in_dma(
u32 length
)
{
- u8 *pBuffer;
+ dma_addr_t pBuffer;
u32 mpkt; /* MaxPacketSize */
u32 lmpkt; /* Last Packet Data Size */
u32 dmacnt; /* IN Data Size */
@@ -1080,7 +1080,7 @@ static int _nbu2ss_in_dma(
_nbu2ss_writel(&preg->EP_DCR[num].EP_DCR2, data);
/* Address setting */
- pBuffer = (u8 *)req->req.dma;
+ pBuffer = req->req.dma;
pBuffer += req->req.actual;
_nbu2ss_writel(&preg->EP_DCR[num].EP_TADR, (u32)pBuffer);
@@ -2728,7 +2728,7 @@ static int nbu2ss_ep_queue(
spin_lock_irqsave(&udc->lock, flags);
#ifdef USE_DMA
- if ((u32)req->req.buf & 0x3)
+ if ((uintptr_t)req->req.buf & 0x3)
req->unaligned = TRUE;
else
req->unaligned = FALSE;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Wed, 18 Nov 2015 21:37:42 +0100 (CET) Jiri Kosina wrote: > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote: > > > The critical section protected by usbhid->lock in hid_ctrl() is too > > big and in rare cases causes a recursive deadlock because of its call > > to hid_input_report(). > > > > This deadlock reproduces on newer wacom tablets like 056a:033c because > > the wacom driver in its irq handler ends up calling hid_hw_request() > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means > > is that it submits a report to reschedule a proximity read through a > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb) > > before calling hid_input_report(). When the irq kicks in on the same > > cpu, it also tries to grab the lock resulting in a recursive deadlock. > > > > The proper fix is to shrink the critical section in hid_ctrl() to > > protect only the instructions which modify usbhid, thus move the lock > > after the hid_input_report() call and the deadlock dissapears. > > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), > isn't it? > That was my first attempt, yes, but the deadlock still happens with interrupts disabled. It is very weird, I know. I tried many configurations, like disabling PREEMPT_RT and other stuff which might affect the call stack in this case, but the only two methods which actually avoid the deadlock are: 1. don't call wacom_intuos_schedule_prox_event() / hid_hw_request() from the wacom driver 2. shrink the critical region to not cover hid_input_report() inside hid_ctrl() I am very open to any ideas on how to better fix this, just to be able to use a mainline kernel with my device without out of tree patching :) -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: musb: USB_TI_CPPI41_DMA requires dmaengine support
On Wednesday 18 November 2015 14:39:10 Bin Liu wrote: > > Ideally we should remove the 'select TI_CPPI41' here as well, but > > what we could do instead is to make that a silent symbol and remove > > the prompt so it always gets enabled implicitly when USB_TI_CPPI41_DMA > > and DMADEVICES are both enabled. > > But what if DMADEVICES was disabled and USB_TI_CPPI41_DMA was enabled? > I would think I had CPPI fully enabled for MUSB, but it didn't because > TI_CPPI41 was disabled. That would cause a runtime failure, just like any other configuration that does not enable all the hardware you want to use. > I would think this patch is the test option so far, we might have to > document somewhere that to dmaengine has to be enabled to use MUSB > CPPI, but I am not sure where the best place is to document... There are hundreds of device drivers that use dmaengines as a backend, we don't normally document this, just like we don't document the fact that you need to enable the right gpio, irqchip, timer, clock etc drivers for your platform. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi, Robert Jarzmik writes: > Hi Robert and Felipe, > > I experience a regression in the 4.4-rc1 kernel which appeared between v4.3 > and > v4.4-rc1. > The test : > - boot the mioa701 kernel (ie. pxa27x_udc driver) > - echo mem > /sys/power/state > - resume from "Suspend to RAM" with a key press > - try to use the udc (in my case, g_ether => ping mioa701) > > I bissected the issue to this : > commit b0bac2581c19 (refs/bisect/bad) > Author: Robert Baldyga > Date: Wed Sep 16 12:10:42 2015 +0200 > > usb: gadget: introduce 'enabled' flag in struct usb_ep > > Could either of you tell me what is happening and how this commit > could explain the suspend/resume breakage ? without any sort of logs it a bit difficult :-) Care to send some output of the failure ? Are there any oopses or what exactly happens ? -- balbi signature.asc Description: PGP signature
[4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi Robert and Felipe, I experience a regression in the 4.4-rc1 kernel which appeared between v4.3 and v4.4-rc1. The test : - boot the mioa701 kernel (ie. pxa27x_udc driver) - echo mem > /sys/power/state - resume from "Suspend to RAM" with a key press - try to use the udc (in my case, g_ether => ping mioa701) I bissected the issue to this : commit b0bac2581c19 (refs/bisect/bad) Author: Robert Baldyga Date: Wed Sep 16 12:10:42 2015 +0200 usb: gadget: introduce 'enabled' flag in struct usb_ep Could either of you tell me what is happening and how this commit could explain the suspend/resume breakage ? Cheers. -- Robert PS: Bisect log for reference git bisect start # good: [6a13feb9c82803e2b815eca72fa7a9f5561d7861] Linux 4.3 git bisect good 6a13feb9c82803e2b815eca72fa7a9f5561d7861 # bad: [8005c49d9aea74d382f474ce11afbbc7d7130bec] Linux 4.4-rc1 git bisect bad 8005c49d9aea74d382f474ce11afbbc7d7130bec # bad: [118c216e16c5ccb028cd03a0dcd56d17a07ff8d7] Merge tag 'staging-4.4-rc1' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging git bisect bad 118c216e16c5ccb028cd03a0dcd56d17a07ff8d7 # good: [e627078a0cbdc0c391efeb5a2c4eb287328fd633] Merge branch 'for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux git bisect good e627078a0cbdc0c391efeb5a2c4eb287328fd633 # good: [c17c6da659571a115c7b4983da6c6ac464317c34] staging: wilc1000: rename pfScanResult of struct scan_attr git bisect good c17c6da659571a115c7b4983da6c6ac464317c34 # good: [7bdb7d554e0e433b92b63f3472523cc3067f8ab4] Staging: rtl8192u: ieee80211: corrected indent git bisect good 7bdb7d554e0e433b92b63f3472523cc3067f8ab4 # good: [ac322de6bf5416cb145b58599297b8be73cd86ac] Merge tag 'md/4.4' of git://neil.brown.name/md git bisect good ac322de6bf5416cb145b58599297b8be73cd86ac # bad: [a4d8e93c3182a54d8d21a4d1cec6538ae1be9e16] Merge tag 'usb-for-v4.4' of git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb into usb-next git bisect bad a4d8e93c3182a54d8d21a4d1cec6538ae1be9e16 # bad: [5390d438e6f4aaf3555acc72aff155660a48cd28] usb: dwc2: gadget: kill ep0 requests before reinitializing core git bisect bad 5390d438e6f4aaf3555acc72aff155660a48cd28 # bad: [58efd4b06df4a421652cb2c8a850a9697a37915c] usb: phy: change some comments git bisect bad 58efd4b06df4a421652cb2c8a850a9697a37915c # good: [f871cb9b2e178667a351a6fae9d930826ec10e95] usb: gadget: fix few outdated comments git bisect good f871cb9b2e178667a351a6fae9d930826ec10e95 # bad: [101382ffb3838d68bf6d6d675c66a2f84ed3cb90] usb: gadget: f_phonet: eliminate abuse of ep->driver data git bisect bad 101382ffb3838d68bf6d6d675c66a2f84ed3cb90 # bad: [34422a5e5a884c8680ce9144cf270ae08b1472be] usb: gadget: f_eem: eliminate abuse of ep->driver data git bisect bad 34422a5e5a884c8680ce9144cf270ae08b1472be # bad: [b0bac2581c1918cc4ab0aca01977ad69f0bc127a] usb: gadget: introduce 'enabled' flag in struct usb_ep git bisect bad b0bac2581c1918cc4ab0aca01977ad69f0bc127a # good: [b67f628c84329a9ce82dbff5fde196dc4624e7c2] usb: gadget: epautoconf: add usb_ep_autoconfig_release() function git bisect good b67f628c84329a9ce82dbff5fde196dc4624e7c2 # first bad commit: [b0bac2581c1918cc4ab0aca01977ad69f0bc127a] usb: gadget: introduce 'enabled' flag in struct usb_ep -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Felipe Balbi writes: > Hi, > > without any sort of logs it a bit difficult :-) Care to send some output > of the failure ? Are there any oopses or what exactly happens ? Ah well, the UDC is the only way to "speak" to the board (no UART), so I don't have any written feedback available. All I have are the logs displayed on the phone's screen in the framebuffer screen. I know there is not panic/crash, as the screen show no stack/panic. I also know that disconnecting and reconnecting the USB cable triggers the logs of the phy vbus detection. >From what I see the kernel is perfectly resumed, ie. key inputs trigger changes on the framebuffer. I have the host side logs : 1035787.154247] usb 1-1: New USB device found, idVendor=049f, idProduct=505a [1035787.154254] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [1035787.154257] usb 1-1: Product: Ethernet Gadget [1035787.154259] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc [1035787.170423] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82 [1035822.149430] usb 1-1: USB disconnect, device number 69 [1035822.149564] cdc_subset 1-1:1.0 usb0: unregister 'cdc_subset' usb-:00:14.0-1, Linux Device --- here the board is gone to "Suspend to RAM" --- [1035829.041922] usb 1-1: new full-speed USB device number 70 using xhci_hcd [1035829.172194] usb 1-1: New USB device found, idVendor=049f, idProduct=505a [1035829.172197] usb 1-1: New USB device strings: Mfr=1, Product=2, SerialNumber=0 [1035829.172199] usb 1-1: Product: Ethernet Gadget [1035829.172200] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc [1035829.187640] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82 Here the board is back from suspend to RAM. It "looks" as if either it lost its IP setup, or something else ... the final effect "felt" is that "ping mioa701" doesn't work anymore. I can try that on another board (ie. with an UART, ie. a mainstone), but I don't have a working setup for suspend/resume, so it will probably take time. Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Robert Jarzmik writes:
> Felipe Balbi writes:
>
>> Hi,
>>
>> without any sort of logs it a bit difficult :-) Care to send some output
>> of the failure ? Are there any oopses or what exactly happens ?
> Ah well, the UDC is the only way to "speak" to the board (no UART), so I don't
> have any written feedback available. All I have are the logs displayed on the
> phone's screen in the framebuffer screen.
I can have logs if I kinda ... revert the patch, applying the diff in [1].
This enables the suspend/resume to work again, and I can gather the logs when
[1] is applied :
[ 63.649528] Suspending console(s) (use no_console_suspend to debug)
[ 63.688100] PM: suspend of devices complete after 37.275 msecs
[ 63.690351] PM: late suspend of devices complete after 2.191 msecs
[ 63.692595] PM: noirq suspend of devices complete after 2.196 msecs
[ 63.694387] PM: noirq resume of devices complete after 1.482 msecs
[ 63.696711] PM: early resume of devices complete after 1.533 msecs
[ 63.802930] PM: resume of devices complete after 106.122 msecs
[ 63.804976] Restarting tasks ...
[ 63.821371] pxa27x-udc pxa27x-udc: USB reset
[ 63.822988] done.
[ 63.933666] pxa27x-udc pxa27x-udc: USB reset
[ 64.064026] g_ether gadget: full-speed config #1: CDC Subset/SAFE
[ 64.069692] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk
already enabled, doing nothing
[ 64.075389] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk
already enabled, doing nothing
[ 64.082081] g_ether gadget: full-speed config #1: CDC Subset/SAFE
[ 64.087876] pxa27x-udc pxa27x-udc: ep4:pxa_ep_enable:usb_ep ep2in-bulk
already enabled, doing nothing
[ 64.093358] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk
already enabled, doing nothing
[ 64.099092] pxa27x-udc pxa27x-udc: ep4:pxa_ep_enable:usb_ep ep2in-bulk
already enabled, doing nothing
[ 64.104835] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk
already enabled, doing nothing
I don't know if that can help debug further ...
Cheers.
--
Robert
[1] Stupid diff to renable the suspend/resume to work on 4.4-rc1
rj@belgarion:~/mio_linux/kernel$ git diff
diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
index 3d583a10b926..2b8dcf546d03 100644
--- a/include/linux/usb/gadget.h
+++ b/include/linux/usb/gadget.h
@@ -267,8 +267,8 @@ static inline int usb_ep_enable(struct usb_ep *ep)
{
int ret;
- if (ep->enabled)
- return 0;
+ /* if (ep->enabled) */
+ /* return 0; */
ret = ep->ops->enable(ep, ep->desc);
if (ret)
@@ -295,8 +295,8 @@ static inline int usb_ep_disable(struct usb_ep *ep)
{
int ret;
- if (!ep->enabled)
- return 0;
+ /* if (!ep->enabled) */
+ /* return 0; */
ret = ep->ops->disable(ep);
if (ret)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi,
Robert Jarzmik writes:
> Felipe Balbi writes:
>
>> Hi,
>>
>> without any sort of logs it a bit difficult :-) Care to send some output
>> of the failure ? Are there any oopses or what exactly happens ?
>
> Ah well, the UDC is the only way to "speak" to the board (no UART), so
> I don't have any written feedback available. All I have are the logs
> displayed on the phone's screen in the framebuffer screen.
you can increase fbcon verbosity, right ? I guess that was
CONFIG_MESSAGE_LOGLEVEL_DEFAULT.
Set it to 9 and you should get everything. Another option is to change
dev_* to dev_crit() and KERN_* to KERN_CRIT.
> I know there is not panic/crash, as the screen show no stack/panic. I
> also know that disconnecting and reconnecting the USB cable triggers
> the logs of the phy vbus detection.
does it reenumerate fine when you unplug and replug ?
> From what I see the kernel is perfectly resumed, ie. key inputs
> trigger changes on the framebuffer.
>
> I have the host side logs :
> 1035787.154247] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
> [1035787.154254] usb 1-1: New USB device strings: Mfr=1, Product=2,
> SerialNumber=0
> [1035787.154257] usb 1-1: Product: Ethernet Gadget
> [1035787.154259] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
> [1035787.170423] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at
> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
> [1035822.149430] usb 1-1: USB disconnect, device number 69
> [1035822.149564] cdc_subset 1-1:1.0 usb0: unregister 'cdc_subset'
> usb-:00:14.0-1, Linux Device
is pxa27x always full-speed ? Seems like it, but I see that max_speed is
never set there. That's a bug, but a very minor one.
> --- here the board is gone to "Suspend to RAM" ---
>
> [1035829.041922] usb 1-1: new full-speed USB device number 70 using xhci_hcd
> [1035829.172194] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
> [1035829.172197] usb 1-1: New USB device strings: Mfr=1, Product=2,
> SerialNumber=0
> [1035829.172199] usb 1-1: Product: Ethernet Gadget
> [1035829.172200] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
> [1035829.187640] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at
> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
so it actually connects again just fine, this means suspend/resume works
as before.
> Here the board is back from suspend to RAM. It "looks" as if either it
> lost its IP setup, or something else ... the final effect "felt" is
But you still have your usb0 interface, right ? Does that have an IP
address when you look at ifconfig ?
> that "ping mioa701" doesn't work anymore.
that's kind of expected considering pxa27x disabled the UDC and
disconnects from the bus on suspend:
static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
{
struct pxa_udc *udc = platform_get_drvdata(_dev);
struct pxa_ep *ep;
ep = &udc->pxa_ep[0];
udc->udccsr0 = udc_ep_readl(ep, UDCCSR);
udc_disable(udc);
udc->pullup_resume = udc->pullup_on;
dplus_pullup(udc, 0);
return 0;
}
I'm actually surprised that it ever worked before :-)
> I can try that on another board (ie. with an UART, ie. a mainstone),
> but I don't have a working setup for suspend/resume, so it will
> probably take time.
yeah, let's try to avoid that if possible :-)
cheers
--
balbi
signature.asc
Description: PGP signature
Re: Change in kernel/mediatek[android-mtk-3.18]: usb: gadget: configfs: Fix interfaces array NULL-termination
"Badhri Jagan Sridharan (Gerrit)" Hi, writes: > Hello Felipe Balbi, > > I'd like you to do a code review. Please visit > > https://android-review.googlesource.com/183164 > > to review the following change. > > Change subject: usb: gadget: configfs: Fix interfaces array NULL-termination > .. > > usb: gadget: configfs: Fix interfaces array NULL-termination > > memset() to 0 interfaces array before reusing > usb_configuration structure. > > This commit fix bug: > > ln -s functions/acm.1 configs/c.1 > ln -s functions/acm.2 configs/c.1 > ln -s functions/acm.3 configs/c.1 > echo "UDC name" > UDC > echo "" > UDC > rm configs/c.1/acm.* > rmdir functions/* > mkdir functions/ecm.usb0 > ln -s functions/ecm.usb0 configs/c.1 > echo "UDC name" > UDC > > [ 82.220969] Unable to handle kernel NULL pointer dereference at virtual > address > [ 82.229009] pgd = c0004000 > [ 82.231698] [] *pgd= > [ 82.235260] Internal error: Oops: 17 [#1] PREEMPT SMP ARM > [ 82.240638] Modules linked in: > [ 82.243681] CPU: 0 PID: 0 Comm: swapper/0 Not tainted 4.0.0-rc2 #39 > [ 82.249926] Hardware name: SAMSUNG EXYNOS (Flattened Device Tree) > [ 82.256003] task: c07cd2f0 ti: c07c8000 task.ti: c07c8000 > [ 82.261393] PC is at composite_setup+0xe3c/0x1674 > [ 82.266073] LR is at composite_setup+0xf20/0x1674 > [ 82.270760] pc : []lr : []psr: 61d3 > [ 82.270760] sp : c07c9df0 ip : c0806448 fp : ed8c9c9c > [ 82.282216] r10: 0001 r9 : r8 : edaae918 > [ 82.287425] r7 : ed551cc0 r6 : 7fff r5 : r4 : ed799634 > [ 82.293934] r3 : 0003 r2 : 00010002 r1 : edaae918 r0 : 002e > [ 82.300446] Flags: nZCv IRQs off FIQs off Mode SVC_32 ISA ARM > Segment kernel > [ 82.307910] Control: 10c5387d Table: 6bc1804a DAC: 0015 > [ 82.313638] Process swapper/0 (pid: 0, stack limit = 0xc07c8210) > [ 82.319627] Stack: (0xc07c9df0 to 0xc07ca000) > [ 82.323969] 9de0: c06e65f4 > c07c9f68 > [ 82.332130] 9e00: 0067 c07c59ac 03f7 edaae918 ed8c9c98 ed799690 > eca2f140 21d3 > [ 82.340289] 9e20: ee79a2d8 c07c9e88 c07c5304 55db 00010002 edaae810 > edaae860 eda96d50 > [ 82.348448] 9e40: 0009 ee264510 0007 c07ca444 edaae860 c0340890 > c0827a40 55e0 > [ 82.356607] 9e60: c0827a40 eda96e40 ee264510 edaae810 edaae860 > 0007 c07ca444 > [ 82.364766] 9e80: edaae860 c0354170 c03407dc c033db4c edaae810 > 0010 > [ 82.372925] 9ea0: 0032 c0341670 0001 eda96e00 > > [ 82.381084] 9ec0: 0032 c0803a23 ee1aa840 0001 c005d54c > 249e2450 > [ 82.389244] 9ee0: 21d3 ee1aa840 ee1aa8a0 ed84f4c0 c07c9f68 > 0067 c07c59ac > [ 82.397403] 9f00: c005d688 ee1aa840 ee1aa8a0 c07db4b4 c006009c > 0032 > [ 82.405562] 9f20: 0001 c005ce20 c07c59ac c005cf34 f002000c c07ca780 > c07c9f68 0057 > [ 82.413722] 9f40: f002 413fc090 0001 c00086b4 c000f804 6053 > c07c9f9c > [ 82.421880] 9f60: c0803a20 c0011fc0 c07c9fb8 c001bee0 > c07ca4f0 c057004c > [ 82.430040] 9f80: c07ca4fc c0803a20 c0803a20 413fc090 0001 > 0100 c07c9fb0 > [ 82.438199] 9fa0: c000f800 c000f804 6053 c0050e70 > c0803bc0 c0783bd8 > [ 82.446358] 9fc0: c0783664 c07b13e8 > c0803e54 > [ 82.454517] 9fe0: c07ca480 c07b13e4 c07ce40c 4000406a 40008074 > > [ 82.462689] [] (composite_setup) from [] > (s3c_hsotg_complete_setup+0xb4/0x418) > [ 82.471626] [] (s3c_hsotg_complete_setup) from [] > (usb_gadget_giveback_request+0xc/0x10) > [ 82.481429] [] (usb_gadget_giveback_request) from [] > (s3c_hsotg_complete_request+0xcc/0x12c) > [ 82.491583] [] (s3c_hsotg_complete_request) from [] > (s3c_hsotg_irq+0x4fc/0x558) > [ 82.500614] [] (s3c_hsotg_irq) from [] > (handle_irq_event_percpu+0x50/0x150) > [ 82.509291] [] (handle_irq_event_percpu) from [] > (handle_irq_event+0x3c/0x5c) > [ 82.518145] [] (handle_irq_event) from [] > (handle_fasteoi_irq+0xd4/0x18c) > [ 82.526650] [] (handle_fasteoi_irq) from [] > (generic_handle_irq+0x20/0x30) > [ 82.535242] [] (generic_handle_irq) from [] > (__handle_domain_irq+0x6c/0xdc) > [ 82.543923] [] (__handle_domain_irq) from [] > (gic_handle_irq+0x2c/0x6c) > [ 82.552256] [] (gic_handle_irq) from [] > (__irq_svc+0x40/0x74) > [ 82.559716] Exception stack(0xc07c9f68 to 0xc07c9fb0) > [ 82.564753] 9f60: c07c9fb8 c001bee0 > c07ca4f0 c057004c > [ 82.572913] 9f80: c07ca4fc c0803a20 c0803a20 413fc090 0001 > 0100 c07c9fb0 > [ 82.581069] 9fa0: c000f800 c000f804 6053 > [ 82.586113] [] (__irq_s
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Felipe Balbi writes:
> Hi,
> you can increase fbcon verbosity, right ? I guess that was
> CONFIG_MESSAGE_LOGLEVEL_DEFAULT.
Sure.
> Set it to 9 and you should get everything. Another option is to change
> dev_* to dev_crit() and KERN_* to KERN_CRIT.
>
>> I know there is not panic/crash, as the screen show no stack/panic. I
>> also know that disconnecting and reconnecting the USB cable triggers
>> the logs of the phy vbus detection.
>
> does it reenumerate fine when you unplug and replug ?
Yeah, I think the log below on host side show this.
>> From what I see the kernel is perfectly resumed, ie. key inputs
>> trigger changes on the framebuffer.
>>
>> I have the host side logs :
>> 1035787.154247] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
>> [1035787.154254] usb 1-1: New USB device strings: Mfr=1, Product=2,
>> SerialNumber=0
>> [1035787.154257] usb 1-1: Product: Ethernet Gadget
>> [1035787.154259] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
>> [1035787.170423] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at
>> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
>> [1035822.149430] usb 1-1: USB disconnect, device number 69
>> [1035822.149564] cdc_subset 1-1:1.0 usb0: unregister 'cdc_subset'
>> usb-:00:14.0-1, Linux Device
>
> is pxa27x always full-speed ? Seems like it, but I see that max_speed is
> never set there. That's a bug, but a very minor one.
Yes, always full-speed, right. As for max_speed, I will look into it.
>> --- here the board is gone to "Suspend to RAM" ---
>>
>> [1035829.041922] usb 1-1: new full-speed USB device number 70 using xhci_hcd
>> [1035829.172194] usb 1-1: New USB device found, idVendor=049f, idProduct=505a
>> [1035829.172197] usb 1-1: New USB device strings: Mfr=1, Product=2,
>> SerialNumber=0
>> [1035829.172199] usb 1-1: Product: Ethernet Gadget
>> [1035829.172200] usb 1-1: Manufacturer: Linux 4.4.0-rc1 with pxa27x_udc
>> [1035829.187640] cdc_subset 1-1:1.0 usb0: register 'cdc_subset' at
>> usb-:00:14.0-1, Linux Device, 9a:7b:ac:1c:65:82
>
> so it actually connects again just fine, this means suspend/resume works
> as before.
>From host side, yes. From device side, it's a bit different, as you can see in
the other mail I sent : all the log containing "already enabled, doing nothing"
are in v4.3 (ie. in working case), and absent in v4.4-rc1 (ie. broken case).
>> Here the board is back from suspend to RAM. It "looks" as if either it
>> lost its IP setup, or something else ... the final effect "felt" is
>
> But you still have your usb0 interface, right ? Does that have an IP
> address when you look at ifconfig ?
Yes, just as before, on host side I see no difference. The interface reappears
and is IP configured automaticaly (my host PC setup takes care of that).
>> that "ping mioa701" doesn't work anymore.
>
> that's kind of expected considering pxa27x disabled the UDC and
> disconnects from the bus on suspend:
Euh it's expected to work _after resume_.
>
> static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
> {
> struct pxa_udc *udc = platform_get_drvdata(_dev);
> struct pxa_ep *ep;
>
> ep = &udc->pxa_ep[0];
> udc->udccsr0 = udc_ep_readl(ep, UDCCSR);
>
> udc_disable(udc);
> udc->pullup_resume = udc->pullup_on;
> dplus_pullup(udc, 0);
>
> return 0;
> }
>
> I'm actually surprised that it ever worked before :-)
I don't follow you : upon resume, ie. after pxa_udc_resume(), it worked
before. The case that bothers me is after resume, not between suspend() and
resume().
>> I can try that on another board (ie. with an UART, ie. a mainstone),
>> but I don't have a working setup for suspend/resume, so it will
>> probably take time.
> yeah, let's try to avoid that if possible :-)
Agreed.
Cheers.
--
Robert
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi,
Robert Jarzmik writes:
> Robert Jarzmik writes:
>
>> Felipe Balbi writes:
>>
>>> Hi,
>>>
>>> without any sort of logs it a bit difficult :-) Care to send some output
>>> of the failure ? Are there any oopses or what exactly happens ?
>> Ah well, the UDC is the only way to "speak" to the board (no UART), so I
>> don't
>> have any written feedback available. All I have are the logs displayed on the
>> phone's screen in the framebuffer screen.
>
> I can have logs if I kinda ... revert the patch, applying the diff in [1].
> This enables the suspend/resume to work again, and I can gather the logs when
> [1] is applied :
>
> [ 63.649528] Suspending console(s) (use no_console_suspend to debug)
> [ 63.688100] PM: suspend of devices complete after 37.275 msecs
> [ 63.690351] PM: late suspend of devices complete after 2.191 msecs
> [ 63.692595] PM: noirq suspend of devices complete after 2.196 msecs
> [ 63.694387] PM: noirq resume of devices complete after 1.482 msecs
> [ 63.696711] PM: early resume of devices complete after 1.533 msecs
> [ 63.802930] PM: resume of devices complete after 106.122 msecs
> [ 63.804976] Restarting tasks ...
> [ 63.821371] pxa27x-udc pxa27x-udc: USB reset
> [ 63.822988] done.
> [ 63.933666] pxa27x-udc pxa27x-udc: USB reset
> [ 64.064026] g_ether gadget: full-speed config #1: CDC Subset/SAFE
> [ 64.069692] pxa27x-udc pxa27x-udc: ep3:pxa_ep_enable:usb_ep ep1out-bulk
> already enabled, doing nothing
this could be a bug in either g_ether or pxa27x... Seems like something
is enabling endpoints which were already enabled. Not sure if this is
pxa27x not _really_ disabling endpoints or g_ether being stupid.
> [1] Stupid diff to renable the suspend/resume to work on 4.4-rc1
> rj@belgarion:~/mio_linux/kernel$ git diff
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 3d583a10b926..2b8dcf546d03 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -267,8 +267,8 @@ static inline int usb_ep_enable(struct usb_ep *ep)
> {
> int ret;
>
> - if (ep->enabled)
> - return 0;
> + /* if (ep->enabled) */
> + /* return 0; */
>
> ret = ep->ops->enable(ep, ep->desc);
> if (ret)
> @@ -295,8 +295,8 @@ static inline int usb_ep_disable(struct usb_ep *ep)
> {
> int ret;
>
> - if (!ep->enabled)
> - return 0;
> + /* if (!ep->enabled) */
> + /* return 0; */
yeah, so something is not disabling endpoints when they should :-) So
this could be a bug with your suspend/resume callbacks. If you're going
to disconnect from the bus, you need to tell the gadget driver about it,
which means after disabling pullups, you should call
gadget_driver->disconnect().
Can you see if this *stupid* and *untested* diff helps :
diff --git a/drivers/usb/gadget/udc/pxa27x_udc.c
b/drivers/usb/gadget/udc/pxa27x_udc.c
index 670ac0b12f00..a08ae19ca410 100644
--- a/drivers/usb/gadget/udc/pxa27x_udc.c
+++ b/drivers/usb/gadget/udc/pxa27x_udc.c
@@ -2535,6 +2535,7 @@ static int pxa_udc_suspend(struct platform_device *_dev,
pm_message_t state)
udc_disable(udc);
udc->pullup_resume = udc->pullup_on;
dplus_pullup(udc, 0);
+ udc->driver->disconnect(&udc->gadget);
return 0;
}
I'm not 100% sure this is enough, as I'm not at all familiar with
pxa27x, but that driver looks hugely unmaintained. Which device are you
using for development/test ? Is it this Mio A701 ? I can't find any
sources of it :-s
--
balbi
signature.asc
Description: PGP signature
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi,
Robert Jarzmik writes:
>>> that "ping mioa701" doesn't work anymore.
>>
>> that's kind of expected considering pxa27x disabled the UDC and
>> disconnects from the bus on suspend:
>
> Euh it's expected to work _after resume_.
sure is, but you can't expect it to have a proper IP. From host's
perspective this is a brand new usb0 network interface. Unless you have
dhcp client running on the phone and a server on your PC, you can't
assume it'll have IP ;-) (or statically configured, yada yada yada ;-)
>> static int pxa_udc_suspend(struct platform_device *_dev, pm_message_t state)
>> {
>> struct pxa_udc *udc = platform_get_drvdata(_dev);
>> struct pxa_ep *ep;
>>
>> ep = &udc->pxa_ep[0];
>> udc->udccsr0 = udc_ep_readl(ep, UDCCSR);
>>
>> udc_disable(udc);
>> udc->pullup_resume = udc->pullup_on;
>> dplus_pullup(udc, 0);
>>
>> return 0;
>> }
>>
>> I'm actually surprised that it ever worked before :-)
>
> I don't follow you : upon resume, ie. after pxa_udc_resume(), it
> worked before. The case that bothers me is after resume, not between
> suspend() and resume().
I got that :-) My surprise is how you managed to get IP
before. Statically configured ?
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/2] usb: doc: dwc3-xilinx: Add devicetree bindings
On Wed, Nov 18, 2015 at 06:20:31PM +0530, Subbaraya Sundeep Bhatta wrote:
> This patch adds binding doc for Xilinx DWC3 glue driver.
>
> Signed-off-by: Subbaraya Sundeep Bhatta
I really dislike how the DWC3 binding has been done. The sub-node here
is pointless. The only thing the outer node does is add clocks which
should simply be part of the DWC3 node. Presumably the IP block needs
some clocks...
Anyway, it's self-contained within the DWC3, so I won't make you clean
up the mess.
Acked-by: Rob Herring
> ---
> .../devicetree/bindings/usb/dwc3-xilinx.txt| 33
> ++
> 1 file changed, 33 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> new file mode 100644
> index 000..30361b3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/dwc3-xilinx.txt
> @@ -0,0 +1,33 @@
> +Xilinx SuperSpeed DWC3 USB SoC controller
> +
> +Required properties:
> +- compatible:Should contain "xlnx,zynqmp-dwc3"
> +- clocks:A list of phandles for the clocks listed in clock-names
> +- clock-names: Should contain the following:
> + "bus_clk" Master/Core clock, have to be >= 125 MHz for SS
> + operation and >= 60MHz for HS operation
> +
> + "ref_clk" Clock source to core during PHY power down
> +
> +Required child node:
> +A child node must exist to represent the core DWC3 IP block. The name of
> +the node is not important. The content of the node is defined in dwc3.txt.
> +
> +Example device node:
> +
> + usb@0 {
> + #address-cells = <0x2>;
> + #size-cells = <0x1>;
> + status = "okay";
> + compatible = "xlnx,zynqmp-dwc3";
> + clock-names = "bus_clk" "ref_clk";
> + clocks = <&clk125>, <&clk125>;
> + ranges;
> +
> + dwc3@fe20 {
> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe20 0x4>;
> + interrupts = <0x0 0x41 0x4>;
> + dr_mode = "host";
> + };
> + };
> --
> 2.1.2
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Infrastructure for zerocopy I/O
By the way QNAP NAS systems are shipped with a 64bit kernel but a 32bit system environment. Those systems support USB 2.0 and USB 3.0. You can expect any kind of combination out there. On Wed, Nov 18, 2015 at 7:23 PM, Bjørn Mork wrote: > "Steinar H. Gunderson" writes: >> On Tue, Nov 17, 2015 at 03:16:55PM -0500, Alan Stern wrote: >>> xHCI always uses 64-bit addresses. But many EHCI controllers don't, >>> and only a few of the EHCI platform drivers support 64-bit DMA. >> >> OK, sure. But are systems with USB2 only and more than 4GB of RAM common? > > Hmpff. They are common in my house at least :) > > bjorn@nemi:~$ lspci -nn|grep USB > 00:1a.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #4 [8086:2937] (rev 03) > 00:1a.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #5 [8086:2938] (rev 03) > 00:1a.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #6 [8086:2939] (rev 03) > 00:1a.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 > EHCI Controller #2 [8086:293c] (rev 03) > 00:1d.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #1 [8086:2934] (rev 03) > 00:1d.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #2 [8086:2935] (rev 03) > 00:1d.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #3 [8086:2936] (rev 03) > 00:1d.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 > EHCI Controller #1 [8086:293a] (rev 03) > bjorn@nemi:~$ grep MemTotal /proc/meminfo > MemTotal:8051536 kB > > bjorn@canardo:~$ lspci -nn|grep USB > 00:1a.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #4 [8086:2937] (rev 02) > 00:1a.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #5 [8086:2938] (rev 02) > 00:1a.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #6 [8086:2939] (rev 02) > 00:1a.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 > EHCI Controller #2 [8086:293c] (rev 02) > 00:1d.0 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #1 [8086:2934] (rev 02) > 00:1d.1 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #2 [8086:2935] (rev 02) > 00:1d.2 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB > UHCI Controller #3 [8086:2936] (rev 02) > 00:1d.7 USB controller [0c03]: Intel Corporation 82801I (ICH9 Family) USB2 > EHCI Controller #1 [8086:293a] (rev 02) > bjorn@canardo:~$ grep MemTotal /proc/meminfo > MemTotal:8195224 kB > > Most systems of that generation can take 8GB RAM, and there isn't really > any reason not to max that out, is there? > >> (And will they not increasingly die out, if nothing else as USB3 becomes >> commonplace?) > > Can you wait 10 years for that to happen, or do you want a solution > earlier? > > > Bjørn -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Felipe Balbi writes: > Hi, > > this could be a bug in either g_ether or pxa27x... Seems like something > is enabling endpoints which were already enabled. Not sure if this is > pxa27x not _really_ disabling endpoints or g_ether being stupid. You're probably right. >From what I remember, g_ether is enabling already enabled endpoints upon >resume, but that memory is pretty thin and goes back to 2008 when I created the driver, so I'm not sure anymore. > yeah, so something is not disabling endpoints when they should :-) So > this could be a bug with your suspend/resume callbacks. If you're going > to disconnect from the bus, you need to tell the gadget driver about it, > which means after disabling pullups, you should call > gadget_driver->disconnect(). Okay. > Can you see if this *stupid* and *untested* diff helps : ...zip... Yes it does. I don't understand why, but it does fix it, and the logs "already enabled" are still gone. I wasn't aware that a disconnect was required in the suspend, nor did I see it in another udc driver (in 2008 of course). > I'm not 100% sure this is enough, as I'm not at all familiar with > pxa27x, but that driver looks hugely unmaintained. That would be my fault, as I'm maintaining it. That's the reason I'm seeing the regression. If you have on mind improvements required just say so, I'll see what I can do. > Which device are you using for development/test ? Is it this Mio A701 ? Yes it is. > I can't find any sources of it :-s What about arch/arm/mach-pxa/mioa701.c ? Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: doc: dwc3-xilinx: Add devicetree bindings
Hi, Rob Herring writes: > On Wed, Nov 18, 2015 at 06:20:31PM +0530, Subbaraya Sundeep Bhatta wrote: >> This patch adds binding doc for Xilinx DWC3 glue driver. >> >> Signed-off-by: Subbaraya Sundeep Bhatta > > I really dislike how the DWC3 binding has been done. The sub-node here > is pointless. The only thing the outer node does is add clocks which > should simply be part of the DWC3 node. Presumably the IP block needs > some clocks... > > Anyway, it's self-contained within the DWC3, so I won't make you clean > up the mess. heh, it's definitely not pointless. We get a lot of free goodies by doing it that way. I'm just not going to repeat it once again. But in summary: a) we force people to write glue layers for *only* their HW specific details b) there's really no way to abuse dwc3 core because there's no symbol exported from dwc3 core to glue c) PM (both system sleep and runtime) becomes a lot simpler with core only caring about what PM details delivered by SNPS (e.g. Hibernation mode from DWC3) and glue only has to consider the SoC integration parts of PM (for OMAP that would be PRCM details, hwmod, etc). I'm definitely not going to change dwc3 because it has made my life a lot saner. Definitely a lot saner than MUSB. Besides, DTS is supposed to describe the HW and that's, really, how the HW is. There's a piece of HW which is somewhat platform agnostic and delivered by SNPS as a black box (SNPS customers don't touch SNPS RTL) and there's another piece of HW which bridges this dwc3 to the platform specific details and integration context of the platform (for OMAP that would the SYSCONFIG/SYSSTATUS registers, Clock autogating, PRCM, etc, etc etc). So you _do_ in fact, have two separate pieces of HW which are SW visible and controllable independently. They each have their own IRQs (though in some SoCs they share the same line), they're own address space, yada yada yada. cheers -- balbi signature.asc Description: PGP signature
Re: [PATCH] usb: dwc2: add support of hi6220
On Wed, Nov 18, 2015 at 03:39:47PM +0800, Zhangfei Gao wrote:
> Support hisilicon,hi6220-usb for HiKey board
>
> Signed-off-by: Zhangfei Gao
> ---
> Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
For the binding:
Acked-by: Rob Herring
> drivers/usb/dwc2/platform.c| 32
> ++
> 2 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index fd132cb..2213682 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -4,6 +4,7 @@ Platform DesignWare HS OTG USB 2.0 controller
> Required properties:
> - compatible : One of:
>- brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
> + - hisilicon,hi6220-usb: The DWC2 USB controller instance in the hi6220 SoC.
>- rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
>- "rockchip,rk3188-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3188
> Soc;
>- "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288
> Soc;
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5859b0f..a5cb1bf 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -54,6 +54,37 @@
>
> static const char dwc2_driver_name[] = "dwc2";
>
> +static const struct dwc2_core_params params_hi6220 = {
> + .otg_cap= 2,/* No HNP/SRP capable */
> + .otg_ver= 0,/* 1.3 */
> + .dma_enable = 1,
> + .dma_desc_enable= 0,
> + .speed = 0,/* High Speed */
> + .enable_dynamic_fifo= 1,
> + .en_multiple_tx_fifo= 1,
> + .host_rx_fifo_size = 512,
> + .host_nperio_tx_fifo_size = 512,
> + .host_perio_tx_fifo_size= 512,
> + .max_transfer_size = 65535,
> + .max_packet_count = 511,
> + .host_channels = 16,
> + .phy_type = 1,/* UTMI */
> + .phy_utmi_width = 8,
> + .phy_ulpi_ddr = 0,/* Single */
> + .phy_ulpi_ext_vbus = 0,
> + .i2c_enable = 0,
> + .ulpi_fs_ls = 0,
> + .host_support_fs_ls_low_power = 0,
> + .host_ls_low_power_phy_clk = 0,/* 48 MHz */
> + .ts_dline = 0,
> + .reload_ctl = 0,
> + .ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> + GAHBCFG_HBSTLEN_SHIFT,
> + .uframe_sched = 0,
> + .external_id_pin_ctl= -1,
> + .hibernation= -1,
> +};
> +
> static const struct dwc2_core_params params_bcm2835 = {
> .otg_cap= 0,/* HNP/SRP capable */
> .otg_ver= 0,/* 1.3 */
> @@ -282,6 +313,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
>
> static const struct of_device_id dwc2_of_match_table[] = {
> { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 },
> + { .compatible = "hisilicon,hi6220-usb", .data = ¶ms_hi6220 },
> { .compatible = "rockchip,rk3066-usb", .data = ¶ms_rk3066 },
> { .compatible = "snps,dwc2", .data = NULL },
> { .compatible = "samsung,s3c6400-hsotg", .data = NULL},
> --
> 1.9.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] phy: add phy-hi6220-usb
On Wed, Nov 18, 2015 at 03:35:23PM +0800, Zhangfei Gao wrote:
> Support hi6220 use phy for HiKey board
>
> Signed-off-by: Zhangfei Gao
> ---
> .../devicetree/bindings/phy/phy-hi6220-usb.txt | 16 ++
For the binding:
Acked-by: Rob Herring
> drivers/phy/Kconfig| 9 ++
> drivers/phy/Makefile | 1 +
> drivers/phy/phy-hi6220-usb.c | 168
> +
> 4 files changed, 194 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/phy-hi6220-usb.txt
> create mode 100644 drivers/phy/phy-hi6220-usb.c
>
> diff --git a/Documentation/devicetree/bindings/phy/phy-hi6220-usb.txt
> b/Documentation/devicetree/bindings/phy/phy-hi6220-usb.txt
> new file mode 100644
> index 000..f17a56e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/phy-hi6220-usb.txt
> @@ -0,0 +1,16 @@
> +Hisilicon hi6220 usb PHY
> +---
> +
> +Required properties:
> +- compatible: should be "hisilicon,hi6220-usb-phy"
> +- #phy-cells: must be 0
> +- hisilicon,peripheral-syscon: phandle of syscon used to control phy.
> +Refer to phy/phy-bindings.txt for the generic PHY binding properties
> +
> +Example:
> + usb_phy: usbphy {
> + compatible = "hisilicon,hi6220-usb-phy";
> + #phy-cells = <0>;
> + phy-supply = <&fixed_5v_hub>;
> + hisilicon,peripheral-syscon = <&sys_ctrl>;
> + };
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index 47da573..c91a612 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -206,6 +206,15 @@ config PHY_HIX5HD2_SATA
> help
> Support for SATA PHY on Hisilicon hix5hd2 Soc.
>
> +config PHY_HI6220_USB
> + tristate "hi6220 USB PHY support"
> + select GENERIC_PHY
> + select MFD_SYSCON
> + help
> + Enable this to support the HISILICON HI6220 USB PHY.
> +
> + To compile this driver as a module, choose M here.
> +
> config PHY_SUN4I_USB
> tristate "Allwinner sunxi SoC USB PHY driver"
> depends on ARCH_SUNXI && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index a5b18c1..0c5ccc9 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_TI_PIPE3) +=
> phy-ti-pipe3.o
> obj-$(CONFIG_TWL4030_USB)+= phy-twl4030-usb.o
> obj-$(CONFIG_PHY_EXYNOS5250_SATA)+= phy-exynos5250-sata.o
> obj-$(CONFIG_PHY_HIX5HD2_SATA) += phy-hix5hd2-sata.o
> +obj-$(CONFIG_PHY_HI6220_USB) += phy-hi6220-usb.o
> obj-$(CONFIG_PHY_SUN4I_USB) += phy-sun4i-usb.o
> obj-$(CONFIG_PHY_SUN9I_USB) += phy-sun9i-usb.o
> obj-$(CONFIG_PHY_SAMSUNG_USB2) += phy-exynos-usb2.o
> diff --git a/drivers/phy/phy-hi6220-usb.c b/drivers/phy/phy-hi6220-usb.c
> new file mode 100644
> index 000..b2141cb
> --- /dev/null
> +++ b/drivers/phy/phy-hi6220-usb.c
> @@ -0,0 +1,168 @@
> +/*
> + * Copyright (c) 2015 Linaro Ltd.
> + * Copyright (c) 2015 Hisilicon Limited.
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#define SC_PERIPH_CTRL4 0x00c
> +
> +#define CTRL4_PICO_SIDDQ BIT(6)
> +#define CTRL4_PICO_OGDISABLE BIT(8)
> +#define CTRL4_PICO_VBUSVLDEXTBIT(10)
> +#define CTRL4_PICO_VBUSVLDEXTSEL BIT(11)
> +#define CTRL4_OTG_PHY_SELBIT(21)
> +
> +#define SC_PERIPH_CTRL5 0x010
> +
> +#define CTRL5_USBOTG_RES_SEL BIT(3)
> +#define CTRL5_PICOPHY_ACAENB BIT(4)
> +#define CTRL5_PICOPHY_BC_MODEBIT(5)
> +#define CTRL5_PICOPHY_CHRGSELBIT(6)
> +#define CTRL5_PICOPHY_VDATSRCEND BIT(7)
> +#define CTRL5_PICOPHY_VDATDETENB BIT(8)
> +#define CTRL5_PICOPHY_DCDENB BIT(9)
> +#define CTRL5_PICOPHY_IDDIG BIT(10)
> +
> +#define SC_PERIPH_CTRL8 0x018
> +#define SC_PERIPH_RSTEN0 0x300
> +#define SC_PERIPH_RSTDIS00x304
> +
> +#define RST0_USBOTG_BUS BIT(4)
> +#define RST0_POR_PICOPHY BIT(5)
> +#define RST0_USBOTG BIT(6)
> +#define RST0_USBOTG_32K BIT(7)
> +
> +#define EYE_PATTERN_PARA 0x7053348c
> +
> +struct hi6220_priv {
> + struct regmap *reg;
> + struct device *dev;
> +};
> +
> +static void hi6220_phy_init(struct hi6220_priv *priv)
> +{
> + struct regmap *reg = priv->reg;
> + u32 val, mask;
> +
> + val = RST0_USBOTG_BUS | RST0_POR_PICOPHY |
> + RST0_USBOTG | RST0_USBOTG_32K;
> + mask = val;
> + regmap_update_bits(reg, SC_PERIPH_RSTEN0,
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Hi, Robert Jarzmik writes: > Felipe Balbi writes: > >> Hi, >> >> this could be a bug in either g_ether or pxa27x... Seems like something >> is enabling endpoints which were already enabled. Not sure if this is >> pxa27x not _really_ disabling endpoints or g_ether being stupid. > You're probably right. > From what I remember, g_ether is enabling already enabled endpoints upon > resume, > but that memory is pretty thin and goes back to 2008 when I created the > driver, > so I'm not sure anymore. > >> yeah, so something is not disabling endpoints when they should :-) So >> this could be a bug with your suspend/resume callbacks. If you're going >> to disconnect from the bus, you need to tell the gadget driver about it, >> which means after disabling pullups, you should call >> gadget_driver->disconnect(). > Okay. > >> Can you see if this *stupid* and *untested* diff helps : > ...zip... > Yes it does. I don't understand why, but it does fix it, and the logs "already > enabled" are still gone. I wasn't aware that a disconnect was required in the > suspend, nor did I see it in another udc driver (in 2008 of course). it's not required on suspend :-) It's required because you disconnect data pullups and, essentially, disconnected from the bus. You won't get an IRQ for it however :-) I'll send this as a fix tomorrow, I'm pretty much done for today. How far back should this be backported ? Does it cover your side if we backport to v3.10+ ? >> I'm not 100% sure this is enough, as I'm not at all familiar with >> pxa27x, but that driver looks hugely unmaintained. > That would be my fault, as I'm maintaining it. That's the reason I'm seeing > the > regression. If you have on mind improvements required just say so, I'll see > what > I can do. > >> Which device are you using for development/test ? Is it this Mio A701 ? > Yes it is. cool. >> I can't find any sources of it :-s > What about arch/arm/mach-pxa/mioa701.c ? I mean a real-world source (ebay, amazon, etc) :-) -- balbi signature.asc Description: PGP signature
Re: [4.4-rc1 regression] pxa27x_udc and suspend/resume
Felipe Balbi writes: > Hi, > >> Euh it's expected to work _after resume_. > > sure is, but you can't expect it to have a proper IP. From host's > perspective this is a brand new usb0 network interface. Unless you have > dhcp client running on the phone and a server on your PC, you can't > assume it'll have IP ;-) (or statically configured, yada yada yada ;-) On the board, it's very static, ie. in an init (think /etc/init.d/XX) file: /sbin/ifconfig usb0 192.168.0.202 up On the host, it's static also, but a bit more robust, as upon usb0 network interface creation the same IP is always assigned. Maybe the board's too "simple" assignement scheme has always been faulty (ie. assigning only once an IP), and the recent commit revealed a long time flaw, who knows ... if the interface is removed then added again, I might be screwed (as the ip configuration is lost) ... > I got that :-) My surprise is how you managed to get IP > before. Statically configured ? Yeah, once as I said on /init execution. That might be what is buggy, and which I only see now ... Cheers. -- Robert -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote: > On Wed, 18 Nov 2015 21:37:42 +0100 (CET) > Jiri Kosina wrote: > > > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote: > > > > > The critical section protected by usbhid->lock in hid_ctrl() is too > > > big and in rare cases causes a recursive deadlock because of its call > > > to hid_input_report(). > > > > > > This deadlock reproduces on newer wacom tablets like 056a:033c because > > > the wacom driver in its irq handler ends up calling hid_hw_request() > > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means > > > is that it submits a report to reschedule a proximity read through a > > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb) > > > before calling hid_input_report(). When the irq kicks in on the same > > > cpu, it also tries to grab the lock resulting in a recursive deadlock. > > > > > > The proper fix is to shrink the critical section in hid_ctrl() to > > > protect only the instructions which modify usbhid, thus move the lock > > > after the hid_input_report() call and the deadlock dissapears. > > > > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), > > isn't it? > > > > That was my first attempt, yes, but the deadlock still happens with interrupts > disabled. It is very weird, I know. I think your best course of action is to figure out why this is the case, instead of continuing with trying to solve the symptoms. Do you have actual callstacks showing the cases where you hit? That might be useful to share (your lockdep picture cuts out the callstacks). Also, have you tried without the PREEMPT_RT patch in the picture at all? Josh signature.asc Description: PGP signature
Re: [PATCH 1/2] usb: doc: dwc3-xilinx: Add devicetree bindings
On Wed, Nov 18, 2015 at 5:02 PM, Felipe Balbi wrote: > > Hi, > > Rob Herring writes: >> On Wed, Nov 18, 2015 at 06:20:31PM +0530, Subbaraya Sundeep Bhatta wrote: >>> This patch adds binding doc for Xilinx DWC3 glue driver. >>> >>> Signed-off-by: Subbaraya Sundeep Bhatta >> >> I really dislike how the DWC3 binding has been done. The sub-node here >> is pointless. The only thing the outer node does is add clocks which >> should simply be part of the DWC3 node. Presumably the IP block needs >> some clocks... >> >> Anyway, it's self-contained within the DWC3, so I won't make you clean >> up the mess. > > heh, it's definitely not pointless. We get a lot of free goodies by > doing it that way. I'm just not going to repeat it once again. But in > summary: > > a) we force people to write glue layers for *only* their HW specific > details > > b) there's really no way to abuse dwc3 core because there's no symbol > exported from dwc3 core to glue Both are doable independent of DT. > c) PM (both system sleep and runtime) becomes a lot simpler with core > only caring about what PM details delivered by SNPS (e.g. Hibernation > mode from DWC3) and glue only has to consider the SoC integration parts > of PM (for OMAP that would be PRCM details, hwmod, etc). No doubt OMAP is special. > I'm definitely not going to change dwc3 because it has made my life a > lot saner. Definitely a lot saner than MUSB. Besides, DTS is supposed to > describe the HW and that's, really, how the HW is. So reading the description, the DWC3 has no clocks? > There's a piece of HW which is somewhat platform agnostic and delivered > by SNPS as a black box (SNPS customers don't touch SNPS RTL) and there's > another piece of HW which bridges this dwc3 to the platform specific > details and integration context of the platform (for OMAP that would the > SYSCONFIG/SYSSTATUS registers, Clock autogating, PRCM, etc, etc etc). It is a black box, but with dozens of configuration options typically. > So you _do_ in fact, have two separate pieces of HW which are SW visible > and controllable independently. They each have their own IRQs (though in > some SoCs they share the same line), they're own address space, yada > yada yada. When that is the case, I have no problem with this split, but I don't see any of these examples in this particular case. So how should the binding look when there is no vendor specific glue? Are we supposed to keep the binding structure to appease the needs of the driver? Rob -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 2/2] usb: dwc2: host: Clear interrupts before handling them
On 11/16/2015 9:22 AM, Doug Anderson wrote:
> Felipe,
>
> On Mon, Nov 16, 2015 at 8:28 AM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Douglas Anderson writes:
>>> In general it is wise to clear interrupts before processing them. If
>>> you don't do that, you can get:
>>> 1. Interrupt happens
>>> 2. You look at system state and process interrupt
>>> 3. A new interrupt happens
>>> 4. You clear interrupt without processing it.
>>>
>>> This patch was actually a first attempt to fix missing device insertions
>>> as described in (usb: dwc2: host: Fix missing device insertions) and it
>>> did solve some of the signal bouncing problems but not all of
>>> them (which is why I submitted the other patch). Specifically, this
>>> patch itself would sometimes change:
>>> 1. hardware sees connect
>>> 2. hardware sees disconnect
>>> 3. hardware sees connect
>>> 4. dwc2_port_intr() - clears connect interrupt
>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...to:
>>> 1. hardware sees connect
>>> 2. hardware sees disconnect
>>> 3. dwc2_port_intr() - clears connect interrupt
>>> 4. hardware sees connect
>>> 5. dwc2_handle_common_intr() - calls dwc2_hcd_disconnect()
>>>
>>> ...but with different timing then sometimes we'd still miss cable
>>> insertions.
>>>
>>> In any case, though this patch doesn't fix any (known) problems, it
>>> still seems wise as a general policy to clear interrupt before handling
>>> them.
>>>
>>> Signed-off-by: Douglas Anderson
>>> ---
>>> Changes in v2: None
>>>
>>> drivers/usb/dwc2/core_intr.c | 55
>>> ++--
>>> drivers/usb/dwc2/hcd_intr.c | 16 ++---
>>> 2 files changed, 35 insertions(+), 36 deletions(-)
>>>
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index 61601d16e233..2a166b7eec41 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -80,15 +80,16 @@ static const char *dwc2_op_state_str(struct dwc2_hsotg
>>> *hsotg)
>>> */
>>> static void dwc2_handle_usb_port_intr(struct dwc2_hsotg *hsotg)
>>> {
>>> - u32 hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> + u32 hprt0;
>>>
>>> + /* Clear interrupt */
>>> + dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>> +
>>> + hprt0 = dwc2_readl(hsotg->regs + HPRT0);
>>> if (hprt0 & HPRT0_ENACHG) {
>>> hprt0 &= ~HPRT0_ENA;
>>> dwc2_writel(hprt0, hsotg->regs + HPRT0);
>>> }
>>> -
>>> - /* Clear interrupt */
>>> - dwc2_writel(GINTSTS_PRTINT, hsotg->regs + GINTSTS);
>>
>> isn't this a regression ? You're first clearing the interrupts and only
>> then reading to check what's pending, however, what's pending has just
>> been cleared. Seems like this should be:
>>
>> hprt0 = dwc2_readl(HPRT0);
>> dwc2_writeal(PRTINT, GINTSTS);
>
> Actually, we could probably remove the setting of GINTSTS_PRTINT
> completely. The docs I have say that the GINTSTS_PRTINT is read only
> and that:
>
>> The core sets this bit to indicate a change in port status of one of the
>> DWC_otg core ports in Host mode. The application must read the
>> Host Port Control and Status (HPRT) register to determine the exact
>> event that caused this interrupt. The application must clear the
>> appropriate status bit in the Host Port Control and Status register to
>> clear this bit.
>
> ...so writing PRTINT is probably useless, but John can confirm.
>
Yup, it seems it can be removed.
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: dwc2: add support of hi6220
On 11/17/2015 11:39 PM, Zhangfei Gao wrote:
> Support hisilicon,hi6220-usb for HiKey board
>
> Signed-off-by: Zhangfei Gao
> ---
> Documentation/devicetree/bindings/usb/dwc2.txt | 1 +
> drivers/usb/dwc2/platform.c| 32
> ++
> 2 files changed, 33 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/usb/dwc2.txt
> b/Documentation/devicetree/bindings/usb/dwc2.txt
> index fd132cb..2213682 100644
> --- a/Documentation/devicetree/bindings/usb/dwc2.txt
> +++ b/Documentation/devicetree/bindings/usb/dwc2.txt
> @@ -4,6 +4,7 @@ Platform DesignWare HS OTG USB 2.0 controller
> Required properties:
> - compatible : One of:
>- brcm,bcm2835-usb: The DWC2 USB controller instance in the BCM2835 SoC.
> + - hisilicon,hi6220-usb: The DWC2 USB controller instance in the hi6220 SoC.
>- rockchip,rk3066-usb: The DWC2 USB controller instance in the rk3066 Soc;
>- "rockchip,rk3188-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3188
> Soc;
>- "rockchip,rk3288-usb", "rockchip,rk3066-usb", "snps,dwc2": for rk3288
> Soc;
> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
> index 5859b0f..a5cb1bf 100644
> --- a/drivers/usb/dwc2/platform.c
> +++ b/drivers/usb/dwc2/platform.c
> @@ -54,6 +54,37 @@
>
> static const char dwc2_driver_name[] = "dwc2";
>
> +static const struct dwc2_core_params params_hi6220 = {
> + .otg_cap= 2,/* No HNP/SRP capable */
> + .otg_ver= 0,/* 1.3 */
> + .dma_enable = 1,
> + .dma_desc_enable= 0,
> + .speed = 0,/* High Speed */
> + .enable_dynamic_fifo= 1,
> + .en_multiple_tx_fifo= 1,
> + .host_rx_fifo_size = 512,
> + .host_nperio_tx_fifo_size = 512,
> + .host_perio_tx_fifo_size= 512,
> + .max_transfer_size = 65535,
> + .max_packet_count = 511,
> + .host_channels = 16,
> + .phy_type = 1,/* UTMI */
> + .phy_utmi_width = 8,
> + .phy_ulpi_ddr = 0,/* Single */
> + .phy_ulpi_ext_vbus = 0,
> + .i2c_enable = 0,
> + .ulpi_fs_ls = 0,
> + .host_support_fs_ls_low_power = 0,
> + .host_ls_low_power_phy_clk = 0,/* 48 MHz */
> + .ts_dline = 0,
> + .reload_ctl = 0,
> + .ahbcfg = GAHBCFG_HBSTLEN_INCR16 <<
> + GAHBCFG_HBSTLEN_SHIFT,
> + .uframe_sched = 0,
> + .external_id_pin_ctl= -1,
> + .hibernation= -1,
> +};
> +
> static const struct dwc2_core_params params_bcm2835 = {
> .otg_cap= 0,/* HNP/SRP capable */
> .otg_ver= 0,/* 1.3 */
> @@ -282,6 +313,7 @@ static int dwc2_driver_remove(struct platform_device *dev)
>
> static const struct of_device_id dwc2_of_match_table[] = {
> { .compatible = "brcm,bcm2835-usb", .data = ¶ms_bcm2835 },
> + { .compatible = "hisilicon,hi6220-usb", .data = ¶ms_hi6220 },
> { .compatible = "rockchip,rk3066-usb", .data = ¶ms_rk3066 },
> { .compatible = "snps,dwc2", .data = NULL },
> { .compatible = "samsung,s3c6400-hsotg", .data = NULL},
>
Acked-by: John Youn
Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
On 18 November 2015 at 23:32, Peter Hurley wrote:
> Hi Baolin,
>
> On 11/16/2015 02:05 AM, Baolin Wang wrote:
>> It dose not work when we want to use the usb-to-serial port based
>> on one usb gadget as a console. Thus this patch adds the console
>> initialization to support this request.
>
> I have some high level concerns.
>
> 1. I would defer registering the console until the port has at least been
>allocated in gserial_alloc_line(), and unregister when the line is freed.
>That also reduces many of the conditions that you shouldn't need to
>check, like port number range and so on.
The 'setup' callback of 'struct console' is just do some memory
allocation and member initialization, that no need to defer
registering the console in gserial_alloc_line(). But the
'gs_console_connect()' which will use the port need to be called in
gserial_connect().
>
>Further, consider deferring the console registration until
> gserial_connect();
>that would further simplify things. In this case, unregistration would
>happen at disconnect.
That sounds reasonable. I would try.
>
> 2. Why are you using a kworker for actual device i/o when all of the i/o
>is done with interrupts disabled anyway?
>Getting rid of the work would eliminate using the 8K intermediate buffer
>as well.
If remove the kworker, there are some deadlocks to call the printk()
when in the process of transferring data with usb endpoint. So we need
a async kworker to complete the io or it can not work.
>
> 3. If for some reason i/o deferral is really necessary, consider using a
> kthread
>kworker instead of the pooled kworker. The scheduler response will be _way_
>better.
>
OK, make sense.
> 4. If for some reason i/o deferral is really necessary, use a circular buffer
>for the intermediate buffer, preferably lockless since there is only
>one producer and one consumer.
>
Yeah, the circular buffer is better but more tricky. I would try.
> Some other review comments below; please ignore anything other reviewers
> have already noted.
>
> Regards,
> Peter Hurley
>
>> Signed-off-by: Baolin Wang
>> ---
>> drivers/usb/gadget/Kconfig |6 +
>> drivers/usb/gadget/function/u_serial.c | 238
>>
>> 2 files changed, 244 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/Kconfig b/drivers/usb/gadget/Kconfig
>> index 33834aa..be5aab9 100644
>> --- a/drivers/usb/gadget/Kconfig
>> +++ b/drivers/usb/gadget/Kconfig
>> @@ -127,6 +127,12 @@ config USB_GADGET_STORAGE_NUM_BUFFERS
>> a module parameter as well.
>> If unsure, say 2.
>>
>> +config U_SERIAL_CONSOLE
>> + bool "Serial gadget console support"
>> + depends on USB_G_SERIAL
>> + help
>> +It supports the serial gadget can be used as a console.
>> +
>> source "drivers/usb/gadget/udc/Kconfig"
>>
>> #
>> diff --git a/drivers/usb/gadget/function/u_serial.c
>> b/drivers/usb/gadget/function/u_serial.c
>> index f7771d8..4ade527 100644
>> --- a/drivers/usb/gadget/function/u_serial.c
>> +++ b/drivers/usb/gadget/function/u_serial.c
>> @@ -27,6 +27,7 @@
>> #include
>> #include
>> #include
>> +#include
>>
>> #include "u_serial.h"
>>
>> @@ -79,6 +80,16 @@
>> */
>> #define QUEUE_SIZE 16
>> #define WRITE_BUF_SIZE 8192/* TX only */
>> +#define GS_BUFFER_SIZE (4096)
>> +#define GS_CONSOLE_BUF_SIZE (2 * GS_BUFFER_SIZE)
>> +
>> +struct gscons_info {
>> + struct gs_port *port;
>> + struct tty_driver *tty_driver;
>> + struct work_struct work;
>> + int buf_tail;
>> + charbuf[GS_CONSOLE_BUF_SIZE];
>> +};
>
> Make struct gscons_info a static declaration instead of
> allocating it.
But I think the dynamic allocation is more reasonable with reducing
some global variables.
>
>>
>> /* circular buffer */
>> struct gs_buf {
>> @@ -118,6 +129,7 @@ struct gs_port {
>>
>> /* REVISIT this state ... */
>> struct usb_cdc_line_coding port_line_coding;/* 8-N-1 etc */
>> + struct usb_request *console_req;
>> };
>>
>> static struct portmaster {
>> @@ -1054,6 +1066,7 @@ gs_port_alloc(unsigned port_num, struct
>> usb_cdc_line_coding *coding)
>>
>> port->port_num = port_num;
>> port->port_line_coding = *coding;
>> + port->console_req = NULL;
>>
>> ports[port_num].port = port;
>> out:
>> @@ -1143,6 +1156,227 @@ err:
>> }
>> EXPORT_SYMBOL_GPL(gserial_alloc_line);
>>
>> +#ifdef CONFIG_U_SERIAL_CONSOLE
>> +
>> +static struct usb_request *gs_request_new(struct usb_ep *ep, int
>> buffer_size)
>
> ^^^
> With only a single caller that uses a symbolic constant, is the
> 'buffer_size' parameter necessary?
>
Yeah, I'll remove the 'buffer_size' parameter.
>
>> +{
>> + struct usb_request *req = usb_ep_alloc_request(ep, GFP_ATOMIC);
>> +
>
> Remove t
Obtaining USB vendor/product ID in C language given a modem file path
Hi, If this isn't the correct mailing list for this question, do let me know if there's a better one where I could ask. I have a C language program that talks to modems and these days it is almost always a USB modem. For diagnostic purposes, I would like to be able to pass the modem's file path and get back the vendor and product ID info (at least), and if I could also get the vendor name, that would be great. Portable code would be the best so I can compile it on Ubuntu, Fedora, Raspbian, etc. For example, given the modem is at /dev/ttyACM0 and lsusb gives: Bus 001 Device 004: ID 0572:1329 Conexant Systems (Rockwell), Inc. I'm looking to pass /dev/ttyACM0 to some C code and ideally get the string "0572:1329 Conexant Systems (Rockwell), Inc.", but if I can only easily get just the "0572:1329" string, that would be OK, too. Once retrieved it would simply be logged to a text file and/or printed on the screen. I already know about modem AT query commands (like ATI3) but I'm looking to get the USB hardware info. Can anyone help? Regards, Todd -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Obtaining USB vendor/product ID in C language given a modem file path
On Wed, 2015-11-18 at 18:56 -0800, ToddA wrote: > Hi, > > If this isn't the correct mailing list for this question, do let me > know > if there's a better one where I could ask. > > I have a C language program that talks to modems and these days it is > almost always a USB modem. For diagnostic purposes, I would like to > be > able to pass the modem's file path and get back the vendor and > product > ID info (at least), and if I could also get the vendor name, that > would > be great. Portable code would be the best so I can compile it on > Ubuntu, > Fedora, Raspbian, etc. > > For example, given the modem is at /dev/ttyACM0 and lsusb gives: > > Bus 001 Device 004: ID 0572:1329 Conexant Systems (Rockwell), > Inc. cat /sys/class/tty/ttyACM0/device/../idVendor cat /sys/class/tty/ttyACM0/device/../idProduct Dan -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v12 1/3] dt-bindings: Add a binding for Mediatek xHCI host controller
Hi, On Tue, 2015-11-17 at 13:58 +0300, Sergei Shtylyov wrote: > Hello. > > On 11/17/2015 12:18 PM, Chunfeng Yun wrote: > > > add a DT binding documentation of xHCI host controller for the > > MT8173 SoC from Mediatek. > > > > Signed-off-by: Chunfeng Yun > > --- > > .../devicetree/bindings/usb/mt8173-xhci.txt| 51 > > ++ > > 1 file changed, 51 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/usb/mt8173-xhci.txt > > > > diff --git a/Documentation/devicetree/bindings/usb/mt8173-xhci.txt > > b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt > > new file mode 100644 > > index 000..a78f20b > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/usb/mt8173-xhci.txt > > @@ -0,0 +1,51 @@ > > +MT8173 xHCI > > + > > +The device node for Mediatek SOC USB3.0 host controller > > + > > +Required properties: > > + - compatible : should contain "mediatek,mt8173-xhci" > > + - reg : specifies physical base address and size of the registers, > > + the first one for MAC, the second for IPPC > > + - interrupts : interrupt used by the controller > > + - power-domains : a phandle to USB power domain node to control USB's > > + mtcmos > > What's that? It works as a gateway in fact which can turn on/off usb power > > > + - vusb33-supply : regulator of USB avdd3.3v > > + > > + - clocks : a list of phandle + clock-specifier pairs, one for each > > + entry in clock-names > > + - clock-names : must contain > > + "sys_ck": for clock of xHCI MAC > > + "wakeup_deb_p0": for USB wakeup debounce clock of port0 > > + "wakeup_deb_p0": for USB wakeup debounce clock of port1 > > "wakeup_deb_p1"? Yes, it's a typo > > > + > > + - phys : a list of phandle + phy specifier pairs > > + > > +Optional properties: > > + - mediatek,wakeup-src : 1: ip sleep wakeup mode; 2: line state wakeup > ^^ IP? Ok > > > + mode; > > + - mediatek,syscon-wakeup : phandle to syscon used to access USB wakeup > > + control register, it depends on "mediatek,wakeup-src". > > + - vbus-supply : reference to the VBUS regulator; > > + - usb3-lpm-capable : supports USB3.0 LPM > > [...] > > MBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/1] usb: kconfig: fix warning of select USB_OTG
When choose randconfig for kernel build, it reports below warning: "warning: (USB_OTG_FSM && FSL_USB2_OTG && USB_MV_OTG) selects USB_OTG which has unmet direct dependencies (USB_SUPPORT && USB && PM)" In fact, USB_OTG is visible symbol and depends on PM, so the driver needs to depend on it to reduce dependency problem. Signed-off-by: Peter Chen Reported-by: Arnd Bergmann Cc: Felipe Balbi Acked-by: Arnd Bergmann --- drivers/usb/core/Kconfig | 3 +-- drivers/usb/phy/Kconfig | 4 +--- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/drivers/usb/core/Kconfig b/drivers/usb/core/Kconfig index a99c89e..dd28010 100644 --- a/drivers/usb/core/Kconfig +++ b/drivers/usb/core/Kconfig @@ -77,8 +77,7 @@ config USB_OTG_BLACKLIST_HUB config USB_OTG_FSM tristate "USB 2.0 OTG FSM implementation" - depends on USB - select USB_OTG + depends on USB && USB_OTG select USB_PHY help Implements OTG Finite State Machine as specified in On-The-Go diff --git a/drivers/usb/phy/Kconfig b/drivers/usb/phy/Kconfig index 1731324..22e8ecb 100644 --- a/drivers/usb/phy/Kconfig +++ b/drivers/usb/phy/Kconfig @@ -21,7 +21,6 @@ config AB8500_USB config FSL_USB2_OTG bool "Freescale USB OTG Transceiver Driver" depends on USB_EHCI_FSL && USB_FSL_USB2 && USB_OTG_FSM && PM - select USB_OTG select USB_PHY help Enable this to support Freescale USB OTG transceiver. @@ -168,8 +167,7 @@ config USB_QCOM_8X16_PHY config USB_MV_OTG tristate "Marvell USB OTG support" - depends on USB_EHCI_MV && USB_MV_UDC && PM - select USB_OTG + depends on USB_EHCI_MV && USB_MV_UDC && PM && USB_OTG select USB_PHY help Say Y here if you want to build Marvell USB OTG transciever -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH net,stable] net: qmi_wwan: add XS Stick W100-2 from 4G Systems
From: Bjørn Mork Date: Wed, 18 Nov 2015 21:13:07 +0100 > Thomas reports ... > Reported-by: Thomas Schäfer > Signed-off-by: Bjørn Mork Applied and queued up for -stable, thanks. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] usb: doc: dwc3-xilinx: Add devicetree bindings
Hi, Rob Herring writes: > On Wed, Nov 18, 2015 at 5:02 PM, Felipe Balbi wrote: >> >> Hi, >> >> Rob Herring writes: >>> On Wed, Nov 18, 2015 at 06:20:31PM +0530, Subbaraya Sundeep Bhatta wrote: This patch adds binding doc for Xilinx DWC3 glue driver. Signed-off-by: Subbaraya Sundeep Bhatta >>> >>> I really dislike how the DWC3 binding has been done. The sub-node here >>> is pointless. The only thing the outer node does is add clocks which >>> should simply be part of the DWC3 node. Presumably the IP block needs >>> some clocks... >>> >>> Anyway, it's self-contained within the DWC3, so I won't make you clean >>> up the mess. >> >> heh, it's definitely not pointless. We get a lot of free goodies by >> doing it that way. I'm just not going to repeat it once again. But in >> summary: >> >> a) we force people to write glue layers for *only* their HW specific >> details >> >> b) there's really no way to abuse dwc3 core because there's no symbol >> exported from dwc3 core to glue > > Both are doable independent of DT. > >> c) PM (both system sleep and runtime) becomes a lot simpler with core >> only caring about what PM details delivered by SNPS (e.g. Hibernation >> mode from DWC3) and glue only has to consider the SoC integration parts >> of PM (for OMAP that would be PRCM details, hwmod, etc). > > No doubt OMAP is special. not only OMAP. Every single SoC will integrate a particular IP in its own way. >> I'm definitely not going to change dwc3 because it has made my life a >> lot saner. Definitely a lot saner than MUSB. Besides, DTS is supposed to >> describe the HW and that's, really, how the HW is. > > So reading the description, the DWC3 has no clocks? of course it has, and you have registers in DWC3 to change some of the parents of the clocks it uses internally. >> There's a piece of HW which is somewhat platform agnostic and delivered >> by SNPS as a black box (SNPS customers don't touch SNPS RTL) and there's >> another piece of HW which bridges this dwc3 to the platform specific >> details and integration context of the platform (for OMAP that would the >> SYSCONFIG/SYSSTATUS registers, Clock autogating, PRCM, etc, etc etc). > > It is a black box, but with dozens of configuration options typically. all of which are detected within the driver. For those which can't be, we have bindings. >> So you _do_ in fact, have two separate pieces of HW which are SW visible >> and controllable independently. They each have their own IRQs (though in >> some SoCs they share the same line), they're own address space, yada >> yada yada. > > When that is the case, I have no problem with this split, but I don't > see any of these examples in this particular case. So how should the > binding look when there is no vendor specific glue? Are we supposed to > keep the binding structure to appease the needs of the driver? If there really is *no* vendor specific glue, nothing prevents them from patching dwc3 to understand *OPTIONAL* clocks and use dwc3 directly. As long as it doesn't break any of the platforms currently supported and doesn't look ugly, fine with me. In fact, there's one company which has been using dwc3 without a glue layer. I forgot who told me they didn't need a glue layer, but it's in the archives. -- balbi signature.asc Description: PGP signature
Re: Udoo support for chipidea
On Wed, Oct 21, 2015 at 10:39:00AM +0800, Peter Chen wrote: > On Tue, Oct 20, 2015 at 02:09:38PM -0200, Fabio Estevam wrote: > > Hi Peter, > > > > On Mon, Oct 19, 2015 at 12:50 AM, Peter Chen > > wrote: > > > > > Add linux-usb. > > > > > > Patryk, your problem is you may need to open 24M OSC for HUB 2514 > > > manually, and you have used IMX6QDL_CLK_CKO for it in the design, > > > but this clock is not controller's clock, controller's clock has > > > already decided at SoC dts file (imx6qdl.dtsi), you don't need to > > > override it at board's dts file. > > > > > > You can try delete clock property at imx6qdl-udoo.dtsi, if it still > > > can't work, try to open IMX6QDL_CLK_CKO at one place to test. > > > > What is the appropriate place to acquire and enable the USB hub clock? > > > > This issue has appeared several times and it seems we don't have a > > solution for this yet. > > > > Any suggestions? > > Add Alan. > > Hi Alan, we have several designs that the on-board HUB need to > be reset by gpio pin and its clock is also from the board or > the SoC. Any suggestions how to add these platform information > for HUB device? How about putting it in the device tree? http://www.firmware.org/1275/bindings/usb/usb-1_0.ps clocks and reset-gpios properties could be added to the USB hub node. regards Philipp -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] hid: usbhid: hid-core: fix recursive deadlock
On Wed, 18 Nov 2015 17:58:56 -0600 Josh Cartwright wrote: > On Wed, Nov 18, 2015 at 11:05:44PM +0200, Ioan-Adrian Ratiu wrote: > > On Wed, 18 Nov 2015 21:37:42 +0100 (CET) > > Jiri Kosina wrote: > > > > > On Wed, 18 Nov 2015, Ioan-Adrian Ratiu wrote: > > > > > > > The critical section protected by usbhid->lock in hid_ctrl() is too > > > > big and in rare cases causes a recursive deadlock because of its call > > > > to hid_input_report(). > > > > > > > > This deadlock reproduces on newer wacom tablets like 056a:033c because > > > > the wacom driver in its irq handler ends up calling hid_hw_request() > > > > from wacom_intuos_schedule_prox_event() in wacom_wac.c. What this means > > > > is that it submits a report to reschedule a proximity read through a > > > > sync ctrl call which grabs the lock in hid_ctrl(struct urb *urb) > > > > before calling hid_input_report(). When the irq kicks in on the same > > > > cpu, it also tries to grab the lock resulting in a recursive deadlock. > > > > > > > > The proper fix is to shrink the critical section in hid_ctrl() to > > > > protect only the instructions which modify usbhid, thus move the lock > > > > after the hid_input_report() call and the deadlock dissapears. > > > > > > I think the proper fix actually is to spin_lock_irqsave() in hid_ctrl(), > > > isn't it? > > > > > > > That was my first attempt, yes, but the deadlock still happens with > > interrupts disabled. It is very weird, I know. > > I think your best course of action is to figure out why this is the > case, instead of continuing with trying to solve the symptoms. Do you > have actual callstacks showing the cases where you hit? That might be > useful to share (your lockdep picture cuts out the callstacks). > > Also, have you tried without the PREEMPT_RT patch in the picture at all? > > Josh Yes, of course I tried it without PREEMPT_RT_FULL :) This happens on vanilla mainline kernels (only after 4.4-rc1 which introduced support for this kind of tablets). I also backported all the wacom patches to 4.1 non-RT and the same deadlock happens. I've sent another email with some lockdep traces and printk's on a running vanilla linux-next, maybe it didn't get through, here are the links again: First part of lockdep report: http://imgur.com/clLsCWe Second part: http://imgur.com/Wa2PzRl Here are some printk's of mine while reproducing + debugging the issue: http://imgur.com/SETOHT7 I'll continue to research this more in depth, but progress is slow because I don't have much time, I'm doing this in my spare time because it's my girlfriend's tablet. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] usb: gadget: Add the console support for usb-to-serial port
>
>> +{
>> + struct gscons_info *info = gserial_cons.data;
>> + int port_num = gserial_cons.index;
>> + struct usb_request *req;
>> + struct gs_port *port;
>> + struct usb_ep *ep;
>> +
>> + if (port_num >= MAX_U_SERIAL_PORTS || port_num < 0) {
>> + pr_err("%s: port num [%d] exceeds the range.\n",
>> +__func__, port_num);
>> + return -ENXIO;
>> + }
>> +
>> + port = ports[port_num].port;
>> + if (!port) {
>> + pr_err("%s: serial line [%d] not allocated.\n",
>> +__func__, port_num);
>> + return -ENODEV;
>> + }
>> +
>> + if (!port->port_usb) {
>> + pr_err("%s: no port usb.\n", __func__);
>> + return -ENODEV;
>> + }
>> +
>> + ep = port->port_usb->in;
>> + if (!ep) {
>> + pr_err("%s: no usb endpoint.\n", __func__);
>> + return -ENXIO;
>> + }
>
> Looking at the caller, gserial_connect(), none of the error
> conditions above look possible.
>
I re-look the code and do some tests, I found the checking is
necessary. Cause we get the port number from the console->index, if
the cmdline is not set the ttyGS0 as the console, the console->index
will be -1 that is a wrong value. Also the serial.c file will create 1
usb-to-seial port as default (default n_ports = 1), so we need to
check the port and the endpoint of the port. So I think here checking
is necessary and I have tested it.
--
Baolin.wang
Best Regards
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 0/4] usb: gadget: add queue depth for sourcesink
Hi Felipe and others, This patch set adds multiple queue support for gadget sourcesink function, it adds additional two parameters for both bulk and iso transfer, the default value is current setting. Changes for v2: - Rebase to latest v4.4-rc1 - Agree Krzysztof's suggestion, and add patch 4/4 to support quit from multiple qlen loop if usb_ep_queue returns error. Peter Chen (4): usb: gadget: f_sourcesink: add queue depth Documentation: usb: gadget-testing: add description for depth of queue Doc: ABI: configfs-usb-gadget-sourcesink: add two entries for depth of queue usb: gadget: f_sourcesink: quit if usb_ep_queue returns error .../ABI/testing/configfs-usb-gadget-sourcesink | 2 + Documentation/usb/gadget-testing.txt | 2 + drivers/usb/gadget/function/f_sourcesink.c | 143 - drivers/usb/gadget/function/g_zero.h | 6 + drivers/usb/gadget/legacy/zero.c | 12 ++ 5 files changed, 132 insertions(+), 33 deletions(-) -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 3/4] Doc: ABI: configfs-usb-gadget-sourcesink: add two entries for depth of queue
Add both bulk and iso depth of queue entries. Signed-off-by: Peter Chen --- Documentation/ABI/testing/configfs-usb-gadget-sourcesink | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink index bc7ff73..f56335a 100644 --- a/Documentation/ABI/testing/configfs-usb-gadget-sourcesink +++ b/Documentation/ABI/testing/configfs-usb-gadget-sourcesink @@ -10,3 +10,5 @@ Description: isoc_mult - 0..2 (hs/ss only) isoc_maxburst - 0..15 (ss only) buflen - buffer length + bulk_qlen - depth of queue for bulk + iso_qlen- depth of queue for iso -- 1.9.1 -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
