Incorrect GPIO register used in t18x/drivers/gpio/gpio-tegra186.c?

rong1129 · June 15, 2017, 4:14am

We have configured a couple of pins, specifically CAM0/1_RST, as GPIO inputs. Pinmux and GPIO were also configured in DT. During boot, we could see the right message printed out:

[ 1.115547] GPIO line 457 (usr_control_in) hogged as input

We then did the following

echo 457 > /sys/class/gpio/export

cat /sys/class/gpio/gpio457/direction

out

Even we forced the direction to be “in”, the pin direction was still printed as “out”

echo in > /sys/class/gpio/gpio457/direction

cat /sys/class/gpio/gpio457/direction

out

Looking at the implementation in t18x/drivers/gpio/gpio-tegra186.c, it appears when reading the pin direction, incorrect register GPIO_OUT_CTRL_REG was used. In the set function set_gpio_direction_mode() bit 1 (GPIO_INOUT_BIT) of GPIO_ENB_CONFIG_REG was used for setting the pin direction:

static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
{
...
       val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
...
}

static void set_gpio_direction_mode(struct gpio_chip *chip, u32 offset,
                                    bool mode)
{
...
        val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
        if (mode)
                val |= GPIO_INOUT_BIT;
        else
                val &= ~GPIO_INOUT_BIT;
        tegra_gpio_writel(tgi, val, offset, GPIO_ENB_CONFIG_REG);
...
}

Since we don’t have the access to Tegra186 TRM, we are not entirely sure it’s indeed an error or intended behavior. Could you please confirm? Thanks.

JerryChang · June 19, 2017, 6:21am

hello rong1129,

thanks for point out, we’ll investigate it and get back to you soon.

JerryChang · June 19, 2017, 8:20am

hello rong1129,

had you got correct pin direction as following modification?

diff --git a/drivers/gpio/gpio-tegra186.c b/drivers/gpio/gpio-tegra186.c
index 43813e9..a0df4b0 100644
--- a/drivers/gpio/gpio-tegra186.c
+++ b/drivers/gpio/gpio-tegra186.c
@@ -671,7 +671,7 @@ static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
        if (!gpio_is_accessible(tgi, offset))
                return 0;

-       val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
+       val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);

rong1129 · June 19, 2017, 11:56pm

Not really. The pin direction bit is bit 1 of GPIO_ENB_CONFIG_REG. I have attached a proper fix below for reference, which also includes a couple of other minor fixes for the same file.

Again, it’s good to check what GPIO_OUT_CTRL_REG actually does. In tegra_gpio_set(), it’s always set to 0. My guess is it probably enables/disables the output buffer, but the pin direction bit in GPIO_ENB_CONFIG_REG still takes the precedence for pin direction control. If you have access to the Tegra186 TRM, please double check. Thanks.

--- a/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c
+++ b/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c
@@ -387,7 +387,7 @@ static int tegra186_gpio_to_wake(struct tegra_gpio_info *tgi, int gpio)
                if (tgi->soc->wake_table[i] == gpio) {
                        pr_info("gpio %s wake%d for gpio=%d(%s:%d)\n",
                                tgi->soc->name, i, gpio,
-                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 7);
+                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 8);
                        return i;
                }
        }
@@ -565,11 +565,11 @@ static int tegra_gpio_is_enabled(struct gpio_chip *chip, unsigned offset)
        u32 val;

        if (!gpio_is_accessible(tgi, offset))
-               return 0;
+               return -EBUSY;

        val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);

-       return !!(val & 0x1);
+       return !!(val & GPIO_ENB_BIT);
 }

 static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
@@ -578,11 +578,11 @@ static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
        u32 val;

        if (!gpio_is_accessible(tgi, offset))
-               return 0;
+               return -EBUSY;

-       val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
+       val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);

-       return (val & 0x1);
+       return !(val & GPIO_INOUT_BIT);
 }

snarky · June 20, 2017, 4:37am

FWIW, using modulo for bit masks is not a great habit, for a few reasons:

Negative input values may return negative output values in modulo (this varies by architecture)
Zeros may generate division exceptions (you may think you’re using a constant today, but habits form)
Dividers usually have much higher latency than bit masks

So, instead of this:

-                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 7);
+                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 8);

I argue that the more robust code is this:

-                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 7);
+                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio & 7);

Does it matter in this particular case? Unlikely. But habits are important!

rong1129 · July 3, 2017, 9:28pm

