[U-Boot] [PATCH V2 1/3] ide: add configuration CONFIG_IDE_SWAP_IO

[U-Boot] [PATCH V2 1/3] ide: add configuration CONFIG_IDE_SWAP_IO

[Albert Aribaud]

Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
This patch set introduces CONFIG_IDE_SWAP_IO as suggested by
Wolfgang Denk, adds orion5x support to cmd_ide.c and adds IDE
support to edminiv2.

Patchset history:

V1: Initial 4-patch set -- not cleanly submitted (does
    not appear on gmane for instance) and contained some
    unrelated changes.

V2: Slimmed down to 3 patches and removed unrelated
    changes.

 arch/powerpc/include/asm/config.h |    3 +++
 common/cmd_ide.c                  |   18 +++++++++---------
 doc/README.PXA_CF                 |    8 ++++++++
 include/configs/ap325rxa.h        |    1 +
 include/configs/ms7720se.h        |    1 +
 include/configs/r2dplus.h         |    1 +
 include/configs/r7780mp.h         |    1 +
 7 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/arch/powerpc/include/asm/config.h b/arch/powerpc/include/asm/config.h
index fc3facb..371989d 100644
--- a/arch/powerpc/include/asm/config.h
+++ b/arch/powerpc/include/asm/config.h
@@ -76,4 +76,7 @@
 /* Relocation to SDRAM works on all PPC boards */
 #define CONFIG_RELOC_FIXUP_WORKS
 
+/* All PPC boards must swap IDE bytes */
+#define CONFIG_IDE_SWAP_IO
+
 #endif /* _ASM_CONFIG_H_ */
diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index d486697..9292a5b 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -853,7 +853,7 @@ input_swap_data(int dev, ulong *sect_buf, int words)
 #endif	/* __LITTLE_ENDIAN || CONFIG_AU1X00 */
 
 
-#if defined(__PPC__) || defined(CONFIG_PXA_PCMCIA) || defined(CONFIG_SH)
+#if defined(CONFIG_IDE_SWAP_IO)
 static void
 output_data(int dev, ulong *sect_buf, int words)
 {
@@ -897,15 +897,15 @@ output_data(int dev, ulong *sect_buf, int words)
 	}
 #endif
 }
-#else	/* ! __PPC__ */
+#else	/* ! CONFIG_IDE_SWAP_IO */
 static void
 output_data(int dev, ulong *sect_buf, int words)
 {
 	outsw(ATA_CURR_BASE(dev)+ATA_DATA_REG, sect_buf, words<<1);
 }
-#endif	/* __PPC__ */
+#endif	/* CONFIG_IDE_SWAP_IO */
 
-#if defined(__PPC__) || defined(CONFIG_PXA_PCMCIA) || defined(CONFIG_SH)
+#if defined(CONFIG_IDE_SWAP_IO)
 static void
 input_data(int dev, ulong *sect_buf, int words)
 {
@@ -955,14 +955,14 @@ input_data(int dev, ulong *sect_buf, int words)
 	}
 #endif
 }
-#else	/* ! __PPC__ */
+#else	/* ! CONFIG_IDE_SWAP_IO */
 static void
 input_data(int dev, ulong *sect_buf, int words)
 {
 	insw(ATA_CURR_BASE(dev)+ATA_DATA_REG, sect_buf, words << 1);
 }
 
-#endif	/* __PPC__ */
+#endif	/* CONFIG_IDE_SWAP_IO */
 
 /* -------------------------------------------------------------------------
  */
@@ -1579,7 +1579,7 @@ int ide_device_present(int dev)
  * ATAPI Support
  */
 
-#if defined(__PPC__) || defined(CONFIG_PXA_PCMCIA)
+#if defined(CONFIG_IDE_SWAP_IO)
 /* since ATAPI may use commands with not 4 bytes alligned length
  * we have our own transfer functions, 2 bytes alligned */
 static void
