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. 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:sinan@writeme.com] Sent: Tuesday, December 01, 2015 3:24 PM To: Tang Yuantian-B29983 Yuantian.Tang@freescale.com; Sun York-R58495 yorksun@freescale.com Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v4] arm: Add sata support on Layerscape ARMv8 board
Hi YuanOn 01/12/15 01:41 AM, Yuantian Tang wrote:
Hi Sinan Akman,
-----Original Message----- From: Sinan Akman [mailto:sinan@writeme.com] Sent: Tuesday, December 01, 2015 2:29 PM To: Tang Yuantian-B29983 Yuantian.Tang@freescale.com; Sun York-R58495 yorksun@freescale.com Cc: u-boot@lists.denx.de Subject: Re: [U-Boot] [PATCH v4] arm: Add sata support on Layerscape ARMv8 board
Hi YuanOn 01/12/15 01:16 AM, Yuantian Tang wrote:
Hi Sinan Akman,
Please see my explanation inline.
> -----Original Message----- > From: Sinan Akman [mailto:sinan@writeme.com] > Sent: Tuesday, December 01, 2015 1:28 AM > To: Tang Yuantian-B29983 Yuantian.Tang@freescale.com; Sun > York-R58495 yorksun@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.Tang@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 >> --- >> 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 notcan 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_SATAwhile 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 thatfile. 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 underSoC 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 anexample.
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 AkmanRegards, Yuantian
Regards Sinan AkmanIt 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