Re: [PATCH/RFC 3/6] usb: gadget: Add NO_DMA dummies for DMA mapping API

[Felipe Balbi]

Hi,

Geert Uytterhoeven  writes:
> Add dummies for usb_gadget_{,un}map_request{,_by_dev}(), to allow
> compile-testing if NO_DMA=y.
>
> This prevents the following from showing up later:
>
> ERROR: "usb_gadget_unmap_request_by_dev" 
> [drivers/usb/renesas_usbhs/renesas_usbhs.ko] undefined!
> ERROR: "usb_gadget_map_request_by_dev" 
> [drivers/usb/renesas_usbhs/renesas_usbhs.ko] undefined!
> ERROR: "usb_gadget_map_request" [drivers/usb/mtu3/mtu3.ko] undefined!
> ERROR: "usb_gadget_unmap_request" [drivers/usb/mtu3/mtu3.ko] undefined!
> ERROR: "usb_gadget_map_request" [drivers/usb/gadget/udc/renesas_usb3.ko] 
> undefined!
> ERROR: "usb_gadget_unmap_request" 
> [drivers/usb/gadget/udc/renesas_usb3.ko] undefined!
>
> Signed-off-by: Geert Uytterhoeven 

Should I take this or is it going with the rest of the series? If you
wanna take it through Trivial or something like that:

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: dwc3: drd: Fix lock-up on ID change during system suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Adding/removing host/gadget controller before .pm_complete()
> causes a lock-up. Let's prevent any dual-role state change
> between .pm_prepare() and .pm_complete() to fix this.
>
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/core.c | 31 +++
>  drivers/usb/dwc3/core.h |  5 +
>  drivers/usb/dwc3/drd.c  | 10 ++
>  3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 42379cc..85388dd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1414,6 +1414,33 @@ static int dwc3_runtime_idle(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_PM_SLEEP
> +static int dwc3_prepare(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long   flags;
> +
> + if (dwc->dr_mode == USB_DR_MODE_OTG) {
> + spin_lock_irqsave(>lock, flags);
> + dwc->dr_keep_role = true;
> + spin_unlock_irqrestore(>lock, flags);
> + }
> +
> + return 0;
> +}
> +
> +static void dwc3_complete(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long   flags;
> +
> + if (dwc->dr_mode == USB_DR_MODE_OTG) {
> + spin_lock_irqsave(>lock, flags);
> + dwc->dr_keep_role = false;
> + spin_unlock_irqrestore(>lock, flags);
> + dwc3_drd_update(dwc);
> + }
> +}

wouldn't it be far easier to just disable OTG IRQ in prepare? and,
perhaps, also flush_work_sync() ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 2/2] usb: dwc3: drd: Fix lock-up on ID change during system suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Adding/removing host/gadget controller before .pm_complete()
> causes a lock-up. Let's prevent any dual-role state change
> between .pm_prepare() and .pm_complete() to fix this.
>
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/core.c | 31 +++
>  drivers/usb/dwc3/core.h |  5 +
>  drivers/usb/dwc3/drd.c  | 10 ++
>  3 files changed, 42 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 42379cc..85388dd 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -1414,6 +1414,33 @@ static int dwc3_runtime_idle(struct device *dev)
>  #endif /* CONFIG_PM */
>  
>  #ifdef CONFIG_PM_SLEEP
> +static int dwc3_prepare(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long   flags;
> +
> + if (dwc->dr_mode == USB_DR_MODE_OTG) {
> + spin_lock_irqsave(>lock, flags);
> + dwc->dr_keep_role = true;
> + spin_unlock_irqrestore(>lock, flags);
> + }
> +
> + return 0;
> +}
> +
> +static void dwc3_complete(struct device *dev)
> +{
> + struct dwc3 *dwc = dev_get_drvdata(dev);
> + unsigned long   flags;
> +
> + if (dwc->dr_mode == USB_DR_MODE_OTG) {
> + spin_lock_irqsave(>lock, flags);
> + dwc->dr_keep_role = false;
> + spin_unlock_irqrestore(>lock, flags);
> + dwc3_drd_update(dwc);
> + }
> +}

wouldn't it be far easier to just disable OTG IRQ in prepare? and,
perhaps, also flush_work_sync() ?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Felipe Balbi]

Hi,

Christophe JAILLET  writes:
> This serie aims to fix 2 issues. (path 2 & 4)
>
> The 2nd patch fixes a memory leak. It uses devm_ function a simplify the
> handling of the memory.
>
> The 4th patch fixes a potential invalid pointer dereference.
>
> The 2 other ones, are just clean-ups to remove useless code and add other
> uses of devm_ function to simplify code.
>
> I've left the request_irq/free_irq because I'm unsure of potential side
> effects if some other resources are freed while an IRQ can still be
> triggered. So I've preferred to leave it as-is.
>
> Christophe JAILLET (4):
>   usb: gadget: fotg210-udc: Remove a useless
>   usb: gadget: fotg210-udc: Fix a memory leak
>   usb: gadget: fotg210-udc: Simplify code
>   usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference

you should NEVER make fixes depend on cleanups. It should be the other
way around :-) First fixes, then cleanups. The reason is that fixes can
get accepted during -rc cycle, but cleanups must wait until the next
merge window.

Please fix up your patches, otherwise I'll have to apply the entire
series for v4.17

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/4] usb: gadget: fotg210-udc: Fixes and cleanup

[Felipe Balbi]

Hi,

Christophe JAILLET  writes:
> This serie aims to fix 2 issues. (path 2 & 4)
>
> The 2nd patch fixes a memory leak. It uses devm_ function a simplify the
> handling of the memory.
>
> The 4th patch fixes a potential invalid pointer dereference.
>
> The 2 other ones, are just clean-ups to remove useless code and add other
> uses of devm_ function to simplify code.
>
> I've left the request_irq/free_irq because I'm unsure of potential side
> effects if some other resources are freed while an IRQ can still be
> triggered. So I've preferred to leave it as-is.
>
> Christophe JAILLET (4):
>   usb: gadget: fotg210-udc: Remove a useless
>   usb: gadget: fotg210-udc: Fix a memory leak
>   usb: gadget: fotg210-udc: Simplify code
>   usb: gadget: fotg210-udc: Fix a potential invalid pointer dereference

you should NEVER make fixes depend on cleanups. It should be the other
way around :-) First fixes, then cleanups. The reason is that fixes can
get accepted during -rc cycle, but cleanups must wait until the next
merge window.

Please fix up your patches, otherwise I'll have to apply the entire
series for v4.17

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume

[Felipe Balbi]

Roger Quadros  writes:

> In order for ULPI PHYs to work, dwc3_phy_setup() and dwc3_ulpi_init()
> must be doene before dwc3_core_get_phy().
>
> commit 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> broke this.
>
> The other issue is that dwc3_core_get_phy() and dwc3_ulpi_init() should
> be called only once during the life cycle of the driver. However,
> as dwc3_core_init() is called during system suspend/resume it will
> result in multiple calls to dwc3_core_get_phy() and dwc3_ulpi_init()
> which is wrong.
>
> Fix this by moving dwc3_ulpi_init() out of dwc3_phy_setup()
> into dwc3_core_ulpi_init(). Use a flag 'ulpi_ready' to ensure that
> dwc3_core_ulpi_init() is called only once from dwc3_core_init().
>
> Use another flag 'phys_ready' to call dwc3_core_get_phy() only once from
> dwc3_core_init().
>
> Fixes: 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> Fixes: f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get 
> the PHY")
> Cc: linux-stable  # >= v4.13
> Signed-off-by: Roger Quadros 

unfortunately, this doesn't apply anymore. Care to rebase on
testing/fixes? (give me a couple hours though, I'm still applying
patches :-)

cheers

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc3: core: Fix ULPI PHYs and prevent phy_get/ulpi_init during suspend/resume

[Felipe Balbi]

Roger Quadros  writes:

> In order for ULPI PHYs to work, dwc3_phy_setup() and dwc3_ulpi_init()
> must be doene before dwc3_core_get_phy().
>
> commit 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> broke this.
>
> The other issue is that dwc3_core_get_phy() and dwc3_ulpi_init() should
> be called only once during the life cycle of the driver. However,
> as dwc3_core_init() is called during system suspend/resume it will
> result in multiple calls to dwc3_core_get_phy() and dwc3_ulpi_init()
> which is wrong.
>
> Fix this by moving dwc3_ulpi_init() out of dwc3_phy_setup()
> into dwc3_core_ulpi_init(). Use a flag 'ulpi_ready' to ensure that
> dwc3_core_ulpi_init() is called only once from dwc3_core_init().
>
> Use another flag 'phys_ready' to call dwc3_core_get_phy() only once from
> dwc3_core_init().
>
> Fixes: 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> Fixes: f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get 
> the PHY")
> Cc: linux-stable  # >= v4.13
> Signed-off-by: Roger Quadros 

unfortunately, this doesn't apply anymore. Care to rebase on
testing/fixes? (give me a couple hours though, I'm still applying
patches :-)

cheers

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Kunihiko Hayashi  writes:
> Hello Felipe,
>
> Thank you for your comments.
>
> On Tue, 23 Jan 2018 15:12:36 +0200  wrote:
>
>> 
>> Hi,
>> 
>> Kunihiko Hayashi  writes:
>> > Add a specific glue layer for UniPhier SoC platform to support
>> > USB host mode. It manages hardware operating sequences to enable multiple
>> > clock gates and assert resets, and to prepare to use dwc3 controller
>> > on the SoC.
>> >
>> > This patch also handles the physical layer that has same register space
>> > as the glue layer, because it needs to integrate initialziation sequence
>> > between glue and phy.
>> >
>> > In case of some SoCs, since some initialization values for PHY are
>> > included in nvmem, this patch includes the way to get the values from 
>> > nvmem.
>> >
>> > It supports PXs2 and LD20 SoCs.
>> >
>> > Signed-off-by: Kunihiko Hayashi 
>> > Signed-off-by: Motoya Tanigawa 
>> > Signed-off-by: Masami Hiramatsu 
>> > ---
>> >  drivers/usb/dwc3/Kconfig |   9 +
>> >  drivers/usb/dwc3/Makefile|   1 +
>> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 
>> > +++
>> >  3 files changed, 564 insertions(+)
>> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> ...snip...
>
>> > +
>> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
>> > +   u32 data)
>> 
>> anything with sshphy or hsphy in the name should probably be part of a
>> PHY driver using drivers/phy/ framework.
>
> I can try to separate phy control from this driver.
> However, phy registers belongs to "dwc3-glue" IO map area (65b0),
> and this area also contains a reset bit for "dwc3-core" hardware.
>
> Although the phy driver is called from dwc3-core driver,
> we need to deassert the reset bit before probing dwc3-core driver.
>
> As shown later, I think that it's difficut to determine the order of
> initializing the registers in this area.
>
>> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
>> > +{
>> > +  int i;
>> > +
>> > +  for (i = 0; i < priv->nvbus; i++) {
>> > +  dwc3u_maskwrite(priv, VBUS_CONTROL(i),
>> > +  DRVVBUS_REG_EN | DRVVBUS_REG,
>> > +  DRVVBUS_REG_EN | 0);
>> > +  }
>> > +}
>> 
>> drivers/regulator maybe?
>
> VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> enables vbus connected with "regulator" hardware.
>
> The regulator driver should manage "regulator" hardware, and
> I don't think that the driver should manage this register.
>
>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>> > +{
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +  usleep_range(1000, 2000);
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>> > +}
>> > +
>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>> > +{
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +}
>> 
>> drivers/reset ?
>
> The reset driver manages "sysctrl" IO map area in our SoC.
>
> RESET_CTL register belongs to "dwc3-glue" IO map area,
> and the kernel can't access this area until enabling usb clocks and
> deasserting usb resets in "sysctrl".
>
> I think that "dwc3-glue" register control should be separated from
> "sysctrl".

Just split your address space and treat your glue as a device with
several children:

glue@65b0 {
compatible = "foo"

phy@bar {
...
};

sysctrl@baz {
...
};

dwc3@foo {
compatible = "snps, dwc3";
...
};
};

Then you know that you can let dwc3/core.c handle the PHY for you. If we
need to teach dwc3/core.c about regulators, we can do that. But we don't
need SoC-specific hacks ;-)

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Kunihiko Hayashi  writes:
> Hello Felipe,
>
> Thank you for your comments.
>
> On Tue, 23 Jan 2018 15:12:36 +0200  wrote:
>
>> 
>> Hi,
>> 
>> Kunihiko Hayashi  writes:
>> > Add a specific glue layer for UniPhier SoC platform to support
>> > USB host mode. It manages hardware operating sequences to enable multiple
>> > clock gates and assert resets, and to prepare to use dwc3 controller
>> > on the SoC.
>> >
>> > This patch also handles the physical layer that has same register space
>> > as the glue layer, because it needs to integrate initialziation sequence
>> > between glue and phy.
>> >
>> > In case of some SoCs, since some initialization values for PHY are
>> > included in nvmem, this patch includes the way to get the values from 
>> > nvmem.
>> >
>> > It supports PXs2 and LD20 SoCs.
>> >
>> > Signed-off-by: Kunihiko Hayashi 
>> > Signed-off-by: Motoya Tanigawa 
>> > Signed-off-by: Masami Hiramatsu 
>> > ---
>> >  drivers/usb/dwc3/Kconfig |   9 +
>> >  drivers/usb/dwc3/Makefile|   1 +
>> >  drivers/usb/dwc3/dwc3-uniphier.c | 554 
>> > +++
>> >  3 files changed, 564 insertions(+)
>> >  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> ...snip...
>
>> > +
>> > +static void dwc3u_ssphy_testio_write(struct dwc3u_priv *priv, int port,
>> > +   u32 data)
>> 
>> anything with sshphy or hsphy in the name should probably be part of a
>> PHY driver using drivers/phy/ framework.
>
> I can try to separate phy control from this driver.
> However, phy registers belongs to "dwc3-glue" IO map area (65b0),
> and this area also contains a reset bit for "dwc3-core" hardware.
>
> Although the phy driver is called from dwc3-core driver,
> we need to deassert the reset bit before probing dwc3-core driver.
>
> As shown later, I think that it's difficut to determine the order of
> initializing the registers in this area.
>
>> > +static void dwc3u_vbus_disable(struct dwc3u_priv *priv)
>> > +{
>> > +  int i;
>> > +
>> > +  for (i = 0; i < priv->nvbus; i++) {
>> > +  dwc3u_maskwrite(priv, VBUS_CONTROL(i),
>> > +  DRVVBUS_REG_EN | DRVVBUS_REG,
>> > +  DRVVBUS_REG_EN | 0);
>> > +  }
>> > +}
>> 
>> drivers/regulator maybe?
>
> VBUS_CONTROL register is used for determing whether "dwc3-glue" hardware
> enables vbus connected with "regulator" hardware.
>
> The regulator driver should manage "regulator" hardware, and
> I don't think that the driver should manage this register.
>
>> > +static void dwc3u_reset_init(struct dwc3u_priv *priv)
>> > +{
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +  usleep_range(1000, 2000);
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, LINK_RESET);
>> > +}
>> > +
>> > +static void dwc3u_reset_clear(struct dwc3u_priv *priv)
>> > +{
>> > +  dwc3u_maskwrite(priv, RESET_CTL, LINK_RESET, 0);
>> > +}
>> 
>> drivers/reset ?
>
> The reset driver manages "sysctrl" IO map area in our SoC.
>
> RESET_CTL register belongs to "dwc3-glue" IO map area,
> and the kernel can't access this area until enabling usb clocks and
> deasserting usb resets in "sysctrl".
>
> I think that "dwc3-glue" register control should be separated from
> "sysctrl".

Just split your address space and treat your glue as a device with
several children:

glue@65b0 {
compatible = "foo"

phy@bar {
...
};

sysctrl@baz {
...
};

dwc3@foo {
compatible = "snps, dwc3";
...
};
};

Then you know that you can let dwc3/core.c handle the PHY for you. If we
need to teach dwc3/core.c about regulators, we can do that. But we don't
need SoC-specific hacks ;-)

-- 
balbi


signature.asc
Description: PGP signature

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

[Felipe Balbi]

Hi,

Kunihiko Hayashi  writes:
> Add a specific glue layer for UniPhier SoC platform to support
> USB host mode. It manages hardware operating sequences to enable multiple
> clock gates and assert resets, and to prepare to use dwc3 controller
> on the SoC.
>
> This patch also handles the physical layer that has same register space
> as the glue layer, because it needs to integrate initialziation sequence
> between glue and phy.
>
> In case of some SoCs, since some initialization values for PHY are
> included in nvmem, this patch includes the way to get the values from nvmem.
>
> It supports PXs2 and LD20 SoCs.
>
> Signed-off-by: Kunihiko Hayashi 
> Signed-off-by: Motoya Tanigawa 
> Signed-off-by: Masami Hiramatsu 
> ---
>  drivers/usb/dwc3/Kconfig |   9 +
>  drivers/usb/dwc3/Makefile|   1 +
>  drivers/usb/dwc3/dwc3-uniphier.c | 554 
> +++
>  3 files changed, 564 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index ab8c0e0..a5cadc6 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -106,4 +106,13 @@ config USB_DWC3_ST
> inside (i.e. STiH407).
> Say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_UNIPHIER
> + tristate "Socionext UniPhier Platforms"
> + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> + default USB_DWC3
> + help
> +   Support USB2/3 functionality in UniPhier platforms.
> +   Say 'Y' or 'M' if your system that UniPhier SoC is implemented
> +   has USB controllers based on DWC USB3 IP.
> +
>  endif
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 7ac7250..31e82b3 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_USB_DWC3_PCI)  += dwc3-pci.o
>  obj-$(CONFIG_USB_DWC3_KEYSTONE)  += dwc3-keystone.o
>  obj-$(CONFIG_USB_DWC3_OF_SIMPLE) += dwc3-of-simple.o
>  obj-$(CONFIG_USB_DWC3_ST)+= dwc3-st.o
> +obj-$(CONFIG_USB_DWC3_UNIPHIER)  += dwc3-uniphier.o
> diff --git a/drivers/usb/dwc3/dwc3-uniphier.c 
> b/drivers/usb/dwc3/dwc3-uniphier.c
> new file mode 100644
> index 000..58e84cd
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-uniphier.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * dwc3-uniphier.c - Socionext UniPhier DWC3 specific glue layer
> + *
> + * Copyright 2015-2018 Socionext Inc.
> + *
> + * Author:
> + *   Kunihiko Hayashi 
> + * Contributors:
> + *  Motoya Tanigawa 
> + *  Masami Hiramatsu 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define RESET_CTL0x000
> +#define LINK_RESET   BIT(15)
> +
> +#define VBUS_CONTROL(n)  (0x100 + 0x10 * (n))
> +#define DRVVBUS_REG  BIT(4)
> +#define DRVVBUS_REG_EN   BIT(3)
> +
> +#define U2PHY_CFG0(n)(0x200 + 0x10 * (n))
> +#define U2PHY_CFG0_HS_I_MASK GENMASK(31, 28)
> +#define U2PHY_CFG0_HSDISC_MASK   GENMASK(27, 26)
> +#define U2PHY_CFG0_SWING_MASKGENMASK(17, 16)
> +#define U2PHY_CFG0_SEL_T_MASKGENMASK(15, 12)
> +#define U2PHY_CFG0_RTERM_MASKGENMASK(7, 6)
> +#define U2PHY_CFG0_TRIMMASK  (U2PHY_CFG0_HS_I_MASK \
> +  | U2PHY_CFG0_SEL_T_MASK \
> +  | U2PHY_CFG0_RTERM_MASK)
> +
> +#define U2PHY_CFG1(n)(0x204 + 0x10 * (n))
> +#define U2PHY_CFG1_DAT_ENBIT(29)
> +#define U2PHY_CFG1_ADR_ENBIT(28)
> +#define U2PHY_CFG1_ADR_MASK  GENMASK(27, 16)
> +#define U2PHY_CFG1_DAT_MASK  GENMASK(23, 16)
> +
> +#define U3PHY_TESTI(n)   (0x300 + 0x10 * (n))
> +#define U3PHY_TESTO(n)   (0x304 + 0x10 * (n))
> +#define TESTI_DAT_MASK   GENMASK(13, 6)
> +#define TESTI_ADR_MASK   GENMASK(5, 1)
> +#define TESTI_WR_EN  BIT(0)
> +
> +#define HOST_CONFIG0 0x400
> +#define NUM_U3_MASK  GENMASK(13, 11)
> +#define NUM_U2_MASK  GENMASK(10, 8)
> +
> +#define PHY_MAX_PARAMS   32
> +
> +struct dwc3u_phy_param {
> + u32 addr;
> + u32 mask;
> + u32 val;
> +};
> +
> +struct dwc3u_trim_param {
> + u32 rterm;
> + u32 sel_t;
> + u32 hs_i;
> +};
> +
> +#define trim_param_is_valid(p)   ((p)->rterm || (p)->sel_t || (p)->hs_i)
> +
> +struct dwc3u_priv {
> + struct device   *dev;
> + void __iomem*base;
> + struct clk  **clks;
> + int nclks;
> + struct reset_control*rst;
> + int

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

[Felipe Balbi]

Hi,

Kunihiko Hayashi  writes:
> Add a specific glue layer for UniPhier SoC platform to support
> USB host mode. It manages hardware operating sequences to enable multiple
> clock gates and assert resets, and to prepare to use dwc3 controller
> on the SoC.
>
> This patch also handles the physical layer that has same register space
> as the glue layer, because it needs to integrate initialziation sequence
> between glue and phy.
>
> In case of some SoCs, since some initialization values for PHY are
> included in nvmem, this patch includes the way to get the values from nvmem.
>
> It supports PXs2 and LD20 SoCs.
>
> Signed-off-by: Kunihiko Hayashi 
> Signed-off-by: Motoya Tanigawa 
> Signed-off-by: Masami Hiramatsu 
> ---
>  drivers/usb/dwc3/Kconfig |   9 +
>  drivers/usb/dwc3/Makefile|   1 +
>  drivers/usb/dwc3/dwc3-uniphier.c | 554 
> +++
>  3 files changed, 564 insertions(+)
>  create mode 100644 drivers/usb/dwc3/dwc3-uniphier.c
>
> diff --git a/drivers/usb/dwc3/Kconfig b/drivers/usb/dwc3/Kconfig
> index ab8c0e0..a5cadc6 100644
> --- a/drivers/usb/dwc3/Kconfig
> +++ b/drivers/usb/dwc3/Kconfig
> @@ -106,4 +106,13 @@ config USB_DWC3_ST
> inside (i.e. STiH407).
> Say 'Y' or 'M' if you have one such device.
>  
> +config USB_DWC3_UNIPHIER
> + tristate "Socionext UniPhier Platforms"
> + depends on (ARCH_UNIPHIER || COMPILE_TEST) && OF
> + default USB_DWC3
> + help
> +   Support USB2/3 functionality in UniPhier platforms.
> +   Say 'Y' or 'M' if your system that UniPhier SoC is implemented
> +   has USB controllers based on DWC USB3 IP.
> +
>  endif
> diff --git a/drivers/usb/dwc3/Makefile b/drivers/usb/dwc3/Makefile
> index 7ac7250..31e82b3 100644
> --- a/drivers/usb/dwc3/Makefile
> +++ b/drivers/usb/dwc3/Makefile
> @@ -48,3 +48,4 @@ obj-$(CONFIG_USB_DWC3_PCI)  += dwc3-pci.o
>  obj-$(CONFIG_USB_DWC3_KEYSTONE)  += dwc3-keystone.o
>  obj-$(CONFIG_USB_DWC3_OF_SIMPLE) += dwc3-of-simple.o
>  obj-$(CONFIG_USB_DWC3_ST)+= dwc3-st.o
> +obj-$(CONFIG_USB_DWC3_UNIPHIER)  += dwc3-uniphier.o
> diff --git a/drivers/usb/dwc3/dwc3-uniphier.c 
> b/drivers/usb/dwc3/dwc3-uniphier.c
> new file mode 100644
> index 000..58e84cd
> --- /dev/null
> +++ b/drivers/usb/dwc3/dwc3-uniphier.c
> @@ -0,0 +1,554 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/**
> + * dwc3-uniphier.c - Socionext UniPhier DWC3 specific glue layer
> + *
> + * Copyright 2015-2018 Socionext Inc.
> + *
> + * Author:
> + *   Kunihiko Hayashi 
> + * Contributors:
> + *  Motoya Tanigawa 
> + *  Masami Hiramatsu 
> + */
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +#define RESET_CTL0x000
> +#define LINK_RESET   BIT(15)
> +
> +#define VBUS_CONTROL(n)  (0x100 + 0x10 * (n))
> +#define DRVVBUS_REG  BIT(4)
> +#define DRVVBUS_REG_EN   BIT(3)
> +
> +#define U2PHY_CFG0(n)(0x200 + 0x10 * (n))
> +#define U2PHY_CFG0_HS_I_MASK GENMASK(31, 28)
> +#define U2PHY_CFG0_HSDISC_MASK   GENMASK(27, 26)
> +#define U2PHY_CFG0_SWING_MASKGENMASK(17, 16)
> +#define U2PHY_CFG0_SEL_T_MASKGENMASK(15, 12)
> +#define U2PHY_CFG0_RTERM_MASKGENMASK(7, 6)
> +#define U2PHY_CFG0_TRIMMASK  (U2PHY_CFG0_HS_I_MASK \
> +  | U2PHY_CFG0_SEL_T_MASK \
> +  | U2PHY_CFG0_RTERM_MASK)
> +
> +#define U2PHY_CFG1(n)(0x204 + 0x10 * (n))
> +#define U2PHY_CFG1_DAT_ENBIT(29)
> +#define U2PHY_CFG1_ADR_ENBIT(28)
> +#define U2PHY_CFG1_ADR_MASK  GENMASK(27, 16)
> +#define U2PHY_CFG1_DAT_MASK  GENMASK(23, 16)
> +
> +#define U3PHY_TESTI(n)   (0x300 + 0x10 * (n))
> +#define U3PHY_TESTO(n)   (0x304 + 0x10 * (n))
> +#define TESTI_DAT_MASK   GENMASK(13, 6)
> +#define TESTI_ADR_MASK   GENMASK(5, 1)
> +#define TESTI_WR_EN  BIT(0)
> +
> +#define HOST_CONFIG0 0x400
> +#define NUM_U3_MASK  GENMASK(13, 11)
> +#define NUM_U2_MASK  GENMASK(10, 8)
> +
> +#define PHY_MAX_PARAMS   32
> +
> +struct dwc3u_phy_param {
> + u32 addr;
> + u32 mask;
> + u32 val;
> +};
> +
> +struct dwc3u_trim_param {
> + u32 rterm;
> + u32 sel_t;
> + u32 hs_i;
> +};
> +
> +#define trim_param_is_valid(p)   ((p)->rterm || (p)->sel_t || (p)->hs_i)
> +
> +struct dwc3u_priv {
> + struct device   *dev;
> + void __iomem*base;
> + struct clk  **clks;
> + int nclks;
> + struct reset_control*rst;
> + int nvbus;
> + const struct dwc3u_soc_data *data;
> +};
> +
> +struct dwc3u_soc_data {
> + int ss_nparams;
> + struct dwc3u_phy_param ss_param[PHY_MAX_PARAMS];
> + int hs_nparams;
> + struct 

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>
> we can get_phy in dwc3_core_init() as it will get called on resume().
> This was the $subject of this patch.

 indeed. thanks :-)

