| Message ID | 20191120152442.26657-19-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Clean-ups: qom-ify serial and remove QDEV_PROP_PTR | expand |
On Wed, 20 Nov 2019 at 15:28, 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); > -- > 2.24.0 Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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. Also, is there a guarantie that default value 115200 will never change in future? Yours, Aleksandar > 2.24.0 > > >
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
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 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). I'm not sure it makes sense to have a default, and I don't know what is the frequency modeled by the SPIM simulator.
On Mon, Nov 25, 2019 at 12:26 PM Philippe Mathieu-Daudé <philmd@redhat.com> 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 > > This property confused me by the past. It is _not_ the baudrate. The name is "boudbase" (whatever that means), so not "boudrate". Can we perhaps track the "inventor" of the property? Google search for the word "boudbase" gives me an address of a person in Maryland, whose last name is Boudbase, and also another address of apparently the same person in Fresno, CA. No serial-device-related results occurred (within the first page of Google results, at least). Sincerely, Aleksandar > 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). > > I'm not sure it makes sense to have a default, and I don't know what is > the frequency modeled by the SPIM simulator. >
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 > > 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); +} +
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); > +} > +
Hi On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daudé <philmd@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.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 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 <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); > > +} > > + >
On Wednesday, November 27, 2019, Marc-André Lureau < marcandre.lureau@redhat.com> wrote: > Hi > > On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daudé > <philmd@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.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 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 <amarkovic@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 <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); > > > +} > > > + > > > >
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);
Signed-off-by: Marc-André Lureau <marcandre.lureau@redhat.com> --- hw/mips/mips_mipssim.c | 1 - 1 file changed, 1 deletion(-)