Re: [PATCH 2/2] pci: quirks: Override Synopsys USB 3.x HAPS device driver

[Thinh Nguyen]

Hi Bjorn,

On 11/6/2018 12:44 AM, Felipe Balbi wrote:
> Thinh Nguyen  writes:
>
>> ++ linux-usb
>> ++ Greg
>>
>> On 11/2/2018 6:47 PM, Thinh Nguyen wrote:
>>> Synopsys USB 3.x host HAPS platform has a class code of
>>> PCI_CLASS_SERIAL_USB_XHCI, and xhci driver can claim it. However, these
>>> devices should use dwc3-haps driver. Set driver_override to dwc3-haps
>>> for these platforms.
>>>
>>> Signed-off-by: Thinh Nguyen 
> FWIW:
>
> Acked-by: Felipe Balbi 
>

Please let me know if you're ok with these 2 patches.  (This and patch
subject "[PATCH 1/2] pci: pci_ids: Move Synopsys HAPS platform device IDs")

Thanks,
Thinh

[PATCH v2 4/4] usb: dwc3: Enable frame number tracking based on reference clock

[Thinh Nguyen]

Version 1.80a of the DWC_usb31 peripheral controller introduced a
feature to track the frame number based the reference clock. This patch
checks and enables this feature.

When operating in USB 2.0 mode, the peripheral controller uses the USB2
PHY clocks to track the frame number. This prevents the controller from
suspending the USB2 PHY when the device goes into low power. This
feature allows the controller to suspend the USB2 PHY when the device
enters low power. This improves power saving for devices that have
isochronous endpoints.

Signed-off-by: Thinh Nguyen 
---
Changes in v2:
- Revise commit message
- Properly check for version and controller type

 drivers/usb/dwc3/core.c | 51 +
 drivers/usb/dwc3/core.h | 12 
 2 files changed, 63 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 32c38f71f874..38597a32cb20 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -882,6 +882,39 @@ static void dwc3_set_incr_burst_type(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
 }
 
+/**
+ * dwc3_enable_refclk_sof - Enable frame number tracking based on ref_clk
+ * @dwc: Pointer to our controller context structure
+ *
+ * Returns 0 on success, otherwise negative errno.
+ */
+static int dwc3_enable_refclk_sof(struct dwc3 *dwc)
+{
+   u8  refclk_period_ns;
+   u32 reg;
+
+   reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
+   refclk_period_ns = DWC3_GUCTL_GET_REFCLKPER(reg);
+
+   /* Only valid for the following reference clock periods */
+   switch (refclk_period_ns) {
+   case DWC3_GUCTL_REFCLKPER_25NS:
+   case DWC3_GUCTL_REFCLKPER_41NS:
+   case DWC3_GUCTL_REFCLKPER_50NS:
+   case DWC3_GUCTL_REFCLKPER_52NS:
+   case DWC3_GUCTL_REFCLKPER_58NS:
+   case DWC3_GUCTL_REFCLKPER_62NS:
+   break;
+   default:
+   return -EINVAL;
+   }
+
+   reg = dwc3_readl(dwc->regs, DWC3_GFLADJ);
+   reg |= DWC3_GFLADJ_REFCLK_FLADJ;
+   dwc3_writel(dwc->regs, DWC3_GFLADJ, reg);
+   return 0;
+}
+
 /**
  * dwc3_core_init - Low-level initialization of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -969,6 +1002,22 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
}
 
+   /*
+* For peripheral controller, frame number tracking based on reference
+* clock is only introduced after DWC_usb31 version 1.80a.
+*/
+   if (dwc->enable_refclk_sof &&
+   (dwc->dr_mode != USB_DR_MODE_PERIPHERAL ||
+(dwc->dr_mode == USB_DR_MODE_PERIPHERAL &&
+ dwc->revision >= DWC3_USB31_REVISION_180A))) {
+   ret = dwc3_enable_refclk_sof(dwc);
+   if (ret) {
+   dev_err(dwc->dev,
+   "can't enable ref_clk frame tracking\n");
+   goto err4;
+   }
+   }
+
/*
 * ENDXFER polling is available on version 3.10a and later of
 * the DWC_usb3 controller. It is NOT available in the
@@ -1261,6 +1310,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,usb3_lpm_capable");
dwc->usb2_lpm_disable = device_property_read_bool(dev,
"snps,usb2-lpm-disable");
+   dwc->enable_refclk_sof = device_property_read_bool(dev,
+   "snps,enable-refclk-sof");
device_property_read_u8(dev, "snps,refclk-period-ns",
>refclk_period_ns);
device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index e190728104e0..dae2f918a932 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -248,6 +248,14 @@
 /* Global User Control Register */
 #define DWC3_GUCTL_HSTINAUTORETRY  BIT(14)
 #define DWC3_GUCTL_REFCLKPER(n)(((n) & 0x3ff) << 22)
+#define DWC3_GUCTL_GET_REFCLKPER(n)(((n) & (0x3ff << 22)) >> 22)
+
+#define DWC3_GUCTL_REFCLKPER_25NS  25
+#define DWC3_GUCTL_REFCLKPER_41NS  41
+#define DWC3_GUCTL_REFCLKPER_50NS  50
+#define DWC3_GUCTL_REFCLKPER_52NS  52
+#define DWC3_GUCTL_REFCLKPER_58NS  58
+#define DWC3_GUCTL_REFCLKPER_62NS  62
 
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28)
@@ -365,6 +373,7 @@
 #define DWC3_GHWPARAMS7_RAM2_DEPTH(n)  (((n) >> 16) & 0x)
 
 /* Global Frame Length Adjustment Register */
+#define DWC3_GFLADJ_REFCLK_FLADJ   BIT(23)
 #define DWC3_GFLADJ_30MHZ_SDBND_SELBIT(7)
 #define DWC3_GFLADJ_30MHZ_MASK 0x3f
 
@@ -1020,6 +1029,7 @@ struct dwc3_scratchpad_array {
  * check during HS transmit.
  * @refclk_period_ns: if set, inform the controller this value as the reference
  *  

[PATCH v2 2/4] usb: dwc3: Set the reference clock period

[Thinh Nguyen]

This patch writes the reference clock period provided from the device
property to GUCTL.REFCLKPER. This value informs the controller of the
reference clock period if the default Core Consultant setting
GUCTL.REFCLKPER is different.

Typical reference clock periods are 25, 41, 50, 52, 58, and 62ns.

Signed-off-by: Thinh Nguyen 
---
Changes in v2:
- Remove reference clock period validations
- Revise commit message

 drivers/usb/dwc3/core.c | 9 +
 drivers/usb/dwc3/core.h | 4 
 2 files changed, 13 insertions(+)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index a1b126f90261..32c38f71f874 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -962,6 +962,13 @@ static int dwc3_core_init(struct dwc3 *dwc)
goto err4;
}
 
+   if (dwc->refclk_period_ns) {
+   reg = dwc3_readl(dwc->regs, DWC3_GUCTL);
+   reg &= ~DWC3_GUCTL_REFCLKPER(~0);
+   reg |= DWC3_GUCTL_REFCLKPER(dwc->refclk_period_ns);
+   dwc3_writel(dwc->regs, DWC3_GUCTL, reg);
+   }
+
/*
 * ENDXFER polling is available on version 3.10a and later of
 * the DWC_usb3 controller. It is NOT available in the
@@ -1254,6 +1261,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
"snps,usb3_lpm_capable");
dwc->usb2_lpm_disable = device_property_read_bool(dev,
"snps,usb2-lpm-disable");
+   device_property_read_u8(dev, "snps,refclk-period-ns",
+   >refclk_period_ns);
device_property_read_u8(dev, "snps,rx-thr-num-pkt-prd",
_thr_num_pkt_prd);
device_property_read_u8(dev, "snps,rx-max-burst-prd",
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index df876418cb78..e190728104e0 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -247,6 +247,7 @@
 
 /* Global User Control Register */
 #define DWC3_GUCTL_HSTINAUTORETRY  BIT(14)
+#define DWC3_GUCTL_REFCLKPER(n)(((n) & 0x3ff) << 22)
 
 /* Global User Control 1 Register */
 #define DWC3_GUCTL1_TX_IPGAP_LINECHECK_DIS BIT(28)
@@ -1017,6 +1018,8 @@ struct dwc3_scratchpad_array {
  * change quirk.
  * @dis_tx_ipgap_linecheck_quirk: set if we disable u2mac linestate
  * check during HS transmit.
+ * @refclk_period_ns: if set, inform the controller this value as the reference
+ * clock period in nanoseconds.
  * @tx_de_emphasis_quirk: set if we enable Tx de-emphasis quirk
  * @tx_de_emphasis: Tx de-emphasis value
  * 0   - -6dB de-emphasis
@@ -1169,6 +1172,7 @@ struct dwc3 {
u8  rx_max_burst_prd;
u8  tx_thr_num_pkt_prd;
u8  tx_max_burst_prd;
+   u8  refclk_period_ns;
 
const char  *hsphy_interface;
 
-- 
2.11.0

[PATCH v2 3/4] usb: dwc3: Add property snps,enable-refclk-sof

[Thinh Nguyen]

This patch adds a property to enable the controller to track the
frame number based on the reference clock.

When operating in USB 2.0 mode, the peripheral controller uses the USB2
PHY clocks to track the frame number. This prevents the controller from
suspending the USB2 PHY when the device goes into low power. Version
1.80a of the DWC_usb31 peripheral controller introduces a way to track
frame number based on the reference clock instead. This feature allows
the controller to suspend the USB2 PHY when the device goes into low
power. This improves power saving for devices that have isochronous
endpoints.

Signed-off-by: Thinh Nguyen 
---
Changes in v2:
- Revise property description
- Rename property from snps,enable-refclk-lpm to snps,enable-refclk-sof

 Documentation/devicetree/bindings/usb/dwc3.txt | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
b/Documentation/devicetree/bindings/usb/dwc3.txt
index b7e67edff9b2..01b948fff0eb 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -101,6 +101,9 @@ Optional properties:
enable periodic ESS TX threshold.
  - snps,refclk-period-ns: if set, this value informs the controller of the
reference clock period in nanoseconds.
+ - snps,enable-refclk-sof: set to enable reference clock based frame number
+   tracking while in low power, allowing the controller to
+   suspend the PHY during low power states.
 
  -  tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
-- 
2.11.0

[PATCH v2 1/4] usb: dwc3: Add property snps,refclk-period-ns

[Thinh Nguyen]

This patch introduces property "snps,refclk-period-ns" to inform the
controller of the reference clock period. If the reference clock period
is different from the default Core Consultant setting, then this
property can be set to the reference clock period.

This property does not control the reference clock rate. The controller
uses this value to perform internal timing calculations that are based
on the reference clock.

Signed-off-by: Thinh Nguyen 
---
Changes in v2:
- Split from "usb: dwc3: Add reference clock properties"
- Revise commit message and property description

 Documentation/devicetree/bindings/usb/dwc3.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
b/Documentation/devicetree/bindings/usb/dwc3.txt
index 8e5265e9f658..b7e67edff9b2 100644
--- a/Documentation/devicetree/bindings/usb/dwc3.txt
+++ b/Documentation/devicetree/bindings/usb/dwc3.txt
@@ -99,6 +99,8 @@ Optional properties:
this and tx-thr-num-pkt-prd to a valid, non-zero value
1-16 (DWC_usb31 programming guide section 1.2.3) to
enable periodic ESS TX threshold.
+ - snps,refclk-period-ns: if set, this value informs the controller of the
+   reference clock period in nanoseconds.
 
  -  tx-fifo-resize: determines if the FIFO *has* to be reallocated.
  - snps,incr-burst-type-adjustment: Value for INCR burst type of GSBUSCFG0
-- 
2.11.0

[PATCH v2 0/4] usb: dwc3: Introduce refclk lpm

[Thinh Nguyen]

This patch series introduce a new feature in DWC_usb31 with more
aggressive low power management using reference clock.

Changes in v2:
- Revise commit messages to correctly describe the feature
- Split previous patch "usb: dwc3: Add reference clock properties" to 2
  separate patches
- Remove reference clock period validation in since DWC_usb3
  can support more periods than DWC_usb31
- Rename property snps,enable-refclk-lpm to snps,enable-refclk-sof


Thinh Nguyen (4):
  usb: dwc3: Add property snps,refclk-period-ns
  usb: dwc3: Set the reference clock period
  usb: dwc3: Add property snps,enable-refclk-sof
  usb: dwc3: Enable frame number tracking based on reference clock

 Documentation/devicetree/bindings/usb/dwc3.txt |  5 +++
 drivers/usb/dwc3/core.c| 60 ++
 drivers/usb/dwc3/core.h| 16 +++
 3 files changed, 81 insertions(+)

-- 
2.11.0

Re: [PATCH 1/3] usb: dwc3: Add reference clock properties

[Thinh Nguyen]

Hi Felipe,

On 11/9/2018 12:54 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen  writes:
>>> Thinh Nguyen  writes:
> Thinh Nguyen  writes:
>> diff --git a/Documentation/devicetree/bindings/usb/dwc3.txt 
>> b/Documentation/devicetree/bindings/usb/dwc3.txt
>> index 636630fb92d7..712b344c3a31 100644
>> --- a/Documentation/devicetree/bindings/usb/dwc3.txt
>> +++ b/Documentation/devicetree/bindings/usb/dwc3.txt
>> @@ -95,6 +95,24 @@ Optional properties:
>>  this and tx-thr-num-pkt-prd to a valid, 
>> non-zero value
>>  1-16 (DWC_usb31 programming guide section 
>> 1.2.3) to
>>  enable periodic ESS TX threshold.
>> + - snps,refclk-period-ns: set to program the reference clock 
>> period. The valid
>> +input periods are as follow:
>> ++-+-+
>> +| Period (ns) | Freq (MHz)  |
>> ++-+-+
>> +| 25  | 39.7/40 |
>> +| 41  | 24.4|
>> +| 50  | 20  |
>> +| 52  | 19.2|
>> +| 58  | 17.2|
>> +| 62  | 16.1|
>> ++-+-+
>> + - snps,enable-refclk-lpm: set to enable low power scheduling of 
>> isochronous
>> +transfers by running SOF/ITP counters using the
>> +reference clock. Only valid for DWC_usb31 
>> peripheral
>> +controller v1.80a and higher. Both
>> +"snps,dis_u2_susphy_quirk" and
>> +"snps,dis_enblslpm_quirk" must not be set.
> sounds like you should rely on clk API here. Then on driver call
> clk_get_rate() to computer whatever you need to compute.
>
 There's nothing to compute here. We can simply enable this feature with
 "snps, enable-refclk-lpm" and the controller will use the default 
 refclk
 settings.
>>> Right, right. What I'm saying, though, is that we have a clock API for
>>> describing a clock. So why wouldn't we rely on that API for this? I
>>> think both of these new properties can be replaced with standard clock
>>> API properties:
>>>
>>> clocks = <>, ..., <_clk>
>>> clock-names = "clock1", , "lpm";
>>>
>>> Then dwc3 core could, simply, check if we have a clock named "lpm" and
>>> if there is, use NSECS_PER_SEC / clk_get_rate() to get its period and
>>> write it to the register that needs the information.
>> There's no new clock here. We are using the ref_clk for SOF and ITP
>> counter for this feature. Also, clocks are optional on non-DT platforms.
>> To use the clock API, then we need to update the driver to allow some
>> optional clock such as "ref" clock for non-DT platforms. Do you want to
>> do it like this?
> I can't think of a problem that would arise from that. Can you? Mark,
> Rob, what do you think?
>
 No problem. That can be done. This will remove the
 "snps,refclk-period-ns" property. But we should have
 "snps,enable-refclk-lpm" to enable this feature.
>>> not really. Just check if you have a clock named lpm. If you do, then
>>> you enable the feature.
>>>
>> But this clock name should be "ref".  The new name "lpm" would make it
>> seem like it's a different clock.
> now I understand. There's no special LPM clock, this is just the regular
> old reference clock being used for LPM. I agree with you, only
> refclk-period-ns will be replaced.
>

I had some misunderstanding with the purpose of GUCTL.REFCLKPER. After a
discussion with the RTL engineers, it's not to control the reference
clock. Setting this register field does not control the reference clock
rate. The controller uses this value to perform internal timing
calculations that are based on the reference clock. So, we will still
need this property to set this value. I will resend the patch series
with some changes and correct the commit messages with the proper
definition of this feature.

Thanks,
Thinh

Re: [PATCH v3 2/2] usb: dwc3: gadget: Report isoc transfer frame number

[Thinh Nguyen]

Hi Felipe,

On 12/6/2018 10:01 PM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen  writes:
 diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
 index 679c12e14522..2de563124fc1 100644
 --- a/drivers/usb/dwc3/gadget.c
 +++ b/drivers/usb/dwc3/gadget.c
 @@ -2254,6 +2254,19 @@ static int 
 dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
  
 +  /*
 +   * For isochronous transfers, the first TRB in a service interval must
 +   * have the Isoc-First type. Track and report its interval frame number.
 +   */
 +  if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
 +  (trb->ctrl & DWC3_TRBCTL_ISOCHRONOUS_FIRST)) {
 +  unsigned int frame_number;
 +
 +  frame_number = DWC3_TRB_CTRL_GET_SID_SOFN(trb->ctrl);
 +  frame_number &= ~(dep->interval - 1);
 +  req->request.frame_number = frame_number;
 +  }
>>> could you refresh my memory on how this is going to be used?
>>>
>> Sure. This informs the upper layer driver what interval the isoc request
>> went out. The users can use this information for applications that
>> require synchronization with the host.
> Thanks. Do you have an example of a gadget that would use it? Perhaps
> g_webcam can rely on this to improve its scheduling? I have this in my
> testing/next already, just looking for an actual user :-)
>

One of our internal IP validation tools uses this to track the isoc data
against the interval it should be in, and it has its custom protocol for
synchronization with the host. But beside debugging purposes, I can also
see that it can be useful for devices that are time-sensitive. I'm not
familiar with g_webcam, but I can imagine that this option can be useful
for it.

When the user queues for a request, it now has the control of what data
should come next. That is, when the user queues for a request, the user
can get the current frame number by calling usb_gadget_frame_number().
Base on the current frame number and what interval the request went out,
the user can determine what data should be queued next and discard any
stale ones.  Without this usb_request->frame_number, the user will not
know how many intervals the request is delayed by.

Thinh

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Rob Herring]

On Wed, Dec 05, 2018 at 07:57:37AM +, PETER CHEN wrote:
>  
> > On 04.12.18 21:01, Fabio Estevam wrote:
> > > Hi Frieder,
> > >
> > > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> > >  wrote:
> > >
> > >> There are many other optional properties for this driver and a lot of
> > >> them are not in the given example. Maybe we should just keep the
> > >> pinctrls for HSIC-mode out of the example, too?
> > >
> > > I am just trying to make life easier for those who want to use HSIC
> > > support with chipidea.
> > >
> > > Can we just add a real dts snippet example of your board into the
> > > binding document?
> > 
> > Sure, here is what I have in my dts:
> > 
> >  {
> > pinctrl-names = "idle", "active";
> > pinctrl-0 = <_usbh2_idle>;
> > pinctrl-1 = <_usbh2_active>;
> > status = "okay";
> > #address-cells = <1>;
> > #size-cells = <0>;
> > 
> > usbnet: smsc@1 {
> > compatible = "usb424,9730";
> > reg = <1>;
> > };
> > };
> > 
> > @Peter: Can you add this as a second example to the binding documentation?
> > 
> 
> So, there are two examples at binding-doc, one for normal, one for HSIC? 
> Fabio, do you
> mean that? If DT maintainer agrees it too, I will add it.

Okay.

Urgently need money? We can help you!

[Mr. Muller Dieter]

Urgently need money? We can help you!
Are you by the current situation in trouble or threatens you in trouble?
In this way, we give you the ability to take a new development.
As a rich person I feel obliged to assist people who are struggling to give 
them a chance. Everyone deserved a second chance and since the Government 
fails, it will have to come from others.
No amount is too crazy for us and the maturity we determine by mutual agreement.
No surprises, no extra costs, but just the agreed amounts and nothing else.
Don't wait any longer and comment on this post. Please specify the amount you 
want to borrow and we will contact you with all the possibilities. contact us 
today at stewarrt.l...@gmail.com

[PATCH 1/8] xhci: remove the unused sw_lpm_support

[Mathias Nyman]

From: Zeng Tao 

It is introduced for the pre-0.96 xHC controllers, and the driver only
support HW LPM for 1.0 and later controllers.It's not actually used now
and is thought not to be used in the future any more, so just remove it.

Signed-off-by: Zeng Tao 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-mem.c | 20 
 drivers/usb/host/xhci.c |  3 +--
 drivers/usb/host/xhci.h |  2 --
 3 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index b1f27aa..791c5d8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -2181,23 +2181,11 @@ static void xhci_add_in_port(struct xhci_hcd *xhci, 
unsigned int num_ports,
if (major_revision < 0x03 && xhci->num_ext_caps < max_caps)
xhci->ext_caps[xhci->num_ext_caps++] = temp;
 
-   /* Check the host's USB2 LPM capability */
-   if ((xhci->hci_version == 0x96) && (major_revision != 0x03) &&
-   (temp & XHCI_L1C)) {
+   if ((xhci->hci_version >= 0x100) && (major_revision != 0x03) &&
+(temp & XHCI_HLC)) {
xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-   "xHCI 0.96: support USB2 software lpm");
-   xhci->sw_lpm_support = 1;
-   }
-
-   if ((xhci->hci_version >= 0x100) && (major_revision != 0x03)) {
-   xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-   "xHCI 1.0: support USB2 software lpm");
-   xhci->sw_lpm_support = 1;
-   if (temp & XHCI_HLC) {
-   xhci_dbg_trace(xhci, trace_xhci_dbg_init,
-   "xHCI 1.0: support USB2 hardware lpm");
-   xhci->hw_lpm_support = 1;
-   }
+  "xHCI 1.0: support USB2 hardware lpm");
+   xhci->hw_lpm_support = 1;
}
 
port_offset--;
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c928dbb..553e974 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4370,8 +4370,7 @@ static int xhci_update_device(struct usb_hcd *hcd, struct 
usb_device *udev)
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
int portnum = udev->portnum - 1;
 
-   if (hcd->speed >= HCD_USB3 || !xhci->sw_lpm_support ||
-   !udev->lpm_capable)
+   if (hcd->speed >= HCD_USB3 || !udev->lpm_capable)
return 0;
 
/* we only support lpm for non-hub device connected to root hub yet */
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 260b259..59b8562 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1858,8 +1858,6 @@ struct xhci_hcd {
struct xhci_port*hw_ports;
struct xhci_hub usb2_rhub;
struct xhci_hub usb3_rhub;
-   /* support xHCI 0.96 spec USB2 software LPM */
-   unsignedsw_lpm_support:1;
/* support xHCI 1.0 spec USB2 hardware LPM */
unsignedhw_lpm_support:1;
/* cached usb2 extened protocol capabilites */
-- 
2.7.4

[PATCH 4/8] xhci: move usb3 speficic bits to own function in get_port_status call

[Mathias Nyman]

refactoring, no functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 68 ++---
 1 file changed, 40 insertions(+), 28 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 5dba0a4..60aeff3 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -818,6 +818,41 @@ static u32 xhci_get_ext_port_status(u32 raw_port_status, 
u32 port_li)
return ext_stat;
 }
 
+static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
+ u32 portsc)
+{
+   struct xhci_hcd *xhci;
+   u32 link_state;
+   u32 portnum;
+
+   xhci = hcd_to_xhci(port->rhub->hcd);
+   link_state = portsc & PORT_PLS_MASK;
+   portnum = port->hcd_portnum;
+
+   /* USB3 specific wPortChange bits
+*
+* Port link change with port in resume state should not be
+* reported to usbcore, as this is an internal state to be
+* handled by xhci driver. Reporting PLC to usbcore may
+* cause usbcore clearing PLC first and port change event
+* irq won't be generated.
+*/
+
+   if (portsc & PORT_PLC && (link_state != XDEV_RESUME))
+   *status |= USB_PORT_STAT_C_LINK_STATE << 16;
+   if (portsc & PORT_WRC)
+   *status |= USB_PORT_STAT_C_BH_RESET << 16;
+   if (portsc & PORT_CEC)
+   *status |= USB_PORT_STAT_C_CONFIG_ERROR << 16;
+
+   /* USB3 specific wPortStatus bits */
+   if (portsc & PORT_POWER)
+   *status |= USB_SS_PORT_STAT_POWER;
+
+   xhci_hub_report_usb3_link_state(xhci, status, portsc);
+   xhci_del_comp_mod_timer(xhci, portsc, portnum);
+}
+
 /*
  * Converts a raw xHCI port status into the format that external USB 2.0 or USB
  * 3.0 hubs use.
@@ -854,22 +889,8 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
if ((raw_port_status & PORT_RC))
status |= USB_PORT_STAT_C_RESET << 16;
/* USB3.0 only */
-   if (hcd->speed >= HCD_USB3) {
-   /* Port link change with port in resume state should not be
-* reported to usbcore, as this is an internal state to be
-* handled by xhci driver. Reporting PLC to usbcore may
-* cause usbcore clearing PLC first and port change event
-* irq won't be generated.
-*/
-   if ((raw_port_status & PORT_PLC) &&
-   (raw_port_status & PORT_PLS_MASK) != XDEV_RESUME)
-   status |= USB_PORT_STAT_C_LINK_STATE << 16;
-   if ((raw_port_status & PORT_WRC))
-   status |= USB_PORT_STAT_C_BH_RESET << 16;
-   if ((raw_port_status & PORT_CEC))
-   status |= USB_PORT_STAT_C_CONFIG_ERROR << 16;
-   }
-
+   if (hcd->speed >= HCD_USB3)
+   xhci_get_usb3_port_status(port, , raw_port_status);
if (hcd->speed < HCD_USB3) {
if ((raw_port_status & PORT_PLS_MASK) == XDEV_U3
&& (raw_port_status & PORT_POWER))
@@ -989,22 +1010,13 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
if (raw_port_status & PORT_RESET)
status |= USB_PORT_STAT_RESET;
if (raw_port_status & PORT_POWER) {
-   if (hcd->speed >= HCD_USB3)
-   status |= USB_SS_PORT_STAT_POWER;
-   else
+   if (hcd->speed < HCD_USB3)
status |= USB_PORT_STAT_POWER;
}
/* Update Port Link State */
-   if (hcd->speed >= HCD_USB3) {
-   xhci_hub_report_usb3_link_state(xhci, , raw_port_status);
-   /*
-* Verify if all USB3 Ports Have entered U0 already.
-* Delete Compliance Mode Timer if so.
-*/
-   xhci_del_comp_mod_timer(xhci, raw_port_status, wIndex);
-   } else {
+   if (hcd->speed < HCD_USB3)
xhci_hub_report_usb2_link_state(, raw_port_status);
-   }
+
if (bus_state->port_c_suspend & (1 << wIndex))
status |= USB_PORT_STAT_C_SUSPEND << 16;
 
-- 
2.7.4

[PATCH 6/8] xhci: cleanup code that sets portstatus and portchange bits

[Mathias Nyman]

Group the code where the wPortstatus and wPortChange bits
are set into one place.

No functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 5af4f90..c0de2d0 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -891,7 +891,7 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
rhub = xhci_get_rhub(hcd);
port = rhub->ports[wIndex];
 
-   /* wPortChange bits */
+   /* common wPortChange bits */
if (raw_port_status & PORT_CSC)
status |= USB_PORT_STAT_C_CONNECTION << 16;
if (raw_port_status & PORT_PEC)
@@ -901,7 +901,19 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
if ((raw_port_status & PORT_RC))
status |= USB_PORT_STAT_C_RESET << 16;
 
-   /* USB2 and USB3 specific bits including Port Link State */
+   /* common wPortStatus bits */
+   if (raw_port_status & PORT_CONNECT) {
+   status |= USB_PORT_STAT_CONNECTION;
+   status |= xhci_port_speed(raw_port_status);
+   }
+   if (raw_port_status & PORT_PE)
+   status |= USB_PORT_STAT_ENABLE;
+   if (raw_port_status & PORT_OC)
+   status |= USB_PORT_STAT_OVERCURRENT;
+   if (raw_port_status & PORT_RESET)
+   status |= USB_PORT_STAT_RESET;
+
+   /* USB2 and USB3 specific bits, including Port Link State */
if (hcd->speed >= HCD_USB3)
xhci_get_usb3_port_status(port, , raw_port_status);
else
@@ -1010,16 +1022,6 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
bus_state->resume_done[wIndex] = 0;
clear_bit(wIndex, _state->resuming_ports);
}
-   if (raw_port_status & PORT_CONNECT) {
-   status |= USB_PORT_STAT_CONNECTION;
-   status |= xhci_port_speed(raw_port_status);
-   }
-   if (raw_port_status & PORT_PE)
-   status |= USB_PORT_STAT_ENABLE;
-   if (raw_port_status & PORT_OC)
-   status |= USB_PORT_STAT_OVERCURRENT;
-   if (raw_port_status & PORT_RESET)
-   status |= USB_PORT_STAT_RESET;
 
