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

[Nishanth Menon]

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?

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