tegra20: tamonten: Fix the early gpio init

PATCH New Alban Bedel 3 participants 8 messages Oct 2, 2014

lore

Share & tools

lore

Alban Bedel

author

2014-10-02 15:14 UTC #0 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.

Change-Id Id0aadb17a71b78e75e8c3f8de374102b3eab767b

Signed-off-by Alban Bedel <[email protected]>

---
 board/avionic-design/common/tamonten.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

board/avionic-design/common/tamonten.c +3 -1

index 9c86779..ea2425a 100644
@@ -23,8 +23,10 @@
 #ifdef CONFIG_BOARD_EARLY_INIT_F
 void gpio_early_init(void)
 {
+#ifndef CONFIG_SPL_BUILD
 	gpio_request(GPIO_PI4, NULL);
-	gpio_direction_output(GPIO_PI4, 1);
+#endif
+	tegra_spl_gpio_direction_output(GPIO_PI4, 1);
 }
 #endif

Signature · 1 line

2.1.1

... although perhaps the SPL and non-SPL code should simply be separated into separate files, so that there's no need for ifdefs, and it's obvious if SPL and non-SPL code are duplicating the same work?

Alban Bedel

author

2014-10-02 16:12 UTC #2 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

Quoted 7 lines · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:

Quoted 4 lines · Alban Bedel
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.

Sorry about this.

Quoted 23 lines

Quoted 10 lines
Signed-off-by Alban Bedel <[email protected]>
---
  board/avionic-design/common/tamonten.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
  #ifdef CONFIG_BOARD_EARLY_INIT_F
  void gpio_early_init(void)
  {
+#ifndef CONFIG_SPL_BUILD
  	gpio_request(GPIO_PI4, NULL);
-	gpio_direction_output(GPIO_PI4, 1);
+#endif
+	tegra_spl_gpio_direction_output(GPIO_PI4, 1);
  }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:

Quoted 6 lines

#ifdef CONFIG_SPL_BUILD
	tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
	gpio_request(GPIO_PI4, NULL);
	gpio_direction_output(GPIO_PI4, 1);

#endif

Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.

AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.

Quoted 3 lines

... although perhaps the SPL and non-SPL code should simply be separated into separate files, so that there's no need for ifdefs, and it's obvious if SPL and non-SPL code are duplicating the same work?

Actually none of the code from tamonten.c is needed for the SPL, a build with both function under #ifndef CONFIG_SPL_BUILD works just fine. Any pointer on how I can get tamonten.c out of the SPL build?

Alban
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20141002/5e94bc1a/attachment.pgp>

Stephen Warren

2014-10-02 18:39 UTC #3 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

Quoted 43 lines · Alban Bedel

Quoted 7 lines · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:
Quoted 4 lines · Alban Bedel
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.
Sorry about this.
Quoted 23 lines
Quoted 10 lines
Signed-off-by Alban Bedel <[email protected]>
---
   board/avionic-design/common/tamonten.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
   #ifdef CONFIG_BOARD_EARLY_INIT_F
   void gpio_early_init(void)
   {
+#ifndef CONFIG_SPL_BUILD
   	gpio_request(GPIO_PI4, NULL);
-	gpio_direction_output(GPIO_PI4, 1);
+#endif
+	tegra_spl_gpio_direction_output(GPIO_PI4, 1);
   }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
	tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
	gpio_request(GPIO_PI4, NULL);
	gpio_direction_output(GPIO_PI4, 1);
#endif

Quoted 7 lines

Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.
AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.

Hmm. CC Simon to comment on which GPIO drivers are available in SPL/non-SPL, and why the above ifdef doesn't work.

Quoted 6 lines

Quoted 3 lines
... although perhaps the SPL and non-SPL code should simply be separated into separate files, so that there's no need for ifdefs, and it's obvious if SPL and non-SPL code are duplicating the same work?
Actually none of the code from tamonten.c is needed for the SPL, a build with both function under #ifndef CONFIG_SPL_BUILD works just fine.

Indeed, if manipulating the GPIO in SPL isn't even necessary, then just wrapping the whole code in #ifndef CONFIG_SPL_BUILD makes sense to me.

Quoted 1 line

Any pointer on how I can get tamonten.c out of the SPL build?

I'm not sure now that the Makefile structure has changed to Kbuild. It might be as simple as:

obj-$(CONFIG_SPL_BUILD) += foo.o

