Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Lu Baolu]

Hi,

On 04/27/2016 08:33 PM, Mark Brown wrote:
> On Wed, Apr 27, 2016 at 09:54:10AM +0800, Lu Baolu wrote:
>
>> Please refer to Documentation/acpi/gpio-properties.txt.
> That's not visibly what your driver is doing, that is also recommending
> using a static name which is what I'm asking for.

Yes, I agree that we should use a static name.

>
>>> Why does the device care?It's requesting the GPIO in
>>> its own context and it's only requesting one GPIO, with DT we're just
>>> always calling the GPIO "gpio" which works fine.
>> This driver is not bound to an ACPI device node directly. It's a child
>> of a mfd device, which is corresponding to a real ACPI device node.
> If it's the child of a MFD it's got an ACPI device, the ACPI device is
> the parent.It should use the parent device or the parent should map
> the GPIO through to the child as many other MFDs do, the whole concept
> of a MFD is a Linux internal implementation detail.

Yes. The mapping of GPIO is done in the parent. And the parent
passes the GPIO by setting ACPI companion to this device (done
in mfd internal). This driver is able to get the gpio descriptor with
a static name.

How about below code?

+   gpiod = gpiod_get(dev, "vbus_en", GPIOD_ASIS);
+   if (IS_ERR(gpiod))
+   return PTR_ERR(gpiod);
+
+   config->gpio = desc_to_gpio(gpiod);
+   config->enable_high = device_property_read_bool(dev,
+   "enable-active-high");
+   gpiod_put(gpiod);

Best regards,
Lu Baolu

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

Re: [PATCH v2 2/2] phy: Group vendor specific phy drivers

[Vivek Gautam]

Hi Kishon,


On Thu, Apr 28, 2016 at 10:28 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Tuesday 26 April 2016 11:36 AM, Vivek Gautam wrote:
>> Hi Kishon,
>>
>>
>> On Wed, Apr 6, 2016 at 7:37 PM, Vivek Gautam  
>> wrote:
>>> Adding vendor specific directories in phy to group
>>> phy drivers under their respective vendor umbrella.
>>>
>>> Also updated the MAINTAINERS file to reflect the correct
>>> directory structure for phy drivers.
>>>
>>> Signed-off-by: Vivek Gautam 
>>> Acked-by: Heiko Stuebner 
>>> Acked-by: Viresh Kumar 
>>> ---
>>>
>>> Changes from v1:
>>>  - Updated the MAINTAINERS file to reflect the same change
>>>in directory structure.
>>>  - Removed PHY_PLAT config as pointed out by Kishon.
>>>So we don't require the second patch in the v1 of this series:
>>>[PATCH 2/2] arm: mach-spear: Enable PHY_PLAT to meet dependency
>>>  - Renamed sunxi --> allwinner; rcar --> renesas.
>>>  - Fixed error coming with qcom Makefile.
>>
>> Does this change looks okay now ?
>> I think you must be planning to take this in after 4.7 rc1 is out, isn't it ?
>
> I'm not sure. I'm thinking which should be the right way to group the phy
> drivers, by vendors or by type (usb, pcie, etc..). I'm more inclined to group
> it by type.

I guess there are atleast couple of phy drivers (PIPE and ST's MiPHY) that
have multiple roles - USB or PCIe or SATA.

So grouping such drivers in one of the phy-types will be tricky, WDUT

>
> Thanks
> Kishon



-- 
Best Regards
Vivek Gautam
Samsung R Institute, Bangalore
India
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] phy: Group vendor specific phy drivers

[Kishon Vijay Abraham I]

Hi,

On Tuesday 26 April 2016 11:36 AM, Vivek Gautam wrote:
> Hi Kishon,
> 
> 
> On Wed, Apr 6, 2016 at 7:37 PM, Vivek Gautam  wrote:
>> Adding vendor specific directories in phy to group
>> phy drivers under their respective vendor umbrella.
>>
>> Also updated the MAINTAINERS file to reflect the correct
>> directory structure for phy drivers.
>>
>> Signed-off-by: Vivek Gautam 
>> Acked-by: Heiko Stuebner 
>> Acked-by: Viresh Kumar 
>> ---
>>
>> Changes from v1:
>>  - Updated the MAINTAINERS file to reflect the same change
>>in directory structure.
>>  - Removed PHY_PLAT config as pointed out by Kishon.
>>So we don't require the second patch in the v1 of this series:
>>[PATCH 2/2] arm: mach-spear: Enable PHY_PLAT to meet dependency
>>  - Renamed sunxi --> allwinner; rcar --> renesas.
>>  - Fixed error coming with qcom Makefile.
> 
> Does this change looks okay now ?
> I think you must be planning to take this in after 4.7 rc1 is out, isn't it ?

I'm not sure. I'm thinking which should be the right way to group the phy
drivers, by vendors or by type (usb, pcie, etc..). I'm more inclined to group
it by type.

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

[PATCH v2 1/2] usb: core: buffer: avoid NULL pointer dereferrence

[Chunfeng Yun]

NULL pointer dereferrence will happen when class driver
wants to allocate zero length buffer and pool_max[0]
can't be used, so simply returns NULL in the case.

Signed-off-by: Chunfeng Yun 
---
 drivers/usb/core/buffer.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/core/buffer.c b/drivers/usb/core/buffer.c
index 2741566..98e39f9 100644
--- a/drivers/usb/core/buffer.c
+++ b/drivers/usb/core/buffer.c
@@ -122,6 +122,9 @@ void *hcd_buffer_alloc(
struct usb_hcd  *hcd = bus_to_hcd(bus);
int i;
 
+   if (size == 0)
+   return NULL;
+
/* some USB hosts just use PIO */
if (!IS_ENABLED(CONFIG_HAS_DMA) ||
(!bus->controller->dma_mask &&
-- 
1.7.9.5

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

[PATCH v2 2/2] usb: misc: usbtest: fix error of urb allocation

[Chunfeng Yun]

urb allocation will fail when usbtest_alloc_urb() tries to
allocate zero length buffer, but it doesn't need it in fact,
so just skips buffer allocation in the case.

Signed-off-by: Chunfeng Yun 
---
 drivers/usb/misc/usbtest.c |3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/misc/usbtest.c b/drivers/usb/misc/usbtest.c
index 92fdb6e..de485d8 100644
--- a/drivers/usb/misc/usbtest.c
+++ b/drivers/usb/misc/usbtest.c
@@ -287,6 +287,9 @@ static struct urb *usbtest_alloc_urb(
if (usb_pipein(pipe))
urb->transfer_flags |= URB_SHORT_NOT_OK;
 
+   if ((bytes + offset) == 0)
+   return urb;
+
if (urb->transfer_flags & URB_NO_TRANSFER_DMA_MAP)
urb->transfer_buffer = usb_alloc_coherent(udev, bytes + offset,
GFP_KERNEL, >transfer_dma);
-- 
1.7.9.5

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

[PATCH 5/5] usb: dwc2: Proper cleanup on dr_mode failure

[John Youn]

Cleanup in probe if we fail to get dr_mode.

Signed-off-by: John Youn 
---
 drivers/usb/dwc2/platform.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/platform.c b/drivers/usb/dwc2/platform.c
index 88629be..fc6f525 100644
--- a/drivers/usb/dwc2/platform.c
+++ b/drivers/usb/dwc2/platform.c
@@ -562,7 +562,7 @@ static int dwc2_driver_probe(struct platform_device *dev)
 
retval = dwc2_get_dr_mode(hsotg);
if (retval)
-   return retval;
+   goto error;
 
/*
 * Reset before dwc2_get_hwparams() then it could get power-on real
-- 
2.7.4

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

[PATCH 3/5] usb: dwc2: host: Setting qtd to NULL after freeing it

[John Youn]

From: Vardan Mikayelyan 

This is safety change added while doing slub debugging.

Affected functions:
dwc2_hcd_qtd_unlink_and_free()
_dwc2_hcd_urb_enqueue()

Signed-off-by: Vardan Mikayelyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/hcd.c | 1 +
 drivers/usb/dwc2/hcd.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/usb/dwc2/hcd.c b/drivers/usb/dwc2/hcd.c
index 1f62551..2df3d04 100644
--- a/drivers/usb/dwc2/hcd.c
+++ b/drivers/usb/dwc2/hcd.c
@@ -4703,6 +4703,7 @@ fail2:
spin_unlock_irqrestore(>lock, flags);
urb->hcpriv = NULL;
kfree(qtd);
+   qtd = NULL;
 fail1:
if (qh_allocated) {
struct dwc2_qtd *qtd2, *qtd2_tmp;
diff --git a/drivers/usb/dwc2/hcd.h b/drivers/usb/dwc2/hcd.h
index 89fa26c..7758bfb 100644
--- a/drivers/usb/dwc2/hcd.h
+++ b/drivers/usb/dwc2/hcd.h
@@ -552,6 +552,7 @@ static inline void dwc2_hcd_qtd_unlink_and_free(struct 
dwc2_hsotg *hsotg,
 {
list_del(>qtd_list_entry);
kfree(qtd);
+   qtd = NULL;
 }
 
 /* Descriptor DMA support functions */
-- 
2.7.4

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

[PATCH 4/5] usb: dwc2: Fixed SOF interrupt enabling/disabling

[John Youn]

From: Sevak Arakelyan 

In case of DDMA mode we don't need to get an SOF interrupt so disable
the unmasking of SOF interrupt in DDMA mode.

Signed-off-by: Sevak Arakelyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/hcd_queue.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/hcd_queue.c b/drivers/usb/dwc2/hcd_queue.c
index 7f634fd..b5c7793 100644
--- a/drivers/usb/dwc2/hcd_queue.c
+++ b/drivers/usb/dwc2/hcd_queue.c
@@ -1709,7 +1709,8 @@ void dwc2_hcd_qh_unlink(struct dwc2_hsotg *hsotg, struct 
dwc2_qh *qh)
 
dwc2_deschedule_periodic(hsotg, qh);
hsotg->periodic_qh_count--;
-   if (!hsotg->periodic_qh_count) {
+   if (!hsotg->periodic_qh_count &&
+   hsotg->core_params->dma_desc_enable <= 0) {
intr_mask = dwc2_readl(hsotg->regs + GINTMSK);
intr_mask &= ~GINTSTS_SOF;
dwc2_writel(intr_mask, hsotg->regs + GINTMSK);
-- 
2.7.4

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

[PATCH 2/5] usb: dwc2: gadget: Prevent handling of host interrupts

[John Youn]

From: Vardan Mikayelyan 

In host slave mode, the core asserts the rxready, txfifoempty interrupts
that get serviced in the gadget irq handler. Prevent servicing of these
when not in the gadget mode of operation.

Signed-off-by: Vardan Mikayelyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/gadget.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index d330190..4c5e300 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2425,6 +2425,9 @@ static irqreturn_t dwc2_hsotg_irq(int irq, void *pw)
u32 gintsts;
u32 gintmsk;
 
+   if (!dwc2_is_device_mode(hsotg))
+   return IRQ_NONE;
+
spin_lock(>lock);
 irq_retry:
gintsts = dwc2_readl(hsotg->regs + GINTSTS);
-- 
2.7.4

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

[PATCH 1/5] usb: dwc2: gadget: Check for ep0 in enable

[John Youn]

From: Vahram Aharonyan 

Replaced the WARN_ON with a check and return of -EINVAL in the
dwc2_hsotg_ep_enable function if ep0 is passed in.

Signed-off-by: Vahram Aharonyan 
Signed-off-by: John Youn 
---
 drivers/usb/dwc2/gadget.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 818f158..d330190 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -2631,7 +2631,10 @@ static int dwc2_hsotg_ep_enable(struct usb_ep *ep,
desc->wMaxPacketSize, desc->bInterval);
 
/* not to be called for EP0 */
-   WARN_ON(index == 0);
+   if (index == 0) {
+   dev_err(hsotg->dev, "%s: called for EP 0\n", __func__);
+   return -EINVAL;
+   }
 
dir_in = (desc->bEndpointAddress & USB_ENDPOINT_DIR_MASK) ? 1 : 0;
if (dir_in != hs_ep->dir_in) {
-- 
2.7.4

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

[PATCH 0/5] usb: dwc2: Various fixes

[John Youn]

Hi Felipe,

These are some various patches for dwc2 for next.

Thanks,
John

John Youn (1):
  usb: dwc2: Proper cleanup on dr_mode failure

Sevak Arakelyan (1):
  usb: dwc2: Fixed SOF interrupt enabling/disabling

Vahram Aharonyan (1):
  usb: dwc2: gadget: Check for ep0 in enable

Vardan Mikayelyan (2):
  usb: dwc2: gadget: Prevent handling of host interrupts
  usb: dwc2: host: Setting qtd to NULL after freeing it

 drivers/usb/dwc2/gadget.c| 8 +++-
 drivers/usb/dwc2/hcd.c   | 1 +
 drivers/usb/dwc2/hcd.h   | 1 +
 drivers/usb/dwc2/hcd_queue.c | 3 ++-
 drivers/usb/dwc2/platform.c  | 2 +-
 5 files changed, 12 insertions(+), 3 deletions(-)

-- 
2.7.4

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

Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Peter Chen]

On Wed, Apr 27, 2016 at 01:59:44PM +0300, Roger Quadros wrote:
> Hi,
> 
> On 27/04/16 06:15, Peter Chen wrote:
> > On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
> >> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
> >>> Hi
> >>>
>  -Original Message-
>  From: Peter Chen [mailto:hzpeterc...@gmail.com]
>  Sent: Tuesday, April 26, 2016 2:28 PM
>  To: Jun Li 
>  Cc: Roger Quadros ; st...@rowland.harvard.edu;
>  ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
>  dan.j.willi...@intel.com; jun...@freescale.com;
>  mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>  abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
>  linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
>  Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> 
>  On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> > Hi
> >
> >> -Original Message-
> >> From: Peter Chen [mailto:hzpeterc...@gmail.com]
> >> Sent: Tuesday, April 26, 2016 11:47 AM
> >> To: Jun Li 
> >> Cc: Roger Quadros ; st...@rowland.harvard.edu;
> >> ba...@kernel.org; gre...@linuxfoundation.org;
> >> peter.c...@freescale.com; dan.j.willi...@intel.com;
> >> jun...@freescale.com; mathias.ny...@linux.intel.com;
> >> t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
> >> r.bald...@samsung.com; linux-usb@vger.kernel.org;
> >> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
> >> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
> >>
> >> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
>  +struct usb_otg *usb_otg_register(struct device *dev,
>  + struct usb_otg_config *config) {
>  +struct usb_otg *otg;
>  +struct otg_wait_data *wait;
>  +int ret = 0;
>  +
>  +if (!dev || !config || !config->fsm_ops)
>  +return ERR_PTR(-EINVAL);
>  +
>  +/* already in list? */
>  +mutex_lock(_list_mutex);
>  +if (usb_otg_get_data(dev)) {
>  +dev_err(dev, "otg: %s: device already in otg list\n",
>  +__func__);
>  +ret = -EINVAL;
>  +goto unlock;
>  +}
>  +
>  +/* allocate and add to list */
>  +otg = kzalloc(sizeof(*otg), GFP_KERNEL);
>  +if (!otg) {
>  +ret = -ENOMEM;
>  +goto unlock;
>  +}
>  +
>  +otg->dev = dev;
>  +otg->caps = config->otg_caps;
>  +
>  +if ((otg->caps->hnp_support || otg->caps->srp_support ||
>  + otg->caps->adp_support) && !config->otg_work)
>  +dev_info(dev, "otg: limiting to dual-role\n");
> >>>
> >>> dev_err, this should be an error.
> >>
> >> The condition may be wrong, but it is an information to show that
> >> current OTG is dual-role.
> >
> > This should not happen in any correct design, I even doubt if we
> > should try to continue by "downgrade" it to be duel role, currently
> > the only example user is dual role, so doing like this can't be tested
> > by real case, this downgrade is not so easy like we image, at least
> > for chipidea otg driver, simply replace a queue worker may not work,
> > as we have much more difference between the 2 configs.
> >
> 
>  Would you show more why chipidea can't work just replace the work item,
>  and see if anything we still can improve for this framework?
> >>>
> >>> In real OTG, we need enable AVV irq,
> >>
> >> Enable and Handling AVV is platform stuff. In this framework, we are
> >> focus on how otg device manages host and gadget together, and the state
> >> machine when the related otg event occurs.
> >>
> >>> but for duel role, nobody care/handle,
> >>> there are much more resource required for OTG: timers, hnp polling,
> >>> otg test device handling... 
> >>
> >> They are common things for fully OTG fsm, you can move them
> >> to common code (In fact, hnp polling handling is already common code).
> >>
> >>>
> >>> with current design, chipidea driver can support real OTG with its own
> >>> queue worker, or DRD with Roger's drd work item if config is correct.
> >>>
> >>> But improve something to work on a *wrong* config will make it complicated
> >>> and does not make much sense IMO.
> >>>
> >>
> >> What does above "config" you mean?
> >>
> >> If the configure is fully OTG, you can choose different state machine,
> >> eg otg_statemachine, if you find it is hard for chipidea to use this
> >> framework, just list the reason, and see if we can improve.
> >>
> > 
> > Roger, after 

我的主页在

[我的主页在]

你的朋友给你分享了一个支付宝红包口令：加群有红包赶快打开支付宝APP，输入红包口令，抢红包，手慢无。

Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

[Bin Liu]

Hi,

On Wed, Apr 27, 2016 at 02:13:56PM -0500, Bin Liu wrote:
> Hi,
> 
> On Wed, Apr 27, 2016 at 09:26:10PM +0300, Maxim Uvarov wrote:
> > 2016-04-27 18:46 GMT+03:00 Bin Liu :
> > > Hi,
> > >
> > > On Wed, Apr 27, 2016 at 09:51:58AM +0300, Max Uvarov wrote:
> > >> Fix soft lockup when resetting remote device attached
> > >> to usb host. Configuration:
> > >> pppd -> musb hub -> usb-serial -> gsm modem
> > >
> > > I have heard a few reports similar to this symptom, but never been able
> > > to reproduce it on my side.
> > >
> > 
> > Ok, I can reproduce it almost very easy.
> > 
> > >> When gsm modem resets, musb rolls in incoming rx interrupts
> > >> which does not give any time to other application as result
> > >> it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
> > >
> > > Have you looked where exact place in the interrupt routine the execution
> > > has stuck in?
> > >
> > 
> > It does not stuck. It goes to that line which print proto error over
> > and over again and
> > nothing stops that. After some time kernel reports lockup. But
> > actually it's not stuck,
> > all cpu time was eaten by executing that handlers.
> > 
> > 
> > >> and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
> > >> for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
> > >
> > > MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures
> > > MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in
> > > musb_regs.h.
> > >
> > >>
> > >> Signed-off-by: Max Uvarov 
> > >> ---
> > >>  v2: use bitwise or for error flags before logical and. (Sergei 
> > >> Shtylyov).
> > >>
> > >>  drivers/usb/musb/musb_host.c | 12 +---
> > >>  1 file changed, 5 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> > >> index c3d5fc9..2d9aa78 100644
> > >> --- a/drivers/usb/musb/musb_host.c
> > >> +++ b/drivers/usb/musb/musb_host.c
> > >> @@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
> > >
> > > What kernel do you use? This line # is away off from upstream kernel.
> > >
> > 
> > I did this patch for 4.1 but 4.6 has the same problem and patch
> > cleanly applies to the latest torvalds/linux.git v4.6-rc5. This
> > interrupt handler has the same code.  And looks like on 3.14
> 
> Yeah, this code hasn't been chaned for year. But in general, it is
> prepfered to create patches on latest kernel to avoid other headache.
> 
> > everything worked. I don't have a time to diff 2 versions. Might be
> > regression.
> > 
> > 
> > >>
> > >>   /* stall; record URB status */
> > >>   status = -EPIPE;
> > >> + } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
> > >>
> > >> - } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
> > >> - dev_dbg(musb->controller, "end %d RX proto error\n", 
> > >> epnum);
> > >> -
> > >> - status = -EPROTO;
> > >> - musb_writeb(epio, MUSB_RXINTERVAL, 0);
> > >> -
> > >> - } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
> > >> + if (rx_csr & MUSB_RXCSR_H_ERROR) {
> > >> + status = -EPROTO;
> > >> + musb_writeb(epio, MUSB_RXINTERVAL, 0);
> > >> + }
> > >
> > > Please help me to understand how this change fixes the issue. I see the
> > > most effect of the change here is directly 'goto finish' so that 'done'
> > > flag is not set, then musb_advance_schedule() is not called. Is this the
> > > case or I missed other important pieces?
> > >
> > 
> > Right that is the goal. On this rxcsr_h_error kernel reschedules
> > current interrupt.  And that continues forever. For example adding
> 
> The MUSB Programming Guide says CPU should clear this MUSB_RXCSR_H_ERROR
> bit, but the current driver doesn't. I am wondering if this causes the
> controller keeps generating the same interrupt. Can you please try the
> following change instead to see if the lockup goes away?
> 
> @@ -1870,6 +1870,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)
> status = -EPROTO;
> musb_writeb(epio, MUSB_RXINTERVAL, 0);
>  
> +   rx_csr &= ~MUSB_RXCSR_H_ERROR;
> +   musb_writew(epio, MUSB_RXCSR, rx_csr);

+   goto finish;

Please also add the line above. I will spend more time to understand
what is happening...

First of all, I don't like the idea of merging the two branches, it
makes the code ugly.

Regards,
-Bin.

> +
> } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
>  
> if (USB_ENDPOINT_XFER_ISOC != qh->type) {
> 
> Regards,
> -Bin.
> 
> > msleep() can give some time for other processes. I'm not an expert in
> > this chip but I think that right solution in that case is not try to
> > reschedule and quick and allow hub to make reset and once again init
> > all devices (in my case ppp/pppd also shutdowns and then I bring
> > everything up with script.). 

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday 27 April 2016 23:05:42 Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
> >> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
> >> 
> >> > I've looked at the usb HCD code now and see this:
> >> > 
> >> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> >> > struct device *dev, const char *bus_name,
> >> > struct usb_hcd *primary_hcd)
> >> > {
> >> >   ...
> >> > hcd->self.controller = dev;
> >> > hcd->self.uses_dma = (dev->dma_mask != NULL);
> >> >   ...
> >> > }
> >> > 
> >> > What I think we need to do here is ensure that the device that gets
> >> > passed here and assigned to hcd->self.controller is the actual DMA
> >> > master device, i.e. the pci_device or platform_device that was created
> >> > from outside of the xhci stack. This is after all the pointer that
> >> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> >> > functions.
> >> 
> >> It would be better to add a new field, since self.controller is also
> >> used for lots of other purposes.  Something like hcd->self.dma_dev.
> >
> > Ok, fair enough. I only took a brief look and all uses I found were
> > either for the DMA mapping API or some printk logging.
> 
> I have a feeling you guys are not considering how the patch to implement
> this will look like.
> 
> How are you expecting dwc3 to pass a pointer to the DMA device from
> dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible 

Not any worse than it already is really. It already uses platform_data
for the exact case that is causing the problem here.

> Also, remember that the DMA device for dwc3 is not always
> dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting
> us to figure that one out ?

Do you have an example for this? The ones I found here either
create the dwc3 device from PCI or from a platform glue driver.

> I still think dma_inherit() (or something along those lines) is
> necessary. Specially when you consider that, as I said previously,
> that's pretty much what of_dma_configure() does.

As I said, this is not an API that can work in general, because
it makes the assumption that everything related to DMA in a
device can be set in that device itself.

The simplest case where this does not work is a PCI device behind
an IOMMU: when you call dma_map_single() or dma_alloc_coherent(),
the dma_map_ops implementation for the IOMMU has to look at the
PCI device to find out the association with an IOMMU context,
and on most architectures you cannot bind an IOMMU context to
a platform device at all.

> Anyway, I'd really like to see a patch implementing this
> hcd->self.dma_dev logic. Consider all the duplication with this
> approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its
> own. Will that be passed to dwc3 as platform_data from glue layer ? What
> about platforms which don't even use a glue layer ?

Let's separate the three problems here.

a) dwc3 creating a "xhci-hcd" platform_device that is not connected
   to any proper bus. We can work around that by adding the "self.dma_dev"
   pointer and pass that in platform_data. This is really easy, it's
   actually less code (and less iffy) than the current implementation of
   copying the parent dma mask.
   In the long run, this could be solved by doing away with the extra
   platform_device, by calling a variant of xhci_probe() from
   xhci_plat_probe() like we do for the normal *HCI drivers.

b) dwc3-pci creating a "dwc3" platform_device under the covers. This
   is pretty much the exact same problem as a) on another layer. In
   the short run, we can pass the device pointer as part of
   struct dwc3_platform_data (dwc3-pci is the only such user anway),
   and in the long run, it should be easy enough to get rid of the
   extra platform device by just calling a variant of dwc3_probe,
   which will again simplify the driver

