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=-13.8 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,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 67EF7C11F65 for ; Wed, 30 Jun 2021 15:16:04 +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 CBF48613EF for ; Wed, 30 Jun 2021 15:16:03 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org CBF48613EF Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=users.sourceforge.jp Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Received: from localhost ([::1]:36608 helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lybwY-0001rz-Ru for qemu-devel@archiver.kernel.org; Wed, 30 Jun 2021 11:16:02 -0400 Received: from eggs.gnu.org ([2001:470:142:3::10]:33178) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1lybvo-000170-BY for qemu-devel@nongnu.org; Wed, 30 Jun 2021 11:15:16 -0400 Received: from mail11.asahi-net.or.jp ([202.224.55.51]:42052) by eggs.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1lybvk-0001qV-Nr for qemu-devel@nongnu.org; Wed, 30 Jun 2021 11:15:15 -0400 Received: from sakura.ysato.name (ik1-413-38519.vs.sakura.ne.jp [153.127.30.23]) (Authenticated sender: PQ4Y-STU) by mail11.asahi-net.or.jp (Postfix) with ESMTPA id 5CF4C225D6; Thu, 1 Jul 2021 00:15:08 +0900 (JST) Received: from localhost.ysato.ml (z215167.dynamic.ppp.asahi-net.or.jp [110.4.215.167]) by sakura.ysato.name (Postfix) with ESMTPSA id D23651C0640; Thu, 1 Jul 2021 00:15:07 +0900 (JST) Date: Thu, 01 Jul 2021 00:15:04 +0900 Message-ID: From: Yoshinori Sato To: Peter Maydell Subject: Re: [PATCH 3/3] hw/sh4: sh7750 using renesas_sci. In-Reply-To: References: <20210616091244.33049-1-ysato@users.sourceforge.jp> <20210616091244.33049-4-ysato@users.sourceforge.jp> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?ISO-8859-4?Q?Goj=F2?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/27.1 (arm-unknown-linux-androideabi) MULE/6.0 (HANACHIRUSATO) MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII Received-SPF: softfail client-ip=202.224.55.51; envelope-from=ysato@users.sourceforge.jp; helo=mail11.asahi-net.or.jp X-Spam_score_int: -11 X-Spam_score: -1.2 X-Spam_bar: - X-Spam_report: (-1.2 / 5.0 requ) BAYES_00=-1.9, SPF_HELO_NONE=0.001, SPF_SOFTFAIL=0.665 autolearn=no autolearn_force=no X-Spam_action: no action 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: QEMU Developers Errors-To: qemu-devel-bounces+qemu-devel=archiver.kernel.org@nongnu.org Sender: "Qemu-devel" On Tue, 29 Jun 2021 22:23:01 +0900, Peter Maydell wrote: > > On Wed, 16 Jun 2021 at 10:14, Yoshinori Sato wrote: > > > > Signed-off-by: Yoshinori Sato > > --- > > include/hw/sh4/sh.h | 8 -------- > > hw/sh4/sh7750.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > hw/sh4/Kconfig | 2 +- > > 3 files changed, 42 insertions(+), 9 deletions(-) > > > > diff --git a/include/hw/sh4/sh.h b/include/hw/sh4/sh.h > > index becb596979..74e1ba59a8 100644 > > --- a/include/hw/sh4/sh.h > > +++ b/include/hw/sh4/sh.h > > @@ -55,14 +55,6 @@ int sh7750_register_io_device(struct SH7750State *s, > > > > /* sh_serial.c */ > > #define SH_SERIAL_FEAT_SCIF (1 << 0) > > -void sh_serial_init(MemoryRegion *sysmem, > > - hwaddr base, int feat, > > - uint32_t freq, Chardev *chr, > > - qemu_irq eri_source, > > - qemu_irq rxi_source, > > - qemu_irq txi_source, > > - qemu_irq tei_source, > > - qemu_irq bri_source); > > This change means that the code in sh_serial.c will no longer compile, > because it has a non-static function with no previous prototype. > The patch as a whole compiles because the change to hw/sh4/Kconfig > file removes the selection of SH_SCI and so we never try to > compile sh_serial.c. But we shouldn't leave dead code around in > the tree. > > Option A: > I guess the idea is to avoid having to rename sh_serial_init(), > which makes sense. If you want to take this route, then > in this patch we should mention that in the commit message, > something like: > > ===begin=== > hw/sh4: Switch sh7750 to new renesas-sci devices > > Switch the sh7750 away from the old serial device implementation > in sh_serial.c to use the new renesas-sci devices. > > Note that deleting the prototype for sh_serial_init() means that > sh_serial.c will no longer be able to compile (it would hit a > warning about having a non-static function without a previous > prototype). This is OK because we remove the only place that > was selecting the SH_SCI Kconfig feature, so we won't attempt > to compile that source file. In the following commit, we will > delete the file entirely. > > ===end=== > > Then in a new patch to go after this one, remove: > * the file sh_serial.c itself > * the "config SH_SCI" section in hw/char/Kconfig > * the line for sh_serial.c in hw/char/meson.build > > > Option B: would be to give your new sh7750.c function a different > name than sh_serial_init() (eg sci_init()) and change the two > callsites in this patch. Then you could keep the old sh_serial_init() > prototype in this patch and delete it as part of the new "remove > sh_serial.c" patch (which is where removal of the prototype more > logically belongs.) > > I don't mind which of the two you go for. I think option B is fine. I would like to change sh7750 to qom as the next step. I don't want to make any major changes at this step. > > /* sh7750.c */ > > qemu_irq sh7750_irl(struct SH7750State *s); > > diff --git a/hw/sh4/sh7750.c b/hw/sh4/sh7750.c > > index d53a436d8c..1ef8b73c65 100644 > > --- a/hw/sh4/sh7750.c > > +++ b/hw/sh4/sh7750.c > > @@ -24,6 +24,7 @@ > > */ > > > > #include "qemu/osdep.h" > > +#include "qapi/error.h" > > #include "hw/irq.h" > > #include "hw/sh4/sh.h" > > #include "sysemu/sysemu.h" > > @@ -32,6 +33,8 @@ > > #include "hw/sh4/sh_intc.h" > > #include "hw/timer/tmu012.h" > > #include "exec/exec-all.h" > > +#include "hw/char/renesas_sci.h" > > +#include "hw/qdev-properties.h" > > > > #define NB_DEVICES 4 > > > > @@ -752,6 +755,44 @@ static const MemoryRegionOps sh7750_mmct_ops = { > > .endianness = DEVICE_NATIVE_ENDIAN, > > }; > > > > +static void sh_serial_init(MemoryRegion *sysmem, > > + hwaddr base, int feat, > > + uint32_t freq, Chardev *chr, > > + qemu_irq eri_source, > > + qemu_irq rxi_source, > > + qemu_irq txi_source, > > + qemu_irq tei_source, > > + qemu_irq bri_source) > > +{ > > + RenesasSCIBaseState *sci; > > + char ckname[16]; > > + > > + switch(feat) { > > + case 0: /* SCI */ > > + sci = RENESAS_SCI_BASE(qdev_new(TYPE_RENESAS_SCI)); > > + snprintf(ckname, sizeof(ckname), "pck_sci"); > > + break; > > + case SH_SERIAL_FEAT_SCIF: > > + sci = RENESAS_SCI_BASE(qdev_new(TYPE_RENESAS_SCIF)); > > + snprintf(ckname, sizeof(ckname), "pck_scif"); > > + break; > > + } > > The ckname[] array seems to be set but never used ? Yes. This array used old changes. I forgot remove it. > > Since you never use 'sci' as a RenesasSCIBaseState, you could > instead declare > > Device *sci; > > and then > sci = qdev_new(TYPE_RENESAS_whatever); > > and avoid some of the DEVICE() casts below. OK. > You might also prefer to have a SysBusDevice *sbd which you > can then set with > sbd = SYS_BUS_DEVICE(sci); > and use sbd instead of casting every time you need it. OK. > > + qdev_prop_set_chr(DEVICE(sci), "chardev", chr); > > + qdev_prop_set_uint32(DEVICE(sci), "register-size", SCI_REGWIDTH_32); > > + qdev_prop_set_uint64(DEVICE(sci), "input-freq", freq); > > + sysbus_connect_irq(SYS_BUS_DEVICE(sci), 0, eri_source); > > + sysbus_connect_irq(SYS_BUS_DEVICE(sci), 1, rxi_source); > > + sysbus_connect_irq(SYS_BUS_DEVICE(sci), 2, txi_source); > > + if (tei_source) > > + sysbus_connect_irq(SYS_BUS_DEVICE(sci), 3, tei_source); > > + if (bri_source) > > + sysbus_connect_irq(SYS_BUS_DEVICE(sci), 3, bri_source); > > QEMU coding style requires braces for all if statements, even when > they're only one line. It was before checking with checkpatch.pl. I will clean it in the next patch. > > + sysbus_realize(SYS_BUS_DEVICE(sci), &error_abort); > > + sysbus_mmio_map(SYS_BUS_DEVICE(sci), 0, base); > > + sysbus_mmio_map(SYS_BUS_DEVICE(sci), 1, P4ADDR(base)); > > + sysbus_mmio_map(SYS_BUS_DEVICE(sci), 2, A7ADDR(base)); > > +} > > thanks > -- PMM -- Yoshinori Sato