[PATCH 0/6] Nintendo Wii GPIO driver

[PATCH 0/6] Nintendo Wii GPIO driver

[Jonathan Neuschäfer]

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.

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                               |   8 +
 drivers/gpio/Makefile                              |   1 +
 drivers/gpio/gpio-hlwd.c                           | 183 +++++++++++++++++++++
 kernel/resource.c                                  |  21 ++-
 8 files changed, 256 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

[PATCH 1/6] resource: Extend the PPC32 reserved memory hack

[Jonathan Neuschäfer]

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 <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/6] powerpc: wii: Explicitly configure GPIO owner for poweroff pin

[Jonathan Neuschäfer]

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 <j.neuschaefer@gmx.net>
---
 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

[PATCH 3/6] gpio: Add GPIO driver for Nintendo Wii

[Jonathan Neuschäfer]

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.

To compare this version of the driver against the original code:
$ git fetch https://github.com/DeltaResero/GC-Wii-Linux-Kernels
$ git co FETCH_HEAD -- arch/powerpc/platforms/embedded6xx/hlwd-gpio.c
$ diff -u arch/powerpc/platforms/embedded6xx/hlwd-gpio.c \
          drivers/gpio/gpio-hlwd.c

Cc: Albert Herranz <albert_herranz@yahoo.es>
Cc: Segher Boessenkool <segher@kernel.crashing.org>
Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>

---
This driver currently uses __raw_readl and __raw_writel to access the
GPIO controller's MMIO registers. I wonder if readl/writel plus explicit
byte-swapping would be more correct, because it could be independent of
the CPU's endianness. That said, this hardware only exists in two
big-endian machines (Wii and Wii U).
---
 drivers/gpio/Kconfig     |   8 +++
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-hlwd.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 192 insertions(+)
 create mode 100644 drivers/gpio/gpio-hlwd.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index d6a8e851ad13..4f85c2053f7d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -229,6 +229,14 @@ 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
+	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..0f8942ea6ed6
--- /dev/null
+++ b/drivers/gpio/gpio-hlwd.c
@@ -0,0 +1,183 @@
+// 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
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/of.h>
+#include <linux/of_gpio.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/gpio/driver.h>
+#include <linux/spinlock.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;
+	spinlock_t lock;
+};
+
+/*
+ * Update the bit with the given bit offset in the given register to a given
+ * value
+ */
+static void hlwd_gpio_update_bit(struct gpio_chip *gc, unsigned int reg,
+		int offset, int value)
+{
+	struct hlwd_gpio *hlwd = gpiochip_get_data(gc);
+	unsigned long flags;
+	u32 bit = 1UL << offset;
+	u32 tmp;
+
+	spin_lock_irqsave(&hlwd->lock, flags);
+	tmp = __raw_readl(hlwd->regs + reg);
+	if (value)
+		__raw_writel(tmp | bit, hlwd->regs + reg);
+	else
+		__raw_writel(tmp & ~bit, hlwd->regs + reg);
+	spin_unlock_irqrestore(&hlwd->lock, flags);
+}
+
+/* Read the bit with the given bit offset in the given register */
+static int hlwd_gpio_read_bit(struct gpio_chip *gc, unsigned int reg,
+		unsigned int offset)
+{
+	struct hlwd_gpio *hlwd = gpiochip_get_data(gc);
+	unsigned long flags;
+	u32 bit = 1UL << offset;
+	u32 tmp;
+
+	spin_lock_irqsave(&hlwd->lock, flags);
+	tmp = __raw_readl(hlwd->regs + reg);
+	spin_unlock_irqrestore(&hlwd->lock, flags);
+
+	return !!(tmp & bit);
+}
+
+static int hlwd_gpio_get(struct gpio_chip *gc, unsigned int offset)
+{
+	return hlwd_gpio_read_bit(gc, HW_GPIOB_IN, offset);
+}
+
+static void hlwd_gpio_set(struct gpio_chip *gc, unsigned int offset, int val)
+{
+	hlwd_gpio_update_bit(gc, HW_GPIOB_OUT, offset, val);
+}
+
+static int hlwd_gpio_dir_in(struct gpio_chip *gc, unsigned int offset)
+{
+	hlwd_gpio_update_bit(gc, HW_GPIOB_DIR, offset, 0);
+
+	return 0;
+}
+
+static int hlwd_gpio_dir_out(struct gpio_chip *gc,
+		unsigned int offset, int val)
+{
+	/* Set the GPIO value, and then set the direction */
+	hlwd_gpio_set(gc, offset, val);
+	hlwd_gpio_update_bit(gc, HW_GPIOB_DIR, offset, 1);
+
+	return 0;
+}
+
+static int hlwd_gpio_probe(struct platform_device *pdev)
+{
+	struct hlwd_gpio *hlwd;
+	struct resource *regs_resource;
+	u32 ngpios;
+
+	hlwd = devm_kzalloc(&pdev->dev, sizeof(*hlwd), GFP_KERNEL);
+	if (!hlwd)
+		return -ENOMEM;
+
+	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_*.
+	 */
+	__raw_writel(0xffffffff, hlwd->regs + HW_GPIO_OWNER);
+
+	spin_lock_init(&hlwd->lock);
+
+	hlwd->gpioc.label = dev_name(&pdev->dev);
+	hlwd->gpioc.parent = &pdev->dev;
+	hlwd->gpioc.owner = THIS_MODULE;
+	hlwd->gpioc.direction_input = hlwd_gpio_dir_in;
+	hlwd->gpioc.direction_output = hlwd_gpio_dir_out;
+	hlwd->gpioc.get = hlwd_gpio_get;
+	hlwd->gpioc.set = hlwd_gpio_set;
+
+	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		= "hlwd_gpio",
+		.of_match_table	= hlwd_gpio_match,
+	},
+	.probe	= hlwd_gpio_probe,
+};
+module_platform_driver(hlwd_gpio_driver);
+
+MODULE_AUTHOR("Jonathan Neuschäfer <j.neuschaefer@gmx.net>");
+MODULE_DESCRIPTION("Nintendo Wii GPIO driver");
+MODULE_LICENSE("GPL");
-- 
2.15.1

