| Message ID | 20230117220523.20911-3-eiakovlev@linux.microsoft.com |
|---|---|
| State | New |
| Headers | show |
| Series | Series of fixes for PL011 char device | expand |
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.) Otherwise Reviewed-by: Peter Maydell <peter.maydell@linaro.org> thanks -- PMM
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. > > Otherwise > Reviewed-by: Peter Maydell <peter.maydell@linaro.org> > > thanks > -- PMM
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
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 >
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]); + } +} + static void pl011_class_init(ObjectClass *oc, void *data) { DeviceClass *dc = DEVICE_CLASS(oc); dc->realize = pl011_realize; + dc->reset = pl011_reset; dc->vmsd = &vmstate_pl011; device_class_set_props(dc, pl011_properties); }
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(-)