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

[Marek Vasut]

On 08/19/2016 10:53 PM, Dmitry Osipenko wrote:
> On 19.08.2016 02:24, Marek Vasut wrote:
>> On 08/18/2016 11:49 AM, Dmitry Osipenko wrote:
>>> 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.
>>
>> Even better, prefix dropped. Thanks
>>
>> So what about patches 1..5 ? Anything I should change there ?
>>
> 
> Unfortunately, I don't have a good sense of bad in those patches. I guess
> maintainers are currently busy with a 2.7 release, and you are probably not 
> the
> only one in the review queue. Just wait for now, it could take a while.
> 
That makes sense.

-- 
Best regards,
Marek Vasut