[U-Boot] [PATCH 2/2] arm: mx6: cm_fx6: use gpio request

[Simon Glass]

Hi Igor,

On 5 October 2014 04:52, Igor Grinberg <grinberg at compulab.co.il> wrote:
> Hi Simon,
>
> On 10/03/14 17:04, Simon Glass wrote:
>> Hi Igor,
>>
>>
>> On 3 October 2014 01:41, Igor Grinberg <grinberg at compulab.co.il> wrote:
>>> Hi Simon,
>>>
>>> On 10/02/14 22:22, Simon Glass wrote:
>>>> Hi Nikita,
>>>>
>>>> On 2 October 2014 08:17, Nikita Kiryanov <nikita at compulab.co.il> wrote:
>>>>> Use gpio_request for all the gpios that are utilized by various
>>>>> subsystems in cm-fx6, and refactor the relevant init functions
>>>>> so that all gpios are requested during board_init(), not during
>>>>> subsystem init, thus avoiding the need to manage gpio ownership
>>>>> each time a subsystem is initialized.
>>>>>
>>>>> The new division of labor is:
>>>>> During board_init() muxes are setup and gpios are requested.
>>>>> During subsystem init gpios are toggled.
>>>>>
>>>>> Cc: Igor Grinberg <grinberg at compulab.co.il>
>>>>> Cc: Simon Glass <sjg at chromium.org>
>>>>> Signed-off-by: Nikita Kiryanov <nikita at compulab.co.il>
>>>>
>>>> Copying my comments from the other patch:
>>>
>>> Please, don't get me wrong, we have of course read and thought about
>>> this also considering your comments.
>>> The thing is that currently, some of those GPIOs are way too board
>>> specific and may be the right thing would be to leave it that way.
>>> Also some are not board specific, and I will be very glad to pass
>>> them to the drivers.
>>> Yet, I think Nikita's patch is very sane for now.
>>> Once we can pass the GPIOs to the drivers we will of course do this.
>>> We will also be glad to help working on this as we always did (when
>>> the schedule permits us).
>>>
>>>>
>>>> I've thought about that quite a lot as part of the driver model work.
>>>> Claiming GPIOs in board code doesn't feel right to me:
>>>>
>>>> 1. If using device tree, the GPIOs are in there, and it means the
>>>> board code needs to go looking there as well as the driver. The board
>>>> code actually needs to sniff around in the driver's device tree nodes.
>>>> That just seems honky.
>>>
>>> I think this is case dependent. Really we're in the boot loader
>>> world, things here are board specific in many cases.
>>
>> Yes it is important to retain that flexibility.
>>
>>>
>>>>
>>>> 2. In the driver model world, we hope that board init will fade away
>>>> to a large extent. Certainly it should be possible to specify most of
>>>> what a driver needs in device tree or platform data. Getting rid of
>>>> the explicit init calls in U-Boot (board_init_f(), board_init(),
>>>> board_late_init(), board_early_init_f(), ...) is a nice effect of
>>>> driver model I hope.
>>>
>>> I don't think it is possible.
>>> There will always be boards that are not by the reference design
>>> and there will have to be stuff done in the board files as it will
>>> not concern any other boards or drivers.
>>
>> Let's see how this pans out. I feel we can more the pendulum a long
>> way towards less board-specific init. For example, for MMC we have a
>> card detect. If we just specify which GPIO it is on, then we don't
>> need all the callbacks.
>
> That is totally agreed for cases where it is possible.

OK good.

