Pinmux bindings proposal V2

Pinmux bindings proposal V2

[Stephen Warren]

Here's V2 of my pinmux binding proposal, after incorporating some feedback
from Shawn Guo and Dong Aisheng. Main changes:

1) Require the "pin config" nodes the be children of the pinmux controller
   node, rather than allowing them to alternatively be children of the
   device node that's using them. This allows removal of the pin controller
   phandle as the first entry in each specifier.

2) Add explicit #pinmux-cells and #pinconfig-cells properties to the pin
   controller, so that pin controllers can define the layout of their mux
   and configuration specifiers.

3) Don't document a "1:1" model; only document a "1:n" model, whereby each
   named state for a device node can refer to multiple pin configuration
   nodes. This allows:

   a) A device to use multiple configuration nodes from a single pin
      controller. For example, one may specify a very common subset of
      the configuration for a particular device, and another may specify
      a few board-specific tweaks on top of that.

   b) A single device to specify configurations for multiple pin
      controllers.

TODO: I should write a real binding document for this, rather than simply
providing a commented example.

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:

/ {
        /*
         * The SoC .dtsi file will define the basic properties of each HW
         * module; compatible, reg, interrupts, ... There's no change here
         * relative to normal practice.
         */
        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 */
                /*
                 * Common pin control bindings specify that these properties
                 * must exist in a pin controller. Each individual pin
                 * controller's bindings specify the values.
                 */
                #pinmux-cells = <2>;
                #pinconfig-cells = <3>;
        };

        sdhci@c8000200 {
                compatible = "nvidia,tegra20-sdhci";
                reg = <0xc8000200 0x200>;
                interrupts = <0 15 0x04>;
        };
};

tegra20.dtis or tegra-harmony.dts:

/{
        /*
         * The pin controller node will contain child nodes that define
         * various subsets of the overall pin state.
         *
         * These nodes may be defined in the SoC .dtsi file if the vendor
         * wants to provide a set of common configurations for customers
         * or users to pick from.
         *
         * These nodes may be defined in the board .dts file too; e.g. if
         * a particular board uses a configuration that isn't in the set
         * of provided common options, or the SoC vendor simply chose not
         * to provide a set of common options.
         */
        &tegra_pmx {
                /*
                 * There are (may be) many of these pin configuration child
                 * nodes. Each is referenced by phandle from device nodes.
                 *
                 * Below I've shown an example of an SDHCI controller. There
                 * is a set of common settings in node pmx_sdhci. The slight
                 * differences between active and standby states are in nodes
                 * pmc_sdhci_active and pmx_sdhci_suspend.
                 *
                 * Note though the the 3-node split is just an example; you
                 * could easily have a single node for devices that don't
                 * need separate active/suspend state, or many more nodes
                 * for a device with a more complex set of shared settings
                 * or that uses more states.
                 */
                pmx_sdhci: pinconfig-sdhci {
                        /*
                         * The mux property is a list of muxable entities
                         * and the mux function to select for it. The number
                         * of cells in each entry is the pin controller's
                         * #pinmux-cells property. The pin controller's
                         * binding defines what the cells mean. The pinctrl
                         * driver is responsible for mapping this data to
                         * the (group, function) pair required to fill in
                         * the pinctrl subsystem's pinmux mapping table.
                         */
                        mux =
                                <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
                                <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
                        /*
                         * The config property is a list of muxable entities
                         * and individual configuration setting. The number
                         * of cells in each entry is the pin controller's
                         * #pinconfig-cells property. The pin controller's
                         * binding defines what the cells mean. The pinctrl
                         * driver is responsible for mapping this data to
                         * the (group, config) pair required to fill in
                         * the pinctrl subsystem's pin configuration table.
                         */
                        config =
                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
                };
                pmx_sdhci_active: pinconfig-sdhci-active {
                        /*
                         * In each of these nodes, both the mux and config
                         * properties are optional. This node represents the
                         * additions to pmx_sdhci that are specific to an
                         * active state. In this case, only pin configuration
                         * settings are different.
                         */
                        config =
                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
                };
                pmx_sdhci_standby: pinconfig-sdhci-standby {
                        config =
                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
                };
                /*
                 * ... continuing from the comment in pmx_sdhci_active:
                 *
                 * However, one could imagine representing GPIO as a regular
                 * pinmux function, and have a board .dts file create an
                 * extra node to represent that, and hence containing just
                 * a mux property:
                 */
                pmx_sdhci_harmony: pinconfig-sdhci-harmony {
                        mux = <TEGRA_PMX_PIN_GPIO_PA1 TEGRA_PMX_MUX_GPIO>;
                };
        };
};

tegra-harmony.dts:

/{
        sdhci@c8000200 {
                /*
                 * Each device node contains a few properties to describe
                 * the named pinmux states that are available to it.
                 *
                 * The binding for the device node specifies the state names
                 * that must be described; common examples such as "default"
                 * or "active" and "suspend" may be universal, but the IO
                 * protocol that a device supports may demand that more
                 * states be defined, such as "active-12mhz", "active-50mhz",
                 * "suspend". Drivers request these named states e.g. using
                 * pinctrl's pinmux_get("state_name") API.
                 *
                 * pinctrl-names lists the available state names.
                 *
                 * Unlike the common clock binding, I assume here that states
                 * are always requested by name. If we don't like that idea,
                 * we'd could make this property optional, and add a new API
                 * pinmux_get(state_id) to the pinctrl subsystem.
                 */
                pinctrl-names = "active", "suspend";
                /*
                 * pinctrl-entries contains a single cell for each state name
                 * in pinctrl-names. It defines how many entries are present
                 * in property "pinctrl" for the given named state.
                 *
                 * Here, state "active" covers 2 entries (0 and 1) in pinctrl,
                 * and state "suspend" covers 2 more entries (2 and 3) in
                 * pinctrl.
                 */
                pinctrl-entries = <2 2>;
                /*
                 * pinctrl contains a list of phandles. Each refers to one of
                 * the pin config nodes within the pin controller. So, to set
                 * up state "active", nodes pmx_sdhci and pmx_sdhci_active
                 * must both be programmed. To set up state "suspend", nodes
                 * pmx_sdhci and pmx_sdhci_standby must be programmed.
                 */
                pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
                          <&pmx_sdhci> <&pmx_sdhci_standby>;
        };
};

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

Hi,

This mostly looks pretty good to me, few more comments below.

* Stephen Warren <swarren@nvidia.com> [120120 13:51]:
> Here's V2 of my pinmux binding proposal, after incorporating some feedback
> from Shawn Guo and Dong Aisheng. Main changes:
> 
> 1) Require the "pin config" nodes the be children of the pinmux controller
>    node, rather than allowing them to alternatively be children of the
>    device node that's using them. This allows removal of the pin controller
>    phandle as the first entry in each specifier.

If you really want to leave out the phandle for each mux entry, this would
require supporting multiple "pin config" entries in a device driver node.

That's because otherwise supporting pins from multiple pinmux controller
instances would not work. Note that we already have two pin controller
instances starting with omap4.

...
 
>                 pmx_sdhci_active: pinconfig-sdhci-active {
>                         /*
>                          * In each of these nodes, both the mux and config
>                          * properties are optional. This node represents the
>                          * additions to pmx_sdhci that are specific to an
>                          * active state. In this case, only pin configuration
>                          * settings are different.
>                          */
>                         config =
>                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
>                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
>                 };
>                 pmx_sdhci_standby: pinconfig-sdhci-standby {
>                         config =
>                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
>                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
>                 };

After thinking about this a bit more, I'm now thinking that we should
probably only describe the active state in the device tree to keep things
simple.

Anything PM related could be initialized later on during the boot by the
device driver, or even from userspace using /lib/firmware or /sys entries.
This would cut down the device tree bloat quite a bit.

Setting the initial state from device tree makes sense, but I'm afraid
the standby and off states will require driver interaction depending on
how the user wants to configure the system.

For example, allowing a device to wake up a system might be user
configurable option, such as "Wake up on tapping the touchscreen".

Then I'd rather not use "config" at all, and just put the value for
initial state mux register(s) in the mux to avoid repeating the same data
multiple time. Looks like with the mux + config/mux-config option you
need to duplicate at least TEGRA_PMX_PG_DTA.

BTW, maybe the name "config" should probably be "mux-config" instead?

>                 /*
>                  * Each device node contains a few properties to describe
>                  * the named pinmux states that are available to it.
>                  *
>                  * The binding for the device node specifies the state names
>                  * that must be described; common examples such as "default"
>                  * or "active" and "suspend" may be universal, but the IO
>                  * protocol that a device supports may demand that more
>                  * states be defined, such as "active-12mhz", "active-50mhz",
>                  * "suspend". Drivers request these named states e.g. using
>                  * pinctrl's pinmux_get("state_name") API.
>                  *
>                  * pinctrl-names lists the available state names.
>                  *
>                  * Unlike the common clock binding, I assume here that states
>                  * are always requested by name. If we don't like that idea,
>                  * we'd could make this property optional, and add a new API
>                  * pinmux_get(state_id) to the pinctrl subsystem.
>                  */
>                 pinctrl-names = "active", "suspend";

If we only allow describing the initial state in device tree, then we can
leave out pinctrl-names, and assume the setting is always the initial bootup
state desired.

So to summarize: I suggest we'll just stick to basics to get the system
booting and devices working using device tree. In most cases the device
drivers should be able to configure the suspend and off states in a generic
way using pinctrl API. Everything else, like debugging, we can probably
do with userspace tools.

This would mean just using a minimal subset of your binding, probably
very close to what you originally suggested.

Regards,

Tony

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Monday, January 23, 2012 2:01 PM:
> This mostly looks pretty good to me, few more comments below.
> 
> * Stephen Warren <swarren@nvidia.com> [120120 13:51]:
> > Here's V2 of my pinmux binding proposal, after incorporating some feedback
> > from Shawn Guo and Dong Aisheng. Main changes:
> >
> > 1) Require the "pin config" nodes the be children of the pinmux controller
> >    node, rather than allowing them to alternatively be children of the
> >    device node that's using them. This allows removal of the pin controller
> >    phandle as the first entry in each specifier.
> 
> If you really want to leave out the phandle for each mux entry, this would
> require supporting multiple "pin config" entries in a device driver node.

Yes. And that's already supported using the pinctrl-entries property;
you can reference n nodes at once for each named state in pinctrl-names,
and each of those nodes could easily be from a different pin controller.

> >                 pmx_sdhci_active: pinconfig-sdhci-active {
> >                         /*
> >                          * In each of these nodes, both the mux and config
> >                          * properties are optional. This node represents the
> >                          * additions to pmx_sdhci that are specific to an
> >                          * active state. In this case, only pin configuration
> >                          * settings are different.
> >                          */
> >                         config =
> >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
> >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
> >                 };
> >                 pmx_sdhci_standby: pinconfig-sdhci-standby {
> >                         config =
> >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> >                 };
> 
> After thinking about this a bit more, I'm now thinking that we should
> probably only describe the active state in the device tree to keep things
> simple.
> 
> Anything PM related could be initialized later on during the boot by the
> device driver, or even from userspace using /lib/firmware or /sys entries.
> This would cut down the device tree bloat quite a bit.

I think it makes sense to describe everything in one place. It's much
easier to ensure everything is consistent and correct if you don't have
to cross-check two sources of data for the same thing.

> Setting the initial state from device tree makes sense, but I'm afraid
> the standby and off states will require driver interaction depending on
> how the user wants to configure the system.
> 
> For example, allowing a device to wake up a system might be user
> configurable option, such as "Wake up on tapping the touchscreen".

That can be supported pretty easily.

The driver for a simple device can require two named states: "active"
and "standby". In your more complex example, the driver can require
three named states: "active", "standby", "standby-wake". During suspend,
it'll simply select the appropriate of the latter two states based on
user request.

You can even retrofit this capability to a previously simple driver that
Originally did:

suspend() {
    pinconf_select(dev, "suspend");
}

To make it do the following instead:

suspend() {
    if (wake)
        state = "suspend-wake";
    else
        state = "suspend-nowake";
    err = pinconf_select(dev, state);
    if (err < 0)
        pinconf_select(dev, "suspend";
}

... which should allow old device trees to work fine, but new device
trees to specify a different if they need.

> Then I'd rather not use "config" at all, and just put the value for
> initial state mux register(s) in the mux to avoid repeating the same data
> multiple time. Looks like with the mux + config/mux-config option you
> need to duplicate at least TEGRA_PMX_PG_DTA.
>
> BTW, maybe the name "config" should probably be "mux-config" instead?

Mux and config are two unrelated things though. Mux is the function
(signal, alternate function, mission mode, ...) that is selected for
the pin/group in question. Config is anything else, but explicitly not
mux, such as pull-up, pull-down, tri-state, drive-strength, etc.

> >                 /*
> >                  * Each device node contains a few properties to describe
> >                  * the named pinmux states that are available to it.
> >                  *
> >                  * The binding for the device node specifies the state names
> >                  * that must be described; common examples such as "default"
> >                  * or "active" and "suspend" may be universal, but the IO
> >                  * protocol that a device supports may demand that more
> >                  * states be defined, such as "active-12mhz", "active-50mhz",
> >                  * "suspend". Drivers request these named states e.g. using
> >                  * pinctrl's pinmux_get("state_name") API.
> >                  *
> >                  * pinctrl-names lists the available state names.
> >                  *
> >                  * Unlike the common clock binding, I assume here that states
> >                  * are always requested by name. If we don't like that idea,
> >                  * we'd could make this property optional, and add a new API
> >                  * pinmux_get(state_id) to the pinctrl subsystem.
> >                  */
> >                 pinctrl-names = "active", "suspend";
> 
> If we only allow describing the initial state in device tree, then we can
> leave out pinctrl-names, and assume the setting is always the initial bootup
> state desired.

That's true, but people have already discussed the usefulness of having
multiple named states, e.g. an SDHCI controller that needs different pin
configuration (perhaps drive strength or slew rate?) depending on the
clock rate of the SD interface itself.

> So to summarize: I suggest we'll just stick to basics to get the system
> booting and devices working using device tree. In most cases the device
> drivers should be able to configure the suspend and off states in a generic
> way using pinctrl API. Everything else, like debugging, we can probably
> do with userspace tools.

But how will they configure suspend/off states in a generic way using the
pinctrl API if the pinctrl API has no way of knowing how to configure the
pins in suspend...

> This would mean just using a minimal subset of your binding, probably
> very close to what you originally suggested.

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120123 14:37]:
> Tony Lindgren wrote at Monday, January 23, 2012 2:01 PM:
> > This mostly looks pretty good to me, few more comments below.
> > 
> > * Stephen Warren <swarren@nvidia.com> [120120 13:51]:
> > > Here's V2 of my pinmux binding proposal, after incorporating some feedback
> > > from Shawn Guo and Dong Aisheng. Main changes:
> > >
> > > 1) Require the "pin config" nodes the be children of the pinmux controller
> > >    node, rather than allowing them to alternatively be children of the
> > >    device node that's using them. This allows removal of the pin controller
> > >    phandle as the first entry in each specifier.
> > 
> > If you really want to leave out the phandle for each mux entry, this would
> > require supporting multiple "pin config" entries in a device driver node.
> 
> Yes. And that's already supported using the pinctrl-entries property;
> you can reference n nodes at once for each named state in pinctrl-names,
> and each of those nodes could easily be from a different pin controller.

OK, that sounds doable.
 
> > >                 pmx_sdhci_active: pinconfig-sdhci-active {
> > >                         /*
> > >                          * In each of these nodes, both the mux and config
> > >                          * properties are optional. This node represents the
> > >                          * additions to pmx_sdhci that are specific to an
> > >                          * active state. In this case, only pin configuration
> > >                          * settings are different.
> > >                          */
> > >                         config =
> > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
> > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
> > >                 };
> > >                 pmx_sdhci_standby: pinconfig-sdhci-standby {
> > >                         config =
> > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> > >                 };
> > 
> > After thinking about this a bit more, I'm now thinking that we should
> > probably only describe the active state in the device tree to keep things
> > simple.
> > 
> > Anything PM related could be initialized later on during the boot by the
> > device driver, or even from userspace using /lib/firmware or /sys entries.
> > This would cut down the device tree bloat quite a bit.
> 
> I think it makes sense to describe everything in one place. It's much
> easier to ensure everything is consistent and correct if you don't have
> to cross-check two sources of data for the same thing.

But we end up repeating the same bits over and over again making the device
tree bloated.

I'm suspecting that the initial state can be used to set the PM states for
pins. Probably most pins can have the PM configuration set from the start.

If dynamic muxing is needed, it should be for a very limited set of pins,
and that can be done in the drivers.
 
