[U-Boot] [PATCH] mx6: mx6qsabrelite/nitrogen6x: Fix use of gpio number in SF chip select
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)); }
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_...
So IMHO the gpio_direction_output() above can be removed completely.
Best regards
Dirk
Hi Dirk, ________________________________________
From: Behme, Dirk - Bosch Sent: Thursday, May 30, 2013 14:50 To: Gabbasov, Andrew Cc: u-boot@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_...
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
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@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_...
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
________________________________________
From: Behme, Dirk - Bosch Sent: Thursday, May 30, 2013 15:50 To: Gabbasov, Andrew Cc: u-boot@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@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_...
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
________________________________________
From: Gabbasov, Andrew Sent: Thursday, May 30, 2013 18:36 To: Behme, Dirk - Bosch Cc: u-boot@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
participants (3)
-
Andrew Gabbasov -
Dirk Behme -
Gabbasov, Andrew