pxa27x and pinctrl-single

Hi,

I'd like to start converting to device tree usage some of the old pxa27x platforms I'm fond of, starting with adding pinctrl support. If I'm not mistaken, this will clear the way for some ongoing updates to the pinctrl/gpio code used by newer Marvell arches.

I noticed that Haojian pulled the pinctrl-pxa driver from the kernel, in favor of using pinctrl-single, so I turned my attention to that. From what I can tell, pinctrl-single is currently inadequate for pxa27x because:

(1) On the pxa27x, setting the mux for a pin involves configuring both the alt function register *and* the direction register, requiring the ability to specify in the device tree multiple reg/value pairs for each pin.

(2) Some of the pinctrl-single code still assumes one register-per-pin; see for example the functions pcs_allocate_pin_table() and pcs_get_pin_by_offset(). Curiously, the recently added pcs_request_gpio() function also suffers from this, unless I'm mistaken.

Quoted 11 lines · Mike Dunn

Hi,

I'd like to start converting to device tree usage some of the old pxa27x platforms I'm fond of, starting with adding pinctrl support. If I'm not mistaken, this will clear the way for some ongoing updates to the pinctrl/gpio code used by newer Marvell arches.

I noticed that Haojian pulled the pinctrl-pxa driver from the kernel, in favor of using pinctrl-single, so I turned my attention to that. From what I can tell, pinctrl-single is currently inadequate for pxa27x because:

Quoted 3 lines

(1) On the pxa27x, setting the mux for a pin involves configuring both the alt function register *and* the direction register, requiring the ability to specify in the device tree multiple reg/value pairs for each pin.

In PXA27x, GPIO controller control both GPIO & pin alternate function. The alternate function is covered by GAFRx registers.

At first, we need to move GAFRx from gpio-pxa driver. Then we can support

it by pinctrl-single driver since pinctrl-single driver could support mulitple pins in one pin registers. There's "bits-per-mux" property in pinctrl-single driver.

Quoted 6 lines

(2) Some of the pinctrl-single code still assumes one register-per-pin; see for example the functions pcs_allocate_pin_table() and pcs_get_pin_by_offset(). Curiously, the recently added pcs_request_gpio() function also suffers from this, unless I'm mistaken.

