Patchwork [U-Boot,v2,01/10] x86: Change board baud_rate to ulong

login
register
mail settings
Submitter Simon Glass
Date Oct. 10, 2012, 11:12 p.m.
Message ID <1349910781-32088-2-git-send-email-sjg@chromium.org>
Download mbox | patch
Permalink /patch/190814/
State Superseded, archived
Delegated to: Simon Glass
Headers show

Comments

Simon Glass - Oct. 10, 2012, 11:12 p.m.
This is a ulong for some architectures and just unsigned for others.
Change x86 to be consistent.

Signed-off-by: Simon Glass <sjg@chromium.org>
---

 arch/x86/include/asm/u-boot.h |    2 +-
 common/cmd_bdinfo.c           |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Graeme Russ - Oct. 11, 2012, 12:21 a.m.
Hi Simon,

Let's do it by the book this time :)

On Thu, Oct 11, 2012 at 10:12 AM, Simon Glass <sjg@chromium.org> wrote:
> This is a ulong for some architectures and just unsigned for others.
> Change x86 to be consistent.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>  arch/x86/include/asm/u-boot.h |    2 +-
>  common/cmd_bdinfo.c           |    2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/x86/include/asm/u-boot.h b/arch/x86/include/asm/u-boot.h
> index da667c5..0671b8d 100644
> --- a/arch/x86/include/asm/u-boot.h
> +++ b/arch/x86/include/asm/u-boot.h
> @@ -48,7 +48,7 @@ typedef struct bd_info {
>         unsigned short  bi_ethspeed;    /* Ethernet speed in Mbps */
>         unsigned long   bi_intfreq;     /* Internal Freq, in MHz */
>         unsigned long   bi_busfreq;     /* Bus Freq, in MHz */
> -       unsigned int    bi_baudrate;    /* Console Baudrate */
> +       unsigned long   bi_baudrate;    /* Console Baudrate */
>         unsigned long   bi_boot_params; /* where this board expects params */
>         struct                          /* RAM configuration */
>         {
> diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
> index 23bd8a5..9bc0ebc 100644
> --- a/common/cmd_bdinfo.c
> +++ b/common/cmd_bdinfo.c
> @@ -439,7 +439,7 @@ int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
>         printf("ip_addr     = %s\n", getenv("ipaddr"));
>         print_mhz("ethspeed",       bd->bi_ethspeed);
>  #endif
> -       printf("baudrate    = %d bps\n", bd->bi_baudrate);
> +       printf("baudrate    = %ld bps\n", bd->bi_baudrate);
>
>         return 0;
>  }
> --
> 1.7.7.3
>

Acked-by: Graeme Russ <graeme.russ@gmail.com>
Wolfgang Denk - Oct. 11, 2012, 7:32 a.m.
Dear Simon Glass,

In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
> This is a ulong for some architectures and just unsigned for others.
> Change x86 to be consistent.

Given the limited range for this variable it makes no sense to use a
long for this.  Please fix this the other way round, i. e. change the
architectures that use a long.

Thanks.

Best regards,

Wolfgang Denk
Graeme Russ - Oct. 11, 2012, 8:38 a.m.
Hi Wolfgang,

On Oct 11, 2012 6:32 PM, "Wolfgang Denk" <wd@denx.de> wrote:
>
> Dear Simon Glass,
>
> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
> > This is a ulong for some architectures and just unsigned for others.
> > Change x86 to be consistent.
>
> Given the limited range for this variable it makes no sense to use a
> long for this.  Please fix this the other way round, i. e. change the
> architectures that use a long.

Will unsigned hold 115200?

Regards,

Graeme
Simon Glass - Oct. 12, 2012, 12:46 a.m.
Hi Wolfgang,

On Thu, Oct 11, 2012 at 12:32 AM, Wolfgang Denk <wd@denx.de> wrote:
> Dear Simon Glass,
>
> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
>> This is a ulong for some architectures and just unsigned for others.
>> Change x86 to be consistent.
>
> Given the limited range for this variable it makes no sense to use a
> long for this.  Please fix this the other way round, i. e. change the
> architectures that use a long.

OK I will send out a series that changes them to unsigned long.

This sort of thing relates to the unified board init series I was
working on earlier it the year, since then all archs can use the same
structures for generic fields like baud rate. I hope to get back to
that one day.

Regards,
Simon

>
> Thanks.
>
> Best regards,
>
> Wolfgang Denk
>
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
> Always try to do things in chronological order; it's  less  confusing
> that way.
Graeme Russ - Oct. 12, 2012, 12:51 a.m.
Hi Simon,

On Fri, Oct 12, 2012 at 11:46 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Wolfgang,
>
> On Thu, Oct 11, 2012 at 12:32 AM, Wolfgang Denk <wd@denx.de> wrote:
>> Dear Simon Glass,
>>
>> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
>>> This is a ulong for some architectures and just unsigned for others.
>>> Change x86 to be consistent.
>>
>> Given the limited range for this variable it makes no sense to use a
>> long for this.  Please fix this the other way round, i. e. change the
>> architectures that use a long.
>
> OK I will send out a series that changes them to unsigned long.

Should we just change them all to u32 to be clear on the value range?

Regards,

Graeme
Simon Glass - Oct. 12, 2012, 12:55 a.m.
Hi,

On Thu, Oct 11, 2012 at 5:51 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Oct 12, 2012 at 11:46 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Wolfgang,
>>
>> On Thu, Oct 11, 2012 at 12:32 AM, Wolfgang Denk <wd@denx.de> wrote:
>>> Dear Simon Glass,
>>>
>>> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
>>>> This is a ulong for some architectures and just unsigned for others.
>>>> Change x86 to be consistent.
>>>
>>> Given the limited range for this variable it makes no sense to use a
>>> long for this.  Please fix this the other way round, i. e. change the
>>> architectures that use a long.
>>
>> OK I will send out a series that changes them to unsigned long.
>
> Should we just change them all to u32 to be clear on the value range?

Sorry, I meant unsigned int.

Are there architectures in U-Boot which use a 16-bit int?

Regards,
Simon

>
> Regards,
>
> Graeme
Graeme Russ - Oct. 12, 2012, 1 a.m.
Hi Simon,

On Fri, Oct 12, 2012 at 11:55 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi,
>
> On Thu, Oct 11, 2012 at 5:51 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>> Hi Simon,
>>
>> On Fri, Oct 12, 2012 at 11:46 AM, Simon Glass <sjg@chromium.org> wrote:
>>> Hi Wolfgang,
>>>
>>> On Thu, Oct 11, 2012 at 12:32 AM, Wolfgang Denk <wd@denx.de> wrote:
>>>> Dear Simon Glass,
>>>>
>>>> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
>>>>> This is a ulong for some architectures and just unsigned for others.
>>>>> Change x86 to be consistent.
>>>>
>>>> Given the limited range for this variable it makes no sense to use a
>>>> long for this.  Please fix this the other way round, i. e. change the
>>>> architectures that use a long.
>>>
>>> OK I will send out a series that changes them to unsigned long.
>>
>> Should we just change them all to u32 to be clear on the value range?
>
> Sorry, I meant unsigned int.
>
> Are there architectures in U-Boot which use a 16-bit int?

The C standard does not guarantee unsigned int will be at least
32-bits. It is possible (although I have not checked it) that
compiling 'real-mode' x86 code can produce 16-bit ints (not that we do
that in U-Boot)

using u32 will always guarantee a 32-bit unsigned value - better to be
safe than sorry

Regards,

Graeme
Simon Glass - Oct. 12, 2012, 1:14 a.m.
Hi Graham,

On Thu, Oct 11, 2012 at 6:00 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
> Hi Simon,
>
> On Fri, Oct 12, 2012 at 11:55 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi,
>>
>> On Thu, Oct 11, 2012 at 5:51 PM, Graeme Russ <graeme.russ@gmail.com> wrote:
>>> Hi Simon,
>>>
>>> On Fri, Oct 12, 2012 at 11:46 AM, Simon Glass <sjg@chromium.org> wrote:
>>>> Hi Wolfgang,
>>>>
>>>> On Thu, Oct 11, 2012 at 12:32 AM, Wolfgang Denk <wd@denx.de> wrote:
>>>>> Dear Simon Glass,
>>>>>
>>>>> In message <1349910781-32088-2-git-send-email-sjg@chromium.org> you wrote:
>>>>>> This is a ulong for some architectures and just unsigned for others.
>>>>>> Change x86 to be consistent.
>>>>>
>>>>> Given the limited range for this variable it makes no sense to use a
>>>>> long for this.  Please fix this the other way round, i. e. change the
>>>>> architectures that use a long.
>>>>
>>>> OK I will send out a series that changes them to unsigned long.
>>>
>>> Should we just change them all to u32 to be clear on the value range?
>>
>> Sorry, I meant unsigned int.
>>
>> Are there architectures in U-Boot which use a 16-bit int?
>
> The C standard does not guarantee unsigned int will be at least
> 32-bits. It is possible (although I have not checked it) that
> compiling 'real-mode' x86 code can produce 16-bit ints (not that we do
> that in U-Boot)
>
> using u32 will always guarantee a 32-bit unsigned value - better to be
> safe than sorry
>

That's fine, but what about all the other fields? Does the same apply
here? It seems that everything uses 'unsigned int' for u32:

arch/nds32/include/asm/types.h:47:typedef unsigned int u32;
arch/avr32/include/asm/types.h:65:typedef unsigned int u32;
arch/arm/include/asm/types.h:37:typedef unsigned int u32;
arch/sandbox/include/asm/types.h:59:typedef unsigned int u32;
arch/blackfin/include/asm/types.h:70:typedef unsigned int u32;
arch/openrisc/include/asm/types.h:64:typedef unsigned int u32;
arch/mips/include/asm/types.h:62:typedef unsigned int u32;
arch/x86/include/asm/types.h:37:typedef unsigned int u32;
arch/nios2/include/asm/types.h:45:typedef unsigned int u32;
arch/sh/include/asm/types.h:46:typedef unsigned int u32;
arch/microblaze/include/asm/types.h:45:typedef unsigned int u32;
arch/powerpc/include/asm/types.h:37:typedef unsigned int u32;
arch/m68k/include/asm/types.h:37:typedef unsigned int u32;
arch/sparc/include/asm/types.h:58:typedef unsigned int u32;


I wonder then if we need to decide which to use (int or long). I
presume 'long' was used to cope with 16-bit machines, but we don't
actually have any (which is presumably Wolfgang's point). If we don't
have any, then 'int' is safe. I'm just a bit unsure about changing a
single field in these two structures to u32, when all the others are
unsigned long/short.

For example:

typedef struct bd_info {
    int			bi_baudrate;	/* serial console baudrate */
    ulong	        bi_arch_number;	/* unique id for this board */
    ulong	        bi_boot_params;	/* where this board expects params */
	unsigned long	bi_arm_freq; /* arm frequency */
	unsigned long	bi_dsp_freq; /* dsp core frequency */
	unsigned long	bi_ddr_freq; /* ddr frequency */
    struct				/* RAM configuration */
    {
	ulong start;
	ulong size;
    }			bi_dram[CONFIG_NR_DRAM_BANKS];
} bd_t;


typedef	struct	global_data {
	bd_t		*bd;
	unsigned long	flags;
	unsigned long	baudrate;
	unsigned long	have_console;	/* serial_init() was called */
#ifdef CONFIG_PRE_CONSOLE_BUFFER
	unsigned long	precon_buf_idx;	/* Pre-Console buffer index */
#endif
	unsigned long	env_addr;	/* Address  of Environment struct */
	unsigned long	env_valid;	/* Checksum of Environment valid? */
	unsigned long	fb_base;	/* base address of frame buffer */
#ifdef CONFIG_FSL_ESDHC
	unsigned long	sdhc_clk;
#endif
#ifdef CONFIG_AT91FAMILY
	/* "static data" needed by at91's clock.c */
	unsigned long	cpu_clk_rate_hz;
	unsigned long	main_clk_rate_hz;
	unsigned long	mck_rate_hz;
	unsigned long	plla_rate_hz;
	unsigned long	pllb_rate_hz;
	unsigned long	at91_pllb_usb_init;
#endif
#ifdef CONFIG_ARM
	/* "static data" needed by most of timer.c on ARM platforms */
	unsigned long	timer_rate_hz;
	unsigned long	tbl;
	unsigned long	tbu;
	unsigned long long	timer_reset_value;
	unsigned long	lastinc;


Guidance appreciated!

....

> Regards,
>
> Graeme

Regards,
Simon

Patch

diff --git a/arch/x86/include/asm/u-boot.h b/arch/x86/include/asm/u-boot.h
index da667c5..0671b8d 100644
--- a/arch/x86/include/asm/u-boot.h
+++ b/arch/x86/include/asm/u-boot.h
@@ -48,7 +48,7 @@  typedef struct bd_info {
 	unsigned short	bi_ethspeed;	/* Ethernet speed in Mbps */
 	unsigned long	bi_intfreq;	/* Internal Freq, in MHz */
 	unsigned long	bi_busfreq;	/* Bus Freq, in MHz */
-	unsigned int	bi_baudrate;	/* Console Baudrate */
+	unsigned long	bi_baudrate;	/* Console Baudrate */
 	unsigned long   bi_boot_params;	/* where this board expects params */
 	struct				/* RAM configuration */
 	{
diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
index 23bd8a5..9bc0ebc 100644
--- a/common/cmd_bdinfo.c
+++ b/common/cmd_bdinfo.c
@@ -439,7 +439,7 @@  int do_bdinfo(cmd_tbl_t *cmdtp, int flag, int argc, char * const argv[])
 	printf("ip_addr     = %s\n", getenv("ipaddr"));
 	print_mhz("ethspeed",	    bd->bi_ethspeed);
 #endif
-	printf("baudrate    = %d bps\n", bd->bi_baudrate);
+	printf("baudrate    = %ld bps\n", bd->bi_baudrate);
 
 	return 0;
 }