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/driv...

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/driv...

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);