[RESEND,1/2] pinctrl: change function behavior for per pin muxing controllers
diff mbox

Message ID 1433948699-19800-2-git-send-email-ludovic.desroches@atmel.com
State New
Headers show

Commit Message

ludovic.desroches@atmel.com June 10, 2015, 3:04 p.m. UTC
When having a controller which allows per pin muxing, declaring with
which groups a function can be used is a useless constraint since groups
are something virtual.

Signed-off-by: Ludovic Desroches <ludovic.desroches@atmel.com>
---
 drivers/pinctrl/pinmux.c       | 58 +++++++++++++++++++++++-------------------
 include/linux/pinctrl/pinmux.h |  3 +++
 2 files changed, 35 insertions(+), 26 deletions(-)

Comments

Stephen Warren June 15, 2015, 3:58 p.m. UTC | #1
On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
> When having a controller which allows per pin muxing, declaring with
> which groups a function can be used is a useless constraint since groups
> are something virtual.

This isn't true.

Irrespective of whether a particular piece of pinmux HW can control the 
mux function for each pin individually, or only in groups, it's quite 
likely that each function can only be selected onto a subset of those 
pins or groups. Requiring the pinctrl driver to inform the core which 
set of pins/groups particular functions can be selected onto seems quite 
reasonable.

In my opinion at least, for HW that can select the mux function at the 
per-pin level, the only sensible set of groups is one group per pin with 
each group containing a single pin. Any other use of groups is a 
SW/user-level construct, and is something unrelated to why the pinctrl 
subsystem supports groups. If we want to represent those groups in 
pinctrl, there should be two separate sets of groups; one to represent 
the actual HW capabilities, and one to represent the SW/user-level 
convenience abstractions.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ludovic.desroches@atmel.com June 17, 2015, 12:38 p.m. UTC | #2
Hi Stephen,

On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote:
> On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
> >When having a controller which allows per pin muxing, declaring with
> >which groups a function can be used is a useless constraint since groups
> >are something virtual.
> 
> This isn't true.
> 
> Irrespective of whether a particular piece of pinmux HW can control the mux
> function for each pin individually, or only in groups, it's quite likely
> that each function can only be selected onto a subset of those pins or
> groups. Requiring the pinctrl driver to inform the core which set of
> pins/groups particular functions can be selected onto seems quite
> reasonable.
> 
> In my opinion at least, for HW that can select the mux function at the
> per-pin level, the only sensible set of groups is one group per pin with
> each group containing a single pin. Any other use of groups is a
> SW/user-level construct, and is something unrelated to why the pinctrl
> subsystem supports groups. If we want to represent those groups in pinctrl,
> there should be two separate sets of groups; one to represent the actual HW
> capabilities, and one to represent the SW/user-level convenience
> abstractions.

Groups that I would like to use are not something related to the user or
software. It's an hardware reality but they would be more flexibles.

Usually the muxing is done by selecting a function (which seems to be
device related: usart, spi, etc.), then you select on which pins you
want this function.

In my case, I can't select a function only by choosing a mux. It is a
combination of the mux and the pin on which it is applied. So my
functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I
will have my i2c clock signal but I can have this signal on pin 58 if I
use function C. Do you understand what I mean? It's not very easy to
explain... So here is a real example:

 -------------------------------------------------- 
|              | pio peripheral                    |
 -------------------------------------------------- 
| signal | dir | func | signal       | dir | ioset |
 -------------------------------------------------- 
| PA8    | I/O | A    | SDMMC0_DAT6  | I/O | 1     |
|        |     | B    | QSPI1_IO1    | I/O | 1     |
|        |     | D    | TCLK5        | I   | 1     |
|        |     | E    | FLEXCOM2_IO2 | I/O | 1     |
|        |     | F    | NWE/NANDWE   | O   | 2     |
 -------------------------------------------------- 
| PD28   | I/O | A    | SPI1_NPCS0   | I/O | 3     |
|        |     | B    | TDI          | I/O | 3     |
|        |     | C    | FLEXCOM2_IO2 | I   | 2     |
 -------------------------------------------------- 


You are right that having a group per pin is a solution.

If I follow your proposal (tell me if I'm wrong):
- I will have 128 groups since I have 128 pins.
- I will have functions GPIO, A, B, C, D, E, F.
- I have to give the groups which can achieve a function so I will have
128 groups for each function. It means 128 x 7 = 896 groups.

Does it seems to be something reasonable and intelligible? From my point
of view no. And what about the sysfs readability?

With my current implementation, I have something quite understandable
for the user if he needs to check the pinmuxing:

  # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
  pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
  pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
  pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0  
                                                                                
  # cat /sys/kernel/debug/pinctrl/pinctrl-maps                                  
  Pinctrl maps:                                                                 
                                                                                
  device b0000000.sdio-host                                                     
  state default                                                                 
  type MUX_GROUP (2)                                                            
  controlling device ahb:apb:pinctrl@fc038000                                   
  group sdmmc1_0                                                                
  function E                                                                    
                                                                                
  device b0000000.sdio-host                                                     
  state default                                                                 
  type CONFIGS_PIN (3)                                                          
  controlling device ahb:apb:pinctrl@fc038000                                   
  pin PA28                                                                      
  config 00010003                                                               
                                                                                
  device b0000000.sdio-host                                                     
  state default                                                                 
  type CONFIGS_PIN (3)                                                          
  controlling device ahb:apb:pinctrl@fc038000                                   
  pin PA18                                                                      
  config 00010003                                                               
                                                                                
                                                  
Doing as you propose, I assume the result should be:                            
                                                                                
  # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins        
  pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)                               
  pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA18
  pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA19

  # cat /sys/kernel/debug/pinctrl/pinctrl-maps
  Pinctrl maps:

  device b0000000.sdio-host
  state default
  type MUX_GROUP (2)
  controlling device ahb:apb:pinctrl@fc038000
  group PA28
  function E

  device b0000000.sdio-host
  state default
  type CONFIGS_PIN (3)
  controlling device ahb:apb:pinctrl@fc038000
  pin PA28
  config 00010003
                                                                                
  device b0000000.sdio-host
  state default
  type MUX_GROUP (2)
  controlling device ahb:apb:pinctrl@fc038000
  group PA18
  function E

  device b0000000.sdio-host
  state default
  type CONFIGS_PIN (3)
  controlling device ahb:apb:pinctrl@fc038000
  pin PA18
  config 00010003

I think it is more difficult to understand what is done here.


I have sent patches months ago trying to improve things by having
something more flexible. I don't think I introduce too big changes.
The only answers I got were from people thinking that pinctrl framework
conception is not good to fit all kind of controllers. I re-sent the
patch series to gain more expose and have no  answer...

I wanted to use as much as possible the generic stuff we have in order
to not add a new "custom" implementation. If it is such a big deal to
do these changes because you (or other people) think that we can use
it as is to describe our pinmux/pinconf configuration then I can
switch to a custom implementation too and not use the pinconf generic
dt_node_to_map. I think this solution is not encouraged by Linus who
prefers to cling the generic infrastructure, isn'it? This is exactly
why I took this approach.


FYI: some context about these patches:

The RFC I send long time ago:
http://www.spinics.net/lists/linux-gpio/msg05198.html

