[PATCH 1/1] usb: dwc3: avoid setting ClearPendIn for Intel Cherry Trail devices

[Lu Baolu]

Commit 50c763f8c1bac ("usb: dwc3: Set the ClearPendIN bit on Clear
Stall EP command") causes Clear Stall EP command failure on Intel
Cherry Trail devices. This patch add a quirk to avoid setting this
bit for those Intel devices.

Cc: sta...@vger.kernel.org # 4.7+
Signed-off-by: Lu Baolu 
---
 drivers/usb/dwc3/core.c |  3 +++
 drivers/usb/dwc3/core.h |  3 +++
 drivers/usb/dwc3/dwc3-pci.c | 11 +++
 drivers/usb/dwc3/gadget.c   |  3 ++-
 4 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index 35d0924..b858c43 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -934,6 +934,9 @@ static int dwc3_probe(struct platform_device *pdev)
device_property_read_u32(dev, "snps,quirk-frame-length-adjustment",
 >fladj);
 
+   dwc->avoid_clearpendin_quirk = device_property_read_bool(dev,
+   "snps,avoid_clearpendin_quirk");
+
dwc->lpm_nyet_threshold = lpm_nyet_threshold;
dwc->tx_de_emphasis = tx_de_emphasis;
 
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 45d6de5..eddfa75 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -805,6 +805,8 @@ struct dwc3_scratchpad_array {
  * 1   - -3.5dB de-emphasis
  * 2   - No de-emphasis
  * 3   - Reserved
+ * @avoid_clearpendin_quirk: set if we avoid setting ClearPendIn bit
+ *   on Clear Stall EP command
  */
 struct dwc3 {
struct usb_ctrlrequest  *ctrl_req;
@@ -945,6 +947,7 @@ struct dwc3 {
 
unsignedtx_de_emphasis_quirk:1;
unsignedtx_de_emphasis:2;
+   unsignedavoid_clearpendin_quirk:1;
 };
 
 /* -- 
*/
diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 0a32430..e89893e 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -117,6 +117,17 @@ static int dwc3_pci_quirks(struct pci_dev *pdev, struct 
platform_device *dwc3)
usleep_range(1, 11000);
}
}
+
+   if (pdev->device == PCI_DEVICE_ID_INTEL_BSW) {
+   struct property_entry bsw_properties[] = {
+   PROPERTY_ENTRY_BOOL(
+   "snps,avoid_clearpendin_quirk"),
+   { }
+   };
+
+   return platform_device_add_properties(dwc3,
+ bsw_properties);
+   }
}
 
if (pdev->vendor == PCI_VENDOR_ID_SYNOPSYS &&
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 7a8d3d8..5a6897e 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -348,7 +348,8 @@ static int dwc3_send_clear_stall_ep_cmd(struct dwc3_ep *dep)
 * IN transfers due to a mishandled error condition. Synopsys
 * STAR 9000614252.
 */
-   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A))
+   if (dep->direction && (dwc->revision >= DWC3_REVISION_260A) &&
+   !dwc->avoid_clearpendin_quirk)
cmd |= DWC3_DEPCMD_CLEARPENDIN;
 
memset(, 0, sizeof(params));
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] usb: dwc3: Fix dr_mode validation

[John Youn]

From: Thinh Nguyen 

This patch follows the similar fix in dwc2. See
commit 5268ed9d2e3b ("usb: dwc2: Fix dr_mode validation")

Currently, the dr_mode is only checked against the module configuration.
It also needs to be checked against the hardware capablities.

The driver now checks if both the module configuration and hardware are
capable of the dr_mode value. If not, then it will issue a warning and
fall back to a supported value. If it is unable to fall back to a
suitable value, then the probe will fail.

Behavior summary:

  module  :  actual
 HW   config  dr_mode :  dr_mode
-
 host  host   any :  host
 host  devany :  INVALID
 host  otgany :  host

 dev   host   any :  INVALID
 dev   devany :  dev
 dev   otgany :  dev

 otg   host   any :  host
 otg   devany :  dev
 otg   otgany :  dr_mode

Signed-off-by: Thinh Nguyen 
Signed-off-by: John Youn 
---

v2:
* Use the cached hwparams
* Use a switch statement

 drivers/usb/dwc3/core.c | 65 -
 drivers/usb/dwc3/core.h |  5 +++-
 2 files changed, 58 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
index d6d3fa0..dcacc70 100644
--- a/drivers/usb/dwc3/core.c
+++ b/drivers/usb/dwc3/core.c
@@ -49,6 +49,57 @@
 
 #define DWC3_DEFAULT_AUTOSUSPEND_DELAY 5000 /* ms */
 
+/**
+ * dwc3_get_dr_mode - Validates and sets dr_mode
+ * @dwc: pointer to our context structure
+ */
+static int dwc3_get_dr_mode(struct dwc3 *dwc)
+{
+   enum usb_dr_mode mode;
+   struct device *dev = dwc->dev;
+   unsigned int hw_mode;
+
+   if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
+   dwc->dr_mode = USB_DR_MODE_OTG;
+
+   mode = dwc->dr_mode;
+   hw_mode = DWC3_GHWPARAMS0_MODE(dwc->hwparams.hwparams0);
+
+   switch (hw_mode) {
+   case DWC3_GHWPARAMS0_MODE_GADGET:
+   if (IS_ENABLED(CONFIG_USB_DWC3_HOST)) {
+   dev_err(dev,
+   "Controller does not support host mode.\n");
+   return -EINVAL;
+   }
+   mode = USB_DR_MODE_PERIPHERAL;
+   break;
+   case DWC3_GHWPARAMS0_MODE_HOST:
+   if (IS_ENABLED(CONFIG_USB_DWC3_GADGET)) {
+   dev_err(dev,
+   "Controller does not support device mode.\n");
+   return -EINVAL;
+   }
+   mode = USB_DR_MODE_HOST;
+   break;
+   default:
+   if (IS_ENABLED(CONFIG_USB_DWC3_HOST))
+   mode = USB_DR_MODE_HOST;
+   else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET))
+   mode = USB_DR_MODE_PERIPHERAL;
+   }
+
+   if (mode != dwc->dr_mode) {
+   dev_warn(dev,
+"Configuration mismatch. dr_mode forced to %s\n",
+mode == USB_DR_MODE_HOST ? "host" : "gadget");
+
+   dwc->dr_mode = mode;
+   }
+
+   return 0;
+}
+
 void dwc3_set_mode(struct dwc3 *dwc, u32 mode)
 {
u32 reg;
@@ -1023,17 +1074,9 @@ static int dwc3_probe(struct platform_device *pdev)
goto err2;
}
 
-   if (IS_ENABLED(CONFIG_USB_DWC3_HOST) &&
-   (dwc->dr_mode == USB_DR_MODE_OTG ||
-   dwc->dr_mode == USB_DR_MODE_UNKNOWN))
-   dwc->dr_mode = USB_DR_MODE_HOST;
-   else if (IS_ENABLED(CONFIG_USB_DWC3_GADGET) &&
-   (dwc->dr_mode == USB_DR_MODE_OTG ||
-   dwc->dr_mode == USB_DR_MODE_UNKNOWN))
-   dwc->dr_mode = USB_DR_MODE_PERIPHERAL;
-
-   if (dwc->dr_mode == USB_DR_MODE_UNKNOWN)
-   dwc->dr_mode = USB_DR_MODE_OTG;
+   ret = dwc3_get_dr_mode(dwc);
+   if (ret)
+   goto err3;
 
ret = dwc3_alloc_scratch_buffers(dwc);
if (ret)
diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index b2317e7..6b60e42 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -245,7 +245,10 @@
 #define DWC3_GEVNTSIZ_SIZE(n)  ((n) & 0x)
 
 /* Global HWPARAMS0 Register */
-#define DWC3_GHWPARAMS0_USB3_MODE(n)   ((n) & 0x3)
+#define DWC3_GHWPARAMS0_MODE(n)((n) & 0x3)
+#define DWC3_GHWPARAMS0_MODE_GADGET0
+#define DWC3_GHWPARAMS0_MODE_HOST  1
+#define DWC3_GHWPARAMS0_MODE_DRD   2
 #define DWC3_GHWPARAMS0_MBUS_TYPE(n)   (((n) >> 3) & 0x7)
 #define DWC3_GHWPARAMS0_SBUS_TYPE(n)   (((n) >> 6) & 0x3)
 #define DWC3_GHWPARAMS0_MDWIDTH(n) (((n) >> 8) & 0xff)
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Joe Perches]

On Wed, 2016-09-07 at 02:00 +, ronnie.ku...@microchip.com wrote:
> Microchip's internal convention is for register (offset) definitions
> to be capitalized (i.e.: MY_REGISTER). Our convention for bits
> (position) definitions within a register is to carry as a prefix the
> name of the register and suffix it with the bit name and adding a
> trailing underscore (i.e. MY_REGISTER_MY_BIT_). The trailing
> underscore is what easily lets us distinguish a bit from a register
> definition when reading code. We have been using this convention for
> many years and has worked very well for us across all projects (by now
> hundreds).

I think it's kind of an ugly convention, but  no
skin off my nose really.

> > Is there anything other than a one-time cost
> > to apply these?  Is the same code used for
> > other platforms?
> 
> Yes, a single header file with the definition of registers and bits is
> shared (either as a standalone file or with its contents pasted into a
> native environment "carrier" header file) across all drivers (and
> other non driver software projects as well) for the same device. So a
> change like this indeed has a high cost for Microchip and we'd rather
> not do this unless it is an absolutely mandated requirement. 

No worries, if you don't like it, don't apply it.

Send a NAK too so David Miller doesn't apply it either.



--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[John Youn]

On 9/6/2016 6:19 PM, Ayaka wrote:
> 
> 
> 從我的 iPad 傳送
> 
>> John Youn  於 2016年9月7日 上午2:54 寫道：
>>
>>> On 9/5/2016 10:15 PM, Kishon Vijay Abraham I wrote:
>>> Hi,
>>>
 On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
 On the rk3288 USB host-only port (the one that's not the OTG-enabled
 port) the PHY can get into a bad state when a wakeup is asserted (not
 just a wakeup from full system suspend but also a wakeup from
 autosuspend).

 We can get the PHY out of its bad state by asserting its "port reset",
 but unfortunately that seems to assert a reset onto the USB bus so it
 could confuse things if we don't actually deenumerate / reenumerate the
 device.

 We can also get the PHY out of its bad state by fully resetting it using
 the reset from the CRU (clock reset unit) in chip, which does a more full
 reset.  The CRU-based reset appears to actually cause devices on the bus
 to be removed and reinserted, which fixes the problem (albeit in a hacky
 way).

 It's unfortunate that we need to do a full re-enumeration of devices at
 wakeup time, but this is better than alternative of letting the bus get
 wedged.

 Signed-off-by: Randy Li 
 ---
 drivers/usb/dwc2/core_intr.c | 12 
 1 file changed, 12 insertions(+)

 diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
 index d85c5c9..08485b7 100644
 --- a/drivers/usb/dwc2/core_intr.c
 +++ b/drivers/usb/dwc2/core_intr.c
 @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct 
 dwc2_hsotg *hsotg)
 static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
 {
int ret;
 +struct device_node *np = hsotg->dev->of_node;

/* Clear interrupt */
dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
 @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct 
 dwc2_hsotg *hsotg)
/* Restart the Phy Clock */
pcgcctl &= ~PCGCTL_STOPPCLK;
dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
 +
 +/*
 + * It is a quirk in Rockchip RK3288, causing by
 + * a hardware bug. This will propagate out and
 + * eventually we'll re-enumerate the device. 
 + * Not great but the best we can do 
 + */
 +if (of_device_is_compatible(np, "rockchip,rk3288-usb")
 +&& (NULL != hsotg->phy->ops->reset))
 +hsotg->phy->ops->reset(hsotg->phy);
>>>
>>> never call the phy ops directly from the controller driver. It has to be
>>> protected as well.
>>
>> It looks like we should be calling an API function instead, correct?
>>
> Could you give me a example for the wrapper of phy ops?

Check these files:
* include/linux/phy/phy.h
* drivers/phy/phy-core.c

Search for "phy_set_mode".

I'm thinking there should be a "phy_reset" API function which calls
the phy->ops->reset in a similar manner. With the same runtime checks
and stub function if CONFIG_GENERIC_PHY is not enabled.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Ronnie.Kunin]

Microchip's internal convention is for register (offset) definitions to be 
capitalized (i.e.: MY_REGISTER). Our convention for bits (position) definitions 
within a register is to carry as a prefix the name of the register and suffix 
it with the bit name and adding a trailing underscore (i.e. 
MY_REGISTER_MY_BIT_). The trailing underscore is what easily lets us 
distinguish a bit from a register definition when reading code. We have been 
using this convention for many years and has worked very well for us across all 
projects (by now hundreds).

>Is there anything other than a one-time cost
>to apply these?  Is the same code used for
>other platforms?
Yes, a single header file with the definition of registers and bits is shared 
(either as a standalone file or with its contents pasted into a native 
environment "carrier" header file) across all drivers (and other non driver 
software projects as well) for the same device. So a change like this indeed 
has a high cost for Microchip and we'd rather not do this unless it is an 
absolutely mandated requirement. 

Thanks,
Ronnie

From: Joe Perches [j...@perches.com]
Sent: Tuesday, September 06, 2016 9:18 PM
To: Woojung Huh - C21699; net...@vger.kernel.org; linux-usb@vger.kernel.org
Cc: f.faine...@gmail.com; UNGLinuxDriver; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

On Tue, 2016-09-06 at 23:19 +, woojung@microchip.com wrote:
> > Joe Perches (2):
> >   lan78xx: Remove locally defined trailing underscores from defines and uses
> >   microchipphy.h and uses: Remove trailing underscores from defines and
> > uses
> >
> >  drivers/net/phy/microchip.c  |4 +-
> >  drivers/net/usb/lan78xx.c|  368 +++
> >  drivers/net/usb/lan78xx.h| 1068 +-
> > 
> >  include/linux/microchipphy.h |   72 +--
> >  4 files changed, 756 insertions(+), 756 deletions(-)
>
>
> Because there is no specific rule how to name defines, I'm not sure it is 
> worth to change 1000+ lines.
> It may be better to set guideline for new submissions.
>
> Welcome any comments.

Generally, more conforming to norms is better.
These FOO_ uses are non-conforming.

Is there anything other than a one-time cost
to apply these?  Is the same code used for
other platforms?--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Joe Perches]

