This series adds a driver for the GPIO controller used in the Nintendo
Wii game console.
The driver itself, and the related devicetree work should be pretty
uncontroversial, but due to the system architecture of the Wii, I also
had to extend an old resource allocation hack to kernel/resource.c: On
the Wii, there are two separate RAM ranges, with MMIO right in the
middle, but AFAIK, Linux on PPC32 doesn't support discontiguous memory
properly. So the hack is to allocate one big RAM range with a hole
(marked as reserved memory) for MMIO in the middle.
Because this series touches different subsystems (GPIO, DT, core
resource management), I guess it should be picked up patch-by-patch by
the different maintainers.
The main difference between v2 and the previous version is that I
rewrote the driver on top of the GPIO_GENERIC library, saving 60 lines
of code.
Jonathan Neuschäfer (6):
resource: Extend the PPC32 reserved memory hack
powerpc: wii: Explicitly configure GPIO owner for poweroff pin
gpio: Add GPIO driver for Nintendo Wii
dt-bindings: gpio: Add binding for Wii GPIO controller
powerpc: wii.dts: Add ngpios property
powerpc: wii.dts: Add GPIO line names
.../bindings/gpio/nintendo,hollywood-gpio.txt | 27 +++++
.../devicetree/bindings/powerpc/nintendo/wii.txt | 9 +-
arch/powerpc/boot/dts/wii.dts | 9 ++
arch/powerpc/platforms/embedded6xx/wii.c | 7 ++
drivers/gpio/Kconfig | 9 ++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-hlwd.c | 123 +++++++++++++++++++++
kernel/resource.c | 21 +++-
8 files changed, 197 insertions(+), 9 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt
create mode 100644 drivers/gpio/gpio-hlwd.c
--
2.15.1
The Nintendo Wii game console has a GPIO controller, which is used for
the optical disk slot LED, buttons, poweroff, etc. This patch adds a
binding for this GPIO controller.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
Reviewed-by: Rob Herring <[email protected]>
---
v2:
- Drop the leading zero in the example, as suggested by Rob Herring
- Add some text to the commit message, as suggested by Linus Walleij
---
.../bindings/gpio/nintendo,hollywood-gpio.txt | 27 ++++++++++++++++++++++
.../devicetree/bindings/powerpc/nintendo/wii.txt | 9 +-------
2 files changed, 28 insertions(+), 8 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt
diff --git a/Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt b/Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt
new file mode 100644
index 000000000000..20fc72d9e61e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt
@@ -0,0 +1,27 @@
+Nintendo Wii (Hollywood) GPIO controller
+
+Required properties:
+- compatible: "nintendo,hollywood-gpio
+- reg: Physical base address and length of the controller's registers.
+- gpio-controller: Marks the device node as a GPIO controller.
+- #gpio-cells: Should be <2>. The first cell is the pin number and the
+ second cell is used to specify optional parameters:
+ - bit 0 specifies polarity (0 for normal, 1 for inverted).
+
+Optional properties:
+- ngpios: see Documentation/devicetree/bindings/gpio/gpio.txt
+- interrupt-controller: Marks the device node as an interrupt controller.
+- #interrupt-cells: Should be two.
+- interrupts: Interrupt specifier for the controller's Broadway (PowerPC)
+ interrupt.
+- interrupt-parent: phandle of the parent interrupt controller.
+
+Example:
+
+ GPIO: gpio@d8000c0 {
+ #gpio-cells = <2>;
+ compatible = "nintendo,hollywood-gpio";
+ reg = <0x0d8000c0 0x40>;
+ gpio-controller;
+ ngpios = <24>;
+ }
diff --git a/Documentation/devicetree/bindings/powerpc/nintendo/wii.txt b/Documentation/devicetree/bindings/powerpc/nintendo/wii.txt
index 36afa322b04b..a3dc4b9fa11a 100644
--- a/Documentation/devicetree/bindings/powerpc/nintendo/wii.txt
+++ b/Documentation/devicetree/bindings/powerpc/nintendo/wii.txt
@@ -152,14 +152,7 @@ Nintendo Wii device tree
1.l) The General Purpose I/O (GPIO) controller node
- Represents the dual access 32 GPIO controller interface.
-
- Required properties:
-
- - #gpio-cells : <2>
- - compatible : should be "nintendo,hollywood-gpio"
- - reg : should contain the IPC registers location and length
- - gpio-controller
+ see Documentation/devicetree/bindings/gpio/nintendo,hollywood-gpio.txt
1.m) The control node
--
2.15.1
The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
that supports a configurable number of pins (up to 32), interrupts, and
some special mechanisms to share the controller between the system's
security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
not supported.
This patch adds a basic driver for this GPIO controller. Interrupt
support will come in a later patch.
This patch is based on code developed by Albert Herranz and the GameCube
Linux Team, file arch/powerpc/platforms/embedded6xx/hlwd-gpio.c,
available at https://github.com/DeltaResero/GC-Wii-Linux-Kernels, but
has grown quite dissimilar.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
Cc: Albert Herranz <[email protected]>
Cc: Segher Boessenkool <[email protected]>
---
v2:
- Change hlwd_gpio_driver.driver.name to "gpio-hlwd" to match the
filename (was "hlwd_gpio")
- Remove unnecessary include of linux/of_gpio.h, as suggested by Linus
Walleij.
- Add struct device pointer to context struct to make it possible to use
dev_info(hlwd->dev, "..."), as suggested by Linus Walleij
- Use the GPIO_GENERIC library to reduce code size, as suggested by
Linus Walleij
- Use iowrite32be instead of __raw_writel for big-endian MMIO access, as
suggested by Linus Walleij
- Remove commit message paragraph suggesting to diff against the
original driver, because it's so different now
---
drivers/gpio/Kconfig | 9 ++++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-hlwd.c | 123 +++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 133 insertions(+)
create mode 100644 drivers/gpio/gpio-hlwd.c
diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6a8e851ad13..47606dfe06cc 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -229,6 +229,15 @@ config GPIO_GRGPIO
Select this to support Aeroflex Gaisler GRGPIO cores from the GRLIB
VHDL IP core library.
+config GPIO_HLWD
+ tristate "Nintendo Wii (Hollywood) GPIO"
+ depends on OF_GPIO
+ select GPIO_GENERIC
+ help
+ Select this to support the GPIO controller of the Nintendo Wii.
+
+ If unsure, say N.
+
config GPIO_ICH
tristate "Intel ICH GPIO"
depends on PCI && X86
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 4bc24febb889..492f62d0eb59 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -54,6 +54,7 @@ obj-$(CONFIG_GPIO_FTGPIO010) += gpio-ftgpio010.o
obj-$(CONFIG_GPIO_GE_FPGA) += gpio-ge.o
obj-$(CONFIG_GPIO_GPIO_MM) += gpio-gpio-mm.o
obj-$(CONFIG_GPIO_GRGPIO) += gpio-grgpio.o
+obj-$(CONFIG_GPIO_HLWD) += gpio-hlwd.o
obj-$(CONFIG_HTC_EGPIO) += gpio-htc-egpio.o
obj-$(CONFIG_GPIO_ICH) += gpio-ich.o
obj-$(CONFIG_GPIO_INGENIC) += gpio-ingenic.o
diff --git a/drivers/gpio/gpio-hlwd.c b/drivers/gpio/gpio-hlwd.c
new file mode 100644
index 000000000000..cf3f05a1621c
--- /dev/null
+++ b/drivers/gpio/gpio-hlwd.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0+
+// Copyright (C) 2008-2009 The GameCube Linux Team
+// Copyright (C) 2008,2009 Albert Herranz
+// Copyright (C) 2017-2018 Jonathan Neuschäfer
+//
+// Nintendo Wii (Hollywood) GPIO driver
+
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/*
+ * Register names and offsets courtesy of WiiBrew:
+ * https://wiibrew.org/wiki/Hardware/Hollywood_GPIOs
+ *
+ * Note that for most registers, there are two versions:
+ * - HW_GPIOB_* Is always accessible by the Broadway PowerPC core, but does
+ * always give access to all GPIO lines
+ * - HW_GPIO_* Is only accessible by the Broadway PowerPC code if the memory
+ * firewall (AHBPROT) in the Hollywood chipset has been configured to allow
+ * such access.
+ *
+ * The ownership of each GPIO line can be configured in the HW_GPIO_OWNER
+ * register: A one bit configures the line for access via the HW_GPIOB_*
+ * registers, a zero bit indicates access via HW_GPIO_*. This driver uses
+ * HW_GPIOB_*.
+ */
+#define HW_GPIOB_OUT 0x00
+#define HW_GPIOB_DIR 0x04
+#define HW_GPIOB_IN 0x08
+#define HW_GPIOB_INTLVL 0x0c
+#define HW_GPIOB_INTFLAG 0x10
+#define HW_GPIOB_INTMASK 0x14
+#define HW_GPIOB_INMIR 0x18
+#define HW_GPIO_ENABLE 0x1c
+#define HW_GPIO_OUT 0x20
+#define HW_GPIO_DIR 0x24
+#define HW_GPIO_IN 0x28
+#define HW_GPIO_INTLVL 0x2c
+#define HW_GPIO_INTFLAG 0x30
+#define HW_GPIO_INTMASK 0x34
+#define HW_GPIO_INMIR 0x38
+#define HW_GPIO_OWNER 0x3c
+
+
+struct hlwd_gpio {
+ struct gpio_chip gpioc;
+ void __iomem *regs;
+ struct device *dev;
+};
+
+static int hlwd_gpio_probe(struct platform_device *pdev)
+{
+ struct hlwd_gpio *hlwd;
+ struct resource *regs_resource;
+ u32 ngpios;
+ int res;
+
+ hlwd = devm_kzalloc(&pdev->dev, sizeof(*hlwd), GFP_KERNEL);
+ if (!hlwd)
+ return -ENOMEM;
+
+ /* Save the struct device pointer so dev_info, etc. can be used. */
+ hlwd->dev = &pdev->dev;
+
+ regs_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (IS_ERR(regs_resource))
+ return PTR_ERR(regs_resource);
+
+ hlwd->regs = devm_ioremap_resource(&pdev->dev, regs_resource);
+ if (IS_ERR(hlwd->regs))
+ return PTR_ERR(hlwd->regs);
+
+ /*
+ * Claim all GPIOs using the OWNER register. This will not work on
+ * systems where the AHBPROT memory firewall hasn't been configured to
+ * permit PPC access to HW_GPIO_*.
+ *
+ * Note that this has to happen before bgpio_init reads the
+ * HW_GPIOB_OUT and HW_GPIOB_DIR, because otherwise it reads the wrong
+ * values.
+ */
+ iowrite32be(0xffffffff, hlwd->regs + HW_GPIO_OWNER);
+
+ res = bgpio_init(&hlwd->gpioc, &pdev->dev, 4,
+ hlwd->regs + HW_GPIOB_IN, hlwd->regs + HW_GPIOB_OUT,
+ NULL, hlwd->regs + HW_GPIOB_DIR, NULL,
+ BGPIOF_BIG_ENDIAN_BYTE_ORDER);
+
+ if (res < 0) {
+ dev_warn(hlwd->dev, "bgpio_init failed: %d\n", res);
+ return res;
+ }
+
+ if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios))
+ ngpios = 32;
+ hlwd->gpioc.ngpio = ngpios;
+
+ return devm_gpiochip_add_data(&pdev->dev, &hlwd->gpioc, hlwd);
+}
+
+static const struct of_device_id hlwd_gpio_match[] = {
+ { .compatible = "nintendo,hollywood-gpio", },
+ {},
+};
+MODULE_DEVICE_TABLE(of, hlwd_gpio_match);
+
+static struct platform_driver hlwd_gpio_driver = {
+ .driver = {
+ .name = "gpio-hlwd",
+ .of_match_table = hlwd_gpio_match,
+ },
+ .probe = hlwd_gpio_probe,
+};
+module_platform_driver(hlwd_gpio_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <[email protected]>");
+MODULE_DESCRIPTION("Nintendo Wii GPIO driver");
+MODULE_LICENSE("GPL");
--
2.15.1
On the Nintendo Wii, there are two ranges of physical memory, and MMIO
in between, but Linux on ppc32 doesn't support discontiguous memory.
Therefore a hack was introduced in commit c5df7f775148 ("powerpc: allow
ioremap within reserved memory regions") and commit de32400dd26e ("wii:
use both mem1 and mem2 as ram"):
- Treat the area from the start of the first memory area (MEM1) to the
end of the second (MEM2) as one big memory area, but mark the part
that doesn't belong to MEM1 or MEM2 as reserved.
- Only on the Wii, allow ioremap to be used on reserved memory.
This hack, however, doesn't account for the "resource"-based API in
kernel/resource.c, because __request_region performs its own checks.
Extend the hack to kernel/resource.c, to allow more drivers to allocate
their MMIO regions on the Wii.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
Cc: Albert Herranz <[email protected]>
---
v2:
- CC Albert Herranz, who introduced this hack in 2009.
---
kernel/resource.c | 21 ++++++++++++++++++++-
1 file changed, 20 insertions(+), 1 deletion(-)
diff --git a/kernel/resource.c b/kernel/resource.c
index 54ba6de3757c..bb3d329329da 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1134,6 +1134,24 @@ resource_size_t resource_alignment(struct resource *res)
static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
+/*
+ * On some ppc32 platforms (Nintendo Wii), reserved memory is used to work
+ * around the fact that Linux doesn't support discontiguous memory (all memory
+ * is treated as one large area with holes punched in it), and reserved memory
+ * is allowed to be allocated.
+ */
+#ifdef CONFIG_PPC32
+static bool conflict_ignored(struct resource *conflict)
+{
+ extern int __allow_ioremap_reserved;
+
+ return __allow_ioremap_reserved &&
+ (conflict->flags & IORESOURCE_SYSRAM);
+}
+#else
+static bool conflict_ignored(struct resource *conflict) { return false; }
+#endif
+
/**
* __request_region - create a new busy resource region
* @parent: parent resource descriptor
@@ -1166,8 +1184,9 @@ struct resource * __request_region(struct resource *parent,
res->desc = parent->desc;
conflict = __request_resource(parent, res);
- if (!conflict)
+ if (!conflict || conflict_ignored(conflict))
break;
+
if (conflict != parent) {
if (!(conflict->flags & IORESOURCE_BUSY)) {
parent = conflict;
--
2.15.1
The Hollywood chipset's GPIO controller has two sets of registers: One
for access by the PowerPC CPU, and one for access by the ARM coprocessor
(but both are accessible from the PPC because the memory firewall
(AHBPROT) is usually disabled when booting Linux, today).
The wii_power_off function currently assumes that the poweroff GPIO pin
is configured for use via the ARM side, but the upcoming GPIO driver
configures all pins for use via the PPC side, breaking poweroff.
Configure the owner register explicitly in wii_power_off to make
wii_power_off work with and without the new GPIO driver.
I think the Wii can be switched to the generic gpio-poweroff driver,
after the GPIO driver is merged.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v2:
- no change
---
arch/powerpc/platforms/embedded6xx/wii.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index 79a1fe54ebc9..6e6db1e16d71 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -45,6 +45,7 @@
#define HW_GPIO_BASE(idx) (idx * 0x20)
#define HW_GPIO_OUT(idx) (HW_GPIO_BASE(idx) + 0)
#define HW_GPIO_DIR(idx) (HW_GPIO_BASE(idx) + 4)
+#define HW_GPIO_OWNER (HW_GPIO_BASE(1) + 0x1c)
#define HW_GPIO_SHUTDOWN (1<<1)
#define HW_GPIO_SLOT_LED (1<<5)
@@ -177,6 +178,12 @@ static void wii_power_off(void)
local_irq_disable();
if (hw_gpio) {
+ /*
+ * set the owner of the shutdown pin to ARM, because it is
+ * accessed through the registers for the ARM, below
+ */
+ clrbits32(hw_gpio + HW_GPIO_OWNER, HW_GPIO_SHUTDOWN);
+
/* make sure that the poweroff GPIO is configured as output */
setbits32(hw_gpio + HW_GPIO_DIR(1), HW_GPIO_SHUTDOWN);
--
2.15.1
The Hollywood GPIO controller supports 32 GPIOs, but on the Wii, only 24
are used.
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v2:
- no change
---
arch/powerpc/boot/dts/wii.dts | 1 +
1 file changed, 1 insertion(+)
diff --git a/arch/powerpc/boot/dts/wii.dts b/arch/powerpc/boot/dts/wii.dts
index 40b324b6391e..7235e375919c 100644
--- a/arch/powerpc/boot/dts/wii.dts
+++ b/arch/powerpc/boot/dts/wii.dts
@@ -176,6 +176,7 @@
compatible = "nintendo,hollywood-gpio";
reg = <0x0d8000c0 0x40>;
gpio-controller;
+ ngpios = <24>;
/*
* This is commented out while a standard binding
--
2.15.1
These are the GPIO line names on a Nintendo Wii, as documented in:
https://wiibrew.org/wiki/Hardware/Hollywood_GPIOs
Signed-off-by: Jonathan Neuschäfer <[email protected]>
---
v2:
- no change
---
arch/powerpc/boot/dts/wii.dts | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/arch/powerpc/boot/dts/wii.dts b/arch/powerpc/boot/dts/wii.dts
index 7235e375919c..07d5e84e98b1 100644
--- a/arch/powerpc/boot/dts/wii.dts
+++ b/arch/powerpc/boot/dts/wii.dts
@@ -178,6 +178,14 @@
gpio-controller;
ngpios = <24>;
+ gpio-line-names =
+ "POWER", "SHUTDOWN", "FAN", "DC_DC",
+ "DI_SPIN", "SLOT_LED", "EJECT_BTN", "SLOT_IN",
+ "SENSOR_BAR", "DO_EJECT", "EEP_CS", "EEP_CLK",
+ "EEP_MOSI", "EEP_MISO", "AVE_SCL", "AVE_SDA",
+ "DEBUG0", "DEBUG1", "DEBUG2", "DEBUG3",
+ "DEBUG4", "DEBUG5", "DEBUG6", "DEBUG7";
+
/*
* This is commented out while a standard binding
* for i2c over gpio is defined.
--
2.15.1
Jonathan Neuschäfer <[email protected]> writes:
> On the Nintendo Wii, there are two ranges of physical memory, and MMIO
> in between, but Linux on ppc32 doesn't support discontiguous memory.
> Therefore a hack was introduced in commit c5df7f775148 ("powerpc: allow
> ioremap within reserved memory regions") and commit de32400dd26e ("wii:
> use both mem1 and mem2 as ram"):
>
> - Treat the area from the start of the first memory area (MEM1) to the
> end of the second (MEM2) as one big memory area, but mark the part
> that doesn't belong to MEM1 or MEM2 as reserved.
> - Only on the Wii, allow ioremap to be used on reserved memory.
>
> This hack, however, doesn't account for the "resource"-based API in
> kernel/resource.c, because __request_region performs its own checks.
>
> Extend the hack to kernel/resource.c, to allow more drivers to allocate
> their MMIO regions on the Wii.
Hi Jonathan,
Sorry but I can't merge a hack like this in generic code.
Has anyone looked at adding proper discontig mem support to PPC32?
Or can we punch a hole in the resource in the right place? Maybe from
add_system_ram_resources() ?
cheers
On Tue, Jan 23, 2018 at 11:58:06PM +1100, Michael Ellerman wrote:
> Jonathan Neuschäfer <[email protected]> writes:
>
> > On the Nintendo Wii, there are two ranges of physical memory, and MMIO
> > in between, but Linux on ppc32 doesn't support discontiguous memory.
> > Therefore a hack was introduced in commit c5df7f775148 ("powerpc: allow
> > ioremap within reserved memory regions") and commit de32400dd26e ("wii:
> > use both mem1 and mem2 as ram"):
> >
> > - Treat the area from the start of the first memory area (MEM1) to the
> > end of the second (MEM2) as one big memory area, but mark the part
> > that doesn't belong to MEM1 or MEM2 as reserved.
> > - Only on the Wii, allow ioremap to be used on reserved memory.
> >
> > This hack, however, doesn't account for the "resource"-based API in
> > kernel/resource.c, because __request_region performs its own checks.
> >
> > Extend the hack to kernel/resource.c, to allow more drivers to allocate
> > their MMIO regions on the Wii.
>
> Hi Jonathan,
>
> Sorry but I can't merge a hack like this in generic code.
Makes sense.
> Has anyone looked at adding proper discontig mem support to PPC32?
I'm not aware of any such effort.
Do you have any pointer on how to implement discontiguous memory
support? CONFIG_ARCH_SPARSEMEM_ENABLE seems relevant.
> Or can we punch a hole in the resource in the right place? Maybe from
> add_system_ram_resources() ?
Not sure. add_system_ram_resources would need the original memblock
table, which is overwritten in wii_memory_fixups, if I read the code
correctly.
If a proper solution doesn't take an overwhelming amount of work, I'd
prefer a proper solution.
Thanks,
Jonathan Neuschäfer
Jonathan Neuschäfer <[email protected]> writes:
> On Tue, Jan 23, 2018 at 11:58:06PM +1100, Michael Ellerman wrote:
>> Jonathan Neuschäfer <[email protected]> writes:
>>
>> > On the Nintendo Wii, there are two ranges of physical memory, and MMIO
>> > in between, but Linux on ppc32 doesn't support discontiguous memory.
>> > Therefore a hack was introduced in commit c5df7f775148 ("powerpc: allow
>> > ioremap within reserved memory regions") and commit de32400dd26e ("wii:
>> > use both mem1 and mem2 as ram"):
>> >
>> > - Treat the area from the start of the first memory area (MEM1) to the
>> > end of the second (MEM2) as one big memory area, but mark the part
>> > that doesn't belong to MEM1 or MEM2 as reserved.
>> > - Only on the Wii, allow ioremap to be used on reserved memory.
>> >
>> > This hack, however, doesn't account for the "resource"-based API in
>> > kernel/resource.c, because __request_region performs its own checks.
>> >
>> > Extend the hack to kernel/resource.c, to allow more drivers to allocate
>> > their MMIO regions on the Wii.
>>
>> Hi Jonathan,
>>
>> Sorry but I can't merge a hack like this in generic code.
>
> Makes sense.
>
>> Has anyone looked at adding proper discontig mem support to PPC32?
>
> I'm not aware of any such effort.
>
> Do you have any pointer on how to implement discontiguous memory
> support? CONFIG_ARCH_SPARSEMEM_ENABLE seems relevant.
I'm not really sure what the key impediment to it working is.
You don't need to go all the way to SPARSEMEM, there is DISCONTIGMEM
which IIUI is quite a bit simpler.
I'd actually be interested to know what happens (ie. breaks) if you just
add the two memblocks and leave the hole in between. Is it the generic
code that breaks or is it something in the powerpc code? If it's the
later maybe we can do a small fix/hack to work around that.
>> Or can we punch a hole in the resource in the right place? Maybe from
>> add_system_ram_resources() ?
>
> Not sure. add_system_ram_resources would need the original memblock
> table, which is overwritten in wii_memory_fixups, if I read the code
> correctly.
Or it just needs to know where the "wii hole" is, and it can skip that
region, that should be doable, but whether it actually works I'm not
100% sure.
> If a proper solution doesn't take an overwhelming amount of work, I'd
> prefer a proper solution.
Thanks.
cheers
On Wed, Jan 24, 2018 at 12:23:05PM +1100, Michael Ellerman wrote:
> Jonathan Neuschäfer <[email protected]> writes:
[...]
> > Do you have any pointer on how to implement discontiguous memory
> > support? CONFIG_ARCH_SPARSEMEM_ENABLE seems relevant.
>
> I'm not really sure what the key impediment to it working is.
>
> You don't need to go all the way to SPARSEMEM, there is DISCONTIGMEM
> which IIUI is quite a bit simpler.
>
> I'd actually be interested to know what happens (ie. breaks) if you just
> add the two memblocks and leave the hole in between. Is it the generic
> code that breaks or is it something in the powerpc code? If it's the
> later maybe we can do a small fix/hack to work around that.
Ok, I did some experimentation.
First, I made wii_memory_fixups return early, before actually doing
anything[1].
[ 0.000000] __ioremap(): phys addr 0xc003000 is RAM lr flipper_pic_init
[ 0.000000] flipper-pic: controller at 0x0c003000 mapped to 0x (null)
[ 0.000000] Unable to handle kernel paging request for data at address 0x00000004
* __ioremap_caller detects overlap with RAM like this: p < virt_to_phys(high_memory)
* flipper_pic_init gets NULL from ioremap, but doesn't check for NULL
Then I hacked up __ioremap_caller to use memblock_is_map_memory[2],
because it considers memblocks correctly. The result was that the system
boots further, but then enters the sleep mode where the power LED shines
yellow. In this mode the ARM runs but the PPC doesn't. The same thing
would happen if GPIO 3 ("DC_DC"[3]) was pulled low. These are the last few
lines:
[ 0.770324] io scheduler mq-deadline registered
[ 0.772472] io scheduler kyber registered
I don't know what exactly is triggering this effect.
Thanks for your help,
Jonathan Neuschäfer
[1]: diff --git a/arch/powerpc/platforms/embedded6xx/wii.c b/arch/powerpc/platforms/embedded6xx/wii.c
index 6e6db1e16d71..cddd5606a63d 100644
--- a/arch/powerpc/platforms/embedded6xx/wii.c
+++ b/arch/powerpc/platforms/embedded6xx/wii.c
@@ -81,6 +81,9 @@ void __init wii_memory_fixups(void)
BUG_ON(memblock.memory.cnt != 2);
BUG_ON(!page_aligned(p[0].base) || !page_aligned(p[1].base));
+ /* don't fix the memory map */
+ return;
+
/* trim unaligned tail */
memblock_remove(ALIGN(p[1].base + p[1].size, PAGE_SIZE),
(phys_addr_t)ULLONG_MAX);
[2]: diff --git a/arch/powerpc/mm/pgtable_32.c b/arch/powerpc/mm/pgtable_32.c
index f6c7f54c0515..bff581003c50 100644
--- a/arch/powerpc/mm/pgtable_32.c
+++ b/arch/powerpc/mm/pgtable_32.c
@@ -154,8 +154,7 @@ __ioremap_caller(phys_addr_t addr, unsigned long size, unsigned long flags,
* Don't allow anybody to remap normal RAM that we're using.
* mem_init() sets high_memory so only do the check after that.
*/
- if (slab_is_available() && (p < virt_to_phys(high_memory)) &&
- !(__allow_ioremap_reserved && memblock_is_region_reserved(p, size))) {
+ if (slab_is_available() && memblock_is_map_memory(p)) {
printk("__ioremap(): phys addr 0x%llx is RAM lr %ps\n",
(unsigned long long)p, __builtin_return_address(0));
return NULL;
[3]: http://wiibrew.org/wiki/Hardware/Hollywood_GPIOs
On Mon, Jan 22, 2018 at 7:04 AM, Jonathan Neuschäfer
<[email protected]> wrote:
Style issues below.
> +#define HW_GPIO_OWNER 0x3c
> +
> +
> +struct hlwd_gpio {
No need extra empty line in between.
> + struct gpio_chip gpioc;
> + void __iomem *regs;
> + struct device *dev;
> +};
> +
> +static int hlwd_gpio_probe(struct platform_device *pdev)
> +{
> + struct hlwd_gpio *hlwd;
> + struct resource *regs_resource;
> + u32 ngpios;
> + int res;
> +
> + hlwd = devm_kzalloc(&pdev->dev, sizeof(*hlwd), GFP_KERNEL);
> + if (!hlwd)
> + return -ENOMEM;
> +
> + /* Save the struct device pointer so dev_info, etc. can be used. */
Useless.
> + hlwd->dev = &pdev->dev;
> +
> + regs_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + if (IS_ERR(regs_resource))
> + return PTR_ERR(regs_resource);
> +
This is redundant. Below does it for ya.
> + hlwd->regs = devm_ioremap_resource(&pdev->dev, regs_resource);
> + if (IS_ERR(hlwd->regs))
> + return PTR_ERR(hlwd->regs);
> + res = bgpio_init(&hlwd->gpioc, &pdev->dev, 4,
> + hlwd->regs + HW_GPIOB_IN, hlwd->regs + HW_GPIOB_OUT,
> + NULL, hlwd->regs + HW_GPIOB_DIR, NULL,
> + BGPIOF_BIG_ENDIAN_BYTE_ORDER);
> +
Remove this extra line.
> + if (res < 0) {
> + dev_warn(hlwd->dev, "bgpio_init failed: %d\n", res);
> + return res;
> + }
> + if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios))
> + ngpios = 32;
A nit: I would rather go with
res = of_property_read(...);
if (res)
ngpios = 32;
--
With Best Regards,
Andy Shevchenko
Hi,
On Sun, Jan 28, 2018 at 07:31:58PM +0200, Andy Shevchenko wrote:
> On Mon, Jan 22, 2018 at 7:04 AM, Jonathan Neuschäfer
> <[email protected]> wrote:
>
> Style issues below.
>
> > +#define HW_GPIO_OWNER 0x3c
> > +
> > +
> > +struct hlwd_gpio {
>
> No need extra empty line in between.
Ok.
> > + struct gpio_chip gpioc;
> > + void __iomem *regs;
> > + struct device *dev;
> > +};
> > +
> > +static int hlwd_gpio_probe(struct platform_device *pdev)
> > +{
> > + struct hlwd_gpio *hlwd;
> > + struct resource *regs_resource;
> > + u32 ngpios;
> > + int res;
> > +
> > + hlwd = devm_kzalloc(&pdev->dev, sizeof(*hlwd), GFP_KERNEL);
> > + if (!hlwd)
> > + return -ENOMEM;
> > +
>
> > + /* Save the struct device pointer so dev_info, etc. can be used. */
>
> Useless.
Ok
> > + hlwd->dev = &pdev->dev;
> > +
>
> > + regs_resource = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>
> > + if (IS_ERR(regs_resource))
> > + return PTR_ERR(regs_resource);
> > +
>
> This is redundant. Below does it for ya.
devm_ioremap_resource does not check if the resource argument is a
negative error (which is what I'm trying to catch in the above code).
But it seems that platform_get_resource can't return a negative error,
so you're right. Thanks.
>
> > + hlwd->regs = devm_ioremap_resource(&pdev->dev, regs_resource);
> > + if (IS_ERR(hlwd->regs))
> > + return PTR_ERR(hlwd->regs);
>
>
> > + res = bgpio_init(&hlwd->gpioc, &pdev->dev, 4,
> > + hlwd->regs + HW_GPIOB_IN, hlwd->regs + HW_GPIOB_OUT,
> > + NULL, hlwd->regs + HW_GPIOB_DIR, NULL,
> > + BGPIOF_BIG_ENDIAN_BYTE_ORDER);
>
> > +
>
> Remove this extra line.
Ok.
>
> > + if (res < 0) {
> > + dev_warn(hlwd->dev, "bgpio_init failed: %d\n", res);
> > + return res;
> > + }
>
> > + if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpios))
> > + ngpios = 32;
>
> A nit: I would rather go with
> res = of_property_read(...);
> if (res)
> ngpios = 32;
Ok, I have no strong opinion on this, so I'll do what you suggest.
Thanks,
Jonathan Neuschäfer
On Mon, Jan 22, 2018 at 6:04 AM, Jonathan Neuschäfer
<[email protected]> wrote:
> The Nintendo Wii game console has a GPIO controller, which is used for
> the optical disk slot LED, buttons, poweroff, etc. This patch adds a
> binding for this GPIO controller.
>
> Signed-off-by: Jonathan Neuschäfer <[email protected]>
> Reviewed-by: Rob Herring <[email protected]>
> ---
>
> v2:
Patch applied for 4.17.
Yours,
Linus Walleij
On Mon, Jan 22, 2018 at 6:04 AM, Jonathan Neuschäfer
<[email protected]> wrote:
> The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> that supports a configurable number of pins (up to 32), interrupts, and
> some special mechanisms to share the controller between the system's
> security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> not supported.
>
> This patch adds a basic driver for this GPIO controller. Interrupt
> support will come in a later patch.
>
> This patch is based on code developed by Albert Herranz and the GameCube
> Linux Team, file arch/powerpc/platforms/embedded6xx/hlwd-gpio.c,
> available at https://github.com/DeltaResero/GC-Wii-Linux-Kernels, but
> has grown quite dissimilar.
>
> Signed-off-by: Jonathan Neuschäfer <[email protected]>
> Cc: Albert Herranz <[email protected]>
> Cc: Segher Boessenkool <[email protected]>
> ---
>
> v2:
All looks very good to me, Andy had some additional comments,
once addressed in v3 I will apply this.
You only need to resend this patch as I already applied the DT
bindings.
Yours,
Linus Walleij
On Wed, Feb 07, 2018 at 01:29:45PM +0100, Linus Walleij wrote:
> On Mon, Jan 22, 2018 at 6:04 AM, Jonathan Neuschäfer
> <[email protected]> wrote:
>
> > The Nintendo Wii's chipset (called "Hollywood") has a GPIO controller
> > that supports a configurable number of pins (up to 32), interrupts, and
> > some special mechanisms to share the controller between the system's
> > security processor (an ARM926) and the PowerPC CPU. Pin multiplexing is
> > not supported.
> >
> > This patch adds a basic driver for this GPIO controller. Interrupt
> > support will come in a later patch.
> >
> > This patch is based on code developed by Albert Herranz and the GameCube
> > Linux Team, file arch/powerpc/platforms/embedded6xx/hlwd-gpio.c,
> > available at https://github.com/DeltaResero/GC-Wii-Linux-Kernels, but
> > has grown quite dissimilar.
> >
> > Signed-off-by: Jonathan Neuschäfer <[email protected]>
> > Cc: Albert Herranz <[email protected]>
> > Cc: Segher Boessenkool <[email protected]>
> > ---
> >
> > v2:
>
> All looks very good to me, Andy had some additional comments,
> once addressed in v3 I will apply this.
>
> You only need to resend this patch as I already applied the DT
> bindings.
This driver can't be used until the resource mapping problem on PPC32
(see patch 1/6 and the related discussion) is solved or worked around
with out-of-tree patches (such as patch 1/6). Should I send v3 anyway?
Thanks,
Jonathan Neuschäfer