Re: problem with pinctrl-single,bits and control_devconf0

[H. Nikolaus Schaller]

Hi Tony,
(I had forgotten to cc LKML so I have added it).

> Am 19.09.2016 um 20:07 schrieb Tony Lindgren <tony@xxxxxxxxxxx>:
> 
> * H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> [160918 06:53]:
>> Hi,
>> I am trying to set up a special McBSP1 CLKR on OMAP3, but I don't understand
>> the logic of the offsets and masks behind pinctrl-single,bits.
>> 
>> I have modified the example given in the bindings document
>> 
>> 	http://lxr.free-electrons.com/source/Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt?v=3.8#L110
>> 
>> to:
>> 
>> / {
>> 	/* pinmux for devconf0 */
>> 	control_devconf0: pinmux@48002274 {
>> 		compatible = "pinctrl-single";
>> 		reg = <0x48002274 4>;   /* Single register */
>> 		#address-cells = <1>;
>> 		#size-cells = <0>;
>> 		pinctrl-single,bit-per-mux;
>> 		pinctrl-single,register-width = <32>;
>> 		pinctrl-single,function-mask = <0x5F>;
>> 	};
>> };
>> 
>> &control_devconf0 {
>> 	pinctrl-names = "default";
>> 	pinctrl-0 = <&mcbsp1_defconf0>;
>> 
>> 	mcbsp1_defconf0: pinmux_mcbsp1_defconf0 {
>> 		/*                   offset bits mask */
>> 		pinctrl-single,bits = <0x00 0x08 0x08>;	/* set MCBSP1_CLKR_MASK */
>> 	};
>> };
>> 
>> This looks reasonable to me to set bit 0x08 of the DEVCONF0 register which is within the set of the bits "enabled" for modification by the mask 0x5F.
> 
> Yes as long as devconf0 is purely a mux register with no extra features
> like regulators or clocks.
> 
>> All I get from this is a reject:
>> 
>> [    1.808258] pinctrl-single 48002274.pinmux: Invalid submask 0x8 for pinmux_mcbsp1_defconf0 at 0x0
>> 
>> So what is wrong?
>> 
>> I have added some printk to drivers/pinctrl/pinctrl-single.c to understand what
>> the bits, offsets and masks are thought to do.
>> 
>> [    1.807220] pinctrl-single: fmask=0000005f fshift=0 fmax=0000005f
>> [    1.807464] pinctrl-single: pcs->fmask = 0000005f
>> [    1.807495] pinctrl-single: mask = 00000008 bit_pos = 3 pin_num_from_lsb = 0 mask_pos = 000002f8 val_pos = 00000008 submask = 00000008
>> [    1.807495] pinctrl-single:  -> mask = 00000000
>> [    1.807525] pinctrl-single 48002274.pinmux: Invalid submask 0x8 for pinmux_mcbsp1_defconf0 at 0x0
>> 
>> What I don't understand at all is why the mask 0x5f gives a mask_pos = 000002f8.
>> Of course then the mask can never be the same as the submask.
>> 
>> Or is the example in the bindings document wrong?
> 
> Hmm can't say what might be wrong, it is pretty widely used for da850 and
> keystone. Maybe check those dtsi files and see if there's some difference
> compared to the docs?

Yes, that was a good hint.

I found how it is used in da850.dtsi and it looks as if the documentation is
too sparse and the example is wrong.

The reason seems to be that pinctrl-single,function-mask has changed its
role some time ago. At least

https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=055cb2a9e065c3a606f57fbcf8de1689f7c1fedf

seems to indicate that.

My current working hypothesis is that function-mask no longer specifies
which bits *can* be changed in total (depending on the individual mask)
but which bits *must* be changed by a single mask.

So the old one was to reject setting bits that are not to be changed while
the new one enforces several bits to be changed as a group.

0x5f did mean: you can change any of these 5 bits but with the modified
scheme it seems to mean: you must set/reset all these bits in parallel.

Hence setting just one bit (my example) or two bits (bindings document
example) is rejected.

This did not at all become clear to me from the bindings description...

I have tried both:

>> pinctrl-single,function-mask = <0x01>;
>> pinctrl-single,bits = <0x00 0x08 0x08>;

>> pinctrl-single,function-mask = <0x5F>;
>> pinctrl-single,bits = <0x00 0x08 0x5f>;

and both are not rejected and set the expected value.

BR and thanks,
Nikolaus