[U-Boot] [PATCH] powerpc: add support for the Freescale P1022DS reference board

[Wolfgang Denk <wd@denx.de>]

Dear Timur Tabi,

In message <1274392909-16422-1-git-send-email-timur@freescale.com> you wrote:
> Add basic suport for the Freescale P1022DS reference board.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>
> ---
> 
> This patch requires the following two patches to be applied first:
> 
> fsl/85xx: add clkdvdr and pmuxcr2 to global utilities structure definition
> fsl: rename 'dma' to 'brdcfg1' in the ngPIXIS structure
> 
>  Makefile                              |    3 +
>  arch/powerpc/cpu/mpc8xxx/pci_cfg.c    |   26 ++
>  board/freescale/p1022ds/Makefile      |   40 +++
>  board/freescale/p1022ds/config.mk     |   14 +
>  board/freescale/p1022ds/ddr.c         |  108 +++++++
>  board/freescale/p1022ds/law.c         |   27 ++
>  board/freescale/p1022ds/p1022ds.c     |  369 ++++++++++++++++++++++++
>  board/freescale/p1022ds/p1022ds_diu.c |  151 ++++++++++
>  board/freescale/p1022ds/tlb.c         |   76 +++++
>  include/configs/P1022DS.h             |  505 +++++++++++++++++++++++++++++++++
>  10 files changed, 1319 insertions(+), 0 deletions(-)
>  create mode 100644 board/freescale/p1022ds/Makefile
>  create mode 100644 board/freescale/p1022ds/config.mk
>  create mode 100644 board/freescale/p1022ds/ddr.c
>  create mode 100644 board/freescale/p1022ds/law.c
>  create mode 100644 board/freescale/p1022ds/p1022ds.c
>  create mode 100644 board/freescale/p1022ds/p1022ds_diu.c
>  create mode 100644 board/freescale/p1022ds/tlb.c
>  create mode 100644 include/configs/P1022DS.h

Entries to MAKEALL and MAINTAINERS missing.

> diff --git a/Makefile b/Makefile
> index 1445e8b..583a576 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -2493,6 +2493,9 @@ P2020DS_36BIT_config \
>  P2020DS_config:		unconfig
>  	@$(MKCONFIG) -t $(@:_config=) P2020DS powerpc mpc85xx p2020ds freescale
>  
> +P1022DS_config:		unconfig
> +	@$(MKCONFIG) -t $(@:_config=) P1022DS powerpc mpc85xx p1022ds freescale
> +
>  P1011RDB_config	\
>  P1011RDB_NAND_config \
>  P1011RDB_SDCARD_config \

Please keep lists sorted / sort them.

