[PATCH v5 1/2] arm: rmobile: Add RZ/G2[HMNE] SoC support

[Marek Vasut]

On 10/8/20 10:59 AM, Biju Das wrote:

[...]

> +static const struct udevice_id tfa_cpu_info[] = {
> +	{ .compatible = "renesas,r8a774a1", .data = RMOBILE_CPU_TYPE_R8A774A1 },
> +	{ .compatible = "renesas,r8a774b1", .data = RMOBILE_CPU_TYPE_R8A774B1 },
> +	{ .compatible = "renesas,r8a774c0", .data = RMOBILE_CPU_TYPE_R8A774C0 },
> +	{ .compatible = "renesas,r8a774e1", .data = RMOBILE_CPU_TYPE_R8A774E1 },
> +	{ },
> +};
> +
> +static const u32 get_cpu_type(u32 soc_id)
> +{
> +	const struct udevice_id *of_match = tfa_cpu_info;
> +	int i;

Isn't there already a function in U-Boot which can match on a table of
OF compatible strings ? I suspect when drivers match on their table of
OF compatible strings, somesuch function must be used.

> +	for (i = 0; i < ARRAY_SIZE(tfa_cpu_info); i++) {
> +		if (soc_id == (of_match->data & PRODUCT_MASK) &&
> +		    of_machine_is_compatible(of_match->compatible))
> +			return of_match->data;
> +		of_match++;
> +	}
> +
> +	return soc_id;
> +}
> +
>  static u32 rmobile_get_prr(void)
>  {
>  #ifdef CONFIG_RCAR_GEN3
> @@ -23,7 +48,9 @@ static u32 rmobile_get_prr(void)
>  
>  u32 rmobile_get_cpu_type(void)
>  {
> -	return (rmobile_get_prr() & 0x00007F00) >> 8;
> +	const u32 soc_id = (rmobile_get_prr() & 0x00007F00) >> 8;

The soc_id = ... can be inlined into get_cpu_type().

However, you might want to cache the result of rmobile_get_cpu_type() ,
because doing OF match every time this is called is expensive.

> +
> +	return get_cpu_type(soc_id);
>  }

[...]

> +/* CPU IDs */
> +#define RMOBILE_CPU_TYPE_SH73A0		(SOC_ID_SH73A0)
> +#define RMOBILE_CPU_TYPE_R8A7740	(SOC_ID_R8A7740)
> +#define RMOBILE_CPU_TYPE_R8A774A1	(SOC_ID_R8A774A1 | RZG_CPU_MASK)
> +#define RMOBILE_CPU_TYPE_R8A774B1	(SOC_ID_R8A774B1 | RZG_CPU_MASK)
> +#define RMOBILE_CPU_TYPE_R8A774C0	(SOC_ID_R8A774C0 | RZG_CPU_MASK)
> +#define RMOBILE_CPU_TYPE_R8A774E1	(SOC_ID_R8A774E1 | RZG_CPU_MASK)
> +#define RMOBILE_CPU_TYPE_R8A7790	(SOC_ID_R8A7790)
> +#define RMOBILE_CPU_TYPE_R8A7791	(SOC_ID_R8A7791)
> +#define RMOBILE_CPU_TYPE_R8A7792	(SOC_ID_R8A7792)
> +#define RMOBILE_CPU_TYPE_R8A7793	(SOC_ID_R8A7793)
> +#define RMOBILE_CPU_TYPE_R8A7794	(SOC_ID_R8A7794)
> +#define RMOBILE_CPU_TYPE_R8A7795	(SOC_ID_R8A7795)
> +#define RMOBILE_CPU_TYPE_R8A7796	(SOC_ID_R8A7796)
> +#define RMOBILE_CPU_TYPE_R8A77965	(SOC_ID_R8A77965)
> +#define RMOBILE_CPU_TYPE_R8A77970	(SOC_ID_R8A77970)
> +#define RMOBILE_CPU_TYPE_R8A77980	(SOC_ID_R8A77980)
> +#define RMOBILE_CPU_TYPE_R8A77990	(SOC_ID_R8A77990)
> +#define RMOBILE_CPU_TYPE_R8A77995	(SOC_ID_R8A77995)

The () parentheses are not needed. Also, is there any need for this
renaming of SOC_ID_R8... to RMOBILE_CPU... ?

> +/* RZ/G CPU Identification Mask */
> +#define RZG_CPU_MASK 0x1000

This mask should have a comment which explicitly states that it is not
related to the PRR register content.