[PATCH] stm32 uart: use 16bit register access

[PATCH] stm32 uart: use 16bit register access

[Spencer Oliver]

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

stm32 uart: use 16bit register access

Access all uart register using 16bit access, as per the ST docs.

[-- Attachment #2: ecos.hg_rev2949.patch --]
[-- Type: text/plain, Size: 9457 bytes --]

# HG changeset patch
# User Spencer Oliver <spen@spen-soft.co.uk>
# Date 1272975594 -3600
# Node ID c88d9f19de1aa679335bbec8b609e824d1fbd015
# Parent  d31e3aac03a66dd841d6b9a48c1261c00b437475
stm32 uart: use 16bit register access

Access all uart register using 16bit access, as per the ST docs.

diff -r d31e3aac03a6 -r c88d9f19de1a packages/devs/serial/cortexm/stm32/current/src/stm32_serial.c
--- a/packages/devs/serial/cortexm/stm32/current/src/stm32_serial.c	Tue May 04 13:18:19 2010 +0100
+++ b/packages/devs/serial/cortexm/stm32/current/src/stm32_serial.c	Tue May 04 13:19:54 2010 +0100
@@ -436,15 +436,15 @@
 
     cr1 |= CYGHWR_HAL_STM32_UART_CR1_TE | CYGHWR_HAL_STM32_UART_CR1_RE;
 
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR2, cr2 );
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR2, cr2 );
 
     // Set up baud rate
     hal_stm32_uart_setbaud( base, select_baud[new_config->baud] );
 
     // Enable the uart
     cr1 |= CYGHWR_HAL_STM32_UART_CR1_UE;
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );    
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );    
 
 
 #ifdef CYGOPT_IO_SERIAL_FLOW_CONTROL_HW
@@ -462,10 +462,10 @@
         cr1 |= CYGHWR_HAL_STM32_UART_CR1_RXNEIE;
         cr3 |= CYGHWR_HAL_STM32_UART_CR3_EIE;
         
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
     }
 
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR3, cr3 );         
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR3, cr3 );         
 
     stm32_chan->tx_active = false;
     
@@ -528,11 +528,11 @@
     const CYG_ADDRWORD base = stm32_chan->base;
     cyg_uint32 status;
     
-    HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_SR, status );
+    HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_SR, status );
 
     if (status & CYGHWR_HAL_STM32_UART_SR_TXE)
     {
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c );
         return true;
     }
     
@@ -549,10 +549,10 @@
     cyg_uint32 status;
 
      do {
-         HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_SR, status );
+         HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_SR, status );
      } while ((status & CYGHWR_HAL_STM32_UART_SR_TXE) == 0);
 
-     HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c );
+     HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c );
 
     return true;
 }
@@ -568,7 +568,7 @@
     CYG_WORD32 c;
 
     // Read data
-    HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c);
+    HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c);
     return (unsigned char) (c&0xFF);
 }
 
@@ -583,10 +583,10 @@
     cyg_uint32 c;
 
     do {
-        HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_SR, stat );
+        HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_SR, stat );
     } while ((stat & CYGHWR_HAL_STM32_UART_SR_RXNE) == 0);
         
-    HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c);
+    HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c);
     
     return (unsigned char) (c&0xFF);
 }
@@ -686,9 +686,9 @@
     if( !stm32_chan->tx_active )
     {
         stm32_chan->tx_active = true;
-        HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
         cr1 |= CYGHWR_HAL_STM32_UART_CR1_TXEIE;
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
     }
 
 }
@@ -706,9 +706,9 @@
     if( stm32_chan->tx_active )
     {
         stm32_chan->tx_active = false;
-        HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
         cr1 &= ~CYGHWR_HAL_STM32_UART_CR1_TXEIE;
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
     }
 
 }
@@ -739,7 +739,7 @@
     cyg_uint32 ret = CYG_ISR_HANDLED;
     cyg_drv_interrupt_acknowledge(vector);
 
-    HAL_READ_UINT32(base + CYGHWR_HAL_STM32_UART_SR, stat);
+    HAL_READ_UINT16(base + CYGHWR_HAL_STM32_UART_SR, stat);
     
     if( stat & CYGHWR_HAL_STM32_UART_SR_RXNE )
     {
@@ -751,7 +751,7 @@
 
             if( next == STM32_RXBUFSIZE ) next = 0;
         
-            HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c);
+            HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c);
         
             if( next != stm32_chan->buf_tail )
             {
@@ -764,15 +764,15 @@
                 // TODO: deal with buffer overflow
             }
 