c) some SoCs that have two separate device nodes to describe their
   dwc3 xhci. I don't think this is causing any additional problems,
   but if we want to make this behave more like other drivers in the
   long run or deal with machines that are missing a "dma-ranges"
   property in the parent node, we can kill off the
   of_platform_populate() hack and instead call dwc3_probe()
   directly from the glue drivers as in b), and have that
   do for_each_child_of_node() or similar to find the child node.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote:
>> Arnd Bergmann  writes:
>> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
>> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
>> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
>> >> > > 
>> >> > > I would be in favour of a dma_inherit() function as well. We could 
>> >> > > hack
>> >> > > something up in the arch code (like below) but I would rather prefer 
>> >> > > an
>> >> > > explicit dma_inherit() call by drivers creating such devices.
>> >> > > 
>> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h 
>> >> > > b/arch/arm64/include/asm/dma-mapping.h
>> >> > > index ba437f090a74..ea6fb9b0e8fa 100644
>> >> > > --- a/arch/arm64/include/asm/dma-mapping.h
>> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h
>> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>> >> > >  
>> >> > >  static inline struct dma_map_ops *__generic_dma_ops(struct device 
>> >> > > *dev)
>> >> > >  {
>> >> > > -   if (dev && dev->archdata.dma_ops)
>> >> > > -   return dev->archdata.dma_ops;
>> >> > > +   while (dev) {
>> >> > > +   if (dev->archdata.dma_ops)
>> >> > > +   return dev->archdata.dma_ops;
>> >> > > +   dev = dev->parent;
>> >> > > +   }
>> >> > 
>> >> > I think this would be a very bad idea: we don't want to have random
>> >> > devices be able to perform DMA just because their parent devices
>> >> > have been set up that way.
>> >> 
>> >> I agree, it's a big hack. It would be nice to have a simpler way to do
>> >> this in driver code rather than explicitly calling
>> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this
>> >> thread.
>> >
>> > I haven't followed the entire discussion, but what's wrong with passing
>> > around a pointer to a 'struct device *hwdev' that represents the physical
>> > device that does the DMA?
>> 
>> that will likely create duplicated solutions in several drivers and
>> it'll be a pain to maintain. There's another complication, dwc3 can be
>> integrated in many different ways. See the device child-parent tree
>> representations below:
>> 
>> a) with a parent PCI device:
>> 
>> pci_bus_type
>>  - dwc3-pci
>>- dwc3
>>  - xhci-plat
>> 
>> b) with a parent platform_device (OF):
>> 
>> platform_bus_type
>>  - dwc3-${omap,st,of-simple,exynos,keystone}
>>- dwc3
>>  - xhci-plat
>> 
>> c) without a parent at all (thanks Grygorii):
>> 
>> platform_bus_type
>>  - dwc3
>>- xhci-plat
>> 
>> (a) and (b) above are the common cases. The DMA-capable device is
>> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
>> having proper DMA configuration in OF platforms (because of the
>> unconditional of_dma_configure() during OF device creation) and
>> xhci-plat not knowing about DMA at all and hardcoding some crappy
>> defaults.
>> 
>> (c) is the uncommon case which creates some problems. In this case, dwc3
>> itself is the DMA-capable device and dwc3->dev->parent is the
>> platform_bus_type itself. Now consider the problem this creates:
>> 
>> i. the patch that I wrote [1] becomes invalid for (c), thanks to
>> Grygorii for pointing this out before it was too late.
>> 
>> ii. xhci-plat can also be described directly in DT (and is in some
>> cases). This means that assuming xhci-plat's parent's parent to be the
>> DMA-capable device is also an invalid assumption.
>> 
>> iii. one might argue that for DT-based platforms *with* a glue layer
>> ((b) above), OF already "copies" some sensible DMA defaults during
>> device creation.
>
> But that is exactly the point I was trying to make: instead of copying
> all the data into the xhci-plat device, just assign one pointer
> inside the xhci-plat device from whoever creates it.

and how do you pass that pointer to xhci-plat from another driver ?
No, platform_data is not an option ;-)

>> The only clean way to fix this up is with a dma_inherit()-like API which
>> would allow dwc3's glue layers ((a) and (b) above) to initialize child's
>> (dwc3) DMA configuration during child's creation. Something like below:
>> 
>> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
>> index adc1e8a624cb..74b599269e2c 100644
>> --- a/drivers/usb/dwc3/dwc3-pci.c
>> +++ b/drivers/usb/dwc3/dwc3-pci.c
>> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
>>  return -ENOMEM;
>>  }
>>  
>> +dma_inherit(>dev, dev);
>> +
>>  memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
>>  
>>  res[0].start= pci_resource_start(pci, 0);
>> 
>> that's all I'm asking for :-) dma_inherit() should, probably, be
>> arch-specific to handle details like IOMMU (which today sits under
>> archdata).
>
> It's still wrong: the archdata in the device can contain all sorts of
> additional information about how to do DMA, and some of that 

Re: [PATCH v2] xhci: Cleanup only when releasing primary hcd.

[Joe Lawrence]

Hello Mathias, Roger, Gabriel

I've been chasing strange MSI / legacy IRQ behavior from xHCI for a
couple days and wanted to report a few things that may be effected by
Gabriel's recent "xhci: Cleanup only when releasing primary hcd" patch
(more on this at the bottom).

After 8c24d6d7b09d "usb: xhci: stop everything on the first call to
xhci_stop" (v4.3+), there exists a device removal window that can later
leak interrupts and other device resources ordinarily cleaned up by
xhci_stop.

The quick summary is that if the xhci_handshake call from xhci_halt
fails, it will set xhci->xhc_state |= XHCI_STATE_HALTED.  Later when the
device is removed, xhci_stop keys off the xhc_state and will exit early
if XHCI_STATE_HALTED is set.  

This manifested itself on my Stratus machine while running repeated
device hotplug removals at various timing intervals during
initialization (for example 3, 6 or 12 seconds after hotplug add).  I
didn't grab a backtrace of xhci_halt, so I'm not exactly sure why it was
called after xhci_run, but note that it is not only called from
xhci_stop, but also xhci_handle_command_timeout and others.

The result was that a later device initialization couldn't allocate its
MSI interrupts -- after much instrumentation I finally traced it back to
the previous initialization / removal failing to free the MSI irqs it
requested. 

Here's the relevent snippet from added trace logging.
(ftmod_ioboard_event and ioboard_bringdown are functions from an
out-of-tree PCI hotplug driver for Stratus hardware platforms.  These
routines will call into PCI API to add or remove a set of PCI devices so
I left them here to help mark device lifetimes.)

Last successful MSI allocation
==

java-17858 [002]   3005.763238: ftmod_ioboard_event: adding bus 
03
java-17858 [009]   3009.780359: usb_add_hcd: 
request_irq(irq=10, desc(88085f41bba8), hcd(8808518e53d8)) return=0
java-17858 [004]   3009.808979: usb_add_hcd: 
request_irq(irq=11, desc(88085f41bef8), hcd(880482d992a8)) return=0
java-17858 [008]   3009.828263: xhci_halt: 
xhci(88083f5a8258)
java-17858 [008]   3009.828269: xhci_quiesce: 
xhci(88083f5a8258) halted=1
java-17858 [008]   3009.828273: xhci_quiesce: 
xhci(88083f5a8258) cmd=0
java-17858 [008]   3009.829614: xhci_setup_msix: 
xhci=88083f5a8258
java-17858 [008]   3009.829627: xhci_setup_msix: 
xhci=88083f5a8258 Failed to enable MSI-X, ret=-22
java-17858 [008]   3009.829635: xhci_run: xhci=88083f5a8258
java-17858 [008]   3009.830069: xhci_run: request_irq(119, ...) 
MSI desc=88083ac32e68 ret=0
java-17858 [007]   3010.009977: xhci_halt: 
xhci(88083f5a8258)

xhci_halt is called, perhaps from
xhci_handle_command_timeout?

java-17858 [007]   3010.009985: xhci_quiesce: 
xhci(88083f5a8258) halted=1
java-17858 [007]   3010.009986: xhci_quiesce: 
xhci(88083f5a8258) cmd=

cmd = ~0, so the hardware is gone!  subsequent
call to xhci_handshake will detect this and return
-ENODEV, xhci_halt will then set 
xhci->xhc_state |= XHCI_STATE_HALTED

java-17858 [007] d...  3010.010381: xhci_irq: 
hcd(88083f5a8008)->irq=0 (depth=0, count=300, unhandled=0), status==~0

xhci_irq notices the missing hardware, too.

java-17858 [007]   3010.014476: xhci_stop: 
xhci(88083f5a8258)->xhc_state=2

xhci_stop is called and exits early because
(xhci->xhc_state & XHCI_STATE_HALTED)
Notice no entries for xhci_free_irq!

java-17858 [007]   3010.029101: ftmod_ioboard_event: adding bus 
79
   kworker/u97:3-974   [009]   3013.264132: ioboard_bringdown: board 0
   kworker/u97:3-974   [009]   3013.264145: ioboard_bringdown: sbus=0, 
removing bus 03
   kworker/u97:3-974   [005]   3016.551113: ioboard_bringdown: sbus=1, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551119: ioboard_bringdown: sbus=2, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551119: ioboard_bringdown: sbus=3, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551119: ioboard_bringdown: sbus=4, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551120: ioboard_bringdown: sbus=5, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551120: ioboard_bringdown: sbus=6, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551120: ioboard_bringdown: sbus=7, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551120: ioboard_bringdown: sbus=8, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551120: ioboard_bringdown: sbus=9, 
bs->TopLevelBridge[sbus] = 0
   kworker/u97:3-974   [005]   3016.551124: ioboard_bringdown: sbus=10, 
pdev =   (null)
   kworker/u97:3-974   

RE: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added comments to CRTSCT flag code.

[Konstantin Shkolnyy]

> -Original Message-
> From: Johan Hovold [mailto:jhov...@gmail.com] On Behalf Of Johan Hovold
> Sent: Tuesday, April 26, 2016 02:26
> To: Konstantin Shkolnyy
> Cc: Johan Hovold; Konstantin Shkolnyy; linux-usb@vger.kernel.org; linux-
> ker...@vger.kernel.org
> Subject: Re: [EXT] Re: [PATCH RESEND 3/5] USB: serial: cp210x: Added
> comments to CRTSCT flag code.
> 
> On Mon, Apr 25, 2016 at 06:09:01PM +, Konstantin Shkolnyy wrote:
> > I was planning to define all these bits in a separate future patch.
> > Would you rather prefer the magic numbers defined before fixing the
> bugs?
> 
> Fixing the RTS bug (patch 1), which is the only "real" bug, should be
> done before adding defines, and fixing and cleaning up the rest.
> 
> > I guess I can do that. Is something like this acceptable?
> >
> > /* CP210X_GET_FLOW/CP210X_SET_FLOW read/write these 0x10 bytes */
> > struct cp210x_flow_ctl {
> > u8  SERIAL_DTR_MASK: 2; /* byte 0 */
> > u8 : 1;
> > u8  SERIAL_CTS_HANDSHAKE   : 1;
> > u8  SERIAL_DSR_HANDSHAKE   : 1;
> > u8  SERIAL_DCD_HANDSHAKE   : 1;
> > u8  SERIAL_DSR_SENSITIVITY : 1;
> > u8 : 1;
> > u8; /* byte 1 */
> > u8; /* byte 2 */
> > u8; /* byte 3 */
> > u8  SERIAL_AUTO_TRANSMIT   : 1; /* byte 4 */
> > u8  SERIAL_AUTO_RECEIVE: 1;
> > u8  SERIAL_ERROR_CHAR  : 1;
> > u8  SERIAL_NULL_STRIPPING  : 1;
> > u8  SERIAL_BREAK_CHAR  : 1;
> > u8 : 1;
> > u8  SERIAL_RTS_MASK: 2;
> > u8; /* byte 5 */
> > u8; /* byte 6 */
> > u8 : 7; /* byte 7 */
> > u8  SERIAL_XOFF_CONTINUE   : 1;
> > __le32  ulXonLimit;
> > __le32  ulXoffLimit;
> > } __packed;
> 
> No, shouldn't rely on the layout of bitfields. Define masks and shifts
> as needed and the message structure as
> 
> struct cp210x_flow_ctl {
>   __le32  ulControlHandshake;
>   __le32  ulFlowReplace;
>   __le32  ulXonLimit;
>   __le32  ulXoffLimit;
> };
> 
> that is, as per AN571.
> 

OK, from searching www I see that bitfields have bad reputation for unclear 
reasons, so I guess it's now easier to avoid them.
But doing it like you suggest, instead of splitting it to bytes, would 
complicate the code with endian conversions.
Is there a reason for this other than making it identical to the spec?\

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
>> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
>> 
>> > I've looked at the usb HCD code now and see this:
>> > 
>> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
>> > struct device *dev, const char *bus_name,
>> > struct usb_hcd *primary_hcd)
>> > {
>> >   ...
>> > hcd->self.controller = dev;
>> > hcd->self.uses_dma = (dev->dma_mask != NULL);
>> >   ...
>> > }
>> > 
>> > What I think we need to do here is ensure that the device that gets
>> > passed here and assigned to hcd->self.controller is the actual DMA
>> > master device, i.e. the pci_device or platform_device that was created
>> > from outside of the xhci stack. This is after all the pointer that
>> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
>> > functions.
>> 
>> It would be better to add a new field, since self.controller is also
>> used for lots of other purposes.  Something like hcd->self.dma_dev.
>
> Ok, fair enough. I only took a brief look and all uses I found were
> either for the DMA mapping API or some printk logging.

I have a feeling you guys are not considering how the patch to implement
this will look like.

How are you expecting dwc3 to pass a pointer to the DMA device from
dwc3.ko to xhci-plat ? platform_data ? That's gonna be horrible :-)

Also, remember that the DMA device for dwc3 is not always
dwc3->dev->parent. It might be dwc3->dev itself. How are you expecting
us to figure that one out ?

I still think dma_inherit() (or something along those lines) is
necessary. Specially when you consider that, as I said previously,
that's pretty much what of_dma_configure() does.

Anyway, I'd really like to see a patch implementing this
hcd->self.dma_dev logic. Consider all the duplication with this
approach, btw. struct dwc3 will *also* need a dwc->dma_dev of its
own. Will that be passed to dwc3 as platform_data from glue layer ? What
about platforms which don't even use a glue layer ?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: devio: declare usbdev_vm_ops as static

[Michele Curti]

usbdev_vm_ops is used in devio.c only, so declare it as static

Signed-off-by: Michele Curti 
---
 drivers/usb/core/devio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/core/devio.c b/drivers/usb/core/devio.c
index 52c4461..73ce871 100644
--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -216,7 +216,7 @@ static void usbdev_vm_close(struct vm_area_struct *vma)
dec_usb_memory_use_count(usbm, >vma_use_count);
 }
 
-struct vm_operations_struct usbdev_vm_ops = {
+static struct vm_operations_struct usbdev_vm_ops = {
.open = usbdev_vm_open,
.close = usbdev_vm_close
 };
-- 
2.8.0

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

Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

[Bin Liu]

Hi,

On Wed, Apr 27, 2016 at 09:26:10PM +0300, Maxim Uvarov wrote:
> 2016-04-27 18:46 GMT+03:00 Bin Liu :
> > Hi,
> >
> > On Wed, Apr 27, 2016 at 09:51:58AM +0300, Max Uvarov wrote:
> >> Fix soft lockup when resetting remote device attached
> >> to usb host. Configuration:
> >> pppd -> musb hub -> usb-serial -> gsm modem
> >
> > I have heard a few reports similar to this symptom, but never been able
> > to reproduce it on my side.
> >
> 
> Ok, I can reproduce it almost very easy.
> 
> >> When gsm modem resets, musb rolls in incoming rx interrupts
> >> which does not give any time to other application as result
> >> it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
> >
> > Have you looked where exact place in the interrupt routine the execution
> > has stuck in?
> >
> 
> It does not stuck. It goes to that line which print proto error over
> and over again and
> nothing stops that. After some time kernel reports lockup. But
> actually it's not stuck,
> all cpu time was eaten by executing that handlers.
> 
> 
> >> and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
> >> for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
> >
> > MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures
> > MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in
> > musb_regs.h.
> >
> >>
> >> Signed-off-by: Max Uvarov 
> >> ---
> >>  v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
> >>
> >>  drivers/usb/musb/musb_host.c | 12 +---
> >>  1 file changed, 5 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> >> index c3d5fc9..2d9aa78 100644
> >> --- a/drivers/usb/musb/musb_host.c
> >> +++ b/drivers/usb/musb/musb_host.c
> >> @@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
> >
> > What kernel do you use? This line # is away off from upstream kernel.
> >
> 
> I did this patch for 4.1 but 4.6 has the same problem and patch
> cleanly applies to the latest torvalds/linux.git v4.6-rc5. This
> interrupt handler has the same code.  And looks like on 3.14

Yeah, this code hasn't been chaned for year. But in general, it is
prepfered to create patches on latest kernel to avoid other headache.

> everything worked. I don't have a time to diff 2 versions. Might be
> regression.
> 
> 
> >>
> >>   /* stall; record URB status */
> >>   status = -EPIPE;
> >> + } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
> >>
> >> - } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
> >> - dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
> >> -
> >> - status = -EPROTO;
> >> - musb_writeb(epio, MUSB_RXINTERVAL, 0);
> >> -
> >> - } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
> >> + if (rx_csr & MUSB_RXCSR_H_ERROR) {
> >> + status = -EPROTO;
> >> + musb_writeb(epio, MUSB_RXINTERVAL, 0);
> >> + }
> >
> > Please help me to understand how this change fixes the issue. I see the
> > most effect of the change here is directly 'goto finish' so that 'done'
> > flag is not set, then musb_advance_schedule() is not called. Is this the
> > case or I missed other important pieces?
> >
> 
> Right that is the goal. On this rxcsr_h_error kernel reschedules
> current interrupt.  And that continues forever. For example adding

