[U-Boot] [PATCH 1/3] ppc4xx: Consolidate pci_target_init() function

[Matthias Fuchs]

Hi Stefan,

good job. Please see two comments and one question below.

> This patch removes the duplicted implementations of the pci_target_init()
> function by introducing a weak default function for it. This weak default
> has a different implementation for 440EP(x)/GR(x) PPC's. It can be
> overridden by a board specific version (e.g. PMC440, korat).
> 
> Signed-off-by: Stefan Roese <sr at denx.de>
> ---
> diff --git a/cpu/ppc4xx/4xx_pci.c b/cpu/ppc4xx/4xx_pci.c
> index 40017f4..28877ce 100644
> --- a/cpu/ppc4xx/4xx_pci.c
> +++ b/cpu/ppc4xx/4xx_pci.c
> @@ -77,6 +77,7 @@
>  #include <asm/4xx_pci.h>
>  #endif
>  #include <asm/processor.h>
> +#include <asm/io.h>
>  #include <pci.h>
>  
>  #ifdef CONFIG_PCI
> @@ -499,6 +500,111 @@ int __is_pci_host(struct pci_controller *hose)
>  int is_pci_host(struct pci_controller *hose)
>  	__attribute__((weak, alias("__is_pci_host")));
>  
> +/*
> + *  pci_target_init
> + *
> + *	The bootstrap configuration provides default settings for the pci
> + *	inbound map (PIM). But the bootstrap config choices are limited and
> + *	may not be sufficient for a given board.
> + */
> +#if defined(CONFIG_SYS_PCI_TARGET_INIT)
> +#if defined(CONFIG_440EP) || defined(CONFIG_440EPX) || \
> +    defined(CONFIG_440GR) || defined(CONFIG_440GRX)
> +void __pci_target_init(struct pci_controller *hose)
> +{
> +	/*
> +	 * Set up Direct MMIO registers
> +	 */
> +
> +	/*
> +	 * PowerPC440 EP PCI Master configuration.
> +	 * Map one 1Gig range of PLB/processor addresses to PCI memory space.
> +	 * PLB address 0xA0000000-0xDFFFFFFF ==> PCI address 0xA0000000-0xDFFFFFFF
> +	 * Use byte reversed out routines to handle endianess.
> +	 * Make this region non-prefetchable.
> +	 */
> +	/* PMM0 Mask/Attribute - disabled b4 setting */
> +	out_le32((void *)PCIL0_PMM0MA, 0x00000000);
> +	/* PMM0 Local Address */
> +	out_le32((void *)PCIL0_PMM0LA, CONFIG_SYS_PCI_MEMBASE);
> +	/* PMM0 PCI Low Address */
> +	out_le32((void *)PCIL0_PMM0PCILA, CONFIG_SYS_PCI_MEMBASE);
> +	/* PMM0 PCI High Address */
> +	out_le32((void *)PCIL0_PMM0PCIHA, 0x00000000);
> +	/* 512M + No prefetching, and enable region */
> +	out_le32((void *)PCIL0_PMM0MA, 0xE0000001);
> +
> +	/* PMM0 Mask/Attribute - disabled b4 setting */
Typo. PMM0<->PMM1 inside comments. 4x.

> +	out_le32((void *)PCIL0_PMM1MA, 0x00000000);
> +	/* PMM0 Local Address */
> +	out_le32((void *)PCIL0_PMM1LA, CONFIG_SYS_PCI_MEMBASE2);
> +	/* PMM0 PCI Low Address */
> +	out_le32((void *)PCIL0_PMM1PCILA, CONFIG_SYS_PCI_MEMBASE2);
> +	/* PMM0 PCI High Address */
> +	out_le32((void *)PCIL0_PMM1PCIHA, 0x00000000);
> +	/* 512M + No prefetching, and enable region */
> +	out_le32((void *)PCIL0_PMM1MA, 0xE0000001);
BTW, do you know why most 440 boards use PMM0+1 to map 1GB?
PMC440 uses PMM0 for this only. I don't see a problem with this when 
CONFIG_SYS_PCI_MEMBASE is aligned to CONFIG_SYS_PCI_MEMBASE.

> +
> +	out_le32((void *)PCIL0_PTM1MS, 0x00000001); /* Memory Size/Attribute */
> +	out_le32((void *)PCIL0_PTM1LA, 0);	/* Local Addr. Reg */
> +	out_le32((void *)PCIL0_PTM2MS, 0);	/* Memory Size/Attribute */
> +	out_le32((void *)PCIL0_PTM2LA, 0);	/* Local Addr. Reg */
> +
> +	/*
> +	 * Set up Configuration registers
> +	 */
> +
> +	/* Program the board's subsystem id/vendor id */
> +	pci_write_config_word(0, PCI_SUBSYSTEM_VENDOR_ID,
> +			      CONFIG_SYS_PCI_SUBSYS_VENDORID);
> +	pci_write_config_word(0, PCI_SUBSYSTEM_ID, CONFIG_SYS_PCI_SUBSYS_ID);
> +
> +	/* Configure command register as bus master */
> +	pci_write_config_word(0, PCI_COMMAND, PCI_COMMAND_MASTER);
> +
> +	/* 240nS PCI clock */
> +	pci_write_config_word(0, PCI_LATENCY_TIMER, 1);
> +
> +	/* No error reporting */
> +	pci_write_config_word(0, PCI_ERREN, 0);
> +
> +	pci_write_config_dword(0, PCI_BRDGOPT2, 0x00000101);
> +}
> +#else /* defined(CONFIG_440EP) ... */
> +void __pci_target_init(struct pci_controller * hose)
> +{
> +	/*
> +	 * Disable everything
> +	 */
> +	out_le32((void *)PCIL0_PIM0SA, 0); /* disable */
> +	out_le32((void *)PCIL0_PIM1SA, 0); /* disable */
> +	out_le32((void *)PCIL0_PIM2SA, 0); /* disable */
> +	out_le32((void *)PCIL0_EROMBA, 0); /* disable expansion rom */
> +
> +	/*
> +	 * Map all of SDRAM to PCI address 0x0000_0000. Note that the 440
> +	 * strapping options do not support sizes such as 128/256 MB.
> +	 */
> +	out_le32((void *)PCIL0_PIM0LAL, CONFIG_SYS_SDRAM_BASE);
> +	out_le32((void *)PCIL0_PIM0LAH, 0);
> +	out_le32((void *)PCIL0_PIM0SA, ~(gd->ram_size - 1) | 1);
> +	out_le32((void *)PCIL0_BAR0, 0);
> +
> +	/*
> +	 * Program the board's subsystem id/vendor id
> +	 */
> +	out_le16((void *)PCIL0_SBSYSVID, CONFIG_SYS_PCI_SUBSYS_VENDORID);
> +	out_le16((void *)PCIL0_SBSYSID, CONFIG_SYS_PCI_SUBSYS_DEVICEID);
> +
> +	out_le16((void *)PCIL0_CMD, in_le16((void *)PCIL0_CMD) |
> +		 PCI_COMMAND_MEMORY);
> +}
> +#endif /* defined(CONFIG_440EP) ... */
> +void pci_target_init(struct pci_controller * hose)
> +	__attribute__((weak, alias("__pci_target_init")));
> +
> +#endif	/* defined(CONFIG_SYS_PCI_TARGET_INIT) */
> +
>  int pci_440_init (struct pci_controller *hose)
>  {
>  	int reg_num = 0;

...

> diff --git a/include/configs/PMC440.h b/include/configs/PMC440.h
> index d6e2f6b..89be2ca 100644
> --- a/include/configs/PMC440.h
> +++ b/include/configs/PMC440.h
> @@ -440,6 +440,7 @@
>  #define CONFIG_SYS_PCI_SUBSYS_VENDORID 0x12FE	/* PCI Vendor ID: esd gmbh      */
>  #define CONFIG_SYS_PCI_SUBSYS_ID_NONMONARCH 0x0441	/* PCI Device ID: Non-Monarch */
>  #define CONFIG_SYS_PCI_SUBSYS_ID_MONARCH 0x0440	/* PCI Device ID: Monarch */
> +#define CONFIG_SYS_PCI_SUBSYS_ID	0x0440	/* for weak __pci_target_init() */
I would prefer:
#define CONFIG_SYS_PCI_SUBSYS_ID CONFIG_SYS_PCI_SUBSYS_ID_MONARCH /* for weak __pci_target_init() */

On the other side I am also not very happy with defines that are required just to satisfy
some overwritten weak code. But in this case it probably the easiest way to go.

>  #define CONFIG_SYS_PCI_CLASSCODE_NONMONARCH	PCI_CLASS_PROCESSOR_POWERPC
>  #define CONFIG_SYS_PCI_CLASSCODE_MONARCH	PCI_CLASS_BRIDGE_HOST
>  

Matthias