ARM core support for hardware I/O coherency in non-SMP platforms

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

- The pages must be set as shareable.

- The SCU must be enabled

Quoted 3 lines · Thomas Petazzoni

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

Quoted 3 lines

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

Arguably, I would make this the default for ARMv6+ CPUs even if UP. It's a hint that the CPU may or may not ignore but it shouldn't break anything (well, maybe some artificial benchmarks designed to show that write-allocate caches are bad).

Quoted 5 lines

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

What about setting this bit in the firmware/bootloader? It's a sane initialisation firmware should do.

Quoted 1 line

- The pages must be set as shareable.

Here you may have some conflict between the initial page tables set in __create_page_tables as non-shareable (that's unless MPIDR shows it as SMP but I guess not since smp-on-up kicks in).

I have to think a bit
more about the implications (the ARM ARM has a chapter on mismatched memory attributes and I think it talks about shareable vs non-shareable).

Quoted 1 line

- The SCU must be enabled

Again, could the firmware do this?

Quoted 4 lines · Catalin Marinas

Quoted 3 lines · Thomas Petazzoni

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

Quoted 8 lines

Quoted 3 lines

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

Arguably, I would make this the default for ARMv6+ CPUs even if UP. It's a hint that the CPU may or may not ignore but it shouldn't break anything (well, maybe some artificial benchmarks designed to show that write-allocate caches are bad).

So, something like:

arch/arm/mm/mmu.c +1 -1

index b68c6b2..a1d6845 100644
@@ -57,7 +57,7 @@ pmd_t *top_pmd;
 #define CPOLICY_WRITEBACK      3
 #define CPOLICY_WRITEALLOC     4
 
-static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK;
+static unsigned int cachepolicy __initdata = CPOLICY_WRITEALLOC;
 static unsigned int ecc_mask __initdata = 0;
 pgprot_t pgprot_user;
 pgprot_t pgprot_kernel;

arch/arm/mm/mmu.c +0 -0

 > >  * On Armada 375/38x (which have single core and dual core variants)

Quoted 7 lines

Quoted 4 lines

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

What about setting this bit in the firmware/bootloader? It's a sane initialisation firmware should do.

I'm sorry, but I don't buy the "fix your bootloader" argument. For various reasons:

- The Linux kernel policy has always been to do *more* things in the kernel, to avoid relying on crappy vendor-specific bootloaders, and avoid having bugs that cannot be fixed at the kernel level. Going in the opposite direction looks completely backward.

- Doing so means crazy complex dependencies of kernel versions against bootloader versions, which are a nightmare for users.

- The kernel is setting the SMP and TLB broadcast bit already in proc-v7.s when the system is SMP. What would these actions have to be done in the bootloader when the system is non-SMP, but I/O coherent, and done in the kernel when the system is SMP? This doesn't make sense.

Quoted 5 lines

Quoted 1 line

- The pages must be set as shareable.

Here you may have some conflict between the initial page tables set in __create_page_tables as non-shareable (that's unless MPIDR shows it as SMP but I guess not since smp-on-up kicks in).

Quoted 4 lines

I have to think a bit
more about the implications (the ARM ARM has a chapter on mismatched memory attributes and I think it talks about shareable vs non-shareable).

Unfortunately, __create_page_tables is called so early that I don't know if it's possible to access 'struct machine_desc' at this point to know whether the platform needs shareable pages or not.

Also, don't we have the same problem with the TTB_FLAGS_{SMP,UP) ?

Quoted 3 lines

Quoted 1 line

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

It's either it's *always* done by the
bootloader and we completely remove the SCU enabling code in the kernel, and add to the ARM boot protocol that enabling the SCU is the responsibility of the bootloader, or the kernel does the SCU enabling in all situations.

Quoted 4 lines · Thomas Petazzoni

Quoted 3 lines · Catalin Marinas

Quoted 2 lines · Thomas Petazzoni

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

The I/O coherency is an SoC property rather than an ARM architecture property. I want to separate the two so that the kernel can boot a significant part assuming sane architecture settings. Once you have DT available and start loading device drivers, you are in the SoC realm and you can do whatever initialisation for buses, interconnects, but not going back to change architected settings.

Quoted 10 lines · Catalin Marinas

Quoted 8 lines · Thomas Petazzoni

Quoted 4 lines · Catalin Marinas

Quoted 1 line · Thomas Petazzoni

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

Quoted 6 lines

The I/O coherency is an SoC property rather than an ARM architecture property. I want to separate the two so that the kernel can boot a significant part assuming sane architecture settings. Once you have DT available and start loading device drivers, you are in the SoC realm and you can do whatever initialisation for buses, interconnects, but not going back to change architected settings.

The SCU has nothing to do with the architecture and really is part of the SOC.

Let's look at this another way. Are there any usecases where you would not enable the SCU? If any cores are coherent or the ACP is coherent, it must be on. So that leaves all core in AMP mode. In this case, does it matter if the SCU is enabled or not?

Quoted 19 lines · Rob Herring

Quoted 10 lines · Catalin Marinas

Quoted 8 lines · Thomas Petazzoni

Quoted 4 lines · Catalin Marinas

Quoted 1 line · Thomas Petazzoni

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

Quoted 6 lines

The I/O coherency is an SoC property rather than an ARM architecture property. I want to separate the two so that the kernel can boot a significant part assuming sane architecture settings. Once you have DT available and start loading device drivers, you are in the SoC realm and you can do whatever initialisation for buses, interconnects, but not going back to change architected settings.

Quoted 2 lines

The SCU has nothing to do with the architecture and really is part of the SOC.

Indeed, it's not part of the architecture but I don't see it any different than other early configuration like SDRAM controller.

Quoted 4 lines

Let's look at this another way. Are there any usecases where you would not enable the SCU? If any cores are coherent or the ACP is coherent, it must be on. So that leaves all core in AMP mode. In this case, does it matter if the SCU is enabled or not?

I don't fully follow the question. You may not enable the SCU if you don't care at all about the SMP mode or other coherency like ACP. Otherwise it should be enabled, the latest before the secondaries start.

Quoted 31 lines · Catalin Marinas

Quoted 19 lines · Rob Herring

Quoted 10 lines · Catalin Marinas

Quoted 8 lines · Thomas Petazzoni

Quoted 4 lines · Catalin Marinas

Quoted 1 line · Thomas Petazzoni

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

Quoted 6 lines

The I/O coherency is an SoC property rather than an ARM architecture property. I want to separate the two so that the kernel can boot a significant part assuming sane architecture settings. Once you have DT available and start loading device drivers, you are in the SoC realm and you can do whatever initialisation for buses, interconnects, but not going back to change architected settings.

Quoted 2 lines

The SCU has nothing to do with the architecture and really is part of the SOC.

Indeed, it's not part of the architecture but I don't see it any different than other early configuration like SDRAM controller.

Quoted 4 lines

Let's look at this another way. Are there any usecases where you would not enable the SCU? If any cores are coherent or the ACP is coherent, it must be on. So that leaves all core in AMP mode. In this case, does it matter if the SCU is enabled or not?

Quoted 3 lines

I don't fully follow the question. You may not enable the SCU if you don't care at all about the SMP mode or other coherency like ACP. Otherwise it should be enabled, the latest before the secondaries start.

My question is should the kernel unconditionally enable the SCU regardless of CONFIG_SMP or any other condition? Is there harm in enabling the SCU when in AMP mode?

Quoted 3 lines

My question is should the kernel unconditionally enable the SCU regardless of CONFIG_SMP or any other condition? Is there harm in enabling the SCU when in AMP mode?

I don't see why we couldn't always enable the SCU at boot, whether the OS runs SMP, AMP or UP mode.

Quoted 10 lines

arch/arm/mm/mmu.c +2 -1

-static unsigned int cachepolicy __initdata = CPOLICY_WRITEBACK;
+static unsigned int cachepolicy __initdata = CPOLICY_WRITEALLOC;
 static unsigned int ecc_mask __initdata = 0;
 pgprot_t pgprot_user;
 pgprot_t pgprot_kernel;

@@ -408,14 +408,14 @@ static void __init build_mem_type_table(void)
                if (cachepolicy > CPOLICY_WRITETHROUGH)
                        cachepolicy = CPOLICY_WRITETHROUGH;
 #endif
+               if (cachepolicy > CPOLICY_WRITEBACK)

arch/arm/mm/mmu.c static void __init build_mem_type_table(void) · line 413

Quoted 2 lines

arch/arm/mm/mmu.c +1 -0

+                       cachepolicy = CPOLICY_WRITEBACK;
        }

There is some more clean-up to do in this file as there are other assumptions about cachepolicy. For example, early_cachepolicy() sets it back to writeback (and possibly the initial kernel mapping to avoid aliases, though having the same type, just the allocation hint doesn't really cause problems).

Quoted 6 lines

(quoted diff) +0 -0

 > >  * On Armada 375/38x (which have single core and dual core variants)

Quoted 5 lines

Quoted 4 lines

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

Quoted 5 lines

Quoted 2 lines

What about setting this bit in the firmware/bootloader? It's a sane initialisation firmware should do.

I'm sorry, but I don't buy the "fix your bootloader" argument. For various reasons:

Well, for arm64 people should get used to this because I'm not taking code for setting up things like SCU and ACTLR during boot.

Quoted 4 lines

- The Linux kernel policy has always been to do *more* things in the kernel, to avoid relying on crappy vendor-specific bootloaders, and avoid having bugs that cannot be fixed at the kernel level. Going in the opposite direction looks completely backward.

There is no written policy here for how close to the hardware reset the kernel should run. With the same arguments people may want to initialise the DRAM controller in Linux while booting in SRAM temporarily (or ROM/flash + XIP).

But in this particular case, let's look at ACTLR. It's a secure-only register, so what if someone decides to deploy Linux as a non-secure OS on this platform? Do you add a config option to have a different kernel build?

Quoted 2 lines

- Doing so means crazy complex dependencies of kernel versions against bootloader versions, which are a nightmare for users.

Because we didn't have a clear policy here, the secure extensions came in later when we were already doing things like this. It's more difficult to enforce it now but we face such issues regularly. For example, on several occasions we had TI asking for per-SoC CPU initialisation (possibly as addition to __v7_setup) for specific SMC calls to the firmware which breaks single Image.

It make sense a lot of sense to place sane initialisation in a SoC-specific firmware/bootloader but whether we want to enforce such policy is Russell's call.

Quoted 5 lines

Unfortunately, __create_page_tables is called so early that I don't know if it's possible to access 'struct machine_desc' at this point to know whether the platform needs shareable pages or not.

Also, don't we have the same problem with the TTB_FLAGS_{SMP,UP) ?

I don't think we do because we use the ALT_{SMP,UP} for TTB settings and we have the smp-on-up fixup running before __v7_setup.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

- The SCU must be enabled

Again, could the firmware do this?

See above. If the kernel does it for SMP cases, why wouldn't it do it also for !SMP I/O coherent cases?

The I/O coherency is an SoC property rather than an ARM architecture property. I want to separate the two so that the kernel can boot a significant part assuming sane architecture settings. Once you have DT available and start loading device drivers, you are in the SoC realm and you can do whatever initialisation for buses, interconnects, but not going back to change architected settings.

Quoted 5 lines

It's either it's *always* done by the
bootloader and we completely remove the SCU enabling code in the kernel, and add to the ARM boot protocol that enabling the SCU is the responsibility of the bootloader, or the kernel does the SCU enabling in all situations.

I'm always for the former (and that's the case for arm64), though for some older boards it's too late to change.

Quoted 6 lines · Catalin Marinas

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

Quoted 8 lines

Quoted 5 lines

Quoted 2 lines

What about setting this bit in the firmware/bootloader? It's a sane initialisation firmware should do.

I'm sorry, but I don't buy the "fix your bootloader" argument. For various reasons:

Well, for arm64 people should get used to this because I'm not taking code for setting up things like SCU and ACTLR during boot.

I have no problem about the bootloader doing some initialization, so if that's a requirement for arm64, no problem. But it should be a requirement regardless of the kernel configuration: in the current kernel, some initialization is done when CONFIG_SMP is enabled. And now that the same initialization is needed in !CONFIG_SMP, I'm asked to do it in the bootloader. That's the thing I don't buy.

Quoted 4 lines

Quoted 4 lines

Unfortunately, __create_page_tables is called so early that I don't know if it's possible to access 'struct machine_desc' at this point to know whether the platform needs shareable pages or not.

Quoted 4 lines

Quoted 1 line

Also, don't we have the same problem with the TTB_FLAGS_{SMP,UP) ?

I don't think we do because we use the ALT_{SMP,UP} for TTB settings and we have the smp-on-up fixup running before __v7_setup.

Ok. For Armada 370, I don't need shareable pages. For Armada 380, I need shareable pages, but even if the processor is single core, I believe it pretends to be SMP in MPIDR.

Quoted 8 lines

Quoted 5 lines

It's either it's *always* done by the
bootloader and we completely remove the SCU enabling code in the kernel, and add to the ARM boot protocol that enabling the SCU is the responsibility of the bootloader, or the kernel does the SCU enabling in all situations.

I'm always for the former (and that's the case for arm64), though for some older boards it's too late to change.

