2021-08-05 19:56:13

by Piyush Mehta

[permalink] [raw]
Subject: [PATCH V2 3/3] gpio: modepin: Add driver support for modepin GPIO controller

This patch adds driver support for the zynqmp modepin GPIO controller.
GPIO modepin driver set and get the value and status of the PS_MODE pin,
based on device-tree pin configuration. These four mode pins are
configurable as input/output. The mode pin has a control register, which
have lower four-bits [0:3] are configurable as input/output, next four-bits
can be used for reading the data as input[4:7], and next setting the
output pin state output[8:11].

Signed-off-by: Piyush Mehta <[email protected]>
---
Changes in v2:
- Modepin driver- Addressed review comments:
- Update APIs
- Removed unwanted variables
- Handle return path for probe function

https://lore.kernel.org/linux-arm-kernel/[email protected]/T/#m276c8a5c52f8dc1ed1cd91a2d660f78d498e4ae5
---
drivers/gpio/Kconfig | 12 +++
drivers/gpio/Makefile | 1 +
drivers/gpio/gpio-zynqmp-modepin.c | 158 +++++++++++++++++++++++++++++++++++++
3 files changed, 171 insertions(+)
create mode 100644 drivers/gpio/gpio-zynqmp-modepin.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index fab5710..90a3a3d 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -755,6 +755,18 @@ config GPIO_ZYNQ
help
Say yes here to support Xilinx Zynq GPIO controller.

