[U-Boot] [PATCH v2] fdt: Pass the device serial number through devicetree

[Paul Kocialkowski]

Le jeudi 21 mai 2015 à 14:32 -0600, Simon Glass a écrit :
> Hi Paul,
> 
> On 21 May 2015 at 03:27, Paul Kocialkowski <contact at paulk.fr> wrote:
> > Before device-tree, the device serial number used to be passed to the kernel
> > using ATAGs (on ARM). This is now deprecated and all the handover to the kernel
> > should now be done using device-tree. Thus, this passes the serial-number
> > property to the kernel using the serial-number property of the root node, as
> > expected by the kernel.
> >
> > The serial number is a string that somewhat represents the device's serial
> > number. It might come from some form of storage (e.g. an eeprom) and be
> > programmed at factory-time by the manufacturer or come from identification
> > bits available in e.g. the SoC.
> >
> > Signed-off-by: Paul Kocialkowski <contact at paulk.fr>
> 
> Reviewed-by: Simon Glass <sgj at chromium.org>
> 
> But please see a nit below.
> 
> > ---
> >  common/fdt_support.c              | 25 +++++++++++++++++++++++++
> >  common/image-fdt.c                |  4 ++++
> >  doc/device-tree-bindings/root.txt |  4 ++++
> >  include/fdt_support.h             |  1 +
> >  4 files changed, 34 insertions(+)
> >  create mode 100644 doc/device-tree-bindings/root.txt
> >
> > diff --git a/common/fdt_support.c b/common/fdt_support.c
> > index 9e50148..10648b5 100644
> > --- a/common/fdt_support.c
> > +++ b/common/fdt_support.c
> > @@ -194,6 +194,31 @@ static inline int fdt_setprop_uxx(void *fdt, int nodeoffset, const char *name,
> >                 return fdt_setprop_u32(fdt, nodeoffset, name, (uint32_t)val);
> >  }
> >
> > +int fdt_root(void *fdt)
> > +{
> > +       char *serial;
> > +       int err;
> > +
> > +       err = fdt_check_header(fdt);
> > +       if (err < 0) {
> > +               printf("fdt_root: %s\n", fdt_strerror(err));
> > +               return err;
> > +       }
> > +
> > +       serial = getenv("serial#");
> > +       if (serial) {
> > +               err = fdt_setprop(fdt, 0, "serial-number", serial,
> > +                                 strlen(serial) + 1);
> > +
> > +               if (err < 0) {
> > +                       printf("WARNING: could not set serial-number %s.\n",
> > +                              fdt_strerror(err));
> > +                       return err;
> > +               }
> > +       }
> > +
> > +       return 0;
> > +}
> >
> >  int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end)
> >  {
> > diff --git a/common/image-fdt.c b/common/image-fdt.c
> > index 7e2da7b..80e3e63 100644
> > --- a/common/image-fdt.c
> > +++ b/common/image-fdt.c
> > @@ -471,6 +471,10 @@ int image_setup_libfdt(bootm_headers_t *images, void *blob,
> >         int ret = -EPERM;
> >         int fdt_ret;
> >
> > +       if (fdt_root(blob) < 0) {
> > +               printf("ERROR: root node setup failed\n");
> > +               goto err;
> > +       }
> >         if (fdt_chosen(blob) < 0) {
> >                 printf("ERROR: /chosen node create failed\n");
> >                 goto err;
> > diff --git a/doc/device-tree-bindings/root.txt b/doc/device-tree-bindings/root.txt
> > new file mode 100644
> > index 0000000..001ccf3
> > --- /dev/null
> > +++ b/doc/device-tree-bindings/root.txt
> > @@ -0,0 +1,4 @@
> > +The root node
> > +
> > +Optional properties:
> > +  - serial-number : a string representing the device's serial number
> > diff --git a/include/fdt_support.h b/include/fdt_support.h
> > index 5d4f28d..56185c9 100644
> > --- a/include/fdt_support.h
> > +++ b/include/fdt_support.h
> > @@ -16,6 +16,7 @@ u32 fdt_getprop_u32_default_node(const void *fdt, int off, int cell,
> >                                 const char *prop, const u32 dflt);
> >  u32 fdt_getprop_u32_default(const void *fdt, const char *path,
> >                                 const char *prop, const u32 dflt);
> > +int fdt_root(void *fdt);
> 
> Please can you add a comment for this in the standard style?

As far as I can see, fdt_root plays a similar role to fdt_chosen and
fdt_initrd, all of which are defined in fdt_support.c and used in
image-fdt.c's image_setup_libfdt.

Their prototypes are defined in fdt_support.h and neither fdt_chosen nor
fdt_initrd have such a comment, so I didn't think it was very consistent
to add one for fdt_root when writing the patch.

Now if you think it's worth adding such a comment for the sake of
documentation, I don't object to it but it still leaves me with a
feeling of inconsistency with regard to other similar prototypes.

> >  int fdt_chosen(void *fdt);
> >  int fdt_initrd(void *fdt, ulong initrd_start, ulong initrd_end);
> >  void do_fixup_by_path(void *fdt, const char *path, const char *prop,
> > --
> > 1.9.1
> >
> 
> Regards,
> Simon

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20150522/3ca6f70d/attachment.sig>