linux-mips
[Top] [All Lists]

Re: [PATCH][MIPS][1/7] AR7: core support

To: Matteo Croce <technoboy85@gmail.com>
Subject: Re: [PATCH][MIPS][1/7] AR7: core support
From: Ralf Baechle <ralf@linux-mips.org>
Date: Wed, 12 Sep 2007 12:10:41 +0100
Cc: Atsushi Nemoto <anemo@mba.ocn.ne.jp>, linux-mips@linux-mips.org, florian@openwrt.org, nbd@openwrt.org, ejka@imfi.kspu.ru, nico@openwrt.org, openwrt-devel@lists.openwrt.org, akpm@linux-foundation.org
In-reply-to: <200709120043.43452.technoboy85@gmail.com>
Original-recipient: rfc822;linux-mips@linux-mips.org
References: <200709080143.12345.technoboy85@gmail.com> <200709080218.50236.technoboy85@gmail.com> <20070909.024020.61508994.anemo@mba.ocn.ne.jp> <200709120043.43452.technoboy85@gmail.com>
Sender: linux-mips-bounce@linux-mips.org
User-agent: Mutt/1.5.14 (2007-02-12)
On Wed, Sep 12, 2007 at 12:43:42AM +0200, Matteo Croce wrote:

> > > --- a/arch/mips/kernel/traps.c
> > > +++ b/arch/mips/kernel/traps.c
> > > @@ -1075,9 +1075,23 @@ void *set_except_vector(int n, void *addr)
> > >  
> > >   exception_handlers[n] = handler;
> > >   if (n == 0 && cpu_has_divec) {
> > > +#ifdef CONFIG_AR7
> > > +         /* lui k0, 0x0000 */
> > > +         *(volatile u32 *)(CAC_BASE+0x200) =

s/CAC_BASE/ebase/

> > > +                         0x3c1a0000 | (handler >> 16);
> > > +         /* ori k0, 0x0000 */
> > > +         *(volatile u32 *)(CAC_BASE+0x204) =
> > > +                         0x375a0000 | (handler & 0xffff);
> > > +         /* jr k0 */
> > > +         *(volatile u32 *)(CAC_BASE+0x208) = 0x03400008;
> > > +         /* nop */
> > > +         *(volatile u32 *)(CAC_BASE+0x20C) = 0x00000000;
> > > +         flush_icache_range(CAC_BASE+0x200, CAC_BASE+0x210);

All these volatile keywords are unnecessary btw.

You may want to read Documentation/volatile-considered-harmful.txt on
why volatile is almost always a bad idea.

> > > +#else
> > >           *(volatile u32 *)(ebase + 0x200) = 0x08000000 |
> > >                                            (0x03ffffff & (handler >> 2));

Just like this one, so I will remove it now.

> > >           flush_icache_range(ebase + 0x200, ebase + 0x204);
> > > +#endif
> > >   }
> > >   return (void *)old_handler;
> > >  }
> > 
> > Runtime checking, something like this would be better than ifdef:
> > 
> >     if ((handler ^ (ebase + 4)) & 0xfc000000)
> >             /* use jr */
> >             ...
> >     } else {
> >             /* use j */
> >             ...
> >     }
> This will not make the code bigger?

It will by a miniscule amount.  Which hardly matters because the function
is (whops, should be ...) __init code anyway.

> What's wrong with #ifdef?

#ifdef makes for harder to read code (To paraphrase Linus - the kernel is
write once and read 10 million times) , is less flexible and has a tendence
to hide bugs in the deactivated part.  So generally avoid.

And actually in this specific case it also should be a runtime decission
simply because in the not so distant future the will be hardware which
will simply need that.

  Ralf

<Prev in Thread] Current Thread [Next in Thread>