[U-Boot] [PATCH 1/4] gpio: xilinx: Find out bank before use in xilinx_gpio_get_function()
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));
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 8ce08d80f491..776a147cc8d7 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]; @@ -607,4 +274,3 @@ U_BOOT_DRIVER(xilinx_gpio) = { .probe = xilinx_gpio_probe, .platdata_auto_alloc_size = sizeof(struct xilinx_gpio_platdata), }; -#endif
Set a value before changing gpio direction. This will ensure that the old value is not propagated when direction has changed but new value is not written yet.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com ---
drivers/gpio/xilinx_gpio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 776a147cc8d7..1e5f3da8d7e8 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -139,14 +139,14 @@ static int xilinx_gpio_direction_output(struct udevice *dev, unsigned offset, if (platdata->bank_input[bank]) return -EINVAL;
+ xilinx_gpio_set_value(dev, offset, value); + if (!platdata->bank_output[bank]) { val = readl(&platdata->regs->gpiodir + bank * 2); val = val & ~(1 << pin); writel(val, &platdata->regs->gpiodir + bank * 2); }
- xilinx_gpio_set_value(dev, offset, value); - return 0; }
Am 30.07.2018 um 14:34 schrieb Michal Simek:
Set a value before changing gpio direction. This will ensure that the old value is not propagated when direction has changed but new value is not written yet.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/gpio/xilinx_gpio.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 776a147cc8d7..1e5f3da8d7e8 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -139,14 +139,14 @@ static int xilinx_gpio_direction_output(struct udevice *dev, unsigned offset, if (platdata->bank_input[bank]) return -EINVAL;
- xilinx_gpio_set_value(dev, offset, value);
- if (!platdata->bank_output[bank]) { val = readl(&platdata->regs->gpiodir + bank * 2); val = val & ~(1 << pin); writel(val, &platdata->regs->gpiodir + bank * 2); }
- xilinx_gpio_set_value(dev, offset, value);
- return 0; }
Reviewed-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net
There is no reason to do read/write for if/else separately.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com ---
drivers/gpio/xilinx_gpio.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 1e5f3da8d7e8..cccfa7561739 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -61,18 +61,17 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset, if (ret) return ret;
+ val = readl(&platdata->regs->gpiodata + bank * 2); + debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n", __func__, (ulong)platdata->regs, value, offset, bank, pin);
- if (value) { - val = readl(&platdata->regs->gpiodata + bank * 2); + if (value) val = val | (1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - } else { - val = readl(&platdata->regs->gpiodata + bank * 2); + else val = val & ~(1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - } + + writel(val, &platdata->regs->gpiodata + bank * 2);
return val; };
Hi Michal,
Am 30.07.2018 um 14:34 schrieb Michal Simek:
There is no reason to do read/write for if/else separately.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/gpio/xilinx_gpio.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 1e5f3da8d7e8..cccfa7561739 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -61,18 +61,17 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset, if (ret) return ret;
- val = readl(&platdata->regs->gpiodata + bank * 2);
- debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n", __func__, (ulong)platdata->regs, value, offset, bank, pin);
- if (value) {
val = readl(&platdata->regs->gpiodata + bank * 2);
- if (value) val = val | (1 << pin);
writel(val, &platdata->regs->gpiodata + bank * 2);- } else {
val = readl(&platdata->regs->gpiodata + bank * 2);
- else val = val & ~(1 << pin);
writel(val, &platdata->regs->gpiodata + bank * 2);- }
writel(val, &platdata->regs->gpiodata + bank * 2);
return val; };
This doesn't work because you read the general purpose input ports (register) and write to the general purpose output ports (register). This leads to problems if an output only pin inside an bidirectional port has no connection between the two registers or the input and output registers has different values.
The value of the output register must be saved inside the driver.
Best regards Stefan
On 30.7.2018 15:31, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 30.07.2018 um 14:34 schrieb Michal Simek:
There is no reason to do read/write for if/else separately.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/gpio/xilinx_gpio.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 1e5f3da8d7e8..cccfa7561739 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -61,18 +61,17 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset, if (ret) return ret; + val = readl(&platdata->regs->gpiodata + bank * 2);
debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n", __func__, (ulong)platdata->regs, value, offset, bank, pin); - if (value) { - val = readl(&platdata->regs->gpiodata + bank * 2); + if (value) val = val | (1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - } else { - val = readl(&platdata->regs->gpiodata + bank * 2); + else val = val & ~(1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - }
+ writel(val, &platdata->regs->gpiodata + bank * 2); return val; };
This doesn't work because you read the general purpose input ports (register) and write to the general purpose output ports (register). This leads to problems if an output only pin inside an bidirectional port has no connection between the two registers or the input and output registers has different values.
The value of the output register must be saved inside the driver.
This is only simplifying logic not doing changes which we are talking about in separate thread. Your comment is how to get that initial value.
Thanks, Michal
Am 30.07.2018 um 15:42 schrieb Michal Simek:
On 30.7.2018 15:31, Stefan Herbrechtsmeier wrote:
Hi Michal,
Am 30.07.2018 um 14:34 schrieb Michal Simek:
There is no reason to do read/write for if/else separately.
Reported-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net Signed-off-by: Michal Simek michal.simek@xilinx.com
drivers/gpio/xilinx_gpio.c | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-)
diff --git a/drivers/gpio/xilinx_gpio.c b/drivers/gpio/xilinx_gpio.c index 1e5f3da8d7e8..cccfa7561739 100644 --- a/drivers/gpio/xilinx_gpio.c +++ b/drivers/gpio/xilinx_gpio.c @@ -61,18 +61,17 @@ static int xilinx_gpio_set_value(struct udevice *dev, unsigned offset, if (ret) return ret; + val = readl(&platdata->regs->gpiodata + bank * 2);
debug("%s: regs: %lx, value: %x, gpio: %x, bank %x, pin %x\n", __func__, (ulong)platdata->regs, value, offset, bank, pin); - if (value) { - val = readl(&platdata->regs->gpiodata + bank * 2); + if (value) val = val | (1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - } else { - val = readl(&platdata->regs->gpiodata + bank * 2); + else val = val & ~(1 << pin); - writel(val, &platdata->regs->gpiodata + bank * 2); - }
+ writel(val, &platdata->regs->gpiodata + bank * 2); return val; };
This doesn't work because you read the general purpose input ports (register) and write to the general purpose output ports (register). This leads to problems if an output only pin inside an bidirectional port has no connection between the two registers or the input and output registers has different values.
The value of the output register must be saved inside the driver.
This is only simplifying logic not doing changes which we are talking about in separate thread. Your comment is how to get that initial value.
Okay. In this case:
Reviewed-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net
Am 30.07.2018 um 14:34 schrieb Michal Simek:
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));
Reviewed-by: Stefan Herbrechtsmeier stefan@herbrechtsmeier.net
participants (2)
-
Michal Simek -
Stefan Herbrechtsmeier