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

[Apurva Nandan]

On 29/01/24 17:49, Nishanth Menon wrote:
> On 17:02-20240129, Apurva Nandan wrote:
>> 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
>>
> I dont see a need for Kconfig - but that is just me.
Ohkay, sending with #define.
>>> 	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);
>>              ...
> What ever is appropriate.
>
>
-- 
Regards,
Apurva Nandan,
Texas Instruments.