@@ -1646,7 +1646,7 @@ input_data_shorts(int dev, ushort *sect_buf, int shorts)
 #endif
 }
 
-#else	/* ! __PPC__ */
+#else	/* ! CONFIG_IDE_SWAP_IO */
 static void
 output_data_shorts(int dev, ushort *sect_buf, int shorts)
 {
@@ -1659,7 +1659,7 @@ input_data_shorts(int dev, ushort *sect_buf, int shorts)
 	insw(ATA_CURR_BASE(dev)+ATA_DATA_REG, sect_buf, shorts);
 }
 
-#endif	/* __PPC__ */
+#endif	/* CONFIG_IDE_SWAP_IO */
 
 /*
  * Wait until (Status & mask) == res, or timeout (in ms)
diff --git a/doc/README.PXA_CF b/doc/README.PXA_CF
index 6a0f236..1d76b32 100644
--- a/doc/README.PXA_CF
+++ b/doc/README.PXA_CF
@@ -6,6 +6,14 @@ follow the connections of the standard lubbock. Anyway just the block
 marked memory configuration should be touched since the other parameters
 are imposed by the PXA architecture.
 
+EDIT 2010-07-01: in common/cmd_ide.c, having CONFIG_PXA_PCMCIA defined
+would cause looping on inw()/outw() rather than using insw()/outsw(),
+thus making sure IDE / ATA bytes are properly swapped. This behaviour
+is now controlled by CONFIG_IDE_SWAP_IO, therefore PXA boards with
+PCMCIA should #define CONFIG_IDE_SWAP_IO.
+
+#define CONFIG_IDE_SWAP_IO
+
 #define CONFIG_PXA_PCMCIA 1
 #define CONFIG_PXA_IDE 1
 
diff --git a/include/configs/ap325rxa.h b/include/configs/ap325rxa.h
index 70dd47e..80a5797 100644
--- a/include/configs/ap325rxa.h
+++ b/include/configs/ap325rxa.h
@@ -138,6 +138,7 @@
 #define CONFIG_SYS_ATA_DATA_OFFSET	0x200	/* data reg offset */
 #define CONFIG_SYS_ATA_REG_OFFSET	0x200	/* reg offset */
 #define CONFIG_SYS_ATA_ALT_OFFSET	0x210	/* alternate register offset */
+#define CONFIG_IDE_SWAP_IO
 
 /* if you use all NOR Flash , you change dip-switch. Please see Manual. */
 #define CONFIG_SYS_MAX_FLASH_BANKS	1
diff --git a/include/configs/ms7720se.h b/include/configs/ms7720se.h
index ba0a3f8..0ea3527 100644
--- a/include/configs/ms7720se.h
+++ b/include/configs/ms7720se.h
@@ -122,5 +122,6 @@
 #define CONFIG_SYS_ATA_DATA_OFFSET	0		/* data reg offset */
 #define CONFIG_SYS_ATA_REG_OFFSET	0		/* reg offset */
 #define CONFIG_SYS_ATA_ALT_OFFSET	0x200		/* alternate register offset */
+#define CONFIG_IDE_SWAP_IO
 
 #endif	/* __MS7720SE_H */
diff --git a/include/configs/r2dplus.h b/include/configs/r2dplus.h
index 8931b97..955f3ff 100644
--- a/include/configs/r2dplus.h
+++ b/include/configs/r2dplus.h
@@ -96,6 +96,7 @@
 #define CONFIG_SYS_ATA_DATA_OFFSET	0x1000	/* data reg offset */
 #define CONFIG_SYS_ATA_REG_OFFSET	0x1000	/* reg offset */
 #define CONFIG_SYS_ATA_ALT_OFFSET	0x800	/* alternate register offset */
