Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add shadow register for UBRRH to keep it separate from UCSRC #309

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add shadow register for UBRRH to keep it separate from UCSRC #309

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 25 additions & 1 deletion simavr/sim/avr_uart.c
View file
Open in desktop
Expand Up @@ -239,8 +239,27 @@ avr_uart_baud_write(
void * param)
{
avr_uart_t * p = (avr_uart_t *)param;
if (addr == p->ubrrh.reg) {
if (p->ubrrh.reg == p->r_ucsrc) {
// UBRRH and UCSRC registers can share the same I/O location.
// URSEL bit decides which register gets written. If it's UBRRH,
// store to shadow register, else to regular register
if ((v & (1 << 7)) == 0) { // URSEL
avr_regbit_setto_shadow(avr, p->ubrrh, v, &p->ubrrh_shadow);
} else {
avr_core_watch_write(avr, addr, v);
}
} else {
// If UBRRH have different I/O locations, update shadow register and
// regular register
avr_core_watch_write(avr, addr, v);
avr_regbit_setto_shadow(avr, p->ubrrh, v, &p->ubrrh_shadow);
}
return;
}
Comment on lines +242 to +259
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that sort of behaviour should be encoded in the core declaration, for example a new field with 'ursel' would both allow feature testing and encode the feature in a generic way. here it's all hard coded.
Also for that sort of feature, I don't /think/ we really need the new _shadow accessors for regbits, it's a simple enough behaviour that is just visible in the set/get for this feature, AFAIK...

Personally, I would introduce a more generic behaviour where the two (or more) values are stored part of a regbit with a discriminant, instead of having to carry around a pointer to an uint8_t...

avr_core_watch_write(avr, addr, v);
uint32_t val = avr_regbit_get(avr,p->ubrrl) | (avr_regbit_get(avr,p->ubrrh) << 8);
uint32_t val = avr_regbit_get(avr,p->ubrrl) |
(avr_regbit_get_shadow(avr, p->ubrrh, &p->ubrrh_shadow) << 8);

const int databits[] = { 5,6,7,8, /* 'reserved', assume 8 */8,8,8, 9 };
int db = databits[avr_regbit_get(avr, p->ucsz) | (avr_regbit_get(avr, p->ucsz2) << 2)];
Expand Down Expand Up @@ -468,6 +487,9 @@ avr_uart_reset(
uart_fifo_reset(&p->input);
p->tx_cnt = 0;

avr_regbit_clear(avr, p->ubrrl);
avr_regbit_clear_shadow(avr, p->ubrrh, &p->ubrrh_shadow);

avr_regbit_set(avr, p->ucsz);
avr_uart_regbit_clear(avr, p->ucsz2);

Expand Down Expand Up @@ -550,6 +572,8 @@ avr_uart_init(
avr_register_io_write(avr, p->r_ucsra, avr_uart_write, p);
if (p->ubrrl.reg)
avr_register_io_write(avr, p->ubrrl.reg, avr_uart_baud_write, p);
if (p->ubrrh.reg)
avr_register_io_write(avr, p->ubrrh.reg, avr_uart_baud_write, p);
avr_register_io_write(avr, p->rxen.reg, avr_uart_write, p);
}

1 change: 1 addition & 0 deletions simavr/sim/avr_uart.h
View file
Open in desktop
Expand Up @@ -113,6 +113,7 @@ typedef struct avr_uart_t {

avr_regbit_t ubrrl;
avr_regbit_t ubrrh;
uint8_t ubrrh_shadow;

avr_int_vector_t rxc;
avr_int_vector_t txc;
Expand Down
51 changes: 51 additions & 0 deletions simavr/sim/sim_regbit.h
View file
Open in desktop
Expand Up @@ -23,6 +23,7 @@
#define __SIM_REGBIT_H__

#include "sim_avr.h"
#include "sim_gdb.h"

#ifdef __cplusplus
extern "C" {
Expand Down Expand Up @@ -52,6 +53,22 @@ static inline uint8_t avr_regbit_set(avr_t * avr, avr_regbit_t rb)
return (avr->data[a] >> rb.bit) & rb.mask;
}

static inline uint8_t avr_regbit_set_shadow(avr_t * avr, avr_regbit_t rb, uint8_t * shadow)
{
uint16_t a = rb.reg;
uint8_t m;

if (!a)
return 0;
m = rb.mask << rb.bit;
if (avr->gdb) {
avr_gdb_handle_watchpoints(avr, a, AVR_GDB_WATCH_WRITE);
}
*shadow = *shadow | m;
avr_core_watch_write(avr, a, avr->data[a] | m);
return (*shadow >> rb.bit) & rb.mask;
}

static inline uint8_t avr_regbit_setto(avr_t * avr, avr_regbit_t rb, uint8_t v)
{
uint16_t a = rb.reg;
Expand All @@ -64,6 +81,21 @@ static inline uint8_t avr_regbit_setto(avr_t * avr, avr_regbit_t rb, uint8_t v)
return (avr->data[a] >> rb.bit) & rb.mask;
}

static inline uint8_t avr_regbit_setto_shadow(avr_t * avr, avr_regbit_t rb, uint8_t v, uint8_t * shadow)
{
uint16_t a = rb.reg;
uint8_t m;

if (!a)
return 0;
m = rb.mask << rb.bit;
if (avr->gdb) {
avr_gdb_handle_watchpoints(avr, a, AVR_GDB_WATCH_WRITE);
}
*shadow = (*shadow & ~(m)) | ((v << rb.bit) & m);
return (*shadow >> rb.bit) & rb.mask;
}

/*
* Set the 'raw' bits, if 'v' is the unshifted value of the bits
*/
Expand All @@ -88,6 +120,15 @@ static inline uint8_t avr_regbit_get(avr_t * avr, avr_regbit_t rb)
return (avr->data[a] >> rb.bit) & rb.mask;
}

static inline uint8_t avr_regbit_get_shadow(avr_t * avr, avr_regbit_t rb, uint8_t * shadow)
{
uint16_t a = rb.reg;
if (!a)
return 0;
//uint8_t m = rb.mask << rb.bit;
return (*shadow >> rb.bit) & rb.mask;
}

/*
* Using regbit from value eliminates some of the
* set to test then clear register operations.
Expand Down Expand Up @@ -124,6 +165,16 @@ static inline uint8_t avr_regbit_clear(avr_t * avr, avr_regbit_t rb)
return avr->data[a];
}

static inline uint8_t avr_regbit_clear_shadow(avr_t * avr, avr_regbit_t rb, uint8_t * shadow)
{
uint16_t a = rb.reg;
uint8_t m = rb.mask << rb.bit;
if (avr->gdb) {
avr_gdb_handle_watchpoints(avr, a, AVR_GDB_WATCH_WRITE);
}
*shadow = *shadow & ~m;
return *shadow;
}

/*
* This reads the bits for an array of avr_regbit_t, make up a "byte" with them.
Expand Down