Re: [PATCH V3] usb: Add Device Tree support to XHCI Platform driver

2013-07-23 Thread Alan Cooper 
This compiles without CONFIG_OF because of_match_ptr() assigns NULL if
CONFIG_OF is not defined.

On Tue, Jul 23, 2013 at 10:35 AM, Matthijs Kooijman matth...@stdin.nl wrote:
 Hi Al,

 @@ -212,11 +213,19 @@ static int xhci_plat_remove(struct platform_device 
 *dev)
   return 0;
  }

 +#ifdef CONFIG_OF
 +static const struct of_device_id usb_xhci_of_match[] = {
 + { .compatible = usb-xhci },
 + {},
 +};
 +#endif
 +
  static struct platform_driver usb_xhci_driver = {
   .probe  = xhci_plat_probe,
   .remove = xhci_plat_remove,
   .driver = {
   .name = xhci-hcd,
 + .of_match_table = of_match_ptr(usb_xhci_of_match),
   },
  };
  MODULE_ALIAS(platform:xhci-hcd);
 This looks like it wouldn't compile without CONFIG_OF?

 Gr.

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

Re: [PATCH V4] usb: Add Device Tree support to XHCI Platform driver

2013-07-25 Thread Alan Cooper 
On Wed, Jul 24, 2013 at 6:10 PM, Sergei Shtylyov
sergei.shtyl...@cogentembedded.com wrote:
 Hello.


 On 07/25/2013 01:21 AM, Sarah Sharp wrote:

 It looks like all the feedback has been addressed, but I'm no device
 tree expert.


 Felipe, Matthijs, and Sergei, does this look good?  If so, I'll queue to
 my xhci tree.


Not quite there yet. Too bad I couldn't notice all the small issues at
 once...


 Sarah Sharp


 On Tue, Jul 23, 2013 at 06:35:33PM -0400, Al Cooper wrote:

 Add Device Tree match table to xhci-plat.c. Add DT bindings document.


 Signed-off-by: Al Cooper alcoop...@gmail.com
 ---
   Documentation/devicetree/bindings/usb/usb-xhci.txt | 14 ++
   drivers/usb/host/xhci-plat.c   | 10 ++
   2 files changed, 24 insertions(+)
   create mode 100644 Documentation/devicetree/bindings/usb/usb-xhci.txt


 diff --git a/Documentation/devicetree/bindings/usb/usb-xhci.txt
 b/Documentation/devicetree/bindings/usb/usb-xhci.txt
 new file mode 100644
 index 000..b88aee7
 --- /dev/null
 +++ b/Documentation/devicetree/bindings/usb/usb-xhci.txt
 @@ -0,0 +1,14 @@
 +USB XHCI controllers


I think the proper name is xHCI.

OK



 +
 +Required properties:
 +  - compatible: should be xhci-platform.
 +  - reg: should contain address and length of the standard XHCI
 +register set for the device.
 +  - interrupts: one XHCI interrupt should be described here.
 +
 +Example:
 +   xhci@f0931000 {


The node should be named just usb, not xhci (no programming interface
 specific names), according to the ePAPR spec [1].


What about the existing node names ohci@ and ehci@?

 +   compatible = xhci-platform;


It again looks somewhat like a driver name, not a device name. What made
 you change the value from usb-xhci, Al? Look at [eo]hci-omap.txt in that
 directory.

I changed the name because MODULE_DEVICE_TABLE() now uses the name and
that means the hotplug system will use it to identify the driver and
it seems like the
name should be unique enough to avoid confusion with something like
the xhci-pci driver.



 +   reg = 0xf0931000 0x8c8;
 +   interrupts = 0x0 0x4e 0x0;
 +   };
 diff --git a/drivers/usb/host/xhci-plat.c b/drivers/usb/host/xhci-plat.c
 index d718134..d547f24 100644
 --- a/drivers/usb/host/xhci-plat.c
 +++ b/drivers/usb/host/xhci-plat.c

 [...]

 @@ -212,11 +213,20 @@ static int xhci_plat_remove(struct platform_device
 *dev)
 return 0;
   }

 +#ifdef CONFIG_OF
 +static const struct of_device_id usb_xhci_of_match[] = {
 +   { .compatible = xhci-platform },
 +   {},


Consistency asks for a space inside {}. AFAIK, that's the usual Linux
 style.


OK

 [1] http://www.power.org/resources/downloads/Power_ePAPR_APPROVED_v1.0.pdf

 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 V4] usb: Add Device Tree support to XHCI Platform driver

