Message ID | fdb61fc16922f78190f0db04a11dd20519cd2fb5.1528291908.git.balaton@eik.bme.hu (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu> > --- > 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 description in the commit message suggests that you want s->whatever = (value & rw_bit_mask) | ro_one_bits_mask; ? > 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? > break; > case SM501_IRQ_MASK: > s->irq_mask = value; thanks -- PMM
On Wed, 6 Jun 2018, Peter Maydell wrote: > On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu> >> --- >> 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
On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Wed, 6 Jun 2018, Peter Maydell wrote: >> >> On 6 June 2018 at 14:31, BALATON Zoltan <balaton@eik.bme.hu> 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 <balaton@eik.bme.hu> >>> --- >>> 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? It will enforce that the RO 1 bits are always 1 (by ORing those bits in). It's not the only way to do it. (NB we OR the "ro_one_bits", not "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; Yes, this works (and I have a feeling it's what we tend to use.) >>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, >>> hwaddr addr, >>> s->dram_control |= value & 0x7FFFFFC3; Hmm, does dram_control also need to behave the same way? >>> 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? I would personally put it in its own patch, but if you put it in this one do mention it in the commit message. Otherwise given the patch is changing four 'foo_control' registers it looks like this one was typoed. (It will look less so if the change for the others isn't just "add a '|'", though.) thanks -- PMM
On Wed, 6 Jun 2018, Peter Maydell wrote: > On 6 June 2018 at 15:28, BALATON Zoltan <balaton@eik.bme.hu> wrote: >> s->whatever &= ro_bits_mask; >> s->whatever |= value & rw_bits_mask; > > Yes, this works (and I have a feeling it's what we tend to use.) OK, I'll do this then in next respin. >>>> @@ -853,7 +853,7 @@ static void sm501_system_config_write(void *opaque, >>>> hwaddr addr, >>>> s->dram_control |= value & 0x7FFFFFC3; > > Hmm, does dram_control also need to behave the same way? It has one read only bit which we are not emulating so maybe not significant but changed it as well for correctness and added some more similar registers as well. >>>> 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? > > I would personally put it in its own patch, but if you put it > in this one do mention it in the commit message. Otherwise > given the patch is changing four 'foo_control' registers it > looks like this one was typoed. (It will look less so if > the change for the others isn't just "add a '|'", though.) I've left it in one patch to avoid having two many patches in the series but added it to commit message together with dram_control which also had a whitespace removed in same patch. Before sending v3 I'm waiting if there are any more comments for other patches in this series (http://patchew.org/QEMU/cover.1528291908.git.balaton@eik.bme.hu/) but looks like David is busy and could not look at them yet. Not sure if you've seen these patches but if you have a moment your comments are welcome. If no other changes are suggested I'll submit v3 in the coming days. Thank you, BALATON Zoltan
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; 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; break; case SM501_IRQ_MASK: s->irq_mask = value;
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 <balaton@eik.bme.hu> --- hw/display/sm501.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)