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