RE: [PATCH v8 08/14] usb: otg: add OTG/dual-role core

[Jun Li]

> -Original Message-
> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> Sent: Wednesday, May 25, 2016 10:44 AM
> To: Roger Quadros 
> Cc: peter.c...@freescale.com; ba...@kernel.org; t...@atomide.com;
> gre...@linuxfoundation.org; dan.j.willi...@intel.com;
> mathias.ny...@linux.intel.com; joao.pi...@synopsys.com;
> sergei.shtyl...@cogentembedded.com; jun...@freescale.com;
> grygorii.stras...@ti.com; yoshihiro.shimoda...@renesas.com;
> r...@kernel.org; nsek...@ti.com; b-...@ti.com; linux-usb@vger.kernel.org;
> linux-o...@vger.kernel.org; linux-ker...@vger.kernel.org;
> devicet...@vger.kernel.org
> Subject: Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core
> 
> On Tue, May 24, 2016 at 12:45:46PM +0300, Roger Quadros wrote:
> > Hi Peter,
> >
> > I have one question here. Please see below.
> >
> > On 13/05/16 13:03, Roger Quadros wrote:
> > > It provides APIs for the following tasks
> > >
> > > - Registering an OTG/dual-role capable controller
> > > - Registering Host and Gadget controllers to OTG core
> > > - Providing inputs to and kicking the OTG state machine
> > >
> > > Provide a dual-role device (DRD) state machine.
> > > DRD mode is a reduced functionality OTG mode. In this mode we don't
> > > support SRP, HNP and dynamic role-swap.
> > >
> > > In DRD operation, the controller mode (Host or Peripheral) is
> > > decided based on the ID pin status. Once a cable plug (Type-A or
> > > Type-B) is attached the controller selects the state and doesn't
> > > change till the cable in unplugged and a different cable type is
> > > inserted.
> > >
> > > As we don't need most of the complex OTG states and OTG timers we
> > > implement a lean DRD state machine in usb-otg.c.
> > > The DRD state machine is only interested in 2 hardware inputs 'id'
> > > and 'b_sess_vld'.
> > >
> > > Signed-off-by: Roger Quadros 
> > > ---
> > >  drivers/usb/common/Makefile  |2 +-
> > >  drivers/usb/common/usb-otg.c | 1042
> ++
> > >  drivers/usb/core/Kconfig |4 +-
> > >  include/linux/usb/gadget.h   |2 +
> > >  include/linux/usb/hcd.h  |1 +
> > >  include/linux/usb/otg-fsm.h  |7 +
> > >  include/linux/usb/otg.h  |  154 ++-
> > >  7 files changed, 1206 insertions(+), 6 deletions(-)  create mode
> > > 100644 drivers/usb/common/usb-otg.c
> > >
> >
> > 
> >
> > > +
> > > +/**
> > > + * usb_otg_register() - Register the OTG/dual-role device to OTG
> > > +core
> > > + * @dev: OTG/dual-role controller device.
> > > + * @config: OTG configuration.
> > > + *
> > > + * Registers the OTG/dual-role controller device with the USB OTG
> core.
> > > + *
> > > + * Return: struct usb_otg * if success, ERR_PTR() if error.
> > > + */
> > > +struct usb_otg *usb_otg_register(struct device *dev,
> > > +  struct usb_otg_config *config) {
> > > + struct usb_otg *otg;
> > > + struct otg_wait_data *wait;
> > > + int ret = 0;
> > > +
> > > + if (!dev || !config || !config->fsm_ops)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + /* already in list? */
> > > + mutex_lock(_list_mutex);
> > > + if (usb_otg_get_data(dev)) {
> > > + dev_err(dev, "otg: %s: device already in otg list\n",
> > > + __func__);
> > > + ret = -EINVAL;
> > > + goto unlock;
> > > + }
> > > +
> > > + /* allocate and add to list */
> > > + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > > + if (!otg) {
> > > + ret = -ENOMEM;
> > > + goto unlock;
> > > + }
> > > +
> > > + otg->dev = dev;
> > > + otg->caps = config->otg_caps;
> >
> > Here, we should be checking if user needs to disable any OTG features.
> > So,
> >
> > if (dev->of_node)
> > of_usb_update_otg_caps(dev->of_node, >caps);
> >
> > Do you agree?
> > This means we need to change otg->caps from 'struct usb_otg_caps *caps;'
> > to 'struct usb_otg_caps caps;' so that we can modify the local copy
> > instead of the one passed by the OTG controller.
> 
> Why can't modify the one from OTG controller directly?

Yes, if user wants to disable any OTG features, this should have been
done in 'config->otg_caps', if not, 'config->otg_caps' from controller
driver is invalid and making no sense.

> 
> >
> > We can also move of_usb_update_otg_caps() to otg.h.
> >
> > We will also need to modify the udc-core code so that it sets
> > gadget->otg_caps to the modified otg_caps from the OTG core. This will
> > ensure that the right OTG descriptors are sent.
> >
> > So we will have to introduce an API.
> >
> > struct usb_otg_caps *usb_otg_get_otg_caps(struct device *otg_dev)
> >
> > And in udc-core.c,
> >
> > static int udc_bind_to_driver(struct usb_udc *udc, struct
> > usb_gadget_driver *driver) { ..
> > ret = driver->bind(udc->gadget, driver);
> > if (ret)
> > goto err1;
> >
> > /* If OTG, the otg core starts the UDC when needed */
> > if (udc->gadget->otg_dev) {
> > +   

Re: [PATCH v8 08/14] usb: otg: add OTG/dual-role core

[Peter Chen]

On Tue, May 24, 2016 at 12:45:46PM +0300, Roger Quadros wrote:
> Hi Peter,
> 
> I have one question here. Please see below.
> 
> On 13/05/16 13:03, Roger Quadros wrote:
> > It provides APIs for the following tasks
> > 
> > - Registering an OTG/dual-role capable controller
> > - Registering Host and Gadget controllers to OTG core
> > - Providing inputs to and kicking the OTG state machine
> > 
> > Provide a dual-role device (DRD) state machine.
> > DRD mode is a reduced functionality OTG mode. In this mode
> > we don't support SRP, HNP and dynamic role-swap.
> > 
> > In DRD operation, the controller mode (Host or Peripheral)
> > is decided based on the ID pin status. Once a cable plug (Type-A
> > or Type-B) is attached the controller selects the state
> > and doesn't change till the cable in unplugged and a different
> > cable type is inserted.
> > 
> > As we don't need most of the complex OTG states and OTG timers
> > we implement a lean DRD state machine in usb-otg.c.
> > The DRD state machine is only interested in 2 hardware inputs
> > 'id' and 'b_sess_vld'.
> > 
> > Signed-off-by: Roger Quadros 
> > ---
> >  drivers/usb/common/Makefile  |2 +-
> >  drivers/usb/common/usb-otg.c | 1042 
> > ++
> >  drivers/usb/core/Kconfig |4 +-
> >  include/linux/usb/gadget.h   |2 +
> >  include/linux/usb/hcd.h  |1 +
> >  include/linux/usb/otg-fsm.h  |7 +
> >  include/linux/usb/otg.h  |  154 ++-
> >  7 files changed, 1206 insertions(+), 6 deletions(-)
> >  create mode 100644 drivers/usb/common/usb-otg.c
> > 
> 
> 
> 
> > +
> > +/**
> > + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> > + * @dev: OTG/dual-role controller device.
> > + * @config: OTG configuration.
> > + *
> > + * Registers the OTG/dual-role controller device with the USB OTG core.
> > + *
> > + * Return: struct usb_otg * if success, ERR_PTR() if error.
> > + */
> > +struct usb_otg *usb_otg_register(struct device *dev,
> > +struct usb_otg_config *config)
> > +{
> > +   struct usb_otg *otg;
> > +   struct otg_wait_data *wait;
> > +   int ret = 0;
> > +
> > +   if (!dev || !config || !config->fsm_ops)
> > +   return ERR_PTR(-EINVAL);
> > +
> > +   /* already in list? */
> > +   mutex_lock(_list_mutex);
> > +   if (usb_otg_get_data(dev)) {
> > +   dev_err(dev, "otg: %s: device already in otg list\n",
> > +   __func__);
> > +   ret = -EINVAL;
> > +   goto unlock;
> > +   }
> > +
> > +   /* allocate and add to list */
> > +   otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> > +   if (!otg) {
> > +   ret = -ENOMEM;
> > +   goto unlock;
> > +   }
> > +
> > +   otg->dev = dev;
> > +   otg->caps = config->otg_caps;
> 
> Here, we should be checking if user needs to disable any OTG features. So,
> 
>   if (dev->of_node)
>   of_usb_update_otg_caps(dev->of_node, >caps);
> 
> Do you agree?
> This means we need to change otg->caps from 'struct usb_otg_caps *caps;'
> to 'struct usb_otg_caps caps;' so that we can modify the local copy instead
> of the one passed by the OTG controller.

Why can't modify the one from OTG controller directly?

> 
> We can also move of_usb_update_otg_caps() to otg.h.
> 
> We will also need to modify the udc-core code so that it sets gadget->otg_caps
> to the modified otg_caps from the OTG core. This will ensure that the right
> OTG descriptors are sent.
> 
> So we will have to introduce an API.
> 
> struct usb_otg_caps *usb_otg_get_otg_caps(struct device *otg_dev)
> 
> And in udc-core.c,
> 
> static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver 
> *driver)
> {
> ..
> ret = driver->bind(udc->gadget, driver);
> if (ret)
> goto err1;
> 
> /* If OTG, the otg core starts the UDC when needed */
> if (udc->gadget->otg_dev) {
> + udc->gadget->is_otg = true;

gadget->is_otg is only set to true if fully OTG is supported and it
needs to send OTG descriptors at this case. DRD devices should not send OTG
descriptors.

> + udc->gadget->otg_caps = 
> usb_otg_get_otg_caps(udc->gadget->otg_dev);

Getting otg capabilities should be prior to driver->bind since
usb_otg_descriptor_init is called at that. Besides, Gadget driver
may be probed before otg driver is registered

I am wonder if we can implement defer probe for gadget/udc/host driver
if otg driver is not probed, in that case, some designs can be simpler
like wait list in otg driver.

> mutex_unlock(_lock);
> usb_otg_register_gadget(udc->gadget, _gadget_intf);
> mutex_lock(_lock);
> } else {
> ret = usb_gadget_udc_start(udc);
> if (ret) {
> driver->unbind(udc->gadget);
> goto err1;
> }
> usb_udc_connect_control(udc);
> }
> ..
> 

Re: [PATCH V3 RESEND] phy: bcm-ns-usb3: new driver for USB 3.0 PHY on Northstar

[Scott Branden]

Hi Rafal,

some comments in line.

On 16-05-22 03:09 PM, Rafał Miłecki wrote:
> Northstar is a family of SoCs used in home routers. They have USB 2.0
> and 3.0 controllers with PHYs that need to be properly initialized.
> This driver provides PHY init support in a generic way and can be bound
> with XHCI controller driver.
> This patch adds 2 defines in bcma header that will be reused in bcma
> driver. Using common header will allow avoiding code duplication.
>
> Signed-off-by: Rafał Miłecki 
> ---
> V2: Redesign the driver to don't depend on bcma. This will allow 
reusing it on
>  Northstar+ which doesn't use bcma. This requires providing two 
different

>  registers ranges in DT which was documented in bindings info.
> V3: Use readl and writel
>  Fix MODULE_LICENSE to match header (v2)
>  Mention C0 series in Documentation
> RESEND: I can see that my PHY driver for USB 2.0:
> [PATCH V4] phy: bcm-ns-usb2: new driver for USB 2.0 PHY on Northstar
> sent on 14 Apr 2016 was accepted, however:
> [PATCH V3] phy: bcm-ns-usb3: new driver for USB 3.0 PHY on Northstar
> sent the very same day wasn't.
> I'm sending a simply rebased version hoping it can be accepted or
> commented somehow (e.g. what needs to be changed).
> ---
>   .../devicetree/bindings/phy/bcm-ns-usb3-phy.txt|  23 ++
>   drivers/phy/Kconfig|   9 +
>   drivers/phy/Makefile   |   1 +
>   drivers/phy/phy-bcm-ns-usb3.c  | 267 
+

>   include/linux/bcma/bcma_driver_chipcommon.h|   3 +
>   5 files changed, 303 insertions(+)
>   create mode 100644 
Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt

>   create mode 100644 drivers/phy/phy-bcm-ns-usb3.c
>
> diff --git 
a/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt 
b/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt

> new file mode 100644
> index 000..09aeba9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/phy/bcm-ns-usb3-phy.txt
> @@ -0,0 +1,23 @@
> +Driver for Broadcom Northstar USB 3.0 PHY
> +
> +Required properties:
> +
> +- compatible: one of: "brcm,ns-ax-usb3-phy", "brcm,ns-bx-usb3-phy".
> +- reg: register mappings for DMP (Device Management Plugin) and 
ChipCommon B

> +   MMI.
> +- reg-names: "dmp" and "ccb-mii"
> +
> +Initialization of USB 3.0 PHY depends on Northstar version. There 
are currently

> +three known series: Ax, Bx and Cx.
> +Known A0: BCM4707 rev 0
> +Known B0: BCM4707 rev 4, BCM53573 rev 2
> +Known B1: BCM4707 rev 6
> +Known C0: BCM47094 rev 0
> +
> +Example:
> +usb3-phy {
> +compatible = "brcm,ns-ax-usb3-phy";
> +reg = <0x18105000 0x1000>, <0x18003000 0x1000>;
> +reg-names = "dmp", "ccb-mii";
> +#phy-cells = <0>;
> +};
> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
> index f2b458f..6967398 100644
> --- a/drivers/phy/Kconfig
> +++ b/drivers/phy/Kconfig
> @@ -24,6 +24,15 @@ config PHY_BCM_NS_USB2
> Enable this to support Broadcom USB 2.0 PHY connected to the USB
> controller on Northstar family.
>
> +config PHY_BCM_NS_USB3
> +tristate "Broadcom Northstar USB 3.0 PHY Driver"
> +depends on ARCH_BCM_IPROC || COMPILE_TEST
> +depends on HAS_IOMEM && OF
> +select GENERIC_PHY
> +help
> +  Enable this to support Broadcom USB 3.0 PHY connected to the USB
> +  controller on Northstar family.
> +
>   config PHY_BERLIN_USB
>   tristate "Marvell Berlin USB PHY Driver"
>   depends on ARCH_BERLIN && RESET_CONTROLLER && HAS_IOMEM && OF
> diff --git a/drivers/phy/Makefile b/drivers/phy/Makefile
> index 0de09e1..fa17ae3 100644
> --- a/drivers/phy/Makefile
> +++ b/drivers/phy/Makefile
> @@ -4,6 +4,7 @@
>
>   obj-$(CONFIG_GENERIC_PHY)+= phy-core.o
>   obj-$(CONFIG_PHY_BCM_NS_USB2)+= phy-bcm-ns-usb2.o
> +obj-$(CONFIG_PHY_BCM_NS_USB3)+= phy-bcm-ns-usb3.o
>   obj-$(CONFIG_PHY_BERLIN_USB)+= phy-berlin-usb.o
>   obj-$(CONFIG_PHY_BERLIN_SATA)+= phy-berlin-sata.o
>   obj-$(CONFIG_PHY_DM816X_USB)+= phy-dm816x-usb.o
> diff --git a/drivers/phy/phy-bcm-ns-usb3.c 
b/drivers/phy/phy-bcm-ns-usb3.c

> new file mode 100644
> index 000..869bc20
> --- /dev/null
> +++ b/drivers/phy/phy-bcm-ns-usb3.c
> @@ -0,0 +1,267 @@
> +/*
> + * Broadcom Northstar USB 3.0 PHY Driver
> + *
> + * Copyright (C) 2016 Rafał Miłecki 
My thought is this code must have originated from Broadcom source code. 
 Where is the copyright notice/license from the original code?

> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + *
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define BCM_NS_USB3_MII_MNG_TIMEOUT_US1000/* usecs */
> +
> 

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Guenter Roeck]

On Thu, May 19, 2016 at 03:44:54PM +0300, Heikki Krogerus wrote:
> The purpose of this class is to provide unified interface for user
> space to get the status and basic information about USB Type-C
> Connectors in the system, control data role swapping, and when USB PD
> is available, also power role swapping and Alternate Modes.
> 
> Signed-off-by: Heikki Krogerus 

[ ... ]

> +
> +static void typec_remove_partner(struct typec_port *port)
> +{
> + sysfs_remove_link(>dev.kobj, "partner");
> + typec_unregister_altmodes(port->partner->alt_modes);

This only unregisters alternate modes registered through typec_add_partner(),
but not alternate modes registered separately. Or is the calling code expected
to set port->partner->alt_modes when calling typec_register_altmodes()
directly ?

[ ... ]

> +
> +void typec_unregister_altmodes(struct typec_altmode *alt_modes)
> +{
> + struct typec_altmode *alt;
> +
This will crash if alt_modes is NULL, which will happen if 
partner->alt_modes is NULL at connection time. Semantically
this is different to typec_register_altmodes(), which does
have a NULL check.

Guenter
--
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] dwc3-exynos: Fix deferred probing storm.

[Steinar H. Gunderson]

dwc3-exynos has two problems during init if the regulators are slow
to come up (for instance if the I2C bus driver is not on the initramfs)
and return probe deferral. First, every time this happens, the driver
leaks the USB phys created; they need to be deallocated on error.

Second, since the phy devices are created before the regulators fail,
this means that there's a new device to re-trigger deferred probing,
which causes it to essentially go into a busy loop of re-probing the
device until the regulators come up.

Move the phy creation to after the regulators have succeeded, and also
fix cleanup on failure. On my ODROID XU4 system (with Debian's initramfs
which doesn't contain the I2C driver), this reduces the number of probe
attempts (for each of the two controllers) from more than 2000 to eight.

Reported-by: Steinar H. Gunderson 
Signed-off-by: Steinar H. Gunderson 
---
 drivers/usb/dwc3/dwc3-exynos.c | 19 +++
 1 file changed, 11 insertions(+), 8 deletions(-)

diff --git a/drivers/usb/dwc3/dwc3-exynos.c b/drivers/usb/dwc3/dwc3-exynos.c
index dd5cb55..2f1fb7e 100644
--- a/drivers/usb/dwc3/dwc3-exynos.c
+++ b/drivers/usb/dwc3/dwc3-exynos.c
@@ -128,12 +128,6 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
 
platform_set_drvdata(pdev, exynos);
 
-   ret = dwc3_exynos_register_phys(exynos);
-   if (ret) {
-   dev_err(dev, "couldn't register PHYs\n");
-   return ret;
-   }
-
exynos->dev = dev;
 
exynos->clk = devm_clk_get(dev, "usbdrd30");
@@ -183,20 +177,29 @@ static int dwc3_exynos_probe(struct platform_device *pdev)
goto err3;
}
 
+   ret = dwc3_exynos_register_phys(exynos);
+   if (ret) {
+   dev_err(dev, "couldn't register PHYs\n");
+   goto err4;
+   }
+
if (node) {
ret = of_platform_populate(node, NULL, NULL, dev);
if (ret) {
dev_err(dev, "failed to add dwc3 core\n");
-   goto err4;
+   goto err5;
}
} else {
dev_err(dev, "no device node, failed to add dwc3 core\n");
ret = -ENODEV;
-   goto err4;
+   goto err5;
}
 
return 0;
 
+err5:
+   platform_device_unregister(exynos->usb2_phy);
+   platform_device_unregister(exynos->usb3_phy);
 err4:
regulator_disable(exynos->vdd10);
 err3:
-- 
2.8.0.rc3.226.g39d4020

--
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] Re: Endless "supply vcc not found, using dummy regulator"

[Steinar H. Gunderson]

On Tue, May 24, 2016 at 05:53:34PM +0200, Krzysztof Kozlowski wrote:
> exynos->clk = devm_clk_get(dev, "usbdrd30");
> if (IS_ERR(exynos->clk)) {
> + // On each error path since here we need to
> + // revert work done by dwc3_exynos_register_phys()
> dev_err(dev, "couldn't get clock\n");
> return -EINVAL;
> }
> clk_prepare_enable(exynos->clk);

OK, so I took Mark's advice and moved the phy instantiation towards the end
of the function (after the regulators have successfully come up). It reduced
the number of probes, even with the original initramfs, dramatically, so
it seems to work quite well. It also reduces the text for each deferred probe
by a lot, since we no longer have the dummy regulator message for each one
(only the message about “no suspend clk specified” is left). Finally, this
arrangement reduced the need for extra error handling to a minimum.

Cc-ing Felipe and and linux-usb@, and adding the patch as a reply to this
message.

/* Steinar */
-- 
Homepage: https://www.sesse.net/
--
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] usb: gadget: Introduce Cadence USB2 UDC Driver

[Laurent Pinchart]

Hi Neil,

On Tuesday 24 May 2016 16:16:16 Neil Armstrong wrote:
> Hi Felipe, Laurent,
> 
> I submitted this driver for a Cadence IP library from a now abandoned
> project.
> 
> The driver was working on a SoC platform ported on a FPGA, but I do not have
> access to this HW anymore.
> 
> But I have a fixed version...
> 
> Now, I am looking for SoCs, vendors and even Cadence people (or even
> Evatronix employees who worked on this IP before being in Cadence
> portfolio) to push this driver upstream by testing it on a real HW or even
> FPGA.
> 
> I hope this message in a bottle will get read !

I've read it, but what do you expect from me ? :-)

-- 
Regards,

Laurent Pinchart

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

AW: ffs-test fails with warning (-19) No such device

[Jan.Huber]

> You shouldn't use epX where X != 0 until you get enable event. You may
> refer to ffs-aio examples which have a suitable flag to wait for that event.

I got the ffs-aio example (simple and multibuff) to work.
I also used the ffs-aio example as a reference and tried to get the ffs-test 
example to work. So I am waiting for both events (Event BIND, Event ENABLE) 
before accessing epX where X != 0.
However I still get the same error (-19) No such device when using ffs-test 
example.

For testing I added a read()-operation to the working ffs-aio-example to make 
sure the endpoint descriptors are not faulty in ffs-test. Also here, I get the 
same error (-19).

>From my understanding, FunctionFS is the successor of GadgetFS. It provides a 
>file for each endpoint (ep0, ep1, ep2, ...). The endpoints can then be 
>accessed with read() / write() operations - please correct me if I'm wrong.

Questions remaining:
1) Why does the ffs-test example not work even when waiting for Event 
BIND/Event ENABLE before accessing epX where X != 0?
2) Is it not possible to access endpoints with read()/write()
3) if 2) is not possible, how did ffs-test ever work? How can the endpoints be 
accessed now?

