[U-Boot] [PATCH V2] ppc4xx: Add 405EP based PMC405DE board

[Wolfgang Denk]

Dear Matthias Fuchs,

In message <200907151251.25985.matthias.fuchs at esd.eu> you wrote:
> Signed-off-by: Matthias Fuchs <matthias.fuchs at esd.eu>
> ---
...
> diff --git a/board/esd/pmc405de/pmc405de.c b/board/esd/pmc405de/pmc405de.c
> new file mode 100644
> index 0000000..ca26d5c
> --- /dev/null
> +++ b/board/esd/pmc405de/pmc405de.c
...
> +	if ((pllmr0 & PLLMR0_CPU_TO_PLB_MASK) == PLLMR0_CPU_PLB_DIV_3) {
> +		/* fCPU=333MHz, fPLB=111MHz */
> +		if (pci_is_66mhz()) {
> +			if ((pllmr0 & PLLMR0_PCI_TO_PLB_MASK) !=
> +			    PLLMR0_PCI_PLB_DIV_1) {
> +				pll_write((pllmr0 & ~PLLMR0_PCI_TO_PLB_MASK) |
> +					  PLLMR0_PCI_PLB_DIV_1, pllmr1);
> +			}
> +		} else {
> +			if ((pllmr0 & PLLMR0_PCI_TO_PLB_MASK) !=
> +			    PLLMR0_PCI_PLB_DIV_2) {
> +				pll_write((pllmr0 & ~PLLMR0_PCI_TO_PLB_MASK) |
> +					  PLLMR0_PCI_PLB_DIV_2, pllmr1);
> +			}
> +		}
> +	} else {
> +		/* fCPU=133|266MHz, fPLB=133MHz */
> +		if (pci_is_66mhz()) {
> +			if ((pllmr0 & PLLMR0_PCI_TO_PLB_MASK) !=
> +			    PLLMR0_PCI_PLB_DIV_2) {
> +				pll_write((pllmr0 & ~PLLMR0_PCI_TO_PLB_MASK) |
> +					  PLLMR0_PCI_PLB_DIV_2, pllmr1);
> +			}
> +		} else {
> +			if ((pllmr0 & PLLMR0_PCI_TO_PLB_MASK) !=
> +			    PLLMR0_PCI_PLB_DIV_3) {
> +				pll_write((pllmr0 & ~PLLMR0_PCI_TO_PLB_MASK) |
> +					  PLLMR0_PCI_PLB_DIV_3, pllmr1);
> +			}
> +		}
> +	}

Please try to factor out repeated code like here.

How about:

	if ((pllmr0 & PLLMR0_CPU_TO_PLB_MASK) == PLLMR0_CPU_PLB_DIV_3) {
		/* fCPU=333MHz, fPLB=111MHz */
		if (pci_is_66mhz())
			foo(PLLMR0_PCI_PLB_DIV_1);
		else
			foo(PLLMR0_PCI_PLB_DIV_2);
	} else {
		/* fCPU=133|266MHz, fPLB=133MHz */
		if (pci_is_66mhz())
			foo(PLLMR0_PCI_PLB_DIV_2);
		else
			foo(PLLMR0_PCI_PLB_DIV_3);
	}

void foo(int i)
{
	if ((pllmr0 & PLLMR0_PCI_TO_PLB_MASK) != PLLMR0_PCI_PLB_DIV_2)
		pll_write((pllmr0 & ~PLLMR0_PCI_TO_PLB_MASK) | i, pllmr1);
}

> +static int is_monarch(void)
> +{
> +	if (in_be32((void*)GPIO0_IR) & CONFIG_SYS_GPIO_MONARCH_N)
> +		return 0;
> +	return 1;

or:
	return (in_be32((void*)GPIO0_IR) & CONFIG_SYS_GPIO_MONARCH_N) == 0;

Umm... and why do we need a cast here? This should be fixed.

> +}
> +
> +static int pci_is_66mhz(void)
> +{
> +	if (in_be32((void*)GPIO0_IR) & CONFIG_SYS_GPIO_M66EN)
> +		return 1;
> +	return 0;

Ditto.

> +}
> +
> +static int board_revision(void)
> +{
> +	return ((in_be32((void*)GPIO0_IR) & CONFIG_SYS_GPIO_HWREV_MASK) >>
> +		CONFIG_SYS_GPIO_HWREV_SHIFT);
> +}

and again.

> +static int cpld_revision(void)
> +{
> +	return ((in_8((void*)CPLD_VERSION) & CPLD_VERSION_MASK));
> +}

and again.

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
Children are natural mimics who act like their parents despite  every
effort to teach them good manners.