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

[Peter Maydell]

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
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