2013-07-25 Thread Alan Cooper 
On Thu, Jul 25, 2013 at 1:10 PM, Sergei Shtylyov
sergei.shtyl...@cogentembedded.com wrote:

  The node should be named just usb, not xhci (no programming
 interface
 specific names), according to the ePAPR spec [1].



 What about the existing node names ohci@ and ehci@?


 Unfortunately, they are all wrong, as many others. It seems almost
 nobody
 reads:


 http://www.devicetree.org/Device_Tree_Usage


 Can you point me to the section that indicates ohci/ehci/xhci are
 incorrect?


 http://devicetree.org/Device_Tree_Usage#Node_Names

 See also section 2.2.2 in the ePAPR spec.

 WBR, Sergei


I read http://devicetree.org/Device_Tree_Usage#Node_Names to say not
to use vendor specific names and xhci is not vendor specific.

ePAPR states:
The name of a node should be somewhat generic, reflecting the
function of the device and not its precise programming model.
And while xhci does describe the programming interface, it also
describes the functionality and fits in better with the already
established ehci and ohci node names.

Al
--
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: xhci_suspend is not stopping the root hub timer for the shared HCD

2014-07-30 Thread Alan Cooper 
Good point. I'll send a V2.

Thanks
Al

On Wed, Jul 30, 2014 at 2:04 PM, Jack Pham ja...@codeaurora.org wrote:
 Hi Al,

 I faced an issue related to this and was about to send a patch myself
 but you beat me to it.

 On Tue, Jul 29, 2014 at 02:21:55PM -0400, Al Cooper wrote:
 --- a/drivers/usb/host/xhci.c
 +++ b/drivers/usb/host/xhci.c
 @@ -871,6 +871,7 @@ int xhci_suspend(struct xhci_hcd *xhci)
   xhci_dbg(xhci, %s: stopping port polling.\n, __func__);
   clear_bit(HCD_FLAG_POLL_RH, hcd-flags);
   del_timer_sync(hcd-rh_timer);

 clear_bit(HCD_FLAG_POLL_RH, hcd-shared_hcd-flags) also?

 + del_timer_sync(hcd-shared_hcd-rh_timer);

 What about at the end of xhci_resume(), does the polling timer of the
 secondary HCD need to be restarted as well?

 /* Don't poll the roothubs on bus suspend. */
 xhci_dbg(xhci, %s: starting port polling.\n, __func__);
 set_bit(HCD_FLAG_POLL_RH, hcd-flags);
 usb_hcd_poll_rh_status(hcd);
 +   set_bit(HCD_FLAG_POLL_RH, xhci-shared_hcd-flags);
 +   usb_hcd_poll_rh_status(xhci-shared_hcd);

 Thanks,
 Jack
 --
 Employee of Qualcomm Innovation Center, Inc.
 The Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
 hosted by The Linux Foundation
--
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 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect

2015-07-24 Thread Alan Cooper 
I'll send a single patch for the minimal bug fix for the rc and I'll
send the rest of the changes as a patch set for 4.3.

Thanks
Al

On Thu, Jul 23, 2015 at 4:05 PM, Felipe Balbi ba...@ti.com wrote:
 On Thu, Jul 23, 2015 at 03:23:26PM -0400, Alan Cooper wrote:
 On Wed, Jul 22, 2015 at 10:03 PM, Felipe Balbi ba...@ti.com wrote:
  Hi,
 
  On Wed, Jul 22, 2015 at 06:27:29PM -0400, Alan Cooper wrote:
  On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi ba...@ti.com wrote:
   On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote:
   On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote:
