[RFC] pinctrl-single: parse #pinctrl-cells = 2
diff mbox series

Message ID 20200615230906.GA3967771@x1
State New
Headers show
Series
  • [RFC] pinctrl-single: parse #pinctrl-cells = 2
Related show

Commit Message

Drew Fustini June 15, 2020, 11:09 p.m. UTC
These changes are based on feedback from Tony [1] concerning changing
pinctrl-single to be able to handle pinctrl-cells = 2 for for the 
"pinctrl-single,pins" property.

AM33XX_PADCONF macro is modified to keep pin conf and pin mux seperate.

If "pinctrl-single,pins" has 3 arguments (offset, conf, mux) then
pcs_parse_one_pinctrl_entry() does an OR operation on to get the
value to store in the register.

[1] https://lore.kernel.org/linux-omap/20200609180404.GR37466@atomide.com/#t

Signed-off-by: Drew Fustini <drew@beagleboard.org>
---
 arch/arm/boot/dts/am33xx-l4.dtsi   |  4 ++--
 drivers/pinctrl/pinctrl-single.c   | 14 ++++++++++++--
 include/dt-bindings/pinctrl/omap.h |  2 +-
 3 files changed, 15 insertions(+), 5 deletions(-)

Comments

Tony Lindgren June 16, 2020, 2:26 p.m. UTC | #1
* Drew Fustini <drew@beagleboard.org> [200615 23:10]:
> These changes are based on feedback from Tony [1] concerning changing
> pinctrl-single to be able to handle pinctrl-cells = 2 for for the 
> "pinctrl-single,pins" property.

Hey this is great! Thanks for sorting it out.

> --- a/drivers/pinctrl/pinctrl-single.c
> +++ b/drivers/pinctrl/pinctrl-single.c
> @@ -1017,11 +1017,21 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
>  			break;
>  		}
>  
> -		/* Index plus one value cell */
>  		offset = pinctrl_spec.args[0];
>  		vals[found].reg = pcs->base + offset;
> -		vals[found].val = pinctrl_spec.args[1];
>  
> +		pr_info("%s: args_count=%d offset=0x%x", __func__,
> +				pinctrl_spec.args_count, offset);
> +		pr_info("%s: args[1]=0x%x", __func__, pinctrl_spec.args[1]);
> +
> +		if (pinctrl_spec.args_count == 2) {
> +			vals[found].val = pinctrl_spec.args[1];
> +		} else if (pinctrl_spec.args_count == 3) {
> +			pr_info("%s: args[2]=0x%x", __func__, pinctrl_spec.args[2]);
> +			vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
> +		}

Maybe do the above with a switch? And leave out the pr_info for the
final version :) Also please do a separate patch for pinctrl-singl.c,
and then another patch for the define and dts change.

Hmm so now the conf and mux values are still register masks in the dts
which is not ideal in all cases. But that's a separate issue and could
be sorted out as needed later on with adding separate conf and mux
shifts and masks. Not sure if we want to do that for the existing use
cases though.

Regards,

Tony
Drew Fustini June 16, 2020, 4:53 p.m. UTC | #2
On Tue, Jun 16, 2020 at 07:26:28AM -0700, Tony Lindgren wrote:
> * Drew Fustini <drew@beagleboard.org> [200615 23:10]:
> > These changes are based on feedback from Tony [1] concerning changing
> > pinctrl-single to be able to handle pinctrl-cells = 2 for for the 
> > "pinctrl-single,pins" property.
> 
> Hey this is great! Thanks for sorting it out.
> 
> > --- a/drivers/pinctrl/pinctrl-single.c
> > +++ b/drivers/pinctrl/pinctrl-single.c
> > @@ -1017,11 +1017,21 @@ static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
> >  			break;
> >  		}
> >  
> > -		/* Index plus one value cell */
> >  		offset = pinctrl_spec.args[0];
> >  		vals[found].reg = pcs->base + offset;
> > -		vals[found].val = pinctrl_spec.args[1];
> >  
> > +		pr_info("%s: args_count=%d offset=0x%x", __func__,
> > +				pinctrl_spec.args_count, offset);
> > +		pr_info("%s: args[1]=0x%x", __func__, pinctrl_spec.args[1]);
> > +
> > +		if (pinctrl_spec.args_count == 2) {
> > +			vals[found].val = pinctrl_spec.args[1];
> > +		} else if (pinctrl_spec.args_count == 3) {
> > +			pr_info("%s: args[2]=0x%x", __func__, pinctrl_spec.args[2]);
> > +			vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
> > +		}
> 
> Maybe do the above with a switch? And leave out the pr_info for the
> final version :) Also please do a separate patch for pinctrl-singl.c,
> and then another patch for the define and dts change.

