Re: [PATCH] USB: OHCI: Fix NULL dereference in HCDs using HCD_LOCAL_MEM

[Alan Stern]

On Fri, 9 Mar 2018, Fredrik Noring wrote:

> Scatter-gather needs to be disabled when using dma_declare_coherent_memory
> and HCD_LOCAL_MEM. Andrea Righi made the equivalent fix for EHCI drivers
> in commit 4307a28eb01284 "USB: EHCI: fix NULL pointer dererence in HCDs
> that use HCD_LOCAL_MEM".
> 
> The following NULL pointer WARN_ON_ONCE triggered with OHCI drivers:
> 
> [ cut here ]
> WARNING: CPU: 0 PID: 49 at drivers/usb/core/hcd.c:1379 
> hcd_alloc_coherent+0x4c/0xc8
> Modules linked in:
> CPU: 0 PID: 49 Comm: usb-storage Not tainted 4.15.0+ #1014
> Stack :   805a78d2 003a 81f5c2cc 8053d367 804d77fc 
> 0031
> 805a3a08 0563 81ee9400 805a  10058c00 81f61b10 
> 805c
>   805a 00d9038e 0004 803ee818 0006 
> 312e3420
> 805c  0073 81f61958   802eb380 
> 804fd538
> 0009 0563 81ee9400 805a 0002 80056148  
> 805a
> ...
> Call Trace:
> [<578af360>] show_stack+0x74/0x104
> [<2f3702c6>] __warn+0x118/0x120
> [] warn_slowpath_null+0x44/0x58
> [] hcd_alloc_coherent+0x4c/0xc8
> [<3578fa36>] usb_hcd_map_urb_for_dma+0x4d8/0x534
> [<110bc94c>] usb_hcd_submit_urb+0x82c/0x834
> [<02eb5baf>] usb_sg_wait+0x14c/0x1a0
> [] usb_stor_bulk_transfer_sglist.part.1+0xac/0x124
> [<87a5c34c>] usb_stor_bulk_srb+0x40/0x60
> [] usb_stor_Bulk_transport+0x160/0x37c
> [] usb_stor_invoke_transport+0x3c/0x500
> [<004754f4>] usb_stor_control_thread+0x258/0x28c
> [<22edf42e>] kthread+0x134/0x13c
> [] ret_from_kernel_thread+0x14/0x1c
> ---[ end trace bcdb825805eefdcc ]---
> 
> Signed-off-by: Fredrik Noring 
> 
> diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
> --- a/drivers/usb/host/ohci-hcd.c
> +++ b/drivers/usb/host/ohci-hcd.c
> @@ -446,7 +446,8 @@ static int ohci_init (struct ohci_hcd *ohci)
>   struct usb_hcd *hcd = ohci_to_hcd(ohci);
>  
>   /* Accept arbitrarily long scatter-gather lists */
> - hcd->self.sg_tablesize = ~0;
> + if (!(hcd->driver->flags & HCD_LOCAL_MEM))
> + hcd->self.sg_tablesize = ~0;
>  
>   if (distrust_firmware)
>   ohci->flags |= OHCI_QUIRK_HUB_POWER;

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 v6 01/12] drivers: base: Unified device connection lookup

[Greg Kroah-Hartman]

On Fri, Mar 02, 2018 at 11:20:46AM +0100, Hans de Goede wrote:
> From: Heikki Krogerus 
> 
> Several frameworks - clk, gpio, phy, pmw, etc. - maintain
> lookup tables for describing connections and provide custom
> API for handling them. This introduces a single generic
> lookup table and API for the connections.
> 
> The motivation for this commit is centralizing the
> connection lookup, but the goal is to ultimately extract the
> connection descriptions also from firmware by using the
> fwnode_graph_* functions and other mechanisms that are
> available.
> 
> Signed-off-by: Heikki Krogerus 
> Reviewed-by: Hans de Goede 
> Reviewed-by: Andy Shevchenko 
> Signed-off-by: Hans de Goede 

Sorry for the delay, just now reviewing this patch...

The content is fine (if not scary for the obvious reason of passing
around 'struct device' of different bus types, but ok...), but the api
naming is "rough":

> --- /dev/null
> +++ b/include/linux/connection.h

"connection.h" is vague, why not just put this in device.h?

> @@ -0,0 +1,34 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#ifndef _LINUX_CONNECTION_H_
> +#define _LINUX_CONNECTION_H_
> +
> +#include 
> +
> +struct device;
> +
> +/**
> + * struct devcon - Device Connection Descriptor
> + * @endpoint: The names of the two devices connected together
> + * @id: Unique identifier for the connection
> + * @list: List head, private for devcon internal use only
> + */
> +struct devcon {

'struct device_connection'?  Spell it out please, people might think
this is a "developer conference" :)

> + const char  *endpoint[2];
> + const char  *id;
> + struct list_headlist;
> +};
> +
> +void *__device_find_connection(struct device *dev, const char *con_id,
> +void *data,
> +void *(*match)(struct devcon *con, int ep,
> +   void *data));

Ick, __* functions are usually "no lock needed", but here you are doing
a lot "more" than the normal device_find_connection() call.  Why not
make this:
device_connection_find_match()?

> +
> +struct device *device_find_connection(struct device *dev, const char 
> *con_id);

device_connection_find()?

> +
> +#define DEVCON(_ep0, _ep1, _id)(struct devcon) { { _ep0, _ep1 }, _id, }

Can you use named identifiers here?

> +
> +void add_device_connection(struct devcon *con);
> +void remove_device_connection(struct devcon *con);

device_connection_add() and device_connection_remove()?

I can make the api name changes in an add-on patch.

thanks,

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

Re: [PATCH] usb: typec: Separate the definitions for data and power roles

[Greg Kroah-Hartman]

On Fri, Mar 09, 2018 at 02:09:18PM +0300, Heikki Krogerus wrote:
> USB Type-C specification v1.2 separated the power and data
> roles more clearly. Dual-Role-Data term was introduced, and
> the meaning of DRP was changed from "Dual-Role-Port" to
> "Dual-Role-Power".
> 
> In order to allow the port drivers to describe the
> capabilities of the ports more clearly according to the
> newest specifications, introducing separate definitions for
> the data roles.
> 
> Signed-off-by: Heikki Krogerus 
> Reviewed-by: Guenter Roeck 
> ---
>  drivers/usb/typec/class.c   | 56 
> ++---

This file isn't in the tree, what tree/branch did you make this patch
against?

confused,

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

[PATCH] USB: OHCI: Fix NULL dereference in HCDs using HCD_LOCAL_MEM

[Fredrik Noring]

Scatter-gather needs to be disabled when using dma_declare_coherent_memory
and HCD_LOCAL_MEM. Andrea Righi made the equivalent fix for EHCI drivers
in commit 4307a28eb01284 "USB: EHCI: fix NULL pointer dererence in HCDs
that use HCD_LOCAL_MEM".

The following NULL pointer WARN_ON_ONCE triggered with OHCI drivers:

[ cut here ]
WARNING: CPU: 0 PID: 49 at drivers/usb/core/hcd.c:1379 
hcd_alloc_coherent+0x4c/0xc8
Modules linked in:
CPU: 0 PID: 49 Comm: usb-storage Not tainted 4.15.0+ #1014
Stack :   805a78d2 003a 81f5c2cc 8053d367 804d77fc 0031
805a3a08 0563 81ee9400 805a  10058c00 81f61b10 805c
  805a 00d9038e 0004 803ee818 0006 312e3420
805c  0073 81f61958   802eb380 804fd538
0009 0563 81ee9400 805a 0002 80056148  805a
...
Call Trace:
[<578af360>] show_stack+0x74/0x104
[<2f3702c6>] __warn+0x118/0x120
[] warn_slowpath_null+0x44/0x58
[] hcd_alloc_coherent+0x4c/0xc8
[<3578fa36>] usb_hcd_map_urb_for_dma+0x4d8/0x534
[<110bc94c>] usb_hcd_submit_urb+0x82c/0x834
[<02eb5baf>] usb_sg_wait+0x14c/0x1a0
[] usb_stor_bulk_transfer_sglist.part.1+0xac/0x124
[<87a5c34c>] usb_stor_bulk_srb+0x40/0x60
[] usb_stor_Bulk_transport+0x160/0x37c
[] usb_stor_invoke_transport+0x3c/0x500
[<004754f4>] usb_stor_control_thread+0x258/0x28c
[<22edf42e>] kthread+0x134/0x13c
[] ret_from_kernel_thread+0x14/0x1c
---[ end trace bcdb825805eefdcc ]---

Signed-off-by: Fredrik Noring 

diff --git a/drivers/usb/host/ohci-hcd.c b/drivers/usb/host/ohci-hcd.c
--- a/drivers/usb/host/ohci-hcd.c
+++ b/drivers/usb/host/ohci-hcd.c
@@ -446,7 +446,8 @@ static int ohci_init (struct ohci_hcd *ohci)
struct usb_hcd *hcd = ohci_to_hcd(ohci);
 
/* Accept arbitrarily long scatter-gather lists */
-   hcd->self.sg_tablesize = ~0;
+   if (!(hcd->driver->flags & HCD_LOCAL_MEM))
+   hcd->self.sg_tablesize = ~0;
 
if (distrust_firmware)
ohci->flags |= OHCI_QUIRK_HUB_POWER;
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 0/7] typec: tcpm: Add sink side support for PPS

[Greg Kroah-Hartman]

On Tue, Jan 02, 2018 at 03:50:48PM +, Adam Thomson wrote:
> This patch set adds sink side support for the PPS feature introduced in the
> USB PD 3.0 specification.
> 
> The source PPS supply is represented using the Power Supply framework to 
> provide
> access and control APIs for dealing with it's operating voltage and current,
> and switching between a standard PDO and PPS APDO operation. During standard 
> PDO
> operation the voltage and current is read-only, but for APDO PPS these are
> writable as well to allow for control.
> 
> It should be noted that the keepalive for PPS is not handled within TCPM. The
> expectation is that the external user will be required to ensure re-requests
> occur regularly to ensure PPS remains and the source does not hard reset.

I've applied the first 3 patches now.  Can you rebase and resend based
on the feedback so far?

thanks,

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

Re: [PATCH v1 11/14] USB: host: sl811: Re-use DEFINE_SHOW_ATTRIBUTE() macro

[Greg Kroah-Hartman]

On Wed, Feb 14, 2018 at 06:08:27PM +0200, Andy Shevchenko wrote:
> ...instead of open coding file operations followed by custom ->open()
> callbacks per each attribute.
> 
> Signed-off-by: Andy Shevchenko 
> ---
>  drivers/usb/host/sl811-hcd.c | 17 +++--
>  1 file changed, 3 insertions(+), 14 deletions(-)

Please test-build your patches, this breaks the build :(

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

Re: [PATCH v7] staging: typec: handle vendor defined part and modify drp toggling flow

[Guenter Roeck]

On Tue, Mar 06, 2018 at 09:50:32AM +0800, ShuFan Lee wrote:
> From: ShuFan Lee 
> 
> Handle vendor defined behavior in tcpci_init, tcpci_set_vconn, 
> tcpci_start_drp_toggling
> and export tcpci_irq. More operations can be extended in tcpci_data if needed.
> According to TCPCI specification, 4.4.5.2 ROLE_CONTROL,
> TCPC shall not start DRP toggling until subsequently the TCPM
> writes to the COMMAND register to start DRP toggling.
> DRP toggling flow is changed as following:
>   - Write DRP = 1, Rp level and RC.CCx to Rd/Rd or Rp/Rp
>   - Set LOOK4CONNECTION command
> 
> Signed-off-by: ShuFan Lee 

I am fine with this version.

Reviewed-by: Guenter Roeck 

> ---
>  drivers/staging/typec/tcpci.c | 127 
> +-
>  drivers/staging/typec/tcpci.h |  15 +
>  2 files changed, 116 insertions(+), 26 deletions(-)
> 
>  patch changelogs between v1 & v2
>  - Remove unnecessary i2c_client in the structure of tcpci.
>  - Rename structure of tcpci_vendor_data to tcpci_data.
>  - Not exporting tcpci read/write wrappers but register i2c regmap in glue 
> driver.
>  - Add set_vconn ops in tcpci_data.
>(It is necessary for RT1711H to enable/disable idle mode before 
> disabling/enabling vconn)
>  - Export tcpci_irq so that vendor can call it in their own IRQ handler.
> 
>  patch changelogs between v2 & v3
>  - Change the return type of tcpci_irq from int to irqreturn_t.
> 
>  patch changelogs between v3 & v4
>  - Directly return regmap_write at the end of drp_toggling function.
>  - Because tcpci_irq is called in _tcpci_irq, move _tcpci_irq to the place 
> after tcpci_irq.
> 
>  patch changelogs between v4 & v5
>  - Add a space between my first & last name, from ShuFanLee to ShuFan Lee.
> 
>  patch changelogs between v5 & v6
>  - Add start_drp_toggling in tcpci_data and call it at the beginning of 
> tcpci_start_drp_toggling.
>  - Modify the flow of tcpci_start_drp_toggling as following:
> - Set Rp level, RC.CCx to Rd/Rd or Rp/Rp and DRP = 1
> - Set LOOK4CONNECTION command
> 
>  patch changelogs between v6 & v7
>  - Remove checking whether tcpci->data is NULL because it is guaranteed to be 
> available.
> 
> diff --git a/drivers/staging/typec/tcpci.c b/drivers/staging/typec/tcpci.c
> index 9bd4412..8043740 100644
> --- a/drivers/staging/typec/tcpci.c
> +++ b/drivers/staging/typec/tcpci.c
> @@ -21,7 +21,6 @@
>  
>  struct tcpci {
>   struct device *dev;
> - struct i2c_client *client;
>  
>   struct tcpm_port *port;
>  
> @@ -30,6 +29,12 @@ struct tcpci {
>   bool controls_vbus;
>  
>   struct tcpc_dev tcpc;
> + struct tcpci_data *data;
> +};
> +
> +struct tcpci_chip {
> + struct tcpci *tcpci;
> + struct tcpci_data data;
>  };
>  
>  static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev *tcpc)
> @@ -37,8 +42,7 @@ static inline struct tcpci *tcpc_to_tcpci(struct tcpc_dev 
> *tcpc)
>   return container_of(tcpc, struct tcpci, tcpc);
>  }
>  
> -static int tcpci_read16(struct tcpci *tcpci, unsigned int reg,
> - u16 *val)
> +static int tcpci_read16(struct tcpci *tcpci, unsigned int reg, u16 *val)
>  {
>   return regmap_raw_read(tcpci->regmap, reg, val, sizeof(u16));
>  }
> @@ -98,9 +102,17 @@ static int tcpci_set_cc(struct tcpc_dev *tcpc, enum 
> typec_cc_status cc)
>  static int tcpci_start_drp_toggling(struct tcpc_dev *tcpc,
>   enum typec_cc_status cc)
>  {
> + int ret;
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>   unsigned int reg = TCPC_ROLE_CTRL_DRP;
>  
> + /* Handle vendor drp toggling */
> + if (tcpci->data->start_drp_toggling) {
> + ret = tcpci->data->start_drp_toggling(tcpci, tcpci->data, cc);
> + if (ret < 0)
> + return ret;
> + }
> +
>   switch (cc) {
>   default:
>   case TYPEC_CC_RP_DEF:
> @@ -117,7 +129,17 @@ static int tcpci_start_drp_toggling(struct tcpc_dev 
> *tcpc,
>   break;
>   }
>  
> - return regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (cc == TYPEC_CC_RD)
> + reg |= (TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RD << TCPC_ROLE_CTRL_CC2_SHIFT);
> + else
> + reg |= (TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC1_SHIFT) |
> +(TCPC_ROLE_CTRL_CC_RP << TCPC_ROLE_CTRL_CC2_SHIFT);
> + ret = regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> + if (ret < 0)
> + return ret;
> + return regmap_write(tcpci->regmap, TCPC_COMMAND,
> + TCPC_CMD_LOOK4CONNECTION);
>  }
>  
>  static enum typec_cc_status tcpci_to_typec_cc(unsigned int cc, bool sink)
> @@ -178,6 +200,13 @@ static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool 
> enable)
>   struct tcpci *tcpci = tcpc_to_tcpci(tcpc);
>   int ret;
>  
> + /* Handle vendor set vconn */
> 

