| Message ID | 20210818081018.2620544-4-piyush.mehta@xilinx.com |
|---|---|
| State | New |
| Headers | show |
| Series | gpio: modepin: Add driver support for modepin GPIO controller | expand |
On 18.08.21 10:10, Piyush Mehta 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 <piyush.mehta@xilinx.com> > Acked-by: Michal Simek <michal.simek@xilinx.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > +/** > + * 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; > +} You say the gpio controller can configure pins as inputs or outputs. Yet, .direction_input is doing nothing. So it's not clear to me, how this sequence could work: - set gpio output high (writes bootmode) - set gpio to input (no-op, pin will remain high, not high impedance) I didn't check the previous discussions, but if this indeed works as intended, the how should be written here into the driver. That is a more useful comment than kernel doc for a stub function.
Hi Ahmad, -----Original Message----- From: Ahmad Fatoum <a.fatoum@pengutronix.de> Sent: Wednesday, August 18, 2021 2:22 PM To: Piyush Mehta <piyushm@xilinx.com>; arnd@arndb.de; zou_wei@huawei.com; gregkh@linuxfoundation.org; linus.walleij@linaro.org; Michal Simek <michals@xilinx.com>; Jiaying Liang <jliang@xilinx.com>; iwamatsu@nigauri.org; bgolaszewski@baylibre.com; robh+dt@kernel.org; Rajan Vaja <RAJANV@xilinx.com> Cc: linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; git <git@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Pengutronix Kernel Team <kernel@pengutronix.de> Subject: Re: [PATCH V3 3/3] gpio: modepin: Add driver support for modepin GPIO controller On 18.08.21 10:10, Piyush Mehta 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 <piyush.mehta@xilinx.com> > Acked-by: Michal Simek <michal.simek@xilinx.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > +/** > + * 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; > +} You say the gpio controller can configure pins as inputs or outputs. These pins are controller via firmware driver. We are updating BOOT_PIN_CTRL 0xFF5E0250 register. [0:3] = When 0, the pins will be inputs from the board to the PS. When 1, the PS will drive these pins [4:7] = Value captured from the mode pins [8:11] = Value driven onto the mode pins, when control register bit set out = 1 The lower four-bits [0:3] we can set either input and output, based on configuration we read pin as for input [4:7] and write on pin [8:11]. Example: If we want to configure pin 1 as output, then we will configure as [0:3]=[0100], for access pin will trigger upper bit [8:11]=[0100]. Based on https://www.xilinx.com/support/documentation/user_guides/ug1085-zynq-ultrascale-trm.pdf page 46 PS_MODE Input/Output Dedicated 4-bit boot mode pins sampled on POR deassertion Xilinx is using this pin for usb phy resets. Yet, .direction_input is doing nothing. So, it's not clear to me, how this sequence could work: - set gpio output high (writes bootmode) - set gpio to input (no-op, pin will remain high, not high impedance) I didn't check the previous discussions, but if this indeed works as intended, the how should be written here into the driver. That is a more useful comment than kernel doc for a stub function.
Hello Piyush, On 18.08.21 12:09, Piyush Mehta wrote: > Hi Ahmad, > > -----Original Message----- > From: Ahmad Fatoum <a.fatoum@pengutronix.de> > Sent: Wednesday, August 18, 2021 2:22 PM > To: Piyush Mehta <piyushm@xilinx.com>; arnd@arndb.de; zou_wei@huawei.com; gregkh@linuxfoundation.org; linus.walleij@linaro.org; Michal Simek <michals@xilinx.com>; Jiaying Liang <jliang@xilinx.com>; iwamatsu@nigauri.org; bgolaszewski@baylibre.com; robh+dt@kernel.org; Rajan Vaja <RAJANV@xilinx.com> > Cc: linux-gpio@vger.kernel.org; devicetree@vger.kernel.org; git <git@xilinx.com>; Srinivas Goud <sgoud@xilinx.com>; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Pengutronix Kernel Team <kernel@pengutronix.de> > Subject: Re: [PATCH V3 3/3] gpio: modepin: Add driver support for modepin GPIO controller > > On 18.08.21 10:10, Piyush Mehta 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 <piyush.mehta@xilinx.com> >> Acked-by: Michal Simek <michal.simek@xilinx.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> --- > >> +/** >> + * 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; >> +} > > You say the gpio controller can configure pins as inputs or outputs. > These pins are controller via firmware driver. We are updating BOOT_PIN_CTRL 0xFF5E0250 register. > [0:3] = When 0, the pins will be inputs from the board to the PS. When 1, the PS will drive these pins Ok. So if you want to configure the pin as input, you should call zynqmp_pm_bootmode_write to write a zero into that bit. But there's only one zynqmp_pm_bootmode_write in the GPIO driver and it's in modepin_gpio_set_value, which does output, not input. If I understand you right, there should be a modepin_gpio_set_value in modepin_gpio_dir_in as well? > Yet, .direction_input is doing nothing. So, it's not clear to me, how this sequence could work: > > - set gpio output high (writes bootmode) > - set gpio to input (no-op, pin will remain high, not high impedance) This is a valid sequence for a GPIO consumer and I don't see how this GPIO driver could honor it. Could you clarify? Cheers, Ahmad > > > > > > > I didn't check the previous discussions, but if this indeed works as intended, the how should be written here into the driver. That is a more useful comment than kernel doc for a stub function. >
On Wed, Aug 18, 2021 at 10:11 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> > Acked-by: Michal Simek <michal.simek@xilinx.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- Which tree should this go through? Bart
Hi Bart, On 8/23/21 10:02 AM, Bartosz Golaszewski wrote: > On Wed, Aug 18, 2021 at 10:11 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> >> Acked-by: Michal Simek <michal.simek@xilinx.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> --- > > Which tree should this go through? I would prefer to go this via gpio tree. Thanks, Michal
On Mon, Aug 23, 2021 at 10:14 AM Michal Simek <michal.simek@xilinx.com> wrote: > > Hi Bart, > > On 8/23/21 10:02 AM, Bartosz Golaszewski wrote: > > On Wed, Aug 18, 2021 at 10:11 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> > >> Acked-by: Michal Simek <michal.simek@xilinx.com> > >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > >> --- > > > > Which tree should this go through? > > I would prefer to go this via gpio tree. > > Thanks, > Michal Sure, just make sure to get an Ack from Rob Herring on the DT bindings. Bart
On Wed, Sep 22, 2021 at 12:18 PM Bartosz Golaszewski <bgolaszewski@baylibre.com> wrote: > > On Mon, Aug 23, 2021 at 10:14 AM Michal Simek <michal.simek@xilinx.com> wrote: > > > > Hi Bart, > > > > On 8/23/21 10:02 AM, Bartosz Golaszewski wrote: > > > On Wed, Aug 18, 2021 at 10:11 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> > > >> Acked-by: Michal Simek <michal.simek@xilinx.com> > > >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > >> --- > > > > > > Which tree should this go through? > > > > I would prefer to go this via gpio tree. > > > > Thanks, > > Michal > > Sure, just make sure to get an Ack from Rob Herring on the DT bindings. > > Bart Nevermind - it's already there. Bart
On 9/22/21 12:21 PM, Bartosz Golaszewski wrote: > On Wed, Sep 22, 2021 at 12:18 PM Bartosz Golaszewski > <bgolaszewski@baylibre.com> wrote: >> >> On Mon, Aug 23, 2021 at 10:14 AM Michal Simek <michal.simek@xilinx.com> wrote: >>> >>> Hi Bart, >>> >>> On 8/23/21 10:02 AM, Bartosz Golaszewski wrote: >>>> On Wed, Aug 18, 2021 at 10:11 AM Piyush Mehta <piyush.mehta@xilinx.com> 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 <piyush.mehta@xilinx.com> >>>>> Acked-by: Michal Simek <michal.simek@xilinx.com> >>>>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>>>> --- >>>> >>>> Which tree should this go through? >>> >>> I would prefer to go this via gpio tree. >>> >>> Thanks, >>> Michal >> >> Sure, just make sure to get an Ack from Rob Herring on the DT bindings. >> >> Bart > > Nevermind - it's already there. yes. that's what I thought. :-) Cheers, Michal
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..d52e391 --- /dev/null +++ b/drivers/gpio/gpio-zynqmp-modepin.c @@ -0,0 +1,153 @@ +// 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/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(®val); + if (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; + + zynqmp_pm_bootmode_read(&bootpin_val); + + 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: set value error %d for pin %d\n", ret, pin); +} + +/** + * 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 <piyush.mehta@xilinx.com>"); +MODULE_DESCRIPTION("ZynqMP Boot PS_MODE Configuration"); +MODULE_LICENSE("GPL v2");