[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

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.

Regards,
Simon