if (bus_state->port_c_suspend & (1 << wIndex))
status |= USB_PORT_STAT_C_SUSPEND << 16;
-- 
2.7.4

[PATCH 5/8] xhci: move usb2 speficic bits to own function in get_port_status call

[Mathias Nyman]

Mostly refactoring, with the exception that USB_PORT_STAT_L1 link state
is reported if xhci port link is in U2 AND port is powered.

Previously we did not check if the port was powered, but according to
xhci spec 4.19.1.1.6 All the 'Enabled' states, including
USB_PORT_STAT_L1 (U2), U1, U0 and U3 must have Port power bit set.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 44 
 1 file changed, 24 insertions(+), 20 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 60aeff3..5af4f90 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -714,13 +714,6 @@ void xhci_test_and_clear_bit(struct xhci_hcd *xhci, struct 
xhci_port *port,
}
 }
 
-/* Updates Link Status for USB 2.1 port */
-static void xhci_hub_report_usb2_link_state(u32 *status, u32 status_reg)
-{
-   if ((status_reg & PORT_PLS_MASK) == XDEV_U2)
-   *status |= USB_PORT_STAT_L1;
-}
-
 /* Updates Link Status for super Speed port */
 static void xhci_hub_report_usb3_link_state(struct xhci_hcd *xhci,
u32 *status, u32 status_reg)
@@ -853,6 +846,25 @@ static void xhci_get_usb3_port_status(struct xhci_port 
*port, u32 *status,
xhci_del_comp_mod_timer(xhci, portsc, portnum);
 }
 
