Hi Nikita,
On 2 October 2014 04:28, Nikita Kiryanov nikita@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@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@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