+#define CONFIG_IDE_SWAP_IO
 
 /*
  * SuperH PCI Bridge Configration
diff --git a/include/configs/r7780mp.h b/include/configs/r7780mp.h
index 71c570e..3afe93a 100644
--- a/include/configs/r7780mp.h
+++ b/include/configs/r7780mp.h
@@ -171,6 +171,7 @@
 #define CONFIG_SYS_ATA_DATA_OFFSET     0x1000          /* data reg offset */
 #define CONFIG_SYS_ATA_REG_OFFSET      0x1000          /* reg offset */
 #define CONFIG_SYS_ATA_ALT_OFFSET      0x800           /* alternate register offset */
+#define CONFIG_IDE_SWAP_IO
 #endif /* CONFIG_CMD_IDE */
 
 #endif /* __R7780RP_H */
-- 
1.6.4.4

[U-Boot] [PATCH V2 2/3] orion5x: add ide support for Marvell SATA

[Albert Aribaud]

Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
 arch/arm/include/asm/arch-orion5x/orion5x.h |   20 ++++++++++++++++++++
 common/cmd_ide.c                            |    4 ++++
 2 files changed, 24 insertions(+), 0 deletions(-)

diff --git a/arch/arm/include/asm/arch-orion5x/orion5x.h b/arch/arm/include/asm/arch-orion5x/orion5x.h
index 4008c84..11de968 100644
--- a/arch/arm/include/asm/arch-orion5x/orion5x.h
+++ b/arch/arm/include/asm/arch-orion5x/orion5x.h
@@ -55,6 +55,26 @@
 #define ORION5X_USB20_PORT0_BASE		(ORION5X_REGISTER(0x50000))
 #define ORION5X_USB20_PORT1_BASE		(ORION5X_REGISTER(0xA0000))
 #define ORION5X_EGIGA_BASE			(ORION5X_REGISTER(0x72000))
+#define ORION5X_SATA_BASE			(ORION5X_REGISTER(0x80000))
+#define ORION5X_SATA_PORT0_OFFSET		0x2000
+#define ORION5X_SATA_PORT1_OFFSET		0x4000
+
+/*
+ * SATA definitions needed for controller initialization
+ */
+/* SControl register address */
+#define ORION5X_SATA_PORT1_SCONTROL_REG \
+	(ORION5X_SATA_BASE+ORION5X_SATA_PORT1_OFFSET+0x308)
+/* Mask and values for device DETection and link initialization */
+#define ORION5X_SATA_SCONTROL_DET_MASK		0x0000000F
+#define ORION5X_SATA_SCONTROL_DET_NONE		0x00000000
+#define ORION5X_SATA_SCONTROL_DET_INIT		0x00000001
+/* Mask and values for device Interface Power Management */
+#define ORION5X_SATA_SCONTROL_IMP_MASK		0x00000F00
+#define ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED	0x00000300
+
+/* Orion5x will need byte-swapping if using ATA registers */
+#define CONFIG_IDE_SWAP_IO
 
 #define CONFIG_MAX_RAM_BANK_SIZE		(64*1024*1024)
 
diff --git a/common/cmd_ide.c b/common/cmd_ide.c
index 9292a5b..7f98ebb 100644
--- a/common/cmd_ide.c
+++ b/common/cmd_ide.c
@@ -45,6 +45,10 @@
 #include <mpc5xxx.h>
 #endif
 
+#ifdef CONFIG_ORION5X
+#include <asm/arch/orion5x.h>
+#endif
+
 #include <ide.h>
 #include <ata.h>
 
-- 
1.6.4.4

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Albert Aribaud]

Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
---
 board/LaCie/edminiv2/edminiv2.c |   26 ++++++++++++++++++++++++++
 include/configs/edminiv2.h      |   29 +++++++++++++++++++++++++++--
 2 files changed, 53 insertions(+), 2 deletions(-)

diff --git a/board/LaCie/edminiv2/edminiv2.c b/board/LaCie/edminiv2/edminiv2.c
index 54c0ffe..84375c2 100644
--- a/board/LaCie/edminiv2/edminiv2.c
+++ b/board/LaCie/edminiv2/edminiv2.c
@@ -80,13 +80,39 @@ ulong board_flash_get_legacy(ulong base, int banknum, flash_info_t *info)
 }
 #endif				/* CONFIG_SYS_FLASH_CFI */
 