> diff --git a/board/freescale/p1022ds/ddr.c b/board/freescale/p1022ds/ddr.c
> new file mode 100644
> index 0000000..c43c902
> --- /dev/null
> +++ b/board/freescale/p1022ds/ddr.c
...
> +void fsl_ddr_get_spd(ddr3_spd_eeprom_t *ctrl_dimms_spd, unsigned int ctrl_num)
> +{
> +	int ret;
> +
> +	/* The P1022 has only one DDR controller, and the board has only one
> +	   DIMM slot. */

Incorrect multiline comment style.

...
> +/* ranges for parameters:
> + *  wr_data_delay = 0-6
> + *  clk adjust = 0-8
> + *  cpo 2-0x1E (30)
> + */

Incorrect multiline comment style.

> +static const board_specific_parameters_t bsp[] = {
> +/* 	memory controller 0 			*/
> +/*	  lo|  hi|  num|  clk| cpo|wrdata|2T	*/
> +/*	 mhz| mhz|ranks|adjst|    | delay|	*/

Incorrect multiline comment style.

Please fix globally!

> +	{  0, 333,    1,    5,  31,    3,  0},
> +	{334, 400,    1,    5,  31,    3,  0},
> +	{401, 549,    1,    5,  31,    3,  0},
> +	{550, 680,    1,    5,  31,    5,  0},
> +	{681, 850,    1,    5,  31,    5,  0},
> +	{  0, 333,    2,    5,  31,    3,  0},
> +	{334, 400,    2,    5,  31,    3,  0},
> +	{401, 549,    2,    5,  31,    3,  0},
> +	{550, 680,    2,    5,  31,    5,  0},
> +	{681, 850,    2,    5,  31,    5,  0},

Please use TABs for vertical alignment.

> +void fsl_ddr_board_options(memctl_options_t *popts, dimm_params_t *pdimm,
> +			   unsigned int ctrl_num)
> +{
...
> +	/* Per AN4039, enable ZQ calibration. */
> +	popts->zq_en = 1;
> +
> +
> +	/*

Drop one of these empty lines, please.

> +int board_early_init_f(void)
> +{
> +	ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
> +
> +	/* Set pmuxcr to allow both i2c1 and i2c2 */
> +	setbits_be32(&gur->pmuxcr, 0x1000);
> +	in_be32(&gur->pmuxcr);

What is this in_be32() needed for? Either add a comment why it's
needed, or remove it.

...
> +phys_size_t initdram(int board_type)
> +{
> +	phys_size_t dram_size = 0;
> +
> +	puts("Initializing....\n");
> +
> +	dram_size = fsl_ddr_sdram();
> +	dram_size = setup_ddr_tlbs(dram_size / 0x100000);
> +	dram_size *= 0x100000;
> +
> +	puts("    DDR: ");
> +	return dram_size;

How about using get_ram_size() for autosizing / testing?

> +	/*  Enable the TFP410 Encoder (I2C address 0x38)
> +	*/

Please use a symbolic name instead of the hard-wired constant.

> +void pci_init_board(void)
> +{
> +	volatile ccsr_gur_t *gur = (void *)(CONFIG_SYS_MPC85xx_GUTS_ADDR);
> +	struct fsl_pci_info pci_info[3];
> +	u32 devdisr, pordevsr, io_sel;
> +	int first_free_busno = 0;
> +	int num = 0;
> +
> +	int pcie_ep, pcie_configured;
> +
> +	devdisr = in_be32(&gur->devdisr);
> +	pordevsr = in_be32(&gur->pordevsr);
> +	io_sel = (pordevsr & MPC85xx_PORDEVSR_IO_SEL) >> 18;
> +
> +	debug ("   pci_init_board: devdisr=%x, io_sel=%x\n", devdisr, io_sel);
> +
> +	if (!(pordevsr & MPC85xx_PORDEVSR_SGMII2_DIS))
> +		printf("    eTSEC2 is in sgmii mode.\n");
> +	if (!(pordevsr & MPC85xx_PORDEVSR_SGMII3_DIS))
> +		printf("    eTSEC3 is in sgmii mode.\n");
> +
> +	puts("\n");

Drop that.

> +#ifdef CONFIG_PCIE2
> +	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_2, io_sel);
> +
> +	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE2)) {
> +		SET_STD_PCIE_INFO(pci_info[num], 2);
> +		pcie_ep = fsl_setup_hose(&pcie2_hose, pci_info[num].regs);
> +		printf("    PCIE2 connected to Slot 3 as %s (base addr %lx)\n",
> +				pcie_ep ? "Endpoint" : "Root Complex",
> +				pci_info[num].regs);
> +		first_free_busno = fsl_pci_init_port(&pci_info[num++],
> +					&pcie2_hose, first_free_busno);
> +
> +	} else {
> +		printf("    PCIE2: disabled\n");
> +	}
> +	puts("\n");

Ditto.

> +#else
> +	setbits_be32(&gur->devdisr, MPC85xx_DEVDISR_PCIE2); /* disable */
> +#endif
> +
> +#ifdef CONFIG_PCIE3
> +	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_3, io_sel);
> +
> +	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE3)) {
> +		SET_STD_PCIE_INFO(pci_info[num], 3);
> +		pcie_ep = fsl_setup_hose(&pcie3_hose, pci_info[num].regs);
> +		printf("    PCIE3 connected to Slot 2 as %s (base addr %lx)\n",
> +				pcie_ep ? "Endpoint" : "Root Complex",
> +				pci_info[num].regs);
> +		first_free_busno = fsl_pci_init_port(&pci_info[num++],
> +					&pcie3_hose, first_free_busno);
> +	} else {
> +		printf("    PCIE3: disabled\n");
> +	}
> +	puts("\n");