> > Setting the initial state from device tree makes sense, but I'm afraid
> > the standby and off states will require driver interaction depending on
> > how the user wants to configure the system.
> > 
> > For example, allowing a device to wake up a system might be user
> > configurable option, such as "Wake up on tapping the touchscreen".
> 
> That can be supported pretty easily.
> 
> The driver for a simple device can require two named states: "active"
> and "standby". In your more complex example, the driver can require
> three named states: "active", "standby", "standby-wake". During suspend,
> it'll simply select the appropriate of the latter two states based on
> user request.
> 
> You can even retrofit this capability to a previously simple driver that
> Originally did:
> 
> suspend() {
>     pinconf_select(dev, "suspend");
> }
> 
> To make it do the following instead:
> 
> suspend() {
>     if (wake)
>         state = "suspend-wake";
>     else
>         state = "suspend-nowake";
>     err = pinconf_select(dev, state);
>     if (err < 0)
>         pinconf_select(dev, "suspend";
> }
> 
> ... which should allow old device trees to work fine, but new device
> trees to specify a different if they need.

You can do just the same from a driver with something like:

	...
	res = pin_config_set_for_pin(pctldev, pin, PIN_STATE_SUSPEND);
	...

As long as the pinconf/pinmux driver knows how to set PIN_STATE_SUSPEND
for one entry. Note that I've avoided using pin_config_set here because
there really should not be any string parsing needed except during driver
init for doing things like this during runtime..
 
> > Then I'd rather not use "config" at all, and just put the value for
> > initial state mux register(s) in the mux to avoid repeating the same data
> > multiple time. Looks like with the mux + config/mux-config option you
> > need to duplicate at least TEGRA_PMX_PG_DTA.
> >
> > BTW, maybe the name "config" should probably be "mux-config" instead?
> 
> Mux and config are two unrelated things though. Mux is the function
> (signal, alternate function, mission mode, ...) that is selected for
> the pin/group in question. Config is anything else, but explicitly not
> mux, such as pull-up, pull-down, tri-state, drive-strength, etc.

Right, but how many registers are you really writing to? In the cases
I've seen it's mostly just one register. In some cases it can be three
tree registers per mux, but only one would change during runtime
potentially.

> > >                 /*
> > >                  * Each device node contains a few properties to describe
> > >                  * the named pinmux states that are available to it.
> > >                  *
> > >                  * The binding for the device node specifies the state names
> > >                  * that must be described; common examples such as "default"
> > >                  * or "active" and "suspend" may be universal, but the IO
> > >                  * protocol that a device supports may demand that more
> > >                  * states be defined, such as "active-12mhz", "active-50mhz",
> > >                  * "suspend". Drivers request these named states e.g. using
> > >                  * pinctrl's pinmux_get("state_name") API.
> > >                  *
> > >                  * pinctrl-names lists the available state names.
> > >                  *
> > >                  * Unlike the common clock binding, I assume here that states
> > >                  * are always requested by name. If we don't like that idea,
> > >                  * we'd could make this property optional, and add a new API
> > >                  * pinmux_get(state_id) to the pinctrl subsystem.
> > >                  */
> > >                 pinctrl-names = "active", "suspend";
> > 
> > If we only allow describing the initial state in device tree, then we can
> > leave out pinctrl-names, and assume the setting is always the initial bootup
> > state desired.
> 
> That's true, but people have already discussed the usefulness of having
> multiple named states, e.g. an SDHCI controller that needs different pin
> configuration (perhaps drive strength or slew rate?) depending on the
> clock rate of the SD interface itself.

Yes but those too should be doable from the device drivers using pin ctrl
framework calls. Usually it's just few pins that needs to change dynamically.
 
> > So to summarize: I suggest we'll just stick to basics to get the system
> > booting and devices working using device tree. In most cases the device
> > drivers should be able to configure the suspend and off states in a generic
> > way using pinctrl API. Everything else, like debugging, we can probably
> > do with userspace tools.
> 
> But how will they configure suspend/off states in a generic way using the
> pinctrl API if the pinctrl API has no way of knowing how to configure the
> pins in suspend...

We can have a set of defined states that the pinmux/pinconf drivers can
implement. Then have functions to get/set a mux or a mux group. And then
the triggering could be hooked up into pm_runtime calls just like clock
framework calls.

Regards,

Tony

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Monday, January 23, 2012 6:21 PM:
> * Stephen Warren <swarren@nvidia.com> [120123 14:37]:
> > Tony Lindgren wrote at Monday, January 23, 2012 2:01 PM:
> > > This mostly looks pretty good to me, few more comments below.
...
> > > >                 pmx_sdhci_active: pinconfig-sdhci-active {
> > > >                         /*
> > > >                          * In each of these nodes, both the mux and config
> > > >                          * properties are optional. This node represents the
> > > >                          * additions to pmx_sdhci that are specific to an
> > > >                          * active state. In this case, only pin configuration
> > > >                          * settings are different.
> > > >                          */
> > > >                         config =
> > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
> > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
> > > >                 };
> > > >                 pmx_sdhci_standby: pinconfig-sdhci-standby {
> > > >                         config =
> > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> > > >                 };
> > >
> > > After thinking about this a bit more, I'm now thinking that we should
> > > probably only describe the active state in the device tree to keep things
> > > simple.
> > >
> > > Anything PM related could be initialized later on during the boot by the
> > > device driver, or even from userspace using /lib/firmware or /sys entries.
> > > This would cut down the device tree bloat quite a bit.
> >
> > I think it makes sense to describe everything in one place. It's much
> > easier to ensure everything is consistent and correct if you don't have
> > to cross-check two sources of data for the same thing.
> 
> But we end up repeating the same bits over and over again making the device
> tree bloated.

What exactly gets repeated; can you please show an example?

Given that each named state can reference n nodes defining part of that
state's pinmux configuration, and some of those nodes can affect the same
pin controller, you can easily split the overall configuration into
"stuff that's common for all states for this device" and "stuff that's
common between these n states of the device but not these m others" and
"stuff that's unique to this 1 state", and hence avoid repeating any
identical information.

> I'm suspecting that the initial state can be used to set the PM states for
> pins. Probably most pins can have the PM configuration set from the start.

Can you please explain more?

> If dynamic muxing is needed, it should be for a very limited set of pins,
> and that can be done in the drivers.

(and later, saying the same thing):
> > > If we only allow describing the initial state in device tree, then we can
> > > leave out pinctrl-names, and assume the setting is always the initial bootup
> > > state desired.
> >
> > That's true, but people have already discussed the usefulness of having
> > multiple named states, e.g. an SDHCI controller that needs different pin
> > configuration (perhaps drive strength or slew rate?) depending on the
> > clock rate of the SD interface itself.
> 
> Yes but those too should be doable from the device drivers using pin ctrl
> framework calls. Usually it's just few pins that needs to change dynamically.

Do you mean drivers explicitly requesting that a named pin/group have
some specific parameter set to some specific value?

That would make the peripheral driver have to know specific details of
each SoC's pin controller. This isn't an issue for one-off IP, but if
you've got some IP block that gets re-used in 2 or 3 different SoCs each
with a SoC-vendor-specific pin controller, then the driver is going to
have to know those same details 3 times over, which is pretty much exactly
what we're trying to avoid by abstracting the details out of the drivers
and into a common binding that's handled by the pinctrl subsystem.

> > > Setting the initial state from device tree makes sense, but I'm afraid
> > > the standby and off states will require driver interaction depending on
> > > how the user wants to configure the system.
> > >
> > > For example, allowing a device to wake up a system might be user
> > > configurable option, such as "Wake up on tapping the touchscreen".
> >
> > That can be supported pretty easily.
> >
> > The driver for a simple device can require two named states: "active"
> > and "standby". In your more complex example, the driver can require
> > three named states: "active", "standby", "standby-wake". During suspend,
> > it'll simply select the appropriate of the latter two states based on
> > user request.
> >
> > You can even retrofit this capability to a previously simple driver that
> > Originally did:
> >
> > suspend() {
> >     pinconf_select(dev, "suspend");
> > }
> >
> > To make it do the following instead:
> >
> > suspend() {
> >     if (wake)
> >         state = "suspend-wake";
> >     else
> >         state = "suspend-nowake";
> >     err = pinconf_select(dev, state);
> >     if (err < 0)
> >         pinconf_select(dev, "suspend";
> > }
> >
> > ... which should allow old device trees to work fine, but new device
> > trees to specify a different if they need.
> 
> You can do just the same from a driver with something like:
> 
> 	...
> 	res = pin_config_set_for_pin(pctldev, pin, PIN_STATE_SUSPEND);
> 	...
> 
> As long as the pinconf/pinmux driver knows how to set PIN_STATE_SUSPEND
> for one entry. Note that I've avoided using pin_config_set here because
> there really should not be any string parsing needed except during driver
> init for doing things like this during runtime..

In order to implement pin_config_set_for_pin(), you still need some
source of data defining what PIN_STATE_SUSPEND means. I has been suggested
to defer that knowledge into each individual pin controller driver, but
since this is probably board-specific in general, that means that each
pin controller driver then needs to parse the data from device tree, so
we end up with exactly the same data in device tree, just represented in
a different way for each SoC vendor. To me, that's exactly what the pinctrl
subsystem and common pinctrl DT bindings are trying to avoid.

> > > Then I'd rather not use "config" at all, and just put the value for
> > > initial state mux register(s) in the mux to avoid repeating the same data
> > > multiple time. Looks like with the mux + config/mux-config option you
> > > need to duplicate at least TEGRA_PMX_PG_DTA.
> > >
> > > BTW, maybe the name "config" should probably be "mux-config" instead?
> >
> > Mux and config are two unrelated things though. Mux is the function
> > (signal, alternate function, mission mode, ...) that is selected for
> > the pin/group in question. Config is anything else, but explicitly not
> > mux, such as pull-up, pull-down, tri-state, drive-strength, etc.
> 
> Right, but how many registers are you really writing to? In the cases
> I've seen it's mostly just one register. In some cases it can be three
> tree registers per mux, but only one would change during runtime
> potentially.

For each pin group on Tegra20, there are 1, 2, or 3 registers that affect
it. In most cases, each register contains fields that affect multiple pin
groups. On Tegra30, there is 1:1 mapping between registers and pin groups.

> > > So to summarize: I suggest we'll just stick to basics to get the system
> > > booting and devices working using device tree. In most cases the device
> > > drivers should be able to configure the suspend and off states in a generic
> > > way using pinctrl API. Everything else, like debugging, we can probably
> > > do with userspace tools.
> >
> > But how will they configure suspend/off states in a generic way using the
> > pinctrl API if the pinctrl API has no way of knowing how to configure the
> > pins in suspend...
> 
> We can have a set of defined states that the pinmux/pinconf drivers can
> implement. Then have functions to get/set a mux or a mux group. And then
> the triggering could be hooked up into pm_runtime calls just like clock
> framework calls.

Does runtime PM allow drivers to request arbitrary states, i.e. make up
device-specific state names? I must admit I'm not at all familiar with it,
but I'd expect it supports "active" and "inactive", whereas to support the
SDHCI controller example that's been mentioned, it'd need to support
"active 24MHz", "active 40MHz", "inactive" (the exact clock rates I use in
this example may be wrong).

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120124 14:02]:
> Tony Lindgren wrote at Monday, January 23, 2012 6:21 PM:
> > * Stephen Warren <swarren@nvidia.com> [120123 14:37]:
> > > Tony Lindgren wrote at Monday, January 23, 2012 2:01 PM:
> > > > This mostly looks pretty good to me, few more comments below.
> ...
> > > > >                 pmx_sdhci_active: pinconfig-sdhci-active {
> > > > >                         /*
> > > > >                          * In each of these nodes, both the mux and config
> > > > >                          * properties are optional. This node represents the
> > > > >                          * additions to pmx_sdhci that are specific to an
> > > > >                          * active state. In this case, only pin configuration
> > > > >                          * settings are different.
> > > > >                          */
> > > > >                         config =
> > > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
> > > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
> > > > >                 };
> > > > >                 pmx_sdhci_standby: pinconfig-sdhci-standby {
> > > > >                         config =
> > > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> > > > >                 };
> > > >
> > > > After thinking about this a bit more, I'm now thinking that we should
> > > > probably only describe the active state in the device tree to keep things
> > > > simple.
> > > >
> > > > Anything PM related could be initialized later on during the boot by the
> > > > device driver, or even from userspace using /lib/firmware or /sys entries.
> > > > This would cut down the device tree bloat quite a bit.
> > >
> > > I think it makes sense to describe everything in one place. It's much
> > > easier to ensure everything is consistent and correct if you don't have
> > > to cross-check two sources of data for the same thing.
> > 
> > But we end up repeating the same bits over and over again making the device
> > tree bloated.
> 
> What exactly gets repeated; can you please show an example?

Well in your example, you have TEGRA_PMX_PG_DTA and TEGRA_PMX_PG_DTD
repeated. It seems you're trying to list slightly different register
values for the the pincontrol modes with pmx_sdhci_active and
pmx_sdhci_standby.

IMHO already that list of mux states seems limited as you may need to
list other driver states too like pmx_sdhci_standby_wake_up_disabled and
pmx_sdhci_standby_off. So it starts to sound a bit like PM policy instead
of letting the driver know what hardware is on the system.

Having multiple config options also means that you end up setting
#pinconfig-cell to something unnecessarily large for all mux nodes even
if you only need the alternative values for one pin on the system.

> Given that each named state can reference n nodes defining part of that
> state's pinmux configuration, and some of those nodes can affect the same
> pin controller, you can easily split the overall configuration into
> "stuff that's common for all states for this device" and "stuff that's
> common between these n states of the device but not these m others" and
> "stuff that's unique to this 1 state", and hence avoid repeating any
> identical information.
> 
> > I'm suspecting that the initial state can be used to set the PM states for
> > pins. Probably most pins can have the PM configuration set from the start.
> 
> Can you please explain more?

For the cases I know the default values work for 95% - 100% of the pins in
the system for PM also. Only few pins on some systems need to be dynamically
remuxed during the runtime. The rest of the pins can be discarded after init.

I'm also suggesting that that the bits that may need to change for dynamic
remuxing for PM are probably not board specific, they're typically things
like change something into GPIO input mode or pull something down to avoid
lines from floating for an external device. Things that the device driver
using the pins should be able to communicate to the pinconf/pinmux driver
via pin_config_* calls or runtime PM in a Linux generic way.

> > If dynamic muxing is needed, it should be for a very limited set of pins,
> > and that can be done in the drivers.
> 
> (and later, saying the same thing):
> > > > If we only allow describing the initial state in device tree, then we can
> > > > leave out pinctrl-names, and assume the setting is always the initial bootup
> > > > state desired.
> > >
> > > That's true, but people have already discussed the usefulness of having
> > > multiple named states, e.g. an SDHCI controller that needs different pin
> > > configuration (perhaps drive strength or slew rate?) depending on the
> > > clock rate of the SD interface itself.
> > 
> > Yes but those too should be doable from the device drivers using pin ctrl
> > framework calls. Usually it's just few pins that needs to change dynamically.
> 
> Do you mean drivers explicitly requesting that a named pin/group have
> some specific parameter set to some specific value?

Yes request pin/group directly or even better, just hook it to runtime PM
which allows implementing platform specific calls.
 
> That would make the peripheral driver have to know specific details of
> each SoC's pin controller. This isn't an issue for one-off IP, but if
> you've got some IP block that gets re-used in 2 or 3 different SoCs each
> with a SoC-vendor-specific pin controller, then the driver is going to
> have to know those same details 3 times over, which is pretty much exactly
> what we're trying to avoid by abstracting the details out of the drivers
> and into a common binding that's handled by the pinctrl subsystem.
> 
> > > > Setting the initial state from device tree makes sense, but I'm afraid
> > > > the standby and off states will require driver interaction depending on
> > > > how the user wants to configure the system.
> > > >
> > > > For example, allowing a device to wake up a system might be user
> > > > configurable option, such as "Wake up on tapping the touchscreen".
> > >
> > > That can be supported pretty easily.
> > >
> > > The driver for a simple device can require two named states: "active"
> > > and "standby". In your more complex example, the driver can require
> > > three named states: "active", "standby", "standby-wake". During suspend,
> > > it'll simply select the appropriate of the latter two states based on
> > > user request.
> > >
> > > You can even retrofit this capability to a previously simple driver that
> > > Originally did:
> > >
> > > suspend() {
> > >     pinconf_select(dev, "suspend");
> > > }
> > >
> > > To make it do the following instead:
> > >
> > > suspend() {
> > >     if (wake)
> > >         state = "suspend-wake";
> > >     else
> > >         state = "suspend-nowake";
> > >     err = pinconf_select(dev, state);
> > >     if (err < 0)
> > >         pinconf_select(dev, "suspend";
> > > }
> > >
> > > ... which should allow old device trees to work fine, but new device
> > > trees to specify a different if they need.
> > 
> > You can do just the same from a driver with something like:
> > 
> > 	...
> > 	res = pin_config_set_for_pin(pctldev, pin, PIN_STATE_SUSPEND);
> > 	...
> > 
> > As long as the pinconf/pinmux driver knows how to set PIN_STATE_SUSPEND
> > for one entry. Note that I've avoided using pin_config_set here because
> > there really should not be any string parsing needed except during driver
> > init for doing things like this during runtime..
> 
> In order to implement pin_config_set_for_pin(), you still need some
> source of data defining what PIN_STATE_SUSPEND means. I has been suggested
> to defer that knowledge into each individual pin controller driver, but
> since this is probably board-specific in general, that means that each
> pin controller driver then needs to parse the data from device tree, so
> we end up with exactly the same data in device tree, just represented in
> a different way for each SoC vendor. To me, that's exactly what the pinctrl
> subsystem and common pinctrl DT bindings are trying to avoid.

It's usually the information about which pins to use and pull values that are
board specific. They can depend on external pull resistors on the board etc.

For the PM related things the predefined states can be Linux generic and
implemented by the pinconf/pinmux drivers easily.
 
> > > > Then I'd rather not use "config" at all, and just put the value for
> > > > initial state mux register(s) in the mux to avoid repeating the same data
> > > > multiple time. Looks like with the mux + config/mux-config option you
> > > > need to duplicate at least TEGRA_PMX_PG_DTA.
> > > >
> > > > BTW, maybe the name "config" should probably be "mux-config" instead?
> > >
> > > Mux and config are two unrelated things though. Mux is the function
> > > (signal, alternate function, mission mode, ...) that is selected for
> > > the pin/group in question. Config is anything else, but explicitly not
> > > mux, such as pull-up, pull-down, tri-state, drive-strength, etc.
> > 
> > Right, but how many registers are you really writing to? In the cases
> > I've seen it's mostly just one register. In some cases it can be three
> > tree registers per mux, but only one would change during runtime
> > potentially.
> 
> For each pin group on Tegra20, there are 1, 2, or 3 registers that affect
> it. In most cases, each register contains fields that affect multiple pin
> groups. On Tegra30, there is 1:1 mapping between registers and pin groups.

I see, thanks for clarifying that.
 
> > > > So to summarize: I suggest we'll just stick to basics to get the system
> > > > booting and devices working using device tree. In most cases the device
> > > > drivers should be able to configure the suspend and off states in a generic
> > > > way using pinctrl API. Everything else, like debugging, we can probably
> > > > do with userspace tools.
> > >
> > > But how will they configure suspend/off states in a generic way using the
> > > pinctrl API if the pinctrl API has no way of knowing how to configure the
> > > pins in suspend...
> > 
> > We can have a set of defined states that the pinmux/pinconf drivers can
> > implement. Then have functions to get/set a mux or a mux group. And then
> > the triggering could be hooked up into pm_runtime calls just like clock
> > framework calls.
> 
> Does runtime PM allow drivers to request arbitrary states, i.e. make up
> device-specific state names? I must admit I'm not at all familiar with it,
> but I'd expect it supports "active" and "inactive", whereas to support the
> SDHCI controller example that's been mentioned, it'd need to support
> "active 24MHz", "active 40MHz", "inactive" (the exact clock rates I use in
> this example may be wrong).

Hmm, I don't think you need device-specifc state names there. It sounds
like you would want to map the device-specific pin states in your
pinconf/pinmux driver. Then the device driver using the pins can have pin
state changes triggered based on pm_runtime_* calls.

The pm_runtime-* calls will find their way to clock framework and mux code
if the bus layer is implemented that way. For the various clock rates it
sounds like you should probably add few clock nodes to the clock framwork?

Maybe the driver itself should select between 24MHz and 40MHz clock using
clk_set_rate, then have pm_runtime_get/put toggle the clock on and off.
Then you could also hook the active vs inactive state change to happen
based on selected pm_runtime_* calls in the bus layer as it should know
what the next state transition is. So really no code needed in the SDHCI
driver except to add pm_runtime_get/put calls for the remuxing.

Regards,

Tony

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Tuesday, January 24, 2012 5:04 PM:
> * Stephen Warren <swarren@nvidia.com> [120124 14:02]:
> > Tony Lindgren wrote at Monday, January 23, 2012 6:21 PM:
> > > * Stephen Warren <swarren@nvidia.com> [120123 14:37]:
> > > > Tony Lindgren wrote at Monday, January 23, 2012 2:01 PM:
> > > > > This mostly looks pretty good to me, few more comments below.
> > ...
> > > > > >                 pmx_sdhci_active: pinconfig-sdhci-active {
> > > > > >                         /*
> > > > > >                          * In each of these nodes, both the mux and config
> > > > > >                          * properties are optional. This node represents the
> > > > > >                          * additions to pmx_sdhci that are specific to an
> > > > > >                          * active state. In this case, only pin configuration
> > > > > >                          * settings are different.
> > > > > >                          */
> > > > > >                         config =
> > > > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
> > > > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
> > > > > >                 };
> > > > > >                 pmx_sdhci_standby: pinconfig-sdhci-standby {
> > > > > >                         config =
> > > > > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
> > > > > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> > > > > >                 };
> > > > >
> > > > > After thinking about this a bit more, I'm now thinking that we should
> > > > > probably only describe the active state in the device tree to keep things
> > > > > simple.
> > > > >
> > > > > Anything PM related could be initialized later on during the boot by the
> > > > > device driver, or even from userspace using /lib/firmware or /sys entries.
> > > > > This would cut down the device tree bloat quite a bit.
> > > >
> > > > I think it makes sense to describe everything in one place. It's much
> > > > easier to ensure everything is consistent and correct if you don't have
> > > > to cross-check two sources of data for the same thing.
> > >
> > > But we end up repeating the same bits over and over again making the device
> > > tree bloated.
> >
> > What exactly gets repeated; can you please show an example?
> 
> Well in your example, you have TEGRA_PMX_PG_DTA and TEGRA_PMX_PG_DTD
> repeated. It seems you're trying to list slightly different register
> values for the the pincontrol modes with pmx_sdhci_active and
> pmx_sdhci_standby.

Hmmm. I guess I was taking "repeated" a little too literally then...

There's an entry for each of DTA and DTD in each of the two nodes because
in this example, I want the value of parameter TEGRA_PMX_CONF_TRISTATE to
be different in each of those two nodes.

If the value wasn't different, there'd be no need to specify the value
of TEGRA_PMX_CONF_TRISTATE in each of the two nodes; you'd typically
specify it just once in the common node.

So, the binding itself isn't forcing you to repeat anything, it's just
in the example we actually want different values for this parameter in
different status. You need /some/ way to specify that.

If we don't do that in the common pinctrl binding, I see a couple choices:

a) Put it in static data in the pinctrl driver.

b) Put it in device tree in a binding that's specific to the individual
pinctrl driver, rather than a common binding.

