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

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

[Jacopo Mondi]

Hi Geert,
   this is 5th round of gpio/pincontroller for RZ/A1 devices.

I have updated the pin controller driver to use the newly introduced
"pinctrl_enable()" function.
This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
the pin controller does not start.

I have incorporated your comments on the device tree bindings documentation,
and added to pinctrl-generic.h header file two macros to unpack generic
properties and their arguments.

Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.

Thanks
   j

v1 -> v2:
- change pin configuration flags as suggested by Chris
- gpio set direction function fixed as suggested by Chris
- add some more example on pin configuration flag usage to dt-binding doc
- fix gpio-controller names to remove unit address as suggested by Geert
- some comments chopped here and there to make the driver less verbose

v2 -> v3:
- fix grammar and syntax in comment and documentation
- fix code style (reverse xmas tree ordering in variable declaration)
- use irqsave/irqrestore in spinlock lock/unlock
- use devm_ version of kasprintf (memory returned was not properly free)
- use bitops.h operation ffs and fls to make sure a single bit is set in pmx
  mask
- Add Geert's reviewed-by to DTS patches

v3 -> v4:
- use "pinmux" property in pmx sub-nodes in place of "renesas,pins"
- use pinconf standard properties to set pin mux additional flags
- add "bi-directional" and "output-enable" to pinconf generic properties
- perform pmx function parsing at dt_node_to_map() time
- change DT bindings to use GENERIC_PINCONF
- change DT bindings to allow sub-nodes to have "pinmux" property specified
- several renames (register names, DT parse functions, set_mux() function)

v4 -> v5:
- use pinctrl_enable() function in pin controller registration function
- update bindings documentation to incorporate Geert's comments
- add generic properties unpack macros

Jacopo Mondi (10):
  pinctrl: generic: Add bi-directional and output-enable
  pinctrl: generic: Add macros to unpack properties
  pinctrl: Renesas RZ/A1 pin and gpio controller
  dt-bindings: pinctrl: Add RZ/A1 bindings doc
  arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
  arm: dts: r7s72100: Add pin controller node
  arm: dts: genmai: Add SCIF2 pin group
  arm: dts: genmai: Add RIIC2 pin group
  arm: dts: genmai: Add user led device nodes
  arm: dts: genmai: Add ethernet pin group

 .../bindings/pinctrl/pinctrl-bindings.txt          |   2 +
 .../bindings/pinctrl/renesas,rza1-pinctrl.txt      | 219 +++++
 arch/arm/boot/dts/r7s72100-genmai.dts              |  76 ++
 arch/arm/boot/dts/r7s72100.dtsi                    |  78 ++
 drivers/pinctrl/Kconfig                            |  11 +
 drivers/pinctrl/Makefile                           |   1 +
 drivers/pinctrl/pinconf-generic.c                  |   3 +
 drivers/pinctrl/pinctrl-rza1.c                     | 995 +++++++++++++++++++++
 include/dt-bindings/pinctrl/r7s72100-pinctrl.h     |  16 +
 include/linux/pinctrl/pinconf-generic.h            |   7 +-
 10 files changed, 1407 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
 create mode 100644 drivers/pinctrl/pinctrl-rza1.c
 create mode 100644 include/dt-bindings/pinctrl/r7s72100-pinctrl.h

--
2.7.4

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

[Jacopo Mondi]

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(+)

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

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

[Andy Shevchenko]

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
>



-- 
With Best Regards,
Andy Shevchenko

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

[Linus Walleij]

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

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

[Geert Uytterhoeven]

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

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

[Linus Walleij]

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

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

[Chris Brandt]

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

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

[Andy Shevchenko]

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

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

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Brandt]

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

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

[Andy Shevchenko]

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

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

Not that one I would like to see...

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

This is useful. Thanks.

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

Had you read the following, esp. Note there?

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

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

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

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

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

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Brandt]

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

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

[Andy Shevchenko]

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

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

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

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

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

No.

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

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

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

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

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

Perhaps needs to be revisited as well.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Brandt]

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

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

[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

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

[jmondi]

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

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

[Andy Shevchenko]

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

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

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

>  and SWIO_[INPUT|OUTPUT] directly there?

Ditto.

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


-- 
With Best Regards,
Andy Shevchenko

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

[Chris Brandt]

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

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

[Andy Shevchenko]

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

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

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

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

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

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

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

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

Yes, that figure helped me a lot to understand.

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Brandt]

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

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

[jmondi]

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

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

