[Patch] Removing legacy method from arm csb337

[Vipul Nayyar]

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 ?

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 ?

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.

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>
> 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
PGP     : Public key available on request.

Diese Nachricht ist keine geschäftliche Mitteilung im Sinne des EHUG.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20130704/e13b7b2c/attachment-0001.html>