V2 - Fix a compiler bug that happend when the config options
CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE
were enabled.
   
ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer
incorrectly by reading the wrong register for the upper 32 bits.
The header file defining the registers was incorrect.
  
   btw, the header file was really incorrect as long as you passed 0 to
   the argument :-p
  
   in fact, the minimal fix for this bug would be the one below:
  
   diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c 
   b/drivers/usb/gadget/udc/bdc/bdc_ep.c
   index b04980cf6dc4..1efa61265d8d 100644
   --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
   +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
   @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct 
   bdc_req *req)
   /* The current hw dequeue pointer */
   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(0));
   deq_ptr_64 = tmp_32;
   -   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(1));
   +   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS1(0));
   deq_ptr_64 |= ((u64)tmp_32  32);
  
   /* we have the dma addr of next bd that will be fetched by 
   hardware */
  
   And $subject becomes a cleanup patch for v4.3. Can you make these
   changes, please ?
 
  Yes, I can make these changes, but first let me explain why I did it this 
  way.
  The 8 End Point Status registers are 32 bit consecutive registers, so
  defining them as:
  #define BDC_EPSTS0(n)  (0x60 + (n * 0x10))
  #define BDC_EPSTS1(n)  (0x64 + (n * 0x10))
  #define BDC_EPSTS2(n)  (0x68 + (n * 0x10))
  #define BDC_EPSTS3(n)  (0x6c + (n * 0x10))
  #define BDC_EPSTS4(n)  (0x70 + (n * 0x10))
  #define BDC_EPSTS5(n)  (0x74 + (n * 0x10))
  #define BDC_EPSTS6(n)  (0x78 + (n * 0x10))
  #define BDC_EPSTS7(n)  (0x7c + (n * 0x10))
  seems misleading, does not reflect the hardware and using anything
  other than (0) would get you to some other unexpected register and
  should be considered a coding error.
 
  it sure is, but the minimal patch for -rc is what I sent above :-) As
  long as you pass 0 as parameters, all your offsets are correct, so
  removing the parameter (which must be always zero) is, actually,
  refactoring happening.
 

 OK, I think I understand. You're asking me to split the bug fix patch
 into two patches, one is a minimal bug fix patch for -rc and the other
 is a cleanup patch (fix the header file) that will go into 4.3. Is
 that correct?

 correct.

  I think the original hardware spec had each End Point Status as a
  block of registers but the first silicon had them as single registers
  and they are expected to stay that way.
 
  I believe they wanted to have:
 
  #define BDC_EPSTS(n)(0x60 + (n * 0x4))
 
  and that way you can access each EP by passing the correct argument to
  that macro.
 

 Actually, the 8 EPSTS registers are all different and together contain
 the info for 1 end point. The original plan was to have 4 32bit
 registers per end point and enough sets of these registers for all
 endpoints, but this turned out to be too expensive in silicon so they
 changed it to 1 set of 8 registers that are updated when an end point
 is stopped.

 oh ok. thanks

 --
 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 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect

2015-07-22 Thread Alan Cooper 
On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi ba...@ti.com wrote:
 On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote:
 On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote:
  V2 - Fix a compiler bug that happend when the config options
  CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE
  were enabled.
 
  ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer
  incorrectly by reading the wrong register for the upper 32 bits.
  The header file defining the registers was incorrect.

 btw, the header file was really incorrect as long as you passed 0 to
 the argument :-p

 in fact, the minimal fix for this bug would be the one below:

 diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c 
 b/drivers/usb/gadget/udc/bdc/bdc_ep.c
 index b04980cf6dc4..1efa61265d8d 100644
 --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
 +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
 @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct bdc_req 
 *req)
 /* The current hw dequeue pointer */
 tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(0));
 deq_ptr_64 = tmp_32;
 -   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(1));
 +   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS1(0));
 deq_ptr_64 |= ((u64)tmp_32  32);

 /* we have the dma addr of next bd that will be fetched by hardware */

 And $subject becomes a cleanup patch for v4.3. Can you make these
 changes, please ?

