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

[Benjamin Herrenschmidt]

On Fri, 2018-03-16 at 11:44 +1100, Benjamin Herrenschmidt wrote:
> 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.

 .../...

Found a bug in handling of zero-length requests in IN endpoints,
will send a v6.

Cheers,
Ben.

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

[Benjamin Herrenschmidt]

On Sat, 2018-03-17 at 09:50 -0400, Alan Stern wrote:
> On Sat, 17 Mar 2018, Benjamin Herrenschmidt wrote:
> 
> > On Fri, 2018-03-16 at 16:56 -0400, Alan Stern wrote:
> > > On Fri, 16 Mar 2018, Benjamin Herrenschmidt wrote:
> > > 
> > > > On Thu, 2018-03-15 at 08:03 +1100, Benjamin Herrenschmidt wrote:
> > > > > > > Do you have more comments for the rest of the driver or that's it 
> > > > > > > ?
> > > > > > 
> > > > > > so far, that's it.
> > > > > 
> > > > > Ok. I'll re-send.
> > > > 
> > > > So I'll resend in a minute, doing a few more tests, however, I've
> > > > noticed something which I wont' have time to track down til at
> > > > best next week, I wonder if it's normal/expected.
> > > > 
> > > > If I just create a mass storage function set to be "removable" and
> > > > "cdrom" with no file attached,  and enable it, I get an endless stream
> > > > of resets. It looks like the host constantly does USB resets.
> > > 
> > > That's not why I get.  There's an endless stream of messages, but it
> > > doesn't include any resets.  Just command failures and endpoint halts.  
> > > For example:
> > 
> >  .../...
> > 
> > In my case, I was getting resets on the host:
> 
 .../...

> Output from usbmon could help.

So that ended up being a bug in my UDC driver. For IN requests I had a
problem with 0-length requests when enabling the multi-descriptor mode
in the EP which seems to affect some MODE_SENSE responses among others.

I fixed that and now see only the silent errors you mentioned.

Felipe, I'll send a respin of the driver with a fix tomorrow.

 .../...

> Maybe...  Even after cdrom support was added to the gadget, the scope 
> was limited.  It was intended to emulate _only_ a CD drive, not a DVD 
> drive.
> 
> Also, if I'm not mistaken, the commands that the emulation doesn't 
> handle are all optional.

Right. One low hanging fruit seems to be 
GET_EVENT_STATUS_NOTIFICATION (0x4a). I'll look into it in my copious
spare time ;-)

There are a few more, which as long as we only support "read only"
CD/DVD should be fairly easy to deal with.

Cheers,
Ben.

> Alan Stern
> 
> > The Aspeed chip is a BMC chip, ie server management processor, and is
> > connected via the USB gadget to the actual server (the host). One of
> > the usage scenario here is to use USB gadget to present distro ISOs as
> > USB CD/DVDs to the host for remote provisioning (sourced over the
> > network via something like nbd).
> > 
> > Note that due to the limitation of having to use a file or a block
> > device, we might end up doing a userspace CDROM emulation instead that
> > can source ISOs via things like HTTPS instead, but initially the above
> > is what we are toying with.
> > 
> > Cheers,
> > Ben.
--
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] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[Daniel Drake]

acpi_dev_pm_get_state() is used to determine the range of allowable
device power states when going into S3 suspend. This is implemented
by executing the _S3D and _S3W ACPI methods.

Linux follows the ACPI spec behaviour in that when _S3D is implemented
and _S3W is not, Linux will not go into a power state deeper than the one
returned by _S3D for a wakeup-enabled device.

However, this same logic is being applied to the case when neither
_S3D nor _S3W are present, and the result is that this function
decides that the device must stay in D0 (fully on) state.

This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and
_S3W are not present, so the USB controller is left in the D0 running
state during S3, and hence it is unable to generate a PME# wake event.

The ACPI spec is unclear on which power states are permissable for
wakeup-enabled devices when both _S3D and _S3W are missing.
However, USB wakeups work fine on these platforms under Windows, where
device manager shows that they are using D3 device state for the USB
controller in S3.

I assume that the "max = min" clamping done by the code here is
specifically written for the _S3D but no _S3W case. By making the
code true to those conditions, avoiding them on these platforms,
the controller will be put into D3 state and USB wakeups start working.

Additionally I feel that this change makes the code more directly
mirror the wording of the ACPI spec and it's associated lack of clarity.

Thanks to Mathias Nyman for pointing us in the right direction.

Signed-off-by: Daniel Drake 
Link: 
http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=bv1xhxv_gy3bzioo_n...@mail.gmail.com

https://phabricator.endlessm.com/T21410
---

Notes:
This should be considered for Linux 4.17 to give it a decent amount
of testing time before release.

v2: fix unused variable warning

 drivers/acpi/device_pm.c | 11 ++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index c4d0a1c912f0..3d96e4da2d98 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct 
acpi_device *adev,
unsigned long long ret;
int d_min, d_max;
bool wakeup = false;
+   bool has_sxd = false;
acpi_status status;
 
/*
@@ -581,6 +582,10 @@ static int acpi_dev_pm_get_state(struct device *dev, 
struct acpi_device *adev,
else
return -ENODATA;
}
+
+   if (status == AE_OK)
+   has_sxd = true;
+
d_min = ret;
wakeup = device_may_wakeup(dev) && adev->wakeup.flags.valid
&& adev->wakeup.sleep_state >= target_state;
@@ -599,7 +604,11 @@ static int acpi_dev_pm_get_state(struct device *dev, 
struct acpi_device *adev,
method[3] = 'W';
status = acpi_evaluate_integer(handle, method, NULL, );
if (status == AE_NOT_FOUND) {
-   if (target_state > ACPI_STATE_S0)
+   /* No _SxW. In this case, the ACPI spec says that we
+* must not go into any power state deeper than the
+* value returned from _SxD.
+*/
+   if (has_sxd && target_state > ACPI_STATE_S0)
d_max = d_min;
} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
/* Fall back to D3cold if ret is not a valid state. */
-- 
2.14.1

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

RE: [PATCH v3 02/12] dt-bindings: usb: add documentation for typec port controller(TCPCI)

[Jun Li]

> -Original Message-
> From: Jun Li
> Sent: 2018年3月20日 10:29
> To: Mats Karrman ; robh...@kernel.org;
> mark.rutl...@arm.com; gre...@linuxfoundation.org;
> heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> shufan_...@richtek.com; o_leve...@orange.fr; linux-usb@vger.kernel.org;
> dl-linux-imx 
> Subject: RE: [PATCH v3 02/12] dt-bindings: usb: add documentation for typec
> port controller(TCPCI)
> 
> Hi
> > -Original Message-
> > From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> > Sent: 2018年3月15日 23:54
> > To: Jun Li ; robh...@kernel.org; mark.rutl...@arm.com;
> > gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> > Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> > shufan_...@richtek.com; o_leve...@orange.fr;
> > linux-usb@vger.kernel.org; dl-linux-imx 
> > Subject: Re: [PATCH v3 02/12] dt-bindings: usb: add documentation for
> > typec port controller(TCPCI)
> >
> > Hi,
> >
> > On 2018-03-13 10:34, Li Jun wrote:
> >
> > > TCPCI stands for typec port controller interface, its implementation
> > > has full typec port control with power delivery support, it's a
> > > standard i2c slave with GPIO input as irq interface, detail see spec
> > > "Universal Serial Bus Type-C Port Controller Interface Specification
> > > Revision 1.0, Version 1.1"
> > >
> > > Signed-off-by: Li Jun 
> > > ---
> > > change for v3:
> > > - change compatible string from "usb,tcpci" to be
> > >"usb-tcpci,chip-specific-string", update example of it.
> > >
> > >   .../devicetree/bindings/usb/typec-tcpci.txt| 36
> > ++
> > >   1 file changed, 36 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > > b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > > new file mode 100644
> > > index 000..fe7d2ff
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > > @@ -0,0 +1,36 @@
> > > +TCPCI(Typec port cotroller interface) binding
> > > +-
> > > +
> > > +Required properties:
> > > +- compatible:   should be "usb-tcpci,chip-specific-string".
> > > +- reg:  the i2c slave address of typec port controller 
> > > device.
> > > +- interrupt-parent: the phandle to the interrupt controller which 
> > > provides
> > > +the interrupt.
> > > +- interrupts:   interrupt specification for tcpci alert.
> > > +
> > > +Required sub-node:
> > > +- connector: The "usb-c-connector" attached to the tcpci chip, the
> > > +bindings
> > > +  of connector node are specified in
> > > +  Documentation/devicetree/bindings/connector/usb-connector.txt
> > > +
> > > +Example:
> > > +
> > > +ptn5110@50 {
> > > + compatible = "usb-tcpci,ptn5110";
> > > + reg = <0x50>;
> > > + interrupt-parent = <>;
> > > + interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> > > +
> > > + usb_con: connector {
> > > + compatible = "usb-c-connector";
> > > + label = "USB-C";
> > > + port-type = "dual";
> > > + try-power-role = "sink"
> > > + source-pdos = <0x380190c8>;
> > > + sink-pdos = <0x380190c8 0x3802d0c8>;
> > > + max-sink-microvolt = <9000>;
> > > + max-sink-microamp = <2000>;
> > > + max-sink-microwatt-hours = <18000>;
> > > + op-sink-microwatt-hours = <9000>;
> >
> > Values should change with suffix, e.g. 9000mV = 900uV
> >
> 
> Yes, thanks for pointing this out, I will update in v4.

Just realized we are going to remove those properties, see[1]:

[1] https://www.spinics.net/lists/linux-usb/msg166986.html

anyway thank you for the comments.

Jun
> 
> Jun
> 
> > // Mats
> >
> > > + };
> > > +};
> > >

RE: [PATCH v3 05/12] usb: typec: add API to get sink and source config

[Jun Li]

> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年3月16日 0:06
> To: Jun Li ; robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> shufan_...@richtek.com; o_leve...@orange.fr; linux-usb@vger.kernel.org;
> dl-linux-imx 
> Subject: Re: [PATCH v3 05/12] usb: typec: add API to get sink and source
> config
> 
> Hi,
> 
> On 2018-03-13 10:34, Li Jun wrote:
> 
> > This patch add 2 APIs to get sink and source power config from
> > firmware description in case the port supports PD.
> >
> > Signed-off-by: Li Jun 
> > ---
> >   drivers/usb/typec/tcpm.c | 47
> +++
> >   include/linux/usb/tcpm.h |  8 +---
> >   2 files changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 7500dc0..0bd34c9 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -13,6 +13,7 @@
> >   #include 
> >   #include 
> >   #include 
> > +#include 
> >   #include 
> >   #include 
> >   #include 
> > @@ -3595,6 +3596,52 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const
> u32 *src_vdo,
> > return nr_vdo;
> >   }
> >
> > +int tcpm_get_src_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg) {
> > +   int ret;
> > +
> > +   if (!fwnode)
> > +   return -EINVAL;
> > +
> > +   ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> > +NULL, 0);
> > +   if (ret <= 0)
> > +   return -EINVAL;
> > +
> > +   tcfg->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
> > +   return fwnode_property_read_u32_array(fwnode, "source-pdos",
> > + tcfg->src_pdo, tcfg->nr_src_pdo); 
> > }
> > +EXPORT_SYMBOL_GPL(tcpm_get_src_config);
> > +
> > +int tcpm_get_snk_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg) {
> > +   int ret;
> > +
> > +   if (!fwnode)
> > +   return -EINVAL;
> > +
> > +   if ((fwnode_property_read_u32(fwnode, "max-sink-microvolt",
> > + >max_snk_mv) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "max-sink-microamp",
> > + >max_snk_ma) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "max-sink-microwatt-hours",
> > + >max_snk_mw) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "op-sink-microwatt-hours",
> > + >operating_snk_mw) < 0))
> 
> If DT is changed to use micro instead of milli, these values needs to be 
> divided
> by 1000.

Yes, I will update in v4, thanks.