The MUSB Programming Guide says CPU should clear this MUSB_RXCSR_H_ERROR
bit, but the current driver doesn't. I am wondering if this causes the
controller keeps generating the same interrupt. Can you please try the
following change instead to see if the lockup goes away?

@@ -1870,6 +1870,9 @@ void musb_host_rx(struct musb *musb, u8 epnum)
status = -EPROTO;
musb_writeb(epio, MUSB_RXINTERVAL, 0);
 
+   rx_csr &= ~MUSB_RXCSR_H_ERROR;
+   musb_writew(epio, MUSB_RXCSR, rx_csr);
+
} else if (rx_csr & MUSB_RXCSR_DATAERROR) {
 
if (USB_ENDPOINT_XFER_ISOC != qh->type) {

Regards,
-Bin.

> msleep() can give some time for other processes. I'm not an expert in
> this chip but I think that right solution in that case is not try to
> reschedule and quick and allow hub to make reset and once again init
> all devices (in my case ppp/pppd also shutdowns and then I bring
> everything up with script.). The same behavior with dma and pio mode.
> 
> Regards,
> Max.
> 
> > Thanks,
> > -Bin.
> >
> >>
> >>   if (USB_ENDPOINT_XFER_ISOC != qh->type) {
> >>   dev_dbg(musb->controller, "RX end %d NAK timeout\n", 
> >> epnum);
> >> --
> >> 1.9.1
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> >> the body of a message to majord...@vger.kernel.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
> 
> 
> -- 
> Best regards,
> Maxim Uvarov
--
To 

Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

[Maxim Uvarov]

2016-04-27 18:46 GMT+03:00 Bin Liu :
> Hi,
>
> On Wed, Apr 27, 2016 at 09:51:58AM +0300, Max Uvarov wrote:
>> Fix soft lockup when resetting remote device attached
>> to usb host. Configuration:
>> pppd -> musb hub -> usb-serial -> gsm modem
>
> I have heard a few reports similar to this symptom, but never been able
> to reproduce it on my side.
>

Ok, I can reproduce it almost very easy.

>> When gsm modem resets, musb rolls in incoming rx interrupts
>> which does not give any time to other application as result
>> it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
>
> Have you looked where exact place in the interrupt routine the execution
> has stuck in?
>

It does not stuck. It goes to that line which print proto error over
and over again and
nothing stops that. After some time kernel reports lockup. But
actually it's not stuck,
all cpu time was eaten by executing that handlers.


>> and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
>> for setting rx stall with MUSB_RXCSR_H_WZC_BITS.
>
> MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures
> MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in
> musb_regs.h.
>
>>
>> Signed-off-by: Max Uvarov 
>> ---
>>  v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
>>
>>  drivers/usb/musb/musb_host.c | 12 +---
>>  1 file changed, 5 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
>> index c3d5fc9..2d9aa78 100644
>> --- a/drivers/usb/musb/musb_host.c
>> +++ b/drivers/usb/musb/musb_host.c
>> @@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
>
> What kernel do you use? This line # is away off from upstream kernel.
>

I did this patch for 4.1 but 4.6 has the same problem and patch
cleanly applies to the
latest torvalds/linux.git v4.6-rc5. This interrupt handler has the
same code.  And looks
like on 3.14 everything worked. I don't have a time to diff 2
versions. Might be regression.


>>
>>   /* stall; record URB status */
>>   status = -EPIPE;
>> + } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
>>
>> - } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
>> - dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
>> -
>> - status = -EPROTO;
>> - musb_writeb(epio, MUSB_RXINTERVAL, 0);
>> -
>> - } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
>> + if (rx_csr & MUSB_RXCSR_H_ERROR) {
>> + status = -EPROTO;
>> + musb_writeb(epio, MUSB_RXINTERVAL, 0);
>> + }
>
> Please help me to understand how this change fixes the issue. I see the
> most effect of the change here is directly 'goto finish' so that 'done'
> flag is not set, then musb_advance_schedule() is not called. Is this the
> case or I missed other important pieces?
>

Right that is the goal. On this rxcsr_h_error kernel reschedules
current interrupt.
And that continues forever. For example adding msleep() can give some
time for other
processes. I'm not an expert in this chip but I think that right
solution in that case is not
try to reschedule and quick and allow hub to make reset and once again
init all devices
(in my case ppp/pppd also shutdowns and then I bring everything up
with script.). The
same behavior with dma and pio mode.

Regards,
Max.

> Thanks,
> -Bin.
>
>>
>>   if (USB_ENDPOINT_XFER_ISOC != qh->type) {
>>   dev_dbg(musb->controller, "RX end %d NAK timeout\n", 
>> epnum);
>> --
>> 1.9.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday 27 April 2016 13:59:13 Alan Stern wrote:
> On Wed, 27 Apr 2016, Arnd Bergmann wrote:
> 
> > I've looked at the usb HCD code now and see this:
> > 
> > struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> > struct device *dev, const char *bus_name,
> > struct usb_hcd *primary_hcd)
> > {
> >   ...
> > hcd->self.controller = dev;
> > hcd->self.uses_dma = (dev->dma_mask != NULL);
> >   ...
> > }
> > 
> > What I think we need to do here is ensure that the device that gets
> > passed here and assigned to hcd->self.controller is the actual DMA
> > master device, i.e. the pci_device or platform_device that was created
> > from outside of the xhci stack. This is after all the pointer that
> > gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> > functions.
> 
> It would be better to add a new field, since self.controller is also
> used for lots of other purposes.  Something like hcd->self.dma_dev.
> 

Ok, fair enough. I only took a brief look and all uses I found were
either for the DMA mapping API or some printk logging.

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

Re: [PATCH] Documentation: ABI: Add doc for usbip-vudc attributes

[Krzysztof Opasiak]

On 04/27/2016 07:37 PM, Greg KH wrote:
> On Wed, Apr 27, 2016 at 07:12:47PM +0200, Krzysztof Opasiak wrote:
>> Signed-off-by: Krzysztof Opasiak 
> 
> No description of why this patch is needed?
> 
> 

I have just sent an updated v2 version but unfortunately I used wrong
message-Id to which I replied so you may find v2 here:

http://marc.info/?l=linux-usb=146177998131867=2

Sorry for your inconvenience,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: usbip: vudc: Rename find_endpoint() to vudc_find_endpoint()

[Krzysztof Opasiak]

As find_endpoint() is a global funcion rename it to vudc_find_endpoint()
to clearly mark where does it come from.

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/usbip/vudc.h  |2 +-
 drivers/usb/usbip/vudc_dev.c  |2 +-
 drivers/usb/usbip/vudc_rx.c   |2 +-
 drivers/usb/usbip/vudc_transfer.c |6 +++---
 4 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/usbip/vudc.h b/drivers/usb/usbip/vudc.h
index 8478923..25e01b0 100644
--- a/drivers/usb/usbip/vudc.h
+++ b/drivers/usb/usbip/vudc.h
@@ -179,7 +179,7 @@ void v_stop_timer(struct vudc *udc);
 struct urbp *alloc_urbp(void);
 void free_urbp_and_urb(struct urbp *urb_p);
 
-struct vep *find_endpoint(struct vudc *udc, u8 address);
+struct vep *vudc_find_endpoint(struct vudc *udc, u8 address);
 
 struct vudc_device *alloc_vudc_device(int devid);
 void put_vudc_device(struct vudc_device *udc_dev);
diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 0523f29..8994a13 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -115,7 +115,7 @@ static void stop_activity(struct vudc *udc)
}
 }
 
