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

[Aleksandar Markovic]

On Wednesday, November 27, 2019, Marc-André Lureau <address@hidden> wrote:
> 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.

I agree. But please update commit message and/or add FIXME so that future readers are given at least some background.

Reviewed-by: Aleksandar Markovic <address@hidden>

 

> > >> 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);
> > > +}
> > > +
> >