Re: [PATCH RESEND] usb: dwc2: Fix a bug in reading the endpoint directions from reg.

2015-02-03 Thread John Youn 
On 02/02/2015 02:55 PM, Roshan Pius wrote:
> According to  the DWC2 datasheet, the HWCFG1 register stores
> the configured endpoint directions for endpoints 0-15 in bit positions
> 0-31.
> ==
> Endpoint Direction (EpDir)
> This 32-bit field uses two bits per endpoint to determine the endpoint
> direction.
> Endpoint
> Bits [31:30]: Endpoint 15 direction
> Bits [29:28]: Endpoint 14 direction
> 
> Bits [3:2]: Endpoint 1 direction
> Bits[1:0]: Endpoint 0 direction (always BIDIR)
> ==
> 
> The DWC2 driver is currently interpreting the contents of the register
> as directions for endpoints 1-15 which leads to an error in determining
> the configured endpoint directions in the core because the first 2 bits
> determine the direction of endpoint 0 and not 1.
> 
> This is based on testing/next branch in Felipe's git.
> 
> Signed-off-by: Roshan Pius 
> ---
>  drivers/usb/dwc2/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
> index 50ae096..706165c 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3167,7 +3167,7 @@ static int s3c_hsotg_hw_cfg(struct dwc2_hsotg *hsotg)
>   hsotg->eps_out[0] = hsotg->eps_in[0];
>  
>   cfg = readl(hsotg->regs + GHWCFG1);
> - for (i = 1; i < hsotg->num_of_eps; i++, cfg >>= 2) {
> + for (i = 1, cfg >>= 2; i < hsotg->num_of_eps; i++, cfg >>= 2) {
>   ep_type = cfg & 3;
>   /* Direction in or both */
>   if (!(ep_type & 2)) {
> 


Good catch on this. 

Acked-by: John Youn 


Hi Felipe,

Any chance you'll accept this into your 3.20 queue?

It affects any DWC2 core instance that has unidirectional endpoints defined.

Regards,
John







--
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 RESEND] usb: dwc2: Fix a bug in reading the endpoint directions from reg.

2015-02-03 Thread Kaukab, Yousaf 
Hi Roshan,

> -Original Message-
> From: Roshan Pius [mailto:rp...@chromium.org]
> Sent: Monday, February 2, 2015 11:56 PM
> To: linux-usb@vger.kernel.org
> Cc: benc...@chromium.org; Kaukab, Yousaf; Roshan Pius
> Subject: [PATCH RESEND] usb: dwc2: Fix a bug in reading the endpoint
> directions from reg.
> 
> According to  the DWC2 datasheet, the HWCFG1 register stores the configured
> endpoint directions for endpoints 0-15 in bit positions 0-31.
> ==
> Endpoint Direction (EpDir)
> This 32-bit field uses two bits per endpoint to determine the endpoint 
> direction.
> Endpoint
> Bits [31:30]: Endpoint 15 direction
> Bits [29:28]: Endpoint 14 direction
> 
> Bits [3:2]: Endpoint 1 direction
> Bits[1:0]: Endpoint 0 direction (always BIDIR) ==
> 
> The DWC2 driver is currently interpreting the contents of the register as
> directions for endpoints 1-15 which leads to an error in determining the
> configured endpoint directions in the core because the first 2 bits determine
> the direction of endpoint 0 and not 1.
> 
> This is based on testing/next branch in Felipe's git.
> 
> Signed-off-by: Roshan Pius 
> ---
>  drivers/usb/dwc2/gadget.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c index
> 50ae096..706165c 100644
> --- a/drivers/usb/dwc2/gadget.c
> +++ b/drivers/usb/dwc2/gadget.c
> @@ -3167,7 +3167,7 @@ static int s3c_hsotg_hw_cfg(struct dwc2_hsotg
> *hsotg)
>   hsotg->eps_out[0] = hsotg->eps_in[0];
> 
>   cfg = readl(hsotg->regs + GHWCFG1);
> - for (i = 1; i < hsotg->num_of_eps; i++, cfg >>= 2) {
> + for (i = 1, cfg >>= 2; i < hsotg->num_of_eps; i++, cfg >>= 2) {

Nice catch! I wouldn't have found it on my hardware as GHWCFG1 is always 0.

>   ep_type = cfg & 3;
>   /* Direction in or both */
>   if (!(ep_type & 2)) {
> --
> 2.2.0.rc0.207.ga3a616c

BR,
Yousaf

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

[PATCH RESEND] usb: dwc2: Fix a bug in reading the endpoint directions from reg.

2015-02-02 Thread Roshan Pius 
According to  the DWC2 datasheet, the HWCFG1 register stores
the configured endpoint directions for endpoints 0-15 in bit positions
0-31.
==
Endpoint Direction (EpDir)
This 32-bit field uses two bits per endpoint to determine the endpoint
direction.
Endpoint
Bits [31:30]: Endpoint 15 direction
Bits [29:28]: Endpoint 14 direction

Bits [3:2]: Endpoint 1 direction
Bits[1:0]: Endpoint 0 direction (always BIDIR)
==

The DWC2 driver is currently interpreting the contents of the register
as directions for endpoints 1-15 which leads to an error in determining
the configured endpoint directions in the core because the first 2 bits
determine the direction of endpoint 0 and not 1.

This is based on testing/next branch in Felipe's git.

Signed-off-by: Roshan Pius 
---
 drivers/usb/dwc2/gadget.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index 50ae096..706165c 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -3167,7 +3167,7 @@ static int s3c_hsotg_hw_cfg(struct dwc2_hsotg *hsotg)
hsotg->eps_out[0] = hsotg->eps_in[0];
 
cfg = readl(hsotg->regs + GHWCFG1);
-   for (i = 1; i < hsotg->num_of_eps; i++, cfg >>= 2) {
+   for (i = 1, cfg >>= 2; i < hsotg->num_of_eps; i++, cfg >>= 2) {
ep_type = cfg & 3;
/* Direction in or both */
if (!(ep_type & 2)) {
-- 
2.2.0.rc0.207.ga3a616c

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