| Message ID | 1410966166-20767-9-git-send-email-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Simon Glass |
| Headers | show |
On 09/17/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> Acked-by: Igor Grinberg <grinberg@compulab.co.il>
On 17/09/2014 17: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> > --- > > Changes in v3: > - Add a check for the Ethernet gpio_request() also > - Add a comment for the CONFIG_SPL_BUILD #ifdef > - Just warn when one of the board init stages fails > > Changes in v2: > - Check return values of gpio_request() > > arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ > board/compulab/cm_fx6/cm_fx6.c | 22 ++++++++++++++++++++-- > board/compulab/cm_fx6/common.c | 8 ++++++++ > 3 files changed, 52 insertions(+), 2 deletions(-) > > diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c > index 70cff5c..aaf6936 100644 > --- a/arch/arm/imx-common/i2c-mxv7.c > +++ b/arch/arm/imx-common/i2c-mxv7.c > @@ -4,6 +4,7 @@ > * SPDX-License-Identifier: GPL-2.0+ > */ > #include <common.h> > +#include <malloc.h> > #include <asm/arch/clock.h> > #include <asm/arch/imx-regs.h> > #include <asm/errno.h> > @@ -72,10 +73,26 @@ static void * const i2c_bases[] = { > int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > struct i2c_pads_info *p) > { > + char *name1, *name2; > int ret; > > if (i2c_index >= ARRAY_SIZE(i2c_bases)) > return -EINVAL; > + > + name1 = malloc(9); > + name2 = malloc(9); > + if (!name1 || !name2) > + return -ENOMEM; > + sprintf(name1, "i2c_sda%d", i2c_index); > + sprintf(name2, "i2c_scl%d", i2c_index); > + ret = gpio_request(p->sda.gp, name1); > + if (ret) > + goto err_req1; > + > + ret = gpio_request(p->scl.gp, name2); > + if (ret) > + goto err_req2; > + > /* Enable i2c clock */ > ret = enable_i2c_clk(1, i2c_index); > if (ret) > @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, > > err_idle: > err_clk: > + gpio_free(p->scl.gp); > +err_req2: > + gpio_free(p->sda.gp); > +err_req1: > + free(name1); > + free(name2); > + > return ret; > } > diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c > index 10a568f..a089e82 100644 > --- a/board/compulab/cm_fx6/cm_fx6.c > +++ b/board/compulab/cm_fx6/cm_fx6.c > @@ -71,8 +71,21 @@ static iomux_v3_cfg_t const sata_pads[] = { > > static int cm_fx6_setup_issd(void) > { > + int ret; > + int i; > + > SETUP_IOMUX_PADS(sata_pads); > + > + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) { > + ret = gpio_request(cm_fx6_issd_gpios[i], "sata"); > + if (ret) > + return ret; > + } > + > /* Make sure this gpio has logical 0 value */ > + ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int"); > + if (ret) > + return ret; > gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); > udelay(100); > > @@ -350,12 +363,17 @@ static int handle_mac_address(void) > > int board_eth_init(bd_t *bis) > { > - int res = handle_mac_address(); > - if (res) > + int err; > + > + err = handle_mac_address(); > + if (err) > puts("No MAC address found\n"); > > SETUP_IOMUX_PADS(enet_pads); > /* phy reset */ > + err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); > + if (err) > + printf("Etnernet NRST gpio request failed: %d\n", err); > gpio_direction_output(CM_FX6_ENET_NRST, 0); > udelay(500); > gpio_set_value(CM_FX6_ENET_NRST, 1); > diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c > index 1f39679..20b758b 100644 > --- a/board/compulab/cm_fx6/common.c > +++ b/board/compulab/cm_fx6/common.c > @@ -79,6 +79,14 @@ void cm_fx6_set_ecspi_iomux(void) > > int board_spi_cs_gpio(unsigned bus, unsigned cs) > { > + /* DM does not support SPL yet and this function is not implemented */ > +#ifndef CONFIG_SPL_BUILD > + int ret; > + > + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); > + if (ret) > + return ret; > +#endif > return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; > } > #endif > Acked-by: Stefano Babic <sbabic@denx.de> Best regards, Stefano Babic
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 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 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 I am going to submit a modified version of the cm_fx6 patches to address these problems.
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? 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? > > 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. board_spi_cs_gpio() was added very recently in commit 155fa9af9. 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. > > 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. > > -- > Regards, > Nikita Kiryanov Regards, Simon
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. > 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. > 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. > >> >> 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. > >> >> -- >> Regards, >> Nikita Kiryanov > > Regards, > Simon >
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
diff --git a/arch/arm/imx-common/i2c-mxv7.c b/arch/arm/imx-common/i2c-mxv7.c index 70cff5c..aaf6936 100644 --- a/arch/arm/imx-common/i2c-mxv7.c +++ b/arch/arm/imx-common/i2c-mxv7.c @@ -4,6 +4,7 @@ * SPDX-License-Identifier: GPL-2.0+ */ #include <common.h> +#include <malloc.h> #include <asm/arch/clock.h> #include <asm/arch/imx-regs.h> #include <asm/errno.h> @@ -72,10 +73,26 @@ static void * const i2c_bases[] = { int setup_i2c(unsigned i2c_index, int speed, int slave_addr, struct i2c_pads_info *p) { + char *name1, *name2; int ret; if (i2c_index >= ARRAY_SIZE(i2c_bases)) return -EINVAL; + + name1 = malloc(9); + name2 = malloc(9); + if (!name1 || !name2) + return -ENOMEM; + sprintf(name1, "i2c_sda%d", i2c_index); + sprintf(name2, "i2c_scl%d", i2c_index); + ret = gpio_request(p->sda.gp, name1); + if (ret) + goto err_req1; + + ret = gpio_request(p->scl.gp, name2); + if (ret) + goto err_req2; + /* Enable i2c clock */ ret = enable_i2c_clk(1, i2c_index); if (ret) @@ -93,5 +110,12 @@ int setup_i2c(unsigned i2c_index, int speed, int slave_addr, err_idle: err_clk: + gpio_free(p->scl.gp); +err_req2: + gpio_free(p->sda.gp); +err_req1: + free(name1); + free(name2); + return ret; } diff --git a/board/compulab/cm_fx6/cm_fx6.c b/board/compulab/cm_fx6/cm_fx6.c index 10a568f..a089e82 100644 --- a/board/compulab/cm_fx6/cm_fx6.c +++ b/board/compulab/cm_fx6/cm_fx6.c @@ -71,8 +71,21 @@ static iomux_v3_cfg_t const sata_pads[] = { static int cm_fx6_setup_issd(void) { + int ret; + int i; + SETUP_IOMUX_PADS(sata_pads); + + for (i = 0; i < ARRAY_SIZE(cm_fx6_issd_gpios); i++) { + ret = gpio_request(cm_fx6_issd_gpios[i], "sata"); + if (ret) + return ret; + } + /* Make sure this gpio has logical 0 value */ + ret = gpio_request(CM_FX6_SATA_PWLOSS_INT, "sata_pwloss_int"); + if (ret) + return ret; gpio_direction_output(CM_FX6_SATA_PWLOSS_INT, 0); udelay(100); @@ -350,12 +363,17 @@ static int handle_mac_address(void) int board_eth_init(bd_t *bis) { - int res = handle_mac_address(); - if (res) + int err; + + err = handle_mac_address(); + if (err) puts("No MAC address found\n"); SETUP_IOMUX_PADS(enet_pads); /* phy reset */ + err = gpio_request(CM_FX6_ENET_NRST, "enet_nrst"); + if (err) + printf("Etnernet NRST gpio request failed: %d\n", err); gpio_direction_output(CM_FX6_ENET_NRST, 0); udelay(500); gpio_set_value(CM_FX6_ENET_NRST, 1); diff --git a/board/compulab/cm_fx6/common.c b/board/compulab/cm_fx6/common.c index 1f39679..20b758b 100644 --- a/board/compulab/cm_fx6/common.c +++ b/board/compulab/cm_fx6/common.c @@ -79,6 +79,14 @@ void cm_fx6_set_ecspi_iomux(void) int board_spi_cs_gpio(unsigned bus, unsigned cs) { + /* DM does not support SPL yet and this function is not implemented */ +#ifndef CONFIG_SPL_BUILD + int ret; + + ret = gpio_request(CM_FX6_ECSPI_BUS0_CS0, "ecspi_bus0_cs0"); + if (ret) + return ret; +#endif return (bus == 0 && cs == 0) ? (CM_FX6_ECSPI_BUS0_CS0) : -1; } #endif
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> --- Changes in v3: - Add a check for the Ethernet gpio_request() also - Add a comment for the CONFIG_SPL_BUILD #ifdef - Just warn when one of the board init stages fails Changes in v2: - Check return values of gpio_request() arch/arm/imx-common/i2c-mxv7.c | 24 ++++++++++++++++++++++++ board/compulab/cm_fx6/cm_fx6.c | 22 ++++++++++++++++++++-- board/compulab/cm_fx6/common.c | 8 ++++++++ 3 files changed, 52 insertions(+), 2 deletions(-)