[U-Boot] [PATCH v5] arm: Add sata support on Layerscape ARMv8 board

[York Sun <york.sun@nxp.com>]

On 12/07/2015 07:04 PM, Tang Yuantian-B29983 wrote:
> Hi York,
> 
>> -----Original Message-----
>> From: York Sun [mailto:yorksun at freescale.com]
>> Sent: Tuesday, December 08, 2015 12:27 AM
>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8 board
>>
>>
>>
>> On 12/06/2015 07:09 PM, Tang Yuantian-B29983 wrote:
>>> Hi York,
>>>
>>> Please see explanation inline.
>>>
>>>> -----Original Message-----
>>>> From: York Sun [mailto:yorksun at freescale.com]
>>>> Sent: Saturday, December 05, 2015 1:25 AM
>>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
>>>> board
>>>>
>>>>
>>>>
>>>> On 12/03/2015 06:47 PM, Tang Yuantian-B29983 wrote:
>>>>> Hi York,
>>>>>
>>>>> Please see my explanation inline.
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: York Sun [mailto:yorksun at freescale.com]
>>>>>> Sent: Friday, December 04, 2015 12:27 AM
>>>>>> To: Tang Yuantian-B29983 <Yuantian.Tang@freescale.com>
>>>>>> Cc: u-boot at lists.denx.de; sinan at writeme.com
>>>>>> Subject: Re: [PATCH v5] arm: Add sata support on Layerscape ARMv8
>>>>>> board
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 12/01/2015 07:27 PM, Yuantian.Tang at 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>
>>>>>>> ---
>>>>>>> v5:
>>>>>>> 	- re-organize the code
>>>>>>> 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
>>>>>> ++++++++++++++++++++++
>>>>>>>  .../include/asm/arch-fsl-layerscape/immap_lsch3.h  |  4 ++
>>>>>>>  arch/arm/include/asm/arch-fsl-layerscape/soc.h     | 31
>>>>>> ++++++++++++++++
>>>>>>>  include/configs/ls1043aqds.h                       | 17 +++++++++
>>>>>>>  include/configs/ls2080aqds.h                       | 18 +++++++++
>>>>>>>  include/configs/ls2080ardb.h                       | 18 +++++++++
>>>>>>>  6 files changed, 131 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();
>>>>>>>  }
>>>>>>>
>>>>>>
>>>>>> Yuantian,
>>>>>>
>>>>>> Please help me understand below.
>>>>>>
>>>>>>> +#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);
>>>>>>
>>>>>> You didn't set pp2c or pp3c. Is it because the default values are
>>>>>> OK or something else?
>>>>>>
>>>>> Those settings of registers vary from soc to soc. If the default
>>>>> value will be
>>>> used if the register is not updated explicitly.
>>>>
>>>> If you put the macros for each SoC, you probably can use one function for
>> all.
>>>> You only want to keep them separated if they have not much in common.
>>>>
>>> I was trying to use one function for all, but I found separating them is
>> better.
>>> Take ls1043a and ls2080a as an example, ls2080a has two controllers, while
>> ls1043a has one.
>>> Ls2080a has two registers that need to be updated while ls1043a has four.
>>> A lot of #ifdef are needed if we unify them, not mention that in the future,
>> changing one of the platforms' register will affect the other.
>>> Maybe I am not thinking it through.  If you can give me more detail that
>> viable, I can give a try.
>>
>> Yuantian,
>>
>> I was thinking to set all registers, including those with default values. Then
>> you can use one function for both. My understand is LS1043 and LS2080 has
>> different default value. It will be easier to update the macros if you need
>> different values, than changing the functions. If we have a new SoC in the
>> same family, you don't have to add another function.
>>
>> Try it to see if you still have to separate them.
>>
> I didn't see any benefit this way. 
> We have 20 registers to set for each soc in this way. In order to use one function, we have to define 20 micro for each soc too.

20 registers? I didn't see that coming. In that case, you can keep it your way.

York