| Message ID | 1343458722-17127-7-git-send-email-haojian.zhuang@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Sat, Jul 28, 2012 at 8:58 AM, Haojian Zhuang
<haojian.zhuang@gmail.com> wrote:
> Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com>
I have no clue as to whether these are good DT bindings or not, so
leaving this up to the PXA maintainers :-)
You don't need my ACK for this part anyway.
Yours,
Linus Walleij
On Saturday 28 July 2012, Haojian Zhuang wrote: > +Required subnode-properties: > +- marvell,pins : An array of strings. Each string contains the name of a pin > + or group. > +- marvell,function : A string containing the name of the function to mux to the > + pin or group. > +- marvell,drive-strength : An integer value that means the drive strength of a > + pin. > +- marvell,pull-up : The property means a pin is pull up for input. > +- marvell,pull-down : The property means a pin is pull down for input. > +- marvell,lowpower-pull-up : The property means a pin is pull up for input in > + sleep state. > +- marvell,lowpower-pull-down : The property means a pin is pull down for input > + in sleep state. > +- marvell,lowpower-drive-high : The property means a pin is driving high for > + output in sleep state. > +- marvell,lowpower-drive-low : The property means a pin is driving low for > + output in sleep state. > +- marvell,lowpower-float : The property means a pin is float in sleep state. > +- marvell,lowpower-zero : The property means a pin is not configured in sleep > + state. These (or most of them) look like they are not very Marvell specific. Should we try to standardize the way they are set across different bindings? On a side-node, you probably don't want to make all of the above properties "required" because some of them are mutually exclusive. Arnd
On 06.08.2012 10:44, Arnd Bergmann wrote: > On Saturday 28 July 2012, Haojian Zhuang wrote: >> +Required subnode-properties: >> +- marvell,pins : An array of strings. Each string contains the name of a pin >> + or group. >> +- marvell,function : A string containing the name of the function to mux to the >> + pin or group. >> +- marvell,drive-strength : An integer value that means the drive strength of a >> + pin. >> +- marvell,pull-up : The property means a pin is pull up for input. >> +- marvell,pull-down : The property means a pin is pull down for input. >> +- marvell,lowpower-pull-up : The property means a pin is pull up for input in >> + sleep state. >> +- marvell,lowpower-pull-down : The property means a pin is pull down for input >> + in sleep state. >> +- marvell,lowpower-drive-high : The property means a pin is driving high for >> + output in sleep state. >> +- marvell,lowpower-drive-low : The property means a pin is driving low for >> + output in sleep state. >> +- marvell,lowpower-float : The property means a pin is float in sleep state. >> +- marvell,lowpower-zero : The property means a pin is not configured in sleep >> + state. > > These (or most of them) look like they are not very Marvell specific. Should > we try to standardize the way they are set across different bindings? Yes please. That way we also have a higher chance that implementations on other platforms will be similar. Daniel
On Mon, Aug 6, 2012 at 4:44 PM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 28 July 2012, Haojian Zhuang wrote: >> +Required subnode-properties: >> +- marvell,pins : An array of strings. Each string contains the name of a pin >> + or group. >> +- marvell,function : A string containing the name of the function to mux to the >> + pin or group. >> +- marvell,drive-strength : An integer value that means the drive strength of a >> + pin. >> +- marvell,pull-up : The property means a pin is pull up for input. >> +- marvell,pull-down : The property means a pin is pull down for input. >> +- marvell,lowpower-pull-up : The property means a pin is pull up for input in >> + sleep state. >> +- marvell,lowpower-pull-down : The property means a pin is pull down for input >> + in sleep state. >> +- marvell,lowpower-drive-high : The property means a pin is driving high for >> + output in sleep state. >> +- marvell,lowpower-drive-low : The property means a pin is driving low for >> + output in sleep state. >> +- marvell,lowpower-float : The property means a pin is float in sleep state. >> +- marvell,lowpower-zero : The property means a pin is not configured in sleep >> + state. > > These (or most of them) look like they are not very Marvell specific. Should > we try to standardize the way they are set across different bindings? > > On a side-node, you probably don't want to make all of the above properties > "required" because some of them are mutually exclusive. > OK, I'll handle it.
On Mon, Aug 6, 2012 at 10:44 AM, Arnd Bergmann <arnd@arndb.de> wrote: > On Saturday 28 July 2012, Haojian Zhuang wrote: >> +Required subnode-properties: >> +- marvell,pins : An array of strings. Each string contains the name of a pin >> + or group. >> +- marvell,function : A string containing the name of the function to mux to the >> + pin or group. >> +- marvell,drive-strength : An integer value that means the drive strength of a >> + pin. >> +- marvell,pull-up : The property means a pin is pull up for input. >> +- marvell,pull-down : The property means a pin is pull down for input. >> +- marvell,lowpower-pull-up : The property means a pin is pull up for input in >> + sleep state. >> +- marvell,lowpower-pull-down : The property means a pin is pull down for input >> + in sleep state. >> +- marvell,lowpower-drive-high : The property means a pin is driving high for >> + output in sleep state. >> +- marvell,lowpower-drive-low : The property means a pin is driving low for >> + output in sleep state. >> +- marvell,lowpower-float : The property means a pin is float in sleep state. >> +- marvell,lowpower-zero : The property means a pin is not configured in sleep >> + state. > > These (or most of them) look like they are not very Marvell specific. Should > we try to standardize the way they are set across different bindings? If the device tree properties are to be standardized, the driver should be moved over to using generic pin config (drivers/pinctrl/pinconf-generic.c, include/linux/pinctrl/pinconf-generic.h) too, or it makes no sense. So please augment the PXA driver to use these, then move the generic bindings to pinconf-generic.txt or something. If pinconf-generic needs extending/augmenting in the process, just do it, it's supposed to be generic... Yours, Linus Walleij
On 07.08.2012 08:49, Linus Walleij wrote: > On Mon, Aug 6, 2012 at 10:44 AM, Arnd Bergmann <arnd@arndb.de> wrote: >> On Saturday 28 July 2012, Haojian Zhuang wrote: >>> +Required subnode-properties: >>> +- marvell,pins : An array of strings. Each string contains the name of a pin >>> + or group. >>> +- marvell,function : A string containing the name of the function to mux to the >>> + pin or group. >>> +- marvell,drive-strength : An integer value that means the drive strength of a >>> + pin. >>> +- marvell,pull-up : The property means a pin is pull up for input. >>> +- marvell,pull-down : The property means a pin is pull down for input. >>> +- marvell,lowpower-pull-up : The property means a pin is pull up for input in >>> + sleep state. >>> +- marvell,lowpower-pull-down : The property means a pin is pull down for input >>> + in sleep state. >>> +- marvell,lowpower-drive-high : The property means a pin is driving high for >>> + output in sleep state. >>> +- marvell,lowpower-drive-low : The property means a pin is driving low for >>> + output in sleep state. >>> +- marvell,lowpower-float : The property means a pin is float in sleep state. >>> +- marvell,lowpower-zero : The property means a pin is not configured in sleep >>> + state. >> >> These (or most of them) look like they are not very Marvell specific. Should >> we try to standardize the way they are set across different bindings? > > If the device tree properties are to be standardized, the driver should be > moved over to using generic pin config (drivers/pinctrl/pinconf-generic.c, > include/linux/pinctrl/pinconf-generic.h) too, or it makes no sense. > > So please augment the PXA driver to use these, then move the generic > bindings to pinconf-generic.txt or something. > > If pinconf-generic needs extending/augmenting in the process, just do it, > it's supposed to be generic... I am looking at the generic pinctrl code right now and I wonder if the following approach would be ok: - each driver that calls pinctrl_register() has its own dt_match_table entry, so the 'compatible' part of the bindings is specific to a pin controller chip. - introduce a pinctrl_parse_of(struct of_node *) and call that after the driver has added all its pins. That function walks all the children of the given node and uses the callbacks provided by the driver to do the actual work. - Unfortunately, we can't parse the node from pinctrl_register() as the gpio ranges are only added after that by the drivers. If that sounds good, I can come up with a patch. Thanks, Daniel
On Thu, Aug 9, 2012 at 4:28 PM, Daniel Mack <zonque@gmail.com> wrote: > I am looking at the generic pinctrl code right now Sweet! > and I wonder if the following approach would be ok: > > - each driver that calls pinctrl_register() has its own dt_match_table > entry, so the 'compatible' part of the bindings is specific to a pin > controller chip. Yep. > - introduce a pinctrl_parse_of(struct of_node *) and call that after the > driver has added all its pins. That function walks all the children of > the given node and uses the callbacks provided by the driver to do the > actual work. Isn't that already how it works? Maybe I misunderstand, do you mean that you want to pass the DT node to pinctrl_register()? Are you talking about using pinctrl hogs to set up the default configuration and muxing? That can already be done today, and has nothing to do with whether you use generic pin config or not. This uses the .dt_node_to_map() and .dt_free_map() callbacks in the pinctrl_ops vtable to generate a map from the device tree, in any way you want. So the infrastructure I think already exists, but whereas all current driver have their own config and mux (etc) parsing code, pinconf-generic could provide something that would be common for all driver using generic pin config. > - Unfortunately, we can't parse the node from pinctrl_register() as the > gpio ranges are only added after that by the drivers. The ranges are a *big* problem and we haven't come up with a proper DT binding for these. After discussion with Grant, he has proposed that GPIO chips should register their ranges using their local numberspace and always passing the struct gpio_chip, instead of pin controllers registering ranges. Documented here: https://blueprints.launchpad.net/linux-linaro/+spec/pinctrl-gpiorange-makeover > If that sounds good, I can come up with a patch. I think you already have what you need, look closer at how e.g. the Tegra or i.MX driver does this. Or maybe I'm only getting it backwards? Yours, Linus Walleij
On 10.08.2012 12:56, Linus Walleij wrote: > On Thu, Aug 9, 2012 at 4:28 PM, Daniel Mack <zonque@gmail.com> wrote: > >> I am looking at the generic pinctrl code right now > > Sweet! > >> and I wonder if the following approach would be ok: >> >> - each driver that calls pinctrl_register() has its own dt_match_table >> entry, so the 'compatible' part of the bindings is specific to a pin >> controller chip. > > Yep. > >> - introduce a pinctrl_parse_of(struct of_node *) and call that after the >> driver has added all its pins. That function walks all the children of >> the given node and uses the callbacks provided by the driver to do the >> actual work. > > Isn't that already how it works? Maybe I misunderstand, do you mean > that you want to pass the DT node to pinctrl_register()? To parse the generic bits, yes. But I realized only now that we already have dt_to_map_one_config() > Are you talking about using pinctrl hogs to set up the default > configuration and muxing? That can already be done today, > and has nothing to do with whether you use generic pin config > or not. > > This uses the .dt_node_to_map() and .dt_free_map() callbacks > in the pinctrl_ops vtable to generate a map from the device tree, > in any way you want. Yes, I see. And this is where duplication emerges, or, where consolidation would need to be done. If we want to standardize the way pins are set up in terms of bias, pullup/pulldown and all that, we would just need to map generic DT entries to PIN_CONFIG_* and call the pin controllers specific functions. > So the infrastructure I think already exists, but whereas all > current driver have their own config and mux (etc) parsing > code, pinconf-generic could provide something that would > be common for all driver using generic pin config. Exactly. And then the question is whether we want implicit behaviour. IOW - let dt_to_map_one_config() parse the generic bits and then call the driver specific augmentation code, if any. Daniel
On Mon, Aug 13, 2012 at 11:43 AM, Daniel Mack <zonque@gmail.com> wrote: > On 10.08.2012 12:56, Linus Walleij wrote: >> So the infrastructure I think already exists, but whereas all >> current driver have their own config and mux (etc) parsing >> code, pinconf-generic could provide something that would >> be common for all driver using generic pin config. > > Exactly. And then the question is whether we want implicit behaviour. > IOW - let dt_to_map_one_config() parse the generic bits and then call > the driver specific augmentation code, if any. Well if you make a patch I think we can see pretty clear from the code what the best way of doing this will be. Yours, Linus Walleij
diff --git a/Documentation/devicetree/bindings/pinctrl/mrvl,pxa3xx-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/mrvl,pxa3xx-pinctrl.txt new file mode 100644 index 0000000..1f516e4 --- /dev/null +++ b/Documentation/devicetree/bindings/pinctrl/mrvl,pxa3xx-pinctrl.txt @@ -0,0 +1,35 @@ +Marvell Technology Group, PXA3XX pinmux controller + +Required properties: +- compatible : "marvell,pxa910-pinmux" + : "marvell,pxa168-pinmux" + : "marvell,mmp2-pinmux" +- reg : Address range of the pinctrl registers + +PXA3xx's pinmux nodes act as a container for an abitrary number of subnodes. +Each of these subnodes represents muxing for a pin, a group, or a list of +pins or groups. + +The name of each subnode is not important; all subnodes should be enumerated +and processed purely based on their content. + +Required subnode-properties: +- marvell,pins : An array of strings. Each string contains the name of a pin + or group. +- marvell,function : A string containing the name of the function to mux to the + pin or group. +- marvell,drive-strength : An integer value that means the drive strength of a + pin. +- marvell,pull-up : The property means a pin is pull up for input. +- marvell,pull-down : The property means a pin is pull down for input. +- marvell,lowpower-pull-up : The property means a pin is pull up for input in + sleep state. +- marvell,lowpower-pull-down : The property means a pin is pull down for input + in sleep state. +- marvell,lowpower-drive-high : The property means a pin is driving high for + output in sleep state. +- marvell,lowpower-drive-low : The property means a pin is driving low for + output in sleep state. +- marvell,lowpower-float : The property means a pin is float in sleep state. +- marvell,lowpower-zero : The property means a pin is not configured in sleep + state. diff --git a/arch/arm/boot/dts/mmp2-brownstone.dts b/arch/arm/boot/dts/mmp2-brownstone.dts index c9b4f27..2ae37df 100644 --- a/arch/arm/boot/dts/mmp2-brownstone.dts +++ b/arch/arm/boot/dts/mmp2-brownstone.dts @@ -24,6 +24,57 @@ soc { apb@d4000000 { + pinmux@d401e000 { + pinctrl-names = "default"; + pinctrl-0 = <&state_default>; + + state_default: pinmux { + uart1 { + marvell,pins = "uart1 2p4"; + marvell,function = "uart1"; + }; + uart2 { + marvell,pins = "uart2 2p7"; + marvell,function = "uart2"; + }; + uart3 { + marvell,pins = "uart3 2p6"; + marvell,function = "uart3"; + }; + twsi2 { + marvell,pins = "twsi2-3"; + marvell,function = "twsi2"; + }; + twsi3 { + marvell,pins = "twsi3-1"; + marvell,function = "twsi3"; + }; + twsi4 { + marvell,pins = "twsi4"; + marvell,function = "twsi4"; + }; + twsi5 { + marvell,pins = "twsi5-3"; + marvell,function = "twsi5"; + }; + twsi6 { + marvell,pins = "twsi6-3"; + marvell,function = "twsi6"; + }; + mmc1 { + marvell,pins = "mmc1 8p1"; + marvell,function = "mmc1"; + }; + mmc2 { + marvell,pins = "mmc2 6p1"; + marvell,function = "mmc2"; + }; + mmc3 { + marvell,pins = "mmc3 10p1"; + marvell,function = "mmc3"; + }; + }; + }; uart3: uart@d4018000 { status = "okay"; }; diff --git a/arch/arm/boot/dts/mmp2.dtsi b/arch/arm/boot/dts/mmp2.dtsi index 80f74e2..cb4ecaf 100644 --- a/arch/arm/boot/dts/mmp2.dtsi +++ b/arch/arm/boot/dts/mmp2.dtsi @@ -120,6 +120,11 @@ reg = <0xd4000000 0x00200000>; ranges; + pinmux@d401e000 { + compatible = "marvell,mmp2-pinmux"; + reg = <0xd401e000 0x4000>; + }; + timer0: timer@d4014000 { compatible = "mrvl,mmp-timer"; reg = <0xd4014000 0x100>; diff --git a/arch/arm/boot/dts/pxa168.dtsi b/arch/arm/boot/dts/pxa168.dtsi index 31a7186..f15491d 100644 --- a/arch/arm/boot/dts/pxa168.dtsi +++ b/arch/arm/boot/dts/pxa168.dtsi @@ -49,6 +49,11 @@ reg = <0xd4000000 0x00200000>; ranges; + pinmux@d401e000 { + compatible = "marvell,pxa168-pinmux"; + reg = <0xd401e000 0x4000>; + }; + timer0: timer@d4014000 { compatible = "mrvl,mmp-timer"; reg = <0xd4014000 0x100>; diff --git a/arch/arm/boot/dts/pxa910-dkb.dts b/arch/arm/boot/dts/pxa910-dkb.dts index e92be5a..d3ba948 100644 --- a/arch/arm/boot/dts/pxa910-dkb.dts +++ b/arch/arm/boot/dts/pxa910-dkb.dts @@ -24,6 +24,53 @@ soc { apb@d4000000 { + pinmux@d401e000 { + pinctrl-names = "default"; + pinctrl-0 = <&state_default>; + + state_default: pinmux { + uart0 { /* BT UART */ + marvell,pins = "uart0 4p"; + marvell,function = "uart0"; + }; + uart1 { /* Serial UART */ + marvell,pins = "uart1 2p1"; + marvell,function = "uart1"; + }; + uart2 { /* GPS UART */ + marvell,pins = "uart2 2p1"; + marvell,function = "uart2"; + }; + nand { + marvell,pins = "nand"; + marvell,function = "nand"; + }; + twsi1 { + marvell,pins = "twsi 2p2"; + marvell,function = "twsi"; + }; + mmc1 { + marvell,pins = "mmc1 12p"; + marvell,function = "mmc1"; + }; + mmc2 { + marvell,pins = "mmc2 6p"; + marvell,function = "mmc2"; + }; + w1 { + marvell,pins = "w1-4"; + marvell,function = "w1"; + }; + ssp1 { + marvell,pins = "ssp1 4p1"; + marvell,function = "ssp1"; + }; + gssp { + marvell,pins = "gssp"; + marvell,function = "gssp"; + }; + }; + }; uart1: uart@d4017000 { status = "okay"; }; diff --git a/arch/arm/boot/dts/pxa910.dtsi b/arch/arm/boot/dts/pxa910.dtsi index aebf32d..187041d 100644 --- a/arch/arm/boot/dts/pxa910.dtsi +++ b/arch/arm/boot/dts/pxa910.dtsi @@ -49,6 +49,11 @@ reg = <0xd4000000 0x00200000>; ranges; + pinmux@d401e000 { + compatible = "marvell,pxa910-pinmux"; + reg = <0xd401e000 0x4000>; + }; + timer0: timer@d4014000 { compatible = "mrvl,mmp-timer"; reg = <0xd4014000 0x100>;
Signed-off-by: Haojian Zhuang <haojian.zhuang@gmail.com> --- .../bindings/pinctrl/mrvl,pxa3xx-pinctrl.txt | 35 ++++++++++++++ arch/arm/boot/dts/mmp2-brownstone.dts | 51 ++++++++++++++++++++ arch/arm/boot/dts/mmp2.dtsi | 5 ++ arch/arm/boot/dts/pxa168.dtsi | 5 ++ arch/arm/boot/dts/pxa910-dkb.dts | 47 ++++++++++++++++++ arch/arm/boot/dts/pxa910.dtsi | 5 ++ 6 files changed, 148 insertions(+) create mode 100644 Documentation/devicetree/bindings/pinctrl/mrvl,pxa3xx-pinctrl.txt