[U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs
Michal Simek
michal.simek at xilinx.com
Mon Jul 30 12:40:22 UTC 2018
On 27.7.2018 10:41, Stefan Herbrechtsmeier wrote:
> 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.
yes.
>
>>>> 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.
yes.
>
>>>>>> 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.
No problem with this change at all.
>
>>>>>> 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.
please look below.
>
>>> 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.
please look below.
>
>>>> 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.
I am looking for IP owner but as far as I see it was created 9 years ago
that's why it will take some time.
Anyway I have created hw design on zcu102 which has output on led and
input on dip and output value is kept in data reg that's why your
snippet above can be used. Documentation is wrong in this
"For each I/O bit programmed as output:
• R: Reads to these bits always return zeros"
I have sent 4 patches which we discussed that's why please reviewed them.
I have one more patch in queue which is keeping output_val for certain
bank but not sure if make sense to apply it because as above when
direction is setup to output you can read desired output value from
gpiodata reg. I have tested it on gpio output only led case and gpiodata
reg contain correct value.
Anyway if I miss any patch in connection to this please send a patch and
let's look at that use case.
Thanks,
Michal
More information about the U-Boot
mailing list