* [U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function()
@ 2018-07-23 11:43 Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 2/4] gpio: xilinx: Swap xilinx_gpio_get_function with xilinx_gpio_get_value Michal Simek
` (2 more replies)
0 siblings, 3 replies; 19+ messages in thread
From: Michal Simek @ 2018-07-23 11:43 UTC (permalink / raw)
To: u-boot
Call xilinx_gpio_get_bank_pin() before use.
Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/gpio/xilinx_gpio.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
index 48b52c985a55..8ce08d80f491 100644
--- a/drivers/gpio/xilinx_gpio.c
+++ b/drivers/gpio/xilinx_gpio.c
@@ -435,6 +435,10 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
int val, ret;
u32 bank, pin;
+ ret = xilinx_gpio_get_bank_pin(offset, &bank, &pin, dev);
+ if (ret)
+ return ret;
+
/* Check if all pins are inputs */
if (platdata->bank_input[bank])
return GPIOF_INPUT;
@@ -443,10 +447,6 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
if (platdata->bank_output[bank])
return GPIOF_OUTPUT;
- ret = xilinx_gpio_get_bank_pin(offset, &bank, &pin, dev);
- if (ret)
- return ret;
-
/* FIXME test on dual */
val = readl(&platdata->regs->gpiodir + bank * 2);
val = !(val & (1 << pin));
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 2/4] gpio: xilinx: Swap xilinx_gpio_get_function with xilinx_gpio_get_value
2018-07-23 11:43 [U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function() Michal Simek
@ 2018-07-23 11:43 ` Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 4/4] gpio: xilinx: Remove !DM driver Michal Simek
2 siblings, 0 replies; 19+ messages in thread
From: Michal Simek @ 2018-07-23 11:43 UTC (permalink / raw)
To: u-boot
Exchange two functions to avoid function declaration for next patch.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++-------------------
1 file changed, 19 insertions(+), 19 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
index 8ce08d80f491..4da9ae114d87 100644
--- a/drivers/gpio/xilinx_gpio.c
+++ b/drivers/gpio/xilinx_gpio.c
@@ -410,25 +410,6 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
return val;
};
-static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
-{
- struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
- int val, ret;
- u32 bank, pin;
-
- ret = xilinx_gpio_get_bank_pin(offset, &bank, &pin, dev);
- if (ret)
- return ret;
-
- debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
- (ulong)platdata->regs, offset, bank, pin);
-
- val = readl(&platdata->regs->gpiodata + bank * 2);
- val = !!(val & (1 << pin));
-
- return val;
-};
-
static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
{
struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
@@ -457,6 +438,25 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
return val;
}
+static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
+{
+ struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
+ int val, ret;
+ u32 bank, pin;
+
+ ret = xilinx_gpio_get_bank_pin(offset, &bank, &pin, dev);
+ if (ret)
+ return ret;
+
+ debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
+ (ulong)platdata->regs, offset, bank, pin);
+
+ val = readl(&platdata->regs->gpiodata + bank * 2);
+ val = !!(val & (1 << pin));
+
+ return val;
+};
+
static int xilinx_gpio_direction_output(struct udevice *dev, unsigned offset,
int value)
{
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-23 11:43 [U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function() Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 2/4] gpio: xilinx: Swap xilinx_gpio_get_function with xilinx_gpio_get_value Michal Simek
@ 2018-07-23 11:43 ` Michal Simek
2018-07-23 18:42 ` Stefan Herbrechtsmeier
2018-07-23 11:43 ` [U-Boot] [PATCH 4/4] gpio: xilinx: Remove !DM driver Michal Simek
2 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-23 11:43 UTC (permalink / raw)
To: u-boot
Reading registers for finding out output value is not working because
input value is read instead in case of tristate.
Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++++++++++++++++-----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
index 4da9ae114d87..9d3e9379d0e5 100644
--- a/drivers/gpio/xilinx_gpio.c
+++ b/drivers/gpio/xilinx_gpio.c
@@ -358,6 +358,11 @@ struct xilinx_gpio_platdata {
int bank_max[XILINX_GPIO_MAX_BANK];
int bank_input[XILINX_GPIO_MAX_BANK];
int bank_output[XILINX_GPIO_MAX_BANK];
+ u32 dout_default[XILINX_GPIO_MAX_BANK];
+};
+
+struct xilinx_gpio_privdata {
+ u32 output_val[XILINX_GPIO_MAX_BANK];
};
static int xilinx_gpio_get_bank_pin(unsigned offset, u32 *bank_num,
@@ -387,6 +392,7 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
int value)
{
struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
+ struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
int val, ret;
u32 bank, pin;
@@ -394,19 +400,21 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
if (ret)
return ret;
- debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n",
- __func__, (ulong)platdata->regs, value, offset, bank, pin);
+ val = priv->output_val[bank];
+
+ debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x, out %x\n",
+ __func__, (ulong)platdata->regs, value, offset, bank, pin, val);
if (value) {
- val = readl(&platdata->regs->gpiodata + bank * 2);
val = val | (1 << pin);
writel(val, &platdata->regs->gpiodata + bank * 2);
} else {
- val = readl(&platdata->regs->gpiodata + bank * 2);
val = val & ~(1 << pin);
writel(val, &platdata->regs->gpiodata + bank * 2);
}
+ priv->output_val[bank] = val;
+
return val;
};
@@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
{
struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
+ struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
int val, ret;
u32 bank, pin;
@@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
(ulong)platdata->regs, offset, bank, pin);
- val = readl(&platdata->regs->gpiodata + bank * 2);
+ if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
+ debug("%s: Read input value from reg\n", __func__);
+ val = readl(&platdata->regs->gpiodata + bank * 2);
+ } else {
+ debug("%s: Read saved output value\n", __func__);
+ val = priv->output_val[bank];
+ }
+
val = !!(val & (1 << pin));
return val;
@@ -558,11 +574,17 @@ static int xilinx_gpio_probe(struct udevice *dev)
{
struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
+ struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
uc_priv->bank_name = dev->name;
uc_priv->gpio_count = platdata->bank_max[0] + platdata->bank_max[1];
+ priv->output_val[0] = platdata->dout_default[0];
+
+ if (platdata->bank_max[1])
+ priv->output_val[1] = platdata->dout_default[1];
+
return 0;
}
@@ -579,6 +601,9 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
"xlnx,all-inputs", 0);
platdata->bank_output[0] = dev_read_u32_default(dev,
"xlnx,all-outputs", 0);
+ platdata->dout_default[0] = dev_read_u32_default(dev,
+ "xlnx,dout-default",
+ 0);
is_dual = dev_read_u32_default(dev, "xlnx,is-dual", 0);
if (is_dual) {
@@ -588,6 +613,8 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
"xlnx,all-inputs-2", 0);
platdata->bank_output[1] = dev_read_u32_default(dev,
"xlnx,all-outputs-2", 0);
+ platdata->dout_default[1] = dev_read_u32_default(dev,
+ "xlnx,dout-default-2", 0);
}
return 0;
@@ -606,5 +633,6 @@ U_BOOT_DRIVER(xilinx_gpio) = {
.ofdata_to_platdata = xilinx_gpio_ofdata_to_platdata,
.probe = xilinx_gpio_probe,
.platdata_auto_alloc_size = sizeof(struct xilinx_gpio_platdata),
+ .priv_auto_alloc_size = sizeof(struct xilinx_gpio_privdata),
};
#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 4/4] gpio: xilinx: Remove !DM driver
2018-07-23 11:43 [U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function() Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 2/4] gpio: xilinx: Swap xilinx_gpio_get_function with xilinx_gpio_get_value Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs Michal Simek
@ 2018-07-23 11:43 ` Michal Simek
2 siblings, 0 replies; 19+ messages in thread
From: Michal Simek @ 2018-07-23 11:43 UTC (permalink / raw)
To: u-boot
There is no user for !DM driver that's why remove it.
Signed-off-by: Michal Simek <michal.simek@xilinx.com>
---
drivers/gpio/xilinx_gpio.c | 338 +--------------------------------------------
1 file changed, 2 insertions(+), 336 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
index 9d3e9379d0e5..2bd4cf331518 100644
--- a/drivers/gpio/xilinx_gpio.c
+++ b/drivers/gpio/xilinx_gpio.c
@@ -10,13 +10,9 @@
#include <asm/io.h>
#include <asm/gpio.h>
#include <dm.h>
+#include <dt-bindings/gpio/gpio.h>
-static LIST_HEAD(gpio_list);
-
-enum gpio_direction {
- GPIO_DIRECTION_OUT = 0,
- GPIO_DIRECTION_IN = 1,
-};
+#define XILINX_GPIO_MAX_BANK 2
/* Gpio simple map */
struct gpio_regs {
@@ -24,335 +20,6 @@ struct gpio_regs {
u32 gpiodir;
};
-#if !defined(CONFIG_DM_GPIO)
-
-#define GPIO_NAME_SIZE 10
-
-struct gpio_names {
- char name[GPIO_NAME_SIZE];
-};
-
-/* Initialized, rxbd_current, rx_first_buf must be 0 after init */
-struct xilinx_gpio_priv {
- struct gpio_regs *regs;
- u32 gpio_min;
- u32 gpio_max;
- u32 gpiodata_store;
- char name[GPIO_NAME_SIZE];
- struct list_head list;
- struct gpio_names *gpio_name;
-};
-
-/* Store number of allocated gpio pins */
-static u32 xilinx_gpio_max;
-
-/* Get associated gpio controller */
-static struct xilinx_gpio_priv *gpio_get_controller(unsigned gpio)
-{
- struct list_head *entry;
- struct xilinx_gpio_priv *priv = NULL;
-
- list_for_each(entry, &gpio_list) {
- priv = list_entry(entry, struct xilinx_gpio_priv, list);
- if (gpio >= priv->gpio_min && gpio <= priv->gpio_max) {
- debug("%s: reg: %x, min-max: %d-%d\n", __func__,
- (u32)priv->regs, priv->gpio_min, priv->gpio_max);
- return priv;
- }
- }
- puts("!!!Can't get gpio controller!!!\n");
- return NULL;
-}
-
-/* Get gpio pin name if used/setup */
-static char *get_name(unsigned gpio)
-{
- u32 gpio_priv;
- struct xilinx_gpio_priv *priv;
-
- debug("%s\n", __func__);
-
- priv = gpio_get_controller(gpio);
- if (priv) {
- gpio_priv = gpio - priv->gpio_min;
-
- return *priv->gpio_name[gpio_priv].name ?
- priv->gpio_name[gpio_priv].name : "UNKNOWN";
- }
- return "UNKNOWN";
-}
-
-/* Get output value */
-static int gpio_get_output_value(unsigned gpio)
-{
- u32 val, gpio_priv;
- struct xilinx_gpio_priv *priv = gpio_get_controller(gpio);
-
- if (priv) {
- gpio_priv = gpio - priv->gpio_min;
- val = !!(priv->gpiodata_store & (1 << gpio_priv));
- debug("%s: reg: %x, gpio_no: %d, dir: %d\n", __func__,
- (u32)priv->regs, gpio_priv, val);
-
- return val;
- }
- return -1;
-}
-
-/* Get input value */
-static int gpio_get_input_value(unsigned gpio)
-{
- u32 val, gpio_priv;
- struct gpio_regs *regs;
- struct xilinx_gpio_priv *priv = gpio_get_controller(gpio);
-
- if (priv) {
- regs = priv->regs;
- gpio_priv = gpio - priv->gpio_min;
- val = readl(®s->gpiodata);
- val = !!(val & (1 << gpio_priv));
- debug("%s: reg: %x, gpio_no: %d, dir: %d\n", __func__,
- (u32)priv->regs, gpio_priv, val);
-
- return val;
- }
- return -1;
-}
-
-/* Set gpio direction */
-static int gpio_set_direction(unsigned gpio, enum gpio_direction direction)
-{
- u32 val, gpio_priv;
- struct gpio_regs *regs;
- struct xilinx_gpio_priv *priv = gpio_get_controller(gpio);
-
- if (priv) {
- regs = priv->regs;
- val = readl(®s->gpiodir);
-
- gpio_priv = gpio - priv->gpio_min;
- if (direction == GPIO_DIRECTION_OUT)
- val &= ~(1 << gpio_priv);
- else
- val |= 1 << gpio_priv;
-
- writel(val, ®s->gpiodir);
- debug("%s: reg: %x, gpio_no: %d, dir: %d\n", __func__,
- (u32)priv->regs, gpio_priv, val);
-
- return 0;
- }
-
- return -1;
-}
-
-/* Get gpio direction */
-static int gpio_get_direction(unsigned gpio)
-{
- u32 val, gpio_priv;
- struct gpio_regs *regs;
- struct xilinx_gpio_priv *priv = gpio_get_controller(gpio);
-
- if (priv) {
- regs = priv->regs;
- gpio_priv = gpio - priv->gpio_min;
- val = readl(®s->gpiodir);
- val = !!(val & (1 << gpio_priv));
- debug("%s: reg: %x, gpio_no: %d, dir: %d\n", __func__,
- (u32)priv->regs, gpio_priv, val);
-
- return val;
- }
-
- return -1;
-}
-
-/*
- * Get input value
- * for example gpio setup to output only can't get input value
- * which is breaking gpio toggle command
- */
-int gpio_get_value(unsigned gpio)
-{
- u32 val;
-
- if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
- val = gpio_get_output_value(gpio);
- else
- val = gpio_get_input_value(gpio);
-
- return val;
-}
-
-/* Set output value */
-static int gpio_set_output_value(unsigned gpio, int value)
-{
- u32 val, gpio_priv;
- struct gpio_regs *regs;
- struct xilinx_gpio_priv *priv = gpio_get_controller(gpio);
-
- if (priv) {
- regs = priv->regs;
- gpio_priv = gpio - priv->gpio_min;
- val = priv->gpiodata_store;
- if (value)
- val |= 1 << gpio_priv;
- else
- val &= ~(1 << gpio_priv);
-
- writel(val, ®s->gpiodata);
- debug("%s: reg: %x, gpio_no: %d, output_val: %d\n", __func__,
- (u32)priv->regs, gpio_priv, val);
- priv->gpiodata_store = val;
-
- return 0;
- }
-
- return -1;
-}
-
-int gpio_set_value(unsigned gpio, int value)
-{
- if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
- return gpio_set_output_value(gpio, value);
-
- return -1;
-}
-
-/* Set GPIO as input */
-int gpio_direction_input(unsigned gpio)
-{
- debug("%s\n", __func__);
- return gpio_set_direction(gpio, GPIO_DIRECTION_IN);
-}
-
-/* Setup GPIO as output and set output value */
-int gpio_direction_output(unsigned gpio, int value)
-{
- int ret = gpio_set_direction(gpio, GPIO_DIRECTION_OUT);
-
- debug("%s\n", __func__);
-
- if (ret < 0)
- return ret;
-
- return gpio_set_output_value(gpio, value);
-}
-
-/* Show gpio status */
-void gpio_info(void)
-{
- unsigned gpio;
-
- struct list_head *entry;
- struct xilinx_gpio_priv *priv = NULL;
-
- list_for_each(entry, &gpio_list) {
- priv = list_entry(entry, struct xilinx_gpio_priv, list);
- printf("\n%s: %s/%x (%d-%d)\n", __func__, priv->name,
- (u32)priv->regs, priv->gpio_min, priv->gpio_max);
-
- for (gpio = priv->gpio_min; gpio <= priv->gpio_max; gpio++) {
- printf("GPIO_%d:\t%s is an ", gpio, get_name(gpio));
- if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
- printf("OUTPUT value = %d\n",
- gpio_get_output_value(gpio));
- else
- printf("INPUT value = %d\n",
- gpio_get_input_value(gpio));
- }
- }
-}
-
-int gpio_request(unsigned gpio, const char *label)
-{
- u32 gpio_priv;
- struct xilinx_gpio_priv *priv;
-
- if (gpio >= xilinx_gpio_max)
- return -EINVAL;
-
- priv = gpio_get_controller(gpio);
- if (priv) {
- gpio_priv = gpio - priv->gpio_min;
-
- if (label != NULL) {
- strncpy(priv->gpio_name[gpio_priv].name, label,
- GPIO_NAME_SIZE);
- priv->gpio_name[gpio_priv].name[GPIO_NAME_SIZE - 1] =
- '\0';
- }
- return 0;
- }
-
- return -1;
-}
-
-int gpio_free(unsigned gpio)
-{
- u32 gpio_priv;
- struct xilinx_gpio_priv *priv;
-
- if (gpio >= xilinx_gpio_max)
- return -EINVAL;
-
- priv = gpio_get_controller(gpio);
- if (priv) {
- gpio_priv = gpio - priv->gpio_min;
- priv->gpio_name[gpio_priv].name[0] = '\0';
-
- /* Do nothing here */
- return 0;
- }
-
- return -1;
-}
-
-int gpio_alloc(u32 baseaddr, const char *name, u32 gpio_no)
-{
- struct xilinx_gpio_priv *priv;
-
- priv = calloc(1, sizeof(struct xilinx_gpio_priv));
-
- /* Setup gpio name */
- if (name != NULL) {
- strncpy(priv->name, name, GPIO_NAME_SIZE);
- priv->name[GPIO_NAME_SIZE - 1] = '\0';
- }
- priv->regs = (struct gpio_regs *)baseaddr;
-
- priv->gpio_min = xilinx_gpio_max;
- xilinx_gpio_max = priv->gpio_min + gpio_no;
- priv->gpio_max = xilinx_gpio_max - 1;
-
- priv->gpio_name = calloc(gpio_no, sizeof(struct gpio_names));
-
- INIT_LIST_HEAD(&priv->list);
- list_add_tail(&priv->list, &gpio_list);
-
- printf("%s: Add %s (%d-%d)\n", __func__, name,
- priv->gpio_min, priv->gpio_max);
-
- /* Return the first gpio allocated for this device */
- return priv->gpio_min;
-}
-
-/* Dual channel gpio is one IP with two independent channels */
-int gpio_alloc_dual(u32 baseaddr, const char *name, u32 gpio_no0, u32 gpio_no1)
-{
- int ret;
-
- ret = gpio_alloc(baseaddr, name, gpio_no0);
- gpio_alloc(baseaddr + 8, strcat((char *)name, "_1"), gpio_no1);
-
- /* Return the first gpio allocated for this device */
- return ret;
-}
-#else
-#include <dt-bindings/gpio/gpio.h>
-
-#define XILINX_GPIO_MAX_BANK 2
-
struct xilinx_gpio_platdata {
struct gpio_regs *regs;
int bank_max[XILINX_GPIO_MAX_BANK];
@@ -635,4 +302,3 @@ U_BOOT_DRIVER(xilinx_gpio) = {
.platdata_auto_alloc_size = sizeof(struct xilinx_gpio_platdata),
.priv_auto_alloc_size = sizeof(struct xilinx_gpio_privdata),
};
-#endif
--
1.9.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-23 11:43 ` [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs Michal Simek
@ 2018-07-23 18:42 ` Stefan Herbrechtsmeier
2018-07-24 10:31 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-23 18:42 UTC (permalink / raw)
To: u-boot
Hi Michal,
Am 23.07.2018 um 13:43 schrieb Michal Simek:
> Reading registers for finding out output value is not working because
> input value is read instead in case of tristate.
>
> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
> ---
>
> drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++++++++++++++++-----
> 1 file changed, 33 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
> index 4da9ae114d87..9d3e9379d0e5 100644
> --- a/drivers/gpio/xilinx_gpio.c
> +++ b/drivers/gpio/xilinx_gpio.c
> @@ -358,6 +358,11 @@ struct xilinx_gpio_platdata {
> int bank_max[XILINX_GPIO_MAX_BANK];
> int bank_input[XILINX_GPIO_MAX_BANK];
> int bank_output[XILINX_GPIO_MAX_BANK];
> + u32 dout_default[XILINX_GPIO_MAX_BANK];
> +};
> +
> +struct xilinx_gpio_privdata {
> + u32 output_val[XILINX_GPIO_MAX_BANK];
> };
>
> static int xilinx_gpio_get_bank_pin(unsigned offset, u32 *bank_num,
> @@ -387,6 +392,7 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
> int value)
> {
> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
> int val, ret;
> u32 bank, pin;
>
> @@ -394,19 +400,21 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset,
> if (ret)
> return ret;
>
> - debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n",
> - __func__, (ulong)platdata->regs, value, offset, bank, pin);
> + val = priv->output_val[bank];
> +
> + debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x, out %x\n",
> + __func__, (ulong)platdata->regs, value, offset, bank, pin, val);
>
> if (value) {
> - val = readl(&platdata->regs->gpiodata + bank * 2);
> val = val | (1 << pin);
> writel(val, &platdata->regs->gpiodata + bank * 2);
> } else {
> - val = readl(&platdata->regs->gpiodata + bank * 2);
> val = val & ~(1 << pin);
> writel(val, &platdata->regs->gpiodata + bank * 2);
> }
You could replace the two writel function calls by one.
>
> + priv->output_val[bank] = val;
> +
> return val;
> };
>
> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct udevice *dev, unsigned offset)
> static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
> {
> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
> int val, ret;
> u32 bank, pin;
>
> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
> (ulong)platdata->regs, offset, bank, pin);
>
> - val = readl(&platdata->regs->gpiodata + bank * 2);
> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
> + debug("%s: Read input value from reg\n", __func__);
> + val = readl(&platdata->regs->gpiodata + bank * 2);
> + } else {
> + debug("%s: Read saved output value\n", __func__);
> + val = priv->output_val[bank];
> + }
Why you don't always read the data register? This doesn't work for three
state outputs.
> +
> val = !!(val & (1 << pin));
>
> return val;
> @@ -558,11 +574,17 @@ static int xilinx_gpio_probe(struct udevice *dev)
> {
> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
> struct gpio_dev_priv *uc_priv = dev_get_uclass_priv(dev);
> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>
> uc_priv->bank_name = dev->name;
>
> uc_priv->gpio_count = platdata->bank_max[0] + platdata->bank_max[1];
>
> + priv->output_val[0] = platdata->dout_default[0];
> +
> + if (platdata->bank_max[1])
> + priv->output_val[1] = platdata->dout_default[1];
> +
> return 0;
> }
>
> @@ -579,6 +601,9 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
> "xlnx,all-inputs", 0);
> platdata->bank_output[0] = dev_read_u32_default(dev,
> "xlnx,all-outputs", 0);
> + platdata->dout_default[0] = dev_read_u32_default(dev,
> + "xlnx,dout-default",
> + 0);
>
> is_dual = dev_read_u32_default(dev, "xlnx,is-dual", 0);
> if (is_dual) {
> @@ -588,6 +613,8 @@ static int xilinx_gpio_ofdata_to_platdata(struct udevice *dev)
> "xlnx,all-inputs-2", 0);
> platdata->bank_output[1] = dev_read_u32_default(dev,
> "xlnx,all-outputs-2", 0);
> + platdata->dout_default[1] = dev_read_u32_default(dev,
> + "xlnx,dout-default-2", 0);
> }
>
> return 0;
> @@ -606,5 +633,6 @@ U_BOOT_DRIVER(xilinx_gpio) = {
> .ofdata_to_platdata = xilinx_gpio_ofdata_to_platdata,
> .probe = xilinx_gpio_probe,
> .platdata_auto_alloc_size = sizeof(struct xilinx_gpio_platdata),
> + .priv_auto_alloc_size = sizeof(struct xilinx_gpio_privdata),
> };
> #endif
Best regards
 Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-23 18:42 ` Stefan Herbrechtsmeier
@ 2018-07-24 10:31 ` Michal Simek
2018-07-24 19:56 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-24 10:31 UTC (permalink / raw)
To: u-boot
On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
> Hi Michal,
>
> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>> Reading registers for finding out output value is not working because
>> input value is read instead in case of tristate.
>>
>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>> ---
>>
>> drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++++++++++++++++-----
>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>> index 4da9ae114d87..9d3e9379d0e5 100644
>> --- a/drivers/gpio/xilinx_gpio.c
>> +++ b/drivers/gpio/xilinx_gpio.c
>> @@ -358,6 +358,11 @@ struct xilinx_gpio_platdata {
>> int bank_max[XILINX_GPIO_MAX_BANK];
>> int bank_input[XILINX_GPIO_MAX_BANK];
>> int bank_output[XILINX_GPIO_MAX_BANK];
>> + u32 dout_default[XILINX_GPIO_MAX_BANK];
>> +};
>> +
>> +struct xilinx_gpio_privdata {
>> + u32 output_val[XILINX_GPIO_MAX_BANK];
>> };
>> static int xilinx_gpio_get_bank_pin(unsigned offset, u32 *bank_num,
>> @@ -387,6 +392,7 @@ static int xilinx_gpio_set_value(struct udevice
>> *dev, unsigned offset,
>> int value)
>> {
>> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>> int val, ret;
>> u32 bank, pin;
>> @@ -394,19 +400,21 @@ static int xilinx_gpio_set_value(struct
>> udevice *dev, unsigned offset,
>> if (ret)
>> return ret;
>> - debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n",
>> - __func__, (ulong)platdata->regs, value, offset, bank, pin);
>> + val = priv->output_val[bank];
>> +
>> + debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x, out
>> %x\n",
>> + __func__, (ulong)platdata->regs, value, offset, bank, pin,
>> val);
>> if (value) {
>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>> val = val | (1 << pin);
>> writel(val, &platdata->regs->gpiodata + bank * 2);
>> } else {
>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>> val = val & ~(1 << pin);
>> writel(val, &platdata->regs->gpiodata + bank * 2);
>> }
>
> You could replace the two writel function calls by one.
>
Will update this in v2.
>> + priv->output_val[bank] = val;
>> +
>> return val;
>> };
>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>> udevice *dev, unsigned offset)
>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
>> {
>> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>> int val, ret;
>> u32 bank, pin;
>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice
>> *dev, unsigned offset)
>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>> (ulong)platdata->regs, offset, bank, pin);
>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>> + debug("%s: Read input value from reg\n", __func__);
>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>> + } else {
>> + debug("%s: Read saved output value\n", __func__);
>> + val = priv->output_val[bank];
>> + }
>
> Why you don't always read the data register? This doesn't work for three
> state outputs.
In three state register every bit/pin is 0 - output, 1 input.
It means else part is output and I read saved value in priv->output_val.
If pin is setup as INPUT then I need read data reg to find out input value.
Maybe you are commenting something else but please let me know if there
is any other bug.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-24 10:31 ` Michal Simek
@ 2018-07-24 19:56 ` Stefan Herbrechtsmeier
2018-07-25 6:39 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-24 19:56 UTC (permalink / raw)
To: u-boot
Am 24.07.2018 um 12:31 schrieb Michal Simek:
> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>> Reading registers for finding out output value is not working because
>>> input value is read instead in case of tristate.
>>>
>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>> ---
>>>
>>> drivers/gpio/xilinx_gpio.c | 38 +++++++++++++++++++++++++++++++++-----
>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>> --- a/drivers/gpio/xilinx_gpio.c
>>> +++ b/drivers/gpio/xilinx_gpio.c
[snip]
>>> + priv->output_val[bank] = val;
>>> +
>>> return val;
>>> };
>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>> udevice *dev, unsigned offset)
>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned offset)
>>> {
>>> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>> int val, ret;
>>> u32 bank, pin;
>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice
>>> *dev, unsigned offset)
>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>>> (ulong)platdata->regs, offset, bank, pin);
>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>> + debug("%s: Read input value from reg\n", __func__);
>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>> + } else {
>>> + debug("%s: Read saved output value\n", __func__);
>>> + val = priv->output_val[bank];
>>> + }
>> Why you don't always read the data register? This doesn't work for three
>> state outputs.
> In three state register every bit/pin is 0 - output, 1 input.
> It means else part is output and I read saved value in priv->output_val.
> If pin is setup as INPUT then I need read data reg to find out input value.
> Maybe you are commenting something else but please let me know if there
> is any other bug.
What happen if I have an open drain output. Even if the gpio output is 1
the input could read a 0. You driver will always return the output value
and not the real input value. According to the picture in documentation
and my tests a data register write writes the output registers and a
data register read reads the input registers.
Why should the driver return the desired state (output register) and not
the real state (input register)?
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-24 19:56 ` Stefan Herbrechtsmeier
@ 2018-07-25 6:39 ` Michal Simek
2018-07-25 18:21 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-25 6:39 UTC (permalink / raw)
To: u-boot
On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>> Reading registers for finding out output value is not working because
>>>> input value is read instead in case of tristate.
>>>>
>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>> ---
>>>>
>>>> drivers/gpio/xilinx_gpio.c | 38
>>>> +++++++++++++++++++++++++++++++++-----
>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>
>>>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>> +++ b/drivers/gpio/xilinx_gpio.c
>
> [snip]
>
>>>> + priv->output_val[bank] = val;
>>>> +
>>>> return val;
>>>> };
>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>> udevice *dev, unsigned offset)
>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>> offset)
>>>> {
>>>> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>> int val, ret;
>>>> u32 bank, pin;
>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice
>>>> *dev, unsigned offset)
>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>>>> (ulong)platdata->regs, offset, bank, pin);
>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>> + debug("%s: Read input value from reg\n", __func__);
>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>> + } else {
>>>> + debug("%s: Read saved output value\n", __func__);
>>>> + val = priv->output_val[bank];
>>>> + }
>>> Why you don't always read the data register? This doesn't work for three
>>> state outputs.
>> In three state register every bit/pin is 0 - output, 1 input.
>> It means else part is output and I read saved value in priv->output_val.
>> If pin is setup as INPUT then I need read data reg to find out input
>> value.
>> Maybe you are commenting something else but please let me know if there
>> is any other bug.
>
> What happen if I have an open drain output. Even if the gpio output is 1
> the input could read a 0. You driver will always return the output value
> and not the real input value. According to the picture in documentation
> and my tests a data register write writes the output registers and a
> data register read reads the input registers.
>
> Why should the driver return the desired state (output register) and not
> the real state (input register)?
First of all thanks for description.
I have another example where you have output only and you can't read
input because there is no wire.
Regarding open drain output.
Also this is what it is written in manual:
"AXI GPIO Data Register Description"
"For each I/O bit programmed as output:
• R: Reads to these bits always return zeros"
If you want to support this configuration then you would have to change
pin to input and read it and revert it back. And all of this should be
done based on flag.
There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
Unfortunately this is for consumer not for generic listing. I can't also
see it in gpio_desc flags to be able to handle it in the driver.
That's why I have doubts if this is supported by any u-boot gpio driver
but I can be wrong.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-25 6:39 ` Michal Simek
@ 2018-07-25 18:21 ` Stefan Herbrechtsmeier
2018-07-26 8:41 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-25 18:21 UTC (permalink / raw)
To: u-boot
Am 25.07.2018 um 08:39 schrieb Michal Simek:
> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>> Reading registers for finding out output value is not working because
>>>>> input value is read instead in case of tristate.
>>>>>
>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>> ---
>>>>>
>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>> +++++++++++++++++++++++++++++++++-----
>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>> [snip]
>>
>>>>> + priv->output_val[bank] = val;
>>>>> +
>>>>> return val;
>>>>> };
>>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>>> udevice *dev, unsigned offset)
>>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>>> offset)
>>>>> {
>>>>> struct xilinx_gpio_platdata *platdata = dev_get_platdata(dev);
>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>> int val, ret;
>>>>> u32 bank, pin;
>>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct udevice
>>>>> *dev, unsigned offset)
>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>> + } else {
>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>> + val = priv->output_val[bank];
>>>>> + }
>>>> Why you don't always read the data register? This doesn't work for three
>>>> state outputs.
>>> In three state register every bit/pin is 0 - output, 1 input.
>>> It means else part is output and I read saved value in priv->output_val.
>>> If pin is setup as INPUT then I need read data reg to find out input
>>> value.
>>> Maybe you are commenting something else but please let me know if there
>>> is any other bug.
>> What happen if I have an open drain output. Even if the gpio output is 1
>> the input could read a 0. You driver will always return the output value
>> and not the real input value. According to the picture in documentation
>> and my tests a data register write writes the output registers and a
>> data register read reads the input registers.
>>
>> Why should the driver return the desired state (output register) and not
>> the real state (input register)?
> First of all thanks for description.
>
> I have another example where you have output only and you can't read
> input because there is no wire.
Does you mean the all outputs configuration? Does this removes the
"gpio_io_i" signal from the IP?
> Regarding open drain output.
>
> Also this is what it is written in manual:
> "AXI GPIO Data Register Description"
> "For each I/O bit programmed as output:
> • R: Reads to these bits always return zeros"
This must be a mistake in the documentation. I could read the input value.
The old driver and the Linux driver always read the register.
Additionally the documentation states that a write to an input doesn't work:
AXI GPIO Data Register.
For each I/O bit programmed as input:
• R: Reads value on the input pin.
• W: No effect.
However the Linux driver use the common sequence and sets first the data
register and afterwards the direction register.
> If you want to support this configuration then you would have to change
> pin to input and read it and revert it back. And all of this should be
> done based on flag.
> There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
> Unfortunately this is for consumer not for generic listing. I can't also
> see it in gpio_desc flags to be able to handle it in the driver.
> That's why I have doubts if this is supported by any u-boot gpio driver
> but I can be wrong.
I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
case the open drain is transparent. The output has only two states
High-Z and 0. The input value of the FPGA output block is always 0 and
the tri-state input is connected to the output register of the GPIO
controller. The output of the FPGA input block is connected to the input
register of the GPIO controller.
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-25 18:21 ` Stefan Herbrechtsmeier
@ 2018-07-26 8:41 ` Michal Simek
2018-07-26 19:46 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-26 8:41 UTC (permalink / raw)
To: u-boot
On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>> Reading registers for finding out output value is not working because
>>>>>> input value is read instead in case of tristate.
>>>>>>
>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>> ---
>>>>>>
>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>> [snip]
>>>
>>>>>> + priv->output_val[bank] = val;
>>>>>> +
>>>>>> return val;
>>>>>> };
>>>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>>>> udevice *dev, unsigned offset)
>>>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>>>> offset)
>>>>>> {
>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>> dev_get_platdata(dev);
>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>> int val, ret;
>>>>>> u32 bank, pin;
>>>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct
>>>>>> udevice
>>>>>> *dev, unsigned offset)
>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>> + } else {
>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>> + val = priv->output_val[bank];
>>>>>> + }
>>>>> Why you don't always read the data register? This doesn't work for
>>>>> three
>>>>> state outputs.
>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>> It means else part is output and I read saved value in
>>>> priv->output_val.
>>>> If pin is setup as INPUT then I need read data reg to find out input
>>>> value.
>>>> Maybe you are commenting something else but please let me know if there
>>>> is any other bug.
>>> What happen if I have an open drain output. Even if the gpio output is 1
>>> the input could read a 0. You driver will always return the output value
>>> and not the real input value. According to the picture in documentation
>>> and my tests a data register write writes the output registers and a
>>> data register read reads the input registers.
>>>
>>> Why should the driver return the desired state (output register) and not
>>> the real state (input register)?
>> First of all thanks for description.
>>
>> I have another example where you have output only and you can't read
>> input because there is no wire.
>
> Does you mean the all outputs configuration? Does this removes the
> "gpio_io_i" signal from the IP?
I am not sure what synthesis is doing with that unused IP pins but I
would consider as a bug if this is automatically connected together. And
also wasting a logic if there is unused part.
But in Vivado you should be able to setup output pins to and input pins
separately. There are In/Out/Tristate.
If you don't want to deal with external pin you can connect them inside PL.
>> Regarding open drain output.
>>
>> Also this is what it is written in manual:
>> "AXI GPIO Data Register Description"
>> "For each I/O bit programmed as output:
>> • R: Reads to these bits always return zeros"
>
> This must be a mistake in the documentation. I could read the input value.
>
> The old driver and the Linux driver always read the register.
In old u-boot driver I see
179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
180 val = gpio_get_output_value(gpio);
181 else
182 val = gpio_get_input_value(gpio);
gpio_get_output_value() reads it from gpiodata_store for output
gpio_get_input_value() reads the reg.
In Linux you are right it is read from reg in both cases.
>
> Additionally the documentation states that a write to an input doesn't
> work:
> AXI GPIO Data Register.
> For each I/O bit programmed as input:
> • R: Reads value on the input pin.
> • W: No effect.
>
> However the Linux driver use the common sequence and sets first the data
> register and afterwards the direction register.
Possible that driver has pretty long history and I don't think it was
tested for all cases.
>> If you want to support this configuration then you would have to change
>> pin to input and read it and revert it back. And all of this should be
>> done based on flag.
>> There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
>> Unfortunately this is for consumer not for generic listing. I can't also
>> see it in gpio_desc flags to be able to handle it in the driver.
>> That's why I have doubts if this is supported by any u-boot gpio driver
>> but I can be wrong.
>
> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
> case the open drain is transparent. The output has only two states
> High-Z and 0. The input value of the FPGA output block is always 0 and
> the tri-state input is connected to the output register of the GPIO
> controller. The output of the FPGA input block is connected to the input
> register of the GPIO controller.
I just found out in connection to second thread we have together that in
gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
not wired anywhere.
I think in your case when these two functions are implemented you will
get this functionality which you are asking about.
My guess is that gpio command should be extended to call these two
functions. When set_open_drain is called special flag for pin is setup
and for this pin get_status will always read data reg in our case and
you get your values.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-26 8:41 ` Michal Simek
@ 2018-07-26 19:46 ` Stefan Herbrechtsmeier
2018-07-27 7:05 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-26 19:46 UTC (permalink / raw)
To: u-boot
Am 26.07.2018 um 10:41 schrieb Michal Simek:
> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>> Reading registers for finding out output value is not working because
>>>>>>> input value is read instead in case of tristate.
>>>>>>>
>>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>> ---
>>>>>>>
>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c
>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>> [snip]
>>>>
>>>>>>> + priv->output_val[bank] = val;
>>>>>>> +
>>>>>>> return val;
>>>>>>> };
>>>>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>>>>> udevice *dev, unsigned offset)
>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>>>>> offset)
>>>>>>> {
>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>> dev_get_platdata(dev);
>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>> int val, ret;
>>>>>>> u32 bank, pin;
>>>>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct
>>>>>>> udevice
>>>>>>> *dev, unsigned offset)
>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n", __func__,
>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>> + } else {
>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>> + val = priv->output_val[bank];
>>>>>>> + }
>>>>>> Why you don't always read the data register? This doesn't work for
>>>>>> three
>>>>>> state outputs.
>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>> It means else part is output and I read saved value in
>>>>> priv->output_val.
>>>>> If pin is setup as INPUT then I need read data reg to find out input
>>>>> value.
>>>>> Maybe you are commenting something else but please let me know if there
>>>>> is any other bug.
>>>> What happen if I have an open drain output. Even if the gpio output is 1
>>>> the input could read a 0. You driver will always return the output value
>>>> and not the real input value. According to the picture in documentation
>>>> and my tests a data register write writes the output registers and a
>>>> data register read reads the input registers.
>>>>
>>>> Why should the driver return the desired state (output register) and not
>>>> the real state (input register)?
>>> First of all thanks for description.
>>>
>>> I have another example where you have output only and you can't read
>>> input because there is no wire.
>> Does you mean the all outputs configuration? Does this removes the
>> "gpio_io_i" signal from the IP?
> I am not sure what synthesis is doing with that unused IP pins but I
> would consider as a bug if this is automatically connected together.
I mean does the IP generator removes the gpio_io_i signal because it
isn't needed? If the IP generator creates the gpio_io_i signal I would
expect that you can't leave it unconnected as this would lead to
undefined values.
> And
> also wasting a logic if there is unused part.
> But in Vivado you should be able to setup output pins to and input pins
> separately. There are In/Out/Tristate.
> If you don't want to deal with external pin you can connect them inside PL.
This isn't my point. I mean that if you have an gpio_io_i signal you
have to connected it to a signal. You could connect it to the output of
an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
connect it to a fixed value (0 or 1) you get this value if you read the
status of this GPIO.
>>> Regarding open drain output.
>>>
>>> Also this is what it is written in manual:
>>> "AXI GPIO Data Register Description"
>>> "For each I/O bit programmed as output:
>>> • R: Reads to these bits always return zeros"
>> This must be a mistake in the documentation. I could read the input value.
>>
>> The old driver and the Linux driver always read the register.
> In old u-boot driver I see
> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
> 180 val = gpio_get_output_value(gpio);
> 181 else
> 182 val = gpio_get_input_value(gpio);
>
> gpio_get_output_value() reads it from gpiodata_store for output
> gpio_get_input_value() reads the reg.
Sorry you are right. I mixed it with the zynq driver.
> In Linux you are right it is read from reg in both cases.
>
>> Additionally the documentation states that a write to an input doesn't
>> work:
>> AXI GPIO Data Register.
>> For each I/O bit programmed as input:
>> • R: Reads value on the input pin.
>> • W: No effect.
>>
>> However the Linux driver use the common sequence and sets first the data
>> register and afterwards the direction register.
> Possible that driver has pretty long history and I don't think it was
> tested for all cases.
I test the IP again and it works like expected and the documentation is
wrong. You could always write the output register and read the input
register. This means you could write the output first and afterwards
enable the output via the tri-state register.
>>> If you want to support this configuration then you would have to change
>>> pin to input and read it and revert it back. And all of this should be
>>> done based on flag.
>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
>>> Unfortunately this is for consumer not for generic listing. I can't also
>>> see it in gpio_desc flags to be able to handle it in the driver.
>>> That's why I have doubts if this is supported by any u-boot gpio driver
>>> but I can be wrong.
>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
>> case the open drain is transparent. The output has only two states
>> High-Z and 0. The input value of the FPGA output block is always 0 and
>> the tri-state input is connected to the output register of the GPIO
>> controller. The output of the FPGA input block is connected to the input
>> register of the GPIO controller.
> I just found out in connection to second thread we have together that in
> gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
> not wired anywhere.
> I think in your case when these two functions are implemented you will
> get this functionality which you are asking about.
No. This function is to control a special register.
Beside the open drain. What happens if the output is stuck to a fixed
signal. This could be detected if you read the input register.
> My guess is that gpio command should be extended to call these two
> functions. When set_open_drain is called special flag for pin is setup
> and for this pin get_status will always read data reg in our case and
> you get your values.
The main quest is which value should the get function return for an
output. The value from the input register which represents the real
value or the value from the output register which represents the desired
value. Because of the "Warning: value of pin is still %d" inside the
gpio cmd I would expect the real value. This is also implemented by most
drivers. Only tegra, kw and rcar distinguish between input and output.
Thereby the rcar explicitly mention a bug:
Testing on r8a7790 shows that INDT does not show correct pin state when
configured as output, so use OUTDT in case of output pins.
A test of the Xilinx GPIO controller shows that a read always returns
the value of the gpio_io_i signal.
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-26 19:46 ` Stefan Herbrechtsmeier
@ 2018-07-27 7:05 ` Michal Simek
2018-07-27 8:41 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-27 7:05 UTC (permalink / raw)
To: u-boot
On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>> because
>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>
>>>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>> [snip]
>>>>>
>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>> +
>>>>>>>> return val;
>>>>>>>> };
>>>>>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>>>>>> offset)
>>>>>>>> {
>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>> dev_get_platdata(dev);
>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>> int val, ret;
>>>>>>>> u32 bank, pin;
>>>>>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct
>>>>>>>> udevice
>>>>>>>> *dev, unsigned offset)
>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>> __func__,
>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>> + } else {
>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>> + val = priv->output_val[bank];
>>>>>>>> + }
>>>>>>> Why you don't always read the data register? This doesn't work for
>>>>>>> three
>>>>>>> state outputs.
>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>> It means else part is output and I read saved value in
>>>>>> priv->output_val.
>>>>>> If pin is setup as INPUT then I need read data reg to find out input
>>>>>> value.
>>>>>> Maybe you are commenting something else but please let me know if
>>>>>> there
>>>>>> is any other bug.
>>>>> What happen if I have an open drain output. Even if the gpio output
>>>>> is 1
>>>>> the input could read a 0. You driver will always return the output
>>>>> value
>>>>> and not the real input value. According to the picture in
>>>>> documentation
>>>>> and my tests a data register write writes the output registers and a
>>>>> data register read reads the input registers.
>>>>>
>>>>> Why should the driver return the desired state (output register)
>>>>> and not
>>>>> the real state (input register)?
>>>> First of all thanks for description.
>>>>
>>>> I have another example where you have output only and you can't read
>>>> input because there is no wire.
>>> Does you mean the all outputs configuration? Does this removes the
>>> "gpio_io_i" signal from the IP?
>> I am not sure what synthesis is doing with that unused IP pins but I
>> would consider as a bug if this is automatically connected together.
>
> I mean does the IP generator removes the gpio_io_i signal because it
> isn't needed? If the IP generator creates the gpio_io_i signal I would
> expect that you can't leave it unconnected as this would lead to
> undefined values.
Normally when you know that you have output only there is no no
gpio_io_i or tristate signal. The same for input only.
>> And
>> also wasting a logic if there is unused part.
>> But in Vivado you should be able to setup output pins to and input pins
>> separately. There are In/Out/Tristate.
>> If you don't want to deal with external pin you can connect them
>> inside PL.
>
> This isn't my point. I mean that if you have an gpio_io_i signal you
> have to connected it to a signal. You could connect it to the output of
> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
> connect it to a fixed value (0 or 1) you get this value if you read the
> status of this GPIO.
Not a HW to tell you what's vivado is going to do with that. But you can
select that ouput/input only where these signals are out.
>>>> Regarding open drain output.
>>>>
>>>> Also this is what it is written in manual:
>>>> "AXI GPIO Data Register Description"
>>>> "For each I/O bit programmed as output:
>>>> • R: Reads to these bits always return zeros"
>>> This must be a mistake in the documentation. I could read the input
>>> value.
>>>
>>> The old driver and the Linux driver always read the register.
>> In old u-boot driver I see
>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>> 180 val = gpio_get_output_value(gpio);
>> 181 else
>> 182 val = gpio_get_input_value(gpio);
>>
>> gpio_get_output_value() reads it from gpiodata_store for output
>> gpio_get_input_value() reads the reg.
>
> Sorry you are right. I mixed it with the zynq driver.
ok.
>> In Linux you are right it is read from reg in both cases.
>>
>>> Additionally the documentation states that a write to an input doesn't
>>> work:
>>> AXI GPIO Data Register.
>>> For each I/O bit programmed as input:
>>> • R: Reads value on the input pin.
>>> • W: No effect.
>>>
>>> However the Linux driver use the common sequence and sets first the data
>>> register and afterwards the direction register.
>> Possible that driver has pretty long history and I don't think it was
>> tested for all cases.
>
> I test the IP again and it works like expected and the documentation is
> wrong. You could always write the output register and read the input
> register. This means you could write the output first and afterwards
> enable the output via the tri-state register.
ok.
>>>> If you want to support this configuration then you would have to change
>>>> pin to input and read it and revert it back. And all of this should be
>>>> done based on flag.
>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>> also
>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>> That's why I have doubts if this is supported by any u-boot gpio driver
>>>> but I can be wrong.
>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
>>> case the open drain is transparent. The output has only two states
>>> High-Z and 0. The input value of the FPGA output block is always 0 and
>>> the tri-state input is connected to the output register of the GPIO
>>> controller. The output of the FPGA input block is connected to the input
>>> register of the GPIO controller.
>> I just found out in connection to second thread we have together that in
>> gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
>> not wired anywhere.
>> I think in your case when these two functions are implemented you will
>> get this functionality which you are asking about.
>
> No. This function is to control a special register.
I am not quite sure if this is really about special register or can be
also used in our case.
> Beside the open drain. What happens if the output is stuck to a fixed
> signal. This could be detected if you read the input register.
And again if that wires are there. Not covering the case where you have
output only without any input signal at all.
>> My guess is that gpio command should be extended to call these two
>> functions. When set_open_drain is called special flag for pin is setup
>> and for this pin get_status will always read data reg in our case and
>> you get your values.
>
> The main quest is which value should the get function return for an
> output. The value from the input register which represents the real
> value or the value from the output register which represents the desired
> value. Because of the "Warning: value of pin is still %d" inside the
> gpio cmd I would expect the real value. This is also implemented by most
> drivers. Only tegra, kw and rcar distinguish between input and output.
> Thereby the rcar explicitly mention a bug:
>
> Testing on r8a7790 shows that INDT does not show correct pin state when
> configured as output, so use OUTDT in case of output pins.
>
> A test of the Xilinx GPIO controller shows that a read always returns
> the value of the gpio_io_i signal.
What about to use this logic?
if (platdata->bank_input[bank]) // input only
val = readl(&platdata->regs->gpiodata + bank * 2);
else if (platdata->bank_output[bank])) // output only
val = priv->output_val[bank];
else { // tristate because it is not output or input only.
state = xilinx_gpio_get_function(dev, offset)
if (state == GPIOF_INPUT) // if input now - just read it
val = readl(&platdata->regs->gpiodata + bank * 2);
else { // if output - change it to input
tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
tmp = tmp1 | (1 << pin);
writel(tmp, &platdata->regs->gpiodir + bank * 2);
// read value
val = readl(&platdata->regs->gpiodata + bank * 2);
// revert it back
writel(tmp1, &platdata->regs->gpiodir + bank * 2);
}
}
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-27 7:05 ` Michal Simek
@ 2018-07-27 8:41 ` Stefan Herbrechtsmeier
2018-07-30 12:40 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-27 8:41 UTC (permalink / raw)
To: u-boot
Am 27.07.2018 um 09:05 schrieb Michal Simek:
> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>>> because
>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>
>>>>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>> ---
>>>>>>>>>
>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>> [snip]
>>>>>>
>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>> +
>>>>>>>>> return val;
>>>>>>>>> };
>>>>>>>>> @@ -441,6 +449,7 @@ static int xilinx_gpio_get_function(struct
>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev, unsigned
>>>>>>>>> offset)
>>>>>>>>> {
>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>> int val, ret;
>>>>>>>>> u32 bank, pin;
>>>>>>>>> @@ -451,7 +460,14 @@ static int xilinx_gpio_get_value(struct
>>>>>>>>> udevice
>>>>>>>>> *dev, unsigned offset)
>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>>> __func__,
>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>> + } else {
>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>> + }
>>>>>>>> Why you don't always read the data register? This doesn't work for
>>>>>>>> three
>>>>>>>> state outputs.
>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>> It means else part is output and I read saved value in
>>>>>>> priv->output_val.
>>>>>>> If pin is setup as INPUT then I need read data reg to find out input
>>>>>>> value.
>>>>>>> Maybe you are commenting something else but please let me know if
>>>>>>> there
>>>>>>> is any other bug.
>>>>>> What happen if I have an open drain output. Even if the gpio output
>>>>>> is 1
>>>>>> the input could read a 0. You driver will always return the output
>>>>>> value
>>>>>> and not the real input value. According to the picture in
>>>>>> documentation
>>>>>> and my tests a data register write writes the output registers and a
>>>>>> data register read reads the input registers.
>>>>>>
>>>>>> Why should the driver return the desired state (output register)
>>>>>> and not
>>>>>> the real state (input register)?
>>>>> First of all thanks for description.
>>>>>
>>>>> I have another example where you have output only and you can't read
>>>>> input because there is no wire.
>>>> Does you mean the all outputs configuration? Does this removes the
>>>> "gpio_io_i" signal from the IP?
>>> I am not sure what synthesis is doing with that unused IP pins but I
>>> would consider as a bug if this is automatically connected together.
>> I mean does the IP generator removes the gpio_io_i signal because it
>> isn't needed? If the IP generator creates the gpio_io_i signal I would
>> expect that you can't leave it unconnected as this would lead to
>> undefined values.
> Normally when you know that you have output only there is no no
> gpio_io_i or tristate signal. The same for input only.
And in this case the device tree flags "xlnx,all-inputs" or
"xlnx,all-outputs" should be set.
>>> And
>>> also wasting a logic if there is unused part.
>>> But in Vivado you should be able to setup output pins to and input pins
>>> separately. There are In/Out/Tristate.
>>> If you don't want to deal with external pin you can connect them
>>> inside PL.
>> This isn't my point. I mean that if you have an gpio_io_i signal you
>> have to connected it to a signal. You could connect it to the output of
>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>> connect it to a fixed value (0 or 1) you get this value if you read the
>> status of this GPIO.
> Not a HW to tell you what's vivado is going to do with that. But you can
> select that ouput/input only where these signals are out.
You mean the all-inputs or all-outputs case.
>>>>> Regarding open drain output.
>>>>>
>>>>> Also this is what it is written in manual:
>>>>> "AXI GPIO Data Register Description"
>>>>> "For each I/O bit programmed as output:
>>>>> • R: Reads to these bits always return zeros"
>>>> This must be a mistake in the documentation. I could read the input
>>>> value.
>>>>
>>>> The old driver and the Linux driver always read the register.
>>> In old u-boot driver I see
>>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>>> 180 val = gpio_get_output_value(gpio);
>>> 181 else
>>> 182 val = gpio_get_input_value(gpio);
>>>
>>> gpio_get_output_value() reads it from gpiodata_store for output
>>> gpio_get_input_value() reads the reg.
>> Sorry you are right. I mixed it with the zynq driver.
> ok.
>
>
>>> In Linux you are right it is read from reg in both cases.
>>>
>>>> Additionally the documentation states that a write to an input doesn't
>>>> work:
>>>> AXI GPIO Data Register.
>>>> For each I/O bit programmed as input:
>>>> • R: Reads value on the input pin.
>>>> • W: No effect.
>>>>
>>>> However the Linux driver use the common sequence and sets first the data
>>>> register and afterwards the direction register.
>>> Possible that driver has pretty long history and I don't think it was
>>> tested for all cases.
>> I test the IP again and it works like expected and the documentation is
>> wrong. You could always write the output register and read the input
>> register. This means you could write the output first and afterwards
>> enable the output via the tri-state register.
> ok.
Maybe the set_direction function should be adapted to avoid glitches
because it first enabled the output with the old value.
>>>>> If you want to support this configuration then you would have to change
>>>>> pin to input and read it and revert it back. And all of this should be
>>>>> done based on flag.
>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT binding.
>>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>>> also
>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>> That's why I have doubts if this is supported by any u-boot gpio driver
>>>>> but I can be wrong.
>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
>>>> case the open drain is transparent. The output has only two states
>>>> High-Z and 0. The input value of the FPGA output block is always 0 and
>>>> the tri-state input is connected to the output register of the GPIO
>>>> controller. The output of the FPGA input block is connected to the input
>>>> register of the GPIO controller.
>>> I just found out in connection to second thread we have together that in
>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
>>> not wired anywhere.
>>> I think in your case when these two functions are implemented you will
>>> get this functionality which you are asking about.
>> No. This function is to control a special register.
> I am not quite sure if this is really about special register or can be
> also used in our case.
In my case the open drain is transparent for the controller. The output
replace a 1 by a High-Z. This only means that you don't know the real
value on the wire.
>> Beside the open drain. What happens if the output is stuck to a fixed
>> signal. This could be detected if you read the input register.
> And again if that wires are there. Not covering the case where you have
> output only without any input signal at all.
This can be detect by the all-outputs flag.
>>> My guess is that gpio command should be extended to call these two
>>> functions. When set_open_drain is called special flag for pin is setup
>>> and for this pin get_status will always read data reg in our case and
>>> you get your values.
>> The main quest is which value should the get function return for an
>> output. The value from the input register which represents the real
>> value or the value from the output register which represents the desired
>> value. Because of the "Warning: value of pin is still %d" inside the
>> gpio cmd I would expect the real value. This is also implemented by most
>> drivers. Only tegra, kw and rcar distinguish between input and output.
>> Thereby the rcar explicitly mention a bug:
>>
>> Testing on r8a7790 shows that INDT does not show correct pin state when
>> configured as output, so use OUTDT in case of output pins.
>>
>> A test of the Xilinx GPIO controller shows that a read always returns
>> the value of the gpio_io_i signal.
> What about to use this logic?
>
> if (platdata->bank_input[bank]) // input only
> val = readl(&platdata->regs->gpiodata + bank * 2);
> else if (platdata->bank_output[bank])) // output only
> val = priv->output_val[bank];
Okay
> else { // tristate because it is not output or input only.
> state = xilinx_gpio_get_function(dev, offset)
> if (state == GPIOF_INPUT) // if input now - just read it
> val = readl(&platdata->regs->gpiodata + bank * 2);
> else { // if output - change it to input
> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
> tmp = tmp1 | (1 << pin);
> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>
> // read value
> val = readl(&platdata->regs->gpiodata + bank * 2);
>
> // revert it back
> writel(tmp1, &platdata->regs->gpiodir + bank * 2);
> }
> }
This will generate a glitch on the wire.
Why don't we simple read the value? The hardware support this and we
have already handle the all-output case:
if (platdata->bank_output[bank]))
val = priv->output_val[bank];
else
val = readl(&platdata->regs->gpiodata + bank * 2);
If you don't trust my test could you please ask your hardware co works
why the data register behavior should depend on the tri-state register.
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-27 8:41 ` Stefan Herbrechtsmeier
@ 2018-07-30 12:40 ` Michal Simek
2018-07-30 13:32 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-30 12:40 UTC (permalink / raw)
To: u-boot
On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>>>> because
>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>
>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>> [snip]
>>>>>>>
>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>> +
>>>>>>>>>> return val;
>>>>>>>>>> };
>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev,
>>>>>>>>>> unsigned
>>>>>>>>>> offset)
>>>>>>>>>> {
>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>> int val, ret;
>>>>>>>>>> u32 bank, pin;
>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>> udevice
>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>>>> __func__,
>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>> + } else {
>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>> + }
>>>>>>>>> Why you don't always read the data register? This doesn't work for
>>>>>>>>> three
>>>>>>>>> state outputs.
>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>> It means else part is output and I read saved value in
>>>>>>>> priv->output_val.
>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>> input
>>>>>>>> value.
>>>>>>>> Maybe you are commenting something else but please let me know if
>>>>>>>> there
>>>>>>>> is any other bug.
>>>>>>> What happen if I have an open drain output. Even if the gpio output
>>>>>>> is 1
>>>>>>> the input could read a 0. You driver will always return the output
>>>>>>> value
>>>>>>> and not the real input value. According to the picture in
>>>>>>> documentation
>>>>>>> and my tests a data register write writes the output registers and a
>>>>>>> data register read reads the input registers.
>>>>>>>
>>>>>>> Why should the driver return the desired state (output register)
>>>>>>> and not
>>>>>>> the real state (input register)?
>>>>>> First of all thanks for description.
>>>>>>
>>>>>> I have another example where you have output only and you can't read
>>>>>> input because there is no wire.
>>>>> Does you mean the all outputs configuration? Does this removes the
>>>>> "gpio_io_i" signal from the IP?
>>>> I am not sure what synthesis is doing with that unused IP pins but I
>>>> would consider as a bug if this is automatically connected together.
>>> I mean does the IP generator removes the gpio_io_i signal because it
>>> isn't needed? If the IP generator creates the gpio_io_i signal I would
>>> expect that you can't leave it unconnected as this would lead to
>>> undefined values.
>> Normally when you know that you have output only there is no no
>> gpio_io_i or tristate signal. The same for input only.
>
> And in this case the device tree flags "xlnx,all-inputs" or
> "xlnx,all-outputs" should be set.
yes.
>
>>>> And
>>>> also wasting a logic if there is unused part.
>>>> But in Vivado you should be able to setup output pins to and input pins
>>>> separately. There are In/Out/Tristate.
>>>> If you don't want to deal with external pin you can connect them
>>>> inside PL.
>>> This isn't my point. I mean that if you have an gpio_io_i signal you
>>> have to connected it to a signal. You could connect it to the output of
>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>> connect it to a fixed value (0 or 1) you get this value if you read the
>>> status of this GPIO.
>> Not a HW to tell you what's vivado is going to do with that. But you can
>> select that ouput/input only where these signals are out.
>
> You mean the all-inputs or all-outputs case.
yes.
>
>>>>>> Regarding open drain output.
>>>>>>
>>>>>> Also this is what it is written in manual:
>>>>>> "AXI GPIO Data Register Description"
>>>>>> "For each I/O bit programmed as output:
>>>>>> • R: Reads to these bits always return zeros"
>>>>> This must be a mistake in the documentation. I could read the input
>>>>> value.
>>>>>
>>>>> The old driver and the Linux driver always read the register.
>>>> In old u-boot driver I see
>>>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>>>> 180 val = gpio_get_output_value(gpio);
>>>> 181 else
>>>> 182 val = gpio_get_input_value(gpio);
>>>>
>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>> gpio_get_input_value() reads the reg.
>>> Sorry you are right. I mixed it with the zynq driver.
>> ok.
>>
>>
>>>> In Linux you are right it is read from reg in both cases.
>>>>
>>>>> Additionally the documentation states that a write to an input doesn't
>>>>> work:
>>>>> AXI GPIO Data Register.
>>>>> For each I/O bit programmed as input:
>>>>> • R: Reads value on the input pin.
>>>>> • W: No effect.
>>>>>
>>>>> However the Linux driver use the common sequence and sets first the
>>>>> data
>>>>> register and afterwards the direction register.
>>>> Possible that driver has pretty long history and I don't think it was
>>>> tested for all cases.
>>> I test the IP again and it works like expected and the documentation is
>>> wrong. You could always write the output register and read the input
>>> register. This means you could write the output first and afterwards
>>> enable the output via the tri-state register.
>> ok.
>
> Maybe the set_direction function should be adapted to avoid glitches
> because it first enabled the output with the old value.
No problem with this change at all.
>
>>>>>> If you want to support this configuration then you would have to
>>>>>> change
>>>>>> pin to input and read it and revert it back. And all of this
>>>>>> should be
>>>>>> done based on flag.
>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>> binding.
>>>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>>>> also
>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>> driver
>>>>>> but I can be wrong.
>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
>>>>> case the open drain is transparent. The output has only two states
>>>>> High-Z and 0. The input value of the FPGA output block is always 0 and
>>>>> the tri-state input is connected to the output register of the GPIO
>>>>> controller. The output of the FPGA input block is connected to the
>>>>> input
>>>>> register of the GPIO controller.
>>>> I just found out in connection to second thread we have together
>>>> that in
>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
>>>> not wired anywhere.
>>>> I think in your case when these two functions are implemented you will
>>>> get this functionality which you are asking about.
>>> No. This function is to control a special register.
>> I am not quite sure if this is really about special register or can be
>> also used in our case.
>
> In my case the open drain is transparent for the controller. The output
> replace a 1 by a High-Z. This only means that you don't know the real
> value on the wire.
please look below.
>
>>> Beside the open drain. What happens if the output is stuck to a fixed
>>> signal. This could be detected if you read the input register.
>> And again if that wires are there. Not covering the case where you have
>> output only without any input signal at all.
>
> This can be detect by the all-outputs flag.
please look below.
>
>>>> My guess is that gpio command should be extended to call these two
>>>> functions. When set_open_drain is called special flag for pin is setup
>>>> and for this pin get_status will always read data reg in our case and
>>>> you get your values.
>>> The main quest is which value should the get function return for an
>>> output. The value from the input register which represents the real
>>> value or the value from the output register which represents the desired
>>> value. Because of the "Warning: value of pin is still %d" inside the
>>> gpio cmd I would expect the real value. This is also implemented by most
>>> drivers. Only tegra, kw and rcar distinguish between input and output.
>>> Thereby the rcar explicitly mention a bug:
>>>
>>> Testing on r8a7790 shows that INDT does not show correct pin state when
>>> configured as output, so use OUTDT in case of output pins.
>>>
>>> A test of the Xilinx GPIO controller shows that a read always returns
>>> the value of the gpio_io_i signal.
>> What about to use this logic?
>>
>> if (platdata->bank_input[bank]) // input only
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>> else if (platdata->bank_output[bank])) // output only
>> val = priv->output_val[bank];
>
> Okay
>
>> else { // tristate because it is not output or input only.
>> state = xilinx_gpio_get_function(dev, offset)
>> if (state == GPIOF_INPUT) // if input now - just read it
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>> else { // if output - change it to input
>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>> tmp = tmp1 | (1 << pin);
>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>
>> // read value
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>
>> // revert it back
>> writel(tmp1, &platdata->regs->gpiodir + bank * 2);
>> }
>> }
>
> This will generate a glitch on the wire.
>
> Why don't we simple read the value? The hardware support this and we
> have already handle the all-output case:
>
> if (platdata->bank_output[bank]))
> val = priv->output_val[bank];
> else
> val = readl(&platdata->regs->gpiodata + bank * 2);
>
> If you don't trust my test could you please ask your hardware co works
> why the data register behavior should depend on the tri-state register.
I am looking for IP owner but as far as I see it was created 9 years ago
that's why it will take some time.
Anyway I have created hw design on zcu102 which has output on led and
input on dip and output value is kept in data reg that's why your
snippet above can be used. Documentation is wrong in this
"For each I/O bit programmed as output:
• R: Reads to these bits always return zeros"
I have sent 4 patches which we discussed that's why please reviewed them.
I have one more patch in queue which is keeping output_val for certain
bank but not sure if make sense to apply it because as above when
direction is setup to output you can read desired output value from
gpiodata reg. I have tested it on gpio output only led case and gpiodata
reg contain correct value.
Anyway if I miss any patch in connection to this please send a patch and
let's look at that use case.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-30 12:40 ` Michal Simek
@ 2018-07-30 13:32 ` Stefan Herbrechtsmeier
2018-07-30 14:10 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-30 13:32 UTC (permalink / raw)
To: u-boot
Am 30.07.2018 um 14:40 schrieb Michal Simek:
> On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
>> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>>>>> because
>>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>>
>>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier <stefan@herbrechtsmeier.net>
>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>> ---
>>>>>>>>>>>
>>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>>> [snip]
>>>>>>>>
>>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>>> +
>>>>>>>>>>> return val;
>>>>>>>>>>> };
>>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev,
>>>>>>>>>>> unsigned
>>>>>>>>>>> offset)
>>>>>>>>>>> {
>>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>> int val, ret;
>>>>>>>>>>> u32 bank, pin;
>>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>>> udevice
>>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>>>>> __func__,
>>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) == GPIOF_INPUT) {
>>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>> + } else {
>>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>>> + }
>>>>>>>>>> Why you don't always read the data register? This doesn't work for
>>>>>>>>>> three
>>>>>>>>>> state outputs.
>>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>>> It means else part is output and I read saved value in
>>>>>>>>> priv->output_val.
>>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>>> input
>>>>>>>>> value.
>>>>>>>>> Maybe you are commenting something else but please let me know if
>>>>>>>>> there
>>>>>>>>> is any other bug.
>>>>>>>> What happen if I have an open drain output. Even if the gpio output
>>>>>>>> is 1
>>>>>>>> the input could read a 0. You driver will always return the output
>>>>>>>> value
>>>>>>>> and not the real input value. According to the picture in
>>>>>>>> documentation
>>>>>>>> and my tests a data register write writes the output registers and a
>>>>>>>> data register read reads the input registers.
>>>>>>>>
>>>>>>>> Why should the driver return the desired state (output register)
>>>>>>>> and not
>>>>>>>> the real state (input register)?
>>>>>>> First of all thanks for description.
>>>>>>>
>>>>>>> I have another example where you have output only and you can't read
>>>>>>> input because there is no wire.
>>>>>> Does you mean the all outputs configuration? Does this removes the
>>>>>> "gpio_io_i" signal from the IP?
>>>>> I am not sure what synthesis is doing with that unused IP pins but I
>>>>> would consider as a bug if this is automatically connected together.
>>>> I mean does the IP generator removes the gpio_io_i signal because it
>>>> isn't needed? If the IP generator creates the gpio_io_i signal I would
>>>> expect that you can't leave it unconnected as this would lead to
>>>> undefined values.
>>> Normally when you know that you have output only there is no no
>>> gpio_io_i or tristate signal. The same for input only.
>> And in this case the device tree flags "xlnx,all-inputs" or
>> "xlnx,all-outputs" should be set.
> yes.
>
>>>>> And
>>>>> also wasting a logic if there is unused part.
>>>>> But in Vivado you should be able to setup output pins to and input pins
>>>>> separately. There are In/Out/Tristate.
>>>>> If you don't want to deal with external pin you can connect them
>>>>> inside PL.
>>>> This isn't my point. I mean that if you have an gpio_io_i signal you
>>>> have to connected it to a signal. You could connect it to the output of
>>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>>> connect it to a fixed value (0 or 1) you get this value if you read the
>>>> status of this GPIO.
>>> Not a HW to tell you what's vivado is going to do with that. But you can
>>> select that ouput/input only where these signals are out.
>> You mean the all-inputs or all-outputs case.
> yes.
>
>>>>>>> Regarding open drain output.
>>>>>>>
>>>>>>> Also this is what it is written in manual:
>>>>>>> "AXI GPIO Data Register Description"
>>>>>>> "For each I/O bit programmed as output:
>>>>>>> • R: Reads to these bits always return zeros"
>>>>>> This must be a mistake in the documentation. I could read the input
>>>>>> value.
>>>>>>
>>>>>> The old driver and the Linux driver always read the register.
>>>>> In old u-boot driver I see
>>>>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>>>>> 180 val = gpio_get_output_value(gpio);
>>>>> 181 else
>>>>> 182 val = gpio_get_input_value(gpio);
>>>>>
>>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>>> gpio_get_input_value() reads the reg.
>>>> Sorry you are right. I mixed it with the zynq driver.
>>> ok.
>>>
>>>
>>>>> In Linux you are right it is read from reg in both cases.
>>>>>
>>>>>> Additionally the documentation states that a write to an input doesn't
>>>>>> work:
>>>>>> AXI GPIO Data Register.
>>>>>> For each I/O bit programmed as input:
>>>>>> • R: Reads value on the input pin.
>>>>>> • W: No effect.
>>>>>>
>>>>>> However the Linux driver use the common sequence and sets first the
>>>>>> data
>>>>>> register and afterwards the direction register.
>>>>> Possible that driver has pretty long history and I don't think it was
>>>>> tested for all cases.
>>>> I test the IP again and it works like expected and the documentation is
>>>> wrong. You could always write the output register and read the input
>>>> register. This means you could write the output first and afterwards
>>>> enable the output via the tri-state register.
>>> ok.
>> Maybe the set_direction function should be adapted to avoid glitches
>> because it first enabled the output with the old value.
> No problem with this change at all.
>
>>>>>>> If you want to support this configuration then you would have to
>>>>>>> change
>>>>>>> pin to input and read it and revert it back. And all of this
>>>>>>> should be
>>>>>>> done based on flag.
>>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>>> binding.
>>>>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>>>>> also
>>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>>> driver
>>>>>>> but I can be wrong.
>>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1. In our
>>>>>> case the open drain is transparent. The output has only two states
>>>>>> High-Z and 0. The input value of the FPGA output block is always 0 and
>>>>>> the tri-state input is connected to the output register of the GPIO
>>>>>> controller. The output of the FPGA input block is connected to the
>>>>>> input
>>>>>> register of the GPIO controller.
>>>>> I just found out in connection to second thread we have together
>>>>> that in
>>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but they are
>>>>> not wired anywhere.
>>>>> I think in your case when these two functions are implemented you will
>>>>> get this functionality which you are asking about.
>>>> No. This function is to control a special register.
>>> I am not quite sure if this is really about special register or can be
>>> also used in our case.
>> In my case the open drain is transparent for the controller. The output
>> replace a 1 by a High-Z. This only means that you don't know the real
>> value on the wire.
> please look below.
>
>>>> Beside the open drain. What happens if the output is stuck to a fixed
>>>> signal. This could be detected if you read the input register.
>>> And again if that wires are there. Not covering the case where you have
>>> output only without any input signal at all.
>> This can be detect by the all-outputs flag.
> please look below.
>
>>>>> My guess is that gpio command should be extended to call these two
>>>>> functions. When set_open_drain is called special flag for pin is setup
>>>>> and for this pin get_status will always read data reg in our case and
>>>>> you get your values.
>>>> The main quest is which value should the get function return for an
>>>> output. The value from the input register which represents the real
>>>> value or the value from the output register which represents the desired
>>>> value. Because of the "Warning: value of pin is still %d" inside the
>>>> gpio cmd I would expect the real value. This is also implemented by most
>>>> drivers. Only tegra, kw and rcar distinguish between input and output.
>>>> Thereby the rcar explicitly mention a bug:
>>>>
>>>> Testing on r8a7790 shows that INDT does not show correct pin state when
>>>> configured as output, so use OUTDT in case of output pins.
>>>>
>>>> A test of the Xilinx GPIO controller shows that a read always returns
>>>> the value of the gpio_io_i signal.
>>> What about to use this logic?
>>>
>>> if (platdata->bank_input[bank]) // input only
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>> else if (platdata->bank_output[bank])) // output only
>>> val = priv->output_val[bank];
>> Okay
>>
>>> else { // tristate because it is not output or input only.
>>> state = xilinx_gpio_get_function(dev, offset)
>>> if (state == GPIOF_INPUT) // if input now - just read it
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>> else { // if output - change it to input
>>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>>> tmp = tmp1 | (1 << pin);
>>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>>
>>> // read value
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>
>>> // revert it back
>>> writel(tmp1, &platdata->regs->gpiodir + bank * 2);
>>> }
>>> }
>> This will generate a glitch on the wire.
>>
>> Why don't we simple read the value? The hardware support this and we
>> have already handle the all-output case:
>>
>> if (platdata->bank_output[bank]))
>> val = priv->output_val[bank];
>> else
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>
>> If you don't trust my test could you please ask your hardware co works
>> why the data register behavior should depend on the tri-state register.
> I am looking for IP owner but as far as I see it was created 9 years ago
> that's why it will take some time.
> Anyway I have created hw design on zcu102 which has output on led and
> input on dip and output value is kept in data reg that's why your
> snippet above can be used.
Do you use a signal port for both or two separated ports?
Have you connect the gpio_io_i and gpio_io_o signals of the outputs?
What happens if you connect the gpio_io_i signals to zero for the output
to save resources?
What happens if you disable the power supply for the LED bank?
> Documentation is wrong in this
> "For each I/O bit programmed as output:
> • R: Reads to these bits always return zeros"
>
> I have sent 4 patches which we discussed that's why please reviewed them.
> I have one more patch in queue which is keeping output_val for certain
> bank but not sure if make sense to apply it because as above when
> direction is setup to output you can read desired output value from
> gpiodata reg.
This is only true if the gpio_io_i and gpio_io_o signals are connected
together or the input and output signals have always the same value.
Otherwise you read the input signal and write it into the output value.
In our use case we have two gpio controllers (for two processors) which
are connected to one io port. Thereby only the output is multiplexed
between the two controllers and the input is always connected to the io.
This means both controller always read the input value of the io but
only one of them controls the output of the io. Your set function reads
the input value of the io (maybe controlled by the other controller) and
thereby overwrite the old output value of the controller.
> I have tested it on gpio output only led case and gpiodata
> reg contain correct value.
Have you test outputs with differences between output value and input value?
> Anyway if I miss any patch in connection to this please send a patch and
> let's look at that use case.
Okay
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-30 13:32 ` Stefan Herbrechtsmeier
@ 2018-07-30 14:10 ` Michal Simek
2018-07-30 19:34 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Michal Simek @ 2018-07-30 14:10 UTC (permalink / raw)
To: u-boot
On 30.7.2018 15:32, Stefan Herbrechtsmeier wrote:
> Am 30.07.2018 um 14:40 schrieb Michal Simek:
>> On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
>>> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>>>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>>>>>> because
>>>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>>>
>>>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier
>>>>>>>>>>>> <stefan@herbrechtsmeier.net>
>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>>>> [snip]
>>>>>>>>>
>>>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>>>> +
>>>>>>>>>>>> return val;
>>>>>>>>>>>> };
>>>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev,
>>>>>>>>>>>> unsigned
>>>>>>>>>>>> offset)
>>>>>>>>>>>> {
>>>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>>> int val, ret;
>>>>>>>>>>>> u32 bank, pin;
>>>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>>>> udevice
>>>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>>>>>> __func__,
>>>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) ==
>>>>>>>>>>>> GPIOF_INPUT) {
>>>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>> + } else {
>>>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>>>> + }
>>>>>>>>>>> Why you don't always read the data register? This doesn't
>>>>>>>>>>> work for
>>>>>>>>>>> three
>>>>>>>>>>> state outputs.
>>>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>>>> It means else part is output and I read saved value in
>>>>>>>>>> priv->output_val.
>>>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>>>> input
>>>>>>>>>> value.
>>>>>>>>>> Maybe you are commenting something else but please let me know if
>>>>>>>>>> there
>>>>>>>>>> is any other bug.
>>>>>>>>> What happen if I have an open drain output. Even if the gpio
>>>>>>>>> output
>>>>>>>>> is 1
>>>>>>>>> the input could read a 0. You driver will always return the output
>>>>>>>>> value
>>>>>>>>> and not the real input value. According to the picture in
>>>>>>>>> documentation
>>>>>>>>> and my tests a data register write writes the output registers
>>>>>>>>> and a
>>>>>>>>> data register read reads the input registers.
>>>>>>>>>
>>>>>>>>> Why should the driver return the desired state (output register)
>>>>>>>>> and not
>>>>>>>>> the real state (input register)?
>>>>>>>> First of all thanks for description.
>>>>>>>>
>>>>>>>> I have another example where you have output only and you can't
>>>>>>>> read
>>>>>>>> input because there is no wire.
>>>>>>> Does you mean the all outputs configuration? Does this removes the
>>>>>>> "gpio_io_i" signal from the IP?
>>>>>> I am not sure what synthesis is doing with that unused IP pins but I
>>>>>> would consider as a bug if this is automatically connected together.
>>>>> I mean does the IP generator removes the gpio_io_i signal because it
>>>>> isn't needed? If the IP generator creates the gpio_io_i signal I would
>>>>> expect that you can't leave it unconnected as this would lead to
>>>>> undefined values.
>>>> Normally when you know that you have output only there is no no
>>>> gpio_io_i or tristate signal. The same for input only.
>>> And in this case the device tree flags "xlnx,all-inputs" or
>>> "xlnx,all-outputs" should be set.
>> yes.
>>
>>>>>> And
>>>>>> also wasting a logic if there is unused part.
>>>>>> But in Vivado you should be able to setup output pins to and input
>>>>>> pins
>>>>>> separately. There are In/Out/Tristate.
>>>>>> If you don't want to deal with external pin you can connect them
>>>>>> inside PL.
>>>>> This isn't my point. I mean that if you have an gpio_io_i signal you
>>>>> have to connected it to a signal. You could connect it to the
>>>>> output of
>>>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>>>> connect it to a fixed value (0 or 1) you get this value if you read
>>>>> the
>>>>> status of this GPIO.
>>>> Not a HW to tell you what's vivado is going to do with that. But you
>>>> can
>>>> select that ouput/input only where these signals are out.
>>> You mean the all-inputs or all-outputs case.
>> yes.
>>
>>>>>>>> Regarding open drain output.
>>>>>>>>
>>>>>>>> Also this is what it is written in manual:
>>>>>>>> "AXI GPIO Data Register Description"
>>>>>>>> "For each I/O bit programmed as output:
>>>>>>>> • R: Reads to these bits always return zeros"
>>>>>>> This must be a mistake in the documentation. I could read the input
>>>>>>> value.
>>>>>>>
>>>>>>> The old driver and the Linux driver always read the register.
>>>>>> In old u-boot driver I see
>>>>>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>>>>>> 180 val = gpio_get_output_value(gpio);
>>>>>> 181 else
>>>>>> 182 val = gpio_get_input_value(gpio);
>>>>>>
>>>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>>>> gpio_get_input_value() reads the reg.
>>>>> Sorry you are right. I mixed it with the zynq driver.
>>>> ok.
>>>>
>>>>
>>>>>> In Linux you are right it is read from reg in both cases.
>>>>>>
>>>>>>> Additionally the documentation states that a write to an input
>>>>>>> doesn't
>>>>>>> work:
>>>>>>> AXI GPIO Data Register.
>>>>>>> For each I/O bit programmed as input:
>>>>>>> • R: Reads value on the input pin.
>>>>>>> • W: No effect.
>>>>>>>
>>>>>>> However the Linux driver use the common sequence and sets first the
>>>>>>> data
>>>>>>> register and afterwards the direction register.
>>>>>> Possible that driver has pretty long history and I don't think it was
>>>>>> tested for all cases.
>>>>> I test the IP again and it works like expected and the
>>>>> documentation is
>>>>> wrong. You could always write the output register and read the input
>>>>> register. This means you could write the output first and afterwards
>>>>> enable the output via the tri-state register.
>>>> ok.
>>> Maybe the set_direction function should be adapted to avoid glitches
>>> because it first enabled the output with the old value.
>> No problem with this change at all.
>>
>>>>>>>> If you want to support this configuration then you would have to
>>>>>>>> change
>>>>>>>> pin to input and read it and revert it back. And all of this
>>>>>>>> should be
>>>>>>>> done based on flag.
>>>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>>>> binding.
>>>>>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>>>>>> also
>>>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>>>> driver
>>>>>>>> but I can be wrong.
>>>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1.
>>>>>>> In our
>>>>>>> case the open drain is transparent. The output has only two states
>>>>>>> High-Z and 0. The input value of the FPGA output block is always
>>>>>>> 0 and
>>>>>>> the tri-state input is connected to the output register of the GPIO
>>>>>>> controller. The output of the FPGA input block is connected to the
>>>>>>> input
>>>>>>> register of the GPIO controller.
>>>>>> I just found out in connection to second thread we have together
>>>>>> that in
>>>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but
>>>>>> they are
>>>>>> not wired anywhere.
>>>>>> I think in your case when these two functions are implemented you
>>>>>> will
>>>>>> get this functionality which you are asking about.
>>>>> No. This function is to control a special register.
>>>> I am not quite sure if this is really about special register or can be
>>>> also used in our case.
>>> In my case the open drain is transparent for the controller. The output
>>> replace a 1 by a High-Z. This only means that you don't know the real
>>> value on the wire.
>> please look below.
>>
>>>>> Beside the open drain. What happens if the output is stuck to a fixed
>>>>> signal. This could be detected if you read the input register.
>>>> And again if that wires are there. Not covering the case where you have
>>>> output only without any input signal at all.
>>> This can be detect by the all-outputs flag.
>> please look below.
>>
>>>>>> My guess is that gpio command should be extended to call these two
>>>>>> functions. When set_open_drain is called special flag for pin is
>>>>>> setup
>>>>>> and for this pin get_status will always read data reg in our case and
>>>>>> you get your values.
>>>>> The main quest is which value should the get function return for an
>>>>> output. The value from the input register which represents the real
>>>>> value or the value from the output register which represents the
>>>>> desired
>>>>> value. Because of the "Warning: value of pin is still %d" inside the
>>>>> gpio cmd I would expect the real value. This is also implemented by
>>>>> most
>>>>> drivers. Only tegra, kw and rcar distinguish between input and output.
>>>>> Thereby the rcar explicitly mention a bug:
>>>>>
>>>>> Testing on r8a7790 shows that INDT does not show correct pin state
>>>>> when
>>>>> configured as output, so use OUTDT in case of output pins.
>>>>>
>>>>> A test of the Xilinx GPIO controller shows that a read always returns
>>>>> the value of the gpio_io_i signal.
>>>> What about to use this logic?
>>>>
>>>> if (platdata->bank_input[bank]) // input only
>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>> else if (platdata->bank_output[bank])) // output only
>>>> val = priv->output_val[bank];
>>> Okay
>>>
>>>> else { // tristate because it is not output or input only.
>>>> state = xilinx_gpio_get_function(dev, offset)
>>>> if (state == GPIOF_INPUT) // if input now - just read it
>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>> else { // if output - change it to input
>>>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>>>> tmp = tmp1 | (1 << pin);
>>>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>>>
>>>> // read value
>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>
>>>> // revert it back
>>>> writel(tmp1, &platdata->regs->gpiodir + bank * 2);
>>>> }
>>>> }
>>> This will generate a glitch on the wire.
>>>
>>> Why don't we simple read the value? The hardware support this and we
>>> have already handle the all-output case:
>>>
>>> if (platdata->bank_output[bank]))
>>> val = priv->output_val[bank];
>>> else
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>
>>> If you don't trust my test could you please ask your hardware co works
>>> why the data register behavior should depend on the tri-state register.
>> I am looking for IP owner but as far as I see it was created 9 years ago
>> that's why it will take some time.
>> Anyway I have created hw design on zcu102 which has output on led and
>> input on dip and output value is kept in data reg that's why your
>> snippet above can be used.
>
> Do you use a signal port for both or two separated ports?
I expect single - yes the same pin 0 for output led and input dip.
>
> Have you connect the gpio_io_i and gpio_io_o signals of the outputs?
It is really pin 0 io_o to one fpga pin and pin0 io_i to another fpga pin.
> What happens if you connect the gpio_io_i signals to zero for the output
> to save resources?
> What happens if you disable the power supply for the LED bank?
didn't try that.
>
>> Documentation is wrong in this
>> "For each I/O bit programmed as output:
>> • R: Reads to these bits always return zeros"
>>
>> I have sent 4 patches which we discussed that's why please reviewed them.
>> I have one more patch in queue which is keeping output_val for certain
>> bank but not sure if make sense to apply it because as above when
>> direction is setup to output you can read desired output value from
>> gpiodata reg.
>
> This is only true if the gpio_io_i and gpio_io_o signals are connected
> together or the input and output signals have always the same value.
> Otherwise you read the input signal and write it into the output value.
That's what I see on zcu102 without any connection between io_i io_o.
There is incorrect behavior when gpio toggle command is called and input
value is 1. Because the first gpio toggle cmd reads input value first
(because code is doing that) and then setting up output with opposite value.
> In our use case we have two gpio controllers (for two processors) which
> are connected to one io port. Thereby only the output is multiplexed
> between the two controllers and the input is always connected to the io.
> This means both controller always read the input value of the io but
> only one of them controls the output of the io. Your set function reads
> the input value of the io (maybe controlled by the other controller) and
> thereby overwrite the old output value of the controller.
>
>> I have tested it on gpio output only led case and gpiodata
>> reg contain correct value.
>
> Have you test outputs with differences between output value and input
> value?
I believe so.
>
>> Anyway if I miss any patch in connection to this please send a patch and
>> let's look at that use case.
>
> Okay
TBH it is starting to be a little bit confusing.
What we should really do is to use standard boards and connection and
describe that cases and expected behavior and changes in code which
should happen to support this.
We are talking about two functions.
xilinx_gpio_set_value and xilinx_gpio_get_value.
And let's take mainline driver with that 4 patches I have sent.
Let's start with xilinx_gpio_get_value()
Mainline is all the time reading value from gpiodata reg no matter if
this is output/input only or io.
Above is this sequence which you have written that it is right
if (platdata->bank_output[bank]))
val = priv->output_val[bank];
else
val = readl(&platdata->regs->gpiodata + bank * 2);
I have took at look at gpio output only case
ZynqMP> gpio toggle gpio at a00010000
gpio: pin gpio at a00010000 (gpio 187) value is 1
ZynqMP> gpio status -a gpio at a0001000
Bank gpio at a0001000:
gpio at a00010000: output: 1 [ ]
gpio at a00010001: output: 0 [ ]
gpio at a00010002: output: 0 [ ]
gpio at a00010003: output: 0 [ ]
gpio at a00010004: output: 0 [ ]
gpio at a00010005: output: 0 [ ]
gpio at a00010006: output: 0 [ ]
gpio at a00010007: output: 0 [ ]
ZynqMP> gpio toggle gpio at a00010004
gpio: pin gpio at a00010004 (gpio 191) value is 1
ZynqMP> gpio status -a gpio at a0001000
Bank gpio at a0001000:
gpio at a00010000: output: 1 [ ]
gpio at a00010001: output: 0 [ ]
gpio at a00010002: output: 0 [ ]
gpio at a00010003: output: 0 [ ]
gpio at a00010004: output: 1 [ ]
gpio at a00010005: output: 0 [ ]
gpio at a00010006: output: 0 [ ]
gpio at a00010007: output: 0 [ ]
ZynqMP> gpio toggle gpio at a00010006
gpio: pin gpio at a00010006 (gpio 193) value is 1
ZynqMP> gpio status -a gpio at a0001000
Bank gpio at a0001000:
gpio at a00010000: output: 1 [ ]
gpio at a00010001: output: 0 [ ]
gpio at a00010002: output: 0 [ ]
gpio at a00010003: output: 0 [ ]
gpio at a00010004: output: 1 [ ]
gpio at a00010005: output: 0 [ ]
gpio at a00010006: output: 1 [ ]
gpio at a00010007: output: 0 [ ]
ZynqMP> gpio toggle gpio at a00010004
gpio: pin gpio at a00010004 (gpio 191) value is 0
ZynqMP> gpio status -a gpio at a0001000
Bank gpio at a0001000:
gpio at a00010000: output: 1 [ ]
gpio at a00010001: output: 0 [ ]
gpio at a00010002: output: 0 [ ]
gpio at a00010003: output: 0 [ ]
gpio at a00010004: output: 0 [ ]
gpio at a00010005: output: 0 [ ]
gpio at a00010006: output: 1 [ ]
gpio at a00010007: output: 0 [ ]
ZynqMP> gpio toggle gpio at a00010007
gpio: pin gpio at a00010007 (gpio 194) value is 1
ZynqMP> gpio status -a gpio at a0001000
Bank gpio at a0001000:
gpio at a00010000: output: 1 [ ]
gpio at a00010001: output: 0 [ ]
gpio at a00010002: output: 0 [ ]
gpio at a00010003: output: 0 [ ]
gpio at a00010004: output: 0 [ ]
gpio at a00010005: output: 0 [ ]
gpio at a00010006: output: 1 [ ]
gpio at a00010007: output: 1 [ ]
And for this case there is no need to take that value from any saved
location.
It means having
val = readl(&platdata->regs->gpiodata + bank * 2);
is enough based on my test.
And for the rest as your example above this should be also fine.
When we have agreement on this we can look at xilinx_gpio_set_value.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-30 14:10 ` Michal Simek
@ 2018-07-30 19:34 ` Stefan Herbrechtsmeier
2018-08-01 18:36 ` Stefan Herbrechtsmeier
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-07-30 19:34 UTC (permalink / raw)
To: u-boot
Am 30.07.2018 um 16:10 schrieb Michal Simek:
> On 30.7.2018 15:32, Stefan Herbrechtsmeier wrote:
>> Am 30.07.2018 um 14:40 schrieb Michal Simek:
>>> On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
>>>> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>>>>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>>>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>>>>> Reading registers for finding out output value is not working
>>>>>>>>>>>>> because
>>>>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>>>>
>>>>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier
>>>>>>>>>>>>> <stefan@herbrechtsmeier.net>
>>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>>>> ---
>>>>>>>>>>>>>
>>>>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>> [snip]
>>>>>>>>>>
>>>>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>>>>> +
>>>>>>>>>>>>> return val;
>>>>>>>>>>>>> };
>>>>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice *dev,
>>>>>>>>>>>>> unsigned
>>>>>>>>>>>>> offset)
>>>>>>>>>>>>> {
>>>>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>>>> int val, ret;
>>>>>>>>>>>>> u32 bank, pin;
>>>>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>>>>> udevice
>>>>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin %x\n",
>>>>>>>>>>>>> __func__,
>>>>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) ==
>>>>>>>>>>>>> GPIOF_INPUT) {
>>>>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>>>>> + }
>>>>>>>>>>>> Why you don't always read the data register? This doesn't
>>>>>>>>>>>> work for
>>>>>>>>>>>> three
>>>>>>>>>>>> state outputs.
>>>>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>>>>> It means else part is output and I read saved value in
>>>>>>>>>>> priv->output_val.
>>>>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>>>>> input
>>>>>>>>>>> value.
>>>>>>>>>>> Maybe you are commenting something else but please let me know if
>>>>>>>>>>> there
>>>>>>>>>>> is any other bug.
>>>>>>>>>> What happen if I have an open drain output. Even if the gpio
>>>>>>>>>> output
>>>>>>>>>> is 1
>>>>>>>>>> the input could read a 0. You driver will always return the output
>>>>>>>>>> value
>>>>>>>>>> and not the real input value. According to the picture in
>>>>>>>>>> documentation
>>>>>>>>>> and my tests a data register write writes the output registers
>>>>>>>>>> and a
>>>>>>>>>> data register read reads the input registers.
>>>>>>>>>>
>>>>>>>>>> Why should the driver return the desired state (output register)
>>>>>>>>>> and not
>>>>>>>>>> the real state (input register)?
>>>>>>>>> First of all thanks for description.
>>>>>>>>>
>>>>>>>>> I have another example where you have output only and you can't
>>>>>>>>> read
>>>>>>>>> input because there is no wire.
>>>>>>>> Does you mean the all outputs configuration? Does this removes the
>>>>>>>> "gpio_io_i" signal from the IP?
>>>>>>> I am not sure what synthesis is doing with that unused IP pins but I
>>>>>>> would consider as a bug if this is automatically connected together.
>>>>>> I mean does the IP generator removes the gpio_io_i signal because it
>>>>>> isn't needed? If the IP generator creates the gpio_io_i signal I would
>>>>>> expect that you can't leave it unconnected as this would lead to
>>>>>> undefined values.
>>>>> Normally when you know that you have output only there is no no
>>>>> gpio_io_i or tristate signal. The same for input only.
>>>> And in this case the device tree flags "xlnx,all-inputs" or
>>>> "xlnx,all-outputs" should be set.
>>> yes.
>>>
>>>>>>> And
>>>>>>> also wasting a logic if there is unused part.
>>>>>>> But in Vivado you should be able to setup output pins to and input
>>>>>>> pins
>>>>>>> separately. There are In/Out/Tristate.
>>>>>>> If you don't want to deal with external pin you can connect them
>>>>>>> inside PL.
>>>>>> This isn't my point. I mean that if you have an gpio_io_i signal you
>>>>>> have to connected it to a signal. You could connect it to the
>>>>>> output of
>>>>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>>>>> connect it to a fixed value (0 or 1) you get this value if you read
>>>>>> the
>>>>>> status of this GPIO.
>>>>> Not a HW to tell you what's vivado is going to do with that. But you
>>>>> can
>>>>> select that ouput/input only where these signals are out.
>>>> You mean the all-inputs or all-outputs case.
>>> yes.
>>>
>>>>>>>>> Regarding open drain output.
>>>>>>>>>
>>>>>>>>> Also this is what it is written in manual:
>>>>>>>>> "AXI GPIO Data Register Description"
>>>>>>>>> "For each I/O bit programmed as output:
>>>>>>>>> • R: Reads to these bits always return zeros"
>>>>>>>> This must be a mistake in the documentation. I could read the input
>>>>>>>> value.
>>>>>>>>
>>>>>>>> The old driver and the Linux driver always read the register.
>>>>>>> In old u-boot driver I see
>>>>>>> 179 if (gpio_get_direction(gpio) == GPIO_DIRECTION_OUT)
>>>>>>> 180 val = gpio_get_output_value(gpio);
>>>>>>> 181 else
>>>>>>> 182 val = gpio_get_input_value(gpio);
>>>>>>>
>>>>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>>>>> gpio_get_input_value() reads the reg.
>>>>>> Sorry you are right. I mixed it with the zynq driver.
>>>>> ok.
>>>>>
>>>>>
>>>>>>> In Linux you are right it is read from reg in both cases.
>>>>>>>
>>>>>>>> Additionally the documentation states that a write to an input
>>>>>>>> doesn't
>>>>>>>> work:
>>>>>>>> AXI GPIO Data Register.
>>>>>>>> For each I/O bit programmed as input:
>>>>>>>> • R: Reads value on the input pin.
>>>>>>>> • W: No effect.
>>>>>>>>
>>>>>>>> However the Linux driver use the common sequence and sets first the
>>>>>>>> data
>>>>>>>> register and afterwards the direction register.
>>>>>>> Possible that driver has pretty long history and I don't think it was
>>>>>>> tested for all cases.
>>>>>> I test the IP again and it works like expected and the
>>>>>> documentation is
>>>>>> wrong. You could always write the output register and read the input
>>>>>> register. This means you could write the output first and afterwards
>>>>>> enable the output via the tri-state register.
>>>>> ok.
>>>> Maybe the set_direction function should be adapted to avoid glitches
>>>> because it first enabled the output with the old value.
>>> No problem with this change at all.
>>>
>>>>>>>>> If you want to support this configuration then you would have to
>>>>>>>>> change
>>>>>>>>> pin to input and read it and revert it back. And all of this
>>>>>>>>> should be
>>>>>>>>> done based on flag.
>>>>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>>>>> binding.
>>>>>>>>> Unfortunately this is for consumer not for generic listing. I can't
>>>>>>>>> also
>>>>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>>>>> driver
>>>>>>>>> but I can be wrong.
>>>>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1.
>>>>>>>> In our
>>>>>>>> case the open drain is transparent. The output has only two states
>>>>>>>> High-Z and 0. The input value of the FPGA output block is always
>>>>>>>> 0 and
>>>>>>>> the tri-state input is connected to the output register of the GPIO
>>>>>>>> controller. The output of the FPGA input block is connected to the
>>>>>>>> input
>>>>>>>> register of the GPIO controller.
>>>>>>> I just found out in connection to second thread we have together
>>>>>>> that in
>>>>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but
>>>>>>> they are
>>>>>>> not wired anywhere.
>>>>>>> I think in your case when these two functions are implemented you
>>>>>>> will
>>>>>>> get this functionality which you are asking about.
>>>>>> No. This function is to control a special register.
>>>>> I am not quite sure if this is really about special register or can be
>>>>> also used in our case.
>>>> In my case the open drain is transparent for the controller. The output
>>>> replace a 1 by a High-Z. This only means that you don't know the real
>>>> value on the wire.
>>> please look below.
>>>
>>>>>> Beside the open drain. What happens if the output is stuck to a fixed
>>>>>> signal. This could be detected if you read the input register.
>>>>> And again if that wires are there. Not covering the case where you have
>>>>> output only without any input signal at all.
>>>> This can be detect by the all-outputs flag.
>>> please look below.
>>>
>>>>>>> My guess is that gpio command should be extended to call these two
>>>>>>> functions. When set_open_drain is called special flag for pin is
>>>>>>> setup
>>>>>>> and for this pin get_status will always read data reg in our case and
>>>>>>> you get your values.
>>>>>> The main quest is which value should the get function return for an
>>>>>> output. The value from the input register which represents the real
>>>>>> value or the value from the output register which represents the
>>>>>> desired
>>>>>> value. Because of the "Warning: value of pin is still %d" inside the
>>>>>> gpio cmd I would expect the real value. This is also implemented by
>>>>>> most
>>>>>> drivers. Only tegra, kw and rcar distinguish between input and output.
>>>>>> Thereby the rcar explicitly mention a bug:
>>>>>>
>>>>>> Testing on r8a7790 shows that INDT does not show correct pin state
>>>>>> when
>>>>>> configured as output, so use OUTDT in case of output pins.
>>>>>>
>>>>>> A test of the Xilinx GPIO controller shows that a read always returns
>>>>>> the value of the gpio_io_i signal.
>>>>> What about to use this logic?
>>>>>
>>>>> if (platdata->bank_input[bank]) // input only
>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>> else if (platdata->bank_output[bank])) // output only
>>>>> val = priv->output_val[bank];
>>>> Okay
>>>>
>>>>> else { // tristate because it is not output or input only.
>>>>> state = xilinx_gpio_get_function(dev, offset)
>>>>> if (state == GPIOF_INPUT) // if input now - just read it
>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>> else { // if output - change it to input
>>>>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>>>>> tmp = tmp1 | (1 << pin);
>>>>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>>>>
>>>>> // read value
>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>
>>>>> // revert it back
>>>>> writel(tmp1, &platdata->regs->gpiodir + bank * 2);
>>>>> }
>>>>> }
>>>> This will generate a glitch on the wire.
>>>>
>>>> Why don't we simple read the value? The hardware support this and we
>>>> have already handle the all-output case:
>>>>
>>>> if (platdata->bank_output[bank]))
>>>> val = priv->output_val[bank];
>>>> else
>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>
>>>> If you don't trust my test could you please ask your hardware co works
>>>> why the data register behavior should depend on the tri-state register.
>>> I am looking for IP owner but as far as I see it was created 9 years ago
>>> that's why it will take some time.
>>> Anyway I have created hw design on zcu102 which has output on led and
>>> input on dip and output value is kept in data reg that's why your
>>> snippet above can be used.
>> Do you use a signal port for both or two separated ports?
> I expect single - yes the same pin 0 for output led and input dip.
The same pin or port?
>> Have you connect the gpio_io_i and gpio_io_o signals of the outputs?
> It is really pin 0 io_o to one fpga pin and pin0 io_i to another fpga pin.
To make sure I understand you correct: The gpio_io_o[0] is connected to
a LED and gpio_io_i[0] is connected to a switch?
Have you connect the gpio_io_t[0] signal?
>> What happens if you connect the gpio_io_i signals to zero for the output
>> to save resources?
>> What happens if you disable the power supply for the LED bank?
> didn't try that.
>
>>> Documentation is wrong in this
>>> "For each I/O bit programmed as output:
>>> • R: Reads to these bits always return zeros"
>>>
>>> I have sent 4 patches which we discussed that's why please reviewed them.
>>> I have one more patch in queue which is keeping output_val for certain
>>> bank but not sure if make sense to apply it because as above when
>>> direction is setup to output you can read desired output value from
>>> gpiodata reg.
>> This is only true if the gpio_io_i and gpio_io_o signals are connected
>> together or the input and output signals have always the same value.
>> Otherwise you read the input signal and write it into the output value.
> That's what I see on zcu102 without any connection between io_i io_o.
> There is incorrect behavior when gpio toggle command is called and input
> value is 1. Because the first gpio toggle cmd reads input value first
> (because code is doing that) and then setting up output with opposite value.
>
>
>> In our use case we have two gpio controllers (for two processors) which
>> are connected to one io port. Thereby only the output is multiplexed
>> between the two controllers and the input is always connected to the io.
>> This means both controller always read the input value of the io but
>> only one of them controls the output of the io. Your set function reads
>> the input value of the io (maybe controlled by the other controller) and
>> thereby overwrite the old output value of the controller.
>>
>>> I have tested it on gpio output only led case and gpiodata
>>> reg contain correct value.
>> Have you test outputs with differences between output value and input
>> value?
> I believe so.
>
>>> Anyway if I miss any patch in connection to this please send a patch and
>>> let's look at that use case.
>> Okay
> TBH it is starting to be a little bit confusing.
Me too.
It looks like we have observe different behavior. I will redo my test in
the next days.
It would be so much easier if we could check the IP's HDL.
The hardware have three physical registers (gpio_io_o, gpio_io_i,
gpio_io_t) per pin and two bus registers (GPIOx_DATA, GPIOx_TRI) per port.
-> GPIOx_TRI
-> Read
-> gpio_io_t
-> Write
-> gpio_io_t
-> GPIOx_DATA
-> Read
-> gpio_io_i (my observation)
-> (gpio_io_t == 1) ? gpio_io_i : gpio_io_o (your observation ?)
-> Write
-> gpio_io_o
> What we should really do is to use standard boards and connection and
> describe that cases and expected behavior and changes in code which
> should happen to support this.
>
> We are talking about two functions.
> xilinx_gpio_set_value and xilinx_gpio_get_value.
>
> And let's take mainline driver with that 4 patches I have sent.
Okay
> Let's start with xilinx_gpio_get_value()
> Mainline is all the time reading value from gpiodata reg no matter if
> this is output/input only or io.
>
> Above is this sequence which you have written that it is right
> if (platdata->bank_output[bank]))
> val = priv->output_val[bank];
> else
> val = readl(&platdata->regs->gpiodata + bank * 2);
Based on your observations the driver should work without this change
because the controller returns the output register value if the tristate
register is cleared. Hopefully this also happens if the all_outputs flag
is set.
> I have took at look at gpio output only case
>
> ZynqMP> gpio toggle gpio at a00010000
> gpio: pin gpio at a00010000 (gpio 187) value is 1
> ZynqMP> gpio status -a gpio at a0001000
> Bank gpio at a0001000:
> gpio at a00010000: output: 1 [ ]
> gpio at a00010001: output: 0 [ ]
> gpio at a00010002: output: 0 [ ]
> gpio at a00010003: output: 0 [ ]
> gpio at a00010004: output: 0 [ ]
> gpio at a00010005: output: 0 [ ]
> gpio at a00010006: output: 0 [ ]
> gpio at a00010007: output: 0 [ ]
> ZynqMP> gpio toggle gpio at a00010004
> gpio: pin gpio at a00010004 (gpio 191) value is 1
> ZynqMP> gpio status -a gpio at a0001000
> Bank gpio at a0001000:
> gpio at a00010000: output: 1 [ ]
> gpio at a00010001: output: 0 [ ]
> gpio at a00010002: output: 0 [ ]
> gpio at a00010003: output: 0 [ ]
> gpio at a00010004: output: 1 [ ]
> gpio at a00010005: output: 0 [ ]
> gpio at a00010006: output: 0 [ ]
> gpio at a00010007: output: 0 [ ]
> ZynqMP> gpio toggle gpio at a00010006
> gpio: pin gpio at a00010006 (gpio 193) value is 1
> ZynqMP> gpio status -a gpio at a0001000
> Bank gpio at a0001000:
> gpio at a00010000: output: 1 [ ]
> gpio at a00010001: output: 0 [ ]
> gpio at a00010002: output: 0 [ ]
> gpio at a00010003: output: 0 [ ]
> gpio at a00010004: output: 1 [ ]
> gpio at a00010005: output: 0 [ ]
> gpio at a00010006: output: 1 [ ]
> gpio at a00010007: output: 0 [ ]
> ZynqMP> gpio toggle gpio at a00010004
> gpio: pin gpio at a00010004 (gpio 191) value is 0
> ZynqMP> gpio status -a gpio at a0001000
> Bank gpio at a0001000:
> gpio at a00010000: output: 1 [ ]
> gpio at a00010001: output: 0 [ ]
> gpio at a00010002: output: 0 [ ]
> gpio at a00010003: output: 0 [ ]
> gpio at a00010004: output: 0 [ ]
> gpio at a00010005: output: 0 [ ]
> gpio at a00010006: output: 1 [ ]
> gpio at a00010007: output: 0 [ ]
> ZynqMP> gpio toggle gpio at a00010007
> gpio: pin gpio at a00010007 (gpio 194) value is 1
> ZynqMP> gpio status -a gpio at a0001000
> Bank gpio at a0001000:
> gpio at a00010000: output: 1 [ ]
> gpio at a00010001: output: 0 [ ]
> gpio at a00010002: output: 0 [ ]
> gpio at a00010003: output: 0 [ ]
> gpio at a00010004: output: 0 [ ]
> gpio at a00010005: output: 0 [ ]
> gpio at a00010006: output: 1 [ ]
> gpio at a00010007: output: 1 [ ]
>
> And for this case there is no need to take that value from any saved
> location.
> It means having
> val = readl(&platdata->regs->gpiodata + bank * 2);
> is enough based on my test.
> And for the rest as your example above this should be also fine.
Yes. If your observations are correct it is impossible to read the input
state if the GPIO is configured as output and if my observations are
correct the read will return the input state independently of the mode.
> When we have agreement on this we can look at xilinx_gpio_set_value.
The direct read is fine for me.
Can you read the two switch states if you run the following commands:
/* Enable LED */
ZynqMP> gpio set gpio at a00010000
/* Read switch state and keep LED on */
ZynqMP> gpio input gpio at a00010000
ZynqMP> gpio status -a gpio at a0001000
/* Manually change switch state and read again */
ZynqMP> gpio status -a gpio at a0001000
/* Disable LED */
ZynqMP> gpio clear gpio at a00010000
ZynqMP> gpio status -a gpio at a0001000
/* Manually change switch state and read again */
ZynqMP> gpio status -a gpio at a0001000
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-07-30 19:34 ` Stefan Herbrechtsmeier
@ 2018-08-01 18:36 ` Stefan Herbrechtsmeier
2018-08-02 12:56 ` Michal Simek
0 siblings, 1 reply; 19+ messages in thread
From: Stefan Herbrechtsmeier @ 2018-08-01 18:36 UTC (permalink / raw)
To: u-boot
Am 30.07.2018 um 21:34 schrieb Stefan Herbrechtsmeier:
> Am 30.07.2018 um 16:10 schrieb Michal Simek:
>> On 30.7.2018 15:32, Stefan Herbrechtsmeier wrote:
>>> Am 30.07.2018 um 14:40 schrieb Michal Simek:
>>>> On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
>>>>> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>>>>>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>>>>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>>>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>>>>>> Reading registers for finding out output value is not
>>>>>>>>>>>>>> working
>>>>>>>>>>>>>> because
>>>>>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier
>>>>>>>>>>>>>> <stefan@herbrechtsmeier.net>
>>>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>> [snip]
>>>>>>>>>>>
>>>>>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>>>>>> +
>>>>>>>>>>>>>> return val;
>>>>>>>>>>>>>> };
>>>>>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice
>>>>>>>>>>>>>> *dev,
>>>>>>>>>>>>>> unsigned
>>>>>>>>>>>>>> offset)
>>>>>>>>>>>>>> {
>>>>>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>>>>> int val, ret;
>>>>>>>>>>>>>> u32 bank, pin;
>>>>>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>>>>>> udevice
>>>>>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin
>>>>>>>>>>>>>> %x\n",
>>>>>>>>>>>>>> __func__,
>>>>>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank
>>>>>>>>>>>>>> * 2);
>>>>>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) ==
>>>>>>>>>>>>>> GPIOF_INPUT) {
>>>>>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>>>>>> + }
>>>>>>>>>>>>> Why you don't always read the data register? This doesn't
>>>>>>>>>>>>> work for
>>>>>>>>>>>>> three
>>>>>>>>>>>>> state outputs.
>>>>>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>>>>>> It means else part is output and I read saved value in
>>>>>>>>>>>> priv->output_val.
>>>>>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>>>>>> input
>>>>>>>>>>>> value.
>>>>>>>>>>>> Maybe you are commenting something else but please let me
>>>>>>>>>>>> know if
>>>>>>>>>>>> there
>>>>>>>>>>>> is any other bug.
>>>>>>>>>>> What happen if I have an open drain output. Even if the gpio
>>>>>>>>>>> output
>>>>>>>>>>> is 1
>>>>>>>>>>> the input could read a 0. You driver will always return the
>>>>>>>>>>> output
>>>>>>>>>>> value
>>>>>>>>>>> and not the real input value. According to the picture in
>>>>>>>>>>> documentation
>>>>>>>>>>> and my tests a data register write writes the output registers
>>>>>>>>>>> and a
>>>>>>>>>>> data register read reads the input registers.
>>>>>>>>>>>
>>>>>>>>>>> Why should the driver return the desired state (output
>>>>>>>>>>> register)
>>>>>>>>>>> and not
>>>>>>>>>>> the real state (input register)?
>>>>>>>>>> First of all thanks for description.
>>>>>>>>>>
>>>>>>>>>> I have another example where you have output only and you can't
>>>>>>>>>> read
>>>>>>>>>> input because there is no wire.
>>>>>>>>> Does you mean the all outputs configuration? Does this removes
>>>>>>>>> the
>>>>>>>>> "gpio_io_i" signal from the IP?
>>>>>>>> I am not sure what synthesis is doing with that unused IP pins
>>>>>>>> but I
>>>>>>>> would consider as a bug if this is automatically connected
>>>>>>>> together.
>>>>>>> I mean does the IP generator removes the gpio_io_i signal
>>>>>>> because it
>>>>>>> isn't needed? If the IP generator creates the gpio_io_i signal I
>>>>>>> would
>>>>>>> expect that you can't leave it unconnected as this would lead to
>>>>>>> undefined values.
>>>>>> Normally when you know that you have output only there is no no
>>>>>> gpio_io_i or tristate signal. The same for input only.
>>>>> And in this case the device tree flags "xlnx,all-inputs" or
>>>>> "xlnx,all-outputs" should be set.
>>>> yes.
>>>>
>>>>>>>> And
>>>>>>>> also wasting a logic if there is unused part.
>>>>>>>> But in Vivado you should be able to setup output pins to and input
>>>>>>>> pins
>>>>>>>> separately. There are In/Out/Tristate.
>>>>>>>> If you don't want to deal with external pin you can connect them
>>>>>>>> inside PL.
>>>>>>> This isn't my point. I mean that if you have an gpio_io_i signal
>>>>>>> you
>>>>>>> have to connected it to a signal. You could connect it to the
>>>>>>> output of
>>>>>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>>>>>> connect it to a fixed value (0 or 1) you get this value if you read
>>>>>>> the
>>>>>>> status of this GPIO.
>>>>>> Not a HW to tell you what's vivado is going to do with that. But you
>>>>>> can
>>>>>> select that ouput/input only where these signals are out.
>>>>> You mean the all-inputs or all-outputs case.
>>>> yes.
>>>>
>>>>>>>>>> Regarding open drain output.
>>>>>>>>>>
>>>>>>>>>> Also this is what it is written in manual:
>>>>>>>>>> "AXI GPIO Data Register Description"
>>>>>>>>>> "For each I/O bit programmed as output:
>>>>>>>>>> • R: Reads to these bits always return zeros"
>>>>>>>>> This must be a mistake in the documentation. I could read the
>>>>>>>>> input
>>>>>>>>> value.
>>>>>>>>>
>>>>>>>>> The old driver and the Linux driver always read the register.
>>>>>>>> In old u-boot driver I see
>>>>>>>> 179 if (gpio_get_direction(gpio) ==
>>>>>>>> GPIO_DIRECTION_OUT)
>>>>>>>> 180 val = gpio_get_output_value(gpio);
>>>>>>>> 181 else
>>>>>>>> 182 val = gpio_get_input_value(gpio);
>>>>>>>>
>>>>>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>>>>>> gpio_get_input_value() reads the reg.
>>>>>>> Sorry you are right. I mixed it with the zynq driver.
>>>>>> ok.
>>>>>>
>>>>>>
>>>>>>>> In Linux you are right it is read from reg in both cases.
>>>>>>>>
>>>>>>>>> Additionally the documentation states that a write to an input
>>>>>>>>> doesn't
>>>>>>>>> work:
>>>>>>>>> AXI GPIO Data Register.
>>>>>>>>> For each I/O bit programmed as input:
>>>>>>>>> • R: Reads value on the input pin.
>>>>>>>>> • W: No effect.
>>>>>>>>>
>>>>>>>>> However the Linux driver use the common sequence and sets
>>>>>>>>> first the
>>>>>>>>> data
>>>>>>>>> register and afterwards the direction register.
>>>>>>>> Possible that driver has pretty long history and I don't think
>>>>>>>> it was
>>>>>>>> tested for all cases.
>>>>>>> I test the IP again and it works like expected and the
>>>>>>> documentation is
>>>>>>> wrong. You could always write the output register and read the
>>>>>>> input
>>>>>>> register. This means you could write the output first and
>>>>>>> afterwards
>>>>>>> enable the output via the tri-state register.
>>>>>> ok.
>>>>> Maybe the set_direction function should be adapted to avoid glitches
>>>>> because it first enabled the output with the old value.
>>>> No problem with this change at all.
>>>>
>>>>>>>>>> If you want to support this configuration then you would have to
>>>>>>>>>> change
>>>>>>>>>> pin to input and read it and revert it back. And all of this
>>>>>>>>>> should be
>>>>>>>>>> done based on flag.
>>>>>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>>>>>> binding.
>>>>>>>>>> Unfortunately this is for consumer not for generic listing. I
>>>>>>>>>> can't
>>>>>>>>>> also
>>>>>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>>>>>> driver
>>>>>>>>>> but I can be wrong.
>>>>>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1.
>>>>>>>>> In our
>>>>>>>>> case the open drain is transparent. The output has only two
>>>>>>>>> states
>>>>>>>>> High-Z and 0. The input value of the FPGA output block is always
>>>>>>>>> 0 and
>>>>>>>>> the tri-state input is connected to the output register of the
>>>>>>>>> GPIO
>>>>>>>>> controller. The output of the FPGA input block is connected to
>>>>>>>>> the
>>>>>>>>> input
>>>>>>>>> register of the GPIO controller.
>>>>>>>> I just found out in connection to second thread we have together
>>>>>>>> that in
>>>>>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but
>>>>>>>> they are
>>>>>>>> not wired anywhere.
>>>>>>>> I think in your case when these two functions are implemented you
>>>>>>>> will
>>>>>>>> get this functionality which you are asking about.
>>>>>>> No. This function is to control a special register.
>>>>>> I am not quite sure if this is really about special register or
>>>>>> can be
>>>>>> also used in our case.
>>>>> In my case the open drain is transparent for the controller. The
>>>>> output
>>>>> replace a 1 by a High-Z. This only means that you don't know the real
>>>>> value on the wire.
>>>> please look below.
>>>>
>>>>>>> Beside the open drain. What happens if the output is stuck to a
>>>>>>> fixed
>>>>>>> signal. This could be detected if you read the input register.
>>>>>> And again if that wires are there. Not covering the case where
>>>>>> you have
>>>>>> output only without any input signal at all.
>>>>> This can be detect by the all-outputs flag.
>>>> please look below.
>>>>
>>>>>>>> My guess is that gpio command should be extended to call these two
>>>>>>>> functions. When set_open_drain is called special flag for pin is
>>>>>>>> setup
>>>>>>>> and for this pin get_status will always read data reg in our
>>>>>>>> case and
>>>>>>>> you get your values.
>>>>>>> The main quest is which value should the get function return for an
>>>>>>> output. The value from the input register which represents the real
>>>>>>> value or the value from the output register which represents the
>>>>>>> desired
>>>>>>> value. Because of the "Warning: value of pin is still %d" inside
>>>>>>> the
>>>>>>> gpio cmd I would expect the real value. This is also implemented by
>>>>>>> most
>>>>>>> drivers. Only tegra, kw and rcar distinguish between input and
>>>>>>> output.
>>>>>>> Thereby the rcar explicitly mention a bug:
>>>>>>>
>>>>>>> Testing on r8a7790 shows that INDT does not show correct pin state
>>>>>>> when
>>>>>>> configured as output, so use OUTDT in case of output pins.
>>>>>>>
>>>>>>> A test of the Xilinx GPIO controller shows that a read always
>>>>>>> returns
>>>>>>> the value of the gpio_io_i signal.
>>>>>> What about to use this logic?
>>>>>>
>>>>>> if (platdata->bank_input[bank]) // input only
>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>> else if (platdata->bank_output[bank])) // output only
>>>>>> val = priv->output_val[bank];
>>>>> Okay
>>>>>
>>>>>> else { // tristate because it is not output or input only.
>>>>>> state = xilinx_gpio_get_function(dev, offset)
>>>>>> if (state == GPIOF_INPUT) // if input now - just read it
>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>> else { // if output - change it to input
>>>>>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>>>>>> tmp = tmp1 | (1 << pin);
>>>>>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>>>>>
>>>>>> // read value
>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>
>>>>>> // revert it back
>>>>>> writel(tmp1, &platdata->regs->gpiodir + bank *
>>>>>> 2);
>>>>>> }
>>>>>> }
>>>>> This will generate a glitch on the wire.
>>>>>
>>>>> Why don't we simple read the value? The hardware support this and we
>>>>> have already handle the all-output case:
>>>>>
>>>>> if (platdata->bank_output[bank]))
>>>>> val = priv->output_val[bank];
>>>>> else
>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>
>>>>> If you don't trust my test could you please ask your hardware co
>>>>> works
>>>>> why the data register behavior should depend on the tri-state
>>>>> register.
>>>> I am looking for IP owner but as far as I see it was created 9
>>>> years ago
>>>> that's why it will take some time.
>>>> Anyway I have created hw design on zcu102 which has output on led and
>>>> input on dip and output value is kept in data reg that's why your
>>>> snippet above can be used.
>>> Do you use a signal port for both or two separated ports?
>> I expect single - yes the same pin 0 for output led and input dip.
>
> The same pin or port?
>
>>> Have you connect the gpio_io_i and gpio_io_o signals of the outputs?
>> It is really pin 0 io_o to one fpga pin and pin0 io_i to another fpga
>> pin.
>
> To make sure I understand you correct: The gpio_io_o[0] is connected
> to a LED and gpio_io_i[0] is connected to a switch?
>
> Have you connect the gpio_io_t[0] signal?
>
>>> What happens if you connect the gpio_io_i signals to zero for the
>>> output
>>> to save resources?
>>> What happens if you disable the power supply for the LED bank?
>> didn't try that.
>>
>>>> Documentation is wrong in this
>>>> "For each I/O bit programmed as output:
>>>> • R: Reads to these bits always return zeros"
>>>>
>>>> I have sent 4 patches which we discussed that's why please reviewed
>>>> them.
>>>> I have one more patch in queue which is keeping output_val for certain
>>>> bank but not sure if make sense to apply it because as above when
>>>> direction is setup to output you can read desired output value from
>>>> gpiodata reg.
>>> This is only true if the gpio_io_i and gpio_io_o signals are connected
>>> together or the input and output signals have always the same value.
>>> Otherwise you read the input signal and write it into the output value.
>> That's what I see on zcu102 without any connection between io_i io_o.
>> There is incorrect behavior when gpio toggle command is called and input
>> value is 1. Because the first gpio toggle cmd reads input value first
>> (because code is doing that) and then setting up output with opposite
>> value.
>>
>>
>>> In our use case we have two gpio controllers (for two processors) which
>>> are connected to one io port. Thereby only the output is multiplexed
>>> between the two controllers and the input is always connected to the
>>> io.
>>> This means both controller always read the input value of the io but
>>> only one of them controls the output of the io. Your set function reads
>>> the input value of the io (maybe controlled by the other controller)
>>> and
>>> thereby overwrite the old output value of the controller.
>>>
>>>> I have tested it on gpio output only led case and gpiodata
>>>> reg contain correct value.
>>> Have you test outputs with differences between output value and input
>>> value?
>> I believe so.
>>
>>>> Anyway if I miss any patch in connection to this please send a
>>>> patch and
>>>> let's look at that use case.
>>> Okay
>> TBH it is starting to be a little bit confusing.
>
> Me too.
>
> It looks like we have observe different behavior. I will redo my test
> in the next days.
>
> It would be so much easier if we could check the IP's HDL.
>
> The hardware have three physical registers (gpio_io_o, gpio_io_i,
> gpio_io_t) per pin and two bus registers (GPIOx_DATA, GPIOx_TRI) per
> port.
>
> -> GPIOx_TRI
> -> Read
> -> gpio_io_t
> -> Write
> -> gpio_io_t
>
> -> GPIOx_DATA
> -> Read
> -> gpio_io_i (my observation)
> -> (gpio_io_t == 1) ? gpio_io_i : gpio_io_o (your
> observation ?)
> -> Write
> -> gpio_io_o
>
>> What we should really do is to use standard boards and connection and
>> describe that cases and expected behavior and changes in code which
>> should happen to support this.
>>
>> We are talking about two functions.
>> xilinx_gpio_set_value and xilinx_gpio_get_value.
>>
>> And let's take mainline driver with that 4 patches I have sent.
>
> Okay
>
>> Let's start with xilinx_gpio_get_value()
>> Mainline is all the time reading value from gpiodata reg no matter if
>> this is output/input only or io.
>>
>> Above is this sequence which you have written that it is right
>> if (platdata->bank_output[bank]))
>> val = priv->output_val[bank];
>> else
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>
> Based on your observations the driver should work without this change
> because the controller returns the output register value if the
> tristate register is cleared. Hopefully this also happens if the
> all_outputs flag is set.
>
>> I have took at look at gpio output only case
>>
>> ZynqMP> gpio toggle gpio at a00010000
>> gpio: pin gpio at a00010000 (gpio 187) value is 1
>> ZynqMP> gpio status -a gpio at a0001000
>> Bank gpio at a0001000:
>> gpio at a00010000: output: 1 [ ]
>> gpio at a00010001: output: 0 [ ]
>> gpio at a00010002: output: 0 [ ]
>> gpio at a00010003: output: 0 [ ]
>> gpio at a00010004: output: 0 [ ]
>> gpio at a00010005: output: 0 [ ]
>> gpio at a00010006: output: 0 [ ]
>> gpio at a00010007: output: 0 [ ]
>> ZynqMP> gpio toggle gpio at a00010004
>> gpio: pin gpio at a00010004 (gpio 191) value is 1
>> ZynqMP> gpio status -a gpio at a0001000
>> Bank gpio at a0001000:
>> gpio at a00010000: output: 1 [ ]
>> gpio at a00010001: output: 0 [ ]
>> gpio at a00010002: output: 0 [ ]
>> gpio at a00010003: output: 0 [ ]
>> gpio at a00010004: output: 1 [ ]
>> gpio at a00010005: output: 0 [ ]
>> gpio at a00010006: output: 0 [ ]
>> gpio at a00010007: output: 0 [ ]
>> ZynqMP> gpio toggle gpio at a00010006
>> gpio: pin gpio at a00010006 (gpio 193) value is 1
>> ZynqMP> gpio status -a gpio at a0001000
>> Bank gpio at a0001000:
>> gpio at a00010000: output: 1 [ ]
>> gpio at a00010001: output: 0 [ ]
>> gpio at a00010002: output: 0 [ ]
>> gpio at a00010003: output: 0 [ ]
>> gpio at a00010004: output: 1 [ ]
>> gpio at a00010005: output: 0 [ ]
>> gpio at a00010006: output: 1 [ ]
>> gpio at a00010007: output: 0 [ ]
>> ZynqMP> gpio toggle gpio at a00010004
>> gpio: pin gpio at a00010004 (gpio 191) value is 0
>> ZynqMP> gpio status -a gpio at a0001000
>> Bank gpio at a0001000:
>> gpio at a00010000: output: 1 [ ]
>> gpio at a00010001: output: 0 [ ]
>> gpio at a00010002: output: 0 [ ]
>> gpio at a00010003: output: 0 [ ]
>> gpio at a00010004: output: 0 [ ]
>> gpio at a00010005: output: 0 [ ]
>> gpio at a00010006: output: 1 [ ]
>> gpio at a00010007: output: 0 [ ]
>> ZynqMP> gpio toggle gpio at a00010007
>> gpio: pin gpio at a00010007 (gpio 194) value is 1
>> ZynqMP> gpio status -a gpio at a0001000
>> Bank gpio at a0001000:
>> gpio at a00010000: output: 1 [ ]
>> gpio at a00010001: output: 0 [ ]
>> gpio at a00010002: output: 0 [ ]
>> gpio at a00010003: output: 0 [ ]
>> gpio at a00010004: output: 0 [ ]
>> gpio at a00010005: output: 0 [ ]
>> gpio at a00010006: output: 1 [ ]
>> gpio at a00010007: output: 1 [ ]
>>
>> And for this case there is no need to take that value from any saved
>> location.
>> It means having
>> val = readl(&platdata->regs->gpiodata + bank * 2);
>> is enough based on my test.
>> And for the rest as your example above this should be also fine.
>
> Yes. If your observations are correct it is impossible to read the
> input state if the GPIO is configured as output and if my observations
> are correct the read will return the input state independently of the
> mode.
>
>> When we have agreement on this we can look at xilinx_gpio_set_value.
>
> The direct read is fine for me.
>
> Can you read the two switch states if you run the following commands:
>
> /* Enable LED */
> ZynqMP> gpio set gpio at a00010000
> /* Read switch state and keep LED on */
> ZynqMP> gpio input gpio at a00010000
> ZynqMP> gpio status -a gpio at a0001000
> /* Manually change switch state and read again */
> ZynqMP> gpio status -a gpio at a0001000
> /* Disable LED */
> ZynqMP> gpio clear gpio at a00010000
> ZynqMP> gpio status -a gpio at a0001000
> /* Manually change switch state and read again */
> ZynqMP> gpio status -a gpio at a0001000
>
> Best regards
> Stefan
>
I have test the GPIO controller by read and write the register via the
md and mw command. The read *always* returns the status of the gpio_io_i
signal independent of the gpio_io_t status. This means to support my
version of the IP the driver needs to save the output value in a
variable. This solution should work for your and my hardware. Could you
repost your patch to add the dout-default and output_val or should I
post a patch based on your patch?
Best regards
Stefan
^ permalink raw reply [flat|nested] 19+ messages in thread
* [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
2018-08-01 18:36 ` Stefan Herbrechtsmeier
@ 2018-08-02 12:56 ` Michal Simek
0 siblings, 0 replies; 19+ messages in thread
From: Michal Simek @ 2018-08-02 12:56 UTC (permalink / raw)
To: u-boot
On 1.8.2018 20:36, Stefan Herbrechtsmeier wrote:
> Am 30.07.2018 um 21:34 schrieb Stefan Herbrechtsmeier:
>> Am 30.07.2018 um 16:10 schrieb Michal Simek:
>>> On 30.7.2018 15:32, Stefan Herbrechtsmeier wrote:
>>>> Am 30.07.2018 um 14:40 schrieb Michal Simek:
>>>>> On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
>>>>>> Am 27.07.2018 um 09:05 schrieb Michal Simek:
>>>>>>> On 26.7.2018 21:46, Stefan Herbrechtsmeier wrote:
>>>>>>>> Am 26.07.2018 um 10:41 schrieb Michal Simek:
>>>>>>>>> On 25.7.2018 20:21, Stefan Herbrechtsmeier wrote:
>>>>>>>>>> Am 25.07.2018 um 08:39 schrieb Michal Simek:
>>>>>>>>>>> On 24.7.2018 21:56, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>>> Am 24.07.2018 um 12:31 schrieb Michal Simek:
>>>>>>>>>>>>> On 23.7.2018 20:42, Stefan Herbrechtsmeier wrote:
>>>>>>>>>>>>>> Am 23.07.2018 um 13:43 schrieb Michal Simek:
>>>>>>>>>>>>>>> Reading registers for finding out output value is not
>>>>>>>>>>>>>>> working
>>>>>>>>>>>>>>> because
>>>>>>>>>>>>>>> input value is read instead in case of tristate.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> Reported-by: Stefan Herbrechtsmeier
>>>>>>>>>>>>>>> <stefan@herbrechtsmeier.net>
>>>>>>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xilinx.com>
>>>>>>>>>>>>>>> ---
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> drivers/gpio/xilinx_gpio.c | 38
>>>>>>>>>>>>>>> +++++++++++++++++++++++++++++++++-----
>>>>>>>>>>>>>>> 1 file changed, 33 insertions(+), 5 deletions(-)
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> diff --git a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>>> b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>>> index 4da9ae114d87..9d3e9379d0e5 100644
>>>>>>>>>>>>>>> --- a/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>>>>> +++ b/drivers/gpio/xilinx_gpio.c
>>>>>>>>>>>> [snip]
>>>>>>>>>>>>
>>>>>>>>>>>>>>> + priv->output_val[bank] = val;
>>>>>>>>>>>>>>> +
>>>>>>>>>>>>>>> return val;
>>>>>>>>>>>>>>> };
>>>>>>>>>>>>>>> @@ -441,6 +449,7 @@ static int
>>>>>>>>>>>>>>> xilinx_gpio_get_function(struct
>>>>>>>>>>>>>>> udevice *dev, unsigned offset)
>>>>>>>>>>>>>>> static int xilinx_gpio_get_value(struct udevice
>>>>>>>>>>>>>>> *dev,
>>>>>>>>>>>>>>> unsigned
>>>>>>>>>>>>>>> offset)
>>>>>>>>>>>>>>> {
>>>>>>>>>>>>>>> struct xilinx_gpio_platdata *platdata =
>>>>>>>>>>>>>>> dev_get_platdata(dev);
>>>>>>>>>>>>>>> + struct xilinx_gpio_privdata *priv = dev_get_priv(dev);
>>>>>>>>>>>>>>> int val, ret;
>>>>>>>>>>>>>>> u32 bank, pin;
>>>>>>>>>>>>>>> @@ -451,7 +460,14 @@ static int
>>>>>>>>>>>>>>> xilinx_gpio_get_value(struct
>>>>>>>>>>>>>>> udevice
>>>>>>>>>>>>>>> *dev, unsigned offset)
>>>>>>>>>>>>>>> debug("%s: regs: %lx, gpio: %x, bank %x, pin
>>>>>>>>>>>>>>> %x\n",
>>>>>>>>>>>>>>> __func__,
>>>>>>>>>>>>>>> (ulong)platdata->regs, offset, bank, pin);
>>>>>>>>>>>>>>> - val = readl(&platdata->regs->gpiodata + bank
>>>>>>>>>>>>>>> * 2);
>>>>>>>>>>>>>>> + if (xilinx_gpio_get_function(dev, offset) ==
>>>>>>>>>>>>>>> GPIOF_INPUT) {
>>>>>>>>>>>>>>> + debug("%s: Read input value from reg\n", __func__);
>>>>>>>>>>>>>>> + val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>>>>>>>>> + } else {
>>>>>>>>>>>>>>> + debug("%s: Read saved output value\n", __func__);
>>>>>>>>>>>>>>> + val = priv->output_val[bank];
>>>>>>>>>>>>>>> + }
>>>>>>>>>>>>>> Why you don't always read the data register? This doesn't
>>>>>>>>>>>>>> work for
>>>>>>>>>>>>>> three
>>>>>>>>>>>>>> state outputs.
>>>>>>>>>>>>> In three state register every bit/pin is 0 - output, 1 input.
>>>>>>>>>>>>> It means else part is output and I read saved value in
>>>>>>>>>>>>> priv->output_val.
>>>>>>>>>>>>> If pin is setup as INPUT then I need read data reg to find out
>>>>>>>>>>>>> input
>>>>>>>>>>>>> value.
>>>>>>>>>>>>> Maybe you are commenting something else but please let me
>>>>>>>>>>>>> know if
>>>>>>>>>>>>> there
>>>>>>>>>>>>> is any other bug.
>>>>>>>>>>>> What happen if I have an open drain output. Even if the gpio
>>>>>>>>>>>> output
>>>>>>>>>>>> is 1
>>>>>>>>>>>> the input could read a 0. You driver will always return the
>>>>>>>>>>>> output
>>>>>>>>>>>> value
>>>>>>>>>>>> and not the real input value. According to the picture in
>>>>>>>>>>>> documentation
>>>>>>>>>>>> and my tests a data register write writes the output registers
>>>>>>>>>>>> and a
>>>>>>>>>>>> data register read reads the input registers.
>>>>>>>>>>>>
>>>>>>>>>>>> Why should the driver return the desired state (output
>>>>>>>>>>>> register)
>>>>>>>>>>>> and not
>>>>>>>>>>>> the real state (input register)?
>>>>>>>>>>> First of all thanks for description.
>>>>>>>>>>>
>>>>>>>>>>> I have another example where you have output only and you can't
>>>>>>>>>>> read
>>>>>>>>>>> input because there is no wire.
>>>>>>>>>> Does you mean the all outputs configuration? Does this removes
>>>>>>>>>> the
>>>>>>>>>> "gpio_io_i" signal from the IP?
>>>>>>>>> I am not sure what synthesis is doing with that unused IP pins
>>>>>>>>> but I
>>>>>>>>> would consider as a bug if this is automatically connected
>>>>>>>>> together.
>>>>>>>> I mean does the IP generator removes the gpio_io_i signal
>>>>>>>> because it
>>>>>>>> isn't needed? If the IP generator creates the gpio_io_i signal I
>>>>>>>> would
>>>>>>>> expect that you can't leave it unconnected as this would lead to
>>>>>>>> undefined values.
>>>>>>> Normally when you know that you have output only there is no no
>>>>>>> gpio_io_i or tristate signal. The same for input only.
>>>>>> And in this case the device tree flags "xlnx,all-inputs" or
>>>>>> "xlnx,all-outputs" should be set.
>>>>> yes.
>>>>>
>>>>>>>>> And
>>>>>>>>> also wasting a logic if there is unused part.
>>>>>>>>> But in Vivado you should be able to setup output pins to and input
>>>>>>>>> pins
>>>>>>>>> separately. There are In/Out/Tristate.
>>>>>>>>> If you don't want to deal with external pin you can connect them
>>>>>>>>> inside PL.
>>>>>>>> This isn't my point. I mean that if you have an gpio_io_i signal
>>>>>>>> you
>>>>>>>> have to connected it to a signal. You could connect it to the
>>>>>>>> output of
>>>>>>>> an IO, to the gpio_io_o signal or to fixed value (0 or 1). If you
>>>>>>>> connect it to a fixed value (0 or 1) you get this value if you read
>>>>>>>> the
>>>>>>>> status of this GPIO.
>>>>>>> Not a HW to tell you what's vivado is going to do with that. But you
>>>>>>> can
>>>>>>> select that ouput/input only where these signals are out.
>>>>>> You mean the all-inputs or all-outputs case.
>>>>> yes.
>>>>>
>>>>>>>>>>> Regarding open drain output.
>>>>>>>>>>>
>>>>>>>>>>> Also this is what it is written in manual:
>>>>>>>>>>> "AXI GPIO Data Register Description"
>>>>>>>>>>> "For each I/O bit programmed as output:
>>>>>>>>>>> • R: Reads to these bits always return zeros"
>>>>>>>>>> This must be a mistake in the documentation. I could read the
>>>>>>>>>> input
>>>>>>>>>> value.
>>>>>>>>>>
>>>>>>>>>> The old driver and the Linux driver always read the register.
>>>>>>>>> In old u-boot driver I see
>>>>>>>>> 179 if (gpio_get_direction(gpio) ==
>>>>>>>>> GPIO_DIRECTION_OUT)
>>>>>>>>> 180 val = gpio_get_output_value(gpio);
>>>>>>>>> 181 else
>>>>>>>>> 182 val = gpio_get_input_value(gpio);
>>>>>>>>>
>>>>>>>>> gpio_get_output_value() reads it from gpiodata_store for output
>>>>>>>>> gpio_get_input_value() reads the reg.
>>>>>>>> Sorry you are right. I mixed it with the zynq driver.
>>>>>>> ok.
>>>>>>>
>>>>>>>
>>>>>>>>> In Linux you are right it is read from reg in both cases.
>>>>>>>>>
>>>>>>>>>> Additionally the documentation states that a write to an input
>>>>>>>>>> doesn't
>>>>>>>>>> work:
>>>>>>>>>> AXI GPIO Data Register.
>>>>>>>>>> For each I/O bit programmed as input:
>>>>>>>>>> • R: Reads value on the input pin.
>>>>>>>>>> • W: No effect.
>>>>>>>>>>
>>>>>>>>>> However the Linux driver use the common sequence and sets
>>>>>>>>>> first the
>>>>>>>>>> data
>>>>>>>>>> register and afterwards the direction register.
>>>>>>>>> Possible that driver has pretty long history and I don't think
>>>>>>>>> it was
>>>>>>>>> tested for all cases.
>>>>>>>> I test the IP again and it works like expected and the
>>>>>>>> documentation is
>>>>>>>> wrong. You could always write the output register and read the
>>>>>>>> input
>>>>>>>> register. This means you could write the output first and
>>>>>>>> afterwards
>>>>>>>> enable the output via the tri-state register.
>>>>>>> ok.
>>>>>> Maybe the set_direction function should be adapted to avoid glitches
>>>>>> because it first enabled the output with the old value.
>>>>> No problem with this change at all.
>>>>>
>>>>>>>>>>> If you want to support this configuration then you would have to
>>>>>>>>>>> change
>>>>>>>>>>> pin to input and read it and revert it back. And all of this
>>>>>>>>>>> should be
>>>>>>>>>>> done based on flag.
>>>>>>>>>>> There is macro GPIO_OPEN_DRAIN which could be specified via DT
>>>>>>>>>>> binding.
>>>>>>>>>>> Unfortunately this is for consumer not for generic listing. I
>>>>>>>>>>> can't
>>>>>>>>>>> also
>>>>>>>>>>> see it in gpio_desc flags to be able to handle it in the driver.
>>>>>>>>>>> That's why I have doubts if this is supported by any u-boot gpio
>>>>>>>>>>> driver
>>>>>>>>>>> but I can be wrong.
>>>>>>>>>> I think the GPIO_OPEN_DRAIN is used to not set the output to 1.
>>>>>>>>>> In our
>>>>>>>>>> case the open drain is transparent. The output has only two
>>>>>>>>>> states
>>>>>>>>>> High-Z and 0. The input value of the FPGA output block is always
>>>>>>>>>> 0 and
>>>>>>>>>> the tri-state input is connected to the output register of the
>>>>>>>>>> GPIO
>>>>>>>>>> controller. The output of the FPGA input block is connected to
>>>>>>>>>> the
>>>>>>>>>> input
>>>>>>>>>> register of the GPIO controller.
>>>>>>>>> I just found out in connection to second thread we have together
>>>>>>>>> that in
>>>>>>>>> gpio-uclass there are dm_gpio_set/get_open_drain functions but
>>>>>>>>> they are
>>>>>>>>> not wired anywhere.
>>>>>>>>> I think in your case when these two functions are implemented you
>>>>>>>>> will
>>>>>>>>> get this functionality which you are asking about.
>>>>>>>> No. This function is to control a special register.
>>>>>>> I am not quite sure if this is really about special register or
>>>>>>> can be
>>>>>>> also used in our case.
>>>>>> In my case the open drain is transparent for the controller. The
>>>>>> output
>>>>>> replace a 1 by a High-Z. This only means that you don't know the real
>>>>>> value on the wire.
>>>>> please look below.
>>>>>
>>>>>>>> Beside the open drain. What happens if the output is stuck to a
>>>>>>>> fixed
>>>>>>>> signal. This could be detected if you read the input register.
>>>>>>> And again if that wires are there. Not covering the case where
>>>>>>> you have
>>>>>>> output only without any input signal at all.
>>>>>> This can be detect by the all-outputs flag.
>>>>> please look below.
>>>>>
>>>>>>>>> My guess is that gpio command should be extended to call these two
>>>>>>>>> functions. When set_open_drain is called special flag for pin is
>>>>>>>>> setup
>>>>>>>>> and for this pin get_status will always read data reg in our
>>>>>>>>> case and
>>>>>>>>> you get your values.
>>>>>>>> The main quest is which value should the get function return for an
>>>>>>>> output. The value from the input register which represents the real
>>>>>>>> value or the value from the output register which represents the
>>>>>>>> desired
>>>>>>>> value. Because of the "Warning: value of pin is still %d" inside
>>>>>>>> the
>>>>>>>> gpio cmd I would expect the real value. This is also implemented by
>>>>>>>> most
>>>>>>>> drivers. Only tegra, kw and rcar distinguish between input and
>>>>>>>> output.
>>>>>>>> Thereby the rcar explicitly mention a bug:
>>>>>>>>
>>>>>>>> Testing on r8a7790 shows that INDT does not show correct pin state
>>>>>>>> when
>>>>>>>> configured as output, so use OUTDT in case of output pins.
>>>>>>>>
>>>>>>>> A test of the Xilinx GPIO controller shows that a read always
>>>>>>>> returns
>>>>>>>> the value of the gpio_io_i signal.
>>>>>>> What about to use this logic?
>>>>>>>
>>>>>>> if (platdata->bank_input[bank]) // input only
>>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>> else if (platdata->bank_output[bank])) // output only
>>>>>>> val = priv->output_val[bank];
>>>>>> Okay
>>>>>>
>>>>>>> else { // tristate because it is not output or input only.
>>>>>>> state = xilinx_gpio_get_function(dev, offset)
>>>>>>> if (state == GPIOF_INPUT) // if input now - just read it
>>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>> else { // if output - change it to input
>>>>>>> tmp1 = readl(&platdata->regs->gpiodir + bank * 2);
>>>>>>> tmp = tmp1 | (1 << pin);
>>>>>>> writel(tmp, &platdata->regs->gpiodir + bank * 2);
>>>>>>>
>>>>>>> // read value
>>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>>
>>>>>>> // revert it back
>>>>>>> writel(tmp1, &platdata->regs->gpiodir + bank *
>>>>>>> 2);
>>>>>>> }
>>>>>>> }
>>>>>> This will generate a glitch on the wire.
>>>>>>
>>>>>> Why don't we simple read the value? The hardware support this and we
>>>>>> have already handle the all-output case:
>>>>>>
>>>>>> if (platdata->bank_output[bank]))
>>>>>> val = priv->output_val[bank];
>>>>>> else
>>>>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>>>>>
>>>>>> If you don't trust my test could you please ask your hardware co
>>>>>> works
>>>>>> why the data register behavior should depend on the tri-state
>>>>>> register.
>>>>> I am looking for IP owner but as far as I see it was created 9
>>>>> years ago
>>>>> that's why it will take some time.
>>>>> Anyway I have created hw design on zcu102 which has output on led and
>>>>> input on dip and output value is kept in data reg that's why your
>>>>> snippet above can be used.
>>>> Do you use a signal port for both or two separated ports?
>>> I expect single - yes the same pin 0 for output led and input dip.
>>
>> The same pin or port?
>>
>>>> Have you connect the gpio_io_i and gpio_io_o signals of the outputs?
>>> It is really pin 0 io_o to one fpga pin and pin0 io_i to another fpga
>>> pin.
>>
>> To make sure I understand you correct: The gpio_io_o[0] is connected
>> to a LED and gpio_io_i[0] is connected to a switch?
>>
>> Have you connect the gpio_io_t[0] signal?
>>
>>>> What happens if you connect the gpio_io_i signals to zero for the
>>>> output
>>>> to save resources?
>>>> What happens if you disable the power supply for the LED bank?
>>> didn't try that.
>>>
>>>>> Documentation is wrong in this
>>>>> "For each I/O bit programmed as output:
>>>>> • R: Reads to these bits always return zeros"
>>>>>
>>>>> I have sent 4 patches which we discussed that's why please reviewed
>>>>> them.
>>>>> I have one more patch in queue which is keeping output_val for certain
>>>>> bank but not sure if make sense to apply it because as above when
>>>>> direction is setup to output you can read desired output value from
>>>>> gpiodata reg.
>>>> This is only true if the gpio_io_i and gpio_io_o signals are connected
>>>> together or the input and output signals have always the same value.
>>>> Otherwise you read the input signal and write it into the output value.
>>> That's what I see on zcu102 without any connection between io_i io_o.
>>> There is incorrect behavior when gpio toggle command is called and input
>>> value is 1. Because the first gpio toggle cmd reads input value first
>>> (because code is doing that) and then setting up output with opposite
>>> value.
>>>
>>>
>>>> In our use case we have two gpio controllers (for two processors) which
>>>> are connected to one io port. Thereby only the output is multiplexed
>>>> between the two controllers and the input is always connected to the
>>>> io.
>>>> This means both controller always read the input value of the io but
>>>> only one of them controls the output of the io. Your set function reads
>>>> the input value of the io (maybe controlled by the other controller)
>>>> and
>>>> thereby overwrite the old output value of the controller.
>>>>
>>>>> I have tested it on gpio output only led case and gpiodata
>>>>> reg contain correct value.
>>>> Have you test outputs with differences between output value and input
>>>> value?
>>> I believe so.
>>>
>>>>> Anyway if I miss any patch in connection to this please send a
>>>>> patch and
>>>>> let's look at that use case.
>>>> Okay
>>> TBH it is starting to be a little bit confusing.
>>
>> Me too.
>>
>> It looks like we have observe different behavior. I will redo my test
>> in the next days.
>>
>> It would be so much easier if we could check the IP's HDL.
>>
>> The hardware have three physical registers (gpio_io_o, gpio_io_i,
>> gpio_io_t) per pin and two bus registers (GPIOx_DATA, GPIOx_TRI) per
>> port.
>>
>> -> GPIOx_TRI
>> -> Read
>> -> gpio_io_t
>> -> Write
>> -> gpio_io_t
>>
>> -> GPIOx_DATA
>> -> Read
>> -> gpio_io_i (my observation)
>> -> (gpio_io_t == 1) ? gpio_io_i : gpio_io_o (your
>> observation ?)
>> -> Write
>> -> gpio_io_o
>>
>>> What we should really do is to use standard boards and connection and
>>> describe that cases and expected behavior and changes in code which
>>> should happen to support this.
>>>
>>> We are talking about two functions.
>>> xilinx_gpio_set_value and xilinx_gpio_get_value.
>>>
>>> And let's take mainline driver with that 4 patches I have sent.
>>
>> Okay
>>
>>> Let's start with xilinx_gpio_get_value()
>>> Mainline is all the time reading value from gpiodata reg no matter if
>>> this is output/input only or io.
>>>
>>> Above is this sequence which you have written that it is right
>>> if (platdata->bank_output[bank]))
>>> val = priv->output_val[bank];
>>> else
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>
>> Based on your observations the driver should work without this change
>> because the controller returns the output register value if the
>> tristate register is cleared. Hopefully this also happens if the
>> all_outputs flag is set.
>>
>>> I have took at look at gpio output only case
>>>
>>> ZynqMP> gpio toggle gpio at a00010000
>>> gpio: pin gpio at a00010000 (gpio 187) value is 1
>>> ZynqMP> gpio status -a gpio at a0001000
>>> Bank gpio at a0001000:
>>> gpio at a00010000: output: 1 [ ]
>>> gpio at a00010001: output: 0 [ ]
>>> gpio at a00010002: output: 0 [ ]
>>> gpio at a00010003: output: 0 [ ]
>>> gpio at a00010004: output: 0 [ ]
>>> gpio at a00010005: output: 0 [ ]
>>> gpio at a00010006: output: 0 [ ]
>>> gpio at a00010007: output: 0 [ ]
>>> ZynqMP> gpio toggle gpio at a00010004
>>> gpio: pin gpio at a00010004 (gpio 191) value is 1
>>> ZynqMP> gpio status -a gpio at a0001000
>>> Bank gpio at a0001000:
>>> gpio at a00010000: output: 1 [ ]
>>> gpio at a00010001: output: 0 [ ]
>>> gpio at a00010002: output: 0 [ ]
>>> gpio at a00010003: output: 0 [ ]
>>> gpio at a00010004: output: 1 [ ]
>>> gpio at a00010005: output: 0 [ ]
>>> gpio at a00010006: output: 0 [ ]
>>> gpio at a00010007: output: 0 [ ]
>>> ZynqMP> gpio toggle gpio at a00010006
>>> gpio: pin gpio at a00010006 (gpio 193) value is 1
>>> ZynqMP> gpio status -a gpio at a0001000
>>> Bank gpio at a0001000:
>>> gpio at a00010000: output: 1 [ ]
>>> gpio at a00010001: output: 0 [ ]
>>> gpio at a00010002: output: 0 [ ]
>>> gpio at a00010003: output: 0 [ ]
>>> gpio at a00010004: output: 1 [ ]
>>> gpio at a00010005: output: 0 [ ]
>>> gpio at a00010006: output: 1 [ ]
>>> gpio at a00010007: output: 0 [ ]
>>> ZynqMP> gpio toggle gpio at a00010004
>>> gpio: pin gpio at a00010004 (gpio 191) value is 0
>>> ZynqMP> gpio status -a gpio at a0001000
>>> Bank gpio at a0001000:
>>> gpio at a00010000: output: 1 [ ]
>>> gpio at a00010001: output: 0 [ ]
>>> gpio at a00010002: output: 0 [ ]
>>> gpio at a00010003: output: 0 [ ]
>>> gpio at a00010004: output: 0 [ ]
>>> gpio at a00010005: output: 0 [ ]
>>> gpio at a00010006: output: 1 [ ]
>>> gpio at a00010007: output: 0 [ ]
>>> ZynqMP> gpio toggle gpio at a00010007
>>> gpio: pin gpio at a00010007 (gpio 194) value is 1
>>> ZynqMP> gpio status -a gpio at a0001000
>>> Bank gpio at a0001000:
>>> gpio at a00010000: output: 1 [ ]
>>> gpio at a00010001: output: 0 [ ]
>>> gpio at a00010002: output: 0 [ ]
>>> gpio at a00010003: output: 0 [ ]
>>> gpio at a00010004: output: 0 [ ]
>>> gpio at a00010005: output: 0 [ ]
>>> gpio at a00010006: output: 1 [ ]
>>> gpio at a00010007: output: 1 [ ]
>>>
>>> And for this case there is no need to take that value from any saved
>>> location.
>>> It means having
>>> val = readl(&platdata->regs->gpiodata + bank * 2);
>>> is enough based on my test.
>>> And for the rest as your example above this should be also fine.
>>
>> Yes. If your observations are correct it is impossible to read the
>> input state if the GPIO is configured as output and if my observations
>> are correct the read will return the input state independently of the
>> mode.
>>
>>> When we have agreement on this we can look at xilinx_gpio_set_value.
>>
>> The direct read is fine for me.
>>
>> Can you read the two switch states if you run the following commands:
>>
>> /* Enable LED */
>> ZynqMP> gpio set gpio at a00010000
>> /* Read switch state and keep LED on */
>> ZynqMP> gpio input gpio at a00010000
>> ZynqMP> gpio status -a gpio at a0001000
>> /* Manually change switch state and read again */
>> ZynqMP> gpio status -a gpio at a0001000
>> /* Disable LED */
>> ZynqMP> gpio clear gpio at a00010000
>> ZynqMP> gpio status -a gpio at a0001000
>> /* Manually change switch state and read again */
>> ZynqMP> gpio status -a gpio at a0001000
>>
>> Best regards
>> Stefan
>>
>
> I have test the GPIO controller by read and write the register via the
> md and mw command. The read *always* returns the status of the gpio_io_i
> signal independent of the gpio_io_t status. This means to support my
> version of the IP the driver needs to save the output value in a
> variable. This solution should work for your and my hardware. Could you
> repost your patch to add the dout-default and output_val or should I
> post a patch based on your patch?
Interesting. Anyway I have sent v2 I hope will all things we have discussed.
Thanks,
Michal
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-08-02 12:56 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-07-23 11:43 [U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function() Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 2/4] gpio: xilinx: Swap xilinx_gpio_get_function with xilinx_gpio_get_value Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs Michal Simek
2018-07-23 18:42 ` Stefan Herbrechtsmeier
2018-07-24 10:31 ` Michal Simek
2018-07-24 19:56 ` Stefan Herbrechtsmeier
2018-07-25 6:39 ` Michal Simek
2018-07-25 18:21 ` Stefan Herbrechtsmeier
2018-07-26 8:41 ` Michal Simek
2018-07-26 19:46 ` Stefan Herbrechtsmeier
2018-07-27 7:05 ` Michal Simek
2018-07-27 8:41 ` Stefan Herbrechtsmeier
2018-07-30 12:40 ` Michal Simek
2018-07-30 13:32 ` Stefan Herbrechtsmeier
2018-07-30 14:10 ` Michal Simek
2018-07-30 19:34 ` Stefan Herbrechtsmeier
2018-08-01 18:36 ` Stefan Herbrechtsmeier
2018-08-02 12:56 ` Michal Simek
2018-07-23 11:43 ` [U-Boot] [PATCH 4/4] gpio: xilinx: Remove !DM driver Michal Simek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.