[U-Boot] [PATCH v4] arm: Add sata support on Layerscape ARMv8 board
York Sun
yorksun at freescale.com
Tue Dec 1 17:44:01 CET 2015
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 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.
> 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 at lists.denx.de
>> http://lists.denx.de/mailman/listinfo/u-boot
>>
>
More information about the U-Boot
mailing list