Re: [PATCH v4 18/37] mips: baudbase is 115200 by default

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 11/25/19 1:54 PM, Philippe Mathieu-Daudé wrote:
> On 11/25/19 12:26 PM, Philippe Mathieu-Daudé wrote:
>> On 11/25/19 11:12 AM, Marc-André Lureau wrote:
>>> Hi
>>>
>>> On Mon, Nov 25, 2019 at 2:07 PM Aleksandar Markovic
>>> <aleksandar.m.mail@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Wednesday, November 20, 2019, Marc-André Lureau 
>>>> <marcandre.lureau@redhat.com> wrote:
>>>>>
>>>>> Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com>
>>>>> ---
>>>>>   hw/mips/mips_mipssim.c | 1 -
>>>>>   1 file changed, 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mips_mipssim.c
>>>>> index bfafa4d7e9..3cd0e6eb33 100644
>>>>> --- a/hw/mips/mips_mipssim.c
>>>>> +++ b/hw/mips/mips_mipssim.c
>>>>> @@ -223,7 +223,6 @@ mips_mipssim_init(MachineState *machine)
>>>>>       if (serial_hd(0)) {
>>>>>           DeviceState *dev = qdev_create(NULL, TYPE_SERIAL_IO);
>>>>>
>>>>> -        qdev_prop_set_uint32(DEVICE(dev), "baudbase", 115200);
>>>>>           qdev_prop_set_chr(dev, "chardev", serial_hd(0));
>>>>>           qdev_set_legacy_instance_id(dev, 0x3f8, 2);
>>>>>           qdev_init_nofail(dev);
>>>>> -- 
>>>>
>>>>
>>>> Please mention in your commit message where the default baudbase is 
>>>> set.
>>>
>>> ok
>>>
>>>> Also, is there a guarantie that default value 115200 will never 
>>>> change in future?
>>>
>>> The level of stability on properties in general is unclear to me.
>>>
>>> Given that 115200 is standard for serial, it is unlikely to change
>>> though.. We can have an assert there instead?
>>>
>>> Peter, what do you think? thanks

IOW, until we merge Damien's "Clock framework API" series, I'd:

- rename 'baudbase' -> 'input_frequency_hz'

- set a 0 default value

  DEFINE_PROP_UINT32("input-frequency-hz", SerialState,
                      input_frequency_hz, 0),

- add a check in serial_realize()

     if (s->input_frequency_hz == 0) {
         error_setg(errp,
               "serial: input-frequency-hz property must be set");
         return;
     }

[*] https://www.mail-archive.com/qemu-devel@nongnu.org/msg642174.html

>> This property confused me by the past. It is _not_ the baudrate.
>> It is the input frequency clocking the UART ('XIN' pin, Xtal INput).
>>
>> Each board has its own frequency, and it can even be variable (the 
>> clock domain tree can reconfigure it at a different rate).
> 
> Laurent pointed me to the following commit which confirms my 
> interpretation:
> 
> $ git show 038eaf82c853
> commit 038eaf82c853f3bf8d4c106c0677bbf4adada7de
> Author: Stefan Weil <weil@mail.berlios.de>
> Date:   Sat Oct 31 11:28:11 2009 +0100
> 
>      serial: Add interface to set reference oscillator frequency
> 
>      Many (most?) serial interfaces have a programmable
>      clock which provides the reference frequency ("baudbase").
>      So a fixed baudbase which is only set once can be wrong.
> 
>      omap1.c is an example which could use the new interface
>      to change baudbase when the programmable clock changes.
>      ar7 system emulation (still not part of standard QEMU)
>      is similar to omap and already uses serial_set_frequency.
> 
>      Signed-off-by: Stefan Weil <weil@mail.berlios.de>
>      Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> diff --git a/hw/pc.h b/hw/pc.h
> index 15fff8d103..03ffc91536 100644
> --- a/hw/pc.h
> +++ b/hw/pc.h
> @@ -13,6 +13,7 @@ SerialState *serial_mm_init (target_phys_addr_t base, 
> int it_shift,
>                                qemu_irq irq, int baudbase,
>                                CharDriverState *chr, int ioregister);
>   SerialState *serial_isa_init(int index, CharDriverState *chr);
> +void serial_set_frequency(SerialState *s, uint32_t frequency);
> 
>   /* parallel.c */
> 
> diff --git a/hw/serial.c b/hw/serial.c
> index fa12dcc075..0063260569 100644
> --- a/hw/serial.c
> +++ b/hw/serial.c
> @@ -730,6 +730,13 @@ static void serial_init_core(SerialState *s)
>                             serial_event, s);
>   }
> 
> +/* Change the main reference oscillator frequency. */
> +void serial_set_frequency(SerialState *s, uint32_t frequency)
> +{
> +    s->baudbase = frequency;
> +    serial_update_parameters(s);
> +}
> +