Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
@ 2019-09-06 12:03 Veeraiyan Chidambaram
  2019-09-06 12:03 ` [PATCH v2 2/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state() Veeraiyan Chidambaram
                   ` (3 more replies)
  0 siblings, 4 replies; 9+ messages in thread
From: Veeraiyan Chidambaram @ 2019-09-06 12:03 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Geert Uytterhoeven,
	Yoshihiro Shimoda, Linux-Renesas
  Cc: linux-usb, Andrew Gabbasov, Eugeniu Rosca, external.veeraiyan.c

From: Eugeniu Rosca <erosca@de.adit-jv.com>

Commit [1] enabled the possibility of checking the DVST (Device State
Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling
the irq_dev_state() handler if the DVST bit is set. But neither
commit [1] nor commit [2] actually enabled the DVSE (Device State
Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable
Register 0). As a consequence, irq_dev_state() handler is getting
called as a side effect of other (non-DVSE) interrupts being fired,
which definitely can't be relied upon, if DVST notifications are of
any value.

Why this doesn't hurt is because usbhsg_irq_dev_state() currently
doesn't do much except of a dev_dbg(). Once more work is added to
the handler (e.g. detecting device "Suspended" state and notifying
other USB gadget components about it), enabling DVSE becomes a hard
requirement. Do it in a standalone commit for better visibility and
clear explanation.

[1] f1407d5 ("usb: renesas_usbhs: Add Renesas USBHS common code")
[2] 2f98382 ("usb: renesas_usbhs: Add Renesas USBHS Gadget")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v2: No change
v1: https://patchwork.kernel.org/patch/10581479/

 drivers/usb/renesas_usbhs/mod.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/mod.c b/drivers/usb/renesas_usbhs/mod.c
index 7475c4f64724..87e08b1512ad 100644
--- a/drivers/usb/renesas_usbhs/mod.c
+++ b/drivers/usb/renesas_usbhs/mod.c
@@ -349,10 +349,6 @@ void usbhs_irq_callback_update(struct usbhs_priv *priv, struct usbhs_mod *mod)
 	 *	usbhs_interrupt
 	 */
 
-	/*
-	 * it don't enable DVSE (intenb0) here
-	 * but "mod->irq_dev_state" will be called.
-	 */
 	if (info->irq_vbus)
 		intenb0 |= VBSE;
 
@@ -363,6 +359,9 @@ void usbhs_irq_callback_update(struct usbhs_priv *priv, struct usbhs_mod *mod)
 		if (mod->irq_ctrl_stage)
 			intenb0 |= CTRE;
 
+		if (mod->irq_dev_state)
+			intenb0 |= DVSE;
+
 		if (mod->irq_empty && mod->irq_bempsts) {
 			usbhs_write(priv, BEMPENB, mod->irq_bempsts);
 			intenb0 |= BEMPE;
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 2/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state()
  2019-09-06 12:03 [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Veeraiyan Chidambaram
@ 2019-09-06 12:03 ` Veeraiyan Chidambaram
  2019-09-06 12:03 ` [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode Veeraiyan Chidambaram
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 9+ messages in thread
From: Veeraiyan Chidambaram @ 2019-09-06 12:03 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Geert Uytterhoeven,
	Yoshihiro Shimoda, Linux-Renesas
  Cc: linux-usb, Andrew Gabbasov, Eugeniu Rosca, external.veeraiyan.c

From: Eugeniu Rosca <erosca@de.adit-jv.com>

Similar to usbhs_status_get_ctrl_stage(), *_get_device_state() is not
supposed to return any error code since its return value is the DVSQ
bitfield of the INTSTS0 register. According to SoC HW manual rev1.00,
every single value of DVSQ[2:0] is valid and none is an error:

----8<----
Device State
000: Powered state
001: Default state
010: Address state
011: Configuration state
1xx: Suspended state
----8<----

Hence, simplify the function body. The motivation behind dropping the
switch/case construct is being able to implement reading the suspended
state. The latter (based on the above DVSQ[2:0] description) doesn't
have a unique value, but is rather a list of states (which makes
switch/case less suitable for reading/validating it):

100: (Suspended) Powered state
101: (Suspended) Default state
110: (Suspended) Address state
111: (Suspended) Configuration state

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v2: No change
v1: https://patchwork.kernel.org/patch/10581485/

 drivers/usb/renesas_usbhs/mod.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/mod.c b/drivers/usb/renesas_usbhs/mod.c
index 87e08b1512ad..3384308169f5 100644
--- a/drivers/usb/renesas_usbhs/mod.c
+++ b/drivers/usb/renesas_usbhs/mod.c
@@ -170,17 +170,7 @@ void usbhs_mod_remove(struct usbhs_priv *priv)
  */
 int usbhs_status_get_device_state(struct usbhs_irq_state *irq_state)
 {
-	int state = irq_state->intsts0 & DVSQ_MASK;
-
-	switch (state) {
-	case POWER_STATE:
-	case DEFAULT_STATE:
-	case ADDRESS_STATE:
-	case CONFIGURATION_STATE:
-		return state;
-	}
-
-	return -EIO;
+	return (int)irq_state->intsts0 & DVSQ_MASK;
 }
 
 int usbhs_status_get_ctrl_stage(struct usbhs_irq_state *irq_state)
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode
  2019-09-06 12:03 [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Veeraiyan Chidambaram
  2019-09-06 12:03 ` [PATCH v2 2/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state() Veeraiyan Chidambaram
@ 2019-09-06 12:03 ` Veeraiyan Chidambaram
  2019-09-09  7:05   ` Yoshihiro Shimoda
  2019-09-09  7:02 ` [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Yoshihiro Shimoda
  2019-09-09 13:05 ` Eugeniu Rosca
  3 siblings, 1 reply; 9+ messages in thread
From: Veeraiyan Chidambaram @ 2019-09-06 12:03 UTC (permalink / raw)
  To: Felipe Balbi, Greg Kroah-Hartman, Geert Uytterhoeven,
	Yoshihiro Shimoda, Linux-Renesas
  Cc: linux-usb, Andrew Gabbasov, Eugeniu Rosca, external.veeraiyan.c,
	Veeraiyan Chidambaram

From: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>

When R-Car Gen3 USB 3.0 is in Gadget mode, if host is detached an interrupt
will be generated and Suspended state bit is set in interrupt status
register. Interrupt handler will call driver->suspend(composite_suspend)
if suspended state bit is set. composite_suspend will call
ffs_func_suspend which will post FUNCTIONFS_SUSPEND and will be consumed
by user space application via /dev/ep0.

To be able to detect host detach, extend the DVSQ_MASK to cover the
Suspended bit of the DVSQ[2:0] bitfield from the Interrupt Status
Register 0 (INTSTS0) register and perform appropriate action in the
DVST interrupt handler (usbhsg_irq_dev_state).

Without this commit, disconnection of the phone from R-Car H3 ES2.0
Salvator-X CN9 port is not recognized and reverse role switch does
not not happen. If phone is connected again it does not enumerate.

With this commit, disconnection will be recognized and reverse role
switch will happen by a user space application. If phone is connected
again it will enumerate properly and will become visible in the output
of 'lsusb'.

Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>
Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
---
v2: if conditions changed
v1: https://patchwork.kernel.org/patch/10581489/

 drivers/usb/renesas_usbhs/common.h     |  3 ++-
 drivers/usb/renesas_usbhs/mod_gadget.c | 12 +++++++++---
 2 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
index 3777af848a35..142319c7585d 100644
--- a/drivers/usb/renesas_usbhs/common.h
+++ b/drivers/usb/renesas_usbhs/common.h
@@ -159,11 +159,12 @@ struct usbhs_priv;
 #define VBSTS	(1 << 7)	/* VBUS_0 and VBUSIN_0 Input Status */
 #define VALID	(1 << 3)	/* USB Request Receive */
 
-#define DVSQ_MASK		(0x3 << 4)	/* Device State */
+#define DVSQ_MASK		(0x7 << 4)	/* Device State */
 #define  POWER_STATE		(0 << 4)
 #define  DEFAULT_STATE		(1 << 4)
 #define  ADDRESS_STATE		(2 << 4)
 #define  CONFIGURATION_STATE	(3 << 4)
+#define  SUSPENDED_STATE	(4 << 4)
 
 #define CTSQ_MASK		(0x7)	/* Control Transfer Stage */
 #define  IDLE_SETUP_STAGE	0	/* Idle stage or setup stage */
diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
index 34ee9ebe12a3..d8972ab3c2f9 100644
--- a/drivers/usb/renesas_usbhs/mod_gadget.c
+++ b/drivers/usb/renesas_usbhs/mod_gadget.c
@@ -456,12 +456,18 @@ static int usbhsg_irq_dev_state(struct usbhs_priv *priv,
 {
 	struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
 	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
+	int state = usbhs_status_get_device_state(irq_state);
 
 	gpriv->gadget.speed = usbhs_bus_get_speed(priv);
 
-	dev_dbg(dev, "state = %x : speed : %d\n",
-		usbhs_status_get_device_state(irq_state),
-		gpriv->gadget.speed);
+	dev_dbg(dev, "state = %x : speed : %d\n", state, gpriv->gadget.speed);
+
+	if (gpriv->gadget.speed != USB_SPEED_UNKNOWN &&
+	    (state & SUSPENDED_STATE)) {
+		if (gpriv->driver && gpriv->driver->suspend)
+			gpriv->driver->suspend(&gpriv->gadget);
+		usb_gadget_set_state(&gpriv->gadget, USB_STATE_SUSPENDED);
+	}
 
 	return 0;
 }
-- 
2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
  2019-09-06 12:03 [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Veeraiyan Chidambaram
  2019-09-06 12:03 ` [PATCH v2 2/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state() Veeraiyan Chidambaram
  2019-09-06 12:03 ` [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode Veeraiyan Chidambaram
@ 2019-09-09  7:02 ` Yoshihiro Shimoda
  2019-09-09  9:55   ` Greg Kroah-Hartman
  2019-09-09 13:05 ` Eugeniu Rosca
  3 siblings, 1 reply; 9+ messages in thread
From: Yoshihiro Shimoda @ 2019-09-09  7:02 UTC (permalink / raw)
  To: Veeraiyan Chidambaram
  Cc: linux-usb, Andrew Gabbasov, REE erosca, Felipe Balbi,
	Greg Kroah-Hartman, Geert Uytterhoeven, Linux-Renesas

Hi Veeraiyan,

> From: Veeraiyan Chidambaram, Sent: Friday, September 6, 2019 9:04 PM
> 
> From: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Commit [1] enabled the possibility of checking the DVST (Device State
> Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling
> the irq_dev_state() handler if the DVST bit is set. But neither
> commit [1] nor commit [2] actually enabled the DVSE (Device State
> Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable
> Register 0). As a consequence, irq_dev_state() handler is getting
> called as a side effect of other (non-DVSE) interrupts being fired,
> which definitely can't be relied upon, if DVST notifications are of
> any value.
> 
> Why this doesn't hurt is because usbhsg_irq_dev_state() currently
> doesn't do much except of a dev_dbg(). Once more work is added to
> the handler (e.g. detecting device "Suspended" state and notifying
> other USB gadget components about it), enabling DVSE becomes a hard
> requirement. Do it in a standalone commit for better visibility and
> clear explanation.
> 
> [1] f1407d5 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> [2] 2f98382 ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>

I think your Signed-off-by is needed here and patch 2/3.

Best regards,
Yoshihiro Shimoda


^ permalink raw reply	[flat|nested] 9+ messages in thread

* RE: [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode
  2019-09-06 12:03 ` [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode Veeraiyan Chidambaram
@ 2019-09-09  7:05   ` Yoshihiro Shimoda
  0 siblings, 0 replies; 9+ messages in thread
From: Yoshihiro Shimoda @ 2019-09-09  7:05 UTC (permalink / raw)
  To: Veeraiyan Chidambaram, Felipe Balbi, Greg Kroah-Hartman,
	Geert Uytterhoeven, Linux-Renesas
  Cc: linux-usb, Andrew Gabbasov, REE erosca, Veeraiyan Chidambaram

Hi Veeraiyan,

Thank you for your patch!

> From: Veeraiyan Chidambaram, Sent: Friday, September 6, 2019 9:04 PM
<snip>
> When R-Car Gen3 USB 3.0 is in Gadget mode, if host is detached an interrupt

This should be "USB 2.0" instead of "USB 3.0".

Best regards,
Yoshihiro Shimoda

> will be generated and Suspended state bit is set in interrupt status
> register. Interrupt handler will call driver->suspend(composite_suspend)
> if suspended state bit is set. composite_suspend will call
> ffs_func_suspend which will post FUNCTIONFS_SUSPEND and will be consumed
> by user space application via /dev/ep0.
> 
> To be able to detect host detach, extend the DVSQ_MASK to cover the
> Suspended bit of the DVSQ[2:0] bitfield from the Interrupt Status
> Register 0 (INTSTS0) register and perform appropriate action in the
> DVST interrupt handler (usbhsg_irq_dev_state).
> 
> Without this commit, disconnection of the phone from R-Car H3 ES2.0
> Salvator-X CN9 port is not recognized and reverse role switch does
> not not happen. If phone is connected again it does not enumerate.
> 
> With this commit, disconnection will be recognized and reverse role
> switch will happen by a user space application. If phone is connected
> again it will enumerate properly and will become visible in the output
> of 'lsusb'.
> 
> Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v2: if conditions changed
> v1: https://patchwork.kernel.org/patch/10581489/
> 
>  drivers/usb/renesas_usbhs/common.h     |  3 ++-
>  drivers/usb/renesas_usbhs/mod_gadget.c | 12 +++++++++---
>  2 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/usb/renesas_usbhs/common.h b/drivers/usb/renesas_usbhs/common.h
> index 3777af848a35..142319c7585d 100644
> --- a/drivers/usb/renesas_usbhs/common.h
> +++ b/drivers/usb/renesas_usbhs/common.h
> @@ -159,11 +159,12 @@ struct usbhs_priv;
>  #define VBSTS	(1 << 7)	/* VBUS_0 and VBUSIN_0 Input Status */
>  #define VALID	(1 << 3)	/* USB Request Receive */
> 
> -#define DVSQ_MASK		(0x3 << 4)	/* Device State */
> +#define DVSQ_MASK		(0x7 << 4)	/* Device State */
>  #define  POWER_STATE		(0 << 4)
>  #define  DEFAULT_STATE		(1 << 4)
>  #define  ADDRESS_STATE		(2 << 4)
>  #define  CONFIGURATION_STATE	(3 << 4)
> +#define  SUSPENDED_STATE	(4 << 4)
> 
>  #define CTSQ_MASK		(0x7)	/* Control Transfer Stage */
>  #define  IDLE_SETUP_STAGE	0	/* Idle stage or setup stage */
> diff --git a/drivers/usb/renesas_usbhs/mod_gadget.c b/drivers/usb/renesas_usbhs/mod_gadget.c
> index 34ee9ebe12a3..d8972ab3c2f9 100644
> --- a/drivers/usb/renesas_usbhs/mod_gadget.c
> +++ b/drivers/usb/renesas_usbhs/mod_gadget.c
> @@ -456,12 +456,18 @@ static int usbhsg_irq_dev_state(struct usbhs_priv *priv,
>  {
>  	struct usbhsg_gpriv *gpriv = usbhsg_priv_to_gpriv(priv);
>  	struct device *dev = usbhsg_gpriv_to_dev(gpriv);
> +	int state = usbhs_status_get_device_state(irq_state);
> 
>  	gpriv->gadget.speed = usbhs_bus_get_speed(priv);
> 
> -	dev_dbg(dev, "state = %x : speed : %d\n",
> -		usbhs_status_get_device_state(irq_state),
> -		gpriv->gadget.speed);
> +	dev_dbg(dev, "state = %x : speed : %d\n", state, gpriv->gadget.speed);
> +
> +	if (gpriv->gadget.speed != USB_SPEED_UNKNOWN &&
> +	    (state & SUSPENDED_STATE)) {
> +		if (gpriv->driver && gpriv->driver->suspend)
> +			gpriv->driver->suspend(&gpriv->gadget);
> +		usb_gadget_set_state(&gpriv->gadget, USB_STATE_SUSPENDED);
> +	}
> 
>  	return 0;
>  }
> --
> 2.7.4


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
  2019-09-09  7:02 ` [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Yoshihiro Shimoda
@ 2019-09-09  9:55   ` Greg Kroah-Hartman
  2019-09-09 11:04     ` veeraiyan chidambaram
  0 siblings, 1 reply; 9+ messages in thread
From: Greg Kroah-Hartman @ 2019-09-09  9:55 UTC (permalink / raw)
  To: Yoshihiro Shimoda
  Cc: Veeraiyan Chidambaram, linux-usb, Andrew Gabbasov, REE erosca,
	Felipe Balbi, Geert Uytterhoeven, Linux-Renesas

On Mon, Sep 09, 2019 at 07:02:46AM +0000, Yoshihiro Shimoda wrote:
> Hi Veeraiyan,
> 
> > From: Veeraiyan Chidambaram, Sent: Friday, September 6, 2019 9:04 PM
> > 
> > From: Eugeniu Rosca <erosca@de.adit-jv.com>
> > 
> > Commit [1] enabled the possibility of checking the DVST (Device State
> > Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling
> > the irq_dev_state() handler if the DVST bit is set. But neither
> > commit [1] nor commit [2] actually enabled the DVSE (Device State
> > Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable
> > Register 0). As a consequence, irq_dev_state() handler is getting
> > called as a side effect of other (non-DVSE) interrupts being fired,
> > which definitely can't be relied upon, if DVST notifications are of
> > any value.
> > 
> > Why this doesn't hurt is because usbhsg_irq_dev_state() currently
> > doesn't do much except of a dev_dbg(). Once more work is added to
> > the handler (e.g. detecting device "Suspended" state and notifying
> > other USB gadget components about it), enabling DVSE becomes a hard
> > requirement. Do it in a standalone commit for better visibility and
> > clear explanation.
> > 
> > [1] f1407d5 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> > [2] 2f98382 ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> > 
> > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> I think your Signed-off-by is needed here and patch 2/3.

Yes, I can't take this as-is without that.

thanks,

greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
  2019-09-09  9:55   ` Greg Kroah-Hartman
@ 2019-09-09 11:04     ` veeraiyan chidambaram
  0 siblings, 0 replies; 9+ messages in thread
From: veeraiyan chidambaram @ 2019-09-09 11:04 UTC (permalink / raw)
  To: Greg Kroah-Hartman
  Cc: Yoshihiro Shimoda, linux-usb, Andrew Gabbasov, REE erosca,
	Felipe Balbi, Geert Uytterhoeven, Linux-Renesas

Hello Shimoda-san, hello greg,

Thanks. 
I have addressed the findings and prepared a v3 patch, please find
it below.
 1. https://patchwork.kernel.org/patch/11137693/
 2. https://patchwork.kernel.org/patch/11137697/
 3. https://patchwork.kernel.org/patch/11137693/

Regards,
Veeraiyan Chidambaram

On Mon, Sep 09, 2019 at 10:55:43AM +0100, Greg Kroah-Hartman wrote:
> On Mon, Sep 09, 2019 at 07:02:46AM +0000, Yoshihiro Shimoda wrote:
> > Hi Veeraiyan,
> > 
> > > From: Veeraiyan Chidambaram, Sent: Friday, September 6, 2019 9:04 PM
> > > 
> > > From: Eugeniu Rosca <erosca@de.adit-jv.com>
> > > 
> > > Commit [1] enabled the possibility of checking the DVST (Device State
> > > Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling
> > > the irq_dev_state() handler if the DVST bit is set. But neither
> > > commit [1] nor commit [2] actually enabled the DVSE (Device State
> > > Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable
> > > Register 0). As a consequence, irq_dev_state() handler is getting
> > > called as a side effect of other (non-DVSE) interrupts being fired,
> > > which definitely can't be relied upon, if DVST notifications are of
> > > any value.
> > > 
> > > Why this doesn't hurt is because usbhsg_irq_dev_state() currently
> > > doesn't do much except of a dev_dbg(). Once more work is added to
> > > the handler (e.g. detecting device "Suspended" state and notifying
> > > other USB gadget components about it), enabling DVSE becomes a hard
> > > requirement. Do it in a standalone commit for better visibility and
> > > clear explanation.
> > > 
> > > [1] f1407d5 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> > > [2] 2f98382 ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> > > 
> > > Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> > 
> > I think your Signed-off-by is needed here and patch 2/3.
> 
> Yes, I can't take this as-is without that.
> 
> thanks,
> 
> greg k-h

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
  2019-09-06 12:03 [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Veeraiyan Chidambaram
                   ` (2 preceding siblings ...)
  2019-09-09  7:02 ` [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Yoshihiro Shimoda
@ 2019-09-09 13:05 ` Eugeniu Rosca
  2019-09-09 16:01   ` veeraiyan chidambaram
  3 siblings, 1 reply; 9+ messages in thread
From: Eugeniu Rosca @ 2019-09-09 13:05 UTC (permalink / raw)
  To: Veeraiyan Chidambaram
  Cc: Felipe Balbi, Greg Kroah-Hartman, Geert Uytterhoeven,
	Yoshihiro Shimoda, Linux-Renesas, linux-usb, Andrew Gabbasov,
	Eugeniu Rosca, Eugeniu Rosca

Hi Veeraiyan,

On Fri, Sep 06, 2019 at 02:03:49PM +0200, Veeraiyan Chidambaram wrote:
> From: Eugeniu Rosca <erosca@de.adit-jv.com>
> 
> Commit [1] enabled the possibility of checking the DVST (Device State
> Transition) bit of INTSTS0 (Interrupt Status Register 0) and calling
> the irq_dev_state() handler if the DVST bit is set. But neither
> commit [1] nor commit [2] actually enabled the DVSE (Device State
> Transition Interrupt Enable) bit in the INTENB0 (Interrupt Enable
> Register 0). As a consequence, irq_dev_state() handler is getting
> called as a side effect of other (non-DVSE) interrupts being fired,
> which definitely can't be relied upon, if DVST notifications are of
> any value.
> 
> Why this doesn't hurt is because usbhsg_irq_dev_state() currently
> doesn't do much except of a dev_dbg(). Once more work is added to
> the handler (e.g. detecting device "Suspended" state and notifying
> other USB gadget components about it), enabling DVSE becomes a hard
> requirement. Do it in a standalone commit for better visibility and
> clear explanation.
> 
> [1] f1407d5 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> [2] 2f98382 ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> 
> Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> ---
> v2: No change
> v1: https://patchwork.kernel.org/patch/10581479/

It looks like this series changes the patch order of v1.
Could you kindly stick to the original order? Many thanks.

-- 
Best Regards,
Eugeniu.

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt
  2019-09-09 13:05 ` Eugeniu Rosca
@ 2019-09-09 16:01   ` veeraiyan chidambaram
  0 siblings, 0 replies; 9+ messages in thread
From: veeraiyan chidambaram @ 2019-09-09 16:01 UTC (permalink / raw)
  To: Eugeniu Rosca
  Cc: Felipe Balbi, Greg Kroah-Hartman, Geert Uytterhoeven,
	Yoshihiro Shimoda, Linux-Renesas, linux-usb, Andrew Gabbasov,
	Eugeniu Rosca

Hello Eugeniu,

Thanks for pointing out.

On Mon, Sep 09, 2019 at 03:05:25PM +0200, Eugeniu Rosca wrote:
> Hi Veeraiyan,
> 
> On Fri, Sep 06, 2019 at 02:03:49PM +0200, Veeraiyan Chidambaram wrote:
> > From: Eugeniu Rosca <erosca@de.adit-jv.com>
> > 
> It looks like this series changes the patch order of v1.
> Could you kindly stick to the original order? Many thanks.

i have restored the patch order as of v1.

Best Regards,
Veeraiyan chidambaram
> -- 
> Best Regards,
> Eugeniu.

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, back to index

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-09-06 12:03 [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Veeraiyan Chidambaram
2019-09-06 12:03 ` [PATCH v2 2/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state() Veeraiyan Chidambaram
2019-09-06 12:03 ` [PATCH v2 3/3] usb: renesas_usbhs: add suspend event support in gadget mode Veeraiyan Chidambaram
2019-09-09  7:05   ` Yoshihiro Shimoda
2019-09-09  7:02 ` [PATCH v2 1/3] usb: renesas_usbhs: enable DVSE interrupt Yoshihiro Shimoda
2019-09-09  9:55   ` Greg Kroah-Hartman
2019-09-09 11:04     ` veeraiyan chidambaram
2019-09-09 13:05 ` Eugeniu Rosca
2019-09-09 16:01   ` veeraiyan chidambaram

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org linux-usb@archiver.kernel.org
	public-inbox-index linux-usb


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/ public-inbox