reason for Allwinner SoC specific pinctrl drivers?

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

        uart0_pins_a: uart0@0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";
                allwinner,drive = <SUN4I_PINCTRL_10_MA>;
                allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
        };

Would it make sense that I sit down and prototype such a driver?

We should keep compatibility with older DTs by keeping the existing drivers in (or maybe emulating the current behaviour by providing just those tables as a fallback) , but newer SoCs (like the A64?) would not need a SoC specific driver, but just go with that generic driver and appropriate DT properties.

I appreciate any comments on this, especially if I missed something which would render this approach impossible or tedious.

Cheers,
Andre.

--
You received this message because you are subscribed to the Google Groups

To unsubscribe from this group and stop receiving emails from it, send an

For more options, visit https://groups.google.com/d/optout.

Hello Andre,
This is something we can do for future SOCs.

Quoted 22 lines · Andre Przywara

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";

Hello,

Quoted 22 lines · Vishnu Patekar

Hello Andre,
This is something we can do for future SOCs.

Quoted 18 lines · Andre Przywara

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

Quoted 4 lines

Quoted 4 lines

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";

As I understand it

1) uart0 is basically a mnemonic for muxval 2

2) if you try to mux uart0 on pins for which it is not in the table it fails

So it makes no sense to have both function and muxval - this is redundant.

And it does not make sense to move from function to muxval - it's like moving from assembly programing to raw machine code programming.

For compatibility it's not possible to move the table to the shared SoC DT

although it would be possible to have the pin tables in DT. However, it would inflate the DT and make working in u-boot (SPL) where full DT parser is not available problematic.

What might be possible is merging the different pinmux drivers in one. Instead of replicating some pinmux boilerplate you will probably end up with lots of ifdefs so only tables for SoC support compiled in the kernel are built into the driver.

I am not sure how large the tables are and if anybody should care but you might be also missing some symbols for them.

Hi Michal,

thanks for your input!

Quoted 10 lines · Michal Suchanek

Hello,

Quoted 7 lines · Vishnu Patekar

Hello Andre,
This is something we can do for future SOCs.

Quoted 3 lines · Andre Przywara

Quoted 8 lines

Quoted 4 lines

Quoted 4 lines

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";

As I understand it

1) uart0 is basically a mnemonic for muxval 2

Not really. At the moment uart0 is used to lookup the (hard-coded) table entry for pins PB22 and PB23, which returns the said value of 0x02 (in this example, cf. line 246 in pinctrl-sun7i-a20.c).
But if there are other pins where UART0 can be muxed too, there is another node describing them (cf. uart3_pins_a and uart3_pins_b in sun7i-a20.dtsi). This one uses the "uart0" name as well, but the muxval returned for _those pins_ can be different. So the string only makes sense in connection with a certain pin.

Quoted 1 line

2) if you try to mux uart0 on pins for which it is not in the table it fails

How would you mux them if they are not in the table?

Quoted 2 lines

So it makes no sense to have both function and muxval - this is redundant.

Kind of, but not if we want to keep compatibility with older and newer DTs and older and newer kernels (drivers, really) - which is my goal. So we just _add_ the muxval values. The existing chip-specific drivers would naturally ignore the values and just use their built-in table for lookup. The generic driver on the other hand would use the DT information. An appropriate compatible string could then be added to refer to the generic driver as a fallback.
For (future) SoCs (which would not have a specific driver) we could omit the function string (if this isn't needed elsewhere, I have to check). So I don't see how the redundancy would be an issue here.

Quoted 3 lines

And it does not make sense to move from function to muxval - it's like moving from assembly programing to raw machine code programming.

But it removes the requirement of relying on the built-in lookup table. So by using a more readable uart0 "mnemonic" we rely on some hardcoded, chip specific table in each kernel, which is just wrong IMHO. Other DT users (be it Xen or *BSD, for instance) would have to replicate this table and since it's really SoC specific, it does not make any sense to me to keep it separate. After all this DT node is SoC specific as well, so I don't see the point of abstracting this with some string lookup.

So to stay with your comparison: Yes, we move from assembly to machine code, but we get rid of the need for a SoC specific assembler, which is maintained separately.

Quoted 3 lines

For compatibility it's not possible to move the table to the shared SoC DT

Why is that? We have the actual pin tables in the shared SoC DT, each board specific DT just refers to the actually connected pins by reference. That wouldn't change at all. So the above example for instance is from sun7i-a20.dtsi, board specific .dts files just use:
pinctrl-0 = <&uart0_pins_a>;

Quoted 4 lines

although it would be possible to have the pin tables in DT. However, it would inflate the DT and make working in u-boot (SPL) where full DT parser is not available problematic.

I don't get this. Having the actual values instead of a string lookup would make it actually easier to lookup for more light-weight kernels.

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";
                allwinner,drive = <SUN4I_PINCTRL_10_MA>;
                allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
        };

