diff mbox series

[U-Boot,3/5] zynq: Support CPU info display

Message ID 20180117135626.22498-4-ezequiel@vanguardiasur.com.ar
State Superseded
Delegated to: Michal Simek
Headers show
Series zynq: Fun with board and CPU info display | expand

Commit Message

Ezequiel Garcia Jan. 17, 2018, 1:56 p.m. UTC 
This commit adds CPU and silicon version information
consuming the SLCR IDCODE and DEVCFG MCTRL registers,
respectively.

Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
---
 arch/arm/mach-zynq/cpu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Michal Simek Jan. 26, 2018, 12:33 p.m. UTC | #1 
Hi,


On 17.1.2018 14:56, Ezequiel Garcia wrote:
> This commit adds CPU and silicon version information
> consuming the SLCR IDCODE and DEVCFG MCTRL registers,
> respectively.
> 
> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
> ---
>  arch/arm/mach-zynq/cpu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
> 
> diff --git a/arch/arm/mach-zynq/cpu.c b/arch/arm/mach-zynq/cpu.c
> index 53a07b0059c2..602f483c162b 100644
> --- a/arch/arm/mach-zynq/cpu.c
> +++ b/arch/arm/mach-zynq/cpu.c
> @@ -35,6 +35,25 @@ static const struct {
>  };
>  #endif
>  
> +#ifdef CONFIG_DISPLAY_CPUINFO
> +static const struct {
> +	u8 idcode;
> +	const char *cpuinfo;
> +} zynq_cpu_info[] = {
> +	{ .idcode = XILINX_ZYNQ_7007S,	.cpuinfo = XILINX_XC7Z007S_NAME },
> +	{ .idcode = XILINX_ZYNQ_7010,	.cpuinfo = XILINX_XC7Z010_NAME },
> +	{ .idcode = XILINX_ZYNQ_7012S,	.cpuinfo = XILINX_XC7Z012S_NAME },
> +	{ .idcode = XILINX_ZYNQ_7014S,	.cpuinfo = XILINX_XC7Z014S_NAME },
> +	{ .idcode = XILINX_ZYNQ_7015,	.cpuinfo = XILINX_XC7Z015_NAME },
> +	{ .idcode = XILINX_ZYNQ_7020,	.cpuinfo = XILINX_XC7Z020_NAME },
> +	{ .idcode = XILINX_ZYNQ_7030,	.cpuinfo = XILINX_XC7Z030_NAME },
> +	{ .idcode = XILINX_ZYNQ_7035,	.cpuinfo = XILINX_XC7Z035_NAME },
> +	{ .idcode = XILINX_ZYNQ_7045,	.cpuinfo = XILINX_XC7Z045_NAME },
> +	{ .idcode = XILINX_ZYNQ_7100,	.cpuinfo = XILINX_XC7Z100_NAME },
> +	{ /* Sentinel */ },

This table pretty much reflect what it is in 2/5.

static const struct {
	u8 idcode;
	const char *cpuinfo; /* or better name devicename */
	u32 fpga_size;
} zynq_cpu_info[] = {

From xilinx_desc I think size is unused but we can keep in filled and
cookie is also not used and can be 0. The rest of data for xilinx_desc
is static anyway.

It means doing this properly will be the best to fill that xilinx_desc
and also it doesn't make sense to call zynq_slcr_get_idcode() twice.
It should be enough to detect chip once and fill pointer to actual
configuration. And then when fpga should be add then use them. The same
for cpuinfo. Link is already setup and you can just use it.

Thanks,
Michal
Ezequiel Garcia Jan. 31, 2018, 9:25 p.m. UTC | #2 
On 26 January 2018 at 09:33, Michal Simek <michal.simek@xilinx.com> wrote:
> Hi,
>
>
> On 17.1.2018 14:56, Ezequiel Garcia wrote:
>> This commit adds CPU and silicon version information
>> consuming the SLCR IDCODE and DEVCFG MCTRL registers,
>> respectively.
>>
>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>> ---
>>  arch/arm/mach-zynq/cpu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>  1 file changed, 46 insertions(+)
>>
>> diff --git a/arch/arm/mach-zynq/cpu.c b/arch/arm/mach-zynq/cpu.c
>> index 53a07b0059c2..602f483c162b 100644
>> --- a/arch/arm/mach-zynq/cpu.c
>> +++ b/arch/arm/mach-zynq/cpu.c
>> @@ -35,6 +35,25 @@ static const struct {
>>  };
>>  #endif
>>
>> +#ifdef CONFIG_DISPLAY_CPUINFO
>> +static const struct {
>> +     u8 idcode;
>> +     const char *cpuinfo;
>> +} zynq_cpu_info[] = {
>> +     { .idcode = XILINX_ZYNQ_7007S,  .cpuinfo = XILINX_XC7Z007S_NAME },
>> +     { .idcode = XILINX_ZYNQ_7010,   .cpuinfo = XILINX_XC7Z010_NAME },
>> +     { .idcode = XILINX_ZYNQ_7012S,  .cpuinfo = XILINX_XC7Z012S_NAME },
>> +     { .idcode = XILINX_ZYNQ_7014S,  .cpuinfo = XILINX_XC7Z014S_NAME },
>> +     { .idcode = XILINX_ZYNQ_7015,   .cpuinfo = XILINX_XC7Z015_NAME },
>> +     { .idcode = XILINX_ZYNQ_7020,   .cpuinfo = XILINX_XC7Z020_NAME },
>> +     { .idcode = XILINX_ZYNQ_7030,   .cpuinfo = XILINX_XC7Z030_NAME },
>> +     { .idcode = XILINX_ZYNQ_7035,   .cpuinfo = XILINX_XC7Z035_NAME },
>> +     { .idcode = XILINX_ZYNQ_7045,   .cpuinfo = XILINX_XC7Z045_NAME },
>> +     { .idcode = XILINX_ZYNQ_7100,   .cpuinfo = XILINX_XC7Z100_NAME },
>> +     { /* Sentinel */ },
>
> This table pretty much reflect what it is in 2/5.
>
> static const struct {
>         u8 idcode;
>         const char *cpuinfo; /* or better name devicename */
>         u32 fpga_size;
> } zynq_cpu_info[] = {
>
> From xilinx_desc I think size is unused but we can keep in filled and
> cookie is also not used and can be 0. The rest of data for xilinx_desc
> is static anyway.
>
> It means doing this properly will be the best to fill that xilinx_desc
> and also it doesn't make sense to call zynq_slcr_get_idcode() twice.
> It should be enough to detect chip once and fill pointer to actual
> configuration. And then when fpga should be add then use them. The same
> for cpuinfo. Link is already setup and you can just use it.
>

So you propose to have just one table instead of two
zynq_cpu_info and zynq_fpga_descs ?

I guess that'll work and might result in simpler code.
On the other side, the reason I kept them separate
is because each of them are compiled-in via different
compile-time options.

Having a single table will mean playing nasty ifdef
games, which usually mean trouble.

Regarding calling zynq_slcr_get_idcode twice,
is it really that bad?
Michal Simek Feb. 1, 2018, 8:26 a.m. UTC | #3 
On 31.1.2018 22:25, Ezequiel Garcia wrote:
> On 26 January 2018 at 09:33, Michal Simek <michal.simek@xilinx.com> wrote:
>> Hi,
>>
>>
>> On 17.1.2018 14:56, Ezequiel Garcia wrote:
>>> This commit adds CPU and silicon version information
>>> consuming the SLCR IDCODE and DEVCFG MCTRL registers,
>>> respectively.
>>>
>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>> ---
>>>  arch/arm/mach-zynq/cpu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 46 insertions(+)
>>>
>>> diff --git a/arch/arm/mach-zynq/cpu.c b/arch/arm/mach-zynq/cpu.c
>>> index 53a07b0059c2..602f483c162b 100644
>>> --- a/arch/arm/mach-zynq/cpu.c
>>> +++ b/arch/arm/mach-zynq/cpu.c
>>> @@ -35,6 +35,25 @@ static const struct {
>>>  };
>>>  #endif
>>>
>>> +#ifdef CONFIG_DISPLAY_CPUINFO
>>> +static const struct {
>>> +     u8 idcode;
>>> +     const char *cpuinfo;
>>> +} zynq_cpu_info[] = {
>>> +     { .idcode = XILINX_ZYNQ_7007S,  .cpuinfo = XILINX_XC7Z007S_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7010,   .cpuinfo = XILINX_XC7Z010_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7012S,  .cpuinfo = XILINX_XC7Z012S_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7014S,  .cpuinfo = XILINX_XC7Z014S_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7015,   .cpuinfo = XILINX_XC7Z015_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7020,   .cpuinfo = XILINX_XC7Z020_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7030,   .cpuinfo = XILINX_XC7Z030_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7035,   .cpuinfo = XILINX_XC7Z035_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7045,   .cpuinfo = XILINX_XC7Z045_NAME },
>>> +     { .idcode = XILINX_ZYNQ_7100,   .cpuinfo = XILINX_XC7Z100_NAME },
>>> +     { /* Sentinel */ },
>>
>> This table pretty much reflect what it is in 2/5.
>>
>> static const struct {
>>         u8 idcode;
>>         const char *cpuinfo; /* or better name devicename */
>>         u32 fpga_size;
>> } zynq_cpu_info[] = {
>>
>> From xilinx_desc I think size is unused but we can keep in filled and
>> cookie is also not used and can be 0. The rest of data for xilinx_desc
>> is static anyway.
>>
>> It means doing this properly will be the best to fill that xilinx_desc
>> and also it doesn't make sense to call zynq_slcr_get_idcode() twice.
>> It should be enough to detect chip once and fill pointer to actual
>> configuration. And then when fpga should be add then use them. The same
>> for cpuinfo. Link is already setup and you can just use it.
>>
> 
> So you propose to have just one table instead of two
> zynq_cpu_info and zynq_fpga_descs ?

I was playing with it last Friday and I have sent that 2 patches as RFC.

> I guess that'll work and might result in simpler code.
> On the other side, the reason I kept them separate
> is because each of them are compiled-in via different
> compile-time options.
> 
> Having a single table will mean playing nasty ifdef
> games, which usually mean trouble.
> 
> Regarding calling zynq_slcr_get_idcode twice,
> is it really that bad?

It is not that bad. It is probably better than having global variable.

Take a look at that RFC. Buildman is also showing pretty nice size
reduction but there are still some pieces which can be done better.
It is based on your code that's why fell free to rework it.

Thanks,
Michal
Ezequiel Garcia Feb. 1, 2018, 1:26 p.m. UTC | #4 
On 1 February 2018 at 05:26, Michal Simek <michal.simek@xilinx.com> wrote:
> On 31.1.2018 22:25, Ezequiel Garcia wrote:
>> On 26 January 2018 at 09:33, Michal Simek <michal.simek@xilinx.com> wrote:
>>> Hi,
>>>
>>>
>>> On 17.1.2018 14:56, Ezequiel Garcia wrote:
>>>> This commit adds CPU and silicon version information
>>>> consuming the SLCR IDCODE and DEVCFG MCTRL registers,
>>>> respectively.
>>>>
>>>> Signed-off-by: Ariel D'Alessandro <ariel@vanguardiasur.com.ar>
>>>> Signed-off-by: Ezequiel Garcia <ezequiel@vanguardiasur.com.ar>
>>>> ---
>>>>  arch/arm/mach-zynq/cpu.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++
>>>>  1 file changed, 46 insertions(+)
>>>>
>>>> diff --git a/arch/arm/mach-zynq/cpu.c b/arch/arm/mach-zynq/cpu.c
>>>> index 53a07b0059c2..602f483c162b 100644
>>>> --- a/arch/arm/mach-zynq/cpu.c
>>>> +++ b/arch/arm/mach-zynq/cpu.c
>>>> @@ -35,6 +35,25 @@ static const struct {
>>>>  };
>>>>  #endif
>>>>
>>>> +#ifdef CONFIG_DISPLAY_CPUINFO
>>>> +static const struct {
>>>> +     u8 idcode;
>>>> +     const char *cpuinfo;
>>>> +} zynq_cpu_info[] = {
>>>> +     { .idcode = XILINX_ZYNQ_7007S,  .cpuinfo = XILINX_XC7Z007S_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7010,   .cpuinfo = XILINX_XC7Z010_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7012S,  .cpuinfo = XILINX_XC7Z012S_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7014S,  .cpuinfo = XILINX_XC7Z014S_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7015,   .cpuinfo = XILINX_XC7Z015_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7020,   .cpuinfo = XILINX_XC7Z020_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7030,   .cpuinfo = XILINX_XC7Z030_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7035,   .cpuinfo = XILINX_XC7Z035_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7045,   .cpuinfo = XILINX_XC7Z045_NAME },
>>>> +     { .idcode = XILINX_ZYNQ_7100,   .cpuinfo = XILINX_XC7Z100_NAME },
>>>> +     { /* Sentinel */ },
>>>
>>> This table pretty much reflect what it is in 2/5.
>>>
>>> static const struct {
>>>         u8 idcode;
>>>         const char *cpuinfo; /* or better name devicename */
>>>         u32 fpga_size;
>>> } zynq_cpu_info[] = {
>>>
>>> From xilinx_desc I think size is unused but we can keep in filled and
>>> cookie is also not used and can be 0. The rest of data for xilinx_desc
>>> is static anyway.
>>>
>>> It means doing this properly will be the best to fill that xilinx_desc
>>> and also it doesn't make sense to call zynq_slcr_get_idcode() twice.
>>> It should be enough to detect chip once and fill pointer to actual
>>> configuration. And then when fpga should be add then use them. The same
>>> for cpuinfo. Link is already setup and you can just use it.
>>>
>>
>> So you propose to have just one table instead of two
>> zynq_cpu_info and zynq_fpga_descs ?
>
> I was playing with it last Friday and I have sent that 2 patches as RFC.
>

I see.

>> I guess that'll work and might result in simpler code.
>> On the other side, the reason I kept them separate
>> is because each of them are compiled-in via different
>> compile-time options.
>>
>> Having a single table will mean playing nasty ifdef
>> games, which usually mean trouble.
>>
>> Regarding calling zynq_slcr_get_idcode twice,
>> is it really that bad?
>
> It is not that bad. It is probably better than having global variable.
>
> Take a look at that RFC. Buildman is also showing pretty nice size
> reduction but there are still some pieces which can be done better.
> It is based on your code that's why fell free to rework it.
>

Indeed, it doesn't look bad at all. Let me try those patches,
see how they work.
diff mbox series

Patch

diff --git a/arch/arm/mach-zynq/cpu.c b/arch/arm/mach-zynq/cpu.c
index 53a07b0059c2..602f483c162b 100644
--- a/arch/arm/mach-zynq/cpu.c
+++ b/arch/arm/mach-zynq/cpu.c
@@ -35,6 +35,25 @@  static const struct {
 };
 #endif
 
+#ifdef CONFIG_DISPLAY_CPUINFO
+static const struct {
+	u8 idcode;
+	const char *cpuinfo;
+} zynq_cpu_info[] = {
+	{ .idcode = XILINX_ZYNQ_7007S,	.cpuinfo = XILINX_XC7Z007S_NAME },
+	{ .idcode = XILINX_ZYNQ_7010,	.cpuinfo = XILINX_XC7Z010_NAME },
+	{ .idcode = XILINX_ZYNQ_7012S,	.cpuinfo = XILINX_XC7Z012S_NAME },
+	{ .idcode = XILINX_ZYNQ_7014S,	.cpuinfo = XILINX_XC7Z014S_NAME },
+	{ .idcode = XILINX_ZYNQ_7015,	.cpuinfo = XILINX_XC7Z015_NAME },
+	{ .idcode = XILINX_ZYNQ_7020,	.cpuinfo = XILINX_XC7Z020_NAME },
+	{ .idcode = XILINX_ZYNQ_7030,	.cpuinfo = XILINX_XC7Z030_NAME },
+	{ .idcode = XILINX_ZYNQ_7035,	.cpuinfo = XILINX_XC7Z035_NAME },
+	{ .idcode = XILINX_ZYNQ_7045,	.cpuinfo = XILINX_XC7Z045_NAME },
+	{ .idcode = XILINX_ZYNQ_7100,	.cpuinfo = XILINX_XC7Z100_NAME },
+	{ /* Sentinel */ },
+};
+#endif
+
 int arch_cpu_init(void)
 {
 	zynq_slcr_unlock();
@@ -99,3 +118,30 @@  const xilinx_desc *zynq_fpga_desc(void)
 	return NULL;
 }
 #endif
+
+#ifdef CONFIG_DISPLAY_CPUINFO
+int print_cpuinfo(void)
+{
+	u32 idcode, version;
+	bool found;
+	u8 i;
+
+	idcode = zynq_slcr_get_idcode();
+	found = false;
+	for (i = 0; zynq_cpu_info[i].idcode; i++) {
+		if (zynq_cpu_info[i].idcode == idcode) {
+			found = true;
+			break;
+		}
+	}
+
+	version = zynq_get_silicon_version() << 1;
+	if (version > (PCW_SILICON_VERSION_3 << 1))
+		version += 1;
+	if (found) {
+		printf("CPU:   Zynq %s\n", zynq_cpu_info[i].cpuinfo);
+		printf("Silicon: v%d.%d\n", version >> 1, version & 1);
+	}
+	return 0;
+}
+#endif