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.
