* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select @ 2013-05-30 10:02 Andrew Gabbasov 2013-05-30 10:50 ` Dirk Behme 0 siblings, 1 reply; 6+ messages in thread From: Andrew Gabbasov @ 2013-05-30 10:02 UTC (permalink / raw) To: u-boot The number of gpio signal is packed inside CONFIG_SF_DEFAULT_CS macro (shifted and or'ed with chip select), so it's incorrect to pass that macro directly as an argument to gpio_direction_output() call. The gpio number should be extracted (shifted back) before that. Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> --- board/boundary/nitrogen6x/nitrogen6x.c | 2 +- board/freescale/mx6qsabrelite/mx6qsabrelite.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c index cc071d6..b588ac2 100644 --- a/board/boundary/nitrogen6x/nitrogen6x.c +++ b/board/boundary/nitrogen6x/nitrogen6x.c @@ -342,7 +342,7 @@ iomux_v3_cfg_t const ecspi1_pads[] = { void setup_spi(void) { - gpio_direction_output(CONFIG_SF_DEFAULT_CS, 1); + gpio_direction_output(CONFIG_SF_DEFAULT_CS >> 8, 1); imx_iomux_v3_setup_multiple_pads(ecspi1_pads, ARRAY_SIZE(ecspi1_pads)); } diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index 9f9cac8..8b71e03 100644 --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c @@ -312,7 +312,7 @@ iomux_v3_cfg_t const ecspi1_pads[] = { void setup_spi(void) { - gpio_direction_output(CONFIG_SF_DEFAULT_CS, 1); + gpio_direction_output(CONFIG_SF_DEFAULT_CS >> 8, 1); imx_iomux_v3_setup_multiple_pads(ecspi1_pads, ARRAY_SIZE(ecspi1_pads)); } -- 1.7.10.4 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select 2013-05-30 10:02 [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select Andrew Gabbasov @ 2013-05-30 10:50 ` Dirk Behme 2013-05-30 11:32 ` Gabbasov, Andrew 0 siblings, 1 reply; 6+ messages in thread From: Dirk Behme @ 2013-05-30 10:50 UTC (permalink / raw) To: u-boot On 30.05.2013 12:02, Andrew Gabbasov wrote: > The number of gpio signal is packed inside CONFIG_SF_DEFAULT_CS macro > (shifted and or'ed with chip select), so it's incorrect to pass > that macro directly as an argument to gpio_direction_output() call. > The gpio number should be extracted (shifted back) before that. > > Signed-off-by: Andrew Gabbasov <andrew_gabbasov@mentor.com> > --- > board/boundary/nitrogen6x/nitrogen6x.c | 2 +- > board/freescale/mx6qsabrelite/mx6qsabrelite.c | 2 +- > 2 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/board/boundary/nitrogen6x/nitrogen6x.c b/board/boundary/nitrogen6x/nitrogen6x.c > index cc071d6..b588ac2 100644 > --- a/board/boundary/nitrogen6x/nitrogen6x.c > +++ b/board/boundary/nitrogen6x/nitrogen6x.c > @@ -342,7 +342,7 @@ iomux_v3_cfg_t const ecspi1_pads[] = { > > void setup_spi(void) > { > - gpio_direction_output(CONFIG_SF_DEFAULT_CS, 1); > + gpio_direction_output(CONFIG_SF_DEFAULT_CS >> 8, 1); > imx_iomux_v3_setup_multiple_pads(ecspi1_pads, > ARRAY_SIZE(ecspi1_pads)); > } > diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c > index 9f9cac8..8b71e03 100644 > --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c > +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c > @@ -312,7 +312,7 @@ iomux_v3_cfg_t const ecspi1_pads[] = { > > void setup_spi(void) > { > - gpio_direction_output(CONFIG_SF_DEFAULT_CS, 1); > + gpio_direction_output(CONFIG_SF_DEFAULT_CS >> 8, 1); > imx_iomux_v3_setup_multiple_pads(ecspi1_pads, > ARRAY_SIZE(ecspi1_pads)); > } To my understanding, above change is correct, but not complete ;) The question is "why has it worked with the wrong setting and nobody ever noticed that its wrong?" To my understanding the answer is "because the SPI driver does it correctly": http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/spi/mxc_spi.c;h=5bed858787f610a9c9a46bb2214665a51d60a9e9;hb=refs/heads/master#l376 So IMHO the gpio_direction_output() above can be removed completely. Best regards Dirk ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select 2013-05-30 10:50 ` Dirk Behme @ 2013-05-30 11:32 ` Gabbasov, Andrew 2013-05-30 11:50 ` Dirk Behme 0 siblings, 1 reply; 6+ messages in thread From: Gabbasov, Andrew @ 2013-05-30 11:32 UTC (permalink / raw) To: u-boot Hi Dirk, ________________________________________ > From: Behme, Dirk - Bosch > Sent: Thursday, May 30, 2013 14:50 > To: Gabbasov, Andrew > Cc: u-boot at lists.denx.de > Subject: Re: [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select [skipped] > To my understanding, above change is correct, but not complete ;) > > The question is "why has it worked with the wrong setting and nobody > ever noticed that its wrong?" > > To my understanding the answer is "because the SPI driver does it > correctly": > > http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/spi/mxc_spi.c;h=5bed858787f610a9c9a46bb2214665a51d60a9e9;hb=refs/heads/master#l376 > > So IMHO the gpio_direction_output() above can be removed completely. > > Best regards > > Dirk Yes, the SPI driver correctly activates and deactivates the CS signal. But before the first activation it relies on what signal state was set before that. Setting it early at startup just adds some confidence that we have correct (inactive) chip select state before the first activation by SPI driver. Otherwise we have to rely on particular pad configuration (e.g. EIM_D19). I understand, that we set its configuration to "pull-up" (and this is also the reset default), and if we do nothing here, it will be recognized as "high". But in order to make sure, it's more safe to explicitly set the signal to 1. Thanks. Best regards, Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select 2013-05-30 11:32 ` Gabbasov, Andrew @ 2013-05-30 11:50 ` Dirk Behme 2013-05-30 14:36 ` Gabbasov, Andrew 0 siblings, 1 reply; 6+ messages in thread From: Dirk Behme @ 2013-05-30 11:50 UTC (permalink / raw) To: u-boot On 30.05.2013 13:32, Gabbasov, Andrew wrote: > Hi Dirk, > ________________________________________ >> From: Behme, Dirk - Bosch >> Sent: Thursday, May 30, 2013 14:50 >> To: Gabbasov, Andrew >> Cc: u-boot at lists.denx.de >> Subject: Re: [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select > > [skipped] > >> To my understanding, above change is correct, but not complete ;) >> >> The question is "why has it worked with the wrong setting and nobody >> ever noticed that its wrong?" >> >> To my understanding the answer is "because the SPI driver does it >> correctly": >> >> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/spi/mxc_spi.c;h=5bed858787f610a9c9a46bb2214665a51d60a9e9;hb=refs/heads/master#l376 >> >> So IMHO the gpio_direction_output() above can be removed completely. >> >> Best regards >> >> Dirk > > Yes, the SPI driver correctly activates and deactivates the CS signal. > But before the first activation it relies on what signal state was set before that. > Setting it early at startup just adds some confidence that we have correct > (inactive) chip select state before the first activation by SPI driver. > Otherwise we have to rely on particular pad configuration (e.g. EIM_D19). > I understand, that we set its configuration to "pull-up" (and this is also > the reset default), and if we do nothing here, it will be recognized as "high". > But in order to make sure, it's more safe to explicitly set the signal to 1. Hmm, what's 'unsure' in the time between calling setup_spi() the first time and calling spi_setup_slave() the first time? Are they even called in this order? How long is the time between these two calls? What's 'unsafe' in this time frame? Why isn't it unsafe _until_ setup_spi() is called, then? Best regards Dirk ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select 2013-05-30 11:50 ` Dirk Behme @ 2013-05-30 14:36 ` Gabbasov, Andrew 2013-05-30 14:50 ` Gabbasov, Andrew 0 siblings, 1 reply; 6+ messages in thread From: Gabbasov, Andrew @ 2013-05-30 14:36 UTC (permalink / raw) To: u-boot ________________________________________ > From: Behme, Dirk - Bosch > Sent: Thursday, May 30, 2013 15:50 > To: Gabbasov, Andrew > Cc: u-boot at lists.denx.de > Subject: Re: [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select > > On 30.05.2013 13:32, Gabbasov, Andrew wrote: > > Hi Dirk, > > ________________________________________ > >> From: Behme, Dirk - Bosch > >> Sent: Thursday, May 30, 2013 14:50 > >> To: Gabbasov, Andrew > >> Cc: u-boot at lists.denx.de > >> Subject: Re: [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select > > > > [skipped] > > > >> To my understanding, above change is correct, but not complete ;) > >> > >> The question is "why has it worked with the wrong setting and nobody > >> ever noticed that its wrong?" > >> > >> To my understanding the answer is "because the SPI driver does it > >> correctly": > >> > >> http://git.denx.de/cgi-bin/gitweb.cgi?p=u-boot.git;a=blob;f=drivers/spi/mxc_spi.c;h=5bed858787f610a9c9a46bb2214665a51d60a9e9;hb=refs/heads/master#l376 > >> > >> So IMHO the gpio_direction_output() above can be removed completely. > >> > >> Best regards > >> > >> Dirk > > > > Yes, the SPI driver correctly activates and deactivates the CS signal. > > But before the first activation it relies on what signal state was set before that. > > Setting it early at startup just adds some confidence that we have correct > > (inactive) chip select state before the first activation by SPI driver. > > Otherwise we have to rely on particular pad configuration (e.g. EIM_D19). > > I understand, that we set its configuration to "pull-up" (and this is also > > the reset default), and if we do nothing here, it will be recognized as "high". > > But in order to make sure, it's more safe to explicitly set the signal to 1. > > Hmm, what's 'unsure' in the time between calling setup_spi() the first > time and calling spi_setup_slave() the first time? > > Are they even called in this order? How long is the time between these > two calls? What's 'unsafe' in this time frame? Why isn't it unsafe > _until_ setup_spi() is called, then? > > Best regards > > Dirk Hi Dirk, It turns out I overlooked that the pad configuration for chip select gpio is actually "pull-down", so, since erroneous gpio_direction_output call does not make effect, the signal seems to be kept low, i.e. active, until first spi_setup_slave call (where it is set correctly before any actual activity is being done on SPI bus). And since indeed nobody yet complained about that, it must be ok to just remove the gpio related call in setup_spi. So, OK, you convinced me ;-) I'll prepare another patch shortly, that just removes gpio_direction_output calls from setup_spi function for both mx6qsabrelite and nitrogen6x. Thanks. Best regards, Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
* [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select 2013-05-30 14:36 ` Gabbasov, Andrew @ 2013-05-30 14:50 ` Gabbasov, Andrew 0 siblings, 0 replies; 6+ messages in thread From: Gabbasov, Andrew @ 2013-05-30 14:50 UTC (permalink / raw) To: u-boot ________________________________________ > From: Gabbasov, Andrew > Sent: Thursday, May 30, 2013 18:36 > To: Behme, Dirk - Bosch > Cc: u-boot at lists.denx.de > Subject: RE: [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select [skipped] > So, OK, you convinced me ;-) I'll prepare another patch shortly, that just removes > gpio_direction_output calls from setup_spi function for both mx6qsabrelite > and nitrogen6x. I have submitted the mentioned patch. See Subject line "mx6: mx6qsabrelite/nitrogen6x: Remove incorrect setting of gpio CS signal" Thanks. Best regards, Andrew ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-05-30 14:50 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-30 10:02 [U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select Andrew Gabbasov 2013-05-30 10:50 ` Dirk Behme 2013-05-30 11:32 ` Gabbasov, Andrew 2013-05-30 11:50 ` Dirk Behme 2013-05-30 14:36 ` Gabbasov, Andrew 2013-05-30 14:50 ` Gabbasov, Andrew
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.