[U-Boot] [PATCH v4 5/8] smbios: Generate type 4 on non-x86 systems

Simon Glass sjg at chromium.org
Thu Aug 18 05:45:31 CEST 2016


Hi Alex,

On 17 August 2016 at 04:29, Alexander Graf <agraf at suse.de> wrote:
> The type 4 table generation code is very x86 centric today. Refactor things
> out into the device model cpu class to allow the tables to get generated for
> other architectures as well.
>
> Signed-off-by: Alexander Graf <agraf at suse.de>
>
> ---
>
> v3 -> v4:
>
>   - Use device model
>
> v4 -> v5:
>
>   - s/get_info/get_vendor/ typo
>   - s/smbios_write_type4_arch/smbios_write_type4_dm/
> ---
>  arch/x86/cpu/baytrail/cpu.c          |  1 +
>  arch/x86/cpu/broadwell/cpu.c         |  1 +
>  arch/x86/cpu/cpu_x86.c               | 13 ++++++++++
>  arch/x86/cpu/ivybridge/model_206ax.c |  1 +
>  arch/x86/include/asm/cpu_x86.h       | 12 +++++++++
>  drivers/cpu/cpu-uclass.c             | 10 ++++++++
>  include/cpu.h                        | 20 +++++++++++++++
>  include/smbios.h                     |  3 +++
>  lib/smbios.c                         | 49 ++++++++++++++++++++++++++----------
>  9 files changed, 97 insertions(+), 13 deletions(-)

This looks good to me, thanks.

Can you please split out the CPU uclass changes into a separate patch?
Also please see a few nits below.

