Patchwork [U-Boot,1/3] i.MX6: mx6qsabrelite: add CONFIG_REVISION_TAG

login
register
mail settings
Submitter Eric Nelson
Date March 2, 2012, 10:55 p.m.
Message ID <1330728909-12203-2-git-send-email-eric.nelson@boundarydevices.com>
Download mbox | patch
Permalink /patch/144376/
State Superseded
Headers show

Comments

Eric Nelson - March 2, 2012, 10:55 p.m.
Freescale 2.6.38 (Non-DT) kernels require the revision atag to
 enable the VPU.

Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
---
 board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
 include/configs/mx6qsabrelite.h               |    1 +
 2 files changed, 8 insertions(+), 0 deletions(-)
Marek Vasut - March 2, 2012, 10:59 p.m.
>  Freescale 2.6.38 (Non-DT) kernels require the revision atag to
>  enable the VPU.
> 
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> ---
>  board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
>  include/configs/mx6qsabrelite.h               |    1 +
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index db1bea9..590030b
> 100644
> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
>  }
>  #endif
> 
> +#ifdef CONFIG_REVISION_TAG
> +u32 get_board_rev(void)
> +{
> +	return 0x63000 ;
> +}
> +#endif
> +
>  #ifdef CONFIG_MXC_SPI
>  iomux_v3_cfg_t ecspi1_pads[] = {
>  	/* SS1 */
> diff --git a/include/configs/mx6qsabrelite.h
> b/include/configs/mx6qsabrelite.h index 93000f0..85f6f7a 100644
> --- a/include/configs/mx6qsabrelite.h
> +++ b/include/configs/mx6qsabrelite.h
> @@ -33,6 +33,7 @@
>  #define CONFIG_CMDLINE_TAG
>  #define CONFIG_SETUP_MEMORY_TAGS
>  #define CONFIG_INITRD_TAG
> +#define CONFIG_REVISION_TAG

I think you can avoid this define altogether, you're using it anyway.

M
Stefano Babic - March 3, 2012, 10:26 a.m.
On 02/03/2012 23:55, Eric Nelson wrote:
>  Freescale 2.6.38 (Non-DT) kernels require the revision atag to
>  enable the VPU.
> 
> Signed-off-by: Eric Nelson <eric.nelson@boundarydevices.com>
> ---
>  board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
>  include/configs/mx6qsabrelite.h               |    1 +
>  2 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> index db1bea9..590030b 100644
> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
>  }
>  #endif
>  
> +#ifdef CONFIG_REVISION_TAG

You add CONFIG_REVISION_TAG, then you can get rid of unneeded #ifdef

