[1/2] arm: rmobile: Add RZ/G2M SoC

Add CPU and PRR IDs for R8A774A1(a.k.a RZ/G2M) SoC.

RZ/Gx SoC's are identical to R-Car SoC's apart from some automotive
peripherals and they also share the same PRR CPU ID's.

For example the RZ/G2M SoC has the same PRR ID 0x52 as R-Car M3W SoC.

To differentiate RZ/G SoC's from R-Car SoC's add a member family_type
in struct rmobile_cpuinfo and compare the compatible string from
device tree for SoC identification of RZ/G SoC.

Also sorted the header alphabetically.

Signed-off-by: Biju Das <biju.das.jz@bp.renesas.com>
Reviewed-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@bp.renesas.com>
---
V1-->V2
   Add comments in the code for SoC identification logic for RZ/G SoC's
---
 arch/arm/mach-rmobile/cpu_info.c             | 83 +++++++++++++++-----
 arch/arm/mach-rmobile/include/mach/rmobile.h |  1 +
 2 files changed, 65 insertions(+), 19 deletions(-)

On 9/18/20 6:03 PM, Biju Das wrote:
> Add CPU and PRR IDs for R8A774A1(a.k.a RZ/G2M) SoC.

[...]

> +static const struct udevice_id *of_soc_match_compatible(void)
> +{
> +	const struct udevice_id *of_match = soc_ids;
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(soc_ids); i++) {
> +		if (!fdt_node_check_compatible(gd->fdt_blob, 0,
> +					       of_match->compatible))
> +			return of_match;
> +		of_match++;
> +	}
> +
> +	return NULL;
> +}

This should rather be a generic function, I think this is something that
already exists in Linux common code too, right ?

>  static int rmobile_cpuinfo_idx(void)
>  {
>  	int i = 0;
>  	u32 cpu_type = rmobile_get_cpu_type();
> +	const struct udevice_id *match = of_soc_match_compatible();
>  
> +	/*
> +	 * This loop identifies CPU based on PRR register, it differentiates
> +	 * RZ/G SoC's from R-Car SoC's by matching RZ/G SoC compatible string
> +	 * from DT against the family_type.
> +	 */
>  	for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++)
> -		if (rmobile_cpuinfo[i].cpu_type == cpu_type)
> -			break;
> +		if (rmobile_cpuinfo[i].cpu_type == cpu_type) {
> +			if (match &&
> +			    rmobile_cpuinfo[i].family_type == match->data)
> +				break;
> +			else if (!match &&
> +				 rmobile_cpuinfo[i].family_type != SOC_RZG2)
> +				break;
> +		}

I still don't understand this, so if cpu_type ==
RMOBILE_CPU_TYPE_R8A7796 , then it can be either RZG2 or R8A7796, right?
And there is no PRR bit or any other bit to tell those two chips apart ?

I would like to avoid using the OF match here, because that fails if you
use MULTI_DTB_FIT , does it not ? So can you please check whether there
might be some way to tell the two SoCs apart ?

Hi Marek,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
>
> On 9/18/20 6:03 PM, Biju Das wrote:
> > Add CPU and PRR IDs for R8A774A1(a.k.a RZ/G2M) SoC.
>
> [...]
>
> > +static const struct udevice_id *of_soc_match_compatible(void) {
> > +const struct udevice_id *of_match = soc_ids;
> > +int i;
> > +
> > +for (i = 0; i < ARRAY_SIZE(soc_ids); i++) {
> > +if (!fdt_node_check_compatible(gd->fdt_blob, 0,
> > +       of_match->compatible))
> > +return of_match;
> > +of_match++;
> > +}
> > +
> > +return NULL;
> > +}
>
> This should rather be a generic function, I think this is something that already
> exists in Linux common code too, right ?

No.  I have seen some other SoC's uses similar logic [1]& [2] .

[1] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/board/samsung/common/exynos5-dt-types.c#L246
[2] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/mach-uniphier/boards.c#L156


> >  static int rmobile_cpuinfo_idx(void)
> >  {
> >  int i = 0;
> >  u32 cpu_type = rmobile_get_cpu_type();
> > +const struct udevice_id *match = of_soc_match_compatible();
> >
> > +/*
> > + * This loop identifies CPU based on PRR register, it differentiates
> > + * RZ/G SoC's from R-Car SoC's by matching RZ/G SoC compatible
> string
> > + * from DT against the family_type.
> > + */
> >  for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++)
> > -if (rmobile_cpuinfo[i].cpu_type == cpu_type)
> > -break;
> > +if (rmobile_cpuinfo[i].cpu_type == cpu_type) {
> > +if (match &&
> > +    rmobile_cpuinfo[i].family_type == match->data)
> > +break;
> > +else if (!match &&
> > + rmobile_cpuinfo[i].family_type !=
> SOC_RZG2)
> > +break;
> > +}
>
> I still don't understand this, so if cpu_type ==
> RMOBILE_CPU_TYPE_R8A7796 , then it can be either RZG2 or R8A7796, right?

Yep you are right.

> And there is no PRR bit or any other bit to tell those two chips apart ?
No. Currently only way you can distinguish is by SoC compatible string and family type.
See [3] for SoC identification logic used to differentiate  RCar and RZ/G2
[3]:- https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/renesas/renesas-soc.c?h=v5.9-rc5#n311

> I would like to avoid using the OF match here, because that fails if you use
> MULTI_DTB_FIT , does it not ?

No. It works OK on both RZ/G2SoC's[4] and RCar[5]

[4]  MULTI_DTB_FIT logs for RZG2[HMN] boards

CPU: Renesas Electronics R8A774E1 rev 3.0
Model: HopeRun HiHope RZ/G2H with sub board
DRAM:  3.9 GiB

CPU: Renesas Electronics R8A774A1 rev 1.3
Model: HopeRun HiHope RZ/G2M with sub board
DRAM:  3.9 GiB

