RE: [PATCH v3] usb: dwc3: Fix assignment of EP transfer resources

[B, Ravi]

Hi John


>Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
>SET_INTERFACE")
>Cc:  # v3.2+
>Reported-by: Ravi Babu 
>Signed-off-by: John Youn 
>---

>Hi Ravi,

>Could you verify that it works with your test scenario?

Yes, it is working fine for me. 

>Thanks,
>John

>+static int dwc3_gadget_set_xfer_resource(struct dwc3 *dwc, struct dwc3_ep 
>*dep);
>+
>+/**
>+ * dwc3_gadget_start_config - Configure EP resources
>+ * @dwc: pointer to our controller context structure
>+ * @dep: endpoint that is being enabled
>+ *
>+ * The assignment of transfer resources cannot perfectly follow the
>+ * data book due to the fact that the controller driver does not have
>+ * all knowledge of the configuration in advance. It is given this
>+ * information piecemeal by the composite gadget framework after every
>+ * SET_CONFIGURATION and SET_INTERFACE. Trying to follow the databook
>+ * programming model in this scenario can cause errors. For two
>+ * reasons:
>+ *
>+ * 1) The databook says to do DEPSTARTCFG for every SET_CONFIGURATION
>+ * and SET_INTERFACE (8.1.5). This is incorrect in the scenario of
>+ * multiple interfaces.
>+ *
>+ * 2) The databook does not mention doing more DEPXFERCFG for new
>+ * endpoint on alt setting (8.1.6).
>+ *
>+ * The following simplified method is used instead:
>+ *
>+ * All hardware endpoints can be assigned a transfer resource and this
>+ * setting will stay persistent until either a core reset or
>+ * hibernation. So whenever we do a DEPSTARTCFG(0) we can go ahead and
>+ * do DEPXFERCFG for every hardware endpoint as well. We are
>+ * guaranteed that there are as many transfer resources as endpoints.
>+ *

The databook says the resource is released after transfer completion event on 
ep,  in this case whether resource
is still valid which was pre-allocated to all eps during initial setup? 
If the core is designed to reuse of unused ep's resources, whether is there any 
performance impact by core
due resource pre-allocated for eps ?
Please confirm with IP folks as well.

>+ * This function is called for each endpoint when it is being enabled
>+ * but is triggered only when called for EP0-out, which always happens
>+ * first, and which should only happen in one of the above conditions.
>+ */
 

Regards
Ravi 

--
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: Fix assignment of EP transfer resources

[B, Ravi]

Hi John

>-Original Message-
>From: John Youn [mailto:john.y...@synopsys.com] 
>Sent: Wednesday, February 10, 2016 1:25 AM
>To: Felipe Balbi; B, Ravi
>Cc: john.y...@synopsys.com; linux-usb@vger.kernel.org
>Subject: [PATCH] usb: dwc3: Fix assignment of EP transfer resources

>The assignement of EP transfer resources was not handled properly in the
>dwc3 driver. Commit aebda6187181 ("usb: dwc3: Reset the transfer
>resource index on SET_INTERFACE") previously fixed one aspect of this
>where resources may be exhausted with multiple calls to SET_INTERFACE.
>However, it introduced an issue where composite devices with multiple
>interfaces can be assigned the same transfer resources for different
>endpoints.

>This patch solves both issues.

>The assigning of transfer resource should go as follows:

>Each hardware endpoint requires a transfer resource before it can
>perform any USB transfer. The transfer resources are allocated using two
>commands: DEPSTARTCFG and DEPXFERCFG.

>In the controller, there is a transfer resource index that is set by the
>DEPSTARTCFG command. The DEPXFERCFG command assigns the resource to an
>endpoint and increments the transfer resource index.

>Upon startup of the driver, the transfer resource index should be set to
>0 by issuing DEPSTARTCFG(0). EP0-out and EP0-in are then assigned a
>resource by DEPXFERCFG. They are assigned resource indexes 0 and 1.

>Every time a SET_CONFIGURATION usb request occurs the DEPSTARTCFG(2)
>command should be issued to reset the index to 2. Transfer resources 0
>and 1 remain assigned to EP0-out and EP0-in.

>Then for every endpoint in the configuration (endpoints that will be
>enabled by the upper layer) call DEPXFERCFG to assign the next
>resource. On SET_INTERFACE, the same or different endpoints may be
>enabled. If the endpoint already has an assigned transfer resource,
>don't assign a new one.

>Fixes: aebda6187181 ("usb: dwc3: Reset the transfer resource index on 
>SET_INTERFACE")
>Reported-by: Ravi Babu <ravib...@ti.com>
>Signed-off-by: John Youn <johny...@synopsys.com>
>---
>Hi Ravi,

>Here is a patch that should solve your issue. Can you review and test
>it out?

>I have tested with multiple SET_INTERFACE for a single interface.

I have run the same test case with composite gadget with two 
interface(NCM+ACM), the resource conflict is not seen.
It resolve the issue. Thanks for the patch.

Regards
Ravi 

 >drivers/usb/dwc3/core.h   |  3 +++
 >drivers/usb/dwc3/ep0.c|  4 
 >drivers/usb/dwc3/gadget.c | 36 +---
 >3 files changed, 36 insertions(+), 7 deletions(-)


--
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] dwc3: gadget: fix for no-resource condition in dwc3 device controller

[B, Ravi]

Felipe

>hi,

>Ravi Babu  writes:
>> The "no-resource" error occurs when the driver issues DEPSTRTXFER 
>> command to start data transfer on specific endpoint, and dwc3

>this seems to imply that simply sending Start Transfer command would trigger 
>no-resource which is untrue. The error you mention happens when there's a 
>transfer ongoing and another Start Transfer is sent.

Yes, when there is on-going transfer on one endpoint in in/out direction (say 
ep1-in), then trying to start transfer on another endpoint in same direction 
(say ep2-in), leads to resource conflict.

>> core throws error "no resource" available to process the request.
>>
>> This condition is occurs in composite gadget scenario where there are 
>> multiple interfaces selected by host and during simulateneous data 
>> traffic on multiple endpoints on these interfaces, the issue

>having several interfaces using different endpoints has no implication into 
>how Start Transfer command is sent, so this shouldn't happen. Are you having 
>issues with one particular interface, perhaps ? 

Issue occurs, during simultaneous transfer happen on endpoints on multiple 
interfaces in same direction, say ep1-in on interface-1 is busy by dwc3 and 
start transfer on ep2-in on interface-2. 

>> occur when software/driver issues DEPSTRTXFER command to specific 
>> endpoint in same direction (in/out) and other endpoint in the same 
>> direction (in/out) is busy or executed by dwc3 controller.

>we have DWC3_EP_BUSY keeping track of that, I fail to see how you're 
>triggering this condition. Any logs available ? Also, which kernel are you 
>using to test this ? v4.4 ? v4.5-rc ?

Yes, I have monitored this flag and found that issue occurs only when ep1-in is 
busy(DWC3_EP_BUSY) and issue start transfer on ep2-in. (same applicable for out 
transfer).
The issue was reproducible on 4.5-rc1.

>> example: if dwc3 core is busy in transferring the request on ep1-in 
>> during this scenario, if software/driver queues another request on 
>> ep2-in and issue start transfer (DEPSTRTXFER command), then dwc3 
>> throws error "no-resource". The same applicable to OUT transfer. 

>this is wrong. EP1's resources are not related to EP2's resources. I have 
>feeling you haven't really understood the problem you're facing.

This issue occurs, because of same resource allocated for both ep1-in and 
ep2-in, due to DEPSTARTCFG issued twice. The RTL designer (synopsis) confirmed 
that DEPSTARTCFG must be issued only once for one configuration, otherwise each 
time DEPSTARTCFG issued, the dwc3 core reset its resource allocation scheme, 
allocates the same resource allocated to endpoints of previous interfaces.

>> The issue is root caused that software issue DEPSTARTCFG "START NEW 
>> CONFIGURATION" command twice during the SET_INTERFACE request received 
>> from host while selecting multiple interface.

>which is expected. See commit aebda61871815 ("usb: dwc3: Reset the transfer 
>resource index on SET_INTERFACE"). In fact, this commit regresses what 
>previous commit fixed.

>This is a clear NAK, sorry.

>> In single configuration with two parallel interfaces, say the
>> interface-1 has two endpoints, ep1-in & ep1-out, and interface-2 has 
>> two endpoints ep2-in and ep2-out. The additional DEPSTARTCFG will make 
>> the core too allocate the same resource, say for ep2-in and ep2-out 
>> which were previously assigned to ep1-in and ep1-out.
>>
>> Therefore during the simulataneous traffic on endpoints on same 
>> direction leads to resource conflict and dwc3 core throws the 
>> "no-resource" error when DEPSTARTCFG command while issuing the start 
>> transfer request.
>>
>> Hence the DEPSTARTCFG must be issued only once after reset and during 
>> SET_CONFIG request from host to allocate the transfer resource 
>> properly for all endpoints on multiple interfaces in composite gadget 
>> scenario. 

>Did you read Synopsys databook ? Did you go through git log to see what that 
>particular line was added ?

Yes, synopsis book says, DEPSTARTCFG should be issued for SET-CONFIGURATION  
--OR-- SET_INTERFACE, does not specify for both. 
This has been confirmed by Synopsis RTL designer. 

>> The issue is reproducible in composite gadget (ACM + NCM) enabled 
>> through CONFIGFS.

>any scripts available to reproduce this with some recent kernel ?

Yes, attached configfs script to enabled NCM+ACM. But I have tested by 
transmitting/receving data over ACM between EVM and Ubuntu host, in parallel do 
NCM transfer.
1) Start ACM serial transfer on both EVM and host. 
2) just do ifconfig usb0, the issue is reproducible. 

>If you can show a large trace output showing the error, that would be great. 
>In fact, if you could annotate it with what you think is wrong, that would be 
>even better ;-)
1) I have attached complete kernel log.
2) The have added below error print  line which was removed in 4.5-rc1, in 
__dwc3_gadget_kick_transfer() function in drivers/usb/dwc3/gadget.c.  

RE: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb-ops

[B, Ravi]