+/*
+ * ED Mini V2 uses SATA PORT1. Initialize this port and
+ * disable low power on it.
+ */
+
+/* mask for isolating IPM and DET fields in SControl register */
+#define EDMINIV2_SCONTROL_MASK (ORION5X_SATA_SCONTROL_DET_MASK \
+	|| ORION5X_SATA_SCONTROL_IMP_MASK)
+
+/* IPM and DET values for initializing link */
+#define EDMINIV2_PORT_INIT (ORION5X_SATA_SCONTROL_DET_INIT \
+	|| ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED)
+
+/* IPM and DET values to use link once initialized */
+#define EDMINIV2_PORT_USE (ORION5X_SATA_SCONTROL_DET_NONE \
+	|| ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED)
+
 int board_init(void)
 {
+	u32 reg;
+
 	/* arch number of board */
 	gd->bd->bi_arch_number = MACH_TYPE_EDMINI_V2;
 
 	/* boot parameter start at 256th byte of RAM base */
 	gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
 
+	/* reset port 1 of SATAHC controller */
+	reg = readl(ORION5X_SATA_PORT1_SCONTROL_REG);
+	reg = (reg & ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_INIT;
+	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);
+	reg = (reg & ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_USE;
+	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);
+
 	return 0;
 }
diff --git a/include/configs/edminiv2.h b/include/configs/edminiv2.h
index c3d95a0..5c328c8 100644
--- a/include/configs/edminiv2.h
+++ b/include/configs/edminiv2.h
@@ -52,6 +52,12 @@
 #define CONFIG_SYS_HZ		1000
 
 /*
+ * __io is necessary for cmd_ide to compile
+ */
+
+#define __io
+
+/*
  * Board-specific values for Orion5x MPP low level init:
  * - MPPs 12 to 15 are SATA LEDs (mode 5)
  * - Others are GPIO/unused (mode 3 for MPP0, mode 5 for
@@ -60,7 +66,7 @@
 
 #define ORION5X_MPP0_7		0x00000003
 #define ORION5X_MPP8_15		0x55550000
-#define ORION5X_MPP16_23	0x00000000
+#define ORION5X_MPP16_23	0x00005555
 
 /*
  * Board-specific values for Orion5x GPIO low level init:
@@ -74,7 +80,6 @@
  */
 
 #define ORION5X_GPIO_OUT_ENABLE	0x03fcffff
-#define ORION5X_GPIO_OUT_VALUE	0x03fcffff
 
 /*
  * NS16550 Configuration
@@ -131,6 +136,7 @@
  * Commands configuration - using default command set for now
  */
 #include <config_cmd_default.h>
+
 /*
  * Disabling some default commands for staggered bring-up
  */
@@ -139,6 +145,25 @@
 #undef CONFIG_CMD_NFS	/* no NFS since no net */
 
 /*
+ * ED Mini has a connector for SATA Port 1
+ */
+
+#define CONFIG_CMD_IDE
+#define CONFIG_DOS_PARTITION
+#define CONFIG_CMD_EXT2
+
+#define CONFIG_LIBATA
+#define CONFIG_SYS_IDE_MAXBUS		1
+#define CONFIG_SYS_IDE_MAXDEVICE	1
+#define CONFIG_SYS_ATA_BASE_ADDR	ORION5X_SATA_BASE
+#define CONFIG_SYS_ATA_IDE0_OFFSET	ORION5X_SATA_PORT1_OFFSET
+#define CONFIG_SYS_ATA_DATA_OFFSET	(0x0100)
+#define CONFIG_SYS_ATA_REG_OFFSET	(0x0100)
+#define CONFIG_SYS_ATA_ALT_OFFSET	(0x0100)
+#define CONFIG_SYS_ATA_STRIDE		4
+#define CONFIG_LBA48
+
+/*
  *  Environment variables configurations
  */
 #define CONFIG_ENV_IS_IN_FLASH		1
