Hi Stephen, On 31 July 2014 10:56, Simon Glass <s...@chromium.org> wrote: > Hi Stephen, > > On 30 July 2014 20:57, Stephen Warren <swar...@wwwdotorg.org> wrote: >> >> On 07/30/2014 10:09 AM, Simon Glass wrote: >>> >>> Hi Stephen, >>> >>> On 30 July 2014 16:47, Stephen Warren <swar...@wwwdotorg.org> wrote: >>>> >>>> On 07/30/2014 09:26 AM, Simon Glass wrote: >>>>> >>>>> >>>>> Hi Stephen, >>>>> >>>>> On 12 June 2014 23:31, Stephen Warren <swar...@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: >>> >>> 1. It avoids duplicating the same kind of lookup in each driver - >>> instead the code is in one place >> >> >> This can easily be avoided by using utility functions. Put the name->ID >> conversion function pointer into the uclass struct. For HW that follows a >> common conversion algorithm, have the driver fill in that pointer with a >> common function provided by the core rather than custom code. That common >> function could use some data in the uclass struct to perform the conversion >> (e.g. base GPIO ID, name prefix string, etc.) > > > We already have this information in the device tree in many cases. It make > more sense to register with the appropriate name than add callback/utility > functions that understand the intricacies or various SoC's GPIO naming. > > The Linux GPIO driver uses the concept of a 'bank', and it doesn't even > match with the TRM. A bank has 32 GPIOs and there are up to 8 banks. Each > bank gets a separate 'struct tegra_gpio_banks'. In no way at they all lumped > together. I am just trying to make sure that a 'struct udevice' corresponds > to a 'struct tegra_gpio_banks', in the sense that we are not unnecessarily > creating a level of data structure that is hidden from driver model. For > example 'tegra_gpio_banks' would not be visible to the 'dm tree' command. > > Does it really matter whether we split things into groups of 8 or groups of > 32 or a large group of 224/256? > >> >>> 2. It allows U-Boot to ensure that there are no conflicts when two >>> devices 'claim' the same name >> >> >> That would imply U-Boot making some run-time decisions about the naming, >> which I don't think would work. >> >> After all, most GPIO controllers will simply number their GPIOs 0..n. >> There's guaranteed to be a conflict in this case any time you have more than >> 1 GPIO controller in the system. The way to solve this is to refer to GPIOs >> by the tuple (controller name, GPIO name/ID) rather than trying to map all >> GPIO names/IDs into a single namespace. Mapping everything into a single >> namespace means somehow modifying the GPIO names so they're unique. >> >> (Now the string representation of that tuple could well be to concatenate >> the controller name plus some seperator plus the GPIO name, and the GPIO DM >> core could do the parsing to split those two parts apart before calling the >> per-GPIO-controller name->GPIOID conversion function, but that's >> semantically the same thing) > > > Sure, although I envisaged something where GPIO controllers would normally > have a name prefix. Even in the case of an I2C I/O extender, you could > imagine having two of these, 8 bits each, both named 'EXT' and thus you > would end up with GPIOs called EXT0-15. But I haven't gone that way so far, > since the other option is to change the GPIO command. > >> >> >>> I can see a future where we enhance the gpio command to be able to >>> specify the relevant GPIO device (in fact this has already been >>> discussed in another context and is a natural evolution of driver >>> model for many commands in U-Boot, such as 'load'). I can then see >>> that we might push the logic down into the driver and resolve >>> conflicts by requiring that the device is always specified (might this >>> be a pain though? An extra argument that is almost always >>> superfluous). >>> >>> Still, I would prefer to take things a step at a time, and avoid >>> changing the gpio command, etc. The driver model conversion is not >>> supposed to be a big bang, I will be most happy if we can move over >>> without people having to adjust their scripts and understanding of >>> U-Boot. The driver model change should be 'behind the scenes' and >>> transparent to U-Boot users. >> >> >> If you want to avoid changing the GPIO command, then the only choice is to >> map each GPIO controller's per-device integer GPIO ID space into some global >> GPIO ID space as is done today. In that case, there are no GPIO names, and >> GPIO bank/controller/... names aren't even relevant since they aren't >> user-visible. As such, I even more don't see the objection to modelling the >> Tegra GPIO controller as a single bank/controller/module/chip/... like it >> is. > > > Firstly we need to establish that GPIOs have names and that these should be > supported in U-Boot. Without agreement on this point we might not get much > further.
Can I please press you on this point, as it is important to establish this first. Regards, Simon _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
