[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Eric Anholt]

The RPi firmware exposes all of the board's GPIO lines through
property calls.  Linux chooses to control most lines directly through
the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
need to access them through the firmware.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 .../bindings/gpio/gpio-raspberrypi-firmware.txt    | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt

diff --git a/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
new file mode 100644
index 000000000000..2b635c23a6f8
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
@@ -0,0 +1,22 @@
+Raspberry Pi power domain driver
+
+Required properties:
+
+- compatible:		Should be "raspberrypi,firmware-gpio"
+- gpio-controller:	Marks the device node as a gpio controller
+- #gpio-cells:		Should be <2> for GPIO number and flags
+- ngpios:		Number of GPIO lines to control.  See gpio.txt
+- firmware:		Reference to the RPi firmware device node
+- raspberrypi,firmware-gpio-offset:
+			Number the firmware uses for the first GPIO line
+			  controlled by this driver
+
+Example:
+fxl6408: firmware-gpio-128 {
+	compatible = "raspberrypi,firmware-gpio";
+	gpio-controller;
+	#gpio-cells = <2>;
+	firmware = <&firmware>;
+	ngpios = <8>;
+	raspberrypi,firmware-gpio-offset = <128>;
+};
-- 
2.9.3

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Eric Anholt]

This driver will be used for accessing the FXL6408 GPIO exander on the
Pi3.  We can't drive it directly from Linux because the firmware is
continuously polling one of the expander's lines to do its
undervoltage detection.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 drivers/gpio/Kconfig                       |   8 ++
 drivers/gpio/Makefile                      |   1 +
 drivers/gpio/gpio-raspberrypi.c            | 159 +++++++++++++++++++++++++++++
 include/soc/bcm2835/raspberrypi-firmware.h |   2 +
 4 files changed, 170 insertions(+)
 create mode 100644 drivers/gpio/gpio-raspberrypi.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index fcbd8e318474..60cf38bb3a44 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -136,6 +136,14 @@ config GPIO_BCM_KONA
 	help
 	  Turn on GPIO support for Broadcom "Kona" chips.
 
+config GPIO_RASPBERRYPI
+	tristate "Raspberry Pi firmware-based GPIO access"
+	depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST)
+	help
+	  Turns on support for using the Raspberry Pi firmware to
+	  control GPIO pins.  Used for access to the FXL6408 GPIO
+	  expander on the Pi 3.
+
 config GPIO_BRCMSTB
 	tristate "BRCMSTB GPIO support"
 	default y if (ARCH_BRCMSTB || BMIPS_GENERIC)
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index b3e039c3ae8d..fa10cf4030ad 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -31,6 +31,7 @@ obj-$(CONFIG_GPIO_ATH79)	+= gpio-ath79.o
 obj-$(CONFIG_GPIO_ASPEED)	+= gpio-aspeed.o
 obj-$(CONFIG_GPIO_AXP209)	+= gpio-axp209.o
 obj-$(CONFIG_GPIO_BCM_KONA)	+= gpio-bcm-kona.o
+obj-$(CONFIG_GPIO_RASPBERRYPI)	+= gpio-raspberrypi.o
 obj-$(CONFIG_GPIO_BRCMSTB)	+= gpio-brcmstb.o
 obj-$(CONFIG_GPIO_BT8XX)	+= gpio-bt8xx.o
 obj-$(CONFIG_GPIO_CLPS711X)	+= gpio-clps711x.o
