RE: [PATCH 4/4] usb: atmel_usba_udc: mask status with enabled irqs

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




From: Boris Brezillon
> Hi David,
> 
> On Mon, 15 Dec 2014 13:34:56 +0000
> David Laight <David.Laight@xxxxxxxxxx> 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.vilchez@xxxxxxxxx>
> > > > Signed-off-by: Boris Brezillon <boris.brezillon@xxxxxxxxxxxxxxxxxx>
> > > > ---
> > > >   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.

> 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.

	David



--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html




[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]
  Powered by Linux