Jun
> 
> // Mats
> 
> > +   return -EINVAL;
> > +
> > +   ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> > +NULL, 0);
> > +   if (ret <= 0)
> > +   return -EINVAL;
> > +
> > +   tcfg->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
> > +   return fwnode_property_read_u32_array(fwnode, "sink-pdos",
> > + tcfg->snk_pdo, tcfg->nr_snk_pdo); 
> > }
> > +EXPORT_SYMBOL_GPL(tcpm_get_snk_config);
> > +
> >   int tcpm_update_source_capabilities(struct tcpm_port *port, const u32
> *pdo,
> > unsigned int nr_pdo)
> >   {
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> > e2e2db3..5d361f6 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -76,10 +76,10 @@ enum tcpm_transmit_type {
> >* @alt_modes:List of supported alternate modes
> >*/
> >   struct tcpc_config {
> > -   const u32 *src_pdo;
> > +   u32 *src_pdo;
> > unsigned int nr_src_pdo;
> >
> > -   const u32 *snk_pdo;
> > +   u32 *snk_pdo;
> > unsigned int nr_snk_pdo;
> >
> > const u32 *snk_vdo;
> > @@ -143,7 +143,7 @@ enum tcpc_mux_mode {
> >* @mux:  Pointer to multiplexer data
> >*/
> >   struct tcpc_dev {
> > -   const struct tcpc_config *config;
> > +   struct tcpc_config *config;
> > struct fwnode_handle *fwnode;
> >
> > int (*init)(struct tcpc_dev *dev);
> > @@ -189,5 +189,7 @@ void tcpm_pd_transmit_complete(struct tcpm_port
> *port,
> >enum tcpm_transmit_status status);
> >   void tcpm_pd_hard_reset(struct tcpm_port *port);
> >   void tcpm_tcpc_reset(struct tcpm_port *port);
> > +int tcpm_get_src_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg); int tcpm_get_snk_config(struct fwnode_handle
> > +*fwnode, struct tcpc_config *tcfg);
> >
> >   #endif /* __LINUX_USB_TCPM_H */
> >

RE: [PATCH v3 02/12] dt-bindings: usb: add documentation for typec port controller(TCPCI)

[Jun Li]

Hi
> -Original Message-
> From: Mats Karrman [mailto:mats.dev.l...@gmail.com]
> Sent: 2018年3月15日 23:54
> To: Jun Li ; robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> shufan_...@richtek.com; o_leve...@orange.fr; linux-usb@vger.kernel.org;
> dl-linux-imx 
> Subject: Re: [PATCH v3 02/12] dt-bindings: usb: add documentation for typec
> port controller(TCPCI)
> 
> Hi,
> 
> On 2018-03-13 10:34, Li Jun wrote:
> 
> > TCPCI stands for typec port controller interface, its implementation
> > has full typec port control with power delivery support, it's a
> > standard i2c slave with GPIO input as irq interface, detail see spec
> > "Universal Serial Bus Type-C Port Controller Interface Specification
> > Revision 1.0, Version 1.1"
> >
> > Signed-off-by: Li Jun 
> > ---
> > change for v3:
> > - change compatible string from "usb,tcpci" to be
> >"usb-tcpci,chip-specific-string", update example of it.
> >
> >   .../devicetree/bindings/usb/typec-tcpci.txt| 36
> ++
> >   1 file changed, 36 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > new file mode 100644
> > index 000..fe7d2ff
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/usb/typec-tcpci.txt
> > @@ -0,0 +1,36 @@
> > +TCPCI(Typec port cotroller interface) binding
> > +-
> > +
> > +Required properties:
> > +- compatible:   should be "usb-tcpci,chip-specific-string".
> > +- reg:  the i2c slave address of typec port controller device.
> > +- interrupt-parent: the phandle to the interrupt controller which provides
> > +the interrupt.
> > +- interrupts:   interrupt specification for tcpci alert.
> > +
> > +Required sub-node:
> > +- connector: The "usb-c-connector" attached to the tcpci chip, the
> > +bindings
> > +  of connector node are specified in
> > +  Documentation/devicetree/bindings/connector/usb-connector.txt
> > +
> > +Example:
> > +
> > +ptn5110@50 {
> > +   compatible = "usb-tcpci,ptn5110";
> > +   reg = <0x50>;
> > +   interrupt-parent = <>;
> > +   interrupts = <3 IRQ_TYPE_LEVEL_LOW>;
> > +
> > +   usb_con: connector {
> > +   compatible = "usb-c-connector";
> > +   label = "USB-C";
> > +   port-type = "dual";
> > +   try-power-role = "sink"
> > +   source-pdos = <0x380190c8>;
> > +   sink-pdos = <0x380190c8 0x3802d0c8>;
> > +   max-sink-microvolt = <9000>;
> > +   max-sink-microamp = <2000>;
> > +   max-sink-microwatt-hours = <18000>;
> > +   op-sink-microwatt-hours = <9000>;
> 
> Values should change with suffix, e.g. 9000mV = 900uV
> 

Yes, thanks for pointing this out, I will update in v4.

Jun

> // Mats
> 
> > +   };
> > +};
> >
N�r��yb�X��ǧv�^�)޺{.n�+{��^n�r���z���h�&���G���h�(�階�ݢj"���m��z�ޖ���f���h���~�m�

[PATCH v2 1/1] usb: musb: gadget: misplaced out of bounds check

[Heinrich Schuchardt]

musb->endpoints[] has array size MUSB_C_NUM_EPS.
We must check array bounds before accessing the array and not afterwards.

Signed-off-by: Heinrich Schuchardt 
---
v2
Only the 4 low bits of epnum are relevant for indexing. 
---
 drivers/usb/musb/musb_gadget_ep0.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
b/drivers/usb/musb/musb_gadget_ep0.c
index 18da4873e52e..96b0fa12f729 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -89,15 +89,20 @@ static int service_tx_status_request(
}
 
is_in = epnum & USB_DIR_IN;
+   epnum &= 0x0f;
+   if (epnum >= MUSB_C_NUM_EPS) {
+   handled = -EINVAL;
+   break;
+   }
+
if (is_in) {
-   epnum &= 0x0f;
ep = >endpoints[epnum].ep_in;
} else {
ep = >endpoints[epnum].ep_out;
}
regs = musb->endpoints[epnum].regs;
 
-   if (epnum >= MUSB_C_NUM_EPS || !ep->desc) {
+   if (!ep->desc) {
handled = -EINVAL;
break;
}
-- 
2.16.2

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

RE: [PATCH v3 05/12] usb: typec: add API to get sink and source config

[Jun Li]

Hi
> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年3月15日 20:06
> To: Jun Li 
> Cc: robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; a.ha...@samsung.com; li...@roeck-us.net;
> yue...@google.com; shufan_...@richtek.com; o_leve...@orange.fr;
> linux-usb@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v3 05/12] usb: typec: add API to get sink and source
> config
> 
> Hi,
> 
> A small nitpick. The subject lines seem to be a little bit inconsistent in 
> this
> series. This patch for example does not mention tcpm at all in its subject or
> even commit message, even though it only modifies tcpm.c.
> 
> Please change the subject lines of all the patches in this series mainly 
> dealing
> with tcpm.c for example to:
> 
> usb: typec: tcpm: ...
> 

Thanks, I will update in v4.

Jun

RE: [PATCH v3 06/12] staging: typec: tcpci: support port config passed via dt

[Jun Li]

Hi
> -Original Message-
> From: Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com]
> Sent: 2018年3月15日 19:43
> To: Jun Li 
> Cc: robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; a.ha...@samsung.com; li...@roeck-us.net;
> yue...@google.com; shufan_...@richtek.com; o_leve...@orange.fr;
> linux-usb@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v3 06/12] staging: typec: tcpci: support port config 
> passed
> via dt
> 
> On Tue, Mar 13, 2018 at 05:34:32PM +0800, Li Jun wrote:
> > User can define the typec port properties in tcpci node to setup the
> > port config.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  drivers/staging/typec/tcpci.c | 63
> > +++
> >  1 file changed, 63 insertions(+)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index 24ad44f..ba4627f 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -466,6 +466,9 @@ static const struct regmap_config
> > tcpci_regmap_config = {
> >
> >  static int tcpci_parse_config(struct tcpci *tcpci)  {
> > +   struct tcpc_config *tcfg;
> > +   int ret = -EINVAL;
> > +
> > tcpci->controls_vbus = true; /* XXX */
> >
> > tcpci->tcpc.fwnode = device_get_named_child_node(tcpci->dev,
> > @@ -475,6 +478,66 @@ static int tcpci_parse_config(struct tcpci *tcpci)
> > return -EINVAL;
> > }
> >
> > +   tcpci->tcpc.config = devm_kzalloc(tcpci->dev, sizeof(*tcfg),
> > + GFP_KERNEL);
> > +   if (!tcpci->tcpc.config)
> > +   return -ENOMEM;
> > +
> > +   tcfg = tcpci->tcpc.config;
> > +
> > +   /* USB data support is optional */
> > +   ret = typec_get_data_type(tcpci->tcpc.fwnode);
> > +   if (ret < 0)
> > +   dev_dbg(tcpci->dev, "USB data is not supported.\n");
> > +   else
> > +   tcfg->data = ret;
> > +
> > +   /* Get port power type */
> > +   ret = typec_get_power_type(tcpci->tcpc.fwnode);
> > +   if (ret < 0) {
> > +   dev_err(tcpci->dev, "failed to get correct port type.\n");
> > +   return ret;
> > +   }
> > +   tcfg->type = ret;
> > +
> > +   if (tcfg->type == TYPEC_PORT_SNK)
> > +   goto sink;
> > +
> > +   tcfg->src_pdo = devm_kcalloc(tcpci->dev, PDO_MAX_OBJECTS,
> > +sizeof(*tcfg->src_pdo), GFP_KERNEL);
> > +   if (!tcfg->src_pdo)
> > +   return -ENOMEM;
> > +
> > +   /* Get source capability */
> > +   ret = tcpm_get_src_config(tcpci->tcpc.fwnode, tcfg);
> > +   if (ret < 0) {
> > +   dev_err(tcpci->dev, "failed to get source pdo %d\n", ret);
> > +   return -EINVAL;
> > +   }
> > +
> > +   if (tcfg->type == TYPEC_PORT_SRC)
> > +   return 0;
> > +
> > +   /* Get the preferred power role for DRP */
> > +   ret = typec_get_preferred_role(tcpci->tcpc.fwnode);
> > +   if (ret < 0) {
> > +   dev_err(tcpci->dev, "failed to get correct preferred role.\n");
> > +   return ret;
> > +   }
> > +   tcfg->default_role = ret;
> > +sink:
> > +   tcfg->snk_pdo = devm_kcalloc(tcpci->dev, PDO_MAX_OBJECTS,
> > +sizeof(*tcfg->snk_pdo), GFP_KERNEL);
> > +   if (!tcfg->snk_pdo)
> > +   return -ENOMEM;
> > +
> > +   /* Get power sink config */
> > +   ret = tcpm_get_snk_config(tcpci->tcpc.fwnode, tcfg);
> > +   if (ret < 0) {
> > +   dev_err(tcpci->dev, "failed to get sink configs %d\n", ret);
> > +   return -EINVAL;
> > +   }
> > +
> > return 0;
> >  }
> 
> I think everything here can be done in tcpm.c.
> 
> Since you are checking some device properties in tcpm.c in any case (in
> tcpm_get_snk_config() and tcpm_get_src_config()), you might as well check all
> these there as well.

Agreed, I will move those properties read to tcpm, then it can be called/checked
by tcpm_register_port().

> 
> 
> Br,
> 
> --
> heikki

RE: [PATCH v1 2/3] usb: dwc2: Change ISOC DDMA flow

[Zengtao (B)]

Hi Minas:

A few minor comments:

>-Original Message-
>From: linux-usb-ow...@vger.kernel.org
>[mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Minas Harutyunyan
>Sent: Saturday, March 17, 2018 5:10 PM
>To: John Youn ; Felipe Balbi ;
>Greg Kroah-Hartman ; linux-usb@vger.kernel.org
>Cc: Minas Harutyunyan 
>Subject: [PATCH v1 2/3] usb: dwc2: Change ISOC DDMA flow
>
>Changed existing two descriptor-chain flow to one chain.
>
>In two-chain implementation BNA interrupt used for switching between two
>chains. BNA interrupt asserted because of returning to beginning of the chain
>based on L-bit of last descriptor.
>
>Because of that we lose packets. This issue resolved by using one desc-chain.
>
>Removed all staff related to two desc-chain flow from DDMA ISOC related
>functions.
>
>Removed request length checking from dwc2_gadget_fill_isoc_desc() function.
>Request length checking added to dwc2_hsotg_ep_queue() function. If request
>length greater than descriptor limits then request not added to queue.
>Additional checking done for High Bandwidth ISOC OUT's which not supported by
>driver. In
>dwc2_gadget_fill_isoc_desc() function also checked desc-chain status (full or 
>not)
>to avoid of reusing not yet processed descriptors.
>
>In dwc2_gadget_start_isoc_ddma() function creation of desc-chain always
>started from descriptor 0. Before filling descriptors, they were initialized by
>HOST BUSY status.
>
>In dwc2_gadget_complete_isoc_request_ddma() added checking for desc-chain
>rollover. Also added checking completion status.
>Request completed successfully if DEV_DMA_STS is DEV_DMA_STS_SUCC,
>otherwise complete with -ETIMEDOUT.
>
>Actually removed dwc2_gadget_start_next_isoc_ddma() function because now
>driver use only one desc-chain and instead that function added
>dwc2_gadget_handle_isoc_bna() function for handling BNA interrupts.
>
>Handling BNA interrupt done by flushing TxFIFOs for OUT EPs, completing
>request with -EIO and resetting desc-chain number and target frame to initial
>values for restarting transfers.
>
>On handling NAK request completed with -ENODATA. Incremented target frame
>to allow fill desc chain and start transfers.
>In DDMA mode avoided of frame number incrementing, because tracking of
>frame number performed in dwc2_gadget_fill_isoc_desc() function.
>
>When core assert XferCompl along with BNA, we should ignore XferCompl in
>dwc2_hsotg_epint() function.
>
>On BNA interrupt replaced dwc2_gadget_start_next_isoc_ddma() by above
>mentioned BNA handler.
>
>In dwc2_hsotg_ep_enable() function added sanity check of bInterval for ISOC IN
>in DDMA mode, because HW not supported EP's with bInterval more than 12.
>
>Signed-off-by: Minas Harutyunyan 
>---
> drivers/usb/dwc2/core.h   |   2 -
> drivers/usb/dwc2/gadget.c | 235 ++
> 2 files changed, 113 insertions(+), 124 deletions(-)
>
>diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h index
>d83be5651f87..093d078adaf4 100644
>--- a/drivers/usb/dwc2/core.h
>+++ b/drivers/usb/dwc2/core.h
>@@ -178,7 +178,6 @@ struct dwc2_hsotg_req;
>  * @desc_list_dma: The DMA address of descriptor chain currently in use.
>  * @desc_list: Pointer to descriptor DMA chain head currently in use.
>  * @desc_count: Count of entries within the DMA descriptor chain of EP.
>- * @isoc_chain_num: Number of ISOC chain currently in use - either 0 or 1.
>  * @next_desc: index of next free descriptor in the ISOC chain under SW
>control.
>  * @total_data: The total number of data bytes done.
>  * @fifo_size: The size of the FIFO (for periodic IN endpoints) @@ -231,7
>+230,6 @@ struct dwc2_hsotg_ep {
>   struct dwc2_dma_desc*desc_list;
>   u8  desc_count;
>
>-  unsigned char   isoc_chain_num;
>   unsigned intnext_desc;
>
>   charname[10];
>diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
>c231321656f9..1b9c84cb58fb 100644
>--- a/drivers/usb/dwc2/gadget.c
>+++ b/drivers/usb/dwc2/gadget.c
>@@ -793,9 +793,7 @@ static void
>dwc2_gadget_config_nonisoc_xfer_ddma(struct dwc2_hsotg_ep *hs_ep,
>  * @dma_buff: usb requests dma buffer.
>  * @len: usb request transfer length.
>  *
>- * Finds out index of first free entry either in the bottom or up half of
>- * descriptor chain depend on which is under SW control and not processed
>- * by HW. Then fills that descriptor with the data of the arrived usb request,
>+ * Fills next free descriptor with the data of the arrived usb request,
>  * frame info, sets Last and IOC bits increments next_desc. If filled
>  * descriptor is not the first one, removes L bit from the previous descriptor
>  * status.
>@@ -810,34 +808,17 @@ static int dwc2_gadget_fill_isoc_desc(struct
>dwc2_hsotg_ep *hs_ep,
>   u32 mask = 0;
>
>   maxsize = 

RE: [PATCH v3 05/12] usb: typec: add API to get sink and source config

[Jun Li]

> -Original Message-
> From: linux-usb-ow...@vger.kernel.org
> [mailto:linux-usb-ow...@vger.kernel.org] On Behalf Of Heikki Krogerus
> Sent: 2018年3月15日 19:21
> To: Jun Li 
> Cc: robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; a.ha...@samsung.com; li...@roeck-us.net;
> yue...@google.com; shufan_...@richtek.com; o_leve...@orange.fr;
> linux-usb@vger.kernel.org; dl-linux-imx 
> Subject: Re: [PATCH v3 05/12] usb: typec: add API to get sink and source
> config
> 
> On Tue, Mar 13, 2018 at 05:34:31PM +0800, Li Jun wrote:
> > This patch add 2 APIs to get sink and source power config from
> > firmware description in case the port supports PD.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  drivers/usb/typec/tcpm.c | 47
> > +++
> >  include/linux/usb/tcpm.h |  8 +---
> >  2 files changed, 52 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c index
> > 7500dc0..0bd34c9 100644
> > --- a/drivers/usb/typec/tcpm.c
> > +++ b/drivers/usb/typec/tcpm.c
> > @@ -13,6 +13,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -3595,6 +3596,52 @@ static int tcpm_copy_vdos(u32 *dest_vdo, const
> u32 *src_vdo,
> > return nr_vdo;
> >  }
> >
> > +int tcpm_get_src_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg) {
> > +   int ret;
> > +
> > +   if (!fwnode)
> > +   return -EINVAL;
> > +
> > +   ret = fwnode_property_read_u32_array(fwnode, "source-pdos",
> > +NULL, 0);
> > +   if (ret <= 0)
> > +   return -EINVAL;
> > +
> > +   tcfg->nr_src_pdo = min(ret, PDO_MAX_OBJECTS);
> > +   return fwnode_property_read_u32_array(fwnode, "source-pdos",
> > + tcfg->src_pdo, tcfg->nr_src_pdo); 
> > }
> > +EXPORT_SYMBOL_GPL(tcpm_get_src_config);
> > +
> > +int tcpm_get_snk_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg) {
> > +   int ret;
> > +
> > +   if (!fwnode)
> > +   return -EINVAL;
> > +
> > +   if ((fwnode_property_read_u32(fwnode, "max-sink-microvolt",
> > + >max_snk_mv) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "max-sink-microamp",
> > + >max_snk_ma) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "max-sink-microwatt-hours",
> > + >max_snk_mw) < 0) ||
> > +   (fwnode_property_read_u32(fwnode, "op-sink-microwatt-hours",
> > + >operating_snk_mw) < 0))
> > +   return -EINVAL;
> > +
> > +   ret = fwnode_property_read_u32_array(fwnode, "sink-pdos",
> > +NULL, 0);
> > +   if (ret <= 0)
> > +   return -EINVAL;
> > +
> > +   tcfg->nr_snk_pdo = min(ret, PDO_MAX_OBJECTS);
> > +   return fwnode_property_read_u32_array(fwnode, "sink-pdos",
> > + tcfg->snk_pdo, tcfg->nr_snk_pdo); 
> > }
> > +EXPORT_SYMBOL_GPL(tcpm_get_snk_config);
> 
> tcpm_register_port() can check these. No need to involve the tcpc drivers.

OK, I will read those properties directly to port->typec_caps without involve
tcpc->config.

> 
> >  int tcpm_update_source_capabilities(struct tcpm_port *port, const u32
> *pdo,
> > unsigned int nr_pdo)
> >  {
> > diff --git a/include/linux/usb/tcpm.h b/include/linux/usb/tcpm.h index
> > e2e2db3..5d361f6 100644
> > --- a/include/linux/usb/tcpm.h
> > +++ b/include/linux/usb/tcpm.h
> > @@ -76,10 +76,10 @@ enum tcpm_transmit_type {
> >   * @alt_modes: List of supported alternate modes
> >   */
> >  struct tcpc_config {
> > -   const u32 *src_pdo;
> > +   u32 *src_pdo;
> > unsigned int nr_src_pdo;
> >
> > -   const u32 *snk_pdo;
> > +   u32 *snk_pdo;
> > unsigned int nr_snk_pdo;
> >
> > const u32 *snk_vdo;
> > @@ -143,7 +143,7 @@ enum tcpc_mux_mode {
> >   * @mux:   Pointer to multiplexer data
> >   */
> >  struct tcpc_dev {
> > -   const struct tcpc_config *config;
> > +   struct tcpc_config *config;
> 
> If you check the properties in tcpm, the above members can continue to be
> declared constant for now.

Agreed.

> 
> > struct fwnode_handle *fwnode;
> >
> > int (*init)(struct tcpc_dev *dev);
> > @@ -189,5 +189,7 @@ void tcpm_pd_transmit_complete(struct tcpm_port
> *port,
> >enum tcpm_transmit_status status);  void
> > tcpm_pd_hard_reset(struct tcpm_port *port);  void
> > tcpm_tcpc_reset(struct tcpm_port *port);
> > +int tcpm_get_src_config(struct fwnode_handle *fwnode, struct
> > +tcpc_config *tcfg); int tcpm_get_snk_config(struct fwnode_handle
> > +*fwnode, struct tcpc_config *tcfg);
> 
> 
> Cheers,
> 
> --
> heikki
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in the 
> body
> of a 

RE: [PATCH v3 10/12] staging: typec: tcpci: keep the not connecting cc line open

[Jun Li]

Hi
> -Original Message-
> From: shufan_lee(李��帆) [mailto:shufan_...@richtek.com]
> Sent: 2018年3月15日 15:45
> To: Jun Li ; robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> o_leve...@orange.fr; linux-usb@vger.kernel.org; dl-linux-imx
> 
> Subject: RE: [PATCH v3 10/12] staging: typec: tcpci: keep the not connecting
> cc line open
> 
> Hi Jun,
> 
> -Original Message-
> From: Jun Li [mailto:jun...@nxp.com]
> Sent: Thursday, March 15, 2018 3:00 PM
> To: shufan_lee(李��帆); robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> o_leve...@orange.fr; linux-usb@vger.kernel.org; dl-linux-imx
> Subject: RE: [PATCH v3 10/12] staging: typec: tcpci: keep the not connecting
> cc line open
> 
> 
> 
> > -Original Message-
> > From: shufan_lee(李��帆) [mailto:shufan_...@richtek.com]
> > Sent: 2018年3月14日 16:59
> > To: Jun Li ; robh...@kernel.org; mark.rutl...@arm.com;
> > gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> > Cc: a.ha...@samsung.com; li...@roeck-us.net; yue...@google.com;
> > o_leve...@orange.fr; linux-usb@vger.kernel.org; dl-linux-imx
> > 
> > Subject: RE: [PATCH v3 10/12] staging: typec: tcpci: keep the not
> > connecting cc line open
> >
> > Hi Jun,
> >
> > -Original Message-
> > From: Li Jun [mailto:jun...@nxp.com]
> > Sent: Tuesday, March 13, 2018 5:35 PM
> > To: robh...@kernel.org; mark.rutl...@arm.com;
> > gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com
> > Cc: a.ha...@samsung.com; jun...@nxp.com; li...@roeck-us.net;
> > yue...@google.com; shufan_lee(李��帆); o_leve...@orange.fr;
> > linux-usb@vger.kernel.org; linux-...@nxp.com
> > Subject: [PATCH v3 10/12] staging: typec: tcpci: keep the not
> > connecting cc line open
> >
> > While set polarity, we should keep the not connecting cc line to be
> > open when attached.
> >
> > Signed-off-by: Li Jun 
> > ---
> >  drivers/staging/typec/tcpci.c | 13 -
> >  1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index
> > 9a230c6..6fdb179 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -185,6 +185,7 @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,
> >enum typec_cc_polarity polarity)  {  struct tcpci *tcpci =
> > tcpc_to_tcpci(tcpc);
> > +unsigned int reg;
> >  int ret;
> >
> >  ret = regmap_write(tcpci->regmap, TCPC_TCPC_CTRL, @@ -193,7 +194,17
> > @@ static int tcpci_set_polarity(struct tcpc_dev *tcpc,  if (ret < 0)
> > return ret;
> >
> > -return 0;
> > +/* Set the not connected cc line open */ ret =
> > +regmap_read(tcpci->regmap, TCPC_ROLE_CTRL, ); if (ret < 0) return
> > +ret; if (polarity == TYPEC_POLARITY_CC2) ret =
> > +TCPC_ROLE_CTRL_CC1_SHIFT; else ret = TCPC_ROLE_CTRL_CC2_SHIFT; reg
> |=
> > +TCPC_ROLE_CTRL_CC_OPEN << ret; reg &= ~TCPC_ROLE_CTRL_DRP; return
> > +regmap_write(tcpci->regmap, TCPC_ROLE_CTRL, reg);
> >  }
> >
> >  static int tcpci_set_vconn(struct tcpc_dev *tcpc, bool enable)
> > --
> > 2.7.4
> >
> >   I applied all of your patches and tested with RT1711H.
> >   I met a case as following:
> >   1. The state machine starts toggling with RC.CCx = Rp/Rp
> >   2. Connect to adapter with an A2C cable
> >   3. open/open is received after setting RC.DRP to 0. Because RC.CCx
> > is Rp/Rp while setting RC.DRP to 0 and the connected A2C cable is a cable
> with Rp.
> >
> 
> What's the RC.CCx value before setting RC.DRP in your case?
> It is Rp/Rp, i.e. reg0x1A = 0x4A
> 
> I suppose the RC.CCx should be updated accordingly by TCPC (and its firmware),
> RT1711H still keep the starting value after detected a Rp?
> Yes, according to TCPCI's spec, upon connection, the TCPC shall resolve to
> either an Rp or Rd and report the CC1/CC2 State in the CC_STATUS register.
> It seems like we only need to present correct role and report to CC_STATUS
> register but not necessary to report the role to RC.CCx.
> So, RT1711H's firmware only present Rd and report it to reg0x1D(CC_STATUS)
> but not change the value of reg0x1A (RC.CCx).
> 

Know your situation, seems we need set cc after attach only for drp toggling 
case.
I will update this patch, thanks.

Jun

> >   According to TCPCI's specification, Figure 4-20. DRP Initialization
> > and Connection Detection, we need to set RC.CCx before setting RC.DRP to 0.
> >
> >   ConnectionDetermine CC & VCONN
> > - Set RC.CC1 & RC.CC2 per decision
> > - Set RC.DRP=0
> > - Set TCPC_CONTROl.PlugOrientation
> > - Set PC.AutoDischargeDisconnect=1 & PC.EnableVconn
> >
> >   If I understand correctly, we'll need to do the following step
> > before setting DRP to 0.
> 
> The read out value has the 

RE: [PATCH v3 07/12] staging: typec: tcpci: register port before request irq

[Jun Li]

Hi
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 2018年3月15日 12:51
> To: Jun Li 
> Cc: robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> a.ha...@samsung.com; yue...@google.com; shufan_...@richtek.com;
> o_leve...@orange.fr; linux-usb@vger.kernel.org; dl-linux-imx
> 
> Subject: Re: [PATCH v3 07/12] staging: typec: tcpci: register port before
> request irq
> 
> On Tue, Mar 13, 2018 at 05:34:33PM +0800, Li Jun wrote:
> > With that we can clear any pending events and the port is registered
> > so driver can be ready to handle typec events once we request irq.
> >
> > Signed-off-by: Peter Chen 
> > Signed-off-by: Li Jun 
> > ---
> >  drivers/staging/typec/tcpci.c | 15 ---
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index ba4627f..f5a3bf5 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -600,25 +600,26 @@ static int tcpci_probe(struct i2c_client *client,
> > if (IS_ERR(chip->data.regmap))
> > return PTR_ERR(chip->data.regmap);
> >
> > +   i2c_set_clientdata(client, chip);
> > +
> > /* Disable chip interrupts before requesting irq */
> > err = regmap_raw_write(chip->data.regmap, TCPC_ALERT_MASK, ,
> >sizeof(u16));
> > if (err < 0)
> > return err;
> >
> > +   chip->tcpci = tcpci_register_port(>dev, >data);
> > +   if (PTR_ERR_OR_ZERO(chip->tcpci))
> > +   return PTR_ERR(chip->tcpci);
> > +
> 
> I am concerned that the regitration above might already trigger commands,
> and that we might miss interrupts if that is the case.
> 
> Would it make sense to check for chip->tcpci in the interrupt handler instead 
> ?

Tcpci_irq()
{
if(!chip->tcpci)
return IRQ_HANDLED;

/* Read alert and clear events */
...
}

With this way, is it possible we repeat enter interrupt handler without chance 
to
clear it?

Thanks
Jun
> 
> > err = devm_request_threaded_irq(>dev, client->irq, NULL,
> > _tcpci_irq,
> > IRQF_ONESHOT | IRQF_TRIGGER_LOW,
> > dev_name(>dev), chip);
> > if (err < 0)
> > -   return err;
> > +   tcpci_unregister_port(chip->tcpci);
> >
> > -   chip->tcpci = tcpci_register_port(>dev, >data);
> > -   if (PTR_ERR_OR_ZERO(chip->tcpci))
> > -   return PTR_ERR(chip->tcpci);
> > -
> > -   i2c_set_clientdata(client, chip);
> > -   return 0;
> > +   return err;
> >  }
> >
> >  static int tcpci_remove(struct i2c_client *client)
> > --
> > 2.7.4
> >

Re: [PATCH v5 1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Benjamin Herrenschmidt]

On Mon, 2018-03-19 at 12:56 +0200, Felipe Balbi wrote:
> >> do you really need this to be safe? You don't seem to be modifying
> >> ep_list here.
> >
> > Yes, ep->dispose() may do just that. In my Aspeed implementation in
> > fact that's pretty much the first thing it does.
> >
> > IE, I'm returning all the endpoints to the common pool, thus taking
> > them out of the gadget EP list.
> >
> > That said, there could be other reasons why a driver might want to know
> > about disposal without actually taking all the EPs back (for example a
> > driver could have some dedicated EPs and some in a pool) so I prefer
> > the list_del remaining in the back-end.
> 
> That seems rather obfuscated. There's no indication that ep_list is
> modified from within that iteration block. Seems like it would be best
> to make the list_del() explicit and ->dispose() just adds the ep to its
> own internal list. No?

The problem with this approach is that other existing UDC drivers who
don't do dynamic EP management might break since they assume the EPs
remain in the EP list for ever.

So we would have to make the list_del conditional to the presence of
the ->dispose callback, or add a flag or something like that, which I
don't find particularily elegant either.

Also it's the backend that adds to the EP list, it should be the
backend that removes from it to. I don't like when you end up in a
situation where a different "layer" does half of the work. It gets more
confusing and bug prone. The ep_list management is under ownership of
the UDC and thus should probably remain that way imho.

I think it makes sense from a high level perspective to say that the
UDC backend can optionally support disposing of EPs. I think all that's
needed here is maybe adding a comment to my patch, something along
those lines:

/*
 * Some UDC backends have a dynamic EP allocation scheme.
 *
 * In that case, the dispose() callback is used to notify the
 * backend that the EPs are no longer in use.
 *
 * Note: The UDC backend can remove the EP from the ep_list as
 *   a result, so we need to use the _safe list iterator.
 */
list_for_each_entry_safe(ep, tmp_ep,
   >gadget->ep_list, ep_list) {
...

Would that work for you ?

Cheers,
Ben.

 
--
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/16] treewide: simplify Kconfig dependencies for removed archs

[Alexandre Belloni]

On 14/03/2018 at 15:43:46 +0100, Arnd Bergmann wrote:
> A lot of Kconfig symbols have architecture specific dependencies.
> In those cases that depend on architectures we have already removed,
> they can be omitted.
> 
> Signed-off-by: Arnd Bergmann 
> ---
>  block/bounce.c   |  2 +-
>  drivers/ide/Kconfig  |  2 +-
>  drivers/ide/ide-generic.c| 12 +---
>  drivers/input/joystick/analog.c  |  2 +-
>  drivers/isdn/hisax/Kconfig   | 10 +-
>  drivers/net/ethernet/davicom/Kconfig |  2 +-
>  drivers/net/ethernet/smsc/Kconfig|  6 +++---
>  drivers/net/wireless/cisco/Kconfig   |  2 +-
>  drivers/pwm/Kconfig  |  2 +-
>  drivers/rtc/Kconfig  |  2 +-

Acked-by: Alexandre Belloni 

>  drivers/spi/Kconfig  |  4 ++--
>  drivers/usb/musb/Kconfig |  2 +-
>  drivers/video/console/Kconfig|  3 +--
>  drivers/watchdog/Kconfig |  6 --
>  drivers/watchdog/Makefile|  6 --
>  fs/Kconfig.binfmt|  5 ++---
>  fs/minix/Kconfig |  2 +-
>  include/linux/ide.h  |  7 +--
>  init/Kconfig |  5 ++---
>  lib/Kconfig.debug| 13 +
>  lib/test_user_copy.c |  2 --
>  mm/Kconfig   |  7 ---
>  mm/percpu.c  |  4 
>  23 files changed, 31 insertions(+), 77 deletions(-)
> 
> diff --git a/block/bounce.c b/block/bounce.c
> index 6a3e68292273..dd0b93f2a871 100644
> --- a/block/bounce.c
> +++ b/block/bounce.c
> @@ -31,7 +31,7 @@
>  static struct bio_set *bounce_bio_set, *bounce_bio_split;
>  static mempool_t *page_pool, *isa_page_pool;
>  
> -#if defined(CONFIG_HIGHMEM) || defined(CONFIG_NEED_BOUNCE_POOL)
> +#if defined(CONFIG_HIGHMEM)
>  static __init int init_emergency_pool(void)
>  {
>  #if defined(CONFIG_HIGHMEM) && !defined(CONFIG_MEMORY_HOTPLUG)
> diff --git a/drivers/ide/Kconfig b/drivers/ide/Kconfig
> index cf1fb3fb5d26..901b8833847f 100644
> --- a/drivers/ide/Kconfig
> +++ b/drivers/ide/Kconfig
> @@ -200,7 +200,7 @@ comment "IDE chipset support/bugfixes"
>  
>  config IDE_GENERIC
>   tristate "generic/default IDE chipset support"
> - depends on ALPHA || X86 || IA64 || M32R || MIPS || ARCH_RPC
> + depends on ALPHA || X86 || IA64 || MIPS || ARCH_RPC
>   default ARM && ARCH_RPC
>   help
> This is the generic IDE driver.  This driver attaches to the
> diff --git a/drivers/ide/ide-generic.c b/drivers/ide/ide-generic.c
> index 54d7c4685d23..80c0d69b83ac 100644
> --- a/drivers/ide/ide-generic.c
> +++ b/drivers/ide/ide-generic.c
> @@ -13,13 +13,10 @@
>  #include 
>  #include 
>  
> -/* FIXME: convert arm and m32r to use ide_platform host driver */
> +/* FIXME: convert arm to use ide_platform host driver */
>  #ifdef CONFIG_ARM
>  #include 
>  #endif
> -#ifdef CONFIG_M32R
> -#include 
> -#endif
>  
>  #define DRV_NAME "ide_generic"
>  
> @@ -35,13 +32,6 @@ static const struct ide_port_info ide_generic_port_info = {
>  #ifdef CONFIG_ARM
>  static const u16 legacy_bases[] = { 0x1f0 };
>  static const int legacy_irqs[]  = { IRQ_HARDDISK };
> -#elif defined(CONFIG_PLAT_M32700UT) || defined(CONFIG_PLAT_MAPPI2) || \
> -  defined(CONFIG_PLAT_OPSPUT)
> -static const u16 legacy_bases[] = { 0x1f0 };
> -static const int legacy_irqs[]  = { PLD_IRQ_CFIREQ };
> -#elif defined(CONFIG_PLAT_MAPPI3)
> -static const u16 legacy_bases[] = { 0x1f0, 0x170 };
> -static const int legacy_irqs[]  = { PLD_IRQ_CFIREQ, PLD_IRQ_IDEIREQ };
>  #elif defined(CONFIG_ALPHA)
>  static const u16 legacy_bases[] = { 0x1f0, 0x170, 0x1e8, 0x168 };
>  static const int legacy_irqs[]  = { 14, 15, 11, 10 };
> diff --git a/drivers/input/joystick/analog.c b/drivers/input/joystick/analog.c
> index be1b4921f22a..eefac7978f93 100644
> --- a/drivers/input/joystick/analog.c
> +++ b/drivers/input/joystick/analog.c
> @@ -163,7 +163,7 @@ static unsigned int get_time_pit(void)
>  #define GET_TIME(x)  do { x = (unsigned int)rdtsc(); } while (0)
>  #define DELTA(x,y)   ((y)-(x))
>  #define TIME_NAME"TSC"
> -#elif defined(__alpha__) || defined(CONFIG_ARM) || defined(CONFIG_ARM64) || 
> defined(CONFIG_RISCV) || defined(CONFIG_TILE)
> +#elif defined(__alpha__) || defined(CONFIG_ARM) || defined(CONFIG_ARM64) || 
> defined(CONFIG_RISCV)
>  #define GET_TIME(x)  do { x = get_cycles(); } while (0)
>  #define DELTA(x,y)   ((y)-(x))
>  #define TIME_NAME"get_cycles"
> diff --git a/drivers/isdn/hisax/Kconfig b/drivers/isdn/hisax/Kconfig
> index eb83d94ab4fe..38cfc8baae19 100644
> --- a/drivers/isdn/hisax/Kconfig
> +++ b/drivers/isdn/hisax/Kconfig
> @@ -109,7 +109,7 @@ config HISAX_16_3
>  
>  config HISAX_TELESPCI
>   bool "Teles PCI"
> - depends on PCI && (BROKEN || !(SPARC || PPC || PARISC || M68K || (MIPS 
> && !CPU_LITTLE_ENDIAN) || FRV || (XTENSA && 

Re: [PATCH v6] usb: core: Add "quirks" parameter for usbcore

[Kai Heng Feng]

Kai Heng Feng  wrote:


Matthew Wilcox  wrote:


On Tue, Mar 13, 2018 at 03:26:19PM +0800, Kai-Heng Feng wrote:

+   usbcore.quirks=
+   [USB] A list of quirks entries to supplement or
+   override the built-in usb core quirk list.  List
+   entries are separated by commas.  Each entry has
+   the form VID:PID:Flags where VID and PID are Vendor
+   and Product ID values (4-digit hex numbers) and
+   Flags is a set of characters, each corresponding
+   to a common usb core quirk flag as follows:


This doesn't really tell me that the specified quirks will be XORed with
the built-in quirks.  Maybe something like ...

[USB] A list of quirk entries to augment the
built-in usb core quirk list.  List entries are
separated by commas.  Each entry has the form
VendorID:ProductID:Flags.  The IDs are 4-digit hex
numbers and Flags is a set of letters.  Each letter
will change the built-in quirk; setting it if it is
clear and clearing it if it is set.  The letters
have the following meanings:


Thanks, will update the description to specify the XOR behavior.


+   /* Each entry consists of VID:PID:flags */
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;
+
+   field = strsep(, ":");
+   if (!field)
+   break;
+
+   if (kstrtou16(field, 16, ))
+   break;


Is there a reason to not use sscanf here like hid-quirks.c does?


Actually I use sscanf in previous patch version. Using kstrto* isn't  
needed anymore, I'll use sscanf in next version.


Okay, I remember why I don't use sscanf, mainly because the quirk_param in  
hid-quirks.c is charp instead of char, so using sscanf is trivia for it.


We use plain char* here because of module_param_cb(), so I guess using  
sscanf doesn't make much sense here. strsep() isn't the most elegant  
solution but it does its job well.


I'll update the description and send a new patch.

Kai-Heng



Kai-Heng



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

[PATCH 4.4 068/134] usb: dwc2: Make sure we disconnect the gadget state

[Greg Kroah-Hartman]

4.4-stable review patch.  If anyone has any objections, please let me know.

--

From: John Stultz 


[ Upstream commit dad3f793f20fbb5c0c342f0f5a0bdf69a4d76089 ]

I had seen some odd behavior with HiKey's usb-gadget interface
that I finally seemed to have chased down. Basically every other
time I plugged in the OTG port, the gadget interface would
properly initialize. The other times, I'd get a big WARN_ON
in dwc2_hsotg_init_fifo() about the fifo_map not being clear.

Ends up if we don't disconnect the gadget state, the fifo-map
doesn't get cleared properly, which causes WARN_ON messages and
also results in the device not properly being setup as a gadget
every other time the OTG port is connected.

So this patch adds a call to dwc2_hsotg_disconnect() in the
reset path so the state is properly cleared.

With it, the gadget interface initializes properly on every
plug in.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Acked-by: John Youn 
Signed-off-by: John Stultz 
Signed-off-by: Felipe Balbi 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/usb/dwc2/hcd.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -1385,6 +1385,7 @@ static void dwc2_conn_id_status_change(s
dwc2_core_init(hsotg, false, -1);
dwc2_enable_global_interrupts(hsotg);
spin_lock_irqsave(>lock, flags);
+   dwc2_hsotg_disconnect(hsotg);
dwc2_hsotg_core_init_disconnected(hsotg, false);
spin_unlock_irqrestore(>lock, flags);
dwc2_hsotg_core_connect(hsotg);


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

[PATCH v3] usb: dwc3: pci: Properly cleanup resource

[Thinh Nguyen]

Platform device is allocated before adding resources. Make sure to
properly cleanup on error case.

Cc: 
Fixes: f1c7e7108109 ("usb: dwc3: convert to pcim_enable_device()")
Signed-off-by: Thinh Nguyen 
---
Changes in v3:
 - Include Cc: 
Changes in v2:
 - Separate from patch series "usb: dwc3: pci: Make device properties 
customizable"


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

diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
index 3ba11136ebf0..c961a94d136b 100644
--- a/drivers/usb/dwc3/dwc3-pci.c
+++ b/drivers/usb/dwc3/dwc3-pci.c
@@ -222,7 +222,7 @@ static int dwc3_pci_probe(struct pci_dev *pci,
ret = platform_device_add_resources(dwc->dwc3, res, ARRAY_SIZE(res));
if (ret) {
dev_err(dev, "couldn't add resources to dwc3 device\n");
-   return ret;
+   goto err;
}
 
dwc->pci = pci;
-- 
2.11.0

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

[PATCH] MAINTAINERS: Update maintainer for dwc2

[John Youn]

Update to show Minas Harutyunyan as the new maintainer for dwc2.

Signed-off-by: John Youn 
---
 MAINTAINERS | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 93a12af4f180..fc23eb016198 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4119,7 +4119,7 @@ S:Supported
 F: drivers/mtd/nand/denali*
 
 DESIGNWARE USB2 DRD IP DRIVER
-M: John Youn 
+M: Minas Harutyunyan 
 L: linux-usb@vger.kernel.org
 T: git git://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git
 S: Maintained
-- 
2.11.0

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

[PATCHv2] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Joel Pepper]

Add bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
Before all "bFrameindex" attributes were set to "1" with no way to
configure the gadget otherwise. This resulted in the host always
negotiating for bFrameIndex 1 (i.e. the first framesize of the gadget).
After the negotiation the host driver will set the user or application
selected framesize, while the gadget is actually set to the first
framesize.

Note that this still requires the gadget to be configured with unique
bFrameIndexes for each frameSize of each format through configfs. An
alternative might be to automatically assign ascending indices when the
format is linked into the streaming header, but the user space gadget
would need a way to check or predict the indices so that it can properly
interpret to PROBE/COMMIT CONTROL requests

v2: Add the new attribute to both MJPEG and uncompressedframe descriptos
in Documentation/ABI, with note that it was added only in a later
kernel version

Signed-off-by: Joel Pepper 
---
 Documentation/ABI/testing/configfs-usb-gadget-uvc | 17 +
 drivers/usb/gadget/function/uvc_configfs.c|  3 +++
 2 files changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/configfs-usb-gadget-uvc 
b/Documentation/ABI/testing/configfs-usb-gadget-uvc
index 1ba0d0f..d435cf7 100644
--- a/Documentation/ABI/testing/configfs-usb-gadget-uvc
+++ b/Documentation/ABI/testing/configfs-usb-gadget-uvc
@@ -194,6 +194,14 @@ Description:   Specific MJPEG frame descriptors
bmCapabilities  - still image support, fixed frame-rate
support
 
+Date:  Mar 2018
+KernelVersion: 4.16
+
+   bFrameIndex - unique id for this framedescriptor;
+   if using multiple framedescriptors for
+   same format, user needs to set distinct
+   value for each frame descriptor
+
 What:  
/config/usb-gadget/gadget/functions/uvc.name/streaming/uncompressed
 Date:  Dec 2014
 KernelVersion: 4.0
@@ -241,6 +249,15 @@ Description:   Specific uncompressed frame descriptors
bmCapabilities  - still image support, fixed frame-rate
support
 
+Date:   Mar 2018
+KernelVersion:  4.16
+
+bFrameIndex - unique id for this framedescriptor;
+if using multiple framedescriptors for
+same format, user needs to set distinct
+value for each frame descriptor
+
+
 What:  /config/usb-gadget/gadget/functions/uvc.name/streaming/header
 Date:  Dec 2014
 KernelVersion: 4.0
diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index c9b8cc4a..5966d65 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
 
 UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
noop_conversion, 8);
+UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
+   noop_conversion, 8);
 UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
@@ -1137,6 +1139,7 @@ UVC_ATTR(uvcg_frame_, dw_frame_interval, dwFrameInterval);
 
 static struct configfs_attribute *uvcg_frame_attrs[] = {
_frame_attr_bm_capabilities,
+   _frame_attr_b_frame_index,
_frame_attr_w_width,
_frame_attr_w_height,
_frame_attr_dw_min_bit_rate,
-- 
2.1.4

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

[PATCH 4.9 139/241] usb: dwc2: Make sure we disconnect the gadget state

[Greg Kroah-Hartman]

4.9-stable review patch.  If anyone has any objections, please let me know.

--

From: John Stultz 


[ Upstream commit dad3f793f20fbb5c0c342f0f5a0bdf69a4d76089 ]

I had seen some odd behavior with HiKey's usb-gadget interface
that I finally seemed to have chased down. Basically every other
time I plugged in the OTG port, the gadget interface would
properly initialize. The other times, I'd get a big WARN_ON
in dwc2_hsotg_init_fifo() about the fifo_map not being clear.

Ends up if we don't disconnect the gadget state, the fifo-map
doesn't get cleared properly, which causes WARN_ON messages and
also results in the device not properly being setup as a gadget
every other time the OTG port is connected.

So this patch adds a call to dwc2_hsotg_disconnect() in the
reset path so the state is properly cleared.

With it, the gadget interface initializes properly on every
plug in.

Cc: Wei Xu 
Cc: Guodong Xu 
Cc: Amit Pundir 
Cc: Rob Herring 
Cc: John Youn 
Cc: Douglas Anderson 
Cc: Chen Yu 
Cc: Felipe Balbi 
Cc: Greg Kroah-Hartman 
Cc: linux-usb@vger.kernel.org
Acked-by: John Youn 
Signed-off-by: John Stultz 
Signed-off-by: Felipe Balbi 
Signed-off-by: Sasha Levin 
Signed-off-by: Greg Kroah-Hartman 
---
 drivers/usb/dwc2/hcd.c |1 +
 1 file changed, 1 insertion(+)

--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -3220,6 +3220,7 @@ static void dwc2_conn_id_status_change(s
dwc2_core_init(hsotg, false);
dwc2_enable_global_interrupts(hsotg);
spin_lock_irqsave(>lock, flags);
+   dwc2_hsotg_disconnect(hsotg);
dwc2_hsotg_core_init_disconnected(hsotg, false);
spin_unlock_irqrestore(>lock, flags);
dwc2_hsotg_core_connect(hsotg);


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

Re: howto debug xhci driver?

[Bin Liu]

On Mon, Mar 05, 2018 at 01:21:42PM -0600, Bin Liu wrote:
> On Mon, Mar 05, 2018 at 10:16:49AM +0200, Felipe Balbi wrote:
> > 
> > Hi,
> > 
> > Bin Liu  writes:
> > > I am relatively new to xhci and its driver. I am trying to get a xhci
> > > driver runtime log to understand how it handles usb transactions, but I
> > > don't get much information with dynamic debug (module xhci_hcd) or
> > > enabling xhci trace events. Is there any other methods you guys use to
> > > debug xhci driver?
> > 
> > tracepoints, the best thing since sliced bread ;-)
> 
> I know, but I didn't get any trace log in bulk IN transfers :(
> It turns out - I was on v4.9, which doesn't have much tracepoints. Now I
> see you added more since v4.11. I will try to move to the latest kernel.
> 
> > 
> > > BTY, the issue I am trying to debug is when reading bulk IN data from a
> > > USB2.0 device, if the device doesn't have data to transmit and NAKs the
> > > IN packet, after 4 pairs of IN-NAK transactions, xhci stops sending
> > > further IN tokens until the next SOF, which leaves ~90us gape on the
> > > bus.
> > >
> > > But when reading data from a USB2.0 thumb drive, this issue doesn't
> > > happen, even if the device NAKs the IN tokens, xhci still keeps sending
> > > IN tokens, which is way more than 4 pairs of IN-NAK transactions.
> > 
> > Thumb drive has Bulk endpoints, what is the other device's transfer type?
> 
> It is bulk too. I asked for device descriptors. This is a remote debug
> effort for me, I don't have that device...
> 
> > 
> > > Any one has a clue on what causes xhci to stop sending IN tokens after
> > > the device NAK'd 4 times?

By accident, I reproduced the issue if addng a hub in the middle...
any comments about why a hub changes this xhci behavior is appreciated.

> > tracepoints, please :-)

I will get tracepoints for cases w/ and w/o a hub.

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 usb-next v10 3/8] usb: core: add a wrapper for the USB PHYs on the HCD

[Martin Blumenstingl]

Hi Roger,

On Mon, Mar 19, 2018 at 9:49 AM, Roger Quadros  wrote:
> Hi,
>
> On 19/03/18 00:29, Martin Blumenstingl wrote:
>> Hi Roger,
>>
>> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros  wrote:
>>> +some TI folks
>>>
>>> Hi Martin,
>>>
>>> On 18/02/18 20:44, Martin Blumenstingl wrote:
 Many SoC platforms have separate devices for the USB PHY which are
 registered through the generic PHY framework. These PHYs have to be
 enabled to make the USB controller actually work. They also have to be
 disabled again on shutdown/suspend.

 Currently (at least) the following HCI platform drivers are using custom
 code to obtain all PHYs via devicetree for the roothub/controller and
 disable/enable them when required:
 - ehci-platform.c has ehci_platform_power_{on,off}
 - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
 - ohci-platform.c has ohci_platform_power_{on,off}

 With this new wrapper the USB PHYs can be specified directly in the
 USB controller's devicetree node (just like on the drivers listed
 above). This allows SoCs like the Amlogic Meson GXL family to operate
 correctly once this is wired up correctly. These SoCs use a dwc3
 controller and require all USB PHYs to be initialized (if one of the USB
 PHYs it not initialized then none of USB port works at all).

 Signed-off-by: Martin Blumenstingl 
 Tested-by: Yixun Lan 
 Cc: Neil Armstrong 
 Cc: Chunfeng Yun 
>>>
>>> This patch is breaking low power cases on TI SoCs when USB is in host mode.
>>> I'll explain why below.
>> based on your explanation and reading the TI PHY drivers I am assuming
>> that the affected SoCs are using the "phy-omap-usb2" driver
>>
> yes and phy-ti-pipe3 as well i.e. "ti,phy-usb3" and "ti,omap-usb3"
I missed that, thanks

 ---
  drivers/usb/core/Makefile |   2 +-
  drivers/usb/core/phy.c| 158 
 ++
  drivers/usb/core/phy.h|   7 ++
  3 files changed, 166 insertions(+), 1 deletion(-)
  create mode 100644 drivers/usb/core/phy.c
  create mode 100644 drivers/usb/core/phy.h

 diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
 index 92c9cefb4317..18e874b0441e 100644
 --- a/drivers/usb/core/Makefile
 +++ b/drivers/usb/core/Makefile
 @@ -6,7 +6,7 @@
  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
 -usbcore-y += port.o
 +usbcore-y += phy.o port.o

  usbcore-$(CONFIG_OF) += of.o
  usbcore-$(CONFIG_USB_PCI)+= hcd-pci.o
 diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
 new file mode 100644
 index ..09b7c43c0ea4
 --- /dev/null
 +++ b/drivers/usb/core/phy.c
 @@ -0,0 +1,158 @@
 +// SPDX-License-Identifier: GPL-2.0+
 +/*
 + * A wrapper for multiple PHYs which passes all phy_* function calls to
 + * multiple (actual) PHY devices. This is comes handy when initializing
 + * all PHYs on a HCD and to keep them all in the same state.
 + *
 + * Copyright (C) 2018 Martin Blumenstingl 
 
 + */
 +
 +#include 
 +#include 
 +#include 
 +#include 
 +
 +#include "phy.h"
 +
 +struct usb_phy_roothub {
 + struct phy  *phy;
 + struct list_headlist;
 +};
 +
 +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
 +{
 + struct usb_phy_roothub *roothub_entry;
 +
 + roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), 
 GFP_KERNEL);
 + if (!roothub_entry)
 + return ERR_PTR(-ENOMEM);
 +
 + INIT_LIST_HEAD(_entry->list);
 +
 + return roothub_entry;
 +}
 +
 +static int usb_phy_roothub_add_phy(struct device *dev, int index,
 +struct list_head *list)
 +{
 + struct usb_phy_roothub *roothub_entry;
 + struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index);
 +
 + if (IS_ERR_OR_NULL(phy)) {
 + if (!phy || PTR_ERR(phy) == -ENODEV)
 + return 0;
 + else
 + return PTR_ERR(phy);
 + }
 +
 + roothub_entry = usb_phy_roothub_alloc(dev);
 + if (IS_ERR(roothub_entry))
 + return PTR_ERR(roothub_entry);
 +
 + roothub_entry->phy = phy;
 +
 + list_add_tail(_entry->list, list);
 +
 + return 0;
 +}
 +
 +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
 +{
 + struct 

Re: [PATCH 1/1] usb: musb: gadget: misplaced out of bounds check

[Bin Liu]

Hi,

On Mon, Mar 19, 2018 at 08:12:28AM +0100, Heinrich Schuchardt wrote:
> musb->endpoints[] has array size MUSB_C_NUM_EPS.
> We must check array bounds before accessing the array and not afterwards.
> 
> Signed-off-by: Heinrich Schuchardt 
> ---
>  drivers/usb/musb/musb_gadget_ep0.c | 7 ++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
> b/drivers/usb/musb/musb_gadget_ep0.c
> index 18da4873e52e..482e7c2f8dc7 100644
> --- a/drivers/usb/musb/musb_gadget_ep0.c
> +++ b/drivers/usb/musb/musb_gadget_ep0.c
> @@ -88,6 +88,11 @@ static int service_tx_status_request(
>   break;
>   }
>  
> + if (epnum >= MUSB_C_NUM_EPS) {

only the LSB 4 bits are the EP number, bit7 is the direction flag. So
you can only compare the LSB 4 bits here.

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 2/2] dt-bindings: usb: rt1711h device tree binding document

[Guenter Roeck]

On Mon, Mar 19, 2018 at 11:49:35AM +0800, ShuFan Lee wrote:
> From: ShuFan Lee 
> 
> Add device tree binding document for Richtek RT1711H Type-C chip driver

Cc: Rob Herring , devicet...@vger.kernel.org
is missing. Added here but it might make sense to resend.

>
> Signed-off-by: ShuFan Lee 
> ---
>  .../devicetree/bindings/usb/richtek,rt1711h.txt| 30 
> ++
>  1 file changed, 30 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> 
>  changelogs between v2 & v3
>  - add dt-bindings document for rt1711h typec chip driver
> 
> diff --git a/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt 
> b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> new file mode 100644
> index ..7da4dac78ea7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/richtek,rt1711h.txt
> @@ -0,0 +1,30 @@
> +Richtek RT1711H TypeC PD Controller.
> +
> +Required properties:
> + - compatible : Must be "richtek,rt1711h".
> + - reg : Must be 0x4e, it's slave address of RT1711H.
> +
> +Recommended properties :
> + - interrupt-parent : the phandle for the interrupt controller that
> +   provides interrupts for this device.
> + - interrupts :  where a is the interrupt number and b represents an
> +   encoding of the sense and level information for the interrupt.
> +
> +Optional properties :
> + - rt,intr-gpios : IRQ GPIO pin that's connected to RT1711H interrupt.
> +   if interrupt-parent & interrupts are not defined, use this property 
> instead.
> +
> +Example :
> +rt1711h@4e {
> + compatible = "richtek,rt1711h";
> + reg = <0x4e>;
> + interrupt-parent = <>;
> + interrupts = <0 IRQ_TYPE_LEVEL_LOW>;
> +};
> +
> +Example using rt,intr-gpios :
> +rt1711h@4e {
> + compatible = "richtek,rt1711h";
> + reg = <0x4e>;
> + rt,intr-gpios = < 0 0x0>;
> +};
> -- 
> 1.9.1
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v7] usb: core: Add "quirks" parameter for usbcore

[Kai-Heng Feng]

Trying quirks in usbcore needs to rebuild the driver or the entire
kernel if it's builtin. It can save a lot of time if usbcore has similar
ability like "usbhid.quirks=" and "usb-storage.quirks=".

Rename the original quirk detection function to "static" as we introduce
this new "dynamic" function.

Now users can use "usbcore.quirks=" as short term workaround before the
next kernel release. Also, the quirk parameter can XOR the builtin
quirks for debugging purpose.

This is inspired by usbhid and usb-storage.

Signed-off-by: Kai-Heng Feng 
---
v7: Update documentation to specify the quirks parameter can XOR the
built-in quirks.

v6: Add missed header file .

v5: Use module_param_cb() to get notified when parameter changes.
Replace linked list with array.

v4: Use kmalloc() to reduce memory usage.
Remove tolower() so we can use capital character in the future.

v3: Stop overridding static quirks.
Use XOR for dynamic quirks.
Save parsed quirk as a list to avoid parsing quirk table everytime.

v2: Use in-kernel tolower() function.

 Documentation/admin-guide/kernel-parameters.txt |  56 
 drivers/usb/core/quirks.c   | 178 +++-
 drivers/usb/core/usb.c  |   1 +
 drivers/usb/core/usb.h  |   1 +
 4 files changed, 231 insertions(+), 5 deletions(-)

diff --git a/Documentation/admin-guide/kernel-parameters.txt 
b/Documentation/admin-guide/kernel-parameters.txt
index 1d1d53f85ddd..e00cdd313dc2 100644
--- a/Documentation/admin-guide/kernel-parameters.txt
+++ b/Documentation/admin-guide/kernel-parameters.txt
@@ -4368,6 +4368,62 @@
 
usbcore.nousb   [USB] Disable the USB subsystem
 
+   usbcore.quirks=
+   [USB] A list of quirk entries to augment the built-in
+   usb core quirk list. List entries are separated by
+   commas. Each entry has the form
+   VendorID:ProductID:Flags. The IDs are 4-digit hex
+   numbers and Flags is a set of letters. Each letter
+   will change the built-in quirk; setting it if it is
+   clear and clearing it if it is set. The letters have
+   the following meanings:
+   a = USB_QUIRK_STRING_FETCH_255 (string
+   descriptors must not be fetched using
+   a 255-byte read);
+   b = USB_QUIRK_RESET_RESUME (device can't resume
+   correctly so reset it instead);
+   c = USB_QUIRK_NO_SET_INTF (device can't handle
+   Set-Interface requests);
+   d = USB_QUIRK_CONFIG_INTF_STRINGS (device can't
+   handle its Configuration or Interface
+   strings);
+   e = USB_QUIRK_RESET (device can't be reset
+   (e.g morph devices), don't use reset);
+   f = USB_QUIRK_HONOR_BNUMINTERFACES (device has
+   more interface descriptions than the
+   bNumInterfaces count, and can't handle
+   talking to these interfaces);
+   g = USB_QUIRK_DELAY_INIT (device needs a pause
+   during initialization, after we read
+   the device descriptor);
+   h = USB_QUIRK_LINEAR_UFRAME_INTR_BINTERVAL (For
+   high speed and super speed interrupt
+   endpoints, the USB 2.0 and USB 3.0 spec
+   require the interval in microframes (1
+   microframe = 125 microseconds) to be
+   calculated as interval = 2 ^
+   (bInterval-1).
+   Devices with this quirk report their
+   bInterval as the result of this
+   calculation instead of the exponent
+   variable used in the calculation);
+   i = USB_QUIRK_DEVICE_QUALIFIER (device can't
+   handle device_qualifier descriptor
+   requests);
+   j = USB_QUIRK_IGNORE_REMOTE_WAKEUP (device
+   generates spurious wakeup, ignore
+   remote wakeup capability);
+   k = 

Re: [PATCH 1/2] staging: typec: rt1711h typec chip driver

[Guenter Roeck]

On Mon, Mar 19, 2018 at 11:49:34AM +0800, ShuFan Lee wrote:
> From: ShuFan Lee 
> 
> Richtek RT1711H Type-C chip driver that works with
> Type-C Port Controller Manager to provide USB PD and
> USB Type-C functionalities.
> Add definition of TCPC_CC_STATUS_TOGGLING.
> 
> Signed-off-by: ShuFan Lee 

No additional feedback on top of Heikki's comments.

As we get more drivers, it might make sense to provide an
API function for rt1711h_check_revision(), or even have
tcpci_register_port() validate vendor and product IDs.
But I think for now we are good.

Reviewed-by: Guenter Roeck 

> ---
>  drivers/staging/typec/Kconfig |   8 +
>  drivers/staging/typec/Makefile|   1 +
>  drivers/staging/typec/tcpci.h |   1 +
>  drivers/staging/typec/tcpci_rt1711h.c | 329 
> ++
>  4 files changed, 339 insertions(+)
>  create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
> 
>  changelogs between v1 and v2
>  - use gpiod_* instead of gpio_*
> 
> diff --git a/drivers/staging/typec/Kconfig b/drivers/staging/typec/Kconfig
> index 5359f556d203..3aa981fbc8f5 100644
> --- a/drivers/staging/typec/Kconfig
> +++ b/drivers/staging/typec/Kconfig
> @@ -9,6 +9,14 @@ config TYPEC_TCPCI
>   help
> Type-C Port Controller driver for TCPCI-compliant controller.
>  
> +config TYPEC_RT1711H
> + tristate "Richtek RT1711H Type-C chip driver"
> + select TYPEC_TCPCI
> + help
> +   Richtek RT1711H Type-C chip driver that works with
> +   Type-C Port Controller Manager to provide USB PD and USB
> +   Type-C functionalities.
> +
>  endif
>  
>  endmenu
> diff --git a/drivers/staging/typec/Makefile b/drivers/staging/typec/Makefile
> index 53d649abcb53..7803d485e1b3 100644
> --- a/drivers/staging/typec/Makefile
> +++ b/drivers/staging/typec/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o
> +obj-$(CONFIG_TYPEC_RT1711H)  += tcpci_rt1711h.o
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index 34c865f0dcf6..303ebde26546 100644
> --- a/drivers/staging/typec/tcpci.h
> +++ b/drivers/staging/typec/tcpci.h
> @@ -59,6 +59,7 @@
>  #define TCPC_POWER_CTRL_VCONN_ENABLE BIT(0)
>  
>  #define TCPC_CC_STATUS   0x1d
> +#define TCPC_CC_STATUS_TOGGLING  BIT(5)
>  #define TCPC_CC_STATUS_TERM  BIT(4)
>  #define TCPC_CC_STATUS_CC2_SHIFT 2
>  #define TCPC_CC_STATUS_CC2_MASK  0x3
> diff --git a/drivers/staging/typec/tcpci_rt1711h.c 
> b/drivers/staging/typec/tcpci_rt1711h.c
> new file mode 100644
> index ..12afac363d6d
> --- /dev/null
> +++ b/drivers/staging/typec/tcpci_rt1711h.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, Richtek Technology Corporation
> + *
> + * Richtek RT1711H Type-C Chip Driver
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "tcpci.h"
> +
> +#define RT1711H_RTCTRL8  0x9B
> +
> +/* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
> +#define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
> + (((ck300) << 7) | ((ship_off) << 5) | \
> + ((auto_idle) << 3) | ((tout) & 0x07))
> +
> +#define RT1711H_RTCTRL11 0x9E
> +
> +/* I2C timeout = (tout + 1) * 12.5ms */
> +#define RT1711H_RTCTRL11_SET(en, tout) \
> +  (((en) << 7) | ((tout) & 0x0F))
> +
> +#define RT1711H_RTCTRL13 0xA0
> +#define RT1711H_RTCTRL14 0xA1
> +#define RT1711H_RTCTRL15 0xA2
> +#define RT1711H_RTCTRL16 0xA3
> +
> +struct rt1711h_chip {
> + struct tcpci_data data;
> + struct tcpci *tcpci;
> + struct device *dev;
> + int irq;
> +};
> +
> +static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 
> *val)
> +{
> + return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u16));
> +}
> +
> +static int rt1711h_write16(struct rt1711h_chip *chip, unsigned int reg, u16 
> val)
> +{
> + return regmap_raw_write(chip->data.regmap, reg, , sizeof(u16));
> +}
> +
> +static int rt1711h_read8(struct rt1711h_chip *chip, unsigned int reg, u8 
> *val)
> +{
> + return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u8));
> +}
> +
> +static int rt1711h_write8(struct rt1711h_chip *chip, unsigned int reg, u8 
> val)
> +{
> + return regmap_raw_write(chip->data.regmap, reg, , sizeof(u8));
> +}
> +
> +static const struct regmap_config rt1711h_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
> +};
> +
> +static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
> +{
> + return container_of(tdata, struct rt1711h_chip, data);
> +}
> +
> +static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> +{
> + int ret;
> + struct rt1711h_chip *chip = 

Re: [PATCH] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Greg KH]

On Mon, Mar 19, 2018 at 11:55:02AM +0100, Joel Pepper wrote:
> This adds bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
> Beforehand all bFrameindex were set to "1" with no way to configure the 
> gadget otherwise.
> 
> This resulted in the host always negotiating for bFrameIndex 1 (i.e. the
> first framesize of the gadget).
> After the negotiation the host driver will set the user- or application-
> selected framesize while the gadget is actually set to the first
> framesize.
> 
> Note that this still requires the gadget to be configured with unique 
> 'bFrameindex's for each frameSize of each format through configfs. An
> alternative might be to automatically assign ascending indices when the
> format is linked into the streaming header, but the user space gadget
> application would need a way to check or predict the indices so that it
> can properly interpret PROBE/COMMIT CONTROL requests.
> 
> Signed-off-by: Joel Pepper 
> ---
>  drivers/usb/gadget/function/uvc_configfs.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
> b/drivers/usb/gadget/function/uvc_configfs.c
> index c9b8cc4a..5966d65 100644
> --- a/drivers/usb/gadget/function/uvc_configfs.c
> +++ b/drivers/usb/gadget/function/uvc_configfs.c
> @@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
>  
>  UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
>   noop_conversion, 8);
> +UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
> + noop_conversion, 8);
>  UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
>  UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
>  UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
> @@ -1137,6 +1139,7 @@ UVC_ATTR(uvcg_frame_, dw_frame_interval, 
> dwFrameInterval);
>  
>  static struct configfs_attribute *uvcg_frame_attrs[] = {
>   _frame_attr_bm_capabilities,
> + _frame_attr_b_frame_index,

Isn't there also a Documentation/ABI/ update you need to do for this new
file?

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: [RFC] musb: removing otg protocol support

[Bin Liu]

On Mon, Mar 19, 2018 at 03:40:48PM +0100, Greg Kroah-Hartman wrote:
> On Mon, Mar 19, 2018 at 09:20:23AM -0500, Bin Liu wrote:
> > On Sun, Mar 18, 2018 at 02:16:25PM +0100, Greg Kroah-Hartman wrote:
> > > On Fri, Mar 16, 2018 at 02:09:51PM -0500, Bin Liu wrote:
> > > > Hi,
> > > > 
> > > > The kernel usb stack and musb drivers have gone through some changes in
> > > > the past several kernel versions, such as adding otg fsm, musb runtime
> > > > PM, and musb otg state moving from musb to musb->xceiv... I am wondering
> > > > if the otg protocol (hnp, srp) functions are already broken in the musb
> > > > drivers, but I don't have a platform to confirm it.
> > > > 
> > > > Do we know by any chance there is still someone using the musb otg
> > > > functions in any relatively newer kernel and we still need to support
> > > > otg in musb?  If not, I am thinking to clean up the otg functions in
> > > > musb drivers to make the code easy to read and maintain.
> > > 
> > > By "clean up" do you mean "delete it"?  :)
> > 
> > Yes, delete it to make the driver state machine simpler and use less
> > flags for recording states.
> > 
> > > 
> > > I don't know of any real OTG hardware that ever shipped, does anyone
> > > else?
> > > 
> > > > If we can make the conclusion to remove it, I propose the patch below
> > > > to disable musb otg first, then clean up the driver later if nobody
> > > > complains about the otg function removal.
> > > 
> > > It will take years for people who make these types of devices to notice
> > > that OTG is removed, so be careful about this.  Refactor away, but
> > 
> > I personally think we can safely say there wasn't any true OTG products
> > where were under development in the last several years, but I am more
> > concerned if there was any such product released in the past, for
> > example omap24xx/34xx based, but was still actively maintained to the
> > latest kernel? Then deleting OTG protocol from musb drivers would break
> > them.
> 
> Wasn't the BeagleBone devices using that chipset?  If not, then you are

No, BeagleBone uses TI AM335x devices which have two MUSB controllers,
and released in around 2012. And I don't think anyone uses the true OTG
on AM335x, but omap24xx/34xx devices are much older which I don't know
the history...

> probably fine.

Sounds good. I will send a patch in a few weeks to disable OTG in musb
for v3.18-rc1, too late for v4.17 now I think...

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: [RFC] musb: removing otg protocol support

[Greg Kroah-Hartman]

On Mon, Mar 19, 2018 at 09:20:23AM -0500, Bin Liu wrote:
> On Sun, Mar 18, 2018 at 02:16:25PM +0100, Greg Kroah-Hartman wrote:
> > On Fri, Mar 16, 2018 at 02:09:51PM -0500, Bin Liu wrote:
> > > Hi,
> > > 
> > > The kernel usb stack and musb drivers have gone through some changes in
> > > the past several kernel versions, such as adding otg fsm, musb runtime
> > > PM, and musb otg state moving from musb to musb->xceiv... I am wondering
> > > if the otg protocol (hnp, srp) functions are already broken in the musb
> > > drivers, but I don't have a platform to confirm it.
> > > 
> > > Do we know by any chance there is still someone using the musb otg
> > > functions in any relatively newer kernel and we still need to support
> > > otg in musb?  If not, I am thinking to clean up the otg functions in
> > > musb drivers to make the code easy to read and maintain.
> > 
> > By "clean up" do you mean "delete it"?  :)
> 
> Yes, delete it to make the driver state machine simpler and use less
> flags for recording states.
> 
> > 
> > I don't know of any real OTG hardware that ever shipped, does anyone
> > else?
> > 
> > > If we can make the conclusion to remove it, I propose the patch below
> > > to disable musb otg first, then clean up the driver later if nobody
> > > complains about the otg function removal.
> > 
> > It will take years for people who make these types of devices to notice
> > that OTG is removed, so be careful about this.  Refactor away, but
> 
> I personally think we can safely say there wasn't any true OTG products
> where were under development in the last several years, but I am more
> concerned if there was any such product released in the past, for
> example omap24xx/34xx based, but was still actively maintained to the
> latest kernel? Then deleting OTG protocol from musb drivers would break
> them.

Wasn't the BeagleBone devices using that chipset?  If not, then you are
probably fine.

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: serial: ftdi_sio: add support for Harman FirmwareHubEmulator

[Johan Hovold]

On Fri, Mar 16, 2018 at 10:20:46AM +0100, Clemens Werther wrote:
> Make the device auto-detectable by the driver
> 
> Signed-off-by: Clemens Werther 

Now applied (with a slightly updated commit message).

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

Re: [RFC] musb: removing otg protocol support

[Bin Liu]

On Sun, Mar 18, 2018 at 05:36:10PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 18-03-18 14:16, Greg Kroah-Hartman wrote:
> >On Fri, Mar 16, 2018 at 02:09:51PM -0500, Bin Liu wrote:
> >>Hi,
> >>
> >>The kernel usb stack and musb drivers have gone through some changes in
> >>the past several kernel versions, such as adding otg fsm, musb runtime
> >>PM, and musb otg state moving from musb to musb->xceiv... I am wondering
> >>if the otg protocol (hnp, srp) functions are already broken in the musb
> >>drivers, but I don't have a platform to confirm it.
> >>
> >>Do we know by any chance there is still someone using the musb otg
> >>functions in any relatively newer kernel and we still need to support
> >>otg in musb?  If not, I am thinking to clean up the otg functions in
> >>musb drivers to make the code easy to read and maintain.
> >
> >By "clean up" do you mean "delete it"?  :)
> >
> >I don't know of any real OTG hardware that ever shipped, does anyone
> >else?
> 
> I'm not aware of any hardware using the fancier otg functions such as
> role-swapping, etc. either. From an Allwinner pov as long as you keep

Thanks for confirming ;)

