| Message ID | 20200615230906.GA3967771@x1 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC] pinctrl-single: parse #pinctrl-cells = 2 | expand |
* 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
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)
* 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
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
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(-)