+static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
+ u32 portsc)
+{
+   u32 link_state;
+
+   link_state = portsc & PORT_PLS_MASK;
+
+   /* USB2 wPortStatus bits */
+   if (portsc & PORT_POWER) {
+   *status |= USB_PORT_STAT_POWER;
+
+   /* link state is only valid if port is powered */
+   if (link_state == XDEV_U3)
+   *status |= USB_PORT_STAT_SUSPEND;
+   if (link_state == XDEV_U2)
+   *status |= USB_PORT_STAT_L1;
+   }
+}
+
 /*
  * Converts a raw xHCI port status into the format that external USB 2.0 or USB
  * 3.0 hubs use.
@@ -888,14 +900,13 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
status |= USB_PORT_STAT_C_OVERCURRENT << 16;
if ((raw_port_status & PORT_RC))
status |= USB_PORT_STAT_C_RESET << 16;
-   /* USB3.0 only */
+
+   /* USB2 and USB3 specific bits including Port Link State */
if (hcd->speed >= HCD_USB3)
xhci_get_usb3_port_status(port, , raw_port_status);
-   if (hcd->speed < HCD_USB3) {
-   if ((raw_port_status & PORT_PLS_MASK) == XDEV_U3
-   && (raw_port_status & PORT_POWER))
-   status |= USB_PORT_STAT_SUSPEND;
-   }
+   else
+   xhci_get_usb2_port_status(port, , raw_port_status);
+
if ((raw_port_status & PORT_PLS_MASK) == XDEV_RESUME &&
!DEV_SUPERSPEED_ANY(raw_port_status) && hcd->speed < HCD_USB3) {
if ((raw_port_status & PORT_RESET) ||
@@ -1009,13 +1020,6 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
status |= USB_PORT_STAT_OVERCURRENT;
if (raw_port_status & PORT_RESET)
status |= USB_PORT_STAT_RESET;
-   if (raw_port_status & PORT_POWER) {
-   if (hcd->speed < HCD_USB3)
-   status |= USB_PORT_STAT_POWER;
-   }
-   /* Update Port Link State */
-   if (hcd->speed < HCD_USB3)
-   xhci_hub_report_usb2_link_state(, raw_port_status);
 
if (bus_state->port_c_suspend & (1 << wIndex))
status |= USB_PORT_STAT_C_SUSPEND << 16;
-- 
2.7.4

[PATCH 8/8] xhci: move usb2 get port status link resume handling to its own function

[Mathias Nyman]

Refactoring, no functional changes.

But worth mentioning that checking for port link resume state is now behind
a additional port power check.

This is fine as ports can't be in resume state if port power bit is not
set.

xhci spec section 4.19.1.1.6 figure 34 shows that port power bit must be
set for all 'Enable' substates, including U0,U1,U2,U3 (suspended), Resume,
and RExit states.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 188 
 1 file changed, 104 insertions(+), 84 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index d86d1d5..c760175 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -795,6 +795,100 @@ static void xhci_del_comp_mod_timer(struct xhci_hcd 
*xhci, u32 status,
}
 }
 
+static int xhci_handle_usb2_port_link_resume(struct xhci_port *port,
+u32 *status, u32 portsc,
+unsigned long flags)
+{
+   struct xhci_bus_state *bus_state;
+   struct xhci_hcd *xhci;
+   struct usb_hcd *hcd;
+   int slot_id;
+   u32 wIndex;
+
+   hcd = port->rhub->hcd;
+   bus_state = >rhub->bus_state;
+   xhci = hcd_to_xhci(hcd);
+   wIndex = port->hcd_portnum;
+
+   if ((portsc & PORT_RESET) || !(portsc & PORT_PE)) {
+   *status = 0x;
+   return -EINVAL;
+   }
+   /* did port event handler already start resume timing? */
+   if (!bus_state->resume_done[wIndex]) {
+   /* If not, maybe we are in a host initated resume? */
+   if (test_bit(wIndex, _state->resuming_ports)) {
+   /* Host initated resume doesn't time the resume
+* signalling using resume_done[].
+* It manually sets RESUME state, sleeps 20ms
+* and sets U0 state. This should probably be
+* changed, but not right now.
+*/
+   } else {
+   /* port resume was discovered now and here,
+* start resume timing
+*/
+   unsigned long timeout = jiffies +
+   msecs_to_jiffies(USB_RESUME_TIMEOUT);
+
+   set_bit(wIndex, _state->resuming_ports);
+   bus_state->resume_done[wIndex] = timeout;
+   mod_timer(>rh_timer, timeout);
+   usb_hcd_start_port_resume(>self, wIndex);
+   }
+   /* Has resume been signalled for USB_RESUME_TIME yet? */
+   } else if (time_after_eq(jiffies, bus_state->resume_done[wIndex])) {
+   int time_left;
+
+   xhci_dbg(xhci, "Resume USB2 port %d\n", wIndex + 1);
+   bus_state->resume_done[wIndex] = 0;
+   clear_bit(wIndex, _state->resuming_ports);
+
+   set_bit(wIndex, _state->rexit_ports);
+
+   xhci_test_and_clear_bit(xhci, port, PORT_PLC);
+   xhci_set_link_state(xhci, port, XDEV_U0);
+
+   spin_unlock_irqrestore(>lock, flags);
+   time_left = wait_for_completion_timeout(
+   _state->rexit_done[wIndex],
+   msecs_to_jiffies(XHCI_MAX_REXIT_TIMEOUT_MS));
+   spin_lock_irqsave(>lock, flags);
+
+   if (time_left) {
+   slot_id = xhci_find_slot_id_by_port(hcd, xhci,
+   wIndex + 1);
+   if (!slot_id) {
+   xhci_dbg(xhci, "slot_id is zero\n");
+   *status = 0x;
+   return -ENODEV;
+   }
+   xhci_ring_device(xhci, slot_id);
+   } else {
+   int port_status = readl(port->addr);
+
+   xhci_warn(xhci, "Port resume %i msec timed out, portsc 
= 0x%x\n",
+ XHCI_MAX_REXIT_TIMEOUT_MS,
+ port_status);
+   *status |= USB_PORT_STAT_SUSPEND;
+   clear_bit(wIndex, _state->rexit_ports);
+   }
+
+   usb_hcd_end_port_resume(>self, wIndex);
+   bus_state->port_c_suspend |= 1 << wIndex;
+   bus_state->suspended_ports &= ~(1 << wIndex);
+   } else {
+   /*
+* The resume has been signaling for less than
+* USB_RESUME_TIME. Report the port status as SUSPEND,
+* let the usbcore check port status again and clear
+* resume signaling later.
+*/
+   *status |= USB_PORT_STAT_SUSPEND;
+   }
+   return 0;
+}
+
 static u32 xhci_get_ext_port_status(u32 raw_port_status, u32 port_li)
 {
 

[PATCH 0/8] xhci features for usb-next

[Mathias Nyman]

Hi Greg

This series for usb-next mostly about refactoring the xhci roothub
side of the get_port_status request

-Mathias

Mathias Nyman (7):
  xhci: move bus_state structure under the xhci_hub structure.
  xhci: remove unused hcd_index()
  xhci: move usb3 speficic bits to own function in get_port_status call
  xhci: move usb2 speficic bits to own function in get_port_status call
  xhci: cleanup code that sets portstatus and portchange bits
  xhci: refactor U0 link state handling in get_port_status
  xhci: move usb2 get port status link resume handling to its own
function

Zeng Tao (1):
  xhci: remove the unused sw_lpm_support

 drivers/usb/host/xhci-hub.c  | 351 ---
 drivers/usb/host/xhci-mem.c  |  30 ++--
 drivers/usb/host/xhci-ring.c |   2 +-
 drivers/usb/host/xhci.c  |  22 +--
 drivers/usb/host/xhci.h  |  13 +-
 5 files changed, 219 insertions(+), 199 deletions(-)

-- 
2.7.4

[PATCH 2/8] xhci: move bus_state structure under the xhci_hub structure.

[Mathias Nyman]

Move the bus_state structure under struct usb_hub.

We need a bus_state strucure for each roothub to keep track of suspend
related info for each port.
Instead of keeping an array of two bus_state structures right under
struct xhci, it makes more sense move them to the xhci_hub structure.

No functional changes.

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c  | 15 ++-
 drivers/usb/host/xhci-mem.c  | 10 +-
 drivers/usb/host/xhci-ring.c |  2 +-
 drivers/usb/host/xhci.c  | 19 ++-
 drivers/usb/host/xhci.h  |  4 ++--
 5 files changed, 24 insertions(+), 26 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index 94aca1b..5dba0a4 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -1031,7 +1031,7 @@ int xhci_hub_control(struct usb_hcd *hcd, u16 typeReq, 
u16 wValue,
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
-   bus_state = >bus_state[hcd_index(hcd)];
+   bus_state = >bus_state;
 
spin_lock_irqsave(>lock, flags);
switch (typeReq) {
@@ -1421,7 +1421,7 @@ int xhci_hub_status_data(struct usb_hcd *hcd, char *buf)
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
-   bus_state = >bus_state[hcd_index(hcd)];
+   bus_state = >bus_state;
 
/* Initial status is no changes */
retval = (max_ports + 8) / 8;
@@ -1480,7 +1480,7 @@ int xhci_bus_suspend(struct usb_hcd *hcd)
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
-   bus_state = >bus_state[hcd_index(hcd)];
+   bus_state = >bus_state;
wake_enabled = hcd->self.root_hub->do_remote_wakeup;
 
spin_lock_irqsave(>lock, flags);
@@ -1622,7 +1622,7 @@ int xhci_bus_resume(struct usb_hcd *hcd)
rhub = xhci_get_rhub(hcd);
ports = rhub->ports;
max_ports = rhub->num_ports;
-   bus_state = >bus_state[hcd_index(hcd)];
+   bus_state = >bus_state;
 
if (time_before(jiffies, bus_state->next_statechange))
msleep(5);
@@ -1723,13 +1723,10 @@ int xhci_bus_resume(struct usb_hcd *hcd)
 
 unsigned long xhci_get_resuming_ports(struct usb_hcd *hcd)
 {
-   struct xhci_hcd *xhci = hcd_to_xhci(hcd);
-   struct xhci_bus_state *bus_state;
-
-   bus_state = >bus_state[hcd_index(hcd)];
+   struct xhci_hub *rhub = xhci_get_rhub(hcd);
 
/* USB3 port wakeups are reported via usb_wakeup_notification() */
-   return bus_state->resuming_ports;   /* USB2 ports only */
+   return rhub->bus_state.resuming_ports;  /* USB2 ports only */
 }
 
 #endif /* CONFIG_PM */
diff --git a/drivers/usb/host/xhci-mem.c b/drivers/usb/host/xhci-mem.c
index 791c5d8..36a3eb8 100644
--- a/drivers/usb/host/xhci-mem.c
+++ b/drivers/usb/host/xhci-mem.c
@@ -1922,8 +1922,8 @@ void xhci_mem_cleanup(struct xhci_hcd *xhci)
 
xhci->page_size = 0;
xhci->page_shift = 0;
-   xhci->bus_state[0].bus_suspended = 0;
-   xhci->bus_state[1].bus_suspended = 0;
+   xhci->usb2_rhub.bus_state.bus_suspended = 0;
+   xhci->usb3_rhub.bus_state.bus_suspended = 0;
 }
 
 static int xhci_test_trb_in_td(struct xhci_hcd *xhci,
@@ -2524,10 +2524,10 @@ int xhci_mem_init(struct xhci_hcd *xhci, gfp_t flags)
for (i = 0; i < MAX_HC_SLOTS; i++)
xhci->devs[i] = NULL;
for (i = 0; i < USB_MAXCHILDREN; i++) {
-   xhci->bus_state[0].resume_done[i] = 0;
-   xhci->bus_state[1].resume_done[i] = 0;
+   xhci->usb2_rhub.bus_state.resume_done[i] = 0;
+   xhci->usb3_rhub.bus_state.resume_done[i] = 0;
/* Only the USB 2.0 completions will ever be used. */
-   init_completion(>bus_state[1].rexit_done[i]);
+   init_completion(>usb2_rhub.bus_state.rexit_done[i]);
}
 
if (scratchpad_alloc(xhci, flags))
diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
index 6575058..40fa25c 100644
--- a/drivers/usb/host/xhci-ring.c
+++ b/drivers/usb/host/xhci-ring.c
@@ -1593,7 +1593,7 @@ static void handle_port_status(struct xhci_hcd *xhci,
}
 
hcd = port->rhub->hcd;
-   bus_state = >bus_state[hcd_index(hcd)];
+   bus_state = >rhub->bus_state;
hcd_portnum = port->hcd_portnum;
portsc = readl(port->addr);
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 553e974..6631e7f 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -169,7 +169,7 @@ int xhci_reset(struct xhci_hcd *xhci)
 {
u32 command;
u32 state;
-   int ret, i;
+   int ret;
 
state = readl(>op_regs->status);
 
@@ -215,11 +215,12 @@ int xhci_reset(struct xhci_hcd *xhci)
ret = xhci_handshake(>op_regs->status,
STS_CNR, 0, 10 * 1000 * 1000);
 
-   for (i = 0; i < 2; i++) {
-   

[PATCH 3/8] xhci: remove unused hcd_index()

[Mathias Nyman]

Now that each root hub has their own bus_state strucure the
hcd_undex() used to get the correct bus_state strucure is
no longer needed.

No functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.h | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index b57b793..3c6b504 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1682,13 +1682,6 @@ struct xhci_bus_state {
  */
 #defineXHCI_MAX_REXIT_TIMEOUT_MS   20
 
-static inline unsigned int hcd_index(struct usb_hcd *hcd)
-{
-   if (hcd->speed >= HCD_USB3)
-   return 0;
-   else
-   return 1;
-}
 struct xhci_port {
__le32 __iomem  *addr;
int hw_portnum;
-- 
2.7.4

[PATCH 7/8] xhci: refactor U0 link state handling in get_port_status

[Mathias Nyman]

Move U0 link state handing to USB3 and USB2 specific functions

Note that
bus_state->resuming_ports:
bus_state->resume_done[]:
are only used for USB2, and don't need to cleared for USB3 ports

No functional changes

Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-hub.c | 32 +++-
 1 file changed, 19 insertions(+), 13 deletions(-)

diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
index c0de2d0..d86d1d5 100644
--- a/drivers/usb/host/xhci-hub.c
+++ b/drivers/usb/host/xhci-hub.c
@@ -814,10 +814,12 @@ static u32 xhci_get_ext_port_status(u32 raw_port_status, 
u32 port_li)
 static void xhci_get_usb3_port_status(struct xhci_port *port, u32 *status,
  u32 portsc)
 {
+   struct xhci_bus_state *bus_state;
struct xhci_hcd *xhci;
u32 link_state;
u32 portnum;
 
+   bus_state = >rhub->bus_state;
xhci = hcd_to_xhci(port->rhub->hcd);
link_state = portsc & PORT_PLS_MASK;
portnum = port->hcd_portnum;
@@ -839,8 +841,12 @@ static void xhci_get_usb3_port_status(struct xhci_port 
*port, u32 *status,
*status |= USB_PORT_STAT_C_CONFIG_ERROR << 16;
 
/* USB3 specific wPortStatus bits */
-   if (portsc & PORT_POWER)
+   if (portsc & PORT_POWER) {
*status |= USB_SS_PORT_STAT_POWER;
+   /* link state handling */
+   if (link_state == XDEV_U0)
+   bus_state->suspended_ports &= ~(1 << portnum);
+   }
 
xhci_hub_report_usb3_link_state(xhci, status, portsc);
xhci_del_comp_mod_timer(xhci, portsc, portnum);
@@ -849,9 +855,13 @@ static void xhci_get_usb3_port_status(struct xhci_port 
*port, u32 *status,
 static void xhci_get_usb2_port_status(struct xhci_port *port, u32 *status,
  u32 portsc)
 {
+   struct xhci_bus_state *bus_state;
u32 link_state;
+   u32 portnum;
 
+   bus_state = >rhub->bus_state;
link_state = portsc & PORT_PLS_MASK;
+   portnum = port->hcd_portnum;
 
/* USB2 wPortStatus bits */
if (portsc & PORT_POWER) {
@@ -862,6 +872,14 @@ static void xhci_get_usb2_port_status(struct xhci_port 
*port, u32 *status,
*status |= USB_PORT_STAT_SUSPEND;
if (link_state == XDEV_U2)
*status |= USB_PORT_STAT_L1;
+   if (link_state == XDEV_U0) {
+   bus_state->resume_done[portnum] = 0;
+   clear_bit(portnum, _state->resuming_ports);
+   if (bus_state->suspended_ports & (1 << portnum)) {
+   bus_state->suspended_ports &= ~(1 << portnum);
+   bus_state->port_c_suspend |= 1 << portnum;
+   }
+   }
}
 }
 
@@ -1011,18 +1029,6 @@ static u32 xhci_get_port_status(struct usb_hcd *hcd,
usb_hcd_end_port_resume(>self, wIndex);
}
 
-
-   if ((raw_port_status & PORT_PLS_MASK) == XDEV_U0 &&
-   (raw_port_status & PORT_POWER)) {
-   if (bus_state->suspended_ports & (1 << wIndex)) {
-   bus_state->suspended_ports &= ~(1 << wIndex);
-   if (hcd->speed < HCD_USB3)
-   bus_state->port_c_suspend |= 1 << wIndex;
-   }
-   bus_state->resume_done[wIndex] = 0;
-   clear_bit(wIndex, _state->resuming_ports);
-   }
-
if (bus_state->port_c_suspend & (1 << wIndex))
status |= USB_PORT_STAT_C_SUSPEND << 16;
 
-- 
2.7.4

Re: [PATCH 0/3] Small improvements to the appledisplay driver

[2...@mok.nu]

> On 5 Dec 2018, at 10:42, Greg Kroah-Hartman  
> wrote:
>> On Tue, Dec 04, 2018 at 11:43:34PM +0100, alex.theis...@me.com wrote:
>> With the exception of the first patch I am not entirely sure if my changes 
>> are
>> correct and justified. This is my first contribution and I am equally 
>> satisfied
>> if someone could point out why my changes are not correct.
>> 
>> The added display is my own and works well with the driver.
> 
> All looks good to me, thanks for the patches!
> 
> greg k-h

I've tested patch 2/3 and 3/3 applied together on top of the upstream 
appledisplay driver. They work well for me. I can offer a;

Tested-by: Mattias Jacobsson <2...@mok.nu>

for them if it isn't too late.

Thanks,
Mattias

Re: [PATCH v3 2/2] usb: dwc3: gadget: Report isoc transfer frame number

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
>>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>>> index 679c12e14522..2de563124fc1 100644
>>> --- a/drivers/usb/dwc3/gadget.c
>>> +++ b/drivers/usb/dwc3/gadget.c
>>> @@ -2254,6 +2254,19 @@ static int 
>>> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>> if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
>>> trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>>>  
>>> +   /*
>>> +* For isochronous transfers, the first TRB in a service interval must
>>> +* have the Isoc-First type. Track and report its interval frame number.
>>> +*/
>>> +   if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
>>> +   (trb->ctrl & DWC3_TRBCTL_ISOCHRONOUS_FIRST)) {
>>> +   unsigned int frame_number;
>>> +
>>> +   frame_number = DWC3_TRB_CTRL_GET_SID_SOFN(trb->ctrl);
>>> +   frame_number &= ~(dep->interval - 1);
>>> +   req->request.frame_number = frame_number;
>>> +   }
>> could you refresh my memory on how this is going to be used?
>>
>
> Sure. This informs the upper layer driver what interval the isoc request
> went out. The users can use this information for applications that
> require synchronization with the host.

Thanks. Do you have an example of a gadget that would use it? Perhaps
g_webcam can rely on this to improve its scheduling? I have this in my
testing/next already, just looking for an actual user :-)

-- 
balbi

Re: [balbi-usb:testing/next 50/50] drivers/usb/dwc3/./trace.h:216:449: warning: 's' may be used uninitialized in this function

[Felipe Balbi]

Hi,

kbuild test robot  writes:

> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
> testing/next
> head:   ad7b607f82731eec3ed17d9d22764eb6f09609f9
> commit: ad7b607f82731eec3ed17d9d22764eb6f09609f9 [50/50] usb: dwc3: trace: 
> add missing break statement to make compiler happy
> config: i386-allyesconfig (attached as .config)
> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
> reproduce:
> git checkout ad7b607f82731eec3ed17d9d22764eb6f09609f9
> # save the attached .config to linux build tree
> make ARCH=i386 
>
> Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
> http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings
>
> All warnings (new ones prefixed by >>):
>
>In file included from include/trace/trace_events.h:394:0,
> from include/trace/define_trace.h:96,
> from drivers/usb/dwc3/trace.h:342,
> from drivers/usb/dwc3/trace.c:11:
>drivers/usb/dwc3/./trace.h: In function 'trace_raw_output_dwc3_log_trb':
>>> drivers/usb/dwc3/./trace.h:216:449: warning: 's' may be used uninitialized 
>>> in this function [-Wmaybe-uninitialized]
> DECLARE_EVENT_CLASS(dwc3_log_trb,
>
>drivers/usb/dwc3/./trace.h:216:647: note: 's' was declared here
> DECLARE_EVENT_CLASS(dwc3_log_trb,
>
>
> vim +/s +216 drivers/usb/dwc3/./trace.h
>
> 2c4cbe6e5a Felipe Balbi2014-04-30  215  
> 2c4cbe6e5a Felipe Balbi2014-04-30 @216  DECLARE_EVENT_CLASS(dwc3_log_trb,
> 2c4cbe6e5a Felipe Balbi2014-04-30  217TP_PROTO(struct dwc3_ep *dep, 
> struct dwc3_trb *trb),
> 2c4cbe6e5a Felipe Balbi2014-04-30  218TP_ARGS(dep, trb),
> 2c4cbe6e5a Felipe Balbi2014-04-30  219TP_STRUCT__entry(
> e42f09b85f Felipe Balbi2017-04-28  220__string(name, 
> dep->name)
> 2c4cbe6e5a Felipe Balbi2014-04-30  221__field(struct dwc3_trb 
> *, trb)
> 68d34c8a74 Felipe Balbi2016-05-30  222__field(u32, allocated)
> 68d34c8a74 Felipe Balbi2016-05-30  223__field(u32, queued)
> 4ac4fc9322 Felipe Balbi2014-09-17  224__field(u32, bpl)
> 4ac4fc9322 Felipe Balbi2014-09-17  225__field(u32, bph)
> 4ac4fc9322 Felipe Balbi2014-09-17  226__field(u32, size)
> 4ac4fc9322 Felipe Balbi2014-09-17  227__field(u32, ctrl)
> fa8d965d73 Felipe Balbi2016-09-28  228__field(u32, type)
> 2c4cbe6e5a Felipe Balbi2014-04-30  229),
> 2c4cbe6e5a Felipe Balbi2014-04-30  230TP_fast_assign(
> e42f09b85f Felipe Balbi2017-04-28  231__assign_str(name, 
> dep->name);
> 2c4cbe6e5a Felipe Balbi2014-04-30  232__entry->trb = trb;
> 4ac4fc9322 Felipe Balbi2014-09-17  233__entry->bpl = trb->bpl;
> 4ac4fc9322 Felipe Balbi2014-09-17  234__entry->bph = trb->bph;
> 4ac4fc9322 Felipe Balbi2014-09-17  235__entry->size = 
> trb->size;
> 4ac4fc9322 Felipe Balbi2014-09-17  236__entry->ctrl = 
> trb->ctrl;
> fa8d965d73 Felipe Balbi2016-09-28  237__entry->type = 
> usb_endpoint_type(dep->endpoint.desc);
> 2c4cbe6e5a Felipe Balbi2014-04-30  238),
> 0bd0f6d201 Felipe Balbi2018-03-26  239TP_printk("%s: trb %p buf 
> %08x%08x size %s%d ctrl %08x (%c%c%c%c:%c%c:%s)",
> 0bd0f6d201 Felipe Balbi2018-03-26  240__get_str(name), 
> __entry->trb, __entry->bph, __entry->bpl,
> fa8d965d73 Felipe Balbi2016-09-28  241({char *s;
> fa8d965d73 Felipe Balbi2016-09-28  242int pcm = 
> ((__entry->size >> 24) & 3) + 1;
> fa8d965d73 Felipe Balbi2016-09-28  243switch (__entry->type) {
> fa8d965d73 Felipe Balbi2016-09-28  244case 
> USB_ENDPOINT_XFER_INT:
> fa8d965d73 Felipe Balbi2016-09-28  245case 
> USB_ENDPOINT_XFER_ISOC:
> fa8d965d73 Felipe Balbi2016-09-28  246switch (pcm) {
> fa8d965d73 Felipe Balbi2016-09-28  247case 1:
> fa8d965d73 Felipe Balbi2016-09-28  248s = "1x 
> ";
> fa8d965d73 Felipe Balbi2016-09-28  249break;
> fa8d965d73 Felipe Balbi2016-09-28  250case 2:
> fa8d965d73 Felipe Balbi2016-09-28  251s = "2x 
> ";
> fa8d965d73 Felipe Balbi2016-09-28  252break;
> fa8d965d73 Felipe Balbi2016-09-28  253case 3:

false positive. PCM can only be 1 2 or 3. I'll just add a default line
here after case 3 to license the warning.

-- 
balbi

Re: [PATCH -next] usb: mtu3: fix dbginfo in qmu_tx_zlp_error_handler

[Chunfeng Yun]

Hi Haibing,

On Fri, 2018-12-07 at 03:52 +, YueHaibing wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
> 
> drivers/usb/mtu3/mtu3_qmu.c: In function 'qmu_tx_zlp_error_handler':
> drivers/usb/mtu3/mtu3_qmu.c:385:22: warning:
>  variable 'req' set but not used [-Wunused-but-set-variable]
> 
> It seems dbginfo original intention is print 'req' other than 'mreq'
> 
> Signed-off-by: YueHaibing 
> ---
>  drivers/usb/mtu3/mtu3_qmu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
> index 73ac042..09f19f7 100644
> --- a/drivers/usb/mtu3/mtu3_qmu.c
> +++ b/drivers/usb/mtu3/mtu3_qmu.c
> @@ -402,7 +402,7 @@ static void qmu_tx_zlp_error_handler(struct mtu3 *mtu, u8 
> epnum)
>   return;
>   }
>  
> - dev_dbg(mtu->dev, "%s send ZLP for req=%p\n", __func__, mreq);
> + dev_dbg(mtu->dev, "%s send ZLP for req=%p\n", __func__, req);
>  
>   mtu3_clrbits(mbase, MU3D_EP_TXCR0(mep->epnum), TX_DMAREQEN);
> 
Acked-by: Chunfeng Yun 

Thanks a lot
> 
> 

[PATCH -next] usb: mtu3: fix dbginfo in qmu_tx_zlp_error_handler

[YueHaibing]

Fixes gcc '-Wunused-but-set-variable' warning:

drivers/usb/mtu3/mtu3_qmu.c: In function 'qmu_tx_zlp_error_handler':
drivers/usb/mtu3/mtu3_qmu.c:385:22: warning:
 variable 'req' set but not used [-Wunused-but-set-variable]

It seems dbginfo original intention is print 'req' other than 'mreq'

Signed-off-by: YueHaibing 
---
 drivers/usb/mtu3/mtu3_qmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/mtu3/mtu3_qmu.c b/drivers/usb/mtu3/mtu3_qmu.c
index 73ac042..09f19f7 100644
--- a/drivers/usb/mtu3/mtu3_qmu.c
+++ b/drivers/usb/mtu3/mtu3_qmu.c
@@ -402,7 +402,7 @@ static void qmu_tx_zlp_error_handler(struct mtu3 *mtu, u8 
epnum)
return;
}
 
-   dev_dbg(mtu->dev, "%s send ZLP for req=%p\n", __func__, mreq);
+   dev_dbg(mtu->dev, "%s send ZLP for req=%p\n", __func__, req);
 
mtu3_clrbits(mbase, MU3D_EP_TXCR0(mep->epnum), TX_DMAREQEN);

[PATCH] Revert "usb: dwc3: pci: Use devm functions to get the phy GPIOs"

[Stephan Gerhold]

Commit 211f658b7b40 ("usb: dwc3: pci: Use devm functions to get
the phy GPIOs") changed the code to claim the PHY GPIOs permanently
for Intel Baytrail devices.

This causes issues when the actual PHY driver attempts to claim the
same GPIO descriptors. For example, tusb1210 now fails to probe with:

  tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)

dwc3-pci needs to turn on the PHY once before dwc3 is loaded, but
usually the PHY driver will then hold the GPIOs to turn off the
PHY when requested (e.g. during suspend).

To fix the problem, this reverts the commit to restore the old
behavior to put the GPIOs immediately after usage.

Link: https://www.spinics.net/lists/linux-usb/msg174681.html
Cc: sta...@vger.kernel.org
Signed-off-by: Stephan Gerhold 
---
 drivers/usb/dwc3/dwc3-pci.c | 8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 842795856bf4..fdc6e4e403e8 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -170,20 +170,20 @@ static int dwc3_pci_quirks(struct dwc3_pci *dwc)
 * put the gpio descriptors again here because the phy 
driver
 * might want to grab them, too.
 */
-   gpio = devm_gpiod_get_optional(>dev, "cs",
-  GPIOD_OUT_LOW);
+   gpio = gpiod_get_optional(>dev, "cs", 
GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
 
gpiod_set_value_cansleep(gpio, 1);
+   gpiod_put(gpio);
 
-   gpio = devm_gpiod_get_optional(>dev, "reset",
-  GPIOD_OUT_LOW);
+   gpio = gpiod_get_optional(>dev, "reset", 
GPIOD_OUT_LOW);
if (IS_ERR(gpio))
return PTR_ERR(gpio);
 
if (gpio) {
gpiod_set_value_cansleep(gpio, 1);
+   gpiod_put(gpio);
usleep_range(1, 11000);
}
}
-- 
2.19.2

Re: [GIT PULL] USB-serial fix for v4.20-rc6

[Greg Kroah-Hartman]

On Thu, Dec 06, 2018 at 05:33:46PM +0100, Johan Hovold wrote:
> The following changes since commit 2595646791c319cadfdbf271563aac97d0843dc7:
> 
>   Linux 4.20-rc5 (2018-12-02 15:07:55 -0800)
> 
> are available in the Git repository at:
> 
>   https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git 
> tags/usb-serial-4.20-rc6
> 
> for you to fetch changes up to f51ccf46217c28758b1f3b5bc0ccfc00eca658b2:
> 
>   USB: serial: console: fix reported terminal settings (2018-12-05 11:29:10 
> +0100)
> 
> 
> USB-serial fix for v4.20-rc6
> 
> Here's a fix for a reported USB-console regression in 4.18 which
> revealed a long-standing bug in the console implementation.
> 
> The patch has been in linux-next over night with no reported issues.
> 
> Signed-off-by: Johan Hovold 

Pulled and pushed out, thanks.

greg k-h

[GIT PULL] USB-serial fix for v4.20-rc6

[Johan Hovold]

The following changes since commit 2595646791c319cadfdbf271563aac97d0843dc7:

  Linux 4.20-rc5 (2018-12-02 15:07:55 -0800)

are available in the Git repository at:

  https://git.kernel.org/pub/scm/linux/kernel/git/johan/usb-serial.git 
tags/usb-serial-4.20-rc6

for you to fetch changes up to f51ccf46217c28758b1f3b5bc0ccfc00eca658b2:

  USB: serial: console: fix reported terminal settings (2018-12-05 11:29:10 
+0100)


USB-serial fix for v4.20-rc6

Here's a fix for a reported USB-console regression in 4.18 which
revealed a long-standing bug in the console implementation.

The patch has been in linux-next over night with no reported issues.

Signed-off-by: Johan Hovold 


Johan Hovold (1):
  USB: serial: console: fix reported terminal settings

 drivers/tty/tty_io.c | 11 +--
 drivers/usb/serial/console.c |  2 +-
 include/linux/tty.h  |  1 +
 3 files changed, 11 insertions(+), 3 deletions(-)

Re: [PATCH] USB: qcaux: Add Motorola modem UARTs

[Tony Lindgren]

* Tony Lindgren  [181206 07:48]:
> * Johan Hovold  [181206 06:00]:
> > How do switch modes by the way?
> 
> The flash mode gets enabled with the control GPIOs. I just
> did a quick test patch for phy-mapphone-mdm6600 using module
> param for that. Then additionally the modem USB can be
> multiplexed to the PC by configuring mode in phy-cpcap-usb
> but I don't have a patch for that.

FYI, below is the test patch against next I used for switching
between normal mode and flash mode with a module param flash_mode
if somebody wants to play with it. For flashing the modem,
Android update-binary does it to deal with the signed modem
firmware, I don't know of the details what happens there.

Kishon, is there maybe some phy framework sysfs property
we could use for switching phy modes?

Regards,

Tony

8< ---
diff --git a/drivers/phy/motorola/phy-mapphone-mdm6600.c 
b/drivers/phy/motorola/phy-mapphone-mdm6600.c
--- a/drivers/phy/motorola/phy-mapphone-mdm6600.c
+++ b/drivers/phy/motorola/phy-mapphone-mdm6600.c
@@ -80,6 +80,10 @@ enum phy_mdm6600_status {
PHY_MDM6600_STATUS_UNDEFINED,
 };
 
+static bool flash_mode;
+module_param(flash_mode, bool, 0);
+MODULE_PARM_DESC(flash_mode, "Start mdm6600 in flash mode");
+
 static const char * const
 phy_mdm6600_status_name[] = {
"off", "busy", "qc_dl", "ram_dl", "awake",
@@ -249,6 +253,9 @@ static irqreturn_t phy_mdm6600_wakeirq_thread(int irq, void 
*data)
struct phy_mdm6600 *ddata = data;
struct gpio_desc *mode_gpio1;
 
+   if (flash_mode)
+   return IRQ_NONE;
+
mode_gpio1 = ddata->mode_gpios->desc[PHY_MDM6600_MODE1];
dev_dbg(ddata->dev, "OOB wake on mode_gpio1: %i\n",
gpiod_get_value(mode_gpio1));
@@ -377,8 +384,13 @@ static int phy_mdm6600_device_power_on(struct phy_mdm6600 
*ddata)
 * to configure USB flashing mode later on based on a module
 * parameter.
 */
-   gpiod_set_value_cansleep(mode_gpio0, 0);
-   gpiod_set_value_cansleep(mode_gpio1, 0);
+   if (flash_mode) {
+   gpiod_set_value_cansleep(mode_gpio0, 1);
+   gpiod_set_value_cansleep(mode_gpio1, 1);
+   } else {
+   gpiod_set_value_cansleep(mode_gpio0, 0);
+   gpiod_set_value_cansleep(mode_gpio1, 0);
+   }
 
/* Request start-up mode */
phy_mdm6600_cmd(ddata, PHY_MDM6600_CMD_NO_BYPASS);
@@ -414,7 +426,12 @@ static int phy_mdm6600_device_power_on(struct phy_mdm6600 
*ddata)
dev_err(ddata->dev, "Timed out powering up\n");
}
 
-   /* Reconfigure mode1 GPIO as input for OOB wake */
+   /* Maybe reconfigure mode1 GPIO as input for OOB wake? */
+   if (flash_mode) {
+   dev_info(ddata->dev, "Started in flash mode\n");
+   goto done;
+   }
+
gpiod_direction_input(mode_gpio1);
 
wakeirq = gpiod_to_irq(mode_gpio1);
@@ -431,7 +448,7 @@ static int phy_mdm6600_device_power_on(struct phy_mdm6600 
*ddata)
if (error)
dev_warn(ddata->dev, "no modem wakeirq irq%i: %i\n",
 wakeirq, error);
-
+done:
ddata->running = true;
 
return error;
@@ -499,6 +516,9 @@ static void phy_mdm6600_modem_wake(struct work_struct *work)
 {
struct phy_mdm6600 *ddata;
 
+   if (flash_mode)
+   return;
+
ddata = container_of(work, struct phy_mdm6600, modem_wake_work.work);
phy_mdm6600_wake_modem(ddata);
schedule_delayed_work(>modem_wake_work,
@@ -509,6 +529,9 @@ static int __maybe_unused 
phy_mdm6600_runtime_suspend(struct device *dev)
 {
struct phy_mdm6600 *ddata = dev_get_drvdata(dev);
 
+   if (flash_mode)
+   return 0;
+
cancel_delayed_work_sync(>modem_wake_work);
ddata->awake = false;
 
@@ -519,6 +542,9 @@ static int __maybe_unused phy_mdm6600_runtime_resume(struct 
device *dev)
 {
struct phy_mdm6600 *ddata = dev_get_drvdata(dev);
 
+   if (flash_mode)
+   return 0;
+
phy_mdm6600_modem_wake(>modem_wake_work.work);
ddata->awake = true;
 
-- 
2.19.2

Re: [PATCH] USB: qcaux: Add Motorola modem UARTs

[Tony Lindgren]

Hi,

* Johan Hovold  [181206 06:00]:
> On Wed, Dec 05, 2018 at 05:54:07PM -0800, Tony Lindgren wrote:
...
> >   idVendor   0x22b8 
> >   idProduct  0x4281 
> 
> This PID is not included in your patch however.

Oops sorry did a cat on wrong two trimmed lsusb -v output
files..

> And this doesn't look like a modem with five (?) interfaces, but I guess
> it's in "flash" mode?

Yes, the wrong lsusb output is  for the flash mode that we're not
adding here and can be ignored for now.

> Could you post the output for the two devices you have in "modem" mode?

Yes correct lsusb -v output below with five UARTs with ff/ff/ff and
two QMI ports that are already handled with an earlier commit
4071898bf0f4 ("net: qmi_wwan: Add USB IDs for MDM6600 modem on
Motorola Droid 4").

> How do switch modes by the way?

The flash mode gets enabled with the control GPIOs. I just
did a quick test patch for phy-mapphone-mdm6600 using module
param for that. Then additionally the modem USB can be
multiplexed to the PC by configuring mode in phy-cpcap-usb
but I don't have a patch for that.

Regards,

Tony

8< ---
Bus 001 Device 002: ID 22b8:2a70  
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x22b8 
  idProduct  0x2a70 
  bcdDevice0.00
  iManufacturer   1 Motorola, Incorporated
  iProduct2 Flash MZ600
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   0x00fb
bNumInterfaces  9
bConfigurationValue 1
iConfiguration  3 Motorola Configuration
bmAttributes 0xe0
  Self Powered
  Remote Wakeup
MaxPower  500mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 
  bInterfaceSubClass255 
  bInterfaceProtocol255 
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber1
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 
  bInterfaceSubClass255 
  bInterfaceProtocol255 
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x82  EP 2 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x02  EP 2 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber2
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 
  bInterfaceSubClass255 
  bInterfaceProtocol255 
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x83  EP 3 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x03  EP 3 OUT
bmAttributes2
  Transfer TypeBulk
 

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Bin Liu]

Peter,

On Wed, Nov 28, 2018 at 01:16:32PM +0200, Peter Ujfalusi wrote:
> Hi,
> 
> On 28/11/2018 13.15, Peter Ujfalusi wrote:
> 
> forgot to fix up Vinod's email address.
> 
> > 
> > 
> > On 12/11/2018 17.40, Bin Liu wrote:
> > 
> > Can you fix up the subject line to:
> > dmaengine: ti: cppi4: delete channel from pending list when stop channel
> > 
> >> The driver defines three states for a cppi channel.
> >> - idle: .chan_busy == 0 && not in .pending list
> >> - pending: .chan_busy == 0 && in .pending list
> >> - busy: .chan_busy == 1 && not in .pending list
> >>
> >> There are cases in which the cppi channel could be in the pending state
> >> when cppi41_dma_issue_pending() is called after cppi41_runtime_suspend()
> >> is called.
> >>
> >> cppi41_stop_chan() has a bug for these cases to set channels to idle state.
> >> It only checks the .chan_busy flag, but not the .pending list, then later
> >> when cppi41_runtime_resume() is called the channels in .pending list will
> >> be transitioned to busy state.
> >>
> >> Removing channels from the .pending list solves the problem.
> > 
> > So, let me see if I understand this correctly:
> > - client issued a transfer _after_ the cppi4 driver is suspended
> > - cppi41_dma_issue_pending() will place it to pending list and will not
> > start the transfer right away as cdd->is_suspended is true.
> > - on resume the cppi4 will pick up the pending transfers from the
> > pending list
> > 
> > This is so far a sane thing to do.
> > 
> > If I guess right, then after the issue_pending the client driver will
> > call terminate_all, presumably from it's suspend callback?
> > 
> > As per the purpose of terminate_all we should terminated all future
> > transfers on the channel, so clearing the pending list is the correct
> > thing to do.
> > 
> > With the fixed subject:
> > Reviewed-by: Peter Ujfalusi 
> > 
> > I have one question:
> > 
> >> Fixes: 975faaeb9985 ("dma: cppi41: start tear down only if channel is 
> >> busy")
> >> Cc: sta...@vger.kernel.org # v3.15+
> >> Signed-off-by: Bin Liu 
> >> ---
> >>  drivers/dma/ti/cppi41.c | 16 +++-
> >>  1 file changed, 15 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/dma/ti/cppi41.c b/drivers/dma/ti/cppi41.c
> >> index 1497da367710..e507ec36c0d3 100644
> >> --- a/drivers/dma/ti/cppi41.c
> >> +++ b/drivers/dma/ti/cppi41.c
> >> @@ -723,8 +723,22 @@ static int cppi41_stop_chan(struct dma_chan *chan)
> >>  
> >>desc_phys = lower_32_bits(c->desc_phys);
> >>desc_num = (desc_phys - cdd->descs_phys) / sizeof(struct cppi41_desc);
> >> -  if (!cdd->chan_busy[desc_num])
> >> +  if (!cdd->chan_busy[desc_num]) {
> >> +  struct cppi41_channel *cc, *_ct;
> >> +
> >> +  /*
> >> +   * channels might still be in the pendling list if
> >> +   * cppi41_dma_issue_pending() is called after
> >> +   * cppi41_runtime_suspend() is called
> >> +   */
> >> +  list_for_each_entry_safe(cc, _ct, >pending, node) {
> >> +  if (cc != c)
> >> +  continue;
> >> +  list_del(>node);
> > 
> > If we delete from the pending list, are we going to leak memory?
> > I'm not familiar with the cppi4, it might not be an issue for it.

Here is no memory leak.
The elements added to the pending list are cppi41 channels which are
allocated in driver _probe(). No dynamic memory allocation happening
when operating this pending list.

Regards,
-Bin.

[balbi-usb:testing/next 50/50] drivers/usb/dwc3/./trace.h:216:449: warning: 's' may be used uninitialized in this function

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
testing/next
head:   ad7b607f82731eec3ed17d9d22764eb6f09609f9
commit: ad7b607f82731eec3ed17d9d22764eb6f09609f9 [50/50] usb: dwc3: trace: add 
missing break statement to make compiler happy
config: i386-allyesconfig (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout ad7b607f82731eec3ed17d9d22764eb6f09609f9
# save the attached .config to linux build tree
make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/trace/trace_events.h:394:0,
from include/trace/define_trace.h:96,
from drivers/usb/dwc3/trace.h:342,
from drivers/usb/dwc3/trace.c:11:
   drivers/usb/dwc3/./trace.h: In function 'trace_raw_output_dwc3_log_trb':
>> drivers/usb/dwc3/./trace.h:216:449: warning: 's' may be used uninitialized 
>> in this function [-Wmaybe-uninitialized]
DECLARE_EVENT_CLASS(dwc3_log_trb,





^   









   drivers/usb/dwc3/./trace.h:216:647: note: 's' was declared here
DECLARE_EVENT_CLASS(dwc3_log_trb,








  ^

vim +/s +216 drivers/usb/dwc3/./trace.h

2c4cbe6e5a Felipe Balbi2014-04-30  215  
2c4cbe6e5a Felipe Balbi2014-04-30 @216  DECLARE_EVENT_CLASS(dwc3_log_trb,
2c4cbe6e5a Felipe Balbi2014-04-30  217  TP_PROTO(struct dwc3_ep *dep, 
struct dwc3_trb *trb),
2c4cbe6e5a Felipe Balbi2014-04-30  218  TP_ARGS(dep, trb),
2c4cbe6e5a Felipe Balbi2014-04-30  219  TP_STRUCT__entry(
e42f09b85f Felipe Balbi2017-04-28  220  __string(name, 
dep->name)
2c4cbe6e5a Felipe Balbi2014-04-30  221  __field(struct dwc3_trb 
*, trb)
68d34c8a74 Felipe Balbi2016-05-30  222  __field(u32, allocated)
68d34c8a74 Felipe Balbi2016-05-30  223  __field(u32, queued)
4ac4fc9322 Felipe Balbi2014-09-17  224  __field(u32, bpl)
4ac4fc9322 Felipe Balbi2014-09-17  225  __field(u32, bph)
4ac4fc9322 Felipe Balbi2014-09-17  226  __field(u32, size)
4ac4fc9322 Felipe Balbi2014-09-17  227  __field(u32, ctrl)
fa8d965d73 Felipe Balbi2016-09-28  228  __field(u32, type)
2c4cbe6e5a Felipe Balbi2014-04-30  229  ),
2c4cbe6e5a Felipe Balbi2014-04-30  230  TP_fast_assign(
e42f09b85f Felipe Balbi2017-04-28  231  __assign_str(name, 
dep->name);
2c4cbe6e5a Felipe Balbi2014-04-30  232  __entry->trb = trb;
4ac4fc9322 Felipe Balbi2014-09-17  233  __entry->bpl = trb->bpl;
4ac4fc9322 Felipe Balbi2014-09-17  234  __entry->bph = trb->bph;
4ac4fc9322 Felipe Balbi2014-09-17  235  __entry->size = 
trb->size;
4ac4fc9322 Felipe Balbi2014-09-17  236  __entry->ctrl = 
trb->ctrl;
fa8d965d73 Felipe Balbi2016-09-28  237  __entry->type = 
usb_endpoint_type(dep->endpoint.desc);
2c4cbe6e5a Felipe Balbi2014-04-30  238  ),

Re: [PATCH drivers\usb\serial\pl2303.c & pl2303.h] Prolific's PL2303G: new USB to UART chip

[Greg KH]

On Thu, Dec 06, 2018 at 03:47:46PM +0800, Charles Yeh wrote:
> Hi Greg,
>   The patch file: diffpl2303.patch is attached..
>  Please you kindly check...

Please fix up the patch to look like all other patches on this mailing
list.

You do not need the huge comment in the file, that should be in the body
of the email for the changelog, you do not have a signed-off-by line,
the patch was not at the right "depth" of the kernel source tree, and
you sent this as an attachment.  Any one of these would prevent the
patch from being accepted...

Please fix this up and resend it properly.

thanks,

greg k-h

Re: [PATCH drivers\usb\serial\pl2303.c & pl2303.h] Prolific's PL2303G: new USB to UART chip

[Charles Yeh]

Hi Greg,
  The patch file: diffpl2303.patch is attached..
 Please you kindly check...

Thanks!

Charles

Charles Yeh  於 2018年11月20日 週二 下午5:35寫道：
>
> Hi Greg,
>Try again...no problem...
>
> make one patch for all of the files modified, <<---OK.
>
> proper changelog text and a signed-off-by line <<-- I will study
> Documentation/SubmittingPatches again.
>
> use tabs, not spaces  <<---OK.
>
> Greg KH  於 2018年11月20日 週二 下午5:28寫道：
> >
> > On Tue, Nov 20, 2018 at 05:21:04PM +0800, Charles Yeh wrote:
> > > Hi Johan,
> > >   After read Documentation\process\submitting-patches.rst,
> > >   I have write some describe in attach file : "diffpl2303c.patch"
> > > "diffpl2303h.patch"
> > >
> > >  If this file does not meet the file requirements, please let me
> > > know how to do it.
> >
> > This is close, but not quite there yet.
> >
> > You need to make one patch for all of the files modified, look at all of
> > the patches on the mailing list for examples of this.
> >
> > Also, you need a proper changelog text and a signed-off-by line, the
> > file Documentation/SubmittingPatches in the kernel source tree will
> > describe all of this and how to do it.
> >
> > And finally, your patch needs to properly use tabs, not spaces, look at
> > the diff you generated for examples of how the lines do not align
> > properly now.
> >
> > Can you try again please?
> >
> > thanks,
> >
> > greg k-h


diffpl2303.patch
Description: Binary data

Re: [PATCH] USB: qcaux: Add Motorola modem UARTs

[Johan Hovold]

On Wed, Dec 05, 2018 at 05:54:07PM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Johan Hovold  [181205 06:17]:
> > On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote:
> > > On Motorola Mapphone devices such as Droid 4 there are five USB ports
> > > that do not use the same layout as Gobi 1K/2K/etc devices listed in
> > > qcserial.c. So we should use qcaux.c or option.c as noted by
> > > Dan Williams .
> > > 
> > > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices.
> > > And we should not add interfaces with 0x0a class (CDC Data) as they
> > > are part of a multi-interface function like for example interface
> > > 0x22b8:0x4281 as noted by Bjørn Mork .
> > 
> > Can you post the output of usb-devices (or lsusb -v) for these three
> > devices (PIDs)?
> 
> Here's two out of three for you to look at. They're all listed in
> drivers/usb/serial/mdm6600.c in at least the Motorola Mapphone
> Android kernels, see for example the LineageOS kernel at [0] if
> you want to look at the USB serial driver.
> 
> I don't have a device with 9600 with 0x2e0a id.
> 
> [0] 
> https://github.com/LineageOS/android_kernel_motorola_omap4-common/blob/cm-14.1/drivers/usb/serial/mdm6600.c

Thanks for the pointer.

> 
> 8< -
> Bus 001 Device 002: ID 22b8:4281  
> Device Descriptor:
>   bLength18
>   bDescriptorType 1
>   bcdUSB   2.00
>   bDeviceClass0 
>   bDeviceSubClass 0 
>   bDeviceProtocol 0 
>   bMaxPacketSize064
>   idVendor   0x22b8 
>   idProduct  0x4281 

This PID is not included in your patch however.

>   bcdDevice0.00
>   iManufacturer   1 Motorola, Incorporated
>   iProduct2 Flash MZ600
>   iSerial 0 
>   bNumConfigurations  1
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength   0x0020
> bNumInterfaces  1
> bConfigurationValue 1
> iConfiguration  3 Motorola Configuration
> bmAttributes 0xe0
>   Self Powered
>   Remote Wakeup
> MaxPower  500mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
>   bNumEndpoints   2
>   bInterfaceClass10 
>   bInterfaceSubClass  0 
>   bInterfaceProtocol252 

And wouldn't match on ff/ff/ff in any case.

>   iInterface  5 Motorola Flash
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81  EP 1 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> bInterval   0
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x01  EP 1 OUT
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> bInterval   0
> Device Status: 0x
>   (Bus Powered)
> 
> Bus 003 Device 109: ID 22b8:900e  
> Device Descriptor:
>   bLength18
>   bDescriptorType 1
>   bcdUSB   2.00
>   bDeviceClass0 
>   bDeviceSubClass 0 
>   bDeviceProtocol 0 
>   bMaxPacketSize064
>   idVendor   0x22b8 
>   idProduct  0x900e 
>   bcdDevice0.00
>   iManufacturer   1 Motorola, Incorporated
>   iProduct2 Flash MZ600
>   iSerial 0 
>   bNumConfigurations  1
>   Configuration Descriptor:
> bLength 9
> bDescriptorType 2
> wTotalLength   0x0020
> bNumInterfaces  1
> bConfigurationValue 1
> iConfiguration  3 Motorola Configuration
> bmAttributes 0xe0
>   Self Powered
>   Remote Wakeup
> MaxPower  500mA
> Interface Descriptor:
>   bLength 9
>   bDescriptorType 4
>   bInterfaceNumber0
>   bAlternateSetting   0
>   bNumEndpoints   2
>   bInterfaceClass   255 
>   bInterfaceSubClass255 
>   bInterfaceProtocol255 
>   iInterface  0 
>   Endpoint Descriptor:
> bLength 7
> bDescriptorType 5
> bEndpointAddress 0x81  EP 1 IN
> bmAttributes2
>   Transfer TypeBulk
>   Synch Type   None
>   Usage Type   Data
> wMaxPacketSize 0x0040  1x 64 bytes
> 

Re: [PATCH v3 2/2] usb: dwc3: gadget: Report isoc transfer frame number

[Thinh Nguyen]

Hi Felipe,

On 12/5/2018 1:15 AM, Felipe Balbi wrote:
> Hi,
>
> Thinh Nguyen  writes:
>> Implement the new frame_number API to report the isochronous interval
>> frame number. This patch checks and reports the interval in which the
>> isoc transfer was transmitted or received via the Isoc-First TRB SOF
>> number field.
>>
>> Signed-off-by: Thinh Nguyen 
>> ---
>> Change in v3:
>> - Implement the change with the redefined frame_number meaning
>> Change in v2:
>> - Capture frame number at request cleanup
>>
>>  drivers/usb/dwc3/core.h   |  1 +
>>  drivers/usb/dwc3/gadget.c | 13 +
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
>> index ed0359d1216d..2c9f7a93147a 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -777,6 +777,7 @@ enum dwc3_link_state {
>>  #define DWC3_TRB_CTRL_ISP_IMI   BIT(10)
>>  #define DWC3_TRB_CTRL_IOC   BIT(11)
>>  #define DWC3_TRB_CTRL_SID_SOFN(n)   (((n) & 0x) << 14)
>> +#define DWC3_TRB_CTRL_GET_SID_SOFN(n)   (((n) & (0x << 14)) >> 14)
>>  
>>  #define DWC3_TRBCTL_TYPE(n) ((n) & (0x3f << 4))
>>  #define DWC3_TRBCTL_NORMAL  DWC3_TRB_CTRL_TRBCTL(1)
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 679c12e14522..2de563124fc1 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -2254,6 +2254,19 @@ static int 
>> dwc3_gadget_ep_reclaim_completed_trb(struct dwc3_ep *dep,
>>  if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
>>  trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>>  
>> +/*
>> + * For isochronous transfers, the first TRB in a service interval must
>> + * have the Isoc-First type. Track and report its interval frame number.
>> + */
>> +if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
>> +(trb->ctrl & DWC3_TRBCTL_ISOCHRONOUS_FIRST)) {
>> +unsigned int frame_number;
>> +
>> +frame_number = DWC3_TRB_CTRL_GET_SID_SOFN(trb->ctrl);
>> +frame_number &= ~(dep->interval - 1);
>> +req->request.frame_number = frame_number;
>> +}
> could you refresh my memory on how this is going to be used?
>

Sure. This informs the upper layer driver what interval the isoc request
went out. The users can use this information for applications that
require synchronization with the host.

Thinh

Re: [PATCH] USB: qcaux: Add Motorola modem UARTs

[Tony Lindgren]

Hi,

* Johan Hovold  [181205 06:17]:
> On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote:
> > On Motorola Mapphone devices such as Droid 4 there are five USB ports
> > that do not use the same layout as Gobi 1K/2K/etc devices listed in
> > qcserial.c. So we should use qcaux.c or option.c as noted by
> > Dan Williams .
> > 
> > The ff/ff/ff interfaces seem to always be UARTs on Motorola devices.
> > And we should not add interfaces with 0x0a class (CDC Data) as they
> > are part of a multi-interface function like for example interface
> > 0x22b8:0x4281 as noted by Bjørn Mork .
> 
> Can you post the output of usb-devices (or lsusb -v) for these three
> devices (PIDs)?

Here's two out of three for you to look at. They're all listed in
drivers/usb/serial/mdm6600.c in at least the Motorola Mapphone
Android kernels, see for example the LineageOS kernel at [0] if
you want to look at the USB serial driver.

I don't have a device with 9600 with 0x2e0a id.

Regards,

Tony

[0] 
https://github.com/LineageOS/android_kernel_motorola_omap4-common/blob/cm-14.1/drivers/usb/serial/mdm6600.c

8< -
Bus 001 Device 002: ID 22b8:4281  
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x22b8 
  idProduct  0x4281 
  bcdDevice0.00
  iManufacturer   1 Motorola, Incorporated
  iProduct2 Flash MZ600
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   0x0020
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  3 Motorola Configuration
bmAttributes 0xe0
  Self Powered
  Remote Wakeup
MaxPower  500mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass10 
  bInterfaceSubClass  0 
  bInterfaceProtocol252 
  iInterface  5 Motorola Flash
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval   0
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval   0
Device Status: 0x
  (Bus Powered)

Bus 003 Device 109: ID 22b8:900e  
Device Descriptor:
  bLength18
  bDescriptorType 1
  bcdUSB   2.00
  bDeviceClass0 
  bDeviceSubClass 0 
  bDeviceProtocol 0 
  bMaxPacketSize064
  idVendor   0x22b8 
  idProduct  0x900e 
  bcdDevice0.00
  iManufacturer   1 Motorola, Incorporated
  iProduct2 Flash MZ600
  iSerial 0 
  bNumConfigurations  1
  Configuration Descriptor:
bLength 9
bDescriptorType 2
wTotalLength   0x0020
bNumInterfaces  1
bConfigurationValue 1
iConfiguration  3 Motorola Configuration
bmAttributes 0xe0
  Self Powered
  Remote Wakeup
MaxPower  500mA
Interface Descriptor:
  bLength 9
  bDescriptorType 4
  bInterfaceNumber0
  bAlternateSetting   0
  bNumEndpoints   2
  bInterfaceClass   255 
  bInterfaceSubClass255 
  bInterfaceProtocol255 
  iInterface  0 
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x81  EP 1 IN
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
  Endpoint Descriptor:
bLength 7
bDescriptorType 5
bEndpointAddress 0x01  EP 1 OUT
bmAttributes2
  Transfer TypeBulk
  Synch Type   None
  Usage Type   Data
wMaxPacketSize 0x0040  1x 64 bytes
bInterval  32
Device Status: 0x
  (Bus Powered)

[balbi-usb:testing/next 50/50] drivers/usb/dwc3/./trace.h:239:2: note: in expansion of macro 'TP_printk'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
testing/next
head:   ad7b607f82731eec3ed17d9d22764eb6f09609f9
commit: ad7b607f82731eec3ed17d9d22764eb6f09609f9 [50/50] usb: dwc3: trace: add 
missing break statement to make compiler happy
config: i386-randconfig-x006-12051027 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
git checkout ad7b607f82731eec3ed17d9d22764eb6f09609f9
# save the attached .config to linux build tree
make ARCH=i386 

Note: it may well be a FALSE warning. FWIW you are at least aware of it now.
http://gcc.gnu.org/wiki/Better_Uninitialized_Warnings

All warnings (new ones prefixed by >>):

   In file included from include/trace/define_trace.h:96:0,
from drivers/usb/dwc3/trace.h:342,
from drivers/usb/dwc3/trace.c:11:
   drivers/usb/dwc3/./trace.h: In function 'trace_raw_output_dwc3_log_trb':
>> include/trace/trace_events.h:360:2: warning: 's' may be used uninitialized 
>> in this function [-Wmaybe-uninitialized]
 trace_seq_printf(s, print); \
 ^~~~
   drivers/usb/dwc3/./trace.h:241:11: note: 's' was declared here
  ({char *s;
  ^
   include/trace/trace_events.h:360:22: note: in definition of macro 
'DECLARE_EVENT_CLASS'
 trace_seq_printf(s, print); \
 ^
>> drivers/usb/dwc3/./trace.h:239:2: note: in expansion of macro 'TP_printk'
 TP_printk("%s: trb %p buf %08x%08x size %s%d ctrl %08x (%c%c%c%c:%c%c:%s)",
 ^
--
   In file included from include/trace/define_trace.h:96:0,
from drivers/usb//dwc3/trace.h:342,
from drivers/usb//dwc3/trace.c:11:
   drivers/usb//dwc3/./trace.h: In function 'trace_raw_output_dwc3_log_trb':
>> include/trace/trace_events.h:360:2: warning: 's' may be used uninitialized 
>> in this function [-Wmaybe-uninitialized]
 trace_seq_printf(s, print); \
 ^~~~
   drivers/usb//dwc3/./trace.h:241:11: note: 's' was declared here
  ({char *s;
  ^
   include/trace/trace_events.h:360:22: note: in definition of macro 
'DECLARE_EVENT_CLASS'
 trace_seq_printf(s, print); \
 ^
   drivers/usb//dwc3/./trace.h:239:2: note: in expansion of macro 'TP_printk'
 TP_printk("%s: trb %p buf %08x%08x size %s%d ctrl %08x (%c%c%c%c:%c%c:%s)",
 ^

vim +/TP_printk +239 drivers/usb/dwc3/./trace.h

2c4cbe6e5a Felipe Balbi2014-04-30  215  
2c4cbe6e5a Felipe Balbi2014-04-30  216  DECLARE_EVENT_CLASS(dwc3_log_trb,
2c4cbe6e5a Felipe Balbi2014-04-30  217  TP_PROTO(struct dwc3_ep *dep, 
struct dwc3_trb *trb),
2c4cbe6e5a Felipe Balbi2014-04-30  218  TP_ARGS(dep, trb),
2c4cbe6e5a Felipe Balbi2014-04-30  219  TP_STRUCT__entry(
e42f09b85f Felipe Balbi2017-04-28  220  __string(name, 
dep->name)
2c4cbe6e5a Felipe Balbi2014-04-30  221  __field(struct dwc3_trb 
*, trb)
68d34c8a74 Felipe Balbi2016-05-30  222  __field(u32, allocated)
68d34c8a74 Felipe Balbi2016-05-30  223  __field(u32, queued)
4ac4fc9322 Felipe Balbi2014-09-17  224  __field(u32, bpl)
4ac4fc9322 Felipe Balbi2014-09-17  225  __field(u32, bph)
4ac4fc9322 Felipe Balbi2014-09-17  226  __field(u32, size)
4ac4fc9322 Felipe Balbi2014-09-17  227  __field(u32, ctrl)
fa8d965d73 Felipe Balbi2016-09-28  228  __field(u32, type)
2c4cbe6e5a Felipe Balbi2014-04-30  229  ),
2c4cbe6e5a Felipe Balbi2014-04-30  230  TP_fast_assign(
e42f09b85f Felipe Balbi2017-04-28  231  __assign_str(name, 
dep->name);
2c4cbe6e5a Felipe Balbi2014-04-30  232  __entry->trb = trb;
4ac4fc9322 Felipe Balbi2014-09-17  233  __entry->bpl = trb->bpl;
4ac4fc9322 Felipe Balbi2014-09-17  234  __entry->bph = trb->bph;
4ac4fc9322 Felipe Balbi2014-09-17  235  __entry->size = 
trb->size;
4ac4fc9322 Felipe Balbi2014-09-17  236  __entry->ctrl = 
trb->ctrl;
fa8d965d73 Felipe Balbi2016-09-28  237  __entry->type = 
usb_endpoint_type(dep->endpoint.desc);
2c4cbe6e5a Felipe Balbi2014-04-30  238  ),
0bd0f6d201 Felipe Balbi2018-03-26 @239  TP_printk("%s: trb %p buf 
%08x%08x size %s%d ctrl %08x (%c%c%c%c:%c%c:%s)",
0bd0f6d201 Felipe Balbi2018-03-26  240  __get_str(name), 
__entry->trb, __entry->bph, __entry->bpl,
fa8d965d73 Felipe Balbi2016-09-28 @241  ({char *s;
fa8d965d73 Felipe Balbi2016-09-28  242  int pcm = 
((__entry->size >> 24) & 3) + 1;
fa8d965d73 Felipe Balbi2016-09-28  243  switch (__entry->type) {
fa8d965d73 Felipe Balbi2016-09-28  244  case 
USB_ENDPOINT_XFER_INT:
fa8d965d73 Felipe Balbi2016-09-28  245  case 
USB_ENDPOINT_XFER_ISOC:
fa8d965d73 Felipe Balbi

[PATCH 0/2] xhci fixes for usb-linus

[Mathias Nyman]

Hi Greg

A couple patches for 4.20.

Finally found a reason why devices attached after a high bandwidth USB3
isoc device may fail to enumerate with bandwidth error.

Second patch is a quirk for AMD SNPS host specific suspend issue.

-Mathias


Mathias Nyman (1):
  xhci: Prevent U1/U2 link pm states if exit latency is too long

Sandeep Singh (1):
  xhci: workaround CSS timeout on AMD SNPS 3.0 xHC

 drivers/usb/host/xhci-pci.c |  4 
 drivers/usb/host/xhci.c | 42 ++
 drivers/usb/host/xhci.h |  3 +++
 3 files changed, 45 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH 1/2] xhci: workaround CSS timeout on AMD SNPS 3.0 xHC

[Mathias Nyman]

From: Sandeep Singh 

Occasionally AMD SNPS 3.0 xHC does not respond to
CSS when set, also it does not flag anything on SRE and HCE
to point the internal xHC errors on USBSTS register. This stalls
the entire system wide suspend and there is no point in stalling
just because of xHC CSS is not responding.

To work around this problem, if the xHC does not flag
anything on SRE and HCE, we can skip the CSS
timeout and allow the system to continue the suspend. Once the
system resume happens we can internally reset the controller
using XHCI_RESET_ON_RESUME quirk

Signed-off-by: Shyam Sundar S K 
Signed-off-by: Sandeep Singh 
cc: Nehal Shah 
Cc: 
Tested-by: Kai-Heng Feng 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c |  4 
 drivers/usb/host/xhci.c | 26 ++
 drivers/usb/host/xhci.h |  3 +++
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index a951526..a9ec705 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -139,6 +139,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
 pdev->device == 0x43bb))
xhci->quirks |= XHCI_SUSPEND_DELAY;
 
+   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+   (pdev->device == 0x15e0 || pdev->device == 0x15e1))
+   xhci->quirks |= XHCI_SNPS_BROKEN_SUSPEND;
+
if (pdev->vendor == PCI_VENDOR_ID_AMD)
xhci->quirks |= XHCI_TRUST_TX_LENGTH;
 
diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c928dbb..c20b85e 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -968,6 +968,7 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
unsigned intdelay = XHCI_MAX_HALT_USEC;
struct usb_hcd  *hcd = xhci_to_hcd(xhci);
u32 command;
+   u32 res;
 
if (!hcd->state)
return 0;
@@ -1021,11 +1022,28 @@ int xhci_suspend(struct xhci_hcd *xhci, bool do_wakeup)
command = readl(>op_regs->command);
command |= CMD_CSS;
writel(command, >op_regs->command);
+   xhci->broken_suspend = 0;
if (xhci_handshake(>op_regs->status,
STS_SAVE, 0, 10 * 1000)) {
-   xhci_warn(xhci, "WARN: xHC save state timeout\n");
-   spin_unlock_irq(>lock);
-   return -ETIMEDOUT;
+   /*
+* AMD SNPS xHC 3.0 occasionally does not clear the
+* SSS bit of USBSTS and when driver tries to poll
+* to see if the xHC clears BIT(8) which never happens
+* and driver assumes that controller is not responding
+* and times out. To workaround this, its good to check
+* if SRE and HCE bits are not set (as per xhci
+* Section 5.4.2) and bypass the timeout.
+*/
+   res = readl(>op_regs->status);
+   if ((xhci->quirks & XHCI_SNPS_BROKEN_SUSPEND) &&
+   (((res & STS_SRE) == 0) &&
+   ((res & STS_HCE) == 0))) {
+   xhci->broken_suspend = 1;
+   } else {
+   xhci_warn(xhci, "WARN: xHC save state timeout\n");
+   spin_unlock_irq(>lock);
+   return -ETIMEDOUT;
+   }
}
spin_unlock_irq(>lock);
 
@@ -1078,7 +1096,7 @@ int xhci_resume(struct xhci_hcd *xhci, bool hibernated)
set_bit(HCD_FLAG_HW_ACCESSIBLE, >shared_hcd->flags);
 
spin_lock_irq(>lock);
-   if (xhci->quirks & XHCI_RESET_ON_RESUME)
+   if ((xhci->quirks & XHCI_RESET_ON_RESUME) || xhci->broken_suspend)
hibernated = true;
 
if (!hibernated) {
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 260b259..c3515ba 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1850,6 +1850,7 @@ struct xhci_hcd {
 #define XHCI_ZERO_64B_REGS BIT_ULL(32)
 #define XHCI_DEFAULT_PM_RUNTIME_ALLOW  BIT_ULL(33)
 #define XHCI_RESET_PLL_ON_DISCONNECT   BIT_ULL(34)
+#define XHCI_SNPS_BROKEN_SUSPENDBIT_ULL(35)
 
unsigned intnum_active_eps;
unsigned intlimit_active_eps;
@@ -1879,6 +1880,8 @@ struct xhci_hcd {
void*dbc;
/* platform-specific data -- must come last */
unsigned long   priv[0] __aligned(sizeof(s64));
+   /* Broken Suspend flag for SNPS Suspend resume issue */
+   u8  broken_suspend;
 };
 
 /* Platform specific overrides to generic XHCI hc_driver ops */
-- 
2.7.4

[PATCH 2/2] xhci: Prevent U1/U2 link pm states if exit latency is too long

[Mathias Nyman]

Don't allow USB3 U1 or U2 if the latency to wake up from the U-state
reaches the service interval for a periodic endpoint.

This is according to xhci 1.1 specification section 4.23.5.2 extra note:

"Software shall ensure that a device is prevented from entering a U-state
 where its worst case exit latency approaches the ESIT."

Allowing too long exit latencies for periodic endpoint confuses xHC
internal scheduling, and new devices may fail to enumerate with a
"Not enough bandwidth for new device state" error from the host.

Cc: 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci.c | 16 
 1 file changed, 16 insertions(+)

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index c20b85e..dae3be1 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -4514,6 +4514,14 @@ static u16 xhci_calculate_u1_timeout(struct xhci_hcd 
*xhci,
 {
unsigned long long timeout_ns;
 
+   /* Prevent U1 if service interval is shorter than U1 exit latency */
+   if (usb_endpoint_xfer_int(desc) || usb_endpoint_xfer_isoc(desc)) {
+   if (xhci_service_interval_to_ns(desc) <= udev->u1_params.mel) {
+   dev_dbg(>dev, "Disable U1, ESIT shorter than exit 
latency\n");
+   return USB3_LPM_DISABLED;
+   }
+   }
+
if (xhci->quirks & XHCI_INTEL_HOST)
timeout_ns = xhci_calculate_intel_u1_timeout(udev, desc);
else
@@ -4570,6 +4578,14 @@ static u16 xhci_calculate_u2_timeout(struct xhci_hcd 
*xhci,
 {
unsigned long long timeout_ns;
 
+   /* Prevent U2 if service interval is shorter than U2 exit latency */
+   if (usb_endpoint_xfer_int(desc) || usb_endpoint_xfer_isoc(desc)) {
+   if (xhci_service_interval_to_ns(desc) <= udev->u2_params.mel) {
+   dev_dbg(>dev, "Disable U2, ESIT shorter than exit 
latency\n");
+   return USB3_LPM_DISABLED;
+   }
+   }
+
if (xhci->quirks & XHCI_INTEL_HOST)
timeout_ns = xhci_calculate_intel_u2_timeout(udev, desc);
else
-- 
2.7.4

[balbi-usb:testing/next 50/50] drivers/usb/gadget/function/f_fs.c:2198:9: error: too few arguments to function 'ffs_do_single_desc'

[kbuild test robot]

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git 
testing/next
head:   3591a2f3900e0f93dbd246d979ddd939251cac3e
commit: 3591a2f3900e0f93dbd246d979ddd939251cac3e [50/50] usb: gadget: f_fs: add 
the support for alternate interface setting
config: arm-omap2plus_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 7.2.0-11) 7.2.0
reproduce:
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
git checkout 3591a2f3900e0f93dbd246d979ddd939251cac3e
# save the attached .config to linux build tree
GCC_VERSION=7.2.0 make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   drivers/usb/gadget/function/f_fs.c: In function 'ffs_do_descs_alt_intf':
>> drivers/usb/gadget/function/f_fs.c:2198:9: error: too few arguments to 
>> function 'ffs_do_single_desc'
  ret = ffs_do_single_desc(data, len, entity, priv);
^~
   drivers/usb/gadget/function/f_fs.c:1966:25: note: declared here
static int __must_check ffs_do_single_desc(char *data, unsigned len,
^~

vim +/ffs_do_single_desc +2198 drivers/usb/gadget/function/f_fs.c

  2148  
  2149  static int do_function_enable_interface(enum ffs_entity_type type, u8 
*valuep,
  2150  struct usb_descriptor_header 
*desc,
  2151  void *priv);
  2152  
  2153  static int do_function_disable_interface(enum ffs_entity_type type, u8 
*valuep,
  2154  struct usb_descriptor_header 
*desc,
  2155  void *priv);
  2156  
  2157  static int __must_check ffs_do_descs_alt_intf(unsigned int count, char 
*data,
  2158  unsigned int len, unsigned int 
intf,
  2159  unsigned int alt, void *priv)
  2160  {
  2161  const unsigned int _len = len;
  2162  unsigned long num = 0;
  2163  struct usb_descriptor_header *_ds;
  2164  struct usb_interface_descriptor *idecs;
  2165  ffs_entity_callback entity = NULL;
  2166  
  2167  ENTER();
  2168  
  2169  for (;;) {
  2170  int ret;
  2171  
  2172  if (num == count)
  2173  data = NULL;
  2174  
  2175  if (!data) {
  2176  pr_vdebug("%s Exiting. No more Data\n", 
__func__);
  2177  return _len - len;
  2178  }
  2179  
  2180  _ds = (void *)data;
  2181  if (_ds->bDescriptorType == USB_DT_INTERFACE) {
  2182  idecs = (void *)_ds;
  2183  
  2184  /*Check the interface no to deal with */
  2185  if (idecs->bInterfaceNumber == intf) {
  2186  if (idecs->bAlternateSetting == alt)
  2187  entity = 
do_function_enable_interface;
  2188  else
  2189  entity = 
do_function_disable_interface;
  2190  } else if (entity &&
  2191  (idecs->bInterfaceNumber != 
intf)) {
  2192  pr_vdebug(
  2193  "%s Exiting.Moved past the interface 
no\n",
  2194  
__func__);
  2195  return _len - len;
  2196  }
  2197  }
> 2198  ret = ffs_do_single_desc(data, len, entity, priv);
  2199  if (unlikely(ret < 0)) {
  2200  pr_err("%s Exiting with err %d\n", __func__, 
ret);
  2201  return ret;
  2202  }
  2203  
  2204  len -= ret;
  2205  data += ret;
  2206  ++num;
  2207  }
  2208  }
  2209  

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

XHCI DbC (debug port) problems

[Tj]

I'm assisting in debugging what appears to be a race condition in I2C
code [0] on Intel ComputeStick STK1A32SC [1] devices. As part of that
effort we were originally using an external USB<>RS232 adapter and
"console=ttyUSB0,115200n8" but that can be extremely slow (connected to
a real UART port) and the debug messages seem to prevent the issue occuring.

We decided to switch to use the XHCI debug port and ordered a set of
USB3 A<>A debug crossover cables [2].

We are following the kernel admin guide's USB3 debug port document [3]

We've hit problems in getting the DbC port to work correctly and need
some advice as to whether there is a kernel issue here?

The target is using custom-built 4.20-rc4 kernels with all required
modules built in. The original issue was first detected in Debian
kernels v4.18.*, but didn't occur in v4.9.

The ComputeStick devices have 2 USB ports, 1 is USB3, which the cable
connects to. The host is using v4.18.18 (Fedora) and has usb_debug
module loaded.

Initially the host reported:

usb 2-6: new SuperSpeed Gen 1 USB device number 9 using xhci_hcd
usb 2-6: LPM exit latency is zeroed, disabling LPM.
usb 2-6: language id specifier not provided by device, defaulting to English
usb 2-6: New USB device found, idVendor=1d6b, idProduct=0011, bcdDevice=
0.10
usb 2-6: New USB device strings: Mfr=1, Product=2, SerialNumber=3
usb 2-6: rejected 1 configuration due to insufficient available bus power
usb 2-6: no configuration chosen from 1 choice
mtp-probe[32428]: checking bus 2, device 9:
"/sys/devices/pci:00/:00:14.0/usb2/2-6"
mtp-probe[32428]: bus: 2, device: 9 was not an MTP device
fwupd[2590]: failed to add USB device 1d6b:0011: 1d6b:0011 is not
supported: USB error on device 1d6b:0011 : Entity not found [-5]

Due to "rejected 1 configuration due to insufficient available bus
power" we then enabled the workaround:

# echo 1 > /sys/bus/usb/devices/.../bConfigurationValue

That got us further but then hits repeated -EPROTO errors:

usb 2-6: no configuration chosen from 1 choice
usb 2-6: new config #1 exceeds power limit by 660mA
usb_debug 2-6:1.0: xhci_dbc converter detected
usb 2-6: xhci_dbc converter now attached to ttyUSB0
xhci_dbc ttyUSB0: usb_serial_generic_write_bulk_callback - nonzero urb
status: -71

The last line is repeated constantly several times each second.

The first thing that has confused us is where is the host getting the
"idProduct=0011" from? Looking at the xhci-dbgcap.h the only USB ID is

#define DBC_PRODUCT_ID  0x0010  /* device 0010 */

and xhci-dbgcap.c seems to write it without change:

dev_info = cpu_to_le32((DBC_DEVICE_REV << 16) | DBC_PRODUCT_ID);

usb_serial does declare both 0x0010 and 0x0011 as aliases but that
doesn't explain why the target is reported as 0x0011.

We're wondering if the warning:

"usb_serial_generic_write_bulk_callback - nonzero urb status: -71"

is a regression or bug, rather than us missing something, since we've
tried to do the same operation between two Fedora 4.18.18 PCs and got
exactly the same results.


Secondly, the "new config #1 exceeds power limit by 660mA" report, if we
calculate correctly, suggests that bMaxPower being sent by the target is
900mA + 660mA = 1560mA, but cannot find in the code where bMaxPower is
being set, if any. How is this set by the target?


Any advice or experience as to what we might be missing?

[0] https://bugs.freedesktop.org/show_bug.cgi?id=108714
[1] https://ark.intel.com/products/91064/Intel-Compute-Stick-STK1A32SC
[2]
https://www.datapro.net/products/usb-3-0-super-speed-a-a-debugging-cable.html
[3]
https://www.kernel.org/doc/html/v4.14/driver-api/usb/usb3-debug-port.html

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Fabio Estevam]

On Wed, Dec 5, 2018 at 5:57 AM PETER CHEN  wrote:

> So, there are two examples at binding-doc, one for normal, one for HSIC? 
> Fabio, do you
> mean that? If DT maintainer agrees it too, I will add it.

Yes, I think it will makes things clearer. Thanks

Re: [PATCH v1] usb: dwc3: trace: Refactor nested switch to make compiler happy

[Felipe Balbi]

Andy Shevchenko  writes:

> On Wed, Dec 05, 2018 at 11:18:45AM +0200, Andy Shevchenko wrote:
>> On Wed, Dec 05, 2018 at 11:10:46AM +0200, Felipe Balbi wrote:
>> > Andy Shevchenko  writes:
>> > 
>> > > The missed break statement in the outer switch makes the code fall 
>> > > through
>> > > always and thus always same value will be printed.
>> > >
>> > > Besides that, compiler warns about missed fall through marker:
>> > >
>> > > drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
>> > > drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall 
>> > > through [-Wimplicit-fallthrough=]
>> > > switch (pcm) {
>> > > ^~
>
>> > easier to add "break" here, no? That would be the minimal fix.
>> 
>> No. Then you would need to add same default to the inner switch.
>
> Ah, you meant that pcm would be never outside of the given cases.
> Yes, that's fine then, consider my patch as a bugreport.

updated locally:

From ad7b607f82731eec3ed17d9d22764eb6f09609f9 Mon Sep 17 00:00:00 2001
From: Andy Shevchenko 
Date: Mon, 3 Dec 2018 11:28:47 +0200
Subject: [PATCH] usb: dwc3: trace: add missing break statement to make
 compiler happy
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The missed break statement in the outer switch makes the code fall through
always and thus always same value will be printed.

Besides that, compiler warns about missed fall through marker:

drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
switch (pcm) {
^~

Add the missing break statement to work correctly without compilation
warnings.

Fixes: fa8d965d736b ("usb: dwc3: trace: pretty print high-bandwidth transfers 
too")
Cc: Felipe Balbi 
Signed-off-by: Andy Shevchenko 
Signed-off-by: Felipe Balbi 
---
 drivers/usb/dwc3/trace.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index 50fb6f2d92dd..36e5a4795fc8 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -254,6 +254,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
s = "3x ";
break;
}
+   break;
default:
s = "";
} s; }),
-- 
2.19.2



-- 
balbi


signature.asc
Description: PGP signature

Re: USB driver resets

[Richard van der Hoff]

On 05/12/2018 09:44, Mika Westerberg wrote:

On Tue, Dec 04, 2018 at 04:02:56PM +, Richard van der Hoff wrote:


Sorry. The system is a Dell XPS13 9350, with an Intel DSL6340 Thunderbolt 3
bridge.

The dock is a Plugable UD-CA1 [2].


Also if you have dual boot or similar, do you experience similar issue
in Windows side?


I do have dual-boot, though I very rarely boot into Windows. Since the issue
is somewhat intermittent (for some reason it seems to happen particularly
when I'm in a video conference...) I wouldn't have a huge amount of
confidence of being able to reproduce it under Windows.

Thanks again.


[1] https://bugzilla.kernel.org/show_bug.cgi?id=201853
[2] https://plugable.com/products/ud-ca1/


This seems to be normal USB-C dock (not TBT).


Yes, that's my understanding too.


In the logs it looks like
at some point the USB link just goes down which makes the host side to
hot-remove xHCI and the intermediate PCIe ports.

The UD-CA1 dock site you linked above mentions connection issues with
some systems so I'm suspecting it is an issue with the dock itself not
with the kernel. It would be good if you could try to reproduce on
Windows side as well, basically doing the same things you do in Linux
(assuming it is possible) and see if it triggers.


OK, will give it a go. It'll probably take me a few days.

Thanks for your help.

Re: [PATCH v2 1/3] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name

[Andy Shevchenko]

On Wed, Dec 05, 2018 at 10:25:36AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Dec 03, 2018 at 09:23:44PM +0200, Andy Shevchenko wrote:
> > On Thu, Nov 15, 2018 at 04:53:03PM +0200, Heikki Krogerus wrote:

> > > FWIW, the series:
> > > Reviewed-by: Heikki Krogerus 
> > 
> > Greg, can you pick up this patch (first in the series, since dwc3 went 
> > through Felipe's tree)?
> 
> Sorry for the delay, now queued up.

NP, thanks!

-- 
With Best Regards,
Andy Shevchenko

Re: USB driver resets

[Mika Westerberg]

Hi,

On Tue, Dec 04, 2018 at 04:02:56PM +, Richard van der Hoff wrote:
> Mathias, Mika, thanks very much for your help.
> 
> On 04/12/2018 15:09, Mika Westerberg wrote:
> > On Tue, Dec 04, 2018 at 04:45:33PM +0200, Mathias Nyman wrote:
> > > > On 19/11/2018 15:27, Richard van der Hoff wrote:
> > > > > I have a USB-3.1 dock, which I use for video (via displayport alt 
> > > > > mode) and power delivery, as well as keyboard, mouse, etc.
> > > > > 
> > > > > From time to time, all connectivity with the dock drops out for a few 
> > > > > seconds, before resetting itself. I'm trying to pin down if this is a 
> > > > > problem with the usb drivers, pci drivers, laptop hardware, or the 
> > > > > dock. It looks to me from the dmesg output like it could be a problem 
> > > > > at the PCI layer, but I am only speculating really.
> > > > > 
> > > > > Attached are the dmesg output during the problem, lsusb -v, and lspci 
> > > > > -v. They are all recorded with a 4.15.0 kernel; I've seen exactly the 
> > > > > same symptoms with a number of kernels between 4.8 and 4.15.
> > > > > 
> > > 
> > > Mika (cc) would be the right person to contact about these kind of 
> > > thunderbolt related issues.
> > > I'm sure he would appreciate logs with a 4.19 - 4.20-rcx kernel.
> 
> I've attached the kernel log output from a 4.19 kernel. I've actually had
> some other problems with a 4.19 kernel [1] so have gone back to the 4.15
> kernel for now, but please do let me know if there is anything specific I
> should try.
> 
> I've also re-attached the lsusb -v and lspci -v output (from a 4.15 kernel)
> for your reference.
> 
> > Right, those would help and also some knowledge about which system and
> > dock we are dealing with :)
> 
> Sorry. The system is a Dell XPS13 9350, with an Intel DSL6340 Thunderbolt 3
> bridge.
> 
> The dock is a Plugable UD-CA1 [2].
> 
> > Also if you have dual boot or similar, do you experience similar issue
> > in Windows side?
> 
> I do have dual-boot, though I very rarely boot into Windows. Since the issue
> is somewhat intermittent (for some reason it seems to happen particularly
> when I'm in a video conference...) I wouldn't have a huge amount of
> confidence of being able to reproduce it under Windows.
> 
> Thanks again.
> 
> 
> [1] https://bugzilla.kernel.org/show_bug.cgi?id=201853
> [2] https://plugable.com/products/ud-ca1/

This seems to be normal USB-C dock (not TBT). In the logs it looks like
at some point the USB link just goes down which makes the host side to
hot-remove xHCI and the intermediate PCIe ports.

The UD-CA1 dock site you linked above mentions connection issues with
some systems so I'm suspecting it is an issue with the dock itself not
with the kernel. It would be good if you could try to reproduce on
Windows side as well, basically doing the same things you do in Linux
(assuming it is possible) and see if it triggers.

Re: [PATCH 0/3] Small improvements to the appledisplay driver

[Greg Kroah-Hartman]

On Tue, Dec 04, 2018 at 11:43:34PM +0100, alex.theis...@me.com wrote:
> With the exception of the first patch I am not entirely sure if my changes are
> correct and justified. This is my first contribution and I am equally 
> satisfied
> if someone could point out why my changes are not correct.
> 
> The added display is my own and works well with the driver.

All looks good to me, thanks for the patches!

greg k-h

Re: [PATCH v1] usb: dwc3: trace: Refactor nested switch to make compiler happy

[Andy Shevchenko]

On Wed, Dec 05, 2018 at 11:18:45AM +0200, Andy Shevchenko wrote:
> On Wed, Dec 05, 2018 at 11:10:46AM +0200, Felipe Balbi wrote:
> > Andy Shevchenko  writes:
> > 
> > > The missed break statement in the outer switch makes the code fall through
> > > always and thus always same value will be printed.
> > >
> > > Besides that, compiler warns about missed fall through marker:
> > >
> > > drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
> > > drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall 
> > > through [-Wimplicit-fallthrough=]
> > > switch (pcm) {
> > > ^~

> > easier to add "break" here, no? That would be the minimal fix.
> 
> No. Then you would need to add same default to the inner switch.

Ah, you meant that pcm would be never outside of the given cases.
Yes, that's fine then, consider my patch as a bugreport.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH] USB: quirks: add NO_LPM quirk for Logitech Flare

[Greg KH]

On Wed, Nov 28, 2018 at 06:19:31PM -0500, Kyle Williams wrote:
> Description: Some USB device / host controller combinations seem to have
> problems with Link Power management. In particular it is described that
> the combination of a Logitech Flare and other powered devices such as
> the Atrus device causes 'not enough bandwidth for new device state'error
> 
> This patch creates a quirk entry for the Logitech Flare device
> indicating LPM should remain disabled for the device.
> 
> Signed-off-by: Kyle Williams 
> Cc: stable 
> ---
>  drivers/usb/core/quirks.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/core/quirks.c b/drivers/usb/core/quirks.c
> index 49c44d89a394..e43c72cc98ee 100644
> --- a/drivers/usb/core/quirks.c
> +++ b/drivers/usb/core/quirks.c
> @@ -224,6 +224,9 @@ static const struct usb_device_id usb_quirk_list[] = {
> /* Logitech Logitech Screen Share */
> { USB_DEVICE(0x046d, 0x086c), .driver_info = USB_QUIRK_NO_LPM },
> 
> +   /* Logitech Flare */
> +   { USB_DEVICE(0x046d, 0x0876), .driver_info = USB_QUIRK_NO_LPM },
> +
> /* Logitech Rally Camera */
> { USB_DEVICE(0x046d, 0x0881), .driver_info = USB_QUIRK_NO_LPM },
> { USB_DEVICE(0x046d, 0x0888), .driver_info = USB_QUIRK_NO_LPM },
> -- 
> 2.18.1

Tabs are all converted to spaces, making this patch impossible to apply
:(

Re: [PATCH v2 1/3] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name

[Greg Kroah-Hartman]

On Mon, Dec 03, 2018 at 09:23:44PM +0200, Andy Shevchenko wrote:
> On Thu, Nov 15, 2018 at 04:53:03PM +0200, Heikki Krogerus wrote:
> > On Thu, Nov 15, 2018 at 03:16:19PM +0200, Andy Shevchenko wrote:
> > > Since we are going to use the same in Designware USB 3 driver,
> > > rename the property to be consistent across the drivers.
> > > 
> > > No functional change intended.
> > > 
> > > Signed-off-by: Andy Shevchenko 
> > > Cc: Hans de Goede 
> > > Cc: Guenter Roeck 
> > > Acked-by: Hans de Goede 
> > > Acked-by: Guenter Roeck 
> > 
> > FWIW, the series:
> > Reviewed-by: Heikki Krogerus 
> 
> Greg, can you pick up this patch (first in the series, since dwc3 went 
> through Felipe's tree)?

Sorry for the delay, now queued up.

greg k-h

Re: [PATCH v1] usb: dwc3: trace: Refactor nested switch to make compiler happy

[Andy Shevchenko]

On Wed, Dec 05, 2018 at 11:10:46AM +0200, Felipe Balbi wrote:
> Andy Shevchenko  writes:
> 
> > The missed break statement in the outer switch makes the code fall through
> > always and thus always same value will be printed.
> >
> > Besides that, compiler warns about missed fall through marker:
> >
> > drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
> > drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall through 
> > [-Wimplicit-fallthrough=]
> > switch (pcm) {
> > ^~
> >
> > Refactor nested switch statements to work correctly without
> > compilation warnings.
> >
> > Fixes: fa8d965d736b ("usb: dwc3: trace: pretty print high-bandwidth 
> > transfers too")
> > Cc: Felipe Balbi 
> > Signed-off-by: Andy Shevchenko 
> > ---
> >  drivers/usb/dwc3/trace.h | 4 +---
> >  1 file changed, 1 insertion(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> > index f22714cce070..8e1625a6c19f 100644
> > --- a/drivers/usb/dwc3/trace.h
> > +++ b/drivers/usb/dwc3/trace.h
> > @@ -238,7 +238,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
> > ),
> > TP_printk("%s: trb %p buf %08x%08x size %s%d ctrl %08x 
> > (%c%c%c%c:%c%c:%s)",
> > __get_str(name), __entry->trb, __entry->bph, __entry->bpl,
> > -   ({char *s;
> > +   ({ char *s = "";
> > int pcm = ((__entry->size >> 24) & 3) + 1;
> > switch (__entry->type) {
> > case USB_ENDPOINT_XFER_INT:
> > @@ -254,8 +254,6 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
> > s = "3x ";
> > break;
> > }
> 
> easier to add "break" here, no? That would be the minimal fix.

No. Then you would need to add same default to the inner switch.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 2/2] usb: dwc3: gadget: Report isoc transfer frame number

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
> Implement the new frame_number API to report the isochronous interval
> frame number. This patch checks and reports the interval in which the
> isoc transfer was transmitted or received via the Isoc-First TRB SOF
> number field.
>
> Signed-off-by: Thinh Nguyen 
> ---
> Change in v3:
> - Implement the change with the redefined frame_number meaning
> Change in v2:
> - Capture frame number at request cleanup
>
>  drivers/usb/dwc3/core.h   |  1 +
>  drivers/usb/dwc3/gadget.c | 13 +
>  2 files changed, 14 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index ed0359d1216d..2c9f7a93147a 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -777,6 +777,7 @@ enum dwc3_link_state {
>  #define DWC3_TRB_CTRL_ISP_IMIBIT(10)
>  #define DWC3_TRB_CTRL_IOCBIT(11)
>  #define DWC3_TRB_CTRL_SID_SOFN(n)(((n) & 0x) << 14)
> +#define DWC3_TRB_CTRL_GET_SID_SOFN(n)(((n) & (0x << 14)) >> 14)
>  
>  #define DWC3_TRBCTL_TYPE(n)  ((n) & (0x3f << 4))
>  #define DWC3_TRBCTL_NORMAL   DWC3_TRB_CTRL_TRBCTL(1)
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 679c12e14522..2de563124fc1 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2254,6 +2254,19 @@ static int dwc3_gadget_ep_reclaim_completed_trb(struct 
> dwc3_ep *dep,
>   if (chain && (trb->ctrl & DWC3_TRB_CTRL_HWO))
>   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
>  
> + /*
> +  * For isochronous transfers, the first TRB in a service interval must
> +  * have the Isoc-First type. Track and report its interval frame number.
> +  */
> + if (usb_endpoint_xfer_isoc(dep->endpoint.desc) &&
> + (trb->ctrl & DWC3_TRBCTL_ISOCHRONOUS_FIRST)) {
> + unsigned int frame_number;
> +
> + frame_number = DWC3_TRB_CTRL_GET_SID_SOFN(trb->ctrl);
> + frame_number &= ~(dep->interval - 1);
> + req->request.frame_number = frame_number;
> + }

could you refresh my memory on how this is going to be used?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v1] usb: dwc3: trace: Refactor nested switch to make compiler happy

[Felipe Balbi]

Andy Shevchenko  writes:

> The missed break statement in the outer switch makes the code fall through
> always and thus always same value will be printed.
>
> Besides that, compiler warns about missed fall through marker:
>
> drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
> drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall through 
> [-Wimplicit-fallthrough=]
> switch (pcm) {
> ^~
>
> Refactor nested switch statements to work correctly without
> compilation warnings.
>
> Fixes: fa8d965d736b ("usb: dwc3: trace: pretty print high-bandwidth transfers 
> too")
> Cc: Felipe Balbi 
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/usb/dwc3/trace.h | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> index f22714cce070..8e1625a6c19f 100644
> --- a/drivers/usb/dwc3/trace.h
> +++ b/drivers/usb/dwc3/trace.h
> @@ -238,7 +238,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
>   ),
>   TP_printk("%s: trb %p buf %08x%08x size %s%d ctrl %08x 
> (%c%c%c%c:%c%c:%s)",
>   __get_str(name), __entry->trb, __entry->bph, __entry->bpl,
> - ({char *s;
> + ({ char *s = "";
>   int pcm = ((__entry->size >> 24) & 3) + 1;
>   switch (__entry->type) {
>   case USB_ENDPOINT_XFER_INT:
> @@ -254,8 +254,6 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
>   s = "3x ";
>   break;
>   }

easier to add "break" here, no? That would be the minimal fix.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] dma: cppi41: delete channel from pending list when stop channel

[Vinod Koul]

On 28-11-18, 13:15, Peter Ujfalusi wrote:
> 
> 
> On 12/11/2018 17.40, Bin Liu wrote:
> 
> Can you fix up the subject line to:
> dmaengine: ti: cppi4: delete channel from pending list when stop channel
> 
> > The driver defines three states for a cppi channel.
> > - idle: .chan_busy == 0 && not in .pending list
> > - pending: .chan_busy == 0 && in .pending list
> > - busy: .chan_busy == 1 && not in .pending list
> > 
> > There are cases in which the cppi channel could be in the pending state
> > when cppi41_dma_issue_pending() is called after cppi41_runtime_suspend()
> > is called.
> > 
> > cppi41_stop_chan() has a bug for these cases to set channels to idle state.
> > It only checks the .chan_busy flag, but not the .pending list, then later
> > when cppi41_runtime_resume() is called the channels in .pending list will
> > be transitioned to busy state.
> > 
> > Removing channels from the .pending list solves the problem.
> 
> So, let me see if I understand this correctly:
> - client issued a transfer _after_ the cppi4 driver is suspended
> - cppi41_dma_issue_pending() will place it to pending list and will not
> start the transfer right away as cdd->is_suspended is true.
> - on resume the cppi4 will pick up the pending transfers from the
> pending list
> 
> This is so far a sane thing to do.
> 
> If I guess right, then after the issue_pending the client driver will
> call terminate_all, presumably from it's suspend callback?
> 
> As per the purpose of terminate_all we should terminated all future
> transfers on the channel, so clearing the pending list is the correct
> thing to do.
> 
> With the fixed subject:
> Reviewed-by: Peter Ujfalusi 

Thanks Peter,

Applied after fixing the title, thanks

-- 
~Vinod

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

On 05.12.18 08:45, Schrempf Frieder wrote:
> Hi Fabio,
> 
> On 04.12.18 21:01, Fabio Estevam wrote:
>> Hi Frieder,
>>
>> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
>>  wrote:
>>
>>> There are many other optional properties for this driver and a lot of
>>> them are not in the given example. Maybe we should just keep the
>>> pinctrls for HSIC-mode out of the example, too?
>>
>> I am just trying to make life easier for those who want to use HSIC
>> support with chipidea.
>>
>> Can we just add a real dts snippet example of your board into the
>> binding document?
> 
> Sure, here is what I have in my dts:
> 
>  {
>   pinctrl-names = "idle", "active";
>   pinctrl-0 = <_usbh2_idle>;
>   pinctrl-1 = <_usbh2_active>;
>   status = "okay";
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   usbnet: smsc@1 {
>   compatible = "usb424,9730";
>   reg = <1>;
>   };
> };

Or merged with the settings from imx6qdl.dtsi this will look like below. 
Maybe this is better as it is the complete node without depending on 
imx6qdl.dtsi:

usbh2: usb@2184400 {
compatible = "fsl,imx6q-usb", "fsl,imx27-usb";
reg = <0x02184400 0x200>;
interrupts = <0 41 IRQ_TYPE_LEVEL_HIGH>;
clocks = < IMX6QDL_CLK_USBOH3>;
fsl,usbphy = <>;
fsl,usbmisc = < 2>;
phy_type = "hsic";
dr_mode = "host";
ahb-burst-config = <0x0>;
tx-burst-size-dword = <0x10>;
rx-burst-size-dword = <0x10>;
pinctrl-names = "idle", "active";
pinctrl-0 = <_usbh2_idle>;
pinctrl-1 = <_usbh2_active>;
#address-cells = <1>;
#size-cells = <0>;

usbnet: smsc@1 {
compatible = "usb424,9730";
reg = <1>;
};
};

> 
> @Peter: Can you add this as a second example to the binding documentation?
> 
> Thanks,
> Frieder
> 

RE: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[PETER CHEN]

 
> On 04.12.18 21:01, Fabio Estevam wrote:
> > Hi Frieder,
> >
> > On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
> >  wrote:
> >
> >> There are many other optional properties for this driver and a lot of
> >> them are not in the given example. Maybe we should just keep the
> >> pinctrls for HSIC-mode out of the example, too?
> >
> > I am just trying to make life easier for those who want to use HSIC
> > support with chipidea.
> >
> > Can we just add a real dts snippet example of your board into the
> > binding document?
> 
> Sure, here is what I have in my dts:
> 
>  {
>   pinctrl-names = "idle", "active";
>   pinctrl-0 = <_usbh2_idle>;
>   pinctrl-1 = <_usbh2_active>;
>   status = "okay";
>   #address-cells = <1>;
>   #size-cells = <0>;
> 
>   usbnet: smsc@1 {
>   compatible = "usb424,9730";
>   reg = <1>;
>   };
> };
> 
> @Peter: Can you add this as a second example to the binding documentation?
> 

So, there are two examples at binding-doc, one for normal, one for HSIC? Fabio, 
do you
mean that? If DT maintainer agrees it too, I will add it.

Peter

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

Hi Fabio,

On 04.12.18 21:01, Fabio Estevam wrote:
> Hi Frieder,
> 
> On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
>  wrote:
> 
>> There are many other optional properties for this driver and a lot of
>> them are not in the given example. Maybe we should just keep the
>> pinctrls for HSIC-mode out of the example, too?
> 
> I am just trying to make life easier for those who want to use HSIC
> support with chipidea.
> 
> Can we just add a real dts snippet example of your board into the
> binding document?

Sure, here is what I have in my dts:

 {
pinctrl-names = "idle", "active";
pinctrl-0 = <_usbh2_idle>;
pinctrl-1 = <_usbh2_active>;
status = "okay";
#address-cells = <1>;
#size-cells = <0>;

usbnet: smsc@1 {
compatible = "usb424,9730";
reg = <1>;
};
};

@Peter: Can you add this as a second example to the binding documentation?

Thanks,
Frieder

Re: [PATCH] USB: qcaux: Add Motorola modem UARTs

[Johan Hovold]

On Sun, Dec 02, 2018 at 05:34:24PM -0800, Tony Lindgren wrote:
> On Motorola Mapphone devices such as Droid 4 there are five USB ports
> that do not use the same layout as Gobi 1K/2K/etc devices listed in
> qcserial.c. So we should use qcaux.c or option.c as noted by
> Dan Williams .
> 
> The ff/ff/ff interfaces seem to always be UARTs on Motorola devices.
> And we should not add interfaces with 0x0a class (CDC Data) as they
> are part of a multi-interface function like for example interface
> 0x22b8:0x4281 as noted by Bjørn Mork .

Can you post the output of usb-devices (or lsusb -v) for these three
devices (PIDs)?

Thanks,
Johan

[PATCH 2/3] usb: appledisplay: Set urb transfer_flags to URB_NO_TRANSFER_DMA_MAP

[alex . theissen]

From: Alexander Theissen 

The driver does allocate a DMA address with usb_alloc_coherent but did
not set the appropriate flag to signal that transfer_dma is set to a
valid value.

Signed-off-by: Alexander Theissen 
---
 drivers/usb/misc/appledisplay.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 39ca31b4de46..2f3c4769238d 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -261,6 +261,7 @@ static int appledisplay_probe(struct usb_interface *iface,
usb_rcvintpipe(udev, int_in_endpointAddr),
pdata->urbdata, ACD_URB_BUFFER_LEN, appledisplay_complete,
pdata, 1);
+   pdata->urb->transfer_flags = URB_NO_TRANSFER_DMA_MAP;
if (usb_submit_urb(pdata->urb, GFP_KERNEL)) {
retval = -EIO;
dev_err(>dev, "Submitting URB failed\n");
-- 
2.19.1

[PATCH 3/3] usb: appledisplay: Remove unnecessary spinlock

[alex . theissen]

From: Alexander Theissen 

The spinlock was inside the urb completion function which is only
called once per display and is then resubmitted from this function.
There was no other place where this lock was used.

Signed-off-by: Alexander Theissen 
---
 drivers/usb/misc/appledisplay.c | 7 ---
 1 file changed, 7 deletions(-)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 2f3c4769238d..ac92725458b5 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -69,7 +69,6 @@ struct appledisplay {
 
struct delayed_work work;
int button_pressed;
-   spinlock_t lock;
struct mutex sysfslock; /* concurrent read and write */
 };
 
@@ -79,7 +78,6 @@ static void appledisplay_complete(struct urb *urb)
 {
struct appledisplay *pdata = urb->context;
struct device *dev = >udev->dev;
-   unsigned long flags;
int status = urb->status;
int retval;
 
@@ -105,8 +103,6 @@ static void appledisplay_complete(struct urb *urb)
goto exit;
}
 
-   spin_lock_irqsave(>lock, flags);
-
switch(pdata->urbdata[1]) {
case ACD_BTN_BRIGHT_UP:
case ACD_BTN_BRIGHT_DOWN:
@@ -119,8 +115,6 @@ static void appledisplay_complete(struct urb *urb)
break;
}
 
-   spin_unlock_irqrestore(>lock, flags);
-
 exit:
retval = usb_submit_urb(pdata->urb, GFP_ATOMIC);
if (retval) {
@@ -229,7 +223,6 @@ static int appledisplay_probe(struct usb_interface *iface,
 
pdata->udev = udev;
 
-   spin_lock_init(>lock);
INIT_DELAYED_WORK(>work, appledisplay_work);
mutex_init(>sysfslock);
 
-- 
2.19.1

[PATCH 0/3] Small improvements to the appledisplay driver

[alex . theissen]

With the exception of the first patch I am not entirely sure if my changes are
correct and justified. This is my first contribution and I am equally satisfied
if someone could point out why my changes are not correct.

The added display is my own and works well with the driver.

drivers/usb/misc/appledisplay.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

[PATCH 1/3] usb: appledisplay: Add 27" Apple Cinema Display

[alex . theissen]

From: Alexander Theissen 

Add another Apple Cinema Display to the list of supported displays.

Signed-off-by: Alexander Theissen 
---
 drivers/usb/misc/appledisplay.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/usb/misc/appledisplay.c b/drivers/usb/misc/appledisplay.c
index 85b48c6ddc7e..39ca31b4de46 100644
--- a/drivers/usb/misc/appledisplay.c
+++ b/drivers/usb/misc/appledisplay.c
@@ -51,6 +51,7 @@ static const struct usb_device_id appledisplay_table[] = {
{ APPLEDISPLAY_DEVICE(0x921c) },
{ APPLEDISPLAY_DEVICE(0x921d) },
{ APPLEDISPLAY_DEVICE(0x9222) },
+   { APPLEDISPLAY_DEVICE(0x9226) },
{ APPLEDISPLAY_DEVICE(0x9236) },
 
/* Terminating entry */
-- 
2.19.1

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Fabio Estevam]

Hi Frieder,

On Tue, Dec 4, 2018 at 12:31 PM Schrempf Frieder
 wrote:

> There are many other optional properties for this driver and a lot of
> them are not in the given example. Maybe we should just keep the
> pinctrls for HSIC-mode out of the example, too?

I am just trying to make life easier for those who want to use HSIC
support with chipidea.

Can we just add a real dts snippet example of your board into the
binding document?

dwc2 isochronous transfers issues

[Andrzej Pietrasiewicz]

Hi All,

I have recently noticed a problem with dwc2 and audio gadget.

The expected behavior:

The uac2 function acts as a kind of a pass-through.
For example to play audio from USB host on an Odroid U3 the following command
is issued at the host:

aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav

while the following command is used at the device:

arecord -f dat -t wav -D plughw:CARD=UAC2Gadget,DEV=0 \|
aplay -D default:CARD=OdroidU3

At the device we "record" from the audio device provided by the usb gadget
and pipe the resulting stream to the actual hardware device present in
the chip on board.

What actually happens:

On recent kernels this does not work in a stable manner and sometimes
the kernel panics. I traced the problem back and it seems that the last
kernel without the below problems is 4.7.

I am using a compiled-in, legacy audio gadget with only uac2 function
selected in Kconfig.

4.7 works
4.8 kernel crashes at boot time
(bisect identifies 381fc8f8228923026b3d75c8230fa2ee4d688f32
"usb: dwc2: gadget: Add Incomplete ISO IN/OUT Interrupt handlers"),
compiling as a module does not help (crash at module load time),
composing the gagdet with configfs does not help either.

4.9 kernel crashes at boot time
4.10kernel crashes at boot time
4.11kernel crashes at boot time
4.12kernel crashes at boot time
4.13kernel crashes at boot time
4.14kernel usually crashes at boot time, but sometimes it boots,
if it boots, it crashes the same way as at boot time when
the gadget is actually used (aplay at host and arecord | aplay
at the device)
4.15kernel sometimes crashes at boot time, but otherwise it boots,
if it boots, it crashes the same way as at boot time when
the gadget is actually used (aplay at host and arecord | aplay
at the device)
4.16kernel crashes at boot time
4.17kernel sometimes crashes at boot time, but otherwise it boots,
if it boots, it crashes the same way as at boot time when
the gadget is actually used (aplay at host and arecord | aplay
at the device)
4.18kernel sometimes crashes at boot time or even silently freezes,
but otherwise it boots, if it boots, it crashes the same way as
at boot time when the gadget is actually used (aplay at host
and arecord | aplay at the device)
4.19 and 4.20-rc4
kernel boots, but after some time the gadget stops working:

at the device with 4.19:

# arecord -f dat -t wav -D plughw:CARD=UAC2Gadget,DEV=0 | aplay -D 
default:CARD=OdroidU3
Recording WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
Playing WAVE 'stdin' : Signed 16 bit Little Endian, Rate 48000 Hz, Stereo
[   26.245056] max98090 1-0010: PLL unlocked
underrun!!! (at least 1256.128 ms long)
[   29.363400] max98090 1-0010: PLL unlocked
underrun!!! (at least 930.612 ms long)
underrun!!! (at least 447.733 ms long)
[   34.475390] max98090 1-0010: PLL unlocked
underrun!!! (at least 319.622 ms long)
[   36.678554] max98090 1-0010: PLL unlocked
underrun!!! (at least 611.690 ms long)
[   39.153660] max98090 1-0010: PLL unlocked
underrun!!! (at least 724.196 ms long)
[   41.741488] max98090 1-0010: PLL unlocked
underrun!!! (at least 695.816 ms long)
[   44.428490] max98090 1-0010: PLL unlocked
underrun!!! (at least 795.061 ms long)
[   47.106276] max98090 1-0010: PLL unlocked
underrun!!! (at least 760.757 ms long)
underrun!!! (at least 2685.446 ms long)
[   54.288479] max98090 1-0010: PLL unlocked
underrun!!! (at least 1591.909 ms long)
[   57.764445] max98090 1-0010: PLL unlocked
underrun!!! (at least 223.942 ms long)
underrun!!! (at least 237.751 ms long)
[   61.974963] max98090 1-0010: PLL unlocked
underrun!!! (at least 677.925 ms long)
[   64.645406] max98090 1-0010: PLL unlocked
underrun!!! (at least 550.651 ms long)
[   67.079379] max98090 1-0010: PLL unlocked
underrun!!! (at least 1575.582 ms long)
[   70.521155] max98090 1-0010: PLL unlocked
underrun!!! (at least 832.604 ms long)
[   73.219814] max98090 1-0010: PLL unlocked
underrun!!! (at least 721.516 ms long)


while at the host:

$ aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav
Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little 
Endian, Rate 48000 Hz, Mono

 several times ok 

$ aplay -D plughw:CARD=Gadget,DEV=0 /usr/share/sounds/alsa/Front_Center.wav
Playing WAVE '/usr/share/sounds/alsa/Front_Center.wav' : Signed 16 bit Little 
Endian, Rate 48000 Hz, Mono
aplay: set_params:1297: Unable to install hw params:
ACCESS:  RW_INTERLEAVED
FORMAT:  S16_LE
SUBFORMAT:  STD
SAMPLE_BITS: 16
FRAME_BITS: 16
CHANNELS: 1
RATE: 48000
PERIOD_TIME: 125000
PERIOD_SIZE: 6000
PERIOD_BYTES: 12000
PERIODS: 4
BUFFER_TIME: 50
BUFFER_SIZE: 24000
BUFFER_BYTES: 48000
TICK_TIME: 0

Any ideas on how to test it further to identify the problem?

Definitely the commit identified with bisect was a problem at least
until 4.13. 

Re: pl2303 regression after v4.17. Bisected to 7041d9c3f01b

[Johan Hovold]

On Mon, Nov 26, 2018 at 02:29:53PM +0200, Jarkko Nikula wrote:
> On 11/24/18 7:32 PM, Florian Zumbiehl wrote:
> > Hi,
> > 
> >> Requested baud setting looks odd to me. Maybe related to this
> >> --keep-baud flag in "/sbin/agetty -o -p -- \u --keep-baud
> >> 115200,38400,9600 ttyUSB0 vt220"?

That indeed seems to be the case.

> > Well, what is the baud rate of the other side? It seems a bit strange
> > that ...
> > 
> 115200.
> 
> >> 1. serial-getty@ttyUSB0.service disabled
> >>
> >> speed 9600 baud; rows 0; columns 0; line = 0;
> > 
> > ... this works ...
> > 
> >> 3. serial-getty@ttyUSB0.service disabled && picocom -b 115200 /dev/ttyUSB0
> >>
> >> speed 115200 baud; rows 0; columns 0; line = 0;
> > 
> > ... and this works as well?!
> > 
> > I think before debugging a potential problem with the new flow control
> > feature, it would be a good idea to first make sure the baud rate is
> > configured correctly. Maybe agetty's baud rate selection is somehow
> > influenced by the new flow control support? I don't see how it would, and
> > maybe that's still a bug somehow, but I suspect that there is no flow
> > control problem at all, but rather it's just a baud rate mismatch that
> > prevents communication.
> > 
> I went debugging the commit and pl2303_set_termios() a bit. I noticed 
> that the tty_termios_hw_change() returns zero when starting the getty 
> service that keeps the existing baud. Same goes with the plain "picocom 
> /dev/ttyUSB0" without specifying the baud which also works before commit 
> 7041d9c3f01b.
> 
> For both cases ixon_change is true so the pl2303_termios_change() 
> returns true causing the pl2303_set_termios() to go forward doing the 
> baudrate (incorrectly 9600 instead of 115200) etc. changes which doesn't 
> happen before commit 7041d9c3f01b.
> 
> This incorrect 9600 baud made me thinking could it be possible that 
> driver doesn't initialize some structure with the current settings the 
> kernel console is using? Would this also explain also why
> "stty -a -F /dev/ttyUSB0" shows 9600 when kernel console is certainly 
> using 115200 (after boot before running neither agetty or picocom)?

Correct. The USB-serial console code has never reported the actual
settings used, but with the pl2303 flow control change this now
obviously broke your setup.

Judging from your reports, including the stty outputs, it seems like the
getty is trying to disable flow control while keeping the baud rate
unchanged. Prior to the pl2303 change this was a noop which left the
baud rate unchanged, but now you end up with the driver default 9600
baud.

> I verified the same setup using legacy 8250 uart and with it the
> "stty -a -F /dev/ttyS0" shows correctly 115200 baud.

Right, the serial console correctly updates the terminal settings,
unlike the USB-serial console code.

I've come up with a non-intrusive fix which fixes the incorrectly
reported terminal settings. Would you mind giving it a try?

Thanks,
Johan

Re: USB driver resets

[Mika Westerberg]

On Tue, Dec 04, 2018 at 04:45:33PM +0200, Mathias Nyman wrote:
> On 03.12.2018 12:36, Richard van der Hoff wrote:
> > Does anybody have any suggestions as to how I could set about debugging 
> > this? I'm seeing the same symptoms on a 4.19 kernel.
> > 
> > 
> > On 19/11/2018 15:27, Richard van der Hoff wrote:
> > > I have a USB-3.1 dock, which I use for video (via displayport alt mode) 
> > > and power delivery, as well as keyboard, mouse, etc.
> > > 
> > > From time to time, all connectivity with the dock drops out for a few 
> > > seconds, before resetting itself. I'm trying to pin down if this is a 
> > > problem with the usb drivers, pci drivers, laptop hardware, or the dock. 
> > > It looks to me from the dmesg output like it could be a problem at the 
> > > PCI layer, but I am only speculating really.
> > > 
> > > Attached are the dmesg output during the problem, lsusb -v, and lspci -v. 
> > > They are all recorded with a 4.15.0 kernel; I've seen exactly the same 
> > > symptoms with a number of kernels between 4.8 and 4.15.
> > > 
> > > Advice appreciated!
> > > 
> 
> Mika (cc) would be the right person to contact about these kind of 
> thunderbolt related issues.
> I'm sure he would appreciate logs with a 4.19 - 4.20-rcx kernel.

Right, those would help and also some knowledge about which system and
dock we are dealing with :)

Also if you have dual boot or similar, do you experience similar issue
in Windows side?

Re: USB driver resets

[Mathias Nyman]

On 03.12.2018 12:36, Richard van der Hoff wrote:

Does anybody have any suggestions as to how I could set about debugging this? 
I'm seeing the same symptoms on a 4.19 kernel.


On 19/11/2018 15:27, Richard van der Hoff wrote:

I have a USB-3.1 dock, which I use for video (via displayport alt mode) and 
power delivery, as well as keyboard, mouse, etc.

From time to time, all connectivity with the dock drops out for a few seconds, 
before resetting itself. I'm trying to pin down if this is a problem with the 
usb drivers, pci drivers, laptop hardware, or the dock. It looks to me from the 
dmesg output like it could be a problem at the PCI layer, but I am only 
speculating really.

Attached are the dmesg output during the problem, lsusb -v, and lspci -v. They 
are all recorded with a 4.15.0 kernel; I've seen exactly the same symptoms with 
a number of kernels between 4.8 and 4.15.

Advice appreciated!



Mika (cc) would be the right person to contact about these kind of thunderbolt 
related issues.
I'm sure he would appreciate logs with a 4.19 - 4.20-rcx kernel.

-Mathias

Re: [PATCH v3 4/4] doc: usb: ci-hdrc-usb2: Add pinctrl properties for HSIC pin groups

[Schrempf Frieder]

Hi Peter, hi Fabio,

On 30.11.18 03:33, PETER CHEN wrote:
>   
>>
>> On Tue, Nov 27, 2018 at 7:31 AM PETER CHEN  wrote:
>>>
>>> For USB HSIC, the data and strobe pin needs to be pulled down at
>>> default, we consider it as "idle" state. When the USB host is ready to
>>> be used, the strobe pin needs to be pulled up, we consider it as
>>> "active" state.
>>>
>>> Signed-off-by: Peter Chen 
>>> ---
>>>   Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt | 8 +++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> index 529e51879fb2..1e5e7ddfb1a5 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> @@ -80,7 +80,10 @@ Optional properties:
>>> controller. It's expected that a mux state of 0 indicates device mode 
>>> and a
>>> mux state of 1 indicates host mode.
>>>   - mux-control-names: Shall be "usb_switch" if mux-controls is specified.
>>> -- pinctrl-names: Names for optional pin modes in "default", "host", 
>>> "device"
>>> +- pinctrl-names: Names for optional pin modes in "default", "host", 
>>> "device".
>>> +  In case of HSIC-mode, "idle" and "active" pin modes are mandatory.
>>> +In this
>>> +  case, the "idle" state needs to pull down the data and strobe pin
>>> +  and the "active" state needs to pull up the strobe pin.
>>>   - pinctrl-n: alternate pin modes
>>>
>>>   i.mx specific properties
>>> @@ -110,4 +113,7 @@ Example:
>>>  phy-clkgate-delay-us = <400>;
>>>  mux-controls = <_switch>;
>>>  mux-control-names = "usb_switch";
>>> +   pinctrl-names = "idle", "active";
>>> +   pinctrl-0 = <_usbh2_1>;
>>> +   pinctrl-1 = <_usbh2_2>;
>>
>> I would put the "idle" and "active" entries inside a explicit section for 
>> HSIC.
>>
>> Otherwise, it may confuse people using non-HSIC bindings.
> 
> It is just an example; the user should check the meaning for optional 
> properties before
> using it, like the property "phy-clkgate-delay-us", only imx7d/7s needs it.

There are many other optional properties for this driver and a lot of 
them are not in the given example. Maybe we should just keep the 
pinctrls for HSIC-mode out of the example, too?

Regards,
Frieder

Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Uwe Kleine-König]

Hello,

On Tue, Dec 04, 2018 at 12:43:21PM +0100, Stefan Wahren wrote:
> Am 04.12.18 um 09:52 schrieb Uwe Kleine-König:
> > But for the review you are right, I added the dt people to Cc for them
> > to comment. In v2 Matthew also noted that he would prefer to handle the
> > situation when both over-current-active-low and over-current-active-high
> > were given differently. I think we don't need that as this is a case of
> > "broken dt" and it doesn't matter much what is done then. (With my patch
> > we're configuring for active high in that case.) A feedback here would
> > be great, too.
> 
> AFAIR such invalid settings should be catched with a proper error
> message and abort of the probing.

I remember a review commend from Rob[1]: "It is not really the kernel's
job to validate crap in bindings. If you put [strange things] in your
DT, then the kernel may or may not handle that."

If this applies here, too, the current patches are fine.

Best regards
Uwe

[1] https://patchwork.kernel.org/patch/8776441/

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

FROM MISS DIANA HOLLAND

[miss Diana]

Attention,



Good day to you, how are you? My name is Diana Holland. I am a U.S. Army
Specialist, serving here in Afghanistan. I found a box containing the
sum of $10,000,000.00
USD (Ten Million United States Dollars) due to my status as a U.S.
Army Specialist,
i can’t be able to transfer these funds to my account in United States.


I need your
help to evacuate this funds and safe keep it until i finish my service here and
come to collect my share of it. The box will be addressed as family valuables
from me to you. If this is possible for you to do, i need your full assurance
that it will be safe in your care. You may be wondering why i decided to
communicate with you,


I have no
one to discuss this with. I am an orphan, please assist me to secure a brighter
future. Trusting someone is very difficult, but i give you my trust, also i
need your trust to make this a success. Your acceptance to this would really
encourage me. Kindly let me know your conclusion to my request so we can
proceed to get this completed soon. With a sincere heart,


I am willing to give you 35% from the total
sum for your assistance and willingness to help me. Please do not betray my
trust and confidence in you. If you truly desire to help me, please reply me
quickly via my Personal email: (diana.hollan...@yahoo.com )



Regards, Diana Holland. Personal Email: (diana.hollan...@yahoo.com)

Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Stefan Wahren]

Am 04.12.18 um 09:52 schrieb Uwe Kleine-König:
> Hello Stefan,
>
> On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote:
>> Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
>>> The status quo on i.MX6 is that if "over-current-active-high" is
>>> specified in the device tree this is configured as expected. If
>>> the property is missing polarity isn't changed and so the
>>> polarity is kept as setup by the bootloader. Reset default is
>>> active high, so active low can only be used with help by the
>>> bootloader. On i.MX7 it is similar, but there disabling of
>>> over current detection has a similar inconsistency.
>>>
>>> This patch introduces a new property that allows to explicitly
>>> configure for active low over current detection and consistently
>>> sets this up. In the absence of an explicit configuration the
>>> bit is kept as is. On i.MX7 over current detection is used unless
>>> disabled in the device tree.
>>>
>>> Signed-off-by: Uwe Kleine-König 
>>> ---
>>>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
>>>  drivers/usb/chipidea/ci_hdrc_imx.c| 16 ---
>>>  drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
>>>  drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
>>>  4 files changed, 43 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
>>> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> index 529e51879fb2..c32f6e983cf6 100644
>>> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
>>> @@ -87,8 +87,9 @@ i.mx specific properties
>>>  - fsl,usbmisc: phandler of non-core register device, with one
>>>argument that indicate usb controller index
>>>  - disable-over-current: disable over current detect
>>> -- over-current-active-high: over current signal polarity is high active,
>>> -  typically over current signal polarity is low active.
>>> +- over-current-active-low: over current signal polarity is active low.
>>> +- over-current-active-high: over current signal polarity is active high.
>>> +  It's recommended to specify the over current polarity.
>>>  - external-vbus-divider: enables off-chip resistor divider for Vbus
>>>  
>>>  Example:
>> thanks for making this configurable. But shouldn't this be a separate
>> patch and reviewed by the devicetree guys?
> I'm not sure about the separate patch part. My impression is it depends
> on the maintainer if a change to the binding (opposed to the
> introduction of a binding) should be separate or not.
>
> But for the review you are right, I added the dt people to Cc for them
> to comment. In v2 Matthew also noted that he would prefer to handle the
> situation when both over-current-active-low and over-current-active-high
> were given differently. I think we don't need that as this is a case of
> "broken dt" and it doesn't matter much what is done then. (With my patch
> we're configuring for active high in that case.) A feedback here would
> be great, too.

AFAIR such invalid settings should be catched with a proper error
message and abort of the probing.

Regards
Stefan

>
> Best regards
> Uwe
>

Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Uwe Kleine-König]

Hello Stefan,

On Tue, Dec 04, 2018 at 09:40:26AM +0100, Stefan Wahren wrote:
> Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
> > The status quo on i.MX6 is that if "over-current-active-high" is
> > specified in the device tree this is configured as expected. If
> > the property is missing polarity isn't changed and so the
> > polarity is kept as setup by the bootloader. Reset default is
> > active high, so active low can only be used with help by the
> > bootloader. On i.MX7 it is similar, but there disabling of
> > over current detection has a similar inconsistency.
> >
> > This patch introduces a new property that allows to explicitly
> > configure for active low over current detection and consistently
> > sets this up. In the absence of an explicit configuration the
> > bit is kept as is. On i.MX7 over current detection is used unless
> > disabled in the device tree.
> >
> > Signed-off-by: Uwe Kleine-König 
> > ---
> >  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
> >  drivers/usb/chipidea/ci_hdrc_imx.c| 16 ---
> >  drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
> >  drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
> >  4 files changed, 43 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..c32f6e983cf6 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -87,8 +87,9 @@ i.mx specific properties
> >  - fsl,usbmisc: phandler of non-core register device, with one
> >argument that indicate usb controller index
> >  - disable-over-current: disable over current detect
> > -- over-current-active-high: over current signal polarity is high active,
> > -  typically over current signal polarity is low active.
> > +- over-current-active-low: over current signal polarity is active low.
> > +- over-current-active-high: over current signal polarity is active high.
> > +  It's recommended to specify the over current polarity.
> >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> >  
> >  Example:
> 
> thanks for making this configurable. But shouldn't this be a separate
> patch and reviewed by the devicetree guys?

I'm not sure about the separate patch part. My impression is it depends
on the maintainer if a change to the binding (opposed to the
introduction of a binding) should be separate or not.

But for the review you are right, I added the dt people to Cc for them
to comment. In v2 Matthew also noted that he would prefer to handle the
situation when both over-current-active-low and over-current-active-high
were given differently. I think we don't need that as this is a case of
"broken dt" and it doesn't matter much what is done then. (With my patch
we're configuring for active high in that case.) A feedback here would
be great, too.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: [PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Stefan Wahren]

Hi Uwe,

Am 04.12.18 um 09:31 schrieb Uwe Kleine-König:
> The status quo on i.MX6 is that if "over-current-active-high" is
> specified in the device tree this is configured as expected. If
> the property is missing polarity isn't changed and so the
> polarity is kept as setup by the bootloader. Reset default is
> active high, so active low can only be used with help by the
> bootloader. On i.MX7 it is similar, but there disabling of
> over current detection has a similar inconsistency.
>
> This patch introduces a new property that allows to explicitly
> configure for active low over current detection and consistently
> sets this up. In the absence of an explicit configuration the
> bit is kept as is. On i.MX7 over current detection is used unless
> disabled in the device tree.
>
> Signed-off-by: Uwe Kleine-König 
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
>  drivers/usb/chipidea/ci_hdrc_imx.c| 16 ---
>  drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
>  drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
>  4 files changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..c32f6e983cf6 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -87,8 +87,9 @@ i.mx specific properties
>  - fsl,usbmisc: phandler of non-core register device, with one
>argument that indicate usb controller index
>  - disable-over-current: disable over current detect
> -- over-current-active-high: over current signal polarity is high active,
> -  typically over current signal polarity is low active.
> +- over-current-active-low: over current signal polarity is active low.
> +- over-current-active-high: over current signal polarity is active high.
> +  It's recommended to specify the over current polarity.
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
>  
>  Example:

thanks for making this configurable. But shouldn't this be a separate
patch and reviewed by the devicetree guys?

[PATCH v3 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified

[Uwe Kleine-König]

The polarity of the over current detection pin isn't configured on i.MX6/7
if it's unspecified in the device tree. So the actual configuration depends
on bootloader behavior which is bad.

So encourage users to fix their device tree by issuing a warning in this
case.

Signed-off-by: Uwe Kleine-König 
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 18b0ca87799c..1e7182272fb5 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -144,6 +144,8 @@ static struct imx_usbmisc_data 
*usbmisc_get_init_data(struct device *dev)
} else if (of_find_property(np, "over-current-active-low", NULL)) {
data->oc_pol_active_low = 1;
data->oc_pol_configured = 1;
+   } else {
+   dev_warn(dev, "No over current polarity defined\n");
}
 
if (of_find_property(np, "external-vbus-divider", NULL))
-- 
2.19.2

[PATCH v3 3/3] usb: chipidea: imx: allow to configure oc polarity on i.MX25

[Uwe Kleine-König]

Up to now the polarity of the over current pin was hard coded to active
high. Use the already defined device tree properties to configure polarity
on i.MX25, too. In difference to i.MX6/7 use active high behavior if the
polarity is unspecified to keep compatibility to existing device trees.

Signed-off-by: Uwe Kleine-König 
---
 drivers/usb/chipidea/usbmisc_imx.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 4a8de1b37f67..47eee5cade84 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -120,6 +120,14 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
*data)
val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
val |= (MX25_EHCI_INTERFACE_DIFF_UNI & 
MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
+
+   /*
+* If the polarity is not configured assume active high for
+* historical reasons.
+*/
+   if (data->oc_pol_configured && data->oc_pol_active_low)
+   val &= ~MX25_OTG_OCPOL_BIT;
+
writel(val, usbmisc->base);
break;
case 1:
@@ -129,6 +137,13 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
*data)
val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
 
+   /*
+* If the polarity is not configured assume active high for
+* historical reasons.
+*/
+   if (data->oc_pol_configured && data->oc_pol_active_low)
+   val &= ~MX25_H1_OCPOL_BIT;
+
writel(val, usbmisc->base);
 
break;
-- 
2.19.2

[PATCH v3 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Uwe Kleine-König]

The status quo on i.MX6 is that if "over-current-active-high" is
specified in the device tree this is configured as expected. If
the property is missing polarity isn't changed and so the
polarity is kept as setup by the bootloader. Reset default is
active high, so active low can only be used with help by the
bootloader. On i.MX7 it is similar, but there disabling of
over current detection has a similar inconsistency.

This patch introduces a new property that allows to explicitly
configure for active low over current detection and consistently
sets this up. In the absence of an explicit configuration the
bit is kept as is. On i.MX7 over current detection is used unless
disabled in the device tree.

Signed-off-by: Uwe Kleine-König 
---
 .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
 drivers/usb/chipidea/ci_hdrc_imx.c| 16 ---
 drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
 drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
index 529e51879fb2..c32f6e983cf6 100644
--- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
+++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
@@ -87,8 +87,9 @@ i.mx specific properties
 - fsl,usbmisc: phandler of non-core register device, with one
   argument that indicate usb controller index
 - disable-over-current: disable over current detect
-- over-current-active-high: over current signal polarity is high active,
-  typically over current signal polarity is low active.
+- over-current-active-low: over current signal polarity is active low.
+- over-current-active-high: over current signal polarity is active high.
+  It's recommended to specify the over current polarity.
 - external-vbus-divider: enables off-chip resistor divider for Vbus
 
 Example:
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 09b37c0d075d..18b0ca87799c 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -132,11 +132,19 @@ static struct imx_usbmisc_data 
*usbmisc_get_init_data(struct device *dev)
 
data->dev = _pdev->dev;
 
-   if (of_find_property(np, "disable-over-current", NULL))
+   /*
+* Check the various over current related properties. If over current
+* detection is disabled we're not interested in the polarity.
+*/
+   if (of_find_property(np, "disable-over-current", NULL)) {
data->disable_oc = 1;
-
-   if (of_find_property(np, "over-current-active-high", NULL))
-   data->oc_polarity = 1;
+   } else if (of_find_property(np, "over-current-active-high", NULL)) {
+   data->oc_pol_active_low = 0;
+   data->oc_pol_configured = 1;
+   } else if (of_find_property(np, "over-current-active-low", NULL)) {
+   data->oc_pol_active_low = 1;
+   data->oc_pol_configured = 1;
+   }
 
if (of_find_property(np, "external-vbus-divider", NULL))
data->evdo = 1;
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h 
b/drivers/usb/chipidea/ci_hdrc_imx.h
index 204275f47573..640721fef0d7 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,13 @@ struct imx_usbmisc_data {
int index;
 
unsigned int disable_oc:1; /* over current detect disabled */
-   unsigned int oc_polarity:1; /* over current polarity if oc enabled */
+
+   /* true if over-current polarity is active low */
+   unsigned int oc_pol_active_low:1;
+
+   /* true if dt specifies polarity */
+   unsigned int oc_pol_configured:1;
+
unsigned int evdo:1; /* set external vbus divider option */
unsigned int ulpi:1; /* connected to an ULPI phy */
 };
diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index def80ff547e4..4a8de1b37f67 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -340,11 +340,17 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data 
*data)
reg = readl(usbmisc->base + data->index * 4);
if (data->disable_oc) {
reg |= MX6_BM_OVER_CUR_DIS;
-   } else if (data->oc_polarity == 1) {
-   /* High active */
-   reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
} else {
-   reg &= ~(MX6_BM_OVER_CUR_DIS);
+   reg &= ~MX6_BM_OVER_CUR_DIS;
+
+   /*
+* If the polarity is not configured keep it as setup by the
+* bootloader.
+*/
+   if (data->oc_pol_configured && data->oc_pol_active_low)
+   reg |= MX6_BM_OVER_CUR_POLARITY;
+   else if (data->oc_pol_configured)
+   reg &= 

[PATCH v3 0/3] usb: chipidea: imx: improve oc handling

[Uwe Kleine-König]

Hello,

compared to v2 (which starts with Message-Id:
<20181202213325.26017-1-u.kleine-koe...@pengutronix.de>) I adapted patch
1 that the oc polarity is only determined if oc isn't disabled. This
results in the warning that is introduced in patch 2 not to be emitted
in this case.

Best regards
Uwe

Re: [PATCH v2 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified

[Uwe Kleine-König]

Hello Peter,

On Tue, Dec 04, 2018 at 06:19:11AM +, PETER CHEN wrote:
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 80b4e4ef9b68..3dcfd0d97f94 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -141,6 +141,8 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > } else if (of_find_property(np, "over-current-active-low", NULL)) {
> > data->oc_pol_active_low = 1;
> > data->oc_pol_configured = 1;
> > +   } else {
> > +   dev_warn(dev, "No over current polarity defined\n");
> > }
> 
> If the platform doesn't support OC, the polarity setting is not needed. You 
> could consider
> getting polarity property only "disable-over-current" is not found at patch 1.

Right, just fixed that up and will send out v3 in a moment.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

RE: [PATCH v2 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified

[PETER CHEN]

 
> Signed-off-by: Uwe Kleine-König 
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 80b4e4ef9b68..3dcfd0d97f94 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -141,6 +141,8 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
>   } else if (of_find_property(np, "over-current-active-low", NULL)) {
>   data->oc_pol_active_low = 1;
>   data->oc_pol_configured = 1;
> + } else {
> + dev_warn(dev, "No over current polarity defined\n");
>   }

If the platform doesn't support OC, the polarity setting is not needed. You 
could consider
getting polarity property only "disable-over-current" is not found at patch 1.

Peter

[PATCH] usb: musb: dsps: fix otg state machine

[Bin Liu]

Due to lack of ID pin interrupt event on AM335x devices, the musb dsps
driver uses polling to detect usb device attach for dual-role port.

But in the case if a micro-A cable adapter is attached without a USB device
attached to the cable, the musb state machine gets stuck in a_wait_vrise
state waiting for the MUSB_CONNECT interrupt which won't happen due to the
usb device is not attached. The state is stuck in a_wait_vrise even after
the micro-A cable is detached, which could cause VBUS retention if then the
dual-role port is attached to a host port.

To fix the problem, make a_wait_vrise as a transient state, then move the
state to either a_wait_bcon for host port or a_idle state for dual-role
port, if no usb device is attached to the port.

Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_dsps.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_dsps.c b/drivers/usb/musb/musb_dsps.c
index 23a0df79ef21..1e6d78b1334e 100644
--- a/drivers/usb/musb/musb_dsps.c
+++ b/drivers/usb/musb/musb_dsps.c
@@ -227,8 +227,13 @@ static int dsps_check_status(struct musb *musb, void 
*unused)
 
switch (musb->xceiv->otg->state) {
case OTG_STATE_A_WAIT_VRISE:
-   dsps_mod_timer_optional(glue);
-   break;
+   if (musb->port_mode == MUSB_HOST) {
+   musb->xceiv->otg->state = OTG_STATE_A_WAIT_BCON;
+   dsps_mod_timer_optional(glue);
+   break;
+   }
+   /* fall through */
+
case OTG_STATE_A_WAIT_BCON:
/* keep VBUS on for host-only mode */
if (musb->port_mode == MUSB_HOST) {
-- 
2.17.1

Re: [PATCH v2 1/3] staging: typec: fusb302: Rename fcs,extcon-name to linux,extcon-name

[Andy Shevchenko]

On Thu, Nov 15, 2018 at 04:53:03PM +0200, Heikki Krogerus wrote:
> On Thu, Nov 15, 2018 at 03:16:19PM +0200, Andy Shevchenko wrote:
> > Since we are going to use the same in Designware USB 3 driver,
> > rename the property to be consistent across the drivers.
> > 
> > No functional change intended.
> > 
> > Signed-off-by: Andy Shevchenko 
> > Cc: Hans de Goede 
> > Cc: Guenter Roeck 
> > Acked-by: Hans de Goede 
> > Acked-by: Guenter Roeck 
> 
> FWIW, the series:
> Reviewed-by: Heikki Krogerus 

Greg, can you pick up this patch (first in the series, since dwc3 went through 
Felipe's tree)?

-- 
With Best Regards,
Andy Shevchenko

USB OF device node "compatible" property

[Thomas Preston]

Hi,
I'm trying to understand how the "compatible" property might be used in USB
device nodes.

The document "Open Firmware Recommended Practice: Universal Serial Bus Version
1" [0] defines a "compatible" property format for USB device nodes. At present,
the kernel does not check this property and it appears to be purely cosmetic.
For example, the following line conforms for the above spec [0], but it is not
referenced or checked at all in the kernel:

arch/arm/boot/dts/bcm283x-rpi-smsc9512.dtsi:10:compatible = 
"usb424,9512";

Instead, each USB device driver defines a `struct usb_device_id` array
containing device vendor IDs (VID) and product IDs (PID) listing all the
compatible devices.

The following patch mentions that "compatible" property is not /currently/ used
for device or interface nodes:

1a7e3948cb9f USB: add device-tree support for interfaces

So, my question is: how /would/ it be used? It seems to me that this is a
redundant definition in the USB OF spec, as USB core already has a mechanism
for driver detection. Also, what happens if the compatible string is wrong?

Any input would be greatly appreciated.

Many thanks,
Thomas

[0] https://www.openbios.org/data/docs/bus.usb.pdf

Re: [PATCH v2 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 04:10:37PM +, Matthew Starr wrote:
> > -Original Message-
> > From: Uwe Kleine-König [mailto:u.kleine-koe...@pengutronix.de]
> > Sent: Sunday, December 02, 2018 3:33 PM
> > 
> > The status quo on i.MX6 is that if "over-current-active-high" is
> > specified in the device tree this is configured as expected. If
> > the property is missing polarity isn't changed and so the
> > polarity is kept as setup by the bootloader. Reset default is
> > active high, so active low can only be used with help by the
> > bootloader. On i.MX7 it is similar, but there disabling of
> > over current detection has a similar inconsistency.
> > 
> > This patch introduces a new property that allows to explicitly
> > configure for active low over current detection and consistently
> > sets this up. In the absence of an explicit configuration the
> > bit is kept as is. On i.MX7 over current detection is used unless
> > disabled in the device tree.
> > 
> > Signed-off-by: Uwe Kleine-König 
> > ---
> >  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
> >  drivers/usb/chipidea/ci_hdrc_imx.c|  9 --
> >  drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
> >  drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
> >  4 files changed, 38 insertions(+), 12 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > index 529e51879fb2..c32f6e983cf6 100644
> > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > @@ -87,8 +87,9 @@ i.mx specific properties
> >  - fsl,usbmisc: phandler of non-core register device, with one
> >argument that indicate usb controller index
> >  - disable-over-current: disable over current detect
> > -- over-current-active-high: over current signal polarity is high active,
> > -  typically over current signal polarity is low active.
> > +- over-current-active-low: over current signal polarity is active low.
> > +- over-current-active-high: over current signal polarity is active high.
> > +  It's recommended to specify the over current polarity.
> >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> > 
> >  Example:
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 09b37c0d075d..80b4e4ef9b68 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -135,8 +135,13 @@ static struct imx_usbmisc_data 
> > *usbmisc_get_init_data(struct device *dev)
> > if (of_find_property(np, "disable-over-current", NULL))
> > data->disable_oc = 1;
> > 
> > -   if (of_find_property(np, "over-current-active-high", NULL))
> > -   data->oc_polarity = 1;
> > +   if (of_find_property(np, "over-current-active-high", NULL)) {
> > +   data->oc_pol_active_low = 0;
> > +   data->oc_pol_configured = 1;
> > +   } else if (of_find_property(np, "over-current-active-low", NULL)) {
> > +   data->oc_pol_active_low = 1;
> > +   data->oc_pol_configured = 1;
> > +   }
> 
> Since both over-current-active-high and over-current-active-low can be
> enabled, the error case of when both are in the device tree is not
> covered here.  An error or warning should be given.  Also in this case
> the safest thing to do would be to leave the OC polarity bit alone and
> leave it the way it was set by the bootloader.

Given that it obviously cannot describe the hardware correctly when both
(over-current-active-high and over-current-active-low) are given it is
IMHO ok to do anything in this case. In my eyes that's similar to the C
language where the compiler is free to do whatever it seems fit when the
programmer does something unspecified.

Added Rob to Cc, maybe he can comment.

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

Re: tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+

[Hans de Goede]

Hi,

On 03-12-18 17:21, Stephan Gerhold wrote:

On Mon, Dec 03, 2018 at 11:50:53AM +0100, Hans de Goede wrote:

Hi,

On 02-12-18 18:08, Stephan Gerhold wrote:

Hi,

thanks for the quick reply! :)

On Sun, Dec 02, 2018 at 03:41:29PM +0100, Hans de Goede wrote:

Hi,

On 01-12-18 13:22, Stephan Gerhold wrote:

Hi,

I have been updating my Intel Baytrail tablet from 4.14 to 4.19 and
noticed that the tusb1210 PHY driver now fails to probe on 4.19.x and
4.20-rc4:

 tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)

The commit that broke this is 211f658b7b40 ("usb: dwc3: pci: Use devm
functions to get the phy GPIOs") because it now claims the phy GPIOs
permanently so the PHY driver (here: tusb1210) cannot get them.

Gadget mode still works with this error and even completely without the
tusb1210 driver, but I have been using the PHY driver additionally
because it has the advantage that it will also power off the PHY during
suspend. (Plus I have some custom hacky code for charger detection in
it at the moment, but I did not have it applied for testing this
problem..)

The comment that still exists above the changed code also mentions why
the GPIO descriptors were put immediately after use:

 /*
  * These GPIOs will turn on the USB2 PHY. Note that we have to
  * put the gpio descriptors again here because the phy driver
  * might want to grab them, too.
  */

Reverting the commit makes everything work again for me. Does this
change fix a problem on another device or was it mostly just cleanup?


It was just a cleanup, I did not realize that another driver would be
claiming the GPIOs and I noticed them not being claimed in
"cat /sys/kernel/debug/gpio" while testing.

But it makes sense that the PHY driver itsef wants to use the GPIOs,
which is likely why the dwc3-pci code was written the way it was
in the first place.

So yes my commit should be reverted. Do you want to submit a revert,
or shall I ?


Hmm, let me try to send it :) I have it mostly prepared locally, just
wanted to ask before I send it.


Ok.


I wanted to suggest that turning on the PHY may not actually be
necessary in dwc3-pci since the tusb1210 driver has the same code in its
probe() method. However, when I tested it it did not work at all because
the tusb1210 driver is not even loaded if the PHY is not turned on
before dwc3 is loaded...
(The ULPI vendor/product ID is wrong with the PHY turned off)


Right, the phy must be turned on, so that the dwc3 driver can create
the struct device representing the phy with the right vendor/product id
in its modalias. This is why the dwc3 code turns it on.


Additional note: The probe error above happens only if:
- The tablet has a TUSB1210/TUSB1211 external PHY (not sure if this is
  the case for all Baytrail tablets..)
- CONFIG_USB_DWC3_ULPI and CONFIG_PHY_TUSB1210 are enabled
- GPIOs are defined in the ACPI DSDT table (Unlike the ACPI GPIOs, the
  fallback GPIO mappings are not inherited to the ULPI device..)


This is why I was not seeing this problem, all my devices lack the GPIO
definition in the ACPI tables. I've put looking into adding the fallback
GPIOs to the tusb1210 driver too on my TODO list, but this is rather low
priority for me.


Somewhat off topic, but:

To be exact my device does not have them in the (original) ACPI table
either. At the moment I override the ACPI DSDT table because there are
some things I haven't found other workarounds for. (It's horrible..)

Some examples:
   - The BCM2E3A Bluetooth device is listed as child under the wrong UART
 controller device, so the kernel never manages to power it up properly
   - The rt5640 (audio codec) device has one of the Bluetooth GPIOs listed
 as interrupt instead of the correct one
   - The GPIOs for the goodix touchscreen were hardcoded in the stock kernel,
 and are therefore entirely missing in the ACPI table

Granted, this device was never intended to be used with a generic
kernel (see my response to your question what hardware I have below),
but nevertheless it is really annoying.


I've one BYT tablet which is/was Android only, which also has some seriously
borked DSTD, but it has an unlocked BIOS and I flipped the OS choice there
(this is a weird thing BYT BIOSes have) to Windows then all of sudden I got
the right DSTD. Asus BIOS-es are typically locked and don't show this option,
but it is probably worth it to take a look.



The BIOS is not locked on my device either. Otherwise I could have
probably never installed something other than Android. It has a "locked"
Android bootloader, but they did not enable UEFI Secure Boot...

I just checked for the OS selection and there is a "Windows 8.X" /
"Android" selection in Advanced -> LPSS & SCC Configuration -> OS
Selection. I'm considering trying it out, but I have to admit that I'm
a bit cautious with changing anything in the BIOS because I'm not sure how
to recover if my tablet does not boot anymore at all after the 

Re: tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+

[Stephan Gerhold]

On Mon, Dec 03, 2018 at 11:50:53AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02-12-18 18:08, Stephan Gerhold wrote:
> > Hi,
> > 
> > thanks for the quick reply! :)
> > 
> > On Sun, Dec 02, 2018 at 03:41:29PM +0100, Hans de Goede wrote:
> > > Hi,
> > > 
> > > On 01-12-18 13:22, Stephan Gerhold wrote:
> > > > Hi,
> > > > 
> > > > I have been updating my Intel Baytrail tablet from 4.14 to 4.19 and
> > > > noticed that the tusb1210 PHY driver now fails to probe on 4.19.x and
> > > > 4.20-rc4:
> > > > 
> > > > tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)
> > > > 
> > > > The commit that broke this is 211f658b7b40 ("usb: dwc3: pci: Use devm
> > > > functions to get the phy GPIOs") because it now claims the phy GPIOs
> > > > permanently so the PHY driver (here: tusb1210) cannot get them.
> > > > 
> > > > Gadget mode still works with this error and even completely without the
> > > > tusb1210 driver, but I have been using the PHY driver additionally
> > > > because it has the advantage that it will also power off the PHY during
> > > > suspend. (Plus I have some custom hacky code for charger detection in
> > > > it at the moment, but I did not have it applied for testing this
> > > > problem..)
> > > > 
> > > > The comment that still exists above the changed code also mentions why
> > > > the GPIO descriptors were put immediately after use:
> > > > 
> > > > /*
> > > >  * These GPIOs will turn on the USB2 PHY. Note that we have to
> > > >  * put the gpio descriptors again here because the phy driver
> > > >  * might want to grab them, too.
> > > >  */
> > > > 
> > > > Reverting the commit makes everything work again for me. Does this
> > > > change fix a problem on another device or was it mostly just cleanup?
> > > 
> > > It was just a cleanup, I did not realize that another driver would be
> > > claiming the GPIOs and I noticed them not being claimed in
> > > "cat /sys/kernel/debug/gpio" while testing.
> > > 
> > > But it makes sense that the PHY driver itsef wants to use the GPIOs,
> > > which is likely why the dwc3-pci code was written the way it was
> > > in the first place.
> > > 
> > > So yes my commit should be reverted. Do you want to submit a revert,
> > > or shall I ?
> > 
> > Hmm, let me try to send it :) I have it mostly prepared locally, just
> > wanted to ask before I send it.
> 
> Ok.
> 
> > > > I wanted to suggest that turning on the PHY may not actually be
> > > > necessary in dwc3-pci since the tusb1210 driver has the same code in its
> > > > probe() method. However, when I tested it it did not work at all because
> > > > the tusb1210 driver is not even loaded if the PHY is not turned on
> > > > before dwc3 is loaded...
> > > > (The ULPI vendor/product ID is wrong with the PHY turned off)
> > > 
> > > Right, the phy must be turned on, so that the dwc3 driver can create
> > > the struct device representing the phy with the right vendor/product id
> > > in its modalias. This is why the dwc3 code turns it on.
> > > 
> > > > Additional note: The probe error above happens only if:
> > > >- The tablet has a TUSB1210/TUSB1211 external PHY (not sure if this 
> > > > is
> > > >  the case for all Baytrail tablets..)
> > > >- CONFIG_USB_DWC3_ULPI and CONFIG_PHY_TUSB1210 are enabled
> > > >- GPIOs are defined in the ACPI DSDT table (Unlike the ACPI GPIOs, 
> > > > the
> > > >  fallback GPIO mappings are not inherited to the ULPI device..)
> > > 
> > > This is why I was not seeing this problem, all my devices lack the GPIO
> > > definition in the ACPI tables. I've put looking into adding the fallback
> > > GPIOs to the tusb1210 driver too on my TODO list, but this is rather low
> > > priority for me.
> > 
> > Somewhat off topic, but:
> > 
> > To be exact my device does not have them in the (original) ACPI table
> > either. At the moment I override the ACPI DSDT table because there are
> > some things I haven't found other workarounds for. (It's horrible..)
> > 
> > Some examples:
> >   - The BCM2E3A Bluetooth device is listed as child under the wrong UART
> > controller device, so the kernel never manages to power it up properly
> >   - The rt5640 (audio codec) device has one of the Bluetooth GPIOs listed
> > as interrupt instead of the correct one
> >   - The GPIOs for the goodix touchscreen were hardcoded in the stock kernel,
> > and are therefore entirely missing in the ACPI table
> > 
> > Granted, this device was never intended to be used with a generic
> > kernel (see my response to your question what hardware I have below),
> > but nevertheless it is really annoying.
> 
> I've one BYT tablet which is/was Android only, which also has some seriously
> borked DSTD, but it has an unlocked BIOS and I flipped the OS choice there
> (this is a weird thing BYT BIOSes have) to Windows then all of sudden I got
> the right DSTD. Asus BIOS-es are typically locked and don't show this option,
> but it is probably worth it to take a look.
> 

RE: [PATCH v2 1/3] usb: chipidea: imx: support configuring for active low oc signal

[Matthew Starr]

> -Original Message-
> From: Uwe Kleine-König [mailto:u.kleine-koe...@pengutronix.de]
> Sent: Sunday, December 02, 2018 3:33 PM
> 
> The status quo on i.MX6 is that if "over-current-active-high" is
> specified in the device tree this is configured as expected. If
> the property is missing polarity isn't changed and so the
> polarity is kept as setup by the bootloader. Reset default is
> active high, so active low can only be used with help by the
> bootloader. On i.MX7 it is similar, but there disabling of
> over current detection has a similar inconsistency.
> 
> This patch introduces a new property that allows to explicitly
> configure for active low over current detection and consistently
> sets this up. In the absence of an explicit configuration the
> bit is kept as is. On i.MX7 over current detection is used unless
> disabled in the device tree.
> 
> Signed-off-by: Uwe Kleine-König 
> ---
>  .../devicetree/bindings/usb/ci-hdrc-usb2.txt  |  5 ++--
>  drivers/usb/chipidea/ci_hdrc_imx.c|  9 --
>  drivers/usb/chipidea/ci_hdrc_imx.h|  8 +-
>  drivers/usb/chipidea/usbmisc_imx.c| 28 ++-
>  4 files changed, 38 insertions(+), 12 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt 
> b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> index 529e51879fb2..c32f6e983cf6 100644
> --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> @@ -87,8 +87,9 @@ i.mx specific properties
>  - fsl,usbmisc: phandler of non-core register device, with one
>argument that indicate usb controller index
>  - disable-over-current: disable over current detect
> -- over-current-active-high: over current signal polarity is high active,
> -  typically over current signal polarity is low active.
> +- over-current-active-low: over current signal polarity is active low.
> +- over-current-active-high: over current signal polarity is active high.
> +  It's recommended to specify the over current polarity.
>  - external-vbus-divider: enables off-chip resistor divider for Vbus
> 
>  Example:
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 09b37c0d075d..80b4e4ef9b68 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -135,8 +135,13 @@ static struct imx_usbmisc_data 
> *usbmisc_get_init_data(struct device *dev)
>   if (of_find_property(np, "disable-over-current", NULL))
>   data->disable_oc = 1;
> 
> - if (of_find_property(np, "over-current-active-high", NULL))
> - data->oc_polarity = 1;
> + if (of_find_property(np, "over-current-active-high", NULL)) {
> + data->oc_pol_active_low = 0;
> + data->oc_pol_configured = 1;
> + } else if (of_find_property(np, "over-current-active-low", NULL)) {
> + data->oc_pol_active_low = 1;
> + data->oc_pol_configured = 1;
> + }

Since both over-current-active-high and over-current-active-low can be enabled, 
the error case of when both are in the device tree is not covered here.  An 
error or warning should be given.  Also in this case the safest thing to do 
would be to leave the OC polarity bit alone and leave it the way it was set by 
the bootloader.

Matt Starr

> 
>   if (of_find_property(np, "external-vbus-divider", NULL))
>   data->evdo = 1;
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h 
> b/drivers/usb/chipidea/ci_hdrc_imx.h
> index 204275f47573..640721fef0d7 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -11,7 +11,13 @@ struct imx_usbmisc_data {
>   int index;
> 
>   unsigned int disable_oc:1; /* over current detect disabled */
> - unsigned int oc_polarity:1; /* over current polarity if oc enabled */
> +
> + /* true if over-current polarity is active low */
> + unsigned int oc_pol_active_low:1;
> +
> + /* true if dt specifies polarity */
> + unsigned int oc_pol_configured:1;
> +
>   unsigned int evdo:1; /* set external vbus divider option */
>   unsigned int ulpi:1; /* connected to an ULPI phy */
>  };
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
> b/drivers/usb/chipidea/usbmisc_imx.c
> index def80ff547e4..4a8de1b37f67 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -340,11 +340,17 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data 
> *data)
>   reg = readl(usbmisc->base + data->index * 4);
>   if (data->disable_oc) {
>   reg |= MX6_BM_OVER_CUR_DIS;
> - } else if (data->oc_polarity == 1) {
> - /* High active */
> - reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
>   } else {
> - reg &= ~(MX6_BM_OVER_CUR_DIS);
> + reg &= ~MX6_BM_OVER_CUR_DIS;
> +
> + /*
> +  * If the 

RE: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Matthew Starr]

> -Original Message-
> From: Uwe Kleine-König [mailto:u.kleine-koe...@pengutronix.de]
> 
> On Mon, Dec 03, 2018 at 09:02:05AM +, PETER CHEN wrote:
> >
> > >
> > > On Mon, Dec 03, 2018 at 03:13:01AM +, PETER CHEN wrote:
> > > > The current OC (Over Current) handling does not consider the default
> > > > and bootloader OC setting well, in this commit, we reset OC setting
> > > > according to dts value:
> > > > - If property "disable-over-current" is set, we will disable OC at
> > > > code; otherwise, we will enable OC.
> > > > - Since most of USB power control chips are low active for OC, we keep
> > > > "over-current-active-high" property unchanging to reduce users effort.
> > > > If this property is set, we set OC polarity as high explicitly;
> > > > otherwise, we set it as low explicitly.
> > > >
> > > > Cc: stable 
> > > > Cc: Peter Chen 
> > > > Cc: Uwe Kleine-König 
> > > > Cc: Matthew Starr 
> > > > Signed-off-by: Peter Chen 
> > > > ---
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++-
> > > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > index 56781c329db0..058468f2de8d 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > > *usbmisc_get_init_data(struct device *dev)
> > > > data->disable_oc = 1;
> > > >
> > > > if (of_find_property(np, "over-current-active-high", NULL))
> > > > -   data->oc_polarity = 1;
> > > > +   data->oc_polarity_high = 1;
> > > >
> > > > if (of_find_property(np, "external-vbus-divider", NULL))
> > > > data->evdo = 1;
> > > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > index fcecab478934..5ea8bb239b38 100644
> > > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > > @@ -11,7 +11,8 @@ struct imx_usbmisc_data {
> > > > int index;
> > > >
> > > > unsigned int disable_oc:1; /* over current detect disabled */
> > > > -   unsigned int oc_polarity:1; /* over current polarity if oc 
> > > > enabled */
> > > > +   /* over current polarity high if oc enabled */
> > > > +   unsigned int oc_polarity_high:1;
> > > > unsigned int evdo:1; /* set external vbus divider option */
> > > > unsigned int ulpi:1; /* connected to an ULPI phy */
> > > > unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > > index 43a15a6e86f5..30d1ada952e1 100644
> > > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct 
> > > > imx_usbmisc_data
> > > *data)
> > > > val = readl(usbmisc->base);
> > > > val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
> > > > val |= (MX25_EHCI_INTERFACE_DIFF_UNI &
> > > MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
> > > > -   val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
> > > > +   val |= MX25_OTG_PM_BIT;
> > > > +   if (data->oc_polarity_high)
> > > > +   /* High active */
> > > > +   val |= MX25_OTG_OCPOL_BIT;
> > > > +   else
> > > > +   val &= ~MX25_OTG_OCPOL_BIT;
> > > > +
> > >
> > > I think we shouldn't change the i.MX25 behaviour in a patch that is 
> > > backported to
> > > stable. Even without this I'd put the change to i.MX25 in a separate 
> > > patch. (As I did
> > > in my series.)
> > >
> > > Also the change is bad, because you're going from hard-coded active high 
> > > to active
> > > low for all users that don't specify "over-current-active-high".
> > > This breaks (I think) most i.MX25 machines.
> > >
> > > Affected in mainline are:
> > >
> > >   imx25-pdk.dts ( + )
> > >   imx25-eukrea-mbimxsd25-baseboard.dts ( + )
> > >
> > > and out of tree probably more. (But maybe these use really active low 
> > > signalling? I
> > > don't know and didn't check.)
> >
> > Are you sure the boards you listed works well for OC now?
> 
> No I'm not. That's why I added the question in parentesis.
> 
> > Both you and Matthew posted patch for OC issue, I think you both
> > probably find the OC function works not well. I have checked the
> > boards (from imx35) at Freescale/NXP internal, all boards are active
> > low for OC.
> 
> I think it is unfortunate to have linux default to active low (in the
> absense of an explicit configuration) while the reset default is active
> high.
> 
> > My intention is to let most of boards work well 

Re: USB driver resets

[Richard van der Hoff]

Does anybody have any suggestions as to how I could set about debugging 
this? I'm seeing the same symptoms on a 4.19 kernel.



On 19/11/2018 15:27, Richard van der Hoff wrote:
I have a USB-3.1 dock, which I use for video (via displayport alt 
mode) and power delivery, as well as keyboard, mouse, etc.


From time to time, all connectivity with the dock drops out for a few 
seconds, before resetting itself. I'm trying to pin down if this is a 
problem with the usb drivers, pci drivers, laptop hardware, or the 
dock. It looks to me from the dmesg output like it could be a problem 
at the PCI layer, but I am only speculating really.


Attached are the dmesg output during the problem, lsusb -v, and lspci 
-v. They are all recorded with a 4.15.0 kernel; I've seen exactly the 
same symptoms with a number of kernels between 4.8 and 4.15.


Advice appreciated!

Re: tusb1210 probe of dwc3.0.auto.ulpi fails with EBUSY on 4.19+

[Hans de Goede]

Hi,

On 02-12-18 18:08, Stephan Gerhold wrote:

Hi,

thanks for the quick reply! :)

On Sun, Dec 02, 2018 at 03:41:29PM +0100, Hans de Goede wrote:

Hi,

On 01-12-18 13:22, Stephan Gerhold wrote:

Hi,

I have been updating my Intel Baytrail tablet from 4.14 to 4.19 and
noticed that the tusb1210 PHY driver now fails to probe on 4.19.x and
4.20-rc4:

tusb1210: probe of dwc3.0.auto.ulpi failed with error -16 (EBUSY)

The commit that broke this is 211f658b7b40 ("usb: dwc3: pci: Use devm
functions to get the phy GPIOs") because it now claims the phy GPIOs
permanently so the PHY driver (here: tusb1210) cannot get them.

Gadget mode still works with this error and even completely without the
tusb1210 driver, but I have been using the PHY driver additionally
because it has the advantage that it will also power off the PHY during
suspend. (Plus I have some custom hacky code for charger detection in
it at the moment, but I did not have it applied for testing this
problem..)

The comment that still exists above the changed code also mentions why
the GPIO descriptors were put immediately after use:

/*
 * These GPIOs will turn on the USB2 PHY. Note that we have to
 * put the gpio descriptors again here because the phy driver
 * might want to grab them, too.
 */

Reverting the commit makes everything work again for me. Does this
change fix a problem on another device or was it mostly just cleanup?


It was just a cleanup, I did not realize that another driver would be
claiming the GPIOs and I noticed them not being claimed in
"cat /sys/kernel/debug/gpio" while testing.

But it makes sense that the PHY driver itsef wants to use the GPIOs,
which is likely why the dwc3-pci code was written the way it was
in the first place.

So yes my commit should be reverted. Do you want to submit a revert,
or shall I ?


Hmm, let me try to send it :) I have it mostly prepared locally, just
wanted to ask before I send it.


Ok.


I wanted to suggest that turning on the PHY may not actually be
necessary in dwc3-pci since the tusb1210 driver has the same code in its
probe() method. However, when I tested it it did not work at all because
the tusb1210 driver is not even loaded if the PHY is not turned on
before dwc3 is loaded...
(The ULPI vendor/product ID is wrong with the PHY turned off)


Right, the phy must be turned on, so that the dwc3 driver can create
the struct device representing the phy with the right vendor/product id
in its modalias. This is why the dwc3 code turns it on.


Additional note: The probe error above happens only if:
   - The tablet has a TUSB1210/TUSB1211 external PHY (not sure if this is
 the case for all Baytrail tablets..)
   - CONFIG_USB_DWC3_ULPI and CONFIG_PHY_TUSB1210 are enabled
   - GPIOs are defined in the ACPI DSDT table (Unlike the ACPI GPIOs, the
 fallback GPIO mappings are not inherited to the ULPI device..)


This is why I was not seeing this problem, all my devices lack the GPIO
definition in the ACPI tables. I've put looking into adding the fallback
GPIOs to the tusb1210 driver too on my TODO list, but this is rather low
priority for me.


Somewhat off topic, but:

To be exact my device does not have them in the (original) ACPI table
either. At the moment I override the ACPI DSDT table because there are
some things I haven't found other workarounds for. (It's horrible..)

Some examples:
  - The BCM2E3A Bluetooth device is listed as child under the wrong UART
controller device, so the kernel never manages to power it up properly
  - The rt5640 (audio codec) device has one of the Bluetooth GPIOs listed
as interrupt instead of the correct one
  - The GPIOs for the goodix touchscreen were hardcoded in the stock kernel,
and are therefore entirely missing in the ACPI table

Granted, this device was never intended to be used with a generic
kernel (see my response to your question what hardware I have below),
but nevertheless it is really annoying.


I've one BYT tablet which is/was Android only, which also has some seriously
borked DSTD, but it has an unlocked BIOS and I flipped the OS choice there
(this is a weird thing BYT BIOSes have) to Windows then all of sudden I got
the right DSTD. Asus BIOS-es are typically locked and don't show this option,
but it is probably worth it to take a look.


So when the fallback mappings
didn't exist yet it was easier for me to add them to the DSDT myself.

As for adding the fallback GPIOs for tusb1210: I was considering this
as well, but the main difficulty for me was that the ULPI device gets
a dynamic device name based on the one from dwc3, e.g. "dwc3.0.auto.ulpi"

Do you think it would be enough to define another static
`gpiod_lookup_table` and replace its `dev_id` with the correct one at
runtime?


Yes, I've not looked into this at al yet, but that sounds like the right
way to do it, esp. since we really want to do this from within the dwc3
driver and not polute the phy driver with this is possible.


It seems 

[PATCH v1] usb: dwc3: trace: Refactor nested switch to make compiler happy

[Andy Shevchenko]

The missed break statement in the outer switch makes the code fall through
always and thus always same value will be printed.

Besides that, compiler warns about missed fall through marker:

drivers/usb/dwc3/./trace.h: In function ‘trace_raw_output_dwc3_log_trb’:
drivers/usb/dwc3/./trace.h:246:4: warning: this statement may fall through 
[-Wimplicit-fallthrough=]
switch (pcm) {
^~

Refactor nested switch statements to work correctly without
compilation warnings.

Fixes: fa8d965d736b ("usb: dwc3: trace: pretty print high-bandwidth transfers 
too")
Cc: Felipe Balbi 
Signed-off-by: Andy Shevchenko 
---
 drivers/usb/dwc3/trace.h | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
index f22714cce070..8e1625a6c19f 100644
--- a/drivers/usb/dwc3/trace.h
+++ b/drivers/usb/dwc3/trace.h
@@ -238,7 +238,7 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
),
TP_printk("%s: trb %p buf %08x%08x size %s%d ctrl %08x 
(%c%c%c%c:%c%c:%s)",
__get_str(name), __entry->trb, __entry->bph, __entry->bpl,
-   ({char *s;
+   ({ char *s = "";
int pcm = ((__entry->size >> 24) & 3) + 1;
switch (__entry->type) {
case USB_ENDPOINT_XFER_INT:
@@ -254,8 +254,6 @@ DECLARE_EVENT_CLASS(dwc3_log_trb,
s = "3x ";
break;
}
-   default:
-   s = "";
} s; }),
DWC3_TRB_SIZE_LENGTH(__entry->size), __entry->ctrl,
__entry->ctrl & DWC3_TRB_CTRL_HWO ? 'H' : 'h',
-- 
2.19.2

Re: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

On Mon, Dec 03, 2018 at 09:02:05AM +, PETER CHEN wrote:
>  
> > 
> > On Mon, Dec 03, 2018 at 03:13:01AM +, PETER CHEN wrote:
> > > The current OC (Over Current) handling does not consider the default
> > > and bootloader OC setting well, in this commit, we reset OC setting
> > > according to dts value:
> > > - If property "disable-over-current" is set, we will disable OC at
> > > code; otherwise, we will enable OC.
> > > - Since most of USB power control chips are low active for OC, we keep
> > > "over-current-active-high" property unchanging to reduce users effort.
> > > If this property is set, we set OC polarity as high explicitly;
> > > otherwise, we set it as low explicitly.
> > >
> > > Cc: stable 
> > > Cc: Peter Chen 
> > > Cc: Uwe Kleine-König 
> > > Cc: Matthew Starr 
> > > Signed-off-by: Peter Chen 
> > > ---
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > > drivers/usb/chipidea/usbmisc_imx.c | 27 ++-
> > >  3 files changed, 25 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 56781c329db0..058468f2de8d 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> > *usbmisc_get_init_data(struct device *dev)
> > >   data->disable_oc = 1;
> > >
> > >   if (of_find_property(np, "over-current-active-high", NULL))
> > > - data->oc_polarity = 1;
> > > + data->oc_polarity_high = 1;
> > >
> > >   if (of_find_property(np, "external-vbus-divider", NULL))
> > >   data->evdo = 1;
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > index fcecab478934..5ea8bb239b38 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > > @@ -11,7 +11,8 @@ struct imx_usbmisc_data {
> > >   int index;
> > >
> > >   unsigned int disable_oc:1; /* over current detect disabled */
> > > - unsigned int oc_polarity:1; /* over current polarity if oc enabled */
> > > + /* over current polarity high if oc enabled */
> > > + unsigned int oc_polarity_high:1;
> > >   unsigned int evdo:1; /* set external vbus divider option */
> > >   unsigned int ulpi:1; /* connected to an ULPI phy */
> > >   unsigned int hsic:1; /* HSIC controlller */ diff --git
> > > a/drivers/usb/chipidea/usbmisc_imx.c
> > > b/drivers/usb/chipidea/usbmisc_imx.c
> > > index 43a15a6e86f5..30d1ada952e1 100644
> > > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct 
> > > imx_usbmisc_data
> > *data)
> > >   val = readl(usbmisc->base);
> > >   val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
> > >   val |= (MX25_EHCI_INTERFACE_DIFF_UNI &
> > MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
> > > - val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
> > > + val |= MX25_OTG_PM_BIT;
> > > + if (data->oc_polarity_high)
> > > + /* High active */
> > > + val |= MX25_OTG_OCPOL_BIT;
> > > + else
> > > + val &= ~MX25_OTG_OCPOL_BIT;
> > > +
> > 
> > I think we shouldn't change the i.MX25 behaviour in a patch that is 
> > backported to
> > stable. Even without this I'd put the change to i.MX25 in a separate patch. 
> > (As I did
> > in my series.)
> > 
> > Also the change is bad, because you're going from hard-coded active high to 
> > active
> > low for all users that don't specify "over-current-active-high".
> > This breaks (I think) most i.MX25 machines.
> > 
> > Affected in mainline are:
> > 
> > imx25-pdk.dts ( + )
> > imx25-eukrea-mbimxsd25-baseboard.dts ( + )
> > 
> > and out of tree probably more. (But maybe these use really active low 
> > signalling? I
> > don't know and didn't check.)
> 
> Are you sure the boards you listed works well for OC now?

No I'm not. That's why I added the question in parentesis.

> Both you and Matthew posted patch for OC issue, I think you both
> probably find the OC function works not well. I have checked the
> boards (from imx35) at Freescale/NXP internal, all boards are active
> low for OC. 

I think it is unfortunate to have linux default to active low (in the
absense of an explicit configuration) while the reset default is active
high.

> My intention is to let most of boards work well without adding dts property 
> since most of
> USB power control chip is active low for OC, the OC function should not test 
> well before.
> 
> Your patches do not break current setting, but need the users to add dts 
> property
> to let OC function work (Since most of chip are active low for OC), almost 
> all i.mx
> boards will see the warning message for without setting OC polarity, and must
> add dts property to fix this warning message. 

You 

RE: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[PETER CHEN]

 
> 
> On Mon, Dec 03, 2018 at 03:13:01AM +, PETER CHEN wrote:
> > The current OC (Over Current) handling does not consider the default
> > and bootloader OC setting well, in this commit, we reset OC setting
> > according to dts value:
> > - If property "disable-over-current" is set, we will disable OC at
> > code; otherwise, we will enable OC.
> > - Since most of USB power control chips are low active for OC, we keep
> > "over-current-active-high" property unchanging to reduce users effort.
> > If this property is set, we set OC polarity as high explicitly;
> > otherwise, we set it as low explicitly.
> >
> > Cc: stable 
> > Cc: Peter Chen 
> > Cc: Uwe Kleine-König 
> > Cc: Matthew Starr 
> > Signed-off-by: Peter Chen 
> > ---
> >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
> > drivers/usb/chipidea/usbmisc_imx.c | 27 ++-
> >  3 files changed, 25 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > index 56781c329db0..058468f2de8d 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> > @@ -140,7 +140,7 @@ static struct imx_usbmisc_data
> *usbmisc_get_init_data(struct device *dev)
> > data->disable_oc = 1;
> >
> > if (of_find_property(np, "over-current-active-high", NULL))
> > -   data->oc_polarity = 1;
> > +   data->oc_polarity_high = 1;
> >
> > if (of_find_property(np, "external-vbus-divider", NULL))
> > data->evdo = 1;
> > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h
> > b/drivers/usb/chipidea/ci_hdrc_imx.h
> > index fcecab478934..5ea8bb239b38 100644
> > --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> > +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> > @@ -11,7 +11,8 @@ struct imx_usbmisc_data {
> > int index;
> >
> > unsigned int disable_oc:1; /* over current detect disabled */
> > -   unsigned int oc_polarity:1; /* over current polarity if oc enabled */
> > +   /* over current polarity high if oc enabled */
> > +   unsigned int oc_polarity_high:1;
> > unsigned int evdo:1; /* set external vbus divider option */
> > unsigned int ulpi:1; /* connected to an ULPI phy */
> > unsigned int hsic:1; /* HSIC controlller */ diff --git
> > a/drivers/usb/chipidea/usbmisc_imx.c
> > b/drivers/usb/chipidea/usbmisc_imx.c
> > index 43a15a6e86f5..30d1ada952e1 100644
> > --- a/drivers/usb/chipidea/usbmisc_imx.c
> > +++ b/drivers/usb/chipidea/usbmisc_imx.c
> > @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data
> *data)
> > val = readl(usbmisc->base);
> > val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
> > val |= (MX25_EHCI_INTERFACE_DIFF_UNI &
> MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
> > -   val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
> > +   val |= MX25_OTG_PM_BIT;
> > +   if (data->oc_polarity_high)
> > +   /* High active */
> > +   val |= MX25_OTG_OCPOL_BIT;
> > +   else
> > +   val &= ~MX25_OTG_OCPOL_BIT;
> > +
> 
> I think we shouldn't change the i.MX25 behaviour in a patch that is 
> backported to
> stable. Even without this I'd put the change to i.MX25 in a separate patch. 
> (As I did
> in my series.)
> 
> Also the change is bad, because you're going from hard-coded active high to 
> active
> low for all users that don't specify "over-current-active-high".
> This breaks (I think) most i.MX25 machines.
> 
> Affected in mainline are:
> 
>   imx25-pdk.dts ( + )
>   imx25-eukrea-mbimxsd25-baseboard.dts ( + )
> 
> and out of tree probably more. (But maybe these use really active low 
> signalling? I
> don't know and didn't check.)

Are you sure the boards you listed works well for OC now? Both you and Matthew 
posted
patch for OC issue, I think you both probably find the OC function works not 
well. I have
checked the boards (from imx35) at Freescale/NXP internal, all boards are 
active low for OC. 

My intention is to let most of boards work well without adding dts property 
since most of
USB power control chip is active low for OC, the OC function should not test 
well before.

Your patches do not break current setting, but need the users to add dts 
property
to let OC function work (Since most of chip are active low for OC), almost all 
i.mx
boards will see the warning message for without setting OC polarity, and must
add dts property to fix this warning message. 

I am OK to accept your patch if both Matthew and Jun are ok with it.

Peter

Re: [PATCH 1/1] usb: chipidea: imx: improve the over current handling

[Uwe Kleine-König]

Hello,

On Mon, Dec 03, 2018 at 03:13:01AM +, PETER CHEN wrote:
> The current OC (Over Current) handling does not consider the default
> and bootloader OC setting well, in this commit, we reset OC setting
> according to dts value:
> - If property "disable-over-current" is set, we will disable OC at
> code; otherwise, we will enable OC.
> - Since most of USB power control chips are low active for OC, we keep
> "over-current-active-high" property unchanging to reduce users effort.
> If this property is set, we set OC polarity as high explicitly;
> otherwise, we set it as low explicitly.
> 
> Cc: stable 
> Cc: Peter Chen 
> Cc: Uwe Kleine-König 
> Cc: Matthew Starr 
> Signed-off-by: Peter Chen 
> ---
>  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
>  drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
>  drivers/usb/chipidea/usbmisc_imx.c | 27 ++-
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
> b/drivers/usb/chipidea/ci_hdrc_imx.c
> index 56781c329db0..058468f2de8d 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.c
> @@ -140,7 +140,7 @@ static struct imx_usbmisc_data 
> *usbmisc_get_init_data(struct device *dev)
>   data->disable_oc = 1;
>  
>   if (of_find_property(np, "over-current-active-high", NULL))
> - data->oc_polarity = 1;
> + data->oc_polarity_high = 1;
>  
>   if (of_find_property(np, "external-vbus-divider", NULL))
>   data->evdo = 1;
> diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h 
> b/drivers/usb/chipidea/ci_hdrc_imx.h
> index fcecab478934..5ea8bb239b38 100644
> --- a/drivers/usb/chipidea/ci_hdrc_imx.h
> +++ b/drivers/usb/chipidea/ci_hdrc_imx.h
> @@ -11,7 +11,8 @@ struct imx_usbmisc_data {
>   int index;
>  
>   unsigned int disable_oc:1; /* over current detect disabled */
> - unsigned int oc_polarity:1; /* over current polarity if oc enabled */
> + /* over current polarity high if oc enabled */
> + unsigned int oc_polarity_high:1;
>   unsigned int evdo:1; /* set external vbus divider option */
>   unsigned int ulpi:1; /* connected to an ULPI phy */
>   unsigned int hsic:1; /* HSIC controlller */
> diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
> b/drivers/usb/chipidea/usbmisc_imx.c
> index 43a15a6e86f5..30d1ada952e1 100644
> --- a/drivers/usb/chipidea/usbmisc_imx.c
> +++ b/drivers/usb/chipidea/usbmisc_imx.c
> @@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
> *data)
>   val = readl(usbmisc->base);
>   val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
>   val |= (MX25_EHCI_INTERFACE_DIFF_UNI & 
> MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
> - val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
> + val |= MX25_OTG_PM_BIT;
> + if (data->oc_polarity_high)
> + /* High active */
> + val |= MX25_OTG_OCPOL_BIT;
> + else
> + val &= ~MX25_OTG_OCPOL_BIT;
> +

I think we shouldn't change the i.MX25 behaviour in a patch that is
backported to stable. Even without this I'd put the change to i.MX25 in
a separate patch. (As I did in my series.)

Also the change is bad, because you're going from hard-coded active high
to active low for all users that don't specify "over-current-active-high".
This breaks (I think) most i.MX25 machines.

Affected in mainline are:

imx25-pdk.dts ( + )
imx25-eukrea-mbimxsd25-baseboard.dts ( + )

and out of tree probably more. (But maybe these use really active low
signalling? I don't know and didn't check.)

Also for i.MX6/7 the overhead isn't that bad to keep compatibility and
give the possibility to explicitly specify the polarity. So I still
prefer my patch set over your approach (which conceptually matches the
patch created by Matthew).

Best regards
Uwe

-- 
Pengutronix e.K.   | Uwe Kleine-König|
Industrial Linux Solutions | http://www.pengutronix.de/  |

[PATCH 1/1] usb: chipidea: imx: improve the over current handling

[PETER CHEN]

The current OC (Over Current) handling does not consider the default
and bootloader OC setting well, in this commit, we reset OC setting
according to dts value:
- If property "disable-over-current" is set, we will disable OC at
code; otherwise, we will enable OC.
- Since most of USB power control chips are low active for OC, we keep
"over-current-active-high" property unchanging to reduce users effort.
If this property is set, we set OC polarity as high explicitly;
otherwise, we set it as low explicitly.

Cc: stable 
Cc: Peter Chen 
Cc: Uwe Kleine-König 
Cc: Matthew Starr 
Signed-off-by: Peter Chen 
---
 drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
 drivers/usb/chipidea/ci_hdrc_imx.h |  3 ++-
 drivers/usb/chipidea/usbmisc_imx.c | 27 ++-
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 56781c329db0..058468f2de8d 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -140,7 +140,7 @@ static struct imx_usbmisc_data 
*usbmisc_get_init_data(struct device *dev)
data->disable_oc = 1;
 
if (of_find_property(np, "over-current-active-high", NULL))
-   data->oc_polarity = 1;
+   data->oc_polarity_high = 1;
 
if (of_find_property(np, "external-vbus-divider", NULL))
data->evdo = 1;
diff --git a/drivers/usb/chipidea/ci_hdrc_imx.h 
b/drivers/usb/chipidea/ci_hdrc_imx.h
index fcecab478934..5ea8bb239b38 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.h
+++ b/drivers/usb/chipidea/ci_hdrc_imx.h
@@ -11,7 +11,8 @@ struct imx_usbmisc_data {
int index;
 
unsigned int disable_oc:1; /* over current detect disabled */
-   unsigned int oc_polarity:1; /* over current polarity if oc enabled */
+   /* over current polarity high if oc enabled */
+   unsigned int oc_polarity_high:1;
unsigned int evdo:1; /* set external vbus divider option */
unsigned int ulpi:1; /* connected to an ULPI phy */
unsigned int hsic:1; /* HSIC controlller */
diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 43a15a6e86f5..30d1ada952e1 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -135,15 +135,26 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
*data)
val = readl(usbmisc->base);
val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
val |= (MX25_EHCI_INTERFACE_DIFF_UNI & 
MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
-   val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
+   val |= MX25_OTG_PM_BIT;
+   if (data->oc_polarity_high)
+   /* High active */
+   val |= MX25_OTG_OCPOL_BIT;
+   else
+   val &= ~MX25_OTG_OCPOL_BIT;
+
writel(val, usbmisc->base);
break;
case 1:
val = readl(usbmisc->base);
val &= ~(MX25_H1_SIC_MASK | MX25_H1_PP_BIT |  
MX25_H1_IPPUE_UP_BIT);
val |= (MX25_EHCI_INTERFACE_SINGLE_UNI & 
MX25_EHCI_INTERFACE_MASK) << MX25_H1_SIC_SHIFT;
-   val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
+   val |= (MX25_H1_PM_BIT | MX25_H1_TLL_BIT |
MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
+   if (data->oc_polarity_high)
+   /* High active */
+   val |= MX25_H1_OCPOL_BIT;
+   else
+   val &= ~MX25_H1_OCPOL_BIT;
 
writel(val, usbmisc->base);
 
@@ -356,12 +367,14 @@ static int usbmisc_imx6q_init(struct imx_usbmisc_data 
*data)
reg = readl(usbmisc->base + data->index * 4);
if (data->disable_oc) {
reg |= MX6_BM_OVER_CUR_DIS;
-   } else if (data->oc_polarity == 1) {
+   } else if (data->oc_polarity_high == 1) {
/* High active */
reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
} else {
-   reg &= ~(MX6_BM_OVER_CUR_DIS);
+   reg &= ~MX6_BM_OVER_CUR_DIS;
+   reg |= MX6_BM_OVER_CUR_POLARITY;
}
+
writel(reg, usbmisc->base + data->index * 4);
 
/* SoC non-burst setting */
@@ -552,10 +565,14 @@ static int usbmisc_imx7d_init(struct imx_usbmisc_data 
*data)
reg = readl(usbmisc->base);
if (data->disable_oc) {
reg |= MX6_BM_OVER_CUR_DIS;
-   } else if (data->oc_polarity == 1) {
+   } else if (data->oc_polarity_high == 1) {
/* High active */
reg &= ~(MX6_BM_OVER_CUR_DIS | MX6_BM_OVER_CUR_POLARITY);
+   } else {
+   reg &= ~MX6_BM_OVER_CUR_DIS;
+   reg |= MX6_BM_OVER_CUR_POLARITY;
}
+
writel(reg, usbmisc->base);
 
reg = readl(usbmisc->base + 

RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low

[PETER CHEN]

 
> 
> > -Original Message-
> > From: Matthew Starr 
> > Sent: 2018年11月30日 23:09
> > To: PETER CHEN ; linux-usb@vger.kernel.org; Jun Li
> > 
> > Subject: RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low
> >
> > > -Original Message-
> > > From: PETER CHEN [mailto:peter.c...@nxp.com]
> > > Sent: Thursday, November 29, 2018 9:30 PM
> > >
> > > >
> > > > The implementation for setting the over current polarity has
> > > > always been the over- current-active-high property.  The problem
> > > > with this is there is no way to enable over current active low
> > > > polarity since the default state of the register bit that controls
> > > > the over current polarity is a 0 which means active high.  This
> > > > value never actually did anything since active high is already the
> > > > default state. Also it is not used in any device tree source in
> > > > the kernel.  The default register bit
> > state was verified by checking the i.MX6Q and i.MX7Dual reference manuals.
> > > >
> > > > The change replaces the over-current-active-high property with the
> > > > over- current- active-low property.  Without this property the
> > > > over current polarity will be active high by default like it always has 
> > > > been.
> > > > With the property the over current polarity will be active low.
> > > >
> > >
> > > Would you please check it?  Why it is opposite for your patch and 
> > > Matthew's?
> >
> > The i.MX6DQ Reference Manual, the i.MX6UL Reference Manual, and the
> > i.MX7D Reference Manual all show that the default reset value of the
> > OVER_CUR_POL bit in the USBNC_USB_OTG_CTRL register is 0.  Here is the
> > reference manual's description for the OVER_CUR_POL bit:
> >
> > OTG Polarity of Overcurrent
> > The polarity of OTG port overcurrent event
> > 1 Low active (low on this signal represents an overcurrent condition)
> > 0 High active (high on this signal represents an overcurrent
> > condition)
> >
> > I verified this on a custom made product using an i.MX6UL.  The
> > overcurrent detection on this product is active low triggered, and
> > only through the modification of the driver enabling active low
> > triggering of the overcurrent functionality was I able to see the USB 
> > detect the
> overcurrent and reset itself.
> >
> > I think this has been missed up till now because the bit next to the
> > OVER_CUR_POL bit is the PWR_POL which has the exact inverse logic with
> > a value of 1 meaning high enable and 0 meaning low enable.  Also I
> > rarely have seen the USB interface go into an overcurrent situation so
> > maybe no one ever caught this, but while forcing it during hardware testing 
> > the
> issue was discovered.
> 
> The intention of use active-high for OC was to make the most of users don't 
> need
> to add the property, that is, by default, OC is active low, this is also 
> matching the
> reality in most cases I think. Meanwhile to avoid break those existing HW 
> before
> this property was introduced, the usbmisc driver wasn't updated to be OC 
> active low,
> but hope the later SoCs correct this by a new init function.
> 
> Now we have the problem of the default value in opposite state(OC is enabled 
> and
> polarity is active high), seems there is no easy way to resolve this, 
> considering the
> existing property hasn't been used, change it to be "active low"
> like this may be the easiest way to resolve it.
> 
> For those platforms without any OC properties added, I don’t think all of 
> them were
> using active high, may be the iomux wasn't configured to be OC input, so the
> default state(low) couldn’t generate false alarm.
> 

Hi Matt, Uwe, Jun,

Thanks for commenting. I suggest resetting all OC settings according to dts 
value and
keep current property "over-current-active-high" since most of users doesn't 
need to set
property. See my patch for detail.

Peter

> Li Jun
> 
> >
> > Matt Starr
> >
> > >
> > > > Signed-off-by: Matthew Starr 
> > > > ---
> > > >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt |  4 ++--
> > > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > > >  drivers/usb/chipidea/usbmisc_imx.c | 10 ++
> > > >  3 files changed, 9 insertions(+), 7 deletions(-)
> > > >
> > > > diff --git
> > > > a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > index 529e51879fb2..3dee58037839 100644
> > > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > > @@ -87,8 +87,8 @@ i.mx specific properties
> > > >  - fsl,usbmisc: phandler of non-core register device, with one
> > > >argument that indicate usb controller index
> > > >  - disable-over-current: disable over current detect
> > > > -- over-current-active-high: over current signal polarity is high
> > > > active,
> > > > -  typically over current signal polarity is low active.
> > > > 

RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low

[Jun Li]

Hi Peter,

> -Original Message-
> From: Matthew Starr 
> Sent: 2018年11月30日 23:09
> To: PETER CHEN ; linux-usb@vger.kernel.org; Jun Li
> 
> Subject: RE: [PATCH] usb: chipidea: imx: Allow OC polarity active low
> 
> > -Original Message-
> > From: PETER CHEN [mailto:peter.c...@nxp.com]
> > Sent: Thursday, November 29, 2018 9:30 PM
> >
> > >
> > > The implementation for setting the over current polarity has always
> > > been the over- current-active-high property.  The problem with this
> > > is there is no way to enable over current active low polarity since
> > > the default state of the register bit that controls the over current
> > > polarity is a 0 which means active high.  This value never actually
> > > did anything since active high is already the default state. Also it
> > > is not used in any device tree source in the kernel.  The default 
> > > register bit
> state was verified by checking the i.MX6Q and i.MX7Dual reference manuals.
> > >
> > > The change replaces the over-current-active-high property with the
> > > over- current- active-low property.  Without this property the over
> > > current polarity will be active high by default like it always has been.
> > > With the property the over current polarity will be active low.
> > >
> >
> > Would you please check it?  Why it is opposite for your patch and Matthew's?
> 
> The i.MX6DQ Reference Manual, the i.MX6UL Reference Manual, and the i.MX7D
> Reference Manual all show that the default reset value of the OVER_CUR_POL bit
> in the USBNC_USB_OTG_CTRL register is 0.  Here is the reference manual's
> description for the OVER_CUR_POL bit:
> 
> OTG Polarity of Overcurrent
> The polarity of OTG port overcurrent event
> 1 Low active (low on this signal represents an overcurrent condition)
> 0 High active (high on this signal represents an overcurrent condition)
> 
> I verified this on a custom made product using an i.MX6UL.  The overcurrent
> detection on this product is active low triggered, and only through the 
> modification
> of the driver enabling active low triggering of the overcurrent functionality 
> was I
> able to see the USB detect the overcurrent and reset itself.
> 
> I think this has been missed up till now because the bit next to the
> OVER_CUR_POL bit is the PWR_POL which has the exact inverse logic with a
> value of 1 meaning high enable and 0 meaning low enable.  Also I rarely have
> seen the USB interface go into an overcurrent situation so maybe no one ever
> caught this, but while forcing it during hardware testing the issue was 
> discovered.

The intention of use active-high for OC was to make the most of users don't need
to add the property, that is, by default, OC is active low, this is also 
matching the
reality in most cases I think. Meanwhile to avoid break those existing HW before
this property was introduced, the usbmisc driver wasn't updated to be OC active
low, but hope the later SoCs correct this by a new init function.

Now we have the problem of the default value in opposite state(OC is enabled
and polarity is active high), seems there is no easy way to resolve this, 
considering the existing property hasn't been used, change it to be "active low"
like this may be the easiest way to resolve it.

For those platforms without any OC properties added, I don’t think all of them
were using active high, may be the iomux wasn't configured to be OC input,
so the default state(low) couldn’t generate false alarm.

Li Jun

> 
> Matt Starr
> 
> >
> > > Signed-off-by: Matthew Starr 
> > > ---
> > >  Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt |  4 ++--
> > >  drivers/usb/chipidea/ci_hdrc_imx.c |  2 +-
> > >  drivers/usb/chipidea/usbmisc_imx.c | 10 ++
> > >  3 files changed, 9 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > index 529e51879fb2..3dee58037839 100644
> > > --- a/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > +++ b/Documentation/devicetree/bindings/usb/ci-hdrc-usb2.txt
> > > @@ -87,8 +87,8 @@ i.mx specific properties
> > >  - fsl,usbmisc: phandler of non-core register device, with one
> > >argument that indicate usb controller index
> > >  - disable-over-current: disable over current detect
> > > -- over-current-active-high: over current signal polarity is high
> > > active,
> > > -  typically over current signal polarity is low active.
> > > +- over-current-active-low: over current signal polarity is low
> > > +active,
> > > +  the default signal polarity is high active.
> > >  - external-vbus-divider: enables off-chip resistor divider for Vbus
> > >
> > >  Example:
> > > diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > b/drivers/usb/chipidea/ci_hdrc_imx.c
> > > index 09b37c0d075d..f7069544fb42 100644
> > > --- a/drivers/usb/chipidea/ci_hdrc_imx.c
> > > +++ 

[PATCH] USB: qcaux: Add Motorola modem UARTs

[Tony Lindgren]

On Motorola Mapphone devices such as Droid 4 there are five USB ports
that do not use the same layout as Gobi 1K/2K/etc devices listed in
qcserial.c. So we should use qcaux.c or option.c as noted by
Dan Williams .

The ff/ff/ff interfaces seem to always be UARTs on Motorola devices.
And we should not add interfaces with 0x0a class (CDC Data) as they
are part of a multi-interface function like for example interface
0x22b8:0x4281 as noted by Bjørn Mork .

The ttyUSB ports on Droid 4 seem to be:

ttyUSB0 DIAG, CQDM-capable
ttyUSB1 MUX or NMEA, no response
ttyUSB2 MUX or NMEA, no response
ttyUSB3 TCMD
ttyUSB4 AT-capable

To enable the MUX or NMEA ports, it seems that something needs
to be done additionally to enable them, maybe via the DIAG or
TCMD port. Who knows, maybe it's just some NVRAM setting.

Cc: Bjørn Mork 
Cc: Johan Hovold 
Cc: Dan Williams 
Cc: Marcel Partap 
Cc: Merlijn Wajer 
Cc: Pavel Machek 
Cc: Sebastian Reichel 
Signed-off-by: Tony Lingren 
---
 drivers/usb/serial/qcaux.c | 9 +
 1 file changed, 9 insertions(+)

diff --git a/drivers/usb/serial/qcaux.c b/drivers/usb/serial/qcaux.c
--- a/drivers/usb/serial/qcaux.c
+++ b/drivers/usb/serial/qcaux.c
@@ -42,6 +42,12 @@
 #define LG_VENDOR_ID   0x1004
 #define LG_PRODUCT_VX4400_6000 0x6000 /* VX4400/VX6000/Rumor */
 
+/* Motorola devices */
+#define MOTOROLA_VENDOR_ID 0x22b8
+#define MOTOROLA_PRODUCT_MDM6600   0x2a70 /* MDM6600 on mapphone */
+#define MOTOROLA_PRODUCT_MDM9600   0x2e0a /* MDM9600 on mapphone */
+#define MOTOROLA_PRODUCT_MDM_FLASH 0x900e /* MDM UART flash mode */
+
 /* Sanyo devices */
 #define SANYO_VENDOR_ID0x0474
 #define SANYO_PRODUCT_KATANA_LX0x0754 /* SCP-3800 
(Katana LX) */
@@ -60,6 +66,9 @@ static const struct usb_device_id id_table[] = {
{ USB_DEVICE_AND_INTERFACE_INFO(CMOTECH_VENDOR_ID, 
CMOTECH_PRODUCT_CDU550, 0xff, 0xff, 0x00) },
{ USB_DEVICE_AND_INTERFACE_INFO(CMOTECH_VENDOR_ID, 
CMOTECH_PRODUCT_CDX650, 0xff, 0xff, 0x00) },
{ USB_DEVICE_AND_INTERFACE_INFO(LG_VENDOR_ID, LG_PRODUCT_VX4400_6000, 
0xff, 0xff, 0x00) },
+   { USB_DEVICE_AND_INTERFACE_INFO(MOTOROLA_VENDOR_ID, 
MOTOROLA_PRODUCT_MDM6600, 0xff, 0xff, 0xff) },
+   { USB_DEVICE_AND_INTERFACE_INFO(MOTOROLA_VENDOR_ID, 
MOTOROLA_PRODUCT_MDM9600, 0xff, 0xff, 0xff) },
+   { USB_DEVICE_AND_INTERFACE_INFO(MOTOROLA_VENDOR_ID, 
MOTOROLA_PRODUCT_MDM_FLASH, 0xff, 0xff, 0xff) },
{ USB_DEVICE_AND_INTERFACE_INFO(SANYO_VENDOR_ID, 
SANYO_PRODUCT_KATANA_LX, 0xff, 0xff, 0x00) },
{ USB_DEVICE_AND_INTERFACE_INFO(SAMSUNG_VENDOR_ID, 
SAMSUNG_PRODUCT_U520, 0xff, 0x00, 0x00) },
{ USB_VENDOR_AND_INTERFACE_INFO(UTSTARCOM_VENDOR_ID, 0xff, 0xfd, 0xff) 
},  /* NMEA */
-- 
2.19.2

[PATCH v2 3/3] usb: chipidea: imx: allow to configure oc polarity on i.MX25

[Uwe Kleine-König]

Up to now the polarity of the over current pin was hard coded to active
high. Use the already defined device tree properties to configure polarity
on i.MX25, too. In difference to i.MX6/7 use active high behavior if the
polarity is unspecified to keep compatibility to existing device trees.

Signed-off-by: Uwe Kleine-König 
---
 drivers/usb/chipidea/usbmisc_imx.c | 15 +++
 1 file changed, 15 insertions(+)

diff --git a/drivers/usb/chipidea/usbmisc_imx.c 
b/drivers/usb/chipidea/usbmisc_imx.c
index 4a8de1b37f67..47eee5cade84 100644
--- a/drivers/usb/chipidea/usbmisc_imx.c
+++ b/drivers/usb/chipidea/usbmisc_imx.c
@@ -120,6 +120,14 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
*data)
val &= ~(MX25_OTG_SIC_MASK | MX25_OTG_PP_BIT);
val |= (MX25_EHCI_INTERFACE_DIFF_UNI & 
MX25_EHCI_INTERFACE_MASK) << MX25_OTG_SIC_SHIFT;
val |= (MX25_OTG_PM_BIT | MX25_OTG_OCPOL_BIT);
+
+   /*
+* If the polarity is not configured assume active high for
+* historical reasons.
+*/
+   if (data->oc_pol_configured && data->oc_pol_active_low)
+   val &= ~MX25_OTG_OCPOL_BIT;
+
writel(val, usbmisc->base);
break;
case 1:
@@ -129,6 +137,13 @@ static int usbmisc_imx25_init(struct imx_usbmisc_data 
*data)
val |= (MX25_H1_PM_BIT | MX25_H1_OCPOL_BIT | MX25_H1_TLL_BIT |
MX25_H1_USBTE_BIT | MX25_H1_IPPUE_DOWN_BIT);
 
+   /*
+* If the polarity is not configured assume active high for
+* historical reasons.
+*/
+   if (data->oc_pol_configured && data->oc_pol_active_low)
+   val &= ~MX25_H1_OCPOL_BIT;
+
writel(val, usbmisc->base);
 
break;
-- 
2.19.2

[PATCH v2 2/3] usb: chipidea: imx: Warn if oc polarity isn't specified

[Uwe Kleine-König]

The polarity of the over current detection pin isn't configured on i.MX6/7
if it's unspecified in the device tree. So the actual configuration depends
on bootloader behavior which is bad.

So encourage users to fix their device tree by issuing a warning in this
case.

Signed-off-by: Uwe Kleine-König 
---
 drivers/usb/chipidea/ci_hdrc_imx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/usb/chipidea/ci_hdrc_imx.c 
b/drivers/usb/chipidea/ci_hdrc_imx.c
index 80b4e4ef9b68..3dcfd0d97f94 100644
--- a/drivers/usb/chipidea/ci_hdrc_imx.c
+++ b/drivers/usb/chipidea/ci_hdrc_imx.c
@@ -141,6 +141,8 @@ static struct imx_usbmisc_data 
*usbmisc_get_init_data(struct device *dev)
} else if (of_find_property(np, "over-current-active-low", NULL)) {
data->oc_pol_active_low = 1;
data->oc_pol_configured = 1;
+   } else {
+   dev_warn(dev, "No over current polarity defined\n");
}
 
if (of_find_property(np, "external-vbus-divider", NULL))
-- 
2.19.2