From mboxrd@z Thu Jan 1 00:00:00 1970 From: Michal Simek Date: Wed, 25 Jul 2018 08:39:06 +0200 Subject: [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs In-Reply-To: <76009864-c697-e216-a7ad-2ad8f728c0c1@herbrechtsmeier.net> References: <61f5486be81d3a2e9170aa7c383de68b78b944cb.1532346215.git.michal.simek@xilinx.com> <36e1b314-74a5-04bc-8563-629c5ebeda22@xilinx.com> <76009864-c697-e216-a7ad-2ad8f728c0c1@herbrechtsmeier.net> Message-ID: <7891cd4a-3c00-720b-3e0d-539630b4b119@xilinx.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>> Signed-off-by: Michal Simek >>>> --- >>>> >>>>    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