CPU: Renesas Electronics R8A774B1 rev 1.1
Model: HopeRun HiHope RZ/G2N with sub board
DRAM:  3.9 GiB

[5] u-boot based on R-Car M3N using rcar3_salvator-x_defconfig,  it reports proper SoC.

CPU: Renesas Electronics R8A77965 rev 1.1
Model: Renesas Salvator-XS board based on r8a77965
DRAM: 1.9 GiB
Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB

MMC: sd@ee100000: 0, sd@ee140000: 1, sd@ee160000: 2
Loading Environment from MMC... OK
In: serial@e6e88000
Out: serial@e6e88000
Err: serial@e6e88000
Net: eth0: ethernet@e6800000
Hit any key to stop autoboot: 0
=>

 So can you please check whether there might
> be some way to tell the two SoCs apart ?

At present there is no way other than matching the SoC compatible string.

Cheers,
Biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

On 9/19/20 1:37 PM, Biju Das wrote:
[...]
>>> +static const struct udevice_id *of_soc_match_compatible(void) {
>>> +const struct udevice_id *of_match = soc_ids;
>>> +int i;
>>> +
>>> +for (i = 0; i < ARRAY_SIZE(soc_ids); i++) {
>>> +if (!fdt_node_check_compatible(gd->fdt_blob, 0,
>>> +       of_match->compatible))
>>> +return of_match;
>>> +of_match++;
>>> +}
>>> +
>>> +return NULL;
>>> +}
>>
>> This should rather be a generic function, I think this is something that already
>> exists in Linux common code too, right ?
> 
> No.  I have seen some other SoC's uses similar logic [1]& [2] .

I mean, this looks like Linux's soc_device_match() , so such a function
is likely generic code, there is nothing platform specific to it, is there ?

> [1] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/board/samsung/common/exynos5-dt-types.c#L246
> [2] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/mach-uniphier/boards.c#L156
> 
> 
>>>  static int rmobile_cpuinfo_idx(void)
>>>  {
>>>  int i = 0;
>>>  u32 cpu_type = rmobile_get_cpu_type();
>>> +const struct udevice_id *match = of_soc_match_compatible();
>>>
>>> +/*
>>> + * This loop identifies CPU based on PRR register, it differentiates
>>> + * RZ/G SoC's from R-Car SoC's by matching RZ/G SoC compatible
>> string
>>> + * from DT against the family_type.
>>> + */
>>>  for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++)
>>> -if (rmobile_cpuinfo[i].cpu_type == cpu_type)
>>> -break;
>>> +if (rmobile_cpuinfo[i].cpu_type == cpu_type) {
>>> +if (match &&
>>> +    rmobile_cpuinfo[i].family_type == match->data)
>>> +break;
>>> +else if (!match &&
>>> + rmobile_cpuinfo[i].family_type !=
>> SOC_RZG2)
>>> +break;
>>> +}
>>
>> I still don't understand this, so if cpu_type ==
>> RMOBILE_CPU_TYPE_R8A7796 , then it can be either RZG2 or R8A7796, right?
> 
> Yep you are right.
> 
>> And there is no PRR bit or any other bit to tell those two chips apart ?
> No. Currently only way you can distinguish is by SoC compatible string and family type.
> See [3] for SoC identification logic used to differentiate  RCar and RZ/G2
> [3]:- https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/soc/renesas/renesas-soc.c?h=v5.9-rc5#n311

So Linux is matching on the compatible string of DT passed from U-Boot ,
right ? Linux has it easier then.

But where does U-Boot get that compatible string from, in case there are
multiple DTs passed to U-Boot and U-Boot needs to find out on which SoC
it is running on ?

Maybe you can pass the compatible from TFA, which is already happening.

>> I would like to avoid using the OF match here, because that fails if you use
>> MULTI_DTB_FIT , does it not ?
> 
> No. It works OK on both RZ/G2SoC's[4] and RCar[5]
> 
> [4]  MULTI_DTB_FIT logs for RZG2[HMN] boards
> 
> CPU: Renesas Electronics R8A774E1 rev 3.0
> Model: HopeRun HiHope RZ/G2H with sub board
> DRAM:  3.9 GiB
> 
> CPU: Renesas Electronics R8A774A1 rev 1.3
> Model: HopeRun HiHope RZ/G2M with sub board
> DRAM:  3.9 GiB
> 
> CPU: Renesas Electronics R8A774B1 rev 1.1
> Model: HopeRun HiHope RZ/G2N with sub board
> DRAM:  3.9 GiB
> 
> [5] u-boot based on R-Car M3N using rcar3_salvator-x_defconfig,  it reports proper SoC.
> 
> CPU: Renesas Electronics R8A77965 rev 1.1
> Model: Renesas Salvator-XS board based on r8a77965
> DRAM: 1.9 GiB
> Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB
> 
> MMC: sd@ee100000: 0, sd@ee140000: 1, sd@ee160000: 2
> Loading Environment from MMC... OK
> In: serial@e6e88000
> Out: serial@e6e88000
> Err: serial@e6e88000
> Net: eth0: ethernet@e6800000
> Hit any key to stop autoboot: 0
> =>
> 
>  So can you please check whether there might
>> be some way to tell the two SoCs apart ?
> 
> At present there is no way other than matching the SoC compatible string.

Thinking about it a bit more, if you were to use the compatible string
psssed from TFA in the / node, you could iterate over soc_ids[] array
and return RMOBILE_CPU_TYPE_x , which could be stored there as .data .
Then you won't even need the SOC_RZG2 and it would all be faster, as all
you would need is a single pass over a smaller array.