On Tue, 2016-09-06 at 23:19 +, woojung@microchip.com wrote:
> > Joe Perches (2):
> >   lan78xx: Remove locally defined trailing underscores from defines and uses
> >   microchipphy.h and uses: Remove trailing underscores from defines and
> > uses
> > 
> >  drivers/net/phy/microchip.c  |4 +-
> >  drivers/net/usb/lan78xx.c|  368 +++
> >  drivers/net/usb/lan78xx.h| 1068 +-
> > 
> >  include/linux/microchipphy.h |   72 +--
> >  4 files changed, 756 insertions(+), 756 deletions(-)
> 
> 
> Because there is no specific rule how to name defines, I'm not sure it is 
> worth to change 1000+ lines.
> It may be better to set guideline for new submissions.
> 
> Welcome any comments.

Generally, more conforming to norms is better.
These FOO_ uses are non-conforming.

Is there anything other than a one-time cost
to apply these?  Is the same code used for
other platforms?
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[Ayaka]

從我的 iPad 傳送

> John Youn  於 2016年9月7日 上午2:54 寫道：
> 
>> On 9/5/2016 10:15 PM, Kishon Vijay Abraham I wrote:
>> Hi,
>> 
>>> On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
>>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>>> port) the PHY can get into a bad state when a wakeup is asserted (not
>>> just a wakeup from full system suspend but also a wakeup from
>>> autosuspend).
>>> 
>>> We can get the PHY out of its bad state by asserting its "port reset",
>>> but unfortunately that seems to assert a reset onto the USB bus so it
>>> could confuse things if we don't actually deenumerate / reenumerate the
>>> device.
>>> 
>>> We can also get the PHY out of its bad state by fully resetting it using
>>> the reset from the CRU (clock reset unit) in chip, which does a more full
>>> reset.  The CRU-based reset appears to actually cause devices on the bus
>>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>>> way).
>>> 
>>> It's unfortunate that we need to do a full re-enumeration of devices at
>>> wakeup time, but this is better than alternative of letting the bus get
>>> wedged.
>>> 
>>> Signed-off-by: Randy Li 
>>> ---
>>> drivers/usb/dwc2/core_intr.c | 12 
>>> 1 file changed, 12 insertions(+)
>>> 
>>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>>> index d85c5c9..08485b7 100644
>>> --- a/drivers/usb/dwc2/core_intr.c
>>> +++ b/drivers/usb/dwc2/core_intr.c
>>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct 
>>> dwc2_hsotg *hsotg)
>>> static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>> {
>>>int ret;
>>> +struct device_node *np = hsotg->dev->of_node;
>>> 
>>>/* Clear interrupt */
>>>dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>>> @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct 
>>> dwc2_hsotg *hsotg)
>>>/* Restart the Phy Clock */
>>>pcgcctl &= ~PCGCTL_STOPPCLK;
>>>dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>>> +
>>> +/*
>>> + * It is a quirk in Rockchip RK3288, causing by
>>> + * a hardware bug. This will propagate out and
>>> + * eventually we'll re-enumerate the device. 
>>> + * Not great but the best we can do 
>>> + */
>>> +if (of_device_is_compatible(np, "rockchip,rk3288-usb")
>>> +&& (NULL != hsotg->phy->ops->reset))
>>> +hsotg->phy->ops->reset(hsotg->phy);
>> 
>> never call the phy ops directly from the controller driver. It has to be
>> protected as well.
> 
> It looks like we should be calling an API function instead, correct?
> 
Could you give me a example for the wrapper of phy ops?
> Similar to the wrappers for the other ops.
> 
> Regards,
> John

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] clk: gxbb: expose USB clocks

[Stephen Boyd]

On 09/04, Martin Blumenstingl wrote:
> USB0_DDR_BRIDGE and USB1_DDR_BRIDGE1 are needed for the related
> dwc2 usb controller. USB, USB0 and USB1 are needed for the PHYs.
> Expose these clocks to DT and comment out in clk driver.
> 
> Signed-off-by: Jerome Brunet 
> Signed-off-by: Martin Blumenstingl 

Is authorship correct on this patch? Did Jerome author it
instead?

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Woojung.Huh]

> Joe Perches (2):
>   lan78xx: Remove locally defined trailing underscores from defines and uses
>   microchipphy.h and uses: Remove trailing underscores from defines and
> uses
> 
>  drivers/net/phy/microchip.c  |4 +-
>  drivers/net/usb/lan78xx.c|  368 +++
>  drivers/net/usb/lan78xx.h| 1068 +-
> 
>  include/linux/microchipphy.h |   72 +--
>  4 files changed, 756 insertions(+), 756 deletions(-)

Because there is no specific rule how to name defines, I'm not sure it is worth 
to change 1000+ lines.
It may be better to set guideline for new submissions.

Welcome any comments.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] lan78xx: Remove locally defined trailing underscores from defines and uses

[Joe Perches]

Macro defines with trailing underscore are hard to read.

These locally defined ones with trailing underscores are all unique
without the trailing underscore, so remove them from the defines and uses.

Global defines that start with LAN88XX_ that are excluded.

Done with:

$ perl -p -i -e 's/\b(?!LAN88XX_)([A-Z0-9_][A-Za-z0-9_]+)_\b/\U\1\E/g' \
  drivers/net/usb/lan78xx.[ch]

and some editing to realign columns in the .h file.

No change in defconfig object.

Signed-off-by: Joe Perches 
---
 drivers/net/usb/lan78xx.c |  340 +++
 drivers/net/usb/lan78xx.h | 1068 ++---
 2 files changed, 704 insertions(+), 704 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 432b8a3..b6d6d0f 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -548,7 +548,7 @@ static int lan78xx_phy_wait_not_busy(struct lan78xx_net 
*dev)
if (unlikely(ret < 0))
return -EIO;
 
-   if (!(val & MII_ACC_MII_BUSY_))
+   if (!(val & MII_ACC_MII_BUSY))
return 0;
} while (!time_after(jiffies, start_time + HZ));
 
@@ -559,13 +559,13 @@ static inline u32 mii_access(int id, int index, int read)
 {
u32 ret;
 
-   ret = ((u32)id << MII_ACC_PHY_ADDR_SHIFT_) & MII_ACC_PHY_ADDR_MASK_;
-   ret |= ((u32)index << MII_ACC_MIIRINDA_SHIFT_) & MII_ACC_MIIRINDA_MASK_;
+   ret = ((u32)id << MII_ACC_PHY_ADDR_SHIFT) & MII_ACC_PHY_ADDR_MASK;
+   ret |= ((u32)index << MII_ACC_MIIRINDA_SHIFT) & MII_ACC_MIIRINDA_MASK;
if (read)
-   ret |= MII_ACC_MII_READ_;
+   ret |= MII_ACC_MII_READ;
else
-   ret |= MII_ACC_MII_WRITE_;
-   ret |= MII_ACC_MII_BUSY_;
+   ret |= MII_ACC_MII_WRITE;
+   ret |= MII_ACC_MII_BUSY;
 
return ret;
 }
@@ -581,13 +581,13 @@ static int lan78xx_wait_eeprom(struct lan78xx_net *dev)
if (unlikely(ret < 0))
return -EIO;
 
-   if (!(val & E2P_CMD_EPC_BUSY_) ||
-   (val & E2P_CMD_EPC_TIMEOUT_))
+   if (!(val & E2P_CMD_EPC_BUSY) ||
+   (val & E2P_CMD_EPC_TIMEOUT))
break;
usleep_range(40, 100);
} while (!time_after(jiffies, start_time + HZ));
 
-   if (val & (E2P_CMD_EPC_TIMEOUT_ | E2P_CMD_EPC_BUSY_)) {
+   if (val & (E2P_CMD_EPC_TIMEOUT | E2P_CMD_EPC_BUSY)) {
netdev_warn(dev->net, "EEPROM read operation timeout");
return -EIO;
}
@@ -606,7 +606,7 @@ static int lan78xx_eeprom_confirm_not_busy(struct 
lan78xx_net *dev)
if (unlikely(ret < 0))
return -EIO;
 
-   if (!(val & E2P_CMD_EPC_BUSY_))
+   if (!(val & E2P_CMD_EPC_BUSY))
return 0;
 
usleep_range(40, 100);
@@ -629,8 +629,8 @@ static int lan78xx_read_raw_eeprom(struct lan78xx_net *dev, 
u32 offset,
 */
ret = lan78xx_read_reg(dev, HW_CFG, );
saved = val;
-   if (dev->chipid == ID_REV_CHIP_ID_7800_) {
-   val &= ~(HW_CFG_LED1_EN_ | HW_CFG_LED0_EN_);
+   if (dev->chipid == ID_REV_CHIP_ID_7800) {
+   val &= ~(HW_CFG_LED1_EN | HW_CFG_LED0_EN);
ret = lan78xx_write_reg(dev, HW_CFG, val);
}
 
@@ -639,8 +639,8 @@ static int lan78xx_read_raw_eeprom(struct lan78xx_net *dev, 
u32 offset,
return retval;
 
for (i = 0; i < length; i++) {
-   val = E2P_CMD_EPC_BUSY_ | E2P_CMD_EPC_CMD_READ_;
-   val |= (offset & E2P_CMD_EPC_ADDR_MASK_);
+   val = E2P_CMD_EPC_BUSY | E2P_CMD_EPC_CMD_READ;
+   val |= (offset & E2P_CMD_EPC_ADDR_MASK);
ret = lan78xx_write_reg(dev, E2P_CMD, val);
if (unlikely(ret < 0)) {
retval = -EIO;
@@ -663,7 +663,7 @@ static int lan78xx_read_raw_eeprom(struct lan78xx_net *dev, 
u32 offset,
 
retval = 0;
 exit:
-   if (dev->chipid == ID_REV_CHIP_ID_7800_)
+   if (dev->chipid == ID_REV_CHIP_ID_7800)
ret = lan78xx_write_reg(dev, HW_CFG, saved);
 
return retval;
@@ -697,8 +697,8 @@ static int lan78xx_write_raw_eeprom(struct lan78xx_net 
*dev, u32 offset,
 */
ret = lan78xx_read_reg(dev, HW_CFG, );
saved = val;
-   if (dev->chipid == ID_REV_CHIP_ID_7800_) {
-   val &= ~(HW_CFG_LED1_EN_ | HW_CFG_LED0_EN_);
+   if (dev->chipid == ID_REV_CHIP_ID_7800) {
+   val &= ~(HW_CFG_LED1_EN | HW_CFG_LED0_EN);
ret = lan78xx_write_reg(dev, HW_CFG, val);
}
 
@@ -707,7 +707,7 @@ static int lan78xx_write_raw_eeprom(struct lan78xx_net 
*dev, u32 offset,
goto exit;
 
/* Issue write/erase enable command */
-   val = E2P_CMD_EPC_BUSY_ | E2P_CMD_EPC_CMD_EWEN_;
+   val = 

[PATCH 0/2] lan78xx: Remove trailing underscores from macros

[Joe Perches]

Joe Perches (2):
  lan78xx: Remove locally defined trailing underscores from defines and uses
  microchipphy.h and uses: Remove trailing underscores from defines and uses

 drivers/net/phy/microchip.c  |4 +-
 drivers/net/usb/lan78xx.c|  368 +++
 drivers/net/usb/lan78xx.h| 1068 +-
 include/linux/microchipphy.h |   72 +--
 4 files changed, 756 insertions(+), 756 deletions(-)

-- 
2.10.0.rc2.1.g053435c

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2] microchipphy.h and uses: Remove trailing underscores from defines and uses

[Joe Perches]

Macro defines with trailing underscore are hard to read.

These uses with trailing underscores are all unique to this .h file,
so remove them from the defines and uses.

Done with:

$ sed -r -i -e 's/\b([A-Z0-9_][A-Za-z0-9_]+)_\b/\U\1\E/g' \
  include/linux/microchipphy.h \
  drivers/net/usb/lan78xx.c \
  drivers/net/phy/microchip.c

and some editing to realign columns in the .h file.

No change in defconfig objects.

Signed-off-by: Joe Perches 
---
 drivers/net/phy/microchip.c  |  4 +--
 drivers/net/usb/lan78xx.c| 28 -
 include/linux/microchipphy.h | 72 ++--
 3 files changed, 52 insertions(+), 52 deletions(-)

diff --git a/drivers/net/phy/microchip.c b/drivers/net/phy/microchip.c
index 15f8206..d8ea811 100644
--- a/drivers/net/phy/microchip.c
+++ b/drivers/net/phy/microchip.c
@@ -39,8 +39,8 @@ static int lan88xx_phy_config_intr(struct phy_device *phydev)
rc = phy_write(phydev, LAN88XX_INT_MASK, 0x7FFF);
rc = phy_read(phydev, LAN88XX_INT_STS);
rc = phy_write(phydev, LAN88XX_INT_MASK,
-  LAN88XX_INT_MASK_MDINTPIN_EN_ |
-  LAN88XX_INT_MASK_LINK_CHANGE_);
+  LAN88XX_INT_MASK_MDINTPIN_EN |
+  LAN88XX_INT_MASK_LINK_CHANGE);
} else {
rc = phy_write(phydev, LAN88XX_INT_MASK, 0);
}
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index b6d6d0f..5e870cc 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1457,27 +1457,27 @@ static void lan78xx_set_mdix_status(struct net_device 
*net, __u8 mdix_ctrl)
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_1);
buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);
-   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
+   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK;
phy_write(phydev, LAN88XX_EXT_MODE_CTRL,
- buf | LAN88XX_EXT_MODE_CTRL_MDI_);
+ buf | LAN88XX_EXT_MODE_CTRL_MDI);
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_0);
} else if (mdix_ctrl == ETH_TP_MDI_X) {
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_1);
buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);
-   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
+   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK;
phy_write(phydev, LAN88XX_EXT_MODE_CTRL,
- buf | LAN88XX_EXT_MODE_CTRL_MDI_X_);
+ buf | LAN88XX_EXT_MODE_CTRL_MDI_X);
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_0);
} else if (mdix_ctrl == ETH_TP_MDI_AUTO) {
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_1);
buf = phy_read(phydev, LAN88XX_EXT_MODE_CTRL);
-   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
+   buf &= ~LAN88XX_EXT_MODE_CTRL_MDIX_MASK;
phy_write(phydev, LAN88XX_EXT_MODE_CTRL,
- buf | LAN88XX_EXT_MODE_CTRL_AUTO_MDIX_);
+ buf | LAN88XX_EXT_MODE_CTRL_AUTO_MDIX);
phy_write(phydev, LAN88XX_EXT_PAGE_ACCESS,
  LAN88XX_EXT_PAGE_SPACE_0);
}
@@ -1499,14 +1499,14 @@ static int lan78xx_get_settings(struct net_device *net, 
struct ethtool_cmd *cmd)
 
