[U-Boot] Some Driver Model Ponderings

Hi U-Booters,

Over the past few days, I had an off-line conversation with Simon Glass on the topic of the Driver Model. At his request, I've edited that conversation into this piece of email for the U-Boot List in the hopes that others in the community might also benefit.

Already, some documentation patches have been offered as a result.

http://patchwork.ozlabs.org/patch/352165/

But try to follow along. This is a conversation that occured over several back-and-forth pieces of email. I've edited and spliced. Hopefully, all the important pieces and parts are present without offense or malice.

I start with the plea:

Hi Simon,

I write to you directly in an effort to

A) Get a new GPIO driver into U-Boot using the new Driver Model B) Hopefully answer many lingering questions rapidly

the background:

Backing up a half step now... I need a PL061 ARM GPIO driver. I have exactly one GPIO I need to drive. But I have 7 GPIO blocks of 8 bits each. Rather than just hacking in yet another GPIO hackery into the general U-Boot fray, I thought I'd try the new driver model approach.

So I think I want, and have written, a simple PL061 driver that fits these standard operations:

static const struct dm_gpio_ops gpio_pl061_ops = { .request = pl061_gpio_request, .free = pl061_gpio_free, .direction_input = pl061_gpio_direction_input, .direction_output = pl061_gpio_direction_output, .get_value = pl061_gpio_get_value, .set_value = pl061_gpio_set_value, .get_state = pl061_gpio_get_state, };

Each of those routines takes a struct device that is mapped into an instance of the struct pl061_priv like so:

struct pl061_priv *priv = dev_get_priv(dev);

My priv structure is this:

struct pl061_priv { u32 regs; /* GPIO bit-masked I/O space, controller */ u32 pad_regs; /* Pad control */ u32 gpio_base; /* GPIO range implemented by this device. */ u32 gpio_count; };

Now, I want to build up 7 of these with different bases for the 'regs', and different segments of the global linear GPIO space, 8-bits apiece.

I am resonably convinced that the UCLASS_GPIO is going to help me do this. And is supposed to. It is some combination of the probe, bind, setup that I am not quite understanding yet.

At this point, Simon states that another reference implementation has been pushed:

I just pushed us-gpioc to

http://git.denx.de/u-boot-x86.git

which includes another conversion - drivers/gpio/s5p_gpio.c. It might be a bit closer to what you need.

So, my UCLASS related questions haven't all been answered yet, but I also had questions about the relationships between the various bits of data along the way: uc_priv, platdata, priv:

I'm not quite understanding the relationship of my "pl061_priv" data to the so-called platdata.

Simon clarifies that:

priv is your device's private data, used while the device is running (i.e. it is set up when the device is probed, and deleted when the device is removed)

platdata is like Linux platform data. It provides the information needed to start up the device - like its register addresses or configuration options.

I make two observations about those two paragraphs that then go on to cause a new documentation patch to be submitted. My first observation is that nowhere in the as-yet-written documentation abou the Driver Model is there any statement about the lifetime of the data pieces. Until that first paragraph. So I ask for more complete data-lifetime documentation.

Secondly, I also observe that there is no data-flow documented. What is the sequence of data as it transitions through the various bind, probe, use, and release phases?

Simon offers to document some data-lifetime, but quotes some existing platdata documentation too:

" platdata (data which tells the driver how to operate on a particular platform)"

Which just sets me off:

This is *exactly* the wording I am arguing against. :-) That uses the same word to define the word. We need *other* words to describe what is meant by "platform data" in this context. Something like what you wrote in the mail to me:

Examples of platform data might include: - the base address of the IP blocks register space - configuration options - board-level physical switch settings - something else obvious - when using DTS, data parsed out of the device node

And perhaps a description of why it is needed in the first place:

The device driver needs instance data for about a device's running state but may not be able to determine it. The platform data convey's the specific device setup data to an instance of the private device driver data that it will not otherwise be able to determine on its own.

Ultimately, all that crap ends up in a pretty good doc patch!

Anyway, from my original piece of mail, I ramble on:

And I'm not understanding how I get my uc_priv for GPIOs to do the glue layer for what I need. And then when I think I get it, I see that after the bind step, all the GPIOs get renumbered! Why? I think I wanted to manually set those up directly myself. And, furthermore, since I really only have *one* GPIO I need to manipulate, I don't really need to instantiate all 7 instance, but just the one instance that holds the one GPIO pin I need. (Which happens to be in Bank 3.)

Simon replies:

The renumbering is done by the uclass. Provided that you set up you devices correctly this should just work, and will give you the numbering you wanted anyway.

I think this issue of renumbering lingers and needs to be addressed. I'm not happy with the renumbering, so I push back:

I don't want a renumbering. I want to be able to declare *just* the one GPIO I need. It happens to be in bank 3 to handle the global GPIO space 24 through 31. I need GPIO 31. But if the renumbering happens, it will need 0 -7, 8-15, 16-23 in the uclist prior to bank 3 for GPIOs 24-31. I want one uclass list item to hold the one GPIO bank that I need for the range 24-31. But I *also* want the ability to allow a future GPIO bank addition to seemlessly fit into the list for bank 6 when I need to add a GPIO from that bank too. The uclass should happily handle a sparse representation of the list of banks and global GPIO ranges. The thing stopping that is willie-nillie renumbering that isn't even necessary in the first place.

If something needs to renumber, or for that matter, renumber according to a *different* scheme, why not make an optional uclass call-out function for "post-processing" and identify where its lifetime and sequencing it takes place.

Simon offers:

Well that's the way it works at present. I'm not sure why you wouldn't just support all the GPIOs from the start - that way you can upstream the code!

Also note that you can give GPIOs bank names A, B, C, D, etc. which might help you.

Which again, just torques my nipples. So I rant:

Wow. Seriously? I don't even know where to begin with that response. This isn't a matter of upstreaming. I get that. This isn't about upstreaming. And upstreaming should never be the justification for leaving bad code design in place. In fact, this is exactly the opposite reasoning at work here. That is, I'm trying to get this driver upstreamed (said that from the onset of this discussion), but the renumbering behind my back will get in the way.

This is a problem with the code performing totally unexpected behavior in a non-obvious, behind your back and broken way.

Let's try an analogy with the Linux Kernel IRQ scheme. You might think of this UCLASS GPIO scheme as being quite similar to the linear mapping space of the irqchips. I set up a series of small IRQ IP blocks that handle ranges of 32 IRQs, which in turn are mapped onto a larger, global space of IRQs maintained by the Linux Kernel. If I set up a series of IRQ blocks, mapping each one to IRQs:

Bank 0 is IRQ 64-95 Bank 1 is IRQ 0-31 Bank 2 is IRQ 32-63

and as I finished the last registration of the irqchip the kernel came along and remapped them behind my back as this:

Bank 0 is IRQ 0-31 Bank 1 is IRQ 32-63 Bank 2 is IRQ 64-95

there would be hell to pay!

So what the kernel did was provide a non-static mapping for the IRQs and irqchips. They allocatge the IRQs on the fly and allow each irqchip to make its own mapping. That way they never have to worry about re-organizing the global IRQ space, and never have to worry about the individual irqchip space needing to be shuffled as well.

I assume you can see where I am headed here now. The problem with our proposed DM for GPIOs is that the global space is a fixed linear space. You have to honor the original registration contract when the uc_privs were created and filled in.

If you want to *sort* the uc_priv blocks into a nicely orderd list so that, say, lookup operations might be faster, that is fine. Maybe even good.

But you can't renumber the regions and ranges behind my back. Unless I am completely missing something here, doing so breaks the association between the UC_GPIO class range mappings and the corresponding base register of the IP block.

So blah. We still need to address the capricious renumbering in the UC_GPIO space.

Simon explains:

Fair enough, but the GPIO uclass does not support dis-contiguous ordering.

The numbering is owned by the board not the driver. For example, another board with the same SoC as you are using might have an I2C expander with 16 GPIOs. It might want these GPIOs to go first or last. Neither driver can determine the ordering. Also the intent is that you should use names in your U-Boot scripts.

