[U-Boot] [PATCH v3 08/11] dm: imx: Use gpio_request() to request GPIOs

[Simon Glass]

Hi Nikita,

On 2 October 2014 04:28, Nikita Kiryanov <nikita at compulab.co.il> wrote:
> Hi Simon,
>
> On 01/10/14 18:22, Simon Glass wrote:
>>
>> Hi Nikita,
>>
>> On 1 October 2014 05:58, Nikita Kiryanov <nikita at compulab.co.il> wrote:
>>>
>>> Hi Simon,
>>>
>>> On 17/09/14 18:02, Simon Glass wrote:
>>>>
>>>>
>>>> GPIOs should be requested before use. Without this, driver model will
>>>> not
>>>> permit the GPIO to be used.
>>>>
>>>> Signed-off-by: Simon Glass <sjg at chromium.org>
>>>
>>>
>>>
>>> This patch introduces a bunch of errors (once the driver model stuff is
>>> turned on), all related to the gpios never being freed, but requested
>>> anew when reinitializing subsystems.
>>>
>>> The errors are:
>>>
>>> CM-FX6 # sata init
>>> Warning: iSSD setup failed!
>>> AHCI 0001.0300 32 slots 1 ports 3 Gbps 0x1 impl SATA mode
>>> flags: ncq stag pm led clo only pmp pio slum part
>>> SATA Device Info:
>>> S/N: 123900127157
>>> Product model number: SanDisk SSD i100 8GB
>>> Firmware version: 11.56.00
>>> Capacity: 15649200 sectors
>>
>>
>> Is it correct to init something twice?
>
>
> Of course. A re-init is a common use case, not just for sata, and anyway
> the reason for a failed re-init shouldn't be because sata code is
> preventing itself from utilizing its own GPIOs.

Yes, it needs to handle re-init, not just init.

>
>> It has already been done when
>> U-Boot boots I think. If it is, then are you thinking of changing
>> cm_fx6_setup_issd() to cope with that?
>
>
> Yes.
>
>>
>>>
>>> CM-FX6 # usb start
>>> (Re)start USB...
>>> USB0:   USB OTG pwr gpio request failed: -16
>>> USB OTG pwr gpio request failed: -16
>>> USB EHCI 1.00
>>> scanning bus 0 for devices... 2 USB Device(s) found
>>> USB1:   USB hub rst gpio request failed: -16
>>> USB hub rst gpio request failed: -16
>>> USB EHCI 1.00
>>> scanning bus 1 for devices... 6 USB Device(s) found
>>>         scanning usb for storage devices... max USB Storage Device
>>> reached: 5
>>> stopping
>>> 5 Storage Device(s) found
>>>
>>> CM-FX6 # sf probe
>>> mxc_spi: cannot setup gpio -16
>>> SF: Failed to set up slave
>>> Failed to initialize SPI flash at 0:0
>>
>>
>> I took at look at how this works for SPI. The approach of calling a
>> board function to find out the GPIO should go away with driver model -
>> we can use device tree, or platform data if that is not yet available.
>>
>> Also if you change the board code to 'stash' the GPIO and not request
>> it a second time, you will need to do that in every board. It seems
>> better to me to make this change in the driver, at least for SPI.
>
>
> There are benefits to stashing (or a better word would be
> 'pre-claiming') it in board code. If a gpio is necessary for SPI to
> work, it is not really meant to be used by other users, even if it's not
> immediately requested on boot. Pre-claiming it in board code enforces
> this.

But my stash idea was not a good one as I mentioned in the other thread.

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.

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.

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.

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.

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.

>
>> board_spi_cs_gpio() was added very recently in commit 155fa9af9.
>
>
> Yes, that is one of my patches :)
>
>> If you change it so that setup_cs_gpio() remembers the GPIO after calling
>> board_spi_cs_gpio() then we can avoid passing the problem on to
>> boards.
>
>
> I would like to revisit this debate after the SPI driver is converted to
> driver model. For now I favour the pre-claiming in board code approach.

OK, but see above. I feel this is going in the wrong direction for
driver model at least.

Are you interested in converting the SPI driver?

>
>>
>>>
>>> CM-FX6 # saveenv
>>> Saving Environment to SPI Flash...
>>> mxc_spi: cannot setup gpio -16
>>> SF: Failed to set up slave
>>> *** Warning - spi_flash_probe() failed, using default environment
>>
>>
>> Same issue.
>>
>>>
>>> I am going to submit a modified version of the cm_fx6 patches to address
>>> these problems.
>>
>>
>> I think those are already merged - I based my patches on your cm_fx6
>> patches and there were already in mainline.
>
>
> I was actually referring to a modified version of your patches that
> touches cm_fx6 code. That is, take your changes as follows:
> imx: Add error checking to setup_i2c() (sans the issd stuff)
> dm: imx: Use gpio_request() to request GPIOs (just the i2c-mxv7.c stuff)
> and add a new patch that refactors cm_fx6 init stuff.

Ah OK, that should have been obvious, sorry (was looking at
samsung/master where they didn't exist). Thanks for any assistance you
can provide.

Regards,
Simon