[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Yuantian.Tang at freescale.com]

From: Tang Yuantian <Yuantian.Tang@freescale.com>

Freescale ARM-based Layerscape contains a SATA controller
which comply with the serial ATA 3.0 specification and the
AHCI 1.3 specification.
This patch adds SATA feature on ls2080aqds, ls2080ardb and
ls1043aqds boards.

Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
---
v5:
	- re-organize the code
v4:
	- rebase to lastest git tree
	- add another ARMv8 platform which is ls1043aqds
v3:
	- rename ls2085a to ls2080a
	- rebase to the latest git tree
	- replace the magic number with micro variable
v2:
	- rebase to the latest git tree

 arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43 ++++++++++++++++++++++
 .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
 arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31 ++++++++++++++++
 include/configs/ls1043aqds.h                       | 17 +++++++++
 include/configs/ls2080aqds.h                       | 18 +++++++++
 include/configs/ls2080ardb.h                       | 18 +++++++++
 6 files changed, 131 insertions(+)

diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
index 8896b70..574ffc4 100644
--- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
+++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
@@ -6,6 +6,8 @@
 
 #include <common.h>
 #include <fsl_ifc.h>
+#include <ahci.h>
+#include <scsi.h>
 #include <asm/arch/soc.h>
 #include <asm/io.h>
 #include <asm/global_data.h>
@@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
 	erratum_a009203();
 }
 
+#ifdef CONFIG_SCSI_AHCI_PLAT
+int sata_init(void)
+{
+	struct ccsr_ahci __iomem *ccsr_ahci;
+
+	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
+	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
+	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
+
+	ccsr_ahci  = (void *)CONFIG_SYS_SATA1;
+	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
+	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
+
+	ahci_init((void __iomem *)CONFIG_SYS_SATA1);
+	scsi_scan(0);
+
+	return 0;
+}
+#endif
+
 #elif defined(CONFIG_LS1043A)
+#ifdef CONFIG_SCSI_AHCI_PLAT
+int sata_init(void)
+{
+	struct ccsr_ahci __iomem *ccsr_ahci = (void *)CONFIG_SYS_SATA;
+
+	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
+	out_le32(&ccsr_ahci->pp2c, AHCI_PORT_PHY_2_CFG);
+	out_le32(&ccsr_ahci->pp3c, AHCI_PORT_PHY_3_CFG);
+	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
+
+	ahci_init((void __iomem *)CONFIG_SYS_SATA);
+	scsi_scan(0);
+
+	return 0;
+}
+#endif
+
 void fsl_lsch2_early_init_f(void)
 {
 	struct ccsr_cci400 *cci = (struct ccsr_cci400 *)CONFIG_SYS_CCI400_ADDR;
@@ -141,6 +180,10 @@ void fsl_lsch2_early_init_f(void)
 #ifdef CONFIG_BOARD_LATE_INIT
 int board_late_init(void)
 {
+#ifdef CONFIG_SCSI_AHCI_PLAT
+	sata_init();
+#endif
+
 	return 0;
 }
 #endif
diff --git a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
index cd96604..91f3ce8 100644
--- a/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
+++ b/arch/arm/include/asm/arch-fsl-layerscape/immap_lsch3.h
@@ -69,6 +69,10 @@
 #define TZASC_REGION_ATTRIBUTES_0(x)	((TZASC1_BASE + (x * 0x10000)) + 0x110)
 #define TZASC_REGION_ID_ACCESS_0(x)	((TZASC1_BASE + (x * 0x10000)) + 0x114)
 
+/* SATA */
+#define AHCI_BASE_ADDR1				(CONFIG_SYS_IMMR + 0x02200000)
+#define AHCI_BASE_ADDR2				(CONFIG_SYS_IMMR + 0x02210000)
+
 /* PCIe */
 #define CONFIG_SYS_PCIE1_ADDR			(CONFIG_SYS_IMMR + 0x2400000)
 #define CONFIG_SYS_PCIE2_ADDR			(CONFIG_SYS_IMMR + 0x2500000)
diff --git a/arch/arm/include/asm/arch-fsl-layerscape/soc.h b/arch/arm/include/asm/arch-fsl-layerscape/soc.h
index 504c1f9..83186d6 100644
--- a/arch/arm/include/asm/arch-fsl-layerscape/soc.h
+++ b/arch/arm/include/asm/arch-fsl-layerscape/soc.h
@@ -51,6 +51,37 @@ struct cpu_type {
 #define SVR_SOC_VER(svr)	(((svr) >> 8) & SVR_WO_E)
 #define IS_E_PROCESSOR(svr)	(!((svr >> 8) & 0x1))
 
+/* ahci port register default value */
+#define AHCI_PORT_PHY_1_CFG    0xa003fffe
+#define AHCI_PORT_PHY_2_CFG    0x28184d1f
+#define AHCI_PORT_PHY_3_CFG    0x0e081509
+#define AHCI_PORT_TRANS_CFG    0x08000025
+
+/* AHCI (sata) register map */
+struct ccsr_ahci {
+	u32 res1[0xa4/4];	/* 0x0 - 0xa4 */
+	u32 pcfg;	/* port config */
+	u32 ppcfg;	/* port phy1 config */
+	u32 pp2c;	/* port phy2 config */
+	u32 pp3c;	/* port phy3 config */
+	u32 pp4c;	/* port phy4 config */
+	u32 pp5c;	/* port phy5 config */
+	u32 axicc;	/* AXI cache control */
+	u32 paxic;	/* port AXI config */
+	u32 axipc;	/* AXI PROT control */
+	u32 ptc;	/* port Trans Config */
+	u32 pts;	/* port Trans Status */
+	u32 plc;	/* port link config */
+	u32 plc1;	/* port link config1 */
+	u32 plc2;	/* port link config2 */
+	u32 pls;	/* port link status */
+	u32 pls1;	/* port link status1 */
+	u32 pcmdc;	/* port CMD config */
+	u32 ppcs;	/* port phy control status */
+	u32 pberr;	/* port 0/1 BIST error */
+	u32 cmds;	/* port 0/1 CMD status error */
+};
+
 #ifdef CONFIG_FSL_LSCH3
 void fsl_lsch3_early_init_f(void);
 #elif defined(CONFIG_FSL_LSCH2)
diff --git a/include/configs/ls1043aqds.h b/include/configs/ls1043aqds.h
index 4aeb238..398f1c3 100644
--- a/include/configs/ls1043aqds.h
+++ b/include/configs/ls1043aqds.h
@@ -88,6 +88,23 @@ unsigned long get_board_ddr_clk(void);
 #define CONFIG_SYS_FSL_PBL_RCW board/freescale/ls1043aqds/ls1043aqds_rcw_sd_ifc.cfg
 #endif
 
+/* SATA */
+#define CONFIG_LIBATA
+#define CONFIG_SCSI_AHCI
+#define CONFIG_SCSI_AHCI_PLAT
+#define CONFIG_CMD_SCSI
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_BOARD_LATE_INIT
+
+#define CONFIG_SYS_SATA				AHCI_BASE_ADDR
+
+#define CONFIG_SYS_SCSI_MAX_SCSI_ID		1
+#define CONFIG_SYS_SCSI_MAX_LUN			1
+#define CONFIG_SYS_SCSI_MAX_DEVICE		(CONFIG_SYS_SCSI_MAX_SCSI_ID * \
+						CONFIG_SYS_SCSI_MAX_LUN)
+
 /*
  * IFC Definitions
  */
diff --git a/include/configs/ls2080aqds.h b/include/configs/ls2080aqds.h
index b8bdf62..100c0ee 100644
--- a/include/configs/ls2080aqds.h
+++ b/include/configs/ls2080aqds.h
@@ -40,6 +40,24 @@ unsigned long get_board_ddr_clk(void);
 #endif
 #define CONFIG_FSL_DDR_BIST	/* enable built-in memory test */
 
+/* SATA */
+#define CONFIG_LIBATA
+#define CONFIG_SCSI_AHCI
+#define CONFIG_SCSI_AHCI_PLAT
+#define CONFIG_CMD_SCSI
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_BOARD_LATE_INIT
+
+#define CONFIG_SYS_SATA1			AHCI_BASE_ADDR1
+#define CONFIG_SYS_SATA2			AHCI_BASE_ADDR2
+
+#define CONFIG_SYS_SCSI_MAX_SCSI_ID		1
+#define CONFIG_SYS_SCSI_MAX_LUN			1
+#define CONFIG_SYS_SCSI_MAX_DEVICE		(CONFIG_SYS_SCSI_MAX_SCSI_ID * \
+						CONFIG_SYS_SCSI_MAX_LUN)
+
 /* undefined CONFIG_FSL_DDR_SYNC_REFRESH for simulator */
 
 #define CONFIG_SYS_NOR0_CSPR_EXT	(0x0)
