Re: [PATCH 2/3] usb: gadget: configfs: Create control_config group

[Jerry Zhang]

> The purpose would be:
> 1) Allow writing no descriptors (maybe also skip the strings) when this
> flag is set
This should be straightforward. I'd rather not skip the strings though
since we can already indicate zero strings in the current struct. If we
skip strings then the difference between writing zero strings and skipping
them would become confusing.

> 2) Disallow linking such an instance to real configuration
> 3) Disallow linking real function implementation to our "magic config"
Hmm the issue with these is that descriptors aren't available at link time,
but rather at bind time. Its currently valid (and possibly useful) to link
a ffs instance and then write descriptors after.
We could do a mount flag but those aren't visible from configfs. How about
we fail to bind() if an empty function is in the normal config, or a
nonempty function is in the control config? This should be easy since it is
all visible in struct usb_function.

--Jerry
--
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: Representative Needed.

[PPMC OFFSHORE]

Good day,

  I am seeking your concept with great gratitude to present you as a 
representative to carry out business transactions with a reasonable share upon 
your interest and cooperation to work with us in trust. If interested please 
get back.

Regards
Kingsley

---
This email has been checked for viruses by Avast antivirus software.
https://www.avast.com/antivirus

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

[PATCH v3] usb: mon: Change return type to vm_fault_t

[Souptick Joarder]

Use new return type vm_fault_t for the fault handler
in struct vm_operations_struct. For now, this is just
documenting that the function returns a VM_FAULT value
rather than an errno. Once all instances are converted,
vm_fault_t will become a distinct type.

Reference id -> 1c8f422059ae ("mm: change return type to
vm_fault_t")

Signed-off-by: Souptick Joarder 
Reviewed-by: Matthew Wilcox 
---
v2: Modified the change logs.

v3: Modified change log

 drivers/usb/mon/mon_bin.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/mon/mon_bin.c b/drivers/usb/mon/mon_bin.c
index 2761fad..34e866ad 100644
--- a/drivers/usb/mon/mon_bin.c
+++ b/drivers/usb/mon/mon_bin.c
@@ -1227,7 +1227,7 @@ static void mon_bin_vma_close(struct vm_area_struct *vma)
 /*
  * Map ring pages to user space.
  */
-static int mon_bin_vma_fault(struct vm_fault *vmf)
+static vm_fault_t mon_bin_vma_fault(struct vm_fault *vmf)
 {
struct mon_reader_bin *rp = vmf->vma->vm_private_data;
unsigned long offset, chunk_idx;
--
1.9.1

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

[PATCH] usb: musb: trace: fix NULL pointer dereference in musb_g_tx()

[Bin Liu]

The usb_request pointer could be NULL in musb_g_tx(), where the
tracepoint call would trigger the NULL pointer dereference failure when
parsing the members of the usb_request pointer.

Move the tracepoint call to where the usb_request pointer is already
checked to solve the issue.

Fixes: fc78003e5345a("usb: musb: gadget: add usb-request tracepoints")

Cc: sta...@vger.kernel.org # v4.8+
Signed-off-by: Bin Liu 
---
 drivers/usb/musb/musb_gadget.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_gadget.c b/drivers/usb/musb/musb_gadget.c
index e564695c6c8d..71c5835ea9cd 100644
--- a/drivers/usb/musb/musb_gadget.c
+++ b/drivers/usb/musb/musb_gadget.c
@@ -417,7 +417,6 @@ void musb_g_tx(struct musb *musb, u8 epnum)
req = next_request(musb_ep);
request = >request;
 
-   trace_musb_req_tx(req);
csr = musb_readw(epio, MUSB_TXCSR);
musb_dbg(musb, "<== %s, txcsr %04x", musb_ep->end_point.name, csr);
 
@@ -456,6 +455,8 @@ void musb_g_tx(struct musb *musb, u8 epnum)
u8  is_dma = 0;
boolshort_packet = false;
 
+   trace_musb_req_tx(req);
+
if (dma && (csr & MUSB_TXCSR_DMAENAB)) {
is_dma = 1;
csr |= MUSB_TXCSR_P_WZC_BITS;
-- 
1.9.1

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

Re: howto debug xhci driver?

[Bin Liu]

Hi,

On Tue, Mar 20, 2018 at 02:28:00PM -0700, Paul Zimmerman wrote:
> Forgot to CC linux-usb.
> 
> 
>  Forwarded Message 
> Subject: Re: howto debug xhci driver?
> Date: Tue, 20 Mar 2018 13:56:21 -0700
> From: Paul Zimmerman 
> To: Bin Liu 
> CC: Felipe Balbi 
> 
> Hi,
> 
> Bin Liu  writes:
> >>Bin Liu  writes:
> BTY, the issue I am trying to debug is when reading
> bulk IN data from a USB2.0 device, if the device
> doesn't have data to transmit and NAKs the IN packet,
> after 4 pairs of IN-NAK transactions, xhci stops
> sending further IN tokens until the next SOF, which
> leaves ~90us gape on the bus.
> 
> But when reading data from a USB2.0 thumb drive, this
> issue doesn't happen, even if the device NAKs the IN
> tokens, xhci still keeps sending IN tokens, which is
> way more than 4 pairs of IN-NAK transactions.
> >>>
> >>>Thumb drive has Bulk endpoints, what is the other
> >>>device's transfer type?
> >>
> >>It is bulk too. I asked for device descriptors. This is a
> >>remote debug effort for me, I don't have that device...
> >>
> >>>
> Any one has a clue on what causes xhci to stop sending
> IN tokens after the device NAK'd 4 times?
> >
> >By accident, I reproduced the issue if addng a hub in the
> >middle... any comments about why a hub changes this xhci
> >behavior is appreciated.
> 
> none off the top of my head. Maybe Mathias can suggest
> something.
> >>>
> >>>The issue seems to be not related to how many bulk IN-NAK pairs
> >>>before host stops sending IN token, but the host stops IN token
> >>>if 1) the device ever NAK'd an bulk IN token, and 2) there is
> >>>about 90~100us left to the next SOF. Then all the rest of
> >>>bandwidth is wasted.
> >>>
> >>>Is it about xhci bandwidth schduling? /me started reading...
> >>
> >>is this AM4 or AM5? Perhaps go after Synopsys' known errata list?
> >
> >I see the issue on both AM4 & AM5. I don't have access to the
> >errata list, I guess I should talk to TI internal for the list?
> 
> I have a hazy recollection of something like this being a "feature" of
> the Synopsys core, to cut down on the excessive number of IN-NAK
> transactions you can sometimes get on the USB bus. So yep, you
> should talk to your Synopsys guy about this.

Thanks for the information. We have been talking to Synopsis for this
issue, the progress is slow, one of the reasons is that the DWC3 version
is very old :(

However, I just realized that in this case even though DWC3 in host mode
doesn't fully utilize the bus bandwidth, it doesn't violate any of the
USB Specs, as the Specs don't define flow control for bulk IN transfer.
The USB device shouldn't use bulk protocol at the first place if it has
bus bandwidth requirements. Isoch probably is a better choice. I will
check if there is anything can be done on the USB device side to solve
the problem.

Thanks all for the help.

Regards,
-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 V5] USB: Increment wakeup count on remote wakeup.

[Alan Stern]

On Fri, 20 Apr 2018, Ravi Chandra Sadineni wrote:

> On chromebooks we depend on wakeup count to identify the wakeup source.
> But currently USB devices do not increment the wakeup count when they
> trigger the remote wake. This patch addresses the same.
> 
> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
> 
> On USB 2.0 devices, a wake capable device, if wake enabled, drives
> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
> has resumed before driving the resume signal from the host and
> C_PORT_SUSPEND is set, then the device attached to the given port might
> be the reason for the last system wakeup. Increment the wakeup count for
> the same.
> 
> On USB 3.0 devices, a function may signal that it wants to exit from device
> suspend by sending a Function Wake Device Notification to the host (USB3.0
> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
> wakeup count.
> 
> Signed-off-by: Ravi Chandra Sadineni 
> ---
> 
> V5: Added the description of changes between different versions of patches.
> V4: Moved the wakeup count increment logic to the existing if which is
> safegaurded by hcd_root_hub_lock spinlock.
> V3: Added a gaurd to check if rh_registered is set before accessing
> root_hub pointer.
> V2: Fixed the build failure error due to uninitialized dev pointer.

Acked-by: Alan Stern 

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

Re: [PATCH V3] USB: Increment wakeup count on remote wakeup.

[Ravi Chandra Sadineni]

On Fri, Apr 20, 2018 at 10:29 AM, Alan Stern  wrote:
> On Fri, 20 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni 
>> ---
>
> At this point you're supposed to list the differences between this
> patch and the preceding versions.

Mentioned the changes between different versions in V5. Thanks.

>
>>  drivers/usb/core/hcd.c |  2 ++
>>  drivers/usb/core/hub.c | 10 +-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..b8024ae4fdcaa 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>>  {
>>   unsigned long flags;
>>
>> + if (hcd->rh_registered)
>> + pm_wakeup_event(>self.root_hub->dev, 0);
>>   spin_lock_irqsave (_root_hub_lock, flags);
>>   if (hcd->rh_registered) {
>>   set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> This isn't good enough.  hcd->rh_registered can change at any time;
> it is protected by the hcd_root_hub_lock spinlock.  That's why I said
> your code should be moved inside the existing "if" statement.

Sorry about this.  Fixed it now. Hope this is fine. Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>>   unsigned int portnum)
>>  {
>>   struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>>   if (!hdev)
>>   return;
>>
>>   hub = usb_hub_to_struct_hub(hdev);
>>   if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>>   set_bit(portnum, hub->wakeup_bits);
>>   kick_hub_wq(hub);
>>   }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
>> pm_message_t msg)
>>
>>   /* Skip the initial Clear-Suspend step for a remote wakeup */
>>   status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>>   goto SuspendCleared;
>> + }
>>
>>   /* see 7.1.7.7; affects power usage, but not budgeting */
>>   if (hub_is_superspeed(hub->hdev))
>>
>
Thanks,
Ravi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH V5] USB: Increment wakeup count on remote wakeup.

[Ravi Chandra Sadineni]

On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.

Resume condition is reported differently on USB 2.0 and USB 3.0 devices.

On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.

On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.

Signed-off-by: Ravi Chandra Sadineni 
---

V5: Added the description of changes between different versions of patches.
V4: Moved the wakeup count increment logic to the existing if which is
safegaurded by hcd_root_hub_lock spinlock.
V3: Added a gaurd to check if rh_registered is set before accessing
root_hub pointer.
V2: Fixed the build failure error due to uninitialized dev pointer.

drivers/usb/core/hcd.c |  1 +
 drivers/usb/core/hub.c | 10 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+   pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
 {
struct usb_hub *hub;
+   struct usb_port *port_dev;
 
if (!hdev)
return;
 
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+   port_dev = hub->ports[portnum - 1];
+   if (port_dev && port_dev->child)
+   pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
pm_message_t msg)
 
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
-   if (status == 0 && !port_is_suspended(hub, portstatus))
+   if (status == 0 && !port_is_suspended(hub, portstatus)) {
+   if (portchange & USB_PORT_STAT_C_SUSPEND)
+   pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+   }
 
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
-- 
2.17.0.484.g0c8726318c-goog

--
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 V4] USB: Increment wakeup count on remote wakeup.

[Ravi Chandra Sadineni]

On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.

Resume condition is reported differently on USB 2.0 and USB 3.0 devices.

On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.

On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.

Signed-off-by: Ravi Chandra Sadineni 
---
 drivers/usb/core/hcd.c |  1 +
 drivers/usb/core/hub.c | 10 +-
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..00bb8417050ff 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2377,6 +2377,7 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
+   pm_wakeup_event(>self.root_hub->dev, 0);
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
queue_work(pm_wq, >wakeup_work);
}
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
 {
struct usb_hub *hub;
+   struct usb_port *port_dev;
 
if (!hdev)
return;
 
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+   port_dev = hub->ports[portnum - 1];
+   if (port_dev && port_dev->child)
+   pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
pm_message_t msg)
 
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
-   if (status == 0 && !port_is_suspended(hub, portstatus))
+   if (status == 0 && !port_is_suspended(hub, portstatus)) {
+   if (portchange & USB_PORT_STAT_C_SUSPEND)
+   pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+   }
 
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
-- 
2.17.0.484.g0c8726318c-goog

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