Indeed. So how do we move forward?

Quoted 6 lines

I have no problem about the bootloader doing some initialization, so if that's a requirement for arm64, no problem. But it should be a requirement regardless of the kernel configuration: in the current kernel, some initialization is done when CONFIG_SMP is enabled. And now that the same initialization is needed in !CONFIG_SMP, I'm asked to do it in the bootloader. That's the thing I don't buy.

And that's a hard thing for me to sell for arm32 ;) because of the current code already doing this.

One of the issues is secure vs non-secure kernel deployment and secure-only registers like ACTLR (and even SCU). I've seen SoCs for which the mainline kernel assumes secure mode but it's deployed as non-secure in the field with out of tree modifications to make it work. I would rather have a single kernel image for both cases.

Quoted 6 lines

Quoted 6 lines

Quoted 5 lines

Unfortunately, __create_page_tables is called so early that I don't know if it's possible to access 'struct machine_desc' at this point to know whether the platform needs shareable pages or not.

Also, don't we have the same problem with the TTB_FLAGS_{SMP,UP) ?

Quoted 6 lines

Quoted 2 lines

I don't think we do because we use the ALT_{SMP,UP} for TTB settings and we have the smp-on-up fixup running before __v7_setup.

Ok. For Armada 370, I don't need shareable pages. For Armada 380, I need shareable pages, but even if the processor is single core, I believe it pretends to be SMP in MPIDR.