diff --git a/include/configs/ls2080ardb.h b/include/configs/ls2080ardb.h
index 65d4f64..75d98dc 100644
--- a/include/configs/ls2080ardb.h
+++ b/include/configs/ls2080ardb.h
@@ -42,6 +42,24 @@ unsigned long get_board_sys_clk(void);
 #endif
 #define CONFIG_FSL_DDR_BIST	/* enable built-in memory test */
 
+/* SATA */
+#define CONFIG_LIBATA
+#define CONFIG_SCSI_AHCI
+#define CONFIG_SCSI_AHCI_PLAT
+#define CONFIG_CMD_SCSI
+#define CONFIG_CMD_FAT
+#define CONFIG_CMD_EXT2
+#define CONFIG_DOS_PARTITION
+#define CONFIG_BOARD_LATE_INIT
+
+#define CONFIG_SYS_SATA1			AHCI_BASE_ADDR1
+#define CONFIG_SYS_SATA2			AHCI_BASE_ADDR2
+
+#define CONFIG_SYS_SCSI_MAX_SCSI_ID		1
+#define CONFIG_SYS_SCSI_MAX_LUN			1
+#define CONFIG_SYS_SCSI_MAX_DEVICE		(CONFIG_SYS_SCSI_MAX_SCSI_ID * \
+						CONFIG_SYS_SCSI_MAX_LUN)
+
 /* undefined CONFIG_FSL_DDR_SYNC_REFRESH for simulator */
 
 #define CONFIG_SYS_NOR0_CSPR_EXT	(0x0)
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Sinan Akman]

On 01/12/15 10:27 PM, Yuantian.Tang at freescale.com wrote:
> From: Tang Yuantian <Yuantian.Tang@freescale.com>
>
> Freescale ARM-based Layerscape contains a SATA controller
> which comply with the serial ATA 3.0 specification and the
> AHCI 1.3 specification.
> This patch adds SATA feature on ls2080aqds, ls2080ardb and
> ls1043aqds boards.
>
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> ---
> v5:
> 	- re-organize the code
> v4:
> 	- rebase to lastest git tree
> 	- add another ARMv8 platform which is ls1043aqds
> v3:
> 	- rename ls2085a to ls2080a
> 	- rebase to the latest git tree
> 	- replace the magic number with micro variable
> v2:
> 	- rebase to the latest git tree
>
>   arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43 ++++++++++++++++++++++
>   .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>   arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31 ++++++++++++++++
>   include/configs/ls1043aqds.h                       | 17 +++++++++
>   include/configs/ls2080aqds.h                       | 18 +++++++++
>   include/configs/ls2080ardb.h                       | 18 +++++++++
>   6 files changed, 131 insertions(+)

   Thanks for the update.

   Reviewed-by: Sinan Akman <sinan@writeme.com>

   Regards
   Sinan Akman

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[York Sun]