I'm not sold here yet. Perhaps we are talking past each other here. The GPIO class *could* support both discontinguous and sparse sets. In fact it would if only they weren't renumbered capriciously.

My original confusion continues with so many "So, ..." issues:

So, I saw your patch for the tegra GPIO conversion. Well timed! But it leaves me a bit baffled due to your apparent two-layers of PORTs and BANKS, and plat data. Some of which I don't think I should need. (?)

So, I read through your gpio_tegra_bind(), and I don't understand what the "parent" device is. Where'd that come from? Who called this with what arguemnt? All leading to what is really happening in that device_bind() call?

Simon says(1):

The parent device is empty - it has no actual GPIOs, but is just a parent for all the real GPIOs devices. Driver model supports child devices.

I'm still baffled on the "artificial parent" issue:

Yes, I get that. And an I2C bus might have a I2C MUX device in the mix as well, and etc. And a DMA controller might have N sub-channel devices, etc.

I don't understand how you are creating and organizing what should be (perhaps only in my case) just a list of 7 UC_GPIO nodes. I think the UC_CLASS for GPIOs has 7 allocated uc_priv instances in a list, one for each of my 7 banks of GPIOs. There is no hierarchy needed anywhere here, right?

It looks to me like you have an extra UC_GPIO node, with no actual {uc_priv->gpio_base, ->gpio_count} set on it, so that it just occupies an extra instance on the UC_CLASS UC_GPIO list. And, I think, the only reason for that node is to house a platdata structure that has a parent base register.

(Which, in the case of the s5p GPIO code you pointed me toward, baffles me a bit: I expected to see the base reg for each block parsed out of the DTS node for that block. Instead it looks like they are formed by doing the "bank++" to step through large register block structures for each bank. Urg.)

Simon confirms my interpretation and the "bank++" approach, but unfortunately doesn't explain either.

Yes that's right - it is just a parent device, with no actual GPIOs in it.

and:

That's the way the hardware is implemented on Exynos (and Tegra BTW).

Hrm.

Meanwhile, another "So,..." issue from my original mail:

So, where did I get multiple uc_priv structures? One for each of my needed 7 instances? Do I allocate them in a bind function? I thought I wanted something like this, but I need one uc_priv for each instance still:

int gpio_pl061_bind(struct device *dev) { struct gpio_dev_priv *uc_priv = dev->uclass_priv; int bank;

for (bank = 0; bank < RAPID_N_GPIO_BANKS; ++bank) { /* Is uc_priv allocated here? */ uc_priv->gpio_base = bank * PL061_GPIOS_PER_BANK; uc_priv->gpio_count = PL061_GPIOS_PER_BANK; } return 0; }

Simon points out several flaws with my approach:

dev->uclass_priv is allocated for you before probe() is called. bind just means that driver model knows about the device - it doesn't mean that it is ready for use yet. You mustn't touch uc_priv in the bind function since it doesn't exist yet.

and notes that the two uc_priv-> assignments instead belong in the probe() function. I think I understand and write:

OK, But then how does the probe code know what bank to operate on? And the answer to that now appears to be "via the platdata." Right?

And who/where was platdata allocated and filled in? In the bind, right?

So bind() first, get reg base and global GIO base. Fill those into plat data. Then probe() where plat-data is copied into both a newly allocated priv, and the uc_priv GPIO fields.

I am not sure when or how the bind() function is called:

Will this bind be called once, or somehow 7 times?

And cryptically, Simon replies:

Well if you call device_bind() you can create new devices, which is what the tegra/exynos drivers do.

And I am now adding a bit of explanation about this comment that I think I have deduced after reading the as-yet-to-be-written new documentation patch that Simon will have submitted later.(2)

Here's the unwritten thing: I think your driver bind() function is supposed to call the lower-layer device_bind().

Simon then adds:

bind() creates the struct device. It puts in the platdata pointer if it has one (i.e. if there was a U_BOOT_DEVICE() declaration). For device tree, it doesn't do that - see ofdata_to_platdata.