[PATCH 4/6] dt-bindings: gpio: Add binding for Wii GPIO controller

[Jonathan Neuschäfer]

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 .../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..a97ce6b5b724
--- /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@0d8000c0 {
+		#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

[PATCH 5/6] powerpc: wii.dts: Add ngpios property

[Jonathan Neuschäfer]

The Hollywood GPIO controller supports 32 GPIOs, but on the Wii, only 24
are used.

Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 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

[PATCH 6/6] powerpc: wii.dts: Add GPIO line names

[Jonathan Neuschäfer]

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 <j.neuschaefer@gmx.net>
---
 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

Re: [PATCH 4/6] dt-bindings: gpio: Add binding for Wii GPIO controller

[Linus Walleij]

On Mon, Jan 15, 2018 at 4:13 AM, Jonathan Neuschäfer
<j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org> wrote:

maybe some small blurb here?

> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>

It looks good, very standard bindings.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/6] gpio: Add GPIO driver for Nintendo Wii

[Linus Walleij]

On Mon, Jan 15, 2018 at 4:13 AM, Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:

> 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.

I'm impressed by this effort. As with all reverse engineering.

> This driver currently uses __raw_readl and __raw_writel to access the
> GPIO controller's MMIO registers. I wonder if readl/writel plus explicit
> byte-swapping would be more correct, because it could be independent of
> the CPU's endianness. That said, this hardware only exists in two
> big-endian machines (Wii and Wii U).

I don't know about PPC but I think you're supposed to use
ioread32be() and iowrite32be() to do explicit BE access.

But when I look at it, I think you can just use the gpio-mmio library
for this driver and cut down code cosiderably.

> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt

Can't you just save a pointer to struct device *dev in the
state container and use dev_info(state->dev, ...) etc instead
of this?

> +#include <linux/of_gpio.h>

This include should not be needed.

> +/*
> + * Update the bit with the given bit offset in the given register to a given
> + * value
> + */
> +static void hlwd_gpio_update_bit(struct gpio_chip *gc, unsigned int reg,
> +               int offset, int value)
> +{
> +       struct hlwd_gpio *hlwd = gpiochip_get_data(gc);
> +       unsigned long flags;
> +       u32 bit = 1UL << offset;

#include <linux/bitops.h>

u32 bit = BIT(offset);

> +       u32 tmp;
> +
> +       spin_lock_irqsave(&hlwd->lock, flags);
> +       tmp = __raw_readl(hlwd->regs + reg);
> +       if (value)
> +               __raw_writel(tmp | bit, hlwd->regs + reg);
> +       else
> +               __raw_writel(tmp & ~bit, hlwd->regs + reg);
> +       spin_unlock_irqrestore(&hlwd->lock, flags);
> +}

This looks very much like it is reimplementing the stuff we already
have in drivers/gpio/gpio-mmio.h.

There is even a big endian access flag for the library.
And you get so much for free with gpio-mmio.

select GPIO_GENERIC
in Kconfig

the helpers come in from <linux/gpio/driver.h>

Look at other drivers for inspiration:
git grep bgpio_init

