Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity for imx6 and imx7
On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote:
> As all usb power supply use low active for over current flag on imx6
> imx7 boards, and the default register setting(0) is for high active,
> this patch is to correct it.
>
We may can't ensure all USB power switch chips work like that, I
suggest you making this as default.
I will change the commit log like below if you are ok.
As most of all usb power switch chips use active-low for over current
flag, but the default register setting(0) is for active-high at
imx6/imx7, this patch changes default value as active-low.
> Signed-off-by: Li Jun
> ---
> drivers/usb/chipidea/usbmisc_imx.c | 18 --
> 1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c
> b/drivers/usb/chipidea/usbmisc_imx.c
> index ab8b027..16e834b 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -56,6 +56,7 @@
>
> #define MX6_BM_NON_BURST_SETTING BIT(1)
> #define MX6_BM_OVER_CUR_DIS BIT(7)
> +#define MX6_BM_OVER_CUR_POLARITY BIT(8)
> #define MX6_BM_WAKEUP_ENABLE BIT(10)
> #define MX6_BM_ID_WAKEUP BIT(16)
> #define MX6_BM_VBUS_WAKEUP BIT(17)
> @@ -266,10 +267,15 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data
> *data)
>
> spin_lock_irqsave(&usbmisc->lock, flags);
>
> + reg = readl(usbmisc->base + data->index * 4);
> if (data->disable_oc) {
> - reg = readl(usbmisc->base + data->index * 4);
> writel(reg | MX6_BM_OVER_CUR_DIS,
> usbmisc->base + data->index * 4);
> + } else {
> + reg &= ~MX6_BM_OVER_CUR_DIS;
> + /* OC flag is active low */
> + writel(reg | MX6_BM_OVER_CUR_POLARITY,
> + usbmisc->base + data->index * 4);
> }
>
> /* SoC non-burst setting */
> @@ -365,9 +371,13 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data
> *data)
> return -EINVAL;
>
> spin_lock_irqsave(&usbmisc->lock, flags);
> + reg = readl(usbmisc->base);
> if (data->disable_oc) {
> - reg = readl(usbmisc->base);
> writel(reg | MX6_BM_OVER_CUR_DIS, usbmisc->base);
> + } else {
> + reg &= ~MX6_BM_OVER_CUR_DIS;
> + /* OC flag is active low */
> + writel(reg | MX6_BM_OVER_CUR_POLARITY, usbmisc->base);
> }
>
> reg = readl(usbmisc->base + MX7D_USBNC_USB_CTRL2);
> @@ -492,6 +502,10 @@ static const struct of_device_id usbmisc_imx_dt_ids[] = {
> .compatible = "fsl,imx6ul-usbmisc",
> .data = &imx6sx_usbmisc_ops,
> },
> + {
> + .compatible = "fsl,imx7d-usbmisc",
> + .data = &imx7d_usbmisc_ops,
> + },
> { /* sentinel */ }
> };
> MODULE_DEVICE_TABLE(of, usbmisc_imx_dt_ids);
> --
> 2.6.6
>
--
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 v2 1/5] drivers: usb: chipidea: Add qoriq platform driver
On Tue, Jul 12, 2016 at 03:59:07AM +, Rajesh Bhagat wrote: > > > + > > > +err_clks: > > > + ci_hdrc_qoriq_disable_unprepare_clks(pdev); > > > > If you have only one clock, it is unnecessary to use dedicated APIs for > > clock operation. > > > > We do have multiple clocks, but currently one is integrated in code. Hence > created > the APIs for future use. If you could not integrate one more clocks, I suggest not creating dedicated API until you need in future. -- 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 v2 2/5] usb: DT binding documentation for qoriq usb 2.0 controller
On Tue, Jul 12, 2016 at 03:59:14AM +, Rajesh Bhagat wrote: > > > > -Original Message- > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > Sent: Monday, July 11, 2016 12:19 PM > > To: Rajesh Bhagat > > Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; > > devicet...@vger.kernel.org; Peter Chen ; > > gre...@linuxfoundation.org; kis...@ti.com; robh...@kernel.org; > > shawn...@kernel.org; linux-arm-ker...@lists.infradead.org > > Subject: Re: [PATCH v2 2/5] usb: DT binding documentation for qoriq usb 2.0 > > controller > > > > On Sat, Jul 09, 2016 at 10:00:53AM +0530, Rajesh Bhagat wrote: > > > Describes the qoriq usb 2.0 controller driver binding, currently used > > > for LS1021A and LS1012A platform. > > > > > > Signed-off-by: Rajesh Bhagat > > > --- > > > Changes in v2: > > > - Adds DT binding documentation for qoriq usb 2.0 controller > > > - Changed the compatible string to fsl,ci-qoriq-usb2 > > > > > > .../devicetree/bindings/usb/ci-hdrc-qoriq.txt | 34 > > ++ > > > 1 file changed, 34 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > > > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > new file mode 100644 > > > index 000..8ad7306 > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > @@ -0,0 +1,34 @@ > > > +* Freescale QorIQ SoC USB 2.0 Controllers > > > + > > > +Required properties: > > > +- compatible: Should be "fsl,ci-qoriq-usb2" > > > + Wherever applicable, the IP version of the USB controller should > > > + also be mentioned (for eg. fsl,ci-qoriq-usb2-vX.Y). > > > + where, X.Y is IP version of USB controller. > > Hello Peter, > > > > > Why you need to add IP version at compatible string? > > Does it can't be read out from ID register of Identification Registers. > > > > I agree. Will drop this controller version thing in DTS in v3. > > > > +- reg: Should contain registers location and length > > > +- interrupts: Should contain controller interrupt > > > +- phy-names: from the *Generic PHY* bindings > > > +- phys: from the *Generic PHY* bindings > > > +- clocks: clock provider specifier > > > +- clock-names: shall be "usb2-clock" > > > +Refer to clk/clock-bindings.txt for generic clock consumer properties > > > + > > > +Recommended properties: > > > +- dr_mode: One of "host" or "peripheral". > > > > Do you support dual-role? > > > > Yes. We do support both host/peripheral mode. > I mean dual-role switch. If you support that, the dr_mode should be "otg". -- 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 1/2] usb: typec: Add USB Power Delivery sink port support
Bin Gao writes:
> This patch implements a simple USB Power Delivery sink port state machine.
> It assumes the hardware only handles PD packet transmitting and receiving
> over the CC line of the USB Type-C connector. The state transition is
> completely controlled by software. This patch only implement the sink port
> function and it doesn't support source port and port swap yet.
>
> This patch depends on these two patches:
> https://lkml.org/lkml/2016/6/29/349
> https://lkml.org/lkml/2016/6/29/350
>
> Signed-off-by: Bin Gao
> ---
> drivers/usb/typec/Kconfig | 13 +
> drivers/usb/typec/Makefile | 1 +
> drivers/usb/typec/pd_sink.c| 967
> +
> include/linux/usb/pd_message.h | 371
> include/linux/usb/pd_sink.h| 286
> 5 files changed, 1638 insertions(+)
> create mode 100644 drivers/usb/typec/pd_sink.c
> create mode 100644 include/linux/usb/pd_message.h
> create mode 100644 include/linux/usb/pd_sink.h
>
> diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> index 7a345a4..a04a900 100644
> --- a/drivers/usb/typec/Kconfig
> +++ b/drivers/usb/typec/Kconfig
> @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
> config TYPEC
> tristate
>
> +config USB_PD_SINK
> + bool "USB Power Delivery Sink Port State Machine Driver"
tristate?
> + select TYPEC
this should depend on TYPEC, not select it.
> + help
> + Enable this to support USB PD(Power Delivery) Sink port.
> + This driver implements a simple USB PD sink state machine.
> + The underlying TypeC phy driver is responsible for cable
> + plug/unplug event, port orientation detection, transmitting
> + and receiving PD messages. This driver only process messages
> + received by the TypeC phy driver and maintain the sink port's
> + state machine.
> +
> config TYPEC_WCOVE
> tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
> depends on ACPI
> depends on INTEL_SOC_PMIC
> depends on INTEL_PMC_IPC
> select TYPEC
> + select USB_PD_SINK
TYPEC without PD is valid, let user select PD support.
> diff --git a/drivers/usb/typec/pd_sink.c b/drivers/usb/typec/pd_sink.c
> new file mode 100644
> index 000..374bdef
> --- /dev/null
> +++ b/drivers/usb/typec/pd_sink.c
> @@ -0,0 +1,967 @@
> +/*
> + * pd_sink.c - USB PD (Power Delivery) sink port state machine driver
> + *
> + * This driver implements a simple USB PD sink port state machine.
> + * It assumes the upper layer, i.e. the user of this driver, handles
> + * the PD message receiving and transmitting. The upper layer receives
> + * PD messages from the Source, queues them to us, and when processing
> + * the received message we'll call upper layer's transmitting function
> + * to send PD messages to the source.
> + * The sink port state machine is maintained in this driver but we also
> + * broadcast some important PD messages to upper layer as events.
> + *
> + * Copyright (C) 2016 Intel Corporation
> + *
> + * Author: Bin Gao
> + *
> + * 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
> +
> +#define MAKE_HEADER(port, header, msg, objs) \
> +do { \
> + header->type = msg; \
> + header->data_role = PD_DATA_ROLE_UFP; \
> + header->revision = port->pd_rev; \
> + header->power_role = PD_POWER_ROLE_SINK; \
> + header->id = roll_msg_id(port); \
> + header->nr_objs = objs; \
> + header->extended = PD_MSG_NOT_EXTENDED; \
> +} while (0)
> +
> +static struct pd_sink_port *sink_ports[MAX_NR_SINK_PORTS];
> +static int nr_ports;
> +
> +BLOCKING_NOTIFIER_HEAD(pd_sink_notifier_list);
> +
> +static char *state_strings[] = {
> + "WAIT_FOR_SOURCE_CAPABILITY",
> + "REQUEST_SENT",
> + "ACCEPT_RECEIVED",
> + "POWER_SUPPLY_READY",
> +};
> +
> +/* Control messages */
> +static char *cmsg_strings[] = {
> + "GOODCRC", /* 1 */
> + "GOTOMIN", /* 2 */
> + "ACCEPT", /* 3 */
> + "REJECT", /* 4 */
> + "PING", /* 5 */
> + "PS_RDY", /* 6 */
> + "GET_SRC_CAP", /* 7 */
> + "GET_SINK_CAP", /* 8 */
> + "DR_SWAP", /* 9 */
> + "PR_SWAP", /* 10 */
> + "VCONN_SWAP", /* 11 */
> + "WAIT", /* 12 */
> + "SOFT_RESET", /* 13 */
> + "RESERVED", /* 14 */
> + "RESERVED", /* 15 */
> + "NOT_SUPPORTED",/* 16 */
> + "GET_SOURCE_CAP_EXTENDED", /* 17 */
> + "GET_STATUS",
Re: [RFC PATCH 0/5] USB Audio Gadget refactoring
>> On Tue, May 24, 2016 at 2:50 AM, Ruslan Bilovol >> wrote: >>> it may break current usecase for some people And what are the benefits that justify breaking the kernel API? Regards, Clemens -- 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/5] usb: DT binding documentation for qoriq usb 2.0 controller
> -Original Message- > From: Peter Chen [mailto:hzpeterc...@gmail.com] > Sent: Friday, July 15, 2016 12:45 PM > To: Rajesh Bhagat > Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; > devicet...@vger.kernel.org; Peter Chen ; > gre...@linuxfoundation.org; kis...@ti.com; robh...@kernel.org; > shawn...@kernel.org; linux-arm-ker...@lists.infradead.org > Subject: Re: [PATCH v2 2/5] usb: DT binding documentation for qoriq usb 2.0 > controller > > On Tue, Jul 12, 2016 at 03:59:14AM +, Rajesh Bhagat wrote: > > > > > > > -Original Message- > > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > > Sent: Monday, July 11, 2016 12:19 PM > > > To: Rajesh Bhagat > > > Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; > > > devicet...@vger.kernel.org; Peter Chen ; > > > gre...@linuxfoundation.org; kis...@ti.com; robh...@kernel.org; > > > shawn...@kernel.org; linux-arm-ker...@lists.infradead.org > > > Subject: Re: [PATCH v2 2/5] usb: DT binding documentation for qoriq > > > usb 2.0 controller > > > > > > On Sat, Jul 09, 2016 at 10:00:53AM +0530, Rajesh Bhagat wrote: > > > > Describes the qoriq usb 2.0 controller driver binding, currently > > > > used for LS1021A and LS1012A platform. > > > > > > > > Signed-off-by: Rajesh Bhagat > > > > --- > > > > Changes in v2: > > > > - Adds DT binding documentation for qoriq usb 2.0 controller > > > > - Changed the compatible string to fsl,ci-qoriq-usb2 > > > > > > > > .../devicetree/bindings/usb/ci-hdrc-qoriq.txt | 34 > > > ++ > > > > 1 file changed, 34 insertions(+) > > > > create mode 100644 > > > > Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > > > > > > diff --git > > > > a/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > > b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > > new file mode 100644 > > > > index 000..8ad7306 > > > > --- /dev/null > > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt > > > > @@ -0,0 +1,34 @@ > > > > +* Freescale QorIQ SoC USB 2.0 Controllers > > > > + > > > > +Required properties: > > > > +- compatible: Should be "fsl,ci-qoriq-usb2" > > > > + Wherever applicable, the IP version of the USB controller > > > > +should > > > > + also be mentioned (for eg. fsl,ci-qoriq-usb2-vX.Y). > > > > + where, X.Y is IP version of USB controller. > > > > Hello Peter, > > > > > > > > Why you need to add IP version at compatible string? > > > Does it can't be read out from ID register of Identification Registers. > > > > > > > I agree. Will drop this controller version thing in DTS in v3. > > > > > > +- reg: Should contain registers location and length > > > > +- interrupts: Should contain controller interrupt > > > > +- phy-names: from the *Generic PHY* bindings > > > > +- phys: from the *Generic PHY* bindings > > > > +- clocks: clock provider specifier > > > > +- clock-names: shall be "usb2-clock" > > > > +Refer to clk/clock-bindings.txt for generic clock consumer > > > > +properties > > > > + > > > > +Recommended properties: > > > > +- dr_mode: One of "host" or "peripheral". > > > > > > Do you support dual-role? > > > > > > > Yes. We do support both host/peripheral mode. > > > Hello Peter, > I mean dual-role switch. If you support that, the dr_mode should be "otg". > For now, we don't support otg mode. Best Regards, Rajesh Bhagat > -- > > 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 1/2] usb: typec: Add USB Power Delivery sink port support
On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
> > +{
> > + unsigned long flags;
> > + struct pd_sink_port *port;
> > +
> > + if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
> > + pr_err("Invalid port number\n");
> > + return -EINVAL;
> > + }
> > +
> > + port = sink_ports[msg->port];
> > +
> > + spin_lock_irqsave(&port->rx_lock, flags);
> > + list_add_tail(&msg->list, &port->rx_list);
> > + spin_unlock_irqrestore(&port->rx_lock, flags);
> > +
> > + queue_work(port->rx_wq, &port->rx_work);
>
> can we really queue several messages at a time? It seems unfeasible to
> me. It's not like we can queue several power request in a role. Why do
> you need this workqueue? Why don't you process message here, in place?
A reset can come at any time.
Regards
Oliver
--
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/5] usb: serial: removing redundant __func__
On Fri, 2016-07-15 at 08:14 +0900, Greg KH wrote: > Doh, nevermind, that's what I get for writing emails early in the > morning while jet-lagged. > > Sorry for the noise, you are correct. No problem. Do you want me to resubmit? Regards Oliver -- 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/4] usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data
Now some usb controllers (such as dwc3 controller) need 'XHCI_SLOW_SUSPEND'
quirk when suspending the xhci, thus we need to add 'usb3_slow_suspend' member
in xhci platform data to support this.
Signed-off-by: Baolin Wang
---
drivers/usb/host/xhci-plat.c |3 +++
include/linux/usb/xhci_pdriver.h |3 +++
2 files changed, 6 insertions(+)
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index e2e2487..162f17c 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -250,6 +250,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
(pdata && pdata->usb3_lpm_capable))
xhci->quirks |= XHCI_LPM_SUPPORT;
+ if (pdata && pdata->usb3_slow_suspend)
+ xhci->quirks |= XHCI_SLOW_SUSPEND;
+
return 0;
diff --git a/include/linux/usb/xhci_pdriver.h b/include/linux/usb/xhci_pdriver.h
index 376654b..1276de1 100644
--- a/include/linux/usb/xhci_pdriver.h
+++ b/include/linux/usb/xhci_pdriver.h
@@ -18,10 +18,13 @@
*
* @usb3_lpm_capable: determines if this xhci platform supports USB3
* LPM capability
+ * @usb3_slow_suspend: determines if it need an extraordinary delay when
+ * suspending xhci.
*
*/
struct usb_xhci_pdata {
unsignedusb3_lpm_capable:1;
+ unsignedusb3_slow_suspend:1;
};
#endif /* __USB_CORE_XHCI_PDRIVER_H */
--
1.7.9.5
--
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/4] usb: dwc3: core: Move the mode setting to the right place
When dwc3 core enters into suspend mode, the system (especially for mobile
device) may power off the dwc3 controller for power saving, that will cause
dwc3 controller lost the mode operation when resuming dwc3 core.
Thus we can move the mode setting into dwc3_core_init() function to avoid this
issue.
Signed-off-by: Baolin Wang
---
drivers/usb/dwc3/core.c | 18 +++---
1 file changed, 15 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 9466431..1485480 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -666,6 +666,21 @@ static int dwc3_core_init(struct dwc3 *dwc)
goto err4;
}
+ switch (dwc->dr_mode) {
+ case USB_DR_MODE_PERIPHERAL:
+ dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
+ break;
+ case USB_DR_MODE_HOST:
+ dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
+ break;
+ case USB_DR_MODE_OTG:
+ dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
+ break;
+ default:
+ dev_warn(dwc->dev, "Unsupported mode %d\n", dwc->dr_mode);
+ break;
+ }
+
return 0;
err4:
@@ -763,7 +778,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
- dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_DEVICE);
ret = dwc3_gadget_init(dwc);
if (ret) {
if (ret != -EPROBE_DEFER)
@@ -772,7 +786,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
}
break;
case USB_DR_MODE_HOST:
- dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_HOST);
ret = dwc3_host_init(dwc);
if (ret) {
if (ret != -EPROBE_DEFER)
@@ -781,7 +794,6 @@ static int dwc3_core_init_mode(struct dwc3 *dwc)
}
break;
case USB_DR_MODE_OTG:
- dwc3_set_mode(dwc, DWC3_GCTL_PRTCAP_OTG);
ret = dwc3_host_init(dwc);
if (ret) {
if (ret != -EPROBE_DEFER)
--
1.7.9.5
--
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 0/4] Support dwc3 host suspend/resume
For mobile devices, they usually require very strict power management. Such as dwc3 controller, we should enter suspend mode when no cable plug in, then we can power off the dwc3 controller for saving power. Now dwc3 gadget can support suspend/resume well, but we also want to suspend/ resume the host when slave device is detached or attached. Baolin Wang (4): usb: host: xhci: Move the xhci quirks checking to the right place usb: host: xhci: Introduce one new 'usb3_slow_suspend' member for xhci private data usb: dwc3: core: Move the mode setting to the right place usb: dwc3: core: Support the dwc3 host suspend/resume drivers/usb/dwc3/Kconfig |7 +++ drivers/usb/dwc3/core.c | 43 ++ drivers/usb/dwc3/core.h | 15 + drivers/usb/dwc3/host.c | 32 drivers/usb/host/xhci-plat.c | 11 ++ include/linux/usb/xhci_pdriver.h |3 +++ 6 files changed, 103 insertions(+), 8 deletions(-) -- 1.7.9.5 -- 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 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
For some mobile devices with strict power management, we also want to suspend
the host when the slave is detached for power saving.
Thus we add the host suspend/resume functions to support this requirement, and
we also should enable the 'XHCI_SLOW_SUSPEND' quirk for extraordinary delay when
suspending the xhci.
Signed-off-by: Baolin Wang
---
drivers/usb/dwc3/Kconfig |7 +++
drivers/usb/dwc3/core.c | 25 -
drivers/usb/dwc3/core.h | 15 +++
drivers/usb/dwc3/host.c | 32
4 files changed, 78 insertions(+), 1 deletion(-)
diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
index a64ce1c..725d2bd 100644
--- a/drivers/usb/dwc3/Kconfig
+++ b/drivers/usb/dwc3/Kconfig
@@ -45,6 +45,13 @@ config USB_DWC3_DUAL_ROLE
This is the default mode of working of DWC3 controller where
both host and gadget features are enabled.
+config USB_DWC3_HOST_SUSPEND
+ bool "Choose if the host (xhci) can be suspend/resume"
+ depends on USB_DWC3_HOST=y || USB_DWC3_DUAL_ROLE=y
+ help
+ We can suspend the host when the slave is detached for power saving,
+ and resume the host when one slave is attached.
+
endchoice
comment "Platform Glue Driver Support"
diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 1485480..5140b4d 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -1103,15 +1103,27 @@ static int dwc3_remove(struct platform_device *pdev)
static int dwc3_suspend_common(struct dwc3 *dwc)
{
unsigned long flags;
+ int ret;
switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
+ spin_lock_irqsave(&dwc->lock, flags);
+ dwc3_gadget_suspend(dwc);
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ break;
case USB_DR_MODE_OTG:
+ ret = dwc3_host_suspend(dwc);
+ if (ret)
+ return ret;
+
spin_lock_irqsave(&dwc->lock, flags);
dwc3_gadget_suspend(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
break;
case USB_DR_MODE_HOST:
+ ret = dwc3_host_suspend(dwc);
+ if (ret)
+ return ret;
default:
/* do nothing */
break;
@@ -1133,12 +1145,23 @@ static int dwc3_resume_common(struct dwc3 *dwc)
switch (dwc->dr_mode) {
case USB_DR_MODE_PERIPHERAL:
+ spin_lock_irqsave(&dwc->lock, flags);
+ dwc3_gadget_resume(dwc);
+ spin_unlock_irqrestore(&dwc->lock, flags);
+ break;
case USB_DR_MODE_OTG:
+ ret = dwc3_host_resume(dwc);
+ if (ret)
+ return ret;
+
spin_lock_irqsave(&dwc->lock, flags);
dwc3_gadget_resume(dwc);
spin_unlock_irqrestore(&dwc->lock, flags);
- /* FALLTHROUGH */
+ break;
case USB_DR_MODE_HOST:
+ ret = dwc3_host_resume(dwc);
+ if (ret)
+ return ret;
default:
/* do nothing */
break;
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45d6de5..0ba203e 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -1177,4 +1177,19 @@ static inline void dwc3_ulpi_exit(struct dwc3 *dwc)
{ }
#endif
+#if IS_ENABLED(USB_DWC3_HOST_SUSPEND)
+int dwc3_host_suspend(struct dwc3 *dwc);
+int dwc3_host_resume(struct dwc3 *dwc);
+#else
+static inline int dwc3_host_suspend(struct dwc3 *dwc)
+{
+ return 0;
+}
+
+static inline int dwc3_host_resume(struct dwc3 *dwc)
+{
+ return 0;
+}
+#endif
+
#endif /* __DRIVERS_USB_DWC3_CORE_H */
diff --git a/drivers/usb/dwc3/host.c b/drivers/usb/dwc3/host.c
index 2e960ed..2ec3eff 100644
--- a/drivers/usb/dwc3/host.c
+++ b/drivers/usb/dwc3/host.c
@@ -17,8 +17,11 @@
#include
#include
+#include
+#include
#include "core.h"
+#include "../host/xhci.h"
int dwc3_host_init(struct dwc3 *dwc)
{
@@ -91,6 +94,8 @@ int dwc3_host_init(struct dwc3 *dwc)
memset(&pdata, 0, sizeof(pdata));
pdata.usb3_lpm_capable = dwc->usb3_lpm_capable;
+ /* dwc3 controller need an extraordinary delay when suspending xhci. */
+ pdata.usb3_slow_suspend = 1;
ret = platform_device_add_data(xhci, &pdata, sizeof(pdata));
if (ret) {
@@ -128,3 +133,30 @@ void dwc3_host_exit(struct dwc3 *dwc)
dev_name(&dwc->xhci->dev));
platform_device_unregister(dwc->xhci);
}
+
+int dwc3_host_suspend(struct dwc3 *dwc)
+{
+ struct usb_hcd *hcd = dev_get_drvdata(&dwc->xhci->dev);
+ struct xhci_hcd *xhci = hcd_to_xhci(hcd);
+ int ret, cnt = 20;
+
+try_again:
+/* We should wait for xhci bus has been into suspend mode firstly. */
+
[PATCH 1/4] usb: host: xhci: Move the xhci quirks checking to the right place
It will reset the xhci quirks in xhci_gen_setup() function when xhci try to add one hcd, thus we need to move the XHCI_LPM_SUPPORT quirk checking after adding hcd. Signed-off-by: Baolin Wang --- drivers/usb/host/xhci-plat.c |8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c index 1f3f981..e2e2487 100644 --- a/drivers/usb/host/xhci-plat.c +++ b/drivers/usb/host/xhci-plat.c @@ -223,10 +223,6 @@ static int xhci_plat_probe(struct platform_device *pdev) goto disable_clk; } - if ((node && of_property_read_bool(node, "usb3-lpm-capable")) || - (pdata && pdata->usb3_lpm_capable)) - xhci->quirks |= XHCI_LPM_SUPPORT; - if (HCC_MAX_PSA(xhci->hcc_params) >= 4) xhci->shared_hcd->can_do_streams = 1; @@ -250,6 +246,10 @@ static int xhci_plat_probe(struct platform_device *pdev) if (ret) goto dealloc_usb2_hcd; + if ((node && of_property_read_bool(node, "usb3-lpm-capable")) || + (pdata && pdata->usb3_lpm_capable)) + xhci->quirks |= XHCI_LPM_SUPPORT; + return 0; -- 1.7.9.5 -- 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: chipidea: usbmisc: set over current polarity for imx6 and imx7
On Fri, Jul 15, 2016 at 07:38:23AM +, Jun Li wrote: > Hi, > > -Original Message- > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > Sent: Friday, July 15, 2016 3:02 PM > > To: Jun Li > > Cc: Peter Chen ; linux-usb@vger.kernel.org > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > > for imx6 and imx7 > > > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > > As all usb power supply use low active for over current flag on imx6 > > > imx7 boards, and the default register setting(0) is for high active, > > > this patch is to correct it. > > > > > > > We may can't ensure all USB power switch chips work like that, I suggest > > you making this as default. > > > > I will change the commit log like below if you are ok. > > > > As most of all usb power switch chips use active-low for over current flag, > > but the default register setting(0) is for active-high at imx6/imx7, this > > patch changes default value as active-low. > > Looks better, I am okay with it except a tiny comment > :%s/As most of all usb power/As most of usb power > > Li Jun > > Since we can't break current default behaviour, but with your patch, the imx6sx sdb board creates over current event. I think you may need to introduce a flag for OC polarity, and use it for exist platforms if necessary. It can narrow down affect only on single platform. For new platforms, you can change SoC values by default. -- 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 v2 2/5] usb: serial: removing redundant __func__
On Fri, Jul 15, 2016 at 10:45:36AM +0200, Oliver Neukum wrote: > On Fri, 2016-07-15 at 08:14 +0900, Greg KH wrote: > > Doh, nevermind, that's what I get for writing emails early in the > > morning while jet-lagged. > > > > Sorry for the noise, you are correct. > > No problem. Do you want me to resubmit? No need. I know a func-prefix is technically redundant, but the two alternatives are not equivalent in my opinion. I find the dev_dbg way of slapping the function name at the start of every debug message makes for unnecessarily hard-to-parse logs. Here's an example: [ 50.500823] tty ttyUSB0: serial_open [ 50.505401] ftdi_sio ttyUSB0: Setting CS8 [ 50.513275] ftdi_sio ttyUSB0: get_ftdi_divisor - tty_get_baud_rate reports speed 9600 [ 50.521728] ftdi_sio ttyUSB0: get_ftdi_divisor - Baud rate set to 9600 (divisor 0x4138) on chip FT232RL [ 50.532989] ftdi_sio ttyUSB0: ftdi_set_termios Turning off hardware flow control [ 50.543029] ftdi_sio ttyUSB0: update_mctrl - DTR HIGH, RTS HIGH [ 50.549926] tty ttyUSB0: serial_write_room [ 50.555328] ftdi_sio ttyUSB0: usb_serial_generic_write_room - returns 4096 [ 50.563354] tty ttyUSB0: serial_write - 3 byte(s) [ 50.569091] tty ttyUSB0: serial_write_room [ 50.574127] ftdi_sio ttyUSB0: usb_serial_generic_write_room - returns 4096 [ 50.582000] tty ttyUSB0: serial_write - 2 byte(s) [ 50.587799] tty ttyUSB0: serial_close [ 50.592468] tty ttyUSB0: serial_chars_in_buffer [ 50.597930] ftdi_sio ttyUSB0: usb_serial_generic_chars_in_buffer - returns 0 [ 50.605987] tty ttyUSB0: serial_wait_until_sent [ 50.611419] ftdi_sio ttyUSB0: usb_serial_generic_wait_until_sent - timeout = 3 ms, period = 1 ms [ 50.622314] ftdi_sio ttyUSB0: ftdi_get_modem_status - 0x0160 [ 50.630096] ftdi_sio ttyUSB0: update_mctrl - DTR LOW, RTS LOW [ 50.640777] tty ttyUSB0: serial_cleanup Now with dev_dbg we instead get (with the %s and __func__ dropped): [ 116.419036] serial_open: tty ttyUSB0: serial_open [ 116.426849] ftdi_set_termios: ftdi_sio ttyUSB0: Setting CS8 [ 116.434295] get_ftdi_divisor: ftdi_sio ttyUSB0: tty_get_baud_rate reports speed 9600 [ 116.445037] get_ftdi_divisor: ftdi_sio ttyUSB0: Baud rate set to 9600 (divisor 0x4138) on chip FT232RL [ 116.457641] ftdi_set_termios: ftdi_sio ttyUSB0: Turning off hardware flow control [ 116.468780] update_mctrl: ftdi_sio ttyUSB0: DTR HIGH, RTS HIGH [ 116.476898] serial_write_room: tty ttyUSB0: [ 116.484039] usb_serial_generic_write_room: ftdi_sio ttyUSB0: returns 4096 [ 116.494812] serial_write: tty ttyUSB0: 3 byte(s) [ 116.501800] serial_write_room: tty ttyUSB0: [ 116.508697] usb_serial_generic_write_room: ftdi_sio ttyUSB0: returns 4096 [ 116.519470] serial_write: tty ttyUSB0: 2 byte(s) [ 116.526702] serial_close: tty ttyUSB0: [ 116.532257] serial_chars_in_buffer: tty ttyUSB0: [ 116.540039] usb_serial_generic_chars_in_buffer: ftdi_sio ttyUSB0: returns 0 [ 116.551452] serial_wait_until_sent: tty ttyUSB0: [ 116.559051] usb_serial_generic_wait_until_sent: ftdi_sio ttyUSB0: timeout = 3 ms, period = 1 ms [ 116.573303] ftdi_get_modem_status: ftdi_sio ttyUSB0: 0x0160 [ 116.583282] update_mctrl: ftdi_sio ttyUSB0: DTR LOW, RTS LOW [ 116.595245] serial_cleanup: tty ttyUSB0: which I find much harder to parse. We don't always enforce a common prefix for function names, making grouping related functions even harder. Usually, what is printed in a debug message only makes sense in combination with the function name (e.g. serial_write: 2 bytes), but now that connection is also less clear. Also consider what the above log would look like if you have more than one device active. Trying to keep the independent traces separate by simply looking at the logs becomes almost impossible. For that reason I also very much prefer having the device name at the start of the message. Another reason is that the device names are of fixed length (for a device and usually class of devices), whereas functions names vary greatly and renders a more jagged "left column", after which device names and message follow. I should probably try to argue for the function-name to be moved in dev_dbg, but until that's at least been discussed further, I prefer to keep the (redundant) __func__s as they are. Thanks, Johan -- 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 0/1] usb: add HCD providers
On 15 July 2016 at 08:22, Peter Chen wrote:
> On Fri, Jul 15, 2016 at 07:48:11AM +0200, Rafał Miłecki wrote:
>> >> > Below I supply another thought, please check if it is feasible.
>> >> > In below design, you don't need to change any usb codes.
>> >> >
>> >> > dts:
>> >> >
>> >> > led_1 {
>> >> > led_gpio_1;
>> >> > usb_port = &ohci_port0, &ehci_port1;
>> >> > }
>> >> >
>> >> > led_2 {
>> >> > led_gpio_2;
>> >> > usb_port = &xhci_port0, &xhci_port1;
>> >> > }
>> >> >
>> >> > ohci@1000 {
>> >> > compatible = "generic-ohci";
>> >> > reg = <0x1000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > ohci_port0: port@0 {
>> >> > reg = <0>;
>> >> > };
>> >> >
>> >> > ohci_port1: port@1 {
>> >> > reg = <1>;
>> >> > };
>> >> > };
>> >> >
>> >> > ehci@2000 {
>> >> > compatible = "generic-ehci";
>> >> > reg = <0x2000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > ehci_port0: port@0 {
>> >> > reg = <0>;
>> >> > };
>> >> >
>> >> > ehci_port1: port@1 {
>> >> > reg = <1>;
>> >> > };
>> >> > };
>> >> >
>> >> > xhci@3000 {
>> >> > compatible = "generic-xhci";
>> >> > reg = <0x3000 0x1000>;
>> >> > interrupts = ;
>> >> >
>> >> > /* for xhci, port 0 - [N-1] is USB3, N - [M-1] is USB2/1.
>> >> > * The port 0 and port N is the same physical port
>> >> > */
>> >> > xhci_port0: port@0 {
>> >> > reg = <0>;
>> >> > };
>> >> >
>> >> > xhci_port1: port@1 {
>> >> > reg = <1>;
>> >> > };
>> >> >
>> >> > };
>> >> >
>> >> > At code, compare the usb_device's device_node at usbport_trig_notify
>> >> > if it is at led_1's usb device list, light on it.
>> >>
>> >> This is quite interesting idea, thanks!
>> >>
>> >> So I got following checking code:
>> >>
>> >> count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> >> for (i = 0; i < count; i++) {
>> >> of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> >> of_property_read_u32(args.np, "reg", &port);
>> >> if (args.np->parent == usb_dev->bus->controller->of_node &&
>> >> port == usb_dev->portnum) {
>> >> of_node_put(args.np);
>> >> return true;
>> >> }
>> >> of_node_put(args.np);
>> >> }
>> >> return false;
>> >
>> > No, compares the USB port directly.
>> >
>> > count = of_count_phandle_with_args(np, "usb-ports", NULL);
>> > for (i = 0; i < count; i++) {
>> > of_parse_phandle_with_args(np, "usb-ports", NULL, i, &args);
>> > if (args.np == usb_dev->dev.of_node)
>> > of_node_put(args.np);
>> > return true;
>> > }
>> > of_node_put(args.np);
>> > }
>> > return false;
>>
>> If we mean to use usb_dev->dev.of_node I *need* to modify USB
>> subsystem, since this pointer is never being set by the current code.
>>
>> [ 71.410505] usb 1-1: new high-speed USB device number 2 using
>> ehci-platform
>> [ 71.564928] [usbport_trig_notify] usb_dev:c6ac1000 &usb_dev->dev:c6ac1068
>> [ 71.579874] [usbport_trig_notify] dev_name(&usb_dev->dev):1-1
>> [ 71.586580] [usbport_trig_notify] usb_dev->dev.of_node: (null)
>>
>> Or am I missing something?
>>
>
> You may need below patches:
>
> commit 69bec725985324e79b1c47ea287815ac4ddb0521
> Author: Peter Chen
> Date: Fri Feb 19 17:26:15 2016 +0800
>
> USB: core: let USB device know device node
>
> commit 7222c832254a75dcd67d683df75753d4a4e125bb
> Author: Nicolai Stange
> Date: Thu Mar 17 23:53:02 2016 +0100
>
> usb/core: usb_alloc_dev(): fix setting of ->portnum
F*k, I just implemented the same thing on my own and I was going to
submit it :/ Thanks for pointing these commits.
>> >> This works, but I see 3 more problems:
>> >>
>> >> 1) How to access list of available USB devices during activation?
>> >
>> > You mean during LED activation? eg your usbport_trig_activate?
>> > Why do you need it?
>>
>> Yes, I mean usbport_trig_activate. If user plugs in USB device and
>> *then* activates this trigger, we want to set a proper initial state.
>> We can't only depend on USB_DEVICE_ADD.
>>
>
> Oh, I see, I asked it before.
>
> Either you need to register USB notifier before activation
It won't work if someone builds usbport as a module and loads it after
connecting USB devices.
> Or you need to implement something like usb_node_to_dev
> eg: like usb_for_each_dev. If device's state is USB_STATE_CONFIGURED
> this USB device is available.
I think I'll need that.
>> >> 2) What about support for non-DT platforms in usbport driver? Should I
>> >> still allow specifying ports manually? Are you OK with that?
>> >
>> > I am afraid I still don't know how to do it for non-DT platforms.
>> > You can show your design.
>>
>> Please take a look at
>> [PATCH] leds: trigger: Introduce an USB port trigger
>> https://lkml.org/l
Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
Hi,
Oliver Neukum writes:
> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>> > +{
>> > + unsigned long flags;
>> > + struct pd_sink_port *port;
>> > +
>> > + if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>> > + pr_err("Invalid port number\n");
>> > + return -EINVAL;
>> > + }
>> > +
>> > + port = sink_ports[msg->port];
>> > +
>> > + spin_lock_irqsave(&port->rx_lock, flags);
>> > + list_add_tail(&msg->list, &port->rx_list);
>> > + spin_unlock_irqrestore(&port->rx_lock, flags);
>> > +
>> > + queue_work(port->rx_wq, &port->rx_work);
>>
>> can we really queue several messages at a time? It seems unfeasible to
>> me. It's not like we can queue several power request in a role. Why do
>> you need this workqueue? Why don't you process message here, in place?
>
> A reset can come at any time.
right, but that's not how this is being used. IMHO, rx_work is a
misnomer. If you look at how typec_wcove (patch 2 in this series) uses
it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
message that was *already* received. We can't have two replies, right?
In any case, this is a minor problem.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
Hi,
Bin Gao writes:
> +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> +{
> + pr_info("sink port %d: %s message %s %s\n", port,
> + is_cmsg ? "Control" : "Data",
> + msg_to_string(is_cmsg, msg),
> + recv ? "received" : "sent(wait GOODCRC)");
> +}
this is problematic. By default, we're all using 115200 8N1 baud
rate. This message alone prints anywhere from 50 to 100 characters (I
didn't really count properly, these are rough numbers), and that takes:
n50chars_time = 50 / (115200 / 10) = 4.3ms
n100chars_time = 100 / (115200 / 10) = 8.6ms
Considering you have 30ms to reply with Power Request after GoodCRC, and
considering you're printing several of these messages, they become
really expensive and eat up valuable time from tSenderReply.
This should really be a pr_debug() or, better yet, a tracepoint.
--
balbi
signature.asc
Description: PGP signature
USB headset doesn't work with USB 3.0 port in kernel 2.6.37 - xhci_hcd error: "ERROR Transfer event TRB DMA ptr not part of current TD"
Hi, I'm working on a project to support USB headset on a device containing TI chipset DM8168 SoC. It runs Linux kernel version 2.6.37. The headset works fine with proper audio when connected to USB 2.0 ports which uses musb host controller driver. However, when I connect the headset to USB 3.0 ports, which use xhci host controller driver, it sometimes throws the following error repeatedly. --- xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD --- After this error, most of the PCM read/writes fail and xhci starts throwing other "ep ring" errors as shown below. --- cannot submit datapipe for urb 2, error -22: internal error xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD cannot submit datapipe for urb 2, error -22: internal error xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD cannot submit datapipe for urb 2, error -22: internal error xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD cannot submit datapipe for urb 2, error -22: internal error xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD cannot submit datapipe for urb 2, error -22: internal error xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD xhci_hcd :09:00.0: ERROR Transfer event TRB DMA ptr not part of current TD xhci_hcd :09:00.0: ERROR no room on ep ring cannot submit datapipe for urb 0, error -12: unknown error xhci_hcd :09:00.0: ERROR no room on ep ring cannot submit datapipe for urb 0, error -12: unknown error xhci_hcd :09:00.0: ERROR no room on ep ring --- I suspect this could be a bug fixed by the patch titled "xhci: Fix bug after deq ptr set to link TRB" which was submitted to kernel in v3.6. Commit id is 50d0206fcaea3e736f912fd5b00ec6233fb4ce44. In the commit message of this patch, it is mentioned that this fix has to be backported to kernels as old as 2.6.31 and a separate patch will be created for older kernels. Following is the excerpt from the commit message of this patch: --- This patch should be backported to kernels as old as 2.6.31. A separate patch will be created for kernels older than 3.4, since inc_deq was modified in 3.4 and this patch will not apply. --- Please let me know if a patch is available for this fix which can be applied to kernel 2.6.37. Appreciate your help on this. Thanks, Goutham BG -- 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/5] usb: serial: removing redundant __func__
On Fri, 2016-07-15 at 11:35 +0200, Johan Hovold wrote: > which I find much harder to parse. We don't always enforce a common > prefix for function names, making grouping related functions even > harder. > > Usually, what is printed in a debug message only makes sense in > combination with the function name (e.g. serial_write: 2 bytes), but > now > that connection is also less clear. > > Also consider what the above log would look like if you have more than > one device active. Trying to keep the independent traces separate by > simply looking at the logs becomes almost impossible. For that reason > I > also very much prefer having the device name at the start of the > message. You are right. But the correct remedy would be to fix dynamic debugging. Indeed printing the function before the module is braindead. Regards Oliver -- 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 00/36] usb: serial: ti_usb_3410_5052: clean driver
On Thu, May 12, 2016 at 10:48:32AM +0200, Mathieu OTHACEHE wrote: > Hi, > > The now reverted mxu11x0 turned out to be a copy of ti_usb_3410_5052 driver. > This aim of this serie is to apply all of the cleanups we did in mxu11x0 to > ti_usb_3410_5052. I apologise for the late review of this one. I've applied the first four now (with some tweaks) and will comment on the rest. Thanks, Johan -- 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 03/36] usb: serial: ti_usb_3410_5052: Remove ti_usb_3410_5052.h
On Thu, May 12, 2016 at 10:48:35AM +0200, Mathieu OTHACEHE wrote: > The definitions in ti_usb_3410_5052.h are only used in > ti_usb_3410_5052.c. > The content of the header is copied in ti_usb_3410_5052.c. > > Also correct a typo in macro TI_PIPE_MODE_CONTINOUS. > > Signed-off-by: Mathieu OTHACEHE > --- > drivers/usb/serial/ti_usb_3410_5052.c | 237 > +- > 1 file changed, 235 insertions(+), 2 deletions(-) You forgot actually remove the now unused header file. Fix it up before applying. Thanks, Johan -- 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 04/36] usb: serial: ti_usb_3410_5052: Use inline functions rather than macro
On Thu, May 12, 2016 at 03:00:39PM +0300, Sergei Shtylyov wrote:
> Hello.
>
> On 5/12/2016 11:48 AM, Mathieu OTHACEHE wrote:
>
> > Inline functions are preferable to macros resembling functions.
> >
> > Signed-off-by: Mathieu OTHACEHE
> > ---
> > drivers/usb/serial/ti_usb_3410_5052.c | 16
> > 1 file changed, 12 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> > b/drivers/usb/serial/ti_usb_3410_5052.c
> > index 6002e8b..2fc3ea1 100644
> > --- a/drivers/usb/serial/ti_usb_3410_5052.c
> > +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> > @@ -248,8 +248,16 @@ struct ti_interrupt {
> > } __packed;
> >
> > /* Interrupt codes */
> > -#define TI_GET_PORT_FROM_CODE(c) (((c) >> 4) - 3)
> > -#define TI_GET_FUNC_FROM_CODE(c) ((c) & 0x0f)
> > +static inline int ti_get_port_from_code(unsigned char code)
> > +{
> > + return (code >> 4) - 3;
> > +}
> > +
> > +static inline int ti_get_func_from_code(unsigned char code)
> > +{
> > + return code & 0x0f;
> > +}
> > +
>
> We don't specify *inline* in the .c files (only in .h), letting gcc
> figure
> it out.
Indeed. I dropped the inline keywords and also moved the functions above
the interrupt-in completion handler, which is the only place they are
used.
Thanks,
Johan
--
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 05/36] usb: serial: ti_usb_3410_5052: Remove unused data structures
On Thu, May 12, 2016 at 10:48:37AM +0200, Mathieu OTHACEHE wrote:
> ti_read_data_request, ti_read_data_bytes and ti_interrupt are unused.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 21 -
> 1 file changed, 21 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 2fc3ea1..af4e145 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -226,27 +226,6 @@ struct ti_write_data_bytes {
> __u8bData[0];
> } __packed;
>
> -struct ti_read_data_request {
> - __u8bAddrType;
> - __u8bDataType;
> - __u8bDataCounter;
> - __be16 wBaseAddrHi;
> - __be16 wBaseAddrLo;
> -} __packed;
> -
> -struct ti_read_data_bytes {
> - __u8bCmdCode;
> - __u8bModuleId;
> - __u8bErrorCode;
> - __u8bData[0];
> -} __packed;
> -
> -/* Interrupt struct */
> -struct ti_interrupt {
> - __u8bICode;
> - __u8bIInfo;
> -} __packed;
I'm not sure this is a good idea as we lose protocol information this
way. The ti_interrupt message is in fact used in the interrupt
completion callback by accessing bICode through data[0] for example.
Why not put the structs to use instead?
Thanks,
Johan
--
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 06/36] usb: serial: ti_usb_3410_5052: Do not use __uX types
On Thu, May 12, 2016 at 10:48:38AM +0200, Mathieu OTHACEHE wrote:
> __uX types should only be used for user-space interactions.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 66
> ++-
> 1 file changed, 34 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index af4e145..164e07b 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -179,23 +179,23 @@
>
> /* Config struct */
> struct ti_uart_config {
> - __u16 wBaudRate;
> - __u16 wFlags;
> - __u8bDataBits;
> - __u8bParity;
> - __u8bStopBits;
> + __be16 wBaudRate;
> + __be16 wFlags;
While I have nothing against replacing the __u, this makes me
worried. How, if at all, is the endianess of these fields handled in the
code?
Ok, I see now it's using cpu_to_be16s after writing host-endian data
into it. Could you clean that to use cpu_to_be16 instead (you may need
to update an error message too)?
Note that the wBaudRate assignment was still using __u16 after this
patch.
> + u8 bDataBits;
> + u8 bParity;
> + u8 bStopBits;
> charcXon;
> charcXoff;
> - __u8bUartMode;
> + u8 bUartMode;
> } __packed;
> static int ti_write_byte(struct usb_serial_port *port,
> - struct ti_device *tdev, unsigned long addr,
> - __u8 mask, __u8 byte)
> + struct ti_device *tdev, unsigned long addr,
> + u8 mask, u8 byte)
> {
> int status;
> unsigned int size;
> @@ -1659,10 +1661,10 @@ static int ti_do_download(struct usb_device *dev, int
> pipe,
> int len;
>
> for (pos = sizeof(struct ti_firmware_header); pos < size; pos++)
> - cs = (__u8)(cs + buffer[pos]);
> + cs = (u8)(cs + buffer[pos]);
>
> header = (struct ti_firmware_header *)buffer;
> - header->wLength = cpu_to_le16((__u16)(size
> + header->wLength = cpu_to_le16((u16)(size
> - sizeof(struct ti_firmware_header)));
Cast not needed.
Thanks,
Johan
--
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: USB headset doesn't work with USB 3.0 port in kernel 2.6.37 - xhci_hcd error: "ERROR Transfer event TRB DMA ptr not part of current TD"
On Fri, Jul 15, 2016 at 10:38:04AM +, Goutham BG wrote: > Hi, > > I'm working on a project to support USB headset on a device containing TI > chipset DM8168 SoC. It runs Linux kernel version 2.6.37. Wow that's an obsolete and very very old kernel version, many hundreds of thousands of changes old. Please work with your vendor who is forcing you to use such a kernel version, as you are already paying for that support from them. There is nothing that we can do here to help you with this, unless you can reproduce this on a 4.6 kernel release. good luck! 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 07/36] usb: serial: ti_usb_3410_5052: Remove closing_wait module parameter
On Thu, May 12, 2016 at 10:48:39AM +0200, Mathieu OTHACEHE wrote:
> Closing wait delay is configurable per device using TIOCSSERIAL.
Please try to make the commit messages self-contained and not rely on
the commit summary to make sense.
> Also initialise tty_port closing_wait in port_probe with default value.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 10 +++---
> 1 file changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 164e07b..1860a5a 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -347,8 +347,6 @@ static int ti_write_byte(struct usb_serial_port *port,
> struct ti_device *tdev,
>
> static int ti_download_firmware(struct ti_device *tdev);
>
> -static int closing_wait = TI_DEFAULT_CLOSING_WAIT;
> -
> static const struct usb_device_id ti_id_table_3410[] = {
> { USB_DEVICE(TI_VENDOR_ID, TI_3410_PRODUCT_ID) },
> { USB_DEVICE(TI_VENDOR_ID, TI_3410_EZ430_ID) },
> @@ -498,10 +496,6 @@ MODULE_FIRMWARE("moxa/moxa-1131.fw");
> MODULE_FIRMWARE("moxa/moxa-1150.fw");
> MODULE_FIRMWARE("moxa/moxa-1151.fw");
>
> -module_param(closing_wait, int, S_IRUGO | S_IWUSR);
> -MODULE_PARM_DESC(closing_wait,
> -"Maximum wait for data to drain in close, in .01 secs, default is 4000");
> -
As there may systems out there relying on this module-parameter still,
I'm not sure it's a good idea to simply remove it.
> MODULE_DEVICE_TABLE(usb, ti_id_table_combined);
>
> module_usb_serial_driver(serial_drivers, ti_id_table_combined);
> @@ -602,7 +596,7 @@ static int ti_port_probe(struct usb_serial_port *port)
> tport->tp_uart_base_addr = TI_UART1_BASE_ADDR;
> else
> tport->tp_uart_base_addr = TI_UART2_BASE_ADDR;
> - port->port.closing_wait = msecs_to_jiffies(10 * closing_wait);
> +
> tport->tp_port = port;
> tport->tp_tdev = usb_get_serial_data(port->serial);
>
> @@ -613,6 +607,8 @@ static int ti_port_probe(struct usb_serial_port *port)
>
> usb_set_serial_port_data(port, tport);
>
> + port->port.closing_wait =
> + msecs_to_jiffies(TI_DEFAULT_CLOSING_WAIT * 10);
Moving initialisation here makes sense though.
> port->port.drain_delay = 3;
>
> return 0;
Thanks,
Johan
--
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: typec: Add USB Power Delivery sink port support
On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>
> Hi,
>
> Bin Gao writes:
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > + pr_info("sink port %d: %s message %s %s\n", port,
> > + is_cmsg ? "Control" : "Data",
> > + msg_to_string(is_cmsg, msg),
> > +recv ? "received" : "sent(wait GOODCRC)");
> > +}
>
> this is problematic. By default, we're all using 115200 8N1 baud
> rate. This message alone prints anywhere from 50 to 100 characters (I
> didn't really count properly, these are rough numbers), and that takes:
>
> n50chars_time = 50 / (115200 / 10) = 4.3ms
> n100chars_time = 100 / (115200 / 10) = 8.6ms
>
> Considering you have 30ms to reply with Power Request after GoodCRC, and
> considering you're printing several of these messages, they become
> really expensive and eat up valuable time from tSenderReply.
printk() should be async, so it shouldn't be that big of a deal.
What is wrong is that this isn't using dev_info().
> This should really be a pr_debug() or, better yet, a tracepoint.
Yes, that would be best (dev_dbg() or a tracepoint.)
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 08/36] usb: serial: ti_usb_3410_5052: Remove useless dev_dbg messages
On Thu, May 12, 2016 at 10:48:40AM +0200, Mathieu OTHACEHE wrote:
> Remove useless or redundant dev_dbg messages.
> Fix debug-message typos.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 28 +---
> 1 file changed, 9 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 1860a5a..88aacf5 100644
> @@ -846,7 +837,6 @@ static int ti_write_room(struct tty_struct *tty)
> room = kfifo_avail(&port->write_fifo);
> spin_unlock_irqrestore(&tport->tp_lock, flags);
>
> - dev_dbg(&port->dev, "%s - returns %d\n", __func__, room);
> return room;
> }
>
> @@ -865,7 +855,6 @@ static int ti_chars_in_buffer(struct tty_struct *tty)
> chars = kfifo_len(&port->write_fifo);
> spin_unlock_irqrestore(&tport->tp_lock, flags);
>
> - dev_dbg(&port->dev, "%s - returns %d\n", __func__, chars);
> return chars;
> }
While I agree with most of this, the above two can actually be useful to
keep.
> @@ -924,11 +913,9 @@ static int ti_ioctl(struct tty_struct *tty,
>
> switch (cmd) {
> case TIOCGSERIAL:
> - dev_dbg(&port->dev, "%s - TIOCGSERIAL\n", __func__);
> return ti_get_serial_info(tport,
> (struct serial_struct __user *)arg);
> case TIOCSSERIAL:
> - dev_dbg(&port->dev, "%s - TIOCSSERIAL\n", __func__);
> return ti_set_serial_info(tty, tport,
> (struct serial_struct __user *)arg);
> }
> @@ -950,9 +937,15 @@ static void ti_set_termios(struct tty_struct *tty,
> cflag = tty->termios.c_cflag;
> iflag = tty->termios.c_iflag;
>
> - dev_dbg(&port->dev, "%s - cflag %08x, iflag %08x\n", __func__, cflag,
> iflag);
> - dev_dbg(&port->dev, "%s - old clfag %08x, old iflag %08x\n", __func__,
> - old_termios->c_cflag, old_termios->c_iflag);
> + dev_dbg(&port->dev,
> + "%s - cflag 0x%08x, iflag 0x%08x\n", __func__, cflag, iflag);
> +
> + if (old_termios) {
> + dev_dbg(&port->dev, "%s - old clfag 0x%08x, old iflag 0x%08x\n",
> + __func__,
> + old_termios->c_cflag,
> + old_termios->c_iflag);
> + }
old_termios is not used in the function, why not drop it instead and fix
up the call sites?
Note that it will currently never be NULL.
>
> if (tport == NULL)
> return;
> @@ -1140,8 +1133,6 @@ static void ti_break(struct tty_struct *tty, int
> break_state)
> struct ti_port *tport = usb_get_serial_port_data(port);
> int status;
>
> - dev_dbg(&port->dev, "%s - state = %d\n", __func__, break_state);
I'd keep this one as well.
> -
> if (tport == NULL)
> return;
>
> @@ -1220,7 +1211,6 @@ static void ti_interrupt_callback(struct urb *urb)
>
> case TI_CODE_MODEM_STATUS:
> msr = data[1];
> - dev_dbg(dev, "%s - port %d, msr 0x%02X\n", __func__,
> port_number, msr);
> ti_handle_new_msr(tport, msr);
> break;
And this one.
Thanks,
Johan
--
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/5] usb: serial: removing redundant __func__
On Fri, Jul 15, 2016 at 12:40:45PM +0200, Oliver Neukum wrote: > On Fri, 2016-07-15 at 11:35 +0200, Johan Hovold wrote: > > which I find much harder to parse. We don't always enforce a common > > prefix for function names, making grouping related functions even > > harder. > > > > Usually, what is printed in a debug message only makes sense in > > combination with the function name (e.g. serial_write: 2 bytes), but > > now > > that connection is also less clear. > > > > Also consider what the above log would look like if you have more than > > one device active. Trying to keep the independent traces separate by > > simply looking at the logs becomes almost impossible. For that reason > > I > > also very much prefer having the device name at the start of the > > message. > > You are right. But the correct remedy would be to fix dynamic debugging. > Indeed printing the function before the module is braindead. Yes, let's fix that instead, let me knock one up right now... 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
[patch] usb: gadget: fsl_qe_udc: signedness bug in qe_get_frame()
We can't assign -EINVAL to a u16.
Fixes: 3948f0e0c999 ('usb: add Freescale QE/CPM USB peripheral controller
driver')
Signed-off-by: Dan Carpenter
diff --git a/drivers/usb/gadget/udc/fsl_qe_udc.c
b/drivers/usb/gadget/udc/fsl_qe_udc.c
index 93d28cb..901366f 100644
--- a/drivers/usb/gadget/udc/fsl_qe_udc.c
+++ b/drivers/usb/gadget/udc/fsl_qe_udc.c
@@ -1878,11 +1878,8 @@ static int qe_get_frame(struct usb_gadget *gadget)
tmp = in_be16(&udc->usb_param->frame_n);
if (tmp & 0x8000)
- tmp = tmp & 0x07ff;
- else
- tmp = -EINVAL;
-
- return (int)tmp;
+ return tmp & 0x07ff;
+ return -EINVAL;
}
static int fsl_qe_start(struct usb_gadget *gadget,
--
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 12/36] usb: serial: ti_usb_3410_5052: Use generic read/write callbacks
On Thu, May 12, 2016 at 10:48:44AM +0200, Mathieu OTHACEHE wrote:
> Remove read_bulk_callback, write_bulk_callback, write, write_room,
> chars_in_buffer, throttle and unthrottle callbacks who uselessly
> reimplements generic functions.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 315
> --
> 1 file changed, 315 deletions(-)
>
> -static void ti_throttle(struct tty_struct *tty)
> -{
> - struct usb_serial_port *port = tty->driver_data;
> - struct ti_port *tport = usb_get_serial_port_data(port);
> -
> - if (I_IXOFF(tty) || C_CRTSCTS(tty))
> - ti_stop_read(tport, tty);
> -
>-}
> -
> -
> -static void ti_unthrottle(struct tty_struct *tty)
> -{
> - struct usb_serial_port *port = tty->driver_data;
> - struct ti_port *tport = usb_get_serial_port_data(port);
> - int status;
> -
> - if (I_IXOFF(tty) || C_CRTSCTS(tty)) {
> - status = ti_restart_read(tport, tty);
> - if (status)
> - dev_err(&port->dev, "%s - cannot restart read, %d\n",
> - __func__, status);
> - }
> -}
> -
> static int ti_ioctl(struct tty_struct *tty,
> unsigned int cmd, unsigned long arg)
> {
> @@ -978,8 +866,6 @@ static void ti_set_termios(struct tty_struct *tty,
> if ((C_BAUD(tty)) != B0)
> config->wFlags |= TI_UART_ENABLE_RTS_IN;
> config->wFlags |= TI_UART_ENABLE_CTS_OUT;
> - } else {
> - ti_restart_read(tport, tty);
> }
>
> if (I_IXOFF(tty) || I_IXON(tty)) {
> @@ -988,8 +874,6 @@ static void ti_set_termios(struct tty_struct *tty,
>
> if (I_IXOFF(tty))
> config->wFlags |= TI_UART_ENABLE_X_IN;
> - else
> - ti_restart_read(tport, tty);
>
> if (I_IXON(tty))
> config->wFlags |= TI_UART_ENABLE_X_OUT;
> @@ -1193,168 +1077,6 @@ exit:
> __func__, retval);
> }
The interactions with software flow control here needs to be looked at
more closely, as the generic implementation ignores them.
>
> -
> -static void ti_bulk_in_callback(struct urb *urb)
> -{
> - struct ti_port *tport = urb->context;
> - struct usb_serial_port *port = tport->tp_port;
> - struct device *dev = &urb->dev->dev;
> - int status = urb->status;
> - int retval = 0;
> -
> - switch (status) {
> - case 0:
> - break;
> - case -ECONNRESET:
> - case -ENOENT:
> - case -ESHUTDOWN:
> - dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
> - tport->tp_tdev->td_urb_error = 1;
> - return;
> - default:
> - dev_err(dev, "%s - nonzero urb status, %d\n",
> - __func__, status);
> - tport->tp_tdev->td_urb_error = 1;
> - }
> -
> - if (status == -EPIPE)
> - goto exit;
> -
> - if (status) {
> - dev_err(dev, "%s - stopping read!\n", __func__);
> - return;
> - }
> -
> - if (urb->actual_length) {
> - usb_serial_debug_data(dev, __func__, urb->actual_length,
> - urb->transfer_buffer);
> -
> - if (!tport->tp_is_open)
> - dev_dbg(dev, "%s - port closed, dropping data\n",
> - __func__);
> - else
> - ti_recv(port, urb->transfer_buffer, urb->actual_length);
> - spin_lock(&tport->tp_lock);
> - port->icount.rx += urb->actual_length;
icount.tx/rx is not updated by the generic implementations either (there
are a few reasons why this driver has not simply been converted to use
the generic implementation already).
A bit too much is going on here at once, and we risk introducing
regression such as the issues raised above.
Please at least try to do the conversion in two steps for the rx and tx
paths.
Thanks,
Johan
--
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: typec: Add USB Power Delivery sink port support
Hi,
Greg Kroah-Hartman writes:
> On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Bin Gao writes:
>> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
>> > +{
>> > + pr_info("sink port %d: %s message %s %s\n", port,
>> > + is_cmsg ? "Control" : "Data",
>> > + msg_to_string(is_cmsg, msg),
>> > + recv ? "received" : "sent(wait GOODCRC)");
>> > +}
>>
>> this is problematic. By default, we're all using 115200 8N1 baud
>> rate. This message alone prints anywhere from 50 to 100 characters (I
>> didn't really count properly, these are rough numbers), and that takes:
>>
>> n50chars_time = 50 / (115200 / 10) = 4.3ms
>> n100chars_time = 100 / (115200 / 10) = 8.6ms
>>
>> Considering you have 30ms to reply with Power Request after GoodCRC, and
>> considering you're printing several of these messages, they become
>> really expensive and eat up valuable time from tSenderReply.
>
> printk() should be async, so it shouldn't be that big of a deal.
I can actually see this causing problems ;-) With this pr_info(),
sometimes tSenderReply times out and Source gives a HardReset. Without
pr_info(), type-c analyzer tells me we reply in less than 1ms.
> What is wrong is that this isn't using dev_info().
right, that too.
--
balbi
signature.asc
Description: PGP signature
Re: [PATCH v2 2/5] usb: serial: removing redundant __func__
On Fri, Jul 15, 2016 at 08:14:26PM +0900, Greg KH wrote: > On Fri, Jul 15, 2016 at 12:40:45PM +0200, Oliver Neukum wrote: > > On Fri, 2016-07-15 at 11:35 +0200, Johan Hovold wrote: > > > which I find much harder to parse. We don't always enforce a common > > > prefix for function names, making grouping related functions even > > > harder. > > > > > > Usually, what is printed in a debug message only makes sense in > > > combination with the function name (e.g. serial_write: 2 bytes), but > > > now > > > that connection is also less clear. > > > > > > Also consider what the above log would look like if you have more than > > > one device active. Trying to keep the independent traces separate by > > > simply looking at the logs becomes almost impossible. For that reason > > > I > > > also very much prefer having the device name at the start of the > > > message. > > > > You are right. But the correct remedy would be to fix dynamic debugging. > > Indeed printing the function before the module is braindead. > > Yes, let's fix that instead, let me knock one up right now... Wait, no, that's not the default at all. module name and function name and line number and thread id are all options that you can ask for in the prefix. If you don't, then they don't show up at all (which is why I never saw it before...) The code looks like it is adding the module name before the function name, and it uses a ":" after the module name, which doesn't match up with what Johan showed in his log: [ 116.426849] ftdi_set_termios: ftdi_sio ttyUSB0: Setting CS8 Johan, how are you enabling dynamic debug for these modules? If you just use "+p" no function name should be there, you have to add "+mf" to get module and function names, right? messy stuff... 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 16/36] usb: serial: ti_usb_3410_5052: Use bulk_out_size in TIOCGSERIAL
On Thu, May 12, 2016 at 10:48:48AM +0200, Mathieu OTHACEHE wrote: > Use bulk_out_size instead of recalculate it with kfifo_size > > Signed-off-by: Mathieu OTHACEHE > --- > drivers/usb/serial/ti_usb_3410_5052.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/usb/serial/ti_usb_3410_5052.c > b/drivers/usb/serial/ti_usb_3410_5052.c > index eb4df1e..5ef721c 100644 > --- a/drivers/usb/serial/ti_usb_3410_5052.c > +++ b/drivers/usb/serial/ti_usb_3410_5052.c > @@ -1205,7 +1205,7 @@ static int ti_get_serial_info(struct ti_port *tport, > ret_serial.type = PORT_16550A; > ret_serial.line = port->minor; > ret_serial.port = port->port_number; > - ret_serial.xmit_fifo_size = kfifo_size(&port->write_fifo); > + ret_serial.xmit_fifo_size = port->bulk_out_size; These two are in not equivalent. bulk_out_size holds the urb buffer size (which coincides with the endpoint size by default, but need to do so), while the fifo size is the size of the write fifo, which is typically always larger. This field is supposed to reflect the hardware fifo size, and we don't really use it consistently for usb-serial drivers. I'd just leave it as is for now. Thanks, Johan -- 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 17/36] usb: serial: ti_usb_3410_5052: Remove useless tty_wakeup
On Thu, May 12, 2016 at 10:48:49AM +0200, Mathieu OTHACEHE wrote:
> The generic driver doesn't call tty_wakeup in
> usb_serial_generic_msr_changed so this tty_wakeup seems useless.
While we strive to have the generic implementation as complete as
possible, it not doing something does not mean it must not be done. :)
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 9 -
> 1 file changed, 9 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 5ef721c..3f2372e 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -1238,7 +1238,6 @@ static int ti_set_serial_info(struct tty_struct *tty,
> struct ti_port *tport,
> static void ti_handle_new_msr(struct ti_port *tport, u8 msr)
> {
> struct async_icount *icount;
> - struct tty_struct *tty;
> unsigned long flags;
>
> dev_dbg(&tport->tp_port->dev, "%s - msr 0x%02X\n", __func__, msr);
> @@ -1259,14 +1258,6 @@ static void ti_handle_new_msr(struct ti_port *tport,
> u8 msr)
> }
>
> tport->tp_msr = msr & TI_MSR_MASK;
> -
> - /* handle CTS flow control */
> - tty = tty_port_tty_get(&tport->tp_port->port);
> - if (tty && C_CRTSCTS(tty)) {
> - if (msr & TI_MSR_CTS)
> - tty_wakeup(tty);
> - }
> - tty_kref_put(tty);
That said, this particular change does seem correct, as any writers
would have been woken up once any blocked (nacked) bulk-out transfers
complete.
Thanks,
Johan
--
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 18/36] usb: serial: ti_usb_3410_5052: Change ti_write_byte function arguments
On Thu, May 12, 2016 at 10:48:50AM +0200, Mathieu OTHACEHE wrote: > Remove useless ti_device pointer, and change addr to u32. > Move function upper to avoid function prototyping. That's just noise. Having an occasional prototype is just fine. You may want to consider reordering functions for the first submission, but no need to move things around after that. > Also change size variable in function from int to size_t. Combining changes with moving code, makes it harder to review for no good reason. Thanks, Johan -- 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 1/5] drivers: usb: chipidea: Add qoriq platform driver
> -Original Message- > From: Peter Chen [mailto:hzpeterc...@gmail.com] > Sent: Friday, July 15, 2016 12:43 PM > To: Rajesh Bhagat > Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; > devicet...@vger.kernel.org; Peter Chen ; > gre...@linuxfoundation.org; kis...@ti.com; robh...@kernel.org; > shawn...@kernel.org; linux-arm-ker...@lists.infradead.org > Subject: Re: [PATCH v2 1/5] drivers: usb: chipidea: Add qoriq platform driver > > On Tue, Jul 12, 2016 at 03:59:07AM +, Rajesh Bhagat wrote: > > > > + > > > > +err_clks: > > > > + ci_hdrc_qoriq_disable_unprepare_clks(pdev); > > > > > > If you have only one clock, it is unnecessary to use dedicated APIs for > > > clock > operation. > > > > > > > We do have multiple clocks, but currently one is integrated in code. > > Hence created the APIs for future use. Hello Peter, > > If you could not integrate one more clocks, I suggest not creating dedicated > API until > you need in future. > Okay, Will take care in v3. Best Regards, Rajesh Bhagat > -- > > 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 v2 2/5] usb: serial: removing redundant __func__
On Fri, Jul 15, 2016 at 08:26:25PM +0900, Greg Kroah-Hartman wrote: > On Fri, Jul 15, 2016 at 08:14:26PM +0900, Greg KH wrote: > > On Fri, Jul 15, 2016 at 12:40:45PM +0200, Oliver Neukum wrote: > > > On Fri, 2016-07-15 at 11:35 +0200, Johan Hovold wrote: > > > > which I find much harder to parse. We don't always enforce a common > > > > prefix for function names, making grouping related functions even > > > > harder. > > > > > > > > Usually, what is printed in a debug message only makes sense in > > > > combination with the function name (e.g. serial_write: 2 bytes), but > > > > now > > > > that connection is also less clear. > > > > > > > > Also consider what the above log would look like if you have more than > > > > one device active. Trying to keep the independent traces separate by > > > > simply looking at the logs becomes almost impossible. For that reason > > > > I > > > > also very much prefer having the device name at the start of the > > > > message. > > > > > > You are right. But the correct remedy would be to fix dynamic debugging. > > > Indeed printing the function before the module is braindead. I think you meant "device name" and not "module" here, Oliver? > > Yes, let's fix that instead, let me knock one up right now... > > Wait, no, that's not the default at all. module name and function name > and line number and thread id are all options that you can ask for in > the prefix. If you don't, then they don't show up at all (which is why > I never saw it before...) > > The code looks like it is adding the module name before the function > name, and it uses a ":" after the module name, which doesn't match up > with what Johan showed in his log: > > [ 116.426849] ftdi_set_termios: ftdi_sio ttyUSB0: Setting CS8 My concern was about the device name (e.g. "ftdi_sio ttyUSB0") being printed after the (module and) function name. You still need to explicitly ask for the function name to be printed though, and I tend not to enable that because of the reasons I mentioned above. And unless people start enabling that "+f" a lot of messages are likely to become very cryptic if we simply drop the %s/__func__. Also note that enabling "+m" would make the above example look like: [ 8649.481781] ftdi_sio:ftdi_set_termios: ftdi_sio ttyUSB0: Setting CS8 > Johan, how are you enabling dynamic debug for these modules? If you > just use "+p" no function name should be there, you have to add "+mf" to > get module and function names, right? Usually I just use "+p", but I could start using "+pf" (or "+pmf") if I could get the device name to be printed before the module and function names. > messy stuff... Good if we can get it sorted, though. Thanks, Johan -- 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 21/36] usb: serial: ti_usb_3410_5052: Use generic close function
On Thu, May 12, 2016 at 10:48:53AM +0200, Mathieu OTHACEHE wrote:
> Use usb_serial_generic_close in close callback.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 12 ++--
> 1 file changed, 2 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index 8350c6b..3d36ae7 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -807,24 +807,16 @@ static void ti_close(struct usb_serial_port *port)
> {
> struct ti_device *tdev;
> struct ti_port *tport;
> - int port_number;
> int status;
> int do_unlock;
> - unsigned long flags;
>
> tdev = usb_get_serial_data(port->serial);
> tport = usb_get_serial_port_data(port);
>
> - usb_kill_urb(port->read_urb);
> - usb_kill_urb(port->write_urb);
> - spin_lock_irqsave(&tport->tp_lock, flags);
> - kfifo_reset_out(&port->write_fifo);
> - spin_unlock_irqrestore(&tport->tp_lock, flags);
> -
> - port_number = port->port_number;
> + usb_serial_generic_close(port);
This change must go with the conversion to use the generic
implementations (e.g. in order to kill both read urbs at close, etc).
Thanks,
Johan
--
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 22/36] usb: serial: ti_usb_3410_5052: Change ti_get/set_serial_info function arguments
On Thu, May 12, 2016 at 10:48:54AM +0200, Mathieu OTHACEHE wrote: > It is sufficient to pass usb_serial_port structure to ti_get_serial_info > and ti_set_serial_info. > > Also move functions above ioctl to avoid function prototyping > and use unsigned int instead of unsigned for cwait variable. Avoid changing things in the same patch as you move something, if moving is at all needed. Thanks, Johan -- 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 24/36] usb: serial: ti_usb_3410_5052: Use usb_serial_generic_open
On Thu, May 12, 2016 at 10:48:56AM +0200, Mathieu OTHACEHE wrote:
> Use usb_serial_generic_open in open callback to start read urb.
> Also remove useless usb_device pointer.
>
> Signed-off-by: Mathieu OTHACEHE
> ---
> drivers/usb/serial/ti_usb_3410_5052.c | 21 -
> 1 file changed, 4 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/usb/serial/ti_usb_3410_5052.c
> b/drivers/usb/serial/ti_usb_3410_5052.c
> index d8bed30..4769c80 100644
> --- a/drivers/usb/serial/ti_usb_3410_5052.c
> +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> @@ -674,7 +674,6 @@ static int ti_open(struct tty_struct *tty, struct
> usb_serial_port *port)
> struct ti_port *tport = usb_get_serial_port_data(port);
> struct usb_serial *serial = port->serial;
> struct ti_device *tdev;
> - struct usb_device *dev;
> struct urb *urb;
> int port_number;
> int status;
> @@ -684,7 +683,6 @@ static int ti_open(struct tty_struct *tty, struct
> usb_serial_port *port)
>TI_PIPE_TIMEOUT_ENABLE |
>(TI_TRANSFER_TIMEOUT << 2));
>
> - dev = port->serial->dev;
> tdev = tport->tp_tdev;
>
> /* only one open on any port on a device at a time */
> @@ -748,8 +746,8 @@ static int ti_open(struct tty_struct *tty, struct
> usb_serial_port *port)
>
> /* reset the data toggle on the bulk endpoints to work around bug in
>* host controllers where things get out of sync some times */
> - usb_clear_halt(dev, port->write_urb->pipe);
> - usb_clear_halt(dev, port->read_urb->pipe);
> + usb_clear_halt(serial->dev, port->write_urb->pipe);
> + usb_clear_halt(serial->dev, port->read_urb->pipe);
>
> if (tty)
> ti_set_termios(tty, port, &tty->termios);
> @@ -770,20 +768,9 @@ static int ti_open(struct tty_struct *tty, struct
> usb_serial_port *port)
> goto unlink_int_urb;
> }
>
> - /* start read urb */
> - urb = port->read_urb;
> - if (!urb) {
> - dev_err(&port->dev, "%s - no read urb\n", __func__);
> - status = -EINVAL;
> - goto unlink_int_urb;
> - }
> - urb->context = tport;
> - status = usb_submit_urb(urb, GFP_KERNEL);
> - if (status) {
> - dev_err(&port->dev, "%s - submit read urb failed, %d\n",
> - __func__, status);
> + status = usb_serial_generic_open(tty, port);
> + if (status)
> goto unlink_int_urb;
> - }
Ok, you did not submit the second urb until here.
I'd rather you see you start using both the generic open and close
callbacks when you convert the read implementation (followed by a write
conversion).
Thanks,
Johan
--
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 25/36] usb: serial: ti_usb_3410_5052: Check old_termios parameter in set_termios
On Thu, May 12, 2016 at 10:48:57AM +0200, Mathieu OTHACEHE wrote: > The old_termios parameter is never used in set_termios callback. There we go. :) > Add a check to old_termios to see if we can return right away because > there is nothing to change. > Add a check to old_termios CBAUD to see if we can set DTR/RTS because > last speed was B0. Try splitting that up in two patches. > Also pass NULL for old_termios in open callback because it is the > initial call to set_termios. That's what you need to do of course (not change the prototype as I said earlier). Thanks, Johan -- 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 35/36] usb: serial: ti_usb_3410_5052: Remove function prototypes
On Thu, May 12, 2016 at 10:49:07AM +0200, Mathieu OTHACEHE wrote: > Declare functions in a the right order to avoid prototyping. > There is no functional change here. I'm not sure this is needed. Thanks, Johan -- 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 00/36] usb: serial: ti_usb_3410_5052: clean driver
On Fri, Jul 15, 2016 at 12:48:25PM +0200, Johan Hovold wrote: > On Thu, May 12, 2016 at 10:48:32AM +0200, Mathieu OTHACEHE wrote: > > Hi, > > > > The now reverted mxu11x0 turned out to be a copy of ti_usb_3410_5052 driver. > > This aim of this serie is to apply all of the cleanups we did in mxu11x0 to > > ti_usb_3410_5052. > > I apologise for the late review of this one. I've applied the first four > now (with some tweaks) and will comment on the rest. These overall look really good. I had few comments to some of the patches though. Thanks again for doing this. Johan -- 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: serial: cp210x: use kmemdup
On Thu, May 19, 2016 at 07:34:36PM +0530, Muhammad Falak R Wani wrote: > Use kmemdup when some other buffer is immediately copied into allocated > region. It replaces call to allocation followed by memcpy, by a single > call to kmemdup. > > Signed-off-by: Muhammad Falak R Wani Now applied, thanks. Johan -- 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/1] USB: serial: option: add support for Telit LE910 PID 0x1206
On Mon, Jun 06, 2016 at 12:38:17PM +0200, Daniele Palmas wrote: > This patch adds support for 0x1206 PID of Telit LE910. > > Since the interfaces positions are the same than the ones for > 0x1043 PID of Telit LE922, telit_le922_blacklist_usbcfg3 is used. > > Signed-off-by: Daniele Palmas Now applied. Sorry about the delay. Thanks, Johan -- 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 1/5] usb: serial: use variable for status
On Thu, Jul 14, 2016 at 03:01:40PM +0200, Oliver Neukum wrote: > This patch turns status in a variable read once from the URB. > The long term plan is to deliver status to the callback. > In addition it makes the code a bit more elegant. > > Signed-off-by: Oliver Neukum I've applied this one, and let's see where the discussion leads regarding the others. Thanks, Johan -- 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: fix gadgetfs aio support.
Fix io submissions failing with ENODEV.
Signed-off-by: Mathieu Laurendeau
Fixes: 7fe3976e0f3a ("gadget: switch ep_io_operations to
->read_iter/->write_iter")
---
drivers/usb/gadget/legacy/inode.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/legacy/inode.c
b/drivers/usb/gadget/legacy/inode.c
index aa3707b..be64798 100644
--- a/drivers/usb/gadget/legacy/inode.c
+++ b/drivers/usb/gadget/legacy/inode.c
@@ -542,7 +542,7 @@ static ssize_t ep_aio(struct kiocb *iocb,
*/
spin_lock_irq(&epdata->dev->lock);
value = -ENODEV;
- if (unlikely(epdata->ep))
+ if (unlikely(epdata->ep == NULL))
goto fail;
req = usb_ep_alloc_request(epdata->ep, GFP_ATOMIC);
--
2.7.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 1/2] usb: typec: Add USB Power Delivery sink port support
Hi again,
Felipe Balbi writes:
> Oliver Neukum writes:
>> On Fri, 2016-07-15 at 10:25 +0300, Felipe Balbi wrote:
>>> > +int pd_sink_queue_msg(struct pd_sink_msg *msg)
>>> > +{
>>> > + unsigned long flags;
>>> > + struct pd_sink_port *port;
>>> > +
>>> > + if (msg->port < 0 || msg->port >= MAX_NR_SINK_PORTS) {
>>> > + pr_err("Invalid port number\n");
>>> > + return -EINVAL;
>>> > + }
>>> > +
>>> > + port = sink_ports[msg->port];
>>> > +
>>> > + spin_lock_irqsave(&port->rx_lock, flags);
>>> > + list_add_tail(&msg->list, &port->rx_list);
>>> > + spin_unlock_irqrestore(&port->rx_lock, flags);
>>> > +
>>> > + queue_work(port->rx_wq, &port->rx_work);
>>>
>>> can we really queue several messages at a time? It seems unfeasible to
>>> me. It's not like we can queue several power request in a role. Why do
>>> you need this workqueue? Why don't you process message here, in place?
>>
>> A reset can come at any time.
>
> right, but that's not how this is being used. IMHO, rx_work is a
> misnomer. If you look at how typec_wcove (patch 2 in this series) uses
> it, you'll see that pd_sink_queue_msg() is called to queue a reply to a
> message that was *already* received. We can't have two replies, right?
>
> In any case, this is a minor problem.
oh wait, it's not a minor problem. If CPU is busy, this workqueue might
take longer than 30ms to get scheduled. This is another problem I just
reproduced, even after changing that pr_info() in print_message() to a
pr_debug().
Everything worked fine when I called rx_msg_worker() directly, instead
of queueing it to the workqueue.
--
balbi
signature.asc
Description: PGP signature
RE: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity for imx6 and imx7
> -Original Message- > From: Peter Chen [mailto:hzpeterc...@gmail.com] > Sent: Friday, July 15, 2016 5:21 PM > To: Jun Li > Cc: Peter Chen ; linux-usb@vger.kernel.org > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > for imx6 and imx7 > > On Fri, Jul 15, 2016 at 07:38:23AM +, Jun Li wrote: > > Hi, > > > -Original Message- > > > From: Peter Chen [mailto:hzpeterc...@gmail.com] > > > Sent: Friday, July 15, 2016 3:02 PM > > > To: Jun Li > > > Cc: Peter Chen ; linux-usb@vger.kernel.org > > > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current > > > polarity for imx6 and imx7 > > > > > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > > > As all usb power supply use low active for over current flag on > > > > imx6 > > > > imx7 boards, and the default register setting(0) is for high > > > > active, this patch is to correct it. > > > > > > > > > > We may can't ensure all USB power switch chips work like that, I > > > suggest you making this as default. > > > > > > I will change the commit log like below if you are ok. > > > > > > As most of all usb power switch chips use active-low for over > > > current flag, but the default register setting(0) is for active-high > > > at imx6/imx7, this patch changes default value as active-low. > > > > Looks better, I am okay with it except a tiny comment :%s/As most of > > all usb power/As most of usb power > > > > Li Jun > > > > Since we can't break current default behaviour, but with your patch, the > imx6sx sdb board creates over current event. I just checked the imx6sx sdb, the OC flag is also active low, but the Pull-up is DNP, if we did not disable OC in dts, a correct polarity setting may cause the problem. > > I think you may need to introduce a flag for OC polarity, and use it for > exist platforms if necessary. It can narrow down affect only on single > platform. > > For new platforms, you can change SoC values by default. Ok. > > -- > > 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] net: usb: ax88172x: use phy_ethtool_{get|set}_link_ksettings
There are two generics functions phy_ethtool_{get|set}_link_ksettings,
so we can use them instead of defining the same code in the driver.
Signed-off-by: Philippe Reynes
---
drivers/net/usb/ax88172a.c | 22 ++
1 files changed, 2 insertions(+), 20 deletions(-)
diff --git a/drivers/net/usb/ax88172a.c b/drivers/net/usb/ax88172a.c
index cf77f2d..163a2c5 100644
--- a/drivers/net/usb/ax88172a.c
+++ b/drivers/net/usb/ax88172a.c
@@ -149,24 +149,6 @@ static const struct net_device_ops ax88172a_netdev_ops = {
.ndo_set_rx_mode= asix_set_multicast,
};
-static int ax88172a_get_settings(struct net_device *net,
-struct ethtool_cmd *cmd)
-{
- if (!net->phydev)
- return -ENODEV;
-
- return phy_ethtool_gset(net->phydev, cmd);
-}
-
-static int ax88172a_set_settings(struct net_device *net,
-struct ethtool_cmd *cmd)
-{
- if (!net->phydev)
- return -ENODEV;
-
- return phy_ethtool_sset(net->phydev, cmd);
-}
-
static int ax88172a_nway_reset(struct net_device *net)
{
if (!net->phydev)
@@ -185,9 +167,9 @@ static const struct ethtool_ops ax88172a_ethtool_ops = {
.get_eeprom_len = asix_get_eeprom_len,
.get_eeprom = asix_get_eeprom,
.set_eeprom = asix_set_eeprom,
- .get_settings = ax88172a_get_settings,
- .set_settings = ax88172a_set_settings,
.nway_reset = ax88172a_nway_reset,
+ .get_link_ksettings = phy_ethtool_get_link_ksettings,
+ .set_link_ksettings = phy_ethtool_set_link_ksettings,
};
static int ax88172a_reset_phy(struct usbnet *dev, int embd_phy)
--
1.7.4.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: bug 120981 - usb controller reset / disconnect -
On Thu, 14 Jul 2016, Warren Postma wrote: > I am also able to reproduce this USB disconnect glitch on a Dell Xeon > workstation. Is there any way I can get more verbose output to dmesg: > > > On Sat, Jun 25, 2016 at 12:29:39PM +, > bugzilla-dae...@bugzilla.kernel.org wrote: > > https://bugzilla.kernel.org/show_bug.cgi?id=120981 > > > > Bug ID: 120981 > >Summary: 4.6.x VIA VL805 USB 3.0 controller resets device > > making it unusable until re-plugged > >Product: Drivers > >Version: 2.5 > > Kernel Version: 4.6.x > > Reproduced on 4.6.2, will try on 4.6.4. Any suggestions on how to > gather additional logging info would be appreciated. In addition to what Peter said, you can acquire a usbmon trace. See the instructions in the kernel source file Documentation/usb/usbmon.txt. 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: [PATCH v2 2/5] usb: serial: removing redundant __func__
On Fri, 2016-07-15 at 13:52 +0200, Johan Hovold wrote: > > Johan, how are you enabling dynamic debug for these modules? If you > > just use "+p" no function name should be there, you have to add > "+mf" to > > get module and function names, right? > > Usually I just use "+p", but I could start using "+pf" (or "+pmf") if > I > could get the device name to be printed before the module and function > names. I really have no strong preferences except that module be before function. Device could be anywhere, except that following the output is easier if function is indeed last, as it changes fastest. So we could have device module function module device function Either would be fine to me. Regards Oliver -- 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: chipidea: usbmisc: set over current polarity for imx6 and imx7
Hi, > -Original Message- > From: Peter Chen [mailto:hzpeterc...@gmail.com] > Sent: Friday, July 15, 2016 3:02 PM > To: Jun Li > Cc: Peter Chen ; linux-usb@vger.kernel.org > Subject: Re: [PATCH 2/2] usb: chipidea: usbmisc: set over current polarity > for imx6 and imx7 > > On Tue, Jul 12, 2016 at 03:24:49PM +0800, Li Jun wrote: > > As all usb power supply use low active for over current flag on imx6 > > imx7 boards, and the default register setting(0) is for high active, > > this patch is to correct it. > > > > We may can't ensure all USB power switch chips work like that, I suggest > you making this as default. > > I will change the commit log like below if you are ok. > > As most of all usb power switch chips use active-low for over current flag, > but the default register setting(0) is for active-high at imx6/imx7, this > patch changes default value as active-low. Looks better, I am okay with it except a tiny comment :%s/As most of all usb power/As most of usb power Li Jun > > > Signed-off-by: Li Jun -- 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 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
Hi, [auto build test ERROR on balbi-usb/next] [cannot apply to v4.7-rc7 next-20160715] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Baolin-Wang/Support-dwc3-host-suspend-resume/20160715-193735 base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next config: x86_64-randconfig-s1-07152158 (attached as .config) compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): drivers/built-in.o: In function `dwc3_runtime_suspend': >> core.c:(.text+0x221c35): undefined reference to `dwc3_gadget_suspend' drivers/built-in.o: In function `dwc3_runtime_resume': >> core.c:(.text+0x223178): undefined reference to `dwc3_gadget_resume' >> core.c:(.text+0x22319a): undefined reference to >> `dwc3_gadget_process_pending_events' --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
RE: [PATCH] rndis_host: Set random MAC for ZTE MF910
From: Bjørn Mork > Sent: 13 July 2016 23:23 ... > Or how about the more generic?: > > if (bp[0] & 0x02) > eth_hw_addr_random(net); > else > ether_addr_copy(net->dev_addr, bp); > > That would catch similar screwups from other vendors too. Not really, that disables 'locally administered' addresses. If a vendor has used the same address on lots of cards it could easily be a 'real' address. Not only that, there certainly used to be manufacturers that used 'locally administered' addresses on all their cards (as well as those that used unallocated address blocks). Not to mention the bit-revered addresses David
Re: [PATCH] rndis_host: Set random MAC for ZTE MF910
David Laight writes: > From: Bjørn Mork >> Sent: 13 July 2016 23:23 > ... >> Or how about the more generic?: >> >> if (bp[0] & 0x02) >> eth_hw_addr_random(net); >> else >> ether_addr_copy(net->dev_addr, bp); >> >> That would catch similar screwups from other vendors too. > > Not really, that disables 'locally administered' addresses. ... when the 'locally administered' addresses comes from firmeare, yes. That was the idea. We are better off using our own random locally administered address if some vendor has been cheap/stupid enough to program that into firmware. The aminstrator is of course still free to set any address, 'locally administered' or whatever. This is not the question here. > If a vendor has used the same address on lots of cards it could easily > be a 'real' address. Sure. We cannot easily detect that. The only way is to keep a blacklist of such 'real' addresses, the way Kristian initially proposed. But I thought that we could simplify this particular screwup since the address in question had the local bit set, and catch every other similar abuse at the same time. If you get the local bit from formware, then you know for sure that there is something wrong. > Not only that, there certainly used to be manufacturers that used 'locally > administered' addresses on all their cards (as well as those that used > unallocated > address blocks). Sure. But is there any reason to care about those addresses? > Not to mention the bit-revered addresses Listing all the ways vendors have screwed is going to be a long and rather boring thread ;) 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 4/4] usb: dwc3: core: Support the dwc3 host suspend/resume
Hi, [auto build test ERROR on balbi-usb/next] [cannot apply to v4.7-rc7 next-20160715] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Baolin-Wang/Support-dwc3-host-suspend-resume/20160715-193735 base: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git next config: x86_64-randconfig-s2-07152253 (attached as .config) compiler: gcc-6 (Debian 6.1.1-1) 6.1.1 20160430 reproduce: # save the attached .config to linux build tree make ARCH=x86_64 All errors (new ones prefixed by >>): >> ERROR: "dwc3_gadget_resume" [drivers/usb/dwc3/dwc3.ko] undefined! >> ERROR: "dwc3_gadget_process_pending_events" [drivers/usb/dwc3/dwc3.ko] >> undefined! >> ERROR: "dwc3_gadget_suspend" [drivers/usb/dwc3/dwc3.ko] undefined! --- 0-DAY kernel test infrastructureOpen Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation .config.gz Description: Binary data
Re: [PATCH 0 / 5] move the common CDC parser
From: Oliver Neukum Date: Thu, 14 Jul 2016 15:41:29 +0200 > Experience has shown that making all CDC drivers depend on usbnet > is not practical, because some of them are not network drivers. > So this patch moves the common parser from usbnet into the messages > helpers of usbcore. > The rest of the series applies it to the non-network CDC drivers. > > I hope it can go through Greg's tree although it touches usbnet. I'm fine with Greg taking this series, sure. -- 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: MAINTAINERS: Oliver Neukum is the new uas maintainer
> "Hans" == Hans de Goede writes: Hans> Oliver Neukum is taking over uas maintainership from me and Gerd Hans> Hoffmann. Applied to 4.8/scsi-queue. -- Martin K. Petersen Oracle Linux Engineering -- 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: ohci-platform: use helper variables in probe function
On Fri, 15 Jul 2016, Rafał Miłecki wrote: > Probing function was using &dev->dev and dev->dev.of_node over 20 times > so I believe it made sense to use helper variables for both of them. > To avoid some uncommon variable name for struct device I first replaced > existing dev variable with pdev. > > Signed-off-by: Rafał Miłecki Acked-by: 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 V2] leds: trigger: Introduce an USB port trigger
This commit adds a new trigger that can turn on LED when USB device gets
connected to the USB port. This can be useful for various home routers
that have USB port and a proper LED telling user a device is connected.
Right now this trigger is usable with a proper DT only, there isn't a
way to specify USB ports from user space. This may change in a future.
Signed-off-by: Rafał Miłecki
---
V2: The first version got support for specifying list of USB ports from
user space only. There was a (big try &) discussion on adding DT
support. It led to a pretty simple solution of comparing of_node of
usb_device to of_node specified in usb-ports property.
Since it appeared DT support may be simpler and non-DT a bit more
complex, this version drops previous support for "ports" and
"new_port" and focuses on DT only. The plan is to see if this
solution with DT is OK, get it accepted and then work on non-DT.
Felipe: if there won't be any objections I'd like to ask for your Ack.
---
Documentation/devicetree/bindings/leds/common.txt | 11 ++
Documentation/leds/ledtrig-usbport.txt| 19 ++
drivers/leds/trigger/Kconfig | 8 +
drivers/leds/trigger/Makefile | 1 +
drivers/leds/trigger/ledtrig-usbport.c| 206 ++
5 files changed, 245 insertions(+)
create mode 100644 Documentation/leds/ledtrig-usbport.txt
create mode 100644 drivers/leds/trigger/ledtrig-usbport.c
diff --git a/Documentation/devicetree/bindings/leds/common.txt
b/Documentation/devicetree/bindings/leds/common.txt
index af10678..75536f7 100644
--- a/Documentation/devicetree/bindings/leds/common.txt
+++ b/Documentation/devicetree/bindings/leds/common.txt
@@ -50,6 +50,12 @@ property can be omitted.
For controllers that have no configurable timeout the flash-max-timeout-us
property can be omitted.
+Trigger specific properties for child nodes:
+
+usbport trigger:
+- usb-ports : List of USB ports that usbport should observed for turning on a
+ given LED.
+
Examples:
system-status {
@@ -58,6 +64,11 @@ system-status {
...
};
+usb {
+ label = "USB";
+ usb-ports = <&ohci_port1>, <&ehci_port1>;
+};
+
camera-flash {
label = "Flash";
led-sources = <0>, <1>;
diff --git a/Documentation/leds/ledtrig-usbport.txt
b/Documentation/leds/ledtrig-usbport.txt
new file mode 100644
index 000..642c4cd
--- /dev/null
+++ b/Documentation/leds/ledtrig-usbport.txt
@@ -0,0 +1,19 @@
+USB port LED trigger
+
+
+This LED trigger can be used for signaling user a presence of USB device in a
+given port. It simply turns on LED when device appears and turns it off when it
+disappears.
+
+It requires specifying a list of USB ports that should be observed. This can be
+done in DT by setting a proper property with list of a phandles. If more than
+one port is specified, LED will be turned on as along as there is at least one
+device connected to any of ports.
+
+This trigger can be activated from user space on led class devices as shown
+below:
+
+ echo usbport > trigger
+
+Nevertheless, current there isn't a way to specify list of USB ports from user
+space.
diff --git a/drivers/leds/trigger/Kconfig b/drivers/leds/trigger/Kconfig
index 9893d91..5b8e7c7 100644
--- a/drivers/leds/trigger/Kconfig
+++ b/drivers/leds/trigger/Kconfig
@@ -126,4 +126,12 @@ config LEDS_TRIGGER_PANIC
a different trigger.
If unsure, say Y.
+config LEDS_TRIGGER_USBPORT
+ tristate "USB port LED trigger"
+ depends on LEDS_TRIGGERS && USB && OF
+ help
+ This allows LEDs to be controlled by USB events. This trigger will
+ enable LED if some USB device gets connected to any of ports specified
+ in DT.
+
endif # LEDS_TRIGGERS
diff --git a/drivers/leds/trigger/Makefile b/drivers/leds/trigger/Makefile
index 8cc64a4..80e2494 100644
--- a/drivers/leds/trigger/Makefile
+++ b/drivers/leds/trigger/Makefile
@@ -10,3 +10,4 @@ obj-$(CONFIG_LEDS_TRIGGER_DEFAULT_ON) += ledtrig-default-on.o
obj-$(CONFIG_LEDS_TRIGGER_TRANSIENT) += ledtrig-transient.o
obj-$(CONFIG_LEDS_TRIGGER_CAMERA) += ledtrig-camera.o
obj-$(CONFIG_LEDS_TRIGGER_PANIC) += ledtrig-panic.o
+obj-$(CONFIG_LEDS_TRIGGER_USBPORT) += ledtrig-usbport.o
diff --git a/drivers/leds/trigger/ledtrig-usbport.c
b/drivers/leds/trigger/ledtrig-usbport.c
new file mode 100644
index 000..97b064c
--- /dev/null
+++ b/drivers/leds/trigger/ledtrig-usbport.c
@@ -0,0 +1,206 @@
+/*
+ * USB port LED trigger
+ *
+ * Copyright (C) 2016 Rafał Miłecki
+ *
+ * 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
+#include
+#include "../leds.h"
+
+struct usbport_trig_port {
Re: [PATCH] r8152: add MODULE_VERSION
From: Grant Grundler Date: Thu, 14 Jul 2016 11:27:16 -0700 > ethtool -i provides a driver version that is hard coded. > Export the same value via "modinfo". > > Signed-off-by: Grant Grundler Applied. -- 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] r8152: add MODULE_VERSION
On Fri, Jul 15, 2016 at 2:25 PM, David Miller wrote: > From: Grant Grundler > Date: Thu, 14 Jul 2016 11:27:16 -0700 > >> ethtool -i provides a driver version that is hard coded. >> Export the same value via "modinfo". >> >> Signed-off-by: Grant Grundler > > Applied. Excellent - thank you. :) grant -- 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 0 / 5] move the common CDC parser
On Fri, Jul 15, 2016 at 11:51:47AM -0700, David Miller wrote: > From: Oliver Neukum > Date: Thu, 14 Jul 2016 15:41:29 +0200 > > > Experience has shown that making all CDC drivers depend on usbnet > > is not practical, because some of them are not network drivers. > > So this patch moves the common parser from usbnet into the messages > > helpers of usbcore. > > The rest of the series applies it to the non-network CDC drivers. > > > > I hope it can go through Greg's tree although it touches usbnet. > > I'm fine with Greg taking this series, sure. Ok, I'll take it, 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 1/2] usb: typec: Add USB Power Delivery sink port support
On Fri, Jul 15, 2016 at 08:31:08AM +0200, Oliver Neukum wrote:
> > +static void ack_message(struct pd_sink_port *port, int msg_id)
> > +{
> > + struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
>
> This must be GFP_NOIO. We are in a cycle that can lead to deadlock.
>
> Assume we are waiting for a request for more power to process IO
> which we need to ack.
>
> 1. memory allocation leads to laundering, blocks on freeing memory
> 2. launderer decides to perform IO which needs more power
> 3. more power has already been requested, wait for it to be granted
>
> 4. BANG - DEADLOCK
Agree, I'll change the GFP flag in next revision.
> > + struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> > + port->nr_ps * PD_OBJ_SIZE, GFP_KERNEL);
>
> Must be GFP_NOIO. For the same reason as above. We may be asked
> this to resolve a mismatch due to needing more power for IO.
Yes will do.
> > +static void handle_soft_reset(struct pd_sink_port *port)
> > +{
> > + struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN, GFP_KERNEL);
> > +
> > + if (!header)
> > + return;
> > +
> > + flush_workqueue(port->rx_wq);
>
> That is problematic. We may be here precisely because something is wrong
> blocking progress. In particular what happens if another soft reset
> is queued?
I'm going to remove the workqueue.
> > + struct pd_msg_header *header = kzalloc(PD_MSG_HEADER_LEN +
> > + PD_OBJ_SIZE, GFP_KERNEL);
>
> GFP_NOIO, same reasons
Yes.
> > +
>
> HTH
> Oliver
Thanks for your review.
--
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: typec: Add USB Power Delivery sink port support
On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> Greg Kroah-Hartman writes:
> > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> >>
> >> Hi,
> >>
> >> Bin Gao writes:
> >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> >> > +{
> >> > +pr_info("sink port %d: %s message %s %s\n", port,
> >> > +is_cmsg ? "Control" : "Data",
> >> > +msg_to_string(is_cmsg, msg),
> >> > + recv ? "received" : "sent(wait GOODCRC)");
> >> > +}
> >>
> >> this is problematic. By default, we're all using 115200 8N1 baud
> >> rate. This message alone prints anywhere from 50 to 100 characters (I
> >> didn't really count properly, these are rough numbers), and that takes:
> >>
> >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> >>
> >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> >> considering you're printing several of these messages, they become
> >> really expensive and eat up valuable time from tSenderReply.
> >
> > printk() should be async, so it shouldn't be that big of a deal.
>
> I can actually see this causing problems ;-) With this pr_info(),
> sometimes tSenderReply times out and Source gives a HardReset. Without
> pr_info(), type-c analyzer tells me we reply in less than 1ms.
>
> > What is wrong is that this isn't using dev_info().
>
> right, that too.
>
> --
> balbi
When we don't have a struct device pointer for this driver,
a dev_info(NULL, fmt, ...) is equivalent to pr_info(). So we have to
use dev_info() here?
But I agree at least it should be pr_debug().
--
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: typec: Add USB Power Delivery sink port support
On Fri, Jul 15, 2016 at 10:25:36AM +0300, Felipe Balbi wrote:
> Bin Gao writes:
>
> > This patch implements a simple USB Power Delivery sink port state machine.
> > It assumes the hardware only handles PD packet transmitting and receiving
> > over the CC line of the USB Type-C connector. The state transition is
> > completely controlled by software. This patch only implement the sink port
> > function and it doesn't support source port and port swap yet.
> >
> > This patch depends on these two patches:
> > https://lkml.org/lkml/2016/6/29/349
> > https://lkml.org/lkml/2016/6/29/350
> >
> > Signed-off-by: Bin Gao
> > ---
> > drivers/usb/typec/Kconfig | 13 +
> > drivers/usb/typec/Makefile | 1 +
> > drivers/usb/typec/pd_sink.c| 967
> > +
> > include/linux/usb/pd_message.h | 371
> > include/linux/usb/pd_sink.h| 286
> > 5 files changed, 1638 insertions(+)
> > create mode 100644 drivers/usb/typec/pd_sink.c
> > create mode 100644 include/linux/usb/pd_message.h
> > create mode 100644 include/linux/usb/pd_sink.h
> >
> > diff --git a/drivers/usb/typec/Kconfig b/drivers/usb/typec/Kconfig
> > index 7a345a4..a04a900 100644
> > --- a/drivers/usb/typec/Kconfig
> > +++ b/drivers/usb/typec/Kconfig
> > @@ -4,12 +4,25 @@ menu "USB PD and Type-C drivers"
> > config TYPEC
> > tristate
> >
> > +config USB_PD_SINK
> > + bool "USB Power Delivery Sink Port State Machine Driver"
>
> tristate?
>
> > + select TYPEC
>
> this should depend on TYPEC, not select it.
>
> > + help
> > + Enable this to support USB PD(Power Delivery) Sink port.
> > + This driver implements a simple USB PD sink state machine.
> > + The underlying TypeC phy driver is responsible for cable
> > + plug/unplug event, port orientation detection, transmitting
> > + and receiving PD messages. This driver only process messages
> > + received by the TypeC phy driver and maintain the sink port's
> > + state machine.
> > +
> > config TYPEC_WCOVE
> > tristate "Intel WhiskeyCove PMIC USB Type-C PHY driver"
> > depends on ACPI
> > depends on INTEL_SOC_PMIC
> > depends on INTEL_PMC_IPC
> > select TYPEC
> > + select USB_PD_SINK
>
> TYPEC without PD is valid, let user select PD support.
Yes will fix the Kconfig in next revision.
> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > +{
> > + pr_info("sink port %d: %s message %s %s\n", port,
> > + is_cmsg ? "Control" : "Data",
> > + msg_to_string(is_cmsg, msg),
> > +recv ? "received" : "sent(wait GOODCRC)");
>
> looks like a debugging message to me. We don't want to spam dmesg with
> every single message transmission.
This should be a pr_debug().
>
> > +static void start_timer(struct pd_sink_port *port, int timeout,
> > + enum hrtimer_restart (*f)(struct hrtimer *))
> > +{
> > + if (hrtimer_active(&port->tx_timer)) {
> > + pr_err("Error: previous timer is still active\n");
> > + return;
> > + }
> > +
> > + port->tx_timer.function = f;
> > + /* timeout comes with ms but ktime_set takes seconds and nanoseconds */
> > + hrtimer_start(&port->tx_timer, ktime_set(timeout / 1000,
>
> I don't think you need HR timers here. A normal mod_timer() should do.
When hrtimer is in place, the old timer becomes "legacy". And the old timer
APIs are implemented on top of hrtimer. It's no harm to use hrtimers anywhere
in the kernel and it would be encouraged in my opinion:-)
>
> > +static enum hrtimer_restart goodcrc_timeout(struct hrtimer *timer)
> > +{
> > + pr_err("GOODCRC message is not received in %d ms: timeout\n",
> > + PD_TIMEOUT_GOODCRC);
> > + return HRTIMER_NORESTART;
> > +}
> > +
> > +/*
> > + * For any message we send, we must get a GOODCRC message from the Source.
> > + * The USB PD spec says the time should be measured between the last bit
> > + * of the sending message's EOP has been transmitted and the last bit of
> > + * the receiving GOODCRC message's EOP has been received. The allowed time
> > + * is minimal 0.9 ms and maximal 1.1 ms. However, this measurement is
> > + * performed in physical layer. When it reaches to the OS and this driver,
> > + * the actual time is difficult to predict because of the scheduling,
> > + * context switch, interrupt preemption and nesting, etc. So we only define
> > + * a safe timeout value (PD_TIMEOUT_GOODCRC) which is large enough to take
> > + * account of all software related latency.
>
> that's true. But here's the thing. From 1ms to 500ms there are two
> orders of magnitude. If we take that long to realize we received
> GoodCRC, it's already too late. Remember that if we don't e.g. request
> power within 30ms after reception of GoodCRC, Source will issue a
> HardReset.
This part is for sure to be revisited even in the beginning of the design.
Re: [PATCH 1/2] usb: typec: Add USB Power Delivery sink port support
On Fri, Jul 15, 2016 at 03:41:10PM -0700, Bin Gao wrote:
> On Fri, Jul 15, 2016 at 02:21:48PM +0300, Felipe Balbi wrote:
> > Greg Kroah-Hartman writes:
> > > On Fri, Jul 15, 2016 at 01:38:12PM +0300, Felipe Balbi wrote:
> > >>
> > >> Hi,
> > >>
> > >> Bin Gao writes:
> > >> > +static void print_message(int port, bool is_cmsg, u8 msg, bool recv)
> > >> > +{
> > >> > + pr_info("sink port %d: %s message %s %s\n", port,
> > >> > + is_cmsg ? "Control" : "Data",
> > >> > + msg_to_string(is_cmsg, msg),
> > >> > + recv ? "received" : "sent(wait GOODCRC)");
> > >> > +}
> > >>
> > >> this is problematic. By default, we're all using 115200 8N1 baud
> > >> rate. This message alone prints anywhere from 50 to 100 characters (I
> > >> didn't really count properly, these are rough numbers), and that takes:
> > >>
> > >> n50chars_time = 50 / (115200 / 10) = 4.3ms
> > >> n100chars_time = 100 / (115200 / 10) = 8.6ms
> > >>
> > >> Considering you have 30ms to reply with Power Request after GoodCRC, and
> > >> considering you're printing several of these messages, they become
> > >> really expensive and eat up valuable time from tSenderReply.
> > >
> > > printk() should be async, so it shouldn't be that big of a deal.
> >
> > I can actually see this causing problems ;-) With this pr_info(),
> > sometimes tSenderReply times out and Source gives a HardReset. Without
> > pr_info(), type-c analyzer tells me we reply in less than 1ms.
> >
> > > What is wrong is that this isn't using dev_info().
> >
> > right, that too.
> >
> > --
> > balbi
>
> When we don't have a struct device pointer for this driver,
Then you should fix that, as this is a driver for hardware :)
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 v2 2/5] usb: DT binding documentation for qoriq usb 2.0 controller
On Sat, Jul 09, 2016 at 10:00:53AM +0530, Rajesh Bhagat wrote:
> Describes the qoriq usb 2.0 controller driver binding, currently used
> for LS1021A and LS1012A platform.
>
> Signed-off-by: Rajesh Bhagat
> ---
> Changes in v2:
> - Adds DT binding documentation for qoriq usb 2.0 controller
> - Changed the compatible string to fsl,ci-qoriq-usb2
>
> .../devicetree/bindings/usb/ci-hdrc-qoriq.txt | 34
> ++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt
> b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt
> new file mode 100644
> index 000..8ad7306
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-qoriq.txt
> @@ -0,0 +1,34 @@
> +* Freescale QorIQ SoC USB 2.0 Controllers
> +
> +Required properties:
> +- compatible: Should be "fsl,ci-qoriq-usb2"
> + Wherever applicable, the IP version of the USB controller should
> + also be mentioned (for eg. fsl,ci-qoriq-usb2-vX.Y).
> + where, X.Y is IP version of USB controller.
Please document known IP versions.
> +- reg: Should contain registers location and length
> +- interrupts: Should contain controller interrupt
> +- phy-names: from the *Generic PHY* bindings
> +- phys: from the *Generic PHY* bindings
> +- clocks: clock provider specifier
> +- clock-names: shall be "usb2-clock"
clock-names is kind of pointless for a single clock and '-clock' is
redundant.
> +Refer to clk/clock-bindings.txt for generic clock consumer properties
> +
> +Recommended properties:
> +- dr_mode: One of "host" or "peripheral".
> +- phy_type: the type of the phy connected to the core. Should be one
> + of "utmi", "utmi_wide", "ulpi", "serial" or "hsic". Without this
> + property the PORTSC register won't be touched
> +
> +Examples:
> +usb@860 {
> + compatible = "fsl,ci-qoriq-usb2",
> + "fsl,ci-qoriq-usb2-v2.5";
Order should be most specific to least specific.
> + reg = <0x0 0x860 0x0 0x1000>;
> + interrupts = <0 139 0x4>;
> + phy-names = "usb2-phy";
> + phys = <&usbphy0>;
> + clock-names = "usb2-clock";
> + clocks = <&clockgen 4 3>;
> + dr_mode = "host";
> + phy_type = "ulpi";
> +};
> --
> 2.6.2.198.g614a2ac
>
--
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 4/5] phy: DT binding documentation for qoriq usb 2.0 phy
On Sat, Jul 09, 2016 at 10:00:55AM +0530, Rajesh Bhagat wrote:
> Describes the qoriq usb 2.0 phy driver binding, currently used
> for LS1021A and LS1012A platform.
>
> Signed-off-by: Rajesh Bhagat
> ---
> Changes in v2:
> - Adds DT binding documentation for qoriq usb 2.0 phy
> - Changed the compatible string to fsl,qoriq-usb2-phy
>
> .../devicetree/bindings/phy/qoriq-usb2-phy.txt | 22
> ++
> 1 file changed, 22 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/phy/qoriq-usb2-phy.txt
>
> diff --git a/Documentation/devicetree/bindings/phy/qoriq-usb2-phy.txt
> b/Documentation/devicetree/bindings/phy/qoriq-usb2-phy.txt
> new file mode 100644
> index 000..f043855
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/qoriq-usb2-phy.txt
> @@ -0,0 +1,22 @@
> +QorIQ SoC USB 2.0 PHY
> +
> +Required properties:
> + - compatible: should be "fsl,qoriq-usb2-phy",
> + Wherever applicable, the version of the USB PHY should
> + also be mentioned (for eg. fsl,qoriq-usb2-phy-vX.Y).
> + where, X = Phy vendor(Legacy = 1, NXP = 2) and Y = PHY version
What does Legacy mean? FSL?
Use SoC specific compatible strings.
> + - reg : Address and length of the usb phy control register set.
> + - phy_type : For multi port host USB controllers, should be one of
> + "ulpi", or "serial". For dual role USB controllers, should be
> + one of "ulpi", "utmi", "utmi_wide", or "serial".
> +
> +The main purpose of this PHY driver is to enable the USB PHY reference clock
> +gate on the QorIQ SOC for USB2 PHY OR implement errata workaround in
> +future. Otherwise it is just an NOP PHY driver.
> +
> +usbphy0: usbphy@860 {
> +compatible = "fsl,qoriq-usb2-phy" "fsl,qoriq-usb2-phy-vX.Y";
most specific first.
> +reg = <0x0 0x860 0x0 0x1000>;
> +#phy-cells = <0>;
> +phy_type = "ulpi";
> +};
--
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/2] usb: gadget: composite: Fix return value in case of error
In 'composite_os_desc_req_prepare', if one of the memory allocations fail,
0 will be returned, which means success.
We should return -ENOMEM instead.
Signed-off-by: Christophe JAILLET
---
drivers/usb/gadget/composite.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index eb64848..8241856 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2124,14 +2124,14 @@ int composite_os_desc_req_prepare(struct
usb_composite_dev *cdev,
cdev->os_desc_req = usb_ep_alloc_request(ep0, GFP_KERNEL);
if (!cdev->os_desc_req) {
- ret = PTR_ERR(cdev->os_desc_req);
+ ret = -ENOMEM;
goto end;
}
/* OS feature descriptor length <= 4kB */
cdev->os_desc_req->buf = kmalloc(4096, GFP_KERNEL);
if (!cdev->os_desc_req->buf) {
- ret = PTR_ERR(cdev->os_desc_req->buf);
+ ret = -ENOMEM;
kfree(cdev->os_desc_req);
goto end;
}
--
2.7.4
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
antivirus Avast.
https://www.avast.com/antivirus
--
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: gadget: composite: Fix function used to free memory
'cdev->os_desc_req' has been allocated with 'usb_ep_alloc_request' so
'usb_ep_free_request' should be used to free it.
Signed-off-by: Christophe JAILLET
---
drivers/usb/gadget/composite.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 8241856..9fe73cf 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2132,7 +2132,7 @@ int composite_os_desc_req_prepare(struct
usb_composite_dev *cdev,
cdev->os_desc_req->buf = kmalloc(4096, GFP_KERNEL);
if (!cdev->os_desc_req->buf) {
ret = -ENOMEM;
- kfree(cdev->os_desc_req);
+ usb_ep_free_request(ep0, cdev->os_desc_req);
goto end;
}
cdev->os_desc_req->context = cdev;
--
2.7.4
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
antivirus Avast.
https://www.avast.com/antivirus
--
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: composite: Fix function used to free memory
'cdev->os_desc_req' has been allocated with 'usb_ep_alloc_request' so
'usb_ep_free_request' should be used to free it.
Signed-off-by: Christophe JAILLET
---
drivers/usb/gadget/composite.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
index 8241856..9fe73cf 100644
--- a/drivers/usb/gadget/composite.c
+++ b/drivers/usb/gadget/composite.c
@@ -2132,7 +2132,7 @@ int composite_os_desc_req_prepare(struct
usb_composite_dev *cdev,
cdev->os_desc_req->buf = kmalloc(4096, GFP_KERNEL);
if (!cdev->os_desc_req->buf) {
ret = -ENOMEM;
- kfree(cdev->os_desc_req);
+ usb_ep_free_request(ep0, cdev->os_desc_req);
goto end;
}
cdev->os_desc_req->context = cdev;
--
2.7.4
---
L'absence de virus dans ce courrier électronique a été vérifiée par le logiciel
antivirus Avast.
https://www.avast.com/antivirus
--
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
