From: Tommy Wu <tommy.wu@sifive.com>
To: "Philippe Mathieu-Daudé" <philmd@linaro.org>
Cc: qemu-devel@nongnu.org, qemu-riscv@nongnu.org,
alistair.francis@wdc.com, palmer@dabbelt.com,
bin.meng@windriver.com, jim.shu@sifive.com,
frank.chang@sifive.com, thuth@redhat.com, liweiwei@iscas.ac.cn
Subject: Re: [PATCH v4 1/3] hw/misc: sifive_e_aon: Support the watchdog timer of HiFive 1 rev b.
Date: Fri, 9 Jun 2023 00:56:14 +0800 [thread overview]
Message-ID: <CANj3q_kX2+XX7L=fJQ9DcOuXv2Ry5yrcfQTxVU662z8a2pk18Q@mail.gmail.com> (raw)
In-Reply-To: <e9ab4620-2878-45c1-264b-305c41958213@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]
Hi Phil,
Thanks for all the suggestions, I'll send patch v5 to fix all the issues.
Best Regards,
Tommy
On Thu, May 25, 2023 at 8:05 PM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:
> Hi Tommy,
>
> On 23/5/23 10:49, Tommy Wu wrote:
> > The watchdog timer is in the always-on domain device of HiFive 1 rev b,
> > so this patch added the AON device to the sifive_e machine. This patch
> > only implemented the functionality of the watchdog timer.
> >
> > Signed-off-by: Tommy Wu <tommy.wu@sifive.com>
> > Reviewed-by: Frank Chang <frank.chang@sifive.com>
> > ---
> > hw/misc/Kconfig | 3 +
> > hw/misc/meson.build | 1 +
> > hw/misc/sifive_e_aon.c | 326 +++++++++++++++++++++++++++++++++
> > include/hw/misc/sifive_e_aon.h | 60 ++++++
> > 4 files changed, 390 insertions(+)
> > create mode 100644 hw/misc/sifive_e_aon.c
> > create mode 100644 include/hw/misc/sifive_e_aon.h
>
>
> > +REG32(AON_WDT_WDOGCFG, 0x0)
> > + FIELD(AON_WDT_WDOGCFG, SCALE, 0, 4)
> > + FIELD(AON_WDT_WDOGCFG, RSVD0, 4, 4)
> > + FIELD(AON_WDT_WDOGCFG, RSTEN, 8, 1)
> > + FIELD(AON_WDT_WDOGCFG, ZEROCMP, 9, 1)
> > + FIELD(AON_WDT_WDOGCFG, RSVD1, 10, 2)
> > + FIELD(AON_WDT_WDOGCFG, EN_ALWAYS, 12, 1)
> > + FIELD(AON_WDT_WDOGCFG, EN_CORE_AWAKE, 13, 1)
> > + FIELD(AON_WDT_WDOGCFG, RSVD2, 14, 14)
> > + FIELD(AON_WDT_WDOGCFG, IP0, 28, 1)
> > + FIELD(AON_WDT_WDOGCFG, RSVD3, 29, 3)
> > +REG32(AON_WDT_WDOGCOUNT, 0x8)
>
> Adding:
>
> FIELD(AON_WDT_WDOGCOUNT, VALUE, 0, 31)
>
> ...
>
> > +REG32(AON_WDT_WDOGS, 0x10)
> > +REG32(AON_WDT_WDOGFEED, 0x18)
> > +REG32(AON_WDT_WDOGKEY, 0x1c)
> > +REG32(AON_WDT_WDOGCMP0, 0x20)
> > +
> > +static void sifive_e_aon_wdt_update_wdogcount(SiFiveEAONState *r)
> > +{
> > + int64_t now;
> > + if (0 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_ALWAYS) &&
> > + 0 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_CORE_AWAKE)) {
> > + return;
> > + }
> > +
> > + now = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + r->wdogcount += muldiv64(now - r->wdog_restart_time,
> > + r->wdogclk_freq, NANOSECONDS_PER_SECOND);
> > +
> > + /* Clean the most significant bit. */
> > + r->wdogcount = ((r->wdogcount << 1) >> 1);
>
> ... you could use:
>
> r->wdogcount &= R_AON_WDT_WDOGCOUNT_VALUE_MASK
>
> > + r->wdog_restart_time = now;
> > +}
> > +
> > +static void sifive_e_aon_wdt_update_state(SiFiveEAONState *r)
> > +{
> > + uint16_t wdogs;
> > + bool cmp_signal = false;
> > + sifive_e_aon_wdt_update_wdogcount(r);
> > + wdogs = (uint16_t)(r->wdogcount >>
> > + FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG,
> SCALE));
> > +
> > + if (wdogs >= r->wdogcmp0) {
> > + cmp_signal = true;
> > + if (1 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, ZEROCMP)) {
> > + r->wdogcount = 0;
> > + wdogs = 0;
> > + }
> > + }
> > +
> > + if (cmp_signal) {
> > + if (1 == FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, RSTEN)) {
>
> You sometimes check bit equality, ...
>
> > + watchdog_perform_action();
> > + }
> > + r->wdogcfg = FIELD_DP32(r->wdogcfg, AON_WDT_WDOGCFG, IP0, 1);
> > + }
> > +
> > + qemu_set_irq(r->wdog_irq, FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG,
> IP0));
> > +
> > + if (wdogs < r->wdogcmp0 &&
> > + (FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_ALWAYS) ||
>
> ... sometimes you don't. Code style consistency would be better.
>
> (Also, most of QEMU code base check equality using constant value
> on the right side of the comparaison).
>
> > + FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, EN_CORE_AWAKE))) {
> > + int64_t next = qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL);
> > + next += muldiv64((r->wdogcmp0 - wdogs) <<
> > + FIELD_EX32(r->wdogcfg, AON_WDT_WDOGCFG, SCALE),
> > + NANOSECONDS_PER_SECOND, r->wdogclk_freq);
> > + timer_mod(r->wdog_timer, next);
> > + } else {
> > + timer_mod(r->wdog_timer, INT64_MAX);
> > + }
> > +}
>
> > +static void sifive_e_aon_init(Object *obj)
> > +{
> > + SysBusDevice *sbd = SYS_BUS_DEVICE(obj);
> > + SiFiveEAONState *r = SIFIVE_E_AON(obj);
> > +
> > + memory_region_init_io(&r->mmio, OBJECT(r), &sifive_e_aon_ops, r,
> > + TYPE_SIFIVE_E_AON, SIFIVE_E_AON_MAX);
> > + sysbus_init_mmio(sbd, &r->mmio);
> > +
> > + /* watchdog timer */
> > + r->wdogclk_freq = SIFIVE_E_LFCLK_DEFAULT_FREQ;
> > + sysbus_init_irq(sbd, &r->wdog_irq);
> > +}
> > +
> > +static void sifive_e_aon_realize(DeviceState *dev, Error **errp)
> > +{
> > + SiFiveEAONState *r = SIFIVE_E_AON(dev);
> > +
> > + /* watchdog timer */
> > + r->wdog_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
> > + sifive_e_aon_wdt_expired_cb, r);
>
> You should be able to create the timer in sifive_e_aon_init().
>
> > +}
>
> Regards,
>
> Phil.
>
[-- Attachment #2: Type: text/html, Size: 6656 bytes --]
next prev parent reply other threads:[~2023-06-08 16:56 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-23 8:49 [PATCH v4 0/3] Implement the watchdog timer of HiFive 1 rev b Tommy Wu
2023-05-23 8:49 ` [PATCH v4 1/3] hw/misc: sifive_e_aon: Support " Tommy Wu
2023-05-25 12:05 ` Philippe Mathieu-Daudé
2023-06-08 16:56 ` Tommy Wu [this message]
2023-05-23 8:49 ` [PATCH v4 2/3] hw/riscv: sifive_e: " Tommy Wu
2023-05-25 2:42 ` Alistair Francis
2023-05-25 12:06 ` Philippe Mathieu-Daudé
2023-05-23 8:49 ` [PATCH v4 3/3] tests/qtest: sifive-e-aon-watchdog-test.c: Add QTest of watchdog of sifive_e Tommy Wu
2023-05-25 7:25 ` Thomas Huth
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CANj3q_kX2+XX7L=fJQ9DcOuXv2Ry5yrcfQTxVU662z8a2pk18Q@mail.gmail.com' \
--to=tommy.wu@sifive.com \
--cc=alistair.francis@wdc.com \
--cc=bin.meng@windriver.com \
--cc=frank.chang@sifive.com \
--cc=jim.shu@sifive.com \
--cc=liweiwei@iscas.ac.cn \
--cc=palmer@dabbelt.com \
--cc=philmd@linaro.org \
--cc=qemu-devel@nongnu.org \
--cc=qemu-riscv@nongnu.org \
--cc=thuth@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).