> plain gadget and host mode as controlled by the id pin work, I'm happy.

Yes, this is not changed.

> Note I'm not really involved in Allwinnner stuff anymore, so if you
> want some testing of the cleanup, you should probably ask the
> free-electron (now bootlin.com) people.

Thanks, but I am not sure how I can find the exact individual contects.

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: [RFC] musb: removing otg protocol support

[Bin Liu]

On Sun, Mar 18, 2018 at 02:16:25PM +0100, Greg Kroah-Hartman wrote:
> On Fri, Mar 16, 2018 at 02:09:51PM -0500, Bin Liu wrote:
> > Hi,
> > 
> > The kernel usb stack and musb drivers have gone through some changes in
> > the past several kernel versions, such as adding otg fsm, musb runtime
> > PM, and musb otg state moving from musb to musb->xceiv... I am wondering
> > if the otg protocol (hnp, srp) functions are already broken in the musb
> > drivers, but I don't have a platform to confirm it.
> > 
> > Do we know by any chance there is still someone using the musb otg
> > functions in any relatively newer kernel and we still need to support
> > otg in musb?  If not, I am thinking to clean up the otg functions in
> > musb drivers to make the code easy to read and maintain.
> 
> By "clean up" do you mean "delete it"?  :)

Yes, delete it to make the driver state machine simpler and use less
flags for recording states.

