Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Sun, Nov 2, 2014 at 9:20 PM, Sören Brinkmann
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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
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
> > 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Fri, 2014-10-31 at 06:36PM +0100, Linus Walleij wrote: > On Fri, Oct 31, 2014 at 5:57 PM, Sören Brinkmann > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Fri, Oct 31, 2014 at 5:57 PM, Sören Brinkmann 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Fri, 2014-10-31 at 09:17AM +0100, Linus Walleij wrote:
> On Tue, Oct 28, 2014 at 5:03 PM, Sören Brinkmann
> wrote:
> > On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote:
> >> On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann
> >> wrote:
> >>
> >> > Add pinctrl descriptions to the zc702 and zc706 device trees.
> >> >
> >> > Signed-off-by: Soren Brinkmann
> >>
> >> (...)
> >> > +&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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Tue, Oct 28, 2014 at 5:03 PM, Sören Brinkmann
wrote:
> On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote:
>> On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann
>> wrote:
>>
>> > Add pinctrl descriptions to the zc702 and zc706 device trees.
>> >
>> > Signed-off-by: Soren Brinkmann
>>
>> (...)
>> > +&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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Tue, 2014-10-28 at 04:05PM +0100, Linus Walleij wrote:
> On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann
> wrote:
>
> > Add pinctrl descriptions to the zc702 and zc706 device trees.
> >
> > Signed-off-by: Soren Brinkmann
>
> (...)
> > +&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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
On Thu, Oct 16, 2014 at 7:11 PM, Soren Brinkmann
wrote:
> Add pinctrl descriptions to the zc702 and zc706 device trees.
>
> Signed-off-by: Soren Brinkmann
(...)
> +&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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
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
> ---
> 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@f800 {
> #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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH RFC v2 8/8] ARM: zynq: DT: Add pinctrl information
Add pinctrl descriptions to the zc702 and zc706 device trees.
Signed-off-by: Soren Brinkmann
---
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@f800 {
#address-cells = <1>;
#size-cells = <1>;
- compatible = "xlnx,zynq-slcr", "syscon";
+ compatible = "xlnx,zynq-slcr", "syscon", "simple-bus";
reg = <0xF800 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 = <40>;
+ 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>;
+ bi
