Re: [PATCH v8 02/16] arm: mach-k3: Add basic support for J784S4 SoC definition

[Apurva Nandan <a-nandan@ti.com>]

Hi,

On 24/01/24 02:17, Nishanth Menon wrote:
> On 20:21-20240123, Apurva Nandan wrote:
> [...]
>>>> +void k3_mem_init(void)
>>>> +{
>>>> +	struct udevice *dev;
>>>> +	int ret, ctr = 1;
>>>> +
>>>> +	if (IS_ENABLED(CONFIG_K3_J721E_DDRSS)) {
>>>> +		ret = uclass_get_device(UCLASS_RAM, 0, &dev);
>>>> +		if (ret)
>>>> +			panic("DRAM 0 init failed: %d\n", ret);
>>>> +
>>>> +		while (dev) {
>>> why loop on dev? is it possible to have ret != 0 and dev = 0?
>>>
>> Some variable needs to be used for loop condition, do you want it to be ret?
>> or maybe you can suggest your idea for this please.
>>>> +			ret = uclass_next_device_err(&dev);
>>>> +			if (ret) {
>>>> +				printf("Initialized %d DRAM controllers\n", ctr);
>>>> +				break;
>>>> +			}
>>>> +			ctr++;
>>> What is the use of ctr++ ?? please do a limit check for instances.
>> This is to keep the logic independent of board evm, so that no include of
>> EVM config is needed.
>> ctr is just used to notify user about how many DDR are up during boot, else
>> it is not needed.
>>
>> I can remove the ctr and printf, if you want.
>>
>> For a limit check, how can we get number of DDR instances on the EVM, I
>> don't know, can you please suggest some way?
>>
>> There is no config that stores this info afaik.
> Why? J784s4 has only specific number of controllerns, correct?
>
> A variant of the below -> but still have a question:
>
> while (ctrl < J784S4_MAX_CONTROLLERS) {

Is J784S4_MAX_CONTROLLERS going to be a #define in j784s4_init.c

Or a Kconfig option like:

config DDR_MAX_CONTROLLERS
     int "Max number of DRAM controllers"
     default 4 if SOC_K3_J784S4
     default 1
     help

> 	ret = uclass_next_device_err(&ret);
> 	if (ret) /* Question: How do we differentiate between valid
> 		  * failure and next instance not being present? */
> 		break;

how about:

             ...
             if (ret == -ENODEV)
                 break;

             if (ret)
                  panic("DRAM %d init failed: %d\n", ctrl,  ret);
             ...

> 	ctrl++;
> }
>
> info("Initialized %d DRAM controllers\n", ctrl - 1);
>>> [...]
>>>
>>> Next time, please respond to the review comment questions so that I
>>> know that you have considered and decided something is not necessary
>>> or something was missed in the new version - for example what happened
>>> to mmc_stop/restart?
>> mmc_stop/restart were removed (mentioned in series changelog)
> Mentioning in diffstat of the patch helps give people the context of the
> change w.r.t the path itself.
okay

-- 
Regards,
Apurva Nandan,
Texas Instruments.