Message-ID: <20150714055749.GB5161@pengutronix.de>
Date:	Tue, 14 Jul 2015 07:57:49 +0200
From:	Sascha Hauer <s.hauer@...gutronix.de>
To:	Stephen Warren <swarren@...dotorg.org>,
	linux-arm-kernel@...ts.infradead.org, linux-gpio@...r.kernel.org,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org,
	linus.walleij@...aro.org, nicolas.ferre@...el.com
Subject: Re: [RESEND PATCH 1/2] pinctrl: change function behavior for per pin
 muxing controllers

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@...38000/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@...38000
> > >   group sdmmc1_0
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@...38000
> > >   pin PA28
> > >   config 00010003
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@...38000
> > >   pin PA18
> > >   config 00010003
> > >
> > >
> > >Doing as you propose, I assume the result should be:
> > >
> > >   # cat /sys/kernel/debug/pinctrl/ahb\:apb\:pinctrl@...38000/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@...38000
> > >   group PA28
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@...38000
> > >   pin PA28
> > >   config 00010003
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type MUX_GROUP (2)
> > >   controlling device ahb:apb:pinctrl@...38000
> > >   group PA18
> > >   function E
> > >
> > >   device b0000000.sdio-host
> > >   state default
> > >   type CONFIGS_PIN (3)
> > >   controlling device ahb:apb:pinctrl@...38000
> > >   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@...38000 {
> 	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

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists