Re: [U-Boot] [PATCH][RESEND]: Fix for U-Boot build failure with CONFIG_SYS_NO_FLASH defined for qemu-mips.

2010-03-20 Thread Wolfgang Denk
Dear Himanshu Chauhan,

I apologize for the long delay (I could try and claim I was waiting
for anybody else to comment first, but it seems noodu feels addessed).

In message 4b2c7961.3070...@symmetricore.com you wrote:
 U-Boot hangs with qemu-system-mips with ##unknown flash error.
 Disabling flash using CONFIG_SYS_NO_FLASH breaks the build.
 This patch fixes the issue. Don't know if its okay.
 
 Signed-off-by: Himanshu Chauhan himan...@symmetricore.com
 
 diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
 index efd6aec..5bd3af0 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -76,7 +76,7 @@ extern void bz_internal_error(int);
  static int image_info (unsigned long addr);
  #endif
 
 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)

I don't like this approach - CONFIG_CMD_IMLS and CONFIG_SYS_NO_FLASH
should not be combined like that. Assume somebody wants to extend the
functionality of the imls command to check for images in NAND flash
(or other similar storage devices) as well.

Disabling NOR flash on a system should be just a matter of system
configuration.  If you have no NOR flash on your board (so you select
CONFIG_SYS_NO_FLASH), then why do you select CONFIG_CMD_IMLS which
obviously attempts to access NOR flash?

 diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
 index 24eb33f..06c7271 100644
 --- a/drivers/mtd/cfi_flash.c
 +++ b/drivers/mtd/cfi_flash.c
 @@ -41,6 +41,7 @@
  #include environment.h
  #include mtd/cfi_flash.h
 
 +#ifndef CONFIG_SYS_NO_FLASH

Same here. Why do you enable the CFI driver at all when you don't have
NOR flash on your system?

 --- a/include/configs/qemu-mips.h
 +++ b/include/configs/qemu-mips.h
 :q
 @@ -142,6 +142,7 @@
  #define CONFIG_SYS_INIT_SP_OFFSET0x40
 
  /* We boot from this flash, selected with dip switch */
 +#define CONFIG_SYS_NO_FLASH
  #define CONFIG_SYS_FLASH_BASE0xbfc0
  #define CONFIG_SYS_MAX_FLASH_BANKS   1
  #define CONFIG_SYS_MAX_FLASH_SECT128
 @@ -149,7 +150,8 @@
  #define CONFIG_FLASH_CFI_DRIVER  1
  #define CONFIG_SYS_FLASH_USE_BUFFER_WRITE1
 
 -#define CONFIG_ENV_IS_IN_FLASH   1
 +#define CONFIG_ENV_IS_IN_FLASH   0
 +#define CONFIG_ENV_IS_NOWHERE
  #define CONFIG_ENV_ADDR  (CONFIG_SYS_FLASH_BASE + 
 CONFIG_SYS_MONITOR_LEN)

Here you should disable NOR related features - like the CFI driver or
the imls command.

Sorry, but I reject this patch.

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: w...@denx.de
PROGRAM - n.  A magic spell cast over a computer  allowing it to turn
one's input into error messages.
v. tr. - To engage in a pastime similar to banging one's head against
a wall, but with fewer opportunities for reward.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH][RESEND]: Fix for U-Boot build failure with CONFIG_SYS_NO_FLASH defined for qemu-mips.

2009-12-20 Thread Shinya Kuribayashi
Himanshu Chauhan wrote:
 May be I really want that CONFIG_SYS_NO_FLASH defined then build should at 
 least not break. Isn't it?

