From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-6.2 required=3.0 tests=DKIM_ADSP_CUSTOM_MED, DKIM_INVALID,DKIM_SIGNED,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,HTML_MESSAGE,INCLUDES_PATCH,MAILING_LIST_MULTI, MIME_BOUND_DIGITS_15,SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 3CBC1C432C0 for ; Wed, 27 Nov 2019 12:21:39 +0000 (UTC) Received: from lists.gnu.org (lists.gnu.org [209.51.188.17]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id EA42A2070B for ; Wed, 27 Nov 2019 12:21:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=gmail.com header.i=@gmail.com header.b="Bz6tCNLP" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org EA42A2070B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:37632 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iZwKA-0000Ie-4N for qemu-devel@archiver.kernel.org; Wed, 27 Nov 2019 07:21:38 -0500 Received: from eggs.gnu.org ([2001:470:142:3::10]:54024) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iZwJF-00087l-CR for qemu-devel@nongnu.org; Wed, 27 Nov 2019 07:20:43 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iZwJD-0007n0-J1 for qemu-devel@nongnu.org; Wed, 27 Nov 2019 07:20:41 -0500 Received: from mail-ot1-x343.google.com ([2607:f8b0:4864:20::343]:41964) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iZwJD-0007mL-CQ for qemu-devel@nongnu.org; Wed, 27 Nov 2019 07:20:39 -0500 Received: by mail-ot1-x343.google.com with SMTP id r27so4363728otc.8 for ; Wed, 27 Nov 2019 04:20:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:in-reply-to:references:from:date:message-id:subject:to :cc; bh=279Fmm/vsf0UQqCpTtaG+gB5Ik8aR3mquYi0KYl8n7s=; b=Bz6tCNLP/DfrF6QfcefEfT1/JzKdYR1eKfsl83+jwp3qq/XMUdA0j93GX24U7CmZjY RQb6+U21dh7kG5GXvz3WlrA4yt1uocamFJYcrtYJacdX9pn6/l/TNXYXHBq1NnsgTryt N+dvn9bgpp2WH1ti/HUPRmMq6Ow0aEF2bmDyRPEXj5lDp11gnX2RmbwkBIqyy0ea2mul 3UqnLFCUQ6KVQiqjtnpPmasVG5q/L6M7VLHTLSFJpB4KbcWIJl6RP0A6pn1kAs2cysXP 9YOwIOfvcTktbY6jox7/0Tmfc/8+8yUSPjfXsGtsjd1N8IMY2/bxGX6ZUwZTct8JhQ4o j3cA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=279Fmm/vsf0UQqCpTtaG+gB5Ik8aR3mquYi0KYl8n7s=; b=bpsGl1LlI8g8z/RPQB+wtMLLbvQmt/irwcIHiZRT3BI8Os8zg781R9I4gVQREUKXdA tm530lsW9Kg3XRvATpzGGS9HMpKLl4PA8H4tEfo9ADrMopDPuvtlYZ7R+D8TPTt0IVOi snRf2j1mJxdrXwbHg+Uuv0e2ONTYlYjCDzeoH2dKUMhFPd13rc1KqbPlyuxViFHNugME 7gHvId/RL1ZSFoeirMj3Ym+CYy23KEg6EkWm3vGUn3LuMF4pxNrrrfFkoT90sglv7RCD /4no89jQs3mDNixRfzDPerPqwoyS2w5QRhI2az8yHQYnvBFcVdy82b+6ZRXq1Ju81vBB gR3w== X-Gm-Message-State: APjAAAWYYEr9COT7WzqN+OBdsd2aD1DnqsspsYm1vR+q8OU+xMlKdCg3 43Vk/z+EpSmD/AidecAk9yfQxP3TtsCSreGg/Mw= X-Google-Smtp-Source: APXvYqw5oRfRMp5jhFxJhUofzPKkWh7t0YkTxAics1dZ0qc8Iipwm696nLVFi3fiXQsygSztZNZxUjKr9//CbuAhyA4= X-Received: by 2002:a9d:3d05:: with SMTP id a5mr3514745otc.295.1574857238445; Wed, 27 Nov 2019 04:20:38 -0800 (PST) MIME-Version: 1.0 Received: by 2002:a05:6830:1391:0:0:0:0 with HTTP; Wed, 27 Nov 2019 04:20:37 -0800 (PST) In-Reply-To: References: <20191120152442.26657-1-marcandre.lureau@redhat.com> <20191120152442.26657-19-marcandre.lureau@redhat.com> <58a5a515-2244-a927-9ca7-c0bb64ceca53@redhat.com> <8992c003-5d65-77e9-17eb-592449cf9fdc@redhat.com> From: Aleksandar Markovic Date: Wed, 27 Nov 2019 13:20:37 +0100 Message-ID: Subject: Re: [PATCH v4 18/37] mips: baudbase is 115200 by default To: =?UTF-8?B?TWFyYy1BbmRyw6kgTHVyZWF1?= Content-Type: multipart/alternative; boundary="0000000000004504890598530697" X-detected-operating-system: by eggs.gnu.org: Genre and OS details not recognized. X-Received-From: 2607:f8b0:4864:20::343 X-BeenThere: qemu-devel@nongnu.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Damien Hedde , "peter.maydell@linaro.org" , "qemu-devel@nongnu.org" , Laurent Vivier , Aleksandar Markovic , Aleksandar Rikalo , =?UTF-8?Q?Philippe_Mathieu=2DDaud=C3=A9?= , Aurelien Jarno Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" --0000000000004504890598530697 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Wednesday, November 27, 2019, Marc-Andr=C3=A9 Lureau < marcandre.lureau@redhat.com> wrote: > Hi > > On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daud=C3=A9 > wrote: > > > > On 11/25/19 1:54 PM, Philippe Mathieu-Daud=C3=A9 wrote: > > > On 11/25/19 12:26 PM, Philippe Mathieu-Daud=C3=A9 wrote: > > >> On 11/25/19 11:12 AM, Marc-Andr=C3=A9 Lureau wrote: > > >>> Hi > > >>> > > >>> On Mon, Nov 25, 2019 at 2:07 PM Aleksandar Markovic > > >>> wrote: > > >>>> > > >>>> > > >>>> > > >>>> On Wednesday, November 20, 2019, Marc-Andr=C3=A9 Lureau > > >>>> wrote: > > >>>>> > > >>>>> Signed-off-by: Marc-Andr=C3=A9 Lureau > > >>>>> --- > > >>>>> 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 =3D 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 i= s > > >>>> 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 =3D=3D 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 > > > >> 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 > > > 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 > > > Signed-off-by: Anthony Liguori > > > > > > 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 bas= e, > > > 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 =3D frequency; > > > + serial_update_parameters(s); > > > +} > > > + > > > > --0000000000004504890598530697 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable

On Wednesday, November 27, 2019, Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com>= wrote:
Hi

On Mon, Nov 25, 2019 at 5:04 PM Philippe Mathieu-Daud=C3=A9
<philmd@redhat.com> wrote: >
> On 11/25/19 1:54 PM, Philippe Mathieu-Daud=C3=A9 wrote:
> > On 11/25/19 12:26 PM, Philippe Mathieu-Daud=C3=A9 wrote:
> >> On 11/25/19 11:12 AM, Marc-Andr=C3=A9 Lureau wrote:
> >>> Hi
> >>>
> >>> On Mon, Nov 25, 2019 at 2:07 PM Aleksandar Markovic
> >>> <aleksa= ndar.m.mail@gmail.com> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On Wednesday, November 20, 2019, Marc-Andr=C3=A9 Lure= au
> >>>> <ma= rcandre.lureau@redhat.com> wrote:
> >>>>>
> >>>>> Signed-off-by: Marc-Andr=C3=A9 Lureau <marcandre.lureau@redhat.com>=
> >>>>> ---
> >>>>>=C2=A0 =C2=A0hw/mips/mips_mipssim.c | 1 -
> >>>>>=C2=A0 =C2=A01 file changed, 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/mips/mips_mipssim.c b/hw/mips/mip= s_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(MachineStat= e *machine)
> >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0if (serial_hd(0)) {
> >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0DeviceSta= te *dev =3D qdev_create(NULL, TYPE_SERIAL_IO);
> >>>>>
> >>>>> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 qdev_prop_set_uint32= (DEVICE(dev), "baudbase", 115200);
> >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qdev_prop= _set_chr(dev, "chardev", serial_hd(0));
> >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qdev_set_= legacy_instance_id(dev, 0x3f8, 2);
> >>>>>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0qdev_init= _nofail(dev);
> >>>>> --
> >>>>
> >>>>
> >>>> Please mention in your commit message where the defau= lt 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 unclea= r 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" serie= s, I'd:
>
> - rename 'baudbase' -> 'input_frequency_hz'
>
> - set a 0 default value
>
>=C2=A0 =C2=A0DEFINE_PROP_UINT32("input-frequency-hz", Se= rialState,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0input_frequency_hz, 0),
>
> - add a check in serial_realize()
>
>=C2=A0 =C2=A0 =C2=A0 if (s->input_frequency_hz =3D=3D 0) {
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 error_setg(errp,
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 "serial: i= nput-frequency-hz property must be set");
>=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return;
>=C2=A0 =C2=A0 =C2=A0 }
>
> [*] 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 ad= d FIXME so that future readers are given at least some background.

Reviewed-by: Aleksandar Markovic <amarkovic@wavecomp.com>
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1px= #ccc solid;padding-left:1ex">
> >> This property confused me by the past. It is _not_ the baudra= te.
> >> It is the input frequency clocking the UART ('XIN' pi= n, 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= eil@mail.berlios.de>
> > Date:=C2=A0 =C2=A0Sat Oct 31 11:28:11 2009 +0100
> >
> >=C2=A0 =C2=A0 =C2=A0 serial: Add interface to set reference oscill= ator frequency
> >
> >=C2=A0 =C2=A0 =C2=A0 Many (most?) serial interfaces have a program= mable
> >=C2=A0 =C2=A0 =C2=A0 clock which provides the reference frequency = ("baudbase").
> >=C2=A0 =C2=A0 =C2=A0 So a fixed baudbase which is only set once ca= n be wrong.
> >
> >=C2=A0 =C2=A0 =C2=A0 omap1.c is an example which could use the new= interface
> >=C2=A0 =C2=A0 =C2=A0 to change baudbase when the programmable cloc= k changes.
> >=C2=A0 =C2=A0 =C2=A0 ar7 system emulation (still not part of stand= ard QEMU)
> >=C2=A0 =C2=A0 =C2=A0 is similar to omap and already uses serial_se= t_frequency.
> >
> >=C2=A0 =C2=A0 =C2=A0 Signed-off-by: Stefan Weil <weil@mail.berlios.de>
> >=C2=A0 =C2=A0 =C2=A0 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,
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 qemu_irq irq, int baudbase, > >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 CharDriverState *chr, int ior= egister);
> >=C2=A0 =C2=A0SerialState *serial_isa_init(int index, CharDriverSta= te *chr);
> > +void serial_set_frequency(SerialState *s, uint32_t frequenc= y);
> >
> >=C2=A0 =C2=A0/* 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)=
> >=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0serial_event, s);
> >=C2=A0 =C2=A0}
> >
> > +/* Change the main reference oscillator frequency. */
> > +void serial_set_frequency(SerialState *s, uint32_t frequenc= y)
> > +{
> > +=C2=A0 =C2=A0 s->baudbase =3D frequency;
> > +=C2=A0 =C2=A0 serial_update_parameters(s);
> > +}
> > +
>

--0000000000004504890598530697--