Felipe

 
 On Wed, May 29, 2013 at 06:37:03PM +0530, Ravi Babu wrote:
  Adding babble_recovery operation as part of musb-ops, used to recover
  from babble condition during babble interrupt.
 
  Signed-off-by: Ravi Babu ravib...@ti.com
  ---
   drivers/usb/musb/musb_core.c |6 ++
   drivers/usb/musb/musb_core.h |7 +++
   2 files changed, 13 insertions(+), 0 deletions(-)
 
  diff --git a/drivers/usb/musb/musb_core.c
  b/drivers/usb/musb/musb_core.c index ab6fa39..411c29d 100644
  --- a/drivers/usb/musb/musb_core.c
  +++ b/drivers/usb/musb/musb_core.c
  @@ -857,6 +857,12 @@ b_host:
  }
  }
 
  +   /* handle babble condition */
  +   if (int_usb  MUSB_INTR_BABBLE) {
  +   pr_info(babble: restarting the musb controller..);
  +   musb_babble_recovery(musb);
  +   }
  +
   #if 0
   /* REVISIT ... this would be for multiplexing periodic endpoints, or
* supporting transfer phasing to prevent exceeding ISO bandwidth
  diff --git a/drivers/usb/musb/musb_core.h
  b/drivers/usb/musb/musb_core.h index f96e899..bf37dc9 100644
  --- a/drivers/usb/musb/musb_core.h
  +++ b/drivers/usb/musb/musb_core.h
  @@ -213,6 +213,8 @@ struct musb_platform_ops {
  int (*adjust_channel_params)(struct dma_channel *channel,
  u16 packet_sz, u8 *mode,
  dma_addr_t *dma_addr, u32 *len);
  +
  +   void(*babble_recovery)(struct musb *musb);
 
 I don't get why can't 'babble_recovery' be generic. Why do we need each glue
 layer to implement it ?
 

Babble is generic, but recovery mechanism is nothing but reset of usbss which 
is SoC dependent and followed by generic restart of the musb controller. 
That is why musb_restart() API is exported used by all glue in babble recovery.

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

RE: [PATCH v1 2/3] usb: musb: core: added babble recovery func-ptr to musb-ops

[B, Ravi]

Hi Felipe

@@ -213,6 +213,8 @@ struct musb_platform_ops {
int (*adjust_channel_params)(struct dma_channel *channel,
u16 packet_sz, u8 *mode,
dma_addr_t *dma_addr, u32 *len);
+
+   void(*babble_recovery)(struct musb *musb);
  
   I don't get why can't 'babble_recovery' be generic. Why do we need
   each glue layer to implement it ?
  
 
  Babble is generic, but recovery mechanism is nothing but reset of
  usbss which is SoC dependent and followed by generic restart of the
  musb controller.
 
 and that's what I don't get. Why do you need to reset usbss ?

On babble condition, the session bit is removed by mentor, hence to make musb 
work
1) setting the session alone will not help.
2) restart the usbss and setting session also not helped.
3) musb works only after usbss reset followed by epfifo table init and 
re-enable all interrupts, then set the session. 

The mentor IP guys (synopsis) confirmed that during babble condition, 
controller is stopped. Only recover is to restart completely, usbss reset,  
reinit epfifo table,  set the session. 

 
 --
 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 v2 2/7] usb: phy: dsps: adding usbphy driver for am33xx platform

[B, Ravi]

 +
 +  res = platform_get_resource_byname(pdev, IORESOURCE_MEM, phy_wkup);
 +  phy-phy_wkup = ioremap(res-start, resource_size(res));

 devm_ioremap?

 The phy_wakeup register is common across two instances of phy, 
 devm_ioremap_resource() will fail to map for twice for same register space.

   I've already told you the register can't be shared between devices like 
 this.
  BTW, you haven't replied to my request concerning your /proc/iomem 
 contents... 

I have missed that specific mail. The /proc/iomem does show this common 
register. As you suggest to create a third device for this shared register? I 
agree it can be done. 
Initially I thought this should be part of control module, but do we have 
control module frame work for every SoC(am335x)?
Every SoC has many such shared register to handle control/status logic for Soc 
specific IP(s)? 
If we create separate device node for each such registers, this will end up in 
adding more files to kernel. 

We should create single control module node for each SoC,

cm: control_module@44e1 {
compatible = ti,dsps-usbphy;
reg = 0x44e10648 0x4
 
   0x44e10XXX 0x4

reg-names = phy_wkup, mmc_control, xxxreg_ctrl, 
xxxreg_status;
id = 0;
};
Have common control module APIs to control these registers, can be used by 
other driver modules. 

--
Ravi B
--
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/7] usb: phy: dsps: adding usbphy driver for am33xx platform

[B, Ravi]

Typo.. corrected
The /proc/iomem does not show this common register

-Original Message-
From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-ow...@vger.kernel.org] 
On Behalf Of B, Ravi
Sent: Wednesday, June 12, 2013 12:51 PM
To: Sergei Shtylyov
Cc: ABRAHAM, KISHON VIJAY; linux-usb@vger.kernel.org; 
linux-ker...@vger.kernel.org; Balbi, Felipe
Subject: RE: [PATCH v2 2/7] usb: phy: dsps: adding usbphy driver for am33xx 
platform

 +
 +  res = platform_get_resource_byname(pdev, IORESOURCE_MEM, phy_wkup);
 +  phy-phy_wkup = ioremap(res-start, resource_size(res));

 devm_ioremap?

 The phy_wakeup register is common across two instances of phy, 
 devm_ioremap_resource() will fail to map for twice for same register space.

   I've already told you the register can't be shared between devices like 
 this.
  BTW, you haven't replied to my request concerning your /proc/iomem 
 contents... 

I have missed that specific mail. The /proc/iomem does not show this common 
register. As you suggest to create a third device for this shared register? I 
agree it can be done. 
Initially I thought this should be part of control module, but do we have 
control module frame work for every SoC(am335x)?
Every SoC has many such shared register to handle control/status logic for Soc 
specific IP(s)? 
If we create separate device node for each such registers, this will end up in 
adding more files to kernel. 

We should create single control module node for each SoC,

cm: control_module@44e1 {
compatible = ti,dsps-usbphy;
reg = 0x44e10648 0x4
 
   0x44e10XXX 0x4

reg-names = phy_wkup, mmc_control, xxxreg_ctrl, 
xxxreg_status;
id = 0;
};
Have common control module APIs to control these registers, can be used by 
other driver modules. 

--
Ravi B
--
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 v2 2/7] usb: phy: dsps: adding usbphy driver for am33xx platform

[B, Ravi]

Kishon

 Cc: linux-usb@vger.kernel.org; linux-ker...@vger.kernel.org; Balbi, Felipe
 Subject: Re: [PATCH v2 2/7] usb: phy: dsps: adding usbphy driver for am33xx 
 platform

 +
 +res = platform_get_resource_byname(pdev, IORESOURCE_MEM, phy_wkup);
 +phy-phy_wkup = ioremap(res-start, resource_size(res));

devm_ioremap?

The phy_wakeup register is common across two instances of phy, 
devm_ioremap_resource() will fail to map for twice for same register space.

 +if (IS_ERR(phy-phy_wkup))
 +return PTR_ERR(phy-phy_wkup);
 +
 +if (np)
 +of_property_read_u32(np, id, phy-id);

 Is this property documented somewhere?

Not it is not documented.

 +
 +phy-phy.dev= phy-dev;
 +phy-phy.label  = dsps-usbphy;
 +dsps_usbphy_power(dsps_phy-phy, 0);
 +dsps_phy-is_suspended = 1;

 Are you using this is_suspended anywhere else?

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

RE: [PATCH v1 0/9] adding dual instance and usb-phy support for am335x platform

[B, Ravi]

Felipe

 Subject: Re: [PATCH v1 0/9] adding dual instance and usb-phy support for 
 am335x platform

 Hi,

On Thu, May 23, 2013 at 11:31:19AM +0530, Ravi Babu wrote:
 This patch set series
 - adds dual musb instances support for am335x platform
 - adds phy-dsps-usb driver based on TI's gs70 driver
 - adds DT bindings for am33xx usb-phy
  - removed references to usb-nop-xceiv from musb

as Sergei pointed out, this would break some DaVinci/DA8xx platforms, so I'm 
dropping from it from my queue.

As I understand, already all musb glue platform drivers(dsps/davinci/da8xx) are 
changed to new usb_get_phy() API set.  
Currently the mainline code snippet as shown.

dsps/davinci/da8xx/xxx_musb_init() {
...
usb_nop_xceiv_register() 
..
musb-xceiv = usb_get_phy(USB_PHY_TYPE_USB2);
if (IS_ERR_OR_NULL(musb-xceiv)) {
ret = -EPROBE_DEFER;
goto fail;
}
..
}
Because of this all glue xxx_musb_init() will fail to get the phy without the 
phy-bindings for each controller. 
Without this patch series am335x musb will fail to get usb_phy(). Similarly phy 
support to be added for all davinci/da8xx
platform also. The usb_nop_xciev_xx() is dummy unused API here and hence 
removed from all glue in this patch series.

1) dsps platforms (am335x/dm81xx) series uses TI gs70 based phy 
This patch adds support for ths dsps phy driver at 
drivers/usb/phy/usb-dsps-phy.c 
2) omapl13x/da8xx series of soc uses different phy
Separate phy driver need to be added at drivers/usb/phy/usb-da8xx-phy.c
3) similarly all davinci series of soc uses separate TI-phy 
Separate phy driver need to added at drivers/usb/phy/usb-davinci-phy.c

The bindings of the respective usb-phy and controller need to done in DT or 
non-DT way.  
I can add usb-phy support for davinci/da8xx platform in similar way.

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

RE: [PATCH v1 2/9] usb: musb: nop: remove unused nop_xceiv_(un)register APIs from glue

[B, Ravi]

Sergei

 Subject: Re: [PATCH v1 2/9] usb: musb: nop: remove unused 
 nop_xceiv_(un)register APIs from glue
 Hello.
 On 23-05-2013 10:01, Ravi Babu wrote:

 removed unused nop xceiv (un_)register API's from all musb
 platform drivers

Since when are they unused?

Please refer to commit id 662dca54 : usb: otg: support for multiple 
transceivers by a single controller.
Usb_get_phy() is used to get the of phy used by controller, phy bindings are 
done through DT. 

 Signed-off-by: Ravi Babu ravib...@ti.com

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

RE: [PATCH v1 7/9] usb: musb: dsps: use get-usb-phy by phandle for multi instance

[B, Ravi]

 Subject: Re: [PATCH v1 7/9] usb: musb: dsps: use get-usb-phy by phandle for 
 multi instance

 On 23-05-2013 10:01, Ravi Babu wrote:

 In case of mutli instance support, use get-phy object using phandle to 
 return to repsective phy xceiv object for each instance

   Only respective and s/xceiv/transceiver/.

Ok. 

 Signed-off-by: Ravi Babu ravib...@ti.com

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

RE: [PATCH v1 8/9] usb: phy: dts: Adding usbphy DT bindings for am33xx

[B, Ravi]

Sergei


 +phy1: usbphy-gs70@44e10620 {
 +compatible = ti,dsps-usbphy;
 +reg = 0x44e10620 0x8
 +   0x44e10648 0x4;
 +reg-names = phy_ctrl,phy_wkup;
 +id = 0;
 +};
 +
 +phy2: usbphy-gs70@44e10628 {
 +compatible = ti,dsps-usbphy;
 +reg = 0x44e10628 0x8
 +   0x44e10648 0x4;

 The second register conflicts with phy1.

The two instances of phy uses common phy wakeup register.

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

RE: [PATCH v1 8/9] usb: phy: dts: Adding usbphy DT bindings for am33xx

[B, Ravi]

Subject: Re: [PATCH v1 8/9] usb: phy: dts: Adding usbphy DT bindings for am33xx

Hello.

On 05/23/2013 09:13 PM, B, Ravi wrote:


 +   phy1: usbphy-gs70@44e10620 {
 +   compatible = ti,dsps-usbphy;
 +   reg = 0x44e10620 0x8
 +  0x44e10648 0x4;
 +   reg-names = phy_ctrl,phy_wkup;
 +   id = 0;
 +   };
 +
 +   phy2: usbphy-gs70@44e10628 {
 +   compatible = ti,dsps-usbphy;
 +   reg = 0x44e10628 0x8
 +  0x44e10648 0x4;
 The second register conflicts with phy1.
 The two instances of phy uses common phy wakeup register.

 That's why there is a resource conflict. Have you actually tried to 
 instantiate the devices out of such tree?
This register should be declared somewhere above the PHYs I think...

I did not face any problem with this, I have tested both instances of phy used 
by dual instance controller, worked fine. 
What do you suggest, in case of common register which both phy have to use this 
for wakeup functionality.  
The DT should support this.  What do you suggest in such case?

--
Ravi B

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

RE: [PATCH v1 2/9] usb: musb: nop: remove unused nop_xceiv_(un)register APIs from glue

[B, Ravi]

Subject: Re: [PATCH v1 2/9] usb: musb: nop: remove unused 
nop_xceiv_(un)register APIs from glue

Hello.

On 05/23/2013 09:07 PM, B, Ravi wrote:

 removed unused nop xceiv (un_)register API's from all musb platform 
 drivers
 Since when are they unused?
 Please refer to commit id 662dca54 : usb: otg: support for multiple 
 transceivers by a single controller.
 Usb_get_phy() is used to get the of phy used by controller, phy bindings are 
 done through DT.

Why are you sure that all these platforms support DT (in all 
 configurations)?
 It seems to me that you're simply breaking the patched glue layers with this 
 patch.
 I'll let Felipe decide the fate of this patch though...

You are correct, the bindings of phy and controller need not to done through DT 
alone, there is a saperate API
Phy API's available for such bindings done in respective board platform files. 


 Signed-off-by: Ravi Babu ravib...@ti.com
 --
 Ravi B

WBR, 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: musb: gadget: fix enumeration on heavy-loaded systems

[B, Ravi]

Ruslan

  Subject: [PATCH] usb: musb: gadget: fix enumeration on heavy-loaded
  systems
 
  From musb point of view, the Address Assignment sequence during
  device enumeration is next:
   - first ep0 interrupt:
* read the address from USB_REQ_SET_ADDRESS request
* set up CSR0L.DataEnd bit (that is ACK
  signalization for the host)
   - second ep0 interrupt:
* indicates that the request completed successfully
* set up musb device address
  Now musb device should answer to this address
 
  From the host perspective, if peripheral device acquires
  SET_ADDRESS request, it now may be accessed only using that address.
  However, on heavy loaded systems, time between first and
  second musb ep0 interrupts may be too long and musb controller
  misses requests between.
 
  What is meant by heavily loaded system? Is the device is heavily loaded
 during enumeration stage? Why second ep0 interrupt is too long? whether
 interrupt occurrence to interrupt service is taking too long?
 
 I mean production system with aggressive power management and tens of
 interrupt sources.
 On such systems and in low CPU frequency case, you may meet condition
 when time between
 IRQ firing and ISR entering is increased in few times.
 
 In particular case of OMAP4 where I met this issue, time between first
 and second ep0 interrupt
 sometimes may be up to 800-900 uS and in this case the USB30CV test fails.
 If this time is 200-300 uS, the test successfully passes.
 
 Unfortunately, this time is not predictable and depends on many factors so
 this patch ensures we change the address as soon as sent ACK to the host.
 
 
  As result, device enumeration may be
  unsuccessful. This can be checked on USB3.0 Host and
  using USB3.0 test suite (from usb.org) running ch9 tests
  for USB2.0 devices.
 
  You mean the usb2.0 musb controller (in device mode) connected to USB3.0
 host?
 
 Correct. USB2.0 musb controller in device mode, connected to USB3.0
 host that runs
 USB30CV utility for USB2.0 devices
 
 
  Usually 'Addressed state/TD9.1: Device Descriptor Test' will fail
 
  The fix consists in checking CSR0L.DataEnd state and assigning
  the device address in the first ep0 interrupt handling, so
  delay is as minimal as possible
 
  Signed-off-by: Ruslan Bilovol ruslan.bilo...@ti.com
  ---
   drivers/usb/musb/musb_gadget_ep0.c |   31
 +++
   1 file changed, 31 insertions(+)
 
  diff --git a/drivers/usb/musb/musb_gadget_ep0.c
  b/drivers/usb/musb/musb_gadget_ep0.c
  index c9c1ac4..59bc5a5 100644
  --- a/drivers/usb/musb/musb_gadget_ep0.c
  +++ b/drivers/usb/musb/musb_gadget_ep0.c
  @@ -885,6 +885,37 @@ stall:
   finish:
musb_writew(regs, MUSB_CSR0,
musb-ackpend);
  +
  + /*
  +  * If we are at end of SET_ADDRESS
 sequence,
  +  * update the address immediately if
 possible,
  +  * otherwise we may miss packets between
  +  * sending ACK from musb side and musb's
 next
  +  * interrupt handler firing (in which we
 update
  +  * the address). At least this fixes next
  +  * USB2.0 ch9 test of USB30CV utility:
  +  * Addressed state - Device Descriptor
 test
  +  */
  + if (musb-set_address  (musb-ackpend 
  +
 MUSB_CSR0_P_DATAEND) 
  + (musb-ep0_state ==
  + MUSB_EP0_STAGE_STATUSIN))
 {
  + u16 ack_delay = 500;
  +
  + while ((musb_readw(regs,
 MUSB_CSR0) 
  +
 MUSB_CSR0_P_DATAEND) 
  + --ack_delay) {
  + cpu_relax();
  + udelay(1);
  + }
  +

No need to loop here. It is self clearing bit.

  + if (ack_delay) {
  + musb-set_address =
 false;
  + musb_writeb(mbase,
 MUSB_FADDR,
  + musb-
 address);
  + }

Setting the address before status phase could lead to dropping of status  
packet(IN token) by controller, because the status phase is addressed to device 
with zero address by host, but device controller already changed to new address.
I believe above delay loop is saving you in this case.

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

RE: [PATCH] usb: musb: gadget: fix enumeration on heavy-loaded systems

[B, Ravi]

 -Original Message-
 From: linux-usb-ow...@vger.kernel.org [mailto:linux-usb-
 ow...@vger.kernel.org] On Behalf Of Bilovol, Ruslan
 Sent: Tuesday, April 16, 2013 9:12 PM
 To: Balbi, Felipe; gre...@linuxfoundation.org; linux-usb@vger.kernel.org;
 linux-ker...@vger.kernel.org
 Subject: [PATCH] usb: musb: gadget: fix enumeration on heavy-loaded
 systems
 
 From musb point of view, the Address Assignment sequence during
 device enumeration is next:
  - first ep0 interrupt:
   * read the address from USB_REQ_SET_ADDRESS request
   * set up CSR0L.DataEnd bit (that is ACK
 signalization for the host)
  - second ep0 interrupt:
   * indicates that the request completed successfully
   * set up musb device address
 Now musb device should answer to this address
 
 From the host perspective, if peripheral device acquires
 SET_ADDRESS request, it now may be accessed only using that address.
 However, on heavy loaded systems, time between first and
 second musb ep0 interrupts may be too long and musb controller
 misses requests between. 

What is meant by heavily loaded system? Is the device is heavily loaded during 
enumeration stage? Why second ep0 interrupt is too long? whether interrupt 
occurrence to interrupt service is taking too long? 

As result, device enumeration may be
 unsuccessful. This can be checked on USB3.0 Host and
 using USB3.0 test suite (from usb.org) running ch9 tests
 for USB2.0 devices.

You mean the usb2.0 musb controller (in device mode) connected to USB3.0 host? 

 Usually 'Addressed state/TD9.1: Device Descriptor Test' will fail
 
 The fix consists in checking CSR0L.DataEnd state and assigning
 the device address in the first ep0 interrupt handling, so
 delay is as minimal as possible
 
 Signed-off-by: Ruslan Bilovol ruslan.bilo...@ti.com
 ---
  drivers/usb/musb/musb_gadget_ep0.c |   31 +++
  1 file changed, 31 insertions(+)
 
 diff --git a/drivers/usb/musb/musb_gadget_ep0.c
 b/drivers/usb/musb/musb_gadget_ep0.c
 index c9c1ac4..59bc5a5 100644
 --- a/drivers/usb/musb/musb_gadget_ep0.c
 +++ b/drivers/usb/musb/musb_gadget_ep0.c
 @@ -885,6 +885,37 @@ stall:
  finish:
   musb_writew(regs, MUSB_CSR0,
   musb-ackpend);
 +
 + /*
 +  * If we are at end of SET_ADDRESS sequence,
 +  * update the address immediately if possible,
 +  * otherwise we may miss packets between
 +  * sending ACK from musb side and musb's next
 +  * interrupt handler firing (in which we update
 +  * the address). At least this fixes next
 +  * USB2.0 ch9 test of USB30CV utility:
 +  * Addressed state - Device Descriptor test
 +  */
 + if (musb-set_address  (musb-ackpend 
 + MUSB_CSR0_P_DATAEND) 
 + (musb-ep0_state ==
 + MUSB_EP0_STAGE_STATUSIN)) {
 + u16 ack_delay = 500;
 +
 + while ((musb_readw(regs, MUSB_CSR0) 
 + MUSB_CSR0_P_DATAEND) 
 + --ack_delay) {
 + cpu_relax();
 + udelay(1);
 + }
 +
 + if (ack_delay) {
 + musb-set_address = false;
 + musb_writeb(mbase, MUSB_FADDR,
 + musb-address);
 + }
 + }
 +
   musb-ackpend = 0;
   }
   }

