From mboxrd@z Thu Jan 1 00:00:00 1970 From: Stefan Herbrechtsmeier Date: Fri, 27 Jul 2018 10:41:43 +0200 Subject: [U-Boot] [PATCH 3/4] gpio: xilinx: Not read output values via regs In-Reply-To: References: <61f5486be81d3a2e9170aa7c383de68b78b944cb.1532346215.git.michal.simek@xilinx.com> <36e1b314-74a5-04bc-8563-629c5ebeda22@xilinx.com> <76009864-c697-e216-a7ad-2ad8f728c0c1@herbrechtsmeier.net> <7891cd4a-3c00-720b-3e0d-539630b4b119@xilinx.com> <6d7886e9-289f-3048-f3f5-75db2f9b4d0a@herbrechtsmeier.net> <050995d0-2ae3-20b7-0a99-c63e3a7fa031@xilinx.com> <17f7640c-001a-8680-8cf7-ec98bf116c9f@herbrechtsmeier.net> Message-ID: <9c2933bf-190c-e471-a51e-106af88740d5@herbrechtsmeier.net> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 8bit To: u-boot@lists.denx.de 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 >>>>>>>>> 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. >>>> 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