Re: [PATCH v2] usb: core: introduce per-port over-current counters

[Greg KH]

On Tue, Feb 20, 2018 at 12:50:33PM +0100, Richard Leitner wrote:
> From: Richard Leitner 
> 
> For some userspace applications information on the number of
> over-current conditions at specific USB hub ports is relevant.
> 
> In our case we have a series of USB hardware (using the cp210x driver)
> which communicates using a proprietary protocol. These devices sometimes
> trigger an over-current situation on some hubs. In case of such an
> over-current situation the USB devices offer an interface for reducing
> the max used power. As these conditions are quite rare and imply
> performance reductions of the device we don't want to reduce the max
> power always.
> 
> Therefore give user-space applications the possibility to react
> adequately by introducing an over_current_counter in the usb port struct
> which is exported via sysfs.
> 
> Signed-off-by: Richard Leitner 
> ---
> Tested on an i.MX6DL based board
> 
> Changes v2:
>   - rename oc_count to over_current_count
>   - add entry to Documentation/ABI
>   - add detailled description to commit message
> ---
>  Documentation/ABI/testing/sysfs-bus-usb |  9 +
>  drivers/usb/core/hub.c  |  4 +++-
>  drivers/usb/core/hub.h  |  1 +
>  drivers/usb/core/port.c | 10 ++
>  4 files changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-usb 
> b/Documentation/ABI/testing/sysfs-bus-usb
> index 0bd731cbb50c..27020293c85b 100644
> --- a/Documentation/ABI/testing/sysfs-bus-usb
> +++ b/Documentation/ABI/testing/sysfs-bus-usb
> @@ -189,6 +189,15 @@ Description:
>   The file will read "hotplug", "wired" and "not used" if the
>   information is available, and "unknown" otherwise.
>  
> +What:/sys/bus/usb/devices/.../(hub 
> interface)/portX/over_current_count
> +Date:February 2018
> +Contact: Richard Leitner 
> +Description:
> + Most hubs are able to detect over-current situations on their
> + ports and report them to the kernel. This attribute is to expose
> + the number of over-current situation occurred on a specific port
> + to user space. This file will contain an unsigned int.

"unsigned int" is very vague :)

How about "this is a 32bit value"?

And what happens when this wraps?  Is that allowed?

thanks,

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

Re: [PATCH 3/4] usb: host: xhci-plat: revert "usb: host: xhci-plat: enable clk in resume timing"

[Greg KH]

