[PATCH 0/2] cmd/sbi: add missing SBI information

[PATCH 0/2] cmd/sbi: add missing SBI information

[Heinrich Schuchardt]

The series provides library functions to read

* SBI implementation version
* machine vendor ID
* machine architecture ID
* machine implementation ID

and enhances the sbi command to display this information.

Heinrich Schuchardt (2):
  riscv: provide missing base extension functions
  cmd/sbi: add missing SBI information

 arch/riscv/include/asm/sbi.h |  4 ++
 arch/riscv/lib/sbi.c         | 72 ++++++++++++++++++++++++++++++++++++
 cmd/riscv/sbi.c              | 19 +++++++++-
 3 files changed, 94 insertions(+), 1 deletion(-)

--
2.30.2

[PATCH 1/2] riscv: provide missing base extension functions

[Heinrich Schuchardt]

Provide library functions to read:

* SBI implementation version
* machine vendor ID
* machine architecture ID
* machine implementation ID

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 arch/riscv/include/asm/sbi.h |  4 ++
 arch/riscv/lib/sbi.c         | 71 ++++++++++++++++++++++++++++++++++++
 2 files changed, 75 insertions(+)

diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
index 53ca316180..262cc9228c 100644
--- a/arch/riscv/include/asm/sbi.h
+++ b/arch/riscv/include/asm/sbi.h
@@ -117,6 +117,10 @@ void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
 void sbi_set_timer(uint64_t stime_value);
 long sbi_get_spec_version(void);
 int sbi_get_impl_id(void);
+long sbi_get_impl_version(void);
 int sbi_probe_extension(int ext);
+long sbi_get_mvendorid(void);
+long sbi_get_marchid(void);
+long sbi_get_mimpid(void);

 #endif
diff --git a/arch/riscv/lib/sbi.c b/arch/riscv/lib/sbi.c
index 77845a73ca..8b8b4b278f 100644
--- a/arch/riscv/lib/sbi.c
+++ b/arch/riscv/lib/sbi.c
@@ -89,6 +89,23 @@ int sbi_get_impl_id(void)
 	return -ENOTSUPP;
 }