On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
> From: Tang Yuantian <Yuantian.Tang@freescale.com>
> 
> Freescale ARM-based Layerscape contains a SATA controller
> which comply with the serial ATA 3.0 specification and the
> AHCI 1.3 specification.
> This patch adds SATA feature on ls2080aqds, ls2080ardb and
> ls1043aqds boards.
> 
> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> ---
> v5:
> 	- re-organize the code
> v4:
> 	- rebase to lastest git tree
> 	- add another ARMv8 platform which is ls1043aqds
> v3:
> 	- rename ls2085a to ls2080a
> 	- rebase to the latest git tree
> 	- replace the magic number with micro variable
> v2:
> 	- rebase to the latest git tree
> 
>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43 ++++++++++++++++++++++
>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31 ++++++++++++++++
>  include/configs/ls1043aqds.h                       | 17 +++++++++
>  include/configs/ls2080aqds.h                       | 18 +++++++++
>  include/configs/ls2080ardb.h                       | 18 +++++++++
>  6 files changed, 131 insertions(+)
> 
> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> index 8896b70..574ffc4 100644
> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> @@ -6,6 +6,8 @@
>  
>  #include <common.h>
>  #include <fsl_ifc.h>
> +#include <ahci.h>
> +#include <scsi.h>
>  #include <asm/arch/soc.h>
>  #include <asm/io.h>
>  #include <asm/global_data.h>
> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
>  	erratum_a009203();
>  }
>  

Yuantian,

Please help me understand below.

> +#ifdef CONFIG_SCSI_AHCI_PLAT
> +int sata_init(void)
> +{
> +	struct ccsr_ahci __iomem *ccsr_ahci;
> +
> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);

You didn't set pp2c or pp3c. Is it because the default values are OK or
something else?

> +
> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA1;
> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> +
> +	ahci_init((void __iomem *)CONFIG_SYS_SATA1);

You only call ahci_init() here but not above. Is SATA2 active?


> +	scsi_scan(0);
> +
> +	return 0;
> +}
> +#endif
> +
>  #elif defined(CONFIG_LS1043A)
> +#ifdef CONFIG_SCSI_AHCI_PLAT
> +int sata_init(void)
> +{
> +	struct ccsr_ahci __iomem *ccsr_ahci = (void *)CONFIG_SYS_SATA;
> +
> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> +	out_le32(&ccsr_ahci->pp2c, AHCI_PORT_PHY_2_CFG);
> +	out_le32(&ccsr_ahci->pp3c, AHCI_PORT_PHY_3_CFG);
> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> +
> +	ahci_init((void __iomem *)CONFIG_SYS_SATA);
> +	scsi_scan(0);
> +
> +	return 0;
> +}
> +#endif
> +

York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Yuantian Tang]

Hi York,

Please see my explanation inline.

