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

[Biju Das]

Hi Marek,

> Subject: Re: [PATCH v5 1/2] arm: rmobile: Add RZ/G2[HMNE] SoC support
> 
> 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.

of_match function for matching device compatible string. But it doesn't match SoC compatible string.
So I added a helper function  is of_match_node[1] to search from rootnode to find the soc compatible string.

[1] http://patchwork.ozlabs.org/project/uboot/patch/20201030140303.11773-1-biju.das.jz@bp.renesas.com/

Apart from this, This helper function can be used to replace the below codes in u-boot tree [2] [3]

[2] https://elixir.bootlin.com/u-boot/latest/source/drivers/serial/serial_uniphier.c#L129

[3] https://elixir.bootlin.com/u-boot/latest/source/drivers/usb/phy/rockchip_usb2_phy.c#L77


> > +	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.

I agree calling OF match is expensive. So I have ported Renesas SoC identification driver  from Linux to u-boot 
which will cache the family type, soc_id and revision. I already sent a patch for supporting soc_id in UCLASS_SOC in ML[4]
[4]  http://patchwork.ozlabs.org/project/uboot/patch/20201030140724.12773-1-biju.das.jz@bp.renesas.com/

On the next version, I will send Renesas SoC identification driver, which supports caching family type which
can be used to provide unique identification for CPU type.

> > +
> > +	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... ?

OK, will take out parenthesis. RMOBILE_CPU_TYPE is unique, where SOC_ID_XXXX is not unique.

> > +/* 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.

Ok. Agreed.

Thanks and regards,
Biju