--
Ravi B
--
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 00/11] usb: musb: add back support for host mode

[B, Ravi]

Daniel

 Hi all,
 
 here are some patches to separate the HCD and gadget part of the musb
 driver so they can be deselected in Kconfig. They also make the driver
 keep track of the configured port mode that is set from DT, so the
 actual runtime configuration can be selected dynamically.
 
 One thing that is still broken is that once pm_suspend() was called on
 a musb device on a USB disconnect, the port won't wake up again when a
 device is plugged back in. 

This could be due to SESSION bit removal when root port is disconnected in 
otg_timer function.

I doubt this is related to my patches, but I
 might be wrong. If that effect rings a bell to anyone, please let me
 know.
 
--
Ravi B

--
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 00/11] usb: musb: add back support for host mode

[B, Ravi]

Daniel

 On 08.04.2013 09:57, B, Ravi wrote:
  Hi all,
 
  here are some patches to separate the HCD and gadget part of the musb
  driver so they can be deselected in Kconfig. They also make the driver
  keep track of the configured port mode that is set from DT, so the
  actual runtime configuration can be selected dynamically.
 
  One thing that is still broken is that once pm_suspend() was called on
  a musb device on a USB disconnect, the port won't wake up again when a
  device is plugged back in.
 
  This could be due to SESSION bit removal when root port is disconnected
 in otg_timer function.
 
 Not sure if we are thinking about the same details, but after debuging
 this a further, turns out that musb_platform_try_idle() eventually
 switches off the entire controller, which then leads to DRVBUS going low
 on the board. That, in turn, prevents the interrupt from being triggered
 on reconnect, because the host port is not powered anymore.
 
 I don't know yet how to cope with that, but for now, I simply disabled
 the call from musb_stage0_irq() to musb_platform_try_idle() locally.

The otg_timer() gets invoked, which removes the session when no device 
connected to root controller, this is required in otg or dual role mode (Not 
for host-only mode). Because otg state is un-defined till user's connected 
a-side of b-side of cable. 

--
Ravi B
--
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 00/11] usb: musb: add back support for host mode

[B, Ravi]

Felipe

 Hi,
 
 On Mon, Apr 08, 2013 at 12:25:38PM +0200, B, Ravi wrote:
  Daniel
 
   On 08.04.2013 09:57, B, Ravi wrote:
Hi all,
   
here are some patches to separate the HCD and gadget part of the
 musb
driver so they can be deselected in Kconfig. They also make the
 driver
keep track of the configured port mode that is set from DT, so the
actual runtime configuration can be selected dynamically.
   
One thing that is still broken is that once pm_suspend() was called
 on
a musb device on a USB disconnect, the port won't wake up again
 when a
device is plugged back in.
   
This could be due to SESSION bit removal when root port is
 disconnected
   in otg_timer function.
  
   Not sure if we are thinking about the same details, but after debuging
   this a further, turns out that musb_platform_try_idle() eventually
   switches off the entire controller, which then leads to DRVBUS going
 low
   on the board. That, in turn, prevents the interrupt from being
 triggered
   on reconnect, because the host port is not powered anymore.
  
   I don't know yet how to cope with that, but for now, I simply disabled
   the call from musb_stage0_irq() to musb_platform_try_idle() locally.
 
  The otg_timer() gets invoked, which removes the session when no device
  connected to root controller, this is required in otg or dual role
  mode (Not for host-only mode). Because otg state is un-defined till
  user's connected a-side of b-side of cable.
 
 Embedded hosts might also want this to save some power while no devices
 are connected. I guess most of those devices would rely on SRP or on a
 polling method to enable VBUS. 

Yes, in case of true otg, SRP wakesup the device to enable VBUS.

By 'polling' I mean that e.g. every 2
 seconds turn vbus on, if no device are enumerated in 200ms, then sleep
 for another 2 seconds.

Yes, we had this workaround mechanism enabled on some earlier davinci platform 
by setting the session periodically to check any device connected for host or 
otg/dual-role controller to save VBUS power when no device connected.

 
 Or something similar. I know of some products (cellphones) which will
 only switch Vbus on when you open e.g. file manager.
 

--
Ravi B
--
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 09/11] usb: musb: re-introduce musb-port_mode

[B, Ravi]

Daniel

 Subject: [PATCH v2 09/11] usb: musb: re-introduce musb-port_mode
 
 Define MUSB_PORT_MODE_{HOST,GADGET,DUAL_ROLE} and store the
 platform-specified value in struct musb.
 
 Note that MUSB_PORT_MODE_HOST has to be set to 1 in order to match
 existing device tree bindings which are already documented but in fact
 unusued.
 
 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  drivers/usb/musb/musb_core.c | 1 +
  drivers/usb/musb/musb_core.h | 7 +++
  2 files changed, 8 insertions(+)
 
 diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
 index fbcf5cb..2640d25 100644
 --- a/drivers/usb/musb/musb_core.c
 +++ b/drivers/usb/musb/musb_core.c
 @@ -1821,6 +1821,7 @@ musb_init_controller(struct device *dev, int nIrq,
 void __iomem *ctrl)
   musb-board_set_power = plat-set_power;
   musb-min_power = plat-min_power;
   musb-ops = plat-platform_ops;
 + musb-port_mode = plat-mode;

I assume plat-mode is fetched from DT. You may need to over-ride mode field 
from DT for host-only or gadget-only configuration through menuconfig.

