[U-Boot] Driver Model and DTS Parsing

[U-Boot] Driver Model and DTS Parsing

[Jon Loeliger]

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?


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?

Thanks,
jdl

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Jon,

On 30 May 2014 08:49, Jon Loeliger <loeliger@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

[U-Boot] Driver Model and DTS Parsing

[Jon Loeliger]

On Mon, Jun 2, 2014 at 8:26 PM, Simon Glass <sjg@chromium.org> wrote:
>
> Driver model works by looking up compatible strings in the top-level
> nodes and binding a driver for each one it finds.

I get that.

I'm saying that isn't sufficient.

> 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.
>
> So we get a bind call on the pinctrl node and then bind each of the banks.

Right.  That's fine for this board and DTS.

However, the existing parsing structure isn't sufficient yet.

>> 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,

Excellent.  I'm saying it should be done.  Specifically, a recursive,
top-down implementation will allow the right parent node to grab
iterative control and handle the sub-nodes that can't handle themselves.
Like the Linux DTS parsing, we'll need to add handling of bus nodes.

But we have to put in place the top-down structure so that we *do*
iterate through the parents properly and at multiple levels.

> ... and it's a small change but this code
> doesn't exist yet.

OK, I'll play that game:  It's an important change and it
needs to exist soon. :-)

> 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.

Right.  A top-down approach will allow for that sort of handling.

> 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.

Sure.  In the meantime, the GPIO model suffers.  Understood. :-)

So two procedural questions:

First, is there a DM repo.  No, I don't mean an "x86 repo gathering DM" patches.
I mean an actual repo with a DM moderator?  I ask because I am carrying around
patches that are going to take for-*ever* to hit mainline...  You know.

Second, how would you like to advance the DM's DTS parsing infrastructure?
Do you want me to take a crack some patches?  Would you rather do it?
Can we get a common basis of patches (repo) somewhere?

> Regards,
> Simon

Thanks,
jdl

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

+Stephen

Hi Jon,

On 3 June 2014 07:39, Jon Loeliger <loeliger@gmail.com> wrote:

> On Mon, Jun 2, 2014 at 8:26 PM, Simon Glass <sjg@chromium.org> wrote:
> >
> > Driver model works by looking up compatible strings in the top-level
> > nodes and binding a driver for each one it finds.
>
> I get that.
>
> I'm saying that isn't sufficient.


> > 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.
> >
> > So we get a bind call on the pinctrl node and then bind each of the
> banks.
>
> Right.  That's fine for this board and DTS.
>
> However, the existing parsing structure isn't sufficient yet.
>
> >> 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,
>
> Excellent.  I'm saying it should be done.  Specifically, a recursive,
> top-down implementation will allow the right parent node to grab
> iterative control and handle the sub-nodes that can't handle themselves.
> Like the Linux DTS parsing, we'll need to add handling of bus nodes.
>
> But we have to put in place the top-down structure so that we *do*
> iterate through the parents properly and at multiple levels.
>
> > ... and it's a small change but this code
> > doesn't exist yet.
>
> OK, I'll play that game:  It's an important change and it
> needs to exist soon. :-)
>

Linux has created all sorts of opaque data structures in the device world.
I am rather hoping that with driver model we can keep a lot of things
attached directly to 'struct udevice' and not have every uclass have lots
of private inaccessible data. An example is GPIO banks. Yes we could
introduce a secondary data structure for this but then no one can find the
GPIO banks, driver model's commands can't list them and everything becomes
a special case for GPIOs.

The uclass idea helps a lot - by categorising devices according to type,
and the operations they support, it should be possible for devices to at
least know what they are talking to.

I hit this badly when converting the charger manager code in Linux to
device tree. I ended up adding lots of special case functions and new APIs
just to be able to find the devices for battery, charger, etc. Partly this
is because Linux apparently has no way to find a device from a node, and
partly that is because of all the opaque second-level data structures which
appear.


> > 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.
>
> Right.  A top-down approach will allow for that sort of handling.
>
> > 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.
>
> Sure.  In the meantime, the GPIO model suffers.  Understood. :-)
>

I can't see why in the case you bring up this feature is important for
GPIOs. I suggested you add an amba driver. A perfectly reasonable thing for
an amba driver to do is to scan its peripherals and bind them. You could
add a helper function for doing that perhaps? The good thing is that if the
amba bus needs any configuration or deals with interrupts you have
somewhere to put that logic.


>
> So two procedural questions:
>
> First, is there a DM repo.  No, I don't mean an "x86 repo gathering DM"
> patches.
> I mean an actual repo with a DM moderator?  I ask because I am carrying
> around
> patches that are going to take for-*ever* to hit mainline...  You know.
>

Not really. We could create one easily enough. I'll send an email.


>
> Second, how would you like to advance the DM's DTS parsing infrastructure?
> Do you want me to take a crack some patches?  Would you rather do it?
> Can we get a common basis of patches (repo) somewhere?
>

Please go ahead, but also consider what I have said above. I really want to
keep things simple and visible to the 'dm tree' command, etc.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 06/03/2014 10:04 AM, Simon Glass wrote:
> +Stephen

I don't think there's anything actionable for me in this email, although
I guess I'll chime in on a couple of points:

I agree that the current way U-Boot parses DT is completely inadequate.
The only way to parse it is to take a top-down recursive approach, with
each node's driver initiating the parsing of any relevant child nodes.
In other words, exactly how Linux (and likely *BSD, Solaris, ...) do it.

I really don't understand the hang up with GPIOs. Here are the possible
HW situations as I see them:

1)

A single GPIO controller HW module, represented as a single DT node.

This should be: One node in DT. One DM device. One bind call (assuming
that's the equivalent of Linux's probe()).

2)

A set of completely separate HW modules, each handling N GPIOs.

This should be: N nodes in DT. N DM devices. N bind calls.

3)

A single HW module that's represented in DT as a top-level node for the
HW module and arbitrarily has N child nodes for some arbitrary bank
concept within the HW module:

This should be: 1 (top-level) node in DT, N child nodes in DT, 1 or N DM
devices, 1 bind call (for just the top-level node). The bind call can
choose whether it creates 1 single DM device object for the top-level
node, or 1 for each of the child node that it manually parses without
additional bind calls. That's an implementation detail in the driver.

