Introduce platform CPU init for ARM

[zbb]

Summary

Add another callback for the platform initialization.
This function can be used when additional configuration
is required to enable platform-specific quirks related
to the CPU/MMU.

Submitted by: Zbigniew Bodek <[e-mail]>
Obtained from: Semihalf
Sponsored by: Stormshield

[andrew]

This won't work in the PLATFORM case. You are trying to dereference the platform object too early in the boot.

sys/arm/arm/mp_machdep.c

160

This must be after platform_probe_and_attach which must be after OF_init.

[andrew]

For the cases I have seen where this is used I would think would be better served by calling platform_late_init late in init_secondary.

The only issue is the existing platform_late_init handlers may need to be updated to only run their code on CPU0.

[zbb]

sys/arm/arm/mp_machdep.c

160

In that case, we may not be able to use platform_ code here. platform_cpu_init() has to be called before cpuinfo_get_actlr_modifier().
Check skra's comment here D10218

[zbb]

Let us summarize what we deal with.

1. For our needs we need to be able to call a code that modifies CPU/MMU settings before reinit_mmu is called.
2. We need to call it for each CPU (for example when we want to enable L1 prefetch)
3. It seems that we cannot use platform_ code that early as it won't work with the GENERIC kernel.

Any suggestions?

[mmel]

I think that this patch is not longer needed?
Both issues (device memory class remap, ACTRL modification) can be processed in platform_late_init().
I'm right?

[zbb]

In D10682#222184, @meloun-miracle-cz wrote:

I think that this patch is not longer needed?
Both issues (device memory class remap, ACTRL modification) can be processed in platform_late_init().
I'm right?

I would prefer to keep this function and drop other patches (for example ACTLR). This is because some of the quirks are per-CPU and is is more clear to put them into the CPU-something function.
Also if we use platform_late_init there would be a need to rework all late_init implementations so that they could be used in the init_secondary.
The suggested approach seems to be less aggressive (although we would need to move it after platform_probe_and_attach() ). What do you think?

[andrew]

In D10682#222298, @zbb wrote:

Also if we use platform_late_init there would be a need to rework all late_init implementations so that they could be used in the init_secondary.

It's not very difficult to rework platform_late_init, see D10733.

[zbb]

In D10682#222312, @andrew wrote:

In D10682#222298, @zbb wrote:

Also if we use platform_late_init there would be a need to rework all late_init implementations so that they could be used in the init_secondary.

It's not very difficult to rework platform_late_init, see D10733.

I'm not saying it is difficult, I'm saying that IMHO it's a little ambiguous to put the CPU/MMU specific code into platform_late_init. Also if we need to change all implementations anyway, why not create a CPU-specific call.
Nevertheless, it is not my call. I will adjust to what you prefer. Please commit this one and we will implement our platform late init guts.

[mw_semihalf.com]

Hi @meloun-miracle-cz and @andrew

Pretty long discussion on this patch - do you think you can agree to one acceptable solution, from a couple mentioned above?

Thanks,
Marcin

[mmel]

Hmm, allow me a short summary:

• This patch simply cannot work. On boot core, the platform_cpu_init() is called before platform is recognized. On secondary cores, platform_cpu_init() is called before given core enters to coherency domain, so each single memory write (outside of stack) causes disaster..
• I don't see any way how we can do any platform specific operation before reinit_mmu() is called.
• The goal can be reached by different, much more consistent way.

I have nothing against Andrew proposal about calling platform_late_init() on all cores (but please see @skra comment about identifying boot core ID, and my about right place for calling it on secondary cores), if you find it useful.

OR

due to other reason, I just committed tunable CPU quirks (very similar to your ACTRL patch). With this, you can add something like

void
cpuinfo_set_cpu_quirks(uint32_t actlr_set, uint32_t actlr_mask)
{
	uint32_t actlr_old;
	uint32_t actlr_new;
	
	cpu_quirks_actlr_set |= actlr_set;
	cpu_quirks_actlr_mask |= actlr_mask;
	actlr_old = cp15_actlr_get();
	actlr_new = actlr_old & ~cpu_quirks_actlr_mask;
	actlr_new ^= cpu_quirks_actlr_set;
	if (actlr_new != actlr_old)
		cp15_actlr_set(actlr_new);
}

or so to cpuinfo.c and use it from your platform_init() code.

[zbb]

@meloun-miracle-cz if I understand correctly, in your commit you simply create a environment variable to modify ACTLR from the loader.conf or etc.
We can use that solution directly for our purposes without any further modifications, platform_late_init() complications and etc. Until ARMADA38X will be compatible with the GENERIC kernel we could potentially add environment configuration file to be compiled-into the kernel. The latter would need testing whether it works with the loader.conf file as well or not (I expect problems with that).
Regardless of the settings for Armada38X we can drop this revision completely and start using environment variables. Agree?

BTW. As for the unsolvable problem with the memory attributes change from DEVICE to SO we could use the same method as you used for ACTLR modification, means we could create another sysctl quirk and so on...
In platform code for Armada38X we could then check whether memory attributes are set as expected and if not we could fire an assertion. What do you think?

[mmel]

In D10682#231259, @zbb wrote:

@meloun-miracle-cz if I understand correctly, in your commit you simply create a environment variable to modify ACTLR from the loader.conf or etc.
We can use that solution directly for our purposes without any further modifications, platform_late_init() complications and etc. Until ARMADA38X will be compatible with the GENERIC kernel we could potentially add environment configuration file to be compiled-into the kernel. The latter would need testing whether it works with the loader.conf file as well or not (I expect problems with that).
Regardless of the settings for Armada38X we can drop this revision completely and start using environment variables. Agree?

Yep, exactly, I agree.

BTW. As for the unsolvable problem with the memory attributes change from DEVICE to SO we could use the same method as you used for ACTLR modification, means we could create another sysctl quirk and so on...
In platform code for Armada38X we could then check whether memory attributes are set as expected and if not we could fire an assertion. What do you think?

Imho, this problem is already solved by pmap_remap_vm_attr() (r318021) - or I didn't notice something important?

[zbb]

In D10682#231585, @meloun-miracle-cz wrote:

BTW. As for the unsolvable problem with the memory attributes change from DEVICE to SO we could use the same method as you used for ACTLR modification, means we could create another sysctl quirk and so on...
In platform code for Armada38X we could then check whether memory attributes are set as expected and if not we could fire an assertion. What do you think?

Imho, this problem is already solved by pmap_remap_vm_attr() (r318021) - or I didn't notice something important?

OK. I though that using this function so late was discussed to be not allowed. But if it is OK to use in platform_late_init than this concludes this issue. So I understand that will get a green light after putting pmap_remap_vm_attr() to platform_late_init() ?

[mmel]

In D10682#231591, @zbb wrote:

In D10682#231585, @meloun-miracle-cz wrote:

BTW. As for the unsolvable problem with the memory attributes change from DEVICE to SO we could use the same method as you used for ACTLR modification, means we could create another sysctl quirk and so on...
In platform code for Armada38X we could then check whether memory attributes are set as expected and if not we could fire an assertion. What do you think?

Imho, this problem is already solved by pmap_remap_vm_attr() (r318021) - or I didn't notice something important?

OK. I though that using this function so late was discussed to be not allowed. But if it is OK to use in platform_late_init than this concludes this issue. So I understand that will get a green light after putting pmap_remap_vm_attr() to platform_late_init() ?

Yes, right. pmap_remap_vm_attr() is intended for usage from platform_late_init() (so it's called from boot core only, before first device gets mapped).

[zbb]

Abandon this revision. Will use environment variables.