[Patch] Removing legacy method from arm csb337
Vipul Nayyar
nayyar_vipul at yahoo.com
Fri Jul 5 15:32:11 UTC 2013
Hello,
Submitting the patch again. Hope I've made all the corrections that I mentioned in my previous email. The network.c file in arm csb337 currently uses 4 space indent in many functions. For the current function at91rm9200_emac_init() , I've made all the statements use 2 space indents. If the main changes in the following patch regarding the generic framework are correct, I'll update the whole file to use 2 space indent later.
commit 9e50e8f22be111aff9f8725bcef6d99f2a6d36f5
Author: Vipul Nayyar <nayyar_vipul at yahoo.com>
Date: Fri Jul 5 20:53:33 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..f577e8e 100644
--- a/c/src/lib/libbsp/arm/csb337/network/network.c
+++ b/c/src/lib/libbsp/arm/csb337/network/network.c
@@ -22,6 +22,7 @@
#include <errno.h>
#include <rtems/error.h>
+#include <assert.h>
#include <sys/param.h>
#include <sys/mbuf.h>
@@ -83,19 +84,9 @@
#define START_TRANSMIT_EVENT RTEMS_EVENT_2
static void at91rm9200_emac_isr (rtems_irq_hdl_param unused);
-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
-};
+static void at91rm9200_emac_isr_on();
+static void at91rm9200_emac_isr_off();
+static int at91rm9200_emac_isr_is_on();
/* use the values defined in linkcmds for our use of SRAM */
@@ -356,51 +347,61 @@ int rtems_at91rm9200_emac_attach (
void at91rm9200_emac_init(void *arg)
{
- at91rm9200_emac_softc_t *sc = arg;
- struct ifnet *ifp = &sc->arpcom.ac_if;
+ 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()
+ * gets called multiple times
+ */
+ if (sc->txDaemonTid == 0) {
+ /* Set up EMAC hardware */
+ at91rm9200_emac_init_hw(sc);
+
+ /* Start driver tasks */
+ sc->rxDaemonTid = rtems_bsdnet_newproc("ENrx",
+ 4096,
+ at91rm9200_emac_rxDaemon,
+ sc);
+ sc->txDaemonTid = rtems_bsdnet_newproc("ENtx",
+ 4096,
+ at91rm9200_emac_txDaemon,
+ sc);
+ } /* if txDaemonTid */
+
+ /* set our priority in the AIC */
+ AIC_SMR_REG(AIC_SMR_EMAC) = AIC_SMR_PRIOR(EMAC_INT_PRIORITY);
+
+ /* install the interrupt handler */
+ status = rtems_interrupt_handler_install(
+ AT91RM9200_INT_EMAC,
+ "Network",
+ RTEMS_INTERRUPT_UNIQUE,
+ at91rm9200_emac_isr,
+ NULL
+ );
+ at91rm9200_emac_isr_on();
- /*
- *This is for stuff that only gets done once (at91rm9200_emac_init()
- * gets called multiple times
- */
- if (sc->txDaemonTid == 0) {
- /* Set up EMAC hardware */
- at91rm9200_emac_init_hw(sc);
-
- /* Start driver tasks */
- sc->rxDaemonTid = rtems_bsdnet_newproc("ENrx",
- 4096,
- at91rm9200_emac_rxDaemon,
- sc);
- sc->txDaemonTid = rtems_bsdnet_newproc("ENtx",
- 4096,
- at91rm9200_emac_txDaemon,
- sc);
- } /* if txDaemonTid */
-
- /* set our priority in the AIC */
- 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);
-
- /* EMAC doesn't support promiscuous, so ignore requests */
- if (ifp->if_flags & IFF_PROMISC) {
- printk ("Warning - AT91RM9200 Ethernet driver"
+ assert(status == RTEMS_SUCCESSFUL);
+
+ /* EMAC doesn't support promiscuous, so ignore requests */
+ if (ifp->if_flags & IFF_PROMISC) {
+ printk ("Warning - AT91RM9200 Ethernet driver"
" doesn't support Promiscuous Mode!\n");
- }
+ }
- /*
- * Tell the world that we're running.
- */
- ifp->if_flags |= IFF_RUNNING;
+ /*
+ * Tell the world that we're running.
+ */
+ ifp->if_flags |= IFF_RUNNING;
- /* Enable TX/RX and clear the statistics counters */
- EMAC_REG(EMAC_CTL) = (EMAC_CTL_TE | EMAC_CTL_RE | EMAC_CTL_CSR);
+ /* Enable TX/RX and clear the statistics counters */
+ EMAC_REG(EMAC_CTL) = (EMAC_CTL_TE | EMAC_CTL_RE | EMAC_CTL_CSR);
- /* clear any pending interrupts */
- EMAC_REG(EMAC_TSR) = 0xffffffff;
- EMAC_REG(EMAC_RSR) = 0xffffffff;
+ /* clear any pending interrupts */
+ EMAC_REG(EMAC_TSR) = 0xffffffff;
+ EMAC_REG(EMAC_RSR) = 0xffffffff;
} /* at91rm9200_emac_init() */
@@ -767,7 +768,7 @@ void at91rm9200_emac_stats (at91rm9200_emac_softc_t *sc)
/* Enables at91rm9200_emac interrupts. */
-static void at91rm9200_emac_isr_on(const rtems_irq_connect_data *unused)
+static void at91rm9200_emac_isr_on()
{
/* Enable various TX/RX interrupts */
EMAC_REG(EMAC_IER) = (EMAC_INT_RCOM | /* Receive complete */
@@ -780,7 +781,7 @@ static void at91rm9200_emac_isr_on(const rtems_irq_connect_data *unused)
}
/* Disables at91rm9200_emac interrupts */
-static void at91rm9200_emac_isr_off(const rtems_irq_connect_data *unused)
+static void at91rm9200_emac_isr_off()
{
/* disable all various TX/RX interrupts */
EMAC_REG(EMAC_IDR) = 0xffffffff;
@@ -791,7 +792,7 @@ static void at91rm9200_emac_isr_off(const rtems_irq_connect_data *unused)
* returns non-0 if so.
* If interrupt is not enabled, returns 0.
*/
-static int at91rm9200_emac_isr_is_on(const rtems_irq_connect_data *irq)
+static int at91rm9200_emac_isr_is_on()
{
return EMAC_REG(EMAC_IMR); /* any interrupts enabled? */
}
Regards
Vipul Nayyar
________________________________
From: Joel Sherrill <joel.sherrill at oarcorp.com>
To: Vipul Nayyar <nayyar_vipul at yahoo.com>
Cc: "sebastian.huber at embedded-brains.de" <sebastian.huber at embedded-brains.de>; "rtems-devel at rtems.org" <rtems-devel at rtems.org>
Sent: Friday, 5 July 2013 6:59 PM
Subject: Re: [Patch] Removing legacy method from arm csb337
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>
>> 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.
>
>
>
--
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/c5d7b07f/attachment.html>
More information about the devel
mailing list