diff --git a/drivers/gpio/gpio-raspberrypi.c b/drivers/gpio/gpio-raspberrypi.c
new file mode 100644
index 000000000000..233c211f45ba
--- /dev/null
+++ b/drivers/gpio/gpio-raspberrypi.c
@@ -0,0 +1,159 @@
+/*
+ * Copyright ? 2016 Broadcom
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+/* This driver supports using the Raspberry Pi's firmware interface to
+ * access its GPIO lines.  This lets us interact with the GPIO lines
+ * on the Raspberry Pi 3's FXL6408 expander, which we otherwise have
+ * no way to access (since the firmware is polling the chip
+ * continuously).
+ */
+
+#include <linux/err.h>
+#include <linux/gpio.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <soc/bcm2835/raspberrypi-firmware.h>
+
+struct rpi_gpio {
+	struct device *dev;
+	struct gpio_chip gc;
+	struct rpi_firmware *firmware;
+
+	/* Offset of our pins in the GET_GPIO_STATE/SET_GPIO_STATE calls. */
+	unsigned offset;
+};
+
+static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
+{
+	/* We don't have direction control. */
+	return -EINVAL;
+}
+
+static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val)
+{
+	/* We don't have direction control. */
+	return -EINVAL;
+}
+
+static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val)
+{
+	struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
+	u32 packet[2] = { rpi->offset + off, val };
+	int ret;
+
+	ret = rpi_firmware_property(rpi->firmware,
+				    RPI_FIRMWARE_SET_GPIO_STATE,
+				    &packet, sizeof(packet));
+	if (ret)
+		dev_err(rpi->dev, "Error setting GPIO %d state: %d\n", off, ret);
+}
+
+static int rpi_gpio_get(struct gpio_chip *gc, unsigned off)
+{
+	struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
+	u32 packet[2] = { rpi->offset + off, 0 };
+	int ret;
+
+	ret = rpi_firmware_property(rpi->firmware,
+				    RPI_FIRMWARE_GET_GPIO_STATE,
+				    &packet, sizeof(packet));
+	if (ret) {
+		dev_err(rpi->dev, "Error getting GPIO state: %d\n", ret);
+		return ret;
+	} else if (packet[0]) {
+		dev_err(rpi->dev, "Firmware error getting GPIO state: %d\n",
+			packet[0]);
+		return -EINVAL;
+	} else {
+		return packet[1];
+	}
+}
+
+static int rpi_gpio_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct device_node *fw_node;
+	struct rpi_gpio *rpi;
+	u32 ngpio;
+	int ret;
+
+	rpi = devm_kzalloc(dev, sizeof *rpi, GFP_KERNEL);
+	if (!rpi)
+		return -ENOMEM;
+	rpi->dev = dev;
+
+	fw_node = of_parse_phandle(np, "firmware", 0);
+	if (!fw_node) {
+		dev_err(dev, "Missing firmware node\n");
+		return -ENOENT;
+	}
+
+	rpi->firmware = rpi_firmware_get(fw_node);
+	if (!rpi->firmware)
+		return -EPROBE_DEFER;
+
+	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio)) {
+		dev_err(dev, "Missing ngpios");
+		return -ENOENT;
+	}
+	if (of_property_read_u32(pdev->dev.of_node,
+				 "raspberrypi,firmware-gpio-offset",
+				 &rpi->offset)) {
+		dev_err(dev, "Missing raspberrypi,firmware-gpio-offset");
+		return -ENOENT;
+	}
+
+	rpi->gc.label = np->full_name;
+	rpi->gc.owner = THIS_MODULE;
+	rpi->gc.of_node = np;
+	rpi->gc.ngpio = ngpio;
+	rpi->gc.direction_input = rpi_gpio_dir_in;
+	rpi->gc.direction_output = rpi_gpio_dir_out;
+	rpi->gc.get = rpi_gpio_get;
+	rpi->gc.set = rpi_gpio_set;
+	rpi->gc.can_sleep = true;
+
+	ret = gpiochip_add(&rpi->gc);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, rpi);
+
+	return 0;
+}
+
+static int rpi_gpio_remove(struct platform_device *pdev)
+{
+	struct rpi_gpio *rpi = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&rpi->gc);
+
+	return 0;
+}
+
+static const struct of_device_id __maybe_unused rpi_gpio_ids[] = {
+	{ .compatible = "raspberrypi,firmware-gpio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, rpi_gpio_ids);
+
+static struct platform_driver rpi_gpio_driver = {
+	.driver	= {
+		.name = "gpio-raspberrypi-firmware",
+		.of_match_table	= of_match_ptr(rpi_gpio_ids),
+	},
+	.probe	= rpi_gpio_probe,
+	.remove	= rpi_gpio_remove,
+};
+module_platform_driver(rpi_gpio_driver);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Eric Anholt <eric@anholt.net>");
+MODULE_DESCRIPTION("Raspberry Pi firmware GPIO driver");
diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
index 3fb357193f09..671ccd00aea2 100644
--- a/include/soc/bcm2835/raspberrypi-firmware.h
+++ b/include/soc/bcm2835/raspberrypi-firmware.h
@@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
 	RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
 	RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
 	RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
+	RPI_FIRMWARE_GET_GPIO_STATE =                         0x00030041,
 	RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
 	RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
 	RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
 	RPI_FIRMWARE_SET_TURBO =                              0x00038009,
 	RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
+	RPI_FIRMWARE_SET_GPIO_STATE =                         0x00038041,
 
 	/* Dispmanx TAGS */
 	RPI_FIRMWARE_FRAMEBUFFER_ALLOCATE =                   0x00040001,
-- 
2.9.3

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Linus Walleij]

On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:

> This driver will be used for accessing the FXL6408 GPIO exander on the
> Pi3.  We can't drive it directly from Linux because the firmware is
> continuously polling one of the expander's lines to do its
> undervoltage detection.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>
(...)

> +config GPIO_RASPBERRYPI
> +       tristate "Raspberry Pi firmware-based GPIO access"
> +       depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST)
> +       help
> +         Turns on support for using the Raspberry Pi firmware to
> +         control GPIO pins.  Used for access to the FXL6408 GPIO
> +         expander on the Pi 3.

Maybe it should be named GPIO_RPI_FXL6408 ?

(No strong opinion.)

> +#include <linux/err.h>
> +#include <linux/gpio.h>

No, only #include <linux/gpio/driver.h>

> +static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
> +{
> +       /* We don't have direction control. */
> +       return -EINVAL;
> +}
> +
> +static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val)
> +{
> +       /* We don't have direction control. */
> +       return -EINVAL;
> +}

IMO this should return OK if you try to set it to the direction
that the line is hardwired for in that case, not just fail everything.

If you return errors here, any generic driver that tries to
set the line as input or output will fail and then require a
second workaround in that driver if it is used on rpi etc etc.

Try to return zero if the consumer asks for the direction that
the line is set to.

Also implement .get_direction(). Doing so will show how to
do the above two calls in the right way.

> +static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val)
> +{
> +       struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
> +       u32 packet[2] = { rpi->offset + off, val };
> +       int ret;
> +
> +       ret = rpi_firmware_property(rpi->firmware,
> +                                   RPI_FIRMWARE_SET_GPIO_STATE,
> +                                   &packet, sizeof(packet));
> +       if (ret)
> +               dev_err(rpi->dev, "Error setting GPIO %d state: %d\n", off, ret);
> +}
> +
> +static int rpi_gpio_get(struct gpio_chip *gc, unsigned off)
> +{
> +       struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
> +       u32 packet[2] = { rpi->offset + off, 0 };
> +       int ret;
> +
> +       ret = rpi_firmware_property(rpi->firmware,
> +                                   RPI_FIRMWARE_GET_GPIO_STATE,
> +                                   &packet, sizeof(packet));
> +       if (ret) {
> +               dev_err(rpi->dev, "Error getting GPIO state: %d\n", ret);
> +               return ret;
> +       } else if (packet[0]) {
> +               dev_err(rpi->dev, "Firmware error getting GPIO state: %d\n",
> +                       packet[0]);
> +               return -EINVAL;
> +       } else {
> +               return packet[1];
> +       }

You can just remove the last } else { } clause and return on a
single line.

Please do it like this though:

return !!packet[1];

So you clamp the returned value to a boolean.

> +static int rpi_gpio_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *np = dev->of_node;
> +       struct device_node *fw_node;
> +       struct rpi_gpio *rpi;
> +       u32 ngpio;
> +       int ret;
> +
> +       rpi = devm_kzalloc(dev, sizeof *rpi, GFP_KERNEL);
> +       if (!rpi)
> +               return -ENOMEM;
> +       rpi->dev = dev;
> +
> +       fw_node = of_parse_phandle(np, "firmware", 0);
> +       if (!fw_node) {
> +               dev_err(dev, "Missing firmware node\n");
> +               return -ENOENT;
> +       }
> +
> +       rpi->firmware = rpi_firmware_get(fw_node);
> +       if (!rpi->firmware)
> +               return -EPROBE_DEFER;
> +
> +       if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio)) {
> +               dev_err(dev, "Missing ngpios");
> +               return -ENOENT;
> +       }