>>>
>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). 
>>> :P
>> 
>> bit of a chicken-and-egg problem. We need to setup the PHY interface
>> before getting the PHYs, but can't get PHY during resume. Maybe the best
>> way here would be to check for the pointers being valid. Something like:
>> 
>> if (!phy)
>>  get_phy();
>> 
>
> OK that should take care of not calling get_phy() on suspend.
> However there is one more issue with the approach
>
>> @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>  dwc->maximum_speed = USB_SPEED_HIGH;
>>  }
>>  
>> -ret = dwc3_core_get_phy(dwc);
>> +ret = dwc3_phy_setup(dwc);
>>  if (ret)
>>  goto err0;
>
> here we configure PHY related bits and register the ulpi interface.
>
>>  
>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>>  if (ret)
>>  goto err0;
>>  
>
> we got the PHYs. all OK here.
>
>> -ret = dwc3_phy_setup(dwc);
>> +ret = dwc3_core_soft_reset(dwc);
>>  if (ret)
>>  goto err0;
>
> Now we do a soft reset. This means we loose the PHY configuration bits that 
> we did
> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not 
> re-register the ulpi interface.
> I can use a flag there so that dwc3_ulpi_init() is done only once.

sounds like it's better to extract out a smaller function that just
checks if we need ULPI bus and registers it, something akin to:

@@ -482,6 +482,21 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
 
+static int dwc3_ulpi_init(struct dwc3 *dwc)
+{
+   int intf;
+
+   intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);
+
+   if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||
+   (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&
+   dwc->hsphy_interface &&
+   !strncmp(dwc->hsphy_interface, "ulpi", 
4)))
+   return dwc3_ulpi_init(dwc);
+
+   return 0;
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -563,11 +578,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
break;
}
/* FALLTHROUGH */
-   case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
-   ret = dwc3_ulpi_init(dwc);
-   if (ret)
-   return ret;
-   /* FALLTHROUGH */
default:
break;
}

Then we just call that outside of any functions that get called during PM.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>
> we can get_phy in dwc3_core_init() as it will get called on resume().
> This was the $subject of this patch.

 indeed. thanks :-)

>>>
>>> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). 
>>> :P
>> 
>> bit of a chicken-and-egg problem. We need to setup the PHY interface
>> before getting the PHYs, but can't get PHY during resume. Maybe the best
>> way here would be to check for the pointers being valid. Something like:
>> 
>> if (!phy)
>>  get_phy();
>> 
>
> OK that should take care of not calling get_phy() on suspend.
> However there is one more issue with the approach
>
>> @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>  dwc->maximum_speed = USB_SPEED_HIGH;
>>  }
>>  
>> -ret = dwc3_core_get_phy(dwc);
>> +ret = dwc3_phy_setup(dwc);
>>  if (ret)
>>  goto err0;
>
> here we configure PHY related bits and register the ulpi interface.
>
>>  
>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>>  if (ret)
>>  goto err0;
>>  
>
> we got the PHYs. all OK here.
>
>> -ret = dwc3_phy_setup(dwc);
>> +ret = dwc3_core_soft_reset(dwc);
>>  if (ret)
>>  goto err0;
>
> Now we do a soft reset. This means we loose the PHY configuration bits that 
> we did
> in dwc3_phy_setup. So we need to call dwc3_phy_setup again but not 
> re-register the ulpi interface.
> I can use a flag there so that dwc3_ulpi_init() is done only once.

sounds like it's better to extract out a smaller function that just
checks if we need ULPI bus and registers it, something akin to:

@@ -482,6 +482,21 @@ static void dwc3_cache_hwparams(struct dwc3 *dwc)
parms->hwparams8 = dwc3_readl(dwc->regs, DWC3_GHWPARAMS8);
 }
 
+static int dwc3_ulpi_init(struct dwc3 *dwc)
+{
+   int intf;
+
+   intf = DWC3_GHWPARAMS3_HSPHY_IFC(dwc->hwparams.hwparams3);
+
+   if (intf == DWC3_GHWPARAMS3_HSPHY_IFC_ULPI ||
+   (intf == DWC3_GHWPARAMS3_HSPHY_IFC_UTMI_ULPI &&
+   dwc->hsphy_interface &&
+   !strncmp(dwc->hsphy_interface, "ulpi", 
4)))
+   return dwc3_ulpi_init(dwc);
+
+   return 0;
+}
+
 /**
  * dwc3_phy_setup - Configure USB PHY Interface of DWC3 Core
  * @dwc: Pointer to our controller context structure
@@ -563,11 +578,6 @@ static int dwc3_phy_setup(struct dwc3 *dwc)
break;
}
/* FALLTHROUGH */
-   case DWC3_GHWPARAMS3_HSPHY_IFC_ULPI:
-   ret = dwc3_ulpi_init(dwc);
-   if (ret)
-   return ret;
-   /* FALLTHROUGH */
default:
break;
}

Then we just call that outside of any functions that get called during PM.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> In host mode runtime suspend/resume could happen very often with
>>> device connected, and resetting h/w on every runtime_resume might not
>>> be desired. And PHYs drivers can also support runtime_suspend which
>>> would be preferred instead of shutting down phy.
>> 
>> We don't do anything when dwc3 is working as a host, we simply assume if
>> we reach dwc3.ko, xhci has done its part. Here's what our
>> suspend_common looks like:
>> 
>> static int dwc3_suspend_common(struct dwc3 *dwc)
>> {
>>  unsigned long   flags;
>> 
>>  switch (dwc->current_dr_role) {
>>  case DWC3_GCTL_PRTCAP_DEVICE:
>>  spin_lock_irqsave(>lock, flags);
>>  dwc3_gadget_suspend(dwc);
>>  spin_unlock_irqrestore(>lock, flags);
>>  dwc3_core_exit(dwc);
>>  break;
>>  case DWC3_GCTL_PRTCAP_HOST:
>>  default:
>>  /* do nothing */
>>  break;
>>  }
>> 
>>  return 0;
>> }
>> 
>> We're not resetting anything, not tearing down anything. No idea why
>> you're saying that in host mode we're breaking things apart. If you have
>> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
>> mainline is at fault.
>> 
>
> This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't
> reinitialize core during host bus-suspend/resume") which is breaking
> low power for TI platforms in Host mode.
>
> If we revert that commit we will be doing dwc3_core_exit() for host
> mode as well. Which is what we want for system suspend but probably
> not for runtime suspend in host case.
>
> This is why Manu wants to differentiate runtime vs system suspend.

We already differentiate them. Maybe the only thing we need is to *not*
call core_exit() in host-mode during runtime_suspend, but call it if
mode is device or during system sleep.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>>> In host mode runtime suspend/resume could happen very often with
>>> device connected, and resetting h/w on every runtime_resume might not
>>> be desired. And PHYs drivers can also support runtime_suspend which
>>> would be preferred instead of shutting down phy.
>> 
>> We don't do anything when dwc3 is working as a host, we simply assume if
>> we reach dwc3.ko, xhci has done its part. Here's what our
>> suspend_common looks like:
>> 
>> static int dwc3_suspend_common(struct dwc3 *dwc)
>> {
>>  unsigned long   flags;
>> 
>>  switch (dwc->current_dr_role) {
>>  case DWC3_GCTL_PRTCAP_DEVICE:
>>  spin_lock_irqsave(>lock, flags);
>>  dwc3_gadget_suspend(dwc);
>>  spin_unlock_irqrestore(>lock, flags);
>>  dwc3_core_exit(dwc);
>>  break;
>>  case DWC3_GCTL_PRTCAP_HOST:
>>  default:
>>  /* do nothing */
>>  break;
>>  }
>> 
>>  return 0;
>> }
>> 
>> We're not resetting anything, not tearing down anything. No idea why
>> you're saying that in host mode we're breaking things apart. If you have
>> out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
>> mainline is at fault.
>> 
>
> This is the case after commit 689bf72c6e0d ("usb: dwc3: Don't
> reinitialize core during host bus-suspend/resume") which is breaking
> low power for TI platforms in Host mode.
>
> If we revert that commit we will be doing dwc3_core_exit() for host
> mode as well. Which is what we want for system suspend but probably
> not for runtime suspend in host case.
>
> This is why Manu wants to differentiate runtime vs system suspend.

We already differentiate them. Maybe the only thing we need is to *not*
call core_exit() in host-mode during runtime_suspend, but call it if
mode is device or during system sleep.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Manu Gautam  writes:
 On 27/09/17 14:19, Manu Gautam wrote:
> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
> resume. While this works fine for gadget mode but in host
> mode there is not re-initialization of host stack. Also, resetting
> bus as part of bus_suspend/resume is not correct which could affect
> (or disconnect) connected devices.
> Fix this by not reinitializing core on suspend/resume in host mode
> for HOST only and OTG/drd configurations.
>
 All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
 called
 during dwc3_suspend() to have the lowest power state for our platforms.

 After this patch, DWC3 controller and PHYs won't be turned off thus
 preventing our platform from reaching low power levels.

 So this is a regression for us (TI) in v4.15-rc.

 Felipe, do you agree?

 If yes I can send a patch which fixes the regression
 and also makes USB host work after suspend/resume.

>>> I think it will be better to separate runtime_suspend and pm_suspend 
>>> handling for
>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across 
>>> system
>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>> correct.
>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>
>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>> reverting this which affects runtime PM with dwc3 host.
>> nope, that would break platforms using hibernation
>
> Please don't mind me asking this if it is very basic, I am probably
> missing something there
>
> We should be able to distinguish between runtime_pm vs
> system_suspend/hibernation and then process accordingly.

I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
Hibernation feature (open up your databook and have a read ;-)

> In host mode runtime suspend/resume could happen very often with
> device connected, and resetting h/w on every runtime_resume might not
> be desired. And PHYs drivers can also support runtime_suspend which
> would be preferred instead of shutting down phy.

We don't do anything when dwc3 is working as a host, we simply assume if
we reach dwc3.ko, xhci has done its part. Here's what our
suspend_common looks like:

static int dwc3_suspend_common(struct dwc3 *dwc)
{
unsigned long   flags;

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
spin_lock_irqsave(>lock, flags);
dwc3_gadget_suspend(dwc);
spin_unlock_irqrestore(>lock, flags);
dwc3_core_exit(dwc);
break;
case DWC3_GCTL_PRTCAP_HOST:
default:
/* do nothing */
break;
}

return 0;
}

We're not resetting anything, not tearing down anything. No idea why
you're saying that in host mode we're breaking things apart. If you have
out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
mainline is at fault.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Manu Gautam  writes:
 On 27/09/17 14:19, Manu Gautam wrote:
> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
> resume. While this works fine for gadget mode but in host
> mode there is not re-initialization of host stack. Also, resetting
> bus as part of bus_suspend/resume is not correct which could affect
> (or disconnect) connected devices.
> Fix this by not reinitializing core on suspend/resume in host mode
> for HOST only and OTG/drd configurations.
>
 All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
 called
 during dwc3_suspend() to have the lowest power state for our platforms.

 After this patch, DWC3 controller and PHYs won't be turned off thus
 preventing our platform from reaching low power levels.

 So this is a regression for us (TI) in v4.15-rc.

 Felipe, do you agree?

 If yes I can send a patch which fixes the regression
 and also makes USB host work after suspend/resume.

>>> I think it will be better to separate runtime_suspend and pm_suspend 
>>> handling for
>>> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across 
>>> system
>>> suspend-resume should be ok but doing that for runtime suspend-resume is not
>>> correct.
>> it sure is. It's part of hibernation-while-disconnected programming sequence
>>
>>> Let me know if that sounds ok, I can provide a patch for same instead of
>>> reverting this which affects runtime PM with dwc3 host.
>> nope, that would break platforms using hibernation
>
> Please don't mind me asking this if it is very basic, I am probably
> missing something there
>
> We should be able to distinguish between runtime_pm vs
> system_suspend/hibernation and then process accordingly.

I'm not talking about Linux suspend to disk; I'm talking about Synopsys'
Hibernation feature (open up your databook and have a read ;-)

> In host mode runtime suspend/resume could happen very often with
> device connected, and resetting h/w on every runtime_resume might not
> be desired. And PHYs drivers can also support runtime_suspend which
> would be preferred instead of shutting down phy.

We don't do anything when dwc3 is working as a host, we simply assume if
we reach dwc3.ko, xhci has done its part. Here's what our
suspend_common looks like:

static int dwc3_suspend_common(struct dwc3 *dwc)
{
unsigned long   flags;

switch (dwc->current_dr_role) {
case DWC3_GCTL_PRTCAP_DEVICE:
spin_lock_irqsave(>lock, flags);
dwc3_gadget_suspend(dwc);
spin_unlock_irqrestore(>lock, flags);
dwc3_core_exit(dwc);
break;
case DWC3_GCTL_PRTCAP_HOST:
default:
/* do nothing */
break;
}

return 0;
}

We're not resetting anything, not tearing down anything. No idea why
you're saying that in host mode we're breaking things apart. If you have
out-of-tree patches on top of v4.15-rc7, fix them instead of claiming
mainline is at fault.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
 -  ret = dwc3_core_soft_reset(dwc);
 +  ret = dwc3_core_get_phy(dwc);
>>>
>>> we can get_phy in dwc3_core_init() as it will get called on resume().
>>> This was the $subject of this patch.
>> 
>> indeed. thanks :-)
>> 
>
> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). :P

bit of a chicken-and-egg problem. We need to setup the PHY interface
before getting the PHYs, but can't get PHY during resume. Maybe the best
way here would be to check for the pointers being valid. Something like:

if (!phy)
get_phy();

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
 -  ret = dwc3_core_soft_reset(dwc);
 +  ret = dwc3_core_get_phy(dwc);
>>>
>>> we can get_phy in dwc3_core_init() as it will get called on resume().
>>> This was the $subject of this patch.
>> 
>> indeed. thanks :-)
>> 
>
> oops sorry. I meant we can't call dwc3_core_get_phy() in dwc3_core_init(). :P

bit of a chicken-and-egg problem. We need to setup the PHY interface
before getting the PHYs, but can't get PHY during resume. Maybe the best
way here would be to check for the pointers being valid. Something like:

if (!phy)
get_phy();

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Manu Gautam  writes:
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
>> called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
>
> I think it will be better to separate runtime_suspend and pm_suspend handling 
> for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.

it sure is. It's part of hibernation-while-disconnected programming sequence

> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.

nope, that would break platforms using hibernation

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Manu Gautam  writes:
>> On 27/09/17 14:19, Manu Gautam wrote:
>>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>>> resume. While this works fine for gadget mode but in host
>>> mode there is not re-initialization of host stack. Also, resetting
>>> bus as part of bus_suspend/resume is not correct which could affect
>>> (or disconnect) connected devices.
>>> Fix this by not reinitializing core on suspend/resume in host mode
>>> for HOST only and OTG/drd configurations.
>>>
>> All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
>> called
>> during dwc3_suspend() to have the lowest power state for our platforms.
>>
>> After this patch, DWC3 controller and PHYs won't be turned off thus
>> preventing our platform from reaching low power levels.
>>
>> So this is a regression for us (TI) in v4.15-rc.
>>
>> Felipe, do you agree?
>>
>> If yes I can send a patch which fixes the regression
>> and also makes USB host work after suspend/resume.
>>
>
> I think it will be better to separate runtime_suspend and pm_suspend handling 
> for
> host mode in dwc3. Powering offf/on PHYs and dwc3_core_exit/init across system
> suspend-resume should be ok but doing that for runtime suspend-resume is not
> correct.

it sure is. It's part of hibernation-while-disconnected programming sequence

> Let me know if that sounds ok, I can provide a patch for same instead of
> reverting this which affects runtime PM with dwc3 host.

nope, that would break platforms using hibernation

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>>> Felipe,
>>>
>>> On 10/01/18 15:11, Roger Quadros wrote:
 The USB PHYs should be requested only once during the life cycle of
 this driver.

 As dwc3_core_init() is called during system suspend/resume
 it will result in multiple calls to dwc3_core_get_phy() which is wrong.

 To prevent that let's move dwc3_core_get_phy() call
 outside dwc3_core_init().

 Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
 initializing phys")
 Cc: linux-stable  # >= v4.13
 Signed-off-by: Roger Quadros 