-- 
1.6.4.4

[U-Boot] [PATCH V2 2/3] orion5x: add ide support for Marvell SATA

[Prafulla Wadaskar]

 

> -----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 06, 2010 9:29 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH V2 2/3] orion5x: add ide support for 
> Marvell SATA
> 
> 
> Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
> ---
>  arch/arm/include/asm/arch-orion5x/orion5x.h |   20 
> ++++++++++++++++++++
>  common/cmd_ide.c                            |    4 ++++
>  2 files changed, 24 insertions(+), 0 deletions(-)
> 
> diff --git a/arch/arm/include/asm/arch-orion5x/orion5x.h 
> b/arch/arm/include/asm/arch-orion5x/orion5x.h
> index 4008c84..11de968 100644
> --- a/arch/arm/include/asm/arch-orion5x/orion5x.h
> +++ b/arch/arm/include/asm/arch-orion5x/orion5x.h
> @@ -55,6 +55,26 @@
>  #define ORION5X_USB20_PORT0_BASE		
> (ORION5X_REGISTER(0x50000))
>  #define ORION5X_USB20_PORT1_BASE		
> (ORION5X_REGISTER(0xA0000))
>  #define ORION5X_EGIGA_BASE			
> (ORION5X_REGISTER(0x72000))
> +#define ORION5X_SATA_BASE			
> (ORION5X_REGISTER(0x80000))
> +#define ORION5X_SATA_PORT0_OFFSET		0x2000
> +#define ORION5X_SATA_PORT1_OFFSET		0x4000
> +
> +/*
> + * SATA definitions needed for controller initialization
> + */
> +/* SControl register address */
> +#define ORION5X_SATA_PORT1_SCONTROL_REG \
> +	(ORION5X_SATA_BASE+ORION5X_SATA_PORT1_OFFSET+0x308)
> +/* Mask and values for device DETection and link initialization */
> +#define ORION5X_SATA_SCONTROL_DET_MASK		0x0000000F
> +#define ORION5X_SATA_SCONTROL_DET_NONE		0x00000000
> +#define ORION5X_SATA_SCONTROL_DET_INIT		0x00000001
> +/* Mask and values for device Interface Power Management */
> +#define ORION5X_SATA_SCONTROL_IMP_MASK		0x00000F00
> +#define ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED	0x00000300

Please use c structure for register defination.

Regards..
Prafulla ..

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Prafulla Wadaskar]

 