Would it make sense that I sit down and prototype such a driver?

We should keep compatibility with older DTs by keeping the existing drivers in (or maybe emulating the current behaviour by providing just those tables as a fallback) , but newer SoCs (like the A64?) would not need a SoC specific driver, but just go with that generic driver and appropriate DT properties.

I appreciate any comments on this, especially if I missed something which would render this approach impossible or tedious.

Hi Andre,

Quoted 27 lines · Andre Przywara

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";
                allwinner,drive = <SUN4I_PINCTRL_10_MA>;
                allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
        };

Would it make sense that I sit down and prototype such a driver?

Quoted 8 lines

We should keep compatibility with older DTs by keeping the existing drivers in (or maybe emulating the current behaviour by providing just those tables as a fallback) , but newer SoCs (like the A64?) would not need a SoC specific driver, but just go with that generic driver and appropriate DT properties.

I appreciate any comments on this, especially if I missed something which would render this approach impossible or tedious.

I think that, as Michal said, merging the drivers might be possible, however there's another three functions the drivers serve:

1. they're good documentation of how it's all configured. I'm not sure your device tree based approach will be as user friendly in this regard.

2. they list stuff we don't have a driver / hardware for yet

3. the policy on device-tree is to only include stuff we know is working, which means we have a driver and hardware for that particular thing. Device tree files for boards or SoCs have been rejected because they list stuff that isn't used yet.

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.
Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right? Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

        uart0_pins_a: uart0 at 0 {
                allwinner,pins =   "PB22", "PB23";
+               allwinner,muxval = <0x02    0x02>;
                allwinner,function = "uart0";
                allwinner,drive = <SUN4I_PINCTRL_10_MA>;
                allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
        };

Would it make sense that I sit down and prototype such a driver?

We should keep compatibility with older DTs by keeping the existing drivers in (or maybe emulating the current behaviour by providing just those tables as a fallback) , but newer SoCs (like the A64?) would not need a SoC specific driver, but just go with that generic driver and appropriate DT properties.

I appreciate any comments on this, especially if I missed something which would render this approach impossible or tedious.

Cheers,
Andre.

--
You received this message because you are subscribed to the Google Groups "linux-sunxi" group.
To unsubscribe from this group and stop receiving emails from it, send an email to linux-sunxi+unsubscribe at googlegroups.com. For more options, visit https://groups.google.com/d/optout.

Hi,

while looking at the Allwinner A64 SoC support, I was wondering why we would actually need a pinctrl driver (file) for each and every Allwinner SoC that we support.

Looking at both the A20 and the A64 doc I don't see any differences in the port controller implementation apart from the actual muxval <-> subsystem assignment, which is just data, right?

Comparing the code files in drivers/pinctrl/sunxi seems to support this, as those drivers only consist of the table and some boilerplate code.

Now I was wondering whether we could get away with one generic Allwinner pinctrl driver and put the SoC specific pin assignments in DT instead. It looks like adding an "allwinner,muxval" property in addition to the existing "allwinner,function" in the SoC's .dtsi would give us all the information we need. This could look like:

	uart0_pins_a: uart0 at 0 {
		allwinner,pins =   "PB22", "PB23";

(quoted diff) +1 -0

+		allwinner,muxval = <0x02    0x02>;

		allwinner,function = "uart0";
		allwinner,drive = <SUN4I_PINCTRL_10_MA>;
		allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
	};

Would it make sense that I sit down and prototype such a driver?

We should keep compatibility with older DTs by keeping the existing drivers in (or maybe emulating the current behaviour by providing just those tables as a fallback) , but newer SoCs (like the A64?) would not need a SoC specific driver, but just go with that generic driver and appropriate DT properties.

This was actually requested during the initial driver submission
http://lists.infradead.org/pipermail/linux-arm-kernel/2012-December/136809.html

And I actually came to like it, even though the initial port is indeed quite painful, for several reasons:

1) In those times, we didn't have a datasheet available for all these SoCs, so no easy way to find out the value to set in the register to mux a particular function in. These days are mostly over, but we're still in that situation for a few SoCs (mostly the R* and H* SoCs though).

2) It's quite easy to read and debug, at least from the DT PoV. The pain mostly comes from writing that table in the first place, but the alternative other SoCs have picked to have something readable in the DT is a SoC specific header file with the defines of all the functions availables for all the pins, which is just as painful to write and review, and is essentialy the same thing.

3) It's easy to maintain. If one of the entry in the table came to be wrong, we simply have to update the table, it's completely contained in the kernel itself and doesn't require fixing (and updating) the DT.

4) It allows to catch misconfiguration in the cases where you set a function on a pin that doesn't support it as well, which is also quite convenient, and wouldn't be possible at all in the case of a header, or simply open-coding the value.