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