| Message ID | 1413479495-14206-9-git-send-email-soren.brinkmann@xilinx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 10/16/2014 07:11 PM, Soren Brinkmann wrote: > Add pinctrl descriptions to the zc702 and zc706 device trees. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > --- > arch/arm/boot/dts/zynq-7000.dtsi | 8 ++- > arch/arm/boot/dts/zynq-zc702.dts | 147 +++++++++++++++++++++++++++++++++++++++ > arch/arm/boot/dts/zynq-zc706.dts | 126 +++++++++++++++++++++++++++++++++ > 3 files changed, 280 insertions(+), 1 deletion(-) > > diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi > index 24036c440440..37d7fe36a129 100644 > --- a/arch/arm/boot/dts/zynq-7000.dtsi > +++ b/arch/arm/boot/dts/zynq-7000.dtsi > @@ -238,7 +238,7 @@ > slcr: slcr@f8000000 { > #address-cells = <1>; > #size-cells = <1>; > - compatible = "xlnx,zynq-slcr", "syscon"; > + compatible = "xlnx,zynq-slcr", "syscon", "simple-bus"; I expect that you have this here to be able to probe the driver. slcr is not the bus. You should be able just to probe child nodes. Thanks, Michal
On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann <soren.brinkmann@xilinx.com> wrote: > Add pinctrl descriptions to the zc702 and zc706 device trees. > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> (...) > +&pinctrl0 { > + pinctrl_can0_default: pinctrl-can0-default { > + common { > + function = "can0"; > + groups = "can0_9_grp"; > + bias-pull-up = <0>; No. If you want pull-up, just use bias-pull-up; If you want to disable pull-up, use bias-disable; > + slew-rate = <0>; If this measure is any kind of time unit, this is against the laws of nature. > + io-standard = <1>; > + }; > + > + rx { > + pins = "MIO46"; > + bias-high-impedance = <1>; Just bias-high-impedance; > + }; > + > + tx { > + pins = "MIO47"; > + bias-high-impedance = <0>; Just bias-disable; Look all these over closely. Yours, Linus Walleij
On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote: > On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > > Add pinctrl descriptions to the zc702 and zc706 device trees. > > > > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > > (...) > > +&pinctrl0 { > > + pinctrl_can0_default: pinctrl-can0-default { > > + common { > > + function = "can0"; > > + groups = "can0_9_grp"; > > + bias-pull-up = <0>; > > No. If you want pull-up, just use > bias-pull-up; > > If you want to disable pull-up, use > bias-disable; But bias-disable also disables high-impedance. That doesn't work for me, I think. > > > + slew-rate = <0>; > > If this measure is any kind of time unit, this is against the laws of nature. It's not. As the bindings say, the argument is driver specific. In the Zynq case you can simply choose fast or slow - whatever that means - which maps to 0 or 1 in the DT. This though raises the question where that documentation lives. Since this is a driver specific interpretation, it should not be in the binding docs, IMHO. So, some other place might need to be found to document the implementation specifics. > > > + io-standard = <1>; > > + }; > > + > > + rx { > > + pins = "MIO46"; > > + bias-high-impedance = <1>; > > Just > bias-high-impedance; Same problem as I have above. To allow all permutations of pull-up and tri-state I can't just have a single disable-bias property. > > > + }; > > + > > + tx { > > + pins = "MIO47"; > > + bias-high-impedance = <0>; > > Just > bias-disable; That would also disable the pull-up. Sören
On Tue, Oct 28, 2014 at 5:03 PM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: > On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote: >> On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann >> <soren.brinkmann@xilinx.com> wrote: >> >> > Add pinctrl descriptions to the zc702 and zc706 device trees. >> > >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> >> >> (...) >> > +&pinctrl0 { >> > + pinctrl_can0_default: pinctrl-can0-default { >> > + common { >> > + function = "can0"; >> > + groups = "can0_9_grp"; >> > + bias-pull-up = <0>; >> >> No. If you want pull-up, just use >> bias-pull-up; >> >> If you want to disable pull-up, use >> bias-disable; > > But bias-disable also disables high-impedance. That doesn't work for me, > I think. Hm. Some sequencing problem right? Like you count on bias-high-impdedance being set in some other state? I think each state should be self-contained, so you set all the stuff you need in a state, do not rely on things coming in pre-set from another state. So in this case just set bias-high-impedance; then and if the state does not have bias-pull-up, *always* disable it in the driver. >> >> > + slew-rate = <0>; >> >> If this measure is any kind of time unit, this is against the laws of nature. > > It's not. As the bindings say, the argument is driver specific. Okay then. >> > + rx { >> > + pins = "MIO46"; >> > + bias-high-impedance = <1>; >> >> Just >> bias-high-impedance; > > Same problem as I have above. To allow all permutations of pull-up and > tri-state I can't just have a single disable-bias property. Again it seems to be a sequencing problem. And device tree is not good at sequences, therefore all states should be self-contained. Yours, Linus Walleij
On Fri, 2014-10-31 at 09:17AM +0100, Linus Walleij wrote: > On Tue, Oct 28, 2014 at 5:03 PM, Sören Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote: > >> On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann > >> <soren.brinkmann@xilinx.com> wrote: > >> > >> > Add pinctrl descriptions to the zc702 and zc706 device trees. > >> > > >> > Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> > >> > >> (...) > >> > +&pinctrl0 { > >> > + pinctrl_can0_default: pinctrl-can0-default { > >> > + common { > >> > + function = "can0"; > >> > + groups = "can0_9_grp"; > >> > + bias-pull-up = <0>; > >> > >> No. If you want pull-up, just use > >> bias-pull-up; > >> > >> If you want to disable pull-up, use > >> bias-disable; > > > > But bias-disable also disables high-impedance. That doesn't work for me, > > I think. > > Hm. Some sequencing problem right? Like you count on > bias-high-impdedance being set in some other state? > > I think each state should be self-contained, so you set > all the stuff you need in a state, do not rely on things coming > in pre-set from another state. > > So in this case just set bias-high-impedance; then and > if the state does not have bias-pull-up, *always* disable it > in the driver. > > >> > >> > + slew-rate = <0>; > >> > >> If this measure is any kind of time unit, this is against the laws of nature. > > > > It's not. As the bindings say, the argument is driver specific. > > Okay then. > > >> > + rx { > >> > + pins = "MIO46"; > >> > + bias-high-impedance = <1>; > >> > >> Just > >> bias-high-impedance; > > > > Same problem as I have above. To allow all permutations of pull-up and > > tri-state I can't just have a single disable-bias property. > > Again it seems to be a sequencing problem. And device tree is > not good at sequences, therefore all states should be self-contained. I agree, but how would I define a pin with pull-up enabled and tri-state disabled - assume the pin is currently in a random state that can have those things set/not set arbitrarily. I don't see how that can be handled without using arguments the way I proposed. I can't put bias-disable in DT since it would potentially disable both and the pull-up enable would have only a transient effect. I can't do the sequencing in the driver either. If I see pull-up enable, I can't imply an effect on tri-state since I can't know whether there was/will be a tri-state property that sets it as well. Sören
On Fri, Oct 31, 2014 at 5:57 PM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: > On Fri, 2014-10-31 at 09:17AM +0100, Linus Walleij wrote: >> Again it seems to be a sequencing problem. And device tree is >> not good at sequences, therefore all states should be self-contained. > > I agree, but how would I define a pin with pull-up enabled and > tri-state disabled - assume the pin is currently in a random state that > can have those things set/not set arbitrarily. I was more thinking as everything you don't enable explicitly in a state is per definition disabled. So if you are in state A and tri-state is enabled there and you move to state B where pull-up is enabled, then tri-state should be disabled, since it is not explicitly enabled. > I can't put bias-disable in DT since it would potentially disable both > and the pull-up enable would have only a transient effect. Well look at the callback from the core: int (*pin_config_set) (struct pinctrl_dev *pctldev, unsigned pin, unsigned long *configs, unsigned num_configs); You get all configs in an array. The driver can walk over the list and make informed decisions on what to do *BEFORE* poking any registers. Avoiding transients as you describe is part of why the callback looks as it does. This is why every driver has its own for-loop. > I can't do the sequencing in the driver either. If I see pull-up enable, > I can't imply an effect on tri-state since I can't know whether there > was/will be a tri-state property that sets it as well. If you define that each state is self-contained you can. Yours, Linus Walleij
On Fri, 2014-10-31 at 06:36PM +0100, Linus Walleij wrote: > On Fri, Oct 31, 2014 at 5:57 PM, Sören Brinkmann > <soren.brinkmann@xilinx.com> wrote: > > On Fri, 2014-10-31 at 09:17AM +0100, Linus Walleij wrote: > > >> Again it seems to be a sequencing problem. And device tree is > >> not good at sequences, therefore all states should be self-contained. > > > > I agree, but how would I define a pin with pull-up enabled and > > tri-state disabled - assume the pin is currently in a random state that > > can have those things set/not set arbitrarily. > > I was more thinking as everything you don't enable explicitly > in a state is per definition disabled. > > So if you are in state A and tri-state is enabled there and you > move to state B where pull-up is enabled, then tri-state should > be disabled, since it is not explicitly enabled. > > > I can't put bias-disable in DT since it would potentially disable both > > and the pull-up enable would have only a transient effect. > > Well look at the callback from the core: > > int (*pin_config_set) (struct pinctrl_dev *pctldev, > unsigned pin, > unsigned long *configs, > unsigned num_configs); > > You get all configs in an array. The driver can walk over the list and > make informed decisions on what to do *BEFORE* poking any registers. > > Avoiding transients as you describe is part of why the callback > looks as it does. This is why every driver has its own for-loop. Okay, I guess that is possible. I find usage of the arguments more elegant since it is more explicit and reduces code in the driver, but I suspect it should work. Thanks, Sören
On Fri, 2014-10-31 at 10:40AM -0700, Sören Brinkmann wrote: > On Fri, 2014-10-31 at 06:36PM +0100, Linus Walleij wrote: > > On Fri, Oct 31, 2014 at 5:57 PM, Sören Brinkmann > > <soren.brinkmann@xilinx.com> wrote: > > > On Fri, 2014-10-31 at 09:17AM +0100, Linus Walleij wrote: > > > > >> Again it seems to be a sequencing problem. And device tree is > > >> not good at sequences, therefore all states should be self-contained. > > > > > > I agree, but how would I define a pin with pull-up enabled and > > > tri-state disabled - assume the pin is currently in a random state that > > > can have those things set/not set arbitrarily. > > > > I was more thinking as everything you don't enable explicitly > > in a state is per definition disabled. > > > > So if you are in state A and tri-state is enabled there and you > > move to state B where pull-up is enabled, then tri-state should > > be disabled, since it is not explicitly enabled. > > > > > I can't put bias-disable in DT since it would potentially disable both > > > and the pull-up enable would have only a transient effect. > > > > Well look at the callback from the core: > > > > int (*pin_config_set) (struct pinctrl_dev *pctldev, > > unsigned pin, > > unsigned long *configs, > > unsigned num_configs); > > > > You get all configs in an array. The driver can walk over the list and > > make informed decisions on what to do *BEFORE* poking any registers. > > > > Avoiding transients as you describe is part of why the callback > > looks as it does. This is why every driver has its own for-loop. > > Okay, I guess that is possible. I find usage of the arguments more > elegant since it is more explicit and reduces code in the driver, but I > suspect it should work. It does work with some limitation though. This was how I originally described a state in DT: pinctrl_uart1_default: pinctrl-uart1-default { common { groups = "uart1_10_grp"; function = "uart1"; bias-pull-up = <0>; slew-rate = <0>; io-standard = <1>; }; rx { pins = "MIO49"; bias-high-impedance = <1>; }; tx { pins = "MIO48"; bias-high-impedance = <0>; }; }; Now, I removed the arguments for tri-state and pull-up. The problem, this state is handled per-sub-node. I.e. one call to the cfg-set callback per node. I.e. I cannot split things in common, rx and tx, but I need to duplicate the pinconf props in rx and tx, resulting in: pinctrl_uart1_default: pinctrl-uart1-default { common { groups = "uart1_10_grp"; function = "uart1"; }; rx { pins = "MIO49"; slew-rate = <0>; io-standard = <1>; bias-high-impedance; }; tx { pins = "MIO48"; slew-rate = <0>; io-standard = <1>; }; }; In a nutshell, yes, it's possible to work without the arguments for pull-up or tri-state. But it adds complexity in the driver and the DT description. Sören
On Sun, Nov 2, 2014 at 9:20 PM, Sören Brinkmann <soren.brinkmann@xilinx.com> wrote: (...) > common { > groups = "uart1_10_grp"; > function = "uart1"; > bias-pull-up = <0>; > slew-rate = <0>; > io-standard = <1>; > }; > > rx { > pins = "MIO49"; > bias-high-impedance = <1>; > }; > > tx { > pins = "MIO48"; > bias-high-impedance = <0>; > }; (...) > common { > groups = "uart1_10_grp"; > function = "uart1"; > }; > > rx { > pins = "MIO49"; > slew-rate = <0>; > io-standard = <1>; > bias-high-impedance; > }; > > tx { > pins = "MIO48"; > slew-rate = <0>; > io-standard = <1>; > }; > }; > > In a nutshell, yes, it's possible to work without the arguments for > pull-up or tri-state. But it adds complexity in the driver and the DT > description. But isn't it obvious that the latter is easier for a human to read and understand (and not misunderstand) and remember readability of config is one of the design goals of DT... Yours, Linus Walleij
diff --git a/arch/arm/boot/dts/zynq-7000.dtsi b/arch/arm/boot/dts/zynq-7000.dtsi index 24036c440440..37d7fe36a129 100644 --- a/arch/arm/boot/dts/zynq-7000.dtsi +++ b/arch/arm/boot/dts/zynq-7000.dtsi @@ -238,7 +238,7 @@ slcr: slcr@f8000000 { #address-cells = <1>; #size-cells = <1>; - compatible = "xlnx,zynq-slcr", "syscon"; + compatible = "xlnx,zynq-slcr", "syscon", "simple-bus"; reg = <0xF8000000 0x1000>; ranges; clkc: clkc@100 { @@ -259,6 +259,12 @@ "dbg_trc", "dbg_apb"; reg = <0x100 0x100>; }; + + pinctrl0: pinctrl@700 { + compatible = "xlnx,pinctrl-zynq"; + reg = <0x700 0x200>; + syscon = <&slcr>; + }; }; dmac_s: dmac@f8003000 { diff --git a/arch/arm/boot/dts/zynq-zc702.dts b/arch/arm/boot/dts/zynq-zc702.dts index 94e2cda6f9b6..b3ec4d26a9b3 100644 --- a/arch/arm/boot/dts/zynq-zc702.dts +++ b/arch/arm/boot/dts/zynq-zc702.dts @@ -40,21 +40,32 @@ &can0 { status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_can0_default>; }; &gem0 { status = "okay"; phy-mode = "rgmii-id"; phy-handle = <ðernet_phy>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gem0_default>; ethernet_phy: ethernet-phy@7 { reg = <7>; }; }; +&gpio0 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gpio0_default>; +}; + &i2c0 { status = "okay"; clock-frequency = <400000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c0_default>; i2cswitch@74 { compatible = "nxp,pca9548"; @@ -128,10 +139,146 @@ }; }; +&pinctrl0 { + pinctrl_can0_default: pinctrl-can0-default { + common { + function = "can0"; + groups = "can0_9_grp"; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + rx { + pins = "MIO46"; + bias-high-impedance = <1>; + }; + + tx { + pins = "MIO47"; + bias-high-impedance = <0>; + }; + }; + + pinctrl_gem0_default: pinctrl-gem0-default { + common { + function = "ethernet0"; + groups = "ethernet0_0_grp"; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <4>; + }; + + rx { + pins = "MIO22", "MIO23", "MIO24", "MIO25", "MIO26", "MIO27"; + bias-high-impedance = <1>; + low-power-disable; + }; + + tx { + pins = "MIO16", "MIO17", "MIO18", "MIO19", "MIO20", "MIO21"; + bias-high-impedance = <0>; + low-power-enable; + }; + + mdio { + function = "mdio0"; + groups = "mdio0_0_grp"; + }; + }; + + pinctrl_gpio0_default: pinctrl-gpio0-default { + common { + function = "gpio0"; + groups = "gpio0_7_grp", "gpio0_8_grp", "gpio0_9_grp", + "gpio0_10_grp", "gpio0_11_grp", "gpio0_12_grp", + "gpio0_13_grp", "gpio0_14_grp"; + bias-high-impedance = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + pull-up { + pins = "MIO9", "MIO10", "MIO11", "MIO12", "MIO13", "MIO14"; + bias-pull-up = <1>; + }; + + pull-none { + pins = "MIO7", "MIO8"; + bias-pull-up = <0>; + }; + + }; + + pinctrl_i2c0_default: pinctrl-i2c0-default { + common { + groups = "i2c0_10_grp"; + function = "i2c0"; + bias-pull-up = <1>; + bias-high-impedance = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + }; + + pinctrl_sdhci0_default: pinctrl-sdhci0-default { + common { + groups = "sdio0_2_grp"; + function = "sdio0"; + bias-high-impedance = <0>; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + cd { + groups = "gpio0_0_grp"; + function = "sdio0_cd"; + bias-high-impedance = <1>; + bias-pull-up = <1>; + slew-rate = <0>; + io-standard = <1>; + }; + + wp { + groups = "gpio0_15_grp"; + function = "sdio0_wp"; + bias-high-impedance = <1>; + bias-pull-up = <1>; + slew-rate = <0>; + io-standard = <1>; + }; + }; + + pinctrl_uart1_default: pinctrl-uart1-default { + common { + groups = "uart1_10_grp"; + function = "uart1"; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + rx { + pins = "MIO49"; + bias-high-impedance = <1>; + }; + + tx { + pins = "MIO48"; + bias-high-impedance = <0>; + }; + }; +}; + &sdhci0 { status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_sdhci0_default>; }; &uart1 { status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart1_default>; }; diff --git a/arch/arm/boot/dts/zynq-zc706.dts b/arch/arm/boot/dts/zynq-zc706.dts index a8bbdfbc7093..42a335431613 100644 --- a/arch/arm/boot/dts/zynq-zc706.dts +++ b/arch/arm/boot/dts/zynq-zc706.dts @@ -33,15 +33,24 @@ status = "okay"; phy-mode = "rgmii-id"; phy-handle = <ðernet_phy>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gem0_default>; ethernet_phy: ethernet-phy@7 { reg = <7>; }; }; +&gpio0 { + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_gpio0_default>; +}; + &i2c0 { status = "okay"; clock-frequency = <400000>; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_i2c0_default>; i2cswitch@74 { compatible = "nxp,pca9548"; @@ -107,10 +116,127 @@ }; }; +&pinctrl0 { + pinctrl_gem0_default: pinctrl-gem0-default { + common { + function = "ethernet0"; + groups = "ethernet0_0_grp"; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <4>; + }; + + rx { + pins = "MIO22", "MIO23", "MIO24", "MIO25", "MIO26", "MIO27"; + bias-high-impedance = <1>; + low-power-disable; + }; + + tx { + pins = "MIO16", "MIO17", "MIO18", "MIO19", "MIO20", "MIO21"; + bias-high-impedance = <0>; + low-power-enable; + }; + + mdio { + function = "mdio0"; + groups = "mdio0_0_grp"; + bias-high-impedance = <0>; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + }; + + pinctrl_gpio0_default: pinctrl-gpio0-default { + common { + function = "gpio0"; + groups = "gpio0_7_grp", "gpio0_46_grp", "gpio0_47_grp"; + bias-high-impedance = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + pull-up { + pins = "MIO46", "MIO47"; + bias-pull-up = <1>; + }; + + pull-none { + pins = "MIO7"; + bias-pull-up = <0>; + }; + }; + + pinctrl_i2c0_default: pinctrl-i2c0-default { + common { + groups = "i2c0_10_grp"; + function = "i2c0"; + bias-high-impedance = <0>; + bias-pull-up = <1>; + slew-rate = <0>; + io-standard = <1>; + }; + }; + + pinctrl_sdhci0_default: pinctrl-sdhci0-default { + common { + groups = "sdio0_2_grp"; + function = "sdio0"; + bias-high-impedance = <0>; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + cd { + groups = "gpio0_14_grp"; + function = "sdio0_cd"; + bias-high-impedance = <1>; + bias-pull-up = <1>; + slew-rate = <0>; + io-standard = <1>; + }; + + wp { + groups = "gpio0_15_grp"; + function = "sdio0_wp"; + bias-high-impedance = <1>; + bias-pull-up = <1>; + slew-rate = <0>; + io-standard = <1>; + }; + }; + + pinctrl_uart1_default: pinctrl-uart1-default { + common { + groups = "uart1_10_grp"; + function = "uart1"; + bias-pull-up = <0>; + slew-rate = <0>; + io-standard = <1>; + }; + + rx { + pins = "MIO49"; + bias-high-impedance = <1>; + }; + + tx { + pins = "MIO48"; + bias-high-impedance = <0>; + }; + }; +}; + &sdhci0 { status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_sdhci0_default>; }; &uart1 { status = "okay"; + pinctrl-names = "default"; + pinctrl-0 = <&pinctrl_uart1_default>; };
Add pinctrl descriptions to the zc702 and zc706 device trees. Signed-off-by: Soren Brinkmann <soren.brinkmann@xilinx.com> --- arch/arm/boot/dts/zynq-7000.dtsi | 8 ++- arch/arm/boot/dts/zynq-zc702.dts | 147 +++++++++++++++++++++++++++++++++++++++ arch/arm/boot/dts/zynq-zc706.dts | 126 +++++++++++++++++++++++++++++++++ 3 files changed, 280 insertions(+), 1 deletion(-)