-            HAL_READ_UINT32(base + CYGHWR_HAL_STM32_UART_SR, stat);
+            HAL_READ_UINT16(base + CYGHWR_HAL_STM32_UART_SR, stat);
         }
     }
     else if( stat & CYGHWR_HAL_STM32_UART_SR_TXE )
     {
         cyg_uint32 cr1;
-        HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
         cr1 &= ~CYGHWR_HAL_STM32_UART_CR1_TXEIE;
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
         
         ret |= CYG_ISR_CALL_DSR;
     }
@@ -782,7 +782,7 @@
     {
         // Clear CTS status if we see it.
         stat &= ~CYGHWR_HAL_STM32_UART_SR_CTS;
-        HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_SR, stat );
+        HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_SR, stat );
     }
     
     if( stat & (CYGHWR_HAL_STM32_UART_SR_FE|CYGHWR_HAL_STM32_UART_SR_NE|CYGHWR_HAL_STM32_UART_SR_ORE) )
@@ -816,7 +816,7 @@
         (chan->callbacks->rcv_char)(chan, c);
     }
 
-    HAL_READ_UINT32(base + CYGHWR_HAL_STM32_UART_SR, stat);
+    HAL_READ_UINT16(base + CYGHWR_HAL_STM32_UART_SR, stat);
     
     if( stm32_chan->tx_active && stat & CYGHWR_HAL_STM32_UART_SR_TXE )
     {
@@ -826,9 +826,9 @@
 
         if( stm32_chan->tx_active )
         {
-            HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+            HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
             cr1 |= CYGHWR_HAL_STM32_UART_CR1_TXEIE;
-            HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
+            HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_CR1, cr1 );
         }
     }
 }
diff -r d31e3aac03a6 -r c88d9f19de1a packages/hal/cortexm/stm32/var/current/src/hal_diag.c
--- a/packages/hal/cortexm/stm32/var/current/src/hal_diag.c	Tue May 04 13:18:19 2010 +0100
+++ b/packages/hal/cortexm/stm32/var/current/src/hal_diag.c	Tue May 04 13:19:54 2010 +0100
@@ -110,11 +110,11 @@
     CYGHWR_HAL_STM32_GPIO_SET( chan->txpin );
 
     cr2 = CYGHWR_HAL_STM32_UART_CR2_STOP_1;
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR2, cr2 );
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR2, cr2 );
 
     cr1 = CYGHWR_HAL_STM32_UART_CR1_M_8;
     cr1 |= CYGHWR_HAL_STM32_UART_CR1_TE | CYGHWR_HAL_STM32_UART_CR1_RE;
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
 
     // Set up Baud rate
     chan->baud_rate = CYGNUM_HAL_VIRTUAL_VECTOR_CONSOLE_CHANNEL_BAUD;
@@ -122,7 +122,7 @@
 
     // Enable the uart
     cr1 |= CYGHWR_HAL_STM32_UART_CR1_UE;
-    HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );    
+    HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );    
 
 }
 
@@ -135,10 +135,10 @@
 
      do
      {
-         HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_SR, sr );
+         HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_SR, sr );
      } while ((sr & CYGHWR_HAL_STM32_UART_SR_TXE) == 0);
 
-     HAL_WRITE_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c );     
+     HAL_WRITE_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c );     
     
     CYGARC_HAL_RESTORE_GP();    
 }
@@ -151,12 +151,12 @@
     cyg_uint32 c;
     CYGARC_HAL_SAVE_GP();
 
-    HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_SR, sr );
+    HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_SR, sr );
 
     if( (sr & CYGHWR_HAL_STM32_UART_SR_RXNE) == 0 )
         return false;
 
-    HAL_READ_UINT32( base + CYGHWR_HAL_STM32_UART_DR, c );     
+    HAL_READ_UINT16( base + CYGHWR_HAL_STM32_UART_DR, c );     
 
     *ch = (cyg_uint8)c;
 
@@ -246,17 +246,17 @@
         chan->irq_state = 1;
         HAL_INTERRUPT_ACKNOWLEDGE( chan->isr_vector );
         HAL_INTERRUPT_UNMASK( chan->isr_vector );