This is suspect you could skip and just hardcode to 8.

> +       if (of_property_read_u32(pdev->dev.of_node,
> +                                "raspberrypi,firmware-gpio-offset",
> +                                &rpi->offset)) {
> +               dev_err(dev, "Missing raspberrypi,firmware-gpio-offset");
> +               return -ENOENT;
> +       }

Can't you just hardcode this into the driver as well?

> +       rpi->gc.label = np->full_name;
> +       rpi->gc.owner = THIS_MODULE;
> +       rpi->gc.of_node = np;
> +       rpi->gc.ngpio = ngpio;
> +       rpi->gc.direction_input = rpi_gpio_dir_in;
> +       rpi->gc.direction_output = rpi_gpio_dir_out;

Implement .get_direction()

> +       rpi->gc.get = rpi_gpio_get;
> +       rpi->gc.set = rpi_gpio_set;
> +       rpi->gc.can_sleep = true;
> +
> +       ret = gpiochip_add(&rpi->gc);
> +       if (ret)
> +               return ret;

Use devm_gpiochip_add_data() and pass NULL as data
so you can still use the devm* function.

> +       platform_set_drvdata(pdev, rpi);

Should not be needed after that.

> +       return 0;
> +}
> +
> +static int rpi_gpio_remove(struct platform_device *pdev)
> +{
> +       struct rpi_gpio *rpi = platform_get_drvdata(pdev);
> +
> +       gpiochip_remove(&rpi->gc);
> +
> +       return 0;
> +}

Should be possible to drop after using devm_gpiochip_add_data()

> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
> index 3fb357193f09..671ccd00aea2 100644
> --- a/include/soc/bcm2835/raspberrypi-firmware.h
> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
>         RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
>         RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
>         RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
> +       RPI_FIRMWARE_GET_GPIO_STATE =                         0x00030041,
>         RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
>         RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
>         RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
>         RPI_FIRMWARE_SET_TURBO =                              0x00038009,
>         RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
> +       RPI_FIRMWARE_SET_GPIO_STATE =                         0x00038041,

Can you please merge this orthogonally into the rpi tree to ARM SoC?

Yours,
Linus Walleij

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
>
>> This driver will be used for accessing the FXL6408 GPIO exander on the
>> Pi3.  We can't drive it directly from Linux because the firmware is
>> continuously polling one of the expander's lines to do its
>> undervoltage detection.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
> (...)
>
>> +config GPIO_RASPBERRYPI
>> +       tristate "Raspberry Pi firmware-based GPIO access"
>> +       depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST)
>> +       help
>> +         Turns on support for using the Raspberry Pi firmware to
>> +         control GPIO pins.  Used for access to the FXL6408 GPIO
>> +         expander on the Pi 3.
>
> Maybe it should be named GPIO_RPI_FXL6408 ?
>
> (No strong opinion.)

See DT binding comment -- I think since this driver has no dependency on
being to the 6408 on the pi3, we shouldn't needlessly bind it to the
FXL6408.  (the help comment was just context for why you would want the
driver today).

>> +static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
>> +{
>> +       /* We don't have direction control. */
>> +       return -EINVAL;
>> +}
>> +
>> +static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val)
>> +{
>> +       /* We don't have direction control. */
>> +       return -EINVAL;
>> +}
>
> IMO this should return OK if you try to set it to the direction
> that the line is hardwired for in that case, not just fail everything.
>
> If you return errors here, any generic driver that tries to
> set the line as input or output will fail and then require a
> second workaround in that driver if it is used on rpi etc etc.
>
> Try to return zero if the consumer asks for the direction that
> the line is set to.
>
> Also implement .get_direction(). Doing so will show how to
> do the above two calls in the right way.

I was worried about the lack of direction support.  The firmware
interface doesn't give me anything for direction, and a get or set
of the value doesn't try to set direction.

I can try to bother them to give me support for that, but if they
cooperate on that it means that users will only get HDMI detection once
they update firmware.

The alternative to new firmware interface would be to provide a bunch of
DT saying which of these should be in/out at boot time and refuse to
change them after that.  That seems like a mess, though.

>> +static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val)
>> +{
>> +       struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
>> +       u32 packet[2] = { rpi->offset + off, val };
>> +       int ret;
>> +
>> +       ret = rpi_firmware_property(rpi->firmware,
>> +                                   RPI_FIRMWARE_SET_GPIO_STATE,
>> +                                   &packet, sizeof(packet));
>> +       if (ret)
>> +               dev_err(rpi->dev, "Error setting GPIO %d state: %d\n", off, ret);
>> +}
>> +
>> +static int rpi_gpio_get(struct gpio_chip *gc, unsigned off)
>> +{
>> +       struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
>> +       u32 packet[2] = { rpi->offset + off, 0 };
>> +       int ret;
>> +
>> +       ret = rpi_firmware_property(rpi->firmware,
>> +                                   RPI_FIRMWARE_GET_GPIO_STATE,
>> +                                   &packet, sizeof(packet));
>> +       if (ret) {
>> +               dev_err(rpi->dev, "Error getting GPIO state: %d\n", ret);
>> +               return ret;
>> +       } else if (packet[0]) {
>> +               dev_err(rpi->dev, "Firmware error getting GPIO state: %d\n",
>> +                       packet[0]);
>> +               return -EINVAL;
>> +       } else {
>> +               return packet[1];
>> +       }
>
> You can just remove the last } else { } clause and return on a
> single line.
>
> Please do it like this though:
>
> return !!packet[1];
>
> So you clamp the returned value to a boolean.

Will do.

>> +       rpi->gc.get = rpi_gpio_get;
>> +       rpi->gc.set = rpi_gpio_set;
>> +       rpi->gc.can_sleep = true;
>> +
>> +       ret = gpiochip_add(&rpi->gc);
>> +       if (ret)
>> +               return ret;
>
> Use devm_gpiochip_add_data() and pass NULL as data
> so you can still use the devm* function.