> In addition I'd recommend you using libusbgx[1] instead of direct
> configfs manipulation. There is even an example which sets usb gadget
> with two instances of functionfs[2]
> 
> Footnotes:
> 1 - https://github.com/libusbgx/libusbgx
> 2 - https://github.com/libusbgx/libusbgx/blob/master/examples/gadget-ffs.c

>From my understanding using libusbgx can be used to instead of using command 
>in the CLI. Thanks for this recommendation, will definitely use it. But for 
>first testing I am meanwhile okay with setting things up manually.


Best regards

Jan Huber



> I'm trying to establish a high performance USB connection from device/gadget 
> to host. On the USB gadget side I'm running on a Atmel ATSAMA5D35-EK. The 
> Linux Kernel version is 4.1 (4.1.0-linux4sam_5.2-00045-g633e08a) and I'm 
> using configfs to set things up.
>
> I strictly followed the steps according to the presentations from the 
> Embedded Linux Conference 2014 (by Alan Ott and Matt Porter)
>
> Here is what I did on the USB device side so far:
>
> # insmod atmel_usba_udc.ko
> # insmod usb_f_fs.ko
>
> # mkdir -p /home/root/configfs
> # mount -t configfs none /home/root/configfs
> # mkdir -p /home/root/configfs/usb_gadget/g1
>
> # echo "0x1a0a" > /home/root/configfs/usb_gadget/g1/idVendor
> # echo "0xbadd" > /home/root/configfs/usb_gadget/g1/idProduct
>
> # mkdir -p /home/root/configfs/usb_gadget/g1/strings/0x409
> # echo "1234" > /home/root/configfs/usb_gadget/g1/strings/0x409/serialnumber
> # echo "manufacturer" > 
> /home/root/configfs/usb_gadget/g1/strings/0x409/manufacturer
> # echo "product" > /home/root/configfs/usb_gadget/g1/strings/0x409/product
>
> # mkdir -p /home/root/configfs/usb_gadget/g1/configs/c.1
> # mkdir -p /home/root/configfs/usb_gadget/g1/configs/c.1/strings/0x409
> # echo "Config1" > 
> /home/root/configfs/usb_gadget/g1/configs/c.1/strings/0x409/configuration
>
> # mkdir -p /home/root/configfs/usb_gadget/g1/functions/ffs.usb0
> # ln -s /home/root/configfs/usb_gadget/g1/functions/ffs.usb0 
> /home/root/configfs/usb_gadget/g1/configs/c.1
>
> # mkdir -p /home/root/ffs
> # mount usb0 /home/root/ffs -t functionfs
>
> # cd /home/root/ffs
> # ../ffs-test 64 &
>
> Setting up the device and functionfs works perfectly fine, but when executing 
> the ffs-test I get the following log messages:
>
> ffs-test: info: ep0: writing descriptors (in v2 format)
> ffs-test: info: ep0: writing strings
> ffs-test: dbg:  ep1: starting
> ffs-test: dbg:  ep2: starting
> ffs-test: info: ep0: starts
> ffs-test: info: ep2: starts
> ffs-test: info: ep1: starts
> ffs-test: warn: ep1: write: (-19) No such device
> ffs-test: warn: ep2: read: (-19) No such device
> ffs-test: info: ep1: ends
> ffs-test: info: ep2: ends
> Event BIND
> Event ENABLE
>

