Linus Walleij wrote at Thursday, June 16, 2011 6:47 AM:
On Wed, Jun 15, 2011 at 12:11 AM, Stephen Warren swarren@nvidia.com 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 on.
The mappings, provided by and specific to a particular board/machine would always expose only the exact configurations actually used on that board:
mapping device: "tegra-kbc" mapping name: "config a" mapping function list: row[7:4], row[3:0], col[3:0]
(note how I added a "mapping name" field here; more on this below. This is related to mapping and function names being different things)
As a single mapping that the driver will just get and use.
Note that if the driver is complex, and can dynamically switch between different setups at run-time on a specific board, then the mappings list might still contain multiple entries for the same device, which the device driver could get all of and switch between. However, I'd expect that to be pretty rare:
mapping device: "tegra-kbc" mapping name: "config a" mapping function list: row[7:4], row[3:0], col[3:0]
mapping device: "tegra-kbc" mapping name: "config b" mapping function list: row[3:0], col[3:0]
Note that this will be *quite* tricky to implement, since you have an array of undetermined size with elements of undetermined size in .functions = {"mmc0-1:0", "mmc0-3:2", "mmc0-7:4"}.
You will need atleast to move the functions out of the mapping something like this:
static char *mmc0_10[] = { "mmc0-1:0", "mmc0-3:2", "mmc0-7:4" };
static struct pinmux_map pmx_mapping[] = { { .functions = &mmc0_10, .functions_len = ARRAY_SIZE(mmc0_10); .dev_name = "tegra-sdhci.0", }, };
Which sort of kludges up the syntax for all the simple cases that will also have to add ARRAY_SIZE() and a separate string array for each of its single-function maps.
So it has the downside of complicating the simple maps.
Yes, you're right. I think I have a simpler solution that degenerates to the same syntax as in your current patch. Starting with your original:
static struct pinmux_map pmx_mapping[] = { { .dev_name = "tegra-sdhci.0", .function = "mmc0-1:0", },
(where devices look up mappings by ".function", among other options)
We then add a new "mapping name" field; you'll see why soon:
static struct pinmux_map pmx_mapping[] = { { .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .function = "mmc0-1:0", },
(where we now use ".map_name" for searching the list instead of ".function", which ties into my comment above about having explicit different sets of names for mapping entries and low-level pinmux driver internal function names.
We can still set ".map_name" = NULL and omit it in the simple case where drivers search based on ".dev_name" and don't specify any specific .map_name to search for, and don't have multiple options for mappings they can switch between.
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.
If there is only a single logical mapping, and hence .map_name is omitted, there could still be multiple entries in the list for the same dev_name; all entries with .map_name==NULL are considered the same name.
Putting the above in database terms, the primary key (albeit not unique) is the tuple (.dev_name, .map_name), and the set of .function values for all rows with that same primary key are logically part of the same mapping. (Yes, this isn't correct normalized form for a database, but it makes the simpler cases simpler to represent)
We can still have multiple .map_name options for a given .dev_name, by simply combining the previous two examples:
static struct pinmux_map pmx_mapping[] = { { .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .function = "mmc0-1:0", }, { .dev_name = "tegra-sdhci.0", .map_name = "4 bit", .function = "mmc0-1:0", }, { .dev_name = "tegra-sdhci.0", .map_name = "4 bit", .function = "mmc0-3:2", },
And as a final addition to this example, suppose we have a second MMC controller, it can also have a "2 bit" map_name entry, since I'd like to assert that ".map_name" be interpreted relative/within/for a given .dev_name in most cases; lookup only by .map_name without a .dev_name should be rare.
{ .dev_name = "tegra-sdhci.1", .map_name = "2 bit", .function = "mmc1-1:0", },
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.
struct pinmux, as returned by pinmux_get(), would need some adjustments to store either an array of map entries, or just the .map_name of the found entry/entries, so the loop could be repeated later.
What I think we need to understand at this point is what will be the most common case: that a mapping selects a single or multiple functions. If the case is rare I'd opt for my scheme (i.e. defining one function per bus width) to keep it simple, whereas if a vast majority of the function tables and mappings for say Tegra will be way simpler this way, it'll be worth the increase in complexity of the function mapping API.
So sorry but just to hammer home my point, the disadvantages of the pinmux driver itself (as opposed to the mapping list) aggregating multiple pins or groups-of-pins into functions for each supported combination are:
* Potential explosion of functions exposed.
* The same point, but think about a SW bit-banged bus consisting of 8 GPIOs for the bus and 100 pins on the device. Each permutation of 8 out of 100 is scary... I'd love a board to be able to represent a single mapping for a particular board's set of GPIOs in this case, but not for the pinmux driver to expose all possibilities!
* By having the pinmux driver expose the pins/groups in exactly the way the HW supports, the pinmux driver is purely a simple hardware driver, and doesn't know anything much beyond "program this pin/group to be this function". All the logic of aggregating sets of pins/groups-of-pins into mappings is defined by the board/machine/... when it sets up the mapping table. I like keeping the pinmux driver simpler, and having the boards describe functions in terms of which pins/groups should be set to which function, rather than picking for a pre-defined large list of all possibilities.
... although struct pinmux_map would probably need to grow a separate name field
I don't get this, the member "function" is a name of the function:
struct pinmux_map { const char *function; (...) };
Hopefully I explained this well above; it's the difference between a function name known to the pinmux driver vs. a mapping name known to the pinmux clients.
to match against pinmux_get's 2nd parameter, rather than assuming the driver's function names and the mapping's function names were the same.
Such a new name field would also introduce a second namespace that'd satisfy my next concern.
** 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.
Actually I don't really like the use of the concept of board and machine for chip packages either. But if I say that without devicetrees it could be something like arch/arm/plat-nomadik/package-db8500.c defining the pins. Then it is still coming from the outside, for a particular platform, for a particular chip package.
Then the init function in that file gets called on relevant systems.
As you can see in the example implementation for U300 I actually do this in the driver itself by including the machine.h file to that one. So this driver first registers pins, then functions.
I think there is broad consensus that this should come in from the device tree in the future. And if it comes from the device tree, it's still the say arch/arm/plat-nomadik/package-db8500.c file that looks up the pins from the device tree and registers them, just it gets the data from the outside.
I think discussion of DeviceTree here is a bit of a distraction; if the pinmux core APIs imply the correct data model, it doesn't seem especially relevant.
OK... maybe too much distraction here. All the DT noise makes me dizzy too :-P
For the pinmux drivers themselves, I expect the SoC's pinmux driver to define the set of pins available, and the set of functions available.
Yes, for those that map 1-to-1 for a certain platform.
Personally, I think in all cases.
For a pinmux driver that can apply to different platforms (e.g. the same register set applies to n different SoCs with different pin layouts), I think the pinmux driver itself can be parameterized with the mapping from register bits/fields to function names exported to the pinmux core. The parameterization could come from platform data, DeviceTree, ...
I'm a little bit sceptical due to the risk of getting into a situation where every implementation claims to be special and ending up having to consolidate a lot of stuff that really wasn't that special a few years down the road, like the ARM tree right now. But maybe that's our destiny so no big deal :-)
There will be a single static set of pins/functions for any Tegra 2 device. Tegra 3 will have a different set of pins/functions since the HW changed quite a bit in this area. As such, DeviceTree doesn't really help much, since there will always be a custom driver per SoC, and the data a given driver uses will not vary between boards, so DeviceTree helps little.
Yep I get it. s/device tree/board file/g and the same reasoning holds for anything coming from the platform/package/machine data.
On the other hand, if you've got a very generic pinmux controller, and it gets included in N (versions of) SoCs with the same register layout, but the actual pin names related to each register bit are different, then I can see it making sense for the pinmux driver itself to parameterize itself with data from DeviceTree.
We have that situation already in Ux500. c.f.: arch/arm/mach-ux500/pins-db5500.h arch/arm/mach-ux500/pins-db8500.h
Two ASICs, same set of hardware registers, but vastly different pin assignments.
OK, does the idea I floated a little above work; having the pinmux driver itself get its register->function-name mapping from some data structure provided to the pinmux driver?
As you'll no doubt realize, I think that works:-)
Either way though, this is purely an implementation detail within the pinmux driver, and not something the core would even be aware of.
OK with me for now atleast, I don't know about other people's ambitions.
That sounds like you agree:-)
However, I do think the board-specific list of struct pinmux_map will come from DeviceTree. This will vary from board to board with potentially little commonality. DeviceTree seems a great solution to keep all that varying data out of the kernel. The core pinmux code could well include the common code to parse the DeviceTree for this data.
Agreed.
Great.
** 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).
Namespace clashes are a problem but the way I see it basically a problem with how you design the namespace. It's just a string and the document is just an example.
The issue here is that the pinmux driver defines a single namespace for functions, and struct pinmux_map's definition passes that single namespace through, since the value of .function comes from that same namespace, and is also the search key for the list of driver-visible functions.
So this would be the case if you say had two identical chips on I2C, both of which could m[u]x some their pins.
Yes.
That (in the sense of the present design) would be an argument to mandate to always pass the function names from platform data to drivers that handle more than once instance, so the function names are always unique.
(This is more about understanding, really, tell me if I got it wrong.)
I think function names should be interpreted only within the context of a specific pinmux driver instance.
I think mapping table entry names should generally be used in conjunction with a device name for lookup, so it's clear which device's mappings you're searching for.
Now, the final question is, given a mapping entry:
{ .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .function = "mmc0-1:0", },
How do you know which pinmux driver to go to when activating the function?
I think the answer is to expand the mapping table again, to be:
{ .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .pmx_dev_name = "tegra-pinmux.0", .function = "mmc0-1:0", },
The above entry means:
For device "tegra-sdhci.0", when it wants to activate its pinmux mapping named "2 bit", (where that name is optional if there's only 1 for the device) go to pinmux driver names "tegra-pinmux.0", and activate function "mmc0-1:0" there.
Each registered pinmux driver would need a name. To cover the case of multiple instances of the exact same driver, the driver's name could e.g. encode I2C bus number and address for example "foochip@0.1a". I think that gpiolib names gpiochips along those lines? The naming that ASoC (ALSA for Systems on Chip; sound/soc/*) uses for I2C devices such as codecs works just like this.
As I alluded to above, if struct pinmux_map had a separate field for the search key (for pinmux_get's use) v.s. the set of driver functions that key maps to, then there can be separate namespaces, and a place to resolve conflicts:
struct pinmux_map_entry { struct pinmux_dev *pmxdev const char *function; // pmxdev's specific function name-space };
This looks scary to me: we're already trying to avoid putting struct device * into the map in favor of using dev_name to look it up.
How do you intend to get struct pinmux_dev * into the map entry?
Yes, having "struct pinmux_dev *" here isn't a great idea. Using a device's name works much better.
Remember that the map is a static thing that is defined at compile-time, whereas struct pinmux_dev * is something that appears at runtime.
It seems smarter to me to use some naming convention, but it definately need to keep track of instances, so we need something like the device_name() from the pinmux_dev:s struct device, that will be unique for each instance, then match on that.
OK, I think you're suggesting basically what I wrote above; my email reading lookahead buffer needs to be larger:-)
But maybe if you make a patch I can understand this better, I might just not see how this could work.
struct pinmux_map { const char *name; // global driver-visible function name-space struct device *dev;
This struct device *dev is curretly unused btw.
I even made my map a __initdata in the U300 case, and I think that should be the norm, this would only be useful in runtime, maybe with device trees.
Hmm. I think the mapping table ends up being used at runtime a lot; I think it's reasonable for drivers to be able to expect to pinmux_get() at any time. New pinmux devices could be added/removed at arbitrary times, e.g. if a hotplugged PCI or USB device contained pinmuxing capabilities.
const char *dev_name; int n_ functions; const struct pinmux_map_entry * functions;};
Thinking about this at a high level, this seems equivalent to the GPIO driver:
Sorry, not following. Which GPIO driver? Are you referring to gpiolib?
Yes.
There are two name-spaces there:
- One used by clients: A global name-space, equivalent to
pinmux_map.name.
- One used by GPIO drivers: A per-driver name-space, equivalent to
pinmux_map_entry.function.
... and the GPIO core maps from (1) to (2) above internally.
In the pinmux case, since everything is named by a string rather than a linear number range, the mapping process needs the pinmux_map_entry.function field, rather than just substracting a GPIO driver's base GPIO number.
I see you refer to the way that the global GPIO ID is mapped to an offset into the respective GPIO driver by subtracting the offset of each GPIO chip. (Correct me if wrong.)
Yes.
I think I understand what you mean, but I would claim that that an unsigned integer number space has quite different semantics from a namespace for them to be compared in that way. For example it is very simple and quick to map one into the other by a simple subtraction of an offset.
In this case, with the design above you need to know two namespaces instead of one when writing your platform data, and that makes things more complex in my view.
Well, I suppose two namespaces is indeed more complex than 1.
However, in the simple case of no duplicates, you can probably get away with using the same names in each namespace... And having the option to use two separate namespaces solves a bunch of problems I've talked about above.
So instead of using a simple 1D string "chip0::mmc0" to identify some function in a specific driver, you use the tuple ("chip0", "mmc0") and I think it's getting complex and hard to express as struct members, but that may be because I don't quite see how clever this is.
To me it seems plausible that the easiest way to do this is that if a driver wants a private namespace it just defines a separator such as "::" in above and we provide some simple string split function to get chipname and function name from that.
Non-surprsingly, that is exactly what the drivers/leds subsystem commonly does to identify LEDs on different chips:
Interesting. Having a mapping be:
{ .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .function = " tegra-pinmux.0::mmc0-1:0", },
Instead of:
{ .dev_name = "tegra-sdhci.0", .map_name = "2 bit", .pmx_dev_name = "tegra-pinmux.0", .function = "mmc0-1:0", },
Seems fine to me.
However, having the pinmux drivers statically define their prefixes in strings at compile-time doesn't seem correct; it prevents one from having multiple instances of the same pinmux/led driver; having separate fields makes this pretty easy, and avoids having to dynamically construct prefixed strings at runtime.
drivers/leds$ grep '::' * dell-led.c: .name = "dell::lid", leds-ams-delta.c: .name = "ams-delta::camera", leds-ams-delta.c: .name = "ams-delta::advert", leds-ams-delta.c: .name = "ams-delta::email", leds-ams-delta.c: .name = "ams-delta::handsfree", leds-ams-delta.c: .name = "ams-delta::voicemail", leds-ams-delta.c: .name = "ams-delta::voice", leds-clevo-mail.c: .name = "clevo::mail", leds-cobalt-qube.c: .name = "qube::front", leds-cobalt-raq.c: .name = "raq::web", leds-cobalt-raq.c: .name = "raq::power-off", leds-net48xx.c: .name = "net48xx::error", leds-wrap.c: .name = "wrap::power", leds-wrap.c: .name = "wrap::error", leds-wrap.c: .name = "wrap::extra",
So can't we use such a convention and some helpers like drivers/leds does instead of trying to actually match split the namespace to C struct members? It's still a namespace, it's just not expressed in C, but in string structure.
If the first part (before the "::") shall be unique per chip instance, that can be generated in runtime from the device name or some specific .init_name to make sure it does not vary too much.
If you're uncertain what to think about this I can implement this namespacing scheme for U300 so we have some example?
So to summarize there are two related areas of discussion here:
- Whether a pinmux map shall map one or 1..N functions
- How to handle per-driver instance namespacing of functions
In both cases I'm currently using simple strings and claiming that by namespacing these strings cleverly we can avoid complexity. So my answer to these are:
- Use several functions with ovelapping maps, just name
them differently 2. Use a string convention and namespace by using platform/machine/package data and string conventions such as a "::" separator
OK. I think I've address what I see as the disadvantages in those solutions in my rather long-wided discussions above:-)
While I *think* (and DO correct me!) that you would argue:
- Make it possible to map several functions to a single
device map
Yes.
- Namespace device instances by different map field
members referring to specific instances
Yes.
Is this correctly understood, even if we may not agree?
Thanks, Linus Walleij
-- nvpublic