Some thing like or better than this
+#ifdef CONFIG_MUSB_DUAL_ROLE
+   Musb-port_mode = plat-mode;
+#else
Force it to host or gadget only configuration
#endif

 
   /* The musb_platform_init() call:
*   - adjusts musb-mregs
 diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
 index 6b65847..174c097 100644
 --- a/drivers/usb/musb/musb_core.h
 +++ b/drivers/usb/musb/musb_core.h
 @@ -77,6 +77,12 @@ struct musb_ep;
  #define is_peripheral_active(m)  (!(m)-is_host)
  #define is_host_active(m)((m)-is_host)
 
 +enum {
 + MUSB_PORT_MODE_HOST = 1,
 + MUSB_PORT_MODE_GADGET,
 + MUSB_PORT_MODE_DUAL_ROLE,
 +};
 +
  #ifdef CONFIG_PROC_FS
  #include linux/fs.h
  #define MUSB_CONFIG_PROC_FS
 @@ -356,6 +362,7 @@ struct musb {
 
   u8  min_power;  /* vbus for periph, in mA/2 */
 
 + int port_mode;  /* MUSB_PORT_MODE_* */
   boolis_host;
 
   int a_wait_bcon;/* VBUS timeout in msecs */
 --
 1.8.1.4
 
 --
 To unsubscribe from this list: send the line unsubscribe linux-usb in
 the body of a message to majord...@vger.kernel.org
 More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
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 07/11] usb: musb: add Kconfig options for HOST, GAGDET or DUAL_ROLE modes

[B, Ravi]

Daniel

 Subject: [PATCH v2 07/11] usb: musb: add Kconfig options for HOST, GAGDET
 or DUAL_ROLE modes
 
 This makes building the actual object files optional to the selected
 mode, which saves users who know which kind of USB mode support they
 need some binary size.
 
 Unimplemented functions are stubbed out with static inline functions.
 
 Signed-off-by: Daniel Mack zon...@gmail.com
 ---
  drivers/usb/musb/Kconfig   | 29 +
  drivers/usb/musb/Makefile  | 10 --
  drivers/usb/musb/musb_gadget.h | 21 +
  drivers/usb/musb/musb_host.h   | 29 +++--
  4 files changed, 85 insertions(+), 4 deletions(-)
 
 diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
 index 47442d3..aab1568 100644
 --- a/drivers/usb/musb/Kconfig
 +++ b/drivers/usb/musb/Kconfig
 @@ -28,6 +28,35 @@ config USB_MUSB_HDRC
  if USB_MUSB_HDRC
 
  choice
 + bool MUSB Mode Selection
 + default USB_MUSB_DUAL_ROLE if (USB  USB_GADGET)
 + default USB_MUSB_HOST if (USB  !USB_GADGET)
 + default USB_MUSB_GADGET if (!USB  USB_GADGET)
 +
 +config USB_MUSB_HOST
 + bool Host only mode
 + depends on USB
 + help
 +   Select this when you want to use MUSB in host mode only,
 +   thereby the gadget feature will be regressed.
 +
 +config USB_MUSB_GADGET
 + bool Gadget only mode
 + depends on USB_GADGET
 + help
 +   Select this when you want to use MUSB in gadget mode only,
 +   thereby the host feature will be regressed.
 +
 +config USB_MUSB_DUAL_ROLE
 + bool Dual Role mode
 + depends on (USB  USB_GADGET)
 + help
 +   This is the default mode of working of MUSB controller where
 +   both host and gadget features are enabled.
 +
 +endchoice

How do you cater for multi-instance support? Where one controller to force as 
host and other as device (similar to am33x beagle configuration).
In general for all combination possible like host, host host, device 
device, host dual, dual etc.

--
Ravi B
--
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 07/11] usb: musb: add Kconfig options for HOST, GAGDET or DUAL_ROLE modes

[B, Ravi]

Hi Daniel

 
 On 08.04.2013 12:42, B, Ravi wrote:
  Subject: [PATCH v2 07/11] usb: musb: add Kconfig options for HOST,
 GAGDET
  or DUAL_ROLE modes
 
  This makes building the actual object files optional to the selected
  mode, which saves users who know which kind of USB mode support they
  need some binary size.
 
  Unimplemented functions are stubbed out with static inline functions.
 
  Signed-off-by: Daniel Mack zon...@gmail.com
  ---
   drivers/usb/musb/Kconfig   | 29 +
   drivers/usb/musb/Makefile  | 10 --
   drivers/usb/musb/musb_gadget.h | 21 +
   drivers/usb/musb/musb_host.h   | 29 +++--
   4 files changed, 85 insertions(+), 4 deletions(-)
 
  diff --git a/drivers/usb/musb/Kconfig b/drivers/usb/musb/Kconfig
  index 47442d3..aab1568 100644
  --- a/drivers/usb/musb/Kconfig
  +++ b/drivers/usb/musb/Kconfig
  @@ -28,6 +28,35 @@ config USB_MUSB_HDRC
   if USB_MUSB_HDRC
 
   choice
  +  bool MUSB Mode Selection
  +  default USB_MUSB_DUAL_ROLE if (USB  USB_GADGET)
  +  default USB_MUSB_HOST if (USB  !USB_GADGET)
  +  default USB_MUSB_GADGET if (!USB  USB_GADGET)
  +
  +config USB_MUSB_HOST
  +  bool Host only mode
  +  depends on USB
  +  help
  +Select this when you want to use MUSB in host mode only,
  +thereby the gadget feature will be regressed.
  +
  +config USB_MUSB_GADGET
  +  bool Gadget only mode
  +  depends on USB_GADGET
  +  help
  +Select this when you want to use MUSB in gadget mode only,
  +thereby the host feature will be regressed.
  +
  +config USB_MUSB_DUAL_ROLE
  +  bool Dual Role mode
  +  depends on (USB  USB_GADGET)
  +  help
  +This is the default mode of working of MUSB controller where
  +both host and gadget features are enabled.
  +
  +endchoice
 
  How do you cater for multi-instance support? Where one controller to
 force as host and other as device (similar to am33x beagle configuration).
  In general for all combination possible like host, host host, device
 device, host dual, dual etc.
 
 The actual mode chosen for an instance is passed in via pdata/DT. The
 Kconfig options only exist in order to stub out code that certainly
 isn't needed for a particular platform. IOW, to reduce the binary size.
 
 So the beagleboard will have to select the DUAL_ROLE config parameters,
 where other board (like mine) can go for the HOST only option.
 
 If a user selects a mode as runtime parameter that is stubbed out by the
 chosen config, it will simply don't do anything. Just as with a driver
 that is referenced from DT but not compiled in. I don't think we should
 be over-smart here and mess with the configured platform data. If the
 platform data is wrong, it has to be fixed anyway. Felipe, do you agree?
 
 

I understand, this is for binary code reduction. But in multi instance case 
force host/device config option shall be selected only when both controller 
need to configured as host only or device only. like host, host or device, 
device. In any other case dual-role need to be chosen.
You can explain in help section of this Kconfig option.

--
Ravi B
--
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 09/11] usb: musb: re-introduce musb-port_mode

[B, Ravi]

Daniel
 On 08.04.2013 12:39, B, Ravi wrote:
  Subject: [PATCH v2 09/11] usb: musb: re-introduce musb-port_mode
 
  Define MUSB_PORT_MODE_{HOST,GADGET,DUAL_ROLE} and store the
  platform-specified value in struct musb.
 
  Note that MUSB_PORT_MODE_HOST has to be set to 1 in order to match
  existing device tree bindings which are already documented but in fact
  unusued.
 
  Signed-off-by: Daniel Mack zon...@gmail.com
  ---
   drivers/usb/musb/musb_core.c | 1 +
   drivers/usb/musb/musb_core.h | 7 +++
   2 files changed, 8 insertions(+)
 
  diff --git a/drivers/usb/musb/musb_core.c
 b/drivers/usb/musb/musb_core.c
  index fbcf5cb..2640d25 100644
  --- a/drivers/usb/musb/musb_core.c
  +++ b/drivers/usb/musb/musb_core.c
  @@ -1821,6 +1821,7 @@ musb_init_controller(struct device *dev, int nIrq,
  void __iomem *ctrl)
 musb-board_set_power = plat-set_power;
 musb-min_power = plat-min_power;
 musb-ops = plat-platform_ops;
  +  musb-port_mode = plat-mode;
 
  I assume plat-mode is fetched from DT.
 
 Yes, that's already done in the current mainline. The problem is that
 this value is not used anywhere, though.
 
  You may need to over-ride mode field from DT for host-only or gadget-
 only configuration through menuconfig.
 
 As stated in the other mail, I don't think this is a good idea at all.
 The config chooses which parts of the kernel you want to build in, the
 runtime config lets you select the actual mode. Mixing them up just
 makes it much harder for people to understand what's going on.
 

My concern is , what if user selects HOST only mode thru menu config and 
provide DT port-mode bindings wrongly. 

--
Ravi B

--
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/5] usb: musb: am335x support

[B, Ravi]

Felipe

 
 Hi,
 
 On Wed, Apr 03, 2013 at 02:43:00PM +0200, Daniel Mack wrote:
  On 03.04.2013 14:04, Felipe Balbi wrote:
   On Wed, Apr 03, 2013 at 02:00:23PM +0200, Daniel Mack wrote:
 
   Felipe, could you explain the background on how the dsps driver is
   supposed to work in host mode at boot time with the rework of the
 driver
   you did for 3.7? It might just be me not understanding the rationale
   behind all these changes, but appearantly, I'm not the only one who's
   affected by that.
  
   right, so the idea with that was to drop the huge amount of ifdeferry
   hack from the MUSB driver. It would be great if someone would send
   *CLEAN* patches adding Kconfig-based role choices again.
 
  Are Kconfig-based rules really what we want here after all? Wouldn't
  run-time configured settings make much more sense, considering that
 
 we need both. Say that you want to build a product with MUSB hardwired
 as host, why would you enable gadget framework ?
 
 I can think of at least am335x where this would be perfectly plausible
 (no EHCI available, only MUSB).
 
  people might want to run a single kernel image on multiple platforms? I
  believe it should be up to the DT to define the actual hardware wiring.
 
 Right, for runtime decision Ravi pointed me to a patch implementing that
 (Ravi, could you post it by any chance as RFC ?) which we could start a
 discussion and hopefully merge for v3.11

Ok. 

--
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent

   On Thu, Mar 28, 2013 at 08:53:03PM +0800, Ming Lei wrote:
On Thu, Mar 28, 2013 at 8:30 PM, B, Ravi ravib...@ti.com wrote:
 For example, in one iteration I have observed, the time taken by
 uvc_video_decode_isoc() was 2175 usec. In this maximum amount of
 time was consumed by uvc_video_decode_data() around 1792 usec.
   
uvc_video_decode_data() is basically a memcpy() from coherent buffer
 to
normal buffer.
  
   if that's the case, this should show, right ?
  
   Maybe not, it might be worse(per my previous test on Pandaboard A1) to
   change to DMA streaming buffer, since DMA unmapping has become
   expensive too for reading from device after invalidating buffer is
 added
   to handle speculative prefetching, and before that, DMA unmapping was
   basically a nop on ARM.
  
   But you guys can do the test again, or do some analysis about such
 slow
   memcpy() on coherent buffer.
 
  Since the uvc_video_complete() callback handler called from interrupt
  context, video post processing or memcpy can be deferred to tasklet or
  bottom half, rather than doing it in interrupt context.
 
 If that's the only way to fix the issue, yes. However, given your really
 poor
 memcpy() performances, I don't think you will be able to sustain the
 incoming
 video bandwidth. The driver would soon run out of URBs.

I agree with you, let me check whether memcpy is the bottle here, I will try to 
get profile info on this. But in general it would be good to move post 
processing to bottom half which helps to reduce interrupt latency.

Thanks

--
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

 
 Laurent
 
On Thu, Mar 28, 2013 at 08:53:03PM +0800, Ming Lei wrote:
 On Thu, Mar 28, 2013 at 8:30 PM, B, Ravi ravib...@ti.com wrote:
  For example, in one iteration I have observed, the time taken by
  uvc_video_decode_isoc() was 2175 usec. In this maximum amount of
  time was consumed by uvc_video_decode_data() around 1792 usec.

 uvc_video_decode_data() is basically a memcpy() from coherent
 buffer
  to
 normal buffer.
   
if that's the case, this should show, right ?
   
Maybe not, it might be worse(per my previous test on Pandaboard A1)
 to
change to DMA streaming buffer, since DMA unmapping has become
expensive too for reading from device after invalidating buffer is
  added
to handle speculative prefetching, and before that, DMA unmapping
 was
basically a nop on ARM.
   
But you guys can do the test again, or do some analysis about such
  slow
memcpy() on coherent buffer.
  
   Since the uvc_video_complete() callback handler called from interrupt
   context, video post processing or memcpy can be deferred to tasklet or
   bottom half, rather than doing it in interrupt context.
 
  If that's the only way to fix the issue, yes. However, given your really
  poor
  memcpy() performances, I don't think you will be able to sustain the
  incoming
  video bandwidth. The driver would soon run out of URBs.
 
 I agree with you, let me check whether memcpy is the bottle here, 

