[v2,1/3] usb: renesas_usbhs: enable DVSE interrupt
diff mbox series

Message ID 1567771431-13235-1-git-send-email-external.veeraiyan.c@de.adit-jv.com
State Superseded
Delegated to: Geert Uytterhoeven
Headers show
Series
  • [v2,1/3] usb: renesas_usbhs: enable DVSE interrupt
Related show

Commit Message

veeraiyan chidambaram Sept. 6, 2019, 12:03 p.m. UTC
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(-)

Comments

Yoshihiro Shimoda Sept. 9, 2019, 7:02 a.m. UTC | #1
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
Greg KH Sept. 9, 2019, 9:55 a.m. UTC | #2
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
veeraiyan chidambaram Sept. 9, 2019, 11:04 a.m. UTC | #3
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
Eugeniu Rosca Sept. 9, 2019, 1:05 p.m. UTC | #4
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.
veeraiyan chidambaram Sept. 9, 2019, 4:01 p.m. UTC | #5
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.

Patch
diff mbox series

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;