Re: [PATCH qemu] Add basic power management to raspi.

[Philippe Mathieu-Daudé]

On 6/2/21 11:33 PM, Nolan wrote:
> On 5/31/21 4:23 AM, Philippe Mathieu-Daudé wrote:
>> Hi Nolan,
>>
>> Thanks for your patch!
>>
>> There is something odd with your email address, which apparently
>> became sourcehut@sigbus.net instead of nolan@sigbus.net.
> 
> Ugh, oops. I was trying out sourcehut for this, and reflexively gave
> them a marker email. I'm pretty sure sourcehut won't sell my email
> address, so I'll just change it.
> 
>> On 5/18/21 10:24 PM, ~nolanl wrote:
>>> From: Nolan Leake <nolan@sigbus.net>
>>>
>>> This is just enough to make reboot and poweroff work.
>>
>> Please precise "for Linux kernels", since this doesn't work
>> with firmwares (ideally we should implement the FIRMWARE_NOTIFY_REBOOT
>> property - which Linux sends - to handle the machine reboot/halt
>> via the videocore firmware model).
> 
> Thanks, good point re: this being tuned to what Linux (and u-boot) do.
> Poking around a bit, it looks like
> "trusted-firmware-a"/"arm-trusted-firmware" uses the same method, as do
> a couple of bare-metal/hobby OSes. Couldn't immediately figure out what
> FreeBSD does.
> 
> I'm not sure I understand your point about FIRMWARE_NOTIFY_REBOOT, my
> understanding is that message is there to tell the GPU firmware that
> we're about to reboot, so it can do things like reload the PCIe USB
> chip's firmware. In my testing without the watchdog module loaded, my
> physical pi4 does not reboot, so it appears that sending
> FIRMWARE_NOTIFY_REBOOT is not enough on its own.

>From the ARM core point of view, once it sent the FIRMWARE_NOTIFY_REBOOT
message, it can't really power-off itself, it waits in a busy loop for
the VC to disable its power domain.

hw/misc/bcm2835_property.c is our model of the VC behavior. IMO this
should be where QEMU shuts down. How I'd model it is:

- ARM: sends FIRMWARE_NOTIFY_REBOOT and loops

- VC emulated via property: delays (200ms?) then calls
  SHUTDOWN_CAUSE_GUEST_RESET.

(it helps to see hw/misc/bcm2835_property.c as an external component).

>>> qemu_system_shutdown_request(SHUTDOWN_CAUSE_GUEST_SHUTDOWN);
>>> +            } else {
>>> +                qemu_system_reset_request(SHUTDOWN_CAUSE_GUEST_RESET);
>>> +            }
>>> +        }
>>
>> Shouldn't we log the unsupported bits?
> 
> I can add that, I didn't originally because the unsupported writes are
> expected.

I'd prefer we log them, even if unsupported, so in case something
behaves oddly we could look at those bits.

>>> +static void bcm2835_powermgt_reset(DeviceState *dev)
>>> +{
>>> +    BCM2835PowerMgtState *s = BCM2835_POWERMGT(dev);
>>> +
>>> +    s->rstc = 0x00000102;
>>> +    s->rsts = 0x00001000;
>>> +    s->wdog = 0x00000000;
>>
>> Where these bits come from?
> 
> From my pi4. https://elinux.org/BCM2835_registers agrees (processed from
> Broadcom source code).

OK, so please add a comment referring to
https://elinux.org/BCM2835_registers#PM.

Looking forward for v2 :)

Phil.