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

Vipin Kumar
2009-12-19T08:56:19Z

Dear Wolfgang,

> In message <1260955110-5656-6-git-send-email-vipin.kumar@st.com> you wrote:
>>
>> Signed-off-by: Vipin <vipin.kumar@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.
>
> ...

Source modified to
if (ethaddr from environment fails)
read mac from i2c memory
This should be ok. Right

>> +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).
>

Code modified to use chip_config. this command is implemented in board
specific file. I guess that should be ok since there is no standard interface
for the command

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

Yes, all values below 333 are acceptable. This command is added for testing
purposes only so no further checking is required

>> + ? ? 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.

Corrected multiline comments globally

>
>> + ? ? 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.

Ok, modified to single line function call

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

Ok, modified to single line function call

>> 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.

Ok. But then bdinfo should ideally contain a generic callback
to be implemented by board specific code.

>> 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.

Ok.removed commented code

Regards
Vipin