Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1631950 > unrolled thread

[PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller

Back to article view | Back to linux.kernel

Contents

  [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
    [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-27 10:40 +0200
        Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1  pinctrl header Simon Horman <horms@verge.net.au> - 2017-04-28 07:20 +0200
    [PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group Simon Horman <horms@verge.net.au> - 2017-04-28 07:30 +0200
    [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-27 12:00 +0200
        RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-27 12:50 +0200
          Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Simon Horman <horms@verge.net.au> - 2017-04-28 07:30 +0200
          Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-28 09:20 +0200
            RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 16:50 +0200
              Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Linus Walleij <linus.walleij@linaro.org> - 2017-05-05 14:10 +0200
                Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-05 14:30 +0200
                RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Chris Brandt <Chris.Brandt@renesas.com> - 2017-05-05 14:50 +0200
                  Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Linus Walleij <linus.walleij@linaro.org> - 2017-05-11 15:50 +0200
      Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Linus Walleij <linus.walleij@linaro.org> - 2017-04-28 11:00 +0200
        RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 16:00 +0200
    [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-27 17:00 +0200
        Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-04-28 10:40 +0200
          Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-28 12:20 +0200
            Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-05-07 23:50 +0200
          RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 14:10 +0200
            Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-28 14:20 +0200
              RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 15:20 +0200
                Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-28 17:00 +0200
                  RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 17:20 +0200
                    Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-28 17:40 +0200
                      RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-04-28 18:50 +0200
                  Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-05-08 01:30 +0200
                    Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-05-08 18:10 +0200
                      Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-08 18:20 +0200
                        RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-05-08 19:10 +0200
                          Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-08 20:30 +0200
                            RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable Chris Brandt <Chris.Brandt@renesas.com> - 2017-05-08 22:10 +0200
                        Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-05-08 19:30 +0200
                          Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-05-08 19:50 +0200
                            Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and  output-enable jmondi <jacopo@jmondi.org> - 2017-05-09 12:00 +0200
                          Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Linus Walleij <linus.walleij@linaro.org> - 2017-05-08 23:20 +0200
                            Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable Geert Uytterhoeven <geert@linux-m68k.org> - 2017-05-09 13:00 +0200
    [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-27 10:40 +0200
      Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties Linus Walleij <linus.walleij@linaro.org> - 2017-04-28 10:20 +0200
        Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-28 12:10 +0200
        Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack  properties jmondi <jacopo@jmondi.org> - 2017-04-28 14:50 +0200
        Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties Andy Shevchenko <andy.shevchenko@gmail.com> - 2017-04-28 18:30 +0200
    [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
      Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-27 10:40 +0200
      Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc Rob Herring <robh@kernel.org> - 2017-04-28 23:10 +0200
    [PATCH v5 06/10] arm: dts: r7s72100: Add pin controller node Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
    [PATCH v5 09/10] arm: dts: genmai: Add user led device nodes Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
    [PATCH v5 08/10] arm: dts: genmai: Add RIIC2 pin group Jacopo Mondi <jacopo+renesas@jmondi.org> - 2017-04-27 10:30 +0200
    Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-27 10:50 +0200
      Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller Simon Horman <horms@verge.net.au> - 2017-04-28 07:30 +0200
      Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller jmondi <jacopo@jmondi.org> - 2017-04-28 09:30 +0200
        Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller Simon Horman <horms@verge.net.au> - 2017-04-28 09:40 +0200
        Re: [PATCH v5 00/10] Renesas RZ/A1 pin and gpio controller Geert Uytterhoeven <geert@linux-m68k.org> - 2017-04-28 09:40 +0200

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

#1632636 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
>> Add bi-directional and output-enable pin configuration properties.
>>
>> bi-directional allows to specify when a pin shall operate in input and
>> output mode at the same time. This is particularly useful in platforms
>> where input and output buffers have to be manually enabled.
>>
>> output-enable is just syntactic sugar to specify that a pin shall
>> operate in output mode, ignoring the provided argument.
>> This pairs with input-enable pin configuration option.
>
> For me it looks like you are trying to alias open-drain + bias or
> alike. Don't actually see the benefit of it.

Andy is bringing up a valid point. And I remember asking about
this before.

What does "bi-directional" really mean, electrically speaking?

Does is just mean open drain and/or open source actually?
(See Documentation/gpio/driver.txt for an explanation of
how open drain/source works.)

When you set an output without setting a value, what happens
electrically?

Isn't this bias-high-impedance / High-Z?

Hopefully you can find the answer from Renesas hardware dept.

You can certainly call it whatever the datasheet calls it
in your driver #defines but for the DT bindings we would
ideally have the physical world things.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1632772 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Linus,

On Fri, Apr 28, 2017 at 10:32 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Apr 27, 2017 at 4:56 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Thu, Apr 27, 2017 at 11:19 AM, Jacopo Mondi
>> <jacopo+renesas@jmondi.org> wrote:
>>> Add bi-directional and output-enable pin configuration properties.
>>>
>>> bi-directional allows to specify when a pin shall operate in input and
>>> output mode at the same time. This is particularly useful in platforms
>>> where input and output buffers have to be manually enabled.
>>>
>>> output-enable is just syntactic sugar to specify that a pin shall
>>> operate in output mode, ignoring the provided argument.
>>> This pairs with input-enable pin configuration option.
>>
>> For me it looks like you are trying to alias open-drain + bias or
>> alike. Don't actually see the benefit of it.
>
> Andy is bringing up a valid point. And I remember asking about
> this before.
>
> What does "bi-directional" really mean, electrically speaking?
>
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of
> how open drain/source works.)
>
> When you set an output without setting a value, what happens
> electrically?
>
> Isn't this bias-high-impedance / High-Z?
>
> Hopefully you can find the answer from Renesas hardware dept.
>
> You can certainly call it whatever the datasheet calls it
> in your driver #defines but for the DT bindings we would
> ideally have the physical world things.

FWIW, you have already applied v4.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

[toc] | [prev] | [next] | [standalone]

#1637033 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 12:09 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:

>> You can certainly call it whatever the datasheet calls it
>> in your driver #defines but for the DT bindings we would
>> ideally have the physical world things.
>
> FWIW, you have already applied v4.

Yeah I noticed when sending pull requests.. let's see if it stays in or
I will have to revert it for more discussion.

Sorry for being so flimsy at times, I guess I'm overloaded.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1632828 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Linus Walleij wrote:
> > For me it looks like you are trying to alias open-drain + bias or
> > alike. Don't actually see the benefit of it.
> 
> Andy is bringing up a valid point. And I remember asking about this before.
> 
> What does "bi-directional" really mean, electrically speaking?
> 
> Does is just mean open drain and/or open source actually?
> (See Documentation/gpio/driver.txt for an explanation of how open
> drain/source works.)
> 
> When you set an output without setting a value, what happens electrically?
> 
> Isn't this bias-high-impedance / High-Z?
> 
> Hopefully you can find the answer from Renesas hardware dept.
> 
> You can certainly call it whatever the datasheet calls it in your driver
> #defines but for the DT bindings we would ideally have the physical world
> things.

The main reason is this pin controller is too dumb to do what it's supposed to with 1 register setting.


Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain). The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.
In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

# side note, the way the registers are arranged is also ridiculous in my opinion. I'm not a fan of this particular IP.

The good news is the RZ/A1 is the only chip series I've seen with this PFC IP (I can't even figure out where it came from internally). And as far as I know, it will not appear in any other RZ series chips.



Chris

[toc] | [prev] | [next] | [standalone]

#1632829 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 3:07 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Linus Walleij wrote:
>> > For me it looks like you are trying to alias open-drain + bias or
>> > alike. Don't actually see the benefit of it.
>>
>> Andy is bringing up a valid point. And I remember asking about this before.
>>
>> What does "bi-directional" really mean, electrically speaking?

> Take the SDHI data pins. You send AND receive data over those pins (and they are not open drain).

Can you point to schematics and electrical characteristics of such buffer?
(Yes, I can imagine one case where it's possible to have an
"automatic" switch based on which current is bigger output of your
side or remote's. But! I would like to see actual specifications to
prove this or otherwise.)

> The issue is that the PFC HW that enables the connections between the SDHI IP block and the I/O pad buffers can only enable one path/signal/direction to the buffer enables (in or out). So for a pin that needs both directions, the PFC enables output and the "bidirectional register" is used to enable the input buffer as well.

> In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control Logical Diagram (but that wasn't obvious to me at first).

Please, post a link to it or copy essential parts.
I'm quite skeptical  that cheap hardware can implement something more
costable than simplest open-source / open-drain + bias.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1632866 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
> Logical Diagram (but that wasn't obvious to me at first).
> 
> Please, post a link to it or copy essential parts.


This board the RZ/A1 GENMAI board.
https://www.renesas.com/en-us/products/software-tools/boards-and-kits/evaluation-demo-solution-boards/genmai-cpu-board-rtk772100bc00000br.html


The schematic is included in the "User's manual"
https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf


The RZ/A1H Hardware manual is here:
https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true&prdLayerId=186374&layerName=RZ%252FA1H&coronrService=document-prd-search&hwDocUrl=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf&hashKey=54f335753742b5add524d4725b7242e6


Chapter 54 is the port/pin controller.

"54.18 Port Control Logical Diagram" is the diagram I was talking about. Note that is says "Note: This figure shows the logic for reference, not the circuit."

"54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register needed to get some pins to work.


> I'm quite skeptical  that cheap hardware can implement something more
> costable than simplest open-source / open-drain + bias

I don't think this is an open-source / open-drain + bias issue. It's a "the internal signal paths are not getting hooked up correctly" issue.


Regardless, on this part, we needed a way to flag that some pins when put in some function modes needed 'an extra register setting'. At first we tried to sneak that info in with a simple #define in the pin/pinmux DT node properties. But, Linus didn't want it there so we had to make up a new generic property called "bi-directional".

What is your end goal here? Get "bi-directional" changed to something else?


Chris

[toc] | [prev] | [next] | [standalone]

#1632955 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 4:18 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> > In the RZ/A1 HW manual you can kind of see that in 54.18 Port Control
>> Logical Diagram (but that wasn't obvious to me at first).
>>
>> Please, post a link to it or copy essential parts.

> The schematic is included in the "User's manual"
> https://www.renesas.com/en-us/doc/products/tool/doc/003/r20ut2596ej_r7s72100evum.pdf

Not that one I would like to see...

> The RZ/A1H Hardware manual is here:
> https://www.renesas.com/en-us/document/hw-manual?hwLayerShowFlg=true&prdLayerId=186374&layerName=RZ%252FA1H&coronrService=document-prd-search&hwDocUrl=%2Fen-us%2Fdoc%2Fproducts%2Fmpumcu%2Fdoc%2Frz%2Fr01uh0403ej0300_rz_a1h.pdf&hashKey=54f335753742b5add524d4725b7242e6
>
> Chapter 54 is the port/pin controller.
>
> "54.18 Port Control Logical Diagram" is the diagram I was talking about. Note that is says "Note: This figure shows the logic for reference, not the circuit."
>
> "54.3.13 Port Bidirection Control Register (PBDCn)" is the magic register needed to get some pins to work.

This is useful. Thanks.

>> I'm quite skeptical  that cheap hardware can implement something more
>> costable than simplest open-source / open-drain + bias
>
> I don't think this is an open-source / open-drain + bias issue. It's a "the internal signal paths are not getting hooked up correctly" issue.

Had you read the following, esp. Note there?

* @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does not
*      affect the pin's ability to drive output.  1 enables input, 0 disables
*      input.

For me manual is clearly tells about this settings:
"This register enables or disables the input buffer while the output
buffer is enabled."

> Regardless, on this part, we needed a way to flag that some pins when put in some function modes needed 'an extra register setting'. At first we tried to sneak that info in with a simple #define in the pin/pinmux DT node properties. But, Linus didn't want it there so we had to make up a new generic property called "bi-directional".
>
> What is your end goal here? Get "bi-directional" changed to something else?

My goal is to reduce amount of (useless) entities. See Occam's razor
for details.

Linus, for me it looks like better to revert that change, until we
will have clear picture why existing configuration parameters can't
work.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1632966 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> Had you read the following, esp. Note there?
> 
> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
> not
> *      affect the pin's ability to drive output.  1 enables input, 0
> disables
> *      input.
> 
> For me manual is clearly tells about this settings:
> "This register enables or disables the input buffer while the output
> buffer is enabled."


But, then if we use "input-enable" to get bi-directional functionality, now we need something to replace what we were using "input-enable" for.
We were using "input-enable" to signal when the pin function that we set also needs to be forcible set to input by the software (once again, because the HW is not smart enough to do it on its own), but is different than the bi-directional functionality (ie, a different register setting).


So, if we replace "bi-directional" with "input-enable" (since logically internally that is what is going on), what do we use for the special pins that the HW manual says "hey, you need to manually set these pins to input with SW because the pin selection HW can't do it correctly)". Note that we added a enable-output for the same reason.
See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that PIPCn.PIPCnm Bit Should be Set to 0"


Chris

[toc] | [prev] | [next] | [standalone]

#1632974 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 6:16 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, April 28, 2017, Andy Shevchenko wrote:
>> Had you read the following, esp. Note there?
>>
>> * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input.  Note that this does
>> not
>> *      affect the pin's ability to drive output.  1 enables input, 0
>> disables
>> *      input.
>>
>> For me manual is clearly tells about this settings:
>> "This register enables or disables the input buffer while the output
>> buffer is enabled."
>
>
> But, then if we use "input-enable" to get bi-directional functionality,

It seems you are missing the point from electrical prospective.
Standard pin buffers (electrically) means input buffer and output
buffer that are driven independently in most cases.

Here is one example:
https://electronics.stackexchange.com/questions/96932/internal-circuitry-of-io-ports-in-mcu/96953#96953

(That's what I asked before as a schematic)

> now we need something to replace what we were using "input-enable" for.

No.

> We were using "input-enable" to signal when the pin function that we set also needs to be forcible set to input by the software (once again, because the HW is not smart enough to do it on its own), but is different than the bi-directional functionality (ie, a different register setting).

You are trying to introduce an abstraction, called BiDi, which is
*not* a separate thing from a set of pin properties.

> So, if we replace "bi-directional" with "input-enable" (since logically internally that is what is going on), what do we use for the special pins that the HW manual says "hey, you need to manually set these pins to input with SW because the pin selection HW can't do it correctly)".

Yes.
BiDi is an alias to output + input enable + other pin configuration
parameters (a set depends on real HW needs).

> Note that we added a enable-output for the same reason.
> See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that PIPCn.PIPCnm Bit Should be Set to 0"

Perhaps needs to be revisited as well.

P.S. It looks like more and more software engineers are going far from
real hardware when developing drivers...

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1633022 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Friday, April 28, 2017, Andy Shevchenko wrote:
> > We were using "input-enable" to signal when the pin function that we set
> also needs to be forcible set to input by the software (once again,
> because the HW is not smart enough to do it on its own), but is different
> than the bi-directional functionality (ie, a different register setting).
> 
> You are trying to introduce an abstraction, called BiDi, which is
> *not* a separate thing from a set of pin properties.

Note, I'm talking about 2 different issues we had:

1) Pins that need input and output buffers enabled during normal use. We created "bi-directional" for that.

2) For whatever reason, the HW manual points out that the PFC hardware can't really automatically set buffers enables correctly for some pin instances, and we have to manually assign the pin as input or output using another register. For that, we were using "input-enable" and "output-enable".


For #2:

> > Note that we added a enable-output for the same reason.
> > See RZ/A1H HW Manual section "Table 54.7 Alternative Functions that
> PIPCn.PIPCnm Bit Should be Set to 0"
> 
> Perhaps needs to be revisited as well.

Sorry, we didn't 'add' anything new. The property "output-enable", (ie, PIN_CONFIG_OUTPUT) already existed and describes what we are doing in the case for output.

But, we still have the issue that we have 2 cases that need the input enabled, but they are not the same situation, so we can't just use "input-enable" for both.


My only suggestion is (and I'm not sure this is possible in the driver):

"input-enable"  : case #2 where you need the pin to be forced as an input
"output-enable" : case #2 where you need the pin to be forced as an output
"input-enable" + "output-enable" : case #1 (replaces "bi-directional").

For example:

	i2c2_pins: i2c2 {
		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
		input-enable;
		output-enable;
	};

So in the SW driver, if we see both, that will signal to us what is going on and what to do about it (as in, set the bi-directional register and not the input direction register).


Thoughts?


Chris

[toc] | [prev] | [next] | [standalone]

#1637148 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:

> Linus, for me it looks like better to revert that change, until we
> will have clear picture why existing configuration parameters can't
> work.

Yeah I'll revert the binding for fixes.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1637540 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Linus,

On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>
> > Linus, for me it looks like better to revert that change, until we
> > will have clear picture why existing configuration parameters can't
> > work.
>
> Yeah I'll revert the binding for fixes.
>

As it seems we won't be able to proceed with the currently proposed solution,
would that be acceptable now that we use the "pinmux" property to add
flags as BIDIR and SWIO_[INPUT|OUTPUT] directly there?
This was my original proposal, rejected because we were using the "pins"
property at the time.

Quoting to the description of "pinmux":

"Each individual pin controller driver bindings documentation shall
specify how those values (pin IDs and pin multiplexing configuration)
are defined and assembled together"

Do you think the "flags" we have failed to describe as generic pin
configuration properties, fit the definition of "pin multiplexing
configuration" to be assembled with pin IDs?

As a reference this was the proposed bindings in v3:
https://www.spinics.net/lists/linux-renesas-soc/msg12792.html

Have a look at Pin multiplexing sub-nodes examples 2 and 3, with
"pinmux" in place of "renesas,pins" property.

Thanks
   j

> Yours,
> Linus Walleij

[toc] | [prev] | [next] | [standalone]

#1637551 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> <andy.shevchenko@gmail.com> wrote:
>>
>> > Linus, for me it looks like better to revert that change, until we
>> > will have clear picture why existing configuration parameters can't
>> > work.
>>
>> Yeah I'll revert the binding for fixes.

> As it seems we won't be able to proceed with the currently proposed solution,
> would that be acceptable now that we use the "pinmux" property to add
> flags as BIDIR

Can you explain what does this *electrically* mean?
Second question, what makes it differ to what already exists?

>  and SWIO_[INPUT|OUTPUT] directly there?

Ditto.

> This was my original proposal, rejected because we were using the "pins"
> property at the time.


-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1637596 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Monday, May 08, 2017, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >>
> >> > Linus, for me it looks like better to revert that change, until we
> >> > will have clear picture why existing configuration parameters can't
> >> > work.
> >>
> >> Yeah I'll revert the binding for fixes.
> 
> > As it seems we won't be able to proceed with the currently proposed
> solution,
> > would that be acceptable now that we use the "pinmux" property to add
> > flags as BIDIR
> 
> Can you explain what does this *electrically* mean?

Bi-Directional:

For any pin that needs to drive (send) or sense (receive) signals, the pin
mux controller can only enable 1 direction of buffers (in this case, it
only the output buffers). Therefore the appropriate bit in the
'bi-directional' register is set in order to enable the signal path in both
directions (ie, enable the input buffers).


> >  and SWIO_[INPUT|OUTPUT] directly there?

In the hardware manual, there is a list of pin functions that if you want
to use, you cannot use the stand pinmux register method that you use for
all the other pins. Instead, you are instructed to do a different
procedure. If course electrically, input/output buffers are still turned
on/off or whatever, but the root reason of why you need to do this
differently for these specific pin/function is not known.

The "SWIO_" portion of the naming comes from the hardware manual which
refers to this as "Software I/O Control Alternative Mode" (which in my
opinion means the HW guys couldn't get the pin directions/buffers to be set
automatically for some reason, so they decided it's the SW guys problem now
for those pins)


> Second question, what makes it differ to what already exists?

We have 3 different flags (properties) that need to be specified for some
pins in some circumstances.
At first, we just tried to pass this additional information in when
defining what function the pin should be just for those pins whose
direction (ie, buffers) would not be set up automatically.
However, this was rejected and we were told to pick from the existing set
generic properties.
But, 3 different generic pinctrl properties that fit what we needed didn't
exist. So, we used the existing "input-enable" and "output-enable", but
then created "bi-directional".


Chris

[toc] | [prev] | [next] | [standalone]

#1637630 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Mon, May 8, 2017 at 8:02 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Monday, May 08, 2017, Andy Shevchenko wrote:
>> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
>> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> wrote:
>> >>
>> >> > Linus, for me it looks like better to revert that change, until we
>> >> > will have clear picture why existing configuration parameters can't
>> >> > work.
>> >>
>> >> Yeah I'll revert the binding for fixes.
>>
>> > As it seems we won't be able to proceed with the currently proposed
>> solution,
>> > would that be acceptable now that we use the "pinmux" property to add
>> > flags as BIDIR
>>
>> Can you explain what does this *electrically* mean?
>
> Bi-Directional:
>
> For any pin that needs to drive (send) or sense (receive) signals, the pin
> mux controller can only enable 1 direction of buffers (in this case, it
> only the output buffers). Therefore the appropriate bit in the
> 'bi-directional' register is set in order to enable the signal path in both
> directions (ie, enable the input buffers).

So, I see this way how it can be enabled:
1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set
2. BiDi bit is set and BUFOE is set

Now the question is what the real use case for 2?

If we find one we need to rename and fix a description of the pin
control configuration property.

like:
@PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.
...
Note: As long as it's enabled the pin's input enabled as well and vice versa.

>> >  and SWIO_[INPUT|OUTPUT] directly there?
>
> In the hardware manual, there is a list of pin functions that if you want
> to use, you cannot use the stand pinmux register method that you use for
> all the other pins. Instead, you are instructed to do a different
> procedure. If course electrically, input/output buffers are still turned
> on/off or whatever, but the root reason of why you need to do this
> differently for these specific pin/function is not known.
>
> The "SWIO_" portion of the naming comes from the hardware manual which
> refers to this as "Software I/O Control Alternative Mode" (which in my
> opinion means the HW guys couldn't get the pin directions/buffers to be set
> automatically for some reason, so they decided it's the SW guys problem now
> for those pins)

Okay, these are related to pin muxing explicitly.
So, you have 10 functions overall?
What prevents you describe them accordingly and hide this
implementation detail in the driver?

>> Second question, what makes it differ to what already exists?
>
> We have 3 different flags (properties) that need to be specified for some
> pins in some circumstances.
> At first, we just tried to pass this additional information in when
> defining what function the pin should be just for those pins whose
> direction (ie, buffers) would not be set up automatically.
> However, this was rejected and we were told to pick from the existing set
> generic properties.
> But, 3 different generic pinctrl properties that fit what we needed didn't
> exist. So, we used the existing "input-enable" and "output-enable", but
> then created "bi-directional".

Yes, that figure helped me a lot to understand.

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1637691 — RE: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hello Andy,

On Monday, May 08, 2017, Andy Shevchenko wrote:
> > Bi-Directional:
> >
> > For any pin that needs to drive (send) or sense (receive) signals, the
> pin
> > mux controller can only enable 1 direction of buffers (in this case, it
> > only the output buffers). Therefore the appropriate bit in the
> > 'bi-directional' register is set in order to enable the signal path in
> both
> > directions (ie, enable the input buffers).
> 
> So, I see this way how it can be enabled:
> 1. IP_ENI + IP_ENO internally defines BiDi when PMC and PIPC bits are set
> 2. BiDi bit is set and BUFOE is set
> 
> Now the question is what the real use case for 2?

For #1, IP_ENI and IP_ENO are controlled by PFC/PFCE/PFCAE.
Those basically equate to the pin function register (as in,
what IP block gets wired up to each pin.) However, it
seems that PFC/PFCE/PFCAE cannot enable both IP_ENI and
IP_ENO signals at the same time (the diagram doesn't really
show you that piece of info), hence they had to make an
'override' register can called it PBDC (bir-dir register)
to manually turn the input buffers on when needed.

Seems like a HW hack if you ask me.


> If we find one we need to rename and fix a description of the pin
> control configuration property.
> 
> like:
> @PIN_CONFIG_OUTPUT_INPUT_ENABLE: this will configure the pin as an output.
> ...
> Note: As long as it's enabled the pin's input enabled as well and vice
> versa.

So your suggestion is to rename PIN_CONFIG_OUTPUT to
PIN_CONFIG_OUTPUT_INPUT_ENABLE?

I would say the description for @PIN_CONFIG_INPUT_ENABLE is probably
'technically' correct for our bi-dir needs, but I didn't like it
because it might confuse users making a DT file for their board (unless
of course they studied the hardware manual in detail to finally come
to the conclusion of the screwy PFC hardware).


> >> >  and SWIO_[INPUT|OUTPUT] directly there?
> >
> > In the hardware manual, there is a list of pin functions that if you
> want
> > to use, you cannot use the stand pinmux register method that you use for
> > all the other pins. Instead, you are instructed to do a different
> > procedure. If course electrically, input/output buffers are still turned
> > on/off or whatever, but the root reason of why you need to do this
> > differently for these specific pin/function is not known.
> >
> > The "SWIO_" portion of the naming comes from the hardware manual which
> > refers to this as "Software I/O Control Alternative Mode" (which in my
> > opinion means the HW guys couldn't get the pin directions/buffers to be
> set
> > automatically for some reason, so they decided it's the SW guys problem
> now
> > for those pins)
> 
> Okay, these are related to pin muxing explicitly.
> So, you have 10 functions overall?
> What prevents you describe them accordingly and hide this
> implementation detail in the driver?

The one issue I was trying to avoid by hiding things in the driver
with some type of look-up table was that this series of parts comes
in different subsets/packages and sometimes the functions comes out
on different port numbers. So now you need multiple look-up tables
and then also a way to signal what part/package you have so you use
the correct look-up table. It seemed easier (and safer) to just pass
the info in (if you needed it) in the Device Tree for the board.

Of these 'special' pins (Table 54.7):

For 16 of them, they truly can operate as input or output, so
the user needs to specify that direction they need in the DT.

For LVDS, Sound and WDT, those will be fixed, so they would be
the only ones you could do a table for, but as I mentioned, Sound
and WDT don't always come out in the same place so a lookup table
isn't so cut and dry.


> >> Second question, what makes it differ to what already exists?
> >
> > We have 3 different flags (properties) that need to be specified for
> some
> > pins in some circumstances.
> > At first, we just tried to pass this additional information in when
> > defining what function the pin should be just for those pins whose
> > direction (ie, buffers) would not be set up automatically.
> > However, this was rejected and we were told to pick from the existing
> set
> > generic properties.
> > But, 3 different generic pinctrl properties that fit what we needed
> didn't
> > exist. So, we used the existing "input-enable" and "output-enable", but
> > then created "bi-directional".
> 
> Yes, that figure helped me a lot to understand.

I see from your other email you sent to Jacopo, it looks like the link
I sent you only brought you to the shorter 'data sheet' version of the
hardware manual, not the full manual that includes 'Figure 54.1'.
Sorry about that.



Chris

[toc] | [prev] | [next] | [standalone]

#1637606 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Andy,

On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> <andy.shevchenko@gmail.com> wrote:
> >>
> >> > Linus, for me it looks like better to revert that change, until we
> >> > will have clear picture why existing configuration parameters can't
> >> > work.
> >>
> >> Yeah I'll revert the binding for fixes.
>
> > As it seems we won't be able to proceed with the currently proposed solution,
> > would that be acceptable now that we use the "pinmux" property to add
> > flags as BIDIR
>
> Can you explain what does this *electrically* mean?

I really don't know what to add to what Chris said in his 2 previous
replies to the same question, and I don't know if I can even get this
information as the most detailed drawing I can provide is what you
have seen already at page 2696 Fig. 54.1 of the following document.

https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c

From my perspective these flags are configurations internal to the pin
controller hardware used to enable/disable input buffers when a pin needs to
perform in both direction.
The level of detail I can provide on this is the logical diagram we have pointed
you to already.

As I assume you are trying to get this answer from us in order to
avoid duplicating things in pin controller sub-system, and I
understand this, but my question here was "can we have those flags as part
of the pinmux property argument list, as that property description
seems to allow us to do that, instead of making them generic pin
configuration properties and upset other developers?"

Anyway, I still fail to see why those configuration flags, only
affecting the way the pin controller hardware enables/disables
its internal buffers and its internal operations have to be
described in term of their externally visible electrically characteristics.

> Second question, what makes it differ to what already exists?

To me, what already exists are pin configuration properties generic to
the whole pin controller subsystem, and I understand you don't want to
see duplication there.

At the same time, to me, those flags are settings the pin controller
wants to have specified by software to overcome its hw design flaws,
and are intended to configure its internal buffers in a way it cannot
do by itself for some very specific operation modes (they are listed
in the hw reference manual, it's not something you can chose to
configure or not, if you want a pin working in i2c mode, you HAVE to
pass those flags to pin controller).

Thanks
   j

Edit: I see Chris have now replied as well so I leave the SWIO part
out, as his answer is already better than what I can give you.

>
> >  and SWIO_[INPUT|OUTPUT] directly there?
>
> Ditto.
>
> > This was my original proposal, rejected because we were using the "pins"
> > property at the time.
>
>
> --
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1637621 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Mon, May 8, 2017 at 8:25 PM, jmondi <jacopo@jmondi.org> wrote:
> Andy,
>
> On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
>> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
>> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
>> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
>> >> <andy.shevchenko@gmail.com> wrote:
>> >>
>> >> > Linus, for me it looks like better to revert that change, until we
>> >> > will have clear picture why existing configuration parameters can't
>> >> > work.
>> >>
>> >> Yeah I'll revert the binding for fixes.
>>
>> > As it seems we won't be able to proceed with the currently proposed solution,
>> > would that be acceptable now that we use the "pinmux" property to add
>> > flags as BIDIR
>>
>> Can you explain what does this *electrically* mean?
>
> I really don't know what to add to what Chris said in his 2 previous
> replies to the same question, and I don't know if I can even get this
> information as the most detailed drawing I can provide is what you
> have seen already at page 2696 Fig. 54.1 of the following document.
>
> https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c

I didn't see before this document. (I had downloaded what Chris
referred to, which has less than 1200 pages).

The figure you pointed to is really nice and explains it, thank you.

So, BiDi in this hardware is just helper to enable Input
simultaneously when you enable output.

This makes me wonder what prevents you to do this in two steps in software?
So, basically in terms of pin control framework you define this pin
configuration as

1. PIN_CONFIG_INPUT_ENABLE:
2. PIN_CONFIG_OUTPUT:

(or wise versa)

> From my perspective these flags are configurations internal to the pin
> controller hardware used to enable/disable input buffers when a pin needs to
> perform in both direction.

> The level of detail I can provide on this is the logical diagram we have pointed
> you to already.
>
> As I assume you are trying to get this answer from us in order to
> avoid duplicating things in pin controller sub-system, and I
> understand this, but my question here was "can we have those flags as part
> of the pinmux property argument list, as that property description
> seems to allow us to do that, instead of making them generic pin
> configuration properties and upset other developers?"

I guess Linus is better than me could answer to this.

> Anyway, I still fail to see why those configuration flags, only
> affecting the way the pin controller hardware enables/disables
> its internal buffers and its internal operations have to be
> described in term of their externally visible electrically characteristics.
>
>> Second question, what makes it differ to what already exists?
>
> To me, what already exists are pin configuration properties generic to
> the whole pin controller subsystem, and I understand you don't want to
> see duplication there.
>
> At the same time, to me, those flags are settings the pin controller
> wants to have specified by software to overcome its hw design flaws,
> and are intended to configure its internal buffers in a way it cannot
> do by itself for some very specific operation modes (they are listed
> in the hw reference manual, it's not something you can chose to
> configure or not, if you want a pin working in i2c mode, you HAVE to
> pass those flags to pin controller).

So, when you configuring pinmux to use group of pins to be i2c, what
does prevent you to apply those settings implicitly?

-- 
With Best Regards,
Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1638021 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

Hi Andy,

On Mon, May 08, 2017 at 08:47:17PM +0300, Andy Shevchenko wrote:
> On Mon, May 8, 2017 at 8:25 PM, jmondi <jacopo@jmondi.org> wrote:
> > Andy,
> >
> > On Mon, May 08, 2017 at 07:08:32PM +0300, Andy Shevchenko wrote:
> >> On Mon, May 8, 2017 at 7:01 PM, jmondi <jacopo@jmondi.org> wrote:
> >> > On Sun, May 07, 2017 at 09:52:49AM +0200, Linus Walleij wrote:
> >> >> On Fri, Apr 28, 2017 at 4:53 PM, Andy Shevchenko
> >> >> <andy.shevchenko@gmail.com> wrote:
> >> >>
> >> >> > Linus, for me it looks like better to revert that change, until we
> >> >> > will have clear picture why existing configuration parameters can't
> >> >> > work.
> >> >>
> >> >> Yeah I'll revert the binding for fixes.
> >>
> >> > As it seems we won't be able to proceed with the currently proposed solution,
> >> > would that be acceptable now that we use the "pinmux" property to add
> >> > flags as BIDIR
> >>
> >> Can you explain what does this *electrically* mean?
> >
> > I really don't know what to add to what Chris said in his 2 previous
> > replies to the same question, and I don't know if I can even get this
> > information as the most detailed drawing I can provide is what you
> > have seen already at page 2696 Fig. 54.1 of the following document.
> >
> > https://www.renesas.com/en-us/doc/products/mpumcu/doc/rz/r01uh0403ej0300_rz_a1h.pdf?key=ccbb2d79446f1cbd015031061140507c
>
> I didn't see before this document. (I had downloaded what Chris
> referred to, which has less than 1200 pages).
>
> The figure you pointed to is really nice and explains it, thank you.

Oh sorry, I thought you had seen this already :)
>
> So, BiDi in this hardware is just helper to enable Input
> simultaneously when you enable output.
>
> This makes me wonder what prevents you to do this in two steps in software?
> So, basically in terms of pin control framework you define this pin
> configuration as
>
> 1. PIN_CONFIG_INPUT_ENABLE:
> 2. PIN_CONFIG_OUTPUT:
>
> (or wise versa)
>

That could be doable, as when we're collecting generic pin
configuration to apply to the pin I can simply check if both of them
are enabled.

That would feel un-natural in dts anyway, for someone that is not that
into the pin controller sub system details.
If I would have to do something like  this, not knowing all the
reasonable pre-conditions we've been discussing about

pins {
    pinmux = < .. >;
    input-enable;
    output-high; /* or output-low, we can ignore the argument here */
}

In place of

pins {
   pinmux = < .. >;
   renesas,bi-directional;
}

And the hardware manual speaks of "bi-directional" everywhere, I would
be wondering what those guys implementing this were thinking :)

> > From my perspective these flags are configurations internal to the pin
> > controller hardware used to enable/disable input buffers when a pin needs to
> > perform in both direction.
>
> > The level of detail I can provide on this is the logical diagram we have pointed
> > you to already.
> >
> > As I assume you are trying to get this answer from us in order to
> > avoid duplicating things in pin controller sub-system, and I
> > understand this, but my question here was "can we have those flags as part
> > of the pinmux property argument list, as that property description
> > seems to allow us to do that, instead of making them generic pin
> > configuration properties and upset other developers?"
>
> I guess Linus is better than me could answer to this.
>
> > Anyway, I still fail to see why those configuration flags, only
> > affecting the way the pin controller hardware enables/disables
> > its internal buffers and its internal operations have to be
> > described in term of their externally visible electrically characteristics.
> >
> >> Second question, what makes it differ to what already exists?
> >
> > To me, what already exists are pin configuration properties generic to
> > the whole pin controller subsystem, and I understand you don't want to
> > see duplication there.
> >
> > At the same time, to me, those flags are settings the pin controller
> > wants to have specified by software to overcome its hw design flaws,
> > and are intended to configure its internal buffers in a way it cannot
> > do by itself for some very specific operation modes (they are listed
> > in the hw reference manual, it's not something you can chose to
> > configure or not, if you want a pin working in i2c mode, you HAVE to
> > pass those flags to pin controller).
>
> So, when you configuring pinmux to use group of pins to be i2c, what
> does prevent you to apply those settings implicitly?
>

Chris already gave some valid reasons why it would be hard to do this
considering the different part numbers this driver may handle, but I
would also like to add that I have counted > 100 cases where
bi-directional flag has to be applied just in the first 5 IO ports (on a
total of 13).

As there are RZ systems out there running with just < 9MB of SRAM,
adding a static table (or several, considering the different part numbers)
with at least 300 entries, is a considerable waste :(

For SWIO it would be easier, there are just 16 cases, all of them
listed in the hardware reference manual as Chris said.

Thanks
   j

> --
> With Best Regards,
> Andy Shevchenko

[toc] | [prev] | [next] | [standalone]

#1637726 — Re: [PATCH v5 01/10] pinctrl: generic: Add bi-directional and output-enable

On Mon, May 8, 2017 at 7:25 PM, jmondi <jacopo@jmondi.org> wrote:

> From my perspective these flags are configurations internal to the pin
> controller hardware used to enable/disable input buffers when a pin needs to
> perform in both direction.
> The level of detail I can provide on this is the logical diagram we have pointed
> you to already.
>
> As I assume you are trying to get this answer from us in order to
> avoid duplicating things in pin controller sub-system, and I
> understand this, but my question here was "can we have those flags as part
> of the pinmux property argument list, as that property description
> seems to allow us to do that, instead of making them generic pin
> configuration properties and upset other developers?"

Pinmux with all it's magic flags baked into one is not any better
or any more readable. The solution is already very pretty except
for these two flags which I am sure we can agree on a way forward
for.

What we choose between is not this or another less transparent
pin configuration mechanism, the mechanism (whether magic bits
to pinmux or reasonable properties) does not matter.

There is a strong preference to use the generic bindings.

So the discussion is whether to use:

bi-directional;
output-enable;

Or some already defined config flags.

If these are too idiomatic to be used by others, they should anyways
look similar, like:

renesas,bi-directional;
renesas,output-enable;

Like the Qualcomm weirdness found in drivers/pinctrl/qcom/pinctrl-spmi-gpio.c

qcom,pull-up-strength = <..>;

Check how they use
#define PMIC_GPIO_CONF_PULL_UP                 (PIN_CONFIG_END + 1)

Etc.

> Anyway, I still fail to see why those configuration flags, only
> affecting the way the pin controller hardware enables/disables
> its internal buffers and its internal operations have to be
> described in term of their externally visible electrically characteristics.

To me internal vs external is not what matters. What matters is
if this is likely to pop up in more platforms, and then the property
should be generic.

The generic pin config definitions are likely to be picked up by other
standards and even be inspiration to hardware engineers so that
is why it matters so much.

> To me, what already exists are pin configuration properties generic to
> the whole pin controller subsystem, and I understand you don't want to
> see duplication there.
>
> At the same time, to me, those flags are settings the pin controller
> wants to have specified by software to overcome its hw design flaws,
> and are intended to configure its internal buffers in a way it cannot
> do by itself for some very specific operation modes (they are listed
> in the hw reference manual, it's not something you can chose to
> configure or not, if you want a pin working in i2c mode, you HAVE to
> pass those flags to pin controller).

Sounds like a case for

renesas,bi-directional;
renesas,output-enable;

following the Qualcomm pattern in that case.

But let's see if something else comes out of this discussion.

Yours,
Linus Walleij

[toc] | [prev] | [next] | [standalone]

Page 2 of 3 — ← Prev page 1 [2] 3  Next page →

Back to top | Article view | linux.kernel

csiph-web