You'd need to check this.

Quoted 4 lines

Quoted 2 lines

I'm always for the former (and that's the case for arm64), though for some older boards it's too late to change.

Indeed. So how do we move forward?

As I said previously, it's not my call, just a recommendation. It's up to the arm/arm-soc maintainers to take this. Give the arm32 SMP initialisation precedent, forcing it for an existing platform is a harder sell.

Quoted 7 lines · Catalin Marinas

Quoted 6 lines

I have no problem about the bootloader doing some initialization, so if that's a requirement for arm64, no problem. But it should be a requirement regardless of the kernel configuration: in the current kernel, some initialization is done when CONFIG_SMP is enabled. And now that the same initialization is needed in !CONFIG_SMP, I'm asked to do it in the bootloader. That's the thing I don't buy.

Quoted 8 lines

And that's a hard thing for me to sell for arm32 ;) because of the current code already doing this.

One of the issues is secure vs non-secure kernel deployment and secure-only registers like ACTLR (and even SCU). I've seen SoCs for which the mainline kernel assumes secure mode but it's deployed as non-secure in the field with out of tree modifications to make it work. I would rather have a single kernel image for both cases.

As things are today, I believe the Armada 370/XP/375/38x are always booted in secure mode. Couldn't the kernel detect if it's running secure or non-secure. If it's running secure, it can do all the ACTLR configuration (and other things that require being in secure mode). If it's running non-secure, then it doesn't do this configuration, and assumes the firmware/bootloader did it.