[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?

-- 
With Best Regards,
Andy Shevchenko

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

[jmondi]

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

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

[Linus Walleij]

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

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

[Geert Uytterhoeven]

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

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

[Linus Walleij]

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

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

[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

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

[Geert Uytterhoeven]

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

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

[Chris Brandt]

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

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

[Geert Uytterhoeven]

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

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

[Chris Brandt]

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

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

[Chris Brandt]

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

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

[Dong Aisheng]

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

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

[jmondi]

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

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

[Linus Walleij]

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

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

[jmondi]

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

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

[Chris Brandt]

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

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

[Dong Aisheng]

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

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

[jmondi]

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

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

[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

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

[jmondi]

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

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

[Dong Aisheng]

On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
> 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.
>

Yes, it may be a bit weird.
I'm not pad internal details expert and can't tell much difference between
output-enable and output-buffer-enable.
I just feel a bit confuse if only using output-buffer-enable.

If enable both input and output, it becomes:
pinctrl_xxx: gpios_xxx_grp {
        pins = <
                ULP1_PAD_PTD0__PTD0
        >;
        input-enable;
        output-buffer-enable;
        bias-pull-up;
};

How about still use output-enable in pairs to input-enable but explain more
in comments?
Aslo update 'input-enable' comment to 'enable input buffer'.
e.g.
diff --git a/drivers/pinctrl/pinconf-generic.c
b/drivers/pinctrl/pinconf-generic.c
index 720a19f..96c83a4 100644
--- a/drivers/pinctrl/pinconf-generic.c
+++ b/drivers/pinctrl/pinconf-generic.c
@@ -172,6 +172,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_ENABLE, 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..d30f4fe 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -59,9 +59,9 @@
  *     which means it will wait for signals to settle when reading inputs. The
  *     argument gives the debounce time in usecs. Setting the
  *     argument to zero turns debouncing off.
- * @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.
+ * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
that this does
+ *     not affect the pin's ability to drive output.
+ *     1 enables input, 0 disables input.
  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
  *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
  *     the threshold value is given on a custom format as argument when
@@ -73,6 +73,9 @@
  *     operation, if several modes of operation are supported these can be
  *     passed in the argument on a custom form, else just use argument 1
  *     to indicate low power mode, argument 0 turns low power mode off.
+ * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
+ *     a value.
+ *     1 enables output buffer, 0 disables output buffer.
  * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
  *     1 to indicate high level, argument 0 to indicate low level. (Please
  *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a

Or
invent both input-buffer-enable and output-buffer-enable and
deprecated input-enable?

Andy,
how about your comments?

Regards
Dong Aisheng

> Thanks
>    j
>
>>
>> Yours,
>> Linus Walleij

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

[jmondi]

Hi Dong,

On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:
> On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
> > 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.
> >
>
> Yes, it may be a bit weird.
> I'm not pad internal details expert and can't tell much difference between
> output-enable and output-buffer-enable.
> I just feel a bit confuse if only using output-buffer-enable.

Yes it is, and I actually like your proposal, I was just trying to
make sure I was not confusing the property semantic with its
real-world effect.

If no one as different opinions on this, I can send a patch later to
add output-enable only, or since you have almost done it down here you
can do the same resusing what you have proposed below.
>
> If enable both input and output, it becomes:
> pinctrl_xxx: gpios_xxx_grp {
>         pins = <
>                 ULP1_PAD_PTD0__PTD0
>         >;
>         input-enable;
>         output-buffer-enable;
>         bias-pull-up;
> };
>
> How about still use output-enable in pairs to input-enable but explain more
> in comments?
> Aslo update 'input-enable' comment to 'enable input buffer'.
> e.g.
> diff --git a/drivers/pinctrl/pinconf-generic.c
> b/drivers/pinctrl/pinconf-generic.c
> index 720a19f..96c83a4 100644
> --- a/drivers/pinctrl/pinconf-generic.c
> +++ b/drivers/pinctrl/pinconf-generic.c
> @@ -172,6 +172,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_ENABLE, 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..d30f4fe 100644
> --- a/include/linux/pinctrl/pinconf-generic.h
> +++ b/include/linux/pinctrl/pinconf-generic.h
> @@ -59,9 +59,9 @@
>   *     which means it will wait for signals to settle when reading inputs. The
>   *     argument gives the debounce time in usecs. Setting the
>   *     argument to zero turns debouncing off.
> - * @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.
> + * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
> that this does
> + *     not affect the pin's ability to drive output.
> + *     1 enables input, 0 disables input.

I would not mention the "input buffer" here, as enabling input implies enabling
the buffer if you want to read values from there. Actually I guess
there may be platforms where buffer enabling may be implicit, so I
would leave this out and let drivers handle it internally.

>   * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>   *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>   *     the threshold value is given on a custom format as argument when
> @@ -73,6 +73,9 @@
>   *     operation, if several modes of operation are supported these can be
>   *     passed in the argument on a custom form, else just use argument 1
>   *     to indicate low power mode, argument 0 turns low power mode off.
> + * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
> + *     a value.
> + *     1 enables output buffer, 0 disables output buffer.
>   * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
>   *     1 to indicate high level, argument 0 to indicate low level. (Please
>   *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
>
> Or
> invent both input-buffer-enable and output-buffer-enable and
> deprecated input-enable?
>
> Andy,
> how about your comments?
>
> Regards
> Dong Aisheng
>
> > Thanks
> >    j
> >
> >>
> >> Yours,
> >> Linus Walleij

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

[Dong Aisheng]

Hi Jmondi,

On Thu, Jun 15, 2017 at 7:11 PM, jmondi <jacopo@jmondi.org> wrote:
> Hi Dong,
>
> On Tue, Jun 13, 2017 at 02:25:08PM +0800, Dong Aisheng wrote:
>> On Mon, Jun 12, 2017 at 5:44 PM, jmondi <jacopo@jmondi.org> wrote:
>> > 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.
>> >
>>
>> Yes, it may be a bit weird.
>> I'm not pad internal details expert and can't tell much difference between
>> output-enable and output-buffer-enable.
>> I just feel a bit confuse if only using output-buffer-enable.
>
> Yes it is, and I actually like your proposal, I was just trying to
> make sure I was not confusing the property semantic with its
> real-world effect.
>
> If no one as different opinions on this, I can send a patch later to
> add output-enable only, or since you have almost done it down here you
> can do the same resusing what you have proposed below.

Please feel free to do it.
Just one thing, may be we could also add PIN_CONFIG_OUTPUT_ENABLE
as well.

>>
>> If enable both input and output, it becomes:
>> pinctrl_xxx: gpios_xxx_grp {
>>         pins = <
>>                 ULP1_PAD_PTD0__PTD0
>>         >;
>>         input-enable;
>>         output-buffer-enable;
>>         bias-pull-up;
>> };
>>
>> How about still use output-enable in pairs to input-enable but explain more
>> in comments?
>> Aslo update 'input-enable' comment to 'enable input buffer'.
>> e.g.
>> diff --git a/drivers/pinctrl/pinconf-generic.c
>> b/drivers/pinctrl/pinconf-generic.c
>> index 720a19f..96c83a4 100644
>> --- a/drivers/pinctrl/pinconf-generic.c
>> +++ b/drivers/pinctrl/pinconf-generic.c
>> @@ -172,6 +172,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_ENABLE, 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..d30f4fe 100644
>> --- a/include/linux/pinctrl/pinconf-generic.h
>> +++ b/include/linux/pinctrl/pinconf-generic.h
>> @@ -59,9 +59,9 @@
>>   *     which means it will wait for signals to settle when reading inputs. The
>>   *     argument gives the debounce time in usecs. Setting the
>>   *     argument to zero turns debouncing off.
>> - * @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.
>> + * @PIN_CONFIG_INPUT_ENABLE: enable the pin's input buffer.  Note
>> that this does
>> + *     not affect the pin's ability to drive output.
>> + *     1 enables input, 0 disables input.
>
> I would not mention the "input buffer" here, as enabling input implies enabling
> the buffer if you want to read values from there. Actually I guess
> there may be platforms where buffer enabling may be implicit, so I
> would leave this out and let drivers handle it internally.
>

I'm fine with it.

Regards
Dong Aisheng

>>   * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>>   *     schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>>   *     the threshold value is given on a custom format as argument when
>> @@ -73,6 +73,9 @@
>>   *     operation, if several modes of operation are supported these can be
>>   *     passed in the argument on a custom form, else just use argument 1
>>   *     to indicate low power mode, argument 0 turns low power mode off.
>> + * @PIN_CONFIG_OUTPUT_ENABLE: only enable the pin's output buffer, not driving
>> + *     a value.
>> + *     1 enables output buffer, 0 disables output buffer.
>>   * @PIN_CONFIG_OUTPUT: this will configure the pin as an output. Use argument
>>   *     1 to indicate high level, argument 0 to indicate low level. (Please
>>   *     see Documentation/pinctrl.txt, section "GPIO mode pitfalls" for a
>>
>> Or
>> invent both input-buffer-enable and output-buffer-enable and
>> deprecated input-enable?
>>
>> Andy,
>> how about your comments?
>>
>> Regards
>> Dong Aisheng
>>
>> > Thanks
>> >    j
>> >
>> >>
>> >> Yours,
>> >> Linus Walleij

[PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[Jacopo Mondi]

Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
unpack generic properties and their arguments

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/linux/pinctrl/pinconf-generic.h | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/pinctrl/pinconf-generic.h b/include/linux/pinctrl/pinconf-generic.h
index 279e3c5..2cd2a03 100644
--- a/include/linux/pinctrl/pinconf-generic.h
+++ b/include/linux/pinctrl/pinconf-generic.h
@@ -118,7 +118,9 @@ enum pin_config_param {
 /*
  * Helpful configuration macro to be used in tables etc.
  */
-#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
+#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))
+#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
+#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
 
 /*
  * The following inlines stuffs a configuration parameter and data value
-- 
2.7.4

Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[Geert Uytterhoeven]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> unpack generic properties and their arguments
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[Linus Walleij]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:

> Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> unpack generic properties and their arguments
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

(...)

/*
  * Helpful configuration macro to be used in tables etc.

Then this should say "macros" rather than "macro".

> -#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> +#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))

Also adding some extra parantheses I see.

> +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
> +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)

But why.

I have these two static inlines just below your new macros:

static inline enum pin_config_param pinconf_to_config_param(unsigned
long config)
{
        return (enum pin_config_param) (config & 0xffUL);
}

static inline u32 pinconf_to_config_argument(unsigned long config)
{
        return (u32) ((config >> 8) & 0xffffffUL);
}

Why can't you use this in your code instead of macros?

We generally prefer static inlines over macros because they are easier
to read.

Yours,
Linus Walleij

Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[Geert Uytterhoeven]

On Fri, Apr 28, 2017 at 10:16 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
>> +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
>> +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
>
> But why.
>
> I have these two static inlines just below your new macros:
>
> static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
> {
>         return (enum pin_config_param) (config & 0xffUL);
> }
>
> static inline u32 pinconf_to_config_argument(unsigned long config)
> {
>         return (u32) ((config >> 8) & 0xffffffUL);
> }

Cool, need...more...context...in...patches ;-)

> Why can't you use this in your code instead of macros?
>
> We generally prefer static inlines over macros because they are easier
> to read.

Sure.

Thanks for noticing!

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

Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[jmondi]

Hi Linus,

On Fri, Apr 28, 2017 at 10:16:22AM +0200, Linus Walleij wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
>
> > Add PIN_CONF_UNPACK_PARAM and PIN_CONF_UNPACK_ARGS macros useful to
> > unpack generic properties and their arguments
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
>
> (...)
>
> /*
>   * Helpful configuration macro to be used in tables etc.
>
> Then this should say "macros" rather than "macro".
>
> > -#define PIN_CONF_PACKED(p, a) ((a << 8) | ((unsigned long) p & 0xffUL))
> > +#define PIN_CONF_PACKED(p, a) (((a) << 8) | ((unsigned long) (p) & 0xffUL))
>
> Also adding some extra parantheses I see.
>
> > +#define PIN_CONF_UNPACK_PARAM(c) ((c) & 0xffUL)
> > +#define PIN_CONF_UNPACK_ARGS(c) ((c) >> 8)
>
> But why.
>
> I have these two static inlines just below your new macros:
>
> static inline enum pin_config_param pinconf_to_config_param(unsigned
> long config)
> {
>         return (enum pin_config_param) (config & 0xffUL);
> }
>
> static inline u32 pinconf_to_config_argument(unsigned long config)
> {
>         return (u32) ((config >> 8) & 0xffffffUL);
> }
>
> Why can't you use this in your code instead of macros?
>
> We generally prefer static inlines over macros because they are easier
> to read.
>

Right. I haven't noticed them.
I'll drop this patch, sorry for noise

> Yours,
> Linus Walleij

Re: [PATCH v5 02/10] pinctrl: generic: Add macros to unpack properties

[Andy Shevchenko]

On Fri, Apr 28, 2017 at 11:16 AM, Linus Walleij
<linus.walleij@linaro.org> wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:

> But why.
>
> I have these two static inlines just below your new macros:

+1.

> We generally prefer static inlines over macros because they are easier
> to read.

Not only. It adds type checking as well AFAIUC.

-- 
With Best Regards,
Andy Shevchenko

[PATCH v5 03/10] pinctrl: Renesas RZ/A1 pin and gpio controller

[Jacopo Mondi]

Add combined gpio and pin controller driver for Renesas RZ/A1
r7s72100 SoC.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 drivers/pinctrl/Kconfig        |  11 +
 drivers/pinctrl/Makefile       |   1 +
 drivers/pinctrl/pinctrl-rza1.c | 995 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 1007 insertions(+)
 create mode 100644 drivers/pinctrl/pinctrl-rza1.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 8f8c2af..8eb84a9 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -163,6 +163,17 @@ config PINCTRL_ROCKCHIP
 	select GENERIC_IRQ_CHIP
 	select MFD_SYSCON
 
+config PINCTRL_RZA1
+	bool "Renesas RZ/A1 gpio and pinctrl driver"
+	depends on OF
+	depends on ARCH_R7S72100 || COMPILE_TEST
+	select GPIOLIB
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
+	select GENERIC_PINCONF
+	help
+	  This selects pinctrl driver for Renesas RZ/A1 platforms.
+
 config PINCTRL_SINGLE
 	tristate "One-register-per-pin type device tree based pinctrl driver"
 	depends on OF
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index a251f43..0c2328d2 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -24,6 +24,7 @@ obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
 obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
+obj-$(CONFIG_PINCTRL_RZA1)	+= pinctrl-rza1.o
 obj-$(CONFIG_PINCTRL_SINGLE)	+= pinctrl-single.o
 obj-$(CONFIG_PINCTRL_SIRF)	+= sirf/
 obj-$(CONFIG_PINCTRL_SX150X)	+= pinctrl-sx150x.o
diff --git a/drivers/pinctrl/pinctrl-rza1.c b/drivers/pinctrl/pinctrl-rza1.c
new file mode 100644
index 0000000..b0f044d
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-rza1.c
@@ -0,0 +1,995 @@
+/*
+ * Combined GPIO and pin controller support for Renesas RZ/A1 (r7s72100) SoC
+ *
+ * Copyright (C) 2017 Jacopo Mondi
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2. This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+/*
+ * This pincontroller/gpio combined driver support Renesas devices of RZ/A1
+ * family.
+ * This includes SoCs which are sub- or super- sets of this particular line,
+ * as RZ/A1H (r7s721000), RZ/A1M (r7s721010) and RZ/A1L (r7s721020) are.
+ */
+
+#include <linux/bitops.h>
+#include <linux/err.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/ioport.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+#include <linux/slab.h>
+
+#include "core.h"
+#include "devicetree.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define DRIVER_NAME			"pinctrl-rza1"
+
+#define RZA1_P_REG			0x0000
+#define RZA1_PPR_REG			0x0200
+#define RZA1_PM_REG			0x0300
+#define RZA1_PMC_REG			0x0400
+#define RZA1_PFC_REG			0x0500
+#define RZA1_PFCE_REG			0x0600
+#define RZA1_PFCEA_REG			0x0a00
+#define RZA1_PIBC_REG			0x4000
+#define RZA1_PBDC_REG			0x4100
+#define RZA1_PIPC_REG			0x4200
+
+#define RZA1_ADDR(mem, reg, port)	((mem) + (reg) + ((port) * 4))
+
+#define RZA1_NPORTS			12
+#define RZA1_PINS_PER_PORT		16
+#define RZA1_NPINS			(RZA1_PINS_PER_PORT * RZA1_NPORTS)
+#define RZA1_PIN_ID_TO_PORT(id)		((id) / RZA1_PINS_PER_PORT)
+#define RZA1_PIN_ID_TO_PIN(id)		((id) % RZA1_PINS_PER_PORT)
+
+/*
+ * Use 16 lower bits [15:0] for pin identifier
+ * Use 16 higher bits [31:16] for pin mux function
+ */
+#define MUX_PIN_ID_MASK			GENMASK(15, 0)
+#define MUX_FUNC_MASK			GENMASK(31, 16)
+
+#define MUX_FUNC_OFFS			16
+#define MUX_FUNC(pinconf)		\
+	((pinconf & MUX_FUNC_MASK) >> MUX_FUNC_OFFS)
+#define MUX_FUNC_PFC_MASK		BIT(0)
+#define MUX_FUNC_PFCE_MASK		BIT(1)
+#define MUX_FUNC_PFCEA_MASK		BIT(2)
+
+/* Pin mux flags: translated from pinconf standard properties */
+#define MUX_FLAGS_BIDIR			BIT(0)
+#define MUX_FLAGS_SWIO_INPUT		BIT(1)
+#define MUX_FLAGS_SWIO_OUTPUT		BIT(2)
+
+/**
+ * rza1_mux_conf - describes a pin multiplexing operation
+ *
+ * @id: the pin identifier from 0 to RZA1_NPINS
+ * @port: the port where pin sits on
+ * @pin: pin id
+ * @mux_func: alternate function id number
+ * @mux_flags: alternate function flags
+ * @value: output value to set the pin to
+ */
+struct rza1_mux_conf {
+	u16 id;
+	u8 port;
+	u8 pin;
+	u8 mux_func;
+	u8 mux_flags;
+	u8 value;
+};
+
+/**
+ * rza1_port - describes a pin port
+ *
+ * This is mostly useful to lock register writes per-bank and not globally.
+ *
+ * @lock: protect access to HW registers
+ * @id: port number
+ * @base: logical address base
+ * @pins: pins sitting on this port
+ */
+struct rza1_port {
+	spinlock_t lock;
+	unsigned int id;
+	void __iomem *base;
+	struct pinctrl_pin_desc *pins;
+};
+
+/**
+ * rza1_pinctrl - RZ pincontroller device
+ *
+ * @dev: parent device structure
+ * @mutex: protect [pinctrl|pinmux]_generic functions
+ * @base: logical address base
+ * @nports: number of pin controller ports
+ * @ports: pin controller banks
+ * @pins: pin array for pinctrl core
+ * @desc: pincontroller desc for pinctrl core
+ * @pctl: pinctrl device
+ */
+struct rza1_pinctrl {
+	struct device *dev;
+
+	struct mutex mutex;
+
+	void __iomem *base;
+
+	unsigned int nport;
+	struct rza1_port *ports;
+
+	struct pinctrl_pin_desc *pins;
+	struct pinctrl_desc desc;
+	struct pinctrl_dev *pctl;
+};
+
+/* ----------------------------------------------------------------------------
+ * RZ/A1 SoC operations
+ */
+
+/**
+ * rza1_set_bit() - un-locked set/clear a single bit in pin configuration
+ *		    registers
+ */
+static inline void rza1_set_bit(struct rza1_port *port, unsigned int reg,
+				unsigned int bit, bool set)
+{
+	void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+	u16 val = ioread16(mem);
+
+	if (set)
+		val |= BIT(bit);
+	else
+		val &= ~BIT(bit);
+
+	iowrite16(val, mem);
+}
+
+static inline unsigned int rza1_get_bit(struct rza1_port *port,
+					unsigned int reg, unsigned int bit)
+{
+	void __iomem *mem = RZA1_ADDR(port->base, reg, port->id);
+
+	return ioread16(mem) & BIT(bit);
+}
+
+/**
+ * rza1_pin_reset() - reset a pin to default initial state
+ *
+ * Reset pin state disabling input buffer and bi-directional control,
+ * and configure it as input port.
+ * Note that pin is now configured with direction as input but with input
+ * buffer disabled. This implies the pin value cannot be read in this state.
+ *
+ * @port: port where pin sits on
+ * @pin: pin offset
+ */
+static void rza1_pin_reset(struct rza1_port *port, unsigned int pin)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&port->lock, irqflags);
+	rza1_set_bit(port, RZA1_PIBC_REG, pin, 0);
+	rza1_set_bit(port, RZA1_PBDC_REG, pin, 0);
+
+	rza1_set_bit(port, RZA1_PM_REG, pin, 1);
+	rza1_set_bit(port, RZA1_PMC_REG, pin, 0);
+	rza1_set_bit(port, RZA1_PIPC_REG, pin, 0);
+	spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline int rza1_pin_get_direction(struct rza1_port *port,
+					 unsigned int pin)
+{
+	unsigned long irqflags;
+	int input;
+
+	spin_lock_irqsave(&port->lock, irqflags);
+	input = rza1_get_bit(port, RZA1_PM_REG, pin);
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	return !!input;
+}
+
+/**
+ * rza1_pin_set_direction() - set I/O direction on a pin in port mode
+ *
+ * When running in output port mode keep PBDC enabled to allow reading the
+ * pin value from PPR.
+ *
+ * @port: port where pin sits on
+ * @pin: pin offset
+ * @input: input enable/disable flag
+ */
+static inline void rza1_pin_set_direction(struct rza1_port *port,
+					  unsigned int pin, bool input)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&port->lock, irqflags);
+
+	rza1_set_bit(port, RZA1_PIBC_REG, pin, 1);
+	if (input) {
+		rza1_set_bit(port, RZA1_PM_REG, pin, 1);
+		rza1_set_bit(port, RZA1_PBDC_REG, pin, 0);
+	} else {
+		rza1_set_bit(port, RZA1_PM_REG, pin, 0);
+		rza1_set_bit(port, RZA1_PBDC_REG, pin, 1);
+	}
+
+	spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline void rza1_pin_set(struct rza1_port *port, unsigned int pin,
+				unsigned int value)
+{
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&port->lock, irqflags);
+	rza1_set_bit(port, RZA1_P_REG, pin, !!value);
+	spin_unlock_irqrestore(&port->lock, irqflags);
+}
+
+static inline int rza1_pin_get(struct rza1_port *port, unsigned int pin)
+{
+	unsigned long irqflags;
+	int val;
+
+	spin_lock_irqsave(&port->lock, irqflags);
+	val = rza1_get_bit(port, RZA1_PPR_REG, pin);
+	spin_unlock_irqrestore(&port->lock, irqflags);
+
+	return val;
+}
+
+/**
+ * rza1_pin_mux_single() - configure pin multiplexing on a single pin
+ *
+ * @pinctrl: RZ/A1 pin controller device
+ * @mux_conf: pin multiplexing descriptor
+ */
+static int rza1_pin_mux_single(struct rza1_pinctrl *rza1_pctl,
+			       struct rza1_mux_conf *mux_conf)
+{
+	struct rza1_port *port = &rza1_pctl->ports[mux_conf->port];
+	unsigned int pin = mux_conf->pin;
+	u8 mux_func = mux_conf->mux_func - 1;
+	u8 mux_flags = mux_conf->mux_flags;
+
+	rza1_pin_reset(port, pin);
+
+	if (mux_flags & MUX_FLAGS_BIDIR)
+		rza1_set_bit(port, RZA1_PBDC_REG, pin, 1);
+
+	/*
+	 * Enable alternate function mode and select it.
+	 *
+	 * Be careful here: the pin mux sub-nodes in device tree
+	 * enumerate alternate functions from 1 to 8;
+	 * subtract 1 before using macros to match registers configuration
+	 * which expects numbers from 0 to 7 instead.
+	 *
+	 * ----------------------------------------------------
+	 * Alternate mode selection table:
+	 *
+	 * PMC	PFC	PFCE	PFCAE	(mux_func - 1)
+	 * 1	0	0	0	0
+	 * 1	1	0	0	1
+	 * 1	0	1	0	2
+	 * 1	1	1	0	3
+	 * 1	0	0	1	4
+	 * 1	1	0	1	5
+	 * 1	0	1	1	6
+	 * 1	1	1	1	7
+	 * ----------------------------------------------------
+	 */
+	rza1_set_bit(port, RZA1_PFC_REG, pin, mux_func & MUX_FUNC_PFC_MASK);
+	rza1_set_bit(port, RZA1_PFCE_REG, pin, mux_func & MUX_FUNC_PFCE_MASK);
+	rza1_set_bit(port, RZA1_PFCEA_REG, pin, mux_func & MUX_FUNC_PFCEA_MASK);
+
+	/*
+	 * All alternate functions except a few need PIPCn = 1.
+	 * If PIPCn has to stay disabled (SW IO mode), configure PMn according
+	 * to I/O direction specified by pin configuration -after- PMC has been
+	 * set to one.
+	 */
+	if (!(mux_flags & (MUX_FLAGS_SWIO_INPUT | MUX_FLAGS_SWIO_OUTPUT)))
+		rza1_set_bit(port, RZA1_PIPC_REG, pin, 1);
+
+	rza1_set_bit(port, RZA1_PMC_REG, pin, 1);
+	rza1_set_bit(port, RZA1_PM_REG, pin, mux_flags & MUX_FLAGS_SWIO_INPUT);
+
+	return 0;
+}
+
+/* ----------------------------------------------------------------------------
+ * gpio operations
+ */
+
+/**
+ * rza1_gpio_request() - configure pin in port mode
+ *
+ * Configure a pin as gpio (port mode).
+ * After reset, the pin is in input mode with input buffer disabled.
+ * To use the pin as input or output, set_direction shall be called first
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static int rza1_gpio_request(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	rza1_pin_reset(port, gpio);
+
+	return 0;
+}
+
+/**
+ * rza1_gpio_disable_free() - reset a pin
+ *
+ * Surprisingly, disable_free a gpio, is equivalent to request it.
+ * Reset pin to port mode, with input buffer disabled. This overwrites all
+ * port direction settings applied with set_direction
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static void rza1_gpio_free(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	rza1_pin_reset(port, gpio);
+}
+
+static int rza1_gpio_get_direction(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	return rza1_pin_get_direction(port, gpio);
+}
+
+static int rza1_gpio_direction_input(struct gpio_chip *chip,
+				     unsigned int gpio)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	rza1_pin_set_direction(port, gpio, true);
+
+	return 0;
+}
+
+static int rza1_gpio_direction_output(struct gpio_chip *chip,
+				      unsigned int gpio,
+				      int value)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	/* Set value before driving pin direction */
+	rza1_pin_set(port, gpio, value);
+	rza1_pin_set_direction(port, gpio, false);
+
+	return 0;
+}
+
+/**
+ * rza1_gpio_get() - read a gpio pin value
+ *
+ * Read gpio pin value through PPR register.
+ * Requires bi-directional mode to work when reading the value of a pin
+ * in output mode
+ *
+ * @chip: gpio chip where the gpio sits on
+ * @gpio: gpio offset
+ */
+static int rza1_gpio_get(struct gpio_chip *chip, unsigned int gpio)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	return rza1_pin_get(port, gpio);
+}
+
+static void rza1_gpio_set(struct gpio_chip *chip, unsigned int gpio,
+			  int value)
+{
+	struct rza1_port *port = gpiochip_get_data(chip);
+
+	rza1_pin_set(port, gpio, value);
+}
+
+struct gpio_chip rza1_gpiochip_template = {
+	.request		= rza1_gpio_request,
+	.free			= rza1_gpio_free,
+	.get_direction		= rza1_gpio_get_direction,
+	.direction_input	= rza1_gpio_direction_input,
+	.direction_output	= rza1_gpio_direction_output,
+	.get			= rza1_gpio_get,
+	.set			= rza1_gpio_set,
+};
+/* ----------------------------------------------------------------------------
+ * pinctrl operations
+ */
+
+/**
+ * rza1_dt_node_pin_count() - Count number of pins in a dt node or in all its
+ *			      children sub-nodes
+ *
+ * @np: device tree node to parse
+ */
+static int rza1_dt_node_pin_count(struct device_node *np)
+{
+	struct device_node *child;
+	struct property *of_pins;
+	unsigned int npins;
+
+	of_pins = of_find_property(np, "pinmux", NULL);
+	if (of_pins)
+		return of_pins->length / sizeof(u32);
+
+	npins = 0;
+	for_each_child_of_node(np, child) {
+		of_pins = of_find_property(child, "pinmux", NULL);
+		if (!of_pins)
+			return -EINVAL;
+
+		npins += of_pins->length / sizeof(u32);
+	}
+
+	return npins;
+}
+
+/**
+ * rza1_parse_pmx_function() - parse a pin mux sub-node
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ * @np: of pmx sub-node
+ * @mux_confs: array of pin mux configurations to fill with parsed info
+ * @grpins: array of pin ids to mux
+ */
+static int rza1_parse_pinmux_node(struct rza1_pinctrl *rza1_pctl,
+				  struct device_node *np,
+				  struct rza1_mux_conf *mux_confs,
+				  unsigned int *grpins)
+{
+	struct pinctrl_dev *pctldev = rza1_pctl->pctl;
+	char const *prop_name = "pinmux";
+	unsigned long *pin_configs;
+	unsigned int npin_configs;
+	struct property *of_pins;
+	unsigned int npins;
+	u8 pinmux_flags;
+	unsigned int i;
+	int ret;
+
+	of_pins = of_find_property(np, prop_name, NULL);
+	if (!of_pins) {
+		dev_err(rza1_pctl->dev, "Missing %s property\n", prop_name);
+		return -ENOENT;
+	}
+	npins = of_pins->length / sizeof(u32);
+
+	/*
+	 * Collect pin configuration properties: they apply to all pins in
+	 * this sub-node
+	 */
+	ret = pinconf_generic_parse_dt_config(np, pctldev, &pin_configs,
+					      &npin_configs);
+	if (ret) {
+		dev_err(rza1_pctl->dev,
+			"Unable to parse pin configuration options for %s\n",
+			np->name);
+		return ret;
+	}
+
+	/*
+	 * Create a mask with pinmux flags from pin configuration;
+	 * only allow a single flag to be set (the first encountered one)
+	 */
+	pinmux_flags = 0;
+	for (i = 0; i < npin_configs && pinmux_flags == 0; i++)
+		switch (PIN_CONF_UNPACK_PARAM(pin_configs[i])) {
+		case PIN_CONFIG_BIDIRECTIONAL:
+			pinmux_flags |= MUX_FLAGS_BIDIR;
+			break;
+		case PIN_CONFIG_INPUT_ENABLE:
+			pinmux_flags |= MUX_FLAGS_SWIO_INPUT;
+			break;
+		case PIN_CONFIG_OUTPUT:
+			pinmux_flags |= MUX_FLAGS_SWIO_OUTPUT;
+			break;
+		}
+
+	kfree(pin_configs);
+
+	/* Collect pin positions and their mux settings. */
+	for (i = 0; i < npins; ++i) {
+		u32 of_pinconf;
+		struct rza1_mux_conf *mux_conf = &mux_confs[i];
+
+		ret = of_property_read_u32_index(np, prop_name, i, &of_pinconf);
+		if (ret)
+			return ret;
+
+		mux_conf->id		= of_pinconf & MUX_PIN_ID_MASK;
+		mux_conf->port		= RZA1_PIN_ID_TO_PORT(mux_conf->id);
+		mux_conf->pin		= RZA1_PIN_ID_TO_PIN(mux_conf->id);
+		mux_conf->mux_func	= MUX_FUNC(of_pinconf);
+		mux_conf->mux_flags	= pinmux_flags;
+
+		if (mux_conf->port >= RZA1_NPORTS ||
+		    mux_conf->pin >= RZA1_PINS_PER_PORT) {
+			dev_err(rza1_pctl->dev,
+				"Wrong port %u pin %u for %s property\n",
+				mux_conf->port, mux_conf->pin, prop_name);
+			return -EINVAL;
+		}
+
+		grpins[i] = mux_conf->id;
+	}
+
+	return npins;
+}
+
+/**
+ * rza1_dt_node_to_map() - map a pin mux node to a function/group
+ *
+ * Parse and register a pin mux function.
+ *
+ * @pctldev: pin controller device
+ * @np: device tree node to parse
+ * @map: pointer to pin map (output)
+ * @num_maps: number of collected maps (output)
+ */
+static int rza1_dt_node_to_map(struct pinctrl_dev *pctldev,
+			       struct device_node *np,
+			       struct pinctrl_map **map,
+			       unsigned int *num_maps)
+{
+	struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rza1_mux_conf *mux_confs, *mux_conf;
+	unsigned int *grpins, *grpin;
+	struct device_node *child;
+	const char *grpname;
+	const char **fngrps;
+	int ret, npins;
+
+	npins = rza1_dt_node_pin_count(np);
+	if (npins < 0) {
+		dev_err(rza1_pctl->dev, "invalid pinmux node structure\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Functions are made of 1 group only;
+	 * in fact, functions and groups are identical for this pin controller
+	 * except that functions carry an array of per-pin mux configuration
+	 * settings.
+	 */
+	mux_confs = devm_kcalloc(rza1_pctl->dev, npins, sizeof(*mux_confs),
+				 GFP_KERNEL);
+	grpins = devm_kcalloc(rza1_pctl->dev, npins, sizeof(*grpins),
+			      GFP_KERNEL);
+	fngrps = devm_kzalloc(rza1_pctl->dev, sizeof(*fngrps), GFP_KERNEL);
+
+	if (!mux_confs || !grpins || !fngrps)
+		return -ENOMEM;
+
+	/*
+	 * Parse the pinmux node.
+	 * If the node does not contain "pinmux" property (-ENOENT)
+	 * that property shall be specified in all its children sub-nodes.
+	 */
+	mux_conf = &mux_confs[0];
+	grpin = &grpins[0];
+
+	ret = rza1_parse_pinmux_node(rza1_pctl, np, mux_conf, grpin);
+	if (ret == -ENOENT)
+		for_each_child_of_node(np, child) {
+			ret = rza1_parse_pinmux_node(rza1_pctl, child, mux_conf,
+						     grpin);
+			if (ret < 0)
+				return ret;
+
+			grpin += ret;
+			mux_conf += ret;
+		}
+	else if (ret < 0)
+		return ret;
+
+	/* Register pin group and function name to pinctrl_generic */
+	grpname	= np->name;
+	fngrps[0] = grpname;
+
+	mutex_lock(&rza1_pctl->mutex);
+	ret = pinctrl_generic_add_group(pctldev, grpname, grpins, npins,
+					NULL);
+	if (ret) {
+		mutex_unlock(&rza1_pctl->mutex);
+		return ret;
+	}
+
+	ret = pinmux_generic_add_function(pctldev, grpname, fngrps, 1,
+					  mux_confs);
+	if (ret)
+		goto remove_group;
+	mutex_unlock(&rza1_pctl->mutex);
+
+	dev_info(rza1_pctl->dev, "Parsed function and group %s with %d pins\n",
+				 grpname, npins);
+
+	/* Create map where to retrieve function and mux settings from */
+	*num_maps = 0;
+	*map = kzalloc(sizeof(**map), GFP_KERNEL);
+	if (!*map) {
+		ret = -ENOMEM;
+		goto remove_function;
+	}
+
+	(*map)->type = PIN_MAP_TYPE_MUX_GROUP;
+	(*map)->data.mux.group = np->name;
+	(*map)->data.mux.function = np->name;
+	*num_maps = 1;
+
+	return 0;
+
+remove_function:
+	mutex_lock(&rza1_pctl->mutex);
+	pinmux_generic_remove_last_function(pctldev);
+
+remove_group:
+	pinctrl_generic_remove_last_group(pctldev);
+	mutex_unlock(&rza1_pctl->mutex);
+
+	dev_info(rza1_pctl->dev, "Unable to parse function and group %s\n",
+				 grpname);
+
+	return ret;
+}
+
+static void rza1_dt_free_map(struct pinctrl_dev *pctldev,
+			     struct pinctrl_map *map, unsigned int num_maps)
+{
+	kfree(map);
+}
+
+static const struct pinctrl_ops rza1_pinctrl_ops = {
+	.get_groups_count	= pinctrl_generic_get_group_count,
+	.get_group_name		= pinctrl_generic_get_group_name,
+	.get_group_pins		= pinctrl_generic_get_group_pins,
+	.dt_node_to_map		= rza1_dt_node_to_map,
+	.dt_free_map		= rza1_dt_free_map,
+};
+
+/* ----------------------------------------------------------------------------
+ * pinmux operations
+ */
+
+/**
+ * rza1_set_mux() - retrieve pins from a group and apply their mux settings
+ *
+ * @pctldev: pin controller device
+ * @selector: function selector
+ * @group: group selector
+ */
+static int rza1_set_mux(struct pinctrl_dev *pctldev, unsigned int selector,
+			   unsigned int group)
+{
+	struct rza1_pinctrl *rza1_pctl = pinctrl_dev_get_drvdata(pctldev);
+	struct rza1_mux_conf *mux_confs;
+	struct function_desc *func;
+	struct group_desc *grp;
+	int i;
+
+	grp = pinctrl_generic_get_group(pctldev, group);
+	if (!grp)
+		return -EINVAL;
+
+	func = pinmux_generic_get_function(pctldev, selector);
+	if (!func)
+		return -EINVAL;
+
+	mux_confs = (struct rza1_mux_conf *)func->data;
+	for (i = 0; i < grp->num_pins; ++i) {
+		int ret;
+
+		ret = rza1_pin_mux_single(rza1_pctl, &mux_confs[i]);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+
+struct pinmux_ops rza1_pinmux_ops = {
+	.get_functions_count	= pinmux_generic_get_function_count,
+	.get_function_name	= pinmux_generic_get_function_name,
+	.get_function_groups	= pinmux_generic_get_function_groups,
+	.set_mux		= rza1_set_mux,
+	.strict			= true,
+};
+
+/* ----------------------------------------------------------------------------
+ * RZ/A1 pin controller driver operations
+ */
+
+static unsigned int rza1_count_gpio_chips(struct device_node *np)
+{
+	struct device_node *child;
+	unsigned int count = 0;
+
+	for_each_child_of_node(np, child) {
+		if (!of_property_read_bool(child, "gpio-controller"))
+			continue;
+
+		count++;
+	}
+
+	return count;
+}
+
+/**
+ * rza1_parse_gpiochip() - parse and register a gpio chip and pin range
+ *
+ * The gpio controller subnode shall provide a "gpio-ranges" list property as
+ * defined by gpio device tree binding documentation.
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ * @np: of gpio-controller node
+ * @chip: gpio chip to register to gpiolib
+ * @range: pin range to register to pinctrl core
+ */
+static int rza1_parse_gpiochip(struct rza1_pinctrl *rza1_pctl,
+			       struct device_node *np,
+			       struct gpio_chip *chip,
+			       struct pinctrl_gpio_range *range)
+{
+	const char *list_name = "gpio-ranges";
+	struct of_phandle_args of_args;
+	unsigned int gpioport;
+	u32 pinctrl_base;
+	int ret;
+
+	ret = of_parse_phandle_with_fixed_args(np, list_name, 3, 0, &of_args);
+	if (ret) {
+		dev_err(rza1_pctl->dev, "Unable to parse %s list property\n",
+			list_name);
+		return ret;
+	}
+
+	/*
+	 * Find out on which port this gpio-chip maps to by inspecting the
+	 * second argument of the "gpio-ranges" property.
+	 */
+	pinctrl_base = of_args.args[1];
+	gpioport = RZA1_PIN_ID_TO_PORT(pinctrl_base);
+	if (gpioport > RZA1_NPORTS) {
+		dev_err(rza1_pctl->dev,
+			"Invalid values in property %s\n", list_name);
+		return -EINVAL;
+	}
+
+	*chip		= rza1_gpiochip_template;
+	chip->base	= -1;
+	chip->label	= devm_kasprintf(rza1_pctl->dev, GFP_KERNEL, "%s-%u",
+					 np->name, gpioport);
+	chip->ngpio	= of_args.args[2];
+	chip->of_node	= np;
+	chip->parent	= rza1_pctl->dev;
+
+	range->id	= gpioport;
+	range->name	= chip->label;
+	range->pin_base	= range->base = pinctrl_base;
+	range->npins	= of_args.args[2];
+	range->gc	= chip;
+
+	ret = devm_gpiochip_add_data(rza1_pctl->dev, chip,
+				     &rza1_pctl->ports[gpioport]);
+	if (ret)
+		return ret;
+
+	pinctrl_add_gpio_range(rza1_pctl->pctl, range);
+
+	dev_info(rza1_pctl->dev, "Parsed gpiochip %s with %d pins\n",
+		 chip->label, chip->ngpio);
+
+	return 0;
+}
+
+/**
+ * rza1_gpio_register() - parse DT to collect gpio-chips and gpio-ranges
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_gpio_register(struct rza1_pinctrl *rza1_pctl)
+{
+	struct device_node *np = rza1_pctl->dev->of_node;
+	struct pinctrl_gpio_range *gpio_ranges;
+	struct gpio_chip *gpio_chips;
+	struct device_node *child;
+	unsigned int ngpiochips;
+	unsigned int i;
+	int ret;
+
+	ngpiochips = rza1_count_gpio_chips(np);
+	if (ngpiochips == 0) {
+		dev_dbg(rza1_pctl->dev, "No gpiochip registered\n");
+		return 0;
+	}
+
+	gpio_chips = devm_kcalloc(rza1_pctl->dev, ngpiochips,
+				  sizeof(*gpio_chips), GFP_KERNEL);
+	gpio_ranges = devm_kcalloc(rza1_pctl->dev, ngpiochips,
+				   sizeof(*gpio_ranges), GFP_KERNEL);
+	if (!gpio_chips || !gpio_ranges)
+		return -ENOMEM;
+
+	i = 0;
+	for_each_child_of_node(np, child) {
+		if (!of_property_read_bool(child, "gpio-controller"))
+			continue;
+
+		ret = rza1_parse_gpiochip(rza1_pctl, child, &gpio_chips[i],
+					  &gpio_ranges[i]);
+		if (ret)
+			goto gpiochip_remove;
+
+		++i;
+	}
+
+	dev_info(rza1_pctl->dev, "Registered %u gpio controllers\n", i);
+
+	return 0;
+
+gpiochip_remove:
+	for (; i > 0; i--)
+		devm_gpiochip_remove(rza1_pctl->dev, &gpio_chips[i - 1]);
+
+	return ret;
+}
+
+/**
+ * rza1_pinctrl_register() - Enumerate pins, ports and gpiochips; register
+ *			     them to pinctrl and gpio cores.
+ *
+ * @rza1_pctl: RZ/A1 pin controller device
+ */
+static int rza1_pinctrl_register(struct rza1_pinctrl *rza1_pctl)
+{
+	struct pinctrl_pin_desc *pins;
+	struct rza1_port *ports;
+	unsigned int i;
+	int ret;
+
+	pins = devm_kcalloc(rza1_pctl->dev, RZA1_NPINS, sizeof(*pins),
+			    GFP_KERNEL);
+	ports = devm_kcalloc(rza1_pctl->dev, RZA1_NPORTS, sizeof(*ports),
+			     GFP_KERNEL);
+	if (!pins || !ports)
+		return -ENOMEM;
+
+	rza1_pctl->pins		= pins;
+	rza1_pctl->desc.pins	= pins;
+	rza1_pctl->desc.npins	= RZA1_NPINS;
+	rza1_pctl->ports	= ports;
+
+	for (i = 0; i < RZA1_NPINS; ++i) {
+		unsigned int pin = RZA1_PIN_ID_TO_PIN(i);
+		unsigned int port = RZA1_PIN_ID_TO_PORT(i);
+
+		pins[i].number = i;
+		pins[i].name = devm_kasprintf(rza1_pctl->dev, GFP_KERNEL,
+					      "P%u-%u", port, pin);
+
+		if (i % RZA1_PINS_PER_PORT == 0) {
+			/*
+			 * Setup ports;
+			 * they provide per-port lock and logical base address.
+			 */
+			unsigned int port_id = RZA1_PIN_ID_TO_PORT(i);
+
+			ports[port_id].id	= port_id;
+			ports[port_id].base	= rza1_pctl->base;
+			ports[port_id].pins	= &pins[i];
+			spin_lock_init(&ports[port_id].lock);
+		}
+	}
+
+	ret = devm_pinctrl_register_and_init(rza1_pctl->dev, &rza1_pctl->desc,
+					     rza1_pctl, &rza1_pctl->pctl);
+	if (ret) {
+		dev_err(rza1_pctl->dev,
+			"RZ/A1 pin controller registration failed\n");
+		return ret;
+	}
+
+	ret = pinctrl_enable(rza1_pctl->pctl);
+	if (ret) {
+		dev_err(rza1_pctl->dev,
+			"RZ/A1 pin controller failed to start\n");
+		return ret;
+	}
+
+	ret = rza1_gpio_register(rza1_pctl);
+	if (ret) {
+		dev_err(rza1_pctl->dev, "RZ/A1 GPIO registration failed\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static int rza1_pinctrl_probe(struct platform_device *pdev)
+{
+	struct rza1_pinctrl *rza1_pctl;
+	struct resource *res;
+	int ret;
+
+	rza1_pctl = devm_kzalloc(&pdev->dev, sizeof(*rza1_pctl), GFP_KERNEL);
+	if (!rza1_pctl)
+		return -ENOMEM;
+
+	rza1_pctl->dev = &pdev->dev;
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (ret)
+		return -ENODEV;
+
+	rza1_pctl->base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(rza1_pctl->base))
+		return PTR_ERR(rza1_pctl->base);
+
+	mutex_init(&rza1_pctl->mutex);
+
+	platform_set_drvdata(pdev, rza1_pctl);
+
+	rza1_pctl->desc.name	= DRIVER_NAME;
+	rza1_pctl->desc.pctlops	= &rza1_pinctrl_ops;
+	rza1_pctl->desc.pmxops	= &rza1_pinmux_ops;
+	rza1_pctl->desc.owner	= THIS_MODULE;
+
+	ret = rza1_pinctrl_register(rza1_pctl);
+	if (ret)
+		return ret;
+
+	dev_info(&pdev->dev,
+		 "RZ/A1 pin controller and gpio successfully registered\n");
+
+	return 0;
+}
+
+static const struct of_device_id rza1_pinctrl_of_match[] = {
+	{ .compatible = "renesas,r7s72100-ports", },
+	{ }
+};
+
+static struct platform_driver rza1_pinctrl_driver = {
+	.driver = {
+		.name = DRIVER_NAME,
+		.of_match_table = rza1_pinctrl_of_match,
+	},
+	.probe = rza1_pinctrl_probe,
+};
+
+static int __init rza1_pinctrl_init(void)
+{
+	return platform_driver_register(&rza1_pinctrl_driver);
+}
+core_initcall(rza1_pinctrl_init);
+
+MODULE_AUTHOR("Jacopo Mondi <jacopo+renesas@jmondi.org");
+MODULE_DESCRIPTION("Pin and gpio controller driver for Reneas RZ/A1 SoC");
+MODULE_LICENSE("GPL v2");
-- 
2.7.4

Re: [PATCH v5 03/10] pinctrl: Renesas RZ/A1 pin and gpio controller

[Geert Uytterhoeven]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add combined gpio and pin controller driver for Renesas RZ/A1
> r7s72100 SoC.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

[PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Jacopo Mondi]

Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
controller.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 .../bindings/pinctrl/renesas,rza1-pinctrl.txt      | 219 +++++++++++++++++++++
 1 file changed, 219 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt

diff --git a/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
new file mode 100644
index 0000000..5a1106d
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt
@@ -0,0 +1,219 @@
+Renesas RZ/A1 combined Pin and GPIO controller
+
+The Renesas SoCs of the RZ/A1 family feature a combined Pin and GPIO controller,
+named "Ports" in the hardware reference manual.
+Pin multiplexing and GPIO configuration is performed on a per-pin basis
+writing configuration values to per-port register sets.
+Each "port" features up to 16 pins, each of them configurable for GPIO
+function (port mode) or in alternate function mode.
+Up to 8 different alternate function modes exist for each single pin.
+
+Pin controller node
+-------------------
+
+Required properties:
+  - compatible
+    this shall be "renesas,r7s72100-ports".
+
+  - reg
+    address base and length of the memory area where the pin controller
+    hardware is mapped to.
+
+Example:
+Pin controller node for RZ/A1H SoC (r7s72100)
+
+pinctrl: pin-controller@fcfe3000 {
+	compatible = "renesas,r7s72100-ports";
+
+	reg = <0xfcfe3000 0x4230>;
+};
+
+Sub-nodes
+---------
+
+The child nodes of the pin controller node describe a pin multiplexing
+function or a GPIO controller alternatively.
+
+- Pin multiplexing sub-nodes:
+  A pin multiplexing sub-node describes how to configure a set of
+  (or a single) pin in some desired alternate function mode.
+  A single sub-node may define several pin configurations.
+  Some alternate functions require special pin configuration flags to be
+  supplied along with the alternate function configuration number.
+  When the hardware reference manual specifies a pin function to be either
+  "bi-directional" or "software IO driven", use the generic properties from
+  the <include/linux/pinctrl/pinconf_generic.h> header file to instruct the
+  pin controller to perform the desired pin configuration operations.
+  Please refer to pinctrl-bindings.txt to get to know more on generic
+  pin properties usage.
+
+  The allowed generic formats for a pin multiplexing sub-node are the
+  following ones:
+
+  node-1 {
+      pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+      GENERIC_PINCONFIG;
+  };
+
+  node-2 {
+      sub-node-1 {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+
+      sub-node-2 {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+
+      ...
+
+      sub-node-n {
+          pinmux = <PIN_ID_AND_MUX>, <PIN_ID_AND_MUX>, ... ;
+          GENERIC_PINCONFIG;
+      };
+  };
+
+  Use the second format when pins part of the same logical group need to have
+  different generic pin configuration flags applied.
+
+  Client sub-nodes shall refer to pin multiplexing sub-nodes using the phandle
+  of the most external one.
+
+  Eg.
+
+  client-1 {
+      ...
+      pinctrl-0 = <&node-1>;
+      ...
+  };
+
+  client-2 {
+      ...
+      pinctrl-0 = <&node-2>;
+      ...
+  };
+
+  Required properties:
+    - pinmux:
+      integer array representing pin number and pin multiplexing configuration.
+      When a pin has to be configured in alternate function mode, use this
+      property to identify the pin by its global index, and provide its
+      alternate function configuration number along with it.
+      When multiple pins are required to be configured as part of the same
+      alternate function they shall be specified as members of the same
+      argument list of a single "pinmux" property.
+      Helper macros to ease assembling the pin index from its position
+      (port where it sits on and pin number) and alternate function identifier
+      are provided by the pin controller header file at:
+      <include/dt-bindings/pinctrl/r7s72100-pinctrl.h>
+      Integers values in "pinmux" argument list are assembled as:
+      ((PORT * 16 + PIN) | MUX_FUNC << 16)
+
+  Optional generic properties:
+    - bi-directional:
+      for pins requiring bi-directional operations.
+    - input-enable:
+      for pins requiring software driven IO input operations.
+    - output-enable:
+      for pins requiring software driven IO output operations.
+
+  The hardware reference manual specifies when a pin has to be configured to
+  work in bi-directional mode and when the IO direction has to be specified
+  by software.
+
+  Example:
+  A serial communication interface with a TX output pin and an RX input pin.
+
+  &pinctrl {
+	scif2_pins: serial2 {
+		pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
+	};
+  };
+
+  Pin #0 on port #3 is configured as alternate function #6.
+  Pin #2 on port #3 is configured as alternate function #4.
+
+  Example 2:
+  I2c master: both SDA and SCL pins need bi-directional operations
+
+  &pinctrl {
+	i2c2_pins: i2c2 {
+		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
+		bi-directional;
+	};
+  };
+
+  Pin #4 on port #1 is configured as alternate function #1.
+  Pin #5 on port #1 is configured as alternate function #1.
+  Both need to work in bi-directional mode.
+
+  Example 3:
+  Multi-function timer input and output compare pins.
+  Configure TIOC0A as software driven input and TIOC0B as software driven
+  output.
+
+  &pinctrl {
+	tioc0_pins: tioc0 {
+		tioc0_input_pins {
+			pinumx = <RZA1_PINMUX(4, 0, 2)>;
+			input-enable;
+		};
+
+		tioc0_output_pins {
+			pinmux = <RZA1_PINMUX(4, 1, 1)>;
+			output-enable;
+		};
+	};
+  };
+
+
+  &tioc0 {
+	...
+	pinctrl-0 = <&tioc0_pins>;
+	...
+  };
+
+  Pin #0 on port #4 is configured as alternate function #2 with IO direction
+  specified by software as input.
+  Pin #1 on port #4 is configured as alternate function #1 with IO direction
+  specified by software as output.
+
+- GPIO controller sub-nodes:
+  Each port of the r7s72100 pin controller hardware is itself a GPIO controller.
+  Different SoCs have different numbers of available pins per port, but
+  generally speaking, each of them can be configured in GPIO ("port") mode
+  on this hardware.
+  Describe GPIO controllers using sub-nodes with the following properties.
+
+  Required properties:
+    - gpio-controller
+      empty property as defined by the GPIO bindings documentation.
+    - #gpio-cells
+      number of cells required to identify and configure a GPIO.
+      Shall be 2.
+    - gpio-ranges
+      Describes a GPIO controller specifying its specific pin base, the pin
+      base in the global pin numbering space, and the number of controlled
+      pins, as defined by the GPIO bindings documentation. Refer to
+      Documentation/devicetree/bindings/gpio/gpio.txt file for a more detailed
+      description.
+
+  Example:
+  A GPIO controller node, controlling 16 pins indexed from 0.
+  The GPIO controller base in the global pin indexing space is pin 48, thus
+  pins [0 - 15] on this controller map to pins [48 - 63] in the global pin
+  indexing space.
+
+  port3: gpio-3 {
+	gpio-controller;
+	#gpio-cells = <2>;
+	gpio-ranges = <&pinctrl 0 48 16>;
+  };
+
+  A device node willing to use pins controlled by this GPIO controller, shall
+  refer to it as follows:
+
+  led1 {
+	gpios = <&port3 10 GPIO_ACTIVE_LOW>;
+  };
-- 
2.7.4

Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Geert Uytterhoeven]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> controller.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [PATCH v5 04/10] dt-bindings: pinctrl: Add RZ/A1 bindings doc

[Rob Herring]

On Thu, Apr 27, 2017 at 10:19:48AM +0200, Jacopo Mondi wrote:
> Add device tree bindings documentation for Renesas RZ/A1 gpio and pin
> controller.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> ---
>  .../bindings/pinctrl/renesas,rza1-pinctrl.txt      | 219 +++++++++++++++++++++
>  1 file changed, 219 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/pinctrl/renesas,rza1-pinctrl.txt

Acked-by: Rob Herring <robh@kernel.org>

[PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

[Jacopo Mondi]

Add dt-bindings for Renesas r7s72100 pin controller header file.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 include/dt-bindings/pinctrl/r7s72100-pinctrl.h | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)
 create mode 100644 include/dt-bindings/pinctrl/r7s72100-pinctrl.h

diff --git a/include/dt-bindings/pinctrl/r7s72100-pinctrl.h b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
new file mode 100644
index 0000000..6b609fe
--- /dev/null
+++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
@@ -0,0 +1,16 @@
+/*
+ * Defines macros and constants for Renesas RZ/A1 pin controller pin
+ * muxing functions.
+ */
+#ifndef __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H
+#define __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H
+
+#define RZA1_PINS_PER_PORT	16
+
+/*
+ * Create the pin index from its bank and position numbers and store in
+ * the upper 16 bits the alternate function identifier
+ */
+#define RZA1_PINMUX(b, p, f)	((b) * RZA1_PINS_PER_PORT + (p) | (f << 16))
+
+#endif /* __DT_BINDINGS_PINCTRL_RENESAS_RZA1_H */
-- 
2.7.4

Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

[Geert Uytterhoeven]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add dt-bindings for Renesas r7s72100 pin controller header file.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- /dev/null
> +++ b/include/dt-bindings/pinctrl/r7s72100-pinctrl.h
> @@ -0,0 +1,16 @@

> +#define RZA1_PINMUX(b, p, f)   ((b) * RZA1_PINS_PER_PORT + (p) | (f << 16))

... | (f) << 16)

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

Re: [PATCH v5 05/10] arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header

[Simon Horman]

On Thu, Apr 27, 2017 at 10:38:39AM +0200, Geert Uytterhoeven wrote:
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
> > Add dt-bindings for Renesas r7s72100 pin controller header file.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

Thanks, I have queued this up.

[PATCH v5 06/10] arm: dts: r7s72100: Add pin controller node

[Jacopo Mondi]

Add pin controller node with 12 gpio controller sub-nodes to
r7s72100 dtsi.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r7s72100.dtsi | 78 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100.dtsi b/arch/arm/boot/dts/r7s72100.dtsi
index b8aa256..9342ff4 100644
--- a/arch/arm/boot/dts/r7s72100.dtsi
+++ b/arch/arm/boot/dts/r7s72100.dtsi
@@ -180,6 +180,84 @@
 		};
 	};
 
+	pinctrl: pin-controller@fcfe3000 {
+		compatible = "renesas,r7s72100-ports";
+
+		reg = <0xfcfe3000 0x4230>;
+
+		port0: gpio-0 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 0 6>;
+		};
+
+		port1: gpio-1 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 16 16>;
+		};
+
+		port2: gpio-2 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 32 16>;
+		};
+
+		port3: gpio-3 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 48 16>;
+		};
+
+		port4: gpio-4 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 64 16>;
+		};
+
+		port5: gpio-5 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 80 11>;
+		};
+
+		port6: gpio-6 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 96 16>;
+		};
+
+		port7: gpio-7 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 112 16>;
+		};
+
+		port8: gpio-8 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 128 16>;
+		};
+
+		port9: gpio-9 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 144 8>;
+		};
+
+		port10: gpio-10 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 160 16>;
+		};
+
+		port11: gpio-11 {
+			gpio-controller;
+			#gpio-cells = <2>;
+			gpio-ranges = <&pinctrl 0 176 16>;
+		};
+	};
+
 	scif0: serial@e8007000 {
 		compatible = "renesas,scif-r7s72100", "renesas,scif";
 		reg = <0xe8007000 64>;
-- 
2.7.4

[PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group

[Jacopo Mondi]

Add pin configuration subnode for SCIF2 serial debug interface.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 118a8e2..c28d74b 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -11,6 +11,7 @@
 
 /dts-v1/;
 #include "r7s72100.dtsi"
+#include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
 
 / {
 	model = "Genmai";
@@ -36,6 +37,14 @@
 	};
 };
 
+&pinctrl {
+
+	scif2_pins: serial2 {
+		/* P3_0 as TxD2; P3_2 as RxD2 */
+		pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
+	};
+};
+
 &extal_clk {
 	clock-frequency = <13330000>;
 };
@@ -60,6 +69,9 @@
 };
 
 &scif2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&scif2_pins>;