>>>
>>> FYI. this patch brings the code back to
>>> revert 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
>>> initializing phys")
>>> revert f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get 
>>> the PHY")
>>>
>>> So looks like this will break ULPI PHY case?
>>>
>>> Where do we initialize ULPI PHY, in dwc3_phy_setup()?
>>>
>>> if so then 541768b08a40 breaks the ULPI PHY case as well, right?
>> 
>> indeed, that commit regressed ULPI PHYs :-(
>> 
>> Seems like it should be more like below:
>> 
>> @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>  dwc->maximum_speed = USB_SPEED_HIGH;
>>  }
>>  
>> -ret = dwc3_core_get_phy(dwc);
>> +ret = dwc3_phy_setup(dwc);
>
> But can we do a dwc3_phy_setup() without doing the soft reset of the 
> controller first?

as long as clocks are running, we can do that, yes.

>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>
> we can get_phy in dwc3_core_init() as it will get called on resume().
> This was the $subject of this patch.

indeed. thanks :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
>> Roger Quadros  writes:
>>> Felipe,
>>>
>>> On 10/01/18 15:11, Roger Quadros wrote:
 The USB PHYs should be requested only once during the life cycle of
 this driver.

 As dwc3_core_init() is called during system suspend/resume
 it will result in multiple calls to dwc3_core_get_phy() which is wrong.

 To prevent that let's move dwc3_core_get_phy() call
 outside dwc3_core_init().

 Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
 initializing phys")
 Cc: linux-stable  # >= v4.13
 Signed-off-by: Roger Quadros 
