reason for Allwinner SoC specific pinctrl drivers?

reason for Allwinner SoC specific pinctrl drivers?

[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@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.

reason for Allwinner SoC specific pinctrl drivers?

[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?

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.

Re: reason for Allwinner SoC specific pinctrl drivers?

[Vishnu Patekar]

[-- Attachment #1: Type: text/plain, Size: 2973 bytes --]

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

On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>
> 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.
AFAIK, there are lot of holes in pin conf registers.
This will complicate the dtsi configuration.
The current pinctrl driver is best possible solution with respect to time
and availability of limited people working on sunxi with the cost of
increase in code size.

It's better to give it a shot to make it generic driver for future SOCs
atleast. Like A83, A64, etc.

Possible solution will always be examined by experts Linus, Maxine, etc.

Regards,
Vishnu

>
> 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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
> For more options, visit https://groups.google.com/d/optout.

-- 
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-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit https://groups.google.com/d/optout.

[-- Attachment #2: Type: text/html, Size: 3987 bytes --]

Re: reason for Allwinner SoC specific pinctrl drivers?

[Michal Suchanek]

Hello,

On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> Hello Andre,
> This is something we can do for future SOCs.
>
> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>> 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";

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.

Thanks

Michal

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Michal Suchanek]

Hello,

On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510@gmail.com> wrote:
> Hello Andre,
> This is something we can do for future SOCs.
>
> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara@arm.com> wrote:
>>
>> 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";

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.

Thanks

Michal

Re: reason for Allwinner SoC specific pinctrl drivers?

[Andre Przywara]

Hi Michal,

thanks for your input!

On 04/01/16 21:36, Michal Suchanek wrote:
> Hello,
> 
> On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> Hello Andre,
>> This is something we can do for future SOCs.
>>
>> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>>>
>>> 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";
> 
> 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.

> 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?

> 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.

> 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.

> 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>;

> 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.

> 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.

That doesn't sound to tempting to me.

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

So I guess I just failed to express my approach properly.
I will try to code a proof of concept this week and convert once SoC
over to the new driver, maybe this clears things up.

If I have a blatant misunderstanding of the concepts (quite possible),
please keep correcting me!

Thanks!
Andre.

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Andre Przywara]

Hi Michal,

thanks for your input!

On 04/01/16 21:36, Michal Suchanek wrote:
> Hello,
> 
> On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510@gmail.com> wrote:
>> Hello Andre,
>> This is something we can do for future SOCs.
>>
>> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara@arm.com> wrote:
>>>
>>> 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";
> 
> 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.

> 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?

> 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.

> 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.

> 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>;

> 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.

> 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.

That doesn't sound to tempting to me.

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

So I guess I just failed to express my approach properly.
I will try to code a proof of concept this week and convert once SoC
over to the new driver, maybe this clears things up.

If I have a blatant misunderstanding of the concepts (quite possible),
please keep correcting me!

Thanks!
Andre.

Re: [linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Michal Suchanek]

On 5 January 2016 at 13:05, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Michal,
>
> thanks for your input!
>
> On 04/01/16 21:36, Michal Suchanek wrote:
>> Hello,
>>
>> On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510@gmail.com> wrote:
>>> Hello Andre,
>>> This is something we can do for future SOCs.
>>>
>>> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara@arm.com> wrote:
>>>>

>>>>
>>>>         uart0_pins_a: uart0@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.
>
>> 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?

You cannot. The muxing fails because you request a function that is
not in the table for that pin - as opposed to muxing some other random
function on the pin silently.

>
>> 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.

The function string is needed for the setting to make any sense.

As you have pointed out yourself the function name may resolve to
different muxval for different pins.

You need those SoC pin tables so you know what you multiplexed to
what. They are in the kernel where they IMHO belong. Not having them
at all is a no-go because then you have no idea what functions you
have enabled on the SoC.

What you suggest is adding duplicate lower level information in the DT
so higher level pinmux driver uses symbolic function name and lower
level driver using raw mux number. This means that maintenance
problems increase with these low level and high level pin mux
descriptions potentially going out of sync in the DT.

>
>> 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.

We cannot get rid of it. And really, the assembly is the same for all
the SoCs. It's the machine code which isn't and which the assembly
abstracts.

>
>> 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>;

Except as has been pointed out in DT unused features are not to be
included so you would lose the functions which are included in the
in-kernel tables but are not wired on any supported board.

Also the uart0_pins_a part is just trashed AFAIK so once the kernel
boots you have no idea what pins you got in the driver. So if the low
level pinmux driver were to replace the high level pinmux driver it
would have to parse the pin tables in the DT and reconstruct the
currently hardcoded pin tables from the DT information so that when
you look at the pinmux state later you know what it means without the
in-kernel hardcoded table. Then all pinmux values are technically used
by the pinmux driver to reconstruct the pinmux table and should be
included in the 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.
>
> 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.

No. You can just include the pin table in the C code as opposed to
linking in a DT parser as well as requiring heap space to parse the
DT.

well, you can always cut and paste the data from DT to any code you
want, anyway. So long as you do have the data somewhere.

Thanks

Michal

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Michal Suchanek]

On 5 January 2016 at 13:05, Andre Przywara <andre.przywara@arm.com> wrote:
> Hi Michal,
>
> thanks for your input!
>
> On 04/01/16 21:36, Michal Suchanek wrote:
>> Hello,
>>
>> On 4 January 2016 at 18:27, Vishnu Patekar <vishnupatekar0510@gmail.com> wrote:
>>> Hello Andre,
>>> This is something we can do for future SOCs.
>>>
>>> On 4 Jan 2016 19:02, "Andre Przywara" <andre.przywara@arm.com> wrote:
>>>>

>>>>
>>>>         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.
>
>> 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?

You cannot. The muxing fails because you request a function that is
not in the table for that pin - as opposed to muxing some other random
function on the pin silently.

>
>> 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.

The function string is needed for the setting to make any sense.

As you have pointed out yourself the function name may resolve to
different muxval for different pins.

You need those SoC pin tables so you know what you multiplexed to
what. They are in the kernel where they IMHO belong. Not having them
at all is a no-go because then you have no idea what functions you
have enabled on the SoC.

What you suggest is adding duplicate lower level information in the DT
so higher level pinmux driver uses symbolic function name and lower
level driver using raw mux number. This means that maintenance
problems increase with these low level and high level pin mux
descriptions potentially going out of sync in the DT.

>
>> 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.

We cannot get rid of it. And really, the assembly is the same for all
the SoCs. It's the machine code which isn't and which the assembly
abstracts.

>
>> 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>;

Except as has been pointed out in DT unused features are not to be
included so you would lose the functions which are included in the
in-kernel tables but are not wired on any supported board.

Also the uart0_pins_a part is just trashed AFAIK so once the kernel
boots you have no idea what pins you got in the driver. So if the low
level pinmux driver were to replace the high level pinmux driver it
would have to parse the pin tables in the DT and reconstruct the
currently hardcoded pin tables from the DT information so that when
you look at the pinmux state later you know what it means without the
in-kernel hardcoded table. Then all pinmux values are technically used
by the pinmux driver to reconstruct the pinmux table and should be
included in the 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.
>
> 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.

No. You can just include the pin table in the C code as opposed to
linking in a DT parser as well as requiring heap space to parse the
DT.

well, you can always cut and paste the data from DT to any code you
want, anyway. So long as you do have the data somewhere.

Thanks

Michal

Re: reason for Allwinner SoC specific pinctrl drivers?

[Julian Calaby]

Hi Andre,

On Mon, Jan 4, 2016 at 10:02 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> 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.

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.

Thanks,

-- 
Julian Calaby

Email: julian.calaby-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
Profile: http://www.google.com/profiles/julian.calaby/

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Julian Calaby]