> -----Original Message-----
> From: York Sun [mailto:yorksun at freescale.com]
> Sent: Friday, December 04, 2015 12:27 AM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: u-boot at lists.denx.de; sinan at writeme.com
> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
> 
> 
> 
> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
> > From: Tang Yuantian <Yuantian.Tang@freescale.com>
> >
> > Freescale ARM-based Layerscape contains a SATA controller which comply
> > with the serial ATA 3.0 specification and the AHCI 1.3 specification.
> > This patch adds SATA feature on ls2080aqds, ls2080ardb and ls1043aqds
> > boards.
> >
> > Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> > ---
> > v5:
> > 	- re-organize the code
> > v4:
> > 	- rebase to lastest git tree
> > 	- add another ARMv8 platform which is ls1043aqds
> > v3:
> > 	- rename ls2085a to ls2080a
> > 	- rebase to the latest git tree
> > 	- replace the magic number with micro variable
> > v2:
> > 	- rebase to the latest git tree
> >
> >  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
> ++++++++++++++++++++++
> >  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
> >  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
> ++++++++++++++++
> >  include/configs/ls1043aqds.h                       | 17 +++++++++
> >  include/configs/ls2080aqds.h                       | 18 +++++++++
> >  include/configs/ls2080ardb.h                       | 18 +++++++++
> >  6 files changed, 131 insertions(+)
> >
> > diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > index 8896b70..574ffc4 100644
> > --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> > @@ -6,6 +6,8 @@
> >
> >  #include <common.h>
> >  #include <fsl_ifc.h>
> > +#include <ahci.h>
> > +#include <scsi.h>
> >  #include <asm/arch/soc.h>
> >  #include <asm/io.h>
> >  #include <asm/global_data.h>
> > @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
> >  	erratum_a009203();
> >  }
> >
> 
> Yuantian,
> 
> Please help me understand below.
> 
> > +#ifdef CONFIG_SCSI_AHCI_PLAT
> > +int sata_init(void)
> > +{
> > +	struct ccsr_ahci __iomem *ccsr_ahci;
> > +
> > +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
> > +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> > +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> 
> You didn't set pp2c or pp3c. Is it because the default values are OK or
> something else?
> 
Those settings of registers vary from soc to soc. If the default value will be used if the register is not updated explicitly.
Speaking of this, I got new information from validation team about giving a new value to PTC register.
So please hold this patch for a while, I will update it in next version. 

> > +
> > +	ccsr_ahci  = (void *)CONFIG_SYS_SATA1;
> > +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> > +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> > +
> > +	ahci_init((void __iomem *)CONFIG_SYS_SATA1);
> 
> You only call ahci_init() here but not above. Is SATA2 active?
> 
AHCI SATA driver only supports one SATA port. On ls2080a we have two ports, so we have to choice one. In this case I choice the first one which is SATA1.

Regards,
Yuantian

> 
> > +	scsi_scan(0);
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> >  #elif defined(CONFIG_LS1043A)
> > +#ifdef CONFIG_SCSI_AHCI_PLAT
> > +int sata_init(void)
> > +{
> > +	struct ccsr_ahci __iomem *ccsr_ahci = (void *)CONFIG_SYS_SATA;
> > +
> > +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> > +	out_le32(&ccsr_ahci->pp2c, AHCI_PORT_PHY_2_CFG);
> > +	out_le32(&ccsr_ahci->pp3c, AHCI_PORT_PHY_3_CFG);
> > +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> > +
> > +	ahci_init((void __iomem *)CONFIG_SYS_SATA);
> > +	scsi_scan(0);
> > +
> > +	return 0;
> > +}
> > +#endif
> > +
> 
> York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[York Sun]

On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
> Hi York,
> 
> Please see my explanation inline.
> 
>> -----Original Message-----
>> From: York Sun [mailto:yorksun at freescale.com]
>> Sent: Friday, December 04, 2015 12:27 AM
>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
>>
>>
>>
>> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
>>> From: Tang Yuantian <Yuantian.Tang@freescale.com>
>>>
>>> Freescale ARM-based Layerscape contains a SATA controller which comply
>>> with the serial ATA 3.0 specification and the AHCI 1.3 specification.
>>> This patch adds SATA feature on ls2080aqds, ls2080ardb and ls1043aqds
>>> boards.
>>>
>>> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
>>> ---
>>> v5:
>>> 	- re-organize the code
>>> v4:
>>> 	- rebase to lastest git tree
>>> 	- add another ARMv8 platform which is ls1043aqds
>>> v3:
>>> 	- rename ls2085a to ls2080a
>>> 	- rebase to the latest git tree
>>> 	- replace the magic number with micro variable
>>> v2:
>>> 	- rebase to the latest git tree
>>>
>>>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
>> ++++++++++++++++++++++
>>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
>> ++++++++++++++++
>>>  include/configs/ls1043aqds.h                       | 17 +++++++++
>>>  include/configs/ls2080aqds.h                       | 18 +++++++++
>>>  include/configs/ls2080ardb.h                       | 18 +++++++++
>>>  6 files changed, 131 insertions(+)
>>>
>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>> index 8896b70..574ffc4 100644
>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>> @@ -6,6 +6,8 @@
>>>
>>>  #include <common.h>
>>>  #include <fsl_ifc.h>
>>> +#include <ahci.h>
>>> +#include <scsi.h>
>>>  #include <asm/arch/soc.h>
>>>  #include <asm/io.h>
>>>  #include <asm/global_data.h>
>>> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
>>>  	erratum_a009203();
>>>  }
>>>
>>
>> Yuantian,
>>
>> Please help me understand below.
>>
>>> +#ifdef CONFIG_SCSI_AHCI_PLAT
>>> +int sata_init(void)
>>> +{
>>> +	struct ccsr_ahci __iomem *ccsr_ahci;
>>> +
>>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
>>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
>>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
>>
>> You didn't set pp2c or pp3c. Is it because the default values are OK or
>> something else?
>>
> Those settings of registers vary from soc to soc. If the default value will be used if the register is not updated explicitly.

If you put the macros for each SoC, you probably can use one function for all.
You only want to keep them separated if they have not much in common.

> Speaking of this, I got new information from validation team about giving a new value to PTC register.
> So please hold this patch for a while, I will update it in next version. 
> 
>>> +
>>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA1;
>>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
>>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
>>> +
>>> +	ahci_init((void __iomem *)CONFIG_SYS_SATA1);
>>
>> You only call ahci_init() here but not above. Is SATA2 active?
>>
> AHCI SATA driver only supports one SATA port. On ls2080a we have two ports, so we have to choice one. In this case I choice the first one which is SATA1.

This should be put into comment, or README if you have one.

York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Yuantian Tang]

Hi York,

Please see explanation inline.

