Re: [Qemu-devel] [PATCH 5/7] nios2: Add periodic timer emulation

[Marek Vasut]

On 08/16/2016 11:38 PM, Dmitry Osipenko wrote:

[...]

>>>> Well what is sane clock frequency for hardware which can have arbitrary
>>>> frequency configured in ?
>>>>
>>>
>>> You could set to the one that is used by "10M50 GHRD" patch for example.
>>
>> That doesn't sound right . I can set it to 1 (Hz) , that should make it
>> slow enough to signalize that something is broken while providing valid
>> number.
>>
> 
> I'm not sure about it. Explicitly showing that something is wrong would be 
> better.
> 
>> +static void altera_timer_realize(DeviceState *dev, Error **errp)
>> +{
>> +    AlteraTimer *t = ALTERA_TIMER(dev);
>> +    SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>> +
>> +    assert(t->freq_hz != 0);
> 
> If you would prefer to keep error'ing out, then I can suggest to add some
> verbose message instead of the assertion, like:
> 
> if (!t->freq_hz) {
>     error_setg(errp, "altera_timer: \"clock-frequency\" property " \
>                      "must be provided");
>     return;
> }

That's better, thanks.

btw breaking strings is frowned upon in linux/u-boot as it makes it
impossible to git grep for error message. Does the same apply to qemu?

>>>>>> For the IIC, I wonder what that'd look like -- probably
>>>>>> qemu_set_irq(parent, 0); ?
>>>>>>
>>>>>
>>>>> No, IRQ's would be "deasserted" by resetting CPUClass state, of which 
>>>>> QEMU takes
>>>>> care itself. No action needed by the IIC.
>>>>
>>>> I see, thanks :)
>>>>
>>>> btw any chance someone can look at the other patches too ?
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Best regards,
Marek Vasut