Hi Marek,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
>
> On 9/19/20 1:37 PM, Biju Das wrote:
> [...]
> >>> +static const struct udevice_id *of_soc_match_compatible(void) {
> >>> +const struct udevice_id *of_match = soc_ids; int i;
> >>> +
> >>> +for (i = 0; i < ARRAY_SIZE(soc_ids); i++) { if
> >>> +(!fdt_node_check_compatible(gd->fdt_blob, 0,
> >>> +       of_match->compatible))
> >>> +return of_match;
> >>> +of_match++;
> >>> +}
> >>> +
> >>> +return NULL;
> >>> +}
> >>
> >> This should rather be a generic function, I think this is something
> >> that already exists in Linux common code too, right ?
> >
> > No.  I have seen some other SoC's uses similar logic [1]& [2] .
>
> I mean, this looks like Linux's soc_device_match() , so such a function is likely
> generic code, there is nothing platform specific to it, is there ?

I agree, we need to have a new generic api for such purpose. The Linux/U-boot soc_device_match  is for adding quirks with in different ES version of same SoC.

What we here need is similar to of_match_compatible for  array of different SoC's.
Can you please confirm  [1] drivers/soc/soc-uclass.c is the right place for such generic api?

[1] https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/drivers/soc/soc-uclass.c

> > [1]
> > https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/board/samsung/co
> > mmon/exynos5-dt-types.c#L246 [2]
> > https://elixir.bootlin.com/u-boot/v2020.10-rc4/source/arch/arm/mach-un
> > iphier/boards.c#L156
> >
> >
> >>>  static int rmobile_cpuinfo_idx(void)  {  int i = 0;
> >>>  u32 cpu_type = rmobile_get_cpu_type();
> >>> +const struct udevice_id *match = of_soc_match_compatible();
> >>>
> >>> +/*
> >>> + * This loop identifies CPU based on PRR register, it
> >>> +differentiates
> >>> + * RZ/G SoC's from R-Car SoC's by matching RZ/G SoC compatible
> >> string
> >>> + * from DT against the family_type.
> >>> + */
> >>>  for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++) -if
> >>> (rmobile_cpuinfo[i].cpu_type == cpu_type) -break;
> >>> +if (rmobile_cpuinfo[i].cpu_type == cpu_type) { if (match &&
> >>> +    rmobile_cpuinfo[i].family_type == match->data) break; else if
> >>> +(!match &&  rmobile_cpuinfo[i].family_type !=
> >> SOC_RZG2)
> >>> +break;
> >>> +}
> >>
> >> I still don't understand this, so if cpu_type ==
> >> RMOBILE_CPU_TYPE_R8A7796 , then it can be either RZG2 or R8A7796,
> right?
> >
> > Yep you are right.
> >
> >> And there is no PRR bit or any other bit to tell those two chips apart ?
> > No. Currently only way you can distinguish is by SoC compatible string and
> family type.
> > See [3] for SoC identification logic used to differentiate  RCar and
> > RZ/G2
> > [3]:-
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tre
> > e/drivers/soc/renesas/renesas-soc.c?h=v5.9-rc5#n311
>
> So Linux is matching on the compatible string of DT passed from U-Boot ,
> right ? Linux has it easier then.
>
> But where does U-Boot get that compatible string from, in case there are
> multiple DTs passed to U-Boot and U-Boot needs to find out on which SoC it
> is running on ?
>
> Maybe you can pass the compatible from TFA, which is already happening.
>
> >> I would like to avoid using the OF match here, because that fails if
> >> you use MULTI_DTB_FIT , does it not ?
> >
> > No. It works OK on both RZ/G2SoC's[4] and RCar[5]
> >
> > [4]  MULTI_DTB_FIT logs for RZG2[HMN] boards
> >
> > CPU: Renesas Electronics R8A774E1 rev 3.0
> > Model: HopeRun HiHope RZ/G2H with sub board
> > DRAM:  3.9 GiB
> >
> > CPU: Renesas Electronics R8A774A1 rev 1.3
> > Model: HopeRun HiHope RZ/G2M with sub board
> > DRAM:  3.9 GiB
> >
> > CPU: Renesas Electronics R8A774B1 rev 1.1
> > Model: HopeRun HiHope RZ/G2N with sub board
> > DRAM:  3.9 GiB
> >
> > [5] u-boot based on R-Car M3N using rcar3_salvator-x_defconfig,  it reports
> proper SoC.
> >
> > CPU: Renesas Electronics R8A77965 rev 1.1
> > Model: Renesas Salvator-XS board based on r8a77965
> > DRAM: 1.9 GiB
> > Bank #0: 0x048000000 - 0x0bfffffff, 1.9 GiB
> >
> > MMC: sd@ee100000: 0, sd@ee140000: 1, sd@ee160000: 2 Loading
> > Environment from MMC... OK
> > In: serial@e6e88000
> > Out: serial@e6e88000
> > Err: serial@e6e88000
> > Net: eth0: ethernet@e6800000
> > Hit any key to stop autoboot: 0
> > =>
> >
> >  So can you please check whether there might
> >> be some way to tell the two SoCs apart ?
> >
> > At present there is no way other than matching the SoC compatible string.
>
> Thinking about it a bit more, if you were to use the compatible string psssed
> from TFA in the / node, you could iterate over soc_ids[] array and return
> RMOBILE_CPU_TYPE_x , which could be stored there as .data .
> Then you won't even need the SOC_RZG2 and it would all be faster, as all you
> would need is a single pass over a smaller array.

Good point. Ok will get rid of SOC_RZG2,  will use smaller array forRZG2.

Are you suggesting to modify "arch_misc_init" directly set "platform" environment variable using match logic, which use a smaller array
Compared to rmobile_cpuinfo.

Basically we match the compatible string from TFA, .data from " RMOBILE_CPU_TYPE_x" matched against PRR values and set the platform type .

Cheers,
Biju



Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

On 9/19/20 8:35 PM, Biju Das wrote:

Hi,

[...]

