pinctrl: Introduce support for iodelay module in TI SoCs

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

Quoted 24 lines · Linus Walleij

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

Quoted 8 lines

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Hey let's get few things straight here. There's nothing stopping the driver from knowing a detailed register layout with the pinctrl-single binding. This can be very easily done based on the compatible flag and match data as needed and check the values provided.

And let's also recap the reasons for the pinctrl-single binding. The the main reason for the pinctrl-single binding is to avoid mapping individual register bits to device tree compatible string properties.

Imagine doing that for hundreds of similar registers. Believe me, I tried using device tree properties initially and it just sucked big time. For larger amounts of dts data, it's best to stick to numeric values and phandles in the device tree data and rely on the preprocessor for getting the values right.

Finally, we don't want to build databases into the kernel drivers for every SoC. We certainly have plenty fo those already.

Quoted 5 lines

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

Quoted 2 lines

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Nishanth, care to explain your reasons for using pinctrl-single binding here vs the generic binding? Is the amount of string parsing with the data an issue here?

Quoted 4 lines

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

We certainly need to make setting delays (with values) generic, no doubt about that.

If the large amount of data is not an issue here, we could maybe set each iodelay register as a separate device? Then the binding could be just along the interrupts-extended type binding:

foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>;

Where the 0x18c is the instance offset of the register within the ti,dra7-iodelay compatible controller.

Quoted 57 lines · Tony Lindgren

Quoted 24 lines · Linus Walleij

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

Quoted 8 lines

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Hey let's get few things straight here. There's nothing stopping the driver from knowing a detailed register layout with the pinctrl-single binding. This can be very easily done based on the compatible flag and match data as needed and check the values provided.

And let's also recap the reasons for the pinctrl-single binding. The the main reason for the pinctrl-single binding is to avoid mapping individual register bits to device tree compatible string properties.

Imagine doing that for hundreds of similar registers. Believe me, I tried using device tree properties initially and it just sucked big time. For larger amounts of dts data, it's best to stick to numeric values and phandles in the device tree data and rely on the preprocessor for getting the values right.

Finally, we don't want to build databases into the kernel drivers for every SoC. We certainly have plenty fo those already.

Quoted 5 lines

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

Quoted 6 lines

Quoted 2 lines

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Nishanth, care to explain your reasons for using pinctrl-single binding here vs the generic binding? Is the amount of string parsing with the data an issue here?

