On 07/05/2018 08:39 PM, David Gibson wrote: > On Thu, Jul 05, 2018 at 11:19:50AM -0700, Michael Davidsaver wrote: > 11;rgb:ffff/ffff/ffff> Need to save HOUR[HOUR12] bit to keep >> track of guest selection of 12-hour mode. >> Write through current time registers to >> achieve this. Will be overwritten >> by the next read/latch. >> >> This was only being done in two of three >> arms of this conditional block. >> >> Signed-off-by: Michael Davidsaver <[email protected]> > > This looks dubious to me, or at least the explanation of it does. The > other branch of the conditional is covering different registers in the > device, which are part of the RTC component, rather than the NVRAM > area. I wouldn't necessarily expect them to persist data as a rule > the way the rest of the block does, even if this specific bit does > need to be preserved.
The fact that the above capture_current_time() included the line
> if (s->nvram[2] & HOURS_12) {
was enough to convince me that the original author intended to persist
the 12/24 hour mode in this way. There are certainly other ways to
accomplish this, but they would involved adding to the vmstate,
which I've tried to avoid in this iteration.
Also, I though I had test coverage of this bug. That's actually how I
noticed it to begin with. But it seems my later change to allow for a
slow test runner also stopped testing readback of the 12/24 hour mode bit.
It just silently uses whichever it reads. I'll be re-issuing an updated
version which restores this check. Then you will be able to easily
see the effect of reverting 'timer: ds1338 persist 12-hour mode selection'.
>> ---
>> hw/timer/ds1338.c | 4 +---
>> 1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/hw/timer/ds1338.c b/hw/timer/ds1338.c
>> index 7298c5af43..b56db5852e 100644
>> --- a/hw/timer/ds1338.c
>> +++ b/hw/timer/ds1338.c
>> @@ -220,10 +220,8 @@ static int ds1338_send(I2CSlave *i2c, uint8_t data)
>> value unchanged. */
>> data = (data & ~CTRL_OSF) | (data & s->nvram[s->ptr] & CTRL_OSF);
>>
>> - s->nvram[s->ptr] = data;
>> - } else {
>> - s->nvram[s->ptr] = data;
>> }
>> + s->nvram[s->ptr] = data;
>> inc_regptr(s);
>> return 0;
>> }
>
signature.asc
Description: OpenPGP digital signature
