Re: problem with pinctrl-single,bits and control_devconf0

[H. Nikolaus Schaller]

Hi,

> Am 20.09.2016 um 23:45 schrieb Tony Lindgren <tony@xxxxxxxxxxx>:
> 
> * H. Nikolaus Schaller <hns@xxxxxxxxxxxxx> [160920 01:44]:
>> 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.
> 
> OK, care to update the doc while at it so we can avoid running
> into this again?

Yes, I'd like to.

The problem is that I need some confirmation that my understanding is
really correct... I just did guess and succeed.

So someone who understands the driver code should comment first.
Then I can prepare a patch for the documentation.

BR,
Nikolaus