+/**
+ * sbi_get_impl_version() - get SBI implementation version
+ *
+ * Return: implementation version
+ */
+long sbi_get_impl_version(void)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION,
+			0, 0, 0, 0, 0, 0);
+	if (!ret.error)
+		return ret.value;
+
+	return -ENOTSUPP;
+}
+
 /**
  * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
  * @extid: The extension ID to be probed.
@@ -108,6 +125,60 @@ int sbi_probe_extension(int extid)
 	return -ENOTSUPP;
 }

+/**
+ * sbi_get_mvendorid() - get machine vendor ID
+ *
+ * Return: implementation vendor ID
+ */
+long sbi_get_mvendorid(void)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID,
+			0, 0, 0, 0, 0, 0);
+	if (!ret.error)
+		if (ret.value)
+			return ret.value;
+
+	return -ENOTSUPP;
+}
+
+/**
+ * sbi_get_marchid() - get machine architecture ID
+ *
+ * Return: implementation version
+ */
+long sbi_get_marchid(void)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MARCHID,
+			0, 0, 0, 0, 0, 0);
+	if (!ret.error)
+		if (ret.value)
+			return ret.value;
+
+	return -ENOTSUPP;
+}
+
+/**
+ * sbi_get_mimpid() - get machine implementation ID
+ *
+ * Return: machine implementation ID
+ */
+long sbi_get_mimpid(void)
+{
+	struct sbiret ret;
+
+	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MIMPID,
+			0, 0, 0, 0, 0, 0);
+	if (!ret.error)
+		if (ret.value)
+			return ret.value;
+
+	return -ENOTSUPP;
+}
+
 #ifdef CONFIG_SBI_V01

 /**
--
2.30.2

Re: [PATCH 1/2] riscv: provide missing base extension functions

[Sean Anderson]

On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
> Provide library functions to read:
> 
> * SBI implementation version
> * machine vendor ID
> * machine architecture ID
> * machine implementation ID
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>   arch/riscv/include/asm/sbi.h |  4 ++
>   arch/riscv/lib/sbi.c         | 71 ++++++++++++++++++++++++++++++++++++
>   2 files changed, 75 insertions(+)
> 
> diff --git a/arch/riscv/include/asm/sbi.h b/arch/riscv/include/asm/sbi.h
> index 53ca316180..262cc9228c 100644
> --- a/arch/riscv/include/asm/sbi.h
> +++ b/arch/riscv/include/asm/sbi.h
> @@ -117,6 +117,10 @@ void sbi_remote_sfence_vma_asid(const unsigned long *hart_mask,
>   void sbi_set_timer(uint64_t stime_value);
>   long sbi_get_spec_version(void);
>   int sbi_get_impl_id(void);
> +long sbi_get_impl_version(void);
>   int sbi_probe_extension(int ext);
> +long sbi_get_mvendorid(void);
> +long sbi_get_marchid(void);
> +long sbi_get_mimpid(void);
> 
>   #endif
> diff --git a/arch/riscv/lib/sbi.c b/arch/riscv/lib/sbi.c
> index 77845a73ca..8b8b4b278f 100644
> --- a/arch/riscv/lib/sbi.c
> +++ b/arch/riscv/lib/sbi.c
> @@ -89,6 +89,23 @@ int sbi_get_impl_id(void)
>   	return -ENOTSUPP;
>   }
> 
> +/**
> + * sbi_get_impl_version() - get SBI implementation version
> + *
> + * Return: implementation version
> + */
> +long sbi_get_impl_version(void)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_IMP_VERSION,
> +			0, 0, 0, 0, 0, 0);
> +	if (!ret.error)
> +		return ret.value;
> +
> +	return -ENOTSUPP;
> +}
> +
>   /**
>    * sbi_probe_extension() - Check if an SBI extension ID is supported or not.
>    * @extid: The extension ID to be probed.
> @@ -108,6 +125,60 @@ int sbi_probe_extension(int extid)
>   	return -ENOTSUPP;
>   }
> 
> +/**
> + * sbi_get_mvendorid() - get machine vendor ID
> + *
> + * Return: implementation vendor ID
> + */
> +long sbi_get_mvendorid(void)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MVENDORID,
> +			0, 0, 0, 0, 0, 0);
> +	if (!ret.error)
> +		if (ret.value)
> +			return ret.value;
> +
> +	return -ENOTSUPP;
> +}
> +
> +/**
> + * sbi_get_marchid() - get machine architecture ID
> + *
> + * Return: implementation version
> + */
> +long sbi_get_marchid(void)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MARCHID,
> +			0, 0, 0, 0, 0, 0);
> +	if (!ret.error)
> +		if (ret.value)
> +			return ret.value;
> +
> +	return -ENOTSUPP;
> +}
> +
> +/**
> + * sbi_get_mimpid() - get machine implementation ID
> + *
> + * Return: machine implementation ID
> + */
> +long sbi_get_mimpid(void)
> +{
> +	struct sbiret ret;
> +
> +	ret = sbi_ecall(SBI_EXT_BASE, SBI_EXT_BASE_GET_MIMPID,
> +			0, 0, 0, 0, 0, 0);
> +	if (!ret.error)
> +		if (ret.value)
> +			return ret.value;
> +
> +	return -ENOTSUPP;
> +}
> +
>   #ifdef CONFIG_SBI_V01
> 
>   /**
> --
> 2.30.2
> 

Reviewed-by: Sean Anderson <seanga2@gmail.com>

Re: [PATCH 1/2] riscv: provide missing base extension functions

[Leo Liang]

On Tue, Jul 20, 2021 at 04:28:34AM +0800, Heinrich Schuchardt wrote:
> Provide library functions to read:
> 
> * SBI implementation version
> * machine vendor ID
> * machine architecture ID
> * machine implementation ID
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Reviewed-by: Leo Yu-Chi Liang <ycliang@andestech.com>

[PATCH 2/2] cmd/sbi: add missing SBI information

[Heinrich Schuchardt]

Let the sbi command display:

* SBI implementation version
* machine vendor ID
* machine architecture ID
* machine implementation ID

With this patch the output for the HiFive Unmatched looks like

    => sbi
    SBI 0.3
    OpenSBI 0.9
    Machine:
      Vendor ID 489
      Architecture ID 8000000000000007
      Implementation ID 20181004
    Extensions:
      sbi_set_timer
      sbi_console_putchar
      sbi_console_getchar
      sbi_clear_ipi
      sbi_send_ipi
      sbi_remote_fence_i
      sbi_remote_sfence_vma
      sbi_remote_sfence_vma_asid
      sbi_shutdown
      SBI Base Functionality
      Timer Extension
      IPI Extension
      RFENCE Extension
      Hart State Management Extension
      System Reset Extension

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
 cmd/riscv/sbi.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
index 90c0811e14..c0db763ba7 100644
--- a/cmd/riscv/sbi.c
+++ b/cmd/riscv/sbi.c
@@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int flag, int argc,
 	if (ret >= 0) {
 		for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
 			if (ret == implementations[i].id) {
-				printf("%s\n", implementations[i].name);
+				printf("%s", implementations[i].name);
+				ret = sbi_get_impl_version();
+				if (ret > 0) {
+					/* OpenSBI specific version encoding */
+					printf(" %ld", ret >> 16);
+					printf(".%ld", ret & 0xffff);
+				}
+				printf("\n");
 				break;
 			}
 		}
 		if (i == ARRAY_SIZE(implementations))
 			printf("Unknown implementation ID %ld\n", ret);
 	}