Now in each case, if you want the value of some parameter on some pin to
be different in each of two states, you need /some/ way to represent that.
Perhaps in (a) above, the literal 32-bit value "DTA" could be implicit,
in that it's an array index into some table, but I don't see how to avoid
specifying the two different values that are desired...

> IMHO already that list of mux states seems limited as you may need to
> list other driver states too like pmx_sdhci_standby_wake_up_disabled and
> pmx_sdhci_standby_off. So it starts to sound a bit like PM policy instead
> of letting the driver know what hardware is on the system.

First off, the binding isn't enforcing any specific set of named states,
it's providing a mechanism to define a set of states with completely
arbitrary names. As such (as I think I gave an example elsewhere in this
thread), a dumb driver could require just one state e.g. "active", a
simple driver could require two states e.g. "active", "standby", and a
complex driver could require a whole bunch of different named states per
its needs. All this is allowed by the common binding which is just
providing a transport not policy. The pinctrl client drivers are what
are defining the required state names, in their individual binding.

Second, as I mentioned before, while some of the states are certainly
PM-related, I don't think all will be, e.g. the case of running an SD
controller at different clock rates to the SD card, and needing to
set different pin parameters based on the clock rate. Is runtime PM
intended cover that kind of thing? The idea here is that the common
pinctrl binding can allow the driver to require different named states
for those different clock rate cases.

> Having multiple config options also means that you end up setting
> #pinconfig-cell to something unnecessarily large for all mux nodes even
> if you only need the alternative values for one pin on the system.

I don't understand here.

In the example above, which I'll repeate:

config =
    <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;

#pinconfig-cells would be 3:
cell 0 is pin/group ID
cell 1 is configuration parameter
cell 2 is value for that configuration parameter

The parsing of the config node involves splitting the whole cell list
into chunks of #pinconfig-cells each, and interpreting each one in turn,
in exactly the same way as #gpio-cells can be used to parse a DT property
containing a list of GPIOs.

> > Given that each named state can reference n nodes defining part of that
> > state's pinmux configuration, and some of those nodes can affect the same
> > pin controller, you can easily split the overall configuration into
> > "stuff that's common for all states for this device" and "stuff that's
> > common between these n states of the device but not these m others" and
> > "stuff that's unique to this 1 state", and hence avoid repeating any
> > identical information.
> >
> > > I'm suspecting that the initial state can be used to set the PM states for
> > > pins. Probably most pins can have the PM configuration set from the start.
> >
> > Can you please explain more?
> 
> For the cases I know the default values work for 95% - 100% of the pins in
> the system for PM also. Only few pins on some systems need to be dynamically
> remuxed during the runtime. The rest of the pins can be discarded after init.

So that's fine.

95% the same means that the nodes you use to describe the differences are
only 5% of the total; no repetition necessary in the DT nodes.

Now for the 95% of the programming that happens at device probe time:
Perhaps we could throw away all the data representing that after it was
performed.

However, that's not related to the DT binding; it's an implementation
detail of the pinctrl subsystem that it parses the pinmux mapping table
early in boot and keeps it around in memory (e.g. to display in sysfs).
I don't believe there's anything in the DT binding that forces the
implementation to do that.

Similarly, if you don't need any (or very little) of the dynamic per-
device stuff, you could probably just set up your base pinmux config
as a huge array of (in pinctrl subsystem terms) "system hog" entries,
or in terms of this DT binding, pin configuration nodes that are associated
with the pin controller itself rather than individual devices, and just
list the differences (if any) in the per-device nodes.


I need a bit more time to digest the rest of your email; the parts where
I think you're saying:

a) For stuff like active/suspend, have the pinctrl driver know what
   that means and implement it rather than explicitly representing
   those states in device tree (but where do we represent them?)

b) For other states, do everything through runtime PM. I need to
   familiarize myself with runtime PM in order to understand how that'd
   work I guess.

-- 
nvpbulic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120126 11:03]:
> Tony Lindgren wrote at Tuesday, January 24, 2012 5:04 PM:
> 
> There's an entry for each of DTA and DTD in each of the two nodes because
> in this example, I want the value of parameter TEGRA_PMX_CONF_TRISTATE to
> be different in each of those two nodes.
> 
> If the value wasn't different, there'd be no need to specify the value
> of TEGRA_PMX_CONF_TRISTATE in each of the two nodes; you'd typically
> specify it just once in the common node.
> 
> So, the binding itself isn't forcing you to repeat anything, it's just
> in the example we actually want different values for this parameter in
> different status. You need /some/ way to specify that.
> 
> If we don't do that in the common pinctrl binding, I see a couple choices:
> 
> a) Put it in static data in the pinctrl driver.

Having data in the driver is fine as long as it's "how to operate certain
kind of features" type data for the driver. Then once you have hundreds of
instances of something, then the data for "where the instances are and what
type of instances they are" should be in device tree so the driver knows
how to use them.
 
> b) Put it in device tree in a binding that's specific to the individual
> pinctrl driver, rather than a common binding.
> 
> Now in each case, if you want the value of some parameter on some pin to
> be different in each of two states, you need /some/ way to represent that.
> Perhaps in (a) above, the literal 32-bit value "DTA" could be implicit,
> in that it's an array index into some table, but I don't see how to avoid
> specifying the two different values that are desired...

I don't think we should try to pass the different possible states from
device tree. The pinmux/pinconf driver should know how to deal with that,
and the driver using the mux should be able to communicate what it wants
to the pinmux/pinconf driver. If people really want to be able to pass
alternative mux states from device tree, they should be standard bindings
for things like active/idle/suspend/off.

Maybe you can just pass the hardware mux type from device tree? Something
like TEGRA_MUX_TYPE_SDHCI and then your pinmux/pinconf driver would know
how to deal with that set no matter where the instances are sprinkled on
new Tegras.

> > IMHO already that list of mux states seems limited as you may need to
> > list other driver states too like pmx_sdhci_standby_wake_up_disabled and
> > pmx_sdhci_standby_off. So it starts to sound a bit like PM policy instead
> > of letting the driver know what hardware is on the system.
> 
> First off, the binding isn't enforcing any specific set of named states,
> it's providing a mechanism to define a set of states with completely
> arbitrary names. As such (as I think I gave an example elsewhere in this
> thread), a dumb driver could require just one state e.g. "active", a
> simple driver could require two states e.g. "active", "standby", and a
> complex driver could require a whole bunch of different named states per
> its needs. All this is allowed by the common binding which is just
> providing a transport not policy. The pinctrl client drivers are what
> are defining the required state names, in their individual binding.

I'd rather prefer a combination of pinmux/pinconf initial register value,
mux type, and Linux generic flags from device tree rather than trying
to list register values for all possible states a device may have.

> Second, as I mentioned before, while some of the states are certainly
> PM-related, I don't think all will be, e.g. the case of running an SD
> controller at different clock rates to the SD card, and needing to
> set different pin parameters based on the clock rate. Is runtime PM
> intended cover that kind of thing? The idea here is that the common
> pinctrl binding can allow the driver to require different named states
> for those different clock rate cases.

For the PM related states, those should be Linux generic. For rate
setting sounds like that's really something you should set up as clocks
in the Tegra wrapper driver for SDHCI?

Ideally the SDHCI driver would be completely arch independent, and
then the SoC specific wrapper driver would know how to communicate to
the pinmux/pinconf framwork or clock framework what it needs using
Linux generic APIs. So I'd rather stay out of random named states for
the pins coming from device tree; If we still need them, they should
be common bindings rather than things like "xyz_clock_hack".

> > Having multiple config options also means that you end up setting
> > #pinconfig-cell to something unnecessarily large for all mux nodes even
> > if you only need the alternative values for one pin on the system.
> 
> I don't understand here.
> 
> In the example above, which I'll repeate:
> 
> config =
>     <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
>     <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
> 
> #pinconfig-cells would be 3:
> cell 0 is pin/group ID
> cell 1 is configuration parameter
> cell 2 is value for that configuration parameter
> 
> The parsing of the config node involves splitting the whole cell list
> into chunks of #pinconfig-cells each, and interpreting each one in turn,
> in exactly the same way as #gpio-cells can be used to parse a DT property
> containing a list of GPIOs.

Ah sorry, I misread your example and thought you had more than three for
some entries. No problem there then.
 
> > > Given that each named state can reference n nodes defining part of that
> > > state's pinmux configuration, and some of those nodes can affect the same
> > > pin controller, you can easily split the overall configuration into
> > > "stuff that's common for all states for this device" and "stuff that's
> > > common between these n states of the device but not these m others" and
> > > "stuff that's unique to this 1 state", and hence avoid repeating any
> > > identical information.
> > >
> > > > I'm suspecting that the initial state can be used to set the PM states for
> > > > pins. Probably most pins can have the PM configuration set from the start.
> > >
> > > Can you please explain more?
> > 
> > For the cases I know the default values work for 95% - 100% of the pins in
> > the system for PM also. Only few pins on some systems need to be dynamically
> > remuxed during the runtime. The rest of the pins can be discarded after init.
> 
> So that's fine.
> 
> 95% the same means that the nodes you use to describe the differences are
> only 5% of the total; no repetition necessary in the DT nodes.

Sure, but why not handle it properly in the drivers? :)

BTW, for how many mux entries do you think you'll need these alternative
custom states?
 
> Now for the 95% of the programming that happens at device probe time:
> Perhaps we could throw away all the data representing that after it was
> performed.

Yes that's what we should do in general unless a pin or mux is marked
as dynamic in device tree.
 
> However, that's not related to the DT binding; it's an implementation
> detail of the pinctrl subsystem that it parses the pinmux mapping table
> early in boot and keeps it around in memory (e.g. to display in sysfs).
> I don't believe there's anything in the DT binding that forces the
> implementation to do that.
> 
> Similarly, if you don't need any (or very little) of the dynamic per-
> device stuff, you could probably just set up your base pinmux config
> as a huge array of (in pinctrl subsystem terms) "system hog" entries,
> or in terms of this DT binding, pin configuration nodes that are associated
> with the pin controller itself rather than individual devices, and just
> list the differences (if any) in the per-device nodes.

Yes it seems that that's what most people prefer.
 
> I need a bit more time to digest the rest of your email; the parts where
> I think you're saying:
> 
> a) For stuff like active/suspend, have the pinctrl driver know what
>    that means and implement it rather than explicitly representing
>    those states in device tree (but where do we represent them?)

Maybe you can represent different types of mux groups in your
pinmux/pinconf driver and just pass the type from device tree?
 
> b) For other states, do everything through runtime PM. I need to
>    familiarize myself with runtime PM in order to understand how that'd
>    work I guess.

That provides the trigger for platform specific state changes from the
driver.

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Thu, Jan 26, 2012 at 06:08:33PM -0800, Tony Lindgren wrote:
> * Stephen Warren <swarren@nvidia.com> [120126 11:03]:
...
> > Second, as I mentioned before, while some of the states are certainly
> > PM-related, I don't think all will be, e.g. the case of running an SD
> > controller at different clock rates to the SD card, and needing to
> > set different pin parameters based on the clock rate. Is runtime PM
> > intended cover that kind of thing? The idea here is that the common
> > pinctrl binding can allow the driver to require different named states
> > for those different clock rate cases.
> 
> For the PM related states, those should be Linux generic. For rate
> setting sounds like that's really something you should set up as clocks
> in the Tegra wrapper driver for SDHCI?
> 
That's right.

> Ideally the SDHCI driver would be completely arch independent, and
> then the SoC specific wrapper driver would know how to communicate to
> the pinmux/pinconf framwork or clock framework what it needs using
> Linux generic APIs.

But that wrapper driver should not be bothered to call pinmux/pinconf
APIs on pin basis to change the pinctrl configuration.  The elegant
way would be something like the following in case that it switches
the bus frequency from 50 MHz to 100 MHz.

	pmx = pinmux_get(dev, "esdhc_50mhz");
	...
	pinmux_put(pmx);
	pmx = pinmux_get(dev, "esdhc_100mhz");
	...

The specific mux and config settings of states esdhc_50mhz and
esdhc_100mhz would be retrieved from device tree.

> So I'd rather stay out of random named states for
> the pins coming from device tree; If we still need them, they should
> be common bindings rather than things like "xyz_clock_hack".
> 
The binding defines the syntax, and I do not see the necessity to
force the particular state name, which is really pinctrl client
device specific.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

Hi,

* Shawn Guo <shawn.guo@linaro.org> [120126 22:15]:
> On Thu, Jan 26, 2012 at 06:08:33PM -0800, Tony Lindgren wrote:
> > * Stephen Warren <swarren@nvidia.com> [120126 11:03]:
> ...
> > > Second, as I mentioned before, while some of the states are certainly
> > > PM-related, I don't think all will be, e.g. the case of running an SD
> > > controller at different clock rates to the SD card, and needing to
> > > set different pin parameters based on the clock rate. Is runtime PM
> > > intended cover that kind of thing? The idea here is that the common
> > > pinctrl binding can allow the driver to require different named states
> > > for those different clock rate cases.
> > 
> > For the PM related states, those should be Linux generic. For rate
> > setting sounds like that's really something you should set up as clocks
> > in the Tegra wrapper driver for SDHCI?
> > 
> That's right.
> 
> > Ideally the SDHCI driver would be completely arch independent, and
> > then the SoC specific wrapper driver would know how to communicate to
> > the pinmux/pinconf framwork or clock framework what it needs using
> > Linux generic APIs.
> 
> But that wrapper driver should not be bothered to call pinmux/pinconf
> APIs on pin basis to change the pinctrl configuration.  The elegant
> way would be something like the following in case that it switches
> the bus frequency from 50 MHz to 100 MHz.
> 
> 	pmx = pinmux_get(dev, "esdhc_50mhz");
> 	...
> 	pinmux_put(pmx);
> 	pmx = pinmux_get(dev, "esdhc_100mhz");
> 	...
> 
> The specific mux and config settings of states esdhc_50mhz and
> esdhc_100mhz would be retrieved from device tree.

Yes whatever mux names can be used, same as with clock framework
for clock names. But that means you'll have to constantly get/put
the mux which is not efficient.

Wouldn't it be cleaner to just clk_get esdhc_clk during init, then
do clk_set_rate on it to toggle the rates?
 
> > So I'd rather stay out of random named states for
> > the pins coming from device tree; If we still need them, they should
> > be common bindings rather than things like "xyz_clock_hack".
> > 
> The binding defines the syntax, and I do not see the necessity to
> force the particular state name, which is really pinctrl client
> device specific.

Do you have some other custom pin state example other than the
clock rate change example above?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 27, 2012 at 09:05:45AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Shawn Guo <shawn.guo@linaro.org> [120126 22:15]:
> > On Thu, Jan 26, 2012 at 06:08:33PM -0800, Tony Lindgren wrote:
> > > * Stephen Warren <swarren@nvidia.com> [120126 11:03]:
> > ...
> > > > Second, as I mentioned before, while some of the states are certainly
> > > > PM-related, I don't think all will be, e.g. the case of running an SD
> > > > controller at different clock rates to the SD card, and needing to
> > > > set different pin parameters based on the clock rate. Is runtime PM
> > > > intended cover that kind of thing? The idea here is that the common
> > > > pinctrl binding can allow the driver to require different named states
> > > > for those different clock rate cases.
> > > 
> > > For the PM related states, those should be Linux generic. For rate
> > > setting sounds like that's really something you should set up as clocks
> > > in the Tegra wrapper driver for SDHCI?
> > > 
> > That's right.
> > 
> > > Ideally the SDHCI driver would be completely arch independent, and
> > > then the SoC specific wrapper driver would know how to communicate to
> > > the pinmux/pinconf framwork or clock framework what it needs using
> > > Linux generic APIs.
> > 
> > But that wrapper driver should not be bothered to call pinmux/pinconf
> > APIs on pin basis to change the pinctrl configuration.  The elegant
> > way would be something like the following in case that it switches
> > the bus frequency from 50 MHz to 100 MHz.
> > 
> > 	pmx = pinmux_get(dev, "esdhc_50mhz");
> > 	...
> > 	pinmux_put(pmx);
> > 	pmx = pinmux_get(dev, "esdhc_100mhz");
> > 	...
> > 
> > The specific mux and config settings of states esdhc_50mhz and
> > esdhc_100mhz would be retrieved from device tree.
> 
> Yes whatever mux names can be used, same as with clock framework
> for clock names. But that means you'll have to constantly get/put
> the mux which is not efficient.
> 
The most important reason that we want to move to pinctrl subsystem
is we need its run-time configuration feature for cases like esdhc
here.  I do not think the switch here is so constant to be inefficient.

> Wouldn't it be cleaner to just clk_get esdhc_clk during init, then
> do clk_set_rate on it to toggle the rates?
>  
It's not an init-time switch but run-time one.  That said,
sdhci_ops.set_clock will be called during run-time.

