[U-Boot] Some Driver Model Ponderings

[Jon Loeliger]

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? :-)