+
 	status = "okay";
 };
 
-- 
2.7.4

Re: [PATCH v5 07/10] arm: dts: genmai: Add SCIF2 pin group

[Simon Horman]

On Thu, Apr 27, 2017 at 10:19:51AM +0200, Jacopo Mondi wrote:
> Add pin configuration subnode for SCIF2 serial debug interface.
> 
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

As the dt-bindings (documentation) has been acked by Geert I'd be happy
to queue up this and other "arm: dts: genmai" DT patches in this series
which do not have any outstanding review comments. Is it safe to do so
without the PFC driver patches in place?

> ---
>  arch/arm/boot/dts/r7s72100-genmai.dts | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
> index 118a8e2..c28d74b 100644
> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts
> @@ -11,6 +11,7 @@
>  
>  /dts-v1/;
>  #include "r7s72100.dtsi"
> +#include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
>  
>  / {
>  	model = "Genmai";
> @@ -36,6 +37,14 @@
>  	};
>  };
>  
> +&pinctrl {
> +
> +	scif2_pins: serial2 {
> +		/* P3_0 as TxD2; P3_2 as RxD2 */
> +		pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
> +	};
> +};
> +
>  &extal_clk {
>  	clock-frequency = <13330000>;
>  };
> @@ -60,6 +69,9 @@
>  };
>  
>  &scif2 {
> +	pinctrl-names = "default";
> +	pinctrl-0 = <&scif2_pins>;
> +
>  	status = "okay";
>  };
>  
> -- 
> 2.7.4
> 

