Re: [PATCH 16/25] hw/arm/stellaris: Convert SSYS to QOM device

[Philippe Mathieu-Daudé]

On 1/25/21 12:48 PM, Peter Maydell wrote:
> On Thu, 21 Jan 2021 at 22:13, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>> On 1/21/21 8:06 PM, Peter Maydell wrote:
>>> Convert the SSYS code in the Stellaris boards (which encapsulates the
>>> system registers) to a proper QOM device.  This will provide us with
>>> somewhere to put the output Clock whose frequency depends on the
>>> setting of the PLL configuration registers.
>>>
>>> This is a migration compatibility break for lm3s811evb, lm3s6965evb.
>>>
>>> We use 3-phase reset here because the Clock will need to propagate
>>> its value in the hold phase.
>>>
>>> For the moment we reset the device during the board creation so that
>>> the system_clock_scale global gets set; this will be removed in a
>>> subsequent commit.
> 
>>> +
>>> +struct ssys_state {
>>> +    SysBusDevice parent_obj;
>>> +
>>>      MemoryRegion iomem;
>>>      uint32_t pborctl;
>>>      uint32_t ldopctl;
>>> @@ -371,11 +376,18 @@ typedef struct {
>>>      uint32_t dcgc[3];
>>>      uint32_t clkvclr;
>>>      uint32_t ldoarst;
>>> +    qemu_irq irq;
>>> +    /* Properties (all read-only registers) */
>>>      uint32_t user0;
>>>      uint32_t user1;
>>> -    qemu_irq irq;
>>> -    stellaris_board_info *board;
>>> -} ssys_state;
>>> +    uint32_t did0;
>>> +    uint32_t did1;
>>> +    uint32_t dc0;
>>> +    uint32_t dc1;
>>> +    uint32_t dc2;
>>> +    uint32_t dc3;
>>> +    uint32_t dc4;
>>
>> Shouldn't these be class properties?
> 
> Could you elaborate on what you think the code ought to look like?

I am thinking something similar how Igor asked me to implement
RaspiMachineClass::board_rev in hw/arm/raspi.c, as the did/dc registers
are read-only. Anyhow this is 1/ probably not necessary and 2/ out of
the scope of this series, this patch is already complex enough, and
the work is done.

> I just used the usual thing of defining uint32 qdev properties so we
> can set these values when we create the device, as a replacement
> for the existing code which either reaches directly into the
> state struct to set the user0/user1 values or sets the
> stellaris_board_info pointer in the state struct.

No problem.

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>