Common neorv32 uart functions #509

akaeba · 2023-02-25T18:23:14Z

Use common UART functions, see #499 for details.

…hieve backward compatibility with c macros

@emb4fun

-> recommended by @emb4fun

stnolting · 2023-02-25T22:19:53Z

This looks good!

I think it would be cleaner to move all "legacy function wrappers" to sw/lib/include/legacy.h.

What about the neorv32_uart*_available and neorv32_uart*_setup functions? Shouldn't these also be generalized?

emb4fun · 2023-02-26T08:17:36Z

I have closed my PR because some of the tests was negative and I do not know why, and could not find the problem.

What about the neorv32_uart*_available and neorv32_uart*_setup functions? Shouldn't these also be generalized?

Yes both functions should also be generalized I think. If you need help, give me a sign.

I would also change two other points.

First: I would like to change the definition for the NEORV32_UART0. At the moment the define looks like:

#define NEORV32_UART0 (*((volatile neorv32_uart_t*) (NEORV32_UART0_BASE)))

I would like to change to:

#define NEORV32_UART0 ((volatile neorv32_uart_t*) (NEORV32_UART0_BASE))

This has the disadvantage that you cannot set the CTRL variable as follows, for example:

NEORV32_UART0.CTRL = 0;

But you have to use the following variant with the "pointer character":

NEORV32_UART0->CTRL = 0;

But this reflects what it really is, a pointer and not a "variable". Yes, that's a bit more work at first, but the changes should only be in the neorv32_uart.c file.

Of course, with this change, the macro must also look slightly different:

#define neorv32_uart1_disable() neorv32_uart_disable(NEORV32_UART0)

Here the "&" character in front of NEORV32_UART0 has now been removed.

Second: I would like to change the new parameter "self" to "UARTx":

void neorv32_uart_enable(volatile neorv32_uart_t *self);

Will be changed to:

void neorv32_uart_enable(volatile neorv32_uart_t *UARTx) ;

And now inside the functions it will look like:

UARTx->CTRL |= ((uint32_t)(1 << UART_CTRL_EN));

That somehow shows better what it is than using "self" here.

akaeba · 2023-02-26T09:47:36Z

@emb4fun : let us work together on this PR.

As next I introduce Stephans recommendation to move the c macros to the legacy.h function.

akaeba · 2023-02-26T09:57:35Z

This looks good!

I think it would be cleaner to move all "legacy function wrappers" to sw/lib/include/legacy.h.

done in f22bafe

akaeba · 2023-02-26T10:00:17Z

Second: I would like to change the new parameter "self" to "UARTx":

@emb4fun : now i work on this.

akaeba · 2023-02-26T10:05:40Z

@emb4fun : your second recommendation is implemented in 5c7c6ec.

…hieve backward compatibility with c macros

@emb4fun

-> recommended by @emb4fun

…aeba/neorv32 into common_neorv32_uart_functions

akaeba · 2023-02-26T10:11:09Z

@embfun: i did also a rebase, now the branch should be on main. Therefore the changes of neorv32.h should possible.

emb4fun · 2023-02-26T13:31:58Z

Hello akaeba,

the changes are looking good so far. But I am missing the first point.
I would like to change the definition for the NEORV32_UART0. At the moment the define looks like:

#define NEORV32_UART0 (*((volatile neorv32_uart_t*) (NEORV32_UART0_BASE)))

I would like to change to:

#define NEORV32_UART0 ((volatile neorv32_uart_t*) (NEORV32_UART0_BASE))

Or do you not want to take over this?

It looks that the following functions was not "translated":

neorv32_uart0_available
neorv32_uart1_available
neorv32_uart0_setup
neorv32_uart1_setup

Should I send you an example for these?

Best regards,
Michael

akaeba · 2023-02-26T13:47:45Z

Should I send you an example for these?

Yes please.

emb4fun · 2023-02-26T14:00:25Z

For neorv32_uart_available the source could look like:

int neorv32_uart_available (volatile neorv32_uart_t *UARTx)
{
   int available = 0;
   
   if( ((int)UARTx == NEORV32_UART0_BASE) && (NEORV32_SYSINFO.SOC & (1 << SYSINFO_SOC_IO_UART0)) )
   {
      available = 1;
   }
   
   if( ((int)UARTx == NEORV32_UART1_BASE) && (NEORV32_SYSINFO.SOC & (1 << SYSINFO_SOC_IO_UART1)) )
   {
      available = 1;
   }
   
   return(available);
}

emb4fun · 2023-02-26T14:00:57Z

What is your problem with the setup functions?

stnolting · 2023-02-26T17:23:05Z

@emb4fun

Your setup functions proposal looks good! 👍

Second: I would like to change the new parameter "self" to "UARTx":

I agree here.

First: I would like to change the definition for the NEORV32_UART0. At the moment the define looks like: [...]

If we change the way of accessing low-level register to the "pointer style" then the only the UARTs would requires this style while all remaining modules would still require "variable style". I think we should keep this identical across all modules.

However, I like the pointer concept. Maybe we can change that for all modules in a future PR?! 😉

emb4fun · 2023-02-26T18:57:15Z

@stnolting

we should discuss about the use of "volatile" too.

I think the keyword volatile is used here wrong in this project.
You actually want to achieve that the structure with which you access the device gets the volatile property.
But in case of the UART functions the volatile is used for the UARTx pointer like:

void neorv32_uart_enable(volatile neorv32_uart_t *UARTx);

If I have understood that correctly, only the pointer is volatile here, but not the structure to which the pointer points.
You would have to make the structure itself volatile here:

typedef volatile struct __attribute__((packed,aligned(4))) {
  uint32_t CTRL;  /**< offset 0: control register (#NEORV32_UART_CTRL_enum) */
  uint32_t DATA;  /**< offset 4: data register (#NEORV32_UART_DATA_enum) */
} neorv32_uart_t;

And now the prototype and function can be used without the volatile keyword:

void neorv32_uart_enable(neorv32_uart_t *UARTx);

Or you can also make every single register volatile via __IO. That was the suggestion I had made before #499.

However, I like the pointer concept. Maybe we can change that for all modules in a future PR?! 😉

Be careful with your wishes 😉

akaeba · 2023-03-04T10:47:58Z

For neorv32_uart_available the source could look like:

int neorv32_uart_available (volatile neorv32_uart_t *UARTx)
{
   int available = 0;
   
   if( ((int)UARTx == NEORV32_UART0_BASE) && (NEORV32_SYSINFO.SOC & (1 << SYSINFO_SOC_IO_UART0)) )
   {
      available = 1;
   }
   
   if( ((int)UARTx == NEORV32_UART1_BASE) && (NEORV32_SYSINFO.SOC & (1 << SYSINFO_SOC_IO_UART1)) )
   {
      available = 1;
   }
   
   return(available);
}

added in 269cc45

stnolting · 2023-03-04T10:54:30Z

Looks good so far! 👍

Only the setup function is still missing, right?

Seems like we have some merge conflicts here... @akaeba can you resolve those after completing your updates? 😉

akaeba · 2023-03-04T11:00:02Z

Only the setup function is still missing, right?

Yes, try to use same approach, like in the available functions?

What about the volatile, should I try to implement this approach:

#define __IO	volatile

typedef struct
{
   __IO uint32_t CTRL;
   __IO uint32_t DATA;
} UART_TypeDef;

but only for UART for start, the rest comes with every single change and the sw framework.

Seems like we have some merge conflicts here... @akaeba can you resolve those after completing your updates? 😉

Yes i can try.

stnolting · 2023-03-04T11:06:56Z

Yes, try to use same approach, like in the available functions?

I do not think an if-else is required as you can use the passed pointer for configuring the according control register.

Yes, try to use same approach, like in the available functions?

Right now we all structs are of type typedef volatile struct - I think that is ok for now.

but only for UART for start, the rest comes with every single change and the sw framework.

We can discuss this in a new issue (let's not change too much within a single PR 😉).

Yes i can try.

👍

akaeba · 2023-03-04T11:08:37Z

We can discuss this in a new issue (let's not change too much within a single PR 😉).

Perfect, let's bring this PR to main :)

akaeba · 2023-03-04T11:12:33Z

I do not think an if-else is required as you can use the passed pointer for configuring the according control register.

the issue is with the UART0_SIM_MODE and UART1_SIM_MODE. In the legacy variant would it be easy. but here i need to distinguish between UART0/1:

#ifdef UART_SIM_MODE
  #warning <UART_SIM_MODE> is obsolete (but still supported for compatibility). Please consider using the new flag <UART0_SIM_MODE>.
#endif
#if defined UART0_SIM_MODE || defined UART_SIM_MODE
  #warning UART0_SIM_MODE (primary UART) enabled! Sending all UART0.TX data to text.io simulation output instead of real UART0 transmitter. Use this for simulations only!
  uint32_t sim_mode = 1 << UART_CTRL_SIM_MODE;
#else
  uint32_t sim_mode = 0;
#endif

stnolting · 2023-03-04T11:14:53Z

I made a proposal for this in #499 (reply in thread):

// setup UART0
// ...
uint32_t sim_mode = 0;
#ifdef UART0_SIM_MODE
#warning UART0_SIM_MODE (primary UART) enabled! Sending all UART0.TX data to text.io simulation output instead of real UART0 transmitter. Use this for simulations only!
  if ((uint32_t(&self) == NEORV32_UART0_BASE) then {
    sim_mode = 1 << UART_CTRL_SIM_MODE;
  }
#endif
// ...

…2_uart_setup)

akaeba · 2023-03-04T11:27:56Z

I made a proposal for this in #499 (reply in thread)

oh, i don't had in mind.

…v32_uart_functions

…mon register definitions

…reg pointer

akaeba · 2023-03-04T11:51:59Z

@stnolting : I think it's mostly done.

emb4fun · 2023-03-04T11:58:36Z

Hello akaeba,

please take a look at the definitions too. The UART definitions looks like:

typedef struct attribute((packed,aligned(4))) {
uint32_t CTRL; /< offset 0: control register (#NEORV32_UART_CTRL_enum) */
uint32_t DATA; /< offset 4: data register (#NEORV32_UART_DATA_enum) */
} neorv32_uart_t;

Here I think the volatile was missing:

typedef volatile struct attribute((packed,aligned(4)))

At the same time, the definition of the UART_POINTER can be changed like:

/** UART0 module base address */
#define NEORV32_UART0_BASE (0xFFFFFFA0U)

/** UART0 module hardware access (#neorv32_uart_t) /
#define NEORV32_UART0 ((neorv32_uart_t) (NEORV32_UART0_BASE))

/** UART1 module base address */
#define NEORV32_UART1_BASE (0xFFFFFFD0U)

/** UART1 module hardware access (#neorv32_uart_t) /
#define NEORV32_UART1 ((neorv32_uart_t) (NEORV32_UART1_BASE))

And in case of the prototypes in legacy.h, the

&NEORV32_UART0

can be defined in

NEORV32_UART0

Best regards,
Michael

akaeba · 2023-03-04T12:00:55Z

@stnolting : now fails the checks. Could you please check where is the issue, probably a timing issue?

6094755000000 fs - UART1.RX                     -   ERROR - Equality check failed - Got cr. Expected 0.

stnolting · 2023-03-04T12:47:20Z

I'll have a look.

stnolting · 2023-03-04T13:05:12Z

Simulation mode of UART0 is never enabled, which leads to a simulation fail.

I think there is a problem with the code I have posted and the actual definition of the UART structs as (uint32_t) &UARTx does not return the actual base address.

@emb4fun has already converted the type of all "hardware handle structs" to typedef volatile struct. I suggest to to the same here. I'll take care of that if you agree and then we can re-evaluate 😉

from "variable style" to "pointer style"

akaeba · 2023-03-04T13:58:44Z

@stnolting great work! Now i think is this branch ready to merge.

akaeba added 3 commits February 19, 2023 12:32

as discussed in stnolting#499 use common functions for UART0/1 and ac…

90ebfc2

…hieve backward compatibility with c macros

remove some blank lines, minor comments

0498265

[sw/lib/neorv32_uart.h]: remove semicolons from macro defines

00566cb

-> recommended by @emb4fun

stnolting added SW software-related coding-style Related to the HW/SW coding style labels Feb 25, 2023

[sw/lib/include] move UART0/1 c macros to legacy.h

f22bafe

[sw/lib/*/neorv32_uart]: rename "self" to "UARTx

5c7c6ec

akaeba added 6 commits February 26, 2023 11:06

as discussed in stnolting#499 use common functions for UART0/1 and ac…

5f1188c

…hieve backward compatibility with c macros

remove some blank lines, minor comments

dd38337

[sw/lib/neorv32_uart.h]: remove semicolons from macro defines

143415a

-> recommended by @emb4fun

[sw/lib/include] move UART0/1 c macros to legacy.h

43f98de

[sw/lib/*/neorv32_uart]: rename "self" to "UARTx

296a65d

Merge branch 'common_neorv32_uart_functions' of https://github.com/ak…

c88f190

…aeba/neorv32 into common_neorv32_uart_functions

stnolting marked this pull request as draft February 26, 2023 17:43

stnolting mentioned this pull request Feb 26, 2023

Use common UART function #508

Closed

[/sw/lib/*/neorv32_uart]: add general neorv32_uart_available

269cc45

[sw/lib/*neorv32_uart]: add common setup function for UART0/1 (neorv3…

3f9da0b

…2_uart_setup)

akaeba added 5 commits March 4, 2023 12:33

Merge remote-tracking branch 'remotes/upstream/main' into common_neor…

68831c6

…v32_uart_functions

[sw/lib/source/neorv32_uart.c]: bugfixes after merge of neorv32.h com…

5907f2d

…mon register definitions

[sw/lib/source/neorv32_uart.c]: bugfix in SIM_MODE ifdef, uses wront …

f60e5ce

…reg pointer

[lib/source/neorv32_uart.c]: bugfix sim mode

6963b2e

[sw/lib/source/neorv32_uart.c]: bugfix sim mode

0a5fb2b

stnolting added 2 commits March 4, 2023 14:07

[sw/exampple] UART: convert "variable style" to "pointer style"

8c086bd

[sw/lib] change UART hardware handle type

68bcf0a

from "variable style" to "pointer style"

stnolting marked this pull request as ready for review March 4, 2023 13:20

stnolting mentioned this pull request Mar 4, 2023

[sw/uart] allow for escaped percent sign #531

Merged

stnolting approved these changes Mar 4, 2023

View reviewed changes

stnolting merged commit efb94ac into stnolting:main Mar 4, 2023

akaeba deleted the common_neorv32_uart_functions branch March 4, 2023 14:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Common neorv32 uart functions #509

Common neorv32 uart functions #509

akaeba commented Feb 25, 2023

stnolting commented Feb 25, 2023

emb4fun commented Feb 26, 2023

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023 •

edited

Loading

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023 •

edited

Loading

emb4fun commented Feb 26, 2023

akaeba commented Feb 26, 2023

emb4fun commented Feb 26, 2023 •

edited

Loading

emb4fun commented Feb 26, 2023

stnolting commented Feb 26, 2023 •

edited

Loading

emb4fun commented Feb 26, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

akaeba commented Mar 4, 2023

emb4fun commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

Common neorv32 uart functions #509

Common neorv32 uart functions #509

Conversation

akaeba commented Feb 25, 2023

stnolting commented Feb 25, 2023

emb4fun commented Feb 26, 2023

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023 • edited Loading

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023

akaeba commented Feb 26, 2023 • edited Loading

emb4fun commented Feb 26, 2023

akaeba commented Feb 26, 2023

emb4fun commented Feb 26, 2023 • edited Loading

emb4fun commented Feb 26, 2023

stnolting commented Feb 26, 2023 • edited Loading

emb4fun commented Feb 26, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

akaeba commented Mar 4, 2023

emb4fun commented Mar 4, 2023

akaeba commented Mar 4, 2023

stnolting commented Mar 4, 2023

stnolting commented Mar 4, 2023

akaeba commented Mar 4, 2023

akaeba commented Feb 26, 2023 •

edited

Loading

akaeba commented Feb 26, 2023 •

edited

Loading

emb4fun commented Feb 26, 2023 •

edited

Loading

stnolting commented Feb 26, 2023 •

edited

Loading