Yes, I can make these changes, but first let me explain why I did it this way.
The 8 End Point Status registers are 32 bit consecutive registers, so
defining them as:
#define BDC_EPSTS0(n)  (0x60 + (n * 0x10))
#define BDC_EPSTS1(n)  (0x64 + (n * 0x10))
#define BDC_EPSTS2(n)  (0x68 + (n * 0x10))
#define BDC_EPSTS3(n)  (0x6c + (n * 0x10))
#define BDC_EPSTS4(n)  (0x70 + (n * 0x10))
#define BDC_EPSTS5(n)  (0x74 + (n * 0x10))
#define BDC_EPSTS6(n)  (0x78 + (n * 0x10))
#define BDC_EPSTS7(n)  (0x7c + (n * 0x10))
seems misleading, does not reflect the hardware and using anything
other than (0) would get you to some other unexpected register and
should be considered a coding error.
I think the original hardware spec had each End Point Status as a
block of registers but the first silicon had them as single registers
and they are expected to stay that way.

Thanks
--
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 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect

2015-07-23 Thread Alan Cooper 
On Wed, Jul 22, 2015 at 10:03 PM, Felipe Balbi ba...@ti.com wrote:
 Hi,

 On Wed, Jul 22, 2015 at 06:27:29PM -0400, Alan Cooper wrote:
 On Wed, Jul 22, 2015 at 5:29 PM, Felipe Balbi ba...@ti.com wrote:
  On Wed, Jul 22, 2015 at 04:26:57PM -0500, Felipe Balbi wrote:
  On Wed, Jul 22, 2015 at 04:58:07PM -0400, Al Cooper wrote:
   V2 - Fix a compiler bug that happend when the config options
   CONFIG_USB_GADGET_DEBUG and CONFIG_USB_GADGET_VERBOSE
   were enabled.
  
   ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer
   incorrectly by reading the wrong register for the upper 32 bits.
   The header file defining the registers was incorrect.
 
  btw, the header file was really incorrect as long as you passed 0 to
  the argument :-p
 
  in fact, the minimal fix for this bug would be the one below:
 
  diff --git a/drivers/usb/gadget/udc/bdc/bdc_ep.c 
  b/drivers/usb/gadget/udc/bdc/bdc_ep.c
  index b04980cf6dc4..1efa61265d8d 100644
  --- a/drivers/usb/gadget/udc/bdc/bdc_ep.c
  +++ b/drivers/usb/gadget/udc/bdc/bdc_ep.c
  @@ -779,7 +779,7 @@ static int ep_dequeue(struct bdc_ep *ep, struct 
  bdc_req *req)
  /* The current hw dequeue pointer */
  tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(0));
  deq_ptr_64 = tmp_32;
  -   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS0(1));
  +   tmp_32 = bdc_readl(bdc-regs, BDC_EPSTS1(0));
  deq_ptr_64 |= ((u64)tmp_32  32);
 
  /* we have the dma addr of next bd that will be fetched by 
  hardware */
 
  And $subject becomes a cleanup patch for v4.3. Can you make these
  changes, please ?

 Yes, I can make these changes, but first let me explain why I did it this 
 way.
 The 8 End Point Status registers are 32 bit consecutive registers, so
 defining them as:
 #define BDC_EPSTS0(n)  (0x60 + (n * 0x10))
 #define BDC_EPSTS1(n)  (0x64 + (n * 0x10))
 #define BDC_EPSTS2(n)  (0x68 + (n * 0x10))
 #define BDC_EPSTS3(n)  (0x6c + (n * 0x10))
 #define BDC_EPSTS4(n)  (0x70 + (n * 0x10))
 #define BDC_EPSTS5(n)  (0x74 + (n * 0x10))
 #define BDC_EPSTS6(n)  (0x78 + (n * 0x10))
 #define BDC_EPSTS7(n)  (0x7c + (n * 0x10))
 seems misleading, does not reflect the hardware and using anything
 other than (0) would get you to some other unexpected register and
 should be considered a coding error.

 it sure is, but the minimal patch for -rc is what I sent above :-) As
 long as you pass 0 as parameters, all your offsets are correct, so
 removing the parameter (which must be always zero) is, actually,
 refactoring happening.


OK, I think I understand. You're asking me to split the bug fix patch
into two patches, one is a minimal bug fix patch for -rc and the other
is a cleanup patch (fix the header file) that will go into 4.3. Is
that correct?

 I think the original hardware spec had each End Point Status as a
 block of registers but the first silicon had them as single registers
 and they are expected to stay that way.

 I believe they wanted to have:

 #define BDC_EPSTS(n)(0x60 + (n * 0x4))

 and that way you can access each EP by passing the correct argument to
 that macro.


