From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Brezillon Subject: Re: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs Date: Mon, 15 Dec 2014 19:12:58 +0100 Message-ID: <20141215191258.146f5c9e@bbrezillon> References: <1418648588-17872-1-git-send-email-boris.brezillon@free-electrons.com> <1418648588-17872-5-git-send-email-boris.brezillon@free-electrons.com> <548EE2DA.6050408@cogentembedded.com> <063D6719AE5E284EB5DD2968C1650D6D1CA0CE20@AcuExch.aculab.com> <20141215180239.05037dcd@bbrezillon> <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF@AcuExch.aculab.com> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CA0D0EF-VkEWCZq2GCInGFn1LkZF6NBPR1lH4CV8@public.gmane.org> Sender: linux-usb-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: David Laight Cc: 'Sergei Shtylyov' , Felipe Balbi , Greg Kroah-Hartman , "linux-usb-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , Nicolas Ferre , Jean-Christophe Plagniol-Villard , Alexandre Belloni , Andrew Victor , Rob Herring , Pawel Moll , Mark Rutland , Ian Campbell , Kumar Gala , "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" List-Id: devicetree@vger.kernel.org On Mon, 15 Dec 2014 17:22:04 +0000 David Laight wrote: > From: Boris Brezillon > > Hi David, > > > > On Mon, 15 Dec 2014 13:34:56 +0000 > > David Laight 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 > > > > > Signed-off-by: Boris Brezillon > > > > > --- > > > > > 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 majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org More majordomo info at http://vger.kernel.org/majordomo-info.html