[U-Boot-Users] [PATCH 2/2] Migrate 5xxx boards from CONFIG_OF_FLAT_TREE to CONFIG_OF_LIBFDT

[Grant Likely]

On 9/4/07, Bartlomiej Sieka <tur at semihalf.com> wrote:
> Grant Likely wrote:
> > From: Grant Likely <grant.likely at secretlab.ca>
> >
> > Affects boards: icecube (lite5200), jupiter, motionpro, tqm5200
> >
> > Tested on: lite5200b
> >
> > Signed-off-by: Grant Likely <grant.likely at secretlab.ca>
> > ---
>
> Hi Grant,
>
> Thanks for including motionpro changes with this patch; see my comments
> below.
>
> [...]
> > diff --git a/cpu/mpc5xxx/cpu.c b/cpu/mpc5xxx/cpu.c
> > index 1eac2bb..e8a928a 100644
> > --- a/cpu/mpc5xxx/cpu.c
> > +++ b/cpu/mpc5xxx/cpu.c
> > @@ -25,14 +25,18 @@
> >   * CPU specific code for the MPC5xxx CPUs
> >   */
> >
> > +#undef DEBUG
> > +
>
> Is this needed?
>
>
> >  #include <common.h>
> >  #include <watchdog.h>
> >  #include <command.h>
> >  #include <mpc5xxx.h>
> > +#include <asm/io.h>
> >  #include <asm/processor.h>
> >
> > -#if defined(CONFIG_OF_FLAT_TREE)
> > -#include <ft_build.h>
> > +#if defined(CONFIG_OF_LIBFDT)
> > +#include <libfdt.h>
> > +#include <libfdt_env.h>
>
> Are we using this?
>
>
> >  #endif
> >
> >  DECLARE_GLOBAL_DATA_PTR;
> > @@ -109,31 +113,46 @@ unsigned long get_tbclk (void)
> >       return (tbclk);
> >  }
> >
> > +
> >  /* ------------------------------------------------------------------------- */
> >
> > -#ifdef CONFIG_OF_FLAT_TREE
> > -void
> > -ft_cpu_setup(void *blob, bd_t *bd)
> > +#ifdef CONFIG_OF_LIBFDT
> > +static void do_fixup(void *fdt, const char *node, const char *prop,
> > +                  const void *val, int len, int create)
> > +{
> > +#if defined(DEBUG)
> > +     int i;
> > +     debug("Updating property '%s/%s' = ", node, prop);
> > +     for (i = 0; i < len; i++)
> > +             debug(" %.2x", *(u8*)(val+i));
> > +     debug("\n");
> > +#endif
> > +     int rc = fdt_find_and_setprop(fdt, node, prop, val, len, create);
> > +     if (rc)
> > +             printf("Unable to update property %s:%s, err=%s\n",
> > +                    node, prop, fdt_strerror(rc));
> > +}
> > +
> > +static void do_fixup_u32(void *fdt, const char *node, const char *prop,
> > +                      u32 val, int create)
> > +{
> > +     val = cpu_to_be32(val);
>
> Shouldn't this be cpu_to_fdt32()? In such a case we need libfdt_env.h of
> course.
>
> > +     do_fixup(fdt, node, prop, &val, sizeof(val), create);
> > +}
>
> I don't think do_fixup() and do_fixup_u32() should be in
> cpu/mpc5xxx/cpu.c - they can and should be used by 83xx (and others)
> without modification. Is libfdt really out of the question because of
> printf() calls?

I think so.  libfdt is *supposed* to be cross platform code that is
used by u-boot, dtc, etc.  Unfortunately there has already been drift
in this regard, :-(  I think libfdt functions should provide feedback
via the return code, and let callers take care of formating output.
The fixup functions in cpu.c is purely shorthand to keep the body of
ft_setup_cpu tidy (so it doesn't need the same boilerplate for each
function call to check the results), but I do agree that they are
useful to more than just 5xxx.  So where should they go?  And as a
secondary question, do we put them there now, or move them in the next
merge window?

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely at secretlab.ca
(403) 399-0195