>>>>> +static const struct udevice_id *of_soc_match_compatible(void) {
>>>>> +const struct udevice_id *of_match = soc_ids; int i;
>>>>> +
>>>>> +for (i = 0; i < ARRAY_SIZE(soc_ids); i++) { if
>>>>> +(!fdt_node_check_compatible(gd->fdt_blob, 0,
>>>>> +       of_match->compatible))
>>>>> +return of_match;
>>>>> +of_match++;
>>>>> +}
>>>>> +
>>>>> +return NULL;
>>>>> +}
>>>>
>>>> This should rather be a generic function, I think this is something
>>>> that already exists in Linux common code too, right ?
>>>
>>> No.  I have seen some other SoC's uses similar logic [1]& [2] .
>>
>> I mean, this looks like Linux's soc_device_match() , so such a function is likely
>> generic code, there is nothing platform specific to it, is there ?
> 
> I agree, we need to have a new generic api for such purpose. The Linux/U-boot soc_device_match  is for adding quirks with in different ES version of same SoC.
> 
> What we here need is similar to of_match_compatible for  array of different SoC's.
> Can you please confirm  [1] drivers/soc/soc-uclass.c is the right place for such generic api?

Can you use of_machine_is_compatible() ?

[...]

>>>  So can you please check whether there might
>>>> be some way to tell the two SoCs apart ?
>>>
>>> At present there is no way other than matching the SoC compatible string.
>>
>> Thinking about it a bit more, if you were to use the compatible string psssed
>> from TFA in the / node, you could iterate over soc_ids[] array and return
>> RMOBILE_CPU_TYPE_x , which could be stored there as .data .
>> Then you won't even need the SOC_RZG2 and it would all be faster, as all you
>> would need is a single pass over a smaller array.
> 
> Good point. Ok will get rid of SOC_RZG2,  will use smaller array forRZG2.
> 
> Are you suggesting to modify "arch_misc_init" directly set "platform" environment variable using match logic, which use a smaller array
> Compared to rmobile_cpuinfo.
> 
> Basically we match the compatible string from TFA, .data from " RMOBILE_CPU_TYPE_x" matched against PRR values and set the platform type .

I don't think you need to modify anything then, the DT passed from TFA
would contain the correct compatible string in / node, and that gets
merged into the U-Boot control DT early on in fdtdec_board_setup() in:
board/renesas/rcar-common/common.c
so all you would have to do is use
of_machine_is_compatible("renesas,r8a7-something-");

Would that work ?

Hi Marek,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
>
> On 9/19/20 8:35 PM, Biju Das wrote:
>
> Hi,
>
> [...]
>
> >>>>> +static const struct udevice_id *of_soc_match_compatible(void) {
> >>>>> +const struct udevice_id *of_match = soc_ids; int i;
> >>>>> +
> >>>>> +for (i = 0; i < ARRAY_SIZE(soc_ids); i++) { if
> >>>>> +(!fdt_node_check_compatible(gd->fdt_blob, 0,
> >>>>> +       of_match->compatible))
> >>>>> +return of_match;
> >>>>> +of_match++;
> >>>>> +}
> >>>>> +
> >>>>> +return NULL;
> >>>>> +}
> >>>>
> >>>> This should rather be a generic function, I think this is something
> >>>> that already exists in Linux common code too, right ?
> >>>
> >>> No.  I have seen some other SoC's uses similar logic [1]& [2] .
> >>
> >> I mean, this looks like Linux's soc_device_match() , so such a
> >> function is likely generic code, there is nothing platform specific to it, is
> there ?
> >
> > I agree, we need to have a new generic api for such purpose. The Linux/U-
> boot soc_device_match  is for adding quirks with in different ES version of
> same SoC.
> >
> > What we here need is similar to of_match_compatible for  array of
> different SoC's.
> > Can you please confirm  [1] drivers/soc/soc-uclass.c is the right place for
> such generic api?
>
> Can you use of_machine_is_compatible() ?
Yes, will use that one.
> [...]
>
> >>>  So can you please check whether there might
> >>>> be some way to tell the two SoCs apart ?
> >>>
> >>> At present there is no way other than matching the SoC compatible
> string.
> >>
> >> Thinking about it a bit more, if you were to use the compatible
> >> string psssed from TFA in the / node, you could iterate over
> >> soc_ids[] array and return RMOBILE_CPU_TYPE_x , which could be stored
> there as .data .
> >> Then you won't even need the SOC_RZG2 and it would all be faster, as
> >> all you would need is a single pass over a smaller array.
> >
> > Good point. Ok will get rid of SOC_RZG2,  will use smaller array forRZG2.
> >
> > Are you suggesting to modify "arch_misc_init" directly set "platform"
> > environment variable using match logic, which use a smaller array
> Compared to rmobile_cpuinfo.
> >
> > Basically we match the compatible string from TFA, .data from "
> RMOBILE_CPU_TYPE_x" matched against PRR values and set the platform
> type .
>
> I don't think you need to modify anything then, the DT passed from TFA
> would contain the correct compatible string in / node, and that gets merged
> into the U-Boot control DT early on in fdtdec_board_setup() in:
> board/renesas/rcar-common/common.c
> so all you would have to do is use
> of_machine_is_compatible("renesas,r8a7-something-");
>
> Would that work ?


Yes, I have added the below function to get cpu name from small array tfa_cpu_table. Will send V3 with this changes.

+static const u8* get_cpu_name(void)
+{
+u32 cpu_type = rmobile_get_cpu_type();
+
+return is_tfa_soc_match(cpu_type) ?
+tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
+rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
+}

+static int tfa_cpuinfo_idx(u32 cpu_type)
+{
+int i = 0;
+
+for (; i < ARRAY_SIZE(tfa_cpuinfo); i++)
+if (tfa_cpuinfo[i].cpu_type == cpu_type)
+break;
+
+return i;
+}
+
+static bool is_tfa_soc_match(u32 cpu_type)
+{
+int idx = tfa_cpuinfo_idx(cpu_type);
+
+if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
+    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
+return true;
+
+return false;
+}

Cheers,
biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

On 9/21/20 12:30 PM, Biju Das wrote:

[...]

