|
From: | BALATON Zoltan |
Subject: | Re: [Qemu-devel] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register |
Date: | Wed, 6 Jun 2018 16:28:41 +0200 (CEST) |
User-agent: | Alpine 2.21 (BSF 202 2017-01-01) |
On Wed, 6 Jun 2018, Peter Maydell wrote:
On 6 June 2018 at 14:31, BALATON Zoltan <address@hidden> wrote:When writing a register that has read only bits besides reserved bits we have to avoid changing read only bits that may have non zero default values. Signed-off-by: BALATON Zoltan <address@hidden> --- hw/display/sm501.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/hw/display/sm501.c b/hw/display/sm501.c index e47be99..7ec1434 100644 --- a/hw/display/sm501.c +++ b/hw/display/sm501.c @@ -836,10 +836,10 @@ static void sm501_system_config_write(void *opaque, hwaddr addr, switch (addr) { case SM501_SYSTEM_CONTROL: - s->system_control = value & 0xE300B8F7; + s->system_control |= value & 0xEF00B8F7;This will mean that you can't clear a r/w bit by writing a zero to it -- is that the hardware behaviour? The
Not really sure about real hardware behaviour. I only have that not very detailed docs we've looked at before but I assume r/w bits could be changed by writing them.
description in the commit message suggests that you want s->whatever = (value & rw_bit_mask) | ro_one_bits_mask;
Wouldn't that always set ro bits? I need to leave ro bits untouched by written value and only set rw bits. The previous version masked out ro bits which also cleared them due to assigning with =. Maybe
s->whatever &= ro_bits_mask; s->whatever |= value & rw_bits_mask; would work? Could this be simplified?
break; case SM501_MISC_CONTROL: - s->misc_control = value & 0xFF7FFF20; + s->misc_control |= value & 0xFF7FFF10; break; case SM501_GPIO31_0_CONTROL: s->gpio_31_0_control = value; @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, hwaddr addr, s->dram_control |= value & 0x7FFFFFC3; break; case SM501_ARBTRTN_CONTROL: - s->arbitration_control = value & 0x37777777; + s->arbitration_control = value & 0x37777777;Was this intended to be changed too?
Yes, just a simple white space cleanup. Does that worth a separate patch or enough to mention in commit message to make it clear it's intended change?
Thanks for the quick review, BALATON Zoltan
[Prev in Thread] | Current Thread | [Next in Thread] |