Actually, the 8 EPSTS registers are all different and together contain
the info for 1 end point. The original plan was to have 4 32bit
registers per end point and enough sets of these registers for all
endpoints, but this turned out to be too expensive in silicon so they
changed it to 1 set of 8 registers that are updated when an end point
is stopped.

 --
 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 1/4] usb: gadget: udc: bdc: Fix a driver crash on disconnect

2015-07-20 Thread Alan Cooper 
I did do a build for ARM dove and x86_64 and didn't see any build
errors. Can you send me the build error?

Thanks
Al

On Mon, Jul 20, 2015 at 1:15 PM, Felipe Balbi ba...@ti.com wrote:
 On Thu, Jul 16, 2015 at 05:47:35PM -0400, Al Cooper wrote:
 ep_dequeue() in bdc_ep.c was capturing the hw dequeue pointer
 incorrectly by reading the wrong register for the upper 32 bits.
 The header file defining the registers was incorrect.

 Signed-off-by: Al Cooper alcoop...@gmail.com

 so you fix a driver crash by introducing a build error ? Test your
 patches before sending them out.

 --
 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: USB resume issue

2015-12-01 Thread Alan Cooper 
On Tue, Dec 1, 2015 at 2:35 PM, Alan Stern  wrote:
>> I'm now sure the host in "non-compliant".
>
> It is non-compliant if it turns off Vbus power during suspend.  But
> that may be caused by the platform more than by the controller itself.
>

I wasn't able to find any document that states that VBUS must be on in
S3 mode. In our SoC, the USB controller is not in the AON block so it
looses power in S3 and that disables the USB VBUS supply. If I can
prove our SoC is out of spec, I can probably get it fixed in future
chips.

Thanks
Al Cooper
--
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 resume issue

2015-12-01 Thread Alan Cooper 
On Tue, Dec 1, 2015 at 4:48 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
> On Tue, 1 Dec 2015, Alan Cooper wrote:
>
>> On Tue, Dec 1, 2015 at 2:35 PM, Alan Stern <st...@rowland.harvard.edu> wrote:
>> >> I'm now sure the host in "non-compliant".
>> >
>> > It is non-compliant if it turns off Vbus power during suspend.  But
>> > that may be caused by the platform more than by the controller itself.
>> >
>>
>> I wasn't able to find any document that states that VBUS must be on in
>> S3 mode.
>
> Hmmm, that's a little tricky.  Were you able to find a document that
> says Vbus must be on in S0?  People tend to make assumptions about
> these things without stating them clearly.
>
> The USB-2.0 spec says (section 7.2.1, the paragraph about root port
> hubs): "Systems that obtain operating power externally, either AC or
> DC, must supply at least five unit loads to each port."  (A unit load
> is 100 mA.)
>
> There's no mention of suspend, S3, or anything else.  However, I think
> we can assume this refers to the case where the root hub is not
> suspended.
>
> Section 7.2.3 discusses power control during suspend/resume.  Among
> other things, it says:
>
> When a hub is in the Suspend state, it must still be able to
> provide the maximum current per port (one unit load of current
> per port for bus-powered hubs and five unit loads per port for
> self-powered hubs). This is necessary to support remote
> wakeup-capable devices that will power-up while the remainder
> of the system is still suspended.
>
> Although it doesn't say so explicitly, this includes root hubs as well
> as external hubs.  The normal current draw during suspend will of
> course be a lot smaller than this; the maximum is needed only during
> the time that a downstream device is powering up because of a wakeup
> event.
>
>>  In our SoC, the USB controller is not in the AON block so it
>> looses power in S3 and that disables the USB VBUS supply. If I can
>> prove our SoC is out of spec, I can probably get it fixed in future
>> chips.
>
> Maybe this reference will help.

I agree that for USB suspend modes, VBUS must be on, and our hardware
behaves that way for runtime suspend. The question is for system wide
suspend (S3/STR) can the system shut down the USB hardware entirely.
The ACPI spec and Linux Documents/power/states.txt seem to allow either
design. From states.txt under S3 mode: "In many cases, all peripheral buses
lose power when entering STR, so devices must be able to handle the
transition back to the On state".

