On Mon, 15 Dec 2014 17:22:04 +0000 David Laight <david.lai...@aculab.com> wrote:
> From: Boris Brezillon > > Hi David, > > > > On Mon, 15 Dec 2014 13:34:56 +0000 > > David Laight <david.lai...@aculab.com> wrote: > > > > > From: Sergei Shtylyov > > > > Hello. > > > > > > > > On 12/15/2014 4:03 PM, Boris Brezillon wrote: > > > > > > > > > Avoid interpreting useless status flags when we're not waiting for > > > > > such > > > > > events by masking the status variable with the interrupt enabled > > > > > register > > > > > value. > > > > > > > > > Reported-by: Patrice VILCHEZ <patrice.vilc...@atmel.com> > > > > > Signed-off-by: Boris Brezillon <boris.brezil...@free-electrons.com> > > > > > --- > > > > > drivers/usb/gadget/udc/atmel_usba_udc.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > diff --git a/drivers/usb/gadget/udc/atmel_usba_udc.c > > > > > b/drivers/usb/gadget/udc/atmel_usba_udc.c > > > > > index 55c8dde..bc3a532 100644 > > > > > --- a/drivers/usb/gadget/udc/atmel_usba_udc.c > > > > > +++ b/drivers/usb/gadget/udc/atmel_usba_udc.c > > > > > @@ -1612,12 +1612,14 @@ static irqreturn_t usba_udc_irq(int irq, void > > > > > *devid) > > > > > > > > > > spin_lock(&udc->lock); > > > > > > > > > > - status = usba_readl(udc, INT_STA); > > > > > + status = usba_readl(udc, INT_STA) & usba_readl(udc, INT_ENB); > ... > > > > Looks like t make sense to read the INT_ENB register into a separate > > > > variable, to save on extra reads? > > > > > > > > > Better still remember the written value in one of the structures so > > > that it doesn't have to be read at all. > > > > Hmm, I'm getting back to this suggestion. > > While I definitely understand why I should use a local variable to > > store INT_ENB value in usba_udc_irq, I don't see the point of mirroring > > INT_EN status in an udc struct field (after all, INT_EN will always > > contain the value we previously set). > > This is exactly why it makes sense to mirror it locally. Absolutely. > > > Is this a performance concern ? > > Absolutely, you really don't want to know how many cpu cycles it is > likely to take to do a read from an io device. > At best it is a uncached read of a fast on-chip peripheral. > If you are reading from a PCIe device then you are looking at hundreds > (if not thousands) of cpu clock cycles. I know there is a perf penalty when accessing IO memory regions (in this case an uncached memory access) compared to standard memory accesses (in other words a cached accesses), just don't know the exact numbers. My point was, is the performance improvement worth the addition of this new field and the code modification (addition of a wrapper function to modify the interrupt register) ? I take your answer as a yes ;-). Thanks, Boris -- Boris Brezillon, Free Electrons Embedded Linux and Kernel engineering http://free-electrons.com -- 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
