[U-Boot] [PATCH v1 8/8] mpc85xx: introduce the kmp204x reference design support

[Valentin Longchamp <valentin.longchamp@keymile.com>]

On 08/13/2013 11:38 PM, Scott Wood wrote:
> On Fri, 2013-07-26 at 12:02 +0200, Valentin Longchamp wrote:
>> This patch introduces the support for Keymile's kmp204x reference
>> design. This design is based on Freescale's P2040/P2041 SoC.
>>
>> The peripherals used by this design are:
>> - DDR3 RAM with SPD support
>> - SPI NOR Flash as boot medium
>> - NAND Flash
>> - 2 PCIe busses (hosts 1 and 3)
>> - 3 FMAN Ethernet devices (FMAN1 DTSEC1/2/5)
>> - 3 Local Bus windows, with one dedicated to the QRIO reset/power mgmt
>>   FPGA
>> - 2 HW I2C busses
>> - last but not least, the mandatory serial port
>>
>> The board/keymile/kmp204x code is mostly based on Freescale's P2041rdb
>> support and was changed according to our design (that means essentially
>> removing what is not present on the designs and a few adaptations).
> 
> A lot of the copied files have had Freescale copyrights removed...  Also
> please try to factor shared code out rather than duplicate, where
> practical.

Well, I had tried to come a first proposal that factored shared code but I was
advised to copy the code: [1]. For the copyrights I will fix it.

[1]

> 
>>
>> There is currently only one prototype board that is based on this design
>> and this patch also introduces it. The board is called kmlion1.
>>
>> Signed-off-by: Stefan Bigler <stefan.bigler@keymile.com>
>> Signed-off-by: Valentin Longchamp <valentin.longchamp@keymile.com>
>> ---
>>  MAINTAINERS                           |   1 +
>>  board/keymile/common/common.c         |   4 +-
>>  board/keymile/kmp204x/Makefile        |  48 ++++
>>  board/keymile/kmp204x/ddr.c           |  84 +++++++
>>  board/keymile/kmp204x/eth.c           |  87 +++++++
>>  board/keymile/kmp204x/kmp204x.c       | 307 ++++++++++++++++++++++
>>  board/keymile/kmp204x/kmp204x.h       |  31 +++
>>  board/keymile/kmp204x/law.c           |  51 ++++
>>  board/keymile/kmp204x/pbi.cfg         |  51 ++++
>>  board/keymile/kmp204x/pci.c           |  49 ++++
>>  board/keymile/kmp204x/rcw_kmp204x.cfg |  11 +
>>  board/keymile/kmp204x/tlb.c           | 122 +++++++++
>>  boards.cfg                            |   1 +
>>  include/configs/km/kmp204x-common.h   | 462 ++++++++++++++++++++++++++++++++++
>>  include/configs/kmp204x.h             |  84 +++++++
>>  15 files changed, 1392 insertions(+), 1 deletion(-)
>>  create mode 100644 board/keymile/kmp204x/Makefile
>>  create mode 100644 board/keymile/kmp204x/ddr.c
>>  create mode 100644 board/keymile/kmp204x/eth.c
>>  create mode 100644 board/keymile/kmp204x/kmp204x.c
>>  create mode 100644 board/keymile/kmp204x/kmp204x.h
>>  create mode 100644 board/keymile/kmp204x/law.c
>>  create mode 100644 board/keymile/kmp204x/pbi.cfg
>>  create mode 100644 board/keymile/kmp204x/pci.c
>>  create mode 100644 board/keymile/kmp204x/rcw_kmp204x.cfg
>>  create mode 100644 board/keymile/kmp204x/tlb.c
>>  create mode 100644 include/configs/km/kmp204x-common.h
>>  create mode 100644 include/configs/kmp204x.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 081cf96..fff77f0 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -778,6 +778,7 @@ Valentin Longchamp <valentin.longchamp@keymile.com>
>>  	mgcoge3un	ARM926EJS (Kirkwood SoC)
>>  	kmcoge5un	ARM926EJS (Kirkwood SoC)
>>  	portl2		ARM926EJS (Kirkwood SoC)
>> +	kmcoge4		MPC85xx (P2041 SoC)
>>  
>>  Nishanth Menon <nm@ti.com>
>>  
>> diff --git a/board/keymile/common/common.c b/board/keymile/common/common.c
>> index ef93ed3..ca833db 100644
>> --- a/board/keymile/common/common.c
>> +++ b/board/keymile/common/common.c
>> @@ -94,7 +94,7 @@ int set_km_env(void)
>>  }
>>  
>>  #if defined(CONFIG_SYS_I2C_INIT_BOARD)
>> -#if !defined(CONFIG_MPC83xx)
>> +#if !defined(CONFIG_MPC83xx) && !defined(CONFIG_PPC_P2041)
> 
> Perhaps you should check for when you do want to run this code, rather
> than when you don't.

