[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

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

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

Regards,
Simon