Oh, nice.

>> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
>> index 3fb357193f09..671ccd00aea2 100644
>> --- a/include/soc/bcm2835/raspberrypi-firmware.h
>> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
>> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
>>         RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
>>         RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
>>         RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
>> +       RPI_FIRMWARE_GET_GPIO_STATE =                         0x00030041,
>>         RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
>>         RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
>>         RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
>>         RPI_FIRMWARE_SET_TURBO =                              0x00038009,
>>         RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
>> +       RPI_FIRMWARE_SET_GPIO_STATE =                         0x00038041,
>
> Can you please merge this orthogonally into the rpi tree to ARM SoC?

This driver would appear in the rpi downstream tree once we settle the
driver here.  Or are you asking me to delay this series until I can get
them to pull just a patch extending the set of packets?
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160923/83848ae6/attachment.sig>

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Linus Walleij]

On Fri, Sep 23, 2016 at 3:15 PM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

>> Maybe it should be named GPIO_RPI_FXL6408 ?
>>
>> (No strong opinion.)
>
> See DT binding comment -- I think since this driver has no dependency on
> being to the 6408 on the pi3, we shouldn't needlessly bind it to the
> FXL6408.  (the help comment was just context for why you would want the
> driver today).

OK

>>> +static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
>>> +{
>>> +       /* We don't have direction control. */
>>> +       return -EINVAL;
>>> +}
>>> +
>>> +static int rpi_gpio_dir_out(struct gpio_chip *gc, unsigned off, int val)
>>> +{
>>> +       /* We don't have direction control. */
>>> +       return -EINVAL;
>>> +}
>>
>> IMO this should return OK if you try to set it to the direction
>> that the line is hardwired for in that case, not just fail everything.
>>
>> If you return errors here, any generic driver that tries to
>> set the line as input or output will fail and then require a
>> second workaround in that driver if it is used on rpi etc etc.
>>
>> Try to return zero if the consumer asks for the direction that
>> the line is set to.
>>
>> Also implement .get_direction(). Doing so will show how to
>> do the above two calls in the right way.
>
> I was worried about the lack of direction support.  The firmware
> interface doesn't give me anything for direction, and a get or set
> of the value doesn't try to set direction.
>
> I can try to bother them to give me support for that, but if they
> cooperate on that it means that users will only get HDMI detection once
> they update firmware.
>
> The alternative to new firmware interface would be to provide a bunch of
> DT saying which of these should be in/out at boot time and refuse to
> change them after that.  That seems like a mess, though.

It has to be resolved one way or another I'm afraid.

Do you have an API in place to ask for the firmware version?
RPI_FIRMWARE_GET_FIRMWARE_REVISION seems to
exist at least?

In that case try to make some compromise, e.g. if lines 0 and 1
are output and the rest input in a certain firmware version:

struct rpi_gpio {
    (...)
    u8 dirs;
};

if (fw_version <= a)
     rpi->dirs = 0x03;
else if (fw_version > a && fw_version <= b)
    rpi->dirs = 0x07;
else
     /* Ask firmware */

Then in e.g.

static int rpi_gpio_dir_in(struct gpio_chip *gc, unsigned off)
{
     struct rpi_gpio *rpi = gpiochip_get_data(gc);

     if (!(rpi->dirs & BIT(off)))
            return 0;
     return -EINVAL;
}

I think this should be managed by code in the driver like this
and not by any DT properties. I suspect also the ngpio number
is better to determine from looking at the fw version number.

>> Use devm_gpiochip_add_data() and pass NULL as data
>> so you can still use the devm* function.
>
> Oh, nice.

I forgot this: with devm_gpiochip_add_data() pass struct rpi_gpio *
as data then you can just:

static void rpi_gpio_set(struct gpio_chip *gc, unsigned off, int val)
{
-       struct rpi_gpio *rpi = container_of(gc, struct rpi_gpio, gc);
+      struct rpi_gpio *rpi = gpiochip_get_data(gc);

Applies everywhere.

>>> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
>>> index 3fb357193f09..671ccd00aea2 100644
>>> --- a/include/soc/bcm2835/raspberrypi-firmware.h
>>> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
>>> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
>>>         RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
>>>         RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
>>>         RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
>>> +       RPI_FIRMWARE_GET_GPIO_STATE =                         0x00030041,
>>>         RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
>>>         RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
>>>         RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
>>>         RPI_FIRMWARE_SET_TURBO =                              0x00038009,
>>>         RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
>>> +       RPI_FIRMWARE_SET_GPIO_STATE =                         0x00038041,
>>
>> Can you please merge this orthogonally into the rpi tree to ARM SoC?
>
> This driver would appear in the rpi downstream tree once we settle the
> driver here.  Or are you asking me to delay this series until I can get
> them to pull just a patch extending the set of packets?

Sorry I am not familiar with your development model. I don't know
about any RPI downstream tree... What I mean is that the patch to
include/soc/bcm2835/raspberrypi-firmware.h should be merged by
whoever is maintaining that file, it is not a GPIO file.

If I get an ACK from the maintainer I can take it into the GPIO
tree.

Yours,
Linus Walleij

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Fri, Sep 23, 2016 at 3:15 PM, Eric Anholt <eric@anholt.net> wrote:
>> Linus Walleij <linus.walleij@linaro.org> writes:
>>>> diff --git a/include/soc/bcm2835/raspberrypi-firmware.h b/include/soc/bcm2835/raspberrypi-firmware.h
>>>> index 3fb357193f09..671ccd00aea2 100644
>>>> --- a/include/soc/bcm2835/raspberrypi-firmware.h
>>>> +++ b/include/soc/bcm2835/raspberrypi-firmware.h
>>>> @@ -73,11 +73,13 @@ enum rpi_firmware_property_tag {
>>>>         RPI_FIRMWARE_GET_DISPMANX_RESOURCE_MEM_HANDLE =       0x00030014,
>>>>         RPI_FIRMWARE_GET_EDID_BLOCK =                         0x00030020,
>>>>         RPI_FIRMWARE_GET_DOMAIN_STATE =                       0x00030030,
>>>> +       RPI_FIRMWARE_GET_GPIO_STATE =                         0x00030041,
>>>>         RPI_FIRMWARE_SET_CLOCK_STATE =                        0x00038001,
>>>>         RPI_FIRMWARE_SET_CLOCK_RATE =                         0x00038002,
>>>>         RPI_FIRMWARE_SET_VOLTAGE =                            0x00038003,
>>>>         RPI_FIRMWARE_SET_TURBO =                              0x00038009,
>>>>         RPI_FIRMWARE_SET_DOMAIN_STATE =                       0x00038030,
>>>> +       RPI_FIRMWARE_SET_GPIO_STATE =                         0x00038041,
>>>
>>> Can you please merge this orthogonally into the rpi tree to ARM SoC?
>>
>> This driver would appear in the rpi downstream tree once we settle the
>> driver here.  Or are you asking me to delay this series until I can get
>> them to pull just a patch extending the set of packets?
>
> Sorry I am not familiar with your development model. I don't know
> about any RPI downstream tree... What I mean is that the patch to
> include/soc/bcm2835/raspberrypi-firmware.h should be merged by
> whoever is maintaining that file, it is not a GPIO file.
>
> If I get an ACK from the maintainer I can take it into the GPIO
> tree.

Oh, people often say "the rpi tree" to mean downstream (currently 4.4).
The maintainer of that file upstream is me, and I was hoping you could
merge through your tree.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160924/7d45838e/attachment.sig>

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Linus Walleij]

On Sat, Sep 24, 2016 at 9:01 AM, Eric Anholt <eric@anholt.net> wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:

>> Sorry I am not familiar with your development model. I don't know
>> about any RPI downstream tree... What I mean is that the patch to
>> include/soc/bcm2835/raspberrypi-firmware.h should be merged by
>> whoever is maintaining that file, it is not a GPIO file.
>>
>> If I get an ACK from the maintainer I can take it into the GPIO
>> tree.
>
> Oh, people often say "the rpi tree" to mean downstream (currently 4.4).
> The maintainer of that file upstream is me, and I was hoping you could
> merge through your tree.

OK no problem, I can merge it once we agree on the mechanics :)