No, there are more cases where this is needed than not needed.

> 
>>  static void i2c_write_start_seq(void)
>>  {
>>  	set_sda(1);
>> @@ -183,6 +183,7 @@ void i2c_init_board(void)
>>  }
>>  #endif
>>  
>> +#ifndef CONFIG_KMP204X
>>  int board_eth_init(bd_t *bis)
> 
> Likewise.

Same as above.

> 
>> +/* TODO: implement the I2C functions */
>> +void i2c_write_start_seq(void){
>> +	return;
>> +}
> 
> Opening brace goes on its own line for function definitions.

OK

> 
>> +	if (en)
>> +		prst &= ~(1<<bit);
>> +	else
>> +		prst |= (1<<bit);
> 
> Spaces around binary operators such as <<

OK

> 
>> +#ifdef CONFIG_SYS_NAND_BASE_PHYS
>> +	SET_LAW(CONFIG_SYS_NAND_BASE_PHYS, LAW_SIZE_1M, LAW_TRGT_IF_LBC),
>> +#endif
> 
> The NAND window is 8K.  There's no reason for the LAW to be more than
> the minimum 32K.

OK, I agree and will test this.

> 
>> diff --git a/board/keymile/kmp204x/tlb.c b/board/keymile/kmp204x/tlb.c
>> new file mode 100644
>> index 0000000..5bbefd5
>> --- /dev/null
>> +++ b/board/keymile/kmp204x/tlb.c
>> @@ -0,0 +1,122 @@
>> +/*
>> + * (C) Copyright 2013 Keymile AG
>> + * Valentin Longchamp <valentin.longchamp@keymile.com>
>> + *
>> + * See file CREDITS for list of people who contributed to this
>> + * project.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License as
>> + * published by the Free Software Foundation; either version 2 of
>> + * the License, or (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.	 See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston,
>> + * MA 02110-1301 USA
>> + */
>> +
>> +#include <common.h>
>> +#include <asm/mmu.h>
>> +
>> +struct fsl_e_tlb_entry tlb_table[] = {
>> +	/* TLB 0 - for temp stack in cache */
>> +	SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR,
>> +		      CONFIG_SYS_INIT_RAM_ADDR_PHYS,
>> +		      MAS3_SW|MAS3_SR, 0,
>> +		      0, 0, BOOKE_PAGESZ_4K, 0),
>> +	SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 4 * 1024,
>> +		      CONFIG_SYS_INIT_RAM_ADDR_PHYS + 4 * 1024,
>> +		      MAS3_SW|MAS3_SR, 0,
>> +		      0, 0, BOOKE_PAGESZ_4K, 0),
>> +	SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 8 * 1024,
>> +		      CONFIG_SYS_INIT_RAM_ADDR_PHYS + 8 * 1024,
>> +		      MAS3_SW|MAS3_SR, 0,
>> +		      0, 0, BOOKE_PAGESZ_4K, 0),
>> +	SET_TLB_ENTRY(0, CONFIG_SYS_INIT_RAM_ADDR + 12 * 1024,
>> +		      CONFIG_SYS_INIT_RAM_ADDR_PHYS + 12 * 1024,
>> +		      MAS3_SW|MAS3_SR, 0,
>> +		      0, 0, BOOKE_PAGESZ_4K, 0),
>> +	/* TLB 1 */
>> +	/* *I*** - Covers boot page */
>> +	/* *I*G - L3SRAM. When L3 is used as 1M SRAM, the address of the
>> +	 * SRAM is at 0xfff00000, it covered the 0xfffff000.
>> +	 */
>> +	SET_TLB_ENTRY(1, CONFIG_SYS_INIT_L3_ADDR, CONFIG_SYS_INIT_L3_ADDR,
>> +		      MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> +		      0, 0, BOOKE_PAGESZ_1M, 1),
> 
> What does that "covers boot page" comment refer to?
> 
> Why is L3SRAM I+G?
> 

