* Pinmux bindings proposal
@ 2012-01-13 20:39 Stephen Warren
2012-01-14 7:09 ` Shawn Guo
` (4 more replies)
0 siblings, 5 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-13 20:39 UTC (permalink / raw)
To: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng
Cc: linux-kernel, linux-arm-kernel, devicetree-discuss
I thought a bit more about pinmux DT bindings. I came up with something
that I like well enough, and is pretty similar to the binding that Dong
posted recently. I think it'll work for both Tegra's and IMX's needs.
Please take a look!
Note: I've used named constants below just to make this easier to read.
We still don't have a solution to actually use named constants in dtc yet.
tegra20.dtsi:
/ {
tegra_pmx: pinmux@70000000 {
compatible = "nvidia,tegra20-pinmux";
reg = <0x70000014 0x10 /* Tri-state registers */
0x70000080 0x20 /* Mux registers */
0x700000a0 0x14 /* Pull-up/down registers */
0x70000868 0xa8>; /* Pad control registers */
};
sdhci@c8000200 {
compatible = "nvidia,tegra20-sdhci";
reg = <0xc8000200 0x200>;
interrupts = <0 15 0x04>;
};
};
tegra-harmony.dts:
/{
sdhci@c8000200 {
cd-gpios = <&gpio 69 0>; /* gpio PI5 */
wp-gpios = <&gpio 57 0>; /* gpio PH1 */
power-gpios = <&gpio 155 0>; /* gpio PT3 */
/*
* A list of named configurations that this device needs.
* Format is a list of <"name" &phandle_of_pmx_configuration>
*
* Multiple "name"s are needed e.g. to support active/suspend,
* or whatever device-defined states are appropriate. The
* device defines which names are needed, just like a device
* defines which regulators, clocks, GPIOs, interrupts, ...
* it needs.
*
* This example shows a 1:1 relation between name and phandle.
* We might want a 1:n relation, so that we can blend multiple
* pre-defined sets of data together, e.g. one pre-defined set
* for the pin mux configuration, another for the pin config
* settings, both being put into the single "default" setting
* for this one device.
*
* A pinmux controller can contain this property too, to
* define "hogged" or "system" pin mux configuration.
*
* Note: Mixing strings and integers in a property seems
* unusual. However, I have seen other bindings floating
* around that are starting to do this...
*/
pinmux =
<"default" &pmx_sdhci_active>
<"suspend" &pmx_sdhci_suspend>;
/* 1:n example: */
pinmux =
<"default" &pmx_sdhci_mux_a>
<"default" &pmx_sdhci_pincfg_a>
<"suspend" &pmx_sdhci_mux_a>
<"suspend" &pmx_sdhci_pincfg_a_suspend>;
/*
* Alternative: One property for each required state. But,
* how does pinctrl core know which properties to parse?
* Every property named "pinctrl*" seems a little too far-
* reaching. Perhaps if we used vendor-name "pinmux", that'd
* be OK, i.e. pinmux,default and pinmux,suspend?
*/
pinmux = <&pmx_sdhci_active>;
pinmux-suspend <&pmx_sdhci_suspend>;
/* 1:n example: */
pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
pinmux-suspend = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a_suspend>;
/*
* The actual definition of the complete state of the
* pinmux as required by some driver.
*
* These can be either directly in the device node, or
* somewhere in tegra20.dtsi in order to provide pre-
* selected/common configurations. Hence, they're referred
* to by phandle above.
*/
pmx_sdhci_active: {
/*
* Pin mux settings. Mandatory?
* Not mandatory if the 1:1 mentioned above is
* extended to 1:n.
*
* Format is <&pmx_controller_phandle muxable_entity_id
* selected_function>.
*
* The pmx phandle is required since there may be more
* than one pinmux controller in the system. Even if
* this node is inside the pinmux controller itself, I
* think it's simpler to just always have this field
* present in the binding for consistency.
*
* Alternative: Format is <&pmx_controller_phandle
* pmx_controller_specific_data>. In this case, the
* pmx controller needs to define #pinmux-mux-cells,
* and provide the pinctrl core with a mapping
* function to handle the rest of the data in the
* property. This is how GPIOs and interrupts work.
* However, this will probably interact badly with
* wanting to parse the entire pinmux map early in
* boot, when perhaps the pinctrl core is initialized,
* but the pinctrl driver itself is not.
*/
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
/* Syntax example */
<&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
/*
* Pin configuration settings. Optional.
*
* Format is <&pmx_controller_phandle muxable_entity_id
* configuration_option configuration_value>.
*/
config =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
/*
* Perhaps allow additional custom properties here to
* express things we haven't thought of. The pinctrl
* drivers would be responsible for parsing them.
*/
};
pmx_sdhci_standby: {
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
config =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
};
};
};
Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
TEGRA_PMX_PG_DTA
TEGRA_PMX_PG_DTD
Each individual pinmux driver's bindings needs to define what each integer
ID represents.
Integer IDs for the "mux functions". Note that these are the raw values
written into hardware, not any driver-defined abstraction, and not any
kind of "virtual group" that's been invented to make OEMs life easier:
TEGRA_PMX_MUX_0
TEGRA_PMX_MUX_1
...
TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
Since these are the raw IDs that go into HW, there's no need to specify
each ID's meanings in the binding.
Integer IDs for "pin config parameters":
TEGRA_PMX_CONF_TRISTATE
TEGRA_PMX_CONF_DRIVE_STRENGTH
TEGRA_PMX_CONF_SLEW_RATE
Each individual pinmux driver's bindings needs to define what each integer
ID represents, and what the legal "value"s are for each one.
Advantages:
* Provides for both mux settings and "pin configuration".
* Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
file if desired to provide pre-defined pinmux configurations to
choose from.
* Allows the "pinmux configuration" nodes to be part of the per-device
node if you don't want to use pre-defined configurations.
* When pre-defined configurations are present, if you need something
custom, you can do it easily.
* Can augment pre-defined configurations by listing n nodes for each
"name"d pinmux configuration, e.g. to add one extra pin config
value.
* Parsing is still quite simple:
1) Parse "pinmux" property in device node to get phandle.
2) Parse "mux" property in the node reference by the phandle,
splitting into a list of pmx phandle, entity, mux func.
3) For each entry, pass entity, function to the appropriate mux
driver. (For U-Boot, this might mean check that the phandle
points at the expected place, and ignore the entry if not?)
4) Mux driver simply converts "muxable entity" to the register
address, write the "function" value straight to the register.
Disadvantages:
* If you're not using pre-defined configurations, you still have to dump
all the pinmux configuration into a sub-node of the device node, and
have a property point at it using a phandle. This is slightly more
complex than simply putting the mux/config properties right into the
device node. However, it additionally allows one to have multiple
"name"d configurations (e.g. for suspend) very easily, and isn't overly
complex, so I can live with this.
Changes to pinctrl subsystem:
Very little, I think:
* Need to pass raw function IDs through to the driver instead of the driver
defining some logical table. Actually, this is just a change to individual
drivers, since they can define the functions "func0", "func1", ... "funcn"
as I mentioned before.
* Need to add the code to actually parse this of course.
One additional thought: If dtc does grow named constants, we can provide
HW-function-based names for the mux functions, e.g.:
TEGRA_PMX_DTA_RSVD0 0
TEGRA_PMX_DTA_SDIO2 1
TEGRA_PMX_DTA_VI 2
TEGRA_PMX_DTA_RSVD3 3
TEGRA_PMX_DTF_I2C3 0
TEGRA_PMX_DTF_RSVD1 1
TEGRA_PMX_DTF_VI 2
TEGRA_PMX_DTF_RSVD3 3
...
That'd allow the .dts to include functionality-based named for the mux
function selection, but the .dtb to still include the raw HW mux field
values. And this is something completely within the control of the SoC
.dtsi file; if you don't like it, you don't have to do it.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
@ 2012-01-14 7:09 ` Shawn Guo
2012-01-17 18:47 ` Stephen Warren
2012-01-16 12:50 ` Dong Aisheng-B29396
` (3 subsequent siblings)
4 siblings, 1 reply; 54+ messages in thread
From: Shawn Guo @ 2012-01-14 7:09 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> I thought a bit more about pinmux DT bindings. I came up with something
> that I like well enough, and is pretty similar to the binding that Dong
> posted recently. I think it'll work for both Tegra's and IMX's needs.
> Please take a look!
>
Thanks for doing this. It's great we are approaching some level of
agreement on the binding. I have a few comments below. Other than
those, this looks like a pretty sensible pinctrl DT binding to me.
> Note: I've used named constants below just to make this easier to read.
> We still don't have a solution to actually use named constants in dtc yet.
>
> tegra20.dtsi:
>
> / {
> tegra_pmx: pinmux@70000000 {
> compatible = "nvidia,tegra20-pinmux";
> reg = <0x70000014 0x10 /* Tri-state registers */
> 0x70000080 0x20 /* Mux registers */
> 0x700000a0 0x14 /* Pull-up/down registers */
> 0x70000868 0xa8>; /* Pad control registers */
> };
>
> sdhci@c8000200 {
> compatible = "nvidia,tegra20-sdhci";
> reg = <0xc8000200 0x200>;
> interrupts = <0 15 0x04>;
> };
> };
>
> tegra-harmony.dts:
>
> /{
> sdhci@c8000200 {
> cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> wp-gpios = <&gpio 57 0>; /* gpio PH1 */
> power-gpios = <&gpio 155 0>; /* gpio PT3 */
>
> /*
> * A list of named configurations that this device needs.
> * Format is a list of <"name" &phandle_of_pmx_configuration>
> *
> * Multiple "name"s are needed e.g. to support active/suspend,
> * or whatever device-defined states are appropriate. The
> * device defines which names are needed, just like a device
> * defines which regulators, clocks, GPIOs, interrupts, ...
> * it needs.
> *
> * This example shows a 1:1 relation between name and phandle.
> * We might want a 1:n relation, so that we can blend multiple
> * pre-defined sets of data together, e.g. one pre-defined set
> * for the pin mux configuration, another for the pin config
> * settings, both being put into the single "default" setting
> * for this one device.
> *
> * A pinmux controller can contain this property too, to
> * define "hogged" or "system" pin mux configuration.
> *
> * Note: Mixing strings and integers in a property seems
> * unusual. However, I have seen other bindings floating
> * around that are starting to do this...
> */
> pinmux =
I prefer to have the property named 'pinctrl' than 'pinmux'.
> <"default" &pmx_sdhci_active>
> <"suspend" &pmx_sdhci_suspend>;
>
I would rather do something like what clock DT binding proposal is
doing.
pinctrl = <&pmx_sdhci_active>, <&pmx_sdhci_suspend>;
pinctrl-names = "default", "suspend";
> /* 1:n example: */
> pinmux =
> <"default" &pmx_sdhci_mux_a>
> <"default" &pmx_sdhci_pincfg_a>
> <"suspend" &pmx_sdhci_mux_a>
> <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>
I personally do not like the 1:n binding. To me, any particular pinctrl
configuration, e.g. pmx_sdhci_active, should consist of a pair of pinmux
and pinconf, which should be given by the pmx_sdhci_active node.
> /*
> * Alternative: One property for each required state. But,
> * how does pinctrl core know which properties to parse?
> * Every property named "pinctrl*" seems a little too far-
> * reaching. Perhaps if we used vendor-name "pinmux", that'd
> * be OK, i.e. pinmux,default and pinmux,suspend?
> */
> pinmux = <&pmx_sdhci_active>;
> pinmux-suspend <&pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
> pinmux-suspend = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a_suspend>;
>
> /*
> * The actual definition of the complete state of the
> * pinmux as required by some driver.
> *
> * These can be either directly in the device node, or
> * somewhere in tegra20.dtsi in order to provide pre-
> * selected/common configurations. Hence, they're referred
> * to by phandle above.
> */
> pmx_sdhci_active: {
> /*
> * Pin mux settings. Mandatory?
> * Not mandatory if the 1:1 mentioned above is
> * extended to 1:n.
> *
> * Format is <&pmx_controller_phandle muxable_entity_id
> * selected_function>.
> *
> * The pmx phandle is required since there may be more
> * than one pinmux controller in the system. Even if
> * this node is inside the pinmux controller itself, I
> * think it's simpler to just always have this field
> * present in the binding for consistency.
> *
I prefer to just put such nodes inside pinctrl controller itself and
drop those phandles.
> * Alternative: Format is <&pmx_controller_phandle
> * pmx_controller_specific_data>. In this case, the
> * pmx controller needs to define #pinmux-mux-cells,
> * and provide the pinctrl core with a mapping
> * function to handle the rest of the data in the
> * property. This is how GPIOs and interrupts work.
> * However, this will probably interact badly with
> * wanting to parse the entire pinmux map early in
> * boot, when perhaps the pinctrl core is initialized,
> * but the pinctrl driver itself is not.
> */
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> /* Syntax example */
> <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> /*
> * Pin configuration settings. Optional.
> *
I guess pinconf can be optional because some pin/group that have pinmux
setting do not necessarily have pinconf setting? If that's case,
yes, agreed.
> * Format is <&pmx_controller_phandle muxable_entity_id
> * configuration_option configuration_value>.
> */
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> /*
> * Perhaps allow additional custom properties here to
> * express things we haven't thought of. The pinctrl
> * drivers would be responsible for parsing them.
> */
> };
> pmx_sdhci_standby: {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
> };
> };
>
> Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
>
> TEGRA_PMX_PG_DTA
> TEGRA_PMX_PG_DTD
>
> Each individual pinmux driver's bindings needs to define what each integer
> ID represents.
>
> Integer IDs for the "mux functions". Note that these are the raw values
> written into hardware, not any driver-defined abstraction, and not any
> kind of "virtual group" that's been invented to make OEMs life easier:
>
> TEGRA_PMX_MUX_0
> TEGRA_PMX_MUX_1
> ...
> TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
>
> Since these are the raw IDs that go into HW, there's no need to specify
> each ID's meanings in the binding.
>
> Integer IDs for "pin config parameters":
>
> TEGRA_PMX_CONF_TRISTATE
> TEGRA_PMX_CONF_DRIVE_STRENGTH
> TEGRA_PMX_CONF_SLEW_RATE
>
> Each individual pinmux driver's bindings needs to define what each integer
> ID represents, and what the legal "value"s are for each one.
>
Agreed on these. But we really need to push named constants support
for dtc, otherwise the binding is so difficult for engineering. (We
have a lot of pinconfig parameters on imx)
Regards,
Shawn
> Advantages:
>
> * Provides for both mux settings and "pin configuration".
>
> * Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
> file if desired to provide pre-defined pinmux configurations to
> choose from.
>
> * Allows the "pinmux configuration" nodes to be part of the per-device
> node if you don't want to use pre-defined configurations.
>
> * When pre-defined configurations are present, if you need something
> custom, you can do it easily.
>
> * Can augment pre-defined configurations by listing n nodes for each
> "name"d pinmux configuration, e.g. to add one extra pin config
> value.
>
> * Parsing is still quite simple:
> 1) Parse "pinmux" property in device node to get phandle.
> 2) Parse "mux" property in the node reference by the phandle,
> splitting into a list of pmx phandle, entity, mux func.
> 3) For each entry, pass entity, function to the appropriate mux
> driver. (For U-Boot, this might mean check that the phandle
> points at the expected place, and ignore the entry if not?)
> 4) Mux driver simply converts "muxable entity" to the register
> address, write the "function" value straight to the register.
>
> Disadvantages:
>
> * If you're not using pre-defined configurations, you still have to dump
> all the pinmux configuration into a sub-node of the device node, and
> have a property point at it using a phandle. This is slightly more
> complex than simply putting the mux/config properties right into the
> device node. However, it additionally allows one to have multiple
> "name"d configurations (e.g. for suspend) very easily, and isn't overly
> complex, so I can live with this.
>
> Changes to pinctrl subsystem:
>
> Very little, I think:
>
> * Need to pass raw function IDs through to the driver instead of the driver
> defining some logical table. Actually, this is just a change to individual
> drivers, since they can define the functions "func0", "func1", ... "funcn"
> as I mentioned before.
>
> * Need to add the code to actually parse this of course.
>
> One additional thought: If dtc does grow named constants, we can provide
> HW-function-based names for the mux functions, e.g.:
>
> TEGRA_PMX_DTA_RSVD0 0
> TEGRA_PMX_DTA_SDIO2 1
> TEGRA_PMX_DTA_VI 2
> TEGRA_PMX_DTA_RSVD3 3
>
> TEGRA_PMX_DTF_I2C3 0
> TEGRA_PMX_DTF_RSVD1 1
> TEGRA_PMX_DTF_VI 2
> TEGRA_PMX_DTF_RSVD3 3
> ...
>
> That'd allow the .dts to include functionality-based named for the mux
> function selection, but the .dtb to still include the raw HW mux field
> values. And this is something completely within the control of the SoC
> .dtsi file; if you don't like it, you don't have to do it.
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
2012-01-14 7:09 ` Shawn Guo
@ 2012-01-16 12:50 ` Dong Aisheng-B29396
2012-01-17 8:23 ` Shawn Guo
2012-01-17 19:09 ` Stephen Warren
2012-01-16 18:28 ` Grant Likely
` (2 subsequent siblings)
4 siblings, 2 replies; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-16 12:50 UTC (permalink / raw)
To: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng
Cc: linux-kernel, linux-arm-kernel, devicetree-discuss
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@nvidia.com]
> Sent: Saturday, January 14, 2012 4:40 AM
> To: Dong Aisheng-B29396; linus.walleij@stericsson.com; s.hauer@pengutronix.de;
> rob.herring@calxeda.com; kernel@pengutronix.de; cjb@laptop.org; Simon Glass
> (sjg@chromium.org); Dong Aisheng
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> devicetree-discuss@lists.ozlabs.org
> Subject: Pinmux bindings proposal
> Importance: High
>
> I thought a bit more about pinmux DT bindings. I came up with something that I
> like well enough, and is pretty similar to the binding that Dong posted recently.
> I think it'll work for both Tegra's and IMX's needs.
> Please take a look!
>
Thanks for doing this. :-)
A few comments:
> Note: I've used named constants below just to make this easier to read.
> We still don't have a solution to actually use named constants in dtc yet.
>
> tegra20.dtsi:
>
> / {
> tegra_pmx: pinmux@70000000 {
> compatible = "nvidia,tegra20-pinmux";
> reg = <0x70000014 0x10 /* Tri-state registers */
> 0x70000080 0x20 /* Mux registers */
> 0x700000a0 0x14 /* Pull-up/down registers */
> 0x70000868 0xa8>; /* Pad control registers */
> };
>
> sdhci@c8000200 {
> compatible = "nvidia,tegra20-sdhci";
> reg = <0xc8000200 0x200>;
> interrupts = <0 15 0x04>;
> };
> };
>
> tegra-harmony.dts:
>
> /{
> sdhci@c8000200 {
> cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> wp-gpios = <&gpio 57 0>; /* gpio PH1 */
> power-gpios = <&gpio 155 0>; /* gpio PT3 */
>
> /*
> * A list of named configurations that this device needs.
> * Format is a list of <"name" &phandle_of_pmx_configuration>
> *
> * Multiple "name"s are needed e.g. to support active/suspend,
> * or whatever device-defined states are appropriate. The
> * device defines which names are needed, just like a device
> * defines which regulators, clocks, GPIOs, interrupts, ...
> * it needs.
> *
> * This example shows a 1:1 relation between name and phandle.
> * We might want a 1:n relation, so that we can blend multiple
> * pre-defined sets of data together, e.g. one pre-defined set
> * for the pin mux configuration, another for the pin config
> * settings, both being put into the single "default" setting
> * for this one device.
> *
> * A pinmux controller can contain this property too, to
> * define "hogged" or "system" pin mux configuration.
> *
> * Note: Mixing strings and integers in a property seems
> * unusual. However, I have seen other bindings floating
> * around that are starting to do this...
> */
> pinmux =
> <"default" &pmx_sdhci_active>
> <"suspend" &pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux =
> <"default" &pmx_sdhci_mux_a>
> <"default" &pmx_sdhci_pincfg_a>
> <"suspend" &pmx_sdhci_mux_a>
> <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>
> /*
> * Alternative: One property for each required state. But,
> * how does pinctrl core know which properties to parse?
> * Every property named "pinctrl*" seems a little too far-
> * reaching. Perhaps if we used vendor-name "pinmux", that'd
> * be OK, i.e. pinmux,default and pinmux,suspend?
It we support whatever device-defined states, it's meaningless to use one property
For each required state since pinctrl core does not know the property name the
customer defined.
> */
> pinmux = <&pmx_sdhci_active>;
> pinmux-suspend <&pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
This looks ok to me since the mux and pincfg usually is 1 to 1.
And if we do not have a pincfg for a pinmux we can find a way to tell the pinctl
core, maybe just set pincfg phandle to 0.
> pinmux-suspend = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a_suspend>;
>
> /*
> * The actual definition of the complete state of the
> * pinmux as required by some driver.
> *
> * These can be either directly in the device node, or
> * somewhere in tegra20.dtsi in order to provide pre-
> * selected/common configurations. Hence, they're referred
> * to by phandle above.
> */
> pmx_sdhci_active: {
> /*
> * Pin mux settings. Mandatory?
Mandatory for what?
> * Not mandatory if the 1:1 mentioned above is
> * extended to 1:n.
> *
> * Format is <&pmx_controller_phandle muxable_entity_id
> * selected_function>.
> *
> * The pmx phandle is required since there may be more
> * than one pinmux controller in the system. Even if
> * this node is inside the pinmux controller itself, I
> * think it's simpler to just always have this field
> * present in the binding for consistency.
> *
> * Alternative: Format is <&pmx_controller_phandle
> * pmx_controller_specific_data>. In this case, the
> * pmx controller needs to define #pinmux-mux-cells,
> * and provide the pinctrl core with a mapping
> * function to handle the rest of the data in the
> * property. This is how GPIOs and interrupts work.
> * However, this will probably interact badly with
> * wanting to parse the entire pinmux map early in
> * boot, when perhaps the pinctrl core is initialized,
> * but the pinctrl driver itself is not.
> */
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> /* Syntax example */
> <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
I'm still think how do we construct the pinmux map for such binding.
The format you're using is:
<&pmx_controller_phandle muxable_entity_id selected_function>
For contruct pinmux map, we need to know at least 3 things for a device:
a) pinctrl device b) function name c) group name.
For a, we can get it from this binding.
But for b and c, since they are constants, how to convert to name string?
> /*
> * Pin configuration settings. Optional.
> *
> * Format is <&pmx_controller_phandle muxable_entity_id
> * configuration_option configuration_value>.
> */
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_CONF_SLEW_RATE 8>;
> /*
> * Perhaps allow additional custom properties here to
> * express things we haven't thought of. The pinctrl
> * drivers would be responsible for parsing them.
> */
> };
> pmx_sdhci_standby: {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
> };
> };
>
> Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
>
If "muxable entities" is pins on IMX, I'm wondering how we define the predefined
Functions and groups or if we still need to do that.
> TEGRA_PMX_PG_DTA
> TEGRA_PMX_PG_DTD
>
> Each individual pinmux driver's bindings needs to define what each integer ID
> represents.
>
Does it mean both pinmux driver and soc.dtsi file need define those macros if dtc
Supports constants?
> Integer IDs for the "mux functions". Note that these are the raw values written
> into hardware, not any driver-defined abstraction, and not any kind of "virtual
> group" that's been invented to make OEMs life easier:
>
> TEGRA_PMX_MUX_0
> TEGRA_PMX_MUX_1
> ...
> TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
>
> Since these are the raw IDs that go into HW, there's no need to specify each
> ID's meanings in the binding.
>
> Integer IDs for "pin config parameters":
>
> TEGRA_PMX_CONF_TRISTATE
> TEGRA_PMX_CONF_DRIVE_STRENGTH
> TEGRA_PMX_CONF_SLEW_RATE
>
> Each individual pinmux driver's bindings needs to define what each integer ID
> represents, and what the legal "value"s are for each one.
>
> Advantages:
>
> * Provides for both mux settings and "pin configuration".
>
> * Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
> file if desired to provide pre-defined pinmux configurations to
> choose from.
>
> * Allows the "pinmux configuration" nodes to be part of the per-device
> node if you don't want to use pre-defined configurations.
>
> * When pre-defined configurations are present, if you need something
> custom, you can do it easily.
>
> * Can augment pre-defined configurations by listing n nodes for each
> "name"d pinmux configuration, e.g. to add one extra pin config
> value.
>
> * Parsing is still quite simple:
> 1) Parse "pinmux" property in device node to get phandle.
> 2) Parse "mux" property in the node reference by the phandle,
> splitting into a list of pmx phandle, entity, mux func.
> 3) For each entry, pass entity, function to the appropriate mux
> driver. (For U-Boot, this might mean check that the phandle
> points at the expected place, and ignore the entry if not?)
> 4) Mux driver simply converts "muxable entity" to the register
> address, write the "function" value straight to the register.
>
> Disadvantages:
>
> * If you're not using pre-defined configurations, you still have to dump
> all the pinmux configuration into a sub-node of the device node, and
> have a property point at it using a phandle. This is slightly more
> complex than simply putting the mux/config properties right into the
> device node. However, it additionally allows one to have multiple
> "name"d configurations (e.g. for suspend) very easily, and isn't overly
> complex, so I can live with this.
I don't think the sub-node of the device node is a good place to define
Custom configurations.
Putting those things into device node will make its size big and also not look good.
Since it uses phandle, why not put it under pinctrl device node in board dts file?
It may also work.
>
> Changes to pinctrl subsystem:
>
> Very little, I think:
>
> * Need to pass raw function IDs through to the driver instead of the driver
> defining some logical table. Actually, this is just a change to individual
> drivers, since they can define the functions "func0", "func1", ... "funcn"
> as I mentioned before.
>
> * Need to add the code to actually parse this of course.
>
> One additional thought: If dtc does grow named constants, we can provide HW-
> function-based names for the mux functions, e.g.:
>
> TEGRA_PMX_DTA_RSVD0 0
> TEGRA_PMX_DTA_SDIO2 1
> TEGRA_PMX_DTA_VI 2
> TEGRA_PMX_DTA_RSVD3 3
>
> TEGRA_PMX_DTF_I2C3 0
> TEGRA_PMX_DTF_RSVD1 1
> TEGRA_PMX_DTF_VI 2
> TEGRA_PMX_DTF_RSVD3 3
> ...
>
> That'd allow the .dts to include functionality-based named for the mux function
> selection, but the .dtb to still include the raw HW mux field values. And this
> is something completely within the control of the SoC .dtsi file; if you don't
> like it, you don't have to do it.
>
> --
> nvpublic
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
2012-01-14 7:09 ` Shawn Guo
2012-01-16 12:50 ` Dong Aisheng-B29396
@ 2012-01-16 18:28 ` Grant Likely
2012-01-18 14:13 ` Tony Lindgren
2012-01-18 12:16 ` Thomas Abraham
2012-01-19 13:10 ` Thomas Abraham
4 siblings, 1 reply; 54+ messages in thread
From: Grant Likely @ 2012-01-16 18:28 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> I thought a bit more about pinmux DT bindings. I came up with something
> that I like well enough, and is pretty similar to the binding that Dong
> posted recently. I think it'll work for both Tegra's and IMX's needs.
> Please take a look!
>
> Note: I've used named constants below just to make this easier to read.
> We still don't have a solution to actually use named constants in dtc yet.
>
> tegra20.dtsi:
>
> / {
> tegra_pmx: pinmux@70000000 {
> compatible = "nvidia,tegra20-pinmux";
> reg = <0x70000014 0x10 /* Tri-state registers */
> 0x70000080 0x20 /* Mux registers */
> 0x700000a0 0x14 /* Pull-up/down registers */
> 0x70000868 0xa8>; /* Pad control registers */
> };
>
> sdhci@c8000200 {
> compatible = "nvidia,tegra20-sdhci";
> reg = <0xc8000200 0x200>;
> interrupts = <0 15 0x04>;
> };
> };
>
> tegra-harmony.dts:
>
> /{
> sdhci@c8000200 {
> cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> wp-gpios = <&gpio 57 0>; /* gpio PH1 */
> power-gpios = <&gpio 155 0>; /* gpio PT3 */
>
> /*
> * A list of named configurations that this device needs.
> * Format is a list of <"name" &phandle_of_pmx_configuration>
> *
> * Multiple "name"s are needed e.g. to support active/suspend,
> * or whatever device-defined states are appropriate. The
> * device defines which names are needed, just like a device
> * defines which regulators, clocks, GPIOs, interrupts, ...
> * it needs.
> *
> * This example shows a 1:1 relation between name and phandle.
> * We might want a 1:n relation, so that we can blend multiple
> * pre-defined sets of data together, e.g. one pre-defined set
> * for the pin mux configuration, another for the pin config
> * settings, both being put into the single "default" setting
> * for this one device.
> *
> * A pinmux controller can contain this property too, to
> * define "hogged" or "system" pin mux configuration.
> *
> * Note: Mixing strings and integers in a property seems
> * unusual. However, I have seen other bindings floating
> * around that are starting to do this...
> */
> pinmux =
> <"default" &pmx_sdhci_active>
> <"suspend" &pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux =
> <"default" &pmx_sdhci_mux_a>
> <"default" &pmx_sdhci_pincfg_a>
> <"suspend" &pmx_sdhci_mux_a>
> <"suspend" &pmx_sdhci_pincfg_a_suspend>;
Yeah, don't do this. Mixing phandle, string and cell values in a
property gets messy and could become troublesome to parse. I've
backed away from it in the clk binding.
pinumx-* is better, but I'm not thrilled with it and I avoided that
pattern too for the latest iteration of the clk binding. I prefer
using a "pinmux" + "pinmux-names" pair of properties when dealing with
an array of like objects (ie. reg, interrupts, clks, etc), but that
might not fit well since each setting has multiple state nodes.
>
> /*
> * Alternative: One property for each required state. But,
> * how does pinctrl core know which properties to parse?
> * Every property named "pinctrl*" seems a little too far-
> * reaching. Perhaps if we used vendor-name "pinmux", that'd
> * be OK, i.e. pinmux,default and pinmux,suspend?
It isn't actually a vendor name, so don't use a ','. "pinmux-" prefix
is fine.
g.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-16 12:50 ` Dong Aisheng-B29396
@ 2012-01-17 8:23 ` Shawn Guo
2012-01-17 9:46 ` Dong Aisheng-B29396
2012-01-17 19:21 ` Stephen Warren
2012-01-17 19:09 ` Stephen Warren
1 sibling, 2 replies; 54+ messages in thread
From: Shawn Guo @ 2012-01-17 8:23 UTC (permalink / raw)
To: Dong Aisheng-B29396
Cc: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
...
> > /*
> > * The actual definition of the complete state of the
> > * pinmux as required by some driver.
> > *
> > * These can be either directly in the device node, or
> > * somewhere in tegra20.dtsi in order to provide pre-
> > * selected/common configurations. Hence, they're referred
> > * to by phandle above.
> > */
> > pmx_sdhci_active: {
> > /*
> > * Pin mux settings. Mandatory?
> Mandatory for what?
>
> > * Not mandatory if the 1:1 mentioned above is
> > * extended to 1:n.
> > *
> > * Format is <&pmx_controller_phandle muxable_entity_id
> > * selected_function>.
> > *
> > * The pmx phandle is required since there may be more
> > * than one pinmux controller in the system. Even if
> > * this node is inside the pinmux controller itself, I
> > * think it's simpler to just always have this field
> > * present in the binding for consistency.
> > *
> > * Alternative: Format is <&pmx_controller_phandle
> > * pmx_controller_specific_data>. In this case, the
> > * pmx controller needs to define #pinmux-mux-cells,
> > * and provide the pinctrl core with a mapping
> > * function to handle the rest of the data in the
> > * property. This is how GPIOs and interrupts work.
> > * However, this will probably interact badly with
> > * wanting to parse the entire pinmux map early in
> > * boot, when perhaps the pinctrl core is initialized,
> > * but the pinctrl driver itself is not.
> > */
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > /* Syntax example */
> > <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> I'm still think how do we construct the pinmux map for such binding.
> The format you're using is:
> <&pmx_controller_phandle muxable_entity_id selected_function>
> For contruct pinmux map, we need to know at least 3 things for a device:
> a) pinctrl device b) function name c) group name.
> For a, we can get it from this binding.
> But for b and c, since they are constants, how to convert to name string?
>
I guess, for function name, it should be retrieved from the client
device node, and for the group name, it should be retrieved from the
node here.
For above example, the function name can be picked from sdhci device
node pinctr-names property I proposed, and the group name can just be
'pmx_sdhci_active', which is not a very nice name here and reminds me
the following point.
Considering the different pinctrl configurations for the same client
device usually share the same pinmux and only pinconf varies. It may
worth introducing another level phandle reference. Something like
the following:
pinmux_sdhci: pinmux-sdhci {
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
};
pinconf_sdhci_active: pinconf-sdhci-active {
config =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
};
pinconf_sdhci_suspend: pinconf-sdhci-suspend {
config =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
};
pinctrl_sdhci_active: pinctrl-sdhci-active {
pinmux = <&pinmux_sdhci>;
pinconf = <&pinconf_sdhci_active>;
};
pinctrl_sdhci_suspend: pinctrl-sdhci-suspend {
pinmux = <&pinmux_sdhci>;
pinconf = <&pinconf_sdhci_suspend>;
};
sdhci@c8000200 {
...
pinctrl = <&pinctrl_sdhci_active> <&pinctrl_sdhci_suspend>;
pinctrl-names = "active", "suspend";
};
This will be pretty useful for imx6 usdhc case, which will have 3
pinctrl configuration for each usdhc device (imx6 has 4 usdhc devices),
pinctrl-50mhz, pinctrl-100mhz and pinctrl-200mhz. All these 3 states
have the exactly same pinmux settings, and only varies on pinconf.
> > /*
> > * Pin configuration settings. Optional.
> > *
> > * Format is <&pmx_controller_phandle muxable_entity_id
> > * configuration_option configuration_value>.
> > */
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > /*
> > * Perhaps allow additional custom properties here to
> > * express things we haven't thought of. The pinctrl
> > * drivers would be responsible for parsing them.
> > */
> > };
> > pmx_sdhci_standby: {
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
> > };
> > };
> >
> > Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
> >
> If "muxable entities" is pins on IMX, I'm wondering how we define the predefined
> Functions and groups or if we still need to do that.
>
Let's put this in the example below.
pinmux_usdhc1: pinmux-usdhc1 {
mux = <IMX6Q_PAD_SD1_DAT1 0>
<IMX6Q_PAD_SD1_DAT2 0>
...
};
pinconf_usdhc1_50mhz: pinconf-usdhc1-50mhz {
config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x834>
<IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x834>
...
};
pinconf_usdhc1_100mhz: pinconf-usdhc1-100mhz {
config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x330>
<IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x330>
...
};
pinconf_usdhc1_200mhz: pinconf-usdhc1-200mhz {
config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x334>
<IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x334>
...
};
pinctrl_usdhc1_50mhz: pinctrl-usdhc1-50mhz {
pinmux = <&pinmux_usdhc1>;
pinconf = <&pinconf_usdhc1_50mhz>;
};
pinctrl_usdhc1_100mhz: pinctrl-usdhc1-100mhz {
pinmux = <&pinmux_usdhc1>;
pinconf = <&pinconf_usdhc1_100mhz>;
};
pinctrl_usdhc1_200mhz: pinctrl-usdhc1-200mhz {
pinmux = <&pinmux_usdhc1>;
pinconf = <&pinconf_usdhc1_200mhz>;
};
usdhc@02190000 { /* uSDHC1 */
...
pinctrl = <&usdhc1_50mhz>, <&usdhc1_100mhz>, <&usdhc1_200mhz>;
pinctrl-names = "usdhc1-50mhz", "usdhc1-100mhz", "usdhc1-200mhz";
};
In this example, we have 3 functions/states for client device usdhc1,
"usdhc1-50mhz", "usdhc1-100mhz", and "usdhc1-200mhz". The group is
being defined by enumerating the pins in property 'mux' of node
pinmux_usdhc1.
> > TEGRA_PMX_PG_DTA
> > TEGRA_PMX_PG_DTD
> >
> > Each individual pinmux driver's bindings needs to define what each integer ID
> > represents.
> >
> Does it mean both pinmux driver and soc.dtsi file need define those macros if dtc
> Supports constants?
>
Yes, I think it does. But we should try to work out some way letting
dts and linux driver include the same header file to avoid maintaining
two copies of the same data.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 8:23 ` Shawn Guo
@ 2012-01-17 9:46 ` Dong Aisheng-B29396
2012-01-17 14:13 ` Shawn Guo
2012-01-17 19:28 ` Stephen Warren
2012-01-17 19:21 ` Stephen Warren
1 sibling, 2 replies; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-17 9:46 UTC (permalink / raw)
To: Shawn Guo
Cc: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
> -----Original Message-----
> From: Shawn Guo [mailto:shawn.guo@linaro.org]
> Sent: Tuesday, January 17, 2012 4:24 PM
> To: Dong Aisheng-B29396
> Cc: Stephen Warren; linus.walleij@stericsson.com; s.hauer@pengutronix.de;
> rob.herring@calxeda.com; kernel@pengutronix.de; cjb@laptop.org; Simon Glass
> (sjg@chromium.org); Dong Aisheng; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree-discuss@lists.ozlabs.org
> Subject: Re: Pinmux bindings proposal
> Importance: High
>
> On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
> ...
> > > /*
> > > * The actual definition of the complete state of the
> > > * pinmux as required by some driver.
> > > *
> > > * These can be either directly in the device node, or
> > > * somewhere in tegra20.dtsi in order to provide pre-
> > > * selected/common configurations. Hence, they're referred
> > > * to by phandle above.
> > > */
> > > pmx_sdhci_active: {
> > > /*
> > > * Pin mux settings. Mandatory?
> > Mandatory for what?
> >
> > > * Not mandatory if the 1:1 mentioned above is
> > > * extended to 1:n.
> > > *
> > > * Format is <&pmx_controller_phandle
> muxable_entity_id
> > > * selected_function>.
> > > *
> > > * The pmx phandle is required since there may be
> more
> > > * than one pinmux controller in the system. Even if
> > > * this node is inside the pinmux controller itself,
> I
> > > * think it's simpler to just always have this field
> > > * present in the binding for consistency.
> > > *
> > > * Alternative: Format is <&pmx_controller_phandle
> > > * pmx_controller_specific_data>. In this case, the
> > > * pmx controller needs to define #pinmux-mux-cells,
> > > * and provide the pinctrl core with a mapping
> > > * function to handle the rest of the data in the
> > > * property. This is how GPIOs and interrupts work.
> > > * However, this will probably interact badly with
> > > * wanting to parse the entire pinmux map early in
> > > * boot, when perhaps the pinctrl core is
> initialized,
> > > * but the pinctrl driver itself is not.
> > > */
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > > /* Syntax example */
> > > <&foo_pmx FOO_PMX_PG_X
> > > FOO_PMX_MUX_0>;
> > I'm still think how do we construct the pinmux map for such binding.
> > The format you're using is:
> > <&pmx_controller_phandle muxable_entity_id selected_function> For
> > contruct pinmux map, we need to know at least 3 things for a device:
> > a) pinctrl device b) function name c) group name.
> > For a, we can get it from this binding.
> > But for b and c, since they are constants, how to convert to name string?
> >
> I guess, for function name, it should be retrieved from the client device node,
> and for the group name, it should be retrieved from the node here.
>
I guess Stephen's idea is to retrieving the function name and group name
>From the pinctrl driver since Tagre prefers to define those things in driver
Rather than in board file or soc.dts file.
But it does not fit for IMX since we define it in soc.dts.
> For above example, the function name can be picked from sdhci device node
> pinctr-names property I proposed,
If I understand correctly, the pinctrl-names property you proposed represents
The pin group state.
> and the group name can just be
> 'pmx_sdhci_active', which is not a very nice name here and reminds me the
> following point.
>
No, I don't think it's suitable for group name since pmx_sdhci_active is not a
group node (actually it includes many groups).
> Considering the different pinctrl configurations for the same client device
> usually share the same pinmux and only pinconf varies.
I have the same doubts before.
Is there a real case that device has the different pinmux in different state?
Stephen?
> It may worth introducing
> another level phandle reference. Something like the following:
>
> pinmux_sdhci: pinmux-sdhci {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> };
>
> pinconf_sdhci_active: pinconf-sdhci-active {
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
>
> pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
>
The config makes sense to me.
The only question is how to get group name to match with the predefined groups.
Besides per pin group configuration support, we may also want per pin configuration
Support as the latest patch sent by Linus.
http://www.spinics.net/lists/arm-kernel/msg155712.html
> pinctrl_sdhci_active: pinctrl-sdhci-active {
> pinmux = <&pinmux_sdhci>;
> pinconf = <&pinconf_sdhci_active>;
> };
>
> pinctrl_sdhci_suspend: pinctrl-sdhci-suspend {
> pinmux = <&pinmux_sdhci>;
> pinconf = <&pinconf_sdhci_suspend>;
> };
>
> sdhci@c8000200 {
> ...
> pinctrl = <&pinctrl_sdhci_active> <&pinctrl_sdhci_suspend>;
> pinctrl-names = "active", "suspend";
> };
>
> This will be pretty useful for imx6 usdhc case, which will have 3 pinctrl
> configuration for each usdhc device (imx6 has 4 usdhc devices), pinctrl-50mhz,
> pinctrl-100mhz and pinctrl-200mhz. All these 3 states have the exactly same
> pinmux settings, and only varies on pinconf.
>
> > > /*
> > > * Pin configuration settings. Optional.
> > > *
> > > * Format is <&pmx_controller_phandle
> muxable_entity_id
> > > * configuration_option configuration_value>.
> > > */
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > /*
> > > * Perhaps allow additional custom properties here
> to
> > > * express things we haven't thought of. The pinctrl
> > > * drivers would be responsible for parsing them.
> > > */
> > > };
> > > pmx_sdhci_standby: {
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_MUX_1>;
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > };
> > > };
> > > };
> > >
> > > Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
> > >
> > If "muxable entities" is pins on IMX, I'm wondering how we define the
> > predefined Functions and groups or if we still need to do that.
> >
> Let's put this in the example below.
>
> pinmux_usdhc1: pinmux-usdhc1 {
> mux = <IMX6Q_PAD_SD1_DAT1 0>
> <IMX6Q_PAD_SD1_DAT2 0>
> ...
> };
>
Yes, I agree.
And in this way we're still using virtual groups.
It has no big difference as we did before like:
pinmux-groups {
uart4grp: group@0 {
grp-name = "uart4grp";
grp-pins = <107 108>;
grp-mux = <4 4>;
};
sd4grp: group@1 {
grp-name = "sd4grp";
grp-pins = <170 171 180 181 182 183 184 185 186 187>;
grp-mux = <0 0 1 1 1 1 1 1 1 1>;
};
};
The real problem is do we need to support individual pin mux
Or still using virtual pin group?
For the way Stephen proposed, we can only support individual pin mux
Since IMX pins are not grouped together in HW.
> pinconf_usdhc1_50mhz: pinconf-usdhc1-50mhz {
> config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x834>
> <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x834>
> ...
> };
>
> pinconf_usdhc1_100mhz: pinconf-usdhc1-100mhz {
> config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x330>
> <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x330>
> ...
> };
>
> pinconf_usdhc1_200mhz: pinconf-usdhc1-200mhz {
> config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x334>
> <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x334>
> ...
> };
>
> pinctrl_usdhc1_50mhz: pinctrl-usdhc1-50mhz {
> pinmux = <&pinmux_usdhc1>;
> pinconf = <&pinconf_usdhc1_50mhz>;
> };
>
> pinctrl_usdhc1_100mhz: pinctrl-usdhc1-100mhz {
> pinmux = <&pinmux_usdhc1>;
> pinconf = <&pinconf_usdhc1_100mhz>;
> };
>
> pinctrl_usdhc1_200mhz: pinctrl-usdhc1-200mhz {
> pinmux = <&pinmux_usdhc1>;
> pinconf = <&pinconf_usdhc1_200mhz>;
> };
>
> usdhc@02190000 { /* uSDHC1 */
> ...
> pinctrl = <&usdhc1_50mhz>, <&usdhc1_100mhz>, <&usdhc1_200mhz>;
> pinctrl-names = "usdhc1-50mhz", "usdhc1-100mhz", "usdhc1-200mhz";
> };
>
> In this example, we have 3 functions/states for client device usdhc1, "usdhc1-
> 50mhz", "usdhc1-100mhz", and "usdhc1-200mhz". The group is being defined by
> enumerating the pins in property 'mux' of node pinmux_usdhc1.
>
Yes, It's still under development by Linus.
The last patch Linus sent still does not support state change for specific device.
But the method is ok to me.
> > > TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_PG_DTD
> > >
> > > Each individual pinmux driver's bindings needs to define what each
> > > integer ID represents.
> > >
> > Does it mean both pinmux driver and soc.dtsi file need define those
> > macros if dtc Supports constants?
> >
> Yes, I think it does. But we should try to work out some way letting dts and
> linux driver include the same header file to avoid maintaining two copies of the
> same data.
>
I'm also care about the potential consistent issue.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-17 9:46 ` Dong Aisheng-B29396
@ 2012-01-17 14:13 ` Shawn Guo
2012-01-17 19:32 ` Stephen Warren
2012-01-18 3:44 ` Dong Aisheng-B29396
2012-01-17 19:28 ` Stephen Warren
1 sibling, 2 replies; 54+ messages in thread
From: Shawn Guo @ 2012-01-17 14:13 UTC (permalink / raw)
To: Dong Aisheng-B29396
Cc: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Tue, Jan 17, 2012 at 09:46:42AM +0000, Dong Aisheng-B29396 wrote:
...
> I guess Stephen's idea is to retrieving the function name and group name
> From the pinctrl driver since Tagre prefers to define those things in driver
> Rather than in board file or soc.dts file.
> But it does not fit for IMX since we define it in soc.dts.
>
Hmm, when he came up with this proposal and said it should work for
Tegra, I assume he plans to move those from Tegra pinctrl driver into
device tree. Stephen?
...
> > Considering the different pinctrl configurations for the same client device
> > usually share the same pinmux and only pinconf varies.
> I have the same doubts before.
> Is there a real case that device has the different pinmux in different state?
> Stephen?
>
> > It may worth introducing
> > another level phandle reference. Something like the following:
> >
> > pinmux_sdhci: pinmux-sdhci {
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > };
> >
> > pinconf_sdhci_active: pinconf-sdhci-active {
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
> >
> > pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
> >
> The config makes sense to me.
> The only question is how to get group name to match with the predefined groups.
>
I still incline to say there should not be any predefined groups for
DT case, if we choose to find the group in use only when pinmux_get()
gets called. Even if you insist to have a global scanning on device
tree for all groups to create the pinmux mapping table, those predefined
groups can only be created by your scanning code, so you should know
how the name comes from device tree, and you should be able to recreate
the name when you are trying to matching the name in the table you
created before.
> Besides per pin group configuration support, we may also want per pin configuration
> Support as the latest patch sent by Linus.
> http://www.spinics.net/lists/arm-kernel/msg155712.html
>
This binding proposal has covered both pin and group configuration,
as the second parameter of 'config' property could be either a pin
or group which depends the on whether the configurable entity at HW
level is a pin or group.
...
> > > If "muxable entities" is pins on IMX, I'm wondering how we define the
> > > predefined Functions and groups or if we still need to do that.
> > >
> > Let's put this in the example below.
> >
> > pinmux_usdhc1: pinmux-usdhc1 {
> > mux = <IMX6Q_PAD_SD1_DAT1 0>
> > <IMX6Q_PAD_SD1_DAT2 0>
> > ...
> > };
> >
> Yes, I agree.
> And in this way we're still using virtual groups.
> It has no big difference as we did before like:
> pinmux-groups {
> uart4grp: group@0 {
> grp-name = "uart4grp";
> grp-pins = <107 108>;
> grp-mux = <4 4>;
> };
>
> sd4grp: group@1 {
> grp-name = "sd4grp";
> grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> };
> };
>
You are right. They are fundamentally same and just in different
format.
> The real problem is do we need to support individual pin mux
> Or still using virtual pin group?
> For the way Stephen proposed, we can only support individual pin mux
> Since IMX pins are not grouped together in HW.
>
I do not see any problem here. If you look at the first column of
'mux' property of node pinmux-usdhc1, it is a group of pins for usdhc1.
Isn't it one virtual pin group for usdhc1?
> > pinconf_usdhc1_50mhz: pinconf-usdhc1-50mhz {
> > config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x834>
> > <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x834>
> > ...
> > };
> >
> > pinconf_usdhc1_100mhz: pinconf-usdhc1-100mhz {
> > config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x330>
> > <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x330>
> > ...
> > };
> >
> > pinconf_usdhc1_200mhz: pinconf-usdhc1-200mhz {
> > config = <IMX6Q_PAD_SD1_DAT1 IMX6Q_PAD_CONF_ALL 0x334>
> > <IMX6Q_PAD_SD1_DAT2 IMX6Q_PAD_CONF_ALL 0x334>
> > ...
> > };
> >
> > pinctrl_usdhc1_50mhz: pinctrl-usdhc1-50mhz {
> > pinmux = <&pinmux_usdhc1>;
> > pinconf = <&pinconf_usdhc1_50mhz>;
> > };
> >
> > pinctrl_usdhc1_100mhz: pinctrl-usdhc1-100mhz {
> > pinmux = <&pinmux_usdhc1>;
> > pinconf = <&pinconf_usdhc1_100mhz>;
> > };
> >
> > pinctrl_usdhc1_200mhz: pinctrl-usdhc1-200mhz {
> > pinmux = <&pinmux_usdhc1>;
> > pinconf = <&pinconf_usdhc1_200mhz>;
> > };
> >
> > usdhc@02190000 { /* uSDHC1 */
> > ...
> > pinctrl = <&usdhc1_50mhz>, <&usdhc1_100mhz>, <&usdhc1_200mhz>;
> > pinctrl-names = "usdhc1-50mhz", "usdhc1-100mhz", "usdhc1-200mhz";
> > };
> >
> > In this example, we have 3 functions/states for client device usdhc1, "usdhc1-
> > 50mhz", "usdhc1-100mhz", and "usdhc1-200mhz". The group is being defined by
> > enumerating the pins in property 'mux' of node pinmux_usdhc1.
> >
> Yes, It's still under development by Linus.
> The last patch Linus sent still does not support state change for specific device.
> But the method is ok to me.
>
Yes, we need per device state switch.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-14 7:09 ` Shawn Guo
@ 2012-01-17 18:47 ` Stephen Warren
2012-01-18 3:32 ` Shawn Guo
0 siblings, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-17 18:47 UTC (permalink / raw)
To: Shawn Guo
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Shawn Guo wrote at Saturday, January 14, 2012 12:09 AM:
> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > I thought a bit more about pinmux DT bindings. I came up with something
> > that I like well enough, and is pretty similar to the binding that Dong
> > posted recently. I think it'll work for both Tegra's and IMX's needs.
> > Please take a look!
...
> > tegra-harmony.dts:
> >
> > /{
> > sdhci@c8000200 {
> > cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> > wp-gpios = <&gpio 57 0>; /* gpio PH1 */
> > power-gpios = <&gpio 155 0>; /* gpio PT3 */
> >
> > /*
> > * A list of named configurations that this device needs.
> > * Format is a list of <"name" &phandle_of_pmx_configuration>
> > *
> > * Multiple "name"s are needed e.g. to support active/suspend,
> > * or whatever device-defined states are appropriate. The
> > * device defines which names are needed, just like a device
> > * defines which regulators, clocks, GPIOs, interrupts, ...
> > * it needs.
> > *
> > * This example shows a 1:1 relation between name and phandle.
> > * We might want a 1:n relation, so that we can blend multiple
> > * pre-defined sets of data together, e.g. one pre-defined set
> > * for the pin mux configuration, another for the pin config
> > * settings, both being put into the single "default" setting
> > * for this one device.
> > *
> > * A pinmux controller can contain this property too, to
> > * define "hogged" or "system" pin mux configuration.
> > *
> > * Note: Mixing strings and integers in a property seems
> > * unusual. However, I have seen other bindings floating
> > * around that are starting to do this...
> > */
> > pinmux =
>
> I prefer to have the property named 'pinctrl' than 'pinmux'.
Yes. I think I worried that "pinctrl" was a little too tied to the Linux
driver so didn't name it that, but you're right that "pinmux" doesn't
encompass everything this binding covers, so "pinctrl" does seem to be
the right choice.
> > <"default" &pmx_sdhci_active>
> > <"suspend" &pmx_sdhci_suspend>;
>
> I would rather do something like what clock DT binding proposal is
> doing.
>
> pinctrl = <&pmx_sdhci_active>, <&pmx_sdhci_suspend>;
> pinctrl-names = "default", "suspend";
Yes, that's a good idea.
I'm afraid I haven't been following the clock binding discussions at
all, but just started looking at them right after I wrote the email you
were responding to.
> > /* 1:n example: */
> > pinmux =
> > <"default" &pmx_sdhci_mux_a>
> > <"default" &pmx_sdhci_pincfg_a>
> > <"suspend" &pmx_sdhci_mux_a>
> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>
> I personally do not like the 1:n binding. To me, any particular pinctrl
> configuration, e.g. pmx_sdhci_active, should consist of a pair of pinmux
> and pinconf, which should be given by the pmx_sdhci_active node.
Just a further explanation on my original code: I always intended that
each entry in that list was a full pinmux configuration that could include
mux and pin configuration settings. Thus, the above is more like:
pinctrl =
<"default" &pmx_sdhci_a>
<"default" &pmx_sdhci_overrides>
(overrides rather than explicit separate mux/config; the separate mux
And config were just an example use case).
My thoughts here:
With this binding, we can certainly define a lot of pre-defined/canned
configurations to a set of pins. However, there are so many pin config
options (at least on Tegra) for different aspects of drive strength, slew
rate, ... that I sincerely doubt every single board is going to be able
to use one of those pre-defined/canned *exactly* without changes. The ways
to cope with these small board-specific differences are:
a) Cut/paste the entire pre-defined/canned configuration and tweak it
as necessary. You can do this with the 1:1 model.
b) Use the pre-defined/canned as a base, then modify it to add extra
configuration options, or change existing configuration options, as
appropriate. I think the 1:n model works best for this. Given previous
comments, I'd now propose the following syntax for a 1:n model:
pinctrl = <&pmx_sdhci_a>, <&pmx_sdhci_overrides>, <&pmx_sdhci_suspend>;
pinctrl-names = "default", "default", "suspend";
This should be easy to implement; instead of roughly:
prop = get_prop(np, "pinctrl-names");
index = find_index(prop, "default");
handle_pinctrl_prop(np, "pinctrl", index);
something more like:
prop = get_prop(np, "pinctrl-names");
prev = NULL;
while (find_index(prop, "default", &prev))
handle_pinctrl_prop(np, "pinctrl", index);
...
> > /*
> > * The actual definition of the complete state of the
> > * pinmux as required by some driver.
> > *
> > * These can be either directly in the device node, or
> > * somewhere in tegra20.dtsi in order to provide pre-
> > * selected/common configurations. Hence, they're referred
> > * to by phandle above.
> > */
> > pmx_sdhci_active: {
> > /*
> > * Pin mux settings. Mandatory?
> > * Not mandatory if the 1:1 mentioned above is
> > * extended to 1:n.
> > *
> > * Format is <&pmx_controller_phandle muxable_entity_id
> > * selected_function>.
> > *
> > * The pmx phandle is required since there may be more
> > * than one pinmux controller in the system. Even if
> > * this node is inside the pinmux controller itself, I
> > * think it's simpler to just always have this field
> > * present in the binding for consistency.
> > *
>
> I prefer to just put such nodes inside pinctrl controller itself and
> drop those phandles.
My rationale here:
Forcing those nodes to be inside the controller node forces us to store
any board-specific custom configurations or overrides in the controller
node too; I'd simply prefer the flexibility to put them anywhere.
Equally, I want SoC vendors to be able to choose whether to use these
pre-defined/canned configuration nodes at all. If you do use them, putting
them into the controller node makes perfect sense. If you don't use them,
putting the pin configuration nodes into the individual device nodes (in
the board.dts file) makes much more sense.
Having each property start with the phandle of the relevant controller
is also far more consistent with the way all the GPIO, IRQ, ... bindings
work.
> > * Alternative: Format is <&pmx_controller_phandle
> > * pmx_controller_specific_data>. In this case, the
> > * pmx controller needs to define #pinmux-mux-cells,
> > * and provide the pinctrl core with a mapping
> > * function to handle the rest of the data in the
> > * property. This is how GPIOs and interrupts work.
> > * However, this will probably interact badly with
> > * wanting to parse the entire pinmux map early in
> > * boot, when perhaps the pinctrl core is initialized,
> > * but the pinctrl driver itself is not.
> > */
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > /* Syntax example */
> > <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> > /*
> > * Pin configuration settings. Optional.
> > *
>
> I guess pinconf can be optional because some pin/group that have pinmux
> setting do not necessarily have pinconf setting? If that's case,
> yes, agreed.
Yes, that's true.
Also, if you're using the 1:n model I mentioned above, your "base" node
may well have both, but your "override" node may well only need to add
additional pinmux settings, or tweak just a single pin control setting,
so forcing someone to specify both wouldn't work.
Still, I suppose we could just force both properties to be present, but
just write an empty list if one type of setting wasn't needed; at least
this would allow for some amount of error-checking of the node content.
...
> > Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
> >
> > TEGRA_PMX_PG_DTA
> > TEGRA_PMX_PG_DTD
> >
> > Each individual pinmux driver's bindings needs to define what each integer
> > ID represents.
...
>
> Agreed on these. But we really need to push named constants support
> for dtc, otherwise the binding is so difficult for engineering. (We
> have a lot of pinconfig parameters on imx)
Yes, there is a discussion about this going on.
(Well, it started a good few years back and had a hiatus. There are a lot
of long-term syntax issues to plan out before we can get the basic support
in though, to make sure we don't screw our future selves.)
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-16 12:50 ` Dong Aisheng-B29396
2012-01-17 8:23 ` Shawn Guo
@ 2012-01-17 19:09 ` Stephen Warren
2012-01-18 7:24 ` Dong Aisheng-B29396
1 sibling, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-17 19:09 UTC (permalink / raw)
To: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, Shawn Guo (shawn.guo@linaro.org)
Cc: linux-kernel, linux-arm-kernel, devicetree-discuss
Dong Aisheng wrote at Monday, January 16, 2012 5:50 AM:
> Stephen Warren wrote at Saturday, January 14, 2012 4:40 AM:
> > I thought a bit more about pinmux DT bindings. I came up with something that I
> > like well enough, and is pretty similar to the binding that Dong posted recently.
> > I think it'll work for both Tegra's and IMX's needs.
> > Please take a look!
...
> > pinmux =
> > <"default" &pmx_sdhci_active>
> > <"suspend" &pmx_sdhci_suspend>;
...
> > /*
> > * Alternative: One property for each required state. But,
> > * how does pinctrl core know which properties to parse?
> > * Every property named "pinctrl*" seems a little too far-
> > * reaching. Perhaps if we used vendor-name "pinmux", that'd
> > * be OK, i.e. pinmux,default and pinmux,suspend?
>
> It we support whatever device-defined states, it's meaningless to use one property
> For each required state since pinctrl core does not know the property name the
> customer defined.
Yes, that's the problem. Shawn proposed copying the clock bindings which
I think solves all the issues; see my previous email.
> > */
> > pinmux = <&pmx_sdhci_active>;
> > pinmux-suspend <&pmx_sdhci_suspend>;
> >
> > /* 1:n example: */
> > pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
>
> This looks ok to me since the mux and pincfg usually is 1 to 1.
> And if we do not have a pincfg for a pinmux we can find a way to tell the pinctl
> core, maybe just set pincfg phandle to 0.
Just to be clear, I'll repeat part of my previous response to Shawn:
* Just a further explanation on my original code: I always intended that
* each entry in that list was a full pinmux configuration that could include
* mux and pin configuration settings. Thus, the above is more like:
*
* pinctrl =
* <"default" &pmx_sdhci_a>
* <"default" &pmx_sdhci_overrides>
*
* (overrides rather than explicit separate mux/config; the separate mux
* And config were just an example use case).
So that was a list where the /examples/ had two entries, one for mux and
one for pin config. In general, there could be 1, 2, 3, ... entries and
each could define whatever mux and config entries they wanted.
> > pinmux-suspend = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a_suspend>;
> >
> > /*
> > * The actual definition of the complete state of the
> > * pinmux as required by some driver.
> > *
> > * These can be either directly in the device node, or
> > * somewhere in tegra20.dtsi in order to provide pre-
> > * selected/common configurations. Hence, they're referred
> > * to by phandle above.
> > */
> > pmx_sdhci_active: {
> > /*
> > * Pin mux settings. Mandatory?
>
> Mandatory for what?
I was thinking that each pin mux configuration node MUST specify at
least some mux settings. However, that may not make sense; it may be
reasonable to specify just pin config settings and no mux settings
(in a 1:n model in the "override" node).
>
> > * Not mandatory if the 1:1 mentioned above is
> > * extended to 1:n.
> > *
> > * Format is <&pmx_controller_phandle muxable_entity_id
> > * selected_function>.
> > *
> > * The pmx phandle is required since there may be more
> > * than one pinmux controller in the system. Even if
> > * this node is inside the pinmux controller itself, I
> > * think it's simpler to just always have this field
> > * present in the binding for consistency.
> > *
> > * Alternative: Format is <&pmx_controller_phandle
> > * pmx_controller_specific_data>. In this case, the
> > * pmx controller needs to define #pinmux-mux-cells,
> > * and provide the pinctrl core with a mapping
> > * function to handle the rest of the data in the
> > * property. This is how GPIOs and interrupts work.
> > * However, this will probably interact badly with
> > * wanting to parse the entire pinmux map early in
> > * boot, when perhaps the pinctrl core is initialized,
> > * but the pinctrl driver itself is not.
> > */
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > /* Syntax example */
> > <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
>
> I'm still think how do we construct the pinmux map for such binding.
> The format you're using is:
> <&pmx_controller_phandle muxable_entity_id selected_function>
> For contruct pinmux map, we need to know at least 3 things for a device:
> a) pinctrl device b) function name c) group name.
> For a, we can get it from this binding.
> But for b and c, since they are constants, how to convert to name string?
a) pinctrl device: We can extract this from pmx_controller_phandle;
simply search for a device with the same OF node, and retrieve that
registered device from the pinctrl subsystem. This is how GPIO and IRQ
work.
b) function name: The pinmux_ops for the driver of pmx_controller_phandle
needs a function to convert integer IDs such as TEGRA_PMX_MUX_1 to whatever
function IDs are used by the pinctrl subsystem.
c) group name: This should be handled just like (b).
Also you'll need to know:
struct pinmux_map's .name field. This is the value in the pinctrl-names
property, assuming we're switching to the following syntax:
pinctrl = <&pmx_sdhci_a_active>, <&pmx_sdhci_a_suspend>;
pinctrl-names = "default", "suspend";
> > /*
> > * Pin configuration settings. Optional.
> > *
> > * Format is <&pmx_controller_phandle muxable_entity_id
> > * configuration_option configuration_value>.
> > */
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > /*
> > * Perhaps allow additional custom properties here to
> > * express things we haven't thought of. The pinctrl
> > * drivers would be responsible for parsing them.
> > */
> > };
> > pmx_sdhci_standby: {
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA
> > TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD
> > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
> > };
> > };
> >
> > Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
>
> If "muxable entities" is pins on IMX, I'm wondering how we define the predefined
> Functions and groups or if we still need to do that.
By "predefined", do you mean:
a) Does the DT need to list all the functions and groups.
The answer here is no in general. However, if you want to parameterize
your pinctrl driver's list of known functions and groups, then you can
certainly do this; the pinctrl binding simple doesn't force you to do
this.
b) Does the pinctrl driver need to list all functions and groups to the
pinctrl core?
As the pinctrl subsystem APIs stand right now, the answer here is yes.
I don't think there's really any way around this, although depending on
HW, you can do things like:
1) Instead of defining logical functions (SDHCI1, I2C1, ...) you could
simply define a function for each mux register value (func0, func1, ...)
2) On IMX, you'd define a pin group for each individual pin, since the
HW muxing is at a pin not a group level.
> > TEGRA_PMX_PG_DTA
> > TEGRA_PMX_PG_DTD
> >
> > Each individual pinmux driver's bindings needs to define what each integer ID
> > represents.
>
> Does it mean both pinmux driver and soc.dtsi file need define those macros if dtc
> Supports constants?
Yes. As Shawn mentions later, perhaps we can find some way to define
these values in one place, and generate both a .dtsi file and a .h file
from that single list, or generate a .h file from the .dtsi file or vice-
versa.
> > Integer IDs for the "mux functions". Note that these are the raw values written
> > into hardware, not any driver-defined abstraction, and not any kind of "virtual
> > group" that's been invented to make OEMs life easier:
> >
> > TEGRA_PMX_MUX_0
> > TEGRA_PMX_MUX_1
> > ...
> > TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
> >
> > Since these are the raw IDs that go into HW, there's no need to specify each
> > ID's meanings in the binding.
> >
> > Integer IDs for "pin config parameters":
> >
> > TEGRA_PMX_CONF_TRISTATE
> > TEGRA_PMX_CONF_DRIVE_STRENGTH
> > TEGRA_PMX_CONF_SLEW_RATE
> >
> > Each individual pinmux driver's bindings needs to define what each integer ID
> > represents, and what the legal "value"s are for each one.
> >
> > Advantages:
> >
> > * Provides for both mux settings and "pin configuration".
> >
> > * Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
> > file if desired to provide pre-defined pinmux configurations to
> > choose from.
> >
> > * Allows the "pinmux configuration" nodes to be part of the per-device
> > node if you don't want to use pre-defined configurations.
> >
> > * When pre-defined configurations are present, if you need something
> > custom, you can do it easily.
> >
> > * Can augment pre-defined configurations by listing n nodes for each
> > "name"d pinmux configuration, e.g. to add one extra pin config
> > value.
> >
> > * Parsing is still quite simple:
> > 1) Parse "pinmux" property in device node to get phandle.
> > 2) Parse "mux" property in the node reference by the phandle,
> > splitting into a list of pmx phandle, entity, mux func.
> > 3) For each entry, pass entity, function to the appropriate mux
> > driver. (For U-Boot, this might mean check that the phandle
> > points at the expected place, and ignore the entry if not?)
> > 4) Mux driver simply converts "muxable entity" to the register
> > address, write the "function" value straight to the register.
> >
> > Disadvantages:
> >
> > * If you're not using pre-defined configurations, you still have to dump
> > all the pinmux configuration into a sub-node of the device node, and
> > have a property point at it using a phandle. This is slightly more
> > complex than simply putting the mux/config properties right into the
> > device node. However, it additionally allows one to have multiple
> > "name"d configurations (e.g. for suspend) very easily, and isn't overly
> > complex, so I can live with this.
>
> I don't think the sub-node of the device node is a good place to define
> Custom configurations.
> Putting those things into device node will make its size big and also not look good.
> Since it uses phandle, why not put it under pinctrl device node in board dts file?
> It may also work.
Both you and Shawn have said you don't like allowing the pin configurations
to be sub-nodes of the devices. Can you expand a little more on that? I
mentioned some of my reasons for allowing this in my previous email, so
see that for reference. I guess I have a couple more points to make:
1) It's optional. If everything you need can be represented in pre-
defined/canned pin configuration nodes under the pin controller's node,
you can certainly do that.
2) Putting this in the device's node seems more consistent with existing
bindings, which put all configuration inside the device node: The IRQ
binding places interrupt IDs and configuration (level/edge, high/low)
in the device node. The GPIO binding places GPIO IDs and possible flags
(e.g. inverted/not) in the device node. Etc.
3) I don't think there will be any .dtb size difference at all between
putting the pin configuration in the pin controller node or the device
node; the content of the sub-node will be entirely identical. The same
holds true for the .dts files (when you consider soc.dtsi and board.dts
together).
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 8:23 ` Shawn Guo
2012-01-17 9:46 ` Dong Aisheng-B29396
@ 2012-01-17 19:21 ` Stephen Warren
2012-01-18 4:01 ` Shawn Guo
2012-01-18 9:32 ` Dong Aisheng-B29396
1 sibling, 2 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-17 19:21 UTC (permalink / raw)
To: Shawn Guo, Dong Aisheng-B29396
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Shawn Guo wrote at Tuesday, January 17, 2012 1:24 AM:
> On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
...
> > > * Not mandatory if the 1:1 mentioned above is
> > > * extended to 1:n.
> > > *
> > > * Format is <&pmx_controller_phandle muxable_entity_id
> > > * selected_function>.
> > > *
> > > * The pmx phandle is required since there may be more
> > > * than one pinmux controller in the system. Even if
> > > * this node is inside the pinmux controller itself, I
> > > * think it's simpler to just always have this field
> > > * present in the binding for consistency.
> > > *
> > > * Alternative: Format is <&pmx_controller_phandle
> > > * pmx_controller_specific_data>. In this case, the
> > > * pmx controller needs to define #pinmux-mux-cells,
> > > * and provide the pinctrl core with a mapping
> > > * function to handle the rest of the data in the
> > > * property. This is how GPIOs and interrupts work.
> > > * However, this will probably interact badly with
> > > * wanting to parse the entire pinmux map early in
> > > * boot, when perhaps the pinctrl core is initialized,
> > > * but the pinctrl driver itself is not.
> > > */
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > > /* Syntax example */
> > > <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> >
> > I'm still think how do we construct the pinmux map for such binding.
> > The format you're using is:
> > <&pmx_controller_phandle muxable_entity_id selected_function>
> > For contruct pinmux map, we need to know at least 3 things for a device:
> > a) pinctrl device b) function name c) group name.
> > For a, we can get it from this binding.
> > But for b and c, since they are constants, how to convert to name string?
>
> I guess, for function name, it should be retrieved from the client
> device node, and for the group name, it should be retrieved from the
> node here.
>
> For above example, the function name can be picked from sdhci device
> node pinctr-names property I proposed, and the group name can just be
> 'pmx_sdhci_active', which is not a very nice name here and reminds me
> the following point.
I responded directly to Dong's email re: how to map the DT values into
something for pinctrl. The mapping being described here isn't quite
what I had in mind....
> Considering the different pinctrl configurations for the same client
> device usually share the same pinmux and only pinconf varies. It may
> worth introducing another level phandle reference. Something like
> the following:
I don't think there's a need for another level of indirection. The 1:n
model I was talking about already handles this, I believe. See below.
> pinmux_sdhci: pinmux-sdhci {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> };
>
> pinconf_sdhci_active: pinconf-sdhci-active {
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
>
> pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
Those 3 nodes make sense to me.
> pinctrl_sdhci_active: pinctrl-sdhci-active {
> pinmux = <&pinmux_sdhci>;
> pinconf = <&pinconf_sdhci_active>;
> };
>
> pinctrl_sdhci_suspend: pinctrl-sdhci-suspend {
> pinmux = <&pinmux_sdhci>;
> pinconf = <&pinconf_sdhci_suspend>;
> };
I think we can avoid those 2 nodes.
> sdhci@c8000200 {
> ...
> pinctrl = <&pinctrl_sdhci_active> <&pinctrl_sdhci_suspend>;
> pinctrl-names = "active", "suspend";
> };
And rewrite that node as:
sdhci@c8000200 {
...
pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
<&pinmux_sdhci> <&pinconf_sdhci_suspend>;
pinctrl-names = "active", "active", "suspend", "suspend";
};
The only slight disadvantage here is that the person constructing the
pinctrl/pinctrl-names properties needs to know to explicitly list both
the separate mux/config phandles for each value in pinctrl-names. Still,
this seems a reasonable compromise; the user is still picking from a
bunch of pre-defined/canned nodes, they simply need to list 2 (or n in
general) of them for each state.
> This will be pretty useful for imx6 usdhc case, which will have 3
> pinctrl configuration for each usdhc device (imx6 has 4 usdhc devices),
> pinctrl-50mhz, pinctrl-100mhz and pinctrl-200mhz. All these 3 states
> have the exactly same pinmux settings, and only varies on pinconf.
Yes, I definitely agree there's a need for this.
As an aside, I wonder if the following would be any better:
sdhci@c8000200 {
...
pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
<&pinmux_sdhci> <&pinconf_sdhci_suspend>;
/* Number of entries in pinctrl for each in pinctrl-names */
pinctrl-entries = <2 2>;
pinctrl-names = "active", "suspend";
};
That seems more complex though.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 9:46 ` Dong Aisheng-B29396
2012-01-17 14:13 ` Shawn Guo
@ 2012-01-17 19:28 ` Stephen Warren
2012-01-18 11:06 ` Dong Aisheng-B29396
1 sibling, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-17 19:28 UTC (permalink / raw)
To: Dong Aisheng-B29396, Shawn Guo
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Dong Aisheng wrote at Tuesday, January 17, 2012 2:47 AM:
> Shawn Guo wrote at Tuesday, January 17, 2012 4:24 PM:
> > On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
> > > Stephen Warren wrote:
> > ...
> > > > mux =
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > > > /* Syntax example */
> > > > <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> > >
> > > I'm still think how do we construct the pinmux map for such binding.
> > > The format you're using is:
> > > <&pmx_controller_phandle muxable_entity_id selected_function> For
> > > contruct pinmux map, we need to know at least 3 things for a device:
> > > a) pinctrl device b) function name c) group name.
> > > For a, we can get it from this binding.
> > > But for b and c, since they are constants, how to convert to name string?
> >
> > I guess, for function name, it should be retrieved from the client device node,
> > and for the group name, it should be retrieved from the node here.
>
> I guess Stephen's idea is to retrieving the function name and group name
> From the pinctrl driver since Tagre prefers to define those things in driver
> Rather than in board file or soc.dts file.
> But it does not fit for IMX since we define it in soc.dts.
You can still get the data from the driver, even if the driver got the
data from the DT instead of static tables.
> > For above example, the function name can be picked from sdhci device node
> > pinctr-names property I proposed,
>
> If I understand correctly, the pinctrl-names property you proposed represents
> The pin group state.
Yes, that's my understanding.
...
> And in this way we're still using virtual groups.
> It has no big difference as we did before like:
> pinmux-groups {
> uart4grp: group@0 {
> grp-name = "uart4grp";
> grp-pins = <107 108>;
> grp-mux = <4 4>;
> };
>
> sd4grp: group@1 {
> grp-name = "sd4grp";
> grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> };
> };
I'd prefer not to call these virtual groups, since "group" already has
a meaning to the pinctrl subsystem that is somewhat different. As you've
probably noticed, I've been using the term "pre-defined/canned pin
configuration"!
> The real problem is do we need to support individual pin mux
> Or still using virtual pin group?
>
> For the way Stephen proposed, we can only support individual pin mux
> Since IMX pins are not grouped together in HW.
I think the "mux" and "config" properties I had in my proposal should
deal purely with raw HW entities; nothing virtual/pre-defined/canned/...
You can get what you're calling "virtual groups" simply by defining a
bunch of pinmux configuration nodes in the pin controller's device node
and exclusively referencing those in the per-device pinctrl properties.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 14:13 ` Shawn Guo
@ 2012-01-17 19:32 ` Stephen Warren
2012-01-18 3:44 ` Dong Aisheng-B29396
1 sibling, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-17 19:32 UTC (permalink / raw)
To: Shawn Guo, Dong Aisheng-B29396
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Shawn Guo wrote at Tuesday, January 17, 2012 7:13 AM:
> On Tue, Jan 17, 2012 at 09:46:42AM +0000, Dong Aisheng-B29396 wrote:
> ...
> > I guess Stephen's idea is to retrieving the function name and group name
> > From the pinctrl driver since Tagre prefers to define those things in driver
> > Rather than in board file or soc.dts file.
> > But it does not fit for IMX since we define it in soc.dts.
>
> Hmm, when he came up with this proposal and said it should work for
> Tegra, I assume he plans to move those from Tegra pinctrl driver into
> device tree. Stephen?
I plan to have the following:
tegra20.dtsi:
A regular device node for the pin controller, listing just the basic
device tree properties: "compatible" and "regs".
${board}.dts:
Each device node lists its pin mux/config setup using the bindings we
decide upon.
drivers/pinctrl/pinctrl-tegra20.c:
Static tables listing all pins, pin groups, mux functions, and pin
configuration options that Tegra supports.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-17 18:47 ` Stephen Warren
@ 2012-01-18 3:32 ` Shawn Guo
2012-01-18 19:00 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Shawn Guo @ 2012-01-18 3:32 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Tue, Jan 17, 2012 at 10:47:14AM -0800, Stephen Warren wrote:
> Shawn Guo wrote at Saturday, January 14, 2012 12:09 AM:
> > On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
...
> > > /* 1:n example: */
> > > pinmux =
> > > <"default" &pmx_sdhci_mux_a>
> > > <"default" &pmx_sdhci_pincfg_a>
> > > <"suspend" &pmx_sdhci_mux_a>
> > > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> >
> > I personally do not like the 1:n binding. To me, any particular pinctrl
> > configuration, e.g. pmx_sdhci_active, should consist of a pair of pinmux
> > and pinconf, which should be given by the pmx_sdhci_active node.
>
> Just a further explanation on my original code: I always intended that
> each entry in that list was a full pinmux configuration that could include
> mux and pin configuration settings. Thus, the above is more like:
>
> pinctrl =
> <"default" &pmx_sdhci_a>
> <"default" &pmx_sdhci_overrides>
>
> (overrides rather than explicit separate mux/config; the separate mux
> And config were just an example use case).
>
> My thoughts here:
>
> With this binding, we can certainly define a lot of pre-defined/canned
> configurations to a set of pins. However, there are so many pin config
> options (at least on Tegra) for different aspects of drive strength, slew
> rate, ... that I sincerely doubt every single board is going to be able
> to use one of those pre-defined/canned *exactly* without changes. The ways
> to cope with these small board-specific differences are:
>
> a) Cut/paste the entire pre-defined/canned configuration and tweak it
> as necessary. You can do this with the 1:1 model.
>
> b) Use the pre-defined/canned as a base, then modify it to add extra
> configuration options, or change existing configuration options, as
> appropriate. I think the 1:n model works best for this. Given previous
> comments, I'd now propose the following syntax for a 1:n model:
>
>
> pinctrl = <&pmx_sdhci_a>, <&pmx_sdhci_overrides>, <&pmx_sdhci_suspend>;
> pinctrl-names = "default", "default", "suspend";
>
> This should be easy to implement; instead of roughly:
>
> prop = get_prop(np, "pinctrl-names");
> index = find_index(prop, "default");
> handle_pinctrl_prop(np, "pinctrl", index);
>
> something more like:
>
> prop = get_prop(np, "pinctrl-names");
> prev = NULL;
> while (find_index(prop, "default", &prev))
> handle_pinctrl_prop(np, "pinctrl", index);
>
Ok, I get it. It seems a comprehensive design to me then.
> ...
> > > /*
> > > * The actual definition of the complete state of the
> > > * pinmux as required by some driver.
> > > *
> > > * These can be either directly in the device node, or
> > > * somewhere in tegra20.dtsi in order to provide pre-
> > > * selected/common configurations. Hence, they're referred
> > > * to by phandle above.
> > > */
> > > pmx_sdhci_active: {
> > > /*
> > > * Pin mux settings. Mandatory?
> > > * Not mandatory if the 1:1 mentioned above is
> > > * extended to 1:n.
> > > *
> > > * Format is <&pmx_controller_phandle muxable_entity_id
> > > * selected_function>.
> > > *
> > > * The pmx phandle is required since there may be more
> > > * than one pinmux controller in the system. Even if
> > > * this node is inside the pinmux controller itself, I
> > > * think it's simpler to just always have this field
> > > * present in the binding for consistency.
> > > *
> >
> > I prefer to just put such nodes inside pinctrl controller itself and
> > drop those phandles.
>
> My rationale here:
>
> Forcing those nodes to be inside the controller node forces us to store
> any board-specific custom configurations or overrides in the controller
> node too; I'd simply prefer the flexibility to put them anywhere.
>
Hmm, this type of flexibility does not make too much point to me. On
the contrary, it's good to have a centralized place for these nodes,
so that they can be well organized and people can find them easily.
> Equally, I want SoC vendors to be able to choose whether to use these
> pre-defined/canned configuration nodes at all. If you do use them, putting
> them into the controller node makes perfect sense. If you don't use them,
> putting the pin configuration nodes into the individual device nodes (in
> the board.dts file) makes much more sense.
>
Having 'pinctrl' phandle point to a configuration node which is in the
same device node looks odd to me. I'd rather define these configuration
nodes in <board>.dts still under controller node.
> Having each property start with the phandle of the relevant controller
> is also far more consistent with the way all the GPIO, IRQ, ... bindings
> work.
>
The GPIO and IRQ gets only one level phandle reference from device node
to the destination, while you are proposing two levels of phandle
reference for pinctrl. Having 'pinctrl' phandle point to the
configuration node which sits under controller node seems well aligned
with GPIO and IRQ etc to me.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 14:13 ` Shawn Guo
2012-01-17 19:32 ` Stephen Warren
@ 2012-01-18 3:44 ` Dong Aisheng-B29396
2012-01-18 4:47 ` Shawn Guo
2012-01-18 19:24 ` Stephen Warren
1 sibling, 2 replies; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-18 3:44 UTC (permalink / raw)
To: Shawn Guo
Cc: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
....
> > > It may worth introducing
> > > another level phandle reference. Something like the following:
> > >
> > > pinmux_sdhci: pinmux-sdhci {
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > > };
> > >
> > > pinconf_sdhci_active: pinconf-sdhci-active {
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > };
> > >
> > > pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > };
> > >
> > The config makes sense to me.
> > The only question is how to get group name to match with the predefined groups.
> >
> I still incline to say there should not be any predefined groups for DT case, if
> we choose to find the group in use only when pinmux_get() gets called.
Then we have too issue I said before:
1) Inconsistent issue
sysfs pinmux map entry has different meaning
2) need to change the pinmux_maps from arrays to list for better to dynamically
Insert the new created pinmux map detected.
But this does affect the non-dt case a lot.
> Even if
> you insist to have a global scanning on device tree for all groups to create the
> pinmux mapping table, those predefined groups can only be created by your
> scanning code,
The predefined groups and functions are in soc.dts file and will be parsed
Independently with the pinmux map table creation.
> so you should know how the name comes from device tree, and you
> should be able to recreate the name when you are trying to matching the name in
> the table you created before.
>
> > Besides per pin group configuration support, we may also want per pin
> > configuration Support as the latest patch sent by Linus.
> > http://www.spinics.net/lists/arm-kernel/msg155712.html
> >
> This binding proposal has covered both pin and group configuration, as the
> second parameter of 'config' property could be either a pin or group which
> depends the on whether the configurable entity at HW level is a pin or group.
>
The pinctrl driver does not know if it's group or pin.
We may find a way to identify it.
> ...
>
> > > > If "muxable entities" is pins on IMX, I'm wondering how we define
> > > > the predefined Functions and groups or if we still need to do that.
> > > >
> > > Let's put this in the example below.
> > >
> > > pinmux_usdhc1: pinmux-usdhc1 {
> > > mux = <IMX6Q_PAD_SD1_DAT1 0>
> > > <IMX6Q_PAD_SD1_DAT2 0>
> > > ...
> > > };
> > >
> > Yes, I agree.
> > And in this way we're still using virtual groups.
> > It has no big difference as we did before like:
> > pinmux-groups {
> > uart4grp: group@0 {
> > grp-name = "uart4grp";
> > grp-pins = <107 108>;
> > grp-mux = <4 4>;
> > };
> >
> > sd4grp: group@1 {
> > grp-name = "sd4grp";
> > grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> > grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> > };
> > };
> >
> You are right. They are fundamentally same and just in different format.
>
> > The real problem is do we need to support individual pin mux Or still
> > using virtual pin group?
> > For the way Stephen proposed, we can only support individual pin mux
> > Since IMX pins are not grouped together in HW.
> >
> I do not see any problem here. If you look at the first column of 'mux'
> property of node pinmux-usdhc1, it is a group of pins for usdhc1.
> Isn't it one virtual pin group for usdhc1?
>
If we treat the whole pins in 'mux' property as a group,
There may be potential inconsistent issue since Tegra will treat each
One in the list of 'mux' as a group.
And it would be a problem for pinctrl core to parse it in a standard way.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-17 19:21 ` Stephen Warren
@ 2012-01-18 4:01 ` Shawn Guo
2012-01-18 9:32 ` Dong Aisheng-B29396
1 sibling, 0 replies; 54+ messages in thread
From: Shawn Guo @ 2012-01-18 4:01 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Tue, Jan 17, 2012 at 11:21:30AM -0800, Stephen Warren wrote:
> Shawn Guo wrote at Tuesday, January 17, 2012 1:24 AM:
...
> > Considering the different pinctrl configurations for the same client
> > device usually share the same pinmux and only pinconf varies. It may
> > worth introducing another level phandle reference. Something like
> > the following:
>
> I don't think there's a need for another level of indirection. The 1:n
> model I was talking about already handles this, I believe. See below.
>
Yes, agreed.
...
> > This will be pretty useful for imx6 usdhc case, which will have 3
> > pinctrl configuration for each usdhc device (imx6 has 4 usdhc devices),
> > pinctrl-50mhz, pinctrl-100mhz and pinctrl-200mhz. All these 3 states
> > have the exactly same pinmux settings, and only varies on pinconf.
>
> Yes, I definitely agree there's a need for this.
>
> As an aside, I wonder if the following would be any better:
>
It does look better to me.
Regards,
Shawn
> sdhci@c8000200 {
> ...
> pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
> <&pinmux_sdhci> <&pinconf_sdhci_suspend>;
> /* Number of entries in pinctrl for each in pinctrl-names */
> pinctrl-entries = <2 2>;
> pinctrl-names = "active", "suspend";
> };
>
> That seems more complex though.
>
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 3:44 ` Dong Aisheng-B29396
@ 2012-01-18 4:47 ` Shawn Guo
2012-01-18 19:24 ` Stephen Warren
1 sibling, 0 replies; 54+ messages in thread
From: Shawn Guo @ 2012-01-18 4:47 UTC (permalink / raw)
To: Dong Aisheng-B29396
Cc: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Wed, Jan 18, 2012 at 03:44:59AM +0000, Dong Aisheng-B29396 wrote:
...
> > > The real problem is do we need to support individual pin mux Or still
> > > using virtual pin group?
> > > For the way Stephen proposed, we can only support individual pin mux
> > > Since IMX pins are not grouped together in HW.
> > >
> > I do not see any problem here. If you look at the first column of 'mux'
> > property of node pinmux-usdhc1, it is a group of pins for usdhc1.
> > Isn't it one virtual pin group for usdhc1?
> >
> If we treat the whole pins in 'mux' property as a group,
> There may be potential inconsistent issue since Tegra will treat each
> One in the list of 'mux' as a group.
> And it would be a problem for pinctrl core to parse it in a standard way.
>
Based on my understanding, the 'group' given by 'mux' property may be
a group of pin/group defined by hardware, or mixing of the two.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 19:09 ` Stephen Warren
@ 2012-01-18 7:24 ` Dong Aisheng-B29396
2012-01-18 19:42 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-18 7:24 UTC (permalink / raw)
To: Stephen Warren, linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, Shawn Guo (shawn.guo@linaro.org)
Cc: linux-kernel, linux-arm-kernel, devicetree-discuss
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@nvidia.com]
> Sent: Wednesday, January 18, 2012 3:09 AM
> To: Dong Aisheng-B29396; linus.walleij@stericsson.com; s.hauer@pengutronix.de;
> rob.herring@calxeda.com; kernel@pengutronix.de; cjb@laptop.org; Simon Glass
> (sjg@chromium.org); Dong Aisheng; Shawn Guo (shawn.guo@linaro.org)
> Cc: linux-kernel@vger.kernel.org; linux-arm-kernel@lists.infradead.org;
> devicetree-discuss@lists.ozlabs.org
> Subject: RE: Pinmux bindings proposal
> Importance: High
>
> Dong Aisheng wrote at Monday, January 16, 2012 5:50 AM:
> > Stephen Warren wrote at Saturday, January 14, 2012 4:40 AM:
> > > I thought a bit more about pinmux DT bindings. I came up with
> > > something that I like well enough, and is pretty similar to the binding that
> Dong posted recently.
> > > I think it'll work for both Tegra's and IMX's needs.
> > > Please take a look!
> ...
> > > pinmux =
> > > <"default" &pmx_sdhci_active>
> > > <"suspend" &pmx_sdhci_suspend>;
> ...
> > > /*
> > > * Alternative: One property for each required state. But,
> > > * how does pinctrl core know which properties to parse?
> > > * Every property named "pinctrl*" seems a little too far-
> > > * reaching. Perhaps if we used vendor-name "pinmux", that'd
> > > * be OK, i.e. pinmux,default and pinmux,suspend?
> >
> > It we support whatever device-defined states, it's meaningless to use
> > one property For each required state since pinctrl core does not know
> > the property name the customer defined.
>
> Yes, that's the problem. Shawn proposed copying the clock bindings which I think
> solves all the issues; see my previous email.
> > > */
> > > pinmux = <&pmx_sdhci_active>;
> > > pinmux-suspend <&pmx_sdhci_suspend>;
> > >
> > > /* 1:n example: */
> > > pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
> >
> > This looks ok to me since the mux and pincfg usually is 1 to 1.
> > And if we do not have a pincfg for a pinmux we can find a way to tell
> > the pinctl core, maybe just set pincfg phandle to 0.
>
> Just to be clear, I'll repeat part of my previous response to Shawn:
>
> * Just a further explanation on my original code: I always intended that
> * each entry in that list was a full pinmux configuration that could include
> * mux and pin configuration settings. Thus, the above is more like:
> *
> * pinctrl =
> * <"default" &pmx_sdhci_a>
> * <"default" &pmx_sdhci_overrides>
> *
> * (overrides rather than explicit separate mux/config; the separate mux
> * And config were just an example use case).
>
> So that was a list where the /examples/ had two entries, one for mux and one for
> pin config. In general, there could be 1, 2, 3, ... entries and each could
> define whatever mux and config entries they wanted.
>
I also read your reply to Shawn's mail.
I guess you mean the first "default" entry may not sufficient to use, so we allow
customer defines extra pmx_sdhci_overrides in board.dts to use?
Personally I did not see big benefits for this way but introduce a bit complexity
and make the code not clear and not easily to understand.
Why not define it completely for one pinmux group?
I think it also does not meet the current pinctrl subsystem design.
The group and functions are defined, customer only needs to tell what they want to
Use. For example, in non-dt case, a pinmux map table is enough to use.
> > > pinmux-suspend = <&pmx_sdhci_mux_a
> > > &pmx_sdhci_pincfg_a_suspend>;
> > >
> > > /*
> > > * The actual definition of the complete state of the
> > > * pinmux as required by some driver.
> > > *
> > > * These can be either directly in the device node, or
> > > * somewhere in tegra20.dtsi in order to provide pre-
> > > * selected/common configurations. Hence, they're referred
> > > * to by phandle above.
> > > */
> > > pmx_sdhci_active: {
> > > /*
> > > * Pin mux settings. Mandatory?
> >
> > Mandatory for what?
>
> I was thinking that each pin mux configuration node MUST specify at least some
> mux settings. However, that may not make sense; it may be reasonable to specify
> just pin config settings and no mux settings (in a 1:n model in the "override"
> node).
>
Yes, after looking Linus's patch to support pin states, I'm thinking for pin config
case we may need such a property or flag to tell which config of state to use by
default since we support multi states.
> >
> > > * Not mandatory if the 1:1 mentioned above is
> > > * extended to 1:n.
> > > *
> > > * Format is <&pmx_controller_phandle
> muxable_entity_id
> > > * selected_function>.
> > > *
> > > * The pmx phandle is required since there may be
> more
> > > * than one pinmux controller in the system. Even if
> > > * this node is inside the pinmux controller itself,
> I
> > > * think it's simpler to just always have this field
> > > * present in the binding for consistency.
> > > *
> > > * Alternative: Format is <&pmx_controller_phandle
> > > * pmx_controller_specific_data>. In this case, the
> > > * pmx controller needs to define #pinmux-mux-cells,
> > > * and provide the pinctrl core with a mapping
> > > * function to handle the rest of the data in the
> > > * property. This is how GPIOs and interrupts work.
> > > * However, this will probably interact badly with
> > > * wanting to parse the entire pinmux map early in
> > > * boot, when perhaps the pinctrl core is
> initialized,
> > > * but the pinctrl driver itself is not.
> > > */
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> > > /* Syntax example */
> > > <&foo_pmx FOO_PMX_PG_X
> > > FOO_PMX_MUX_0>;
> >
> > I'm still think how do we construct the pinmux map for such binding.
> > The format you're using is:
> > <&pmx_controller_phandle muxable_entity_id selected_function> For
> > contruct pinmux map, we need to know at least 3 things for a device:
> > a) pinctrl device b) function name c) group name.
> > For a, we can get it from this binding.
> > But for b and c, since they are constants, how to convert to name string?
>
> a) pinctrl device: We can extract this from pmx_controller_phandle; simply
> search for a device with the same OF node, and retrieve that registered device
> from the pinctrl subsystem. This is how GPIO and IRQ work.
>
Yes, I know this.
> b) function name: The pinmux_ops for the driver of pmx_controller_phandle needs
> a function to convert integer IDs such as TEGRA_PMX_MUX_1 to whatever function
> IDs are used by the pinctrl subsystem.
>
No, I guess Tegra can do this since tegra prefers to define functions and groups
In pinctrl driver.
But for IMX, we do not have these info before the imx-pinctrl driver gets run and
Parse the device tree.
> c) group name: This should be handled just like (b).
>
> Also you'll need to know:
>
> struct pinmux_map's .name field. This is the value in the pinctrl-names property,
> assuming we're switching to the following syntax:
>
> pinctrl = <&pmx_sdhci_a_active>, <&pmx_sdhci_a_suspend>;
> pinctrl-names = "default", "suspend";
>
Here the pinctrl-names are representing 'state'.
Is pinmux_map.name field used for this?
> > > /*
> > > * Pin configuration settings. Optional.
> > > *
> > > * Format is <&pmx_controller_phandle
> muxable_entity_id
> > > * configuration_option configuration_value>.
> > > */
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > /*
> > > * Perhaps allow additional custom properties here
> to
> > > * express things we haven't thought of. The pinctrl
> > > * drivers would be responsible for parsing them.
> > > */
> > > };
> > > pmx_sdhci_standby: {
> > > mux =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_MUX_1>;
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD
> > > TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > };
> > > };
> > > };
> > >
> > > Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
> >
> > If "muxable entities" is pins on IMX, I'm wondering how we define the
> > predefined Functions and groups or if we still need to do that.
>
> By "predefined", do you mean:
>
> a) Does the DT need to list all the functions and groups.
>
> The answer here is no in general. However, if you want to parameterize your
> pinctrl driver's list of known functions and groups, then you can certainly do
> this; the pinctrl binding simple doesn't force you to do this.
>
> b) Does the pinctrl driver need to list all functions and groups to the pinctrl
> core?
>
> As the pinctrl subsystem APIs stand right now, the answer here is yes.
>
> I don't think there's really any way around this, although depending on HW, you
> can do things like:
>
> 1) Instead of defining logical functions (SDHCI1, I2C1, ...) you could simply
> define a function for each mux register value (func0, func1, ...)
>
> 2) On IMX, you'd define a pin group for each individual pin, since the HW muxing
> is at a pin not a group level.
>
Yes, this is what I'm thinking now.
If we decide to do that, the pin groups concepts of current pinmux subsystem
Seems does not fit for IMX a lot since each group is only one pin.
And the functions/groups would be hugh since IMX6Q has 196 pins and each pin may have
7 functions.
> > > TEGRA_PMX_PG_DTA
> > > TEGRA_PMX_PG_DTD
> > >
> > > Each individual pinmux driver's bindings needs to define what each
> > > integer ID represents.
> >
> > Does it mean both pinmux driver and soc.dtsi file need define those
> > macros if dtc Supports constants?
>
> Yes. As Shawn mentions later, perhaps we can find some way to define these
> values in one place, and generate both a .dtsi file and a .h file from that
> single list, or generate a .h file from the .dtsi file or vice- versa.
>
> > > Integer IDs for the "mux functions". Note that these are the raw
> > > values written into hardware, not any driver-defined abstraction,
> > > and not any kind of "virtual group" that's been invented to make OEMs life
> easier:
> > >
> > > TEGRA_PMX_MUX_0
> > > TEGRA_PMX_MUX_1
> > > ...
> > > TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
> > >
> > > Since these are the raw IDs that go into HW, there's no need to
> > > specify each ID's meanings in the binding.
> > >
> > > Integer IDs for "pin config parameters":
> > >
> > > TEGRA_PMX_CONF_TRISTATE
> > > TEGRA_PMX_CONF_DRIVE_STRENGTH
> > > TEGRA_PMX_CONF_SLEW_RATE
> > >
> > > Each individual pinmux driver's bindings needs to define what each
> > > integer ID represents, and what the legal "value"s are for each one.
> > >
> > > Advantages:
> > >
> > > * Provides for both mux settings and "pin configuration".
> > >
> > > * Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
> > > file if desired to provide pre-defined pinmux configurations to
> > > choose from.
> > >
> > > * Allows the "pinmux configuration" nodes to be part of the per-device
> > > node if you don't want to use pre-defined configurations.
> > >
> > > * When pre-defined configurations are present, if you need something
> > > custom, you can do it easily.
> > >
> > > * Can augment pre-defined configurations by listing n nodes for each
> > > "name"d pinmux configuration, e.g. to add one extra pin config
> > > value.
> > >
> > > * Parsing is still quite simple:
> > > 1) Parse "pinmux" property in device node to get phandle.
> > > 2) Parse "mux" property in the node reference by the phandle,
> > > splitting into a list of pmx phandle, entity, mux func.
> > > 3) For each entry, pass entity, function to the appropriate mux
> > > driver. (For U-Boot, this might mean check that the phandle
> > > points at the expected place, and ignore the entry if not?)
> > > 4) Mux driver simply converts "muxable entity" to the register
> > > address, write the "function" value straight to the register.
> > >
> > > Disadvantages:
> > >
> > > * If you're not using pre-defined configurations, you still have to dump
> > > all the pinmux configuration into a sub-node of the device node, and
> > > have a property point at it using a phandle. This is slightly more
> > > complex than simply putting the mux/config properties right into the
> > > device node. However, it additionally allows one to have multiple
> > > "name"d configurations (e.g. for suspend) very easily, and isn't overly
> > > complex, so I can live with this.
> >
> > I don't think the sub-node of the device node is a good place to
> > define Custom configurations.
> > Putting those things into device node will make its size big and also not look
> good.
> > Since it uses phandle, why not put it under pinctrl device node in board dts
> file?
> > It may also work.
>
> Both you and Shawn have said you don't like allowing the pin configurations to
> be sub-nodes of the devices. Can you expand a little more on that?
Actually I agree to reserve the pinctrl device's phandle
But..
> I mentioned
> some of my reasons for allowing this in my previous email, so see that for
> reference. I guess I have a couple more points to make:
>
> 1) It's optional. If everything you need can be represented in pre-
> defined/canned pin configuration nodes under the pin controller's node, you can
> certainly do that.
>
> 2) Putting this in the device's node seems more consistent with existing
> bindings, which put all configuration inside the device node: The IRQ binding
> places interrupt IDs and configuration (level/edge, high/low) in the device node.
> The GPIO binding places GPIO IDs and possible flags (e.g. inverted/not) in the
> device node. Etc.
>
Actually it's not a big issue and we do not have the mandatory requirement that
You have to put it in somewhere since it's referenced by phandle.
I just prefer to put it to under pinctrl device node in board.dts file to
Concentrate manager, becase I think it's a little big and more complicated and
not like not like IRQ and GPIO case.
> 3) I don't think there will be any .dtb size difference at all between putting
> the pin configuration in the pin controller node or the device node; the content
> of the sub-node will be entirely identical. The same holds true for the .dts
> files (when you consider soc.dtsi and board.dts together).
>
> --
> nvpublic
>
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 19:21 ` Stephen Warren
2012-01-18 4:01 ` Shawn Guo
@ 2012-01-18 9:32 ` Dong Aisheng-B29396
1 sibling, 0 replies; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-18 9:32 UTC (permalink / raw)
To: Stephen Warren, Shawn Guo
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@nvidia.com]
....
> > Considering the different pinctrl configurations for the same client
> > device usually share the same pinmux and only pinconf varies. It may
> > worth introducing another level phandle reference. Something like the
> > following:
>
> I don't think there's a need for another level of indirection. The 1:n model I
> was talking about already handles this, I believe. See below.
>
> > pinmux_sdhci: pinmux-sdhci {
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > };
> >
> > pinconf_sdhci_active: pinconf-sdhci-active {
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
> >
> > pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
>
> Those 3 nodes make sense to me.
>
> > pinctrl_sdhci_active: pinctrl-sdhci-active {
> > pinmux = <&pinmux_sdhci>;
> > pinconf = <&pinconf_sdhci_active>;
> > };
> >
> > pinctrl_sdhci_suspend: pinctrl-sdhci-suspend {
> > pinmux = <&pinmux_sdhci>;
> > pinconf = <&pinconf_sdhci_suspend>;
> > };
>
> I think we can avoid those 2 nodes.
>
> > sdhci@c8000200 {
> > ...
> > pinctrl = <&pinctrl_sdhci_active> <&pinctrl_sdhci_suspend>;
> > pinctrl-names = "active", "suspend";
> > };
>
> And rewrite that node as:
>
> sdhci@c8000200 {
> ...
> pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
> <&pinmux_sdhci> <&pinconf_sdhci_suspend>;
> pinctrl-names = "active", "active", "suspend", "suspend"; };
>
> The only slight disadvantage here is that the person constructing the
> pinctrl/pinctrl-names properties needs to know to explicitly list both the
> separate mux/config phandles for each value in pinctrl-names. Still, this seems
> a reasonable compromise; the user is still picking from a bunch of pre-
> defined/canned nodes, they simply need to list 2 (or n in
> general) of them for each state.
>
> > This will be pretty useful for imx6 usdhc case, which will have 3
> > pinctrl configuration for each usdhc device (imx6 has 4 usdhc
> > devices), pinctrl-50mhz, pinctrl-100mhz and pinctrl-200mhz. All these
> > 3 states have the exactly same pinmux settings, and only varies on pinconf.
>
> Yes, I definitely agree there's a need for this.
>
> As an aside, I wonder if the following would be any better:
>
> sdhci@c8000200 {
> ...
> pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
> <&pinmux_sdhci> <&pinconf_sdhci_suspend>;
> /* Number of entries in pinctrl for each in pinctrl-names */
> pinctrl-entries = <2 2>;
> pinctrl-names = "active", "suspend"; };
>
> That seems more complex though.
>
This makes sense to me.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-17 19:28 ` Stephen Warren
@ 2012-01-18 11:06 ` Dong Aisheng-B29396
2012-01-20 20:28 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Dong Aisheng-B29396 @ 2012-01-18 11:06 UTC (permalink / raw)
To: Stephen Warren, Shawn Guo
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
> -----Original Message-----
> From: Stephen Warren [mailto:swarren@nvidia.com]
> Sent: Wednesday, January 18, 2012 3:29 AM
> To: Dong Aisheng-B29396; Shawn Guo
> Cc: linus.walleij@stericsson.com; s.hauer@pengutronix.de;
> rob.herring@calxeda.com; kernel@pengutronix.de; cjb@laptop.org; Simon Glass
> (sjg@chromium.org); Dong Aisheng; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree-discuss@lists.ozlabs.org
> Subject: RE: Pinmux bindings proposal
> Importance: High
>
> Dong Aisheng wrote at Tuesday, January 17, 2012 2:47 AM:
> > Shawn Guo wrote at Tuesday, January 17, 2012 4:24 PM:
> > > On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
> > > > Stephen Warren wrote:
> > > ...
> > > > > mux =
> > > > > <&tegra_pmx TEGRA_PMX_PG_DTA
> TEGRA_PMX_MUX_1>
> > > > > <&tegra_pmx TEGRA_PMX_PG_DTD
> TEGRA_PMX_MUX_1>
> > > > > /* Syntax example */
> > > > > <&foo_pmx FOO_PMX_PG_X
> > > > > FOO_PMX_MUX_0>;
> > > >
> > > > I'm still think how do we construct the pinmux map for such binding.
> > > > The format you're using is:
> > > > <&pmx_controller_phandle muxable_entity_id selected_function> For
> > > > contruct pinmux map, we need to know at least 3 things for a device:
> > > > a) pinctrl device b) function name c) group name.
> > > > For a, we can get it from this binding.
> > > > But for b and c, since they are constants, how to convert to name string?
> > >
> > > I guess, for function name, it should be retrieved from the client
> > > device node, and for the group name, it should be retrieved from the node
> here.
> >
> > I guess Stephen's idea is to retrieving the function name and group
> > name From the pinctrl driver since Tagre prefers to define those
> > things in driver Rather than in board file or soc.dts file.
> > But it does not fit for IMX since we define it in soc.dts.
>
> You can still get the data from the driver, even if the driver got the data from
> the DT instead of static tables.
>
The problem is that the pinmux map parsing is earlier than parsing the pinmux data,
And the driver still have no chance to run.
This is using bus notifier case.
For scanning case, the pinmux map parsing could be later than driver running.
> > > For above example, the function name can be picked from sdhci device
> > > node pinctr-names property I proposed,
> >
> > If I understand correctly, the pinctrl-names property you proposed
> > represents The pin group state.
>
> Yes, that's my understanding.
>
> ...
> > And in this way we're still using virtual groups.
> > It has no big difference as we did before like:
> > pinmux-groups {
> > uart4grp: group@0 {
> > grp-name = "uart4grp";
> > grp-pins = <107 108>;
> > grp-mux = <4 4>;
> > };
> >
> > sd4grp: group@1 {
> > grp-name = "sd4grp";
> > grp-pins = <170 171 180 181 182 183 184 185 186 187>;
> > grp-mux = <0 0 1 1 1 1 1 1 1 1>;
> > };
> > };
>
> I'd prefer not to call these virtual groups, since "group" already has a meaning
> to the pinctrl subsystem that is somewhat different. As you've probably noticed,
> I've been using the term "pre-defined/canned pin configuration"!
>
> > The real problem is do we need to support individual pin mux Or still
> > using virtual pin group?
> >
> > For the way Stephen proposed, we can only support individual pin mux
> > Since IMX pins are not grouped together in HW.
>
> I think the "mux" and "config" properties I had in my proposal should deal
> purely with raw HW entities; nothing virtual/pre-defined/canned/...
>
The question here is we defining a common model for pinctrl dt binding and
different SoC may have different characteristics like between Tegra and IMX.
So the proposal based on one SoC's raw HW entities should fit other SoCs.
> You can get what you're calling "virtual groups" simply by defining a bunch of
> pinmux configuration nodes in the pin controller's device node and exclusively
> referencing those in the per-device pinctrl properties.
>
Yes, for the pin config I think we have done some agreement.
Now i'm still thinking if the mux way we discussed here fits IMX.
For example:
> pinmux_sdhci: pinmux-sdhci {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> };
It may work well for Tegra.
When pinctrl core parsing the mux proterty, it will creat each maps
For each entry in the list.
But for IMX like:
> > pinmux_usdhc1: pinmux-usdhc1 {
> > mux = <IMX6Q_PAD_SD1_DAT1 0>
> > <IMX6Q_PAD_SD1_DAT2 0>
> > ...
> > };
Each entry in the list is PIN rather than group.
So we cannot create each map for each entry in the list except we treat the PIN as
A group(this will cause a huge maps and predefined groups and functions).
A little different way I'm thinking now is cover the mux difference(pin or group)
between Tegra and IMX in Pinctrl driver, then we may not need the second mux value
(e.g: TEGRA_PMX_MUX_1)in the standard mux property:
The mux property can become:
pinmux_sdhci: pinmux-sdhci {
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA_X>
<&tegra_pmx TEGRA_PMX_PG_DTD_Y>;
};
The group TEGRA_PMX_PG_DTA_X should already know it's mux value(TEGRA_PMX_MUX_1) for a specific
Function.
For IMX, it could be like:
pinmux_sdhci: pinmux-sdhci {
mux =
<&imx_pmx &pinmux_sd1grp>;
};
For cover the difference whether the pingroups are defined in drive or soc.dts file.
The second param could be a name, like:
pinmux_sdhci: pinmux-sdhci {
mux =
<&imx_pmx "sd1grp">;
};
And the using is still like:
sdhci@c8000200 {
...
pinctrl = <&pinmux_sdhci> <&pinconf_sdhci_active>
<&pinmux_sdhci> <&pinconf_sdhci_suspend>;
/* Number of entries in pinctrl for each in pinctrl-names */
pinctrl-entries = <2 2>;
pinctrl-names = "active", "suspend"; };
Another reason for this mode is that maybe some SoCs even does not have mux function for
Each pin or group. And current pinctrl subsystem design also does not have this conception.
Regards
Dong Aisheng
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
` (2 preceding siblings ...)
2012-01-16 18:28 ` Grant Likely
@ 2012-01-18 12:16 ` Thomas Abraham
2012-01-18 19:52 ` Stephen Warren
2012-01-19 13:10 ` Thomas Abraham
4 siblings, 1 reply; 54+ messages in thread
From: Thomas Abraham @ 2012-01-18 12:16 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Hi Stephen,
On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
> I thought a bit more about pinmux DT bindings. I came up with something
> that I like well enough, and is pretty similar to the binding that Dong
> posted recently. I think it'll work for both Tegra's and IMX's needs.
> Please take a look!
>
> Note: I've used named constants below just to make this easier to read.
> We still don't have a solution to actually use named constants in dtc yet.
>
> tegra20.dtsi:
>
> / {
> tegra_pmx: pinmux@70000000 {
> compatible = "nvidia,tegra20-pinmux";
> reg = <0x70000014 0x10 /* Tri-state registers */
> 0x70000080 0x20 /* Mux registers */
> 0x700000a0 0x14 /* Pull-up/down registers */
> 0x70000868 0xa8>; /* Pad control registers */
> };
>
> sdhci@c8000200 {
> compatible = "nvidia,tegra20-sdhci";
> reg = <0xc8000200 0x200>;
> interrupts = <0 15 0x04>;
> };
> };
>
> tegra-harmony.dts:
>
> /{
> sdhci@c8000200 {
> cd-gpios = <&gpio 69 0>; /* gpio PI5 */
> wp-gpios = <&gpio 57 0>; /* gpio PH1 */
> power-gpios = <&gpio 155 0>; /* gpio PT3 */
>
> /*
> * A list of named configurations that this device needs.
> * Format is a list of <"name" &phandle_of_pmx_configuration>
> *
> * Multiple "name"s are needed e.g. to support active/suspend,
> * or whatever device-defined states are appropriate. The
> * device defines which names are needed, just like a device
> * defines which regulators, clocks, GPIOs, interrupts, ...
> * it needs.
> *
> * This example shows a 1:1 relation between name and phandle.
> * We might want a 1:n relation, so that we can blend multiple
> * pre-defined sets of data together, e.g. one pre-defined set
> * for the pin mux configuration, another for the pin config
> * settings, both being put into the single "default" setting
> * for this one device.
> *
> * A pinmux controller can contain this property too, to
> * define "hogged" or "system" pin mux configuration.
> *
> * Note: Mixing strings and integers in a property seems
> * unusual. However, I have seen other bindings floating
> * around that are starting to do this...
> */
> pinmux =
> <"default" &pmx_sdhci_active>
> <"suspend" &pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux =
> <"default" &pmx_sdhci_mux_a>
> <"default" &pmx_sdhci_pincfg_a>
> <"suspend" &pmx_sdhci_mux_a>
> <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>
> /*
> * Alternative: One property for each required state. But,
> * how does pinctrl core know which properties to parse?
> * Every property named "pinctrl*" seems a little too far-
> * reaching. Perhaps if we used vendor-name "pinmux", that'd
> * be OK, i.e. pinmux,default and pinmux,suspend?
> */
> pinmux = <&pmx_sdhci_active>;
> pinmux-suspend <&pmx_sdhci_suspend>;
>
> /* 1:n example: */
> pinmux = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a>
> pinmux-suspend = <&pmx_sdhci_mux_a &pmx_sdhci_pincfg_a_suspend>;
>
> /*
> * The actual definition of the complete state of the
> * pinmux as required by some driver.
> *
> * These can be either directly in the device node, or
> * somewhere in tegra20.dtsi in order to provide pre-
> * selected/common configurations. Hence, they're referred
> * to by phandle above.
> */
> pmx_sdhci_active: {
> /*
> * Pin mux settings. Mandatory?
> * Not mandatory if the 1:1 mentioned above is
> * extended to 1:n.
> *
> * Format is <&pmx_controller_phandle muxable_entity_id
> * selected_function>.
> *
> * The pmx phandle is required since there may be more
> * than one pinmux controller in the system. Even if
> * this node is inside the pinmux controller itself, I
> * think it's simpler to just always have this field
> * present in the binding for consistency.
> *
> * Alternative: Format is <&pmx_controller_phandle
> * pmx_controller_specific_data>. In this case, the
> * pmx controller needs to define #pinmux-mux-cells,
> * and provide the pinctrl core with a mapping
> * function to handle the rest of the data in the
> * property. This is how GPIOs and interrupts work.
> * However, this will probably interact badly with
> * wanting to parse the entire pinmux map early in
> * boot, when perhaps the pinctrl core is initialized,
> * but the pinctrl driver itself is not.
> */
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> /* Syntax example */
> <&foo_pmx FOO_PMX_PG_X FOO_PMX_MUX_0>;
> /*
> * Pin configuration settings. Optional.
> *
> * Format is <&pmx_controller_phandle muxable_entity_id
> * configuration_option configuration_value>.
> */
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> /*
> * Perhaps allow additional custom properties here to
> * express things we haven't thought of. The pinctrl
> * drivers would be responsible for parsing them.
> */
> };
> pmx_sdhci_standby: {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
> };
> };
>
> Integer IDs for "muxable entities": Pins on IMX, pin groups on Tegra:
>
> TEGRA_PMX_PG_DTA
> TEGRA_PMX_PG_DTD
>
> Each individual pinmux driver's bindings needs to define what each integer
> ID represents.
>
> Integer IDs for the "mux functions". Note that these are the raw values
> written into hardware, not any driver-defined abstraction, and not any
> kind of "virtual group" that's been invented to make OEMs life easier:
>
> TEGRA_PMX_MUX_0
> TEGRA_PMX_MUX_1
> ...
> TEGRA_PMX_MUX_3 (for Tegra, 7 for IMX)
>
> Since these are the raw IDs that go into HW, there's no need to specify
> each ID's meanings in the binding.
>
> Integer IDs for "pin config parameters":
>
> TEGRA_PMX_CONF_TRISTATE
> TEGRA_PMX_CONF_DRIVE_STRENGTH
> TEGRA_PMX_CONF_SLEW_RATE
>
> Each individual pinmux driver's bindings needs to define what each integer
> ID represents, and what the legal "value"s are for each one.
>
> Advantages:
>
> * Provides for both mux settings and "pin configuration".
>
> * Allows the "pinmux configuration" nodes to be part of the SoC .dtsi
> file if desired to provide pre-defined pinmux configurations to
> choose from.
>
> * Allows the "pinmux configuration" nodes to be part of the per-device
> node if you don't want to use pre-defined configurations.
>
> * When pre-defined configurations are present, if you need something
> custom, you can do it easily.
>
> * Can augment pre-defined configurations by listing n nodes for each
> "name"d pinmux configuration, e.g. to add one extra pin config
> value.
>
> * Parsing is still quite simple:
> 1) Parse "pinmux" property in device node to get phandle.
> 2) Parse "mux" property in the node reference by the phandle,
> splitting into a list of pmx phandle, entity, mux func.
> 3) For each entry, pass entity, function to the appropriate mux
> driver. (For U-Boot, this might mean check that the phandle
> points at the expected place, and ignore the entry if not?)
> 4) Mux driver simply converts "muxable entity" to the register
> address, write the "function" value straight to the register.
>
> Disadvantages:
>
> * If you're not using pre-defined configurations, you still have to dump
> all the pinmux configuration into a sub-node of the device node, and
> have a property point at it using a phandle. This is slightly more
> complex than simply putting the mux/config properties right into the
> device node. However, it additionally allows one to have multiple
> "name"d configurations (e.g. for suspend) very easily, and isn't overly
> complex, so I can live with this.
>
> Changes to pinctrl subsystem:
>
> Very little, I think:
>
> * Need to pass raw function IDs through to the driver instead of the driver
> defining some logical table. Actually, this is just a change to individual
> drivers, since they can define the functions "func0", "func1", ... "funcn"
> as I mentioned before.
>
> * Need to add the code to actually parse this of course.
>
> One additional thought: If dtc does grow named constants, we can provide
> HW-function-based names for the mux functions, e.g.:
>
> TEGRA_PMX_DTA_RSVD0 0
> TEGRA_PMX_DTA_SDIO2 1
> TEGRA_PMX_DTA_VI 2
> TEGRA_PMX_DTA_RSVD3 3
>
> TEGRA_PMX_DTF_I2C3 0
> TEGRA_PMX_DTF_RSVD1 1
> TEGRA_PMX_DTF_VI 2
> TEGRA_PMX_DTF_RSVD3 3
> ...
>
> That'd allow the .dts to include functionality-based named for the mux
> function selection, but the .dtb to still include the raw HW mux field
> values. And this is something completely within the control of the SoC
> .dtsi file; if you don't like it, you don't have to do it.
>
> --
> nvpublic
The pinmux_get() function checks if there is an active user of the
pinmux and declines requests if the pinmux has been taken. With the dt
bindings that you have listed, can such a check be still enforced.
Also, will it be possible to support runtime pinmuxing with the above
listed dt bindings?
Thanks,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-16 18:28 ` Grant Likely
@ 2012-01-18 14:13 ` Tony Lindgren
2012-01-18 14:30 ` Shawn Guo
2012-01-18 20:02 ` Stephen Warren
0 siblings, 2 replies; 54+ messages in thread
From: Tony Lindgren @ 2012-01-18 14:13 UTC (permalink / raw)
To: Grant Likely
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Hi,
* Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > pinmux =
> > <"default" &pmx_sdhci_active>
> > <"suspend" &pmx_sdhci_suspend>;
> >
> > /* 1:n example: */
> > pinmux =
> > <"default" &pmx_sdhci_mux_a>
> > <"default" &pmx_sdhci_pincfg_a>
> > <"suspend" &pmx_sdhci_mux_a>
> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>
>
> Yeah, don't do this. Mixing phandle, string and cell values in a
> property gets messy and could become troublesome to parse. I've
> backed away from it in the clk binding.
Yup, that's because the string is embedded directly into the mixed
mode array and will likely make the following data unaligned. That
means it's extremely flakey to parse, and will lead into horrible
errors if you have typos in the .dts file.. Tried that and gave up
on it.
I think I've found a way to avoid using names at all, assuming we set
each pin as a phandle for the drivers to use :)
If some drivers need to use pin names, they should be optional
optional for the cases where the phandle is not available.
Here's what I'm currently using:
pinnmux@4a100000 {
compatible = "pinmux-simple";
reg = <0x4a100000 0x01d4>;
#address-cells = <1>;
#size-cells = <0>;
#pinmux-cells = <2>;
/* uart3_rx_irrx dmtimer8_pwm_evt na gpio_143 na na na safe_mode */
uart3_rx_irrx: mux-uart3_rx_irrx@4a100144 {
reg = <0x4a100144>;
gpios = <&gpio1 1 0>;
#pin-args = <1>;
};
/* uart3_tx_irtx dmtimer9_pwm_evt na gpio_144 na na na safe_mode */
uart3_tx_irtx: mux-uart3_tx_irtx@4a100146 {
reg = <0x4a100146>;
gpios = <&gpio1 2 0>;
#pin-args = <1>;
};
...
};
serial@48020000 {
compatible = "ti,8250";
reg = <0x48020000 0x100>;
reg-shift = <2>;
interrupts = <106>;
pins = <&uart3_rx_irrx 0x10
&uart3_tx_irtx 0x0>;
};
Here I have just one value for each pin register but note that the
#pin-args allows specifying the number or configuration options
depending on the hardware like of_gpio.c does using
of_parse_phandle_with_args().
The various functions for each pin controller register are only
in the comments, so currently the function needs to be provided as
a value. A preprosessor should solve that issue at some point.
This should work for anything that has one register per pin.I'd
assume the pin groups can be described here too for the hardware
that one register per pin group only just by specifying group-pins
array there?
Cheers,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 14:13 ` Tony Lindgren
@ 2012-01-18 14:30 ` Shawn Guo
2012-01-18 15:32 ` Tony Lindgren
2012-01-18 20:02 ` Stephen Warren
1 sibling, 1 reply; 54+ messages in thread
From: Shawn Guo @ 2012-01-18 14:30 UTC (permalink / raw)
To: Tony Lindgren
Cc: Grant Likely, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
On 18 January 2012 22:13, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
>> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
>> > pinmux =
>> > <"default" &pmx_sdhci_active>
>> > <"suspend" &pmx_sdhci_suspend>;
>> >
>> > /* 1:n example: */
>> > pinmux =
>> > <"default" &pmx_sdhci_mux_a>
>> > <"default" &pmx_sdhci_pincfg_a>
>> > <"suspend" &pmx_sdhci_mux_a>
>> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
>>
>>
>> Yeah, don't do this. Mixing phandle, string and cell values in a
>> property gets messy and could become troublesome to parse. I've
>> backed away from it in the clk binding.
>
> Yup, that's because the string is embedded directly into the mixed
> mode array and will likely make the following data unaligned. That
> means it's extremely flakey to parse, and will lead into horrible
> errors if you have typos in the .dts file.. Tried that and gave up
> on it.
>
> I think I've found a way to avoid using names at all, assuming we set
> each pin as a phandle for the drivers to use :)
>
The problem with doing that is we will have to represent each pin as a
node in device tree. For imx6q case, we have 197 pins. Doing so will
bloat the device tree.
--
Regards,
Shawn
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 14:30 ` Shawn Guo
@ 2012-01-18 15:32 ` Tony Lindgren
2012-01-18 16:29 ` Tony Lindgren
2012-01-18 20:20 ` Grant Likely
0 siblings, 2 replies; 54+ messages in thread
From: Tony Lindgren @ 2012-01-18 15:32 UTC (permalink / raw)
To: Shawn Guo
Cc: Grant Likely, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
* Shawn Guo <shawn.guo@linaro.org> [120118 05:57]:
> On 18 January 2012 22:13, Tony Lindgren <tony@atomide.com> wrote:
> > Hi,
> >
> > * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> >> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> >> > pinmux =
> >> > <"default" &pmx_sdhci_active>
> >> > <"suspend" &pmx_sdhci_suspend>;
> >> >
> >> > /* 1:n example: */
> >> > pinmux =
> >> > <"default" &pmx_sdhci_mux_a>
> >> > <"default" &pmx_sdhci_pincfg_a>
> >> > <"suspend" &pmx_sdhci_mux_a>
> >> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> >>
> >>
> >> Yeah, don't do this. Mixing phandle, string and cell values in a
> >> property gets messy and could become troublesome to parse. I've
> >> backed away from it in the clk binding.
> >
> > Yup, that's because the string is embedded directly into the mixed
> > mode array and will likely make the following data unaligned. That
> > means it's extremely flakey to parse, and will lead into horrible
> > errors if you have typos in the .dts file.. Tried that and gave up
> > on it.
> >
> > I think I've found a way to avoid using names at all, assuming we set
> > each pin as a phandle for the drivers to use :)
> >
> The problem with doing that is we will have to represent each pin as a
> node in device tree. For imx6q case, we have 197 pins. Doing so will
> bloat the device tree.
Sure there's some overhead. I've got it working with 220 pins, it's
not too bad as threre's not much string parsing involved.
I don't have all the devices mapping the pins though. The .dtb for
omap4 is about 25k now.
If we wanted to avoid adding phandles for each pin, then we could do:
serial@0x48020000 {
compatible = "ti,8250";
reg = <0x48020000 0x100>;
reg-shift = <2>;
interrupts = <106>;
/* controller, offset, value */
pins = <&mux1 0xabcd 0x10
&mux1 0xabcf 0x0>;
};
But then the .dts file becomes an unreadable matrix unless we have
a preprocessor..
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 15:32 ` Tony Lindgren
@ 2012-01-18 16:29 ` Tony Lindgren
2012-01-18 20:22 ` Grant Likely
2012-01-18 20:20 ` Grant Likely
1 sibling, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-18 16:29 UTC (permalink / raw)
To: Shawn Guo
Cc: Grant Likely, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
* Tony Lindgren <tony@atomide.com> [120118 07:00]:
> * Shawn Guo <shawn.guo@linaro.org> [120118 05:57]:
> > On 18 January 2012 22:13, Tony Lindgren <tony@atomide.com> wrote:
> > > Hi,
> > >
> > > * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> > >> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > >> > pinmux =
> > >> > <"default" &pmx_sdhci_active>
> > >> > <"suspend" &pmx_sdhci_suspend>;
> > >> >
> > >> > /* 1:n example: */
> > >> > pinmux =
> > >> > <"default" &pmx_sdhci_mux_a>
> > >> > <"default" &pmx_sdhci_pincfg_a>
> > >> > <"suspend" &pmx_sdhci_mux_a>
> > >> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> > >>
> > >>
> > >> Yeah, don't do this. Mixing phandle, string and cell values in a
> > >> property gets messy and could become troublesome to parse. I've
> > >> backed away from it in the clk binding.
> > >
> > > Yup, that's because the string is embedded directly into the mixed
> > > mode array and will likely make the following data unaligned. That
> > > means it's extremely flakey to parse, and will lead into horrible
> > > errors if you have typos in the .dts file.. Tried that and gave up
> > > on it.
> > >
> > > I think I've found a way to avoid using names at all, assuming we set
> > > each pin as a phandle for the drivers to use :)
> > >
> > The problem with doing that is we will have to represent each pin as a
> > node in device tree. For imx6q case, we have 197 pins. Doing so will
> > bloat the device tree.
>
> Sure there's some overhead. I've got it working with 220 pins, it's
> not too bad as threre's not much string parsing involved.
>
> I don't have all the devices mapping the pins though. The .dtb for
> omap4 is about 25k now.
>
> If we wanted to avoid adding phandles for each pin, then we could do:
>
> serial@0x48020000 {
> compatible = "ti,8250";
> reg = <0x48020000 0x100>;
> reg-shift = <2>;
> interrupts = <106>;
>
> /* controller, offset, value */
> pins = <&mux1 0xabcd 0x10
> &mux1 0xabcf 0x0>;
> };
>
> But then the .dts file becomes an unreadable matrix unless we have
> a preprocessor..
Forgot to mention that as long as we all standardize to use something
common for #pin-args and of_parse_phandle_with_args(), the pin mapping
could depend on the pinmux driver for selecting whether or not to use
a phandle for each pin.
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 3:32 ` Shawn Guo
@ 2012-01-18 19:00 ` Stephen Warren
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-18 19:00 UTC (permalink / raw)
To: Shawn Guo
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Shawn Guo wrote at Tuesday, January 17, 2012 8:32 PM:
> On Tue, Jan 17, 2012 at 10:47:14AM -0800, Stephen Warren wrote:
> > Shawn Guo wrote at Saturday, January 14, 2012 12:09 AM:
> > > On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
...
> > > > /*
> > > > * The actual definition of the complete state of the
> > > > * pinmux as required by some driver.
> > > > *
> > > > * These can be either directly in the device node, or
> > > > * somewhere in tegra20.dtsi in order to provide pre-
> > > > * selected/common configurations. Hence, they're referred
> > > > * to by phandle above.
> > > > */
> > > > pmx_sdhci_active: {
> > > > /*
> > > > * Pin mux settings. Mandatory?
> > > > * Not mandatory if the 1:1 mentioned above is
> > > > * extended to 1:n.
> > > > *
> > > > * Format is <&pmx_controller_phandle muxable_entity_id
> > > > * selected_function>.
> > > > *
> > > > * The pmx phandle is required since there may be more
> > > > * than one pinmux controller in the system. Even if
> > > > * this node is inside the pinmux controller itself, I
> > > > * think it's simpler to just always have this field
> > > > * present in the binding for consistency.
> > > > *
> > >
> > > I prefer to just put such nodes inside pinctrl controller itself and
> > > drop those phandles.
> >
> > My rationale here:
> >
> > Forcing those nodes to be inside the controller node forces us to store
> > any board-specific custom configurations or overrides in the controller
> > node too; I'd simply prefer the flexibility to put them anywhere.
>
> Hmm, this type of flexibility does not make too much point to me. On
> the contrary, it's good to have a centralized place for these nodes,
> so that they can be well organized and people can find them easily.
I still would really rather have the flexibility to just put the nodes
anywhere.
However, I don't think we lose any functionality by forcing the nodes
to be inside the pin controller's node. And as you say, we do get to
remove the phandle from the mux and config properties then too, which
very marginally simplifies parsing.
One other concern I had was a device that needed to configure the pin
mux on two different pin controllers (think the main SoC and some complex
chip attached to one of the SoC's external busses). Ideally, there'd be
a Linux device for each end of the bus, and each would configure the
respective chip's pinmux. However, I can certainly foresee cases where
there's only a Linux device for the main SoC, and it'd need to configure
both chips' pinmux. Still, I think we can deal with that easily enough,
since that device's pinctrl property can simply use the 1:n feature, and
have multiple entries pointing at both chip's pinctrl device:
foo@c8000200 {
...
pinctrl = <&pinmux_soc_xxx> <&pinmux_otherchip_xxx>
<&pinmux_soc_xxx_suspend> <&pinmux_otherchip_xxx_suspend>;
pinctrl-entries = <2 2>;
pinctrl-names = "active", "suspend";
};
So, I can live with requiring the pin configuration nodes to be underneath
the relevant pin controller's node, and removing the phandle from the
mux and config properties in the pin configuration nodes.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 3:44 ` Dong Aisheng-B29396
2012-01-18 4:47 ` Shawn Guo
@ 2012-01-18 19:24 ` Stephen Warren
1 sibling, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-18 19:24 UTC (permalink / raw)
To: Dong Aisheng-B29396, Shawn Guo
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Dong Aisheng wrote at Tuesday, January 17, 2012 8:45 PM:
....
> > > > It may worth introducing
> > > > another level phandle reference. Something like the following:
> > > >
> > > > pinmux_sdhci: pinmux-sdhci {
> > > > mux =
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > > > };
> > > >
> > > > pinconf_sdhci_active: pinconf-sdhci-active {
> > > > config =
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > > };
> > > >
> > > > pinconf_sdhci_suspend: pinconf-sdhci-suspend {
> > > > config =
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > > };
> > >
> > > The config makes sense to me.
> > > The only question is how to get group name to match with the predefined groups.
> >
> > I still incline to say there should not be any predefined groups for DT case, if
> > we choose to find the group in use only when pinmux_get() gets called.
>
> Then we have too issue I said before:
> 1) Inconsistent issue
> sysfs pinmux map entry has different meaning
Ah.
When you talked about the pinmux map table being inconsistent between
the board and device tree case, I thought your main/only concern was
the enumeration timing issue; that with a board file, the entire map
was available at boot yet with DT it might not be complete if we didn't
explicitly search the whole DT for pinmux configuration nodes.
However, I think you're also concerned that if we force the DT to use
only pure HW groups (or none at all if the HW doesn't use groups), then
that would be different from a board file which did use "virtual groups".
So I'm sure you can predict what my answer is! Don't use "virtual groups"
in the board case /either/; if the HW doesn't do groups, don't have the
pinctrl driver pretend that it does.
As a response, you'll say that this makes life harder for authors of the
pinmux mapping table, because then they have to list out every single
pin's mux state instead of picking between various pre-defined configur-
ations that would have been provided by "virtual groups". I believe you
can solve this by providing these pre-defined configurations using macros
instead of "virtual groups". Each macro would simply contain a bunch of
pinmux mapping table initialization entries, and the board file author
could simply use the relevant macros when writing their mapping table:
Some SoC header:
#define PMX_MAP_SDIO4_ATB_GMA_GME \
PINMUX_MAP("default", "pinctrl-foo", "atb", "sdio4"), \
PINMUX_MAP("default", "pinctrl-foo", "gma", "sdio4"), \
PINMUX_MAP("default", "pinctrl-foo", "gme", "sdio4")
Board file:
static struct pinmux_map pmx_map[] = {
/* Using a pre-defined configuration */
PMX_MAP_SDIO4_ATB_GMA_GME,
/* Custom stuff if needed */
PINMUX_MAP("default", "pinctrl-foo", "xyz", "pqrs")
};
(This kind of thing is exactly why I pushed to allow the pinmux mapping
table to contain multiple entries for the same "name"+device combination)
> 2) need to change the pinmux_maps from arrays to list for better to dynamically
> Insert the new created pinmux map detected.
> But this does affect the non-dt case a lot.
I believe the pinctrl subsystem already supports multiple map tables to
be registered. I do recall some discussion that there was work left to
do here. Either way, this is purely an implementation detail that's
almost entirely hidden with the pinctrl subsystem, and I don't think
influences the device tree binding design at all.
...
> > so you should know how the name comes from device tree, and you
> > should be able to recreate the name when you are trying to matching the name in
> > the table you created before.
> >
> > > Besides per pin group configuration support, we may also want per pin
> > > configuration Support as the latest patch sent by Linus.
> > > http://www.spinics.net/lists/arm-kernel/msg155712.html
> >
> > This binding proposal has covered both pin and group configuration, as the
> > second parameter of 'config' property could be either a pin or group which
> > depends the on whether the configurable entity at HW level is a pin or group.
>
> The pinctrl driver does not know if it's group or pin.
> We may find a way to identify it.
The common binding simply states that the entities identified in the
mux and config properties are "muxable entities". It's up to the
individual pin controller's binding to define what a "muxable entity"
is. For HW that muxes pins, this should be a pin. For HW that muxes
groups, this should be a group.
Now, the pinctrl subsystem only knows about muxing whole groups. If
the HW muxes per pin rather than per group, its pinctrl driver must
currently define a group for each pin, containing just that pin, and
use those groups in the mapping table. I'd originally hoped that the
mapping table entries could refer to either pins or groups, but that's
not what is implemented now. So, we could change that in pinctrl, or
just create these single-pin groups within the relevant pinctrl drivers.
That's certainly what I did for Tegra30 (which muxes per pin unlike
Tegra20 which muxes per group).
When parsing the device tree, something is going to need to convert the
integer values in device tree to the names that the pinctrl core uses.
This should be a function in the individual pinctrl driver. Hence, it
can easily handle the two difference cases I mention above.
(Re: the last paragraph, I notice you raised some issues re: the order
of pinctrl driver probe v.s. when the pinmux map can be created. I'll
address those issues elsewhere where you raise them)
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 7:24 ` Dong Aisheng-B29396
@ 2012-01-18 19:42 ` Stephen Warren
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-18 19:42 UTC (permalink / raw)
To: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, Shawn Guo (shawn.guo@linaro.org)
Cc: linux-kernel, linux-arm-kernel, devicetree-discuss
Dong Aisheng wrote at Wednesday, January 18, 2012 12:24 AM:
> Stephen Warren wrote at Wednesday, January 18, 2012 3:09 AM:
> > Dong Aisheng wrote at Monday, January 16, 2012 5:50 AM:
> > > Stephen Warren wrote at Saturday, January 14, 2012 4:40 AM:
> > > > I thought a bit more about pinmux DT bindings. I came up with
> > > > something that I like well enough, and is pretty similar to the binding that
> > > > Dong posted recently.
...
> > Just to be clear, I'll repeat part of my previous response to Shawn:
> >
> > * Just a further explanation on my original code: I always intended that
> > * each entry in that list was a full pinmux configuration that could include
> > * mux and pin configuration settings. Thus, the above is more like:
> > *
> > * pinctrl =
> > * <"default" &pmx_sdhci_a>
> > * <"default" &pmx_sdhci_overrides>
> > *
> > * (overrides rather than explicit separate mux/config; the separate mux
> > * And config were just an example use case).
> >
> > So that was a list where the /examples/ had two entries, one for mux and one for
> > pin config. In general, there could be 1, 2, 3, ... entries and each could
> > define whatever mux and config entries they wanted.
>
> I also read your reply to Shawn's mail.
> I guess you mean the first "default" entry may not sufficient to use, so we allow
> customer defines extra pmx_sdhci_overrides in board.dts to use?
>
> Personally I did not see big benefits for this way but introduce a bit complexity
> and make the code not clear and not easily to understand.
> Why not define it completely for one pinmux group?
I have already enumeration the reasons. The most relevant one here is
so that if you have a pre-defined pin configuration node that's 99% what
you need, you can use that, and add any custom options on top of it
without cut/pasting the whole thing first.
> I think it also does not meet the current pinctrl subsystem design.
> The group and functions are defined, customer only needs to tell what they want to
> use. For example, in non-dt case, a pinmux map table is enough to use.
Well, we can always change the pinctrl subsystem to suite everyone's needs.
But, I don't see what doesn't fit the existing pinctrl design, at least
for muxing (pin config is still in flux irrespective of device tree...)
Is your issue that with board files, you'd typically register a single
large pinmux mapping table, whereas with this proposed device tree binding,
you end up representing a lot of separate chunks of the table in different
places? When parsing those chunks, you can certainly aggregate them all
into a single table to register with the pinctrl subsystem. That said,
the pinctrl subsystem does allow you to incrementally register more and
more table entries though, so you shouldn't actually have to convert
everything into a single table.
...
> Yes, after looking Linus's patch to support pin states, I'm thinking for pin config
> case we may need such a property or flag to tell which config of state to use by
> default since we support multi states.
I don't think we need an explicit flag. We could either:
a) Say "first entry is the default". IIRC, this is how the pinmux mapping
table works.
Or:
b) The entries are named. For the system-wide pin config table that's
in Linus's latest patch, we can simply define a specific name for the
default, e.g. "default". When I get around to responding to Linus's
pin config patch, I expect to make this comment.
c) The entries are named. Drivers request explicit names for the
pin mux configuration, and can do the same for any separate pin config
requests if they aren't wrapped into a single request. Thus, it's up
to the driver to define what the default name is. I'd pick "default"
myself.
> > b) function name: The pinmux_ops for the driver of pmx_controller_phandle needs
> > a function to convert integer IDs such as TEGRA_PMX_MUX_1 to whatever function
> > IDs are used by the pinctrl subsystem.
>
> No, I guess Tegra can do this since tegra prefers to define functions and groups
> In pinctrl driver.
> But for IMX, we do not have these info before the imx-pinctrl driver gets run and
> Parse the device tree.
The same issue exists for interrupts and GPIO, where parsing a device's
reference to an IRQ/GPIO controller needs that controller to have been
probed first, and the conversion function registered.
For interrupts, this has been solved by explicitly initializing the IRQ
controller very early in boot.
For GPIOs, I'm don't think this has been solved; it works by accident
in most cases.
I think the solution for GPIO and pinmux is to rely on the device probe
deferral patches that Grant posted a while back; defer parsing of the
pinmux properties until after the controller that contains those pin
configuration nodes has been probed (or perhaps force it to be probed
when its node is first encountered?)
>
> > c) group name: This should be handled just like (b).
> >
> > Also you'll need to know:
> >
> > struct pinmux_map's .name field. This is the value in the pinctrl-names property,
> > assuming we're switching to the following syntax:
> >
> > pinctrl = <&pmx_sdhci_a_active>, <&pmx_sdhci_a_suspend>;
> > pinctrl-names = "default", "suspend";
>
> Here the pinctrl-names are representing 'state'.
> Is pinmux_map.name field used for this?
Yes.
...
> > 2) On IMX, you'd define a pin group for each individual pin, since the HW muxing
> > is at a pin not a group level.
>
> Yes, this is what I'm thinking now.
> If we decide to do that, the pin groups concepts of current pinmux subsystem
> Seems does not fit for IMX a lot since each group is only one pin.
> And the functions/groups would be hugh since IMX6Q has 196 pins and each pin may have
> 7 functions.
We should investigate modifying the pinctrl subsystem so that either:
a) The pin mux mapping table can refer to either pins or groups, on a
per controller basis, based on some flag in struct pinctrl_desc.
b) Automatically create a group for each pin where the pinctrl driver
asks it to do so, so the single-pin groups don't all have to be
explicitly enumerated by the pinctrl driver, again based on some flag
in struct pinctrl_desc.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 12:16 ` Thomas Abraham
@ 2012-01-18 19:52 ` Stephen Warren
2012-01-19 17:01 ` Tony Lindgren
0 siblings, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-18 19:52 UTC (permalink / raw)
To: Thomas Abraham
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Thomas Abraham wrote at Wednesday, January 18, 2012 5:16 AM:
> On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
> > I thought a bit more about pinmux DT bindings. I came up with something
> > that I like well enough, and is pretty similar to the binding that Dong
> > posted recently. I think it'll work for both Tegra's and IMX's needs.
> > Please take a look!
...
> The pinmux_get() function checks if there is an active user of the
> pinmux and declines requests if the pinmux has been taken. With the dt
> bindings that you have listed, can such a check be still enforced.
I believe so.
I see these bindings as simply providing the data to populate the same
pinmux mapping table that's currently used by the pinctrl subsystem.
therefore, there are no changes to the operation of the pinctrl subsystem
(beyond a little extra code to parse the map from DT instead of receiving
a static table from a board file), and no changes to the way drivers
use the pinctrl APIs. Hence, all that error-checking will still operate
as-is.
> Also, will it be possible to support runtime pinmuxing with the above
> listed dt bindings?
Yes, the pinctrl/pinctrl-names properties allow defining a set of names
states, just like the mapping table does, and a driver can get one,
release it, then get a different named state, etc.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 14:13 ` Tony Lindgren
2012-01-18 14:30 ` Shawn Guo
@ 2012-01-18 20:02 ` Stephen Warren
2012-01-19 10:57 ` Tony Lindgren
1 sibling, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-18 20:02 UTC (permalink / raw)
To: Tony Lindgren, Grant Likely
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Tony Lindgren wrote at Wednesday, January 18, 2012 7:13 AM:
> * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> > On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > > pinmux =
> > > <"default" &pmx_sdhci_active>
> > > <"suspend" &pmx_sdhci_suspend>;
> > >
> > > /* 1:n example: */
> > > pinmux =
> > > <"default" &pmx_sdhci_mux_a>
> > > <"default" &pmx_sdhci_pincfg_a>
> > > <"suspend" &pmx_sdhci_mux_a>
> > > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> >
> >
> > Yeah, don't do this. Mixing phandle, string and cell values in a
> > property gets messy and could become troublesome to parse. I've
> > backed away from it in the clk binding.
>
> Yup, that's because the string is embedded directly into the mixed
> mode array and will likely make the following data unaligned. That
> means it's extremely flakey to parse, and will lead into horrible
> errors if you have typos in the .dts file.. Tried that and gave up
> on it.
>
> I think I've found a way to avoid using names at all, assuming we set
> each pin as a phandle for the drivers to use :)
I'd prefer not to do that for my platforms, for the reason Shawn points
out in his reply to yours.
However, I believe the bindings I proposed are flexible enough to allow
you to do exactly this for your platforms without requiring that everyone
do it.
Recall my proposal was:
pmx_sdhci_standby: pinctrl@0 {
/* Format is <&pmx_controller_phandle muxable_entity_id
* selected_function>.
*/
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
config =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
};
(Note that I think we've agreed to remove the first cell above, &tegra_pmx,
now by requiring such nodes exist as children of the pin controller.)
My assertion is that the common pinmux bindings define that the
Interpretation of muxable_entity_id is left up to the binding of the
specific pin controller. Hence, I can says "it's an integer, and here
is the list of valid values and what they mean", and you can say "it's
a phandle, which must refer to one of the per-pin nodes defined by the
pin controller".
Does that work for you?
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 15:32 ` Tony Lindgren
2012-01-18 16:29 ` Tony Lindgren
@ 2012-01-18 20:20 ` Grant Likely
2012-01-19 10:31 ` Tony Lindgren
1 sibling, 1 reply; 54+ messages in thread
From: Grant Likely @ 2012-01-18 20:20 UTC (permalink / raw)
To: Tony Lindgren
Cc: Shawn Guo, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
On Wed, Jan 18, 2012 at 07:32:56AM -0800, Tony Lindgren wrote:
> * Shawn Guo <shawn.guo@linaro.org> [120118 05:57]:
> > On 18 January 2012 22:13, Tony Lindgren <tony@atomide.com> wrote:
> > > Hi,
> > >
> > > * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> > >> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > >> > pinmux =
> > >> > <"default" &pmx_sdhci_active>
> > >> > <"suspend" &pmx_sdhci_suspend>;
> > >> >
> > >> > /* 1:n example: */
> > >> > pinmux =
> > >> > <"default" &pmx_sdhci_mux_a>
> > >> > <"default" &pmx_sdhci_pincfg_a>
> > >> > <"suspend" &pmx_sdhci_mux_a>
> > >> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> > >>
> > >>
> > >> Yeah, don't do this. Mixing phandle, string and cell values in a
> > >> property gets messy and could become troublesome to parse. I've
> > >> backed away from it in the clk binding.
> > >
> > > Yup, that's because the string is embedded directly into the mixed
> > > mode array and will likely make the following data unaligned. That
> > > means it's extremely flakey to parse, and will lead into horrible
> > > errors if you have typos in the .dts file.. Tried that and gave up
> > > on it.
> > >
> > > I think I've found a way to avoid using names at all, assuming we set
> > > each pin as a phandle for the drivers to use :)
> > >
> > The problem with doing that is we will have to represent each pin as a
> > node in device tree. For imx6q case, we have 197 pins. Doing so will
> > bloat the device tree.
>
> Sure there's some overhead. I've got it working with 220 pins, it's
> not too bad as threre's not much string parsing involved.
>
> I don't have all the devices mapping the pins though. The .dtb for
> omap4 is about 25k now.
>
> If we wanted to avoid adding phandles for each pin, then we could do:
>
> serial@0x48020000 {
> compatible = "ti,8250";
> reg = <0x48020000 0x100>;
> reg-shift = <2>;
> interrupts = <106>;
>
> /* controller, offset, value */
> pins = <&mux1 0xabcd 0x10
> &mux1 0xabcf 0x0>;
> };
>
> But then the .dts file becomes an unreadable matrix unless we have
> a preprocessor..
One node per pin does get excessive in a hurry. I prefer the one node
per pin controller. Tools can be written to make writing the
definition easier.
g.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 16:29 ` Tony Lindgren
@ 2012-01-18 20:22 ` Grant Likely
0 siblings, 0 replies; 54+ messages in thread
From: Grant Likely @ 2012-01-18 20:22 UTC (permalink / raw)
To: Tony Lindgren
Cc: Shawn Guo, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
On Wed, Jan 18, 2012 at 08:29:43AM -0800, Tony Lindgren wrote:
> * Tony Lindgren <tony@atomide.com> [120118 07:00]:
> > * Shawn Guo <shawn.guo@linaro.org> [120118 05:57]:
> > > On 18 January 2012 22:13, Tony Lindgren <tony@atomide.com> wrote:
> > > > Hi,
> > > >
> > > > * Grant Likely <grant.likely@secretlab.ca> [120116 09:55]:
> > > >> On Fri, Jan 13, 2012 at 12:39:42PM -0800, Stephen Warren wrote:
> > > >> > pinmux =
> > > >> > <"default" &pmx_sdhci_active>
> > > >> > <"suspend" &pmx_sdhci_suspend>;
> > > >> >
> > > >> > /* 1:n example: */
> > > >> > pinmux =
> > > >> > <"default" &pmx_sdhci_mux_a>
> > > >> > <"default" &pmx_sdhci_pincfg_a>
> > > >> > <"suspend" &pmx_sdhci_mux_a>
> > > >> > <"suspend" &pmx_sdhci_pincfg_a_suspend>;
> > > >>
> > > >>
> > > >> Yeah, don't do this. Mixing phandle, string and cell values in a
> > > >> property gets messy and could become troublesome to parse. I've
> > > >> backed away from it in the clk binding.
> > > >
> > > > Yup, that's because the string is embedded directly into the mixed
> > > > mode array and will likely make the following data unaligned. That
> > > > means it's extremely flakey to parse, and will lead into horrible
> > > > errors if you have typos in the .dts file.. Tried that and gave up
> > > > on it.
> > > >
> > > > I think I've found a way to avoid using names at all, assuming we set
> > > > each pin as a phandle for the drivers to use :)
> > > >
> > > The problem with doing that is we will have to represent each pin as a
> > > node in device tree. For imx6q case, we have 197 pins. Doing so will
> > > bloat the device tree.
> >
> > Sure there's some overhead. I've got it working with 220 pins, it's
> > not too bad as threre's not much string parsing involved.
> >
> > I don't have all the devices mapping the pins though. The .dtb for
> > omap4 is about 25k now.
> >
> > If we wanted to avoid adding phandles for each pin, then we could do:
> >
> > serial@0x48020000 {
> > compatible = "ti,8250";
> > reg = <0x48020000 0x100>;
> > reg-shift = <2>;
> > interrupts = <106>;
> >
> > /* controller, offset, value */
> > pins = <&mux1 0xabcd 0x10
> > &mux1 0xabcf 0x0>;
> > };
> >
> > But then the .dts file becomes an unreadable matrix unless we have
> > a preprocessor..
>
> Forgot to mention that as long as we all standardize to use something
> common for #pin-args and of_parse_phandle_with_args(), the pin mapping
> could depend on the pinmux driver for selecting whether or not to use
> a phandle for each pin.
Yes, the actual layout parsing should remain in the control of the
pinmux controller like it does for irqs, gpios, etc.
g.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 20:20 ` Grant Likely
@ 2012-01-19 10:31 ` Tony Lindgren
0 siblings, 0 replies; 54+ messages in thread
From: Tony Lindgren @ 2012-01-19 10:31 UTC (permalink / raw)
To: Grant Likely
Cc: Shawn Guo, linus.walleij, devicetree-discuss, s.hauer,
linux-kernel, rob.herring, kernel, cjb, linux-arm-kernel,
Dong Aisheng
* Grant Likely <grant.likely@secretlab.ca> [120118 11:48]:
> On Wed, Jan 18, 2012 at 07:32:56AM -0800, Tony Lindgren wrote:
> >
> > But then the .dts file becomes an unreadable matrix unless we have
> > a preprocessor..
>
> One node per pin does get excessive in a hurry. I prefer the one node
> per pin controller. Tools can be written to make writing the
> definition easier.
OK that's fine with me. It would be nice to have the .dts readable
too, so I'd prefer a preprocessor over a .dts generation tool to
keep the .dts files readable. Bugs in .dts file can be a big pain
to debug it seems.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 20:02 ` Stephen Warren
@ 2012-01-19 10:57 ` Tony Lindgren
2012-01-20 20:50 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-19 10:57 UTC (permalink / raw)
To: Stephen Warren
Cc: Grant Likely, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Stephen Warren <swarren@nvidia.com> [120118 11:29]:
> Tony Lindgren wrote at Wednesday, January 18, 2012 7:13 AM:
>
> I'd prefer not to do that for my platforms, for the reason Shawn points
> out in his reply to yours.
>
> However, I believe the bindings I proposed are flexible enough to allow
> you to do exactly this for your platforms without requiring that everyone
> do it.
Well I can easily use one phandle per pinmux controller instance
instead of one phandle per pin, so let's plan on doing that.
> Recall my proposal was:
Yes I think that's pretty close to what I'm using, just few
minor comments below.
> pmx_sdhci_standby: pinctrl@0 {
> /* Format is <&pmx_controller_phandle muxable_entity_id
> * selected_function>.
> */
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
Assuming this is describing the pins a driver is using, how
about calling it pins?
That's because you might want to do all the muxing in a
bootloader, but still need to tell how many pins you're using
for MMC on a device. So it actually has a wider meaning than just
mux.
Also, we need to standardize on some name to use for parsing pins
using of_parse_phandle_with_args, and I suggested #pin-args.
> config =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> };
Here I don't quite understand how config is different from pins/mux
above? It seems to set the driver/pull stuff, but why don't you
just make #pin-args larger and have a wider pin array?
Something like:
pins =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1
&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1>;
and in the parent set #pin-args to 3.
> (Note that I think we've agreed to remove the first cell above, &tegra_pmx,
> now by requiring such nodes exist as children of the pin controller.)
Sorry I don't quite follow, can you please maybe repost a complete
.dts entry for your pin controller and one driver entry?
> My assertion is that the common pinmux bindings define that the
> Interpretation of muxable_entity_id is left up to the binding of the
> specific pin controller. Hence, I can says "it's an integer, and here
> is the list of valid values and what they mean", and you can say "it's
> a phandle, which must refer to one of the per-pin nodes defined by the
> pin controller".
>
> Does that work for you?
Yes it does, other than the comments above.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
` (3 preceding siblings ...)
2012-01-18 12:16 ` Thomas Abraham
@ 2012-01-19 13:10 ` Thomas Abraham
2012-01-19 16:56 ` Tony Lindgren
2012-01-20 21:11 ` Stephen Warren
4 siblings, 2 replies; 54+ messages in thread
From: Thomas Abraham @ 2012-01-19 13:10 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
> I thought a bit more about pinmux DT bindings. I came up with something
> that I like well enough, and is pretty similar to the binding that Dong
> posted recently. I think it'll work for both Tegra's and IMX's needs.
> Please take a look!
>
> Note: I've used named constants below just to make this easier to read.
> We still don't have a solution to actually use named constants in dtc yet.
>
[...]
For Samsung io-pad controllers, I had been considering a dt approach
which has been described below. Kindly review and any comments will be
helpful.
* Main points to be considered:
All Samsung SoC's use a io-pad controller that includes gpio, pinmux
and pinconfig functionality in a single controller, i.e. there is a
single intermixed address space for gpio/pinmux/pinconfig portions in
the controller. Additionally, all the drivers for the Samsung SoC's
setup pinmux/pinconfig in its probe function (and in resume if
required).
* IO Pad controllers in dts file:
The io-pad controller (gpio/pinmux/pinconfig) can be represented in a
dtsi file as below. There could be multiple io-pad controllers
supported in the system.
pctrl0: pinctrl@11400000 {
compatible = "samsung,exynos4210-pinctrl";
reg = <0x11400000 0x1000>;
interrupts = <.......>;
pin-banks = <....>;
[... other properties TBD ...]
#pinctrl-cells = <5>;
};
Each such node instantiates one instance of the pin-controller device.
The 'struct pinctrl_dev' should include a new member 'struct of_node'
to point to the corresponding pin-controller node in dt which
instantiated the controller. The pinctrl_register() function called
from drivers/pinctrl/pinctrl-xyz.c then registers the pin-controller
instance.
* Specifying the pinmux/pinconfig settings in dts files:
Device nodes which require specific pinmux/pinconfig settings should
include information about the required settings. For example, a i2c
controller node in dts file is listed below.
i2c0: i2c@18000000 {
[... other properties ...]
pinctrl-active = <&pctrl0 5 0 2 3 0>,
<&pctrl0 5 1 2 3 0>;
pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
<&pctrl0 5 1 2 0 0>;
};
In the example above, the specifier of pinctrl-* is specific for
Samsung io-pad controllers. Its format/syntax would be mainly
dependent on the io-pad controller used, the above is only an example
for Samsung io-pad controller. In the above node, the format of the
pinctrl-* specifier is
property-name = <phandle of the pin-controller
pin bank within the pin-controller
pin number within the pin-bank
pin-mux function number
pull up/down config
drive strength config>;
* Using the pinmux/pinconfig specifier in device nodes to configure hardware.
A driver (for a device instantiated from device tree) would require
code to be made aware of the pinmux/pinconfig options available. The
typical sequence in a probe function would be as below.
(a) call of_get_named_gpio() to get the gpio number. In case of
Samsung pinctrl driver, the pinctrl driver itself provides the gpiolib
functionality and it attaches a gpio specifier translator with the
gpio_chip. This translator is capable of translating the pinctrl-* and
returning a gpio number.
(b) gpio_request() the gpio number obtained in step (a) above.
(c) Repeat steps (a) and (b) until all the gpios required by the
driver are requested. In case a request fails, give up all the
successfully requested gpio and return failure.
(d) For all entries in pinctrl-* property, use
of_parse_phandles_with_args() and get the pinctrl node pointer and the
pinctrl specifier. As an example, the i2c driver would do the
following, incrementing pin-index parameter for each call.
ret = of_parse_phandle_with_args(i2c_np, "pinctrl-active",
"#pinctrl-cells", pin-index, &pctrl_np, &pctrl_specifier);
(e) There should be a new api in pinctrl subsystem whose signature
could be something like
int pinctrl_dt_parse_and_set_mux_cfg(struct device_node *, const void *);
For each iteration of step (d) in the driver, this new api will be
called. The value of pctrl_np and pctrl_specifier obtained from step
(d) is passed as a parameters to this new api. This api will do the
following
(e1) Find the pin-controller (struct pinctrl_dev) that has a
device_node pointer which is same as the first parameter. To recap, it
was mentioned above that: "The 'struct pinctrl_dev' should include a
new member 'struct of_node' to point to the corresponding
pin-controller node in dt which instantiated the controller."
(e2) A new callback 'xlate_pinctrl' is required in 'struct
pinctrl_ops' which can translate the second parameter of
'pinctrl_dt_parse_and_set_mux_cfg' function. From the pin controller
found in step (e1), call pinctrl_dev->desc->pctlops->xlate_pinctrl
passing the second parameter of 'pinctrl_dt_parse_and_set_mux_cfg'
function. The pin-controller specific translator function will
translate the parameter and program the hardware registers. The
xlate_pinctrl is specific to each pin-controller is it knows how to
decode the specifier and program the registers.
The above method may have some elements that are specific to Samsung
io-pad controller. But the main requirement for the above procedure
from the pinctrl subsystem is the new API
'pinctrl_dt_parse_and_set_mux_cfg' and related additions to 'struct
pinctrl_dev' and ''struct pinctrl_ops'. These additions are generic
and any io-pad controller could use it.
Thanks for any feedback.
Regards,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-19 13:10 ` Thomas Abraham
@ 2012-01-19 16:56 ` Tony Lindgren
2012-01-19 17:38 ` Thomas Abraham
2012-01-20 21:15 ` Stephen Warren
2012-01-20 21:11 ` Stephen Warren
1 sibling, 2 replies; 54+ messages in thread
From: Tony Lindgren @ 2012-01-19 16:56 UTC (permalink / raw)
To: Thomas Abraham
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Thomas Abraham <thomas.abraham@linaro.org> [120119 04:37]:
>
> The io-pad controller (gpio/pinmux/pinconfig) can be represented in a
> dtsi file as below. There could be multiple io-pad controllers
> supported in the system.
FYI we too have too mux controller instances on omap4. That seems to
work just fine thanks to the "pinctrl: make it possible to add multiple maps"
patch by Linus W.
> pctrl0: pinctrl@11400000 {
> compatible = "samsung,exynos4210-pinctrl";
> reg = <0x11400000 0x1000>;
> interrupts = <.......>;
> pin-banks = <....>;
> [... other properties TBD ...]
> #pinctrl-cells = <5>;
> };
>
> Each such node instantiates one instance of the pin-controller device.
> The 'struct pinctrl_dev' should include a new member 'struct of_node'
> to point to the corresponding pin-controller node in dt which
> instantiated the controller. The pinctrl_register() function called
> from drivers/pinctrl/pinctrl-xyz.c then registers the pin-controller
> instance.
Looks similar to what I have too.
> * Specifying the pinmux/pinconfig settings in dts files:
>
> Device nodes which require specific pinmux/pinconfig settings should
> include information about the required settings. For example, a i2c
> controller node in dts file is listed below.
>
> i2c0: i2c@18000000 {
> [... other properties ...]
> pinctrl-active = <&pctrl0 5 0 2 3 0>,
> <&pctrl0 5 1 2 3 0>;
> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
> <&pctrl0 5 1 2 0 0>;
> };
Maybe we should have just the active/suspend/off flags in the
same array? Otherwise we'll end up unnecessarily repeating the
the pin information. See the pins + #pin-args example I posted,
which is otherwise similar.
> In the example above, the specifier of pinctrl-* is specific for
> Samsung io-pad controllers. Its format/syntax would be mainly
> dependent on the io-pad controller used, the above is only an example
> for Samsung io-pad controller. In the above node, the format of the
> pinctrl-* specifier is
>
> property-name = <phandle of the pin-controller
> pin bank within the pin-controller
> pin number within the pin-bank
> pin-mux function number
> pull up/down config
> drive strength config>;
Are these all just bits in one register? If so, let's say you
have 16-bits per pin, we might be able to share the generic
pinmux-simple.c driver I have almost ready..
The format would then be
#pin-args <4>;
...
/* controller offset on idle off */
pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
>
> A driver (for a device instantiated from device tree) would require
> code to be made aware of the pinmux/pinconfig options available. The
> typical sequence in a probe function would be as below.
>
> (a) call of_get_named_gpio() to get the gpio number. In case of
> Samsung pinctrl driver, the pinctrl driver itself provides the gpiolib
> functionality and it attaches a gpio specifier translator with the
> gpio_chip. This translator is capable of translating the pinctrl-* and
> returning a gpio number.
>
> (b) gpio_request() the gpio number obtained in step (a) above.
>
> (c) Repeat steps (a) and (b) until all the gpios required by the
> driver are requested. In case a request fails, give up all the
> successfully requested gpio and return failure.
Can't the driver do this to request the gpio pins that you
can get from DT and pinmux? Even if you need to do dynamic pinmuxing
to GPIO pins for wake-up events, you can do that from the driver
as long as you get the GPIO number for the pin from mux code.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-18 19:52 ` Stephen Warren
@ 2012-01-19 17:01 ` Tony Lindgren
0 siblings, 0 replies; 54+ messages in thread
From: Tony Lindgren @ 2012-01-19 17:01 UTC (permalink / raw)
To: Stephen Warren
Cc: Thomas Abraham, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Stephen Warren <swarren@nvidia.com> [120118 11:21]:
> Thomas Abraham wrote at Wednesday, January 18, 2012 5:16 AM:
> > On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
> > > I thought a bit more about pinmux DT bindings. I came up with something
> > > that I like well enough, and is pretty similar to the binding that Dong
> > > posted recently. I think it'll work for both Tegra's and IMX's needs.
> > > Please take a look!
> ...
> > The pinmux_get() function checks if there is an active user of the
> > pinmux and declines requests if the pinmux has been taken. With the dt
> > bindings that you have listed, can such a check be still enforced.
>
> I believe so.
>
> I see these bindings as simply providing the data to populate the same
> pinmux mapping table that's currently used by the pinctrl subsystem.
> therefore, there are no changes to the operation of the pinctrl subsystem
> (beyond a little extra code to parse the map from DT instead of receiving
> a static table from a board file), and no changes to the way drivers
> use the pinctrl APIs. Hence, all that error-checking will still operate
> as-is.
FYI I have the pinmux maps dynamically created based on the driver pins
requested in .dts file and so far don't see a need for static stables
except to support legacy drivers. I'll update my pinmux-simple.c to
use one phandle per controller instance and post it when I'm back home,
so hopefully next week at some point. Sorry it's been dragging, but it's
been looking "just few more things" to fix for a while now..
Cheers,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-19 16:56 ` Tony Lindgren
@ 2012-01-19 17:38 ` Thomas Abraham
2012-01-19 18:20 ` Tony Lindgren
2012-01-20 21:15 ` Stephen Warren
1 sibling, 1 reply; 54+ messages in thread
From: Thomas Abraham @ 2012-01-19 17:38 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Hi Tony,
On 19 January 2012 22:26, Tony Lindgren <tony@atomide.com> wrote:
> * Thomas Abraham <thomas.abraham@linaro.org> [120119 04:37]:
>>
>> The io-pad controller (gpio/pinmux/pinconfig) can be represented in a
>> dtsi file as below. There could be multiple io-pad controllers
>> supported in the system.
>
> FYI we too have too mux controller instances on omap4. That seems to
> work just fine thanks to the "pinctrl: make it possible to add multiple maps"
> patch by Linus W.
>
>> pctrl0: pinctrl@11400000 {
>> compatible = "samsung,exynos4210-pinctrl";
>> reg = <0x11400000 0x1000>;
>> interrupts = <.......>;
>> pin-banks = <....>;
>> [... other properties TBD ...]
>> #pinctrl-cells = <5>;
>> };
>>
>> Each such node instantiates one instance of the pin-controller device.
>> The 'struct pinctrl_dev' should include a new member 'struct of_node'
>> to point to the corresponding pin-controller node in dt which
>> instantiated the controller. The pinctrl_register() function called
>> from drivers/pinctrl/pinctrl-xyz.c then registers the pin-controller
>> instance.
>
> Looks similar to what I have too.
>
>> * Specifying the pinmux/pinconfig settings in dts files:
>>
>> Device nodes which require specific pinmux/pinconfig settings should
>> include information about the required settings. For example, a i2c
>> controller node in dts file is listed below.
>>
>> i2c0: i2c@18000000 {
>> [... other properties ...]
>> pinctrl-active = <&pctrl0 5 0 2 3 0>,
>> <&pctrl0 5 1 2 3 0>;
>> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
>> <&pctrl0 5 1 2 0 0>;
>> };
>
> Maybe we should have just the active/suspend/off flags in the
> same array? Otherwise we'll end up unnecessarily repeating the
> the pin information. See the pins + #pin-args example I posted,
> which is otherwise similar.
Yes, I agree. That would be better.
>
>> In the example above, the specifier of pinctrl-* is specific for
>> Samsung io-pad controllers. Its format/syntax would be mainly
>> dependent on the io-pad controller used, the above is only an example
>> for Samsung io-pad controller. In the above node, the format of the
>> pinctrl-* specifier is
>>
>> property-name = <phandle of the pin-controller
>> pin bank within the pin-controller
>> pin number within the pin-bank
>> pin-mux function number
>> pull up/down config
>> drive strength config>;
>
> Are these all just bits in one register? If so, let's say you
> have 16-bits per pin, we might be able to share the generic
> pinmux-simple.c driver I have almost ready..
no, these get written to different registers, one for pinmux, one for
pull up/down and one for drive strength. It would be helpful of your
pinmux-simple.c can support writes to multiple registers also.
>
> The format would then be
>
> #pin-args <4>;
> ...
>
> /* controller offset on idle off */
> pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
>
>> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
>>
>> A driver (for a device instantiated from device tree) would require
>> code to be made aware of the pinmux/pinconfig options available. The
>> typical sequence in a probe function would be as below.
>>
>> (a) call of_get_named_gpio() to get the gpio number. In case of
>> Samsung pinctrl driver, the pinctrl driver itself provides the gpiolib
>> functionality and it attaches a gpio specifier translator with the
>> gpio_chip. This translator is capable of translating the pinctrl-* and
>> returning a gpio number.
>>
>> (b) gpio_request() the gpio number obtained in step (a) above.
>>
>> (c) Repeat steps (a) and (b) until all the gpios required by the
>> driver are requested. In case a request fails, give up all the
>> successfully requested gpio and return failure.
>
> Can't the driver do this to request the gpio pins that you
> can get from DT and pinmux? Even if you need to do dynamic pinmuxing
> to GPIO pins for wake-up events, you can do that from the driver
> as long as you get the GPIO number for the pin from mux code.
Yes, it is the driver that finds out the gpio number from dt and
requests it. Maybe, I did not understand your point here.
With the approach that I mentioned in the previous email, there are no
pinmux, pin groups or pin desc tables built from dt. Each device node
specifies what needs to configured (pinmux,pinconfig) and the code
using that device node uses pinctrl driver to get those
pinmux/pinconfig values written to the hardware registers.
Does the dt support for pinmux/pinconfig that you are developing build
the pinmux, pin groups or pin desc tables? Do drivers have to call
pinmux_{get/put}() or pin_config_set() functions even in dt case?
Thanks,
Thomas.
>
> Regards,
>
> Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-19 17:38 ` Thomas Abraham
@ 2012-01-19 18:20 ` Tony Lindgren
2012-01-19 18:38 ` Thomas Abraham
0 siblings, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-19 18:20 UTC (permalink / raw)
To: Thomas Abraham
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Thomas Abraham <thomas.abraham@linaro.org> [120119 09:05]:
>
> On 19 January 2012 22:26, Tony Lindgren <tony@atomide.com> wrote:
> > * Thomas Abraham <thomas.abraham@linaro.org> [120119 04:37]:
> >>
> >> i2c0: i2c@18000000 {
> >> [... other properties ...]
> >> pinctrl-active = <&pctrl0 5 0 2 3 0>,
> >> <&pctrl0 5 1 2 3 0>;
> >> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
> >> <&pctrl0 5 1 2 0 0>;
> >> };
> >
> > Maybe we should have just the active/suspend/off flags in the
> > same array? Otherwise we'll end up unnecessarily repeating the
> > the pin information. See the pins + #pin-args example I posted,
> > which is otherwise similar.
>
> Yes, I agree. That would be better.
OK great.
> >
> >> In the example above, the specifier of pinctrl-* is specific for
> >> Samsung io-pad controllers. Its format/syntax would be mainly
> >> dependent on the io-pad controller used, the above is only an example
> >> for Samsung io-pad controller. In the above node, the format of the
> >> pinctrl-* specifier is
> >>
> >> property-name = <phandle of the pin-controller
> >> pin bank within the pin-controller
> >> pin number within the pin-bank
> >> pin-mux function number
> >> pull up/down config
> >> drive strength config>;
> >
> > Are these all just bits in one register? If so, let's say you
> > have 16-bits per pin, we might be able to share the generic
> > pinmux-simple.c driver I have almost ready..
>
> no, these get written to different registers, one for pinmux, one for
> pull up/down and one for drive strength. It would be helpful of your
> pinmux-simple.c can support writes to multiple registers also.
Hmm yeah that should be doable, and actually we have the old legacy
omap1510 set up with three different registers per pin..
The question is: Do we want to add the on/idle/off modes for each
pin in .dts, or do we want to have the driver set the dynamic muxing
flags separately?
> > The format would then be
> >
> > #pin-args <4>;
> > ...
> >
> > /* controller offset on idle off */
> > pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
> >
> >> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
> >>
> >> A driver (for a device instantiated from device tree) would require
> >> code to be made aware of the pinmux/pinconfig options available. The
> >> typical sequence in a probe function would be as below.
> >>
> >> (a) call of_get_named_gpio() to get the gpio number. In case of
> >> Samsung pinctrl driver, the pinctrl driver itself provides the gpiolib
> >> functionality and it attaches a gpio specifier translator with the
> >> gpio_chip. This translator is capable of translating the pinctrl-* and
> >> returning a gpio number.
> >>
> >> (b) gpio_request() the gpio number obtained in step (a) above.
> >>
> >> (c) Repeat steps (a) and (b) until all the gpios required by the
> >> driver are requested. In case a request fails, give up all the
> >> successfully requested gpio and return failure.
> >
> > Can't the driver do this to request the gpio pins that you
> > can get from DT and pinmux? Even if you need to do dynamic pinmuxing
> > to GPIO pins for wake-up events, you can do that from the driver
> > as long as you get the GPIO number for the pin from mux code.
>
> Yes, it is the driver that finds out the gpio number from dt and
> requests it. Maybe, I did not understand your point here.
Oh ok, sorry I misunderstood thinking you want to do all that in the
pinmux driver..
> With the approach that I mentioned in the previous email, there are no
> pinmux, pin groups or pin desc tables built from dt. Each device node
> specifies what needs to configured (pinmux,pinconfig) and the code
> using that device node uses pinctrl driver to get those
> pinmux/pinconfig values written to the hardware registers.
>
> Does the dt support for pinmux/pinconfig that you are developing build
> the pinmux, pin groups or pin desc tables? Do drivers have to call
> pinmux_{get/put}() or pin_config_set() functions even in dt case?
Yeah the pinmux/pingroups and functions are built from the DT. Drivers
would have to still call pinmux_get and set right now. But some of this
could probably be automatically done using runtime PM?
Cheers,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-19 18:20 ` Tony Lindgren
@ 2012-01-19 18:38 ` Thomas Abraham
2012-01-20 10:05 ` Tony Lindgren
0 siblings, 1 reply; 54+ messages in thread
From: Thomas Abraham @ 2012-01-19 18:38 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On 19 January 2012 23:50, Tony Lindgren <tony@atomide.com> wrote:
> * Thomas Abraham <thomas.abraham@linaro.org> [120119 09:05]:
>>
>> On 19 January 2012 22:26, Tony Lindgren <tony@atomide.com> wrote:
>> > * Thomas Abraham <thomas.abraham@linaro.org> [120119 04:37]:
>> >>
>> >> i2c0: i2c@18000000 {
>> >> [... other properties ...]
>> >> pinctrl-active = <&pctrl0 5 0 2 3 0>,
>> >> <&pctrl0 5 1 2 3 0>;
>> >> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
>> >> <&pctrl0 5 1 2 0 0>;
>> >> };
>> >
>> > Maybe we should have just the active/suspend/off flags in the
>> > same array? Otherwise we'll end up unnecessarily repeating the
>> > the pin information. See the pins + #pin-args example I posted,
>> > which is otherwise similar.
>>
>> Yes, I agree. That would be better.
>
> OK great.
>
>> >
>> >> In the example above, the specifier of pinctrl-* is specific for
>> >> Samsung io-pad controllers. Its format/syntax would be mainly
>> >> dependent on the io-pad controller used, the above is only an example
>> >> for Samsung io-pad controller. In the above node, the format of the
>> >> pinctrl-* specifier is
>> >>
>> >> property-name = <phandle of the pin-controller
>> >> pin bank within the pin-controller
>> >> pin number within the pin-bank
>> >> pin-mux function number
>> >> pull up/down config
>> >> drive strength config>;
>> >
>> > Are these all just bits in one register? If so, let's say you
>> > have 16-bits per pin, we might be able to share the generic
>> > pinmux-simple.c driver I have almost ready..
>>
>> no, these get written to different registers, one for pinmux, one for
>> pull up/down and one for drive strength. It would be helpful of your
>> pinmux-simple.c can support writes to multiple registers also.
>
> Hmm yeah that should be doable, and actually we have the old legacy
> omap1510 set up with three different registers per pin..
>
> The question is: Do we want to add the on/idle/off modes for each
> pin in .dts, or do we want to have the driver set the dynamic muxing
> flags separately?
>
It would be better to include the pin muxing and config options in the
node of a device. So a i2c device node would have pin mux and pin
config options listed in its node. But this means, each board dts file
would have to repeat this information. On the other hand, describing
mux and config options for each pin in a SoC dtsi file would be too
much of information. I would go with the first option.
>> > The format would then be
>> >
>> > #pin-args <4>;
>> > ...
>> >
>> > /* controller offset on idle off */
>> > pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
>> >
>> >> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
>> >>
>> >> A driver (for a device instantiated from device tree) would require
>> >> code to be made aware of the pinmux/pinconfig options available. The
>> >> typical sequence in a probe function would be as below.
>> >>
>> >> (a) call of_get_named_gpio() to get the gpio number. In case of
>> >> Samsung pinctrl driver, the pinctrl driver itself provides the gpiolib
>> >> functionality and it attaches a gpio specifier translator with the
>> >> gpio_chip. This translator is capable of translating the pinctrl-* and
>> >> returning a gpio number.
>> >>
>> >> (b) gpio_request() the gpio number obtained in step (a) above.
>> >>
>> >> (c) Repeat steps (a) and (b) until all the gpios required by the
>> >> driver are requested. In case a request fails, give up all the
>> >> successfully requested gpio and return failure.
>> >
>> > Can't the driver do this to request the gpio pins that you
>> > can get from DT and pinmux? Even if you need to do dynamic pinmuxing
>> > to GPIO pins for wake-up events, you can do that from the driver
>> > as long as you get the GPIO number for the pin from mux code.
>>
>> Yes, it is the driver that finds out the gpio number from dt and
>> requests it. Maybe, I did not understand your point here.
>
> Oh ok, sorry I misunderstood thinking you want to do all that in the
> pinmux driver..
>
>> With the approach that I mentioned in the previous email, there are no
>> pinmux, pin groups or pin desc tables built from dt. Each device node
>> specifies what needs to configured (pinmux,pinconfig) and the code
>> using that device node uses pinctrl driver to get those
>> pinmux/pinconfig values written to the hardware registers.
>>
>> Does the dt support for pinmux/pinconfig that you are developing build
>> the pinmux, pin groups or pin desc tables? Do drivers have to call
>> pinmux_{get/put}() or pin_config_set() functions even in dt case?
>
> Yeah the pinmux/pingroups and functions are built from the DT. Drivers
> would have to still call pinmux_get and set right now. But some of this
> could probably be automatically done using runtime PM?
>
I would like to understand the need for populating the
pinmux/pingroups tables from dt. The question here is when we have
something like
pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
which specifies the values that need to be written to the hardware
registers, would populating pinmux/pingroup tables from dt required.
The SoC specific pinctrl driver can provide a way (with the help of
pinctrl core) to translate these values and write to corresponding
hardware registers. Is there any particular reason for populating the
pinmux/pingroups tables from dt?
Thanks,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-19 18:38 ` Thomas Abraham
@ 2012-01-20 10:05 ` Tony Lindgren
2012-01-20 16:17 ` Thomas Abraham
0 siblings, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-20 10:05 UTC (permalink / raw)
To: Thomas Abraham
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Thomas Abraham <thomas.abraham@linaro.org> [120119 10:05]:
> On 19 January 2012 23:50, Tony Lindgren <tony@atomide.com> wrote:
>
> I would like to understand the need for populating the
> pinmux/pingroups tables from dt. The question here is when we have
> something like
>
> pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
>
> which specifies the values that need to be written to the hardware
> registers, would populating pinmux/pingroup tables from dt required.
> The SoC specific pinctrl driver can provide a way (with the help of
> pinctrl core) to translate these values and write to corresponding
> hardware registers. Is there any particular reason for populating the
> pinmux/pingroups tables from dt?
Hmm I see. Yes it's still needed as we only want to parse the DT once
because it's slower unless it was one time only configuration during
init.
If you only need to set pins once during the init, then we could add
an option for freeing all or most pins after init. That's what we
have for the current mach-omap2 mux framework as only few pins need
to be dynamically remuxed. That will require some changes to the
pinctrl framework though. We would need flags for each pin from DT
for init_only/dynamic.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 10:05 ` Tony Lindgren
@ 2012-01-20 16:17 ` Thomas Abraham
2012-01-20 17:53 ` Tony Lindgren
0 siblings, 1 reply; 54+ messages in thread
From: Thomas Abraham @ 2012-01-20 16:17 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On 20 January 2012 15:35, Tony Lindgren <tony@atomide.com> wrote:
> * Thomas Abraham <thomas.abraham@linaro.org> [120119 10:05]:
>> On 19 January 2012 23:50, Tony Lindgren <tony@atomide.com> wrote:
>>
>> I would like to understand the need for populating the
>> pinmux/pingroups tables from dt. The question here is when we have
>> something like
>>
>> pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
>>
>> which specifies the values that need to be written to the hardware
>> registers, would populating pinmux/pingroup tables from dt required.
>> The SoC specific pinctrl driver can provide a way (with the help of
>> pinctrl core) to translate these values and write to corresponding
>> hardware registers. Is there any particular reason for populating the
>> pinmux/pingroups tables from dt?
>
> Hmm I see. Yes it's still needed as we only want to parse the DT once
> because it's slower unless it was one time only configuration during
> init.
Ok. The time spent on searching for the pin-config property can be
reduced by having the device driver (say, i2c) keep a pointer to all
the pinconfig properties in its node. The next time a driver needs to
reconfigure the pins, the search time can be reduced. The time to
parse the property values though would still be applicable. But I
would still not opt to build pinmux/pinconfig/pindesc tables from dt.
>
> If you only need to set pins once during the init, then we could add
> an option for freeing all or most pins after init. That's what we
> have for the current mach-omap2 mux framework as only few pins need
> to be dynamically remuxed. That will require some changes to the
> pinctrl framework though. We would need flags for each pin from DT
> for init_only/dynamic.
>
> Regards,
>
> Tony
Thanks,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 16:17 ` Thomas Abraham
@ 2012-01-20 17:53 ` Tony Lindgren
2012-01-21 1:38 ` Thomas Abraham
0 siblings, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-20 17:53 UTC (permalink / raw)
To: Thomas Abraham
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Thomas Abraham <thomas.abraham@linaro.org> [120120 16:45]:
> On 20 January 2012 15:35, Tony Lindgren <tony@atomide.com> wrote:
> > * Thomas Abraham <thomas.abraham@linaro.org> [120119 10:05]:
> >> On 19 January 2012 23:50, Tony Lindgren <tony@atomide.com> wrote:
> >>
> >> I would like to understand the need for populating the
> >> pinmux/pingroups tables from dt. The question here is when we have
> >> something like
> >>
> >> pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
> >>
> >> which specifies the values that need to be written to the hardware
> >> registers, would populating pinmux/pingroup tables from dt required.
> >> The SoC specific pinctrl driver can provide a way (with the help of
> >> pinctrl core) to translate these values and write to corresponding
> >> hardware registers. Is there any particular reason for populating the
> >> pinmux/pingroups tables from dt?
> >
> > Hmm I see. Yes it's still needed as we only want to parse the DT once
> > because it's slower unless it was one time only configuration during
> > init.
>
> Ok. The time spent on searching for the pin-config property can be
> reduced by having the device driver (say, i2c) keep a pointer to all
> the pinconfig properties in its node. The next time a driver needs to
> reconfigure the pins, the search time can be reduced. The time to
> parse the property values though would still be applicable. But I
> would still not opt to build pinmux/pinconfig/pindesc tables from dt.
Hmm that's something to consider to save memory as the node will stay
there. This would allow making all the pinctrl framework data __initdata
in some cases. You'd probably want to copy the data into the driver in
some pinctrl framework struct so you could still use pinctrl framework
functions with this data and not have to parse the node again.
For building the tables from dt, what I have currently is building
the tables without any specific knowledge about the pinmux functions.
I'm thinking that any further knowledge for debugging etc can be done
later on using user space tools to avoid storing the data in kernel.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-18 11:06 ` Dong Aisheng-B29396
@ 2012-01-20 20:28 ` Stephen Warren
2012-01-27 12:00 ` Linus Walleij
0 siblings, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-20 20:28 UTC (permalink / raw)
To: Dong Aisheng-B29396, Shawn Guo
Cc: linus.walleij, s.hauer, rob.herring, kernel, cjb,
Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Dong Aisheng wrote at Wednesday, January 18, 2012 4:06 AM:
> Stephen Warren wrote at Wednesday, January 18, 2012 3:29 AM:
> > Dong Aisheng wrote at Tuesday, January 17, 2012 2:47 AM:
> > > Shawn Guo wrote at Tuesday, January 17, 2012 4:24 PM:
> > > > On Mon, Jan 16, 2012 at 12:50:02PM +0000, Dong Aisheng-B29396 wrote:
> > > > > Stephen Warren wrote:
...
> Now i'm still thinking if the mux way we discussed here fits IMX.
> For example:
>
> > pinmux_sdhci: pinmux-sdhci {
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
> > };
>
> It may work well for Tegra.
> When pinctrl core parsing the mux proterty, it will creat each maps
> For each entry in the list.
> But for IMX like:
>
> > > pinmux_usdhc1: pinmux-usdhc1 {
> > > mux = <IMX6Q_PAD_SD1_DAT1 0>
> > > <IMX6Q_PAD_SD1_DAT2 0>
> > > ...
> > > };
>
> Each entry in the list is PIN rather than group.
> So we cannot create each map for each entry in the list except we treat the PIN as
> A group(this will cause a huge maps and predefined groups and functions).
Linusw has NAKd the idea that we should not use these "virtual groups",
i.e. he believes they're fine to use if you wish.
Given that, I think what you should do is:
* Your pinctrl driver should enumerate whatever kind of groups it wishes.
* Use the same sets of groups for both board-based and DT-based systems
(well, that's true irrespective of anything else)
* The mux properties above would not refer to pins, but whatever muxable
entities your pinctrl driver exposes.
For Tegra20, I'll use the raw HW pin groups.
For Tegra30, I'll use raw HW pins here, since each pin is individually
muxable. In the pinctrl driver, pin IDs and these mux group IDs will
be equivalent, since there will be 1 group per pin.
For IMX, you can use "virtual group" IDs in the DT properties.
> A little different way I'm thinking now is cover the mux difference(pin or group)
> between Tegra and IMX in Pinctrl driver, then we may not need the second mux value
> (e.g: TEGRA_PMX_MUX_1)in the standard mux property:
> The mux property can become:
>
> pinmux_sdhci: pinmux-sdhci {
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA_X>
> <&tegra_pmx TEGRA_PMX_PG_DTD_Y>;
> };
I definitely don't like putting the group ID and mux function into a
single value; this seems redundant.
However, since I'm leaning towards it being up to the individual pinctrl
driver to convert the DT property values into the group/function values
that the pinctrl core uses, you are free to define your binding however
you wish.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-19 10:57 ` Tony Lindgren
@ 2012-01-20 20:50 ` Stephen Warren
2012-01-23 20:13 ` Tony Lindgren
2012-01-27 13:11 ` Linus Walleij
0 siblings, 2 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-20 20:50 UTC (permalink / raw)
To: Tony Lindgren
Cc: Grant Likely, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Tony wrote at Lindgren:
> * Stephen Warren <swarren@nvidia.com> [120118 11:29]:
> > Tony Lindgren wrote at Wednesday, January 18, 2012 7:13 AM:
> >
> > I'd prefer not to do that for my platforms, for the reason Shawn points
> > out in his reply to yours.
> >
> > However, I believe the bindings I proposed are flexible enough to allow
> > you to do exactly this for your platforms without requiring that everyone
> > do it.
>
> Well I can easily use one phandle per pinmux controller instance
> instead of one phandle per pin, so let's plan on doing that.
>
> > Recall my proposal was:
>
> Yes I think that's pretty close to what I'm using, just few
> minor comments below.
>
> > pmx_sdhci_standby: pinctrl@0 {
> > /* Format is <&pmx_controller_phandle muxable_entity_id
> > * selected_function>.
> > */
> > mux =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
>
> Assuming this is describing the pins a driver is using, how
> about calling it pins?
It's not always pins.
For a lot of HW it is pins, you're right.
For Tegra20 (and IIRC some other HW), the pin mux HW actually muxes
groups of pins; one register field sets n (1, 2, 3, ...) pins to that
function at once. Hence, the entries are real physical groups.
For some HW that controls the mux per pin, the SoC vendors wish the
pinctrl driver to expose what I call "virtual groups" of pins, e.g. all
the pins that are typically used together as a single I2C or SD/MMC
interface, as a single muxable entity (a group in pinctrl parlance).
In this case, the values listed here will be these group IDs
> That's because you might want to do all the muxing in a
> bootloader, but still need to tell how many pins you're using
> for MMC on a device. So it actually has a wider meaning than just
> mux.
I don't think that affects the name of the property:-)
I see your point about separate ownership of pins/groups and the actual
HW register programming. The pinctrl subsystem doesn't have a concept
of separating those two things at the moment though. I don't think its
unreasonable to still have to write the mux values into the device tree
and even reprogram them to the same value when Linux boots though. Do
you see a problem with that? If there is a problem, we need to fix it
in pinctrl too, irrespective of any device tree work.
> Also, we need to standardize on some name to use for parsing pins
> using of_parse_phandle_with_args, and I suggested #pin-args.
"cells" is the suffix that's typically used rather than "args".
I certainly foresee the need for a #mux-cells and a #config-cells
Property so that pinctrl bindings/nodes can tell the core parser of
the mux/config properties how many cells to pull out for each entry.
In my binding proposal, I original assumed that every controller would
always have group,function for mux and group,param,value for config.
However, having #mux-cells and #config-cells is certainly a better idea.
> > config =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > };
>
> Here I don't quite understand how config is different from pins/mux
> above? It seems to set the driver/pull stuff, but why don't you
> just make #pin-args larger and have a wider pin array?
The idea is that "mux" lists each pin/group that the device uses, and
the pin mux selection for it, whereas "config" lists all the other
configuration values that could be applied to a pin; pull-up, pull-down,
tri-state, drive strength, ...
I suppose we could lump all those into a single property like:
pinctrl =
<MUX &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<MUX &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
<CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
<CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
<CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
<CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
<CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
(where e.g. MUX=0, CFG=1, both interpreted by the parsing code in the
core pinctrl subsystem)
However, I'd tend towards keeping them in separate properties, especially
since simple chips won't have any CFG (even on Tegra, 99% of the settings
we make are MUX not CFG) and requiring this MUX value in every mux entry
seems wasteful.
> Something like:
>
> pins =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1
> &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1>;
>
> and in the parent set #pin-args to 3.
That doesn't support setting a variable number of config values per pin/
group. Tegra30 has 13 define TEGRA_PMX_CONF_* values, and requiring every
one of those to be set for every pin/group would be a bit unwieldy,
especially since 99% of the time we just rely on the HW defaults, and I
imagine many other SoCs are the same.
> > (Note that I think we've agreed to remove the first cell above, &tegra_pmx,
> > now by requiring such nodes exist as children of the pin controller.)
>
> Sorry I don't quite follow, can you please maybe repost a complete
> .dts entry for your pin controller and one driver entry?
Yes, I'll try to do that very soon. The change is simple; from:
mux =
<&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
to:
mux =
<TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
<TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
We can do this by requiring the node that contains the mux property to
be a child of the pin controller node, so the phandle value is always
the node's parent node.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-19 13:10 ` Thomas Abraham
2012-01-19 16:56 ` Tony Lindgren
@ 2012-01-20 21:11 ` Stephen Warren
2012-01-21 1:27 ` Thomas Abraham
1 sibling, 1 reply; 54+ messages in thread
From: Stephen Warren @ 2012-01-20 21:11 UTC (permalink / raw)
To: Thomas Abraham
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Thomas Abraham wrote at Thursday, January 19, 2012 6:10 AM:
> On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
> > I thought a bit more about pinmux DT bindings. I came up with something
> > that I like well enough, and is pretty similar to the binding that Dong
> > posted recently. I think it'll work for both Tegra's and IMX's needs.
> > Please take a look!
> >
> > Note: I've used named constants below just to make this easier to read.
> > We still don't have a solution to actually use named constants in dtc yet.
>
> [...]
>
> For Samsung io-pad controllers, I had been considering a dt approach
> which has been described below. Kindly review and any comments will be
> helpful.
>
>
> * Main points to be considered:
>
> All Samsung SoC's use a io-pad controller that includes gpio, pinmux
> and pinconfig functionality in a single controller, i.e. there is a
> single intermixed address space for gpio/pinmux/pinconfig portions in
> the controller. Additionally, all the drivers for the Samsung SoC's
> setup pinmux/pinconfig in its probe function (and in resume if
> required).
>
>
> * IO Pad controllers in dts file:
>
> The io-pad controller (gpio/pinmux/pinconfig) can be represented in a
> dtsi file as below. There could be multiple io-pad controllers
> supported in the system.
>
> pctrl0: pinctrl@11400000 {
> compatible = "samsung,exynos4210-pinctrl";
> reg = <0x11400000 0x1000>;
> interrupts = <.......>;
> pin-banks = <....>;
> [... other properties TBD ...]
> #pinctrl-cells = <5>;
> };
>
> Each such node instantiates one instance of the pin-controller device.
> The 'struct pinctrl_dev' should include a new member 'struct of_node'
> to point to the corresponding pin-controller node in dt which
> instantiated the controller. The pinctrl_register() function called
> from drivers/pinctrl/pinctrl-xyz.c then registers the pin-controller
> instance.
>
Yes, that's all reasonable, and exactly what I had in mind when I wrote
my binding proposal.
> * Specifying the pinmux/pinconfig settings in dts files:
>
> Device nodes which require specific pinmux/pinconfig settings should
> include information about the required settings. For example, a i2c
> controller node in dts file is listed below.
>
> i2c0: i2c@18000000 {
> [... other properties ...]
> pinctrl-active = <&pctrl0 5 0 2 3 0>,
> <&pctrl0 5 1 2 3 0>;
> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
> <&pctrl0 5 1 2 0 0>;
> };
The idea of encoding the state names into the property names came up
before in this thread. The problem is that it's hard for core code
to know which properties are actually related to pinctrl and which
aren't. reserving one name such as "pinctrl" seems reasonable, whereas
reserving a whole class of names; everything prefixed "pinctrl-foo" is
a little less so.
If the parsing of these nodes were a direct result of a driver calling
pinmux_get("name") or similar, this might be less relevant. However,
there's a desire to parse all the pinmux properties/nodes up-front so
that the pinctrl pinmux mapping table can be completely populated early
during boot, and hence contain all pinmux information, just as if that
mapping table were registered from static tables by a board file in the
non DT case. So, being certain what's a pinctrl node/property without
information on the state names drivers will use is important.
> In the example above, the specifier of pinctrl-* is specific for
> Samsung io-pad controllers. Its format/syntax would be mainly
> dependent on the io-pad controller used, the above is only an example
> for Samsung io-pad controller. In the above node, the format of the
> pinctrl-* specifier is
>
> property-name = <phandle of the pin-controller
> pin bank within the pin-controller
> pin number within the pin-bank
> pin-mux function number
> pull up/down config
> drive strength config>;
Yes, that seems reasonable.
The only thing different between that example and my proposal is that:
a) In my proposal, the property in the device nodes doesn't actually
contain all those fields, but a phandle to a child node of the pin
controller where that data is kept. This keeps all the pin mux data
stored under the pin controller's node for neatness.
Thus, there's 1 extra level of indirection.
This allows common sets of values to be defined and the device
nodes can simply reference this, rather than cut/pasting the data
into every board file that uses the same configuration.
(Well, I actually proposed that referenced node could be anywhere,
but others were insistent it should only be allowed under the pin
controller node)
b) I didn't actually include a #pinctrl-cells property in my proposal,
assuming that a fixed-format would be acceptable for these properties.
However, I will include such a property in my V2 proposal.
> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
>
> A driver (for a device instantiated from device tree) would require
> code to be made aware of the pinmux/pinconfig options available. The
> typical sequence in a probe function would be as below.
>
> (a) call of_get_named_gpio() to get the gpio number. In case of
Not everything is a GPIO in all SoCs, so initiating pin mux configuration
from of_get_named_gpio() doesn't really make sense.
The pinctrl subsystem already has APIs such as pinmux_get() and
pinmux_enable() that initiating pin mux programming, so I've been
assuming they'd be used identically for both systems that use board
files and systems that use DT.
...
> (d) For all entries in pinctrl-* property, use
> of_parse_phandles_with_args() and get the pinctrl node pointer and the
> pinctrl specifier. As an example, the i2c driver would do the
> following, incrementing pin-index parameter for each call.
>
> ret = of_parse_phandle_with_args(i2c_np, "pinctrl-active",
> "#pinctrl-cells", pin-index, &pctrl_np, &pctrl_specifier);
>
> (e) There should be a new api in pinctrl subsystem whose signature
> could be something like
>
> int pinctrl_dt_parse_and_set_mux_cfg(struct device_node *, const void *);
>
> For each iteration of step (d) in the driver, this new api will be
> called. The value of pctrl_np and pctrl_specifier obtained from step
> (d) is passed as a parameters to this new api. This api will do the
> following
>
> (e1) Find the pin-controller (struct pinctrl_dev) that has a
> device_node pointer which is same as the first parameter. To recap, it
> was mentioned above that: "The 'struct pinctrl_dev' should include a
> new member 'struct of_node' to point to the corresponding
> pin-controller node in dt which instantiated the controller."
>
> (e2) A new callback 'xlate_pinctrl' is required in 'struct
> pinctrl_ops' which can translate the second parameter of
> 'pinctrl_dt_parse_and_set_mux_cfg' function. From the pin controller
> found in step (e1), call pinctrl_dev->desc->pctlops->xlate_pinctrl
> passing the second parameter of 'pinctrl_dt_parse_and_set_mux_cfg'
> function. The pin-controller specific translator function will
> translate the parameter and program the hardware registers. The
> xlate_pinctrl is specific to each pin-controller is it knows how to
> decode the specifier and program the registers.
But yes, I'd expect much of those mechanics to be used roughly as you
describe, just initiated by other APIs; some DT-wide parsing of the
pinmux properties at either system boot time or pin controller
registration time, and application of the data during pinmux_get().
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-19 16:56 ` Tony Lindgren
2012-01-19 17:38 ` Thomas Abraham
@ 2012-01-20 21:15 ` Stephen Warren
1 sibling, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-20 21:15 UTC (permalink / raw)
To: Tony Lindgren, Thomas Abraham
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Tony Lindgren wrote at Thursday, January 19, 2012 9:56 AM:
> * Thomas Abraham <thomas.abraham@linaro.org> [120119 04:37]:
...
> > * Specifying the pinmux/pinconfig settings in dts files:
> >
> > Device nodes which require specific pinmux/pinconfig settings should
> > include information about the required settings. For example, a i2c
> > controller node in dts file is listed below.
> >
> > i2c0: i2c@18000000 {
> > [... other properties ...]
> > pinctrl-active = <&pctrl0 5 0 2 3 0>,
> > <&pctrl0 5 1 2 3 0>;
> > pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
> > <&pctrl0 5 1 2 0 0>;
> > };
>
> Maybe we should have just the active/suspend/off flags in the
> same array? Otherwise we'll end up unnecessarily repeating the
> the pin information. See the pins + #pin-args example I posted,
> which is otherwise similar.
Earlier discussions in this thread have discussed how to share common
settings between the various named states, by allowing the device to
refer to multiple properties or nodes for each named state; roughly:
foo@c8000200 {
...
pinctrl = <&pinmux_foo> <&pinmux_foo_extra>
<&pinmux_foo_suspend> <&pinmux_foo_extra_suspend>;
pinctrl-entries = <2 2>;
pinctrl-names = "active", "suspend";
};
I'll describe this more completely in my V2 bindings proposal.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 21:11 ` Stephen Warren
@ 2012-01-21 1:27 ` Thomas Abraham
2012-01-23 22:43 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Thomas Abraham @ 2012-01-21 1:27 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Hi Stephen,
On 21 January 2012 02:41, Stephen Warren <swarren@nvidia.com> wrote:
> Thomas Abraham wrote at Thursday, January 19, 2012 6:10 AM:
>> On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
>> > I thought a bit more about pinmux DT bindings. I came up with something
>> > that I like well enough, and is pretty similar to the binding that Dong
>> > posted recently. I think it'll work for both Tegra's and IMX's needs.
>> > Please take a look!
>> >
>> > Note: I've used named constants below just to make this easier to read.
>> > We still don't have a solution to actually use named constants in dtc yet.
>>
>> [...]
>>
>> For Samsung io-pad controllers, I had been considering a dt approach
>> which has been described below. Kindly review and any comments will be
>> helpful.
>>
>>
>> * Main points to be considered:
>>
>> All Samsung SoC's use a io-pad controller that includes gpio, pinmux
>> and pinconfig functionality in a single controller, i.e. there is a
>> single intermixed address space for gpio/pinmux/pinconfig portions in
>> the controller. Additionally, all the drivers for the Samsung SoC's
>> setup pinmux/pinconfig in its probe function (and in resume if
>> required).
>>
>>
>> * IO Pad controllers in dts file:
>>
>> The io-pad controller (gpio/pinmux/pinconfig) can be represented in a
>> dtsi file as below. There could be multiple io-pad controllers
>> supported in the system.
>>
>> pctrl0: pinctrl@11400000 {
>> compatible = "samsung,exynos4210-pinctrl";
>> reg = <0x11400000 0x1000>;
>> interrupts = <.......>;
>> pin-banks = <....>;
>> [... other properties TBD ...]
>> #pinctrl-cells = <5>;
>> };
>>
>> Each such node instantiates one instance of the pin-controller device.
>> The 'struct pinctrl_dev' should include a new member 'struct of_node'
>> to point to the corresponding pin-controller node in dt which
>> instantiated the controller. The pinctrl_register() function called
>> from drivers/pinctrl/pinctrl-xyz.c then registers the pin-controller
>> instance.
>>
>
> Yes, that's all reasonable, and exactly what I had in mind when I wrote
> my binding proposal.
>
>> * Specifying the pinmux/pinconfig settings in dts files:
>>
>> Device nodes which require specific pinmux/pinconfig settings should
>> include information about the required settings. For example, a i2c
>> controller node in dts file is listed below.
>>
>> i2c0: i2c@18000000 {
>> [... other properties ...]
>> pinctrl-active = <&pctrl0 5 0 2 3 0>,
>> <&pctrl0 5 1 2 3 0>;
>> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
>> <&pctrl0 5 1 2 0 0>;
>> };
>
> The idea of encoding the state names into the property names came up
> before in this thread.
Yes, I did borrow the idea of states 'active' and 'suspend' from the
dt binding discussions here. The current Samsung mainline dt support
for gpio and pinmux is using this type of encoding but did not have
the states.
> The problem is that it's hard for core code
> to know which properties are actually related to pinctrl and which
> aren't. reserving one name such as "pinctrl" seems reasonable, whereas
> reserving a whole class of names; everything prefixed "pinctrl-foo" is
> a little less so.
The basic premise with which I proposed about the dt support for pinctrl was
- The pinctrl core code does not have to know anything about these
bindings inside device nodes (of say i2c).
- Device drivers (such as i2c) retrieve the value of pinctrl-*
property and pass on two parameters to the pinctrl code.
(a) The 'struct device_node' pointer of the intended pinctrl_dev
instance (obtained from the phandle above).
(b) The encoding that specifies the hardware register values.
- Pinctrl core scans its list for all registered pinctrl_dev, matches
it against the 'device node' specified and selects one of the
pinctrl_dev. It then passes on the second parameter to the pinctrl
driver which decodes it and writes appropriate values to the hardware
registers.
- pinmux/pinconfig/pindesc tables are not built from DT. There are no
static tables built into the SoC specific pinctrl driver.
Detailed steps on how this can be handled (from driver getting hold of
pinctrl-* property till hardware registers getting programmed) is
listed in the previous email.
>
> If the parsing of these nodes were a direct result of a driver calling
> pinmux_get("name") or similar, this might be less relevant. However,
> there's a desire to parse all the pinmux properties/nodes up-front so
> that the pinctrl pinmux mapping table can be completely populated early
> during boot, and hence contain all pinmux information, just as if that
> mapping table were registered from static tables by a board file in the
> non DT case. So, being certain what's a pinctrl node/property without
> information on the state names drivers will use is important.
Yes, that is true if the pinmux/pinconfig tables are built from DT.
But it would be easier if no tables are built.
>
>> In the example above, the specifier of pinctrl-* is specific for
>> Samsung io-pad controllers. Its format/syntax would be mainly
>> dependent on the io-pad controller used, the above is only an example
>> for Samsung io-pad controller. In the above node, the format of the
>> pinctrl-* specifier is
>>
>> property-name = <phandle of the pin-controller
>> pin bank within the pin-controller
>> pin number within the pin-bank
>> pin-mux function number
>> pull up/down config
>> drive strength config>;
>
> Yes, that seems reasonable.
>
> The only thing different between that example and my proposal is that:
>
> a) In my proposal, the property in the device nodes doesn't actually
> contain all those fields, but a phandle to a child node of the pin
> controller where that data is kept. This keeps all the pin mux data
> stored under the pin controller's node for neatness.
I am not sure if that is required. In case dt support for interrupt
controller such as gic, the interrupt number, type of interrupt and
edge/level type flags are all listed in the device node itself and not
under the gic device node.
>
> Thus, there's 1 extra level of indirection.
>
> This allows common sets of values to be defined and the device
> nodes can simply reference this, rather than cut/pasting the data
> into every board file that uses the same configuration.
Yes, that is true. I also had the same concern on this.
>
> (Well, I actually proposed that referenced node could be anywhere,
> but others were insistent it should only be allowed under the pin
> controller node)
>
> b) I didn't actually include a #pinctrl-cells property in my proposal,
> assuming that a fixed-format would be acceptable for these properties.
> However, I will include such a property in my V2 proposal.
>
>> * Using the pinmux/pinconfig specifier in device nodes to configure hardware.
>>
>> A driver (for a device instantiated from device tree) would require
>> code to be made aware of the pinmux/pinconfig options available. The
>> typical sequence in a probe function would be as below.
>>
>> (a) call of_get_named_gpio() to get the gpio number. In case of
>
> Not everything is a GPIO in all SoCs, so initiating pin mux configuration
> from of_get_named_gpio() doesn't really make sense.
Right, I was mainly writing about what can be done for Samsung
platforms. Other platforms can opt to do things differently. But what
is important here is, the ability in pinctrl core to identify a
pinctrl driver which can decode the bindings and have it program the
registers.
>
> The pinctrl subsystem already has APIs such as pinmux_get() and
> pinmux_enable() that initiating pin mux programming, so I've been
> assuming they'd be used identically for both systems that use board
> files and systems that use DT.
>
In case of DT based boot, the code that handles the device node
containing pinctrl-* (either device driver or platform code) need not
call the pinmux_get(). The encoding in the pinctrl-* (as listed above)
is sufficient to setup the pinmux/pinconfig as required.
> ...
>> (d) For all entries in pinctrl-* property, use
>> of_parse_phandles_with_args() and get the pinctrl node pointer and the
>> pinctrl specifier. As an example, the i2c driver would do the
>> following, incrementing pin-index parameter for each call.
>>
>> ret = of_parse_phandle_with_args(i2c_np, "pinctrl-active",
>> "#pinctrl-cells", pin-index, &pctrl_np, &pctrl_specifier);
>>
>> (e) There should be a new api in pinctrl subsystem whose signature
>> could be something like
>>
>> int pinctrl_dt_parse_and_set_mux_cfg(struct device_node *, const void *);
>>
>> For each iteration of step (d) in the driver, this new api will be
>> called. The value of pctrl_np and pctrl_specifier obtained from step
>> (d) is passed as a parameters to this new api. This api will do the
>> following
>>
>> (e1) Find the pin-controller (struct pinctrl_dev) that has a
>> device_node pointer which is same as the first parameter. To recap, it
>> was mentioned above that: "The 'struct pinctrl_dev' should include a
>> new member 'struct of_node' to point to the corresponding
>> pin-controller node in dt which instantiated the controller."
>>
>> (e2) A new callback 'xlate_pinctrl' is required in 'struct
>> pinctrl_ops' which can translate the second parameter of
>> 'pinctrl_dt_parse_and_set_mux_cfg' function. From the pin controller
>> found in step (e1), call pinctrl_dev->desc->pctlops->xlate_pinctrl
>> passing the second parameter of 'pinctrl_dt_parse_and_set_mux_cfg'
>> function. The pin-controller specific translator function will
>> translate the parameter and program the hardware registers. The
>> xlate_pinctrl is specific to each pin-controller is it knows how to
>> decode the specifier and program the registers.
>
> But yes, I'd expect much of those mechanics to be used roughly as you
> describe, just initiated by other APIs; some DT-wide parsing of the
> pinmux properties at either system boot time or pin controller
> registration time, and application of the data during pinmux_get().
>
> --
> nvpublic
>
Thanks for having a look on this.
Regards,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 17:53 ` Tony Lindgren
@ 2012-01-21 1:38 ` Thomas Abraham
0 siblings, 0 replies; 54+ messages in thread
From: Thomas Abraham @ 2012-01-21 1:38 UTC (permalink / raw)
To: Tony Lindgren
Cc: Stephen Warren, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On 20 January 2012 23:23, Tony Lindgren <tony@atomide.com> wrote:
> * Thomas Abraham <thomas.abraham@linaro.org> [120120 16:45]:
>> On 20 January 2012 15:35, Tony Lindgren <tony@atomide.com> wrote:
>> > * Thomas Abraham <thomas.abraham@linaro.org> [120119 10:05]:
>> >> On 19 January 2012 23:50, Tony Lindgren <tony@atomide.com> wrote:
>> >>
>> >> I would like to understand the need for populating the
>> >> pinmux/pingroups tables from dt. The question here is when we have
>> >> something like
>> >>
>> >> pins = <&pinctrl0 0x0030 0x15 0x15 0x7>;
>> >>
>> >> which specifies the values that need to be written to the hardware
>> >> registers, would populating pinmux/pingroup tables from dt required.
>> >> The SoC specific pinctrl driver can provide a way (with the help of
>> >> pinctrl core) to translate these values and write to corresponding
>> >> hardware registers. Is there any particular reason for populating the
>> >> pinmux/pingroups tables from dt?
>> >
>> > Hmm I see. Yes it's still needed as we only want to parse the DT once
>> > because it's slower unless it was one time only configuration during
>> > init.
>>
>> Ok. The time spent on searching for the pin-config property can be
>> reduced by having the device driver (say, i2c) keep a pointer to all
>> the pinconfig properties in its node. The next time a driver needs to
>> reconfigure the pins, the search time can be reduced. The time to
>> parse the property values though would still be applicable. But I
>> would still not opt to build pinmux/pinconfig/pindesc tables from dt.
>
> Hmm that's something to consider to save memory as the node will stay
> there. This would allow making all the pinctrl framework data __initdata
> in some cases. You'd probably want to copy the data into the driver in
> some pinctrl framework struct so you could still use pinctrl framework
> functions with this data and not have to parse the node again.
Is it required to use pinctrl functions when we already have the
hardware register values for pinmux/pinconfig in the device nodes? I
was of the opinion that we need not use pinctrl API in case of dt
based boot.
>
> For building the tables from dt, what I have currently is building
> the tables without any specific knowledge about the pinmux functions.
> I'm thinking that any further knowledge for debugging etc can be done
> later on using user space tools to avoid storing the data in kernel.
>
Ok. I will go through your patches when you post them.
Thanks,
Thomas.
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 20:50 ` Stephen Warren
@ 2012-01-23 20:13 ` Tony Lindgren
2012-01-23 22:54 ` Stephen Warren
2012-01-27 13:11 ` Linus Walleij
1 sibling, 1 reply; 54+ messages in thread
From: Tony Lindgren @ 2012-01-23 20:13 UTC (permalink / raw)
To: Stephen Warren
Cc: Grant Likely, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
* Stephen Warren <swarren@nvidia.com> [120120 12:20]:
> Tony wrote at Lindgren:
> > * Stephen Warren <swarren@nvidia.com> [120118 11:29]:
> > > Tony Lindgren wrote at Wednesday, January 18, 2012 7:13 AM:
> >
> > Assuming this is describing the pins a driver is using, how
> > about calling it pins?
>
> It's not always pins.
>
> For a lot of HW it is pins, you're right.
>
> For Tegra20 (and IIRC some other HW), the pin mux HW actually muxes
> groups of pins; one register field sets n (1, 2, 3, ...) pins to that
> function at once. Hence, the entries are real physical groups.
OK fair enough, mux instead of pins works fine for me.
> For some HW that controls the mux per pin, the SoC vendors wish the
> pinctrl driver to expose what I call "virtual groups" of pins, e.g. all
> the pins that are typically used together as a single I2C or SD/MMC
> interface, as a single muxable entity (a group in pinctrl parlance).
> In this case, the values listed here will be these group IDs
For the virtual groups, do you also want to specify them separate
in the .dts files?
IMHO we should let device tree take care of that as it already
allows specifying the pins for each device entry. Then the
dynamically generated group name can be nc->full_name of the
device.
But other than that, I don't know if there's need to specify
pingroups in the .dts files outside the device node using the
pins?
> > That's because you might want to do all the muxing in a
> > bootloader, but still need to tell how many pins you're using
> > for MMC on a device. So it actually has a wider meaning than just
> > mux.
>
> I don't think that affects the name of the property:-)
Yes mux for the name works fine there too.
> I see your point about separate ownership of pins/groups and the actual
> HW register programming. The pinctrl subsystem doesn't have a concept
> of separating those two things at the moment though. I don't think its
> unreasonable to still have to write the mux values into the device tree
> and even reprogram them to the same value when Linux boots though. Do
> you see a problem with that? If there is a problem, we need to fix it
> in pinctrl too, irrespective of any device tree work.
Yeah I think we can handle some pinmuxing cases automatically.
But basically we need to support also the following typical cases:
1. Static muxing in bootloader only and no mux entries in the .dts files
2. Init time only muxing based on the entries in .dts files
3. Dynamic remuxing of pins for the pins specified in the .dts files
For the last one we need to do some extra work to avoid wasting memory,
and need some pin specific flag to describe if the pin is init time only
or dynamic.
> > Also, we need to standardize on some name to use for parsing pins
> > using of_parse_phandle_with_args, and I suggested #pin-args.
>
> "cells" is the suffix that's typically used rather than "args".
OK
> I certainly foresee the need for a #mux-cells and a #config-cells
> Property so that pinctrl bindings/nodes can tell the core parser of
> the mux/config properties how many cells to pull out for each entry.
>
> In my binding proposal, I original assumed that every controller would
> always have group,function for mux and group,param,value for config.
> However, having #mux-cells and #config-cells is certainly a better idea.
>
> > > config =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> > > <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
> > > };
> >
> > Here I don't quite understand how config is different from pins/mux
> > above? It seems to set the driver/pull stuff, but why don't you
> > just make #pin-args larger and have a wider pin array?
>
> The idea is that "mux" lists each pin/group that the device uses, and
> the pin mux selection for it, whereas "config" lists all the other
> configuration values that could be applied to a pin; pull-up, pull-down,
> tri-state, drive strength, ...
>
> I suppose we could lump all those into a single property like:
>
> pinctrl =
> <MUX &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <MUX &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
> <CFG &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
>
> (where e.g. MUX=0, CFG=1, both interpreted by the parsing code in the
> core pinctrl subsystem)
>
> However, I'd tend towards keeping them in separate properties, especially
> since simple chips won't have any CFG (even on Tegra, 99% of the settings
> we make are MUX not CFG) and requiring this MUX value in every mux entry
> seems wasteful.
>
> > Something like:
> >
> > pins =
> > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1
> > &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1>;
> >
> > and in the parent set #pin-args to 3.
>
> That doesn't support setting a variable number of config values per pin/
> group. Tegra30 has 13 define TEGRA_PMX_CONF_* values, and requiring every
> one of those to be set for every pin/group would be a bit unwieldy,
> especially since 99% of the time we just rely on the HW defaults, and I
> imagine many other SoCs are the same.
But aren't these 13 defines for TEGRA_PMX_CONF_* just bit offsets in some
register?
If so, why don't you just have one register to define the values? Then when
the preprocessing of .dts files works, you can use defines there?
Even if you had multiple registers per pin(group), you just need the
phandle and value for each register?
> > > (Note that I think we've agreed to remove the first cell above, &tegra_pmx,
> > > now by requiring such nodes exist as children of the pin controller.)
> >
> > Sorry I don't quite follow, can you please maybe repost a complete
> > .dts entry for your pin controller and one driver entry?
>
> Yes, I'll try to do that very soon. The change is simple; from:
>
> mux =
> <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <&tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
>
>
> to:
>
> mux =
> <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1>
> <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1>;
>
> We can do this by requiring the node that contains the mux property to
> be a child of the pin controller node, so the phandle value is always
> the node's parent node.
Ah I see. I don't think we can leave out the phandle as that will
not work for mixing pins from multiple pin controller instances.
Regards,
Tony
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-21 1:27 ` Thomas Abraham
@ 2012-01-23 22:43 ` Stephen Warren
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-23 22:43 UTC (permalink / raw)
To: Thomas Abraham
Cc: Dong Aisheng-B29396, linus.walleij, s.hauer, rob.herring, kernel,
cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Thomas Abraham wrote at Friday, January 20, 2012 6:28 PM:
> On 21 January 2012 02:41, Stephen Warren <swarren@nvidia.com> wrote:
> > Thomas Abraham wrote at Thursday, January 19, 2012 6:10 AM:
> >> On 14 January 2012 02:09, Stephen Warren <swarren@nvidia.com> wrote:
...
> >> * Specifying the pinmux/pinconfig settings in dts files:
> >>
> >> Device nodes which require specific pinmux/pinconfig settings should
> >> include information about the required settings. For example, a i2c
> >> controller node in dts file is listed below.
> >>
> >> i2c0: i2c@18000000 {
> >> [... other properties ...]
> >> pinctrl-active = <&pctrl0 5 0 2 3 0>,
> >> <&pctrl0 5 1 2 3 0>;
> >> pinctrl-suspend = <&pctrl0 5 0 2 0 0>,
> >> <&pctrl0 5 1 2 0 0>;
> >> };
> >
> > The idea of encoding the state names into the property names came up
> > before in this thread.
>
> Yes, I did borrow the idea of states 'active' and 'suspend' from the
> dt binding discussions here. The current Samsung mainline dt support
> for gpio and pinmux is using this type of encoding but did not have
> the states.
>
> > The problem is that it's hard for core code
> > to know which properties are actually related to pinctrl and which
> > aren't. reserving one name such as "pinctrl" seems reasonable, whereas
> > reserving a whole class of names; everything prefixed "pinctrl-foo" is
> > a little less so.
>
> The basic premise with which I proposed about the dt support for pinctrl was
>
> - The pinctrl core code does not have to know anything about these
> bindings inside device nodes (of say i2c).
>
> - Device drivers (such as i2c) retrieve the value of pinctrl-*
> property and pass on two parameters to the pinctrl code.
> (a) The 'struct device_node' pointer of the intended pinctrl_dev
> instance (obtained from the phandle above).
> (b) The encoding that specifies the hardware register values.
That seems like a bunch of extra work for device drivers, and also
something that's only required for device tree. By having a standardized
representation (or framework for one) of the pinctrl properties within
each device node, then drivers can:
a) Drivers don't have to do a/b above, instead the pinctrl core will
do it.
b) Drivers can use the exact same pinctrl APIs when the system booted
using DT or not.
> - Pinctrl core scans its list for all registered pinctrl_dev, matches
> it against the 'device node' specified and selects one of the
> pinctrl_dev. It then passes on the second parameter to the pinctrl
> driver which decodes it and writes appropriate values to the hardware
> registers.
>
> - pinmux/pinconfig/pindesc tables are not built from DT. There are no
> static tables built into the SoC specific pinctrl driver.
Not having any pin/group/function tables in either the driver or DT
would be a pretty radical departure from the way the pinctrl subsystem
works today. It'd basically make pinctrl irrelevant on DT, I think, if
I'm understanding what you're saying correctly. I don't know if we
really want to go down that path.
Not having the pinctrl pinmux mapping table parsed up front, but instead
deferring it until a driver called pinmux_get() might be reasonable. I'm
not totally opposed to that, but others have expressed a desire to parse
up front, so that the pinctrl sysfs files that contain the pinmux mapping
table are identical for a non-DT and DT boot.
...
> >> In the example above, the specifier of pinctrl-* is specific for
> >> Samsung io-pad controllers. Its format/syntax would be mainly
> >> dependent on the io-pad controller used, the above is only an example
> >> for Samsung io-pad controller. In the above node, the format of the
> >> pinctrl-* specifier is
> >>
> >> property-name = <phandle of the pin-controller
> >> pin bank within the pin-controller
> >> pin number within the pin-bank
> >> pin-mux function number
> >> pull up/down config
> >> drive strength config>;
> >
> > Yes, that seems reasonable.
> >
> > The only thing different between that example and my proposal is that:
> >
> > a) In my proposal, the property in the device nodes doesn't actually
> > contain all those fields, but a phandle to a child node of the pin
> > controller where that data is kept. This keeps all the pin mux data
> > stored under the pin controller's node for neatness.
>
> I am not sure if that is required. In case dt support for interrupt
> controller such as gic, the interrupt number, type of interrupt and
> edge/level type flags are all listed in the device node itself and not
> under the gic device node.
Yes, there's no specific need to place those nodes inside the
pin controller's node. I intended to put them inside the individual
device nodes for my boards. However, others have expressed a strong
desire to only allow these nodes to be inside the pin controller node,
and I've gone along with that in order to move the binding forward.
While this is a little different to GPIOs and IRQs, I don't see a
significant disadvantage to this requirement; when I started looking
at pinmux DT months ago, I had planned on simply putting a single big
table inside the pin controller node anyway, so this isn't so different.
...
> > The pinctrl subsystem already has APIs such as pinmux_get() and
> > pinmux_enable() that initiate pin mux programming, so I've been
> > assuming they'd be used identically for both systems that use board
> > files and systems that use DT.
>
> In case of DT based boot, the code that handles the device node
> containing pinctrl-* (either device driver or platform code) need not
> call the pinmux_get(). The encoding in the pinctrl-* (as listed above)
> is sufficient to setup the pinmux/pinconfig as required.
I think drivers should call pinmux_get() in all cases. We don't want
some drivers using pinmux_get() and only working without DT, and others
using some different API and only working with DT. We need to hide all
the DT/non-DT details behind the existing APIs so that drivers are as
portable as possible.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-23 20:13 ` Tony Lindgren
@ 2012-01-23 22:54 ` Stephen Warren
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-23 22:54 UTC (permalink / raw)
To: Tony Lindgren
Cc: Grant Likely, Dong Aisheng-B29396, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Tony Lindgren wrote at Monday, January 23, 2012 1:13 PM:
> * Stephen Warren <swarren@nvidia.com> [120120 12:20]:
> > Tony wrote at Lindgren:
> > > * Stephen Warren <swarren@nvidia.com> [120118 11:29]:
> > > > Tony Lindgren wrote at Wednesday, January 18, 2012 7:13 AM:
> > >
> > > Assuming this is describing the pins a driver is using, how
> > > about calling it pins?
> >
> > It's not always pins.
> >
> > For a lot of HW it is pins, you're right.
> >
> > For Tegra20 (and IIRC some other HW), the pin mux HW actually muxes
> > groups of pins; one register field sets n (1, 2, 3, ...) pins to that
> > function at once. Hence, the entries are real physical groups.
>
> OK fair enough, mux instead of pins works fine for me.
>
> > For some HW that controls the mux per pin, the SoC vendors wish the
> > pinctrl driver to expose what I call "virtual groups" of pins, e.g. all
> > the pins that are typically used together as a single I2C or SD/MMC
> > interface, as a single muxable entity (a group in pinctrl parlance).
> > In this case, the values listed here will be these group IDs
>
> For the virtual groups, do you also want to specify them separate
> in the .dts files?
/I/ don't want to specify virtual groups at all:-)
It's up to individual pinctrl drivers what groups they define. Equally,
it's up to the drivers whether they define their list of groups as static
data within the driver, or as nodes/properties in the device tree.
Nothing that I put into my binding proposal is in any way related to what
I call virtual groups; the "pin configuration nodes" are simply a listing
of mux or config values for whatever pins or groups the pinctrl driver
happens to expose, whether they're real HW pins/groups or not, and whether
the list they expose came from a static table or not.
> IMHO we should let device tree take care of that as it already
> allows specifying the pins for each device entry. Then the
> dynamically generated group name can be nc->full_name of the
> device.
I don't quite follow that, but I think it's important to allow drivers
flexibility to store their list of pins/groups/functions wherever they
want; mandating that the lists be stored in device tree won't please
everyone.
...
> > I see your point about separate ownership of pins/groups and the actual
> > HW register programming. The pinctrl subsystem doesn't have a concept
> > of separating those two things at the moment though. I don't think its
> > unreasonable to still have to write the mux values into the device tree
> > and even reprogram them to the same value when Linux boots though. Do
> > you see a problem with that? If there is a problem, we need to fix it
> > in pinctrl too, irrespective of any device tree work.
>
> Yeah I think we can handle some pinmuxing cases automatically.
> But basically we need to support also the following typical cases:
>
> 1. Static muxing in bootloader only and no mux entries in the .dts files
That's easily handled in the binding I proposed; simply don't put any
mux/config settings into the device tree. Portable drives will still
call pinmux_get(), so there need to be nodes to back that up, but the
list of strings that get applied can be empty if you wish, assuming the
driver doesn't need different configurations at runtime.
> 2. Init time only muxing based on the entries in .dts files
Similar answer to (1).
> 3. Dynamic remuxing of pins for the pins specified in the .dts files
Yes, that's handled by simply putting multiple named configurations into
the device tree.
...
> > > Something like:
> > >
> > > pins =
> > > <&tegra_pmx TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1
> > > &tegra_pmx TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_1 TEGRA_PMX_CONF_TRISTATE 1>;
> > >
> > > and in the parent set #pin-args to 3.
> >
> > That doesn't support setting a variable number of config values per pin/
> > group. Tegra30 has 13 define TEGRA_PMX_CONF_* values, and requiring every
> > one of those to be set for every pin/group would be a bit unwieldy,
> > especially since 99% of the time we just rely on the HW defaults, and I
> > imagine many other SoCs are the same.
>
> But aren't these 13 defines for TEGRA_PMX_CONF_* just bit offsets in some
> register?
No.
The same TEGRA_PMX_CONF_* parameter doesn't necessarily exist for all
pins or all groups.
Where it does exist, different pins/groups may store the value in a
different bit number in the/some register based on the group, and the
width of the bit field may vary too.
So, the TEGRA_PMX_CONF_* are just a logical name/ID of the parameter to be
configured, which is then looked up in the table of pins/groups known to
the Tegra pinctrl driver, which also determines the register, bit, and
field size for the combination.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 20:28 ` Stephen Warren
@ 2012-01-27 12:00 ` Linus Walleij
2012-01-27 16:58 ` Stephen Warren
0 siblings, 1 reply; 54+ messages in thread
From: Linus Walleij @ 2012-01-27 12:00 UTC (permalink / raw)
To: Stephen Warren
Cc: Dong Aisheng-B29396, Shawn Guo, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Fri, Jan 20, 2012 at 9:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Dong Aisheng wrote at Wednesday, January 18, 2012 4:06 AM:
>>
>> Each entry in the list is PIN rather than group.
>> So we cannot create each map for each entry in the list except we treat the PIN as
>> A group(this will cause a huge maps and predefined groups and functions).
>
> Linusw has NAKd the idea that we should not use these "virtual groups",
> i.e. he believes they're fine to use if you wish.
Interesting double inversion here: I NACK to NOT use groups
this way...
The groups are not "virtual" if they share some physical
property. Such as being used by the same device.
The subsystem has callbacks to list, name and get the
pins belonging to a group. What defines a group is up
to the driver, it better be something that makes sense
else the driver will likely become hard to maintain.
If and only if you enable the *OPTIONAL* pinmux part
of the pin control subsystem, there is something
called pinmuxes, these have pins associated with them,
and by design they have to present the pins associated
with them by naming one or more groups.
It is possible that the pinmux should be able to
contain single pins as well, Tony indicated something
like that to me. Currently you would have to use a
group with 1 pin in it to associate a single pin with
a pinmux, maybe this is not so elegant, it can be
fixed,
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 54+ messages in thread
* Re: Pinmux bindings proposal
2012-01-20 20:50 ` Stephen Warren
2012-01-23 20:13 ` Tony Lindgren
@ 2012-01-27 13:11 ` Linus Walleij
1 sibling, 0 replies; 54+ messages in thread
From: Linus Walleij @ 2012-01-27 13:11 UTC (permalink / raw)
To: Stephen Warren
Cc: Tony Lindgren, Grant Likely, Dong Aisheng-B29396, linus.walleij,
s.hauer, rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
On Fri, Jan 20, 2012 at 9:50 PM, Stephen Warren <swarren@nvidia.com> wrote:
> For Tegra20 (and IIRC some other HW), the pin mux HW actually muxes
> groups of pins; one register field sets n (1, 2, 3, ...) pins to that
> function at once. Hence, the entries are real physical groups.
Maybe I am getting things wrong, but I've read:
http://en.wiktionary.org/wiki/real
http://en.wiktionary.org/wiki/virtual
And now I am trying to address this from my understanding of
"real" as defined in clause 1:
"1. That has physical existence.
No one has ever seen a real unicorn."
And "virtual" as defined in clause 3:
"3. Of something that is simulated in a computer or on-line.
The virtual world of his computer game allowed character interaction."
Please tell me if I have the wrong definitions in my
head, I am not trying to play smart, I just want to understand
why this rubs me the wrong way. So:
On the Nomadik GPIO controller (not yet migrated to the pin
control subsystem, mea culpa) pins 0,1,2,3 can be used for UART0.
These four pins can also be used for GPIO, and two other
functions.
Does that make the group UART0 = {0,1,2,3} a virtual group
in sense (3) of the wiktionary article, i.e. "simulated in a
computer"?
To me: no, not at all.
Because it is a very real physical property of these pins that
UART0 can *only* appear on these 4 pins, not 4 other pins.
It is clear from the data sheet.
And even if they could appear on other pins, the number of
such appearances would be limited by physical factors, and
UART0_1 = {0,1,2,3} vs UART0_2 = {4,5,6,7} wouldn't
make it virtual. This is also a physical property, not simulated or virtual.
The *only* case where I could *maybe* accept the "virtual"
terminology would be for a phone-exchange type of
pinmux, where say this UART could appear on any 4 pins,
arbitrarily chosen. Still this is not fully virtual since it is
a physical fact that the UART group has to have 4 pins,
not 1, not 17.
Notice that I don't involve the use of that group on a
specific board in this argument, since the *silicon* limits the
muxable properties of this group, and it is a very real
group of pins.
This is why I think it is a bad idea to try to define pins as
being in a "real" or "virtual" group like this, where the only
criteria for that terminology is whether each pin has its own
control register or not.
I'm all happy with "Tegra type groups" and "Tegra type pins"
or "groups with separate control registers" or "pins with
separate control registers" which tells us what it really is,
"real" or "virtual" does not fit our discussion here the way
I see it.
Yours,
Linus Walleij
^ permalink raw reply [flat|nested] 54+ messages in thread
* RE: Pinmux bindings proposal
2012-01-27 12:00 ` Linus Walleij
@ 2012-01-27 16:58 ` Stephen Warren
0 siblings, 0 replies; 54+ messages in thread
From: Stephen Warren @ 2012-01-27 16:58 UTC (permalink / raw)
To: Linus Walleij
Cc: Dong Aisheng-B29396, Shawn Guo, linus.walleij, s.hauer,
rob.herring, kernel, cjb, Simon Glass (sjg@chromium.org),
Dong Aisheng, linux-kernel, linux-arm-kernel, devicetree-discuss
Linus Walleij wrote at Friday, January 27, 2012 5:01 AM:
> On Fri, Jan 20, 2012 at 9:28 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Dong Aisheng wrote at Wednesday, January 18, 2012 4:06 AM:
> >>
> >> Each entry in the list is PIN rather than group.
> >> So we cannot create each map for each entry in the list except we treat the PIN as
> >> A group(this will cause a huge maps and predefined groups and functions).
> >
> > Linusw has NAKd the idea that we should not use these "virtual groups",
> > i.e. he believes they're fine to use if you wish.
>
> Interesting double inversion here: I NACK to NOT use groups
> this way...
I don't think that's true.
If you did that, you would be /forcing/ people to use virtual pin groups,
which means that the Tegra driver I proposed wouldn't be acceptable
since it only defines groups that actually exist in HW.
> The groups are not "virtual" if they share some physical
> property. Such as being used by the same device.
No, that's not what a virtual group is (I'm the person who came up
with that term, and really the only person using it, so I get to decide
what I mean by it!) I suppose you can mean something different, but that's
just going to be ridiculously confusing.
In HW, there are individual register fields. Each one of these affects
often a single pin, but on some chips n pins at a time. A physical
group would be precisely those groups of pins which the actual HW
registers affect. A virtual group is a SW group that ends up affecting
more than one of those physical groups, since when a virtual groups is
programmed, the pinctrl driver goes and programs n register fields and
hence affects n physical groups.
And those registers and either physical or virtual groups don't have to
be anything to do with muxing. On Tegra, there are certainly groups that
only affect e.g. drive strength and not mux selection.
--
nvpublic
^ permalink raw reply [flat|nested] 54+ messages in thread
end of thread, other threads:[~2012-01-27 16:59 UTC | newest]
Thread overview: 54+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-01-13 20:39 Pinmux bindings proposal Stephen Warren
2012-01-14 7:09 ` Shawn Guo
2012-01-17 18:47 ` Stephen Warren
2012-01-18 3:32 ` Shawn Guo
2012-01-18 19:00 ` Stephen Warren
2012-01-16 12:50 ` Dong Aisheng-B29396
2012-01-17 8:23 ` Shawn Guo
2012-01-17 9:46 ` Dong Aisheng-B29396
2012-01-17 14:13 ` Shawn Guo
2012-01-17 19:32 ` Stephen Warren
2012-01-18 3:44 ` Dong Aisheng-B29396
2012-01-18 4:47 ` Shawn Guo
2012-01-18 19:24 ` Stephen Warren
2012-01-17 19:28 ` Stephen Warren
2012-01-18 11:06 ` Dong Aisheng-B29396
2012-01-20 20:28 ` Stephen Warren
2012-01-27 12:00 ` Linus Walleij
2012-01-27 16:58 ` Stephen Warren
2012-01-17 19:21 ` Stephen Warren
2012-01-18 4:01 ` Shawn Guo
2012-01-18 9:32 ` Dong Aisheng-B29396
2012-01-17 19:09 ` Stephen Warren
2012-01-18 7:24 ` Dong Aisheng-B29396
2012-01-18 19:42 ` Stephen Warren
2012-01-16 18:28 ` Grant Likely
2012-01-18 14:13 ` Tony Lindgren
2012-01-18 14:30 ` Shawn Guo
2012-01-18 15:32 ` Tony Lindgren
2012-01-18 16:29 ` Tony Lindgren
2012-01-18 20:22 ` Grant Likely
2012-01-18 20:20 ` Grant Likely
2012-01-19 10:31 ` Tony Lindgren
2012-01-18 20:02 ` Stephen Warren
2012-01-19 10:57 ` Tony Lindgren
2012-01-20 20:50 ` Stephen Warren
2012-01-23 20:13 ` Tony Lindgren
2012-01-23 22:54 ` Stephen Warren
2012-01-27 13:11 ` Linus Walleij
2012-01-18 12:16 ` Thomas Abraham
2012-01-18 19:52 ` Stephen Warren
2012-01-19 17:01 ` Tony Lindgren
2012-01-19 13:10 ` Thomas Abraham
2012-01-19 16:56 ` Tony Lindgren
2012-01-19 17:38 ` Thomas Abraham
2012-01-19 18:20 ` Tony Lindgren
2012-01-19 18:38 ` Thomas Abraham
2012-01-20 10:05 ` Tony Lindgren
2012-01-20 16:17 ` Thomas Abraham
2012-01-20 17:53 ` Tony Lindgren
2012-01-21 1:38 ` Thomas Abraham
2012-01-20 21:15 ` Stephen Warren
2012-01-20 21:11 ` Stephen Warren
2012-01-21 1:27 ` Thomas Abraham
2012-01-23 22:43 ` Stephen Warren
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).