From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36492) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bz6LZ-0005sp-2p for qemu-devel@nongnu.org; Tue, 25 Oct 2016 14:21:14 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bz6LV-0003EE-UX for qemu-devel@nongnu.org; Tue, 25 Oct 2016 14:21:13 -0400 Received: from bh-25.webhostbox.net ([208.91.199.152]:41029) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1bz6LV-0003Dz-Nb for qemu-devel@nongnu.org; Tue, 25 Oct 2016 14:21:09 -0400 Date: Tue, 25 Oct 2016 11:21:07 -0700 From: Guenter Roeck Message-ID: <20161025182107.GA31474@roeck-us.net> References: <1477361212-18833-1-git-send-email-linux@roeck-us.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: Subject: Re: [Qemu-devel] [PATCH] hw/arm/pxa2xx: Correctly handle external GPIO reset requests List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: QEMU Developers On Tue, Oct 25, 2016 at 02:40:22PM +0100, Peter Maydell wrote: > On 25 October 2016 at 14:27, Guenter Roeck wrote: > > Hi Peter, > > > > > > On 10/25/2016 04:49 AM, Peter Maydell wrote: > >> > >> On 25 October 2016 at 03:06, Guenter Roeck 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 > >>> --- > >>> 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