-struct vep *find_endpoint(struct vudc *udc, u8 address)
+struct vep *vudc_find_endpoint(struct vudc *udc, u8 address)
 {
int i;
 
diff --git a/drivers/usb/usbip/vudc_rx.c b/drivers/usb/usbip/vudc_rx.c
index 0b7abbc..344bd94 100644
--- a/drivers/usb/usbip/vudc_rx.c
+++ b/drivers/usb/usbip/vudc_rx.c
@@ -117,7 +117,7 @@ static int v_recv_cmd_submit(struct vudc *udc,
address |= USB_DIR_IN;
 
spin_lock_irq(>lock);
-   urb_p->ep = find_endpoint(udc, address);
+   urb_p->ep = vudc_find_endpoint(udc, address);
if (!urb_p->ep) {
/* we don't know the type, there may be isoc data! */
dev_err(>pdev->dev, "request to nonexistent endpoint");
diff --git a/drivers/usb/usbip/vudc_transfer.c 
b/drivers/usb/usbip/vudc_transfer.c
index 5ccde52..aba6bd4 100644
--- a/drivers/usb/usbip/vudc_transfer.c
+++ b/drivers/usb/usbip/vudc_transfer.c
@@ -110,7 +110,7 @@ static int handle_control_request(struct vudc *udc, struct 
urb *urb,
}
} else if (setup->bRequestType == EP_REQUEST) {
/* endpoint halt */
-   ep2 = find_endpoint(udc, w_index);
+   ep2 = vudc_find_endpoint(udc, w_index);
if (!ep2 || ep2->ep.name == udc->ep[0].ep.name) {
ret_val = -EOPNOTSUPP;
break;
@@ -143,7 +143,7 @@ static int handle_control_request(struct vudc *udc, struct 
urb *urb,
}
} else if (setup->bRequestType == EP_REQUEST) {
/* endpoint halt */
-   ep2 = find_endpoint(udc, w_index);
+   ep2 = vudc_find_endpoint(udc, w_index);
if (!ep2) {
ret_val = -EOPNOTSUPP;
break;
@@ -167,7 +167,7 @@ static int handle_control_request(struct vudc *udc, struct 
urb *urb,
buf = (char *)urb->transfer_buffer;
if (urb->transfer_buffer_length > 0) {
if (setup->bRequestType == EP_INREQUEST) {
-   ep2 = find_endpoint(udc, w_index);
+   ep2 = vudc_find_endpoint(udc, w_index);
if (!ep2) {
ret_val = -EOPNOTSUPP;
break;
-- 
1.7.9.5

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

[PATCH v2] usb: usbip: vudc: Fix WARN_ON() usage pattern

[Krzysztof Opasiak]

Fix WARN_ON() macro usage as suggested by Felipe.
Instead of using:
if (cond) {
   WARN_ON(1);
   do_stuff();
}

Use a better pattern with WARN_ON() placed in if condition:

if (WARN_ON(cond))
   do_stuff();

Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- Update commit message
---
 drivers/usb/usbip/vudc_dev.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 43047f4..0523f29 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -312,10 +312,9 @@ static void vep_free_request(struct usb_ep *_ep, struct 
usb_request *_req)
 {
struct vrequest *req;
 
-   if (!_ep || !_req) {
-   WARN_ON(1);
+   if (WARN_ON(!_ep || !_req))
return;
-   }
+
req = to_vrequest(_req);
kfree(req);
 }
-- 
1.7.9.5

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

[PATCH v2] Documentation: ABI: Add doc for usbip-vudc attributes

[Krzysztof Opasiak]

As vudc provides some new attributes using sysfs infrastructure,
add a suitable documentation file for those attributes.

Signed-off-by: Krzysztof Opasiak 
---
Changes since v1:
- Update commit message

---
 .../ABI/testing/sysfs-platform-usbip-vudc  |   35 
 1 file changed, 35 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-platform-usbip-vudc

diff --git a/Documentation/ABI/testing/sysfs-platform-usbip-vudc 
b/Documentation/ABI/testing/sysfs-platform-usbip-vudc
new file mode 100644
index 000..81fcfb4
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-platform-usbip-vudc
@@ -0,0 +1,35 @@
+What:  /sys/devices/platform/usbip-vudc.%d/dev_desc
+Date:  April 2016
+KernelVersion: 4.6
+Contact:   Krzysztof Opasiak 
+Description:
+   This file allows to read device descriptor of
+   gadget driver which is currently bound to this
+   controller. It is possible to read this file
+   only if gadget driver is bound, otherwise error
+   is returned.
+
+What:  /sys/devices/platform/usbip-vudc.%d/usbip_status
+Date:  April 2016
+KernelVersion: 4.6
+Contact:   Krzysztof Opasiak 
+Description:
+   Current status of the device.
+   Allowed values:
+   1 - Device is available and can be exported
+   2 - Device is currently exported
+   3 - Fatal error occurred during communication
+ with peer
+
+What:  /sys/devices/platform/usbip-vudc.%d/usbip_sockfd
+Date:  April 2016
+KernelVersion: 4.6
+Contact:   Krzysztof Opasiak 
+Description:
+   This file allows to export usb device to
+   connection peer. It is done by writing to this
+   file socket fd (as a string for example "8")
+   associated with a connection to remote peer who
+   would like to use this device. It is possible to
+   close the connection by writing -1 instead of
+   socked fd.
-- 
1.7.9.5

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Alan Stern]

On Wed, 27 Apr 2016, Arnd Bergmann wrote:

> I've looked at the usb HCD code now and see this:
> 
> struct usb_hcd *usb_create_shared_hcd(const struct hc_driver *driver,
> struct device *dev, const char *bus_name,
> struct usb_hcd *primary_hcd)
> {
>   ...
> hcd->self.controller = dev;
> hcd->self.uses_dma = (dev->dma_mask != NULL);
>   ...
> }
> 
> What I think we need to do here is ensure that the device that gets
> passed here and assigned to hcd->self.controller is the actual DMA
> master device, i.e. the pci_device or platform_device that was created
> from outside of the xhci stack. This is after all the pointer that
> gets passed into all the dma_map_*/dma_sync_*/dma_alloc_*/...
> functions.

It would be better to add a new field, since self.controller is also
used for lots of other purposes.  Something like hcd->self.dma_dev.

Alan Stern

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday 27 April 2016 19:53:51 Felipe Balbi wrote:
> Arnd Bergmann  writes:
> > On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
> >> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> >> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> >> > > 
> >> > > I would be in favour of a dma_inherit() function as well. We could hack
> >> > > something up in the arch code (like below) but I would rather prefer an
> >> > > explicit dma_inherit() call by drivers creating such devices.
> >> > > 
> >> > > diff --git a/arch/arm64/include/asm/dma-mapping.h 
> >> > > b/arch/arm64/include/asm/dma-mapping.h
> >> > > index ba437f090a74..ea6fb9b0e8fa 100644
> >> > > --- a/arch/arm64/include/asm/dma-mapping.h
> >> > > +++ b/arch/arm64/include/asm/dma-mapping.h
> >> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> >> > >  
> >> > >  static inline struct dma_map_ops *__generic_dma_ops(struct device 
> >> > > *dev)
> >> > >  {
> >> > > -   if (dev && dev->archdata.dma_ops)
> >> > > -   return dev->archdata.dma_ops;
> >> > > +   while (dev) {
> >> > > +   if (dev->archdata.dma_ops)
> >> > > +   return dev->archdata.dma_ops;
> >> > > +   dev = dev->parent;
> >> > > +   }
> >> > 
> >> > I think this would be a very bad idea: we don't want to have random
> >> > devices be able to perform DMA just because their parent devices
> >> > have been set up that way.
> >> 
> >> I agree, it's a big hack. It would be nice to have a simpler way to do
> >> this in driver code rather than explicitly calling
> >> of_dma_configure/arch_setup_dma_ops as per the original patch in this
> >> thread.
> >
> > I haven't followed the entire discussion, but what's wrong with passing
> > around a pointer to a 'struct device *hwdev' that represents the physical
> > device that does the DMA?
> 
> that will likely create duplicated solutions in several drivers and
> it'll be a pain to maintain. There's another complication, dwc3 can be
> integrated in many different ways. See the device child-parent tree
> representations below:
> 
> a) with a parent PCI device:
> 
> pci_bus_type
>  - dwc3-pci
>- dwc3
>  - xhci-plat
> 
> b) with a parent platform_device (OF):
> 
> platform_bus_type
>  - dwc3-${omap,st,of-simple,exynos,keystone}
>- dwc3
>  - xhci-plat
> 
> c) without a parent at all (thanks Grygorii):
> 
> platform_bus_type
>  - dwc3
>- xhci-plat
> 
> (a) and (b) above are the common cases. The DMA-capable device is
> clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
> having proper DMA configuration in OF platforms (because of the
> unconditional of_dma_configure() during OF device creation) and
> xhci-plat not knowing about DMA at all and hardcoding some crappy
> defaults.
> 
> (c) is the uncommon case which creates some problems. In this case, dwc3
> itself is the DMA-capable device and dwc3->dev->parent is the
> platform_bus_type itself. Now consider the problem this creates:
> 
> i. the patch that I wrote [1] becomes invalid for (c), thanks to
> Grygorii for pointing this out before it was too late.
> 
> ii. xhci-plat can also be described directly in DT (and is in some
> cases). This means that assuming xhci-plat's parent's parent to be the
> DMA-capable device is also an invalid assumption.
> 
> iii. one might argue that for DT-based platforms *with* a glue layer
> ((b) above), OF already "copies" some sensible DMA defaults during
> device creation.

But that is exactly the point I was trying to make: instead of copying
all the data into the xhci-plat device, just assign one pointer
inside the xhci-plat device from whoever creates it.

> The only clean way to fix this up is with a dma_inherit()-like API which
> would allow dwc3's glue layers ((a) and (b) above) to initialize child's
> (dwc3) DMA configuration during child's creation. Something like below:
> 
> diff --git a/drivers/usb/dwc3/dwc3-pci.c b/drivers/usb/dwc3/dwc3-pci.c
> index adc1e8a624cb..74b599269e2c 100644
> --- a/drivers/usb/dwc3/dwc3-pci.c
> +++ b/drivers/usb/dwc3/dwc3-pci.c
> @@ -152,6 +152,8 @@ static int dwc3_pci_probe(struct pci_dev *pci,
>   return -ENOMEM;
>   }
>  
> + dma_inherit(>dev, dev);
> +
>   memset(res, 0x00, sizeof(struct resource) * ARRAY_SIZE(res));
>  
>   res[0].start= pci_resource_start(pci, 0);
> 
> that's all I'm asking for :-) dma_inherit() should, probably, be
> arch-specific to handle details like IOMMU (which today sits under
> archdata).

It's still wrong: the archdata in the device can contain all sorts of
additional information about how to do DMA, and some of that information
is bus specific, e.g. when your dma_map_ops look like the on sparc:

static inline struct dma_map_ops *get_dma_ops(struct device *dev)
{
#ifdef CONFIG_SPARC_LEON
if (sparc_cpu_model == sparc_leon)
return leon_dma_ops;
#endif
#if defined(CONFIG_SPARC32) 

Re: [PATCH] Documentation: ABI: Add doc for usbip-vudc attributes

[Krzysztof Opasiak]

On 04/27/2016 07:37 PM, Greg KH wrote:
> On Wed, Apr 27, 2016 at 07:12:47PM +0200, Krzysztof Opasiak wrote:
>> Signed-off-by: Krzysztof Opasiak 
> 
> No description of why this patch is needed?
> 

Sorry, though that title line is enough self-descriptive. I will resend
this with suitable descriptions.

Best regards,
-- 
Krzysztof Opasiak
Samsung R Institute Poland
Samsung Electronics
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: usbip: vudc: Fix WARN_ON() usage

[Greg KH]

On Wed, Apr 27, 2016 at 07:13:50PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak 

I can't take a patch with no changelog text :(
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Documentation: ABI: Add doc for usbip-vudc attributes

[Greg KH]

On Wed, Apr 27, 2016 at 07:12:47PM +0200, Krzysztof Opasiak wrote:
> Signed-off-by: Krzysztof Opasiak 

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

[PATCH] usb/host/: const data must use __initconst not __initdata

[Nicolas Pitre]

Init data marked const should be annotated with __initconst for
correctness and not __initdata.  This also fixes LTO builds that
otherwise fail with section mismatch errors.

Signed-off-by: Nicolas Pitre 

diff --git a/drivers/usb/host/ehci-exynos.c b/drivers/usb/host/ehci-exynos.c
index df538fd10a..42e5b66353 100644
--- a/drivers/usb/host/ehci-exynos.c
+++ b/drivers/usb/host/ehci-exynos.c
@@ -321,7 +321,7 @@ static struct platform_driver exynos_ehci_driver = {
.of_match_table = of_match_ptr(exynos_ehci_match),
}
 };
-static const struct ehci_driver_overrides exynos_overrides __initdata = {
+static const struct ehci_driver_overrides exynos_overrides __initconst = {
.extra_priv_size = sizeof(struct exynos_ehci_hcd),
 };
 
diff --git a/drivers/usb/host/ehci-msm.c b/drivers/usb/host/ehci-msm.c
index 3e226ef6ca..d3afc89d00 100644
--- a/drivers/usb/host/ehci-msm.c
+++ b/drivers/usb/host/ehci-msm.c
@@ -229,7 +229,7 @@ static struct platform_driver ehci_msm_driver = {
},
 };
 
-static const struct ehci_driver_overrides msm_overrides __initdata = {
+static const struct ehci_driver_overrides msm_overrides __initconst = {
.reset = ehci_msm_reset,
 };
 
diff --git a/drivers/usb/host/ehci-omap.c b/drivers/usb/host/ehci-omap.c
index a24720beb3..94ea9fff13 100644
--- a/drivers/usb/host/ehci-omap.c
+++ b/drivers/usb/host/ehci-omap.c
@@ -86,7 +86,7 @@ static inline u32 ehci_read(void __iomem *base, u32 reg)
 
 static struct hc_driver __read_mostly ehci_omap_hc_driver;
 
-static const struct ehci_driver_overrides ehci_omap_overrides __initdata = {
+static const struct ehci_driver_overrides ehci_omap_overrides __initconst = {
.extra_priv_size = sizeof(struct omap_hcd),
 };
 
diff --git a/drivers/usb/host/ehci-spear.c b/drivers/usb/host/ehci-spear.c
index 3c4e525395..1f25c7985f 100644
--- a/drivers/usb/host/ehci-spear.c
+++ b/drivers/usb/host/ehci-spear.c
@@ -163,7 +163,7 @@ static struct platform_driver spear_ehci_hcd_driver = {
}
 };
 
-static const struct ehci_driver_overrides spear_overrides __initdata = {
+static const struct ehci_driver_overrides spear_overrides __initconst = {
.extra_priv_size = sizeof(struct spear_ehci),
 };
 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] usb: usbip: vudc: Fix WARN_ON() usage

[Krzysztof Opasiak]

Signed-off-by: Krzysztof Opasiak 
---
 drivers/usb/usbip/vudc_dev.c |5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/usbip/vudc_dev.c b/drivers/usb/usbip/vudc_dev.c
index 43047f4..0523f29 100644
--- a/drivers/usb/usbip/vudc_dev.c
+++ b/drivers/usb/usbip/vudc_dev.c
@@ -312,10 +312,9 @@ static void vep_free_request(struct usb_ep *_ep, struct 
usb_request *_req)
 {
struct vrequest *req;
 
-   if (!_ep || !_req) {
-   WARN_ON(1);
+   if (WARN_ON(!_ep || !_req))
return;
-   }
+
req = to_vrequest(_req);
kfree(req);
 }
-- 
1.7.9.5

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

Re: [PATCH 07/12] usbip: vudc: Add UDC specific ops

[Krzysztof Opasiak]

Hi
On 04/27/2016 07:18 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Krzysztof Opasiak  writes:
>> From: Igor Kotrasinski 
>>
>> Add endpoints definitions and ops for both endpoints and gadget.
>> Add also a suitable platform driver and functions for handling
>> usbip events.
>>
>> This commit is a result of cooperation between Samsung R Institute
>> Poland and Open Operating Systems Student Society at University
>> of Warsaw (O2S3@UW) consisting of:
>>
>> Igor Kotrasinski 
>> Karol Kosik 
>> Ewelina Kosmider <3w3l...@gmail.com>
>> Dawid Lazarczyk 
>> Piotr Szulc 
>>
>> Tutor and project owner:
>> Krzysztof Opasiak 
>>
>> Signed-off-by: Igor Kotrasinski 
>> Signed-off-by: Karol Kosik 
>> [Various bug fixes, improvements and commit msg update]
>> Signed-off-by: Krzysztof Opasiak 
> 
> just a couple minor nits below. Nice work
> 
>> +struct vep *find_endpoint(struct vudc *udc, u8 address)
> 
> polluting namespace. Care to add 'static'?

Unfortunately this cannot be static as it is used in different files.

$ cd $KERN_DIR/drivers/usb/usbip
$ git grep -l find_endpoint
vudc.h
vudc_dev.c
vudc_rx.c
vudc_transfer.c

of course I can make it static inline and put it into header but I'm not
sure if this makes any sense for this function?

> 
>> +static void vep_free_request(struct usb_ep *_ep, struct usb_request *_req)
>> +{
>> +struct vrequest *req;
>> +
>> +if (!_ep || !_req) {
>> +WARN_ON(1);
>> +return;
>> +}
> 
> if (WARN_ON(!_ep || !_req))
>   return;
> 

Sorry I missed this one. Fixed. Thanks

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Felipe Balbi]

Hi,

Arnd Bergmann  writes:
> On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
>> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
>> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
>> > > 
>> > > I would be in favour of a dma_inherit() function as well. We could hack
>> > > something up in the arch code (like below) but I would rather prefer an
>> > > explicit dma_inherit() call by drivers creating such devices.
>> > > 
>> > > diff --git a/arch/arm64/include/asm/dma-mapping.h 
>> > > b/arch/arm64/include/asm/dma-mapping.h
>> > > index ba437f090a74..ea6fb9b0e8fa 100644
>> > > --- a/arch/arm64/include/asm/dma-mapping.h
>> > > +++ b/arch/arm64/include/asm/dma-mapping.h
>> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>> > >  
>> > >  static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
>> > >  {
>> > > -   if (dev && dev->archdata.dma_ops)
>> > > -   return dev->archdata.dma_ops;
>> > > +   while (dev) {
>> > > +   if (dev->archdata.dma_ops)
>> > > +   return dev->archdata.dma_ops;
>> > > +   dev = dev->parent;
>> > > +   }
>> > 
>> > I think this would be a very bad idea: we don't want to have random
>> > devices be able to perform DMA just because their parent devices
>> > have been set up that way.
>> 
>> I agree, it's a big hack. It would be nice to have a simpler way to do
>> this in driver code rather than explicitly calling
>> of_dma_configure/arch_setup_dma_ops as per the original patch in this
>> thread.
>
> I haven't followed the entire discussion, but what's wrong with passing
> around a pointer to a 'struct device *hwdev' that represents the physical
> device that does the DMA?

that will likely create duplicated solutions in several drivers and
it'll be a pain to maintain. There's another complication, dwc3 can be
integrated in many different ways. See the device child-parent tree
representations below:

a) with a parent PCI device:

pci_bus_type
 - dwc3-pci
   - dwc3
 - xhci-plat

b) with a parent platform_device (OF):

platform_bus_type
 - dwc3-${omap,st,of-simple,exynos,keystone}
   - dwc3
 - xhci-plat

c) without a parent at all (thanks Grygorii):

platform_bus_type
 - dwc3
   - xhci-plat

(a) and (b) above are the common cases. The DMA-capable device is
clearly dwc3-${pci,omap,st,of-simple,exynos,keystone} with dwc3 only
having proper DMA configuration in OF platforms (because of the
unconditional of_dma_configure() during OF device creation) and
xhci-plat not knowing about DMA at all and hardcoding some crappy
defaults.

(c) is the uncommon case which creates some problems. In this case, dwc3
itself is the DMA-capable device and dwc3->dev->parent is the
platform_bus_type itself. Now consider the problem this creates:

i. the patch that I wrote [1] becomes invalid for (c), thanks to
Grygorii for pointing this out before it was too late.

ii. xhci-plat can also be described directly in DT (and is in some
cases). This means that assuming xhci-plat's parent's parent to be the
DMA-capable device is also an invalid assumption.

iii. one might argue that for DT-based platforms *with* a glue layer
((b) above), OF already "copies" some sensible DMA defaults during
device creation. PCI-based systems just don't have the luxury of
creating random PCI devices like that :-) I say it copies because I can
pass *any* struct device_node pointer and it'll just copy that to the
struct device argument. Here's of_dma_configure() to make your life
easier:

void of_dma_configure(struct device *dev, struct device_node *np)
{
u64 dma_addr, paddr, size;
int ret;
bool coherent;
unsigned long offset;
struct iommu_ops *iommu;

/*
 * Set default coherent_dma_mask to 32 bit.  Drivers are expected to
 * setup the correct supported mask.
 */
if (!dev->coherent_dma_mask)
dev->coherent_dma_mask = DMA_BIT_MASK(32);

/*
 * Set it to coherent_dma_mask by default if the architecture
 * code has not set it.
 */
if (!dev->dma_mask)
dev->dma_mask = >coherent_dma_mask;

ret = of_dma_get_range(np, _addr, , );
if (ret < 0) {
dma_addr = offset = 0;
size = dev->coherent_dma_mask + 1;
} else {
offset = PFN_DOWN(paddr - dma_addr);

/*
 * Add a work around to treat the size as mask + 1 in case
 * it is defined in DT as a mask.
 */
if (size & 1) {
dev_warn(dev, "Invalid size 0x%llx for dma-range\n",
 size);
size = size + 1;
}

if (!size) {
dev_err(dev, "Adjusted size 0x%llx invalid\n", size);
return;
}

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday 27 April 2016 16:50:19 Catalin Marinas wrote:
> On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> > On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> > > 
> > > I would be in favour of a dma_inherit() function as well. We could hack
> > > something up in the arch code (like below) but I would rather prefer an
> > > explicit dma_inherit() call by drivers creating such devices.
> > > 
> > > diff --git a/arch/arm64/include/asm/dma-mapping.h 
> > > b/arch/arm64/include/asm/dma-mapping.h
> > > index ba437f090a74..ea6fb9b0e8fa 100644
> > > --- a/arch/arm64/include/asm/dma-mapping.h
> > > +++ b/arch/arm64/include/asm/dma-mapping.h
> > > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> > >  
> > >  static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> > >  {
> > > -   if (dev && dev->archdata.dma_ops)
> > > -   return dev->archdata.dma_ops;
> > > +   while (dev) {
> > > +   if (dev->archdata.dma_ops)
> > > +   return dev->archdata.dma_ops;
> > > +   dev = dev->parent;
> > > +   }
> > 
> > I think this would be a very bad idea: we don't want to have random
> > devices be able to perform DMA just because their parent devices
> > have been set up that way.
> 
> I agree, it's a big hack. It would be nice to have a simpler way to do
> this in driver code rather than explicitly calling
> of_dma_configure/arch_setup_dma_ops as per the original patch in this
> thread.
> 


I haven't followed the entire discussion, but what's wrong with passing
around a pointer to a 'struct device *hwdev' that represents the physical
device that does the DMA?

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Catalin Marinas]

On Wed, Apr 27, 2016 at 04:11:17PM +0200, Arnd Bergmann wrote:
> On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> > 
> > I would be in favour of a dma_inherit() function as well. We could hack
> > something up in the arch code (like below) but I would rather prefer an
> > explicit dma_inherit() call by drivers creating such devices.
> > 
> > diff --git a/arch/arm64/include/asm/dma-mapping.h 
> > b/arch/arm64/include/asm/dma-mapping.h
> > index ba437f090a74..ea6fb9b0e8fa 100644
> > --- a/arch/arm64/include/asm/dma-mapping.h
> > +++ b/arch/arm64/include/asm/dma-mapping.h
> > @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
> >  
> >  static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
> >  {
> > -   if (dev && dev->archdata.dma_ops)
> > -   return dev->archdata.dma_ops;
> > +   while (dev) {
> > +   if (dev->archdata.dma_ops)
> > +   return dev->archdata.dma_ops;
> > +   dev = dev->parent;
> > +   }
> 
> I think this would be a very bad idea: we don't want to have random
> devices be able to perform DMA just because their parent devices
> have been set up that way.

I agree, it's a big hack. It would be nice to have a simpler way to do
this in driver code rather than explicitly calling
of_dma_configure/arch_setup_dma_ops as per the original patch in this
thread.

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

Re: [PATCHv2] musb_host: fix lockup on rxcsr_h_error

[Bin Liu]

Hi,

On Wed, Apr 27, 2016 at 09:51:58AM +0300, Max Uvarov wrote:
> Fix soft lockup when resetting remote device attached
> to usb host. Configuration:
> pppd -> musb hub -> usb-serial -> gsm modem

I have heard a few reports similar to this symptom, but never been able
to reproduce it on my side.

> When gsm modem resets, musb rolls in incoming rx interrupts
> which does not give any time to other application as result
> it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR

Have you looked where exact place in the interrupt routine the execution
has stuck in?

> and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
> for setting rx stall with MUSB_RXCSR_H_WZC_BITS.

MUSB_RXCSR_H_WZC_BITS itself does not set rx stall, it just ensures
MUSB_RXCSR_H_RXSTALL not to be cleared. Please check its comment in
musb_regs.h.

> 
> Signed-off-by: Max Uvarov 
> ---
>  v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).
> 
>  drivers/usb/musb/musb_host.c | 12 +---
>  1 file changed, 5 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
> index c3d5fc9..2d9aa78 100644
> --- a/drivers/usb/musb/musb_host.c
> +++ b/drivers/usb/musb/musb_host.c
> @@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)

What kernel do you use? This line # is away off from upstream kernel.

>  
>   /* stall; record URB status */
>   status = -EPIPE;
> + } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
>  
> - } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
> - dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
> -
> - status = -EPROTO;
> - musb_writeb(epio, MUSB_RXINTERVAL, 0);
> -
> - } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
> + if (rx_csr & MUSB_RXCSR_H_ERROR) {
> + status = -EPROTO;
> + musb_writeb(epio, MUSB_RXINTERVAL, 0);
> + }

Please help me to understand how this change fixes the issue. I see the
most effect of the change here is directly 'goto finish' so that 'done'
flag is not set, then musb_advance_schedule() is not called. Is this the
case or I missed other important pieces?

Thanks,
-Bin.

>  
>   if (USB_ENDPOINT_XFER_ISOC != qh->type) {
>   dev_dbg(musb->controller, "RX end %d NAK timeout\n", 
> epnum);
> -- 
> 1.9.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Clear the host side toggle manually when endpoint is 'soft reset'

[Julien D'ascenzio]

On 27/04/2016 16:07, Mathias Nyman wrote:
> Hi
>
> On 26.04.2016 12:23, Julien D'ascenzio wrote:
>> Hi,
>>
>> I see you make this patch in the kernel mainline:
>>
>> xhci: Clear the ho27082e2654dc148078b0abdfc3c8e5ccbde0ebfa: st side:
>> toggle manually when endpoint is 'soft reset'
>>
>> then you revert it:
>>
>> d0167ad2954ee2d1c70704c454c646086b6653d6: Revert "xhci: Clear the host
>> side toggle manually when endpoint is 'soft reset'"
>>
>> This patch correct a problem I have with a camera (IDS US3 uEye CP). I
>> think, it's a similar problem as:
>> http://www.spinics.net/lists/linux-usb/msg132221.html
>>
>> Sometime, This error appear when I transfer a raw video with my camera :
>>
>> xhci-hcd xhci-hcd.6.auto: URB transfer length is wrong, xHC issue? req.
>> len = 0, act. len = 4294967288
>>
>> After that, the camera doesn't work and I must physically disconnect it
>>
>> You find attached to this email the usb camera trace (wireshark) when
>> this error occurs.
>> The transfer seems to start in packet number 21 (URB_CONTROL).
>> The error seems to occur after packet number 694 when a "CLEAR FEATURE"
>> is request.
>>
>> I'd like to know if you still work to a new patch to correct this? I
>> could help you to make some test with my hardware.
>>
>> Best regards
>>
>> Julien D'Ascenzio
>>
>
> I haven't been looking at this for a long time, so with the "xhci: 
> Clear the host
> side toggle manually when endpoint is soft reset" patch everything works?
Yes, everything works thanks to this patch! I forgot to mention that I 
currently worked on the
last kernel V4.6-rc5 for odroid-xu3
>
> Some additional questions.
> Are you using uvc or some custom usbfs with libusb implementation?
> If libusb is used, is there some extra libusb_clear_halt() used as a 
> "soft reset"
> instead of actually clearing a halted endpoint?
Unfortunately, the driver to communicate with the camera is a 
proprietary library...
Their code doesn't seem to use libusb. Their library and executable 
don't have any trace of libusb.
They seem to use directly the device: "/dev/bus/usb/004/019" with their 
own code.

This is the Device Descriptor of the camera (they don't seem to use UVC):

Bus 004 Device 019: ID 1409:3240 IDS Imaging Development Systems GmbH
Device Descriptor:
   bLength18
   bDescriptorType 1
   bcdUSB   3.00
   bDeviceClass  255 Vendor Specific Class
   bDeviceSubClass   255 Vendor Specific Subclass
   bDeviceProtocol   255 Vendor Specific Protocol
   bMaxPacketSize0 9
   idVendor   0x1409 IDS Imaging Development Systems GmbH
   idProduct  0x3240
   bcdDevice0.00
   iManufacturer   1 Camera Manufacturer
   iProduct2 USB 3.0 Camera
   iSerial 0
   bNumConfigurations  1
   Configuration Descriptor:
 bLength 9
 bDescriptorType 2
 wTotalLength   57
 bNumInterfaces  1
 bConfigurationValue 1
 iConfiguration  0
 bmAttributes 0x80
   (Bus Powered)
 MaxPower  224mA
 Interface Descriptor:
   bLength 9
   bDescriptorType 4
   bInterfaceNumber0
   bAlternateSetting   0
   bNumEndpoints   3
   bInterfaceClass   255 Vendor Specific Class
   bInterfaceSubClass255 Vendor Specific Subclass
   bInterfaceProtocol255 Vendor Specific Protocol
   iInterface  0
   Endpoint Descriptor:
 bLength 7
 bDescriptorType 5
 bEndpointAddress 0x82  EP 2 IN
 bmAttributes2
   Transfer TypeBulk
   Synch Type   None
   Usage Type   Data
 wMaxPacketSize 0x0400  1x 1024 bytes
 bInterval   0
 bMaxBurst  15
   Endpoint Descriptor:
 bLength 7
 bDescriptorType 5
 bEndpointAddress 0x01  EP 1 OUT
 bmAttributes2
   Transfer TypeBulk
   Synch Type   None
   Usage Type   Data
 wMaxPacketSize 0x0400  1x 1024 bytes
 bInterval   0
 bMaxBurst  15
   Endpoint Descriptor:
 bLength 7
 bDescriptorType 5
 bEndpointAddress 0x81  EP 1 IN
 bmAttributes2
   Transfer TypeBulk
   Synch Type   None
   Usage Type   Data
 wMaxPacketSize 0x0400  1x 1024 bytes
 bInterval   0
 bMaxBurst  15
Binary Object Store Descriptor:
   bLength 5
   bDescriptorType15
   wTotalLength   22
   bNumDeviceCaps  2
   USB 2.0 Extension Device Capability:
 bLength

Re: Bug 116821 - Regression: System freeze when unplugging unmounted USB flash drive from powered hub

[Alan Stern]

On Thu, 21 Apr 2016, John wrote:

> jbMacAZ  2016-04-20 22:39:07 UTC
> 
> System hangs requiring hard reboot. System behaves normally with earlier 
> kernels upto 4.6-rc3. Reverting all edits to drivers/USB/ between rc3 
> and rc4 does NOT fix the problem. Problem first appears at 
> 4.6-rc3-next20160412, still present 4.6-rc4-next20160420.
> 
> To reproduce, attach a powered USB hub to USB3 port on an ASUS T100CHI. 
> Attach a USB thumb drive (mine was 32GB Samsung USB3.0) to powered hub. 
> Boot into latest kernel, unmount flash drive. Unplug it and the system 
> freezes. May freeze when plugging in a flash drive, but not as 
> consistent. Simplest workaround is to use older kernel (like -rc3).
> 
> Other details: Ubuntu 16.04 (i686) 2GB Ram, Intel baytrail atom Z3775, 
> generic USB2 4-port hub (BC 1.1 charging) OTG cable micro USB3.0 to USB-A.

Have you tried using git-bisect to find the commit that caused the 
problem?

I recommend not using the -next for tracking down a bug like this.  
That repository is not stable over time.

Alan Stern

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

Re: [PATCH v3] usb: core: hub: hub_port_init lock controller instead of bus

[Mathias Nyman]

On 25.04.2016 15:48, Chris Bainbridge wrote:

The XHCI controller presents two USB buses to the system - one for USB2
and one for USB3. The hub init code (hub_port_init) is reentrant but
only locks one bus per thread, leading to a race condition failure when
two threads attempt to simultaneously initialise a USB2 and USB3 device:

[8.034843] xhci_hcd :00:14.0: Timeout while waiting for setup device 
command
[   13.183701] usb 3-3: device descriptor read/all, error -110

On a test system this failure occurred on 6% of all boots.

The call traces at the point of failure are:

Call Trace:
  [] schedule+0x37/0x90
  [] usb_kill_urb+0x8d/0xd0
  [] ? wake_up_atomic_t+0x30/0x30
  [] usb_start_wait_urb+0xbe/0x150
  [] usb_control_msg+0xbc/0xf0
  [] hub_port_init+0x51e/0xb70
  [] hub_event+0x817/0x1570
  [] process_one_work+0x1ff/0x620
  [] ? process_one_work+0x15f/0x620
  [] worker_thread+0x64/0x4b0
  [] ? rescuer_thread+0x390/0x390
  [] kthread+0x105/0x120
  [] ? kthread_create_on_node+0x200/0x200
  [] ret_from_fork+0x3f/0x70
  [] ? kthread_create_on_node+0x200/0x200

Call Trace:
  [] xhci_setup_device+0x53d/0xa40
  [] xhci_address_device+0xe/0x10
  [] hub_port_init+0x1bf/0xb70
  [] ? trace_hardirqs_on+0xd/0x10
  [] hub_event+0x817/0x1570
  [] process_one_work+0x1ff/0x620
  [] ? process_one_work+0x15f/0x620
  [] worker_thread+0x64/0x4b0
  [] ? rescuer_thread+0x390/0x390
  [] kthread+0x105/0x120
  [] ? kthread_create_on_node+0x200/0x200
  [] ret_from_fork+0x3f/0x70
  [] ? kthread_create_on_node+0x200/0x200

Which results from the two call chains:

hub_port_init
  usb_get_device_descriptor
   usb_get_descriptor
usb_control_msg
 usb_internal_control_msg
  usb_start_wait_urb
   usb_submit_urb / wait_for_completion_timeout / usb_kill_urb

hub_port_init
  hub_set_address
   xhci_address_device
xhci_setup_device

Mathias Nyman explains the current behaviour violates the XHCI spec:

  hub_port_reset() will end up moving the corresponding xhci device slot
  to default state.

  As hub_port_reset() is called several times in hub_port_init() it
  sounds reasonable that we could end up with two threads having their
  xhci device slots in default state at the same time, which according to
  xhci 4.5.3 specs still is a big no no:

  "Note: Software shall not transition more than one Device Slot to the
   Default State at a time"

  So both threads fail at their next task after this.
  One fails to read the descriptor, and the other fails addressing the
  device.

Fix this in hub_port_init by locking the USB controller (instead of an
individual bus) to prevent simultaneous initialisation of both buses.

Fixes: 638139eb95d2 ("usb: hub: allow to process more usb hub events in 
parallel")
Link: https://lkml.org/lkml/2016/2/8/312
Link: https://lkml.org/lkml/2016/2/4/748
Signed-off-by: Chris Bainbridge 
---


Acked-by: Mathias Nyman 

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

Re: [PATCH v6 11/12] usb: core: hub: Notify OTG fsm when A device sets b_hnp_enable

[Roger Quadros]

On 18/04/16 10:08, Peter Chen wrote:
> On Tue, Apr 05, 2016 at 05:05:16PM +0300, Roger Quadros wrote:
>> This is the a_set_b_hnp_enable flag in the OTG state machine
>> diagram and must be set when the A-Host has successfully set
>> the b_hnp_enable feature of the OTG-B-Peripheral attached to it.
>>
>> When this bit changes we kick our OTG FSM to make note of the
>> change and act accordingly.
> 
> Since we have still not added fsm.a_set_b_hnp_en in OTG FSM, and this
> patch set is mainly for DRD, would you please move out this patch from
> this set?

Sure.

cheers,
-roger

> 
>>
>> Signed-off-by: Roger Quadros 
>> ---
>>  drivers/usb/core/hub.c | 17 +++--
>>  1 file changed, 15 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c
>> index 38cc4ba..27e3b4c 100644
>> --- a/drivers/usb/core/hub.c
>> +++ b/drivers/usb/core/hub.c
>> @@ -2273,6 +2273,7 @@ static int usb_enumerate_device_otg(struct usb_device 
>> *udev)
>>  && udev->parent == udev->bus->root_hub) {
>>  struct usb_otg_descriptor   *desc = NULL;
>>  struct usb_bus  *bus = udev->bus;
>> +struct usb_hcd  *hcd = bus_to_hcd(bus);
>>  unsignedport1 = udev->portnum;
>>  
>>  /* descriptor may appear anywhere in config */
>> @@ -2302,6 +2303,9 @@ static int usb_enumerate_device_otg(struct usb_device 
>> *udev)
>>  dev_err(>dev, "can't set HNP mode: %d\n",
>>  err);
>>  bus->b_hnp_enable = 0;
>> +} else {
>> +/* notify OTG fsm about a_set_b_hnp_enable */
>> +usb_otg_kick_fsm(hcd->otg_dev);
>>  }
>>  } else if (desc->bLength == sizeof
>>  (struct usb_otg_descriptor)) {
>> @@ -2312,10 +2316,14 @@ static int usb_enumerate_device_otg(struct 
>> usb_device *udev)
>>  USB_DEVICE_A_ALT_HNP_SUPPORT,
>>  0, NULL, 0,
>>  USB_CTRL_SET_TIMEOUT);
>> -if (err < 0)
>> +if (err < 0) {
>>  dev_err(>dev,
>>  "set a_alt_hnp_support failed: %d\n",
>>  err);
>> +} else {
>> +/* notify OTG fsm about a_set_b_hnp_enable */
>> +usb_otg_kick_fsm(hcd->otg_dev);
>> +}
>>  }
>>  }
>>  #endif
>> @@ -4355,8 +4363,13 @@ hub_port_init(struct usb_hub *hub, struct usb_device 
>> *udev, int port1,
>>   */
>>  if (!hdev->parent) {
>>  delay = HUB_ROOT_RESET_TIME;
>> -if (port1 == hdev->bus->otg_port)
>> +if (port1 == hdev->bus->otg_port) {
>>  hdev->bus->b_hnp_enable = 0;
>> +#ifdef CONFIG_USB_OTG
>> +/* notify OTG fsm about a_set_b_hnp_enable change */
>> +usb_otg_kick_fsm(hcd->otg_dev);
>> +#endif
>> +}
>>  }
>>  
>>  /* Some low speed devices have problems with the quick delay, so */
>> -- 
>> 2.5.0
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
>> the body of a message to majord...@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 04/27/2016 04:59 PM, Catalin Marinas wrote:

On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote:

Grygorii Strashko  writes:

On 04/26/2016 09:17 AM, Felipe Balbi wrote:

Grygorii Strashko  writes:

Now not all DMA paremters configured properly for "xhci-hcd" platform
device which is created manually. For example: dma_pfn_offset, dam_ops
and iommu configuration will not corresponds "dwc3" devices
configuration. As result, this will cause problems like wrong DMA
addresses translation on platforms with LPAE enabled like Keystone 2.

When platform is using DT boot mode the DMA configuration will be
parsed and applied from DT, so, to fix this issue, reuse
of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
from DWC3 device node.


patch is incomplete. You left out non-DT users which might suffer from
the same problem.


Honestly, I don't know how to fix it gracefully for non-DT case.
I can update commit message to mention that this is fix for DT case only.


no, that won't do :-) There are other users for this driver and they are
all "out-of-compliance" when it comes to DMA usage. Apparently, the
desired behavior is to pass correct device to DMA API which the gadget
side is already doing (see below). For the host side, the fix has to be
more involved.

Frankly, I'd prefer that DMA setup could be inherited from parent
device, then it wouldn't really matter and a bunch of this could be
simplified. Some sort of dma_inherit(struct device *dev, struct device
*parent) would go a long way, IMHO.


I would be in favour of a dma_inherit() function as well. We could hack
something up in the arch code (like below) but I would rather prefer an
explicit dma_inherit() call by drivers creating such devices.

diff --git a/arch/arm64/include/asm/dma-mapping.h 
b/arch/arm64/include/asm/dma-mapping.h
index ba437f090a74..ea6fb9b0e8fa 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;

  static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
  {
-   if (dev && dev->archdata.dma_ops)
-   return dev->archdata.dma_ops;
+   while (dev) {
+   if (dev->archdata.dma_ops)
+   return dev->archdata.dma_ops;
+   dev = dev->parent;
+   }

/*
 * We expect no ISA devices, and all other DMA masters are expected to



It's no enough to W/A just dma_ops :(

 dma_inherit()...
FYI: http://www.spinics.net/lists/arm-kernel/msg384012.html
Maybe you'll be able to find the way to make it acceptable.

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Catalin Marinas]

On Wed, Apr 27, 2016 at 08:41:06AM +0300, Felipe Balbi wrote:
> Grygorii Strashko  writes:
> > On 04/26/2016 09:17 AM, Felipe Balbi wrote:
> >> Grygorii Strashko  writes:
> >>> Now not all DMA paremters configured properly for "xhci-hcd" platform
> >>> device which is created manually. For example: dma_pfn_offset, dam_ops
> >>> and iommu configuration will not corresponds "dwc3" devices
> >>> configuration. As result, this will cause problems like wrong DMA
> >>> addresses translation on platforms with LPAE enabled like Keystone 2.
> >>>
> >>> When platform is using DT boot mode the DMA configuration will be
> >>> parsed and applied from DT, so, to fix this issue, reuse
> >>> of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
> >>> from DWC3 device node.
> >> 
> >> patch is incomplete. You left out non-DT users which might suffer from
> >> the same problem.
> >
> > Honestly, I don't know how to fix it gracefully for non-DT case.
> > I can update commit message to mention that this is fix for DT case only.
> 
> no, that won't do :-) There are other users for this driver and they are
> all "out-of-compliance" when it comes to DMA usage. Apparently, the
> desired behavior is to pass correct device to DMA API which the gadget
> side is already doing (see below). For the host side, the fix has to be
> more involved.
> 
> Frankly, I'd prefer that DMA setup could be inherited from parent
> device, then it wouldn't really matter and a bunch of this could be
> simplified. Some sort of dma_inherit(struct device *dev, struct device
> *parent) would go a long way, IMHO.

I would be in favour of a dma_inherit() function as well. We could hack
something up in the arch code (like below) but I would rather prefer an
explicit dma_inherit() call by drivers creating such devices.

diff --git a/arch/arm64/include/asm/dma-mapping.h 
b/arch/arm64/include/asm/dma-mapping.h
index ba437f090a74..ea6fb9b0e8fa 100644
--- a/arch/arm64/include/asm/dma-mapping.h
+++ b/arch/arm64/include/asm/dma-mapping.h
@@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
 
 static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
 {
-   if (dev && dev->archdata.dma_ops)
-   return dev->archdata.dma_ops;
+   while (dev) {
+   if (dev->archdata.dma_ops)
+   return dev->archdata.dma_ops;
+   dev = dev->parent;
+   }
 
/*
 * We expect no ISA devices, and all other DMA masters are expected to

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Arnd Bergmann]

On Wednesday 27 April 2016 14:59:00 Catalin Marinas wrote:
> 
> I would be in favour of a dma_inherit() function as well. We could hack
> something up in the arch code (like below) but I would rather prefer an
> explicit dma_inherit() call by drivers creating such devices.
> 
> diff --git a/arch/arm64/include/asm/dma-mapping.h 
> b/arch/arm64/include/asm/dma-mapping.h
> index ba437f090a74..ea6fb9b0e8fa 100644
> --- a/arch/arm64/include/asm/dma-mapping.h
> +++ b/arch/arm64/include/asm/dma-mapping.h
> @@ -29,8 +29,11 @@ extern struct dma_map_ops dummy_dma_ops;
>  
>  static inline struct dma_map_ops *__generic_dma_ops(struct device *dev)
>  {
> -   if (dev && dev->archdata.dma_ops)
> -   return dev->archdata.dma_ops;
> +   while (dev) {
> +   if (dev->archdata.dma_ops)
> +   return dev->archdata.dma_ops;
> +   dev = dev->parent;
> +   }

I think this would be a very bad idea: we don't want to have random
devices be able to perform DMA just because their parent devices
have been set up that way.

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

Re: [PATCH 0/2] musb fixes for v4.6-rc5

[Bin Liu]

Greg,

On Tue, Apr 26, 2016 at 02:57:05PM -0700, Greg KH wrote:
> On Mon, Apr 18, 2016 at 11:38:23AM -0500, Bin Liu wrote:
> > Hi Greg,
> > 
> > Here are a couple fixes for musb for v4.6-rc5.
> 
> Sorry for the delay, I picked these up with the request for 4.6-rc6,
> right?

Yes, for -rc6.

Thanks,
-Bin.

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

Re: Clear the host side toggle manually when endpoint is 'soft reset'

[Mathias Nyman]

Hi

On 26.04.2016 12:23, Julien D'ascenzio wrote:

Hi,

I see you make this patch in the kernel mainline:

xhci: Clear the ho27082e2654dc148078b0abdfc3c8e5ccbde0ebfa: st side:
toggle manually when endpoint is 'soft reset'

then you revert it:

d0167ad2954ee2d1c70704c454c646086b6653d6: Revert "xhci: Clear the host
side toggle manually when endpoint is 'soft reset'"

This patch correct a problem I have with a camera (IDS US3 uEye CP). I
think, it's a similar problem as:
http://www.spinics.net/lists/linux-usb/msg132221.html

Sometime, This error appear when I transfer a raw video with my camera :

xhci-hcd xhci-hcd.6.auto: URB transfer length is wrong, xHC issue? req.
len = 0, act. len = 4294967288

After that, the camera doesn't work and I must physically disconnect it

You find attached to this email the usb camera trace (wireshark) when
this error occurs.
The transfer seems to start in packet number 21 (URB_CONTROL).
The error seems to occur after packet number 694 when a "CLEAR FEATURE"
is request.

I'd like to know if you still work to a new patch to correct this? I
could help you to make some test with my hardware.

Best regards

Julien D'Ascenzio



I haven't been looking at this for a long time, so with the "xhci: Clear the 
host
side toggle manually when endpoint is soft reset" patch everything works?

Some additional questions.
Are you using uvc or some custom usbfs with libusb implementation?
If libusb is used, is there some extra libusb_clear_halt() used as a "soft 
reset"
instead of actually clearing a halted endpoint?

If the device endpoint is not actually halted then the clear halt request will
clear the data toggle for the connected device, but the host will continue using
the old toggle. -> out of sync.

If endpoint is really halted then host will reset the host side endpoint 
together
with a clear halt request to the device, and both toggles will be cleared.

-Mathias

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

Re: [PATCH] usb: hub: fix panic caused by NULL bos pointer during reset device

[Tony Battersby]

On 04/26/2016 10:53 PM, Du, Changbin wrote:
>> On Tue, Mar 08, 2016 at 05:15:17PM +0800, changbin...@intel.com wrote:
>>> From: "Du, Changbin" 
>>>
>>> This is a reworked patch based on reverted commit d8f00cd685f5 ("usb:
>>> hub: do not clear BOS field during reset device").
>>>
>>> The privious one caused double mem-free if run to re_enumerate label.
>>> New patch title changed to distinguish from old one. And I have tested
>>> it with memory debugging options.
>>>
>>> In function usb_reset_and_verify_device, the old BOS descriptor may
>>> still be used before allocating a new one. (usb_disable_lpm function
>>> uses it under the situation that it fails at usb_disable_link_state.)
>>> So we cannot set the udev->bos to NULL before that, just keep what it
>>> was. It will be overwrite when allocating a new one.
>>>
>>> How to reproduce:
>>> 1. connect one usb3 hub to xhci port.
>>> 2. connect several lpm-capable super-speed usb disk to the hub.
>>> 3. copy big files to the usb disks.
>>> 4. disconnect the hub and repeat step 1-4.
>>>
>>> Crash log:
>>> BUG: unable to handle kernel NULL pointer dereference at
>>> 0010
>>> IP: [] usb_enable_link_state+0x2d/0x2f0
>>> Call Trace:
>>> [] ? usb_set_lpm_timeout+0x12b/0x140
>>> [] usb_enable_lpm+0x81/0xa0
>>> [] usb_disable_lpm+0xa8/0xc0
>>> [] usb_unlocked_disable_lpm+0x2c/0x50
>>> [] usb_reset_and_verify_device+0xc3/0x710
>>> [] ? usb_sg_wait+0x13d/0x190
>>> [] usb_reset_device+0x133/0x280
>>> [] usb_stor_port_reset+0x61/0x70
>>> [] usb_stor_invoke_transport+0x88/0x520
>>>
>>> Signed-off-by: Du, Changbin 
>>> ---
>>>  drivers/usb/core/hub.c | 14 +-
>>>  1 file changed, 9 insertions(+), 5 deletions(-)
>> Is this patch still needed?  I thought we had some other fix in this
>> area...
>>
>> confused,
>>
>> greg k-h
>>
> Hi, Greg k-h,
> Sorry for it confused you. This patch still need. This is same fix with
> previous commit d8f00cd685f5 ("usb: hub: do not clear BOS field
> during reset device"). But d8f00cd685f5 is buggy and reverted. This
> new patch should be the final fix.
>
> Best Regards,
> Du, Changbin
>

I think Greg is referring to commit 464ad8c43a9e ("usb: core : hub: Fix
BOS 'NULL pointer' kernel panic"), which has already been applied
upstream.  It looks to me like that patch might have fixed the same
problem in a different way, in which case Changbin's patch is not
needed.  But I haven't been involved in developing or testing that
patch, so I can't say for sure.  At the very least, 464ad8c43a9e
conflicts with Changbin's patch.

Changbin, can you take a look at 464ad8c43a9e and see if that fixes the
same problem that your patch did?

Thanks,
Tony Battersby

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

RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core

[Jun Li]

Hi,

> -Original Message-
> From: Jun Li [mailto:jun...@nxp.com]
> Sent: Wednesday, April 27, 2016 8:50 PM
> To: Roger Quadros ; st...@rowland.harvard.edu;
> ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com
> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
> Subject: RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core
> 
> Hi
> > -Original Message-
> > From: Roger Quadros [mailto:rog...@ti.com]
> > Sent: Wednesday, April 27, 2016 7:12 PM
> > To: Jun Li ; st...@rowland.harvard.edu;
> > ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com
> > Cc: dan.j.willi...@intel.com; jun...@freescale.com;
> > mathias.ny...@linux.intel.com; t...@atomide.com;
> > joao.pi...@synopsys.com; abres...@chromium.org; r.bald...@samsung.com;
> > linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org;
> > linux-o...@vger.kernel.org
> > Subject: Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core
> >
> > On 27/04/16 14:00, Roger Quadros wrote:
> > > On 27/04/16 13:16, Jun Li wrote:
> > >> Hi
> > >>
> > >>>
> > >>> +
> > >>> +static struct otg_hcd_ops otg_hcd_intf = {
> > >>> +   .add = usb_add_hcd,
> > >>> +   .remove = usb_remove_hcd,
> > >>> +   .usb_bus_start_enum = usb_bus_start_enum,
> > >>
> > >> Build break if CONFIG_USB_OTG is not enabled:
> > >>
> > >> drivers/built-in.o:(.data+0x1db30): undefined reference to
> > `usb_bus_start_enum'
> > >> Makefile:948: recipe for target 'vmlinux' failed
> > >> make: *** [vmlinux] Error 1
> >
> > I couldn't get this error. Could you please send me your .config? Thanks.
> 
> imx_v6_v7_defconfig

My bad, no problem now.

Li Jun

> 
> >
> > cheers,
> > -roger
> > >
> > > Thanks. Will fix it.
> > >
> > > cheers,
> > > -roger
> > >
> > >>
> > >>> +   .usb_control_msg = usb_control_msg,
> > >>> +   .usb_hub_find_child = usb_hub_find_child, };
> > >>> +
> > >>
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > > in the body of a message to majord...@vger.kernel.org More majordomo
> > > info at  http://vger.kernel.org/majordomo-info.html
> > >
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 06:25:16PM +0800, Jisheng Zhang wrote:
> On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:
> > On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:

> > > > +   vbus = devm_regulator_get(>dev, "vbus");  

> > > devm_regulator_get_optional() ??  

> > Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> > this to be just a normal regulator_get().

> Per spec no. But the vbus may be transparent to SW on some platforms, so I
> think devm_regulator_get_optional() is better.

Really, no.  If it's physically there the software needs to be written
as such.  Please see the documentation and list archives for extensive
discussion on this topic.

> > This is all completely broken unless the supply is optional.

> The supply is optional.

To repeat a supply is only optional if it might be physically absent.


signature.asc
Description: PGP signature

RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core

[Jun Li]

Hi
> -Original Message-
> From: Roger Quadros [mailto:rog...@ti.com]
> Sent: Wednesday, April 27, 2016 7:12 PM
> To: Jun Li ; st...@rowland.harvard.edu; ba...@kernel.org;
> gre...@linuxfoundation.org; peter.c...@freescale.com
> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
> abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
> Subject: Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core
> 
> On 27/04/16 14:00, Roger Quadros wrote:
> > On 27/04/16 13:16, Jun Li wrote:
> >> Hi
> >>
> >>>
> >>> +
> >>> +static struct otg_hcd_ops otg_hcd_intf = {
> >>> + .add = usb_add_hcd,
> >>> + .remove = usb_remove_hcd,
> >>> + .usb_bus_start_enum = usb_bus_start_enum,
> >>
> >> Build break if CONFIG_USB_OTG is not enabled:
> >>
> >> drivers/built-in.o:(.data+0x1db30): undefined reference to
> `usb_bus_start_enum'
> >> Makefile:948: recipe for target 'vmlinux' failed
> >> make: *** [vmlinux] Error 1
> 
> I couldn't get this error. Could you please send me your .config? Thanks.

imx_v6_v7_defconfig

> 
> cheers,
> -roger
> >
> > Thanks. Will fix it.
> >
> > cheers,
> > -roger
> >
> >>
> >>> + .usb_control_msg = usb_control_msg,
> >>> + .usb_hub_find_child = usb_hub_find_child, };
> >>> +
> >>
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb"
> > in the body of a message to majord...@vger.kernel.org More majordomo
> > info at  http://vger.kernel.org/majordomo-info.html
> >
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 04/10] regulator: fixed: add support for ACPI interface

[Mark Brown]

On Wed, Apr 27, 2016 at 09:54:10AM +0800, Lu Baolu wrote:

> Please refer to Documentation/acpi/gpio-properties.txt.

That's not visibly what your driver is doing, that is also recommending
using a static name which is what I'm asking for.

> > Why does the device care?It's requesting the GPIO in
> > its own context and it's only requesting one GPIO, with DT we're just
> > always calling the GPIO "gpio" which works fine.

> This driver is not bound to an ACPI device node directly. It's a child
> of a mfd device, which is corresponding to a real ACPI device node.

If it's the child of a MFD it's got an ACPI device, the ACPI device is
the parent.  It should use the parent device or the parent should map
the GPIO through to the child as many other MFDs do, the whole concept
of a MFD is a Linux internal implementation detail.


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Sergei Shtylyov]

Hello.

On 4/27/2016 11:29 AM, changbin...@intel.com wrote:


From: "Du, Changbin" 

The dwc3 controller can't generate a disconnect event after it is
stopped. Thus gadget dissconnect callback is not invoked when do
soft dissconnect. Call dissconnect here to workaround this issue.


   "Disconnect" everywhere.


Note, most time we still see disconnect be called that because
it is invoked by dwc3_gadget_reset_interrupt(). But if we
disconnect cable once pullup disabled quickly, issue can be
observed.

Signed-off-by: Du, Changbin 
---
 drivers/usb/dwc3/gadget.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8e4a1b1..cd73187 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c

[...]

@@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int 
is_on)
is_on = !!is_on;

spin_lock_irqsave(>lock, flags);
+   /**
+* WORKAROUND: The dwc3 controller can't generate a disconnect
+* event after it is stopped. Thus gadget dissconnect callback
+* is not invoked when do soft dissconnect. Call dissconnect
+* here to workaround this issue.


   "Disconnect" everywhere.


+* Note, most time we still see disconnect be called that because


   I couldn't parse that.


+* it is invoked by dwc3_gadget_reset_interrupt(). But if we
+* disconnect cable once pullup disabled quickly, issue can be
+* observed.
+*/
+   if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
+   dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");


   Disconnect.


+   dwc3_disconnect_gadget(dwc);
+   dwc->gadget.speed = USB_SPEED_UNKNOWN;
+   }
ret = dwc3_gadget_run_stop(dwc, is_on, false);
spin_unlock_irqrestore(>lock, flags);


[...]

MBR, Sergei

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

Re: [PATCH] usb: dwc3: host: inherit dma configuration from parent dev

[Grygorii Strashko]

On 04/27/2016 08:41 AM, Felipe Balbi wrote:
> 
> Hi,
> 
> Grygorii Strashko  writes:
>> On 04/26/2016 09:17 AM, Felipe Balbi wrote:
>>>
>>> Hi,
>>>
>>> Grygorii Strashko  writes:
 Now not all DMA paremters configured properly for "xhci-hcd" platform
 device which is created manually. For example: dma_pfn_offset, dam_ops
 and iommu configuration will not corresponds "dwc3" devices
 configuration. As result, this will cause problems like wrong DMA
 addresses translation on platforms with LPAE enabled like Keystone 2.

 When platform is using DT boot mode the DMA configuration will be
 parsed and applied from DT, so, to fix this issue, reuse
 of_dma_configure() API and retrieve DMA configuartion for "xhci-hcd"
 from DWC3 device node.
>>>
>>> patch is incomplete. You left out non-DT users which might suffer from
>>> the same problem.
>>>
>>
>> Honestly, I don't know how to fix it gracefully for non-DT case.
>> I can update commit message to mention that this is fix for DT case only.
> 
> no, that won't do :-) There are other users for this driver and they are
> all "out-of-compliance" when it comes to DMA usage. Apparently, the
> desired behavior is to pass correct device to DMA API which the gadget
> side is already doing (see below). For the host side, the fix has to be
> more involved.
> 
> Frankly, I'd prefer that DMA setup could be inherited from parent
> device, then it wouldn't really matter and a bunch of this could be
> simplified. Some sort of dma_inherit(struct device *dev, struct device
> *parent) would go a long way, IMHO.
> 
> 8< cut here 
> commit 2725d6f974c4c268ae5fb746f8e3b33b76135aa8
> Author: Felipe Balbi 
> Date:   Tue Apr 19 16:18:12 2016 +0300
> 
>  usb: dwc3: use parent device for DMA
>  
>  our parent device is the one which was initialized
>  by either PCI or DeviceTree and that's the one which
>  is configured properly for DMA access.
>  
>  Instead of copying DMA bits from parent to child,
>  let's just rely on parent device for the entire DMA
>  API.

1) Patch I've sent fixes "xhci-hcd" platform device and not dwc3 core.
On TI boards dwc3 core devices are created from DT and so, I do not see
any problems with dwc3 core. Problem is with xhci.

2) there is minimum one dtsi file where dwc3 ("snps,dwc3") does not have parent 
device
ls1021a.dtsi (and 5 dtsi in arm64 folder)



