Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

2014-11-14 Thread Marek Szyprowski 


On 2014-11-13 21:50, Paul Zimmerman wrote:

From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Thursday, November 13, 2014 5:40 AM

On 2014-10-31 19:15, Paul Zimmerman wrote:

From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Friday, October 31, 2014 1:04 AM
To: linux-...@vger.kernel.org; linux-samsung-soc@vger.kernel.org
Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof 
Kozlowski; Felipe

Balbi

Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
session' irq

This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
look a bit more suitable for this event, but it is asserted only in
host mode, so in device mode we need to use something else.

Additional check has been added in s3c_hsotg_disconnect() function
to ensure that the event is reported only once after successful device
enumeration.

Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
   drivers/usb/dwc2/core.h   |  1 +
   drivers/usb/dwc2/gadget.c | 10 ++
   2 files changed, 11 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..b42df32e7737 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -212,6 +212,7 @@ struct s3c_hsotg {
struct usb_gadget   gadget;
unsigned intsetup;
unsigned long   last_rst;
+   unsigned intaddress;
struct s3c_hsotg_ep *eps;
   };

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..6304efba11aa 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
*hsotg,
 DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
writel(dcfg, hsotg-regs + DCFG);

+   hsotg-address = ctrl-wValue;
dev_info(hsotg-dev, new address %d\n, ctrl-wValue);

ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
@@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
   {
unsigned ep;

+   if (!hsotg-address)
+   return;
+
+   hsotg-address = 0;
for (ep = 0; ep  hsotg-num_of_eps; ep++)
kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);

@@ -2290,6 +2295,11 @@ irq_retry:
dev_info(hsotg-dev, OTGInt: %08x\n, otgint);

writel(otgint, hsotg-regs + GOTGINT);
+
+   if (otgint  GOTGINT_SES_END_DET) {
+   s3c_hsotg_disconnect(hsotg);
+   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
+   }
}

if (gintsts  GINTSTS_SESSREQINT) {

I don't think this is right. The host can send control requests to
the device before it sends a SetAddress to change from the default
address of 0. So if a GOTGINT_SES_END_DET happens before the
SetAddress, it will be ignored.

Or am I missing something?

Well, right. However before finishing enumeration (setting the address)
host usually
only retrieves some usb descriptors what doesn't change the state of the
gadget.
Right now we always reported 'disconnected' event before setting the new
address,
what is a bit overkill (in some cases gadget driver got this even more
than once).
The above code handles all cases correctly and reports disconnect event
only once.

Well, if the disconnect happens before the SetAddress, the disconnect
won't be reported at all. Unless I'm reading the code wrong.


Right, although this is not really an issue for any gadget driver. 
However if you
prefer to report disconnect event more than once, I'm okay and I will 
remove the

check based on the device address.

Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

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

Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

2014-11-14 Thread Marek Szyprowski 

Hello,

On 2014-11-13 21:50, Paul Zimmerman wrote:

From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Thursday, November 13, 2014 5:40 AM

On 2014-10-31 19:15, Paul Zimmerman wrote:

From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Friday, October 31, 2014 1:04 AM
To: linux-...@vger.kernel.org; linux-samsung-soc@vger.kernel.org
Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof 
Kozlowski; Felipe

Balbi

Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
session' irq

This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
look a bit more suitable for this event, but it is asserted only in
host mode, so in device mode we need to use something else.

Additional check has been added in s3c_hsotg_disconnect() function
to ensure that the event is reported only once after successful device
enumeration.

Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
   drivers/usb/dwc2/core.h   |  1 +
   drivers/usb/dwc2/gadget.c | 10 ++
   2 files changed, 11 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..b42df32e7737 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -212,6 +212,7 @@ struct s3c_hsotg {
struct usb_gadget   gadget;
unsigned intsetup;
unsigned long   last_rst;
+   unsigned intaddress;
struct s3c_hsotg_ep *eps;
   };

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..6304efba11aa 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
*hsotg,
 DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
writel(dcfg, hsotg-regs + DCFG);

+   hsotg-address = ctrl-wValue;
dev_info(hsotg-dev, new address %d\n, ctrl-wValue);

ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
@@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
   {
unsigned ep;

+   if (!hsotg-address)
+   return;
+
+   hsotg-address = 0;
for (ep = 0; ep  hsotg-num_of_eps; ep++)
kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);

@@ -2290,6 +2295,11 @@ irq_retry:
dev_info(hsotg-dev, OTGInt: %08x\n, otgint);

writel(otgint, hsotg-regs + GOTGINT);
+
+   if (otgint  GOTGINT_SES_END_DET) {
+   s3c_hsotg_disconnect(hsotg);
+   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
+   }
}