> 
> I don't know of any real OTG hardware that ever shipped, does anyone
> else?
> 
> > If we can make the conclusion to remove it, I propose the patch below
> > to disable musb otg first, then clean up the driver later if nobody
> > complains about the otg function removal.
> 
> It will take years for people who make these types of devices to notice
> that OTG is removed, so be careful about this.  Refactor away, but

I personally think we can safely say there wasn't any true OTG products
where were under development in the last several years, but I am more
concerned if there was any such product released in the past, for
example omap24xx/34xx based, but was still actively maintained to the
latest kernel? Then deleting OTG protocol from musb drivers would break
them.

> watch out you don't break someone's stuff and get told about it in 2
> years :(

My plan is to take two steps if we decided we can delete it.

First use the patch I copied in my first email to disable OTG protocols
support and wait for 2~3 quarters, we can simply revert it back if we
got yelled for any breakage ;)

Then *gradually* cleaning out any HNP/SRP related code through several
kernel versions (I don't think I have that much time to clean up all OTG
code within one kernel version anyway.)

If we break anyone in the second step, we can handle it as for any
typical regression, shouldn't be an issue.

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] usb: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

Hi Felipe,

Thanks for reviewing the patch , please find my comments inline

>-Original Message-
>From: Felipe Balbi [mailto:ba...@kernel.org]
>Sent: Monday, March 19, 2018 2:21 PM
>To: Anurag Kumar Vulisha ; Greg Kroah-Hartman
>
>Cc: v.anuragku...@gmail.com; Ajay Yugalkishore Pandey
>; linux-usb@vger.kernel.org; linux-
>ker...@vger.kernel.org; Anurag Kumar Vulisha 
>Subject: Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs
>
>
>Hi,
>
>Anurag Kumar Vulisha  writes:
>> This patch fixes two issues
>>
>> 1. The code logic in dwc3_prepare_one_trb() incorrectly uses the
>> address and length given in req packet even for scattergather lists.
>> This patch correct's the code to use sg->address and sg->length when
>> scattergather lists are present.
>>
>> 2. The present code correctly fetches the req's which were not queued
>> from the started_list but fails to start from the sg where it
>> previously stopped queuing because of the unavailable TRB's. This
>> patch correct's the code to start queuing from the correct sg in sglist.
>
>these two should be in separate patches, then.
>
Will create separate patches in next version