> -----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 06, 2010 9:29 PM
> To: u-boot at lists.denx.de
> Subject: [U-Boot] [PATCH V2 3/3] edmini: add IDE support
> 
> 
> Signed-off-by: Albert Aribaud <albert.aribaud@free.fr>
> ---
>  board/LaCie/edminiv2/edminiv2.c |   26 ++++++++++++++++++++++++++
>  include/configs/edminiv2.h      |   29 +++++++++++++++++++++++++++--
>  2 files changed, 53 insertions(+), 2 deletions(-)
> 
> diff --git a/board/LaCie/edminiv2/edminiv2.c 
> b/board/LaCie/edminiv2/edminiv2.c
> index 54c0ffe..84375c2 100644
> --- a/board/LaCie/edminiv2/edminiv2.c
> +++ b/board/LaCie/edminiv2/edminiv2.c
> @@ -80,13 +80,39 @@ ulong board_flash_get_legacy(ulong base, 
> int banknum, flash_info_t *info)
>  }
>  #endif				/* CONFIG_SYS_FLASH_CFI */
>  
> +/*
> + * ED Mini V2 uses SATA PORT1. Initialize this port and
> + * disable low power on it.
> + */
> +
> +/* mask for isolating IPM and DET fields in SControl register */
> +#define EDMINIV2_SCONTROL_MASK (ORION5X_SATA_SCONTROL_DET_MASK \
> +	|| ORION5X_SATA_SCONTROL_IMP_MASK)
> +
> +/* IPM and DET values for initializing link */
> +#define EDMINIV2_PORT_INIT (ORION5X_SATA_SCONTROL_DET_INIT \
> +	|| ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED)
> +
> +/* IPM and DET values to use link once initialized */
> +#define EDMINIV2_PORT_USE (ORION5X_SATA_SCONTROL_DET_NONE \
> +	|| ORION5X_SATA_SCONTROL_IMP_NO_LP_ALLOWED)
> +
>  int board_init(void)
>  {
> +	u32 reg;
> +
>  	/* arch number of board */
>  	gd->bd->bi_arch_number = MACH_TYPE_EDMINI_V2;
>  
>  	/* boot parameter start at 256th byte of RAM base */
>  	gd->bd->bi_boot_params = gd->bd->bi_dram[0].start + 0x100;
>  
> +	/* reset port 1 of SATAHC controller */

don't you think it is better to add separate driver for Orion/Kirkwood/Marvell SATAC so that this code and even some configurations can be shared by other boards.
 
> +	reg = readl(ORION5X_SATA_PORT1_SCONTROL_REG);
> +	reg = (reg & ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_INIT;
> +	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);
> +	reg = (reg & ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_USE;
> +	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);

make use of c-structure for registers

> +
>  	return 0;
>  }
> diff --git a/include/configs/edminiv2.h b/include/configs/edminiv2.h
> index c3d95a0..5c328c8 100644
> --- a/include/configs/edminiv2.h
> +++ b/include/configs/edminiv2.h
> @@ -52,6 +52,12 @@
>  #define CONFIG_SYS_HZ		1000
>  
>  /*
> + * __io is necessary for cmd_ide to compile
> + */
> +
> +#define __io
> +
> +/*
>   * Board-specific values for Orion5x MPP low level init:
>   * - MPPs 12 to 15 are SATA LEDs (mode 5)
>   * - Others are GPIO/unused (mode 3 for MPP0, mode 5 for
> @@ -60,7 +66,7 @@
>  
>  #define ORION5X_MPP0_7		0x00000003
>  #define ORION5X_MPP8_15		0x55550000
> -#define ORION5X_MPP16_23	0x00000000
> +#define ORION5X_MPP16_23	0x00005555
>  
>  /*
>   * Board-specific values for Orion5x GPIO low level init:
> @@ -74,7 +80,6 @@
>   */
>  
>  #define ORION5X_GPIO_OUT_ENABLE	0x03fcffff
> -#define ORION5X_GPIO_OUT_VALUE	0x03fcffff
>  
>  /*
>   * NS16550 Configuration
> @@ -131,6 +136,7 @@
>   * Commands configuration - using default command set for now
>   */
>  #include <config_cmd_default.h>
> +
>  /*
>   * Disabling some default commands for staggered bring-up
>   */
> @@ -139,6 +145,25 @@
>  #undef CONFIG_CMD_NFS	/* no NFS since no net */
>  
>  /*
> + * ED Mini has a connector for SATA Port 1
> + */
> +
> +#define CONFIG_CMD_IDE
> +#define CONFIG_DOS_PARTITION
> +#define CONFIG_CMD_EXT2
> +
> +#define CONFIG_LIBATA
> +#define CONFIG_SYS_IDE_MAXBUS		1
> +#define CONFIG_SYS_IDE_MAXDEVICE	1
> +#define CONFIG_SYS_ATA_BASE_ADDR	ORION5X_SATA_BASE
> +#define CONFIG_SYS_ATA_IDE0_OFFSET	ORION5X_SATA_PORT1_OFFSET
> +#define CONFIG_SYS_ATA_DATA_OFFSET	(0x0100)
> +#define CONFIG_SYS_ATA_REG_OFFSET	(0x0100)
> +#define CONFIG_SYS_ATA_ALT_OFFSET	(0x0100)

can you avoid magic numbers, if not some comments please