>
>>
>>>
>>>>
>>>> 3. Even if not using device tree, and using platform data, where the
>>>> board code may specify the platform data, it still feels honky for the
>>>> board to be parsing its own data (designed for use by the driver) to
>>>> claim GPIOs.
>>>
>>> Why even? Not using DT is not a bad practice at all!
>>> DT has been designed as an API/ABI to the OS and not for the boot loader!
>>> Boot loaders are board specific, period.
>>> I don't mind using DT in the boot loader for ease and abstraction, but
>>> don't be obsessed with it, because it will only lead to another,
>>> pre-bootloader boot loader which will accommodate all the stuff you
>>> are trying to avoid.
>>>
>>> Regarding your feeling honky about parsing data by the board code:
>>> There are so many cases, that I don't think you have considered,
>>> where using DT _instead_ of run time initializations is a total
>>> madness.
>>> Here is one:
>>> Same board, different configuration/revision/extension/variation/etc.
>>> Instead of parsing stuff at runtime and adjusting things according
>>> to detection, you propose an army of DT blobs? This sounds insane.
>>
>> We are talking here about the code/data trade-off. I feel that U-Boot
>> currently requires lots of code to be written where with device
>> tree/platform data it could be data, and the benefit is fewer code
>> paths and easier integration of new boards. What are these revisions
>> doing? If they are changing the MMC card detect line then you can have
>> two different platform data blobs for the board, or two device trees.
>> It's not mandatory but it certainly works.
>
> Right. Yet, you still need code that will detect the revision
> and make the correct choice for platform data/DT blob.
> What I'm saying is that in this case, you don't really need several
> DT blobs or platform data structs, but only one which can be
> adjusted by the same (or not the same) revision detection code.
>
> Sometimes, it can be 3, 4, 5 revisions of the CoM, plus even more of
> a base board. You can't really have that much DT blobs in a pocket
> to pull out...

Sure you can. But if you have board rev GPIOs you can do at least three things:

1. Use them only during production to configure the image to flash (an
image that only works with a single board rev)
2. Use them at run time to select which platform data or device tree info to use
3. Use them at run-time to adjust the platform data or device tree
(even if just to change the 'status' property from "okay" to
"disabled")

I feel all of these have their uses depending on the situation. There
a lots of trade-offs to be had.

For base boards (and maybe even board revs) I wonder if the DT merging
feature might be useful?

>
>>
>> If it is easier to check a few GPIOs to find the board ID and then
>> adjust things at runtime then that works too. That sort of code should
>> live in the board files though, not the drivers.
>
> So, it merely repeats what I'm saying...
>
>>
>>>
>>>>
>>>> 4. I don't really see why pre-claiming enforces anything. If two
>>>> subsystems claim the same GPIO you are going to get an error somewhere
>>>> in any case.
>>>
>>> Two subsystems should never own the same GPIO at the same time.
>>> If you follow that rule, there will be errors only in case when there
>>> should errors.
>>
>> Yes. My point was that pre-claiming would still result in an error in this case.
>
> It will have the benefit of _not_ playing the request - free game.
> IMO, request should be done once (for GPIOs that are designed to be used
> in only one subsystem) and then the subsystem can toggle the GPIO as
> it likes - w/o the need for requesting once again.

OK. In general I prefer the device to claim the GPIOs rather than the
board. With driver model this is pretty easy as there is a formal
'probe' function which will only be called once (until after 'remove'
is called at least). I think request/free is is only a problem because
of the ad-hoc driver approach. But I see that as a general direction
to take, and not a hard and fast rule.

>
>>
>>>
>>>>
>>>> 5. If you look at the calls that drivers make to find out information
>>>> from the board file, or perform some init (mmc does this, USB,
>>>> ethernet, etc.) it is mostly because the driver has no idea of the
>>>> specifics of the board. Device tree and platform data fix exactly this
>>>> problem. The driver has the best idea of when it needs to start up,
>>>> when it needs this resource of that. In some cases the driver have the
>>>> ability to operate in two modes (e.g. 4 bit or 8 bit SDIO, UART
>>>> with/without flow control) and this means the init depends on the
>>>> driver's mode.
>>>
>>> This is correct. No doubt about this.
>>> Yet, generic driver may have prerequisite on a custom board and
>>> don't even know about this prerequisite.
>>
>> Or in fact the driver may request that the board be set up a
>> particular way. This is effectively what happens with MMC - 'please
>> set up the card-detect line for me to use'.
>>
>>>
>>>>
>>>> Having said that, it's your board and if you really want to go this
>>>> way in the interim, then I'm not going to strongly object.
>>>
>>> Thanks!
>>> I do really like the idea of DM and I think this should be developed
>>> year ago. Yet, any framework should be flexible enough to give some
>>> place on the stage to the board specific code.
>>
>> I strongly agree with this statement and have been careful so far to
>> maintain this flexibility in the framework. Let's keep an eye on it.

Regards,
Simon