+	printf("Machine:\n");
+	ret = sbi_get_mvendorid();
+	if (ret != -ENOTSUPP)
+		printf("  Vendor ID %lx\n", ret);
+	ret = sbi_get_marchid();
+	if (ret != -ENOTSUPP)
+		printf("  Architecture ID %lx\n", ret);
+	ret = sbi_get_mimpid();
+	if (ret != -ENOTSUPP)
+		printf("  Implementation ID %lx\n", ret);
 	printf("Extensions:\n");
 	for (i = 0; i < ARRAY_SIZE(extensions); ++i) {
 		ret = sbi_probe_extension(extensions[i].id);
--
2.30.2

Re: [PATCH 2/2] cmd/sbi: add missing SBI information

[Sean Anderson]

On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
> Let the sbi command display:
> 
> * SBI implementation version
> * machine vendor ID
> * machine architecture ID
> * machine implementation ID
> 
> With this patch the output for the HiFive Unmatched looks like
> 
>      => sbi
>      SBI 0.3
>      OpenSBI 0.9
>      Machine:
>        Vendor ID 489
>        Architecture ID 8000000000000007
>        Implementation ID 20181004
>      Extensions:
>        sbi_set_timer
>        sbi_console_putchar
>        sbi_console_getchar
>        sbi_clear_ipi
>        sbi_send_ipi
>        sbi_remote_fence_i
>        sbi_remote_sfence_vma
>        sbi_remote_sfence_vma_asid
>        sbi_shutdown
>        SBI Base Functionality
>        Timer Extension
>        IPI Extension
>        RFENCE Extension
>        Hart State Management Extension
>        System Reset Extension
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
>   cmd/riscv/sbi.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
> index 90c0811e14..c0db763ba7 100644
> --- a/cmd/riscv/sbi.c
> +++ b/cmd/riscv/sbi.c
> @@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int flag, int argc,
>   	if (ret >= 0) {
>   		for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
>   			if (ret == implementations[i].id) {
> -				printf("%s\n", implementations[i].name);
> +				printf("%s", implementations[i].name);
> +				ret = sbi_get_impl_version();
> +				if (ret > 0) {

Shouldn't this have to check to ensure that i == 1?

> +					/* OpenSBI specific version encoding */
> +					printf(" %ld", ret >> 16);
> +					printf(".%ld", ret & 0xffff);
> +				}
> +				printf("\n");
>   				break;
>   			}
>   		}
>   		if (i == ARRAY_SIZE(implementations))
>   			printf("Unknown implementation ID %ld\n", ret);
>   	}
> +	printf("Machine:\n");
> +	ret = sbi_get_mvendorid();
> +	if (ret != -ENOTSUPP)
> +		printf("  Vendor ID %lx\n", ret);