Thanks for the suggestions.  Yes, I wasn't sure what the best way to
express this "rough draft" solution.  I'll create a patch series in
future.

> Hmm so now the conf and mux values are still register masks in the dts
> which is not ideal in all cases. But that's a separate issue and could
> be sorted out as needed later on with adding separate conf and mux
> shifts and masks. Not sure if we want to do that for the existing use
> cases though.
> 
> Regards,
> 
> Tony

By register masks, do you mean the #define's like PIN_OUTPUT_PULLDOWN
and MUX_MODE6?

AM33XX_PADCONF(AM335X_PIN_GPMC_A2, PIN_OUTPUT_PULLDOWN, MUX_MODE6)

There is a possibility to also use the pinconf properties defined in
pinctrl-single.c:

	static const struct pcs_conf_type prop2[] = {
		{ "pinctrl-single,drive-strength", PIN_CONFIG_DRIVE_STRENGTH, },
		{ "pinctrl-single,slew-rate", PIN_CONFIG_SLEW_RATE, },
		{ "pinctrl-single,input-schmitt", PIN_CONFIG_INPUT_SCHMITT, },
		{ "pinctrl-single,low-power-mode", PIN_CONFIG_LOW_POWER_MODE, },
	};
	static const struct pcs_conf_type prop4[] = {
		{ "pinctrl-single,bias-pullup", PIN_CONFIG_BIAS_PULL_UP, },
		{ "pinctrl-single,bias-pulldown", PIN_CONFIG_BIAS_PULL_DOWN, },
		{ "pinctrl-single,input-schmitt-enable",
			PIN_CONFIG_INPUT_SCHMITT_ENABLE, },
	};

For example, I was experimenting with how to define the bias properties:

        ehrpwm0_pins: pinmux-ehrpwm0-pins {
                pinctrl-single,pins = <
                        AM33XX_PADCONF(AM335X_PIN_MCASP0_ACLKX, PIN_OUTPUT_PULLDOWN, MUX_MODE1)
                        /* (A13) mcasp0_aclkx.ehrpwm0A */
                >;
                pinctrl-single,bias-pullup = <24 24 0 24>;
        };

        ehrpwm1_pins: pinmux-ehrpwm1-pins {
                pinctrl-single,pins = <
                        AM33XX_PADCONF(AM335X_PIN_GPMC_A2, PIN_OUTPUT_PULLDOWN, MUX_MODE6)
                        /* (U14) gpmc_a2.ehrpwm1A */
                >;
                pinctrl-single,bias-pulldown = <8 8 0 24>;
        };

I found the binding documentation [0] for the bias properties to be very
confusing as to how those 4 values work:

> - pinctrl-single,bias-pullup : array of value that are used to configure the
>   input bias pullup in the pinmux register.
>
> 		/* input, enabled pullup bits, disabled pullup bits, mask */
> 		pinctrl-single,bias-pullup = <0 1 0 1>;
> 
> - pinctrl-single,bias-pulldown : array of value that are used to configure the
>   input bias pulldown in the pinmux register.

>		/* input, enabled pulldown bits, disabled pulldown bits, mask */
>		pinctrl-single,bias-pulldown = <2 2 0 2>;

For AM3358, the pin conf
register has the format [1]:

bit	attribute      value
  6	slew           { 0: fast, 1: slow }
  5     rx_active      { 0: rx disable, 1: rx enabled }
  4     pu_typesel     { 0: pulldown select, 1: pullup select }
  3     puden          { 0: pud enable, 1: disabled }
  2     mode           3 bits to selec mode 0 to 7
  1     mode
  0     mode

And I figured out the values for the bias-pull{up,down} properties:

        16      8       4       2       1
        2^4     2^3     2^2     2^1     2^0
mask    1       1       0       0       0       24
pull-up 1       1       0       0       0       24
pull-dn 0       1       0       0       0       8
none    0       0       0       0       0       0


I did some testing with pr_info's sprinkled in and I think those values
are correct but I would be happy to hear from someone with more insight
in the design of bias-pulldown and bias-pullup.

thanks,
drew

