[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Qemu-arm] [RFC PATCH] hw: arm: Add basic support for c
From: |
Philippe Mathieu-Daudé |
Subject: |
Re: [Qemu-devel] [Qemu-arm] [RFC PATCH] hw: arm: Add basic support for cprman (clock subsystem) |
Date: |
Wed, 10 Oct 2018 15:25:22 +0200 |
Hi Guenter,
On Tue, Jul 17, 2018 at 6:08 AM Guenter Roeck <address@hidden> wrote:
>
> On 07/16/2018 06:53 PM, Philippe Mathieu-Daudé wrote:
> > Hi Guenter,
> >
> > On 07/15/2018 07:06 PM, Guenter Roeck wrote:
> >> Add basic support for BCM283x CPRMAN. Provide support for reading and
> >> writing CPRMAN registers and initialize registers with sensible default
> >> values. During runtime retain any written values.
> >>
> >> Basic CPRMAN support is necessary and sufficient to boot Linux on raspi2
> >> and raspi3 systems.
> >>
> >> Signed-off-by: Guenter Roeck <address@hidden>
> >> ---
> >> I don't seriously expect this patch to get accepted, but I thought
> >> it might be valuable enough for others to use it when playing with
> >> raspi2 and raspi3 emulations.
> >
> > I've been working on a very similar patch... But due to soft-freeze I
> > postponed it.
> >
>
> > I don't feel very confident with my local work, as you, it is based on
> > looking at the Broadcom firmware [1] and how the Linux kernel initialize
> > the devices. I'll however compare with your work.
> >
>
> I'll be more than happy to drop my patch and go with yours instead.
> Let's just do that - it looks like it is much more comprehensive.
Did you make any progress with your work?
Your patch might be useful to solve the following issue:
https://bugs.launchpad.net/bugs/1779017
>
> > [1]: https://github.com/raspberrypi/firmware/tree/master/boot
> >
> >>
> >> hw/arm/bcm2835_peripherals.c | 15 +++++
> >> hw/misc/Makefile.objs | 1 +
> >> hw/misc/bcm2835_cprman.c | 126
> >> +++++++++++++++++++++++++++++++++++
> >> include/hw/arm/bcm2835_peripherals.h | 2 +
> >> include/hw/arm/raspi_platform.h | 1 +
> >> include/hw/misc/bcm2835_cprman.h | 28 ++++++++
> >> 6 files changed, 173 insertions(+)
> >> create mode 100644 hw/misc/bcm2835_cprman.c
> >> create mode 100644 include/hw/misc/bcm2835_cprman.h
> >>
> >> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
> >> index 6be7660..1a8993f 100644
> >> --- a/hw/arm/bcm2835_peripherals.c
> >> +++ b/hw/arm/bcm2835_peripherals.c
> >> @@ -85,6 +85,11 @@ static void bcm2835_peripherals_init(Object *obj)
> >> object_property_add_const_link(OBJECT(&s->property), "dma-mr",
> >> OBJECT(&s->gpu_bus_mr), &error_abort);
> >>
> >> + /* Clock subsystem */
> >> + object_initialize(&s->cprman, sizeof(s->cprman), TYPE_BCM2835_CPRMAN);
> >> + object_property_add_child(obj, "cprman", OBJECT(&s->cprman), NULL);
> >> + qdev_set_parent_bus(DEVICE(&s->cprman), sysbus_get_default());
> >> +
> >> /* Random Number Generator */
> >> object_initialize(&s->rng, sizeof(s->rng), TYPE_BCM2835_RNG);
> >> object_property_add_child(obj, "rng", OBJECT(&s->rng), NULL);
> >> @@ -244,6 +249,13 @@ static void bcm2835_peripherals_realize(DeviceState
> >> *dev, Error **errp)
> >> sysbus_connect_irq(SYS_BUS_DEVICE(&s->property), 0,
> >> qdev_get_gpio_in(DEVICE(&s->mboxes),
> >> MBOX_CHAN_PROPERTY));
> >>
> >> + /* Clock subsystem */
> >> + object_property_set_bool(OBJECT(&s->cprman), true, "realized", &err);
> >> + if (err) {
> >> + error_propagate(errp, err);
> >> + return;
> >> + }
> >> +
> >> /* Random Number Generator */
> >> object_property_set_bool(OBJECT(&s->rng), true, "realized", &err);
> >> if (err) {
> >> @@ -251,6 +263,9 @@ static void bcm2835_peripherals_realize(DeviceState
> >> *dev, Error **errp)
> >> return;
> >> }
> >>
> >> + memory_region_add_subregion(&s->peri_mr, CPRMAN_OFFSET,
> >> + sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->cprman), 0));
> >> +
> >> memory_region_add_subregion(&s->peri_mr, RNG_OFFSET,
> >> sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->rng), 0));
> >>
> >> diff --git a/hw/misc/Makefile.objs b/hw/misc/Makefile.objs
> >> index 9350900..ae5fc8a 100644
> >> --- a/hw/misc/Makefile.objs
> >> +++ b/hw/misc/Makefile.objs
> >> @@ -52,6 +52,7 @@ obj-$(CONFIG_OMAP) += omap_tap.o
> >> obj-$(CONFIG_RASPI) += bcm2835_mbox.o
> >> obj-$(CONFIG_RASPI) += bcm2835_property.o
> >> obj-$(CONFIG_RASPI) += bcm2835_rng.o
> >> +obj-$(CONFIG_RASPI) += bcm2835_cprman.o
> >> obj-$(CONFIG_SLAVIO) += slavio_misc.o
> >> obj-$(CONFIG_ZYNQ) += zynq_slcr.o
> >> obj-$(CONFIG_ZYNQ) += zynq-xadc.o
> >> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
> >> new file mode 100644
> >> index 0000000..4051f2b
> >> --- /dev/null
> >> +++ b/hw/misc/bcm2835_cprman.c
> >> @@ -0,0 +1,126 @@
> >> +/*
> >> + * BCM2835 Clock subsystem (poor man's version)
> >> + *
> >> + * Copyright (C) 2018 Guenter Roeck <address@hidden>
> >> + *
> >> + * 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 "qemu/osdep.h"
> >> +#include "qemu/log.h"
> >> +#include "qapi/error.h"
> >> +#include "crypto/random.h"
> >> +#include "hw/misc/bcm2835_cprman.h"
> >> +
> >> +static uint64_t bcm2835_cprman_read(void *opaque, hwaddr offset,
> >> + unsigned size)
> >> +{
> >> + BCM2835CprmanState *s = (BCM2835CprmanState *)opaque;
> >> + uint32_t res = 0;
> >> +
> >> + assert(size == 4);
> >> +
> >> + if (offset / 4 < CPRMAN_NUM_REGS) {
> >> + res = s->regs[offset / 4];
> >> + }
> >> +
> >> + return res;
> >> +}
> >
> > Yes, this read() works.
> >
> > A bit more verbose:
> >
> > static const char *names[64] = {
> > [0] = "GNRIC",
> > [1] = "VPU",
> > [2] = "SYS",
> > [3] = "PERIA",
> > [4] = "PERII",
> > [5] = "H264",
> > [6] = "ISP",
> > [7] = "V3D",
> > [8] = "CAM0",
> > [9] = "CAM1",
> > [10] = "CCP2",
> > [11] = "DSI0E",
> > [12] = "DSI0P",
> > [13] = "DPI",
> > [14] = "GP0",
> > [15] = "GP1",
> > [16] = "GP2",
> > [17] = "HSM",
> > [18] = "OTP",
> > [19] = "PCM",
> > [20] = "PWM",
> > [21] = "SLIM",
> > [22] = "SMI",
> > [24] = "TCNT",
> > [25] = "TEC",
> > [26] = "TD0",
> > [27] = "TD1",
> > [28] = "TSENS",
> > [29] = "TIMER",
> > [30] = "UART",
> > [31] = "VEC",
> > [50] = "PULSE",
> > [53] = "SDC",
> > [54] = "ARM",
> > [55] = "AVEO",
> > [56] = "EMMC",
> > };
> >
> > static uint64_t bcm2835_cprman_read(void *opaque, hwaddr offset,
> > unsigned size)
> > {
> > BCM2835CprmanState *s = (BCM2835CprmanState *)opaque;
> > int idx = addr / 8;
> > bool div = addr % 8;
> > const char *name = names[idx] ? names[idx] : "UNKN";
> > uint64_t value = s->regs[addr >> 2];
> >
> > switch (addr) {
> > case 0 ... 0x100:
> > qemu_log_mask(LOG_UNIMP, "[CM]: %s unimp r%02d 0x%04"
> > HWADDR_PRIx " = 0x%"PRIx64 " (%s)\n",
> > div ? "DIV" : "CTL", size << 3, addr, value, name);
> > break;
> > case 0x104 ... 0x114:
> > qemu_log_mask(LOG_UNIMP, "[CM]: PLL? unimp r%02d 0x%04"
> > HWADDR_PRIx " = 0x%"PRIx64 "\n",
> > size << 3, addr, value);
> > value = -1; /* FIXME PLL lock? */
> > break;
> > }
> > return value;
> > }
> >
> >> +
> >> +#define CM_PASSWORD 0x5a000000
> >> +#define CM_PASSWORD_MASK 0xff000000
> >> +
> >> +static void bcm2835_cprman_write(void *opaque, hwaddr offset,
> >> + uint64_t value, unsigned size)
> >> +{
> >> + BCM2835CprmanState *s = (BCM2835CprmanState *)opaque;
> >> +
> >> + assert(size == 4);
> >
> > Instead of this assert() ...
> >
> >> +
> >> + if ((value & 0xff000000) == CM_PASSWORD &&
> >> + offset / 4 < CPRMAN_NUM_REGS)
> >> + s->regs[offset / 4] = value & ~CM_PASSWORD_MASK;
> >> +}
> >> +
> >> +static const MemoryRegionOps bcm2835_cprman_ops = {
> >> + .read = bcm2835_cprman_read,
> >> + .write = bcm2835_cprman_write,
> >
> > ... I used:
> >
> > .impl.min_access_size = 4,
> > .valid.min_access_size = 4,
> >
> >> + .endianness = DEVICE_NATIVE_ENDIAN,
> >> +};
> >
> > Same verbose write():
> >
> > static void bcm2835_cprman_write(void *opaque, hwaddr offset,
> > uint64_t value, unsigned size)
> > {
> > BCM2835CprmanState *s = (BCM2835CprmanState *)opaque;
> > int idx = addr / 8;
> > bool div = addr % 8;
> > const char *name = names[idx] ? names[idx] : "UNKN";
> >
> > if (extract64(value, 24, 8) != CM_KEY) {
> > qemu_log_mask(LOG_GUEST_ERROR, "[CM]: %s error w%02d *0x%04"
> > HWADDR_PRIx " = 0x%" PRIx64 " (%s)\n",
> > div ? "DIV" : "CTL", size << 3, addr, value, name);
> > return;
> > }
> > s->regs[addr >> 2] = extract64(value, 0, 24);
> > switch (addr) {
> > case 0 ... 0x100:
> > if (div) {
> > qemu_log_mask(LOG_UNIMP, "[CM]: DIV unimp w%02d *0x%04"
> > HWADDR_PRIx " = 0x%" PRIx64 " (%s) %" PRIu64 ".%" PRIu64 "\n",
> > size << 3, addr, value, name, extract64(value,
> > 12, 12), 1000 * extract64(value, 0, 12) / 1024);
> > } else {
> > qemu_log_mask(LOG_UNIMP, "[CM]: CTL unimp w%02d *0x%04"
> > HWADDR_PRIx " = 0x%" PRIx64 " (%s) src=%" PRIu64 " ena:%u\n",
> > size << 3, addr, value, name, value & 0xf,
> > !!(value & (1 << 4)));
> > }
> > break;
> > case 0x104 ... 0x114:
> > qemu_log_mask(LOG_UNIMP, "[CM]: PLL? unimp w%02d *0x%04"
> > HWADDR_PRIx " = 0x%" PRIx64 "\n",
> > size << 3, addr, value);
> > break;
> > }
> > }
> >
> >> +
> >> +static const VMStateDescription vmstate_bcm2835_cprman = {
> >> + .name = TYPE_BCM2835_CPRMAN,
> >> + .version_id = 1,
> >> + .minimum_version_id = 1,
> >> + .fields = (VMStateField[]) {
> >> + VMSTATE_UINT32_ARRAY(regs, BCM2835CprmanState, CPRMAN_NUM_REGS),
> >> + VMSTATE_END_OF_LIST()
> >> + }
> >> +};
> >> +
> >> +static void bcm2835_cprman_init(Object *obj)
> >> +{
> >> + BCM2835CprmanState *s = BCM2835_CPRMAN(obj);
> >> +
> >> + memory_region_init_io(&s->iomem, obj, &bcm2835_cprman_ops, s,
> >> + TYPE_BCM2835_CPRMAN, 0x2000);
> >> + sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> >> +}
> >> +
> >> +#define CM_GNRICCTL (0x000 / 4)
> >> +#define CM_VECCTL (0x0f8 / 4)
> >> +#define CM_DFTCTL (0x168 / 4)
> >> +#define CM_EMMCCTL (0x1c0 / 4)
> >
> >> +#define A2W_PLLA_CTRL (0x1100 / 4)
> >> +#define A2W_PLLB_CTRL (0x11e0 / 4)
> >
> > The A2W is another device you don't need to worry about, just use add a
> > 0x1000 UNIMP device.
> >
>
> I thought it was needed to get a non-zero divider, but my memory may defeat
> me.
Hmm I never hit this, but you might be using more recent
firmwares/kernels than the ones I tried, which might now access this.
>
> Guenter
>
> >> +
> >> +static void bcm2835_cprman_reset(DeviceState *dev)
> >> +{
> >> + BCM2835CprmanState *s = BCM2835_CPRMAN(dev);
> >> + int i;
> >> +
> >> + /*
> >> + * Available information suggests that CPRMAN registers have default
> >> + * values which are not overwritten by ROMMON (u-boot). The hardware
> >> + * default values are unknown at this time.
> >> + * The default values selected here are necessary and sufficient
> >> + * to boot Linux directly (on raspi2 and raspi3). The selected
> >> + * values enable all clocks and set clock rates to match their
> >> + * parent rates.
> >> + */
> >
> > Lucky you...
> >
> >> + for (i = CM_GNRICCTL; i <= CM_VECCTL; i += 2) {
> >> + s->regs[i] = 0x11;
> >> + s->regs[i + 1] = 0x1000;
> >> + }
> >> + for (i = CM_DFTCTL; i <= CM_EMMCCTL; i += 2) {
> >> + s->regs[i] = 0x11;
> >> + s->regs[i + 1] = 0x1000;
> >> + }
> >> + for (i = A2W_PLLA_CTRL; i <= A2W_PLLB_CTRL; i += 8) {
> >> + s->regs[i] = 0x10001;
> >> + }
> >> +}
> >
> > ... I remember having headaches with this CRAP reset():
> >
> > static void bcm2836_enable_clk(BCM2835ClockState *s, int clk_id, int
> > clk_src, int div_int, int div_frac)
> > {
> > s->regs[2 * clk_id + 0] = (/* MASH */1 << 9) | (1 << 4) | (clk_src
> > << 0);
> > s->regs[2 * clk_id + 1] = (div_int << 12) | (div_frac << 0);
> > }
> >
> > static void bcm2836_cm_reset(DeviceState *d)
> > {
> > BCM2835ClockState *s = BCM2835_CLOCK(d);
> >
> > int osc_freq_hz = 2400000; /* TODO property */
> > int baudrate = 115200; /* TODO property */
> > int vpu_fracbits = 12; /* TODO property */
> > div_t qi = div(osc_freq_hz, baudrate);
> > div_t qf = div(qi.rem << vpu_fracbits, baudrate);
> >
> > bcm2836_enable_clk(s, 1 /* VPU */, 1 /* OSC */, qi.quot, qf.quot);
> > bcm2836_enable_clk(s, 1 /* VPU */, 1 /* OSC */, 1, 0);
> > }
> >
> > I can't say it is correct until I reopen this branch (September?).
... November? ...
> >
> >> +
> >> +static void bcm2835_cprman_class_init(ObjectClass *klass, void *data)
> >> +{
> >> + DeviceClass *dc = DEVICE_CLASS(klass);
> >> +
> >> + dc->reset = bcm2835_cprman_reset;
> >> + dc->vmsd = &vmstate_bcm2835_cprman;
> >> +}
> >> +
> >> +static TypeInfo bcm2835_cprman_info = {
> >> + .name = TYPE_BCM2835_CPRMAN,
> >> + .parent = TYPE_SYS_BUS_DEVICE,
> >> + .instance_size = sizeof(BCM2835CprmanState),
> >> + .class_init = bcm2835_cprman_class_init,
> >> + .instance_init = bcm2835_cprman_init,
> >> +};
> >> +
> >> +static void bcm2835_cprman_register_types(void)
> >> +{
> >> + type_register_static(&bcm2835_cprman_info);
> >> +}
> >> +
> >> +type_init(bcm2835_cprman_register_types)
> >> diff --git a/include/hw/arm/bcm2835_peripherals.h
> >> b/include/hw/arm/bcm2835_peripherals.h
> >> index f5b193f..f9f53e3 100644
> >> --- a/include/hw/arm/bcm2835_peripherals.h
> >> +++ b/include/hw/arm/bcm2835_peripherals.h
> >> @@ -19,6 +19,7 @@
> >> #include "hw/intc/bcm2835_ic.h"
> >> #include "hw/misc/bcm2835_property.h"
> >> #include "hw/misc/bcm2835_rng.h"
> >> +#include "hw/misc/bcm2835_cprman.h"
> >> #include "hw/misc/bcm2835_mbox.h"
> >> #include "hw/sd/sdhci.h"
> >> #include "hw/sd/bcm2835_sdhost.h"
> >> @@ -44,6 +45,7 @@ typedef struct BCM2835PeripheralState {
> >> BCM2835ICState ic;
> >> BCM2835PropertyState property;
> >> BCM2835RngState rng;
> >> + BCM2835CprmanState cprman;
> >> BCM2835MboxState mboxes;
> >> SDHCIState sdhci;
> >> BCM2835SDHostState sdhost;
> >> diff --git a/include/hw/arm/raspi_platform.h
> >> b/include/hw/arm/raspi_platform.h
> >> index 6467e88..412d010 100644
> >> --- a/include/hw/arm/raspi_platform.h
> >> +++ b/include/hw/arm/raspi_platform.h
> >> @@ -36,6 +36,7 @@
> >> * Doorbells &
> >> Mailboxes */
> >> #define PM_OFFSET 0x100000 /* Power Management, Reset
> >> controller
> >> * and Watchdog registers */
> >> +#define CPRMAN_OFFSET 0x101000
> >> #define PCM_CLOCK_OFFSET 0x101098
> >> #define RNG_OFFSET 0x104000
> >> #define GPIO_OFFSET 0x200000
> >> diff --git a/include/hw/misc/bcm2835_cprman.h
> >> b/include/hw/misc/bcm2835_cprman.h
> >> new file mode 100644
> >> index 0000000..db250b3
> >> --- /dev/null
> >> +++ b/include/hw/misc/bcm2835_cprman.h
> >> @@ -0,0 +1,28 @@
> >> +/*
> >> + * BCM2835 Poor-man's version of CPRMAN
> >> + *
> >> + * Copyright (C) 2018 Guenter Roeck <address@hidden>
> >> + *
> >> + * This work is licensed under the terms of the GNU GPL, version 2 or
> >> later.
> >> + * See the COPYING file in the top-level directory.
> >> + */
> >> +
> >> +#ifndef BCM2835_CPRMAN_H
> >> +#define BCM2835_CPRMAN_H
> >> +
> >> +#include "hw/sysbus.h"
> >> +
> >> +#define TYPE_BCM2835_CPRMAN "bcm2835-cprman"
> >> +#define BCM2835_CPRMAN(obj) \
> >> + OBJECT_CHECK(BCM2835CprmanState, (obj), TYPE_BCM2835_CPRMAN)
> >> +
> >> +#define CPRMAN_NUM_REGS (0x1200 / 4)
> >
> > I have 64 registers (64-bit wide, see below), so the region is 0x200 B.
> >
> >> +
> >> +typedef struct {
> >> + SysBusDevice busdev;
> >> + MemoryRegion iomem;
> >> +
> >> + uint32_t regs[CPRMAN_NUM_REGS];
> >
> > I used uint64_t for registers.
> >
> >> +} BCM2835CprmanState;
> >> +
> >> +#endif
> >>
> >
> > Regards,
> >
> > Phil.
> >
>
- Re: [Qemu-devel] [Qemu-arm] [RFC PATCH] hw: arm: Add basic support for cprman (clock subsystem),
Philippe Mathieu-Daudé <=