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

[Nishanth Menon]

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.

> > 	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,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D