[U-Boot] [PATCH 1/3] Initial mflash support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:09 Tue 17 Feb     , unsik Kim wrote:
> Hello?
>
> Thanks for comments.
> Some patches will be posted for your requests.
> Also I wrote my opinion for some comments.
>
>>> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
>>> index 59388d9..eccefc1 100644
>>> --- a/drivers/block/Makefile
>>> +++ b/drivers/block/Makefile
>>> @@ -25,13 +25,14 @@ include $(TOPDIR)/config.mk
>>>   LIB	:= $(obj)libblock.a
>>>  -COBJS-$(CONFIG_SCSI_AHCI) += ahci.o
>> why?
> This list badly sorted before I add. I just resort it to
> alphabetical order and add new option. Nothing removed.
>
>>>  COBJS-$(CONFIG_ATA_PIIX) += ata_piix.o
>>> +COBJS-$(CONFIG_CMD_MG_DISK) += mg_disk.o
>>>  COBJS-$(CONFIG_FSL_SATA) += fsl_sata.o
>>> +COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>>>  COBJS-$(CONFIG_LIBATA) += libata.o
>>>  COBJS-$(CONFIG_PATA_BFIN) += pata_bfin.o
>>>  COBJS-$(CONFIG_SATA_SIL3114) += sata_sil3114.o
>>> -COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>>> +COBJS-$(CONFIG_SCSI_AHCI) += ahci.o
>>>  COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>>>  COBJS-$(CONFIG_SYSTEMACE) += systemace.o
>>>  diff --git a/drivers/block/mg_disk.c b/drivers/block/mg_disk.c
>>> new file mode 100644
>>> index 0000000..4454fca
>>> --- /dev/null
>>> +++ b/drivers/block/mg_disk.c
>>> +
>>> +#define MG_BASE	(host.drv_data->base)
>> an inline function will be better
>  This isn't a function.
yes but please use an inline function which will allow better compiler check
and error message
as done in a lots of the linux drivers as example the netdev with this
static inline void *netdev_priv(const struct net_device *dev)
{
	return dev->priv;
}
>>> +
>
>>> +
>>> +/*
>>> + * copy src to dest, skipping leading and trailing blanks and null
>>> + * terminate the string
>>> + * "len" is the size of available memory including the terminating '\0'
>>> + */
>>> +static void mg_ident_cpy (unsigned char *dst, unsigned char *src,
>>> +		unsigned int len)
>>> +{
>>> +	unsigned char *end, *last;
>>> +
>>> +	last = dst;
>>> +	end  = src + len - 1;
>>> +
>>> +	/* reserve space for '\0' */
>>> +	if (len < 2)
>>> +		goto OUT;
>>> +
>>> +	/* skip leading white space */
>>> +	while ((*src) && (src<end) && (*src==' '))
>> please add a space before and after '<' '==' & co
>>> +		++src;
>>> +
>>> +	/* copy string, omitting trailing white space */
>>> +	while ((*src) && (src<end)) {
>>> +		*dst++ = *src;
>>> +		if (*src++ != ' ')
>>> +			last = dst;
>>> +	}
>>> +OUT:
>>> +	*last = '\0';
>>> +}
>> why do you need this?
> for parsing string parts of ATAID
it's mflash specific??
>
>>> +static unsigned int mg_do_read_sects(void *buff, u32 sect_num, u32 sect_cnt)
>>> +{
>>> +	u32 i, j, err;
>>> +	u8 *buff_ptr = buff;
>>> +
>>> +	err = mg_out(sect_num, sect_cnt, MG_CMD_RD);
>>> +	if (err)
>>> +		return err;
>>> +
>>> +	for (i = 0; i < sect_cnt; i++) {
>>> +		err = mg_wait(MG_REG_STATUS_BIT_DATA_REQ, 3000);
>>> +		if (err)
>>> +			return err;
>>> +
>>> +		/* TODO : u16 unaligned case */
>>> +		for(j = 0; j < MG_SECTOR_SIZE >> 1; j++) {
>>> +			*(u16 *)buff_ptr =
>>> +				readw(MG_BASE + MG_BUFF_OFFSET + (j << 1));
>> ??
> I can't guess your intention. Please write more clearly
you store a u16 in a u8 :(
>>> +			buff_ptr += 2;
>>> +		}
>>> +
>>> +		writeb(MG_CMD_RD_CONF, MG_BASE + MG_REG_COMMAND);
>>> +
>>> +		MG_DBG("%u (0x%8.8x) sector read", sect_num + i,
>>> +			(sect_num + i) * MG_SECTOR_SIZE);
>>> +	}
>>> +
>>> +	return err;
>>> +}
>
>>> +unsigned int mg_disk_read_sects(void *buff, u32 sect_num, u32 sect_cnt)
>>> +{
>>> +	u32 quotient, residue, i, err;
>>> +	u8 *buff_ptr = buff;
>>> +
>>> +	quotient = sect_cnt >> 8;
>>> +	residue = sect_cnt % 256;
>>> +
>>> +	for (i = 0; i < quotient; i++) {
>>> +		MG_DBG("sect num : %u buff : 0x%8.8x", sect_num, (u32)buff_ptr);
>>> +		err = mg_do_read_sects(buff_ptr, sect_num, 256);
>>> +		if (err)
>>> +			return err;
>>> +		sect_num += 256;
>>> +		buff_ptr += 256 * MG_SECTOR_SIZE;
>>> +	}
>>> +
>>> +	if (residue) {
>>> +		MG_DBG("sect num : %u buff : %8.8x", sect_num, (u32)buff_ptr);
>> please use debug(x)
> MG_DBG prints lots of messages. I think changing MG_DBG to debug might
> confuse other driver's debug message.
> If u-boot policy only allow debug(x), I'll follow.
>>> +		err = mg_do_read_sects(buff_ptr, sect_num, residue);
>>> +	}
>>> +
>>> +	return err;
>>> +}
>
>>> +/* must override this function */
>>> +struct mg_drv_data * __attribute__((weak)) mg_get_drv_data (void)
>>> +{
>>> +	puts ("### WARNING ### port mg_get_drv_data function\n");
>>> +	return NULL;
>>> +}
>> please do an compile error not a run time error
> IMHO, compile error (or warning) is not a good choice for this case.
sorry no beacause everyone does not have the board to test it, a compile error
will at least help use to check if we brake the board or not
> Compile time error always generate error even though override function
> exist.
not necessarely
does you driver allow multiple instance of mflash?
if no you does not implement the function

Best Regards,
J.