Thanks for you input
Al Cooper
--
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 resume issue

2015-12-01 Thread Alan Cooper 
On Tue, Dec 1, 2015 at 11:07 AM, Alan Stern  wrote:
> On Tue, 1 Dec 2015, Oliver Neukum wrote:
>
>> On Tue, 2015-12-01 at 10:41 -0500, Alan Stern wrote:
>> > > A recent patch, 7fa40910e0bf5ef32eca49595d950cb24f6402bf, added a
>> > > CONNECTED retry for a different reason and I could simply increase
>> > > this retry time. Any thoughts?
>> >
>> > I don't know.  You've got a non-compliant host combined with an
>> > excessively slow device.  It seems unwise to penalize everybody by
>> > slowing down their resumes (by 500 ms!) just because of this one
>> > bad combination.

I'm now sure the host in "non-compliant". It looks like the USB-persist feature
was created for the loss of "power session" on suspend/resume. Hibernate
will usually remove power and it's just because the BIOS re-enables VBUS
that PC's don't see this problem. Also there doesn't seem to be any spec for
how long after VBUS a 2.0 (or 3.0) device should become CONNECTED.
Most 2.0 or 3.0 hard drive based devices, either using a SATA to USB dongle
or devices like the WD Passport take >500ms to set CONNECTED. I've tested
many different 3.0 devices plugged into a 2.0 only port and all of them take
longer than the 100ms currently needed to succeed.

>> >
>> > On the other hand, I don't have any better ideas.
>>
>> Yet another quirk. Assume this to be necessary only if the port
>> was connected to a quirky device before loss of power.
>
> Yeah, okay.  Although we have no way to know that the non-compliant
> host controller turned off the port power.
>
> Alan, can you provide the vendor and product IDs for your USB-3 flash
> drive?  Or would you prefer to write a patch yourself?

I think the best solution would be to use "wait_for_ss_port_enable()"
(renamed) for 2.0 devices and not just 3.0 devices as discussed in:
http://marc.info/?l=linux-usb=143768271119144=2

Al Cooper
--
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 resume issue

2015-12-01 Thread Alan Cooper 
On Tue, Dec 1, 2015 at 12:12 PM, Alan Cooper <alcoop...@gmail.com> wrote:
> On Tue, Dec 1, 2015 at 11:07 AM, Alan Stern <st...@rowland.harvard.edu> wrote:
>> On Tue, 1 Dec 2015, Oliver Neukum wrote:
>>
>>> On Tue, 2015-12-01 at 10:41 -0500, Alan Stern wrote:
>>> > > A recent patch, 7fa40910e0bf5ef32eca49595d950cb24f6402bf, added a
>>> > > CONNECTED retry for a different reason and I could simply increase
>>> > > this retry time. Any thoughts?
>>> >
>>> > I don't know.  You've got a non-compliant host combined with an
>>> > excessively slow device.  It seems unwise to penalize everybody by
>>> > slowing down their resumes (by 500 ms!) just because of this one
>>> > bad combination.
>
> I'm now sure the host in "non-compliant". It looks like the USB-persist 
> feature
> was created for the loss of "power session" on suspend/resume. Hibernate
> will usually remove power and it's just because the BIOS re-enables VBUS
> that PC's don't see this problem. Also there doesn't seem to be any spec for
> how long after VBUS a 2.0 (or 3.0) device should become CONNECTED.
> Most 2.0 or 3.0 hard drive based devices, either using a SATA to USB dongle
> or devices like the WD Passport take >500ms to set CONNECTED. I've tested
> many different 3.0 devices plugged into a 2.0 only port and all of them take
> longer than the 100ms currently needed to succeed.
>
>>> >
>>> > On the other hand, I don't have any better ideas.
>>>
>>> Yet another quirk. Assume this to be necessary only if the port
>>> was connected to a quirky device before loss of power.
>>
>> Yeah, okay.  Although we have no way to know that the non-compliant
>> host controller turned off the port power.
>>
>> Alan, can you provide the vendor and product IDs for your USB-3 flash
>> drive?  Or would you prefer to write a patch yourself?
>
> I think the best solution would be to use "wait_for_ss_port_enable()"
> (renamed) for 2.0 devices and not just 3.0 devices as discussed in:
> http://marc.info/?l=linux-usb=143768271119144=2

