| Message ID | 1477361212-18833-1-git-send-email-linux@roeck-us.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
Your series seems to have some coding style problems. See output below for
more information:
Subject: [Qemu-devel] [PATCH] hw/arm/pxa2xx: Correctly handle external GPIO reset requests
Type: series
Message-id: 1477361212-18833-1-git-send-email-linux@roeck-us.net
=== TEST SCRIPT BEGIN ===
#!/bin/bash
BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0
# Useful git options
git config --local diff.renamelimit 0
git config --local diff.renames True
commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
echo "Checking PATCH $n/$total: $(git show --no-patch --format=%s $c)..."
if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
failed=1
echo
fi
n=$((n+1))
done
exit $failed
=== TEST SCRIPT END ===
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
5e1b6c1 hw/arm/pxa2xx: Correctly handle external GPIO reset requests
=== OUTPUT BEGIN ===
Checking PATCH 1/1: hw/arm/pxa2xx: Correctly handle external GPIO reset requests...
ERROR: code indent should never use tabs
#29: FILE: hw/arm/pxa2xx.c:2056:
+ if (line == 1 && level && (s->pm_regs[PCFR >> 2] & 0x10)) {^I/* GPR_EN */$
total: 1 errors, 0 warnings, 19 lines checked
Your patch has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
=== OUTPUT END ===
Test command exited with code: 1
---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@freelists.org
On 25 October 2016 at 03:06, Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net> > --- > 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? thanks -- PMM
Hi Peter, On 10/25/2016 04:49 AM, Peter Maydell wrote: > On 25 October 2016 at 03:06, Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net> >> --- >> 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 ? Thanks, Guenter
On 25 October 2016 at 14:27, Guenter Roeck <linux@roeck-us.net> wrote: > Hi Peter, > > > On 10/25/2016 04:49 AM, Peter Maydell wrote: >> >> On 25 October 2016 at 03:06, Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net> >>> --- >>> 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 ? thanks -- PMM
On Tue, Oct 25, 2016 at 02:40:22PM +0100, Peter Maydell wrote: > On 25 October 2016 at 14:27, Guenter Roeck <linux@roeck-us.net> wrote: > > Hi Peter, > > > > > > On 10/25/2016 04:49 AM, Peter Maydell wrote: > >> > >> On 25 October 2016 at 03:06, Guenter Roeck <linux@roeck-us.net> 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 <linux@roeck-us.net> > >>> --- > >>> 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
On 25 October 2016 at 19:21, Guenter Roeck <linux@roeck-us.net> wrote: > 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). Yes, that code looks dodgy (among other things it shouldn't be reaching into the PXA2xxState's structure members directly like that; par for the course on an unmaintained elderly bit of QEMU code like this though). > How should I correctly handle that situation ? Allocate a second irq > for non-GPIO01 reset requests ? What does the hardware actually do here? It seems unlikely that the board really wires up the GPIO 89 pin back into the CPU/SoC to ask the CPU to do a full reset, rather than just handling reset itself. If it's handled at board level then the right thing would be for the spitz.c code to have its own GPIO handler that does a power-on-reset. thanks -- PMM
On Thu, Oct 27, 2016 at 03:34:29PM +0100, Peter Maydell wrote: > On 25 October 2016 at 19:21, Guenter Roeck <linux@roeck-us.net> wrote: > > 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). > > Yes, that code looks dodgy (among other things it shouldn't > be reaching into the PXA2xxState's structure members directly > like that; par for the course on an unmaintained elderly bit > of QEMU code like this though). > > > How should I correctly handle that situation ? Allocate a second irq > > for non-GPIO01 reset requests ? > > What does the hardware actually do here? It seems unlikely that > the board really wires up the GPIO 89 pin back into the CPU/SoC to > ask the CPU to do a full reset, rather than just handling reset itself. Yes, I am quite sure that is what it does. > If it's handled at board level then the right thing would be > for the spitz.c code to have its own GPIO handler that does > a power-on-reset. > Makes sense. I'll fix spitz.c (and tosa.c, which does the same) along that line. Thanks, Guenter
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(); + } } /* Initialise a PXA270 integrated chip (ARM based core). */
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 <linux@roeck-us.net> --- hw/arm/pxa2xx.c | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-)