perhaps %0.8lx? And should we decode the JEDEC id?

> +	ret = sbi_get_marchid();
> +	if (ret != -ENOTSUPP)
> +		printf("  Architecture ID %lx\n", ret);
> +	ret = sbi_get_mimpid();
> +	if (ret != -ENOTSUPP)
> +		printf("  Implementation ID %lx\n", ret);
>   	printf("Extensions:\n");
>   	for (i = 0; i < ARRAY_SIZE(extensions); ++i) {
>   		ret = sbi_probe_extension(extensions[i].id);
> --
> 2.30.2
> 

Re: [PATCH 2/2] cmd/sbi: add missing SBI information

[Heinrich Schuchardt]

Am 20. Juli 2021 03:11:34 MESZ schrieb Sean Anderson <seanga2@gmail.com>:
>On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
>> Let the sbi command display:
>> 
>> * SBI implementation version
>> * machine vendor ID
>> * machine architecture ID
>> * machine implementation ID
>> 
>> With this patch the output for the HiFive Unmatched looks like
>> 
>>      => sbi
>>      SBI 0.3
>>      OpenSBI 0.9
>>      Machine:
>>        Vendor ID 489
>>        Architecture ID 8000000000000007
>>        Implementation ID 20181004
>>      Extensions:
>>        sbi_set_timer
>>        sbi_console_putchar
>>        sbi_console_getchar
>>        sbi_clear_ipi
>>        sbi_send_ipi
>>        sbi_remote_fence_i
>>        sbi_remote_sfence_vma
>>        sbi_remote_sfence_vma_asid
>>        sbi_shutdown
>>        SBI Base Functionality
>>        Timer Extension
>>        IPI Extension
>>        RFENCE Extension
>>        Hart State Management Extension
>>        System Reset Extension
>> 
>> Signed-off-by: Heinrich Schuchardt
><heinrich.schuchardt@canonical.com>
>> ---
>>   cmd/riscv/sbi.c | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 1 deletion(-)
>> 
>> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
>> index 90c0811e14..c0db763ba7 100644
>> --- a/cmd/riscv/sbi.c
>> +++ b/cmd/riscv/sbi.c
>> @@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int
>flag, int argc,
>>   	if (ret >= 0) {
>>   		for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
>>   			if (ret == implementations[i].id) {
>> -				printf("%s\n", implementations[i].name);
>> +				printf("%s", implementations[i].name);
>> +				ret = sbi_get_impl_version();
>> +				if (ret > 0) {
>
>Shouldn't this have to check to ensure that i == 1?

Other SBI implementions may also provide a version number. I did not analyze how the should be formatted.

>
>> +					/* OpenSBI specific version encoding */
>> +					printf(" %ld", ret >> 16);
>> +					printf(".%ld", ret & 0xffff);
>> +				}
>> +				printf("\n");
>>   				break;
>>   			}
>>   		}
>>   		if (i == ARRAY_SIZE(implementations))
>>   			printf("Unknown implementation ID %ld\n", ret);
>>   	}
>> +	printf("Machine:\n");
>> +	ret = sbi_get_mvendorid();
>> +	if (ret != -ENOTSUPP)

Should we use an unsigned int as return value and 0 to indicate a missing implementation? mvendorid is only 32 bits wide.

>> +		printf("  Vendor ID %lx\n", ret);
>
>perhaps %0.8lx? And should we decode the JEDEC id?

Leading zeros won't add any meaning here. Splitting into the 25 bit and 7 bit subfields could be reasonable.

Decoding could result in up to 2^26 digits. I don't want to wait for all of these on my serial console.

Best regards

Heinrich

