dm: soc: Add SoC id for attribute matching

Add SoC identification string for attribute matching.
Also changed the comments from "an SOC" to "a SoC".

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
 drivers/soc/soc-uclass.c  | 19 ++++++++++++++++++-
 drivers/soc/soc_sandbox.c |  8 ++++++++
 include/soc.h             | 39 +++++++++++++++++++++++++++++++++------
 test/dm/soc.c             |  8 ++++++++
 4 files changed, 67 insertions(+), 7 deletions(-)

Hi,

On 10/30/20 9:07 AM, Biju Das wrote:
> Add SoC identification string for attribute matching.
> Also changed the comments from "an SOC" to "a SoC".

This is not a correct change, "an" should be used if the word that 
follows starts with a vowel sound, which "SoC" does.

"SOC" could be changed to "SoC", no strong feelings there.

> 
> Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
> ---
>   drivers/soc/soc-uclass.c  | 19 ++++++++++++++++++-
>   drivers/soc/soc_sandbox.c |  8 ++++++++
>   include/soc.h             | 39 +++++++++++++++++++++++++++++++++------
>   test/dm/soc.c             |  8 ++++++++
>   4 files changed, 67 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/soc/soc-uclass.c b/drivers/soc/soc-uclass.c
> index c32d647864..a3f8be841b 100644
> --- a/drivers/soc/soc-uclass.c
> +++ b/drivers/soc/soc-uclass.c
> @@ -46,6 +46,16 @@ int soc_get_revision(struct udevice *dev, char *buf, int size)
>   	return ops->get_revision(dev, buf, size);
>   }
>   
> +int soc_get_soc_id(struct udevice *dev, char *buf, int size)
Is there any additional background here? Why is soc_id needed? Can 
"machine" not meet the same purpose?

My thought was "family" would cover a range of parts and "machine" was 
to be a specific SoC. Is a more specific ID needed?

