[U-Boot] [PATCH v3 1/4] Add support new arch: c6x

[Mike Frysinger <vapier@gentoo.org>]

On Monday 25 June 2012 14:02:38 Dmitry Bondar wrote:
> C6X (C6000) is Texas Instruments architecture of fixed and floating-point
> DSPs. This patch add basic support.
> Many of code in arch/c6x/include/asm come from c6x-linux project
> (http://linux-c6x.org)

you should also update README and doc/README.standalone with some notes about 
the C6X architecture.  just search for "blackfin" for the areas you should fill 
in.

> --- /dev/null
> +++ b/arch/c6x/config.mk
> @@ -0,0 +1 @@
> +CROSS_COMPILE ?= c6x-uclinux-

missing intro comment block w/copyright/licnese.  look at any other arch's 
config.mk to see an example.

you should prob also add a default CONFIG_STANDALONE_LOAD_ADDR

> --- /dev/null
> +++ b/arch/c6x/include/asm/global_data.h
>
> +#ifndef	__ASM_GBL_DATA_H

should be space after the ifndef, not a tab

> +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 */
> +
> +	/* "static data" needed by most of timer.c (like on ARM platform) */
> +	unsigned long	timer_rate_hz;
> +	unsigned long	tbl;
> +	unsigned long	tbu;
> +	unsigned long long	timer_reset_value;
> +	unsigned long	lastinc;
> +
> +	phys_size_t     ram_size;       /* RAM size */
> +#if 0
> +	unsigned long	relocaddr;	/* Start address of U-Boot in RAM */
> +	phys_size_t	ram_size;	/* RAM size */
> +	unsigned long	mon_len;	/* monitor len */
> +	unsigned long	irq_sp;		/* irq stack pointer */
> +	unsigned long	start_addr_sp;	/* start_addr_stackpointer */
> +	unsigned long	reloc_off;
> +#endif

delete dead code.  you should also kill any fields you don't actually need 
rather than just copying from another arch.  the 
timer_rate_hz/tbl/tbu/timer_reset_value/lastinc look suspicious to me.

> --- /dev/null
> +++ b/arch/c6x/include/asm/sizes.h

do you actually need this ?  i don't think you do, so kill it.  if you have 
some file that includes it, fix it and get rid of this file.

> --- /dev/null
> +++ b/arch/c6x/include/asm/u-boot-c6x.h

please rename to sections.h

> +#ifndef _U_BOOT_C6X_H_
> +#define _U_BOOT_C6X_H_ 1

no need to define to 1

> --- /dev/null
> +++ b/arch/c6x/include/asm/u-boot.h
>
> +typedef struct bd_info {
> +	int		bi_baudrate;	/* serial console baudrate */
> +	unsigned long	bi_ip_addr;	/* IP Address */
> +	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;

again, please go through here and delete members you don't actually need

> +/* For image.h:image_check_target_arch() */
> +#define IH_ARCH_DEFAULT IH_ARCH_ARM

brrrr-wrong

> --- /dev/null
> +++ b/arch/c6x/lib/board.c
>
> +#ifdef CONFIG_STATUS_LED
> +#include <status_led.h>
> +#endif

no need to conditionalize the include

> +static int display_banner(void)
> +{
> +	printf("\n\n%s\n\n", version_string);

just call display_options()

> +	WATCHDOG_RESET();
> +/*	interrupt_init();*/

delete dead code

> +int do_reset(cmd_tbl_t *cmdtp, int flag, int argc,
> +	char *const argv[])
> +{
> +	reset_cpu(0);
> +	return 0;
> +}
> +

delete trailing newlines

> --- /dev/null
> +++ b/arch/c6x/lib/bootm.c
>
> +int disable_interrupts()

(void)

> +void enable_interrupts()

(void)

> --- /dev/null
> +++ b/arch/c6x/lib/memcmp.c
>
> +int memcmp(const void *cs, const void *ct, size_t count)
> +{
> +	const unsigned char *su1, *su2;
> +
> +	for (su1 = cs, su2 = ct; 0 < count; ++su1, ++su2, count--)
> +		if (*su1 != *su2)
> +			return (*su1 < *su2) ? -1 : 1;
> +	return 0;
> +}
>
> --- /dev/null
> +++ b/arch/c6x/lib/memmove.c
>
> +void *memmove(void *s1, const void *s2, size_t n)
> +{
> +	register char *s = (char *) s1;
> +	register const char *p = (const char *) s2;
> +
> +	if (p >= s) {
> +		return memcpy(s, p, n);
> +	} else {
> +		while (n) {
> +			--n;
> +			s[n] = p[n];
> +		}
> +	}
> +	return s1;
> +}
>
> --- /dev/null
> +++ b/arch/c6x/lib/memset.c
>
> +void *memset(void *mem, register int ch, register size_t length)
> +{
> +	register char *m = (char *)mem - 1;
> +	while (length--)
> +		*++m = ch;
> +	return mem;
> +}

i can't see how these would be any better than the ones we already have in 
lib/string.c.  i say delete these files and don't bother.
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20120718/6039e63b/attachment.pgp>