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

[Igor Grinberg <grinberg@compulab.co.il>]

Hi Simon,

On 10/03/14 17:04, Simon Glass wrote:
> Hi Igor,
> 
> 
> On 3 October 2014 01:41, Igor Grinberg <grinberg@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@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@compulab.co.il>
>>>> Cc: Simon Glass <sjg@chromium.org>
>>>> Signed-off-by: Nikita Kiryanov <nikita@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.

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

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

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

-- 
Regards,
Igor.