Not exactly. pinctrl-single driver could support multiple pins in one register. Now Manjunathappa also fixed some issue in pinctrl-single for supporting multiple pins. (https://lkml.org/lkml/2013/5/21/226) I suggest you to develop based on his patches.

Thanks for the reply Haojian.

Quoted 12 lines · Haojian Zhuang

Quoted 11 lines · Mike Dunn

Hi,

I'd like to start converting to device tree usage some of the old pxa27x platforms I'm fond of, starting with adding pinctrl support. If I'm not mistaken, this will clear the way for some ongoing updates to the pinctrl/gpio code used by newer Marvell arches.

I noticed that Haojian pulled the pinctrl-pxa driver from the kernel, in favor of using pinctrl-single, so I turned my attention to that. From what I can tell, pinctrl-single is currently inadequate for pxa27x because:

Quoted 7 lines

Quoted 3 lines

(1) On the pxa27x, setting the mux for a pin involves configuring both the alt function register *and* the direction register, requiring the ability to specify in the device tree multiple reg/value pairs for each pin.

In PXA27x, GPIO controller control both GPIO & pin alternate function. The alternate function is covered by GAFRx registers.

At first, we need to move GAFRx from gpio-pxa driver. Then we can support

Thanks... I wondered about division of labor between pinctrl and gpio. Pinctrl did seem to be the appropriate place.

Quoted 3 lines

it by pinctrl-single driver since pinctrl-single driver could support mulitple pins in one pin registers. There's "bits-per-mux" property in pinctrl-single driver.

Yes, but currently pinctrl-single only supports writing one register for a given pin (with multiple pins sharing a register if bit-per-mux==true). On pxa27x, a pin's alt function is determined by the values written to both the GAFRx and the GPDRx registers, so I think that pinctrl-single may need to allow a device tree to specify multiple reg/value/mask sets for any one pin. I don't have a pxa3xx/mmp developer's manual handy, but from the code it appears that on these arches the direction register is irrelevant to the mux setting.

Quoted 1 line · Mike Dunn

Quoted 9 lines

Yes, but currently pinctrl-single only supports writing one register for a given pin (with multiple pins sharing a register if bit-per-mux==true). On pxa27x, a pin's alt function is determined by the values written to both the GAFRx and the GPDRx registers, so I think that pinctrl-single may need to allow a device tree to specify multiple reg/value/mask sets for any one pin. I don't have a pxa3xx/mmp developer's manual handy, but from the code it appears that on these arches the direction register is irrelevant to the mux setting.

No, it's not need to access both GAFRx & GPDRx. Let's focus on PXA27x first.

GPIO ?x? Alternate Function Select Bits (where x = 112 through 120) A bit-pair in this register determines the corresponding GPIO pin?s functionality as one of the alternate functions that is mapped to it or as a generic GPIO pin.

0b00 = The corresponding GPIO pin (GPIO<x>) is used as a general- purpose I/O.
0b01 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 1.
0b10 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 2.
0b11 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 3.

We can see that GAFRx configures the pinmux. That's all. We can use pinctrl-single
driver to cover this.

I know that you're also talking the implementation in gpio-pxa driver. __gpio_is_occupied() checks both GAFRx & GPDRx. Because this GPIO pin should be configured as GPIO input. Actually we could also discard the checking.

So there's no relationship between GAFRx and GPDRx.

Quoted 2 lines · Haojian Zhuang

Quoted 1 line · Mike Dunn

[...]

Quoted 26 lines

Quoted 9 lines

Yes, but currently pinctrl-single only supports writing one register for a given pin (with multiple pins sharing a register if bit-per-mux==true). On pxa27x, a pin's alt function is determined by the values written to both the GAFRx and the GPDRx registers, so I think that pinctrl-single may need to allow a device tree to specify multiple reg/value/mask sets for any one pin. I don't have a pxa3xx/mmp developer's manual handy, but from the code it appears that on these arches the direction register is irrelevant to the mux setting.

No, it's not need to access both GAFRx & GPDRx. Let's focus on PXA27x first.

GPIO ?x? Alternate Function Select Bits (where x = 112 through 120) A bit-pair in this register determines the corresponding GPIO pin?s functionality as one of the alternate functions that is mapped to it or as a generic GPIO pin.

0b00 = The corresponding GPIO pin (GPIO<x>) is used as a general- purpose I/O.
0b01 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 1.
0b10 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 2.
0b11 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 3.

Quoted 3 lines

We can see that GAFRx configures the pinmux. That's all. We can use pinctrl-single
driver to cover this.

But on the pxa27x there are usually *two* meanings for any one of the 3 alt functions, depending on the direction. As a random example... gpio28 alt fn 0b01 is AC97_BITCLK if the direction is input, but is I2S_BITCLK if the direction is output (from PXA27x Developer's Manual, Table 24-2).

Quoted 7 lines

I know that you're also talking the implementation in gpio-pxa driver. __gpio_is_occupied() checks both GAFRx & GPDRx. Because this GPIO pin should be configured as GPIO input. Actually we could also discard the checking.

Quoted 1 line

So there's no relationship between GAFRx and GPDRx.

But I am thinking on pxa27x there is... see my example above. Perhaps this is not true for the later Marvell arches and you are missing this point? Or else I am stating the obvious to you and I am missing your point. In either case, thanks for your patience.

Quoted 39 lines · Mike Dunn

Quoted 2 lines · Haojian Zhuang

Quoted 1 line · Mike Dunn

[...]

Quoted 26 lines

Quoted 9 lines

Yes, but currently pinctrl-single only supports writing one register for a given pin (with multiple pins sharing a register if bit-per-mux==true). On pxa27x, a pin's alt function is determined by the values written to both the GAFRx and the GPDRx registers, so I think that pinctrl-single may need to allow a device tree to specify multiple reg/value/mask sets for any one pin. I don't have a pxa3xx/mmp developer's manual handy, but from the code it appears that on these arches the direction register is irrelevant to the mux setting.

No, it's not need to access both GAFRx & GPDRx. Let's focus on PXA27x first.

GPIO ?x? Alternate Function Select Bits (where x = 112 through 120) A bit-pair in this register determines the corresponding GPIO pin?s functionality as one of the alternate functions that is mapped to it or as a generic GPIO pin.

0b00 = The corresponding GPIO pin (GPIO<x>) is used as a general- purpose I/O.
0b01 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 1.
0b10 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 2.
0b11 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 3.

Quoted 3 lines

We can see that GAFRx configures the pinmux. That's all. We can use pinctrl-single
driver to cover this.

Quoted 5 lines

But on the pxa27x there are usually *two* meanings for any one of the 3 alt functions, depending on the direction. As a random example... gpio28 alt fn 0b01 is AC97_BITCLK if the direction is input, but is I2S_BITCLK if the direction is output (from PXA27x Developer's Manual, Table 24-2).

You didn't understand the alternate function. GPIO mode is only one mode of alternate functions. Because GPIO means that user can program the pin, you also need to configure the direction for GPIO usage.

Most of pins are either input or output. If the alternate function is AC97_BITCLK,
input mode is configured by hardware automatically. You shouldn't configure it in GPDR register. Since it's working in AC97 mode, not GPIO mode. Configuring in GPDR register can't work.

Quoted 50 lines · Haojian Zhuang

Quoted 39 lines · Mike Dunn

Quoted 2 lines · Haojian Zhuang

Quoted 1 line · Mike Dunn

[...]

Quoted 26 lines

Quoted 9 lines

Yes, but currently pinctrl-single only supports writing one register for a given pin (with multiple pins sharing a register if bit-per-mux==true). On pxa27x, a pin's alt function is determined by the values written to both the GAFRx and the GPDRx registers, so I think that pinctrl-single may need to allow a device tree to specify multiple reg/value/mask sets for any one pin. I don't have a pxa3xx/mmp developer's manual handy, but from the code it appears that on these arches the direction register is irrelevant to the mux setting.

No, it's not need to access both GAFRx & GPDRx. Let's focus on PXA27x first.

GPIO ?x? Alternate Function Select Bits (where x = 112 through 120) A bit-pair in this register determines the corresponding GPIO pin?s functionality as one of the alternate functions that is mapped to it or as a generic GPIO pin.

0b00 = The corresponding GPIO pin (GPIO<x>) is used as a general- purpose I/O.
0b01 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 1.
0b10 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 2.
0b11 = The corresponding GPIO pin (GPIO<x>) is used for its alternate function 3.

Quoted 3 lines

We can see that GAFRx configures the pinmux. That's all. We can use pinctrl-single
driver to cover this.

Quoted 5 lines

But on the pxa27x there are usually *two* meanings for any one of the 3 alt functions, depending on the direction. As a random example... gpio28 alt fn 0b01 is AC97_BITCLK if the direction is input, but is I2S_BITCLK if the direction is output (from PXA27x Developer's Manual, Table 24-2).

You didn't understand the alternate function. GPIO mode is only one mode of alternate functions. Because GPIO means that user can program the pin, you also need to configure the direction for GPIO usage.

Quoted 5 lines

Most of pins are either input or output. If the alternate function is AC97_BITCLK,
input mode is configured by hardware automatically. You shouldn't configure it in GPDR register. Since it's working in AC97 mode, not GPIO mode. Configuring in GPDR register can't work.

This is where you are wrong Haojian. Now I am sure that this peculiarity is unique to pxa2xx. From pxa27x developer's manual (section 24.4.2)...

GPIO pins can have as many as three alternate input and three alternate output functions.

And in the comments above the __gpio_is_occupied() function in gpio-pxa.c you mentioned earlier...

/*
* On PXA25x and PXA27x, GAFRx and GPDRx together decide the alternate
* function of a GPIO, and GPDRx cannot be altered once configured. It

I'll try to state it another way... on the old Intel pxa arches, the direction bit is effectively a third alt function bit, as "alt function" is defined by the newer Marvell processors.

I'll be happy to send you the pxa27x developer's manual if you still have doubts. In the gpio chapter you will see a table with a row for each gpio pin, each row containing 6 columns labeled

alt fn 1 (in)
alt fn 2 (in)
alt fn 3 (in)
alt fn 1 (out)
alt fn 2 (out)
alt fn 3 (out)

I hope that makes sense now.

Thanks again,
Mike

Since you need to configure both GPDRx and GAFRx. If we are talking pinctrl-single
as reference, we can make it work by this way.

We need to define two pinmux controllers. One is for GPDRx, and the other is for GAFRx.
Both of them need to support pinctrl-single,bits property. For any alternate pins in DTS,
we could include these two pins from two pinmux controllers. What's your opinion?

Quoted 1 line · Haojian Zhuang

[...]

Quoted 10 lines

Since you need to configure both GPDRx and GAFRx. If we are talking pinctrl-single
as reference, we can make it work by this way.

We need to define two pinmux controllers. One is for GPDRx, and the other is for GAFRx.
Both of them need to support pinctrl-single,bits property. For any alternate pins in DTS,
we could include these two pins from two pinmux controllers. What's your opinion?

Are you are suggesting that the dts file can be defined such that the desired values are written to GAFR and GPDR, without having to make any changes to the pinctrl-single driver code? If I understand correctly, we would be defining two "pins" in the device tree for each actual pin. That seems very ugly.

I was thinking that pinctrl-single could be modified to support multiple reg/value/mask pairs for each pin listed in the pinctrl-single,bits property. There is a comment at the top of pcs_parse_one_pinctrl_entry() that seems to suggest the possibility...

* Note that this binding currently supports only sets of one register + value.

I am still learning pinctrl and devicetree and studying the driver, so I am not adament about anything at this point. Guidance appreciated!

Thanks,
Mike

Quoted 23 lines · Mike Dunn

Quoted 1 line · Haojian Zhuang

[...]

Quoted 10 lines

Since you need to configure both GPDRx and GAFRx. If we are talking pinctrl-single
as reference, we can make it work by this way.

We need to define two pinmux controllers. One is for GPDRx, and the other is for GAFRx.
Both of them need to support pinctrl-single,bits property. For any alternate pins in DTS,
we could include these two pins from two pinmux controllers. What's your opinion?

Are you are suggesting that the dts file can be defined such that the desired values are written to GAFR and GPDR, without having to make any changes to the pinctrl-single driver code? If I understand correctly, we would be defining two "pins" in the device tree for each actual pin. That seems very ugly.

Quoted 4 lines

I was thinking that pinctrl-single could be modified to support multiple reg/value/mask pairs for each pin listed in the pinctrl-single,bits property. There is a comment at the top of pcs_parse_one_pinctrl_entry() that seems to suggest the possibility...

We only support continuous register offset in pinctrl-single driver. GPDRx is in range of 0x40e0000c~0x40e0010c, GAFRx is in range of 0x40e00054~0x40e00070.

So I suggest you to split them as two pinmux controller. If you define them into one pinmux controler, it's also OK. But they are still two pins in the on pinmux controller. And you should avoid to access those spare pins in the middle.

I don't suggest you to support multiple reg/value/mask pairs for each pin in pinctrl-single driver. It's too complex. It already exceeds the design scope of the pinctrl-single driver.