>  
>  Signed-off-by: Felipe Balbi 
> 
> diff --git a/drivers/usb/dwc3/core.c b/drivers/usb/dwc3/core.c
> index c050a88c16d4..09e4ff71a50f 100644
> --- a/drivers/usb/dwc3/core.c
> +++ b/drivers/usb/dwc3/core.c
> @@ -180,7 +180,7 @@ static void dwc3_frame_length_adjustment(struct dwc3 
> *dwc, u32 fladj)
>   static void dwc3_free_one_event_buffer(struct dwc3 *dwc,
>   struct dwc3_event_buffer *evt)
>   {
> - dma_free_coherent(dwc->dev, evt->length, evt->buf, evt->dma);
> + dma_free_coherent(dwc->dev->parent, evt->length, evt->buf, evt->dma);
>   }

[...]

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

Re: [PATCH] usb: dwc3: omap: get rid of dma_status

[Roger Quadros]

Felipe,

On 11/04/16 17:18, Roger Quadros wrote:
> dma_status bit flag is set but never really used
> so get rid of it.
> 
> Reported-by: Felipe Balbi 
> Signed-off-by: Roger Quadros 

Gentle ping on this one for -next. Thanks.

cheers,
-roger

> ---
>  drivers/usb/dwc3/dwc3-omap.c | 6 --
>  1 file changed, 6 deletions(-)
> 
> diff --git a/drivers/usb/dwc3/dwc3-omap.c b/drivers/usb/dwc3/dwc3-omap.c
> index 55da2c7..890b8a6 100644
> --- a/drivers/usb/dwc3/dwc3-omap.c
> +++ b/drivers/usb/dwc3/dwc3-omap.c
> @@ -126,8 +126,6 @@ struct dwc3_omap {
>   u32 debug_offset;
>   u32 irq0_offset;
>  
> - u32 dma_status:1;
> -
>   struct extcon_dev   *edev;
>   struct notifier_block   vbus_nb;
>   struct notifier_block   id_nb;
> @@ -277,9 +275,6 @@ static irqreturn_t dwc3_omap_interrupt(int irq, void 
> *_omap)
>  
>   reg = dwc3_omap_read_irqmisc_status(omap);
>  
> - if (reg & USBOTGSS_IRQMISC_DMADISABLECLR)
> - omap->dma_status = false;
> -
>   dwc3_omap_write_irqmisc_status(omap, reg);
>  
>   reg = dwc3_omap_read_irq0_status(omap);
> @@ -504,7 +499,6 @@ static int dwc3_omap_probe(struct platform_device *pdev)
>  
>   /* check the DMA Status */
>   reg = dwc3_omap_readl(omap->base, USBOTGSS_SYSCONFIG);
> - omap->dma_status = !!(reg & USBOTGSS_SYSCONFIG_DMADISABLE);
>  
>   ret = devm_request_irq(dev, omap->irq, dwc3_omap_interrupt, 0,
>   "dwc3-omap", omap);
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Du, Changbin]