> -----Original Message-----
> From: York Sun [mailto:yorksun at freescale.com]
> Sent: Saturday, December 05, 2015 1:25 AM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: u-boot at lists.denx.de; sinan at writeme.com
> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
> 
> 
> 
> On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
> > Hi York,
> >
> > Please see my explanation inline.
> >
> >> -----Original Message-----
> >> From: York Sun [mailto:yorksun at freescale.com]
> >> Sent: Friday, December 04, 2015 12:27 AM
> >> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> >> Cc: u-boot at lists.denx.de; sinan at writeme.com
> >> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
> >> board
> >>
> >>
> >>
> >> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
> >>> From: Tang Yuantian <Yuantian.Tang@freescale.com>
> >>>
> >>> Freescale ARM-based Layerscape contains a SATA controller which
> >>> comply with the serial ATA 3.0 specification and the AHCI 1.3
> specification.
> >>> This patch adds SATA feature on ls2080aqds, ls2080ardb and
> >>> ls1043aqds boards.
> >>>
> >>> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> >>> ---
> >>> v5:
> >>> 	- re-organize the code
> >>> v4:
> >>> 	- rebase to lastest git tree
> >>> 	- add another ARMv8 platform which is ls1043aqds
> >>> v3:
> >>> 	- rename ls2085a to ls2080a
> >>> 	- rebase to the latest git tree
> >>> 	- replace the magic number with micro variable
> >>> v2:
> >>> 	- rebase to the latest git tree
> >>>
> >>>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
> >> ++++++++++++++++++++++
> >>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
> >>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
> >> ++++++++++++++++
> >>>  include/configs/ls1043aqds.h                       | 17 +++++++++
> >>>  include/configs/ls2080aqds.h                       | 18 +++++++++
> >>>  include/configs/ls2080ardb.h                       | 18 +++++++++
> >>>  6 files changed, 131 insertions(+)
> >>>
> >>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>> index 8896b70..574ffc4 100644
> >>> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>> @@ -6,6 +6,8 @@
> >>>
> >>>  #include <common.h>
> >>>  #include <fsl_ifc.h>
> >>> +#include <ahci.h>
> >>> +#include <scsi.h>
> >>>  #include <asm/arch/soc.h>
> >>>  #include <asm/io.h>
> >>>  #include <asm/global_data.h>
> >>> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
> >>>  	erratum_a009203();
> >>>  }
> >>>
> >>
> >> Yuantian,
> >>
> >> Please help me understand below.
> >>
> >>> +#ifdef CONFIG_SCSI_AHCI_PLAT
> >>> +int sata_init(void)
> >>> +{
> >>> +	struct ccsr_ahci __iomem *ccsr_ahci;
> >>> +
> >>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
> >>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> >>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> >>
> >> You didn't set pp2c or pp3c. Is it because the default values are OK
> >> or something else?
> >>
> > Those settings of registers vary from soc to soc. If the default value will be
> used if the register is not updated explicitly.
> 
> If you put the macros for each SoC, you probably can use one function for all.
> You only want to keep them separated if they have not much in common.
> 
I was trying to use one function for all, but I found separating them is better.
Take ls1043a and ls2080a as an example, ls2080a has two controllers, while ls1043a has one.
Ls2080a has two registers that need to be updated while ls1043a has four.
A lot of #ifdef are needed if we unify them, not mention that in the future, changing one of the platforms' register will affect the other.
Maybe I am not thinking it through.  If you can give me more detail that viable, I can give a try.

> > Speaking of this, I got new information from validation team about giving a
> new value to PTC register.
> > So please hold this patch for a while, I will update it in next version.
> >
> >>> +
> >>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA1;
> >>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> >>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> >>> +
> >>> +	ahci_init((void __iomem *)CONFIG_SYS_SATA1);
> >>
> >> You only call ahci_init() here but not above. Is SATA2 active?
> >>
> > AHCI SATA driver only supports one SATA port. On ls2080a we have two
> ports, so we have to choice one. In this case I choice the first one which is
> SATA1.
> 
> This should be put into comment, or README if you have one.
This phenomenon is not LS platform specific, that's uboot's issue which needs another patch to fix.
I think uboot know that and choice to not fix it because for uboot supporting two sata port is not that significant.
It is just like that uboot doesn't support local sata and PCIe sata simutaniously.

If a comment is needed, it would be better to put it in our own README (in this case, ls2080a) document. 

Regards,
Yuantian
> 
> York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Sinan Akman]

   Hi Yuantian

On 06/12/15 10:09 PM, Yuantian Tang wrote:
> Hi York,
>
> Please see explanation inline.
> [...]
> I was trying to use one function for all, but I found separating them is better.
> Take ls1043a and ls2080a as an example, ls2080a has two controllers, while ls1043a has one.
> Ls2080a has two registers that need to be updated while ls1043a has four.
> A lot of #ifdef are needed if we unify them, not mention that in the future, changing one of the platforms' register will affect the other.

    You might want to take into consideration that in the near future we 
will be moving
this to dm. In that respect having all that in one file already will 
probably make things
much easier. If you consider this, perhaps you will have a different view.

> Maybe I am not thinking it through.  If you can give me more detail that viable, I can give a try.
>
>> [...]
>> ports, so we have to choice one. In this case I choice the first one which is
>> SATA1.
>>
>> This should be put into comment, or README if you have one.
> This phenomenon is not LS platform specific, that's uboot's issue which needs another patch to fix.
> I think uboot know that and choice to not fix it because for uboot supporting two sata port is not that significant.

   Again, with dm and reading all the hardware properties from device 
tree will
also change this. If both device nodes are enabled we will have to 
support both
as long as there is no hardware limitation. So I think there is no 
reason why
having both SATA and PCIe would not be significant. It is just that the 
current
implementation has this limitation and there is already some timeline 
for removing
these limitations.

   Regards
   Sinan Akman

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Yuantian Tang]

Hi Sinan,

> -----Original Message-----
> From: Sinan Akman [mailto:sinan at writeme.com]
> Sent: Monday, December 07, 2015 2:04 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>; Sun York-R58495
> <yorksun@freescale.com>
> Cc: u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH v5] arm: Add sata support on Layerscape
> ARMv8 board
> 
> 
>    Hi Yuantian
> 
> On 06/12/15 10:09 PM, Yuantian Tang wrote:
> > Hi York,
> >
> > Please see explanation inline.
> > [...]
> > I was trying to use one function for all, but I found separating them is
> better.
> > Take ls1043a and ls2080a as an example, ls2080a has two controllers, while
> ls1043a has one.
> > Ls2080a has two registers that need to be updated while ls1043a has four.
> > A lot of #ifdef are needed if we unify them, not mention that in the future,
> changing one of the platforms' register will affect the other.
> 
>     You might want to take into consideration that in the near future we will be
> moving this to dm. In that respect having all that in one file already will
> probably make things much easier. If you consider this, perhaps you will have
> a different view.
> 
They are in the same file but different functions.