RE: [PATCH 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL for usb gadget

[Leo Li]

> -Original Message-
> From: Tiago Brusamarello [mailto:tbr...@gmail.com]
> Sent: Thursday, April 19, 2018 6:58 AM
> To: Leo Li 
> Cc: nikhil.bad...@freescale.com; linux-usb@vger.kernel.org
> Subject: [PATCH 1/1] drivers: usb: Introduce FSL_USB2_PHY_UTMI_DUAL for
> usb gadget
> 
> Introduce FSL_USB2_PHY_UTMI_DUAL in gadget driver for setting
> phy in SOCs with utmi dual phy
> 
> Signed-off-by: Nikhil Badola 

[snip]
> Change-Id: I2c53b89d9916bd17b5be8b5d9e32454943172d55
> Reviewed-on:
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fgit.a
> m.freescale.net%3A8181%2F25528=02%7C01%7Cleoyang.li%40nxp.co
> m%7C7e18a2921b7b43abb8db08d5a5ecc059%7C686ea1d3bc2b4c6fa92cd99c5
> c301635%7C0%7C0%7C636597358611709638=U3moxDqSc%2F576XLDS
> qJLdfYhDJeccNLzF%2FWr%2BQv0bRU%3D=0
> Tested-by: Review Code-CDREVIEW 
> Reviewed-by: Suresh Gupta 
> Reviewed-by: Matthew Weigel 
[/snip]

The patch looks good.  But we probably should remove the above internal 
information as they are not needed for upstream.

> Tested-by: Tiago Brusamarello 

Otherwise

Acked-by: Li Yang 

> ---
>  drivers/usb/gadget/udc/fsl_udc_core.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/usb/gadget/udc/fsl_udc_core.c
> b/drivers/usb/gadget/udc/fsl_udc_core.c
> index 56b517a..a3c092b 100644
> --- a/drivers/usb/gadget/udc/fsl_udc_core.c
> +++ b/drivers/usb/gadget/udc/fsl_udc_core.c
> @@ -253,6 +253,7 @@ static int dr_controller_setup(struct fsl_udc *udc)
>          portctrl |= PORTSCX_PTW_16BIT;
>          /* fall through */
>      case FSL_USB2_PHY_UTMI:
> +    case FSL_USB2_PHY_UTMI_DUAL:
>          if (udc->pdata->have_sysif_regs) {
>              if (udc->pdata->controller_ver) {
>                  /* controller version 1.6 or above */
> --
> 2.7.4

Re: [PATCH V3] USB: Increment wakeup count on remote wakeup.

[Alan Stern]

On Fri, 20 Apr 2018, Ravi Chandra Sadineni wrote:

> On chromebooks we depend on wakeup count to identify the wakeup source.
> But currently USB devices do not increment the wakeup count when they
> trigger the remote wake. This patch addresses the same.
> 
> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
> 
> On USB 2.0 devices, a wake capable device, if wake enabled, drives
> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
> has resumed before driving the resume signal from the host and
> C_PORT_SUSPEND is set, then the device attached to the given port might
> be the reason for the last system wakeup. Increment the wakeup count for
> the same.
> 
> On USB 3.0 devices, a function may signal that it wants to exit from device
> suspend by sending a Function Wake Device Notification to the host (USB3.0
> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
> wakeup count.
> 
> Signed-off-by: Ravi Chandra Sadineni 
> ---

At this point you're supposed to list the differences between this 
patch and the preceding versions.

>  drivers/usb/core/hcd.c |  2 ++
>  drivers/usb/core/hub.c | 10 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae63674..b8024ae4fdcaa 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>  {
>   unsigned long flags;
>  
> + if (hcd->rh_registered)
> + pm_wakeup_event(>self.root_hub->dev, 0);
>   spin_lock_irqsave (_root_hub_lock, flags);
>   if (hcd->rh_registered) {
>   set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);

This isn't good enough.  hcd->rh_registered can change at any time;  
it is protected by the hcd_root_hub_lock spinlock.  That's why I said
your code should be moved inside the existing "if" statement.

Alan Stern

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index f6ea16e9f6bb9..aa9968d90a48c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>   unsigned int portnum)
>  {
>   struct usb_hub *hub;
> + struct usb_port *port_dev;
>  
>   if (!hdev)
>   return;
>  
>   hub = usb_hub_to_struct_hub(hdev);
>   if (hub) {
> + port_dev = hub->ports[portnum - 1];
> + if (port_dev && port_dev->child)
> + pm_wakeup_event(_dev->child->dev, 0);
> +
>   set_bit(portnum, hub->wakeup_bits);
>   kick_hub_wq(hub);
>   }
> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
> pm_message_t msg)
>  
>   /* Skip the initial Clear-Suspend step for a remote wakeup */
>   status = hub_port_status(hub, port1, , );
> - if (status == 0 && !port_is_suspended(hub, portstatus))
> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
> + if (portchange & USB_PORT_STAT_C_SUSPEND)
> + pm_wakeup_event(>dev, 0);
>   goto SuspendCleared;
> + }
>  
>   /* see 7.1.7.7; affects power usage, but not budgeting */
>   if (hub_is_superspeed(hub->hdev))
> 

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

Re: [PATCH 1/2] usb: typec: tps6598x: handle block reads separately with plain-I2C adapters

[Guenter Roeck]

On Wed, Apr 18, 2018 at 03:34:09PM +0300, Heikki Krogerus wrote:
> If the I2C adapter that the PD controller is attached to
> does not support SMBus protocol, the driver needs to handle
> block reads separately. The first byte returned in block
> read protocol will show the total number of bytes. It needs
> to be stripped away.
> 
> This is handled separately in the driver only because right
> now we have no way of requesting the used protocol with
> regmap-i2c. This is in practice a workaround for what is
> really a problem in regmap-i2c. The other option would have
> been to register custom regmap, or not use regmap at all,
> however, since the solution is very simple, I choose to use
> it in this case for convenience. It is easy to remove once
> we figure out how to handle this kind of cases in
> regmap-i2c.
> 
> Fixes: 0a4c005bd171 ("usb: typec: driver for TI TPS6598x USB Power Delivery 
> controllers")
> Signed-off-by: Heikki Krogerus 
> ---
>  drivers/usb/typec/tps6598x.c | 42 ++--
>  1 file changed, 35 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 8b8406867c02..82f09cd9792d 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -73,6 +73,7 @@ struct tps6598x {
>   struct device *dev;
>   struct regmap *regmap;
>   struct mutex lock; /* device lock */
> + u8 i2c_protocol:1;
>  
>   struct typec_port *port;
>   struct typec_partner *partner;
> @@ -80,6 +81,23 @@ struct tps6598x {
>   struct typec_capability typec_cap;
>  };
>  
> +static int
> +tps6598x_block_read(struct tps6598x *tps, u8 reg, void *val, ssize_t len)
> +{
> + u8 data[len + 1];
> + int ret;
> +
> + if (!tps->i2c_protocol)
> + return regmap_raw_read(tps->regmap, reg, val, len);
> +
> + ret = regmap_raw_read(tps->regmap, reg, data, sizeof(data));
> + if (ret)
> + return ret;
> +

Sanity check ?
if (data[0] != len)
return -Esomething;

Other than that,

Reviewed-by: Guenter Roeck 

> + memcpy(val, [1], len);
> + return 0;
> +}
> +
>  static inline int tps6598x_read16(struct tps6598x *tps, u8 reg, u16 *val)
>  {
>   return regmap_raw_read(tps->regmap, reg, val, sizeof(u16));
> @@ -87,12 +105,12 @@ static inline int tps6598x_read16(struct tps6598x *tps, 
> u8 reg, u16 *val)
>  
>  static inline int tps6598x_read32(struct tps6598x *tps, u8 reg, u32 *val)
>  {
> - return regmap_raw_read(tps->regmap, reg, val, sizeof(u32));
> + return tps6598x_block_read(tps, reg, val, sizeof(u32));
>  }
>  
>  static inline int tps6598x_read64(struct tps6598x *tps, u8 reg, u64 *val)
>  {
> - return regmap_raw_read(tps->regmap, reg, val, sizeof(u64));
> + return tps6598x_block_read(tps, reg, val, sizeof(u64));
>  }
>  
>  static inline int tps6598x_write16(struct tps6598x *tps, u8 reg, u16 val)
> @@ -121,8 +139,8 @@ static int tps6598x_read_partner_identity(struct tps6598x 
> *tps)
>   struct tps6598x_rx_identity_reg id;
>   int ret;
>  
> - ret = regmap_raw_read(tps->regmap, TPS_REG_RX_IDENTITY_SOP,
> -   , sizeof(id));
> + ret = tps6598x_block_read(tps, TPS_REG_RX_IDENTITY_SOP,
> +   , sizeof(id));
>   if (ret)
>   return ret;
>  
> @@ -224,13 +242,13 @@ static int tps6598x_exec_cmd(struct tps6598x *tps, 
> const char *cmd,
>   } while (val);
>  
>   if (out_len) {
> - ret = regmap_raw_read(tps->regmap, TPS_REG_DATA1,
> -   out_data, out_len);
> + ret = tps6598x_block_read(tps, TPS_REG_DATA1,
> +   out_data, out_len);
>   if (ret)
>   return ret;
>   val = out_data[0];
>   } else {
> - ret = regmap_read(tps->regmap, TPS_REG_DATA1, );
> + ret = tps6598x_block_read(tps, TPS_REG_DATA1, , sizeof(u8));
>   if (ret)
>   return ret;
>   }
> @@ -385,6 +403,16 @@ static int tps6598x_probe(struct i2c_client *client)
>   if (!vid)
>   return -ENODEV;
>  
> + /*
> +  * Checking can the adapter handle SMBus protocol. If if can not, the
> +  * driver needs to take care of block reads separately.
> +  *
> +  * FIXME: Testing with I2C_FUNC_I2C. regmap-i2c uses I2C protocol
> +  * unconditionally if the adapter has I2C_FUNC_I2C set.
> +  */
> + if (i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
> + tps->i2c_protocol = true;
> +
>   ret = tps6598x_read32(tps, TPS_REG_STATUS, );
>   if (ret < 0)
>   return ret;
> -- 
> 2.17.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  

Re: [PATCH v5 0/5] usb: typec: remove max_snk_mv/ma/mw

[Guenter Roeck]

On Mon, Apr 16, 2018 at 02:54:32PM +0800, Li Jun wrote:
> This patch set is to remove max_snk_mv/ma/mw configs, as we should
> define the sink capability by sink PDOs, the first patch update
> the source PDO match policy by compare the voltage range between
> source and sink PDOs no matter what type they are, the following
> patchs remove those 3 variables from 2 existing users by adding
> a variable PDO, then finial patch remove the max_snk_* from tcpm.
> 
> Change for v5:
> - Fix ret = 0 missing when found matching source pdo, and add
>   a variable to get the return value of tcpm_pd_select_pdo()
>   in patch 1/5, tested on my platform.
> - Add document in fusb302_composite_snk_pdo_array() to explain
>   why use deprecated properties in patch 2/5.
>  

For the series:

Reviewed-by: Guenter Roeck 

> Change for v4:
> - Add Hans's reviewed-by tag for the whole patch set.
> 
> Changes for v3:
> - Remove 3 variables: nr_fxied, nr_var and nr_batt from tcpm_port;
>   so nr_type_pdos() is not needed and removed.
> - Simplify fusb302_composite_snk_pdo_array() by only considering
>   existing setting as Hans suggested.
> - Add Rob's reviewed-by for dt-binding patch.
> 
> Changes for v2:
> - rebase the 1st patch to be based on commit 6f566af34628
>   ("Revert "typec: tcpm: Only request matching pdos"").
> - Convert the device properties passing max_snk_* to be a
>   variable sink pdo for fusb302.
> 
> Li Jun (5):
>   usb: typec: tcpm: pdo matching optimization
>   usb: typec: fusb302: remove max_snk_* for sink config
>   dt-bindings: usb: fusb302: remove max-sink-* properties
>   usb: typec: wcove: remove max_snk_* for sink config
>   usb: typec: tcpm: remove max_snk_mv/ma/mw
> 
>  .../devicetree/bindings/usb/fcs,fusb302.txt|   6 --
>  drivers/usb/typec/fusb302/fusb302.c|  42 +---
>  drivers/usb/typec/tcpm.c   | 117 
> -
>  drivers/usb/typec/typec_wcove.c|   4 +-
>  include/linux/usb/tcpm.h   |   9 --
>  5 files changed, 96 insertions(+), 82 deletions(-)
> 
> -- 
> 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 V2] USB: Increment wakeup count on remote wakeup.

[Ravi Chandra Sadineni]

On Fri, Apr 20, 2018 at 7:12 AM, Alan Stern  wrote:
> On Thu, 19 Apr 2018, Ravi Chandra Sadineni wrote:
>
>> On chromebooks we depend on wakeup count to identify the wakeup source.
>> But currently USB devices do not increment the wakeup count when they
>> trigger the remote wake. This patch addresses the same.
>>
>> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
>>
>> On USB 2.0 devices, a wake capable device, if wake enabled, drives
>> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
>> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
>> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
>> has resumed before driving the resume signal from the host and
>> C_PORT_SUSPEND is set, then the device attached to the given port might
>> be the reason for the last system wakeup. Increment the wakeup count for
>> the same.
>>
>> On USB 3.0 devices, a function may signal that it wants to exit from device
>> suspend by sending a Function Wake Device Notification to the host (USB3.0
>> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
>> wakeup count.
>>
>> Signed-off-by: Ravi Chandra Sadineni 
>> ---
>>  drivers/usb/core/hcd.c |  2 ++
>>  drivers/usb/core/hub.c | 10 +-
>>  2 files changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
>> index 777036ae63674..ee0f3ec57ce49 100644
>> --- a/drivers/usb/core/hcd.c
>> +++ b/drivers/usb/core/hcd.c
>> @@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
>>  void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>>  {
>>   unsigned long flags;
>> + struct device *dev = >self.root_hub->dev;
>>
>> + pm_wakeup_event(dev, 0);
>>   spin_lock_irqsave (_root_hub_lock, flags);
>>   if (hcd->rh_registered) {
>>   set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
>
> In general, hcd->self.root_hub is guaranteed to be non-NULL only when
> hcd->rh_registered is set.  Therefore the assignment to dev and the
> call to pm_wakeup_event() should be moved within this "if" statement.
>
> The rest of the patch looks okay.

Added the check in V3.  Thanks.
>
> Alan Stern
>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index f6ea16e9f6bb9..aa9968d90a48c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>>   unsigned int portnum)
>>  {
>>   struct usb_hub *hub;
>> + struct usb_port *port_dev;
>>
>>   if (!hdev)
>>   return;
>>
>>   hub = usb_hub_to_struct_hub(hdev);
>>   if (hub) {
>> + port_dev = hub->ports[portnum - 1];
>> + if (port_dev && port_dev->child)
>> + pm_wakeup_event(_dev->child->dev, 0);
>> +
>>   set_bit(portnum, hub->wakeup_bits);
>>   kick_hub_wq(hub);
>>   }
>> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
>> pm_message_t msg)
>>
>>   /* Skip the initial Clear-Suspend step for a remote wakeup */
>>   status = hub_port_status(hub, port1, , );
>> - if (status == 0 && !port_is_suspended(hub, portstatus))
>> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
>> + if (portchange & USB_PORT_STAT_C_SUSPEND)
>> + pm_wakeup_event(>dev, 0);
>>   goto SuspendCleared;
>> + }
>>
>>   /* see 7.1.7.7; affects power usage, but not budgeting */
>>   if (hub_is_superspeed(hub->hdev))
>>
>

Thanks,
Ravi
--
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 V3] USB: Increment wakeup count on remote wakeup.

[Ravi Chandra Sadineni]

On chromebooks we depend on wakeup count to identify the wakeup source.
But currently USB devices do not increment the wakeup count when they
trigger the remote wake. This patch addresses the same.

Resume condition is reported differently on USB 2.0 and USB 3.0 devices.

On USB 2.0 devices, a wake capable device, if wake enabled, drives
resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
The upstream facing port then sets C_PORT_SUSPEND bit and reports a
port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
has resumed before driving the resume signal from the host and
C_PORT_SUSPEND is set, then the device attached to the given port might
be the reason for the last system wakeup. Increment the wakeup count for
the same.

On USB 3.0 devices, a function may signal that it wants to exit from device
suspend by sending a Function Wake Device Notification to the host (USB3.0
spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
wakeup count.

Signed-off-by: Ravi Chandra Sadineni 
---
 drivers/usb/core/hcd.c |  2 ++
 drivers/usb/core/hub.c | 10 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 777036ae63674..b8024ae4fdcaa 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -2375,6 +2375,8 @@ void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
 {
unsigned long flags;
 
+   if (hcd->rh_registered)
+   pm_wakeup_event(>self.root_hub->dev, 0);
spin_lock_irqsave (_root_hub_lock, flags);
if (hcd->rh_registered) {
set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);
diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
index f6ea16e9f6bb9..aa9968d90a48c 100644
--- a/drivers/usb/core/hub.c
+++ b/drivers/usb/core/hub.c
@@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
unsigned int portnum)
 {
struct usb_hub *hub;
+   struct usb_port *port_dev;
 
if (!hdev)
return;
 
hub = usb_hub_to_struct_hub(hdev);
if (hub) {
+   port_dev = hub->ports[portnum - 1];
+   if (port_dev && port_dev->child)
+   pm_wakeup_event(_dev->child->dev, 0);
+
set_bit(portnum, hub->wakeup_bits);
kick_hub_wq(hub);
}
@@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
pm_message_t msg)
 
/* Skip the initial Clear-Suspend step for a remote wakeup */
status = hub_port_status(hub, port1, , );
-   if (status == 0 && !port_is_suspended(hub, portstatus))
+   if (status == 0 && !port_is_suspended(hub, portstatus)) {
+   if (portchange & USB_PORT_STAT_C_SUSPEND)
+   pm_wakeup_event(>dev, 0);
goto SuspendCleared;
+   }
 
/* see 7.1.7.7; affects power usage, but not budgeting */
if (hub_is_superspeed(hub->hdev))
-- 
2.17.0.484.g0c8726318c-goog

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

Re: BUG xhci_hcd usb3 module WARNS with JMicron JMS567 when writing to disk

[Tomas M]

Hello Menion.

Yes, Ive already circunvented it connecting to a USB2 port instead. It
would be nice to have this feature working. that is all.

ehci-pci kernel module works.
xhci-hcd kernel module fails.

on both cases its running as UAS, but one module is broken. Are you
sure the issue is with the chipset itself?

Thanks

Tomas



On Fri, Apr 20, 2018 at 11:42 AM, Menion  wrote:
> I think that JMS567 is mostly broken nowadays in Linux UAS
> Mine in an Orico multibay encolsure produce a lot of CMD timeout
> Recently Orico is just silently disabling in firmware the UAS support
> for this bridge (still advertising it on the webpage, usual chinese
> behaviour)
> Try to disable UAS with usb-storage quirks instead of blacklisting the
> entire usb-storage
>
> 2018-04-20 16:10 GMT+02:00 Tomas M :
>> Hello,
>>
>> Tested 4.16.2 with the same results. Anything i can do to help find the 
>> problem?
>>
>> Thanks
>>
>> Tomas
>>
>> On Mon, Mar 12, 2018 at 8:22 AM, Tomas M  wrote:
>>> Hello, I think its UAS:
>>> From the dmesg:
>>> [  652.335275] usb 1-1.3: Product: SABRENT
>>> [  652.335279] usb 1-1.3: Manufacturer: SABRENT
>>> [  652.335283] usb 1-1.3: SerialNumber: DB98765432123
>>> [  652.337283] scsi host8: uas
>>> [  652.338731] scsi 8:0:0:0: Direct-Access SABRENT
>>>   4101 PQ: 0 ANSI: 6
>>>
>>> SABRENT Is the HDD craddle.
>>>
>>> I have an external Toshiba HDD running on the same computer with the
>>> xhci_hcd driver for years without problems. But it reads:
>>>  >>> usb-storage 4-1:1.0: USB Mass Storage device detected
>>> so i think its fundamentally different.
>>>
>>> Yes. data corruption ocurred when Oopsing.
>>>
>>> Like i said, i tried different HDDs and have two HDD Craddles to play
>>> with. all combinations brought the same result. Both craddles have
>>> SATA-USB3 chipsets from JMicrion. running on a USB2 chipset with the
>>> ehci driver works fine.
>>>
>>> Regards
>>>
>>> Tomas
>>>
>>> On Mon, Mar 12, 2018 at 6:52 AM, Oliver Neukum  wrote:
 Am Sonntag, den 11.03.2018, 10:48 -0300 schrieb Tomas M:
> Hello,
>
> I reported the bug here and was suggested to forward it to the ML.
> https://bugzilla.kernel.org/show_bug.cgi?id=199075
>
> find attached the owed dmesg with the stack trace.
>
> using the ehci ports produces no error.
> using the xhci ports fails very fast when writing.
>
> this happens with the
> Bus 001 Device 005: ID 152d:0578 JMicron Technology Corp. / JMicron
> USA Technology Corp. JMS567 SATA 6Gb/s bridge
>
> and another similar chipset from JMicron too.
>
> I have another external USB3 drive using the xhci driver without problems.
>
> Regards

 UAS or storage?

 A CRC error is really unusual and an oops in RCU is arcane.
 Do you have errors with other Super Speed devices? This looks
 like data corruption.

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

Re: [PATCH v5 1/5] usb: typec: tcpm: pdo matching optimization

[Guenter Roeck]

On Fri, Apr 20, 2018 at 06:13:30PM +0300, Heikki Krogerus wrote:
> Hi,
> 
> On Mon, Apr 16, 2018 at 02:54:33PM +0800, Li Jun wrote:
> > This patch is a combination of commit 57e6f0d7b804
> > ("typec: tcpm: Only request matching pdos") and source
> > pdo selection optimization based on it, instead of only
> > compare between the same pdo type of sink and source,
> > we should check source pdo voltage range is within the
> > voltage range of one sink pdo.
> > 
> > Reviewed-by: Hans de Goede 
> > Signed-off-by: Li Jun 
> 
> I don't see any problem with this, but I hope Guenter has time to
> check it.
> 

I don't see anything wrong, but then the changes are so complex that
I can only hope they were well tested. Unfortunately, I won't have
time to do such testing myself.

Acked-by: Guenter Roeck 

> Acked-by: Heikki Krogerus 
> 
> > ---
> >  drivers/usb/typec/tcpm.c | 105 
> > ++-
> >  1 file changed, 67 insertions(+), 38 deletions(-)
> > 
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> > index 677d121..048b953 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -1772,39 +1772,63 @@ static int tcpm_pd_check_request(struct tcpm_port 
> > *port)
> > return 0;
> >  }
> >  
> > -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> > +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> > +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> > +
> > +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> > + int *src_pdo)
> >  {
> > -   unsigned int i, max_mw = 0, max_mv = 0;
> > +   unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> > +max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> > +min_snk_mv = 0;
> 
> Don't resend because of this comment, but just for the record, you are
> declaring here so many variables at once that it is not easy to see
> what you are introducing (at least for me). It is OK to declare every
> variable even separately.
> 
> > int ret = -EINVAL;
> >  
> > /*
> > -* Select the source PDO providing the most power while staying within
> > -* the board's voltage limits. Prefer PDO providing exp
> > +* Select the source PDO providing the most power which has a
> > +* matchig sink cap.
> >  */
> > for (i = 0; i < port->nr_source_caps; i++) {
> > u32 pdo = port->source_caps[i];
> > enum pd_pdo_type type = pdo_type(pdo);
> > -   unsigned int mv, ma, mw;
> >  
> > -   if (type == PDO_TYPE_FIXED)
> > -   mv = pdo_fixed_voltage(pdo);
> > -   else
> > -   mv = pdo_min_voltage(pdo);
> > +   if (type == PDO_TYPE_FIXED) {
> > +   max_src_mv = pdo_fixed_voltage(pdo);
> > +   min_src_mv = max_src_mv;
> > +   } else {
> > +   max_src_mv = pdo_max_voltage(pdo);
> > +   min_src_mv = pdo_min_voltage(pdo);
> > +   }
> >  
> > if (type == PDO_TYPE_BATT) {
> > -   mw = pdo_max_power(pdo);
> > +   src_mw = pdo_max_power(pdo);
> > } else {
> > -   ma = min(pdo_max_current(pdo),
> > -port->max_snk_ma);
> > -   mw = ma * mv / 1000;
> > +   src_ma = pdo_max_current(pdo);
> > +   src_mw = src_ma * min_src_mv / 1000;
> > }
> >  
> > -   /* Perfer higher voltages if available */
> > -   if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> > -   mv <= port->max_snk_mv) {
> > -   ret = i;
> > -   max_mw = mw;
> > -   max_mv = mv;
> > +   for (j = 0; j < port->nr_snk_pdo; j++) {
> > +   pdo = port->snk_pdo[j];
> > +
> > +   if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> > +   min_snk_mv = pdo_fixed_voltage(pdo);
> > +   max_snk_mv = pdo_fixed_voltage(pdo);
> > +   } else {
> > +   min_snk_mv = pdo_min_voltage(pdo);
> > +   max_snk_mv = pdo_max_voltage(pdo);
> > +   }
> > +
> > +   if (max_src_mv <= max_snk_mv &&
> > +   min_src_mv >= min_snk_mv) {
> > +   /* Prefer higher voltages if available */
> > +   if ((src_mw == max_mw && min_src_mv > max_mv) ||
> > +   src_mw > max_mw) {
> > +   *src_pdo = i;
> > +   *sink_pdo = j;
> > +   max_mw = src_mw;
> > +   max_mv = 

Re: [PATCH v4 0/3] lan78xx: Read configuration from Device Tree

[David Miller]

From: Phil Elwell 
Date: Thu, 19 Apr 2018 17:59:37 +0100

> The Microchip LAN78XX family of devices are Ethernet controllers with
> a USB interface. Despite being discoverable devices it can be useful to
> be able to configure them from Device Tree, particularly in low-cost
> applications without an EEPROM or programmed OTP.
> 
> This patch set adds support for reading the MAC address and LED modes from
> Device Tree.
> 
> v4:
> - Rename nodes in bindings doc.
> 
> v3:
> - Move LED setting into PHY driver.
> 
> v2:
> - Use eth_platform_get_mac_address.
> - Support up to 4 LEDs, and move LED mode constants into dt-bindings header.
> - Improve bindings document.
> - Remove EEE support.

Series applied, thanks Phil.
--
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 RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Hans de Goede]

Hi,

On 04/20/2018 04:13 PM, Guenter Roeck wrote:

On 04/20/2018 03:51 AM, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月20日 17:21
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx

Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

Hi,

On 20-04-18 11:18, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月18日 19:40
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org;
dl-linux-imx 
Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw
for rdo

Hi,

On 17-04-18 08:01, Li Jun wrote:

This patch is a further update for rdo based on [1], which removed
max_snk_ma/mv/mw but kept operating_snk_mw.
operating_snk_mw is only used to judge capability mismatch, per PD
spec, we can achieve this via compare the selected source PDO and
matching sink PDO, also after patch [1], we don't limit the PDO
matching between the same type, so the rdo operating and max
current/power calculation should be updated accordingly.


I do not believe that this is correct, lets take a device with a
fusb302 tcpc with the following PDO-s:

PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
PDO_VAR(5000, 12000, 3000);

And an operating_snk_mw of 2500mW then according to your new code a
charger which supplies 12V 2A will now get the mismatch bit set even
though it is delivering 24W.



Per PD spec, my understanding is to judge capability mismatch, we
should use *max* current/power, not this operating power.


Ok, I'm not familiar with the PD spec, perhaps someone who knows it well can
comment here ?


Hi Guenter,
Could you please comment here?



I am not, and never was, the kind of person who would blindly follow a 
specification.
I use specifications as guideline. For PD, when I implemented the code, I made 
sure
that the implementation worked with as many power adapters and dongles as I 
could get
my hand on.

At this point in time, I would only change the code myself if it is known to be 
broken.
Otherwise I would leave it alone. I would _never_ make a change because my 
interpretation
of the specification is different, unless I can prove that the current code is 
wrong and
causes problems.

Unfortunately, I don't currently have a setup to test the current version of 
the code,
nor the time to do all the testing. I am more and more concerned, though, that 
changes
are made out of principle, not because something has been shown to be broken. 
But that is
just a personal concern, and without re-testing everything I don't really have 
a strong
case for or against those changes.


AFAIK Jun Li's previous series was actually necessary because some boards only
contain some input voltages rather then a range, so that series was triggered
by an actual problem encountered with real hardware, right Jun Li ?

But yes this patch feels like a change purely based on a (re)interpretation
of the SPEC, so lets drop this change and keep the current behavior.

Regards,

Hans






Guenter


Thanks
Jun




In your case(VAR PDO), the max current is 3000mA, can I say the max
power is 3000mA * 5V = 15W? so the max current required at 12V is 15W
/ 12V = 1.25A, so yes, the 12V@2A is enough, do you think this kind of
calculation is reasonable?


That does not sound unreasonable, the real question is when exactly should we
set the capability mismatch bit, once we have that more clear how to
implement this should become clear too.


Fully agree, below is the capability mismatch bit related description from PD 
spec:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value larger than
the maximum current/power offered in the Source_Capabilities Message’s PDO
as referenced by the Object position field. This enables the Sink to indicate 
that
it requires more current/power than is being offered. If the Sink requires a
different voltage this will be indicated by its Sink_Capabilities Message."

"6.4.2.8 Maximum Operating Current
The Maximum Operating Current field in the Request Message shall be set to
the highest current the Sink will ever require. The difference between
the Operating Current and Maximum Operating Current fields
(when the GiveBack Flag is cleared) is used by the Device Policy Manager
in the Source to calculate the size of the Power Reserve to be maintained
(see Section 8.2.5.1). The Operating Current value shall be less than or
equal to the Maximum Operating Current value.
When the Capabilities Mismatch bit is set to zero the requested Maximum 
Operating
Current shall be less than or equal to the current in the offered Source 
Capabilities
since the Source will need to reserve this power for future use. ...
When the Capabilities 

Re: [PATCH] usb: roles: intel_xhci: Don't use PMIC for port type identification

[Hans de Goede]

Hi,

On 04/20/2018 04:35 PM, Heikki Krogerus wrote:

On Fri, Apr 20, 2018 at 11:16:05AM +0200, Hans de Goede wrote:

Hi Heikki,

On 20-04-18 10:06, Heikki Krogerus wrote:

This will add an array of known USB Type-C Port devices that
will be used as a blacklist for enabling userspace-control,
and remove the PMIC ACPI HID which was used for the same
purpose.

It turns out that on some CHT based platforms the X-Powers
PMIC is handled in firmware. The ACPI HID for it is
therefore not usable for determining the port type. The
driver now searches for known USB Type-C port devices
instead.

Signed-off-by: Heikki Krogerus 
---
Hi Hans,

So it seems that we can't rely on the PMIC. This is my proposal for a
fix. I'm in practice just using blacklist instead of whitelist for
identifying the port type.

Let me know if it's OK to you.


I'm afraid that this is not going to work, almost all CHT devices
(and some BYT devices to) define an INT33FE device, here is a quick grep
on a directory where I store dsdt files from various CHT and BYT devices:

[hans@shalem ~]$ ls /home/archive/hwinfo | wc -l
64
[hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l
36

So 36 out of 64 devices define an INT33FE device and at least half
of the devices there are BYT.


That does not tell you anything. You need to check the status of the
device to know if it's there or not:

 % grep . /sys/bus/acpi/devices/INT33FE*/status


The INT33FE device is described as a "XPOWER Battery Device", so I'm
not sure why you are checking for this to determine if the Type-C
connector is controlled by firmware.


Well, INT33F4 has DOS Device Name set to "XPOWER PMIC Controller". It
does not give any more hints regarding the USB connector.


INT33F4 is the AXP288 PMIC controller and in general boards with that
PMIC do not have Type-C functionality, as described in my
list of devices some devices do have a Type-C connector, but they
use it as a glorified micro-usb connector.


I only used INT33FE quite simply because it is the HID used in the
driver that populates the device for fusb302.


INT33FE is some weird messed up PMIC co-device used for battery
monitoring combined with a proprietary opregion which is registered
by the INT33FE driver.

The driver registering the fusb302 device has the following checks
to make sure the INT33FE it is dealing with is paired with a
Whiskey Cove PMIC:


status = acpi_evaluate_integer(ACPI_HANDLE(dev), "PTYP", NULL, );
if (ACPI_FAILURE(status)) {
dev_err(dev, "Error getting PTYPE\n");
return -ENODEV;
}

/*
 * The same ACPI HID is used for different configurations check PTYP
 * to ensure that we are dealing with the expected config.
 */
if (ptyp != EXPECTED_PTYPE)
return -ENODEV;

/* Check presence of INT34D3 (hardware-rev 3) expected for ptype == 4 */
if (!acpi_dev_present("INT34D3", "1", 3)) {
dev_err(dev, "Error PTYPE == %d, but no INT34D3 device\n",
EXPECTED_PTYPE);
return -ENODEV;
}

At least for CHT there seems to be a 1:1 relationship between which PMIC
is used and if Type-C functionality is available.




But I can see now that _HID INT33FE is in practice used as the
identifier for a "type" of devices instead of identifier for specific
IP. That to be honest is pretty scary. _CID could have been used for
that purpose, but every IP should have its own _HID. It's of course
too late to complain about that.

INT33FE does not indeed seem like usable for this case.


I have a bunch of CHT devices with a Type-C connector:

GPD win:

This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller,
pi3usb30532. Which is all driven by the tcpm code.

Chuwi Hi8 Pro and Chuwi Vi8 Plus:
-
These use a Type-C connector as a glorified micro-usb connector,
the Hi8 Pro supports Superspeed using a hardwired asmedia switch
to deal with upright / upside-down insertion. The Vi8 Plus is
USB2 only.

Host/device mode is determine by treating the Cc pin as an id-pin
(it has a gpio connected which is either low or high depending
on the Cc being pulled up/down).

These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger
detection there is no PD and no 3A / 1.5A / 0.5A detection based
on the Cc pull up resistor, resulting in charging at 0.5 A
when using a Type-C charger which does not do BC1.2 on its
USB2 lines.

As said these really treat the Type-C connector as a micro-sub
connector, so we need userspace control to be able to switch
to host mode when using an otg charging-hub (so that the user can
still use devices attached to the hub while charging).

Asus T100HA and Lenovo Ideapad Miix 320:

These have an USB host only Type-C connector, which does
do Superspeed (likely contain a fixed switch to deal 

Re: [PATCH v5 5/5] usb: typec: tcpm: remove max_snk_mv/ma/mw

[Heikki Krogerus]

On Mon, Apr 16, 2018 at 02:54:37PM +0800, Li Jun wrote:
> Since there is no user of max_snk_*, so we can remove them from tcpm.
> 
> Reviewed-by: Hans de Goede 
> Signed-off-by: Li Jun 

Acked-by: Heikki Krogerus 

> ---
>  drivers/usb/typec/tcpm.c | 12 
>  include/linux/usb/tcpm.h |  9 -
>  2 files changed, 21 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 048b953..27192083 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -257,9 +257,6 @@ struct tcpm_port {
>   u32 snk_vdo[VDO_MAX_OBJECTS];
>   unsigned int nr_snk_vdo;
>  
> - unsigned int max_snk_mv;
> - unsigned int max_snk_ma;
> - unsigned int max_snk_mw;
>   unsigned int operating_snk_mw;
>  
>   /* Requested current / voltage */
> @@ -3598,9 +3595,6 @@ EXPORT_SYMBOL_GPL(tcpm_update_source_capabilities);
>  
>  int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> unsigned int nr_pdo,
> -   unsigned int max_snk_mv,
> -   unsigned int max_snk_ma,
> -   unsigned int max_snk_mw,
> unsigned int operating_snk_mw)
>  {
>   if (tcpm_validate_caps(port, pdo, nr_pdo))
> @@ -3608,9 +3602,6 @@ int tcpm_update_sink_capabilities(struct tcpm_port 
> *port, const u32 *pdo,
>  
>   mutex_lock(>lock);
>   port->nr_snk_pdo = tcpm_copy_pdos(port->snk_pdo, pdo, nr_pdo);
> - port->max_snk_mv = max_snk_mv;
> - port->max_snk_ma = max_snk_ma;
> - port->max_snk_mw = max_snk_mw;
>   port->operating_snk_mw = operating_snk_mw;
>  
>   switch (port->state) {
> @@ -3676,9 +3667,6 @@ struct tcpm_port *tcpm_register_port(struct device 
> *dev, struct tcpc_dev *tcpc)
>   port->nr_snk_vdo = tcpm_copy_vdos(port->snk_vdo, tcpc->config->snk_vdo,
> tcpc->config->nr_snk_vdo);
>  
> - port->max_snk_mv = tcpc->config->max_snk_mv;
> - port->max_snk_ma = tcpc->config->max_snk_ma;
> - port->max_snk_mw = tcpc->config->max_snk_mw;
>   port->operating_snk_mw = tcpc->config->operating_snk_mw;
>   if (!tcpc->config->try_role_hw)
>   port->try_role = tcpc->config->default_role;
> diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h
> index f0d839d..f5bda9a 100644
> --- a/include/linux/usb/tcpm.h
> +++ b/include/linux/usb/tcpm.h
> @@ -62,9 +62,6 @@ enum tcpm_transmit_type {
>   * @snk_pdo: PDO parameters sent to partner as response to
>   *   PD_CTRL_GET_SINK_CAP message
>   * @nr_snk_pdo:  Number of entries in @snk_pdo
> - * @max_snk_mv:  Maximum acceptable sink voltage in mV
> - * @max_snk_ma:  Maximum sink current in mA
> - * @max_snk_mw:  Maximum required sink power in mW
>   * @operating_snk_mw:
>   *   Required operating sink power in mW
>   * @type:Port type (TYPEC_PORT_DFP, TYPEC_PORT_UFP, or
> @@ -85,9 +82,6 @@ struct tcpc_config {
>   const u32 *snk_vdo;
>   unsigned int nr_snk_vdo;
>  
> - unsigned int max_snk_mv;
> - unsigned int max_snk_ma;
> - unsigned int max_snk_mw;
>   unsigned int operating_snk_mw;
>  
>   enum typec_port_type type;
> @@ -174,9 +168,6 @@ int tcpm_update_source_capabilities(struct tcpm_port 
> *port, const u32 *pdo,
>   unsigned int nr_pdo);
>  int tcpm_update_sink_capabilities(struct tcpm_port *port, const u32 *pdo,
> unsigned int nr_pdo,
> -   unsigned int max_snk_mv,
> -   unsigned int max_snk_ma,
> -   unsigned int max_snk_mw,
> unsigned int operating_snk_mw);
>  
>  void tcpm_vbus_change(struct tcpm_port *port);
> -- 
> 2.7.4

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

Re: [PATCH v5 4/5] usb: typec: wcove: remove max_snk_* for sink config

[Heikki Krogerus]

On Mon, Apr 16, 2018 at 02:54:36PM +0800, Li Jun wrote:
> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> variable PDO for sink config.
> 
> Reviewed-by: Hans de Goede 
> Signed-off-by: Li Jun 

Acked-by: Heikki Krogerus 

> ---
>  drivers/usb/typec/typec_wcove.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/typec/typec_wcove.c b/drivers/usb/typec/typec_wcove.c
> index 19cca7f..39cff11 100644
> --- a/drivers/usb/typec/typec_wcove.c
> +++ b/drivers/usb/typec/typec_wcove.c
> @@ -558,6 +558,7 @@ static const u32 src_pdo[] = {
>  static const u32 snk_pdo[] = {
>   PDO_FIXED(5000, 500, PDO_FIXED_DUAL_ROLE | PDO_FIXED_DATA_SWAP |
> PDO_FIXED_USB_COMM),
> + PDO_VAR(5000, 12000, 3000),
>  };
>  
>  static struct tcpc_config wcove_typec_config = {
> @@ -566,9 +567,6 @@ static struct tcpc_config wcove_typec_config = {
>   .snk_pdo = snk_pdo,
>   .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
>  
> - .max_snk_mv = 12000,
> - .max_snk_ma = 3000,
> - .max_snk_mw = 36000,
>   .operating_snk_mw = 15000,
>  
>   .type = TYPEC_PORT_DRP,

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

Re: [PATCH v5 2/5] usb: typec: fusb302: remove max_snk_* for sink config

[Heikki Krogerus]

On Mon, Apr 16, 2018 at 02:54:34PM +0800, Li Jun wrote:
> Since max_snk_* is to be deprecated, so remove max_snk_* by adding a
> variable PDO for sink config.
> 
> Reviewed-by: Hans de Goede 
> Signed-off-by: Li Jun 

Acked-by: Heikki Krogerus 

> ---
>  drivers/usb/typec/fusb302/fusb302.c | 42 
> -
>  1 file changed, 28 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/usb/typec/fusb302/fusb302.c 
> b/drivers/usb/typec/fusb302/fusb302.c
> index 7036171..664463d 100644
> --- a/drivers/usb/typec/fusb302/fusb302.c
> +++ b/drivers/usb/typec/fusb302/fusb302.c
> @@ -120,6 +120,7 @@ struct fusb302_chip {
>   enum typec_cc_polarity cc_polarity;
>   enum typec_cc_status cc1;
>   enum typec_cc_status cc2;
> + u32 snk_pdo[PDO_MAX_OBJECTS];
>  
>  #ifdef CONFIG_DEBUG_FS
>   struct dentry *dentry;
> @@ -1212,11 +1213,6 @@ static const u32 snk_pdo[] = {
>  static const struct tcpc_config fusb302_tcpc_config = {
>   .src_pdo = src_pdo,
>   .nr_src_pdo = ARRAY_SIZE(src_pdo),
> - .snk_pdo = snk_pdo,
> - .nr_snk_pdo = ARRAY_SIZE(snk_pdo),
> - .max_snk_mv = 5000,
> - .max_snk_ma = 3000,
> - .max_snk_mw = 15000,
>   .operating_snk_mw = 2500,
>   .type = TYPEC_PORT_DRP,
>   .data = TYPEC_PORT_DRD,
> @@ -1756,6 +1752,29 @@ static int init_gpio(struct fusb302_chip *chip)
>   return 0;
>  }
>  
> +static int fusb302_composite_snk_pdo_array(struct fusb302_chip *chip)
> +{
> + struct device *dev = chip->dev;
> + u32 max_uv, max_ua;
> +
> + chip->snk_pdo[0] = PDO_FIXED(5000, 400, PDO_FIXED_FLAGS);
> +
> + /*
> +  * As max_snk_ma/mv/mw is not needed for tcpc_config,
> +  * those settings should be passed in via sink PDO, so
> +  * "fcs, max-sink-*" properties will be deprecated, to
> +  * perserve compatibility with existing users of them,
> +  * we read those properties to convert them to be a var
> +  * PDO.
> +  */
> + if (device_property_read_u32(dev, "fcs,max-sink-microvolt", _uv) ||
> + device_property_read_u32(dev, "fcs,max-sink-microamp", _ua))
> + return 1;
> +
> + chip->snk_pdo[1] = PDO_VAR(5000, max_uv / 1000, max_ua / 1000);
> + return 2;
> +}
> +
>  static int fusb302_probe(struct i2c_client *client,
>const struct i2c_device_id *id)
>  {
> @@ -1784,18 +1803,13 @@ static int fusb302_probe(struct i2c_client *client,
>   chip->tcpc_dev.config = >tcpc_config;
>   mutex_init(>lock);
>  
> - if (!device_property_read_u32(dev, "fcs,max-sink-microvolt", ))
> - chip->tcpc_config.max_snk_mv = v / 1000;
> -
> - if (!device_property_read_u32(dev, "fcs,max-sink-microamp", ))
> - chip->tcpc_config.max_snk_ma = v / 1000;
> -
> - if (!device_property_read_u32(dev, "fcs,max-sink-microwatt", ))
> - chip->tcpc_config.max_snk_mw = v / 1000;
> -
>   if (!device_property_read_u32(dev, "fcs,operating-sink-microwatt", ))
>   chip->tcpc_config.operating_snk_mw = v / 1000;
>  
> + /* Composite sink PDO */
> + chip->tcpc_config.nr_snk_pdo = fusb302_composite_snk_pdo_array(chip);
> + chip->tcpc_config.snk_pdo = chip->snk_pdo;
> +
>   /*
>* Devicetree platforms should get extcon via phandle (not yet
>* supported). On ACPI platforms, we get the name from a device prop.
> -- 
> 2.7.4

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

Re: [PATCH v5 1/5] usb: typec: tcpm: pdo matching optimization

[Heikki Krogerus]

Hi,

On Mon, Apr 16, 2018 at 02:54:33PM +0800, Li Jun wrote:
> This patch is a combination of commit 57e6f0d7b804
> ("typec: tcpm: Only request matching pdos") and source
> pdo selection optimization based on it, instead of only
> compare between the same pdo type of sink and source,
> we should check source pdo voltage range is within the
> voltage range of one sink pdo.
> 
> Reviewed-by: Hans de Goede 
> Signed-off-by: Li Jun 

I don't see any problem with this, but I hope Guenter has time to
check it.

Acked-by: Heikki Krogerus 

> ---
>  drivers/usb/typec/tcpm.c | 105 
> ++-
>  1 file changed, 67 insertions(+), 38 deletions(-)
> 
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index 677d121..048b953 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1772,39 +1772,63 @@ static int tcpm_pd_check_request(struct tcpm_port 
> *port)
>   return 0;
>  }
>  
> -static int tcpm_pd_select_pdo(struct tcpm_port *port)
> +#define min_power(x, y) min(pdo_max_power(x), pdo_max_power(y))
> +#define min_current(x, y) min(pdo_max_current(x), pdo_max_current(y))
> +
> +static int tcpm_pd_select_pdo(struct tcpm_port *port, int *sink_pdo,
> +   int *src_pdo)
>  {
> - unsigned int i, max_mw = 0, max_mv = 0;
> + unsigned int i, j, max_src_mv = 0, min_src_mv = 0, max_mw = 0,
> +  max_mv = 0, src_mw = 0, src_ma = 0, max_snk_mv = 0,
> +  min_snk_mv = 0;

Don't resend because of this comment, but just for the record, you are
declaring here so many variables at once that it is not easy to see
what you are introducing (at least for me). It is OK to declare every
variable even separately.

>   int ret = -EINVAL;
>  
>   /*
> -  * Select the source PDO providing the most power while staying within
> -  * the board's voltage limits. Prefer PDO providing exp
> +  * Select the source PDO providing the most power which has a
> +  * matchig sink cap.
>*/
>   for (i = 0; i < port->nr_source_caps; i++) {
>   u32 pdo = port->source_caps[i];
>   enum pd_pdo_type type = pdo_type(pdo);
> - unsigned int mv, ma, mw;
>  
> - if (type == PDO_TYPE_FIXED)
> - mv = pdo_fixed_voltage(pdo);
> - else
> - mv = pdo_min_voltage(pdo);
> + if (type == PDO_TYPE_FIXED) {
> + max_src_mv = pdo_fixed_voltage(pdo);
> + min_src_mv = max_src_mv;
> + } else {
> + max_src_mv = pdo_max_voltage(pdo);
> + min_src_mv = pdo_min_voltage(pdo);
> + }
>  
>   if (type == PDO_TYPE_BATT) {
> - mw = pdo_max_power(pdo);
> + src_mw = pdo_max_power(pdo);
>   } else {
> - ma = min(pdo_max_current(pdo),
> -  port->max_snk_ma);
> - mw = ma * mv / 1000;
> + src_ma = pdo_max_current(pdo);
> + src_mw = src_ma * min_src_mv / 1000;
>   }
>  
> - /* Perfer higher voltages if available */
> - if ((mw > max_mw || (mw == max_mw && mv > max_mv)) &&
> - mv <= port->max_snk_mv) {
> - ret = i;
> - max_mw = mw;
> - max_mv = mv;
> + for (j = 0; j < port->nr_snk_pdo; j++) {
> + pdo = port->snk_pdo[j];
> +
> + if (pdo_type(pdo) == PDO_TYPE_FIXED) {
> + min_snk_mv = pdo_fixed_voltage(pdo);
> + max_snk_mv = pdo_fixed_voltage(pdo);
> + } else {
> + min_snk_mv = pdo_min_voltage(pdo);
> + max_snk_mv = pdo_max_voltage(pdo);
> + }
> +
> + if (max_src_mv <= max_snk_mv &&
> + min_src_mv >= min_snk_mv) {
> + /* Prefer higher voltages if available */
> + if ((src_mw == max_mw && min_src_mv > max_mv) ||
> + src_mw > max_mw) {
> + *src_pdo = i;
> + *sink_pdo = j;
> + max_mw = src_mw;
> + max_mv = min_src_mv;
> + ret = 0;
> + }
> + }
>   }
>   }
>  
> @@ -1816,13 +1840,16 @@ static int tcpm_pd_build_request(struct tcpm_port 
> *port, u32 *rdo)
>   unsigned int mv, ma, mw, flags;
>   unsigned int max_ma, max_mw;
>   enum pd_pdo_type type;
> - int index;
> 

Re: [PATCH v5 3/5] dt-bindings: usb: fusb302: remove max-sink-* properties

[Heikki Krogerus]

On Mon, Apr 16, 2018 at 02:54:35PM +0800, Li Jun wrote:
> Remove max-sink-* properties since they are deprecated.
> 
> Reviewed-by: Rob Herring 
> Reviewed-by: Hans de Goede 
> Signed-off-by: Li Jun 

FWIW:

Revieved-by: Heikki Krogerus 

> ---
>  Documentation/devicetree/bindings/usb/fcs,fusb302.txt | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt 
> b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> index 472facf..6087dc7 100644
> --- a/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> +++ b/Documentation/devicetree/bindings/usb/fcs,fusb302.txt
> @@ -6,12 +6,6 @@ Required properties :
>  - interrupts : Interrupt specifier
>  
>  Optional properties :
> -- fcs,max-sink-microvolt : Maximum voltage to negotiate when configured as 
> sink
> -- fcs,max-sink-microamp  : Maximum current to negotiate when configured as 
> sink
> -- fcs,max-sink-microwatt : Maximum power to negotiate when configured as sink
> -If this is less then max-sink-microvolt *
> -max-sink-microamp then the configured current will
> -be clamped.
>  - fcs,operating-sink-microwatt :
>  Minimum amount of power accepted from a sink
>  when negotiating
> -- 
> 2.7.4

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

Re: [PATCH 2/4] usb: gadget: composite: add function to increment delayed_status

[Roger Quadros]

Hi,

On 19/04/18 22:42, Laurent Pinchart wrote:
> Hi Paul,
> 
> (CC'ing Felipe Balbi and Roger Quadros)
> 
> Thank you for the patch.
> 
> Have you used scripts/get_maintainer.pl ? It should point you to Felipe 
> Balbi, 
> the maintainer of the USB gadget subsystem, who I recommend you CC, at least 
> on patch 2/4. The script also points to Greg, who I don't think needs to be 
> CC'ed as he doesn't deal with USB gadget as much as with USB in general, and 
> who is fairly busy as usual.
> 
> While the get_maintainer script doesn't point to Roger, his name can be found 
> as the author of the USB_GADGET_DELAYED_STATUS mechanism. He authored commit 
> 1b9ba000177e ("usb: gadget: composite: Allow function drivers to pause 
> control 
> transfers") with his nokia.com address back then, but git log --author 'Roger 
> Quadros' shows that he's still active on USB and working for TI now. I've 
> thus 
> CC'ed him on this reply.
> 
> On Wednesday, 18 April 2018 06:18:14 EEST Paul Elder wrote:
>> The completion of the usb status phase from uvc_function_set_alt needs
>> to be delayed until uvc_v4l2_streamon/off. This is currently done by
>> uvc_function_set_alt returning USB_GADGET_DELAYED_STATUS and
>> composite_setup detecting this to increment cdev->delayed_status.
>> However, if uvc_v4l2_streamon/off is called in between this return and
>> increment, uvc_function_setup_continue within uvc_v4l2_streamon/off will
>> WARN that cdev->delayed_status is zero.
> 
> While this is correct, I wouldn't mention UVC here as the patch is for the 
> USB 
> composite gadget framework and isn't specific to UVC. You should write a more 
> generic explanation of the problem to explain why the race between returning 
> USB_GADGET_DELAYED_STATUS (and processing it in the caller) and calling 
> usb_composite_setup_continue() can't be properly solved in gadget drivers, 
> thus requiring a new function.
> 
>> To fix situations like this, add a function to increment
>> cdev->delayed_status.
>>
>> Signed-off-by: Paul Elder 
>> ---
>>  drivers/usb/gadget/composite.c | 6 ++
>>  include/linux/usb/composite.h  | 2 ++
>>  2 files changed, 8 insertions(+)
>>
>> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
>> index 77c7ecca816a..c02ab640a7ae 100644
>> --- a/drivers/usb/gadget/composite.c
>> +++ b/drivers/usb/gadget/composite.c
>> @@ -1548,6 +1548,12 @@ static int fill_ext_prop(struct usb_configuration *c,
>> int interface, u8 *buf) return 0;
>>  }
>>
>> +void usb_composite_setup_delay(struct usb_composite_dev *cdev)
>> +{
>> +cdev->delayed_status++;
> 
> According to include/linux/usb/composite.h, the delayed_status field should 
> be 
> protected by cdev->lock, which you should use here.
> 
> I've read through the code and found out that, while all callers of 
> reset_config(), as well as usb_composite_setup_continue(), correctly take the 
> lock, it isn't taken around f->set_alt() in composite_setup(). This causes 
> the 
> race condition. I wonder if a simpler fix wouldn't be to take the lock before 
> calling f->set_alt() and releasing it after incrementing delayed_status. I am 
> however worried that this could lead to deadlocks if one of the existing 
> set_alt() handlers calls a function that takes the same lock. Another worry 
> is that some of the .set_alt() handlers might not expect to be called with 
> interrupts disabled. This should be analyzed, and I hope that Roger and/or 
> Felipe will have some insight on this.

Yes, I too think it is necessary to serialize .set_alt() and 
usb_composite_setup_continue().

> 
> If usb_composite_setup_delay() turns out to be the preferred solution, it 
> would be nice to document the function with kerneldoc.
> 
> Finally, I just came to wonder whether the UVC gadget driver really needs to 
> defer the status phase of the SET_INTERFACE request, or if it couldn't just 
> proceed normally.

Might be worth a try :)

> 
>> +}
>> +EXPORT_SYMBOL(usb_composite_setup_delay);
>> +
>>  /*
>>   * The setup() callback implements all the ep0 functionality that's
>>   * not handled lower down, in hardware or the hardware driver(like
>> diff --git a/include/linux/usb/composite.h b/include/linux/usb/composite.h
>> index cef0e44601f8..049f77a4d42b 100644
>> --- a/include/linux/usb/composite.h
>> +++ b/include/linux/usb/composite.h
>> @@ -524,6 +524,8 @@ extern int composite_setup(struct usb_gadget *gadget,
>>  extern void composite_suspend(struct usb_gadget *gadget);
>>  extern void composite_resume(struct usb_gadget *gadget);
>>
>> +extern void usb_composite_setup_delay(struct usb_composite_dev *c);
>> +
>>  /*
>>   * Some systems will need runtime overrides for the  product identifiers
>>   * published in the device descriptor, either numbers or strings or both.
> 

-- 
cheers,
-roger

Texas Instruments Finland Oy, Porkkalankatu 22, 00180 Helsinki.
Y-tunnus/Business ID: 0615521-4. Kotipaikka/Domicile: Helsinki
--
To unsubscribe from this list: send the 

Re: BUG xhci_hcd usb3 module WARNS with JMicron JMS567 when writing to disk

[Menion]

I think that JMS567 is mostly broken nowadays in Linux UAS
Mine in an Orico multibay encolsure produce a lot of CMD timeout
Recently Orico is just silently disabling in firmware the UAS support
for this bridge (still advertising it on the webpage, usual chinese
behaviour)
Try to disable UAS with usb-storage quirks instead of blacklisting the
entire usb-storage

2018-04-20 16:10 GMT+02:00 Tomas M :
> Hello,
>
> Tested 4.16.2 with the same results. Anything i can do to help find the 
> problem?
>
> Thanks
>
> Tomas
>
> On Mon, Mar 12, 2018 at 8:22 AM, Tomas M  wrote:
>> Hello, I think its UAS:
>> From the dmesg:
>> [  652.335275] usb 1-1.3: Product: SABRENT
>> [  652.335279] usb 1-1.3: Manufacturer: SABRENT
>> [  652.335283] usb 1-1.3: SerialNumber: DB98765432123
>> [  652.337283] scsi host8: uas
>> [  652.338731] scsi 8:0:0:0: Direct-Access SABRENT
>>   4101 PQ: 0 ANSI: 6
>>
>> SABRENT Is the HDD craddle.
>>
>> I have an external Toshiba HDD running on the same computer with the
>> xhci_hcd driver for years without problems. But it reads:
>>  >>> usb-storage 4-1:1.0: USB Mass Storage device detected
>> so i think its fundamentally different.
>>
>> Yes. data corruption ocurred when Oopsing.
>>
>> Like i said, i tried different HDDs and have two HDD Craddles to play
>> with. all combinations brought the same result. Both craddles have
>> SATA-USB3 chipsets from JMicrion. running on a USB2 chipset with the
>> ehci driver works fine.
>>
>> Regards
>>
>> Tomas
>>
>> On Mon, Mar 12, 2018 at 6:52 AM, Oliver Neukum  wrote:
>>> Am Sonntag, den 11.03.2018, 10:48 -0300 schrieb Tomas M:
 Hello,

 I reported the bug here and was suggested to forward it to the ML.
 https://bugzilla.kernel.org/show_bug.cgi?id=199075

 find attached the owed dmesg with the stack trace.

 using the ehci ports produces no error.
 using the xhci ports fails very fast when writing.

 this happens with the
 Bus 001 Device 005: ID 152d:0578 JMicron Technology Corp. / JMicron
 USA Technology Corp. JMS567 SATA 6Gb/s bridge

 and another similar chipset from JMicron too.

 I have another external USB3 drive using the xhci driver without problems.

 Regards
>>>
>>> UAS or storage?
>>>
>>> A CRC error is really unusual and an oops in RCU is arcane.
>>> Do you have errors with other Super Speed devices? This looks
>>> like data corruption.
>>>
>>> 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
--
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 v1 0/2] usb: dwc2: gadget: Fixes for LPM

[Grigor Tovmasyan]

Hi Balbi,

On 4/10/2018 2:21 PM, Grigor Tovmasyan wrote:
> Here are two little fixes for LPM feature.
> 
> First one is coverity warning fix.
> 
> The Second one was asserted by Stefan Wahren.
> 
> Changes from version 0:
> 
> 1/2:
> - Instead of converting parameter in the CHECK_RANGE macro
>   to int, changed hird_threshold type from u8 to int.
> 
> 
> Grigor Tovmasyan (2):
>usb: dwc2: gadget: Fix coverity issue
>usb: dwc2: gadget: Change LPM default values
> 
>   drivers/usb/dwc2/core.h   | 2 +-
>   drivers/usb/dwc2/params.c | 8 
>   2 files changed, 5 insertions(+), 5 deletions(-)
> 

Please don't merge this patch yet. I need to review it again and made 
some updates to submit it as version 2.

Thanks,
Grigor.
--
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: roles: intel_xhci: Don't use PMIC for port type identification

[Heikki Krogerus]

On Fri, Apr 20, 2018 at 11:16:05AM +0200, Hans de Goede wrote:
> Hi Heikki,
> 
> On 20-04-18 10:06, Heikki Krogerus wrote:
> > This will add an array of known USB Type-C Port devices that
> > will be used as a blacklist for enabling userspace-control,
> > and remove the PMIC ACPI HID which was used for the same
> > purpose.
> > 
> > It turns out that on some CHT based platforms the X-Powers
> > PMIC is handled in firmware. The ACPI HID for it is
> > therefore not usable for determining the port type. The
> > driver now searches for known USB Type-C port devices
> > instead.
> > 
> > Signed-off-by: Heikki Krogerus 
> > ---
> > Hi Hans,
> > 
> > So it seems that we can't rely on the PMIC. This is my proposal for a
> > fix. I'm in practice just using blacklist instead of whitelist for
> > identifying the port type.
> > 
> > Let me know if it's OK to you.
> 
> I'm afraid that this is not going to work, almost all CHT devices
> (and some BYT devices to) define an INT33FE device, here is a quick grep
> on a directory where I store dsdt files from various CHT and BYT devices:
> 
> [hans@shalem ~]$ ls /home/archive/hwinfo | wc -l
> 64
> [hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l
> 36
> 
> So 36 out of 64 devices define an INT33FE device and at least half
> of the devices there are BYT.

That does not tell you anything. You need to check the status of the
device to know if it's there or not:

% grep . /sys/bus/acpi/devices/INT33FE*/status

> The INT33FE device is described as a "XPOWER Battery Device", so I'm
> not sure why you are checking for this to determine if the Type-C
> connector is controlled by firmware.

Well, INT33F4 has DOS Device Name set to "XPOWER PMIC Controller". It
does not give any more hints regarding the USB connector.

I only used INT33FE quite simply because it is the HID used in the
driver that populates the device for fusb302.

But I can see now that _HID INT33FE is in practice used as the
identifier for a "type" of devices instead of identifier for specific
IP. That to be honest is pretty scary. _CID could have been used for
that purpose, but every IP should have its own _HID. It's of course
too late to complain about that.

INT33FE does not indeed seem like usable for this case.

> I have a bunch of CHT devices with a Type-C connector:
> 
> GPD win:
> 
> This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller,
> pi3usb30532. Which is all driven by the tcpm code.
> 
> Chuwi Hi8 Pro and Chuwi Vi8 Plus:
> -
> These use a Type-C connector as a glorified micro-usb connector,
> the Hi8 Pro supports Superspeed using a hardwired asmedia switch
> to deal with upright / upside-down insertion. The Vi8 Plus is
> USB2 only.
> 
> Host/device mode is determine by treating the Cc pin as an id-pin
> (it has a gpio connected which is either low or high depending
> on the Cc being pulled up/down).
> 
> These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger
> detection there is no PD and no 3A / 1.5A / 0.5A detection based
> on the Cc pull up resistor, resulting in charging at 0.5 A
> when using a Type-C charger which does not do BC1.2 on its
> USB2 lines.
> 
> As said these really treat the Type-C connector as a micro-sub
> connector, so we need userspace control to be able to switch
> to host mode when using an otg charging-hub (so that the user can
> still use devices attached to the hub while charging).
> 
> Asus T100HA and Lenovo Ideapad Miix 320:
> 
> These have an USB host only Type-C connector, which does
> do Superspeed (likely contain a fixed switch to deal with upright /
> upside-down insertion).
> 
> These cannot charge at all through the Type-C connector (they
> do turn of the 5v boost when used in device mode), theoretically
> the port may work in device mode (depending on which lanes they
> used), but the device-mode USB controller is disabled by the BIOS
> with no option to change it.

In these cases we don't really need the mux. Actually, we probable
should not even register the driver for it in these cases.

> I also have 5 different CHT devices with a micro-usb connector:
> -Cube iWork8 Air
> -Jumper Ezpad mini 3
> -Pipo W2s
> -Teclast X80 pro
> -Lenovo Ideapad Miix 310
> 
> All of which use an AXP288 PMIC and can do charging, host and
> device mode through their micro-usb connector with the
> exception of the Lenovo Ideapad Miix 310 where the micro-usb
> is host mode only (like the Type-C on the 320) and there is
> a separate power-barrel connector for charging.
> 
> All these define an INT33FE device, although at least on
> the Cube and Lenovo ones the INT33FE's device _STA returns 0,
> so your check should not match, but on others the _STA for
> the INT33FE device does return 0x0f.
> 
> TL;DR: Checking the INT33FE device to determine if a Type-C
> connector is present is not a good way to handle this.

Fair 

Re: [PATCH] usb: musb: Support gadget mode when the port is set to dual role

[Bin Liu]

On Thu, Mar 29, 2018 at 01:57:24PM +0200, Paul Kocialkowski wrote:
> Hi,
> 
> On Thu, 2018-03-29 at 11:23 +0200, Maxime Ripard wrote:
> > On Wed, Mar 28, 2018 at 11:52:13PM +0200, Paul Kocialkowski wrote:
> > > This allows dual-role ports to be reported as having gadget mode by
> > > the
> > > musb_has_gadget helper. This is required to enable MUSB at all with
> > > MUSB
> > > glue layers that set the port mode to MUSB_PORT_MODE_DUAL_ROLE at
> > > init.
> > > 
> > > Most notably, this allows calling musb_start when needed in the
> > > virtual
> > > MUSB root HUB, regardless of whether the current mode should be
> > > gadget
> > > or host.
> > > 
> > > This fixes USB OTG on Allwinner devices that I could test it with,
> > > mainly A20 devices.
> > > 
> > > Signed-off-by: Paul Kocialkowski 
> > 
> > Surely there's more to it than that. The gadget mode of A20 boards
> > have been working in the past, including when compiling with mUSB
> > setup as dual role.
> >
> > Is this a regression since a particular commit? Or is there another,
> > deeper issue overlooked in the commit log?
> 
> The root of the issue here is that musb_start is not called at any point
> without this patch. My understanding of the flow is the following: when
> the PHY detects that there was a VBUS/ID change, it will notify its
> listeners (mainly the musb sunxi glue layer). This will then schedule
> the driver's work (sunxi_musb_work), which does nothing since the
> SUNXI_MUSB_FL_ENABLED bit was never set. This bit is only set after
> calling sunxi_musb_enable, which is called from musb_platform_enable,
> that originates from musb_start.
> 
> Currently I see two places where musb_start is called:
> * musb_virthub
> * musb_gadget
> 
> In the latter case, it is in turn called from udc_start, which should
> probably (correct me if I'm wrong) happen later in the call chain than
> ID/VBUS change notification time.

I don't think it is correct that udc_start() is triggered by ID/VBUS
events, but I don't have an Allwinner platform to verify the callflow.

Have you tried to load with a gadget driver? When a gadget function is
bound to UDC, udc_start() is triggered, which in turn calls
musb_start().

> 
> In the former case, musb_start is called in the root controller hub
> control, when setting the USB_PORT_FEAT_POWER feature. This looks
> perfectly legit and IMO this is where it should be initially calling
> musb_start in the dual role case. The kernel is indeed setting the

No actually. A dual-role port should be in b_idle state by default, so
logically all actions should go to the gadget path until the port
switches to host mode.

> feature, only that it fails to enable musb without this patch.
> 
> First, I'd like to make sure that this understanding of the flow is
> correct as I may have missed something here. Does it make sense?

I am guessing you didn't load a gadget driver when testing?

> Then, it seems that the offending commit is: be9d39881fc4f
> ("usb: musb: host: rely on port_mode to call musb_start()")
> 
> That itself fixed: ae44df2e21b5
> ("usb: musb: call musb_start() only once in OTG mode")
> 
> Still, this commit was authored in June 2015, so almost 3 years ago.
> In the meantime, the sunxi driver has received feature improvements, so
> it seems hard to believe that it was broken all this time...
> 
> Cheers,

Regards,
-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 V2] USB: Increment wakeup count on remote wakeup.

[Alan Stern]

On Thu, 19 Apr 2018, Ravi Chandra Sadineni wrote:

> On chromebooks we depend on wakeup count to identify the wakeup source.
> But currently USB devices do not increment the wakeup count when they
> trigger the remote wake. This patch addresses the same.
> 
> Resume condition is reported differently on USB 2.0 and USB 3.0 devices.
> 
> On USB 2.0 devices, a wake capable device, if wake enabled, drives
> resume signal to indicate a remote wake (USB 2.0 spec section 7.1.7.7).
> The upstream facing port then sets C_PORT_SUSPEND bit and reports a
> port change event (USB 2.0 spec section 11.24.2.7.2.3). Thus if a port
> has resumed before driving the resume signal from the host and
> C_PORT_SUSPEND is set, then the device attached to the given port might
> be the reason for the last system wakeup. Increment the wakeup count for
> the same.
> 
> On USB 3.0 devices, a function may signal that it wants to exit from device
> suspend by sending a Function Wake Device Notification to the host (USB3.0
> spec section 8.5.6.4) Thus on receiving the Function Wake, increment the
> wakeup count.
> 
> Signed-off-by: Ravi Chandra Sadineni 
> ---
>  drivers/usb/core/hcd.c |  2 ++
>  drivers/usb/core/hub.c | 10 +-
>  2 files changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 777036ae63674..ee0f3ec57ce49 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -2374,7 +2374,9 @@ static void hcd_resume_work(struct work_struct *work)
>  void usb_hcd_resume_root_hub (struct usb_hcd *hcd)
>  {
>   unsigned long flags;
> + struct device *dev = >self.root_hub->dev;
>  
> + pm_wakeup_event(dev, 0);
>   spin_lock_irqsave (_root_hub_lock, flags);
>   if (hcd->rh_registered) {
>   set_bit(HCD_FLAG_WAKEUP_PENDING, >flags);

In general, hcd->self.root_hub is guaranteed to be non-NULL only when 
hcd->rh_registered is set.  Therefore the assignment to dev and the 
call to pm_wakeup_event() should be moved within this "if" statement.

The rest of the patch looks okay.

Alan Stern

> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
> index f6ea16e9f6bb9..aa9968d90a48c 100644
> --- a/drivers/usb/core/hub.c
> +++ b/drivers/usb/core/hub.c
> @@ -653,12 +653,17 @@ void usb_wakeup_notification(struct usb_device *hdev,
>   unsigned int portnum)
>  {
>   struct usb_hub *hub;
> + struct usb_port *port_dev;
>  
>   if (!hdev)
>   return;
>  
>   hub = usb_hub_to_struct_hub(hdev);
>   if (hub) {
> + port_dev = hub->ports[portnum - 1];
> + if (port_dev && port_dev->child)
> + pm_wakeup_event(_dev->child->dev, 0);
> +
>   set_bit(portnum, hub->wakeup_bits);
>   kick_hub_wq(hub);
>   }
> @@ -3434,8 +3439,11 @@ int usb_port_resume(struct usb_device *udev, 
> pm_message_t msg)
>  
>   /* Skip the initial Clear-Suspend step for a remote wakeup */
>   status = hub_port_status(hub, port1, , );
> - if (status == 0 && !port_is_suspended(hub, portstatus))
> + if (status == 0 && !port_is_suspended(hub, portstatus)) {
> + if (portchange & USB_PORT_STAT_C_SUSPEND)
> + pm_wakeup_event(>dev, 0);
>   goto SuspendCleared;
> + }
>  
>   /* see 7.1.7.7; affects power usage, but not budgeting */
>   if (hub_is_superspeed(hub->hdev))
> 

--
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 RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Guenter Roeck]

On 04/20/2018 03:51 AM, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月20日 17:21
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx

Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

Hi,

On 20-04-18 11:18, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月18日 19:40
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org;
dl-linux-imx 
Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw
for rdo

Hi,

On 17-04-18 08:01, Li Jun wrote:

This patch is a further update for rdo based on [1], which removed
max_snk_ma/mv/mw but kept operating_snk_mw.
operating_snk_mw is only used to judge capability mismatch, per PD
spec, we can achieve this via compare the selected source PDO and
matching sink PDO, also after patch [1], we don't limit the PDO
matching between the same type, so the rdo operating and max
current/power calculation should be updated accordingly.


I do not believe that this is correct, lets take a device with a
fusb302 tcpc with the following PDO-s:

PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
PDO_VAR(5000, 12000, 3000);

And an operating_snk_mw of 2500mW then according to your new code a
charger which supplies 12V 2A will now get the mismatch bit set even
though it is delivering 24W.



Per PD spec, my understanding is to judge capability mismatch, we
should use *max* current/power, not this operating power.


Ok, I'm not familiar with the PD spec, perhaps someone who knows it well can
comment here ?


Hi Guenter,
Could you please comment here?



I am not, and never was, the kind of person who would blindly follow a 
specification.
I use specifications as guideline. For PD, when I implemented the code, I made 
sure
that the implementation worked with as many power adapters and dongles as I 
could get
my hand on.

At this point in time, I would only change the code myself if it is known to be 
broken.
Otherwise I would leave it alone. I would _never_ make a change because my 
interpretation
of the specification is different, unless I can prove that the current code is 
wrong and
causes problems.

Unfortunately, I don't currently have a setup to test the current version of 
the code,
nor the time to do all the testing. I am more and more concerned, though, that 
changes
are made out of principle, not because something has been shown to be broken. 
But that is
just a personal concern, and without re-testing everything I don't really have 
a strong
case for or against those changes.

Guenter


Thanks
Jun




In your case(VAR PDO), the max current is 3000mA, can I say the max
power is 3000mA * 5V = 15W? so the max current required at 12V is 15W
/ 12V = 1.25A, so yes, the 12V@2A is enough, do you think this kind of
calculation is reasonable?


That does not sound unreasonable, the real question is when exactly should we
set the capability mismatch bit, once we have that more clear how to
implement this should become clear too.


Fully agree, below is the capability mismatch bit related description from PD 
spec:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value larger than
the maximum current/power offered in the Source_Capabilities Message’s PDO
as referenced by the Object position field. This enables the Sink to indicate 
that
it requires more current/power than is being offered. If the Sink requires a
different voltage this will be indicated by its Sink_Capabilities Message."

"6.4.2.8 Maximum Operating Current
The Maximum Operating Current field in the Request Message shall be set to
the highest current the Sink will ever require. The difference between
the Operating Current and Maximum Operating Current fields
(when the GiveBack Flag is cleared) is used by the Device Policy Manager
in the Source to calculate the size of the Power Reserve to be maintained
(see Section 8.2.5.1). The Operating Current value shall be less than or
equal to the Maximum Operating Current value.
When the Capabilities Mismatch bit is set to zero the requested Maximum 
Operating
Current shall be less than or equal to the current in the offered Source 
Capabilities
since the Source will need to reserve this power for future use. ...
  
When the Capabilities Mismatch bit is set to one the requested Maximum

Operating Current may be greater than the current in the offered Source
Capabilities since the Source will need this information to ascertain
the Sink’s actual needs".



Regards,

Hans






I really don't see any way to fix this and I believe we should just
keep the operating_snk_mw field.


I had the same worry of breaking existing users, but found there is
something not matching 

Re: BUG xhci_hcd usb3 module WARNS with JMicron JMS567 when writing to disk

[Tomas M]

Hello,

Tested 4.16.2 with the same results. Anything i can do to help find the problem?

Thanks

Tomas

On Mon, Mar 12, 2018 at 8:22 AM, Tomas M  wrote:
> Hello, I think its UAS:
> From the dmesg:
> [  652.335275] usb 1-1.3: Product: SABRENT
> [  652.335279] usb 1-1.3: Manufacturer: SABRENT
> [  652.335283] usb 1-1.3: SerialNumber: DB98765432123
> [  652.337283] scsi host8: uas
> [  652.338731] scsi 8:0:0:0: Direct-Access SABRENT
>   4101 PQ: 0 ANSI: 6
>
> SABRENT Is the HDD craddle.
>
> I have an external Toshiba HDD running on the same computer with the
> xhci_hcd driver for years without problems. But it reads:
>  >>> usb-storage 4-1:1.0: USB Mass Storage device detected
> so i think its fundamentally different.
>
> Yes. data corruption ocurred when Oopsing.
>
> Like i said, i tried different HDDs and have two HDD Craddles to play
> with. all combinations brought the same result. Both craddles have
> SATA-USB3 chipsets from JMicrion. running on a USB2 chipset with the
> ehci driver works fine.
>
> Regards
>
> Tomas
>
> On Mon, Mar 12, 2018 at 6:52 AM, Oliver Neukum  wrote:
>> Am Sonntag, den 11.03.2018, 10:48 -0300 schrieb Tomas M:
>>> Hello,
>>>
>>> I reported the bug here and was suggested to forward it to the ML.
>>> https://bugzilla.kernel.org/show_bug.cgi?id=199075
>>>
>>> find attached the owed dmesg with the stack trace.
>>>
>>> using the ehci ports produces no error.
>>> using the xhci ports fails very fast when writing.
>>>
>>> this happens with the
>>> Bus 001 Device 005: ID 152d:0578 JMicron Technology Corp. / JMicron
>>> USA Technology Corp. JMS567 SATA 6Gb/s bridge
>>>
>>> and another similar chipset from JMicron too.
>>>
>>> I have another external USB3 drive using the xhci driver without problems.
>>>
>>> Regards
>>
>> UAS or storage?
>>
>> A CRC error is really unusual and an oops in RCU is arcane.
>> Do you have errors with other Super Speed devices? This looks
>> like data corruption.
>>
>> Regards
>> Oliver
>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/4] usb: gadget: composite: add function to increment delayed_status

[Alan Stern]

On Thu, 19 Apr 2018, Laurent Pinchart wrote:

> According to include/linux/usb/composite.h, the delayed_status field should 
> be 
> protected by cdev->lock, which you should use here.
> 
> I've read through the code and found out that, while all callers of 
> reset_config(), as well as usb_composite_setup_continue(), correctly take the 
> lock, it isn't taken around f->set_alt() in composite_setup(). This causes 
> the 
> race condition. I wonder if a simpler fix wouldn't be to take the lock before 
> calling f->set_alt() and releasing it after incrementing delayed_status. I am 
> however worried that this could lead to deadlocks if one of the existing 
> set_alt() handlers calls a function that takes the same lock. Another worry 
> is that some of the .set_alt() handlers might not expect to be called with 
> interrupts disabled. This should be analyzed, and I hope that Roger and/or 
> Felipe will have some insight on this.

set_alt handlers generally have to disable and enable endpoints, which 
requires a process context.  They cannot run with interrupts disabled.

Alan Stern

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

[PATCH 1/3] xhci: Fix USB ports for Dell Inspiron 5775

[Mathias Nyman]

From: Kai-Heng Feng 

The Dell Inspiron 5775 is a Raven Ridge. The Enable Slot command timed
out when a USB device gets plugged:
[ 212.156326] xhci_hcd :03:00.3: Error while assigning device slot ID
[ 212.156340] xhci_hcd :03:00.3: Max number of devices this xHCI host 
supports is 64.
[ 212.156348] usb usb2-port3: couldn't allocate usb_device

AMD suggests that a delay before xHC suspends can fix the issue.

I can confirm it fixes the issue, so use the suspend delay quirk for
Raven Ridge's xHC.

Cc: sta...@vger.kernel.org
Signed-off-by: Kai-Heng Feng 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-pci.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/host/xhci-pci.c b/drivers/usb/host/xhci-pci.c
index f17b7ea..85ffda8 100644
--- a/drivers/usb/host/xhci-pci.c
+++ b/drivers/usb/host/xhci-pci.c
@@ -126,7 +126,10 @@ static void xhci_pci_quirks(struct device *dev, struct 
xhci_hcd *xhci)
if (pdev->vendor == PCI_VENDOR_ID_AMD && usb_amd_find_chipset_info())
xhci->quirks |= XHCI_AMD_PLL_FIX;
 
-   if (pdev->vendor == PCI_VENDOR_ID_AMD && pdev->device == 0x43bb)
+   if (pdev->vendor == PCI_VENDOR_ID_AMD &&
+   (pdev->device == 0x15e0 ||
+pdev->device == 0x15e1 ||
+pdev->device == 0x43bb))
xhci->quirks |= XHCI_SUSPEND_DELAY;
 
if (pdev->vendor == PCI_VENDOR_ID_AMD)
-- 
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

[PATCH 3/3] usb: host: xhci-plat: Fix clock resource by adding a register clock

[Mathias Nyman]

From: Gregory CLEMENT 

On Armada 7K/8K we need to explicitly enable the register clock. This
clock is optional because not all the SoCs using this IP need it but at
least for Armada 7K/8K it is actually mandatory.

The change was done at xhci-plat level and not at a xhci-mvebu.c because,
it is expected that other SoC would have this kind of constraint.

The binding documentation is updating accordingly.

Signed-off-by: Gregory CLEMENT 
Signed-off-by: Mathias Nyman 
---
 Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 -
 drivers/usb/host/xhci-plat.c   | 25 ++
 drivers/usb/host/xhci.h|  3 ++-
 3 files changed, 27 insertions(+), 6 deletions(-)

diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt 
b/Documentation/devicetree/bindings/usb/usb-xhci.txt
index c4c00df..bd1dd31 100644
--- a/Documentation/devicetree/bindings/usb/usb-xhci.txt
+++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
@@ -28,7 +28,10 @@ Required properties:
   - interrupts: one XHCI interrupt should be described here.
 
 Optional properties:
-  - clocks: reference to a clock
+  - clocks: reference to the clocks
+  - clock-names: mandatory if there is a second clock, in this case
+the name must be "core" for the first clock and "reg" for the
+second one
   - usb2-lpm-disable: indicate if we don't want to enable USB2 HW LPM
   - usb3-lpm-capable: determines if platform is USB3 LPM capable
   - quirk-broken-port-ped: set if the controller has broken port disable 
mechanism
diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index f0231fe..596e7a7 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -157,6 +157,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
struct resource *res;
struct usb_hcd  *hcd;
struct clk  *clk;
+   struct clk  *reg_clk;
int ret;
int irq;
 
@@ -226,17 +227,27 @@ static int xhci_plat_probe(struct platform_device *pdev)
hcd->rsrc_len = resource_size(res);
 
/*
-* Not all platforms have a clk so it is not an error if the
-* clock does not exists.
+* Not all platforms have clks so it is not an error if the
+* clock do not exist.
 */
+   reg_clk = devm_clk_get(>dev, "reg");
+   if (!IS_ERR(reg_clk)) {
+   ret = clk_prepare_enable(reg_clk);
+   if (ret)
+   goto put_hcd;
+   } else if (PTR_ERR(reg_clk) == -EPROBE_DEFER) {
+   ret = -EPROBE_DEFER;
+   goto put_hcd;
+   }
+
clk = devm_clk_get(>dev, NULL);
if (!IS_ERR(clk)) {
ret = clk_prepare_enable(clk);
if (ret)
-   goto put_hcd;
+   goto disable_reg_clk;
} else if (PTR_ERR(clk) == -EPROBE_DEFER) {
ret = -EPROBE_DEFER;
-   goto put_hcd;
+   goto disable_reg_clk;
}
 
xhci = hcd_to_xhci(hcd);
@@ -252,6 +263,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
device_wakeup_enable(hcd->self.controller);
 
xhci->clk = clk;
+   xhci->reg_clk = reg_clk;
xhci->main_hcd = hcd;
xhci->shared_hcd = __usb_create_hcd(driver, sysdev, >dev,
dev_name(>dev), hcd);
@@ -322,6 +334,9 @@ static int xhci_plat_probe(struct platform_device *pdev)
 disable_clk:
clk_disable_unprepare(clk);
 
+disable_reg_clk:
+   clk_disable_unprepare(reg_clk);
+
 put_hcd:
usb_put_hcd(hcd);
 
@@ -337,6 +352,7 @@ static int xhci_plat_remove(struct platform_device *dev)
struct usb_hcd  *hcd = platform_get_drvdata(dev);
struct xhci_hcd *xhci = hcd_to_xhci(hcd);
struct clk *clk = xhci->clk;
+   struct clk *reg_clk = xhci->reg_clk;
 
xhci->xhc_state |= XHCI_STATE_REMOVING;
 
@@ -347,6 +363,7 @@ static int xhci_plat_remove(struct platform_device *dev)
usb_put_hcd(xhci->shared_hcd);
 
clk_disable_unprepare(clk);
+   clk_disable_unprepare(reg_clk);
usb_put_hcd(hcd);
 
pm_runtime_set_suspended(>dev);
diff --git a/drivers/usb/host/xhci.h b/drivers/usb/host/xhci.h
index 05c909b..6dfc486 100644
--- a/drivers/usb/host/xhci.h
+++ b/drivers/usb/host/xhci.h
@@ -1729,8 +1729,9 @@ struct xhci_hcd {
int page_shift;
/* msi-x vectors */
int msix_count;
-   /* optional clock */
+   /* optional clocks */
struct clk  *clk;
+   struct clk  *reg_clk;
/* data structures */
struct xhci_device_context_array *dcbaa;
struct xhci_ring*cmd_ring;
-- 
2.7.4

--
To unsubscribe from this list: send the line 

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

[Mathias Nyman]

Hi Greg

A few fixes to get xhci on Aramada 7K/8K and Dell Inspiron 5775 to work
properly

Armada changes are needed for 4.17, but not stable if I understand
Gregory CLEMENT correctly:

"I've just realized that this series sent 2 months ago was not merged in
v4.17. The issue is that now the USB support on the Armada 7K/8K is
broken, because the clock support part was already merged."

Gregory CLEMENT (2):
  usb: host: xhci-plat: Remove useless test before clk_disable_unprepare
  usb: host: xhci-plat: Fix clock resource by adding a register clock

Kai-Heng Feng (1):
  xhci: Fix USB ports for Dell Inspiron 5775

 Documentation/devicetree/bindings/usb/usb-xhci.txt |  5 +++-
 drivers/usb/host/xhci-pci.c|  5 +++-
 drivers/usb/host/xhci-plat.c   | 31 --
 drivers/usb/host/xhci.h|  3 ++-
 4 files changed, 33 insertions(+), 11 deletions(-)

-- 
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

[PATCH 2/3] usb: host: xhci-plat: Remove useless test before clk_disable_unprepare

[Mathias Nyman]

From: Gregory CLEMENT 

clk_disable_unprepare() already checks that the clock pointer is valid.
No need to test it before calling it.

Signed-off-by: Gregory CLEMENT 
Signed-off-by: Mathias Nyman 
---
 drivers/usb/host/xhci-plat.c | 6 ++
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
index df327dc..f0231fe 100644
--- a/drivers/usb/host/xhci-plat.c
+++ b/drivers/usb/host/xhci-plat.c
@@ -320,8 +320,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
usb_put_hcd(xhci->shared_hcd);
 
 disable_clk:
-   if (!IS_ERR(clk))
-   clk_disable_unprepare(clk);
+   clk_disable_unprepare(clk);
 
 put_hcd:
usb_put_hcd(hcd);
@@ -347,8 +346,7 @@ static int xhci_plat_remove(struct platform_device *dev)
usb_remove_hcd(hcd);
usb_put_hcd(xhci->shared_hcd);
 
-   if (!IS_ERR(clk))
-   clk_disable_unprepare(clk);
+   clk_disable_unprepare(clk);
usb_put_hcd(hcd);
 
pm_runtime_set_suspended(>dev);
-- 
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: [RFT/PATCH 00/38] usb: dwc3: gadget: Rework & Refactoring

[Bin Liu]

On Fri, Apr 20, 2018 at 01:57:23PM +0300, Felipe Balbi wrote:
> 
> Hi,
> 
> Bin Liu  writes:
> >> Felipe Balbi  writes:
> >> >>> Bin Liu  writes:
> >> >>> > On Mon, Apr 09, 2018 at 02:26:14PM +0300, Felipe Balbi wrote:
> >> >>> >> Hi guys,
> >> >>> >> 
> >> >>> >> I've been working on this series for a while now. I feels like after
> >> >>> >> this series the transfer management code is far easier to read and
> >> >>> >> understand.
> >> >>> >> 
> >> >>> >> Based on my tests, I have no regressions. Tested g_mass_storage and 
> >> >>> >> all
> >> >>> >> of testusb's tests (including ISOC).
> >> >>> >> 
> >> >>> >> Patches are also available on dwc3-improve-isoc-endpoints in my 
> >> >>> >> k.org
> >> >>> >> tree. Test reports would be VERY, VERY, VERY welcome. Please give 
> >> >>> >> this a
> >> >>> >> go so we avoid regressions on v4.18.
> >> >>> >
> >> >>> > Have you tested g_webcam? I just tested your 
> >> >>> > dwc3-improve-isoc-endpoints
> >> >>> 
> >> >>> for isoc I only tested g_zero.
> >> >>
> >> >> Then writting down details on what I observed probably won't help you :(
> >> 
> >> I got webcam running here, it sure _is_ helpful to let me know how you
> >
> > Great!
> >
> >> trigger the problem ;-) Also, high-speed or super-speed?
> >
> > Both. Long story short - super-speed doesn't stream the yuv video,
> > gadget side kernel prints "VS request completed with status -18."
> > flooding messages.
> >
> > high-speed does stream the video, but stopping the host application
> > (both yavta and luvcview) causes gadget side kernel prints error message
> > (I didn't keep the log). Then restart the host application doesn't
> > stream the video any more.
> >
> > To run the test:
> > gadget side:
> > # modprobe g_webcam (streaming_maxpacket=3072 for super-speed)
> > # uvc-gadget
> > host side:
> > $ yavta -c -f YUYV -t 1/30 -s 640x360 -n4 /dev/video1, or
> > $ luvcview -d /dev/video1 -f yuv
> 
> okay, found the problem. This is actually a problem on webcam gadget
> which kills the stream in case of Missed Isoc. The reason why it "works"
> with dwc3 today is because dwc3, up until now, is really harsh whenever
> we miss and interval.
> 
> Currently we stop the transfer and wait for the next XferNotReady. This
> may cause us to loose many frames.
> 
> I've been talking with Laurent Pinchart (in Cc) about what would be the
> best way to sort this out and, our conclusion, is that webcam gadget
> should have a way for a single buffer to be treated as erroneous, but it
> shouldn't stop the stream.
> 
> There's also another error in webcam gadget where default interval,
> maxburst and maxpacket aren't very good for HS or SS.
> 
> Note that streaming_interval is, by default, 1. Which for FS means 1ms,
> but for HS/SS it means 1 * 125us. So host side is actually polling
> webcam gadget every uFrame. I got webcam gadget to behave really well
> with streaming_interval=4 streaming_maxburst=16
> streaming_maxpacket=3072. With that, in SS, I can get around 100
> fps. There are a few cases when FPS drops to 33, but that's because it
> coincides with a missed isoc and webcam stops and restarts the stream.

Great!
Drop me an email whenever it is ready to re-test, in case I missed any
related conversation/patch in the mailing list.

Regards,
-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 v2 0/3] usb: dwc2: gadget: Update ISOC DDMA flow.

[Minas Harutyunyan]

Hi Filipe,

On 3/29/2018 6:28 PM, Minas Harutyunyan wrote:
> This series fully update existing ISOC DDMA flow which initially based on
> 2 descriptor chains. Switching between desc chains performing based on BNA
> interrupt. Because of BNA interrupt few packets can be lost.
> 
> 1/3 patch unmask ISOC IN BNA interrupt only.
> 2/3 patch changing ISOC IN/OUT flow as described above.
> 3/3 patch add High bandwidth ISOC OUT transfers support.
> 
> All patches were tested on HAPS-DX7 platform using internal USB test tool.
> 
> Changes from version 1:
> 
> In patch 2/3 removed redundant warning print in dwc2_hsotg_ep_queue()
> (Zengtao (B) )
> 
> In patch 2/3 replace loop counter from 'ret' to 'i' in
> dwc2_gadget_start_isoc_ddma() (Zengtao (B) )
> 
> In patch 3/3 fix 2 kbuild test robot warnings.
> 
> In patch 3/3 updated target_frame number width from 11-bit to 14-bit.
> 
> In patches 2/3 and 3/3 updated all requests completion status codes to 0
> instead of error codes (ENODATA, EIO, ETIMEDOUT). In case of errors,
> set request actual length to 0, to keep consistency with original code.
> 
> Changes from version 0:
> 
> Fix kbuild test robot warnings on idents.
> 
> 
> Minas Harutyunyan (3):
>usb: dwc2: Enable BNA interrupt for IN endpoints
>usb: dwc2: Change ISOC DDMA flow
>usb: dwc2: Add High Bandwidth ISOC OUT support
> 
>   drivers/usb/dwc2/core.h   |   2 -
>   drivers/usb/dwc2/gadget.c | 498 
> --
>   2 files changed, 348 insertions(+), 152 deletions(-)
> 

Please ignore this patch set because I'm going to improve it to consider 
systems with high IRQ latency.

Thanks,
Minas
--
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 v1 0/2] usb: dwc2: gadget: Fixes for LPM

[Grigor Tovmasyan]

Hi Simon,

On 4/19/2018 8:31 PM, Simon Shields wrote:
> Hi all,
> 
> On 10/04/2018 10:21 PM, Grigor Tovmasyan wrote:
>> Here are two little fixes for LPM feature.
>>
>> First one is coverity warning fix.
>>
>> The Second one was asserted by Stefan Wahren.
>>
>> Changes from version 0:
>>
>> 1/2:
>>  - Instead of converting parameter in the CHECK_RANGE macro
>>to int, changed hird_threshold type from u8 to int.
>>
>>
>> Grigor Tovmasyan (2):
>> usb: dwc2: gadget: Fix coverity issue
>> usb: dwc2: gadget: Change LPM default values
>>
>>drivers/usb/dwc2/core.h   | 2 +-
>>drivers/usb/dwc2/params.c | 8 
>>2 files changed, 5 insertions(+), 5 deletions(-)
> 
> The second patch in this series fixes a regression in 4.17-rc1 using dwc2 in 
> gadget
> mode on Exynos4412, introduced by commit 7455f8b7f0b3 ("usb: dwc2: Enable
> LPM"). The regression is that using the cdc_acm serial gadget (and
> presumably other gadgets) serial console output will only sporadically
> show up on the host (it seems to only show up as input is sent).
> 
The second patch is not fix for described by you issue. We will try to 
reproduce your issue and provide fix. Could you provide some logs or usb 
traces for issue?

> However, I'm unsure if completely disabling LPM is the correct fix, as the 
> dwc2
> revision in Exynos4412 (0x4f54281a) should support LPM according to the
> source
Yes, we can enable LPM based on hardware configuration.

> I'm not really sure how to debug this any further (vendor kernel
> releases contain no mention of LPM in the gadget drivers), so any pointers
> in that direction would be much appreciated.
> 
> Cheers,
> Simon
> 

--
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 RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Hans de Goede]

Hi,

On 20-04-18 12:51, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月20日 17:21
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx

Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

Hi,

On 20-04-18 11:18, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月18日 19:40
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org;
dl-linux-imx 
Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw
for rdo

Hi,

On 17-04-18 08:01, Li Jun wrote:

This patch is a further update for rdo based on [1], which removed
max_snk_ma/mv/mw but kept operating_snk_mw.
operating_snk_mw is only used to judge capability mismatch, per PD
spec, we can achieve this via compare the selected source PDO and
matching sink PDO, also after patch [1], we don't limit the PDO
matching between the same type, so the rdo operating and max
current/power calculation should be updated accordingly.


I do not believe that this is correct, lets take a device with a
fusb302 tcpc with the following PDO-s:

PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
PDO_VAR(5000, 12000, 3000);

And an operating_snk_mw of 2500mW then according to your new code a
charger which supplies 12V 2A will now get the mismatch bit set even
though it is delivering 24W.



Per PD spec, my understanding is to judge capability mismatch, we
should use *max* current/power, not this operating power.


Ok, I'm not familiar with the PD spec, perhaps someone who knows it well can
comment here ?


Hi Guenter,
Could you please comment here?

Thanks
Jun




In your case(VAR PDO), the max current is 3000mA, can I say the max
power is 3000mA * 5V = 15W? so the max current required at 12V is 15W
/ 12V = 1.25A, so yes, the 12V@2A is enough, do you think this kind of
calculation is reasonable?


That does not sound unreasonable, the real question is when exactly should we
set the capability mismatch bit, once we have that more clear how to
implement this should become clear too.


Fully agree, below is the capability mismatch bit related description from PD 
spec:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value larger than
the maximum current/power offered in the Source_Capabilities Message’s PDO
as referenced by the Object position field. This enables the Sink to indicate 
that
it requires more current/power than is being offered. If the Sink requires a
different voltage this will be indicated by its Sink_Capabilities Message."

"6.4.2.8 Maximum Operating Current
The Maximum Operating Current field in the Request Message shall be set to
the highest current the Sink will ever require. The difference between
the Operating Current and Maximum Operating Current fields
(when the GiveBack Flag is cleared) is used by the Device Policy Manager
in the Source to calculate the size of the Power Reserve to be maintained
(see Section 8.2.5.1). The Operating Current value shall be less than or
equal to the Maximum Operating Current value.
When the Capabilities Mismatch bit is set to zero the requested Maximum 
Operating
Current shall be less than or equal to the current in the offered Source 
Capabilities
since the Source will need to reserve this power for future use. ...
  
When the Capabilities Mismatch bit is set to one the requested Maximum

Operating Current may be greater than the current in the offered Source
Capabilities since the Source will need this information to ascertain
the Sink’s actual needs".


Thank you for looking that up, so to what value are we setting the
"Maximum Operating Current" field in the request we are sending?

Regards,

Hans







Regards,

Hans






I really don't see any way to fix this and I believe we should just
keep the operating_snk_mw field.


I had the same worry of breaking existing users, but found there is
something not matching PD spec, so want to get comments, if we really
need another input (beside sink PDO) in practice, we can keep it.

Thanks
Jun


Regards

Hans




[1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%
2F




patchwork.kernel.org%2Fpatch%2F10342299%2F=02%7C01%7Cjun.li%4

0nxp

.com%7C796c943804cc4a333a7c08d5a5212e41%7C686ea1d3bc2b4c6fa9

2cd

99c5c30





1635%7C0%7C0%7C636596484284807487=BH74zzVppAi7Fa8jtRt2cwc

d%2F%2F

o6idNC5hk5zyoWIA4%3D=0

Signed-off-by: Li Jun 
---
drivers/usb/typec/tcpm.c | 52

++--

1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index
27192083..0be04b3 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ 

Re: [RFT/PATCH 00/38] usb: dwc3: gadget: Rework & Refactoring

[Felipe Balbi]

Hi,

Bin Liu  writes:
>> Felipe Balbi  writes:
>> >>> Bin Liu  writes:
>> >>> > On Mon, Apr 09, 2018 at 02:26:14PM +0300, Felipe Balbi wrote:
>> >>> >> Hi guys,
>> >>> >> 
>> >>> >> I've been working on this series for a while now. I feels like after
>> >>> >> this series the transfer management code is far easier to read and
>> >>> >> understand.
>> >>> >> 
>> >>> >> Based on my tests, I have no regressions. Tested g_mass_storage and 
>> >>> >> all
>> >>> >> of testusb's tests (including ISOC).
>> >>> >> 
>> >>> >> Patches are also available on dwc3-improve-isoc-endpoints in my k.org
>> >>> >> tree. Test reports would be VERY, VERY, VERY welcome. Please give 
>> >>> >> this a
>> >>> >> go so we avoid regressions on v4.18.
>> >>> >
>> >>> > Have you tested g_webcam? I just tested your 
>> >>> > dwc3-improve-isoc-endpoints
>> >>> 
>> >>> for isoc I only tested g_zero.
>> >>
>> >> Then writting down details on what I observed probably won't help you :(
>> 
>> I got webcam running here, it sure _is_ helpful to let me know how you
>
> Great!
>
>> trigger the problem ;-) Also, high-speed or super-speed?
>
> Both. Long story short - super-speed doesn't stream the yuv video,
> gadget side kernel prints "VS request completed with status -18."
> flooding messages.
>
> high-speed does stream the video, but stopping the host application
> (both yavta and luvcview) causes gadget side kernel prints error message
> (I didn't keep the log). Then restart the host application doesn't
> stream the video any more.
>
> To run the test:
> gadget side:
>   # modprobe g_webcam (streaming_maxpacket=3072 for super-speed)
>   # uvc-gadget
> host side:
>   $ yavta -c -f YUYV -t 1/30 -s 640x360 -n4 /dev/video1, or
>   $ luvcview -d /dev/video1 -f yuv

okay, found the problem. This is actually a problem on webcam gadget
which kills the stream in case of Missed Isoc. The reason why it "works"
with dwc3 today is because dwc3, up until now, is really harsh whenever
we miss and interval.

Currently we stop the transfer and wait for the next XferNotReady. This
may cause us to loose many frames.

I've been talking with Laurent Pinchart (in Cc) about what would be the
best way to sort this out and, our conclusion, is that webcam gadget
should have a way for a single buffer to be treated as erroneous, but it
shouldn't stop the stream.

There's also another error in webcam gadget where default interval,
maxburst and maxpacket aren't very good for HS or SS.

Note that streaming_interval is, by default, 1. Which for FS means 1ms,
but for HS/SS it means 1 * 125us. So host side is actually polling
webcam gadget every uFrame. I got webcam gadget to behave really well
with streaming_interval=4 streaming_maxburst=16
streaming_maxpacket=3072. With that, in SS, I can get around 100
fps. There are a few cases when FPS drops to 33, but that's because it
coincides with a missed isoc and webcam stops and restarts the stream.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Jun Li]

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: 2018年4月20日 17:21
> To: Jun Li ; li...@roeck-us.net;
> heikki.kroge...@linux.intel.com
> Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx
> 
> Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo
> 
> Hi,
> 
> On 20-04-18 11:18, Jun Li wrote:
> >
> >> -Original Message-
> >> From: Hans de Goede [mailto:hdego...@redhat.com]
> >> Sent: 2018年4月18日 19:40
> >> To: Jun Li ; li...@roeck-us.net;
> >> heikki.kroge...@linux.intel.com
> >> Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org;
> >> dl-linux-imx 
> >> Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw
> >> for rdo
> >>
> >> Hi,
> >>
> >> On 17-04-18 08:01, Li Jun wrote:
> >>> This patch is a further update for rdo based on [1], which removed
> >>> max_snk_ma/mv/mw but kept operating_snk_mw.
> >>> operating_snk_mw is only used to judge capability mismatch, per PD
> >>> spec, we can achieve this via compare the selected source PDO and
> >>> matching sink PDO, also after patch [1], we don't limit the PDO
> >>> matching between the same type, so the rdo operating and max
> >>> current/power calculation should be updated accordingly.
> >>
> >> I do not believe that this is correct, lets take a device with a
> >> fusb302 tcpc with the following PDO-s:
> >>
> >>PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
> >>PDO_VAR(5000, 12000, 3000);
> >>
> >> And an operating_snk_mw of 2500mW then according to your new code a
> >> charger which supplies 12V 2A will now get the mismatch bit set even
> >> though it is delivering 24W.
> >>
> >
> > Per PD spec, my understanding is to judge capability mismatch, we
> > should use *max* current/power, not this operating power.
> 
> Ok, I'm not familiar with the PD spec, perhaps someone who knows it well can
> comment here ?

Hi Guenter,
Could you please comment here?

Thanks
Jun

> 
> > In your case(VAR PDO), the max current is 3000mA, can I say the max
> > power is 3000mA * 5V = 15W? so the max current required at 12V is 15W
> > / 12V = 1.25A, so yes, the 12V@2A is enough, do you think this kind of
> > calculation is reasonable?
> 
> That does not sound unreasonable, the real question is when exactly should we
> set the capability mismatch bit, once we have that more clear how to
> implement this should become clear too.

Fully agree, below is the capability mismatch bit related description from PD 
spec:

"If the Capability Mismatch bit is set to one
The Maximum Operating Current/Power field may contain a value larger than
the maximum current/power offered in the Source_Capabilities Message’s PDO
as referenced by the Object position field. This enables the Sink to indicate 
that
it requires more current/power than is being offered. If the Sink requires a
different voltage this will be indicated by its Sink_Capabilities Message."

"6.4.2.8 Maximum Operating Current
The Maximum Operating Current field in the Request Message shall be set to
the highest current the Sink will ever require. The difference between
the Operating Current and Maximum Operating Current fields
(when the GiveBack Flag is cleared) is used by the Device Policy Manager
in the Source to calculate the size of the Power Reserve to be maintained
(see Section 8.2.5.1). The Operating Current value shall be less than or
equal to the Maximum Operating Current value.
When the Capabilities Mismatch bit is set to zero the requested Maximum 
Operating
Current shall be less than or equal to the current in the offered Source 
Capabilities
since the Source will need to reserve this power for future use. ...
 
When the Capabilities Mismatch bit is set to one the requested Maximum
Operating Current may be greater than the current in the offered Source
Capabilities since the Source will need this information to ascertain
the Sink’s actual needs".

> 
> Regards,
> 
> Hans
> 
> 
> 
> >
> >> I really don't see any way to fix this and I believe we should just
> >> keep the operating_snk_mw field.
> >
> > I had the same worry of breaking existing users, but found there is
> > something not matching PD spec, so want to get comments, if we really
> > need another input (beside sink PDO) in practice, we can keep it.
> >
> > Thanks
> > Jun
> >>
> >> Regards
> >>
> >> Hans
> >>
> >>
> >>
> >>> [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%
> >>> 2F
> >>>
> >>
> patchwork.kernel.org%2Fpatch%2F10342299%2F=02%7C01%7Cjun.li%4
> >> 0nxp
> >>> .com%7C796c943804cc4a333a7c08d5a5212e41%7C686ea1d3bc2b4c6fa9
> 2cd
> >> 99c5c30
> >>>
> >>
> 1635%7C0%7C0%7C636596484284807487=BH74zzVppAi7Fa8jtRt2cwc
> >> d%2F%2F
> >>> o6idNC5hk5zyoWIA4%3D=0
> >>>
> >>> Signed-off-by: Li Jun 
> >>> ---
> >>>drivers/usb/typec/tcpm.c | 52
> >> ++--
> >>>1 file changed, 33 insertions(+), 19 deletions(-)

Re: [PATCH v2 4/6] USB: show USB 3.2 Dual-lane devices as Gen Xx2 during device enumeration

[Oliver Neukum]

Am Donnerstag, den 19.04.2018, 19:05 +0300 schrieb Mathias Nyman:
> USB 3.2 specification adds a Gen XxY notion for USB3 devices where
> X is the signaling rate on the wire. Gen 1xY is 5Gbps Superspeed
> and Gen 2xY is 10Gbps SuperSpeedPlus. Y is the lane count.
> 
> For normal, non inter-chip (SSIC) devies the rx and tx lane count is
> symmetric, and the maximum lane count for USB 3.2 devices is 2 (dual-lane).
> 
> SSIC devices may have asymmetric lane counts, with up to four
> lanes per direction. The USB 3.2 specification doesn't point out
> how to use the Gen XxY notion for these devices, so we limit the Gen Xx2
> notion to symmertic Dual lane devies.
> For other devices just show Gen1 or Gen2

If we detect an asymmetry, we should say so. Otherwise all looks good.

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 resend] usb: chipidea: Don't select EXTCON

[Peter Chen]

 
> >
> > Sorry to reply late, are you really care 2KB code side? Since many
> > users use EXTCON to handle vbus and id, it is hard just delete it. I
> > could accept patch for your specific platforms, like:
> >
> > +   select EXTCON if !ARCH_
> 
> The patch doesn't remove extcon support from chipidea driver.
> I just want to not select EXTCON unconditionally, but let the users choose. 
> If the
> users need extcon, they could enable EXTCON themselves
> 
> I just searched all the dts in arch/arm/boot/dts and arch/arm64/boot/dts only 
> the four
> dts give extcon phandle to chipidea host, other users don't make use of it:
> 
> arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi
> 
> arch/arm/boot/dts/qcom-apq8074-dragonboard.dts
> 
> arch/arm/boot/dts/qcom-msm8974-fairphone-fp2.dts
> 
> arch/arm/boot/dts/qcom-msm8974-sony-xperia-castor.dts
> 

I see, but I do not want to break msm platforms. You may try to create Glue 
driver Kconfig
entry for chipidea like dwc3, and let msm depends on EXTCON.

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

RE: [PATCH v2] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Chen]

 
> >> @@ -3,6 +3,8 @@ config USB_CHIPIDEA
> >>depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD &&
> >> !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> >>select EXTCON
> >>select RESET_CONTROLLER
> >> +  select MULTIPLEXER
> >> +  select MUX_GPIO
> >
> > The above two configurations are only used at your specific platforms,
> > please add them at either your platform defconfig or the related hardware 
> > driver's
> Kconfig.
> 
> "select MUX_GPIO" is indeed questionable and should be somewhere else because
> this driver will work with any other mux as well. It's simply something else 
> that
> requires it. If it was the case that MUX_GPIO is indeed required then the 
> whole use
> of the mux subsystem is questionable and the thing controlled might as well be
> controlled directly with the GPIO line. The mux subsystem is good when a 
> single mux
> "controller" is shared between several unrelated drivers utilizing different 
> muxes
> controlled by that same mux "controller" (think several muxes controlled by 
> the same
> GPIO line/lines). The mux subsystem is also useful when the driver does not 
> want to
> handle/know how the specific mux is controlled. That said, it's of course not 
> wrong to
> use the mux subsystem in cases like this either, but I think it might be much 
> easier
> and direct to just twiddle the single GPIO line directly here?
> 
> Or do you expect some future HW variant that will use some other means to 
> control
> this mux?
> 

No, this mux is only used at Yossi's boards, I expect these two configurations 
is under
MUX's hardware driver Kconfig if it is existed, or under its platform 
defconfig. Someone
has already complained too many selects increase the code size:

https://patchwork.kernel.org/patch/10349293/

> >>help
> >>  Say Y here if your system has a dual role high speed USB
> >>  controller based on ChipIdea silicon IP. It supports:
> >> diff --git a/drivers/usb/chipidea/core.c
> >> b/drivers/usb/chipidea/core.c index
> >> 33ae87f..8fa0991 100644
> >> --- a/drivers/usb/chipidea/core.c
> >> +++ b/drivers/usb/chipidea/core.c
> >> @@ -61,6 +61,7 @@
> >>  #include 
> >>  #include   #include
> >> 
> >> +#include 
> >>
> >>  #include "ci.h"
> >>  #include "udc.h"
> >> @@ -687,6 +688,10 @@ static int ci_get_platdata(struct device *dev,
> >>if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
> >>platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
> >>
> >> +  platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
> >> +  if (IS_ERR(platdata->usb_switch))
> >> +  return PTR_ERR(platdata->usb_switch);
> >> +
> >>ext_id = ERR_PTR(-ENODEV);
> >>ext_vbus = ERR_PTR(-ENODEV);
> >>if (of_property_read_bool(dev->of_node, "extcon")) { diff --git
> >> a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index
> >> af45aa32..d9d2d00
> >> 100644
> >> --- a/drivers/usb/chipidea/host.c
> >> +++ b/drivers/usb/chipidea/host.c
> >> @@ -13,6 +13,7 @@
> >>  #include 
> >>  #include 
> >>  #include 
> >> +#include 
> >>
> >>  #include "../host/ehci.h"
> >>
> >> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
> >>if (ci_otg_is_fsm_mode(ci)) {
> >>otg->host = >self;
> >>hcd->self.otg_port = 1;
> >> +  } else {
> >> +  ret = mux_control_select(ci->platdata->usb_switch, 1);
> >> +  if (ret)
> >> +  goto disable_reg;
> >
> > What will happen if ci->platdata->usb_switch  is NULL?
> 
> What has not been mentioned in this patch is that it depends on another patch 
> which
> is not yet upstream. You can google for
> 
> mux: add mux_control_get_optional() API
> 
> to get an idea (it's also in linux-next). Anyway, with that patch this is not 
> a problem.
> 

Thanks, I see it judges NULL pointer, then it is OK.

 
> >>  /**
> >>   * struct ci_hdrc_cable - structure for external connector cable
> >> state tracking @@ -
> >> 76,6 +77,7 @@ struct ci_hdrc_platform_data {
> >>/* VBUS and ID signal state tracking, using extcon framework */
> >>struct ci_hdrc_cablevbus_extcon;
> >>struct ci_hdrc_cableid_extcon;
> >> +  struct mux_control  *usb_switch;
> >>u32 phy_clkgate_delay_us;
> >
> > If CONFIG_USB_CHIPIDEA_HOST is not defined, it may cause build error
> 
> How is that related? There is a forward declaration above?
> 

Sorry, I did not notice drivers/usb/chipidea/core.c also includes 
.

Peter

Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Hans de Goede]

Hi,

On 20-04-18 11:18, Jun Li wrote:



-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年4月18日 19:40
To: Jun Li ; li...@roeck-us.net;
heikki.kroge...@linux.intel.com
Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx

Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

Hi,

On 17-04-18 08:01, Li Jun wrote:

This patch is a further update for rdo based on [1], which removed
max_snk_ma/mv/mw but kept operating_snk_mw.
operating_snk_mw is only used to judge capability mismatch, per PD
spec, we can achieve this via compare the selected source PDO and
matching sink PDO, also after patch [1], we don't limit the PDO
matching between the same type, so the rdo operating and max
current/power calculation should be updated accordingly.


I do not believe that this is correct, lets take a device with a fusb302 tcpc 
with
the following PDO-s:

PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
PDO_VAR(5000, 12000, 3000);

And an operating_snk_mw of 2500mW then according to your new code a
charger which supplies 12V 2A will now get the mismatch bit set even though it
is delivering 24W.



Per PD spec, my understanding is to judge capability mismatch, we should
use *max* current/power, not this operating power.


Ok, I'm not familiar with the PD spec, perhaps someone who knows it well
can comment here ?


In your case(VAR PDO), the max current is 3000mA, can I say the max
power is 3000mA * 5V = 15W? so the max current required at 12V
is 15W / 12V = 1.25A, so yes, the 12V@2A is enough, do you think
this kind of calculation is reasonable?


That does not sound unreasonable, the real question is when exactly should
we set the capability mismatch bit, once we have that more clear how to
implement this should become clear too.

Regards,

Hans






I really don't see any way to fix this and I believe we should just keep the
operating_snk_mw field.


I had the same worry of breaking existing users, but found there is something
not matching PD spec, so want to get comments, if we really need another input
(beside sink PDO) in practice, we can keep it.

Thanks
Jun


Regards

Hans




[1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F


patchwork.kernel.org%2Fpatch%2F10342299%2F=02%7C01%7Cjun.li%4
0nxp

.com%7C796c943804cc4a333a7c08d5a5212e41%7C686ea1d3bc2b4c6fa92cd

99c5c30



1635%7C0%7C0%7C636596484284807487=BH74zzVppAi7Fa8jtRt2cwc
d%2F%2F

o6idNC5hk5zyoWIA4%3D=0

Signed-off-by: Li Jun 
---
   drivers/usb/typec/tcpm.c | 52

++--

   1 file changed, 33 insertions(+), 19 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
27192083..0be04b3 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -1854,28 +1854,42 @@ static int tcpm_pd_build_request(struct

tcpm_port *port, u32 *rdo)

else
mv = pdo_min_voltage(pdo);

-   /* Select maximum available current within the sink pdo's limit */
-   if (type == PDO_TYPE_BATT) {
-   mw = min_power(pdo, matching_snk_pdo);
-   ma = 1000 * mw / mv;
-   } else {
-   ma = min_current(pdo, matching_snk_pdo);
-   mw = ma * mv / 1000;
-   }
-
flags = RDO_USB_COMM | RDO_NO_SUSPEND;

-   /* Set mismatch bit if offered power is less than operating power */
-   max_ma = ma;
-   max_mw = mw;
-   if (mw < port->operating_snk_mw) {
-   flags |= RDO_CAP_MISMATCH;
-   if (type == PDO_TYPE_BATT &&
-   (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
-   max_mw = pdo_max_power(matching_snk_pdo);
-   else if (pdo_max_current(matching_snk_pdo) >
-pdo_max_current(pdo))
+   switch (type) {
+   case PDO_TYPE_FIXED:
+   case PDO_TYPE_VAR:
+   if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
+   max_ma = pdo_max_power(matching_snk_pdo) * 1000 / mv;
+   else
max_ma = pdo_max_current(matching_snk_pdo);
+
+   if (max_ma > pdo_max_current(pdo)) {
+   flags |= RDO_CAP_MISMATCH;
+   ma = pdo_max_current(pdo);
+   } else {
+   ma = max_ma;
+   }
+   break;
+   case PDO_TYPE_BATT:
+   if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
+   max_mw = pdo_max_power(matching_snk_pdo);
+   else
+   max_mw = pdo_max_current(matching_snk_pdo) *
+pdo_min_voltage(matching_snk_pdo) /
+1000;
+
+   if (max_mw > pdo_max_power(pdo)) {
+   flags |= RDO_CAP_MISMATCH;
+   mw = pdo_max_power(pdo);
+   } else 

RE: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo

[Jun Li]

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: 2018年4月18日 19:40
> To: Jun Li ; li...@roeck-us.net;
> heikki.kroge...@linux.intel.com
> Cc: gre...@linuxfoundation.org; linux-usb@vger.kernel.org; dl-linux-imx
> 
> Subject: Re: [PATCH RFC] usb: typec: tcpm: remove operating_snk_mw for rdo
> 
> Hi,
> 
> On 17-04-18 08:01, Li Jun wrote:
> > This patch is a further update for rdo based on [1], which removed
> > max_snk_ma/mv/mw but kept operating_snk_mw.
> > operating_snk_mw is only used to judge capability mismatch, per PD
> > spec, we can achieve this via compare the selected source PDO and
> > matching sink PDO, also after patch [1], we don't limit the PDO
> > matching between the same type, so the rdo operating and max
> > current/power calculation should be updated accordingly.
> 
> I do not believe that this is correct, lets take a device with a fusb302 tcpc 
> with
> the following PDO-s:
> 
>   PDO_FIXED(5000, 400, PDO_FIXED_FLAGS)
>   PDO_VAR(5000, 12000, 3000);
> 
> And an operating_snk_mw of 2500mW then according to your new code a
> charger which supplies 12V 2A will now get the mismatch bit set even though it
> is delivering 24W.
> 

Per PD spec, my understanding is to judge capability mismatch, we should
use *max* current/power, not this operating power.

In your case(VAR PDO), the max current is 3000mA, can I say the max
power is 3000mA * 5V = 15W? so the max current required at 12V
is 15W / 12V = 1.25A, so yes, the 12V@2A is enough, do you think
this kind of calculation is reasonable? 

> I really don't see any way to fix this and I believe we should just keep the
> operating_snk_mw field.

I had the same worry of breaking existing users, but found there is something
not matching PD spec, so want to get comments, if we really need another input
(beside sink PDO) in practice, we can keep it.

Thanks
Jun
> 
> Regards
> 
> Hans
> 
> 
> 
> > [1]https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2F
> >
> patchwork.kernel.org%2Fpatch%2F10342299%2F=02%7C01%7Cjun.li%4
> 0nxp
> > .com%7C796c943804cc4a333a7c08d5a5212e41%7C686ea1d3bc2b4c6fa92cd
> 99c5c30
> >
> 1635%7C0%7C0%7C636596484284807487=BH74zzVppAi7Fa8jtRt2cwc
> d%2F%2F
> > o6idNC5hk5zyoWIA4%3D=0
> >
> > Signed-off-by: Li Jun 
> > ---
> >   drivers/usb/typec/tcpm.c | 52
> ++--
> >   1 file changed, 33 insertions(+), 19 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 27192083..0be04b3 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -1854,28 +1854,42 @@ static int tcpm_pd_build_request(struct
> tcpm_port *port, u32 *rdo)
> > else
> > mv = pdo_min_voltage(pdo);
> >
> > -   /* Select maximum available current within the sink pdo's limit */
> > -   if (type == PDO_TYPE_BATT) {
> > -   mw = min_power(pdo, matching_snk_pdo);
> > -   ma = 1000 * mw / mv;
> > -   } else {
> > -   ma = min_current(pdo, matching_snk_pdo);
> > -   mw = ma * mv / 1000;
> > -   }
> > -
> > flags = RDO_USB_COMM | RDO_NO_SUSPEND;
> >
> > -   /* Set mismatch bit if offered power is less than operating power */
> > -   max_ma = ma;
> > -   max_mw = mw;
> > -   if (mw < port->operating_snk_mw) {
> > -   flags |= RDO_CAP_MISMATCH;
> > -   if (type == PDO_TYPE_BATT &&
> > -   (pdo_max_power(matching_snk_pdo) > pdo_max_power(pdo)))
> > -   max_mw = pdo_max_power(matching_snk_pdo);
> > -   else if (pdo_max_current(matching_snk_pdo) >
> > -pdo_max_current(pdo))
> > +   switch (type) {
> > +   case PDO_TYPE_FIXED:
> > +   case PDO_TYPE_VAR:
> > +   if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
> > +   max_ma = pdo_max_power(matching_snk_pdo) * 1000 / mv;
> > +   else
> > max_ma = pdo_max_current(matching_snk_pdo);
> > +
> > +   if (max_ma > pdo_max_current(pdo)) {
> > +   flags |= RDO_CAP_MISMATCH;
> > +   ma = pdo_max_current(pdo);
> > +   } else {
> > +   ma = max_ma;
> > +   }
> > +   break;
> > +   case PDO_TYPE_BATT:
> > +   if (pdo_type(matching_snk_pdo) == PDO_TYPE_BATT)
> > +   max_mw = pdo_max_power(matching_snk_pdo);
> > +   else
> > +   max_mw = pdo_max_current(matching_snk_pdo) *
> > +pdo_min_voltage(matching_snk_pdo) /
> > +1000;
> > +
> > +   if (max_mw > pdo_max_power(pdo)) {
> > +   flags |= RDO_CAP_MISMATCH;
> > +   mw = pdo_max_power(pdo);
> > +   } else {
> > +   mw = max_mw;
> > +   }
> > +
> > +   ma = mw * 1000 / mv;
> > +   break;
> > +   default:
> > +   break;
> > }
> >
> >  

Re: [PATCH] usb: roles: intel_xhci: Don't use PMIC for port type identification

[Hans de Goede]

Hi Heikki,

On 20-04-18 10:06, Heikki Krogerus wrote:

This will add an array of known USB Type-C Port devices that
will be used as a blacklist for enabling userspace-control,
and remove the PMIC ACPI HID which was used for the same
purpose.

It turns out that on some CHT based platforms the X-Powers
PMIC is handled in firmware. The ACPI HID for it is
therefore not usable for determining the port type. The
driver now searches for known USB Type-C port devices
instead.

Signed-off-by: Heikki Krogerus 
---
Hi Hans,

So it seems that we can't rely on the PMIC. This is my proposal for a
fix. I'm in practice just using blacklist instead of whitelist for
identifying the port type.

Let me know if it's OK to you.


I'm afraid that this is not going to work, almost all CHT devices
(and some BYT devices to) define an INT33FE device, here is a quick grep
on a directory where I store dsdt files from various CHT and BYT devices:

[hans@shalem ~]$ ls /home/archive/hwinfo | wc -l
64
[hans@shalem ~]$ ack -l INT33FE /home/archive/hwinfo/*/dsdt.dsl | wc -l
36

So 36 out of 64 devices define an INT33FE device and at least half
of the devices there are BYT.

The INT33FE device is described as a "XPOWER Battery Device", so I'm
not sure why you are checking for this to determine if the Type-C
connector is controlled by firmware.

I have a bunch of CHT devices with a Type-C connector:

GPD win:

This uses a Whiskey Cove PMIC combined with FUSB302 Type-C controller,
pi3usb30532. Which is all driven by the tcpm code.

Chuwi Hi8 Pro and Chuwi Vi8 Plus:
-
These use a Type-C connector as a glorified micro-usb connector,
the Hi8 Pro supports Superspeed using a hardwired asmedia switch
to deal with upright / upside-down insertion. The Vi8 Plus is
USB2 only.

Host/device mode is determine by treating the Cc pin as an id-pin
(it has a gpio connected which is either low or high depending
on the Cc being pulled up/down).

These use an AXP288 PMIC and use the USB2 lines to BC-1.2 charger
detection there is no PD and no 3A / 1.5A / 0.5A detection based
on the Cc pull up resistor, resulting in charging at 0.5 A
when using a Type-C charger which does not do BC1.2 on its
USB2 lines.

As said these really treat the Type-C connector as a micro-sub
connector, so we need userspace control to be able to switch
to host mode when using an otg charging-hub (so that the user can
still use devices attached to the hub while charging).

Asus T100HA and Lenovo Ideapad Miix 320:

These have an USB host only Type-C connector, which does
do Superspeed (likely contain a fixed switch to deal with upright /
upside-down insertion).

These cannot charge at all through the Type-C connector (they
do turn of the 5v boost when used in device mode), theoretically
the port may work in device mode (depending on which lanes they
used), but the device-mode USB controller is disabled by the BIOS
with no option to change it.

I also have 5 different CHT devices with a micro-usb connector:
-Cube iWork8 Air
-Jumper Ezpad mini 3
-Pipo W2s
-Teclast X80 pro
-Lenovo Ideapad Miix 310

All of which use an AXP288 PMIC and can do charging, host and
device mode through their micro-usb connector with the
exception of the Lenovo Ideapad Miix 310 where the micro-usb
is host mode only (like the Type-C on the 320) and there is
a separate power-barrel connector for charging.

All these define an INT33FE device, although at least on
the Cube and Lenovo ones the INT33FE's device _STA returns 0,
so your check should not match, but on others the _STA for
the INT33FE device does return 0x0f.

TL;DR: Checking the INT33FE device to determine if a Type-C
connector is present is not a good way to handle this.

Can you describe the device on which you are seeing
userspace control being enabled even though it should not be
enabled and maybe mail me an ACPI dump for it?

Regards,

Hans












Thanks,

---
  .../usb/roles/intel-xhci-usb-role-switch.c| 40 +++
  1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c 
b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index de72eedb762e..1696d1f25769 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -38,19 +38,26 @@ struct intel_xhci_usb_data {
void __iomem *base;
  };
  
-struct intel_xhci_acpi_match {

-   const char *hid;
-   int hrv;
+static const struct acpi_device_id typec_port_devices[] = {
+   { "PNP0CA0", 0 }, /* UCSI */
+   { "INT33FE", 0 }, /* CHT USB Type-C PHY */
+   { },
  };
  
-/*

- * ACPI IDs for PMICs which do not support separate data and power role
- * detection (USB ACA detection for micro USB OTG), we allow userspace to
- * change the role manually on these.
- */
-static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
-   { 

Re: [PATCH resend] usb: chipidea: Don't select EXTCON

[Jisheng Zhang]

Hi Peter

On Fri, 20 Apr 2018 01:38:42 + Peter Chen wrote:

>  
> >  drivers/usb/chipidea/Kconfig | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/usb/chipidea/Kconfig b/drivers/usb/chipidea/Kconfig 
> > index
> > 785f0ed037f7..97509172d536 100644
> > --- a/drivers/usb/chipidea/Kconfig
> > +++ b/drivers/usb/chipidea/Kconfig
> > @@ -1,7 +1,6 @@
> >  config USB_CHIPIDEA
> > tristate "ChipIdea Highspeed Dual Role Controller"
> > depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD
> > && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
> > -   select EXTCON
> > select RESET_CONTROLLER
> > help
> >   Say Y here if your system has a dual role high speed USB
> > --
> > 2.17.0  
> 
> Hi Jisheng,
> 
> Sorry to reply late, are you really care 2KB code side? Since many users use
> EXTCON to handle vbus and id, it is hard just delete it. I could accept patch
> for your specific platforms, like:
> 
> + select EXTCON if !ARCH_

The patch doesn't remove extcon support from chipidea driver.
I just want to not select EXTCON unconditionally, but let the users
choose. If the users need extcon, they could enable EXTCON themselves

I just searched all the dts in arch/arm/boot/dts and arch/arm64/boot/dts
only the four dts give extcon phandle to chipidea host, other users
don't make use of it:

arch/arm64/boot/dts/qcom/apq8016-sbc.dtsi

arch/arm/boot/dts/qcom-apq8074-dragonboard.dts

arch/arm/boot/dts/qcom-msm8974-fairphone-fp2.dts

arch/arm/boot/dts/qcom-msm8974-sony-xperia-castor.dts

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

[PATCH] usb: roles: intel_xhci: Don't use PMIC for port type identification

[Heikki Krogerus]

This will add an array of known USB Type-C Port devices that
will be used as a blacklist for enabling userspace-control,
and remove the PMIC ACPI HID which was used for the same
purpose.

It turns out that on some CHT based platforms the X-Powers
PMIC is handled in firmware. The ACPI HID for it is
therefore not usable for determining the port type. The
driver now searches for known USB Type-C port devices
instead.

Signed-off-by: Heikki Krogerus 
---
Hi Hans,

So it seems that we can't rely on the PMIC. This is my proposal for a
fix. I'm in practice just using blacklist instead of whitelist for
identifying the port type.

Let me know if it's OK to you.


Thanks,

---
 .../usb/roles/intel-xhci-usb-role-switch.c| 40 +++
 1 file changed, 24 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/roles/intel-xhci-usb-role-switch.c 
b/drivers/usb/roles/intel-xhci-usb-role-switch.c
index de72eedb762e..1696d1f25769 100644
--- a/drivers/usb/roles/intel-xhci-usb-role-switch.c
+++ b/drivers/usb/roles/intel-xhci-usb-role-switch.c
@@ -38,19 +38,26 @@ struct intel_xhci_usb_data {
void __iomem *base;
 };
 
-struct intel_xhci_acpi_match {
-   const char *hid;
-   int hrv;
+static const struct acpi_device_id typec_port_devices[] = {
+   { "PNP0CA0", 0 }, /* UCSI */
+   { "INT33FE", 0 }, /* CHT USB Type-C PHY */
+   { },
 };
 
-/*
- * ACPI IDs for PMICs which do not support separate data and power role
- * detection (USB ACA detection for micro USB OTG), we allow userspace to
- * change the role manually on these.
- */
-static const struct intel_xhci_acpi_match allow_userspace_ctrl_ids[] = {
-   { "INT33F4",  3 }, /* X-Powers AXP288 PMIC */
-};
+static acpi_status intel_xhci_userspace_ctrl(acpi_handle handle, u32 level,
+void *ctx, void **ret)
+{
+   struct acpi_device *adev;
+
+   if (acpi_bus_get_device(handle, ))
+   return AE_OK;
+
+   /* Don't allow userspace-control with Type-C ports */
+   if (!acpi_match_device_ids(adev, typec_port_devices))
+   return AE_ACCESS;
+
+   return AE_OK;
+}
 
 static int intel_xhci_usb_set_role(struct device *dev, enum usb_role role)
 {
@@ -137,7 +144,7 @@ static int intel_xhci_usb_probe(struct platform_device 
*pdev)
struct device *dev = >dev;
struct intel_xhci_usb_data *data;
struct resource *res;
-   int i;
+   acpi_status status;
 
data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
if (!data)
@@ -148,10 +155,11 @@ static int intel_xhci_usb_probe(struct platform_device 
*pdev)
if (!data->base)
return -ENOMEM;
 
-   for (i = 0; i < ARRAY_SIZE(allow_userspace_ctrl_ids); i++)
-   if (acpi_dev_present(allow_userspace_ctrl_ids[i].hid, "1",
-allow_userspace_ctrl_ids[i].hrv))
-   sw_desc.allow_userspace_control = true;
+   status = acpi_walk_namespace(ACPI_TYPE_DEVICE, ACPI_ROOT_OBJECT,
+ACPI_UINT32_MAX, intel_xhci_userspace_ctrl,
+NULL, NULL, NULL);
+   if (ACPI_SUCCESS(status))
+   sw_desc.allow_userspace_control = true;
 
platform_set_drvdata(pdev, data);
 
-- 
2.17.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 55/61] usb: mtu3: simplify getting .drvdata

[Chunfeng Yun]

hi,

On Thu, 2018-04-19 at 16:06 +0200, Wolfram Sang wrote:
> We should get drvdata from struct device directly. Going via
> platform_device is an unneeded step back and forth.
> 
> Signed-off-by: Wolfram Sang 
> ---
> 
> Build tested only. buildbot is happy. Please apply individually.
> 
>  drivers/usb/mtu3/mtu3_plat.c | 6 ++
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/mtu3/mtu3_plat.c b/drivers/usb/mtu3/mtu3_plat.c
> index 628d5ce356ca..46551f6d16fd 100644
> --- a/drivers/usb/mtu3/mtu3_plat.c
> +++ b/drivers/usb/mtu3/mtu3_plat.c
> @@ -447,8 +447,7 @@ static int mtu3_remove(struct platform_device *pdev)
>   */
>  static int __maybe_unused mtu3_suspend(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct ssusb_mtk *ssusb = platform_get_drvdata(pdev);
> + struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
>  
>   dev_dbg(dev, "%s\n", __func__);
>  
> @@ -466,8 +465,7 @@ static int __maybe_unused mtu3_suspend(struct device *dev)
>  
>  static int __maybe_unused mtu3_resume(struct device *dev)
>  {
> - struct platform_device *pdev = to_platform_device(dev);
> - struct ssusb_mtk *ssusb = platform_get_drvdata(pdev);
> + struct ssusb_mtk *ssusb = dev_get_drvdata(dev);
>   int ret;
>  
>   dev_dbg(dev, "%s\n", __func__);

Acked-by: Chunfeng Yun 

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 v2] usb: chipidea: Hook into mux framework to toggle usb switch

[Peter Rosin]

On 2018-04-20 04:00, Peter Chen wrote:
>  
>  
>> --- a/drivers/usb/chipidea/Kconfig
>> +++ b/drivers/usb/chipidea/Kconfig
>> @@ -3,6 +3,8 @@ config USB_CHIPIDEA
>>  depends on ((USB_EHCI_HCD && USB_GADGET) || (USB_EHCI_HCD
>> && !USB_GADGET) || (!USB_EHCI_HCD && USB_GADGET)) && HAS_DMA
>>  select EXTCON
>>  select RESET_CONTROLLER
>> +select MULTIPLEXER
>> +select MUX_GPIO
> 
> The above two configurations are only used at your specific platforms, please 
> add
> them at either your platform defconfig or the related hardware driver's 
> Kconfig. 

"select MUX_GPIO" is indeed questionable and should be somewhere else because
this driver will work with any other mux as well. It's simply something else
that requires it. If it was the case that MUX_GPIO is indeed required then the
whole use of the mux subsystem is questionable and the thing controlled might
as well be controlled directly with the GPIO line. The mux subsystem is good
when a single mux "controller" is shared between several unrelated drivers
utilizing different muxes controlled by that same mux "controller" (think
several muxes controlled by the same GPIO line/lines). The mux subsystem is
also useful when the driver does not want to handle/know how the specific mux
is controlled. That said, it's of course not wrong to use the mux subsystem
in cases like this either, but I think it might be much easier and direct to
just twiddle the single GPIO line directly here?

Or do you expect some future HW variant that will use some other means to
control this mux?

>>  help
>>Say Y here if your system has a dual role high speed USB
>>controller based on ChipIdea silicon IP. It supports:
>> diff --git a/drivers/usb/chipidea/core.c b/drivers/usb/chipidea/core.c index
>> 33ae87f..8fa0991 100644
>> --- a/drivers/usb/chipidea/core.c
>> +++ b/drivers/usb/chipidea/core.c
>> @@ -61,6 +61,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "ci.h"
>>  #include "udc.h"
>> @@ -687,6 +688,10 @@ static int ci_get_platdata(struct device *dev,
>>  if (of_find_property(dev->of_node, "non-zero-ttctrl-ttha", NULL))
>>  platdata->flags |= CI_HDRC_SET_NON_ZERO_TTHA;
>>
>> +platdata->usb_switch = devm_mux_control_get_optional(dev, "usb_switch");
>> +if (IS_ERR(platdata->usb_switch))
>> +return PTR_ERR(platdata->usb_switch);
>> +
>>  ext_id = ERR_PTR(-ENODEV);
>>  ext_vbus = ERR_PTR(-ENODEV);
>>  if (of_property_read_bool(dev->of_node, "extcon")) { diff --git
>> a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c index 
>> af45aa32..d9d2d00
>> 100644
>> --- a/drivers/usb/chipidea/host.c
>> +++ b/drivers/usb/chipidea/host.c
>> @@ -13,6 +13,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "../host/ehci.h"
>>
>> @@ -161,6 +162,10 @@ static int host_start(struct ci_hdrc *ci)
>>  if (ci_otg_is_fsm_mode(ci)) {
>>  otg->host = >self;
>>  hcd->self.otg_port = 1;
>> +} else {
>> +ret = mux_control_select(ci->platdata->usb_switch, 1);
>> +if (ret)
>> +goto disable_reg;
> 
> What will happen if ci->platdata->usb_switch  is NULL?

What has not been mentioned in this patch is that it depends on another
patch which is not yet upstream. You can google for

mux: add mux_control_get_optional() API

to get an idea (it's also in linux-next). Anyway, with that patch this
is not a problem.

>>  }
>>  }
>>
>> @@ -181,6 +186,8 @@ static void host_stop(struct ci_hdrc *ci)
>>  struct usb_hcd *hcd = ci->hcd;
>>
>>  if (hcd) {
>> +if (!ci_otg_is_fsm_mode(ci))
>> +mux_control_deselect(ci->platdata->usb_switch);
> 
> Ditto.
> 
>>  if (ci->platdata->notify_event)
>>  ci->platdata->notify_event(ci,
>>  CI_HDRC_CONTROLLER_STOPPED_EVENT);
>> diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index
>> 9852ec5..209d3f6 100644
>> --- a/drivers/usb/chipidea/udc.c
>> +++ b/drivers/usb/chipidea/udc.c
>> @@ -19,6 +19,7 @@
>>  #include 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include "ci.h"
>>  #include "udc.h"
>> @@ -1965,16 +1966,26 @@ void ci_hdrc_gadget_destroy(struct ci_hdrc *ci)
>>
>>  static int udc_id_switch_for_device(struct ci_hdrc *ci)  {
>> +int ret = 0;
>> +
>>  if (ci->is_otg)
>>  /* Clear and enable BSV irq */
>>  hw_write_otgsc(ci, OTGSC_BSVIS | OTGSC_BSVIE,
>>  OTGSC_BSVIS | OTGSC_BSVIE);
>>
>> -return 0;
>> +if (!ci_otg_is_fsm_mode(ci))
>> +ret = mux_control_select(ci->platdata->usb_switch, 0);
>> +
> 
> Ditto
> 
>> +if (ci->is_otg && ret)
>> +hw_write_otgsc(ci, OTGSC_BSVIE | OTGSC_BSVIS,
>> OTGSC_BSVIS);
>> +
>> +return ret;
>>  }
>>
>>  static void