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

On 23:20-20240119, Apurva Nandan wrote:

[...]

+void k3_spl_init(void) +{

• struct udevice *dev;
• int ret;
• /*

• * Cannot delay this further as there is a chance that
  

• * K3_BOOT_PARAM_TABLE_INDEX can be over written by SPL MALLOC section.
  

• */
  

• store_boot_info_from_rom();
• /* Make all control module registers accessible */
• ctrl_mmr_unlock();
• if (IS_ENABLED(CONFIG_CPU_V7R)) {

• disable_linefill_optimization();
  

• setup_k3_mpu_regions();
  

• }
• /* Init DM early */
• ret = spl_early_init();
• /* Prepare console output */
• preloader_console_init();
• if (IS_ENABLED(CONFIG_CPU_V7R)) {

• /*
  

•  * Process pinctrl for the serial0 a.k.a. WKUP_UART0 module and continue
  

•  * regardless of the result of pinctrl. Do this without probing the
  

•  * device, but instead by searching the device that would request the
  

•  * given sequence number if probed. The UART will be used by the system
  

•  * firmware (SYSFW) image for various purposes and SYSFW depends on us
  

Nit pick - there is no SYSFW image anymore - it is either TIFS or DM.

•  * to initialize its pin settings.
  

•  */
  

• ret = uclass_find_device_by_seq(UCLASS_SERIAL, 0, &dev);
  

• if (!ret)
  

• 	pinctrl_select_state(dev, "default");
  

• /*
  

•  * Load, start up, and configure system controller firmware. Provide
  

•  * the U-Boot console init function to the SYSFW post-PM configuration
  

•  * callback hook, effectively switching on (or over) the console
  

•  * output.
  

•  */
  

• k3_sysfw_loader(is_rom_loaded_sysfw(&bootdata), NULL, NULL);
  

• if (IS_ENABLED(CONFIG_SPL_CLK_K3)) {
  

• 	/*
  

• 	 * Force probe of clk_k3 driver here to ensure basic default clock
  

• 	 * configuration is always done for enabling PM services.
  

• 	 */
  

• 	ret = uclass_get_device_by_driver(UCLASS_CLK,
  

• 					  DM_DRIVER_GET(ti_clk),
  

• 					  &dev);
  

• 	if (ret)
  

• 		panic("Failed to initialize clk-k3!\n");
  

• }
  

• remove_fwl_configs(cbass_hc_cfg0_fwls, ARRAY_SIZE(cbass_hc_cfg0_fwls));
  

• remove_fwl_configs(cbass_hc2_fwls, ARRAY_SIZE(cbass_hc2_fwls));
  

• remove_fwl_configs(cbass_rc_cfg0_fwls, ARRAY_SIZE(cbass_rc_cfg0_fwls));
  

• remove_fwl_configs(infra_cbass0_fwls, ARRAY_SIZE(infra_cbass0_fwls));
  

• remove_fwl_configs(mcu_cbass0_fwls, ARRAY_SIZE(mcu_cbass0_fwls));
  

• remove_fwl_configs(wkup_cbass0_fwls, ARRAY_SIZE(wkup_cbass0_fwls));
  

• remove_fwl_configs(navss_cbass0_fwls, ARRAY_SIZE(navss_cbass0_fwls));
  

• }
• /* Output System Firmware version info */
• k3_sysfw_print_ver();

+}

+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?

• 	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.

[...]

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?