> > > So I'd rather stay out of random named states for
> > > the pins coming from device tree; If we still need them, they should
> > > be common bindings rather than things like "xyz_clock_hack".
> > > 
> > The binding defines the syntax, and I do not see the necessity to
> > force the particular state name, which is really pinctrl client
> > device specific.
> 
> Do you have some other custom pin state example other than the
> clock rate change example above?
> 
I have another case PM related.  To aggressively save power, the pins
configured for particular function during active mode need to be
muxed on gpio mode and output 0 in low-power mode.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [120129 17:13]:
> On Fri, Jan 27, 2012 at 09:05:45AM -0800, Tony Lindgren wrote:
> > Hi,
> > 
> > * Shawn Guo <shawn.guo@linaro.org> [120126 22:15]:
> > > On Thu, Jan 26, 2012 at 06:08:33PM -0800, Tony Lindgren wrote:
> > > > * Stephen Warren <swarren@nvidia.com> [120126 11:03]:
> > > ...
> > > > > Second, as I mentioned before, while some of the states are certainly
> > > > > PM-related, I don't think all will be, e.g. the case of running an SD
> > > > > controller at different clock rates to the SD card, and needing to
> > > > > set different pin parameters based on the clock rate. Is runtime PM
> > > > > intended cover that kind of thing? The idea here is that the common
> > > > > pinctrl binding can allow the driver to require different named states
> > > > > for those different clock rate cases.
> > > > 
> > > > For the PM related states, those should be Linux generic. For rate
> > > > setting sounds like that's really something you should set up as clocks
> > > > in the Tegra wrapper driver for SDHCI?
> > > > 
> > > That's right.
> > > 
> > > > Ideally the SDHCI driver would be completely arch independent, and
> > > > then the SoC specific wrapper driver would know how to communicate to
> > > > the pinmux/pinconf framwork or clock framework what it needs using
> > > > Linux generic APIs.
> > > 
> > > But that wrapper driver should not be bothered to call pinmux/pinconf
> > > APIs on pin basis to change the pinctrl configuration.  The elegant
> > > way would be something like the following in case that it switches
> > > the bus frequency from 50 MHz to 100 MHz.
> > > 
> > > 	pmx = pinmux_get(dev, "esdhc_50mhz");
> > > 	...
> > > 	pinmux_put(pmx);
> > > 	pmx = pinmux_get(dev, "esdhc_100mhz");
> > > 	...
> > > 
> > > The specific mux and config settings of states esdhc_50mhz and
> > > esdhc_100mhz would be retrieved from device tree.
> > 
> > Yes whatever mux names can be used, same as with clock framework
> > for clock names. But that means you'll have to constantly get/put
> > the mux which is not efficient.
> > 
> The most important reason that we want to move to pinctrl subsystem
> is we need its run-time configuration feature for cases like esdhc
> here.  I do not think the switch here is so constant to be inefficient.
> 
> > Wouldn't it be cleaner to just clk_get esdhc_clk during init, then
> > do clk_set_rate on it to toggle the rates?
> >  
> It's not an init-time switch but run-time one.  That said,
> sdhci_ops.set_clock will be called during run-time.

Right, basically you don't want to do clk_get or pinmux_get during
runtime, you do that once one during init. Then do clk_set_rate or
whaterver during runtime.

Is there anything stopping from implementing sdhci_ops.set_rate
using clock framework and clk_set_rate in this case BTW?
 
> > > > So I'd rather stay out of random named states for
> > > > the pins coming from device tree; If we still need them, they should
> > > > be common bindings rather than things like "xyz_clock_hack".
> > > > 
> > > The binding defines the syntax, and I do not see the necessity to
> > > force the particular state name, which is really pinctrl client
> > > device specific.
> > 
> > Do you have some other custom pin state example other than the
> > clock rate change example above?
> > 
> I have another case PM related.  To aggressively save power, the pins
> configured for particular function during active mode need to be
> muxed on gpio mode and output 0 in low-power mode.

OK, but basically only a small subset of pins of the total pins?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Mon, Jan 30, 2012 at 09:20:42AM -0800, Tony Lindgren wrote:
> * Shawn Guo <shawn.guo@linaro.org> [120129 17:13]:
> > On Fri, Jan 27, 2012 at 09:05:45AM -0800, Tony Lindgren wrote:
...
> > > Wouldn't it be cleaner to just clk_get esdhc_clk during init, then
> > > do clk_set_rate on it to toggle the rates?
> > >  
> > It's not an init-time switch but run-time one.  That said,
> > sdhci_ops.set_clock will be called during run-time.
> 
> Right, basically you don't want to do clk_get or pinmux_get during
> runtime, you do that once one during init. Then do clk_set_rate or
> whaterver during runtime.
> 
> Is there anything stopping from implementing sdhci_ops.set_rate
> using clock framework and clk_set_rate in this case BTW?
>  
We are doing this exactly for clk, and trying to figure out how to
handle pinctrl here.  I do not see how we can do pinmux_get at
init-time and pinmux_set_whatever at run-time.  The pinmux API does
not work that way.

> > > > > So I'd rather stay out of random named states for
> > > > > the pins coming from device tree; If we still need them, they should
> > > > > be common bindings rather than things like "xyz_clock_hack".
> > > > > 
> > > > The binding defines the syntax, and I do not see the necessity to
> > > > force the particular state name, which is really pinctrl client
> > > > device specific.
> > > 
> > > Do you have some other custom pin state example other than the
> > > clock rate change example above?
> > > 
> > I have another case PM related.  To aggressively save power, the pins
> > configured for particular function during active mode need to be
> > muxed on gpio mode and output 0 in low-power mode.
> 
> OK, but basically only a small subset of pins of the total pins?
> 
Actually, all pins used by the block.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [120130 16:49]:
> On Mon, Jan 30, 2012 at 09:20:42AM -0800, Tony Lindgren wrote:
> > * Shawn Guo <shawn.guo@linaro.org> [120129 17:13]:
> > > On Fri, Jan 27, 2012 at 09:05:45AM -0800, Tony Lindgren wrote:
> ...
> > > > Wouldn't it be cleaner to just clk_get esdhc_clk during init, then
> > > > do clk_set_rate on it to toggle the rates?
> > > >  
> > > It's not an init-time switch but run-time one.  That said,
> > > sdhci_ops.set_clock will be called during run-time.
> > 
> > Right, basically you don't want to do clk_get or pinmux_get during
> > runtime, you do that once one during init. Then do clk_set_rate or
> > whaterver during runtime.
> > 
> > Is there anything stopping from implementing sdhci_ops.set_rate
> > using clock framework and clk_set_rate in this case BTW?
> >  
> We are doing this exactly for clk, and trying to figure out how to
> handle pinctrl here.  I do not see how we can do pinmux_get at
> init-time and pinmux_set_whatever at run-time.  The pinmux API does
> not work that way.

Hmm OK I see what you mean. Maybe we should have something like
pinmux_get/set_function to change the mux without having to do
pinmux_put and pinmux_get during runtime? As long as the locking is
per pin that should be safe to do.
 
> > > > > > So I'd rather stay out of random named states for
> > > > > > the pins coming from device tree; If we still need them, they should
> > > > > > be common bindings rather than things like "xyz_clock_hack".
> > > > > > 
> > > > > The binding defines the syntax, and I do not see the necessity to
> > > > > force the particular state name, which is really pinctrl client
> > > > > device specific.
> > > > 
> > > > Do you have some other custom pin state example other than the
> > > > clock rate change example above?
> > > > 
> > > I have another case PM related.  To aggressively save power, the pins
> > > configured for particular function during active mode need to be
> > > muxed on gpio mode and output 0 in low-power mode.
> > 
> > OK, but basically only a small subset of pins of the total pins?
> > 
> Actually, all pins used by the block.

That's still a fraction of the total pins on the SoC that need dynamic
remuxing, right?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On 31 January 2012 10:29, Tony Lindgren <tony@atomide.com> wrote:
...
> That's still a fraction of the total pins on the SoC that need dynamic
> remuxing, right?
>
That's right.

Regards,
Shawn

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Thursday, January 26, 2012 7:09 PM:
...
> I don't think we should try to pass the different possible states from
> device tree. The pinmux/pinconf driver should know how to deal with that,

Somehow, the pinctrl driver needs to know how to implement each state. In 
general, I believe this will be board-specific.

Do you disagree with this assertion?

If the data is board-specific, I don't see how it can be represented
anywhere but the device tree.

> and the driver using the mux should be able to communicate what it wants
> to the pinmux/pinconf driver. If people really want to be able to pass
> alternative mux states from device tree, they should be standard bindings
> for things like active/idle/suspend/off.

As I've mentioned before, people have asked for driver-specific states to
handle the case where e.g. drive strength must be adjusted based on clock
rates of the interface. Again, I believe that's board-specific data since
the actual values to use may be derived during board calibration, not
SoC design.

Do you disagree that this data may be board specific?

--
nvpublic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120127 09:05]:
> Tony Lindgren wrote at Thursday, January 26, 2012 7:09 PM:
> ...
> > I don't think we should try to pass the different possible states from
> > device tree. The pinmux/pinconf driver should know how to deal with that,
> 
> Somehow, the pinctrl driver needs to know how to implement each state. In 
> general, I believe this will be board-specific.
> 
> Do you disagree with this assertion?
> 
> If the data is board-specific, I don't see how it can be represented
> anywhere but the device tree.

Agreed, for board specific things device tree is the place to put it.
 
> > and the driver using the mux should be able to communicate what it wants
> > to the pinmux/pinconf driver. If people really want to be able to pass
> > alternative mux states from device tree, they should be standard bindings
> > for things like active/idle/suspend/off.
> 
> As I've mentioned before, people have asked for driver-specific states to
> handle the case where e.g. drive strength must be adjusted based on clock
> rates of the interface. Again, I believe that's board-specific data since
> the actual values to use may be derived during board calibration, not
> SoC design.
> 
> Do you disagree that this data may be board specific?

Right, but for the dynamic mux cases I've seen the alternative states
are usually specific to the driver using the mux. That's why I'm
suspicious of the board specific custom alternative states :)

Anyways, do you think the pinctrl-static + pinctrl-dynamic binding
I just posted as a reply to Simon's email would work for you?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

Hi Tony,

On Mon, Jan 23, 2012 at 01:00:52PM -0800, Tony Lindgren wrote:
...
> So to summarize: I suggest we'll just stick to basics to get the system
> booting and devices working using device tree. In most cases the device
> drivers should be able to configure the suspend and off states in a generic
> way using pinctrl API. Everything else, like debugging, we can probably
> do with userspace tools.
> 
> This would mean just using a minimal subset of your binding, probably
> very close to what you originally suggested.
> 
IMHO, as a generic device tree binding, it should be able to cope with
different use cases.  It's really free for you to use the minimal
subset of the binding as your need, but we should not make the binding
design just be that minimal subset to force that everyone else can
only use the minimal subset.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

Hi,

* Shawn Guo <shawn.guo@linaro.org> [120126 00:53]:
> 
> On Mon, Jan 23, 2012 at 01:00:52PM -0800, Tony Lindgren wrote:
> ...
> > So to summarize: I suggest we'll just stick to basics to get the system
> > booting and devices working using device tree. In most cases the device
> > drivers should be able to configure the suspend and off states in a generic
> > way using pinctrl API. Everything else, like debugging, we can probably
> > do with userspace tools.
> > 
> > This would mean just using a minimal subset of your binding, probably
> > very close to what you originally suggested.
> > 
> IMHO, as a generic device tree binding, it should be able to cope with
> different use cases.  It's really free for you to use the minimal
> subset of the binding as your need, but we should not make the binding
> design just be that minimal subset to force that everyone else can
> only use the minimal subset.

The main issue I have is that the example posted in this thread repeats
the same registers five times for one driver entry alone in the device
tree data. The repeated registers are TEGRA_PMX_PG_DTA and TEGRA_PMX_PG_DTD
in the example.

The alternative values are something that the pinmux/pinconf driver can
set based on state changes communicated from the driver using these pins.

That's why I think these alternative states should not be listed in the
device tree.

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Thu, Jan 26, 2012 at 09:51:23AM -0800, Tony Lindgren wrote:
> Hi,
> 
> * Shawn Guo <shawn.guo@linaro.org> [120126 00:53]:
> > 
> > On Mon, Jan 23, 2012 at 01:00:52PM -0800, Tony Lindgren wrote:
> > ...
> > > So to summarize: I suggest we'll just stick to basics to get the system
> > > booting and devices working using device tree. In most cases the device
> > > drivers should be able to configure the suspend and off states in a generic
> > > way using pinctrl API. Everything else, like debugging, we can probably
> > > do with userspace tools.
> > > 
> > > This would mean just using a minimal subset of your binding, probably
> > > very close to what you originally suggested.
> > > 
> > IMHO, as a generic device tree binding, it should be able to cope with
> > different use cases.  It's really free for you to use the minimal
> > subset of the binding as your need, but we should not make the binding
> > design just be that minimal subset to force that everyone else can
> > only use the minimal subset.
> 
> The main issue I have is that the example posted in this thread repeats
> the same registers five times for one driver entry alone in the device
> tree data. The repeated registers are TEGRA_PMX_PG_DTA and TEGRA_PMX_PG_DTD
> in the example.
> 
> The alternative values are something that the pinmux/pinconf driver can
> set based on state changes communicated from the driver using these pins.

I'm against the idea that requires client device drivers call pinctrl
APIs on pin basis.  They do not even know or care about the pins that
they are using.  That's all pinctrl driver's business.  All client
driver needs to do when it wants to change pinctrl setup should just
be calling pinmux/pinconf_get/put[enable/disable](dev, "state").

Regards,
Shawn

> 
> That's why I think these alternative states should not be listed in the
> device tree.

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [120126 22:37]:
> On Thu, Jan 26, 2012 at 09:51:23AM -0800, Tony Lindgren wrote:
> > Hi,
> > 
> > * Shawn Guo <shawn.guo@linaro.org> [120126 00:53]:
> > > 
> > > On Mon, Jan 23, 2012 at 01:00:52PM -0800, Tony Lindgren wrote:
> > > ...
> > > > So to summarize: I suggest we'll just stick to basics to get the system
> > > > booting and devices working using device tree. In most cases the device
> > > > drivers should be able to configure the suspend and off states in a generic
> > > > way using pinctrl API. Everything else, like debugging, we can probably
> > > > do with userspace tools.
> > > > 
> > > > This would mean just using a minimal subset of your binding, probably
> > > > very close to what you originally suggested.
> > > > 
> > > IMHO, as a generic device tree binding, it should be able to cope with
> > > different use cases.  It's really free for you to use the minimal
> > > subset of the binding as your need, but we should not make the binding
> > > design just be that minimal subset to force that everyone else can
> > > only use the minimal subset.
> > 
> > The main issue I have is that the example posted in this thread repeats
> > the same registers five times for one driver entry alone in the device
> > tree data. The repeated registers are TEGRA_PMX_PG_DTA and TEGRA_PMX_PG_DTD
> > in the example.
> > 
> > The alternative values are something that the pinmux/pinconf driver can
> > set based on state changes communicated from the driver using these pins.
> 
> I'm against the idea that requires client device drivers call pinctrl
> APIs on pin basis.  They do not even know or care about the pins that
> they are using.  That's all pinctrl driver's business.  All client
> driver needs to do when it wants to change pinctrl setup should just
> be calling pinmux/pinconf_get/put[enable/disable](dev, "state").

That should work fine too. As long as the states are generic, such as
PMX_STATE_ENABLED, PMX_STATE_IDLE, PMX_STATE_SUSPEND, PMX_STATE_OFF
and so on. You really don't want to change states with some string
names if you need to toggle something every time your enter and exit
idle. And it should be possible to set it on per pin or pingroup basis.
For wake-up events you typically just toggle one pin for the device,
such as serial rx line.

I guess the analog we should follow here is clk_get and clk_set_rate,
except we would have pinconf_set_state with predefined states.

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 27, 2012 at 09:16:53AM -0800, Tony Lindgren wrote:
...
> I guess the analog we should follow here is clk_get and clk_set_rate,
> except we would have pinconf_set_state with predefined states.
> 
It seems working for cases that we only change pinconf but never pinmux
for different configuration states.  But how would that work for cases
that require mux change for different configuration states?

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [120129 17:27]:
> On Fri, Jan 27, 2012 at 09:16:53AM -0800, Tony Lindgren wrote:
> ...
> > I guess the analog we should follow here is clk_get and clk_set_rate,
> > except we would have pinconf_set_state with predefined states.
> > 
> It seems working for cases that we only change pinconf but never pinmux
> for different configuration states.  But how would that work for cases
> that require mux change for different configuration states?

I don't see why we should not allow changing the mux state with pinconf
too, after all it's the mux/pin that's locked, not the functionality of
the pin.

An example of this would be remuxing a shared UART line between rx and
tx. Those kind of cases could be defined as PMX_DIRECTION_INPUT and
PMX_DIRECTION_OUTPUT so driver could call Linux generic functions for
those if implemented.

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Mon, Jan 30, 2012 at 09:43:23AM -0800, Tony Lindgren wrote:
> * Shawn Guo <shawn.guo@linaro.org> [120129 17:27]:
> > On Fri, Jan 27, 2012 at 09:16:53AM -0800, Tony Lindgren wrote:
> > ...
> > > I guess the analog we should follow here is clk_get and clk_set_rate,
> > > except we would have pinconf_set_state with predefined states.
> > > 
> > It seems working for cases that we only change pinconf but never pinmux
> > for different configuration states.  But how would that work for cases
> > that require mux change for different configuration states?
> 
> I don't see why we should not allow changing the mux state with pinconf
> too, after all it's the mux/pin that's locked, not the functionality of
> the pin.
> 
My point is I do not see how we can use the current pinmux APIs design
to change mux in the way how clk_get and clk_set_rate work.  Or you
have a pinmux APIs reconstruction on your mind?

Regards,
Shawn

> An example of this would be remuxing a shared UART line between rx and
> tx. Those kind of cases could be defined as PMX_DIRECTION_INPUT and
> PMX_DIRECTION_OUTPUT so driver could call Linux generic functions for
> those if implemented.
> 

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 20, 2012 at 02:22:20PM -0800, Stephen Warren wrote:
> Here's V2 of my pinmux binding proposal, after incorporating some feedback
> from Shawn Guo and Dong Aisheng. Main changes:
> 
It looks all good to me.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Simon Glass]

Hi Stephen,

