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

[Apurva Nandan]

On 20/01/24 01:04, Nishanth Menon wrote:
> 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?
>
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.

>
> [...]
>
> 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)

-- 
Regards,
Apurva Nandan,
Texas Instruments.