I have taken this from the corenet SYS_RAMBOOT boot scenario since it's also the
way our board boots.

>> +#ifdef CONFIG_SYS_NAND_BASE
>> +	/*
>> +	 * *I*G - NAND
>> +	 * entry 14 and 15 has been used hard coded, they will be disabled
>> +	 * in cpu_init_f, so we use entry 16 for nand.
>> +	 */
>> +	SET_TLB_ENTRY(1, CONFIG_SYS_NAND_BASE, CONFIG_SYS_NAND_BASE_PHYS,
>> +		      MAS3_SX|MAS3_SW|MAS3_SR, MAS2_I|MAS2_G,
>> +		      0, 16, BOOKE_PAGESZ_1M, 1),
>> +#endif
> 
> Again, NAND is an 8K window.  Don't set MAS3_SX.

OK

> 
>> +};
>> +
>> +int num_tlb_entries = ARRAY_SIZE(tlb_table);
>> diff --git a/boards.cfg b/boards.cfg
>> index 6a368de..b818f1e 100644
>> --- a/boards.cfg
>> +++ b/boards.cfg
>> @@ -744,6 +744,7 @@ tuge1                        powerpc     mpc83xx     km83xx              keymile
>>  tuxx1                        powerpc     mpc83xx     km83xx              keymile        -           tuxx1:TUXX1
>>  kmopti2                      powerpc     mpc83xx     km83xx              keymile        -           tuxx1:KMOPTI2
>>  kmsupx5                      powerpc     mpc83xx     km83xx              keymile        -           tuxx1:KMSUPX5
>> +kmlion1                      powerpc     mpc85xx     kmp204x             keymile        -           kmp204x:KMLION1
> 
> Is it kmlion1 or km204x?  The target name should match what the board
> prints out on boot.

It is the kmlion1 board that is based on the kmp204x "generic" design.

> 
>> +#define CONFIG_SYS_NAND_BASE_LIST     {CONFIG_SYS_NAND_BASE}
>> +#define CONFIG_SYS_MAX_NAND_DEVICE	1
>> +#define CONFIG_MTD_NAND_VERIFY_WRITE
>> +#define CONFIG_CMD_NAND
>> +#define CONFIG_SYS_NAND_BLOCK_SIZE    (128 * 1024)
>> +
>> +#define CONFIG_NAND_ECC_BCH
>> +#define CONFIG_BCH
>>
>> +/* NAND flash config */
>> +#define CONFIG_SYS_NAND_BR_PRELIM  (BR_PHYS_ADDR(CONFIG_SYS_NAND_BASE_PHYS) \
>> +			       | BR_PS_8	       /* Port Size = 8 bit */ \
>> +			       | BR_MS_FCM	       /* MSEL = FCM */ \
>> +			       | BR_V)		       /* valid */
>> +
>> +#define CONFIG_SYS_NAND_OR_PRELIM  (OR_AM_256KB	      /* length 256K */ \
>> +			       | OR_FCM_BCTLD	/* LBCTL not ass */	\
>> +			       | OR_FCM_SCY_1	/* 1 clk wait cycle */	\
>> +			       | OR_FCM_RST	/* 1 clk read setup */	\
>> +			       | OR_FCM_PGS	/* Large Page 128k */	\
>> +			       | OR_FCM_CST)	/* 0.25 command setup */
> 
> What does "large page 128k" mean?  128k is the block size, not the page
> size.
> 

OK

Valentin