You shouldn't use epX where X != 0 until you get enable event. You may
refer to ffs-aio examples which have a suitable flag to wait for that event.

In addition I'd recommend you using libusbgx[1] instead of direct
configfs manipulation. There is even an example which sets usb gadget
with two instances of functionfs[2]

Footnotes:
1 - https://github.com/libusbgx/libusbgx
2 - https://github.com/libusbgx/libusbgx/blob/master/examples/gadget-ffs.c

Best regards,
--
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics--
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] usb: gadget: Introduce Cadence USB2 UDC Driver

[Felipe Balbi]

Hi,

(please break lines at 80-columns and don't top-post ;)

Neil Armstrong  writes:
> [ Unknown signature status ]
> Hi Felipe, Laurent,
>
> I submitted this driver for a Cadence IP library from a now abandoned
> project.
>
> The driver was working on a SoC platform ported on a FPGA, but I do
> not have access to this HW anymore.
>
> But I have a fixed version...
>
> Now, I am looking for SoCs, vendors and even Cadence people (or even
> Evatronix employees who worked on this IP before being in Cadence
> portfolio) to push this driver upstream by testing it on a real HW or
> even FPGA.
>
> I hope this message in a bottle will get read !

if you manage to get it tested, send it to linux-usb ;)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: core: add debugobjects support for urb object

[Alan Stern]

On Tue, 24 May 2016 changbin...@intel.com wrote:

> From: "Du, Changbin" 
> 
> Add debugobject support to track the life time of struct urb.
> This feature help us detect violation of urb operations by
> generating a warning message from debugobject core.

I'm pretty sure the USB core already does this tracking.  If it
doesn't, why not make the appropriate changes instead of adding a whole
new infrastructure?

>  And we fix
> the possible issues at runtime to avoid oops if we can.

This is questionable.  Under what conditions do you think you can fix
up a possible issue?

> I have done some tests with some class drivers, no violation
> found in them which is good. Expect this feature can be used
> for debugging future problems.
> 
> Signed-off-by: Du, Changbin 
> ---
>  drivers/usb/core/hcd.c |   1 +
>  drivers/usb/core/urb.c | 117 
> +++--
>  include/linux/usb.h|   8 
>  lib/Kconfig.debug  |   8 
>  4 files changed, 130 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
> index 34b837a..a8ea128 100644
> --- a/drivers/usb/core/hcd.c
> +++ b/drivers/usb/core/hcd.c
> @@ -1750,6 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
>   /* pass ownership to the completion handler */
>   urb->status = status;
>  
> + debug_urb_deactivate(urb);
>   /*
>* We disable local IRQs here avoid possible deadlock because
>* drivers may call spin_lock() to hold lock which might be

> diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
> index c601e25..0d1eccb 100644
> --- a/drivers/usb/core/urb.c
> +++ b/drivers/usb/core/urb.c
> @@ -10,6 +10,100 @@
>  
>  #define to_urb(d) container_of(d, struct urb, kref)
>  
> +#ifdef CONFIG_DEBUG_OBJECTS_URB
> +static struct debug_obj_descr urb_debug_descr;
> +
> +static void *urb_debug_hint(void *addr)
> +{
> + return ((struct urb *) addr)->complete;
> +}
> +
> +/*
> + * fixup_init is called when:
> + * - an active object is initialized
> + */
> +static bool urb_fixup_init(void *addr, enum debug_obj_state state)
> +{
> + struct urb *urb = addr;
> +
> + switch (state) {
> + case ODEBUG_STATE_ACTIVE:
> + usb_kill_urb(urb);
> + debug_object_init(urb, _debug_descr);
> + return true;
> + default:
> + return false;
> + }
> +}

Is it guaranteed that this routine and the other new ones can be called
only in process context?  I don't think so -- but usb_kill_urb will 
oops if it is called with interrupts disabled.

> @@ -539,10 +635,23 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
>   }
>   }
>  
> - return usb_hcd_submit_urb(urb, mem_flags);
> + ret = debug_urb_activate(urb);
> + if (ret)
> + return ret;
> + ret = usb_hcd_submit_urb(urb, mem_flags);
> + if (ret)
> + debug_urb_deactivate(urb);
> +
> + return ret;
>  }
>  EXPORT_SYMBOL_GPL(usb_submit_urb);
>  
> +static inline int __usb_unlink_urb(struct urb *urb, int status)
> +{
> + debug_urb_deactivate(urb);
> + return usb_hcd_unlink_urb(urb, status);
> +}

This is a mistake.  The URB is still active until it is given back
to the completion routine.

Alan Stern


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

Re: [RFC PATCH] usb: gadget: Introduce Cadence USB2 UDC Driver

[Neil Armstrong]

Hi Felipe, Laurent,

I submitted this driver for a Cadence IP library from a now abandoned project.

The driver was working on a SoC platform ported on a FPGA, but I do not have 
access to this HW anymore.

But I have a fixed version...

Now, I am looking for SoCs, vendors and even Cadence people (or even Evatronix 
employees who worked on this IP before being in Cadence portfolio)
to push this driver upstream by testing it on a real HW or even FPGA.

I hope this message in a bottle will get read !

Regards,
Neil

On 11/16/2015 05:11 PM, Felipe Balbi wrote:
> 
> Hi,
> 
> Neil Armstrong  writes:
>> Introduces UDC support for the Device-Mode only version of the
>> Cadence USB2 Controller IP Core.
>>
>> Host mode and OTG mode are not implemented by lack of hardware.
>> Support for Isochronous endpoints is not implemented by lack of time.
>>
>> Internal DMA is supported and can be activated by DT property.
>>
>> Signed-off-by: Neil Armstrong 
> 
> looks like there are a few checkpatch warnings to be fixed:
> 
> WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
> #89: 
> new file mode 100644
> 
> WARNING: 'tranfer' may be misspelled - perhaps 'transfer'?
> #233: FILE: drivers/usb/gadget/udc/cadence_hsudc.c:44:
> + *  - EP Bulk and Interrupt tranfer
> 
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
> than BUG() or BUG_ON()
> #968: FILE: drivers/usb/gadget/udc/cadence_hsudc.c:779:
> + BUG();
> 
> WARNING: Avoid crashing the kernel - try using WARN_ON & recovery code rather 
> than BUG() or BUG_ON()
> #1492: FILE: drivers/usb/gadget/udc/cadence_hsudc.c:1303:
> + BUG();
> 
> WARNING: DT compatible string "cdns,usbhs-udc" appears un-documented -- check 
> workspace/linux/Documentation/devicetree/bindings/
> #1882: FILE: drivers/usb/gadget/udc/cadence_hsudc.c:1693:
> + { .compatible = "cdns,usbhs-udc" },
> 
> WARNING: 'tranfer' may be misspelled - perhaps 'transfer'?
> #2603: FILE: drivers/usb/gadget/udc/cadence_hsudc_regs.h:272:
> +#define HSUDC_DMA_BUSCTRL_HSIZE_8BIT (0 << 1) /* 8-bit data tranfer */
> 
> WARNING: 'tranfer' may be misspelled - perhaps 'transfer'?
> #2604: FILE: drivers/usb/gadget/udc/cadence_hsudc_regs.h:273:
> +#define HSUDC_DMA_BUSCTRL_HSIZE_16BIT(1 << 1) /* 16-bit data tranfer 
> */
> 
> WARNING: 'tranfer' may be misspelled - perhaps 'transfer'?
> #2605: FILE: drivers/usb/gadget/udc/cadence_hsudc_regs.h:274:
> +#define HSUDC_DMA_BUSCTRL_HSIZE_32BIT(2 << 1) /* 32-bit data tranfer 
> */
> 
> WARNING: 'tranfer' may be misspelled - perhaps 'transfer'?
> #2607: FILE: drivers/usb/gadget/udc/cadence_hsudc_regs.h:276:
> +#define HSUDC_DMA_BUSCTRL_BURST_SINGLE   (0 << 4) /* Single tranfer */
> 
> total: 0 errors, 9 warnings, 2499 lines checked
> 
> Your patch has style problems, please review.
> 
> NOTE: If any of the errors are false positives, please report
>   them to the maintainer, see CHECKPATCH in MAINTAINERS.
> 




signature.asc
Description: OpenPGP digital signature

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Guenter Roeck]

On 05/19/2016 05:44 AM, Heikki Krogerus wrote:

The purpose of this class is to provide unified interface for user
space to get the status and basic information about USB Type-C
Connectors in the system, control data role swapping, and when USB PD
is available, also power role swapping and Alternate Modes.

Signed-off-by: Heikki Krogerus 
---
  drivers/usb/Kconfig |   2 +
  drivers/usb/Makefile|   2 +
  drivers/usb/type-c/Kconfig  |   7 +
  drivers/usb/type-c/Makefile |   1 +
  drivers/usb/type-c/typec.c  | 957 
  include/linux/usb/typec.h   | 230 +++
  6 files changed, 1199 insertions(+)
  create mode 100644 drivers/usb/type-c/Kconfig
  create mode 100644 drivers/usb/type-c/Makefile
  create mode 100644 drivers/usb/type-c/typec.c
  create mode 100644 include/linux/usb/typec.h



Hi,


+/*
+ * struct typec_capability - USB Type-C Port Capabilities
+ * @role: DFP (Host-only), UFP (Device-only) or DRP (Dual Role)
+ * @usb_pd: USB Power Delivery support
+ * @alt_modes: Alternate Modes the connector supports (null terminated)
+ * @audio_accessory: Audio Accessory Adapter Mode support
+ * @debug_accessory: Debug Accessory Mode support
+ * @fix_role: Set a fixed data role for DRP port
+ * @dr_swap: Data Role Swap support
+ * @pr_swap: Power Role Swap support
+ * @vconn_swap: VCONN Swap support
+ * @activate_mode: Enter/exit given Alternate Mode
+ *
+ * Static capabilities of a single USB Type-C port.
+ */
+struct typec_capability {
+   enum typec_data_rolerole;
+   unsigned intusb_pd:1;
+   struct typec_altmode*alt_modes;
+   unsigned intaudio_accessory:1;
+   unsigned intdebug_accessory:1;
+
+   int (*fix_role)(struct typec_port *,
+   enum typec_data_role);
+
+   int (*dr_swap)(struct typec_port *);
+   int (*pr_swap)(struct typec_port *);
+   int (*vconn_swap)(struct typec_port *);
+


The function parameter in those calls is all but useless to the caller.
It needs to store the typec_port returned from typec_register(), create a
list of ports, and then search through this list each time one of the
functions is called. This is quite expensive for no good reason.

Previously, with typec_port exported, the called code could use the stored
caps pointer to map to its internal data structures. This is no longer
possible.

I think it would be useful to provide a better means for the called function
to identify its context. Maybe provide a pointer to the private data in
the registration function and use it as parameter in the callback functions ?

Thanks,
Guenter

--
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 PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Thu, 2016-05-19 at 15:44 +0300, Heikki Krogerus wrote:

Hi,

as this discussion seems to go in circles, I am starting anew
at the top.

> Like I've told some of you guys, I'm trying to implement a bus for
> the Alternate Modes, but I'm still nowhere near finished with that
> one, so let's just get the class ready now. The altmode bus should in
> any case not affect the userspace interface proposed in this patch.
> 
> As you can see, the Alternate Modes are handled completely differently
> compared to the original proposal. Every Alternate Mode will have
> their own device instance (which will be then later bound to an
> Alternate Mode specific driver once we have the bus), but also every
> partner, cable and cable plug will have their own device instances
> representing them.

The API works for a DFP. I fail to see how the UFP learns about entering
an alternate mode.
Secondly, support to trigger a reset is missing

> An other change is that the data role is now handled in two ways.
> The current_data_role file will represent static mode of the port, and
> it will use the names for the roles as they are defined in the spec:
> DFP, UFP and DRP. This file should be used if the port needs to be

Good, but support for Try.SRC and Try.SNK is missing.
An additional problem with that is that it needs to work
without user space during boot. So I think module parameters
to set the default are necessary.

> fixed to one specific role with DRP ports. So this approach will
> replace the suggestions for "preferred" data role we had. The
> current_usb_data_role will use values "host" and "device" and it will
> be used for data role swapping when already connected.
> 
> The tree of devices that will be populated when the cable is active
> and when the cable has controller on both plug, will look as
> following:
> 
> usbc0
> |- usbc0-cable
> |   |- usbc0-plug0
> |   |   |- usbc0-plug.svid:xxx
> |   |   |   |-mode0
> |   |   |   |   |- vdo
> |   |   |   |   |- desc
> |   |   |   |   |- active
> ...
> |   |- usbc0-plug1
> |   |   |-usbc0-partner
> |   |   |   |- usbc0-partner.svid:
> |   |   |   | |-mode0
> |   |   |   | |   |- vdo
> |   |   |   | |   |- desc
> |   |   |   | |   |- active
> |   |   |   | |-mode1
> ...
> |   |   |- usbc0-plug1.svid:xxx
> |   |   |   |-mode0
> |   |   |   |   |- vdo
> ...
> 
> If there is no active cable, the partner will be directly attached to
> the port, but symlink to the partner is now always added to the port
> folder in any case. I'm not sure about this approach. There is a
> question about it in the code. Please check it.

This approach looks workable.
An the whole the approach looks good, but needs to be extended.

Regards
Oliver


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

Re: [PATCH] USB: uas: Fix slave queue_depth not being set

[James Bottomley]

On Tue, 2016-05-24 at 08:53 +0200, Hans de Goede wrote:
> Hi,
> 
> On 23-05-16 19:36, James Bottomley wrote:
> > On Mon, 2016-05-23 at 13:49 +0200, Hans de Goede wrote:
> > > Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host
> > > level")
> > > removed the scsi_change_queue_depth() call from 
> > > uas_slave_configure() assuming that the slave would inherit the 
> > > host's queue_depth, which that commit sets to the same value.
> > > 
> > > This is incorrect, without the scsi_change_queue_depth() call the
> > > slave's queue_depth defaults to 1, introducing a performance
> > > regression.
> > > 
> > > This commit restores the call, fixing the performance regression.
> > > 
> > > Cc: sta...@vger.kernel.org
> > > Fixes: 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host
> > > level")
> > > Reported-by: Tom Yan 
> > > Signed-off-by: Hans de Goede 
> > > ---
> > >  drivers/usb/storage/uas.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/usb/storage/uas.c
> > > b/drivers/usb/storage/uas.c
> > > index 16bc679..ecc7d4b 100644
> > > --- a/drivers/usb/storage/uas.c
> > > +++ b/drivers/usb/storage/uas.c
> > > @@ -835,6 +835,7 @@ static int uas_slave_configure(struct
> > > scsi_device
> > > *sdev)
> > >   if (devinfo->flags & US_FL_BROKEN_FUA)
> > >   sdev->broken_fua = 1;
> > > 
> > > + scsi_change_queue_depth(sdev, devinfo->qdepth - 2);
> > 
> > Are you sure about this?  For spinning rust, experiments imply that 
> > the optimal queue depth per device is somewhere between 2 and 4. 
> >  Obviously that's not true for SSDs, so it depends on your use 
> > case.  Plus, for ATA NCQ devices (which I believe most UAS is 
> > bridged to) you have a maximum NCQ depth of 31.
> 
> So this value is the same as host.can_queue, and is what uas has 
> always used, basically this says it is ok to queue as much as the 
> bridge can handle. We've seen a few rare multi-lun devices, but 
> typically almost all uas devices have one lun, what I really want to 
> do here is give a maximum and let say the sd driver lower that if it
> is sub-optimal.

If that's what you actually want, you should be setting sdev
->max_queue_depth and .track_queue_depth = 1 in the template.

You might also need to add calls to scsi_track_queue_full() but only if
the devices aren't responding QUEUE_FULL correctly.

James

> Also notice that uas is used a lot with ssd-s, that is mostly what
> I want to optimize for, but it is definitely also used with spinning
> rust.
> 
> And yes almost all uas devices are bridged sata devices (this may
> change in the near future though, with ssd-s specifically designed
> for usb-3 attachment, although sofar these all seem to use an
> embbeded sata bridge), so from this pov an upper limit of 31 makes 
> sense, I guess, but I've not seen any bridges which actually do more 
> then 32 streams anyways.
> 
> Still this is a bug-fix patch, essentially a partial revert, to 
> address performance regressions, so lets get this out as is and take 
> our time to come up with some tweaks (if necessary) for the say 4.8.
> 
> > There's a good reason why you don't want a queue deeper than you 
> > can handle: it tends to interfere with writeback because you build 
> > up a lot of pending I/O in the queue which can't be issued (it's 
> > very similar to why bufferbloat is a problem in networks).  In 
> > theory, as long as your devices return the correct indicator 
> > (QUEUE_FULL status), we'll handle most of this in the mid-layer by 
> > plugging the block queue, but given what I've seen from UAS
> > devices, that's less than probable.
> 
> So any smart ideas how to be nicer to spinning rust, without 
> negatively impacting ssd-s? As said if I've to choice I think we 
> should chose optimizing ssd-s, as that is where uas is used a lot 
> (although usb  attached harddisks are switching over to it too).
> 
> Note I just checked the 1TB sata/ahci harddisk in my workstation and 
> it is using a queue_depth of 31 too, so this really does seem like a 
> mid-layer problem to me.
> 
> Regards,
> 
> Hans
> --
> To unsubscribe from this list: send the line "unsubscribe linux-scsi"
> in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: [PATCH v8 5/5] usb: dwc3: core: cleanup IRQ resources