Hi, Balbi,

The step to reproduce this issue is:
1) connect device to a host and wait its enumeration.
2) trigger software disconnect by calling function
usb_gadget_disconnect(), which finally call
   dwc3_gadget_pullup(false). Do not reconnect device
  (I mean no enumeration go on, keep bit Run/Stop 0.).

At here, gadget driver's disconnect callback should be
Called, right? We has been disconnected. But no, as
You said " not generating disconnect IRQ after you
drop Run/Stop is expected".

And I am testing on an Android device, Android only
use dwc3_gadget_pullup(false) to issue a soft disconnection.
This confused user that the UI still show usb as connected
State, caused by missing a disconnect event.

> Hi,
> 
> changbin...@intel.com writes:
> > From: "Du, Changbin" 
> >
> > The dwc3 controller can't generate a disconnect event after it is
> > stopped. Thus gadget dissconnect callback is not invoked when do
> 
> not generating disconnect IRQ after you drop Run/Stop is expected.
> 
> > soft dissconnect. Call dissconnect here to workaround this issue.
> 
> I'm rather sure this is a bug elsewhere. How do you trigger this ?
> 
> > Note, most time we still see disconnect be called that because
> > it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > disconnect cable once pullup disabled quickly, issue can be
> > observed.
> 
> I can't understand what you're trying to say with this.
> 
> > Signed-off-by: Du, Changbin 
> > ---
> >  drivers/usb/dwc3/gadget.c | 33 -
> >  1 file changed, 24 insertions(+), 9 deletions(-)
> >
> > diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> > index 8e4a1b1..cd73187 100644
> > --- a/drivers/usb/dwc3/gadget.c
> > +++ b/drivers/usb/dwc3/gadget.c
> > @@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3
> *dwc, int is_on, int suspend)
> > return 0;
> >  }
> >
> > +static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> > +{
> > +   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> > +   spin_unlock(>lock);
> > +   dwc->gadget_driver->disconnect(>gadget);
> > +   spin_lock(>lock);
> > +   }
> > +}
> > +
> >  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
> >  {
> > struct dwc3 *dwc = gadget_to_dwc(g);
> > @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget
> *g, int is_on)
> > is_on = !!is_on;
> >
> > spin_lock_irqsave(>lock, flags);
> > +   /**
> > +* WORKAROUND: The dwc3 controller can't generate a disconnect
> > +* event after it is stopped. Thus gadget dissconnect callback
> > +* is not invoked when do soft dissconnect. Call dissconnect
> > +* here to workaround this issue.
> > +* Note, most time we still see disconnect be called that because
> > +* it is invoked by dwc3_gadget_reset_interrupt(). But if we
> > +* disconnect cable once pullup disabled quickly, issue can be
> > +* observed.
> > +*/
> > +   if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> > +   dev_dbg(dwc->dev, "fake dissconnect event on pullup
> off\n");
> > +   dwc3_disconnect_gadget(dwc);
> > +   dwc->gadget.speed = USB_SPEED_UNKNOWN;
> > +   }
> 
> this is *really* wrong. You shouldn't be calling pullup directly. This
> patch looks wrong and you didn't even explain how to trigger this
> problem; when does the problem happen ?
> 
> Gadget load/unload does the right thing here, so that can't be the
> case. We also do the right thing on soft_connect sysfs file:
> 
> static ssize_t usb_udc_softconn_store(struct device *dev,
>   struct device_attribute *attr, const char *buf, size_t n)
> {
>   struct usb_udc  *udc = container_of(dev, struct usb_udc,
> dev);
> 
>   if (!udc->driver) {
>   dev_err(dev, "soft-connect without a gadget driver\n");
>   return -EOPNOTSUPP;
>   }
> 
>   if (sysfs_streq(buf, "connect")) {
>   usb_gadget_udc_start(udc);
>   usb_gadget_connect(udc->gadget);
>   } else if (sysfs_streq(buf, "disconnect")) {
>   usb_gadget_disconnect(udc->gadget);
>   udc->driver->disconnect(udc->gadget);
>   usb_gadget_udc_stop(udc);
>   } else {
>   dev_err(dev, "unsupported command '%s'\n", buf);
>   return -EINVAL;
>   }
> 
>   return n;
> }
> static DEVICE_ATTR(soft_connect, S_IWUSR, NULL,
> usb_udc_softconn_store);
> 
> So really, what _is_ the problem ?
> 
> --
> balbi
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core

[Roger Quadros]

On 26/04/16 03:07, Jun Li wrote:
> Hi
> 
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Monday, April 25, 2016 10:04 PM
>> To: Jun Li ; st...@rowland.harvard.edu; ba...@kernel.org;
>> gre...@linuxfoundation.org; peter.c...@freescale.com
>> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>> abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
>> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
>> Subject: Re: [PATCH v6 09/12] usb: gadget: udc: adapt to OTG core
>>
>> Hi,
>>
>> On 21/04/16 09:38, Jun Li wrote:
>>> Hi,
>>>
>>> ...

  /**
 + * usb_gadget_start - start the usb gadget controller and connect to
 +bus
 + * @gadget: the gadget device to start
 + *
 + * This is external API for use by OTG core.
 + *
 + * Start the usb device controller and connect to bus (enable pull).
 + */
 +static int usb_gadget_start(struct usb_gadget *gadget) {
 +  int ret;
 +  struct usb_udc *udc = NULL;
 +
 +  dev_dbg(>dev, "%s\n", __func__);
 +  mutex_lock(_lock);
 +  list_for_each_entry(udc, _list, list)
 +  if (udc->gadget == gadget)
 +  goto found;
 +
 +  dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
 +  __func__);
 +  mutex_unlock(_lock);
 +  return -EINVAL;
 +
 +found:
 +  ret = usb_gadget_udc_start(udc);
 +  if (ret)
 +  dev_err(>dev, "USB Device Controller didn't start: %d\n",
 +  ret);
 +  else
 +  usb_udc_connect_control(udc);
>>>
>>> For drd, it's fine, but for real otg, gadget connect should be done by
>>> loc_conn() instead of gadget start.
>>
>> It is upto the OTG state machine to call gadget_start() when it needs to
>> connect to the bus (i.e. loc_conn()). I see no point in calling gadget
>> start before.
>>
>> Do you see any issue in doing so?
> 
> This is what OTG state machine does:
> case OTG_STATE_B_PERIPHERAL:
>  otg_chrg_vbus(otg, 0);
>  otg_loc_sof(otg, 0);
>  otg_set_protocol(fsm, PROTO_GADGET);
>  otg_loc_conn(otg, 1);
>  break;
> 
> You intend to abstract something common in this api when start gadget,
> which should be called by otg_set_protocol(fsm, PROTO_GADGET); and
> drd_set_protocol(fsm, PROTO_GADGET); right?
> 
> So you may move usb_udc_connect_control(IMO usb_gadget_connect()
> is better)out of usb_gadget_start(), then for drd:
> 
> case OTG_STATE_B_PERIPHERAL:
>  drd_set_protocol(fsm, PROTO_GADGET);
>  otg_drv_vbus(otg, 0);
>  usb_gadget_connect();

OK. I understand now. I'll implement your suggestion. Thanks.

cheers,
-roger

>>>
 +
 +  mutex_unlock(_lock);
 +
 +  return ret;
 +}
 +
 +/**
 + * usb_gadget_stop - disconnect from bus and stop the usb gadget
 + * @gadget: The gadget device we want to stop
 + *
 + * This is external API for use by OTG core.
 + *
 + * Disconnect from the bus (disable pull) and stop the
 + * gadget controller.
 + */
 +static int usb_gadget_stop(struct usb_gadget *gadget) {
 +  struct usb_udc *udc = NULL;
 +
 +  dev_dbg(>dev, "%s\n", __func__);
 +  mutex_lock(_lock);
 +  list_for_each_entry(udc, _list, list)
 +  if (udc->gadget == gadget)
 +  goto found;
 +
 +  dev_err(gadget->dev.parent, "%s: gadget not registered.\n",
 +  __func__);
 +  mutex_unlock(_lock);
 +  return -EINVAL;
 +
 +found:
 +  usb_gadget_disconnect(udc->gadget);
>>>
>>> Likewise, gadget disconnect also should be done by loc_conn() instead
>>> of gadget stop.
>>>
 +  udc->driver->disconnect(udc->gadget);
 +  usb_gadget_udc_stop(udc);
 +  mutex_unlock(_lock);
 +
 +  return 0;
 +}
 +
>>>
>>> Li Jun
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Roger Quadros]

Hi Jun,

On 26/04/16 05:07, Jun Li wrote:
> Hi Roger
> 
>> -Original Message-
>> From: Roger Quadros [mailto:rog...@ti.com]
>> Sent: Tuesday, April 05, 2016 10:05 PM
>> To: st...@rowland.harvard.edu; ba...@kernel.org;
>> gre...@linuxfoundation.org; peter.c...@freescale.com
>> Cc: dan.j.willi...@intel.com; jun...@freescale.com;
>> mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
>> abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
>> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org; Roger Quadros
>> 
>> Subject: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>
>> It provides APIs for the following tasks
>>
>> - Registering an OTG/dual-role capable controller
>> - Registering Host and Gadget controllers to OTG core
>> - Providing inputs to and kicking the OTG state machine
>>
>> Provide a dual-role device (DRD) state machine.
>> DRD mode is a reduced functionality OTG mode. In this mode we don't
>> support SRP, HNP and dynamic role-swap.
>>
>> In DRD operation, the controller mode (Host or Peripheral) is decided
>> based on the ID pin status. Once a cable plug (Type-A or Type-B) is
>> attached the controller selects the state and doesn't change till the
>> cable in unplugged and a different cable type is inserted.
>>
>> As we don't need most of the complex OTG states and OTG timers we
>> implement a lean DRD state machine in usb-otg.c.
>> The DRD state machine is only interested in 2 hardware inputs 'id' and
>> 'b_sess_vld'.
>>
>> Signed-off-by: Roger Quadros 
>> ---
> 
> ...
> 
>> +/**
>> + * Register pending host/gadget and remove entry from wait list  */
>> +static void usb_otg_flush_wait(struct device *otg_dev) {
>> +struct otg_wait_data *wait;
>> +struct otg_hcd *host;
>> +struct otg_gcd *gadget;
>> +
>> +mutex_lock(_list_mutex);
>> +
>> +wait = usb_otg_get_wait(otg_dev);
>> +if (!wait)
>> +goto done;
>> +
>> +dev_dbg(otg_dev, "otg: registering pending host/gadget\n");
>> +gadget = >gcd;
>> +if (gadget)
> 
> If (gadget->gadget)

good catch :)
I'll probably rename the local variables
host to hcd
gadget to gcd.

> 
>> +usb_otg_register_gadget(gadget->gadget, gadget->ops);
>> +
>> +host = >primary_hcd;
>> +if (host->hcd)
>> +usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> + host->ops);
>> +
>> +host = >shared_hcd;
>> +if (host->hcd)
>> +usb_otg_register_hcd(host->hcd, host->irqnum, host->irqflags,
>> + host->ops);
>> +
>> +list_del(>list);
>> +kfree(wait);
>> +
>> +done:
>> +mutex_unlock(_list_mutex);
>> +}
>> +
>> +/**
>> + * Check if the OTG device is in our OTG list and return
>> + * usb_otg data, else NULL.
>> + *
>> + * otg_list_mutex must be held.
>> + */
>> +static struct usb_otg *usb_otg_get_data(struct device *otg_dev) {
>> +struct usb_otg *otg;
>> +
>> +if (!otg_dev)
>> +return NULL;
>> +
>> +list_for_each_entry(otg, _list, list) {
>> +if (otg->dev == otg_dev)
>> +return otg;
>> +}
>> +
>> +return NULL;
>> +}
> 
> Could you export it to be a public API, we may need access usb_otg
> in common host driver for handling of enumeration of otg test device.