>
>> +	ret = sbi_get_marchid();
>> +	if (ret != -ENOTSUPP)
>> +		printf("  Architecture ID %lx\n", ret);
>> +	ret = sbi_get_mimpid();
>> +	if (ret != -ENOTSUPP)
>> +		printf("  Implementation ID %lx\n", ret);
>>   	printf("Extensions:\n");
>>   	for (i = 0; i < ARRAY_SIZE(extensions); ++i) {
>>   		ret = sbi_probe_extension(extensions[i].id);
>> --
>> 2.30.2
>> 

Re: [PATCH 2/2] cmd/sbi: add missing SBI information

[Sean Anderson]

On 7/20/21 12:59 AM, Heinrich Schuchardt wrote:
> Am 20. Juli 2021 03:11:34 MESZ schrieb Sean Anderson <seanga2@gmail.com>:
>> On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
>>> Let the sbi command display:
>>>
>>> * SBI implementation version
>>> * machine vendor ID
>>> * machine architecture ID
>>> * machine implementation ID
>>>
>>> With this patch the output for the HiFive Unmatched looks like
>>>
>>>       => sbi
>>>       SBI 0.3
>>>       OpenSBI 0.9
>>>       Machine:
>>>         Vendor ID 489
>>>         Architecture ID 8000000000000007
>>>         Implementation ID 20181004
>>>       Extensions:
>>>         sbi_set_timer
>>>         sbi_console_putchar
>>>         sbi_console_getchar
>>>         sbi_clear_ipi
>>>         sbi_send_ipi
>>>         sbi_remote_fence_i
>>>         sbi_remote_sfence_vma
>>>         sbi_remote_sfence_vma_asid
>>>         sbi_shutdown
>>>         SBI Base Functionality
>>>         Timer Extension
>>>         IPI Extension
>>>         RFENCE Extension
>>>         Hart State Management Extension
>>>         System Reset Extension
>>>
>>> Signed-off-by: Heinrich Schuchardt
>> <heinrich.schuchardt@canonical.com>
>>> ---
>>>    cmd/riscv/sbi.c | 19 ++++++++++++++++++-
>>>    1 file changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
>>> index 90c0811e14..c0db763ba7 100644
>>> --- a/cmd/riscv/sbi.c
>>> +++ b/cmd/riscv/sbi.c
>>> @@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int
>> flag, int argc,
>>>    	if (ret >= 0) {
>>>    		for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
>>>    			if (ret == implementations[i].id) {
>>> -				printf("%s\n", implementations[i].name);
>>> +				printf("%s", implementations[i].name);
>>> +				ret = sbi_get_impl_version();
>>> +				if (ret > 0) {
>>
>> Shouldn't this have to check to ensure that i == 1?
> 
> Other SBI implementions may also provide a version number. I did not analyze how the should be formatted.

Right, so shouldn't we default to the raw hex string?

> 
>>
>>> +					/* OpenSBI specific version encoding */
>>> +					printf(" %ld", ret >> 16);
>>> +					printf(".%ld", ret & 0xffff);
>>> +				}
>>> +				printf("\n");
>>>    				break;
>>>    			}
>>>    		}
>>>    		if (i == ARRAY_SIZE(implementations))
>>>    			printf("Unknown implementation ID %ld\n", ret);
>>>    	}
>>> +	printf("Machine:\n");
>>> +	ret = sbi_get_mvendorid();
>>> +	if (ret != -ENOTSUPP)
> 
> Should we use an unsigned int as return value and 0 to indicate a missing implementation? mvendorid is only 32 bits wide.
> 
>>> +		printf("  Vendor ID %lx\n", ret);
>>
>> perhaps %0.8lx? And should we decode the JEDEC id?
> 
> Leading zeros won't add any meaning here. Splitting into the 25 bit and 7 bit subfields could be reasonable.

I think this would be a good option.

> Decoding could result in up to 2^26 digits. I don't want to wait for all of these on my serial console.

Well, they're only up to 12 continuation codes, so imposing a small maximum would likely not be too onerous.

--Sean

Re: [PATCH 2/2] cmd/sbi: add missing SBI information

[Leo Liang]