[Roger Quadros]

On 24/05/16 12:35, Felipe Balbi wrote:
> 
> Hi,
> 
> Roger Quadros  writes:
>> Implementations might use different IRQs for
>> host, gadget and OTG so use named interrupt resources
>> to allow Device tree to specify the 3 interrupts.
>>
>> Following are the interrupt names
>>
>> Peripheral Interrupt - peripheral
>> HOST Interrupt - host
>> OTG Interrupt - otg
>>
>> We still maintain backward compatibility for a single named
>> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
>> single unnamed interrupt for all 3 interrupts (e.g. old DT).
>>
>> Signed-off-by: Roger Quadros 
> 
> fails to apply, unfortunately:
> 
> checking file drivers/usb/dwc3/core.c
> Hunk #1 succeeded at 845 (offset 31 lines).
> checking file drivers/usb/dwc3/core.h
> Hunk #1 succeeded at 738 (offset 22 lines).
> Hunk #2 FAILED at 818.
> 1 out of 2 hunks FAILED
> checking file drivers/usb/dwc3/gadget.c
> Hunk #1 succeeded at 1710 with fuzz 2 (offset 105 lines).
> Hunk #2 FAILED at 1728.
> Hunk #3 FAILED at 1740.
> Hunk #4 succeeded at 2835 (offset 54 lines).
> 2 out of 4 hunks FAILED
> checking file drivers/usb/dwc3/host.c
> 
> I'll update my testing/next shortly, if you could rebase the remaining
> of these patches on that, I'd be glad.

No Problem Felipe. I'll rebase and re-send this series.

cheers,
-roger
--
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 PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Tue, May 24, 2016 at 12:18:38PM +0200, Oliver Neukum wrote:
> On Tue, 2016-05-24 at 13:08 +0300, Heikki Krogerus wrote:
> > Hi Guenter,
> > 
> > On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> > > On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > > > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > > > 
> > > > > > Now I am confused. Are you saying that the choice of Alternate Mode 
> > > > > > does
> > > > > > not belong into user space?
> > > > > >
> > > > > 
> > > > > No; sorry for the confusion. The above was meant to apply to my use
> > > > > of "preferred mode", not yours. I was trying to say that the choice of
> > > > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > > > should belong primarily into the kernel, to be determined by the 
> > > > > platform
> > > > > (presumably via ACPI, devicetree data, or platform data). If it should
> > > > 
> > > > Why on earth? That is most clearly a policy decision.
> > > > 
> > > 
> > > The question is not that much if it is policy (it is), but if the policy
> > > should be driven by the platform or by user space. I think there needs
> > > to be at least a default driven by the platform. As already mentioned,
> > > I am ok with a means to override this platform default from user space.
> > > But if user space doesn't say, there still needs to be a default.
> > 
> > I don't completely agree with that. The platform should not, and
> > actually in most cases with ACPI AFAIK, will not provide any
> > "preferences" to the OS about anything. The platform should only
> > provide the OS the physical capabilities and nothing else. So if for
> > example the platform is capable of acting as only source with a Type-C
> > port, that is what it needs to tell to the OS so possibly the PHY can
> > be programmed accordingly, etc.
> 
> May I suggest that the point is moot as long as we agree that user space
> needs to be able to set a policy? The OS cannot ignore the port before
> user space tells it what to do. So a default will be needed.
> 
> The OS must act conservatively, which means it cannot deactivate
> hardware. So if a port can do DRP, that must be the default.

So exactly my point.

> I would go so far that I would suggest that we add a module parameter
> for Try.SRC and Try.SNK to avoid trouble during boot.

I would be fine with that, but apparently module parameters are so 90s
:(

> > So IMO, just like with any decision related to what the system will
> > ultimately be used for, decision about the preferred role really
> > belongs to the userspace.
> 
> Yes, but ought the APIs for role, mode and PD be separate or not.
> Sorry to sound like a broken record, but you need to provide some
> higher level planning here.

I was kinda hoping that we could formulate the plan together in this
thread.


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: xhci DWC3 flavor problem

[Felipe Balbi]

Hi João,

(please avoid top-posting ;)

Joao Pinto  writes:
> Hi,
> Just to give you an update.
> All is working great, the cause was an FPGA configuration issue.
> Thank you for your support.

good to know :)

-- 
balbi


signature.asc
Description: PGP signature

Re: xhci DWC3 flavor problem

[Joao Pinto]

Hi,
Just to give you an update.
All is working great, the cause was an FPGA configuration issue.
Thank you for your support.

Joao

On 5/19/2016 4:42 PM, Joao Pinto wrote:
> 
> After a few moments the schedule problem happen again:
> 
> # INFO: task kworker/0:1:349 blocked for more than 120 seconds.
>   Not tainted 4.6.0-rc5 #9
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/0:1 D ff8008086c60 0   349  2 0x
> Workqueue: usb_hub_wq hub_event
> Call trace:
> [] __switch_to+0xc8/0xd4
> [] __schedule+0x18c/0x5c8
> [] schedule+0x38/0x98
> [] schedule_timeout+0x160/0x1ac
> [] wait_for_common+0xac/0x150
> [] wait_for_completion+0x14/0x1c
> [] xhci_alloc_dev+0xf4/0x2a0
> [] usb_alloc_dev+0x68/0x2cc
> [] hub_event+0x784/0x11f4
> [] process_one_work+0x130/0x2f4
> [] worker_thread+0x54/0x434
> [] kthread+0xd4/0xe8
> [] ret_from_fork+0x10/0x40
> INFO: task kworker/0:1:349 blocked for more than 120 seconds.
>   Not tainted 4.6.0-rc5 #9
> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> kworker/0:1 D ff8008086c60 0   349  2 0x
> Workqueue: usb_hub_wq hub_event
> Call trace:
> [] __switch_to+0xc8/0xd4
> [] __schedule+0x18c/0x5c8
> [] schedule+0x38/0x98
> [] schedule_timeout+0x160/0x1ac
> [] wait_for_common+0xac/0x150
> [] wait_for_completion+0x14/0x1c
> [] xhci_alloc_dev+0xf4/0x2a0
> [] usb_alloc_dev+0x68/0x2cc
> [] hub_event+0x784/0x11f4
> [] process_one_work+0x130/0x2f4
> [] worker_thread+0x54/0x434
> [] kthread+0xd4/0xe8
> [] ret_from_fork+0x10/0x40
> 
> So Chris' patch did not solve this problem either.
> 
> Thanks.
> 
> On 5/19/2016 4:39 PM, Joao Pinto wrote:
>> Hi Felipe and Mathias,
>>
>> On 5/19/2016 1:22 PM, Mathias Nyman wrote:
>>> On 19.05.2016 14:23, Joao Pinto wrote:
 Hi Felipe,

 On 5/19/2016 11:32 AM, Felipe Balbi wrote:
>
> Hi,
>
>
> Note that we really did get a command timeout. Can you add a little
> extra debugging to try and figure out why that command failed?
>

 After instrumenting and capturing FPGA signals, the driver could go a bit
 further... Could this be a FPGA timming issue?

>>> ..
>>>
 xhci-hcd xhci-hcd.0.auto: Timeout while waiting for setup device command
 usb 3-1: hub failed to enable device, error -62
 xhci-hcd xhci-hcd.0.auto: Endpoint 0x0 ep reset callback called
 xhci-hcd xhci-hcd.0.auto: xHCI dying or halted, can't queue_command
 xhci-hcd xhci-hcd.0.auto: FIXME: allocate a command ring segment
 usb usb3-port1: couldn't allocate usb_device

 Joao