On Fri, Jan 20, 2012 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
> Here's V2 of my pinmux binding proposal, after incorporating some feedback
> from Shawn Guo and Dong Aisheng. Main changes:
>
> 1) Require the "pin config" nodes the be children of the pinmux controller
>   node, rather than allowing them to alternatively be children of the
>   device node that's using them. This allows removal of the pin controller
>   phandle as the first entry in each specifier.
>
> 2) Add explicit #pinmux-cells and #pinconfig-cells properties to the pin
>   controller, so that pin controllers can define the layout of their mux
>   and configuration specifiers.
>
> 3) Don't document a "1:1" model; only document a "1:n" model, whereby each
>   named state for a device node can refer to multiple pin configuration
>   nodes. This allows:
>
>   a) A device to use multiple configuration nodes from a single pin
>      controller. For example, one may specify a very common subset of
>      the configuration for a particular device, and another may specify
>      a few board-specific tweaks on top of that.
>
>   b) A single device to specify configurations for multiple pin
>      controllers.
>
> TODO: I should write a real binding document for this, rather than simply
> providing a commented example.
>
> 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:
>
> / {
>        /*
>         * The SoC .dtsi file will define the basic properties of each HW
>         * module; compatible, reg, interrupts, ... There's no change here
>         * relative to normal practice.
>         */
>        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 */
>                /*
>                 * Common pin control bindings specify that these properties
>                 * must exist in a pin controller. Each individual pin
>                 * controller's bindings specify the values.
>                 */
>                #pinmux-cells = <2>;
>                #pinconfig-cells = <3>;
>        };
>
>        sdhci@c8000200 {
>                compatible = "nvidia,tegra20-sdhci";
>                reg = <0xc8000200 0x200>;
>                interrupts = <0 15 0x04>;
>        };
> };
>
> tegra20.dtis or tegra-harmony.dts:
>
> /{
>        /*
>         * The pin controller node will contain child nodes that define
>         * various subsets of the overall pin state.
>         *
>         * These nodes may be defined in the SoC .dtsi file if the vendor
>         * wants to provide a set of common configurations for customers
>         * or users to pick from.
>         *
>         * These nodes may be defined in the board .dts file too; e.g. if
>         * a particular board uses a configuration that isn't in the set
>         * of provided common options, or the SoC vendor simply chose not
>         * to provide a set of common options.
>         */
>        &tegra_pmx {
>                /*
>                 * There are (may be) many of these pin configuration child
>                 * nodes. Each is referenced by phandle from device nodes.
>                 *
>                 * Below I've shown an example of an SDHCI controller. There
>                 * is a set of common settings in node pmx_sdhci. The slight
>                 * differences between active and standby states are in nodes
>                 * pmc_sdhci_active and pmx_sdhci_suspend.
>                 *
>                 * Note though the the 3-node split is just an example; you
>                 * could easily have a single node for devices that don't
>                 * need separate active/suspend state, or many more nodes
>                 * for a device with a more complex set of shared settings
>                 * or that uses more states.
>                 */
>                pmx_sdhci: pinconfig-sdhci {
>                        /*
>                         * The mux property is a list of muxable entities
>                         * and the mux function to select for it. The number
>                         * of cells in each entry is the pin controller's
>                         * #pinmux-cells property. The pin controller's
>                         * binding defines what the cells mean. The pinctrl
>                         * driver is responsible for mapping this data to
>                         * the (group, function) pair required to fill in
>                         * the pinctrl subsystem's pinmux mapping table.
>                         */
>                        mux =
>                                <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
>                                <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
>                        /*
>                         * The config property is a list of muxable entities
>                         * and individual configuration setting. The number
>                         * of cells in each entry is the pin controller's
>                         * #pinconfig-cells property. The pin controller's
>                         * binding defines what the cells mean. The pinctrl
>                         * driver is responsible for mapping this data to
>                         * the (group, config) pair required to fill in
>                         * the pinctrl subsystem's pin configuration table.
>                         */
>                        config =
>                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
>                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
>                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
>                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
>                };
>                pmx_sdhci_active: pinconfig-sdhci-active {
>                        /*
>                         * In each of these nodes, both the mux and config
>                         * properties are optional. This node represents the
>                         * additions to pmx_sdhci that are specific to an
>                         * active state. In this case, only pin configuration
>                         * settings are different.
>                         */
>                        config =
>                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 0>
>                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 0>;
>                };
>                pmx_sdhci_standby: pinconfig-sdhci-standby {
>                        config =
>                                <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_TRISTATE 1>
>                                <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_TRISTATE 1>;
>                };
>                /*
>                 * ... continuing from the comment in pmx_sdhci_active:
>                 *
>                 * However, one could imagine representing GPIO as a regular
>                 * pinmux function, and have a board .dts file create an
>                 * extra node to represent that, and hence containing just
>                 * a mux property:
>                 */
>                pmx_sdhci_harmony: pinconfig-sdhci-harmony {
>                        mux = <TEGRA_PMX_PIN_GPIO_PA1 TEGRA_PMX_MUX_GPIO>;
>                };
>        };
> };
>
> tegra-harmony.dts:
>
> /{
>        sdhci@c8000200 {
>                /*
>                 * Each device node contains a few properties to describe
>                 * the named pinmux states that are available to it.
>                 *
>                 * The binding for the device node specifies the state names
>                 * that must be described; common examples such as "default"
>                 * or "active" and "suspend" may be universal, but the IO
>                 * protocol that a device supports may demand that more
>                 * states be defined, such as "active-12mhz", "active-50mhz",
>                 * "suspend". Drivers request these named states e.g. using
>                 * pinctrl's pinmux_get("state_name") API.
>                 *
>                 * pinctrl-names lists the available state names.
>                 *
>                 * Unlike the common clock binding, I assume here that states
>                 * are always requested by name. If we don't like that idea,
>                 * we'd could make this property optional, and add a new API
>                 * pinmux_get(state_id) to the pinctrl subsystem.
>                 */
>                pinctrl-names = "active", "suspend";
>                /*
>                 * pinctrl-entries contains a single cell for each state name
>                 * in pinctrl-names. It defines how many entries are present
>                 * in property "pinctrl" for the given named state.
>                 *
>                 * Here, state "active" covers 2 entries (0 and 1) in pinctrl,
>                 * and state "suspend" covers 2 more entries (2 and 3) in
>                 * pinctrl.
>                 */
>                pinctrl-entries = <2 2>;
>                /*
>                 * pinctrl contains a list of phandles. Each refers to one of
>                 * the pin config nodes within the pin controller. So, to set
>                 * up state "active", nodes pmx_sdhci and pmx_sdhci_active
>                 * must both be programmed. To set up state "suspend", nodes
>                 * pmx_sdhci and pmx_sdhci_standby must be programmed.
>                 */
>                pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
>                          <&pmx_sdhci> <&pmx_sdhci_standby>;
>        };
> };
>
> --
> nvpublic
>

I have been through this carefully and thought about how this might be
implemented. Your example has really helped me to understand this
properly, as the 70-email-long thread was a huge amount of reading.

So I have the following comments in general:

1. It doesn't seem to make full use of the device tree format. For example,

   <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>

would be better as something like

    drive-strength = <5>;

if we could arrange it. It also reduces the need for these
TEGRA_PMX_CONF_DRIVE_STRENGTH defines.

2. The TEGRA_PMX_ prefix on everything seems unnecessary to me and
makes it harder to read and more long-winded. Other SOCs will not
include the same header, I think. Can we just drop this?

3. To me, the pinctrl-entries following by pinctrl also does not make
great use of the device tree. It seems to me that this:

pinctrl-entries = <2 2>;
pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
         <&pmx_sdhci> <&pmx_sdhci_standby>;

where you say:
>                 * Here, state "active" covers 2 entries (0 and 1) in pinctrl,
>                 * and state "suspend" covers 2 more entries (2 and 3) in
>                 * pinctrl.

might be better as something like:

state-active {
       mux =  <&pmx_sdhci> <&pmx_sdhci_active>;
};

state-standby {
       mux =  <&pmx_sdhci> <&pmx_sdhci_standby>;
};

which means that we don't need to represent a 2-dimensional array in a
single property, which seems error-prone and complicated. In general I
wonder if we can make more use of hierarchy. This might help it be
more extensible in future also.

4. Your example for different 'states' is clear, but I'm not sure if
you are intending to handling actual different pin assignments in
these 'states'. To me this is the big point: does SDIO1 come out pin
groups DTA and DTC; or does it come out pin groups SLXA-D. That's what
the boards want to select. I think the state side of it may be best as
a separate thing, which drivers may even just handle themselves if
they are basic implementations. Or they may just want to implement the
active state (U-Boot).

5. My previous email suggested that the SOC define several possible
options for each peripheral so that the board vendor can just select
them from a menu. All the device tree complexity would then live in
the SOC file (tegra20.dtsi in this case) and very little in the board
file. I still think that is a worthwhile goal. You have this in there,
but I feel it could be simplified further, to a single phandle
perhaps.

6. Looking ahead I imagine that the pinmux nodes will become large. I
don't know how large but looking at some of the kernel bindings for TI
I do get a little alarmed. Board vendors may wish to try to cut down
the size of the device tree blob, particularly for use in the boot
loader. With the binding in your email I think that would be hard to
do. In particular if people know that standby is not being used in the
boot loader, I think they would need to do things like change:

pinctrl-entries = <2 2>;
pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
         <&pmx_sdhci> <&pmx_sdhci_standby>;

to:

pinctrl-entries = <2>;
pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>;

just so that they can delete the standby nodes and reduce the blob
size. It seems a little messy. For this purpose it would be much nicer
if the board vendor could just remove nodes [please, discussion about
whether board vendors SHOULD do this aside]. If everything is nicely
set up in the device tree hierarchy that would be simpler. For
example, board vendors could remove all the pinmux node that they are
not using (let's say they keep SDIO1 DTA, DTC and SDIO2 SLX but remove
the other SDIO1 and SDIO2 nodes and remove all SDIO3 and SDIO4 nodes).
Even better if one day we have a way of automatically filtering out
these unused nodes when creating the device tree blob for a board -
separate discussion.

7. It would be nice to avoid huge data tables in the code, and keep
these in the device tree where possible. I only mention this because
it keeps coming up in the threads.

So, rather than just provide vague feedback, I though I would try to
modify your binding according to the above, so it is below, for
consideration. I am not entirely happen with having the states under
each pin, but I do think it makes the meaning very clear. The example
pinmux is made up and doesn't correspond properly to Tegra2.


In tegra-harmony.dts:

/ {
	/* Select pin mux for these two SDHCI ports */
	sdhci@c8000200 {
		pinctrl = <&pmx_sdhci1_dta_dtd>;
	};

	sdhci@c8000400 {
		pinctrl = <&pmx_sdhci2_sdb_sdc>;
	};
};

In tegra20.dtsi:

/ {
	&tegra_pmx {
		#address-cells = <1>;
		#size-cells = <0>;

		/*
		 * This is the first option for SDIO1. It comes out
		 * on pin groups DTA and DTD. Boards can simply use this
		 * phandle in the driver node to get this option. Any options
		 * not used could potentially be dropped from the device tree
		 * blob for space-constrained boot loaders.
		 */
		pmx_sdhci1_dta_dtd: sdhci1-dta-dtd@0 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>;
			label = "SDIO1 on DTA, DTD (4-bit)";

			/*
			 * Here are the pin groups needed for this option.
			 * First DTA, then DTD.
			 */
			pmx@dta {
				reg = <PG_DTA>;
				mux = <PMX_MUX_SDIO1>;
				drive-strengh = <5>;
				slew-rate = <4>;

				/*
				 * We support two states here, active
				 * and standby. Properties in these child
				 * nodes can override the ones at this level.
				 * Drivers can move between states just by
				 * making the changes in these nodes.
				 */
				state-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
				};
				state-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
					drive-strengh = <2>;
				};
			};
			pmx@dtd {
				reg = <PG_DTD>;
				mux = <PMX_MUX_SDIO1>;
				slew-rate = <8>;
				pmx-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
					drive-strengh = <5>;
				};
				pmx-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
					drive-strengh = <2>;
				};
			};
		};

		/*
		 * This is the second option for SDIO1. It comes out
		 * on four SLX pins and supports 8-bit data.
		 */
		pmx_sdhci_slx: sdhci-slx@1 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <1>;
			label = "SDIO1 on SLXA, C, D, X (8-bit)";
			pmx@slxa {
				reg = <PG_SLXA>;
				mux = <PMX_MUX_SDIO1>;
				drive-strengh = <5>;
				slew-rate = <4>;
				state-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
				};
				state-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
				};
			}
			pmx@slxb {
				reg = <PG_SLXB>;
				mux = <PMX_MUX_SDIO1>;
				drive-strengh = <5>;
				slew-rate = <4>;
				state-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
				};
				state-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
				};
			}
			pmx@slxc {
				reg = <PG_SLXC>;
				mux = <PMX_MUX_SDIO1>;
				drive-strengh = <5>;
				slew-rate = <4>;
				state-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
				};
				state-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
				};
			}
			pmx@slxd {
				reg = <PG_SLXD>;
				mux = <PMX_MUX_SDIO1>;
				drive-strengh = <5>;
				slew-rate = <4>;
				state-active {
					reg = <PMX_CONF_ACTIVE>;
					tristate = <0>;
				};
				state-standby {
					reg = <PMX_CONF_STANDBY>;
					tristate = <1>;
				};
			};
		};

		/*
		 * This is the first option for SDIO2...
		 */
		pmx_sdhci2_sdb_sdc: sdhci2-sdb-sdc@0 {
			#address-cells = <1>;
			#size-cells = <0>;
			reg = <0>;
			label = "SDIO2 on SDB, SDC (4-bit)";
			...
		...
		};
	};
};


Regards,
Simon

Re: Pinmux bindings proposal V2

[Tony Lindgren]

Hi,

* Simon Glass <sjg@chromium.org> [120126 09:11]:
> 
> On Fri, Jan 20, 2012 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
> 
> 1. It doesn't seem to make full use of the device tree format. For example,
> 
>    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> 
> would be better as something like
> 
>     drive-strength = <5>;
> 
> if we could arrange it. It also reduces the need for these
> TEGRA_PMX_CONF_DRIVE_STRENGTH defines.

I agree. This is something that most pinmux/pinconf drivers need to
implement, so it's best done in a generic way.
 
