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