>>>
>>> Does the patch from Chris Bainbridge help?
>>> It's currently only Gregs tree in the usb-next branch.
>>>
>>> It fixes a locking issue where hw can't handle several ports being in 
>>> default
>>> state at
>>> the same time, and setup device command timeout issue when both usb2 and 
>>> usb3
>>> devices
>>> try to enumerate at the same time.
>>>
>>> commit feb26ac31a2a5cb88d86680d9a94916a6343e9e6
>>> usb: core: hub: hub_port_init lock controller instead of bus
>>
>> Applied Chris' patch and apparently solved the cyclic crash problem that
>> happened every time the wait completion timeout and context had to change. 
>> But
>> the log after insert pen drive remains the same:
>>
>> xhci-hcd xhci-hcd.0.auto: Port Status Change Event for port 1
>> xhci-hcd xhci-hcd.0.auto: resume root hub
>> xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
>> xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x206e1
>> xhci-hcd xhci-hcd.0.auto: Get port status returned 0x10101
>> xhci-hcd xhci-hcd.0.auto: clear port connect change, actual port 0 status  = 
>> 0x6e1
>> xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x6e1
>> xhci-hcd xhci-hcd.0.auto: Get port status returned 0x101
>> xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
>> xhci-hcd xhci-hcd.0.auto: // Ding dong!
>> xhci-hcd xhci-hcd.0.auto: Command timeout
>> xhci-hcd xhci-hcd.0.auto: Abort command ring
>>
>>
>> It worked a bit a few hours ago like I tiold you in a past e-mail:
>> After instrumenting and capturing FPGA signals, the driver could go a bit
>> further... Could this be a FPGA timming issue?
>>
>>
>> xhci-hcd xhci-hcd.0.auto: Port Status Change Event for port 1
>> xhci-hcd xhci-hcd.0.auto: resume root hub
>> xhci-hcd xhci-hcd.0.auto: handle_port_status: starting port polling.
>> xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x206e1
>> xhci-hcd xhci-hcd.0.auto: Get port status returned 0x10101
>> xhci-hcd xhci-hcd.0.auto: clear port connect change, actual port 0 status  = 
>> 0x6e1
>> xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
>> xhci-hcd xhci-hcd.0.auto: get port status, actual port 0 status  = 0x6e1
>> xhci-hcd xhci-hcd.0.auto: Get port status returned 0x101
>> xhci-hcd xhci-hcd.0.auto: xhci_hub_status_data: stopping port polling.
>> xhci-hcd 

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Tue, 2016-05-24 at 13:08 +0300, Heikki Krogerus wrote:
> Hi Guenter,
> 
> On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> > On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > > 
> > > > > Now I am confused. Are you saying that the choice of Alternate Mode 
> > > > > does
> > > > > not belong into user space?
> > > > >
> > > > 
> > > > No; sorry for the confusion. The above was meant to apply to my use
> > > > of "preferred mode", not yours. I was trying to say that the choice of
> > > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > > should belong primarily into the kernel, to be determined by the 
> > > > platform
> > > > (presumably via ACPI, devicetree data, or platform data). If it should
> > > 
> > > Why on earth? That is most clearly a policy decision.
> > > 
> > 
> > The question is not that much if it is policy (it is), but if the policy
> > should be driven by the platform or by user space. I think there needs
> > to be at least a default driven by the platform. As already mentioned,
> > I am ok with a means to override this platform default from user space.
> > But if user space doesn't say, there still needs to be a default.
> 
> I don't completely agree with that. The platform should not, and
> actually in most cases with ACPI AFAIK, will not provide any
> "preferences" to the OS about anything. The platform should only
> provide the OS the physical capabilities and nothing else. So if for
> example the platform is capable of acting as only source with a Type-C
> port, that is what it needs to tell to the OS so possibly the PHY can
> be programmed accordingly, etc.

May I suggest that the point is moot as long as we agree that user space
needs to be able to set a policy? The OS cannot ignore the port before
user space tells it what to do. So a default will be needed.

The OS must act conservatively, which means it cannot deactivate
hardware. So if a port can do DRP, that must be the default.
I would go so far that I would suggest that we add a module parameter
for Try.SRC and Try.SNK to avoid trouble during boot.

> So IMO, just like with any decision related to what the system will
> ultimately be used for, decision about the preferred role really
> belongs to the userspace.

Yes, but ought the APIs for role, mode and PD be separate or not.
Sorry to sound like a broken record, but you need to provide some
higher level planning here.

Regards
Oliver


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

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

Hi Guenter,

On Mon, May 23, 2016 at 09:52:12AM -0700, Guenter Roeck wrote:
> On Mon, May 23, 2016 at 05:55:04PM +0200, Oliver Neukum wrote:
> > On Mon, 2016-05-23 at 07:43 -0700, Guenter Roeck wrote:
> > > On 05/23/2016 06:58 AM, Oliver Neukum wrote:
> > 
> > > > Now I am confused. Are you saying that the choice of Alternate Mode does
> > > > not belong into user space?
> > > >
> > > 
> > > No; sorry for the confusion. The above was meant to apply to my use
> > > of "preferred mode", not yours. I was trying to say that the choice of
> > > preferred roles (which determines if Try.SRC or Try.SNK is enabled)
> > > should belong primarily into the kernel, to be determined by the platform
> > > (presumably via ACPI, devicetree data, or platform data). If it should
> > 
> > Why on earth? That is most clearly a policy decision.
> > 
> 
> The question is not that much if it is policy (it is), but if the policy
> should be driven by the platform or by user space. I think there needs
> to be at least a default driven by the platform. As already mentioned,
> I am ok with a means to override this platform default from user space.
> But if user space doesn't say, there still needs to be a default.

I don't completely agree with that. The platform should not, and
actually in most cases with ACPI AFAIK, will not provide any
"preferences" to the OS about anything. The platform should only
provide the OS the physical capabilities and nothing else. So if for
example the platform is capable of acting as only source with a Type-C
port, that is what it needs to tell to the OS so possibly the PHY can
be programmed accordingly, etc.

So IMO, just like with any decision related to what the system will
ultimately be used for, decision about the preferred role really
belongs to the userspace.


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: [PATCH v8 08/14] usb: otg: add OTG/dual-role core

[Roger Quadros]

Hi Peter,

I have one question here. Please see below.

On 13/05/16 13:03, Roger Quadros wrote:
> It provides APIs for the following tasks
> 
> - Registering an OTG/dual-role capable controller
> - Registering Host and Gadget controllers to OTG core
> - Providing inputs to and kicking the OTG state machine
> 
> Provide a dual-role device (DRD) state machine.
> DRD mode is a reduced functionality OTG mode. In this mode
> we don't support SRP, HNP and dynamic role-swap.
> 
> In DRD operation, the controller mode (Host or Peripheral)
> is decided based on the ID pin status. Once a cable plug (Type-A
> or Type-B) is attached the controller selects the state
> and doesn't change till the cable in unplugged and a different
> cable type is inserted.
> 
> As we don't need most of the complex OTG states and OTG timers
> we implement a lean DRD state machine in usb-otg.c.
> The DRD state machine is only interested in 2 hardware inputs
> 'id' and 'b_sess_vld'.
> 
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/common/Makefile  |2 +-
>  drivers/usb/common/usb-otg.c | 1042 
> ++
>  drivers/usb/core/Kconfig |4 +-
>  include/linux/usb/gadget.h   |2 +
>  include/linux/usb/hcd.h  |1 +
>  include/linux/usb/otg-fsm.h  |7 +
>  include/linux/usb/otg.h  |  154 ++-
>  7 files changed, 1206 insertions(+), 6 deletions(-)
>  create mode 100644 drivers/usb/common/usb-otg.c
> 



> +
> +/**
> + * usb_otg_register() - Register the OTG/dual-role device to OTG core
> + * @dev: OTG/dual-role controller device.
> + * @config: OTG configuration.
> + *
> + * Registers the OTG/dual-role controller device with the USB OTG core.
> + *
> + * Return: struct usb_otg * if success, ERR_PTR() if error.
> + */
> +struct usb_otg *usb_otg_register(struct device *dev,
> +  struct usb_otg_config *config)
> +{
> + struct usb_otg *otg;
> + struct otg_wait_data *wait;
> + int ret = 0;
> +
> + if (!dev || !config || !config->fsm_ops)
> + return ERR_PTR(-EINVAL);
> +
> + /* already in list? */
> + mutex_lock(_list_mutex);
> + if (usb_otg_get_data(dev)) {
> + dev_err(dev, "otg: %s: device already in otg list\n",
> + __func__);
> + ret = -EINVAL;
> + goto unlock;
> + }
> +
> + /* allocate and add to list */
> + otg = kzalloc(sizeof(*otg), GFP_KERNEL);
> + if (!otg) {
> + ret = -ENOMEM;
> + goto unlock;
> + }
> +
> + otg->dev = dev;
> + otg->caps = config->otg_caps;

Here, we should be checking if user needs to disable any OTG features. So,

if (dev->of_node)
of_usb_update_otg_caps(dev->of_node, >caps);

Do you agree?
This means we need to change otg->caps from 'struct usb_otg_caps *caps;'
to 'struct usb_otg_caps caps;' so that we can modify the local copy instead
of the one passed by the OTG controller.

We can also move of_usb_update_otg_caps() to otg.h.

We will also need to modify the udc-core code so that it sets gadget->otg_caps
to the modified otg_caps from the OTG core. This will ensure that the right
OTG descriptors are sent.

So we will have to introduce an API.

struct usb_otg_caps *usb_otg_get_otg_caps(struct device *otg_dev)

And in udc-core.c,

static int udc_bind_to_driver(struct usb_udc *udc, struct usb_gadget_driver 
*driver)
{
..
ret = driver->bind(udc->gadget, driver);
if (ret)
goto err1;

/* If OTG, the otg core starts the UDC when needed */
if (udc->gadget->otg_dev) {
+   udc->gadget->is_otg = true;
+   udc->gadget->otg_caps = 
usb_otg_get_otg_caps(udc->gadget->otg_dev);
mutex_unlock(_lock);
usb_otg_register_gadget(udc->gadget, _gadget_intf);
mutex_lock(_lock);
} else {
ret = usb_gadget_udc_start(udc);
if (ret) {
driver->unbind(udc->gadget);
goto err1;
}
usb_udc_connect_control(udc);
}
..
}

What do you say?

regards,
-roger
--
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 v8 5/5] usb: dwc3: core: cleanup IRQ resources

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Implementations might use different IRQs for
> host, gadget and OTG so use named interrupt resources
> to allow Device tree to specify the 3 interrupts.
>
> Following are the interrupt names
>
> Peripheral Interrupt - peripheral
> HOST Interrupt - host
> OTG Interrupt - otg
>
> We still maintain backward compatibility for a single named
> interrupt for all 3 interrupts (e.g. for dwc3-pci) and
> single unnamed interrupt for all 3 interrupts (e.g. old DT).
>
> Signed-off-by: Roger Quadros 