[PATCH v5 08/10] arm: dts: genmai: Add RIIC2 pin group

[Jacopo Mondi]

Add pin configuration subnode for RIIC2 interface.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index c28d74b..9add1b6 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -43,6 +43,12 @@
 		/* P3_0 as TxD2; P3_2 as RxD2 */
 		pinmux = <RZA1_PINMUX(3, 0, 6)>, <RZA1_PINMUX(3, 2, 4)>;
 	};
+
+	i2c2_pins: i2c2 {
+		/* RIIC2: P1_4 as SCL, P1_5 as SDA */
+		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
+		bi-directional;
+	};
 };
 
 &extal_clk {
@@ -61,6 +67,9 @@
 	status = "okay";
 	clock-frequency = <400000>;
 
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c2_pins>;
+
 	eeprom@50 {
 		compatible = "renesas,24c128";
 		reg = <0x50>;
-- 
2.7.4

[PATCH v5 09/10] arm: dts: genmai: Add user led device nodes

[Jacopo Mondi]

Add device nodes for user leds on Genmai board.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index 9add1b6..f7c512e 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -11,6 +11,7 @@
 
 /dts-v1/;
 #include "r7s72100.dtsi"
+#include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/pinctrl/r7s72100-pinctrl.h>
 
 / {
@@ -35,6 +36,19 @@
 		#address-cells = <1>;
 		#size-cells = <1>;
 	};
+
+	leds {
+		status = "okay";
+		compatible = "gpio-leds";
+
+		led1 {
+			gpios = <&port4 10 GPIO_ACTIVE_LOW>;
+		};
+
+		led2 {
+			gpios = <&port4 11 GPIO_ACTIVE_LOW>;
+		};
+	};
 };
 
 &pinctrl {
-- 
2.7.4

[PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Jacopo Mondi]

Add pin configuration subnode for ETHER ethernet controller.

Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
---
 arch/arm/boot/dts/r7s72100-genmai.dts | 41 +++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/arch/arm/boot/dts/r7s72100-genmai.dts b/arch/arm/boot/dts/r7s72100-genmai.dts
index f7c512e..328f4c9 100644
--- a/arch/arm/boot/dts/r7s72100-genmai.dts
+++ b/arch/arm/boot/dts/r7s72100-genmai.dts
@@ -63,6 +63,34 @@
 		pinmux = <RZA1_PINMUX(1, 4, 1)>, <RZA1_PINMUX(1, 5, 1)>;
 		bi-directional;
 	};
+
+	ether_pins: ether {
+		pins {
+			/* Ethernet on Ports 1,2,3,5 */
+			pinmux = <RZA1_PINMUX(1, 14, 4)>,/* P1_14 = ET_COL  */
+				 <RZA1_PINMUX(5, 9, 2)>, /* P5_9 = ET_MDC   */
+				 <RZA1_PINMUX(3, 4, 2)>, /* P3_4 = ET_RXCLK */
+				 <RZA1_PINMUX(3, 5, 2)>, /* P3_5 = ET_RXER  */
+				 <RZA1_PINMUX(3, 6, 2)>, /* P3_6 = ET_RXDV  */
+				 <RZA1_PINMUX(2, 0, 2)>, /* P2_0 = ET_TXCLK */
+				 <RZA1_PINMUX(2, 1, 2)>, /* P2_1 = ET_TXER  */
+				 <RZA1_PINMUX(2, 2, 2)>, /* P2_2 = ET_TXEN  */
+				 <RZA1_PINMUX(2, 3, 2)>, /* P2_3 = ET_CRS   */
+				 <RZA1_PINMUX(2, 4, 2)>, /* P2_4 = ET_TXD0  */
+				 <RZA1_PINMUX(2, 5, 2)>, /* P2_5 = ET_TXD1  */
+				 <RZA1_PINMUX(2, 6, 2)>, /* P2_6 = ET_TXD2  */
+				 <RZA1_PINMUX(2, 7, 2)>, /* P2_7 = ET_TXD3  */
+				 <RZA1_PINMUX(2, 8, 2)>, /* P2_8 = ET_RXD0  */
+				 <RZA1_PINMUX(2, 9, 2)>, /* P2_9 = ET_RXD1  */
+				 <RZA1_PINMUX(2, 10, 2)>,/* P2_10 = ET_RXD2 */
+				 <RZA1_PINMUX(2, 11, 2)>;/* P2_11 = ET_RXD3 */
+		};
+
+		pins_bidir {
+			pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO  */
+			bi-directional;
+		};
+	};
 };
 
 &extal_clk {
@@ -77,6 +105,19 @@
 	status = "okay";
 };
 