> +#define CONFIG_SYS_ATA_STRIDE		4
> +#define CONFIG_LBA48
> +
> +/*
>   *  Environment variables configurations
>   */
>  #define CONFIG_ENV_IS_IN_FLASH		1
> -- 
> 1.6.4.4
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> http://lists.denx.de/mailman/listinfo/u-boot
> 

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Albert ARIBAUD]

Le 07/07/2010 10:38, Prafulla Wadaskar a ?crit :

> don't you think it is better to add separate driver for Orion/Kirkwood/Marvell
> SATAC so that this code and even some configurations can be shared by other boards.

If you mean 'why use cmd_ide rather than the Marvell sata driver also 
posted recently', I have expressed my opinion there: 
<http://article.gmane.org/gmane.comp.boot-loaders.u-boot/80702>.

If you mean 'why not make this a standalone driver, e.g. 
driver/net/mvsata_ide.{ch} under a configuration option 
CONFIG_MVSATA_IDE' -- why not, but it would basically be empty of 
'driving' functionality (this is handled by cmd_ide) and would only 
provide initialization code, subject to board options (e.g. 
CONFIG_MVSATA_IDE_ENABLE_PORT{0,1}).

>> +	reg = readl(ORION5X_SATA_PORT1_SCONTROL_REG);
>> +	reg = (reg&  ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_INIT;
>> +	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);
>> +	reg = (reg&  ~EDMINIV2_SCONTROL_MASK) | EDMINIV2_PORT_USE;
>> +	writel(reg, ORION5X_SATA_PORT1_SCONTROL_REG);
>
> make use of c-structure for registers

Will do.

>> +#define CONFIG_SYS_ATA_DATA_OFFSET	(0x0100)
>> +#define CONFIG_SYS_ATA_REG_OFFSET	(0x0100)
>> +#define CONFIG_SYS_ATA_ALT_OFFSET	(0x0100)
>
> can you avoid magic numbers, if not some comments please

Will fix this.

Thanks Prafulla for your feedback.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 2/3] orion5x: add ide support for Marvell SATA

[Albert ARIBAUD]

Le 07/07/2010 10:25, Prafulla Wadaskar a ?crit :

>> +/* SControl register address */
>> +#define ORION5X_SATA_PORT1_SCONTROL_REG \
>> +	(ORION5X_SATA_BASE+ORION5X_SATA_PORT1_OFFSET+0x308)
>
> Please use c structure for register defination.

Hmm... There is a single register definition here -- I don't need more 
than this one. If I do a struct, it will only contain a single field. 
Still want this?

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 2/3] orion5x: add ide support for Marvell SATA

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Wednesday, July 07, 2010 4:54 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de
> Subject: Re: [PATCH V2 2/3] orion5x: add ide support for Marvell SATA
> 
> Le 07/07/2010 10:25, Prafulla Wadaskar a ?crit :
> 
> >> +/* SControl register address */
> >> +#define ORION5X_SATA_PORT1_SCONTROL_REG \
> >> +	(ORION5X_SATA_BASE+ORION5X_SATA_PORT1_OFFSET+0x308)
> >
> > Please use c structure for register defination.
> 
> Hmm... There is a single register definition here -- I don't 
> need more 
> than this one. If I do a struct, it will only contain a single field. 
> Still want this?

As per strategy and guidelines from Wolfgang- YES.
we need it, I need to correct it for Kirkwood too

Regards..
Prafulla . .

> 
> Amicalement,
> -- 
> Albert.
> 

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Wednesday, July 07, 2010 4:20 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V2 3/3] edmini: add IDE support
> 
> Le 07/07/2010 10:38, Prafulla Wadaskar a ?crit :
> 
> > don't you think it is better to add separate driver for 
> Orion/Kirkwood/Marvell
> > SATAC so that this code and even some configurations can be 
> shared by other boards.
> 
> If you mean 'why use cmd_ide rather than the Marvell sata driver also 
> posted recently', I have expressed my opinion there: 
> <http://article.gmane.org/gmane.comp.boot-loaders.u-boot/80702>.
> 
> If you mean 'why not make this a standalone driver, e.g. 
> driver/net/mvsata_ide.{ch} under a configuration option 
> CONFIG_MVSATA_IDE' -- why not, but it would basically be empty of 
> 'driving' functionality (this is handled by cmd_ide) and would only 
> provide initialization code, subject to board options (e.g. 
> CONFIG_MVSATA_IDE_ENABLE_PORT{0,1}).