fails to apply, unfortunately:

checking file drivers/usb/dwc3/core.c
Hunk #1 succeeded at 845 (offset 31 lines).
checking file drivers/usb/dwc3/core.h
Hunk #1 succeeded at 738 (offset 22 lines).
Hunk #2 FAILED at 818.
1 out of 2 hunks FAILED
checking file drivers/usb/dwc3/gadget.c
Hunk #1 succeeded at 1710 with fuzz 2 (offset 105 lines).
Hunk #2 FAILED at 1728.
Hunk #3 FAILED at 1740.
Hunk #4 succeeded at 2835 (offset 54 lines).
2 out of 4 hunks FAILED
checking file drivers/usb/dwc3/host.c

I'll update my testing/next shortly, if you could rebase the remaining
of these patches on that, I'd be glad.

Thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 5/5] usb: dwc3: rockchip: add devicetree bindings documentation

[Felipe Balbi]

Hi,

William Wu  writes:
> This patch documents the device tree documentation required for
> Rockchip USB3.0 core wrapper consist of USB3.0 IP from Synopsys.
>
> It could operate in device mode (SS, HS, FS) and host
> mode (SS, HS, FS, LS).
>
> Signed-off-by: William Wu 
> ---
> Changes in v2:
> - add rockchip,dwc3.txt to Documentation/devicetree/bindings/ (Felipe, Brian)
>
>  .../devicetree/bindings/usb/rockchip,dwc3.txt  | 45 
> ++
>  1 file changed, 45 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
>
> diff --git a/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt 
> b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> new file mode 100644
> index 000..10303d9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/usb/rockchip,dwc3.txt
> @@ -0,0 +1,45 @@
> +Rockchip SuperSpeed DWC3 USB SoC controller
> +
> +Required properties:
> +- compatible:should contain "rockchip,dwc3"
> +- clocks:A list of phandle + clock-specifier pairs for the
> + clocks listed in clock-names
> +- clock-names:   Should contain the following:
> +  "clk_usb3otg0_ref" Controller reference clk
> +  "clk_usb3otg0_suspend"Controller suspend clk, can use 24 MHz or 32 KHz
> +  "aclk_usb3"Master/Core clock, have to be >= 62.5 MHz for 
> SS operation
> +
> +
> +Optional clocks:
> +  "aclk_usb3otg0"Aclk for specific usb controller clock.
> +  "aclk_usb3_rksoc_axi_perf"  USB AXI perf clock.  Not present on all 
> platforms.
> +  "aclk_usb3_grf"USB grf clock.  Not present on all platforms.
> +
> +Required child node:
> +A child node must exist to represent the core DWC3 IP block. The name of
> +the node is not important. The content of the node is defined in dwc3.txt.
> +
> +Phy documentation is provided in the following places:
> +
> +Example device nodes:
> +
> + usbdrd3_0: usb@fe80 {
> +

no reg property?
compatible = "rockchip,dwc3";
> + clocks = < SCLK_USB3OTG0_REF>, < SCLK_USB3OTG0_SUSPEND>,
> +  < ACLK_USB3>, < ACLK_USB3OTG0>,
> +  < ACLK_USB3_RKSOC_AXI_PERF>, < ACLK_USB3_GRF>;
> + clock-names = "clk_usb3otg0_ref", "clk_usb3otg0_suspend",
> +   "aclk_usb3", "aclk_usb3otg0",
> +   "aclk_usb3_rksoc_axi_perf", "aclk_usb3_grf";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> + status = "disabled";
> + usbdrd_dwc3_0: dwc3 {

no address here?

> + compatible = "snps,dwc3";
> + reg = <0x0 0xfe80 0x0 0x10>;
> + interrupts = ;
> + dr_mode = "otg";
> + status = "disabled";
> + };
> + };
> -- 
> 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

-- 
balbi


signature.asc
Description: PGP signature

Re: [RFC PATCHv2] usb: USB Type-C Connector Class

[Heikki Krogerus]

On Mon, May 23, 2016 at 01:25:19PM +0200, Oliver Neukum wrote:
> So for Alternate Modes we need on a high level the following features
> 
> 1. discovery of available Alternate Modes
> 2. selection of an Alternate Mode
> 3. notification about entering an Alternate Mode
> 4. triggering a reset
> 5. notification about resets
> 
> 6. discovery about the current role
> 7. switching roles
> 8. setting preferred roles (Try.SRC and Try.SNK)
> 
> You covered 1. and 2.
> 3. can be covered by specific drivers
> 4. and 5. are not covered (and it makes no sense to tie it
> to specific drivers)
> 
> 6. and 7. is covered
> 8. is not
> 
> And 8. needs to be covered. It affects who selects the Alternate Mode.
> You cannot tie it to USB and it doesn't fit with pure PD stuff.
> 
> I like your API as it is now. But it is incomplete.

OK, Got it.


Thanks Oliver,

-- 
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: [uas/scsi] untagged commands?

[Hans de Goede]

Hi,

On 24-05-16 10:18, Tom Yan wrote:

In this commit:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/storage/uas.c?id=198de51dbc3454d95b015ca0a055b673f85f01bb

There is the following comment:

1 tag is reserved for untagged commands


So my question is, what exactly are "untagged commands"?


https://en.wikipedia.org/wiki/Tagged_Command_Queuing

Regards,

Hans
--
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 PATCHv2] usb: USB Type-C Connector Class

[Oliver Neukum]