+&ether {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ether_pins>;
+
+	status = "okay";
+
+	renesas,no-ether-link;
+	phy-handle = <&phy0>;
+	phy0: ethernet-phy@0 {
+		reg = <0>;
+	};
+};
+
 &i2c2 {
 	status = "okay";
 	clock-frequency = <400000>;
-- 
2.7.4

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

> --- a/arch/arm/boot/dts/r7s72100-genmai.dts
> +++ b/arch/arm/boot/dts/r7s72100-genmai.dts

> @@ -77,6 +105,19 @@
>         status = "okay";
>  };
>
> +&ether {
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&ether_pins>;
> +
> +       status = "okay";
> +
> +       renesas,no-ether-link;
> +       phy-handle = <&phy0>;
> +       phy0: ethernet-phy@0 {
> +               reg = <0>;

Shouldn't the interrupt (connected to P1_15) be described?

> +       };
> +};

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

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > +&ether {
> > +       pinctrl-names = "default";
> > +       pinctrl-0 = <&ether_pins>;
> > +
> > +       status = "okay";
> > +
> > +       renesas,no-ether-link;
> > +       phy-handle = <&phy0>;
> > +       phy0: ethernet-phy@0 {
> > +               reg = <0>;
> 
> Shouldn't the interrupt (connected to P1_15) be described?


That interrupt pin from the PHY is not used. It did not need to be connected.


Chris

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Simon Horman]

On Thu, Apr 27, 2017 at 10:48:45AM +0000, Chris Brandt wrote:
> Hi Geert,
> 
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
> > > +&ether {
> > > +       pinctrl-names = "default";
> > > +       pinctrl-0 = <&ether_pins>;
> > > +
> > > +       status = "okay";
> > > +
> > > +       renesas,no-ether-link;
> > > +       phy-handle = <&phy0>;
> > > +       phy0: ethernet-phy@0 {
> > > +               reg = <0>;
> > 
> > Shouldn't the interrupt (connected to P1_15) be described?
> 
> 
> That interrupt pin from the PHY is not used. It did not need to be connected.

So things are fine as above or should I expect to see v6?

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Geert Uytterhoeven]

Hi Chris,

On Thu, Apr 27, 2017 at 12:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Thursday, April 27, 2017, Geert Uytterhoeven wrote:
>> > +&ether {
>> > +       pinctrl-names = "default";
>> > +       pinctrl-0 = <&ether_pins>;
>> > +
>> > +       status = "okay";
>> > +
>> > +       renesas,no-ether-link;
>> > +       phy-handle = <&phy0>;
>> > +       phy0: ethernet-phy@0 {
>> > +               reg = <0>;
>>
>> Shouldn't the interrupt (connected to P1_15) be described?
>
> That interrupt pin from the PHY is not used. It did not need to be connected.

But it is connected, according to the schematics.

DT describes hardware, not software limitations.

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

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

Hi Geert,

On Friday, April 28, 2017, Geert Uytterhoeven wrote:
> >> Shouldn't the interrupt (connected to P1_15) be described?
> >
> > That interrupt pin from the PHY is not used. It did not need to be
> connected.
> 
> But it is connected, according to the schematics.

P1_15 can be configured as:
 * GPIO_IN
 * AN7
 * AVB_CAPTURE

So...I guess you would 'describe' it as an GPIO-input.


> DT describes hardware, not software limitations.

Describing things is fine, but the kernel code takes the DT and then start configuring things based on it.

For example, on the RSK board, that line is connected to P4_14. P4_14 can be configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might want to use one of those other choices, so I don't want to describe P4_14 as an interrupt.

If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of the RZ/A1H", that's fine because it's hardware connection that's not going to change.
But...the DT also defines the pin muxing...which is a software decision (do I want to get interrupt or just manually poll or simple ignore it).
This is the part of the whole "DT is for hardware description only" that doesn't really make sense to me.


Chris

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Linus Walleij]

On Fri, Apr 28, 2017 at 4:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:

I think this was written by me, not Geert...

>> DT describes hardware, not software limitations.
>
> Describing things is fine, but the kernel code takes the DT and then start configuring things based on it.
>
> For example, on the RSK board, that line is connected to P4_14. P4_14 can be configured as IRQ6...but IRQ6 comes out in 8 different pin choices, and I might want to use one of those other choices, so I don't want to describe P4_14 as an interrupt.
>
> If it was just describing that "pin 15 of the PHY chip is tied to pin Y19 of the RZ/A1H", that's fine because it's hardware connection that's not going to change.
> But...the DT also defines the pin muxing...which is a software decision (do I want to get interrupt or just manually poll or simple ignore it).
> This is the part of the whole "DT is for hardware description only" that doesn't really make sense to me.

OK yeah we do hardware description AND configuration.

And we never do interpreted languages.

And then there is a bunch of grayzone things. For example we have
a linux,input binding for connecting keypresses to certain Linux input
codes. That is really grayzone, but very useful.

Yours,
Linus Walleij

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Geert Uytterhoeven]

Hi Linus,

On Fri, May 5, 2017 at 2:06 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Apr 28, 2017 at 4:48 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
>
> I think this was written by me, not Geert...
>
>>> DT describes hardware, not software limitations.

Na, I did write it.

I can understand you sometimes mix up things, esp. when you're
enjoying LLC ;-)

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

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, May 05, 2017, Linus Walleij wrote:
> > This is the part of the whole "DT is for hardware description only" that
> doesn't really make sense to me.
> 
> OK yeah we do hardware description AND configuration.
> 
> And we never do interpreted languages.
> 
> And then there is a bunch of grayzone things. For example we have a
> linux,input binding for connecting keypresses to certain Linux input codes.
> That is really grayzone, but very useful.

Ah....

	compatible = "linux,grayzone";


Thanks for the reply. I'll stop ranting now.
Of course, I'll still probably screw it up again on a future patch somehow...

Cheers

Chris

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Linus Walleij]