On Tue, Jul 20, 2021 at 01:13:56PM +0800, Sean Anderson wrote:
> On 7/20/21 12:59 AM, Heinrich Schuchardt wrote:
> > Am 20. Juli 2021 03:11:34 MESZ schrieb Sean Anderson <seanga2@gmail.com>:
> >> On 7/19/21 4:28 PM, Heinrich Schuchardt wrote:
> >>> Let the sbi command display:
> >>>
> >>> * SBI implementation version
> >>> * machine vendor ID
> >>> * machine architecture ID
> >>> * machine implementation ID
> >>>
> >>> With this patch the output for the HiFive Unmatched looks like
> >>>
> >>>       => sbi
> >>>       SBI 0.3
> >>>       OpenSBI 0.9
> >>>       Machine:
> >>>         Vendor ID 489
> >>>         Architecture ID 8000000000000007
> >>>         Implementation ID 20181004
> >>>       Extensions:
> >>>         sbi_set_timer
> >>>         sbi_console_putchar
> >>>         sbi_console_getchar
> >>>         sbi_clear_ipi
> >>>         sbi_send_ipi
> >>>         sbi_remote_fence_i
> >>>         sbi_remote_sfence_vma
> >>>         sbi_remote_sfence_vma_asid
> >>>         sbi_shutdown
> >>>         SBI Base Functionality
> >>>         Timer Extension
> >>>         IPI Extension
> >>>         RFENCE Extension
> >>>         Hart State Management Extension
> >>>         System Reset Extension
> >>>
> >>> Signed-off-by: Heinrich Schuchardt
> >> <heinrich.schuchardt@canonical.com>
> >>> ---
> >>>    cmd/riscv/sbi.c | 19 ++++++++++++++++++-
> >>>    1 file changed, 18 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/cmd/riscv/sbi.c b/cmd/riscv/sbi.c
> >>> index 90c0811e14..c0db763ba7 100644
> >>> --- a/cmd/riscv/sbi.c
> >>> +++ b/cmd/riscv/sbi.c
> >>> @@ -59,13 +59,30 @@ static int do_sbi(struct cmd_tbl *cmdtp, int
> >> flag, int argc,
> >>>    	if (ret >= 0) {
> >>>    		for (i = 0; i < ARRAY_SIZE(implementations); ++i) {
> >>>    			if (ret == implementations[i].id) {
> >>> -				printf("%s\n", implementations[i].name);
> >>> +				printf("%s", implementations[i].name);
> >>> +				ret = sbi_get_impl_version();
> >>> +				if (ret > 0) {
> >>
> >> Shouldn't this have to check to ensure that i == 1?
> > 
> > Other SBI implementions may also provide a version number. I did not analyze how the should be formatted.
> 
> Right, so shouldn't we default to the raw hex string?

+1

> 
> > 
> >>
> >>> +					/* OpenSBI specific version encoding */
> >>> +					printf(" %ld", ret >> 16);
> >>> +					printf(".%ld", ret & 0xffff);
> >>> +				}
> >>> +				printf("\n");
> >>>    				break;
> >>>    			}
> >>>    		}
> >>>    		if (i == ARRAY_SIZE(implementations))
> >>>    			printf("Unknown implementation ID %ld\n", ret);
> >>>    	}
> >>> +	printf("Machine:\n");
> >>> +	ret = sbi_get_mvendorid();
> >>> +	if (ret != -ENOTSUPP)
> > 
> > Should we use an unsigned int as return value and 0 to indicate a missing implementation? mvendorid is only 32 bits wide.
> > 
> >>> +		printf("  Vendor ID %lx\n", ret);
> >>
> >> perhaps %0.8lx? And should we decode the JEDEC id?
> > 
> > Leading zeros won't add any meaning here. Splitting into the 25 bit and 7 bit subfields could be reasonable.
> 
> I think this would be a good option.

+1 with 25, 7 split.
IMHO, perhaps we could show only the "numbers" of the continuation code and the "decoded offset" ?
Something like: vendor ID: 489, continuation code: 9, offset decoded: 0x89.

Best regards,
Leo

> 
> > Decoding could result in up to 2^26 digits. I don't want to wait for all of these on my serial console.
> 
> Well, they're only up to 12 continuation codes, so imposing a small maximum would likely not be too onerous.
> 
> --Sean