[U-Boot] [PATCH v4] arm: Add sata support on Layerscape ARMv8 board
Yuantian Tang
Yuantian.Tang at freescale.com
Tue Dec 1 09:36:22 CET 2015
Hi Sinan Akman,
Please see my explanation inline.
> -----Original Message-----
> From: Sinan Akman [mailto:sinan at writeme.com]
> Sent: Tuesday, December 01, 2015 3:24 PM
> To: Tang Yuantian-B29983 <Yuantian.Tang at freescale.com>; Sun York-R58495
> <yorksun at freescale.com>
> Cc: u-boot at 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:sinan at writeme.com]
> >> Sent: Tuesday, December 01, 2015 2:29 PM
> >> To: Tang Yuantian-B29983 <Yuantian.Tang at freescale.com>; Sun
> >> York-R58495 <yorksun at freescale.com>
> >> Cc: u-boot at 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:sinan at writeme.com]
> >>>> Sent: Tuesday, December 01, 2015 1:28 AM
> >>>> To: Tang Yuantian-B29983 <Yuantian.Tang at freescale.com>; Sun
> >>>> York-R58495 <yorksun at freescale.com>
> >>>> Cc: u-boot at 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 at freescale.com wrote:
> >>>>> From: Tang Yuantian <Yuantian.Tang at 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 at 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
> >>>>> */
> >>>>>
> >>>>>
More information about the U-Boot
mailing list