Yours,
Linus Walleij

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Stephen Warren]

On 09/23/2016 07:15 AM, Eric Anholt wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
>>
>>> This driver will be used for accessing the FXL6408 GPIO exander on the
>>> Pi3.  We can't drive it directly from Linux because the firmware is
>>> continuously polling one of the expander's lines to do its
>>> undervoltage detection.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> (...)
>>
>>> +config GPIO_RASPBERRYPI
>>> +       tristate "Raspberry Pi firmware-based GPIO access"
>>> +       depends on OF_GPIO && RASPBERRYPI_FIRMWARE && (ARCH_BCM2835 || COMPILE_TEST)
>>> +       help
>>> +         Turns on support for using the Raspberry Pi firmware to
>>> +         control GPIO pins.  Used for access to the FXL6408 GPIO
>>> +         expander on the Pi 3.
>>
>> Maybe it should be named GPIO_RPI_FXL6408 ?
>>
>> (No strong opinion.)
>
> See DT binding comment -- I think since this driver has no dependency on
> being to the 6408 on the pi3, we shouldn't needlessly bind it to the
> FXL6408.  (the help comment was just context for why you would want the
> driver today).

I'd suggest including "FW" or "FIRMWARE" in the Kconfig option too; the 
Raspberry Pi has multiple GPIO drivers; one accessing the BCM283x HW 
directly and the other going through the FW. It'd be good if each 
Kconfig name was pretty explicit re: which one it represented.

[PATCH 2/3] gpio: Add a driver for the Raspberry Pi's firmware GPIO calls.

[Stefan Wahren]

Hi Eric,

> Eric Anholt <eric@anholt.net> hat am 19. September 2016 um 18:13 geschrieben:
> 
> 
> This driver will be used for accessing the FXL6408 GPIO exander on the
> Pi3.  We can't drive it directly from Linux because the firmware is
> continuously polling one of the expander's lines to do its
> undervoltage detection.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
> ...
> +
> +static int rpi_gpio_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct device_node *fw_node;
> +	struct rpi_gpio *rpi;
> +	u32 ngpio;
> +	int ret;
> +
> +	rpi = devm_kzalloc(dev, sizeof *rpi, GFP_KERNEL);
> +	if (!rpi)
> +		return -ENOMEM;
> +	rpi->dev = dev;
> +
> +	fw_node = of_parse_phandle(np, "firmware", 0);

AFAIK fw_node must be freed with of_node_put() after usage

> +	if (!fw_node) {
> +		dev_err(dev, "Missing firmware node\n");
> +		return -ENOENT;
> +	}
> +
> +	rpi->firmware = rpi_firmware_get(fw_node);
> +	if (!rpi->firmware)
> +		return -EPROBE_DEFER;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "ngpios", &ngpio)) {
> +		dev_err(dev, "Missing ngpios");
> +		return -ENOENT;
> +	}
> +	if (of_property_read_u32(pdev->dev.of_node,
> +				 "raspberrypi,firmware-gpio-offset",
> +				 &rpi->offset)) {
> +		dev_err(dev, "Missing raspberrypi,firmware-gpio-offset");
> +		return -ENOENT;
> +	}
> +
> +	rpi->gc.label = np->full_name;
> +	rpi->gc.owner = THIS_MODULE;
> +	rpi->gc.of_node = np;
> +	rpi->gc.ngpio = ngpio;
> +	rpi->gc.direction_input = rpi_gpio_dir_in;
> +	rpi->gc.direction_output = rpi_gpio_dir_out;
> +	rpi->gc.get = rpi_gpio_get;
> +	rpi->gc.set = rpi_gpio_set;
> +	rpi->gc.can_sleep = true;

i think it's better to assign rpi->gc.base explicit.

Stefan

[PATCH 3/3] arm64: Add the Raspberry Pi firmware's interface to the FXL6408.

[Eric Anholt]

This gets us hotplug detection of HDMI, so that graphics now works at
boot.  Tested with watching the output of xrandr while plugging and
unplugging the HDMI cable.

