[v2,7/8] sm501: Do not clear read only bits when writing register

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

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