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

[Dmitry Osipenko]

On 17.08.2016 23:19, Marek Vasut wrote:
> 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?
> 

Actually, "altera_timer: " prefix isn't needed, as it should be already included
in the error message produced by by the error_setg(), so that string could be
squeezed into one line. And, of course, it could be rephrased more concisely.

>>>>>>> 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 ?
>>>>>
>>>>>
>>>>
>>>>
>>>
>>>
>>
>>
> 
> 


-- 
Dmitry