if (gintsts  GINTSTS_SESSREQINT) {

I don't think this is right. The host can send control requests to
the device before it sends a SetAddress to change from the default
address of 0. So if a GOTGINT_SES_END_DET happens before the
SetAddress, it will be ignored.

Or am I missing something?

Well, right. However before finishing enumeration (setting the address)
host usually
only retrieves some usb descriptors what doesn't change the state of the
gadget.
Right now we always reported 'disconnected' event before setting the new
address,
what is a bit overkill (in some cases gadget driver got this even more
than once).
The above code handles all cases correctly and reports disconnect event
only once.

Well, if the disconnect happens before the SetAddress, the disconnect
won't be reported at all. Unless I'm reading the code wrong.


Ok, I found other way to ensure that disconnect event is reported only 
once.

I will post a patch in a few minutes.


Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

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

Re: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

2014-11-13 Thread Marek Szyprowski 

Hello,

On 2014-10-31 19:15, Paul Zimmerman wrote:

From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
Sent: Friday, October 31, 2014 1:04 AM
To: linux-...@vger.kernel.org; linux-samsung-soc@vger.kernel.org
Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; Krzysztof 
Kozlowski; Felipe Balbi
Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
session' irq

This patch adds a call to s3c_hsotg_disconnect() from 'end session'
interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
look a bit more suitable for this event, but it is asserted only in
host mode, so in device mode we need to use something else.

Additional check has been added in s3c_hsotg_disconnect() function
to ensure that the event is reported only once after successful device
enumeration.

Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 10 ++
  2 files changed, 11 insertions(+)

diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
index 55c90c53f2d6..b42df32e7737 100644
--- a/drivers/usb/dwc2/core.h
+++ b/drivers/usb/dwc2/core.h
@@ -212,6 +212,7 @@ struct s3c_hsotg {
struct usb_gadget   gadget;
unsigned intsetup;
unsigned long   last_rst;
+   unsigned intaddress;
struct s3c_hsotg_ep *eps;
  };

diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
index fcd2bb55ccca..6304efba11aa 100644
--- a/drivers/usb/dwc2/gadget.c
+++ b/drivers/usb/dwc2/gadget.c
@@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
*hsotg,
 DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
writel(dcfg, hsotg-regs + DCFG);

+   hsotg-address = ctrl-wValue;
dev_info(hsotg-dev, new address %d\n, ctrl-wValue);

ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
@@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg *hsotg)
  {
unsigned ep;

+   if (!hsotg-address)
+   return;
+
+   hsotg-address = 0;
for (ep = 0; ep  hsotg-num_of_eps; ep++)
kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);

@@ -2290,6 +2295,11 @@ irq_retry:
dev_info(hsotg-dev, OTGInt: %08x\n, otgint);

writel(otgint, hsotg-regs + GOTGINT);
+
+   if (otgint  GOTGINT_SES_END_DET) {
+   s3c_hsotg_disconnect(hsotg);
+   hsotg-gadget.speed = USB_SPEED_UNKNOWN;
+   }
}