Signed-off-by: Eric Anholt <eric@anholt.net>
---
 arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
index 7841b724e340..2460b47737e9 100644
--- a/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
+++ b/arch/arm64/boot/dts/broadcom/bcm2837-rpi-3-b.dts
@@ -23,8 +23,25 @@
 			linux,default-trigger = "default-on";
 		};
 	};
+
+	soc {
+		fxl6408: firmware-gpio-128 {
+			compatible = "raspberrypi,firmware-gpio";
+			gpio-controller;
+			#gpio-cells = <2>;
+			firmware = <&firmware>;
+			ngpios = <8>;
+			gpio-line-names = "BT_ON", "WL_ON", "", "LAN_RUN", "HPD_N", "CAM_GPIO0", "CAM_GPIO1", "PWR_LOW_N";
+
+			raspberrypi,firmware-gpio-offset = <128>;
+		};
+	};
 };
 
 &uart1 {
 	status = "okay";
 };
+
+&hdmi {
+	hpd-gpios = <&fxl6408 4 GPIO_ACTIVE_HIGH>;
+};
-- 
2.9.3

[PATCH 3/3] arm64: Add the Raspberry Pi firmware's interface to the FXL6408.

[Gerd Hoffmann]

On Mo, 2016-09-19 at 17:13 +0100, Eric Anholt wrote:
> This gets us hotplug detection of HDMI, so that graphics now works at
> boot.  Tested with watching the output of xrandr while plugging and
> unplugging the HDMI cable.

Very nice.  Whole patch series:

Tested-by: Gerd Hoffmann <kraxel@redhat.com>

/me hopes this lands in the 4.9 merge window.

cheers,
  Gerd

[PATCH 3/3] arm64: Add the Raspberry Pi firmware's interface to the FXL6408.

[Linus Walleij]

On Thu, Sep 22, 2016 at 10:44 PM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mo, 2016-09-19 at 17:13 +0100, Eric Anholt wrote:
>> This gets us hotplug detection of HDMI, so that graphics now works at
>> boot.  Tested with watching the output of xrandr while plugging and
>> unplugging the HDMI cable.
>
> Very nice.  Whole patch series:
>
> Tested-by: Gerd Hoffmann <kraxel@redhat.com>
>
> /me hopes this lands in the 4.9 merge window.

Sorry no way. I have comments on the driver and
bindings.

Yours,
Linus Walleij

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Linus Walleij]

On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:

> The RPi firmware exposes all of the board's GPIO lines through
> property calls.  Linux chooses to control most lines directly through
> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
> need to access them through the firmware.
>
> Signed-off-by: Eric Anholt <eric@anholt.net>

Aha

> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> @@ -0,0 +1,22 @@
> +Raspberry Pi power domain driver

Really? :)

> +Required properties:
> +
> +- compatible:          Should be "raspberrypi,firmware-gpio"

Usually this is vendor,compat, is the vendors name "raspberrypi"?

> +- gpio-controller:     Marks the device node as a gpio controller
> +- #gpio-cells:         Should be <2> for GPIO number and flags
> +- ngpios:              Number of GPIO lines to control.  See gpio.txt

Is this ever anything else than 8? Else omit it and hardcode
8 in the driver instead.

> +- firmware:            Reference to the RPi firmware device node

Reference the DT binding for this.

> +- raspberrypi,firmware-gpio-offset:
> +                       Number the firmware uses for the first GPIO line
> +                         controlled by this driver

Does this differ between different instances of this hardware or
can it just be open coded in the driver instead?

Yours,
Linus Walleij

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Eric Anholt]

Linus Walleij <linus.walleij@linaro.org> writes:

> On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
>
>> The RPi firmware exposes all of the board's GPIO lines through
>> property calls.  Linux chooses to control most lines directly through
>> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
>> need to access them through the firmware.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>
> Aha
>
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>> @@ -0,0 +1,22 @@
>> +Raspberry Pi power domain driver
>
> Really? :)

Thanks.

>> +Required properties:
>> +
>> +- compatible:          Should be "raspberrypi,firmware-gpio"
>
> Usually this is vendor,compat, is the vendors name "raspberrypi"?

Yes, this driver is for part of the Raspberry Pi Foundation's firmware
code (you can find the same pattern in the firmware and firmware power
domain drivers).

>> +- gpio-controller:     Marks the device node as a gpio controller
>> +- #gpio-cells:         Should be <2> for GPIO number and flags
>> +- ngpios:              Number of GPIO lines to control.  See gpio.txt
>
> Is this ever anything else than 8? Else omit it and hardcode
> 8 in the driver instead.

(see below)

>> +- firmware:            Reference to the RPi firmware device node
>
> Reference the DT binding for this.
>
>> +- raspberrypi,firmware-gpio-offset:
>> +                       Number the firmware uses for the first GPIO line
>> +                         controlled by this driver
>
> Does this differ between different instances of this hardware or
> can it just be open coded in the driver instead?