Quoted 13 lines

Quoted 12 lines

Quoted 8 lines

Quoted 5 lines

Unfortunately, __create_page_tables is called so early that I don't know if it's possible to access 'struct machine_desc' at this point to know whether the platform needs shareable pages or not.

Also, don't we have the same problem with the TTB_FLAGS_{SMP,UP) ?

I don't think we do because we use the ALT_{SMP,UP} for TTB settings and we have the smp-on-up fixup running before __v7_setup.

Ok. For Armada 370, I don't need shareable pages. For Armada 380, I need shareable pages, but even if the processor is single core, I believe it pretends to be SMP in MPIDR.

Quoted 1 line

You'd need to check this.

I got the confirmation. *But* this is only going to affect CONFIG_SMP builds. Armada 380 being single core, we want I/O coherency to work on it in !CONFIG_SMP configurations. And in !CONFIG_SMP configuration, it's always ALT_UP() that is used, regardless of what MPIDR says. And therefore the TTB_FLAGS_UP will be used.

Quoted 5 lines

Quoted 4 lines

Quoted 2 lines

I'm always for the former (and that's the case for arm64), though for some older boards it's too late to change.

Indeed. So how do we move forward?

Quoted 4 lines

As I said previously, it's not my call, just a recommendation. It's up to the arm/arm-soc maintainers to take this. Give the arm32 SMP initialisation precedent, forcing it for an existing platform is a harder sell.

Ok. So I should keep going with the approach proposed in my patch series, obviously after fixing the various other comments that were posted?

Quoted 1 line

- The pages must be set as shareable.