(but using whatever the opposite of CONFIG_SPL_BUILD is.

Simon Glass

2014-10-02 19:09 UTC #4 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

Hi Stephen,

Quoted 56 lines · Stephen Warren

Quoted 55 lines · Alban Bedel
Quoted 9 lines · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:
Quoted 5 lines · Alban Bedel
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.
Sorry about this.
Quoted 25 lines
Quoted 11 lines
Signed-off-by Alban Bedel <[email protected]>
---
   board/avionic-design/common/tamonten.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
   #ifdef CONFIG_BOARD_EARLY_INIT_F
   void gpio_early_init(void)
   {
+#ifndef CONFIG_SPL_BUILD
        gpio_request(GPIO_PI4, NULL);
-       gpio_direction_output(GPIO_PI4, 1);
+#endif
+       tegra_spl_gpio_direction_output(GPIO_PI4, 1);
   }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
        tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
        gpio_request(GPIO_PI4, NULL);
        gpio_direction_output(GPIO_PI4, 1);
#endif
Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.

Quoted 6 lines

Quoted 2 lines
AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.
Hmm. CC Simon to comment on which GPIO drivers are available in SPL/non-SPL, and why the above ifdef doesn't work.

See my previous reply.

Quoted 8 lines

Quoted 8 lines
Quoted 4 lines
... although perhaps the SPL and non-SPL code should simply be separated into separate files, so that there's no need for ifdefs, and it's obvious if SPL and non-SPL code are duplicating the same work?
Actually none of the code from tamonten.c is needed for the SPL, a build with both function under #ifndef CONFIG_SPL_BUILD works just fine.

I think this will drop the message. But I should have mentioned in my other reply that we don't really need to call gpio_early_init_uart() from board_early_init_f() if we are already calling it from spl_board_init(). Separate issue to this patch of course.

Quoted 8 lines

Indeed, if manipulating the GPIO in SPL isn't even necessary, then just wrapping the whole code in #ifndef CONFIG_SPL_BUILD makes sense to me.
Quoted 1 line
Any pointer on how I can get tamonten.c out of the SPL build?

Quoted 6 lines

I'm not sure now that the Makefile structure has changed to Kbuild. It might be as simple as:
obj-$(CONFIG_SPL_BUILD) += foo.o
(but using whatever the opposite of CONFIG_SPL_BUILD is.

Regards,
Simon

Simon Glass

2014-10-02 19:05 UTC #5 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

In reply to Alban Bedel

Hi Alban,

Quoted 5 lines · Alban Bedel

Quoted 1 line · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:
On 10/02/2014 09:14 AM, Alban Bedel wrote:

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.
Sorry about this.

If you use patman it will take these out for you automatically without you having to think about it.

Quoted 32 lines

Quoted 25 lines
Quoted 12 lines
Signed-off-by Alban Bedel <[email protected]>
---
  board/avionic-design/common/tamonten.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
  #ifdef CONFIG_BOARD_EARLY_INIT_F
  void gpio_early_init(void)
  {
+#ifndef CONFIG_SPL_BUILD
    gpio_request(GPIO_PI4, NULL);
-   gpio_direction_output(GPIO_PI4, 1);
+#endif
+   tegra_spl_gpio_direction_output(GPIO_PI4, 1);
  }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
      tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
      gpio_request(GPIO_PI4, NULL);
      gpio_direction_output(GPIO_PI4, 1);
#endif

Quoted 7 lines

Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.
AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.

This is a temporary measure until the SPL series is applied to dm/next at least (I expect sometime this month). While driver model is available before relocation, the Tegra GPIO driver is not marked in the device tree with "u-boot,dm-pre-reloc", so is not available this early. Device tree additions give Stephen a headache, and I decided it was better for it not to work for now, and just use the SPL function.

There are three stages to consider:

1. SPL
2. U-Boot, before relocation
3. U-Boot, after relocation

At present, driver model does not support 1 (see dm/spl-working for patches if you want to see this). It supports 2 but only for drivers marked pre-relocation (except serial, for which a hack forces the console to be bound and used in the device tree case without the u-boot,dm-pre-reloc marker). It supports 3 fully.

There is a bit of an update on current status here:

http://www.denx.de/wiki/U-Boot/DriverModel

My approach with driver model is incremental, since that's the only way to make any progress.

Quoted 8 lines

Quoted 4 lines
... although perhaps the SPL and non-SPL code should simply be separated into separate files, so that there's no need for ifdefs, and it's obvious if SPL and non-SPL code are duplicating the same work?
Actually none of the code from tamonten.c is needed for the SPL, a build with both function under #ifndef CONFIG_SPL_BUILD works just fine. Any pointer on how I can get tamonten.c out of the SPL build?

This should work:

ifndef CONFIG_SPL_BUILD
obj-y += tamonten.o
endif

But it seems odd to me. At least on seaboard I didn't get the SPL message unless this code was executed.

Regards,
Simon

Stephen Warren

2014-10-02 19:08 UTC #6 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

Quoted 73 lines · Simon Glass

Hi Alban,
Quoted 5 lines · Alban Bedel
Quoted 1 line · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:
On 10/02/2014 09:14 AM, Alban Bedel wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 4 lines
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.
Sorry about this.
If you use patman it will take these out for you automatically without you having to think about it.
Quoted 32 lines
Quoted 25 lines
Quoted 12 lines
Signed-off-by Alban Bedel <[email protected]>
---
   board/avionic-design/common/tamonten.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
   #ifdef CONFIG_BOARD_EARLY_INIT_F
   void gpio_early_init(void)
   {
+#ifndef CONFIG_SPL_BUILD
     gpio_request(GPIO_PI4, NULL);
-   gpio_direction_output(GPIO_PI4, 1);
+#endif
+   tegra_spl_gpio_direction_output(GPIO_PI4, 1);
   }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
       tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
       gpio_request(GPIO_PI4, NULL);
       gpio_direction_output(GPIO_PI4, 1);
#endif
Quoted 7 lines
Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.
AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.
This is a temporary measure until the SPL series is applied to dm/next at least (I expect sometime this month). While driver model is available before relocation, the Tegra GPIO driver is not marked in the device tree with "u-boot,dm-pre-reloc", so is not available this early. Device tree additions give Stephen a headache, and I decided it was better for it not to work for now, and just use the SPL function.
There are three stages to consider:
1. SPL
2. U-Boot, before relocation
3. U-Boot, after relocation

Quoted 5 lines

At present, driver model does not support 1 (see dm/spl-working for patches if you want to see this). It supports 2 but only for drivers marked pre-relocation (except serial, for which a hack forces the console to be bound and used in the device tree case without the u-boot,dm-pre-reloc marker). It supports 3 fully.

That's roughly what I expected. Given that, I fail to see why the following wouldn't work then:

Quoted 6 lines

#ifdef CONFIG_SPL_BUILD
       tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
       gpio_request(GPIO_PI4, NULL);
       gpio_direction_output(GPIO_PI4, 1);

#endif

(or have just 1 of those two ifdef branches; presumably the GPIO only needs to be initialized in one place not both?)

Simon Glass

2014-10-02 19:11 UTC #7 lore raw

PATCH 1/1 tegra20: tamonten: Fix the early gpio init

Hi Stephen,

Quoted 102 lines · Stephen Warren

Quoted 84 lines · Simon Glass
Hi Alban,
Quoted 10 lines · Alban Bedel
Quoted 5 lines · On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <[email protected]> wrote:
Quoted 4 lines · Alban Bedel
To set gpio during the early init we now need to use tegra_spl_gpio_direction_output(), copied from seaboard.
Quoted 7 lines
Quoted 4 lines
Quoted 1 line
Change-Id: Id0aadb17a71b78e75e8c3f8de374102b3eab767b
That shouldn't be present on upstream patches.
Sorry about this.
If you use patman it will take these out for you automatically without you having to think about it.
Quoted 35 lines
Quoted 27 lines
Quoted 13 lines
Signed-off-by Alban Bedel <[email protected]>
---
   board/avionic-design/common/tamonten.c | 4 +++-
   1 file changed, 3 insertions(+), 1 deletion(-)
board/avionic-design/common/tamonten.c +0 -0
index 9c86779..ea2425a 100644
board/avionic-design/common/tamonten.c line 30
Quoted 9 lines
board/avionic-design/common/tamonten.c +3 -1
@@ -23,8 +23,10 @@
   #ifdef CONFIG_BOARD_EARLY_INIT_F
   void gpio_early_init(void)
   {
+#ifndef CONFIG_SPL_BUILD
     gpio_request(GPIO_PI4, NULL);
-   gpio_direction_output(GPIO_PI4, 1);
+#endif
+   tegra_spl_gpio_direction_output(GPIO_PI4, 1);
   }
Surely you only want to call tegra_spl_*() from SPL, and not from non-SPL code? In other words, don't you need something more like:
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
       tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
       gpio_request(GPIO_PI4, NULL);
       gpio_direction_output(GPIO_PI4, 1);
#endif
Quoted 7 lines
Sadly not, at this point the gpio driver isn't available yet, that's why one need to use tegra_spl_gpio_direction_output(). As mentioned in the commit log I copied this from seaboard, assuming it would be correct.
AFAICT the gpio_request() could be removed too, it doesn't work at this point anyway.
This is a temporary measure until the SPL series is applied to dm/next at least (I expect sometime this month). While driver model is available before relocation, the Tegra GPIO driver is not marked in the device tree with "u-boot,dm-pre-reloc", so is not available this early. Device tree additions give Stephen a headache, and I decided it was better for it not to work for now, and just use the SPL function.
There are three stages to consider:
1. SPL
2. U-Boot, before relocation
3. U-Boot, after relocation
Quoted 5 lines
At present, driver model does not support 1 (see dm/spl-working for patches if you want to see this). It supports 2 but only for drivers marked pre-relocation (except serial, for which a hack forces the console to be bound and used in the device tree case without the u-boot,dm-pre-reloc marker). It supports 3 fully.
That's roughly what I expected. Given that, I fail to see why the following wouldn't work then:
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
#ifdef CONFIG_SPL_BUILD
       tegra_spl_gpio_direction_output(GPIO_PI4, 1);
#else
       gpio_request(GPIO_PI4, NULL);
       gpio_direction_output(GPIO_PI4, 1);
#endif

Quoted 2 lines

(or have just 1 of those two ifdef branches; presumably the GPIO only needs to be initialized in one place not both?)

Yes that's true, although be aware that the gpio_request() and gpio_direction_output() will in fact always fail at present, since they are called before the GPIO driver is inited pre-reloc. But since it has happened in SPL it doesn't matter.

I haven't tested this, and it's icky to rely on failure and not checking the return code, and I slightly prefer Alban's solution at least for now. This is only going to be a problem for another month or so.

Regards,
Simon