> > Maybe I am not thinking it through.  If you can give me more detail that
> viable, I can give a try.
> >
> >> [...]
> >> ports, so we have to choice one. In this case I choice the first one
> >> which is SATA1.
> >>
> >> This should be put into comment, or README if you have one.
> > This phenomenon is not LS platform specific, that's uboot's issue which
> needs another patch to fix.
> > I think uboot know that and choice to not fix it because for uboot
> supporting two sata port is not that significant.
> 
>    Again, with dm and reading all the hardware properties from device tree
> will also change this. If both device nodes are enabled we will have to
> support both as long as there is no hardware limitation. So I think there is no
> reason why having both SATA and PCIe would not be significant. It is just that
> the current implementation has this limitation and there is already some
> timeline for removing these limitations.
> 
I am not seeing what we are arguing here? 
Are we talking about if this limitation is important?
Please point out what's wrong with this patch.

Regards,
Yuantian

>    Regards
>    Sinan Akman

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Sinan Akman]

   Hi Yuantian

On 07/12/15 02:04 AM, Yuantian Tang wrote:
> [...]
> They are in the same file but different functions.

     OK

>>> I think uboot know that and choice to not fix it because for uboot
>> supporting two sata port is not that significant.
>> [...]
>> Please point out what's wrong with this patch.
        I only expressed my opinion on your view for u-boot's assumption
on supporting two ports. Please don't take it personal.

        Regards
        Sinan Akman

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[York Sun]

