[U-Boot] Driver Model and DTS Parsing
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...
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
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...
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@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
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
+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
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.
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:
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()).
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
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.
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
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@a0@i2c0, pca9555@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...).
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@a0@i2c0, pca9555@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
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@a0@i2c0, pca9555@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:
- 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.)
- 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.
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@a0@i2c0, pca9555@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:
- 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?
- 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
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@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.
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@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
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@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
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@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
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@a0@i2c0, pca9555@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:
- 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?
- 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
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.
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
participants (3)
-
Jon Loeliger -
Simon Glass -
Stephen Warren