On Fri, May 5, 2017 at 2:45 PM, Chris Brandt <Chris.Brandt@renesas.com> wrote:
> On Friday, May 05, 2017, Linus Walleij wrote:
>> > This is the part of the whole "DT is for hardware description only" that
>> doesn't really make sense to me.
>>
>> OK yeah we do hardware description AND configuration.
>>
>> And we never do interpreted languages.
>>
>> And then there is a bunch of grayzone things. For example we have a
>> linux,input binding for connecting keypresses to certain Linux input codes.
>> That is really grayzone, but very useful.
>
> Ah....
>
>         compatible = "linux,grayzone";
>
> Thanks for the reply. I'll stop ranting now.
> Of course, I'll still probably screw it up again on a future patch somehow...

For this driver I remarked in some other thread that we may
end up with some

"renesas,bidirectional"

or so. But yeah, it's kinda grayzone too.

Yours,
Linus Walleij

Re: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Linus Walleij]

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:

> Add pin configuration subnode for ETHER ethernet controller.
>
> Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
(...)
> +               pins_bidir {
> +                       pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 = ET_MDIO  */
> +                       bi-directional;
> +               };

So I'm against merging this until someone explains what "bi-directional"
actually means, electrically speaking. What happens physically on this pin?

I think this just means open drain.

