[U-Boot] Driver Model and DTS Parsing

[Simon Glass]

Hi Stephen,

On 31 July 2014 12:04, Stephen Warren <swarren at wwwdotorg.org> wrote:
> On 07/31/2014 03:56 AM, Simon Glass wrote:
>>
>> Hi Stephen,
>>
>> On 30 July 2014 20:57, Stephen Warren <swarren at wwwdotorg.org
>> <mailto: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
>>         <mailto: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 <mailto:swarren at 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.

>
> ...
>
>> 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 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 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).

>
> Whether the internals of that driver have a "struct tegra_gpio_banks" or not
> is an implementation detail that has zero impact on the driver model. As I
> mentioned above, there's no need for the driver to have a "struct
> tegra_gpio_banks"; the register address calculations are trivial enough that
> there's no need to divide the GPIO ID -> register address calculation into
> two steps (the intermediate step generating this annoying "bank" ID, which
> as you mentioned might not even correspond to what the TRM calls a bank.)
>
>
>> Does it really matter whether we split things into groups of 8 or groups
>> of 32 or a large group of 224/256?
>
>
> Yes. Anything external to the GPIO controller driver must see the HW as it
> exists and is documented; a single HW module that controls 250 GPIOs.

Above the level of the GPIO uclass we can arrange that, as mentioned.
Is that good enough?

I've tested the series on beaver, so I'll resend it for you to try
out. Or you can check out the correct commit at u-boot-dm.git branch
working.

Regards,
Simon