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

[Peter Maydell]

On 6 June 2018 at 15:28, BALATON Zoltan <address@hidden> wrote:
> 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?

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