On Wednesday, November 27, 2019, Marc-André Lureau <
marcandre.lur...@redhat.com> wrote:
> Hi
>
> On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daudé
> <phi...@redhat.com> 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
> > >>> <aleksandar.m.m...@gmail.com> wrote:
> > >>>>
> > >>>>
> > >>>>
> > >>>> On Wednesday, November 20, 2019, Marc-André Lureau
> > >>>> <marcandre.lur...@redhat.com> wrote:
> > >>>>>
> > >>>>> Signed-off-by: Marc-André Lureau <marcandre.lur...@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 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 <amarko...@wavecomp.com>
>
> > >> 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 <w...@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 <w...@mail.berlios.de>
> > > Signed-off-by: Anthony Liguori <aligu...@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);
> > > +}
> > > +
> >
>
>