We can always do that later. As of now nobody is using it so let's keep it 
private.
> 
> ...
> 
>> +/**
>> + * Called when entering a DRD state.
>> + * fsm->lock must be held.
>> + */
>> +static void drd_set_state(struct otg_fsm *fsm, enum usb_otg_state
>> +new_state) {
>> +struct usb_otg *otg = container_of(fsm, struct usb_otg, fsm);
>> +
>> +if (otg->state == new_state)
>> +return;
>> +
>> +fsm->state_changed = 1;
>> +dev_dbg(otg->dev, "otg: set state: %s\n",
>> +usb_otg_state_string(new_state));
>> +switch (new_state) {
>> +case OTG_STATE_B_IDLE:
>> +drd_set_protocol(fsm, PROTO_UNDEF);
>> +otg_drv_vbus(otg, 0);
>> +break;
>> +case OTG_STATE_B_PERIPHERAL:
>> +drd_set_protocol(fsm, PROTO_GADGET);
>> +otg_drv_vbus(otg, 0);
>> +break;
>> +case OTG_STATE_A_HOST:
>> +drd_set_protocol(fsm, PROTO_HOST);
>> +otg_drv_vbus(otg, 1);
>> +break;
>> +case OTG_STATE_UNDEFINED:
>> +case OTG_STATE_B_SRP_INIT:
>> +case OTG_STATE_B_WAIT_ACON:
>> +case OTG_STATE_B_HOST:
>> +case OTG_STATE_A_IDLE:
>> +case OTG_STATE_A_WAIT_VRISE:
>> +case OTG_STATE_A_WAIT_BCON:
>> +case OTG_STATE_A_SUSPEND:
>> +case OTG_STATE_A_PERIPHERAL:
>> +case OTG_STATE_A_WAIT_VFALL:
>> +case OTG_STATE_A_VBUS_ERR:
> 
> Remove above unused states.

OK.
> 
>> +default:
>> +dev_warn(otg->dev, "%s: otg: invalid state: %s\n",
>> + __func__, usb_otg_state_string(new_state));
>> +break;
>> +}
>> +

Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core

[Roger Quadros]

On 27/04/16 14:00, Roger Quadros wrote:
> On 27/04/16 13:16, Jun Li wrote:
>> Hi
>>
>>>
>>> +
>>> +static struct otg_hcd_ops otg_hcd_intf = {
>>> +   .add = usb_add_hcd,
>>> +   .remove = usb_remove_hcd,
>>> +   .usb_bus_start_enum = usb_bus_start_enum,
>>
>> Build break if CONFIG_USB_OTG is not enabled:
>>
>> drivers/built-in.o:(.data+0x1db30): undefined reference to 
>> `usb_bus_start_enum'
>> Makefile:948: recipe for target 'vmlinux' failed
>> make: *** [vmlinux] Error 1

I couldn't get this error. Could you please send me your .config? Thanks.

cheers,
-roger
> 
> Thanks. Will fix it.
> 
> cheers,
> -roger
> 
>>  
>>> +   .usb_control_msg = usb_control_msg,
>>> +   .usb_hub_find_child = usb_hub_find_child, };
>>> +
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v6 08/12] usb: hcd: Adapt to OTG core

[Roger Quadros]

On 27/04/16 13:16, Jun Li wrote:
> Hi
> 
>>
>> +
>> +static struct otg_hcd_ops otg_hcd_intf = {
>> +.add = usb_add_hcd,
>> +.remove = usb_remove_hcd,
>> +.usb_bus_start_enum = usb_bus_start_enum,
> 
> Build break if CONFIG_USB_OTG is not enabled:
> 
> drivers/built-in.o:(.data+0x1db30): undefined reference to 
> `usb_bus_start_enum'
> Makefile:948: recipe for target 'vmlinux' failed
> make: *** [vmlinux] Error 1

Thanks. Will fix it.

cheers,
-roger

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

Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

[Roger Quadros]

Hi,

On 27/04/16 06:15, Peter Chen wrote:
> On Tue, Apr 26, 2016 at 04:21:07PM +0800, Peter Chen wrote:
>> On Tue, Apr 26, 2016 at 07:00:22AM +, Jun Li wrote:
>>> Hi
>>>
 -Original Message-
 From: Peter Chen [mailto:hzpeterc...@gmail.com]
 Sent: Tuesday, April 26, 2016 2:28 PM
 To: Jun Li 
 Cc: Roger Quadros ; st...@rowland.harvard.edu;
 ba...@kernel.org; gre...@linuxfoundation.org; peter.c...@freescale.com;
 dan.j.willi...@intel.com; jun...@freescale.com;
 mathias.ny...@linux.intel.com; t...@atomide.com; joao.pi...@synopsys.com;
 abres...@chromium.org; r.bald...@samsung.com; linux-usb@vger.kernel.org;
 linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
 Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core

 On Tue, Apr 26, 2016 at 05:11:36AM +, Jun Li wrote:
> Hi
>
>> -Original Message-
>> From: Peter Chen [mailto:hzpeterc...@gmail.com]
>> Sent: Tuesday, April 26, 2016 11:47 AM
>> To: Jun Li 
>> Cc: Roger Quadros ; st...@rowland.harvard.edu;
>> ba...@kernel.org; gre...@linuxfoundation.org;
>> peter.c...@freescale.com; dan.j.willi...@intel.com;
>> jun...@freescale.com; mathias.ny...@linux.intel.com;
>> t...@atomide.com; joao.pi...@synopsys.com; abres...@chromium.org;
>> r.bald...@samsung.com; linux-usb@vger.kernel.org;
>> linux-ker...@vger.kernel.org; linux-o...@vger.kernel.org
>> Subject: Re: [PATCH v6 07/12] usb: otg: add OTG/dual-role core
>>
>> On Tue, Apr 26, 2016 at 02:07:56AM +, Jun Li wrote:
 +struct usb_otg *usb_otg_register(struct device *dev,
 +   struct usb_otg_config *config) {
 +  struct usb_otg *otg;
 +  struct otg_wait_data *wait;
 +  int ret = 0;
 +
 +  if (!dev || !config || !config->fsm_ops)
 +  return ERR_PTR(-EINVAL);
 +
 +  /* already in list? */
 +  mutex_lock(_list_mutex);
 +  if (usb_otg_get_data(dev)) {
 +  dev_err(dev, "otg: %s: device already in otg list\n",
 +  __func__);
 +  ret = -EINVAL;
 +  goto unlock;
 +  }
 +
 +  /* allocate and add to list */
 +  otg = kzalloc(sizeof(*otg), GFP_KERNEL);
 +  if (!otg) {
 +  ret = -ENOMEM;
 +  goto unlock;
 +  }
 +
 +  otg->dev = dev;
 +  otg->caps = config->otg_caps;
 +
 +  if ((otg->caps->hnp_support || otg->caps->srp_support ||
 +   otg->caps->adp_support) && !config->otg_work)
 +  dev_info(dev, "otg: limiting to dual-role\n");
>>>
>>> dev_err, this should be an error.
>>
>> The condition may be wrong, but it is an information to show that
>> current OTG is dual-role.
>
> This should not happen in any correct design, I even doubt if we
> should try to continue by "downgrade" it to be duel role, currently
> the only example user is dual role, so doing like this can't be tested
> by real case, this downgrade is not so easy like we image, at least
> for chipidea otg driver, simply replace a queue worker may not work,
> as we have much more difference between the 2 configs.
>

 Would you show more why chipidea can't work just replace the work item,
 and see if anything we still can improve for this framework?
>>>
>>> In real OTG, we need enable AVV irq,
>>
>> Enable and Handling AVV is platform stuff. In this framework, we are
>> focus on how otg device manages host and gadget together, and the state
>> machine when the related otg event occurs.
>>
>>> but for duel role, nobody care/handle,
>>> there are much more resource required for OTG: timers, hnp polling,
>>> otg test device handling... 
>>
>> They are common things for fully OTG fsm, you can move them
>> to common code (In fact, hnp polling handling is already common code).
>>
>>>
>>> with current design, chipidea driver can support real OTG with its own
>>> queue worker, or DRD with Roger's drd work item if config is correct.
>>>
>>> But improve something to work on a *wrong* config will make it complicated
>>> and does not make much sense IMO.
>>>
>>
>> What does above "config" you mean?
>>
>> If the configure is fully OTG, you can choose different state machine,
>> eg otg_statemachine, if you find it is hard for chipidea to use this
>> framework, just list the reason, and see if we can improve.
>>
> 
> Roger, after discussing with Jun off line, we think usb_otg_register
> should return -ENOTSUPP if platform is OTG capabilities (HNP || SRP ||
> ADP), since this patch set does not cover fully otg features, the users

But this series isn't preventing full otg 

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

Hi,

Mark Brown  writes:
> On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
>> Mark Brown  writes:
>
>> > this to be just a normal regulator_get().
>
>> jokes aside, this regulator is optional because not all platforms
>> require a SW controlled regulator, no ? Will normal regulator_get() give
>> us a dummy regulator in case it's not listed in DT/ACPI ?
>
> Yes we do that, but even regulators that are not software controlled

okay, good.

> should really be described anyway since it's a much simpler rule for

okay, we'll wait until all vendors update their ACPI tables ;-)

> people to understand, it ensures that we can just scale up on systems
> where there does happen to be software control and it makes all the
> resulting code much simpler and hence less error prone if we're not
> randomly ignoring some errors.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 01:25:27PM +0300, Felipe Balbi wrote:
> Mark Brown  writes:

> > this to be just a normal regulator_get().

> jokes aside, this regulator is optional because not all platforms
> require a SW controlled regulator, no ? Will normal regulator_get() give
> us a dummy regulator in case it's not listed in DT/ACPI ?

Yes we do that, but even regulators that are not software controlled
should really be described anyway since it's a much simpler rule for
people to understand, it ensures that we can just scale up on systems
where there does happen to be software control and it makes all the
resulting code much simpler and hence less error prone if we're not
randomly ignoring some errors.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Jisheng Zhang]

Dear Mark,

On Wed, 27 Apr 2016 10:57:39 +0100 Mark Brown wrote:

> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> > Jisheng Zhang  writes:  
> 
> > > + vbus = devm_regulator_get(>dev, "vbus");  
> 
> > devm_regulator_get_optional() ??  
> 
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect
> this to be just a normal regulator_get().

Per spec no. But the vbus may be transparent to SW on some platforms, so I
think devm_regulator_get_optional() is better.

> 
> >   
> > > + if (PTR_ERR(vbus) == -ENODEV) {
> > > + vbus = NULL;
> > > + } else if (IS_ERR(vbus)) {
> > > + ret = PTR_ERR(vbus);
> > > + goto disable_clk;
> > > + } else if (vbus) {
> > > + ret = regulator_enable(vbus);
> > > + if (ret) {
> > > + dev_err(>dev,
> > > + "failed to enable usb vbus regulator: %d\n",
> > > + ret);
> > > + goto disable_clk;
> > > + }
> > > + }  
> 
> This is all completely broken unless the supply is optional.

The supply is optional.

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

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Felipe Balbi]

Hi,

Mark Brown  writes:
> On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
>> Jisheng Zhang  writes:
>
>> > +  vbus = devm_regulator_get(>dev, "vbus");
>
>> devm_regulator_get_optional() ??
>
> Does USB work without a VBUS?  Unless the answer is yes then I'd expect

 it can, just source VBUS from a lab power supply 

> this to be just a normal regulator_get().

jokes aside, this regulator is optional because not all platforms
require a SW controlled regulator, no ? Will normal regulator_get() give
us a dummy regulator in case it's not listed in DT/ACPI ?

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH v6 08/12] usb: hcd: Adapt to OTG core

[Jun Li]

Hi

