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