From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([209.51.188.92]:38700) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h0RWE-0005Pe-RA for qemu-devel@nongnu.org; Sun, 03 Mar 2019 08:51:08 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1h0RWC-0003oM-S6 for qemu-devel@nongnu.org; Sun, 03 Mar 2019 08:51:06 -0500 Received: from mail03.asahi-net.or.jp ([202.224.55.15]:43056) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1h0RWC-0003jg-DD for qemu-devel@nongnu.org; Sun, 03 Mar 2019 08:51:04 -0500 Date: Sun, 03 Mar 2019 22:50:59 +0900 Message-ID: <87lg1w3w3g.wl-ysato@users.sourceforge.jp> From: Yoshinori Sato In-Reply-To: References: <20190122121413.31437-1-ysato@users.sourceforge.jp> <20190302062138.10713-1-ysato@users.sourceforge.jp> <20190302062138.10713-9-ysato@users.sourceforge.jp> MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH RFC v3 08/11] RX62N internal serial communication interface List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Philippe =?ISO-8859-1?Q?Mathieu-Daud=E9?= Cc: qemu-devel@nongnu.org, peter.maydell@linaro.org, richard.henderson@linaro.org On Sun, 03 Mar 2019 04:21:10 +0900, Philippe Mathieu-Daud=E9 wrote: >=20 > Hi Yoshinori, >=20 > On 3/2/19 7:21 AM, Yoshinori Sato wrote: > > This module supported only non FIFO type. > > Hardware manual. > > https://www.renesas.com/us/en/doc/products/mpumcu/doc/rx_family/r01uh00= 33ej0140_rx62n.pdf?key=3D086621e01bd70347c18ea7f794aa9cc3 >=20 > This link also works without the trailing > "?key=3D086621e01bd70347c18ea7f794aa9cc3". OK. It is probably a parameter for searching. remove it. > >=20 > > Signed-off-by: Yoshinori Sato > > --- > > hw/char/Makefile.objs | 2 +- > > hw/char/renesas_sci.c | 288 ++++++++++++++++++++++++++++++++++= ++++++++ > > include/hw/char/renesas_sci.h | 42 ++++++ >=20 > QEMU provides a git config helper to improve git diff by showing headers > first and C sources after: scripts/git.orderfile > I recommend you to look at it. OK. > I'll review the header, then back to source. >=20 > > 3 files changed, 331 insertions(+), 1 deletion(-) > > create mode 100644 hw/char/renesas_sci.c > > create mode 100644 include/hw/char/renesas_sci.h > >=20 > > diff --git a/hw/char/Makefile.objs b/hw/char/Makefile.objs > > index c4947d7ae7..68eae7b9a5 100644 > > --- a/hw/char/Makefile.objs > > +++ b/hw/char/Makefile.objs > > @@ -15,7 +15,7 @@ common-obj-$(CONFIG_CADENCE) +=3D cadence_uart.o > > obj-$(CONFIG_EXYNOS4) +=3D exynos4210_uart.o > > obj-$(CONFIG_COLDFIRE) +=3D mcf_uart.o > > obj-$(CONFIG_OMAP) +=3D omap_uart.o > > -obj-$(CONFIG_SH4) +=3D sh_serial.o > > +obj-$(CONFIG_RENESAS_SCI) +=3D renesas_sci.o > > obj-$(CONFIG_PSERIES) +=3D spapr_vty.o > > obj-$(CONFIG_DIGIC) +=3D digic-uart.o > > obj-$(CONFIG_STM32F2XX_USART) +=3D stm32f2xx_usart.o > > diff --git a/hw/char/renesas_sci.c b/hw/char/renesas_sci.c > > new file mode 100644 > > index 0000000000..56d070a329 > > --- /dev/null > > +++ b/hw/char/renesas_sci.c > > @@ -0,0 +1,288 @@ > > +/* > > + * Renesas Serial Communication Interface >=20 > I'd add here: >=20 > Datasheet: RX62N Group, RX621 Group User's Manual: Hardware > (Rev.1.40 R01UH0033EJ0140) OK. > > + * > > + * Copyright (c) 2019 Yoshinori Sato > > + * > > + * This program is free software; you can redistribute it and/or modif= y it > > + * under the terms and conditions of the GNU General Public License, > > + * version 2 or later, as published by the Free Software Foundation. > > + * > > + * This program is distributed in the hope it will be useful, but WITH= OUT > > + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY = or > > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public Licen= se for > > + * more details. > > + * > > + * You should have received a copy of the GNU General Public License a= long with > > + * this program. If not, see . > > + */ > > + > > +#include "qemu/osdep.h" > > +#include "qemu-common.h" > > +#include "cpu.h" > > +#include "hw/hw.h" > > +#include "hw/sysbus.h" > > +#include "hw/char/renesas_sci.h" > > +#include "qemu/error-report.h" > > + > > +#define freq_to_ns(freq) (1000000000LL / freq) >=20 > You can directly use NANOSECONDS_PER_SECOND in update_trtime(). OK. > > + > > +static int can_receive(void *opaque) > > +{ > > + RSCIState *sci =3D RSCI(opaque); > > + if (sci->rx_next > qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL)) { > > + return 0; > > + } else { > > + return sci->scr & 0x10; >=20 > It is way easier to review a code using definitions generated by the > "hw/registerfields.h" API, a recent example is hw/char/cmsdk-apb-uart.c. OK. Other part have same code. I will update it as well. > > + } > > +} > > + > > +static void receive(void *opaque, const uint8_t *buf, int size) > > +{ > > + RSCIState *sci =3D RSCI(opaque); > > + sci->rdr =3D buf[0]; > > + if (sci->ssr & 0x40 || size > 1) { > > + sci->ssr |=3D 0x20; > > + if (sci->scr & 0x40) { > > + qemu_set_irq(sci->irq[ERI], 1); > > + } > > + } else { > > + sci->ssr |=3D 0x40; > > + sci->rx_next =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + sci->= trtime; > > + if (sci->scr & 0x40) { > > + qemu_set_irq(sci->irq[RXI], 1); > > + qemu_set_irq(sci->irq[RXI], 0); >=20 > Both lines are equivalent to: >=20 > qemu_irq_pulse(sci->irq[RXI]); OK.=20 >=20 > > + } > > + } > > +} > > + > > +static void send_byte(RSCIState *sci) > > +{ > > + if (qemu_chr_fe_backend_connected(&sci->chr)) { > > + qemu_chr_fe_write_all(&sci->chr, &sci->tdr, 1); > > + } > > + timer_mod(sci->timer, > > + qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + sci->trtime); > > + sci->ssr &=3D ~0x04; > > + sci->ssr |=3D 0x80; > > + qemu_set_irq(sci->irq[TEI], 0); > > + if (sci->scr & 0x80) { > > + qemu_set_irq(sci->irq[TXI], 1); > > + qemu_set_irq(sci->irq[TXI], 0); > > + } > > +} > > + > > +static void txend(void *opaque) > > +{ > > + RSCIState *sci =3D RSCI(opaque); > > + if ((sci->ssr & 0x80) =3D=3D 0) { > > + send_byte(sci); > > + } else { > > + sci->ssr |=3D 0x04; > > + if (sci->scr & 0x04) { > > + qemu_set_irq(sci->irq[TEI], 1); > > + } > > + } > > +} > > + > > +static void update_trtime(RSCIState *sci) > > +{ > > + static const int div[] =3D {1, 4, 16, 64}; > > + int w; > > + > > + w =3D (sci->smr & 0x40) ? 7 : 8; /* CHR */ > > + w +=3D (sci->smr >> 5) & 1; /* PE */ > > + w +=3D (sci->smr & 0x08) ? 2 : 1; /* STOP */ > > + sci->trtime =3D w * freq_to_ns(sci->input_freq) * > > + 32 * div[sci->smr & 0x03] * sci->brr; >=20 > Or: >=20 > sci->trtime =3D (sci->smr & 0x40) ? 7 : 8; > sci->trtime +=3D (sci->smr >> 5) & 1; > sci->trtime +=3D (sci->smr & 0x08) ? 2 : 1; > sci->trtime *=3D 32 * sci->brr; > sci->trtime <<=3D 2 * (sci->smr & 0x03); > sci->trtime *=3D NANOSECONDS_PER_SECOND; > sci->trtime /=3D sci->input_freq; OK. > > +} > > + > > +static void sci_write(void *opaque, hwaddr addr, uint64_t val, unsigne= d size) > > +{ > > + hwaddr offset =3D addr & 0x07; >=20 > You create the region with memory_region_init_io(size=3D8), so no need to= &=3D7. OK. > > + RSCIState *sci =3D RSCI(opaque); > > + int error =3D 0; > > + > > + switch (offset) { > > + case 0: /* SMR */ > > + if ((sci->scr & 0x30) =3D=3D 0) { > > + sci->smr =3D val; > > + update_trtime(sci); > > + } > > + break; > > + case 1: /* BRR */ > > + if ((sci->scr & 0x30) =3D=3D 0) { > > + sci->brr =3D val; > > + update_trtime(sci); > > + } > > + break; > > + case 2: /* SCR */ > > + sci->scr =3D val; > > + if (sci->scr & 0x20) { > > + sci->ssr |=3D 0x84; > > + qemu_set_irq(sci->irq[TXI], 1); > > + qemu_set_irq(sci->irq[TXI], 0); > > + } > > + if ((sci->scr & 0x04) =3D=3D 0) { > > + qemu_set_irq(sci->irq[TEI], 0); > > + } > > + if ((sci->scr & 0x40) =3D=3D 0) { > > + qemu_set_irq(sci->irq[ERI], 0); > > + } > > + break; > > + case 3: /* TDR */ > > + sci->tdr =3D val; > > + if (sci->ssr & 0x04) { > > + send_byte(sci); > > + } else{ > > + sci->ssr &=3D ~0x80; > > + } > > + break; > > + case 4: /* SSR */ > > + sci->ssr &=3D ~0x38 | (val & 0x38); >=20 > This expression is not clear... Don't you want: >=20 > sci->ssr &=3D ~0x38; > sci->ssr |=3D val & 0x38; Yes. It more clearly. > > + if (((sci->read_ssr & 0x38) ^ (sci->ssr & 0x38)) && > > + (sci->ssr & 0x38) =3D=3D 0) { >=20 > Is this equivalent to: >=20 > if ((sci->ssr & 0x38) =3D=3D 0 && (sci->read_ssr & 0x38) !=3D = 0) { OK. > > + qemu_set_irq(sci->irq[ERI], 0); > > + } > > + break; > > + case 5: /* RDR */ > > + error =3D 1; break; >=20 > Here error is due to read-only register, QEMU provides: >=20 > qemu_log_mask(LOG_GUEST_ERROR, "Read only register: "... OK. > > + case 6: /* SCMR */ > > + sci->scmr =3D val; break; > > + case 7: /* SEMR */ > > + sci->semr =3D val; break; > > + } > > + > > + if (error) { > > + error_report("rsci: unsupported write request to %08lx", addr); >=20 > "unsupported" is not very clear, for unimplemented you can use: >=20 > qemu_log_mask(LOG_UNIMP, > "Register XX not implemented\n"); OK. > > + } > > +} > > + > > +static uint64_t sci_read(void *opaque, hwaddr addr, unsigned size) > > +{ > > + hwaddr offset =3D addr & 0x07; > > + RSCIState *sci =3D RSCI(opaque); > > + int error =3D 0; > > + switch (offset) { > > + case 0: /* SMR */ > > + return sci->smr; > > + case 1: /* BRR */ > > + return sci->brr; > > + case 2: /* SCR */ > > + return sci->scr; > > + case 3: /* TDR */ > > + return sci->tdr; > > + case 4: /* SSR */ > > + sci->read_ssr =3D sci->ssr; > > + return sci->ssr; > > + case 5: /* RDR */ > > + sci->ssr &=3D ~0x40; > > + return sci->rdr; > > + case 6: /* SCMR */ > > + return sci->scmr; > > + case 7: /* SEMR */ > > + return sci->semr; > > + } > > + > > + if (error) { > > + error_report("rsci: unsupported write request to %08lx", addr); > > + } > > + return -1; > > +} > > + > > +static const MemoryRegionOps sci_ops =3D { > > + .write =3D sci_write, > > + .read =3D sci_read, > > + .endianness =3D DEVICE_NATIVE_ENDIAN, > > + .impl =3D { > > + .min_access_size =3D 1, >=20 > You can drop the implicit ".min_access_size =3D 1". OK. > > + .max_access_size =3D 1, > > + }, > > +}; > > + > > +static void rsci_reset(DeviceState *dev) > > +{ > > + RSCIState *sci =3D RSCI(dev); > > + sci->smr =3D sci->scr =3D 0x00; > > + sci->brr =3D 0xff; > > + sci->tdr =3D 0xff; > > + sci->rdr =3D 0x00; > > + sci->ssr =3D 0x84; > > + sci->scmr =3D 0x00; > > + sci->semr =3D 0x00; > > + sci->rx_next =3D qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL); > > +} > > + > > +static void sci_event(void *opaque, int event) > > +{ > > + RSCIState *sci =3D RSCI(opaque); > > + if (event =3D=3D CHR_EVENT_BREAK) { > > + sci->ssr |=3D 0x10; > > + if (sci->scr & 0x40) { > > + qemu_set_irq(sci->irq[ERI], 1); > > + } > > + } > > +} > > + > > +static void rsci_realize(DeviceState *dev, Error **errp) > > +{ > > + RSCIState *sci =3D RSCI(dev); > > + > > + qemu_chr_fe_set_handlers(&sci->chr, can_receive, receive, > > + sci_event, NULL, sci, NULL, true); >=20 > You might want to check s->input_freq !=3D 0 here. OK. > > +} > > + > > +static void rsci_init(Object *obj) > > +{ > > + SysBusDevice *d =3D SYS_BUS_DEVICE(obj); > > + RSCIState *sci =3D RSCI(obj); > > + int i; > > + > > + memory_region_init_io(&sci->memory, OBJECT(sci), &sci_ops, > > + sci, "renesas-sci", 0x8); > > + sysbus_init_mmio(d, &sci->memory); > > + > > + for (i =3D 0; i < 4; i++) { >=20 > 4 -> ARRAY_COUNT(sci->irq) or SCI_IRQ_COUNT. OK. > > + sysbus_init_irq(d, &sci->irq[i]); > > + } > > + sci->timer =3D timer_new_ns(QEMU_CLOCK_VIRTUAL, txend, sci); > > +} > > + > > +static const VMStateDescription vmstate_rcmt =3D { > > + .name =3D "renesas-sci", > > + .version_id =3D 1, > > + .minimum_version_id =3D 1, > > + .fields =3D (VMStateField[]) { > > + VMSTATE_END_OF_LIST() > > + } > > +}; > > + > > +static Property rsci_properties[] =3D { > > + DEFINE_PROP_UINT64("input-freq", RSCIState, input_freq, 0), > > + DEFINE_PROP_CHR("chardev", RSCIState, chr), > > + DEFINE_PROP_END_OF_LIST(), > > +}; > > + > > +static void rsci_class_init(ObjectClass *klass, void *data) > > +{ > > + DeviceClass *dc =3D DEVICE_CLASS(klass); > > + > > + dc->realize =3D rsci_realize; > > + dc->props =3D rsci_properties; > > + dc->vmsd =3D &vmstate_rcmt; > > + dc->reset =3D rsci_reset; > > +} > > + > > +static const TypeInfo rsci_info =3D { > > + .name =3D TYPE_RENESAS_SCI, > > + .parent =3D TYPE_SYS_BUS_DEVICE, > > + .instance_size =3D sizeof(RSCIState), > > + .instance_init =3D rsci_init, > > + .class_init =3D rsci_class_init, > > +}; > > + > > +static void rsci_register_types(void) > > +{ > > + type_register_static(&rsci_info); > > +} > > + > > +type_init(rsci_register_types) > > diff --git a/include/hw/char/renesas_sci.h b/include/hw/char/renesas_sc= i.h > > new file mode 100644 > > index 0000000000..47e3e7a5d7 > > --- /dev/null > > +++ b/include/hw/char/renesas_sci.h > > @@ -0,0 +1,42 @@ > > +/* > > + * Renesas Serial Communication Interface > > + * > > + * Copyright (c) 2018 Yoshinori Sato > > + * > > + * This code is licensed under the GPL version 2 or later. > > + * > > + */ > > + > > +#include "chardev/char-fe.h" > > +#include "qemu/timer.h" > > +#include "hw/sysbus.h" > > + > > +#define TYPE_RENESAS_SCI "renesas-sci" > > +#define RSCI(obj) OBJECT_CHECK(RSCIState, (obj), TYPE_RENESAS_SCI) > > + >=20 > Since those definitions are related, I recommend using: >=20 > enum SciIrqIndex { > ERI =3D 0, > RXI =3D 1, > ... >=20 > > +#define ERI 0 > > +#define RXI 1 > > +#define TXI 2 > > +#define TEI 3 >=20 > SCI_IRQ_COUNT =3D 4 > }; OK. > > + > > +typedef struct { > > + SysBusDevice parent_obj; > > + MemoryRegion memory; > > + > > + uint8_t smr; > > + uint8_t brr; > > + uint8_t scr; > > + uint8_t tdr; > > + uint8_t ssr; > > + uint8_t rdr; > > + uint8_t scmr; > > + uint8_t semr; > > + > > + uint8_t read_ssr; > > + long long trtime; > > + long long rx_next; >=20 > Can you use int64_t to keep both consistent? OK. > > + QEMUTimer *timer; > > + CharBackend chr; > > + uint64_t input_freq; > > + qemu_irq irq[4]; >=20 > Now you can use irq[SCI_IRQ_COUNT]; OK. > > +} RSCIState; > >=20 >=20 > Regards, >=20 > Phil. >=20 Your comment is very useful. Thank you. --=20 Yosinori Sato