> 
> +
> +static struct otg_hcd_ops otg_hcd_intf = {
> + .add = usb_add_hcd,
> + .remove = usb_remove_hcd,
> + .usb_bus_start_enum = usb_bus_start_enum,

Build break if CONFIG_USB_OTG is not enabled:

drivers/built-in.o:(.data+0x1db30): undefined reference to `usb_bus_start_enum'
Makefile:948: recipe for target 'vmlinux' failed
make: *** [vmlinux] Error 1

Li Jun
 
> + .usb_control_msg = usb_control_msg,
> + .usb_hub_find_child = usb_hub_find_child, };
> +

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

Re: [RESEND PATCH v2 7/7] usb: xhci: plat: add vbus regulator control

[Mark Brown]

On Wed, Apr 27, 2016 at 08:37:20AM +0300, Felipe Balbi wrote:
> Jisheng Zhang  writes:

> > +   vbus = devm_regulator_get(>dev, "vbus");

> devm_regulator_get_optional() ??

Does USB work without a VBUS?  Unless the answer is yes then I'd expect
this to be just a normal regulator_get().

> 
> > +   if (PTR_ERR(vbus) == -ENODEV) {
> > +   vbus = NULL;
> > +   } else if (IS_ERR(vbus)) {
> > +   ret = PTR_ERR(vbus);
> > +   goto disable_clk;
> > +   } else if (vbus) {
> > +   ret = regulator_enable(vbus);
> > +   if (ret) {
> > +   dev_err(>dev,
> > +   "failed to enable usb vbus regulator: %d\n",
> > +   ret);
> > +   goto disable_clk;
> > +   }
> > +   }

This is all completely broken unless the supply is optional.


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

Hi,

Heikki Krogerus  writes:
> On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
>> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device 
>> *pdev)
>>  if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>>  xhci->shared_hcd->can_do_streams = 1;
>>  
>> +hcd->phy = devm_phy_get(>dev, "usb2-phy");
>> +if (IS_ERR(hcd->phy)) {
>> +ret = PTR_ERR(hcd->phy);
>> +if (ret == -EPROBE_DEFER)
>> +goto put_usb3_hcd;
>> +hcd->phy = NULL;
>> +}
>> +
>> +phy = devm_phy_get(>dev, "usb-phy");
>
> "usb-phy" for what I understand is the USB3 PHY right?

that doesn't appear to be documented anywhere ;-)

> I was unable to find any definition for the phy names for example from
> Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
> this needs to be "usb3-phy" and the phy names need to be defined
> somewhere.

yeah, usb3-phy sounds like a better name.

-- 
balbi


signature.asc
Description: PGP signature

Re: [PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[Felipe Balbi]

Hi,

changbin...@intel.com writes:
> From: "Du, Changbin" 
>
> The dwc3 controller can't generate a disconnect event after it is
> stopped. Thus gadget dissconnect callback is not invoked when do

not generating disconnect IRQ after you drop Run/Stop is expected.

> soft dissconnect. Call dissconnect here to workaround this issue.

I'm rather sure this is a bug elsewhere. How do you trigger this ?

> Note, most time we still see disconnect be called that because
> it is invoked by dwc3_gadget_reset_interrupt(). But if we
> disconnect cable once pullup disabled quickly, issue can be
> observed.

I can't understand what you're trying to say with this.

> Signed-off-by: Du, Changbin 
> ---
>  drivers/usb/dwc3/gadget.c | 33 -
>  1 file changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
> index 8e4a1b1..cd73187 100644
> --- a/drivers/usb/dwc3/gadget.c
> +++ b/drivers/usb/dwc3/gadget.c
> @@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int 
> is_on, int suspend)
>   return 0;
>  }
>  
> +static void dwc3_disconnect_gadget(struct dwc3 *dwc)
> +{
> + if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
> + spin_unlock(>lock);
> + dwc->gadget_driver->disconnect(>gadget);
> + spin_lock(>lock);
> + }
> +}
> +
>  static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
>  {
>   struct dwc3 *dwc = gadget_to_dwc(g);
> @@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, 
> int is_on)
>   is_on = !!is_on;
>  
>   spin_lock_irqsave(>lock, flags);
> + /**
> +  * WORKAROUND: The dwc3 controller can't generate a disconnect
> +  * event after it is stopped. Thus gadget dissconnect callback
> +  * is not invoked when do soft dissconnect. Call dissconnect
> +  * here to workaround this issue.
> +  * Note, most time we still see disconnect be called that because
> +  * it is invoked by dwc3_gadget_reset_interrupt(). But if we
> +  * disconnect cable once pullup disabled quickly, issue can be
> +  * observed.
> +  */
> + if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
> + dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");
> + dwc3_disconnect_gadget(dwc);
> + dwc->gadget.speed = USB_SPEED_UNKNOWN;
> + }

this is *really* wrong. You shouldn't be calling pullup directly. This
patch looks wrong and you didn't even explain how to trigger this
problem; when does the problem happen ?

Gadget load/unload does the right thing here, so that can't be the
case. We also do the right thing on soft_connect sysfs file:

static ssize_t usb_udc_softconn_store(struct device *dev,
struct device_attribute *attr, const char *buf, size_t n)
{
struct usb_udc  *udc = container_of(dev, struct usb_udc, dev);

if (!udc->driver) {
dev_err(dev, "soft-connect without a gadget driver\n");
return -EOPNOTSUPP;
}

if (sysfs_streq(buf, "connect")) {
usb_gadget_udc_start(udc);
usb_gadget_connect(udc->gadget);
} else if (sysfs_streq(buf, "disconnect")) {
usb_gadget_disconnect(udc->gadget);
udc->driver->disconnect(udc->gadget);
usb_gadget_udc_stop(udc);
} else {
dev_err(dev, "unsupported command '%s'\n", buf);
return -EINVAL;
}

return n;
}
static DEVICE_ATTR(soft_connect, S_IWUSR, NULL, usb_udc_softconn_store);

So really, what _is_ the problem ?

-- 
balbi


signature.asc
Description: PGP signature

[PATCH] usb: dwc3: usb/dwc3: fake dissconnect event when turn off pullup

[changbin . du]

From: "Du, Changbin" 

The dwc3 controller can't generate a disconnect event after it is
stopped. Thus gadget dissconnect callback is not invoked when do
soft dissconnect. Call dissconnect here to workaround this issue.

Note, most time we still see disconnect be called that because
it is invoked by dwc3_gadget_reset_interrupt(). But if we
disconnect cable once pullup disabled quickly, issue can be
observed.

Signed-off-by: Du, Changbin 
---
 drivers/usb/dwc3/gadget.c | 33 -
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
index 8e4a1b1..cd73187 100644
--- a/drivers/usb/dwc3/gadget.c
+++ b/drivers/usb/dwc3/gadget.c
@@ -1566,6 +1566,15 @@ static int dwc3_gadget_run_stop(struct dwc3 *dwc, int 
is_on, int suspend)
return 0;
 }
 
+static void dwc3_disconnect_gadget(struct dwc3 *dwc)
+{
+   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
+   spin_unlock(>lock);
+   dwc->gadget_driver->disconnect(>gadget);
+   spin_lock(>lock);
+   }
+}
+
 static int dwc3_gadget_pullup(struct usb_gadget *g, int is_on)
 {
struct dwc3 *dwc = gadget_to_dwc(g);
@@ -1575,6 +1584,21 @@ static int dwc3_gadget_pullup(struct usb_gadget *g, int 
is_on)
is_on = !!is_on;
 
spin_lock_irqsave(>lock, flags);
+   /**
+* WORKAROUND: The dwc3 controller can't generate a disconnect
+* event after it is stopped. Thus gadget dissconnect callback
+* is not invoked when do soft dissconnect. Call dissconnect
+* here to workaround this issue.
+* Note, most time we still see disconnect be called that because
+* it is invoked by dwc3_gadget_reset_interrupt(). But if we
+* disconnect cable once pullup disabled quickly, issue can be
+* observed.
+*/
+   if (!is_on && (dwc->gadget.speed != USB_SPEED_UNKNOWN)) {
+   dev_dbg(dwc->dev, "fake dissconnect event on pullup off\n");
+   dwc3_disconnect_gadget(dwc);
+   dwc->gadget.speed = USB_SPEED_UNKNOWN;
+   }
ret = dwc3_gadget_run_stop(dwc, is_on, false);
spin_unlock_irqrestore(>lock, flags);
 
@@ -2144,15 +2168,6 @@ static void dwc3_endpoint_interrupt(struct dwc3 *dwc,
}
 }
 
-static void dwc3_disconnect_gadget(struct dwc3 *dwc)
-{
-   if (dwc->gadget_driver && dwc->gadget_driver->disconnect) {
-   spin_unlock(>lock);
-   dwc->gadget_driver->disconnect(>gadget);
-   spin_lock(>lock);
-   }
-}
-
 static void dwc3_suspend_gadget(struct dwc3 *dwc)
 {
if (dwc->gadget_driver && dwc->gadget_driver->suspend) {
-- 
2.7.4

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

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Heikki Krogerus]

Hi,

On Tue, Apr 26, 2016 at 08:57:39PM +0800, Jisheng Zhang wrote:
> @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device *pdev)
>   if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
>   xhci->shared_hcd->can_do_streams = 1;
>  
> + hcd->phy = devm_phy_get(>dev, "usb2-phy");
> + if (IS_ERR(hcd->phy)) {
> + ret = PTR_ERR(hcd->phy);
> + if (ret == -EPROBE_DEFER)
> + goto put_usb3_hcd;
> + hcd->phy = NULL;
> + }
> +
> + phy = devm_phy_get(>dev, "usb-phy");

"usb-phy" for what I understand is the USB3 PHY right?

I was unable to find any definition for the phy names for example from
Documentation/devicetree/bindings/usb/usb-xhci.txt, so I would say
this needs to be "usb3-phy" and the phy names need to be defined
somewhere.


Thanks,

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

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Felipe Balbi]

Hi,

Jisheng Zhang  writes:
>> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
>> > +{
>> > +  if (hcd->phy) {
>> > +  phy_power_off(hcd->phy);
>> > +  phy_exit(hcd->phy);
>> > +  } else {
>> > +  usb_phy_shutdown(hcd->usb_phy);
>> > +  }
>> > +}
>> > +
>> >  static int xhci_plat_probe(struct platform_device *pdev)
>> >  {
>> >struct device_node  *node = pdev->dev.of_node;
>> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device 
>> > *pdev)
>> >struct usb_hcd  *hcd;
>> >struct clk  *clk;
>> >struct usb_phy  *usb_phy;
>> > +  struct phy  *phy;  
>> 
>> so, one phy driver using USB PHY layer and another using generic PHY
>> layer ? Why ? I think the first thing your series should do would be to
>
> It's different platforms. E.g
> platform A may write the phy driver under usb phy layer, while platform B
> may have generic phy driver.

right, but both APIs should be supported with *two* PHYs for the time being.

> The questions are: when adding phy support to xhci-plat, the generic phy
> has existed for a long time, what's the reason to use the deprecated usb
> phy APIs.

I don't know, ask the author :-) Maybe the PHY driver was already
available on the USB PHY layer ? What we should do is push that PHY
driver to be moved over to generic PHY layer, then we can get rid of USB
PHY layer from xhci-plat.

> And per my check, it's only MVEBU platforms use this support? I'm not sure
> if we could remove usbphy code from xhci-plat first then add generic phy then
> adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

First the PHY driver(s) depending on that should be converted over.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Felipe Balbi]

Hi,

Jisheng Zhang  writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:
>
>> Jisheng Zhang  writes:
>> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
>> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
>> > unconditionally.
>> >
>> > This patch is to simplify probe error and remove code paths.  
>> 
>> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
>> initialized to a valid struct clk * or some ERR_PTR() value.
>
> Commit 63589e92c2d9 could also ignore error value ;)

oh okay, thanks for that. That's, IMHO, quite dangerous ;-)

-- 
balbi


signature.asc
Description: PGP signature

RE: [PATCH] option.c: Support for Gemalto's Cinterion PH8 and AHxx products added

[Schemmel Hans-Christoph]

> -Original Message-
> From: Johan Hovold [mailto:jhov...@gmail.com] On Behalf Of Johan Hovold
> Sent: Sonntag, 24. April 2016 12:15
> To: Schemmel Hans-Christoph
> Cc: Johan Hovold; gre...@linuxfoundation.org; linux-usb@vger.kernel.org
> Subject: Re: [PATCH] option.c: Support for Gemalto's Cinterion PH8 and AHxx 
> products added
> 
> 
> Could you provide the usb-devices output for this one as well? As AHXX was 
> already using USB_DEVICE_INTERFACE_CLASS I suspect
> you should not modify this one at all.
> 


Hello Johan,

ok, great. I´m going to prepare a patch with USB_DEVICE_INTERFACE_CLASS instead 
of USB_DEVICE.


Please find below the AHXX usb-devices output:

T:  Bus=01 Lev=01 Prnt=01 Port=02 Cnt=01 Dev#= 24 Spd=480 MxCh= 0
D:  Ver= 2.00 Cls=ef(misc ) Sub=02 Prot=01 MxPS=64 #Cfgs=  1
P:  Vendor=1e2d ProdID=0055 Rev=00.00
S:  Manufacturer=Cinterion
S:  Product=AHx
C:  #Ifs= 6 Cfg#= 1 Atr=e0 MxPwr=10mA
I:  If#= 0 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I:  If#= 1 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I:  If#= 2 Alt= 0 #EPs= 2 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I:  If#= 3 Alt= 0 #EPs= 3 Cls=ff(vend.) Sub=ff Prot=ff Driver=option
I:  If#= 4 Alt= 0 #EPs= 1 Cls=02(commc) Sub=06 Prot=00 Driver=cdc_ether
I:  If#= 5 Alt= 1 #EPs= 2 Cls=0a(data ) Sub=00 Prot=00 Driver=cdc_ether

You´re right, it can be also handled with USB_DEVICE_INTERFACE_CLASS - no need 
to modify it.

Thanks a lot.

Kind regards
Christoph


smime.p7s
Description: S/MIME cryptographic signature

[PATCHv2] musb_host: fix lockup on rxcsr_h_error

[Max Uvarov]

Fix soft lockup when resetting remote device attached
to usb host. Configuration:
pppd -> musb hub -> usb-serial -> gsm modem
When gsm modem resets, musb rolls in incoming rx interrupts
which does not give any time to other application as result
it totally lock ups. Solution is to keep original logic for RXCSR_H_ERROR
and merge RXCSR_DATAERROR and RXCSR_H_ERROR branches to call same code
for setting rx stall with MUSB_RXCSR_H_WZC_BITS.

Signed-off-by: Max Uvarov 
---
 v2: use bitwise or for error flags before logical and. (Sergei Shtylyov).

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

diff --git a/drivers/usb/musb/musb_host.c b/drivers/usb/musb/musb_host.c
index c3d5fc9..2d9aa78 100644
--- a/drivers/usb/musb/musb_host.c
+++ b/drivers/usb/musb/musb_host.c
@@ -1592,14 +1592,12 @@ void musb_host_rx(struct musb *musb, u8 epnum)
 
/* stall; record URB status */
status = -EPIPE;
+   } else if (rx_csr & (MUSB_RXCSR_DATAERROR | MUSB_RXCSR_H_ERROR)) {
 
-   } else if (rx_csr & MUSB_RXCSR_H_ERROR) {
-   dev_dbg(musb->controller, "end %d RX proto error\n", epnum);
-
-   status = -EPROTO;
-   musb_writeb(epio, MUSB_RXINTERVAL, 0);
-
-   } else if (rx_csr & MUSB_RXCSR_DATAERROR) {
+   if (rx_csr & MUSB_RXCSR_H_ERROR) {
+   status = -EPROTO;
+   musb_writeb(epio, MUSB_RXINTERVAL, 0);
+   }
 
if (USB_ENDPOINT_XFER_ISOC != qh->type) {
dev_dbg(musb->controller, "RX end %d NAK timeout\n", 
epnum);
-- 
1.9.1

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

Re: [RESEND PATCH v2 6/7] usb: xhci: plat: add generic PHY support

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:35:58 +0300 Felipe Balbi wrote:

> Hi,
> 
> Jisheng Zhang  writes:
> > Marvell BG4CT SoC needs two phy: one for usb2 and another for usb3. Add
> > the calls to retrieve generic PHY to xhci plat in order to support this.
> >
> > Signed-off-by: Jisheng Zhang 
> > ---
> >  drivers/usb/host/xhci-plat.c | 87 
> > ++--
> >  1 file changed, 75 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 83669d0..d7f4f3c 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -16,6 +16,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #include 
> > @@ -134,6 +135,37 @@ static const struct of_device_id usb_xhci_of_match[] = 
> > {
> >  MODULE_DEVICE_TABLE(of, usb_xhci_of_match);
> >  #endif
> >  
> > +static int xhci_plat_phy_init(struct usb_hcd *hcd)
> > +{
> > +   int ret;
> > +
> > +   if (hcd->phy) {
> > +   ret = phy_init(hcd->phy);
> > +   if (ret)
> > +   return ret;
> > +
> > +   ret = phy_power_on(hcd->phy);
> > +   if (ret) {
> > +   phy_exit(hcd->phy);
> > +   return ret;
> > +   }
> > +   } else {
> > +   ret = usb_phy_init(hcd->usb_phy);
> > +   }
> > +
> > +   return ret;
> > +}
> > +
> > +static void xhci_plat_phy_exit(struct usb_hcd *hcd)
> > +{
> > +   if (hcd->phy) {
> > +   phy_power_off(hcd->phy);
> > +   phy_exit(hcd->phy);
> > +   } else {
> > +   usb_phy_shutdown(hcd->usb_phy);
> > +   }
> > +}
> > +
> >  static int xhci_plat_probe(struct platform_device *pdev)
> >  {
> > struct device_node  *node = pdev->dev.of_node;
> > @@ -145,6 +177,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > struct usb_hcd  *hcd;
> > struct clk  *clk;
> > struct usb_phy  *usb_phy;
> > +   struct phy  *phy;  
> 
> so, one phy driver using USB PHY layer and another using generic PHY
> layer ? Why ? I think the first thing your series should do would be to

It's different platforms. E.g
platform A may write the phy driver under usb phy layer, while platform B
may have generic phy driver.

The questions are: when adding phy support to xhci-plat, the generic phy
has existed for a long time, what's the reason to use the deprecated usb
phy APIs.

And per my check, it's only MVEBU platforms use this support? I'm not sure
if we could remove usbphy code from xhci-plat first then add generic phy then
adding MVEBU xhci phy support bak with the new code. So Cc mvebu maintainers

Thanks,
Jisheng

> add proper suport for both APIs with two PHYs and make them all optional
> for xhci-plat.
> 
> > @@ -232,22 +265,44 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> > if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> > xhci->shared_hcd->can_do_streams = 1;
> >  
> > +   hcd->phy = devm_phy_get(>dev, "usb2-phy");
> > +   if (IS_ERR(hcd->phy)) {
> > +   ret = PTR_ERR(hcd->phy);
> > +   if (ret == -EPROBE_DEFER)
> > +   goto put_usb3_hcd;
> > +   hcd->phy = NULL;
> > +   }
> > +
> > +   phy = devm_phy_get(>dev, "usb-phy");
> > +   if (IS_ERR(phy)) {
> > +   ret = PTR_ERR(phy);
> > +   if (ret == -EPROBE_DEFER)
> > +   goto put_usb3_hcd;
> > +   phy = NULL;
> > +   }
> > +
> > usb_phy = devm_usb_get_phy_by_phandle(>dev, "usb-phy", 0);
> > if (IS_ERR(usb_phy)) {
> > ret = PTR_ERR(usb_phy);
> > if (ret == -EPROBE_DEFER)
> > goto put_usb3_hcd;
> > usb_phy = NULL;
> > -   } else {
> > -   ret = usb_phy_init(usb_phy);
> > -   if (ret)
> > -   goto put_usb3_hcd;
> > }
> > +
> > xhci->shared_hcd->usb_phy = usb_phy;
> > +   xhci->shared_hcd->phy = phy;
> > +
> > +   ret = xhci_plat_phy_init(hcd);
> > +   if (ret)
> > +   goto put_usb3_hcd;
> > +
> > +   ret = xhci_plat_phy_init(xhci->shared_hcd);
> > +   if (ret)
> > +   goto disable_usb2_phy;
> >  
> > ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> > if (ret)
> > -   goto disable_usb_phy;
> > +   goto disable_usb3_phy;
> >  
> > ret = usb_add_hcd(xhci->shared_hcd, irq, IRQF_SHARED);
> > if (ret)
> > @@ -259,8 +314,11 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> >  dealloc_usb2_hcd:
> > usb_remove_hcd(hcd);
> >  
> > -disable_usb_phy:
> > -   usb_phy_shutdown(usb_phy);
> > +disable_usb3_phy:
> > +   xhci_plat_phy_exit(xhci->shared_hcd);
> > +
> > +disable_usb2_phy:
> > +   xhci_plat_phy_exit(hcd);
> >  
> >  put_usb3_hcd:
> > usb_put_hcd(xhci->shared_hcd);
> > @@ -281,11 +339,11 @@ static int xhci_plat_remove(struct platform_device 
> > *dev)
> > struct clk 

[PATCH v3 1/1] usbip: safe completion against unbind operation

[Nobuo Iwata]

This patch adds a code fragment to ignore completing URBs in closing 
connection.

Regarding this patch, 2 execution contexts are related.

1) stub_tx.c: stub_complete() which is called from USB core
1-1) add to unlink list and free URB or
1-2) move to tx list

2) stub_dev.c: stub_shutdown_connection() which is invoked by unbind 
operation through sysfs.
2-1) stop TX/RX threads
2-2) close TCP connection and set ud.tcp_socket to NULL
2-3) cleanup pending URBs by stub_device_cleanup_urbs(sdev)
2-4) free unlink list (no lock)

In the race condition, URBs which will be cleared in 2-3) may be 
handled in 1).
In case 1-1), it will not be transferred bcause tx threads are stooped 
in 2-1).
In case 1-2), may be freed in 2-4). 

With this patch, after 2-2), completing URBs in 1) will not be handled 
and cleared in 2-3).

The kernel log with this patch is as below.

kernel: usbip_core: usbip_kernel_unlink:792: shutting down tcp_socket 
ef61d980
kernel: usbip-host 1-3: free sdev f5df6180
kernel: usbip-host 1-3: free urb f5df6700
kernel: usbip-host 1-3: Enter
kernel: usbip_core: usbip_stop_eh:132: usbip_eh waiting completion 5
kernel: usbip_host: stub_complete:71: complete! status 0
kernel: usbip_host: stub_complete:102: ignore urb for closed connection 
e725fc00 (*)
kernel: usbip_host: stub_complete:71: complete! status -2
kernel: usbip-host 1-3: stopped by a call to usb_kill_urb() because of 
cleaning up a virtual connection
kernel: usbip-host 1-3: free urb e725fc00 (**)
kernel: usbip-host 1-3: free urb e725e000
kernel: usbip_host: stub_complete:71: complete! status -2
kernel: usbip-host 1-3: stopped by a call to usb_kill_urb() because of 
cleaning up a virtual connection
kernel: usbip-host 1-3: free urb e725f800
kernel: usbip_host: stub_complete:71: complete! status -2
kernel: usbip-host 1-3: stopped by a call to usb_kill_urb() because of 
cleaning up a virtual connection
kernel: usbip-host 1-3: free urb e725e800
kernel: usbip_host: stub_complete:71: complete! status -2
kernel: usbip-host 1-3: stopped by a call to usb_kill_urb() because of 
cleaning up a virtual connection
kernel: usbip-host 1-3: device reset
kernel: usbip-host 1-3: lock for reset
kernel: usbip_host: store_match_busid:178: del busid 1-3
kernel: uvcvideo: Found UVC 1.00 device Venus USB2.0 Camera (056e:700a)
kernel: input: Venus USB2.0 Camera as 
/devices/pci:00/:00:1a.7/usb1/1-3/1-3:1.0/input/input22

(*) skipped with this patch in completion
(**) released in 2-3

---
Version information

v3)
# Fixed misoperation that changing log level in v2 was not included.

v2)
# Changed log level of ignore message from info to debug.
# Updated log capture in changelog with the log level modification.

Signed-off-by: Nobuo Iwata 
---
 drivers/usb/usbip/stub_tx.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/usbip/stub_tx.c b/drivers/usb/usbip/stub_tx.c
index dbcabc9..dc223af 100644
--- a/drivers/usb/usbip/stub_tx.c
+++ b/drivers/usb/usbip/stub_tx.c
@@ -97,7 +97,10 @@ void stub_complete(struct urb *urb)
 
/* link a urb to the queue of tx. */
spin_lock_irqsave(>priv_lock, flags);
-   if (priv->unlinking) {
+   if (sdev->ud.tcp_socket == NULL) {
+   usbip_dbg_stub_tx("ignore urb for closed connection %p", urb);
+   /* It will be freed in stub_device_cleanup_urbs(). */
+   } else if (priv->unlinking) {
stub_enqueue_ret_unlink(sdev, priv->seqnum, urb->status);
stub_free_priv_and_urb(priv);
} else {
-- 
2.1.0

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

Re: [RESEND PATCH v2 5/7] usb: xhci: plat: Remove checks for optional clock in error/remove path

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:33:52 +0300 Felipe Balbi wrote:

> Jisheng Zhang  writes:
> > Commit 63589e92c2d9 ("clk: Ignore error and NULL pointers passed to
> > clk_{unprepare, disable}()") allows NULL or error pointer to be passed
> > unconditionally.
> >
> > This patch is to simplify probe error and remove code paths.  
> 
> this seems wrong to me. xhci->clk isn't initialized to NULL, it's either
> initialized to a valid struct clk * or some ERR_PTR() value.

Commit 63589e92c2d9 could also ignore error value ;)

> 
> Care to explain ?
> 
> > Signed-off-by: Jisheng Zhang 
> > ---
> >  drivers/usb/host/xhci-plat.c | 6 ++
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 0e69712..83669d0 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -266,8 +266,7 @@ put_usb3_hcd:
> > usb_put_hcd(xhci->shared_hcd);
> >  
> >  disable_clk:
> > -   if (!IS_ERR(clk))
> > -   clk_disable_unprepare(clk);
> > +   clk_disable_unprepare(clk);
> >  
> >  put_hcd:
> > usb_put_hcd(hcd);
> > @@ -287,8 +286,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> > usb_remove_hcd(hcd);
> > usb_put_hcd(xhci->shared_hcd);
> >  
> > -   if (!IS_ERR(clk))
> > -   clk_disable_unprepare(clk);
> > +   clk_disable_unprepare(clk);
> > usb_put_hcd(hcd);
> >  
> > return 0;
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

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

Re: [RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Felipe Balbi]

Hi,

Jisheng Zhang  writes:
> Dear Felipe,
>
> On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:
>
>> Hi,
>> 
>> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)
>
> Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
> To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to 
> updated.
>
>> 
>> Jisheng Zhang  writes:
>> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
>> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
>> 
>> where did you see that's the USB3 phy ? I can't see it.
>
> Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:
>
> "The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

That could be a typo. VBUS is defined by the original USB specification
and there is only a single VBUS ping for all speeds ;-)

> So I think it means USB3 phy.

we should ask patch author :-)

> Here I have two questions: Per my understanding, usb_phy is deprecated, all
> users and new code are encouraged to use generic phy. I think there must
> be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
> I know what's the reason?

I don't know, maybe author knows

> And could the "USB3 VBUS" support be achieved through regulator framework?

depends on how the PHY is integrated but, IMO, it should be doable and
is probably preferrable.

-- 
balbi


signature.asc
Description: PGP signature

Re: [RESEND PATCH v2 2/7] usb: xhci: plat: attach the usb_phy to the correct hcd

[Jisheng Zhang]

Dear Felipe,

On Wed, 27 Apr 2016 08:29:34 +0300 Felipe Balbi wrote:

> Hi,
> 
> (Cc authors and maintainer, otherwise you're patch might be forgotten ;-)

Thanks a lot for the kind remind. I just run get_maintainer.pl to get the
To and Cc lists. It seems Mathias's email changed, MAINTAINERS need to updated.

> 
> Jisheng Zhang  writes:
> > Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") adds the
> > usb_phy for usb3, but it attached the usb_phy to incorrect hcd. The  
> 
> where did you see that's the USB3 phy ? I can't see it.

Commit 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support") says:

"The Marvell Armada 385 AP needs a dumb phy in order to enable the USB3 VBUS."

So I think it means USB3 phy.

Here I have two questions: Per my understanding, usb_phy is deprecated, all
users and new code are encouraged to use generic phy. I think there must
be some special reason commit 7b8ef22ea547 still made use of usb_phy, could
I know what's the reason?

And could the "USB3 VBUS" support be achieved through regulator framework?

Thanks in advance,
Jisheng


> 
> > xhci->shared_hcd is the hcd for usb3, this patch fixes this issue
> > by attach the usb_phy to the xhci->shared_hcd.  
> 
> Fixes: 7b8ef22ea547 ("usb: xhci: plat: Add USB phy support")
> Cc:  
> Maxime, any comments ? It _is_ odd that only one PHY got added.
> 
> > Signed-off-by: Jisheng Zhang 
> > ---
> >  drivers/usb/host/xhci-plat.c | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
> > index 8cb46cb..9ff89e9 100644
> > --- a/drivers/usb/host/xhci-plat.c
> > +++ b/drivers/usb/host/xhci-plat.c
> > @@ -144,6 +144,7 @@ static int xhci_plat_probe(struct platform_device *pdev)
> > struct resource *res;
> > struct usb_hcd  *hcd;
> > struct clk  *clk;
> > +   struct usb_phy  *usb_phy;
> > int ret;
> > int irq;
> >  
> > @@ -231,17 +232,18 @@ static int xhci_plat_probe(struct platform_device 
> > *pdev)
> > if (HCC_MAX_PSA(xhci->hcc_params) >= 4)
> > xhci->shared_hcd->can_do_streams = 1;
> >  
> > -   hcd->usb_phy = devm_usb_get_phy_by_phandle(>dev, "usb-phy", 0);
> > -   if (IS_ERR(hcd->usb_phy)) {
> > -   ret = PTR_ERR(hcd->usb_phy);
> > +   usb_phy = devm_usb_get_phy_by_phandle(>dev, "usb-phy", 0);
> > +   if (IS_ERR(usb_phy)) {
> > +   ret = PTR_ERR(usb_phy);
> > if (ret == -EPROBE_DEFER)
> > goto put_usb3_hcd;
> > -   hcd->usb_phy = NULL;
> > +   usb_phy = NULL;
> > } else {
> > -   ret = usb_phy_init(hcd->usb_phy);
> > +   ret = usb_phy_init(usb_phy);
> > if (ret)
> > goto put_usb3_hcd;
> > }
> > +   xhci->shared_hcd->usb_phy = usb_phy;
> >  
> > ret = usb_add_hcd(hcd, irq, IRQF_SHARED);
> > if (ret)
> > @@ -258,7 +260,7 @@ dealloc_usb2_hcd:
> > usb_remove_hcd(hcd);
> >  
> >  disable_usb_phy:
> > -   usb_phy_shutdown(hcd->usb_phy);
> > +   usb_phy_shutdown(usb_phy);
> >  
> >  put_usb3_hcd:
> > usb_put_hcd(xhci->shared_hcd);
> > @@ -280,7 +282,7 @@ static int xhci_plat_remove(struct platform_device *dev)
> > struct clk *clk = xhci->clk;
> >  
> > usb_remove_hcd(xhci->shared_hcd);
> > -   usb_phy_shutdown(hcd->usb_phy);
> > +   usb_phy_shutdown(xhci->shared_hcd->usb_phy);
> >  
> > usb_remove_hcd(hcd);
> > usb_put_hcd(xhci->shared_hcd);
> > -- 
> > 2.8.1
> >
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-usb" in
> > the body of a message to majord...@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html  
> 

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