[PATCH 1/8] dm: soc: Introduce UCLASS_SOC for SOC ID and attribute matching
Dave Gerlach
d-gerlach at ti.com
Thu Jul 9 01:16:45 CEST 2020
Simon,
On 7/2/20 10:50 PM, Simon Glass wrote:
> On Mon, 29 Jun 2020 at 22:38, Dave Gerlach <d-gerlach at ti.com> wrote:
>>
>> Introduce UCLASS_SOC to be used for SOC identification and attribute
>> matching based on the SoC ID info. This allows drivers to be provided
>> for SoCs to retrieve SoC identifying information and also for matching
>> device attributes for selecting SoC specific data.
>>
>> This is useful for other device drivers that may need different
>> parameters or quirks enabled depending on the specific device variant in
>> use.
>>
>> Signed-off-by: Dave Gerlach <d-gerlach at ti.com>
>> ---
>> drivers/soc/Kconfig | 9 +++
>> drivers/soc/Makefile | 1 +
>> drivers/soc/soc-uclass.c | 102 ++++++++++++++++++++++++++++++
>> include/dm/uclass-id.h | 1 +
>> include/soc.h | 132 +++++++++++++++++++++++++++++++++++++++
>> 5 files changed, 245 insertions(+)
>> create mode 100644 drivers/soc/soc-uclass.c
>> create mode 100644 include/soc.h
>
> Reviewed-by: Simon Glass <sjg at chromium.org>
>
> As Tom says, docs please but looks great otherwise.
>
Yes will add docs in v2.
> Nits below. Do you think it might make sense to have an
> integer-encoded version? Maybe it would be more hassle than it is
> worth.
Well my primary driver for using strings was to be consistent with the
kernel counterpart, but I also thought it was convenient beacuse it
allows the identifiers to be descriptive and also to be printed out
directly once retrieved. Attributes could be added as needed in the future.
>
>>
>> diff --git a/drivers/soc/Kconfig b/drivers/soc/Kconfig
>> index 7b4e4d613088..e715dfd01712 100644
>> --- a/drivers/soc/Kconfig
>> +++ b/drivers/soc/Kconfig
>> @@ -1,5 +1,14 @@
>> menu "SOC (System On Chip) specific Drivers"
>>
>> +config SOC_DEVICE
>> + bool "Enable SoC Device ID drivers using Driver Model"
>> + help
>> + This allows drivers to be provided for SoCs to help in identifying
>> + the SoC in use and matching SoC attributes for selecting SoC
>> + specific data. This is useful for other device drivers that may
>> + need different parameters or quirks enabled depending on the
>> + specific device variant in use.
>> +
>> source "drivers/soc/ti/Kconfig"
>>
>> endmenu
>> diff --git a/drivers/soc/Makefile b/drivers/soc/Makefile
>> index ce253b7aa886..1c09a8465670 100644
>> --- a/drivers/soc/Makefile
>> +++ b/drivers/soc/Makefile
>> @@ -3,3 +3,4 @@
>> # Makefile for the U-Boot SOC specific device drivers.
>>
>> obj-$(CONFIG_SOC_TI) += ti/
>> +obj-$(CONFIG_SOC_DEVICE) += soc-uclass.o
>> diff --git a/drivers/soc/soc-uclass.c b/drivers/soc/soc-uclass.c
>> new file mode 100644
>> index 000000000000..22f89514ed7d
>> --- /dev/null
>> +++ b/drivers/soc/soc-uclass.c
>> @@ -0,0 +1,102 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + * (C) Copyright 2020 - Texas Instruments Incorporated - http://www.ti.com/
>> + * Dave Gerlach <d-gerlach at ti.com>
>> + */
>> +
>> +#include <common.h>
>> +#include <soc.h>
>> +#include <dm.h>
>> +#include <errno.h>
>> +#include <dm/lists.h>
>> +#include <dm/root.h>
>> +
>> +int soc_get(struct udevice **devp)
>> +{
>> + return uclass_first_device_err(UCLASS_SOC, devp);
>> +}
>> +
>> +int soc_get_machine(struct udevice *dev, char *buf, int size)
>> +{
>> + struct soc_ops *ops = soc_get_ops(dev);
>> +
>> + if (!ops->get_machine)
>> + return -ENOSYS;
>> +
>> + return ops->get_machine(dev, buf, size);
>> +}
>> +
>> +int soc_get_family(struct udevice *dev, char *buf, int size)
>> +{
>> + struct soc_ops *ops = soc_get_ops(dev);
>> +
>> + if (!ops->get_family)
>> + return -ENOSYS;
>> +
>> + return ops->get_family(dev, buf, size);
>> +}
>> +
>> +int soc_get_revision(struct udevice *dev, char *buf, int size)
>> +{
>> + struct soc_ops *ops = soc_get_ops(dev);
>> +
>> + if (!ops->get_revision)
>> + return -ENOSYS;
>> +
>> + return ops->get_revision(dev, buf, size);
>> +}
>> +
>> +const struct soc_device_attribute *
>> +soc_device_match(const struct soc_device_attribute *matches)
>> +{
>> + bool match;
>> + struct udevice *soc;
>> + char str[SOC_MAX_STR_SIZE];
>> +
>> + if (!matches)
>> + return NULL;
>> +
>> + if (soc_get(&soc))
>> + return NULL;
>> +
>> + while (1) {
>> + if (!(matches->machine || matches->family ||
>> + matches->revision))
>> + break;
>> +
>> + match = true;
>> +
>> + if (matches->machine) {
>> + if (!soc_get_machine(soc, str, SOC_MAX_STR_SIZE)) {
>> + if (strcmp(matches->machine, str))
>> + match = false;
>> + }
>> + }
>> +
>> + if (matches->family) {
>> + if (!soc_get_family(soc, str, SOC_MAX_STR_SIZE)) {
>> + if (strcmp(matches->family, str))
>> + match = false;
>> + }
>> + }
>> +
>> + if (matches->revision) {
>> + if (!soc_get_revision(soc, str, SOC_MAX_STR_SIZE)) {
>> + if (strcmp(matches->revision, str))
>> + match = false;
>> + }
>> + }
>> +
>> + if (match)
>> + return matches;
>> +
>> + matches++;
>> + }
>> +
>> + return NULL;
>> +}
>> +
>> +UCLASS_DRIVER(soc) = {
>> + .id = UCLASS_SOC,
>> + .name = "soc",
>> +};
>> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
>> index 7837d459f18c..690a8ed4df49 100644
>> --- a/include/dm/uclass-id.h
>> +++ b/include/dm/uclass-id.h
>> @@ -97,6 +97,7 @@ enum uclass_id {
>> UCLASS_SERIAL, /* Serial UART */
>> UCLASS_SIMPLE_BUS, /* Bus with child devices */
>> UCLASS_SMEM, /* Shared memory interface */
>> + UCLASS_SOC, /* SOC Device */
>> UCLASS_SOUND, /* Playing simple sounds */
>> UCLASS_SPI, /* SPI bus */
>> UCLASS_SPI_FLASH, /* SPI flash */
>> diff --git a/include/soc.h b/include/soc.h
>> new file mode 100644
>> index 000000000000..98e3cc000198
>> --- /dev/null
>> +++ b/include/soc.h
>> @@ -0,0 +1,132 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/*
>> + * (C) Copyright 2020 - Texas Instruments Incorporated - http://www.ti.com/
>> + * Dave Gerlach <d-gerlach at ti.com>
>> + */
>> +
>> +#ifndef __SOC_H
>> +#define __SOC_H
>> +
>> +#define SOC_MAX_STR_SIZE 128
>> +
>> +struct soc_device_attribute {
>
> Could this be soc_attr or maybe soc_dev_attr? This is too long.
Good idea, it is too long for no reason, I will shorten it.
>
>> + const char *machine;
>> + const char *family;
>> + const char *revision;
>> + const void *data;
>
> Please can you comment this struct with what each thing means and an
> example for your SoC.
Will do.
>
>> +};
>> +
>> +#ifdef CONFIG_SOC_DEVICE
>
> Please drop the #ifdef as there is no harm in defining this.
Ok, will move it lower to wrap the functions so that stubs are still
defined when !CONFIG_SOC_DEVICE.
>
>> +struct soc_ops {
>> + /**
>> + * get_machine() - Get machine name of an SOC
>> + *
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> + int (*get_machine)(struct udevice *dev, char *buf, int size);
>> +
>> + /**
>> + * get_revision() - Get revision name of a SOC
>> + *
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> + int (*get_revision)(struct udevice *dev, char *buf, int size);
>> +
>> + /**
>> + * get_family() - Get family name of an SOC
>> + *
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> + int (*get_family)(struct udevice *dev, char *buf, int size);
>> +};
>> +
>> +#define soc_get_ops(dev) ((struct soc_ops *)(dev)->driver->ops)
>> +
>> +/**
>> + * soc_get() - Return the soc device for the soc in use.
>> + * @devp: Pointer to structure to receive the soc device.
>> + *
>> + * Since there can only be at most one SOC instance, the API can supply a
>> + * function that returns the unique device.
>> + *
>> + * Return: 0 if OK, -ve on error.
>> + */
>> +int soc_get(struct udevice **devp);
>> +
>> +/**
>> + * soc_get_machine() - Get machine name of an SOC
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + *
>> + * Return: 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> +int soc_get_machine(struct udevice *dev, char *buf, int size);
>> +
>> +/**
>> + * soc_get_revision() - Get revision name of an SOC
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + *
>> + * Return: 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> +int soc_get_revision(struct udevice *dev, char *buf, int size);
>> +
>> +/**
>> + * soc_get_family() - Get family name of an SOC
>> + * @dev: Device to check (UCLASS_SOC)
>> + * @buf: Buffer to place string
>> + * @size: Size of string space
>> + *
>> + * Return: 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>> + */
>> +int soc_get_family(struct udevice *dev, char *buf, int size);
>> +
>> +/**
>> + * soc_device_match() - Return match from an array of soc_device_attribute
>> + * @matches: Array with any combination of family, revision or machine set
>> + *
>> + * Return: Pointer to struct from matches array with set attributes matching
>> + * those provided by the soc device.
>
> or NULL ?
Yes, must document the not found case as well. Will add. Appreciate the
comments!
Regards,
Dave
>
>> + */
>> +const struct soc_device_attribute *
>> +soc_device_match(const struct soc_device_attribute *matches);
>> +
>> +#else
>> +static inline int soc_get(struct udevice **devp)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +static inline int soc_get_machine(struct udevice *dev, char *buf, int size)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +static inline int soc_get_revision(struct udevice *dev, char *buf, int size)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +static inline int soc_get_family(struct udevice *dev, char *buf, int size)
>> +{
>> + return -ENOSYS;
>> +}
>> +
>> +static inline const struct soc_device_attribute *
>> +soc_device_match(const struct soc_device_attribute *matches)
>> +{
>> + return NULL;
>> +}
>> +#endif
>> +#endif /* _SOC_H */
>> --
>> 2.20.1
>>
>
> Regards,
> Simon
>
More information about the U-Boot
mailing list