> In tegra20.dtsi:
> 
> / {
> 	&tegra_pmx {
> 		#address-cells = <1>;
> 		#size-cells = <0>;
> 
> 		/*
> 		 * This is the first option for SDIO1. It comes out
> 		 * on pin groups DTA and DTD. Boards can simply use this
> 		 * phandle in the driver node to get this option. Any options
> 		 * not used could potentially be dropped from the device tree
> 		 * blob for space-constrained boot loaders.
> 		 */
> 		pmx_sdhci1_dta_dtd: sdhci1-dta-dtd@0 {
> 			#address-cells = <1>;
> 			#size-cells = <0>;
> 			reg = <0>;
> 			label = "SDIO1 on DTA, DTD (4-bit)";
> 
> 			/*
> 			 * Here are the pin groups needed for this option.
> 			 * First DTA, then DTD.
> 			 */
> 			pmx@dta {
> 				reg = <PG_DTA>;
> 				mux = <PMX_MUX_SDIO1>;
> 				drive-strengh = <5>;
> 				slew-rate = <4>;

Using reg for the register offsets here makes sense to me. But doesn't that
mean that now we're back to having a node for each pin? And that is something
we're trying to avoid because of the bloat as most systems have one register
per pin, so this is not efficient for listing multiple pins.

So maybe we should just use what Stephen suggested for the array of registers
here:

				mux = <MUX_OFFSET1 INITIAL_MUX_VALUE1
				       MUX_OFFSET2 INITIAL_MUX_VALUE2>;

> 
> 				/*
> 				 * We support two states here, active
> 				 * and standby. Properties in these child
> 				 * nodes can override the ones at this level.
> 				 * Drivers can move between states just by
> 				 * making the changes in these nodes.
> 				 */
> 				state-active {
> 					reg = <PMX_CONF_ACTIVE>;
> 					tristate = <0>;
> 				};
> 				state-standby {
> 					reg = <PMX_CONF_STANDBY>;
> 					tristate = <1>;
> 					drive-strengh = <2>;
> 				};
> 			};

This seems like a qood way to represent the alternative mux states for
the muxes that need them. This is assuming the states have standard
bindings and not some random names. I still don't know if we need them
though. And again when we have multiple registers, we'd have to have
either multiple entries for each pin, or use the array instead of reg.

Maybe we need two bindings: A minimal subset of what Stephen is suggesting
that can handle 95% of the muxes with minimal overhead, then what you're
suggesting for the few muxes that need multiple states?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Simon Glass]

Hi Tony,

On Thu, Jan 26, 2012 at 6:21 PM, Tony Lindgren <tony@atomide.com> wrote:
> Hi,
>
> * Simon Glass <sjg@chromium.org> [120126 09:11]:
>>
>> On Fri, Jan 20, 2012 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
>>
>> 1. It doesn't seem to make full use of the device tree format. For example,
>>
>>    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
>>
>> would be better as something like
>>
>>     drive-strength = <5>;
>>
>> if we could arrange it. It also reduces the need for these
>> TEGRA_PMX_CONF_DRIVE_STRENGTH defines.
>
> I agree. This is something that most pinmux/pinconf drivers need to
> implement, so it's best done in a generic way.
>
>> In tegra20.dtsi:
>>
>> / {
>>       &tegra_pmx {
>>               #address-cells = <1>;
>>               #size-cells = <0>;
>>
>>               /*
>>                * This is the first option for SDIO1. It comes out
>>                * on pin groups DTA and DTD. Boards can simply use this
>>                * phandle in the driver node to get this option. Any options
>>                * not used could potentially be dropped from the device tree
>>                * blob for space-constrained boot loaders.
>>                */
>>               pmx_sdhci1_dta_dtd: sdhci1-dta-dtd@0 {
>>                       #address-cells = <1>;
>>                       #size-cells = <0>;
>>                       reg = <0>;
>>                       label = "SDIO1 on DTA, DTD (4-bit)";
>>
>>                       /*
>>                        * Here are the pin groups needed for this option.
>>                        * First DTA, then DTD.
>>                        */
>>                       pmx@dta {
>>                               reg = <PG_DTA>;
>>                               mux = <PMX_MUX_SDIO1>;
>>                               drive-strengh = <5>;
>>                               slew-rate = <4>;
>
> Using reg for the register offsets here makes sense to me. But doesn't that
> mean that now we're back to having a node for each pin? And that is something
> we're trying to avoid because of the bloat as most systems have one register
> per pin, so this is not efficient for listing multiple pins.

Yes it does add a lot of overhead - I am responding to what I see as a
lot of effort to 'work around' the device tree format but turning
everything into property numbers rather than using properties more as
intended by the format. Using the device fully makes things
considerably easier to read.

The cost of the pmx@dta node is about 12 bytes for the header (it
depends on the length of the name), and each of the properties above
is 16 bytes. So in total this node is 76 bytes. If we have 250 pins
being muxed as Tegra3 then this is about 20KB (including a bit of
slack for longer names). My point about being able to 'optimise out'
some of these remains, though, but probably not for the kernel.

Stephen's 'mux' property uses 12 bytes plus 8 bytes per pin/group (I
am removing the prefixes):

                       mux =
                               <PG_DTA   MUX_SDIO1>
                               <PG_DTD   MUX_SDIO1>;

so 28 bytes. What I proposed would use (12 + 2 * 16) per pin/group, or
44 bytes (60% bigger):

>>                       pmx@dta {
>>                               reg = <PG_DTA>;
>>                               mux = <PMX_MUX_SDIO1>;
>
> So maybe we should just use what Stephen suggested for the array of registers
> here:
>
>                                mux = <MUX_OFFSET1 INITIAL_MUX_VALUE1
>                                       MUX_OFFSET2 INITIAL_MUX_VALUE2>;

Yes I think it does the job, it's just not very pretty. It would be
worth putting together an example for both and see how much larger the
'node-per-pin' binding becomes.

My main concern is with the 'pinctrl-entries' property which specifies
the size of the 2-dimensional pinctrl property. Perhaps that at least
could use the device tree hierarchy without much overhead, since it is
only one per device. Also I really like the idea of the board being
able to just specify a single simple 'pmux = <&phandle>' to get its
pinmux for a peripheral.

Looking at the config:

                       config =
                               <PG_DTA   DRIVE_STRENGTH   5>
                               <PG_DTD   DRIVE_STRENGTH   5>
                               <PG_DTA   SLEW_RATE   4>
                               <PG_DTD   SLEW_RATE   8>;

This is 12 bytes + 12 * 4 = 60 bytes. If we do things as:

>>                               drive-strengh = <5>;
>>                               slew-rate = <4>;

this costs 16 bytes per property instead of 12 (33% bigger). This is
64 bytes if you have already paid the node overhead, perhaps 96 if you
haven't.

(Also remember my point about the ease of chopping back the fdt for
constrained systems).

>
>>
>>                               /*
>>                                * We support two states here, active
>>                                * and standby. Properties in these child
>>                                * nodes can override the ones at this level.
>>                                * Drivers can move between states just by
>>                                * making the changes in these nodes.
>>                                */
>>                               state-active {
>>                                       reg = <PMX_CONF_ACTIVE>;
>>                                       tristate = <0>;
>>                               };
>>                               state-standby {
>>                                       reg = <PMX_CONF_STANDBY>;
>>                                       tristate = <1>;
>>                                       drive-strengh = <2>;
>>                               };
>>                       };
>
> This seems like a qood way to represent the alternative mux states for
> the muxes that need them. This is assuming the states have standard
> bindings and not some random names. I still don't know if we need them
> though. And again when we have multiple registers, we'd have to have
> either multiple entries for each pin, or use the array instead of reg.
>
> Maybe we need two bindings: A minimal subset of what Stephen is suggesting
> that can handle 95% of the muxes with minimal overhead, then what you're
> suggesting for the few muxes that need multiple states?

Perhaps that would work, it certainly deals nicely with making the
rare cases less ugly if indeed they are rare. Of course a single
binding that is not too ugly and still reasonably efficient would be
best.

I will have a think about this a bit more and see if anything leaps
out. It's quite an interesting problem...

Regards,
Simon

>
> Regards,
>
> Tony

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Simon Glass <sjg@chromium.org> [120127 07:12]:
> 
> On Thu, Jan 26, 2012 at 6:21 PM, Tony Lindgren <tony@atomide.com> wrote:
> >
> > Maybe we need two bindings: A minimal subset of what Stephen is suggesting
> > that can handle 95% of the muxes with minimal overhead, then what you're
> > suggesting for the few muxes that need multiple states?
> 
> Perhaps that would work, it certainly deals nicely with making the
> rare cases less ugly if indeed they are rare. Of course a single
> binding that is not too ugly and still reasonably efficient would be
> best.
> 
> I will have a think about this a bit more and see if anything leaps
> out. It's quite an interesting problem...

Just to try to recap what has come up so far:

1. We need to avoid bloating things for basic cases to initialize
   several hundred pins.

2. Some muxes need to define multiple states.

3. We need to pass a flag for each mux to know whether or
   not it can be freed after init.

So how about let's do separate static and dynamic bindings,
something like this:

	/*
	 * Static init time only mux where
	 * we only specify phandle to driver
	 * and, offset of the mux, and the value.
	 * These pins are discarded after init.
	 *
	 * Format:	  mux_ctrl      offset value
	 */
	pinctrl-static = <&pmx_driver1  0x0020 0x1245
			   &pmx_driver2 0x0022 0x6578>;

	/*
	 * Dynamic mux where the mux is kept around after
	 * init and multiple states can be defined for
	 * a mux as a subnode of the pinmux controller.
	 *
	 * Format:	   mux_phandle   initial state
	 */ 
	pinctrl-dynamic = <&pmx_sdhci    PMX_STATE_ENABLED
			   &pmx_ehci_xcv PMX_STATE_ENABLED>;

This would make pinctrl-static binding follow the same
standard as GPIO binding and can be parsed easily with
of_parse_phandle_with_args.

Then for pinctrl-dynamic we can make a custom parser,
and the binding can follow the more readable format as
Simon posted.

Regards,

Tony

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Friday, January 27, 2012 10:38 AM:
...
> So how about let's do separate static and dynamic bindings,
> something like this:
> 
> 	/*
> 	 * Static init time only mux where
> 	 * we only specify phandle to driver
> 	 * and, offset of the mux, and the value.
> 	 * These pins are discarded after init.
> 	 *
> 	 * Format:	  mux_ctrl      offset value
> 	 */
> 	pinctrl-static = <&pmx_driver1  0x0020 0x1245
> 			   &pmx_driver2 0x0022 0x6578>;

So those are direct register writes? That sounds pretty scary, and a
royal pain to author the device tree.

If we do go with this, I think we'd need a mask for each register write
too, so you can leave certain bits unaffected; there's no reason to
believe in general that each pin has a dedicated register only for that
pin, or even that only pinmux/config data is in the register.

This also makes it difficult to extract semantic information from the
DT. How can the pinctrl subsystem know which pins are in use and which
aren't here? This is relevant if some module loads later and attempts
to claim some pins - are they already in use by another driver or not?

Now individual pinctrl drivers could interpret those register values
and know that this means pin/group "x" is programmed to mux value "y",
but does that mean pin "x" is actually /used/, or just that the init
table had to program value "y" because the default for that pin is "z"
which conflicted in HW with some other mux setting that the board
needed (e.g. muxing signal "y" to some other pin).

(Put another way, this binding completely bypasses the pinctrl subsystem;
is that OK?)

> 	/*
> 	 * Dynamic mux where the mux is kept around after
> 	 * init and multiple states can be defined for
> 	 * a mux as a subnode of the pinmux controller.
> 	 *
> 	 * Format:	   mux_phandle   initial state
> 	 */
> 	pinctrl-dynamic = <&pmx_sdhci    PMX_STATE_ENABLED
> 			   &pmx_ehci_xcv PMX_STATE_ENABLED>;
> 
> This would make pinctrl-static binding follow the same
> standard as GPIO binding and can be parsed easily with
> of_parse_phandle_with_args.
> 
> Then for pinctrl-dynamic we can make a custom parser,
> and the binding can follow the more readable format as
> Simon posted.

I don't think there's any point in having 2 separate bindings; it's been
hard enough to come up with /one/ binding! If we do go for raw register
writes for the static stuff, we should just do the same for the dynamic
stuff too.

In fact, given this would all bypass the pinctrl subsystem entirely,
perhaps lets not even define a standard format for pinctrl-static or
pinctrl-dynamic, and just have each pin controller driver parse tables
inside its own node, in a format specific to that pin controller's
binding. I already had that working for the static case back in last
August and would love to just apply those patches and be done with this.

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Stephen Warren <swarren@nvidia.com> [120127 09:21]:
> Tony Lindgren wrote at Friday, January 27, 2012 10:38 AM:
> ...
> > So how about let's do separate static and dynamic bindings,
> > something like this:
> > 
> > 	/*
> > 	 * Static init time only mux where
> > 	 * we only specify phandle to driver
> > 	 * and, offset of the mux, and the value.
> > 	 * These pins are discarded after init.
> > 	 *
> > 	 * Format:	  mux_ctrl      offset value
> > 	 */
> > 	pinctrl-static = <&pmx_driver1  0x0020 0x1245
> > 			   &pmx_driver2 0x0022 0x6578>;
> 
> So those are direct register writes? That sounds pretty scary, and a
> royal pain to author the device tree.

Driver specific, either registers or enumeration. It could also
it could be also generic defines, something along the lines of
PMX_MUX_FUNCTION_1 | PMX_MUX_GPIO_INPUT.
 
> If we do go with this, I think we'd need a mask for each register write
> too, so you can leave certain bits unaffected; there's no reason to
> believe in general that each pin has a dedicated register only for that
> pin, or even that only pinmux/config data is in the register.

I'm currently using "pinmux-simple,function-mask", but yeah sounds like
it should be a generic mask name.
 
> This also makes it difficult to extract semantic information from the
> DT. How can the pinctrl subsystem know which pins are in use and which
> aren't here? This is relevant if some module loads later and attempts
> to claim some pins - are they already in use by another driver or not?

We could just have one spinlock for all the discarded pins instead of
having a single spinlock for each discarded pin?
 
> Now individual pinctrl drivers could interpret those register values
> and know that this means pin/group "x" is programmed to mux value "y",
> but does that mean pin "x" is actually /used/, or just that the init
> table had to program value "y" because the default for that pin is "z"
> which conflicted in HW with some other mux setting that the board
> needed (e.g. muxing signal "y" to some other pin).
> 
> (Put another way, this binding completely bypasses the pinctrl subsystem;
> is that OK?)

Well I was thinking we should still register the pins, and have pinctrl
fwk set those values, then discard those pins but still keep them as
locked.
 
> > 	/*
> > 	 * Dynamic mux where the mux is kept around after
> > 	 * init and multiple states can be defined for
> > 	 * a mux as a subnode of the pinmux controller.
> > 	 *
> > 	 * Format:	   mux_phandle   initial state
> > 	 */
> > 	pinctrl-dynamic = <&pmx_sdhci    PMX_STATE_ENABLED
> > 			   &pmx_ehci_xcv PMX_STATE_ENABLED>;
> > 
> > This would make pinctrl-static binding follow the same
> > standard as GPIO binding and can be parsed easily with
> > of_parse_phandle_with_args.
> > 
> > Then for pinctrl-dynamic we can make a custom parser,
> > and the binding can follow the more readable format as
> > Simon posted.
> 
> I don't think there's any point in having 2 separate bindings; it's been
> hard enough to come up with /one/ binding! If we do go for raw register
> writes for the static stuff, we should just do the same for the dynamic
> stuff too.

But then we need to waste a register for the static/dynamic flag for
each mux.. Or make the tree deeper.
 
> In fact, given this would all bypass the pinctrl subsystem entirely,
> perhaps lets not even define a standard format for pinctrl-static or
> pinctrl-dynamic, and just have each pin controller driver parse tables
> inside its own node, in a format specific to that pin controller's
> binding. I already had that working for the static case back in last
> August and would love to just apply those patches and be done with this.

Hmm I don't think it would by pass it, we just need to let pinctrl
fwk deal with the init time only pins too.

For the dynamic pins, note that PMX_STATE_* defines would be one of the
standard states supported by the pinctrl fwk. Then of course where
pmx_sdhci binding is implemented, you could have either hardware specific
register values, enumeration or generic defines depending on how the
pinctrl driver is implemented.

Regards,

Tony 

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 27, 2012 at 09:51:58AM -0800, Stephen Warren wrote:
> I don't think there's any point in having 2 separate bindings; it's been
> hard enough to come up with /one/ binding!

+1

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 27, 2012 at 07:43:36AM -0800, Simon Glass wrote:
...
> The cost of the pmx@dta node is about 12 bytes for the header (it
> depends on the length of the name), and each of the properties above
> is 16 bytes. So in total this node is 76 bytes. If we have 250 pins
> being muxed as Tegra3 then this is about 20KB (including a bit of
> slack for longer names). My point about being able to 'optimise out'
> some of these remains, though, but probably not for the kernel.
> 
> Stephen's 'mux' property uses 12 bytes plus 8 bytes per pin/group (I
> am removing the prefixes):
> 
>                        mux =
>                                <PG_DTA   MUX_SDIO1>
>                                <PG_DTD   MUX_SDIO1>;
> 
> so 28 bytes. What I proposed would use (12 + 2 * 16) per pin/group, or
> 44 bytes (60% bigger):
> 
It's not only about size but also run-time tree travelling efficiency.
Your proposal requires every single pin show as a node in device tree.
Looking at these for_each_node_by_*() APIs in include/linux/of.h, you
might agree we should avoid bloating device tree with so many nodes.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Shawn Guo <shawn.guo@linaro.org> [120129 18:30]:
> On Fri, Jan 27, 2012 at 07:43:36AM -0800, Simon Glass wrote:
> ...
> > The cost of the pmx@dta node is about 12 bytes for the header (it
> > depends on the length of the name), and each of the properties above
> > is 16 bytes. So in total this node is 76 bytes. If we have 250 pins
> > being muxed as Tegra3 then this is about 20KB (including a bit of
> > slack for longer names). My point about being able to 'optimise out'
> > some of these remains, though, but probably not for the kernel.
> > 
> > Stephen's 'mux' property uses 12 bytes plus 8 bytes per pin/group (I
> > am removing the prefixes):
> > 
> >                        mux =
> >                                <PG_DTA   MUX_SDIO1>
> >                                <PG_DTD   MUX_SDIO1>;
> > 
> > so 28 bytes. What I proposed would use (12 + 2 * 16) per pin/group, or
> > 44 bytes (60% bigger):
> > 
> It's not only about size but also run-time tree travelling efficiency.
> Your proposal requires every single pin show as a node in device tree.
> Looking at these for_each_node_by_*() APIs in include/linux/of.h, you
> might agree we should avoid bloating device tree with so many nodes.

And that's why I'm suggesting two bindings: A minimal pinctrl-static
binding and more verbose pinctrl-dynamic binding.

AFAIK the number of pinctrl-dynamic bindings needed are just a fraction
of the pinctrl-static bindings. So the extra parsing needed for a few
pinctrl-dynamic bindings should not matter.

Sure it would be nice to have it all in a single binding, but these
bindings have conflicting requirements. So it may not be possible to
do it in a single binding in an efficient way.

Regards,

Tony

RE: Pinmux bindings proposal V2

[Stephen Warren]

Tony Lindgren wrote at Thursday, January 26, 2012 7:21 PM:
> * Simon Glass <sjg@chromium.org> [120126 09:11]:
> >
> > On Fri, Jan 20, 2012 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
> >
> > 1. It doesn't seem to make full use of the device tree format. For example,
> >
> >    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> >
> > would be better as something like
> >
> >     drive-strength = <5>;
> >
> > if we could arrange it. It also reduces the need for these
> > TEGRA_PMX_CONF_DRIVE_STRENGTH defines.
> 
> I agree. This is something that most pinmux/pinconf drivers need to
> implement, so it's best done in a generic way.

Yet:

* Some controllers don't have a "drive strength" property
* Others have a single "drive strength" property
* Others configure drive strength separately for driving a signal high
or low.

Hence, representing this in a generic fashion doesn't seem possible to
me, except through (key, value) pairs where the individual drivers or
bindings define what the keys are.

-- 
nvpublic

RE: Pinmux bindings proposal V2

[Stephen Warren]

Simon Glass wrote at Thursday, January 26, 2012 10:42 AM:
> On Fri, Jan 20, 2012 at 2:22 PM, Stephen Warren <swarren@nvidia.com> wrote:
> > Here's V2 of my pinmux binding proposal, after incorporating some feedback
> > from Shawn Guo and Dong Aisheng. Main changes:
> > [large quote elided; see https://lkml.org/lkml/2012/1/20/310 for content]

> I have been through this carefully and thought about how this might be
> implemented. Your example has really helped me to understand this
> properly, as the 70-email-long thread was a huge amount of reading.
> 
> So I have the following comments in general:
> 
> 1. It doesn't seem to make full use of the device tree format. For example,
> 
>    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> 
> would be better as something like
> 
>     drive-strength = <5>;
> 
> if we could arrange it. It also reduces the need for these
> TEGRA_PMX_CONF_DRIVE_STRENGTH defines.

Yes I can see the argument this is more readable.

However, it:

* Requires a lot of string handling when parsing the device tree, since
  you have to search for lots of individual properties by name.

* Bloats the device tree quite a bit due to representing each parameter
  as a separate property, with a longish name, rather than a single u32
  cell in the config property I proposed.

* Makes the pinmux binding vary much more between different SoCs, since
  each SoC will have a different set of configuration parameters that
  can be applied to each pin/group, and hence will have a different set
  of property names and value formats.

  I'm assuming (and nobody has contradicted in this DT binding discussion
  or the pinctrl API discussion that I can recall) that the properties on
  all SoCs can be represented as a list of (parameter, value) pairs, with
  the SoC defining the list of legal parameters.

Still, the binding proposal does rely on named constants in dtc to be
really readable, and I have no idea if/when they will appear...

> 2. The TEGRA_PMX_ prefix on everything seems unnecessary to me and
> makes it harder to read and more long-winded. Other SOCs will not
> include the same header, I think. Can we just drop this?

I'd rather not due to namespace conflict reasons; it's the only way you
can guarantee that a system with a pinmux on both the primary SoC and
on some peripheral device won't both define the same named constatns;
exactly the same as a C header file.

> 3. To me, the pinctrl-entries following by pinctrl also does not make
> great use of the device tree. It seems to me that this:
> 
> pinctrl-entries = <2 2>;
> pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
>          <&pmx_sdhci> <&pmx_sdhci_standby>;
> 
> where you say:
> >                 * Here, state "active" covers 2 entries (0 and 1) in pinctrl,
> >                 * and state "suspend" covers 2 more entries (2 and 3) in
> >                 * pinctrl.
> 
> might be better as something like:
> 
> state-active {
>        mux =  <&pmx_sdhci> <&pmx_sdhci_active>;
> };
> 
> state-standby {
>        mux =  <&pmx_sdhci> <&pmx_sdhci_standby>;
> };
> 
> which means that we don't need to represent a 2-dimensional array in a
> single property, which seems error-prone and complicated. In general I
> wonder if we can make more use of hierarchy. This might help it be
> more extensible in future also.

Yes, that's simpler.

However, that then requires name-based access.

Other bindings that support named access also support access based on
integer IDs. So, in:

pinctrl-entries = <2 2>;
pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
          <&pmx_sdhci> <&pmx_sdhci_standby>;
pinctrl-names = "active" "standby";

You could either:

a) Search for name "active" in the pinctrl-names array, then used the
index where it was found to search by index.

b) Search directly by index in pinctrl-entries array. If only supporting
search-by-index, the pinctrl-names property could be elided.

With the alternative form you proposed, you could only search by index
if you forced a pinctrl-names to always exist, so you could map the index
to a name and then form the new property name to look for.

Perhaps it's reasonable to only allow search-by-name though. IIRC, this
wasn't acceptable for other bindings though.

Finally, rather than:

    state-active {
        mux =  <&pmx_sdhci> <&pmx_sdhci_active>;
    };

The following would be much simpler to parse:

    pinctrl-mux-active = <&pmx_sdhci> <&pmx_sdhci_active>;
    pinctrl-config-active = <&pmx_sdhci> <&pmx_sdhci_active>;

since there'd be one less node. It should be equally extensible to
adding new stuff (other than mux/config) to the binding if needed,
although admittedly if we really think that's likely (I don't since
the config parameters should be able to represent anything) we probably
should put all that stuff in a node as you say just to group stuff
together.

