Re: [PATCH v2 2/4] hw/char/pl011: implement a reset method

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

On Thu, 19 Jan 2023 at 21:57, Evgeny Iakovlev
<eiakovlev@linux.microsoft.com> wrote:
>
>
> On 1/19/2023 14:27, Peter Maydell wrote:
> > On Tue, 17 Jan 2023 at 22:05, Evgeny Iakovlev
> > <eiakovlev@linux.microsoft.com> wrote:
> >> PL011 currently lacks a reset method. Implement it.
> >>
> >> Signed-off-by: Evgeny Iakovlev <eiakovlev@linux.microsoft.com>
> >> ---
> >>   hw/char/pl011.c | 31 ++++++++++++++++++++++++++-----
> >>   1 file changed, 26 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/hw/char/pl011.c b/hw/char/pl011.c
> >> index 329cc6926d..404d52a3b8 100644
> >> --- a/hw/char/pl011.c
> >> +++ b/hw/char/pl011.c
> >> @@ -397,11 +397,6 @@ static void pl011_init(Object *obj)
> >>       s->clk = qdev_init_clock_in(DEVICE(obj), "clk", pl011_clock_update, s,
> >>                                   ClockUpdate);
> >>
> >> -    s->read_trigger = 1;
> >> -    s->ifl = 0x12;
> >> -    s->cr = 0x300;
> >> -    s->flags = 0x90;
> >> -
> >>       s->id = pl011_id_arm;
> >>   }
> >>
> >> @@ -413,11 +408,37 @@ static void pl011_realize(DeviceState *dev, Error **errp)
> >>                                pl011_event, NULL, s, NULL, true);
> >>   }
> >>
> >> +static void pl011_reset(DeviceState *dev)
> >> +{
> >> +    PL011State *s = PL011(dev);
> >> +    int i;
> >> +
> >> +    s->lcr = 0;
> >> +    s->rsr = 0;
> >> +    s->dmacr = 0;
> >> +    s->int_enabled = 0;
> >> +    s->int_level = 0;
> >> +    s->ilpr = 0;
> >> +    s->ibrd = 0;
> >> +    s->fbrd = 0;
> >> +    s->read_pos = 0;
> >> +    s->read_count = 0;
> >> +    s->read_trigger = 1;
> >> +    s->ifl = 0x12;
> >> +    s->cr = 0x300;
> >> +    s->flags = 0x90;
> >> +
> >> +    for (i = 0; i < ARRAY_SIZE(s->irq); i++) {
> >> +        qemu_irq_lower(s->irq[i]);
> >> +    }
> > Reset should never touch outbound qemu_irq lines.
> > (The other end of the line will also reset and will end
> > up in the correct "as if the input is 0" state.)
>
>
> Really? I saw this reset happening on other devices in hw/char (don't
> remember which ones specifically), so i though it is needed.

A few devices mess with their IRQ line in a reset handler;
this is a bug but usually one you can get away with. Some
devices use the newer "three phase reset" approach which
does allow you to change IRQ line state in the 'hold' phase.
But generally the standard is not to touch the IRQ line if
its reset state would be 'low'. You only need to do odd
things for the rare case where an IRQ line is supposed to
be taken high on reset.

(The reason for the "no touching IRQs in reset" rule is that
there's no ordering on device reset, so if you raise an
IRQ line in your reset handler, you don't know if the
device on the other end has already reset and thus will
correctly deal with the 0->1 transition it sees, or if
it has not yet reset and is about to undo the effects of
that 0->1 transition. 3-phase reset lets devices split
their reset handling up, so you know that if you do something
with an IRQ line in the 'hold' phase that the device you're
talking to has definitely already done its 'enter' phase.
Though 3-phase reset is pretty new, so a lot of devices
don't use it yet, and I have a fairly strong suspicion
that there are still some issues with the design that we
will need to iron out as we make more use of it.)

thanks
-- PMM