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

[Nolan]

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?