From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michal Simek <michal.simek@xilinx.com>
Date: Tue, 24 Jul 2018 12:31:55 +0200
Subject: [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via
	regs
In-Reply-To: <f02731e4-7e94-09fa-ef55-1779446930d2@herbrechtsmeier.net>
References: <61f5486be81d3a2e9170aa7c383de68b78b944cb.1532346215.git.michal.simek@xilinx.com>
 <c0bbc06f6f2033844a73a78bc994c14fa965048c.1532346215.git.michal.simek@xilinx.com>
 <f02731e4-7e94-09fa-ef55-1779446930d2@herbrechtsmeier.net>
Message-ID: <36e1b314-74a5-04bc-8563-629c5ebeda22@xilinx.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit
To: u-boot@lists.denx.de

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