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

[Marc-André Lureau]

Hi

On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daudé
<address@hidden> wrote:
>
> 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
> >>> <address@hidden> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On Wednesday, November 20, 2019, Marc-André Lureau
> >>>> <address@hidden> wrote:
> >>>>>
> >>>>> Signed-off-by: Marc-André Lureau <address@hidden>
> >>>>> ---
> >>>>>   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/address@hidden/msg642174.html
>

This is getting further away from this series goal, and my initial
goal. Let's add this to the backlog. I can drop a FIXME there.

> >> 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 <address@hidden>
> > 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 <address@hidden>
> >      Signed-off-by: Anthony Liguori <address@hidden>
> >
> > 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);
> > +}
> > +
>