[Patch] Removing legacy method from arm csb337
Joel Sherrill
joel.sherrill at OARcorp.com
Fri Jul 5 13:29:35 UTC 2013
On 7/4/2013 10:24 AM, Vipul Nayyar wrote:
> Hello,
>
> Before submitting a patch again, I wanted to clarify some things which
> I understood & places where I need a little more guidance, so that I
> do things the right way.
>
> 1) Before submitting any patch I'll definitely compile it first. So do
> I need to bootstrap, run the configure using specific BSP name &
> target in cmd options at top of RTEMS tree and do make every time, I
> change something ? Or is there a simpler method ?
>
Sebastian pointed out that you can build all ARM BSPs in one pass. Or
specify a set of
BSPs with --enable-rtemsbsp="XXX YYY".
I don't think youneed to run bootstrap after your tree is cloned and
bootstrapped once.
It only needs to be run if you addfiles, delete files, or something else
that impacts the
configure.ac or Makefile.am files. But you can run bootstrap from any
directory with
a configure.ac and it will handle things recursively from there. If you
are working on a single
BSP, it saves a lot of time.
You also shouldn't need to build all the tests. I would recommend
--enable-tests=samples
and --disable-posix on your build. I believe you still want to have
networking enabled because
there are network drivers.
> 2) According to the legacy method BSP_install_rtems_irq_handler, it
> uses the 'on' ( at91rm9200_emac_isr_on() in case of csb337) function
> only after calling rtems_interrupt_handler_install. And these
> functions require a pointer to a rtems_irq_connect_data type.So I
> should probably remove that parameter from the function definition right ?
>
Sebastian.
> 3) rtems_interrupt_handler_install() does not accept pointer to the
> rtems_irq_connect_data type , so it definitely does not
> use at91rm9200_emac_isr_off() & at91rm9200_emac_isr_is_on() functions.
> I think we should remove these 2 functions, because they're not being
> used in the any csb337 file.
>
Sebastian..
> 4) If you say so, I'll use assert() for time being. Plus, do I need to
> include <assert.h> ? If yes, then in each file where I used assert()
> or it can be included somewhere central ?
>
> Hoping to hear from you
>
> Regards
> Vipul Nayyar
>
>
> ------------------------------------------------------------------------
> *From:* Sebastian Huber <sebastian.huber at embedded-brains.de>
> *To:* Vipul Nayyar <nayyar_vipul at yahoo.com>
> *Cc:* "rtems-devel at rtems.org" <rtems-devel at rtems.org>; Joel Sherrill
> <joel.sherrill at oarcorp.com>
> *Sent:* Wednesday, 3 July 2013 12:37 PM
> *Subject:* Re: [Patch] Removing legacy method from arm csb337
>
> On 07/02/2013 01:26 PM, Vipul Nayyar wrote:
> > Hello,
> >
> > Thanks for pointing out the mistakes in the previous patches.
> Updated the
> > csb337 patch, and did away with the use of rtems_irq_connect_data
> structure in
> > the file. Is this what you were suggesting ? Hoping I've followed the
> > indentation rules.
> >
> > commit d24fbcb472b0164ed4e6b51233155df7f6dd57b4
> > Author: Vipul Nayyar <nayyar_vipul at yahoo.com
> <mailto:nayyar_vipul at yahoo.com>>
> > Date: Tue Jul 2 16:52:29 2013 +0530
> >
> > Updated Legacy code in arm csb337
> >
> > diff --git a/c/src/lib/libbsp/arm/csb337/network/network.c
> > b/c/src/lib/libbsp/arm/csb337/network/network.c
> > index e31bcc0..1c1dd0e 100644
> > --- a/c/src/lib/libbsp/arm/csb337/network/network.c
> > +++ b/c/src/lib/libbsp/arm/csb337/network/network.c
> > @@ -87,17 +87,6 @@ static void at91rm9200_emac_isr_on(const
> > rtems_irq_connect_data *unused);
> > static void at91rm9200_emac_isr_off(const rtems_irq_connect_data
> *unused);
> > static int at91rm9200_emac_isr_is_on(const rtems_irq_connect_data
> *irq);
> > -/* Replace the first value with the clock's interrupt name. */
> > -rtems_irq_connect_data at91rm9200_emac_isr_data = {
> > - AT91RM9200_INT_EMAC,
> > - at91rm9200_emac_isr,
> > - NULL,
> > - at91rm9200_emac_isr_on,
> > - at91rm9200_emac_isr_off,
> > - at91rm9200_emac_isr_is_on
>
> Where are the at91rm9200_emac_isr_on(), at91rm9200_emac_isr_off() and
> at91rm9200_emac_isr_is_on() called now? Did you compile the code
> after the
> changes? You should get warnings now about unused static functions.
>
> Are they really all superfluous?
>
> > -};
> > -
> > -
> > /* use the values defined in linkcmds for our use of SRAM */
> > extern void * at91rm9200_emac_rxbuf_hdrs;
> > extern void * at91rm9200_emac_txbuf;
> > @@ -358,6 +347,7 @@ void at91rm9200_emac_init(void *arg)
> > {
> > at91rm9200_emac_softc_t *sc = arg;
> > struct ifnet *ifp = &sc->arpcom.ac_if;
> > + rtems_status_code status = RTEMS_SUCCESSFUL;
> > /*
> > *This is for stuff that only gets done once
> (at91rm9200_emac_init()
> > @@ -382,7 +372,19 @@ void at91rm9200_emac_init(void *arg)
> > AIC_SMR_REG(AIC_SMR_EMAC) = AIC_SMR_PRIOR(EMAC_INT_PRIORITY);
> > /* install the interrupt handler */
> > - BSP_install_rtems_irq_handler(&at91rm9200_emac_isr_data);
> > + status = rtems_interrupt_handler_install(
> > + AT91RM9200_INT_EMAC,
> > + "Network",
> > + RTEMS_INTERRUPT_UNIQUE,
> > + at91rm9200_emac_isr,
> > + NULL
> > + );
> > + if (status != RTEMS_SUCCESSFUL) {
> > + rtems_fatal(
> > + RTEMS_FATAL_SOURCE_BSP_SPECIFIC,
> > + BSP_GENERIC_FATAL_INTERRUPT_INITIALIZATION
> > + );
> > + }
> [...]
>
> As I wrote before, the indentation level is two characters.
>
> The purpose of the fatal errors is
>
> 1. terminate the system, and
> 2. allow the exact identification of the error source in the code.
>
> From 2. follows that a (source, code) pair must be used a most once in
> the
> RTEMS source code.
>
> In the documentation we have:
>
> http://www.rtems.org/onlinedocs/doxygen/cpukit/html/group__ScoreIntErr.html#ga878b4de77df7d0b83d19609d4de42c26
>
> [...]
> RTEMS_FATAL_SOURCE_BSP_GENERIC
>
> Fatal source for generic BSP errors.
>
> The fatal codes are defined in <bsp/generic-fatal.h>. Examples are
> interrupt
> and exception initialization.
>
> See also:
> bsp_generic_fatal_code and bsp_generic_fatal().
>
> RTEMS_FATAL_SOURCE_BSP_SPECIFIC
>
> Fatal source for BSP specific errors.
>
> The fatal code is BSP specific.
> [...]
>
> The BSP_GENERIC_FATAL_INTERRUPT_INITIALIZATION is generic BSP error
> code, so
> don't use it with RTEMS_FATAL_SOURCE_BSP_SPECIFIC.
>
> I would use simply assert(). If there is time, we can change all the
> assert()
> into BSP specific fatal errors later.
>
> --
> Sebastian Huber, embedded brains GmbH
>
> Address : Dornierstr. 4, D-82178 Puchheim, Germany
> Phone : +49 89 189 47 41-16
> Fax : +49 89 189 47 41-09
> E-Mail : sebastian.huber at embedded-brains.de
> <mailto:sebastian.huber at embedded-brains.de>
> PGP : Public key available on request.
>
> Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
>
>
--
Joel Sherrill, Ph.D. Director of Research & Development
joel.sherrill at OARcorp.com On-Line Applications Research
Ask me about RTEMS: a free RTOS Huntsville AL 35805
Support Available (256) 722-9985
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20130705/fd4cb498/attachment.html>
More information about the devel
mailing list