From: "Philippe Mathieu-Daudé" <f4bug@amsat.org>
To: Luc Michel <luc@lmichel.fr>
Cc: Peter Maydell <peter.maydell@linaro.org>,
qemu-arm@nongnu.org, qemu-devel@nongnu.org,
Andrew Baumann <Andrew.Baumann@microsoft.com>
Subject: Re: [PATCH 04/14] hw/arm/raspi: add a skeleton implementation of the cprman
Date: Fri, 2 Oct 2020 16:37:04 +0200 [thread overview]
Message-ID: <4aa9f0c3-dc4b-1e87-d601-87b0498de8b1@amsat.org> (raw)
In-Reply-To: <20200928084515.r7s3cl6jlzm465iw@sekoia-pc.home.lmichel.fr>
On 9/28/20 10:45 AM, Luc Michel wrote:
> On 23:05 Sat 26 Sep , Philippe Mathieu-Daudé wrote:
>> On 9/25/20 12:17 PM, Luc Michel wrote:
>>> The BCM2835 cprman is the clock manager of the SoC. It is composed of a
>>
>> Can we use CPRMAN in caps?
>>
>>> main oscillator, and several sub-components (PLLs, multiplexers, ...) to
>>> generate the BCM2835 clock tree.
>>>
>>> This commit adds a skeleton of the cprman, with a dummy register
>>> read/write implementation. It embeds the main oscillator (xosc) from
>>> which all the clocks will be derived.
>>>
>>> Signed-off-by: Luc Michel <luc@lmichel.fr>
>>> ---
>>> include/hw/arm/bcm2835_peripherals.h | 3 +-
>>> include/hw/misc/bcm2835_cprman.h | 37 +++++
>>> include/hw/misc/bcm2835_cprman_internals.h | 24 +++
>>> hw/arm/bcm2835_peripherals.c | 11 +-
>>> hw/misc/bcm2835_cprman.c | 171 +++++++++++++++++++++
>>> hw/misc/meson.build | 1 +
>>> hw/misc/trace-events | 5 +
>>> 7 files changed, 250 insertions(+), 2 deletions(-)
>>> create mode 100644 include/hw/misc/bcm2835_cprman.h
>>> create mode 100644 include/hw/misc/bcm2835_cprman_internals.h
>>> create mode 100644 hw/misc/bcm2835_cprman.c
>>>
>>> diff --git a/include/hw/arm/bcm2835_peripherals.h b/include/hw/arm/bcm2835_peripherals.h
>>> index 199088425a..002bb5e73b 100644
>>> --- a/include/hw/arm/bcm2835_peripherals.h
>>> +++ b/include/hw/arm/bcm2835_peripherals.h
>>> @@ -21,10 +21,11 @@
>>> #include "hw/misc/bcm2835_property.h"
>>> #include "hw/misc/bcm2835_rng.h"
>>> #include "hw/misc/bcm2835_mbox.h"
>>> #include "hw/misc/bcm2835_mphi.h"
>>> #include "hw/misc/bcm2835_thermal.h"
>>> +#include "hw/misc/bcm2835_cprman.h"
>>> #include "hw/sd/sdhci.h"
>>> #include "hw/sd/bcm2835_sdhost.h"
>>> #include "hw/gpio/bcm2835_gpio.h"
>>> #include "hw/timer/bcm2835_systmr.h"
>>> #include "hw/usb/hcd-dwc2.h"
>>> @@ -45,11 +46,11 @@ struct BCM2835PeripheralState {
>>>
>>> BCM2835SystemTimerState systmr;
>>> BCM2835MphiState mphi;
>>> UnimplementedDeviceState armtmr;
>>> UnimplementedDeviceState powermgt;
>>> - UnimplementedDeviceState cprman;
>>> + BCM2835CprmanState cprman;
>>> PL011State uart0;
>>> BCM2835AuxState aux;
>>> BCM2835FBState fb;
>>> BCM2835DMAState dma;
>>> BCM2835ICState ic;
>>> diff --git a/include/hw/misc/bcm2835_cprman.h b/include/hw/misc/bcm2835_cprman.h
>>> new file mode 100644
>>> index 0000000000..de9bd01b23
>>> --- /dev/null
>>> +++ b/include/hw/misc/bcm2835_cprman.h
>>> @@ -0,0 +1,37 @@
>>> +/*
>>> + * BCM2835 cprman clock manager
>>> + *
>>> + * Copyright (c) 2020 Luc Michel <luc@lmichel.fr>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef HW_MISC_CPRMAN_H
>>> +#define HW_MISC_CPRMAN_H
>>> +
>>> +#include "hw/sysbus.h"
>>> +#include "hw/qdev-clock.h"
>>> +
>>> +#define TYPE_BCM2835_CPRMAN "bcm2835-cprman"
>>> +
>>> +typedef struct BCM2835CprmanState BCM2835CprmanState;
>>> +
>>> +DECLARE_INSTANCE_CHECKER(BCM2835CprmanState, CPRMAN,
>>> + TYPE_BCM2835_CPRMAN)
>>> +
>>> +#define CPRMAN_NUM_REGS (0x2000 / sizeof(uint32_t))
>>> +
>>> +struct BCM2835CprmanState {
>>> + /*< private >*/
>>> + SysBusDevice parent_obj;
>>> +
>>> + /*< public >*/
>>> + MemoryRegion iomem;
>>> +
>>> + uint32_t regs[CPRMAN_NUM_REGS];
>>> + uint32_t xosc_freq;
>>> +
>>> + Clock *xosc;
Isn't it xosc external to the CPRMAN?
>>> +};
>>> +
>>> +#endif
>>> diff --git a/include/hw/misc/bcm2835_cprman_internals.h b/include/hw/misc/bcm2835_cprman_internals.h
>>> new file mode 100644
>>> index 0000000000..6a10b60930
>>> --- /dev/null
>>> +++ b/include/hw/misc/bcm2835_cprman_internals.h
>>> @@ -0,0 +1,24 @@
>>> +/*
>>> + * BCM2835 cprman clock manager
>>> + *
>>> + * Copyright (c) 2020 Luc Michel <luc@lmichel.fr>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +#ifndef HW_MISC_CPRMAN_INTERNALS_H
>>> +#define HW_MISC_CPRMAN_INTERNALS_H
>>> +
>>> +#include "hw/registerfields.h"
>>> +#include "hw/misc/bcm2835_cprman.h"
>>> +
>>> +/* Register map */
>>> +
>>> +/*
>>> + * This field is common to all registers. Each register write value must match
>>> + * the CPRMAN_PASSWORD magic value in its 8 MSB.
>>> + */
>>> +FIELD(CPRMAN, PASSWORD, 24, 8)
>>> +#define CPRMAN_PASSWORD 0x5a
>>
>> s/PASSWORD/KEY/?
> I'm not a big fan of this name either, but this is actually how this
> field is named in the datasheet and in various sources.
>
> See:
> - https://www.raspberrypi.org/app/uploads/2012/02/BCM2835-ARM-Peripherals.pdf
> (page 107)
> - https://elinux.org/BCM2835_registers#CM (auto-generated from
> Broadcom GPU related code)
> - The Linux driver also use this name FWIW.
OK.
>>
>>> +
>>> +#endif
>>> diff --git a/hw/arm/bcm2835_peripherals.c b/hw/arm/bcm2835_peripherals.c
>>> index f0802c91e0..958aadeeb9 100644
>>> --- a/hw/arm/bcm2835_peripherals.c
>>> +++ b/hw/arm/bcm2835_peripherals.c
>>> @@ -119,10 +119,13 @@ static void bcm2835_peripherals_init(Object *obj)
>>> object_initialize_child(obj, "mphi", &s->mphi, TYPE_BCM2835_MPHI);
>>>
>>> /* DWC2 */
>>> object_initialize_child(obj, "dwc2", &s->dwc2, TYPE_DWC2_USB);
>>>
>>> + /* CPRMAN clock manager */
>>> + object_initialize_child(obj, "cprman", &s->cprman, TYPE_BCM2835_CPRMAN);
>>> +
>>> object_property_add_const_link(OBJECT(&s->dwc2), "dma-mr",
>>> OBJECT(&s->gpu_bus_mr));
>>> }
>>>
>>> static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>>> @@ -158,10 +161,17 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>>> /* Interrupt Controller */
>>> if (!sysbus_realize(SYS_BUS_DEVICE(&s->ic), errp)) {
>>> return;
>>> }
>>>
>>> + /* CPRMAN clock manager */
>>> + if (!sysbus_realize(SYS_BUS_DEVICE(&s->cprman), 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, ARMCTRL_IC_OFFSET,
>>> sysbus_mmio_get_region(SYS_BUS_DEVICE(&s->ic), 0));
>>> sysbus_pass_irq(SYS_BUS_DEVICE(s), SYS_BUS_DEVICE(&s->ic));
>>>
>>> /* Sys Timer */
>>> @@ -343,11 +353,10 @@ static void bcm2835_peripherals_realize(DeviceState *dev, Error **errp)
>>> qdev_get_gpio_in_named(DEVICE(&s->ic), BCM2835_IC_GPU_IRQ,
>>> INTERRUPT_USB));
>>>
>>> create_unimp(s, &s->armtmr, "bcm2835-sp804", ARMCTRL_TIMER0_1_OFFSET, 0x40);
>>> create_unimp(s, &s->powermgt, "bcm2835-powermgt", PM_OFFSET, 0x114);
>>> - create_unimp(s, &s->cprman, "bcm2835-cprman", CPRMAN_OFFSET, 0x2000);
>>> create_unimp(s, &s->i2s, "bcm2835-i2s", I2S_OFFSET, 0x100);
>>> create_unimp(s, &s->smi, "bcm2835-smi", SMI_OFFSET, 0x100);
>>> create_unimp(s, &s->spi[0], "bcm2835-spi0", SPI0_OFFSET, 0x20);
>>> create_unimp(s, &s->bscsl, "bcm2835-spis", BSC_SL_OFFSET, 0x100);
>>> create_unimp(s, &s->i2c[0], "bcm2835-i2c0", BSC0_OFFSET, 0x20);
>>> diff --git a/hw/misc/bcm2835_cprman.c b/hw/misc/bcm2835_cprman.c
>>> new file mode 100644
>>> index 0000000000..d2ea0c9236
>>> --- /dev/null
>>> +++ b/hw/misc/bcm2835_cprman.c
>>> @@ -0,0 +1,171 @@
>>> +/*
>>> + * BCM2835 cprman clock manager
>>> + *
>>> + * Copyright (c) 2020 Luc Michel <luc@lmichel.fr>
>>> + *
>>> + * SPDX-License-Identifier: GPL-2.0-or-later
>>> + */
>>> +
>>> +/*
>>> + * This peripheral is roughly divided into 3 main parts:
>>> + * - the PLLs
>>> + * - the PLL channels
>>> + * - the clock muxes
>>> + *
>>> + * A main oscillator (xosc) feeds all the PLLs. Each PLLs has one or more
>>> + * channels. Those channel are then connected to the clock muxes. Each mux has
>>> + * multiples sources (usually the xosc, some of the PLL channels and some "test
>>> + * debug" clocks). It can selects one or the other through a control register.
>>
>> "It" is unclear (to me) in this sentence. Assuming the mux.
>>
>>> + * Each mux has one output clock that also goes out of the CPRMAN. It usually
>>
>> Here is "It" the mux or the output clock? Assuming the mux.
>>
>>> + * connects to another peripheral in the SoC (so a given mux is dedicated to a
>>> + * peripheral).
>>> + *
>>> + * At each level (PLL, channel and mux), the clock can be altered through
>>> + * dividers (and multipliers in case of the PLLs), and can be disabled (in this
>>> + * case, the next levels see no clock).
>>> + *
>>> + * This can be sum-up as follows (this is an example and not the actual BCM2835
>>> + * clock tree):
>>> + *
>>> + * /-->[PLL]-|->[PLL channel]--... [mux]--> to peripherals
>>> + * | |->[PLL channel] muxes takes [mux]
>>> + * | \->[PLL channel] inputs from [mux]
>>> + * | some channels [mux]
>>> + * [xosc]---|-->[PLL]-|->[PLL channel] and other srcs [mux]
>>> + * | \->[PLL channel] ...-->[mux]
>>> + * | [mux]
>>> + * \-->[PLL]--->[PLL channel] [mux]
>>> + *
>>> + * The page at https://elinux.org/The_Undocumented_Pi gives the actual clock
>>> + * tree configuration.
>>> + */
>>> +
>>> +#include "qemu/osdep.h"
>>> +#include "qemu/log.h"
>>> +#include "migration/vmstate.h"
>>> +#include "hw/qdev-properties.h"
>>> +#include "hw/misc/bcm2835_cprman.h"
>>> +#include "hw/misc/bcm2835_cprman_internals.h"
>>> +#include "trace.h"
>>> +
>>> +/* CPRMAN "top level" model */
>>> +
>>> +static uint64_t cprman_read(void *opaque, hwaddr offset,
>>> + unsigned size)
>>
>> Indent off.
>>
>>> +{
>>> + BCM2835CprmanState *s = CPRMAN(opaque);
>>> + uint64_t r = 0;
>>> + size_t idx = offset / sizeof(uint32_t);
>>> +
>>> + switch (idx) {
>>> + default:
>>> + r = s->regs[idx];
>>> + }
>>> +
>>> + trace_bcm2835_cprman_read(offset, r);
>>> + return r;
>>> +}
>>> +
>>> +static void cprman_write(void *opaque, hwaddr offset,
>>> + uint64_t value, unsigned size)
>>> +{
>>> + BCM2835CprmanState *s = CPRMAN(opaque);
>>> + size_t idx = offset / sizeof(uint32_t);
>>> +
>>> + if (FIELD_EX32(value, CPRMAN, PASSWORD) != CPRMAN_PASSWORD) {
>>> + trace_bcm2835_cprman_write_invalid_magic(offset, value);
>>> + return;
>>> + }
>>> +
>>> + value &= ~R_CPRMAN_PASSWORD_MASK;
>>> +
>>> + trace_bcm2835_cprman_write(offset, value);
>>> + s->regs[idx] = value;
>>> +
>>> +}
>>> +
>>> +static const MemoryRegionOps cprman_ops = {
>>> + .read = cprman_read,
>>> + .write = cprman_write,
>>> + .endianness = DEVICE_LITTLE_ENDIAN,
>>> + .valid = {
>>> + .min_access_size = 4,
>>> + .max_access_size = 4,
>>
>> I couldn't find this in the public datasheets (any pointer?).
>>
>> Since your implementation is 32bit, can you explicit .impl
>> min/max = 4?
>
> I could not find this information either, but I assumed this is the
> case, mainly because of the 'PASSWORD' field in all registers.
Good point. Do you mind adding a comment about it here please?
>
> Regarding .impl, I thought that having .valid was enough?
Until we eventually figure out we can do 64-bit accesses,
so someone change .valid.max to 8 and your model is broken :/
>
>>
>>> + .unaligned = false,
>>> + },
>>> +};
>>> +
>>> +static void cprman_reset(DeviceState *dev)
>>> +{
>>> + BCM2835CprmanState *s = CPRMAN(dev);
>>> +
>>> + memset(s->regs, 0, sizeof(s->regs));
>>> +
>>> + clock_update_hz(s->xosc, s->xosc_freq);
>>> +}
>>> +
>>> +static Clock *init_internal_clock(BCM2835CprmanState *s,
>>> + const char *name)
>>
>> Interesting. Shouldn't this be a public function from the
>> Clock API? (Taking Object + name arguments)?
>>
> Yes, I hesitated to do just that. I'll add a patch for it when I re-roll.
>>> +{
>>> + Object *obj;
>>> + Clock *clk;
>>> +
>>> + obj = object_new(TYPE_CLOCK);
>>> + object_property_add_child(OBJECT(s), name, obj);
>>> + object_unref(obj);
>>> +
>>> + clk = CLOCK(obj);
>>> + clock_setup_canonical_path(clk);
>>> +
>>> + return clk;
>>> +}
>>> +
>>> +static void cprman_init(Object *obj)
>>> +{
>>> + BCM2835CprmanState *s = CPRMAN(obj);
>>> +
>>> + s->xosc = init_internal_clock(s, "xosc");
>>> +
>>> + memory_region_init_io(&s->iomem, obj, &cprman_ops,
>>> + s, "bcm2835-cprman", 0x2000);
>>
>> Again assuming this is a 8KB MMIO device:
>>
>> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> Thanks!
>
next prev parent reply other threads:[~2020-10-02 14:38 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-25 10:17 [PATCH 00/14] raspi: add the bcm2835 cprman clock manager Luc Michel
2020-09-25 10:17 ` [PATCH 01/14] hw/core/clock: provide the VMSTATE_ARRAY_CLOCK macro Luc Michel
2020-09-26 20:36 ` Philippe Mathieu-Daudé
2020-09-28 8:38 ` Damien Hedde
2020-09-25 10:17 ` [PATCH 02/14] hw/core/clock: trace clock values in Hz instead of ns Luc Michel
2020-09-26 20:36 ` Philippe Mathieu-Daudé
2020-09-28 8:42 ` Damien Hedde
2020-09-25 10:17 ` [PATCH 03/14] hw/arm/raspi: fix cprman base address Luc Michel
2020-09-26 21:04 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 04/14] hw/arm/raspi: add a skeleton implementation of the cprman Luc Michel
2020-09-26 21:05 ` Philippe Mathieu-Daudé
2020-09-28 8:45 ` Luc Michel
2020-10-02 14:37 ` Philippe Mathieu-Daudé [this message]
2020-10-03 11:54 ` Luc Michel
2020-10-03 18:14 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 05/14] hw/misc/bcm2835_cprman: add a PLL skeleton implementation Luc Michel
2020-09-26 21:17 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 06/14] hw/misc/bcm2835_cprman: implement PLLs behaviour Luc Michel
2020-09-26 21:26 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 07/14] hw/misc/bcm2835_cprman: add a PLL channel skeleton implementation Luc Michel
2020-09-26 21:32 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 08/14] hw/misc/bcm2835_cprman: implement PLL channels behaviour Luc Michel
2020-09-26 21:36 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 09/14] hw/misc/bcm2835_cprman: add a clock mux skeleton implementation Luc Michel
2020-10-02 14:42 ` Philippe Mathieu-Daudé
2020-10-02 15:34 ` Philippe Mathieu-Daudé
2020-10-04 19:34 ` Luc Michel
2020-10-04 20:17 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 10/14] hw/misc/bcm2835_cprman: implement clock mux behaviour Luc Michel
2020-09-26 21:40 ` Philippe Mathieu-Daudé
2020-10-02 14:51 ` Philippe Mathieu-Daudé
2020-10-04 18:37 ` Luc Michel
2020-10-05 19:50 ` Luc Michel
2020-09-25 10:17 ` [PATCH 11/14] hw/misc/bcm2835_cprman: add the DSI0HSCK multiplexer Luc Michel
2020-10-02 14:55 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [PATCH 12/14] hw/misc/bcm2835_cprman: add sane reset values to the registers Luc Michel
2020-09-25 10:17 ` [RFC PATCH 13/14] hw/char/pl011: add a clock input Luc Michel
2020-09-25 10:36 ` Philippe Mathieu-Daudé
2020-09-25 10:17 ` [RFC PATCH 14/14] hw/arm/bcm2835_peripherals: connect the UART clock Luc Michel
2020-09-25 11:56 ` [PATCH 00/14] raspi: add the bcm2835 cprman clock manager no-reply
2020-09-25 12:55 ` Philippe Mathieu-Daudé
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=4aa9f0c3-dc4b-1e87-d601-87b0498de8b1@amsat.org \
--to=f4bug@amsat.org \
--cc=Andrew.Baumann@microsoft.com \
--cc=luc@lmichel.fr \
--cc=peter.maydell@linaro.org \
--cc=qemu-arm@nongnu.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).