>> Signed-off-by: Anurag Kumar Vulisha 
>> ---
>>  drivers/usb/dwc3/core.h   |  4 
>>  drivers/usb/dwc3/gadget.c | 42
>> --
>>  2 files changed, 40 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h index
>> 860d2bc..2779e58 100644
>> --- a/drivers/usb/dwc3/core.h
>> +++ b/drivers/usb/dwc3/core.h
>> @@ -718,7 +718,9 @@ struct dwc3_hwparams {
>>   * @list: a list_head used for request queueing
>>   * @dep: struct dwc3_ep owning this request
>>   * @sg: pointer to first incomplete sg
>> + * @sg_to_start: pointer to the sg which should be queued next
>>   * @num_pending_sgs: counter to pending sgs
>> + * @num_queued_sgs: counter to the number of sgs which already got
>> + queued
>
>this is the same as num_pending_sgs.

num_pending_sgs is initially pointing to num_mapped_sgs, which gets decremented 
in dwc3_cleanup_done_reqs(). Consider a case where the driver failed to queue 
all sgs into TRBs because of insufficient TRB number. For example, lets assume 
req has 5 sgs and only 3 got queued. In this scenario ,when the 
dwc3_cleanup_done_reqs() gets called the value of  num_pending_sgs = 5. Since 
the value of num_pending_sgs is greater than 0, __dwc3_gadget_kick_transfer() 
gets called with num_pending_sgs = 5-1 = 4 . Eventually 
__dwc3_gadget_kick_transfer()  calls dwc3_prepare_one_trb_sg() which has 
for_each_sg() call which loops for num_pending_sgs times (4 times in our 
example). This is incorrect, ideally it should be called only for 2 times 
because we have only 2 sgs which previously were not queued . So, we added an 
extra flag num_queued_sgs which counts the number of sgs that got queued 
successfully and make  for_each_sg() loop for num_mapped_sgs - num_queued_sgs 
times only . In our example case with the updated logic, it will loop for 5 - 3 
= 2 times only. Because of this reason added num_queued_sgs  flag. Please 
correct me if I am wrong.

>
>>   * @remaining: amount of data remaining
>>   * @epnum: endpoint number to which this request refers
>>   * @trb: pointer to struct dwc3_trb
>> @@ -734,8 +736,10 @@ struct dwc3_request {
>> struct list_headlist;
>> struct dwc3_ep  *dep;
>> struct scatterlist  *sg;
>> +   struct scatterlist  *sg_to_start;
>
>indeed, we seem to need something like this.
>
>> unsignednum_pending_sgs;
>> +   unsigned intnum_queued_sgs;
>
>this should be unnecessary.

The explanation given above is valid for this comment also. Please refer the 
explanation given above

>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 2bda4eb..1cffed5 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -978,11 +978,20 @@ static void dwc3_prepare_one_trb(struct dwc3_ep
>*dep,
>> struct dwc3_request *req, unsigned chain, unsigned
>> node)  {
>> struct dwc3_trb *trb;
>> -   unsignedlength = req->request.length;
>> +   unsigned intlength;
>> +   dma_addr_t  dma;
>> unsignedstream_id = req->request.stream_id;
>> unsignedshort_not_ok = req->request.short_not_ok;
>> unsignedno_interrupt = req->request.no_interrupt;
>> -   dma_addr_t  dma = req->request.dma;
>> +
>> +   if (req->request.num_sgs > 0) {
>> +   /* Use scattergather list address and length */
>
>unnecessary comment

Will remove the comment

>
>> +   length = sg_dma_len(req->sg_to_start);
>> +   dma = 

Re: [PATCH] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[kbuild test robot]

Hi Daniel,

I love your patch! Perhaps something to improve:

[auto build test WARNING on v4.16-rc4]
[also build test WARNING on next-20180319]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Daniel-Drake/ACPI-PM-allow-deeper-wakeup-power-states-with-no-_SxD-nor-_SxW/20180319-185209
config: i386-randconfig-x074-201811 (attached as .config)
compiler: gcc-7 (Debian 7.3.0-1) 7.3.0
reproduce:
# save the attached .config to linux build tree
make ARCH=i386 

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

All warnings (new ones prefixed by >>):

   drivers/acpi/device_pm.c: In function 'acpi_dev_pm_get_state':
>> drivers/acpi/device_pm.c:607:7: warning: 'sxd_status' may be used 
>> uninitialized in this function [-Wmaybe-uninitialized]
   if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
  ^

vim +/sxd_status +607 drivers/acpi/device_pm.c

   516  
   517  /**
   518   * acpi_dev_pm_get_state - Get preferred power state of ACPI device.
   519   * @dev: Device whose preferred target power state to return.
   520   * @adev: ACPI device node corresponding to @dev.
   521   * @target_state: System state to match the resultant device state.
   522   * @d_min_p: Location to store the highest power state available to the 
device.
   523   * @d_max_p: Location to store the lowest power state available to the 
device.
   524   *
   525   * Find the lowest power (highest number) and highest power (lowest 
number) ACPI
   526   * device power states that the device can be in while the system is in 
the
   527   * state represented by @target_state.  Store the integer numbers 
representing
   528   * those stats in the memory locations pointed to by @d_max_p and 
@d_min_p,
   529   * respectively.
   530   *
   531   * Callers must ensure that @dev and @adev are valid pointers and that 
@adev
   532   * actually corresponds to @dev before using this function.
   533   *
   534   * Returns 0 on success or -ENODATA when one of the ACPI methods fails 
or
   535   * returns a value that doesn't make sense.  The memory locations 
pointed to by
   536   * @d_max_p and @d_min_p are only modified on success.
   537   */
   538  static int acpi_dev_pm_get_state(struct device *dev, struct acpi_device 
*adev,
   539   u32 target_state, int *d_min_p, int 
*d_max_p)
   540  {
   541  char method[] = { '_', 'S', '0' + target_state, 'D', '\0' };
   542  acpi_handle handle = adev->handle;
   543  unsigned long long ret;
   544  int d_min, d_max;
   545  bool wakeup = false;
   546  acpi_status sxd_status;
   547  acpi_status status;
   548  
   549  /*
   550   * If the system state is S0, the lowest power state the device 
can be
   551   * in is D3cold, unless the device has _S0W and is supposed to 
signal
   552   * wakeup, in which case the return value of _S0W has to be 
used as the
   553   * lowest power state available to the device.
   554   */
   555  d_min = ACPI_STATE_D0;
   556  d_max = ACPI_STATE_D3_COLD;
   557  
   558  /*
   559   * If present, _SxD methods return the minimum D-state (highest 
power
   560   * state) we can use for the corresponding S-states.  
Otherwise, the
   561   * minimum D-state is D0 (ACPI 3.x).
   562   */
   563  if (target_state > ACPI_STATE_S0) {
   564  /*
   565   * We rely on acpi_evaluate_integer() not clobbering 
the integer
   566   * provided if AE_NOT_FOUND is returned.
   567   */
   568  ret = d_min;
   569  sxd_status = acpi_evaluate_integer(handle, method, 
NULL, );
   570  if ((ACPI_FAILURE(sxd_status) && sxd_status != 
AE_NOT_FOUND)
   571  || ret > ACPI_STATE_D3_COLD)
   572  return -ENODATA;
   573  
   574  /*
   575   * We need to handle legacy systems where D3hot and 
D3cold are
   576   * the same and 3 is returned in both cases, so fall 
back to
   577   * D3cold if D3hot is not a valid state.
   578   */
   579  if (!adev->power.states[ret].flags.valid) {
   580  if (ret == ACPI_STATE_D3_HOT)
   581  ret = ACPI_STATE_D3_COLD;
   582  else
   583  return -ENODATA;
   584  }
   585  d_min = ret;
   586  wakeup = device_may_wakeup(dev) && 
adev->wakeup.flags.valid
   587 

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

[Minas Harutyunyan]

Hi,

On 3/19/2018 3:36 PM, Minas Harutyunyan wrote:
> Hi,
> 
> On 3/19/2018 12:55 PM, Felipe Balbi wrote:
>>
>> Hi,
>>
>> Minas Harutyunyan  writes:
 Thanks for picking this for -next.
 Is it better to have this in v4.16-rc fixes?
 and also stable? v4.12+
>>>
>>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit
>>> log ;-)
>>>
>>> The best we can do now, is wait for -rc1 and manually send the commit to
>>> stable.
>>>
>>
>> That's fine. Thanks.
>>
>
> Same issue seen in dwc3_gadget_ep_dequeue() function where also used
> wait_event_lock_irq() - as result infinite loop.

 how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded
 a gadget driver?

>>> No, not during rmmod's.
>>> We using our internal USB testing tool. Test case; ISOC OUT, transfer
>>> size N frames. When host starts ISOC OUT traffic then the dwc3 based on
>>> "Transfer not ready" event in frame F starts transfers staring from
>>> frame F+4 (for bInterval=1) as result 4 requests, which already queued
>>> on device side, remain incomplete. Function driver on some timeout
>>> trying dequeue these 4 requests (without disabling EP) to complete test.
>>> For IN ISOC's these requests completed on MISSED ISOC event, but for
>>> ISOC OUT required call dequeue on some timeout.
>>
>> okay
>>
> Actually to fix this issue I updated condition of wait function
> from:
> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)
> to:
> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED)

 you're not fixing anything. You're, essentially, removing the entire
 end transfer pending logic.
>>> yes, you are right, but how to overcome this infinite loop? Replace
>>> wait_event_lock_irq() by  wait_event_interruptible_lock_irq_timeout()?
>>
>> The best way here would be to figure why we're missing command complete
>> IRQ in those cases. According to documentation, we *should* receive that
>> interrupt, so why is it missing?
>>
> 
> Additional info on test. Core configuration is HS only mode, test speed
> HS, core version v2.90a. Maybe it will help to understand cause of issue.
> BTW, currently to pass above describe ISOC OUT test we just commented
> wait_event_lock_irq() in dwc3_gadget_ep_dequeue() function and
> successfully received request completion in function driver.
> Thanks,
> Minas
> 

One more info: while function driver call dequeue, host periodically 
send control read command to get status of test from function - test In 
Progress or Finished.
Thanks,
Minas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Heikki Krogerus]

Hi Guenter,

On Fri, Mar 16, 2018 at 02:32:06PM -0700, Guenter Roeck wrote:
> On Fri, Mar 09, 2018 at 06:19:18PM +0300, Heikki Krogerus wrote:
> > 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]));
> 
> This can return NULL ...
> 
> > +
> > 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]);
> 
> ... and NULL is not handled by typec_altmode_attention().

True.

> > +   return 0;
> > default:
> > +   if (typec_altmode_vdm(altmode, p[0], [1], cnt) ==
> > +   VDM_OK)
> 
> typec_altmode_vdm() returns old fashioned error 

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

[Heikki Krogerus]

Hi Guenter,

