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

[eiakovlev@linux.microsoft.com]

On 1/20/23 12:45 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> 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.)

I see. Thanks a lot for explaining.

> 
> thanks
> -- PMM
>