[Patch] Removing legacy method from arm csb336

Vipul Nayyar nayyar_vipul at yahoo.com
Sun Jul 7 11:33:34 UTC 2013


Hello,

Worked again on csb336 patch. Following points I've kept in mind while doing this :

1) Update legacy install and remove methods to the new generic ones
2)Removed all uses of tems_irq_connect_data type
3) Inserted protoypes for functions which didn't had one
4) Made functions static that are only used in that file

Hoping to get reviews, so that I can move on with other BSPs.

commit e22d51a1f9d2efeba77e97ff0c81a9a5800af20c
Author: Vipul Nayyar <nayyar_vipul at yahoo.com>
Date:   Sun Jul 7 16:51:07 2013 +0530

    Updated Legacy code in arm csb336

diff --git a/c/src/lib/libbsp/arm/csb336/console/uart.c b/c/src/lib/libbsp/arm/csb336/console/uart.c
index 4dc409b..96126fa 100644
--- a/c/src/lib/libbsp/arm/csb336/console/uart.c
+++ b/c/src/lib/libbsp/arm/csb336/console/uart.c
@@ -13,6 +13,7 @@
 #include <libchip/sersupp.h>
 #include <rtems/error.h>
 #include <rtems/bspIo.h>
+#include <assert.h>
 #include <termios.h>
 #include <rtems/irq.h>
 #include <bsp/irq.h>
@@ -34,13 +35,17 @@ static int imx_uart_set_attrs(int, const struct termios *);
 static void imx_uart_init(int minor);
 static void imx_uart_set_baud(int, int);
 static ssize_t imx_uart_poll_write(int, const char *, size_t);
+static int imx_uart_poll_read_char(int minor);
+static void imx_uart_poll_write_char(int minor, char c);
+static void _BSP_output_char(char c);
+static int _BSP_poll_char(void);
+
 
 #if defined(USE_INTERRUPTS)
 static void imx_uart_tx_isr(rtems_irq_hdl_param);
 static void imx_uart_rx_isr(rtems_irq_hdl_param);
-static void imx_uart_isr_on(const rtems_irq_connect_data *irq);
-static void imx_uart_isr_off(const rtems_irq_connect_data *irq);
-static int imx_uart_isr_is_on(const rtems_irq_connect_data *irq);
+static void imx_uart_isr_on(const rtems_irq_number);
+static void imx_uart_isr_off(const rtems_irq_number);
 static ssize_t imx_uart_intr_write(int, const char *, size_t);
 #endif
 
@@ -71,11 +76,6 @@ rtems_termios_callbacks imx_uart_cbacks = {
 };
 #endif
 
-#if defined(USE_INTERRUPTS)
-static rtems_irq_connect_data imx_uart_tx_isr_data[NUM_DEVS];
-static rtems_irq_connect_data imx_uart_rx_isr_data[NUM_DEVS];
-#endif
-
 typedef struct {
     int minor;
     mc9328mxl_uart_regs_t * regs;
@@ -184,17 +184,9 @@ static void imx_uart_init(int minor)
     imx_uart_data[minor].idx   = 0;
 
     if (minor == 0) {
-#if defined(USE_INTERRUPTS)
-        imx_uart_tx_isr_data[minor].name = BSP_INT_UART1_TX;
-        imx_uart_rx_isr_data[minor].name = BSP_INT_UART1_RX;
-#endif
         imx_uart_data[minor].regs =
             (mc9328mxl_uart_regs_t *) MC9328MXL_UART1_BASE;
     } else if (minor == 1) {
-#if defined(USE_INTERRUPTS)
-        imx_uart_tx_isr_data[minor].name = BSP_INT_UART2_TX;
-        imx_uart_rx_isr_data[minor].name = BSP_INT_UART2_RX;
-#endif
         imx_uart_data[minor].regs =
             (mc9328mxl_uart_regs_t *) MC9328MXL_UART2_BASE;
     } else {
@@ -202,20 +194,6 @@ static void imx_uart_init(int minor)
                     __FUNCTION__, __LINE__, minor);
     }
 
