From: Jean-Christophe DUBOIS <jcd@tribudubois.net>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: QEMU Developers <qemu-devel@nongnu.org>, crosthwaite.peter@gmail.com
Subject: Re: [Qemu-devel] [PATCH 4/6] i.MX: Add i.MX6 System Reset Controller device.
Date: Sat, 6 Feb 2016 21:07:37 +0100 [thread overview]
Message-ID: <56B65289.2090103@tribudubois.net> (raw)
In-Reply-To: <CAFEAcA99m1cc7HY6Vd5iLH7aApHYJS7KO-JUWtcT--0xUiKyvw@mail.gmail.com>
Le 02/02/2016 17:46, Peter Maydell a écrit :
> On 26 January 2016 at 21:45, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> This controller is also present in i.MX5X devices but they are not
>> yet emulated by Qemu.
> QEMU is all-caps.
>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> ---
>> hw/misc/Makefile.objs | 1 +
>> hw/misc/imx6_src.c | 353 +++++++++++++++++++++++++++++++++++++++++++++
>> include/hw/misc/imx6_src.h | 72 +++++++++
>> 3 files changed, 426 insertions(+)
>> create mode 100644 hw/misc/imx6_src.c
>> create mode 100644 include/hw/misc/imx6_src.h
>>
>> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
>> index a2a8e91..6bac654 100644
>> --- a/hw/misc/Makefile.objs
>> +++ b/hw/misc/Makefile.objs
>> @@ -29,6 +29,7 @@ obj-$(CONFIG_IMX) += imx_ccm.o
>> obj-$(CONFIG_IMX) += imx31_ccm.o
>> obj-$(CONFIG_IMX) += imx25_ccm.o
>> obj-$(CONFIG_IMX) += imx6_ccm.o
>> +obj-$(CONFIG_IMX) += imx6_src.o
>> obj-$(CONFIG_MILKYMIST) += milkymist-hpdmc.o
>> obj-$(CONFIG_MILKYMIST) += milkymist-pfpu.o
>> obj-$(CONFIG_MAINSTONE) += mst_fpga.o
>> diff --git a/hw/misc/imx6_src.c b/hw/misc/imx6_src.c
>> new file mode 100644
>> index 0000000..d1be7c1
>> --- /dev/null
>> +++ b/hw/misc/imx6_src.c
>> @@ -0,0 +1,353 @@
>> +/*
>> + * IMX6 System Reset Controller
>> + *
>> + * Copyright (c) 2015 Jean-Christophe Dubois <jcd@tribudubois.net>
>> + *
>> + * This work is licensed under the terms of the GNU GPL, version 2 or later.
>> + * See the COPYING file in the top-level directory.
>> + *
>> + */
>> +
>> +#include "hw/misc/imx6_src.h"
>> +#include "sysemu/sysemu.h"
>> +#include "qemu/bitops.h"
>> +
>> +#ifndef DEBUG_IMX6_SRC
>> +#define DEBUG_IMX6_SRC 0
>> +#endif
>> +
>> +#define DPRINTF(fmt, args...) \
>> + do { \
>> + if (DEBUG_IMX6_SRC) { \
>> + fprintf(stderr, "[%s]%s: " fmt , TYPE_IMX6_SRC, \
>> + __func__, ##args); \
>> + } \
>> + } while (0)
>> +
>> +static char const *imx6_src_reg_name(uint32_t reg)
>> +{
>> + static char unknown[20];
>> +
>> + switch (reg) {
>> + case SRC_SCR:
>> + return "SRC_SCR";
>> + case SRC_SBMR1:
>> + return "SRC_SBMR1";
>> + case SRC_SRSR:
>> + return "SRC_SRSR";
>> + case SRC_SISR:
>> + return "SRC_SISR";
>> + case SRC_SIMR:
>> + return "SRC_SIMR";
>> + case SRC_SBMR2:
>> + return "SRC_SBMR2";
>> + case SRC_GPR1:
>> + return "SRC_GPR1";
>> + case SRC_GPR2:
>> + return "SRC_GPR2";
>> + case SRC_GPR3:
>> + return "SRC_GPR3";
>> + case SRC_GPR4:
>> + return "SRC_GPR4";
>> + case SRC_GPR5:
>> + return "SRC_GPR5";
>> + case SRC_GPR6:
>> + return "SRC_GPR6";
>> + case SRC_GPR7:
>> + return "SRC_GPR7";
>> + case SRC_GPR8:
>> + return "SRC_GPR8";
>> + case SRC_GPR9:
>> + return "SRC_GPR9";
>> + case SRC_GPR10:
>> + return "SRC_GPR10";
>> + default:
>> + sprintf(unknown, "%d ?", reg);
>> + return unknown;
>> + }
>> +}
>> +
>> +static const VMStateDescription vmstate_imx6_src = {
>> + .name = TYPE_IMX6_SRC,
>> + .version_id = 1,
>> + .minimum_version_id = 1,
>> + .fields = (VMStateField[]) {
>> + VMSTATE_UINT32_ARRAY(regs, IMX6SRCState, SRC_MAX),
>> + VMSTATE_END_OF_LIST()
>> + },
>> +};
>> +
>> +static void imx6_src_reset(DeviceState *dev)
>> +{
>> + IMX6SRCState *s = IMX6_SRC(dev);
>> +
>> + DPRINTF("\n");
>> +
>> + /*
>> + * We only clear the first registers as all GPR registers are preserved
>> + * over resets
>> + */
>> + memset(s->regs, 0, SRC_GPR1 * sizeof(uint32_t));
> Reset for a QEMU device means "full power cycle reset", so we should
> return the state to the same as if QEMU had just been started.
Is there any way to support the warm reset then?
>> +
>> + /* Set reset values */
>> + s->regs[SRC_SCR] = 0x521;
>> + s->regs[SRC_SRSR] = 0x1;
>> + s->regs[SRC_SIMR] = 0x1F;
>> +}
>> +
>> +static CPUState *imx6_src_get_cpu_by_id(uint32_t id)
>> +{
>> + CPUState *cpu;
>> +
>> + DPRINTF("cpu %d\n", id);
>> +
>> + CPU_FOREACH(cpu) {
>> + ARMCPU *armcpu = ARM_CPU(cpu);
>> +
>> + if (armcpu->mp_affinity == id) {
>> + return cpu;
>> + }
>> + }
>> +
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Resquesting unknown CPU %d\n",
>> + TYPE_IMX6_SRC, __func__, id);
>> +
>> + return NULL;
>> +}
>> +
>> +static void imx6_src_cpu_on(uint32_t cpuid, uint32_t entry, uint32_t context_id)
>> +{
>> + CPUState *target_cpu_state;
>> + ARMCPU *target_cpu;
>> + CPUClass *target_cpu_class;
>> +
>> + DPRINTF("cpu %d @ 0x%08x with R0 = 0x%08x\n", cpuid, entry, context_id);
>> +
>> + /* change to the cpu we are powering up */
>> + target_cpu_state = imx6_src_get_cpu_by_id(cpuid);
>> + if (!target_cpu_state) {
>> + return;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> + if (!target_cpu->powered_off) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: CPU %d is already running\n",
>> + TYPE_IMX6_SRC, __func__, cpuid);
>> + return;
>> + }
>> + target_cpu_class = CPU_GET_CLASS(target_cpu);
>> +
>> + /* Initialize the cpu we are turning on */
>> + cpu_reset(target_cpu_state);
>> + target_cpu->powered_off = false;
>> + target_cpu_state->halted = 0;
>> +
>> + target_cpu->env.regs[0] = context_id;
>> + target_cpu->env.thumb = entry & 1;
>> +
>> + target_cpu_class->set_pc(target_cpu_state, entry);
> This all looks pretty dangerous to me. Handling power-on of
> another CPU needs careful thought, especially since we're moving
> towards multi-threaded TCG, in which case the other CPU might
> in thory be really running.
In i.MX6 (and SOC using the SRC in general) all cores (except core 0
obviously) are initialized in "powered-off" mode. Therefore they should
not be running before this function is called.
Basically this code is just an adaptation of a similar code in
target-arm/psci.c.
Now it might not be the right way to to things (event if it does work)
and I would be thankful if you could point me in the right direction.
>
>> +}
>> +
>> +static void imx6_src_cpu_off(uint32_t cpuid)
>> +{
>> + CPUState *target_cpu_state;
>> + ARMCPU *target_cpu;
>> +
>> + DPRINTF("cpu %d\n", cpuid);
>> +
>> + /* change to the cpu we are powering up */
>> + target_cpu_state = imx6_src_get_cpu_by_id(cpuid);
>> + if (!target_cpu_state) {
>> + return;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> + if (target_cpu->powered_off) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: CPU %d is already off\n",
>> + TYPE_IMX6_SRC, __func__, cpuid);
>> + return;
>> + }
>> +
>> + target_cpu->powered_off = true;
>> + target_cpu_state->halted = 1;
>> + target_cpu_state->exception_index = EXCP_HLT;
>> + cpu_loop_exit(target_cpu_state);
>> +}
>> +
>> +static void imx6_src_cpu_reset(uint32_t cpuid)
>> +{
>> + CPUState *target_cpu_state;
>> + ARMCPU *target_cpu;
>> +
>> + DPRINTF("cpu %d\n", cpuid);
>> +
>> + /* change to the cpu we are powering up */
>> + target_cpu_state = imx6_src_get_cpu_by_id(cpuid);
>> + if (!target_cpu_state) {
>> + return;
>> + }
>> + target_cpu = ARM_CPU(target_cpu_state);
>> + if (target_cpu->powered_off) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: CPU %d is off\n",
>> + TYPE_IMX6_SRC, __func__, cpuid);
>> + return;
>> + }
>> +
>> + /* Reset the cpu we are turning on */
>> + cpu_reset(target_cpu_state);
>> +}
>> +
>> +static uint64_t imx6_src_read(void *opaque, hwaddr offset, unsigned size)
>> +{
>> + uint32 value = 0;
>> + IMX6SRCState *s = (IMX6SRCState *)opaque;
>> + uint32_t index = offset >> 2;
>> +
>> + if (index < SRC_MAX) {
>> + value = s->regs[index];
>> + } else {
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
>> + HWADDR_PRIx "\n", TYPE_IMX6_SRC, __func__, offset);
>> +
>> + }
>> +
>> + DPRINTF("reg[%s] => 0x%" PRIx32 "\n", imx6_src_reg_name(index), value);
>> +
>> + return (uint64_t)value;
>> +}
>> +
>> +static void imx6_src_write(void *opaque, hwaddr offset, uint64_t value,
>> + unsigned size)
>> +{
>> + IMX6SRCState *s = (IMX6SRCState *)opaque;
>> + uint32_t index = offset >> 2;
>> + uint64_t change_mask;
>> +
>> + if (index >= SRC_MAX) {
>> + qemu_log_mask(LOG_GUEST_ERROR, "[%s]%s: Bad register at offset 0x%"
>> + HWADDR_PRIx "\n", TYPE_IMX6_SRC, __func__, offset);
>> + return;
>> + }
>> +
>> + DPRINTF("reg[%s] <= 0x%" PRIx32 "\n", imx6_src_reg_name(index),
>> + (uint32_t)value);
>> +
>> + change_mask = s->regs[index] ^ (uint32_t)value;
>> +
>> + switch (index) {
>> + case SRC_SCR:
>> + if (EXTRACT(change_mask, CORE3_ENABLE)) {
>> + if (EXTRACT(value, CORE3_ENABLE)) {
>> + /* CORE 3 is brought up */
>> + imx6_src_cpu_on(3, s->regs[SRC_GPR7], s->regs[SRC_GPR8]);
>> + } else {
>> + /* CORE 3 is shut down */
>> + imx6_src_cpu_off(3);
>> + }
>> + /* We clear the reset bits as the processor chaged state */
> "changed"
>
>> +#define EXTRACT(value, name) (((value) >> name##_SHIFT) & name##_MASK)
> Again, please don't create local bit-manipulation functions or macros;
> use the ones in bitops.h.
>
> thanks
> -- PMM
>
next prev parent reply other threads:[~2016-02-06 20:07 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-01-26 21:45 [Qemu-devel] [PATCH 4/6] i.MX: Add i.MX6 System Reset Controller device Jean-Christophe Dubois
2016-02-02 16:46 ` Peter Maydell
2016-02-06 20:07 ` Jean-Christophe DUBOIS [this message]
2016-02-06 20:20 ` Peter Maydell
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=56B65289.2090103@tribudubois.net \
--to=jcd@tribudubois.net \
--cc=crosthwaite.peter@gmail.com \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/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).