typo mistake, read as bottle-neck

I will
 try to get profile info on this. But in general it would be good to move
 post processing to bottom half which helps to reduce interrupt latency.
 
 Thanks
 
 --
 Ravi B
 --
 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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent


Since the uvc_video_complete() callback handler called from
 interrupt
context, video post processing or memcpy can be deferred to tasklet
 or
bottom half, rather than doing it in interrupt context.
  
   If that's the only way to fix the issue, yes. However, given your
 really
   poor
   memcpy() performances, I don't think you will be able to sustain the
   incoming
   video bandwidth. The driver would soon run out of URBs.
 
  I agree with you, let me check whether memcpy is the bottle here,
 
 typo mistake, read as bottle-neck
 
 I will
  try to get profile info on this. But in general it would be good to move
  post processing to bottom half which helps to reduce interrupt latency.
 

You are correct, I have profiled, the timing I have posted in previous mail are 
due to the memcpy() in uvc_video_decode_data().  Which is the bottle-neck and 
consumes most of the time.

Regards
Ravi B

 
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent/Ming Lei

 
 Since the uvc_video_complete() callback handler called from
  interrupt
 context, video post processing or memcpy can be deferred to
 tasklet
  or
 bottom half, rather than doing it in interrupt context.
   
If that's the only way to fix the issue, yes. However, given your
  really
poor
memcpy() performances, I don't think you will be able to sustain the
incoming
video bandwidth. The driver would soon run out of URBs.
  
   I agree with you, let me check whether memcpy is the bottle here,
 
  typo mistake, read as bottle-neck
 
  I will
   try to get profile info on this. But in general it would be good to
 move
   post processing to bottom half which helps to reduce interrupt latency.
  
 
 You are correct, I have profiled, the timing I have posted in previous
 mail are due to the memcpy() in uvc_video_decode_data().  Which is the
 bottle-neck and consumes most of the time.

There is an improvement in timing after defining CONFIG_DMA_NONCOHERENT, the 
coherent memory access is very slow leads to this issue. 

#ifndef CONFIG_DMA_NONCOHERENT
stream-urb_buffer[i] = usb_alloc_coherent(
stream-dev-udev, stream-urb_size,
gfp_flags | __GFP_NOWARN, stream-urb_dma[i]);
#else
stream-urb_buffer[i] =
kmalloc(stream-urb_size, gfp_flags | __GFP_NOWARN);
#endif

The mentor host controller (driver/usb/musb) puts all overhead on SW to 
schedule the next urb, unlike like ehci/xhci where the multiple urbs (TDs) can 
be queued and HW executes the transfers on the bus without SW intervention. 
In case of musb host controller, the SW has to program the urb one by 
one, hence it is critical that urb completion callback called in interrupt 
context must be very thin. 

--
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

 
 Laurent
 
 
  Hi Ravi,
 
  On Wednesday 27 March 2013 10:43:24 B, Ravi wrote:
   Hi
  
   I am observing issue while streaming video from usb camera connected
 to
  host
   controller based on mentor graphics. The issue is root caused that
 there
   are huge SOF gaps seen between the two isochronous IN token issued by
  host
   controller. This is due to fact, significant amount of time is spent
 in
  uvc
   callback function handler. Due to this next request programmed is
  delayed
   leads to this failure of video streaming. I have measured time taken
 by
  uvc
   callback function is in range minimum of 11 microsecond to maximum of
  3318
   microsecond.   Looks like callback handler doing some processing and
  takes
   more time to return rather than giveback immediately. Need your help
 to
   understand why uvc callback handler takes much time, if it does any
   processing can it move to different task context and return
 immediately,
   this will reduce the latency.
 
  I'm surprised by that large delay. A quick look at the UVC URB
 completion
  handler (I assume you're talking about the host driver, not the gadget
  driver)
 
 Thanks for reply, Yes, I am using the musb host controller driver
 (driver/usb/musb).
 
  doesn't show any significant source of delay. You will likely need to
  profile
  the code to find out where the problem comes from.
 
 I have profiled by adding timestamp before and after the
 usb_hcd_giveback_urb() @ drivers/usb/musb/musb_host.c and found the
 microsecond delay is ranging from 11usec to 3318usec while capturing video
 stream frames of resolution 640x480 from usb camera.
 
 static void musb_giveback(struct musb *musb, struct urb *urb, int status)
  __releases(musb-lock)
  __acquires(musb-lock)
  {
 +   struct timeval st, et, t;
 +   int is_isoc;
 .
 +   if (is_isoc)
 +   do_gettimeofday(st); /* get start time */
 usb_hcd_giveback_urb(musb_to_hcd(musb), urb, status);
 +   if (is_isoc) {
 +   do_gettimeofday(et); /* get end time */
 +   t.tv_sec = et.tv_sec - st.tv_sec;
 +   t.tv_usec = et.tv_usec - st.tv_usec;
 +   gtime[gIndex++ % MAX_LOG_TIME] = t; /* compute diff time
 */
 +   }
 
 Anything I miss here?
 
 You should also make
  sure
  that no other IRQ handler on the system keeps IRQs disabled for a long
  time.

Some more debugging, Most of the time spend in stream-decode() function in 
uvc_video_complete() callback handler.

--
Ravi B

--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent

 On Thursday 28 March 2013 06:48:09 B, Ravi wrote:
   On Wednesday 27 March 2013 10:43:24 B, Ravi wrote:
   Hi
  
   I am observing issue while streaming video from usb camera connected
 to
   host controller based on mentor graphics. The issue is root caused
 that
   there are huge SOF gaps seen between the two isochronous IN token
 issued
   by host controller. This is due to fact, significant amount of time
 is
   spent in uvc callback function handler. Due to this next request
   programmed is delayed leads to this failure of video streaming. I
 have
   measured time taken by uvc callback function is in range minimum of
 11
   microsecond to maximum of 3318 microsecond. Looks like callback
 handler
   doing some processing and takes more time to return rather than
 giveback
   immediately. Need your help to understand why uvc callback handler
 takes
   much time, if it does any processing can it move to different task
   context and return immediately, this will reduce the latency.
  
   I'm surprised by that large delay. A quick look at the UVC URB
 completion
   handler (I assume you're talking about the host driver, not the
 gadget
   driver)
  
   Thanks for reply, Yes, I am using the musb host controller driver
   (driver/usb/musb).
  
   doesn't show any significant source of delay. You will likely need to
   profile the code to find out where the problem comes from.
  
   I have profiled by adding timestamp before and after the
   usb_hcd_giveback_urb() @ drivers/usb/musb/musb_host.c and found the
   microsecond delay is ranging from 11usec to 3318usec while capturing
 video
   stream frames of resolution 640x480 from usb camera.
  
   static void musb_giveback(struct musb *musb, struct urb *urb, int
 status)
  
__releases(musb-lock)
__acquires(musb-lock)
{
  
   +   struct timeval st, et, t;
   +   int is_isoc;
   .
   +   if (is_isoc)
   +   do_gettimeofday(st); /* get start time */
  
   usb_hcd_giveback_urb(musb_to_hcd(musb), urb, status);
  
   +   if (is_isoc) {
   +   do_gettimeofday(et); /* get end time */
   +   t.tv_sec = et.tv_sec - st.tv_sec;
   +   t.tv_usec = et.tv_usec - st.tv_usec;
   +   gtime[gIndex++ % MAX_LOG_TIME] = t; /* compute diff
 time
   */
   +   }
  
   Anything I miss here?
  
   You should also make sure that no other IRQ handler on the system
 keeps
   IRQs disabled for a long time.
 
  Some more debugging, Most of the time spend in stream-decode() function
 
 That points to uvc_video_decode_isoc().

You are correct, that points to uvc_video_decode_isoc()

 
  in uvc_video_complete() callback handler.
 
 It's not very surprising, but doesn't tell where the problem comes from.
 As
 Ming Lei pointed out, slow access to coherent memory might be an
 explanation.
 Could you find out how the time is spent between uvc_video_decode_start(),
 uvc_video_decode_data() and uvc_video_decode_data() ? It might also be
 worth
 it timing the uvc_queue_next_buffer() calls, in case the function is
 delayed
 by spinlock contention (if you're running on an SMP system).

I did not dig further, I try to get this timing info.   

Quickly I tried another experiment, instead of calling stream-decode() in 
callback, initiated work thread, which perform stream-decode()  re-submit 
urb. This reduces the uvc_video_callback() time to 12usec, But after 900 urb 
completion, submit_urb() failed with -EBUSY error. Further I stopped debugging, 
something I may not be doing right at uvc level.

Thanks Laurent.

Regards
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent

  
   Some more debugging, Most of the time spend in stream-decode()
 function
 
  That points to uvc_video_decode_isoc().
 
 You are correct, that points to uvc_video_decode_isoc()
 
 
   in uvc_video_complete() callback handler.
 
  It's not very surprising, but doesn't tell where the problem comes from.
  As
  Ming Lei pointed out, slow access to coherent memory might be an
  explanation.
  Could you find out how the time is spent between
 uvc_video_decode_start(),
  uvc_video_decode_data() and uvc_video_decode_data() ? It might also be
  worth
  it timing the uvc_queue_next_buffer() calls, in case the function is
  delayed
  by spinlock contention (if you're running on an SMP system).
 
 I did not dig further, I try to get this timing info.
 
 Quickly I tried another experiment, instead of calling stream-decode() in
 callback, initiated work thread, which perform stream-decode()  re-
 submit urb. This reduces the uvc_video_callback() time to 12usec, But
 after 900 urb completion, submit_urb() failed with -EBUSY error. Further I
 stopped debugging, something I may not be doing right at uvc level.
 
 Thanks Laurent.

I profiled the uvc_video_decode_isoc(), the maximum time was taken by 
uvc_video_decode_data() function. 

For example, in one iteration I have observed, the time taken by 
uvc_video_decode_isoc() was 2175 usec. In this maximum amount of time was 
consumed by uvc_video_decode_data() around 1792 usec.

The function uvc_video_decode_data() was called in loop, below the time taken 
in each iteration. The total was around 1792 usec.

SUM(108, 59, 72, 57, 108, 58, 108, 58, 
72, 87, 72, 58, 108, 59, 82, 58, 
108,59, 72, 87, 74, 59, 109) = 1792 usec

let me know if you need any further info.

--
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Hi,

On Thu, Mar 28, 2013 at 9:27 PM, Felipe Balbi ba...@ti.com wrote:
On Thu, Mar 28, 2013 at 03:23:46PM +0200, Felipe Balbi wrote:
 Hi,

 On Thu, Mar 28, 2013 at 08:53:03PM +0800, Ming Lei wrote:
  On Thu, Mar 28, 2013 at 8:30 PM, B, Ravi ravib...@ti.com wrote:
  
   For example, in one iteration I have observed, the time taken by
   uvc_video_decode_isoc() was 2175 usec. In this maximum amount of
   time was consumed by uvc_video_decode_data() around 1792 usec.
 
  uvc_video_decode_data() is basically a memcpy() from coherent buffer to
  normal buffer.

 if that's the case, this should show, right ?

 Maybe not, it might be worse(per my previous test on Pandaboard A1) to
 change to DMA streaming buffer, since DMA unmapping has become
 expensive too for reading from device after invalidating buffer is added
 to handle speculative prefetching, and before that, DMA unmapping was
 basically a nop on ARM.

 But you guys can do the test again, or do some analysis about such slow
 memcpy() on coherent buffer.

Since the uvc_video_complete() callback handler called from interrupt context, 
video post processing or memcpy can be deferred to tasklet or bottom half, 
rather than doing it in interrupt context.

It is not odd to see I/O performance isn't very good on ARM...

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

usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Hi

I am observing issue while streaming video from usb camera connected to host 
controller based on mentor graphics. The issue is root caused that there are 
huge SOF gaps seen between the two isochronous IN token issued by host 
controller. This is due to fact, significant amount of time is spent in uvc 
callback function handler. Due to this next request programmed is delayed leads 
to this failure of video streaming. I have measured time taken by uvc callback 
function is in range minimum of 11 microsecond to maximum of 3318 microsecond.  
 Looks like callback handler doing some processing and takes more time to 
return rather than giveback immediately. Need your help to understand why uvc 
callback handler takes much time, if it does any processing can it move to 
different task context and return immediately, this will reduce the latency.

Appreciate your help.

Regards
Ravi B
--
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: usb video capture issue due to uvc_complete callback spends more time

[B, Ravi]

Laurent

 
 Hi Ravi,
 
 On Wednesday 27 March 2013 10:43:24 B, Ravi wrote:
  Hi
 
  I am observing issue while streaming video from usb camera connected to
 host
  controller based on mentor graphics. The issue is root caused that there
  are huge SOF gaps seen between the two isochronous IN token issued by
 host
  controller. This is due to fact, significant amount of time is spent in
 uvc
  callback function handler. Due to this next request programmed is
 delayed
  leads to this failure of video streaming. I have measured time taken by
 uvc
  callback function is in range minimum of 11 microsecond to maximum of
 3318
  microsecond.   Looks like callback handler doing some processing and
 takes
  more time to return rather than giveback immediately. Need your help to
  understand why uvc callback handler takes much time, if it does any
  processing can it move to different task context and return immediately,
  this will reduce the latency.
 
 I'm surprised by that large delay. A quick look at the UVC URB completion
 handler (I assume you're talking about the host driver, not the gadget
 driver)

Thanks for reply, Yes, I am using the musb host controller driver 
(driver/usb/musb). 

 doesn't show any significant source of delay. You will likely need to
 profile
 the code to find out where the problem comes from. 

I have profiled by adding timestamp before and after the usb_hcd_giveback_urb() 
@ drivers/usb/musb/musb_host.c and found the microsecond delay is ranging from 
11usec to 3318usec while capturing video stream frames of resolution 640x480 
from usb camera.

static void musb_giveback(struct musb *musb, struct urb *urb, int status)
 __releases(musb-lock)
 __acquires(musb-lock)
 {
+   struct timeval st, et, t;
+   int is_isoc;
.
+   if (is_isoc)
+   do_gettimeofday(st); /* get start time */
usb_hcd_giveback_urb(musb_to_hcd(musb), urb, status);
+   if (is_isoc) {
+   do_gettimeofday(et); /* get end time */
+   t.tv_sec = et.tv_sec - st.tv_sec;
+   t.tv_usec = et.tv_usec - st.tv_usec;
+   gtime[gIndex++ % MAX_LOG_TIME] = t; /* compute diff time */
+   }

Anything I miss here?

You should also make
 sure
 that no other IRQ handler on the system keeps IRQs disabled for a long
 time.


Regards
Ravi B

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

RE: [RFC PATCH 0/7] usb: musb: add driver for control module

[B, Ravi]

 Hi,
 
 On Tue, Jan 15, 2013 at 08:09:22PM +0530, kishon wrote:
  Hi Arnd,
  
  On Tuesday 15 January 2013 07:11 PM, Arnd Bergmann wrote:
  On Tuesday 15 January 2013, Kishon Vijay Abraham I wrote:
  Added a new driver for the usb part of control module. 
 This has an 
  API to power on the USB2 phy and an API to write to the mailbox 
  depending on whether MUSB has to act in host mode or in 
 device mode.
  
  Writing to control module registers for doing the above 
 task which 
  was previously done in omap glue and in omap-usb2 phy is removed.
  
  Also added the dt data to get MUSB working in OMAP platforms.
  This series has patches for both drivers and ARCH 
 folders, so If it 
  has to be split I'll do it.
  
  
  The series looks good to me, I just had a minor comment on 
 one patch.
  
  One a somewhat related topic, I wonder whether there are 
 any plans on 
  your side to change this driver to support multiple bus 
 glues to be 
  built for one kernel image. With a multiplatform kernel, 
 we may need 
  all of TUSB6010/OMAP2PLUS/DSPS/UX500 for instance.
  
  We don't have plans as of now. I actually don't expect any 
 changes in 
  the driver other than the Kconfig changes. Anyways the 
 probe of glue's 
  other than the platform it's running won't get called. right Felipe?

If understand correctly the control module driver used to configure the 
respective usb phy of SoC to respective usb modes using the common set of 
control module APIs. What if, if control module interface (register defintions) 
varies b/w different revision or spin of same type of SoCs, if usbphy type is 
changed. In this case whether the single instance of control module driver is 
good enough to cater of all cpu types of same SoC series ? 
Whether cpu_is_xxx() can be used to differentiate b/w different cpu types in CM 
driver?

 
 AFAICT there's nothing preventing those from being built 
 together as long as you don't use DMA (yeah, that's a touchy 
 subject still with MUSB).
 
 If there are any build breaks, please report them so bus glue 
 owners can fix. I see that at least the davinci folks need to 
 work a bit
 
 $ git grep -e mach\/ drivers/usb/musb/ 
 drivers/usb/musb/da8xx.c:#include mach/da8xx.h 
 drivers/usb/musb/davinci.c:#include mach/cputype.h 
 drivers/usb/musb/davinci.c:#include mach/hardware.h
 
 I'm adding Ravi B to the loop here for those.
 
 --
 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 0/5] usb: musb: am335x support

[B, Ravi]

 
 On 02.11.2012 17:31, Afzal Mohammed wrote:
  This series adds usb support to am335x SoC's found on boards like 
  Beagle Bone. Here only first instance is supported, as currently 
  multiple phy's of same type is not supported (am335x has two USB2 
  phy's).
 
 I'm testing these patches with an AM33xx board that has the 
 first musb port wired to an USB type A plug, but it doesn't 
 yet work for me.
 
 The messages I'm getting are:
 
 [1.219339] ehci_hcd: USB 2.0 'Enhanced' Host Controller 
 (EHCI) Driver
 [1.226568] ohci_hcd: USB 1.1 'Open' Host Controller (OHCI) Driver

Why ehci, ohci has selected for am335xx platform?

 [1.233444] usbcore: registered new interface driver uas
 [1.239066] Initializing USB Mass Storage driver...
 [1.244395] usbcore: registered new interface driver usb-storage
 [1.250732] USB Mass Storage support registered.
 [1.255573] musb-hdrc: version 6.0, ?dma?, otg (peripheral+host)
 [1.262671] musb-hdrc: ConfigData=0xde (UTMI-8, dyn FIFOs, bulk
 combine, bulk split, HB-ISO Rx, HB-ISO Tx, SoftConn)
 [1.262889] musb-hdrc: MHDRC RTL version 2.0
 [1.262907] musb-hdrc: setup fifo_mode 4
 [1.262940] musb-hdrc: 28/31 max ep, 16384/16384 memory
 
 So there is no host interface registered. I'm unsure on how 

The host interface will be registered when you load the gadget driver, (like 
insmod g_zero.ko ,etc).

 to fix this, and I didn't get an answer yet to that question 
 when I asked Felipe about how interface drivers like dsps are 
 supposed to act in order to get host mode back after the 
 recent musb cleanups.
 
 What type of hardware do you test this with? Does host mode 
 work for you?
 

By default only otg build is supported, you must insert the gadget module for 
each usb port in order to enable the host/device functionality. Have you 
checked with loading any gadget module (g_ether.ko, or g_ether.ko etc)? 

-RaviBabu
 
 Many thanks,
 Daniel
 
 
 --
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 v9 00/13] usb: musb: adding multi instance support

[B, Ravi]

 
 Hi,
 
 On Fri, Aug 31, 2012 at 04:39:46PM +0530, Ravi Babu wrote:
  This series of patches adds,
  a) Multi instances support in musb driver
  b) DT support for musb_dsps glue layer
  c) DT support for NOP transceiver
  
  AM33xx and TI81xx has dual musb controller and has two usb 
 PHY of same type.
  This patch series uses 'phandle' based API 
  devm_usb_get_phy_by_phandle() to get the PHY of same type. This API 
  support is being added by Kishon's patch discussed at [1]
  
  The series applies to felipe/master [1] branch
  + Vaibhav baseport patches on his tree at [4]
  + Kishon's multi phy patches on Felipe's branch 'xceiv'
  + Kishon's patch on phandle at [2]
  + Damodar's recent patch at [3] 
  + Ajay's  Damodar's patches at [5] and [6] included in 
 this series
  
  1. http://git.kernel.org/?p=linux/kernel/git/balbi/usb.git;a=summary
  2. http://marc.info/?l=linux-usbm=134070369306112w=2
  3. http://marc.info/?l=linux-usbm=134200284230689w=2
  4. 
  
 https://github.com/hvaibhav/am335x-linux/commits/am335x-upstream-stagi
  ng 5. http://marc.info/?l=linux-usbm=134200285530701w=2
  6. http://marc.info/?l=linux-usbm=134208820028625w=2
  
  Changes from v8:
  - included Sergei's comment, removing underscore in 
 device tree file
  - removed duplicated signoff from patches Changes from v7:
  - patches rebased on felipe/master branch  verified
  - included additional two patches 0001  0002 as part 
 of this series