-#if defined(USE_INTERRUPTS)
-    imx_uart_tx_isr_data[minor].hdl    = imx_uart_tx_isr;
-    imx_uart_tx_isr_data[minor].handle = &imx_uart_data[minor];
-    imx_uart_tx_isr_data[minor].on     = imx_uart_isr_on;
-    imx_uart_tx_isr_data[minor].off    = imx_uart_isr_off;
-    imx_uart_tx_isr_data[minor].isOn   = imx_uart_isr_is_on;
-
-    imx_uart_rx_isr_data[minor].hdl    = imx_uart_rx_isr;
-    imx_uart_rx_isr_data[minor].handle  = &imx_uart_data[minor];
-    imx_uart_rx_isr_data[minor].on     = imx_uart_isr_on;
-    imx_uart_rx_isr_data[minor].off    = imx_uart_isr_off;
-    imx_uart_rx_isr_data[minor].isOn   = imx_uart_isr_is_on;
-#endif
-
     imx_uart_data[minor].regs->cr1 = (
         MC9328MXL_UART_CR1_UARTCLKEN |
         MC9328MXL_UART_CR1_UARTEN);
@@ -243,13 +221,43 @@ static void imx_uart_init(int minor)
 static int imx_uart_first_open(int major, int minor, void *arg)
 {
     rtems_libio_open_close_args_t *args = arg;
+    rtems_status_code status = RTEMS_SUCCESSFUL;
+    rtems_irq_number name_uart_tx, name_uart_rx;
 
     imx_uart_data[minor].tty   = args->iop->data1;
 
-#if defined(USE_INTERRUPTS)
-    BSP_install_rtems_irq_handler(&imx_uart_tx_isr_data[minor]);
-    BSP_install_rtems_irq_handler(&imx_uart_rx_isr_data[minor]);
+    if (minor == 0) {
+        name_uart_tx = BSP_INT_UART1_TX;
+        name_uart_rx = BSP_INT_UART1_RX;
+    } else if (minor == 1) {
+        name_uart_tx = BSP_INT_UART2_TX;
+        name_uart_rx = BSP_INT_UART2_RX;
+    } else {
+        rtems_panic("%s:%d Unknown UART minor number %d\n",
+                    __FUNCTION__, __LINE__, minor);
+    }
 
+#if defined(USE_INTERRUPTS)
+    status = rtems_interrupt_handler_install(
+        name_uart_tx,
+        "Network",
+        RTEMS_INTERRUPT_UNIQUE,
+        imx_uart_tx_isr,
+        &imx_uart_data[minor]
+    );
+    assert(status == RTEMS_SUCCESSFUL);
+    imx_uart_isr_on(name_uart_tx);
+
+    status = rtems_interrupt_handler_install(
+        name_uart_rx,
+        "Network",
+        RTEMS_INTERRUPT_UNIQUE,
+        imx_uart_rx_isr,
+        &imx_uart_data[minor]
+    );
+    assert(status == RTEMS_SUCCESSFUL);
+    imx_uart_isr_on(name_uart_rx);
+ 
     imx_uart_data[minor].regs->cr1 |= MC9328MXL_UART_CR1_RRDYEN;
 #endif
 
@@ -258,9 +266,35 @@ static int imx_uart_first_open(int major, int minor, void *arg)
 
 static int imx_uart_last_close(int major, int minor, void *arg)
 {
+    rtems_status_code status = RTEMS_SUCCESSFUL;
+    rtems_irq_number name_uart_tx, name_uart_rx;
+
+    if (minor == 0) {
+        name_uart_tx = BSP_INT_UART1_TX;
+        name_uart_rx = BSP_INT_UART1_RX;
+    } else if (minor == 1) {
+        name_uart_tx = BSP_INT_UART2_TX;
+        name_uart_rx = BSP_INT_UART2_RX;
+    } else {
+        rtems_panic("%s:%d Unknown UART minor number %d\n",
+                    __FUNCTION__, __LINE__, minor);
+    }
 #if defined(USE_INTERRUPTS)
-    BSP_remove_rtems_irq_handler(&imx_uart_tx_isr_data[minor]);
-    BSP_remove_rtems_irq_handler(&imx_uart_rx_isr_data[minor]);
+    imx_uart_isr_off(name_uart_tx);
+    status = rtems_interrupt_handler_remove(
+        name_uart_tx,
+        imx_uart_tx_isr,
+        &imx_uart_data[minor]
+    );
+    assert(status == RTEMS_SUCCESSFUL);
+
+    imx_uart_isr_off(name_uart_rx);
+    status = rtems_interrupt_handler_remove(
+        name_uart_rx,
+        imx_uart_rx_isr,
+        &imx_uart_data[minor]
+    );
+    assert(status == RTEMS_SUCCESSFUL);
 #endif
 
     return 0;
@@ -317,22 +351,13 @@ static int imx_uart_set_attrs(int minor, const struct termios *t)
 }
 
 #if defined(USE_INTERRUPTS)
-static void imx_uart_isr_on(const rtems_irq_connect_data *irq)
+static void imx_uart_isr_on(const rtems_irq_number name)
 {
-    MC9328MXL_AITC_INTENNUM = irq->name;
+    MC9328MXL_AITC_INTENNUM = name;
 }
-static void imx_uart_isr_off(const rtems_irq_connect_data *irq)
+static void imx_uart_isr_off(const rtems_irq_number name)
 {
-    MC9328MXL_AITC_INTDISNUM = irq->name;
-}
-static int imx_uart_isr_is_on(const rtems_irq_connect_data *irq)
-{
-    int irq_num = (int)irq->name;
-    if (irq_num < 32) {
-        return MC9328MXL_AITC_INTENABLEL & (1 << irq_num);
-    } else {
-        return MC9328MXL_AITC_INTENABLEH & (1 << (irq_num - 32));
-    }
+    MC9328MXL_AITC_INTDISNUM = name;
 }
 
 static void imx_uart_rx_isr(rtems_irq_hdl_param param)
@@ -417,7 +442,7 @@ static void imx_uart_set_baud(int minor, int baud)
 /*
  * Polled, non-blocking read from UART
  */
-int imx_uart_poll_read_char(int minor)
+static int imx_uart_poll_read_char(int minor)
 {
     return imx_uart_poll_read(minor);
 }
@@ -425,7 +450,7 @@ int imx_uart_poll_read_char(int minor)
 /*
  * Polled, blocking write from UART
  */
-void  imx_uart_poll_write_char(int minor, char c)
+static void  imx_uart_poll_write_char(int minor, char c)
 {
     imx_uart_poll_write(minor, &c, 1);
 }
@@ -433,7 +458,7 @@ void  imx_uart_poll_write_char(int minor, char c)
 /*
  * Functions for printk() and friends.
  */
-void _BSP_output_char(char c)
+static void _BSP_output_char(char c)
 {
     poll_write(c);
     if (c == '\n') {
@@ -443,7 +468,7 @@ void _BSP_output_char(char c)
 
 BSP_output_char_function_type BSP_output_char = _BSP_output_char;
 
-int _BSP_poll_char(void)
+static int _BSP_poll_char(void)
 {
     return poll_read();
 }
diff --git a/c/src/lib/libbsp/arm/csb336/network/network.c b/c/src/lib/libbsp/arm/csb336/network/network.c
index e513595..1367a8f 100644
--- a/c/src/lib/libbsp/arm/csb336/network/network.c
+++ b/c/src/lib/libbsp/arm/csb336/network/network.c
@@ -19,6 +19,7 @@
 #include <errno.h>
 #include <rtems/error.h>
 #include <rtems/bspIo.h>
+#include <assert.h>
 
 #include <sys/param.h>
 #include <sys/mbuf.h>
@@ -39,19 +40,8 @@
 #define START_TRANSMIT_EVENT    RTEMS_EVENT_2
 
 static void enet_isr(rtems_irq_hdl_param);
-static void enet_isr_on(const rtems_irq_connect_data *unused);
-static void enet_isr_off(const rtems_irq_connect_data *unused);
-static int enet_isr_is_on(const rtems_irq_connect_data *irq);
-
-/* Replace the first value with the clock's interrupt name. */
-rtems_irq_connect_data mc9328mxl_enet_isr_data = {
-    .name    = BSP_INT_GPIO_PORTA,
-    .hdl     = enet_isr,
-    .handle  = (void *)BSP_INT_GPIO_PORTA,
-    .on      = enet_isr_on,
-    .off     = enet_isr_off,
-    .isOn    = enet_isr_is_on,
-};
+static void enet_isr_on(void);
+
 typedef struct {
   unsigned long rx_packets;        /* total packets received         */
   unsigned long tx_packets;        /* total packets transmitted      */
@@ -238,6 +228,7 @@ void  mc9328mxl_enet_init_hw(mc9328mxl_enet_softc_t *sc)
 {
     uint16_t stat;
     uint16_t my = 0;
+    rtems_status_code status = RTEMS_SUCCESSFUL;
 
     lan91c11x_write_reg(LAN91C11X_RCR, LAN91C11X_RCR_RST);
     lan91c11x_write_reg(LAN91C11X_RCR, 0);
@@ -334,7 +325,15 @@ void  mc9328mxl_enet_init_hw(mc9328mxl_enet_softc_t *sc)
     MC9328MXL_GPIOA_IMR |= bit(3);
 
     /* Install the interrupt handler */
-    BSP_install_rtems_irq_handler(&mc9328mxl_enet_isr_data);
+    status = rtems_interrupt_handler_install(
+        BSP_INT_GPIO_PORTA,
+        "Network",
+        RTEMS_INTERRUPT_UNIQUE,
+        enet_isr,
+        (void *)BSP_INT_GPIO_PORTA
+    );
+    assert(status == RTEMS_SUCCESSFUL);
+    enet_isr_on();
 
 } /* mc9328mxl_enet_init_hw() */
 
@@ -602,7 +601,7 @@ void mc9328mxl_enet_stats (mc9328mxl_enet_softc_t *sc)
 
 
 /* Enables mc9328mxl_enet interrupts. */
-static void enet_isr_on(const rtems_irq_connect_data *unused)
+static void enet_isr_on()
 {
     /* Enable interrupts */
     MC9328MXL_AITC_INTENNUM = MC9328MXL_INT_GPIO_PORTA;
@@ -610,23 +609,6 @@ static void enet_isr_on(const rtems_irq_connect_data *unused)
     return;
 }
 
-/* Disables enet interrupts */
-static void enet_isr_off(const rtems_irq_connect_data *unused)
-{
-    /* disable all various TX/RX interrupts */
-    MC9328MXL_AITC_INTDISNUM = MC9328MXL_INT_GPIO_PORTA;
-
-    return;
-}
-
-/* Tests to see if mc9328mxl_enet interrupts are enabled, and returns non-0 if so.
- * If interrupt is not enabled, returns 0.
- */
-static int enet_isr_is_on(const rtems_irq_connect_data *irq)
-{
-    return MC9328MXL_AITC_INTENABLEL & (1 << MC9328MXL_INT_GPIO_PORTA);
-}
-
 /*  Driver ioctl handler */
 static int
 mc9328mxl_enet_ioctl (struct ifnet *ifp, ioctl_command_t command, caddr_t data)



Regards
Vipul Nayyar 



________________________________
 From: Vipul Nayyar <nayyar_vipul at yahoo.com>
To: Joel Sherrill <Joel.Sherrill at oarcorp.com> 
Cc: "rtems-devel at rtems.org" <rtems-devel at rtems.org> 
Sent: Sunday, 30 June 2013 4:17 PM
Subject: Re: [Patch] Removing legacy method from arm csb336
 


Hello,

Trying once gain with the arm csb336 patch. I'm explicitly checking the status code returned now, but am I doing the right thing if the status code returned is not RTEMS_SUCCESSFUL ? For now I'm doing the same what happens in BSP_install_rtems_irq_handler & BSP_remove_rtems_irq_handler ? 

commit 1887514fd17d17c089c4305f34963c244324eba3
Author: Vipul Nayyar <nayyar_vipul at yahoo.com>
Date:   Sun Jun 30 16:08:43 2013 +0530

    Updated Legacy code in arm csb336

diff --git a/c/src/lib/libbsp/arm/csb336/console/uart.c b/c/src/lib/libbsp/arm/csb336/console/uart.c
index 4dc409b..758f103 100644
--- a/c/src/lib/libbsp/arm/csb336/console/uart.c
+++ b/c/src/lib/libbsp/arm/csb336/console/uart.c
@@ -246,9 +246,36 @@ static int imx_uart_first_open(int major, int minor, void *arg)
 
     imx_uart_data[minor].tty   = args->iop->data1;
 
+    rtems_status_code sc = RTEMS_SUCCESSFUL;
+
 #if defined(USE_INTERRUPTS)
-    BSP_install_rtems_irq_handler(&imx_uart_tx_isr_data[minor]);
-    BSP_install_rtems_irq_handler(&imx_uart_rx_isr_data[minor]);
+    sc = rtems_interrupt_handler_install(
+      imx_uart_tx_isr_data[minor].name,
+      "Console",
+      RTEMS_INTERRUPT_UNIQUE,
+      imx_uart_tx_isr_data[minor].hdl,
+      imx_uart_tx_isr_data[minor].handle
+    );
+    if (sc != RTEMS_SUCCESSFUL) {
+    return 0;
+    }
+    if (imx_uart_tx_isr_data[minor].on != NULL) {
+    imx_uart_tx_isr_data[minor].on(&imx_uart_tx_isr_data[minor]);
+    }
+
+    sc = rtems_interrupt_handler_install(
+      imx_uart_rx_isr_data[minor].name,
+      "Console",
+      RTEMS_INTERRUPT_UNIQUE,
+      imx_uart_rx_isr_data[minor].hdl,
+      imx_uart_rx_isr_data[minor].handle
+    );
+    if (sc != RTEMS_SUCCESSFUL) {
+    return 0;
+    }
+    if (imx_uart_rx_isr_data[minor].on != NULL) {
+    imx_uart_rx_isr_data[minor].on(&imx_uart_rx_isr_data[minor]);
+    }
 
     imx_uart_data[minor].regs->cr1 |= MC9328MXL_UART_CR1_RRDYEN;
 #endif
@@ -258,9 +285,32 @@ static int imx_uart_first_open(int major, int minor, void *arg)
 
 static int imx_uart_last_close(int major, int minor, void *arg)
 {
+    rtems_status_code sc = RTEMS_SUCCESSFUL;
+
 #if defined(USE_INTERRUPTS)
-    BSP_remove_rtems_irq_handler(&imx_uart_tx_isr_data[minor]);
-    BSP_remove_rtems_irq_handler(&imx_uart_rx_isr_data[minor]);
+    if (imx_uart_tx_isr_data[minor].off != NULL) {
+    imx_uart_tx_isr_data[minor].off(&imx_uart_tx_isr_data[minor]);
+    }
+    sc = rtems_interrupt_handler_remove(
+        imx_uart_tx_isr_data[minor].name,
+        imx_uart_tx_isr_data[minor].hdl,
+        imx_uart_tx_isr_data[minor].handle
+    );
+    if (sc != RTEMS_SUCCESSFUL) {
+    return 0;
+    }
+    if (imx_uart_rx_isr_data[minor].off != NULL) {
+    imx_uart_rx_isr_data[minor].off(&imx_uart_rx_isr_data[minor]);
+    }
+
+    sc = rtems_interrupt_handler_remove(
+        imx_uart_rx_isr_data[minor].name,
+        imx_uart_rx_isr_data[minor].hdl,
+        imx_uart_rx_isr_data[minor].handle
+    );
+    if (sc != RTEMS_SUCCESSFUL) {
+    return 0;
+    }
 #endif
 
     return 0;
diff --git a/c/src/lib/libbsp/arm/csb336/network/network.c b/c/src/lib/libbsp/arm/csb336/network/network.c
index e513595..2441c01 100644
--- a/c/src/lib/libbsp/arm/csb336/network/network.c
+++ b/c/src/lib/libbsp/arm/csb336/network/network.c
@@ -238,6 +238,7 @@ void  mc9328mxl_enet_init_hw(mc9328mxl_enet_softc_t *sc)
 {
     uint16_t stat;
     uint16_t my = 0;
+    rtems_status_code sc = RTEMS_SUCCESSFUL;
 
     lan91c11x_write_reg(LAN91C11X_RCR, LAN91C11X_RCR_RST);
     lan91c11x_write_reg(LAN91C11X_RCR, 0);
@@ -334,7 +335,19 @@ void  mc9328mxl_enet_init_hw(mc9328mxl_enet_softc_t *sc)
     MC9328MXL_GPIOA_IMR |= bit(3);
 
     /* Install the interrupt handler */
-    BSP_install_rtems_irq_handler(&mc9328mxl_enet_isr_data);
+    sc = rtems_interrupt_handler_install(
+      mc9328mxl_enet_isr_data.name,
+      "Network",
+      RTEMS_INTERRUPT_UNIQUE,
+      mc9328mxl_enet_isr_data.hdl,
+      mc9328mxl_enet_isr_data.handle
+    );
+    if (sc != RTEMS_SUCCESSFUL) {
+    return 0;
+    }
+    if (mc9328mxl_enet_isr_data.on != NULL) {
+    mc9328mxl_enet_isr_data.on(&mc9328mxl_enet_isr_data);
+  }
 
 } /* mc9328mxl_enet_init_hw() */
 
Regards

Vipul Nayyar 



________________________________
 From: Joel Sherrill <Joel.Sherrill at OARcorp.com>
To: Vipul Nayyar <nayyar_vipul at yahoo.com> 
Cc: "rtems-devel at rtems.org" <rtems-devel at rtems.org>; Sebastian Huber <sebastian.huber at embedded-brains.de>; "gedare at rtems.org" <gedare at rtems.org> 
Sent: Sunday, 30 June 2013 2:04 AM
Subject: Re: [Patch] Removing legacy method from arm csb336
 


Please return the status code into a variable and test it separately in the assert. I will defer to Sebastian on whether assert should be used. I tend to think it should be a fatal error call with an explicit test. Using assert increases the code size. Plus this first one gives you a pattern for the rest. 



_______________________________________________
rtems-devel mailing list
rtems-devel at rtems.org
http://www.rtems.org/mailman/listinfo/rtems-devel
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.rtems.org/pipermail/devel/attachments/20130707/d568942e/attachment.html>


More information about the devel mailing list