Hi Andre,

On Mon, Jan 4, 2016 at 10:02 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 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.

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.

Thanks,

-- 
Julian Calaby

Email: julian.calaby at gmail.com
Profile: http://www.google.com/profiles/julian.calaby/

Re: reason for Allwinner SoC specific pinctrl drivers?

[Andre Przywara]

Hi Julian,

On 04/01/16 22:04, Julian Calaby wrote:
> Hi Andre,
> 
> On Mon, Jan 4, 2016 at 10:02 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>> 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.
> 
> 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.

Those are good points, thanks for bringing them up.
Hopefully they are no show stoppers... I will try to come up with some
kind of workaround for 2. and 3.
We could find a way to document the stuff somewhere, I guess the sunxi
wiki would be a good start.
But actually we should just reference to publicly available docs, say
the Allwinner documentation github repo. I know, I know ... ;-)
Is there any known wrong or missing information for the port controller
and mux settings in Allwinner's doc?

Cheers,
Andre.

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Andre Przywara]

Hi Julian,

On 04/01/16 22:04, Julian Calaby wrote:
> Hi Andre,
> 
> On Mon, Jan 4, 2016 at 10:02 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> 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.
> 
> 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.

Those are good points, thanks for bringing them up.
Hopefully they are no show stoppers... I will try to come up with some
kind of workaround for 2. and 3.
We could find a way to document the stuff somewhere, I guess the sunxi
wiki would be a good start.
But actually we should just reference to publicly available docs, say
the Allwinner documentation github repo. I know, I know ... ;-)
Is there any known wrong or missing information for the port controller
and mux settings in Allwinner's doc?