It is dangerous to merge things we don't understand.

Surely someone inside Renesas can answer this question.

Yours,
Linus Walleij

RE: [PATCH v5 10/10] arm: dts: genmai: Add ethernet pin group

[Chris Brandt]

On Friday, April 28, 2017, Linus Walleij wrote:
> > Add pin configuration subnode for ETHER ethernet controller.
> >
> > Signed-off-by: Jacopo Mondi <jacopo+renesas@jmondi.org>
> (...)
> > +               pins_bidir {
> > +                       pinmux = <RZA1_PINMUX(3, 3, 2)>;/* P3_3 =
> ET_MDIO  */
> > +                       bi-directional;
> > +               };
> 
> So I'm against merging this until someone explains what "bi-directional"
> actually means, electrically speaking. What happens physically on this
> pin?
> 
> I think this just means open drain.
> 
> It is dangerous to merge things we don't understand.
> 
> Surely someone inside Renesas can answer this question.

I don't think this has anything to do with open drain because you need it for any pin that the peripheral IP block needs to transmit and receive over the same line, regardless of if it's a SDHI, I2C, Ethernet MDIO, etc...
It's more about of allowing the internal IP block signals to get hooked up to the IO pad signals.

Chris

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

[Geert Uytterhoeven]

Hi Jacopo,