which are already submitted [5]  [6] Changes from v6:
  - Removed parent_pdev to get glue and used 
 dev_get_getdrv() as per
Felipe's comment
  - use pr_debug() instead of pr_info() as per Felipe's 
 comment Changes 
  from v5:
  - Removed musb-id as per Felipe's comment
  - used nop_ida as per Felipe's comment Changes from v4:
  - Fixed Felipe's comment for adding EXPORT_SYMBOL_GPL()
  - Fixed Felipe's comment on using dev_set_mask() 
 Changes from v3:
  - Fixed Kishon's comment on removing id from phy struct and
removing unneeded #else part.
  Changes from v2:
  - Fixed Sergei's comment on not using address prefix in 
 musb_dsps
glue and nop transceiver dt dats.
  - Also removed the ti string in compatible property 
 for nop data.
  Changes from v1:
  - Defined musb_ida to manage core ids based on Felipe's comment
in [PATCH 01/11]
 
 I tried appliying this, but this doesn't apply. Please rebase 
 on my musb branch. Unfortunately there's no time anymore to 
 wait otherwise the entire musb branch will miss this merge window.
 
 I'm sorry

Felipe, patch set is ready, I will re-send the patches shortly by today.

 
 --
 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 v9 00/13] usb: musb: adding multi instance support

[B, Ravi]

   
   On Fri, Aug 31, 2012 at 04:39:46PM +0530, Ravi Babu wrote:
This series of patches adds,
a) Multi instances support in musb driver
b) DT support for musb_dsps glue layer
c) DT support for NOP transceiver

AM33xx and TI81xx has dual musb controller and has two usb
   PHY of same type.
This patch series uses 'phandle' based API
devm_usb_get_phy_by_phandle() to get the PHY of same type. This 
API support is being added by Kishon's patch discussed at [1]

The series applies to felipe/master [1] branch
+ Vaibhav baseport patches on his tree at [4]
+ Kishon's multi phy patches on Felipe's branch 'xceiv'
+ Kishon's patch on phandle at [2]
+ Damodar's recent patch at [3] 
+ Ajay's  Damodar's patches at [5] and [6] included in
   this series

1. 

 http://git.kernel.org/?p=linux/kernel/git/balbi/usb.git;a=summary
2. http://marc.info/?l=linux-usbm=134070369306112w=2
3. http://marc.info/?l=linux-usbm=134200284230689w=2
4. 

   
 https://github.com/hvaibhav/am335x-linux/commits/am335x-upstream-sta
   gi
ng 5. http://marc.info/?l=linux-usbm=134200285530701w=2
6. http://marc.info/?l=linux-usbm=134208820028625w=2

Changes from v8:
- included Sergei's comment, removing underscore in
   device tree file
- removed duplicated signoff from patches 
 Changes from v7:
- patches rebased on felipe/master branch  verified
- included additional two patches 0001  0002 as part
   of this series
  which are already submitted [5]  [6] Changes from v6:
- Removed parent_pdev to get glue and used
   dev_get_getdrv() as per
  Felipe's comment
- use pr_debug() instead of pr_info() as per Felipe's
   comment Changes
from v5:
- Removed musb-id as per Felipe's comment
- used nop_ida as per Felipe's comment Changes from v4:
- Fixed Felipe's comment for adding EXPORT_SYMBOL_GPL()
- Fixed Felipe's comment on using dev_set_mask()
   Changes from v3:
- Fixed Kishon's comment on removing id from 
 phy struct and
  removing unneeded #else part.
Changes from v2:
- Fixed Sergei's comment on not using address prefix in
   musb_dsps
  glue and nop transceiver dt dats.
- Also removed the ti string in compatible property
   for nop data.
Changes from v1:
- Defined musb_ida to manage core ids based on 
 Felipe's comment
  in [PATCH 01/11]
   
   I tried appliying this, but this doesn't apply. Please 
 rebase on my 
   musb branch. Unfortunately there's no time anymore to 
 wait otherwise 
   the entire musb branch will miss this merge window.
   
   I'm sorry
  
  Felipe, patch set is ready, I will re-send the patches 
 shortly by today.
 
 Thanks a lot. So I'll wait for a few more hours before 
 sending out my musb pull request ;-)
 