On Fri, Mar 16, 2018 at 02:33:36PM -0700, Guenter Roeck wrote:
> On Fri, Mar 09, 2018 at 06:19:17PM +0300, Heikki Krogerus wrote:
> > diff --git a/drivers/usb/typec/bus.c b/drivers/usb/typec/bus.c
> > new file mode 100644
> > index ..92944aaf3d6a
> > --- /dev/null
> > +++ b/drivers/usb/typec/bus.c
> > @@ -0,0 +1,421 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/**
> > + * Bus for USB Type-C Alternate Modes
> > + *
> > + * Copyright (C) 2018 Intel Corporation
> > + * Author: Heikki Krogerus 
> > + */
> > +
> > +#include "bus.h"
> > +
> > +/* 
> > -- 
> > */
> > +/* Common API */
> > +
> > +/**
> > + * typec_altmode_notify - Communication between the OS and alternate mode 
> > driver
> > + * @adev: Handle to the alternate mode
> > + * @conf: Alternate mode specific configuration value
> > + * @data: Alternate mode specific data
> > + *
> > + * The primary purpose for this function is to allow the alternate mode 
> > drivers
> > + * to tell which pin configuration has been negotiated with the partner. 
> > That
> > + * information will then be used for example to configure the muxes.
> > + * Communication to the other direction is also possible, and low level 
> > device
> > + * drivers can also send notifications to the alternate mode drivers. The 
> > actual
> > + * communication will be specific for every SVID.
> > + */
> > +int typec_altmode_notify(struct typec_altmode *adev,
> > +unsigned long conf, void *data)
> > +{
> > +   struct altmode *altmode;
> > +   struct altmode *partner;
> > +   int ret;
> > +
> > +   /*
> > +* All SVID specific configuration values must start from
> > +* TYPEC_STATE_MODAL. The first values are reserved for the pin states
> > +* defined in USB Type-C specification: TYPEC_STATE_USB and
> > +* TYPEC_STATE_SAFE. We'll follow this rule even with modes that do not
> > +* require pin reconfiguration for the sake of simplicity.
> > +*/
> > +   if (conf < TYPEC_STATE_MODAL)
> > +   return -EINVAL;
> > +
> > +   if (!adev)
> > +   return 0;
> > +
> > +   altmode = to_altmode(adev);
> > +
> > +   if (!altmode->partner)
> > +   return -ENODEV;
> > +
> > +   ret = typec_set_mode(typec_altmode2port(adev), (int)conf);
> > +   if (ret)
> > +   return ret;
> > +
> > +   partner = altmode->partner;
> > +
> > +   blocking_notifier_call_chain(is_typec_port(adev->dev.parent) ?
> > +>nh : >nh,
> > +conf, data);
> > +
> > +   if (partner->ops && partner->ops->notify)
> > +   return partner->ops->notify(>adev, conf, data);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_altmode_notify);
> > +
> > +/**
> > + * typec_altmode_enter - Enter Mode
> > + * @adev: The alternate mode
> > + *
> > + * The alternate mode drivers use this function to enter mode. The port 
> > drivers
> > + * use this to inform the alternate mode drivers that their mode has been
> > + * entered successfully.
> > + */
> > +int typec_altmode_enter(struct typec_altmode *adev)
> > +{
> > +   struct altmode *partner = to_altmode(adev)->partner;
> > +   struct typec_altmode *pdev = >adev;
> > +   int ret;
> > +
> > +   if (is_typec_port(adev->dev.parent)) {
> > +   typec_altmode_update_active(pdev, pdev->mode, true);
> > +   sysfs_notify(>dev.kobj, NULL, "active");
> > +   goto enter_mode;
> > +   }
> > +
> > +   if (!pdev->active)
> > +   return -EPERM;
> > +
> > +   /* First moving to USB Safe State */
> > +   ret = typec_set_mode(typec_altmode2port(adev), TYPEC_STATE_SAFE);
> > +   if (ret)
> > +   return ret;
> > +
> > +   blocking_notifier_call_chain(>nh, TYPEC_STATE_SAFE, NULL);
> > +
> > +enter_mode:
> > +   /* Enter Mode command */
> > +   if (partner->ops && partner->ops->enter)
> > +   partner->ops->enter(pdev);
> > +
> > +   return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(typec_altmode_enter);
> > +
> > +/**
> > + * typec_altmode_enter - Exit Mode
> 
> typec_altmode_exit

Yes.

> > + * @adev: The alternate mode
> > + *
> > + * The alternate mode drivers use this function to exit mode. The port 
> > drivers
> > + * can also inform the alternate mode drivers with this function that the 
> > mode
> > + * was successfully exited.
> > + */
> > +int typec_altmode_exit(struct typec_altmode *adev)
> > +{
> > +   struct altmode *partner = to_altmode(adev)->partner;
> > +   struct typec_port *port = typec_altmode2port(adev);
> > +   struct typec_altmode *pdev = >adev;
> > +   int ret;
> > +
> > +   /* In case of port, just calling the driver and exiting */
> > +   if (is_typec_port(adev->dev.parent)) {
> > +   typec_altmode_update_active(pdev, pdev->mode, false);
> > +   sysfs_notify(>dev.kobj, NULL, "active");
> > +
> > +   if (partner->ops && partner->ops->exit)
> > + 

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

[Minas Harutyunyan]

Hi,

On 3/19/2018 12:55 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Minas Harutyunyan  writes:
>>> Thanks for picking this for -next.
>>> Is it better to have this in v4.16-rc fixes?
>>> and also stable? v4.12+
>>
>> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit
>> log ;-)
>>
>> The best we can do now, is wait for -rc1 and manually send the commit to
>> stable.
>>
>
> That's fine. Thanks.
>

 Same issue seen in dwc3_gadget_ep_dequeue() function where also used
 wait_event_lock_irq() - as result infinite loop.
>>>
>>> how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded
>>> a gadget driver?
>>>
>> No, not during rmmod's.
>> We using our internal USB testing tool. Test case; ISOC OUT, transfer
>> size N frames. When host starts ISOC OUT traffic then the dwc3 based on
>> "Transfer not ready" event in frame F starts transfers staring from
>> frame F+4 (for bInterval=1) as result 4 requests, which already queued
>> on device side, remain incomplete. Function driver on some timeout
>> trying dequeue these 4 requests (without disabling EP) to complete test.
>> For IN ISOC's these requests completed on MISSED ISOC event, but for
>> ISOC OUT required call dequeue on some timeout.
> 
> okay
> 
 Actually to fix this issue I updated condition of wait function
 from:
 !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)
 to:
 !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED)
>>>
>>> you're not fixing anything. You're, essentially, removing the entire
>>> end transfer pending logic.
>> yes, you are right, but how to overcome this infinite loop? Replace
>> wait_event_lock_irq() by  wait_event_interruptible_lock_irq_timeout()?
> 
> The best way here would be to figure why we're missing command complete
> IRQ in those cases. According to documentation, we *should* receive that
> interrupt, so why is it missing?
> 

Additional info on test. Core configuration is HS only mode, test speed 
HS, core version v2.90a. Maybe it will help to understand cause of issue.
BTW, currently to pass above describe ISOC OUT test we just commented 
wait_event_lock_irq() in dwc3_gadget_ep_dequeue() function and 
successfully received request completion in function driver.
Thanks,
Minas
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] staging: typec: rt1711h typec chip driver

[Heikki Krogerus]

On Mon, Mar 19, 2018 at 11:49:34AM +0800, ShuFan Lee wrote:
> From: ShuFan Lee 
> 
> Richtek RT1711H Type-C chip driver that works with
> Type-C Port Controller Manager to provide USB PD and
> USB Type-C functionalities.
> Add definition of TCPC_CC_STATUS_TOGGLING.
> 
> Signed-off-by: ShuFan Lee 

Looks OK to me. I'll put a few nitpics below in case you make one more
version for some other reason. In any case:

Reviewed-by: Heikki Krogerus 

> ---
>  drivers/staging/typec/Kconfig |   8 +
>  drivers/staging/typec/Makefile|   1 +
>  drivers/staging/typec/tcpci.h |   1 +
>  drivers/staging/typec/tcpci_rt1711h.c | 329 
> ++
>  4 files changed, 339 insertions(+)
>  create mode 100644 drivers/staging/typec/tcpci_rt1711h.c
> 
>  changelogs between v1 and v2
>  - use gpiod_* instead of gpio_*
> 
> diff --git a/drivers/staging/typec/Kconfig b/drivers/staging/typec/Kconfig
> index 5359f556d203..3aa981fbc8f5 100644
> --- a/drivers/staging/typec/Kconfig
> +++ b/drivers/staging/typec/Kconfig
> @@ -9,6 +9,14 @@ config TYPEC_TCPCI
>   help
> Type-C Port Controller driver for TCPCI-compliant controller.
>  
> +config TYPEC_RT1711H
> + tristate "Richtek RT1711H Type-C chip driver"
> + select TYPEC_TCPCI
> + help
> +   Richtek RT1711H Type-C chip driver that works with
> +   Type-C Port Controller Manager to provide USB PD and USB
> +   Type-C functionalities.
> +
>  endif
>  
>  endmenu
> diff --git a/drivers/staging/typec/Makefile b/drivers/staging/typec/Makefile
> index 53d649abcb53..7803d485e1b3 100644
> --- a/drivers/staging/typec/Makefile
> +++ b/drivers/staging/typec/Makefile
> @@ -1 +1,2 @@
>  obj-$(CONFIG_TYPEC_TCPCI)+= tcpci.o
> +obj-$(CONFIG_TYPEC_RT1711H)  += tcpci_rt1711h.o
> diff --git a/drivers/staging/typec/tcpci.h b/drivers/staging/typec/tcpci.h
> index 34c865f0dcf6..303ebde26546 100644
> --- a/drivers/staging/typec/tcpci.h
> +++ b/drivers/staging/typec/tcpci.h
> @@ -59,6 +59,7 @@
>  #define TCPC_POWER_CTRL_VCONN_ENABLE BIT(0)
>  
>  #define TCPC_CC_STATUS   0x1d
> +#define TCPC_CC_STATUS_TOGGLING  BIT(5)
>  #define TCPC_CC_STATUS_TERM  BIT(4)
>  #define TCPC_CC_STATUS_CC2_SHIFT 2
>  #define TCPC_CC_STATUS_CC2_MASK  0x3
> diff --git a/drivers/staging/typec/tcpci_rt1711h.c 
> b/drivers/staging/typec/tcpci_rt1711h.c
> new file mode 100644
> index ..12afac363d6d
> --- /dev/null
> +++ b/drivers/staging/typec/tcpci_rt1711h.c
> @@ -0,0 +1,329 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Copyright (C) 2018, Richtek Technology Corporation
> + *
> + * Richtek RT1711H Type-C Chip Driver
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include "tcpci.h"
> +
> +#define RT1711H_RTCTRL8  0x9B
> +
> +/* Autoidle timeout = (tout * 2 + 1) * 6.4ms */
> +#define RT1711H_RTCTRL8_SET(ck300, ship_off, auto_idle, tout) \
> + (((ck300) << 7) | ((ship_off) << 5) | \
> + ((auto_idle) << 3) | ((tout) & 0x07))
> +
> +#define RT1711H_RTCTRL11 0x9E
> +
> +/* I2C timeout = (tout + 1) * 12.5ms */
> +#define RT1711H_RTCTRL11_SET(en, tout) \
> +  (((en) << 7) | ((tout) & 0x0F))
> +
> +#define RT1711H_RTCTRL13 0xA0
> +#define RT1711H_RTCTRL14 0xA1
> +#define RT1711H_RTCTRL15 0xA2
> +#define RT1711H_RTCTRL16 0xA3
> +
> +struct rt1711h_chip {
> + struct tcpci_data data;
> + struct tcpci *tcpci;
> + struct device *dev;
> + int irq;
> +};
> +
> +static int rt1711h_read16(struct rt1711h_chip *chip, unsigned int reg, u16 
> *val)
> +{
> + return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u16));
> +}
> +
> +static int rt1711h_write16(struct rt1711h_chip *chip, unsigned int reg, u16 
> val)
> +{
> + return regmap_raw_write(chip->data.regmap, reg, , sizeof(u16));
> +}
> +
> +static int rt1711h_read8(struct rt1711h_chip *chip, unsigned int reg, u8 
> *val)
> +{
> + return regmap_raw_read(chip->data.regmap, reg, val, sizeof(u8));
> +}
> +
> +static int rt1711h_write8(struct rt1711h_chip *chip, unsigned int reg, u8 
> val)
> +{
> + return regmap_raw_write(chip->data.regmap, reg, , sizeof(u8));
> +}
> +
> +static const struct regmap_config rt1711h_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> +
> + .max_register = 0xFF, /* 0x80 .. 0xFF are vendor defined */
> +};
> +
> +static struct rt1711h_chip *tdata_to_rt1711h(struct tcpci_data *tdata)
> +{
> + return container_of(tdata, struct rt1711h_chip, data);
> +}
> +
> +static int rt1711h_init(struct tcpci *tcpci, struct tcpci_data *tdata)
> +{
> + int ret;
> + struct rt1711h_chip *chip = tdata_to_rt1711h(tdata);
> +
> + /* CK 300K from 320K, shipping off, auto_idle enable, tout = 32ms */
> + ret = 

Re: [PATCH v1 2/3] usb: dwc2: Change ISOC DDMA flow

[Minas Harutyunyan]

Hi Zengtao,

You email not received by linux-usb@vger.kernel.org. I suspect it 
rejected due to your email format have some issues, violate 
linux-usb@vger.kernel.org rules. I think you received email from 
linux-usb similar to this:

: host vger.kernel.org[209.132.180.67] said: 550
 5.7.1 Content-Policy reject msg: Your message is highly unlikely to be
 accepted here BF:; S1751783AbeCPIS1 (in reply to end of DATA 
command)

Kindly request you to correct your email and resend. I will reply you as 
soon as will be received by linux-usb and will be seen archive.
It's important that our communication archived.


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

[PATCH] usb/gadget/uvc-configs Fix host unable to negotiate framesizes other than first

[Joel Pepper]

This adds bFrameIndex as a UVCG_FRAME_ATTR for each frame size.
Beforehand all bFrameindex were set to "1" with no way to configure the 
gadget otherwise.

This resulted in the host always negotiating for bFrameIndex 1 (i.e. the
first framesize of the gadget).
After the negotiation the host driver will set the user- or application-
selected framesize while the gadget is actually set to the first
framesize.

Note that this still requires the gadget to be configured with unique 
'bFrameindex's for each frameSize of each format through configfs. An
alternative might be to automatically assign ascending indices when the
format is linked into the streaming header, but the user space gadget
application would need a way to check or predict the indices so that it
can properly interpret PROBE/COMMIT CONTROL requests.

Signed-off-by: Joel Pepper 
---
 drivers/usb/gadget/function/uvc_configfs.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/gadget/function/uvc_configfs.c 
b/drivers/usb/gadget/function/uvc_configfs.c
index c9b8cc4a..5966d65 100644
--- a/drivers/usb/gadget/function/uvc_configfs.c
+++ b/drivers/usb/gadget/function/uvc_configfs.c
@@ -992,6 +992,8 @@ UVC_ATTR(uvcg_frame_, cname, aname);
 
 UVCG_FRAME_ATTR(bm_capabilities, bmCapabilities, noop_conversion,
noop_conversion, 8);
+UVCG_FRAME_ATTR(b_frame_index, bFrameIndex, noop_conversion,
+   noop_conversion, 8);
 UVCG_FRAME_ATTR(w_width, wWidth, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(w_height, wHeight, le16_to_cpu, cpu_to_le16, 16);
 UVCG_FRAME_ATTR(dw_min_bit_rate, dwMinBitRate, le32_to_cpu, cpu_to_le32, 32);
@@ -1137,6 +1139,7 @@ UVC_ATTR(uvcg_frame_, dw_frame_interval, dwFrameInterval);
 
 static struct configfs_attribute *uvcg_frame_attrs[] = {
_frame_attr_bm_capabilities,
+   _frame_attr_b_frame_index,
_frame_attr_w_width,
_frame_attr_w_height,
_frame_attr_dw_min_bit_rate,
-- 
2.1.4

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

Re: [PATCH v5 1/2] usb/gadget: Add an EP dispose() callback for EP lifetime tracking

[Felipe Balbi]

Hi,

Benjamin Herrenschmidt  writes:
> On Fri, 2018-03-16 at 13:02 +0200, Felipe Balbi wrote:
>> Hi,
>> 
>> Benjamin Herrenschmidt  writes:
>> > Some UDC may want to allocate endpoints dynamically, either because
>> > the HW supports an arbitrary large number or because (like the Aspeed
>> > BMC SoCs), the pool of HW endpoints is shared between multiple gadgets.
>> > 
>> > The allocation side can be done rather easily using the existing
>> > match_ep() UDC hook.
>> > 
>> > However we have no good place to "free" them.
>> > 
>> > This implements a "simple" variant of this, which calls an EP dispose
>> > callback on all EPs associated with a gadget when the composite device
>> > gets unbound.
>> > 
>> > This is required by my upcoming Aspeed vHub driver.
>> > 
>> > Signed-off-by: Benjamin Herrenschmidt 
>> > ---
>> >  drivers/usb/gadget/composite.c | 8 
>> >  include/linux/usb/gadget.h | 1 +
>> >  2 files changed, 9 insertions(+)
>> > 
>> > diff --git a/drivers/usb/gadget/composite.c 
>> > b/drivers/usb/gadget/composite.c
>> > index 77c7ecca816a..ec99c9cfc1a2 100644
>> > --- a/drivers/usb/gadget/composite.c
>> > +++ b/drivers/usb/gadget/composite.c
>> > @@ -2172,6 +2172,7 @@ int composite_os_desc_req_prepare(struct 
>> > usb_composite_dev *cdev,
>> >  void composite_dev_cleanup(struct usb_composite_dev *cdev)
>> >  {
>> >struct usb_gadget_string_container *uc, *tmp;
>> > +  struct usb_ep  *ep, *tmp_ep;
>> >  
>> >list_for_each_entry_safe(uc, tmp, >gstrings, list) {
>> >list_del(>list);
>> > @@ -2193,6 +2194,13 @@ void composite_dev_cleanup(struct usb_composite_dev 
>> > *cdev)
>> >}
>> >cdev->next_string_id = 0;
>> >device_remove_file(>gadget->dev, _attr_suspended);
>> > +
>> > +  /* Dispose EPs if the UDC driver tracks lifetime */
>> > +  list_for_each_entry_safe(ep, tmp_ep,
>> > +   >gadget->ep_list, ep_list) {
>> 
>> do you really need this to be safe? You don't seem to be modifying
>> ep_list here.
>
> Yes, ep->dispose() may do just that. In my Aspeed implementation in
> fact that's pretty much the first thing it does.
>
> IE, I'm returning all the endpoints to the common pool, thus taking
> them out of the gadget EP list.
>
> That said, there could be other reasons why a driver might want to know
> about disposal without actually taking all the EPs back (for example a
> driver could have some dedicated EPs and some in a pool) so I prefer
> the list_del remaining in the back-end.

That seems rather obfuscated. There's no indication that ep_list is
modified from within that iteration block. Seems like it would be best
to make the list_del() explicit and ->dispose() just adds the ep to its
own internal list. No?

-- 
balbi


signature.asc
Description: PGP signature

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

[Adam Thomson]

On 12 March 2018 08:33, Adam Thomson wrote:

> On 09 March 2018 17:34, Greg Kroah-Hartman wrote:
> 
> > 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?
> 
> That's great. Thanks. Need to also rebase based on the patch
> 'Revert "typec: tcpm: Only request matching pdos"' as this impacts the APDO
> selection logic. This is on my TODO list and will try and deal with it ASAP,
> hopefully this week.

One quick question. The patch 'Revert "typec: tcpm: Only request matching pdos"'
from Hans isn't on your next branch. Is that something that will get pulled in
soon? Need this as well as my updates you already pulled into your next branch
before I can resubmit. For now I cherry-picked that patch locally for testing,
but that's no good for submission.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v3 08/12] staging: typec: tcpci: enable vbus detection

[Jun Li]

Hi
> -Original Message-
> From: Guenter Roeck [mailto:groe...@gmail.com] On Behalf Of Guenter
> Roeck
> Sent: 2018年3月15日 12:47
> To: Jun Li 
> Cc: robh...@kernel.org; mark.rutl...@arm.com;
> gre...@linuxfoundation.org; heikki.kroge...@linux.intel.com;
> a.ha...@samsung.com; yue...@google.com; shufan_...@richtek.com;
> o_leve...@orange.fr; linux-usb@vger.kernel.org; dl-linux-imx
> 
> Subject: Re: [PATCH v3 08/12] staging: typec: tcpci: enable vbus detection
> 
> On Tue, Mar 13, 2018 at 05:34:34PM +0800, Li Jun wrote:
> > TCPCI implementation may need SW to enable VBUS detection to generate
> > power status events.
> >
> > Signed-off-by: Li Jun 
> 
> Makes sense to me. Only question might be if this should be dones before
> checking the power status at the beginnng of the function. Any thoughts ?
> 
Per spec, that power status checking is to make sure the tcpc has completed
Initialization and all registers are valid, I think it may not safe to issue 
tcpc
command before that.

Thanks
Jun

> Otherwise
> 
> Reviewed-by: Guenter Roeck 
> 
> > ---
> >  drivers/staging/typec/tcpci.c | 6 ++
> >  1 file changed, 6 insertions(+)
> >
> > diff --git a/drivers/staging/typec/tcpci.c
> > b/drivers/staging/typec/tcpci.c index f5a3bf5..9a230c6 100644
> > --- a/drivers/staging/typec/tcpci.c
> > +++ b/drivers/staging/typec/tcpci.c
> > @@ -373,6 +373,12 @@ static int tcpci_init(struct tcpc_dev *tcpc)
> > if (ret < 0)
> > return ret;
> >
> > +   /* Enable Vbus detection */
> > +   ret = regmap_write(tcpci->regmap, TCPC_COMMAND,
> > +  TCPC_CMD_ENABLE_VBUS_DETECT);
> > +   if (ret < 0)
> > +   return ret;
> > +
> > reg = TCPC_ALERT_TX_SUCCESS | TCPC_ALERT_TX_FAILED |
> > TCPC_ALERT_TX_DISCARDED | TCPC_ALERT_RX_STATUS |
> > TCPC_ALERT_RX_HARD_RST | TCPC_ALERT_CC_STATUS;
> > --
> > 2.7.4
> >

Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink variable pdo

[Hans de Goede]

HI,

On 19-03-18 10:43, Jun Li wrote:

Hi


-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年3月14日 18:39
To: Jun Li ; gre...@linuxfoundation.org; li...@roeck-us.net;
heikki.kroge...@linux.intel.com; adam.thomson.opensou...@diasemi.com;
bad...@google.com
Cc: linux-usb@vger.kernel.org; dl-linux-imx 
Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink
variable pdo

Hi,

On 14-03-18 10:32, Jun Li wrote:

Hi

-Original Message-
From: Hans de Goede [mailto:hdego...@redhat.com]
Sent: 2018年3月13日 19:36
To: Jun Li ; gre...@linuxfoundation.org;
li...@roeck-us.net; heikki.kroge...@linux.intel.com;
adam.thomson.opensou...@diasemi.com;
bad...@google.com
Cc: linux-usb@vger.kernel.org; dl-linux-imx 
Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo
with sink variable pdo

Hi,

On 13-03-18 01:02, Li Jun wrote:

This patch tries to fix the problem describled on revert patch
commit[1], suppose any supported voltage ranges of sink should be
describled by variable pdo, so instead of revert the patch of only
comparing source and sink pdo to select one source pdo, this patch
adds the match between source fixed pdo and sink variable pdo.

[1]




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

w

.spinics.net%2Flists%2Flinux-usb%2Fmsg166366.html=02%7C01%7

Cju

n.l





i%40nxp.com%7C569be5e2496442f514bd08d588d69fc6%7C686ea1d3bc2b4

c6fa92cd





99c5c301635%7C0%7C0%7C636565377744465803=hUPib1nNtXArpZ

Pn0TfMdui

ARnVPvc6CezTr0UxJFCI%3D=0

CC: Hans de Goede 
CC: Guenter Roeck 
CC: Heikki Krogerus 
CC: Adam Thomson 
CC: Badhri Jagan Sridharan 
Signed-off-by: Li Jun 
---
drivers/usb/typec/tcpm.c | 31 +++
1 file changed, 31 insertions(+)

diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
index
ef3a60d..8a74a43 100644
--- a/drivers/usb/typec/tcpm.c
+++ b/drivers/usb/typec/tcpm.c
@@ -1815,6 +1815,37 @@ static int tcpm_pd_select_pdo(struct
tcpm_port

*port, int *sink_pdo,

break;
}
}
+
+   /*
+* If the source pdo has the same voltage with one
+* fixed pdo, no need check variable pdo for it.
+*/
+   if (j < port->nr_fixed)
+   continue;
+
+   for (j = port->nr_fixed +
+port->nr_batt;
+j < port->nr_fixed +
+port->nr_batt +
+port->nr_var;
+j++) {
+   if (pdo_fixed_voltage(pdo) >=
+pdo_min_voltage(port->snk_pdo[j]) &&
+pdo_fixed_voltage(pdo) <=
+pdo_max_voltage(port->snk_pdo[j])) {
+   ma = min_current(pdo, port->snk_pdo[j]);
+   mv = pdo_fixed_voltage(pdo);
+   mw = ma * mv / 1000;
+   if (mw > max_mw ||
+   (mw == max_mw && mv > max_mv)) {
+   ret = 0;
+   *src_pdo = i;
+   *sink_pdo = j;
+   max_mw = mw;
+   max_mv = mv;
+   }
+   }
+   }
} else if (type == PDO_TYPE_BATT) {
for (j = port->nr_fixed;
 j < port->nr_fixed +



First of all, this seems to be a fix on top of your previous, reverted patch.



It's on top of the patch to be reverted by you.


Since your patch has been reverted, please send a new fixed patch replacing

it.




It's Badhri's patch, not mine.

Author: Badhri Jagan Sridharan 
Date:   Wed Nov 15 17:01:56 2017 -0800

  typec: tcpm: Only request matching pdos


As for the proposed fix, you are just fixing the fixed source,
variable snk cap case here. What if things are the other way around,
so fixed snk, variable source?


I think that may not work, as the sink defines itself only can work at
one fixed voltage, a variable PDO can make sure the output voltage fixed?

Below is the description of variable supply PDO from PD spec:
"The Variable Supply (non-Battery) PDO exposes very poorly regulated

Sources.

The output voltage of a Variable Supply 

RE: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink variable pdo

[Jun Li]

Hi

> -Original Message-
> From: Hans de Goede [mailto:hdego...@redhat.com]
> Sent: 2018年3月14日 18:39
> To: Jun Li ; gre...@linuxfoundation.org; li...@roeck-us.net;
> heikki.kroge...@linux.intel.com; adam.thomson.opensou...@diasemi.com;
> bad...@google.com
> Cc: linux-usb@vger.kernel.org; dl-linux-imx 
> Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo with sink
> variable pdo
> 
> Hi,
> 
> On 14-03-18 10:32, Jun Li wrote:
> > Hi
> >> -Original Message-
> >> From: Hans de Goede [mailto:hdego...@redhat.com]
> >> Sent: 2018年3月13日 19:36
> >> To: Jun Li ; gre...@linuxfoundation.org;
> >> li...@roeck-us.net; heikki.kroge...@linux.intel.com;
> >> adam.thomson.opensou...@diasemi.com;
> >> bad...@google.com
> >> Cc: linux-usb@vger.kernel.org; dl-linux-imx 
> >> Subject: Re: [RFC PATCH] usb: typec: tcpm: match source fixed pdo
> >> with sink variable pdo
> >>
> >> Hi,
> >>
> >> On 13-03-18 01:02, Li Jun wrote:
> >>> This patch tries to fix the problem describled on revert patch
> >>> commit[1], suppose any supported voltage ranges of sink should be
> >>> describled by variable pdo, so instead of revert the patch of only
> >>> comparing source and sink pdo to select one source pdo, this patch
> >>> adds the match between source fixed pdo and sink variable pdo.
> >>>
> >>> [1]
> >>>
> >>
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> >> w
> >>> .spinics.net%2Flists%2Flinux-usb%2Fmsg166366.html=02%7C01%7
> Cju
> >> n.l
> >>>
> >>
> i%40nxp.com%7C569be5e2496442f514bd08d588d69fc6%7C686ea1d3bc2b4
> >> c6fa92cd
> >>>
> >>
> 99c5c301635%7C0%7C0%7C636565377744465803=hUPib1nNtXArpZ
> >> Pn0TfMdui
> >>> ARnVPvc6CezTr0UxJFCI%3D=0
> >>>
> >>> CC: Hans de Goede 
> >>> CC: Guenter Roeck 
> >>> CC: Heikki Krogerus 
> >>> CC: Adam Thomson 
> >>> CC: Badhri Jagan Sridharan 
> >>> Signed-off-by: Li Jun 
> >>> ---
> >>>drivers/usb/typec/tcpm.c | 31 +++
> >>>1 file changed, 31 insertions(+)
> >>>
> >>> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> >>> index
> >>> ef3a60d..8a74a43 100644
> >>> --- a/drivers/usb/typec/tcpm.c
> >>> +++ b/drivers/usb/typec/tcpm.c
> >>> @@ -1815,6 +1815,37 @@ static int tcpm_pd_select_pdo(struct
> >>> tcpm_port
> >> *port, int *sink_pdo,
> >>>   break;
> >>>   }
> >>>   }
> >>> +
> >>> + /*
> >>> +  * If the source pdo has the same voltage with one
> >>> +  * fixed pdo, no need check variable pdo for it.
> >>> +  */
> >>> + if (j < port->nr_fixed)
> >>> + continue;
> >>> +
> >>> + for (j = port->nr_fixed +
> >>> +  port->nr_batt;
> >>> +  j < port->nr_fixed +
> >>> +  port->nr_batt +
> >>> +  port->nr_var;
> >>> +  j++) {
> >>> + if (pdo_fixed_voltage(pdo) >=
> >>> +  pdo_min_voltage(port->snk_pdo[j]) &&
> >>> +  pdo_fixed_voltage(pdo) <=
> >>> +  pdo_max_voltage(port->snk_pdo[j])) {
> >>> + ma = min_current(pdo, port->snk_pdo[j]);
> >>> + mv = pdo_fixed_voltage(pdo);
> >>> + mw = ma * mv / 1000;
> >>> + if (mw > max_mw ||
> >>> + (mw == max_mw && mv > max_mv)) {
> >>> + ret = 0;
> >>> + *src_pdo = i;
> >>> + *sink_pdo = j;
> >>> + max_mw = mw;
> >>> + max_mv = mv;
> >>> + }
> >>> + }
> >>> + }
> >>>   } else if (type == PDO_TYPE_BATT) {
> >>>   for (j = port->nr_fixed;
> >>>j < port->nr_fixed +
> >>>
> >>
> >> First of all, this seems to be a fix on top of your previous, reverted 
> >> patch.
> >>
> >
> > It's on top of the patch to be reverted by you.
> >
> >> Since your patch has been reverted, please send a new fixed patch replacing
> it.
> >>
> >
> > It's Badhri's patch, not mine.
> >
> > Author: Badhri Jagan Sridharan 
> > Date:   Wed Nov 15 17:01:56 2017 -0800
> >
> >  typec: tcpm: Only request matching pdos
> >
> >> As for the proposed fix, you are just fixing the fixed source,
> >> variable snk cap case here. What if things 

RE: [PATCH v3 04/12] usb: typec: add API to get typec basic port power and data config

[Jun Li]

Hi
> 
> I been thinking about this. Perhaps it's better that we don't read any
> properties in these helpers. These helpers become more useful that way, and
> we can use the in other places as well if needed.
> 
> So my proposal is that the callers of these functions are responsible of 
> reading
> the property values. Here we just convert the strings to
> values:
> 
> int typec_find_preferred_role(const char *name) {
>   return match_string(typec_roles, ARRAY_SIZE(typec_roles), name); }
> EXPORT_SYMBOL_GPL(typec_find_preferred_role);
> 
> int typec_find_power_role(const char *name) {
>   return match_string(typec_port_types, ARRAY_SIZE(typec_data_types),
>   name);
> }
> EXPORT_SYMBOL_GPL(typec_find_data_role);
> 
> int typec_find_data_role(const char *name) {
>   return match_string(typec_data_types, ARRAY_SIZE(typec_data_types),
>   name);
> }
> EXPORT_SYMBOL_GPL(typec_find_data_role);
> 

A better idea for basic typec class, thanks, I will change to be this proposal 
in v4.

Jun
> 
> 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  http://vger.kernel.org/majordomo-info.html

RE: About patchwork for linux-usb

[Peter Chen]

> >
> > At some situations, we may need to save reviewing patches from web,
> > but I can't find linux-usb at
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatchwork.k
> ernel.org%2F=02%7C01%7Cpeter.chen%40nxp.com%7Cbef6ada7885641ed5
> bf408d58b131139%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C6365
> 67836369109099=w8dVZjbsb%2BrboggllBzJbdkxR6r7BQHOEtf7tQ5mCPE%
> 3D=0,  any there any patchworks for linux-usb mailist at Internet? 
> Thanks.
> 
> I do not know of any, as I do not use patchwork, sorry.  If you feel that we 
> should add
> it to patchwork.kernel.org, that can easily be done.
> 
> thanks,
> 

Some may would like to test the reviewing patches, but they do not have the 
tool like
mutt to save patches, it is useful for them.  It will be great we would do 
that, thanks.

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

Re: [PATCH v2] usb: dwc3: pci: Properly cleanup resource

[Felipe Balbi]

Hi,

Thinh Nguyen  writes:
> Platform device is allocated before adding resources. Make sure to
> properly cleanup on error case.
>
> Fixes: f1c7e7108109 ("usb: dwc3: convert to pcim_enable_device()")

this commit is from way back in 3.16, please resend with a "Cc:
" tag before this fixes line.

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Minas Harutyunyan  writes:
>> Thanks for picking this for -next.
>> Is it better to have this in v4.16-rc fixes?
>> and also stable? v4.12+
>
> Well, there was no "Fixes: foobar" or "Cc: stable" lines in the commit
> log ;-)
>
> The best we can do now, is wait for -rc1 and manually send the commit to
> stable.
>

 That's fine. Thanks.

>>>
>>> Same issue seen in dwc3_gadget_ep_dequeue() function where also used
>>> wait_event_lock_irq() - as result infinite loop.
>> 
>> how did this happen? During rmmod dwc3? Or, perhaps, after you unloaded
>> a gadget driver?
>> 
> No, not during rmmod's.
> We using our internal USB testing tool. Test case; ISOC OUT, transfer 
> size N frames. When host starts ISOC OUT traffic then the dwc3 based on 
> "Transfer not ready" event in frame F starts transfers staring from 
> frame F+4 (for bInterval=1) as result 4 requests, which already queued 
> on device side, remain incomplete. Function driver on some timeout 
> trying dequeue these 4 requests (without disabling EP) to complete test.
> For IN ISOC's these requests completed on MISSED ISOC event, but for 
> ISOC OUT required call dequeue on some timeout.

okay

>>> Actually to fix this issue I updated condition of wait function
>>> from:
>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING)
>>> to:
>>> !(dep->flags & DWC3_EP_END_TRANSFER_PENDING & DWC3_EP_ENABLED)
>> 
>> you're not fixing anything. You're, essentially, removing the entire
>> end transfer pending logic. 
> yes, you are right, but how to overcome this infinite loop? Replace 
> wait_event_lock_irq() by  wait_event_interruptible_lock_irq_timeout()?

The best way here would be to figure why we're missing command complete
IRQ in those cases. According to documentation, we *should* receive that
interrupt, so why is it missing?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH usb-next v10 3/8] usb: core: add a wrapper for the USB PHYs on the HCD

[Roger Quadros]

Hi,

On 19/03/18 00:29, Martin Blumenstingl wrote:
> Hi Roger,
> 
> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros  wrote:
>> +some TI folks
>>
>> Hi Martin,
>>
>> On 18/02/18 20:44, Martin Blumenstingl wrote:
>>> Many SoC platforms have separate devices for the USB PHY which are
>>> registered through the generic PHY framework. These PHYs have to be
>>> enabled to make the USB controller actually work. They also have to be
>>> disabled again on shutdown/suspend.
>>>
>>> Currently (at least) the following HCI platform drivers are using custom
>>> code to obtain all PHYs via devicetree for the roothub/controller and
>>> disable/enable them when required:
>>> - ehci-platform.c has ehci_platform_power_{on,off}
>>> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
>>> - ohci-platform.c has ohci_platform_power_{on,off}
>>>
>>> With this new wrapper the USB PHYs can be specified directly in the
>>> USB controller's devicetree node (just like on the drivers listed
>>> above). This allows SoCs like the Amlogic Meson GXL family to operate
>>> correctly once this is wired up correctly. These SoCs use a dwc3
>>> controller and require all USB PHYs to be initialized (if one of the USB
>>> PHYs it not initialized then none of USB port works at all).
>>>
>>> Signed-off-by: Martin Blumenstingl 
>>> Tested-by: Yixun Lan 
>>> Cc: Neil Armstrong 
>>> Cc: Chunfeng Yun 
>>
>> This patch is breaking low power cases on TI SoCs when USB is in host mode.
>> I'll explain why below.
> based on your explanation and reading the TI PHY drivers I am assuming
> that the affected SoCs are using the "phy-omap-usb2" driver
> 
yes and phy-ti-pipe3 as well i.e. "ti,phy-usb3" and "ti,omap-usb3"

>>> ---
>>>  drivers/usb/core/Makefile |   2 +-
>>>  drivers/usb/core/phy.c| 158 
>>> ++
>>>  drivers/usb/core/phy.h|   7 ++
>>>  3 files changed, 166 insertions(+), 1 deletion(-)
>>>  create mode 100644 drivers/usb/core/phy.c
>>>  create mode 100644 drivers/usb/core/phy.h
>>>
>>> diff --git a/drivers/usb/core/Makefile b/drivers/usb/core/Makefile
>>> index 92c9cefb4317..18e874b0441e 100644
>>> --- a/drivers/usb/core/Makefile
>>> +++ b/drivers/usb/core/Makefile
>>> @@ -6,7 +6,7 @@
>>>  usbcore-y := usb.o hub.o hcd.o urb.o message.o driver.o
>>>  usbcore-y += config.o file.o buffer.o sysfs.o endpoint.o
>>>  usbcore-y += devio.o notify.o generic.o quirks.o devices.o
>>> -usbcore-y += port.o
>>> +usbcore-y += phy.o port.o
>>>
>>>  usbcore-$(CONFIG_OF) += of.o
>>>  usbcore-$(CONFIG_USB_PCI)+= hcd-pci.o
>>> diff --git a/drivers/usb/core/phy.c b/drivers/usb/core/phy.c
>>> new file mode 100644
>>> index ..09b7c43c0ea4
>>> --- /dev/null
>>> +++ b/drivers/usb/core/phy.c
>>> @@ -0,0 +1,158 @@
>>> +// SPDX-License-Identifier: GPL-2.0+
>>> +/*
>>> + * A wrapper for multiple PHYs which passes all phy_* function calls to
>>> + * multiple (actual) PHY devices. This is comes handy when initializing
>>> + * all PHYs on a HCD and to keep them all in the same state.
>>> + *
>>> + * Copyright (C) 2018 Martin Blumenstingl 
>>> 
>>> + */
>>> +
>>> +#include 
>>> +#include 
>>> +#include 
>>> +#include 
>>> +
>>> +#include "phy.h"
>>> +
>>> +struct usb_phy_roothub {
>>> + struct phy  *phy;
>>> + struct list_headlist;
>>> +};
>>> +
>>> +static struct usb_phy_roothub *usb_phy_roothub_alloc(struct device *dev)
>>> +{
>>> + struct usb_phy_roothub *roothub_entry;
>>> +
>>> + roothub_entry = devm_kzalloc(dev, sizeof(*roothub_entry), GFP_KERNEL);
>>> + if (!roothub_entry)
>>> + return ERR_PTR(-ENOMEM);
>>> +
>>> + INIT_LIST_HEAD(_entry->list);
>>> +
>>> + return roothub_entry;
>>> +}
>>> +
>>> +static int usb_phy_roothub_add_phy(struct device *dev, int index,
>>> +struct list_head *list)
>>> +{
>>> + struct usb_phy_roothub *roothub_entry;
>>> + struct phy *phy = devm_of_phy_get_by_index(dev, dev->of_node, index);
>>> +
>>> + if (IS_ERR_OR_NULL(phy)) {
>>> + if (!phy || PTR_ERR(phy) == -ENODEV)
>>> + return 0;
>>> + else
>>> + return PTR_ERR(phy);
>>> + }
>>> +
>>> + roothub_entry = usb_phy_roothub_alloc(dev);
>>> + if (IS_ERR(roothub_entry))
>>> + return PTR_ERR(roothub_entry);
>>> +
>>> + roothub_entry->phy = phy;
>>> +
>>> + list_add_tail(_entry->list, list);
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
>>> +{
>>> + struct usb_phy_roothub *phy_roothub;
>>> + struct usb_phy_roothub *roothub_entry;
>>> + struct list_head *head;
>>> + int i, num_phys, err;
>>> +
>>> + num_phys = of_count_phandle_with_args(dev->of_node, "phys",
>>> +

Re: [PATCH] usb: dwc3: gadget: Correct the logic for queuing sgs

[Felipe Balbi]

Hi,

Anurag Kumar Vulisha  writes:
> This patch fixes two issues
>
> 1. The code logic in dwc3_prepare_one_trb() incorrectly uses the address
> and length given in req packet even for scattergather lists. This patch
> correct's the code to use sg->address and sg->length when scattergather
> lists are present.
>
> 2. The present code correctly fetches the req's which were not queued from
> the started_list but fails to start from the sg where it previously stopped
> queuing because of the unavailable TRB's. This patch correct's the code to
> start queuing from the correct sg in sglist.

these two should be in separate patches, then.

> Signed-off-by: Anurag Kumar Vulisha 
> ---
>  drivers/usb/dwc3/core.h   |  4 
>  drivers/usb/dwc3/gadget.c | 42 --
>  2 files changed, 40 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 860d2bc..2779e58 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -718,7 +718,9 @@ struct dwc3_hwparams {
>   * @list: a list_head used for request queueing
>   * @dep: struct dwc3_ep owning this request
>   * @sg: pointer to first incomplete sg
> + * @sg_to_start: pointer to the sg which should be queued next
>   * @num_pending_sgs: counter to pending sgs
> + * @num_queued_sgs: counter to the number of sgs which already got queued

this is the same as num_pending_sgs.

>   * @remaining: amount of data remaining
>   * @epnum: endpoint number to which this request refers
>   * @trb: pointer to struct dwc3_trb
> @@ -734,8 +736,10 @@ struct dwc3_request {
> struct list_headlist;
> struct dwc3_ep  *dep;
> struct scatterlist  *sg;
> +   struct scatterlist  *sg_to_start;

indeed, we seem to need something like this.

> unsignednum_pending_sgs;
> +   unsigned intnum_queued_sgs;

this should be unnecessary.

> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 2bda4eb..1cffed5 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -978,11 +978,20 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
> struct dwc3_request *req, unsigned chain, unsigned node)
>  {
> struct dwc3_trb *trb;
> -   unsignedlength = req->request.length;
> +   unsigned intlength;
> +   dma_addr_t  dma;
> unsignedstream_id = req->request.stream_id;
> unsignedshort_not_ok = req->request.short_not_ok;
> unsignedno_interrupt = req->request.no_interrupt;
> -   dma_addr_t  dma = req->request.dma;
> +
> +   if (req->request.num_sgs > 0) {
> +   /* Use scattergather list address and length */

unnecessary comment

> +   length = sg_dma_len(req->sg_to_start);
> +   dma = sg_dma_address(req->sg_to_start);
> +   } else {
> +   length = req->request.length;
> +   dma = req->request.dma;
> +   }
>
> trb = >trb_pool[dep->trb_enqueue];
>
> @@ -1048,11 +1057,14 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
>  static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
> struct dwc3_request *req)
>  {
> -   struct scatterlist *sg = req->sg;
> +   struct scatterlist *sg = req->sg_to_start;
> struct scatterlist *s;
> int i;
>
> -   for_each_sg(sg, s, req->num_pending_sgs, i) {
> +   unsigned int remaining = req->request.num_mapped_sgs
> +   - req->num_queued_sgs;

already tracked as num_pending_sgs

> +   for_each_sg(sg, s, remaining, i) {
> unsigned int length = req->request.length;
> unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
> unsigned int rem = length % maxp;
> @@ -1081,6 +1093,16 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep 
> *dep,
> dwc3_prepare_one_trb(dep, req, chain, i);
> }
>
> +   /* In the case where not able to queue trbs for all sgs in

wrong comment style

> +* request because of trb not available, update sg_to_start
> +* to next sg from which we can start queing trbs once trbs
> +* availbale
 ^
 available

sg_to_start is too long and awkward to type. Sure, I'm nitpicking, but
start_sg would be better.

> +*/
> +   if (chain)
> +   req->sg_to_start = sg_next(s);
> +
> +   req->num_queued_sgs++;
> +
> if (!dwc3_calc_trbs_left(dep))
> break;
> }
> @@ -1171,6 +1193,8 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
> return;
>
> 

Re: [PATCH 8/9] usb: host: xhci-rcar: add support for r8a77965

[Mathias Nyman]

On 16.03.2018 16:38, Greg KH wrote:

On Fri, Mar 16, 2018 at 04:33:05PM +0200, Mathias Nyman wrote:

From: Yoshihiro Shimoda 

This patch adds support for r8a77965 (R-Car M3-N).

Signed-off-by: Yoshihiro Shimoda 
Reviewed-by: Rob Herring 
Signed-off-by: Mathias Nyman 
---
  Documentation/devicetree/bindings/usb/usb-xhci.txt | 1 +
  drivers/usb/host/xhci-rcar.c   | 4 
  2 files changed, 5 insertions(+)


This is already in Linus's tree, right?



Ah, right, so it is.
It apparently got picked via usb-linus already, didn't notice it as I
had my set based on usb-next

I saw you took the reset of the set, 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

[PATCH] ACPI / PM: allow deeper wakeup power states with no _SxD nor _SxW

[Daniel Drake]

acpi_dev_pm_get_state() is used to determine the range of allowable
device power states when going into S3 suspend. This is implemented
by executing the _S3D and _S3W ACPI methods.

Linux follows the ACPI spec behaviour in that when _S3D is implemented
and _S3W is not, Linux will not go into a power state deeper than the one
returned by _S3D for a wakeup-enabled device.

However, this same logic is being applied to the case when neither
_S3D nor _S3W are present, and the result is that this function
decides that the device must stay in D0 (fully on) state.

This is breaking USB wakeups on Asus V222GA and Acer XC-830. _S3D and
_S3W are not present, so the USB controller is left in the D0 running
state during S3, and hence it is unable to generate a PME# wake event.

The ACPI spec is unclear on which power states are permissable for
wakeup-enabled devices when both _S3D and _S3W are missing.
However, USB wakeups work fine on these platforms under Windows, where
device manager shows that they are using D3 device state for the USB
controller in S3.

I assume that the "max = min" clamping done by the code here is
specifically written for the _S3D but no _S3W case. By making the
code true to those conditions, avoiding them on these platforms,
the controller will be put into D3 state and USB wakeups start working.

Additionally I feel that this change makes the code more directly
mirror the wording of the ACPI spec and it's associated lack of clarity.

Thanks to Mathias Nyman for pointing us in the right direction.

Signed-off-by: Daniel Drake 
Link: 
http://lkml.kernel.org/r/CAB4CAwf_k-WsF3zL4epm9TKAOu0h=bv1xhxv_gy3bzioo_n...@mail.gmail.com
---
 drivers/acpi/device_pm.c | 11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/acpi/device_pm.c b/drivers/acpi/device_pm.c
index c4d0a1c912f0..b945e37bcac0 100644
--- a/drivers/acpi/device_pm.c
+++ b/drivers/acpi/device_pm.c
@@ -543,6 +543,7 @@ static int acpi_dev_pm_get_state(struct device *dev, struct 
acpi_device *adev,
unsigned long long ret;
int d_min, d_max;
bool wakeup = false;
+   acpi_status sxd_status;
acpi_status status;
 
/*
@@ -565,8 +566,8 @@ static int acpi_dev_pm_get_state(struct device *dev, struct 
acpi_device *adev,
 * provided if AE_NOT_FOUND is returned.
 */
ret = d_min;
-   status = acpi_evaluate_integer(handle, method, NULL, );
-   if ((ACPI_FAILURE(status) && status != AE_NOT_FOUND)
+   sxd_status = acpi_evaluate_integer(handle, method, NULL, );
+   if ((ACPI_FAILURE(sxd_status) && sxd_status != AE_NOT_FOUND)
|| ret > ACPI_STATE_D3_COLD)
return -ENODATA;
 
@@ -599,7 +600,11 @@ static int acpi_dev_pm_get_state(struct device *dev, 
struct acpi_device *adev,
method[3] = 'W';
status = acpi_evaluate_integer(handle, method, NULL, );
if (status == AE_NOT_FOUND) {
-   if (target_state > ACPI_STATE_S0)
+   /* No _SxW. In this case, the ACPI spec says that we
+* must not go into any power state deeper than the
+* value returned from _SxD.
+*/
+   if (sxd_status == AE_OK && target_state > ACPI_STATE_S0)
d_max = d_min;
} else if (ACPI_SUCCESS(status) && ret <= ACPI_STATE_D3_COLD) {
/* Fall back to D3cold if ret is not a valid state. */
-- 
2.14.1

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

[PATCH 1/1] usb: musb: gadget: misplaced out of bounds check

[Heinrich Schuchardt]

musb->endpoints[] has array size MUSB_C_NUM_EPS.
We must check array bounds before accessing the array and not afterwards.

Signed-off-by: Heinrich Schuchardt 
---
 drivers/usb/musb/musb_gadget_ep0.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_gadget_ep0.c 
b/drivers/usb/musb/musb_gadget_ep0.c
index 18da4873e52e..482e7c2f8dc7 100644
--- a/drivers/usb/musb/musb_gadget_ep0.c
+++ b/drivers/usb/musb/musb_gadget_ep0.c
@@ -88,6 +88,11 @@ static int service_tx_status_request(
break;
}
 
+   if (epnum >= MUSB_C_NUM_EPS) {
+   handled = -EINVAL;
+   break;
+   }
+
is_in = epnum & USB_DIR_IN;
if (is_in) {
epnum &= 0x0f;
@@ -97,7 +102,7 @@ static int service_tx_status_request(
}
regs = musb->endpoints[epnum].regs;
 
-   if (epnum >= MUSB_C_NUM_EPS || !ep->desc) {
+   if (!ep->desc) {
handled = -EINVAL;
break;
}
-- 
2.16.2

--
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: dwc3: gadget: Correct the logic for queuing sgs

[Anurag Kumar Vulisha]

This patch fixes two issues

1. The code logic in dwc3_prepare_one_trb() incorrectly uses the address
and length given in req packet even for scattergather lists. This patch
correct's the code to use sg->address and sg->length when scattergather
lists are present.

2. The present code correctly fetches the req's which were not queued from
the started_list but fails to start from the sg where it previously stopped
queuing because of the unavailable TRB's. This patch correct's the code to
start queuing from the correct sg in sglist.

Signed-off-by: Anurag Kumar Vulisha 
---
 drivers/usb/dwc3/core.h   |  4 
 drivers/usb/dwc3/gadget.c | 42 --
 2 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
index 860d2bc..2779e58 100644
--- a/drivers/usb/dwc3/core.h
+++ b/drivers/usb/dwc3/core.h
@@ -718,7 +718,9 @@ struct dwc3_hwparams {
  * @list: a list_head used for request queueing
  * @dep: struct dwc3_ep owning this request
  * @sg: pointer to first incomplete sg
+ * @sg_to_start: pointer to the sg which should be queued next
  * @num_pending_sgs: counter to pending sgs
+ * @num_queued_sgs: counter to the number of sgs which already got queued
  * @remaining: amount of data remaining
  * @epnum: endpoint number to which this request refers
  * @trb: pointer to struct dwc3_trb
@@ -734,8 +736,10 @@ struct dwc3_request {
struct list_headlist;
struct dwc3_ep  *dep;
struct scatterlist  *sg;
+   struct scatterlist  *sg_to_start;

unsignednum_pending_sgs;
+   unsigned intnum_queued_sgs;
unsignedremaining;
u8  epnum;
struct dwc3_trb *trb;
diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 2bda4eb..1cffed5 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -978,11 +978,20 @@ static void dwc3_prepare_one_trb(struct dwc3_ep *dep,
struct dwc3_request *req, unsigned chain, unsigned node)
 {
struct dwc3_trb *trb;
-   unsignedlength = req->request.length;
+   unsigned intlength;
+   dma_addr_t  dma;
unsignedstream_id = req->request.stream_id;
unsignedshort_not_ok = req->request.short_not_ok;
unsignedno_interrupt = req->request.no_interrupt;
-   dma_addr_t  dma = req->request.dma;
+
+   if (req->request.num_sgs > 0) {
+   /* Use scattergather list address and length */
+   length = sg_dma_len(req->sg_to_start);
+   dma = sg_dma_address(req->sg_to_start);
+   } else {
+   length = req->request.length;
+   dma = req->request.dma;
+   }

trb = >trb_pool[dep->trb_enqueue];

@@ -1048,11 +1057,14 @@ static u32 dwc3_calc_trbs_left(struct dwc3_ep *dep)
 static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
struct dwc3_request *req)
 {
-   struct scatterlist *sg = req->sg;
+   struct scatterlist *sg = req->sg_to_start;
struct scatterlist *s;
int i;

-   for_each_sg(sg, s, req->num_pending_sgs, i) {
+   unsigned int remaining = req->request.num_mapped_sgs
+   - req->num_queued_sgs;
+
+   for_each_sg(sg, s, remaining, i) {
unsigned int length = req->request.length;
unsigned int maxp = usb_endpoint_maxp(dep->endpoint.desc);
unsigned int rem = length % maxp;
@@ -1081,6 +1093,16 @@ static void dwc3_prepare_one_trb_sg(struct dwc3_ep *dep,
dwc3_prepare_one_trb(dep, req, chain, i);
}

+   /* In the case where not able to queue trbs for all sgs in
+* request because of trb not available, update sg_to_start
+* to next sg from which we can start queing trbs once trbs
+* availbale
+*/
+   if (chain)
+   req->sg_to_start = sg_next(s);
+
+   req->num_queued_sgs++;
+
if (!dwc3_calc_trbs_left(dep))
break;
}
@@ -1171,6 +1193,8 @@ static void dwc3_prepare_trbs(struct dwc3_ep *dep)
return;

req->sg = req->request.sg;
+   req->sg_to_start= req->sg;
+   req->num_queued_sgs = 0;
req->num_pending_sgs= req->request.num_mapped_sgs;

if (req->num_pending_sgs > 0)
@@ -2327,8 +2351,14 @@ static int dwc3_cleanup_done_reqs(struct dwc3 *dwc, 
struct dwc3_ep *dep,

req->request.actual = length - req->remaining;

-   if ((req->request.actual < length) && req->num_pending_sgs)
-   return 

Re: [PATCH usb-next v10 3/8] usb: core: add a wrapper for the USB PHYs on the HCD

[Chunfeng Yun]

Hi,
On Sun, 2018-03-18 at 23:29 +0100, Martin Blumenstingl wrote:
> Hi Roger,
> 
> On Fri, Mar 16, 2018 at 3:32 PM, Roger Quadros  wrote:
> > +some TI folks
> >
> > Hi Martin,
> >
> > On 18/02/18 20:44, Martin Blumenstingl wrote:
> >> Many SoC platforms have separate devices for the USB PHY which are
> >> registered through the generic PHY framework. These PHYs have to be
> >> enabled to make the USB controller actually work. They also have to be
> >> disabled again on shutdown/suspend.
> >>
> >> Currently (at least) the following HCI platform drivers are using custom
> >> code to obtain all PHYs via devicetree for the roothub/controller and
> >> disable/enable them when required:
> >> - ehci-platform.c has ehci_platform_power_{on,off}
> >> - xhci-mtk.c has xhci_mtk_phy_{init,exit,power_on,power_off}
> >> - ohci-platform.c has ohci_platform_power_{on,off}
> >>
> >> With this new wrapper the USB PHYs can be specified directly in the
> >> USB controller's devicetree node (just like on the drivers listed
> >> above). This allows SoCs like the Amlogic Meson GXL family to operate
> >> correctly once this is wired up correctly. These SoCs use a dwc3
> >> controller and require all USB PHYs to be initialized (if one of the USB
> >> PHYs it not initialized then none of USB port works at all).
> >>
> >> Signed-off-by: Martin Blumenstingl 
> >> Tested-by: Yixun Lan 
> >> Cc: Neil Armstrong 
> >> Cc: Chunfeng Yun 
> >
> > This patch is breaking low power cases on TI SoCs when USB is in host mode.
> > I'll explain why below.
> based on your explanation and reading the TI PHY drivers I am assuming
> that the affected SoCs are using the "phy-omap-usb2" driver
> 
[...]
> >> +
> >> +struct usb_phy_roothub *usb_phy_roothub_init(struct device *dev)
> >> +{
> >> + struct usb_phy_roothub *phy_roothub;
> >> + struct usb_phy_roothub *roothub_entry;
> >> + struct list_head *head;
> >> + int i, num_phys, err;
> >> +
> >> + num_phys = of_count_phandle_with_args(dev->of_node, "phys",
> >> +   "#phy-cells");
> >> + if (num_phys <= 0)
> >> + return NULL;
> >> +
> >> + phy_roothub = usb_phy_roothub_alloc(dev);
> >> + if (IS_ERR(phy_roothub))
> >> + return phy_roothub;
> >> +
> >> + for (i = 0; i < num_phys; i++) {
> >> + err = usb_phy_roothub_add_phy(dev, i, _roothub->list);
> >> + if (err)
> >> + goto err_out;
> >> + }
> >> +
> >> + head = _roothub->list;
> >> +
> >> + list_for_each_entry(roothub_entry, head, list) {
> >> + err = phy_init(roothub_entry->phy);
> >
> > The phy_init() function actually enables the PHY clocks.
> > It should be moved to the usb_phy_roothub_exit() routine just before 
> > calling phy_power_on().
> do you mean that phy_init should be moved to usb_phy_roothub_power_on
> (just before phy_power_on is called within usb_phy_roothub_power_on)?
> 
> an earlier version of my patch did exactly this, but it caused
> problems during a suspend/resume cycle on Mediatek devices
> Chunfeng Yun reported that issue here [0], quote from that mail for
> easier reading:
> "In order to keep link state on mt8173, we just power off all phys(not
> exit) when system enter suspend, then power on them again (needn't
> init, otherwise device will be disconnected) when system resume, this
> can avoid re-enumerating device."
> 
> >> + if (err)
> >> + goto err_exit_phys;
> >> + }
> >> +
> >> + return phy_roothub;
> >> +
> >> +err_exit_phys:
> >> + list_for_each_entry_continue_reverse(roothub_entry, head, list)
> >> + phy_exit(roothub_entry->phy);
> >> +
> >> +err_out:
> >> + return ERR_PTR(err);
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_init);
> >> +
> >> +int usb_phy_roothub_exit(struct usb_phy_roothub *phy_roothub)
> >> +{
> >> + struct usb_phy_roothub *roothub_entry;
> >> + struct list_head *head;
> >> + int err, ret = 0;
> >> +
> >> + if (!phy_roothub)
> >> + return 0;
> >> +
> >> + head = _roothub->list;
> >> +
> >> + list_for_each_entry(roothub_entry, head, list) {
> >> + err = phy_exit(roothub_entry->phy);
> >> + if (err)
> >> + ret = ret;
> >> + }
> >
> > phy_exit() should be moved to usb_phy_roothub_poweroff() just after calling 
> > phy_power_off().
> if I understood Chunfeng Yun correctly this will require
> re-enumeration of the USB devices after a suspend/resume cycle on
> Mediatek SoCs
You are right
> 
> > With that there is nothing else being done here. Shouldn't we be doing the 
> > equivalent of
> > usb_phy_roothub_del_phy() and usb_phy_roothub_free() here?
> >
> >> +
> >> + return ret;
> >> +}
> >> +EXPORT_SYMBOL_GPL(usb_phy_roothub_exit);
> >> +
> >> +int