All of lore.kernel.org
 help / color / mirror / Atom feed
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: Sat, 3 Oct 2020 20:14:15 +0200	[thread overview]
Message-ID: <dfb488db-afa4-d307-cac1-ce71e5ebf29a@amsat.org> (raw)
In-Reply-To: <20201003115444.m2woqcpit34vfv3u@sekoia-pc.home.lmichel.fr>

On 10/3/20 1:54 PM, Luc Michel wrote:
> On 16:37 Fri 02 Oct     , Philippe Mathieu-Daudé wrote:
> 
> [snip]
> 
>>>>> +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?
>>
> Yes on real hardware I'm pretty sure it's the oscillator we can see on
> the board itself, near the SoC (on the bottom side). This is how I first
> planned to implement it. I then realized that would add complexity to
> the BCM2835Peripherals model for no good reasons IMHO (mainly because of
> migration). So at the end I put it inside the CPRMAN for simplicity, and
> added a property to set its frequency.

OK as long as all boards have a 19.2MHz crystal, but if the property
is not easily accessible why not use a #define in
"hw/arm/raspi_platform.h" instead?

Else we should alias the property using object_property_add_alias()
in TYPE_BCM2835_PERIPHERALS.

> 
>>>>> +};
> 
> [snip]
> 
>>>>> +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?
>>
> 
> OK
> 
>>>
>>> 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 :/
> 
> OK, I'll add the .impl constraints.
> 
> [snip]
> 


  reply	other threads:[~2020-10-03 18:15 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é
2020-10-03 11:54         ` Luc Michel
2020-10-03 18:14           ` Philippe Mathieu-Daudé [this message]
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=dfb488db-afa4-d307-cac1-ce71e5ebf29a@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.