Dear Kim,
In message 57afda040901052341g3b00f741r445c0ce8d33b7b71@mail.gmail.com you wrote:
I wrote mflash IO mode block device driver for U-Boot.
Thanks for your contribution. Here a few comments:
diff --git a/common/Makefile b/common/Makefile index 93e3963..f93e575 100644 --- a/common/Makefile +++ b/common/Makefile @@ -57,6 +57,7 @@ COBJS-$(CONFIG_ENV_IS_IN_NAND) += env_nand.o COBJS-$(CONFIG_ENV_IS_IN_NVRAM) += env_nvram.o COBJS-$(CONFIG_ENV_IS_IN_ONENAND) += env_onenand.o COBJS-$(CONFIG_ENV_IS_IN_SPI_FLASH) += env_sf.o +COBJS-$(CONFIG_ENV_IS_IN_MG_DISK) += env_mgdisk.o COBJS-$(CONFIG_ENV_IS_NOWHERE) += env_nowhere.o
# command @@ -138,6 +139,7 @@ endif COBJS-$(CONFIG_CMD_XIMG) += cmd_ximg.o COBJS-$(CONFIG_YAFFS2) += cmd_yaffs2.o COBJS-$(CONFIG_VFD) += cmd_vfd.o +COBJS-$(CONFIG_CMD_MG_DISK) += cmd_mgdisk.o
Please keep such lists sorted.
...
+U_BOOT_CMD(
- mgd, 5, 0, do_mg_disk_cmd,
- "mgd - mgine m[g]flash command\n",
- ": mgine mflash IO mode (disk) command\n"
- "\t- initialize : mgd init\n"
- "\t- random read : mgd read [from] [to] [size]\n"
- "\t\tbelow example read 256 bytes from 0x300000 of mflash\n"
- "\t\tto 0xA0100000 of host memory\n"
- "\t\tex) mgd read 0x300000 0xA0100000 256\n"
- "\t- random write : mgd write [from] [to] [size]\n"
- "\t\tex) mgd write 0xA0100000 0x300000 256\n"
- "\t- sector read : mgd readsec [sector] [to] [counts]\n"
- "\t\tbelow example read 10 sectors starts from 400 sector\n"
- "\t\tto 0xA0100000\n"
- "\t\tex) mgd readsec 400 0xA0100000 10\n"
- "\t- sector write : mgd writesec [from] [sector] [counts]\n"
+);
Please avoid using TAB characters here.
Also,pleas ebe terse - don;t give examples here. Rather add a README.mflash file to the doc/ directory.
diff --git a/common/cmd_nvedit.c b/common/cmd_nvedit.c index 85025da..4945aeb 100644 --- a/common/cmd_nvedit.c +++ b/common/cmd_nvedit.c @@ -59,8 +59,9 @@ DECLARE_GLOBAL_DATA_PTR; !defined(CONFIG_ENV_IS_IN_NAND) && \ !defined(CONFIG_ENV_IS_IN_ONENAND) && \ !defined(CONFIG_ENV_IS_IN_SPI_FLASH) && \
- !defined(CONFIG_ENV_IS_IN_MG_DISK) && \ !defined(CONFIG_ENV_IS_NOWHERE)
Please keep lists sorted.
-# error Define one of CONFIG_ENV_IS_IN_{NVRAM|EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|NOWHERE} +# error Define one of CONFIG_ENV_IS_IN_{NVRAM|EEPROM|FLASH|DATAFLASH|ONENAND|SPI_FLASH|MG_DISK|NOWHERE}
Please keep lists sorted.
Also note that your mailer wrapped long lines here, thus corrupting the patch. Please fix your mailer.
And please avoid such long lines, too!
...
+void env_relocate_spec(void) +{
- unsigned int err;
- err = mg_disk_read(CONFIG_ENV_ADDR, (u_char *) env_ptr, CONFIG_ENV_SIZE);
- if (err) {
puts ("*** Warning - mg_disk_read error, using default environment\n\n");
Line too long.
set_default_env();return;- }
- if (crc32(0, env_ptr->data, ENV_SIZE) != env_ptr->crc) {
puts ("*** Warning - CRC error, using default environment\n\n");set_default_env();- }
+}
+int saveenv(void) +{
- unsigned int err;
- env_ptr->crc = crc32(0, env_ptr->data, ENV_SIZE);
- err = mg_disk_write(CONFIG_ENV_ADDR, (u_char *) env_ptr, CONFIG_ENV_SIZE);
Line too long.
- if (err)
puts ("*** Warning - mg_disk_write error\n\n");- return (int)err;
The cast should not be needed here.
diff --git a/disk/part.c b/disk/part.c index e353cee..c007060 100644 --- a/disk/part.c +++ b/disk/part.c @@ -39,7 +39,8 @@ defined(CONFIG_CMD_SCSI) || \ defined(CONFIG_CMD_USB) || \ defined(CONFIG_MMC) || \
defined(CONFIG_SYSTEMACE) )
defined(CONFIG_SYSTEMACE) || \defined(CONFIG_CMD_MG_DISK))
Please keep lists sorted.
@@ -95,7 +99,8 @@ block_dev_desc_t *get_dev(char* ifname, int dev) defined(CONFIG_CMD_SCSI) || \ defined(CONFIG_CMD_USB) || \ defined(CONFIG_MMC) || \
defined(CONFIG_SYSTEMACE) )
defined(CONFIG_SYSTEMACE) || \defined(CONFIG_CMD_MG_DISK))
Ditto.
@@ -207,7 +212,8 @@ void dev_print (block_dev_desc_t *dev_desc) defined(CONFIG_CMD_SCSI) || \ defined(CONFIG_CMD_USB) || \ defined(CONFIG_MMC) || \
defined(CONFIG_SYSTEMACE) )
defined(CONFIG_SYSTEMACE) || \defined(CONFIG_CMD_MG_DISK))
Ditto.
diff --git a/disk/part_amiga.c b/disk/part_amiga.c index 6c3d748..b4c2820 100644 --- a/disk/part_amiga.c +++ b/disk/part_amiga.c @@ -30,7 +30,8 @@ defined(CONFIG_CMD_SCSI) || \ defined(CONFIG_CMD_USB) || \ defined(CONFIG_MMC) || \
defined(CONFIG_SYSTEMACE) ) && defined(CONFIG_AMIGA_PARTITION)
defined(CONFIG_SYSTEMACE) || \defined(CONFIG_CMD_MG_DISK)) && defined(CONFIG_AMIGA_PARTITION)
Ditto.
@@ -40,6 +41,8 @@ #define PRINTF(fmt, args...) #endif
+#define atoi(x) simple_strtoul(x,NULL,10)
Please avoid that.
struct block_header { u32 id; diff --git a/disk/part_dos.c b/disk/part_dos.c index 4d778ec..30dc39f 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -40,7 +40,8 @@ defined(CONFIG_CMD_SCSI) || \ defined(CONFIG_CMD_USB) || \ defined(CONFIG_MMC) || \
defined(CONFIG_SYSTEMACE) ) && defined(CONFIG_DOS_PARTITION)
defined(CONFIG_SYSTEMACE) || \defined(CONFIG_CMD_MG_DISK)) && defined(CONFIG_DOS_PARTITION)
more incorrect sort order, even more following below.
diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c new file mode 100644 index 0000000..06c9bd8 --- /dev/null +++ b/drivers/block/mg_disk.c @@ -0,0 +1,595 @@ +/*
- (C) Copyright 2009 mGine co.
- unsik Kim donari75@gmail.com
- See file CREDITS for list of people who contributed to this
- project.
- This program is free software; you can redistribute it and/or
- modify it under the terms of the GNU General Public License as
- published by the Free Software Foundation; either version 2 of
- the License, or (at your option) any later version.
- This program is distributed in the hope that it will be useful,
- but WITHOUT ANY WARRANTY; without even the implied warranty of
- MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- GNU General Public License for more details.
- You should have received a copy of the GNU General Public License
- along with this program; if not, write to the Free Software
- Foundation, Inc., 59 Temple Place, Suite 330, Boston,
- MA 02111-1307 USA
- */
+#include <common.h> +#include <malloc.h> +#include <part.h> +#include <ata.h> +#include "mg_disk_prv.h"
+#ifdef CONFIG_CMD_MG_DISK
+#undef printk +#define printk printf
Please don't.
+#undef KERN_ERR +#define KERN_ERR +#undef KERN_DEBUG +#define KERN_DEBUG +#undef KERN_INFO +#define KERN_INFO
Please don't.
+#undef inb +#undef inw +#undef outb +#undef outw
+#define inb(a) (*(volatile unsigned char *)(a)) +#define inw(a) (*(volatile unsigned short *)(a)) +#define outb(v, a) (*(volatile unsigned char *)(a) = (v)) +#define outw(v, a) (*(volatile unsigned short *)(a) = (v))
A strict NO, NO here. Please never do that.
Please do use theproper accessor functions that are needed for the respective architecture. PLain volatile pointer accesses are bound to fail. Never do that.
...
- printk("%s: %s: status=0x%02x { ", name, msg, stat & 0xff);
- if (stat & MG_REG_STATUS_BIT_BUSY) printk("Busy ");
- if (stat & MG_REG_STATUS_BIT_READY) printk("DriveReady ");
- if (stat & MG_REG_STATUS_BIT_WRITE_FAULT) printk("WriteFault ");
- if (stat & MG_REG_STATUS_BIT_SEEK_DONE) printk("SeekComplete ");
- if (stat & MG_REG_STATUS_BIT_DATA_REQ) printk("DataRequest ");
- if (stat & MG_REG_STATUS_BIT_CORRECTED_ERROR) printk("CorrectedError ");
- if (stat & MG_REG_STATUS_BIT_ERROR) printk("Error ");
Bad coding style. Please reformat.
- printk("}\n");
- if ((stat & MG_REG_STATUS_BIT_ERROR)) {
printk("%s: %s: error=0x%02x { ", name, msg, err & 0xff);if (err & MG_REG_ERR_BBK) printk("BadSector ");if (err & MG_REG_ERR_UNC) printk("UncorrectableError ");if (err & MG_REG_ERR_IDNF) printk("SectorIdNotFound ");if (err & MG_REG_ERR_ABRT) printk("DriveStatusError ");if (err & MG_REG_ERR_AMNF) printk("AddrMarkNotFound ");
Ditto.
+static unsigned int msecs_to_hz (u32 msec) +{
- u32 hz = CONFIG_SYS_HZ / 1000 * msec;
- if (!hz)
hz = 1;- return hz;
This makes no sense. CONFIG_SYS_HZ is always 1000. Please consider it a constant.
...
- from = get_timer(0);
- expire = from + msecs_to_hz(msec);
I don;t understand y ou logic here. get_timer() is defined to operate in munits of millisconds. What's the msecs_to_hz() stuff gotta do here?
- } while (cur < expire);
You are aware of the overflow issues buried here, are you?
if (!err) {if((iop->field_valid & 1) == 0) {err = MG_ERR_TRANSLATION;} else {mg_ident_cpy((unsigned char*)mg_disk_dev.revision, iop->fw_rev,sizeof(mg_disk_dev.revision));
mg_ident_cpy((unsigned char*)mg_disk_dev.vendor, iop->model,sizeof(mg_disk_dev.vendor));
mg_ident_cpy((unsigned char*)mg_disk_dev.product, iop->serial_no,sizeof(mg_disk_dev.product));
Lines way too long, and wrapped by mailer.
+static int mg_disk_reset (void) +{
- struct mg_drv_data *prv_data = MG_HOST->drv_data;
- s32 err;
- u8 init_status;
- /* hdd rst low */
- prv_data->mg_hdrst_pin(0);
- err = mg_wait(MG_REG_STATUS_BIT_BUSY, 300);
- if(err) return err;
Please refoemat according to CodingStyle requirements.
- if ((err = mg_wait(MG_STAT_READY, 3000))) {
return err;- }
No braces for one-liners, please.
+static unsigned int mg_do_read_sects(void *buff, u32 sect_num, u32 sect_cnt) +{
- u32 i, j, err;
- u8 *buff_ptr = buff;
- if ((err = mg_out(sect_num, sect_cnt, MG_CMD_RD))) {
return err;- }
Ditto.
- for (i = 0; i < sect_cnt; i++) {
if ((err = mg_wait(MG_REG_STATUS_BIT_DATA_REQ, 3000))) {return err;}
Ditto. And so on.
MG_DBG("%u (0x%8.8x) sector read", sect_num + i, (sect_num + i) *MG_SECTOR_SIZE);
Please check all line lengths!
diff --git a/lib_arm/board.c b/lib_arm/board.c index 2358beb..6a26bd2 100644 --- a/lib_arm/board.c +++ b/lib_arm/board.c @@ -48,6 +48,7 @@ #include <serial.h> #include <nand.h> #include <onenand_uboot.h> +#include <mg_disk.h>
#ifdef CONFIG_DRIVER_SMC91111 #include "../drivers/net/smc91111.h" @@ -348,6 +349,10 @@ void start_armboot (void) onenand_init(); #endif
+#if defined(CONFIG_CMD_MG_DISK)
- mg_disk_init();
+#endif
Please don't.
First it's wrong to add it for ARM only - what about other architectures that want to use that technology?
Second it's wrong to always call the init code. This shall be done only upon first access. See the longish discussion we had for example about how to handle the S-ATA init code - please see the archives.
Best regards,
Wolfgang Denk