>>>
>>> FYI. this patch brings the code back to
>>> revert 541768b08a40 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
>>> initializing phys")
>>> revert f54edb539c11 ("usb: dwc3: core: initialize ULPI before trying to get 
>>> the PHY")
>>>
>>> So looks like this will break ULPI PHY case?
>>>
>>> Where do we initialize ULPI PHY, in dwc3_phy_setup()?
>>>
>>> if so then 541768b08a40 breaks the ULPI PHY case as well, right?
>> 
>> indeed, that commit regressed ULPI PHYs :-(
>> 
>> Seems like it should be more like below:
>> 
>> @@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
>>  dwc->maximum_speed = USB_SPEED_HIGH;
>>  }
>>  
>> -ret = dwc3_core_get_phy(dwc);
>> +ret = dwc3_phy_setup(dwc);
>
> But can we do a dwc3_phy_setup() without doing the soft reset of the 
> controller first?

as long as clocks are running, we can do that, yes.

>> -ret = dwc3_core_soft_reset(dwc);
>> +ret = dwc3_core_get_phy(dwc);
>
> we can get_phy in dwc3_core_init() as it will get called on resume().
> This was the $subject of this patch.

indeed. thanks :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Felipe,
>
> On 10/01/18 15:11, Roger Quadros wrote:
>> The USB PHYs should be requested only once during the life cycle of
>> this driver.
>> 
>> As dwc3_core_init() is called during system suspend/resume
>> it will result in multiple calls to dwc3_core_get_phy() which is wrong.
>> 
>> To prevent that let's move dwc3_core_get_phy() call
>> outside dwc3_core_init().
>> 
>> Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
>> initializing phys")
>> Cc: linux-stable  # >= v4.13
>> Signed-off-by: Roger Quadros 
>
> FYI. this patch brings the code back to
> revert 541768b08a40   ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> revert f54edb539c11   ("usb: dwc3: core: initialize ULPI before trying to get 
> the PHY")
>
> So looks like this will break ULPI PHY case?
>
> Where do we initialize ULPI PHY, in dwc3_phy_setup()?
>
> if so then 541768b08a40 breaks the ULPI PHY case as well, right?

indeed, that commit regressed ULPI PHYs :-(

Seems like it should be more like below:

@@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc->maximum_speed = USB_SPEED_HIGH;
}
 
-   ret = dwc3_core_get_phy(dwc);
+   ret = dwc3_phy_setup(dwc);
if (ret)
goto err0;
 
-   ret = dwc3_core_soft_reset(dwc);
+   ret = dwc3_core_get_phy(dwc);
if (ret)
goto err0;
 
-   ret = dwc3_phy_setup(dwc);
+   ret = dwc3_core_soft_reset(dwc);
if (ret)
goto err0;
 
And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to
make the name match what the function actually does. Can you check that
it won't regress the case reported by Carlos? If that works, then we
would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and
dwc3_core_get_phy() outside of dwc3_core_init(), which would mean
duplicated code in suspend/resume handlers.

I'm sure we can sort that out in another way; but the proper order is:

-> initialize ULPI (if necessary)
-> get phy
-> soft reset

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: core: Don't try to get PHYs during suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Felipe,
>
> On 10/01/18 15:11, Roger Quadros wrote:
>> The USB PHYs should be requested only once during the life cycle of
>> this driver.
>> 
>> As dwc3_core_init() is called during system suspend/resume
>> it will result in multiple calls to dwc3_core_get_phy() which is wrong.
>> 
>> To prevent that let's move dwc3_core_get_phy() call
>> outside dwc3_core_init().
>> 
>> Fixes: 541768b08a4 ("usb: dwc3: core: Call dwc3_core_get_phy() before 
>> initializing phys")
>> Cc: linux-stable  # >= v4.13
>> Signed-off-by: Roger Quadros 
>
> FYI. this patch brings the code back to
> revert 541768b08a40   ("usb: dwc3: core: Call dwc3_core_get_phy() before 
> initializing phys")
> revert f54edb539c11   ("usb: dwc3: core: initialize ULPI before trying to get 
> the PHY")
>
> So looks like this will break ULPI PHY case?
>
> Where do we initialize ULPI PHY, in dwc3_phy_setup()?
>
> if so then 541768b08a40 breaks the ULPI PHY case as well, right?

indeed, that commit regressed ULPI PHYs :-(

Seems like it should be more like below:

@@ -754,15 +754,15 @@ static int dwc3_core_init(struct dwc3 *dwc)
dwc->maximum_speed = USB_SPEED_HIGH;
}
 
-   ret = dwc3_core_get_phy(dwc);
+   ret = dwc3_phy_setup(dwc);
if (ret)
goto err0;
 
-   ret = dwc3_core_soft_reset(dwc);
+   ret = dwc3_core_get_phy(dwc);
if (ret)
goto err0;
 
-   ret = dwc3_phy_setup(dwc);
+   ret = dwc3_core_soft_reset(dwc);
if (ret)
goto err0;
 
And maybe we rename dwc3_phy_setup() to dwc3_phy_intf_config() just to
make the name match what the function actually does. Can you check that
it won't regress the case reported by Carlos? If that works, then we
would have to move BOTH dwc3_phy_setup() (dwc3_phy_intf_config()) and
dwc3_core_get_phy() outside of dwc3_core_init(), which would mean
duplicated code in suspend/resume handlers.

I'm sure we can sort that out in another way; but the proper order is:

-> initialize ULPI (if necessary)
-> get phy
-> soft reset

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>> 
>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
> called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.

A power consumption regression is still a regression. But it's super
late in the -rc cycle, I think we need to get this merged after 4.16-rc1
and make sure to Cc stable.

Should be more than enough time to review and test patches.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH 1/3] usb: dwc3: Don't reinitialize core during host bus-suspend/resume

[Felipe Balbi]

Hi,

Roger Quadros  writes:
> Hi Manu,
>
> On 27/09/17 14:19, Manu Gautam wrote:
>> Driver powers-off PHYs and reinitializes DWC3 core and gadget on
>> resume. While this works fine for gadget mode but in host
>> mode there is not re-initialization of host stack. Also, resetting
>> bus as part of bus_suspend/resume is not correct which could affect
>> (or disconnect) connected devices.
>> Fix this by not reinitializing core on suspend/resume in host mode
>> for HOST only and OTG/drd configurations.
>> 
>
> All this seems correct but we (TI) were relying on dwc3_core_exit() to be 
> called
> during dwc3_suspend() to have the lowest power state for our platforms.
>
> After this patch, DWC3 controller and PHYs won't be turned off thus
> preventing our platform from reaching low power levels.
>
> So this is a regression for us (TI) in v4.15-rc.
>
> Felipe, do you agree?
>
> If yes I can send a patch which fixes the regression
> and also makes USB host work after suspend/resume.

A power consumption regression is still a regression. But it's super
late in the -rc cycle, I think we need to get this merged after 4.16-rc1
and make sure to Cc stable.

Should be more than enough time to review and test patches.

-- 
balbi


signature.asc
Description: PGP signature

Re: [usb_add_gadget_udc_release] BUG: KASAN: double-free or invalid-free in (null)

[Felipe Balbi]

Hi,

Alan Stern <st...@rowland.harvard.edu> writes:
> On Sun, 17 Dec 2017, Fengguang Wu wrote:
>
>> Hello,
>> 
>> FYI this happens in mainline kernel 4.15.0-rc3.
>> It looks like a new regression.
>> 
>> It occurs in 23 out of 36 boots.
>> 
>> [   38.592360] LUN: removable file: (no medium)
>> [   38.593442] no file given for LUN0
>> [   38.594589] g_mass_storage usbip-vudc.0: failed to start g_mass_storage: 
>> -22
>> [   38.600881] udc usbip-vudc.0: releasing 'usbip-vudc.0'
>> [   38.604397] 
>> ==
>> [   38.605034] BUG: KASAN: double-free or invalid-free in   (null)
>> [   38.605034]
>> [   38.605034] CPU: 0 PID: 1 Comm: swapper Not tainted 4.15.0-rc3 #468
>> [   38.605034] Call Trace:
>> [   38.605034]  dump_stack+0x2f/0x3e:
>>  __dump_stack at 
>> lib/dump_stack.c:17
>>   (inlined by) dump_stack at 
>> lib/dump_stack.c:63
>> [   38.605034]  print_address_description+0xc2/0x3b7:
>>  print_address_description at 
>> mm/kasan/report.c:253
>> [   38.605034]  kasan_report_double_free+0x50/0x8c:
>>  kasan_report_double_free at 
>> mm/kasan/report.c:334
>> [   38.605034]  kasan_slab_free+0x60/0x1ef:
>>  kasan_slab_free at 
>> mm/kasan/kasan.c:514
>> [   38.605034]  ? ftrace_likely_update+0x5c/0xc4:
>>  ftrace_likely_update at 
>> kernel/trace/trace_branch.c:223
>> [   38.605034]  ? kobj_kset_leave+0x193/0x1dc:
>>  kobj_kset_leave at 
>> lib/kobject.c:184
>> [   38.605034]  ? lock_acquired+0x8d2/0x8d2:
>>  lock_release at 
>> kernel/locking/lockdep.c:4013
>> [   38.605034]  ? ftrace_likely_update+0x5c/0xc4:
>>  ftrace_likely_update at 
>> kernel/trace/trace_branch.c:223
>> [   38.605034]  ? trace_preempt_on+0x489/0x4d7:
>>  trace_preempt_enable_rcuidle at 
>> include/trace/events/preemptirq.h:50
>>   (inlined by) trace_preempt_on 
>> at kernel/trace/trace_irqsoff.c:855
>> [   38.605034]  ? static_obj+0x40/0x40:
>>  match_held_lock at 
>> kernel/locking/lockdep.c:3567
>> [   38.605034]  ? kobject_put+0xf5/0x642:
>>  refcount_dec_and_test at 
>> arch/x86/include/asm/refcount.h:75
>>   (inlined by) kref_put at 
>> include/linux/kref.h:69
>>   (inlined by) kobject_put at 
>> lib/kobject.c:694
>> [   38.605034]  ? trace_hardirqs_off+0x17/0x1f:
>>  trace_hardirqs_off at 
>> kernel/locking/lockdep.c:2984
>> [   38.605034]  ? kfree+0x419/0x5e7:
>>  slab_free_hook at mm/slub.c:1380
>>   (inlined by) 
>> slab_free_freelist_hook at mm/slub.c:1412
>>   (inlined by) slab_free at 
>> mm/slub.c:2968
>>   (inlined by) kfree at 
>> mm/slub.c:3899
>> [   38.605034]  kfree+0x43c/0x5e7:
>>      slab_free at mm/slub.c:2973
>>   (inlined by) kfree at 
>> mm/slub.c:3899
>> [   38.605034]  usb_add_gadget_udc_release+0x693/0x6ca:
>>  usb_add_gadget_udc_release at 
>> drivers/usb/gadget/udc/core.c:1199
>
> Boy, the error handling in that routine is a mess.  The patch below 
> should straighten it out.

looks good:

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>


> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/udc/core.c
> ===
> --- usb-4.x.orig/drivers/usb/gadget/udc/core.c
> +++ usb-4.x/drivers/usb/gadget/udc/core.c
> @@ -1147,11 +1147,7 @@ int usb_add_gadget_udc_release(struct de
>  
>   udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>   if (!udc)
> - goto err1;
> -
> - ret = device_add(>dev);
> - if (ret)
> - 

Re: [usb_add_gadget_udc_release] BUG: KASAN: double-free or invalid-free in (null)

[Felipe Balbi]

Hi,

Alan Stern  writes:
> On Sun, 17 Dec 2017, Fengguang Wu wrote:
>
>> Hello,
>> 
>> FYI this happens in mainline kernel 4.15.0-rc3.
>> It looks like a new regression.
>> 
>> It occurs in 23 out of 36 boots.
>> 
>> [   38.592360] LUN: removable file: (no medium)
>> [   38.593442] no file given for LUN0
>> [   38.594589] g_mass_storage usbip-vudc.0: failed to start g_mass_storage: 
>> -22
>> [   38.600881] udc usbip-vudc.0: releasing 'usbip-vudc.0'
>> [   38.604397] 
>> ==
>> [   38.605034] BUG: KASAN: double-free or invalid-free in   (null)
>> [   38.605034]
>> [   38.605034] CPU: 0 PID: 1 Comm: swapper Not tainted 4.15.0-rc3 #468
>> [   38.605034] Call Trace:
>> [   38.605034]  dump_stack+0x2f/0x3e:
>>  __dump_stack at 
>> lib/dump_stack.c:17
>>   (inlined by) dump_stack at 
>> lib/dump_stack.c:63
>> [   38.605034]  print_address_description+0xc2/0x3b7:
>>  print_address_description at 
>> mm/kasan/report.c:253
>> [   38.605034]  kasan_report_double_free+0x50/0x8c:
>>  kasan_report_double_free at 
>> mm/kasan/report.c:334
>> [   38.605034]  kasan_slab_free+0x60/0x1ef:
>>  kasan_slab_free at 
>> mm/kasan/kasan.c:514
>> [   38.605034]  ? ftrace_likely_update+0x5c/0xc4:
>>  ftrace_likely_update at 
>> kernel/trace/trace_branch.c:223
>> [   38.605034]  ? kobj_kset_leave+0x193/0x1dc:
>>  kobj_kset_leave at 
>> lib/kobject.c:184
>> [   38.605034]  ? lock_acquired+0x8d2/0x8d2:
>>  lock_release at 
>> kernel/locking/lockdep.c:4013
>> [   38.605034]  ? ftrace_likely_update+0x5c/0xc4:
>>  ftrace_likely_update at 
>> kernel/trace/trace_branch.c:223
>> [   38.605034]  ? trace_preempt_on+0x489/0x4d7:
>>  trace_preempt_enable_rcuidle at 
>> include/trace/events/preemptirq.h:50
>>   (inlined by) trace_preempt_on 
>> at kernel/trace/trace_irqsoff.c:855
>> [   38.605034]  ? static_obj+0x40/0x40:
>>  match_held_lock at 
>> kernel/locking/lockdep.c:3567
>> [   38.605034]  ? kobject_put+0xf5/0x642:
>>  refcount_dec_and_test at 
>> arch/x86/include/asm/refcount.h:75
>>   (inlined by) kref_put at 
>> include/linux/kref.h:69
>>   (inlined by) kobject_put at 
>> lib/kobject.c:694
>> [   38.605034]  ? trace_hardirqs_off+0x17/0x1f:
>>  trace_hardirqs_off at 
>> kernel/locking/lockdep.c:2984
>> [   38.605034]  ? kfree+0x419/0x5e7:
>>  slab_free_hook at mm/slub.c:1380
>>   (inlined by) 
>> slab_free_freelist_hook at mm/slub.c:1412
>>   (inlined by) slab_free at 
>> mm/slub.c:2968
>>   (inlined by) kfree at 
>> mm/slub.c:3899
>> [   38.605034]  kfree+0x43c/0x5e7:
>>          slab_free at mm/slub.c:2973
>>   (inlined by) kfree at 
>> mm/slub.c:3899
>> [   38.605034]  usb_add_gadget_udc_release+0x693/0x6ca:
>>  usb_add_gadget_udc_release at 
>> drivers/usb/gadget/udc/core.c:1199
>
> Boy, the error handling in that routine is a mess.  The patch below 
> should straighten it out.

looks good:

Acked-by: Felipe Balbi 


> Alan Stern
>
>
>
> Index: usb-4.x/drivers/usb/gadget/udc/core.c
> ===
> --- usb-4.x.orig/drivers/usb/gadget/udc/core.c
> +++ usb-4.x/drivers/usb/gadget/udc/core.c
> @@ -1147,11 +1147,7 @@ int usb_add_gadget_udc_release(struct de
>  
>   udc = kzalloc(sizeof(*udc), GFP_KERNEL);
>   if (!udc)
> - goto err1;
> -
> - ret = device_add(>dev);
> - if (ret)
> - goto err2;
> + goto err_put_gadget;
>  
>   device_initial

Re: [PATCH] usb: gadget: uvc:change the UVC_NUM_REQUESTS value

[Felipe Balbi]

Hi,

Lipengcheng <lpc...@hisilicon.com> writes:
> The value is 4, it can cache four descriptors. When streaming_interval = 1,
> it can tolerate 500us. Some busy scenes, it may be more than 500us because
> cpu scheduling is not timely. There will have some problems. It is better
> set to eight.
>
> Signed-off-by: Pengcheng Li <lpc...@hisilicon.com>
> ---
>  drivers/usb/gadget/function/uvc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc.h 
> b/drivers/usb/gadget/function/uvc.h
> index a64e07e..901487e 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -90,7 +90,7 @@ extern unsigned int uvc_gadget_trace_param;
>   * Driver specific constants
>   */
>
> -#define UVC_NUM_REQUESTS   4
> +#define UVC_NUM_REQUESTS   8

if you want to be taken seriously, the bare minimum you can do is to use
scripts/get_maintainer.pl to help with a proper Cc list:

$ scripts/get_maintainer.pl -f drivers/usb/gadget/function/uvc.h
Laurent Pinchart <laurent.pinch...@ideasonboard.com> (maintainer:USB WEBCAM 
GADGET)
Felipe Balbi <ba...@kernel.org> (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)
Greg Kroah-Hartman <gre...@linuxfoundation.org> (supporter:USB SUBSYSTEM)
linux-...@vger.kernel.org (open list:USB WEBCAM GADGET)
linux-kernel@vger.kernel.org (open list)

Laurent, what do you think about this?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: uvc:change the UVC_NUM_REQUESTS value

[Felipe Balbi]

Hi,

Lipengcheng  writes:
> The value is 4, it can cache four descriptors. When streaming_interval = 1,
> it can tolerate 500us. Some busy scenes, it may be more than 500us because
> cpu scheduling is not timely. There will have some problems. It is better
> set to eight.
>
> Signed-off-by: Pengcheng Li 
> ---
>  drivers/usb/gadget/function/uvc.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/function/uvc.h 
> b/drivers/usb/gadget/function/uvc.h
> index a64e07e..901487e 100644
> --- a/drivers/usb/gadget/function/uvc.h
> +++ b/drivers/usb/gadget/function/uvc.h
> @@ -90,7 +90,7 @@ extern unsigned int uvc_gadget_trace_param;
>   * Driver specific constants
>   */
>
> -#define UVC_NUM_REQUESTS   4
> +#define UVC_NUM_REQUESTS   8

if you want to be taken seriously, the bare minimum you can do is to use
scripts/get_maintainer.pl to help with a proper Cc list:

$ scripts/get_maintainer.pl -f drivers/usb/gadget/function/uvc.h
Laurent Pinchart  (maintainer:USB WEBCAM 
GADGET)
Felipe Balbi  (maintainer:USB GADGET/PERIPHERAL SUBSYSTEM)
Greg Kroah-Hartman  (supporter:USB SUBSYSTEM)
linux-...@vger.kernel.org (open list:USB WEBCAM GADGET)
linux-kernel@vger.kernel.org (open list)

Laurent, what do you think about this?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: gadget: decrease the queued_requests in removal

[Felipe Balbi]

Hi,

Lipengcheng  writes:
> In removal requests, it is necessary to make the corresponding trb
> disable state (HWO = 1) and dep->queued_requests a corresponding reduction.
> It is better to use a alone funtion to disable trb (HWO = 0).

this shouldn't be necessary. What problem are you facing? what happens?
Where are the tracepoint logs? Which kernel are you using? did you try
mainline?

> Signed-off-by: Pengcheng Li 
> ---
> drivers/usb/dwc3/gadget.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1e6c42e..273b51d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -707,6 +707,36 @@ static void dwc3_remove_requests(struct dwc3 *dwc, 
> struct dwc3_ep *dep)
>     while (!list_empty(>started_list)) {

why all these underscore characters? Are you sending html emails?

>     req = next_request(>started_list);
>
> +   if (req->trb) {
> +   if (req->num_pending_sgs) {
> +   struct dwc3_trb *trb;
> +   int i = 0;
> +
> +   for (i = 0; i < req->num_pending_sgs; i++) {
> +   trb = req->trb + i;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +
> +   if (req->unaligned || req->zero) {
> +   trb = req->trb + req->num_pending_sgs + 1;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +   } else {
> +   struct dwc3_trb *trb = req->trb;
> +
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +
> +   if (req->unaligned || req->zero) {
> +   trb = req->trb + 1;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +   }
> +   }
> +   dep->queued_requests--;
>     dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
>     }

you have a lot to fix here.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: gadget: decrease the queued_requests in removal

[Felipe Balbi]

Hi,

Lipengcheng  writes:
> In removal requests, it is necessary to make the corresponding trb
> disable state (HWO = 1) and dep->queued_requests a corresponding reduction.
> It is better to use a alone funtion to disable trb (HWO = 0).

this shouldn't be necessary. What problem are you facing? what happens?
Where are the tracepoint logs? Which kernel are you using? did you try
mainline?

> Signed-off-by: Pengcheng Li 
> ---
> drivers/usb/dwc3/gadget.c | 30 ++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 1e6c42e..273b51d 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -707,6 +707,36 @@ static void dwc3_remove_requests(struct dwc3 *dwc, 
> struct dwc3_ep *dep)
>     while (!list_empty(>started_list)) {

why all these underscore characters? Are you sending html emails?

>     req = next_request(>started_list);
>
> +   if (req->trb) {
> +   if (req->num_pending_sgs) {
> +   struct dwc3_trb *trb;
> +   int i = 0;
> +
> +   for (i = 0; i < req->num_pending_sgs; i++) {
> +   trb = req->trb + i;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +
> +   if (req->unaligned || req->zero) {
> +   trb = req->trb + req->num_pending_sgs + 1;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +   } else {
> +   struct dwc3_trb *trb = req->trb;
> +
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +
> +   if (req->unaligned || req->zero) {
> +   trb = req->trb + 1;
> +   trb->ctrl &= ~DWC3_TRB_CTRL_HWO;
> +   dwc3_ep_inc_deq(dep);
> +   }
> +   }
> +   }
> +   dep->queued_requests--;
>     dwc3_gadget_giveback(dep, req, -ESHUTDOWN);
>     }

you have a lot to fix here.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: dwc3: gadget:Core consumes a trb software to fill a trb, in ISO

[Felipe Balbi]

Hi,

Lipengcheng  writes:
>> Lipengcheng  writes:
>> 
>> > Iso transmission, the current process is that all trb(HWO=1) is handled.
>> > Then core generate DWC3_DEPEVT_XFERNOTREADY event, Software begin
>> > refill trb, this will produce 0 length package, the patch is to
>> > achieve the core consumes a trb, and then the software fill a trb.
>> > Normally, there will never be DWC3_DEPEVT_XFERNOTREADY event and 0-length 
>> > packet.
>> >
>> > Signed-off-by: l00229106 
>> 
>> who is 100229106??
> Sorry. It is my job number. I will use Pengcheng li to replace it.

thanks

>> > ---
>> >  drivers/usb/dwc3/gadget.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 981fd98..1e6c42e 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -2420,7 +2420,7 @@ static void dwc3_endpoint_transfer_complete(struct 
>> > dwc3 *dwc,
>> > if (!dep->endpoint.desc)
>> > return;
>> >
>> > -   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
>> > +   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) || (dep->flags &
>> > + DWC3_EP_TRANSFER_STARTED))
>> 
>> this is wrong. isoc endpoints should NEVER be prestarted.
> The main purpose is to core handle a trb and sofware re-fill the next trb in 
> the DWC3_DEPEVT_XFERINPROGRESS interrupt. Mayebe it can be modified:
> if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
> __dwc3_gadget_kick_transfer(dep);
> +   else
> +   dwc3_prepare_trbs(dep);
> +

no, this would be wrong too. Care to show me tracepoint data of what you
mean? I really can't understand what problem you're facing here.

Also, try a more recent kernel. I can't accept patches against a v4.4
kernel.

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: dwc3: gadget:Core consumes a trb software to fill a trb, in ISO

[Felipe Balbi]

Hi,

Lipengcheng  writes:
>> Lipengcheng  writes:
>> 
>> > Iso transmission, the current process is that all trb(HWO=1) is handled.
>> > Then core generate DWC3_DEPEVT_XFERNOTREADY event, Software begin
>> > refill trb, this will produce 0 length package, the patch is to
>> > achieve the core consumes a trb, and then the software fill a trb.
>> > Normally, there will never be DWC3_DEPEVT_XFERNOTREADY event and 0-length 
>> > packet.
>> >
>> > Signed-off-by: l00229106 
>> 
>> who is 100229106??
> Sorry. It is my job number. I will use Pengcheng li to replace it.

thanks

>> > ---
>> >  drivers/usb/dwc3/gadget.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> > index 981fd98..1e6c42e 100644
>> > --- a/drivers/usb/dwc3/gadget.c
>> > +++ b/drivers/usb/dwc3/gadget.c
>> > @@ -2420,7 +2420,7 @@ static void dwc3_endpoint_transfer_complete(struct 
>> > dwc3 *dwc,
>> > if (!dep->endpoint.desc)
>> > return;
>> >
>> > -   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
>> > +   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) || (dep->flags &
>> > + DWC3_EP_TRANSFER_STARTED))
>> 
>> this is wrong. isoc endpoints should NEVER be prestarted.
> The main purpose is to core handle a trb and sofware re-fill the next trb in 
> the DWC3_DEPEVT_XFERINPROGRESS interrupt. Mayebe it can be modified:
> if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
> __dwc3_gadget_kick_transfer(dep);
> +   else
> +   dwc3_prepare_trbs(dep);
> +

no, this would be wrong too. Care to show me tracepoint data of what you
mean? I really can't understand what problem you're facing here.

Also, try a more recent kernel. I can't accept patches against a v4.4
kernel.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb:dwc3:fix access poisoned list_head in dwc3_gadget_giveback

[Felipe Balbi]

Hi,

Yu Chen  writes:
> From: Yu Chen 
>
> Unable to handle kernel paging request at virtual address dead0108
> pgd = fff7a3179000
> [dead0108] *pgd=230e0003, *pud=230e0003,
> *pmd=
> Internal error: Oops: 9644 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 2 PID: 1 Comm: init Tainted: GW   4.4.23+ #1

try mainline

> TGID: 1 Comm: init
> Hardware name: kirin970 (DT)
> task: fff99f19 ti: fff99f1740e0 task.ti: fff99f1740e0
> PC is at dwc3_gadget_giveback+0xa8/0x228
> LR is at dwc3_remove_requests+0x44/0x88
>
> The crash occurred when usb work as rndis device and
> __dwc3_gadget_kick_transfer return error in __dwc3_gadget_ep_queue.
> The request submited in __dwc3_gadget_ep_queue is moved to started_list
> but not kicked. It is stil on started_list although
> __dwc3_gadget_kick_transfer failed. When dwc3_gadget_ep_queue return

why did kick_transfer fail? Where are the tracepoints showing the
failure?

> error to u_ether driver, the request will be resubmit to dwc3 driver.
> At last, the same request is both on started_list and pending_list,
> it will be list_del twice in dwc3_remove_requests and cause crash.
>
> Signed-off-by: Yu Chen 
> ---
>  drivers/usb/dwc3/gadget.c | 28 +++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 639dd1b163a0..a913e64ca4e0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1278,9 +1278,28 @@ static void dwc3_gadget_start_isoc(struct dwc3 *dwc,
>   __dwc3_gadget_start_isoc(dwc, dep, cur_uf);
>  }
>  
> +static int dwc3_gadget_is_req_pengding_or_started(struct dwc3_ep *dep,
> + struct dwc3_request *req)
> +{
> + struct dwc3_request *iterate_req;
> +
> + list_for_each_entry(iterate_req, >pending_list, list) {
> + if (iterate_req == req)
> + return 1;
> + }
> +
> + list_for_each_entry(iterate_req, >started_list, list) {
> + if (iterate_req == req)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request 
> *req)
>  {
>   struct dwc3 *dwc = dep->dwc;
> + int ret;
>  
>   if (!dep->endpoint.desc) {
>   dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> @@ -1334,7 +1353,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
> struct dwc3_request *req)
>   }
>  
>  out:
> - return __dwc3_gadget_kick_transfer(dep);
> + ret = __dwc3_gadget_kick_transfer(dep);
> + if (ret && dwc3_gadget_is_req_pengding_or_started(dep, req)) {

first we need to figure out why kick_transfer failed. It shouldn't fail,
so why did it? Then I need you to try with a more recent kernel v4.4 is
rather old and a lot has changed WRT transfer handling:

$ git rev-list --count --no-merges v4.4..linus/master -- 
drivers/usb/dwc3/gadget.c
184

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb:dwc3:fix access poisoned list_head in dwc3_gadget_giveback

[Felipe Balbi]

Hi,

Yu Chen  writes:
> From: Yu Chen 
>
> Unable to handle kernel paging request at virtual address dead0108
> pgd = fff7a3179000
> [dead0108] *pgd=230e0003, *pud=230e0003,
> *pmd=
> Internal error: Oops: 9644 [#1] PREEMPT SMP
> Modules linked in:
> CPU: 2 PID: 1 Comm: init Tainted: GW   4.4.23+ #1

try mainline

> TGID: 1 Comm: init
> Hardware name: kirin970 (DT)
> task: fff99f19 ti: fff99f1740e0 task.ti: fff99f1740e0
> PC is at dwc3_gadget_giveback+0xa8/0x228
> LR is at dwc3_remove_requests+0x44/0x88
>
> The crash occurred when usb work as rndis device and
> __dwc3_gadget_kick_transfer return error in __dwc3_gadget_ep_queue.
> The request submited in __dwc3_gadget_ep_queue is moved to started_list
> but not kicked. It is stil on started_list although
> __dwc3_gadget_kick_transfer failed. When dwc3_gadget_ep_queue return

why did kick_transfer fail? Where are the tracepoints showing the
failure?

> error to u_ether driver, the request will be resubmit to dwc3 driver.
> At last, the same request is both on started_list and pending_list,
> it will be list_del twice in dwc3_remove_requests and cause crash.
>
> Signed-off-by: Yu Chen 
> ---
>  drivers/usb/dwc3/gadget.c | 28 +++-
>  1 file changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 639dd1b163a0..a913e64ca4e0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1278,9 +1278,28 @@ static void dwc3_gadget_start_isoc(struct dwc3 *dwc,
>   __dwc3_gadget_start_isoc(dwc, dep, cur_uf);
>  }
>  
> +static int dwc3_gadget_is_req_pengding_or_started(struct dwc3_ep *dep,
> + struct dwc3_request *req)
> +{
> + struct dwc3_request *iterate_req;
> +
> + list_for_each_entry(iterate_req, >pending_list, list) {
> + if (iterate_req == req)
> + return 1;
> + }
> +
> + list_for_each_entry(iterate_req, >started_list, list) {
> + if (iterate_req == req)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
>  static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, struct dwc3_request 
> *req)
>  {
>   struct dwc3 *dwc = dep->dwc;
> + int ret;
>  
>   if (!dep->endpoint.desc) {
>   dev_err(dwc->dev, "%s: can't queue to disabled endpoint\n",
> @@ -1334,7 +1353,14 @@ static int __dwc3_gadget_ep_queue(struct dwc3_ep *dep, 
> struct dwc3_request *req)
>   }
>  
>  out:
> - return __dwc3_gadget_kick_transfer(dep);
> + ret = __dwc3_gadget_kick_transfer(dep);
> + if (ret && dwc3_gadget_is_req_pengding_or_started(dep, req)) {

first we need to figure out why kick_transfer failed. It shouldn't fail,
so why did it? Then I need you to try with a more recent kernel v4.4 is
rather old and a lot has changed WRT transfer handling:

$ git rev-list --count --no-merges v4.4..linus/master -- 
drivers/usb/dwc3/gadget.c
184

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: gadget:Core consumes a trb software to fill a trb, in ISO

[Felipe Balbi]

Hi,

Lipengcheng  writes:

> Iso transmission, the current process is that all trb(HWO=1) is handled.
> Then core generate DWC3_DEPEVT_XFERNOTREADY event, Software begin  refill
> trb, this will produce 0 length package, the patch is to achieve the core
> consumes a trb, and then the software fill a trb. Normally, there will never
> be DWC3_DEPEVT_XFERNOTREADY event and 0-length packet.
>
> Signed-off-by: l00229106 

who is 100229106??

> ---
>  drivers/usb/dwc3/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 981fd98..1e6c42e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2420,7 +2420,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 
> *dwc,
> if (!dep->endpoint.desc)
> return;
>
> -   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
> +   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) || (dep->flags & 
> DWC3_EP_TRANSFER_STARTED))

this is wrong. isoc endpoints should NEVER be prestarted.

-- 
balbi

Re: [PATCH] usb: dwc3: gadget:Core consumes a trb software to fill a trb, in ISO

[Felipe Balbi]

Hi,

Lipengcheng  writes:

> Iso transmission, the current process is that all trb(HWO=1) is handled.
> Then core generate DWC3_DEPEVT_XFERNOTREADY event, Software begin  refill
> trb, this will produce 0 length package, the patch is to achieve the core
> consumes a trb, and then the software fill a trb. Normally, there will never
> be DWC3_DEPEVT_XFERNOTREADY event and 0-length packet.
>
> Signed-off-by: l00229106 

who is 100229106??

> ---
>  drivers/usb/dwc3/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 981fd98..1e6c42e 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2420,7 +2420,7 @@ static void dwc3_endpoint_transfer_complete(struct dwc3 
> *dwc,
> if (!dep->endpoint.desc)
> return;
>
> -   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc))
> +   if (!usb_endpoint_xfer_isoc(dep->endpoint.desc) || (dep->flags & 
> DWC3_EP_TRANSFER_STARTED))

this is wrong. isoc endpoints should NEVER be prestarted.

-- 
balbi

Re: [-next PATCH 0/4] sysfs and DEVICE_ATTR_

[Felipe Balbi]

Hi,

Joe Perches <j...@perches.com> writes:
>  drivers/usb/phy/phy-tahvo.c|  2 +-

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi

Re: [-next PATCH 0/4] sysfs and DEVICE_ATTR_

[Felipe Balbi]

Hi,

Joe Perches  writes:
>  drivers/usb/phy/phy-tahvo.c|  2 +-

Acked-by: Felipe Balbi 

-- 
balbi

Re: linux-next: Tree for Dec 14 (usb/dwc3)

[Felipe Balbi]

Hi,

Randy Dunlap  writes:
> On 12/13/2017 11:06 PM, Stephen Rothwell wrote:
>> Hi all,
>> 
>> Changes since 20171213:
>> 
>
> on i386:
>
> ERROR: "__tracepoint_dwc3_writel" [drivers/usb/dwc3/dwc3.ko] undefined!
> ERROR: "__tracepoint_dwc3_readl" [drivers/usb/dwc3/dwc3.ko] undefined!
>
>
>
> I can send the randconfig file is you need it.

please do. write/readl tracepoints haven't changed in ages :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: linux-next: Tree for Dec 14 (usb/dwc3)

[Felipe Balbi]

Hi,

Randy Dunlap  writes:
> On 12/13/2017 11:06 PM, Stephen Rothwell wrote:
>> Hi all,
>> 
>> Changes since 20171213:
>> 
>
> on i386:
>
> ERROR: "__tracepoint_dwc3_writel" [drivers/usb/dwc3/dwc3.ko] undefined!
> ERROR: "__tracepoint_dwc3_readl" [drivers/usb/dwc3/dwc3.ko] undefined!
>
>
>
> I can send the randconfig file is you need it.

please do. write/readl tracepoints haven't changed in ages :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Hi,

Douglas Anderson  writes:
> On rk3288-veyron devices on Chrome OS it was found that plugging in an
> Arduino-based USB device could cause the system to lockup, especially
> if the CPU Frequency was at one of the slower operating points (like
> 100 MHz / 200 MHz).
>
> Upon tracing, I found that the following was happening:
> * The USB device (full speed) was connected to a high speed hub and
>   then to the rk3288.  Thus, we were dealing with split transactions,
>   which is all handled in software on dwc2.
> * Userspace was initiating a BULK IN transfer
> * When we sent the SSPLIT (to start the split transaction), we got an
>   ACK.  Good.  Then we issued the CSPLIT.
> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>   the interrupt handler) started to retry and sent another SSPLIT.
> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>   handler.
> * The handling of the interrupts was (because of the low CPU speed and
>   the inefficiency of the dwc2 interrupt handler) was actually taking
>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>   were _always_ in the USB interrupt routine.
> * The fact that USB interrupts were always going off was preventing
>   other things from happening in the system.  This included preventing
>   the system from being able to transition to a higher CPU frequency.
>
> As I understand it, there is no requirement to retry super quickly
> after a NAK, we just have to retry sometime in the future.  Thus one
> solution to the above is to just add a delay between getting a NAK and
> retrying the transmission.  If this delay is sufficiently long to get
> out of the interrupt routine then the rest of the system will be able
> to make forward progress.  Even a 25 us delay would probably be
> enough, but we'll be extra conservative and try to delay 1 ms (the
> exact amount depends on HZ and the accuracy of the jiffy and how close
> the current jiffy is to ticking, but could be as much as 20 ms or as
> little as 1 ms).
>
> Presumably adding a delay like this could impact the USB throughput,
> so we only add the delay with repeated NAKs.
>
> NOTE: Upon further testing of a pl2303 serial adapter, I found that
> this fix may help with problems there.  Specifically I found that the
> pl2303 serial adapters tend to respond with a NAK when they have
> nothing to say and thus we end with this same sequence.
>
> Signed-off-by: Douglas Anderson 
> Cc: sta...@vger.kernel.org
> Reviewed-by: Julius Werner 
> Tested-by: Stefan Wahren 

This seems too big for -rc or -stable inclusion. In any case, this
doesn't apply to my testing/next branch. Care to rebase and collect acks
you received while doing that?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 1/2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Hi,

Douglas Anderson  writes:
> On rk3288-veyron devices on Chrome OS it was found that plugging in an
> Arduino-based USB device could cause the system to lockup, especially
> if the CPU Frequency was at one of the slower operating points (like
> 100 MHz / 200 MHz).
>
> Upon tracing, I found that the following was happening:
> * The USB device (full speed) was connected to a high speed hub and
>   then to the rk3288.  Thus, we were dealing with split transactions,
>   which is all handled in software on dwc2.
> * Userspace was initiating a BULK IN transfer
> * When we sent the SSPLIT (to start the split transaction), we got an
>   ACK.  Good.  Then we issued the CSPLIT.
> * When we sent the CSPLIT, we got back a NAK.  We immediately (from
>   the interrupt handler) started to retry and sent another SSPLIT.
> * The device kept NAKing our CSPLIT, so we kept ping-ponging between
>   sending a SSPLIT and a CSPLIT, each time sending from the interrupt
>   handler.
> * The handling of the interrupts was (because of the low CPU speed and
>   the inefficiency of the dwc2 interrupt handler) was actually taking
>   _longer_ than it took the other side to send the ACK/NAK.  Thus we
>   were _always_ in the USB interrupt routine.
> * The fact that USB interrupts were always going off was preventing
>   other things from happening in the system.  This included preventing
>   the system from being able to transition to a higher CPU frequency.
>
> As I understand it, there is no requirement to retry super quickly
> after a NAK, we just have to retry sometime in the future.  Thus one
> solution to the above is to just add a delay between getting a NAK and
> retrying the transmission.  If this delay is sufficiently long to get
> out of the interrupt routine then the rest of the system will be able
> to make forward progress.  Even a 25 us delay would probably be
> enough, but we'll be extra conservative and try to delay 1 ms (the
> exact amount depends on HZ and the accuracy of the jiffy and how close
> the current jiffy is to ticking, but could be as much as 20 ms or as
> little as 1 ms).
>
> Presumably adding a delay like this could impact the USB throughput,
> so we only add the delay with repeated NAKs.
>
> NOTE: Upon further testing of a pl2303 serial adapter, I found that
> this fix may help with problems there.  Specifically I found that the
> pl2303 serial adapters tend to respond with a NAK when they have
> nothing to say and thus we end with this same sequence.
>
> Signed-off-by: Douglas Anderson 
> Cc: sta...@vger.kernel.org
> Reviewed-by: Julius Werner 
> Tested-by: Stefan Wahren 

This seems too big for -rc or -stable inclusion. In any case, this
doesn't apply to my testing/next branch. Care to rebase and collect acks
you received while doing that?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

[Felipe Balbi]

Hi,

(please break your lines at 80-characters)

Yinbo Zhu  writes:
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 
>>> 5cb3f6795b0b..071e7cea8cbb 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1106,6 +1106,9 @@ static void dwc3_get_properties(struct dwc3 
>>> *dwc)
>>>  
>>> dwc->quirk_reverse_in_out = device_property_read_bool(dev,
>>> "snps,quirk_reverse_in_out");
>
>>This was generated on vendor tree. This quirk doesn't exist in dwc3. Also,
>  >update your tree and review MAINTAINERS file. It has been almost 2 years 
> since I left TI :-)
>
>>--
>>Balbi
>
> Hi Balbi,
>
> The quirk that I had add it in dwc3. Your meaning is that I can't use
> quirk to enable or disable the erratum, isn't it? The tree is
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git, I had
> updated it.

-*- mode: grep; default-directory: "~/workspace/linux/" -*-
Grep started at Mon Dec 11 10:50:47

git --no-pager grep --color -nH -e quirk_reverse_in_out

Grep finished with no matches found at Mon Dec 11 10:50:48

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

[Felipe Balbi]

Hi,

(please break your lines at 80-characters)

Yinbo Zhu  writes:
>>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c index 
>>> 5cb3f6795b0b..071e7cea8cbb 100644
>>> --- a/drivers/usb/dwc3/core.c
>>> +++ b/drivers/usb/dwc3/core.c
>>> @@ -1106,6 +1106,9 @@ static void dwc3_get_properties(struct dwc3 
>>> *dwc)
>>>  
>>> dwc->quirk_reverse_in_out = device_property_read_bool(dev,
>>> "snps,quirk_reverse_in_out");
>
>>This was generated on vendor tree. This quirk doesn't exist in dwc3. Also,
>  >update your tree and review MAINTAINERS file. It has been almost 2 years 
> since I left TI :-)
>
>>--
>>Balbi
>
> Hi Balbi,
>
> The quirk that I had add it in dwc3. Your meaning is that I can't use
> quirk to enable or disable the erratum, isn't it? The tree is
> git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git, I had
> updated it.

-*- mode: grep; default-directory: "~/workspace/linux/" -*-
Grep started at Mon Dec 11 10:50:47

git --no-pager grep --color -nH -e quirk_reverse_in_out

Grep finished with no matches found at Mon Dec 11 10:50:48

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
> On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo@nxp.com wrote:
>> From: "yinbo.zhu" 
>> 
>> Description: This is a occasional problem where the software
>
> No need for a "Description:" word.  That's just assumed here, right?
>
>> issues an End Transfer command while a USB transfer is in progress,
>> resulting in the TxFIFO  being flushed when the lower layer is waiting
>> for data,causing the super speed (SS) transmit to get blocked.
>> If the End Transfer command is issued on an IN endpoint to
>> flush out the pending transfers when the same IN endpoint
>> is doing transfers on the USB, then depending upon the timing
>> of the End Transfer (and the resulting internal FIFO flush),the
>> lower layer (U3PTL/U3MAC) could get stuck waiting for data
>> indefinitely. This blocks the transmission path on the SS, and no
>> DP/ACK/ERDY/DEVNOTIF packets can be sent from the device.
>> Impact: If this issue happens and the transmission gets blocked,
>> then the USB host aborts and resets/re-enumerates the device.
>> This unblocks the transmitt engine and the device functions normally.
>> 
>> Workaround: Software must wait for all existing TRBs to complete before
>> issuing End transfer command.
>> 
>> Configs Affected:
>> LS1088-48A-R1.0, LS2081A-R1.1, LS2088-48A-R1.0, LS2088-48A-R1.1,
>> LX2160-2120-2080A-R1.
>
> What are these Configs?  That doesn't seem to match up with anything
> that is in the kernel tree that I can see.
>
>> 
>> Signed-off-by: yinbo.zhu 
>> ---
>>  drivers/usb/dwc3/core.c  |  3 +++
>>  drivers/usb/dwc3/core.h  |  3 +++
>>  drivers/usb/dwc3/host.c  |  3 +++
>>  drivers/usb/host/xhci-plat.c |  4 
>>  drivers/usb/host/xhci.c  | 24 ++--
>>  drivers/usb/host/xhci.h  |  1 +
>>  6 files changed, 32 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 5cb3f6795b0b..071e7cea8cbb 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1106,6 +1106,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  
>>  dwc->quirk_reverse_in_out = device_property_read_bool(dev,
>>  "snps,quirk_reverse_in_out");

This was generated on vendor tree. This quirk doesn't exist in
dwc3. Also, update your tree and review MAINTAINERS file. It has been
almost 2 years since I left TI :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: host: Implement workaround for Erratum A-009611

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
> On Fri, Dec 08, 2017 at 05:49:41PM +0800, yinbo@nxp.com wrote:
>> From: "yinbo.zhu" 
>> 
>> Description: This is a occasional problem where the software
>
> No need for a "Description:" word.  That's just assumed here, right?
>
>> issues an End Transfer command while a USB transfer is in progress,
>> resulting in the TxFIFO  being flushed when the lower layer is waiting
>> for data,causing the super speed (SS) transmit to get blocked.
>> If the End Transfer command is issued on an IN endpoint to
>> flush out the pending transfers when the same IN endpoint
>> is doing transfers on the USB, then depending upon the timing
>> of the End Transfer (and the resulting internal FIFO flush),the
>> lower layer (U3PTL/U3MAC) could get stuck waiting for data
>> indefinitely. This blocks the transmission path on the SS, and no
>> DP/ACK/ERDY/DEVNOTIF packets can be sent from the device.
>> Impact: If this issue happens and the transmission gets blocked,
>> then the USB host aborts and resets/re-enumerates the device.
>> This unblocks the transmitt engine and the device functions normally.
>> 
>> Workaround: Software must wait for all existing TRBs to complete before
>> issuing End transfer command.
>> 
>> Configs Affected:
>> LS1088-48A-R1.0, LS2081A-R1.1, LS2088-48A-R1.0, LS2088-48A-R1.1,
>> LX2160-2120-2080A-R1.
>
> What are these Configs?  That doesn't seem to match up with anything
> that is in the kernel tree that I can see.
>
>> 
>> Signed-off-by: yinbo.zhu 
>> ---
>>  drivers/usb/dwc3/core.c  |  3 +++
>>  drivers/usb/dwc3/core.h  |  3 +++
>>  drivers/usb/dwc3/host.c  |  3 +++
>>  drivers/usb/host/xhci-plat.c |  4 
>>  drivers/usb/host/xhci.c  | 24 ++--
>>  drivers/usb/host/xhci.h  |  1 +
>>  6 files changed, 32 insertions(+), 6 deletions(-)
>> 
>> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
>> index 5cb3f6795b0b..071e7cea8cbb 100644
>> --- a/drivers/usb/dwc3/core.c
>> +++ b/drivers/usb/dwc3/core.c
>> @@ -1106,6 +1106,9 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>>  
>>  dwc->quirk_reverse_in_out = device_property_read_bool(dev,
>>  "snps,quirk_reverse_in_out");

This was generated on vendor tree. This quirk doesn't exist in
dwc3. Also, update your tree and review MAINTAINERS file. It has been
almost 2 years since I left TI :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] kernel: trace: pass export pointer as argument to ->write()

[Felipe Balbi]

Hi,

Alexander Shishkin <alexander.shish...@linux.intel.com> writes:
> On Mon, Dec 04, 2017 at 07:09:38AM -0500, Steven Rostedt wrote:
>> On Wed, 17 May 2017 22:52:28 -0400
>> Steven Rostedt <rost...@goodmis.org> wrote:
>> 
>> > On Thu, 18 May 2017 10:26:59 +0800
>> > Chunyan Zhang <zhang.chun...@linaro.org> wrote:
>> > 
>> > > On 17 May 2017 at 16:05, Felipe Balbi <felipe.ba...@linux.intel.com> 
>> > > wrote:  
>> > > > That way, users don't need to keep a global static pointer and can
>> > > > rely on container_of() to fetch their own structure.
>> > > >
>> > > > Signed-off-by: Felipe Balbi <felipe.ba...@linux.intel.com>
>> > > 
>> > > Reviewed-by: Chunyan Zhang <zhang.chun...@linaro.org>  
>> > 
>> > 
>> > If someone wants to pull this through their tree, feel free to do
>> > so. I've already acked it.
>> 
>> I guess nobody pulled this in. Does it still need to go? I'll add it to
>> my tree and start testing it, but I'll drop it if it's not needed (or I
>> don't get a response to this email).
>
> Ouch, it somehow got lost in my inbox. I don't mind taking it through my
> tree if there are no objections.

let me know if you guys need a rebase.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] kernel: trace: pass export pointer as argument to ->write()

[Felipe Balbi]

Hi,

Alexander Shishkin  writes:
> On Mon, Dec 04, 2017 at 07:09:38AM -0500, Steven Rostedt wrote:
>> On Wed, 17 May 2017 22:52:28 -0400
>> Steven Rostedt  wrote:
>> 
>> > On Thu, 18 May 2017 10:26:59 +0800
>> > Chunyan Zhang  wrote:
>> > 
>> > > On 17 May 2017 at 16:05, Felipe Balbi  
>> > > wrote:  
>> > > > That way, users don't need to keep a global static pointer and can
>> > > > rely on container_of() to fetch their own structure.
>> > > >
>> > > > Signed-off-by: Felipe Balbi 
>> > > 
>> > > Reviewed-by: Chunyan Zhang   
>> > 
>> > 
>> > If someone wants to pull this through their tree, feel free to do
>> > so. I've already acked it.
>> 
>> I guess nobody pulled this in. Does it still need to go? I'll add it to
>> my tree and start testing it, but I'll drop it if it's not needed (or I
>> don't get a response to this email).
>
> Ouch, it somehow got lost in my inbox. I don't mind taking it through my
> tree if there are no objections.

let me know if you guys need a rebase.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 35/36] usb/gadget/NCM: Replace tasklet with softirq hrtimer

[Felipe Balbi]

Anna-Maria Gleixner <anna-ma...@linutronix.de> writes:

> From: Thomas Gleixner <t...@linutronix.de>
>
> The tx_tasklet tasklet is used in invoke the hrtimer (task_timer) in
> softirq context. This can be also achieved without the tasklet but
> with HRTIMER_MODE_SOFT as hrtimer mode.
>
> Signed-off-by: Thomas Gleixner <t...@linutronix.de>
> Signed-off-by: Anna-Maria Gleixner <anna-ma...@linutronix.de>
> Cc: Felipe Balbi <ba...@kernel.org>
> Cc: linux-...@vger.kernel.org

This doesn't compile, so I'm assuming it depends on previous patches on
this series?

In that case:

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 35/36] usb/gadget/NCM: Replace tasklet with softirq hrtimer

[Felipe Balbi]

Anna-Maria Gleixner  writes:

> From: Thomas Gleixner 
>
> The tx_tasklet tasklet is used in invoke the hrtimer (task_timer) in
> softirq context. This can be also achieved without the tasklet but
> with HRTIMER_MODE_SOFT as hrtimer mode.
>
> Signed-off-by: Thomas Gleixner 
> Signed-off-by: Anna-Maria Gleixner 
> Cc: Felipe Balbi 
> Cc: linux-...@vger.kernel.org

This doesn't compile, so I'm assuming it depends on previous patches on
this series?

In that case:

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] ARM: dts: dra7: Disable USB metastability workaround for USB2

[Felipe Balbi]

Roger Quadros <rog...@ti.com> writes:

> The metastability workaround causes Erratic errors [1]
> on the HighSpeed USB PHY which can cause upto 2 seconds
> delay in enumerating to a USB host while in Gadget mode.
>
> Disable the Run/Stop metastability workaround to avoid this
> ill effect. We are aware that this opens up the opportunity
> for Run/Stop metastability, however this issue has never been
> observed in TI releases so we think that Run/Stop metastability
> is a lesser evil than the PHY Erratic errors. So disable it.
>
> [1] USB controller trace during gadget enumeration
> irq/90-dwc3-969   [000] d...52.323145: dwc3_event: event (0901): 
> Erratic Error [U0]
> irq/90-dwc3-969   [000] d...52.560646: dwc3_event: event (0901): 
> Erratic Error [U0]
> irq/90-dwc3-969   [000] d...52.798144: dwc3_event: event (0901): 
> Erratic Error [U0]
>
> Signed-off-by: Roger Quadros <rog...@ti.com>

FWIW:

Acked-by: Felipe Balbi <felipe.balbi@linux/intel.com>

I'm taking the dwc3 counterpart to v4.16

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] ARM: dts: dra7: Disable USB metastability workaround for USB2

[Felipe Balbi]

Roger Quadros  writes:

> The metastability workaround causes Erratic errors [1]
> on the HighSpeed USB PHY which can cause upto 2 seconds
> delay in enumerating to a USB host while in Gadget mode.
>
> Disable the Run/Stop metastability workaround to avoid this
> ill effect. We are aware that this opens up the opportunity
> for Run/Stop metastability, however this issue has never been
> observed in TI releases so we think that Run/Stop metastability
> is a lesser evil than the PHY Erratic errors. So disable it.
>
> [1] USB controller trace during gadget enumeration
> irq/90-dwc3-969   [000] d...52.323145: dwc3_event: event (0901): 
> Erratic Error [U0]
> irq/90-dwc3-969   [000] d...52.560646: dwc3_event: event (0901): 
> Erratic Error [U0]
> irq/90-dwc3-969   [000] d...52.798144: dwc3_event: event (0901): 
> Erratic Error [U0]
>
> Signed-off-by: Roger Quadros 

FWIW:

Acked-by: Felipe Balbi 

I'm taking the dwc3 counterpart to v4.16

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/1] USB Audio Device Class 3.0 Gadget support

[Felipe Balbi]

Hi,

Ruslan Bilovol  writes:
> On Tue, Nov 7, 2017 at 3:52 AM, Ruslan Bilovol  
> wrote:
>> Hi,
>>
>> This patch adds USB Audio Device Class 3.0 [1] function
>> support to gadget subsystem.
>> I didn't add UAC3 support to legacy gadget as it will
>> make preprocessor configuration too complex (UAC3 device
>> must have two configurations for backward compatibility,
>> first is UAC1/2 and second is UAC3), yet also I'm too lazy
>> to do that and verify all possible configurations.
>>
>> For modern ConfigFS interface I'll provide my configuration
>> for testing below; testing was done on a BeagleBone Black
>> board.
>>
>> This patch depends on uac3 header files from include dir
>> which I'll post as part of ALSA host UAC3 patch and will
>> provide the link to it here.
>
> http://www.spinics.net/lists/alsa-devel/msg69071.html

Once that patch hits upstream, then we can queue this for merge window
otherwise we will just have issues and create unbisectable points in the
tree.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 0/1] USB Audio Device Class 3.0 Gadget support

[Felipe Balbi]

Hi,

Ruslan Bilovol  writes:
> On Tue, Nov 7, 2017 at 3:52 AM, Ruslan Bilovol  
> wrote:
>> Hi,
>>
>> This patch adds USB Audio Device Class 3.0 [1] function
>> support to gadget subsystem.
>> I didn't add UAC3 support to legacy gadget as it will
>> make preprocessor configuration too complex (UAC3 device
>> must have two configurations for backward compatibility,
>> first is UAC1/2 and second is UAC3), yet also I'm too lazy
>> to do that and verify all possible configurations.
>>
>> For modern ConfigFS interface I'll provide my configuration
>> for testing below; testing was done on a BeagleBone Black
>> board.
>>
>> This patch depends on uac3 header files from include dir
>> which I'll post as part of ALSA host UAC3 patch and will
>> provide the link to it here.
>
> http://www.spinics.net/lists/alsa-devel/msg69071.html

Once that patch hits upstream, then we can queue this for merge window
otherwise we will just have issues and create unbisectable points in the
tree.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/1] usb: gadget: u_serial: Use kfifo instead of homemade circular buffer

[Felipe Balbi]

Hi,

Lu Baolu  writes:
> The kernel FIFO implementation, kfifo, provides interfaces to manipulate
> a first-in-first-out circular buffer.  Use kfifo instead of the homemade
> one to make the code more concise and readable.
>
> Signed-off-by: Lu Baolu 

Thanks :-) Can you give a little description of how you tested this?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 1/1] usb: gadget: u_serial: Use kfifo instead of homemade circular buffer

[Felipe Balbi]

Hi,

Lu Baolu  writes:
> The kernel FIFO implementation, kfifo, provides interfaces to manipulate
> a first-in-first-out circular buffer.  Use kfifo instead of the homemade
> one to make the code more concise and readable.
>
> Signed-off-by: Lu Baolu 

Thanks :-) Can you give a little description of how you tested this?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping <j...@metanate.com> writes:
> On Mon, 13 Nov 2017 12:57:21 +0200, Felipe Balbi wrote:
>> Good point. Then how about we just force the value to 1 in f_fs.c and
>> remove the check?
>
> That seems reasonable.  Something like this?
>
> -- >8 --
> Subject: [PATCH] usb: f_fs: Force Reserved1=1 in OS_DESC_EXT_COMPAT
>
> The specification says that the Reserved1 field in OS_DESC_EXT_COMPAT
> must have the value "1", but when this feature was first implemented we
> rejected any non-zero values.
>
> This was adjusted to accept all non-zero values (while now rejecting
> zero) in commit 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on
> reserved1 of OS_DESC_EXT_COMPAT"), but that breaks any userspace
> programs that worked previously by returning EINVAL when Reserved1 == 0
> which was previously the only value that succeeded!
>
> If we just set the field to "1" ourselves, both old and new userspace
> programs continue to work correctly and, as a bonus, old programs are
> now compliant with the specification without having to fix anything
> themselves.
>
> Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
> OS_DESC_EXT_COMPAT")
> Cc: sta...@vger.kernel.org
> Signed-off-by: John Keeping <j...@metanate.com>
> ---
>  drivers/usb/gadget/function/f_fs.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 652397eda6d6..520a96e7ef9a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2282,9 +2282,18 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type 
> type,
>   int i;
>  
>   if (len < sizeof(*d) ||
> - d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> - !d->Reserved1)
> + d->bFirstInterfaceNumber >= ffs->interfaces_count)
>   return -EINVAL;
> + if (d->Reserved1 != 1) {
> + /*
> +  * According to the spec, Reserved1 must be set to 1
> +  * but older kernels incorrectly rejected non-zero
> +  * values.  We fix it here to avoid returning EINVAL
> +  * in response to values we used to accept.
> +  */
> + pr_debug("usb_ext_compat_desc::Reserved1 forced to 
> 1\n");
> + d->Reserved1 = 1;
> + }

exactly like that. Care to send as a formal patch so I can apply?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping  writes:
> On Mon, 13 Nov 2017 12:57:21 +0200, Felipe Balbi wrote:
>> Good point. Then how about we just force the value to 1 in f_fs.c and
>> remove the check?
>
> That seems reasonable.  Something like this?
>
> -- >8 --
> Subject: [PATCH] usb: f_fs: Force Reserved1=1 in OS_DESC_EXT_COMPAT
>
> The specification says that the Reserved1 field in OS_DESC_EXT_COMPAT
> must have the value "1", but when this feature was first implemented we
> rejected any non-zero values.
>
> This was adjusted to accept all non-zero values (while now rejecting
> zero) in commit 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on
> reserved1 of OS_DESC_EXT_COMPAT"), but that breaks any userspace
> programs that worked previously by returning EINVAL when Reserved1 == 0
> which was previously the only value that succeeded!
>
> If we just set the field to "1" ourselves, both old and new userspace
> programs continue to work correctly and, as a bonus, old programs are
> now compliant with the specification without having to fix anything
> themselves.
>
> Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
> OS_DESC_EXT_COMPAT")
> Cc: sta...@vger.kernel.org
> Signed-off-by: John Keeping 
> ---
>  drivers/usb/gadget/function/f_fs.c | 13 +++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 652397eda6d6..520a96e7ef9a 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2282,9 +2282,18 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type 
> type,
>   int i;
>  
>   if (len < sizeof(*d) ||
> - d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> - !d->Reserved1)
> + d->bFirstInterfaceNumber >= ffs->interfaces_count)
>   return -EINVAL;
> + if (d->Reserved1 != 1) {
> + /*
> +  * According to the spec, Reserved1 must be set to 1
> +  * but older kernels incorrectly rejected non-zero
> +  * values.  We fix it here to avoid returning EINVAL
> +  * in response to values we used to accept.
> +  */
> + pr_debug("usb_ext_compat_desc::Reserved1 forced to 
> 1\n");
> + d->Reserved1 = 1;
> + }

exactly like that. Care to send as a formal patch so I can apply?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
>> Ran Wang  writes:
>> > Add support for USB3 snooping by asserting bits in register
>> > DWC3_GSBUSCFG0 for data and descriptor.
>> 
>> we know *how* to enable a feature :-) It's always the same, you fiddle with
>> some registers and it works. What you failed to tell us is:
>> 
>> a) WHY do you need this?
>> b) WHY do we need another DT property for this?
>> c) WHAT does this mean for PCI devices?
>
> So far I cannot have the answer for you, will get you back after some 
> discussion
> with my colleagues.

IOW, you have no idea why you need this, right? We're not patching
things for the sake of patching things. We need to understand what these
changes mean to the HW before we send out a patch publicly.

Remember that the moment a patch like this is accepted, it has the
potential of changing behavior for *ALL* users.

>> > +  }
>> > +
>> > +  dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
>> 
>> this will *always* read and write GSBUSCFG0 even for those platforms which
>> don't need to change anything on this register. You should just bail out 
>> early
>> if !dwc->dma_coherent
>> 
>> Also, I think dma_coherent is likely not the best name for this property.
>> 
>> Another question is: Why wasn't this setup properly during coreConsultant
>> instantiation of the RTL? Do you have devices on the market already that
>> need this or is this some early FPGA model or test-only ASIC?
>
> Yes, you are right. Actually I thought that all dwc3 IP  will have this 
> register, and
> it can be controlled by DTS property. 

they all *have* the register, however, it's sort of expected that RTL
engineer will setup good defaults when instantiating the RTL using SNPS'
coreConsultant tool.

Does your platform work without this patch?

>> > +
>> >  /* Global Debug Queue/FIFO Space Available Register */
>> >  #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f)
>> >  #define DWC3_GDBGFIFOSPACE_TYPE(n)(((n) << 5) & 0x1e0)
>> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>> >   *3   - Reserved
>> >   * @imod_interval: set the interrupt moderation interval in 250ns
>> >   * increments or 0 to disable.
>> > + * @dma_coherent: set if enable dma-coherent.
>> 
>> you're not enabling dma coherency, you're enabling cache snooping. And
>> this property should describe that. Also, keep in mind that different devices
>> may want different cache types for each of those fields, so your property
>> would have to be a lot more complex. Something like:
>> 
>>  snps,cache-type = , , ...
>> 
>> Then driver would have to parse this properly to setup GSBUSCFG0.
>
> Got it, learn a lot, need more time to digest and test, thanks for
> your patiently explanation.

no problem, please figure out the answers to my previous questions,
without which I can't accept your patch.

>> In any
>> case, I still want to know why do you really need this? What's the reason?
>> What happens if you don't fix GSBUSCFG0? What's the value you have there
>> by default? Why isn't that default good enough?
>
> So far the Layerscape SoC (such as LS1088A) has enabled this feature and I
> have tested it. Once we add dma-coherent on DTS without this Patch, dwc3
> will fail on device enumeration as below:
> [   15.124031] xhci-hcd xhci-hcd.0.auto: Error while assigning device slot ID
> [   15.130912] xhci-hcd xhci-hcd.0.auto: Max number of devices this xHCI host 
> supports is 127.
> [   15.139268] usb usb1-port1: couldn't allocate usb_device

okay, so without these changes, your host doesn't work. What is the
default value on your platform without these changes? (revert patch,
boot platform, let it fail, get register output from our regdump in debugfs)

-- 
balbi

