More pinmux feedback, and GPIO vs. pinmux interaction
Linus (W), Some more thoughts on pinmux: == GPIO/pinmux API interactions I'd like to understand how the gpio and pinmux subsystems are intended to interact. Quoting from Documentation/gpio.txt: Note that requesting a GPIO does NOT cause it to be configured in any way; it just marks that GPIO as in use. Separate code must handle any pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown). Due to this, the current Tegra GPIO driver contains additional non- standard functions: void tegra_gpio_enable(int gpio); void tegra_gpio_disable(int gpio); In some cases, those functions are called by board files at boot time, and in other cases by drivers themselves, right before gpio_request(). Is it your intention that such custom functions be replaced by pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable op should call tegra_gpio_enable? That seems like the right idea to me. == pinmux calling GPIO or vice-versa? Having pinmux call gpio appears to conflict with a couple of things from your recent pinmux patchset: > diff --git a/drivers/Kconfig b/drivers/Kconfig > +# pinctrl before gpio - gpio drivers may need it > + > +source "drivers/pinctrl/Kconfig" > + > source "drivers/gpio/Kconfig" > > diff --git a/drivers/Makefile b/drivers/Makefile >... > +# GPIO must come after pinctrl as gpios may need to mux pins etc > +obj-y+= pinctrl/ Don't those patches imply that the GPIO controller code is calling into the pinmux code to perform muxing, not the other way around? == When pins get marked as in-used There seems to be a discrepancy in the way the client APIs work: For special functions, the client calls pinmux_get, then later pinmux_enable, whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not sure of the benefit of having separate pinmux_get and pinmux_enable calls; I thought the idea was that a client could: a = pinmux_get(dev, "funca"); b = pinmux_get(dev, "funcb"); pinmux_enable(a); ... pinmux_disable(a); pinmux_enable(b); ... pinmux_disable(b); pinmux_put(b); pinmux_put(a); However, since the pins are marked as reserved/in-use when pinmux_get is called rather than pinmux_enable, the code above won't work; it'd need to be: a = pinmux_get(dev, "funca"); pinmux_enable(a); ... pinmux_disable(a); pinmux_put(a); b = pinmux_get(dev, "funcb"); pinmux_enable(b); ... pinmux_disable(b); pinmux_put(b); Perhaps pin usage marking should be moved into enable/disable? == Matched request/free calls for GPIOs A couple more comments on your recent pinmux patches in light of this: >From pin_request(): + /* +* If there is no kind of request function for the pin we just assume +* we got it by default and proceed. +*/ + if (gpio && ops->gpio_request_enable) + /* This requests and enables a single GPIO pin */ + status = ops->gpio_request_enable(pmxdev, + pin - pmxdev->desc->base); + else if (ops->request) + status = ops->request(pmxdev, + pin - pmxdev->desc->base); + else + status = 0; a) I feel the default should be to error out, not succeed; the whole point of the API is for HW where something must be programmed to enable each function. As such, if you don't provide that ops->request* function, that seems like an error. I think the logic above should be to always call ops->gpio_request_enable if (gpio): if (gpio) if (ops->gpio_request_enable) /* This requests and enables a single GPIO pin */ status = ops->gpio_request_enable(pmxdev, pin - pmxdev->desc->base); else status = ops->request(pmxdev, pin - pmxdev->desc->base); (ops->gpio_request_enable could be optional if GPIOs aren't supported on any pinmux pins, whereas ops->request isn't optional, and should be checked during pinmux_register) Also, ops->gpio_request_enable should also be passed the GPIO number, so the driver can validate that it's the correct one. Often, only one value would be legal, but perhaps some pinmuxs allow 1 of N different GPIOs to be mapped to some pin, so selection of which GPIO is on par with selection of which special function to mux out, yet the driver still doesn't want to expose every GPIO possibility as a pinmux "function" due to explosion of the number of functions if it did that. b) When ops->gpio_request_enable is called to request a pin, it'd be nice if the core could track this, and specifically call a new ops->gpio_disable to free it, rather than the generic ops->free. There are two cases in HW: b1) "GPIO" is just another special function in a list, e.g. SPI, I2C, GPIO. In this case, free for a GPIO needs to either do nothing (the next ca
RE: [PATCH 1/2] drivers: create a pinmux subsystem v3
Linus Walleij wrote at Thursday, June 16, 2011 6:47 AM:
> On Wed, Jun 15, 2011 at 12:11 AM, Stephen Warren wrote:
> > [Me]
> >> Can't you just send some patch or example .h file for the API
> >> you would like to see so I understand how you think about
> >> this?
> >
> > Are your patches in git somewhere? It's much easier for me to pull
> > at present than grab patches out of email; something I certainly need
> > to fix...
>
> Yep:
> git://git.kernel.org/pub/scm/linux/kernel/git/linusw/linux-stericsson.git
> pinmux-subsystem
> It's based on v3.0-rc3 as of now. I tend to rebase it though.
Great. Thanks.
> >> static unsigned int mmc0_1_pins[] = { 56, 57 };
> >> static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
> >> static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
> >>
> >> static struct foo_pmx_func myfuncs[] = {
> >> {
> >> .name = "mmc0-2bit",
> >> .pins = mmc0_1_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_1_pins),
> >> },
> >> {
> >> .name = "mmc0-4bit",
> >> .pins = mmc0_2_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_2_pins),
> >> },
> >> {
> >> .name = "mmc0-8bit",
> >> .pins = mmc0_3_pins,
> >> .num_pins = ARRAY_SIZE(mmc0_3_pins),
> >> },
> >> };
> >>
> >> Looks OK?
> >
> > I think that's exactly the setup I was looking to avoid. See the portion
> > of the thread starting from:
>
> Gah. Baudelaire once said something like the reason people think
> they agree is that they misunderstand each other all the time.
> Sorry for my lameness :-/
>
> > static unsigned int mmc0_1_pins[] = { 56, 57 };
> > static unsigned int mmc0_2_pins[] = { 58, 59 };
> > static unsigned int mmc0_3_pins[] = { 60, 61, 62, 63 };
> >
> >{
> >.name = "mmc0-1:0",
> >.pins = mmc0_1_pins,
> >.num_pins = ARRAY_SIZE(mmc0_1_pins),
> >},
> >{
> >.name = "mmc0-3:2",
> >.pins = mmc0_2_pins,
> >.num_pins = ARRAY_SIZE(mmc0_2_pins),
> >},
> >{
> >.name = "mmc0-7:4",
> >.pins = mmc0_3_pins,
> >.num_pins = ARRAY_SIZE(mmc0_3_pins),
> >},
> >
> > So a board that happens to use a 2-bit bus would define:
> >
> > static struct pinmux_map pmx_mapping[] = {
> >{
> >.functions = {"mmc0-1:0"},
> >.dev_name = "tegra-sdhci.0",
> >},
> > };
> >
> > But a board that happens to use an 8-bit bus would define:
> >
> > static struct pinmux_map pmx_mapping[] = {
> >{
> >.functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"},
> >.dev_name = "tegra-sdhci.0",
> >},
> > };
>
> NOW I *think* I get it.
>
> So we're basically talking about the function mapping API here.
Yes, basically.
In more detail, I'm talking about making the "functions" exposed to
clients of the pinmux be a different set than the "functions"
implemented by the pinmux driver. Perhaps we should call the former
"mappings" not make that clear; I see you did just below!
> So the idea is that one mapping can reference several functions.
Yes.
> So with this scheme actually both ways of doing this would be
> possible IIUC.
Yes, it's certainly possible for a board/machine/... to define
mappings that expose the bus in the chunks that the HW supports,
or aggregate the whole thing.
But, I think that's slightly missing the point. I would expect:
The pinmux driver itself to *always* expose functions as the raw chunks
that the HW supports (whether HW supports individual pins, or groups of
pins, whether multiple pins/groups end up being used as a bus or not).
For Tegra's keyboard controller, that might be the following functions
(actual names/widths probably incorrect; just for example):
row[3:0]
row[7:4]
row[9:8]
col[3:0]
col[7:4]
Recall that Tegra's keyboard controller module can be used with many
different combinations of those sets of pins. I assert the pinmux driver
itself needs to work identically, and expose the same data (functions,
pins) without any knowledge of the board it's being used
RE: More pinmux feedback, and GPIO vs. pinmux interaction
Linus Walleij wrote at Thursday, June 16, 2011 9:07 AM: > On Wed, Jun 15, 2011 at 11:48 PM, Stephen Warren wrote: > > > Some more thoughts on pinmux: > > Thanks! > > Keep it coming until you get tired of me :-) > > > == GPIO/pinmux API interactions > > > > I'd like to understand how the gpio and pinmux subsystems are intended > > to interact. > > Mainly I'd like Grant to comment on that a bit. It refers to the (long) > reply I wrote in response to his feeback on these issues with some > open questions in it. > > > Note that requesting a GPIO does NOT cause it to be configured in any > > way; it just marks that GPIO as in use. Separate code must handle any > > pin setup (e.g. controlling which pin the GPIO uses, pullup/pulldown). > > > > Due to this, the current Tegra GPIO driver contains additional non- > > standard functions: > > > > void tegra_gpio_enable(int gpio); > > void tegra_gpio_disable(int gpio); > > > > In some cases, those functions are called by board files at boot time, > > and in other cases by drivers themselves, right before gpio_request(). > > > > Is it your intention that such custom functions be replaced by > > pinmux_request_gpio(), and the Tegra pinmux driver's gpio_request_enable > > op should call tegra_gpio_enable? > > > > That seems like the right idea to me. > > Yes and no. As you saw in the last iteration I renamed the pinmux > framework "pinctrl" or "pin control". So being such a hopeless optimist > I set out to solve all pin controlling in this subsystem. The sales rap > would be something like: > > - Do you need to bias your pin to 3.3 V with an 1MOhm resistor? > - Do you want to mux your pin with differetn functions? > - Do you want to set the load capacitance of you pin to 10pF? > - Do you want to drive your pin as open collector? > - Is all or some the above software-controlled in your ASIC? > - Do you despair from the absence of relevant frameworks? > > DON'T WORRY! > The pinctrl subsystem is here to save YOU! > > However that's a bit of vaporware, since I just implemented the > pin numberspace and pin registration (that is the generic part) > and then the pinmux part. The rest (assorted pin control) is > yet to come. Just one note on the extra stuff for the future: * Tegra has say 100 muxable pins. * GPIO vs. special function is selectable per pin. * Muxing of which special function (and some other features, such as pullup/down) is only at a granularity of "group of pins". * Some other pin features (such as slew rate, drive strength) is also only at a granularity of "group of pins" **but** the groups are defined differently for these features than for muxing. I figure the general model is: * N pins, each perhaps with some per-pin controls. * M groups of pins, each allowing muxing, and possibly other controls and also an arbitrary number of: * P groups of pins, allowing various other controls > > == pinmux calling GPIO or vice-versa? > > > > Having pinmux call gpio appears to conflict with a couple of things from > > your recent pinmux patchset: > > > >> diff --git a/drivers/Kconfig b/drivers/Kconfig > >> +# pinctrl before gpio - gpio drivers may need it > >> + > >> +source "drivers/pinctrl/Kconfig" > >> + > >> source "drivers/gpio/Kconfig" > >> > >> diff --git a/drivers/Makefile b/drivers/Makefile > >>... > >> +# GPIO must come after pinctrl as gpios may need to mux pins etc > >> +obj-y+= pinctrl/ > > > > Don't those patches imply that the GPIO controller code is calling into > > the pinmux code to perform muxing, not the other way around? > > Yes. > > Grant does not seem to like the idea of the gpio subsystem > meddeling with all this stuff anyway, so I intend to take all that > into pinctrl, and then gpio only deal with, well GPIO. Setting > bits quickly and such. > > That is, irrespective of the fact that these functions may live > in the same register range, this is a divide between functionality > not hardware blocks. > > > == When pins get marked as in-used > > > > There seems to be a discrepancy in the way the client APIs work: For > > special functions, the client calls pinmux_get, then later pinmux_enable, > > whereas for GPIOs, the client just calls pinmux_request_gpio. I'm not > > sure of the benefit of having separate pinmux_get and pinmux_enable > > calls; I thought the idea was that a client could: > > > > a = pinmux_get(dev, "fu
RFC: drivers: swarren's pinmux API concept
Linus W,
This "patch" is a quick-and-dirty outline of where I'd love to see the
pinmux API go, but this time in code rather than English description.
Note that I elided a bunch of stuff from the headers; some comments weren't
updated, I removed the inline functions for when pinmux is disabled, etc.
The code has not been compiled. The main idea is just to give an idea of
what I'm thinking.
Let me know if any of this looks reasonable to you; if so, I can look into
fixing the issues above, making the core pinmux code actually work in this
model, or whatever.
Thanks!
---
drivers/pinctrl/pinmux-tegra2.c | 85 ++
include/linux/pinctrl/consumer.h | 47
include/linux/pinctrl/machine.h | 47
include/linux/pinctrl/provider.h | 149 ++
4 files changed, 328 insertions(+), 0 deletions(-)
create mode 100644 drivers/pinctrl/pinmux-tegra2.c
create mode 100644 include/linux/pinctrl/consumer.h
create mode 100644 include/linux/pinctrl/machine.h
create mode 100644 include/linux/pinctrl/provider.h
diff --git a/drivers/pinctrl/pinmux-tegra2.c b/drivers/pinctrl/pinmux-tegra2.c
new file mode 100644
index 000..d010f9c
--- /dev/null
+++ b/drivers/pinctrl/pinmux-tegra2.c
@@ -0,0 +1,85 @@
+/*
+ * pinctrl driver for NVIDIA Tegra 2
+ *
+ * Author: Stephen Warren
+ * Copyright (C) 2011 NVIDIA, Inc.
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+
+#include
+
+struct tegra_pin {
+ struct pinctrl_pin core;
+ int reg_addr; /* e.g. for per-pin config */
+ /* And other info */
+};
+
+struct tegra_function {
+ struct pinctrl_function core;
+ int reg_value;
+ /* And other info */
+}
+
+struct tegra_group {
+ struct pinctrl_group core;
+ int reg_addr; /* e.g. for muxing and per-pin config */
+ int reg_bit;
+ /* And other info */
+}
+
+struct tegra_pin tegra_pins[] = {
+ { .core = { .name = "a0", }, }, /* 0 */
+ { .core = { .name = "a1", }, }, /* 1 */
+ { .core = { .name = "b0", }, }, /* 2 */
+ { .core = { .name = "b1", }, }, /* 3 */
+ { .core = { .name = "c0", }, }, /* 4 */
+ { .core = { .name = "c1", }, }, /* 5 */
+ { .core = { .name = "d0", }, }, /* 6 */
+ { .core = { .name = "d1", }, }, /* 7 */
+ { .core = { .name = "e0", }, }, /* 8 */
+ { .core = { .name = "e1", }, }, /* 9 */
+
+/* Or: */
+#define PIN_B0 2
+ .PIN_B0 = { .core = { .name = "b0", }, },
+};
+
+struct tegra_function tegra_function[] = {
+ { .core = { .name = "kbc-col-1:0", }, 1 }, /* 0 */
+ { .core = { .name = "kbc-row-1:0", }, 2 }, /* 1 */
+ { .core = { .name = "i2c0", },3 }, /* 2 */
+ { .core = { .name = "uartrxtx0", }, 4 }, /* 3 */
+ { .core = { .name = "spiclkdata0", }, 5 }, /* 4 */
+ { .core = { .name = "gpioctrlr0", }, 0x8001}, /* 5 */
+ { .core = { .name = "gpioctrlr1", }, 0x8002}, /* 6 */
+
+/* Or: */
+#define FUNC_I2C0 2
+ .FUNC_I2C0 = { .core = { .name = "i2c0", }, 3},
+
+};
+
+int pins_kbca[] = { 0, 1 };
+int funcs_kbca[] = { 0, 5 };
+
+int pins_kbcb[] = { 2, 3 };
+int funcs_kbcb[] = { 1, 5 };
+
+int pins_ddca[] = { 4, 5 };
+int funcs_ddca[] = { 2, 3, 5, 6 };
+
+int pins_ddcb[] = { 6, 7 };
+int funcs_ddcb[] = { 2, 4, 5, 6 };
+
+int pins_xx[] = { 8, 9};
+int funcs_xx[] = { 5, 6};
+
+struct tegra_group tegra_groups[] = {
+ { .core = { .name = "kbca", npins = 2, pins = pins_kbca, nfunctions =
2, functions = funcs_kbca} },
+ { .core = { .name = "kbcb", npins = 2, pins = pins_kbcb, nfunctions =
2, functions = funcs_kbcb} },
+ { .core = { .name = "ddca", npins = 2, pins = pins_ddca, nfunctions =
4, functions = funcs_ddca} },
+ { .core = { .name = "ddcb", npins = 2, pins = pins_ddcb, nfunctions =
4, functions = funcs_ddcb} },
+ { .core = { .name = "xx", npins = 2, pins = pins_xx, nfunctions =
2, functions = funcs_xx } },
+};
+
diff --git a/include/linux/pinctrl/consumer.h b/include/linux/pinctrl/consumer.h
new file mode 100644
index 000..81cedf3
--- /dev/null
+++ b/include/linux/pinctrl/consumer.h
@@ -0,0 +1,47 @@
+/*
+ * pinctrl subsystem's client/consumer interface
+ *
+ * Copyright (C) 2011 ST-Ericsson SA
+ * Written on behalf of Linaro for ST-Ericsson
+ * Based on bits of regulator core, gpio core and clk core
+ *
+ * Author: Linus Walleij
+ *
+ * Copyright (C) 2011 NVIDIA, Inc.
+ *
+ * License terms: GNU General Public License (GPL) version 2
+ */
+#ifndef __LINUX_PINCTRL_CONSUMER_H
+#define __LINUX_PINCTRL_CONSUMER_H
+
+#include
+#include
+
+/* This struct is private to the core and should be regarded as a cookie */
+s
RE: [PATCH 1/2] drivers: create a pinmux subsystem v3
Linus Walleij wrote at Monday, June 27, 2011 8:35 AM:
> On Thu, Jun 16, 2011 at 9:10 PM, Stephen Warren wrote:
...
> > Now, we can have multiple entries with the same .map_name:
> >
> > static struct pinmux_map pmx_mapping[] = {
> > {
> > .dev_name = "tegra-sdhci.0",
> > .map_name = "4 bit", # Note this is 4 now
> > .function = "mmc0-1:0",
> > },
> > {
> > .dev_name = "tegra-sdhci.0",
> > .map_name = "4 bit",
> > .function = "mmc0-3:2",
> > },
> >
> > This means that if a client request map name "4 bit", then both functions
> > "mmc0-1:0" and "mmc0-3:2" are part of that mapping.
>
> (...)
>
> > In terms of implementation, this is still pretty easy; all we do when
> > reserving or activating a given mapping is to walk the entire list, find
> > *all* entries that match the client's search terms, and operate on all of
> > them, instead of stopping at the first one.
>
> So:
> pmx = pinmux_get(dev, "4 bit");
>
> in this case would reserve pins for two functions on pinmux_get() and
> activate two different functions after one another when
> we pinmux_enable(pmx) on this, "mmc0-1:0" and "mmc0-3:2" in some
> undefined order (inferenced across namespace).
>
> I don't think it's as simple as you say, this gets hairy pretty quickly:
>
> Since my previous pinmux_get() would create a struct pinmux * cookie
> for each map entry, assuming a 1:1 relationship between map entries
> and pinmuxes, we now break this, and you need to introduce
> more complex logic to search through the pinmux_list and dynamically
> add more functions to each entry with a matching map_name.
In the patch you posted, pinmux_get looked up a single specific
pinmux_map* and stored it in the struct pinmux. You're right that
continuing to do something similar means needing to store an array of
pinmux_map* in the struct pinmux, and dynamically adding more and more
entries as you search through the mapping table. A bit painful.
However, if instead of that, you store the original parameters to
pinmux_get() in struct pinmux, there's nothing to dynamically store
there. Instead, the implementation of pinmux_get and pinmux_enable is
very roughly:
pinmux_get:
Allocate struct pinmux, fill it in:
list_for_each_entry(mapping table)
if (matches search terms)
check_not_already_in_use
acquire_pins()
// Note: no break here
pinmux_enable:
// Note we redo the whole search here based on the original terms
// rather than having pinmux_get pre-calculate the search result
list_for_each_entry(mapping table)
if (matches search terms)
ops->enable()
In fact, I'd imagine that the list_for_each_entry call above could be
placed into a utility function that implements the searching logic, to
keep the code in one place, with a function parameter:
pinmux_search(terms, callback):
list_for_each_entry(mapping table)
if (matches search terms)
callback()
pinmux_get_body():
check_not_already_in_use
acquire_pins()
pinmux_get(terms):
Allocate struct pinmux, fill it in:
pinmux_search(terms, pinmux_get_body)
pinmux_enable_body():
ops->enable()
pinmux_enable():
pinmux_search(terms, pinmux_enable_body)
Alternatively, each map entry contains a list node that's set up.
The list is set up by pinmux_get, and contains all mapping entries
associated with the original search terms. Struct pinmux points at
the head of the list. pinmux_enable then just needs to iterate over
that list, not the whole mapping array, and doesn't need to implement
the search algorithm. That might actually be simpler. Since each
mapping entry would only be get'd once, there'd be no conflicts with
multiple things trying to use the one list node at once.
> Then you need to take care of the case where acquiring pins for
> one of the functions fail: then you need to go back and free the
> already acquired pins in the error path.
That's not that hard; just iterate over the whole list again, applying
a function that undoes what you did before. It's just like the error
handling code that already exists in acquire_pins for example. The only
hard part might be stopping when you get to the point you already got
to in the first loop, but actually that's just a matter of passing in
a "stop at this index" parameter to the pinmux_search() I mentioned
above.
> I tried quickly to see if I could code this up, and it got very complex
> real quick, sadly. Maybe if I can just get to iterate a v4 first, I
> could venture down this track.
>
> But if y
RE: [PATCH 1/4 v4] drivers: create a pin control subsystem
Linus Walleij wrote at Friday, August 19, 2011 3:54 AM: > From: Linus Walleij > > This creates a subsystem for handling of pin control devices. > These are devices that control different aspects of package > pins. Sorry to keep harping on about this, but I'm still not convinced the data model is correct. Note: There are two places where a data model is relevant: (a) The API exposed to clients of the pinmux API (b) the API between the pinmux API core and the pinmux chip implementations. Your patches currently use the same data model for both. I believe we can use a different data model for the two interfaces. My discussion below talks solely about the interface at point (b); I can talk more about (a) later. I'm trying to talk about one thing at a time right now to avoid my previous overwhelming emails, and gain consensus on one point at a time. Here are the two possible models: Model 1 (as implemented in the pin control subsystem): * Each pinmux chip defines a number of functions. * Each function describes what functionality is mux'd onto the pins. * Each function describes which pins are affected by the function. Result: If there are a couple of controllers that can each be mux'd out two places, you get four functions: Function i2c0-0: Controller i2c0 is mux'd onto pins 0 and 1 Function i2c0-1: Controller i2c0 is mux'd onto pins 2 and 3 Function spi0-0: Controller spi0 is mux'd onto pins 0 and 1 Function spi0-1: Controller spi0 is mux'd onto pins 4 and 5 Model 2 (what I'd like to see) * Each pinmux chip defines a number of functions. * Each function describes the functionality that can be mux'd out. * Each pinmux chip defines a number of pins. * Each pinmux chip defines which functions are legal on which pins. Result: With the same controllers and pins above, you get: Function i2c0 Function spi0 Pins 0, 1, 2, 3, 4, 5 Pins 0, 1 accept functions i2c0, spi0 Pins 2, 3 accept functions i2c0 Pins 4, 5 accept functions spi0 We'd still have a single controller-wide namespace for functions, it's just that functions are something you mux onto pins, not the combination of mux'd functionality and the pins it's been mux'd onto. Advantages: * I believe this model is much more directly maps to the way most HW works; there's a register for each pin where you write a value to select which functionality to mux onto that pin. Assuming that's true, using this data model might lead to simpler pinmux chip implementations; they'd require fewer mapping tables. * Given that's the way Tegra HW works at least, the patches I recently posted to initialize the Tegra pinmux from device-tree define bindings that use this model. I think it's important that such bindings use the same data model as the pinmux chip data model. This keeps consistency between boot-time initialization and the pinmux chip portion of any run-time pinmux modifications. As an aside, I see your patches use the pinmux API at driver probe time to set up the pinmux, whereas my init-pinmux-driver-from-dt patches do a pinmux-controller-wide initialization at probe time. There was some discussion in response to earlier patches re: which way was better, and I got the impression that the pinmux API was mainly for runtime changes, rather than boot-time initial configuration. If that's not the case, then I guess we should drop my proposed init-pinmux-driver-from-dt patches and put all that code into your pinmux subsystem... -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/4 v4] drivers: create a pin control subsystem
Linus Walleij wrote at Thursday, August 25, 2011 4:13 AM:
> On Wed, Aug 24, 2011 at 8:29 PM, Stephen Warren wrote:
> > Linus Walleij wrote at Friday, August 19, 2011 3:54 AM:
> >>
> >> This creates a subsystem for handling of pin control devices.
> >> These are devices that control different aspects of package
> >> pins.
> >
> > Sorry to keep harping on about this, but I'm still not convinced the data
> > model is correct.
>
> Don't feel sorry! I'm getting very much done and much important
> research and thinking is happening thanks to you valuable feedback.
> Keep doing this!
>
> What I notice is that the review comments have changed focus
> from the earlier contention point about having multiple functions
> per map entry to something else, which to me seems like it
> comes from device tree work, and is about describing how each
> and every pin is wired to its function. Does this correspond to
> your recent pinmux experience?
I think everything I wrote before still stands.
I was simply trying to write a shorter email more focused on a single
point, and a point that was more directly related to HW and driver
implementation.
Then, if/when we reach(ed) a consensus on that, I was going to start
talking about the client driver <-> pinmux driver mapping table.
> > Here are the two possible models:
> >
> > Model 1 (as implemented in the pin control subsystem):
> >
> > * Each pinmux chip defines a number of functions.
> > * Each function describes what functionality is mux'd onto the pins.
> > * Each function describes which pins are affected by the function.
>
> This is correct.
>
> > Result:
> >
> > If there are a couple of controllers that can each be mux'd out two
> > places, you get four functions:
> >
> > Function i2c0-0: Controller i2c0 is mux'd onto pins 0 and 1
> > Function i2c0-1: Controller i2c0 is mux'd onto pins 2 and 3
> > Function spi0-0: Controller spi0 is mux'd onto pins 0 and 1
> > Function spi0-1: Controller spi0 is mux'd onto pins 4 and 5
>
> Yes, if and only if the pinmux driver find it useful to expose
> these two ports on the two sets of pins each.
>
> For example: there really *is* a bus soldered to pin {0,1}
> and another bus soldered to pin {2,3}, and each has devices
> on it. But I have only one single i2c controller i2c0.
>
> If pins {2,3} were not soldered or used for something else
> it doesn't make sense for the pin controller to expose
> two functions like this.
>
> So this is dependent on platform/board data. We need
> to know how the chip has been deployed in this very
> machine to know what functions to expose.
So this is definitely an important point where I'm thinking differently.
I see the pinmux driver as *always* exposing *all* possible combinations
of pin/function/... The pinmux driver would be purely SoC-specific and
not have any knowledge at all of the board/machine/... Most likely, the
configuration the pinmux driver exposes would be hard-coded in tables in
the driver file, and never come from board-files or device-tree, since it
would not vary.
To me, this keeps the pinmux driver as simple as possible; it always works
in exactly the same way, all its data is hard-coded, so there's never a
need to parse it out of device-tree, etc. All (board) policy is implemented
by the pinmux core, and the pinmux driver doesn't know anything about it.
(although the initial board-specific setup of the HW may come from device-
tree, that would be a one-time setup, and wouldn't affect the data exposed
to the interface between the pinmux core and pinmux driver)
To make pinmux work in a board-/machine-specific fashion, the function-
mapping table (as processed by the pinmux core only) would be board-/
machine-specific, and come from board-files or device-tree. It would be
purely down to this mapping table which functions were legal to use on
which pins for a given board/machine.
> > Model 2 (what I'd like to see)
> >
> > * Each pinmux chip defines a number of functions.
> > * Each function describes the functionality that can be mux'd out.
> > * Each pinmux chip defines a number of pins.
> > * Each pinmux chip defines which functions are legal on which pins.
> >
> > Result:
> >
> > With the same controllers and pins above, you get:
> >
> > Function i2c0
> > Function spi0
> > Pins 0, 1, 2, 3, 4, 5
> > Pins 0, 1 accept functions i2c0, spi0
> > Pins 2, 3 accept functions i2c0
> > Pins 4, 5 accept functions spi0
> >
> > We'd still have a single controller-wide namespace for functions, it's
> > just that functions
RE: [PATCH 0/4 v4] pin controller subsystem v4
Barry Song wrote at Thursday, August 25, 2011 7:59 PM:
> 2011/8/22 Linus Walleij :
> > On Sun, Aug 21, 2011 at 4:42 PM, Barry Song <21cn...@gmail.com> wrote:
> >
> >> it seems there is not an actual example that gpio requests pin from
> >> pinctrl yet. i might give one on SiRFprimaII.
> >
> > No good example yet, no.
> >
> > The reason is that for the U300 that I use as guinea pig, the
> > GPIO driver is tangled up in discussions about how to handle
> > the special control mechanics like requesting muxing and
> > biasing pins. Right now it seems easier to rewrite all that
> > to use the new pinctrl subsystem rather than actually trying
> > to work it into the GPIO subsystem first and refactor from
> > there, and that needs quite a bit of upfront work...
>
> Do you want the pinmux_request_gpio called by the gpiolib driver or by
> every device driver who uses this gpio?
> Do you think the following make sense in gpiolib driver?
>
> static int xxx_gpio_request(struct gpio_chip *chip, unsigned offset)
> {
> int ret = 0;
>
> ret = pinmux_request_gpio(chip->base + offset);
> if (ret)
> goto out;
> .
> out:
> return ret;
> }
I would have expected this to work the other way around; "gpio" is just
another function that can be assigned to a pin.
This has the benefit of catering to the following cases:
Below, "SF" == "some special function"
1) Pin is SF or a particular GPIO.
2) Pin is SF or a particular GPIO from GPIO controller A, or a particular
GPIO from controller B
3) Pin is SF, or one of a number of user-selectable GPIOs.
Case (1) is covered by the code quoted above from earlier in the thread.
Cases (2) and (3) can't be covered by that code, and according to comments
in earlier replies, both actually exist in real HW.
For reference, here's how I'd imagine modeling those three cases in
pinmux (all based on my earlier comments about the data model I imagine,
rather than what's currently in the pinmux patches):
1) Have a single function "gpio" that can be applied to any pin that
supports it. The actual GPIO number that gets mux'd onto the pin differs
per pin, and is determine by HW design. But logically, we're connecting
that pin to function "gpio", so we only need one function name for that.
2) Have a function for each GPIO controller that can be attached to a
pin; "gpioa" or "gpiob". Based on the pin being configured, and which of
those two GPIO functions is selected, the HW determines the specific GPIO
number that's assigned to the pin.
3) Where the GPIO ID assigned to pins is user-selectable, have a function
per GPIO ID; "gpio1", "gpio2", "gpio3", ... "gpio31". This sounds like
it'd cause a huge explosion in the number of functions; one to represent
each GPIO ID. However, I suspect this won't be too bad in practice, since
there's presumably some practical limit to the amount of muxing logic that
can be applied to each pin in HW, so the set of options won't be too large.
If the set of GPIO IDs that can be assigned to any particular pin is a subset
of the whole GPIO number space, e.g.:
pina: gpio1, gpio2, gpio3, gpio4
pinb: gpio2, gpio3, gpio4, gpio5
pinc: gpio3, gpio4, gpio5, gpio6
... then I imagine HW has already defined an enumerator gpioa, gpiob, gpioc,
gpiod and a mapping from those to the specific GPIO ID that each means when
assigned to a given pin, so those gpioa/b/c/d values could be used as the
function exposed by the pinmux driver.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/4 v4] drivers: create a pin control subsystem
Linus Walleij wrote at Friday, August 26, 2011 2:35 AM:
> On Thu, Aug 25, 2011 at 9:13 PM, Stephen Warren wrote:
> > Linus Walleij wrote at Thursday, August 25, 2011 4:13 AM:
> >
> >> So this is radically different in that it requires the pin control
> >> system to assume basically that any one pin can be used for
> >> any one function.
> >
> > I think that's the wrong conclusion; 1:many isn't the same as 1:all/any.
> > The data model might be structured to allow that, but in practice most
> > HW allows 1:some_subset, not 1:all/any. I think this was well-covered in
> > some other recent responses in this thread.
>
> OK what I was mainly after was if the data model should
> be structured to accept phone-exchange type muxing. If it
> does and such a hardware appears - I mean a hardware where
> any pin can be muxed anywhere, and given the second point
> you make that the pinmux subsystem should expose all possible
> combinations, it will lead to a situation where the driver
> needs to expose all permutations i.e. (n over k) combinations
> per function where n is the number of available pins and k
> is the number of pins used by any one function. That would
> just explode...
>
> So if we assume that such a hardware does not exist but
> the number of permutations of functions will always be limited,
> it makes much more sense.
I guess I don't quite understand the implications of what you wrote here;
I interpret the above as meaning you still prefer the data model that's
in your existing patches. However, this can't be true given your response
about the function mappings below. So, I'll mainly ignore the above and
focus on responding below:-)
> I'll encode this theoretical assumption in
> Documentation/pinctrl.txt as I go along.
>
> >> So the data model I'm assuming is:
> >>
> >> - Pins has a 1..* relation to functions
> >> - Functions in general have a 1..1 relation to pins,
> >> - Device drivers in general have a 1..1 relation to
> >> functions
> >> - Functions with 1..* relation to pins is uncommon
> >> as is 1..* realtions between device drivers and
> >> functions.
> >>
> >> The latter is the crucial point where I think we have
> >> different assumptions.
> >
> > As a few other replies pointed out, a number of chips do allow the at
> > least some logical functions to be mux'd onto different pins. Tegra
> > certainly isn't unique in this.
>
> Yeah I get this now... and it's a handful of alternatives for a
> few functions, sorry for being such a slow learner.
>
> >> If it holds, it leads to the fact that a pinmux driver
> >> will present a few functions for say i2s0 usually only
> >> one, maybe two, three, never hundreds.
> >
> > Certainly I'd assume the number of pins/groups that a given function
> > could be mux'd out onto is small, say 1-3. But, certainly not limited
> > to just 1 in many cases.
>
> Sure, we're on the same page. So I now need to find a
> way to expose a few different localities per function from
> the system and all the way to the map, and drop the string
> naming system so instead of using spi0-0, spi0-1, spi0-2
> I use some tuple like {"spi0", 0}, {"spi0", 1}, {"spi0", 2}
> and I call the latter integer something like "locality" or
> "position".
OK. That sounds like exactly what I was asking for.
I'd argue that "locality" or "position" is in fact the pin name.
So, re-using my previous example of the data exposed by a pinmux driver:
> > Function i2c0
> > Function spi0
> > Pins 0, 1, 2, 3, 4, 5
> > Pins 0, 1 accept functions i2c0, spi0
> > Pins 2, 3 accept functions i2c0
> > Pins 4, 5 accept functions spi0
Then, I think that the mapping table processed by the pinmux core might
look like:
devicedevices_function_name pin_to_configure driver_function_name_for_pin
- -
foo-i2c.0 busa 0i2c0
foo-i2c.0 busa 1i2c0
foo-i2c.0 busb 0i2c0
foo-i2c.0 busb 1i2c0
(supplied by board files or device-tree)
So, when device foo-i2c.0 requests function "busa", the pinmux core would
make a couple calls to the actual pinmux driver:
configure pin 0 for function i2c0
configure pin 1 for function i2c0
This model does require that the pinmux core potentially process multiple
entries in the mapping table for each driver-re
RE: [PATCH 1/4 v5] drivers: create a pin control subsystem v5
Linus Walleij wrote at Monday, August 29, 2011 3:10 AM: > This creates a subsystem for handling of pin control devices. > These are devices that control different aspects of package > pins. ... > - Defined a "position" for each function, so the pin controller now > tracks a function in a certain position, and the pinmux maps define > what position you want the function in. (Feedback from Stephen > Warren and Sascha Hauer). My thoughts: I guess "positions" can be used to represent the muxing capabilities of Tegra. However, I still don't think this is the right model. 1) There is still a 1:1 correspondence between what a function in the pinmux core<->driver interface, and the pinmux mapping table. I believe we need a 1:n correspondence. HW typically has a set of pins (or pingroups on Tegra), and there's a register field for each of these that indicates which signal (or set of signals in the pin group case) are mux'd to it/them. At this level, setting a pin/pingroup to a particular function isn't all that's required to use a particular IO controller; you typically need to apply the same function to many pins/pingroups. However, the pinmux API defines a function as everything needed to use a an entire driver/controller. Now, I do see that the concept of "position" can encompass this, but I don't think it's entirely right for it to do so: * "Positions" don't map very well to the HW programming model I assert above. * With the current "position" usage, the data structures are not in "normal form" (normalized), taking a leaf from database technology. * Normalizing would help the "combinatorial explosion" problem mentioned before. i.e. rather than: driver function/position list: function@position pins - mmc0@00, 1, 2, 3 mmc0@10, 1, 2, 3, 4, 5 mmc0@20, 1, 2, 3, 4, 5, 6, 7, 8, 9 core mapping table: driver name function@position -- - mmc02-bit mmc0@0 mmc04-bit mmc0@1 mmc08-bit mmc0@2 I'd far rather see: driver function list: function - mmc0 driver pin/pingrup list: (names of pins or pingroups): pingroup A B C core mapping table: driver name position function -- - mmc02-bit A mmc0 mmc04-bit A mmc0 mmc04-bit B mmc0 mmc08-bit A mmc0 mmc08-bit B mmc0 mmc08-bit C mmc0 Note again that I'm assuming above that the driver-supplied function and pingroup list included all possible options the SoC HW supports, whereas the mapping table would include just those configurations ever actually used by the particular board the code is executed upon; from board files or devicetree data. Hence, a given board would only need rows 0, 1+2, or 3+4+5 from the mapping table shown above, assuming the width of the MMC port didn't vary at run-time. 2) "Positions" are defined per "function", rather than as a global concept. This leads to positions having meaningless integer names. As such, constructing the mapping table will be error-prone; who could remember that position 0 is a 2-bit bus using a certain set of pins, yet position 1 is an 8-bit bus using a different set of pins? I suppose textual names might help here. However, by replacing the concept of positions with multiple explicit entries in the mapping table (as in my example above), that problem is solved; we name the pins (or pingroups) to which we apply the driver-level function in each entry, thus it's more self-documenting. 3) It's unclear how well pinmux_config() can be applied. Some pin parameters might be defined per: * Pin (probably most systems other than Tegra) * Pin group (for pull-up/down, tristate on Tegra) * Drive group (another set of groups of pins on Tegra that arbitrarily overlap/intersect with the mux pin groups (for drive-strength settings on Tegra). For pinmux_config() to work, we'd need some API-level concept in order to name what pin/group to apply various settings to. Currently, that naming is an entry from the core mapping table, since pinmux_config() works on functions returned by pinmux_get(). That doesn't seem right, since it prevents the API being used for individual pins/pingroups. Even where say 5 different pins/pingroups are used by the same mapping table function, there's no reason to believe that their tristate or drive strength attributes would all be identical; at least input and output pins or control and data might be programmed differently. So in summary, I really think the data model should be as below. Now, I know you've referred to this as a "telephone exchange" model, and argued that we don't need full telephone exchange support, but even if that's true (and I'm not convinced
RE: [PATCH 1/4 v5] drivers: create a pin control subsystem v5
Linus Walleij wrote at Thursday, September 01, 2011 3:13 AM:
...
> I will need Arnds or similars advice on it so we don't
> grow a lib/mysql into the kernel with this kind of
> schemes and get slammed for overcomplicating
> things when trying to merge the beast.
I /think/ the whole multi-row thing is just doing this:
for each row of table:
if it matches search terms:
Activate the specified function on the group
Whereas handling just one entry is almost the same:
for each row of table:
if it matches search terms:
Activate the specified function on the group
break
Of course, in the former case, the error-handling also has to iterate
over all the already-processed rows and unreserved/unprogram the HW too,
but that should be a pretty simple loop too.
And replying to your immediately earlier email:
...
> > Hence, a given board would only need rows 0, 1+2, or
> > 3+4+5 from the mapping table shown above, assuming the width of the MMC
> > port didn't vary at run-time.
>
> OK we're on the same page I think. If I fix this, then
> you're happy?
I haven't read patchset 6 yet, but from the descriptions in your email
and the patch 0 changelog, it certainly sounds like we're on very
similar pages now.
> > Some pin parameters might be defined per:
> >
> > * Pin (probably most systems other than Tegra)
> > * Pin group (for pull-up/down, tristate on Tegra)
> > * Drive group (another set of groups of pins on Tegra that arbitrarily
> > overlap/intersect with the mux pin groups (for drive-strength settings
> > on Tegra).
>
> Since these things are unrelated to muxing I prefer that
> we add that as a separate set of ops in struct pinctrl_device.
>
> The driver can very well reuse the same groups internally,
> nothing prevents that.
>
> I am not intending to address the issue of pin bias,
> driver strength, load capacitance, schmitt-trigger input
> etc etc etc in this first iteration of the pin control subsystem,
> what I want is to get the infrastructure and pin muxing
> into the kernel then extend it with other pin control.
>
> I'm trying to avoid excess upfront design.
>
> Do you think these features are needed from day 1
> to be of any use for Tegra?
I think having just muxing support would work fine for a first pass.
Looking forward a little, I expect that different SoCs have such a
different set of biasing/driver-strength/... options that actually having
some formalized semantic representation of what those options are doesn't
make sense; all we need is a standard API to set the values for a given
pin or pingroup; just like the config API you already had, but applied
to pingroups rather than explicitly GPIOs, and where the driver supplies
the names for the settings and interprets what the values mean for a
given setting. I imagine having the config API in the first pass wouldn't
be contentious, provided it applied to pingroups.
Just possibly, I could imagine needing config entries in the mapping table,
so when switching between mapping entries at run-time you could both program
muxing and other configuration. However, that's definitely not a first-pass
thing, as you say. And I'm just guessing if it'll be needed anyway.
> > Or, in more normalized fashion, with multiple rows per pingroup, each
> > entry containing:
> > * Name of pingroup
> > * One pin with in the pingroup
>
> I don't get this "one pin within the the pingroup"
> what if the same pin is part of multiple groups?
It was just a data-representation thing. You could either have:
struct {
char * pingroup;
int npins;
int * pins;
};
Where one row represents everything about a pingroup, or:
struct {
char * pingroup;
int pin;
};
Where you'd need a row for each pin in a pingroup.
Go with whatever you find is easiest; I was just thinking databases for
a while!
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/4 v6] drivers: create a pin control subsystem v6
Linus Walleij wrote at Thursday, September 01, 2011 3:32 AM: > This creates a subsystem for handling of pin control devices. > These are devices that control different aspects of package > pins. Overall, I think this is beginning to look very good. Thanks for taking care of my requests. I want to check one thing: A given pin can be in multiple pin groups at once. This will be important when biasing/... is added, since Tegra will then having 2 sets of overlapping pin groups that cover all/most pins; one set for muxing and one for biasing etc. Each pin will be in one of the mux groups and one of the bias groups. I'm pretty sure this is the case, but just wanted to double-check. As you already acknowledge, one missing feature is handling multiple map entries for each pinmux_get()/enable(). My final area of contention is the GPIO-specific APIs and mapping. The only thing pinmux allows you to do with the GPIO ranges are request_gpio and free_gpio. Some thoughts: * These only exist to prevent an explosion in the number of functions. I don't think we need these APIs to avoid this; see my discussion below. * These APIs don't work in the same way as mapping table entries; what if switching a controller between two sets of pins also required using a different GPIO; I might want to put the equivalent of request_gpio and free_gpio into the mapping table. This can only be done if we represent GPIOs as just another function, rather than special- casing them. * If we get rid of the GPIO APIs, we can then get rid of the GPIO ranges too, which cuts out a lot of code. I would imagine treating GPIOs as just another function. I'll repeat some text I wrote previously (https://lkml.org/lkml/2011/8/26/298) about how I see this working: >SW For reference, here's how I'd imagine modeling those three cases in >SW pinmux (all based on my earlier comments about the data model I imagine, >SW rather than what's currently in the pinmux patches): >SW 1) Have a single function "gpio" that can be applied to any pin that >SW supports it. The actual GPIO number that gets mux'd onto the pin differs >SW per pin, and is determine by HW design. But logically, we're connecting >SW that pin to function "gpio", so we only need one function name for that. So here, the only issue is that if "GPIO" can be assigned per pin, we'd need to define a pingroup per pin, even if we had a set of other groups for muxing. (And a pin would have to be in its per-pin pingroup, and mux group, which goes back to my very first comment above.) I suppose this might end up being a lot of pingroups. However, this is all data, and it seems like having large data is better than having large code? Still, these per-pin groups might end up existing for other functionality like biasing anyway, depending on HW. >SW 2) Have a function for each GPIO controller that can be attached to a >SW pin; "gpioa" or "gpiob". Based on the pin being configured, and which of >SW those two GPIO functions is selected, the HW determines the specific GPIO >SW number that's assigned to the pin. >SW 3) Where the GPIO ID assigned to pins is user-selectable, have a function >SW per GPIO ID; "gpio1", "gpio2", "gpio3", ... "gpio31". This sounds like >SW it'd cause a huge explosion in the number of functions; one to represent >SW each GPIO ID. However, I suspect this won't be too bad in practice, since >SW there's presumably some practical limit to the amount of muxing logic that >SW can be applied to each pin in HW, so the set of options won't be too large. In https://lkml.org/lkml/2011/8/29/74, it sounded like you weren't averse to the idea of treating GPIOs like any other function. A number of smaller comments directed at specific parts of the patch text are below. > +++ b/Documentation/pinctrl.txt > +The pin control subsystem will call the .list_groups() function repeatedly > +beginning on 0 until it returns non-zero to determine legal selectors Given you said that pingroups are mandatory, I wonder why struct pinctrl_desc doesn't just have an ngroups field to; that'd save having to iterate over calls to list_groups to find the total number of groups. > +Pinmux conventions > +== Just wanted to say that I agree with the description of the data model given in this section of the docs, with the exceptions listed early in this email. > +- FUNCTIONS using a certain PIN GROUP on a certain PIN CONTROLLER are > provided > + on a first-come first-serve basis, It might be better to say that pins are provided on a first-come first- serve bases, and hence pin groups too. Function doesn't really come into it; the function is just how you program the pins/groups after you've applied first-come first-serve. > +Pinmux drivers > +== > + > +It is the responsibility of the pinmux driver to determine whether or not > +the requested function can actually be enabled, and in that case poke the > +hardware so that this happens. Perhaps augment that to make it clear that
RE: [PATCH 2/4 v6] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Thursday, September 01, 2011 3:33 AM:
> This adds a driver for the U300 pinmux portions of the system
> controller "SYSCON". It also serves as an example of how to use
> the pinmux subsystem. This driver also houses the platform data
> for the only supported platform.
> diff --git a/drivers/pinctrl/pinmux-u300.c b/drivers/pinctrl/pinmux-u300.c
> +/*
> + * We define explicit indexes of the groups since these will be
> + * referenced in the pinmuxes below.
> + */
> +#define POWERGRP_INDEX 0
> +#define UART0GRP_INDEX 1
> +#define MMC0GRP_INDEX 2
> +#define SPI0GRP_INDEX 3
> +
> +static const struct u300_pin_group u300_pin_groups[] = {
> + [POWERGRP_INDEX] = {
> + .name = "powergrp",
> + .pins = power_pins,
> + .num_pins = ARRAY_SIZE(power_pins),
> + },
...
> +/**
> + * struct u300_pmx_func - describes U300 pinmux functions
> + * @name: the name of this specific function
> + * @groups: corresponding pin groups
> + * @onmask: bits to set to enable this when doing pin muxing
> + */
> +struct u300_pmx_func {
> + const char *name;
> + const unsigned groups[1];
> + const struct u300_pmx_mask *mask;
> +};
> +
> +static const struct u300_pmx_func u300_pmx_functions[] = {
> + {
> + .name = "power",
> + .groups = { POWERGRP_INDEX },
> + /* Mask is N/A */
> + },
Hmmm. That's a lot of _INDEX defines that'd need to be set up, at least
to fully represent a chip like Tegra. Can the pinmux core be modified
such that the group list is an array of names (char*) rather than the
actual numeric IDs of the groups? Still, perhaps we could use the enum
we already have for this, so perhaps it isn't a big deal...
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/4 v6] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Friday, September 02, 2011 2:12 AM:
> On Thu, Sep 1, 2011 at 11:33 PM, Stephen Warren wrote:
>
> >> +static const struct u300_pmx_func u300_pmx_functions[] = {
> >> + {
> >> + .name = "power",
> >> + .groups = { POWERGRP_INDEX },
> >> + /* Mask is N/A */
> >> + },
> >
> > Hmmm. That's a lot of _INDEX defines that'd need to be set up, at least
> > to fully represent a chip like Tegra. Can the pinmux core be modified
> > such that the group list is an array of names (char*) rather than the
> > actual numeric IDs of the groups? Still, perhaps we could use the enum
> > we already have for this, so perhaps it isn't a big deal...
>
> Well I could think about a lot of ways to do this, but it's basically up
> to the driver, the U300 is just some simple example of what you can
> do, it's just trying to satisfy the API.
>
> Maybe as part of writing the nVidia driver you find a clever
> mechanism for doing this, if it's looking generally useful at that
> point then let's move it to the core I'd say.
The reason I asked about the pinmux core handling this is because the
driver op get_function_groups:
+ int (*get_function_groups) (struct pinctrl_dev *pctldev,
+ unsigned selector,
+ unsigned ** const groups,
+ unsigned * const num_groups);
returns an array of integer indexes. I think the only way to get rid of
the index definitions in the drivers is to allow get_function_groups to
return an array of strings instead.
Well, I suppose the implementation of get_function_groups could convert
from an internal array of strings to the returned array of integers at
run-time, but that doesn't seem like a great idea.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/4 v6] drivers: create a pin control subsystem v6
Linus Walleij wrote at Friday, September 02, 2011 6:59 AM: > > I would imagine treating GPIOs as just another function. I'll repeat > > some text I wrote previously (https://lkml.org/lkml/2011/8/26/298) about > > how I see this working: > > > >>SW For reference, here's how I'd imagine modeling those three cases in > >>SW pinmux (all based on my earlier comments about the data model I imagine, > >>SW rather than what's currently in the pinmux patches): > > > >>SW 1) Have a single function "gpio" that can be applied to any pin that > >>SW supports it. The actual GPIO number that gets mux'd onto the pin differs > >>SW per pin, and is determine by HW design. But logically, we're connecting > >>SW that pin to function "gpio", so we only need one function name for that. > > > > So here, the only issue is that if "GPIO" can be assigned per pin, we'd > > need to define a pingroup per pin, even if we had a set of other groups > > for muxing. (And a pin would have to be in its per-pin pingroup, and > > mux group, which goes back to my very first comment above.) I suppose this > > might end up being a lot of pingroups. However, this is all data, and > > it seems like having large data is better than having large code? Still, > > these per-pin groups might end up existing for other functionality like > > biasing anyway, depending on HW. > > Hm I have a hard time figuring out how that code would look... > I'm worried that the drivers could get pretty hard to read. For a simple chip that allowed everything to be configured at a pin level rather than pingroup level: Functions: spi i2c gpio pins: A1 A2 A3 A4 Groups, with pin list and legal functions: A1: A1: gpio spi A2: A2: gpio spi A3: A3: gpio i2c A4: A4: gpio i2c For a chip that allowed muxing to be configured at a group level, but GPIOs to be configured at a pin level: Functions: spi i2c gpio pins: A1 A2 A3 A4 Groups, with legal functions: A1: A1: gpio A2: A2: gpio A3: A3: gpio A4: A4: gpio GROUPA: A1, A2: spi GROUPB: A3, A4: i2c ... > Right now I have resorted to the diplomatic solution of actually > supporting both. > > pinmux_request_gpio() will request a gpio pin named > "gpioN" where N is the global GPIO number. > > This fragment from pinmux.c sort of specifies it: > > if (gpio && ops->gpio_request_enable) > /* This requests and enables a single GPIO pin */ > status = ops->gpio_request_enable(pctldev, gpio_range, pin); > else if (ops->request) > status = ops->request(pctldev, pin); > else > status = 0; > > So if the driver doesn't implement the quick ->gpio_request_enable() > the second else-clause will attempt to get the function "gpioN". > > So if the driver just wants to present a single function per pin instead, > it can readily do so, but it must still specify which GPIO ranges it > supports. Ah, OK. The only issue I see with that is that is there end up being a whole ton of functions named "gpio0", "gpio1", ..., "gpio125" etc. Do those functions have to be listed in the table of functions exported by the driver, or does the core expect to pass these "created" function names to the driver without their previously existing? Whereas if you do a function-per-gpio-controller, or function-per-gpio- that- can-be-configured-on-the-pin, you limit the number of function names that are required. > >> diff --git a/include/linux/pinctrl/pinmux.h > >> b/include/linux/pinctrl/pinmux.h > > > >> +/** > >> + * struct pinmux_ops - pinmux operations, to be implemented by pin > >> controller > >> + * drivers that support pinmuxing > >> + * @request: called by the core to see if a certain pin can be made > >> available > >> + * available for muxing. This is called by the core to acquire the pins > >> + * before selecting any actual mux setting across a function. The driver > >> + * is allowed to answer "no" by returning a negative error code > >> + * @free: the reverse function of the request() callback, frees a pin > >> after > >> + * being requested > > > > Shouldn't request/free be for a pingroup, not a pin, since that's what > > functions are assigned to? Also, the function should be passed, since > > some restrictions these functions might need to check for might depend > > on what the pin/group will be used for. > > This is not for checking anything on function or group level. > It's exclusively for things that need to be on a per-pin level, so any > pin can deny being selected for muxing at any time. > > So what you're saying is that you need a function to check > also on group level as part of the pinmux_get() sematics? > > We can add that and have two optional checks: @request_pin() > on pin level and say @request_function_and_group() on the > group+function level, would that work for your scenarios? Well, that's a move in the right direction, but not quite everything I'd like. The basic issue is that a single logical function (I2C controller 0) c
RE: [PATCH 0/2] pin controller subsystem v7
Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
> From: Linus Walleij
>
> This is the sixth iteration of the controller subsystem...
Overall, the changelog sounds like a great move in the right direction.
There's just one small thing I'd comment on here:
> ChangeLog v6->v7:
>
> - Make it possible to have several map entries matching the
> same device, pin controller and function, but using
> a different group, and alter the semantics so that
> pinmux_get() will pick all matching map entries, and
> store the associated groups in a list. The list will
> then be iterated over at pinmux_enable()/pinmux_disable()
> and corresponding driver functions called for each
> defined group. Notice that you're only allowed to map
> multiple *groups* to the same
> { device, pin controller, function } triplet, attempts
> to map the same device to multiple pin controllers will
> for example fail. This is hopefully the crucial feature
> requested by Stephen Warren.
I've been viewing the map table as:
input: (device, device's function)
output: list of (controller, controller's group, controller's function)
... hence I was surprised to see that you explicitly note that mapping a
single device to multiple controllers was disallowed.
That said, I suppose this restriction won't cause any issues for any
use-case I'm aware of; the only possibilities might be:
a) Multiple pinmux controllers within the SoC, but perhaps the driver
should just aggregate multiple HW modules into a single Linux device
anyway?
b) Where the pinmux map wants to affect the pinmux on both ends of some
bus, where the two ends are different chips each having obviously separate
pinmux controllers.
Then again, if this need ever does arise, we should be able to just relax
this restriction without causing any backwards-compatibility issues, so
I don't see a specific need to change this now; I just thought I'd mention
it so you're aware of what I'm thinking.
I'll go review the code now.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/2] drivers: create a pin control subsystem v7
Linus Walleij wrote at Friday, September 16, 2011 6:13 AM: > This creates a subsystem for handling of pin control devices. > These are devices that control different aspects of package > pins. I've read through the documentation and header files, but not the .c files, and this looks almost perfect as far as I can tell right now. I'll try to review the .c files sometime too. I just have one comment: > diff --git a/include/linux/pinctrl/pinmux.h b/include/linux/pinctrl/pinmux.h ... > +/* External interface to pinmux */ > +extern int pinmux_request_gpio(unsigned gpio); > +extern void pinmux_free_gpio(unsigned gpio); > +extern struct pinmux * __must_check pinmux_get(struct device *dev, const > char *name); > +extern void pinmux_put(struct pinmux *pmx); > +extern int pinmux_enable(struct pinmux *pmx); > +extern void pinmux_disable(struct pinmux *pmx); > +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long *data); That definition of pinmux_config doesn't seem as useful as it could be; I'd like the ability to execute pinmux_config on a /named/ group, and I can certainly see a use-case for applying it to /named/ pins too. The issues with applying pinmux_config to a mapping table entry are: * When there are multiple mapping table entries referenced by one pinmux_get, you don't necessarily want to apply the same configuration to all of the groups; think of a bus with a combination of low-speed output control signals and high-speed input data signals or something like that. * When muxing works in groups, you may want to apply the configuration to individual pins rather than the whole groups using in the mapping table. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Friday, September 16, 2011 6:14 AM:
> This adds a driver for the U300 pinmux portions of the system
> controller "SYSCON". It also serves as an example of how to use
> the pinmux subsystem. This driver also houses the platform data
> for the only supported platform.
...
> diff --git a/arch/arm/mach-u300/core.c b/arch/arm/mach-u300/core.c
...
> +/* Pinmux settings */
> +static struct pinmux_map u300_pinmux_map[] = {
> + /* anonymous maps for chip power and EMIFs */
> + PINMUX_MAP_PRIMARY_SYS_HOG("POWER", "power"),
> + PINMUX_MAP_PRIMARY_SYS_HOG("EMIF0", "emif0"),
> + PINMUX_MAP_PRIMARY_SYS_HOG("EMIF1", "emif1"),
> + /* per-device maps for MMC/SD, SPI and UART */
> + PINMUX_MAP_PRIMARY("MMCSD", "mmc0", "mmci"),
> + PINMUX_MAP_PRIMARY("SPI", "spi0", "pl022"),
> + PINMUX_MAP_PRIMARY("UART0", "uart0", "uart0"),
> +};
> +
> +struct u300_mux_hog {
> + const char *name;
> + struct device *dev;
> + struct pinmux *pmx;
> +};
> +
> +static struct u300_mux_hog u300_mux_hogs[] = {
> + {
> + .name = "uart0",
> + .dev = &uart0_device.dev,
> + },
> + {
> + .name = "spi0",
> + .dev = &pl022_device.dev,
> + },
> + {
> + .name = "mmc0",
> + .dev = &mmcsd_device.dev,
> + },
> +};
> +
> +static int __init u300_pinmux_fetch(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(u300_mux_hogs); i++) {
> + struct pinmux *pmx;
> + int ret;
> +
> + pmx = pinmux_get(u300_mux_hogs[i].dev, NULL);
> + if (IS_ERR(pmx)) {
> + pr_err("u300: could not get pinmux hog %s\n",
> +u300_mux_hogs[i].name);
> + continue;
> + }
> + ret = pinmux_enable(pmx);
> + if (ret) {
> + pr_err("u300: could enable pinmux hog %s\n",
> +u300_mux_hogs[i].name);
> + continue;
> + }
> + u300_mux_hogs[i].pmx = pmx;
> + }
> + return 0;
> +}
> +subsys_initcall(u300_pinmux_fetch);
Why not just have the pinmux core support hogging on non-"system" mapping
entries; then everything I quoted above except u300_pinmux_map[] could
be deleted, and the "hog" flag set on the last 3 u300_pinmux_map[] entries.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Wednesday, September 21, 2011 2:25 AM:
> On Wed, Sep 21, 2011 at 12:15 AM, Stephen Warren wrote:
> > Linus Walleij wrote at Friday, September 16, 2011 6:14 AM:
> >> + for (i = 0; i < ARRAY_SIZE(u300_mux_hogs); i++) {
> >> + struct pinmux *pmx;
> >> + int ret;
> >> +
> >> + pmx = pinmux_get(u300_mux_hogs[i].dev, NULL);
> >> + if (IS_ERR(pmx)) {
> >> + pr_err("u300: could not get pinmux hog %s\n",
> >> + u300_mux_hogs[i].name);
> >> + continue;
> >> + }
> >> + ret = pinmux_enable(pmx);
> >> + if (ret) {
> >> + pr_err("u300: could enable pinmux hog %s\n",
> >> + u300_mux_hogs[i].name);
> >> + continue;
> >> + }
> >> + u300_mux_hogs[i].pmx = pmx;
> >> + }
> >> + return 0;
> >> +}
> >> +subsys_initcall(u300_pinmux_fetch);
> >
> > Why not just have the pinmux core support hogging on non-"system" mapping
> > entries; then everything I quoted above except u300_pinmux_map[] could
> > be deleted, and the "hog" flag set on the last 3 u300_pinmux_map[] entries.
>
> Very good question, luckily I have a good answer.
>
> There is no way for the pinmux core to traverse the system and look
> up the apropriate struct device * pointers.
It's unclear to me why that would be needed.
For non-system mappings, either the device-driver in question will be
get'ing and enabling them (this case isn't relevant to this discussion),
or they'll be hogged. In the hog case, I doubt anything would ever want
to disable them and switch the device to a different mapping entry; if
that were the case, the entries shouldn't be hogged, but get/enabled by
the device anyway. Hence, I don't see the need for an activation of a
hog'd non-system mapping entry to care about the device fields that are
in the mapping table at all; they could just be ignored couldn't they?
> When/if we have device tree support, I think this will be possible, then I
> will be able to have the pinmux hog look up devices from device tree
> and hog their pinmux.
>
> At that point we'll likely have the mapping in the device tree too and
> the core does not need to be involved at all.
>
> What I could do right now is add some open-ended function like
> pinmux_hog_device_pinmuxes(struct device **devices);
> that can take an array of devices and hog their respective
> pinmuxes in the hog list. Do you think it's a good idea?
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/2] drivers: create a pin control subsystem v7
Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
> On Tue, Sep 20, 2011 at 11:58 PM, Stephen Warren wrote:
> > Linus Walleij wrote at Friday, September 16, 2011 6:13 AM:
> >> This creates a subsystem for handling of pin control devices.
> >> These are devices that control different aspects of package
> >> pins.
> >
> > I've read through the documentation and header files, but not the .c files,
> > and this looks almost perfect as far as I can tell right now. I'll try to
> > review the .c files sometime too.
>
> Great, I'm hunting your Acked-by/Reviewed-by ...
Very close; I was just holding off until we resolved the discussion on
hog'd non-system mapping table entries, and I figured I'd wait until v8
which presumably would delete the pinmux_config function from the header?
> I will likely request inclusion into linux-next soon-ish.
>
> > I just have one comment:
> >
> >> diff --git a/include/linux/pinctrl/pinmux.h
> >> b/include/linux/pinctrl/pinmux.h
> > ...
> >> +/* External interface to pinmux */
> >> +extern int pinmux_request_gpio(unsigned gpio);
> >> +extern void pinmux_free_gpio(unsigned gpio);
> >> +extern struct pinmux * __must_check pinmux_get(struct device *dev, const
> >> char *name);
> >> +extern void pinmux_put(struct pinmux *pmx);
> >> +extern int pinmux_enable(struct pinmux *pmx);
> >> +extern void pinmux_disable(struct pinmux *pmx);
> >> +extern int pinmux_config(struct pinmux *pmx, u16 param, unsigned long
> >> *data);
> >
> > That definition of pinmux_config doesn't seem as useful as it could be;
>
> It should be removed. It's just there in the header file, I killed off
> the implementation because specific control of a mux doesn't make
> sense. We want to do stuff to pin groups directly, not related to
> muxing, so that kind of thing needs to be in the generic pinctrl
> interface.
OK.
> > I'd like the ability to execute pinmux_config on a /named/ group, and I
> > can certainly see a use-case for applying it to /named/ pins too.
>
> That sounds correct to me.
>
> To abstract things the stuff we can do with the group should be
> something enumerated too. So:
>
> pinctrl_config_group(const char *pinctrl_device, const char *group,
> const char *mode);
> pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
>
> So the driver need an API to enumerate pin and group modes.
>
> I might want to save this thing for post-merge of the basic API and
> pinmux stuff though so we don't try to push too much upfront
> design at once.
Yes, adding in a replacement _config API can certainly be a later patch.
One comment on the API above: I think you want "mode" (or "setting"?)
/and/ a value parameter; Tegra's pull strength definition for example
is:
enum tegra_pull_strength {
TEGRA_PULL_0 = 0,
TEGRA_PULL_1,
// ... (every value in between)
TEGRA_PULL_30,
TEGRA_PULL_31,
TEGRA_MAX_PULL,
};
And it seems better to represent that as ("pull", 0) ... ("pull", 31) than
"pull0" .. "pull31" such that the value needs to be parsed out of the "mode"
string somehow by the driver.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/2] drivers: create a pin control subsystem v7
Linus Walleij wrote at Tuesday, September 27, 2011 1:51 AM:
> On Wed, Sep 21, 2011 at 9:45 PM, Stephen Warren wrote:
> > Linus Walleij wrote at Wednesday, September 21, 2011 3:17 AM:
> >> To abstract things the stuff we can do with the group should be
> >> something enumerated too. So:
> >>
> >> pinctrl_config_group(const char *pinctrl_device, const char *group,
> >> const char *mode);
> >> pinctrl_config_pin(const char *pinctrl_device, int pin, const char *mode);
> >>
> >> So the driver need an API to enumerate pin and group modes.
> >>
> >> I might want to save this thing for post-merge of the basic API and
> >> pinmux stuff though so we don't try to push too much upfront
> >> design at once.
> >
> > Yes, adding in a replacement _config API can certainly be a later patch.
>
> If I'm efficient enough we might get that add-on before the v3.2
> merge window, I'll iterate that patch separately though.
>
> > One comment on the API above: I think you want "mode" (or "setting"?)
> > /and/ a value parameter; Tegra's pull strength definition for example
> > is:
> >
> > enum tegra_pull_strength {
> > TEGRA_PULL_0 = 0,
> > TEGRA_PULL_1,
> > // ... (every value in between)
> > TEGRA_PULL_30,
> > TEGRA_PULL_31,
> > TEGRA_MAX_PULL,
> > };
> >
> > And it seems better to represent that as ("pull", 0) ... ("pull", 31) than
> > "pull0" .. "pull31" such that the value needs to be parsed out of the "mode"
> > string somehow by the driver.
>
> OK so we might want some public defintion of "things you can do"
> with pins, then an opaque parameter.
>
> like:
>
> #define PINCTRL_PULL "pull"
> #define PINCTRL_BIAS "bias"
> #define PINCTRL_LOADCAP "load-capacitance"
> #define PINCTRL_DRIVE "drive"
Yes although Tegra has quite a few more weird options than that; I'm not
sure exactly how many of the options that Tegra supports would be useful
for other SoCs, or what the best way to represent all the device-specific
knobs is. I guess we need another survey of a bunch of SoCs like you did
when designing the pinmux API.
> ...but then it's just simple enumerators, and we might be better off
> with a simple enum after all:
>
> enum pinctrl_pin_ops {
> PINCTRL_PULL,
> PINCTRL_BIAS,
> PINCTRL_LOADCAP,
> PINCTRL_DRIVE,
> }
>
> Surely the device tree will have to translate that enum into strings
> (I guess? Sorry for my low DT competence) but that is more of a
> DT pecularity and can be kept in the DT pin control parser?
DT as a data format can handle strings or integers just fine. However,
there's currently no support for named constants (i.e. #defines) in the
device-tree source format, so strings end up being a lot more self-
documenting. Whatever the DT format ends up being, we can certainly
map from whatever-dt-contains to whatever-Linux-APIs-need inside
whatever kernel function is written to extract the data from DT.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Tuesday, September 27, 2011 2:01 AM:
> On Wed, Sep 21, 2011 at 9:39 PM, Stephen Warren wrote:
> > Linus Walleij wrote at Wednesday, September 21, 2011 2:25 AM:
> >> On Wed, Sep 21, 2011 at 12:15 AM, Stephen Warren
> >> wrote:
> >> > Linus Walleij wrote at Friday, September 16, 2011 6:14 AM:
> >> >> + for (i = 0; i < ARRAY_SIZE(u300_mux_hogs); i++) {
> >> >> + struct pinmux *pmx;
> >> >> + int ret;
> >> >> +
> >> >> + pmx = pinmux_get(u300_mux_hogs[i].dev, NULL);
> >> >> + if (IS_ERR(pmx)) {
> >> >> + pr_err("u300: could not get pinmux hog %s\n",
> >> >> + u300_mux_hogs[i].name);
> >> >> + continue;
> >> >> + }
> >> >> + ret = pinmux_enable(pmx);
> >> >> + if (ret) {
> >> >> + pr_err("u300: could enable pinmux hog %s\n",
> >> >> + u300_mux_hogs[i].name);
> >> >> + continue;
> >> >> + }
> >> >> + u300_mux_hogs[i].pmx = pmx;
> >> >> + }
> >> >> + return 0;
> >> >> +}
> >> >> +subsys_initcall(u300_pinmux_fetch);
> >> >
> >> > Why not just have the pinmux core support hogging on non-"system" mapping
> >> > entries; then everything I quoted above except u300_pinmux_map[] could
> >> > be deleted, and the "hog" flag set on the last 3 u300_pinmux_map[]
> >> > entries.
> >>
> >> Very good question, luckily I have a good answer.
> >>
> >> There is no way for the pinmux core to traverse the system and look
> >> up the apropriate struct device * pointers.
> >
> > It's unclear to me why that would be needed.
> >
> > For non-system mappings, either the device-driver in question will be
> > get'ing and enabling them (this case isn't relevant to this discussion),
> > or they'll be hogged. In the hog case, I doubt anything would ever want
> > to disable them and switch the device to a different mapping entry; if
> > that were the case, the entries shouldn't be hogged, but get/enabled by
> > the device anyway. Hence, I don't see the need for an activation of a
> > hog'd non-system mapping entry to care about the device fields that are
> > in the mapping table at all; they could just be ignored couldn't they?
>
> I don't think so, atleast not in this case. I think it is helpful for the
> system
> to know which device is related to the specific hog, so it turns up in
> debugfs etc "aha, it is hogged for that device" etc. Not doing that
> makes the subsystem loose relevant information.
>
> So for that reason I prefer to pass in the relevant struct device *
> pointers, atleast when they're readily available as in this boardfile.
>
> If they are not available, or hard to get at, we could just redefine
> them as system mappings and remove the device fields, but
> in this case I'd say let's keep it around.
Yes, it makes perfect sense to show this in debugfs.
But can't debugfs just get its information from the device name field in
the mapping table? I'm not sure why the need to use that information for
debugfs prevents the having entries with both the hog flag set and a
device name set?
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2 v7] pinmux: add a driver for the U300 pinmux
Linus Walleij wrote at Wednesday, September 28, 2011 5:59 AM: > On Wed, Sep 28, 2011 at 2:15 AM, Stephen Warren wrote: > > > But can't debugfs just get its information from the device name field in > > the mapping table? I'm not sure why the need to use that information for > > debugfs prevents the having entries with both the hog flag set and a > > device name set? > > That feels unsafe - then you have some string claiming to represent the > device for that pinmux, but the system has not matched that string > against dev_name() for any real device, so it could be a lie, any string > like "foo" would do. > > What I do now for U300 is complete resolution by matching dev_name > in the mapping to dev_name(device) on the struct device * actually > instantiated, which means you're 100% sure it is really mapped > to that very device. > > I like this since it's much more stringent... OK, I can see the advantage, and I suppose one could still move u300_pinmux_fetch() into the pinmux core as a helper function in the future if needed without too much issue. If it's hard for some other machine to set up the .dev entries in the table the function uses, one can always fall back on system/non-device hog entries in the main mapping table. So I'll drop my objection to this. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH] drivers: create a pin control subsystem v8
Linus Walleij wrote at Wednesday, September 28, 2011 6:04 AM: > This creates a subsystem for handling of pin control devices. > These are devices that control different aspects of package > pins. I still haven't had a chance to look through the .c files, just the docs and headers, but they look good to me now, so: Acked-by: Stephen Warren -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
pinctrl_config APIs, and other pinmux questions
Linus, (and other people interested in pinmux!)
I've started to look at porting the Tegra pinmux driver to your pinctrl
subsystem. In order to replace the existing pinmux driver, I need a way
to support configuring options such as tri-state, pull-up/down, drive-
strength, etc.
At this point, I have a couple of options:
1) Initialize all those parameters (and indeed the initial system-level
mux configuration) using custom code in the Tegra pinmux driver, and just
use the pin controller subsystem APIs for dynamic pin-muxing. As you
recall, I did post some patches to the Tegra pinmux driver to do this,
but I'm hoping to do (2) instead.
2) Enhance the pin controller subsystem to support configuring these
properties.
The following is some issues that need to be thought about if we do (2):
Issue 1: Pinctrl core -> driver interface
I propose adding the following to pinctrl_ops:
int (*pin_config)(unsigned pin, u32 param, u32 data);
int (*group_config)(unsigned group, u32 param, u32 data);
Where "param" is some driver-specific parameter, such as:
#define TEGRA_PINCTRL_PARAM_TRISTATE_ENABLE0
#define TEGRA_PINCTRL_PARAM_PULL_TYPE 1
#define TEGRA_PINCTRL_PARAM_SCHMITT_ENABLE 2
#define TEGRA_PINCTRL_PARAM_HIGH_SPEED_ENABLE 3
#define TEGRA_PINCTRL_PARAM_DRIVE_STRENGTH 4
#define TEGRA_PINCTRL_PARAM_PULL_UP_STRENGTH 5
#define TEGRA_PINCTRL_PARAM_PULL_DOWN_STRENGTH 6
#define TEGRA_PINCTRL_PARAM_SLEW_RATE_RISING 7
#define TEGRA_PINCTRL_PARAM_SLEW_RATE_FALLING 8
Rationale:
Param being SoC-specific:
I looked at a bunch of existing pinmux drivers in the mainline kernel.
There are certainly some common concepts between SoCs. However, the
details are often different enough that I don't think attempting to
unify the set of configuration options will be that successful; I
doubt we could create a generic enough abstraction of these settings
for a cross-SoC driver to be able to usefully name and select values
for all the different pin/group options.
Data being a plain u32 rather than a pointer as your v7 patchset had it:
Using a pointer does allow passing arbitrary data into the driver, which
seems useful. However, this also makes it much more difficult for the
core pin controller API to handle the values, e.g. in the mapping table,
in a device-tree representation thereof, etc.
Having both config_pin and config_group functions:
Tegra at least has settings that are applicable to groups. Most (all??)
other SoCs apply configurations to individual pins. I think we need
separate functions to make this explicit, so the function knows if it's
looking up selector as a pin name (ID) or a group name (ID).
Issue 2: Do we need a new "pin settings" table passed from the board to
the pin controller core?
Issue 3: Do we need to change "pin settings" on-the-fly, or only at boot?
These issues are closely related.
If we only care about statically configuring pins at boot time, then
pinmux_register_mappings() could be enhanced to accept two tables; the
existing one for the mux settings, and a second one that's essentially
just a list of pin/group_config calls to make at pinmux init time.
However, if we ever want things to change dynamically (say, suspend/
resume), or only be activated when a device initializes (just like the
mux settings), we need to enhance struct pinmux_map to contain either
mux settings or config settings; perhaps rework it as follows:
enum pinmux_map_type {
MUX,
PIN_CONFIG,
GROUP_CONFIG
};
struct pinmux_map {
const char *name;
struct device *ctrl_dev;
const char *ctrl_dev_name;
enum pinmux_map_type type;
union {
struct {
const char *function;
const char *group;
} mux;
struct {
u32 param;
u32 data;
} config;
} data;
struct device *dev;
const char *dev_name;
const bool hog_on_boot;
};
Such that pinmux_enable would both activate all the mux settings, and
also call pin/group_config based on the mapping table.
Issue 4: Device-tree
I'd like to enhance the pinmux core to allow the mapping table to be read
from device tree. If we end up with a separate "pin configuration" table,
the same applies there. Do you have any thoughts here, or should I just go
ahead and create/propose something? I'd probably base this partially on my
previous patches that do this within the Tegra pinmux driver itself.
Issue 5: Suspend/resume, and other state changes
First, some HW background on Tegra, which I imagine applies to any SoC
capable of muxing a single "function" onto different sets of pins:
Lets take Tegra's first I2C controller. It can be mux'd onto pingroups
"I2CP" or "RM". Given the register definitions (for each pingroup, there's
a "function select" register field), it's possible to tell the HW to mux
that I2C function onto *both* pingroups at once. However, we must avoid
this, because th
RE: pinctrl_config APIs, and other pinmux questions
Shawn Guo wrote at Friday, October 14, 2011 8:59 AM:
> It might be a good place for me to catch up the pinctrl subsystem
> discussion, as far as imx migration concerned.
>
> I have not read the backlog of all the previous discussion, so please
> excuse me if something I put here have been discussed.
>
> On Thu, Oct 13, 2011 at 01:59:55PM -0700, Stephen Warren wrote:
> > Linus, (and other people interested in pinmux!)
> >
> > I've started to look at porting the Tegra pinmux driver to your pinctrl
> > subsystem. In order to replace the existing pinmux driver, I need a way
> > to support configuring options such as tri-state, pull-up/down, drive-
> > strength, etc.
...
> > 2) Enhance the pin controller subsystem to support configuring these
> > properties.
>
> Yeah. I remember Linus.W originally named the subsystem pinmux and
> changed it to pinctrl later for that he wants to cover not only pinmux
> but also pin/pad configuration with this subsystem.
AIUI, the pin control subsystem is intended to encompass both pin muxing
and pin configuration, it's just that the pin configuration part isn't
fleshed out yet, and will be via a separate ops structure that the overall
driver can provide.
...
> > I propose adding the following to pinctrl_ops:
> >
> > int (*pin_config)(unsigned pin, u32 param, u32 data);
> > int (*group_config)(unsigned group, u32 param, u32 data);
...
> > Having both config_pin and config_group functions:
> >
> > Tegra at least has settings that are applicable to groups. Most (all??)
> > other SoCs apply configurations to individual pins. I think we need
> > separate functions to make this explicit, so the function knows if it's
> > looking up selector as a pin name (ID) or a group name (ID).
> >
> The "group" defined in this subsystem does not necessarily require
> multiple pins be controlled by single register at hardware level.
> A group of pins muxed for given function can be controlled by hardware
> in the way of individual pin. Similar to pinmux, I guess that the
> interface between pincfg and core can take group only, and leave the
> mapping between group and pins to the driver. Single-pin function can
> be a particular case of group-pins.
That's true; we could define a group for each pin, which no function uses
as a legal group/position, but does allow config() to be used.
> > Issue 2: Do we need a new "pin settings" table passed from the board to
> > the pin controller core?
> >
> > Issue 3: Do we need to change "pin settings" on-the-fly, or only at boot?
...
> > However, if we ever want things to change dynamically (say, suspend/
> > resume), or only be activated when a device initializes (just like the
> > mux settings), we need to enhance struct pinmux_map to contain either
> > mux settings or config settings; perhaps rework it as follows:
>
> I would think that we need to enhance pinmux_map to contain both mux
> *and* config settings. To me, the mapping seems to be a group of pins
> with specific mux and config settings for a given function.
>
> > enum pinmux_map_type {
> > MUX,
> > PIN_CONFIG,
> > GROUP_CONFIG
> > };
> >
> > struct pinmux_map {
> > const char *name;
> > struct device *ctrl_dev;
> > const char *ctrl_dev_name;
> > enum pinmux_map_type type;
> > union {
> > struct {
> > const char *function;
> > const char *group;
> > } mux;
> > struct {
> > u32 param;
> > u32 data;
> > } config;
> > } data;
> > struct device *dev;
> > const char *dev_name;
> > const bool hog_on_boot;
> > };
>
> If the config has limited number of possible settings for given
> function, we can probably enumerate it in pinctrl driver just like
> what we are doing with pinmux option, and add parameter "config" to
> pinmux_map to select pincfg option for given mapping.
>
> @@ -46,6 +46,7 @@ struct pinmux_map {
> const char *ctrl_dev_name;
> const char *function;
> const char *group;
> + const char *config;
> struct device *dev;
> const char *dev_name;
> const bool hog_on_boot;
>
> > Such that pinmux_enable would both activate all the mux settings, and
> > also call pin/group_config based on the mapping table.
Having the driver expose a list of all possible combinations of pin
configurations seems impractical, for the same reason as I objected to
the
RE: pinctrl_config APIs, and other pinmux questions
Shawn Guo wrote at Friday, October 14, 2011 9:12 PM: > On Fri, Oct 14, 2011 at 08:53:33AM -0700, Stephen Warren wrote: ... > > Having the driver expose a list of all possible combinations of pin > > configurations seems impractical, for the same reason as I objected to > > the original proposal for how the driver listed functions; there are > > simply far too many pin config parameters and legal value to list the > > entire set of combinations. > > > I did not mean to list the entire set of combinations. For given > function, the applicable number of config should be very limited. > For most cases, it could be just one. For imx6q usdhc example, it's > 3, for 50M, 100M and 200M SD bus clock cases. Shawn, Are you talking about entries in the (board-specific) mapping table, which I agree would contain the useful subset of combinations of options, or a list of possible settings exposed by the SoC driver, which would have to expose all possibilities, or they wouldn't be available for the mapping table to select from? In the case of "a list of possible settings exposed by the SoC driver", * If such a list (of combinations) were to exist, I think it'd need to include all combinations (the cross-product of all individual config params) in general. * I can certainly imagine that for some SoCs, or for a particular device on a SoC, or for a particular board, only a subset of those would be useful, and hence a limited set would be useful. However, that selection is up to the board mapping table not the SoC driver in general. * In Tegra's case at least, I think a number of the numeric values (e.g. pull strength with range 0..31) may be for board calibration, and besides that, most combinations of param values would be useful in some case, and hence we'd always have to expose everything, in order to allow the board mapping table to be able to pick anything the designer needed. * As such, I think the SoC driver should at most list the legal range for each param individually, and let the board-specific mapping table choose the combination(s) required. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: pinctrl_config APIs, and other pinmux questions
Linus Walleij wrote at Monday, October 17, 2011 9:03 AM:
> Thanks for a long letter, took som time to read, but as usual it
> contains good stuff!
>
> I think you're talking about two things here:
>
> 1) The need to configure per-pin "stuff" like biasing, driving,
> load capacitance ... whatever
>
> 2) The need to handle state transitions of pinmux settings.
Yes, that's true.
> I prefer that we try to keep these two separate and not conflate
> them too much.
>
> On Thu, Oct 13, 2011 at 10:59 PM, Stephen Warren wrote:
>
> > 2) Enhance the pin controller subsystem to support configuring these
> > properties.
>
> This is definately what we want to do. That is why the subsystem was
> renamed from pinmux to pinctrl in the first place, and also the rationale
> for introducing the abstract pin groups.
>
> > int (*pin_config)(unsigned pin, u32 param, u32 data);
> > int (*group_config)(unsigned group, u32 param, u32 data);
>
> Do you mean
> int (*group_config)(const char *group, u32 param, u32 data);
>
> On the latter one? We identify groups by name mainly.
> The selectors is an internal thing between pinctrl core and
> drivers.
That was a proposal for the core->driver API, so I think using an int
instead of a string makes sense?
> > Where "param" is some driver-specific parameter, such as:
> >
> > #define TEGRA_PINCTRL_PARAM_TRISTATE_ENABLE 0
> (...)
> > I looked at a bunch of existing pinmux drivers in the mainline kernel.
> > There are certainly some common concepts between SoCs. However, the
> > details are often different enough that I don't think attempting to
> > unify the set of configuration options will be that successful; I
> > doubt we could create a generic enough abstraction of these settings
> > for a cross-SoC driver to be able to usefully name and select values
> > for all the different pin/group options.
>
> I don't know, I could easily say the same thing about say input devices:
> all of them are essentially different, still they opted to create the file
> with this kind of stuff:
>
> ...
> #define KEY_COMMA 51
> #define KEY_DOT 52
> #define KEY_SLASH 53
> #define KEY_RIGHTSHIFT 54
> #define KEY_KPASTERISK 55
> ...
>
> And it has proven to be very useful. I'd rather opt to just remove
> the TEGRA_ prefix from all the above, and that we try to create some
> common sematics to these calls.
Hmm. Keys seem to have a bit more uniform behavior than the various
pin configuration data. Still, I guess I'm fine defining some unified
numbering space for all the parameters, so long as we can add Soc-specific
entries to the list when they don't match anything from other SoCs.
> > Data being a plain u32 rather than a pointer as your v7 patchset had it:
>
> Actually it looked like this:
>
> static inline int pinmux_config(struct pinmux *pmx, u16 param,
>unsigned long *data)
>
> That was supposed to be an unsigned long (not a *pointer), which
> is exchangeable to pointer, as per the example known from
> interrupt handlers. It can also be used to pass a plain argument.
> It was designed to be "ioctl()-like".
>
> So I prefer we say:
> int (*pin_config)(unsigned pin, u32 param, unsigned long data);
> int (*group_config)(const char *group, u32 param, unsigned long data);
For the data function parameter, that's the same as my proposal, except
for "unsigned long" rather than "u32".
...
> > Now let's suppose that we've enhanced the mapping table to support pin/
> > group_config values, and that a driver is written to expect a pinmux
> > mapping table with the following mappings:
> >
> > "active"
> > "suspend"
>
> I'm not following why this should be different mappings.
>
> I would rather contrast it with the similar concept from the
> regulator framework, these have modes like these:
>
> enum regulator_status {
> REGULATOR_STATUS_OFF,
> REGULATOR_STATUS_ON,
> REGULATOR_STATUS_ERROR,
> /* fast/normal/idle/standby are flavors of "on" */
> REGULATOR_STATUS_FAST,
> REGULATOR_STATUS_NORMAL,
> REGULATOR_STATUS_IDLE,
> REGULATOR_STATUS_STANDBY,
> };
>
> So I think we should have pin group states in a similar
> manner:
>
> enum pinmux_state {
> PINMUX_STATE_DEFAULT, /* == active */
> PINMUX_STATE_LOWPOWER,
> };
>
> And associated calls:
>
> pinmux_set_state(const char *group, enum pingroup_state state);
I don&#
RE: [PATCH 1/2] pinctrl: move group lookup to core
Linus Walleij wrote at Wednesday, October 19, 2011 10:21 AM: > Now also the core needs to look up pin groups so move the lookup > function there and expose it in the internal header. > > Signed-off-by: Linus Walleij Acked-by: Stephen Warren -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 3/4] pinctrl: Don't copy pin names when registering them
A pin controller's names array is no longer marked __refdata. Hence, we
can avoid copying a pin's name into the descriptor when registering it.
Instead, just point at the string supplied in the pin array.
This both simplifies and speeds up pin controller initialization, but
also removes the hard-coded maximum pin name length.
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.c |5 ++---
drivers/pinctrl/core.h |2 +-
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 97a4eeb..15d7e5a 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -168,9 +168,8 @@ static int pinctrl_register_one_pin(struct pinctrl_dev
*pctldev,
/* Set owner */
pindesc->pctldev = pctldev;
- /* Copy optional basic pin info */
- if (name)
- strlcpy(pindesc->name, name, sizeof(pindesc->name));
+ /* Copy basic pin info */
+ pindesc->name = name;
spin_lock(&pctldev->pin_desc_tree_lock);
radix_tree_insert(&pctldev->pin_desc_tree, number, pindesc);
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index 698221e..a493ba6 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -55,7 +55,7 @@ struct pinctrl_dev {
*/
struct pin_desc {
struct pinctrl_dev *pctldev;
- charname[16];
+ const char *name;
spinlock_t lock;
/* These fields only added when supporting pinmux drivers */
#ifdef CONFIG_PINMUX
--
1.7.0.4
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 4/4] pinctrl: Don't copy function name when requesting a pin
Instead, store a pointer to the currently assigned function.
This allows us to delete the mux_requested variable from pin_desc; a pin
is requested if its currently assigned function is non-NULL.
When a pin is requested as a GPIO rather than a regular function, the
assigned function name is dynamically constructed. In this case, we have
to kstrdup() the dynamically constructed name, so that mux_function doesn't
pointed at stack data. This requires pin_free to be told whether to free
the mux_function pointer or not.
This removes the hard-coded maximum function name length.
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.h |3 +--
drivers/pinctrl/pinmux.c | 36 +++-
2 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/pinctrl/core.h b/drivers/pinctrl/core.h
index a493ba6..783c075 100644
--- a/drivers/pinctrl/core.h
+++ b/drivers/pinctrl/core.h
@@ -59,8 +59,7 @@ struct pin_desc {
spinlock_t lock;
/* These fields only added when supporting pinmux drivers */
#ifdef CONFIG_PINMUX
- boolmux_requested;
- charmux_function[16];
+ const char *mux_function;
#endif
};
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 4679ae6..f139609 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -130,14 +130,13 @@ static int pin_request(struct pinctrl_dev *pctldev,
}
spin_lock(&desc->lock);
- if (desc->mux_requested) {
+ if (desc->mux_function) {
spin_unlock(&desc->lock);
dev_err(&pctldev->dev,
"pin already requested\n");
goto out;
}
- desc->mux_requested = true;
- strncpy(desc->mux_function, function, sizeof(desc->mux_function));
+ desc->mux_function = function;
spin_unlock(&desc->lock);
/* Let each pin increase references to this module */
@@ -168,8 +167,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
out_free_pin:
if (status) {
spin_lock(&desc->lock);
- desc->mux_requested = false;
- desc->mux_function[0] = '\0';
+ desc->mux_function = NULL;
spin_unlock(&desc->lock);
}
out:
@@ -184,8 +182,9 @@ out:
* pin_free() - release a single muxed in pin so something else can be muxed
* @pctldev: pin controller device handling this pin
* @pin: the pin to free
+ * @free_func: whether to free the pin's assigned function name string
*/
-static void pin_free(struct pinctrl_dev *pctldev, int pin)
+static void pin_free(struct pinctrl_dev *pctldev, int pin, int free_func)
{
const struct pinmux_ops *ops = pctldev->desc->pmxops;
struct pin_desc *desc;
@@ -201,8 +200,9 @@ static void pin_free(struct pinctrl_dev *pctldev, int pin)
ops->free(pctldev, pin);
spin_lock(&desc->lock);
- desc->mux_requested = false;
- desc->mux_function[0] = '\0';
+ if (free_func)
+ kfree(desc->mux_function);
+ desc->mux_function = NULL;
spin_unlock(&desc->lock);
module_put(pctldev->owner);
}
@@ -214,6 +214,7 @@ static void pin_free(struct pinctrl_dev *pctldev, int pin)
int pinmux_request_gpio(unsigned gpio)
{
char gpiostr[16];
+ const char *function;
struct pinctrl_dev *pctldev;
struct pinctrl_gpio_range *range;
int ret;
@@ -229,7 +230,15 @@ int pinmux_request_gpio(unsigned gpio)
/* Conjure some name stating what chip and pin this is taken by */
snprintf(gpiostr, 15, "%s:%d", range->name, gpio);
- return pin_request(pctldev, pin, gpiostr, true, range);
+ function = kstrdup(gpiostr, GFP_KERNEL);
+ if (!function)
+ return -EINVAL;
+
+ ret = pin_request(pctldev, pin, function, true, range);
+ if (ret < 0)
+ kfree(function);
+
+ return ret;
}
EXPORT_SYMBOL_GPL(pinmux_request_gpio);
@@ -251,7 +260,7 @@ void pinmux_free_gpio(unsigned gpio)
/* Convert to the pin controllers number space */
pin = gpio - range->base;
- pin_free(pctldev, pin);
+ pin_free(pctldev, pin, true);
}
EXPORT_SYMBOL_GPL(pinmux_free_gpio);
@@ -351,7 +360,7 @@ static int acquire_pins(struct pinctrl_dev *pctldev,
/* On error release all taken pins */
i--; /* this pin just failed */
for (; i >= 0; i--)
- pin_free(pctldev, pins[i]);
+ pin_free(pctldev, pins[i], false);
return -ENODEV;
}
}
@@ -381,7 +390,7 @@ static void release_pins(struct pinctrl_dev *pctldev,
return;
}
[PATCH 1/4] pinctrl: get_group_pins() const fixes
get_group_pins() "returns" a pointer to an array of const objects, through
a pointer parameter. Fix the prototype so what's pointed at by the returned
pointer is const, rather than the function parameter being const.
This also allows the removal of a cast in each of the two current pinmux
drivers.
Signed-off-by: Stephen Warren
---
drivers/pinctrl/core.c |2 +-
drivers/pinctrl/pinmux-sirf.c |6 +++---
drivers/pinctrl/pinmux-u300.c |6 +++---
drivers/pinctrl/pinmux.c|4 ++--
include/linux/pinctrl/pinctrl.h |4 ++--
5 files changed, 11 insertions(+), 11 deletions(-)
diff --git a/drivers/pinctrl/core.c b/drivers/pinctrl/core.c
index 623afbd..97a4eeb 100644
--- a/drivers/pinctrl/core.c
+++ b/drivers/pinctrl/core.c
@@ -332,7 +332,7 @@ static int pinctrl_groups_show(struct seq_file *s, void
*what)
seq_puts(s, "registered pin groups:\n");
while (ops->list_groups(pctldev, selector) >= 0) {
- unsigned *pins;
+ const unsigned *pins;
unsigned num_pins;
const char *gname = ops->get_group_name(pctldev, selector);
int ret;
diff --git a/drivers/pinctrl/pinmux-sirf.c b/drivers/pinctrl/pinmux-sirf.c
index de5c78a..cad4f5d 100644
--- a/drivers/pinctrl/pinmux-sirf.c
+++ b/drivers/pinctrl/pinmux-sirf.c
@@ -869,12 +869,12 @@ static const char *sirfsoc_get_group_name(struct
pinctrl_dev *pctldev,
}
static int sirfsoc_get_group_pins(struct pinctrl_dev *pctldev, unsigned
selector,
- unsigned ** const pins,
- unsigned * const num_pins)
+ const unsigned **pins,
+ const unsigned *num_pins)
{
if (selector >= ARRAY_SIZE(sirfsoc_pin_groups))
return -EINVAL;
- *pins = (unsigned *) sirfsoc_pin_groups[selector].pins;
+ *pins = sirfsoc_pin_groups[selector].pins;
*num_pins = sirfsoc_pin_groups[selector].num_pins;
return 0;
}
diff --git a/drivers/pinctrl/pinmux-u300.c b/drivers/pinctrl/pinmux-u300.c
index 23f082f..be6e04d 100644
--- a/drivers/pinctrl/pinmux-u300.c
+++ b/drivers/pinctrl/pinmux-u300.c
@@ -849,12 +849,12 @@ static const char *u300_get_group_name(struct pinctrl_dev
*pctldev,
}
static int u300_get_group_pins(struct pinctrl_dev *pctldev, unsigned selector,
- unsigned ** const pins,
- unsigned * const num_pins)
+ const unsigned **pins,
+ unsigned *num_pins)
{
if (selector >= ARRAY_SIZE(u300_pin_groups))
return -EINVAL;
- *pins = (unsigned *) u300_pin_groups[selector].pins;
+ *pins = u300_pin_groups[selector].pins;
*num_pins = u300_pin_groups[selector].num_pins;
return 0;
}
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index 08e7b0f..4679ae6 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -326,7 +326,7 @@ static int acquire_pins(struct pinctrl_dev *pctldev,
const struct pinmux_ops *pmxops = pctldev->desc->pmxops;
const char *func = pmxops->get_function_name(pctldev,
func_selector);
- unsigned *pins;
+ const unsigned *pins;
unsigned num_pins;
int ret;
int i;
@@ -367,7 +367,7 @@ static void release_pins(struct pinctrl_dev *pctldev,
unsigned group_selector)
{
const struct pinctrl_ops *pctlops = pctldev->desc->pctlops;
- unsigned *pins;
+ const unsigned *pins;
unsigned num_pins;
int ret;
int i;
diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h
index 1e6f67e..be9f537 100644
--- a/include/linux/pinctrl/pinctrl.h
+++ b/include/linux/pinctrl/pinctrl.h
@@ -75,8 +75,8 @@ struct pinctrl_ops {
unsigned selector);
int (*get_group_pins) (struct pinctrl_dev *pctldev,
unsigned selector,
- unsigned ** const pins,
- unsigned * const num_pins);
+ const unsigned **pins,
+ unsigned *num_pins);
void (*pin_dbg_show) (struct pinctrl_dev *pctldev, struct seq_file *s,
unsigned offset);
};
--
1.7.0.4
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 2/4] pinctrl: Remove unsafe __refdata
A pin controller's pin definitions are used both during pinctrl_register()
and pinctrl_unregister(). The latter happens outside of __init/__devinit
time, and hence it is unsafe to mark the pin array as __refdata.
Signed-off-by: Stephen Warren
---
drivers/pinctrl/pinmux-sirf.c |2 +-
drivers/pinctrl/pinmux-u300.c |2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/pinctrl/pinmux-sirf.c b/drivers/pinctrl/pinmux-sirf.c
index cad4f5d..3bc083c 100644
--- a/drivers/pinctrl/pinmux-sirf.c
+++ b/drivers/pinctrl/pinmux-sirf.c
@@ -30,7 +30,7 @@
* pad list for the pinmux subsystem
* refer to CS-131858-DC-6A.xls
*/
-static const struct pinctrl_pin_desc __refdata sirfsoc_pads[] = {
+static const struct pinctrl_pin_desc sirfsoc_pads[] = {
PINCTRL_PIN(4, "pwm0"),
PINCTRL_PIN(5, "pwm1"),
PINCTRL_PIN(6, "pwm2"),
diff --git a/drivers/pinctrl/pinmux-u300.c b/drivers/pinctrl/pinmux-u300.c
index be6e04d..ed8bcd8 100644
--- a/drivers/pinctrl/pinmux-u300.c
+++ b/drivers/pinctrl/pinmux-u300.c
@@ -179,7 +179,7 @@
#define U300_NUM_PADS 467
/* Pad names for the pinmux subsystem */
-static const struct pinctrl_pin_desc __refdata u300_pads[] = {
+static const struct pinctrl_pin_desc u300_pads[] = {
/* Pads along the top edge of the chip */
PINCTRL_PIN(0, "P PAD VDD 28"),
PINCTRL_PIN(1, "P PAD GND 28"),
--
1.7.0.4
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2] pinctrl: add a generic control interface
Linus Walleij wrote at Wednesday, October 19, 2011 10:21 AM: > This add per-pin and per-group pin control interfaces for biasing, > driving and other such electronic properties. The intention is > clearly to enumerate all things you can do with pins, hoping that > these are enumerable. > > Signed-off-by: Linus Walleij > diff --git a/include/linux/pinctrl/pinctrl.h b/include/linux/pinctrl/pinctrl.h > +/** > + * enum pin_config_param - possible pin configuration parameters > + * @PIN_CONFIG_BIAS_UNKNOWN: this bias mode is not known to us I'm not sure what use that would be; shouldn't we remove that, and add new PIN_CONFIG_BIAS_* values as/when they're needed? > + * @PIN_CONFIG_BIAS_FLOAT: no specific bias, the pin will float or state > + * is not controlled by software "float" and "not controlled by SW" are very different things. How is float different to high impedance? > + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high impedance > + * mode, also know as "third-state" (tristate) or "high-Z" or "floating". > + * On output pins this effectively disconnects the pin, which is useful > + * if for example some other pin is going to drive the signal connected > + * to it for a while. Pins used for input are usually always high > + * impedance. > + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high > + * impedance to VDD), if the controller supports specifying a certain > + * pull-up resistance, this is given as an argument (in Ohms) when > + * setting this parameter What value should be used to disable a pull-up; 0? What value should be used when requesting pull-up where the SoC doesn't have a configurable pull-up resistor; anything non-zero? Tegra has separate configurations for pull-up/down strength (0..31) and pull-up/down enable (up/down/none), though I suppose we could map 0 to off, 1..32 to on with strength 0..31, although that'd prevent a driver from toggling the enable bit on/off without knowing what the strength code should be programmed to... Tegra's pull-up/down strengths aren't documented in terms of ohms, but rather a "drive strength code" on scale 0..31. I'm not sure what that truly maps to in hardware. > + * @PIN_CONFIG_BIAS_PULL_DOWN: the pin will be pulled down (usually with high > + * impedance to GROUND), if the controller supports specifying a certain > + * pull-down resistance, this is given as an argument (in Ohms) when > + * setting this parameter > + * @PIN_CONFIG_BIAS_HIGH: the pin will be wired high, connected to VDD > + * @PIN_CONFIG_BIAS_GROUND: the pin will be grounded, connected to GROUND At least some of these options appear mutually exclusive; I wonder if we shouldn't have a PIN_CONFIG_BIAS parameter, and have the legal values be HIGH/GROUND/NONE. Pull up/down are probably separate. HIGH_IMPEDANCE seems more like a PIN_CONFIG_DRIVE value than a PIN_CONFIG_BIAS value. > + * @PIN_CONFIG_DRIVE_UNKNOWN: we don't know the drive mode of this pin, for > + * example since it is controlled by hardware or the information is not > + * accessible but we need a meaningful enumerator in e.g. initialization > + * code Again, I'm not sure this is useful; if this is not a configurable parameter, surely initialization code would simply skip attempting to set this param? > + * @PIN_CONFIG_DRIVE_PUSH_PULL: the pin will be driven actively high and > + * low, this is the most typical case and is typically achieved with two > + * active transistors on the output. If the pin can support different > + * drive strengths for push/pull, the strength is given on a custom format > + * as argument when setting pins to this mode > + * @PIN_CONFIG_DRIVE_OPEN_DRAIN: the pin will be driven with open drain (open > + * collector) which means it is usually wired with other output ports > + * which are then pulled up with an external resistor. If the pin can > + * support different drive strengths for the open drain pin, the strength > + * is given on a custom format as argument when setting pins to this mode > + * @PIN_CONFIG_DRIVE_OPEN_SOURCE: the pin will be driven with open drain > + * (open emitter) which is the same as open drain mutatis mutandis but > + * pulled to ground. If the pin can support different drive strengths for > + * the open drain pin, the strength is given on a custom format as > + * argument when setting pins to this mode > + * @PIN_CONFIG_DRIVE_OFF: the pin is set to inactive drive mode, off These seem like mutually exclusive values for a PIN_CONFIG_DRIVE param. > + * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in > + * schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis, > + * the threshold value is given on a custom format as argument when > + * setting pins to this mode > + * @PIN_CONFIG_SLEW_RATE_RISING: this will configure the slew rate for rising > + * signals on the pin. The argument gives the rise time in nanoseconds > + * @PIN_CO
RE: [PATCH 2/2] pinctrl: add a generic control interface
Shawn Guo wrote at Wednesday, October 19, 2011 8:32 PM:
> On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
...
> > +int pin_config_group(struct pinctrl_dev *pctldev, const char *pin_group,
> > +enum pin_config_param param, unsigned long data)
...
> > +enum pin_config_param {
> > + PIN_CONFIG_BIAS_UNKNOWN,
> > + PIN_CONFIG_BIAS_FLOAT,
> > + PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> > + PIN_CONFIG_BIAS_PULL_UP,
> > + PIN_CONFIG_BIAS_PULL_DOWN,
> > + PIN_CONFIG_BIAS_HIGH,
> > + PIN_CONFIG_BIAS_GROUND,
> > + PIN_CONFIG_DRIVE_UNKNOWN,
> > + PIN_CONFIG_DRIVE_PUSH_PULL,
> > + PIN_CONFIG_DRIVE_OPEN_DRAIN,
> > + PIN_CONFIG_DRIVE_OPEN_SOURCE,
> > + PIN_CONFIG_DRIVE_OFF,
> > + PIN_CONFIG_INPUT_SCHMITT,
> > + PIN_CONFIG_SLEW_RATE_RISING,
> > + PIN_CONFIG_SLEW_RATE_FALLING,
> > + PIN_CONFIG_LOAD_CAPACITANCE,
> > + PIN_CONFIG_WAKEUP_ENABLE,
> > + PIN_CONFIG_END,
> > +};
>
> IMO, trying to enumerate all possible pin_config options is just to
> make everyone's life harder. Firstly, this enumeration is far from
> completion, for imx6 iomuxc example, we have 3 options for pull-up,
> 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> 60, 80, 120, and 240 Ohm. It's just so hard to make this enumeration
> complete. Secondly, defining this param as enum requires the API
> user to call the function multiple times to configure one pin. For
> example, if I want to configure pin_foo as slow-slew, open-drain,
> 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
>
> I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> to encode/decode this u32 for their pinctrl controller. It makes
> people's life much easier.
That's not quite what I meant.
I meant that I thought the types for param and value should be simple
integers, with meanings of the values defined by the individual drivers,
rather than a system-defined enum.
However, I wasn't envisaging packing multiple fields into the "value"
parameter; that would essentially be packing a struct into a u32 for
transport. I still figured that "param" would logically be an enum,
and represent a single modifiable parameter, and "data"/"value" would
be the single value of that one parameter.
Still, perhaps packing stuff is an option that makes sense in some cases,
depending on what API we end up with to manipulate the parameters, and
where the source of "data"/"value" is (encoded into client driver, or
from some hidden table passed to pinmux core by board, with the values
being passed directly to the pinmux drivers without the client drivers
seeing them)
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2] pinctrl: add a generic control interface
Linus Walleij wrote at Thursday, October 20, 2011 7:44 AM: > On Thu, Oct 20, 2011 at 4:45 AM, Shawn Guo wrote: > > > We should not require device driver to call these APIs directly. There > > are so many pinctrl subsystem internal details left to its users. > > As explained I already have drivers that need to do this. OK they > are out-of-tree, but they do exist. > > For U5500 we have a SIM card driver which sets pull down resistance > and load capacitance in response to information retrieved from the > card at runtime. This is not static or internal to pinctrl at all. How should > I solve this if pin configuration is not exposed? I think drivers should operate at a higher level than "set pin X's param Y to value Z". I imagine the set of states that the SIM card driver needs to set up are defined by the SIM protocol. So, the driver communicates with the SIM card, find out that it supports Certain clock frequencies or capabilities etc., and then needs to program the SIM card and associated SoC-side HW to operate in the selected mode. The set of legal modes for the SIM card protocol is presumably pretty small, so the SIM driver might want to request "basic" or "fast" operation (note: I have zero knowledge of SIM cars, so those mode names are bogus). Similarly, SD/MMC drivers might know about a mode name for each clock frequency (or range) that an SD card is allowed to support by spec. However, I still think it's a good idea for pinctrl drivers to expose each of their available configuration options with as little veneer or abstraction as possible. To bridge these two levels of abstraction, I think the pinctrl core needs to be provided with information or an API to map between them. I imagine this could be implemented as a table rather similar to the existing mux table, or even part of that mux table. The table would be at least partially board-specific (since a given SD instance might be mux'd onto a different set of pins between two boards, and the values for each param might be determined during board characterization), but for a given HW module (SIM, SD), at least some of the data might be the same across any board that use that function, irrespective of which set of pins is in use or independent of board characterization. So, we might have to think about how the board and SoC code (and/or DT board/SoC bindings) could interact to reduce duplication of common settings, but we should probably defer that discussion. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2] pinctrl: add a generic control interface
Linus Walleij wrote at Thursday, October 20, 2011 4:25 AM: > On Thu, Oct 20, 2011 at 1:04 AM, Stephen Warren wrote: ... > >> + * @PIN_CONFIG_BIAS_HIGH_IMPEDANCE: the pin will be set to a high > >> impedance > >> + * mode, also know as "third-state" (tristate) or "high-Z" or > >> "floating". > >> + * On output pins this effectively disconnects the pin, which is useful > >> + * if for example some other pin is going to drive the signal connected > >> + * to it for a while. Pins used for input are usually always high > >> + * impedance. > >> + * @PIN_CONFIG_BIAS_PULL_UP: the pin will be pulled up (usually with high > >> + * impedance to VDD), if the controller supports specifying a certain > >> + * pull-up resistance, this is given as an argument (in Ohms) when > >> + * setting this parameter > > > > What value should be used to disable a pull-up; 0? > > A semantic question would also be if pull up is implicitly disabled > if you issue PIN_CONFIG_BIAS_PULL_DOWN when you are > in PULL_UP state. > > I added PIN_CONFIG_BIAS_DISABLED to > set_pin_config(pin, PIN_CONFIG_BIAS_DISABLED); > > So we can transition to a state of totally disabled pin bias. I'm not too sure I like that; the core's definition of PIN_CONFIG_BIAS_* is then imposing semantics that the HW might not have. So, Tegra's pull configuration is up/down/none, as a register field with 3 values. Another chip could easily have 1 bit to pull-up-enable and a separate bit for pull-down-enable. It might be silly to set them both, but the HW could quite easily be designed such that it'd work as one would exect electrically. I'm not convinced that the PIN_CONFIG_BIAS_* definitions should be defined to force one model over the other. With SoC-defined param names, the pinctrl driver can expose exactly what the HW supports without abstraction. And how to hide the abstraction from drivers? Some kind of mapping table or API; see my other email for details. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH] pinctrl: Add explicit gpio_disable_free pinmux_op
Some pinctrl drivers (Tegra at least) program a pin to be a GPIO in a
completely different manner than they select which function to mux out of
that pin. In order to support a single "free" pinmux_op, the driver would
need to maintain a per-pin state of requested-for-gpio vs. requested-for-
function. However, that's a lot of work when the core already has explicit
separate paths for gpio request/free and function request/free.
So, add a gpio_disable_free op to struct pinmux_ops, and make pin_free()
call it when appropriate.
When doing this, I noticed that when calling pin_request():
!!gpio == (gpio_range != NULL)
... and so I collapsed those two parameters in both pin_request(), and
when adding writing the new code in pin_free().
Also, for pin_free():
!!free_func == (gpio_range != NULL)
However, I didn't want pin_free() to know about the GPIO function naming
special case, so instead, I reworked pin_free() to always return the pin's
previously requested function, and now pinmux_free_gpio() calls
kfree(function). This is much more balanced with the allocation having
been performed in pinmux_request_gpio().
Signed-off-by: Stephen Warren
---
drivers/pinctrl/pinmux.c | 39 +--
include/linux/pinctrl/pinmux.h |3 +++
2 files changed, 28 insertions(+), 14 deletions(-)
diff --git a/drivers/pinctrl/pinmux.c b/drivers/pinctrl/pinmux.c
index a5467f8..8a95e45 100644
--- a/drivers/pinctrl/pinmux.c
+++ b/drivers/pinctrl/pinmux.c
@@ -98,12 +98,11 @@ struct pinmux_hog {
* @function: a functional name to give to this pin, passed to the driver
* so it knows what function to mux in, e.g. the string "gpioNN"
* means that you want to mux in the pin for use as GPIO number NN
- * @gpio: if this request concerns a single GPIO pin
* @gpio_range: the range matching the GPIO pin if this is a request for a
* single GPIO pin
*/
static int pin_request(struct pinctrl_dev *pctldev,
- int pin, const char *function, bool gpio,
+ int pin, const char *function,
struct pinctrl_gpio_range *gpio_range)
{
struct pin_desc *desc;
@@ -152,7 +151,7 @@ static int pin_request(struct pinctrl_dev *pctldev,
* If there is no kind of request function for the pin we just assume
* we got it by default and proceed.
*/
- if (gpio && ops->gpio_request_enable)
+ if (gpio_range && ops->gpio_request_enable)
/* This requests and enables a single GPIO pin */
status = ops->gpio_request_enable(pctldev, gpio_range, pin);
else if (ops->request)
@@ -182,29 +181,39 @@ out:
* pin_free() - release a single muxed in pin so something else can be muxed
* @pctldev: pin controller device handling this pin
* @pin: the pin to free
- * @free_func: whether to free the pin's assigned function name string
+ * @gpio_range: the range matching the GPIO pin if this is a request for a
+ * single GPIO pin
*/
-static void pin_free(struct pinctrl_dev *pctldev, int pin, int free_func)
+static const char *pin_free(struct pinctrl_dev *pctldev, int pin,
+ struct pinctrl_gpio_range *gpio_range)
{
const struct pinmux_ops *ops = pctldev->desc->pmxops;
struct pin_desc *desc;
+ const char *func;
desc = pin_desc_get(pctldev, pin);
if (desc == NULL) {
dev_err(&pctldev->dev,
"pin is not registered so it cannot be freed\n");
- return;
+ return NULL;
}
- if (ops->free)
+ /*
+* If there is no kind of request function for the pin we just assume
+* we got it by default and proceed.
+*/
+ if (gpio_range && ops->gpio_disable_free)
+ ops->gpio_disable_free(pctldev, gpio_range, pin);
+ else if (ops->free)
ops->free(pctldev, pin);
spin_lock(&desc->lock);
- if (free_func)
- kfree(desc->mux_function);
+ func = desc->mux_function;
desc->mux_function = NULL;
spin_unlock(&desc->lock);
module_put(pctldev->owner);
+
+ return func;
}
/**
@@ -234,7 +243,7 @@ int pinmux_request_gpio(unsigned gpio)
if (!function)
return -EINVAL;
- ret = pin_request(pctldev, pin, function, true, range);
+ ret = pin_request(pctldev, pin, function, range);
if (ret < 0)
kfree(function);
@@ -252,6 +261,7 @@ void pinmux_free_gpio(unsigned gpio)
struct pinctrl_gpio_range *range;
int ret;
int pin;
+ const char *func;
ret = pinctrl_get_device_gpio_range(gpio, &pctldev, &range);
if (ret)
@@ -260,7 +270,8 @@ void pinmux_free_gpio(unsigned gpio)
RE: [PATCH 2/2] pinctrl: add a generic control interface
Shawn Guo wrote at Sunday, October 23, 2011 2:26 AM:
> On Thu, Oct 20, 2011 at 10:26:43AM -0700, Stephen Warren wrote:
> > Shawn Guo wrote at Wednesday, October 19, 2011 8:32 PM:
> > > On Wed, Oct 19, 2011 at 06:21:14PM +0200, Linus Walleij wrote:
> > ...
> > > > +int pin_config_group(struct pinctrl_dev *pctldev, const char
> > > > *pin_group,
> > > > +enum pin_config_param param, unsigned long data)
> > ...
> > > > +enum pin_config_param {
> > > > + PIN_CONFIG_BIAS_UNKNOWN,
> > > > + PIN_CONFIG_BIAS_FLOAT,
> > > > + PIN_CONFIG_BIAS_HIGH_IMPEDANCE,
> > > > + PIN_CONFIG_BIAS_PULL_UP,
> > > > + PIN_CONFIG_BIAS_PULL_DOWN,
> > > > + PIN_CONFIG_BIAS_HIGH,
> > > > + PIN_CONFIG_BIAS_GROUND,
> > > > + PIN_CONFIG_DRIVE_UNKNOWN,
> > > > + PIN_CONFIG_DRIVE_PUSH_PULL,
> > > > + PIN_CONFIG_DRIVE_OPEN_DRAIN,
> > > > + PIN_CONFIG_DRIVE_OPEN_SOURCE,
> > > > + PIN_CONFIG_DRIVE_OFF,
> > > > + PIN_CONFIG_INPUT_SCHMITT,
> > > > + PIN_CONFIG_SLEW_RATE_RISING,
> > > > + PIN_CONFIG_SLEW_RATE_FALLING,
> > > > + PIN_CONFIG_LOAD_CAPACITANCE,
> > > > + PIN_CONFIG_WAKEUP_ENABLE,
> > > > + PIN_CONFIG_END,
> > > > +};
> > >
> > > IMO, trying to enumerate all possible pin_config options is just to
> > > make everyone's life harder. Firstly, this enumeration is far from
> > > completion, for imx6 iomuxc example, we have 3 options for pull-up,
> > > 22, 47 and 100 kOhm, and 7 options for driver-strength, 34, 40, 48,
> > > 60, 80, 120, and 240 Ohm. It's just so hard to make this enumeration
> > > complete. Secondly, defining this param as enum requires the API
> > > user to call the function multiple times to configure one pin. For
> > > example, if I want to configure pin_foo as slow-slew, open-drain,
> > > 100 kOhm pull-up and 120 Ohm driver-strength, I will have to call
> > > pin_config(pctldev, pin_foo, ...) 4 times to achieve that.
> > >
> > > I like Stephen's idea about having 'u32 param' and let pinctrl drivers
> > > to encode/decode this u32 for their pinctrl controller. It makes
> > > people's life much easier.
> >
> > That's not quite what I meant.
> >
> > I meant that I thought the types for param and value should be simple
> > integers, with meanings of the values defined by the individual drivers,
> > rather than a system-defined enum.
> >
> > However, I wasn't envisaging packing multiple fields into the "value"
> > parameter; that would essentially be packing a struct into a u32 for
> > transport. I still figured that "param" would logically be an enum,
> > and represent a single modifiable parameter, and "data"/"value" would
> > be the single value of that one parameter.
> >
> Oops, I misread your idea. Reading it correctly, I do not like it
> either :) It does not resolve my concern that we need to call the API
> multiple times to configure one pin.
>
> > Still, perhaps packing stuff is an option that makes sense in some cases,
>
> I feel strongly that this is what we want.
Perhaps the solution is for the pinctrl core -> pinctrl driver API to take
arrays of params and values. A simple driver can simply iterate over the
arrays 1 element at a time, and a more complex driver can read the relevant
regs, apply all the changes, then write all the regs back. This would allow
atomically updating multiple values at once, without much function call
overhead, yet still keep the data for each param a simple unpacked value.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/2] pinctrl: add a generic control interface
Shawn Guo wrote at Sunday, October 23, 2011 2:51 AM: ... > To me, pin_config() and its parameters should be invisible to client > drivers. For amba-pl011 example, do you think any of those SoCs will > need multiple sets of uart pin configurations to switch for different > working modes? If that's case, the individual SoC pinctrl driver > should be responsible for mapping particular pin configuration to > specific pl011 working mode. Otherwise, the amba-pl011 can simply > call pinmux_enable(...) to have both mux and cfg set up. I agree. Somebody at the ARM workshop yesterday (and I'm sorry, I forget who; I'm missing too much sleep) pointed out another very good reason: If there's some IP block that's used in a bunch of SoCs, and each SoC has different sets of params (or values) that can be set, the IP block driver shouldn't know about each SoC, and set those values; instead, the board should be telling the pinmux system exactly what parameters to set, and the IP block driver should simply be telling pinmux "set up the pins". Now, I suppose that this could also be done by providing all the params and values to the driver through platform data, however: a) The driver is already calling pinctrl to set up the muxing in an abstract way (mapping table hides the details); I'd like to see pin config work in the same way. b) If this were implemented through platform data in each driver, then every driver's DT binding and probe code will need to define the format of this data and parse it at probe time. Instead, if this is all in some table handled by the pinctrl system, it's defined once, and parse once, which seems a lot simpler. -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH] pinctrl: hide subsystem from the populace
Linus Walleij wrote at Monday, November 07, 2011 7:15 AM:
> From: Linus Walleij
>
> Machines that have embedded pin controllers need to select them
> explicitly, so why broadcast their config options to menuconfig.
> We provide a helpful submenu for those machines that do select
> it, making it possible to enable debugging for example.
...
> -menuconfig PINCTRL
> - bool "PINCTRL Support"
> +config PINCTRL
> + bool
> depends on EXPERIMENTAL
> - help
> - This enables the PINCTRL subsystem for controlling pins
> - on chip packages, for example multiplexing pins on primarily
> - PGA and BGA packages for systems on chip.
> -
> - If unsure, say N.
This seems OK.
> if PINCTRL
>
> +menu "Pin controllers"
> + depends on PINCTRL
> +
> config PINMUX
> bool "Support pinmux controllers"
> help
But shouldn't PINMUX and PINMUX_${soc} also have their help text removed;
it seems like if a SoC selects PINCTRL, it would also select PINMUX if
appropriate, and the relevant PINMUX_${soc} too?
I assume there are also patches for arch/arm/mach-${foo}/Kconfig somewhere
to select PINCTRL when appropriate; they didn't show up in my inbox, but
I haven't checked my sub-folders yet, so perhaps they're already there.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH] pinctrl: Add explicit gpio_disable_free pinmux_op
Stephen Warren wrote at Friday, October 21, 2011 12:26 PM: > Some pinctrl drivers (Tegra at least) program a pin to be a GPIO in a > completely different manner than they select which function to mux out of > that pin. In order to support a single "free" pinmux_op, the driver would > need to maintain a per-pin state of requested-for-gpio vs. requested-for- > function. However, that's a lot of work when the core already has explicit > separate paths for gpio request/free and function request/free. > > So, add a gpio_disable_free op to struct pinmux_ops, and make pin_free() > call it when appropriate. LinusW, Does this patch look good? > When doing this, I noticed that when calling pin_request(): > > !!gpio == (gpio_range != NULL) > > ... and so I collapsed those two parameters in both pin_request(), and > when adding writing the new code in pin_free(). > > Also, for pin_free(): > > !!free_func == (gpio_range != NULL) > > However, I didn't want pin_free() to know about the GPIO function naming > special case, so instead, I reworked pin_free() to always return the pin's > previously requested function, and now pinmux_free_gpio() calls > kfree(function). This is much more balanced with the allocation having > been performed in pinmux_request_gpio(). > > Signed-off-by: Stephen Warren -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH v2] pinctrl: hide subsystem from the populace
Linus Walleij wrote at Wednesday, November 09, 2011 7:13 AM: > Machines that have embedded pin controllers need to select them > explicitly, so why broadcast their config options to menuconfig. > We provide a helpful submenu for those machines that do select > it, making it possible to enable debugging for example. > > Reported-by: Linus Torvalds > Signed-off-by: Linus Walleij Acked-by: Stephen Warren -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [RFC 1/3] pinctrl: add a driver for the OMAP pinmux
Tony Lindgren wrote at Tuesday, November 22, 2011 10:54 AM:
> * Linus Walleij [22 03:30]:
> > On Tue, Nov 22, 2011 at 12:09 PM, Thomas Abraham
> > wrote:
> > > On 17 November 2011 19:27, Linus Walleij wrote:
> > >>
> > >> Maybe I'm mistaken about the device tree ambitions, but
> > >> I was sort of hoping that it would not contain too much
> > >> custom magic numbers that need to be cross-referenced
> > >> elsewhere ... or rather - the more understandable the device
> > >> tree is, the more we win.
> > >
> > > Device tree is expected to describe the hardware. So to
> > > cross-reference the hardware manual to understand device tree should
> > > be fine I guess. For instance, GPIO numbers in dts would be written as
> > > a numeric number and not a enum or other format. And by looking up the
> > > manual, we understand the actual details of the GPIO pin.
> > >
> > > If dt-compiler had a option to support #define like in C, the numbers
> > > could have been made more easier to understand. Like, 3 to mean
> > > GPIO_PULL_UP in Exynos dts file.
> >
> > OK I think I get it now, so DT bindings shall really NOT be read
> > by any of the pinctrl core, it is *supposed* to be all driver-specific.
> > Then it makes perfect sense to have it as it is.
>
> Yes the driver nodes should describe in DT which pins to use:
>
> serial@1234 {
> compatible = "8250";
> reg = <0x1234 0x40>;
> reg-shift = <2>;
> interrupts = < 10 >;
> pins = "uart1_rx", "uart1_tx";
> };
Sorry to jump in late here, but I wasn't aware of this thread.
I don't necessarily agree with that. Describing the HW doesn't necessarily
mean that each device needs to describe what pinmux pins it uses; one
could quite easily have the pinmux describe what settings the various
pins should have and which drivers will use those pins. That would map
very well to the pinctrl subsystem's mapping table, and at least Tegra's
existing pinmux tables, which are both just a big array of settings which
end up getting provided to drivers.
I'll try and track down the rest of this thread and catch up though...
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH Resend V2] dt: add helper function to read u8 & u16 variables & arrays
On 11/18/2012 11:41 PM, Viresh Kumar wrote:
> On 19 November 2012 12:05, Rajanikanth HV wrote:
>> On 19 November 2012 12:00, Viresh Kumar wrote:
>>> Firstly you tried square braces [ ], I am not sure if that is allowed.
>>> Can you point me to the specification?
>> http://www.devicetree.org/Device_Tree_Usage
>> "
>> a-byte-data-property = [0x01 0x23 0x34 0x56];
>> "
>
> Ok, but what about 16 bit then {} :)
Support for byte- and word- properties is relatively recent I believe
(or at least, the /bits/ syntax is). Which dtc version are you using?
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH] dt: add helper function to read u8 & u16 variables & arrays
On 11/19/2012 09:45 PM, Viresh Kumar wrote: > This adds following helper routines: > - of_property_read_u8_array() > - of_property_read_u16_array() > - of_property_read_u8() > - of_property_read_u16() > > This expects arrays from DT to be passed as: > - u8 array: > property = /bits/ 8 <0x50 0x60 0x70>; > - u16 array: > property = /bits/ 16 <0x5000 0x6000 0x7000>; Reviewed-by: Stephen Warren ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/3] cpufreq: TEGRA: Set policy->cpus from driver->init()
On 01/31/2013 11:40 PM, Viresh Kumar wrote: > For multicore SoC's, with cores sharing clock line, we are required to set > policy->cpus and policy->related_cpus with mask of cpus. > > With following patch, we need to set policy->cpus with mask of all possible > cpus > and policy->related_cpus would be filled automatically by the cpufreq core. > > commit 4948b355e90080cd5ec1e91189f65a01e4186ef2 > Author: Viresh Kumar > Date: Tue Jan 29 14:39:08 2013 + > > cpufreq: Simplify cpufreq_add_dev() > > Current Tegra driver fills only ->related_cpus and not ->cpus, which looks to > be > incorrect. Lets fix it. Joseph Lo reviewed/tested this and it looks fine, so, Acked-by: Stephen Warren ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 2/5] clk: notifier handler for dynamic voltage scaling
On 03/01/2013 02:41 AM, Bill Huang wrote: > On Thu, 2013-02-28 at 12:49 +0800, Mike Turquette wrote: >> Dynamic voltage and frequency scaling (dvfs) is a common power saving >> technique in many of today's modern processors. This patch introduces a >> common clk rate-change notifier handler which scales voltage >> appropriately whenever clk_set_rate is called on an affected clock. > > I really think clk_enable and clk_disable should also be triggering > notifier call and DVFS should act accordingly since there are cases > drivers won't set clock rate but instead disable its clock directly, do > you agree? >> >> There are three prerequisites to using this feature: >> >> 1) the affected clocks must be using the common clk framework >> 2) voltage must be scaled using the regulator framework >> 3) clock frequency and regulator voltage values must be paired via the >> OPP library > > Just a note, Tegra Core won't meet prerequisite #3 since each regulator > voltage values is associated with clocks driving those many sub-HW > blocks in it. Perhaps that "just" means extending the dvfs.c code here to iterate over each clock consumer (rather than each clock provider), and having each set a minimum voltage (rather than a specific voltage), and having the regulator core apply the maximum of those minimum constraints? Or something like that anyway. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/12/2013 07:47 PM, Bill Huang wrote: > On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote: >> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote: >>> Add the below four notifier events so drivers which are interested in >>> knowing the clock status can act accordingly. This is extremely useful >>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design. >>> >>> PRE_CLK_ENABLE >>> POST_CLK_ENABLE >>> PRE_CLK_DISABLE >>> POST_CLK_DISABLE >>> >>> Signed-off-by: Bill Huang >> >> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this. >> >> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should >> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and >> clk_unprepare(). Those two functions are *merely* helpers for drivers >> who don't wish to make the individual calls. >> >> Drivers are still completely free to call the individual functions, at >> which point your proposal breaks horribly - and they _do_ call the >> individual functions. > > I'm proposing to give device driver a choice when it knows that some > driver might be interested in knowing its clock's enabled/disabled state > change at runtime, this is very important for centralized DVFS core > driver. It is not meant to be covering all cases especially for drivers > which is not part of the DVFS, so we don't care if it is calling > clk_enable/disable directly or not. I believe the point Russell is making is not that the idea behind this patch is wrong, but simply that the function where you put the hooks is wrong. The hooks should at least be in clk_enable/clk_disable and not clk_prepare_enable/clk_disable_unprepare, since any driver is free to call clk_prepare separately from clk_enable. The hooks should be implemented in the lowest-level common function that all driver-accessible paths call through. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/12/2013 11:08 PM, Bill Huang wrote: > On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote: >> On 03/12/2013 07:47 PM, Bill Huang wrote: >>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote: >>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote: >>>>> Add the below four notifier events so drivers which are interested in >>>>> knowing the clock status can act accordingly. This is extremely useful >>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design. >>>>> >>>>> PRE_CLK_ENABLE >>>>> POST_CLK_ENABLE >>>>> PRE_CLK_DISABLE >>>>> POST_CLK_DISABLE >>>>> >>>>> Signed-off-by: Bill Huang >>>> >>>> NAK. *Sigh* NO, this is the wrong level to be doing stuff like this. >>>> >>>> The *ONLY* thing that clk_prepare_enable() and clk_prepare_disable() should >>>> *EVER* be doing is calling clk_prepare(), clk_enable(), clk_disable() and >>>> clk_unprepare(). Those two functions are *merely* helpers for drivers >>>> who don't wish to make the individual calls. >>>> >>>> Drivers are still completely free to call the individual functions, at >>>> which point your proposal breaks horribly - and they _do_ call the >>>> individual functions. >>> >>> I'm proposing to give device driver a choice when it knows that some >>> driver might be interested in knowing its clock's enabled/disabled state >>> change at runtime, this is very important for centralized DVFS core >>> driver. It is not meant to be covering all cases especially for drivers >>> which is not part of the DVFS, so we don't care if it is calling >>> clk_enable/disable directly or not. >> >> I believe the point Russell is making is not that the idea behind this >> patch is wrong, but simply that the function where you put the hooks is >> wrong. The hooks should at least be in clk_enable/clk_disable and not >> clk_prepare_enable/clk_disable_unprepare, since any driver is free to >> call clk_prepare separately from clk_enable. The hooks should be >> implemented in the lowest-level common function that all >> driver-accessible paths call through. > > Thanks, I know the point, but unfortunately there is no good choice for > hooking this since those low level functions clk_enable/clk_disable will > be called in interrupt context so it is not possible to send notify. We > might need to come out a better approach if we can think of any. > Currently I still think this is acceptable (Having all the drivers which > are using our interested clocks call these function to enable/disable > clock in their runtime_pm calls) though it's not perfect. No, that definitely won't work. Not all drivers use those APIs, nor should they. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/12/2013 11:40 PM, Bill Huang wrote: > On Wed, 2013-03-13 at 13:24 +0800, Stephen Warren wrote: >> On 03/12/2013 11:08 PM, Bill Huang wrote: >>> On Wed, 2013-03-13 at 12:42 +0800, Stephen Warren wrote: >>>> On 03/12/2013 07:47 PM, Bill Huang wrote: >>>>> On Tue, 2013-03-12 at 21:40 +0800, Russell King - ARM Linux wrote: >>>>>> On Tue, Mar 12, 2013 at 05:37:41AM -0700, Bill Huang wrote: >>>>>>> Add the below four notifier events so drivers which are interested in >>>>>>> knowing the clock status can act accordingly. This is extremely useful >>>>>>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design. >>>>>>> >>>>>>> PRE_CLK_ENABLE >>>>>>> POST_CLK_ENABLE >>>>>>> PRE_CLK_DISABLE >>>>>>> POST_CLK_DISABLE ... >>> Thanks, I know the point, but unfortunately there is no good choice for >>> hooking this since those low level functions clk_enable/clk_disable will >>> be called in interrupt context so it is not possible to send notify. We >>> might need to come out a better approach if we can think of any. >>> Currently I still think this is acceptable (Having all the drivers which >>> are using our interested clocks call these function to enable/disable >>> clock in their runtime_pm calls) though it's not perfect. >> >> No, that definitely won't work. Not all drivers use those APIs, nor >> should they. >> > That will be too bad, it looks like we deadlock in the mechanism, we > cannot change existing drivers behavior (that means some call > clk_disable/enable directly, some are not), and we cannot hook notifier > in clk_disable/enable either, that means there seems no any chance to > get what we want, any idea? I don't know the correct answer. But I have a question: Why can't we run notifications from the real clk_enable? Does the notification mechanism itself inherently block, or are we worried that implementations/receivers of these notifications might block. Perhaps we can simply define that these notification types may be triggered in atomic context and hence implementations have to support executing in atomic context. Is possible to make that a requirement? Is it possible to implement the code that receives these notifications in atomic context? Is it possible to defer triggering of notifications to some non-atomic context, even if they happen in an atomic context? I also wonder if this is the right conceptual level to be hooking into the system. Perhaps DVFS is not something that should be triggered by noticing that clocks have changed rates, but rather it should be some form of higher layer that controls (calls into) both the regulator and clock APIs? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/14/2013 03:28 AM, Bill Huang wrote: > On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote: >> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote: >> >>> I don't think deferring will work either, considering the usage of DVFS, >>> device voltage is tightly coupled with frequency, when clock rate is >>> about to increase, we have to boost voltage first and we can lower the >>> voltage after the clock rate has decreased. All the above sequence have >>> to be guaranteed or you might crash, so deferring not only make thing >>> complicated in controlling the order but also hurt performance. >> >> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage >> no? >> As clk_prepare/clk_unprepare have to be called before clk_enable or after >> clk_disable, the voltage can be raised to a safe level, before the clock >> becomes active. > > Thanks Peter, actually I'm just about to propose my v2 RFC which add > notifier in clk_prepare/clk_unprepare. Can't clk_set_rate() be called while the clock is prepared, or even enabled? I don't see how your proposal would work. ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/14/2013 07:20 PM, Bill Huang wrote: > On Fri, 2013-03-15 at 01:54 +0800, Stephen Warren wrote: >> On 03/14/2013 03:28 AM, Bill Huang wrote: >>> On Thu, 2013-03-14 at 17:21 +0800, Peter De Schrijver wrote: >>>> On Thu, Mar 14, 2013 at 03:15:11AM +0100, Bill Huang wrote: >>>> >>>>> I don't think deferring will work either, considering the usage of DVFS, >>>>> device voltage is tightly coupled with frequency, when clock rate is >>>>> about to increase, we have to boost voltage first and we can lower the >>>>> voltage after the clock rate has decreased. All the above sequence have >>>>> to be guaranteed or you might crash, so deferring not only make thing >>>>> complicated in controlling the order but also hurt performance. >>>> >>>> But we could use notifiers in clk_prepare/clk_unprepare to set the voltage >>>> no? >>>> As clk_prepare/clk_unprepare have to be called before clk_enable or after >>>> clk_disable, the voltage can be raised to a safe level, before the clock >>>> becomes active. >>> >>> Thanks Peter, actually I'm just about to propose my v2 RFC which add >>> notifier in clk_prepare/clk_unprepare. >> >> Can't clk_set_rate() be called while the clock is prepared, or even >> enabled? I don't see how your proposal would work. > > I think it works with just a little sacrifice on saving more power but > that's related minor. Taking clk_prepare as an indicator on that clock > will be enabled later, so we can raise the voltage to a safe level > (according to the current rate or maybe default rate when clk_prepare is > called, some time late when clk_set_rate() is called we can adjust again > according to the requested rate change) Is clk_set_rate() only legal to call in non-atomic contexts then? The header file doesn't say, although I guess since many other functions explicitly say they can't, then by omission it can... ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC 1/1] clk: Add notifier support in clk_prepare_enable/clk_disable_unprepare
On 03/15/2013 06:33 AM, Ulf Hansson wrote: > On 15 March 2013 13:06, Bill Huang wrote: >> On Fri, 2013-03-15 at 18:08 +0800, Ulf Hansson wrote: ... >>> Some prerequisites; I think am in favor of using the clk API to >>> trigger DVFS changes and then I agree on that clk_prepare|unprepare >>> needs to be possible to track from a DVFS perspective. clk_set_rate is >>> not enough. >>> >>> So if we decide to do the above (using the clk API to trigger DVFS >>> changes), I believe we should discuss two possible solutions; >>> - clk notifiers or.. >>> - dvfs clock type. >>> >>> I am trying to make up my mind of what I think is the best solution. >>> Have you considered "dvfs clock type"? >>> I put some comments about this for "[PATCH 2/5] clk: notifier handler >>> for dynamic voltage scaling" recently as well. >>> >>> What could the advantages/disadvantages be between the two options? >> >> I personally prefer clk notifiers since that's easy and all the existing >> device drivers don't need to be modified, a new clock or API might be >> more thoroughly considered (and hence maybe more graceful) but that >> means we need more time to cook and many drivers need to plug into that >> API when it comes out, a lot of test/verification or maybe chaos >> follows, I'm not sure will that be a little overkill. > > I guess you did not fully got what I meant with "dvfs clock type". It > will not affect the clock API. But instead the dvfs is handled by > implementing a specific clk hw type. So the same thing is accomplished > as with clk notifiers, no changes should be needed to device drivers. > > The difference is only that no notifiers will be needed, and all the > dvfs stuff will be handled in the clk hw instead. It will mean that we > will bundle dvfs stuff into the clock drivers, instead of separating > the code outside the clock drivers. But, on the other hand no > notifiers will be needed. The advantage here is that I assume that a notifier would continually have to check whether the clock being modified was one that the DVFS notifier cared about. By integrating the CVFS logic into the clk_hw itself, it'll only ever get executed for clocks that really care about DVFS. Presumably, the code that implements the clk_hw could also use some common DVFS library as part of the implementation, and still share code. Or perhaps, what about putting DVFS "ops" into a clk_hw alongside any other existing ops, and having the clock core call them whenever appropriate? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [RFC v2 1/1] clk: Add notifier support in clk_prepare/clk_unprepare
On 03/15/2013 11:45 AM, Russell King - ARM Linux wrote:
> On Thu, Mar 14, 2013 at 02:31:04AM -0700, Bill Huang wrote:
>> Add the below two notifier events so drivers which are interested in
>> knowing the clock status can act accordingly. This is extremely useful
>> in some of the DVFS (Dynamic Voltage Frequency Scaling) design.
>>
>> CLK_PREPARED
>> CLK_UNPREPARED
>>
>> Signed-off-by: Bill Huang
>> ---
>> drivers/clk/clk.c |3 +++
>> include/linux/clk.h |2 ++
>> 2 files changed, 5 insertions(+)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index ed87b24..3292cec 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -550,6 +550,7 @@ void clk_unprepare(struct clk *clk)
>> {
>> mutex_lock(&prepare_lock);
>> __clk_unprepare(clk);
>> +__clk_notify(clk, CLK_UNPREPARED, clk->rate, clk->rate);
>> mutex_unlock(&prepare_lock);
>> }
>> EXPORT_SYMBOL_GPL(clk_unprepare);
>> @@ -598,6 +599,8 @@ int clk_prepare(struct clk *clk)
>>
>> mutex_lock(&prepare_lock);
>> ret = __clk_prepare(clk);
>> +if (!ret)
>> +__clk_notify(clk, CLK_PREPARED, clk->rate, clk->rate);
>
> So, on prepare, we notify after we've prepared the clock. On unprepare,
> we notify after the clock has been shut down. Are you sure that's the
> correct ordering? Would it not be better to view it in a stack-like
> fashion, iow:
> get
> prepare
> notify-prepare
> enable
> disable
> notify-unprepare
> unprepare
> put
Yes, these should be stacked/nested better for consistency.
But for DVFS, the voltage needs to be raised before the prepare body is
run, so that if clk_prepare actually enables the clock, the voltage is
already at the safe level required by that clock. Similarly, for
unprepare, you can only lower the voltage after having turned off the
clock, which is guaranteed after the unprepare body. So, I think you
want to move the notifier for prepare in the code above (and rename it
to pre/before_prepare?), rather than the notifier for unprepare.
In order to cover more cases, you might have both
{pre,post}_{un,}prepare notifiers, although I'm not sure when you'd use
the other two options.
>> diff --git a/include/linux/clk.h b/include/linux/clk.h
>> +#define CLK_PREPAREDBIT(3)
>> +#define CLK_UNPREPARED BIT(4)
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
Re: [PATCH 1/1] clk: Add notifier support in clk_prepare/clk_unprepare
On 03/28/2013 04:01 PM, Mike Turquette wrote: > Quoting Colin Cross (2013-03-21 17:06:25) >> On Thu, Mar 21, 2013 at 3:36 PM, Mike Turquette >> wrote: >>> To my knowledge, devfreq performs one task: implements an algorithm >>> (typically one that loops/polls) and applies this heuristic towards a >>> dvfs transition. >>> >>> It is a policy layer, a high level layer. It should not be used as a >>> lower-level mechanism. Please correct me if my understanding is wrong. >>> >>> I think the very idea of the clk framework calling into devfreq is >>> backwards. Ideally a devfreq driver would call clk_set_rate as part of >>> it's target callback. This is analogous to a cpufreq .target callback >>> which calls clk_set_rate and regulator_set_voltage. Can you imagine the >>> clock framework cross-calling into cpufreq when clk_set_rate is called? >>> I think that would be strange. >>> >>> I think that all of this discussion highlights the fact that there is a >>> missing piece of infrastructure. It isn't devfreq or clock rate-change >>> notifiers. It is that there is not a dvfs mechanism which neatly builds >>> on top of these lower-level frameworks (clocks & regulators). Clearly >>> some higher-level abstraction layer is needed. >> >> I went through all of this on Tegra2. For a while I had a >> dvfs_set_rate api for drivers that needed to modify the voltage when >> they updated a clock, but I ended up dropping it. Drivers rarely care >> about the voltage, all they want to do is set their clock rate. The >> voltage necessary to support that clock is an implementation detail of >> the silicon that is irrelevant to the driver > > Hi Colin, > > I agree about voltage scaling being an implementation detail, but I > think that drivers similarly do not care about enabling clocks, clock > domains, power domains, voltage domains, etc. The just want to say > "give me what I need to turn on and run", and "I'm done with that stuff > now, lazily turn off if you want to". Runtime pm gives drivers that > abstraction layer today. I don't understand how runtime PM gives this abstraction today. All the implementations of runtime PM that I've seen involve the driver itself implementing its own runtime PM callbacks, and explicitly managing the clocks itself. I don't see how that hides those details from the driver. Have I been looking at runtime PM implementations that aren't implemented philosophically correctly? ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH] ARM: Add dtbuImage target
U-Boot wrapped dtbImage; useful for testing DT with an unmodified U-Boot. Signed-off-by: Stephen Warren --- This patch is based on: git://kernel.ubuntu.com/jk/dt/linux-2.6.git dtbimage However, I actually developed and tested it only on: git://git.secretlab.ca/git/linux-2.6 devicetree/test plus the following commits from jk's dtbimage branch: 4c2eddb89542f73fe626813e3cdafc789f931aec arm: dtbImage support 4cb80ac96489220554d28f6fde527aeef83e628b arm/dtbimage: copy dtb blob to offset from image base I wonder if rather than simply applying my patch below to jk's dtbimage branch, perhaps it would be better to cherry-pick the two commits I mentioned above into the devicetree/test tree, after which I can submit the original change I wrote there? arch/arm/Makefile |2 +- arch/arm/boot/Makefile |8 2 files changed, 9 insertions(+), 1 deletions(-) diff --git a/arch/arm/Makefile b/arch/arm/Makefile index dab066a..3ce1751 100644 --- a/arch/arm/Makefile +++ b/arch/arm/Makefile @@ -264,7 +264,7 @@ archprepare: # Convert bzImage to zImage bzImage: zImage -zImage Image xipImage bootpImage uImage dtbImage: vmlinux +zImage Image xipImage bootpImage uImage dtbImage dtbuImage: vmlinux $(Q)$(MAKE) $(build)=$(boot) MACHINE=$(MACHINE) $(boot)/$@ zinstall install: vmlinux diff --git a/arch/arm/boot/Makefile b/arch/arm/boot/Makefile index c608c98..a8f4251 100644 --- a/arch/arm/boot/Makefile +++ b/arch/arm/boot/Makefile @@ -66,15 +66,19 @@ quiet_cmd_uimage = UIMAGE $@ ifeq ($(CONFIG_ZBOOT_ROM),y) $(obj)/uImage: LOADADDR=$(CONFIG_ZBOOT_ROM_TEXT) +$(obj)/dtbuImage: LOADADDR=$(CONFIG_ZBOOT_ROM_TEXT) else $(obj)/uImage: LOADADDR=$(ZRELADDR) +$(obj)/dtbuImage: LOADADDR=$(ZRELADDR) endif ifeq ($(CONFIG_THUMB2_KERNEL),y) # Set bit 0 to 1 so that "mov pc, rx" switches to Thumb-2 mode $(obj)/uImage: STARTADDR=$(shell echo $(LOADADDR) | sed -e "s/.$$/1/") +$(obj)/dtbuImage: STARTADDR=$(shell echo $(LOADADDR) | sed -e "s/.$$/1/") else $(obj)/uImage: STARTADDR=$(LOADADDR) +$(obj)/dtbuImage: STARTADDR=$(LOADADDR) endif $(obj)/uImage: $(obj)/zImage FORCE @@ -97,6 +101,10 @@ $(obj)/dtbImage: $(obj)/dt/dtb FORCE $(call if_changed,objcopy) @echo ' Kernel: $@ is ready' +$(obj)/dtbuImage: $(obj)/dtbImage FORCE + $(call if_changed,uimage) + @echo ' Image $@ is ready' + PHONY += initrd FORCE initrd: @test "$(INITRD_PHYS)" != "" || \ -- 1.7.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 2/3] ARM: Tegra: dt: Fix machine desc to match other boards.
The content of the machine descriptions has be re-organized. Without fixing
the board-dt.c copy, the system will fail to boot (BUG_ON during timer
initialization, which happens before printk is operational, leading to a
silent hang early during kernel boot). Solve this basiclaly by copying the
existing Harmony version over the copy in board-dt.c
Signed-off-by: Stephen Warren
---
arch/arm/mach-tegra/board-dt.c | 11 ++-
1 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 899311b..7b9d469 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -175,10 +175,11 @@ static const char * tegra_dt_board_compat[] = {
};
DT_MACHINE_START(TEGRA_DT, "nVidia Tegra (Flattened Device Tree)")
- .fixup = tegra_dt_fixup,
- .init_irq = tegra_init_irq,
- .init_machine = tegra_dt_init,
- .map_io = tegra_map_common_io,
- .timer = &tegra_timer,
+ .boot_params= 0x0100,
+ .map_io = tegra_map_common_io,
+ .init_early = tegra_init_early,
+ .init_irq = tegra_init_irq,
+ .timer = &tegra_timer,
+ .init_machine = tegra_dt_init,
.dt_compat = tegra_dt_board_compat,
MACHINE_END
--
1.7.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 1/3] ARM: Tegra: dt: Compile fix; tegra_common_init removed
tegra_common_init was removed by: 0cf6230af909a86f81907455eca2a5c9b8f68fe6 ARM: tegra: Move tegra_common_init to tegra_init_early Fix the Tegra devicetree support to match. Signed-off-by: Stephen Warren --- arch/arm/mach-tegra/board-dt.c |2 -- 1 files changed, 0 insertions(+), 2 deletions(-) diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c index bc9777e..899311b 100644 --- a/arch/arm/mach-tegra/board-dt.c +++ b/arch/arm/mach-tegra/board-dt.c @@ -156,8 +156,6 @@ static void __init tegra_dt_init(void) */ of_platform_prepare(NULL, tegra_dt_match_table); - tegra_common_init(); - tegra_clk_init_from_table(tegra_dt_clk_init_table); harmony_pinmux_init(); -- 1.7.1 ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 0/3] Fix Tegra devicetree support
I tried out: git://git.secretlab.ca/git/linux-2.6.git devicetree/test plus the following commits from jk's dtbimage branch: 4c2eddb89542f73fe626813e3cdafc789f931aec arm: dtbImage support 4cb80ac96489220554d28f6fde527aeef83e628b arm/dtbimage: copy dtb blob to offset from image base A local patch to modify the Tegra devicetree support to support Seaboard instead of Harmony. ... on an NVIDIA Tegra Seaboard board. This patch series includes fixes to that branch to get Tegra booting to user-space. Note that soon after user-space starts, the system still crashes with: Unhandled prefetch abort: debug event (0x002) at 0x40e97714 However, this happens with or without actually providing a devicetree image to the kernel (I modified jk's dtbimage wrapper just to jump straight to the regular kernel startup, thus not providing a devicetree at all). So, I suspect it's some unrelated issue. I'll try to follow up and solve that too. Stephen Warren (3): ARM: Tegra: dt: Compile fix; tegra_common_init removed ARM: Tegra: dt: Fix machine desc to match other boards. ARM: Tegra: Move Harmony .dts file to correct place arch/arm/boot/dts/tegra-harmony.dts | 110 +++- arch/arm/mach-tegra/board-dt.c| 13 ++-- arch/arm/mach-tegra/board-harmony.dts | 113 - 3 files changed, 114 insertions(+), 122 deletions(-) delete mode 100644 arch/arm/mach-tegra/board-harmony.dts ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
[PATCH 3/3] ARM: Tegra: Move Harmony .dts file to correct place
The following workflow:
make dtbs
make dtbuImage # See my earlier patch which adds this based on
Jeremy Kerr's patch to add a dtbImage target.
... seems to require the *.dts file to be in arch/arm/boot/dts. I'm not
sure if any other devicetree testing flows will be negatively affected by
this change.
Signed-off-by: Stephen Warren
---
arch/arm/boot/dts/tegra-harmony.dts | 110 +++-
arch/arm/mach-tegra/board-harmony.dts | 113 -
2 files changed, 108 insertions(+), 115 deletions(-)
delete mode 100644 arch/arm/mach-tegra/board-harmony.dts
diff --git a/arch/arm/boot/dts/tegra-harmony.dts
b/arch/arm/boot/dts/tegra-harmony.dts
index 81032e7..82fa0c2 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -1,7 +1,113 @@
+
/dts-v1/;
-/include/ "skeleton.dtsi"
/ {
- model = "NVIDIA Tegra2 Harmony evaluation board";
+ model = "nVidia Harmony";
compatible = "nvidia,harmony", "nvidia,tegra250";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ interrupt-parent = <&intc>;
+
+ chosen {
+ bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8
root=/dev/mmcblk0p2 rw rootdelay=2";
+ };
+
+ memory {
+ device_type = "memory";
+ reg = < 0x 0x1C00
+ 0x2000 0x2000 >;
+ };
+
+ amba {
+ compatible = "simple-bus";
+ #address-cells = <1>;
+ #size-cells = <1>;
+ ranges;
+
+ intc: intc {
+ interrupt-controller;
+ #interrupt-cells = <1>;
+ };
+ };
+
+ gpio: gpio@6000d000 {
+ compatible = "nvidia,tegra250-gpio";
+ reg = < 0x6000d000 0x1000 >;
+ interrupts = < 64 65 66 67 87 119 121 >;
+ #gpio-cells = <2>;
+ gpio-controller;
+ };
+
+ serial@70006000 {
+ compatible = "nvidia,tegra250-uart";
+ reg = <0x70006000 0x40>;
+ reg-shift = <2>;
+ interrupts = < 68 >;
+ status = "disabled";
+ };
+
+ serial@70006040 {
+ compatible = "nvidia,tegra250-uart";
+ reg = <0x70006040 0x40>;
+ reg-shift = <2>;
+ interrupts = < 69 >;
+ status = "disabled";
+ };
+
+ serial@70006200 {
+ compatible = "nvidia,tegra250-uart";
+ reg = <0x70006200 0x100>;
+ reg-shift = <2>;
+ interrupts = < 78 >;
+ status = "disabled";
+ };
+
+ serial@70006300 {
+ compatible = "nvidia,tegra250-uart";
+ reg = <0x70006300 0x100>;
+ reg-shift = <2>;
+ interrupts = < 122 >;
+
+ clock-frequency = < 21600 >;
+ };
+
+ serial@70006400 {
+ compatible = "nvidia,tegra250-uart";
+ reg = <0x70006400 0x100>;
+ reg-shift = <2>;
+ interrupts = < 123 >;
+ status = "disabled";
+ };
+
+ sdhci@c800 {
+ compatible = "nvidia,tegra250-sdhci";
+ reg = <0xc800 0x200>;
+ interrupts = < 46 >;
+ status = "disabled";
+ };
+
+ sdhci@c8000200 {
+ compatible = "nvidia,tegra250-sdhci";
+ reg = <0xc8000200 0x200>;
+ interrupts = < 47 >;
+ gpios = <&gpio 69 0>, /* cd, gpio PI5 */
+ <&gpio 57 0>, /* wp, gpio PH1 */
+ <&gpio 155 0>; /* power, gpio PT3 */
+ };
+
+ sdhci@c8000400 {
+ compatible = "nvidia,tegra250-sdhci";
+ reg = <0xc8000400 0x200>;
+ interrupts = < 51 >;
+ status = "disabled";
+ };
+
+ sdhci@c8000600 {
+ compatible = "nvidia,tegra250-sdhci";
+ reg = <0xc8000600 0x200>;
+ interrupts = < 63 >;
+ gpios = <&gpio 58 0>, /* cd, gpio PH2 */
+ <&gpio 59 0>, /* wp, gpio PH3 */
+ <&gpio 70 0>; /* power, gpio PI6 */
+ };
};
diff --git a/arch/arm/mach-tegra/board-harmony.dts
b/arch/arm/mach-tegra/board-harmony.dts
deleted file
[PATCH] ARM: Tegra: dt: HACK: Support Seaboard not Harmony
(Do not apply)
This is only necessary at present since the devicetree support relies on
some aspects of some existing hard-coded board file for some parts of
initialization. In the long run, that code can be ported to devicetree
too.
Signed-off-by: Stephen Warren
---
I needed to test devicetree support on Tegra Seaboard rather than Harmony.
This patch is the changes I needed to make to have that work. It builds on
the previous patches I recently posted. It's only an FYI just in case
anyone else finds it useful.
arch/arm/boot/dts/tegra-harmony.dts | 14 ++
arch/arm/mach-tegra/board-dt.c | 14 ++
2 files changed, 8 insertions(+), 20 deletions(-)
diff --git a/arch/arm/boot/dts/tegra-harmony.dts
b/arch/arm/boot/dts/tegra-harmony.dts
index 82fa0c2..6fe09d7 100644
--- a/arch/arm/boot/dts/tegra-harmony.dts
+++ b/arch/arm/boot/dts/tegra-harmony.dts
@@ -9,7 +9,8 @@
interrupt-parent = <&intc>;
chosen {
- bootargs = "vmalloc=192M video=tegrafb console=ttyS0,115200n8
root=/dev/mmcblk0p2 rw rootdelay=2";
+ /*bootargs = "vmalloc=192M video=tegrafb
console=ttyS0,115200n8 root=/dev/mmcblk0p2 rw rootdelay=2";*/
+ bootargs = "earlyprintk
console=ttyS0,115200n8 root=/dev/mmcblk1p3 ro rootwait noinitrd
lp0_vec=0x2000@0x1C406000 loglevel=8 kcrashmem=0x10@0x0200 mem=384M@0M
nvmem=128M@384M mem=512M@512M";
};
memory {
@@ -90,24 +91,21 @@
compatible = "nvidia,tegra250-sdhci";
reg = <0xc8000200 0x200>;
interrupts = < 47 >;
- gpios = <&gpio 69 0>, /* cd, gpio PI5 */
- <&gpio 57 0>, /* wp, gpio PH1 */
- <&gpio 155 0>; /* power, gpio PT3 */
+ status = "disabled";
};
sdhci@c8000400 {
compatible = "nvidia,tegra250-sdhci";
reg = <0xc8000400 0x200>;
interrupts = < 51 >;
- status = "disabled";
+ gpios = <&gpio 69 0>, /* cd, gpio PI5 */
+ <&gpio 57 0>, /* wp, gpio PH1 */
+ <&gpio 70 0>; /* power, gpio PI6 */
};
sdhci@c8000600 {
compatible = "nvidia,tegra250-sdhci";
reg = <0xc8000600 0x200>;
interrupts = < 63 >;
- gpios = <&gpio 58 0>, /* cd, gpio PH2 */
- <&gpio 59 0>, /* wp, gpio PH3 */
- <&gpio 70 0>; /* power, gpio PI6 */
};
};
diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
index 7b9d469..b67c382 100644
--- a/arch/arm/mach-tegra/board-dt.c
+++ b/arch/arm/mach-tegra/board-dt.c
@@ -36,7 +36,7 @@
#include
#include "board.h"
-#include "board-harmony.h"
+#include "board-seaboard.h"
#include "clock.h"
static struct resource sdhci_resource1[] = {
@@ -126,16 +126,6 @@ static struct platform_device *harmony_devices[]
__initdata = {
&tegra_sdhci_device4,
};
-static void __init tegra_dt_fixup(struct machine_desc *desc,
- struct tag *tags, char **cmdline, struct meminfo *mi)
-{
- mi->nr_banks = 2;
- mi->bank[0].start = PHYS_OFFSET;
- mi->bank[0].size = 448 * SZ_1M;
- mi->bank[1].start = SZ_512M;
- mi->bank[1].size = SZ_512M;
-}
-
static __initdata struct tegra_clk_init_table tegra_dt_clk_init_table[] = {
/* name parent rateenabled */
{ "uartd", "pll_p",21600, true },
@@ -158,7 +148,7 @@ static void __init tegra_dt_init(void)
tegra_clk_init_from_table(tegra_dt_clk_init_table);
- harmony_pinmux_init();
+ seaboard_pinmux_init();
platform_add_devices(harmony_devices, ARRAY_SIZE(harmony_devices));
--
1.7.1
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
devicetree: Tegra I2C muxing, and of_i2c_register_devices
Tegra's I2C driver has a unique feature built in (although not mainlined
yet); it can multiplex the I2C bus onto different sets of pins using the
Tegra pinmux module, and hence can register more than 1 I2C bus with the
I2C core for each controller. The exact number of busses registers, and
the pinmux settings to be used, are provided by platform data. See:
http://git.chromium.org/git/kernel-next.git chromeos-2.6.38
include/linux/i2c-tegra.h
arch/arm/mach-tegra/board-seaboard.c
If I understand the direction of devicetree on ARM, there should be a
single devicetree node for the Tegra I2C controller, which will get
matched up with a static platform device registration in e.g.
mach-tegra/board-dt.c
I can easily see how to provide the appropriate platform data to the
Tegra I2C driver, by definining the appropriate fields in a custom
devicetree binding.
To cater for the multiple child busses, the simplest solution seems to
be to define each child node's reg property to be a tuple, (c.f. existing ). E.g.:
IIC0: i2c@ {
compatible = "nvidia,tegra250-iic";
#address-cells = <2>;
#size-cells = <0>;
bus_muxes = ;
rtc@68 {
compatible = "stm,m41t80";
reg = <0 0x68>;
};
sttm@48 {
compatible = "ad,ad7414";
reg = <1 0x48>;
};
};
The problem is then that of_i2c_register_devices won't work well for the
Tegra I2C controller, since it enforces that each child node's reg
property be a single value, the I2C address.
To solve this, should we:
a) Have each mux'd bus have a separate devicetree node underneath the
main I2C device, e.g.:
IIC0: i2c@ {
compatible = "nvidia,tegra250-iic";
// @0 doesn't really end up matching a
// reg property within the child though
bus@0 {
#address-cells = <1>;
#size-cells = <0>;
pinmux_group = ;
pinmux_value = ;
rtc@68 {
compatible = "stm,m41t80";
reg = <0x68>;
};
}
bus@1 {
#address-cells = <1>;
#size-cells = <0>;
pinmux_group = ;
pinmux_value = ;
sttm@48 {
compatible = "ad,ad7414";
reg = <0x48>;
};
}
};
Then, the bus@0 or bus@1 nodes could be placed into adap->dev.of_node,
since the devices are hung off there.
b)
Implement custom devicetree child enumeration code in the Tegra I2C
driver to replace of_i2c_register_devices, which implements the
two-entry reg format. The first devicetree example in this email could
then be used.
c)
Perhaps remove the bus mux functionality from the Tegra I2C driver, and
implement a separate I2C mux driver for it.
I'm not sure if devicetree has a defined representation for I2C bus
muxes, and even if it does, since the mux control isn't accessed by I2C
registers (as most standalone I2C bus muxes are), but rather Tegra-
specific pinmux API calls, I'm not quite sure how to represent it in
device-tree.
Thanks for any thoughts!
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
devicetree: Tegra SoC configuration tables
Tegra's board files currently contain quite a number of tables, with board-specific content. For example, (in mainline) arch/arm/mach-tegra/ board-seaboard-pinmux.c contains a table of pinmux settings, a table of pin drive strengths, and a list of all GPIOs that must be enabled. In the ChromeOS kernel, there are a number of additional tables for DVFS limits on various clocks and regulators, memory controller timing settings, etc. etc. In a devicetree-enabled kernel, it seems like these could simply be pushed into the Tegra SoC/CPU DT node as custom fields holding many- valued tuples that contain the raw table data. Does this seem like a reasonable thing to do? -- nvpublic ___ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/3] ARM: Tegra: dt: Fix machine desc to match other boards.
Grant Likely wrote at Friday, April 29, 2011 5:35 PM:
> On Sat, Apr 23, 2011 at 10:30:03PM -0600, Stephen Warren wrote:
> > The content of the machine descriptions has be re-organized. Without fixing
> > the board-dt.c copy, the system will fail to boot (BUG_ON during timer
> > initialization, which happens before printk is operational, leading to a
> > silent hang early during kernel boot). Solve this basiclaly by copying the
> > existing Harmony version over the copy in board-dt.c
> >
> > Signed-off-by: Stephen Warren
> > ---
> > arch/arm/mach-tegra/board-dt.c | 11 ++-
> > 1 files changed, 6 insertions(+), 5 deletions(-)
> >
> > diff --git a/arch/arm/mach-tegra/board-dt.c b/arch/arm/mach-tegra/board-dt.c
> > index 899311b..7b9d469 100644
> > --- a/arch/arm/mach-tegra/board-dt.c
> > +++ b/arch/arm/mach-tegra/board-dt.c
> > @@ -175,10 +175,11 @@ static const char * tegra_dt_board_compat[] = {
> > };
> >
> > DT_MACHINE_START(TEGRA_DT, "nVidia Tegra (Flattened Device Tree)")
> > - .fixup = tegra_dt_fixup,
>
> I take it that with memory ranges encoded into the device tree this
> fixup is no longer needed? I'll drop the tegra_dt_fixup() function
> from the file too then.
I actually tested this all on Seaboard not Harmony, and locally modified
the tegra-harmony.dts to add to the kernel cmdline mem=/nvmem= options
identical to what we use for Seaboard + ChromeOS. Now I have things
working, I should probably remove the kernel cmdline mem=/nvmem= options,
and validate that the dt memory setup is working correctly. I believe the
fixup function was only there to cater for broken bootloader setups that
didn't supply the correct mem=/nvmem= on the kernel cmdline, so this
should all work OK...
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 2/3] ARM: Tegra: dt: Fix machine desc to match other boards.
Stephen Warren wrote at Sent: Friday, April 29, 2011 7:52 PM:
> To: Grant Likely
> Cc: linaro-dev@lists.linaro.org; linux-te...@vger.kernel.org
> Subject: RE: [PATCH 2/3] ARM: Tegra: dt: Fix machine desc to match other
> boards.
>
> Grant Likely wrote at Friday, April 29, 2011 5:35 PM:
> > On Sat, Apr 23, 2011 at 10:30:03PM -0600, Stephen Warren wrote:
> > > The content of the machine descriptions has be re-organized. Without
> > > fixing
> > > the board-dt.c copy, the system will fail to boot (BUG_ON during timer
> > > initialization, which happens before printk is operational, leading to a
> > > silent hang early during kernel boot). Solve this basiclaly by copying the
> > > existing Harmony version over the copy in board-dt.c
> > >
> > > Signed-off-by: Stephen Warren
> > > ---
> > > arch/arm/mach-tegra/board-dt.c | 11 ++-
> > > 1 files changed, 6 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/arch/arm/mach-tegra/board-dt.c
> > > b/arch/arm/mach-tegra/board-dt.c
> > > index 899311b..7b9d469 100644
> > > --- a/arch/arm/mach-tegra/board-dt.c
> > > +++ b/arch/arm/mach-tegra/board-dt.c
> > > @@ -175,10 +175,11 @@ static const char * tegra_dt_board_compat[] = {
> > > };
> > >
> > > DT_MACHINE_START(TEGRA_DT, "nVidia Tegra (Flattened Device Tree)")
> > > - .fixup = tegra_dt_fixup,
> >
> > I take it that with memory ranges encoded into the device tree this
> > fixup is no longer needed? I'll drop the tegra_dt_fixup() function
> > from the file too then.
>
> I actually tested this all on Seaboard not Harmony, and locally modified
> the tegra-harmony.dts to add to the kernel cmdline mem=/nvmem= options
> identical to what we use for Seaboard + ChromeOS. Now I have things
> working, I should probably remove the kernel cmdline mem=/nvmem= options,
> and validate that the dt memory setup is working correctly. I believe the
> fixup function was only there to cater for broken bootloader setups that
> didn't supply the correct mem=/nvmem= on the kernel cmdline, so this
> should all work OK...
I've now tested on Seaboard with the fixup removed as above, no mem= or
nvmem= kernel cmdline options, and hence just the devicetree memory
definitions. Everything looks like it's fine.
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [RFC] (early draft) dt: Linux dt usage model documentation
Grant Likely wrote at Monday, June 13, 2011 7:32 AM:
> Signed-off-by: Grant Likely
> ---
>
> Hey all,
>
> This is an early draft of the usage model document for the device
> tree, but I wanted to get it out there for feedback, and so that some
> of the Linaro engineers could get started on migrating board ports.
This looks great. I'll certainly raise awareness of this documentation
within NVIDIA. I'd written a few brief internal notes on this topic, but
yours is much more complete.
As far as the content goes, it all looks good to me. Of course, I'm already
somewhat familiar with DT...
So, unfortunately, almost all I have to contribute to below are some nit-
pick typos etc.
...
> +The "Open Firmware Device Tree", or simply Device Tree (DT), is a data
> +structure and language for describing hardware. More specifically, it
> +is an description of hardware that is readable by an operating system
^^ a
...
> +History
Should the HTML tags be here?
...
> +In the majority of cases, the machine identity is irrelevant, and the
> +kernel will instead select setup code based on the machines core
machine's
...
> +The reasoning behind this scheme is the observation that in the majority
> +of cases, a single machine_desc can support a large number of boards
> +if that all use the same SoC, or same family of SoCs. However,
they (or delete "if")
...
> +Instead, the compatible list allows a generic machine_desc to provide
> +support for a wide common set of boards by specifying "less
> +compatible" value in the dt_compat list. In the example above,
> +generic board support can claim compatibility with "ti,omap3" or
> +"ti,omap3450". If a bug was discovered on the original beagleboard
> +that required special workaround code during early boot, then a new
> +machine_desc could be added which implements the workarounds and only
> +matches on "ti,beagleboard".
The exact example above is "ti,omap3-beagleboard".
...
> +Most of this data is contained in the /chosen node, and when booting
> +Linux it will look something like this:
> +
> + chosen {
> + bootargs = "console=ttyS0,115200 loglevel=8";
> + initrd-start = <0xc800>;
> + initrd-end = <0xc820>;
HTML conversion issue here too.
...
> +The simplest case is when .init_machine() is only responsible for
> +registering a block of platform_devices. Platform devices are concept
are *a* concept
> +About now is a good time to lay out an example. Here is part of the
> +device tree for the NVIDIA Tegra board.
...
> + sound {
> + compatible = "nvidia,harmony-sound";
> + i2s-controller = <&i2s-1>;
> + i2s-codec = <&wm8903>;
> + };
I'm not sure if the bindings for ASoC devices are agreed upon well enough
yet to include that part of the example?
...
> +In the Tegra example, this accounts for the /soc and /sound nodes, but
> +what about the children of the soc node? Shouldn't they be registered
> +as platform devices too? For Linux DT support, the generic behaviour
> +is for child devices to be registered by the parent's device driver at
> +driver .probe() time. So, an i2c bus device driver will register a
see right margin:^^ an?
...
> +i2c_client for each child node, an spi bus driver will register
> +it's spi_device children, and similarly for other bus_types.
^^ its
> +It turns out that registering children of certain platform_devices as
> +more platform_devices is a common pattern, and the device tree support
> +code reflects that. The second argument to of_platform_populate() is
> +an of_device_id table, and any node that matches an entry in that
> +table will also get it's child nodes registered. In the tegra case,
> +the code can look something like this:
> +
> +static struct of_device_id harmony_bus_ids[] __initdata = {
> + { .compatible = "simple-bus", },
> + {}
> +};
> +
> +static void __init harmony_init_machine(void)
> +{
> + /* ... */
> + of_platform_populate(NULL, harmony_bus_ids, NULL);
> +}
> +
> +"simple-bus" is defined in the ePAPR 1.0 specification as a property
> +meaning a simple memory mapped bus, so the of_platform_populate() code
> +could be written to just assume simple-bus compatible nodes will
> +always be traversed. However, we pass it in as an argument so that
> +board support code can always override the default behaviour.
An example for I2C drivers enumerating their I2C child devices might be
educational, so as to push the description of the DT enumeration model
all the way through the entire tree.
...
--
nvpublic
___
linaro-dev mailing list
linaro-dev@lists.linaro.org
http://lists.linaro.org/mailman/listinfo/linaro-dev
RE: [PATCH 1/2] drivers: create a pinmux subsystem v3
Linus Walleij wrote at Monday, June 13, 2011 10:58 AM: > This creates a subsystem for handling of pinmux devices. These are > devices that enable and disable groups of pins on primarily PGA and > BGA type of chip packages and common in embedded systems. I'm a little confused by this version. In particular: * I don't see some changes that I thought we'd agreed upon during earlier review rounds, such as: ** Removing pinmux_ops members list_functions, get_function_name, get_function_pins; it seems odd not to push this into the pinmux core as data, but instead have the core pull it out of the driver in a "polling" function. ** Similarly, I'd asked for at least some documentation about how to handle the "multi-width bus" problem, but I didn't see that. * I'm confused by the new data model even more than before: ** How can the board/machine name pins? That should be a function of the chip (i.e. pinmux driver), since that's where the pins are located. A chip's pins don't have different names on different boards; the names of the signals on the PCB connected to those pins are defined by the board, but not the names of the actual pins themselves. ** struct pinmux_map requires that each function name be globally unique, since one can only specify "function" not "function on a specific chip". This can't be a requirement; what if there are two instances of the same chip on the board (think some expansion chip attached to a memory-mapped bus rather than the primary SoC itself). ** Perhaps primarily due to the lack of consideration in the documentation, I'm not convinced we have a clearly defined path to solve the "multi-width bus" issue. This needs to be a feature of the core pinmux API, rather than something that's deferred to the board/machine files setting up different function mappings, since it's not possible to solve purely in function mappins as they're defined today. Some minor mainly typo, patch-separation, etc. feedback below. ... > +Pinmux, also known as padmux, ballmux, alternate functions or mission modes > +is a way for chip vendors producing some kind of electrical packages to use > +a certain physical bin (ball, pad, finger, etc) for multiple mutually > exclusive s/bin/pin/ ... > +A simple driver for the above example will work by setting bits 0, 1 or 2 > +into some register mamed MUX, so it enumerates its available settings and s/mamed/named > +++ b/drivers/pinctrl/Kconfig ... > +config PINMUX_U300 > + bool "U300 pinmux driver" > + depends on ARCH_U300 > + help > + Say Y here to enable the U300 pinmux driver > + > +endif Shouldn't that portion be part of the second patch? > +++ b/drivers/pinctrl/Makefile ... > +obj-$(CONFIG_PINMUX_U300)+= pinmux-u300.o Same here. > +++ b/include/linux/pinctrl/machine.h > +/** > + * struct pinmux_map - boards/machines shall provide this map for devices > + * @function: a functional name for this mapping so it can be passed down > + * to the driver to invoke that function and be referenced by this ID > + * in e.g. pinmux_get() > + * @dev: the device using this specific mapping, may be NULL if you provide > + * .dev_name instead (this is more common) > + * @dev_name: the name of the device using this specific mapping, the name > + * must be the same that will return your struct device* s/that will return/as in/ ? > +++ b/include/linux/pinctrl/pinmux.h > +struct pinmux; > + > +#ifdef CONFIG_PINCTRL > + > +struct pinmux_dev; I think "struct pinmux_map" is needed outside that ifdef, or an include of . > +/** > + * struct pinmux_ops - pinmux operations, to be implemented by drivers > + * @request: called by the core to see if a certain pin can be muxed in > + * and made available in a certain mux setting The driver is allowed > + * to answer "no" by returning a negative error code That says "and made available in a certain mux setting", but no mux setting is passed in. s/a certain/the current/? Documentation for @free is missing here > +/** > + * struct pinmux_desc - pinmux descriptor, register this to pinmux subsystem > + * @name: name for the pinmux > + * @ops: pinmux operation table > + * @owner: module providing the pinmux, used for refcounting > + * @base: the number of the first pin handled by this pinmux, in the global > + * pin space, subtracted from a given pin to get the offset into the range > + * of a certain pinmux > + * @npins: the number of pins handled by this pinmux - note that > + * this is the number of possible pin settings, if your driver handles > + * 8 pins that each can be muxed in 3 different ways, you reserve 24 > + * pins in the global pin space and set this to 24 That's not correct, right; if you have 8 pins, you just say 8 here? The multiplier for N functions per pin is through list_functions, get_function_name, get_function_pins as I understand it. > +static inline int pinmux_register_mappings(struct pinmux_map const *map, > +unsigned
RE: [PATCH 1/2] drivers: create a pinmux subsystem v3
Linus Walleij wrote at Tuesday, June 14, 2011 8:26 AM:
> On Tue, Jun 14, 2011 at 1:28 AM, Stephen Warren wrote:
>
> > I'm a little confused by this version.
>
> Sorry, I'll try to clarify.
>
> > In particular:
> >
> > * I don't see some changes that I thought we'd agreed upon during earlier
> > review rounds, such as:
> >
> > ** Removing pinmux_ops members list_functions, get_function_name,
> > get_function_pins; it seems odd not to push this into the pinmux core
> > as data, but instead have the core pull it out of the driver in a
> > "polling" function.
>
> I don't find any particular discussion on that, did I miss something?
> It might be that I simply do not agree...
That was:
https://lkml.org/lkml/2011/5/19/365
https://lkml.org/lkml/2011/5/19/379
> Anyway, this is modeled exactly on how the regulator subsystem
> interact with its drivers to enumerate voltages. It seems intuitive
> to me, it's an established kernel design pattern that drivers know
> the hardware particulars, and so I don't get the problem here.
> Would you argue that the regulator subsystem has bad design
> too or is this case different?
Well, I guess it does do something like this on a much smaller scale.
I'm surprised this style exists; I'd expect the following:
driver:
pinmux_register_driver(
ops
list of pins
list of functions);
to be much simpler than:
driver:
pinmux_register_driver(ops)
core:
for loop:
ops->list_functions
ops->get_function_name
ops->get_function_pins
Still, this is something that doesn't affect the publically visible
client interface, and is easy to change in the future without a lot
of work, so probably not a huge deal.
> Can't you just send some patch or example .h file for the API
> you would like to see so I understand how you think about
> this?
Are your patches in git somewhere? It's much easier for me to pull
at present than grab patches out of email; something I certainly need
to fix...
> I might be thinking inside the box of my current design here so
> help me get out of it, I just have a hard time seeing how to
> do it.
>
> > ** Similarly, I'd asked for at least some documentation about how to
> > handle the "multi-width bus" problem, but I didn't see that.
>
> SORRY! Missed it.
>
> So sure that can be added, so something like this?
>
> A B C D E F G H
> +---+
>8 | o | o o o o o o o
> | |
>7 | o | o o o o o o o
> | |
>6 | o | o o o o o o o
> +---+---+
>5 | o | o | o o o o o o
> +---+---+ +---+
>4o o o o o o | o | o
> | |
>3o o o o o o | o | o
> | |
>2o o o o o o | o | o
> +---+---+---+---+---+
>1 | o o | o o | o o | o | o |
> +---+---+---+---+---+
>
> (...)
>
> On the botton row at { A1, B1, C1, D1, E1, F1, G1, H1 } we have something
> special - it's an external MMC bus that can be 2, 4 or 8 bits wide, and it
> will
> consume 2, 4 or 8 pins respectively, so either { A1, B1 } are taken or
> { A1, B1, C1, D1 } or all of them. If we use all 8 bits, we cannot use the SPI
> port on pins { G4, G3, G2, G1 } of course.
>
> (...)
>
> A simple driver for the above example will work by setting bits 0, 1, 2, 3,
> 4 or 5 into some register named MUX, so it enumerates its available
> settings
> and their pin assignments, and expose them like this:
>
> #include
>
> struct foo_pmx_func {
> char *name;
> const unsigned int *pins;
> const unsigned num_pins;
> };
>
> static unsigned int spi0_0_pins[] = { 0, 8, 16, 24 };
> static unsigned int i2c0_pins[] = { 24, 25 };
> static unsigned int spi0_1_pins[] = { 38, 46, 54, 62 };
> static unsigned int mmc0_1_pins[] = { 56, 57 };
> static unsigned int mmc0_2_pins[] = { 56, 57, 58, 59 };
> static unsigned int mmc0_3_pins[] = { 56, 57, 58, 59, 60, 61, 62, 63 };
>
> static struct foo_pmx_func myfuncs[] = {
> {
> .name = "spi0-0",
> .pins = spi0_0_pins,
> .num_pins = ARRAY_SIZE(spi0_1_pins),
> },
> {
> .name = "i2c0",
> .pins = i2c0_pins,
> .num_pins = ARRAY_SIZE(i2c0_pins),
> },
> {
> .name = "spi0-1",
> .pins = spi0_1_pins,
> .num_pins = ARRAY_SIZE(spi0_1_pins),
> },
> {
&g