If you need IRQ support you should probably have your own file
for this driver, but it will be just a few lines of wrapper using
bgpio_init() and BGPIOF_BIG_ENDIAN and/or possibly
BGPIOF_BIG_ENDIAN_BYTE_ORDER.

See the other drivers.

Yours,
Linus Walleij

Re: [PATCH 3/6] gpio: Add GPIO driver for Nintendo Wii

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 3222 bytes --]

On Tue, Jan 16, 2018 at 10:42:54AM +0100, Linus Walleij wrote:
> On Mon, Jan 15, 2018 at 4:13 AM, Jonathan Neuschäfer
> <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org> wrote:
> 
> > 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.
> 
> I'm impressed by this effort. As with all reverse engineering.
> 
> > This driver currently uses __raw_readl and __raw_writel to access the
> > GPIO controller's MMIO registers. I wonder if readl/writel plus explicit
> > byte-swapping would be more correct, because it could be independent of
> > the CPU's endianness. That said, this hardware only exists in two
> > big-endian machines (Wii and Wii U).
> 
> I don't know about PPC but I think you're supposed to use
> ioread32be() and iowrite32be() to do explicit BE access.

Ah, that's the name! I didn't find ioread32*/iowrite32* in the
documentation or source code.

> But when I look at it, I think you can just use the gpio-mmio library
> for this driver and cut down code cosiderably.

I'll look into it. So far it looks good (drivers/gpio/gpio-iop.c has
just 60 lines).

> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> 
> Can't you just save a pointer to struct device *dev in the
> state container and use dev_info(state->dev, ...) etc instead
> of this?

Makes sense. I'll try this out.

> > +#include <linux/of_gpio.h>
> 
> This include should not be needed.

Okay.

> > +/*
> > + * Update the bit with the given bit offset in the given register to a given
> > + * value
> > + */
> > +static void hlwd_gpio_update_bit(struct gpio_chip *gc, unsigned int reg,
> > +               int offset, int value)
> > +{
> > +       struct hlwd_gpio *hlwd = gpiochip_get_data(gc);
> > +       unsigned long flags;
> > +       u32 bit = 1UL << offset;
> 
> #include <linux/bitops.h>
> 
> u32 bit = BIT(offset);
> 
> > +       u32 tmp;
> > +
> > +       spin_lock_irqsave(&hlwd->lock, flags);
> > +       tmp = __raw_readl(hlwd->regs + reg);
> > +       if (value)
> > +               __raw_writel(tmp | bit, hlwd->regs + reg);
> > +       else
> > +               __raw_writel(tmp & ~bit, hlwd->regs + reg);
> > +       spin_unlock_irqrestore(&hlwd->lock, flags);
> > +}
> 
> This looks very much like it is reimplementing the stuff we already
> have in drivers/gpio/gpio-mmio.h.
> 
> There is even a big endian access flag for the library.
> And you get so much for free with gpio-mmio.
> 
> select GPIO_GENERIC
> in Kconfig
> 
> the helpers come in from <linux/gpio/driver.h>
> 
> Look at other drivers for inspiration:
> git grep bgpio_init
> 
> If you need IRQ support you should probably have your own file
> for this driver, but it will be just a few lines of wrapper using
> bgpio_init() and BGPIOF_BIG_ENDIAN and/or possibly
> BGPIOF_BIG_ENDIAN_BYTE_ORDER.

Yes, I plan to add IRQ support in a later patch.

> 
> See the other drivers.

Yep, gpio-mmio looks like a good option, thanks for the pointer!


Thanks,
Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 4/6] dt-bindings: gpio: Add binding for Wii GPIO controller

[Rob Herring]

On Mon, Jan 15, 2018 at 04:13:59AM +0100, Jonathan Neuschäfer wrote:
> Signed-off-by: Jonathan Neuschäfer <j.neuschaefer-hi6Y0CQ0nG0@public.gmane.org>
> ---
>  .../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..a97ce6b5b724
> --- /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@0d8000c0 {

Drop the leading 0.

With that,

Reviewed-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/6] dt-bindings: gpio: Add binding for Wii GPIO controller

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 717 bytes --]

On Fri, Jan 19, 2018 at 05:05:21PM -0600, Rob Herring wrote:
> On Mon, Jan 15, 2018 at 04:13:59AM +0100, Jonathan Neuschäfer wrote:
> > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
> > ---
> >  .../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
[...]
> > +Example:
> > +
> > +	GPIO: gpio@0d8000c0 {
> 
> Drop the leading 0.

Ok, will do.

> With that,
> 
> Reviewed-by: Rob Herring <robh@kernel.org>

Thanks.


Jonathan Neuschäfer

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]