Note that Tegra should fall into case (1) above. I'm not familiar enough
with Exynos HW (which was mentioned in the email I'm replying to but
didn't bother quoting) to have an opinion re: which approach is most
suitable for it.

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 3 June 2014 12:33, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/03/2014 10:04 AM, Simon Glass wrote:
>> +Stephen
>
> I don't think there's anything actionable for me in this email, although
> I guess I'll chime in on a couple of points:
>
> I agree that the current way U-Boot parses DT is completely inadequate.
> The only way to parse it is to take a top-down recursive approach, with
> each node's driver initiating the parsing of any relevant child nodes.
> In other words, exactly how Linux (and likely *BSD, Solaris, ...) do it.

See the other thread - that has been my intention all along and is why
I avoided adding this to driver model. I've ended up with a helper
function only in the implementation I'm fiddling with at present.

>
> I really don't understand the hang up with GPIOs. Here are the possible
> HW situations as I see them:
>
> 1)
>
> A single GPIO controller HW module, represented as a single DT node.
>
> This should be: One node in DT. One DM device. One bind call (assuming
> that's the equivalent of Linux's probe()).
>
> 2)
>
> A set of completely separate HW modules, each handling N GPIOs.
>
> This should be: N nodes in DT. N DM devices. N bind calls.
> 3)
>
> A single HW module that's represented in DT as a top-level node for the
> HW module and arbitrarily has N child nodes for some arbitrary bank
> concept within the HW module:
>
> This should be: 1 (top-level) node in DT, N child nodes in DT, 1 or N DM
> devices, 1 bind call (for just the top-level node). The bind call can
> choose whether it creates 1 single DM device object for the top-level
> node, or 1 for each of the child node that it manually parses without
> additional bind calls. That's an implementation detail in the driver.
>
> Note that Tegra should fall into case (1) above. I'm not familiar enough
> with Exynos HW (which was mentioned in the email I'm replying to but
> didn't bother quoting) to have an opinion re: which approach is most
> suitable for it.

Thanks for this summary which is useful.

device_bind() is how child devices are created, so I don't think we
want to avoid using that. What's the point? How else are we going to
allocate a device?

I've basically settled on option 3 for now, with the device defined as
a 'GPIO bank'. We then put the banks together (each can be named) to
support all GPIOs on the SoC. Exynos happens to have pinctrl
definitions for each bank, so we can iterate through these calling
device_bind() for each bank. But note that only the top-level pinctrl
has a compatible string, so we cannot call device_probe() on the banks
- they have no compatible string so don't exist as far as driver model
is concerned. Anyway they aren't top-level nodes.

Tegra doesn't have much in the device tree for GPIOs - it seems to be
all hard-coded in the software. So I ended up with the code you saw
which just iterates over a known number of banks, creating a device
for each.

I don't want to create a separate data structure for 'gpio chip' like
Linux for reasons I think I mentioned (briefly it adds a level of
indirection, creates an unnecessary structure, hides that structure
from 'dm tree' and the like, and sets a precedent of lots of little
private data structures that are opaque to the poor user looking at
what is in the system). Or at least I'd like to delay that until it is
strictly necessary. Let's keep it all visible to driver model, and
also save having code we really don't need.

I hope that clarifies things.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 06/11/2014 10:55 PM, Simon Glass wrote:
...
> Tegra doesn't have much in the device tree for GPIOs - it seems to be
> all hard-coded in the software. So I ended up with the code you saw
> which just iterates over a known number of banks, creating a device
> for each.

That still sounds wrong. Tegra HW has a single GPIO controller that
exposes a bunch of GPIOs. It isn't logically divided into banks or any
other construct that is multi-level  Although the naming of the
individual GPIOs does call something a bank, that's just a name of a
register, not separate HW blocks. It's just going to be confusing to
users if the U-Boot representation doesn't match what the HW actually has.

There's zero extra indirection caused by SW correctly describing the HW
as a single bank. I have absolutely no idea what you mean my extra
indirection here; any time there is a driver for a GPIO, you call a
function to set a GPIO. That doesn't change based on whether there are
32 or 1 GPIO controller drivers.  The only difference is how many
drivers you have to search through to find the right one. For Tegra at
least, I'm arguing for 1 driver to match the 1 HW module.

DT is supposed to represent the differences between boards more than the
differences between SoCs. Anything that the driver can reasonably derive
from the compatible value shouldn't be represented in the DT. That's why
the Tegra GPIO DT node just has a compatible value, register address,
and list of interrupts. Nothing more is required. If anything else were
put in DT, you'd end up just wasting time parsing from DT static data
that could just be in the driver.

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/11/2014 10:55 PM, Simon Glass wrote:
> ...
>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>> all hard-coded in the software. So I ended up with the code you saw
>> which just iterates over a known number of banks, creating a device
>> for each.
>
> That still sounds wrong. Tegra HW has a single GPIO controller that
> exposes a bunch of GPIOs. It isn't logically divided into banks or any
> other construct that is multi-level  Although the naming of the
> individual GPIOs does call something a bank, that's just a name of a
> register, not separate HW blocks. It's just going to be confusing to
> users if the U-Boot representation doesn't match what the HW actually has.

I'm getting back to this as I just re-issued the series.

I don't see the mismatch you are referring to here. U-Boot people are
used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
also.

>
> There's zero extra indirection caused by SW correctly describing the HW
> as a single bank. I have absolutely no idea what you mean my extra
> indirection here; any time there is a driver for a GPIO, you call a
> function to set a GPIO. That doesn't change based on whether there are
> 32 or 1 GPIO controller drivers.  The only difference is how many
> drivers you have to search through to find the right one. For Tegra at
> least, I'm arguing for 1 driver to match the 1 HW module.

OK let me explain a bit more.

At the moment, the GPIO uclass supports a single GPIO bank, defined as
something that has a name, like A, or B. Within that bank there can be
several individual GPIO lines. This is what the Tegra TRM refers to as
A0, A1, B0, B1, etc.

Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
device, we would need to redo the uclass to support this. It would
need to support having more than one bank (a one-to-many relationship)
and thus we would need a second-level data structure to hold the bank
names of each bank. In that case each GPIO device would hold a list of
banks, each bank having a set of GPIOs. The 'gpio' command would need
to query the device for the list of available banks and use that in
decoding its command line parameters.

The list of banks is the secondary data structure that I am referring
to, and is what I would like to avoid for now, given that in U-Boot
devices can have children. It may be in the future that we end up
going that way, but so far I would prefer to avoid secondary data
structures and keep things really simple.

>
> DT is supposed to represent the differences between boards more than the
> differences between SoCs. Anything that the driver can reasonably derive
> from the compatible value shouldn't be represented in the DT. That's why
> the Tegra GPIO DT node just has a compatible value, register address,
> and list of interrupts. Nothing more is required. If anything else were
> put in DT, you'd end up just wasting time parsing from DT static data
> that could just be in the driver.

This is fine, although it is entirely a trade-off between code and
data. Some SoCs use the device tree to specify differences between the
SoCs (e.g. pinmux on exynos) and some don't. There doesn't seem to be
a hard-and-fast rule. In this case I was just expressing the fact that
the device tree is not really used for the GPIO devices on Tegra.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 07/30/2014 09:26 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/11/2014 10:55 PM, Simon Glass wrote:
>> ...
>>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>>> all hard-coded in the software. So I ended up with the code you saw
>>> which just iterates over a known number of banks, creating a device
>>> for each.
>>
>> That still sounds wrong. Tegra HW has a single GPIO controller that
>> exposes a bunch of GPIOs. It isn't logically divided into banks or any
>> other construct that is multi-level  Although the naming of the
>> individual GPIOs does call something a bank, that's just a name of a
>> register, not separate HW blocks. It's just going to be confusing to
>> users if the U-Boot representation doesn't match what the HW actually has.
>
> I'm getting back to this as I just re-issued the series.
>
> I don't see the mismatch you are referring to here. U-Boot people are
> used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
> also.

The mismaatch is that in HW, there is a single GPIO controller that has 
a large number of GPIOs, not a number of GPIO controllers that each has 
a smaller number of GPIOs.

U-Boot's commands/APIs/... should model this directly; a single 
controller object that has a large number of GPIOs within it.

As such, an example user-visible GPIO command needs to be roughly 
something like:

# using integer IDs
gpio set tegra 128
          ^^    ^^ (controller instance name) (GPIO ID)
or:

# using names within the controller
gpio set tegra PQ0
          ^^    ^^ (controller instance name) (GPIO name)

(note that there must be separate controller ID and GPIO ID parameters 
in the commands in order to e.g. differentiate between 2 instances of 
the same I2C GPIO expander chip; something like pca9555 at a0@i2c0, 
pca9555 at i2c1@a4)

not:

gpio set tegraP 10
          ^^     ^^ (hypothetical bank name) (GPIO ID within bank)

or:

gpio set P10
          ^^ GPIO name without any controller ID

>> There's zero extra indirection caused by SW correctly describing the HW
>> as a single bank. I have absolutely no idea what you mean my extra
>> indirection here; any time there is a driver for a GPIO, you call a
>> function to set a GPIO. That doesn't change based on whether there are
>> 32 or 1 GPIO controller drivers.  The only difference is how many
>> drivers you have to search through to find the right one. For Tegra at
>> least, I'm arguing for 1 driver to match the 1 HW module.
>
> OK let me explain a bit more.
>
> At the moment, the GPIO uclass supports a single GPIO bank, defined as
> something that has a name, like A, or B.

The GPIO bank name should just be "Tegra". The Tegra TRM specifies a 
single GPIO controller, in the address map etc., and there should be a 
single U-Boot object that represents it.  Really the phrase "bank" in 
U-Boot needs to be replaced with "controller".

 > Within that bank there can be
> several individual GPIO lines. This is what the Tegra TRM refers to as
> A0, A1, B0, B1, etc.

While the TRM does use the phrase "bank", I believe this is just a 
collision with the term you happened to choose. It's not used for the 
same semantic purposes. There's no intent to divide the Tegra GPIO 
controller into a bunch of logically separate HW blocks. "bank" in the 
TRM is just a convenient word to refer the fact that more than 32 GPIOs 
are supported, so they don't all fit into a single 32-bit register.

So, the semantics of the HW are:

A single GPIO controller named "tegra".

Within that, there are a bunch of GPIOs. Each has a number e.g. 0..250 
(for Tegra124) and a name (PA0, PA1, ... PA7, PB0, PB1, ..., PFF2). 
Users should be able to refer to those GPIOs either by integer ID, or by 
name.

To support this, the GPIO uclass would need:

* A string name for the controller

* A set of functions/ops to manipulate the GPIOs (e.g. set input, set 
output, set output value) that accept integer IDs as the parameter for 
the GPIO ID.

* If GPIOs have names as well as numbers, an optional function to 
convert a string name to an integer GPIO ID.

> Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
> device, we would need to redo the uclass to support this.

No you wouldn't. Just put all the GPIOs into a single uclass instance. 
For naming, you can have the optional string->int conversion function in 
the uclass, or perhaps just ignore names (the kernel operates on 
integers for GPIOs...).

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/30/2014 09:26 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> On 06/11/2014 10:55 PM, Simon Glass wrote:
>>> ...
>>>>
>>>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>>>> all hard-coded in the software. So I ended up with the code you saw
>>>> which just iterates over a known number of banks, creating a device
>>>> for each.
>>>
>>>
>>> That still sounds wrong. Tegra HW has a single GPIO controller that
>>> exposes a bunch of GPIOs. It isn't logically divided into banks or any
>>> other construct that is multi-level  Although the naming of the
>>> individual GPIOs does call something a bank, that's just a name of a
>>> register, not separate HW blocks. It's just going to be confusing to
>>> users if the U-Boot representation doesn't match what the HW actually
>>> has.
>>
>>
>> I'm getting back to this as I just re-issued the series.
>>
>> I don't see the mismatch you are referring to here. U-Boot people are
>> used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
>> also.
>
>
> The mismaatch is that in HW, there is a single GPIO controller that has a
> large number of GPIOs, not a number of GPIO controllers that each has a
> smaller number of GPIOs.
>
> U-Boot's commands/APIs/... should model this directly; a single controller
> object that has a large number of GPIOs within it.
>
> As such, an example user-visible GPIO command needs to be roughly something
> like:
>
> # using integer IDs
> gpio set tegra 128
>          ^^    ^^ (controller instance name) (GPIO ID)
> or:
>
> # using names within the controller
> gpio set tegra PQ0
>          ^^    ^^ (controller instance name) (GPIO name)
>
> (note that there must be separate controller ID and GPIO ID parameters in
> the commands in order to e.g. differentiate between 2 instances of the same
> I2C GPIO expander chip; something like pca9555 at a0@i2c0, pca9555 at i2c1@a4)
>
> not:
>
> gpio set tegraP 10
>          ^^     ^^ (hypothetical bank name) (GPIO ID within bank)
>
> or:
>
> gpio set P10
>          ^^ GPIO name without any controller ID
>

This will require enhancing the gpio command further, right?

>
>>> There's zero extra indirection caused by SW correctly describing the HW
>>> as a single bank. I have absolutely no idea what you mean my extra
>>> indirection here; any time there is a driver for a GPIO, you call a
>>> function to set a GPIO. That doesn't change based on whether there are
>>> 32 or 1 GPIO controller drivers.  The only difference is how many
>>> drivers you have to search through to find the right one. For Tegra at
>>> least, I'm arguing for 1 driver to match the 1 HW module.
>>
>>
>> OK let me explain a bit more.
>>
>> At the moment, the GPIO uclass supports a single GPIO bank, defined as
>> something that has a name, like A, or B.
>
>
> The GPIO bank name should just be "Tegra". The Tegra TRM specifies a single
> GPIO controller, in the address map etc., and there should be a single
> U-Boot object that represents it.  Really the phrase "bank" in U-Boot needs
> to be replaced with "controller".

But that would change the meaning - at present a GPIO device in U-Boot
is a GPIO bank.

>
>
>> Within that bank there can be
>>
>> several individual GPIO lines. This is what the Tegra TRM refers to as
>> A0, A1, B0, B1, etc.
>
>
> While the TRM does use the phrase "bank", I believe this is just a collision
> with the term you happened to choose. It's not used for the same semantic
> purposes. There's no intent to divide the Tegra GPIO controller into a bunch
> of logically separate HW blocks. "bank" in the TRM is just a convenient word
> to refer the fact that more than 32 GPIOs are supported, so they don't all
> fit into a single 32-bit register.

As an aside, using this logic, it is odd that there are only 8 GPIOs
per bank, instead of 32?

>
> So, the semantics of the HW are:
>
> A single GPIO controller named "tegra".
>
> Within that, there are a bunch of GPIOs. Each has a number e.g. 0..250 (for
> Tegra124) and a name (PA0, PA1, ... PA7, PB0, PB1, ..., PFF2). Users should
> be able to refer to those GPIOs either by integer ID, or by name.
>
> To support this, the GPIO uclass would need:
>
> * A string name for the controller
>
> * A set of functions/ops to manipulate the GPIOs (e.g. set input, set
> output, set output value) that accept integer IDs as the parameter for the
> GPIO ID.
>
> * If GPIOs have names as well as numbers, an optional function to convert a
> string name to an integer GPIO ID.
>
>
>> Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
>> device, we would need to redo the uclass to support this.
>
>
> No you wouldn't. Just put all the GPIOs into a single uclass instance. For
> naming, you can have the optional string->int conversion function in the
> uclass, or perhaps just ignore names (the kernel operates on integers for
> GPIOs...).
>

OK here we are talking about enhancing the uclass interface to support
conversion of names into numbers. I would prefer to have that logic be
common, and sit a level higher than the driver, because:

1. It avoids duplicating the same kind of lookup in each driver -
instead the code is in one place
2. It allows U-Boot to ensure that there are no conflicts when two
devices 'claim' the same name

I can see a future where we enhance the gpio command to be able to
specify the relevant GPIO device (in fact this has already been
discussed in another context and is a natural evolution of driver
model for many commands in U-Boot, such as 'load'). I can then see
that we might push the logic down into the driver and resolve
conflicts by requiring that the device is always specified (might this
be a pain though? An extra argument that is almost always
superfluous).

Still, I would prefer to take things a step at a time, and avoid
changing the gpio command, etc. The driver model conversion is not
supposed to be a big bang, I will be most happy if we can move over
without people having to adjust their scripts and understanding of
U-Boot. The driver model change should be 'behind the scenes' and
transparent to U-Boot users.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 07/30/2014 10:09 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 07/30/2014 09:26 AM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> On 06/11/2014 10:55 PM, Simon Glass wrote:
>>>> ...
>>>>>
>>>>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>>>>> all hard-coded in the software. So I ended up with the code you saw
>>>>> which just iterates over a known number of banks, creating a device
>>>>> for each.
>>>>
>>>>
>>>> That still sounds wrong. Tegra HW has a single GPIO controller that
>>>> exposes a bunch of GPIOs. It isn't logically divided into banks or any
>>>> other construct that is multi-level  Although the naming of the
>>>> individual GPIOs does call something a bank, that's just a name of a
>>>> register, not separate HW blocks. It's just going to be confusing to
>>>> users if the U-Boot representation doesn't match what the HW actually
>>>> has.
>>>
>>>
>>> I'm getting back to this as I just re-issued the series.
>>>
>>> I don't see the mismatch you are referring to here. U-Boot people are
>>> used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
>>> also.
>>
>>
>> The mismaatch is that in HW, there is a single GPIO controller that has a
>> large number of GPIOs, not a number of GPIO controllers that each has a
>> smaller number of GPIOs.
>>
>> U-Boot's commands/APIs/... should model this directly; a single controller
>> object that has a large number of GPIOs within it.
>>
>> As such, an example user-visible GPIO command needs to be roughly something
>> like:
>>
>> # using integer IDs
>> gpio set tegra 128
>>           ^^    ^^ (controller instance name) (GPIO ID)
>> or:
>>
>> # using names within the controller
>> gpio set tegra PQ0
>>           ^^    ^^ (controller instance name) (GPIO name)
>>
>> (note that there must be separate controller ID and GPIO ID parameters in
>> the commands in order to e.g. differentiate between 2 instances of the same
>> I2C GPIO expander chip; something like pca9555 at a0@i2c0, pca9555 at i2c1@a4)
>>
>> not:
>>
>> gpio set tegraP 10
>>           ^^     ^^ (hypothetical bank name) (GPIO ID within bank)
>>
>> or:
>>
>> gpio set P10
>>           ^^ GPIO name without any controller ID
>>
>
> This will require enhancing the gpio command further, right?

Sure, but that's going to be needed irrespective of this discussion, right?

>>>> There's zero extra indirection caused by SW correctly describing the HW
>>>> as a single bank. I have absolutely no idea what you mean my extra
>>>> indirection here; any time there is a driver for a GPIO, you call a
>>>> function to set a GPIO. That doesn't change based on whether there are
>>>> 32 or 1 GPIO controller drivers.  The only difference is how many
>>>> drivers you have to search through to find the right one. For Tegra at
>>>> least, I'm arguing for 1 driver to match the 1 HW module.
>>>
>>>
>>> OK let me explain a bit more.
>>>
>>> At the moment, the GPIO uclass supports a single GPIO bank, defined as
>>> something that has a name, like A, or B.
>>
>>
>> The GPIO bank name should just be "Tegra". The Tegra TRM specifies a single
>> GPIO controller, in the address map etc., and there should be a single
>> U-Boot object that represents it.  Really the phrase "bank" in U-Boot needs
>> to be replaced with "controller".
>
> But that would change the meaning - at present a GPIO device in U-Boot
> is a GPIO bank.

So just define the Tegra GPIO controller as having a single bank. The 
fact that U-Boot and Tegra both chose the word "bank" to mean different 
things doesn't mean that the U-Boot term has to be forced to apply to 
Tegra where the documentation talks about a "bank".

I don't think "bank" is a good description or level of abstraction 
anyway; U-Boot should use the term "controller", "chip", or "module" 
(which would apply to an entire HW module or GPIO expander chip), since 
"bank" is usually an internal implementation detail rather than 
something the user cares about. Put another way: all banks in a 
controller/chip/module/... would typically use the same 
operation/op/callback functions, just with different GPIO IDs or 
per-GPIO data, whereas different controllers/chips/modules/... would use 
different operation/op/callback functions. It therefore makes much more 
sense to abstract at the level of controller/chip/module/... rather than 
"bank" level, which is an internal implementation detail.

>>> Within that bank there can be
>>>
>>> several individual GPIO lines. This is what the Tegra TRM refers to as
>>> A0, A1, B0, B1, etc.
>>
>>
>> While the TRM does use the phrase "bank", I believe this is just a collision
>> with the term you happened to choose. It's not used for the same semantic
>> purposes. There's no intent to divide the Tegra GPIO controller into a bunch
>> of logically separate HW blocks. "bank" in the TRM is just a convenient word
>> to refer the fact that more than 32 GPIOs are supported, so they don't all
>> fit into a single 32-bit register.
>
> As an aside, using this logic, it is odd that there are only 8 GPIOs
> per bank, instead of 32?

This may have to do with the HW module having been designed for an 8-bit 
bus, and then ported to HW with a larger register bus?

>> So, the semantics of the HW are:
>>
>> A single GPIO controller named "tegra".
>>
>> Within that, there are a bunch of GPIOs. Each has a number e.g. 0..250 (for
>> Tegra124) and a name (PA0, PA1, ... PA7, PB0, PB1, ..., PFF2). Users should
>> be able to refer to those GPIOs either by integer ID, or by name.
>>
>> To support this, the GPIO uclass would need:
>>
>> * A string name for the controller
>>
>> * A set of functions/ops to manipulate the GPIOs (e.g. set input, set
>> output, set output value) that accept integer IDs as the parameter for the
>> GPIO ID.
>>
>> * If GPIOs have names as well as numbers, an optional function to convert a
>> string name to an integer GPIO ID.
>>
>>
>>> Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
>>> device, we would need to redo the uclass to support this.
>>
>>
>> No you wouldn't. Just put all the GPIOs into a single uclass instance. For
>> naming, you can have the optional string->int conversion function in the
>> uclass, or perhaps just ignore names (the kernel operates on integers for
>> GPIOs...).
>>
>
> OK here we are talking about enhancing the uclass interface to support
> conversion of names into numbers. I would prefer to have that logic be
> common, and sit a level higher than the driver, because:
>
> 1. It avoids duplicating the same kind of lookup in each driver -
> instead the code is in one place

This can easily be avoided by using utility functions. Put the name->ID 
conversion function pointer into the uclass struct. For HW that follows 
a common conversion algorithm, have the driver fill in that pointer with 
a common function provided by the core rather than custom code. That 
common function could use some data in the uclass struct to perform the 
conversion (e.g. base GPIO ID, name prefix string, etc.)

> 2. It allows U-Boot to ensure that there are no conflicts when two
> devices 'claim' the same name

That would imply U-Boot making some run-time decisions about the naming, 
which I don't think would work.

After all, most GPIO controllers will simply number their GPIOs 0..n. 
There's guaranteed to be a conflict in this case any time you have more 
than 1 GPIO controller in the system. The way to solve this is to refer 
to GPIOs by the tuple (controller name, GPIO name/ID) rather than trying 
to map all GPIO names/IDs into a single namespace. Mapping everything 
into a single namespace means somehow modifying the GPIO names so 
they're unique.

(Now the string representation of that tuple could well be to 
concatenate the controller name plus some seperator plus the GPIO name, 
and the GPIO DM core could do the parsing to split those two parts apart 
before calling the per-GPIO-controller name->GPIOID conversion function, 
but that's semantically the same thing)

> I can see a future where we enhance the gpio command to be able to
> specify the relevant GPIO device (in fact this has already been
> discussed in another context and is a natural evolution of driver
> model for many commands in U-Boot, such as 'load'). I can then see
> that we might push the logic down into the driver and resolve
> conflicts by requiring that the device is always specified (might this
> be a pain though? An extra argument that is almost always
> superfluous).
>
> Still, I would prefer to take things a step at a time, and avoid
> changing the gpio command, etc. The driver model conversion is not
> supposed to be a big bang, I will be most happy if we can move over
> without people having to adjust their scripts and understanding of
> U-Boot. The driver model change should be 'behind the scenes' and
> transparent to U-Boot users.

If you want to avoid changing the GPIO command, then the only choice is 
to map each GPIO controller's per-device integer GPIO ID space into some 
global GPIO ID space as is done today. In that case, there are no GPIO 
names, and GPIO bank/controller/... names aren't even relevant since 
they aren't user-visible. As such, I even more don't see the objection 
to modelling the Tegra GPIO controller as a single 
bank/controller/module/chip/... like it is.

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 07/30/2014 10:09 AM, Simon Glass wrote:
>
>> Hi Stephen,
>>
>> On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>>> On 07/30/2014 09:26 AM, Simon Glass wrote:
>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>>>
>>>>> On 06/11/2014 10:55 PM, Simon Glass wrote:
>>>>> ...
>>>>>
>>>>>>
>>>>>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>>>>>> all hard-coded in the software. So I ended up with the code you saw
>>>>>> which just iterates over a known number of banks, creating a device
>>>>>> for each.
>>>>>>
>>>>>
>>>>>
>>>>> That still sounds wrong. Tegra HW has a single GPIO controller that
>>>>> exposes a bunch of GPIOs. It isn't logically divided into banks or any
>>>>> other construct that is multi-level  Although the naming of the
>>>>> individual GPIOs does call something a bank, that's just a name of a
>>>>> register, not separate HW blocks. It's just going to be confusing to
>>>>> users if the U-Boot representation doesn't match what the HW actually
>>>>> has.
>>>>>
>>>>
>>>>
>>>> I'm getting back to this as I just re-issued the series.
>>>>
>>>> I don't see the mismatch you are referring to here. U-Boot people are
>>>> used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
>>>> also.
>>>>
>>>
>>>
>>> The mismaatch is that in HW, there is a single GPIO controller that has a
>>> large number of GPIOs, not a number of GPIO controllers that each has a
>>> smaller number of GPIOs.
>>>
>>> U-Boot's commands/APIs/... should model this directly; a single
>>> controller
>>> object that has a large number of GPIOs within it.
>>>
>>> As such, an example user-visible GPIO command needs to be roughly
>>> something
>>> like:
>>>
>>> # using integer IDs
>>> gpio set tegra 128
>>>           ^^    ^^ (controller instance name) (GPIO ID)
>>> or:
>>>
>>> # using names within the controller
>>> gpio set tegra PQ0
>>>           ^^    ^^ (controller instance name) (GPIO name)
>>>
>>> (note that there must be separate controller ID and GPIO ID parameters in
>>> the commands in order to e.g. differentiate between 2 instances of the
>>> same
>>> I2C GPIO expander chip; something like pca9555 at a0@i2c0, pca9555 at i2c1@a4)
>>>
>>> not:
>>>
>>> gpio set tegraP 10
>>>           ^^     ^^ (hypothetical bank name) (GPIO ID within bank)
>>>
>>> or:
>>>
>>> gpio set P10
>>>           ^^ GPIO name without any controller ID
>>>
>>>
>> This will require enhancing the gpio command further, right?
>>
>
> Sure, but that's going to be needed irrespective of this discussion, right?


No, the current series does not include this. It would be a separate
enhancement, probably proceeded by a wide-ranging discussion about the
U-Boot command line and how it will deal with multiple devices, etc (i.e.
not just for GPIOs).


>
>
>  There's zero extra indirection caused by SW correctly describing the HW
>>>>> as a single bank. I have absolutely no idea what you mean my extra
>>>>> indirection here; any time there is a driver for a GPIO, you call a
>>>>> function to set a GPIO. That doesn't change based on whether there are
>>>>> 32 or 1 GPIO controller drivers.  The only difference is how many
>>>>> drivers you have to search through to find the right one. For Tegra at
>>>>> least, I'm arguing for 1 driver to match the 1 HW module.
>>>>>
>>>>
>>>>
>>>> OK let me explain a bit more.
>>>>
>>>> At the moment, the GPIO uclass supports a single GPIO bank, defined as
>>>> something that has a name, like A, or B.
>>>>
>>>
>>>
>>> The GPIO bank name should just be "Tegra". The Tegra TRM specifies a
>>> single
>>> GPIO controller, in the address map etc., and there should be a single
>>> U-Boot object that represents it.  Really the phrase "bank" in U-Boot
>>> needs
>>> to be replaced with "controller".
>>>
>>
>> But that would change the meaning - at present a GPIO device in U-Boot
>> is a GPIO bank.
>>
>
> So just define the Tegra GPIO controller as having a single bank. The fact
> that U-Boot and Tegra both chose the word "bank" to mean different things
> doesn't mean that the U-Boot term has to be forced to apply to Tegra where
> the documentation talks about a "bank".
>
> I don't think "bank" is a good description or level of abstraction anyway;
> U-Boot should use the term "controller", "chip", or "module" (which would
> apply to an entire HW module or GPIO expander chip), since "bank" is
> usually an internal implementation detail rather than something the user
> cares about. Put another way: all banks in a controller/chip/module/...
> would typically use the same operation/op/callback functions, just with
> different GPIO IDs or per-GPIO data, whereas different
> controllers/chips/modules/... would use different operation/op/callback
> functions. It therefore makes much more sense to abstract at the level of
> controller/chip/module/... rather than "bank" level, which is an internal
> implementation detail.


Are you saying that 'bank' should be renamed 'chip' and then you would be
happy? Or are you still talking about a separate level of data structure?


>
>  Within that bank there can be
>>>>
>>>> several individual GPIO lines. This is what the Tegra TRM refers to as
>>>> A0, A1, B0, B1, etc.
>>>>
>>>
>>>
>>> While the TRM does use the phrase "bank", I believe this is just a
>>> collision
>>> with the term you happened to choose. It's not used for the same semantic
>>> purposes. There's no intent to divide the Tegra GPIO controller into a
>>> bunch
>>> of logically separate HW blocks. "bank" in the TRM is just a convenient
>>> word
>>> to refer the fact that more than 32 GPIOs are supported, so they don't
>>> all
>>> fit into a single 32-bit register.
>>>
>>
>> As an aside, using this logic, it is odd that there are only 8 GPIOs
>> per bank, instead of 32?
>>
>
> This may have to do with the HW module having been designed for an 8-bit
> bus, and then ported to HW with a larger register bus?


Yes, could be.


>
>
>  So, the semantics of the HW are:
>>>
>>> A single GPIO controller named "tegra".
>>>
>>> Within that, there are a bunch of GPIOs. Each has a number e.g. 0..250
>>> (for
>>> Tegra124) and a name (PA0, PA1, ... PA7, PB0, PB1, ..., PFF2). Users
>>> should
>>> be able to refer to those GPIOs either by integer ID, or by name.
>>>
>>> To support this, the GPIO uclass would need:
>>>
>>> * A string name for the controller
>>>
>>> * A set of functions/ops to manipulate the GPIOs (e.g. set input, set
>>> output, set output value) that accept integer IDs as the parameter for
>>> the
>>> GPIO ID.
>>>
>>> * If GPIOs have names as well as numbers, an optional function to
>>> convert a
>>> string name to an integer GPIO ID.
>>>
>>>
>>>  Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
>>>> device, we would need to redo the uclass to support this.
>>>>
>>>
>>>
>>> No you wouldn't. Just put all the GPIOs into a single uclass instance.
>>> For
>>> naming, you can have the optional string->int conversion function in the
>>> uclass, or perhaps just ignore names (the kernel operates on integers for
>>> GPIOs...).
>>>
>>>
>> OK here we are talking about enhancing the uclass interface to support
>> conversion of names into numbers. I would prefer to have that logic be
>> common, and sit a level higher than the driver, because:
>>
>> 1. It avoids duplicating the same kind of lookup in each driver -
>> instead the code is in one place
>>
>
> This can easily be avoided by using utility functions. Put the name->ID
> conversion function pointer into the uclass struct. For HW that follows a
> common conversion algorithm, have the driver fill in that pointer with a
> common function provided by the core rather than custom code. That common
> function could use some data in the uclass struct to perform the conversion
> (e.g. base GPIO ID, name prefix string, etc.)


We already have this information in the device tree in many cases. It make
more sense to register with the appropriate name than add callback/utility
functions that understand the intricacies or various SoC's GPIO naming.

The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
match with the TRM. A bank has 32 GPIOs and there are up to 8 banks. Each
bank gets a separate 'struct tegra_gpio_banks'. In no way at they all
lumped together. I am just trying to make sure that a 'struct udevice'
corresponds to a 'struct tegra_gpio_banks', in the sense that we are not
unnecessarily creating a level of data structure that is hidden from driver
model. For example 'tegra_gpio_banks' would not be visible to the 'dm tree'
command.

Does it really matter whether we split things into groups of 8 or groups of
32 or a large group of 224/256?


>  2. It allows U-Boot to ensure that there are no conflicts when two
>> devices 'claim' the same name
>>
>
> That would imply U-Boot making some run-time decisions about the naming,
> which I don't think would work.
>
> After all, most GPIO controllers will simply number their GPIOs 0..n.
> There's guaranteed to be a conflict in this case any time you have more
> than 1 GPIO controller in the system. The way to solve this is to refer to
> GPIOs by the tuple (controller name, GPIO name/ID) rather than trying to
> map all GPIO names/IDs into a single namespace. Mapping everything into a
> single namespace means somehow modifying the GPIO names so they're unique.
>
> (Now the string representation of that tuple could well be to concatenate
> the controller name plus some seperator plus the GPIO name, and the GPIO DM
> core could do the parsing to split those two parts apart before calling the
> per-GPIO-controller name->GPIOID conversion function, but that's
> semantically the same thing)


Sure, although I envisaged something where GPIO controllers would normally
have a name prefix. Even in the case of an I2C I/O extender, you could
imagine having two of these, 8 bits each, both named 'EXT' and thus you
would end up with GPIOs called EXT0-15. But I haven't gone that way so far,
since the other option is to change the GPIO command.


>
>  I can see a future where we enhance the gpio command to be able to
>> specify the relevant GPIO device (in fact this has already been
>> discussed in another context and is a natural evolution of driver
>> model for many commands in U-Boot, such as 'load'). I can then see
>> that we might push the logic down into the driver and resolve
>> conflicts by requiring that the device is always specified (might this
>> be a pain though? An extra argument that is almost always
>> superfluous).
>>
>> Still, I would prefer to take things a step at a time, and avoid
>> changing the gpio command, etc. The driver model conversion is not
>> supposed to be a big bang, I will be most happy if we can move over
>> without people having to adjust their scripts and understanding of
>> U-Boot. The driver model change should be 'behind the scenes' and
>> transparent to U-Boot users.
>>
>
> If you want to avoid changing the GPIO command, then the only choice is to
> map each GPIO controller's per-device integer GPIO ID space into some
> global GPIO ID space as is done today. In that case, there are no GPIO
> names, and GPIO bank/controller/... names aren't even relevant since they
> aren't user-visible. As such, I even more don't see the objection to
> modelling the Tegra GPIO controller as a single
> bank/controller/module/chip/... like it is.
>

Firstly we need to establish that GPIOs have names and that these should be
supported in U-Boot. Without agreement on this point we might not get much
further.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 07/31/2014 03:56 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org
> <mailto:swarren@wwwdotorg.org>> wrote:
>
>     On 07/30/2014 10:09 AM, Simon Glass wrote:
>
>         Hi Stephen,
>
>         On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org
>         <mailto:swarren@wwwdotorg.org>> wrote:
>
>             On 07/30/2014 09:26 AM, Simon Glass wrote:
>
>
>                 Hi Stephen,
>
>                 On 12 June 2014 23:31, Stephen Warren
>                 <swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>>
>                 wrote:
>
>
>                     On 06/11/2014 10:55 PM, Simon Glass wrote:
>                     ...

Hmmm. This email has funny formatting, but hopefully it won't be too 
hard to follow.

...
>     So just define the Tegra GPIO controller as having a single bank.
>     The fact that U-Boot and Tegra both chose the word "bank" to mean
>     different things doesn't mean that the U-Boot term has to be forced
>     to apply to Tegra where the documentation talks about a "bank".
>
>     I don't think "bank" is a good description or level of abstraction
>     anyway; U-Boot should use the term "controller", "chip", or "module"
>     (which would apply to an entire HW module or GPIO expander chip),
>     since "bank" is usually an internal implementation detail rather
>     than something the user cares about. Put another way: all banks in a
>     controller/chip/module/... would typically use the same
>     operation/op/callback functions, just with different GPIO IDs or
>     per-GPIO data, whereas different controllers/chips/modules/... would
>     use different operation/op/callback functions. It therefore makes
>     much more sense to abstract at the level of
>     controller/chip/module/... rather than "bank" level, which is an
>     internal implementation detail.
>
>
> Are you saying that 'bank' should be renamed 'chip' and then you would
> be happy? Or are you still talking about a separate level of data structure?

My primary objection is:

In Tegra, the GPIO controller should be represented as a single entity 
that controls 250 GPIOs, not as a set of separate entities that control 
8 or 32 GPIOs each.

Note that I'm talking about what the GPIO controller looks like to 
anything outside the driver. Whether the driver internally uses the 
concept of banks (or any other name) isn't relevant, since that would be 
an entirely hidden implementation detail.

Renaming "bank" to "chip" or "controller" certainly makes sense to me, 
but if that's the only change made, it would not address the objection I 
just mentioned.

...
> The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
> match with the TRM. A bank has 32 GPIOs and there are up to 8 banks.
> Each bank gets a separate 'struct tegra_gpio_banks'. In no way at they
> all lumped together.

That's not quite true.

*Internally* to the driver, there is a struct tegra_gpio_banks, I agree. 
As an aside, there really doesn't need to be, since the calculations are 
trivial enough that we should just do them for each access, but that's 
beside the point.

*Externally* to the driver, there is a single "struct gpio_chip" 
registered with the GPIO core. This chip owns/exposes all 250 GPIOs. 
That's because in terms of HW, there is a single GPIO controller that 
owns all 250 GPIOs.

 > I am just trying to make sure that a 'struct
> udevice' corresponds to a 'struct tegra_gpio_banks', in the sense that
> we are not unnecessarily creating a level of data structure that is
> hidden from driver model. For example 'tegra_gpio_banks' would not be
> visible to the 'dm tree' command.

I very specifically want U-Boot, just like the kernel, to have a single 
driver for the entire GPIO controller.

Whether the internals of that driver have a "struct tegra_gpio_banks" or 
not is an implementation detail that has zero impact on the driver 
model. As I mentioned above, there's no need for the driver to have a 
"struct tegra_gpio_banks"; the register address calculations are trivial 
enough that there's no need to divide the GPIO ID -> register address 
calculation into two steps (the intermediate step generating this 
annoying "bank" ID, which as you mentioned might not even correspond to 
what the TRM calls a bank.)

> Does it really matter whether we split things into groups of 8 or groups
> of 32 or a large group of 224/256?

Yes. Anything external to the GPIO controller driver must see the HW as 
it exists and is documented; a single HW module that controls 250 GPIOs.

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 31 July 2014 10:56, Simon Glass <sjg@chromium.org> wrote:
> Hi Stephen,
>
> On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>
>> On 07/30/2014 10:09 AM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>
>>>> On 07/30/2014 09:26 AM, Simon Glass wrote:
>>>>>
>>>>>
>>>>> Hi Stephen,
>>>>>
>>>>> On 12 June 2014 23:31, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 06/11/2014 10:55 PM, Simon Glass wrote:
>>>>>> ...
>>>>>>>
>>>>>>>
>>>>>>> Tegra doesn't have much in the device tree for GPIOs - it seems to be
>>>>>>> all hard-coded in the software. So I ended up with the code you saw
>>>>>>> which just iterates over a known number of banks, creating a device
>>>>>>> for each.
>>>>>>
>>>>>>
>>>>>>
>>>>>> That still sounds wrong. Tegra HW has a single GPIO controller that
>>>>>> exposes a bunch of GPIOs. It isn't logically divided into banks or any
>>>>>> other construct that is multi-level  Although the naming of the
>>>>>> individual GPIOs does call something a bank, that's just a name of a
>>>>>> register, not separate HW blocks. It's just going to be confusing to
>>>>>> users if the U-Boot representation doesn't match what the HW actually
>>>>>> has.
>>>>>
>>>>>
>>>>>
>>>>> I'm getting back to this as I just re-issued the series.
>>>>>
>>>>> I don't see the mismatch you are referring to here. U-Boot people are
>>>>> used to seeing GPIOs as named banks, and the Tegra TRM uses bank names
>>>>> also.
>>>>
>>>>
>>>>
>>>> The mismaatch is that in HW, there is a single GPIO controller that has
>>>> a
>>>> large number of GPIOs, not a number of GPIO controllers that each has a
>>>> smaller number of GPIOs.
>>>>
>>>> U-Boot's commands/APIs/... should model this directly; a single
>>>> controller
>>>> object that has a large number of GPIOs within it.
>>>>
>>>> As such, an example user-visible GPIO command needs to be roughly
>>>> something
>>>> like:
>>>>
>>>> # using integer IDs
>>>> gpio set tegra 128
>>>>           ^^    ^^ (controller instance name) (GPIO ID)
>>>> or:
>>>>
>>>> # using names within the controller
>>>> gpio set tegra PQ0
>>>>           ^^    ^^ (controller instance name) (GPIO name)
>>>>
>>>> (note that there must be separate controller ID and GPIO ID parameters
>>>> in
>>>> the commands in order to e.g. differentiate between 2 instances of the
>>>> same
>>>> I2C GPIO expander chip; something like pca9555 at a0@i2c0, pca9555 at i2c1@a4)
>>>>
>>>> not:
>>>>
>>>> gpio set tegraP 10
>>>>           ^^     ^^ (hypothetical bank name) (GPIO ID within bank)
>>>>
>>>> or:
>>>>
>>>> gpio set P10
>>>>           ^^ GPIO name without any controller ID
>>>>
>>>
>>> This will require enhancing the gpio command further, right?
>>
>>
>> Sure, but that's going to be needed irrespective of this discussion,
>> right?
>
>
> No, the current series does not include this. It would be a separate
> enhancement, probably proceeded by a wide-ranging discussion about the
> U-Boot command line and how it will deal with multiple devices, etc (i.e.
> not just for GPIOs).
>
>>
>>
>>
>>>>>> There's zero extra indirection caused by SW correctly describing the
>>>>>> HW
>>>>>> as a single bank. I have absolutely no idea what you mean my extra
>>>>>> indirection here; any time there is a driver for a GPIO, you call a
>>>>>> function to set a GPIO. That doesn't change based on whether there are
>>>>>> 32 or 1 GPIO controller drivers.  The only difference is how many
>>>>>> drivers you have to search through to find the right one. For Tegra at
>>>>>> least, I'm arguing for 1 driver to match the 1 HW module.
>>>>>
>>>>>
>>>>>
>>>>> OK let me explain a bit more.
>>>>>
>>>>> At the moment, the GPIO uclass supports a single GPIO bank, defined as
>>>>> something that has a name, like A, or B.
>>>>
>>>>
>>>>
>>>> The GPIO bank name should just be "Tegra". The Tegra TRM specifies a
>>>> single
>>>> GPIO controller, in the address map etc., and there should be a single
>>>> U-Boot object that represents it.  Really the phrase "bank" in U-Boot
>>>> needs
>>>> to be replaced with "controller".
>>>
>>>
>>> But that would change the meaning - at present a GPIO device in U-Boot
>>> is a GPIO bank.
>>
>>
>> So just define the Tegra GPIO controller as having a single bank. The fact
>> that U-Boot and Tegra both chose the word "bank" to mean different things
>> doesn't mean that the U-Boot term has to be forced to apply to Tegra where
>> the documentation talks about a "bank".
>>
>> I don't think "bank" is a good description or level of abstraction anyway;
>> U-Boot should use the term "controller", "chip", or "module" (which would
>> apply to an entire HW module or GPIO expander chip), since "bank" is usually
>> an internal implementation detail rather than something the user cares
>> about. Put another way: all banks in a controller/chip/module/... would
>> typically use the same operation/op/callback functions, just with different
>> GPIO IDs or per-GPIO data, whereas different controllers/chips/modules/...
>> would use different operation/op/callback functions. It therefore makes much
>> more sense to abstract at the level of controller/chip/module/... rather
>> than "bank" level, which is an internal implementation detail.
>
>
> Are you saying that 'bank' should be renamed 'chip' and then you would be
> happy? Or are you still talking about a separate level of data structure?
>
>>
>>
>>>>> Within that bank there can be
>>>>>
>>>>> several individual GPIO lines. This is what the Tegra TRM refers to as
>>>>> A0, A1, B0, B1, etc.
>>>>
>>>>
>>>>
>>>> While the TRM does use the phrase "bank", I believe this is just a
>>>> collision
>>>> with the term you happened to choose. It's not used for the same
>>>> semantic
>>>> purposes. There's no intent to divide the Tegra GPIO controller into a
>>>> bunch
>>>> of logically separate HW blocks. "bank" in the TRM is just a convenient
>>>> word
>>>> to refer the fact that more than 32 GPIOs are supported, so they don't
>>>> all
>>>> fit into a single 32-bit register.
>>>
>>>
>>> As an aside, using this logic, it is odd that there are only 8 GPIOs
>>> per bank, instead of 32?
>>
>>
>> This may have to do with the HW module having been designed for an 8-bit
>> bus, and then ported to HW with a larger register bus?
>
>
> Yes, could be.
>
>>
>>
>>
>>>> So, the semantics of the HW are:
>>>>
>>>> A single GPIO controller named "tegra".
>>>>
>>>> Within that, there are a bunch of GPIOs. Each has a number e.g. 0..250
>>>> (for
>>>> Tegra124) and a name (PA0, PA1, ... PA7, PB0, PB1, ..., PFF2). Users
>>>> should
>>>> be able to refer to those GPIOs either by integer ID, or by name.
>>>>
>>>> To support this, the GPIO uclass would need:
>>>>
>>>> * A string name for the controller
>>>>
>>>> * A set of functions/ops to manipulate the GPIOs (e.g. set input, set
>>>> output, set output value) that accept integer IDs as the parameter for
>>>> the
>>>> GPIO ID.
>>>>
>>>> * If GPIOs have names as well as numbers, an optional function to
>>>> convert a
>>>> string name to an integer GPIO ID.
>>>>
>>>>
>>>>> Should we wish to support banks A-Z, AA, BB, etc. all as a single GPIO
>>>>> device, we would need to redo the uclass to support this.
>>>>
>>>>
>>>>
>>>> No you wouldn't. Just put all the GPIOs into a single uclass instance.
>>>> For
>>>> naming, you can have the optional string->int conversion function in the
>>>> uclass, or perhaps just ignore names (the kernel operates on integers
>>>> for
>>>> GPIOs...).
>>>>
>>>
>>> OK here we are talking about enhancing the uclass interface to support
>>> conversion of names into numbers. I would prefer to have that logic be
>>> common, and sit a level higher than the driver, because:
>>>
>>> 1. It avoids duplicating the same kind of lookup in each driver -
>>> instead the code is in one place
>>
>>
>> This can easily be avoided by using utility functions. Put the name->ID
>> conversion function pointer into the uclass struct. For HW that follows a
>> common conversion algorithm, have the driver fill in that pointer with a
>> common function provided by the core rather than custom code. That common
>> function could use some data in the uclass struct to perform the conversion
>> (e.g. base GPIO ID, name prefix string, etc.)
>
>
> We already have this information in the device tree in many cases. It make
> more sense to register with the appropriate name than add callback/utility
> functions that understand the intricacies or various SoC's GPIO naming.
>
> The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
> match with the TRM. A bank has 32 GPIOs and there are up to 8 banks. Each
> bank gets a separate 'struct tegra_gpio_banks'. In no way at they all lumped
> together. I am just trying to make sure that a 'struct udevice' corresponds
> to a 'struct tegra_gpio_banks', in the sense that we are not unnecessarily
> creating a level of data structure that is hidden from driver model. For
> example 'tegra_gpio_banks' would not be visible to the 'dm tree' command.
>
> Does it really matter whether we split things into groups of 8 or groups of
> 32 or a large group of 224/256?
>
>>
>>> 2. It allows U-Boot to ensure that there are no conflicts when two
>>> devices 'claim' the same name
>>
>>
>> That would imply U-Boot making some run-time decisions about the naming,
>> which I don't think would work.
>>
>> After all, most GPIO controllers will simply number their GPIOs 0..n.
>> There's guaranteed to be a conflict in this case any time you have more than
>> 1 GPIO controller in the system. The way to solve this is to refer to GPIOs
>> by the tuple (controller name, GPIO name/ID) rather than trying to map all
>> GPIO names/IDs into a single namespace. Mapping everything into a single
>> namespace means somehow modifying the GPIO names so they're unique.
>>
>> (Now the string representation of that tuple could well be to concatenate
>> the controller name plus some seperator plus the GPIO name, and the GPIO DM
>> core could do the parsing to split those two parts apart before calling the
>> per-GPIO-controller name->GPIOID conversion function, but that's
>> semantically the same thing)
>
>
> Sure, although I envisaged something where GPIO controllers would normally
> have a name prefix. Even in the case of an I2C I/O extender, you could
> imagine having two of these, 8 bits each, both named 'EXT' and thus you
> would end up with GPIOs called EXT0-15. But I haven't gone that way so far,
> since the other option is to change the GPIO command.
>
>>
>>
>>> I can see a future where we enhance the gpio command to be able to
>>> specify the relevant GPIO device (in fact this has already been
>>> discussed in another context and is a natural evolution of driver
>>> model for many commands in U-Boot, such as 'load'). I can then see
>>> that we might push the logic down into the driver and resolve
>>> conflicts by requiring that the device is always specified (might this
>>> be a pain though? An extra argument that is almost always
>>> superfluous).
>>>
>>> Still, I would prefer to take things a step at a time, and avoid
>>> changing the gpio command, etc. The driver model conversion is not
>>> supposed to be a big bang, I will be most happy if we can move over
>>> without people having to adjust their scripts and understanding of
>>> U-Boot. The driver model change should be 'behind the scenes' and
>>> transparent to U-Boot users.
>>
>>
>> If you want to avoid changing the GPIO command, then the only choice is to
>> map each GPIO controller's per-device integer GPIO ID space into some global
>> GPIO ID space as is done today. In that case, there are no GPIO names, and
>> GPIO bank/controller/... names aren't even relevant since they aren't
>> user-visible. As such, I even more don't see the objection to modelling the
>> Tegra GPIO controller as a single bank/controller/module/chip/... like it
>> is.
>
>
> Firstly we need to establish that GPIOs have names and that these should be
> supported in U-Boot. Without agreement on this point we might not get much
> further.

Can I please press you on this point, as it is important to establish
this first.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 07/31/2014 03:58 PM, Simon Glass wrote:
> On 31 July 2014 10:56, Simon Glass <sjg@chromium.org> wrote:
...
>> Firstly we need to establish that GPIOs have names and that these should be
>> supported in U-Boot. Without agreement on this point we might not get much
>> further.
>
> Can I please press you on this point, as it is important to establish
> this first.

Oh, you were talking about agreeing with me? I thought this was more of 
a general comment, since you'd indicated earlier that amending the gpio 
to handle names was outside the scope of this patchset, and GPIO names 
are basically only relevant if the user-interface exposes/handles the names.

Well sure, GPIOs obviously have various sets of (controller-relative) 
names and IDs.

There certainly aren't any "global" or "absolute" names though, unless 
you include the controller name/ID (or something derived from it) as 
part of the GPIO's name/ID (either numerically via controller base IDs 
or textually by e.g. prefixing GPIO names with the controller name).

It would be good for any user-visible command/script to be able to 
handle either numeric IDs (either in an absolute space or preferably 
relative to a specified controller) or names.

[U-Boot] Driver Model and DTS Parsing

[Jon Loeliger]

>>> Firstly we need to establish that GPIOs have names and that these should
>>>be  supported in U-Boot. Without agreement on this point we might not get
>>> much further.
>>
>> Can I please press you on this point, as it is important to establish
>> this first.
>
> Oh, you were talking about agreeing with me? I thought this was more of a
> general comment, since you'd indicated earlier that amending the gpio to
> handle names was outside the scope of this patchset, and GPIO names are
> basically only relevant if the user-interface exposes/handles the names.

Yeah, but that applied to my request to use names
that were derived and identified by those strings/names
supplied in the DTS files.

jdl

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 31 July 2014 12:04, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 07/31/2014 03:56 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org
>> <mailto:swarren@wwwdotorg.org>> wrote:
>>
>>     On 07/30/2014 10:09 AM, Simon Glass wrote:
>>
>>         Hi Stephen,
>>
>>         On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org
>>         <mailto:swarren@wwwdotorg.org>> wrote:
>>
>>             On 07/30/2014 09:26 AM, Simon Glass wrote:
>>
>>
>>                 Hi Stephen,
>>
>>                 On 12 June 2014 23:31, Stephen Warren
>>                 <swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>>
>>
>>                 wrote:
>>
>>
>>                     On 06/11/2014 10:55 PM, Simon Glass wrote:
>>                     ...
>
>
> Hmmm. This email has funny formatting, but hopefully it won't be too hard to
> follow.
>
> ...
>
>>     So just define the Tegra GPIO controller as having a single bank.
>>     The fact that U-Boot and Tegra both chose the word "bank" to mean
>>     different things doesn't mean that the U-Boot term has to be forced
>>     to apply to Tegra where the documentation talks about a "bank".
>>
>>     I don't think "bank" is a good description or level of abstraction
>>     anyway; U-Boot should use the term "controller", "chip", or "module"
>>     (which would apply to an entire HW module or GPIO expander chip),
>>     since "bank" is usually an internal implementation detail rather
>>     than something the user cares about. Put another way: all banks in a
>>     controller/chip/module/... would typically use the same
>>     operation/op/callback functions, just with different GPIO IDs or
>>     per-GPIO data, whereas different controllers/chips/modules/... would
>>     use different operation/op/callback functions. It therefore makes
>>     much more sense to abstract at the level of
>>     controller/chip/module/... rather than "bank" level, which is an
>>     internal implementation detail.
>>
>>
>> Are you saying that 'bank' should be renamed 'chip' and then you would
>> be happy? Or are you still talking about a separate level of data
>> structure?
>
>
> My primary objection is:
>
> In Tegra, the GPIO controller should be represented as a single entity that
> controls 250 GPIOs, not as a set of separate entities that control 8 or 32
> GPIOs each.
>
> Note that I'm talking about what the GPIO controller looks like to anything
> outside the driver. Whether the driver internally uses the concept of banks
> (or any other name) isn't relevant, since that would be an entirely hidden
> implementation detail.
>
> Renaming "bank" to "chip" or "controller" certainly makes sense to me, but
> if that's the only change made, it would not address the objection I just
> mentioned.

If you look at the driver, it does actually have a single top-level
device which includes all the GPIOs. The other devices are children of
it.

We can certainly make it work such that accessing a GPIO can be done
using the top-level device and a number. That might get us far enough
for now. Although of course at present nothing actually uses the new
driver API except the GPIO uclass itself - all GPIO access in U-Boot
is still through the generic GPIO API.

BTW see this comment in gpio.h no less, similar for each Tegra board.

/*
 * The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports,
 * each with 8 GPIOs.
 */

Seems pretty clear to me.

>
> ...
>
>> The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
>> match with the TRM. A bank has 32 GPIOs and there are up to 8 banks.
>> Each bank gets a separate 'struct tegra_gpio_banks'. In no way at they
>> all lumped together.
>
>
> That's not quite true.
>
> *Internally* to the driver, there is a struct tegra_gpio_banks, I agree. As
> an aside, there really doesn't need to be, since the calculations are
> trivial enough that we should just do them for each access, but that's
> beside the point.
>
> *Externally* to the driver, there is a single "struct gpio_chip" registered
> with the GPIO core. This chip owns/exposes all 250 GPIOs. That's because in
> terms of HW, there is a single GPIO controller that owns all 250 GPIOs.

OK, but in U-Boot terms you are here you are talking about the
interface between the uclass and the device. We want that to be as
simple as possible to lessen the overhead on the driver writer. Of
course we can add new features in the future, or even refactor it if
we come up with a better idea.

>
>
>> I am just trying to make sure that a 'struct
>>
>> udevice' corresponds to a 'struct tegra_gpio_banks', in the sense that
>> we are not unnecessarily creating a level of data structure that is
>> hidden from driver model. For example 'tegra_gpio_banks' would not be
>> visible to the 'dm tree' command.
>
>
> I very specifically want U-Boot, just like the kernel, to have a single
> driver for the entire GPIO controller.

I think you mean device here. See my comments above about there being
a single top-level device. Also please look at the exynos driver which
can't do things this way. The GPIO controllers each have a variable
number of named banks split into 4-6 chunks. In Linux this is modelled
as multiple chips.

I wonder whether you might reconsider this request? It doesn't seem
very important to me how many child devices exist. But if you insist
then I think the best approach for now might be to drop the GPIO
naming for Tegra. That would be a shame, but a lesser evil I think
than forcing additional complexity on the GPIO uclass at this very
early stage. (As I said I'm quite happy to revisit it later when we do
a few more SoCs and have a bit more experience).

>
> Whether the internals of that driver have a "struct tegra_gpio_banks" or not
> is an implementation detail that has zero impact on the driver model. As I
> mentioned above, there's no need for the driver to have a "struct
> tegra_gpio_banks"; the register address calculations are trivial enough that
> there's no need to divide the GPIO ID -> register address calculation into
> two steps (the intermediate step generating this annoying "bank" ID, which
> as you mentioned might not even correspond to what the TRM calls a bank.)
>
>
>> Does it really matter whether we split things into groups of 8 or groups
>> of 32 or a large group of 224/256?
>
>
> Yes. Anything external to the GPIO controller driver must see the HW as it
> exists and is documented; a single HW module that controls 250 GPIOs.

Above the level of the GPIO uclass we can arrange that, as mentioned.
Is that good enough?

I've tested the series on beaver, so I'll resend it for you to try
out. Or you can check out the correct commit at u-boot-dm.git branch
working.

Regards,
Simon

[U-Boot] Driver Model and DTS Parsing

[Stephen Warren]

On 08/04/2014 04:22 AM, Simon Glass wrote:
> Hi Stephen,
>
> On 31 July 2014 12:04, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 07/31/2014 03:56 AM, Simon Glass wrote:
>>>
>>> Hi Stephen,
>>>
>>> On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org
>>> <mailto:swarren@wwwdotorg.org>> wrote:
>>>
>>>      On 07/30/2014 10:09 AM, Simon Glass wrote:
>>>
>>>          Hi Stephen,
>>>
>>>          On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org
>>>          <mailto:swarren@wwwdotorg.org>> wrote:
>>>
>>>              On 07/30/2014 09:26 AM, Simon Glass wrote:
>>>
>>>
>>>                  Hi Stephen,
>>>
>>>                  On 12 June 2014 23:31, Stephen Warren
>>>                  <swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>>
>>>
>>>                  wrote:
>>>
>>>
>>>                      On 06/11/2014 10:55 PM, Simon Glass wrote:
>>>                      ...
>>
>>
>> Hmmm. This email has funny formatting, but hopefully it won't be too hard to
>> follow.
>>
>> ...
>>
>>>      So just define the Tegra GPIO controller as having a single bank.
>>>      The fact that U-Boot and Tegra both chose the word "bank" to mean
>>>      different things doesn't mean that the U-Boot term has to be forced
>>>      to apply to Tegra where the documentation talks about a "bank".
>>>
>>>      I don't think "bank" is a good description or level of abstraction
>>>      anyway; U-Boot should use the term "controller", "chip", or "module"
>>>      (which would apply to an entire HW module or GPIO expander chip),
>>>      since "bank" is usually an internal implementation detail rather
>>>      than something the user cares about. Put another way: all banks in a
>>>      controller/chip/module/... would typically use the same
>>>      operation/op/callback functions, just with different GPIO IDs or
>>>      per-GPIO data, whereas different controllers/chips/modules/... would
>>>      use different operation/op/callback functions. It therefore makes
>>>      much more sense to abstract at the level of
>>>      controller/chip/module/... rather than "bank" level, which is an
>>>      internal implementation detail.
>>>
>>>
>>> Are you saying that 'bank' should be renamed 'chip' and then you would
>>> be happy? Or are you still talking about a separate level of data
>>> structure?
>>
>>
>> My primary objection is:
>>
>> In Tegra, the GPIO controller should be represented as a single entity that
>> controls 250 GPIOs, not as a set of separate entities that control 8 or 32
>> GPIOs each.
>>
>> Note that I'm talking about what the GPIO controller looks like to anything
>> outside the driver. Whether the driver internally uses the concept of banks
>> (or any other name) isn't relevant, since that would be an entirely hidden
>> implementation detail.
>>
>> Renaming "bank" to "chip" or "controller" certainly makes sense to me, but
>> if that's the only change made, it would not address the objection I just
>> mentioned.
>
> If you look at the driver, it does actually have a single top-level
> device which includes all the GPIOs. The other devices are children of
> it.
>
> We can certainly make it work such that accessing a GPIO can be done
> using the top-level device and a number. That might get us far enough
> for now. Although of course at present nothing actually uses the new
> driver API except the GPIO uclass itself - all GPIO access in U-Boot
> is still through the generic GPIO API.
>
> BTW see this comment in gpio.h no less, similar for each Tegra board.
>
> /*
>   * The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports,
>   * each with 8 GPIOs.
>   */
>
> Seems pretty clear to me.

If the interface between the uclass and the driver is the way GPIOs are 
(or will be) manipulated, and is such that the Tegra HW is exposed as a 
single device with 246 GPIOs, then everything is fine.

If the driver internally splits the HW up into a bunch of banks, I think 
that's not necessary, but it's then an implementation detail that has 
zero impact on what's visible through the uclass interface, so can 
easily be changed with zero impact, so I don't feel as strongly about that.

>> ...
>>
>>> The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
>>> match with the TRM. A bank has 32 GPIOs and there are up to 8 banks.
>>> Each bank gets a separate 'struct tegra_gpio_banks'. In no way at they
>>> all lumped together.
>>
>>
>> That's not quite true.
>>
>> *Internally* to the driver, there is a struct tegra_gpio_banks, I agree. As
>> an aside, there really doesn't need to be, since the calculations are
>> trivial enough that we should just do them for each access, but that's
>> beside the point.
>>
>> *Externally* to the driver, there is a single "struct gpio_chip" registered
>> with the GPIO core. This chip owns/exposes all 250 GPIOs. That's because in
>> terms of HW, there is a single GPIO controller that owns all 250 GPIOs.
>
> OK, but in U-Boot terms you are here you are talking about the
> interface between the uclass and the device. We want that to be as
> simple as possible to lessen the overhead on the driver writer. Of
> course we can add new features in the future, or even refactor it if
> we come up with a better idea.

I don't believe there's any impact on simplicity.

>>> I am just trying to make sure that a 'struct
>>>
>>> udevice' corresponds to a 'struct tegra_gpio_banks', in the sense that
>>> we are not unnecessarily creating a level of data structure that is
>>> hidden from driver model. For example 'tegra_gpio_banks' would not be
>>> visible to the 'dm tree' command.
>>
>>
>> I very specifically want U-Boot, just like the kernel, to have a single
>> driver for the entire GPIO controller.
>
> I think you mean device here. See my comments above about there being
> a single top-level device. Also please look at the exynos driver which
> can't do things this way. The GPIO controllers each have a variable
> number of named banks split into 4-6 chunks. In Linux this is modelled
> as multiple chips.

I don't know the Exynos HW at all, but it sounds like modelling it as 
multiple "chips" is a good thing to do. I'd expect U-Boot to do the same 
thing as the kernel here, so that people moving between the two aspects 
of the SW stack don't have to adjust their mental model of the HW 
between the two. Still, I have no say over what Exynos-related SW does...

> I wonder whether you might reconsider this request? It doesn't seem
> very important to me how many child devices exist. But if you insist
> then I think the best approach for now might be to drop the GPIO
> naming for Tegra. That would be a shame, but a lesser evil I think
> than forcing additional complexity on the GPIO uclass at this very
> early stage. (As I said I'm quite happy to revisit it later when we do
> a few more SoCs and have a bit more experience).

Given you say that the uclass<->driver interface already exposes Tegra 
as a single chip, I don't think there's anything to change? What I want 
is already there?

As an aside, since you said you weren't going to modify how the gpio 
command works, I don't see how you could add GPIO naming support anyway, 
since the gpio command currently takes a GPIO number not a name. So not 
adding it in the first instance seems like the default, not a loss.

>> Whether the internals of that driver have a "struct tegra_gpio_banks" or not
>> is an implementation detail that has zero impact on the driver model. As I
>> mentioned above, there's no need for the driver to have a "struct
>> tegra_gpio_banks"; the register address calculations are trivial enough that
>> there's no need to divide the GPIO ID -> register address calculation into
>> two steps (the intermediate step generating this annoying "bank" ID, which
>> as you mentioned might not even correspond to what the TRM calls a bank.)
>>
>>> Does it really matter whether we split things into groups of 8 or groups
>>> of 32 or a large group of 224/256?
>>
>>
>> Yes. Anything external to the GPIO controller driver must see the HW as it
>> exists and is documented; a single HW module that controls 250 GPIOs.
>
> Above the level of the GPIO uclass we can arrange that, as mentioned.
> Is that good enough?
>
> I've tested the series on beaver, so I'll resend it for you to try
> out. Or you can check out the correct commit at u-boot-dm.git branch
> working.
>
> Regards,
> Simon

[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 4 August 2014 11:38, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 08/04/2014 04:22 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 31 July 2014 12:04, Stephen Warren <swarren@wwwdotorg.org> wrote:
>>>
>>> On 07/31/2014 03:56 AM, Simon Glass wrote:
>>>>
>>>>
>>>> Hi Stephen,
>>>>
>>>> On 30 July 2014 20:57, Stephen Warren <swarren@wwwdotorg.org
>>>> <mailto:swarren@wwwdotorg.org>> wrote:
>>>>
>>>>      On 07/30/2014 10:09 AM, Simon Glass wrote:
>>>>
>>>>          Hi Stephen,
>>>>
>>>>          On 30 July 2014 16:47, Stephen Warren <swarren@wwwdotorg.org
>>>>          <mailto:swarren@wwwdotorg.org>> wrote:
>>>>
>>>>              On 07/30/2014 09:26 AM, Simon Glass wrote:
>>>>
>>>>
>>>>                  Hi Stephen,
>>>>
>>>>                  On 12 June 2014 23:31, Stephen Warren
>>>>                  <swarren at wwwdotorg.org <mailto:swarren@wwwdotorg.org>>
>>>>
>>>>                  wrote:
>>>>
>>>>
>>>>                      On 06/11/2014 10:55 PM, Simon Glass wrote:
>>>>                      ...
>>>
>>>
>>>
>>> Hmmm. This email has funny formatting, but hopefully it won't be too hard
>>> to
>>> follow.
>>>
>>> ...
>>>
>>>>      So just define the Tegra GPIO controller as having a single bank.
>>>>      The fact that U-Boot and Tegra both chose the word "bank" to mean
>>>>      different things doesn't mean that the U-Boot term has to be forced
>>>>      to apply to Tegra where the documentation talks about a "bank".
>>>>
>>>>      I don't think "bank" is a good description or level of abstraction
>>>>      anyway; U-Boot should use the term "controller", "chip", or
>>>> "module"
>>>>      (which would apply to an entire HW module or GPIO expander chip),
>>>>      since "bank" is usually an internal implementation detail rather
>>>>      than something the user cares about. Put another way: all banks in
>>>> a
>>>>      controller/chip/module/... would typically use the same
>>>>      operation/op/callback functions, just with different GPIO IDs or
>>>>      per-GPIO data, whereas different controllers/chips/modules/...
>>>> would
>>>>      use different operation/op/callback functions. It therefore makes
>>>>      much more sense to abstract at the level of
>>>>      controller/chip/module/... rather than "bank" level, which is an
>>>>      internal implementation detail.
>>>>
>>>>
>>>> Are you saying that 'bank' should be renamed 'chip' and then you would
>>>> be happy? Or are you still talking about a separate level of data
>>>> structure?
>>>
>>>
>>>
>>> My primary objection is:
>>>
>>> In Tegra, the GPIO controller should be represented as a single entity
>>> that
>>> controls 250 GPIOs, not as a set of separate entities that control 8 or
>>> 32
>>> GPIOs each.
>>>
>>> Note that I'm talking about what the GPIO controller looks like to
>>> anything
>>> outside the driver. Whether the driver internally uses the concept of
>>> banks
>>> (or any other name) isn't relevant, since that would be an entirely
>>> hidden
>>> implementation detail.
>>>
>>> Renaming "bank" to "chip" or "controller" certainly makes sense to me,
>>> but
>>> if that's the only change made, it would not address the objection I just
>>> mentioned.
>>
>>
>> If you look at the driver, it does actually have a single top-level
>> device which includes all the GPIOs. The other devices are children of
>> it.
>>
>> We can certainly make it work such that accessing a GPIO can be done
>> using the top-level device and a number. That might get us far enough
>> for now. Although of course at present nothing actually uses the new
>> driver API except the GPIO uclass itself - all GPIO access in U-Boot
>> is still through the generic GPIO API.
>>
>> BTW see this comment in gpio.h no less, similar for each Tegra board.
>>
>> /*
>>   * The Tegra 3x GPIO controller has 246 GPIOS in 8 banks of 4 ports,
>>   * each with 8 GPIOs.
>>   */
>>
>> Seems pretty clear to me.
>
>
> If the interface between the uclass and the driver is the way GPIOs are (or
> will be) manipulated, and is such that the Tegra HW is exposed as a single
> device with 246 GPIOs, then everything is fine.
>
> If the driver internally splits the HW up into a bunch of banks, I think
> that's not necessary, but it's then an implementation detail that has zero
> impact on what's visible through the uclass interface, so can easily be
> changed with zero impact, so I don't feel as strongly about that.

OK. Then it sounds like we have a path forward for now, and we can
revisit this after we have a few more GPIO drivers in the bag. Thanks.

>
>
>>> ...
>>>
>>>> The Linux GPIO driver uses the concept of a 'bank', and it doesn't even
>>>> match with the TRM. A bank has 32 GPIOs and there are up to 8 banks.
>>>> Each bank gets a separate 'struct tegra_gpio_banks'. In no way at they
>>>> all lumped together.
>>>
>>>
>>>
>>> That's not quite true.
>>>
>>> *Internally* to the driver, there is a struct tegra_gpio_banks, I agree.
>>> As
>>> an aside, there really doesn't need to be, since the calculations are
>>> trivial enough that we should just do them for each access, but that's
>>> beside the point.
>>>
>>> *Externally* to the driver, there is a single "struct gpio_chip"
>>> registered
>>> with the GPIO core. This chip owns/exposes all 250 GPIOs. That's because
>>> in
>>> terms of HW, there is a single GPIO controller that owns all 250 GPIOs.
>>
>>
>> OK, but in U-Boot terms you are here you are talking about the
>> interface between the uclass and the device. We want that to be as
>> simple as possible to lessen the overhead on the driver writer. Of
>> course we can add new features in the future, or even refactor it if
>> we come up with a better idea.
>
>
> I don't believe there's any impact on simplicity.

I'll leave that point for now since it sounds like you are comfortable
with the child device approach for now. Let's see how things look once
we are actually using the GPIO uclass properly, instead of just
through the generic GPIO interface.

>
>
>>>> I am just trying to make sure that a 'struct
>>>>
>>>> udevice' corresponds to a 'struct tegra_gpio_banks', in the sense that
>>>> we are not unnecessarily creating a level of data structure that is
>>>> hidden from driver model. For example 'tegra_gpio_banks' would not be
>>>> visible to the 'dm tree' command.
>>>
>>>
>>>
>>> I very specifically want U-Boot, just like the kernel, to have a single
>>> driver for the entire GPIO controller.
>>
>>
>> I think you mean device here. See my comments above about there being
>> a single top-level device. Also please look at the exynos driver which
>> can't do things this way. The GPIO controllers each have a variable
>> number of named banks split into 4-6 chunks. In Linux this is modelled
>> as multiple chips.
>
>
> I don't know the Exynos HW at all, but it sounds like modelling it as
> multiple "chips" is a good thing to do. I'd expect U-Boot to do the same
> thing as the kernel here, so that people moving between the two aspects of
> the SW stack don't have to adjust their mental model of the HW between the
> two. Still, I have no say over what Exynos-related SW does...
>
>
>> I wonder whether you might reconsider this request? It doesn't seem
>> very important to me how many child devices exist. But if you insist
>> then I think the best approach for now might be to drop the GPIO
>> naming for Tegra. That would be a shame, but a lesser evil I think
>> than forcing additional complexity on the GPIO uclass at this very
>> early stage. (As I said I'm quite happy to revisit it later when we do
>> a few more SoCs and have a bit more experience).
>
>
> Given you say that the uclass<->driver interface already exposes Tegra as a
> single chip, I don't think there's anything to change? What I want is
> already there?

There is a top-level device which contains child devices for each
bank, so yes. What is needed (when we start using the uclass properly)
is the ability to use the top-level device as you suggest. It should
be quite easy. Also by then we might have a bit more experience to go
on. Also while I have driver model running pre-relocation, it is not
running in SPL yet.

>
> As an aside, since you said you weren't going to modify how the gpio command
> works, I don't see how you could add GPIO naming support anyway, since the
> gpio command currently takes a GPIO number not a name. So not adding it in
> the first instance seems like the default, not a loss.
>

I am reluctant to change it *now*, since then it looks like we have to
change everything just to get driver model running. I really want
driver model to be seamless. That said, I very much see a benefit to
moving away from numbers and towards names in our use of devices, and
the 'gpio' command will benefit from that too, as you point out. I'm
trying to restrain myself from doing too many things at once.

[snip]

Regards,
Simon