[PATCH v4 1/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state()

[PATCH v4 1/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state()

[Veeraiyan Chidambaram]

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>
Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>
---
v4: patch sequence change
v3: https://patchwork.kernel.org/patch/11137697/
v2: https://patchwork.kernel.org/patch/11135111/
v1: https://patchwork.kernel.org/patch/10581479/

 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 7475c4f64724..4fbb1d538b82 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

[PATCH v4 2/3] usb: renesas_usbhs: enable DVSE interrupt

[Veeraiyan Chidambaram]

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] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")
[2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")

Signed-off-by: Eugeniu Rosca <erosca@de.adit-jv.com>
Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>
---
v4: patch sequence change
v3: https://patchwork.kernel.org/patch/11137693/
v2: https://patchwork.kernel.org/patch/11135109/
v1: https://patchwork.kernel.org/patch/10581485/
 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 4fbb1d538b82..3384308169f5 100644
--- a/drivers/usb/renesas_usbhs/mod.c
+++ b/drivers/usb/renesas_usbhs/mod.c
@@ -339,10 +339,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;
 
@@ -353,6 +349,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

RE: [PATCH v4 2/3] usb: renesas_usbhs: enable DVSE interrupt

[Yoshihiro Shimoda]

Hi Veeraiyan,

Thank you for the patch!

> From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM
<snip>
> 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] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")

I'm afraid I should have realized but, according to checkpatch.pl,
these formats cause ERROR like below. So, they should be fixed.

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")'
#90:
[1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")

ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")'
#91:
[2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")

Best regards,
Yoshihiro Shimoda

Re: [PATCH v4 2/3] usb: renesas_usbhs: enable DVSE interrupt

[veeraiyan chidambaram]

Hello shimoda-san,

Thanks for point out checkpatch warning. After resolving checkpatch warning,
below  checkpatch warning is seen.

WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
#23:
[1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")


is this warning fine for you? .

Regards,

veeraiyan chidambaram.

On Tue, Sep 10, 2019 at 04:45:29AM +0000, Yoshihiro Shimoda wrote:
> Hi Veeraiyan,
> 
> Thank you for the patch!
> 
> > From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM
> <snip>
> > 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] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> > [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> 
> I'm afraid I should have realized but, according to checkpatch.pl,
> these formats cause ERROR like below. So, they should be fixed.
> 
> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")'
> #90:
> [1] f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> 
> ERROR: Please use git commit description style 'commit <12+ chars of sha1> ("<title line>")' - ie: 'commit 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")'
> #91:
> [2] 2f98382dcdfe ("usb: renesas_usbhs: Add Renesas USBHS Gadget")
> 
> Best regards,
> Yoshihiro Shimoda
> 

RE: [PATCH v4 2/3] usb: renesas_usbhs: enable DVSE interrupt

[Yoshihiro Shimoda]

Hello Veeraiyan-san,

> From: veeraiyan chidambaram, Sent: Tuesday, September 10, 2019 6:31 PM
> 
> Hello shimoda-san,
> 
> Thanks for point out checkpatch warning. After resolving checkpatch warning,
> below  checkpatch warning is seen.
> 
> WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> #23:
> [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")

I checked other commit log, and it seems adding a new line is better like below:

[1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common
    code")

JFYI (out-of-topic though), checkpatch.pl doesn't warn if a "Fixes:" tag with
more than 75 chars like following commit [2].

[2]
---
commit d950fd992ef89f39ff8908f389ed6cbd2fdc0513
Author: Niklas Söderlund <***>
Date:   Wed Feb 13 17:07:54 2019 -0500

    media: rcar-vin: Fix lockdep warning at stream on

    Changes to v4l2-fwnode in commit [1] triggered a lockdep warning in
    rcar-vin. The first attempt to solve this warning in the rcar-vin driver
    was incomplete and only pushed the warning to happen at stream on time
    instead of at probe time.

    This change reverts the incomplete fix and properly fixes the warning by
    removing the need to hold the rcar-vin specific group lock when calling
    v4l2_async_notifier_parse_fwnode_endpoints_by_port(). And instead takes
    it in the callback where it's really needed.

    [1] commit eae2aed1eab9bf08 ("media: v4l2-fwnode: Switch to
    v4l2_async_notifier_add_subdev")

    Fixes: 6458afc8c49148f0 ("media: rcar-vin: remove unneeded locking in async callbacks")
<snip>
---

Best regards,
Yoshihiro Shimoda

Re: [PATCH v4 2/3] usb: renesas_usbhs: enable DVSE interrupt

[veeraiyan chidambaram]

Hello shimoda-san,

Thanks a lot for those hints.
On Wed, Sep 11, 2019 at 02:45:52AM +0000, Yoshihiro Shimoda wrote:
> Hello Veeraiyan-san,
> 
> > From: veeraiyan chidambaram, Sent: Tuesday, September 10, 2019 6:31 PM
> > 
> > Hello shimoda-san,
> > 
> > Thanks for point out checkpatch warning. After resolving checkpatch warning,
> > below  checkpatch warning is seen.
> > 
> > WARNING: Possible unwrapped commit description (prefer a maximum 75 chars per line)
> > #23:
> > [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common code")
> 
> I checked other commit log, and it seems adding a new line is better like below:
> 
> [1] commit f1407d5c6624 ("usb: renesas_usbhs: Add Renesas USBHS common
>     code")

I have fixed and submitted v5 patch https://patchwork.kernel.org/patch/11141085/.

Best Regards,
Veeraiyan Chidambaram

[PATCH v4 3/3] usb: renesas_usbhs: add suspend event support in gadget mode

[Veeraiyan Chidambaram]

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

When R-Car Gen3 USB 2.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 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>
---
v4: No change
v3: https://patchwork.kernel.org/patch/11137701/ 
v2: https://patchwork.kernel.org/patch/11135115/
v1: https://patchwork.kernel.org/patch/11135115/

 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

RE: [PATCH v4 3/3] usb: renesas_usbhs: add suspend event support in gadget mode

[Yoshihiro Shimoda]

Hi Veeraiyan,

> From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM
<snip>
> 
> When R-Car Gen3 USB 2.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 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>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

And, I tested this patch on my environment [1] and works correctly. So,

Tested-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

[1]
 - Two R-Car Gen3 boards.
 - Connected a usb cable to each CN9.
 -- Use g_mass_storage.ko.

Best regards,
Yoshihiro Shimoda

RE: [PATCH v4 1/3] usb: renesas_usbhs: simplify usbhs_status_get_device_state()

[Yoshihiro Shimoda]

Hi Veeraiyan,

> From: Veeraiyan Chidambaram, Sent: Tuesday, September 10, 2019 12:46 AM
<snip>
> 
> 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>
> Signed-off-by: Veeraiyan Chidambaram <veeraiyan.chidambaram@in.bosch.com>

Thank you for the patch!

Reviewed-by: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>

Best regards,
Yoshihiro Shimoda