Cheers,
Andre.

Re: reason for Allwinner SoC specific pinctrl drivers?

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3714 bytes --]

Hi Andre,

On Mon, Jan 04, 2016 at 11:02:37AM +0000, Andre Przywara wrote:
> 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.

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.

> 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?

There's also the interrupts over GPIOs that change slightly, the A20
having a single bank and the A64 having several of them.

The interrupt mechanism is quite interesting, since it's also a muxing
option of a given pin, and only a few pins implement that option. On
the A20 and earlier, we had 32 interrupts scattered around the
available pins, on A31 and later, we still have the same mechanism,
this time with multiple banks of 32 interrupts.

We have to have a table somewhere as well to give the mapping a pin
and the interrupt that might be connected to it.

> 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 guess in the such a case you wouldn't need the function string at
all then?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

reason for Allwinner SoC specific pinctrl drivers?

[Maxime Ripard]

Hi Andre,

On Mon, Jan 04, 2016 at 11:02:37AM +0000, Andre Przywara wrote:
> 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.

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.

> 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?

There's also the interrupts over GPIOs that change slightly, the A20
having a single bank and the A64 having several of them.

The interrupt mechanism is quite interesting, since it's also a muxing
option of a given pin, and only a few pins implement that option. On
the A20 and earlier, we had 32 interrupts scattered around the
available pins, on A31 and later, we still have the same mechanism,
this time with multiple banks of 32 interrupts.

We have to have a table somewhere as well to give the mapping a pin
and the interrupt that might be connected to it.

> 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 guess in the such a case you wouldn't need the function string at
all then?

Thanks,
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160105/27733273/attachment.sig>

Re: reason for Allwinner SoC specific pinctrl drivers?

[Linus Walleij]

On Tue, Jan 5, 2016 at 2:10 PM, Maxime Ripard
<maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org> wrote:

> 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.

They call it Stockholm Syndrome when you start to sympathize with
your kidnappers :D

On a serious note, we are in violent agreement on these bullets.
This driver is easy to maintain and debug, and easy for new developers
to get into precisely because so much knowledge is in the kernel
and not in DT. A quick tour in /sys/kernel/debug/pinctrl/*
should convince anyone that this is actually very helpful to get
things right.

Yours,
Linus Walleij

reason for Allwinner SoC specific pinctrl drivers?

[Linus Walleij]

On Tue, Jan 5, 2016 at 2:10 PM, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:

> 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.

They call it Stockholm Syndrome when you start to sympathize with
your kidnappers :D

On a serious note, we are in violent agreement on these bullets.
This driver is easy to maintain and debug, and easy for new developers
to get into precisely because so much knowledge is in the kernel
and not in DT. A quick tour in /sys/kernel/debug/pinctrl/*
should convince anyone that this is actually very helpful to get
things right.

Yours,
Linus Walleij

Re: [linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Chen-Yu Tsai]

On Mon, Jan 4, 2016 at 7:02 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 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>;
>         };

Using a mux value over a string does not gain us much, other than
smaller driver code, since all the strings are gone, and smaller DT,
again no strings. However, if you want some sensible debug output
from the driver, you're going to keep the strings anyway.

The mux values would need to be macros defined in some header file,
instead of raw values which are hard to understand.

That's only half of it. The driver should know what pins are available
and which mux values can be used for them. So you'd still need a table
of some sort. That means it's unlikely you can have a generic driver.
Leaking the internals into the DT is not a good way either.

The current approach of a generic driver library and SoC specific
tables maybe isn't the best approach, but it is easy enough to add new
SoC support.


Regards
ChenYu

> 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@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[linux-sunxi] reason for Allwinner SoC specific pinctrl drivers?

[Chen-Yu Tsai]

On Mon, Jan 4, 2016 at 7:02 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 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>;
>         };

Using a mux value over a string does not gain us much, other than
smaller driver code, since all the strings are gone, and smaller DT,
again no strings. However, if you want some sensible debug output
from the driver, you're going to keep the strings anyway.

The mux values would need to be macros defined in some header file,
instead of raw values which are hard to understand.

That's only half of it. The driver should know what pins are available
and which mux values can be used for them. So you'd still need a table
of some sort. That means it's unlikely you can have a generic driver.
Leaking the internals into the DT is not a good way either.

The current approach of a generic driver library and SoC specific
tables maybe isn't the best approach, but it is easy enough to add new
SoC support.


Regards
ChenYu

> 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.