then you should add below initialization code in cpu.c for respective SoCs
and function call here

Regards..
Prafulla . .

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Albert ARIBAUD]

Le 07/07/2010 13:46, Prafulla Wadaskar a ?crit :

>> If you mean 'why not make this a standalone driver, e.g.
>> driver/net/mvsata_ide.{ch} under a configuration option
>> CONFIG_MVSATA_IDE' -- why not, but it would basically be empty of
>> 'driving' functionality (this is handled by cmd_ide) and would only
>> provide initialization code, subject to board options (e.g.
>> CONFIG_MVSATA_IDE_ENABLE_PORT{0,1}).
>
> then you should add below initialization code in cpu.c for respective SoCs
> and function call here

Ok, but that means duplicating this code across SoCs aven though it is 
actually identical for all of them.

Amicalement,
-- 
Albert.

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Prafulla Wadaskar]

 

> -----Original Message-----
> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr] 
> Sent: Wednesday, July 07, 2010 7:39 PM
> To: Prafulla Wadaskar
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V2 3/3] edmini: add IDE support
> 
> Le 07/07/2010 13:46, Prafulla Wadaskar a ?crit :
> 
> >> If you mean 'why not make this a standalone driver, e.g.
> >> driver/net/mvsata_ide.{ch} under a configuration option
> >> CONFIG_MVSATA_IDE' -- why not, but it would basically be empty of
> >> 'driving' functionality (this is handled by cmd_ide) and would only
> >> provide initialization code, subject to board options (e.g.
> >> CONFIG_MVSATA_IDE_ENABLE_PORT{0,1}).
> >
> > then you should add below initialization code in cpu.c for 
> respective SoCs
> > and function call here
> 
> Ok, but that means duplicating this code across SoCs aven 
> though it is 
> actually identical for all of them.

That's why I initially suggested to put it as separate driver than can be used across SoCs.
Even though it is very small code, it makes more sense creating driver for it.

Regards..
Prafulla . .

[U-Boot] [PATCH V2 3/3] edmini: add IDE support

[Albert ARIBAUD]

Le 08/07/2010 08:01, Prafulla Wadaskar a ?crit :
>
>
>> -----Original Message-----
>> From: Albert ARIBAUD [mailto:albert.aribaud at free.fr]
>> Sent: Wednesday, July 07, 2010 7:39 PM
>> To: Prafulla Wadaskar
>> Cc: u-boot at lists.denx.de
>> Subject: Re: [U-Boot] [PATCH V2 3/3] edmini: add IDE support
>>
>> Le 07/07/2010 13:46, Prafulla Wadaskar a ?crit :
>>
>>>> If you mean 'why not make this a standalone driver, e.g.
>>>> driver/net/mvsata_ide.{ch} under a configuration option
>>>> CONFIG_MVSATA_IDE' -- why not, but it would basically be empty of
>>>> 'driving' functionality (this is handled by cmd_ide) and would only
>>>> provide initialization code, subject to board options (e.g.
>>>> CONFIG_MVSATA_IDE_ENABLE_PORT{0,1}).
>>>
>>> then you should add below initialization code in cpu.c for
>> respective SoCs
>>> and function call here
>>
>> Ok, but that means duplicating this code across SoCs aven
>> though it is
>> actually identical for all of them.
>
> That's why I initially suggested to put it as separate driver than can be used across SoCs.
> Even though it is very small code, it makes more sense creating driver for it.

Ok then, a driver it is.

Amicalement,
-- 
Albert.