> +{
> +	struct soc_ops *ops = soc_get_ops(dev);
> +
> +	if (!ops->get_soc_id)
> +		return -ENOSYS;
> +
> +	return ops->get_soc_id(dev, buf, size);
> +}
> +
>   const struct soc_attr *
>   soc_device_match(const struct soc_attr *matches)
>   {
> @@ -61,7 +71,7 @@ soc_device_match(const struct soc_attr *matches)
>   
>   	while (1) {
>   		if (!(matches->machine || matches->family ||
> -		      matches->revision))
> +		      matches->revision || matches->soc_id))
>   			break;
>   
>   		match = true;
> @@ -87,6 +97,13 @@ soc_device_match(const struct soc_attr *matches)
>   			}
>   		}
>   
> +		if (matches->soc_id) {
> +			if (!soc_get_soc_id(soc, str, SOC_MAX_STR_SIZE)) {
> +				if (strcmp(matches->soc_id, str))
> +					match = false;
> +			}
> +		}
> +
>   		if (match)
>   			return matches;
>   
> diff --git a/drivers/soc/soc_sandbox.c b/drivers/soc/soc_sandbox.c
> index 5c82ad84fc..1a81d3562a 100644
> --- a/drivers/soc/soc_sandbox.c
> +++ b/drivers/soc/soc_sandbox.c
> @@ -31,10 +31,18 @@ int soc_sandbox_get_revision(struct udevice *dev, char *buf, int size)
>   	return 0;
>   }
>   
> +int soc_sandbox_get_soc_id(struct udevice *dev, char *buf, int size)
> +{
> +	snprintf(buf, size, "r8a774a1");
> +
> +	return 0;
> +}
> +
>   static const struct soc_ops soc_sandbox_ops = {
>   	.get_family = soc_sandbox_get_family,
>   	.get_revision = soc_sandbox_get_revision,
>   	.get_machine = soc_sandbox_get_machine,
> +	.get_soc_id = soc_sandbox_get_soc_id,
>   };
>   
>   int soc_sandbox_probe(struct udevice *dev)
> diff --git a/include/soc.h b/include/soc.h
> index a55eb1b572..ae7b36846f 100644
> --- a/include/soc.h
> +++ b/include/soc.h
> @@ -20,18 +20,20 @@
>    *	       variants. Example: am33
>    * @machine  - Name of a specific SoC. Example: am3352
>    * @revision - Name of a specific SoC revision. Example: SR1.1
> + * @soc_id   - SoC identification String. Example: r8a774a1
>    * @data     - A pointer to user data for the SoC variant
>    */
>   struct soc_attr {
>   	const char *family;
>   	const char *machine;
>   	const char *revision;
> +	const char *soc_id;
>   	const void *data;
>   };
>   
>   struct soc_ops {
>   	/**
> -	 * get_machine() - Get machine name of an SOC
> +	 * get_machine() - Get machine name of a SoC
>   	 *
>   	 * @dev:	Device to check (UCLASS_SOC)
>   	 * @buf:	Buffer to place string
> @@ -41,7 +43,7 @@ struct soc_ops {
>   	int (*get_machine)(struct udevice *dev, char *buf, int size);
>   
>   	/**
> -	 * get_revision() - Get revision name of a SOC
> +	 * get_revision() - Get revision name of a SoC
>   	 *
>   	 * @dev:	Device to check (UCLASS_SOC)
>   	 * @buf:	Buffer to place string
> @@ -51,7 +53,7 @@ struct soc_ops {
>   	int (*get_revision)(struct udevice *dev, char *buf, int size);
>   
>   	/**
> -	 * get_family() - Get family name of an SOC
> +	 * get_family() - Get family name of a SoC
>   	 *
>   	 * @dev:	Device to check (UCLASS_SOC)
>   	 * @buf:	Buffer to place string
> @@ -59,6 +61,16 @@ struct soc_ops {
>   	 * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
>   	 */
>   	int (*get_family)(struct udevice *dev, char *buf, int size);
> +
> +	/**
> +	 * get_soc_id() - Get SoC identification 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_soc_id)(struct udevice *dev, char *buf, int size);
>   };
>   
>   #define soc_get_ops(dev)        ((struct soc_ops *)(dev)->driver->ops)
> @@ -76,7 +88,7 @@ struct soc_ops {
>   int soc_get(struct udevice **devp);
>   
>   /**
> - * soc_get_machine() - Get machine name of an SOC
> + * soc_get_machine() - Get machine name of a SoC
>    * @dev:	Device to check (UCLASS_SOC)
>    * @buf:	Buffer to place string
>    * @size:	Size of string space
> @@ -86,7 +98,7 @@ int soc_get(struct udevice **devp);
>   int soc_get_machine(struct udevice *dev, char *buf, int size);
>   
>   /**
> - * soc_get_revision() - Get revision name of an SOC
> + * soc_get_revision() - Get revision name of a SoC
>    * @dev:	Device to check (UCLASS_SOC)
>    * @buf:	Buffer to place string
>    * @size:	Size of string space
> @@ -96,7 +108,7 @@ int soc_get_machine(struct udevice *dev, char *buf, int size);
>   int soc_get_revision(struct udevice *dev, char *buf, int size);
>   
>   /**
> - * soc_get_family() - Get family name of an SOC
> + * soc_get_family() - Get family name of a SoC
>    * @dev:	Device to check (UCLASS_SOC)
>    * @buf:	Buffer to place string
>    * @size:	Size of string space
> @@ -105,6 +117,16 @@ int soc_get_revision(struct udevice *dev, char *buf, int size);
>    */
>   int soc_get_family(struct udevice *dev, char *buf, int size);
>   
> +/**
> + * soc_get_soc_id() - Get SoC identification 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 soc_get_soc_id(struct udevice *dev, char *buf, int size);
> +
>   /**
>    * soc_device_match() - Return match from an array of soc_attr
>    * @matches:	Array with any combination of family, revision or machine set
> @@ -136,6 +158,11 @@ static inline int soc_get_family(struct udevice *dev, char *buf, int size)
>   	return -ENOSYS;
>   }
>   
> +static inline int soc_get_soc_id(struct udevice *dev, char *buf, int size)
> +{
> +	return -ENOSYS;
> +}
> +
>   static inline const struct soc_attr *
>   soc_device_match(const struct soc_attr *matches)
>   {
> diff --git a/test/dm/soc.c b/test/dm/soc.c
> index 17e1b5ba01..82cd53ff29 100644
> --- a/test/dm/soc.c
> +++ b/test/dm/soc.c
> @@ -32,24 +32,28 @@ static int dm_test_soc(struct unit_test_state *uts)
>   			.family = "SANDBOX0xx",
>   			.machine = "SANDBOX012",
>   			.revision = "1.0",
> +			.soc_id = "r8a774a1",

Some more thought will need to be given here, you can't just add the new 
value everywhere, some should not match.

Regards,
Dave

>   			.data = NULL,
>   		},
>   		{
>   			.family = "SANDBOX1xx",
>   			.machine = "SANDBOX107",
>   			.revision = "1.0",
> +			.soc_id = "r8a774a1",
>   			.data = NULL,
>   		},
>   		{
>   			.family = "SANDBOX1xx",
>   			.machine = "SANDBOX123",
>   			.revision = "1.0",
> +			.soc_id = "r8a774a1",
>   			.data = &soc_sandbox123_data,
>   		},
>   		{
>   			.family = "SANDBOX1xx",
>   			.machine = "SANDBOX131",
>   			.revision = "2.0",
> +			.soc_id = "r8a774a1",
>   			.data = NULL,
>   		},
>   		{ /* sentinel */ }
> @@ -78,6 +82,7 @@ static int dm_test_soc(struct unit_test_state *uts)
>   		{
>   			.family = "SANDBOX0xx",
>   			.revision = "1.0",
> +			.soc_id = "r8a774b1",
>   			.data = NULL,
>   		},
>   		{
> @@ -99,6 +104,9 @@ static int dm_test_soc(struct unit_test_state *uts)
>   	ut_assertok(soc_get_revision(dev, text, sizeof(text)));
>   	ut_asserteq_str(text, "1.0");
>   
> +	ut_assertok(soc_get_soc_id(dev, text, sizeof(text)));
> +	ut_asserteq_str(text, "r8a774a1");
> +
>   	soc_data = soc_device_match(sb_soc_devices_full);
>   	ut_assert(soc_data);
>   
>

Hi Dave,

Thanks for the feedback.

> Subject: Re: [PATCH] dm: soc: Add SoC id for attribute matching
> 
> Hi,
> 
> On 10/30/20 9:07 AM, Biju Das wrote:
> > Add SoC identification string for attribute matching.
> > Also changed the comments from "an SOC" to "a SoC".
> 
> This is not a correct change, "an" should be used if the word that follows
> starts with a vowel sound, which "SoC" does.

Agreed. Will change back to an SoC on V2.

The below one still uses "a SOC". Will change to "an SoC" as well.
-	 * get_revision() - Get revision name of a SOC
+	 * get_revision() - Get revision name of a SoC

> "SOC" could be changed to "SoC", no strong feelings there.

OK.
> >
> > Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
> > Reviewed-by: Lad Prabhakar <prabhakar.mahadev-
> lad.rj@bp.renesas.com>
> > ---
> >   drivers/soc/soc-uclass.c  | 19 ++++++++++++++++++-
> >   drivers/soc/soc_sandbox.c |  8 ++++++++
> >   include/soc.h             | 39 +++++++++++++++++++++++++++++++++------
> >   test/dm/soc.c             |  8 ++++++++
> >   4 files changed, 67 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/soc/soc-uclass.c b/drivers/soc/soc-uclass.c index
> > c32d647864..a3f8be841b 100644
> > --- a/drivers/soc/soc-uclass.c
> > +++ b/drivers/soc/soc-uclass.c
> > @@ -46,6 +46,16 @@ int soc_get_revision(struct udevice *dev, char *buf,
> int size)
> >   	return ops->get_revision(dev, buf, size);
> >   }
> >
> > +int soc_get_soc_id(struct udevice *dev, char *buf, int size)
> Is there any additional background here? Why is soc_id needed? Can
> "machine" not meet the same purpose?

