| Message ID | 1493281194-5200-2-git-send-email-jacopo+renesas@jmondi.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Geert Uytterhoeven |
| Headers | show |
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. > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> > --- > Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++ > drivers/pinctrl/pinconf-generic.c | 3 +++ > include/linux/pinctrl/pinconf-generic.h | 3 +++ > 3 files changed, 8 insertions(+) > > diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > index bf3f7b0..f2ed458 100644 > --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt > @@ -222,6 +222,7 @@ bias-bus-hold - latch weakly > bias-pull-up - pull up the pin > bias-pull-down - pull down the pin > bias-pull-pin-default - use pin-default pull state > +bi-directional - pin supports simultaneous input/output operations > drive-push-pull - drive actively high and low > drive-open-drain - drive with open drain > drive-open-source - drive with open source > @@ -234,6 +235,7 @@ input-debounce - debounce mode with debound time X > power-source - select between different power supplies > low-power-enable - enable low power mode > low-power-disable - disable low power mode > +output-enable - enable output on pin regardless of output value > output-low - set the pin to output mode with low level > output-high - set the pin to output mode with high level > slew-rate - set the slew rate > diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c > index ce3335a..03e6808 100644 > --- a/drivers/pinctrl/pinconf-generic.c > +++ b/drivers/pinctrl/pinconf-generic.c > @@ -35,6 +35,7 @@ static const struct pin_config_item conf_items[] = { > PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, > "input bias pull to pin specific state", NULL, false), > PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false), > + PCONFDUMP(PIN_CONFIG_BIDIRECTIONAL, "bi-directional pin operations", NULL, false), > PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false), > PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false), > PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false), > @@ -160,6 +161,7 @@ static const struct pinconf_generic_params dt_params[] = { > { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, > { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 }, > { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, > + { "bi-directional", PIN_CONFIG_BIDIRECTIONAL, 1 }, > { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, > { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, > { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, > @@ -172,6 +174,7 @@ static const struct pinconf_generic_params dt_params[] = { > { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, > { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 }, > { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 }, > + { "output-enable", PIN_CONFIG_OUTPUT, 1, }, > { "output-high", PIN_CONFIG_OUTPUT, 1, }, > { "output-low", PIN_CONFIG_OUTPUT, 0, }, > { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, > diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h > index 7620eb1..279e3c5 100644 > --- a/include/linux/pinctrl/pinconf-generic.h > +++ b/include/linux/pinctrl/pinconf-generic.h > @@ -42,6 +42,8 @@ > * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high > * impedance to VDD). If the argument is != 0 pull-up is enabled, > * if it is 0, pull-up is total, i.e. the pin is connected to VDD. > + * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow simultaneous > + * input and output operations. > * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open > * collector) which means it is usually wired with other output ports > * which are then pulled up with an external resistor. Setting this > @@ -96,6 +98,7 @@ enum pin_config_param { > PIN_CONFIG_BIAS_PULL_DOWN, > PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, > PIN_CONFIG_BIAS_PULL_UP, > + PIN_CONFIG_BIDIRECTIONAL, > PIN_CONFIG_DRIVE_OPEN_DRAIN, > PIN_CONFIG_DRIVE_OPEN_SOURCE, > PIN_CONFIG_DRIVE_PUSH_PULL, > -- > 2.7.4 >
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
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
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
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.
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
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.
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
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...
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
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
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
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
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.
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
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
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?
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.
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
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
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
Hi Linus et al, On Mon, May 8, 2017 at 11:19 PM, Linus Walleij <linus.walleij@linaro.org> wrote: > 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) The question I'm asking myself is: are these settings related to pin configuration (i.e. depending on the use of the pin, and several settings are valid, depending on the use case), or are they related to pinmux (i.e. defined by the function)? Configuring drive strength or pull-up are clearly related to pin configuration. Depending on what you connect, or on how you connect it, you want a different drive strength, or choose a different output buffer type (e.g. totem pole vs. open-collector). All of these are valid configurations, depending on the use case. But the settings RZ/A1H needs are different. Some (e.g. "input-enable") may sound like related to pin configuration. However, the big discerning factor is that these settings are implied by the pinmux function. Their presence is purely dictated by the chosen pinmux function. There's no use case for doing otherwise (i.e. adding them when not needed, or removing them when needed, according to the datasheet). Note that e.g. the existing "input-enable" property is clearly a pin configuration property. This is even reflected in DT, as the property is not specified as part of the "pinmux" property, but in the surrounding pin subnode of the pin-controller. Hence I think we should not use generic pin properties, but consider these settings to be part of pinmux configuration. As having large tables in the driver is undesirable, I think storing the settings in the "pinmux" property (by encoding them as flags passed to the RZA1_PINMUX() macro) is our best option. 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
On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > The question I'm asking myself is: are these settings related to pin > configuration (i.e. depending on the use of the pin, and several settings > are valid, depending on the use case), or are they related to pinmux > (i.e. defined by the function)? If they are intrinsic to the function, i.e. whenever someone wants to use that function they need to do this (nb the Documentation/pinctrl.txt definition of "function", nothing else) then this should IMO not be in the device tree at all, but hard-coded in the driver. E.g. if someone needs to use a function such as "i2c0", on whatever pins, then it should just be set up by the driver, no DT involved. It is a hardware pecularity, drivers drive hardware so... If however it depends on which pins it is used with, then it is pin configuration and should use a DT property. Yours, Linus Walleij
On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: Oops missed this: > Hence I think we should not use generic pin properties, but consider these > settings to be part of pinmux configuration. > As having large tables in the driver is undesirable, I think storing the > settings in the "pinmux" property (by encoding them as flags passed to the > RZA1_PINMUX() macro) is our best option. I think it is better to have large tables in the driver in this case. It is the lesser evil. Having unintelligible and hard to grasp stuff in the device tree that no user will understand or dare to touch is not good, then it is better to have it with the code, where it is being used, so the developers of the driver can see it when they are dealing with this (quirky) hardware. As you say this is actually fixing hardware bugs, we can expect these quirky tables to be gone in the next hardware generation, right? Then the right place for it is in the quirky driver for the quirky first-generation hardware. Yours, Linus Walleij
Hi Linus, On Fri, May 12, 2017 at 11:04 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > On Tue, May 9, 2017 at 12:54 PM, Geert Uytterhoeven > <geert@linux-m68k.org> wrote: > > Oops missed this: > >> Hence I think we should not use generic pin properties, but consider these >> settings to be part of pinmux configuration. >> As having large tables in the driver is undesirable, I think storing the >> settings in the "pinmux" property (by encoding them as flags passed to the >> RZA1_PINMUX() macro) is our best option. > > I think it is better to have large tables in the driver in this case. Jacopo, Chris: Would two bits per pin/function (none, input, output, bidir) be sufficient? That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes. Plus code to handle it. After all not that bad... > It is the lesser evil. > > Having unintelligible and hard to grasp stuff in the device tree that > no user will understand or dare to touch is not good, then it is better > to have it with the code, where it is being used, so the developers of > the driver can see it when they are dealing with this (quirky) hardware. > > As you say this is actually fixing hardware bugs, we can expect these > quirky tables to be gone in the next hardware generation, right? Let's hope so. Chris has a better crystal ball than I have ;-) 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
On Friday, May 12, 2017, linux-renesas-soc-owner@vger.kernel.org wrote: > As you say this is actually fixing hardware bugs, we can expect these > quirky tables to be gone in the next hardware generation, right? I see this particular pin controller as a one shot deal. For the next part in this series we are moving to a completely different controller which you could probably get away with simply using pinctrl-single. > Then the right place for it is in the quirky driver for the quirky > first-generation > hardware. Maybe to be more clear, I would say the design is not a 'bug', but rather an under sight of the original designers where are they stared to need pins that would be both in and out, they started tacking on more hardware. Chris
Hi Geert and Linus, On Friday, May 12, 2017, Geert Uytterhoeven wrote: > Jacopo, Chris: Would two bits per pin/function (none, input, output, > bidir) > be sufficient? > That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes. > Plus code to handle it. After all not that bad... OK...I give up! If that's what it takes to get it, I'm fine. NOTE, your math is a little off, the issue is that depending on the function that you use, you might need to do extra settings, so you'd have to have a lookup table for every pin & function. Each pin can have 1 of 8 functions (which is good because a 'byte' has 8 bits). So, 12 ports x 16 pins => 384 bytes (this table would just be for checking if bi-dir is needed) 12 ports x 16 pins => 384 bytes (this table would just be for checking if input is needed) 12 ports x 16 pins => 384 bytes (this table would just be for checking if input is needed) ------------ 1,152 bytes But then...there are package variations so you need another entire table for those parts. 1,152 bytes x 2 = 2,304 bytes However, if these tables are constants, they will reside in flash for the XIP_KERNEL systems, so that's OK. #What we should really do is just make a look-up table (tables) for the 'special' ones. But, we can have that discussion in a different thread. One final note: There is still a need for "input-enable" and "output-enable" for the timer pins. Because, when you choose the pin to be connected to the MTU2 timer, the pin can be used as either input-capture/output-compare/PWM and that's the user's choice. So that's probably a valid usage of the generic pin properties for configuration. Sorry Jacopo, but we'll need another round of patches. It sounds like for sure the bi-direction needs to get ripped back out. Chris
Hi Chris, On Fri, May 12, 2017 at 2:13 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote: > On Friday, May 12, 2017, Geert Uytterhoeven wrote: >> Jacopo, Chris: Would two bits per pin/function (none, input, output, >> bidir) >> be sufficient? >> That makes one u16 per pin. So roughtly 12 ports x 16 pins => 384 bytes. >> Plus code to handle it. After all not that bad... > > OK...I give up! > If that's what it takes to get it, I'm fine. > > NOTE, your math is a little off, the issue is that depending on the > function that you use, you might need to do extra settings, so you'd > have to have a lookup table for every pin & function. > Each pin can have 1 of 8 functions (which is good because a 'byte' has > 8 bits). > > So, > 12 ports x 16 pins => 384 bytes (this table would just be for checking if bi-dir is needed) > 12 ports x 16 pins => 384 bytes (this table would just be for checking if input is needed) > 12 ports x 16 pins => 384 bytes (this table would just be for checking if input is needed) ------------ > 1,152 bytes 12 x 16 = 192, not 384. Do you need all possible combinations of input, output, and bi-dir? I assumed they're mutually exclusive. If not, you need 3 bits/pin/function. > But then...there are package variations so you need another entire > table for those parts. > 1,152 bytes x 2 = 2,304 bytes With packages, do you mean e.g. RZ/A1H vs. RZ/A1L? These indeed differ, but should use different compatible values. Or do you mean QFP/BGA256 vs. BGA324? Isn't the former a subset of the latter? > #What we should really do is just make a look-up table (tables) for the > 'special' ones. But, we can have that discussion in a different thread. Yep, depending on what gives the smallest code/data size. > There is still a need for "input-enable" and "output-enable" for the timer > pins. Because, when you choose the pin to be connected to the MTU2 timer, > the pin can be used as either input-capture/output-compare/PWM and that's > the user's choice. So that's probably a valid usage of the generic pin > properties for configuration. OK. 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
Hi Geert, On Friday, May 12, 2017, Geert Uytterhoeven wrote: > 12 x 16 = 192, not 384. Opps, my math was off! (I think I need another cup of coffee this morning) > Do you need all possible combinations of input, output, and bi-dir? > I assumed they're mutually exclusive. If not, you need 3 bits/pin/function You're right, they are mutually exclusive, so 2 bits are fine. Also, the other pinout variations chips (RZ/A1L,A1LU,A1LC) only have 10 ports: 12 ports x 16 pins x 2bits-per-bit => 384 bytes 10 ports x 16 pins x 2bits-per-bit => 320 bytes 384 + 320 = 704 bytes (of const data) > With packages, do you mean e.g. RZ/A1H vs. RZ/A1L? Yes. > These indeed differ, > but > should use different compatible values. OK. Since r7s72100.dtsi has compatible = "renesas,r7s72100-ports"; We'll just override that in say my-rza1l-board.dts &pinctrl { compatible = "renesas,r7s72102-ports"; }; and then look for that in the driver. > Or do you mean QFP/BGA256 vs. BGA324? Isn't the former a subset of the > latter? The "package" doesn't matter, it's the pin assignments on the die: RZ/A1H = RZ/A1M = pin assignments #1 RZ/A1L = RZ/A1LU = RZ/A1LC = pin assignments #2 Then each of those parts above have multiple 'package options', but of course doesn't change the actual pin/function assignments (that's part of the silicon die) Chris
On Tue, May 9, 2017 at 5:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > 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. > I did not follow too much. But it seems IMX7ULP/Vybrid to be also a fan of generic output-enable/input-enable property. See: Figure 5-2. GPIO PAD in Page 241 http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf It has separate register bits to control input buffer enable and output buffer enable and we need set it property for GPIO function. Regards Dong Aisheng
Hi Linus, On Tue, May 23, 2017 at 06:08:31PM +0800, Dong Aisheng wrote: > On Tue, May 9, 2017 at 5:19 AM, Linus Walleij <linus.walleij@linaro.org> wrote: > > 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. > > > > I did not follow too much. > But it seems IMX7ULP/Vybrid to be also a fan of generic > output-enable/input-enable > property. > > See: > Figure 5-2. GPIO PAD in Page 241 > http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf > > It has separate register bits to control input buffer enable and > output buffer enable > and we need set it property for GPIO function. As it seems we have another user for 'output-enable' here, what if we just add that one to the generic bindings properties list, and we keep 'bi-directional' (which seems to be the most debated property we have added) out of generic properties? We can handle 'bi-directional' pins with static tables in our pin controller driver and not have it anywhere in DT. I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked in some previous email. I can send another version of that patch with only 'output-enable' if you wish. Once we reach consesus, I can then send v6 of our pin controller driver based on that. Thanks j [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0 > > Regards > Dong Aisheng
On Tue, May 23, 2017 at 8:37 PM, jmondi <jacopo@jmondi.org> wrote: >> I did not follow too much. >> But it seems IMX7ULP/Vybrid to be also a fan of generic >> output-enable/input-enable >> property. >> >> See: >> Figure 5-2. GPIO PAD in Page 241 >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf >> >> It has separate register bits to control input buffer enable and >> output buffer enable >> and we need set it property for GPIO function. > > As it seems we have another user for 'output-enable' here, what if we just > add that one to the generic bindings properties list, and we keep > 'bi-directional' (which seems to be the most debated property we have > added) out of generic properties? > > We can handle 'bi-directional' pins with static tables in our pin > controller driver and not have it anywhere in DT. This sounds like a viable approach. I just want to know if "output-enable" is the right name? "output-buffer-enable"? > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked > in some previous email. I'm just overloaded. I sent that revert to Torvalds today. > I can send another version of that patch with > only 'output-enable' if you wish. That's what we want. > Once we reach consesus, I can then send v6 of our pin controller driver > based on that. OK sounds like a plan. Sorry for the mess, I'm just trying to get this right :/ Yours, Linus Walleij
Hi Linus, On Mon, May 29, 2017 at 10:45:44AM +0200, Linus Walleij wrote: > On Tue, May 23, 2017 at 8:37 PM, jmondi <jacopo@jmondi.org> wrote: > > >> I did not follow too much. > >> But it seems IMX7ULP/Vybrid to be also a fan of generic > >> output-enable/input-enable > >> property. > >> > >> See: > >> Figure 5-2. GPIO PAD in Page 241 > >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf > >> > >> It has separate register bits to control input buffer enable and > >> output buffer enable > >> and we need set it property for GPIO function. > > > > As it seems we have another user for 'output-enable' here, what if we just > > add that one to the generic bindings properties list, and we keep > > 'bi-directional' (which seems to be the most debated property we have > > added) out of generic properties? > > > > We can handle 'bi-directional' pins with static tables in our pin > > controller driver and not have it anywhere in DT. > > This sounds like a viable approach. > > I just want to know if "output-enable" is the right name? > "output-buffer-enable"? Great! Thanks! On naming: if we need "output-buffer-enable" should we add "input-buffer-enable" as well? Currently we are using "input-enable" to pair with "output-enable", but as you said, just "output-enable" when "output-high" and "output-low" are there already seems a bit confusing. At the same time "input-buffer-enable" seems to actually be just electrically equivalent to "input-enable", so adding it is a bit of a waste as well. I see three options here: 1) Add "output-buffer-enable" and "input-buffer-enable" we end up with "output-high" "output-low" "input-enable" "output-buffer-enable" "input-buffer-enable" 2) Add "output-buffer-enable" only we end up with "output-high" "output-low" "input-enable" "output-buffer-enable" Binding may be confusing as in one case we use "output-buffer-enable" while in the other "input-enable" 3) Add "output-enable" only "output-high" "output-low" "input-enable" "output-enable" As you, I don't like "output-enable" that much but it pairs better with "input-enable". I'll let you and DT people decide on this, as it's really an ABI definition problem and you have better judgment there. > > > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked > > in some previous email. > > I'm just overloaded. I sent that revert to Torvalds today. Thank you. Didn't want to put pressure ;) > > > I can send another version of that patch with > > only 'output-enable' if you wish. > > That's what we want. > > > Once we reach consesus, I can then send v6 of our pin controller driver > > based on that. > > OK sounds like a plan. > > Sorry for the mess, I'm just trying to get this right :/ Not a mess, and thanks for your effort in maintaining all of this Thanks j > > Yours, > Linus Walleij
Hello Jacopo and Linus, On Monday, May 29, 2017, jmondi wrote: > > > We can handle 'bi-directional' pins with static tables in our pin > > > controller driver and not have it anywhere in DT. > > > > This sounds like a viable approach. > > > > I just want to know if "output-enable" is the right name? > > "output-buffer-enable"? > > Great! Thanks! > > On naming: if we need "output-buffer-enable" should we add > "input-buffer-enable" as well? > > Currently we are using "input-enable" to pair with "output-enable", > but as you said, just "output-enable" when "output-high" and > "output-low" are there already seems a bit confusing. > At the same time "input-buffer-enable" seems to actually be just > electrically equivalent to "input-enable", so adding it is a bit of a > waste as well. Here is what I think: In the case of this driver, after we remove the 'bi-directional' properties and hide the other odd-ball pin configurations in an internal table, we are left with the MTU2 timer pins that can be either input or output depending on what you want to do with them. * If you want to use a MTU2 channel as a PWM, you set the pin as an output. * If you want to use a MTU2 channel as a input capture, you set the pin as an input. They are simply "direction-input" and "direction-output" properties that don't really need to talk about "buffers". But, instead of making any new properties, for the Renesas driver, let's just stick with what already exists today: * If you want a MTU2 channel as a PWM: select "output-low" * If you want a MTU2 channel as a input capture: select "input-enable" Side Note: You can also use output-high in addition to output-low because it doesn't matter (the driver can't set the pin level anyway because as soon as you assign the pin to MTU2, the MTU2 controls the pin, not the PFC). So the Renesas driver can check for both. Chris
Hi Linus & j, On Mon, May 29, 2017 at 6:42 PM, jmondi <jacopo@jmondi.org> wrote: > Hi Linus, > > On Mon, May 29, 2017 at 10:45:44AM +0200, Linus Walleij wrote: >> On Tue, May 23, 2017 at 8:37 PM, jmondi <jacopo@jmondi.org> wrote: >> >> >> I did not follow too much. >> >> But it seems IMX7ULP/Vybrid to be also a fan of generic >> >> output-enable/input-enable >> >> property. >> >> >> >> See: >> >> Figure 5-2. GPIO PAD in Page 241 >> >> http://www.nxp.com/assets/documents/data/en/reference-manuals/VFXXXRM.pdf >> >> >> >> It has separate register bits to control input buffer enable and >> >> output buffer enable >> >> and we need set it property for GPIO function. >> > >> > As it seems we have another user for 'output-enable' here, what if we just >> > add that one to the generic bindings properties list, and we keep >> > 'bi-directional' (which seems to be the most debated property we have >> > added) out of generic properties? >> > >> > We can handle 'bi-directional' pins with static tables in our pin >> > controller driver and not have it anywhere in DT. >> >> This sounds like a viable approach. >> >> I just want to know if "output-enable" is the right name? >> "output-buffer-enable"? > > Great! Thanks! > > On naming: if we need "output-buffer-enable" should we add > "input-buffer-enable" as well? > > Currently we are using "input-enable" to pair with "output-enable", > but as you said, just "output-enable" when "output-high" and > "output-low" are there already seems a bit confusing. > At the same time "input-buffer-enable" seems to actually be just > electrically equivalent to "input-enable", so adding it is a bit of a > waste as well. > > I see three options here: > > 1) Add "output-buffer-enable" and "input-buffer-enable" > we end up with > "output-high" > "output-low" > "input-enable" > "output-buffer-enable" > "input-buffer-enable" > > 2) Add "output-buffer-enable" only > we end up with > "output-high" > "output-low" > "input-enable" > "output-buffer-enable" > > Binding may be confusing as in one case we use "output-buffer-enable" > while in the other "input-enable" > > 3) Add "output-enable" only > "output-high" > "output-low" > "input-enable" > "output-enable" > > As you, I don't like "output-enable" that much but it pairs better with > "input-enable". > > I'll let you and DT people decide on this, as it's really an ABI definition > problem and you have better judgment there. > What's the final decision of this? I saw the following revert patch in pinctrl-next but did not see a successive patch to add output-enable back? IMX7ULP pinctrl driver is pending on this because it needs use both input-enable and output-enable if we want to make them generic property. commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b Author: Linus Walleij <linus.walleij@linaro.org> Date: Mon May 8 10:48:21 2017 +0200 Revert "pinctrl: generic: Add bi-directional and output-enable" This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0. It turns out that applying these generic properties was premature: the properties used in the driver using this are of unclear electrical nature and the subject need to be discussed. Signed-off-by: Linus Walleij <linus.walleij@linaro.org> Regards Dong Aisheng >> >> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked >> > in some previous email. >> >> I'm just overloaded. I sent that revert to Torvalds today. > > Thank you. Didn't want to put pressure ;) >> >> > I can send another version of that patch with >> > only 'output-enable' if you wish. >> >> That's what we want. >> >> > Once we reach consesus, I can then send v6 of our pin controller driver >> > based on that. >> >> OK sounds like a plan. >> >> Sorry for the mess, I'm just trying to get this right :/ > > Not a mess, and thanks for your effort in maintaining all of this > > Thanks > j >> >> Yours, >> Linus Walleij
Hi Dong, On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote: > Hi Linus & j, > > >> > >> I just want to know if "output-enable" is the right name? > >> "output-buffer-enable"? > > > > Great! Thanks! > > > > On naming: if we need "output-buffer-enable" should we add > > "input-buffer-enable" as well? > > > > Currently we are using "input-enable" to pair with "output-enable", > > but as you said, just "output-enable" when "output-high" and > > "output-low" are there already seems a bit confusing. > > At the same time "input-buffer-enable" seems to actually be just > > electrically equivalent to "input-enable", so adding it is a bit of a > > waste as well. > > > > I see three options here: > > > > 1) Add "output-buffer-enable" and "input-buffer-enable" > > we end up with > > "output-high" > > "output-low" > > "input-enable" > > "output-buffer-enable" > > "input-buffer-enable" > > > > 2) Add "output-buffer-enable" only > > we end up with > > "output-high" > > "output-low" > > "input-enable" > > "output-buffer-enable" > > > > Binding may be confusing as in one case we use "output-buffer-enable" > > while in the other "input-enable" > > > > 3) Add "output-enable" only > > "output-high" > > "output-low" > > "input-enable" > > "output-enable" > > > > As you, I don't like "output-enable" that much but it pairs better with > > "input-enable". > > > > I'll let you and DT people decide on this, as it's really an ABI definition > > problem and you have better judgment there. > > > > What's the final decision of this? I admit a was buying a bit of time and post-poned the gentle ping for any final word on this. But since you're asking I'll second your question :) > > I saw the following revert patch in pinctrl-next but did not see a successive > patch to add output-enable back? > Still waiting to have a feedback on which properties to add, that's why I have not sent anything yet. Thanks j > IMX7ULP pinctrl driver is pending on this because it needs use both > input-enable and output-enable if we want to make them generic property. > > commit b4d2ea2af95cb77e2f320e24da526280d4aa2f6b > Author: Linus Walleij <linus.walleij@linaro.org> > Date: Mon May 8 10:48:21 2017 +0200 > > Revert "pinctrl: generic: Add bi-directional and output-enable" > > This reverts commit 8c58f1a7a4b6d1d723bf25fef9d842d5a11200d0. > > It turns out that applying these generic properties was > premature: the properties used in the driver using this > are of unclear electrical nature and the subject need to > be discussed. > > Signed-off-by: Linus Walleij <linus.walleij@linaro.org> > > Regards > Dong Aisheng > > >> > >> > I see commit 42d5a11200d0[1] has not been reverted yet as Andy asked > >> > in some previous email. > >> > >> I'm just overloaded. I sent that revert to Torvalds today. > > > > Thank you. Didn't want to put pressure ;) > >> > >> > I can send another version of that patch with > >> > only 'output-enable' if you wish. > >> > >> That's what we want. > >> > >> > Once we reach consesus, I can then send v6 of our pin controller driver > >> > based on that. > >> > >> OK sounds like a plan. > >> > >> Sorry for the mess, I'm just trying to get this right :/ > > > > Not a mess, and thanks for your effort in maintaining all of this > > > > Thanks > > j > >> > >> Yours, > >> Linus Walleij
On Fri, Jun 9, 2017 at 9:50 AM, jmondi <jacopo@jmondi.org> wrote: > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote: >> > I see three options here: >> > >> > 1) Add "output-buffer-enable" and "input-buffer-enable" >> > we end up with >> > "output-high" >> > "output-low" >> > "input-enable" >> > "output-buffer-enable" >> > "input-buffer-enable" >> > >> > 2) Add "output-buffer-enable" only >> > we end up with >> > "output-high" >> > "output-low" >> > "input-enable" >> > "output-buffer-enable" >> > >> > Binding may be confusing as in one case we use "output-buffer-enable" >> > while in the other "input-enable" >> > >> > 3) Add "output-enable" only >> > "output-high" >> > "output-low" >> > "input-enable" >> > "output-enable" >> > >> > As you, I don't like "output-enable" that much but it pairs better with >> > "input-enable". >> > >> > I'll let you and DT people decide on this, as it's really an ABI definition >> > problem and you have better judgment there. >> > >> >> What's the final decision of this? > > I admit a was buying a bit of time and post-poned the gentle ping for > any final word on this. But since you're asking I'll second your > question :) I suspect it is time to quote Documentation/process/management-style.rst (Torvalds): 1) Decisions Everybody thinks managers make decisions, and that decision-making is important. The bigger and more painful the decision, the bigger the manager must be to make it. That's very deep and obvious, but it's not actually true. The name of the game is to **avoid** having to make a decision. In particular, if somebody tells you "choose (a) or (b), we really need you to decide on this", you're in trouble as a manager. The people you manage had better know the details better than you, so if they come to you for a technical decision, you're screwed. You're clearly not competent to make that decision for them. (It goes on, it's the best part of the entire Documentation/* dir in my opinion, please take the time to read it in full.) So: what do you guys, using this feature, and Andy, who raised serious concerns, think is the right binding? That is what *I* need to know. Yours, Linus Walleij
Hi Linus, On Sun, Jun 11, 2017 at 11:45:49PM +0200, Linus Walleij wrote: > On Fri, Jun 9, 2017 at 9:50 AM, jmondi <jacopo@jmondi.org> wrote: > > On Fri, Jun 09, 2017 at 03:26:57PM +0800, Dong Aisheng wrote: > > >> > I see three options here: > >> > > >> > 1) Add "output-buffer-enable" and "input-buffer-enable" > >> > we end up with > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-buffer-enable" > >> > "input-buffer-enable" > >> > > >> > 2) Add "output-buffer-enable" only > >> > we end up with > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-buffer-enable" > >> > > >> > Binding may be confusing as in one case we use "output-buffer-enable" > >> > while in the other "input-enable" > >> > > >> > 3) Add "output-enable" only > >> > "output-high" > >> > "output-low" > >> > "input-enable" > >> > "output-enable" > >> > > >> > As you, I don't like "output-enable" that much but it pairs better with > >> > "input-enable". > >> > > >> > I'll let you and DT people decide on this, as it's really an ABI definition > >> > problem and you have better judgment there. > >> > > >> > >> What's the final decision of this? > > > > I admit a was buying a bit of time and post-poned the gentle ping for > > any final word on this. But since you're asking I'll second your > > question :) > > I suspect it is time to quote > Documentation/process/management-style.rst > (Torvalds): > > 1) Decisions > > Everybody thinks managers make decisions, and that decision-making is > important. The bigger and more painful the decision, the bigger the > manager must be to make it. That's very deep and obvious, but it's not > actually true. > > The name of the game is to **avoid** having to make a decision. In > particular, if somebody tells you "choose (a) or (b), we really need you > to decide on this", you're in trouble as a manager. The people you > manage had better know the details better than you, so if they come to > you for a technical decision, you're screwed. You're clearly not > competent to make that decision for them. > > (It goes on, it's the best part of the entire Documentation/* dir in my > opinion, please take the time to read it in full.) > > So: what do you guys, using this feature, and Andy, who raised serious > concerns, think is the right binding? That is what *I* need to know. Fair enough :) I'll try to keep this short: I don't like "output-enable", and at the same time I don't think "output-high" and "output-low" fit well for this purpose, as they electrically means something different from what our (and IMX) use case is: enabling/disabling input/output buffers internal to pin controller/gpio block HW and not driving a value there. This seems clear to me from the "GPIO mode pitfalls" section of pinctrl.txt documentation examples and from the fact that generic bindings did not expose an "output" flag because if you drive an output line, you reasonably either drive it high or low. Unfortunately I cannot convince myself that the same reasons apply to the input use case. Enabling input on a pin implies the pinctrl/gpio driver has to enable any input buffer required to use that pin as a properly working input line, and enabling an input buffer implies being able to sense the line value from there, so I don't see that much use for "input-buffer-enable" alone. So, even if bindings could look a bit weird as there won't be a direct matching between properties names used to enable input/output buffers, my vote is to add "output-buffer-enable" only, and keep using the already there "input-enable" properties for the input use case. Thanks j > > Yours, > Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt index bf3f7b0..f2ed458 100644 --- a/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt +++ b/Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt @@ -222,6 +222,7 @@ bias-bus-hold - latch weakly bias-pull-up - pull up the pin bias-pull-down - pull down the pin bias-pull-pin-default - use pin-default pull state +bi-directional - pin supports simultaneous input/output operations drive-push-pull - drive actively high and low drive-open-drain - drive with open drain drive-open-source - drive with open source @@ -234,6 +235,7 @@ input-debounce - debounce mode with debound time X power-source - select between different power supplies low-power-enable - enable low power mode low-power-disable - disable low power mode +output-enable - enable output on pin regardless of output value output-low - set the pin to output mode with low level output-high - set the pin to output mode with high level slew-rate - set the slew rate diff --git a/drivers/pinctrl/pinconf-generic.c b/drivers/pinctrl/pinconf-generic.c index ce3335a..03e6808 100644 --- a/drivers/pinctrl/pinconf-generic.c +++ b/drivers/pinctrl/pinconf-generic.c @@ -35,6 +35,7 @@ static const struct pin_config_item conf_items[] = { PCONFDUMP(PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, "input bias pull to pin specific state", NULL, false), PCONFDUMP(PIN_CONFIG_BIAS_PULL_UP, "input bias pull up", NULL, false), + PCONFDUMP(PIN_CONFIG_BIDIRECTIONAL, "bi-directional pin operations", NULL, false), PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_DRAIN, "output drive open drain", NULL, false), PCONFDUMP(PIN_CONFIG_DRIVE_OPEN_SOURCE, "output drive open source", NULL, false), PCONFDUMP(PIN_CONFIG_DRIVE_PUSH_PULL, "output drive push pull", NULL, false), @@ -160,6 +161,7 @@ static const struct pinconf_generic_params dt_params[] = { { "bias-pull-up", PIN_CONFIG_BIAS_PULL_UP, 1 }, { "bias-pull-pin-default", PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, 1 }, { "bias-pull-down", PIN_CONFIG_BIAS_PULL_DOWN, 1 }, + { "bi-directional", PIN_CONFIG_BIDIRECTIONAL, 1 }, { "drive-open-drain", PIN_CONFIG_DRIVE_OPEN_DRAIN, 0 }, { "drive-open-source", PIN_CONFIG_DRIVE_OPEN_SOURCE, 0 }, { "drive-push-pull", PIN_CONFIG_DRIVE_PUSH_PULL, 0 }, @@ -172,6 +174,7 @@ static const struct pinconf_generic_params dt_params[] = { { "input-schmitt-enable", PIN_CONFIG_INPUT_SCHMITT_ENABLE, 1 }, { "low-power-disable", PIN_CONFIG_LOW_POWER_MODE, 0 }, { "low-power-enable", PIN_CONFIG_LOW_POWER_MODE, 1 }, + { "output-enable", PIN_CONFIG_OUTPUT, 1, }, { "output-high", PIN_CONFIG_OUTPUT, 1, }, { "output-low", PIN_CONFIG_OUTPUT, 0, }, { "power-source", PIN_CONFIG_POWER_SOURCE, 0 }, diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h index 7620eb1..279e3c5 100644 --- a/include/linux/pinctrl/pinconf-generic.h +++ b/include/linux/pinctrl/pinconf-generic.h @@ -42,6 +42,8 @@ * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high * impedance to VDD). If the argument is != 0 pull-up is enabled, * if it is 0, pull-up is total, i.e. the pin is connected to VDD. + * @PIN_CONFIG_BIDIRECTIONAL: the pin will be configured to allow simultaneous + * input and output operations. * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open * collector) which means it is usually wired with other output ports * which are then pulled up with an external resistor. Setting this @@ -96,6 +98,7 @@ enum pin_config_param { PIN_CONFIG_BIAS_PULL_DOWN, PIN_CONFIG_BIAS_PULL_PIN_DEFAULT, PIN_CONFIG_BIAS_PULL_UP, + PIN_CONFIG_BIDIRECTIONAL, PIN_CONFIG_DRIVE_OPEN_DRAIN, PIN_CONFIG_DRIVE_OPEN_SOURCE, PIN_CONFIG_DRIVE_PUSH_PULL,
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. Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org> --- Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt | 2 ++ drivers/pinctrl/pinconf-generic.c | 3 +++ include/linux/pinctrl/pinconf-generic.h | 3 +++ 3 files changed, 8 insertions(+)