>> I don't think you need to modify anything then, the DT passed from TFA
>> would contain the correct compatible string in / node, and that gets merged
>> into the U-Boot control DT early on in fdtdec_board_setup() in:
>> board/renesas/rcar-common/common.c
>> so all you would have to do is use
>> of_machine_is_compatible("renesas,r8a7-something-");
>>
>> Would that work ?
> 
> 
> Yes, I have added the below function to get cpu name from small array tfa_cpu_table. Will send V3 with this changes.
> 
> +static const u8* get_cpu_name(void)
> +{
> +u32 cpu_type = rmobile_get_cpu_type();
> +
> +return is_tfa_soc_match(cpu_type) ?
> +tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
> +rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
> +}
> 
> +static int tfa_cpuinfo_idx(u32 cpu_type)
> +{
> +int i = 0;
> +
> +for (; i < ARRAY_SIZE(tfa_cpuinfo); i++)
> +if (tfa_cpuinfo[i].cpu_type == cpu_type)
> +break;
> +
> +return i;
> +}
> +
> +static bool is_tfa_soc_match(u32 cpu_type)
> +{
> +int idx = tfa_cpuinfo_idx(cpu_type);
> +
> +if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
> +    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
> +return true;
> +
> +return false;
> +}

There might be even better way. Look at rmobile_get_cpu_type() , that is
a weak function. So if you can implement one for RZG2 , then that
function can return CPU_TYPE_RZG2_something ; and rmobile_get_cpu_type()
for RZG2 can be implemented using the match on /compatible string .

Take a look at how arch/arm/mach-rmobile/cpu_info-rcar.c implements it
using PRR, you might need cpu_info-rzg.c I think.

Also, I hope there should already be some function to which you provide
a compatible string and a table of supported compatible strings (of
match table), from which it will return the .data field of the matching
entry in that table. And that .data field can already be the
CPU_TYPE_RZG_something , so you don't have to implement the table look
up again.

What do you think ?

Hi Marek,

Thanks for the feedback.

> Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
>
> On 9/21/20 12:30 PM, Biju Das wrote:
>
> [...]
>
> >> I don't think you need to modify anything then, the DT passed from
> >> TFA would contain the correct compatible string in / node, and that
> >> gets merged into the U-Boot control DT early on in fdtdec_board_setup()
> in:
> >> board/renesas/rcar-common/common.c
> >> so all you would have to do is use
> >> of_machine_is_compatible("renesas,r8a7-something-");
> >>
> >> Would that work ?
> >
> >
> > Yes, I have added the below function to get cpu name from small array
> tfa_cpu_table. Will send V3 with this changes.
> >
> > +static const u8* get_cpu_name(void)
> > +{
> > +u32 cpu_type = rmobile_get_cpu_type();
> > +
> > +return is_tfa_soc_match(cpu_type) ?
> > +tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
> > +rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
> > +}
> >
> > +static int tfa_cpuinfo_idx(u32 cpu_type) { int i = 0;
> > +
> > +for (; i < ARRAY_SIZE(tfa_cpuinfo); i++) if (tfa_cpuinfo[i].cpu_type
> > +== cpu_type) break;
> > +
> > +return i;
> > +}
> > +
> > +static bool is_tfa_soc_match(u32 cpu_type) { int idx =
> > +tfa_cpuinfo_idx(cpu_type);
> > +
> > +if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
> > +    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
> > +return true;
> > +
> > +return false;
> > +}
>
> There might be even better way. Look at rmobile_get_cpu_type() , that is a
> weak function. So if you can implement one for RZG2 , then that function can
> return CPU_TYPE_RZG2_something ; and rmobile_get_cpu_type() for RZG2
> can be implemented using the match on /compatible string .
>
> Take a look at how arch/arm/mach-rmobile/cpu_info-rcar.c implements it
> using PRR, you might need cpu_info-rzg.c I think.

As mentioned in the commit message PRR values of both R-Car M3-W and RZ/G2M are identical. So there is no need for separate cpu_info-rzg.c.
I believe it is duplication of code.

We are matching PRR first (device binding)  and then use TFA SoC compatible string to differentiate from R-Car family.
Please see the diff below[3].

> Also, I hope there should already be some function to which you provide a
> compatible string and a table of supported compatible strings (of match
> table), from which it will return the .data field of the matching entry in that
> table. And that .data field can already be the CPU_TYPE_RZG_something , so
> you don't have to implement the table look up again.
>
> What do you think ?

Device binding is important use case, run time you need to match PRR, that is same for both RCar-M3W and RZ/G2E.
In RZ/G2 case, we miss device binding if just go with TFA compatible Approach. So we need both.

What do you think?

[3]
+static const struct {
+char *compatible;
+u16 cpu_type;
+u8 cpu_name[10];
+} tfa_cpuinfo[] = {
+{ "renesas,r8a774a1", RMOBILE_CPU_TYPE_R8A774A1, "R8A774A1" },
+{ },
+};
+
+static int tfa_cpuinfo_idx(u32 cpu_type)
+{
+int i = 0;
+
+for (; i < ARRAY_SIZE(tfa_cpuinfo); i++)
+if (tfa_cpuinfo[i].cpu_type == cpu_type)
+break;
+
+return i;
+}
+
+#if CONFIG_IS_ENABLED(OF_CONTROL)
+static bool is_tfa_soc_match(u32 cpu_type)
+{
+int idx = tfa_cpuinfo_idx(cpu_type);
+
+if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
+    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
+return true;
+
+return false;
+}
+#else
+static bool __is_tfa_soc_match(u32 cpu_type)
+{
+return false;
+}
+bool is_tfa_soc_match(u32 cpu_type)
+__attribute__((weak, alias("__is_tfa_soc_match")));
+#endif
+
 /* CPU infomation table */
 static const struct {
 u16 cpu_type;
@@ -74,10 +115,9 @@ static const struct {
 { 0x0, "CPU" },
 };

-static int rmobile_cpuinfo_idx(void)
+static int rmobile_cpuinfo_idx(u32 cpu_type)
 {
 int i = 0;
-u32 cpu_type = rmobile_get_cpu_type();

 for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++)
 if (rmobile_cpuinfo[i].cpu_type == cpu_type)
@@ -86,14 +126,24 @@ static int rmobile_cpuinfo_idx(void)
 return i;
 }

