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

[Nolan <nolan@sigbus.net>]

On 6/2/21 4:22 PM, Philippe Mathieu-Daudé wrote:
> 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).

This is not what I see on my hardware pi4. With the following kernel config:
...
CONFIG_RASPBERRYPI_FIRMWARE=y
...
CONFIG_BCM2835_WDT=m
...

if I reboot the machine without the WDT module (but with the firmware 
module), I get this:
#  echo b > /proc/sysrq-trigger
[   54.498768] sysrq: Resetting
[   54.501713] SMP: stopping secondary CPUs
[   54.505701] Reboot failed -- System halted

and it hangs forever there.

If I load the WDT module, it reboots successfully.

>>>> 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.

In v2, I log the writes, since any side effects they have (notably the 
watchdog register) are unimplemented. I didn't log the reads, since they 
actually behave exactly as the real hardware does...

>>>> +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 :)

Already sent. Is the link comment here worth a v3?