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

[Matthias Fuchs]

Hi Wolfgang,

On Wednesday 15 July 2009 17:08, Wolfgang Denk wrote:
> 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.
Ack.
> > +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;
Ack.
> 
> Umm... and why do we need a cast here? This should be fixed.
The in/out_be16/32 functions expect a pointer. When you grep over the
U-Boot sources you will find tons of places with this cast.
So I will and cannot fix this with my patch.

We could put the cast into the GPIO_IR (and friends) definitions.
Do you want me to remove the cast and life with a compiler warning until
this has been globally fixed?

Because this is not a particular problem with this patch it should be
addressed in a separate discussion. But you are rigth - this cast is
a little bit nerving :-)

> 
> > +}
> > +
> > +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
>