Re: [ECOS] Problems with interrupt and RTC on AT91EB40A -update +solution

[Nick Garnett <nickg at ecoscentric dot com>]

"Martin" <msie02@control.auc.dk> writes:

> Problem solved. 
>  
> The problem was that the timer counter 0 (TC0) in the ARM7TDMI processer was
> not enabled, i.e. it was not running. 
>  
> A solution to the problem is to modify two files plf_io.h and at91_misc.c
> and then rebuild. This was inspired by
> http://sources.redhat.com/ml/ecos-discuss/2003-02/msg00272.html, but changes
> made there was compliant with an older CVS version. I have attached the two
> modified files as we use them, and if changes to the original files in CVS
> are made later on, then look through the changes made by me and take what is
> needed. The changes are shown between 
>  
> // Begin Antenna2k4 changes 
> 	...Code... 
> // End Antenna2k4 changes 
>  
> I have tried to use the timer0 on two new AT91EB40A evaluation kits, and
> both of them were not able to start timer0, before the changes was made. 
>  
> Regards,
> 
> Martin 
> 

I'm not really happy with any of these changes. Even if they just stay
in your code, many seem to be just plain wrong.

See my comments below:


> void hal_clock_initialize(cyg_uint32 period)
> {
>     CYG_ADDRESS timer = AT91_TC+AT91_TC_TC0;
> 
>     // Begin Antenna2k4 changes
>     cyg_uint32 ps_clock_register;
>     
>     // Enable Timer Counter 0 in Power Save clock enable register
>     HAL_READ_UINT32(AT91_PS+AT91_PS_PCSR, ps_clock_register);
>     ps_clock_register &= (AT91_PS_PCER_PIO |
>                           AT91_PS_PCER_US0 |
>                           AT91_PS_PCER_US1);
>     ps_clock_register |= AT91_PS_PCER_TC0;
>     HAL_WRITE_UINT32(AT91_PS+AT91_PS_PCER, ps_clock_register);
>     
>     // End Antenna2k4 changes

I'm not sure I understand why this is necessary at all. The EB40A we
have here is working fine from the repository sources. All the
peripheral clocks are enabled in hal_platform_setup.h. If something
has changed in the EB40A such that that assignment no longer works,
then we should really try to discover what that is, rather than kludge
it here.

> // -------------------------------------------------------------------------
> // This routine is called to respond to a hardware interrupt (IRQ).  It
> // should interrogate the hardware and return the IRQ vector number.
> 
> // Begin Antenna2k4 changes
> /*
> int hal_IRQ_handler(void)
> {
>     cyg_uint32 irq_num;
>     cyg_uint32 ivr;
>     
>     // Calculate active interrupt (updates ISR)
>     HAL_READ_UINT32(AT91_AIC+AT91_AIC_IVR, ivr);
> 
>     HAL_READ_UINT32(AT91_AIC+AT91_AIC_ISR, irq_num);
> 
>     // No valid interrrupt source, treat as spurious interrupt    
>     if (irq_num < CYGNUM_HAL_ISR_MIN || irq_num > CYGNUM_HAL_ISR_MAX)
>       irq_num = CYGNUM_HAL_INTERRUPT_NONE;
>     
>     return irq_num;
> }
> */
> 
> int hal_IRQ_handler(void)
> {
>     cyg_uint32 irq_num;
>     cyg_uint32 ipr, imr;
> 
>     HAL_READ_UINT32(AT91_AIC+AT91_AIC_IPR, ipr);
>     HAL_READ_UINT32(AT91_AIC+AT91_AIC_IMR, imr);
>     ipr &= imr;
>     for (irq_num = 0;  irq_num < 19;  irq_num++) {
>         if (ipr & (1 << irq_num)) {
>             break;
>         }
>     }
> 
>     return irq_num;
> }
> 
> // End Antenna2k4 changes

This seems like an unnecessary change. In fact it is a backward step
since it substitutes a constant-time implementation for something that
is non-deterministic. What was wrong with the original code?

> void hal_interrupt_acknowledge(int vector)
> {
> 
>   // Begin Antenna2k4 changes
>     CYG_ASSERT(vector <= CYGNUM_HAL_ISR_MAX &&
>                vector >= CYGNUM_HAL_ISR_MIN , "Invalid vector");
> 
>     HAL_WRITE_UINT32(AT91_AIC+AT91_AIC_ICCR, (1<<vector));
>   // End Antenna2k4 changes
>
>     // No check for valid vector here! Spurious interrupts
>     // must be acknowledged, too.
>     HAL_WRITE_UINT32(AT91_AIC+AT91_AIC_EOI, 0xFFFFFFFF);  
> }

As the comment says, there should be no assert here since even invalid
vectors need to be acknowledged. 

What does the write to the ICCR achieve? The EOI will clear the
current interrupt and allow the next one through. The ICCR and ISCR
are really only present for testing. 

> // Begin Antenna2k4 changes
> 
> // 
> // Diagnostic LEDs - there are three colored LEDs which can be used
> // to send a simple diagnostic value (8 bits)
> //
> 
> void 
> _at91_led(int val)
> {
>     int i, to;
> 
>     HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_CODR, 0x06);  // DATA+CLOCK LEDs off
>     for (to = 0;  to < 0x200000; to++) ;
>     for (i = 0;  i < 8;  i++) {        
>         HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_SODR, ((val>>(7-i)) & 0x01)<<2);  // DATA LED
>         HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_SODR, 0x02);  // CLOCK LED on
>         for (to = 0;  to < 0x80000; to++) ;
>         HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_CODR, 0x02);  // CLOCK LED off
>         for (to = 0;  to < 0x40000; to++) ;
>         HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_CODR, 0x04);  // DATA LED off
>     }
> }
> 
> void
> set_leds(int val)
> {
>     HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_CODR, 0x16);
>     HAL_WRITE_UINT32(AT91_PIO+AT91_PIO_SODR, val);
> }
> // End Antenna2k4 changes
>

This appears to be a version of the EB40 LED code. It certainly should
not be here in the generic AT91 sources. In any case, you claim to
have an EB40A, which has a different LED arrangement, so I would not
expect this code to work at all.


All of your changes in plf_io.h really belong in var_io.h since they
are generic to all AT91 based boards.

-- 
Nick Garnett                    eCos Kernel Architect
http://www.ecoscentric.com/     The eCos and RedBoot experts


-- 
Before posting, please read the FAQ: http://ecos.sourceware.org/fom/ecos
and search the list archive: http://ecos.sourceware.org/ml/ecos-discuss