On Mon, 2016-05-23 at 10:09 -0700, Guenter Roeck wrote:
> On Mon, May 23, 2016 at 01:25:19PM +0200, Oliver Neukum wrote:
> > On Mon, 2016-05-23 at 12:57 +0300, Heikki Krogerus wrote:
> >
> > A reset is a generic function, so it does not belong to specific
> > drivers.
> > 
> A would expect the driver to execute the reset.
> 
> Maybe the question should be phrased differently: Even USCI (which
> doesn't provide for everything) has commands to reset the policy
> manager and to reset the connector. The class should provide a means
> to execute those commands.

Yes.

> > So for Alternate Modes we need on a high level the following features
> > 
> > 1. discovery of available Alternate Modes
> > 2. selection of an Alternate Mode
> > 3. notification about entering an Alternate Mode
> > 4. triggering a reset
> > 5. notification about resets
> > 
> > 6. discovery about the current role
> > 7. switching roles
> > 8. setting preferred roles (Try.SRC and Try.SNK)
> > 
> 
> Isn't reset and role handling orthogonal to alternate mode functionality ?
> Both will still be needed even if alternate mode support is not implemented
> at all.

In part. A reset can cause the Alternate Mode to be left unexpectedly
and unintentionally.
So how many APIs do we want?
Three:

- Alternate Modes
- USB PD
- type C for roles and reset

Or another number?

> > I like your API as it is now. But it is incomplete.
> > 
> 
> Same here.

So what is to be done?

Regards
Oliver


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

[PATCH V3 2/2] usb: musb: Stop bulk endpoint while queue is rotated

[Andrew Goodbody]

Ensure that the endpoint is stopped by clearing REQPKT before
clearing DATAERR_NAKTIMEOUT before rotating the queue on the
dedicated bulk endpoint.
This addresses an issue where a race could result in the endpoint
receiving data before it was reprogrammed resulting in a warning
about such data from musb_rx_reinit before it was thrown away.
The data thrown away was a valid packet that had been correctly
ACKed which meant the host and device got out of sync.

Signed-off-by: Andrew Goodbody 
Cc: sta...@vger.kernel.org
---
V3 removed the old comment, moved the new comment in place of the old one
   and updated it to better reference the programmers guide
V2 added comment about clearing REQPKT before DATAERR_NAKTIMEOUT

 drivers/usb/musb/musb_host.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index e5b6aba..17421d0 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -994,9 +994,15 @@ static void musb_bulk_nak_timeout(struct musb *musb, 
struct musb_hw_ep *ep,
if (is_in) {
dma = is_dma_capable() ? ep->rx_channel : NULL;
 
-   /* clear nak timeout bit */
+   /*
+* Need to stop the transaction by clearing REQPKT first
+* then the NAK Timeout bit ref MUSBMHDRC USB 2.0 HIGH-SPEED
+* DUAL-ROLE CONTROLLER Programmer's Guide, section 9.2.2
+*/
rx_csr = musb_readw(epio, MUSB_RXCSR);
rx_csr |= MUSB_RXCSR_H_WZC_BITS;
+   rx_csr &= ~MUSB_RXCSR_H_REQPKT;
+   musb_writew(epio, MUSB_RXCSR, rx_csr);
rx_csr &= ~MUSB_RXCSR_DATAERROR;
musb_writew(epio, MUSB_RXCSR, rx_csr);
 
-- 
2.7.4

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

[PATCH V3 1/2] usb: musb: Ensure rx reinit occurs for shared_fifo endpoints

[Andrew Goodbody]

shared_fifo endpoints would only get a previous tx state cleared
out, the rx state was only cleared for non shared_fifo endpoints
Change this so that the rx state is cleared for all endpoints.
This addresses an issue that resulted in rx packets being dropped
silently.

Signed-off-by: Andrew Goodbody 
Cc: sta...@vger.kernel.org
---
V3 no change
V2 removed debugging call

 drivers/usb/musb/musb_host.c | 13 ++---
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index b7a02ce..e5b6aba 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -594,14 +594,13 @@ musb_rx_reinit(struct musb *musb, struct musb_qh *qh, u8 
epnum)
musb_writew(ep->regs, MUSB_TXCSR, 0);
 
/* scrub all previous state, clearing toggle */
-   } else {
-   csr = musb_readw(ep->regs, MUSB_RXCSR);
-   if (csr & MUSB_RXCSR_RXPKTRDY)
-   WARNING("rx%d, packet/%d ready?\n", ep->epnum,
-   musb_readw(ep->regs, MUSB_RXCOUNT));
-
-   musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
}
+   csr = musb_readw(ep->regs, MUSB_RXCSR);
+   if (csr & MUSB_RXCSR_RXPKTRDY)
+   WARNING("rx%d, packet/%d ready?\n", ep->epnum,
+   musb_readw(ep->regs, MUSB_RXCOUNT));
+
+   musb_h_flush_rxfifo(ep, MUSB_RXCSR_CLRDATATOG);
 
/* target addr and (for multipoint) hub addr/port */
if (musb->is_multipoint) {
-- 
2.7.4

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

Re: [PATCH 5/5] usb: dwc3: Endianness issue on dwc3_log_ctrl

[Felipe Balbi]

John Youn  writes:
> Sparse complains even though it looks ok. Probably it cannot detect that
> the wValue, wIndex, and wLength are declared __le16 due to the macro
> magic.

should we fix sparse, instead? Oh well, doesn't hurt applying.

>
> Redeclare them as CPU endianness and make the conversion on assignment.
>
> Signed-off-by: John Youn 
> ---
>  drivers/usb/dwc3/trace.h | 16 
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/usb/dwc3/trace.h b/drivers/usb/dwc3/trace.h
> index 3ac7252..d0461eb 100644
> --- a/drivers/usb/dwc3/trace.h
> +++ b/drivers/usb/dwc3/trace.h
> @@ -85,21 +85,21 @@ DECLARE_EVENT_CLASS(dwc3_log_ctrl,
>   TP_STRUCT__entry(
>   __field(__u8, bRequestType)
>   __field(__u8, bRequest)
> - __field(__le16, wValue)
> - __field(__le16, wIndex)
> - __field(__le16, wLength)
> + __field(__u16, wValue)
> + __field(__u16, wIndex)
> + __field(__u16, wLength)
>   ),
>   TP_fast_assign(
>   __entry->bRequestType = ctrl->bRequestType;
>   __entry->bRequest = ctrl->bRequest;
> - __entry->wValue = ctrl->wValue;
> - __entry->wIndex = ctrl->wIndex;
> - __entry->wLength = ctrl->wLength;
> + __entry->wValue = le16_to_cpu(ctrl->wValue);
> + __entry->wIndex = le16_to_cpu(ctrl->wIndex);
> + __entry->wLength = le16_to_cpu(ctrl->wLength);
>   ),
>   TP_printk("bRequestType %02x bRequest %02x wValue %04x wIndex %04x 
> wLength %d",
>   __entry->bRequestType, __entry->bRequest,
> - le16_to_cpu(__entry->wValue), le16_to_cpu(__entry->wIndex),
> - le16_to_cpu(__entry->wLength)
> + __entry->wValue, __entry->wIndex,
> + __entry->wLength
>   )
>  );
>  
> -- 
> 2.8.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

-- 
balbi


signature.asc
Description: PGP signature

[uas/scsi] untagged commands?

[Tom Yan]

In this commit:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/usb/storage/uas.c?id=198de51dbc3454d95b015ca0a055b673f85f01bb

There is the following comment:
> 1 tag is reserved for untagged commands

So my question is, what exactly are "untagged commands"?

If we need to reserve (only) 1 tag for them, does that mean "untagged
commands" will never be queued? (I mean, not counting the "tagged
commands"; so in the queue there will always be at most 1 "untagged
command" and a certain number, or none, of tagged commands?)

Also, what determines whether a command is tagged or not? Is it merely
depending on how a program (or maybe the kernel) issue the commands?
Or can a device (or maybe the driver) require specific SCSI command(s)
to always be issued untagged?

In any case, is there a way (e.g. a debug option/level of the sd or
the uas driver, etc.) to show/log whether the issued commands are
tagged or untagged?
--
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: core: add debugobjects support for urb object

[changbin . du]

From: "Du, Changbin" 

Add debugobject support to track the life time of struct urb.
This feature help us detect violation of urb operations by
generating a warning message from debugobject core. And we fix
the possible issues at runtime to avoid oops if we can.

I have done some tests with some class drivers, no violation
found in them which is good. Expect this feature can be used
for debugging future problems.

Signed-off-by: Du, Changbin 
---
 drivers/usb/core/hcd.c |   1 +
 drivers/usb/core/urb.c | 117 +++--
 include/linux/usb.h|   8 
 lib/Kconfig.debug  |   8 
 4 files changed, 130 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c
index 34b837a..a8ea128 100644
--- a/drivers/usb/core/hcd.c
+++ b/drivers/usb/core/hcd.c
@@ -1750,6 +1750,7 @@ static void __usb_hcd_giveback_urb(struct urb *urb)
/* pass ownership to the completion handler */
urb->status = status;
 
+   debug_urb_deactivate(urb);
/*
 * We disable local IRQs here avoid possible deadlock because
 * drivers may call spin_lock() to hold lock which might be
diff --git a/drivers/usb/core/urb.c b/drivers/usb/core/urb.c
index c601e25..0d1eccb 100644
--- a/drivers/usb/core/urb.c
+++ b/drivers/usb/core/urb.c
@@ -10,6 +10,100 @@
 
 #define to_urb(d) container_of(d, struct urb, kref)
 
+#ifdef CONFIG_DEBUG_OBJECTS_URB
+static struct debug_obj_descr urb_debug_descr;
+
+static void *urb_debug_hint(void *addr)
+{
+   return ((struct urb *) addr)->complete;
+}
+
+/*
+ * fixup_init is called when:
+ * - an active object is initialized
+ */
+static bool urb_fixup_init(void *addr, enum debug_obj_state state)
+{
+   struct urb *urb = addr;
+
+   switch (state) {
+   case ODEBUG_STATE_ACTIVE:
+   usb_kill_urb(urb);
+   debug_object_init(urb, _debug_descr);
+   return true;
+   default:
+   return false;
+   }
+}
+
+/*
+ * fixup_activate is called when:
+ * - an active object is activated
+ * - an unknown non-static object is activated
+ */
+static bool urb_fixup_activate(void *addr, enum debug_obj_state state)
+{
+   struct urb *urb = urb;
+
+   switch (state) {
+   case ODEBUG_STATE_ACTIVE:
+   usb_kill_urb(urb);
+   debug_object_activate(urb, _debug_descr);
+   return true;
+   default:
+   return false;
+   }
+}
+
+/*
+ * fixup_free is called when:
+ * - an active object is freed
+ */
+static bool urb_fixup_free(void *addr, enum debug_obj_state state)
+{
+   struct urb *urb = addr;
+
+   switch (state) {
+   case ODEBUG_STATE_ACTIVE:
+   usb_kill_urb(urb);
+   debug_object_free(urb, _debug_descr);
+   return true;
+   default:
+   return false;
+   }
+}
+
+static struct debug_obj_descr urb_debug_descr = {
+   .name   = "urb",
+   .debug_hint = urb_debug_hint,
+   .fixup_init = urb_fixup_init,
+   .fixup_activate = urb_fixup_activate,
+   .fixup_free = urb_fixup_free,
+};
+
+static void debug_urb_init(struct urb *urb)
+{
+   /**
+* The struct urb structure must never be
+* created statically, so no init object
+* on stack case.
+*/
+   debug_object_init(urb, _debug_descr);
+}
+
+int debug_urb_activate(struct urb *urb)
+{
+   return debug_object_activate(urb, _debug_descr);
+}
+
+void debug_urb_deactivate(struct urb *urb)
+{
+   debug_object_deactivate(urb, _debug_descr);
+}
+
+#else
+static inline void debug_urb_init(struct urb *urb) { }
+#endif
 
 static void urb_destroy(struct kref *kref)
 {
@@ -41,6 +135,7 @@ void usb_init_urb(struct urb *urb)
memset(urb, 0, sizeof(*urb));
kref_init(>kref);
INIT_LIST_HEAD(>anchor_list);
+   debug_urb_init(urb);
}
 }
 EXPORT_SYMBOL_GPL(usb_init_urb);
@@ -331,6 +426,7 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
struct usb_host_endpoint*ep;
int is_out;
unsigned intallowed;
+   int ret;
 
if (!urb || !urb->complete)
return -EINVAL;
@@ -539,10 +635,23 @@ int usb_submit_urb(struct urb *urb, gfp_t mem_flags)
}
}
 
-   return usb_hcd_submit_urb(urb, mem_flags);
+   ret = debug_urb_activate(urb);
+   if (ret)
+   return ret;
+   ret = usb_hcd_submit_urb(urb, mem_flags);
+   if (ret)
+   debug_urb_deactivate(urb);
+
+   return ret;
 }
 EXPORT_SYMBOL_GPL(usb_submit_urb);
 
+static inline int __usb_unlink_urb(struct urb *urb, int status)
+{
+   debug_urb_deactivate(urb);
+   return usb_hcd_unlink_urb(urb, status);
+}
+
 

Re: [PATCH v2 1/1] uas: remove can_queue set in host template

[Hans de Goede]

Hi,

On 23-05-16 21:28, tom.t...@gmail.com wrote:

From: Tom Yan 

Commit 198de51dbc34 ("USB: uas: Limit qdepth at the scsi-host level") made
qdepth limit set in host template (`.can_queue = MAX_CMNDS`) redundant.
Removing it to avoid confusion.

Signed-off-by: Tom Yan 


I agree removing this is good:

Acked-by: Hans de Goede 

Regards,

Hans




diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c
index 4d49fce..e03c490 100644
--- a/drivers/usb/storage/uas.c
+++ b/drivers/usb/storage/uas.c
@@ -848,7 +848,6 @@ static struct scsi_host_template uas_host_template = {
.slave_configure = uas_slave_configure,
.eh_abort_handler = uas_eh_abort_handler,
.eh_bus_reset_handler = uas_eh_bus_reset_handler,
-   .can_queue = MAX_CMNDS,
.this_id = -1,
.sg_tablesize = SG_NONE,
.skip_settle_delay = 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