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

[BALATON Zoltan <balaton@eik.bme.hu>]

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