[U-Boot] [PATCH 5/9] SPEAr600 SoC support added

[Wolfgang Denk]

Dear Vipin KUMAR,

In message <1260955110-5656-6-git-send-email-vipin.kumar at st.com> you wrote:
> 
> Signed-off-by: Vipin <vipin.kumar at st.com>
...
> +int misc_init_r(void)
> +{
> +#if defined(CONFIG_CMD_NET)
> +	uchar mac_id[6];
> +
> +	if (!i2c_read_mac(mac_id))
> +		eth_setenv_enetaddr("ethaddr", mac_id);
> +#endif
> +	setenv("verify", "n");

NAK.

Please do not enforce such a policy on all your users.  Let them
decide if they want this, or not.

...
> +int do_setfreq(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	void (*sram_setfreq) (unsigned int, unsigned int);
> +	unsigned int frequency;
> +
> +	if (argc != 3) {
> +		printf("Usage:\n%s\n", cmdtp->usage);
> +		return 1;
> +	}
> +
> +	frequency = simple_strtoul(argv[2], NULL, 0);
> +
> +	if (frequency > 333) {
> +		printf("Frequency is limited to 333MHz\n");
> +		return 1;
> +	}
> +	sram_setfreq = memcpy((void *)0xD2801000, setfreq, setfreq_sz);
> +
> +	if (!strcmp(argv[1], "ddr")) {
> +		sram_setfreq(DDR, frequency);
> +		printf("DDR frequency changed to %u\n", frequency);
> +
> +	} else if (!strcmp(argv[1], "cpu")) {
> +		sram_setfreq(CPU, frequency);
> +		printf("CPU frequency changed to %u\n", frequency);
> +	} else {
> +		printf("Usage:\n%s\n", cmdtp->usage);
> +		return 1;
> +	}
> +
> +	return 0;
> +}
> +
> +U_BOOT_CMD(setfreq, 3, 1, do_setfreq,
> +	   "change ddr/cpu frequency", "device[ddr/cpu] frequency");
> +#endif

Do we need a new custom command for this? Either allow setting by
environment variable only ("cpuclk" is already used by a number of
systems), or use a common command name ("chip_config" was chosen for
this after some discussion a while ago).

Also, is no further checking needed? Can I set arbitrary frequencies,
like 307 for CPU and 277 for DDR?

> +	if ((buf[0] == 0x55) && (buf[1] == 0xAA)) {
> +		/* Valid MAC address is saved in I2C EEPROM,
> +		   read the MAC address from the
> +		   EEPROM & update the buffer */

Incorrect multiline comment once more.


> +	buf[0] = 0x55;
> +	buf[1] = 0xAA;
> +	i2c_write(0x50,		/* Chip address */
> +		  0x0,		/* Offset */
> +		  1,		/* Address length */
> +		  buf,		/* buffer */
> +		  2);		/* Length */

Argh. Please do not do this.

> +	buf[0] = 0x44;
> +	buf[1] = 0x66;
> +
> +	i2c_read(0x50,		/* Chip address */
> +		 0x0,		/* Offset */
> +		 1,		/* Address length */
> +		 buf,		/* buffer */
> +		 2);		/* Length */

Ditto.

> diff --git a/common/cmd_bdinfo.c b/common/cmd_bdinfo.c
> index 11c1547..412cad6 100644
> --- a/common/cmd_bdinfo.c
> +++ b/common/cmd_bdinfo.c
> @@ -27,6 +27,10 @@
>  #include <common.h>
>  #include <command.h>
>  
> +#if defined(CONFIG_SPEAR600) || defined(CONFIG_SPEAR3XX)
> +#include <asm/arch/spr_xloader_table.h>
> +#endif
> +
>  DECLARE_GLOBAL_DATA_PTR;
>  
>  static void print_num(const char *, ulong);
> @@ -339,6 +343,22 @@ int do_bdinfo ( cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
>  #endif
>  	printf ("baudrate    = %d bps\n", bd->bi_baudrate);
>  
> +#if defined(CONFIG_SPEAR600) || defined(CONFIG_SPEAR3XX)
> +	if (bd->dramfreq == -1)
> +		printf("DDR Freq    = Not Known\n");
> +	else
> +		printf("DDR Freq    = %d\n", bd->dramfreq);
> +
> +	if (bd->dramtype == DDRMOBILE)
> +		printf("DDR Type    = MOBILE\n");
> +	else if (bd->dramtype == DDR2)
> +		printf("DDR Type    = DDR2\n");
> +	else
> +		printf("DDR Type    = Not Known\n");
> +
> +	printf("Xloader Rev = %s\n", bd->version);
> +#endif
> +
>  	return 0;
>  }

I don't like such board-specific code in this common file.

> diff --git a/include/configs/spear600.h b/include/configs/spear600.h
> new file mode 100755
> index 0000000..755114d
...
> +/* #define CONFIG_ETHADDR                 	00:11:22:33:44:55 */
> +
> +/* #define CONFIG_NETMASK                 	255.255.255.0 */
> +/* #define CONFIG_IPADDR                  	192.168.1.10 */
> +/* #define CONFIG_SERVERIP                	192.168.1.1 */
> +/* #define CONFIG_GATEWAYIP               	192.168.1.1 */

Please remove.


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 at denx.de
"A witty saying proves nothing."                           - Voltaire