Best regards,
Stefano Babic
Eric Nelson - March 4, 2012, 8:35 p.m.
On 03/02/2012 03:59 PM, Marek Vasut wrote:
>>   Freescale 2.6.38 (Non-DT) kernels require the revision atag to
>>   enable the VPU.
>>
>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>> ---
>>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
>>   include/configs/mx6qsabrelite.h               |    1 +
>>   2 files changed, 8 insertions(+), 0 deletions(-)
>>
>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index db1bea9..590030b
>> 100644
>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
>>   }
>>   #endif
>>
>> +#ifdef CONFIG_REVISION_TAG
>> +u32 get_board_rev(void)
>> +{
>> +	return 0x63000 ;
>> +}
>> +#endif
>> +
>>   #ifdef CONFIG_MXC_SPI
>>   iomux_v3_cfg_t ecspi1_pads[] = {
>>   	/* SS1 */
>> diff --git a/include/configs/mx6qsabrelite.h
>> b/include/configs/mx6qsabrelite.h index 93000f0..85f6f7a 100644
>> --- a/include/configs/mx6qsabrelite.h
>> +++ b/include/configs/mx6qsabrelite.h
>> @@ -33,6 +33,7 @@
>>   #define CONFIG_CMDLINE_TAG
>>   #define CONFIG_SETUP_MEMORY_TAGS
>>   #define CONFIG_INITRD_TAG
>> +#define CONFIG_REVISION_TAG
>
> I think you can avoid this define altogether, you're using it anyway.
>
Hi Marek,

I'm not sure I understand.

I need to define CONFIG_REVISION_TAG in order to get bootm to add
the tag:
	http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/lib/bootm.c;h=afa0093df7620606ee12b4dc1dd2ee37adee347c;hb=master#l137

I made get_board_rev() conditional so we can get rid of it if (once)
we're only using DT kernels.
Marek Vasut - March 4, 2012, 8:59 p.m.
> On 03/02/2012 03:59 PM, Marek Vasut wrote:
> >>   Freescale 2.6.38 (Non-DT) kernels require the revision atag to
> >>   enable the VPU.
> >> 
> >> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >> ---
> >> 
> >>   board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
> >>   include/configs/mx6qsabrelite.h               |    1 +
> >>   2 files changed, 8 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index db1bea9..590030b
> >> 100644
> >> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
> >> 
> >>   }
> >>   #endif
> >> 
> >> +#ifdef CONFIG_REVISION_TAG
> >> +u32 get_board_rev(void)
> >> +{
> >> +	return 0x63000 ;
> >> +}
> >> +#endif
> >> +
> >> 
> >>   #ifdef CONFIG_MXC_SPI
> >>   iomux_v3_cfg_t ecspi1_pads[] = {
> >>   
> >>   	/* SS1 */
> >> 
> >> diff --git a/include/configs/mx6qsabrelite.h
> >> b/include/configs/mx6qsabrelite.h index 93000f0..85f6f7a 100644
> >> --- a/include/configs/mx6qsabrelite.h
> >> +++ b/include/configs/mx6qsabrelite.h
> >> @@ -33,6 +33,7 @@
> >> 
> >>   #define CONFIG_CMDLINE_TAG
> >>   #define CONFIG_SETUP_MEMORY_TAGS
> >>   #define CONFIG_INITRD_TAG
> >> 
> >> +#define CONFIG_REVISION_TAG
> > 
> > I think you can avoid this define altogether, you're using it anyway.
> 
> Hi Marek,
> 
> I'm not sure I understand.
> 
> I need to define CONFIG_REVISION_TAG in order to get bootm to add
> the tag:
> 	http://git.denx.de/?p=u-
boot.git;a=blob;f=arch/arm/lib/bootm.c;h=afa0093df
> 7620606ee12b4dc1dd2ee37adee347c;hb=master#l137
> 
> I made get_board_rev() conditional so we can get rid of it if (once)
> we're only using DT kernels.

You have CONFIG_REVISION_TAG set unconditionally, so you don't need the macro 
around that get_revision() function. I might be wrong.

M
Eric Nelson - March 4, 2012, 9:04 p.m.
On 03/04/2012 01:59 PM, Marek Vasut wrote:
>> On 03/02/2012 03:59 PM, Marek Vasut wrote:
>>>>    Freescale 2.6.38 (Non-DT) kernels require the revision atag to
>>>>    enable the VPU.
>>>>
>>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
>>>> ---
>>>>
>>>>    board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
>>>>    include/configs/mx6qsabrelite.h               |    1 +
>>>>    2 files changed, 8 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>>> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index db1bea9..590030b
>>>> 100644
>>>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
>>>> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
>>>>
>>>>    }
>>>>    #endif
>>>>
>>>> +#ifdef CONFIG_REVISION_TAG
>>>> +u32 get_board_rev(void)
>>>> +{
>>>> +	return 0x63000 ;
>>>> +}
>>>> +#endif
>>>> +
>>>>
>>>>    #ifdef CONFIG_MXC_SPI
>>>>    iomux_v3_cfg_t ecspi1_pads[] = {
>>>>
>>>>    	/* SS1 */
>>>>
>>>> diff --git a/include/configs/mx6qsabrelite.h
>>>> b/include/configs/mx6qsabrelite.h index 93000f0..85f6f7a 100644
>>>> --- a/include/configs/mx6qsabrelite.h
>>>> +++ b/include/configs/mx6qsabrelite.h
>>>> @@ -33,6 +33,7 @@
>>>>
>>>>    #define CONFIG_CMDLINE_TAG
>>>>    #define CONFIG_SETUP_MEMORY_TAGS
>>>>    #define CONFIG_INITRD_TAG
>>>>
>>>> +#define CONFIG_REVISION_TAG
>>>
>>> I think you can avoid this define altogether, you're using it anyway.
>>
>> Hi Marek,
>>
>> I'm not sure I understand.
>>
>> I need to define CONFIG_REVISION_TAG in order to get bootm to add
>> the tag:
>> 	http://git.denx.de/?p=u-
> boot.git;a=blob;f=arch/arm/lib/bootm.c;h=afa0093df
>> 7620606ee12b4dc1dd2ee37adee347c;hb=master#l137
>>
>> I made get_board_rev() conditional so we can get rid of it if (once)
>> we're only using DT kernels.
>
> You have CONFIG_REVISION_TAG set unconditionally, so you don't need the macro
> around that get_revision() function. I might be wrong.
>

You're right. Its just that I did that deliberately so you can save the
(admittedly) small amount of code by editing mx6qsabrelite.h.

Since U-Boot doesn't really have a configuration step, I find that
folks tend to tweak the board configuration file quite a bit based
on their needs.

Though it's a bit of a hack, we even formalized it on our i.MX5x products
to allow customers to keep their git repositories clean:
	http://boundarydevices.com/blogs/customizing-u-boot-on-i-mx
Marek Vasut - March 4, 2012, 9:18 p.m.
> On 03/04/2012 01:59 PM, Marek Vasut wrote:
> >> On 03/02/2012 03:59 PM, Marek Vasut wrote:
> >>>>    Freescale 2.6.38 (Non-DT) kernels require the revision atag to
> >>>>    enable the VPU.
> >>>> 
> >>>> Signed-off-by: Eric Nelson<eric.nelson@boundarydevices.com>
> >>>> ---
> >>>> 
> >>>>    board/freescale/mx6qsabrelite/mx6qsabrelite.c |    7 +++++++
> >>>>    include/configs/mx6qsabrelite.h               |    1 +
> >>>>    2 files changed, 8 insertions(+), 0 deletions(-)
> >>>> 
> >>>> diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >>>> b/board/freescale/mx6qsabrelite/mx6qsabrelite.c index db1bea9..590030b
> >>>> 100644
> >>>> --- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >>>> +++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
> >>>> @@ -215,6 +215,13 @@ int board_mmc_init(bd_t *bis)
> >>>> 
> >>>>    }
> >>>>    #endif
> >>>> 
> >>>> +#ifdef CONFIG_REVISION_TAG
> >>>> +u32 get_board_rev(void)
> >>>> +{
> >>>> +	return 0x63000 ;
> >>>> +}
> >>>> +#endif
> >>>> +
> >>>> 
> >>>>    #ifdef CONFIG_MXC_SPI
> >>>>    iomux_v3_cfg_t ecspi1_pads[] = {
> >>>>    
> >>>>    	/* SS1 */
> >>>> 
> >>>> diff --git a/include/configs/mx6qsabrelite.h
> >>>> b/include/configs/mx6qsabrelite.h index 93000f0..85f6f7a 100644
> >>>> --- a/include/configs/mx6qsabrelite.h
> >>>> +++ b/include/configs/mx6qsabrelite.h
> >>>> @@ -33,6 +33,7 @@
> >>>> 
> >>>>    #define CONFIG_CMDLINE_TAG
> >>>>    #define CONFIG_SETUP_MEMORY_TAGS
> >>>>    #define CONFIG_INITRD_TAG
> >>>> 
> >>>> +#define CONFIG_REVISION_TAG
> >>> 
> >>> I think you can avoid this define altogether, you're using it anyway.
> >> 
> >> Hi Marek,
> >> 
> >> I'm not sure I understand.
> >> 
> >> I need to define CONFIG_REVISION_TAG in order to get bootm to add
> >> 
> >> the tag:
> >> 	http://git.denx.de/?p=u-
> > 
> > boot.git;a=blob;f=arch/arm/lib/bootm.c;h=afa0093df
> > 
> >> 7620606ee12b4dc1dd2ee37adee347c;hb=master#l137
> >> 
> >> I made get_board_rev() conditional so we can get rid of it if (once)
> >> we're only using DT kernels.
> > 
> > You have CONFIG_REVISION_TAG set unconditionally, so you don't need the
> > macro around that get_revision() function. I might be wrong.
> 
> You're right. Its just that I did that deliberately so you can save the
> (admittedly) small amount of code by editing mx6qsabrelite.h.

Shouldn't that be removed by the compiler anyway?
> 
> Since U-Boot doesn't really have a configuration step, I find that
> folks tend to tweak the board configuration file quite a bit based
> on their needs.

This needs to be fixed, we need kbuild. Who's up to doing it ? ;-D
> 
> Though it's a bit of a hack, we even formalized it on our i.MX5x products
> to allow customers to keep their git repositories clean:
> 	http://boundarydevices.com/blogs/customizing-u-boot-on-i-mx

That's good, thank you :)

Cheers!
M
Wolfgang Denk - March 4, 2012, 10:06 p.m.
Dear Eric Nelson,

In message <4F53D8E6.4000408@boundarydevices.com> you wrote:
>
> Since U-Boot doesn't really have a configuration step, I find that
> folks tend to tweak the board configuration file quite a bit based
> on their needs.

Yes, peaople do that a lot, nd it has always been a very good idea to
at least detect such modifications of the code.  The worst thing to
happen is when you receive a bug report for version FOO, and you have
no chance of detecting that somebody actually messed with the
configuration and/or the code.

> Though it's a bit of a hack, we even formalized it on our i.MX5x products
> to allow customers to keep their git repositories clean:
> 	....
[Link deleted to not further distribute this horrible stuff.]

Frankly, this "keep their git repositories clean" is a really, really
bad idea.  We pay special care to add the git commit ID to the U-Boot
version string so you can easily detect if somebody messed with the
code, and you come up and invent methods of circumventing that.

This is totally counterproductive.

If you use this, that's your business.  But please do me the favour and
do not recommend this to others.


Thanks.


Wolfgang Denk
Eric Nelson - March 4, 2012, 10:41 p.m.
On 03/04/2012 03:06 PM, Wolfgang Denk wrote:
> Dear Eric Nelson,
>
> In message<4F53D8E6.4000408@boundarydevices.com>  you wrote:
>>
>> Since U-Boot doesn't really have a configuration step, I find that
>> folks tend to tweak the board configuration file quite a bit based
>> on their needs.
>
> Yes, peaople do that a lot, nd it has always been a very good idea to
> at least detect such modifications of the code.  The worst thing to
> happen is when you receive a bug report for version FOO, and you have
> no chance of detecting that somebody actually messed with the
> configuration and/or the code.
>
>> Though it's a bit of a hack, we even formalized it on our i.MX5x products
>> to allow customers to keep their git repositories clean:
>> 	....
> [Link deleted to not further distribute this horrible stuff.]
>
> Frankly, this "keep their git repositories clean" is a really, really
> bad idea.  We pay special care to add the git commit ID to the U-Boot
> version string so you can easily detect if somebody messed with the
> code, and you come up and invent methods of circumventing that.
>
> This is totally counterproductive.
>
> If you use this, that's your business.  But please do me the favour and
> do not recommend this to others.
>

We don't. Sorry if I implied otherwise.

We recommend that customers keep their own repositories with changes
from our "standard" releases, but we have a number of customers whose
complete set of differences amounts to default environment variables
and/or choices of serial consoles for which we don't want to carry and
publish an entire configuration.

This hack was borne out of frustration when getting a 'revision-dirty'
reference back from a customer because they've changed something
simple (usually environment settings).

Patch

diff --git a/board/freescale/mx6qsabrelite/mx6qsabrelite.c b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
index db1bea9..590030b 100644
--- a/board/freescale/mx6qsabrelite/mx6qsabrelite.c
+++ b/board/freescale/mx6qsabrelite/mx6qsabrelite.c
@@ -215,6 +215,13 @@  int board_mmc_init(bd_t *bis)
 }
 #endif
 
+#ifdef CONFIG_REVISION_TAG
+u32 get_board_rev(void)
+{
+	return 0x63000 ;
+}
+#endif
+
 #ifdef CONFIG_MXC_SPI
 iomux_v3_cfg_t ecspi1_pads[] = {
 	/* SS1 */
diff --git a/include/configs/mx6qsabrelite.h b/include/configs/mx6qsabrelite.h
index 93000f0..85f6f7a 100644
--- a/include/configs/mx6qsabrelite.h
+++ b/include/configs/mx6qsabrelite.h
@@ -33,6 +33,7 @@ 
 #define CONFIG_CMDLINE_TAG
 #define CONFIG_SETUP_MEMORY_TAGS
 #define CONFIG_INITRD_TAG
+#define CONFIG_REVISION_TAG
 
 /* Size of malloc() pool */
 #define CONFIG_SYS_MALLOC_LEN          (CONFIG_ENV_SIZE + 2 * 1024 * 1024)