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

[Nishanth Menon <nm@ti.com>]

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) {
	ret = uclass_next_device_err(&ret);
	if (ret) /* Question: How do we differentiate between valid
		  * failure and next instance not being present? */
		break;
	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.

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D