Hi York
On 01/12/15 11:44 AM, York Sun wrote:
Sinan,
Thanks for your review.
Yuantian,
You are right about putting things together. We have some macros in wrong
places, including config.h. If you can, please try to move them into proper
files, such as immap_lsch3.h. Enabling SATA should be done at board level
because it depends on the board to have physical connection.
Just to make this clear one final time, I am not against enabling
SATA at the board level but
my suggestion is to define the SATA register values (which is *not*
board dependent) in the
soc specific header file, then at the board level if SATA is not enabled
we don't use it, if it is
enabled we use it and use the same register values across all boards
with this SoC. That's all.
Regards
Sinan Akman
The features to
enable file system and commands can stay in board file as well.
York
On 12/01/2015 06:46 AM, Sinan Akman wrote:
Hi Yuan
On 01/12/15 03:36 AM, Yuantian Tang wrote:
Hi Sinan Akman,
Please see my explanation inline.
-----Original Message-----
From: Sinan Akman [mailto:si...@writeme.com]
Sent: Tuesday, December 01, 2015 3:24 PM
To: Tang Yuantian-B29983 <yuantian.t...@freescale.com>; Sun York-R58495
<york...@freescale.com>
Cc: u-boot@lists.denx.de
Subject: Re: [U-Boot] [PATCH v4] arm: Add sata support on Layerscape
ARMv8 board
Hi Yuan
On 01/12/15 01:41 AM, Yuantian Tang wrote:
Hi Sinan Akman,
-----Original Message-----
From: Sinan Akman [mailto:si...@writeme.com]
Sent: Tuesday, December 01, 2015 2:29 PM
To: Tang Yuantian-B29983 <yuantian.t...@freescale.com>; Sun
York-R58495 <york...@freescale.com>
Cc: u-boot@lists.denx.de
Subject: Re: [U-Boot] [PATCH v4] arm: Add sata support on Layerscape
ARMv8 board
Hi Yuan
On 01/12/15 01:16 AM, Yuantian Tang wrote:
Hi Sinan Akman,
Please see my explanation inline.
-----Original Message-----
From: Sinan Akman [mailto:si...@writeme.com]
Sent: Tuesday, December 01, 2015 1:28 AM
To: Tang Yuantian-B29983 <yuantian.t...@freescale.com>; Sun
York-R58495 <york...@freescale.com>
Cc: u-boot@lists.denx.de
Subject: Re: [U-Boot] [PATCH v4] arm: Add sata support on
Layerscape
ARMv8 board
Hi Yuan
On 30/11/15 02:44 AM, yuantian.t...@freescale.com wrote:
From: Tang Yuantian <yuantian.t...@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.t...@freescale.com>
---
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
+++++++++++++++++++++++
arch/arm/include/asm/arch-fsl-layerscape/config.h | 18
++++++++++
arch/arm/include/asm/arch-fsl-layerscape/soc.h | 31
++++++++++++++++
include/configs/ls1043aqds.h | 17 +++++++++
4 files changed, 109 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/config.h
b/arch/arm/include/asm/arch-fsl-layerscape/config.h
index b5a2d28..be3acc3 100644
--- a/arch/arm/include/asm/arch-fsl-layerscape/config.h
+++ b/arch/arm/include/asm/arch-fsl-layerscape/config.h
@@ -54,6 +54,24 @@
#define CONFIG_SYS_MEMAC_LITTLE_ENDIAN
+/* 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 (CONFIG_SYS_IMMR
+ 0x02200000)
+#define CONFIG_SYS_SATA2 (CONFIG_SYS_IMMR
+ 0x02210000)
Why do we have CONFIG_SYS_SATA1 and CONFIG_SYS_SATA2 here
and
then CONFIG_SYS_SATA in another file (see later below)?
CONFIG_SYS_SATA1 and CONFIG_SYS_SATA seem to have the same
macro
value : (CONFIG_SYS_IMMR + 0x02200000)
normally we put all those definitions in board specific head file.
but config.h is created for all layerscape ARMv8 board too which
include
ls2080a and ls1043a.
So I add those definitions here for ls2080a to avoid defining them
in every
ls2080a board head file.
For ls1043a, only ls1043aqds supports SATA, so, I put all those
definitions in
board head file which is the normal way.
What is the real reason ls1043aqds is not using that config.h ?
Config.h is used for both ls1043aqds and ls1043ardb. Since ls1043ardb
doesn't support SATA, it's better to not use it.
What I don't feel comfortable here is whether there is SATA support or not
can be board specific but the SATA register addresses are not. They should
be in SoC or architecture specific include files. With your method if someone
designs a new board with ls1043 and wants to have SATA support this
address has to be repeated in board config file again. I suggest we keep a
uniform approach. If config.h is not suitable for
ls1043 SoC then we should have another file defining SATA regs for ls1043.
Each board can then decide to initialize SATA based on a config value (e.g.
CONFIG_FSL_SATA) for SATA
support which then can be included in the board specific config file in
include/configs.
Take a look at T102xQDS.h vs T102xRDB.h as an example. Both could refer to
CONFIG_SYS_MPC85xx_SATA1_ADDR but only T102xQDS is using SATA. So
ls1043aqds vs ls1043rdb should be modeled similar to this. Does this make
sense ?
You may mix the SATA feature and SATA register.
No I am mixing the two.
Whether T102xRDB supports SATA is not decided by CONFIG_SYS_MPC85xx_SATA1_ADDR.
I am aware of this and therefore I referred to CONFIG_FSL_SATA
while explaining above,
please reread it if you prefer.
For ls1043a, the best place for CONFIG_SYS_SATA definition is at: immap_lsch2.h.
But I want to put all the SATA related definitions together. That's why I move
it to config.h
And I can see yet you didn't put SATA definition for ls1043 in that
file. If you did this I'd see no
problem. I agree all the SATA related definitions should be together and
in this case it is not.
For the rest of the micro definition which is to enable SATA feature, they
should be placed in config.h or ls1043axxx.h. I think either file is acceptable.
I think the very deciding factor here is, config.h which is under
SoC or architecture specific
directory should contain SoC or architecture specific definitions. The
SATA registers addresses
are SoC specific so they are expected to be there. On the other hand,
e.g ls1043axxx.h or
any other board specific config files should have board specific config
definitions. Otherwise
why would we have two separate files. So the fact that <boardname>.h
should have particular
definitions for this board only and SATA register addresses are not
board registers make your
approach not correct. It might be acceptable but not the cleanest thing
to do.
If a new ls1043a board with SATA support, those micro should be defined like
what we did on ls1043aqds.
As explained this is not what I see in the other boards I gave as an
example.
It is not necessary to create a common file only for SATA since putting those
SATA definition in board head file is a common way and no any drawback.
This is exactly my point that having CONFIG_SYS_IMMR + 0x02200000 in
every board
specific file is error prone and not the correct thing to do. You could
have e.g
#define CONFIG_SYS_SATA CONFIG_SYS_SATA1
in a <board_name>.h file and use that CONFIG_SYS_SATA1
file from a common header file which is defined specific to
SoC or architecture.
Yuan, I think I explained my points several times here clearly but
it seems that you already made up your mind and you don't see
any issue having SATA register values unnecessarily repeated for
each board that supports SATA.
I don't agree with this but I see no point of discussing any further.
You can go ahead and implement it the way you prefer.
Regards
Sinan Akman
Regards,
Yuantian
Regards
Sinan Akman
Regards,
Yuantian
Regards
Sinan Akman
It may lead to a little confusion, but I think it is acceptable.
Regards,
Yuantian
+
+#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)
+
/* Generic Interrupt Controller Definitions */
#define GICD_BASE 0x06000000
#define GICR_BASE 0x06100000
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..bd7af00 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
(CONFIG_SYS_IMMR
+ 0x02200000)
I think this is becoming ad-hoc. Should we not have all these
SYS_SATA definitions in the include files both for 2080a as well as
ls1043 instead of throwing them in board specific config file.
Is this something changes for each board ?
+
+#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
*/
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
