Re: [Qemu-trivial] [PATCH V2 1/2] ARM: PL061: Clear PL061 device state after reset

[Peter Maydell <peter.maydell@linaro.org>]

On 1 February 2016 at 20:49, Wei Huang <wei@redhat.com> wrote:
> Current QEMU doesn't clear PL061 state after reset. This causes a
> weird issue with guest reboot via GPIO. Here is the device state
> description with two reboot requests:
>
>   (PL061State fields)           data   old_in_data   istate
> VM boot                         0      0             0
> After 1st ACPI reboot request   8      8             8
> After VM PL061 driver ACK       8      8             0
> After VM reboot                 8      8             0
> ------------------------------------------------------------
> 2nd ACPI reboot request         8
>
> In the second reboot request above, because old_in_data field is 8,
> QEMU decides that there is a pending edge IRQ already (see
> pl061_update()) in input; so it doesn't raise up IRQ again. As a result
> the second reboot request is lost. The correct way is to clear PL061
> device state after reset.
>
> NOTE: The reset state is found from the following documentation:
>  - PL061 Technical Reference Manual
>  - Stellaris LM3S8962 Microcontroller Data Sheet
>  - Stellaris LM3S5P31 Microcontroller Data Sheet
>
> Signed-off-by: Wei Huang <wei@redhat.com>
> ---
>  hw/gpio/pl061.c | 32 ++++++++++++++++++++++++++++++--
>  1 file changed, 30 insertions(+), 2 deletions(-)
>
> diff --git a/hw/gpio/pl061.c b/hw/gpio/pl061.c
> index e5a696e..342a70d 100644
> --- a/hw/gpio/pl061.c
> +++ b/hw/gpio/pl061.c
> @@ -284,8 +284,35 @@ static void pl061_write(void *opaque, hwaddr offset,
>
>  static void pl061_reset(PL061State *s)
>  {
> -  s->locked = 1;
> -  s->cr = 0xff;
> +    /* reset values from PL061 TRM, Stellaris LM3S5P31 & LM3S8962 Data Sheet */
> +    s->data = 0;
> +    s->old_out_data = 0;
> +    s->old_in_data = 0;
> +    s->dir = 0;
> +    s->isense = 0;
> +    s->ibe = 0;
> +    s->iev = 0;
> +    s->im = 0;
> +    s->istate = 0;
> +    s->afsel = 0;
> +    s->dr2r = 0xff;
> +    s->dr4r = 0;
> +    s->dr8r = 0;
> +    s->odr = 0;
> +    s->pur = 0;
> +    s->pdr = 0;
> +    s->slr = 0;
> +    s->den = 0;
> +    s->locked = 1;
> +    s->cr = 0xff;
> +    s->amsel = 0;
> +}

These reset values are all OK...

> +
> +static void pl061_state_reset(DeviceState *dev)
> +{
> +    PL061State *s = PL061(dev);
> +
> +    pl061_reset(s);
>  }

...but you don't need to have this wrapper function.
You can just do the reset in a function called pl061_reset()
with the function signature we need for dc->reset.
The only place that currently calls the existing pl061_reset()
is the device's init function, and you can delete that call
because the Device framework automatically calls the dc->reset
function after device initialization.

thanks
-- PMM