Here you may have some conflict between the initial page tables set in __create_page_tables as non-shareable (that's unless MPIDR shows it as SMP but I guess not since smp-on-up kicks in). I have to think a bit more about the implications (the ARM ARM has a chapter on mismatched memory attributes and I think it talks about shareable vs non-shareable).

Hello,

Disclaimer: this is an RFC patch series.

Several of the latest Marvell Armada EBU platforms have a special feature that is fairly uncommon on ARM platforms: hardware I/O coherency. This feature allows to remove all cache maintenance operations that are normally needed when doing DMA transfers (only an I/O sync barrier is needed for DMA transfers from device).

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

* On Armada XP (which has dual core and quad core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP bit in the Auxiliary Functional Modes Control Register 0 must be set (remember the CPU core is PJ4B)

- The pages must be set as shareable.

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

- The pages must be set as shareable.
- The SCU must be enabled

All of these requirements are met when the kernel is configured with CONFIG_SMP *and* when the processor is actually a multiple core processors (otherwise, due to the CONFIG_SMP_ON_UP logic, is_smp() returns false, and most of the requirements above are not met).

For example, as of today, Armada 370 is broken because even if we build the kernel CONFIG_SMP, the pages do not have the "write allocate" attribute, because Armada 370 is single core, therefore the CONFIG_SMP_ON_UP logic decides that we're not on an SMP system, and therefore is_smp() returns false. Therefore, the fact that hardware I/O coherency is enabled on Armada 370 today is incorrect, and there is no way to fix that without making changes to the ARM core code.

Similarly, the Armada 380 (single core variant of the Armada 385) will have the same problem: it's a single core processor, but that requires several characteristics of an SMP configuration to properly use hardware I/O coherency.

Also:

- Not being able to use hardware I/O coherency on Armada 370 and Armada 380 is not really an option, as it is a major feature of those SoCs.

- Having to enable CONFIG_SMP is also not very nice, as it comes with other performance penalties that could be avoided by using a !CONFIG_SMP configuration on those single core systems. And again, enabling CONFIG_SMP does *not* work for single core processors due to CONFIG_SMP_ON_UP.

Therefore, this RFC patch series proposes one solution to allow these requirements to be met in the situations we are interested. The patch series is *only* a proposal, and I'm definitely interested in hearing about other implementation possibilites to make things look nicer.

Basically, the patch series goes like this:

* PATCH 1 adds a 'flags' field to 'struct machine_desc' so that each platform can tell the ARM kernel core some of its requirements. We maybe could have used the Device Tree as well, but accessing the Device Tree as early as in paging_init() is a bit problematic as far as I understand.

Two flags are defined: MACHINE_NEEDS_CPOLICY_WRITEALLOC and MACHINE_NEEDS_SHAREABLE_PAGES. We need separate flags because Armada 370 cannot have the shareable attribute on page tables.

Quoted 6 lines

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

Why do we want !SMP to be no write allocate in the first place? Seems like we should always enable write-allocate at least for v7.

Quoted 5 lines

* On Armada XP (which has dual core and quad core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP bit in the Auxiliary Functional Modes Control Register 0 must be set (remember the CPU core is PJ4B)

I know you don't like my answer of this should be done in the bootloader.

Quoted 1 line

- The pages must be set as shareable.

What is the impact of setting shareable bit for !SMP? I would think it would be a don't care for true UP systems. Does the 370 actually not work with shareable pages? AMP systems could have an issue though.

Quoted 5 lines

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

bootloader :)

Quoted 2 lines

- The pages must be set as shareable.
- The SCU must be enabled

This is also needed on A9s using ACP port. (Could be in the bootloader as well.)

While I understand the we can't update the bootloader/firmware arguments, we also would reasonably expect ACPI based server systems from the same vendor. Those systems are going to have to get things right in the firmware and thus will require firmware updates in the early stages before production systems and OS's are ready. I don't know that the vendor mindset will ever be changed other than by refusing to accept changes upstream which should be fixed in firmware (and distros also refusing to carry them).

Quoted 4 lines

All of these requirements are met when the kernel is configured with CONFIG_SMP *and* when the processor is actually a multiple core processors (otherwise, due to the CONFIG_SMP_ON_UP logic, is_smp() returns false, and most of the requirements above are not met).

I think there are some other problems here with is_smp(). For example, there are several cases where it is used for determining to read the MPIDR register. For things such as matching MPIDR to DT cpu nodes, we need that to work for !SMP kernels.

Quoted 8 lines

Basically, the patch series goes like this:

* PATCH 1 adds a 'flags' field to 'struct machine_desc' so that each platform can tell the ARM kernel core some of its requirements. We maybe could have used the Device Tree as well, but accessing the Device Tree as early as in paging_init() is a bit problematic as far as I understand.

Quoted 3 lines

Two flags are defined: MACHINE_NEEDS_CPOLICY_WRITEALLOC and MACHINE_NEEDS_SHAREABLE_PAGES. We need separate flags because Armada 370 cannot have the shareable attribute on page tables.

I think these are too specific. We used to have arch_is_coherent() which somewhat does what you need, but it was not multi-platform friendly. I think you need something like "is_io_coherent" and/or "is_mp".

Quoted 5 lines · Rob Herring

Quoted 5 lines

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

* On Armada 370 (a single core processor)

Quoted 4 lines

Quoted 1 line

- The cache policy of pages must be set to "write allocate".

Why do we want !SMP to be no write allocate in the first place? Seems like we should always enable write-allocate at least for v7.

Ok, seems like it matches the suggestion from Catalin. I've sent a quick patch in my reply to Catalin, if you could have a look and tell if it's OK, I can use that in my next version.

I really don't see any justification for why it some situations the bootloader would be responsible for it, and why in some other situations the kernel would be responsible for it.

Quoted 10 lines

Quoted 6 lines

Quoted 3 lines

* On Armada 370 (a single core processor)

- The cache policy of pages must be set to "write allocate".

Why do we want !SMP to be no write allocate in the first place? Seems like we should always enable write-allocate at least for v7.

Ok, seems like it matches the suggestion from Catalin. I've sent a quick patch in my reply to Catalin, if you could have a look and tell if it's OK, I can use that in my next version.

It's probably also worth mentioning in your commit log that modern ARM CPUs can change the allocation policy dynamically based on the access pattern (e.g. switch to no-allocate when there is a series of streaming stores), so historical reasons for forcing write-no-allocate aren't applicable on these cores.

Quoted 3 lines

I really don't see any justification for why it some situations the bootloader would be responsible for it, and why in some other situations the kernel would be responsible for it.

Well, for better or worse, we are moving in the direction of Linux running non-secure with a non-trivial amount of higher privileged software running in the system. We can use this as a basis to decide whether or not Linux should be setting configuration bits by looking at whether or not these bits are accessible from non-secure svc mode.

I'm not denying that we've not been following this rule in the past, but given where we're going with ARMv8 and arm64, the sooner people realise that the firmware and bootloader have some duties in the way of system configuration, then the less pain they will have later on when they try to boot Linux.

Dear Rob Herring,

Quoted 5 lines · Rob Herring

Quoted 5 lines

This hardware I/O coherency mechanism needs a set of ARM core requirements to operate properly:

* On Armada 370 (a single core processor)

Quoted 4 lines

Quoted 1 line

- The cache policy of pages must be set to "write allocate".

Why do we want !SMP to be no write allocate in the first place? Seems like we should always enable write-allocate at least for v7.

Ok, seems like it matches the suggestion from Catalin. I've sent a quick patch in my reply to Catalin, if you could have a look and tell if it's OK, I can use that in my next version.

Quoted 7 lines

Quoted 5 lines

* On Armada XP (which has dual core and quad core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP bit in the Auxiliary Functional Modes Control Register 0 must be set (remember the CPU core is PJ4B)

I know you don't like my answer of this should be done in the bootloader.

See my answer to Catalin about this.

Quoted 5 lines

Quoted 1 line

- The pages must be set as shareable.

What is the impact of setting shareable bit for !SMP? I would think it would be a don't care for true UP systems. Does the 370 actually not work with shareable pages? AMP systems could have an issue though.

No, the Armada 370 doesn't work with shareable pages. See my cover letter that details the requirements for the different SoCs, and I've only mentioned shareable pages as part of the requirements for Armada XP, Armada 375 and Armada 38x.

Quoted 6 lines

Quoted 5 lines

* On Armada 375/38x (which have single core and dual core variants)

- The cache policy of pages must be set to "write allocate".
- The SMP and TLB broadcast bits must be set in the Auxiliary Control Register (the core is a Cortex-A9)

Quoted 1 line

bootloader :)

No. It's done by the kernel for the SMP case, there's no reason it should be done in the bootloader for the !SMP I/O coherent case. Or, we remove the SMP and TLB broadcast bit fiddling in the kernel *completely* and make it part of the ARM boot protocol that the bootloader is required to do that.

I really don't see any justification for why it some situations the bootloader would be responsible for it, and why in some other situations the kernel would be responsible for it.

Quoted 5 lines

Quoted 2 lines

- The pages must be set as shareable.
- The SCU must be enabled

This is also needed on A9s using ACP port. (Could be in the bootloader as well.)

Quoted 8 lines

While I understand the we can't update the bootloader/firmware arguments, we also would reasonably expect ACPI based server systems from the same vendor. Those systems are going to have to get things right in the firmware and thus will require firmware updates in the early stages before production systems and OS's are ready. I don't know that the vendor mindset will ever be changed other than by refusing to accept changes upstream which should be fixed in firmware (and distros also refusing to carry them).

See above. It's not only about having issues to get the vendor to fix the firmware. It's also about:

* Having a consistent strategy with regard to what the kernel does and what the bootloader does. Why would the kernel set the SMP and TLB broadcast bit when CONFIG_SMP is set and the processor is SMP, but not when CONFIG_SMP is disabled and the system is I/O coherent?

* User support issues. Having gazillions of bootloader updates every time a piece of configuration settings is rejected by the kernel maintainers is going to be a nightmare for users.

I certainly do understand the wish to put pressure on vendors to do their firmware correctly, and I'm continuously pushing Marvell to be more open, and I'm hoping to get them to do progress in the bootloader area in the future.

However, by forcing users to do gazillions of bootloader upgrades, you also make the mainline kernel very difficult to use for them. The result is that those users will prefer to use the vendor BSP kernels which can carry all the crazy fixes, and therefore the mainline kernel is completely untested, unused, and basically just a sort of repository of code that helps vendor reduce the overhead of bumping from one kernel version to the other for their own vendor BSP. Is this really what you want? Clearly not me.

Quoted 9 lines

Quoted 4 lines

All of these requirements are met when the kernel is configured with CONFIG_SMP *and* when the processor is actually a multiple core processors (otherwise, due to the CONFIG_SMP_ON_UP logic, is_smp() returns false, and most of the requirements above are not met).

I think there are some other problems here with is_smp(). For example, there are several cases where it is used for determining to read the MPIDR register. For things such as matching MPIDR to DT cpu nodes, we need that to work for !SMP kernels.

Could you point more specifically where this is happening?

In fact, another possible solution would be to make is_smp() returns "true" when the system is !SMP but has I/O coherency enabled. However, I have the issue that Armada 370 does *not* want shareable pages, while Armada XP/375/38x *require* that.

Quoted 8 lines

Quoted 8 lines

Basically, the patch series goes like this:

* PATCH 1 adds a 'flags' field to 'struct machine_desc' so that each platform can tell the ARM kernel core some of its requirements. We maybe could have used the Device Tree as well, but accessing the Device Tree as early as in paging_init() is a bit problematic as far as I understand.

Quoted 8 lines

Quoted 3 lines

Two flags are defined: MACHINE_NEEDS_CPOLICY_WRITEALLOC and MACHINE_NEEDS_SHAREABLE_PAGES. We need separate flags because Armada 370 cannot have the shareable attribute on page tables.

I think these are too specific. We used to have arch_is_coherent() which somewhat does what you need, but it was not multi-platform friendly. I think you need something like "is_io_coherent" and/or "is_mp".

I agree the flags are very specific, but unfortunately I haven't found a better way of describing things. The main problem here is that the different platforms have different requirements: Armada XP/375/38x require shareable pages, but Armada 370 does not want them. So a single "is_io_coherent" flag was not sufficient, because each platform has its own requirements. I'm really open to suggestions on this, but a single is_io_coherent flag doesn't work, unfortunately.

Quoted 6 lines

Quoted 5 lines

Quoted 1 line

- The pages must be set as shareable.

What is the impact of setting shareable bit for !SMP? I would think it would be a don't care for true UP systems. Does the 370 actually not work with shareable pages? AMP systems could have an issue though.

Quoted 4 lines

No, the Armada 370 doesn't work with shareable pages. See my cover letter that details the requirements for the different SoCs, and I've only mentioned shareable pages as part of the requirements for Armada XP, Armada 375 and Armada 38x.

I asked because you only mentioned platforms requiring shared pages.

Quoted 5 lines

No. It's done by the kernel for the SMP case, there's no reason it should be done in the bootloader for the !SMP I/O coherent case. Or, we remove the SMP and TLB broadcast bit fiddling in the kernel *completely* and make it part of the ARM boot protocol that the bootloader is required to do that.

No, it depends on the A9 configuration. The TLB broadcast is not modifiable in non-secure mode. You can control non-secure access to the SMP bit, but it can work with it enabled by secure world. This is what highbank does.

Quoted 7 lines

See above. It's not only about having issues to get the vendor to fix the firmware. It's also about:

* Having a consistent strategy with regard to what the kernel does and what the bootloader does. Why would the kernel set the SMP and TLB broadcast bit when CONFIG_SMP is set and the processor is SMP, but not when CONFIG_SMP is disabled and the system is I/O coherent?

It is not that simple. The SMP bit has different meanings depending on the core, but is more related to cache usage than running multi-core or not. On the A15 for example, IIRC, the SMP bit basically means disable cache lookups while the C bit only means disable cache allocations (i.e. cache hits can occur with the "cache disabled").

Quoted 3 lines

* User support issues. Having gazillions of bootloader updates every time a piece of configuration settings is rejected by the kernel maintainers is going to be a nightmare for users.

It's an artifact of trying to upstream the kernel after shipping rather than before. If these issues were fixed in the bootloader at the same time as someone decided to just hack up the vendor kernel, then probably none of us would even know about the issue.

As long as upstream is not a requirement, you are not in a position you can win.

You can never keep up with someone that has the freedom to just go and change whatever they want without having to deal with those annoying kernel maintainers. You fix these issues, there will just be other reasons why mainline is not usable (Do they still have the random one line deletion of a goto statement in the scheduler).

Speaking from experience rebasing i.MX vendor kernels, nothing upstream is easier to update than partially upstream if you try to build upon what is upstream. Generally speaking, vendors don't care what their delta against upstream is any more than they care about upstream being production quality.

Obviously, Marvell does care about mainline to some extent or I wouldn't be talking to you. But how do we get them to care enough to make mainline production quality? Part of the problem is we have distro's willing to take your money and vendor kernel (they are also willing to take your money and upstream kernel as well).

Quoted 10 lines

Quoted 9 lines

Quoted 4 lines

All of these requirements are met when the kernel is configured with CONFIG_SMP *and* when the processor is actually a multiple core processors (otherwise, due to the CONFIG_SMP_ON_UP logic, is_smp() returns false, and most of the requirements above are not met).

I think there are some other problems here with is_smp(). For example, there are several cases where it is used for determining to read the MPIDR register. For things such as matching MPIDR to DT cpu nodes, we need that to work for !SMP kernels.

Quoted 1 line

Could you point more specifically where this is happening?

Just grep for is_smp. IIRC, the DT cpu parsing code spews a warning when running a !SMP kernel on an MP system. It may only be when the MPIDR is not 0 like highbank which has 0x900.

Quoted 4 lines

In fact, another possible solution would be to make is_smp() returns "true" when the system is !SMP but has I/O coherency enabled. However, I have the issue that Armada 370 does *not* want shareable pages, while Armada XP/375/38x *require* that.

We'd need to look more closely at how is_smp is used to redefine it's meaning. We probably need to be able to describe "I have MP extensions", "I'm MP on SMP kernel" and "I'm UP running an SMP kernel".

Quoted 17 lines

Quoted 8 lines

Quoted 8 lines

Basically, the patch series goes like this:

* PATCH 1 adds a 'flags' field to 'struct machine_desc' so that each platform can tell the ARM kernel core some of its requirements. We maybe could have used the Device Tree as well, but accessing the Device Tree as early as in paging_init() is a bit problematic as far as I understand.

Quoted 8 lines

Quoted 3 lines

Two flags are defined: MACHINE_NEEDS_CPOLICY_WRITEALLOC and MACHINE_NEEDS_SHAREABLE_PAGES. We need separate flags because Armada 370 cannot have the shareable attribute on page tables.

I think these are too specific. We used to have arch_is_coherent() which somewhat does what you need, but it was not multi-platform friendly. I think you need something like "is_io_coherent" and/or "is_mp".

Quoted 7 lines

I agree the flags are very specific, but unfortunately I haven't found a better way of describing things. The main problem here is that the different platforms have different requirements: Armada XP/375/38x require shareable pages, but Armada 370 does not want them. So a single "is_io_coherent" flag was not sufficient, because each platform has its own requirements. I'm really open to suggestions on this, but a single is_io_coherent flag doesn't work, unfortunately.

With the write-allocate change, you only have a single flag so it is really just a question of name and how to control the flag.

Dear Rob Herring,

Quoted 8 lines · Rob Herring

Quoted 7 lines

See above. It's not only about having issues to get the vendor to fix the firmware. It's also about:

* Having a consistent strategy with regard to what the kernel does and what the bootloader does. Why would the kernel set the SMP and TLB broadcast bit when CONFIG_SMP is set and the processor is SMP, but not when CONFIG_SMP is disabled and the system is I/O coherent?

Quoted 5 lines

It is not that simple. The SMP bit has different meanings depending on the core, but is more related to cache usage than running multi-core or not. On the A15 for example, IIRC, the SMP bit basically means disable cache lookups while the C bit only means disable cache allocations (i.e. cache hits can occur with the "cache disabled").

Hum, right, and?

Quoted 8 lines

Quoted 3 lines

* User support issues. Having gazillions of bootloader updates every time a piece of configuration settings is rejected by the kernel maintainers is going to be a nightmare for users.

It's an artifact of trying to upstream the kernel after shipping rather than before. If these issues were fixed in the bootloader at the same time as someone decided to just hack up the vendor kernel, then probably none of us would even know about the issue.

Sorry, but that just doesn't work. The time it takes to mainline things is *way* longer than the time it takes for the SoC vendor to start providing early SoC releases and development boards with bootloaders to customers. There will also be some bootloader pushed out to customers before the code is mainlined in Linux.

Quoted 2 lines

As long as upstream is not a requirement, you are not in a position you can win.

Have you seen the number of patches we have sent over the last two years for these platforms? You must have missed them, or otherwise I don't understand how you manage to conclude that upstream is not a requirement.

Quoted 12 lines

You can never keep up with someone that has the freedom to just go and change whatever they want without having to deal with those annoying kernel maintainers. You fix these issues, there will just be other reasons why mainline is not usable (Do they still have the random one line deletion of a goto statement in the scheduler).

Speaking from experience rebasing i.MX vendor kernels, nothing upstream is easier to update than partially upstream if you try to build upon what is upstream. Generally speaking, vendors don't care what their delta against upstream is any more than they care about upstream being production quality.

Quoted 5 lines

Obviously, Marvell does care about mainline to some extent or I wouldn't be talking to you. But how do we get them to care enough to make mainline production quality? Part of the problem is we have distro's willing to take your money and vendor kernel (they are also willing to take your money and upstream kernel as well).

Could you be more specific about what Marvell isn't doing today to be mainline production ready? As I said, we've been working hard since two years to push to mainline the support for their SoCs. Our patches can certainly be criticized, but the end result is clear: their Armada 370, XP and now 375/38x platforms are quite well supported in mainline. And since the patches have been merged, they surely have matched the mainline kernel quality requirements, no?