Re: [RESEND PATCH 1/2] pinctrl: change function behavior for per pin muxing controllers

Ludovic Desroches <ludovic.desroches@xxxxxxxxx> · Wed, 17 Jun 2015 14:38:16 +0200

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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html