| Message ID | 1567771431-13235-1-git-send-email-external.veeraiyan.c@de.adit-jv.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/3] usb: renesas_usbhs: enable DVSE interrupt | expand |
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
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
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
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.
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.
Hi Felipe, 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> > > > > 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. I see this _superseded_ patch version (and apparently the whole series) present on your "next" branch, as well as on linux-next: https://git.kernel.org/pub/scm/linux/kernel/git/balbi/usb.git/commit/?h=next&id=8b20d00f0f08 https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=8b20d00f0f08 The most recent v5 version of this series has been recently pushed to (not yet visible in upstream): https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb.git/log/?h=usb-testing
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;