Hi Yoshinori,
On 5/16/19 7:52 AM, Yoshinori Sato wrote:
> Some RX peripheral using 8bit and 16bit registers.
> Added 8bit and 16bit APIs.
>
> Signed-off-by: Yoshinori Sato <ys...@users.sourceforge.jp>
> Reviewed-by: Richard Henderson <richard.hender...@linaro.org>
> ---
> include/hw/registerfields.h | 32 +++++++++++++++++++++++++++++++-
> 1 file changed, 31 insertions(+), 1 deletion(-)
>
> diff --git a/include/hw/registerfields.h b/include/hw/registerfields.h
> index 2659a58737..a0bb0654d6 100644
> --- a/include/hw/registerfields.h
> +++ b/include/hw/registerfields.h
> @@ -22,6 +22,14 @@
> enum { A_ ## reg = (addr) }; \
> enum { R_ ## reg = (addr) / 4 };
>
> +#define REG8(reg, addr) \
> + enum { A_ ## reg = (addr) }; \
> + enum { R_ ## reg = (addr) };
This macro is used in your devices (previous patches) and we can not
build the previous patches without this one:
CC rx-softmmu/hw/char/renesas_sci.o
./hw/char/renesas_sci.c:34:10: error: expected ‘)’ before numeric constant
REG8(SMR, 0)
^~
)
./hw/char/renesas_sci.c:42:10: error: expected ‘)’ before numeric constant
REG8(BRR, 1)
^~
)
./hw/char/renesas_sci.c:51:10: error: expected ‘)’ before numeric constant
REG8(TDR, 3)
^~
)
./hw/char/renesas_sci.c:62:10: error: expected ‘)’ before numeric constant
REG8(RDR, 5)
^~
)
./hw/char/renesas_sci.c:68:11: error: expected ‘)’ before numeric constant
REG8(SEMR, 7)
^~
)
./hw/char/renesas_sci.c: In function ‘can_receive’:
./hw/char/renesas_sci.c:78:16: error: implicit declaration of function
‘FIELD_EX8’; did you mean ‘FIELD_EX64’?
[-Werror=implicit-function-declaration]
return FIELD_EX8(sci->scr, SCR, RE);
^~~~~~~~~
FIELD_EX64
./hw/char/renesas_sci.c:78:16: error: nested extern declaration of
‘FIELD_EX8’ [-Werror=nested-externs]
./hw/char/renesas_sci.c:78:36: error: ‘SCR’ undeclared (first use in
this function)
return FIELD_EX8(sci->scr, SCR, RE);
^~~
./hw/char/renesas_sci.c:78:36: note: each undeclared identifier is
reported only once for each function it appears in
./hw/char/renesas_sci.c:78:41: error: ‘RE’ undeclared (first use in this
function)
return FIELD_EX8(sci->scr, SCR, RE);
^~
./hw/char/renesas_sci.c: In function ‘receive’:
./hw/char/renesas_sci.c:86:29: error: ‘SSR’ undeclared (first use in
this function)
if (FIELD_EX8(sci->ssr, SSR, RDRF) || size > 1) {
^~~
./hw/char/renesas_sci.c:86:34: error: ‘RDRF’ undeclared (first use in
this function)
if (FIELD_EX8(sci->ssr, SSR, RDRF) || size > 1) {
^~~~
./hw/char/renesas_sci.c:87:20: error: implicit declaration of function
‘FIELD_DP8’; did you mean ‘FIELD_DP32’?
[-Werror=implicit-function-declaration]
sci->ssr = FIELD_DP8(sci->ssr, SSR, ORER, 1);
^~~~~~~~~
FIELD_DP32
./hw/char/renesas_sci.c:87:20: error: nested extern declaration of
‘FIELD_DP8’ [-Werror=nested-externs]
./hw/char/renesas_sci.c:87:45: error: ‘ORER’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, ORER, 1);
^~~~
./hw/char/renesas_sci.c:88:33: error: ‘SCR’ undeclared (first use in
this function)
if (FIELD_EX8(sci->scr, SCR, RIE)) {
^~~
./hw/char/renesas_sci.c:88:38: error: ‘RIE’ undeclared (first use in
this function); did you mean ‘PIE’?
if (FIELD_EX8(sci->scr, SCR, RIE)) {
^~~
PIE
./hw/char/renesas_sci.c: In function ‘send_byte’:
./hw/char/renesas_sci.c:107:36: error: ‘SSR’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, TEND, 0);
^~~
./hw/char/renesas_sci.c:107:41: error: ‘TEND’ undeclared (first use in
this function); did you mean ‘TEI’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, TEND, 0);
^~~~
TEI
./hw/char/renesas_sci.c:108:41: error: ‘TDRE’ undeclared (first use in
this function); did you mean ‘TRUE’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, TDRE, 1);
^~~~
TRUE
./hw/char/renesas_sci.c:110:29: error: ‘SCR’ undeclared (first use in
this function)
if (FIELD_EX8(sci->scr, SCR, TIE)) {
^~~
./hw/char/renesas_sci.c:110:34: error: ‘TIE’ undeclared (first use in
this function); did you mean ‘PIE’?
if (FIELD_EX8(sci->scr, SCR, TIE)) {
^~~
PIE
./hw/char/renesas_sci.c: In function ‘txend’:
./hw/char/renesas_sci.c:118:30: error: ‘SSR’ undeclared (first use in
this function)
if (!FIELD_EX8(sci->ssr, SSR, TDRE)) {
^~~
./hw/char/renesas_sci.c:118:35: error: ‘TDRE’ undeclared (first use in
this function); did you mean ‘TRUE’?
if (!FIELD_EX8(sci->ssr, SSR, TDRE)) {
^~~~
TRUE
./hw/char/renesas_sci.c:121:45: error: ‘TEND’ undeclared (first use in
this function); did you mean ‘TEI’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, TEND, 1);
^~~~
TEI
./hw/char/renesas_sci.c:122:33: error: ‘SCR’ undeclared (first use in
this function)
if (FIELD_EX8(sci->scr, SCR, TEIE)) {
^~~
./hw/char/renesas_sci.c:122:38: error: ‘TEIE’ undeclared (first use in
this function); did you mean ‘TEI’?
if (FIELD_EX8(sci->scr, SCR, TEIE)) {
^~~~
TEI
./hw/char/renesas_sci.c: In function ‘update_trtime’:
./hw/char/renesas_sci.c:131:43: error: ‘SMR’ undeclared (first use in
this function)
sci->trtime = 8 - FIELD_EX8(sci->smr, SMR, CHR);
^~~
./hw/char/renesas_sci.c:131:48: error: ‘CHR’ undeclared (first use in
this function)
sci->trtime = 8 - FIELD_EX8(sci->smr, SMR, CHR);
^~~
./hw/char/renesas_sci.c:132:45: error: ‘PE’ undeclared (first use in
this function); did you mean ‘PIE’?
sci->trtime += FIELD_EX8(sci->smr, SMR, PE);
^~
PIE
./hw/char/renesas_sci.c:133:45: error: ‘STOP’ undeclared (first use in
this function)
sci->trtime += FIELD_EX8(sci->smr, SMR, STOP) + 1;
^~~~
./hw/char/renesas_sci.c:136:55: error: ‘CKS’ undeclared (first use in
this function)
sci->trtime *= 1 << (2 * FIELD_EX8(sci->smr, SMR, CKS));
^~~
./hw/char/renesas_sci.c: In function ‘sci_write’:
./hw/char/renesas_sci.c:150:10: error: ‘A_SMR’ undeclared (first use in
this function); did you mean ‘AF_SMC’?
case A_SMR:
^~~~~
AF_SMC
./hw/char/renesas_sci.c:142:21: error: ‘SCR’ undeclared (first use in
this function)
(FIELD_EX8(scr, SCR, TE) || FIELD_EX8(scr, SCR, RE))
^~~
./hw/char/renesas_sci.c:151:14: note: in expansion of macro ‘IS_TR_ENABLED’
if (!IS_TR_ENABLED(sci->scr)) {
^~~~~~~~~~~~~
./hw/char/renesas_sci.c:142:26: error: ‘TE’ undeclared (first use in
this function); did you mean ‘TEI’?
(FIELD_EX8(scr, SCR, TE) || FIELD_EX8(scr, SCR, RE))
^~
./hw/char/renesas_sci.c:151:14: note: in expansion of macro ‘IS_TR_ENABLED’
if (!IS_TR_ENABLED(sci->scr)) {
^~~~~~~~~~~~~
./hw/char/renesas_sci.c:142:53: error: ‘RE’ undeclared (first use in
this function)
(FIELD_EX8(scr, SCR, TE) || FIELD_EX8(scr, SCR, RE))
^~
./hw/char/renesas_sci.c:151:14: note: in expansion of macro ‘IS_TR_ENABLED’
if (!IS_TR_ENABLED(sci->scr)) {
^~~~~~~~~~~~~
./hw/char/renesas_sci.c:156:10: error: ‘A_BRR’ undeclared (first use in
this function)
case A_BRR:
^~~~~
./hw/char/renesas_sci.c:162:10: error: ‘A_SCR’ undeclared (first use in
this function)
case A_SCR:
^~~~~
./hw/char/renesas_sci.c:165:44: error: ‘SSR’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, TDRE, 1);
^~~
./hw/char/renesas_sci.c:165:49: error: ‘TDRE’ undeclared (first use in
this function); did you mean ‘TRUE’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, TDRE, 1);
^~~~
TRUE
./hw/char/renesas_sci.c:166:49: error: ‘TEND’ undeclared (first use in
this function); did you mean ‘TEI’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, TEND, 1);
^~~~
TEI
./hw/char/renesas_sci.c:167:42: error: ‘TIE’ undeclared (first use in
this function); did you mean ‘PIE’?
if (FIELD_EX8(sci->scr, SCR, TIE)) {
^~~
PIE
./hw/char/renesas_sci.c:171:39: error: ‘TEIE’ undeclared (first use in
this function); did you mean ‘TEI’?
if (!FIELD_EX8(sci->scr, SCR, TEIE)) {
^~~~
TEI
./hw/char/renesas_sci.c:174:39: error: ‘RIE’ undeclared (first use in
this function); did you mean ‘PIE’?
if (!FIELD_EX8(sci->scr, SCR, RIE)) {
^~~
PIE
./hw/char/renesas_sci.c:178:10: error: ‘A_TDR’ undeclared (first use in
this function)
case A_TDR:
^~~~~
./hw/char/renesas_sci.c:186:10: error: ‘A_SSR’ undeclared (first use in
this function); did you mean ‘A_PSW’?
case A_SSR:
^~~~~
A_PSW
./hw/char/renesas_sci.c:187:45: error: ‘MPBT’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, MPBT,
^~~~
./hw/char/renesas_sci.c:189:45: error: ‘ERR’ undeclared (first use in
this function); did you mean ‘ERI’?
sci->ssr = FIELD_DP8(sci->ssr, SSR, ERR,
^~~
ERI
./hw/char/renesas_sci.c:196:10: error: ‘A_RDR’ undeclared (first use in
this function); did you mean ‘O_RDWR’?
case A_RDR:
^~~~~
O_RDWR
./hw/char/renesas_sci.c:199:10: error: ‘A_SCMR’ undeclared (first use in
this function); did you mean ‘S_ISCHR’?
case A_SCMR:
^~~~~~
S_ISCHR
./hw/char/renesas_sci.c:201:10: error: ‘A_SEMR’ undeclared (first use in
this function); did you mean ‘AF_SMC’?
case A_SEMR: /* SEMR */
^~~~~~
AF_SMC
./hw/char/renesas_sci.c: In function ‘sci_read’:
./hw/char/renesas_sci.c:215:10: error: ‘A_SMR’ undeclared (first use in
this function); did you mean ‘AF_SMC’?
case A_SMR:
^~~~~
AF_SMC
./hw/char/renesas_sci.c:217:10: error: ‘A_BRR’ undeclared (first use in
this function)
case A_BRR:
^~~~~
./hw/char/renesas_sci.c:219:10: error: ‘A_SCR’ undeclared (first use in
this function)
case A_SCR:
^~~~~
./hw/char/renesas_sci.c:221:10: error: ‘A_TDR’ undeclared (first use in
this function)
case A_TDR:
^~~~~
./hw/char/renesas_sci.c:223:10: error: ‘A_SSR’ undeclared (first use in
this function); did you mean ‘A_PSW’?
case A_SSR:
^~~~~
A_PSW
./hw/char/renesas_sci.c:226:10: error: ‘A_RDR’ undeclared (first use in
this function); did you mean ‘O_RDWR’?
case A_RDR:
^~~~~
O_RDWR
./hw/char/renesas_sci.c:227:40: error: ‘SSR’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, RDRF, 0);
^~~
./hw/char/renesas_sci.c:227:45: error: ‘RDRF’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, RDRF, 0);
^~~~
./hw/char/renesas_sci.c:229:10: error: ‘A_SCMR’ undeclared (first use in
this function); did you mean ‘S_ISCHR’?
case A_SCMR:
^~~~~~
S_ISCHR
./hw/char/renesas_sci.c:231:10: error: ‘A_SEMR’ undeclared (first use in
this function); did you mean ‘AF_SMC’?
case A_SEMR:
^~~~~~
AF_SMC
./hw/char/renesas_sci.c: In function ‘sci_event’:
./hw/char/renesas_sci.c:266:40: error: ‘SSR’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, FER, 1);
^~~
./hw/char/renesas_sci.c:266:45: error: ‘FER’ undeclared (first use in
this function)
sci->ssr = FIELD_DP8(sci->ssr, SSR, FER, 1);
^~~
./hw/char/renesas_sci.c:267:33: error: ‘SCR’ undeclared (first use in
this function)
if (FIELD_EX8(sci->scr, SCR, RIE)) {
^~~
./hw/char/renesas_sci.c:267:38: error: ‘RIE’ undeclared (first use in
this function); did you mean ‘PIE’?
if (FIELD_EX8(sci->scr, SCR, RIE)) {
^~~
PIE
./hw/char/renesas_sci.c: In function ‘can_receive’:
./hw/char/renesas_sci.c:80:1: error: control reaches end of non-void
function [-Werror=return-type]
}
^
cc1: all warnings being treated as errors
I already commented that in a previous review.
> +
> +#define REG16(reg, addr) \
> + enum { A_ ## reg = (addr) }; \
> + enum { R_ ## reg = (addr) / 2 };
I wonder why you didn't include those macros before REG32 to keep them
ordered, not a big deal.
> +
> /* Define SHIFT, LENGTH and MASK constants for a field within a register */
>
> /* This macro will define R_FOO_BAR_MASK, R_FOO_BAR_SHIFT and
> R_FOO_BAR_LENGTH
> @@ -34,6 +42,12 @@
> MAKE_64BIT_MASK(shift, length)};
>
> /* Extract a field from a register */
> +#define FIELD_EX8(storage, reg, field) \
> + extract8((storage), R_ ## reg ## _ ## field ## _SHIFT, \
Then this function ...
> + R_ ## reg ## _ ## field ## _LENGTH)
> +#define FIELD_EX16(storage, reg, field) \
> + extract16((storage), R_ ## reg ## _ ## field ## _SHIFT, \
... and this one are added in the next patch, so you series is not
bisectable:
CC rx-softmmu/hw/char/renesas_sci.o
In file included from ./target/rx/cpu.h:24,
from ./hw/char/renesas_sci.c:26:
./hw/char/renesas_sci.c: In function ‘can_receive’:
./include/hw/registerfields.h:46:5: error: implicit declaration of
function ‘extract8’; did you mean ‘extract64’?
[-Werror=implicit-function-declaration]
extract8((storage), R_ ## reg ## _ ## field ## _SHIFT, \
^~~~~~~~
./hw/char/renesas_sci.c:78:16: note: in expansion of macro ‘FIELD_EX8’
return FIELD_EX8(sci->scr, SCR, RE);
^~~~~~~~~
./include/hw/registerfields.h:46:5: error: nested extern declaration of
‘extract8’ [-Werror=nested-externs]
extract8((storage), R_ ## reg ## _ ## field ## _SHIFT, \
^~~~~~~~
./hw/char/renesas_sci.c:78:16: note: in expansion of macro ‘FIELD_EX8’
return FIELD_EX8(sci->scr, SCR, RE);
^~~~~~~~~
cc1: all warnings being treated as errors
Richard: Can you reorder the series with:
- patch 11 first "Add extract8 and extract16"
- patch 10 then "Add 8bit and 16bit register macros"
(while here, can you strip the trailing dot in patch subject?)
- rest of this series, patches 1...9
- finally patch 12 (personally I'd have squashed this one
in patch 9 "Add rx-softmmu")
> + R_ ## reg ## _ ## field ## _LENGTH)
> #define FIELD_EX32(storage, reg, field) \
> extract32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
> R_ ## reg ## _ ## field ## _LENGTH)
> @@ -49,6 +63,22 @@
> * Assigning values larger then the target field will result in
> * compilation warnings.
> */
> +#define FIELD_DP8(storage, reg, field, val) ({ \
> + struct { \
> + unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
> + } v = { .v = val }; \
> + uint8_t d; \
> + d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
> + R_ ## reg ## _ ## field ## _LENGTH, v.v); \
> + d; })
> +#define FIELD_DP16(storage, reg, field, val) ({ \
> + struct { \
> + unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
> + } v = { .v = val }; \
> + uint16_t d; \
> + d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
> + R_ ## reg ## _ ## field ## _LENGTH, v.v); \
> + d; })
> #define FIELD_DP32(storage, reg, field, val) ({ \
> struct { \
> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
> @@ -57,7 +87,7 @@
> d = deposit32((storage), R_ ## reg ## _ ## field ## _SHIFT, \
> R_ ## reg ## _ ## field ## _LENGTH, v.v); \
> d; })
> -#define FIELD_DP64(storage, reg, field, val) ({ \
> +#define FIELD_DP64(storage, reg, field, val) ({ \
I suppose this is change is a mistake. Richard, do you mind dropping it?
> struct { \
> unsigned int v:R_ ## reg ## _ ## field ## _LENGTH; \
> } v = { .v = val }; \
>
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