Thanks Felipe, these v9 patches are created on felipe/master branch. Now 
working on, to rebase on felipe/musb branch, will try to provide by end of 
today. 

 --
 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 v9 01/13] usb: musb: dsps: add phy control logic to glue

[B, Ravi]

 On Fri, Aug 31, 2012 at 06:51:04PM +0530, ABRAHAM, KISHON VIJAY wrote:
  Hi,
  
  On Fri, Aug 31, 2012 at 5:53 PM, Felipe Balbi ba...@ti.com wrote:
   Hi,
  
   On Fri, Aug 31, 2012 at 04:39:47PM +0530, Ravi Babu wrote:
   From: Santhapuri, Damodar damodar.santhap...@ti.com
  
   AM335x uses NOP transceiver driver and need to enable 
 builtin PHY 
   by writing into usb_ctrl register available in system control 
   module register space. This is being added at musb glue driver 
   layer untill a separate system control module driver is 
 available.
  
   Signed-off-by: Ajay Kumar Gupta ajay.gu...@ti.com
   Signed-off-by: Santhapuri, Damodar damodar.santhap...@ti.com
   Signed-off-by: Ravi Babu ravib...@ti.com
  
   Kishon, you were adding a real phy driver for OMAP's internal phy 
   logic on one of your patches and I believe this will 
 conflict with 
   your changes, right ?
  
  Indeed. My final patch of that series removes some of the functions 
  from omap_phy_internal.c (which was taken care in the phy driver).
  
   How does this look to you ? Is this at least correct ? I 
 suppose the 
   correct way would be to actually have the system control module 
   driver which we have been waiting, right ?
  
  Correct. I think once we have the system control module driver in 
  place, we'll have everything wrt control module register writes 
  implemented in correct way.
 
 So $SUBJECT will pretty much be thrown away once we have SCM 
 driver, in that case it's best to wait a bit longer and apply 
 this series once SCM driver is available and after your 
 series too... you agree ?
 

Felipe, I am sure there are patches in this series[0/13], which are not 
dependent on this patch or control module,
Can we pull in those patches (all dual instances support patches)? So that I 
can re-work and submit again? 

 --
 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 v9 01/13] usb: musb: dsps: add phy control logic to glue

[B, Ravi]

Hi

 AM335x uses NOP transceiver driver and need to enable
   builtin PHY
 by writing into usb_ctrl register available in 
 system control 
 module register space. This is being added at musb 
 glue driver 
 layer untill a separate system control module driver is
   available.

 Signed-off-by: Ajay Kumar Gupta ajay.gu...@ti.com
 Signed-off-by: Santhapuri, Damodar 
 damodar.santhap...@ti.com
 Signed-off-by: Ravi Babu ravib...@ti.com

 Kishon, you were adding a real phy driver for OMAP's internal 
 phy logic on one of your patches and I believe this will
   conflict with
 your changes, right ?

Indeed. My final patch of that series removes some of the 
functions from omap_phy_internal.c (which was taken 
 care in the phy driver).

 How does this look to you ? Is this at least correct ? I
   suppose the
 correct way would be to actually have the system 
 control module 
 driver which we have been waiting, right ?

Correct. I think once we have the system control module 
 driver in 
place, we'll have everything wrt control module register writes 
implemented in correct way.
   
   So $SUBJECT will pretty much be thrown away once we have 
 SCM driver, 
   in that case it's best to wait a bit longer and apply this series 
   once SCM driver is available and after your series too... 
 you agree 
   ?
   
  
  Felipe, I am sure there are patches in this series[0/13], which are 
  not dependent on this patch or control module, Can we pull in those 
  patches (all dual instances support patches)? So that I can re-work 
  and submit again?
 
 sure, will do, don't worry :-)

Thanks.
Then shall I rework patches [3/13 to 13/13] and re-submit only musb dual 
instances patches which 
are independent of control module. 

 
 --
 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 v8 08/13] arm/dts: am33xx: Add dt data for usbss

[B, Ravi]

 On Thu, Aug 30, 2012 at 03:39:40PM +0400, Sergei Shtylyov wrote:
  Hello.
  
  On 30-08-2012 14:50, Ravi Babu wrote:
  
  From: Ajay Kumar Gupta ajay.gu...@ti.com
  
  Added device tree data for usbss on am33xx. There are two musb 
  controllers on am33xx platform so have port0_mode and 
 port1_mode additional data.
  
  Signed-off-by: Ajay Kumar Gupta ajay.gu...@ti.com
  Signed-off-by: Ravi Babu ravib...@ti.com
  Signed-off-by: Santhapuri, Damodar damodar.santhap...@ti.com
  Signed-off-by: Ravi Babu ravib...@ti.com
  ---
arch/arm/boot/dts/am33xx.dtsi |   11 +++
1 files changed, 11 insertions(+), 0 deletions(-)
  
  diff --git a/arch/arm/boot/dts/am33xx.dtsi 
  b/arch/arm/boot/dts/am33xx.dtsi index 59509c4..778b95e 100644
  --- a/arch/arm/boot/dts/am33xx.dtsi
  +++ b/arch/arm/boot/dts/am33xx.dtsi
  @@ -154,5 +154,16 @@
 #size-cells = 0;
 ti,hwmods = i2c3;
 };
  +
  +  usb_otg_hs: usb_otg_hs {
  +  compatible = ti,musb-am33xx;
  +  ti,hwmods = usb_otg_hs;
  +  multipoint = 1;
  +  num_eps = 16;
  +  ram_bits = 12;
  +  port0_mode = 3;
  +  port1_mode = 3;
  
 Hyphen is preferred traditionally to underscore in the 
 device tree files...
 
 Are we having a v2 of this patch ??

Felipe, I will send it shortly.

Regards
Ravi B
 
 --
 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 v7 00/11] usb: musb: adding multi instance support

[B, Ravi]

Hi Daniel

   This series of patches adds,
   a) Multi instances support in musb driver
   b) DT support for musb_dsps glue layer
   c) DT support for NOP transceiver
  
   AM33xx and TI81xx has dual musb controller and has two usb
   PHY of same type.
   This patch series uses 'phandle' based API
   devm_usb_get_phy_by_phandle() to get the PHY of same type. This 
   API support is being added by Kishon's patch discussed at [1]
  
   The series applies to linux-omap (master branch)
   + Vaibhav baseport patches on his tree at [3]
   + Kishon's multi phy patches on Felipe's branch 'xceiv'
   + Kishon's patch on phandle at [1]
   + AM33xx musb glue compile and bugfix patches at [4],
   [5], [6] and [7]
   + Damodar's recent patch at [2]
  
   and have been tested on Beaglebone board.
  
   Have you applied the above patches before applying these patches.
  
  Somehow, I was missing some of Ajay's patches. I resolved that, and 
  now the series applied.
  
  However, I needed to add a phandle usb0-phy = usb0_phy to the 
  usb_otg_hs DTSI block, otherwise devm_usb_get_phy_by_phandle() in 
  drivers/usb/musb/musb_dsps.c would fail. Is that correct? I 
 can't seem 
  to find that in your patches.
 
 It's getting done in patch 11/11.

Refer patch 11/11 available at 
http://marc.info/?l=linux-usbm=134390988804627w=2 

Ravi Babu

 
  
  With this addition, I see the following:
  
  [1.782180] musb-hdrc: version 6.0, ?dma?, otg (peripheral+host)
  [1.809966] musb-hdrc musb-hdrc.0: MUSB HDRC host driver
  [1.819068] musb-hdrc musb-hdrc.0: new USB bus 
 registered, assigned
  bus number 1
  [1.827970] usb usb1: New USB device found, idVendor=1d6b,
  idProduct=0002
  [1.835184] usb usb1: New USB device strings: Mfr=3, Product=2,
  SerialNumber=1
  [1.842818] usb usb1: Product: MUSB HDRC host driver
  [1.848031] usb usb1: Manufacturer: Linux
  3.6.0-rc1-00038-g8a1ec8f-dirty musb-hcd
  [1.855933] usb usb1: SerialNumber: musb-hdrc.0
  [1.866913] hub 1-0:1.0: USB hub found
  [1.871192] hub 1-0:1.0: 1 port detected
  [1.878106] musb-hdrc musb-hdrc.0: USB Host mode controller at
  d08c using PIO, IRQ 18  
  
  ... but no USB functions. Also, every two seconds, the following 
  message is
  printed:
  
  [   11.036608] musb_bus_suspend 2308: trying to suspend as 
 a_wait_vrise
  while active
  [   13.044811] musb_bus_suspend 2308: trying to suspend as 
 a_wait_vrise
  while active
  [   15.052196] musb_bus_suspend 2308: trying to suspend as 
 a_wait_vrise
  while active
 
 Do you see them even when you connect a device to port?
 
 Ajay
  
  
  Anything obvious that I'm missing?
  
  
  Thanks,
  Daniel
  --
  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
 --
 -
 This email message is for the sole use of the intended 
 recipient(s) and may contain confidential information.  Any 
 unauthorized review, use, disclosure or distribution is 
 prohibited.  If you are not the intended recipient, please 
 contact the sender by reply email and destroy all copies of 
 the original message.
 --
 -
 --
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: storage: stop all current urbs when device is disconnected

[B, Ravi]

 
 On Fri, 3 Aug 2012, B, Ravi wrote:
 
  Alan, I did not understand, if the driver has unbound from 
 the device, 
  what is the need to wait for 30seconds timeout to cancel the URB.
  Can you explain in detail.
 
 Consider a disk drive with a large cache.  When usb-storage 
 unbinds from the device, the SCSI layer will want to tell the 
 disk to write out its cache.  Flushing the cache could take a 
 while, so the SYNCHRONIZE CACHE command needs to have a long 
 timeout.  Otherwise the contents of the cache could get lost.
 

Alan, what is meant by usb-stroage unbinds from the device? when the 
usb-storage unbinds from device, is it not when the device got disconnected? Or 
device gets umounted ?

Why scsi to wait for 30 seconds timeout upon receiving 
 the DEVICE 
DICONNECT Notfication ?
   
   That's the timeout for all the SCSI commands.  The 
 timeout doesn't 
   change when the disconnect notification is received.
  
  During disconnect, it is better to cancel all queued and active 
  current URB, why to wait for 30sec timeout.
 
 It's not better.  See above for an example where you _want_ 
 to wait for the command to finish.
 
 Now, if the device really has been unplugged then you can't 
 tell it to flush its cache.  In that situation you really do 
 want to avoid the 30-second timeout.  But neither usb-storage 
 nor the SCSI layer knows whether the device has been unplugged.
 
 However the host controller driver _does_ know (or at least, 
 it _should_ know).  It should guarantee that all URBs fail 
 quickly when the device is not plugged in.  It should not 
 wait forever for a DMA transfer that will never complete.
 
 That's why I suggested you fix the musb driver instead of 
 trying to change usb-storage.
 

The current MUSB HCD driver does not fail the URBs when the device got 
disconnected. 

 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: storage: stop all current urbs when device is disconnected

[B, Ravi]

Hi 

 On Thu, 2 Aug 2012, B, Ravi wrote:
 
--- a/drivers/usb/storage/usb.c
+++ b/drivers/usb/storage/usb.c
@@ -844,6 +844,8 @@ static void
   quiesce_and_remove_host(struct us_data *us)
 */
scsi_lock(host);
set_bit(US_FLIDX_DISCONNECTING, us-dflags);
+   /* stop the current urbs when the device got 
 disconnected */
+   usb_stor_stop_transport(us);
   
   This shouldn't be necessary.  This code runs after
   scsi_remove_host() returns, so there should not be any 
 URBs running 
   at this point.
   
   Have you actually encountered a problem that this patch fixes?
  
  In specific condition, where the transmit request is in 
 progress and 
  device is unplugged from host, found that this current tx 
 request is 
  not dequeued/unlinked during disconnect.
 
 Please provide more information.  What specific condition?  
 What was the current request?  How did this happen?
 
 If the device was unplugged while an URB was active, the URB 
 should have completed very quickly with a failure.  Why 
 didn't it fail?
 
 As far as I can tell, this shouldn't be needed.  
 scsi_remove_host() calls scsi_forget_host(), which calls 
 __scsi_remove_device(), which calls scsi_free_queue(), which 
 calls blk_cleanup_queue(), which calls blk_drain_queue(), 
 which should wait until all pending requests are finished.


