Re: [PATCH 10/10] staging: dwc2: properly separate common and host interrupt enabling
Hi Paul, --- a/drivers/staging/dwc2/core.c +++ b/drivers/staging/dwc2/core.c ... + /* Enable common interrupts without disturbing host mode interrupts */ + intmsk = readl(hsotg-regs + GINTMSK); + intmsk |= GINTSTS_DISCONNINT | GINTSTS_MODEMIS | GINTSTS_OTGINT | + GINTSTS_CONIDSTSCHNG | GINTSTS_WKUPINT | GINTSTS_USBSUSP | GINTSTS_SESSREQINT; This should use the interrupt list that you just added in patch 8/10, right? Well, the list is not necessarily the same. For the common interrupts, the GINTSTS_RESTOREDONE interrupt is in GINTMSK_COMMON, but it is never enabled. There is only some dummy handling for this interrupt, so I'll submit a separate patch to remove that dummy handling (just like for I2CINT). This should make GINTMSK_COMMON identical to the above list. However, for host-mode interrupts, there are a number of interrupts that are not enabled in dwc2_enable_host_interrupts, but later on when needed. They still need to be in GINTMSK_HOST, since they must be handled by dwc2_handle_hcd_intr and disabled by dwc2_disable_host_interrupts (both of which use GINTMSK_HOST). Given that dwc2_enable_host_interrupts cannot simply use GINTMSK_HOST, it makes sense to not use GINTMSK_COMMON in dwc2_enable_common_interrupts either. However, I did ponder about using these masks in the enable functions, since it helps robustness if you cannot accidentally enable an interrupt that is never disabled. How about the following: - in dwc2_enable_common_interrupts we just enable GINTMSK_COMMON - in dwc2_enable_host_interrupts we enable GINTMSK_HOST ~(GINTSTS_SOF|GINTSTS_PTXFEMP|GINTSTS_NPTXFEMP) (e.g., writing a list of interrupts to enable later, instead of only specifying the interrupts to enable now). The status of GINTSTS_RXFLVL would need to be handled separately, as it is now, of course. How does that look? Gr. Matthijs -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 10/10] staging: dwc2: properly separate common and host interrupt enabling
From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Tuesday, April 16, 2013 9:47 AM --- a/drivers/staging/dwc2/core.c +++ b/drivers/staging/dwc2/core.c ... + /* Enable common interrupts without disturbing host mode interrupts */ + intmsk = readl(hsotg-regs + GINTMSK); + intmsk |= GINTSTS_DISCONNINT | GINTSTS_MODEMIS | GINTSTS_OTGINT | + GINTSTS_CONIDSTSCHNG | GINTSTS_WKUPINT | GINTSTS_USBSUSP | GINTSTS_SESSREQINT; This should use the interrupt list that you just added in patch 8/10, right? Well, the list is not necessarily the same. For the common interrupts, the GINTSTS_RESTOREDONE interrupt is in GINTMSK_COMMON, but it is never enabled. There is only some dummy handling for this interrupt, so I'll submit a separate patch to remove that dummy handling (just like for I2CINT). This should make GINTMSK_COMMON identical to the above list. However, for host-mode interrupts, there are a number of interrupts that are not enabled in dwc2_enable_host_interrupts, but later on when needed. They still need to be in GINTMSK_HOST, since they must be handled by dwc2_handle_hcd_intr and disabled by dwc2_disable_host_interrupts (both of which use GINTMSK_HOST). Given that dwc2_enable_host_interrupts cannot simply use GINTMSK_HOST, it makes sense to not use GINTMSK_COMMON in dwc2_enable_common_interrupts either. I don't really see why. However, I did ponder about using these masks in the enable functions, since it helps robustness if you cannot accidentally enable an interrupt that is never disabled. How about the following: - in dwc2_enable_common_interrupts we just enable GINTMSK_COMMON - in dwc2_enable_host_interrupts we enable GINTMSK_HOST ~(GINTSTS_SOF|GINTSTS_PTXFEMP|GINTSTS_NPTXFEMP) (e.g., writing a list of interrupts to enable later, instead of only specifying the interrupts to enable now). The status of GINTSTS_RXFLVL would need to be handled separately, as it is now, of course. How does that look? Yeah, I think that's better. And maybe add a comment saying these three interrupts will be enabled later if needed or something like that. -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 10/10] staging: dwc2: properly separate common and host interrupt enabling
From: Matthijs Kooijman [mailto:matth...@stdin.nl] Sent: Monday, April 15, 2013 7:14 AM Before, enabling common interrupts would implicitly disable all other interrupts and enabling host interrupts would disable all interrupts and then enable both common and host interrupts. Now, these two are properly separated: each enable function only enables its own interrupts and leaves all others alone. This makes the functions do what you'd expect them to. In practice, both of these functions are only called together during initialization and cleanup, so there shouldn't be any behaviour change (though some timing changes could occur). Signed-off-by: Matthijs Kooijman matth...@stdin.nl --- drivers/staging/dwc2/core.c | 22 +- 1 file changed, 9 insertions(+), 13 deletions(-) diff --git a/drivers/staging/dwc2/core.c b/drivers/staging/dwc2/core.c index 09b6d19..6b75821 100644 --- a/drivers/staging/dwc2/core.c +++ b/drivers/staging/dwc2/core.c @@ -69,13 +69,13 @@ static void dwc2_enable_common_interrupts(struct dwc2_hsotg *hsotg) /* Clear any pending OTG Interrupts */ writel(0x, hsotg-regs + GOTGINT); - /* Clear any pending interrupts */ - writel(0x, hsotg-regs + GINTSTS); + /* Clear any pending common interrupts */ + writel(GINTMSK_COMMON, hsotg-regs + GINTSTS); - /* Enable the interrupts in the GINTMSK */ - intmsk = GINTSTS_DISCONNINT | GINTSTS_MODEMIS | GINTSTS_OTGINT; - - intmsk |= GINTSTS_CONIDSTSCHNG | GINTSTS_WKUPINT | GINTSTS_USBSUSP | + /* Enable common interrupts without disturbing host mode interrupts */ + intmsk = readl(hsotg-regs + GINTMSK); + intmsk |= GINTSTS_DISCONNINT | GINTSTS_MODEMIS | GINTSTS_OTGINT | + GINTSTS_CONIDSTSCHNG | GINTSTS_WKUPINT | GINTSTS_USBSUSP | GINTSTS_SESSREQINT; This should use the interrupt list that you just added in patch 8/10, right? -- Paul -- To unsubscribe from this list: send the line unsubscribe linux-usb in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