> 4. Your example for different 'states' is clear, but I'm not sure if
> you are intending to handling actual different pin assignments in
> these 'states'. To me this is the big point: does SDIO1 come out pin
> groups DTA and DTC; or does it come out pin groups SLXA-D. That's what
> the boards want to select. I think the state side of it may be best as
> a separate thing, which drivers may even just handle themselves if
> they are basic implementations. Or they may just want to implement the
> active state (U-Boot).

In the main, I think the multiple states are a runtime thing, so typically
would use the same set of pins.

> 5. My previous email suggested that the SOC define several possible
> options for each peripheral so that the board vendor can just select
> them from a menu. All the device tree complexity would then live in
> the SOC file (tegra20.dtsi in this case) and very little in the board
> file. I still think that is a worthwhile goal. You have this in there,
> but I feel it could be simplified further, to a single phandle
> perhaps.

For any single named state, you can already construct the definitions of
those named states to be fully within a single node and single phandle.

So you're asking for a single phandle to represent the entire set of
named states. That seems even more complex; introducing another level
of node nesting to define the set of states and still allow them to be
pieced together from common parts so as not to duplicate everything,
unless we don't care about that any more.

> 6. Looking ahead I imagine that the pinmux nodes will become large. I
> don't know how large but looking at some of the kernel bindings for TI
> I do get a little alarmed. Board vendors may wish to try to cut down
> the size of the device tree blob, particularly for use in the boot
> loader. With the binding in your email I think that would be hard to
> do. In particular if people know that standby is not being used in the
> boot loader, I think they would need to do things like change:
> 
> pinctrl-entries = <2 2>;
> pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>
>          <&pmx_sdhci> <&pmx_sdhci_standby>;
> 
> to:
> 
> pinctrl-entries = <2>;
> pinctrl = <&pmx_sdhci> <&pmx_sdhci_active>;

I don't think that makes sense.

You can either:

a) Feed the same DT into both the bootloader and the kernel. There's
no duplication here, and the DT size is the superset of anything needed
by the bootloader and the kernel.

b) Create a cut-down subset of the DT to feed to the bootloader. In this
case, everything that's common is duplicated. At worst, you take 2x the
size to store both DTs. At best, you take 1.xx% where xx% is the common
part and obviously > 0. I don't see where the space saving is here.

...
> So, rather than just provide vague feedback, I though I would try to
> modify your binding according to the above, so it is below,

[large example elided; see https://lkml.org/lkml/2012/1/26/308]

That looks pretty nice and readable. The issues I see are:

* Bloat of DT due using strings to represent each configuration parameter
as a property name, putting each param in a different property, etc.

* There's no use of phandles within each state definition node, so
there's no way to piece together common partial pieces of a state. So,
if two nodes are 99% identical but simply have e.g. a slightly different
board-specific drive strength setting, you need to cut/paste the whole
thing rather than referencing the common portion from two places and
providing the differences. Note, I'm not talking about differences
between 2 named states, but between two boards' usage of SDIO1 routed
to DTA+DTD for example.

Perhaps there are ways to overcome those issues?

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Jan 27, 2012 at 09:29:50AM -0800, Stephen Warren wrote:
> Simon Glass wrote at Thursday, January 26, 2012 10:42 AM:
...
> > 1. It doesn't seem to make full use of the device tree format. For example,
> > 
> >    <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
> > 
> > would be better as something like
> > 
> >     drive-strength = <5>;
> > 
> > if we could arrange it. It also reduces the need for these
> > TEGRA_PMX_CONF_DRIVE_STRENGTH defines.
> 
> Yes I can see the argument this is more readable.
> 
> However, it:
> 
> * Requires a lot of string handling when parsing the device tree, since
>   you have to search for lots of individual properties by name.
> 
> * Bloats the device tree quite a bit due to representing each parameter
>   as a separate property, with a longish name, rather than a single u32
>   cell in the config property I proposed.
> 
It bloats device tree more due to the proposal needs to represent every
single muxable entity (pin for imx case) as a node to accommodate the
properties like 'drive-strength' here.

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Shawn Guo]

Hi Stephen,

I had a talk with Dong about this binding, and we think that it should
work well for imx if we have a couple of small pieces added.

On Fri, Jan 20, 2012 at 02:22:20PM -0800, Stephen Warren wrote:
...
>                 pmx_sdhci: pinconfig-sdhci {
>                         /*
>                          * The mux property is a list of muxable entities
>                          * and the mux function to select for it. The number
>                          * of cells in each entry is the pin controller's
>                          * #pinmux-cells property. The pin controller's
>                          * binding defines what the cells mean. The pinctrl
>                          * driver is responsible for mapping this data to
>                          * the (group, function) pair required to fill in
>                          * the pinctrl subsystem's pinmux mapping table.
>                          */
>                         mux =
>                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
>                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;

We need a property like 'mux-unit' whose value can be either 'pin' or
'pingroup' to reflect something you mentioned as muxable entity.

The reason behind this is the DT logic inside pinctrl core needs to
know how the pinmux_map should be constructed from device tree.  In
tegra case, the 'mux-unit' is 'pingroup', the core should construct
pinmux_map entry for each row/element of 'mux'.  In imx case, the
'mux-unit' will be 'pin', and we would expect core construct only
one pinmux_map entry there, with all the pins listed in 'mux' composing
the group that pinmux_map needs.

And in case of 'mux-unit' is 'pin', we would need one more property
'mux-name' to present the group name.  Then we have all the pieces to
construct pinmux_map for cases like imx, where we do not define all
those functions, groups in pinctrl driver at all.

Regards,
Shawn

>                         /*
>                          * The config property is a list of muxable entities
>                          * and individual configuration setting. The number
>                          * of cells in each entry is the pin controller's
>                          * #pinconfig-cells property. The pin controller's
>                          * binding defines what the cells mean. The pinctrl
>                          * driver is responsible for mapping this data to
>                          * the (group, config) pair required to fill in
>                          * the pinctrl subsystem's pin configuration table.
>                          */
>                         config =
>                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
>                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_DRIVE_STRENGTH 5>
>                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_CONF_SLEW_RATE 4>
>                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_CONF_SLEW_RATE 8>;
>                 };

RE: Pinmux bindings proposal V2

[Stephen Warren]

Shawn Guo wrote at Wednesday, February 01, 2012 7:36 AM:
...
> I had a talk with Dong about this binding, and we think that it should
> work well for imx if we have a couple of small pieces added.
> 
> On Fri, Jan 20, 2012 at 02:22:20PM -0800, Stephen Warren wrote:
> ...
> >                 pmx_sdhci: pinconfig-sdhci {
> >                         /*
> >                          * The mux property is a list of muxable entities
> >                          * and the mux function to select for it. The number
> >                          * of cells in each entry is the pin controller's
> >                          * #pinmux-cells property. The pin controller's
> >                          * binding defines what the cells mean. The pinctrl
> >                          * driver is responsible for mapping this data to
> >                          * the (group, function) pair required to fill in
> >                          * the pinctrl subsystem's pinmux mapping table.
> >                          */
> >                         mux =
> >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
> >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
> 
> We need a property like 'mux-unit' whose value can be either 'pin' or
> 'pingroup' to reflect something you mentioned as muxable entity.

I'm not sure I agree; see below.

> The reason behind this is the DT logic inside pinctrl core needs to
> know how the pinmux_map should be constructed from device tree.

As a general statement, yes.

> In tegra case, the 'mux-unit' is 'pingroup', the core should construct
> pinmux_map entry for each row/element of 'mux'.

Yes.

> In imx case, the 'mux-unit' will be 'pin',

OK.

Just a note: Tegra30 also has per-pin muxability. Only Tegra20 muxes pins
in groups. (although Tegra30 does some if its pin configuration in groups)

> and we would expect core construct only
> one pinmux_map entry there, with all the pins listed in 'mux' composing
> the group that pinmux_map needs.

This is where I disagree.

If the pinmux_map should only contain a single entry, wouldn't the DT
mux property only contain a single entry?

The reason being that if there's a single entry in the pinmux_map, the
group name used in that entry must be a group that's supported directly
by the pinctrl driver (that's just the way pinctrl works). As such, why
not just write the device tree in terms of those groups?

The only way I can see this not being true is if your pinctrl driver is
also parsing these mux properties, and dynamically creating the groups
that it exposes based on the list of pins in the mux property. However,
that seems like the wrong approach; If you're dynamically defining groups
in DT, I'd expect separate explicit driver-specific properties/nodes to
define those groups, such that the pinctrl core's processing of the mux
property to be identical in all cases.

i.e. instead of what your "mux-unit" proposal:

    pmx_sdhci: pinconfig-sdhci {
        mux =
            <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
            <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
        mux-unit = "pingroup";
        mux-name = "sdio-config-1";
    };

I'd expect something more like:

    /* Standardized pinctrl properties */
    pmx_sdhci: pinconfig-sdhci {
        mux = <IMX_PG_SDIO_CFG_1 IMX_MUX_SDIO1>;
    };

    /*
     * Driver-specific properties which tell the driver which potentially
     * board-specific pin-groups to implement.
     */
    imx-pingroup-sdio-cfg-1 {
        id = <IMX_PG_SDIO_CFG_1>;
        pins = <IMX_PIN_SDIO1_CLK IMX_PIN_SDIO1_CMD IMX_PIN_SDIO1_DAT0...>;
    };

Does that make sense?

> And in case of 'mux-unit' is 'pin', we would need one more property
> 'mux-name' to present the group name.  Then we have all the pieces to
> construct pinmux_map for cases like imx, where we do not define all
> those functions, groups in pinctrl driver at all.

-- 
nvpublic

Re: Pinmux bindings proposal V2

[Dong Aisheng]

On Fri, Feb 3, 2012 at 2:36 AM, Stephen Warren <swarren@nvidia.com> wrote:
> Shawn Guo wrote at Wednesday, February 01, 2012 7:36 AM:
> ...
>> I had a talk with Dong about this binding, and we think that it should
>> work well for imx if we have a couple of small pieces added.
>>
>> On Fri, Jan 20, 2012 at 02:22:20PM -0800, Stephen Warren wrote:
>> ...
>> >                 pmx_sdhci: pinconfig-sdhci {
>> >                         /*
>> >                          * The mux property is a list of muxable entities
>> >                          * and the mux function to select for it. The number
>> >                          * of cells in each entry is the pin controller's
>> >                          * #pinmux-cells property. The pin controller's
>> >                          * binding defines what the cells mean. The pinctrl
>> >                          * driver is responsible for mapping this data to
>> >                          * the (group, function) pair required to fill in
>> >                          * the pinctrl subsystem's pinmux mapping table.
>> >                          */
>> >                         mux =
>> >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
>> >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
>>
>> We need a property like 'mux-unit' whose value can be either 'pin' or
>> 'pingroup' to reflect something you mentioned as muxable entity.
>
First i'd explain more about this way for better discussion.

It could be:
pinctrl_usdhc4: pinconfig-usdhc4 {
        /* 0: pin 1: group */
        mux-entity = <0>;
        func-name = "usdhc4func";
        grp-name = "usdhc4grp";
        mux =
                <MX6Q_SD4_CMD  0>
                <MX6Q_SD4_CLK  0>
                <MX6Q_SD4_DAT0 1>
                <MX6Q_SD4_DAT1 1>
                <MX6Q_SD4_DAT2 1>
                <MX6Q_SD4_DAT3 1>
                <MX6Q_SD4_DAT4 1>
                <MX6Q_SD4_DAT5 1>
                <MX6Q_SD4_DAT6 1>
                <MX6Q_SD4_DAT7 1>;
        config =
                <MX6Q_SD4_CMD  MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_CLK  MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT0 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT2 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT3 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT4 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT5 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT6 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT7 MX6Q_USDHC_PAD_CTRL>;
};

Actually i think i'd rather do not use config property, then i could
be more compact:
(anyway it's another issue and is flexible to be controlled by #pinmux-cells)
pinctrl_usdhc4: pinconfig-usdhc4 {
        /* 0: pin 1: group */
        mux-entity = <0>;
        func-name = "usdhc4func";
        grp-name = "usdhc4grp";
        mux =
                <MX6Q_SD4_CMD  0 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_CLK  0 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT0 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT1 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT2 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT3 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT4 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT5 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT6 1 MX6Q_USDHC_PAD_CTRL>
                <MX6Q_SD4_DAT7 1 MX6Q_USDHC_PAD_CTRL>;
};

The idea is that pinctrl-imx driver will parse all pinmux nodes like
pinctrl_usdhc4 to create the pinctrl functions and groups during
system boot up. For IMX, the mux entity is PIN
then we will treat all PINS in mux property is only on group. The
group name is grp-name##grp and the function name is mux-name##func.
Then we have predefined functions and groups.

When do pinmux map parsing during pinmux_get calling, the pinctrl core
will handle the two different cases for different muxable entity by
checking the mux-entity value.
If entity is PIN, all pins are a group and only one pinmux map
created, if entity is group,
each entry is a group and one map per group.

> I'm not sure I agree; see below.
>
>> The reason behind this is the DT logic inside pinctrl core needs to
>> know how the pinmux_map should be constructed from device tree.
>
> As a general statement, yes.
>
>> In tegra case, the 'mux-unit' is 'pingroup', the core should construct
>> pinmux_map entry for each row/element of 'mux'.
>
> Yes.
>
>> In imx case, the 'mux-unit' will be 'pin',
>
> OK.
>
> Just a note: Tegra30 also has per-pin muxability. Only Tegra20 muxes pins
> in groups. (although Tegra30 does some if its pin configuration in groups)
>
>> and we would expect core construct only
>> one pinmux_map entry there, with all the pins listed in 'mux' composing
>> the group that pinmux_map needs.
>
> This is where I disagree.
>
> If the pinmux_map should only contain a single entry, wouldn't the DT
> mux property only contain a single entry?
>
The reason is that the single entry of IMX is a PIN rather than a
group as tegra.

> The reason being that if there's a single entry in the pinmux_map, the
> group name used in that entry must be a group that's supported directly
> by the pinctrl driver (that's just the way pinctrl works). As such, why
> not just write the device tree in terms of those groups?
>
Because with the format:
<muxable_entity  muxed_function>
The muxed_function applies for the whole muxable_entity.
If we use virtual group for muxable_entity here, for imx, we can not
use one muxed_function for whole pins in that group.
That means we need a more complicated data for muxed_function to represent
all function for each pin in that virtual group.

> The only way I can see this not being true is if your pinctrl driver is
> also parsing these mux properties, and dynamically creating the groups
> that it exposes based on the list of pins in the mux property.
Yes.

>  However,
> that seems like the wrong approach; If you're dynamically defining groups
> in DT, I'd expect separate explicit driver-specific properties/nodes to
> define those groups, such that the pinctrl core's processing of the mux
> property to be identical in all cases.
>
> i.e. instead of what your "mux-unit" proposal:
>
>    pmx_sdhci: pinconfig-sdhci {
>        mux =
>            <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
>            <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
>        mux-unit = "pingroup";
>        mux-name = "sdio-config-1";
>    };
>
> I'd expect something more like:
>
>    /* Standardized pinctrl properties */
>    pmx_sdhci: pinconfig-sdhci {
>        mux = <IMX_PG_SDIO_CFG_1 IMX_MUX_SDIO1>;
>    };
>
>    /*
>     * Driver-specific properties which tell the driver which potentially
>     * board-specific pin-groups to implement.
>     */
>    imx-pingroup-sdio-cfg-1 {
>        id = <IMX_PG_SDIO_CFG_1>;
>        pins = <IMX_PIN_SDIO1_CLK IMX_PIN_SDIO1_CMD IMX_PIN_SDIO1_DAT0...>;
>    };
>
> Does that make sense?
>
As i explained above, we can not represent a function with one single
data IMX_MUX_SDIO1 for all pins in the group IMX_PG_SDIO_CFG_1.

A similar way i did before is:
imx-pingroup-sdio-cfg-1 {
     id = <IMX_PG_SDIO_CFG_1>;
    pins = <IMX_PIN_SDIO1_CLK mux_1>
              <IMX_PIN_SDIO1_CMD mux_2...>;
};

pmx_sdhci: pinconfig-sdhci {
   mux = <IMX_PG_SDIO_CFG_1>;
};

And i prefered IMX_PG_SDIO_CFG_1 to be a phandle to imx-pingroup-sdio-cfg-1.

The real case i have done before is that:
soc.dtsi:

iomuxc@020e0000 {
        compatible = "fsl,imx6q-iomuxc";
        reg = <0x020e0000 0x4000>;
        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>;
                };
        };

        pinmux-functions {
                uart4func: func@0 {
                        func-name = "uart4";
                        groups = <&uart4grp>;
                };

                sd4func: func@1 {
                        func-name = "sd4";
                        groups = <&sd4grp>;
                };
        };
};

board.dts:

uart3: uart@021f0000 { /* UART4 */
        status = "okay";
        pinmux = <&sd4grp>;
};

To be more like the way you proposed, it could be:
board.dts:
pmx_usdhc4: pinconfig-usdhc4 {
        mux = <&sd4grp>
        /* we can add a similar config node for each pin*/
        config = <&sd4config>
};

uart3: uart@021f0000 { /* UART4 */
        status = "okay";
        pinmux = <&pmx_usdhc4>;
};

This is a working way and i have discussed it with Shawn before.
However, since these things are a little pinctrl core specific,
so more or less we're a little more like the data model you proposed
for Tegra since it represents hw better.
And the pinmux map parsing is done in pinctrl core, we also want to
align the binding as tegra.
That why we consider keep using your proposed data model and find a
new way(introduce mux_entity and create pinmux map differently) to use
virtual group for IMX.

However If IMX uses the data model i described above, the binding is
then a little different from tegra. that means we may need to change
to  let each soc's pinctrl driver do real pinmux map parsing (maybe
add a callback in pinctrl.ops) based on their soc specific pinctrl
configuration node like pmx_usdhc4 above instread of let pinctrl core
do a standard pinmux map parsing which is our target we discussed so
long for.

Regards
Dong Aisheng

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Fri, Feb 03, 2012 at 04:07:23AM +0800, Dong Aisheng wrote:
...
> However If IMX uses the data model i described above, the binding is
> then a little different from tegra. that means we may need to change
> to  let each soc's pinctrl driver do real pinmux map parsing (maybe
> add a callback in pinctrl.ops) based on their soc specific pinctrl
> configuration node like pmx_usdhc4 above instread of let pinctrl core
> do a standard pinmux map parsing which is our target we discussed so
> long for.
> 
Yeah, this seems a reasonable alternative to me.  Pushing the pinmux_map
construction down to individual pinctrl driver, who can interpret 'mux'
property best, will relieve the pinctrl core from understanding the
property for different cases, so that we can even save the property
'mux-unit' I asked for.  And doing so somehow aligns with non-dt case,
where the pinmux_map is constructed by individual pinctrl driver too
(with help from PINMUX_MAP_* macros in board file).

-- 
Regards,
Shawn

Re: Pinmux bindings proposal V2

[Dong Aisheng]

On Fri, Feb 3, 2012 at 10:02 PM, Shawn Guo <shawn.guo@linaro.org> wrote:
> On Fri, Feb 03, 2012 at 04:07:23AM +0800, Dong Aisheng wrote:
> ...
>> However If IMX uses the data model i described above, the binding is
>> then a little different from tegra. that means we may need to change
>> to  let each soc's pinctrl driver do real pinmux map parsing (maybe
>> add a callback in pinctrl.ops) based on their soc specific pinctrl
>> configuration node like pmx_usdhc4 above instread of let pinctrl core
>> do a standard pinmux map parsing which is our target we discussed so
>> long for.
>>
> Yeah, this seems a reasonable alternative to me.  Pushing the pinmux_map
> construction down to individual pinctrl driver, who can interpret 'mux'
> property best, will relieve the pinctrl core from understanding the
> property for different cases, so that we can even save the property
> 'mux-unit' I asked for.  And doing so somehow aligns with non-dt case,
> where the pinmux_map is constructed by individual pinctrl driver too
> (with help from PINMUX_MAP_* macros in board file).
>
Yes, that's i think may be right way we should go now.
The reasons are as we met so many issues and discussed so long, we
feel like that it's hard to make a standard dt binding in pintrl core
based on one mux property format to cover all the differences for
different SOCs likeTegra(using HW group) and IMX(using virtual group).
We need to fix these issues on a higher level that each SOC pinctrl
driver implements their pinmux map parsing function, pinctrl core
calls these functions to get the pinmux maps the driver parsed instead
of pinctrl core parsing itself.
It could be something like:
in pinmux_get() {
    struct pinmux_map *maps;
    u32 num;
    ...
    maps = pinctrl.ops.get_pinmux_map(pinmux_node, &num);
    ret = pinmux_map_register(maps, num);
    ...
    ret = pinmux_enable_maps(maps, num);
}