On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
<jacopo+renesas@jmondi.org> wrote:
>    this is 5th round of gpio/pincontroller for RZ/A1 devices.
>
> I have updated the pin controller driver to use the newly introduced
> "pinctrl_enable()" function.
> This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> the pin controller does not start.
>
> I have incorporated your comments on the device tree bindings documentation,
> and added to pinctrl-generic.h header file two macros to unpack generic
> properties and their arguments.
>
> Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.

Thanks for the update!

> Jacopo Mondi (10):
>   pinctrl: generic: Add bi-directional and output-enable

Already applied by LinusW.

>   pinctrl: generic: Add macros to unpack properties

LinusW: do you want me to queue this together with the driver for v4.13,
or will you take this single patch for v4.12?

>   pinctrl: Renesas RZ/A1 pin and gpio controller
>   dt-bindings: pinctrl: Add RZ/A1 bindings doc

Will queue in sh-pfc-for-v4.13.

>   arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
>   arm: dts: r7s72100: Add pin controller node
>   arm: dts: genmai: Add SCIF2 pin group
>   arm: dts: genmai: Add RIIC2 pin group
>   arm: dts: genmai: Add user led device nodes
>   arm: dts: genmai: Add ethernet pin group

These are for Simon.

Does applying the DTS changes before the driver introduce regressions?
If no, Simon can queue them for v4.13.
If yes, they'll have to wait for v4.14.

Thanks!

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

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

[Simon Horman]

On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
> 
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
> >    this is 5th round of gpio/pincontroller for RZ/A1 devices.
> >
> > I have updated the pin controller driver to use the newly introduced
> > "pinctrl_enable()" function.
> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > the pin controller does not start.
> >
> > I have incorporated your comments on the device tree bindings documentation,
> > and added to pinctrl-generic.h header file two macros to unpack generic
> > properties and their arguments.
> >
> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
> 
> Thanks for the update!
> 
> > Jacopo Mondi (10):
> >   pinctrl: generic: Add bi-directional and output-enable
> 
> Already applied by LinusW.
> 
> >   pinctrl: generic: Add macros to unpack properties
> 
> LinusW: do you want me to queue this together with the driver for v4.13,
> or will you take this single patch for v4.12?
> 
> >   pinctrl: Renesas RZ/A1 pin and gpio controller
> >   dt-bindings: pinctrl: Add RZ/A1 bindings doc
> 
> Will queue in sh-pfc-for-v4.13.
> 
> >   arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> >   arm: dts: r7s72100: Add pin controller node
> >   arm: dts: genmai: Add SCIF2 pin group
> >   arm: dts: genmai: Add RIIC2 pin group
> >   arm: dts: genmai: Add user led device nodes
> >   arm: dts: genmai: Add ethernet pin group
> 
> These are for Simon.
> 
> Does applying the DTS changes before the driver introduce regressions?
> If no, Simon can queue them for v4.13.
> If yes, they'll have to wait for v4.14.

That is my question too.

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

[jmondi]

Hi Geert, Simon,

On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> Hi Jacopo,
>
> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> <jacopo+renesas@jmondi.org> wrote:
> >    this is 5th round of gpio/pincontroller for RZ/A1 devices.
> >
> > I have updated the pin controller driver to use the newly introduced
> > "pinctrl_enable()" function.
> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > the pin controller does not start.
> >
> > I have incorporated your comments on the device tree bindings documentation,
> > and added to pinctrl-generic.h header file two macros to unpack generic
> > properties and their arguments.
> >
> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>
> Thanks for the update!
>
> > Jacopo Mondi (10):
> >   pinctrl: generic: Add bi-directional and output-enable
>
> Already applied by LinusW.
>
> >   pinctrl: generic: Add macros to unpack properties
>
> LinusW: do you want me to queue this together with the driver for v4.13,
> or will you take this single patch for v4.12?
>
> >   pinctrl: Renesas RZ/A1 pin and gpio controller
> >   dt-bindings: pinctrl: Add RZ/A1 bindings doc
>
> Will queue in sh-pfc-for-v4.13.
>
> >   arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> >   arm: dts: r7s72100: Add pin controller node
> >   arm: dts: genmai: Add SCIF2 pin group
> >   arm: dts: genmai: Add RIIC2 pin group
> >   arm: dts: genmai: Add user led device nodes
> >   arm: dts: genmai: Add ethernet pin group
>
> These are for Simon.
>
> Does applying the DTS changes before the driver introduce regressions?
> If no, Simon can queue them for v4.13.
> If yes, they'll have to wait for v4.14.
>

I tried reverting patch [03/10] which introduces the driver, and I
lose serial output on Genmai. So I guess the dts patches have to be
taken after the driver has been merged.

One question: why can't they be taken at the same time? (eg. for
v4.13?)

Thanks
   j

> Thanks!
>
> 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

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

[Geert Uytterhoeven]

Hi Jacopo,

On Fri, Apr 28, 2017 at 9:24 AM, jmondi <jacopo@jmondi.org> wrote:
> On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
>> On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
>> <jacopo+renesas@jmondi.org> wrote:
>> >    this is 5th round of gpio/pincontroller for RZ/A1 devices.
>> >
>> > I have updated the pin controller driver to use the newly introduced
>> > "pinctrl_enable()" function.
>> > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
>> > the pin controller does not start.
>> >
>> > I have incorporated your comments on the device tree bindings documentation,
>> > and added to pinctrl-generic.h header file two macros to unpack generic
>> > properties and their arguments.
>> >
>> > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
>>
>> Thanks for the update!
>>
>> > Jacopo Mondi (10):
>> >   pinctrl: generic: Add bi-directional and output-enable
>>
>> Already applied by LinusW.
>>
>> >   pinctrl: generic: Add macros to unpack properties
>>
>> LinusW: do you want me to queue this together with the driver for v4.13,
>> or will you take this single patch for v4.12?
>>
>> >   pinctrl: Renesas RZ/A1 pin and gpio controller
>> >   dt-bindings: pinctrl: Add RZ/A1 bindings doc
>>
>> Will queue in sh-pfc-for-v4.13.
>>
>> >   arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
>> >   arm: dts: r7s72100: Add pin controller node
>> >   arm: dts: genmai: Add SCIF2 pin group
>> >   arm: dts: genmai: Add RIIC2 pin group
>> >   arm: dts: genmai: Add user led device nodes
>> >   arm: dts: genmai: Add ethernet pin group
>>
>> These are for Simon.
>>
>> Does applying the DTS changes before the driver introduce regressions?
>> If no, Simon can queue them for v4.13.
>> If yes, they'll have to wait for v4.14.
>>
>
> I tried reverting patch [03/10] which introduces the driver, and I
> lose serial output on Genmai. So I guess the dts patches have to be
> taken after the driver has been merged.
>
> One question: why can't they be taken at the same time? (eg. for
> v4.13?)

Because the driver goes in through me and LinusW, while the DTS goes in
through Simon and arm-soc.

If Simon applies the DTS patches, it will regress genmai in renesas-devel
until the release of v4.13-rc1.

An immutable branch would solve that, but for adding PFC we usuable just
postpone the DT part by one cycle.

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

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

[Simon Horman]

On Fri, Apr 28, 2017 at 09:24:57AM +0200, jmondi wrote:
> Hi Geert, Simon,
> 
> On Thu, Apr 27, 2017 at 10:42:02AM +0200, Geert Uytterhoeven wrote:
> > Hi Jacopo,
> >
> > On Thu, Apr 27, 2017 at 10:19 AM, Jacopo Mondi
> > <jacopo+renesas@jmondi.org> wrote:
> > >    this is 5th round of gpio/pincontroller for RZ/A1 devices.
> > >
> > > I have updated the pin controller driver to use the newly introduced
> > > "pinctrl_enable()" function.
> > > This is required since v4.11-rc7 as otherwise, as reported by Chris Brandt,
> > > the pin controller does not start.
> > >
> > > I have incorporated your comments on the device tree bindings documentation,
> > > and added to pinctrl-generic.h header file two macros to unpack generic
> > > properties and their arguments.
> > >
> > > Tested with SCIF, RIIC, ETHER and gpio-leds on Genmai board.
> >
> > Thanks for the update!
> >
> > > Jacopo Mondi (10):
> > >   pinctrl: generic: Add bi-directional and output-enable
> >
> > Already applied by LinusW.
> >
> > >   pinctrl: generic: Add macros to unpack properties
> >
> > LinusW: do you want me to queue this together with the driver for v4.13,
> > or will you take this single patch for v4.12?
> >
> > >   pinctrl: Renesas RZ/A1 pin and gpio controller
> > >   dt-bindings: pinctrl: Add RZ/A1 bindings doc
> >
> > Will queue in sh-pfc-for-v4.13.
> >
> > >   arm: dts: dt-bindings: Add Renesas RZ/A1 pinctrl header
> > >   arm: dts: r7s72100: Add pin controller node
> > >   arm: dts: genmai: Add SCIF2 pin group
> > >   arm: dts: genmai: Add RIIC2 pin group
> > >   arm: dts: genmai: Add user led device nodes
> > >   arm: dts: genmai: Add ethernet pin group
> >
> > These are for Simon.
> >
> > Does applying the DTS changes before the driver introduce regressions?
> > If no, Simon can queue them for v4.13.
> > If yes, they'll have to wait for v4.14.
> >
> 
> I tried reverting patch [03/10] which introduces the driver, and I
> lose serial output on Genmai. So I guess the dts patches have to be
> taken after the driver has been merged.
> 
> One question: why can't they be taken at the same time? (eg. for
> v4.13?)

The problem is that the changes go through different trees. It may,
however, be possible to arrange for both sets of changes to go into v4.13
by negotiating with the ARM-SoC maintainers.