RE: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
>> Ran Wang  writes:
>> > Add support for USB3 snooping by asserting bits in register
>> > DWC3_GSBUSCFG0 for data and descriptor.
>> 
>> we know *how* to enable a feature :-) It's always the same, you fiddle with
>> some registers and it works. What you failed to tell us is:
>> 
>> a) WHY do you need this?
>> b) WHY do we need another DT property for this?
>> c) WHAT does this mean for PCI devices?
>
> So far I cannot have the answer for you, will get you back after some 
> discussion
> with my colleagues.

IOW, you have no idea why you need this, right? We're not patching
things for the sake of patching things. We need to understand what these
changes mean to the HW before we send out a patch publicly.

Remember that the moment a patch like this is accepted, it has the
potential of changing behavior for *ALL* users.

>> > +  }
>> > +
>> > +  dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);
>> 
>> this will *always* read and write GSBUSCFG0 even for those platforms which
>> don't need to change anything on this register. You should just bail out 
>> early
>> if !dwc->dma_coherent
>> 
>> Also, I think dma_coherent is likely not the best name for this property.
>> 
>> Another question is: Why wasn't this setup properly during coreConsultant
>> instantiation of the RTL? Do you have devices on the market already that
>> need this or is this some early FPGA model or test-only ASIC?
>
> Yes, you are right. Actually I thought that all dwc3 IP  will have this 
> register, and
> it can be controlled by DTS property. 

they all *have* the register, however, it's sort of expected that RTL
engineer will setup good defaults when instantiating the RTL using SNPS'
coreConsultant tool.

Does your platform work without this patch?

>> > +
>> >  /* Global Debug Queue/FIFO Space Available Register */
>> >  #define DWC3_GDBGFIFOSPACE_NUM(n) ((n) & 0x1f)
>> >  #define DWC3_GDBGFIFOSPACE_TYPE(n)(((n) << 5) & 0x1e0)
>> > @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>> >   *3   - Reserved
>> >   * @imod_interval: set the interrupt moderation interval in 250ns
>> >   * increments or 0 to disable.
>> > + * @dma_coherent: set if enable dma-coherent.
>> 
>> you're not enabling dma coherency, you're enabling cache snooping. And
>> this property should describe that. Also, keep in mind that different devices
>> may want different cache types for each of those fields, so your property
>> would have to be a lot more complex. Something like:
>> 
>>  snps,cache-type = , , ...
>> 
>> Then driver would have to parse this properly to setup GSBUSCFG0.
>
> Got it, learn a lot, need more time to digest and test, thanks for
> your patiently explanation.

no problem, please figure out the answers to my previous questions,
without which I can't accept your patch.

>> In any
>> case, I still want to know why do you really need this? What's the reason?
>> What happens if you don't fix GSBUSCFG0? What's the value you have there
>> by default? Why isn't that default good enough?
>
> So far the Layerscape SoC (such as LS1088A) has enabled this feature and I
> have tested it. Once we add dma-coherent on DTS without this Patch, dwc3
> will fail on device enumeration as below:
> [   15.124031] xhci-hcd xhci-hcd.0.auto: Error while assigning device slot ID
> [   15.130912] xhci-hcd xhci-hcd.0.auto: Max number of devices this xHCI host 
> supports is 127.
> [   15.139268] usb usb1-port1: couldn't allocate usb_device

okay, so without these changes, your host doesn't work. What is the
default value on your platform without these changes? (revert patch,
boot platform, let it fail, get register output from our regdump in debugfs)

-- 
balbi

Re: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
> Add support for USB3 snooping by asserting bits
> in register DWC3_GSBUSCFG0 for data and descriptor.

we know *how* to enable a feature :-) It's always the same, you fiddle
with some registers and it works. What you failed to tell us is:

a) WHY do you need this?
b) WHY do we need another DT property for this?
c) WHAT does this mean for PCI devices?

> Signed-off-by: Changming Huang 
> Signed-off-by: Rajesh Bhagat 
> Signed-off-by: Ran Wang 
> ---
>  drivers/usb/dwc3/core.c | 24 
>  drivers/usb/dwc3/core.h | 10 ++
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 07832509584f..ffc078ab4a1c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -236,6 +236,26 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>   return -ETIMEDOUT;
>  }
>  
> +/*
> + * dwc3_enable_snooping - Enable snooping feature
> + * @dwc3: Pointer to our controller context structure
> + */
> +static void dwc3_enable_snooping(struct dwc3 *dwc)
> +{
> + u32 cfg;
> +
> + cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> + if (dwc->dma_coherent) {
> + cfg &= ~DWC3_GSBUSCFG0_SNP_MASK;
> + cfg |= (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATARD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCRD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATAWR_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCWR_SHIFT);

This "value << shift" looks super clumsy. I would rather have something
akin to:

cfg |= DWC3_GSBUSCFG0_DATARD_CACHEABLE |
DWC3_GSBUSCFG0_DESCRD_CACHEABLE ...

and so on.

> + }
> +
> + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);

this will *always* read and write GSBUSCFG0 even for those platforms
which don't need to change anything on this register. You should just
bail out early if !dwc->dma_coherent

Also, I think dma_coherent is likely not the best name for this property.

Another question is: Why wasn't this setup properly during
coreConsultant instantiation of the RTL? Do you have devices on the
market already that need this or is this some early FPGA model or
test-only ASIC?

> @@ -776,6 +796,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>   /* Adjust Frame Length */
>   dwc3_frame_length_adjustment(dwc);
>  
> + dwc3_enable_snooping(dwc);
> +
>   usb_phy_set_suspend(dwc->usb2_phy, 0);
>   usb_phy_set_suspend(dwc->usb3_phy, 0);
>   ret = phy_power_on(dwc->usb2_generic_phy);
> @@ -1021,6 +1043,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>   _threshold);
>   dwc->usb3_lpm_capable = device_property_read_bool(dev,
>   "snps,usb3_lpm_capable");
> + dwc->dma_coherent = device_property_read_bool(dev,
> + "dma-coherent");
>  
>   dwc->disable_scramble_quirk = device_property_read_bool(dev,
>   "snps,disable_scramble_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4a4a4c98508c..6e6a66650e53 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -153,6 +153,14 @@
>  
>  /* Bit fields */
>  
> +/* Global SoC Bus Configuration Register 0 */
> +#define AXI3_CACHE_TYPE_SNP  0x2 /* cacheable */
> +#define DWC3_GSBUSCFG0_DATARD_SHIFT  28
> +#define DWC3_GSBUSCFG0_DESCRD_SHIFT  24
> +#define DWC3_GSBUSCFG0_DATAWR_SHIFT  20
> +#define DWC3_GSBUSCFG0_DESCWR_SHIFT  16
> +#define DWC3_GSBUSCFG0_SNP_MASK  0x



> +
>  /* Global Debug Queue/FIFO Space Available Register */
>  #define DWC3_GDBGFIFOSPACE_NUM(n)((n) & 0x1f)
>  #define DWC3_GDBGFIFOSPACE_TYPE(n)   (((n) << 5) & 0x1e0)
> @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>   *   3   - Reserved
>   * @imod_interval: set the interrupt moderation interval in 250ns
>   * increments or 0 to disable.
> + * @dma_coherent: set if enable dma-coherent.

you're not enabling dma coherency, you're enabling cache snooping. And
this property should describe that. Also, keep in mind that different
devices may want different cache types for each of those fields, so your
property would have to be a lot more complex. Something like:

snps,cache-type = , , ...

Then driver would have to parse this properly to setup GSBUSCFG0. In any
case, I still want to know why do you really need this? What's the
reason? What happens if you don't fix GSBUSCFG0? What's the value you
have there by default? Why isn't that default good enough?

ps: since you're fiddling with DT, you should also include
devicetree@vger

-- 
balbi

Re: [PATCH] usb: dwc3: Enable the USB snooping

[Felipe Balbi]

Hi,

Ran Wang  writes:
> Add support for USB3 snooping by asserting bits
> in register DWC3_GSBUSCFG0 for data and descriptor.

we know *how* to enable a feature :-) It's always the same, you fiddle
with some registers and it works. What you failed to tell us is:

a) WHY do you need this?
b) WHY do we need another DT property for this?
c) WHAT does this mean for PCI devices?

> Signed-off-by: Changming Huang 
> Signed-off-by: Rajesh Bhagat 
> Signed-off-by: Ran Wang 
> ---
>  drivers/usb/dwc3/core.c | 24 
>  drivers/usb/dwc3/core.h | 10 ++
>  2 files changed, 34 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 07832509584f..ffc078ab4a1c 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -236,6 +236,26 @@ static int dwc3_core_soft_reset(struct dwc3 *dwc)
>   return -ETIMEDOUT;
>  }
>  
> +/*
> + * dwc3_enable_snooping - Enable snooping feature
> + * @dwc3: Pointer to our controller context structure
> + */
> +static void dwc3_enable_snooping(struct dwc3 *dwc)
> +{
> + u32 cfg;
> +
> + cfg = dwc3_readl(dwc->regs, DWC3_GSBUSCFG0);
> + if (dwc->dma_coherent) {
> + cfg &= ~DWC3_GSBUSCFG0_SNP_MASK;
> + cfg |= (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATARD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCRD_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DATAWR_SHIFT) |
> + (AXI3_CACHE_TYPE_SNP << DWC3_GSBUSCFG0_DESCWR_SHIFT);

This "value << shift" looks super clumsy. I would rather have something
akin to:

cfg |= DWC3_GSBUSCFG0_DATARD_CACHEABLE |
DWC3_GSBUSCFG0_DESCRD_CACHEABLE ...

and so on.

> + }
> +
> + dwc3_writel(dwc->regs, DWC3_GSBUSCFG0, cfg);

this will *always* read and write GSBUSCFG0 even for those platforms
which don't need to change anything on this register. You should just
bail out early if !dwc->dma_coherent

Also, I think dma_coherent is likely not the best name for this property.

Another question is: Why wasn't this setup properly during
coreConsultant instantiation of the RTL? Do you have devices on the
market already that need this or is this some early FPGA model or
test-only ASIC?

> @@ -776,6 +796,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>   /* Adjust Frame Length */
>   dwc3_frame_length_adjustment(dwc);
>  
> + dwc3_enable_snooping(dwc);
> +
>   usb_phy_set_suspend(dwc->usb2_phy, 0);
>   usb_phy_set_suspend(dwc->usb3_phy, 0);
>   ret = phy_power_on(dwc->usb2_generic_phy);
> @@ -1021,6 +1043,8 @@ static void dwc3_get_properties(struct dwc3 *dwc)
>   _threshold);
>   dwc->usb3_lpm_capable = device_property_read_bool(dev,
>   "snps,usb3_lpm_capable");
> + dwc->dma_coherent = device_property_read_bool(dev,
> + "dma-coherent");
>  
>   dwc->disable_scramble_quirk = device_property_read_bool(dev,
>   "snps,disable_scramble_quirk");
> diff --git a/drivers/usb/dwc3/core.h b/drivers/usb/dwc3/core.h
> index 4a4a4c98508c..6e6a66650e53 100644
> --- a/drivers/usb/dwc3/core.h
> +++ b/drivers/usb/dwc3/core.h
> @@ -153,6 +153,14 @@
>  
>  /* Bit fields */
>  
> +/* Global SoC Bus Configuration Register 0 */
> +#define AXI3_CACHE_TYPE_SNP  0x2 /* cacheable */
> +#define DWC3_GSBUSCFG0_DATARD_SHIFT  28
> +#define DWC3_GSBUSCFG0_DESCRD_SHIFT  24
> +#define DWC3_GSBUSCFG0_DATAWR_SHIFT  20
> +#define DWC3_GSBUSCFG0_DESCWR_SHIFT  16
> +#define DWC3_GSBUSCFG0_SNP_MASK  0x



> +
>  /* Global Debug Queue/FIFO Space Available Register */
>  #define DWC3_GDBGFIFOSPACE_NUM(n)((n) & 0x1f)
>  #define DWC3_GDBGFIFOSPACE_TYPE(n)   (((n) << 5) & 0x1e0)
> @@ -859,6 +867,7 @@ struct dwc3_scratchpad_array {
>   *   3   - Reserved
>   * @imod_interval: set the interrupt moderation interval in 250ns
>   * increments or 0 to disable.
> + * @dma_coherent: set if enable dma-coherent.

you're not enabling dma coherency, you're enabling cache snooping. And
this property should describe that. Also, keep in mind that different
devices may want different cache types for each of those fields, so your
property would have to be a lot more complex. Something like:

snps,cache-type = , , ...

Then driver would have to parse this properly to setup GSBUSCFG0. In any
case, I still want to know why do you really need this? What's the
reason? What happens if you don't fix GSBUSCFG0? What's the value you
have there by default? Why isn't that default good enough?

ps: since you're fiddling with DT, you should also include
devicetree@vger

-- 
balbi

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
>> +static int dbc_buf_alloc(struct dbc_buf *db, unsigned int size)
>> +{
>> +db->buf_buf = kzalloc(size, GFP_KERNEL);
>> +if (!db->buf_buf)
>> +return -ENOMEM;
>> +
>> +db->buf_size = size;
>> +db->buf_put = db->buf_buf;
>> +db->buf_get = db->buf_buf;
>> +
>> +return 0;
>> +}

you may wanna have a look at kfifo.

-- 
balbi

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Mathias Nyman  writes:
>> +static int dbc_buf_alloc(struct dbc_buf *db, unsigned int size)
>> +{
>> +db->buf_buf = kzalloc(size, GFP_KERNEL);
>> +if (!db->buf_buf)
>> +return -ENOMEM;
>> +
>> +db->buf_size = size;
>> +db->buf_put = db->buf_buf;
>> +db->buf_get = db->buf_buf;
>> +
>> +return 0;
>> +}

you may wanna have a look at kfifo.

-- 
balbi

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping <j...@metanate.com> writes:
> On Fri, 10 Nov 2017 12:40:39 +0200, Felipe Balbi wrote:
>
>> John Keeping <j...@metanate.com> writes:
>> > This check has gone through several incompatible variations in commits
>> > 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of
>> > OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat
>> > descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
>> > Fix ExtCompat descriptor validation"") after initially being introduced
>> > in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
>> >
>> > The various changes make it impossible for a single userspace
>> > implementation to work with different kernel versions, so let's just
>> > drop the condition to avoid breaking userspace.
>> >
>> > Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
>> > OS_DESC_EXT_COMPAT")
>> > Cc: sta...@vger.kernel.org # v4.7+
>> > Signed-off-by: John Keeping <j...@metanate.com>
>> > ---
>> >  drivers/usb/gadget/function/f_fs.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/function/f_fs.c 
>> > b/drivers/usb/gadget/function/f_fs.c
>> > index 652397eda6d6..0d9962834345 100644
>> > --- a/drivers/usb/gadget/function/f_fs.c
>> > +++ b/drivers/usb/gadget/function/f_fs.c
>> > @@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum 
>> > ffs_os_desc_type type,
>> >int i;
>> >  
>> >if (len < sizeof(*d) ||
>> > -  d->bFirstInterfaceNumber >= ffs->interfaces_count ||
>> > -  !d->Reserved1)
>> > +  d->bFirstInterfaceNumber >= ffs->interfaces_count)
>> >return -EINVAL;
>> >for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
>> >if (d->Reserved2[i])  
>> 
>> Sorry, but no. We want to be compliant with the specification. If there
>> are older still-maintained stable trees which are not working, we need
>> to backport a fix to them, but we're not allowing uncompliant
>> implementations.
>
> Aren't we allowing non-compliant implementations now?  The spec says the
> value must be 1 but since v4.7 this code has allowed all non-zero
> values.

Good point. Then how about we just force the value to 1 in f_fs.c and
remove the check?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping  writes:
> On Fri, 10 Nov 2017 12:40:39 +0200, Felipe Balbi wrote:
>
>> John Keeping  writes:
>> > This check has gone through several incompatible variations in commits
>> > 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of
>> > OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat
>> > descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
>> > Fix ExtCompat descriptor validation"") after initially being introduced
>> > in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
>> >
>> > The various changes make it impossible for a single userspace
>> > implementation to work with different kernel versions, so let's just
>> > drop the condition to avoid breaking userspace.
>> >
>> > Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
>> > OS_DESC_EXT_COMPAT")
>> > Cc: sta...@vger.kernel.org # v4.7+
>> > Signed-off-by: John Keeping 
>> > ---
>> >  drivers/usb/gadget/function/f_fs.c | 3 +--
>> >  1 file changed, 1 insertion(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/usb/gadget/function/f_fs.c 
>> > b/drivers/usb/gadget/function/f_fs.c
>> > index 652397eda6d6..0d9962834345 100644
>> > --- a/drivers/usb/gadget/function/f_fs.c
>> > +++ b/drivers/usb/gadget/function/f_fs.c
>> > @@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum 
>> > ffs_os_desc_type type,
>> >int i;
>> >  
>> >if (len < sizeof(*d) ||
>> > -  d->bFirstInterfaceNumber >= ffs->interfaces_count ||
>> > -  !d->Reserved1)
>> > +  d->bFirstInterfaceNumber >= ffs->interfaces_count)
>> >return -EINVAL;
>> >for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
>> >if (d->Reserved2[i])  
>> 
>> Sorry, but no. We want to be compliant with the specification. If there
>> are older still-maintained stable trees which are not working, we need
>> to backport a fix to them, but we're not allowing uncompliant
>> implementations.
>
> Aren't we allowing non-compliant implementations now?  The spec says the
> value must be 1 but since v4.7 this code has allowed all non-zero
> values.

Good point. Then how about we just force the value to 1 in f_fs.c and
remove the check?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] USB :core :Prevent USB devices to autosuspend while setting interface

[Felipe Balbi]

Hi,

abhij...@vger.kernel.org, ku...@vger.kernel.org writes:

these emails don't exist. Fix your email client.

> From: abhijeet kumar 

capitalize names

> Runtime resume USB device in order to ensure that PM framework
> knows that the we might be using the device in a short time and doesn't
> autosuspend the device while we updating it's interface.

this doesn't tell me about any problem. What, exactly, are you trying to
fix?

> Signed-off-by: abhijeet kumar 

capitalize names

> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 371a07d874a3..658603ed779e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1305,6 +1305,9 @@ int usb_set_interface(struct usb_device *dev, int 
> interface, int alternate)
>   if (iface->unregistering)
>   return -ENODEV;
>  
> + /*Letting runtime PM now that we wish to use the device in a short time
> +  *pm_runtime_get(>dev);
> +  */

why is it so that adding commented out code help? Did you *really* test
this at all?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] USB :core :Prevent USB devices to autosuspend while setting interface

[Felipe Balbi]

Hi,

abhij...@vger.kernel.org, ku...@vger.kernel.org writes:

these emails don't exist. Fix your email client.

> From: abhijeet kumar 

capitalize names

> Runtime resume USB device in order to ensure that PM framework
> knows that the we might be using the device in a short time and doesn't
> autosuspend the device while we updating it's interface.

this doesn't tell me about any problem. What, exactly, are you trying to
fix?

> Signed-off-by: abhijeet kumar 

capitalize names

> diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c
> index 371a07d874a3..658603ed779e 100644
> --- a/drivers/usb/core/message.c
> +++ b/drivers/usb/core/message.c
> @@ -1305,6 +1305,9 @@ int usb_set_interface(struct usb_device *dev, int 
> interface, int alternate)
>   if (iface->unregistering)
>   return -ENODEV;
>  
> + /*Letting runtime PM now that we wish to use the device in a short time
> +  *pm_runtime_get(>dev);
> +  */

why is it so that adding commented out code help? Did you *really* test
this at all?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping  writes:
> This check has gone through several incompatible variations in commits
> 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of
> OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat
> descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
> Fix ExtCompat descriptor validation"") after initially being introduced
> in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
>
> The various changes make it impossible for a single userspace
> implementation to work with different kernel versions, so let's just
> drop the condition to avoid breaking userspace.
>
> Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
> OS_DESC_EXT_COMPAT")
> Cc: sta...@vger.kernel.org # v4.7+
> Signed-off-by: John Keeping 
> ---
>  drivers/usb/gadget/function/f_fs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 652397eda6d6..0d9962834345 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type 
> type,
>   int i;
>  
>   if (len < sizeof(*d) ||
> - d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> - !d->Reserved1)
> + d->bFirstInterfaceNumber >= ffs->interfaces_count)
>   return -EINVAL;
>   for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
>   if (d->Reserved2[i])

Sorry, but no. We want to be compliant with the specification. If there
are older still-maintained stable trees which are not working, we need
to backport a fix to them, but we're not allowing uncompliant
implementations.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: f_fs: Drop check on Reserved1 field on OS_DESC_EXT_COMPAT

[Felipe Balbi]

Hi,