On Fri, Mar 09, 2018 at 01:48:37AM +, Yoshihiro Shimoda wrote:
> Hi Greg,
> 
> > From: Greg KH, Sent: Friday, March 9, 2018 2:06 AM
> > 
> > On Thu, Mar 08, 2018 at 05:17:16PM +0200, Mathias Nyman wrote:
> > > From: Yoshihiro Shimoda 
> > >
> > > This patch reverts the commit 835e4241e714 ("usb: host: xhci-plat:
> > > enable clk in resume timing") because this driver also has runtime PM
> > > and the commit 560869100b99 ("clk: renesas: cpg-mssr: Restore module
> > > clocks during resume") will restore the clock on R-Car H3 environment.
> > >
> > > If the xhci_plat_suspend() disables the clk, the system cannot enable
> > > the clk in resume like the following behavior:
> > >
> > > < In resume >
> > >  - genpd_resume_noirq() runs and enable the clk (enable_count = 1)
> > >  - cpg_mssr_resume_noirq() restores the clk register.
> > >   -- Since the clk was disabled in suspend, cpg_mssr_resume_noirq()
> > >  will disable the clk and keep the enable_count.
> > >  - Even if xhci_plat_resume() calls clk_prepare_enable(), since
> > >the enable_count is 1, the clk will be not enabled.
> > >
> > > After this patch is applied, the cpg-mssr driver will save the clk
> > > as enable, so the clk will be enabled in resume.
> > >
> > > Fixes: 835e4241e714 ("usb: host: xhci-plat: enable clk in resume timing")
> > > Signed-off-by: Yoshihiro Shimoda 
> > > Signed-off-by: Mathias Nyman 
> > 
> > This really should go to 4.15-stable, right?  I'll go add the tag if
> > needed, but want your confirmation first.
> 
> Since 4.15 is not longterm release, I think Renesas will not consume 4.15.

But it fixes a bug in 4.15, so someone might care :)

> So, I don't think this really should go to 4.15-stable.
> In such case, should I not add Fixes: tag? If not, I'll be careful in the 
> future.

No, the fixes tag was great.  I'll just leave it as-is.

thanks,

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

Re: [PATCH 4/4] xhci: Fix front USB ports on ASUS PRIME B350M-A

[Greg KH]

On Fri, Mar 09, 2018 at 11:29:20AM +0800, Kai Heng Feng wrote:
> Hi,
> 
> > On Mar 9, 2018, at 1:06 AM, Greg KH  wrote:
> > 
> > On Thu, Mar 08, 2018 at 05:17:17PM +0200, Mathias Nyman wrote:
> > > From: Kai-Heng Feng 
> > > 
> > > When a USB device gets plugged on ASUS PRIME B350M-A's front ports, the
> > > xHC stops working:
> > > [  549.114587] xhci_hcd :02:00.0: WARN: xHC CMD_RUN timeout
> > > [  549.114608] suspend_common(): xhci_pci_suspend+0x0/0xc0 returns -110
> > > [  549.114638] xhci_hcd :02:00.0: can't suspend
> > > (hcd_pci_runtime_suspend returned -110)
> > > 
> > > Delay before running xHC command CMD_RUN can workaround the issue.
> > > 
> > > Use a new quirk to make the delay only targets to the affected xHC.
> > > 
> > > Signed-off-by: Kai-Heng Feng 
> > > Signed-off-by: Mathias Nyman 
> > > ---
> > >  drivers/usb/host/xhci-pci.c | 3 +++
> > >  drivers/usb/host/xhci.c | 3 +++
> > >  drivers/usb/host/xhci.h | 1 +
> > >  3 files changed, 7 insertions(+)
> > 
> > Any reason this shouldn't go to the stable kernel trees?
> 
> It would be really nice to have this in stable tree.

Great, will do so, thanks for the quick response.

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

Re: [PATCH] usb: typec: Separate the definitions for data and power roles

[Guenter Roeck]

On Fri, Mar 09, 2018 at 02:09:18PM +0300, Heikki Krogerus wrote:
> USB Type-C specification v1.2 separated the power and data
> roles more clearly. Dual-Role-Data term was introduced, and
> the meaning of DRP was changed from "Dual-Role-Port" to
> "Dual-Role-Power".
> 
> In order to allow the port drivers to describe the
> capabilities of the ports more clearly according to the
> newest specifications, introducing separate definitions for
> the data roles.
> 
> Signed-off-by: Heikki Krogerus 

Reviewed-by: Guenter Roeck 

> ---
>  drivers/usb/typec/class.c   | 56 
> ++---
>  drivers/usb/typec/fusb302/fusb302.c |  1 +
>  drivers/usb/typec/tcpm.c|  9 +++---
>  drivers/usb/typec/tps6598x.c| 26 +++--
>  drivers/usb/typec/typec_wcove.c |  1 +
>  drivers/usb/typec/ucsi/ucsi.c   | 13 +++--
>  include/linux/usb/tcpm.h|  1 +
>  include/linux/usb/typec.h   | 14 --
>  8 files changed, 80 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
> index 2620a694704f..53df10df2f9d 100644
> --- a/drivers/usb/typec/class.c
> +++ b/drivers/usb/typec/class.c
> @@ -282,10 +282,10 @@ typec_altmode_roles_show(struct device *dev, struct 
> device_attribute *attr,
>   ssize_t ret;
>  
>   switch (mode->roles) {
> - case TYPEC_PORT_DFP:
> + case TYPEC_PORT_SRC:
>   ret = sprintf(buf, "source\n");
>   break;
> - case TYPEC_PORT_UFP:
> + case TYPEC_PORT_SNK:
>   ret = sprintf(buf, "sink\n");
>   break;
>   case TYPEC_PORT_DRP:
> @@ -797,14 +797,14 @@ static const char * const typec_data_roles[] = {
>  };
>  
>  static const char * const typec_port_types[] = {
> - [TYPEC_PORT_DFP] = "source",
> - [TYPEC_PORT_UFP] = "sink",
> + [TYPEC_PORT_SRC] = "source",
> + [TYPEC_PORT_SNK] = "sink",
>   [TYPEC_PORT_DRP] = "dual",
>  };
>  
>  static const char * const typec_port_types_drp[] = {
> - [TYPEC_PORT_DFP] = "dual [source] sink",
> - [TYPEC_PORT_UFP] = "dual source [sink]",
> + [TYPEC_PORT_SRC] = "dual [source] sink",
> + [TYPEC_PORT_SNK] = "dual source [sink]",
>   [TYPEC_PORT_DRP] = "[dual] source sink",
>  };
>  
> @@ -875,9 +875,7 @@ static ssize_t data_role_store(struct device *dev,
>   return ret;
>  
>   mutex_lock(>port_type_lock);
> - if (port->port_type != TYPEC_PORT_DRP) {
> - dev_dbg(dev, "port type fixed at \"%s\"",
> -  typec_port_types[port->port_type]);
> + if (port->cap->data != TYPEC_PORT_DRD) {
>   ret = -EOPNOTSUPP;
>   goto unlock_and_ret;
>   }
> @@ -897,7 +895,7 @@ static ssize_t data_role_show(struct device *dev,
>  {
>   struct typec_port *port = to_typec_port(dev);
>  
> - if (port->cap->type == TYPEC_PORT_DRP)
> + if (port->cap->data == TYPEC_PORT_DRD)
>   return sprintf(buf, "%s\n", port->data_role == TYPEC_HOST ?
>  "[host] device" : "host [device]");
>  
> @@ -1328,7 +1326,6 @@ struct typec_port *typec_register_port(struct device 
> *parent,
>  const struct typec_capability *cap)
>  {
>   struct typec_port *port;
> - int role;
>   int ret;
>   int id;
>  
> @@ -1354,21 +1351,36 @@ struct typec_port *typec_register_port(struct device 
> *parent,
>   goto err_mux;
>   }
>  
> - if (cap->type == TYPEC_PORT_DFP)
> - role = TYPEC_SOURCE;
> - else if (cap->type == TYPEC_PORT_UFP)
> - role = TYPEC_SINK;
> - else
> - role = cap->prefer_role;
> -
> - if (role == TYPEC_SOURCE) {
> - port->data_role = TYPEC_HOST;
> + switch (cap->type) {
> + case TYPEC_PORT_SRC:
>   port->pwr_role = TYPEC_SOURCE;
>   port->vconn_role = TYPEC_SOURCE;
> - } else {
> - port->data_role = TYPEC_DEVICE;
> + break;
> + case TYPEC_PORT_SNK:
>   port->pwr_role = TYPEC_SINK;
>   port->vconn_role = TYPEC_SINK;
> + break;
> + case TYPEC_PORT_DRP:
> + if (cap->prefer_role != TYPEC_NO_PREFERRED_ROLE)
> + port->pwr_role = cap->prefer_role;
> + else
> + port->pwr_role = TYPEC_SINK;
> + break;
> + }
> +
> + switch (cap->data) {
> + case TYPEC_PORT_DFP:
> + port->data_role = TYPEC_HOST;
> + break;
> + case TYPEC_PORT_UFP:
> + port->data_role = TYPEC_DEVICE;
> + break;
> + case TYPEC_PORT_DRD:
> + if (cap->prefer_role == TYPEC_SOURCE)
> + port->data_role = TYPEC_HOST;
> + else
> + port->data_role = TYPEC_DEVICE;
> + break;
>   }
>  
>   

Re: Without US_FL_NEEDS_CAP16 read_capacity_10 is forced despite of the device capability and HDD size

[Menion]

Yeah
I am in touch with scsi mantainers
I have already suggested to lower it to KERN_DEBUG, not really pushed
it, but they have said that from their perspective, this is a
sub-optimal read_capacity path taken, that shall be logged
Also, since this flag is named "try_rc_10_first" I have also suggested
that once the read_capacity_16 succeed and the attached device needs
the rad_capacity_16 due to large capacity, the scsi subsystem could
set this flag to 0 internally, but I am not sure how they feel about
this (if feasible), I am waiting for an answer
Bye

2018-03-09 17:29 GMT+01:00 Alan Stern :
> On Fri, 9 Mar 2018, Menion wrote:
>
>> Hi all
>> I am discussing about an issue I get with my Orico USBtoSATA 5 bays enclosure
>> The issue is that every 5 minutes the BTRFS perform a revalidate
>> action that will fill the dmesg with this log:
>>
>> [   98.917660] sd 0:0:0:1: [sdb] Very big device. Trying to use READ
>> CAPACITY(16).
>> [   99.057592] sd 0:0:0:0: [sda] Very big device. Trying to use READ
>> CAPACITY(16).
>> [   99.304848] sd 0:0:0:2: [sdc] Very big device. Trying to use READ
>> CAPACITY(16).
>> [   99.465374] sd 0:0:0:3: [sdd] Very big device. Trying to use READ
>> CAPACITY(16).
>> [   99.669241] sd 0:0:0:4: [sde] Very big device. Trying to use READ
>> CAPACITY(16).
>>
>> After some investigation I think I have found out the reason
>> The sd_read_capacity in scsi layer check if read_capacity_10 or
>> read_capacity_16 should be used
>> It does it based on what this function return:
>>
>> static int sd_try_rc16_first(struct scsi_device *sdp)
>> {
>> if (sdp->host->max_cmd_len < 16)
>> return 0;
>> if (sdp->try_rc_10_first)
>> return 0;
>> if (sdp->scsi_level > SCSI_SPC_2)
>> return 1;
>> if (scsi_device_protection(sdp))
>> return 1;
>> return 0;
>> }
>>
>> the problem is that in the usb to scsi glue layer, the try_rc_10_first
>> is picked like this:
>>
>> /*
>>  * Many devices do not respond properly to READ_CAPACITY_16.
>>  * Tell the SCSI layer to try READ_CAPACITY_10 first.
>>  * However some USB 3.0 drive enclosures return capacity
>>  * modulo 2TB. Those must use READ_CAPACITY_16
>>  */
>> if (!(us->fflags & US_FL_NEEDS_CAP16))
>> sdev->try_rc_10_first = 1;
>>
>> Are we sure this is the correct option? I mean to me the default
>> should be to go for rc_10 if there is a specific request in the
>> unusual devices, rather then if we dont' need cap16 we go for it,
>> like:
>>
>> if ((us->fflags & US_FL_NEEDS_CAP10))
>> sdev->try_rc_10_first = 1;
>>
>> What you think about?
>
> I think there used to be a lot of USB devices which would work with
> READ CAPACITY 10 but would not work with READ CAPACITY 16.  The
> situation may not be quite so bad nowadays, but I would still prefer to
> have the kernel try RC10 first in all cases except when the NEEDS_CAP16
> flag is set.
>
> As for the the SCSI messages filling your log every 5 minutes, perhaps
> the SCSI maintainers could be convinced to change that message to be
> debug level.  It would still occupy space in the kernel's log buffer,
> but by default it would be ignored by userspace logging programs.
>
> If we changed the strategy the way you suggest, we would probably get a
> lot of errors from older devices that don't support RC16.  Some of them
> might even crash when they receive the command, which would make them
> unusable until a NEEDS_CAP10 flag entry was created for them.
>
> Or to put it another way, I think the population of devices which
> misbehave when they receive RC16 is probably larger than the population
> of devices which misbehave when they receive RC10.  :-)
>
> 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: Without US_FL_NEEDS_CAP16 read_capacity_10 is forced despite of the device capability and HDD size

[Alan Stern]

On Fri, 9 Mar 2018, Menion wrote:

> Hi all
> I am discussing about an issue I get with my Orico USBtoSATA 5 bays enclosure
> The issue is that every 5 minutes the BTRFS perform a revalidate
> action that will fill the dmesg with this log:
> 
> [   98.917660] sd 0:0:0:1: [sdb] Very big device. Trying to use READ
> CAPACITY(16).
> [   99.057592] sd 0:0:0:0: [sda] Very big device. Trying to use READ
> CAPACITY(16).
> [   99.304848] sd 0:0:0:2: [sdc] Very big device. Trying to use READ
> CAPACITY(16).
> [   99.465374] sd 0:0:0:3: [sdd] Very big device. Trying to use READ
> CAPACITY(16).
> [   99.669241] sd 0:0:0:4: [sde] Very big device. Trying to use READ
> CAPACITY(16).
> 
> After some investigation I think I have found out the reason
> The sd_read_capacity in scsi layer check if read_capacity_10 or
> read_capacity_16 should be used
> It does it based on what this function return:
> 
> static int sd_try_rc16_first(struct scsi_device *sdp)
> {
> if (sdp->host->max_cmd_len < 16)
> return 0;
> if (sdp->try_rc_10_first)
> return 0;
> if (sdp->scsi_level > SCSI_SPC_2)
> return 1;
> if (scsi_device_protection(sdp))
> return 1;
> return 0;
> }
> 
> the problem is that in the usb to scsi glue layer, the try_rc_10_first
> is picked like this:
> 
> /*
>  * Many devices do not respond properly to READ_CAPACITY_16.
>  * Tell the SCSI layer to try READ_CAPACITY_10 first.
>  * However some USB 3.0 drive enclosures return capacity
>  * modulo 2TB. Those must use READ_CAPACITY_16
>  */
> if (!(us->fflags & US_FL_NEEDS_CAP16))
> sdev->try_rc_10_first = 1;
> 
> Are we sure this is the correct option? I mean to me the default
> should be to go for rc_10 if there is a specific request in the
> unusual devices, rather then if we dont' need cap16 we go for it,
> like:
> 
> if ((us->fflags & US_FL_NEEDS_CAP10))
> sdev->try_rc_10_first = 1;
> 
> What you think about?

I think there used to be a lot of USB devices which would work with 
READ CAPACITY 10 but would not work with READ CAPACITY 16.  The 
situation may not be quite so bad nowadays, but I would still prefer to 
have the kernel try RC10 first in all cases except when the NEEDS_CAP16 
flag is set.

As for the the SCSI messages filling your log every 5 minutes, perhaps 
the SCSI maintainers could be convinced to change that message to be
debug level.  It would still occupy space in the kernel's log buffer, 
but by default it would be ignored by userspace logging programs.

If we changed the strategy the way you suggest, we would probably get a 
lot of errors from older devices that don't support RC16.  Some of them 
might even crash when they receive the command, which would make them 
unusable until a NEEDS_CAP10 flag entry was created for them.

Or to put it another way, I think the population of devices which
misbehave when they receive RC16 is probably larger than the population
of devices which misbehave when they receive RC10.  :-)

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

[RFC PATCH v2 0/3] usb: typec: Support for Alternate Modes

[Heikki Krogerus]

Hi guys,

This is second version of my proposal for more complete USB Type-C
Alternate Mode support. The original proposal can be read from here:
https://www.spinics.net/lists/linux-usb/msg161098.html

These patches now depend on series from Hans where he is introducing
mux handling support for USB Type-C and USB in general:
https://lkml.org/lkml/2018/3/2/340

The major difference compared to v1 is that I'm proposing change to
the sysfs ABI we have for the alternate mode devices. The files are
not changed, but they are moved to the parent directory from the
mode folder. Since the alternate mode devices are not yet used
and in practice not supported in mainline, I felt brave enough to
propose that.

The reason for removing the mode folder is because as in patch
1/3 I now create a device for every mode of every SVID, there will
never be more then one mode folder. I.e. the folder serves no purpose.
The mode is still kept for now, but it's just deprecated.

There are no alternate mode drivers included yet in this version.


Original commit message (subject was "usb: typec: alternate mode
bus"):

The bus allows SVID specific communication with the partners to be
handled in separate drivers for each alternate mode.

Alternate mode handling happens with two separate logical devices:
1. Partner alternate mode devices which represent the alternate modes
   on the partner. The driver for them will handle the alternate mode
   specific communication with the partner using VDMs.
2. Port alternate mode devices which represent connections from the
   USB Type-C port to devices on the platform.

The drivers will be bind to the partner alternate modes. The alternate
mode drivers will need to deliver the result of the negotiated pin
configurations to the rest of the platform (towards the port alternate
mode devices). This series includes API for that, however, not the
final implementation yet.

The connections to the other devices on the platform the ports have
can be described by using the remote endpoint concept [1][2] on ACPI
and DT platforms, but I have no solution for the "platform data" case
where we have neither DT nor ACPI to describe the connections for us.

[1] Documentation/devicetree/bindings/graph.txt
[2] Documentation/acpi/dsd/graph.txt


Heikki Krogerus (3):
  usb: typec: Register a device for every mode
  usb: typec: Bus type for alternate modes
  usb: typec: tcpm: Support for Alternate Modes

 Documentation/ABI/obsolete/sysfs-class-typec |  48 +++
 Documentation/ABI/testing/sysfs-bus-typec|  51 
 Documentation/ABI/testing/sysfs-class-typec  |  62 +---
 Documentation/driver-api/usb/typec_bus.rst   | 143 +
 drivers/usb/typec/Makefile   |   2 +-
 drivers/usb/typec/bus.c  | 421 ++
 drivers/usb/typec/bus.h  |  37 +++
 drivers/usb/typec/class.c| 424 ++-
 drivers/usb/typec/tcpm.c | 156 +++---
 include/linux/mod_devicetable.h  |  15 +
 include/linux/usb/typec.h|  50 +---
 include/linux/usb/typec_altmode.h| 136 +
 scripts/mod/devicetable-offsets.c|   4 +
 scripts/mod/file2alias.c |  13 +
 14 files changed, 1285 insertions(+), 277 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-class-typec
 create mode 100644 Documentation/ABI/testing/sysfs-bus-typec
 create mode 100644 Documentation/driver-api/usb/typec_bus.rst
 create mode 100644 drivers/usb/typec/bus.c
 create mode 100644 drivers/usb/typec/bus.h
 create mode 100644 include/linux/usb/typec_altmode.h

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

[RFC PATCH v2 2/3] usb: typec: Bus type for alternate modes

[Heikki Krogerus]

Introducing a simple bus for the alternate modes. Bus allows
binding drivers to the discovered alternate modes the
partners support.

Signed-off-by: Heikki Krogerus 
---
 Documentation/ABI/obsolete/sysfs-class-typec |  48 +++
 Documentation/ABI/testing/sysfs-bus-typec|  51 
 Documentation/ABI/testing/sysfs-class-typec  |  62 +---
 Documentation/driver-api/usb/typec_bus.rst   | 143 +
 drivers/usb/typec/Makefile   |   2 +-
 drivers/usb/typec/bus.c  | 421 +++
 drivers/usb/typec/bus.h  |  37 +++
 drivers/usb/typec/class.c| 325 +
 include/linux/mod_devicetable.h  |  15 +
 include/linux/usb/typec.h|  16 +-
 include/linux/usb/typec_altmode.h| 136 +
 scripts/mod/devicetable-offsets.c|   4 +
 scripts/mod/file2alias.c |  13 +
 13 files changed, 1138 insertions(+), 135 deletions(-)
 create mode 100644 Documentation/ABI/obsolete/sysfs-class-typec
 create mode 100644 Documentation/ABI/testing/sysfs-bus-typec
 create mode 100644 Documentation/driver-api/usb/typec_bus.rst
 create mode 100644 drivers/usb/typec/bus.c
 create mode 100644 drivers/usb/typec/bus.h
 create mode 100644 include/linux/usb/typec_altmode.h

diff --git a/Documentation/ABI/obsolete/sysfs-class-typec 
b/Documentation/ABI/obsolete/sysfs-class-typec
new file mode 100644
index ..32623514ee87
--- /dev/null
+++ b/Documentation/ABI/obsolete/sysfs-class-typec
@@ -0,0 +1,48 @@
+These files are deprecated and will be removed. The same files are available
+under /sys/bus/typec (see Documentation/ABI/testing/sysfs-bus-typec).
+
+What:  /sys/class/typec///svid
+Date:  April 2017
+Contact:   Heikki Krogerus 
+Description:
+   The SVID (Standard or Vendor ID) assigned by USB-IF for this
+   alternate mode.
+
+What:  /sys/class/typec///mode/
+Date:  April 2017
+Contact:   Heikki Krogerus 
+Description:
+   Every supported mode will have its own directory. The name of
+   a mode will be "mode" (for example mode1), where 
+   is the actual index to the mode VDO returned by Discover Modes
+   USB power delivery command.
+
+What:  
/sys/class/typec///mode/description
+Date:  April 2017
+Contact:   Heikki Krogerus 
+Description:
+   Shows description of the mode. The description is optional for
+   the drivers, just like with the Billboard Devices.
+
+What:  /sys/class/typec///mode/vdo
+Date:  April 2017
+Contact:   Heikki Krogerus 
+Description:
+   Shows the VDO in hexadecimal returned by Discover Modes command
+   for this mode.
+
+What:  /sys/class/typec///mode/active
+Date:  April 2017
+Contact:   Heikki Krogerus 
+Description:
+   Shows if the mode is active or not. The attribute can be used
+   for entering/exiting the mode with partners and cable plugs, and
+   with the port alternate modes it can be used for disabling
+   support for specific alternate modes. Entering/exiting modes is
+   supported as synchronous operation so write(2) to the attribute
+   does not return until the enter/exit mode operation has
+   finished. The attribute is notified when the mode is
+   entered/exited so poll(2) on the attribute wakes up.
+   Entering/exiting a mode will also generate uevent KOBJ_CHANGE.
+
+   Valid values: yes, no
diff --git a/Documentation/ABI/testing/sysfs-bus-typec 
b/Documentation/ABI/testing/sysfs-bus-typec
new file mode 100644
index ..ead63f97d9a2
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-typec
@@ -0,0 +1,51 @@
+What:  /sys/bus/typec/devices/.../active
+Date:  April 2018
+Contact:   Heikki Krogerus 
+Description:
+   Shows if the mode is active or not. The attribute can be used
+   for entering/exiting the mode. Entering/exiting modes is
+   supported as synchronous operation so write(2) to the attribute
+   does not return until the enter/exit mode operation has
+   finished. The attribute is notified when the mode is
+   entered/exited so poll(2) on the attribute wakes up.
+   Entering/exiting a mode will also generate uevent KOBJ_CHANGE.
+
+   Valid values are boolean.
+
+What:  /sys/bus/typec/devices/.../description

[RFC PATCH v2 1/3] usb: typec: Register a device for every mode

[Heikki Krogerus]

Before a device was created for every discovered SVID, but
this will create a device for every discovered mode of every
SVID. The idea is to make it easier to create mode specific
drivers once a bus for the alternate mode is added.

Signed-off-by: Heikki Krogerus 
---
 drivers/usb/typec/class.c | 163 +++---
 drivers/usb/typec/tcpm.c  |  45 ++---
 include/linux/usb/typec.h |  34 +++---
 3 files changed, 79 insertions(+), 163 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 53df10df2f9d..26eeab1491b7 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -13,31 +13,20 @@
 #include 
 #include 
 
-struct typec_mode {
-   int index;
+struct typec_altmode {
+   struct device   dev;
+   u16 svid;
+   u8  mode;
+
u32 vdo;
char*desc;
enum typec_port_typeroles;
-
-   struct typec_altmode*alt_mode;
-
unsigned intactive:1;
 
+   struct attribute*attrs[5];
chargroup_name[6];
struct attribute_group  group;
-   struct attribute*attrs[5];
-   struct device_attribute vdo_attr;
-   struct device_attribute desc_attr;
-   struct device_attribute active_attr;
-   struct device_attribute roles_attr;
-};
-
-struct typec_altmode {
-   struct device   dev;
-   u16 svid;
-   int n_modes;
-   struct typec_mode   modes[ALTMODE_MAX_MODES];
-   const struct attribute_group*mode_groups[ALTMODE_MAX_MODES];
+   const struct attribute_group*groups[2];
 };
 
 struct typec_plug {
@@ -186,13 +175,12 @@ static void typec_report_identity(struct device *dev)
 void typec_altmode_update_active(struct typec_altmode *alt, int mode,
 bool active)
 {
-   struct typec_mode *m = >modes[mode];
char dir[6];
 
-   if (m->active == active)
+   if (alt->active == active)
return;
 
-   m->active = active;
+   alt->active = active;
snprintf(dir, sizeof(dir), "mode%d", mode);
sysfs_notify(>dev.kobj, dir, "active");
kobject_uevent(>dev.kobj, KOBJ_CHANGE);
@@ -220,42 +208,37 @@ struct typec_port *typec_altmode2port(struct 
typec_altmode *alt)
 EXPORT_SYMBOL_GPL(typec_altmode2port);
 
 static ssize_t
-typec_altmode_vdo_show(struct device *dev, struct device_attribute *attr,
-  char *buf)
+vdo_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-   struct typec_mode *mode = container_of(attr, struct typec_mode,
-  vdo_attr);
+   struct typec_altmode *alt = to_altmode(dev);
 
-   return sprintf(buf, "0x%08x\n", mode->vdo);
+   return sprintf(buf, "0x%08x\n", alt->vdo);
 }
+static DEVICE_ATTR_RO(vdo);
 
 static ssize_t
-typec_altmode_desc_show(struct device *dev, struct device_attribute *attr,
-   char *buf)
+description_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-   struct typec_mode *mode = container_of(attr, struct typec_mode,
-  desc_attr);
+   struct typec_altmode *alt = to_altmode(dev);
 
-   return sprintf(buf, "%s\n", mode->desc ? mode->desc : "");
+   return sprintf(buf, "%s\n", alt->desc ? alt->desc : "");
 }
+static DEVICE_ATTR_RO(description);
 
 static ssize_t
-typec_altmode_active_show(struct device *dev, struct device_attribute *attr,
- char *buf)
+active_show(struct device *dev, struct device_attribute *attr, char *buf)
 {
-   struct typec_mode *mode = container_of(attr, struct typec_mode,
-  active_attr);
+   struct typec_altmode *alt = to_altmode(dev);
 
-   return sprintf(buf, "%s\n", mode->active ? "yes" : "no");
+   return sprintf(buf, "%s\n", alt->active ? "yes" : "no");
 }
 
 static ssize_t
-typec_altmode_active_store(struct device *dev, struct device_attribute *attr,
+active_store(struct device *dev, struct device_attribute *attr,
   const char *buf, size_t size)
 {
-   struct typec_mode *mode = container_of(attr, struct typec_mode,
-  active_attr);
-   struct typec_port *port = typec_altmode2port(mode->alt_mode);
+   struct typec_altmode *alt = to_altmode(dev);
+   struct typec_port *port = typec_altmode2port(alt);
bool activate;
int ret;
 
@@ -266,22 +249,22 @@ typec_altmode_active_store(struct device *dev, struct 

[RFC PATCH v2 3/3] usb: typec: tcpm: Support for Alternate Modes

[Heikki Krogerus]

This adds more complete handling of VDMs and registration of
partner alternate modes, and introduces callbacks for
alternate mode operations.

Only DFP role is supported for now.

Signed-off-by: Heikki Krogerus 
---
 drivers/usb/typec/tcpm.c | 133 +++
 1 file changed, 111 insertions(+), 22 deletions(-)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index bfb843ebffa6..34bf5c1f4d81 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -158,13 +158,14 @@ enum pd_msg_request {
 /* Alternate mode support */
 
 #define SVID_DISCOVERY_MAX 16
+#define ALTMODE_DISCOVERY_MAX  (SVID_DISCOVERY_MAX * MODE_DISCOVERY_MAX)
 
 struct pd_mode_data {
int svid_index; /* current SVID index   */
int nsvids;
u16 svids[SVID_DISCOVERY_MAX];
int altmodes;   /* number of alternate modes*/
-   struct typec_altmode_desc altmode_desc[SVID_DISCOVERY_MAX];
+   struct typec_altmode_desc altmode_desc[ALTMODE_DISCOVERY_MAX];
 };
 
 struct tcpm_port {
@@ -280,8 +281,8 @@ struct tcpm_port {
/* Alternate mode data */
 
struct pd_mode_data mode_data;
-   struct typec_altmode *partner_altmode[SVID_DISCOVERY_MAX * 6];
-   struct typec_altmode *port_altmode[SVID_DISCOVERY_MAX * 6];
+   struct typec_altmode *partner_altmode[ALTMODE_DISCOVERY_MAX];
+   struct typec_altmode *port_altmode[ALTMODE_DISCOVERY_MAX];
 
/* Deadline in jiffies to exit src_try_wait state */
unsigned long max_wait;
@@ -985,36 +986,53 @@ static void svdm_consume_modes(struct tcpm_port *port, 
const __le32 *payload,
 pmdata->altmodes, paltmode->svid,
 paltmode->mode, paltmode->vdo);
 
-   port->partner_altmode[pmdata->altmodes] =
-   typec_partner_register_altmode(port->partner, paltmode);
-   if (!port->partner_altmode[pmdata->altmodes]) {
-   tcpm_log(port,
-"Failed to register modes for SVID 0x%04x",
-paltmode->svid);
-   return;
-   }
pmdata->altmodes++;
}
 }
 
+static void tcpm_register_partner_altmodes(struct tcpm_port *port)
+{
+   struct pd_mode_data *modep = >mode_data;
+   struct typec_altmode *altmode;
+   int i;
+
+   for (i = 0; i < modep->altmodes; i++) {
+   altmode = typec_partner_register_altmode(port->partner,
+   >altmode_desc[i]);
+   if (!altmode)
+   tcpm_log(port, "Failed to register partner SVID 0x%04x",
+modep->altmode_desc[i].svid);
+   port->partner_altmode[i] = altmode;
+   }
+}
+
 #define supports_modal(port)   
PD_IDH_MODAL_SUPP((port)->partner_ident.id_header)
 
 static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
u32 *response)
 {
-   u32 p0 = le32_to_cpu(payload[0]);
-   int cmd_type = PD_VDO_CMDT(p0);
-   int cmd = PD_VDO_CMD(p0);
+   struct typec_altmode *altmode;
struct pd_mode_data *modep;
+   u32 p[PD_MAX_PAYLOAD];
int rlen = 0;
-   u16 svid;
+   int cmd_type;
+   int cmd;
int i;
 
+   for (i = 0; i < cnt; i++)
+   p[i] = le32_to_cpu(payload[i]);
+
+   cmd_type = PD_VDO_CMDT(p[0]);
+   cmd = PD_VDO_CMD(p[0]);
+
tcpm_log(port, "Rx VDM cmd 0x%x type %d cmd %d len %d",
-p0, cmd_type, cmd, cnt);
+p[0], cmd_type, cmd, cnt);
 
modep = >mode_data;
 
+   altmode = typec_match_altmode(port->port_altmode, ALTMODE_DISCOVERY_MAX,
+ PD_VDO_VID(p[0]), PD_VDO_OPOS(p[0]));
+
switch (cmd_type) {
case CMDT_INIT:
switch (cmd) {
@@ -1036,17 +1054,21 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const 
__le32 *payload, int cnt,
case CMD_EXIT_MODE:
break;
case CMD_ATTENTION:
-   break;
+   typec_altmode_attention(altmode, p[1]);
+   return 0;
default:
+   if (typec_altmode_vdm(altmode, p[0], [1], cnt) ==
+   VDM_OK)
+   return 0;
break;
}
if (rlen >= 1) {
-   response[0] = p0 | VDO_CMDT(CMDT_RSP_ACK);
+   response[0] = p[0] | VDO_CMDT(CMDT_RSP_ACK);
} else if (rlen == 0) {
-   response[0] = p0 | VDO_CMDT(CMDT_RSP_NAK);
+   response[0] = p[0] | VDO_CMDT(CMDT_RSP_NAK);
rlen = 1;
} else {
-   

Re: [PATCHv4] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

[Tony Lindgren]

* Pavel Machek  [180309 09:45]:
> Hi!
> 
> > Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> > It is used on some Motorola Mapphone series of phones and tablets such
> > as Droid 4.
> ...
> > So it seems that MDM6600 is currently not yet idling even with it's
> > radio turned off, but that's something that is beyond the control of
> > this USB PHY driver. This patch does get us to the point where modem
> > data and GPS are usable with libqmi and ModemManager for example.
> > Voice calls need more audio driver work.
> 
> Thanks for the good work. Looks like I'll need to get droid
> 4... fortunately it is not available in czech republic so I don't get
> excuse to get another toy.
> 
> Oh.. no. It is available in Czech republic. Is Motorola Droid 4 XT894
> the right one? Hmm, and LTE modem is useless in Europe, while the GSM
> one does not work, right?

Yup xt894 is the model number for droid 4.

Regards,

Tony


--
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: musb: Fix external abort in musb_remove on omap2430

[Bin Liu]

Hi,

On Thu, Mar 08, 2018 at 11:19:48PM +0100, Merlijn Wajer wrote:
> This fixes an oops on unbind / module unload (on the musb omap2430
> platform).
> 
> musb_remove function now calls musb_platform_exit before disabling
> runtime pm.
> 
> Signed-off-by: Merlijn Wajer 

Applied. Thanks.

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 v1 10/14] usb: dwc2: Add dwc2_enter_hibernation(), dwc2_exit_hibernation()

[Grigor Tovmasyan]

Hi Balbi,

This patch didn't apply, because you missing 01/14 and 02/14 commits
from this patch seria.
Please apply following patches and try again.
[PATCH v1 01/14] usb: dwc2: Rename hibernation to partial_power_down
[PATCH v1 02/14] usb: dwc2: Add hibernation field into dwc2_hw_params

On 3/9/2018 5:10 PM, Grigor Tovmasyan wrote:
> Hi  Balbi,
>
> Seems like your testing/next was changed.
> I will rebese and send them again.
>
> On 3/9/2018 1:07 PM, Felipe Balbi wrote:
>> Grigor Tovmasyan  writes:
>>
>>> From: Vardan Mikayelyan 
>>>
>>> These are wrapper functions which are calling device or host
>>> enter/exit hibernation functions.
>>>
>>> Signed-off-by: Vardan Mikayelyan 
>>> Signed-off-by: John Youn 
>>> Signed-off-by: Grigor Tovmasyan 
>> didn't apply to testing/next. Care to rebase on testing/next?
>>
>> best
>>
> BR,
> 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: musb: Fix external abort in musb_remove

[Bin Liu]

On Thu, Mar 08, 2018 at 11:17:48PM +0100, Merlijn Wajer wrote:
> Hi,
> 
> On 08/03/18 22:15, Bin Liu wrote:
> 
> > please add patch version numbers in the subject when necessary. This
> > helps cross-referencing.
> 
> Will do. I naively assumed that the first patch would implicitly be
> number 1. Will send out v2 now.

Sorry, my bad, I forgot the first patch was a RFC. You are doing it
right.

> 
> >>  
> >> +  musb_writeb(musb->mregs, MUSB_DEVCTL, 0);
> > 
> > Does it solve the issue if not moving this line? I'd like to have
> > minimum change if possible.
> 
> Yes, it does. The only reason I moved musb_writeb is because I
> understood you wanted me to move both (per previous message: "This can
> be move down to out side of holding the spinlock").

Typically the comments would only applies to the modified code.
Otherwise the comments have to be explicit to avoid confusion.
No worries here anyway, confusion does happen sometimes ;)

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 v1 10/14] usb: dwc2: Add dwc2_enter_hibernation(), dwc2_exit_hibernation()

[Grigor Tovmasyan]

Hi  Balbi,

Seems like your testing/next was changed.
I will rebese and send them again.

On 3/9/2018 1:07 PM, Felipe Balbi wrote:
> Grigor Tovmasyan  writes:
>
>> From: Vardan Mikayelyan 
>>
>> These are wrapper functions which are calling device or host
>> enter/exit hibernation functions.
>>
>> Signed-off-by: Vardan Mikayelyan 
>> Signed-off-by: John Youn 
>> Signed-off-by: Grigor Tovmasyan 
> didn't apply to testing/next. Care to rebase on testing/next?
>
> best
>

BR,
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 11/11] usb: dwc2: Enable LPM

[Grigor Tovmasyan]

Hi John,

Could you please add your Signed-off to this patch?

On 3/9/2018 12:06 PM, Felipe Balbi wrote:
> Hi,
>
> Grigor Tovmasyan  writes:
>> From: John Youn 
>>
>> Set 'lpm_capable' flag in the gadget structure so
>> indicating that LPM is supported.
>>
>> Signed-off-by: Sevak Arakelyan 
>> Signed-off-by: Grigor Tovmasyan 
> Missing John's Signed-off-by. What gives? I'll drop this from my queue
> for now.
>

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: [balbi-usb:testing/next 45/57] drivers/usb/dwc2/core.c:438:13: error: static declaration of 'dwc2_clear_force_mode' follows non-static declaration

[Grigor Tovmasyan]

Hi Balbi,

I saw you applied "e9c07984979d usb: dwc2: Force mode optimizations"
patch to your testing/next.
Please re-apply this commit.

On 2/15/2018 6:37 PM, Grigor Tovmasyan wrote:
> Hi Balbi
>
> On 2/15/2018 17:34, Felipe Balbi wrote:
>> Hi,
>>
>> kbuild test robot  writes:
>>> tree:   
>>> https://urldefense.proofpoint.com/v2/url?u=https-3A__git.kernel.org_pub_scm_linux_kernel_git_balbi_usb.git=DwIFAg=DPL6_X_6JkXFx7AXWqB0tg=K1ULVL1slpLXpMJJlAXSOxws4tRq0IkTBqxDkyW2hUQ=2Hq2CZMELUidsi9x-Kqf4yuAh4IoxCX3Th1gZtmyZmc=Kw25jbj17VVP-cEEsCvpsfmS6NIuM8JAI8vfOtC2Nt4=
>>>  testing/next
>>> head:   65223a0ed97c8c4b18c4899653745058f87d67e3
>>> commit: 4caf1fe8fb537388810a4c8fecfa5324b26f80ea [45/57] usb: dwc2: Make 
>>> dwc2_force_mode() static
>>> config: i386-randconfig-x072-201806 (attached as .config)
>>> compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
>>> reproduce:
>>>  git checkout 4caf1fe8fb537388810a4c8fecfa5324b26f80ea
>>>  # save the attached .config to linux build tree
>>>  make ARCH=i386
>>>
>>> All errors (new ones prefixed by >>):
>>>
> drivers/usb/dwc2/core.c:438:13: error: static declaration of 
> 'dwc2_clear_force_mode' follows non-static declaration
>>>  static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>>>  ^
>>> In file included from drivers/usb/dwc2/core.c:57:0:
>>> drivers/usb/dwc2/core.h:1101:6: note: previous declaration of 
>>> 'dwc2_clear_force_mode' was here
>>>  void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg);
>>>   ^
>>>
>>> vim +/dwc2_clear_force_mode +438 drivers/usb/dwc2/core.c
>>>
>>> 428 
>>> 429 /**
>>> 430  * dwc2_clear_force_mode() - Clears the force mode bits.
>>> 431  *
>>> 432  * After clearing the bits, wait up to 100 ms to account for any
>>> 433  * potential IDDIG filter delay. We can't know if we expect 
>>> this delay
>>> 434  * or not because the value of the connector ID status is 
>>> affected by
>>> 435  * the force mode. We only need to call this once during probe 
>>> if
>>> 436  * dr_mode == OTG.
>>> 437  */
>>>   > 438 static void dwc2_clear_force_mode(struct dwc2_hsotg *hsotg)
>> patch dropped.
>>
> This patch have a dependency from "[PATCH 6/6] usb: dwc2: Force mode"
> ("[PATCH 0/6] usb: dwc2: minor fixes" patches) which I missed.
>
> I will rebase
> [PATCH 5/6] usb: dwc2: eliminate irq parameter from dwc2_gadget_init
> [PATCH 6/6] usb: dwc2: Force mode
>
> Please apply this patch after applying that two patches.
>
> 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

[PATCH v2] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

In the following test we get stuck by sleeping forever in _dwc3_set_mode()
after which dual-role switching doesn't work.

On dra7-evm's dual-role port,
- Load g_zero gadget driver and enumerate to host
- suspend to mem
- disconnect USB cable to host and connect otg cable with Pen drive in it.
- resume system
- we sleep indefinitely in _dwc3_set_mode due to.
  dwc3_gadget_exit()->usb_del_gadget_udc()->udc_stop()->
dwc3_gadget_stop()->wait_event_lock_irq()

To fix this instead of waiting indefinitely with wait_event_lock_irq()
we use wait_event_interruptible_lock_irq_timeout() and print
and error message if there was a timeout.

Signed-off-by: Roger Quadros 
---

Changelog:

v2:
- use wait_event_interruptible_lock_irq_timeout() instead of 
wait_event_lock_irq()

 drivers/usb/dwc3/gadget.c | 23 ---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2bda4eb..7c3a6e4 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1950,6 +1950,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
struct dwc3 *dwc = gadget_to_dwc(g);
unsigned long   flags;
int epnum;
+   u32 tmo_eps = 0;
 
spin_lock_irqsave(>lock, flags);
 
@@ -1960,6 +1961,7 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
 
for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
struct dwc3_ep  *dep = dwc->eps[epnum];
+   int ret;
 
if (!dep)
continue;
@@ -1967,9 +1969,24 @@ static int dwc3_gadget_stop(struct usb_gadget *g)
if (!(dep->flags & DWC3_EP_END_TRANSFER_PENDING))
continue;
 
-   wait_event_lock_irq(dep->wait_end_transfer,
-   !(dep->flags & 
DWC3_EP_END_TRANSFER_PENDING),
-   dwc->lock);
+   ret = 
wait_event_interruptible_lock_irq_timeout(dep->wait_end_transfer,
+   !(dep->flags & DWC3_EP_END_TRANSFER_PENDING),
+   dwc->lock, msecs_to_jiffies(5));
+
+   if (ret <= 0) {
+   /* Timed out or interrupted! There's nothing much
+* we can do so we just log here and print which
+* endpoints timed out at the end.
+*/
+   tmo_eps |= 1 << epnum;
+   dep->flags &= DWC3_EP_END_TRANSFER_PENDING;
+   }
+   }
+
+   if (tmo_eps) {
+   dev_err(dwc->dev,
+   "end transfer timed out on endpoints 0x%x [bitmap]\n",
+   tmo_eps);
}
 
 out:
-- 
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 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/3] usb: xhci: tegra: Add runtime PM support

[Jon Hunter]

On 09/03/18 09:13, Mathias Nyman wrote:
> On 09.03.2018 10:36, Thierry Reding wrote:
>> On Thu, Mar 08, 2018 at 09:31:07PM +, Jon Hunter wrote:
>>>
>>> On 01/03/18 14:18, Mathias Nyman wrote:
 On 14.02.2018 18:34, Jon Hunter wrote:
> Add runtime PM support to the Tegra XHCI driver and move the function
> calls to enable/disable the clocks, regulators and PHY into the
> runtime
> PM callbacks.
>
> Signed-off-by: Jon Hunter 
> ---
>    drivers/usb/host/xhci-tegra.c | 80
> ++-
>    1 file changed, 56 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/usb/host/xhci-tegra.c
> b/drivers/usb/host/xhci-tegra.c
> index 02b0b24faa58..42aa67858b53 100644
> --- a/drivers/usb/host/xhci-tegra.c
> +++ b/drivers/usb/host/xhci-tegra.c
> @@ -18,6 +18,7 @@
>    #include 
>    #include 
>    #include 
> +#include 
>    #include 
>    #include 
>    #include 
> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
> platform_device *pdev)
>     */
>    platform_set_drvdata(pdev, tegra);
>    -    err = tegra_xusb_clk_enable(tegra);
> -    if (err) {
> -    dev_err(>dev, "failed to enable clocks: %d\n", err);
> -    goto put_usb2;
> -    }
> -
> -    err = regulator_bulk_enable(tegra->soc->num_supplies,
> tegra->supplies);
> -    if (err) {
> -    dev_err(>dev, "failed to enable regulators: %d\n",
> err);
> -    goto disable_clk;
> -    }
> +    pm_runtime_enable(>dev);
>    -    err = tegra_xusb_phy_enable(tegra);
> +    err = pm_runtime_get_sync(>dev);
>    if (err < 0) {

 Does this mean that if runtime PM is disabled then clocks and regulator
 will never be enabled
 for Tegra xhci?

 How about keeping the clock and regualtor enabling in probe, and
 instead
 add something like:

 pm_runtime_set_active(>dev);
 pm_runtime_enable(>dev);
 pm_runtime_get_noresume(>dev);
>>>
>>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>>> is disabled.
>>>
>>> Typically we do something like ...
>>>
>>>  pm_runtime_enable(>dev);
>>>  if (!pm_runtime_enabled(>dev))
>>> ret = tegra_xusb_runtime_resume(>dev);
>>>  else
>>>  ret = pm_runtime_get_sync(>dev);
>>>
>>> That way we can keep the regulator and clock stuff in the handler. I
>>> will update this series.
>>
>> Is there any good reason why we don't depend on PM for 32-bit as well?
>> I'm not aware of any differences in drivers that are 32-bit specific for
>> Tegra, and I'm not even sure the !PM case gets any testing at all. And
>> even if, do we really still want to support that?
>>
>> I don't see any advantage these days for having it disabled.
> 
> I don't know much about Tegra, but I'd still like to turn this question
> around:
> 
> Is there any reason why clks and regulators can't initially be turned on
> in probe,
> and then let runtime PM handle them later if PM is supported?

I personally prefer having the regulator, clock, etc handling for
enabling and disabling for a device in their own handler. Duplicating
the calls to the regulator and clock frameworks in probe and the rpm
handlers seems more prone to mistakes. Hence, that's why I would prefer
the option I suggested above, which IMO is the best of both worlds.

> Shouldn't this work in all cases, and it avoids creating new dependencies?

Yes but when you want to use frameworks such as genpd it becomes more
complex to support !PM and that is why for 64-bit Tegra we have a
dependency today.

Cheers
Jon

-- 
nvpublic
--
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/3] usb: xhci: tegra: Add runtime PM support

[Jon Hunter]

On 09/03/18 08:36, Thierry Reding wrote:
> On Thu, Mar 08, 2018 at 09:31:07PM +, Jon Hunter wrote:
>>
>> On 01/03/18 14:18, Mathias Nyman wrote:
>>> On 14.02.2018 18:34, Jon Hunter wrote:
 Add runtime PM support to the Tegra XHCI driver and move the function
 calls to enable/disable the clocks, regulators and PHY into the runtime
 PM callbacks.

 Signed-off-by: Jon Hunter 
 ---
   drivers/usb/host/xhci-tegra.c | 80
 ++-
   1 file changed, 56 insertions(+), 24 deletions(-)

 diff --git a/drivers/usb/host/xhci-tegra.c
 b/drivers/usb/host/xhci-tegra.c
 index 02b0b24faa58..42aa67858b53 100644
 --- a/drivers/usb/host/xhci-tegra.c
 +++ b/drivers/usb/host/xhci-tegra.c
 @@ -18,6 +18,7 @@
   #include 
   #include 
   #include 
 +#include 
   #include 
   #include 
   #include 
 @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
 platform_device *pdev)
    */
   platform_set_drvdata(pdev, tegra);
   -    err = tegra_xusb_clk_enable(tegra);
 -    if (err) {
 -    dev_err(>dev, "failed to enable clocks: %d\n", err);
 -    goto put_usb2;
 -    }
 -
 -    err = regulator_bulk_enable(tegra->soc->num_supplies,
 tegra->supplies);
 -    if (err) {
 -    dev_err(>dev, "failed to enable regulators: %d\n", err);
 -    goto disable_clk;
 -    }
 +    pm_runtime_enable(>dev);
   -    err = tegra_xusb_phy_enable(tegra);
 +    err = pm_runtime_get_sync(>dev);
   if (err < 0) {
>>>
>>> Does this mean that if runtime PM is disabled then clocks and regulator
>>> will never be enabled
>>> for Tegra xhci?
>>>
>>> How about keeping the clock and regualtor enabling in probe, and instead
>>> add something like:
>>>
>>> pm_runtime_set_active(>dev);
>>> pm_runtime_enable(>dev);
>>> pm_runtime_get_noresume(>dev);
>>
>> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
>> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
>> is disabled.
>>
>> Typically we do something like ...
>>
>> pm_runtime_enable(>dev);
>> if (!pm_runtime_enabled(>dev))
>>  ret = tegra_xusb_runtime_resume(>dev);
>> else
>> ret = pm_runtime_get_sync(>dev);
>>
>> That way we can keep the regulator and clock stuff in the handler. I
>> will update this series.
> 
> Is there any good reason why we don't depend on PM for 32-bit as well?

Not that I am aware of.

> I'm not aware of any differences in drivers that are 32-bit specific for
> Tegra, and I'm not even sure the !PM case gets any testing at all. And
> even if, do we really still want to support that?
> 
> I don't see any advantage these days for having it disabled.

It would be fine IMO.

Cheers
Jon

-- 
nvpublic
--
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: typec: Separate the definitions for data and power roles

[Heikki Krogerus]

USB Type-C specification v1.2 separated the power and data
roles more clearly. Dual-Role-Data term was introduced, and
the meaning of DRP was changed from "Dual-Role-Port" to
"Dual-Role-Power".

In order to allow the port drivers to describe the
capabilities of the ports more clearly according to the
newest specifications, introducing separate definitions for
the data roles.

Signed-off-by: Heikki Krogerus 
---
 drivers/usb/typec/class.c   | 56 ++---
 drivers/usb/typec/fusb302/fusb302.c |  1 +
 drivers/usb/typec/tcpm.c|  9 +++---
 drivers/usb/typec/tps6598x.c| 26 +++--
 drivers/usb/typec/typec_wcove.c |  1 +
 drivers/usb/typec/ucsi/ucsi.c   | 13 +++--
 include/linux/usb/tcpm.h|  1 +
 include/linux/usb/typec.h   | 14 --
 8 files changed, 80 insertions(+), 41 deletions(-)

diff --git a/drivers/usb/typec/class.c b/drivers/usb/typec/class.c
index 2620a694704f..53df10df2f9d 100644
--- a/drivers/usb/typec/class.c
+++ b/drivers/usb/typec/class.c
@@ -282,10 +282,10 @@ typec_altmode_roles_show(struct device *dev, struct 
device_attribute *attr,
ssize_t ret;
 
switch (mode->roles) {
-   case TYPEC_PORT_DFP:
+   case TYPEC_PORT_SRC:
ret = sprintf(buf, "source\n");
break;
-   case TYPEC_PORT_UFP:
+   case TYPEC_PORT_SNK:
ret = sprintf(buf, "sink\n");
break;
case TYPEC_PORT_DRP:
@@ -797,14 +797,14 @@ static const char * const typec_data_roles[] = {
 };
 
 static const char * const typec_port_types[] = {
-   [TYPEC_PORT_DFP] = "source",
-   [TYPEC_PORT_UFP] = "sink",
+   [TYPEC_PORT_SRC] = "source",
+   [TYPEC_PORT_SNK] = "sink",
[TYPEC_PORT_DRP] = "dual",
 };
 
 static const char * const typec_port_types_drp[] = {
-   [TYPEC_PORT_DFP] = "dual [source] sink",
-   [TYPEC_PORT_UFP] = "dual source [sink]",
+   [TYPEC_PORT_SRC] = "dual [source] sink",
+   [TYPEC_PORT_SNK] = "dual source [sink]",
[TYPEC_PORT_DRP] = "[dual] source sink",
 };
 
@@ -875,9 +875,7 @@ static ssize_t data_role_store(struct device *dev,
return ret;
 
mutex_lock(>port_type_lock);
-   if (port->port_type != TYPEC_PORT_DRP) {
-   dev_dbg(dev, "port type fixed at \"%s\"",
-typec_port_types[port->port_type]);
+   if (port->cap->data != TYPEC_PORT_DRD) {
ret = -EOPNOTSUPP;
goto unlock_and_ret;
}
@@ -897,7 +895,7 @@ static ssize_t data_role_show(struct device *dev,
 {
struct typec_port *port = to_typec_port(dev);
 
-   if (port->cap->type == TYPEC_PORT_DRP)
+   if (port->cap->data == TYPEC_PORT_DRD)
return sprintf(buf, "%s\n", port->data_role == TYPEC_HOST ?
   "[host] device" : "host [device]");
 
@@ -1328,7 +1326,6 @@ struct typec_port *typec_register_port(struct device 
*parent,
   const struct typec_capability *cap)
 {
struct typec_port *port;
-   int role;
int ret;
int id;
 
@@ -1354,21 +1351,36 @@ struct typec_port *typec_register_port(struct device 
*parent,
goto err_mux;
}
 
-   if (cap->type == TYPEC_PORT_DFP)
-   role = TYPEC_SOURCE;
-   else if (cap->type == TYPEC_PORT_UFP)
-   role = TYPEC_SINK;
-   else
-   role = cap->prefer_role;
-
-   if (role == TYPEC_SOURCE) {
-   port->data_role = TYPEC_HOST;
+   switch (cap->type) {
+   case TYPEC_PORT_SRC:
port->pwr_role = TYPEC_SOURCE;
port->vconn_role = TYPEC_SOURCE;
-   } else {
-   port->data_role = TYPEC_DEVICE;
+   break;
+   case TYPEC_PORT_SNK:
port->pwr_role = TYPEC_SINK;
port->vconn_role = TYPEC_SINK;
+   break;
+   case TYPEC_PORT_DRP:
+   if (cap->prefer_role != TYPEC_NO_PREFERRED_ROLE)
+   port->pwr_role = cap->prefer_role;
+   else
+   port->pwr_role = TYPEC_SINK;
+   break;
+   }
+
+   switch (cap->data) {
+   case TYPEC_PORT_DFP:
+   port->data_role = TYPEC_HOST;
+   break;
+   case TYPEC_PORT_UFP:
+   port->data_role = TYPEC_DEVICE;
+   break;
+   case TYPEC_PORT_DRD:
+   if (cap->prefer_role == TYPEC_SOURCE)
+   port->data_role = TYPEC_HOST;
+   else
+   port->data_role = TYPEC_DEVICE;
+   break;
}
 
port->id = id;
diff --git a/drivers/usb/typec/fusb302/fusb302.c 
b/drivers/usb/typec/fusb302/fusb302.c
index b267b907bf24..76c9d955fa40 100644
--- a/drivers/usb/typec/fusb302/fusb302.c
+++ b/drivers/usb/typec/fusb302/fusb302.c
@@ 

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> When we set up the DWC3_DEPCMD_ENDTRANSFER command in
>> dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
>> then there will no endpoint command complete interrupts I think.
>>
>> cmd |= DWC3_DEPCMD_CMDIOC;
>
> I remember some part of the databook mandating CMDIOC to be set. We
> could test it out without and see if anything blows up. I would,
> however, require a lengthy comment explaining that we're deviating from
> databook revision x.yya, section foobar because $reasons. :-)
>

 This is what the v3.10 databook says

 "When issuing an End Transfer command, software must set the CmdIOC
 bit (field 8) so that an Endpoint Command Complete event is generated
 after the transfer ends. This is necessary to synchronize the
 conclusion of system bus traffic before the End Transfer command is
 completed."

 with a note

 "If GUCTL2[Rst_actbitlater] is set, Software can poll the completion
 of the End Transfer command by polling the command active bit to be
 cleared to 0."

 fyi.

 Rst_actbitlater - "Enable clearing of the command active bit for the
 ENDXFER command after the command execution is completed.  This bit is
 valid in device mode only."

 So I'd prefer not to clear CMDIOC for all cases.

 Could we some how just tackle the dwc3_gadget_exit case like I did in
 this patch?
>>>
>>> if you can send a version that doesn't iterate over all endpoints twice,
>>> sure. We still need a comment somewhere, and I fear we may get
>>> interrupts later in some cases. How would we deal with that?
>>>
>> 
>> how about explicitly masking that interrupt? Is it possible?
>> 
>
> Other easy option is to use wait_event_interruptible_lock_irq_timeout()
> instead of wait_event_lock_irq() in dwc3_gadget_stop().
>
> Is a 200ms timeout sufficient? And after the first timeout we assume all
> will timeout so no point in waiting 200ms for each endpoint.

We can do that. And I think some 5ms is more than enough :-) I'd be
surprised if it takes anything over some 200us for the EndTransfer
command to complete.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> This is what the v3.10 databook says
>>>
>>> "When issuing an End Transfer command, software must set the CmdIOC
>>> bit (field 8) so that an Endpoint Command Complete event is generated
>>> after the transfer ends. This is necessary to synchronize the
>>> conclusion of system bus traffic before the End Transfer command is
>>> completed."
>>>
>>> with a note
>>>
>>> "If GUCTL2[Rst_actbitlater] is set, Software can poll the completion
>>> of the End Transfer command by polling the command active bit to be
>>> cleared to 0."
>>>
>>> fyi.
>>>
>>> Rst_actbitlater - "Enable clearing of the command active bit for the
>>> ENDXFER command after the command execution is completed.  This bit is
>>> valid in device mode only."
>>>
>>> So I'd prefer not to clear CMDIOC for all cases.
>>>
>>> Could we some how just tackle the dwc3_gadget_exit case like I did in
>>> this patch?
>> 
>> if you can send a version that doesn't iterate over all endpoints twice,
>> sure. We still need a comment somewhere, and I fear we may get
>> interrupts later in some cases. How would we deal with that?
>> 
>
> how about explicitly masking that interrupt? Is it possible?

I think I showed that the bit is reserved on recent dwc3 core releases
(anytyhing 2.40a+, at least).

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 1/6] dt-bindings: add bindings for USB physical connector

[Roger Quadros]

Hi,

On 27/02/18 09:11, Andrzej Hajda wrote:
> These bindings allow to describe most known standard USB connectors
> and it should be possible to extend it if necessary.
> USB connectors, beside USB can be used to route other protocols,
> for example UART, Audio, MHL. In such case every device passing data
> through the connector should have appropriate graph bindings.
> 
> Signed-off-by: Andrzej Hajda 
> ---
> v4:
> - improved 'type' description (Rob),
> - improved description of 2nd example (Rob).
> v3:
> - removed MHL port (samsung connector will have separate bindings),
> - added 2nd example for USB-C,
> - improved formatting.
> v2:
> - moved connector type(A,B,C) to compatible string (Rob),
> - renamed size property to type (Rob),
> - changed type description to be less confusing (Laurent),
> - removed vendor specific compatibles (implied by graph port number),
> - added requirement of connector being a child of IC (Rob),
> - removed max-mode (subtly suggested by Rob, it should be detected anyway
>   by USB Controller in runtime, downside is that device is not able to
>   report its real capabilities, maybe better would be to make it optional(?)),
> - assigned port numbers to data buses (Rob).
> 
> Regards
> Andrzej
> ---
>  .../bindings/connector/usb-connector.txt   | 75 
> ++
>  1 file changed, 75 insertions(+)
>  create mode 100644 
> Documentation/devicetree/bindings/connector/usb-connector.txt
> 
> diff --git a/Documentation/devicetree/bindings/connector/usb-connector.txt 
> b/Documentation/devicetree/bindings/connector/usb-connector.txt
> new file mode 100644
> index ..e1463f14af38
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/connector/usb-connector.txt
> @@ -0,0 +1,75 @@
> +USB Connector
> +=
> +
> +USB connector node represents physical USB connector. It should be
> +a child of USB interface controller.
> +
> +Required properties:
> +- compatible: describes type of the connector, must be one of:
> +"usb-a-connector",
> +"usb-b-connector",
> +"usb-c-connector".

compatible should be just "usb-connector"

Type should be a property

type: type of usb connector "A", "B", "AB", "C"
AB is for dual-role connectors.

micro super-speed and high-speed connectors are different. How do you 
differentiate that?

> +
> +Optional properties:
> +- label: symbolic name for the connector,

Why do you need label? We can't maintain consistency as people will put 
creative names there.
Device/bus driver could generate a valid label.

> +- type: size of the connector, should be specified in case of USB-A, USB-B
> +  non-fullsize connectors: "mini", "micro".

type is misleading. Type is usually A/B/C. It should be size here instead.

size: size of the connector if not standard size. "mini", "micro"

If not specified it is treated as standard sized connector.
e.g. for Type-C there is no mini/micro. so size doesn't have to be specificed

What about Type-C connector?
> +
> +Required nodes:
> +- any data bus to the connector should be modeled using the OF graph bindings

s/modeled/modelled

> +  specified in bindings/graph.txt, unless the bus is between parent node and
> +  the connector. Since single connector can have multpile data buses every 
> bus

s/multpile/multiple

> +  has assigned OF graph port number as follows:
> +0: High Speed (HS), present in all connectors,
> +1: Super Speed (SS), present in SS capable connectors,
> +2: Sideband use (SBU), present in USB-C.
> +
> +Examples
> +
> +
> +1. Micro-USB connector with HS lines routed via controller (MUIC):
> +
> +muic-max77843@66 {
> + ...
> + usb_con: connector {
> + compatible = "usb-b-connector";
> + label = "micro-USB";
> + type = "micro";
> + };
> +};
> +
> +2. USB-C connector attached to CC controller (s2mm005), HS lines routed
> +to companion PMIC (max77865), SS lines to USB3 PHY and SBU to DisplayPort.
> +DisplayPort video lines are routed to the connector via SS mux in USB3 PHY.
> +
> +ccic: s2mm005@33 {
> + ...
> + usb_con: connector {
> + compatible = "usb-c-connector";
> + label = "USB-C";

The label is not consistent with the earlier example.

> +
> + ports {
> + #address-cells = <1>;
> + #size-cells = <0>;
> +
> + port@0 {
> + reg = <0>;
> + usb_con_hs: endpoint {
> + remote-endpoint = <_usbc_hs>;
> + };
> + };
> + port@1 {
> + reg = <1>;
> + usb_con_ss: endpoint {
> + remote-endpoint = <_phy_ss>;
> + };
> + };
> + port@2 {
> + reg = <2>;
> 

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

On 09/03/18 11:26, Roger Quadros wrote:
> On 09/03/18 11:23, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Roger Quadros  writes:
>>
>> 
>>
> When we set up the DWC3_DEPCMD_ENDTRANSFER command in
> dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
> then there will no endpoint command complete interrupts I think.
>
> cmd |= DWC3_DEPCMD_CMDIOC;

 I remember some part of the databook mandating CMDIOC to be set. We
 could test it out without and see if anything blows up. I would,
 however, require a lengthy comment explaining that we're deviating from
 databook revision x.yya, section foobar because $reasons. :-)

>>>
>>> This is what the v3.10 databook says
>>>
>>> "When issuing an End Transfer command, software must set the CmdIOC
>>> bit (field 8) so that an Endpoint Command Complete event is generated
>>> after the transfer ends. This is necessary to synchronize the
>>> conclusion of system bus traffic before the End Transfer command is
>>> completed."
>>>
>>> with a note
>>>
>>> "If GUCTL2[Rst_actbitlater] is set, Software can poll the completion
>>> of the End Transfer command by polling the command active bit to be
>>> cleared to 0."
>>>
>>> fyi.
>>>
>>> Rst_actbitlater - "Enable clearing of the command active bit for the
>>> ENDXFER command after the command execution is completed.  This bit is
>>> valid in device mode only."
>>>
>>> So I'd prefer not to clear CMDIOC for all cases.
>>>
>>> Could we some how just tackle the dwc3_gadget_exit case like I did in
>>> this patch?
>>
>> if you can send a version that doesn't iterate over all endpoints twice,
>> sure. We still need a comment somewhere, and I fear we may get
>> interrupts later in some cases. How would we deal with that?
>>
> 
> how about explicitly masking that interrupt? Is it possible?
> 

Other easy option is to use wait_event_interruptible_lock_irq_timeout()
instead of wait_event_lock_irq() in dwc3_gadget_stop().

Is a 200ms timeout sufficient? And after the first timeout we assume all
will timeout so no point in waiting 200ms for each endpoint.

-- 
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 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: [PATCHv4] phy: mapphone-mdm6600: Add USB PHY driver for MDM6600 on Droid 4

[Pavel Machek]

Hi!

> Let's add support for the GPIO controlled USB PHY on the MDM6600 modem.
> It is used on some Motorola Mapphone series of phones and tablets such
> as Droid 4.
...
> So it seems that MDM6600 is currently not yet idling even with it's
> radio turned off, but that's something that is beyond the control of
> this USB PHY driver. This patch does get us to the point where modem
> data and GPS are usable with libqmi and ModemManager for example.
> Voice calls need more audio driver work.

Thanks for the good work. Looks like I'll need to get droid
4... fortunately it is not available in czech republic so I don't get
excuse to get another toy.

Oh.. no. It is available in Czech republic. Is Motorola Droid 4 XT894
the right one? Hmm, and LTE modem is useless in Europe, while the GSM
one does not work, right?

Best regards,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

On 09/03/18 11:23, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros  writes:
> 
> 
> 
 When we set up the DWC3_DEPCMD_ENDTRANSFER command in
 dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
 then there will no endpoint command complete interrupts I think.

 cmd |= DWC3_DEPCMD_CMDIOC;
>>>
>>> I remember some part of the databook mandating CMDIOC to be set. We
>>> could test it out without and see if anything blows up. I would,
>>> however, require a lengthy comment explaining that we're deviating from
>>> databook revision x.yya, section foobar because $reasons. :-)
>>>
>>
>> This is what the v3.10 databook says
>>
>> "When issuing an End Transfer command, software must set the CmdIOC
>> bit (field 8) so that an Endpoint Command Complete event is generated
>> after the transfer ends. This is necessary to synchronize the
>> conclusion of system bus traffic before the End Transfer command is
>> completed."
>>
>> with a note
>>
>> "If GUCTL2[Rst_actbitlater] is set, Software can poll the completion
>> of the End Transfer command by polling the command active bit to be
>> cleared to 0."
>>
>> fyi.
>>
>> Rst_actbitlater - "Enable clearing of the command active bit for the
>> ENDXFER command after the command execution is completed.  This bit is
>> valid in device mode only."
>>
>> So I'd prefer not to clear CMDIOC for all cases.
>>
>> Could we some how just tackle the dwc3_gadget_exit case like I did in
>> this patch?
> 
> if you can send a version that doesn't iterate over all endpoints twice,
> sure. We still need a comment somewhere, and I fear we may get
> interrupts later in some cases. How would we deal with that?
> 

how about explicitly masking that interrupt? Is it possible?

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

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:



>>> When we set up the DWC3_DEPCMD_ENDTRANSFER command in
>>> dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
>>> then there will no endpoint command complete interrupts I think.
>>>
>>> cmd |= DWC3_DEPCMD_CMDIOC;
>> 
>> I remember some part of the databook mandating CMDIOC to be set. We
>> could test it out without and see if anything blows up. I would,
>> however, require a lengthy comment explaining that we're deviating from
>> databook revision x.yya, section foobar because $reasons. :-)
>> 
>
> This is what the v3.10 databook says
>
> "When issuing an End Transfer command, software must set the CmdIOC
> bit (field 8) so that an Endpoint Command Complete event is generated
> after the transfer ends. This is necessary to synchronize the
> conclusion of system bus traffic before the End Transfer command is
> completed."
>
> with a note
>
> "If GUCTL2[Rst_actbitlater] is set, Software can poll the completion
> of the End Transfer command by polling the command active bit to be
> cleared to 0."
>
> fyi.
>
> Rst_actbitlater - "Enable clearing of the command active bit for the
> ENDXFER command after the command execution is completed.  This bit is
> valid in device mode only."
>
> So I'd prefer not to clear CMDIOC for all cases.
>
> Could we some how just tackle the dwc3_gadget_exit case like I did in
> this patch?

if you can send a version that doesn't iterate over all endpoints twice,
sure. We still need a comment somewhere, and I fear we may get
interrupts later in some cases. How would we deal with that?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v5 0/6] dt-bindings: add bindings for USB physical connector

[Andrzej Hajda]

Hi Chanwoo,

On 08.03.2018 02:52, Chanwoo Choi wrote:
> Hi Andrzej, Archit,
>
> On 2018년 03월 07일 20:13, Andrzej Hajda wrote:
>> Hi Chanwoo, Archit,
>>
>> On 07.03.2018 05:48, Chanwoo Choi wrote:
>>> On 2018년 03월 07일 11:12, Chanwoo Choi wrote:
 Hi Rob and Andrzej,

 On 2018년 03월 06일 21:53, Andrzej Hajda wrote:
> Hi Rob, Chanwoo, Krzysztof,
>
>
> On 27.02.2018 08:11, Andrzej Hajda wrote:
>> Hi,
>>
>> Thanks for reviews of previous iterations.
>>
>> This patchset introduces USB physical connector bindings, together with
>> working example.
>> I have removed RFC prefix - the patchset seems to be heading
>> to a happy end :)
>>
>> v5: fixed extra parenthesis in dts, renamed extcon function.
>> v4: improved binding descriptions, added missing reg in dts.
>> v3: Separate binding for Samsung 11-pin connector, added full-blown USB-C
>> example.
>> v2: I have addressed comments by Rob and Laurent, thanks 
>>
>> Changes in datail are described in the patches.
>>
>> Regards
>> Andrzej
>>
>>
>> Andrzej Hajda (5):
>>   dt-bindings: add bindings for USB physical connector
>>   dt-bindings: add bindings for Samsung micro-USB 11-pin connector
>>   arm64: dts: exynos: add micro-USB connector node to TM2 platforms
>>   arm64: dts: exynos: add OF graph between MHL and USB connector
>>   extcon: add possibility to get extcon device by OF node
>>
>> Maciej Purski (1):
>>   drm/bridge/sii8620: use micro-USB cable detection logic to detect MHL
> It looks like all patches received R-B or acks (I forgot add Krzysztof's
> acks for dts part).
> Now I have a question how to merge them.
> The only functional dependency is between bridge and extcon, and from
> the formal PoV bindings should be merged 1st.
> I can merge it:
> 1. All patches via drm-misc tree.
> 2. All patches except dts via drm-misc, and Krzysztof will merge dts via
> samsung-soc tree.
>
> Is it OK, for all? Better ideas?
 Krzysztof picked the dts patches. I'll make the immutable branch based on 
 v4.16-rc1
 and apply them except for dts patchs. And I'll send the immutable branch 
 to Rob and Andrzej.


>>> I made the immutable branch[1] as following: If you agree, I'll send pull 
>>> request.
>>> [1] 
>>> https://git.kernel.org/pub/scm/linux/kernel/git/chanwoo/extcon.git/log/?h=ib-extcon-drm-dt-v4.17
>>>
>>> Or you can make the immutable branch and send pull request to Rob and me.
>>>
>> It seems you took v5 instead of v6 version of extcon patch.
> My mistake. I picked v6 and made the new immutable branch.
> After Archit confirm it, I'll send pull request.
>


Lets just queue all patches (except dts) via extcon branch (thanks
Daniel for advice), without making immutable branch.
It seems to be a simplest acceptable approach.

You can add my ack to sii8620 bridge patch (as bridge maintainer), to
fulfill process requirements.

Regards
Andrzej

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

Re: [PATCH v4 2/2] usb/gadget: Add driver for Aspeed SoC virtual hub

[Felipe Balbi]

Benjamin Herrenschmidt  writes:

> The Aspeed BMC SoCs support a "virtual hub" function. It provides some
> HW support for a top-level USB2 hub behind which sit 5 gadget "ports".
>
> This driver adds support for the full functionality, emulating the
> hub standard requests and exposing 5 UDC gadget drivers corresponding
> to the ports.
>
> The hub itself has HW provided dedicated EP0 and EP1 (the latter for
> hub interrupts). It also has dedicated EP0s for each function. For
> other endpoints, there's a pool of 15 "generic" endpoints that are
> shared among the ports.
>
> The driver relies on my previous patch adding a "dispose" EP op to
> handle EP allocation between ports. EPs are allocated from the shared
> pool in the UDC "match_ep" callback and assigned to the UDC instance
> (added to the gadget ep_list).
>
> When the composite driver gets unbound, the new hook will allow the UDC
> to clean things up and return those EPs to the shared pool.
>
> Signed-off-by: Benjamin Herrenschmidt 
> ---
> v4. - Fix missing unlock ast_vhub_udc_wakeup() error path
> - Make "irq" signed to deal with error from platform_get_irq
> - Fix Makefile for module builds
> - Fix a missing endian conversion
>
> v3. - Rebased
> - Add clk stuff
>
> v2. - Cosmetic fixes
> - Properly "allocate" device addresses instead of using a never
>   reset counter
> - Move .dtsi updates to a different patch
>
> foo
>
> goo
> ---
>  drivers/usb/gadget/udc/Kconfig  |   2 +
>  drivers/usb/gadget/udc/Makefile |   1 +
>  drivers/usb/gadget/udc/aspeed-vhub/Kconfig  |   6 +
>  drivers/usb/gadget/udc/aspeed-vhub/Makefile |   3 +
>  drivers/usb/gadget/udc/aspeed-vhub/core.c   | 429 ++
>  drivers/usb/gadget/udc/aspeed-vhub/dev.c| 580 +++
>  drivers/usb/gadget/udc/aspeed-vhub/ep0.c| 474 
>  drivers/usb/gadget/udc/aspeed-vhub/epn.c| 840 
> 
>  drivers/usb/gadget/udc/aspeed-vhub/hub.c| 822 +++
>  drivers/usb/gadget/udc/aspeed-vhub/vhub.h   | 496 
>  10 files changed, 3653 insertions(+)
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/Kconfig
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/Makefile
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/core.c
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/dev.c
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/ep0.c
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/epn.c
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/hub.c
>  create mode 100644 drivers/usb/gadget/udc/aspeed-vhub/vhub.h
>
> diff --git a/drivers/usb/gadget/udc/Kconfig b/drivers/usb/gadget/udc/Kconfig
> index 1e9567091d86..14cf31a2703a 100644
> --- a/drivers/usb/gadget/udc/Kconfig
> +++ b/drivers/usb/gadget/udc/Kconfig
> @@ -439,6 +439,8 @@ config USB_GADGET_XILINX
> dynamically linked module called "udc-xilinx" and force all
> gadget drivers to also be dynamically linked.
>  
> +source "drivers/usb/gadget/udc/aspeed-vhub/Kconfig"
> +
>  #
>  # LAST -- dummy/emulated controller
>  #
> diff --git a/drivers/usb/gadget/udc/Makefile b/drivers/usb/gadget/udc/Makefile
> index ce865b129fd6..897f648f3cf1 100644
> --- a/drivers/usb/gadget/udc/Makefile
> +++ b/drivers/usb/gadget/udc/Makefile
> @@ -39,4 +39,5 @@ obj-$(CONFIG_USB_MV_U3D)+= mv_u3d_core.o
>  obj-$(CONFIG_USB_GR_UDC) += gr_udc.o
>  obj-$(CONFIG_USB_GADGET_XILINX)  += udc-xilinx.o
>  obj-$(CONFIG_USB_SNP_UDC_PLAT) += snps_udc_plat.o
> +obj-$(CONFIG_USB_ASPEED_VHUB)+= aspeed-vhub/
>  obj-$(CONFIG_USB_BDC_UDC)+= bdc/
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/Kconfig 
> b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
> new file mode 100644
> index ..cb022c885425
> --- /dev/null
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/Kconfig
> @@ -0,0 +1,6 @@
> +config USB_ASPEED_VHUB
> + tristate "Aspeed vHub UDC driver"
> + depends on ARCH_ASPEED || COMPILE_TEST
> + help
> +   USB peripheral controller for the Aspeed AST2500 family
> +   SoCs supporting the "vHub" functionality and USB2.0
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/Makefile 
> b/drivers/usb/gadget/udc/aspeed-vhub/Makefile
> new file mode 100644
> index ..4256266c0a8a
> --- /dev/null
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/Makefile
> @@ -0,0 +1,3 @@
> +obj-$(CONFIG_USB_ASPEED_VHUB)+= aspeed-vhub.o
> +aspeed-vhub-y:= core.o ep0.o epn.o dev.o hub.o
> +
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c 
> b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> new file mode 100644
> index ..31ed2b6e241b
> --- /dev/null
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -0,0 +1,429 @@

missing SPDX license identifier (all files)

> +static bool force_usb1 = false;
> +static bool no_dma_desc = false;
> +
> +module_param_named(force_usb1, 

Re: [PATCH] usb: dwc3: Prevent indefinite sleep in _dwc3_set_mode during suspend/resume

[Roger Quadros]

On 05/03/18 13:27, Felipe Balbi wrote:
> 
> Hi,
> 
> Baolin Wang  writes:
>>  void dwc3_gadget_exit(struct dwc3 *dwc)
>>  {
>> +  int epnum;
>> +  unsigned long flags;
>> +
>> +  spin_lock_irqsave(>lock, flags);
>> +  for (epnum = 2; epnum < DWC3_ENDPOINTS_NUM; epnum++) {
>> +  struct dwc3_ep  *dep = dwc->eps[epnum];
>> +
>> +  if (!dep)
>> +  continue;
>> +
>> +  dep->flags &= ~DWC3_EP_END_TRANSFER_PENDING;
>> +  }
>> +  spin_unlock_irqrestore(>lock, flags);
>> +
>>usb_del_gadget_udc(>gadget);
>>dwc3_gadget_free_endpoints(dwc);
>
> free endpoints is a better place for this. It's already going to free
> the memory anyway. Might as well clear all flags to 0 there.
>

 But it won't solve the deadlock issue. Since 
 dwc3_gadget_free_endpoints()
 is called after usb_del_gadget_udc() and the deadlock happens when

 usb_del_gadget_udc()->udc_stop()->dwc3_gadget_stop()->wait_event_lock_irq()

 and DWC3_EP_END_TRANSFER_PENDING flag is set.
>>>
>>> indeed. Iterating twice over the entire endpoint list seems
>>> wasteful. Perhaps we just shouldn't wait when removing the UDC since
>>> that's essentially what this patch will do, right? If you clear the flag
>>> before calling ->udc_stop(), this means the loop in dwc3_gadget_stop()
>>> will do nothing. Might as well remove it.
>>>
>>
>> This means that we will never wait for DWC3_EP_END_TRANSFER_PENDING to 
>> clear
>> in dwc3_gadget_stop() like we used to. This is perfectly fine, right?
>>
>> It makes sense to me as dwc3_gadget_stop() calls __dwc3_gadget_stop() 
>> which
>> masks all interrupts and nobody will ever clear that flag if it was set.
>
> I don't think so. It can not mask the endpoint events, please check
> the events which will be masked in DEVTEN register. The reason why we
> should wait for DWC3_EP_END_TRANSFER_PENDING to clear is that,
> sometimes the DWC3_DEPEVT_EPCMDCMPLT event will be triggered later
> than 100us, but now we may have freed the gadget irq which will cause
> crash.

 We could mask command complete events as soon as ->udc_stop() is called,
 right? Hmm, actually, __dwc3_gadget_stop() already clears DEVTEN
 completely.
>>>
>>> But which bit in DEVTEN says Endpoint events are disabled?
>>
>> When we set up the DWC3_DEPCMD_ENDTRANSFER command in
>> dwc3_stop_active_transfer(), we can do not set DWC3_DEPCMD_CMDIOC,
>> then there will no endpoint command complete interrupts I think.
>>
>> cmd |= DWC3_DEPCMD_CMDIOC;
> 
> I remember some part of the databook mandating CMDIOC to be set. We
> could test it out without and see if anything blows up. I would,
> however, require a lengthy comment explaining that we're deviating from
> databook revision x.yya, section foobar because $reasons. :-)
> 

This is what the v3.10 databook says

"When issuing an End Transfer command, software must set the CmdIOC bit (field 
8) so that an Endpoint
Command Complete event is generated after the transfer ends. This is necessary 
to synchronize the
conclusion of system bus traffic before the End Transfer command is completed."

with a note
"If GUCTL2[Rst_actbitlater] is set, Software can poll the completion of the End 
Transfer
command by polling the command active bit to be cleared to 0."

fyi.
Rst_actbitlater - "Enable clearing of the command active bit for the ENDXFER
command after the command execution is completed.
This bit is valid in device mode only."

So I'd prefer not to clear CMDIOC for all cases.

Could we some how just tackle the dwc3_gadget_exit case like I did in this 
patch?

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

Re: [PATCH] usb: dwc2: Print error if unable to set DMA coherent mask

[Felipe Balbi]

Stefan Wahren  writes:

>> Stefan Wahren  hat am 12. Februar 2018 um 21:20 
>> geschrieben:
>> 
>> 
>> We better print an error in case probing of dwc2 fails on
>> setting the DMA coherent mask.
>> 
>> Signed-off-by: Stefan Wahren 
>> ---
>>  drivers/usb/dwc2/platform.c | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
>> index 4703478..4ddbdbd 100644
>> --- a/drivers/usb/dwc2/platform.c
>> +++ b/drivers/usb/dwc2/platform.c
>> @@ -382,8 +382,10 @@ static int dwc2_driver_probe(struct platform_device 
>> *dev)
>>  if (!dev->dev.dma_mask)
>>  dev->dev.dma_mask = >dev.coherent_dma_mask;
>>  retval = dma_set_coherent_mask(>dev, DMA_BIT_MASK(32));
>> -if (retval)
>> +if (retval) {
>> +dev_err(>dev, "can't set coherent DMA mask: %d\n", retval);
>>  return retval;
>> +}
>>  
>>  res = platform_get_resource(dev, IORESOURCE_MEM, 0);
>>  hsotg->regs = devm_ioremap_resource(>dev, res);
>> -- 
>> 2.7.4
>>
>
> ping ...

https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=testing/next=57b1d49d4b9c6cde9fbe95c1a211b6bbea61c6fc

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/3] usb: xhci: tegra: Add runtime PM support

[Mathias Nyman]

On 09.03.2018 10:36, Thierry Reding wrote:

On Thu, Mar 08, 2018 at 09:31:07PM +, Jon Hunter wrote:


On 01/03/18 14:18, Mathias Nyman wrote:

On 14.02.2018 18:34, Jon Hunter wrote:

Add runtime PM support to the Tegra XHCI driver and move the function
calls to enable/disable the clocks, regulators and PHY into the runtime
PM callbacks.

Signed-off-by: Jon Hunter 
---
   drivers/usb/host/xhci-tegra.c | 80
++-
   1 file changed, 56 insertions(+), 24 deletions(-)

diff --git a/drivers/usb/host/xhci-tegra.c
b/drivers/usb/host/xhci-tegra.c
index 02b0b24faa58..42aa67858b53 100644
--- a/drivers/usb/host/xhci-tegra.c
+++ b/drivers/usb/host/xhci-tegra.c
@@ -18,6 +18,7 @@
   #include 
   #include 
   #include 
+#include 
   #include 
   #include 
   #include 
@@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
platform_device *pdev)
    */
   platform_set_drvdata(pdev, tegra);
   -    err = tegra_xusb_clk_enable(tegra);
-    if (err) {
-    dev_err(>dev, "failed to enable clocks: %d\n", err);
-    goto put_usb2;
-    }
-
-    err = regulator_bulk_enable(tegra->soc->num_supplies,
tegra->supplies);
-    if (err) {
-    dev_err(>dev, "failed to enable regulators: %d\n", err);
-    goto disable_clk;
-    }
+    pm_runtime_enable(>dev);
   -    err = tegra_xusb_phy_enable(tegra);
+    err = pm_runtime_get_sync(>dev);
   if (err < 0) {


Does this mean that if runtime PM is disabled then clocks and regulator
will never be enabled
for Tegra xhci?

How about keeping the clock and regualtor enabling in probe, and instead
add something like:

pm_runtime_set_active(>dev);
pm_runtime_enable(>dev);
pm_runtime_get_noresume(>dev);


For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
AFAIK there is not and so yes we should handle the case when PM_RUNTIME
is disabled.

Typically we do something like ...

 pm_runtime_enable(>dev);
 if (!pm_runtime_enabled(>dev))
ret = tegra_xusb_runtime_resume(>dev);
 else
 ret = pm_runtime_get_sync(>dev);

That way we can keep the regulator and clock stuff in the handler. I
will update this series.


Is there any good reason why we don't depend on PM for 32-bit as well?
I'm not aware of any differences in drivers that are 32-bit specific for
Tegra, and I'm not even sure the !PM case gets any testing at all. And
even if, do we really still want to support that?

I don't see any advantage these days for having it disabled.


I don't know much about Tegra, but I'd still like to turn this question around:

Is there any reason why clks and regulators can't initially be turned on in 
probe,
and then let runtime PM handle them later if PM is supported?

Shouldn't this work in all cases, and it avoids creating new dependencies?

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

Re: [PATCH v1 10/14] usb: dwc2: Add dwc2_enter_hibernation(), dwc2_exit_hibernation()

[Felipe Balbi]

Grigor Tovmasyan  writes:

> From: Vardan Mikayelyan 
>
> These are wrapper functions which are calling device or host
> enter/exit hibernation functions.
>
> Signed-off-by: Vardan Mikayelyan 
> Signed-off-by: John Youn 
> Signed-off-by: Grigor Tovmasyan 

didn't apply to testing/next. Care to rebase on testing/next?

best

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/4] usb: dwc3: add dwc3 glue layer for UniPhier SoCs

[Felipe Balbi]

Hi,

Masahiro Yamada  writes:
 > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
 > +{
 > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
 > +  usleep_range(1000, 2000);
 > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
 > +}
 > +
 > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
 > +{
 > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
 > +}

 drivers/reset ?
>>>
>>> The reset driver manages "sysctrl" IO map area in our SoC.
>>>
>>> RESET_CTL register belongs to "dwc3-glue" IO map area,
>>> and the kernel can't access this area until enabling usb clocks and
>>> deasserting usb resets in "sysctrl".
>>>
>>> I think that "dwc3-glue" register control should be separated from
>>> "sysctrl".
>>
>> Just split your address space and treat your glue as a device with
>> several children:
>>
>> glue@65b0 {
>> compatible = "foo"
>>
>> phy@bar {
>> ...
>> };
>>
>> sysctrl@baz {
>> ...
>> };
>>
>> dwc3@foo {
>> compatible = "snps, dwc3";
>> ...
>> };
>> };
>>
>> Then you know that you can let dwc3/core.c handle the PHY for you. If we
>> need to teach dwc3/core.c about regulators, we can do that. But we don't
>> need SoC-specific hacks ;-)
>>
>> --
>> balbi
>
>
> Slightly related question.
>
>
> Why don't we put clocks and resets to dwc3/core.c?

We can do that for the simpler platforms, no worries.

> dwc3-of-simple.c only handles clocks and resets.
> This is generic enough to be added to dwc3/core.c, I think.
>
>
> I checked the two instances of dwc3-of-simple.
>
> "qcom,dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/qcom/msm8996.dtsi#L780
>
> "rockchip,rk3399-dwc3"
> https://github.com/torvalds/linux/blob/v4.16-rc3/arch/arm64/boot/dts/rockchip/rk3399.dtsi#L395
>
>
> They just contain clocks, resets, and "snps,dwc3" sub-node.
>
>
> If we do that,
>
> usb@760 {
> compatible = "qcom,dwc3";
> clocks = ...;
>
> dwc3@760 {
> compatible = "snps,dwc3";
> reg = ...;
> interrupts = ...;
> phys = ...;
> }
> };
>
>
> will be turned into
>
>
> dwc3@760 {
> compatible = "qcom,dwc3", "snps,dwc3";
> reg = ...;
> clocks = ...;
> interrupts = ...;
> phys = ...;
> };
>
>
> This looks simpler.

yup. This will only work for the simpler platforms, though. TI platforms
and PCI-based platforms, this really won't work :-)

> Also, dwc3/core.c and dwc3-of-simple.c
> duplicate runtime PM.

they "kinda" duplicate :-) Some platforms have platform-specific clocks
which are not generic enough to be stuffed into dwc3/core.c. Some of
those clocks may need special handling of some sorts. It's best to keep
the option for peculiar clock tree setups available.

If your platform is simple enough that you can get away without a glue
layer, sure thing. More power for you :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Hi,
>
> On 08/03/18 18:49, Brian Norris wrote:
>> Hi,
>> 
>> On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote:
>>> William Wu  writes:
 The dwc3_core_init() gets the PHYs and initializes the PHYs with
 the usb_phy_init() and phy_init() functions before initializing
 core, and power on the PHYs after core initialization is done.

 However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C
 USB3 PHY), it needs to do some special operation while power on
 the Type-C PHY before initializing DWC3 core. It's because that
 the RK3399 Type-C PHY requires to hold the DWC3 controller in
 reset state to keep the PIPE power state in P2 while configuring
 the Type-C PHY, otherwise, it may cause waiting for the PIPE ready
 timeout. In this case, if we power on the PHYs after the DWC3 core
 initialization is done, the core will be reset to uninitialized
 state after power on the PHYs.

 Fix this by powering on the PHYs before initializing core. And
 because the GUID register may also be reset in this case, so we
 need to configure the GUID register after powering on the PHYs.

 Signed-off-by: William Wu 
>>>
>>> does this cause any regressions for your boards?
>> 
>> I'm not Roger, but I believe it was determined we don't need this for
>> the Rockchip systems for which William was originally sending this. At
>> least not right now. I believe our PHY init problems were mostly
>> resolved in other ways.
>> 
>> (Although I hear USB is currently pretty broken around suspend/resume
>> for us on -next. Likely unrelated.)
>> 
>> I guess we never clearly replied stating the above. I hope this isn't
>> merged anywhere? Or I guess it's no problem to me at the moment, but it
>> might be needless churn.
>> 
>
> I did some quick tests on TI platforms and didn't see any issues with this 
> patch.
> Since this patch isn't really fixing your problem and we didn't have any
> problems to start with I'd suggest to avoid this churn for now.

fair enough, I won't apply it :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: power on PHYs before initializing core

[Roger Quadros]

Hi,

On 08/03/18 18:49, Brian Norris wrote:
> Hi,
> 
> On Thu, Mar 08, 2018 at 12:43:40PM +0200, Felipe Balbi wrote:
>> William Wu  writes:
>>> The dwc3_core_init() gets the PHYs and initializes the PHYs with
>>> the usb_phy_init() and phy_init() functions before initializing
>>> core, and power on the PHYs after core initialization is done.
>>>
>>> However, some platforms (e.g. Rockchip RK3399 DWC3 with Type-C
>>> USB3 PHY), it needs to do some special operation while power on
>>> the Type-C PHY before initializing DWC3 core. It's because that
>>> the RK3399 Type-C PHY requires to hold the DWC3 controller in
>>> reset state to keep the PIPE power state in P2 while configuring
>>> the Type-C PHY, otherwise, it may cause waiting for the PIPE ready
>>> timeout. In this case, if we power on the PHYs after the DWC3 core
>>> initialization is done, the core will be reset to uninitialized
>>> state after power on the PHYs.
>>>
>>> Fix this by powering on the PHYs before initializing core. And
>>> because the GUID register may also be reset in this case, so we
>>> need to configure the GUID register after powering on the PHYs.
>>>
>>> Signed-off-by: William Wu 
>>
>> does this cause any regressions for your boards?
> 
> I'm not Roger, but I believe it was determined we don't need this for
> the Rockchip systems for which William was originally sending this. At
> least not right now. I believe our PHY init problems were mostly
> resolved in other ways.
> 
> (Although I hear USB is currently pretty broken around suspend/resume
> for us on -next. Likely unrelated.)
> 
> I guess we never clearly replied stating the above. I hope this isn't
> merged anywhere? Or I guess it's no problem to me at the moment, but it
> might be needless churn.
> 

I did some quick tests on TI platforms and didn't see any issues with this 
patch.
Since this patch isn't really fixing your problem and we didn't have any
problems to start with I'd suggest to avoid this churn for now.

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

RE: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request matching pdos"

[Jun Li]

Hi,
> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Heikki Krogerus
> Sent: 2018年3月6日 20:33
> To: Hans de Goede 
> Cc: Greg Kroah-Hartman ; Guenter Roeck
> ; linux-usb@vger.kernel.org; Badhri Jagan Sridharan
> 
> Subject: Re: [PATCH 4.16 REGRESSION fix] Revert "typec: tcpm: Only request
> matching pdos"
> 
> On Tue, Mar 06, 2018 at 10:50:05AM +0100, Hans de Goede wrote:
> > Commit 57e6f0d7b804 ("typec: tcpm: Only request matching pdos") is
> > causing a regression, before this commit e.g. the GPD win and GPD
> > pocket devices were charging at 9V 3A with a PD charger, now they are
> > instead slowly discharging  at 5V 0.4A, as this commit causes the
> > ports max_snk_mv/ma/mw settings to be completely ignored.
> >

So max_snk_mv/ma/mw settings are back, I am dealing with PD properties
definition, so want to ask for long term, should we keep them, or use the
reverting patch way(only compare sink PDOs Vs source PDOs) but fix the
problem with it.

Thanks
Jun Li

> > Arguably the way to fix this would be to add a PDO_VAR() describing
> > the voltage range to the snk_caps of boards which can handle any
> > voltage in their range, but the "typec: tcpm: Only request matching
> > pdos" commit looks at the type of PDO advertised by the source/charger
> > and if that is fixed (as it typically is) only compairs against
> > PDO_FIXED entries in the snk_caps so supporting a range of voltage
> > would require adding a PDO_FIXED entry for *every possible* voltage to
> snk_caps.
> >
> > AFAICT there is no reason why a fixed source_cap cannot be matched
> > against a variable snk_cap, so at a minimum the commit should be
> > rewritten to support that.
> >
> > For now lets revert the "typec: tcpm: Only request matching pdos"
> > commit, fixing the regression.
> >
> > Cc: Badhri Jagan Sridharan 
> > Signed-off-by: Hans de Goede 
> 
> You are correct. The patch should be rewritten.
> 
> Acked-by: Heikki Krogerus 
> 
> 
> Thanks,
> 
> --
> 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
> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> ernel.org%2Fmajordomo-info.html=02%7C01%7Cjun.li%40nxp.com%7C
> 6cd4e7a122244982de5e08d5835e618c%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C636559363813086942=nZw4ta%2Frro44iG%2FfH
> bhUrhOKZBdE16JvgSYUsb71qzg%3D=0

Re: [PATCH 2/3] usb: xhci: tegra: Add runtime PM support

[Thierry Reding]

On Thu, Mar 08, 2018 at 09:31:07PM +, Jon Hunter wrote:
> 
> On 01/03/18 14:18, Mathias Nyman wrote:
> > On 14.02.2018 18:34, Jon Hunter wrote:
> >> Add runtime PM support to the Tegra XHCI driver and move the function
> >> calls to enable/disable the clocks, regulators and PHY into the runtime
> >> PM callbacks.
> >>
> >> Signed-off-by: Jon Hunter 
> >> ---
> >>   drivers/usb/host/xhci-tegra.c | 80
> >> ++-
> >>   1 file changed, 56 insertions(+), 24 deletions(-)
> >>
> >> diff --git a/drivers/usb/host/xhci-tegra.c
> >> b/drivers/usb/host/xhci-tegra.c
> >> index 02b0b24faa58..42aa67858b53 100644
> >> --- a/drivers/usb/host/xhci-tegra.c
> >> +++ b/drivers/usb/host/xhci-tegra.c
> >> @@ -18,6 +18,7 @@
> >>   #include 
> >>   #include 
> >>   #include 
> >> +#include 
> >>   #include 
> >>   #include 
> >>   #include 
> >> @@ -1067,22 +1068,12 @@ static int tegra_xusb_probe(struct
> >> platform_device *pdev)
> >>    */
> >>   platform_set_drvdata(pdev, tegra);
> >>   -    err = tegra_xusb_clk_enable(tegra);
> >> -    if (err) {
> >> -    dev_err(>dev, "failed to enable clocks: %d\n", err);
> >> -    goto put_usb2;
> >> -    }
> >> -
> >> -    err = regulator_bulk_enable(tegra->soc->num_supplies,
> >> tegra->supplies);
> >> -    if (err) {
> >> -    dev_err(>dev, "failed to enable regulators: %d\n", err);
> >> -    goto disable_clk;
> >> -    }
> >> +    pm_runtime_enable(>dev);
> >>   -    err = tegra_xusb_phy_enable(tegra);
> >> +    err = pm_runtime_get_sync(>dev);
> >>   if (err < 0) {
> > 
> > Does this mean that if runtime PM is disabled then clocks and regulator
> > will never be enabled
> > for Tegra xhci?
> > 
> > How about keeping the clock and regualtor enabling in probe, and instead
> > add something like:
> > 
> > pm_runtime_set_active(>dev);
> > pm_runtime_enable(>dev);
> > pm_runtime_get_noresume(>dev);
> 
> For 64-bit Tegra there is a dependency on CONFIG_PM, but for 32-bit
> AFAIK there is not and so yes we should handle the case when PM_RUNTIME
> is disabled.
> 
> Typically we do something like ...
> 
> pm_runtime_enable(>dev);
> if (!pm_runtime_enabled(>dev))
>   ret = tegra_xusb_runtime_resume(>dev);
> else
> ret = pm_runtime_get_sync(>dev);
> 
> That way we can keep the regulator and clock stuff in the handler. I
> will update this series.

Is there any good reason why we don't depend on PM for 32-bit as well?
I'm not aware of any differences in drivers that are 32-bit specific for
Tegra, and I'm not even sure the !PM case gets any testing at all. And
even if, do we really still want to support that?

I don't see any advantage these days for having it disabled.

Thierry


signature.asc
Description: PGP signature

Without US_FL_NEEDS_CAP16 read_capacity_10 is forced despite of the device capability and HDD size

[Menion]

Hi all
I am discussing about an issue I get with my Orico USBtoSATA 5 bays enclosure
The issue is that every 5 minutes the BTRFS perform a revalidate
action that will fill the dmesg with this log:

[   98.917660] sd 0:0:0:1: [sdb] Very big device. Trying to use READ
CAPACITY(16).
[   99.057592] sd 0:0:0:0: [sda] Very big device. Trying to use READ
CAPACITY(16).
[   99.304848] sd 0:0:0:2: [sdc] Very big device. Trying to use READ
CAPACITY(16).
[   99.465374] sd 0:0:0:3: [sdd] Very big device. Trying to use READ
CAPACITY(16).
[   99.669241] sd 0:0:0:4: [sde] Very big device. Trying to use READ
CAPACITY(16).

After some investigation I think I have found out the reason
The sd_read_capacity in scsi layer check if read_capacity_10 or
read_capacity_16 should be used
It does it based on what this function return:

static int sd_try_rc16_first(struct scsi_device *sdp)
{
if (sdp->host->max_cmd_len < 16)
return 0;
if (sdp->try_rc_10_first)
return 0;
if (sdp->scsi_level > SCSI_SPC_2)
return 1;
if (scsi_device_protection(sdp))
return 1;
return 0;
}

the problem is that in the usb to scsi glue layer, the try_rc_10_first
is picked like this:

/*
 * Many devices do not respond properly to READ_CAPACITY_16.
 * Tell the SCSI layer to try READ_CAPACITY_10 first.
 * However some USB 3.0 drive enclosures return capacity
 * modulo 2TB. Those must use READ_CAPACITY_16
 */
if (!(us->fflags & US_FL_NEEDS_CAP16))
sdev->try_rc_10_first = 1;

Are we sure this is the correct option? I mean to me the default
should be to go for rc_10 if there is a specific request in the
unusual devices, rather then if we dont' need cap16 we go for it,
like:

if ((us->fflags & US_FL_NEEDS_CAP10))
sdev->try_rc_10_first = 1;

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

Re: [PATCH v7 4/4] soc/tegra: pmc: Use the new reset APIs to manage reset controllers

[Thierry Reding]

On Wed, Jul 19, 2017 at 05:59:08PM +0200, Philipp Zabel wrote:
> From: Vivek Gautam 
> 
> Make use of of_reset_control_array_get_exclusive() to manage
> an array of reset controllers available with the device.
> 
> Cc: Jon Hunter 
> Cc: Thierry Reding 
> Signed-off-by: Vivek Gautam 
> [p.za...@pengutronix.de: switch to hidden reset control array]
> Signed-off-by: Philipp Zabel 
> ---
> No changes since v6.
> ---
>  drivers/soc/tegra/pmc.c | 82 
> -
>  1 file changed, 20 insertions(+), 62 deletions(-)

I've applied this with the static inline change that Jon suggested.

Thanks,
Thierry


signature.asc
Description: PGP signature

Re: [PATCH 11/11] usb: dwc2: Enable LPM

[Felipe Balbi]

Hi,

Grigor Tovmasyan  writes:
> From: John Youn 
>
> Set 'lpm_capable' flag in the gadget structure so
> indicating that LPM is supported.
>
> Signed-off-by: Sevak Arakelyan 
> Signed-off-by: Grigor Tovmasyan 

Missing John's Signed-off-by. What gives? I'll drop this from my queue
for now.

-- 
balbi


signature.asc
Description: PGP signature