if (gintsts  GINTSTS_SESSREQINT) {

I don't think this is right. The host can send control requests to
the device before it sends a SetAddress to change from the default
address of 0. So if a GOTGINT_SES_END_DET happens before the
SetAddress, it will be ignored.

Or am I missing something?


Well, right. However before finishing enumeration (setting the address) 
host usually
only retrieves some usb descriptors what doesn't change the state of the 
gadget.
Right now we always reported 'disconnected' event before setting the new 
address,
what is a bit overkill (in some cases gadget driver got this even more 
than once).
The above code handles all cases correctly and reports disconnect event 
only once.


Best regards
--
Marek Szyprowski, PhD
Samsung RD Institute Poland

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

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

2014-11-13 Thread Paul Zimmerman 
 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Thursday, November 13, 2014 5:40 AM
 
 On 2014-10-31 19:15, Paul Zimmerman wrote:
  From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
  Sent: Friday, October 31, 2014 1:04 AM
  To: linux-...@vger.kernel.org; linux-samsung-soc@vger.kernel.org
  Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; 
  Krzysztof Kozlowski; Felipe
 Balbi
  Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
  session' irq
 
  This patch adds a call to s3c_hsotg_disconnect() from 'end session'
  interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
  about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
  look a bit more suitable for this event, but it is asserted only in
  host mode, so in device mode we need to use something else.
 
  Additional check has been added in s3c_hsotg_disconnect() function
  to ensure that the event is reported only once after successful device
  enumeration.
 
  Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
  ---
drivers/usb/dwc2/core.h   |  1 +
drivers/usb/dwc2/gadget.c | 10 ++
2 files changed, 11 insertions(+)
 
  diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
  index 55c90c53f2d6..b42df32e7737 100644
  --- a/drivers/usb/dwc2/core.h
  +++ b/drivers/usb/dwc2/core.h
  @@ -212,6 +212,7 @@ struct s3c_hsotg {
 struct usb_gadget   gadget;
 unsigned intsetup;
 unsigned long   last_rst;
  +  unsigned intaddress;
 struct s3c_hsotg_ep *eps;
};
 
  diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
  index fcd2bb55ccca..6304efba11aa 100644
  --- a/drivers/usb/dwc2/gadget.c
  +++ b/drivers/usb/dwc2/gadget.c
  @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct 
  s3c_hsotg *hsotg,
  DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
 writel(dcfg, hsotg-regs + DCFG);
 
  +  hsotg-address = ctrl-wValue;
 dev_info(hsotg-dev, new address %d\n, ctrl-wValue);
 
 ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
  @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
  *hsotg)
{
 unsigned ep;
 
  +  if (!hsotg-address)
  +  return;
  +
  +  hsotg-address = 0;
 for (ep = 0; ep  hsotg-num_of_eps; ep++)
 kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
  @@ -2290,6 +2295,11 @@ irq_retry:
 dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
 writel(otgint, hsotg-regs + GOTGINT);
  +
  +  if (otgint  GOTGINT_SES_END_DET) {
  +  s3c_hsotg_disconnect(hsotg);
  +  hsotg-gadget.speed = USB_SPEED_UNKNOWN;
  +  }
 }
 
 if (gintsts  GINTSTS_SESSREQINT) {
  I don't think this is right. The host can send control requests to
  the device before it sends a SetAddress to change from the default
  address of 0. So if a GOTGINT_SES_END_DET happens before the
  SetAddress, it will be ignored.
 
  Or am I missing something?
 
 Well, right. However before finishing enumeration (setting the address)
 host usually
 only retrieves some usb descriptors what doesn't change the state of the
 gadget.
 Right now we always reported 'disconnected' event before setting the new
 address,
 what is a bit overkill (in some cases gadget driver got this even more
 than once).
 The above code handles all cases correctly and reports disconnect event
 only once.

Well, if the disconnect happens before the SetAddress, the disconnect
won't be reported at all. Unless I'm reading the code wrong.

-- 
Paul

RE: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end session' irq

2014-10-31 Thread Paul Zimmerman 
 From: Marek Szyprowski [mailto:m.szyprow...@samsung.com]
 Sent: Friday, October 31, 2014 1:04 AM
 To: linux-...@vger.kernel.org; linux-samsung-soc@vger.kernel.org
 Cc: Marek Szyprowski; Kyungmin Park; Robert Baldyga; Paul Zimmerman; 
 Krzysztof Kozlowski; Felipe Balbi
 Subject: [PATCH v3] usb: dwc2/gadget: report disconnect event from 'end 
 session' irq
 
 This patch adds a call to s3c_hsotg_disconnect() from 'end session'
 interrupt (GOTGINT_SES_END_DET) to correctly notify gadget subsystem
 about unplugged usb cable. 'disconnected' interrupt (DISCONNINT) might
 look a bit more suitable for this event, but it is asserted only in
 host mode, so in device mode we need to use something else.
 
 Additional check has been added in s3c_hsotg_disconnect() function
 to ensure that the event is reported only once after successful device
 enumeration.
 
 Signed-off-by: Marek Szyprowski m.szyprow...@samsung.com
 ---
  drivers/usb/dwc2/core.h   |  1 +
  drivers/usb/dwc2/gadget.c | 10 ++
  2 files changed, 11 insertions(+)
 
 diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h
 index 55c90c53f2d6..b42df32e7737 100644
 --- a/drivers/usb/dwc2/core.h
 +++ b/drivers/usb/dwc2/core.h
 @@ -212,6 +212,7 @@ struct s3c_hsotg {
   struct usb_gadget   gadget;
   unsigned intsetup;
   unsigned long   last_rst;
 + unsigned intaddress;
   struct s3c_hsotg_ep *eps;
  };
 
 diff --git a/drivers/usb/dwc2/gadget.c b/drivers/usb/dwc2/gadget.c
 index fcd2bb55ccca..6304efba11aa 100644
 --- a/drivers/usb/dwc2/gadget.c
 +++ b/drivers/usb/dwc2/gadget.c
 @@ -1114,6 +1114,7 @@ static void s3c_hsotg_process_control(struct s3c_hsotg 
 *hsotg,
DCFG_DEVADDR_SHIFT)  DCFG_DEVADDR_MASK;
   writel(dcfg, hsotg-regs + DCFG);
 
 + hsotg-address = ctrl-wValue;
   dev_info(hsotg-dev, new address %d\n, ctrl-wValue);
 
   ret = s3c_hsotg_send_reply(hsotg, ep0, NULL, 0);
 @@ -2031,6 +2032,10 @@ static void s3c_hsotg_disconnect(struct s3c_hsotg 
 *hsotg)
  {
   unsigned ep;
 
 + if (!hsotg-address)
 + return;
 +
 + hsotg-address = 0;
   for (ep = 0; ep  hsotg-num_of_eps; ep++)
   kill_all_requests(hsotg, hsotg-eps[ep], -ESHUTDOWN, true);
 
 @@ -2290,6 +2295,11 @@ irq_retry:
   dev_info(hsotg-dev, OTGInt: %08x\n, otgint);
 
   writel(otgint, hsotg-regs + GOTGINT);
 +
 + if (otgint  GOTGINT_SES_END_DET) {
 + s3c_hsotg_disconnect(hsotg);
 + hsotg-gadget.speed = USB_SPEED_UNKNOWN;
 + }
   }
 
   if (gintsts  GINTSTS_SESSREQINT) {

I don't think this is right. The host can send control requests to
the device before it sends a SetAddress to change from the default
address of 0. So if a GOTGINT_SES_END_DET happens before the
SetAddress, it will be ignored.

Or am I missing something?

-- 
Paul

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