>
> diff --git a/arch/x86/cpu/baytrail/cpu.c b/arch/x86/cpu/baytrail/cpu.c
> index b1faf8c..c454db0 100644
> --- a/arch/x86/cpu/baytrail/cpu.c
> +++ b/arch/x86/cpu/baytrail/cpu.c
> @@ -141,6 +141,7 @@ static const struct cpu_ops cpu_x86_baytrail_ops = {
>         .get_desc       = cpu_x86_get_desc,
>         .get_info       = baytrail_get_info,
>         .get_count      = baytrail_get_count,
> +       .get_vendor     = cpu_x86_get_vendor,
>  };
>
>  static const struct udevice_id cpu_x86_baytrail_ids[] = {
> diff --git a/arch/x86/cpu/broadwell/cpu.c b/arch/x86/cpu/broadwell/cpu.c
> index 3ba21aa..6977e86 100644
> --- a/arch/x86/cpu/broadwell/cpu.c
> +++ b/arch/x86/cpu/broadwell/cpu.c
> @@ -743,6 +743,7 @@ static const struct cpu_ops cpu_x86_broadwell_ops = {
>         .get_desc       = cpu_x86_get_desc,
>         .get_info       = broadwell_get_info,
>         .get_count      = broadwell_get_count,
> +       .get_vendor     = cpu_x86_get_vendor,
>  };
>
>  static const struct udevice_id cpu_x86_broadwell_ids[] = {
> diff --git a/arch/x86/cpu/cpu_x86.c b/arch/x86/cpu/cpu_x86.c
> index 0941041..3b0458d 100644
> --- a/arch/x86/cpu/cpu_x86.c
> +++ b/arch/x86/cpu/cpu_x86.c
> @@ -15,9 +15,21 @@ DECLARE_GLOBAL_DATA_PTR;
>  int cpu_x86_bind(struct udevice *dev)
>  {
>         struct cpu_platdata *plat = dev_get_parent_platdata(dev);
> +       struct cpuid_result res;
>
>         plat->cpu_id = fdtdec_get_int(gd->fdt_blob, dev->of_offset,
>                                       "intel,apic-id", -1);
> +       plat->family = gd->arch.x86;
> +       res = cpuid(1);
> +       plat->id[0] = res.eax;
> +       plat->id[1] = res.edx;
> +
> +       return 0;
> +}
> +
> +int cpu_x86_get_vendor(struct udevice *dev, const char **vendor)
> +{
> +       *vendor = cpu_vendor_name(gd->arch.x86_vendor);
>
>         return 0;
>  }
> @@ -60,6 +72,7 @@ static int cpu_x86_get_count(struct udevice *dev)
>  static const struct cpu_ops cpu_x86_ops = {
>         .get_desc       = cpu_x86_get_desc,
>         .get_count      = cpu_x86_get_count,
> +       .get_vendor     = cpu_x86_get_vendor,
>  };
>
>  static const struct udevice_id cpu_x86_ids[] = {
> diff --git a/arch/x86/cpu/ivybridge/model_206ax.c b/arch/x86/cpu/ivybridge/model_206ax.c
> index 38e244b..d8eb2e7 100644
> --- a/arch/x86/cpu/ivybridge/model_206ax.c
> +++ b/arch/x86/cpu/ivybridge/model_206ax.c
> @@ -478,6 +478,7 @@ static const struct cpu_ops cpu_x86_model_206ax_ops = {
>         .get_desc       = cpu_x86_get_desc,
>         .get_info       = model_206ax_get_info,
>         .get_count      = model_206ax_get_count,
> +       .get_vendor     = cpu_x86_get_vendor,
>  };
>
>  static const struct udevice_id cpu_x86_model_206ax_ids[] = {
> diff --git a/arch/x86/include/asm/cpu_x86.h b/arch/x86/include/asm/cpu_x86.h
> index 1940480..79aaf2d 100644
> --- a/arch/x86/include/asm/cpu_x86.h
> +++ b/arch/x86/include/asm/cpu_x86.h
> @@ -31,4 +31,16 @@ int cpu_x86_bind(struct udevice *dev);
>   */
>  int cpu_x86_get_desc(struct udevice *dev, char *buf, int size);
>
> +/**
> + * cpu_x86_get_vendor() - Get a vendor string for an x86 CPU
> + *
> + * This uses cpu_vendor_name() and is suitable to use as the get_vendor()
> + * method for the CPU uclass.
> + *
> + * @dev:       Device to check (UCLASS_CPU)
> + * @vendor:    Pointer to vendor string pointer
> + * @return:    0 if OK, -ve on error
> + */
> +int cpu_x86_get_vendor(struct udevice *dev, const char **vendor);

Instead of returning a pointer, can you use a buffer and size as with
get_desc()? Or is there a reason why this is worse? It does make
things inconsistent.

> +
>  #endif /* _ASM_CPU_X86_H */
> diff --git a/drivers/cpu/cpu-uclass.c b/drivers/cpu/cpu-uclass.c
> index 7660f99..b25c3b4 100644
> --- a/drivers/cpu/cpu-uclass.c
> +++ b/drivers/cpu/cpu-uclass.c
> @@ -44,6 +44,16 @@ int cpu_get_count(struct udevice *dev)
>         return ops->get_count(dev);
>  }
>
> +int cpu_get_vendor(struct udevice *dev, const char **vendor)
> +{
> +       struct cpu_ops *ops = cpu_get_ops(dev);
> +
> +       if (!ops->vendor)
> +               return -ENOSYS;
> +
> +       return ops->get_vendor(dev, vendor);
> +}
> +
>  U_BOOT_DRIVER(cpu_bus) = {
>         .name   = "cpu_bus",
>         .id     = UCLASS_SIMPLE_BUS,
> diff --git a/include/cpu.h b/include/cpu.h
> index bda5315..2285aee 100644
> --- a/include/cpu.h
> +++ b/include/cpu.h
> @@ -21,6 +21,8 @@ struct cpu_platdata {
>         int cpu_id;
>         int ucode_version;
>         ulong device_id;
> +       u16 family;
> +       u32 id[2];

Please add comments on these. Actually I see that device_id is missing
a comment, oops.
>  };
>
>  /* CPU features - mostly just a placeholder for now */
> @@ -71,6 +73,15 @@ struct cpu_ops {
>          * @return CPU count if OK, -ve on error
>          */
>         int (*get_count)(struct udevice *dev);
> +
> +       /**
> +        * get_vendor() - Get vendor name of a CPU
> +        *
> +        * @dev:        Device to check (UCLASS_CPU)
> +        * @vendor:     Pointer to put the pointer to the vendor string
> +        * @return 0 if OK, -ve on error
> +        */
> +       int (*get_vendor)(struct udevice *dev, const char **vendor);
>  };
>
>  #define cpu_get_ops(dev)        ((struct cpu_ops *)(dev)->driver->ops)
> @@ -102,4 +113,13 @@ int cpu_get_info(struct udevice *dev, struct cpu_info *info);
>   */
>  int cpu_get_count(struct udevice *dev);
>
> +/**
> + * cpu_get_vendor() - Get vendor name of a CPU
> + *
> + * @dev:       Device to check (UCLASS_CPU)
> + * @vendor:    Pointer to put the pointer to the vendor string
> + * @return 0 if OK, -ve on error
> + */
> +int cpu_get_vendor(struct udevice *dev, const char **vendor);
> +
>  #endif
> diff --git a/include/smbios.h b/include/smbios.h
> index 5962d4c..3cbc687 100644
> --- a/include/smbios.h
> +++ b/include/smbios.h
> @@ -139,6 +139,9 @@ struct __packed smbios_type3 {
>  #define SMBIOS_PROCESSOR_STATUS_ENABLED        1
>  #define SMBIOS_PROCESSOR_UPGRADE_NONE  6
>
> +#define SMBIOS_PROCESSOR_FAMILY_OTHER  1
> +#define SMBIOS_PROCESSOR_FAMILY_UNKNOWN        2
> +
>  struct __packed smbios_type4 {
>         u8 type;
>         u8 length;
> diff --git a/lib/smbios.c b/lib/smbios.c
> index 8dfd486..6e8f00a 100644
> --- a/lib/smbios.c
> +++ b/lib/smbios.c
> @@ -10,7 +10,11 @@
>  #include <smbios.h>
>  #include <tables_csum.h>
>  #include <version.h>
> -#include <asm/cpu.h>
> +#ifdef CONFIG_CPU
> +#include <cpu.h>
> +#include <dm.h>
> +#include <dm/uclass-internal.h>
> +#endif
>
>  DECLARE_GLOBAL_DATA_PTR;
>
> @@ -152,26 +156,45 @@ static int smbios_write_type3(uintptr_t *current, int handle)
>         return len;
>  }
>
> +static void smbios_write_type4_dm(struct smbios_type4 *t)
> +{
> +       u16 processor_family = SMBIOS_PROCESSOR_FAMILY_UNKNOWN;
> +       const char *vendor = "Unknown";
> +       const char *name = "Unknown";
> +
> +#ifdef CONFIG_CPU
> +       char processor_name[49];
> +       struct udevice *dev = NULL;
> +
> +       uclass_find_first_device(UCLASS_CPU, &dev);
> +       if (dev) {
> +               struct cpu_platdata *plat = dev_get_parent_platdata(dev);
> +
> +               if (plat->family)
> +                       processor_family = plat->family;
> +               t->processor_id[0] = plat->id[0];
> +               t->processor_id[1] = plat->id[1];
> +
> +               cpu_get_vendor(dev, &vendor);
> +               if (!cpu_get_desc(dev, processor_name, sizeof(processor_name)))
> +                       name = processor_name;
> +       }
> +#endif
> +
> +       t->processor_family = processor_family;
> +       t->processor_manufacturer = smbios_add_string(t->eos, vendor);
> +       t->processor_version = smbios_add_string(t->eos, name);
> +}
> +
>  static int smbios_write_type4(uintptr_t *current, int handle)
>  {
>         struct smbios_type4 *t = (struct smbios_type4 *)*current;
>         int len = sizeof(struct smbios_type4);
> -       const char *vendor;
> -       char *name;
> -       char processor_name[CPU_MAX_NAME_LEN];
> -       struct cpuid_result res;
>
>         memset(t, 0, sizeof(struct smbios_type4));
>         fill_smbios_header(t, SMBIOS_PROCESSOR_INFORMATION, len, handle);
>         t->processor_type = SMBIOS_PROCESSOR_TYPE_CENTRAL;
> -       t->processor_family = gd->arch.x86;
> -       vendor = cpu_vendor_name(gd->arch.x86_vendor);
> -       t->processor_manufacturer = smbios_add_string(t->eos, vendor);
> -       res = cpuid(1);
> -       t->processor_id[0] = res.eax;
> -       t->processor_id[1] = res.edx;
> -       name = cpu_get_name(processor_name);
> -       t->processor_version = smbios_add_string(t->eos, name);
> +       smbios_write_type4_dm(t);
>         t->status = SMBIOS_PROCESSOR_STATUS_ENABLED;
>         t->processor_upgrade = SMBIOS_PROCESSOR_UPGRADE_NONE;
>         t->l1_cache_handle = 0xffff;
> --
> 1.8.5.6
>

Regards,
Simon


More information about the U-Boot mailing list