Renesas SoC and other SoC vendors use SoC_ID for SoC identification.See the details here [1]

[1]  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/renesas/renesas-soc.c?h=v5.10-rc1

Please see other SoC vendors as well [2] which uses SoC_ID for identification.
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/samsung/exynos-chipid.c?h=v5.10-rc1


> My thought was "family" would cover a range of parts and "machine" was to
> be a specific SoC. Is a more specific ID needed?

Yes, Please see above. 

Family in over case is:  RCar Gen3 SoC family or RZ/G2 SoC family
Machine is  Name of machine for eg;- "HopeRun HiHope RZ/G2M with sub board"
SoC_ID is some thing SoC specific for eg:- r8a774a1
Revision is : ES x.y.

I think in your case, it is different??

> 
> > +{
> > +	struct soc_ops *ops = soc_get_ops(dev);
> > +
> > +	if (!ops->get_soc_id)
> > +		return -ENOSYS;
> > +
> > +	return ops->get_soc_id(dev, buf, size); }
> > +
> >   const struct soc_attr *
> >   soc_device_match(const struct soc_attr *matches)
> >   {
> > @@ -61,7 +71,7 @@ soc_device_match(const struct soc_attr *matches)
> >
> >   	while (1) {
> >   		if (!(matches->machine || matches->family ||
> > -		      matches->revision))
> > +		      matches->revision || matches->soc_id))
> >   			break;
> >
> >   		match = true;
> > @@ -87,6 +97,13 @@ soc_device_match(const struct soc_attr *matches)
> >   			}
> >   		}
> >
> > +		if (matches->soc_id) {
> > +			if (!soc_get_soc_id(soc, str, SOC_MAX_STR_SIZE)) {
> > +				if (strcmp(matches->soc_id, str))
> > +					match = false;
> > +			}
> > +		}
> > +
> >   		if (match)
> >   			return matches;
> >
> > diff --git a/drivers/soc/soc_sandbox.c b/drivers/soc/soc_sandbox.c
> > index 5c82ad84fc..1a81d3562a 100644
> > --- a/drivers/soc/soc_sandbox.c
> > +++ b/drivers/soc/soc_sandbox.c
> > @@ -31,10 +31,18 @@ int soc_sandbox_get_revision(struct udevice *dev,
> char *buf, int size)
> >   	return 0;
> >   }
> >
> > +int soc_sandbox_get_soc_id(struct udevice *dev, char *buf, int size)
> > +{
> > +	snprintf(buf, size, "r8a774a1");
> > +
> > +	return 0;
> > +}
> > +
> >   static const struct soc_ops soc_sandbox_ops = {
> >   	.get_family = soc_sandbox_get_family,
> >   	.get_revision = soc_sandbox_get_revision,
> >   	.get_machine = soc_sandbox_get_machine,
> > +	.get_soc_id = soc_sandbox_get_soc_id,
> >   };
> >
> >   int soc_sandbox_probe(struct udevice *dev) diff --git
> > a/include/soc.h b/include/soc.h index a55eb1b572..ae7b36846f 100644
> > --- a/include/soc.h
> > +++ b/include/soc.h
> > @@ -20,18 +20,20 @@
> >    *	       variants. Example: am33
> >    * @machine  - Name of a specific SoC. Example: am3352
> >    * @revision - Name of a specific SoC revision. Example: SR1.1
> > + * @soc_id   - SoC identification String. Example: r8a774a1
> >    * @data     - A pointer to user data for the SoC variant
> >    */
> >   struct soc_attr {
> >   	const char *family;
> >   	const char *machine;
> >   	const char *revision;
> > +	const char *soc_id;
> >   	const void *data;
> >   };
> >
> >   struct soc_ops {
> >   	/**
> > -	 * get_machine() - Get machine name of an SOC
> > +	 * get_machine() - Get machine name of a SoC
> >   	 *
> >   	 * @dev:	Device to check (UCLASS_SOC)
> >   	 * @buf:	Buffer to place string
> > @@ -41,7 +43,7 @@ struct soc_ops {
> >   	int (*get_machine)(struct udevice *dev, char *buf, int size);
> >
> >   	/**
> > -	 * get_revision() - Get revision name of a SOC
> > +	 * get_revision() - Get revision name of a SoC
> >   	 *
> >   	 * @dev:	Device to check (UCLASS_SOC)
> >   	 * @buf:	Buffer to place string
> > @@ -51,7 +53,7 @@ struct soc_ops {
> >   	int (*get_revision)(struct udevice *dev, char *buf, int size);
> >
> >   	/**
> > -	 * get_family() - Get family name of an SOC
> > +	 * get_family() - Get family name of a SoC
> >   	 *
> >   	 * @dev:	Device to check (UCLASS_SOC)
> >   	 * @buf:	Buffer to place string
> > @@ -59,6 +61,16 @@ struct soc_ops {
> >   	 * @return 0 if OK, -ENOSPC if buffer is too small, other -ve on error
> >   	 */
> >   	int (*get_family)(struct udevice *dev, char *buf, int size);
> > +
> > +	/**
> > +	 * get_soc_id() - Get SoC identification 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_soc_id)(struct udevice *dev, char *buf, int size);
> >   };
> >
> >   #define soc_get_ops(dev)        ((struct soc_ops *)(dev)->driver->ops)
> > @@ -76,7 +88,7 @@ struct soc_ops {
> >   int soc_get(struct udevice **devp);
> >
> >   /**
> > - * soc_get_machine() - Get machine name of an SOC
> > + * soc_get_machine() - Get machine name of a SoC
> >    * @dev:	Device to check (UCLASS_SOC)
> >    * @buf:	Buffer to place string
> >    * @size:	Size of string space
> > @@ -86,7 +98,7 @@ int soc_get(struct udevice **devp);
> >   int soc_get_machine(struct udevice *dev, char *buf, int size);
> >
> >   /**
> > - * soc_get_revision() - Get revision name of an SOC
> > + * soc_get_revision() - Get revision name of a SoC
> >    * @dev:	Device to check (UCLASS_SOC)
> >    * @buf:	Buffer to place string
> >    * @size:	Size of string space
> > @@ -96,7 +108,7 @@ int soc_get_machine(struct udevice *dev, char *buf,
> int size);
> >   int soc_get_revision(struct udevice *dev, char *buf, int size);
> >
> >   /**
> > - * soc_get_family() - Get family name of an SOC
> > + * soc_get_family() - Get family name of a SoC
> >    * @dev:	Device to check (UCLASS_SOC)
> >    * @buf:	Buffer to place string
> >    * @size:	Size of string space
> > @@ -105,6 +117,16 @@ int soc_get_revision(struct udevice *dev, char
> *buf, int size);
> >    */
> >   int soc_get_family(struct udevice *dev, char *buf, int size);
> >
> > +/**
> > + * soc_get_soc_id() - Get SoC identification 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 soc_get_soc_id(struct udevice *dev, char *buf, int
> > +size);
> > +
> >   /**
> >    * soc_device_match() - Return match from an array of soc_attr
> >    * @matches:	Array with any combination of family, revision or
> machine set
> > @@ -136,6 +158,11 @@ static inline int soc_get_family(struct udevice
> *dev, char *buf, int size)
> >   	return -ENOSYS;
> >   }
> >
> > +static inline int soc_get_soc_id(struct udevice *dev, char *buf, int
> > +size) {
> > +	return -ENOSYS;
> > +}
> > +
> >   static inline const struct soc_attr *
> >   soc_device_match(const struct soc_attr *matches)
> >   {
> > diff --git a/test/dm/soc.c b/test/dm/soc.c index
> > 17e1b5ba01..82cd53ff29 100644
> > --- a/test/dm/soc.c
> > +++ b/test/dm/soc.c
> > @@ -32,24 +32,28 @@ static int dm_test_soc(struct unit_test_state *uts)
> >   			.family = "SANDBOX0xx",
> >   			.machine = "SANDBOX012",
> >   			.revision = "1.0",
> > +			.soc_id = "r8a774a1",
> 
> Some more thought will need to be given here, you can't just add the new
> value everywhere, some should not match.

I have ran the ut_dm test case and is passing.

./test/py/test.py --bd sandbox --build -k ut_dm.

Please let me know, if I should remove the same.

Regards,
Biju

> 
> >   			.data = NULL,
> >   		},
> >   		{
> >   			.family = "SANDBOX1xx",
> >   			.machine = "SANDBOX107",
> >   			.revision = "1.0",
> > +			.soc_id = "r8a774a1",
> >   			.data = NULL,
> >   		},
> >   		{
> >   			.family = "SANDBOX1xx",
> >   			.machine = "SANDBOX123",
> >   			.revision = "1.0",
> > +			.soc_id = "r8a774a1",
> >   			.data = &soc_sandbox123_data,
> >   		},
> >   		{
> >   			.family = "SANDBOX1xx",
> >   			.machine = "SANDBOX131",
> >   			.revision = "2.0",
> > +			.soc_id = "r8a774a1",
> >   			.data = NULL,
> >   		},
> >   		{ /* sentinel */ }
> > @@ -78,6 +82,7 @@ static int dm_test_soc(struct unit_test_state *uts)
> >   		{
> >   			.family = "SANDBOX0xx",
> >   			.revision = "1.0",
> > +			.soc_id = "r8a774b1",
> >   			.data = NULL,
> >   		},
> >   		{
> > @@ -99,6 +104,9 @@ static int dm_test_soc(struct unit_test_state *uts)
> >   	ut_assertok(soc_get_revision(dev, text, sizeof(text)));
> >   	ut_asserteq_str(text, "1.0");
> >
> > +	ut_assertok(soc_get_soc_id(dev, text, sizeof(text)));
> > +	ut_asserteq_str(text, "r8a774a1");
> > +
> >   	soc_data = soc_device_match(sb_soc_devices_full);
> >   	ut_assert(soc_data);
> >
> >