John Keeping  writes:
> This check has gone through several incompatible variations in commits
> 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of
> OS_DESC_EXT_COMPAT"), 354bc45bf329 ("usb: gadget: f_fs: Fix ExtCompat
> descriptor validation") and 3ba534df815f ("Revert "usb: gadget: f_fs:
> Fix ExtCompat descriptor validation"") after initially being introduced
> in commit f0175ab51993 ("usb: gadget: f_fs: OS descriptors support").
>
> The various changes make it impossible for a single userspace
> implementation to work with different kernel versions, so let's just
> drop the condition to avoid breaking userspace.
>
> Fixes: 53642399aa71 ("usb: gadget: f_fs: Fix wrong check on reserved1 of 
> OS_DESC_EXT_COMPAT")
> Cc: sta...@vger.kernel.org # v4.7+
> Signed-off-by: John Keeping 
> ---
>  drivers/usb/gadget/function/f_fs.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/usb/gadget/function/f_fs.c 
> b/drivers/usb/gadget/function/f_fs.c
> index 652397eda6d6..0d9962834345 100644
> --- a/drivers/usb/gadget/function/f_fs.c
> +++ b/drivers/usb/gadget/function/f_fs.c
> @@ -2282,8 +2282,7 @@ static int __ffs_data_do_os_desc(enum ffs_os_desc_type 
> type,
>   int i;
>  
>   if (len < sizeof(*d) ||
> - d->bFirstInterfaceNumber >= ffs->interfaces_count ||
> - !d->Reserved1)
> + d->bFirstInterfaceNumber >= ffs->interfaces_count)
>   return -EINVAL;
>   for (i = 0; i < ARRAY_SIZE(d->Reserved2); ++i)
>   if (d->Reserved2[i])

Sorry, but no. We want to be compliant with the specification. If there
are older still-maintained stable trees which are not working, we need
to backport a fix to them, but we're not allowing uncompliant
implementations.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: Fix use-after-free in ffs_free_inst

[Felipe Balbi]

Greg KH <g...@kroah.com> writes:

> On Wed, Nov 08, 2017 at 10:13:15AM -0700, Andrew Gabbasov wrote:
>> KASAN enabled configuration reports an error
>> 
>> BUG: KASAN: use-after-free in ffs_free_inst+... [usb_f_fs] at addr ...
>> Write of size 8 by task ...
>> 
>> This is observed after "ffs-test" is run and interrupted. If after that
>> functionfs is unmounted and g_ffs module is unloaded, that use-after-free
>> occurs during g_ffs module removal.
>> 
>> Although the report indicates ffs_free_inst() function, the actual
>> use-after-free condition occurs in _ffs_free_dev() function, which
>> is probably inlined into ffs_free_inst().
>> 
>> This happens due to keeping the ffs_data reference in device structure
>> during functionfs unmounting, while ffs_data itself is freed as no longer
>> needed. The fix is to clear that reference in ffs_closed() function,
>> which is a counterpart of ffs_ready(), where the reference is stored.
>> 
>> Fixes: 3262ad824307 ("usb: gadget: f_fs: Stop ffs_closed NULL pointer 
>> dereference")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Andrew Gabbasov <andrew_gabba...@mentor.com>
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Felipe, want me to take this directly?

If you can still squeeze it into the merge window, sure:

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

Thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_fs: Fix use-after-free in ffs_free_inst

[Felipe Balbi]

Greg KH  writes:

> On Wed, Nov 08, 2017 at 10:13:15AM -0700, Andrew Gabbasov wrote:
>> KASAN enabled configuration reports an error
>> 
>> BUG: KASAN: use-after-free in ffs_free_inst+... [usb_f_fs] at addr ...
>> Write of size 8 by task ...
>> 
>> This is observed after "ffs-test" is run and interrupted. If after that
>> functionfs is unmounted and g_ffs module is unloaded, that use-after-free
>> occurs during g_ffs module removal.
>> 
>> Although the report indicates ffs_free_inst() function, the actual
>> use-after-free condition occurs in _ffs_free_dev() function, which
>> is probably inlined into ffs_free_inst().
>> 
>> This happens due to keeping the ffs_data reference in device structure
>> during functionfs unmounting, while ffs_data itself is freed as no longer
>> needed. The fix is to clear that reference in ffs_closed() function,
>> which is a counterpart of ffs_ready(), where the reference is stored.
>> 
>> Fixes: 3262ad824307 ("usb: gadget: f_fs: Stop ffs_closed NULL pointer 
>> dereference")
>> Cc: sta...@vger.kernel.org
>> Signed-off-by: Andrew Gabbasov 
>> ---
>>  drivers/usb/gadget/function/f_fs.c | 1 +
>>  1 file changed, 1 insertion(+)
>
> Felipe, want me to take this directly?

If you can still squeeze it into the merge window, sure:

Acked-by: Felipe Balbi 

Thanks

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 3/3] usb: dwc3: Enable USB 3.0 phy driver

[Felipe Balbi]

Hi,

Ran Wang  writes:
> Adds entry point at dwc3 core init function to enable
> USB 3.0 PHY driver.
>
> Signed-off-by: Ran Wang 
> ---
> Change in v2:
>   - New file
>
>  drivers/usb/dwc3/core.c | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 03474d3575ab..a9df03c64a7b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -746,6 +746,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  {
>   u32 reg;
>   int ret;
> + int retval;
> + struct phy *phy;
>  
>   if (!dwc3_core_is_valid(dwc)) {
>   dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
> @@ -770,6 +772,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>   if (ret)
>   goto err0;
>  
> + phy = phy_get(dwc->dev, "usb-phy");
> + if (IS_ERR(phy)) {
> + retval = PTR_ERR(phy);
> + if (retval == -EPROBE_DEFER)
> + dev_dbg(dwc->dev, "usb-phy no found\n");
> + } else {
> + retval = phy_init(phy);
> + if (retval) {
> + phy_put(phy);
> + dev_dbg(dwc->dev, "phy_init() error!\n");
> + }
> + }
> +

what We already do this. See dwc3_core_get_phy().

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2 3/3] usb: dwc3: Enable USB 3.0 phy driver

[Felipe Balbi]

Hi,

Ran Wang  writes:
> Adds entry point at dwc3 core init function to enable
> USB 3.0 PHY driver.
>
> Signed-off-by: Ran Wang 
> ---
> Change in v2:
>   - New file
>
>  drivers/usb/dwc3/core.c | 15 +++
>  1 file changed, 15 insertions(+)
>
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index 03474d3575ab..a9df03c64a7b 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -746,6 +746,8 @@ static int dwc3_core_init(struct dwc3 *dwc)
>  {
>   u32 reg;
>   int ret;
> + int retval;
> + struct phy *phy;
>  
>   if (!dwc3_core_is_valid(dwc)) {
>   dev_err(dwc->dev, "this is not a DesignWare USB3 DRD Core\n");
> @@ -770,6 +772,19 @@ static int dwc3_core_init(struct dwc3 *dwc)
>   if (ret)
>   goto err0;
>  
> + phy = phy_get(dwc->dev, "usb-phy");
> + if (IS_ERR(phy)) {
> + retval = PTR_ERR(phy);
> + if (retval == -EPROBE_DEFER)
> + dev_dbg(dwc->dev, "usb-phy no found\n");
> + } else {
> + retval = phy_init(phy);
> + if (retval) {
> + phy_put(phy);
> + dev_dbg(dwc->dev, "phy_init() error!\n");
> + }
> + }
> +

what We already do this. See dwc3_core_get_phy().

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>>  > >> Greg Kroah-Hartman  writes:
>> >>> > >> >> > xHCI compatible USB host controllers(i.e. super-speed USB3 
>> >>> > >> >> > controllers)
>> >>> > >> >> > can be implemented with the Debug Capability(DbC). It 
>> >>> > >> >> > presents a debug
>> >>> > >> >> > device which is fully compliant with the USB framework and 
>> >>> > >> >> > provides the
>> >>> > >> >> > equivalent of a very high performance full-duplex serial 
>> >>> > >> >> > link. The debug
>> >>> > >> >> > capability operation model and registers interface are 
>> >>> > >> >> > defined in 7.6.8
>> >>> > >> >> > of the xHCI specification, revision 1.1.
>> >>> > >> >> >
>> >>> > >> >> > The DbC debug device shares a root port with the xHCI 
>> >>> > >> >> > host. By default,
>> >>> > >> >> > the debug capability is disabled and the root port is 
>> >>> > >> >> > assigned to xHCI.
>> >>> > >> >> > When the DbC is enabled, the root port will be assigned to 
>> >>> > >> >> > the DbC debug
>> >>> > >> >> > device, and the xHCI sees nothing on this port. This 
>> >>> > >> >> > implementation uses
>> >>> > >> >> > a sysfs node named  under the xHCI device to manage 
>> >>> > >> >> > the enabling
>> >>> > >> >> > and disabling of the debug capability.
>> >>> > >> >> >
>> >>> > >> >> > When the debug capability is enabled, it will present a 
>> >>> > >> >> > debug device
>> >>> > >> >> > through the debug port. This debug device is fully 
>> >>> > >> >> > compliant with the
>> >>> > >> >> > USB3 framework, and it can be enumerated by a debug host 
>> >>> > >> >> > on the other
>> >>> > >> >> > end of the USB link. As soon as the debug device is 
>> >>> > >> >> > configured, a TTY
>> >>> > >> >> > serial device named /dev/ttyDBC0 will be created.
>> >>> > >> >> >
>> >>> > >> >> > One use of this link is running a login service on the 
>> >>> > >> >> > debug target.
>> >>> > >> >> > Hence it can be remote accessed by a debug host. Another 
>> >>> > >> >> > use case can
>> >>> > >> >> > probably be found in servers. It provides a peer-to-peer 
>> >>> > >> >> > USB link
>> >>> > >> >> > between two host-only machines. This provides a reasonable 
>> >>> > >> >> > out-of-band
>> >>> > >> >> > communication method between two servers.
>> >>> > >> >> >
>> >>> > >> >> > Signed-off-by: Lu Baolu 
>> >>> > >> >> > ---
>> >>> > >> >> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/Kconfig   |9 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/Makefile  |5 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> >>> > >> >> > 
>> >>> > >> >> >  drivers/usb/host/xhci-dbgcap.h |  247 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci-dbgtty.c |  586 
>> >>> > >> >> > +++
>> >>> > >> >> >  drivers/usb/host/xhci-trace.h  |   60 
>> >>> > >> >> > ++
>> >>> > >> >> >  drivers/usb/host/xhci.c|   10 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci.h|1 
>> >>> > >> >> > +
>> >>> > >> >> >  9 files changed, 1959 insertions(+)
>> >>> > >> >> >  create mode 100644 
>> >>> > >> >> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >>> > >> >> >
>> >> > >> >> 
>> >> > >> >> [snip]
>> >> > >> >> 
>> >>> > >> >> > +#define DBC_VENDOR_ID 0x1d6b  /* 
>> >>> > >> >> > Linux Foundation 0x1d6b */
>> >>> > >> >> > +#define DBC_PRODUCT_ID0x0004  /* 
>> >>> > >> >> > device 0004 */
>> >>> > >> >> >
>> >> > >> >> 
>> >> > >> >> The DbC (xHCI DeBug Capability) is an optional functionality 
>> >> > >> >> in
>> >> > >> >> some xHCI host controllers. It will present a super-speed 
>> >> > >> >> debug
>> >> > >> >> device through the debug port after it is enabled.
>> >> > >> >> 
>> >> > >> >> The DbC register set defines an interface for system software
>> >> > >> >> to specify the vendor id and product id of the debug device.
>> >> > >> >> These two values will be presented by the debug device in its
>> >> > >> >> device descriptor idVendor and idProduct fields.
>> >> > >> >> 
>> >> > >> >> Microsoft Windows have a well 

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>>  > >> Greg Kroah-Hartman  writes:
>> >>> > >> >> > xHCI compatible USB host controllers(i.e. super-speed USB3 
>> >>> > >> >> > controllers)
>> >>> > >> >> > can be implemented with the Debug Capability(DbC). It 
>> >>> > >> >> > presents a debug
>> >>> > >> >> > device which is fully compliant with the USB framework and 
>> >>> > >> >> > provides the
>> >>> > >> >> > equivalent of a very high performance full-duplex serial 
>> >>> > >> >> > link. The debug
>> >>> > >> >> > capability operation model and registers interface are 
>> >>> > >> >> > defined in 7.6.8
>> >>> > >> >> > of the xHCI specification, revision 1.1.
>> >>> > >> >> >
>> >>> > >> >> > The DbC debug device shares a root port with the xHCI 
>> >>> > >> >> > host. By default,
>> >>> > >> >> > the debug capability is disabled and the root port is 
>> >>> > >> >> > assigned to xHCI.
>> >>> > >> >> > When the DbC is enabled, the root port will be assigned to 
>> >>> > >> >> > the DbC debug
>> >>> > >> >> > device, and the xHCI sees nothing on this port. This 
>> >>> > >> >> > implementation uses
>> >>> > >> >> > a sysfs node named  under the xHCI device to manage 
>> >>> > >> >> > the enabling
>> >>> > >> >> > and disabling of the debug capability.
>> >>> > >> >> >
>> >>> > >> >> > When the debug capability is enabled, it will present a 
>> >>> > >> >> > debug device
>> >>> > >> >> > through the debug port. This debug device is fully 
>> >>> > >> >> > compliant with the
>> >>> > >> >> > USB3 framework, and it can be enumerated by a debug host 
>> >>> > >> >> > on the other
>> >>> > >> >> > end of the USB link. As soon as the debug device is 
>> >>> > >> >> > configured, a TTY
>> >>> > >> >> > serial device named /dev/ttyDBC0 will be created.
>> >>> > >> >> >
>> >>> > >> >> > One use of this link is running a login service on the 
>> >>> > >> >> > debug target.
>> >>> > >> >> > Hence it can be remote accessed by a debug host. Another 
>> >>> > >> >> > use case can
>> >>> > >> >> > probably be found in servers. It provides a peer-to-peer 
>> >>> > >> >> > USB link
>> >>> > >> >> > between two host-only machines. This provides a reasonable 
>> >>> > >> >> > out-of-band
>> >>> > >> >> > communication method between two servers.
>> >>> > >> >> >
>> >>> > >> >> > Signed-off-by: Lu Baolu 
>> >>> > >> >> > ---
>> >>> > >> >> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/Kconfig   |9 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/Makefile  |5 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> >>> > >> >> > 
>> >>> > >> >> >  drivers/usb/host/xhci-dbgcap.h |  247 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci-dbgtty.c |  586 
>> >>> > >> >> > +++
>> >>> > >> >> >  drivers/usb/host/xhci-trace.h  |   60 
>> >>> > >> >> > ++
>> >>> > >> >> >  drivers/usb/host/xhci.c|   10 
>> >>> > >> >> > +
>> >>> > >> >> >  drivers/usb/host/xhci.h|1 
>> >>> > >> >> > +
>> >>> > >> >> >  9 files changed, 1959 insertions(+)
>> >>> > >> >> >  create mode 100644 
>> >>> > >> >> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >>> > >> >> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >>> > >> >> >
>> >> > >> >> 
>> >> > >> >> [snip]
>> >> > >> >> 
>> >>> > >> >> > +#define DBC_VENDOR_ID 0x1d6b  /* 
>> >>> > >> >> > Linux Foundation 0x1d6b */
>> >>> > >> >> > +#define DBC_PRODUCT_ID0x0004  /* 
>> >>> > >> >> > device 0004 */
>> >>> > >> >> >
>> >> > >> >> 
>> >> > >> >> The DbC (xHCI DeBug Capability) is an optional functionality 
>> >> > >> >> in
>> >> > >> >> some xHCI host controllers. It will present a super-speed 
>> >> > >> >> debug
>> >> > >> >> device through the debug port after it is enabled.
>> >> > >> >> 
>> >> > >> >> The DbC register set defines an interface for system software
>> >> > >> >> to specify the vendor id and product id of the debug device.
>> >> > >> >> These two values will be presented by the debug device in its
>> >> > >> >> device descriptor idVendor and idProduct fields.
>> >> > >> >> 
>> >> > >> >> Microsoft Windows have a well established protocol for
>> >> > >> >> debugging over DbC. And it assigns 

Re: [PATCH v2] USB: add SPDX identifiers to all remaining files in drivers/usb/

[Felipe Balbi]

Greg Kroah-Hartman <gre...@linuxfoundation.org> writes:

> It's good to have SPDX identifiers in all files to make it easier to
> audit the kernel tree for correct licenses.
>
> Update the drivers/usb/ and include/linux/usb* files with the correct
> SPDX license identifier based on the license text in the file itself.
> The SPDX identifier is a legally binding shorthand, which can be used
> instead of the full boiler plate text.
>
> This work is based on a script and data from Thomas Gleixner, Philippe
> Ombredanne, and Kate Stewart.
>
> Cc: Thomas Gleixner <t...@linutronix.de>
> Cc: Kate Stewart <kstew...@linuxfoundation.org>
> Cc: Philippe Ombredanne <pombreda...@nexb.com>
> Signed-off-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] USB: add SPDX identifiers to all remaining files in drivers/usb/

[Felipe Balbi]

Greg Kroah-Hartman  writes:

> It's good to have SPDX identifiers in all files to make it easier to
> audit the kernel tree for correct licenses.
>
> Update the drivers/usb/ and include/linux/usb* files with the correct
> SPDX license identifier based on the license text in the file itself.
> The SPDX identifier is a legally binding shorthand, which can be used
> instead of the full boiler plate text.
>
> This work is based on a script and data from Thomas Gleixner, Philippe
> Ombredanne, and Kate Stewart.
>
> Cc: Thomas Gleixner 
> Cc: Kate Stewart 
> Cc: Philippe Ombredanne 
> Signed-off-by: Greg Kroah-Hartman 

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: mtu3: fix dma_addr_t printk output again

[Felipe Balbi]

Hi,

Arnd Bergmann <a...@arndb.de> writes:
> The support for 36-bit addresses originally came with an incorrect
> printk format for dma addresses. Felipe changed the format string it
> while applying, but the result was still incorrect, since we now have
> to pass a pointer to the address instead of the integer value:
>
> drivers/usb/mtu3/mtu3_qmu.c: In function 'mtu3_prepare_tx_gpd':
> drivers/usb/mtu3/mtu3_qmu.c:261:25: error: format '%p' expects argument of 
> type 'void *', but argument 7 has type 'dma_addr_t {aka unsigned int}' 
> [-Werror=format=]
> drivers/usb/mtu3/mtu3_qmu.c: In function 'mtu3_prepare_rx_gpd':
> drivers/usb/mtu3/mtu3_qmu.c:300:25: error: format '%p' expects argument of 
> type 'void *', but argument 7 has type 'dma_addr_t {aka unsigned int}' 
> [-Werror=format=]
>
> This fixes the printk argument accordingly.
>
> Fixes: 1a46dfea0841 ("usb: mtu3: support 36-bit DMA address")
> Signed-off-by: Arnd Bergmann <a...@arndb.de>

oh, so it wants a pointer afterall :-)

sorry.

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi

Re: [PATCH] usb: mtu3: fix dma_addr_t printk output again

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> The support for 36-bit addresses originally came with an incorrect
> printk format for dma addresses. Felipe changed the format string it
> while applying, but the result was still incorrect, since we now have
> to pass a pointer to the address instead of the integer value:
>
> drivers/usb/mtu3/mtu3_qmu.c: In function 'mtu3_prepare_tx_gpd':
> drivers/usb/mtu3/mtu3_qmu.c:261:25: error: format '%p' expects argument of 
> type 'void *', but argument 7 has type 'dma_addr_t {aka unsigned int}' 
> [-Werror=format=]
> drivers/usb/mtu3/mtu3_qmu.c: In function 'mtu3_prepare_rx_gpd':
> drivers/usb/mtu3/mtu3_qmu.c:300:25: error: format '%p' expects argument of 
> type 'void *', but argument 7 has type 'dma_addr_t {aka unsigned int}' 
> [-Werror=format=]
>
> This fixes the printk argument accordingly.
>
> Fixes: 1a46dfea0841 ("usb: mtu3: support 36-bit DMA address")
> Signed-off-by: Arnd Bergmann 

oh, so it wants a pointer afterall :-)

sorry.

Acked-by: Felipe Balbi 

-- 
balbi

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>> Greg Kroah-Hartman  writes:
>> >> > xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> >> > can be implemented with the Debug Capability(DbC). It presents a debug
>> >> > device which is fully compliant with the USB framework and provides the
>> >> > equivalent of a very high performance full-duplex serial link. The debug
>> >> > capability operation model and registers interface are defined in 7.6.8
>> >> > of the xHCI specification, revision 1.1.
>> >> >
>> >> > The DbC debug device shares a root port with the xHCI host. By default,
>> >> > the debug capability is disabled and the root port is assigned to xHCI.
>> >> > When the DbC is enabled, the root port will be assigned to the DbC debug
>> >> > device, and the xHCI sees nothing on this port. This implementation uses
>> >> > a sysfs node named  under the xHCI device to manage the enabling
>> >> > and disabling of the debug capability.
>> >> >
>> >> > When the debug capability is enabled, it will present a debug device
>> >> > through the debug port. This debug device is fully compliant with the
>> >> > USB3 framework, and it can be enumerated by a debug host on the other
>> >> > end of the USB link. As soon as the debug device is configured, a TTY
>> >> > serial device named /dev/ttyDBC0 will be created.
>> >> >
>> >> > One use of this link is running a login service on the debug target.
>> >> > Hence it can be remote accessed by a debug host. Another use case can
>> >> > probably be found in servers. It provides a peer-to-peer USB link
>> >> > between two host-only machines. This provides a reasonable out-of-band
>> >> > communication method between two servers.
>> >> >
>> >> > Signed-off-by: Lu Baolu 
>> >> > ---
>> >> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 +
>> >> >  drivers/usb/host/Kconfig   |9 +
>> >> >  drivers/usb/host/Makefile  |5 +
>> >> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> >> > 
>> >> >  drivers/usb/host/xhci-dbgcap.h |  247 +
>> >> >  drivers/usb/host/xhci-dbgtty.c |  586 +++
>> >> >  drivers/usb/host/xhci-trace.h  |   60 ++
>> >> >  drivers/usb/host/xhci.c|   10 +
>> >> >  drivers/usb/host/xhci.h|1 +
>> >> >  9 files changed, 1959 insertions(+)
>> >> >  create mode 100644 
>> >> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >> >
>> >> 
>> >> [snip]
>> >> 
>> >> > +#define DBC_VENDOR_ID  0x1d6b  /* Linux Foundation 
>> >> > 0x1d6b */
>> >> > +#define DBC_PRODUCT_ID 0x0004  /* device 0004 */
>> >> >
>> >> 
>> >> The DbC (xHCI DeBug Capability) is an optional functionality in
>> >> some xHCI host controllers. It will present a super-speed debug
>> >> device through the debug port after it is enabled.
>> >> 
>> >> The DbC register set defines an interface for system software
>> >> to specify the vendor id and product id of the debug device.
>> >> These two values will be presented by the debug device in its
>> >> device descriptor idVendor and idProduct fields.
>> >> 
>> >> Microsoft Windows have a well established protocol for
>> >> debugging over DbC. And it assigns below values for its use.
>> >> 
>> >> USB\VID_045E_062D.DeviceDesc="Microsoft USB Debug Target"
>> >> 
>> >> I'm going to use 0x1d6b/0x0004 value pair for DbC use in
>> >> Linux. Do you approve me to do so?
>> >
>> > No.  Why can't you use the same ids as Windows?  This is implementing
>> > the same protocol, right?
>> 
>> the protocol running on top is 100% vendor specific. More than likely,
>> we would just run kgdb on top of this, right? We really don't support
>> microsoft's debug architecture.
>
> Ah, I didn't know about the protocol specifics here, if it is
> vendor-specific, then yes, we need our own id.

Great, thanks :-)

Let us know which one we're allowed to use and I'm sure Baolu can respin
the patch in no time.

> As the above text said "well established protocol", I assumed we
> implemented the same thing :)