I spotted likely another error in the same file (debounce bit setting in tegra_gpio_set_debounce()), so I consolidated the fixes below in addition to snarky’s suggestion:

diff --git a/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c b/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c
index f4a714555..2002e4cee 100644
--- a/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c
+++ b/Linux_for_Tegra/sources/kernel/t18x/drivers/gpio/gpio-tegra186.c
@@ -387,7 +387,7 @@ static int tegra186_gpio_to_wake(struct tegra_gpio_info *tgi, int gpio)
                if (tgi->soc->wake_table[i] == gpio) {
                        pr_info("gpio %s wake%d for gpio=%d(%s:%d)\n",
                                tgi->soc->name, i, gpio,
-                               tgi->soc->port[GPIO_PORT(gpio)].port_name, gpio % 7);
+                               tgi->soc->port[GPIO_PORT(gpio)].port_name, GPIO_PIN(gpio));
                        return i;
                }
        }
@@ -454,12 +454,12 @@ static inline bool gpio_is_accessible(struct tegra_gpio_info *tgi, u32 offset)

 static void tegra_gpio_enable(struct tegra_gpio_info *tgi, int gpio)
 {
-       tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
+       tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, GPIO_ENB_BIT, GPIO_ENB_BIT);
 }

 static void tegra_gpio_disable(struct tegra_gpio_info *tgi, int gpio)
 {
-       tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, 0x1, 0x0);
+       tegra_gpio_update(tgi, gpio, GPIO_ENB_CONFIG_REG, GPIO_ENB_BIT, 0x0);
 }

 static int tegra_gpio_request(struct gpio_chip *chip, unsigned offset)
@@ -505,14 +505,8 @@ static void set_gpio_direction_mode(struct gpio_chip *chip, u32 offset,
                                    bool mode)
 {
        struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
-       u32 val;

-       val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);
-       if (mode)
-               val |= GPIO_INOUT_BIT;
-       else
-               val &= ~GPIO_INOUT_BIT;
-       tegra_gpio_writel(tgi, val, offset, GPIO_ENB_CONFIG_REG);
+       tegra_gpio_update(tgi, offset, GPIO_ENB_CONFIG_REG, GPIO_INOUT_BIT, mode ? GPIO_INOUT_BIT : 0);
 }

 static int tegra_gpio_direction_input(struct gpio_chip *chip, unsigned offset)
@@ -551,8 +545,7 @@ static int tegra_gpio_set_debounce(struct gpio_chip *chip, unsigned offset,
        struct tegra_gpio_info *tgi = gpiochip_get_data(chip);
        unsigned dbc_ms = DIV_ROUND_UP(debounce, 1000);

-       tegra_gpio_update(tgi, offset, GPIO_ENB_CONFIG_REG, 0x1, 0x1);
-       tegra_gpio_update(tgi, offset, GPIO_DEB_FUNC_BIT, 0x5, 0x1);
+       tegra_gpio_update(tgi, offset, GPIO_ENB_CONFIG_REG, GPIO_ENB_BIT | GPIO_DEB_FUNC_BIT, GPIO_ENB_BIT);

        /* Update debounce threshold */
        tegra_gpio_writel(tgi, dbc_ms, offset, GPIO_DBC_THRES_REG);
@@ -565,11 +558,11 @@ static int tegra_gpio_is_enabled(struct gpio_chip *chip, unsigned offset)
        u32 val;

        if (!gpio_is_accessible(tgi, offset))
-               return 0;
+               return -EBUSY;

        val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);

-       return !!(val & 0x1);
+       return !!(val & GPIO_ENB_BIT);
 }

 static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
@@ -578,11 +571,11 @@ static int tegra_gpio_get_direction(struct gpio_chip *chip, unsigned offset)
        u32 val;

        if (!gpio_is_accessible(tgi, offset))
-               return 0;
+               return -EBUSY;

-       val = tegra_gpio_readl(tgi, offset, GPIO_OUT_CTRL_REG);
+       val = tegra_gpio_readl(tgi, offset, GPIO_ENB_CONFIG_REG);

-       return (val & 0x1);
+       return !(val & GPIO_INOUT_BIT);
 }

 static int tegra_gpio_to_irq(struct gpio_chip *chip, unsigned offset)

JerryChang · July 4, 2017, 3:16am

hi all,

thanks for point out the mistake, we had correct the gpio pin direction function and the changes has already check-in.
we’ll review the tegra_gpio_set_debounce() and let you know the feedback.