[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] hw/arm/pxa2xx: Correctly handle external GPIO r
From: |
Guenter Roeck |
Subject: |
Re: [Qemu-devel] [PATCH] hw/arm/pxa2xx: Correctly handle external GPIO reset requests |
Date: |
Tue, 25 Oct 2016 11:21:07 -0700 |
User-agent: |
Mutt/1.5.23 (2014-03-12) |
On Tue, Oct 25, 2016 at 02:40:22PM +0100, Peter Maydell wrote:
> On 25 October 2016 at 14:27, Guenter Roeck <address@hidden> wrote:
> > Hi Peter,
> >
> >
> > On 10/25/2016 04:49 AM, Peter Maydell wrote:
> >>
> >> On 25 October 2016 at 03:06, Guenter Roeck <address@hidden> wrote:
> >>>
> >>> The internal GPIO reset, enabled with GPR_EN, only applies to GPIO pin 1.
> >>> If other GPIO pins are used for reset, this is unrelated to GPR_EN, the
> >>> reset is an external reset pin, and it resets the entire system.
> >>>
> >>> This fixes GPIO reset failures seen with various PXA270 emulations
> >>> (akita,
> >>> borzoi, spitz, tosa, terrier) when running Linux.
> >>>
> >>> Signed-off-by: Guenter Roeck <address@hidden>
> >>> ---
> >>> hw/arm/pxa2xx.c | 10 +++++++++-
> >>> 1 file changed, 9 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/hw/arm/pxa2xx.c b/hw/arm/pxa2xx.c
> >>> index cb55704..2a2a821 100644
> >>> --- a/hw/arm/pxa2xx.c
> >>> +++ b/hw/arm/pxa2xx.c
> >>> @@ -2048,10 +2048,18 @@ static void pxa2xx_reset(void *opaque, int line,
> >>> int level)
> >>> {
> >>> PXA2xxState *s = (PXA2xxState *) opaque;
> >>>
> >>> - if (level && (s->pm_regs[PCFR >> 2] & 0x10)) { /* GPR_EN */
> >>> + /*
> >>> + * GPIO pin 1 is the CPU internal GPIO reset, enabled with GPR_EN.
> >>> + * Any other pin is board specific and resets the entire system.
> >>> + */
> >>> + if (line == 1 && level && (s->pm_regs[PCFR >> 2] & 0x10)) {
> >>> /* GPR_EN */
> >>> cpu_reset(CPU(s->cpu));
> >>> /* TODO: reset peripherals */
> >>> }
> >>> +
> >>> + if (line != 1 && level) {
> >>> + qemu_system_reset_request();
> >>> + }
> >>
> >>
> >> It doesn't look to me like we wire up more than the first
> >> line (at least the qdev_connect_gpio_out() calls which
> >> connect up to s->reset in pxa255_init() and pxa270_init()
> >> only connect up one line). What am I missing that can
> >> cause line to be something other than 1?
> >>
> >
> > Here is what added logging into the function tells me:
> >
> > reboot: Restarting system
> > GR>> pxa2xx_reset(): line=0, level=1
> >
> > Maybe it is line 0 that is wired up ?
>
> Yes, that makes sense -- the 'line' variable will be 0
> on input to this function, but it is wired up to the
> pxa2xx GPIO's output 1 (not 0). Should that be a power-on
> reset or a CPU reset ?
>
Guess maybe the line == 0 check is wrong then.
Problem though is that the kernel doesn't really set GPIO01.
It sets SPITZ_GPIO_ON_RESET (for spitz), which is 89.
This is mapped to the reset function in spitz.c.
/* Handle reset */
qdev_connect_gpio_out(cpu->gpio, SPITZ_GPIO_ON_RESET, cpu->reset);
This doesn't seem correct; with that override, the reset is supposed
to happen unconditionally, not only if bit 4 of PCFR is set (and that
bit isn't set for spitz).
How should I correctly handle that situation ? Allocate a second irq
for non-GPIO01 reset requests ?
s->reset = qemu_allocate_irq(pxa2xx_reset, s, 0);
s->reset_board = qemu_allocate_irq(pxa2xx_reset, s, 1);
and then wire s->reset_board if other GPIO pins are used ?
Or just use a second function, such as pxa2xx_reset_board, in that case ?
s->reset_board = qemu_allocate_irq(pxa2xx_reset_board, s, 0);
Thanks,
Guenter