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. Whether T102xRDB supports SATA is not decided by CONFIG_SYS_MPC85xx_SATA1_ADDR. 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 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. If a new ls1043a board with SATA support, those micro should be defined like what we did on ls1043aqds. 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. 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