This is which range (128-135) of the firmware's GPIOs we're controlling.
If another GPIO expander appears later (quite believable, I think
they're down to 1 spare line on this expander), then we would just make
another node with a new offset and ngpios for that expander.

Sort of related: I also worry that we have races with the firmware for
the platform GPIO bits, since both ARM and firmware are doing RMWs (or,
even worse, maybe just Ws?) of the registers controlled by the pinctrl
driver.  Hopefully I can get the firmware to pass control of devices
like this over to Linux, with firmware making requests to us, but I
don't know if that will happen and we may need to access other GPIOs
using this interface :(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 800 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160923/2ef4c585/attachment.sig>

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Linus Walleij]

On Fri, Sep 23, 2016 at 3:08 PM, Eric Anholt <eric@anholt.net> wrote:

> Sort of related: I also worry that we have races with the firmware for
> the platform GPIO bits, since both ARM and firmware are doing RMWs (or,
> even worse, maybe just Ws?) of the registers controlled by the pinctrl
> driver.  Hopefully I can get the firmware to pass control of devices
> like this over to Linux, with firmware making requests to us, but I
> don't know if that will happen and we may need to access other GPIOs
> using this interface :(

For the race with firmware I have no good solutions, it's just one of
those things I guess :(

When two kernel drivers compete about registers, say for example
one driver needs to RMW bits 0-5 and another driver needs to
RMW bits 7-11, the right solution is usually to use syscon
and then regmap-mmio will deal with the concurrency as part
of regmap_update_bits() etc.

Yours,
Linus Walleij

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Stephen Warren]

On 09/23/2016 07:08 AM, Eric Anholt wrote:
> Linus Walleij <linus.walleij@linaro.org> writes:
>
>> On Mon, Sep 19, 2016 at 6:13 PM, Eric Anholt <eric@anholt.net> wrote:
>>
>>> The RPi firmware exposes all of the board's GPIO lines through
>>> property calls.  Linux chooses to control most lines directly through
>>> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
>>> need to access them through the firmware.
>>>
>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>
>> Aha
>>
>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>> @@ -0,0 +1,22 @@
>>> +Raspberry Pi power domain driver
>>
>> Really? :)
>
> Thanks.
>
>>> +Required properties:
>>> +
>>> +- compatible:          Should be "raspberrypi,firmware-gpio"
>>
>> Usually this is vendor,compat, is the vendors name "raspberrypi"?
>
> Yes, this driver is for part of the Raspberry Pi Foundation's firmware
> code (you can find the same pattern in the firmware and firmware power
> domain drivers).
>
>>> +- gpio-controller:     Marks the device node as a gpio controller
>>> +- #gpio-cells:         Should be <2> for GPIO number and flags
>>> +- ngpios:              Number of GPIO lines to control.  See gpio.txt
>>
>> Is this ever anything else than 8? Else omit it and hardcode
>> 8 in the driver instead.
>
> (see below)
>
>>> +- firmware:            Reference to the RPi firmware device node
>>
>> Reference the DT binding for this.
>>
>>> +- raspberrypi,firmware-gpio-offset:
>>> +                       Number the firmware uses for the first GPIO line
>>> +                         controlled by this driver
>>
>> Does this differ between different instances of this hardware or
>> can it just be open coded in the driver instead?
>
> This is which range (128-135) of the firmware's GPIOs we're controlling.
> If another GPIO expander appears later (quite believable, I think
> they're down to 1 spare line on this expander), then we would just make
> another node with a new offset and ngpios for that expander.

Why would we make another node for that? Wouldn't we always have a 
single node to represent the FW's control over GPIOs, and have that node 
expose all GPIOs that the FW supports. Which GPIO IDs clients actually 
use will simply be determined by the HW schematic, and kernel-side SW 
would just act as a conduit to pass those IDs between clients and the FW.

> Sort of related: I also worry that we have races with the firmware for
> the platform GPIO bits, since both ARM and firmware are doing RMWs (or,
> even worse, maybe just Ws?) of the registers controlled by the pinctrl
> driver.  Hopefully I can get the firmware to pass control of devices
> like this over to Linux, with firmware making requests to us, but I
> don't know if that will happen and we may need to access other GPIOs
> using this interface :(

Aren't there write-to-set/write-to-clear registers? If not, then either 
FW owns everything in a particular register or Linux does; the HW won't 
allow sharing.

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Stefan Wahren]

Hi Eric,

> Eric Anholt <eric@anholt.net> hat am 19. September 2016 um 18:13 geschrieben:
> 
> 
> The RPi firmware exposes all of the board's GPIO lines through
> property calls.  Linux chooses to control most lines directly through
> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
> need to access them through the firmware.
> 
> Signed-off-by: Eric Anholt <eric@anholt.net>
> ---
>  .../bindings/gpio/gpio-raspberrypi-firmware.txt    | 22
> ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> 
> diff --git
> a/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> new file mode 100644
> index 000000000000..2b635c23a6f8
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> @@ -0,0 +1,22 @@
> +Raspberry Pi power domain driver
> +
> +Required properties:
> +
> +- compatible:		Should be "raspberrypi,firmware-gpio"

i think the compatible should be more specific like

raspberrypi,rpi3-firmware-gpio

and all information which aren't requestable from the firmware should be stored
in a info structure. This makes the driver easier to extend in the future by
adding new compatibles and their info structures.

> +- gpio-controller:	Marks the device node as a gpio controller
> +- #gpio-cells:		Should be <2> for GPIO number and flags
> +- ngpios:		Number of GPIO lines to control.  See gpio.txt
> +- firmware:		Reference to the RPi firmware device node
> +- raspberrypi,firmware-gpio-offset:
> +			Number the firmware uses for the first GPIO line
> +			  controlled by this driver

We should avoid such properties because they don't really describe the hardware.

Stefan

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Stephen Warren]

On 09/23/2016 12:39 PM, Stefan Wahren wrote:
> Hi Eric,
>
>> Eric Anholt <eric@anholt.net> hat am 19. September 2016 um 18:13 geschrieben:
>>
>>
>> The RPi firmware exposes all of the board's GPIO lines through
>> property calls.  Linux chooses to control most lines directly through
>> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
>> need to access them through the firmware.
>>
>> Signed-off-by: Eric Anholt <eric@anholt.net>
>> ---
>>  .../bindings/gpio/gpio-raspberrypi-firmware.txt    | 22
>> ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644
>> Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>
>> diff --git
>> a/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>> b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>> new file mode 100644
>> index 000000000000..2b635c23a6f8
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>> @@ -0,0 +1,22 @@
>> +Raspberry Pi power domain driver
>> +
>> +Required properties:
>> +
>> +- compatible:		Should be "raspberrypi,firmware-gpio"
>
> i think the compatible should be more specific like
>
> raspberrypi,rpi3-firmware-gpio
>
> and all information which aren't requestable from the firmware should be stored
> in a info structure. This makes the driver easier to extend in the future by
> adding new compatibles and their info structures.

Is this actually specific to the Pi3 at all? Isn't the FW the same 
across all Pis; the part that's specific to the Pi3 is whether it's 
useful to use that API?

As such, I'd suggest just raspberrypi,firmware-gpio as the compatible value.

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Stefan Wahren]

