| Message ID | 1412262898-26349-1-git-send-email-alban.bedel@avionic-design.de |
|---|---|
| State | Superseded |
| Delegated to: | Tom Warren |
| Headers | show |
On 10/02/2014 09:14 AM, Alban Bedel wrote: > 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. > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > --- > board/avionic-design/common/tamonten.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c > index 9c86779..ea2425a 100644 > --- a/board/avionic-design/common/tamonten.c > +++ b/board/avionic-design/common/tamonten.c > @@ -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: #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 ... 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?
On Thu, 02 Oct 2014 09:42:18 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/02/2014 09:14 AM, Alban Bedel wrote: > > 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. > > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > > --- > > board/avionic-design/common/tamonten.c | 4 +++- > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c > > index 9c86779..ea2425a 100644 > > --- a/board/avionic-design/common/tamonten.c > > +++ b/board/avionic-design/common/tamonten.c > > @@ -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: > > #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. > ... 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
On 10/02/2014 10:12 AM, Alban Bedel wrote: > On Thu, 02 Oct 2014 09:42:18 -0600 > Stephen Warren <swarren@wwwdotorg.org> wrote: > >> On 10/02/2014 09:14 AM, Alban Bedel wrote: >>> 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. > >>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> >>> --- >>> board/avionic-design/common/tamonten.c | 4 +++- >>> 1 file changed, 3 insertions(+), 1 deletion(-) >>> >>> diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c >>> index 9c86779..ea2425a 100644 >>> --- a/board/avionic-design/common/tamonten.c >>> +++ b/board/avionic-design/common/tamonten.c >>> @@ -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: >> >> #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. Hmm. CC Simon to comment on which GPIO drivers are available in SPL/non-SPL, and why the above ifdef doesn't work. >> ... 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. > 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.
Hi Alban, On 2 October 2014 10:12, Alban Bedel <alban.bedel@avionic-design.de> wrote: > > On Thu, 02 Oct 2014 09:42:18 -0600 > Stephen Warren <swarren@wwwdotorg.org> wrote: > > > On 10/02/2014 09:14 AM, Alban Bedel wrote: > > > 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. > > > > > Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> > > > --- > > > board/avionic-design/common/tamonten.c | 4 +++- > > > 1 file changed, 3 insertions(+), 1 deletion(-) > > > > > > diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c > > > index 9c86779..ea2425a 100644 > > > --- a/board/avionic-design/common/tamonten.c > > > +++ b/board/avionic-design/common/tamonten.c > > > @@ -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: > > > > #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. 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. > > > ... 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
On 10/02/2014 01:05 PM, Simon Glass wrote: > Hi Alban, > > On 2 October 2014 10:12, Alban Bedel <alban.bedel@avionic-design.de> wrote: >> >> On Thu, 02 Oct 2014 09:42:18 -0600 >> Stephen Warren <swarren@wwwdotorg.org> wrote: >> >>> On 10/02/2014 09:14 AM, Alban Bedel wrote: >>>> 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. > >> >> >>>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> >>>> --- >>>> board/avionic-design/common/tamonten.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c >>>> index 9c86779..ea2425a 100644 >>>> --- a/board/avionic-design/common/tamonten.c >>>> +++ b/board/avionic-design/common/tamonten.c >>>> @@ -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: >>> >>> #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. > > 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. That's roughly what I expected. Given that, I fail to see why the following wouldn't work then: >>> #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?)
Hi Stephen, On 2 October 2014 12:39, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/02/2014 10:12 AM, Alban Bedel wrote: >> >> On Thu, 02 Oct 2014 09:42:18 -0600 >> Stephen Warren <swarren@wwwdotorg.org> wrote: >> >>> On 10/02/2014 09:14 AM, Alban Bedel wrote: >>>> >>>> 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. >> >>>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> >>>> --- >>>> board/avionic-design/common/tamonten.c | 4 +++- >>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/board/avionic-design/common/tamonten.c >>>> b/board/avionic-design/common/tamonten.c >>>> index 9c86779..ea2425a 100644 >>>> --- a/board/avionic-design/common/tamonten.c >>>> +++ b/board/avionic-design/common/tamonten.c >>>> @@ -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: >>> >>> #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. > > > 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. > >>> ... 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. > > > 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. > >> 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. Regards, Simon
Hi Stephen, On 2 October 2014 13:08, Stephen Warren <swarren@wwwdotorg.org> wrote: > On 10/02/2014 01:05 PM, Simon Glass wrote: >> >> Hi Alban, >> >> On 2 October 2014 10:12, Alban Bedel <alban.bedel@avionic-design.de> >> wrote: >>> >>> >>> On Thu, 02 Oct 2014 09:42:18 -0600 >>> Stephen Warren <swarren@wwwdotorg.org> wrote: >>> >>>> On 10/02/2014 09:14 AM, Alban Bedel wrote: >>>>> >>>>> 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. >> >>> >>> >>>>> Signed-off-by: Alban Bedel <alban.bedel@avionic-design.de> >>>>> --- >>>>> board/avionic-design/common/tamonten.c | 4 +++- >>>>> 1 file changed, 3 insertions(+), 1 deletion(-) >>>>> >>>>> diff --git a/board/avionic-design/common/tamonten.c >>>>> b/board/avionic-design/common/tamonten.c >>>>> index 9c86779..ea2425a 100644 >>>>> --- a/board/avionic-design/common/tamonten.c >>>>> +++ b/board/avionic-design/common/tamonten.c >>>>> @@ -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: >>>> >>>> #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. >> >> >> 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. > > > That's roughly what I expected. Given that, I fail to see why the following > wouldn't work then: > >>>> #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?) 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
diff --git a/board/avionic-design/common/tamonten.c b/board/avionic-design/common/tamonten.c index 9c86779..ea2425a 100644 --- a/board/avionic-design/common/tamonten.c +++ b/board/avionic-design/common/tamonten.c @@ -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
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 <alban.bedel@avionic-design.de> --- board/avionic-design/common/tamonten.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)