Wrong option chosen, I suppose :( - alright, lets discuss the alternative.

Quoted 7 lines

Quoted 3 lines

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

We certainly need to make setting delays (with values) generic, no doubt about that.

Quoted 8 lines

If the large amount of data is not an issue here, we could maybe set each iodelay register as a separate device? Then the binding could be just along the interrupts-extended type binding:

foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>;

Where the 0x18c is the instance offset of the register within the ti,dra7-iodelay compatible controller.

if mmc2_pins_default point at pins for mmc pin configuration for control_core (pinctrl-single), are you proposing the following?

mmc2_pins_default: mmc2_pins_default {
pinctrl-single,pins = <
0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a23.mmc2_clk */

                 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_cs1.mmc2_cmd */
                 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a24.mmc2_dat0 */
                 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a25.mmc2_dat1 */
                 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a26.mmc2_dat2 */
                 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a27.mmc2_dat3 */
                 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a19.mmc2_dat4 */
                 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a20.mmc2_dat5 */
                 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a21.mmc2_dat6 */
                 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a22.mmc2_dat7 */
         >;

};

&mmc2 {
...
pinctrl-1 =

(quoted diff) +0 -0

 	&mmc2_pins_default,	/* points to mmc control core pins */
 	<&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A19_IN */
 	<&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>,     /* CFG_GPMC_A20_IN */
 	<&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A21_IN */
 	<&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A22_IN */
 	<&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>,     /* CFG_GPMC_A23_IN */
 	<&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>,     /* CFG_GPMC_A24_IN */
 	<&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>,         /* CFG_GPMC_A25_IN */
 	<&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>,       /* CFG_GPMC_A26_IN */
 	<&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>,     /* CFG_GPMC_A27_IN */
 	<&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>;         /* CFG_GPMC_CS1_IN */

I have to check if we are capable of parsing that. but if that is the approach chosen, I suppose we might be able to figure something, I suppose..

Quoted 58 lines · Nishanth Menon

Quoted 57 lines · Tony Lindgren

Quoted 24 lines · Linus Walleij

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

Quoted 8 lines

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Hey let's get few things straight here. There's nothing stopping the driver from knowing a detailed register layout with the pinctrl-single binding. This can be very easily done based on the compatible flag and match data as needed and check the values provided.

And let's also recap the reasons for the pinctrl-single binding. The the main reason for the pinctrl-single binding is to avoid mapping individual register bits to device tree compatible string properties.

Imagine doing that for hundreds of similar registers. Believe me, I tried using device tree properties initially and it just sucked big time. For larger amounts of dts data, it's best to stick to numeric values and phandles in the device tree data and rely on the preprocessor for getting the values right.

Finally, we don't want to build databases into the kernel drivers for every SoC. We certainly have plenty fo those already.

Quoted 5 lines

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Nishanth, care to explain your reasons for using pinctrl-single binding here vs the generic binding? Is the amount of string parsing with the data an issue here?

Wrong option chosen, I suppose :( - alright, lets discuss the alternative.

Heh well now we know :)

Quoted 42 lines

Quoted 7 lines

Quoted 3 lines

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

We certainly need to make setting delays (with values) generic, no doubt about that.

Quoted 8 lines

If the large amount of data is not an issue here, we could maybe set each iodelay register as a separate device? Then the binding could be just along the interrupts-extended type binding:

foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>;

Where the 0x18c is the instance offset of the register within the ti,dra7-iodelay compatible controller.

if mmc2_pins_default point at pins for mmc pin configuration for control_core (pinctrl-single), are you proposing the following?

mmc2_pins_default: mmc2_pins_default {
pinctrl-single,pins = <
0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a23.mmc2_clk */

                 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_cs1.mmc2_cmd */
                 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a24.mmc2_dat0 */
                 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a25.mmc2_dat1 */
                 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a26.mmc2_dat2 */
                 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a27.mmc2_dat3 */
                 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a19.mmc2_dat4 */
                 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a20.mmc2_dat5 */
                 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a21.mmc2_dat6 */
                 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a22.mmc2_dat7 */
         >;

Quoted 1 line

};

Yeah so existing pinctrl-single binding above, with additional iodelay binding below..

Quoted 15 lines

&mmc2 {
...
pinctrl-1 =

(quoted diff) +0 -0

 	&mmc2_pins_default,	/* points to mmc control core pins */
 	<&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A19_IN */
 	<&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>,     /* CFG_GPMC_A20_IN */
 	<&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A21_IN */
 	<&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A22_IN */
 	<&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>,     /* CFG_GPMC_A23_IN */
 	<&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>,     /* CFG_GPMC_A24_IN */
 	<&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>,         /* CFG_GPMC_A25_IN */
 	<&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>,       /* CFG_GPMC_A26_IN */
 	<&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>,     /* CFG_GPMC_A27_IN */
 	<&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>;         /* CFG_GPMC_CS1_IN */

Quoted 3 lines

I have to check if we are capable of parsing that. but if that is the approach chosen, I suppose we might be able to figure something, I suppose..

Yes except I'd make use of some kind of #pinctrl-cells here just like interrupt controller has #interrupt-cells. Then you can have the values seprate and the controller knows what to do with them based on the compatible flag and #pinctrl-cells.

Quoted 132 lines · Tony Lindgren

Quoted 58 lines · Nishanth Menon

Quoted 57 lines · Tony Lindgren

Quoted 24 lines · Linus Walleij

Quoted 9 lines · Nishanth Menon

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +9 -0

+Configuration definition follows similar model as the pinctrl-single:
+The groups of pin configuration are defined under "pinctrl-single,pins"
+
+&dra7_iodelay_core {
+       mmc2_iodelay_3v3_conf: mmc2_iodelay_3v3_conf {
+               pinctrl-single,pins = <
+                       0x18c (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A19_IN */
+                       0x1a4 (A_DELAY(265) | G_DELAY(360))     /* CFG_GPMC_A20_IN */
+                       0x1b0 (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A21_IN */

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt line 42

Quoted 10 lines

Documentation/devicetree/bindings/pinctrl/ti,iodelay-pinctrl.txt new file +10 -0

+                       0x1bc (A_DELAY(0) | G_DELAY(120))       /* CFG_GPMC_A22_IN */
+                       0x1c8 (A_DELAY(287) | G_DELAY(420))     /* CFG_GPMC_A23_IN */
+                       0x1d4 (A_DELAY(144) | G_DELAY(240))     /* CFG_GPMC_A24_IN */
+                       0x1e0 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_A25_IN */
+                       0x1ec (A_DELAY(120) | G_DELAY(0))       /* CFG_GPMC_A26_IN */
+                       0x1f8 (A_DELAY(120) | G_DELAY(180))     /* CFG_GPMC_A27_IN */
+                       0x360 (A_DELAY(0) | G_DELAY(0))         /* CFG_GPMC_CS1_IN */
+               >;
+       };
+};

But wait.

Quoted 8 lines

The promise when we merged pinctrl-single was that this driver was to be used when the system had a one-register-per-pin layout and it was easy to do device trees based on that.

We were very reluctant to accept that even though we didn't even have the generic pin control bindings in place, the argument being that the driver should know the detailed register layout, it should not be described in the device tree. We eventually caved in and accepted it as an exception.

Hey let's get few things straight here. There's nothing stopping the driver from knowing a detailed register layout with the pinctrl-single binding. This can be very easily done based on the compatible flag and match data as needed and check the values provided.

And let's also recap the reasons for the pinctrl-single binding. The the main reason for the pinctrl-single binding is to avoid mapping individual register bits to device tree compatible string properties.

Imagine doing that for hundreds of similar registers. Believe me, I tried using device tree properties initially and it just sucked big time. For larger amounts of dts data, it's best to stick to numeric values and phandles in the device tree data and rely on the preprocessor for getting the values right.

Finally, we don't want to build databases into the kernel drivers for every SoC. We certainly have plenty fo those already.

Quoted 5 lines

Since this pin controller is not using pinctrl-single it does not enjoy that privilege and you need to explain why this pin controller cannot use the generic bindings like so many other pin controllers have since started to do. ("It is in the same SoC" is not an acceptable argument.)

Quoted 8 lines

Quoted 6 lines

Quoted 2 lines

What is wrong with this:
Documentation/devicetree/bindings/pinctrl/pinctrl-bindings.txt

Nishanth, care to explain your reasons for using pinctrl-single binding here vs the generic binding? Is the amount of string parsing with the data an issue here?

Wrong option chosen, I suppose :( - alright, lets discuss the alternative.

Heh well now we know :)

Quoted 42 lines

Quoted 7 lines

Quoted 3 lines

We can add generic delay bindings to the list, it even seems like a good idea to do so, as it is likely something that will come up in other hardware and will be needed for ACPI etc going forward.

We certainly need to make setting delays (with values) generic, no doubt about that.

Quoted 8 lines

If the large amount of data is not an issue here, we could maybe set each iodelay register as a separate device? Then the binding could be just along the interrupts-extended type binding:

foo = <&bar 0x18c A_DELAY(0) G_DELAY(120)>;

Where the 0x18c is the instance offset of the register within the ti,dra7-iodelay compatible controller.

if mmc2_pins_default point at pins for mmc pin configuration for control_core (pinctrl-single), are you proposing the following?

mmc2_pins_default: mmc2_pins_default {
pinctrl-single,pins = <
0x9c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a23.mmc2_clk */

                 0xb0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_cs1.mmc2_cmd */
                 0xa0 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a24.mmc2_dat0 */
                 0xa4 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a25.mmc2_dat1 */
                 0xa8 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a26.mmc2_dat2 */
                 0xac (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a27.mmc2_dat3 */
                 0x8c (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a19.mmc2_dat4 */
                 0x90 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a20.mmc2_dat5 */
                 0x94 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a21.mmc2_dat6 */
                 0x98 (PIN_INPUT_PULLUP | MANUAL_MODE | MUX_MODE1) /*
gpmc_a22.mmc2_dat7 */
         >;

Quoted 1 line

};

Yeah so existing pinctrl-single binding above, with additional iodelay binding below..

Quoted 15 lines

&mmc2 {
...
pinctrl-1 =

(quoted diff) +0 -0

 	&mmc2_pins_default,	/* points to mmc control core pins */
 	<&iodelay 0x18c A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A19_IN */
 	<&iodelay 0x1a4 A_DELAY(265) | G_DELAY(360)>,     /* CFG_GPMC_A20_IN */
 	<&iodelay 0x1b0 A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A21_IN */
 	<&iodelay 0x1bc A_DELAY(0) | G_DELAY(120)>,       /* CFG_GPMC_A22_IN */
 	<&iodelay 0x1c8 A_DELAY(287) | G_DELAY(420)>,     /* CFG_GPMC_A23_IN */
 	<&iodelay 0x1d4 A_DELAY(144) | G_DELAY(240)>,     /* CFG_GPMC_A24_IN */
 	<&iodelay 0x1e0 A_DELAY(0) | G_DELAY(0)>,         /* CFG_GPMC_A25_IN */
 	<&iodelay 0x1ec A_DELAY(120) | G_DELAY(0)>,       /* CFG_GPMC_A26_IN */
 	<&iodelay 0x1f8 A_DELAY(120) | G_DELAY(180)>,     /* CFG_GPMC_A27_IN */
 	<&iodelay 0x360 A_DELAY(0) | G_DELAY(0)>;         /* CFG_GPMC_CS1_IN */

Quoted 8 lines

Quoted 3 lines

I have to check if we are capable of parsing that. but if that is the approach chosen, I suppose we might be able to figure something, I suppose..

Yes except I'd make use of some kind of #pinctrl-cells here just like interrupt controller has #interrupt-cells. Then you can have the values seprate and the controller knows what to do with them based on the compatible flag and #pinctrl-cells.

Something like the following I suppose, where pinctrl-cells is optional?

dra7_pmx_core: pinmux at 4a003400 {
        compatible = "ti,dra7-padconf", "pinctrl-single";
        reg = <0x4a003400 0x0464>;
        #address-cells = <1>;
        #size-cells = <0>;
        #interrupt-cells = <1>;
        interrupt-controller;
        pinctrl-single,register-width = <32>;
        pinctrl-single,function-mask = <0x3fffffff>;
};

dra7_iodelay_core: padconf at 4844a000 {
        compatible = "ti,dra7-iodelay";
        reg = <0x4844a000 0x0d1c>;
        #address-cells = <1>;
        #size-cells = <0>;
	#pinctrl-cells = <2>;
};

Linus,

I hope you are ok with the above?

Quoted 4 lines · Tony Lindgren

Yes except I'd make use of some kind of #pinctrl-cells here just like interrupt controller has #interrupt-cells. Then you can have the values seprate and the controller knows what to do with them based on the compatible flag and #pinctrl-cells.

Something like the following I suppose, where pinctrl-cells is optional?

dra7_pmx_core: pinmux at 4a003400 {
        compatible = "ti,dra7-padconf", "pinctrl-single";
        reg = <0x4a003400 0x0464>;
        #address-cells = <1>;
        #size-cells = <0>;
        #interrupt-cells = <1>;
        interrupt-controller;
        pinctrl-single,register-width = <32>;
        pinctrl-single,function-mask = <0x3fffffff>;
};

dra7_iodelay_core: padconf at 4844a000 {
        compatible = "ti,dra7-iodelay";
        reg = <0x4844a000 0x0d1c>;
        #address-cells = <1>;
        #size-cells = <0>;
        #pinctrl-cells = <2>;
};

Linus,

I hope you are ok with the above?

Quoted 28 lines · Nishanth Menon

Quoted 4 lines · Tony Lindgren

Yes except I'd make use of some kind of #pinctrl-cells here just like interrupt controller has #interrupt-cells. Then you can have the values seprate and the controller knows what to do with them based on the compatible flag and #pinctrl-cells.

Something like the following I suppose, where pinctrl-cells is optional?

dra7_pmx_core: pinmux at 4a003400 {
        compatible = "ti,dra7-padconf", "pinctrl-single";
        reg = <0x4a003400 0x0464>;
        #address-cells = <1>;
        #size-cells = <0>;
        #interrupt-cells = <1>;
        interrupt-controller;
        pinctrl-single,register-width = <32>;
        pinctrl-single,function-mask = <0x3fffffff>;
};

dra7_iodelay_core: padconf at 4844a000 {
        compatible = "ti,dra7-iodelay";
        reg = <0x4844a000 0x0d1c>;
        #address-cells = <1>;
        #size-cells = <0>;
        #pinctrl-cells = <2>;
};

Quoted 3 lines

Linus,

I hope you are ok with the above?

Hm depends on where the documentation hits I guess?

Such a generic cell count property has to be to the generic pinctrl-bindings.txt document if I read it right.

Overall I guess this will be acceptable but you really need to reuse some more code between this driver and pinctrl-single.c if I read it right.

Yeah agreed. I suggest discussing the binding and the generic parsing code for it first :)

It seems with the generic binding the actual driver should be just the hardware specific code hopefully.

Quoted 5 lines · Tony Lindgren

Yeah agreed. I suggest discussing the binding and the generic parsing code for it first :)

It seems with the generic binding the actual driver should be just the hardware specific code hopefully.

Did this thread go anywhere in the last month? I am certainly looking forward to seeing what the resolution is to this, given for our use the boot loader setup is not appealing at all.

I am yet to post a new revision to this series - few other stuff got in the way. IODelay driver in no way removes the constraint that the SoC architecture has - most of the pins still need to be muxed in bootloader - we cannot escape that. The reasoning for doing the mux in bootloader is independent of the need for iodelay.

Reasoning for mux in bootloader is because the mux and pull fields are glitchy - much more than previous generations of TI SoCs and significantly long enough to cause issues depending on the pins being muxed.

Reasoning for iodelay is different - it is a hardware block meant to control the timing of signals in a particular signal path to ensure that specification compliance is met.

Lets try not to mix the two.

Quoted 10 lines · Nishanth Menon

I am yet to post a new revision to this series - few other stuff got in the way. IODelay driver in no way removes the constraint that the SoC architecture has - most of the pins still need to be muxed in bootloader - we cannot escape that. The reasoning for doing the mux in bootloader is independent of the need for iodelay.

Reasoning for mux in bootloader is because the mux and pull fields are glitchy - much more than previous generations of TI SoCs and significantly long enough to cause issues depending on the pins being muxed.

Well if we know glitching is NOT an issue on our boards, then we don't have to do anything in the boot loader other than the basic setup for the serial console and emmc and SD, which has always been necesary.

I consider moving the mux setup to the bootloader a terrible design and won't go along with it. We make sure all external devices have reset lines being held while the pinmux is being setup, so glitching is a non issue.

Quoted 5 lines

Reasoning for iodelay is different - it is a hardware block meant to control the timing of signals in a particular signal path to ensure that specification compliance is met.

Lets try not to mix the two.

Well I was told by multiple people from TI that the reason for moving the pinmux setup to the bootloader was because of the iodelay issue, so you will have to get the message made clear within TI then.

drivers/pinctrl/Kconfig +9 -0

+	depends on OF
+	select PINCONF
+	select GENERIC_PINCONF
+	select REGMAP_MMIO
+	help
+	  Say Y here to support Texas Instruments' IODelay pinconf driver.
+	  IODelay module is used for the DRA7 SoC family. This driver is in
+	  addition to PINCTRL_SINGLE which controls the mux.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090

drivers/pinctrl/Makefile +0 -0

index 0475206dd600..e441cd71aedf 100644

drivers/pinctrl/Makefile +0 -0

 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o

drivers/pinctrl/pinctrl-ti-iodelay.c new file +0 -0

new file mode 100644
index 000000000000..e4c6e25a781c

drivers/pinctrl/pinctrl-ti-iodelay.c new file +7 -0

+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>

Nishanth Menon schreef op di 03-03-2015 om 18:00 [-0600]:

drivers/pinctrl/Kconfig config PINCTRL_TEGRA_XUSB · line 164

Quoted 10 lines

drivers/pinctrl/Kconfig +2 -0

index ee9f44ad7f02..8e463d75fbb2 100644
@@ -160,6 +160,17 @@ config PINCTRL_TEGRA_XUSB
 	select PINCONF
 	select PINMUX
 
+config PINCTRL_TI_IODELAY
+	bool "TI IODelay Module pinconf driver"

This adds a boolean Kconfig symbol.

Quoted 17 lines

drivers/pinctrl/Kconfig +9 -0

+	depends on OF
+	select PINCONF
+	select GENERIC_PINCONF
+	select REGMAP_MMIO
+	help
+	  Say Y here to support Texas Instruments' IODelay pinconf driver.
+	  IODelay module is used for the DRA7 SoC family. This driver is in
+	  addition to PINCTRL_SINGLE which controls the mux.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090

drivers/pinctrl/Makefile +0 -0

index 0475206dd600..e441cd71aedf 100644

drivers/pinctrl/Makefile obj-$(CONFIG_PINCTRL_TEGRA30) += pinctrl-tegra30.o · line 31

Quoted 4 lines

drivers/pinctrl/Makefile +1 -0

@@ -28,6 +28,7 @@ obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_TEGRA114)	+= pinctrl-tegra114.o
 obj-$(CONFIG_PINCTRL_TEGRA124)	+= pinctrl-tegra124.o
 obj-$(CONFIG_PINCTRL_TEGRA_XUSB)	+= pinctrl-tegra-xusb.o
+obj-$(CONFIG_PINCTRL_TI_IODELAY)	+= pinctrl-ti-iodelay.o

So this objectfile will either be built-in or not be built at all.

Quoted 9 lines

drivers/pinctrl/Makefile +0 -0

 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o

drivers/pinctrl/pinctrl-ti-iodelay.c new file +0 -0

new file mode 100644
index 000000000000..e4c6e25a781c

drivers/pinctrl/pinctrl-ti-iodelay.c line 15

Quoted 6 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +5 -0

@@ -0,0 +1,963 @@
[...]
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>

This is, I think, a bit of a red flag.

Quoted 7 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +7 -0

+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>

[...]

drivers/pinctrl/pinctrl-ti-iodelay.c line 954

Quoted 11 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +11 -0

+static const struct of_device_id ti_iodelay_of_match[] = {
+	{.compatible = "ti,dra7-iodelay", .data = &dra7_iodelay_data},
+	{ /* Hopefully no more.. */ },
+};
+MODULE_DEVICE_TABLE(of, ti_iodelay_of_match);
+
+static struct platform_driver ti_iodelay_driver = {
+	.probe = ti_iodelay_probe,
+	.remove = ti_iodelay_remove,
+	.driver = {
+		   .owner = THIS_MODULE,

So THIS_MODULE will, basically, be NULL.

drivers/pinctrl/pinctrl-ti-iodelay.c line 963

Quoted 9 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +9 -0

+		   .name = DRIVER_NAME,
+		   .of_match_table = ti_iodelay_of_match,
+		   },
+};
+module_platform_driver(ti_iodelay_driver);
+
+MODULE_AUTHOR("Texas Instruments, Inc.");
+MODULE_DESCRIPTION("Pinconf driver for TI's IO Delay module");
+MODULE_LICENSE("GPL v2");

And these three macros will be preprocessed away.

(Perhaps there are non-modular alternatives to MODULE_DEVICE_TABLE and module_platform_driver, but I'm not sure how those two macros actually work.)

Quoted 9 lines · Paul Bolle

Nishanth Menon schreef op di 03-03-2015 om 18:00 [-0600]:

Quoted 8 lines

drivers/pinctrl/Kconfig +0 -0

index ee9f44ad7f02..8e463d75fbb2 100644
@@ -160,6 +160,17 @@ config PINCTRL_TEGRA_XUSB
 	select PINCONF
 	select PINMUX
 

Quoted 4 lines

drivers/pinctrl/Kconfig config PINCTRL_TEGRA_XUSB · line 164

Quoted 2 lines

drivers/pinctrl/Kconfig +2 -0

+config PINCTRL_TI_IODELAY
+	bool "TI IODelay Module pinconf driver"

This adds a boolean Kconfig symbol.

Quoted 79 lines

Quoted 18 lines

drivers/pinctrl/Kconfig +9 -0

+	depends on OF
+	select PINCONF
+	select GENERIC_PINCONF
+	select REGMAP_MMIO
+	help
+	  Say Y here to support Texas Instruments' IODelay pinconf driver.
+	  IODelay module is used for the DRA7 SoC family. This driver is in
+	  addition to PINCTRL_SINGLE which controls the mux.
+
 config PINCTRL_TZ1090
 	bool "Toumaz Xenif TZ1090 pin control driver"
 	depends on SOC_TZ1090

drivers/pinctrl/Makefile +0 -0

index 0475206dd600..e441cd71aedf 100644

drivers/pinctrl/Makefile obj-$(CONFIG_PINCTRL_TEGRA30) += pinctrl-tegra30.o · line 31

Quoted 4 lines

drivers/pinctrl/Makefile +1 -0

@@ -28,6 +28,7 @@ obj-$(CONFIG_PINCTRL_TEGRA30)	+= pinctrl-tegra30.o
 obj-$(CONFIG_PINCTRL_TEGRA114)	+= pinctrl-tegra114.o
 obj-$(CONFIG_PINCTRL_TEGRA124)	+= pinctrl-tegra124.o
 obj-$(CONFIG_PINCTRL_TEGRA_XUSB)	+= pinctrl-tegra-xusb.o
+obj-$(CONFIG_PINCTRL_TI_IODELAY)	+= pinctrl-ti-iodelay.o

So this objectfile will either be built-in or not be built at all.

Quoted 9 lines

drivers/pinctrl/Makefile +0 -0

 obj-$(CONFIG_PINCTRL_TZ1090)	+= pinctrl-tz1090.o
 obj-$(CONFIG_PINCTRL_TZ1090_PDC)	+= pinctrl-tz1090-pdc.o
 obj-$(CONFIG_PINCTRL_U300)	+= pinctrl-u300.o

drivers/pinctrl/pinctrl-ti-iodelay.c new file +0 -0

new file mode 100644
index 000000000000..e4c6e25a781c

drivers/pinctrl/pinctrl-ti-iodelay.c line 15

Quoted 6 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +5 -0

@@ -0,0 +1,963 @@
[...]
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/list.h>
+#include <linux/module.h>

This is, I think, a bit of a red flag.

Quoted 7 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +7 -0

+#include <linux/of_device.h>
+#include <linux/of.h>
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>

[...]

drivers/pinctrl/pinctrl-ti-iodelay.c line 954

Quoted 11 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +11 -0

+static const struct of_device_id ti_iodelay_of_match[] = {
+	{.compatible = "ti,dra7-iodelay", .data = &dra7_iodelay_data},
+	{ /* Hopefully no more.. */ },
+};
+MODULE_DEVICE_TABLE(of, ti_iodelay_of_match);
+
+static struct platform_driver ti_iodelay_driver = {
+	.probe = ti_iodelay_probe,
+	.remove = ti_iodelay_remove,
+	.driver = {
+		   .owner = THIS_MODULE,

So THIS_MODULE will, basically, be NULL.

drivers/pinctrl/pinctrl-ti-iodelay.c line 963

Quoted 9 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +9 -0

+		   .name = DRIVER_NAME,
+		   .of_match_table = ti_iodelay_of_match,
+		   },
+};
+module_platform_driver(ti_iodelay_driver);
+
+MODULE_AUTHOR("Texas Instruments, Inc.");
+MODULE_DESCRIPTION("Pinconf driver for TI's IO Delay module");
+MODULE_LICENSE("GPL v2");

And these three macros will be preprocessed away.

Quoted 3 lines

(Perhaps there are non-modular alternatives to MODULE_DEVICE_TABLE and module_platform_driver, but I'm not sure how those two macros actually work.)

This should be just made to work as a regular loadable module. We already have pinctrl-single for the same platforms working as a loadable module.

SoC family such as DRA7 family of processors have, in addition to the regular muxing of pins (as done by pinctrl-single), an additional hardware module called IODelay which is also expected to be configured. This "IODelay" module has it's own register space that is independent of the control module.

It is advocated strongly in TI's official documentation considering the existing design of the DRA7 family of processors during mux or IODelay reconfiguration, there is a potential for a significant glitch which may cause functional impairment to certain hardware. It is hence recommended to do as little of muxing as absolutely necessary without I/O isolation (which can only be done in initial stages of bootloader).

NOTE: with the system wide I/O isolation scheme present in DRA7 SoC family, it is not reasonable to do stop all I/O operations for every such pad configuration scheme. So, we will let it glitch when used in this mode.

Even with the above limitation, certain functionality such as MMC has mandatory need for IODelay reconfiguration requirements, depending on speed of transfer. In these cases, with careful examination of usecase involved, the expected glitch can be controlled such that it does not impact functionality.

In short, IODelay module support as a padconf driver being introduced here is not expected to do SoC wide I/O Isolation and is meant for a limited subset of IODelay configuration requirements that need to be dynamic and whose glitchy behavior will not cause functionality failure for that interface.

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Lokesh Vutla <[email protected]>

drivers/pinctrl/pinctrl-ti-iodelay.c new file +6 -0

+       u16 fine_delay_count;
+
+       u16 ref_clk_period;
+
+       u32 cdpe;
+       u32 fdpe;

drivers/pinctrl/pinctrl-ti-iodelay.c new file +14 -0

+/**
+ * struct ti_iodelay_device - Represents information for a IOdelay instance
+ * @dev: device pointer
+ * @reg_base: Remapped virtual address
+ * @regmap: Regmap for this IOdelay instance
+ * @pctl: Pinctrl device
+ * @desc: pinctrl descriptor for pctl
+ * @pa: pinctrl pin wise description
+ * @names: names of the pins
+ * @groups: list of pinconf groups for iodelay instance
+ * @ngroups: number of groups in the list
+ * @mutex: mutex to protect group list modification
+ * @reg_data: Register definition data for the IODelay instance
+ * @reg_init_conf_values: Initial configuration values.

drivers/pinctrl/pinctrl-ti-iodelay.c new file +65 -0

+/**
+ * ti_iodelay_pinconf_set() - Configure the pin configuration
+ * @iod:       IODelay device
+ * @val:       Configuration value
+ *
+ * Update the configuration register as per TRM and lockup once done.
+ * *IMPORTANT NOTE* SoC TRM does recommend doing iodelay programmation only
+ * while in Isolation. But, then, isolation also implies that every pin
+ * on the SoC(including DDR) will be isolated out. The only benefit being
+ * a glitchless configuration, However, the intent of this driver is purely
+ * to support a "glitchy" configuration where applicable.
+ *
+ * Return: 0 in case of success, else appropriate error value
+ */
+static int ti_iodelay_pinconf_set(struct ti_iodelay_device *iod,
+                                 struct ti_iodelay_conf_vals *val)
+{
+       const struct ti_iodelay_reg_data *reg = iod->reg_data;
+       struct ti_iodelay_reg_values *ival = &iod->reg_init_conf_values;
+       struct device *dev = iod->dev;
+       u32 g_delay_coarse, g_delay_fine;
+       u32 a_delay_coarse, a_delay_fine;
+       u32 c_elements, f_elements;
+       u32 total_delay;
+       u32 reg_mask, reg_val, tmp_val;
+       int r;
+
+       /* NOTE: Truncation is expected in all division below */
+       g_delay_coarse = val->g_delay / 920;
+       g_delay_fine = ((val->g_delay % 920) * 10) / 60;
+
+       a_delay_coarse = val->a_delay / ival->cdpe;
+       a_delay_fine = ((val->a_delay % ival->cdpe) * 10) / ival->fdpe;
+
+       c_elements = g_delay_coarse + a_delay_coarse;
+       f_elements = (g_delay_fine + a_delay_fine) / 10;
+
+       if (f_elements > 22) {
+               total_delay = c_elements * ival->cdpe + f_elements * ival->fdpe;
+               c_elements = total_delay / ival->cdpe;
+               f_elements = (total_delay % ival->cdpe) / ival->fdpe;
+       }
+
+       reg_mask = reg->signature_mask;
+       reg_val = reg->signature_value << __ffs(reg->signature_mask);
+
+       reg_mask |= reg->binary_data_coarse_mask;
+       tmp_val = c_elements << __ffs(reg->binary_data_coarse_mask);
+       if (tmp_val & ~reg->binary_data_coarse_mask) {
+               dev_err(dev, "Masking overflow of coarse elements %08x\n",
+                       tmp_val);
+               tmp_val &= reg->binary_data_coarse_mask;
+       }
+       reg_val |= tmp_val;
+
+       reg_mask |= reg->binary_data_fine_mask;
+       tmp_val = f_elements << __ffs(reg->binary_data_fine_mask);
+       if (tmp_val & ~reg->binary_data_fine_mask) {
+               dev_err(dev, "Masking overflow of fine elements %08x\n",
+                       tmp_val);
+               tmp_val &= reg->binary_data_fine_mask;
+       }
+       reg_val |= tmp_val;
+
+       /* Write with lock value - we DONOT want h/w updates */

drivers/pinctrl/pinctrl-ti-iodelay.c new file +5 -0

+       r = regmap_read(iod->regmap, reg->reg_coarse_offset, &val);
+       if (r)
+               return r;
+       ival->coarse_ref_count =
+           ti_iodelay_extract(val, reg->coarse_ref_count_mask);

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+/**
+ * ti_iodelay_pinctrl_get_group_pins() - get group pins
+ * @pctldev:   pinctrl device representing IODelay device
+ * @gselector: Group selector
+ * @pins:      pointer to the pins
+ * @npins:     number of pins
+ *
+ * Dummy implementation since we do not track pins, we track configurations
+ * Forced by pinctrl's pinctrl_check_ops()
+ *

+ group = ti_iodelay_get_group(iod, gselector);

Quoted 25 lines · Nishanth Menon

SoC family such as DRA7 family of processors have, in addition to the regular muxing of pins (as done by pinctrl-single), an additional hardware module called IODelay which is also expected to be configured. This "IODelay" module has it's own register space that is independent of the control module.

It is advocated strongly in TI's official documentation considering the existing design of the DRA7 family of processors during mux or IODelay reconfiguration, there is a potential for a significant glitch which may cause functional impairment to certain hardware. It is hence recommended to do as little of muxing as absolutely necessary without I/O isolation (which can only be done in initial stages of bootloader).

NOTE: with the system wide I/O isolation scheme present in DRA7 SoC family, it is not reasonable to do stop all I/O operations for every such pad configuration scheme. So, we will let it glitch when used in this mode.

Even with the above limitation, certain functionality such as MMC has mandatory need for IODelay reconfiguration requirements, depending on speed of transfer. In these cases, with careful examination of usecase involved, the expected glitch can be controlled such that it does not impact functionality.

Quoted 8 lines

In short, IODelay module support as a padconf driver being introduced here is not expected to do SoC wide I/O Isolation and is meant for a limited subset of IODelay configuration requirements that need to be dynamic and whose glitchy behavior will not cause functionality failure for that interface.

Signed-off-by Nishanth Menon <[email protected]>

Signed-off-by Lokesh Vutla <[email protected]>

Pending the conclusion of my remark on the bindings that this should use the generic pin config bindings... here are some "normal" review comments.

drivers/pinctrl/Kconfig config PINCTRL_TEGRA_XUSB · line 172

Quoted 10 lines

drivers/pinctrl/Kconfig +10 -0

+config PINCTRL_TI_IODELAY
+       bool "TI IODelay Module pinconf driver"
+       depends on OF
+       select PINCONF
+       select GENERIC_PINCONF
+       select REGMAP_MMIO
+       help
+         Say Y here to support Texas Instruments' IODelay pinconf driver.
+         IODelay module is used for the DRA7 SoC family. This driver is in
+         addition to PINCTRL_SINGLE which controls the mux.

If the driver is "in addition to PINCTRL_SINGLE", then it should depend on it.

drivers/pinctrl/pinctrl-ti-iodelay.c line 27

Quoted 2 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +2 -0

+/* Should I change this? Abuse? */
+#define IODELAY_MUX_PINS_NAME          "pinctrl-single,pins"

Yeah this is abuse and is why you should use the generic bindings with just "pins" instead, and use the generic pinconf parameters to set them up.

drivers/pinctrl/pinctrl-ti-iodelay.c line 42

Quoted 10 lines

+ * struct ti_iodelay_conf_vals - Description of each configuration parameters.

+ * @offset:    Configuration register offset
+ * @a_delay:   Agnostic Delay (in ps)
+ * @g_delay:   Gnostic Delay (in ps)

+ */

drivers/pinctrl/pinctrl-ti-iodelay.c new file +5 -0

+struct ti_iodelay_conf_vals {
+       u16 offset;
+       u16 a_delay;
+       u16 g_delay;
+};

So I don't think it's a good idea to get the register offset from the device tree. The driver should know these.

drivers/pinctrl/pinctrl-ti-iodelay.c line 109

Quoted 12 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +12 -0

+/**
+ * struct ti_iodelay_reg_values - Computed io_reg configuration values (see TRM)
+ * @coarse_ref_count:  Coarse reference count
+ * @coarse_delay_count:        Coarse delay count
+ * @fine_ref_count:    Fine reference count
+ * @fine_delay_count:  Fine Delay count
+ * @ref_clk_period:    Reference Clock period
+ * @cdpe:              Coarse delay parameter
+ * @fdpe:              Fine delay parameter
+ */
+struct ti_iodelay_reg_values {
+       u16 coarse_ref_count;

If you're doing reference counting, use kref.
<linux/kref.h>

drivers/pinctrl/pinctrl-ti-iodelay.c line 112

Quoted 3 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +3 -0

+       u16 coarse_delay_count;
+
+       u16 fine_ref_count;

Dito.

Quoted 6 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +6 -0

+       u16 fine_delay_count;
+
+       u16 ref_clk_period;
+
+       u32 cdpe;
+       u32 fdpe;

drivers/pinctrl/pinctrl-ti-iodelay.c line 127

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +8 -0

+};
 
 
+/**
+ * struct ti_iodelay_pin_name - name of the pins
+ * @name: name
+ */
+struct ti_iodelay_pin_name {
+       char name[IODELAY_REG_NAME_LEN];
+};

What is this? The pin control subsystem already has a struct for naming pins. struct pinctrl_pin_desc.

Are you reimplementing core functionality?

The generic pin config reference pins by name, and since you obviously like to encode these into you data you can just as well use that method for this too.

Again I think this is a side effect from using the pinctrl-single concepts, if you think of this as some other pinctrl driver I think this will fall out nicely.

Quoted 14 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +14 -0

+/**
+ * struct ti_iodelay_device - Represents information for a IOdelay instance
+ * @dev: device pointer
+ * @reg_base: Remapped virtual address
+ * @regmap: Regmap for this IOdelay instance
+ * @pctl: Pinctrl device
+ * @desc: pinctrl descriptor for pctl
+ * @pa: pinctrl pin wise description
+ * @names: names of the pins
+ * @groups: list of pinconf groups for iodelay instance
+ * @ngroups: number of groups in the list
+ * @mutex: mutex to protect group list modification
+ * @reg_data: Register definition data for the IODelay instance
+ * @reg_init_conf_values: Initial configuration values.

drivers/pinctrl/pinctrl-ti-iodelay.c line 172

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+ */
+struct ti_iodelay_device {
+       struct device *dev;
+       void __iomem *reg_base;
+       struct regmap *regmap;
+
+       struct pinctrl_dev *pctl;
+       struct pinctrl_desc desc;
+       struct pinctrl_pin_desc *pa;
+       struct ti_iodelay_pin_name *names;

Unhappy about that. pinctrl_desc contains
struct pinctrl_pin_desc const *pins; !

drivers/pinctrl/pinctrl-ti-iodelay.c line 176

Quoted 4 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +4 -0

+
+       struct list_head groups;
+       int ngroups;
+       struct mutex mutex; /* list protection */

Why is this needed? Usually the pin control core serialize calls into the drivers. Care to explain? Have the potential race really triggered?

drivers/pinctrl/pinctrl-ti-iodelay.c line 216

Quoted 12 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +12 -0

+static inline u32 ti_iodelay_compute_dpe(u16 period, u16 ref, u16 delay,
+                                        u16 delay_m)
+{
+       u64 m, d;
+
+       /* Handle overflow conditions */
+       m = 10 * (u64)period * (u64)ref;
+       d = 2 * (u64)delay * (u64)delay_m;
+
+       /* Truncate result back to 32 bits */
+       return div64_u64(m, d);
+}

Serious business ...

Quoted 65 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +65 -0

+/**
+ * ti_iodelay_pinconf_set() - Configure the pin configuration
+ * @iod:       IODelay device
+ * @val:       Configuration value
+ *
+ * Update the configuration register as per TRM and lockup once done.
+ * *IMPORTANT NOTE* SoC TRM does recommend doing iodelay programmation only
+ * while in Isolation. But, then, isolation also implies that every pin
+ * on the SoC(including DDR) will be isolated out. The only benefit being
+ * a glitchless configuration, However, the intent of this driver is purely
+ * to support a "glitchy" configuration where applicable.
+ *
+ * Return: 0 in case of success, else appropriate error value
+ */
+static int ti_iodelay_pinconf_set(struct ti_iodelay_device *iod,
+                                 struct ti_iodelay_conf_vals *val)
+{
+       const struct ti_iodelay_reg_data *reg = iod->reg_data;
+       struct ti_iodelay_reg_values *ival = &iod->reg_init_conf_values;
+       struct device *dev = iod->dev;
+       u32 g_delay_coarse, g_delay_fine;
+       u32 a_delay_coarse, a_delay_fine;
+       u32 c_elements, f_elements;
+       u32 total_delay;
+       u32 reg_mask, reg_val, tmp_val;
+       int r;
+
+       /* NOTE: Truncation is expected in all division below */
+       g_delay_coarse = val->g_delay / 920;
+       g_delay_fine = ((val->g_delay % 920) * 10) / 60;
+
+       a_delay_coarse = val->a_delay / ival->cdpe;
+       a_delay_fine = ((val->a_delay % ival->cdpe) * 10) / ival->fdpe;
+
+       c_elements = g_delay_coarse + a_delay_coarse;
+       f_elements = (g_delay_fine + a_delay_fine) / 10;
+
+       if (f_elements > 22) {
+               total_delay = c_elements * ival->cdpe + f_elements * ival->fdpe;
+               c_elements = total_delay / ival->cdpe;
+               f_elements = (total_delay % ival->cdpe) / ival->fdpe;
+       }
+
+       reg_mask = reg->signature_mask;
+       reg_val = reg->signature_value << __ffs(reg->signature_mask);
+
+       reg_mask |= reg->binary_data_coarse_mask;
+       tmp_val = c_elements << __ffs(reg->binary_data_coarse_mask);
+       if (tmp_val & ~reg->binary_data_coarse_mask) {
+               dev_err(dev, "Masking overflow of coarse elements %08x\n",
+                       tmp_val);
+               tmp_val &= reg->binary_data_coarse_mask;
+       }
+       reg_val |= tmp_val;
+
+       reg_mask |= reg->binary_data_fine_mask;
+       tmp_val = f_elements << __ffs(reg->binary_data_fine_mask);
+       if (tmp_val & ~reg->binary_data_fine_mask) {
+               dev_err(dev, "Masking overflow of fine elements %08x\n",
+                       tmp_val);
+               tmp_val &= reg->binary_data_fine_mask;
+       }
+       reg_val |= tmp_val;
+
+       /* Write with lock value - we DONOT want h/w updates */

drivers/pinctrl/pinctrl-ti-iodelay.c line 292

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+       reg_mask |= reg->lock_mask;
+       reg_val |= reg->lock_val << __ffs(reg->lock_mask);
+       r = regmap_update_bits(iod->regmap, val->offset, reg_mask, reg_val);
+
+       dev_dbg(dev, "Set reg 0x%x Delay(a=%d g=%d), Elements(C=%d F=%d)0x%x\n",
+               val->offset, val->a_delay, val->g_delay, c_elements,
+               f_elements, reg_val);
+
+       return r;
+}

Looking at this terse thing I think it's obviously easier to use a more verbose device tree description with

drive-delay = <some SI unit>;

drivers/pinctrl/pinctrl-ti-iodelay.c line 321

Quoted 12 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +12 -0

+       /* unlock the IOdelay region */
+       r = regmap_update_bits(iod->regmap, reg->reg_global_lock_offset,
+                              reg->global_lock_mask, reg->global_unlock_val);
+       if (r)
+               return r;
+
+       /* Read up Recalibration sequence done by bootloader */
+       r = regmap_read(iod->regmap, reg->reg_refclk_offset, &val);
+       if (r)
+               return r;
+       ival->ref_clk_period = ti_iodelay_extract(val, reg->refclk_period_mask);
+       dev_dbg(dev, "refclk_period=0x%04x\n", ival->ref_clk_period);

That sounds hightec. So the bootloader calibrates the delays and computes the refclock period? So is that sime PLL or something?

Quoted 5 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +5 -0

+       r = regmap_read(iod->regmap, reg->reg_coarse_offset, &val);
+       if (r)
+               return r;
+       ival->coarse_ref_count =
+           ti_iodelay_extract(val, reg->coarse_ref_count_mask);

drivers/pinctrl/pinctrl-ti-iodelay.c line 337

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+       ival->coarse_delay_count =
+           ti_iodelay_extract(val, reg->coarse_delay_count_mask);
+       if (!ival->coarse_delay_count) {
+               dev_err(dev, "Invalid Coarse delay count (0) (reg=0x%08x)\n",
+                       val);
+               return -EINVAL;
+       }
+       ival->cdpe = ti_iodelay_compute_dpe(ival->ref_clk_period,
+                                           ival->coarse_ref_count,
+                                           ival->coarse_delay_count, 88);

88? What does that mean... This should be #defined I think.

drivers/pinctrl/pinctrl-ti-iodelay.c line 432

Quoted 4 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +4 -0

+static int ti_iodelay_dt_node_to_map(struct pinctrl_dev *pctldev,
+                                    struct device_node *np,
+                                    struct pinctrl_map **map,
+                                    unsigned *num_maps)

If you use the generic bindings this quite complex parsing and everything associated with it goes away into the core.

drivers/pinctrl/pinctrl-ti-iodelay.c line 533

Quoted 2 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +2 -0

+static void ti_iodelay_dt_free_map(struct pinctrl_dev *pctldev,
+                                  struct pinctrl_map *map, unsigned num_maps)

Dito.

BTW even if you proceed this way I suspect these are verbatim copies from pinctrl-single so they shold obviouslyt be abstracted out as library functions in that case.

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+/**
+ * ti_iodelay_pinctrl_get_group_pins() - get group pins
+ * @pctldev:   pinctrl device representing IODelay device
+ * @gselector: Group selector
+ * @pins:      pointer to the pins
+ * @npins:     number of pins
+ *
+ * Dummy implementation since we do not track pins, we track configurations
+ * Forced by pinctrl's pinctrl_check_ops()
+ *

drivers/pinctrl/pinctrl-ti-iodelay.c line 629

Quoted 10 lines

drivers/pinctrl/pinctrl-ti-iodelay.c new file +10 -0

+ * Return: -EINVAL
+ */
+static int ti_iodelay_pinctrl_get_group_pins(struct pinctrl_dev *pctldev,
+                                            unsigned gselector,
+                                            const unsigned **pins,
+                                            unsigned *npins)
+{
+       /* Dummy implementation - we dont do pin mux */
+       return -EINVAL;
+}

This is not about muxing. It is about enumerating the pins in the group that will be affected by the setting, which is something you will want to do when using the generic bindings.

Quoted 1 line

+ group = ti_iodelay_get_group(iod, gselector);

And this seems to be your re-implementation of exactly that pin control core functionality. Which again should be shared with pinctrl-single if you anyway go down this path.

I'll halt review here pending discussion on the bindings & stuff.