Re: [Qemu-ppc] [PATCH v2 7/8] sm501: Do not clear read only bits when writing register

[BALATON Zoltan]

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