Correct, it should build even with CONFIG_SYS_NO_FLASH.

 diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
 index efd6aec..5bd3af0 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -76,7 +76,7 @@ extern void bz_internal_error(int);
   static int image_info (unsigned long addr);
   #endif

 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   #includeflash.h
   extern flash_info_t flash_info[]; /* info for FLASH chips */
   static int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char
 *argv[]);
 @@ -1153,7 +1153,7 @@ U_BOOT_CMD(
   /***/
   /* imls - list all images found in flash */
   /***/
 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
   {
   flash_info_t *info;
 Disabling IMLS command in configs/qemu_mips.h?
 This should have been disabled anyways if this patch was applied:
 http://www.mail-archive.com/u-boot@lists.denx.de/msg07407.html

The patch is applied.

|diff --git a/include/configs/qemu-mips.h b/include/configs/qemu-mips.h
|index cbacdf9..49a1a1c 100644
|--- a/include/configs/qemu-mips.h
|+++ b/include/configs/qemu-mips.h
|:q
|@@ -142,6 +142,7 @@
| #define CONFIG_SYS_INIT_SP_OFFSET 0x40
|
| /* We boot from this flash, selected with dip switch */
|+#define CONFIG_SYS_NO_FLASH
| #define CONFIG_SYS_FLASH_BASE 0xbfc0
| #define CONFIG_SYS_MAX_FLASH_BANKS1
| #define CONFIG_SYS_MAX_FLASH_SECT 128

FWIW, how about putting CONFIG_SYS_NO_FLASH prior to config_cmd_
default.h?

 diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
 index 24eb33f..06c7271 100644
 --- a/drivers/mtd/cfi_flash.c
 +++ b/drivers/mtd/cfi_flash.c
 @@ -41,6 +41,7 @@
   #includeenvironment.h
   #includemtd/cfi_flash.h

 +#ifndef CONFIG_SYS_NO_FLASH
   /*
* This file implements a Common Flash Interface (CFI) driver for
* U-Boot.
 @@ -2020,3 +2021,5 @@ unsigned long flash_init (void)

   return (size);
   }
 +
 +#endif
 Removing CONFIG_CFI_DRIVER from configs/qemu_mips.h?
 If CONFIG_SYS_NO_FLASH is defined should it be compiled?

No.  In U-Boot, in general, drivers should be predefined through
configs/boardname.h.  If we don't use CFI driver, just disable
it in top-level board config file.  This is to avoid #ifdef mess,
and prevent the drivers from being clobbered.

-- 
Shinya Kuribayashi
NEC Electronics
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH][RESEND]: Fix for U-Boot build failure with CONFIG_SYS_NO_FLASH defined for qemu-mips.

2009-12-19 Thread Shinya Kuribayashi
Hi,

On 12/19/09 3:57 PM, Himanshu Chauhan wrote:
 U-Boot hangs with qemu-system-mips with ##unknown flash error.

Do you have any idea what's the root cause of that unknown flash
error?  Is this U-Boot CFI driver issue, or Qemu-side problem?

Using CONFIG_SYS_NO_FLASH is a quick, enough workaround for your
trial, but does not fix anything.  Could you sort out the issue?
I don't think I can help regarding debugging Qemu (sorry!), but
u-boot/doc/README.qemu_mips and U-Boot/Qemu community will help.

 Disabling flash using CONFIG_SYS_NO_FLASH breaks the build.
 This patch fixes the issue. Don't know if its okay.

 Signed-off-by: Himanshu Chauhanhiman...@symmetricore.com

Heh, let's use git-format-patch when preparing patches.

$ git format-patch HEAD^..
$ git format-patch --no-thread HEAD^^..
$ mkdir foo  git format-patch -o foo/ HEAD~3..
and so on.

 diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
 index efd6aec..5bd3af0 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -76,7 +76,7 @@ extern void bz_internal_error(int);
   static int image_info (unsigned long addr);
   #endif

 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   #includeflash.h
   extern flash_info_t flash_info[]; /* info for FLASH chips */
   static int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[]);
 @@ -1153,7 +1153,7 @@ U_BOOT_CMD(
   /***/
   /* imls - list all images found in flash */
   /***/
 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
   {
   flash_info_t *info;

Disabling IMLS command in configs/qemu_mips.h?

 diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
 index 24eb33f..06c7271 100644
 --- a/drivers/mtd/cfi_flash.c
 +++ b/drivers/mtd/cfi_flash.c
 @@ -41,6 +41,7 @@
   #includeenvironment.h
   #includemtd/cfi_flash.h

 +#ifndef CONFIG_SYS_NO_FLASH
   /*
* This file implements a Common Flash Interface (CFI) driver for
* U-Boot.
 @@ -2020,3 +2021,5 @@ unsigned long flash_init (void)

   return (size);
   }
 +
 +#endif

Removing CONFIG_CFI_DRIVER from configs/qemu_mips.h?

 diff --git a/include/configs/qemu-mips.h b/include/configs/qemu-mips.h
 index cbacdf9..49a1a1c 100644
 --- a/include/configs/qemu-mips.h
 +++ b/include/configs/qemu-mips.h
 :q
 @@ -142,6 +142,7 @@
   #define CONFIG_SYS_INIT_SP_OFFSET   0x40

   /* We boot from this flash, selected with dip switch */
 +#define CONFIG_SYS_NO_FLASH
   #define CONFIG_SYS_FLASH_BASE   0xbfc0
   #define CONFIG_SYS_MAX_FLASH_BANKS  1
   #define CONFIG_SYS_MAX_FLASH_SECT   128
 @@ -149,7 +150,8 @@
   #define CONFIG_FLASH_CFI_DRIVER 1
   #define CONFIG_SYS_FLASH_USE_BUFFER_WRITE   1

 -#define CONFIG_ENV_IS_IN_FLASH   1
 +#define CONFIG_ENV_IS_IN_FLASH   0
 +#define CONFIG_ENV_IS_NOWHERE
   #define CONFIG_ENV_ADDR (CONFIG_SYS_FLASH_BASE + 
 CONFIG_SYS_MONITOR_LEN)

   /* Address and size of Primary Environment Sector */

[...]

 diff --git a/include/mtd/cfi_flash.h b/include/mtd/cfi_flash.h
 index 2aa6911..2229ddf 100644
 --- a/include/mtd/cfi_flash.h
 +++ b/include/mtd/cfi_flash.h
 @@ -151,7 +151,8 @@ struct cfi_pri_hdr {
   u8  minor_version;
   } __attribute__((packed));

 -void flash_write_cmd(flash_info_t * info, flash_sect_t sect,
 -  uint offset, u32 cmd);
 +#ifndef CONFIG_SYS_NO_FLASH
 +void flash_write_cmd(flash_info_t * info, flash_sect_t sect, uint offset, 
 u32 cmd);
 +#endif

   #endif /* __CFI_FLASH_H__ */

Removing CONFIG_CFI_DRIVER from configs/qemu_mips.h?

Anyway, I'd like to leave this issue as-is for now, and look
forward to the read bug fix.

   Shinya
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot


Re: [U-Boot] [PATCH][RESEND]: Fix for U-Boot build failure with CONFIG_SYS_NO_FLASH defined for qemu-mips.

2009-12-19 Thread Himanshu Chauhan
Shinya Kuribayashi wrote:
 Hi,
 
 On 12/19/09 3:57 PM, Himanshu Chauhan wrote:
 U-Boot hangs with qemu-system-mips with ##unknown flash error.
 
 Do you have any idea what's the root cause of that unknown flash
 error?  Is this U-Boot CFI driver issue, or Qemu-side problem?
 
 Using CONFIG_SYS_NO_FLASH is a quick, enough workaround for your
 trial, but does not fix anything.  Could you sort out the issue?
 I don't think I can help regarding debugging Qemu (sorry!), but
 u-boot/doc/README.qemu_mips and U-Boot/Qemu community will help.

May be I really want that CONFIG_SYS_NO_FLASH defined then build should at 
least not break. Isn't it?

 
 Disabling flash using CONFIG_SYS_NO_FLASH breaks the build.
 This patch fixes the issue. Don't know if its okay.

 Signed-off-by: Himanshu Chauhanhiman...@symmetricore.com
 
 Heh, let's use git-format-patch when preparing patches.
 
 $ git format-patch HEAD^..
 $ git format-patch --no-thread HEAD^^..
 $ mkdir foo  git format-patch -o foo/ HEAD~3..
 and so on.
 
Thanks, I will take care of that next time.

 diff --git a/common/cmd_bootm.c b/common/cmd_bootm.c
 index efd6aec..5bd3af0 100644
 --- a/common/cmd_bootm.c
 +++ b/common/cmd_bootm.c
 @@ -76,7 +76,7 @@ extern void bz_internal_error(int);
   static int image_info (unsigned long addr);
   #endif

 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   #includeflash.h
   extern flash_info_t flash_info[]; /* info for FLASH chips */
   static int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char
 *argv[]);
 @@ -1153,7 +1153,7 @@ U_BOOT_CMD(
   /***/
   /* imls - list all images found in flash */
   /***/
 -#if defined(CONFIG_CMD_IMLS)
 +#if !defined(CONFIG_SYS_NO_FLASH)  defined(CONFIG_CMD_IMLS)
   int do_imls (cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
   {
   flash_info_t *info;
 
 Disabling IMLS command in configs/qemu_mips.h?
This should have been disabled anyways if this patch was applied:
http://www.mail-archive.com/u-boot@lists.denx.de/msg07407.html

 
 diff --git a/drivers/mtd/cfi_flash.c b/drivers/mtd/cfi_flash.c
 index 24eb33f..06c7271 100644
 --- a/drivers/mtd/cfi_flash.c
 +++ b/drivers/mtd/cfi_flash.c
 @@ -41,6 +41,7 @@
   #includeenvironment.h
   #includemtd/cfi_flash.h

 +#ifndef CONFIG_SYS_NO_FLASH
   /*
* This file implements a Common Flash Interface (CFI) driver for
* U-Boot.
 @@ -2020,3 +2021,5 @@ unsigned long flash_init (void)

   return (size);
   }
 +
 +#endif
 
 Removing CONFIG_CFI_DRIVER from configs/qemu_mips.h?
If CONFIG_SYS_NO_FLASH is defined should it be compiled?

Regards
Himanshu
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot