Re: problem with pinctrl-single,bits and control_devconf0

[Tony Lindgren]

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

Regards,

Tony