I forgot to say that I'll submit the patch if this seems reasonable.
I thought it might also be useful to add a sysfs entry that could extend the
timeout. I have a Lexar 3.0 device that takes 6 seconds to become connected
in either a 3.0 or 2.0 port.

Al
--
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 resume issue

2015-11-30 Thread Alan Cooper 
I'm seeing a problem that looks like an issue in the USB-Persist
feature. I'm finding that all my 3.0 thumb drives are torn down and
brought back up (fail USB-Persist) during resume from "suspend to
memory" if they are plugged into a 2.0 EHCI/OHCI only port on my
embedded system. This embedded system seems to be unusual because it
removes VBUS during the suspend, but it looks like USB-Persist was
written to handle this and normal 2.0 thumb drives work correctly.
What I find is that the 3.0 thumb drives take 200ms-500ms after Port
Power is applied to set the "Current Connection Status" bit in the
PORTSC register and this causes check_port_resume_type() in hub.c to
return ENODEV. My 2.0 thumb drives show CONNECTED in < 100ms and this
works because hub_power_on() in hub.c waits 100ms for the power to
become stable. It looks to me like check_port_resume_type() should
look for the CONNECTED bit for hundreds of ms. I checked my Dell
laptop to see why it wasn't having a problem with the 3.0 thumb drives
and found that pm-suspend leaves the port power on and pm-hibernate
re-enables the port power in the BIOS which gives much more than a
500ms delay before the kernel resume code checks for CONNECTED.

A recent patch, 7fa40910e0bf5ef32eca49595d950cb24f6402bf, added a
CONNECTED retry for a different reason and I could simply increase
this retry time. Any thoughts?

Thanks
Al Cooper
--
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 3/6] dt-bindings: mtu3: add devicetree bindings

2016-05-12 Thread Alan Cooper 
On Thu, May 12, 2016 at 3:24 AM, chunfeng yun  wrote:
>> > + - mediatek,enable-manual-drd : supports manual dual-role switch by sysfs
>> > +   interface; only used when receptacle is TYPE-A and also wants to 
>> > support
>> > +   dual-role mode.
>>
>> sysfs is a Linux detail that doesn't apply to the binding. Does the
>> property mean "the IP block supports or doesn't support switching"  or
>> "I want to enable switching feature". Only the former belongs in DT.
>>
> It is the former case, and has little relation with sysfs interface.
> I will modify it later, sorry for my unclear description.

Could the property name be just "enable-manual-drd" instead of
"mediatek,enable-manual-drd"? I expect to have to add this
functionality to our USB driver in the near future.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH V3 2/2] usb: phy: phy-brcm-usb: Add Broadcom STB USB Phy driver

2016-08-31 Thread Alan Cooper 
On Wed, Aug 31, 2016 at 5:57 AM, Kishon Vijay Abraham I  wrote:
> Hi,
>
> On Tuesday 30 August 2016 08:00 PM, Al Cooper wrote:
>> Add a new USB Phy driver for Broadcom STB SoCs. This driver
>> supports all Broadcom STB ARM SoCs. This driver in combination
>> with the generic ohci, ehci and xhci platform drivers will enable
>> USB1.1, USB2.0 and USB3.0 support. This Phy driver also supports
>> the Broadcom UDC gadget driver.
>
> Remove usb: from $subject

Ok


>>
>> Signed-off-by: Al Cooper 
>> ---
>>  .../bindings/phy/brcm,brcmstb-usb-phy.txt  |  39 +
>>  MAINTAINERS|   7 +
>>  drivers/phy/Kconfig|  10 +
>>  drivers/phy/Makefile   |   2 +
>>  drivers/phy/phy-brcm-usb-init.c| 792 
>> +
>
> other broadcom phy drivers use only "bcm" in the file name.