The basic binding idea for this way is:
1) pinctrl core is only responsible for the pinmux map parsing.
Each pinctrl driver is responsible for the function and group creation
no matter whether they're defined in drivers or parsed from device
tree.
2) The pinctrl driver is responsible for their pinmux node definitions
to cover their hw difference. However we prefer to reference the
format as Stephen proposed to align each SOC best. And each pinctrl
driver is responsible for the real pinmux map parsing by implementing
the pinmux_map_parse callback function.
3) when calling pinmux_get, we dynamically parse and create the pinmux
map table for each device.
4) the hog_on_boot map nodes are defined under SoC's pinctrl device node.
The pinctrl driver is responsible for parsing and register them.

Generally speaking, this way is very flexible and can cover most
differences between each SoCs.
It should work for both IMX and Tegra well. Tegra can still do on the
same binding as before as Stephen proposed. The only difference is
that the real pinmux map parsing is moved from pinctrl core to Tegra
pinctrl driver. And IMX can still use virtual group as we wished(i
think this is correct since pinctrl subsystem does not have a
restriction that user can only use pure hw group).
For other SOCs, i guess it should also work.

Regards
Dong Aisheng

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Dong Aisheng <dongas86@gmail.com> [120202 11:36]:
> 
> Actually i think i'd rather do not use config property, then i could
> be more compact:
> (anyway it's another issue and is flexible to be controlled by #pinmux-cells)
> pinctrl_usdhc4: pinconfig-usdhc4 {
>         /* 0: pin 1: group */
>         mux-entity = <0>;
>         func-name = "usdhc4func";
>         grp-name = "usdhc4grp";

The func-name and grp-name should be optional here.
This mux entry is already the group, and can be used as
the group name. And the function name can be generated
dynamically in most cases. I'm currently using np->full_name
of the driver claiming these pins as the function name.

>         mux =
>                 <MX6Q_SD4_CMD  0 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_CLK  0 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT0 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT1 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT2 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT3 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT4 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT5 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT6 1 MX6Q_USDHC_PAD_CTRL>
>                 <MX6Q_SD4_DAT7 1 MX6Q_USDHC_PAD_CTRL>;
> };

For listing basic pins this format works fine for me. It seems
to have low overhead for parsing. And the width of the array
can be driver specific.

Looks like it's the binding for altenative states that's still a
bit open..

So how about let's first standardize on the mux format above?

Then we can enhance it later for describing alternative states?
 
Regards,

Tony

Re: Pinmux bindings proposal V2

[Dong Aisheng]

On Sat, Feb 4, 2012 at 1:32 AM, Tony Lindgren <tony@atomide.com> wrote:
> * Dong Aisheng <dongas86@gmail.com> [120202 11:36]:
>>
>> Actually i think i'd rather do not use config property, then i could
>> be more compact:
>> (anyway it's another issue and is flexible to be controlled by #pinmux-cells)
>> pinctrl_usdhc4: pinconfig-usdhc4 {
>>         /* 0: pin 1: group */
>>         mux-entity = <0>;
>>         func-name = "usdhc4func";
>>         grp-name = "usdhc4grp";
>
> The func-name and grp-name should be optional here.
> This mux entry is already the group, and can be used as
> the group name.
For the case i discussed here, the mux entry is PIN.
(the mux-entity value is 0).
we introduce this value here for treating all pins is one group.
When do map parsing, only one pinmux map will be created.
So we need a grp-name.
And we also need a func-name here for construct pinmux map.

> And the function name can be generated
> dynamically in most cases. I'm currently using np->full_name
> of the driver claiming these pins as the function name.
>
Why i did not use np->name as function name is because the np->name
can be different
while actually these nodes may represent the same function but just
different pins, so the function name should be same.

>>         mux =
>>                 <MX6Q_SD4_CMD  0 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_CLK  0 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT0 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT1 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT2 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT3 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT4 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT5 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT6 1 MX6Q_USDHC_PAD_CTRL>
>>                 <MX6Q_SD4_DAT7 1 MX6Q_USDHC_PAD_CTRL>;
>> };
>
> For listing basic pins this format works fine for me. It seems
> to have low overhead for parsing. And the width of the array
> can be driver specific.
>
> Looks like it's the binding for altenative states that's still a
> bit open..
>
Yes, it does not have states support.

> So how about let's first standardize on the mux format above?
>
I'm afraid it may be hard for us to standardize the mux format for a
standard binding in pinctrl core due to  hw difference.
I'm think the new way which i sent in this thread after the mail you replied.
You can refer to them to see if it's reasonable for you too.

Regards
Dong Aisheng

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Dong Aisheng <dongas86@gmail.com> [120203 09:42]:
> On Sat, Feb 4, 2012 at 1:32 AM, Tony Lindgren <tony@atomide.com> wrote:
> > * Dong Aisheng <dongas86@gmail.com> [120202 11:36]:
> >>
> >> Actually i think i'd rather do not use config property, then i could
> >> be more compact:
> >> (anyway it's another issue and is flexible to be controlled by #pinmux-cells)
> >> pinctrl_usdhc4: pinconfig-usdhc4 {
> >>         /* 0: pin 1: group */
> >>         mux-entity = <0>;
> >>         func-name = "usdhc4func";
> >>         grp-name = "usdhc4grp";
> >
> > The func-name and grp-name should be optional here.
> > This mux entry is already the group, and can be used as
> > the group name.
> For the case i discussed here, the mux entry is PIN.
> (the mux-entity value is 0).
> we introduce this value here for treating all pins is one group.
> When do map parsing, only one pinmux map will be created.
> So we need a grp-name.
> And we also need a func-name here for construct pinmux map.

Sounds like a similar setup I have, I do not need any func-name
or grp-name though.. The group name is already unique with
pinconfig-usdhc4 in your example?
 
> > And the function name can be generated
> > dynamically in most cases. I'm currently using np->full_name
> > of the driver claiming these pins as the function name.
>
> Why i did not use np->name as function name is because the np->name
> can be different
> while actually these nodes may represent the same function but just
> different pins, so the function name should be same.

I used a function name based on the group name initially
(pinconfig-usdhc4 in your example), then renamed it to the
np->full_name of the device requesting the mux when the
mux was found ;)

> >>         mux =
> >>                 <MX6Q_SD4_CMD  0 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_CLK  0 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT0 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT1 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT2 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT3 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT4 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT5 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT6 1 MX6Q_USDHC_PAD_CTRL>
> >>                 <MX6Q_SD4_DAT7 1 MX6Q_USDHC_PAD_CTRL>;
> >> };
> >
> > For listing basic pins this format works fine for me. It seems
> > to have low overhead for parsing. And the width of the array
> > can be driver specific.
> >
> > Looks like it's the binding for altenative states that's still a
> > bit open..
> >
> Yes, it does not have states support.
> 
> > So how about let's first standardize on the mux format above?
> >
> I'm afraid it may be hard for us to standardize the mux format for a
> standard binding in pinctrl core due to  hw difference.

Yes the width would have to be hardare specific for the array.

> I'm think the new way which i sent in this thread after the mail you replied.
> You can refer to them to see if it's reasonable for you too.

Hmm, sorry now I'm confused. Got a link for that mail as so
many have been posted?

Regards,

Tony

Re: Pinmux bindings proposal V2

[Dong Aisheng]

On Sat, Feb 4, 2012 at 5:05 AM, Tony Lindgren <tony@atomide.com> wrote:
....
>> I'm think the new way which i sent in this thread after the mail you replied.
>> You can refer to them to see if it's reasonable for you too.
>
> Hmm, sorry now I'm confused. Got a link for that mail as so
> many have been posted?
>
Pls see:
https://lkml.org/lkml/2012/2/3/276

For a simple example, you can refer to.
https://lkml.org/lkml/2012/2/2/405

Shawn and me will attend Linaro connect meeting on Feb 6th.
If you also attend, we may discuss it together with Linus and Stephen.

Regards
Dong Aisheng

Re: Pinmux bindings proposal V2

[Tony Lindgren]

* Dong Aisheng <dongas86@gmail.com> [120204 08:24]:
> On Sat, Feb 4, 2012 at 5:05 AM, Tony Lindgren <tony@atomide.com> wrote:
> ....
> >> I'm think the new way which i sent in this thread after the mail you replied.
> >> You can refer to them to see if it's reasonable for you too.
> >
> > Hmm, sorry now I'm confused. Got a link for that mail as so
> > many have been posted?
> >
> Pls see:
> https://lkml.org/lkml/2012/2/3/276
> 
> For a simple example, you can refer to.
> https://lkml.org/lkml/2012/2/2/405

Thanks for clarifying. Yes I'm using something that's a subset
of that already, except I'm creating the functions and groups
automatically.

> Shawn and me will attend Linaro connect meeting on Feb 6th.
> If you also attend, we may discuss it together with Linus and Stephen.

Yes let's talk more there, I won't be there until on Tuesday
the 7th though.

Regards,

Tony

Re: Pinmux bindings proposal V2

[Shawn Guo]

On Thu, Feb 02, 2012 at 10:36:54AM -0800, Stephen Warren wrote:
> Shawn Guo wrote at Wednesday, February 01, 2012 7:36 AM:
> ...
> > I had a talk with Dong about this binding, and we think that it should
> > work well for imx if we have a couple of small pieces added.
> > 
> > On Fri, Jan 20, 2012 at 02:22:20PM -0800, Stephen Warren wrote:
> > ...
> > >                 pmx_sdhci: pinconfig-sdhci {
> > >                         /*
> > >                          * The mux property is a list of muxable entities
> > >                          * and the mux function to select for it. The number
> > >                          * of cells in each entry is the pin controller's
> > >                          * #pinmux-cells property. The pin controller's
> > >                          * binding defines what the cells mean. The pinctrl
> > >                          * driver is responsible for mapping this data to
> > >                          * the (group, function) pair required to fill in
> > >                          * the pinctrl subsystem's pinmux mapping table.
> > >                          */
> > >                         mux =
> > >                                 <TEGRA_PMX_PG_DTA TEGRA_PMX_MUX_SDIO1>
> > >                                 <TEGRA_PMX_PG_DTD TEGRA_PMX_MUX_SDIO1>;
> > 
> > We need a property like 'mux-unit' whose value can be either 'pin' or
> > 'pingroup' to reflect something you mentioned as muxable entity.
> 
> I'm not sure I agree; see below.
> 
> > The reason behind this is the DT logic inside pinctrl core needs to
> > know how the pinmux_map should be constructed from device tree.
> 
> As a general statement, yes.
> 
> > In tegra case, the 'mux-unit' is 'pingroup', the core should construct
> > pinmux_map entry for each row/element of 'mux'.
> 
> Yes.
> 
> > In imx case, the 'mux-unit' will be 'pin',
> 
> OK.
> 
> Just a note: Tegra30 also has per-pin muxability. Only Tegra20 muxes pins
> in groups. (although Tegra30 does some if its pin configuration in groups)
> 
> > and we would expect core construct only
> > one pinmux_map entry there, with all the pins listed in 'mux' composing
> > the group that pinmux_map needs.
> 
> This is where I disagree.
> 
So what I read is you disagree how pinctrl core uses property 'mux-unit'
not the property itself.

> If the pinmux_map should only contain a single entry, wouldn't the DT
> mux property only contain a single entry?
> 
So you are saying we should have a pinmux_map for each entry in the
mux property?  I disagree with that.  For imx usdhc example, we have
10 entries in mux property representing 10 pins and their mux values.
What we need is one pinmux_map rather than 10 for just one client
device.

> The reason being that if there's a single entry in the pinmux_map, the
> group name used in that entry must be a group that's supported directly
> by the pinctrl driver (that's just the way pinctrl works). As such, why
> not just write the device tree in terms of those groups?
> 
> The only way I can see this not being true is if your pinctrl driver is
> also parsing these mux properties, and dynamically creating the groups
> that it exposes based on the list of pins in the mux property.

Yes, that's exactly what we are trying to do for imx.

> However,
> that seems like the wrong approach;

Where does it go wrong exactly?

> If you're dynamically defining groups
> in DT, I'd expect separate explicit driver-specific properties/nodes to
> define those groups, such that the pinctrl core's processing of the mux
> property to be identical in all cases.
> 
It's not imx specific.  It's generally useful for any soc that has pin
as the muxable entity.  I think it should be the common part of this
binding and implemented in pinctrl core DT support. 

-- 
Regards,
Shawn

RE: Pinmux bindings proposal V2

[Stephen Warren]

Here is a summary of what we discussed and agreed upon in the pinctrl
meetings at Linaro Connect:

* We will allow drivers that define the SoC's entire set of capabilities
in static tables in the pinctrl driver. However, drivers will not be
required to do this; it's a decision of the individual driver writer
whether to

** Store the data in static tables in the driver, or in device tree, or
elsewhere.

** Expose all of the SoC's capabilities to the pinctrl core, or only
those capabilities required for the current board (this is somewhat more
of an issue for drivers storing their data in device tree).

* We will merge the pinmux mapping table and pin config mapping table
into a single table, so that a single set of APIs will program both
pin mux and pin config into HW at the same time.

* The pinctrl client-side API will be reworked slightly, to support this;
it'll use "named states" rather like the recent pin config API propsals:

struct pinctrl *p;
struct pinctrl_state *active;
struct pinctrl_state *standby;

// Slowpath: locks all mappings and pins for this device irrespective
// of state name
p = pintrl_get(dev); 
active = pinctrl_get_state(p, "active"); // Slowpath
standby = pinctrl_get_state(p, "standby"); // Slowpath
pinctrl_select_state(p, active); // Fastpath
pinctrl_select_state(p, standby); // Fastpath
pinctrl_put(p); // Slowpath

Note: I added the "p" parameter to pinctrl_select_state() as proposed
by Simon Glass. I think we did briefly agree to this. It'll give us
completely flexibility for the opaque values "p", "active", and "standby"
if we need to change them in the future.

Note: There was some discussion re: pinctrl_get_state() being imbalanced
since there is no pinctrl_put_state(); pinctrl_put() does that. We didn't
entirely resolve that, I think. I propose renaming pinctrl_get_state() to
pinctrl_lookup_state() to avoid that imbalance. Does anyone disagree with
that? (I'm hoping not to start a bikeshed discussion here:-)

* We will remove the "hog_on_boot" field from the mapping table. Instead,
such entries will be represented as part of the pin controller driver's
own pin states. Linusw has already sent the patches to do this.

* We will add an (optional) "op" (function) to the pinctrl driver struct,
which informs the driver when all mux/config values have been set for the
selected named state. This can be used to either flush a register cache
to HW, perform error-checking on the complete HW configuration, etc.

* We will add an "of_node" field to the mapping table. When dynamically
constructing the mapping table from device tree, we can use this field
to match entries against drivers. This removes the requirement to know
a device's dev_name when creating the mapping table, which may happen
before drivers are probed.

swarren notes now: This point is no longer relevant given the next:

* When creating the mapping table from device tree, we will defer
parsing of the common pinctrl properties of each client devices's node
until that device's driver calls pinctrl_get(). This removes the need
for the pinctrl core to scan the entire device tree early during boot
looking for all nodes that use pinctrl properties, in order to construct
the mapping table; instead it may be constructed lazily, and feed into
deferred probe nicely.

* The /common/ pinctrl bindings will be:

Each device will have a set of numbered states that can be activated.
These will be numbered contiguously starting with 0, so 0, 1, 2, ...

Optionally, these states may have names too.

Each client driver's binding or driver defines the number of states,
what each state ID means, whether states have names, and what those
names are. The pinctrl common bindings don't impose any rules here.

For each state, there will be an associated list of phandles that describe
the HW programming for that state. A list is needed primarily to allow a
device to rely on configuring multiple pin controllers at once, but may be
used for other purposes such as combining different common configuration
subsets together.

Each of the nodes referenced by the listed phandles must be stored
underneath the pin controller that it affects. The content of those nodes
is not described in any way at all by the common pinctrl bindings; it is
/entirely/ up to the bindings of the individual pin controller to define
the content of that node. Each pinctrl driver will provide an "op"
(function) to convert these nodes into a list of mapping table entries
for use during DT parsing.

Given:

        pinmux@70000000 {
                compatible = "nvidia,tegra20-pinmux";
                reg = <...>;
                // SoC-specific properties here
                pmx_sdhci: pmx_sdhci {
                        // SoC-specific properties here
                };
        };

The simplest possible common bindings example is:

        sdhci@c8000200 {
                pinctrl-0 = <&pmx_sdhci>;
        };

A slightly more complex example:

        sdhci@c8000200 {
                pinctrl-0 = <&pmx_sdhci &pmx_sdhci_active>;
                pinctrl-1 = <&pmx_sdhci &pmx_sdhci_standby>;
                /* An optional list of state names; depends on SDHCI driver */
                pinctrl-names = "active", "suspend";
        };

* Since I proposed most of this and completely understand what I mean, I
will write patches to document and implement all of the above, ASAP.

-- 
nvpublic