-        HAL_READ_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_READ_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
         cr1 |= CYGHWR_HAL_STM32_UART_CR1_RXNEIE;
-        HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );        
+        HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );        
         break;
     case __COMMCTL_IRQ_DISABLE:
         ret = chan->irq_state;
         chan->irq_state = 0;
         HAL_INTERRUPT_MASK( chan->isr_vector );
-        HAL_READ_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
+        HAL_READ_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );
         cr1 &= ~CYGHWR_HAL_STM32_UART_CR1_RXNEIE;
-        HAL_WRITE_UINT32( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );        
+        HAL_WRITE_UINT16( base+CYGHWR_HAL_STM32_UART_CR1, cr1 );        
         break;
     case __COMMCTL_DBG_ISR_VECTOR:
         ret = chan->isr_vector;

Re: [PATCH] stm32 uart: use 16bit register access

[Jonathan Larmour]

Spencer Oliver wrote:
> stm32 uart: use 16bit register access
> 
> Access all uart register using 16bit access, as per the ST docs.
[snip]

Thanks for that. It would seem wise to change the types of what's being 
written as well from cyg_uint32 to cyg_uint16.

Don't forget ChangeLog entries, thanks :).

Jifl
-- 
------["The best things in life aren't things."]------      Opinions==mine

Re: [PATCH] stm32 uart: use 16bit register access

[Nick Garnett]

Spencer Oliver <spen@spen-soft.co.uk> writes:

> stm32 uart: use 16bit register access
> 
> Access all uart register using 16bit access, as per the ST docs.

The ST docs I have, RM0008 version 11 from April 2010, show these
registers as 32 bits wide. The top 16 bits are alway marked as
reserved, so 16 bit accesses should work and the document says "The
peripheral registers can be accessed by half-words (16-bit) or words
(32-bit)." Nothing that requires 16 bit accesses. Given that ST have
left the 32 bit headroom, and that some of the registers are already
using all 16 low order bits, I think it would be a mistake to switch
to 16 bit accesses. Future versions might start using the higher order
bits, and we would just have to switch back.

Was there a specific reason you wanted to use 16 bit accesses rather
than 32 bit?

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

Re: [PATCH] stm32 uart: use 16bit register access

[Spencer Oliver]

On 17/05/2010 12:10, Nick Garnett wrote:
> Spencer Oliver<spen@spen-soft.co.uk>  writes:
>
>> stm32 uart: use 16bit register access
>>
>> Access all uart register using 16bit access, as per the ST docs.
>
> The ST docs I have, RM0008 version 11 from April 2010, show these
> registers as 32 bits wide. The top 16 bits are alway marked as
> reserved, so 16 bit accesses should work and the document says "The
> peripheral registers can be accessed by half-words (16-bit) or words
> (32-bit)." Nothing that requires 16 bit accesses. Given that ST have
> left the 32 bit headroom, and that some of the registers are already
> using all 16 low order bits, I think it would be a mistake to switch
> to 16 bit accesses. Future versions might start using the higher order
> bits, and we would just have to switch back.
>
> Was there a specific reason you wanted to use 16 bit accesses rather
> than 32 bit?
>

I was getting a Access Fault when testing, changing to use 16bit access 
cured the problem for me.
This was when the Enable Usage, Bus and Mem fault handlers were enabled 
in hal_reset_vsr.

Looking at reference code from the stm32lib shows all the accesses as 16bit.

Oh well, just rebuilt all and working fine now :)

Cheers
Spen

Re: [PATCH] stm32 uart: use 16bit register access

[Nick Garnett]

Spencer Oliver <spen@spen-soft.co.uk> writes:

> On 17/05/2010 12:10, Nick Garnett wrote:
> > Was there a specific reason you wanted to use 16 bit accesses rather
> > than 32 bit?
> >
> 
> I was getting a Access Fault when testing, changing to use 16bit
> access cured the problem for me.
> This was when the Enable Usage, Bus and Mem fault handlers were
> enabled in hal_reset_vsr.

Very odd. In theory these registers should be quite safe for all types
of access.

> 
> Looking at reference code from the stm32lib shows all the accesses
> as 16bit.

I think if we already had 16 bit accesses we wouldn't change them to
32 bit. But there seems little real need to go the other way either.

> 
> Oh well, just rebuilt all and working fine now :)
> 

OK, I think we should just leave things as they are for now. If you
come across it again and find compelling evidence to change then we
can revisit it.


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