Re: [Qemu-devel] [PATCH v2 6/9] i.MX: Add i.MX6 System Reset Controller device.

[Jean-Christophe DUBOIS <jcd@tribudubois.net>]

Hi Peter and Peter,

I need to test that the changes I did for PSCI (factor out on/off code) 
do not introduce any regression.

On which QEMU target should I test my changes to PSCI to check I didn't 
mess up anything?

Thanks

JC

Le 16/02/2016 16:35, Peter Maydell a écrit :
> On 8 February 2016 at 22:08, Jean-Christophe Dubois <jcd@tribudubois.net> wrote:
>> This controller is also present in i.MX5X devices but they are not
>> yet emulated by QEMU.
>>
>> Signed-off-by: Jean-Christophe Dubois <jcd@tribudubois.net>
>> @@ -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"
> #include "qemu/osdep.h" as first #include line.
>
>> +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_MAX * sizeof(uint32_t));
> Comment doesn't seem to match code?
>
>> +    /* 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);
>> +}
>> +
>> +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);
>> +}
> The code that is messing about with target CPUs to power them up
> and down needs to be abstracted out into a public API in target-arm/,
> so it can be used both by your device and by target-arm/psci.c.
> I don't want variations on this code to duplicate into various
> devices, because chances are good it will need to change for
> multithreaded TCG.
>
>> +static void imx6_src_realize(DeviceState *dev, Error **errp)
>> +{
>> +    IMX6SRCState *s = IMX6_SRC(dev);
>> +
>> +    memory_region_init_io(&s->iomem, OBJECT(dev), &imx6_src_ops, s,
>> +                          TYPE_IMX6_SRC, 0x1000);
>> +    sysbus_init_mmio(SYS_BUS_DEVICE(dev), &s->iomem);
>> +}
>> +
>> +static void imx6_src_class_init(ObjectClass *klass, void *data)
>> +{
>> +    DeviceClass *dc = DEVICE_CLASS(klass);
>> +
>> +    dc->realize = imx6_src_realize;
>> +    dc->reset = imx6_src_reset;
>> +    dc->vmsd = &vmstate_imx6_src;
>> +    dc->desc = "i.MX6 System Reset Controller";
>> +}
>> +/* SRC_SCR */
>> +#define CORE3_ENABLE_SHIFT     (24)
>> +#define CORE3_ENABLE_LENGTH    (1)
>> +#define CORE2_ENABLE_SHIFT     (23)
>> +#define CORE2_ENABLE_LENGTH    (1)
>> +#define CORE1_ENABLE_SHIFT     (22)
>> +#define CORE1_ENABLE_LENGTH    (1)
>> +#define CORE3_RST_SHIFT        (16)
>> +#define CORE3_RST_LENGTH       (1)
>> +#define CORE2_RST_SHIFT        (15)
>> +#define CORE2_RST_LENGTH       (1)
>> +#define CORE1_RST_SHIFT        (14)
>> +#define CORE1_RST_LENGTH       (1)
>> +#define CORE0_RST_SHIFT        (13)
>> +#define CORE0_RST_LENGTH       (1)
>> +#define SW_IPU1_RST_SHIFT      (3)
>> +#define SW_IPU1_RST_LENGTH     (1)
>> +#define SW_IPU2_RST_SHIFT      (12)
>> +#define SW_IPU2_RST_LENGTH     (1)
>> +#define WARM_RST_ENABLE_SHIFT  (0)
>> +#define WARM_RST_ENABLE_LENGTH (1)
> The brackets here are unnecessary.
>
> thanks
> -- PMM
>