Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

[Martin Laabs]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

Hi,

the CYGPKG_IO_SERIAL_ARM_LPC2XXX package support up to two serial
devices. Therefore it uses the VIC (vectored interrupt controller)
of the LPC devices. If both channels are enables in the configtool
both will also get the same interrupt priority. This is not possible
with the LPC VIC and causes, that only the last interrupt is
enables and the first one generates "spurious interrupts" (from eCos
point of view) that leads to data fetch and illegal instruction 
exeptions.

I added support to assign the priority in the configtool/driver.

Greetings,
 Martin

PS: Is it possible to avoid assignment of equal priorities
to different IRQs at same time in configtool/cdl.



[-- Attachment #2: prio.patch --]
[-- Type: application/octet-stream, Size: 2938 bytes --]

diff -urp /tmp/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl
--- /tmp/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl	2008-11-13 20:49:42.000000000 +0100
+++ lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl	2008-11-22 12:19:58.000000000 +0100
@@ -53,6 +53,8 @@ cdl_package CYGPKG_IO_SERIAL_ARM_LPC2XXX
     parent        CYGPKG_IO_SERIAL_DEVICES
     active_if     CYGPKG_IO_SERIAL
     active_if     CYGPKG_HAL_ARM_LPC2XXX
+    implements    CYGINT_IO_SERIAL_GENERIC_16X5X_CHAN_INTPRIO                   
+
 
     requires      CYGPKG_ERROR
     include_dir   cyg/io
@@ -121,6 +123,19 @@ cdl_package CYGPKG_IO_SERIAL_ARM_LPC2XXX
                 This option specifies the size of the internal buffers
                 used for the ARM LPC2XXX port 0."
         }
+	
+	cdl_option CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO {
+	     display "Interrupt priority of the serial port 0 ISR"
+	     flavor  data
+	     legal_values 0 to 15
+	     default_value 14
+	     description "
+	         This option specifies the interrupt priority of the
+		 ISR of the serial port 0 interrupt in the VIC.
+		 Slot 0 has the highest priority and slot 15 the lowest."
+	}
+
+
     }
 
     cdl_component CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL1 {
@@ -166,6 +181,18 @@ cdl_package CYGPKG_IO_SERIAL_ARM_LPC2XXX
                  This option specifies the size of the internal
                  buffers used for the ARM LPC2XXX port 1."
          }
+
+	
+	cdl_option CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO {
+	     display "Interrupt priority of the serial port 1 ISR"
+	     flavor  data
+	     legal_values 0 to 15
+	     default_value 15
+	     description "
+	         This option specifies the interrupt priority of the
+		 ISR of the serial port 1 interrupt in the VIC.
+		 Slot 0 has the highest priority and slot 15 the lowest."
+	}
     }
 
     cdl_component CYGPKG_IO_SERIAL_ARM_LPC2XXX_TESTING {

diff -urp /tmp/lpc2xxx/current/include/arm_lpc2xxx_ser.inl lpc2xxx/current/include/arm_lpc2xxx_ser.inl
--- /tmp/lpc2xxx/current/include/arm_lpc2xxx_ser.inl	2008-11-13 20:49:42.000000000 +0100
+++ lpc2xxx/current/include/arm_lpc2xxx_ser.inl	2008-11-22 12:20:24.000000000 +0100
@@ -87,7 +87,8 @@ static unsigned int select_baud[] = {
 #ifdef CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL0
 static pc_serial_info lpc2xxx_serial_info0 = 
   { CYGARC_HAL_LPC2XXX_REG_UART0_BASE,
-    CYGNUM_HAL_INTERRUPT_UART0
+    CYGNUM_HAL_INTERRUPT_UART0,
+    CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO
   };
 
 #if CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_BUFSIZE > 0
@@ -135,7 +136,8 @@ DEVTAB_ENTRY(lpc2xxx_serial_io0, 
 #ifdef CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL1
 static pc_serial_info lpc2xxx_serial_info1 = 
   { CYGARC_HAL_LPC2XXX_REG_UART1_BASE,
-    CYGNUM_HAL_INTERRUPT_UART1
+    CYGNUM_HAL_INTERRUPT_UART1,
+    CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO
   };
 #if CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_BUFSIZE > 0
 static unsigned char 

Re: Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

[Andrew Lunn]

> PS: Is it possible to avoid assignment of equal priorities
> to different IRQs at same time in configtool/cdl.

You can use an implies statement in one of the options. eg in
CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO0 have


    requires { is_active(CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO1) 
                   implies { CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO0 != 
                             CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO1 }
             }

     Andrew

Re: Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

[Andrew Lunn]

[-- Attachment #1: Type: text/plain, Size: 1192 bytes --]

On Sat, Nov 22, 2008 at 05:05:52PM +0100, Martin Laabs wrote:
> Hi,
> 
> the CYGPKG_IO_SERIAL_ARM_LPC2XXX package support up to two serial
> devices. Therefore it uses the VIC (vectored interrupt controller)
> of the LPC devices. If both channels are enables in the configtool
> both will also get the same interrupt priority. This is not possible
> with the LPC VIC and causes, that only the last interrupt is
> enables and the first one generates "spurious interrupts" (from eCos
> point of view) that leads to data fetch and illegal instruction 
> exeptions.
> 
> I added support to assign the priority in the configtool/driver.

Hi Martin

I extended the patch a little. I added a ChangeLog entry and some CDL
as i suggested to ensure the priorities are different between the
serial ports. However if some other device is using the same priority
this will not be detected. The correct place to fix that is in the
HAL. Humm, interesting. If you have asserts enabled, which you should
when developing, the function hal_interrupt_set_level() has:


        CYG_ASSERT((reg_val == 0) || (reg_val == (vector | 0x20)), 
                   "Priority already used by another vector");

   Andrew

[-- Attachment #2: devs_serial_arm_lpc2xxx.diff --]
[-- Type: text/x-diff, Size: 4157 bytes --]

Index: devs/serial/arm/lpc2xxx/current/ChangeLog
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/serial/arm/lpc2xxx/current/ChangeLog,v
retrieving revision 1.3
diff -u -r1.3 ChangeLog
--- devs/serial/arm/lpc2xxx/current/ChangeLog	22 Jun 2007 11:41:49 -0000	1.3
+++ devs/serial/arm/lpc2xxx/current/ChangeLog	23 Nov 2008 13:47:05 -0000
@@ -1,3 +1,10 @@
+2008-11-23  Martin Laabs <martin.laabs@mailbox.tu-dresden.de>
+            Andrew Lunn  <andrew@lunn.ch>
+
+	* cdl/ser_arm_lpc2xxx.cdl:
+	* include/arm_lpc2xxx_ser.inl: Ensure the serial interrupts are
+	using different priorities, otherwise we get spurious interrupts.
+
 2007-06-22  Alexander Aganichev <aaganichev@gmail.com>
 
 	* cdl/ser_arm_lpc2xxx.cdl:
Index: devs/serial/arm/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/serial/arm/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl,v
retrieving revision 1.2
diff -u -r1.2 ser_arm_lpc2xxx.cdl
--- devs/serial/arm/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl	22 Jun 2007 11:41:49 -0000	1.2
+++ devs/serial/arm/lpc2xxx/current/cdl/ser_arm_lpc2xxx.cdl	23 Nov 2008 13:47:06 -0000
@@ -53,6 +53,8 @@
     parent        CYGPKG_IO_SERIAL_DEVICES
     active_if     CYGPKG_IO_SERIAL
     active_if     CYGPKG_HAL_ARM_LPC2XXX
+    implements    CYGINT_IO_SERIAL_GENERIC_16X5X_CHAN_INTPRIO                   
+
 
     requires      CYGPKG_ERROR
     include_dir   cyg/io
@@ -121,6 +123,23 @@
                 This option specifies the size of the internal buffers
                 used for the ARM LPC2XXX port 0."
         }
+	
+	cdl_option CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO {
+	     display "Interrupt priority of the serial port 0 ISR"
+	     flavor  data
+	     legal_values 0 to 15
+	     default_value 14
+             requires { is_active(CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO)
+                   implies CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO !=
+                           CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO 
+             }
+	     description "
+	         This option specifies the interrupt priority of the
+		 ISR of the serial port 0 interrupt in the VIC.
+		 Slot 0 has the highest priority and slot 15 the lowest."
+	}
+
+
     }
 
     cdl_component CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL1 {
@@ -166,6 +185,18 @@
                  This option specifies the size of the internal
                  buffers used for the ARM LPC2XXX port 1."
          }
+
+	
+	cdl_option CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO {
+	     display "Interrupt priority of the serial port 1 ISR"
+	     flavor  data
+	     legal_values 0 to 15
+	     default_value 15
+	     description "
+	         This option specifies the interrupt priority of the
+		 ISR of the serial port 1 interrupt in the VIC.
+		 Slot 0 has the highest priority and slot 15 the lowest."
+	}
     }
 
     cdl_component CYGPKG_IO_SERIAL_ARM_LPC2XXX_TESTING {
Index: devs/serial/arm/lpc2xxx/current/include/arm_lpc2xxx_ser.inl
===================================================================
RCS file: /cvs/ecos/ecos/packages/devs/serial/arm/lpc2xxx/current/include/arm_lpc2xxx_ser.inl,v
retrieving revision 1.2
diff -u -r1.2 arm_lpc2xxx_ser.inl
--- devs/serial/arm/lpc2xxx/current/include/arm_lpc2xxx_ser.inl	15 Nov 2004 09:20:27 -0000	1.2
+++ devs/serial/arm/lpc2xxx/current/include/arm_lpc2xxx_ser.inl	23 Nov 2008 13:47:06 -0000
@@ -87,7 +87,8 @@
 #ifdef CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL0
 static pc_serial_info lpc2xxx_serial_info0 = 
   { CYGARC_HAL_LPC2XXX_REG_UART0_BASE,
-    CYGNUM_HAL_INTERRUPT_UART0
+    CYGNUM_HAL_INTERRUPT_UART0,
+    CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO
   };
 
 #if CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_BUFSIZE > 0
@@ -135,7 +136,8 @@
 #ifdef CYGPKG_IO_SERIAL_ARM_LPC2XXX_SERIAL1
 static pc_serial_info lpc2xxx_serial_info1 = 
   { CYGARC_HAL_LPC2XXX_REG_UART1_BASE,
-    CYGNUM_HAL_INTERRUPT_UART1
+    CYGNUM_HAL_INTERRUPT_UART1,
+    CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_INTPRIO
   };
 #if CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL1_BUFSIZE > 0
 static unsigned char 

Re: Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

[Martin Laabs]

Hi Andrew,

> I extended the patch a little. I added a ChangeLog entry and some CDL
> as i suggested to ensure the priorities are different between the
> serial ports.

Thank you.

> However if some other device is using the same priority
> this will not be detected. The correct place to fix that is in the
> HAL. Humm, interesting. If you have asserts enabled, which you should
> when developing, the function hal_interrupt_set_level() has:
>
>
>         CYG_ASSERT((reg_val == 0) || (reg_val == (vector | 0x20)),
>                    "Priority already used by another vector");

Yes - this was my starting point. I wonder that nobody else had this
error since eCos crashed with the two serial ports enabled. (And
IMHO the LPC2XXX serial driver is quite old.)


Greetings,
 Martin

Re: Patch for CYGPKG_IO_SERIAL_ARM_LPC2XXX

[Bart Veer]

>>>>> "Andrew" == Andrew Lunn <andrew@lunn.ch> writes:

    >> PS: Is it possible to avoid assignment of equal priorities to
    >> different IRQs at same time in configtool/cdl.

    Andrew> You can use an implies statement in one of the options. eg
    Andrew> in CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO0 have


    Andrew>     requires { is_active(CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO1) 
    Andrew>                    implies { CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO0 != 
    Andrew>                              CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO1 }
    Andrew>              }

That works for the specific case of preventing the two serial devices
from sharing the same interrupt priority, but does not prevent a
serial device from being given the same priority as some other device
(which I assume is the limitation within the LPC VIC).

I have run into similar problems on some ColdFire processors.
Unfortunately right now there is no good way of imposing the
constraint in CDL. The syntax for that would look something like:

  cdl_option CYGDAT_HAL_ARM_LPC2XXX_INTPRIO(0 to 31) {
      flavor        data
      default_value "user"
  }

  cdl_option CYGNUM_IO_SERIAL_ARM_LPC2XXX_SERIAL0_INTPRIO {
      legal_values 0 to 31
      requires    { CYGDAT_HAL_ARM_LPC2XXX_INTPRIO($self) == "uart0" }
  }

As long as all relevant device drivers, and anything else that
installed an interrupt handler, had a config option like that per
interrupt source the "requires" constraints would prevent two devices
being given the same interrupt priority. Also, the application
developer could see which interrupt priorities were still available,
and could reserve certain ones by setting user values.

Unfortunately there are no plans at present to extend the CDL language
with facilities like that.

What I have done in the past is to add a testcase to the processor HAL
which walks through the interrupt vectors, reports the priority
associated with each one, and checks for any conflicts. You can then
run that testcase in your configuration. However that won't catch
clashes between priorities used by device drivers and ones used by
application code. Another approach is to have a debug version of the
interrupt SET_LEVEL() macro which does a run-time check.

Bart

-- 
Bart Veer                                   eCos Configuration Architect
eCosCentric Limited    The eCos experts      http://www.ecoscentric.com/
Barnwell House, Barnwell Drive, Cambridge, UK.      Tel: +44 1223 245571
Registered in England and Wales: Reg No 4422071.