buf = lan78xx_get_mdix_status(net);
 
-   buf &= LAN88XX_EXT_MODE_CTRL_MDIX_MASK_;
-   if (buf == LAN88XX_EXT_MODE_CTRL_AUTO_MDIX_) {
+   buf &= LAN88XX_EXT_MODE_CTRL_MDIX_MASK;
+   if (buf == LAN88XX_EXT_MODE_CTRL_AUTO_MDIX) {
cmd->eth_tp_mdix = ETH_TP_MDI_AUTO;
cmd->eth_tp_mdix_ctrl = ETH_TP_MDI_AUTO;
-   } else if (buf == LAN88XX_EXT_MODE_CTRL_MDI_) {
+   } else if (buf == LAN88XX_EXT_MODE_CTRL_MDI) {
cmd->eth_tp_mdix = ETH_TP_MDI;
cmd->eth_tp_mdix_ctrl = ETH_TP_MDI;
-   } else if (buf == LAN88XX_EXT_MODE_CTRL_MDI_X_) {
+   } else if (buf == LAN88XX_EXT_MODE_CTRL_MDI_X) {
cmd->eth_tp_mdix = ETH_TP_MDI_X;
cmd->eth_tp_mdix_ctrl = ETH_TP_MDI_X;
}
@@ -1815,7 +1815,7 @@ static void lan78xx_link_status_change(struct net_device 
*net)
if (!phydev->autoneg && (phydev->speed == 100)) {
/* disable phy interrupt */
temp = phy_read(phydev, LAN88XX_INT_MASK);
-   temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN_;
+   temp &= ~LAN88XX_INT_MASK_MDINTPIN_EN;
ret = phy_write(phydev, LAN88XX_INT_MASK, temp);
 
temp = phy_read(phydev, MII_BMCR);
@@ -1829,7 +1829,7 @@ 

Re: [PATCH] lan78xx: mark symbols static where possible

[David Miller]

From: Baoyou Xie 
Date: Tue,  6 Sep 2016 16:19:02 +0800

> We get a few warnings when building kernel with W=1:
> drivers/net/usb/lan78xx.c:1182:6: warning: no previous prototype for 
> 'lan78xx_defer_kevent' [-Wmissing-prototypes]
> drivers/net/usb/lan78xx.c:1409:5: warning: no previous prototype for 
> 'lan78xx_nway_reset' [-Wmissing-prototypes]
> drivers/net/usb/lan78xx.c:2000:5: warning: no previous prototype for 
> 'lan78xx_set_mac_addr' [-Wmissing-prototypes]
> 
> 
> In fact, these functions are only used in the file in which they are
> declared and don't need a declaration, but can be made static.
> so this patch marks these functions with 'static'.
> 
> Signed-off-by: Baoyou Xie 

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] net: asix: autoneg will set WRITE_MEDIUM reg

[Robert Foss]

On 2016-09-06 12:41 PM, Grant Grundler wrote:

On Thu, Sep 1, 2016 at 10:02 AM, Eric Dumazet  wrote:

On Thu, 2016-09-01 at 12:47 -0400, Robert Foss wrote:


I'm not quite sure how the first From line was added, it
should not have been.
Grant Grundler is most definitely the author.

Would you like me to resubmit in v++ and make sure that it has been
corrected?


This is too late, patches are already merged in David Miller net-next
tree.

These kinds of errors can not be fixed, we have to be very careful at
submission/review time.

I guess Grant does not care, but some contributors, especially new ones
would like to get proper attribution.


I do not mind. Robert will get email about bugs instead of me. :D


Thanks Grant, sorry about the mixup!


Rob.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFT PATCH v4 0/3] usb: dwc2: Fix core reset and force mode delays

[Stefan Wahren]

Hi John,

> John Youn  hat am 1. September 2016 um 23:07
> geschrieben:
> 
> 
> This series accounts for the delay from the IDDIG debounce filter when
> switching modes. This delay is a function of the PHY clock speed and
> can range from 5-50 ms. This delay must be taken into account on core
> reset and force modes. A full explanation is provided in the patch
> commit log and code comments.
> 
> This revision of the series increases the IDDIG delay to 100 ms. Some
> rockchip platforms seem to timeout even with 50 ms so I have doubled
> this.
> 
> Appreciate any testing on RK3188 and RPi platforms.

i tested the whole series successful with a Raspberry Pi B in dr_mode "host" and
"otg"

Tested-by: Stefan Wahren 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] smsc95xx: Remove trailing underscores from macros and uses

[Joe Perches]

Make the #defines a bit more sensible to read.

Done with:

$ sed -r -i -e 's/\b([A-Z0-9_][A-Za-z0-9_]+)_\b/\U\1\E/g' \
  drivers/net/usb/smsc95xx.[ch]

and a little editing of the .h file to realign tabstop define values.

There are 3 [RT]x_COE_ defines that were also uppercased.

No change in defconfig objects.

Signed-off-by: Joe Perches 
---
 drivers/net/usb/smsc95xx.c | 198 +--
 drivers/net/usb/smsc95xx.h | 330 ++---
 2 files changed, 264 insertions(+), 264 deletions(-)

diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
index 831aa33..c657fe1 100644
--- a/drivers/net/usb/smsc95xx.c
+++ b/drivers/net/usb/smsc95xx.c
@@ -178,7 +178,7 @@ static int __must_check __smsc95xx_phy_wait_not_busy(struct 
usbnet *dev,
return ret;
}
 
-   if (!(val & MII_BUSY_))
+   if (!(val & MII_BUSY))
return 0;
} while (!time_after(jiffies, start_time + HZ));
 