+config GPIO_ZYNQMP_MODEPIN
+ tristate "ZynqMP ps-mode pin gpio configuration driver"
+ depends on ZYNQMP_FIRMWARE
+ default ZYNQMP_FIRMWARE
+ help
+ Say yes here to support the ZynqMP ps-mode pin gpio configuration
+ driver.
+
+ This ps-mode pin gpio driver is based on GPIO framework, PS_MODE
+ is 4-bits boot mode pins. It sets and gets the status of
+ the ps-mode pin. Every pin can be configured as input/output.
+
config GPIO_LOONGSON1
tristate "Loongson1 GPIO support"
depends on MACH_LOONGSON32
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index 32a3265..978dc4595 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -183,3 +183,4 @@ obj-$(CONFIG_GPIO_XRA1403) += gpio-xra1403.o
obj-$(CONFIG_GPIO_XTENSA) += gpio-xtensa.o
obj-$(CONFIG_GPIO_ZEVIO) += gpio-zevio.o
obj-$(CONFIG_GPIO_ZYNQ) += gpio-zynq.o
+obj-$(CONFIG_GPIO_ZYNQMP_MODEPIN) += gpio-zynqmp-modepin.o
diff --git a/drivers/gpio/gpio-zynqmp-modepin.c b/drivers/gpio/gpio-zynqmp-modepin.c
new file mode 100644
index 0000000..99c69df
--- /dev/null
+++ b/drivers/gpio/gpio-zynqmp-modepin.c
@@ -0,0 +1,158 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Driver for the ps-mode pin configuration.
+ *
+ * Copyright (c) 2021 Xilinx, Inc.
+ */
+
+#include <linux/delay.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/firmware/xlnx-zynqmp.h>
+
+/* 4-bit boot mode pins */
+#define MODE_PINS 4
+
+/**
+ * modepin_gpio_get_value - Get the state of the specified pin of GPIO device
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ *
+ * This function reads the state of the specified pin of the GPIO device.
+ *
+ * Return: 0 if the pin is low, 1 if pin is high, -EINVAL wrong pin configured
+ * or error value.
+ */
+static int modepin_gpio_get_value(struct gpio_chip *chip, unsigned int pin)
+{
+ u32 regval = 0;
+ int ret;
+
+ ret = zynqmp_pm_bootmode_read(&regval);
+ if (ret) {
+ pr_err("modepin: get value err %d\n", ret);
+ return ret;
+ }
+
+ return !!(regval & BIT(pin + 8));
+}
+
+/**
+ * modepin_gpio_set_value - Modify the state of the pin with specified value
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ * @state: value used to modify the state of the specified pin
+ *
+ * This function reads the state of the specified pin of the GPIO device, mask
+ * with the capture state of GPIO pin, and update pin of GPIO device.
+ *
+ * Return: None.
+ */
+static void modepin_gpio_set_value(struct gpio_chip *chip, unsigned int pin,
+ int state)
+{
+ u32 bootpin_val = 0;
+ int ret;
+
+ ret = zynqmp_pm_bootmode_read(&bootpin_val);
+ if (ret)
+ pr_err("modepin: get value err %d\n", ret);
+
+ if (state)
+ bootpin_val |= BIT(pin + 8);
+ else
+ bootpin_val &= ~BIT(pin + 8);
+
+ /* Configure bootpin value */
+ ret = zynqmp_pm_bootmode_write(bootpin_val);
+ if (ret)
+ pr_err("modepin: %s failed\n", __func__);
+}
+
+/**
+ * modepin_gpio_dir_in - Set the direction of the specified GPIO pin as input
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ *
+ * Return: 0 always
+ */
+static int modepin_gpio_dir_in(struct gpio_chip *chip, unsigned int pin)
+{
+ return 0;
+}
+
+/**
+ * modepin_gpio_dir_out - Set the direction of the specified GPIO pin as output
+ * @chip: gpio_chip instance to be worked on
+ * @pin: gpio pin number within the device
+ * @state: value to be written to specified pin
+ *
+ * Return: 0 always
+ */
+static int modepin_gpio_dir_out(struct gpio_chip *chip, unsigned int pin,
+ int state)
+{
+ return 0;
+}
+
+/**
+ * modepin_gpio_probe - Initialization method for modepin_gpio
+ * @pdev: platform device instance
+ *
+ * Return: 0 on success, negative error otherwise.
+ */
+static int modepin_gpio_probe(struct platform_device *pdev)
+{
+ struct gpio_chip *chip;
+ int status;
+
+ chip = devm_kzalloc(&pdev->dev, sizeof(*chip), GFP_KERNEL);
+ if (!chip)
+ return -ENOMEM;
+
+ platform_set_drvdata(pdev, chip);
+
+ /* configure the gpio chip */
+ chip->base = -1;
+ chip->ngpio = MODE_PINS;
+ chip->owner = THIS_MODULE;
+ chip->parent = &pdev->dev;
+ chip->get = modepin_gpio_get_value;
+ chip->set = modepin_gpio_set_value;
+ chip->direction_input = modepin_gpio_dir_in;
+ chip->direction_output = modepin_gpio_dir_out;
+ chip->label = dev_name(&pdev->dev);
+
+ /* modepin gpio registration */
+ status = devm_gpiochip_add_data(&pdev->dev, chip, chip);
+ if (status)
+ return dev_err_probe(&pdev->dev, status,
+ "Failed to add GPIO chip\n");
+
+ return status;
+}
+
+static const struct of_device_id modepin_platform_id[] = {
+ { .compatible = "xlnx,zynqmp-gpio-modepin", },
+ { }
+};
+
+static struct platform_driver modepin_platform_driver = {
+ .driver = {
+ .name = "modepin-gpio",
+ .of_match_table = modepin_platform_id,
+ },
+ .probe = modepin_gpio_probe,
+};
+
+module_platform_driver(modepin_platform_driver);
+
+MODULE_AUTHOR("Piyush Mehta <[email protected]>");
+MODULE_DESCRIPTION("ZynqMP Boot PS_MODE Configuration");
+MODULE_LICENSE("GPL v2");
--
2.7.4


2021-08-11 13:02:24

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] gpio: modepin: Add driver support for modepin GPIO controller

Hi Piyush,

thanks for your patch!

