Re: [Qemu-devel] [PATCHv3 1/2] m48t59: introduce new base_year qdev property

[Mark Cave-Ayland]

On 14/02/15 18:16, Hervé Poussineau wrote:

> Hi,
> 
> Le 14/02/2015 09:52, Mark Cave-Ayland a écrit :
>> Currently the m48t59 device uses the hardware model in order to determine
>> whether the year value is offset from the hardware value. As this will
>> soon be required by the x59 model, create a qdev base_year property to
>> represent the base year and update the callers appropriately.
>>
>> Signed-off-by: Mark Cave-Ayland <address@hidden>
>> ---
>>   hw/ppc/ppc405_boards.c    |    2 +-
>>   hw/ppc/prep.c             |    2 +-
>>   hw/sparc/sun4m.c          |    2 +-
>>   hw/sparc64/sun4u.c        |    2 +-
>>   hw/timer/m48t59.c         |   27 +++++++++++++++------------
>>   include/hw/timer/m48t59.h |    5 +++--
>>   6 files changed, 22 insertions(+), 18 deletions(-)
>>
>> diff --git a/hw/ppc/ppc405_boards.c b/hw/ppc/ppc405_boards.c
>> index 1dcea77..5019f20 100644
>> --- a/hw/ppc/ppc405_boards.c
>> +++ b/hw/ppc/ppc405_boards.c
>> @@ -283,7 +283,7 @@ static void ref405ep_init(MachineState *machine)
>>   #ifdef DEBUG_BOARD_INIT
>>       printf("%s: register NVRAM\n", __func__);
>>   #endif
>> -    m48t59_init(NULL, 0xF0000000, 0, 8192, 8);
>> +    m48t59_init(NULL, 0xF0000000, 0, 8192, 2000, 8);
>>       /* Load kernel */
>>       linux_boot = (kernel_filename != NULL);
>>       if (linux_boot) {
> 
> Before this patch, m48t59 model 8 (ie m48t08) was handled with a base
> year of 1968 by default. Here, you're changing the behaviour. Is is
> expected, or did you meant 1968?

Yes, you are correct - looks like I made a mistake here when fixing up
the callers.

>> [...]
> 
>> @@ -387,11 +388,7 @@ static void m48t59_write(M48t59State *NVRAM,
>> uint32_t addr, uint32_t val)
>>       tmp = from_bcd(val);
>>       if (tmp >= 0 && tmp <= 99) {
>>           get_time(NVRAM, &tm);
>> -            if (NVRAM->model == 8) {
>> -                tm.tm_year = from_bcd(val) + 68; // Base year is 1968
>> -            } else {
>> -                tm.tm_year = from_bcd(val);
>> -            }
>> +            tm.tm_year = from_bcd(val) + NVRAM->base_year - 1900;
>>           set_time(NVRAM, &tm);
>>       }
>>           break;
> 
> Here, 1968 was the default base year for m48t08 on writes.
> 
>> @@ -493,11 +490,7 @@ static uint32_t m48t59_read(M48t59State *NVRAM,
>> uint32_t addr)
>>       case 0x07FF:
>>           /* year */
>>           get_time(NVRAM, &tm);
>> -        if (NVRAM->model == 8) {
>> -            retval = to_bcd(tm.tm_year - 68); // Base year is 1968
>> -        } else {
>> -            retval = to_bcd(tm.tm_year);
>> -        }
>> +        retval = to_bcd((tm.tm_year + 1900 - NVRAM->base_year) % 100);
>>           break;
>>       default:
>>           /* Check lock registers state */
> 
> Here, 1968 was the default base year for m48t08 on reads.
> 
>>  [...]
> 
>> @@ -809,6 +805,7 @@ static void m48txx_isa_toggle_lock(Nvram *obj, int
>> lock)
>>   }
>>
>>   static Property m48t59_isa_properties[] = {
>> +    DEFINE_PROP_INT32("base_year", M48txxISAState, state.base_year, 0),
>>       DEFINE_PROP_UINT32("iobase", M48txxISAState, io_base, 0x74),
>>       DEFINE_PROP_END_OF_LIST(),
>>   };
> 
> It seems like tha QEMU way of naming properties is with an hyphen, not
> with an underscore. So you should use "base-year" instead of "base_year".
> 
>> @@ -852,6 +849,11 @@ static void m48txx_sysbus_toggle_lock(Nvram *obj,
>> int lock)
>>       m48t59_toggle_lock(&d->state, lock);
>>   }
>>
>> +static Property m48t59_sysbus_properties[] = {
>> +    DEFINE_PROP_INT32("base_year", M48txxSysBusState,
>> state.base_year, 0),
>> +    DEFINE_PROP_END_OF_LIST(),
>> +};
>> +
> 
> Again here.

Okay. I'll fix this up and resend the series again. Thanks for taking
the time to review!


ATB,

Mark.