On 12/06/2015 07:09 PM, Tang Yuantian-B29983 wrote:
> Hi York,
> 
> Please see explanation inline.
> 
>> -----Original Message-----
>> From: York Sun [mailto:yorksun at freescale.com]
>> Sent: Saturday, December 05, 2015 1:25 AM
>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
>>
>>
>>
>> On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
>>> Hi York,
>>>
>>> Please see my explanation inline.
>>>
>>>> -----Original Message-----
>>>> From: York Sun [mailto:yorksun at freescale.com]
>>>> Sent: Friday, December 04, 2015 12:27 AM
>>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
>>>> board
>>>>
>>>>
>>>>
>>>> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
>>>>> From: Tang Yuantian <Yuantian.Tang@freescale.com>
>>>>>
>>>>> Freescale ARM-based Layerscape contains a SATA controller which
>>>>> comply with the serial ATA 3.0 specification and the AHCI 1.3
>> specification.
>>>>> This patch adds SATA feature on ls2080aqds, ls2080ardb and
>>>>> ls1043aqds boards.
>>>>>
>>>>> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
>>>>> ---
>>>>> v5:
>>>>> 	- re-organize the code
>>>>> v4:
>>>>> 	- rebase to lastest git tree
>>>>> 	- add another ARMv8 platform which is ls1043aqds
>>>>> v3:
>>>>> 	- rename ls2085a to ls2080a
>>>>> 	- rebase to the latest git tree
>>>>> 	- replace the magic number with micro variable
>>>>> v2:
>>>>> 	- rebase to the latest git tree
>>>>>
>>>>>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
>>>> ++++++++++++++++++++++
>>>>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>>>>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
>>>> ++++++++++++++++
>>>>>  include/configs/ls1043aqds.h                       | 17 +++++++++
>>>>>  include/configs/ls2080aqds.h                       | 18 +++++++++
>>>>>  include/configs/ls2080ardb.h                       | 18 +++++++++
>>>>>  6 files changed, 131 insertions(+)
>>>>>
>>>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>> index 8896b70..574ffc4 100644
>>>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>> @@ -6,6 +6,8 @@
>>>>>
>>>>>  #include <common.h>
>>>>>  #include <fsl_ifc.h>
>>>>> +#include <ahci.h>
>>>>> +#include <scsi.h>
>>>>>  #include <asm/arch/soc.h>
>>>>>  #include <asm/io.h>
>>>>>  #include <asm/global_data.h>
>>>>> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
>>>>>  	erratum_a009203();
>>>>>  }
>>>>>
>>>>
>>>> Yuantian,
>>>>
>>>> Please help me understand below.
>>>>
>>>>> +#ifdef CONFIG_SCSI_AHCI_PLAT
>>>>> +int sata_init(void)
>>>>> +{
>>>>> +	struct ccsr_ahci __iomem *ccsr_ahci;
>>>>> +
>>>>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
>>>>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
>>>>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
>>>>
>>>> You didn't set pp2c or pp3c. Is it because the default values are OK
>>>> or something else?
>>>>
>>> Those settings of registers vary from soc to soc. If the default value will be
>> used if the register is not updated explicitly.
>>
>> If you put the macros for each SoC, you probably can use one function for all.
>> You only want to keep them separated if they have not much in common.
>>
> I was trying to use one function for all, but I found separating them is better.
> Take ls1043a and ls2080a as an example, ls2080a has two controllers, while ls1043a has one.
> Ls2080a has two registers that need to be updated while ls1043a has four.
> A lot of #ifdef are needed if we unify them, not mention that in the future, changing one of the platforms' register will affect the other.
> Maybe I am not thinking it through.  If you can give me more detail that viable, I can give a try.

Yuantian,

I was thinking to set all registers, including those with default values. Then
you can use one function for both. My understand is LS1043 and LS2080 has
different default value. It will be easier to update the macros if you need
different values, than changing the functions. If we have a new SoC in the same
family, you don't have to add another function.

Try it to see if you still have to separate them.

York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[Yuantian Tang]

Hi York,

> -----Original Message-----
> From: York Sun [mailto:yorksun at freescale.com]
> Sent: Tuesday, December 08, 2015 12:27 AM
> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> Cc: u-boot at lists.denx.de; sinan at writeme.com
> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
> 
> 
> 
> On 12/06/2015 07:09 PM, Tang Yuantian-B29983 wrote:
> > Hi York,
> >
> > Please see explanation inline.
> >
> >> -----Original Message-----
> >> From: York Sun [mailto:yorksun at freescale.com]
> >> Sent: Saturday, December 05, 2015 1:25 AM
> >> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> >> Cc: u-boot at lists.denx.de; sinan at writeme.com
> >> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
> >> board
> >>
> >>
> >>
> >> On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
> >>> Hi York,
> >>>
> >>> Please see my explanation inline.
> >>>
> >>>> -----Original Message-----
> >>>> From: York Sun [mailto:yorksun at freescale.com]
> >>>> Sent: Friday, December 04, 2015 12:27 AM
> >>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
> >>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
> >>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
> >>>> board
> >>>>
> >>>>
> >>>>
> >>>> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
> >>>>> From: Tang Yuantian <Yuantian.Tang@freescale.com>
> >>>>>
> >>>>> Freescale ARM-based Layerscape contains a SATA controller which
> >>>>> comply with the serial ATA 3.0 specification and the AHCI 1.3
> >> specification.
> >>>>> This patch adds SATA feature on ls2080aqds, ls2080ardb and
> >>>>> ls1043aqds boards.
> >>>>>
> >>>>> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
> >>>>> ---
> >>>>> v5:
> >>>>> 	- re-organize the code
> >>>>> v4:
> >>>>> 	- rebase to lastest git tree
> >>>>> 	- add another ARMv8 platform which is ls1043aqds
> >>>>> v3:
> >>>>> 	- rename ls2085a to ls2080a
> >>>>> 	- rebase to the latest git tree
> >>>>> 	- replace the magic number with micro variable
> >>>>> v2:
> >>>>> 	- rebase to the latest git tree
> >>>>>
> >>>>>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
> >>>> ++++++++++++++++++++++
> >>>>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
> >>>>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
> >>>> ++++++++++++++++
> >>>>>  include/configs/ls1043aqds.h                       | 17 +++++++++
> >>>>>  include/configs/ls2080aqds.h                       | 18 +++++++++
> >>>>>  include/configs/ls2080ardb.h                       | 18 +++++++++
> >>>>>  6 files changed, 131 insertions(+)
> >>>>>
> >>>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>>>> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>>>> index 8896b70..574ffc4 100644
> >>>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
> >>>>> @@ -6,6 +6,8 @@
> >>>>>
> >>>>>  #include <common.h>
> >>>>>  #include <fsl_ifc.h>
> >>>>> +#include <ahci.h>
> >>>>> +#include <scsi.h>
> >>>>>  #include <asm/arch/soc.h>
> >>>>>  #include <asm/io.h>
> >>>>>  #include <asm/global_data.h>
> >>>>> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
> >>>>>  	erratum_a009203();
> >>>>>  }
> >>>>>
> >>>>
> >>>> Yuantian,
> >>>>
> >>>> Please help me understand below.
> >>>>
> >>>>> +#ifdef CONFIG_SCSI_AHCI_PLAT
> >>>>> +int sata_init(void)
> >>>>> +{
> >>>>> +	struct ccsr_ahci __iomem *ccsr_ahci;
> >>>>> +
> >>>>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
> >>>>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
> >>>>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
> >>>>
> >>>> You didn't set pp2c or pp3c. Is it because the default values are
> >>>> OK or something else?
> >>>>
> >>> Those settings of registers vary from soc to soc. If the default
> >>> value will be
> >> used if the register is not updated explicitly.
> >>
> >> If you put the macros for each SoC, you probably can use one function for
> all.
> >> You only want to keep them separated if they have not much in common.
> >>
> > I was trying to use one function for all, but I found separating them is
> better.
> > Take ls1043a and ls2080a as an example, ls2080a has two controllers, while
> ls1043a has one.
> > Ls2080a has two registers that need to be updated while ls1043a has four.
> > A lot of #ifdef are needed if we unify them, not mention that in the future,
> changing one of the platforms' register will affect the other.
> > Maybe I am not thinking it through.  If you can give me more detail that
> viable, I can give a try.
> 
> Yuantian,
> 
> I was thinking to set all registers, including those with default values. Then
> you can use one function for both. My understand is LS1043 and LS2080 has
> different default value. It will be easier to update the macros if you need
> different values, than changing the functions. If we have a new SoC in the
> same family, you don't have to add another function.
> 
> Try it to see if you still have to separate them.
> 
I didn't see any benefit this way. 
We have 20 registers to set for each soc in this way. In order to use one function, we have to define 20 micro for each soc too.
If the soc's version is upgraded, we may have to redefine the value if the default value is changed.
All what we do is just to make it in one function. 
Currently, we just need to set the register that requires to change which only have 4 and 2 registers respectively.

I thank your suggestion complicates things.

Regards,
Yuantian
> York

[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[York Sun]

On 12/07/2015 07:04 PM, Tang Yuantian-B29983 wrote:
> Hi York,
> 
>> -----Original Message-----
>> From: York Sun [mailto:yorksun at freescale.com]
>> Sent: Tuesday, December 08, 2015 12:27 AM
>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
>>
>>
>>
>> On 12/06/2015 07:09 PM, Tang Yuantian-B29983 wrote:
>>> Hi York,
>>>
>>> Please see explanation inline.
>>>
>>>> -----Original Message-----
>>>> From: York Sun [mailto:yorksun at freescale.com]
>>>> Sent: Saturday, December 05, 2015 1:25 AM
>>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
>>>> board
>>>>
>>>>
>>>>
>>>> On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
>>>>> Hi York,
>>>>>
>>>>> Please see my explanation inline.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: York Sun [mailto:yorksun at freescale.com]
>>>>>> Sent: Friday, December 04, 2015 12:27 AM
>>>>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>>>>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>>>>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
>>>>>> board
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/01/2015 07:27 PM, Yuantian.Tang at freescale.com wrote:
>>>>>>> From: Tang Yuantian <Yuantian.Tang@freescale.com>
>>>>>>>
>>>>>>> Freescale ARM-based Layerscape contains a SATA controller which
>>>>>>> comply with the serial ATA 3.0 specification and the AHCI 1.3
>>>> specification.
>>>>>>> This patch adds SATA feature on ls2080aqds, ls2080ardb and
>>>>>>> ls1043aqds boards.
>>>>>>>
>>>>>>> Signed-off-by: Tang Yuantian <Yuantian.Tang@freescale.com>
>>>>>>> ---
>>>>>>> v5:
>>>>>>> 	- re-organize the code
>>>>>>> v4:
>>>>>>> 	- rebase to lastest git tree
>>>>>>> 	- add another ARMv8 platform which is ls1043aqds
>>>>>>> v3:
>>>>>>> 	- rename ls2085a to ls2080a
>>>>>>> 	- rebase to the latest git tree
>>>>>>> 	- replace the magic number with micro variable
>>>>>>> v2:
>>>>>>> 	- rebase to the latest git tree
>>>>>>>
>>>>>>>  arch/arm/cpu/armv8/fsl-layerscape/soc.c            | 43
>>>>>> ++++++++++++++++++++++
>>>>>>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>>>>>>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
>>>>>> ++++++++++++++++
>>>>>>>  include/configs/ls1043aqds.h                       | 17 +++++++++
>>>>>>>  include/configs/ls2080aqds.h                       | 18 +++++++++
>>>>>>>  include/configs/ls2080ardb.h                       | 18 +++++++++
>>>>>>>  6 files changed, 131 insertions(+)
>>>>>>>
>>>>>>> diff --git a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>>>> b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>>>> index 8896b70..574ffc4 100644
>>>>>>> --- a/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>>>> +++ b/arch/arm/cpu/armv8/fsl-layerscape/soc.c
>>>>>>> @@ -6,6 +6,8 @@
>>>>>>>
>>>>>>>  #include <common.h>
>>>>>>>  #include <fsl_ifc.h>
>>>>>>> +#include <ahci.h>
>>>>>>> +#include <scsi.h>
>>>>>>>  #include <asm/arch/soc.h>
>>>>>>>  #include <asm/io.h>
>>>>>>>  #include <asm/global_data.h>
>>>>>>> @@ -120,7 +122,44 @@ void fsl_lsch3_early_init_f(void)
>>>>>>>  	erratum_a009203();
>>>>>>>  }
>>>>>>>
>>>>>>
>>>>>> Yuantian,
>>>>>>
>>>>>> Please help me understand below.
>>>>>>
>>>>>>> +#ifdef CONFIG_SCSI_AHCI_PLAT
>>>>>>> +int sata_init(void)
>>>>>>> +{
>>>>>>> +	struct ccsr_ahci __iomem *ccsr_ahci;
>>>>>>> +
>>>>>>> +	ccsr_ahci  = (void *)CONFIG_SYS_SATA2;
>>>>>>> +	out_le32(&ccsr_ahci->ppcfg, AHCI_PORT_PHY_1_CFG);
>>>>>>> +	out_le32(&ccsr_ahci->ptc, AHCI_PORT_TRANS_CFG);
>>>>>>
>>>>>> You didn't set pp2c or pp3c. Is it because the default values are
>>>>>> OK or something else?
>>>>>>
>>>>> Those settings of registers vary from soc to soc. If the default
>>>>> value will be
>>>> used if the register is not updated explicitly.
>>>>
>>>> If you put the macros for each SoC, you probably can use one function for
>> all.
>>>> You only want to keep them separated if they have not much in common.
>>>>
>>> I was trying to use one function for all, but I found separating them is
>> better.
>>> Take ls1043a and ls2080a as an example, ls2080a has two controllers, while
>> ls1043a has one.
>>> Ls2080a has two registers that need to be updated while ls1043a has four.
>>> A lot of #ifdef are needed if we unify them, not mention that in the future,
>> changing one of the platforms' register will affect the other.
>>> Maybe I am not thinking it through.  If you can give me more detail that
>> viable, I can give a try.
>>
>> Yuantian,
>>
>> I was thinking to set all registers, including those with default values. Then
>> you can use one function for both. My understand is LS1043 and LS2080 has
>> different default value. It will be easier to update the macros if you need
>> different values, than changing the functions. If we have a new SoC in the
>> same family, you don't have to add another function.
>>
>> Try it to see if you still have to separate them.
>>
> I didn't see any benefit this way. 
> We have 20 registers to set for each soc in this way. In order to use one function, we have to define 20 micro for each soc too.

20 registers? I didn't see that coming. In that case, you can keep it your way.

York