It's "well established" from Microsoft's point of view :-) They have
that same protocol running over USB, TCP, UDP...

It would be nice if we could ditch TTY and teach gdb about other
transports, but then again, why bother if we can reuse code that already
works? :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>> Greg Kroah-Hartman  writes:
>> >> > xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> >> > can be implemented with the Debug Capability(DbC). It presents a debug
>> >> > device which is fully compliant with the USB framework and provides the
>> >> > equivalent of a very high performance full-duplex serial link. The debug
>> >> > capability operation model and registers interface are defined in 7.6.8
>> >> > of the xHCI specification, revision 1.1.
>> >> >
>> >> > The DbC debug device shares a root port with the xHCI host. By default,
>> >> > the debug capability is disabled and the root port is assigned to xHCI.
>> >> > When the DbC is enabled, the root port will be assigned to the DbC debug
>> >> > device, and the xHCI sees nothing on this port. This implementation uses
>> >> > a sysfs node named  under the xHCI device to manage the enabling
>> >> > and disabling of the debug capability.
>> >> >
>> >> > When the debug capability is enabled, it will present a debug device
>> >> > through the debug port. This debug device is fully compliant with the
>> >> > USB3 framework, and it can be enumerated by a debug host on the other
>> >> > end of the USB link. As soon as the debug device is configured, a TTY
>> >> > serial device named /dev/ttyDBC0 will be created.
>> >> >
>> >> > One use of this link is running a login service on the debug target.
>> >> > Hence it can be remote accessed by a debug host. Another use case can
>> >> > probably be found in servers. It provides a peer-to-peer USB link
>> >> > between two host-only machines. This provides a reasonable out-of-band
>> >> > communication method between two servers.
>> >> >
>> >> > Signed-off-by: Lu Baolu 
>> >> > ---
>> >> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 +
>> >> >  drivers/usb/host/Kconfig   |9 +
>> >> >  drivers/usb/host/Makefile  |5 +
>> >> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> >> > 
>> >> >  drivers/usb/host/xhci-dbgcap.h |  247 +
>> >> >  drivers/usb/host/xhci-dbgtty.c |  586 +++
>> >> >  drivers/usb/host/xhci-trace.h  |   60 ++
>> >> >  drivers/usb/host/xhci.c|   10 +
>> >> >  drivers/usb/host/xhci.h|1 +
>> >> >  9 files changed, 1959 insertions(+)
>> >> >  create mode 100644 
>> >> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >> >
>> >> 
>> >> [snip]
>> >> 
>> >> > +#define DBC_VENDOR_ID  0x1d6b  /* Linux Foundation 
>> >> > 0x1d6b */
>> >> > +#define DBC_PRODUCT_ID 0x0004  /* device 0004 */
>> >> >
>> >> 
>> >> The DbC (xHCI DeBug Capability) is an optional functionality in
>> >> some xHCI host controllers. It will present a super-speed debug
>> >> device through the debug port after it is enabled.
>> >> 
>> >> The DbC register set defines an interface for system software
>> >> to specify the vendor id and product id of the debug device.
>> >> These two values will be presented by the debug device in its
>> >> device descriptor idVendor and idProduct fields.
>> >> 
>> >> Microsoft Windows have a well established protocol for
>> >> debugging over DbC. And it assigns below values for its use.
>> >> 
>> >> USB\VID_045E_062D.DeviceDesc="Microsoft USB Debug Target"
>> >> 
>> >> I'm going to use 0x1d6b/0x0004 value pair for DbC use in
>> >> Linux. Do you approve me to do so?
>> >
>> > No.  Why can't you use the same ids as Windows?  This is implementing
>> > the same protocol, right?
>> 
>> the protocol running on top is 100% vendor specific. More than likely,
>> we would just run kgdb on top of this, right? We really don't support
>> microsoft's debug architecture.
>
> Ah, I didn't know about the protocol specifics here, if it is
> vendor-specific, then yes, we need our own id.

Great, thanks :-)

Let us know which one we're allowed to use and I'm sure Baolu can respin
the patch in no time.

> As the above text said "well established protocol", I assumed we
> implemented the same thing :)

It's "well established" from Microsoft's point of view :-) They have
that same protocol running over USB, TCP, UDP...

It would be nice if we could ditch TTY and teach gdb about other
transports, but then again, why bother if we can reuse code that already
works? :-)

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>> > xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> > can be implemented with the Debug Capability(DbC). It presents a debug
>> > device which is fully compliant with the USB framework and provides the
>> > equivalent of a very high performance full-duplex serial link. The debug
>> > capability operation model and registers interface are defined in 7.6.8
>> > of the xHCI specification, revision 1.1.
>> >
>> > The DbC debug device shares a root port with the xHCI host. By default,
>> > the debug capability is disabled and the root port is assigned to xHCI.
>> > When the DbC is enabled, the root port will be assigned to the DbC debug
>> > device, and the xHCI sees nothing on this port. This implementation uses
>> > a sysfs node named  under the xHCI device to manage the enabling
>> > and disabling of the debug capability.
>> >
>> > When the debug capability is enabled, it will present a debug device
>> > through the debug port. This debug device is fully compliant with the
>> > USB3 framework, and it can be enumerated by a debug host on the other
>> > end of the USB link. As soon as the debug device is configured, a TTY
>> > serial device named /dev/ttyDBC0 will be created.
>> >
>> > One use of this link is running a login service on the debug target.
>> > Hence it can be remote accessed by a debug host. Another use case can
>> > probably be found in servers. It provides a peer-to-peer USB link
>> > between two host-only machines. This provides a reasonable out-of-band
>> > communication method between two servers.
>> >
>> > Signed-off-by: Lu Baolu 
>> > ---
>> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 +
>> >  drivers/usb/host/Kconfig   |9 +
>> >  drivers/usb/host/Makefile  |5 +
>> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> > 
>> >  drivers/usb/host/xhci-dbgcap.h |  247 +
>> >  drivers/usb/host/xhci-dbgtty.c |  586 +++
>> >  drivers/usb/host/xhci-trace.h  |   60 ++
>> >  drivers/usb/host/xhci.c|   10 +
>> >  drivers/usb/host/xhci.h|1 +
>> >  9 files changed, 1959 insertions(+)
>> >  create mode 100644 
>> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >
>> 
>> [snip]
>> 
>> > +#define DBC_VENDOR_ID 0x1d6b  /* Linux Foundation 
>> > 0x1d6b */
>> > +#define DBC_PRODUCT_ID0x0004  /* device 0004 */
>> >
>> 
>> The DbC (xHCI DeBug Capability) is an optional functionality in
>> some xHCI host controllers. It will present a super-speed debug
>> device through the debug port after it is enabled.
>> 
>> The DbC register set defines an interface for system software
>> to specify the vendor id and product id of the debug device.
>> These two values will be presented by the debug device in its
>> device descriptor idVendor and idProduct fields.
>> 
>> Microsoft Windows have a well established protocol for
>> debugging over DbC. And it assigns below values for its use.
>> 
>> USB\VID_045E_062D.DeviceDesc="Microsoft USB Debug Target"
>> 
>> I'm going to use 0x1d6b/0x0004 value pair for DbC use in
>> Linux. Do you approve me to do so?
>
> No.  Why can't you use the same ids as Windows?  This is implementing
> the same protocol, right?

the protocol running on top is 100% vendor specific. More than likely,
we would just run kgdb on top of this, right? We really don't support
microsoft's debug architecture.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v3 2/3] usb: xhci: Add DbC support in xHCI driver

[Felipe Balbi]

Hi,

Greg Kroah-Hartman  writes:
>> > xHCI compatible USB host controllers(i.e. super-speed USB3 controllers)
>> > can be implemented with the Debug Capability(DbC). It presents a debug
>> > device which is fully compliant with the USB framework and provides the
>> > equivalent of a very high performance full-duplex serial link. The debug
>> > capability operation model and registers interface are defined in 7.6.8
>> > of the xHCI specification, revision 1.1.
>> >
>> > The DbC debug device shares a root port with the xHCI host. By default,
>> > the debug capability is disabled and the root port is assigned to xHCI.
>> > When the DbC is enabled, the root port will be assigned to the DbC debug
>> > device, and the xHCI sees nothing on this port. This implementation uses
>> > a sysfs node named  under the xHCI device to manage the enabling
>> > and disabling of the debug capability.
>> >
>> > When the debug capability is enabled, it will present a debug device
>> > through the debug port. This debug device is fully compliant with the
>> > USB3 framework, and it can be enumerated by a debug host on the other
>> > end of the USB link. As soon as the debug device is configured, a TTY
>> > serial device named /dev/ttyDBC0 will be created.
>> >
>> > One use of this link is running a login service on the debug target.
>> > Hence it can be remote accessed by a debug host. Another use case can
>> > probably be found in servers. It provides a peer-to-peer USB link
>> > between two host-only machines. This provides a reasonable out-of-band
>> > communication method between two servers.
>> >
>> > Signed-off-by: Lu Baolu 
>> > ---
>> >  .../ABI/testing/sysfs-bus-pci-drivers-xhci_hcd |   25 +
>> >  drivers/usb/host/Kconfig   |9 +
>> >  drivers/usb/host/Makefile  |5 +
>> >  drivers/usb/host/xhci-dbgcap.c | 1016 
>> > 
>> >  drivers/usb/host/xhci-dbgcap.h |  247 +
>> >  drivers/usb/host/xhci-dbgtty.c |  586 +++
>> >  drivers/usb/host/xhci-trace.h  |   60 ++
>> >  drivers/usb/host/xhci.c|   10 +
>> >  drivers/usb/host/xhci.h|1 +
>> >  9 files changed, 1959 insertions(+)
>> >  create mode 100644 
>> > Documentation/ABI/testing/sysfs-bus-pci-drivers-xhci_hcd
>> >  create mode 100644 drivers/usb/host/xhci-dbgcap.c
>> >  create mode 100644 drivers/usb/host/xhci-dbgcap.h
>> >  create mode 100644 drivers/usb/host/xhci-dbgtty.c
>> >
>> 
>> [snip]
>> 
>> > +#define DBC_VENDOR_ID 0x1d6b  /* Linux Foundation 
>> > 0x1d6b */
>> > +#define DBC_PRODUCT_ID0x0004  /* device 0004 */
>> >
>> 
>> The DbC (xHCI DeBug Capability) is an optional functionality in
>> some xHCI host controllers. It will present a super-speed debug
>> device through the debug port after it is enabled.
>> 
>> The DbC register set defines an interface for system software
>> to specify the vendor id and product id of the debug device.
>> These two values will be presented by the debug device in its
>> device descriptor idVendor and idProduct fields.
>> 
>> Microsoft Windows have a well established protocol for
>> debugging over DbC. And it assigns below values for its use.
>> 
>> USB\VID_045E_062D.DeviceDesc="Microsoft USB Debug Target"
>> 
>> I'm going to use 0x1d6b/0x0004 value pair for DbC use in
>> Linux. Do you approve me to do so?
>
> No.  Why can't you use the same ids as Windows?  This is implementing
> the same protocol, right?

the protocol running on top is 100% vendor specific. More than likely,
we would just run kgdb on top of this, right? We really don't support
microsoft's debug architecture.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: gadget: Fix .udc_set_speed()

[Felipe Balbi]

Roger Quadros  writes:

> UDC core calls .udc_set_speed() with the speed parameter
> containing the maximum speed supported by the gadget function
> driver. This might very well be more or less than that
> supported by the dwc3 controller driver.
>
> Select the lesser of the 2 speeds so both are operating
> within limits.
>
> This fixes PHY Erratic errors and 2 second enumeration delay on
> TI's AM437x platforms.
>
> Fixes: 7d8d0639565f ("usb: dwc3: gadget: implement ->udc_set_speed()")
> Cc:  # v4.13+
> Reported-by: Dylan Howey 
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f064f15..9f27ec0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2008,6 +2008,8 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>   unsigned long   flags;
>   u32 reg;
>  
> + speed = min(g->max_speed, speed);

should we do that in udc core itself?

@@ -1080,8 +1080,12 @@ static inline void usb_gadget_udc_stop(struct usb_udc 
*udc)
 static inline void usb_gadget_udc_set_speed(struct usb_udc *udc,
enum usb_device_speed speed)
 {
-   if (udc->gadget->ops->udc_set_speed)
-   udc->gadget->ops->udc_set_speed(udc->gadget, speed);
+   if (udc->gadget->ops->udc_set_speed) {
+   enum usb_device_speed s;
+
+   s = min(speed, udc->gadget->max_speed);
+   udc->gadget->ops->udc_set_speed(udc->gadget, s);
+   }
 }
 
 /**

then the fix applies for all UDCs.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: gadget: Fix .udc_set_speed()

[Felipe Balbi]

Roger Quadros  writes:

> UDC core calls .udc_set_speed() with the speed parameter
> containing the maximum speed supported by the gadget function
> driver. This might very well be more or less than that
> supported by the dwc3 controller driver.
>
> Select the lesser of the 2 speeds so both are operating
> within limits.
>
> This fixes PHY Erratic errors and 2 second enumeration delay on
> TI's AM437x platforms.
>
> Fixes: 7d8d0639565f ("usb: dwc3: gadget: implement ->udc_set_speed()")
> Cc:  # v4.13+
> Reported-by: Dylan Howey 
> Signed-off-by: Roger Quadros 
> ---
>  drivers/usb/dwc3/gadget.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index f064f15..9f27ec0 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -2008,6 +2008,8 @@ static void dwc3_gadget_set_speed(struct usb_gadget *g,
>   unsigned long   flags;
>   u32 reg;
>  
> + speed = min(g->max_speed, speed);

should we do that in udc core itself?

@@ -1080,8 +1080,12 @@ static inline void usb_gadget_udc_stop(struct usb_udc 
*udc)
 static inline void usb_gadget_udc_set_speed(struct usb_udc *udc,
enum usb_device_speed speed)
 {
-   if (udc->gadget->ops->udc_set_speed)
-   udc->gadget->ops->udc_set_speed(udc->gadget, speed);
+   if (udc->gadget->ops->udc_set_speed) {
+   enum usb_device_speed s;
+
+   s = min(speed, udc->gadget->max_speed);
+   udc->gadget->ops->udc_set_speed(udc->gadget, s);
+   }
 }
 
 /**

then the fix applies for all UDCs.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Doug Anderson <diand...@chromium.org> writes:

> Hi,
>
> On Mon, Oct 30, 2017 at 1:32 AM, Felipe Balbi <ba...@kernel.org> wrote:
>>
>> Hi,
>>
>> Doug Anderson <diand...@chromium.org> writes:
>>> Hi,
>>>
>>> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren <stefan.wah...@i2se.com> 
>>> wrote:
>>>> Hi Doug,
>>>>
>>>> [add Felipe since this should go through his tree]
>>>
>>> Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past
>>
>> No problems :-)
>>
>>> but for some reason get_maintainer didn't ID you so I thought maybe
>>> you weren't doing it anymore.  Please let me know if you'd like me to
>>> send this to you again with collected Reviewed-by and Tested-by tags.
>>
>> Yeah, please resend with all tags collected, however let's wait for a
>> week or so and give other people time to catch up. I already sent my
>> pull request to Greg, this would, anyway, go into the -rc cycle.
>
> Doh!  I just re-read this one more time (after sending v3) and
> realized I had read it incorrectly.  I read it as "please send the

that's okay :-) s**t happens

> patch with the tags and I'll wait a week before landing", but you
> actually said "please wait a week before re-sending".  Sorry for the
> noise.  In the very least, you should be on the "To" line now so if
> anyone else has any extra tags it should be very easy for you to see
> them.
>
> Right that there's no massive urgency.  It's been broken forever.

alright then :-)

>> Please add a Cc stable tag too, if necessary.
>
> Good point.  It's a little weird since it doesn't "fix" any specific
> commit, so I guess it will be up to stable folks to decide how far to
> go back.  The dwc2 devices I work with are actually on 3.14, but we
> have some pretty massive backports related to dwc2 there...

fair enough

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Doug Anderson  writes:

> Hi,
>
> On Mon, Oct 30, 2017 at 1:32 AM, Felipe Balbi  wrote:
>>
>> Hi,
>>
>> Doug Anderson  writes:
>>> Hi,
>>>
>>> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren  
>>> wrote:
>>>> Hi Doug,
>>>>
>>>> [add Felipe since this should go through his tree]
>>>
>>> Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past
>>
>> No problems :-)
>>
>>> but for some reason get_maintainer didn't ID you so I thought maybe
>>> you weren't doing it anymore.  Please let me know if you'd like me to
>>> send this to you again with collected Reviewed-by and Tested-by tags.
>>
>> Yeah, please resend with all tags collected, however let's wait for a
>> week or so and give other people time to catch up. I already sent my
>> pull request to Greg, this would, anyway, go into the -rc cycle.
>
> Doh!  I just re-read this one more time (after sending v3) and
> realized I had read it incorrectly.  I read it as "please send the

that's okay :-) s**t happens

> patch with the tags and I'll wait a week before landing", but you
> actually said "please wait a week before re-sending".  Sorry for the
> noise.  In the very least, you should be on the "To" line now so if
> anyone else has any extra tags it should be very easy for you to see
> them.
>
> Right that there's no massive urgency.  It's been broken forever.

alright then :-)

>> Please add a Cc stable tag too, if necessary.
>
> Good point.  It's a little weird since it doesn't "fix" any specific
> commit, so I guess it will be up to stable folks to decide how far to
> go back.  The dwc2 devices I work with are actually on 3.14, but we
> have some pretty massive backports related to dwc2 there...

fair enough

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Hi,

Doug Anderson  writes:
> Hi,
>
> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren  wrote:
>> Hi Doug,
>>
>> [add Felipe since this should go through his tree]
>
> Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past

No problems :-)

> but for some reason get_maintainer didn't ID you so I thought maybe
> you weren't doing it anymore.  Please let me know if you'd like me to
> send this to you again with collected Reviewed-by and Tested-by tags.

Yeah, please resend with all tags collected, however let's wait for a
week or so and give other people time to catch up. I already sent my
pull request to Greg, this would, anyway, go into the -rc cycle.

Please add a Cc stable tag too, if necessary.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH v2] usb: dwc2: host: Don't retry NAKed transactions right away

[Felipe Balbi]

Hi,

Doug Anderson  writes:
> Hi,
>
> On Sat, Oct 28, 2017 at 8:51 AM, Stefan Wahren  wrote:
>> Hi Doug,
>>
>> [add Felipe since this should go through his tree]
>
> Ah.  Sorry Felipe!  I know you've landed some dwc2 stuff in the past

No problems :-)

> but for some reason get_maintainer didn't ID you so I thought maybe
> you weren't doing it anymore.  Please let me know if you'd like me to
> send this to you again with collected Reviewed-by and Tested-by tags.

Yeah, please resend with all tags collected, however let's wait for a
week or so and give other people time to catch up. I already sent my
pull request to Greg, this would, anyway, go into the -rc cycle.

Please add a Cc stable tag too, if necessary.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: composite: mark expected switch fall-throughs

[Felipe Balbi]

"Gustavo A. R. Silva" <garsi...@embeddedor.com> writes:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: composite: mark expected switch fall-throughs

[Felipe Balbi]

"Gustavo A. R. Silva"  writes:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva 

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: udc: dummy_hcd: mark expected switch fall-throughs

[Felipe Balbi]

"Gustavo A. R. Silva" <garsi...@embeddedor.com> writes:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: udc: dummy_hcd: mark expected switch fall-throughs

[Felipe Balbi]

"Gustavo A. R. Silva"  writes:

> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Signed-off-by: Gustavo A. R. Silva 

Acked-by: Felipe Balbi 

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/3] usb: phy: remove phy-qcom-8x16-usb.c

[Felipe Balbi]

Alex Elder  writes:

> No Qualcomm SoC requires the "phy-qcom-8x16-usb.c" USB phy driver
> support any more, so remove the code.
>
> Suggested-by: Stephen Boyd 
> Signed-off-by: Alex Elder 
> Acked-by: Bjorn Andersson 
> Acked-by: Andy Gross 

Kconfig?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH 3/3] usb: phy: remove phy-qcom-8x16-usb.c

[Felipe Balbi]

Alex Elder  writes:

> No Qualcomm SoC requires the "phy-qcom-8x16-usb.c" USB phy driver
> support any more, so remove the code.
>
> Suggested-by: Stephen Boyd 
> Signed-off-by: Alex Elder 
> Acked-by: Bjorn Andersson 
> Acked-by: Andy Gross 

Kconfig?

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: gadget: f_phonet: mark expected switch fall-throughs

[Felipe Balbi]

Hi,

"Gustavo A. R. Silva" <garsi...@embeddedor.com> writes:
> In preparation to enabling -Wimplicit-fallthrough, mark switch cases
> where we are expecting to fall through.
>
> Addresses-Coverity-ID: 115004
> Addresses-Coverity-ID: 115005
> Signed-off-by: Gustavo A. R. Silva <garsi...@embeddedor.com>

Greg, I have already sent you my pull request. If you want, I can
prepare a part 2, otherwise:

Acked-by: Felipe Balbi <felipe.ba...@linux.intel.com>

-- 
balbi


signature.asc
Description: PGP signature