[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Jon,

On 30 May 2014 08:49, Jon Loeliger <loeliger at gmail.com> wrote:
> Folks,
>
> I'd like to discuss the new Driver Model's parsing of the DTS file
> for the purposes of instancing and binding devices as I was not
> able to get the existing code to work anything like I was expecting.
>
> The current code only finds and binds the top-level nodes of the
> DTS file.  This leads to a bind function one-level above where it
> should be for each of the actual device nodes and an extra "parent"
> node that is empty.  For example, the gpio_exynos_gpio_bind()
> function here:
>
> http://git.denx.de/?p=u-boot/u-boot-x86.git;a=blob;f=drivers/gpio/s5p_gpio.c;h=8100d50ed2da686353c593ed90693c441cf24b4f;hb=refs/heads/us-gpioc
>
> Looks like this:
>
> 386 /**
> 387  * We have a top-level GPIO device with no actual GPIOs. It has a child
> 388  * device for each Exynos GPIO bank.
> 389  */
> 390 static int gpio_exynos_bind(struct device *parent)
> 391 {
> 392         struct exynos_gpio_platdata *plat = parent->platdata;
> 393         struct s5p_gpio_bank *bank;
> 394         const void *blob = gd->fdt_blob;
> 395         int node;
> 396
> 397         /* If this is a child device, there is nothing to do here */
> 398         if (plat)
> 399                 return 0;
> 400
> 401         bank = (struct s5p_gpio_bank *)fdtdec_get_addr(gd->fdt_blob,
> 402
> parent->of_offset, "reg");
> 403         for (node = fdt_first_subnode(blob, parent->of_offset); node > 0;
> 404              node = fdt_next_subnode(blob, node)) {
> 405                 struct exynos_gpio_platdata *plat;
> 406                 struct device *dev;
> 407                 int ret;
> 408
> 409                 plat = calloc(1, sizeof(*plat));
> 410                 if (!plat)
> 411                         return -ENOMEM;
> 412                 plat->bank = bank;
> 413                 plat->port_name = fdt_get_name(blob, node, NULL);
> 414
> 415                 ret = device_bind(parent, parent->driver,
> 416                                         plat->port_name, plat, -1, &dev);
> 417                 if (ret)
> 418                         return ret;
> 419                 dev->of_offset = parent->of_offset;
> 420                 bank++;
> 421         }
> 422
> 423         return 0;
> 424 }
>
> Why is this function being called once at the parent node, which
> then iterates over each device, instantiates and binds it?  Why
> isn't this function instead called once for each individual device
> as matched from the DTS?  Where did the compatible matching
> and check take place in this implementation?

Driver model works by looking up compatible strings in the top-level
nodes and binding a driver for each one it finds.

The exynos pinctrl device tree binding does not have a compatible
string in each of its banks. In fact it just has a single compatible
string at the level above the banks. So there seems to be no
alternative but to iterate through the banks binding devices as we go.

The node looks something like this:

    pinctrl at 13410000 {
        compatible = "samsung,exynos5420-pinctrl";
        reg = <0x13410000 0x0000011f>;
        interrupts = <0x00000000 0x67706330 0x000004a6>;
        gpc0 {
            gpio-controller;
            #gpio-cells = <0x00000002>;
            interrupt-controller;
            #interrupt-cells = <0x00000002>;
        };
        gpc1 {
            gpio-controller;
            #gpio-cells = <0x00000002>;
            interrupt-controller;
            #interrupt-cells = <0x00000002>;
        };

So we get a bind call on the pinctrl node and then bind each of the banks.


>
>
> Instead, I think it should be a recursive structure essentially
> identical in structure to the Linux of_platform_populate() function.
> There should be a compatible matching step, and then the
> call to bind the specific instance.
>
> Am I missing something here?  Or is this code that just needs to
> be developed further still?

Certainly this could be done, and it's a small change but this code
doesn't exist yet. I deliberately left this out of the implementation
until we have I2C implemented, or something similar. Then it will be
clearer what is needed here.

My concern is partly that access to the device may be mediated through
the parent and thus not discoverable by the child. As an example, the
Chrome OS EC driver can attached to I2C, SPI or LPC. The connection
needs to be made at the parent level, which provides a
'communications' layer for the generic driver.

So in short I think we need to address these things and make the
decisions as we go. For the same reason I didn't implement driver
model for SPL or pre-relocation (although I have a series out for the
latter now).

A lot of things will be clearer to me when I2C is ported over.

Regards,
Simon