Hi Wolfgang,
On Wednesday 15 July 2009 17:08, Wolfgang Denk wrote:
> Dear Matthias Fuchs,
>
> In message <200907151251.25985.matthias.fu...@esd.eu> you wrote:
> > Signed-off-by: Matthias Fuchs <matthias.fu...@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
>
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