@@ -204,7 +204,7 @@ static int __smsc95xx_mdio_read(struct net_device *netdev, 
int phy_id, int idx,
/* set the address, index & direction (read from PHY) */
phy_id &= dev->mii.phy_id_mask;
idx &= dev->mii.reg_num_mask;
-   addr = (phy_id << 11) | (idx << 6) | MII_READ_ | MII_BUSY_;
+   addr = (phy_id << 11) | (idx << 6) | MII_READ | MII_BUSY;
ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
if (ret < 0) {
netdev_warn(dev->net, "Error writing MII_ADDR\n");
@@ -256,7 +256,7 @@ static void __smsc95xx_mdio_write(struct net_device 
*netdev, int phy_id,
/* set the address, index & direction (write to PHY) */
phy_id &= dev->mii.phy_id_mask;
idx &= dev->mii.reg_num_mask;
-   addr = (phy_id << 11) | (idx << 6) | MII_WRITE_ | MII_BUSY_;
+   addr = (phy_id << 11) | (idx << 6) | MII_WRITE | MII_BUSY;
ret = __smsc95xx_write_reg(dev, MII_ADDR, addr, in_pm);
if (ret < 0) {
netdev_warn(dev->net, "Error writing MII_ADDR\n");
@@ -309,12 +309,12 @@ static int __must_check smsc95xx_wait_eeprom(struct 
usbnet *dev)
return ret;
}
 
-   if (!(val & E2P_CMD_BUSY_) || (val & E2P_CMD_TIMEOUT_))
+   if (!(val & E2P_CMD_BUSY) || (val & E2P_CMD_TIMEOUT))
break;
udelay(40);
} while (!time_after(jiffies, start_time + HZ));
 
-   if (val & (E2P_CMD_TIMEOUT_ | E2P_CMD_BUSY_)) {
+   if (val & (E2P_CMD_TIMEOUT | E2P_CMD_BUSY)) {
netdev_warn(dev->net, "EEPROM read operation timeout\n");
return -EIO;
}
@@ -335,7 +335,7 @@ static int __must_check 
smsc95xx_eeprom_confirm_not_busy(struct usbnet *dev)
return ret;
}
 
-   if (!(val & E2P_CMD_BUSY_))
+   if (!(val & E2P_CMD_BUSY))
return 0;
 
udelay(40);
@@ -359,7 +359,7 @@ static int smsc95xx_read_eeprom(struct usbnet *dev, u32 
offset, u32 length,
return ret;
 
for (i = 0; i < length; i++) {
-   val = E2P_CMD_BUSY_ | E2P_CMD_READ_ | (offset & E2P_CMD_ADDR_);
+   val = E2P_CMD_BUSY | E2P_CMD_READ | (offset & E2P_CMD_ADDR);
ret = smsc95xx_write_reg(dev, E2P_CMD, val);
if (ret < 0) {
netdev_warn(dev->net, "Error writing E2P_CMD\n");
@@ -397,7 +397,7 @@ static int smsc95xx_write_eeprom(struct usbnet *dev, u32 
offset, u32 length,
return ret;
 
/* Issue write/erase enable command */
-   val = E2P_CMD_BUSY_ | E2P_CMD_EWEN_;
+   val = E2P_CMD_BUSY | E2P_CMD_EWEN;
ret = smsc95xx_write_reg(dev, E2P_CMD, val);
if (ret < 0) {
netdev_warn(dev->net, "Error writing E2P_DATA\n");
@@ -419,7 +419,7 @@ static int smsc95xx_write_eeprom(struct usbnet *dev, u32 
offset, u32 length,
}
 
/* Send "write" command */
-   val = E2P_CMD_BUSY_ | E2P_CMD_WRITE_ | (offset & E2P_CMD_ADDR_);
+   val = E2P_CMD_BUSY | E2P_CMD_WRITE | (offset & E2P_CMD_ADDR);
ret = smsc95xx_write_reg(dev, E2P_CMD, val);
if (ret < 0) {
netdev_warn(dev->net, "Error writing E2P_CMD\n");
@@ -478,17 +478,17 @@ static void smsc95xx_set_multicast(struct net_device 
*netdev)
 
if (dev->net->flags & IFF_PROMISC) {
netif_dbg(dev, drv, dev->net, "promiscuous mode enabled\n");
-   pdata->mac_cr |= MAC_CR_PRMS_;
-   pdata->mac_cr &= ~(MAC_CR_MCPAS_ | MAC_CR_HPFILT_);
+   pdata->mac_cr |= MAC_CR_PRMS;
+   pdata->mac_cr &= ~(MAC_CR_MCPAS | MAC_CR_HPFILT);
} else if (dev->net->flags & IFF_ALLMULTI) {
netif_dbg(dev, drv, dev->net, "receive all multicast 
enabled\n");
- 

Re: [PATCH 0/2] hso: neatening

[David Miller]

From: Joe Perches 
Date: Fri,  2 Sep 2016 15:58:00 -0700

> This seems to be the only code in the kernel that uses
> macro defines with a trailing underscore.  Fix that.

Series applied, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/3] da8xx USB PHY platform devices and clocks

[Bin Liu]

Hi Greg and Alan,

On Mon, Sep 05, 2016 at 03:00:30PM -0500, David Lechner wrote:
> Just resending to get these merged into usb. The phy parts of this patch 
> series
> have already been merged into Linus' tree.
> 
> I have rebased on 4.8-rc5 but there have not been any changes to these since
> the last time I submitted.
> 
> David Lechner (3):
>   usb: ohci-da8xx: Remove code that references mach
>   usb: musb: da8xx: Use devm in probe
>   usb: musb: da8xx: Remove mach code

How do we take care this kinda patch sets, which touch multiple modules?
I take the two for musb or just acked-by them?

Thanks,
-Bin.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 2/3] usb: musb: da8xx: Use devm in probe

[Bin Liu]

Hi,

On Mon, Sep 05, 2016 at 03:00:32PM -0500, David Lechner wrote:
> Simplify things a bit by using devm functions where possible.
> 
> Signed-off-by: David Lechner 
> ---
>  drivers/usb/musb/da8xx.c | 19 +--
>  1 file changed, 5 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/musb/da8xx.c b/drivers/usb/musb/da8xx.c
> index b03d3b8..0c1997c 100644
> --- a/drivers/usb/musb/da8xx.c
> +++ b/drivers/usb/musb/da8xx.c
> @@ -490,20 +490,18 @@ static int da8xx_probe(struct platform_device *pdev)
>   struct da8xx_glue   *glue;
>   struct platform_device_info pinfo;
>   struct clk  *clk;
> + int ret;
>  
> - int ret = -ENOMEM;
> -
> - glue = kzalloc(sizeof(*glue), GFP_KERNEL);
> + glue = devm_kzalloc(>dev, sizeof(*glue), GFP_KERNEL);
>   if (!glue) {
>   dev_err(>dev, "failed to allocate glue context\n");

Greg's usb-next already has a patch which drops this line. You might
have to rebase this patch, so that it can go to v4.9-rc1.

Thanks,
-Bin.

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 3/4] usb: dwc2: assert phy reset when waking up in rk3288 platform

[John Youn]

On 9/5/2016 10:15 PM, Kishon Vijay Abraham I wrote:
> Hi,
> 
> On Sunday 04 September 2016 03:25 AM, Randy Li wrote:
>> On the rk3288 USB host-only port (the one that's not the OTG-enabled
>> port) the PHY can get into a bad state when a wakeup is asserted (not
>> just a wakeup from full system suspend but also a wakeup from
>> autosuspend).
>>
>> We can get the PHY out of its bad state by asserting its "port reset",
>> but unfortunately that seems to assert a reset onto the USB bus so it
>> could confuse things if we don't actually deenumerate / reenumerate the
>> device.
>>
>> We can also get the PHY out of its bad state by fully resetting it using
>> the reset from the CRU (clock reset unit) in chip, which does a more full
>> reset.  The CRU-based reset appears to actually cause devices on the bus
>> to be removed and reinserted, which fixes the problem (albeit in a hacky
>> way).
>>
>> It's unfortunate that we need to do a full re-enumeration of devices at
>> wakeup time, but this is better than alternative of letting the bus get
>> wedged.
>>
>> Signed-off-by: Randy Li 
>> ---
>>  drivers/usb/dwc2/core_intr.c | 12 
>>  1 file changed, 12 insertions(+)
>>
>> diff --git a/drivers/usb/dwc2/core_intr.c b/drivers/usb/dwc2/core_intr.c
>> index d85c5c9..08485b7 100644
>> --- a/drivers/usb/dwc2/core_intr.c
>> +++ b/drivers/usb/dwc2/core_intr.c
>> @@ -345,6 +345,7 @@ static void dwc2_handle_session_req_intr(struct 
>> dwc2_hsotg *hsotg)
>>  static void dwc2_handle_wakeup_detected_intr(struct dwc2_hsotg *hsotg)
>>  {
>>  int ret;
>> +struct device_node *np = hsotg->dev->of_node;
>>  
>>  /* Clear interrupt */
>>  dwc2_writel(GINTSTS_WKUPINT, hsotg->regs + GINTSTS);
>> @@ -379,6 +380,17 @@ static void dwc2_handle_wakeup_detected_intr(struct 
>> dwc2_hsotg *hsotg)
>>  /* Restart the Phy Clock */
>>  pcgcctl &= ~PCGCTL_STOPPCLK;
>>  dwc2_writel(pcgcctl, hsotg->regs + PCGCTL);
>> +
>> +/*
>> + * It is a quirk in Rockchip RK3288, causing by
>> + * a hardware bug. This will propagate out and
>> + * eventually we'll re-enumerate the device. 
>> + * Not great but the best we can do 
>> + */
>> +if (of_device_is_compatible(np, "rockchip,rk3288-usb")
>> +&& (NULL != hsotg->phy->ops->reset))
>> +hsotg->phy->ops->reset(hsotg->phy);
> 
> never call the phy ops directly from the controller driver. It has to be
> protected as well.

It looks like we should be calling an API function instead, correct?

Similar to the wrappers for the other ops.

Regards,
John
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: generic: request regulator optionally

[Stefan Agner]

On 2016-09-06 01:22, Mark Brown wrote:
> On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
>> Stefan Agner  writes:
> 
>> > According to the device tree bindings the vcc-supply is optional.
> 
> This is nonsense unless the device can work without this supply.  Given
> that the supply is called VCC that doesn't seem entirely likely.

Afaik it is kind of a generic device tree binding, I guess the physical
device can have various appearances and properties...

A quick survey showed several device trees which do not specify
vcc-supply...

That said, I checked the device at hand, and it actually has a USB PHY
power supply inputs, but the device tree does not model them.

>> > +  nop->vcc = devm_regulator_get_optional(dev, "vcc");
>> >if (IS_ERR(nop->vcc)) {
>> >dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>> >PTR_ERR(nop->vcc));
>> > -  if (needs_vcc)
>> > -  return -EPROBE_DEFER;
>> > +  if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
>> > +  return PTR_ERR(nop->vcc);
> 
>> does this look okay from a regulator API perspective?
> 
> That's how to use _get_optional() but it's really unusual that you
> should be using _get_optional().

Despite the above findings, I still think it is the right thing to do as
long as we specify vcc-supply to be optional.

--
Stefan
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 10/22] usb: chipidea: Consolidate extcon notifiers

[Stephen Boyd]

On Sun, Sep 4, 2016 at 7:39 PM, Peter Chen  wrote:
> On Fri, Sep 02, 2016 at 06:03:06PM -0700, Stephen Boyd wrote:
>> On Thu, Sep 1, 2016 at 8:17 PM, Peter Chen  wrote:
>> > On Wed, Aug 31, 2016 at 05:40:24PM -0700, Stephen Boyd wrote:
>> >>
>> >>
>> >>   if (cable->state)
>> >> - val |= OTGSC_ID;
>> >> + val &= ~OTGSC_ID; /* A device */
>> >>   else
>> >> - val &= ~OTGSC_ID;
>> >> + val |= OTGSC_ID; /* B device */
>
> One more comment, would you change the comment to "host" and "device"?
> Unless we are supporting OTG-compliance device, otherwise, we should
> not mention "A" or "B" for USB device.
>

Done
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]

[Ritesh Raj Sarraf]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hello Ulf,

On Tue, 2016-09-06 at 11:42 +0200, Ulf Hansson wrote:
> 
> By reviewing the code of the rtsx_usb_sdmmc driver, particularly how
> it calls pm_runtime_get|put() I am guessing those calls may not be
> properly deployed. Perhaps rtsx_usb_sdmmc should convert to use the
> usb_autopm_put|get_interface() and friends, although I didn't want to
> make that change at this point so instead I have cooked a patch that
> might fixes the behaviour.
> 
> Ritesh, can you please try it out to see what happens?

I was able to hit the issue again, with your patch applied. I tried it on top of
the 4.8-rc5 kernel.

I ensured to capture usbmon trace.
https://people.debian.org/~rrs/tmp/usbmon-ulf.txt.gz


- -- 
Ritesh Raj Sarraf
RESEARCHUT - http://www.researchut.com
"Necessity is the mother of invention."
-BEGIN PGP SIGNATURE-

iQIcBAEBCgAGBQJXzvgmAAoJEKY6WKPy4XVpJ/oQAIR/bC82KdqD3xuQpq8r62RT
fV8DSduNhr394GIDO5PBDSKUAFDKCHL9SyCBx9I5zx1U7OF+IfvhgpRji+XcBsQ5
TOtDAhiiTatKfWl/zLGbVqukNS8QLzyU26w0HRabvK3RZ0lGxBugQ8KneUC8wA/Q
NYWAgio5gN5jpD6BgGEZXo6cdzcdI1HM5kyNxcs4VfhD0zDor4wlrW024458a3Fs
02QAkmyV1aBwR4w/Ntw7D9DXb8sVujGgeEOr+KukzdFOPW+w4JOANv3gregDnKSc
EgIYmtTE95d0jWyJuS5sbZlbf4QOcrv9eVwn86TOdhZHU1XlaqaIEbRPzu15ZtZF
B+zYbWg+TgTYFgTYJoaB8T+CUJMMnNh62IC72LXa5K/C0kjgjlHUzis04XdFmT2q
jYC767M1utXi7a4LcXxw8V8uoV0nnmuvxWzhrcdBxi6+a85BLL8bPKT+nrGxkxB0
Hg7Wmnc9moohpjYD3KlAYR3bW56dDYrGgsj8xRUnplBkUp10g2vhix7bk/q+eUwU
gJghnI/WgEE5hBQabtuqCTCsvMdTXmZVjzMg1k43V5v+UR0a7mKVW0uBWu5cU38p
Ga9ZrNmPEzxOVmeXH3XCB8fCXRgce8ieu/KEPHpvbegia0nX4X7BcCHhK2KGsqnX
Y4f+TvpJw2idUe/cvdtw
=/aP1
-END PGP SIGNATURE-

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 5/5] net: asix: autoneg will set WRITE_MEDIUM reg

[Grant Grundler]

On Thu, Sep 1, 2016 at 10:02 AM, Eric Dumazet  wrote:
> On Thu, 2016-09-01 at 12:47 -0400, Robert Foss wrote:
>
>> I'm not quite sure how the first From line was added, it
>> should not have been.
>> Grant Grundler is most definitely the author.
>>
>> Would you like me to resubmit in v++ and make sure that it has been
>> corrected?
>
> This is too late, patches are already merged in David Miller net-next
> tree.
>
> These kinds of errors can not be fixed, we have to be very careful at
> submission/review time.
>
> I guess Grant does not care, but some contributors, especially new ones
> would like to get proper attribution.

I do not mind. Robert will get email about bugs instead of me. :D

cheers,
grant

>
> Thanks.
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Fast Loans

[Financial Service]

Apply for a loan at 2% reply to this Email for more Info
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 10:46:55AM -0400, Alan Stern wrote:

Not knowing where INFO() goes, you should use trace_printk() not
printk(), as the former is strictly per cpu, while the latter is
globally serialized and can hide all these problems.

> Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> ===
> --- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
> +++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
> @@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
>   spin_lock(>lock);
>   bh->outreq_busy = 0;
>   bh->state = BUF_STATE_FULL;
> + if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
> + INFO(common, "compl: bh %p state %d\n", bh, bh->state);
>   wakeup_thread(common);
>   spin_unlock(>lock);
>  }
> @@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
>   rc = sleep_thread(common, true);
>   if (rc)
>   return rc;
> + INFO(common, "next: bh %p state %d\n", bh, bh->state);
>   }
>   smp_rmb();
>   rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Alan Stern]

On Tue, 6 Sep 2016, Peter Zijlstra wrote:

> On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
> > On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:
> 
> > > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> > > adds extra overhead which makes the problem much, much less likely to
> > > happen. Does that sound plausible to you?
> > 
> > I did consider that, but I've not sufficiently grokked the code to rule
> > out actual fail. So let me stare at this a bit more.
> 
> OK, so I'm really not seeing it, we've got:
> 
> while (bh->state != FULL) {
> for (;;) {
> set_current_state(INTERRUPTIBLE); /* MB after */
> if (signal_pending(current))
> return -EINTR;
> if (common->thread_wakeup_needed)
> break;
> schedule(); /* MB */
> }
> __set_current_state(RUNNING);
> common->thread_wakeup_needed = 0;
> smp_rmb(); /* NOP */
> }
> 
> 
> VS.
> 
> 
> spin_lock(>lock); /* MB */
> bh->state = FULL;
> smp_wmb(); /* NOP */
> common->thread_wakeup_needed = 1;
> wake_up_process(common->thread_task); /* MB before */
> spin_unlock(>lock);
> 
> 
> 
> (the MB annotations specific to x86, not true in general)
> 
> 
> If we observe thread_wakeup_needed, we must also observe bh->state.
> 
> And the sleep/wakeup ordering is also correct, we either see
> thread_wakeup_needed and continue, or we see task->state == RUNNING
> (from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
> then matches with the MB from wake_up_process() to ensure we must see
> thead_wakeup_needed.
> 
> Or, we go sleep, and get woken up, at which point the same happens.
> Since the waking CPU gets the task back on its RQ the happens-before
> chain includes the waking CPUs state along with the state of the task
> itself before it went to sleep.
> 
> At which point we're back where we started, once we see
> thread_wakeup_needed we must then also see bh->state (and all state
> prior to that on the waking CPU).
> 
> 
> 
> There's enough cruft in the while-sleep loop to force reload bh->state.
> 
> Load/store tearing cannot be a problem because all values are single
> bytes (the variables are multi bytes, but all values used only affect
> the LSB).
> 
> Colour me puzzled.

Felipe, can you please try this patch on an unmodified tree?  If the 
problem still occurs, what shows up in the kernel log?

Alan Stern



Index: usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
===
--- usb-4.x.orig/drivers/usb/gadget/function/f_mass_storage.c
+++ usb-4.x/drivers/usb/gadget/function/f_mass_storage.c
@@ -485,6 +485,8 @@ static void bulk_out_complete(struct usb
spin_lock(>lock);
bh->outreq_busy = 0;
bh->state = BUF_STATE_FULL;
+   if (bh->bulk_out_intended_length == US_BULK_CB_WRAP_LEN)
+   INFO(common, "compl: bh %p state %d\n", bh, bh->state);
wakeup_thread(common);
spin_unlock(>lock);
 }
@@ -2207,6 +2209,7 @@ static int get_next_command(struct fsg_c
rc = sleep_thread(common, true);
if (rc)
return rc;
+   INFO(common, "next: bh %p state %d\n", bh, bh->state);
}
smp_rmb();
rc = fsg_is_set(common) ? received_cbw(common->fsg, bh) : -EIO;

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: xhci-plat: Add generic PHY support

[Mathias Nyman]

On 29.08.2016 10:26, Srinath Mannam wrote:

Hi Mathias Nyman,

Could you please provide your inputs on this?
If you find it in good condition, please consider it for the next release.



I'm not that involved in the usb phy or generic phy, but to me it looks like
we should let usb core handle this and not duplicate any more of the phy code to
xhci-plat.c

usb_add_hcd() in hcd.c already takes care of both usb phy and generic phy cases,

I'd like to remove the phy code from xhci-plat.c if possible, but haven't
looked into if there are some parts that must be done in xhci-plat.c still.

-Mathias




--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Tuesday, September 6, 2016 1:50:48 PM CEST Felipe Balbi wrote:
> Hi,
> 
> Arnd Bergmann  writes:
> > On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> >> 
> >> this only solves the problem for DT devices. Legacy devices and
> >> PCI-based systems will still suffer from the same problem. At least for
> >> dwc3, I will only be taking patches that solve the problem for all
> >> users, not a subset of them.
> >
> > I don't think legacy devices are a worry, because they wouldn't
> > have this problem. For the PCI case, you are right that it cannot
> > work, in particular for machines that have complex IOMMU setup.
> >
> > Some architectures (at least arm64 and sparc) check the bus_type of
> > a device in order to find the correct set of dma_map_ops for that
> > device, so there is no real way to handle this as long as you
> > pass a platform_device into an API that expects a pci_device.
> 
> Then I guess we're left with adding a "struct device *dma_dev" to struct
> dwc3 and trying to figure out if we should use parent or self. Does
> anybody see any problems with that?

I think we actually need the device pointer in the usb_hcd structure,
so it can be passed in these API calls from the USB core

drivers/usb/core/buffer.c:  return dma_alloc_coherent(hcd->self.controller, 
size, dma, mem_flags);
drivers/usb/core/buffer.c:  dma_free_coherent(hcd->self.controller, size, 
addr, dma);
drivers/usb/core/buffer.c:  (!hcd->self.controller->dma_mask &&
drivers/usb/core/buffer.c:  hcd->pool[i] = dma_pool_create(name, 
hcd->self.controller,
drivers/usb/core/hcd.c: urb->setup_dma = dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: n = dma_map_sg(
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_page(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/hcd.c: urb->transfer_dma = 
dma_map_single(
drivers/usb/core/hcd.c: if 
(dma_mapping_error(hcd->self.controller,
drivers/usb/core/usb.c: urb->transfer_dma = dma_map_single(controller,
drivers/usb/core/usb.c: return dma_map_sg(controller, sg, nents,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_single_for_cpu(controller,
drivers/usb/core/usb.c: dma_sync_sg_for_cpu(controller, sg, n_hw_ents,

as these are all called on behalf of the host controller node.
Looking for more instances of hcd->self.controller, I find this
instance:

drivers/usb/core/hcd.c: struct usb_phy *phy = 
usb_get_phy_dev(hcd->self.controller, 0);
drivers/usb/core/hcd.c: struct phy *phy = phy_get(hcd->self.controller, 
"usb");

I'm unsure which device pointer we want here, but I suspect this also
needs to be the one that has the device node in order to make the lookup
of the phy structure by device node work right. Can you clarify how
this works today?

We probably also need to add the of_node of the host controller device
to struct usb_hcd in order to make usb_of_get_child_node() work
in the case where the hcd itself is not device that is listed
in DT. It might be a good idea to use 'struct fwnode_handle' for that,
so we can in the future also allow ACPI platforms to specify 

> Note, we would NOT be passing device pointers are platform_data, we
> would have dwc3.ko figure out if it should use self or its parent device
> for dma.

Ok, sounds good.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH for v4.8] usb: dwc3: mark PM function as __maybe_unused

[Arnd Bergmann]

Starting with v4.8-rc5, we get a warning about the dwc3_pci_pm_dummy function
when CONFIG_PM_SLEEP is disabled:

drivers/usb/dwc3/dwc3-pci.c:253:12: warning: 'dwc3_pci_pm_dummy' defined but 
not used

Using __maybe_unused instead of #ifdef lets the compiler figure out
whether it should drop the definitions silently and avoids this warning.

Signed-off-by: Arnd Bergmann 
Fixes: f6c274e11e3b ("usb: dwc3: pci: runtime_resume child device")
---
 drivers/usb/dwc3/dwc3-pci.c | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

After we fixed all other warnings reported by kernelci.org, this is
now the only thing we get a warning for in the configurations we build,
so it would be good to get this fixed before v4.8 to get a clean
build for the first time in a release.

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 0a32430f4c41..6a82b36f0d59 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -234,8 +234,7 @@ static const struct pci_device_id dwc3_pci_id_table[] = {
 };
 MODULE_DEVICE_TABLE(pci, dwc3_pci_id_table);
 
-#ifdef CONFIG_PM
-static int dwc3_pci_runtime_suspend(struct device *dev)
+static int __maybe_unused dwc3_pci_runtime_suspend(struct device *dev)
 {
if (device_run_wake(dev))
return 0;
@@ -243,14 +242,14 @@ static int dwc3_pci_runtime_suspend(struct device *dev)
return -EBUSY;
 }
 
-static int dwc3_pci_runtime_resume(struct device *dev)
+static int __maybe_unused dwc3_pci_runtime_resume(struct device *dev)
 {
struct platform_device *dwc3 = dev_get_drvdata(dev);
 
return pm_runtime_get(>dev);
 }
 
-static int dwc3_pci_pm_dummy(struct device *dev)
+static int __maybe_unused dwc3_pci_pm_dummy(struct device *dev)
 {
/*
 * There's nothing to do here. No, seriously. Everything is either taken
@@ -262,7 +261,6 @@ static int dwc3_pci_pm_dummy(struct device *dev)
 */
return 0;
 }
-#endif /* CONFIG_PM */
 
 static struct dev_pm_ops dwc3_pci_dev_pm_ops = {
SET_SYSTEM_SLEEP_PM_OPS(dwc3_pci_pm_dummy, dwc3_pci_pm_dummy)
-- 
2.9.0

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 01:49:37PM +0200, Peter Zijlstra wrote:
> On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> > adds extra overhead which makes the problem much, much less likely to
> > happen. Does that sound plausible to you?
> 
> I did consider that, but I've not sufficiently grokked the code to rule
> out actual fail. So let me stare at this a bit more.

OK, so I'm really not seeing it, we've got:

while (bh->state != FULL) {
for (;;) {
set_current_state(INTERRUPTIBLE); /* MB after */
if (signal_pending(current))
return -EINTR;
if (common->thread_wakeup_needed)
break;
schedule(); /* MB */
}
__set_current_state(RUNNING);
common->thread_wakeup_needed = 0;
smp_rmb(); /* NOP */
}


VS.


spin_lock(>lock); /* MB */
bh->state = FULL;
smp_wmb(); /* NOP */
common->thread_wakeup_needed = 1;
wake_up_process(common->thread_task); /* MB before */
spin_unlock(>lock);



(the MB annotations specific to x86, not true in general)


If we observe thread_wakeup_needed, we must also observe bh->state.

And the sleep/wakeup ordering is also correct, we either see
thread_wakeup_needed and continue, or we see task->state == RUNNING
(from the wakeup) and NO-OP schedule(). The MB from set_current_statE()
then matches with the MB from wake_up_process() to ensure we must see
thead_wakeup_needed.

Or, we go sleep, and get woken up, at which point the same happens.
Since the waking CPU gets the task back on its RQ the happens-before
chain includes the waking CPUs state along with the state of the task
itself before it went to sleep.

At which point we're back where we started, once we see
thread_wakeup_needed we must then also see bh->state (and all state
prior to that on the waking CPU).



There's enough cruft in the while-sleep loop to force reload bh->state.

Load/store tearing cannot be a problem because all values are single
bytes (the variables are multi bytes, but all values used only affect
the LSB).

Colour me puzzled.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Tue, Sep 06, 2016 at 02:43:39PM +0300, Felipe Balbi wrote:

> > Could you confirm that bulk_{in,out}_complete() work on different
> > usb_request structures, and they can not, at any time, get called on the
> > _same_ request?
> 
> usb_requests are allocated for a specific endpoint and USB Device
> Controller (UDC) drivers refuse to queue requests allocated for epX to
> epY, so this really can never happen.

Good, thanks!

> My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
> adds extra overhead which makes the problem much, much less likely to
> happen. Does that sound plausible to you?

I did consider that, but I've not sufficiently grokked the code to rule
out actual fail. So let me stare at this a bit more.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Felipe Balbi]

Hi,

Peter Zijlstra  writes:
> On Mon, Sep 05, 2016 at 11:29:26AM -0400, Alan Stern wrote:
>> On Mon, 5 Sep 2016, Peter Zijlstra wrote:
>> 
>> > > Actually, seeing it written out like this, one realizes that it really 
>> > > ought to be:
>> > > 
>> > >  CPU 0   CPU 1
>> > > --
>> > > Start DMAHandle DMA-complete irq
>> > > Sleep until bh->statesmp_mb()
>> > >  set bh->state
>> > >  Wake up CPU 0
>> > >  smp_mb()
>> > >  Compute rc based on contents of the DMA buffer
>> > > 
>> > > (Bear in mind also that on some platforms, the I/O operation is carried 
>> > > out by PIO rather than DMA.)
>
>> > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
>> > Its only defined to do CPU/CPU interactions.
>> 
>> Suppose the DMA master finishes filling up an input buffer and issues a
>> completion irq to CPU1.  Presumably the data would then be visible to
>> CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
>> before setting bh->state and waking up CPU0, and if CPU0 executes
>> smp_mb after testing bh->state and before reading the data buffer,
>> wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
>> never did go to sleep?
>
> Couple of notes here; I would expect the DMA master to make its stores
> _globally_ visible on 'completion'. Because I'm not sure our smp_mb()
> would help make it globally visible, since its only defined on CPU/CPU
> interactions, not external.
>
> Note that for example ARM has the distinction where smp_mb() uses
> DMB-ISH barrier, dma_[rw]mb() uses DMB-OSH{LD,ST} and mb() uses DSB-SY.
>
> The ISH domain is the Inner-SHarable (IIRC) and only includes CPUs. The
> OSH is Outer-SHarable and adds external agents like DMA (also includes
> CPUs). The DSB-SY thing is even heavier and syncs world or something; I
> always forget these details.
>
>> Or would something more be needed?
>
> The thing is, this is x86 (TSO). Most everything is globally
> ordered/visible and full barriers.
>
> The only reorder allowed by TSO is a later read can happen before a
> prior store. That is, only:
>
>   X = 1;
>   smp_mb();
>   r = Y;
>
> actually needs a full barrier. All the other variants are no-ops.
>
> Note that x86's dma_[rw]mb() are no-ops (like the regular smp_[rw]mb()).
> Which seems to imply DMA is coherent and globally visible.
>
>> > I would very much expect the device IO stuff to order things for us in
>> > this case. "Start DMA" should very much include sufficient fences to
>> > ensure the data under DMA is visible to the DMA engine, this would very
>> > much include things like flushing store buffers and maybe even writeback
>> > caches, depending on platform needs.
>> > 
>> > At the same time, I would expect "Handle DMA-complete irq", even if it
>> > were done with a PIO polling loop, to guarantee ordering against later
>> > operations such that 'complete' really means that.
>> 
>> That's what I would expect too.
>> 
>> Back in the original email thread where the problem was first reported, 
>> Felipe says that the problem appears to be something else.  Here's what 
>> it looks like now, in schematic form:
>> 
>>  CPU0
>>  
>>  get_next_command():
>>while (bh->state != BUF_STATE_EMPTY)
>>  sleep_thread();
>>start an input request for bh
>>while (bh->state != BUF_STATE_FULL)
>>  sleep_thread();
>> 
>> As mentioned above, the input involves DMA and is terminated by an irq.
>> The request's completion handler is bulk_out_complete():
>> 
>>  CPU1
>>  
>>  bulk_out_complete():
>>bh->state = BUF_STATE_FULL;
>>wakeup_thread();
>> 
>> According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
>> value different from BUF_STATE_FULL.  So it goes back to sleep again 
>> and doesn't make any forward progress.
>> 
>> It's possible that something else is changing bh->state when it 
>> shouldn't.  But if this were the explanation, why would Felipe see that 
>> the problem goes away when he changes the memory barriers in 
>> sleep_thread() and wakeup_thread() to smp_mb()?
>
> Good question, let me stare at the code more.
>
> Could you confirm that bulk_{in,out}_complete() work on different
> usb_request structures, and they can not, at any time, get called on the
> _same_ request?

usb_requests are allocated for a specific endpoint and USB Device
Controller (UDC) drivers refuse to queue requests allocated for epX to
epY, so this really can never happen.

My fear now, however, is that changing smp_[rw]mb() to smp_mb() just
adds extra overhead which makes the problem much, much less likely to
happen. Does that sound plausible to you?

-- 
balbi


signature.asc
Description: PGP signature

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 11:29:26AM -0400, Alan Stern wrote:
> On Mon, 5 Sep 2016, Peter Zijlstra wrote:
> 
> > > Actually, seeing it written out like this, one realizes that it really 
> > > ought to be:
> > > 
> > >   CPU 0   CPU 1
> > > - -
> > > Start DMA Handle DMA-complete irq
> > > Sleep until bh->state smp_mb()
> > >   set bh->state
> > >   Wake up CPU 0
> > >   smp_mb()
> > >   Compute rc based on contents of the DMA buffer
> > > 
> > > (Bear in mind also that on some platforms, the I/O operation is carried 
> > > out by PIO rather than DMA.)

> > Also, smp_mb() doesn't necessarily interact with MMIO/DMA at all IIRC.
> > Its only defined to do CPU/CPU interactions.
> 
> Suppose the DMA master finishes filling up an input buffer and issues a
> completion irq to CPU1.  Presumably the data would then be visible to
> CPU1 if the interrupt handler looked at it.  So if CPU1 executes smp_mb
> before setting bh->state and waking up CPU0, and if CPU0 executes
> smp_mb after testing bh->state and before reading the data buffer,
> wouldn't CPU0 then see the correct data in the buffer?  Even if CPU0 
> never did go to sleep?

Couple of notes here; I would expect the DMA master to make its stores
_globally_ visible on 'completion'. Because I'm not sure our smp_mb()
would help make it globally visible, since its only defined on CPU/CPU
interactions, not external.

Note that for example ARM has the distinction where smp_mb() uses
DMB-ISH barrier, dma_[rw]mb() uses DMB-OSH{LD,ST} and mb() uses DSB-SY.

The ISH domain is the Inner-SHarable (IIRC) and only includes CPUs. The
OSH is Outer-SHarable and adds external agents like DMA (also includes
CPUs). The DSB-SY thing is even heavier and syncs world or something; I
always forget these details.

> Or would something more be needed?

The thing is, this is x86 (TSO). Most everything is globally
ordered/visible and full barriers.

The only reorder allowed by TSO is a later read can happen before a
prior store. That is, only:

X = 1;
smp_mb();
r = Y;

actually needs a full barrier. All the other variants are no-ops.

Note that x86's dma_[rw]mb() are no-ops (like the regular smp_[rw]mb()).
Which seems to imply DMA is coherent and globally visible.

> > I would very much expect the device IO stuff to order things for us in
> > this case. "Start DMA" should very much include sufficient fences to
> > ensure the data under DMA is visible to the DMA engine, this would very
> > much include things like flushing store buffers and maybe even writeback
> > caches, depending on platform needs.
> > 
> > At the same time, I would expect "Handle DMA-complete irq", even if it
> > were done with a PIO polling loop, to guarantee ordering against later
> > operations such that 'complete' really means that.
> 
> That's what I would expect too.
> 
> Back in the original email thread where the problem was first reported, 
> Felipe says that the problem appears to be something else.  Here's what 
> it looks like now, in schematic form:
> 
>   CPU0
>   
>   get_next_command():
> while (bh->state != BUF_STATE_EMPTY)
>   sleep_thread();
> start an input request for bh
> while (bh->state != BUF_STATE_FULL)
>   sleep_thread();
> 
> As mentioned above, the input involves DMA and is terminated by an irq.
> The request's completion handler is bulk_out_complete():
> 
>   CPU1
>   
>   bulk_out_complete():
> bh->state = BUF_STATE_FULL;
> wakeup_thread();
> 
> According to Felipe, when CPU0 wakes up and checks bh->state, it sees a 
> value different from BUF_STATE_FULL.  So it goes back to sleep again 
> and doesn't make any forward progress.
> 
> It's possible that something else is changing bh->state when it 
> shouldn't.  But if this were the explanation, why would Felipe see that 
> the problem goes away when he changes the memory barriers in 
> sleep_thread() and wakeup_thread() to smp_mb()?

Good question, let me stare at the code more.

Could you confirm that bulk_{in,out}_complete() work on different
usb_request structures, and they can not, at any time, get called on the
_same_ request?


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Memory barrier needed with wake_up_process()?

[Peter Zijlstra]

On Mon, Sep 05, 2016 at 10:43:11AM +0100, Will Deacon wrote:
> On Sat, Sep 03, 2016 at 12:16:29AM +0200, Peter Zijlstra wrote:

> > Forgot to Cc Will. Will, does ARM64 need to make smp_mb__before_spinlock
> > smp_mb() too?
> 
> Yes, probably. Just to confirm, the test is something like:
> 
> 
> CPU0
> 
> 
> Wx=1
> smp_mb__before_spinlock()
> LOCK(y)
> Rz=0
> 
> CPU1
> 
> 
> Wz=1
> smp_mb()
> Rx=0
> 
> 
> and that should be forbidden?

Indeed.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
>> 
>> this only solves the problem for DT devices. Legacy devices and
>> PCI-based systems will still suffer from the same problem. At least for
>> dwc3, I will only be taking patches that solve the problem for all
>> users, not a subset of them.
>
> I don't think legacy devices are a worry, because they wouldn't
> have this problem. For the PCI case, you are right that it cannot
> work, in particular for machines that have complex IOMMU setup.
>
> Some architectures (at least arm64 and sparc) check the bus_type of
> a device in order to find the correct set of dma_map_ops for that
> device, so there is no real way to handle this as long as you
> pass a platform_device into an API that expects a pci_device.

Then I guess we're left with adding a "struct device *dma_dev" to struct
dwc3 and trying to figure out if we should use parent or self. Does
anybody see any problems with that?

Note, we would NOT be passing device pointers are platform_data, we
would have dwc3.ko figure out if it should use self or its parent device
for dma.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Tuesday, September 6, 2016 9:40:19 AM CEST Felipe Balbi wrote:
> 
> this only solves the problem for DT devices. Legacy devices and
> PCI-based systems will still suffer from the same problem. At least for
> dwc3, I will only be taking patches that solve the problem for all
> users, not a subset of them.

I don't think legacy devices are a worry, because they wouldn't
have this problem. For the PCI case, you are right that it cannot
work, in particular for machines that have complex IOMMU setup.

Some architectures (at least arm64 and sparc) check the bus_type of
a device in order to find the correct set of dma_map_ops for that
device, so there is no real way to handle this as long as you
pass a platform_device into an API that expects a pci_device.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Logitech Precision Gamepad shows axis activity upon connection

[Eddie Stanley]

SUMMARY:
Logitech Precision Gamepad shows axis activity upon connection

DESCRIPTION:
When I connect my Logitech Precision Gamepad (or the machine starts up
with it connected) it exhibits activity on both axes until I press a
button/press the d-pad.

eddie@codey:~$ cat /proc/version
Linux version 4.8.0-040800rc5-generic (kernel@tangerine) (gcc version
6.2.0 20160830 (Ubuntu 6.2.0-2ubuntu11) ) #201609041832 SMP Sun Sep 4
22:34:01 UTC 2016

Output from jstest i.e. jstest /dev/input/js0

UPON PLUGGING USB CONNECTOR IN:
=
Driver version is 2.1.0.
Joystick (Logitech Logitech(R) Precision(TM) Gamepad) has 2 axes (X, Y)
and 10 buttons (Trigger, ThumbBtn, ThumbBtn2, TopBtn, TopBtn2,
PinkieBtn, BaseBtn, BaseBtn2, BaseBtn3, BaseBtn4).
Testing ... (interrupt to exit)
Axes: 0:-32767 1:-32767 Buttons: 0:off 1:off 2:off 3:off 4:off 5:off
6:off 7:off 8:off 9:off

AFTER PRESSING ANY BUTTON OR THE D-PAD:
===
Driver version is 2.1.0.
Joystick (Logitech Logitech(R) Precision(TM) Gamepad) has 2 axes (X, Y)
and 10 buttons (Trigger, ThumbBtn, ThumbBtn2, TopBtn, TopBtn2,
PinkieBtn, BaseBtn, BaseBtn2, BaseBtn3, BaseBtn4).
Testing ... (interrupt to exit)
Axes: 0: 0 1: 0 Buttons: 0:off 1:off 2:off 3:off 4:off 5:off 6:off 7:off
8:off 9:off

EXPECTED BEHAVIOUR:
===
Both axes (0 & 1) should be "at rest" (i.e. 0, not +/- 32767) when the
gamepad is initially plugged in.

Note - I'm not sure whether the d-pad should be classified as a pair of
axes or a pair of hats - it is a digital d-pad though, *not* analog.

ENVIRONMENT:

eddie@codey:~$ lsb_release -rd
Description: Ubuntu 16.04.1 LTS
Release: 16.04

eddie@codey:/usr/src$ cat /proc/cpuinfo
processor : 0
vendor_id : GenuineIntel
cpu family : 6
model : 76
model name : Intel(R) Celeron(R) CPU  N3050  @ 1.60GHz
stepping : 3
microcode : 0x363
cpu MHz : 479.980
cache size : 1024 KB
physical id : 0
siblings : 2
core id : 0
cpu cores : 2
apicid : 0
initial apicid : 0
fpu : yes
fpu_exception : yes
cpuid level : 11
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology
nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx
est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 movbe popcnt
tsc_deadline_timer aes rdrand lahf_lm 3dnowprefetch epb tpr_shadow vnmi
flexpriority ept vpid tsc_adjust smep erms dtherm ida arat
bugs :
bogomips : 3200.00
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:

processor : 1
vendor_id : GenuineIntel
cpu family : model : 76
model name : Intel(R) Celeron(R) CPU  N3050  @ 1.60GHz
stepping : 3
microcode : 0x363
cpu MHz : 479.980
cache size : 1024 KB
physical id : 0
siblings : 2
core id : 2
cpu cores : 2
apicid : 4
initial apicid : 4
fpu : yes
fpu_exception : yes
cpuid level : 11
wp : yes
flags : fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov
pat pse36 clflush dts acpi mmx fxsr sse sse2 ss ht tm pbe syscall nx
rdtscp lm constant_tsc arch_perfmon pebs bts rep_good nopl xtopology
nonstop_tsc aperfmperf eagerfpu pni pclmulqdq dtes64 monitor ds_cpl vmx
est tm2 ssse3 cx16 xtpr pdcm sse4_1 sse4_2 movbe popcnt
tsc_deadline_timer aes rdrand lahf_lm 3dnowprefetch epb tpr_shadow vnmi
flexpriority ept vpid tsc_adjust smep erms dtherm ida arat
bugs :
bogomips : 3203.34
clflush size : 64
cache_alignment : 64
address sizes : 36 bits physical, 48 bits virtual
power management:

eddie@codey:/usr/src$ cat /proc/modules
hid_generic 16384 0 - Live 0x
rfcomm 77824 0 - Live 0x
ir_xmp_decoder 16384 0 - Live 0x
ir_mce_kbd_decoder 16384 0 - Live 0x
ir_sharp_decoder 16384 0 - Live 0x
ir_sanyo_decoder 16384 0 - Live 0x
ir_sony_decoder 16384 0 - Live 0x
ir_jvc_decoder 16384 0 - Live 0x
ir_nec_decoder 16384 0 - Live 0x
ir_rc5_decoder 16384 0 - Live 0x
bnep 20480 2 - Live 0x
ir_rc6_decoder 16384 0 - Live 0x
intel_rapl 20480 0 - Live 0x
intel_powerclamp 16384 0 - Live 0x
coretemp 16384 0 - Live 0x
kvm_intel 192512 0 - Live 0x
kvm 593920 1 kvm_intel, Live 0x
irqbypass 16384 1 kvm, Live 0x
crct10dif_pclmul 16384 0 - Live 0x
crc32_pclmul 16384 0 - Live 0x
ghash_clmulni_intel 16384 0 - Live 0x
aesni_intel 167936 0 - Live 0x
aes_x86_64 20480 1 aesni_intel, Live 0x
lrw 16384 1 aesni_intel, Live 0x
gf128mul 16384 1 lrw, Live 0x
glue_helper 16384 1 aesni_intel, Live 0x
ablk_helper 16384 1 aesni_intel, Live 0x
cryptd 24576 3 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Tuesday, September 6, 2016 2:35:29 PM CEST Peter Chen wrote:
> On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote:
> > On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote:

> > 
> > Most of these are probably never used with any nonstandard
> > DMA settings (IOMMU, cache coherency, offset, ...).
> > 
> > One thing we could possibly do is to go through these and
> > replace the hardcoded dma mask setup with of_dma_configure()
> > in all cases in which we actually use DT for probing, which
> > should cover the interesting cases.
> > 
> 
> One case I am going to work is to let USB chipidea driver support iommu,
> the chipidea core device is no of_node, and created by
> platform_add_device on the runtime. Using of_dma_configure with parent
> of_node is a solution from my point, like [1].
> 
> https://lkml.org/lkml/2016/2/22/7

Right, that should make it work with iommu as well. However, it does
not solve the other issue I mentioned above, with boards that have
USB devices hardwired to a chipidea host controller that need
configuration from DT. For that, we still need to come up with another
way to associate the DT hierarchy in the host bridge node with
the Linux platform_device.

Arnd
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 0/8] power: add power sequence library

[Vaibhav Hiremath]

a

On Friday 02 September 2016 06:40 AM, Peter Chen wrote:

On Wed, Aug 31, 2016 at 10:28:20PM +0530, Vaibhav Hiremath wrote:


On Wednesday 31 August 2016 03:22 PM, Peter Chen wrote:

On Wed, Aug 31, 2016 at 01:46:30PM +0530, Vaibhav Hiremath wrote:

On Monday 29 August 2016 04:40 PM, Peter Chen wrote:

On Wed, Aug 24, 2016 at 04:53:35PM +0800, Peter Chen wrote:

On Tue, Aug 23, 2016 at 04:02:48PM +0530, Vaibhav Hiremath wrote:

veOn Monday 15 August 2016 02:43 PM, Peter Chen wrote:

Hi all,

This is a follow-up for my last power sequence framework patch set [1].
According to Rob Herring and Ulf Hansson's comments[2], I use a generic
power sequence library for parsing the power sequence elements on DT,
and implement generic power sequence on library. The host driver
can allocate power sequence instance, and calls pwrseq APIs accordingly.

In future, if there are special power sequence requirements, the special
power sequence library can be created.

This patch set is tested on i.mx6 sabresx evk using a dts change, I use
two hot-plug devices to simulate this use case, the related binding
change is updated at patch [1/6], The udoo board changes were tested
using my last power sequence patch set.[3]

Except for hard-wired MMC and USB devices, I find the USB ULPI PHY also
need to power on itself before it can be found by ULPI bus.

[1]http://www.spinics.net/lists/linux-usb/msg142755.html
[2]http://www.spinics.net/lists/linux-usb/msg143106.html
[3]http://www.spinics.net/lists/linux-usb/msg142815.html

(Please ignore my response on V2)

Sorry being so late in the discussion...

If I am not missing anything, then I am afraid to say that the
generic library
implementation in this patch series is not going to solve many of
the custom
requirement of power on, off, etc...
I know you mentioned about adding another library when we come
across such platforms, but should we not keep provision (or easy
hooks/path)
to enable that ?

Let me bring in the use case I am dealing with,


   Host
|
V
USB port

|
V
   USB HUB device (May need custom on/off seq)
|
V
   =
  | |
  V V
  Device-1   Device-2
(Needs special power   (Needs special power
  on/off sequence.   on/off sequence.
  Also may need custom   Also, may need custom
  sequence for   sequence for
  suspend/resume)suspend/resume)


Note: Both Devices are connected to HUB via HSIC and may differ
   in terms of functionality, features they support.

In the above case, both Device-1 and Device-2, need separate
power on/off sequence. So generic library currently we have in this
patch series is not going to satisfy the need here.

I looked at all 6 revisions of this patch-series, went through the
review comments, and looked at MMC power sequence code;
what I can say here is, we need something similar to
MMC power sequence here, where every device can have its own
power sequence (if needed).

I know Rob is not in favor of creating platform device for
this, and I understand his comment.
If not platform device, but atleast we need mechanism to
connect each device back to its of_node and its respective
driver/library fns. For example, the Devices may support different
boot modes, and platform driver needs to make sure that
the right sequence is followed for booting.

Peter, My apologies for taking you back again on this series.
I am OK, if you wish to address this in incremental addition,
but my point is, we know that the current generic way is not
enough for us, so I think we should try to fix it in initial phase only.


Rob, it seems generic power sequence can't cover all cases.
Without information from DT, we can't know which power sequence
for which device.


Vaibhav, do you agree that I create pwrseq library list using postcore_initcall
for each library, and choose pwrseq library according to compatible
string first, if there is no compatible string for this library, just
use generic pwrseq library.


Lets hear from MMC folks. Ulf, do you agree on approach
for pwr seq ??


With above approach, I kind of agree on it, but I have couple
of comments,

  - How DTS looks like now ?? Can you put example here ?

The dts is the same with current version.

How would consumer driver get the power sequence ?
You must point to right power sequence driver.
For example, in the above example, Device-1, should
get handle to pwrseq-1, and Device-2 to pwrseq-2.

According to compatible string at device's of_node, we will have a list

Re: Problem(s) with Seagate Backup Plus 4TB portable drive in UAS mode

[Oliver Neukum]

On Tue, 2016-08-30 at 08:14 -0400, Robert Krawitz wrote:
> > This is the typical error handling of UAS, which works in your case,
> > albeit with a performance penalty. But the actual cause of kicking
> in
> > the error handler is missing in your log. You cut too much at the
> > beginning.
> 
> Here's an excerpt from the full log, from the very first message after
> I physically plugged in the drive:

Please use the 
scsi_logging_level
tool to capture the error the drive reports.

Regards
Oliver


--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] usb: rename bU2DevExitLat to wU2DevExitLat

[Felipe Balbi]

Hans Petter Selasky  writes:

> On 09/06/16 08:44, Felipe Balbi wrote:
>> Hi,
>>
>> Felipe Balbi  writes:
>>> > According to USB 3.1 Specification, that field is 2
>>> > bytes wide and is named with a 'w' prefix, not 'b'.
>>> >
>>> > Just to make grepping in the spec easier, let's
>>> > match the name.
>>> >
>>> > Signed-off-by: Felipe Balbi 
>> I'll wait for another week or so, if nobody has any objections to this
>> series, I plan on sending it to Greg on my pull request.
>
> FYI: Affects Linux's libusb header files too. They have the same 
> mis-spelling.

Oh darn it. I'm touching . I'm not sure we can go
ahead with this. At least not that easily. I wonder if below would work:

diff --git a/include/uapi/linux/usb/ch9.h b/include/uapi/linux/usb/ch9.h
index a8acc24765fe..87ad22e81a8b 100644
--- a/include/uapi/linux/usb/ch9.h
+++ b/include/uapi/linux/usb/ch9.h
@@ -889,7 +889,8 @@ struct usb_ss_cap_descriptor {  /* Link Power 
Management */
 #define USB_5GBPS_OPERATION(1 << 3) /* Operation at 5Gbps */
__u8  bFunctionalitySupport;
__u8  bU1devExitLat;
-   __le16 bU2DevExitLat;
+#define bU2DevExitLat wU2DevExitLat
+   __le16 wU2DevExitLat;
 } __attribute__((packed));
 
 #define USB_DT_USB_SS_CAP_SIZE 10

Then we shouldn't break anything. A quick build test shows that it
builds just fine.

-- 
balbi


signature.asc
Description: PGP signature

Re: [uas] ORICO 2588US3 USB HDD enclosure goes offline after idle

[Oliver Neukum]

On Mon, 2016-09-05 at 14:31 +0300, Dmitry Gutov wrote:
> On 05.09.2016 14:19, Oliver Neukum wrote:
> 
> > You add the device to the kernel quirks list. But if
> > you don't use autosuspend, the point is moot.
> 
> The device does get spinned down (after not using it for a while, I have 
> to wait before it responds for the first time), but not sure which 
> method is used.

Well, it is supposed to spin down actually. The problem is
not spinning up again. I see three possibilities

1. Your drive is defective with respect to LPM.
   The attached patch will test that
2. It claims to not need START_STOP_UNIT but needs it
3. It reports some wierd error

> > Do you use LPM, as described in
> > Documentation/usb/power-management.txt
> > ?
> 
> Is this what you're looking for?
> 
> $ cat /sys/bus/usb/devices/2-1/power/usb3_hardware_lpm_u1
> enabled
> 
> $ cat /sys/bus/usb/devices/2-1/power/usb3_hardware_lpm_u2
> enabled

Could you apply the attached patch, enable SCSI logging (with
scsi_logging_level) and retest?

Regards
Oliver



From c3e03c4946727a73de1777372f9f626b083cce1d Mon Sep 17 00:00:00 2001
From: Oliver Neukum 
Date: Tue, 6 Sep 2016 11:52:33 +0200
Subject: [PATCH] uas: experimentally NO_LPM quirk

Add USB_QUIRK_NO_LPM for ORICO 2588US3 USB HDD

Signed-off-by: Oliver Neukum 
---
 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 d2e50a2..81e1d33 100644
--- a/drivers/usb/core/quirks.c
+++ b/drivers/usb/core/quirks.c
@@ -211,6 +211,9 @@ static const struct usb_device_id usb_quirk_list[] = {
 	/* Blackmagic Design UltraStudio SDI */
 	{ USB_DEVICE(0x1edb, 0xbd4f), .driver_info = USB_QUIRK_NO_LPM },
 
+	/* ORICO 2588US3 USB HDD */
+	{ USB_DEVICE(0x357d, 0x7788), .driver_info = USB_QUIRK_NO_LPM },
+
 	/* INTEL VALUE SSD */
 	{ USB_DEVICE(0x8086, 0xf1a5), .driver_info = USB_QUIRK_RESET_RESUME },
 
-- 
2.1.4

Re: [PATCH 1/3] usb: rename bU2DevExitLat to wU2DevExitLat

[Hans Petter Selasky]

On 09/06/16 08:44, Felipe Balbi wrote:

Hi,

Felipe Balbi  writes:

> According to USB 3.1 Specification, that field is 2
> bytes wide and is named with a 'w' prefix, not 'b'.
>
> Just to make grepping in the spec easier, let's
> match the name.
>
> Signed-off-by: Felipe Balbi 

I'll wait for another week or so, if nobody has any objections to this
series, I plan on sending it to Greg on my pull request.


FYI: Affects Linux's libusb header files too. They have the same 
mis-spelling.


--HPS
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: xHCI problem? [was Re: Erratic USB device behavior and device loss]

[Ulf Hansson]

On 5 September 2016 at 17:58, Alan Stern  wrote:
> On Mon, 5 Sep 2016, Ritesh Raj Sarraf wrote:
>
>> -BEGIN PGP SIGNED MESSAGE-
>> Hash: SHA512
>>
>> On Sun, 2016-09-04 at 15:46 -0400, Alan Stern wrote:
>> >
>> > This is not the problem I was discussing with Ulf.  The problem was why
>> > the device kept going into and out of runtime suspend every three
>> > seconds.  The kernel log above does not say whether this was happening.
>> > One way to tell is to look at a usbmon trace (like we did before).
>>
>> https://people.debian.org/~rrs/tmp/usbmon.txt.gz
>>
>> This should have the logs you asked for, running on 4.8-rc4.
>
> Confirmed, the runtime suspends and resumes are still happening.
>
> Ulf, any insights?

Alan, Ritesh,

Yes, I am starting to understand more about what goes on here.
Although I need help to test as I don't have the HW.
As you already guessed, I suspect the problem is within the runtime PM
deployment in the drivers/mmc/host/rtsx_usb_sdmmc.c.

Let me start by first give you some background to how the mmc core
deals with runtime PM.

*)
The mmc core manages most of the calls to the pm_runtime_get|put*()
and pm_runtime_mark_last_busy() for the mmc host device. The gets/puts
are done when the core is about to access the mmc host device, via the
mmc host ops driver interface. You may search for calls to the
mmc_claim|release_host() functions to find out when the gets/puts are
done.

**)
The mmc core have also deployed runtime PM for the mmc *card* device
and which has the runtime PM autosuspend feature enabled with a 3s
default timeout. One important point is that the mmc card device has
the mmc host device assigned as being its parent device. I guessing
the reason to why you are encountering strange 3s intervals of runtime
PM suspend/resume is related to this.

Now, in this case, the rtsx_usb_sdmmc driver seems to need a bit of
special runtime PM deployment, as the calls to pm_runtime_get|put*()
also controls the power to the usb device and thus also the power to
the card. I am guessing that's done via the usb device being assigned
as parent for the mmc host's platform device!?

By reviewing the code of the rtsx_usb_sdmmc driver, particularly how
it calls pm_runtime_get|put() I am guessing those calls may not be
properly deployed. Perhaps rtsx_usb_sdmmc should convert to use the
usb_autopm_put|get_interface() and friends, although I didn't want to
make that change at this point so instead I have cooked a patch that
might fixes the behaviour.

Ritesh, can you please try it out to see what happens?

---
 drivers/mmc/host/rtsx_usb_sdmmc.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/mmc/host/rtsx_usb_sdmmc.c
b/drivers/mmc/host/rtsx_usb_sdmmc.c
index 6c71fc9..3d6fe51 100644
--- a/drivers/mmc/host/rtsx_usb_sdmmc.c
+++ b/drivers/mmc/host/rtsx_usb_sdmmc.c
@@ -1138,11 +1138,6 @@ static void sdmmc_set_ios(struct mmc_host *mmc,
struct mmc_ios *ios)
dev_dbg(sdmmc_dev(host), "%s\n", __func__);
mutex_lock(>dev_mutex);

-   if (rtsx_usb_card_exclusive_check(ucr, RTSX_USB_SD_CARD)) {
-   mutex_unlock(>dev_mutex);
-   return;
-   }
-
sd_set_power_mode(host, ios->power_mode);
sd_set_bus_width(host, ios->bus_width);
sd_set_timing(host, ios->timing, >ddr_mode);
@@ -1336,7 +1331,7 @@ static void rtsx_usb_init_host(struct
rtsx_usb_sdmmc *host)
MMC_CAP_MMC_HIGHSPEED | MMC_CAP_BUS_WIDTH_TEST |
MMC_CAP_UHS_SDR12 | MMC_CAP_UHS_SDR25 | MMC_CAP_UHS_SDR50 |
MMC_CAP_NEEDS_POLL;
-   mmc->caps2 = MMC_CAP2_NO_PRESCAN_POWERUP | MMC_CAP2_FULL_PWR_CYCLE;
+   mmc->caps2 = MMC_CAP2_FULL_PWR_CYCLE;

mmc->max_current_330 = 400;
mmc->max_current_180 = 800;
-- 

Kind regards
Uffe
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: generic: request regulator optionally

[Mark Brown]

On Tue, Sep 06, 2016 at 10:45:19AM +0300, Felipe Balbi wrote:
> Stefan Agner  writes:

> > According to the device tree bindings the vcc-supply is optional.

This is nonsense unless the device can work without this supply.  Given
that the supply is called VCC that doesn't seem entirely likely.

> > +   nop->vcc = devm_regulator_get_optional(dev, "vcc");
> > if (IS_ERR(nop->vcc)) {
> > dev_dbg(dev, "Error getting vcc regulator: %ld\n",
> > PTR_ERR(nop->vcc));
> > -   if (needs_vcc)
> > -   return -EPROBE_DEFER;
> > +   if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
> > +   return PTR_ERR(nop->vcc);

> does this look okay from a regulator API perspective?

That's how to use _get_optional() but it's really unusual that you
should be using _get_optional().


signature.asc
Description: PGP signature

[PATCH] lan78xx: mark symbols static where possible

[Baoyou Xie]

We get a few warnings when building kernel with W=1:
drivers/net/usb/lan78xx.c:1182:6: warning: no previous prototype for 
'lan78xx_defer_kevent' [-Wmissing-prototypes]
drivers/net/usb/lan78xx.c:1409:5: warning: no previous prototype for 
'lan78xx_nway_reset' [-Wmissing-prototypes]
drivers/net/usb/lan78xx.c:2000:5: warning: no previous prototype for 
'lan78xx_set_mac_addr' [-Wmissing-prototypes]


In fact, these functions are only used in the file in which they are
declared and don't need a declaration, but can be made static.
so this patch marks these functions with 'static'.

Signed-off-by: Baoyou Xie 
---
 drivers/net/usb/lan78xx.c | 24 +---
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 6a9d474..8c467a5 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1179,7 +1179,7 @@ static int lan78xx_link_reset(struct lan78xx_net *dev)
  * NOTE:  annoying asymmetry:  if it's active, schedule_work() fails,
  * but tasklet_schedule() doesn't. hope the failure is rare.
  */
-void lan78xx_defer_kevent(struct lan78xx_net *dev, int work)
+static void lan78xx_defer_kevent(struct lan78xx_net *dev, int work)
 {
set_bit(work, >flags);
if (!schedule_delayed_work(>wq, 0))
@@ -1406,7 +1406,7 @@ static u32 lan78xx_get_link(struct net_device *net)
return net->phydev->link;
 }
 
-int lan78xx_nway_reset(struct net_device *net)
+static int lan78xx_nway_reset(struct net_device *net)
 {
return phy_start_aneg(net->phydev);
 }
@@ -1997,7 +1997,7 @@ static int lan78xx_change_mtu(struct net_device *netdev, 
int new_mtu)
return 0;
 }
 
-int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
+static int lan78xx_set_mac_addr(struct net_device *netdev, void *p)
 {
struct lan78xx_net *dev = netdev_priv(netdev);
struct sockaddr *addr = p;
@@ -2371,7 +2371,7 @@ static void lan78xx_terminate_urbs(struct lan78xx_net 
*dev)
remove_wait_queue(_wakeup, );
 }
 
-int lan78xx_stop(struct net_device *net)
+static int lan78xx_stop(struct net_device *net)
 {
struct lan78xx_net  *dev = netdev_priv(net);
 
@@ -2533,7 +2533,8 @@ static void lan78xx_queue_skb(struct sk_buff_head *list,
entry->state = state;
 }
 
-netdev_tx_t lan78xx_start_xmit(struct sk_buff *skb, struct net_device *net)
+static netdev_tx_t
+lan78xx_start_xmit(struct sk_buff *skb, struct net_device *net)
 {
struct lan78xx_net *dev = netdev_priv(net);
struct sk_buff *skb2 = NULL;
@@ -2562,7 +2563,8 @@ netdev_tx_t lan78xx_start_xmit(struct sk_buff *skb, 
struct net_device *net)
return NETDEV_TX_OK;
 }
 
-int lan78xx_get_endpoints(struct lan78xx_net *dev, struct usb_interface *intf)
+static int
+lan78xx_get_endpoints(struct lan78xx_net *dev, struct usb_interface *intf)
 {
int tmp;
struct usb_host_interface *alt = NULL;
@@ -2700,7 +2702,7 @@ static void lan78xx_rx_csum_offload(struct lan78xx_net 
*dev,
}
 }
 
-void lan78xx_skb_return(struct lan78xx_net *dev, struct sk_buff *skb)
+static void lan78xx_skb_return(struct lan78xx_net *dev, struct sk_buff *skb)
 {
int status;
 
@@ -3285,7 +3287,7 @@ static void lan78xx_disconnect(struct usb_interface *intf)
usb_put_dev(udev);
 }
 
-void lan78xx_tx_timeout(struct net_device *net)
+static void lan78xx_tx_timeout(struct net_device *net)
 {
struct lan78xx_net *dev = netdev_priv(net);
 
@@ -3605,7 +3607,7 @@ static int lan78xx_set_suspend(struct lan78xx_net *dev, 
u32 wol)
return 0;
 }
 
-int lan78xx_suspend(struct usb_interface *intf, pm_message_t message)
+static int lan78xx_suspend(struct usb_interface *intf, pm_message_t message)
 {
struct lan78xx_net *dev = usb_get_intfdata(intf);
struct lan78xx_priv *pdata = (struct lan78xx_priv *)(dev->data[0]);
@@ -3701,7 +3703,7 @@ out:
return ret;
 }
 
-int lan78xx_resume(struct usb_interface *intf)
+static int lan78xx_resume(struct usb_interface *intf)
 {
struct lan78xx_net *dev = usb_get_intfdata(intf);
struct sk_buff *skb;
@@ -3768,7 +3770,7 @@ int lan78xx_resume(struct usb_interface *intf)
return 0;
 }
 
-int lan78xx_reset_resume(struct usb_interface *intf)
+static int lan78xx_reset_resume(struct usb_interface *intf)
 {
struct lan78xx_net *dev = usb_get_intfdata(intf);
 
-- 
2.7.4

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: phy: generic: request regulator optionally

[Felipe Balbi]

Mark,

Stefan Agner  writes:
> According to the device tree bindings the vcc-supply is optional.
> So far the driver did request the regulator using devm_regulator_get
> which creates a dummy regulator for convenience. Since we can have
> the supply unconnected, we should make use of the optional variant
> of the regulator call which does not return a dummy regulator but
> -ENODEV. The driver already has checks in case the regulator is an
> error pointer.
>
> Note that with this change the behavior is slightly different in
> case devm_regulator_get_optional returns -EPROBE_DEFER: The driver
> returns -EPROBE_DEFER even if needs_vcc is set false. This is the
> correct behavior, since even if the regulator is optional, it might
> get initialized later...
>
> Signed-off-by: Stefan Agner 
> ---
> This gets rid of warnings such as this (seen on i.MX 7):
> 3080.aips-bus:usbphynop1 supply vcc not found, using dummy regulator
>
> --
> Stefan
>
>  drivers/usb/phy/phy-generic.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/phy/phy-generic.c b/drivers/usb/phy/phy-generic.c
> index 980c9de..38ceadc 100644
> --- a/drivers/usb/phy/phy-generic.c
> +++ b/drivers/usb/phy/phy-generic.c
> @@ -275,12 +275,12 @@ int usb_phy_gen_create_phy(struct device *dev, struct 
> usb_phy_generic *nop,
>   }
>   }
>  
> - nop->vcc = devm_regulator_get(dev, "vcc");
> + nop->vcc = devm_regulator_get_optional(dev, "vcc");
>   if (IS_ERR(nop->vcc)) {
>   dev_dbg(dev, "Error getting vcc regulator: %ld\n",
>   PTR_ERR(nop->vcc));
> - if (needs_vcc)
> - return -EPROBE_DEFER;
> + if (needs_vcc || PTR_ERR(nop->vcc) == -EPROBE_DEFER)
> + return PTR_ERR(nop->vcc);

does this look okay from a regulator API perspective?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v16 0/4] Introduce usb charger framework to deal with the usb gadget power negotation

[Felipe Balbi]

Hi,

NeilBrown  writes:
> Firstly, you have made the current limit associated with each cable type
> configurable (__usb_charger_set_cur_limit_by_type).   This is nonsense.
> The standard (e.g. BC-1.2) declares what the current limits are.  There
> is no reason for those not to be hard coded.

I had raised the same concern WRT configuration current limits.

> Secondly, you treat each charger type as having a single "cur_limit" and
> utilize that limit by telling the PMIC to draw that much current.
> Again, this is inconsistent with the specification.
> BC-1.2 defines, for each charger type, a minimum and maximum current
> level.
> The minimum should always be available.   Attempting to draw more than
> that (but less that the max) might work or might cause the charger
> to shut down, but you can be sure that the charger will respond to the
> increased load by first reducing the voltage, and will not shut down
> until the voltage has dropped below 2V.
> If you try to draw more current than the maximum, then the charger might
> shut down before the voltage drops below 2V.

Very well put :-)

> Given this understanding of the current available from the charger,
> there are two approaches the PMIC might take.
> 1/ if the PMIC is able to exercise fine control over the current it
>   draws, and if it can monitor the voltage on the charger, then it
>   could gradually increase the power being requested until the voltage
>   drops below some threshold (e.g. 4.75V), and then (probably) back off
>   a little.  It should only increase at most up to the maximum, even if
>   the voltage remains high.  It should probably start at zero rather
>   than at the minimum.  This allows for lossage elsewhere.

That's what most charging control SW I've seen in the past ends up
doing. Correct

> 2/ If the PMIC cannot measure voltage, or doesn't have sufficiently fine
>control of the current requested, then it should request only the
>minimum available current.  Any more is not safe.

correct

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/3] usb: rename bU2DevExitLat to wU2DevExitLat

[Felipe Balbi]

Hi,

Felipe Balbi  writes:
> According to USB 3.1 Specification, that field is 2
> bytes wide and is named with a 'w' prefix, not 'b'.
>
> Just to make grepping in the spec easier, let's
> match the name.
>
> Signed-off-by: Felipe Balbi 

I'll wait for another week or so, if nobody has any objections to this
series, I plan on sending it to Greg on my pull request.

If anybody needs a resend, just let me know ;-) (patch left below for
reference)

> ---
>  drivers/usb/core/hub.c | 8 
>  drivers/usb/gadget/composite.c | 4 ++--
>  drivers/usb/host/xhci-hub.c| 2 +-
>  drivers/usb/host/xhci.c| 2 +-
>  include/linux/usb/gadget.h | 2 +-
>  include/uapi/linux/usb/ch9.h   | 2 +-
>  6 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index 1d5fc32d06d0..0f87c62a81db 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -152,7 +152,7 @@ int usb_device_supports_lpm(struct usb_device *udev)
>   }
>  
>   if (udev->bos->ss_cap->bU1devExitLat == 0 &&
> - udev->bos->ss_cap->bU2DevExitLat == 0) {
> + udev->bos->ss_cap->wU2DevExitLat == 0) {
>   if (udev->parent)
>   dev_info(>dev, "LPM exit latency is zeroed, 
> disabling LPM.\n");
>   else
> @@ -311,9 +311,9 @@ static void usb_set_lpm_parameters(struct usb_device 
> *udev)
>   return;
>  
>   udev_u1_del = udev->bos->ss_cap->bU1devExitLat;
> - udev_u2_del = le16_to_cpu(udev->bos->ss_cap->bU2DevExitLat);
> + udev_u2_del = le16_to_cpu(udev->bos->ss_cap->wU2DevExitLat);
>   hub_u1_del = udev->parent->bos->ss_cap->bU1devExitLat;
> - hub_u2_del = le16_to_cpu(udev->parent->bos->ss_cap->bU2DevExitLat);
> + hub_u2_del = le16_to_cpu(udev->parent->bos->ss_cap->wU2DevExitLat);
>  
>   usb_set_lpm_mel(udev, >u1_params, udev_u1_del,
>   hub, >parent->u1_params, hub_u1_del);
> @@ -3909,7 +3909,7 @@ static void usb_enable_link_state(struct usb_hcd *hcd, 
> struct usb_device *udev,
>  {
>   int timeout, ret;
>   __u8 u1_mel = udev->bos->ss_cap->bU1devExitLat;
> - __le16 u2_mel = udev->bos->ss_cap->bU2DevExitLat;
> + __le16 u2_mel = udev->bos->ss_cap->wU2DevExitLat;
>  
>   /* If the device says it doesn't have *any* exit latency to come out of
>* U1 or U2, it's probably lying.  Assume it doesn't implement that link
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index 32176f779861..7c914dabfed4 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -646,11 +646,11 @@ static int bos_desc(struct usb_composite_dev *cdev)
>   cdev->gadget->ops->get_config_params(_config_params);
>   else {
>   dcd_config_params.bU1devExitLat = USB_DEFAULT_U1_DEV_EXIT_LAT;
> - dcd_config_params.bU2DevExitLat =
> + dcd_config_params.wU2DevExitLat =
>   cpu_to_le16(USB_DEFAULT_U2_DEV_EXIT_LAT);
>   }
>   ss_cap->bU1devExitLat = dcd_config_params.bU1devExitLat;
> - ss_cap->bU2DevExitLat = dcd_config_params.bU2DevExitLat;
> + ss_cap->wU2DevExitLat = dcd_config_params.wU2DevExitLat;
>  
>   /* The SuperSpeedPlus USB Device Capability descriptor */
>   if (gadget_is_superspeed_plus(cdev->gadget)) {
> diff --git a/drivers/usb/host/xhci-hub.c b/drivers/usb/host/xhci-hub.c
> index 730b9fd26685..ea1a7349ef02 100644
> --- a/drivers/usb/host/xhci-hub.c
> +++ b/drivers/usb/host/xhci-hub.c
> @@ -48,7 +48,7 @@ static u8 usb_bos_descriptor [] = {
>   0x03,   /* bFunctionalitySupport,
>  USB 3.0 speed only */
>   0x00,   /* bU1DevExitLat, set later. */
> - 0x00, 0x00, /* __le16 bU2DevExitLat, set later. */
> + 0x00, 0x00, /* __le16 wU2DevExitLat, set later. */
>   /* Second device capability, SuperSpeedPlus */
>   0x1c,   /* bLength 28, will be adjusted later */
>   USB_DT_DEVICE_CAPABILITY,   /* Device Capability */
> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
> index 01d96c9b3a75..6b86d1112ef1 100644
> --- a/drivers/usb/host/xhci.c
> +++ b/drivers/usb/host/xhci.c
> @@ -4405,7 +4405,7 @@ static unsigned long long 
> xhci_calculate_intel_u2_timeout(
>   (xhci_service_interval_to_ns(desc) > timeout_ns))
>   timeout_ns = xhci_service_interval_to_ns(desc);
>  
> - u2_del_ns = le16_to_cpu(udev->bos->ss_cap->bU2DevExitLat) * 1000ULL;
> + u2_del_ns = le16_to_cpu(udev->bos->ss_cap->wU2DevExitLat) * 1000ULL;
>   if (u2_del_ns > timeout_ns)
>   timeout_ns = u2_del_ns;
>  
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Peter Chen  writes:
> On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote:
>> On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote:
>> > 
>> > Can we use the firmware or bootloader information to provide the
>> > default dma-mapping attributes for devices that doesn't have an
>> > of_node pointer or ACPI data?  This will at least restore what we had
>> > previously provided .  I'm concerned that changing all the drivers
>> > that are creating child device will be a big effort.  Like I mentioned
>> > in another thread, there are many instances of platform_device_add()
>> > under the drivers/ directory.
>> 
>> Fortunately, there are not too many drivers that call platform_device_add
>> *and* try to set up a dma mask for the child device:
>> 
>> git grep -wl dma_mask drivers | xargs grep -wl 
>> 'platform_device_\(add\|register\)'
>> 
>> drivers/base/platform.c
>> drivers/bcma/main.c
>> drivers/eisa/virtual_root.c
>> drivers/mfd/mfd-core.c
>> drivers/mfd/omap-usb-host.c
>> drivers/misc/mic/card/mic_x100.c
>> drivers/platform/goldfish/pdev_bus.c
>> drivers/ssb/main.c
>> drivers/usb/chipidea/core.c
>> drivers/usb/dwc3/dwc3-exynos.c
>> drivers/usb/dwc3/host.c
>> drivers/usb/gadget/udc/bdc/bdc_pci.c
>> drivers/usb/host/bcma-hcd.c
>> drivers/usb/host/fsl-mph-dr-of.c
>> drivers/usb/host/ssb-hcd.c
>> drivers/usb/misc/ftdi-elan.c
>> drivers/usb/musb/blackfin.c
>> drivers/usb/musb/musb_dsps.c
>> drivers/usb/musb/omap2430.c
>> drivers/usb/musb/ux500.c
>> 
>> Most of these are probably never used with any nonstandard
>> DMA settings (IOMMU, cache coherency, offset, ...).
>> 
>> One thing we could possibly do is to go through these and
>> replace the hardcoded dma mask setup with of_dma_configure()
>> in all cases in which we actually use DT for probing, which
>> should cover the interesting cases.
>> 
>
> One case I am going to work is to let USB chipidea driver support iommu,
> the chipidea core device is no of_node, and created by
> platform_add_device on the runtime. Using of_dma_configure with parent
> of_node is a solution from my point, like [1].
>
> https://lkml.org/lkml/2016/2/22/7

this only solves the problem for DT devices. Legacy devices and
PCI-based systems will still suffer from the same problem. At least for
dwc3, I will only be taking patches that solve the problem for all
users, not a subset of them.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Peter Chen]

On Mon, Sep 05, 2016 at 05:39:27PM +0200, Arnd Bergmann wrote:
> On Friday, September 2, 2016 5:16:31 PM CEST Leo Li wrote:
> > 
> > Can we use the firmware or bootloader information to provide the
> > default dma-mapping attributes for devices that doesn't have an
> > of_node pointer or ACPI data?  This will at least restore what we had
> > previously provided .  I'm concerned that changing all the drivers
> > that are creating child device will be a big effort.  Like I mentioned
> > in another thread, there are many instances of platform_device_add()
> > under the drivers/ directory.
> 
> Fortunately, there are not too many drivers that call platform_device_add
> *and* try to set up a dma mask for the child device:
> 
> git grep -wl dma_mask drivers | xargs grep -wl 
> 'platform_device_\(add\|register\)'
> 
> drivers/base/platform.c
> drivers/bcma/main.c
> drivers/eisa/virtual_root.c
> drivers/mfd/mfd-core.c
> drivers/mfd/omap-usb-host.c
> drivers/misc/mic/card/mic_x100.c
> drivers/platform/goldfish/pdev_bus.c
> drivers/ssb/main.c
> drivers/usb/chipidea/core.c
> drivers/usb/dwc3/dwc3-exynos.c
> drivers/usb/dwc3/host.c
> drivers/usb/gadget/udc/bdc/bdc_pci.c
> drivers/usb/host/bcma-hcd.c
> drivers/usb/host/fsl-mph-dr-of.c
> drivers/usb/host/ssb-hcd.c
> drivers/usb/misc/ftdi-elan.c
> drivers/usb/musb/blackfin.c
> drivers/usb/musb/musb_dsps.c
> drivers/usb/musb/omap2430.c
> drivers/usb/musb/ux500.c
> 
> Most of these are probably never used with any nonstandard
> DMA settings (IOMMU, cache coherency, offset, ...).
> 
> One thing we could possibly do is to go through these and
> replace the hardcoded dma mask setup with of_dma_configure()
> in all cases in which we actually use DT for probing, which
> should cover the interesting cases.
> 

One case I am going to work is to let USB chipidea driver support iommu,
the chipidea core device is no of_node, and created by
platform_add_device on the runtime. Using of_dma_configure with parent
of_node is a solution from my point, like [1].

https://lkml.org/lkml/2016/2/22/7

-- 

Best Regards,
Peter Chen
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 1/8] binding-doc: power: pwrseq-generic: add binding doc for generic power sequence library

[Vaibhav Hiremath]

On Friday 02 September 2016 06:30 AM, Peter Chen wrote:

On Thu, Sep 01, 2016 at 01:33:22PM +0530, Vaibhav Hiremath wrote:


On Monday 15 August 2016 02:43 PM, Peter Chen wrote:

Add binding doc for generic power sequence library.

Signed-off-by: Peter Chen 
Acked-by: Philipp Zabel 
Acked-by: Rob Herring 
---
  .../bindings/power/pwrseq/pwrseq-generic.txt   | 48 ++
  1 file changed, 48 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt

diff --git a/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt 
b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
new file mode 100644
index 000..ebf0d47
--- /dev/null
+++ b/Documentation/devicetree/bindings/power/pwrseq/pwrseq-generic.txt
@@ -0,0 +1,48 @@
+The generic power sequence library
+
+Some hard-wired devices (eg USB/MMC) need to do power sequence before
+the device can be enumerated on the bus, the typical power sequence
+like: enable USB PHY clock, toggle reset pin, etc. But current
+Linux device driver lacks of such code to do it, it may cause some
+hard-wired devices works abnormal or can't be recognized by
+controller at all. The power sequence will be done before this device
+can be found at the bus.
+
+The power sequence properties is under the device node.
+
+Optional properties:
+- clocks: the input clocks for device.
+- reset-gpios: Should specify the GPIO for reset.
+- reset-duration-us: the duration in microsecond for assert reset signal.
+
+Below is the example of USB power sequence properties on USB device
+nodes which have two level USB hubs.
+
+ {
+   vbus-supply = <_usb_otg1_vbus>;
+   pinctrl-names = "default";
+   pinctrl-0 = <_usb_otg1_id>;
+   status = "okay";
+
+   #address-cells = <1>;
+   #size-cells = <0>;
+   genesys: hub@1 {
+   compatible = "usb5e3,608";
+   reg = <1>;
+
+   clocks = < IMX6SX_CLK_CKO>;
+   reset-gpios = < 5 GPIO_ACTIVE_LOW>; /* hub reset pin */
+   reset-duration-us = <10>;
+
+   #address-cells = <1>;
+   #size-cells = <0>;
+   asix: ethernet@1 {
+   compatible = "usbb95,1708";

So I assume, with our recent discussion and the change
we are proposing, the library would have some knowledge
about this compatible string, right?

Yes


what I was asking on other email was, how are you connecting
multiple power sequence libraries to their respective consumers ?


The consumers has its of_node, then it can find related power sequence
library according to compatible string.



Exactly. Thats what I was referring and wanted to confirm.

Thanks,
Vaibhav


--
Thanks,
Vaibhav

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html