> Stephen Warren <swarren@wwwdotorg.org> hat am 26. September 2016 um 18:38
> geschrieben:
> 
> 
> On 09/23/2016 12:39 PM, Stefan Wahren wrote:
> > Hi Eric,
> >
> >> Eric Anholt <eric@anholt.net> hat am 19. September 2016 um 18:13
> >> geschrieben:
> >>
> >>
> >> The RPi firmware exposes all of the board's GPIO lines through
> >> property calls.  Linux chooses to control most lines directly through
> >> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
> >> need to access them through the firmware.
> >>
> >> Signed-off-by: Eric Anholt <eric@anholt.net>
> >> ---
> >>  .../bindings/gpio/gpio-raspberrypi-firmware.txt    | 22
> >> ++++++++++++++++++++++
> >>  1 file changed, 22 insertions(+)
> >>  create mode 100644
> >> Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> >>
> >> diff --git
> >> a/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> >> b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> >> new file mode 100644
> >> index 000000000000..2b635c23a6f8
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
> >> @@ -0,0 +1,22 @@
> >> +Raspberry Pi power domain driver
> >> +
> >> +Required properties:
> >> +
> >> +- compatible:		Should be "raspberrypi,firmware-gpio"
> >
> > i think the compatible should be more specific like
> >
> > raspberrypi,rpi3-firmware-gpio
> >
> > and all information which aren't requestable from the firmware should be
> > stored
> > in a info structure. This makes the driver easier to extend in the future by
> > adding new compatibles and their info structures.
> 
> Is this actually specific to the Pi3 at all? 

AFAIK only the Raspberry Pi 3 has a GPIO expander which is accessible via the
common FW. My suggestion tries to follow the basic guideline "A precise
compatible string is better than a vague one" from Device Tree for Dummies [1].
So in case the next Raspberry Pi would have a different GPIO expander with
different parameters we could add a new compatible.

But you are right the word order in "rpi3-firmware-gpio" suggests that there are
different FW which is wrong. At the end it's only a compatible string. So no
strong opinion about the naming.

[1] -
https://events.linuxfoundation.org/sites/events/files/slides/petazzoni-device-tree-dummies.pdf

> Isn't the FW the same 
> across all Pis; the part that's specific to the Pi3 is whether it's 
> useful to use that API?
> 
> As such, I'd suggest just raspberrypi,firmware-gpio as the compatible value.
>

[PATCH 1/3] dt-bindings: Add a binding for the RPi firmware GPIO driver.

[Stephen Warren]

On 09/26/2016 12:42 PM, Stefan Wahren wrote:
>
>> Stephen Warren <swarren@wwwdotorg.org> hat am 26. September 2016 um 18:38
>> geschrieben:
>>
>>
>> On 09/23/2016 12:39 PM, Stefan Wahren wrote:
>>> Hi Eric,
>>>
>>>> Eric Anholt <eric@anholt.net> hat am 19. September 2016 um 18:13
>>>> geschrieben:
>>>>
>>>>
>>>> The RPi firmware exposes all of the board's GPIO lines through
>>>> property calls.  Linux chooses to control most lines directly through
>>>> the pinctrl driver, but for the FXL6408 GPIO expander on the Pi3, we
>>>> need to access them through the firmware.
>>>>
>>>> Signed-off-by: Eric Anholt <eric@anholt.net>
>>>> ---
>>>>  .../bindings/gpio/gpio-raspberrypi-firmware.txt    | 22
>>>> ++++++++++++++++++++++
>>>>  1 file changed, 22 insertions(+)
>>>>  create mode 100644
>>>> Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>>>
>>>> diff --git
>>>> a/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>>> b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>>> new file mode 100644
>>>> index 000000000000..2b635c23a6f8
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/gpio/gpio-raspberrypi-firmware.txt
>>>> @@ -0,0 +1,22 @@
>>>> +Raspberry Pi power domain driver
>>>> +
>>>> +Required properties:
>>>> +
>>>> +- compatible:		Should be "raspberrypi,firmware-gpio"
>>>
>>> i think the compatible should be more specific like
>>>
>>> raspberrypi,rpi3-firmware-gpio
>>>
>>> and all information which aren't requestable from the firmware should be
>>> stored
>>> in a info structure. This makes the driver easier to extend in the future by
>>> adding new compatibles and their info structures.
>>
>> Is this actually specific to the Pi3 at all?
>
> AFAIK only the Raspberry Pi 3 has a GPIO expander which is accessible via the
> common FW. My suggestion tries to follow the basic guideline "A precise
> compatible string is better than a vague one" from Device Tree for Dummies [1].
> So in case the next Raspberry Pi would have a different GPIO expander with
> different parameters we could add a new compatible.
>
> But you are right the word order in "rpi3-firmware-gpio" suggests that there are
> different FW which is wrong. At the end it's only a compatible string. So no
> strong opinion about the naming.
>
> [1] -
> https://events.linuxfoundation.org/sites/events/files/slides/petazzoni-device-tree-dummies.pdf

To deal with that requirement, what you'd want is the following:

RPi 1:
"raspberrypi,firmware-gpio"

RPi 2:
"raspberrypi,rpi2-firmware-gpio", "raspberrypi,firmware-gpio"

RPi 3:
"raspberrypi,rpi3-firmware-gpio", "raspberrypi,firmware-gpio"

Compatible values should always list both the most specific value and 
the baseline value that it's 100% backwards compatible with.

However, I don't think this argument applies in this case. It isn't the 
case that there's a separate Pi1 FW, Pi2 FW, Pi3 FW. Rather, there is a 
single FW image that runs on all (currently existing) Pis. As such, I 
believe that using solely "raspberrypi,firmware-gpio" is appropriate 
everywhere, since the thing being described is always the exact same 
thing with no variance.

This contrasts with HW modules in the SoC, since the different Pis 
really do have a different SoC, and hence potentially different HW 
modules, so e.g. compatible = "brcm,bcm2837-i2c", "brcm,bcm2835-i2c" 
could actually be useful.

>> Isn't the FW the same
>> across all Pis; the part that's specific to the Pi3 is whether it's
>> useful to use that API?
>>
>> As such, I'd suggest just raspberrypi,firmware-gpio as the compatible value.