Can you explain one thing to me: since this is now a GPIO driver
that means "General Purpos Input/Output", then these bits are
accessed like this:

On Thu, Aug 5, 2021 at 7:43 PM Piyush Mehta <[email protected]> wrote:

> + ret = zynqmp_pm_bootmode_read(&bootpin_val);

This does not look very general purpose. These seem to be all about
boot mode, right?

So can you explain why this should be a GPIO driver at all?

I understand it is sometimes convenient to describe stuff as GPIO even
if it is not (for example to get a convenient userspace interface) but
as maintainers
we really need to make sure that the subsystem is not being abused
for things not GPIO.

Yours,
Linus Walleij

2021-08-11 13:34:07

by Michal Simek

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] gpio: modepin: Add driver support for modepin GPIO controller

Hi Linus,

On 8/11/21 2:59 PM, Linus Walleij wrote:
> Hi Piyush,
>
> thanks for your patch!
>
> Can you explain one thing to me: since this is now a GPIO driver
> that means "General Purpos Input/Output", then these bits are
> accessed like this:
>
> On Thu, Aug 5, 2021 at 7:43 PM Piyush Mehta <[email protected]> wrote:
>
>> + ret = zynqmp_pm_bootmode_read(&bootpin_val);
>
> This does not look very general purpose. These seem to be all about
> boot mode, right?
>
> So can you explain why this should be a GPIO driver at all?
>
> I understand it is sometimes convenient to describe stuff as GPIO even
> if it is not (for example to get a convenient userspace interface) but
> as maintainers
> we really need to make sure that the subsystem is not being abused
> for things not GPIO.

They are bootmode pins because that pins are designed and used by ROM to
get information which boot device should be used.
But after this is it is really behaving as generic purpose I/O pins.
Xilinx is using them for years for usb phy resets. I have also seen them
to be used for other reset functionality.
And that's exactly what we are trying to do here by this driver. Because
usb hubs/phys have reset normally connected via gpio pin which is
toggled. And we have boards where these resets are connected via these
pins or via hard gpio IP or via i2c-gpio expanders that's why IMHO this
option should be designed in the same way to have gpio reset
functionality added dwc3 and based on DT/board bootmode gpio, ps gpio,
pl gpio, i2c gpio or gpio over power regulators, etc will be used.

Please let me know if you want to get more information about it.

Thanks,
Michal

2021-08-11 14:46:20

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] gpio: modepin: Add driver support for modepin GPIO controller

On Wed, Aug 11, 2021 at 3:30 PM Michal Simek <[email protected]> wrote:

> They are bootmode pins because that pins are designed and used by ROM to
> get information which boot device should be used.
> But after this is it is really behaving as generic purpose I/O pins.
> Xilinx is using them for years for usb phy resets. I have also seen them
> to be used for other reset functionality.

OK if they are used for general purpose tasks then a GPIO driver
is fine, I was worried that it was just a way to read these pins from
userspace.

Go ahead with this patch series!

Yours,
Linus Walleij

2021-08-11 15:16:55

by Linus Walleij

[permalink] [raw]
Subject: Re: [PATCH V2 3/3] gpio: modepin: Add driver support for modepin GPIO controller

On Thu, Aug 5, 2021 at 7:43 PM Piyush Mehta <[email protected]> wrote:

> This patch adds driver support for the zynqmp modepin GPIO controller.
> GPIO modepin driver set and get the value and status of the PS_MODE pin,
> based on device-tree pin configuration. These four mode pins are
> configurable as input/output. The mode pin has a control register, which
> have lower four-bits [0:3] are configurable as input/output, next four-bits
> can be used for reading the data as input[4:7], and next setting the
> output pin state output[8:11].
>
> Signed-off-by: Piyush Mehta <[email protected]>
> ---
> Changes in v2:

After discussion with Michal:
Reviewed-by: Linus Walleij <[email protected]>

Yours,
Linus Walleij