[0] Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
[1] https://www.ti.com/lit/ug/spruh73q/spruh73q.pdf (see Figure 9-51)
Tony Lindgren June 17, 2020, 4:54 p.m. UTC | #3
* Drew Fustini <drew@beagleboard.org> [200616 16:54]:
> By register masks, do you mean the #define's like PIN_OUTPUT_PULLDOWN
> and MUX_MODE6?
> 
> AM33XX_PADCONF(AM335X_PIN_GPMC_A2, PIN_OUTPUT_PULLDOWN, MUX_MODE6)
> 
> There is a possibility to also use the pinconf properties defined in
> pinctrl-single.c:
> 
> 	static const struct pcs_conf_type prop2[] = {
> 		{ "pinctrl-single,drive-strength", PIN_CONFIG_DRIVE_STRENGTH, },
> 		{ "pinctrl-single,slew-rate", PIN_CONFIG_SLEW_RATE, },
> 		{ "pinctrl-single,input-schmitt", PIN_CONFIG_INPUT_SCHMITT, },
> 		{ "pinctrl-single,low-power-mode", PIN_CONFIG_LOW_POWER_MODE, },
> 	};
> 	static const struct pcs_conf_type prop4[] = {
> 		{ "pinctrl-single,bias-pullup", PIN_CONFIG_BIAS_PULL_UP, },
> 		{ "pinctrl-single,bias-pulldown", PIN_CONFIG_BIAS_PULL_DOWN, },
> 		{ "pinctrl-single,input-schmitt-enable",
> 			PIN_CONFIG_INPUT_SCHMITT_ENABLE, },
> 	};

Oh right I forgot about that :)

> I did some testing with pr_info's sprinkled in and I think those values
> are correct but I would be happy to hear from someone with more insight
> in the design of bias-pulldown and bias-pullup.

OK thanks for testing with the pinconf properties.

Regards,

Tony

Patch
diff mbox series

diff --git a/arch/arm/boot/dts/am33xx-l4.dtsi b/arch/arm/boot/dts/am33xx-l4.dtsi
index 340ea331e54d..0b0439b53a0e 100644
--- a/arch/arm/boot/dts/am33xx-l4.dtsi
+++ b/arch/arm/boot/dts/am33xx-l4.dtsi
@@ -288,9 +288,9 @@  scm: scm@0 {
 				ranges = <0 0 0x2000>;
 
 				am33xx_pinmux: pinmux@800 {
-					compatible = "pinctrl-single";
+					compatible = "pinconf-single";
 					reg = <0x800 0x238>;
-					#pinctrl-cells = <1>;
+					#pinctrl-cells = <2>;
 					pinctrl-single,register-width = <32>;
 					pinctrl-single,function-mask = <0x7f>;
 				};
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c
index 02f677eb1d53..080083b8b5be 100644
--- a/drivers/pinctrl/pinctrl-single.c
+++ b/drivers/pinctrl/pinctrl-single.c
@@ -1017,11 +1017,21 @@  static int pcs_parse_one_pinctrl_entry(struct pcs_device *pcs,
 			break;
 		}
 
-		/* Index plus one value cell */
 		offset = pinctrl_spec.args[0];
 		vals[found].reg = pcs->base + offset;
-		vals[found].val = pinctrl_spec.args[1];
 
+		pr_info("%s: args_count=%d offset=0x%x", __func__,
+				pinctrl_spec.args_count, offset);
+		pr_info("%s: args[1]=0x%x", __func__, pinctrl_spec.args[1]);
+
+		if (pinctrl_spec.args_count == 2) {
+			vals[found].val = pinctrl_spec.args[1];
+		} else if (pinctrl_spec.args_count == 3) {
+			pr_info("%s: args[2]=0x%x", __func__, pinctrl_spec.args[2]);
+			vals[found].val = (pinctrl_spec.args[1] | pinctrl_spec.args[2]);
+		}
+
+		pr_info("%s: vals[%d].val=0x%x", __func__, found, vals[found].val);
 		dev_dbg(pcs->dev, "%pOFn index: 0x%x value: 0x%x\n",
 			pinctrl_spec.np, offset, pinctrl_spec.args[1]);
 
diff --git a/include/dt-bindings/pinctrl/omap.h b/include/dt-bindings/pinctrl/omap.h
index 625718042413..2d2a8c737822 100644
--- a/include/dt-bindings/pinctrl/omap.h
+++ b/include/dt-bindings/pinctrl/omap.h
@@ -65,7 +65,7 @@ 
 #define DM814X_IOPAD(pa, val)		OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
 #define DM816X_IOPAD(pa, val)		OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
 #define AM33XX_IOPAD(pa, val)		OMAP_IOPAD_OFFSET((pa), 0x0800) (val)
-#define AM33XX_PADCONF(pa, dir, mux)	OMAP_IOPAD_OFFSET((pa), 0x0800) ((dir) | (mux))
+#define AM33XX_PADCONF(pa, conf, mux)	OMAP_IOPAD_OFFSET((pa), 0x0800) (conf) (mux)
 
 /*
  * Macros to allow using the offset from the padconf physical address