Ditto.

> +#else
> +	setbits_be32(&gur->devdisr, MPC85xx_DEVDISR_PCIE3); /* disable */
> +#endif
> +
> +#ifdef CONFIG_PCIE1
> +	pcie_configured = is_fsl_pci_cfg(LAW_TRGT_IF_PCIE_1, io_sel);
> +
> +	if (pcie_configured && !(devdisr & MPC85xx_DEVDISR_PCIE)) {
> +		SET_STD_PCIE_INFO(pci_info[num], 1);
> +		pcie_ep = fsl_setup_hose(&pcie1_hose, pci_info[num].regs);
> +		printf("    PCIE1 connected to Slot 1 as %s (base addr %lx)\n",
> +				pcie_ep ? "Endpoint" : "Root Complex",
> +				pci_info[num].regs);
> +		first_free_busno = fsl_pci_init_port(&pci_info[num++],
> +					&pcie1_hose, first_free_busno);
> +	} else {
> +		printf("    PCIE1: disabled\n");
> +	}
> +	puts("\n");

Ditto.


Hm... looks as if you were repeating the same code 3 times. Please make
this a function.


> +#ifdef CONFIG_GET_CLK_FROM_ICS307

This CONFIG_GET_CLK_FROM_ICS307 is documented?

> +/* decode S[0-2] to Output Divider (OD) */
> +static u8 ics307_S_to_OD[] = {
> +	10, 2, 8, 4, 5, 7, 3, 6
> +};
> +
> +/* Calculate frequency being generated by ICS307-02 clock chip based upon
> + * the control bytes being programmed into it. */
> +/* XXX: This function should probably go into a common library */
> +static unsigned long ics307_clk_freq(unsigned char cw0, unsigned char cw1,
> +				     unsigned char cw2)
> +{
> +	const unsigned long InputFrequency = CONFIG_ICS307_REFCLK_HZ;
> +	unsigned long VDW = ((cw1 << 1) & 0x1FE) + ((cw2 >> 7) & 1);
> +	unsigned long RDW = cw2 & 0x7F;
> +	unsigned long OD = ics307_S_to_OD[cw0 & 0x7];
> +	unsigned long freq;

Please do not use any CamelCase or UPPER CASE identifiers.

Please fix globally.

> +#ifdef CONFIG_PCIE3
> +	ft_fsl_pci_setup(blob, "pci2", &pcie3_hose);
> +#endif
> +#ifdef CONFIG_PCIE2
> +	ft_fsl_pci_setup(blob, "pci0", &pcie2_hose);
> +#endif
> +#ifdef CONFIG_PCIE1
> +	ft_fsl_pci_setup(blob, "pci1", &pcie1_hose);
> +#endif

Is this intentional? Looks weird to me...

PCIE1 => pci1 => pcie1_hose
PCIE2 => pci0 => pcie2_hose
PCIE3 => pci2 => pcie3_hose

Weird, weird...

> +++ b/board/freescale/p1022ds/p1022ds_diu.c
> @@ -0,0 +1,151 @@

This should probably go to drivers/video/ ?

> + * Copyright 2007-2010 Freescale Semiconductor, Inc.
> + * Authors: York Sun <yorksun@freescale.com>
> + *          Srikanth Srinivasan <srikanth.srinivasan@freescale.com>
> + *          Timur Tabi <timur@freescale.com>
> + *
> + * 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.
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <asm/io.h>
> +
> +#include "../common/ngpixis.h"
> +#include "../common/fsl_diu_fb.h"
> +
> +#if defined(CONFIG_VIDEO) || defined(CONFIG_CFB_CONSOLE)
> +#include <stdio_dev.h>
> +#include <video_fb.h>
> +#endif
> +
> +extern unsigned int FSL_Logo_BMP[];
> +
> +static unsigned int xres, yres;
> +
> +void diu_set_pixel_clock(unsigned int pixclock)
> +{
> +	ccsr_gur_t *gur = (void *)CONFIG_SYS_MPC85xx_GUTS_ADDR;
> +	unsigned long speed_ccb, temp, pixval;
> +
> +	speed_ccb = get_bus_freq(0);
> +	temp = 1000000000UL / pixclock;
> +	temp *= 1000;
> +	pixval = speed_ccb / temp;
> +	debug("DIU pixval = %lu\n", pixval);
> +
> +	/* Modify PXCLK in GUTS CLKDVDR */
> +	debug("DIU: Current value of CLKDVDR = 0x%08x\n", in_be32(&gur->clkdvdr));
> +	clrbits_be32(&gur->clkdvdr, 0xdfff0000);	/* turn off clock */
> +	setbits_be32(&gur->clkdvdr, 0x80000000 | ((pixval & 0x1F) << 16))
> +	debug("DIU: Modified value of CLKDVDR = 0x%08x\n", in_be32(&gur->clkdvdr));
> +}

Hmmm... this looks pretty much similar to
board/freescale/mpc8610hpcd/mpc8610hpcd_diu.c;

I think you should merge these two into one driver.

> +int p1022diu_init_show_bmp(cmd_tbl_t *cmdtp, int flag, int argc, char *argv[])
> +{
> +	unsigned int addr;

...while doing so, please clean up to use the standard bmp command
instead.

> +U_BOOT_CMD(
> +	diufb, CONFIG_SYS_MAXARGS, 1, p1022diu_init_show_bmp,
> +	"Init or Display BMP file",
> +	"init\n    - initialize DIU\n"
> +	"addr\n    - display bmp at address 'addr'"
> +);

This should go away.

> diff --git a/include/configs/P1022DS.h b/include/configs/P1022DS.h
> new file mode 100644
> index 0000000..65a1265
> --- /dev/null
> +++ b/include/configs/P1022DS.h
...
> +#ifndef __ASSEMBLY__
> +extern unsigned long calculate_board_sys_clk(void);
> +extern unsigned long calculate_board_ddr_clk(void);
> +#endif

Please move to appropriate header file.

> +/*
> + * Base addresses -- Note these are effective addresses where the
> + * actual resources get mapped (not physical addresses)
> + */
> +#define CONFIG_SYS_CCSRBAR_DEFAULT	0xff700000	/* CCSRBAR Default */
> +#define CONFIG_SYS_CCSRBAR		0xffe00000	/* relocated CCSRBAR */
> +#define CONFIG_SYS_CCSRBAR_PHYS		0xfffe00000ull	/* physical addr of CCSRBAR */
> +#define CONFIG_SYS_IMMR			CONFIG_SYS_CCSRBAR	/* PQII uses CONFIG_SYS_IMMR */

Lines too long. Please fix globally.

> +/* ECC will be enabled based on perf_mode environment variable */
> +//#define	CONFIG_DDR_ECC

No C++ comments. Please fix globally.

> +/* video */
> +/* #define CONFIG_VIDEO */

Please do not add dead code. Please fix globally.

> +#define CONFIG_BOOTDELAY 10	/* -1 disables auto-boot */
> +#undef  CONFIG_BOOTARGS		/* the boot command will set bootargs */

Do not undefine what is not defined anyway.

> +#define	CONFIG_EXTRA_ENV_SETTINGS				\
> + "perf_mode=stable\0"			\
> + "memctl_intlv_ctl=2\0"						\
> + "netdev=eth0\0"						\
> + "uboot=" MK_STR(CONFIG_UBOOTPATH) "\0"				\
> + "tftpflash=tftpboot $loadaddr $uboot; "			\
> +	"protect off " MK_STR(TEXT_BASE) " +$filesize; "	\
> +	"erase " MK_STR(TEXT_BASE) " +$filesize; "		\
> +	"cp.b $loadaddr " MK_STR(TEXT_BASE) " $filesize; "	\
> +	"protect on " MK_STR(TEXT_BASE) " +$filesize; "		\
> +	"cmp.b $loadaddr " MK_STR(TEXT_BASE) " $filesize\0"	\

Please indent by TABs only.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
It's hard to make a program foolproof because fools are so ingenious.