Alan you may be correct. 
Let me explain, the issue occurs while unplugging the device during Tx 
transfer, 
1) During Tx transfer, TX-DMA is programmed to transfer the data. 
2) The TX-DMA completes the transfer and generates completion interrupt. 
3) On tx completion interrupt context the URB is given back. 
In normal scenario, the sequence 1 to 3 occurs during tx transfers.
Issue occurs when the device is unplugged while TX-DMA transfer is in progress. 
The DMA halts and never generates completion interrupt. In this scenario, this 
active request at DMA is given back when application calls dequeue or unlink 
the URB. Since in this scenario since there is no timeout occurs on the URB, we 
expect the scsi layer should unlink the active URB upon DISCONNECT of device. 

In this scenario, when the DISCONNECT occur, scsi does not unlink the active 
URB, this patch fixing this issue. 
Can you point me which function unlink the active URB (which is not returned). 
Let me know if you have better solution or any suggestion.

Thanks
Ravi B

 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: storage: stop all current urbs when device is disconnected

[B, Ravi]

Hi

Thanks for the quick response.

  
   Alan you may be correct.  Let me explain, the issue occurs while 
   unplugging the device during Tx transfer,
   1) During Tx transfer, TX-DMA is programmed to transfer the data. 
   2) The TX-DMA completes the transfer and generates 
 completion interrupt. 
   3) On tx completion interrupt context the URB is given back. 
   In normal scenario, the sequence 1 to 3 occurs during tx 
 transfers.
   Issue occurs when the device is unplugged while TX-DMA 
 transfer is 
   in progress. The DMA halts and never generates completion 
 interrupt. 
   In
  
  What host controller driver are you using?

It is mentor controller integrated with CPPI41-DMA 

  
  Why doesn't the controller hardware detect that the device fails to 
  send handshake packets and abort the transfer?  According 
 to the USB-2 
  spec, after three failures the controller must end the 
 transfer.  This 
  sounds like a bug in the controller or its driver.

The usb flash drive is connected directly to root port. 
Hence when the device is disconnected, the controller does not continue (no SOF)
due to all devices connected to port are disconnected. 
Hence controller will not retry or transfer the data to device. 
This must be expected behavior.

  
   this scenario, this active request at DMA is given back when 
   application calls dequeue or unlink the URB. Since in 
 this scenario 
   since there is no timeout occurs on the URB, we expect the scsi 
   layer should unlink the active URB upon DISCONNECT of device.
  
  Not until the command times out.
  
   In this scenario, when the DISCONNECT occur, scsi does not unlink 
   the active URB, this patch fixing this issue.
   Can you point me which function unlink the active URB 
 (which is not 
   returned).
   Let me know if you have better solution or any suggestion.
  
  I would expect the command to time out and be unlinked by
  command_abort() after 30 seconds or so, and I would expect
  scsi_remove_host() to wait until this happens.  If this doesn't 
  happen, it indicates a bug in the SCSI or block layers.
 

During disconnect event in the middle of transfer, the scsi knows that the 
device no more exits, 
I think there is no need to wait for timeout for active URB, it is safe to 
unlink or cancel the active URB.

 I tried doing a similar experiment on my system.  I got rid 
 of the code that makes ehci-hcd stop retrying after a maximum 
 number of transaction errors (so that it would continue to 
 retry indefinitely), and then pulled out my USB flash drive 
 while a program was reading from it.
 
 The result was exactly as predicted: After 30 seconds the 
 current transfer was aborted, and scsi_remove_host() waited 
 for this to happen.

Why scsi to wait for 30 seconds timeout upon receiving the DEVICE DICONNECT
Notfication ?
Have you connected the device through HUB ? 
What happen if the device connected directly to root port (without HUB).

Regards
Ravi Babu
 
 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: storage: stop all current urbs when device is disconnected

[B, Ravi]

 

 -Original Message-
 From: Alan Stern [mailto:st...@rowland.harvard.edu] 
 Sent: Friday, August 03, 2012 7:15 AM
 To: B, Ravi
 Cc: Matthew Dharm; Greg Kroah-Hartman; 
 linux-usb@vger.kernel.org; usb-stor...@lists.one-eyed-alien.net
 Subject: RE: [PATCH] usb: storage: stop all current urbs when 
 device is disconnected
 
 On Fri, 3 Aug 2012, B, Ravi wrote:
 
What host controller driver are you using?
  
  It is mentor controller integrated with CPPI41-DMA
 
 Then you're using the musb driver?
 
Why doesn't the controller hardware detect that the 
 device fails 
to send handshake packets and abort the transfer?  According
   to the USB-2
spec, after three failures the controller must end the
   transfer.  This
sounds like a bug in the controller or its driver.
  
  The usb flash drive is connected directly to root port. 
  Hence when the device is disconnected, the controller does not 
  continue (no SOF) due to all devices connected to port are 
 disconnected.
  Hence controller will not retry or transfer the data to device. 
  This must be expected behavior.
 
 Well, I suppose it's the _designed_ behavior.  But it's still 
 arguably a bug.
 
 Maybe the musb driver should be changed.  It should cancel 
 all the active URBs when all the ports are disconnected or suspended.
 
I would expect the command to time out and be unlinked by
command_abort() after 30 seconds or so, and I would expect
scsi_remove_host() to wait until this happens.  If this doesn't 
happen, it indicates a bug in the SCSI or block layers.
   
  
  During disconnect event in the middle of transfer, the scsi 
 knows that 
  the device no more exits, I think there is no need to wait 
 for timeout 
  for active URB, it is safe to unlink or cancel the active URB.
 
 Actually the SCSI layer does _not_ know that the device has 
 been removed.  It only knows that the driver has been unbound 
 from the device.  The behavior is the same as if you did 
 rmmod usb-storage  
 while leaving the device plugged in.
 
 Anyway, it seems clear that your patch won't help.  The new 
 code you added won't run until after the 30-second timeout 
 has expired, and by then it's not needed any more.

Alan, I did not understand, if the driver has unbound from the device, what is 
the need to wait for 30seconds timeout to cancel the URB. 
Can you explain in detail.

 
   I tried doing a similar experiment on my system.  I got 
 rid of the 
   code that makes ehci-hcd stop retrying after a maximum number of 
   transaction errors (so that it would continue to retry 
   indefinitely), and then pulled out my USB flash drive while a 
   program was reading from it.
   
   The result was exactly as predicted: After 30 seconds the current 
   transfer was aborted, and scsi_remove_host() waited for this to 
   happen.
  
  Why scsi to wait for 30 seconds timeout upon receiving the DEVICE 
  DICONNECT Notfication ?
 
 That's the timeout for all the SCSI commands.  The timeout 
 doesn't change when the disconnect notification is received.

During disconnect, it is better to cancel all queued and active current URB, 
why to wait for 30sec timeout.
The API usb_stor_stop_transport() does the same. This API is safe it only 
unlink if there is active URB.

 
  Have you connected the device through HUB ? 
 
 No.
 
  What happen if the device connected directly to root port 
 (without HUB).
 
 As I described above.
 
 Alan Stern
 
 --
To unsubscribe from this list: send the line unsubscribe linux-usb in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH v6 03/11] usb: musb: am335x: add support for dual instance

[B, Ravi]

 Hi,
 
 On Fri, Jul 27, 2012 at 02:01:59PM +0530, Ravi B wrote:
  From: Ajay Kumar Gupta ajay.gu...@ti.com
  
  AM335x and TI81xx platform has dual musb controller so updating the 
  musb_dspc.c to support the same.
  
  Changes:
  - Moved otg_workaround timer to glue structure
  - Moved static local variable last_timer to glue structure
  - PHY on/off related cleanups
  
  Signed-off-by: Ajay Kumar Gupta ajay.gu...@ti.com
  Signed-off-by: Ravi B ravib...@ti.com
  ---
   drivers/usb/musb/musb_dsps.c |  118 
  +
   1 files changed, 72 insertions(+), 46 deletions(-)
  
  diff --git a/drivers/usb/musb/musb_dsps.c 
  b/drivers/usb/musb/musb_dsps.c index 2174699..2fd5dc8 100644
  --- a/drivers/usb/musb/musb_dsps.c
  +++ b/drivers/usb/musb/musb_dsps.c
  @@ -105,6 +105,8 @@ struct dsps_musb_wrapper {
  /* miscellaneous stuff */
  u32 musb_core_offset;
  u8  poll_seconds;
  +   /* number of musb instances */
  +   u8  instances;
   };
   
   /**
  @@ -112,16 +114,18 @@ struct dsps_musb_wrapper {
*/
   struct dsps_glue {
  struct device *dev;
  -   struct platform_device *musb;   /* child musb pdev */
  +   struct platform_device *musb[2];/* child musb pdev */
  const struct dsps_musb_wrapper *wrp; /* wrapper 
 register offsets */
  -   struct timer_list timer;/* otg_workaround timer */
  -   u32 __iomem *usb_ctrl;
  +   struct timer_list timer[2]; /* otg_workaround timer */
  +   unsigned long last_timer[2];/* last timer data for 
 each instance */
  +   u32 __iomem *usb_ctrl[2];
  u8  usbss_rev;
   };
   
   /**
* musb_dsps_phy_control - phy on/off
* @glue: struct dsps_glue *
  + * @id: musb instance
* @on: flag for phy to be switched on or off
*
* This is to enable the PHY using usb_ctrl register in system 
  control @@ -130,11 +134,11 @@ struct dsps_glue {
* XXX: This function will be removed once we have a 
 seperate driver for
* control module
*/
  -static void musb_dsps_phy_control(struct dsps_glue *glue, u8 on)
  +static void musb_dsps_phy_control(struct dsps_glue *glue, 
 u8 id, u8 
  +on)
   {
  u32 usbphycfg;
   
  -   usbphycfg = __raw_readl(glue-usb_ctrl);
  +   usbphycfg = __raw_readl(glue-usb_ctrl[id]);
   
  if (on) {
  if (glue-usbss_rev == MUSB_USBSS_REV_816X) { 
 @@ -157,7 +161,7 @@ 
  static void musb_dsps_phy_control(struct dsps_glue *glue, u8 on)
  glue-usbss_rev == MUSB_USBSS_REV_33XX)
  usbphycfg |= USBPHY_CM_PWRDN | USBPHY_OTG_PWRDN;
  }
  -   __raw_writel(usbphycfg, glue-usb_ctrl);
  +   __raw_writel(usbphycfg, glue-usb_ctrl[id]);
   }
   /**
* dsps_musb_enable - enable interrupts @@ -207,8 +211,9 @@ static 
  void otg_timer(unsigned long _musb)
  struct musb *musb = (void *)_musb;
  void __iomem *mregs = musb-mregs;
  struct device *dev = musb-controller;
  -   struct platform_device *pdev = to_platform_device(dev-parent);
  -   struct dsps_glue *glue = platform_get_drvdata(pdev);
  +   struct platform_device *pdev = to_platform_device(dev);
  +   struct platform_device *parent_pdev = 
 to_platform_device(dev-parent);
  +   struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
  const struct dsps_musb_wrapper *wrp = glue-wrp;
  u8 devctl;
  unsigned long flags;
  @@ -247,7 +252,7 @@ static void otg_timer(unsigned long _musb)
   
  devctl = dsps_readb(mregs, MUSB_DEVCTL);
  if (devctl  MUSB_DEVCTL_BDEVICE)
  -   mod_timer(glue-timer,
  +   mod_timer(glue-timer[pdev-id],
  jiffies + 
 wrp-poll_seconds * HZ);
  else
  musb-xceiv-state = OTG_STATE_A_IDLE; 
 @@ -261,9 +266,9 @@ static 
  void otg_timer(unsigned long _musb)  static void 
  dsps_musb_try_idle(struct musb *musb, unsigned long timeout)  {
  struct device *dev = musb-controller;
  -   struct platform_device *pdev = to_platform_device(dev-parent);
  -   struct dsps_glue *glue = platform_get_drvdata(pdev);
  -   static unsigned long last_timer;
  +   struct platform_device *pdev = to_platform_device(dev);
  +   struct platform_device *parent_pdev = 
 to_platform_device(dev-parent);
  +   struct dsps_glue *glue = platform_get_drvdata(parent_pdev);
 
 just one thing that could be cleaned on a later patch:
 
 if parent_pdev is only used to get to struct dsps_glue, you 
 could just:
 
 struct dsps_glue *glue = dev_get_drvdata(dev-parent);
 
 with no need to do a container_of() to the platform_device ;-)

Yes Felipe, parent_pdev is only to get dsps_glue. I will include this change in 
later patch.

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