[U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
Stefan Herbrechtsmeier
stefan at herbrechtsmeier.net
Fri Jul 27 08:41:43 UTC 2018
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 at herbrechtsmeier.net>
>>>>>>>>> Signed-off-by: Michal Simek <michal.simek at 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
More information about the U-Boot
mailing list