| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20120114070858.GG1810@S2101-09.ap.freescale.net>
Date: Sat, 14 Jan 2012 15:09:00 +0800
From: Shawn Guo <shawn.guo@...aro.org>
To: Stephen Warren <swarren@...dia.com>
Cc: Dong Aisheng-B29396 <B29396@...escale.com>,
"linus.walleij@...ricsson.com" <linus.walleij@...ricsson.com>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"rob.herring@...xeda.com" <rob.herring@...xeda.com>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"cjb@...top.org" <cjb@...top.org>,
"Simon Glass (sjg@...omium.org)" <sjg@...omium.org>,
Dong Aisheng <dongas86@...il.com>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"devicetree-discuss@...ts.ozlabs.org"
<devicetree-discuss@...ts.ozlabs.org>
Subject: Re: Pinmux bindings proposal
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@...00000 {
> 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@...00200 {
> compatible = "nvidia,tegra20-sdhci";
> reg = <0xc8000200 0x200>;
> interrupts = <0 15 0x04>;
> };
> };
>
> tegra-harmony.dts:
>
> /{
> sdhci@...00200 {
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists