[U-Boot] [PATCH V5 2/4] ide: add mvsata_ide driver

[Albert ARIBAUD <albert.aribaud@free.fr>]

Hi Prafulla et al.,

Le 21/07/2010 12:16, Prafulla Wadaskar a ?crit :
>
>
>> -----Original Message-----
>> From: u-boot-bounces at lists.denx.de
>> [mailto:u-boot-bounces at lists.denx.de] On Behalf Of Albert Aribaud
>> Sent: Tuesday, July 13, 2010 5:33 PM
>> To: u-boot at lists.denx.de
>> Subject: [U-Boot] [PATCH V5 2/4] ide: add mvsata_ide driver
>>
>> This driver only provides initialization code; actual driving
>> is done by cmd_ide.c using the ATA compatibility mode of the
>> Marvell SATAHC controller.
>>
>> Signed-off-by: Albert Aribaud<albert.aribaud@free.fr>
>> ---
>>   drivers/block/Makefile     |    7 +++--
>>   drivers/block/mvsata_ide.c |   60
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   include/mvsata_ide.h       |   55
>> ++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 119 insertions(+), 3 deletions(-)
>>   create mode 100644 drivers/block/mvsata_ide.c
>>   create mode 100644 include/mvsata_ide.h
>>
>> diff --git a/drivers/block/Makefile b/drivers/block/Makefile
>> index 3f6ad5c..64dcf4e 100644
>> --- a/drivers/block/Makefile
>> +++ b/drivers/block/Makefile
>> @@ -25,15 +25,16 @@ include $(TOPDIR)/config.mk
>>
>>   LIB	:= $(obj)libblock.a
>>
>> +COBJS-$(CONFIG_SCSI_AHCI) += ahci.o
>>   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_CMD_MG_DISK) += mg_disk.o
>> +COBJS-$(CONFIG_MVSATA_IDE) += mvsata_ide.o
>>   COBJS-$(CONFIG_PATA_BFIN) += pata_bfin.o
>>   COBJS-$(CONFIG_SATA_DWC) += sata_dwc.o
>>   COBJS-$(CONFIG_SATA_SIL3114) += sata_sil3114.o
>> -COBJS-$(CONFIG_SCSI_AHCI) += ahci.o
>> +COBJS-$(CONFIG_IDE_SIL680) += sil680.o
>>   COBJS-$(CONFIG_SCSI_SYM53C8XX) += sym53c8xx.o
>>   COBJS-$(CONFIG_SYSTEMACE) += systemace.o
>>
>> diff --git a/drivers/block/mvsata_ide.c b/drivers/block/mvsata_ide.c
>> new file mode 100644
>> index 0000000..f538839
>> --- /dev/null
>> +++ b/drivers/block/mvsata_ide.c
>> @@ -0,0 +1,60 @@
>> +/*
>> + * Copyright (C) 2010 Albert ARIBAUD<albert.aribaud@free.fr>
>> + *
>> + * Written-by: Albert ARIBAUD<albert.aribaud@free.fr>
>> + *
>> + * 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., 51 Franklin Street, Fifth Floor, Boston,
>> + * MA 02110-1301 USA
>> + */
>> +
>> +#include<common.h>
>> +#include<asm/io.h>
>> +#include "mvsata_ide.h"
>> +
>> +/* Mask and values for device DETection and link initialization */
>> +#define MVSATA_SCONTROL_DET_MASK		0x0000000F
>> +#define MVSATA_SCONTROL_DET_NONE		0x00000000
>> +#define MVSATA_SCONTROL_DET_INIT		0x00000001
>> +
>> +/* Mask and values for device Interface Power Management */
>> +#define MVSATA_SCONTROL_IPM_MASK		0x00000F00
>> +#define MVSATA_SCONTROL_IPM_NO_LP_ALLOWED	0x00000300
>> +
>> +#define MVSATA_SCONTROL_MASK \
>> +	(MVSATA_SCONTROL_DET_MASK|MVSATA_SCONTROL_IPM_MASK)
>> +
>> +#define MVSATA_PORT_INIT \
>> +	(MVSATA_SCONTROL_DET_INIT|MVSATA_SCONTROL_IPM_NO_LP_ALLOWED)
>> +
>> +#define MVSATA_PORT_USE \
>> +	(MVSATA_SCONTROL_DET_NONE|MVSATA_SCONTROL_IPM_NO_LP_ALLOWED)
>> +
>
> Can you move above macros to header file?

They are used / useful only in this .c file, and defined only to avoid 
magic numbers. What would be the point in moving them to a header file?

>> +void mvsata_ide_initialize_port(struct mvsata_port_registers *port)
>> +{
>> +	u32 reg;
>> +
>> +	reg = readl(&port->SControl);
>> +
>> +	reg = (reg&  ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_INIT;
>> +
>> +	writel(reg,&port->SControl);
>> +
>> +	reg = (reg&  ~MVSATA_SCONTROL_MASK) | MVSATA_PORT_USE;
>> +
>> +	writel(reg,&port->SControl);
>> +}
>> diff --git a/include/mvsata_ide.h b/include/mvsata_ide.h
>> new file mode 100644
>> index 0000000..ad0f854
>> --- /dev/null
>> +++ b/include/mvsata_ide.h
>> @@ -0,0 +1,55 @@
>> +/*
>> + * Copyright (C) 2010 Albert ARIBAUD<albert.aribaud@free.fr>
>> + *
>> + * Written-by: Albert ARIBAUD<albert.aribaud@free.fr>
>> + *
>> + * 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., 51 Franklin Street, Fifth Floor, Boston,
>> + * MA 02110-1301 USA
>> + */
>> +
>> +#ifndef _MVSATA_IDE_H
>> +#define _MVSATA_IDE_H
>> +
>> +#ifndef __ASSEMBLY__
>> +
>> +/* SATA port registers */
>> +struct mvsata_port_registers
>> +{
>> +	u32 Reserved1[192];
>> +	/* offset 0x300 : ATA Interface registers */
>> +	u32 SStatus;
>> +	u32 SError;
>> +	u32 SControl;
>> +	u32 LTMode;
>> +	u32 PhyMode3;
>> +	u32 PhyMode4;
>> +	u32 Reserved2[5];
>> +	u32 PhyMode1;
>> +	u32 PhyMode2;
>> +	u32 BIST_CR;
>> +	u32 BIST_DW1;
>> +	u32 BIST_DW2;
>> +	u32 SErrorIntrMask;
>
> As a part fo standard coding practice, let avoide mix letters
> Can you please use all lowercase for all variables?

Ok.

> Regards..
> Prafulla . .

Amicalement,
-- 
Albert.