Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 06/18/2013 03:29 AM, Christian Ruppert wrote:
> The pinmux driver of the Abilis Systems TB10x platform based on ARC700 CPUs.
> Used to control the pinmux and is a prerequisite for the GPIO driver.
> diff --git a/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt
> b/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt
> +Port definitions
> +
> +
> +Ports are defined (and referenced) by sub-nodes of the pin controller. Every
> +sub-node defines exactly one port (i.e. a set of pins). Ports are predefined
> +as named pin groups inside the pin controller driver and these names are used
> +to associate pin group predefinitions to pin controller sub-nodes.
> +
> +Required port definition subnode properties:
> + - pingrp: should be set to the name of the port's pin group.
This seems odd More on that where I comment on the example.
> +The following pin groups are available:
> + - GPIO ports: gpioa_pins, gpiob_pins, gpioc_pins, gpiod_pins, gpioe_pins,
> +gpiof_pins, gpiog_pins, gpioh_pins, gpioi_pins, gpioj_pins,
> +gpiok_pins, gpiol_pins, gpiom_pins, gpion_pins
...
> + - JTAG: jtag_pins
I'd suggest removing "_pins" from all those names, since it's the same
in all names and hence isn't necessary.
> +GPIO ranges definition
> +--
> +
> +The named pin groups of GPIO ports can be used to define GPIO ranges as
> +explained in Documentation/devicetree/bindings/gpio/gpio.txt.
I wouldn't mention that here; the GPIO node contains the gpio-ranges
property, not the pin controller node. Hence, the binding for the GPIO
DT node should describe the property, not the binding for this node.
> +Example
> +---
> +
> +iomux: iomux@FF10601c {
> + compatible = "abilis,tb10x-iomux";
> + reg = <0xFF10601c 0x4>;
> + pctl_gpio_a: pctl-gpio-a {
> + pingrp = "gpioa_pins";
> + };
> + pctl_uart0: pctl-uart0 {
> + pingrp = "uart0_pins";
> + };
> +};
The two nodes pctl-gpio-a and pctl-uart0 seem to be missing data. The
idea here is that you define nodes that says:
* This node applies to these pin(s)/group(s).
* Select mux function X on those pins/groups and/or apply these pin
configuration options to those pins/groups.
The examples above don't include any mux/config options, nor does the
binding say how to do specify them.
The set of pin groups defined by this binding should correspond directly
to the set of pin groups that actually exist in HW. So, if you have 3
pin groups (A, B, C) in HW each of which has two mux functions (X, Y),
your DT binding should define just 3 pin groups (A, B, C), not 6 (A_X,
A_Y, B_X, B_Y, C_X, C_Y). In other words, the pin group name shouldn't
imply the mux function.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 06/26/2013 05:50 AM, Christian Ruppert wrote:
> On Wed, Jun 19, 2013 at 04:35:14PM -0600, Stephen Warren wrote:
>> On 06/18/2013 03:29 AM, Christian Ruppert wrote:
>>> The pinmux driver of the Abilis Systems TB10x platform based on ARC700 CPUs.
>>> Used to control the pinmux and is a prerequisite for the GPIO driver.
>>
>>> diff --git
>>> a/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt
>>> b/Documentation/devicetree/bindings/pinctrl/abilis,tb10x-iomux.txt
>>
>>> +Port definitions
>>> +
>>> +
>>> +Ports are defined (and referenced) by sub-nodes of the pin controller.
>>> Every
>>> +sub-node defines exactly one port (i.e. a set of pins). Ports are
>>> predefined
>>> +as named pin groups inside the pin controller driver and these names are
>>> used
>>> +to associate pin group predefinitions to pin controller sub-nodes.
>>> +
>>> +Required port definition subnode properties:
>>> + - pingrp: should be set to the name of the port's pin group.
>>
>> This seems odd More on that where I comment on the example.
>>
>>> +The following pin groups are available:
>>> + - GPIO ports: gpioa_pins, gpiob_pins, gpioc_pins, gpiod_pins, gpioe_pins,
>>> +gpiof_pins, gpiog_pins, gpioh_pins, gpioi_pins, gpioj_pins,
>>> +gpiok_pins, gpiol_pins, gpiom_pins, gpion_pins
>> ...
>>> + - JTAG: jtag_pins
>>
>> I'd suggest removing "_pins" from all those names, since it's the same
>> in all names and hence isn't necessary.
>>
>>> +GPIO ranges definition
>>> +--
>>> +
>>> +The named pin groups of GPIO ports can be used to define GPIO ranges as
>>> +explained in Documentation/devicetree/bindings/gpio/gpio.txt.
>>
>> I wouldn't mention that here; the GPIO node contains the gpio-ranges
>> property, not the pin controller node. Hence, the binding for the GPIO
>> DT node should describe the property, not the binding for this node.
>>
>>> +Example
>>> +---
>>> +
>>> +iomux: iomux@FF10601c {
>>> + compatible = "abilis,tb10x-iomux";
>>> + reg = <0xFF10601c 0x4>;
>>> + pctl_gpio_a: pctl-gpio-a {
>>> + pingrp = "gpioa_pins";
>>> + };
>>> + pctl_uart0: pctl-uart0 {
>>> + pingrp = "uart0_pins";
>>> + };
>>> +};
>>
>> The two nodes pctl-gpio-a and pctl-uart0 seem to be missing data. The
>> idea here is that you define nodes that says:
>>
>> * This node applies to these pin(s)/group(s).
>> * Select mux function X on those pins/groups and/or apply these pin
>> configuration options to those pins/groups.
>>
>> The examples above don't include any mux/config options, nor does the
>> binding say how to do specify them.
>>
>> The set of pin groups defined by this binding should correspond directly
>> to the set of pin groups that actually exist in HW. So, if you have 3
>> pin groups (A, B, C) in HW each of which has two mux functions (X, Y),
>> your DT binding should define just 3 pin groups (A, B, C), not 6 (A_X,
>> A_Y, B_X, B_Y, C_X, C_Y). In other words, the pin group name shouldn't
>> imply the mux function.
>
> Can we consider it as agreed now that this implementation is acceptable
> for the TB10x pin controller?
There are two issues here:
1) What is a pin group:
1a) Must it solely represent a group of pins that actually exists in HW
(e.g. it's an RTL port, or a set of pins all controlled at once by a
single bit/field in a register)
1b) A SW-defined group of pins, simply because it's convenient to talk
about that set of pins at once, even though HW doesn't impose that those
pins are in a group in any way.
Defining groups for either of those reasons is fine, although this is
the area where my preference and LinusW's differ.
2) Can groups represent just a set of pins, or can it also imply that a
particular mux function is selected on that group?
I believe that both LinusW and I are in agreement that a group is simply
a list/set/group of pins. You select mux functions onto groups. A
groups's definition can't imply that a particular mux function is
selected onto it.
If we don't follow this rule, then you end up with a combinatorial
explosion of groups; the cross-product of all possible groups of pins
vs. the mux function to select on them, rather than simply having a list
of groups of pins, which is a much smaller set/list.
So, in the DT example above, I still believe that you need an extra
property that defines which mux function to select onto the specified
group. The group name can't imply this, so there needs to be some way of
specifying it.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 07/05/2013 03:49 AM, Christian Ruppert wrote:
> On Wed, Jun 26, 2013 at 11:40:42AM -0600, Stephen Warren wrote:
>> On 06/26/2013 05:50 AM, Christian Ruppert wrote:
>>> On Wed, Jun 19, 2013 at 04:35:14PM -0600, Stephen Warren wrote:
On 06/18/2013 03:29 AM, Christian Ruppert wrote:
>> [...]
> +Example
> +---
> +
> +iomux: iomux@FF10601c {
> + compatible = "abilis,tb10x-iomux";
> + reg = <0xFF10601c 0x4>;
> + pctl_gpio_a: pctl-gpio-a {
> + pingrp = "gpioa_pins";
> + };
> + pctl_uart0: pctl-uart0 {
> + pingrp = "uart0_pins";
> + };
> +};
The two nodes pctl-gpio-a and pctl-uart0 seem to be missing data. The
idea here is that you define nodes that says:
* This node applies to these pin(s)/group(s).
* Select mux function X on those pins/groups and/or apply these pin
configuration options to those pins/groups.
The examples above don't include any mux/config options, nor does the
binding say how to do specify them.
The set of pin groups defined by this binding should correspond directly
to the set of pin groups that actually exist in HW. So, if you have 3
pin groups (A, B, C) in HW each of which has two mux functions (X, Y),
your DT binding should define just 3 pin groups (A, B, C), not 6 (A_X,
A_Y, B_X, B_Y, C_X, C_Y). In other words, the pin group name shouldn't
imply the mux function.
>>>
>>> Can we consider it as agreed now that this implementation is acceptable
>>> for the TB10x pin controller?
>>
>> There are two issues here:
>>
>> 1) What is a pin group:
>>
>> 1a) Must it solely represent a group of pins that actually exists in HW
>> (e.g. it's an RTL port, or a set of pins all controlled at once by a
>> single bit/field in a register)
>>
>> 1b) A SW-defined group of pins, simply because it's convenient to talk
>> about that set of pins at once, even though HW doesn't impose that those
>> pins are in a group in any way.
>>
>> Defining groups for either of those reasons is fine, although this is
>> the area where my preference and LinusW's differ.
>>
>> 2) Can groups represent just a set of pins, or can it also imply that a
>> particular mux function is selected on that group?
>>
>> I believe that both LinusW and I are in agreement that a group is simply
>> a list/set/group of pins. You select mux functions onto groups. A
>> groups's definition can't imply that a particular mux function is
>> selected onto it.
>>
>> If we don't follow this rule, then you end up with a combinatorial
>> explosion of groups; the cross-product of all possible groups of pins
>> vs. the mux function to select on them, rather than simply having a list
>> of groups of pins, which is a much smaller set/list.
>>
>> So, in the DT example above, I still believe that you need an extra
>> property that defines which mux function to select onto the specified
>> group. The group name can't imply this, so there needs to be some way of
>> specifying it.
>
> In your opinion, would something in the lines of
>
> pctl_spi1: pctl-spi1 {
> abilis,pingrp = "spi1";
So that defines a list of pins.
> abilis,ioport = <4>; /* spi1 is routed to port4 inside the
> pin controller */
I assume that defines the mux function value; the value that's
programmed into the HW register to select which HW module's signals are
routed out to the pins specified by abilis,pingrp.
> abilis,ioconf = <1>; /* spi1 is available in configuration 1
> of that port. */
But I don't understand what that is. ...
...
> In future, this could even be extended to allow several alternative
> configurations for a given function, e.g.
>
> pctl_spi3: pctl-spi3 {
> abilis,pingrp = "spi3";
> abilis,ioport = <6>;
> abilis,ioconf = <0 3>; /* spi3 is available in both
> configurations 0 and 3. Depending on
> what other functions are requested, the
> pinctrl driver can choose either of the
> two. */
... especially if you're talking about "spi3 being available in multiple
configurations". What's a configuration?
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 07/08/2013 07:02 AM, Christian Ruppert wrote:
...
> OK, a small drawing of our hardware should make this clear, let's take
> an imaginary example of one port with 10 pins, one i2c interface, one
> spi interface and one GPIO bank:
>
> | mux N-1|
> ++
> || 2
> |+--/-- i2c
> ||
>10 || 4
>Pins --/--+ mux N +--/-- spi
> ||
> || 10
> |+--/-- GPIO
> ||
> ++
> | mux N+1|
>
> This example shows the mux N inside the pin controller. It controls
> all pins associated to port N through a single register value M. Let's
> assume the pins are configured as follows in function of the register
> value:
>
> pin M=0 M=1 M=2 M=3
> 0 GPIO0 SPI_MISO GPIO0 SPI_MISO
> 1 GPIO1 SPI_MOSI GPIO1 SPI_MOSI
> 2 GPIO2SPI_CK GPIO2SPI_CK
> 3 GPIO3SPI_CS GPIO3SPI_CS
> 4 GPIO4GPIO4GPIO4GPIO4
> 5 GPIO5GPIO5GPIO5GPIO5
> 6 GPIO6GPIO6GPIO6GPIO6
> 7 GPIO7GPIO7GPIO7GPIO7
> 8 GPIO8GPIO8 I2C_SDA I2C_SDA
> 9 GPIO9GPIO9 I2C_SCL I2C_SCL
In that scenario, in the language of Linux's pinctrl subsystem, what you
have is:
10 pins, named 0..9
1 pin group, named perhaps "mux N".
4 different functions; values M==0, 1, 2, 3.
> We now have three pin groups defined, corresponding to the chip-side
> ports of the pin controller:
> GPIO = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}
> SPI = {0, 1, 2, 3}
> I2C = {8, 9}
You would usually only define pin groups for the pin/ball/package side
of the pinmux HW. IIRC, you're also wanting to define pin groups for the
intra-chip side of the pinmux HW. However, you're not muxing functions
onto those pingroups; they're just there to help with naming the
GPIO<->pinmux mapping. You only mux functions onto the pin/ball/package
side pins/pingroups.
> abilis,pingrp now specifies one of the three pin groups. Note that I2C
>and SPI can be requested independently in a completely orthogonal
>manner: The information if I2C is reqired or not is confined to
>the I2C request and does not leak into the SPI request as would
>be the case if we configured the entire port at the same time.
The pingrp should represent the pin/ball/package side pins/groups. In
this case, it should specify "N".
> abilis,ioport specifies N.
That is replaced be pingrp.
> abilis,ioconf specifies M.
That'd be better named "function" or something like that, in order to
indicate that it specifies which function is mux'd onto the specified
pin(s)/pingroup(s).
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 07/16/2013 02:47 AM, Christian Ruppert wrote:
> On Wed, Jul 10, 2013 at 01:27:52PM -0600, Stephen Warren wrote:
>> On 07/08/2013 07:02 AM, Christian Ruppert wrote:
>> ...
>>> OK, a small drawing of our hardware should make this clear, let's take
>>> an imaginary example of one port with 10 pins, one i2c interface, one
>>> spi interface and one GPIO bank:
>>>
>>> | mux N-1|
>>> ++
>>> || 2
>>> |+--/-- i2c
>>> ||
>>>10 || 4
>>>Pins --/--+ mux N +--/-- spi
>>> ||
>>> || 10
>>> |+--/-- GPIO
>>> ||
>>> ++
>>> | mux N+1|
>>>
>>> This example shows the mux N inside the pin controller. It controls
>>> all pins associated to port N through a single register value M. Let's
>>> assume the pins are configured as follows in function of the register
>>> value:
>>>
>>> pin M=0 M=1 M=2 M=3
>>> 0 GPIO0 SPI_MISO GPIO0 SPI_MISO
>>> 1 GPIO1 SPI_MOSI GPIO1 SPI_MOSI
>>> 2 GPIO2SPI_CK GPIO2SPI_CK
>>> 3 GPIO3SPI_CS GPIO3SPI_CS
>>> 4 GPIO4GPIO4GPIO4GPIO4
>>> 5 GPIO5GPIO5GPIO5GPIO5
>>> 6 GPIO6GPIO6GPIO6GPIO6
>>> 7 GPIO7GPIO7GPIO7GPIO7
>>> 8 GPIO8GPIO8 I2C_SDA I2C_SDA
>>> 9 GPIO9GPIO9 I2C_SCL I2C_SCL
>>
>>
>> In that scenario, in the language of Linux's pinctrl subsystem, what you
>> have is:
>>
>> 10 pins, named 0..9
>> 1 pin group, named perhaps "mux N".
>> 4 different functions; values M==0, 1, 2, 3.
>>
>>> We now have three pin groups defined, corresponding to the chip-side
>>> ports of the pin controller:
>>> GPIO = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9}
>>> SPI = {0, 1, 2, 3}
>>> I2C = {8, 9}
>>
>> You would usually only define pin groups for the pin/ball/package side
>> of the pinmux HW. IIRC, you're also wanting to define pin groups for the
>> intra-chip side of the pinmux HW. However, you're not muxing functions
>> onto those pingroups; they're just there to help with naming the
>> GPIO<->pinmux mapping. You only mux functions onto the pin/ball/package
>> side pins/pingroups.
>
> Well, the GPIO<->pinmux mapping is not the only reason for defining
> these groups wrt. the chip-side of the pin controller. The other reasons
> are:
> - Make different interfaces on the same MUX orthogonal wrt. each
> other, i.e. make it possible to request one independently of the
> other. In the example above, SPI and I2C can be requested completely
> independently and the pin controller driver decides which mode to
> use.
But the pinctrl subsystem and bindings don't have any concept of that;
what gets requested is the pins on the chip, or the "outer" side of the
pin controller. There's no concept of resource management for the
"inside" of the pin controller.
> - Make pin allocation more fine-grained (in the example above, only
> pins 0-4 are "allocated" in case SPI is requested). This makes
> GPIO<->interface pin conflict management more natural.
I think you'd want to either:
a) Just deal with this in the driver; it knows the HW, and it knows
which mux function is selected for each mux, and hence knows exactly
which pins can be requested as GPIOs for each combination, and can
therefore allow/disallow any GPIO request or mux function change.
b) Extend the pinctrl core to know about this explicitly, and pass
information to the pinctrl core. Presumably, for each combination of
(pingroup, mux function), you'd need a list or bitmask indicating which
pins within the pingroup are actually used. Then, the pinctrl core can
perform all the validation. If you do this, you don't need to invent new
pinctrl groups in order to try and shoe-horn this into pinctrl.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH 2/4] pinmux: Add TB10x pinmux driver
On 07/18/2013 10:07 AM, Christian Ruppert wrote:
...
> Well, perhaps my definition of "inside"/"outside" pins was not quite
> clear: The pin groups define the set of (kernel internal) pin numbers of
> "outside" pins which are used by pin controller to map a given
> interface. Inside pins are not numbered and the inside interfaces are
> only used to determine which outside pins are part of the same group
> (namely those for which the pin controller hardware provides a mux
> connection to the same inside interface):
>
>4
> 4 /|--/-- SPI
>PINS[0..3] --/--|| 4
>\|--/-- GPIO[0..3]
>
>4
>PINS[4..7] -/-- GPIO[4..7]
>
>2
> 2 /|--/-- I2C
>PINS[8..9] --/--|| 2
>\|--/-- GPIO[8..9]
>
> Pins 0..3 are in the SPI group because on the "inside" they can be muxed
> to the SPI interface.
> Pins 8..9 are in the I2C group because on the "inside" they can be muxed
> to the I2C interface.
> Pins 0..9 are in the GPIO group because on the "inside" they can be
> muxed to the GPIO controller.
>
> All pin numbers are relative to the "outside", however, or conflict
> management would not be possible. I hope this is more understandable
> than my previous explanations.
> Both muxes are controlled by the same register. In our overly simplistic
> example this is not strictly necessary but in reality you might have pin
> conflicts between the different interfaces.
Same register, or same field/bits in that register?
If it's the same field/bits, I would expect to see the following pin groups:
1) PINS[0..3], PINS[8..9]
2) PINS[4..7]
... since those are the things that are independently muxable.
Otherwise, I'd expect to see the following groups:
1) PINS[0..3]
2) PINS[4..7]
3) PINS[8..9]
> After the discussion we had so far I'm not so sure if extending the
> pinctrl system with this kind of features is a very good idea.
That makes things simple:-)
One thing I still don't understand; in a previous mail, you'd mentioned
3 DT properties for configuring the pinmux; one represented the pin
group, one represented the mux function that was selected for that pin
group, and there was a third ("config"?) property. I still don't
understand that third property. I only see pins/pingroups and mux
functions in the diagram I quoted above.
___
devicetree-discuss mailing list
devicetree-discuss@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/devicetree-discuss
