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

[Yuantian Tang]

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 at 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 at 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 at 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 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>
> >>>>> ---
> >>>>> 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.
If the soc's version is upgraded, we may have to redefine the value if the default value is changed.
All what we do is just to make it in one function. 
Currently, we just need to set the register that requires to change which only have 4 and 2 registers respectively.

I thank your suggestion complicates things.

Regards,
Yuantian
> York