+static const u8 *get_cpu_name(void)
+{
+u32 cpu_type = rmobile_get_cpu_type();
+
+return is_tfa_soc_match(cpu_type) ?
+tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
+rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
+}
+
 #ifdef CONFIG_ARCH_MISC_INIT
 int arch_misc_init(void)
 {
-int i, idx = rmobile_cpuinfo_idx();
+const u8 *cpu_name = get_cpu_name();
 char cpu[10] = { 0 };
+int i;

 for (i = 0; i < sizeof(cpu); i++)
-cpu[i] = tolower(rmobile_cpuinfo[idx].cpu_name[i]);
+cpu[i] = tolower(cpu_name[i]);

 env_set("platform", cpu);

@@ -103,10 +153,8 @@ int arch_misc_init(void)

 int print_cpuinfo(void)
 {
-int i = rmobile_cpuinfo_idx();
-
 printf("CPU: Renesas Electronics %s rev %d.%d\n",
-rmobile_cpuinfo[i].cpu_name, rmobile_get_cpu_rev_integer(),
+get_cpu_name(), rmobile_get_cpu_rev_integer(),
 rmobile_get_cpu_rev_fraction());

Cheers,
Biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Hi Marek,

> -----Original Message-----
> From: Biju Das
> Sent: 21 September 2020 18:30
> To: Marek Vasut <marek.vasut@gmail.com>; Nobuhiro Iwamatsu
> <iwamatsu@nigauri.org>
> Cc: u-boot@lists.denx.de; Chris Paterson <Chris.Paterson2@renesas.com>;
> Prabhakar Mahadev Lad <prabhakar.mahadev-lad.rj@bp.renesas.com>
> Subject: RE: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
>
>
> Hi Marek,
>
> Thanks for the feedback.
>
> > Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
> >
> > On 9/21/20 12:30 PM, Biju Das wrote:
> >
> > [...]
> >
> > >> I don't think you need to modify anything then, the DT passed from
> > >> TFA would contain the correct compatible string in / node, and that
> > >> gets merged into the U-Boot control DT early on in
> > >> fdtdec_board_setup()
> > in:
> > >> board/renesas/rcar-common/common.c
> > >> so all you would have to do is use
> > >> of_machine_is_compatible("renesas,r8a7-something-");
> > >>
> > >> Would that work ?
> > >
> > >
> > > Yes, I have added the below function to get cpu name from small
> > > array
> > tfa_cpu_table. Will send V3 with this changes.
> > >
> > > +static const u8* get_cpu_name(void) {
> > > +u32 cpu_type = rmobile_get_cpu_type();
> > > +
> > > +return is_tfa_soc_match(cpu_type) ?
> > > +tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
> > > +rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
> > > +}
> > >
> > > +static int tfa_cpuinfo_idx(u32 cpu_type) { int i = 0;
> > > +
> > > +for (; i < ARRAY_SIZE(tfa_cpuinfo); i++) if
> > > +(tfa_cpuinfo[i].cpu_type == cpu_type) break;
> > > +
> > > +return i;
> > > +}
> > > +
> > > +static bool is_tfa_soc_match(u32 cpu_type) { int idx =
> > > +tfa_cpuinfo_idx(cpu_type);
> > > +
> > > +if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
> > > +    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
> > > +return true;
> > > +
> > > +return false;
> > > +}
> >
> > There might be even better way. Look at rmobile_get_cpu_type() , that
> > is a weak function. So if you can implement one for RZG2 , then that
> > function can return CPU_TYPE_RZG2_something ; and
> > rmobile_get_cpu_type() for RZG2 can be implemented using the match on
> /compatible string .
> >
> > Take a look at how arch/arm/mach-rmobile/cpu_info-rcar.c implements it
> > using PRR, you might need cpu_info-rzg.c I think.
>
> As mentioned in the commit message PRR values of both R-Car M3-W and
> RZ/G2M are identical. So there is no need for separate cpu_info-rzg.c.
> I believe it is duplication of code.
>
> We are matching PRR first (device binding)  and then use TFA SoC compatible
> string to differentiate from R-Car family.
> Please see the diff below[3].
>
> > Also, I hope there should already be some function to which you
> > provide a compatible string and a table of supported compatible
> > strings (of match table), from which it will return the .data field of
> > the matching entry in that table. And that .data field can already be
> > the CPU_TYPE_RZG_something , so you don't have to implement the table
> look up again.
> >
> > What do you think ?
>

Thinking further, we can define CPU_TYPE_RZG_MASK as 0x1000. The value CPU_TYPE_RZG_MASK | SOC_PR Is added into tfa_cpu_info table.

We just match  PRR of the SoC  and compatible string from TFA with smaller array table. If match found, we set cputype= CPU_TYPE_RZG_MASK | SOC_PR.
Then the remaining logic in the code works as usual and changes are minimal.

I prefer this match function to be vendor specific, iterating cputype in smaller array(tfa_cpu_info ) is faster than iterating compatible string from device tree.

Please share your views on this.

Cheers,
Biju

> Device binding is important use case, run time you need to match PRR, that is
> same for both RCar-M3W and RZ/G2E.
> In RZ/G2 case, we miss device binding if just go with TFA compatible
> Approach. So we need both.
>
> What do you think?




>
> [3]
> +static const struct {
> +char *compatible;
> +u16 cpu_type;
> +u8 cpu_name[10];
> +} tfa_cpuinfo[] = {
> +{ "renesas,r8a774a1", RMOBILE_CPU_TYPE_R8A774A1, "R8A774A1" },
> +{ },
> +};
> +
> +static int tfa_cpuinfo_idx(u32 cpu_type) {
> +int i = 0;
> +
> +for (; i < ARRAY_SIZE(tfa_cpuinfo); i++)
> +if (tfa_cpuinfo[i].cpu_type == cpu_type)
> +break;
> +
> +return i;
> +}
> +
> +#if CONFIG_IS_ENABLED(OF_CONTROL)
> +static bool is_tfa_soc_match(u32 cpu_type) {
> +int idx = tfa_cpuinfo_idx(cpu_type);
> +
> +if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
> +    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
> +return true;
> +
> +return false;
> +}
> +#else
> +static bool __is_tfa_soc_match(u32 cpu_type) {
> +return false;
> +}
> +bool is_tfa_soc_match(u32 cpu_type)
> +__attribute__((weak, alias("__is_tfa_soc_match"))); #endif
> +
>  /* CPU infomation table */
>  static const struct {
>  u16 cpu_type;
> @@ -74,10 +115,9 @@ static const struct {
>  { 0x0, "CPU" },
>  };
>
> -static int rmobile_cpuinfo_idx(void)
> +static int rmobile_cpuinfo_idx(u32 cpu_type)
>  {
>  int i = 0;
> -u32 cpu_type = rmobile_get_cpu_type();
>
>  for (; i < ARRAY_SIZE(rmobile_cpuinfo); i++)
>  if (rmobile_cpuinfo[i].cpu_type == cpu_type) @@ -86,14
> +126,24 @@ static int rmobile_cpuinfo_idx(void)
>  return i;
>  }
>
> +static const u8 *get_cpu_name(void)
> +{
> +u32 cpu_type = rmobile_get_cpu_type();
> +
> +return is_tfa_soc_match(cpu_type) ?
> +tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
> +
> rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
> +}
> +
>  #ifdef CONFIG_ARCH_MISC_INIT
>  int arch_misc_init(void)
>  {
> -int i, idx = rmobile_cpuinfo_idx();
> +const u8 *cpu_name = get_cpu_name();
>  char cpu[10] = { 0 };
> +int i;
>
>  for (i = 0; i < sizeof(cpu); i++)
> -cpu[i] = tolower(rmobile_cpuinfo[idx].cpu_name[i]);
> +cpu[i] = tolower(cpu_name[i]);
>
>  env_set("platform", cpu);
>
> @@ -103,10 +153,8 @@ int arch_misc_init(void)
>
>  int print_cpuinfo(void)
>  {
> -int i = rmobile_cpuinfo_idx();
> -
>  printf("CPU: Renesas Electronics %s rev %d.%d\n",
> -rmobile_cpuinfo[i].cpu_name,
> rmobile_get_cpu_rev_integer(),
> +get_cpu_name(), rmobile_get_cpu_rev_integer(),
>  rmobile_get_cpu_rev_fraction());
>
> Cheers,
> Biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

Hi Marek,

> > > Subject: Re: [PATCH 1/2] arm: rmobile: Add RZ/G2M SoC
> > >
> > > On 9/21/20 12:30 PM, Biju Das wrote:
> > >
> > > [...]
> > >
> > > >> I don't think you need to modify anything then, the DT passed
> > > >> from TFA would contain the correct compatible string in / node,
> > > >> and that gets merged into the U-Boot control DT early on in
> > > >> fdtdec_board_setup()
> > > in:
> > > >> board/renesas/rcar-common/common.c
> > > >> so all you would have to do is use
> > > >> of_machine_is_compatible("renesas,r8a7-something-");
> > > >>
> > > >> Would that work ?
> > > >
> > > >
> > > > Yes, I have added the below function to get cpu name from small
> > > > array
> > > tfa_cpu_table. Will send V3 with this changes.
> > > >
> > > > +static const u8* get_cpu_name(void) {
> > > > +u32 cpu_type = rmobile_get_cpu_type();
> > > > +
> > > > +return is_tfa_soc_match(cpu_type) ?
> > > > +tfa_cpuinfo[tfa_cpuinfo_idx(cpu_type)].cpu_name :
> > > > +rmobile_cpuinfo[rmobile_cpuinfo_idx(cpu_type)].cpu_name;
> > > > +}
> > > >
> > > > +static int tfa_cpuinfo_idx(u32 cpu_type) { int i = 0;
> > > > +
> > > > +for (; i < ARRAY_SIZE(tfa_cpuinfo); i++) if
> > > > +(tfa_cpuinfo[i].cpu_type == cpu_type) break;
> > > > +
> > > > +return i;
> > > > +}
> > > > +
> > > > +static bool is_tfa_soc_match(u32 cpu_type) { int idx =
> > > > +tfa_cpuinfo_idx(cpu_type);
> > > > +
> > > > +if (idx != ARRAY_SIZE(tfa_cpuinfo) &&
> > > > +    of_machine_is_compatible(tfa_cpuinfo[idx].compatible))
> > > > +return true;
> > > > +
> > > > +return false;
> > > > +}
> > >
> > > There might be even better way. Look at rmobile_get_cpu_type() ,
> > > that is a weak function. So if you can implement one for RZG2 , then
> > > that function can return CPU_TYPE_RZG2_something ; and
> > > rmobile_get_cpu_type() for RZG2 can be implemented using the match
> > > on
> > /compatible string .
> > >
> > > Take a look at how arch/arm/mach-rmobile/cpu_info-rcar.c implements
> > > it using PRR, you might need cpu_info-rzg.c I think.
> >
> > As mentioned in the commit message PRR values of both R-Car M3-W and
> > RZ/G2M are identical. So there is no need for separate cpu_info-rzg.c.
> > I believe it is duplication of code.
> >
> > We are matching PRR first (device binding)  and then use TFA SoC
> > compatible string to differentiate from R-Car family.
> > Please see the diff below[3].
> >
> > > Also, I hope there should already be some function to which you
> > > provide a compatible string and a table of supported compatible
> > > strings (of match table), from which it will return the .data field
> > > of the matching entry in that table. And that .data field can
> > > already be the CPU_TYPE_RZG_something , so you don't have to
> > > implement the table
> > look up again.
> > >
> > > What do you think ?
> >
>
> Thinking further, we can define CPU_TYPE_RZG_MASK as 0x1000. The value
> CPU_TYPE_RZG_MASK | SOC_PR Is added into tfa_cpu_info table.
>
> We just match  PRR of the SoC  and compatible string from TFA with smaller
> array table. If match found, we set cputype= CPU_TYPE_RZG_MASK |
> SOC_PR.
> Then the remaining logic in the code works as usual and changes are minimal.
>
> I prefer this match function to be vendor specific, iterating cputype in smaller
> array(tfa_cpu_info ) is faster than iterating compatible string from device
> tree.
>
> Please share your views on this.

The new diffs looks like this. I will send V3.

As you suggested, I have used match function which returns RZG2_CPU_ID.

+#if CONFIG_IS_ENABLED(OF_CONTROL)
+static const struct udevice_id tfa_soc_ids[] = {
+{ .compatible = "renesas,r8a774a1", .data = RMOBILE_CPU_TYPE_R8A774A1 },
+{ },
+};
+
+static const u16 get_rzg_soc_cpu_type(void)
+{
+const struct udevice_id *of_match = tfa_soc_ids;
+u32 cpu_type = rmobile_get_cpu_type();
+int i;
+
+for (i = 0; i < ARRAY_SIZE(tfa_soc_ids); i++) {
+if (cpu_type == of_match->data &&
+    of_machine_is_compatible(of_match->compatible))
+return (cpu_type | RZG_CPU_MASK);
+of_match++;
+}
+
+return 0;
+}
+#else
+static const const u16 __get_rzg_soc_cpu_type(void)
+{
+return 0;
+}
+
+static const const u16 get_rzg_soc_cpu_type(void)
+__attribute__((weak, alias("__get_rzg_soc_cpu_type")));
+#endif
+
 /* CPU infomation table */
 static const struct {
 u16 cpu_type;
@@ -59,6 +91,7 @@ static const struct {
 } rmobile_cpuinfo[] = {
 { RMOBILE_CPU_TYPE_SH73A0, "SH73A0" },
 { RMOBILE_CPU_TYPE_R8A7740, "R8A7740" },
+{ RMOBILE_CPU_TYPE_R8A774A1 | RZG_CPU_MASK, "R8A774A1" },
 { RMOBILE_CPU_TYPE_R8A7790, "R8A7790" },
 { RMOBILE_CPU_TYPE_R8A7791, "R8A7791" },
 { RMOBILE_CPU_TYPE_R8A7792, "R8A7792" },
@@ -77,7 +110,8 @@ static const struct {
 static int rmobile_cpuinfo_idx(void)
 {
 int i = 0;
-u32 cpu_type = rmobile_get_cpu_type();
+u16 rzg2_cpu_type = get_rzg_soc_cpu_type();
+u32 cpu_type = rzg2_cpu_type ? rzg2_cpu_type : rmobile_get_cpu_type();


>
> > Device binding is important use case, run time you need to match PRR,
> > that is same for both RCar-M3W and RZ/G2E.
> > In RZ/G2 case, we miss device binding if just go with TFA compatible
> > Approach. So we need both.

Please look at the code above, looks like we don't need generic function. Since we need to compare both CPUID and Compatible string
Iterating through CPU ID is much faster than iterating through compatible string.

Cheers,
Biju


Renesas Electronics Europe GmbH, Geschaeftsfuehrer/President: Carsten Jauch, Sitz der Gesellschaft/Registered office: Duesseldorf, Arcadiastrasse 10, 40472 Duesseldorf, Germany, Handelsregister/Commercial Register: Duesseldorf, HRB 3708 USt-IDNr./Tax identification no.: DE 119353406 WEEE-Reg.-Nr./WEEE reg. no.: DE 14978647

On 9/21/20 7:29 PM, Biju Das wrote:

[...]

>> There might be even better way. Look at rmobile_get_cpu_type() , that is a
>> weak function. So if you can implement one for RZG2 , then that function can
>> return CPU_TYPE_RZG2_something ; and rmobile_get_cpu_type() for RZG2
>> can be implemented using the match on /compatible string .
>>
>> Take a look at how arch/arm/mach-rmobile/cpu_info-rcar.c implements it
>> using PRR, you might need cpu_info-rzg.c I think.
> 
> As mentioned in the commit message PRR values of both R-Car M3-W and RZ/G2M are identical. So there is no need for separate cpu_info-rzg.c.
> I believe it is duplication of code.

I wonder whether it wouldn't be easier to simply ignore PRR on RZG
altogether, and simply match on the /compatible string from the DT.

> We are matching PRR first (device binding)  and then use TFA SoC compatible string to differentiate from R-Car family.
> Please see the diff below[3].

I wonder whether we need the PRR matching at all ?

>> Also, I hope there should already be some function to which you provide a
>> compatible string and a table of supported compatible strings (of match
>> table), from which it will return the .data field of the matching entry in that
>> table. And that .data field can already be the CPU_TYPE_RZG_something , so
>> you don't have to implement the table look up again.
>>
>> What do you think ?
> 
> Device binding is important use case, run time you need to match PRR, that is same for both RCar-M3W and RZ/G2E.
> In RZ/G2 case, we miss device binding if just go with TFA compatible Approach. So we need both.
> 
> What do you think?
> 
> [3]
> +static const struct {
> +char *compatible;
> +u16 cpu_type;
> +u8 cpu_name[10];
> +} tfa_cpuinfo[] = {
> +{ "renesas,r8a774a1", RMOBILE_CPU_TYPE_R8A774A1, "R8A774A1" },
> +{ },
> +};

btw Can you please fix your mailer so it doesn't drop indent ? It's real
hard to read the code.

[...]