My second attempt:
http://comments.gmane.org/gmane.linux.kernel.gpio/7767

It is not the first time, there are discussions about it. Sascha sent a
patch which fits part of my needs:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318452.html

And it seems some controllers use this approach:
http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318452.html


Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren June 17, 2015, 3:55 p.m. UTC | #3
On 06/17/2015 06:38 AM, Ludovic Desroches wrote:
> Hi Stephen,
>
> On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote:
>> On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
>>> When having a controller which allows per pin muxing, declaring with
>>> which groups a function can be used is a useless constraint since groups
>>> are something virtual.
>>
>> This isn't true.
>>
>> Irrespective of whether a particular piece of pinmux HW can control the mux
>> function for each pin individually, or only in groups, it's quite likely
>> that each function can only be selected onto a subset of those pins or
>> groups. Requiring the pinctrl driver to inform the core which set of
>> pins/groups particular functions can be selected onto seems quite
>> reasonable.
>>
>> In my opinion at least, for HW that can select the mux function at the
>> per-pin level, the only sensible set of groups is one group per pin with
>> each group containing a single pin. Any other use of groups is a
>> SW/user-level construct, and is something unrelated to why the pinctrl
>> subsystem supports groups. If we want to represent those groups in pinctrl,
>> there should be two separate sets of groups; one to represent the actual HW
>> capabilities, and one to represent the SW/user-level convenience
>> abstractions.
>
> Groups that I would like to use are not something related to the user or
> software. It's an hardware reality but they would be more flexibles.
>
> Usually the muxing is done by selecting a function (which seems to be
> device related: usart, spi, etc.), then you select on which pins you
> want this function.
>
> In my case, I can't select a function only by choosing a mux. It is a
> combination of the mux and the pin on which it is applied. So my
> functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I
> will have my i2c clock signal but I can have this signal on pin 58 if I
> use function C. Do you understand what I mean? It's not very easy to
> explain... So here is a real example:
>
>   --------------------------------------------------
> |              | pio peripheral                    |
>   --------------------------------------------------
> | signal | dir | func | signal       | dir | ioset |
>   --------------------------------------------------
> | PA8    | I/O | A    | SDMMC0_DAT6  | I/O | 1     |
> |        |     | B    | QSPI1_IO1    | I/O | 1     |
> |        |     | D    | TCLK5        | I   | 1     |
> |        |     | E    | FLEXCOM2_IO2 | I/O | 1     |
> |        |     | F    | NWE/NANDWE   | O   | 2     |
>   --------------------------------------------------
> | PD28   | I/O | A    | SPI1_NPCS0   | I/O | 3     |
> |        |     | B    | TDI          | I/O | 3     |
> |        |     | C    | FLEXCOM2_IO2 | I   | 2     |
>   --------------------------------------------------
>
>
> You are right that having a group per pin is a solution.
>
> If I follow your proposal (tell me if I'm wrong):
> - I will have 128 groups since I have 128 pins.

Yes.

> - I will have functions GPIO, A, B, C, D, E, F.

You could have functions A..F, and require the user to determine what 
option they want for each pin, find the pin-specific mux function value 
for each pin, and put that into the DT (or other pinmux data source). 
For example, the bcm2835 pinctrl driver works this way.

In that case, each of the functions A..F could be selected on each pin, 
so you'd have a very simple "get pins for function" implementation.

Alternatively, you could define a logic function per IO controller or 
signal that gets pinmuxed. In the example above, FLEXCOM2_IO2 is one 
such example. Given that set of functions, you'd need a mapping table to 
convert from the logical functions seen by the pinctrl subsystem to the 
HW values that need to be written into registers. For example, the Tegra 
pinctrl drivers work this way.

In that case, each (pinctrl) function could only be selected on a 
specific subset of all pins/groups, and so you'd probably need a 
table-based implementation of "get pins for function".

> - I have to give the groups which can achieve a function so I will have
> 128 groups for each function. It means 128 x 7 = 896 groups.

I don't think so no. I'm not sure why you'd consider multiplying 128 and 
7 here. I'd expect 128 groups since you have 128 pins[1].

Well, it's possible to have slightly more groups if, say, mux function 
is selectable per pin, whereas something else like drive strength is 
configured by a register that affects multiple pins at once. You'd need 
separate sets of groups for muxing and for drive strength configuration. 
Some Tegra SoCs are like this. Still, we just add the different sets of 
groups together here, not multiply. The overall set of groups is not 
that much larger than the set of pins.

> Does it seems to be something reasonable and intelligible? From my point
> of view no. And what about the sysfs readability?
>
> With my current implementation, I have something quite understandable
> for the user if he needs to check the pinmuxing:
>
>    # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
>    pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
>    pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
>    pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
>
>    # cat /sys/kernel/debug/pinctrl/pinctrl-maps
>    Pinctrl maps:
>
>    device b0000000.sdio-host
>    state default
>    type MUX_GROUP (2)
>    controlling device ahb:apb:pinctrl@fc038000
>    group sdmmc1_0
>    function E
>
>    device b0000000.sdio-host
>    state default
>    type CONFIGS_PIN (3)
>    controlling device ahb:apb:pinctrl@fc038000
>    pin PA28
>    config 00010003
>
>    device b0000000.sdio-host
>    state default
>    type CONFIGS_PIN (3)
>    controlling device ahb:apb:pinctrl@fc038000
>    pin PA18
>    config 00010003
>
>
> Doing as you propose, I assume the result should be:
>
>    # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
>    pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
>    pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA18
>    pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA19
>
>    # cat /sys/kernel/debug/pinctrl/pinctrl-maps
>    Pinctrl maps:
>
>    device b0000000.sdio-host
>    state default
>    type MUX_GROUP (2)
>    controlling device ahb:apb:pinctrl@fc038000
>    group PA28
>    function E
>
>    device b0000000.sdio-host
>    state default
>    type CONFIGS_PIN (3)
>    controlling device ahb:apb:pinctrl@fc038000
>    pin PA28
>    config 00010003
>
>    device b0000000.sdio-host
>    state default
>    type MUX_GROUP (2)
>    controlling device ahb:apb:pinctrl@fc038000
>    group PA18
>    function E
>
>    device b0000000.sdio-host
>    state default
>    type CONFIGS_PIN (3)
>    controlling device ahb:apb:pinctrl@fc038000
>    pin PA18
>    config 00010003
>
> I think it is more difficult to understand what is done here.

I don't think I agree. The HW level groups are the individual pins, so I 
think the second option is clearer and more correct. What is the 
"sdmmc1_0" group in the first example? Does any such thing even exist in HW?

> I have sent patches months ago trying to improve things by having
> something more flexible. I don't think I introduce too big changes.
> The only answers I got were from people thinking that pinctrl framework
> conception is not good to fit all kind of controllers. I re-sent the
> patch series to gain more expose and have no  answer...

I don't see anything in your description which implies pinctrl isn't 
perfectly suitable for your HW.

Note that I'm on vacation for a couple weeks soon, and I don't expect to 
follow this conversation during that time. Ultimately, LinusW owns the 
pinctrl subsystem, and you need to convince him of any changes.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ludovic.desroches@atmel.com June 18, 2015, 12:33 p.m. UTC | #4
On Wed, Jun 17, 2015 at 09:55:56AM -0600, Stephen Warren wrote:
> On 06/17/2015 06:38 AM, Ludovic Desroches wrote:
> >Hi Stephen,
> >
> >On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote:
> >>On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
> >>>When having a controller which allows per pin muxing, declaring with
> >>>which groups a function can be used is a useless constraint since groups
> >>>are something virtual.
> >>
> >>This isn't true.
> >>
> >>Irrespective of whether a particular piece of pinmux HW can control the mux
> >>function for each pin individually, or only in groups, it's quite likely
> >>that each function can only be selected onto a subset of those pins or
> >>groups. Requiring the pinctrl driver to inform the core which set of
> >>pins/groups particular functions can be selected onto seems quite
> >>reasonable.
> >>
> >>In my opinion at least, for HW that can select the mux function at the
> >>per-pin level, the only sensible set of groups is one group per pin with
> >>each group containing a single pin. Any other use of groups is a
> >>SW/user-level construct, and is something unrelated to why the pinctrl
> >>subsystem supports groups. If we want to represent those groups in pinctrl,
> >>there should be two separate sets of groups; one to represent the actual HW
> >>capabilities, and one to represent the SW/user-level convenience
> >>abstractions.
> >
> >Groups that I would like to use are not something related to the user or
> >software. It's an hardware reality but they would be more flexibles.
> >
> >Usually the muxing is done by selecting a function (which seems to be
> >device related: usart, spi, etc.), then you select on which pins you
> >want this function.
> >
> >In my case, I can't select a function only by choosing a mux. It is a
> >combination of the mux and the pin on which it is applied. So my
> >functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I
> >will have my i2c clock signal but I can have this signal on pin 58 if I
> >use function C. Do you understand what I mean? It's not very easy to
> >explain... So here is a real example:
> >
> >  --------------------------------------------------
> >|              | pio peripheral                    |
> >  --------------------------------------------------
> >| signal | dir | func | signal       | dir | ioset |
> >  --------------------------------------------------
> >| PA8    | I/O | A    | SDMMC0_DAT6  | I/O | 1     |
> >|        |     | B    | QSPI1_IO1    | I/O | 1     |
> >|        |     | D    | TCLK5        | I   | 1     |
> >|        |     | E    | FLEXCOM2_IO2 | I/O | 1     |
> >|        |     | F    | NWE/NANDWE   | O   | 2     |
> >  --------------------------------------------------
> >| PD28   | I/O | A    | SPI1_NPCS0   | I/O | 3     |
> >|        |     | B    | TDI          | I/O | 3     |
> >|        |     | C    | FLEXCOM2_IO2 | I   | 2     |
> >  --------------------------------------------------
> >
> >
> >You are right that having a group per pin is a solution.
> >
> >If I follow your proposal (tell me if I'm wrong):
> >- I will have 128 groups since I have 128 pins.
> 
> Yes.
> 
> >- I will have functions GPIO, A, B, C, D, E, F.
> 
> You could have functions A..F, and require the user to determine what option
> they want for each pin, find the pin-specific mux function value for each
> pin, and put that into the DT (or other pinmux data source). For example,
> the bcm2835 pinctrl driver works this way.
> 
> In that case, each of the functions A..F could be selected on each pin, so
> you'd have a very simple "get pins for function" implementation.
> 
> Alternatively, you could define a logic function per IO controller or signal
> that gets pinmuxed. In the example above, FLEXCOM2_IO2 is one such example.
> Given that set of functions, you'd need a mapping table to convert from the
> logical functions seen by the pinctrl subsystem to the HW values that need
> to be written into registers. For example, the Tegra pinctrl drivers work
> this way.
> 
> In that case, each (pinctrl) function could only be selected on a specific
> subset of all pins/groups, and so you'd probably need a table-based
> implementation of "get pins for function".

Thanks for giving me some examples. Let's take a look at these drivers.
My concern is that I didnn't want to have many and/or big tables in my
driver. I will have to update it for a new SoC even if the pin
controller is the same. I prefer to have it in the device as we did
before and as some drivers continue to do.

> 
> >- I have to give the groups which can achieve a function so I will have
> >128 groups for each function. It means 128 x 7 = 896 groups.
> 
> I don't think so no. I'm not sure why you'd consider multiplying 128 and 7
> here. I'd expect 128 groups since you have 128 pins[1].
> 

Sorry my mistake, I mean 896 possibilites since I have 128 groups for a
function.

> Well, it's possible to have slightly more groups if, say, mux function is
> selectable per pin, whereas something else like drive strength is configured
> by a register that affects multiple pins at once. You'd need separate sets
> of groups for muxing and for drive strength configuration. Some Tegra SoCs
> are like this. Still, we just add the different sets of groups together
> here, not multiply. The overall set of groups is not that much larger than
> the set of pins.
> 
> >Does it seems to be something reasonable and intelligible? From my point
> >of view no. And what about the sysfs readability?
> >
> >With my current implementation, I have something quite understandable
> >for the user if he needs to check the pinmuxing:
> >
> >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> >
> >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> >   Pinctrl maps:
> >
> >   device b0000000.sdio-host
> >   state default
> >   type MUX_GROUP (2)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   group sdmmc1_0
> >   function E
> >
> >   device b0000000.sdio-host
> >   state default
> >   type CONFIGS_PIN (3)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   pin PA28
> >   config 00010003
> >
> >   device b0000000.sdio-host
> >   state default
> >   type CONFIGS_PIN (3)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   pin PA18
> >   config 00010003
> >
> >
> >Doing as you propose, I assume the result should be:
> >
> >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA18
> >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA19
> >
> >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> >   Pinctrl maps:
> >
> >   device b0000000.sdio-host
> >   state default
> >   type MUX_GROUP (2)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   group PA28
> >   function E
> >
> >   device b0000000.sdio-host
> >   state default
> >   type CONFIGS_PIN (3)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   pin PA28
> >   config 00010003
> >
> >   device b0000000.sdio-host
> >   state default
> >   type MUX_GROUP (2)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   group PA18
> >   function E
> >
> >   device b0000000.sdio-host
> >   state default
> >   type CONFIGS_PIN (3)
> >   controlling device ahb:apb:pinctrl@fc038000
> >   pin PA18
> >   config 00010003
> >
> >I think it is more difficult to understand what is done here.
> 
> I don't think I agree. The HW level groups are the individual pins, so I
> think the second option is clearer and more correct. What is the "sdmmc1_0"
> group in the first example? Does any such thing even exist in HW?

If a user see this, I will have to take the datasheet to undersand what
using function E on this pin really means.

sdmmc1_0 group doesn't really exists as a group but a collection of
pins. The minimum requirement is to have CK, CMD and DAT0 signals. The
user can choose if he wants to use DAT1,2,3,4,5,6,7 and CD. Maybe he
would like to keep this pins available for another function.

This is what I have in my device tree.

pinctrl@fc038000 {
	group_defs {
		sdmmc1_0 {
			pins = <PIN_PA22__SDMMC1_CK>,
			       <PIN_PA28__SDMMC1_CMD>,
			       <PIN_PA18__SDMMC1_DAT0>,
			       <PIN_PA19__SDMMC1_DAT1>,
			       <PIN_PA20__SDMMC1_DAT2>,
			       <PIN_PA21__SDMMC1_DAT3>,
			       <PIN_PA30__SDMMC1_CD>;
		};
	};

	pinctrl_sdmmc1_default: sdmmc1_default {
		mux {
			function = "E";
			groups = "sdmmc1_0";
		};

		conf-cmd_data {
			pins = <PIN_PA28__SDMMC1_CMD>,
			       <PIN_PA18__SDMMC1_DAT0>,
			       <PIN_PA19__SDMMC1_DAT1>,
			       <PIN_PA20__SDMMC1_DAT2>,
			       <PIN_PA21__SDMMC1_DAT3>;
			bias-pull-up;
		};

		conf-ck_cd {
			pins = <PIN_PA22__SDMMC1_CK>,
			       <PIN_PA30__SDMMC1_CD>;
			bias-disable;
		};
	};
}

From my point of view, it is not so far from what generic pinconf does.
The only addition is the group_defs node because I don't want SoC
dependant tables in my driver.

> 
> >I have sent patches months ago trying to improve things by having
> >something more flexible. I don't think I introduce too big changes.
> >The only answers I got were from people thinking that pinctrl framework
> >conception is not good to fit all kind of controllers. I re-sent the
> >patch series to gain more expose and have no  answer...
> 
> I don't see anything in your description which implies pinctrl isn't
> perfectly suitable for your HW.
> 

It could but it doesn't fit all my requirements:
- no SoC dependant tables in the driver
- comprehensive sysfs

I agree that these two requirements could be controversial, particulary
driver tables vs DT. At the moment, I feel there are pros and cons which are
all admissible so in my opinion both approach should be acceptable.

> Note that I'm on vacation for a couple weeks soon, and I don't expect to
> follow this conversation during that time. Ultimately, LinusW owns the
> pinctrl subsystem, and you need to convince him of any changes.

Ok, I'll think about this discussion and try to see what in involves for
my case.


Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Nicolas Ferre June 30, 2015, 9:17 a.m. UTC | #5
Le 17/06/2015 17:55, Stephen Warren a écrit :
> On 06/17/2015 06:38 AM, Ludovic Desroches wrote:

[..]

>> I have sent patches months ago trying to improve things by having
>> something more flexible. I don't think I introduce too big changes.
>> The only answers I got were from people thinking that pinctrl framework
>> conception is not good to fit all kind of controllers. I re-sent the
>> patch series to gain more expose and have no  answer...
> 
> I don't see anything in your description which implies pinctrl isn't 
> perfectly suitable for your HW.

We are not talking about suitability, we are talking about some little
changes to the generic part, just to have more accurate information and
a little bit more flexibility with our controller.

We read the drivers that Stephen pointed out and it seems that it even
doesn't use the whole generic part of the pinconf. Moreover, we do think
that the statement "one pin" == "one group" leads to a loss of
information and ease of use.
We are not talking about the use of defines, tables, macros to reach an
usable pinctrl: let's say that we have different views.

> Note that I'm on vacation for a couple weeks soon, and I don't expect to 
> follow this conversation during that time. Ultimately, LinusW owns the 
> pinctrl subsystem, and you need to convince him of any changes.

Okay, so we are back at the same situation we had ended up with several
months ago:

- no agreement on 3 points:
1/ ways to use groups in generic pinctrl
2/ ways to describe a comprehensive configuration in device tree
3/ readability of a sysfs information

- no way out on the generic pinctrl little changes that Ludovic proposed
as Linus W. never gave his point of view (RFC posts on April the 2nd).

Ludovic explained at length our point of view and gave detailed
technical arguments. We don't intend to convince you, we just would like
the harmless modifications to be integrated.

As we preferred to give a chance to the generic pinconf/pinctrl for our
use by adding a little bit of flexibility, we are now in a situation
where we are nearly obliged to give up this approach and write a new
driver without the use of the generic facilities: what a pity!
We lost several months of useless work to match what we thought the
maintainer would prefer.

So Linus, do you confirm that we won't go further with this approach?

We are pretty disappointed by the way this interaction with the pinctrl
sub-system went.

Best regards,
Linus Walleij July 13, 2015, 12:07 p.m. UTC | #6
On Tue, Jun 30, 2015 at 11:17 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:

> - no agreement on 3 points:
> 1/ ways to use groups in generic pinctrl
> 2/ ways to describe a comprehensive configuration in device tree
> 3/ readability of a sysfs information
>
> - no way out on the generic pinctrl little changes that Ludovic proposed
> as Linus W. never gave his point of view (RFC posts on April the 2nd).

Yeah I know. I am battling with this, it is one of those topics
where you feel "eeeehhhh" and try to avoid looking at it. Sadly
I have to...

I refer to Documentation/ManagementStyle, chapter 1,
"Decisions".

What makes me feel uneasy about these things is that the decisions
are irreversible due to the nature of the device tree bindings.

I am not a manager, but a maintainer...

> Ludovic explained at length our point of view and gave detailed
> technical arguments. We don't intend to convince you, we just would like
> the harmless modifications to be integrated.

So is it universally agreed that the changes are harmless?

If they are harmless, I would be able to revert the patch and
nothing breaks in the world, I don't think that is the case. The
case is a piece of code and functionality that is not AT91-specific
but has to be maintained in the core pinctrl code forever.

That is basically the big problem with anything device tree.
It etches stuff in stone so it can't be changed, like ever.
Further, the decision on whether to etch this or that is
pushed to Linux subsystem maintainers, who are clearly
unsuited for the task. :(

> As we preferred to give a chance to the generic pinconf/pinctrl for our
> use by adding a little bit of flexibility, we are now in a situation
> where we are nearly obliged to give up this approach and write a new
> driver without the use of the generic facilities: what a pity!
> We lost several months of useless work to match what we thought the
> maintainer would prefer.
>
> So Linus, do you confirm that we won't go further with this approach?
>
> We are pretty disappointed by the way this interaction with the pinctrl
> sub-system went.

I'm sorry, I am just trying to wait for consensus but it seems to
be hard for that to happen.

Things I need:

- More DT bindings people to look at this patch.

- More other driver maintainers to look at this patch.
  Sacha is one of those I'd like to get an opinion from
  for example. ACKs, Reviewed-by's are always good.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 13, 2015, 12:13 p.m. UTC | #7
On Wed, Jun 17, 2015 at 2:38 PM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:

> It is not the first time, there are discussions about it. Sascha sent a
> patch which fits part of my needs:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2015-January/318452.html

It would be helpful to have Sascha's review of this patch set.

If it also fixes his issues, we will have some partial consensus,
as in "fixes more that AT91 problems".

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sascha Hauer July 14, 2015, 5:57 a.m. UTC | #8
On Thu, Jun 18, 2015 at 02:33:48PM +0200, Ludovic Desroches wrote:
> On Wed, Jun 17, 2015 at 09:55:56AM -0600, Stephen Warren wrote:
> > On 06/17/2015 06:38 AM, Ludovic Desroches wrote:
> > >Hi Stephen,
> > >
> > >On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote:
> > >>On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
> > >>>When having a controller which allows per pin muxing, declaring with
> > >>>which groups a function can be used is a useless constraint since groups
> > >>>are something virtual.
> > >>
> > >>This isn't true.
> > >>
> > >>Irrespective of whether a particular piece of pinmux HW can control the mux
> > >>function for each pin individually, or only in groups, it's quite likely
> > >>that each function can only be selected onto a subset of those pins or
> > >>groups. Requiring the pinctrl driver to inform the core which set of
> > >>pins/groups particular functions can be selected onto seems quite
> > >>reasonable.
> > >>
> > >>In my opinion at least, for HW that can select the mux function at the
> > >>per-pin level, the only sensible set of groups is one group per pin with
> > >>each group containing a single pin. Any other use of groups is a
> > >>SW/user-level construct, and is something unrelated to why the pinctrl
> > >>subsystem supports groups. If we want to represent those groups in pinctrl,
> > >>there should be two separate sets of groups; one to represent the actual HW
> > >>capabilities, and one to represent the SW/user-level convenience
> > >>abstractions.
> > >
> > >Groups that I would like to use are not something related to the user or
> > >software. It's an hardware reality but they would be more flexibles.
> > >
> > >Usually the muxing is done by selecting a function (which seems to be
> > >device related: usart, spi, etc.), then you select on which pins you
> > >want this function.
> > >
> > >In my case, I can't select a function only by choosing a mux. It is a
> > >combination of the mux and the pin on which it is applied. So my
> > >functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I
> > >will have my i2c clock signal but I can have this signal on pin 58 if I
> > >use function C. Do you understand what I mean? It's not very easy to
> > >explain... So here is a real example:
> > >
> > >  --------------------------------------------------
> > >|              | pio peripheral                    |
> > >  --------------------------------------------------
> > >| signal | dir | func | signal       | dir | ioset |
> > >  --------------------------------------------------
> > >| PA8    | I/O | A    | SDMMC0_DAT6  | I/O | 1     |
> > >|        |     | B    | QSPI1_IO1    | I/O | 1     |
> > >|        |     | D    | TCLK5        | I   | 1     |
> > >|        |     | E    | FLEXCOM2_IO2 | I/O | 1     |
> > >|        |     | F    | NWE/NANDWE   | O   | 2     |
> > >  --------------------------------------------------
> > >| PD28   | I/O | A    | SPI1_NPCS0   | I/O | 3     |
> > >|        |     | B    | TDI          | I/O | 3     |
> > >|        |     | C    | FLEXCOM2_IO2 | I   | 2     |
> > >  --------------------------------------------------
> > >
> > >
> > >You are right that having a group per pin is a solution.
> > >
> > >If I follow your proposal (tell me if I'm wrong):
> > >- I will have 128 groups since I have 128 pins.
> > 
> > Yes.
> > 
> > >- I will have functions GPIO, A, B, C, D, E, F.
> > 
> > You could have functions A..F, and require the user to determine what option
> > they want for each pin, find the pin-specific mux function value for each
> > pin, and put that into the DT (or other pinmux data source). For example,
> > the bcm2835 pinctrl driver works this way.
> > 
> > In that case, each of the functions A..F could be selected on each pin, so
> > you'd have a very simple "get pins for function" implementation.
> > 
> > Alternatively, you could define a logic function per IO controller or signal
> > that gets pinmuxed. In the example above, FLEXCOM2_IO2 is one such example.
> > Given that set of functions, you'd need a mapping table to convert from the
> > logical functions seen by the pinctrl subsystem to the HW values that need
> > to be written into registers. For example, the Tegra pinctrl drivers work
> > this way.
> > 
> > In that case, each (pinctrl) function could only be selected on a specific
> > subset of all pins/groups, and so you'd probably need a table-based
> > implementation of "get pins for function".
> 
> Thanks for giving me some examples. Let's take a look at these drivers.
> My concern is that I didnn't want to have many and/or big tables in my
> driver. I will have to update it for a new SoC even if the pin
> controller is the same. I prefer to have it in the device as we did
> before and as some drivers continue to do.
> 
> > 
> > >- I have to give the groups which can achieve a function so I will have
> > >128 groups for each function. It means 128 x 7 = 896 groups.
> > 
> > I don't think so no. I'm not sure why you'd consider multiplying 128 and 7
> > here. I'd expect 128 groups since you have 128 pins[1].
> > 
> 
> Sorry my mistake, I mean 896 possibilites since I have 128 groups for a
> function.
> 
> > Well, it's possible to have slightly more groups if, say, mux function is
> > selectable per pin, whereas something else like drive strength is configured
> > by a register that affects multiple pins at once. You'd need separate sets
> > of groups for muxing and for drive strength configuration. Some Tegra SoCs
> > are like this. Still, we just add the different sets of groups together
> > here, not multiply. The overall set of groups is not that much larger than
> > the set of pins.
> > 
> > >Does it seems to be something reasonable and intelligible? From my point
> > >of view no. And what about the sysfs readability?
> > >
> > >With my current implementation, I have something quite understandable
> > >for the user if he needs to check the pinmuxing:
> > >
> > >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> > >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> > >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> > >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> > >
> > >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> > >   Pinctrl maps:
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type MUX_GROUP (2)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   group sdmmc1_0
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   pin PA28
> > >   config 00010003
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   pin PA18
> > >   config 00010003
> > >
> > >
> > >Doing as you propose, I assume the result should be:
> > >
> > >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> > >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> > >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA18
> > >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA19
> > >
> > >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> > >   Pinctrl maps:
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type MUX_GROUP (2)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   group PA28
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   pin PA28
> > >   config 00010003
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type MUX_GROUP (2)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   group PA18
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@fc038000
> > >   pin PA18
> > >   config 00010003
> > >
> > >I think it is more difficult to understand what is done here.
> > 
> > I don't think I agree. The HW level groups are the individual pins, so I
> > think the second option is clearer and more correct. What is the "sdmmc1_0"
> > group in the first example? Does any such thing even exist in HW?
> 
> If a user see this, I will have to take the datasheet to undersand what
> using function E on this pin really means.
> 
> sdmmc1_0 group doesn't really exists as a group but a collection of
> pins. The minimum requirement is to have CK, CMD and DAT0 signals. The
> user can choose if he wants to use DAT1,2,3,4,5,6,7 and CD. Maybe he
> would like to keep this pins available for another function.
> 
> This is what I have in my device tree.
> 
> pinctrl@fc038000 {
> 	group_defs {
> 		sdmmc1_0 {
> 			pins = <PIN_PA22__SDMMC1_CK>,
> 			       <PIN_PA28__SDMMC1_CMD>,
> 			       <PIN_PA18__SDMMC1_DAT0>,
> 			       <PIN_PA19__SDMMC1_DAT1>,
> 			       <PIN_PA20__SDMMC1_DAT2>,
> 			       <PIN_PA21__SDMMC1_DAT3>,
> 			       <PIN_PA30__SDMMC1_CD>;
> 		};
> 	};
> 
> 	pinctrl_sdmmc1_default: sdmmc1_default {
> 		mux {
> 			function = "E";
> 			groups = "sdmmc1_0";
> 		};
> 
> 		conf-cmd_data {
> 			pins = <PIN_PA28__SDMMC1_CMD>,
> 			       <PIN_PA18__SDMMC1_DAT0>,
> 			       <PIN_PA19__SDMMC1_DAT1>,
> 			       <PIN_PA20__SDMMC1_DAT2>,
> 			       <PIN_PA21__SDMMC1_DAT3>;
> 			bias-pull-up;
> 		};
> 
> 		conf-ck_cd {
> 			pins = <PIN_PA22__SDMMC1_CK>,
> 			       <PIN_PA30__SDMMC1_CD>;
> 			bias-disable;
> 		};
> 	};
> }
> 
> From my point of view, it is not so far from what generic pinconf does.
> The only addition is the group_defs node because I don't want SoC
> dependant tables in my driver.

+1 for not having SoC dependent tables in the driver. This additional
indirection makes the pinctrl drivers quite big and hard to follow.

However, I don't understand why you need the group_defs node. I can
understand why having this may be helpful in the code, but otherwise it
doesn't contain any new information. It basically collects all pins also
contained in the sdmmc1_default node, so this information can be
retrieved there. The "Function E" seems rather uninteresting since not
all pins have to be configured in function E. I bet it's possible to
implement card detection as gpio and then you'll have PIN_PA30__GPIO_X_Y
which is not function E.

So my take is: remove the group_defs node.

Note that in the recently introduced Mediatek pinctrl driver we used
'pinmux' for the property that you name 'pins' here. We probably want to
use the same name.

Sascha
Sascha Hauer July 14, 2015, 6:54 a.m. UTC | #9
On Mon, Jul 13, 2015 at 02:07:45PM +0200, Linus Walleij wrote:
> On Tue, Jun 30, 2015 at 11:17 AM, Nicolas Ferre <nicolas.ferre@atmel.com> wrote:
> 
> > - no agreement on 3 points:
> > 1/ ways to use groups in generic pinctrl
> > 2/ ways to describe a comprehensive configuration in device tree
> > 3/ readability of a sysfs information
> >
> > - no way out on the generic pinctrl little changes that Ludovic proposed
> > as Linus W. never gave his point of view (RFC posts on April the 2nd).
> 
> Yeah I know. I am battling with this, it is one of those topics
> where you feel "eeeehhhh" and try to avoid looking at it. Sadly
> I have to...
> 
> I refer to Documentation/ManagementStyle, chapter 1,
> "Decisions".
> 
> What makes me feel uneasy about these things is that the decisions
> are irreversible due to the nature of the device tree bindings.
> 
> I am not a manager, but a maintainer...
> 
> > Ludovic explained at length our point of view and gave detailed
> > technical arguments. We don't intend to convince you, we just would like
> > the harmless modifications to be integrated.
> 
> So is it universally agreed that the changes are harmless?
> 
> If they are harmless, I would be able to revert the patch and
> nothing breaks in the world, I don't think that is the case. The
> case is a piece of code and functionality that is not AT91-specific
> but has to be maintained in the core pinctrl code forever.

If this change turns out to be wrong it doesn't necessarily have to stay
in the core pinctrl code. It can be pushed back into the AT91 driver
(and the others using it), won't be used for new bindings and after some
years can be pushed into some dark corner.

> 
> That is basically the big problem with anything device tree.
> It etches stuff in stone so it can't be changed, like ever.
> Further, the decision on whether to etch this or that is
> pushed to Linux subsystem maintainers, who are clearly
> unsuited for the task. :(
> 
> > As we preferred to give a chance to the generic pinconf/pinctrl for our
> > use by adding a little bit of flexibility, we are now in a situation
> > where we are nearly obliged to give up this approach and write a new
> > driver without the use of the generic facilities: what a pity!
> > We lost several months of useless work to match what we thought the
> > maintainer would prefer.
> >
> > So Linus, do you confirm that we won't go further with this approach?
> >
> > We are pretty disappointed by the way this interaction with the pinctrl
> > sub-system went.
> 
> I'm sorry, I am just trying to wait for consensus but it seems to
> be hard for that to happen.
> 
> Things I need:
> 
> - More DT bindings people to look at this patch.
> 
> - More other driver maintainers to look at this patch.
>   Sacha is one of those I'd like to get an opinion from
>   for example. ACKs, Reviewed-by's are always good.

I welcome the addition of the combined pin number / mux value
properties like we have in the Mediatek driver. Making the pinctrl
core aware of these defines like Ludovic did is a good idea IMO.

Sascha with two 's'
ludovic.desroches@atmel.com July 15, 2015, 7:46 a.m. UTC | #10
On Tue, Jul 14, 2015 at 07:57:49AM +0200, Sascha Hauer wrote:
> On Thu, Jun 18, 2015 at 02:33:48PM +0200, Ludovic Desroches wrote:
> > On Wed, Jun 17, 2015 at 09:55:56AM -0600, Stephen Warren wrote:
> > > On 06/17/2015 06:38 AM, Ludovic Desroches wrote:
> > > >Hi Stephen,
> > > >
> > > >On Mon, Jun 15, 2015 at 09:58:05AM -0600, Stephen Warren wrote:
> > > >>On 06/10/2015 09:04 AM, Ludovic Desroches wrote:
> > > >>>When having a controller which allows per pin muxing, declaring with
> > > >>>which groups a function can be used is a useless constraint since groups
> > > >>>are something virtual.
> > > >>
> > > >>This isn't true.
> > > >>
> > > >>Irrespective of whether a particular piece of pinmux HW can control the mux
> > > >>function for each pin individually, or only in groups, it's quite likely
> > > >>that each function can only be selected onto a subset of those pins or
> > > >>groups. Requiring the pinctrl driver to inform the core which set of
> > > >>pins/groups particular functions can be selected onto seems quite
> > > >>reasonable.
> > > >>
> > > >>In my opinion at least, for HW that can select the mux function at the
> > > >>per-pin level, the only sensible set of groups is one group per pin with
> > > >>each group containing a single pin. Any other use of groups is a
> > > >>SW/user-level construct, and is something unrelated to why the pinctrl
> > > >>subsystem supports groups. If we want to represent those groups in pinctrl,
> > > >>there should be two separate sets of groups; one to represent the actual HW
> > > >>capabilities, and one to represent the SW/user-level convenience
> > > >>abstractions.
> > > >
> > > >Groups that I would like to use are not something related to the user or
> > > >software. It's an hardware reality but they would be more flexibles.
> > > >
> > > >Usually the muxing is done by selecting a function (which seems to be
> > > >device related: usart, spi, etc.), then you select on which pins you
> > > >want this function.
> > > >
> > > >In my case, I can't select a function only by choosing a mux. It is a
> > > >combination of the mux and the pin on which it is applied. So my
> > > >functions will be GPIO, A, B, C, etc. If I use function A on pin 12, I
> > > >will have my i2c clock signal but I can have this signal on pin 58 if I
> > > >use function C. Do you understand what I mean? It's not very easy to
> > > >explain... So here is a real example:
> > > >
> > > >  --------------------------------------------------
> > > >|              | pio peripheral                    |
> > > >  --------------------------------------------------
> > > >| signal | dir | func | signal       | dir | ioset |
> > > >  --------------------------------------------------
> > > >| PA8    | I/O | A    | SDMMC0_DAT6  | I/O | 1     |
> > > >|        |     | B    | QSPI1_IO1    | I/O | 1     |
> > > >|        |     | D    | TCLK5        | I   | 1     |
> > > >|        |     | E    | FLEXCOM2_IO2 | I/O | 1     |
> > > >|        |     | F    | NWE/NANDWE   | O   | 2     |
> > > >  --------------------------------------------------
> > > >| PD28   | I/O | A    | SPI1_NPCS0   | I/O | 3     |
> > > >|        |     | B    | TDI          | I/O | 3     |
> > > >|        |     | C    | FLEXCOM2_IO2 | I   | 2     |
> > > >  --------------------------------------------------
> > > >
> > > >
> > > >You are right that having a group per pin is a solution.
> > > >
> > > >If I follow your proposal (tell me if I'm wrong):
> > > >- I will have 128 groups since I have 128 pins.
> > > 
> > > Yes.
> > > 
> > > >- I will have functions GPIO, A, B, C, D, E, F.
> > > 
> > > You could have functions A..F, and require the user to determine what option
> > > they want for each pin, find the pin-specific mux function value for each
> > > pin, and put that into the DT (or other pinmux data source). For example,
> > > the bcm2835 pinctrl driver works this way.
> > > 
> > > In that case, each of the functions A..F could be selected on each pin, so
> > > you'd have a very simple "get pins for function" implementation.
> > > 
> > > Alternatively, you could define a logic function per IO controller or signal
> > > that gets pinmuxed. In the example above, FLEXCOM2_IO2 is one such example.
> > > Given that set of functions, you'd need a mapping table to convert from the
> > > logical functions seen by the pinctrl subsystem to the HW values that need
> > > to be written into registers. For example, the Tegra pinctrl drivers work
> > > this way.
> > > 
> > > In that case, each (pinctrl) function could only be selected on a specific
> > > subset of all pins/groups, and so you'd probably need a table-based
> > > implementation of "get pins for function".
> > 
> > Thanks for giving me some examples. Let's take a look at these drivers.
> > My concern is that I didnn't want to have many and/or big tables in my
> > driver. I will have to update it for a new SoC even if the pin
> > controller is the same. I prefer to have it in the device as we did
> > before and as some drivers continue to do.
> > 
> > > 
> > > >- I have to give the groups which can achieve a function so I will have
> > > >128 groups for each function. It means 128 x 7 = 896 groups.
> > > 
> > > I don't think so no. I'm not sure why you'd consider multiplying 128 and 7
> > > here. I'd expect 128 groups since you have 128 pins[1].
> > > 
> > 
> > Sorry my mistake, I mean 896 possibilites since I have 128 groups for a
> > function.
> > 
> > > Well, it's possible to have slightly more groups if, say, mux function is
> > > selectable per pin, whereas something else like drive strength is configured
> > > by a register that affects multiple pins at once. You'd need separate sets
> > > of groups for muxing and for drive strength configuration. Some Tegra SoCs
> > > are like this. Still, we just add the different sets of groups together
> > > here, not multiply. The overall set of groups is not that much larger than
> > > the set of pins.
> > > 
> > > >Does it seems to be something reasonable and intelligible? From my point
> > > >of view no. And what about the sysfs readability?
> > > >
> > > >With my current implementation, I have something quite understandable
> > > >for the user if he needs to check the pinmuxing:
> > > >
> > > >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> > > >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> > > >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> > > >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group sdmmc1_0
> > > >
> > > >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> > > >   Pinctrl maps:
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type MUX_GROUP (2)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   group sdmmc1_0
> > > >   function E
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type CONFIGS_PIN (3)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   pin PA28
> > > >   config 00010003
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type CONFIGS_PIN (3)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   pin PA18
> > > >   config 00010003
> > > >
> > > >
> > > >Doing as you propose, I assume the result should be:
> > > >
> > > >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@fc038000/pinmux-pins
> > > >   pin 17 (PA17): (MUX UNCLAIMED) (GPIO UNCLAIMED)
> > > >   pin 18 (PA18): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA18
> > > >   pin 19 (PA19): b0000000.sdio-host (GPIO UNCLAIMED) function E group PA19
> > > >
> > > >   # cat /sys/kernel/debug/pinctrl/pinctrl-maps
> > > >   Pinctrl maps:
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type MUX_GROUP (2)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   group PA28
> > > >   function E
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type CONFIGS_PIN (3)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   pin PA28
> > > >   config 00010003
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type MUX_GROUP (2)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   group PA18
> > > >   function E
> > > >
> > > >   device b0000000.sdio-host
> > > >   state default
> > > >   type CONFIGS_PIN (3)
> > > >   controlling device ahb:apb:pinctrl@fc038000
> > > >   pin PA18
> > > >   config 00010003
> > > >
> > > >I think it is more difficult to understand what is done here.
> > > 
> > > I don't think I agree. The HW level groups are the individual pins, so I
> > > think the second option is clearer and more correct. What is the "sdmmc1_0"
> > > group in the first example? Does any such thing even exist in HW?
> > 
> > If a user see this, I will have to take the datasheet to undersand what
> > using function E on this pin really means.
> > 
> > sdmmc1_0 group doesn't really exists as a group but a collection of
> > pins. The minimum requirement is to have CK, CMD and DAT0 signals. The
> > user can choose if he wants to use DAT1,2,3,4,5,6,7 and CD. Maybe he
> > would like to keep this pins available for another function.
> > 
> > This is what I have in my device tree.
> > 
> > pinctrl@fc038000 {
> > 	group_defs {
> > 		sdmmc1_0 {
> > 			pins = <PIN_PA22__SDMMC1_CK>,
> > 			       <PIN_PA28__SDMMC1_CMD>,
> > 			       <PIN_PA18__SDMMC1_DAT0>,
> > 			       <PIN_PA19__SDMMC1_DAT1>,
> > 			       <PIN_PA20__SDMMC1_DAT2>,
> > 			       <PIN_PA21__SDMMC1_DAT3>,
> > 			       <PIN_PA30__SDMMC1_CD>;
> > 		};
> > 	};
> > 
> > 	pinctrl_sdmmc1_default: sdmmc1_default {
> > 		mux {
> > 			function = "E";
> > 			groups = "sdmmc1_0";
> > 		};
> > 
> > 		conf-cmd_data {
> > 			pins = <PIN_PA28__SDMMC1_CMD>,
> > 			       <PIN_PA18__SDMMC1_DAT0>,
> > 			       <PIN_PA19__SDMMC1_DAT1>,
> > 			       <PIN_PA20__SDMMC1_DAT2>,
> > 			       <PIN_PA21__SDMMC1_DAT3>;
> > 			bias-pull-up;
> > 		};
> > 
> > 		conf-ck_cd {
> > 			pins = <PIN_PA22__SDMMC1_CK>,
> > 			       <PIN_PA30__SDMMC1_CD>;
> > 			bias-disable;
> > 		};
> > 	};
> > }
> > 
> > From my point of view, it is not so far from what generic pinconf does.
> > The only addition is the group_defs node because I don't want SoC
> > dependant tables in my driver.
> 
> +1 for not having SoC dependent tables in the driver. This additional
> indirection makes the pinctrl drivers quite big and hard to follow.
> 
> However, I don't understand why you need the group_defs node. I can
> understand why having this may be helpful in the code, but otherwise it
> doesn't contain any new information. It basically collects all pins also
> contained in the sdmmc1_default node, so this information can be
> retrieved there.

You are right, I don't really need the group_defs node, I did it in this way
because I wanted to split pinmux and pinconf and I didn't want to
implement my own dt_node_to_map() function but to use the
pinconf_generic_dt_node_to_map_all() one.

> The "Function E" seems rather uninteresting since not
> all pins have to be configured in function E. I bet it's possible to
> implement card detection as gpio and then you'll have PIN_PA30__GPIO_X_Y
> which is not function E.

No with our new sdhci device, card detection is not a gpio. Groups
definition has to be done in order to have the same function for all the
pins of the group.

A full example here:
https://github.com/linux4sam/linux-at91/blob/master/arch/arm/boot/dts/at91-sama5d2_xplained.dts

> 
> So my take is: remove the group_defs node.
> 
> Note that in the recently introduced Mediatek pinctrl driver we used
> 'pinmux' for the property that you name 'pins' here. We probably want to
> use the same name.

This driver fits most of my needs but I didn't do it in this way for the
two previous reasons. If it is not an issue to add a new
dt_node_to_map() implementation which should be quite close to the
mediatek one, let's do it.

Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ludovic.desroches@atmel.com July 15, 2015, 8:29 a.m. UTC | #11
On Wed, Jul 15, 2015 at 09:46:42AM +0200, Ludovic Desroches wrote:
> This driver fits most of my needs but I didn't do it in this way for the
> two previous reasons. If it is not an issue to add a new
> dt_node_to_map() implementation which should be quite close to the
> mediatek one, let's do it.

Thinking more about it, there is something I have missed that can change
many things is that one pin is one group in Mediatek driver.

If I add the pin mux value in my macros with the pin number and the
ioset, group_defs node is still useful to declare group of pins.

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij July 27, 2015, 9:43 a.m. UTC | #12
On Wed, Jul 15, 2015 at 9:46 AM, Ludovic Desroches
<ludovic.desroches@atmel.com> wrote:
> On Tue, Jul 14, 2015 at 07:57:49AM +0200, Sascha Hauer wrote:
>>
>> Note that in the recently introduced Mediatek pinctrl driver we used
>> 'pinmux' for the property that you name 'pins' here. We probably want to
>> use the same name.
>
> This driver fits most of my needs but I didn't do it in this way for the
> two previous reasons. If it is not an issue to add a new
> dt_node_to_map() implementation which should be quite close to the
> mediatek one, let's do it.

OK if you can do that so we have some order and obtain Sascha's
ACK I'm in for the patch.

Maybe there is something like that further up in my mailbox, need
to browse :)

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
ludovic.desroches@atmel.com July 27, 2015, 12:12 p.m. UTC | #13
On Mon, Jul 27, 2015 at 11:43:52AM +0200, Linus Walleij wrote:
> On Wed, Jul 15, 2015 at 9:46 AM, Ludovic Desroches
> <ludovic.desroches@atmel.com> wrote:
> > On Tue, Jul 14, 2015 at 07:57:49AM +0200, Sascha Hauer wrote:
> >>
> >> Note that in the recently introduced Mediatek pinctrl driver we used
> >> 'pinmux' for the property that you name 'pins' here. We probably want to
> >> use the same name.
> >
> > This driver fits most of my needs but I didn't do it in this way for the
> > two previous reasons. If it is not an issue to add a new
> > dt_node_to_map() implementation which should be quite close to the
> > mediatek one, let's do it.
> 
> OK if you can do that so we have some order and obtain Sascha's
> ACK I'm in for the patch.
> 
> Maybe there is something like that further up in my mailbox, need
> to browse :)

I am doing the changes and merging my gpio driver into it. I'll send a
new version probably this week.

Regards

Ludovic
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index b874458..15fe3f7 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -40,7 +40,7 @@  int pinmux_check_ops(struct pinctrl_dev *pctldev)
 	if (!ops ||
 	    !ops->get_functions_count ||
 	    !ops->get_function_name ||
-	    !ops->get_function_groups ||
+	    (!ops->get_function_groups & !ops->mux_per_pin) ||
 	    !ops->set_mux) {
 		dev_err(pctldev->dev, "pinmux ops lacks necessary functions\n");
 		return -EINVAL;
@@ -338,36 +338,42 @@  int pinmux_map_to_setting(struct pinctrl_map const *map,
 	}
 	setting->data.mux.func = ret;
 
-	ret = pmxops->get_function_groups(pctldev, setting->data.mux.func,
-					  &groups, &num_groups);
-	if (ret < 0) {
-		dev_err(pctldev->dev, "can't query groups for function %s\n",
-			map->data.mux.function);
-		return ret;
-	}
-	if (!num_groups) {
-		dev_err(pctldev->dev,
-			"function %s can't be selected on any group\n",
-			map->data.mux.function);
-		return -EINVAL;
-	}
-	if (map->data.mux.group) {
-		bool found = false;
-		group = map->data.mux.group;
-		for (i = 0; i < num_groups; i++) {
-			if (!strcmp(group, groups[i])) {
-				found = true;
-				break;
-			}
+	if (!pmxops->mux_per_pin) {
+		ret = pmxops->get_function_groups(pctldev,
+						  setting->data.mux.func,
+						  &groups, &num_groups);
+		if (ret < 0) {
+			dev_err(pctldev->dev,
+				"can't query groups for function %s\n",
+				map->data.mux.function);
+			return ret;
 		}
-		if (!found) {
+		if (!num_groups) {
 			dev_err(pctldev->dev,
-				"invalid group \"%s\" for function \"%s\"\n",
-				group, map->data.mux.function);
+				"function %s can't be selected on any group\n",
+				map->data.mux.function);
 			return -EINVAL;
 		}
+		if (map->data.mux.group) {
+			bool found = false;
+			group = map->data.mux.group;
+			for (i = 0; i < num_groups; i++) {
+				if (!strcmp(group, groups[i])) {
+					found = true;
+					break;
+				}
+			}
+			if (!found) {
+				dev_err(pctldev->dev,
+					"invalid group \"%s\" for function \"%s\"\n",
+					group, map->data.mux.function);
+				return -EINVAL;
+			}
+		} else {
+			group = groups[0];
+		}
 	} else {
-		group = groups[0];
+		group = map->data.mux.group;
 	}
 
 	ret = pinctrl_get_group_selector(pctldev, group);
diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h
index 511bda9..51b84eb 100644
--- a/include/linux/pinctrl/pinmux.h
+++ b/include/linux/pinctrl/pinmux.h
@@ -23,6 +23,8 @@  struct pinctrl_dev;
 /**
  * struct pinmux_ops - pinmux operations, to be implemented by pin controller
  * drivers that support pinmuxing
+ * @mux_per_pin: in case of per pin muxing, it removes the need to declare
+ *	with which groups a function can be used.
  * @request: called by the core to see if a certain pin can be made
  *	available for muxing. This is called by the core to acquire the pins
  *	before selecting any actual mux setting across a function. The driver
@@ -58,6 +60,7 @@  struct pinctrl_dev;
  *	to the GPIO controllers that need pin muxing.
  */
 struct pinmux_ops {
+	bool mux_per_pin;
 	int (*request) (struct pinctrl_dev *pctldev, unsigned offset);
 	int (*free) (struct pinctrl_dev *pctldev, unsigned offset);
 	int (*get_functions_count) (struct pinctrl_dev *pctldev);