We have a mixed use of both names throughout the kernel. Notice
"phy-brcm-sata.c in the phy directory. "brcm" seems a little better
because it matches the device tree prefix used by all drivers and is
used by many of the latest drivers.


>> diff --git a/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt 
>> b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> new file mode 100644
>> index 000..34fa9dd
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/phy/brcm,brcmstb-usb-phy.txt
>> @@ -0,0 +1,39 @@
>> +Broadcom STB USB PHY
>> +
>> +Required properties:
>> + - compatible: brcm,brcmstb-usb-phy
>> + - reg: two offset and length pairs. The second pair specifies the
>> +USB 3.0 related registers and is only required for PHYs
>> +that support USB 3.0
>
> I think the right way to model this should be to have separate subnodes for
> usb2 and usb3.

The first register block contains both the 2.0 and 3.0 phy registers.
The second register block contains an optional "workaround" register
set used on just a few SoCs to workaround a bug in the XHCI phy. Also,
this approach allows us to use the already parsed resource and saves
some additional code to get and map the registers.


>> + - #phy-cells: Shall be 1 as it expects one argument for setting
>> +the type of the PHY. Possible values are 0 (1.1 and 2.0),
>> +1 (3.0)
>> +
>> +
> spurious blank space.

Ok


>> +Optional Properties:
>> +- clocks : phandle + clock specifier for the phy clocks
>> +- clock-names: string, clock name
>> +- ipp: Invert Port Power
>> +- ioc: Invert Over Current detection
>> +- has_xhci: Contains an optional 3.0 PHY
>
> prefix all the broadcom specific properties with "bcm," or whatever is used 
> for
> broadcom specific properties.

Ok


>> +- device: PHY Device mode. Possible values are: 0 (Host), 1 (Device)
>> +  or 2 (DRD)
>> +
>> +
>> +
>
> spurious blank spaces..

Ok


>> +Example:
>> +
>> +usbphy_0: usb-phy@f0470200 {
>> + reg = <0xf0470200 0xb8>,
>> + <0xf0471940 0x6c0>;
>> + #address-cells = <1>;
>> + #size-cells = <1>;
>
> Why do you need address cells, size cells? Include it in the documentation.

I don't, I'll remove them.


>> diff --git a/drivers/phy/Kconfig b/drivers/phy/Kconfig
>> index 19bff3a..5ff5e47 100644
>> --- a/drivers/phy/Kconfig
>> +++ b/drivers/phy/Kconfig
>> @@ -443,6 +443,16 @@ config PHY_CYGNUS_PCIE
>> Enable this to support the Broadcom Cygnus PCIe PHY.
>> If unsure, say N.
>>
>> +config BRCM_USB_PHY
>> + tristate "Broadcom USB PHY driver"
>> + depends on OF && USB && ARCH_BRCMSTB
>
> Please also include COMPILE_TEST

Ok


>> + select GENERIC_PHY
>> + default y
>> + help
>> +   Enable this to support the Broadcom USB PHY on
>> +   Broadcom STB SoCs.
>> +   If unsure, say Y.
>
> Generally it is N.

Since this driver should work on any "ARCH_BRCMSTB" system and
"ARCH_BRCMSTB" is included in the "depends", it seems like this should
be defaulted to Y.


>> diff --git a/drivers/phy/phy-brcm-usb-init.c 
>> b/drivers/phy/phy-brcm-usb-init.c
>> new file mode 100644
>> index 000..f5d8c32
>> --- /dev/null
>> +++ b/drivers/phy/phy-brcm-usb-init.c
>> @@ -0,0 +1,792 @@
>> +/*
>> + * Copyright (C) 2014-2016 Broadcom
>> + *
>> + * Redistribution and use in source and binary forms, with or without
>> + * modification, are permitted provided that the following conditions
>> + * are met:
>> + * 1. Redistributions of source code must retain the above copyright
>> + *notice, this list of conditions and the following disclaimer.
>> + * 2. Redistributions in binary form must reproduce the above copyright
>> + *notice, this list of conditions and the following disclaimer in the
>> + *documentation and/or other materials provided with the distribution.
>> + * 3. Neither the name of the project nor the names of its contributors
>> + *may be used to endorse or promote products derived from this software
>> + *without specific