and offers a doc patch to clarify this some more.

OK, so, on to compiling issues....

First off, it appears that I will need to define my new CONFIG_GPIO_PL061 to get my driver file. Good. Got that. And then also CONFIG_DM_GPIO so that I get the Driver Model GPIO uclass layer. Good. I want that generic mapping of global GPIO numbers into my specific device... Check.

But then it wants me to also suddenly define CONFIG_SANDBOX_GPIO_COUNT. Whichi I think is broken. I should never have to define something for another configuration (sandbox). If I have to somehow accommodate some new unit testing scenario, that should be something *else* standalone.

And somehow I am missing a bunch of symbols still too: uclass_get, uclass_first_device, dev_get_prand weirdly iv, all the sandbox GPIO functions too.

Oh, right, I also, also need CONFIG_DM. Hrm. That causes list code to be compiled and it looks totally sloppy and has compilation errors all over it still. Still getting test/dm/gpio.o in the mix and likely shouldn't.

And, about the time I sent that rambling series of issues, Simon was furiously developing this series of patches:

Yes, this is a bug. You should apply the driver model patches from my tegra series v3, for example:

http://patchwork.ozlabs.org/patch/347491/

And yes, applying the patch "dm: Allow driver model tests only for sandbox" was vastly helpful.

Mid-conversation, I trump up a few new questions:

- When in the overall U-Boot flow are the devices bound and subsequently probed to make them available?

- The DTS file. I have a DTS that I am passing on to Linux *through* U-Boot on the bootz command line. Is the same DTS file that being used to setup and configure U-Boot now? Or does U-Boot have its *own* (perhaps different) DTS? Specifically, are the GPIO nodes in my Linux DTS file the *same* nodes that might now be probed by U-Boot? If so, then the compatibles have to agree. That is, I'll have to use "arm,pl061". This sort of note also needs to be present in the DM README.

Simon responds:

Yes you should use the same file. Perhaps README.fdt-control would be a good place for that. We want U-Boot to use the same bindings as Linux.

But I think my point was missed. It shouldn't be "the same file" with "the same data". I think it should be the actual DTB as already stored in flash waiting to be handed off to Linux.

I wrote:

That's not what is happening, though. Yesterday I worked through the craziness that is CONFIG_OF_{EMBED,SEPARATE}. It caused me to create a duplicate DTS, totally different than the one that I pass to Linux already.

My Linux DTB is over in my flash, in an area of memory that is perfectly accessible to U-Boot if it wanted to read it. But it thinks it can't. So I have an OF_EMBED or OF_SEPARATE choice. Either of those cause an entirely new copy of the DTS, located in the U-Boot source tree now, to be DTC'ed and glued to the u-boot image. I don't want that at all.

And having now worked through the issues around CONFIG_OF_SEPARATE and CONFIG_OF_EMBED, I think there needs to be a third alternative that says something like CONFIG_OF_SHARED_IN_FLASH. Dunno. More work.

I've also had some difficulty gettin the DTB to be parsed. Further investigation revealed that my DTB is being found and parsed, but that only the "top level" nodes are being used for "bind"operations. As my GPIO nodes are second-level, under an SOC node, they are not being found and bound. Again, more work there.

I close with a plea:

You know. :-)

Any help for the weary?

Thanks, jdl

And so does Simon:

It would be good to cc this to the list - please considering doing that so that everyone benefits. You are not missing anything obvious and I'm sure others will benefit from your efforts.

Regards, Simon

I offer to do the dirty work:

Simon, thank you! This really helped me. I don't want to sign this address up on the U-Boot list, so I will cut-n-paste this onto my gmail address and post a *slightly* modified version to the list!

And, with this mail, I have answered Simon's final plea:

Let's move this discussion to the list, as the issues you raise above are much better discussed in public. Driver model is in the very early stages and others will have comments also.

Regards, Simon

Thanks, jdl

(1) -- See what I did there? (2) -- Are you following my timeline still? :-)