Thanks

I'm sorry, I wanted to send it to D50309.

Very briefly, I think this clashes with devmap_add_entry(), which is used on armv7 to reduce the TLB load. The pmap_mapdev_attr() function does not respect the required attributes for pages previously mapped by devmap_add_entry(). Imho, we don't have a method for changing page attributes on multiple mapped pages in an architecturally correct way. (And I'm afraid this is true for arm64 , for pages mapped by DMAP and also in some other way).

I plan to MFC it, sure. I just forgot to copy all tag lines into the review.

I disagree, __packed resets the alignment to 1, so __attribute__((aligned(x))) can increase it. Imho, internally, __packed does nothing but reset the alignment of all members to 1 . Everything else (padding, size) is derived from that.
see: https://developer.arm.com/documentation/dui0472/m/Compiler-specific-Features/--attribute----packed---variable-attribute

They are all softfloat - all aeabi functions use softfloat calling convections and compiler-rt implementation doesn't use VFP. I removed the optimized VFP variants from the build (because they needed support in the RT linker). Imho these floating point functions are only used for softfloat binaries and also for Thumb-1 code.

I think that packed attribute also forces the object to be aligned to 1, so it begins conform with the C language rules. I think this is the only way to reduce the alignment of an object -> attribute__((packed, aligned(x)))

imho, using two temporary ulong variables for function calls and assigning them to mem_region members gives the compiler a chance to complain. But both versions are OK for me.
And my apology for not committed this fix in time...
Michal

In D49655#1132066, @andrew wrote:

How will multiple devices inside a single power domain be handled?

I really didn't want to force you to revert it. And it's just my problem that I didn't notice it at the right time. I just wanted to pass on this (perhaps obvious) information to someone else who is less sclerotic than me, that's all. Mostly because I've been super lazy lately, so no one can expect anything from me that just a few words.

LGTM. Just note that the newly added OF_device_unregister_xref() function does not work as expected without some sort of reference couting. A single device can (and do it in many cases) provide multiple functions (for example - pinmux, gpio, and interrupt controller), so is registered using respective frameworks 3x...

Unfortunately, the assumption about "In all the devices we currently support, the clock names in the "clock-output-names" property" is clearly incorrect. The documented binding for the A10/A20 does not allow any of these clocks. so this breaks every single A10/A20 board...

@mmel I also find it odd that you were the first to suggest an enum.

That's a misunderstanding. It seems I was too brief and probably inaccurate. I apologize for that.
I always meant to say that INTR_TYPE should not be a #define with bitfield-like values, but with enum-like values.
Given my aversion to having enum in interfaces, it didn't occur to me at all that someone might take that as a recommendation to use enum. My bad, I'll try not to repeat it next time.

+1 for deduplication

In D47279#1078241, @jrtc27 wrote:

In D47279#1078240, @mmel wrote:

In D47279#1078231, @jrtc27 wrote:

In D47279#1078226, @mmel wrote:

And we also don't compile the kernel with -fno-short-enums. So the actual behavior/variant depends only on the compiler (for which both variants are equally valid).
What happens when someone uses this enum as a member of KABI compliant structure?

They get the same as any other enum in a structure? There are plenty already out there, and unless you start to reach 128 enum values there is no risk of the type changing by adding one.

???? The enum (with less than 128 values) size changes with the default compiler settings.

Well, yours is sparse. In the case of intr_root_type it's not, it's required to be contiguous from 0, so it must hit 128 distinct root types (SCHAR_MAX + 1) to potentially change.

No, sizeof(enum) with < 128 values is 4 (with -fno-short-enums) or 1 (with -fshort-enums).

In D47279#1078231, @jrtc27 wrote:

In D47279#1078226, @mmel wrote:

And we also don't compile the kernel with -fno-short-enums. So the actual behavior/variant depends only on the compiler (for which both variants are equally valid).
What happens when someone uses this enum as a member of KABI compliant structure?

They get the same as any other enum in a structure? There are plenty already out there, and unless you start to reach 128 enum values there is no risk of the type changing by adding one.

And we also don't compile the kernel with -fno-short-enums. So the actual behavior/variant depends only on the compiler (for which both variants are equally valid).
What happens when someone uses this enum as a member of KABI compliant structure?

This code

ohh, sorry, taking back this part. I noticed u_register_t in the original commit and missed the subsequent change.
But my complaint about enum) being a low-level interface remains. if I'm not mistaken, the enum size is not fixed which is not a good feature for the interface.
You cannot assume sizeof(MyEnum) == sizeof(int) in general. see

imho the original commit is wrong and should be reverted. Enum is clearly not the right type of variable for a low-level interface, especially if it should be used in assembler. A direct consequence of this problem is that the patch uses two different types (enum and u_register_t) for the same variable.

imho, arm32 has slightly more correct code/condition, see|: https://cgit.freebsd.org/src/tree/sys/arm/arm/trap-v6.c#n403

While it probably does not directly apply to this case, we should be very careful with similar constructions.
Clang is slightly schizophrenic and declare minimal common list of extension if -march is passed to the compiler, but maximal list of extensions (including optional ones) if -mcpu is passed to the compiler.
See https://github.com/llvm/llvm-project/issues/90365.
To my eyes this is a bug, nothing else.

Adapted to kib's objections.

I've written this several times in other reviews, I think. Initialization of PICs on secondary cores must be coordinated with the parent PIC and must follow the PIC hierarchy. Imho, the only way to do this correctly is implement this in pic_init_secondary() for each PIC driver.

Hmm, shouldn't the -masm-syntax-unified be added to gcc CFLAGS? (just guessing, untested)

I think it's the wrong way. The initiation of child PICs is the job of the parent PIC and must be done in right order and in coordination with the parent PIC. Please see D43452 for more details.

However, these new IPIs still generate interrupts/exceptions - so some form of more or less abstract PIC is still needed. Of course, if these instructions have evolved into a fixed part of the CPU (that is, the IPI will be generated by the exact instruction and raise an exact exception), we can hardcode it and omit it from INTRNQ...

Thanks fixing my bugs..

Maximum PLL lock time is 20 microsecond. Alternatively, we may use the 'us' to avoid UTF-8 in source files.

I'm afraid that using virtual time outside the hypervisor environment is somewhat problematic, or creates a new (not always fulfilled) dependency on firmware. The virtual timer uses the value stored in CNTVOFF as an offset to generate the timer value. CNTVOFF is stored in a bank with an undefined reset value, so it *MUST* be initialized, otherwise we can end up with a situation where each core will have a different, non-zero value - so each core will have a different time. So the actual code expects someone (firmware, hypervisor) to initialize CNTVOFF on all cores - which is not always true (or better said, it is uncommon in the ARMv7 world and guaranteed in ARM64 ).

This broke (at least) armv7 cross-buildworld on amd64.

I certainly don't want to block your work. I just don't want to introduce new KPIs that are problematic ( at least for me) for further expansion , nothing else.

I see. So if I assume that the full implementation should have multiple roots (per type). My main objection is about using bitfield for interrupt type. Is misleading, not much compatible with multiple root implementation and it allows only single bit value almost everywhere.

The problem is that the FIQ tree may have a different topology than a normal interrupts. So instead of passing flags to the root handler, we should create a new root for the FIQ tree (or for any other type of interrupt), including a new intr_pic_claim_root_<foo>() (or we can convert intr_irq_root_dev and irq_root_* to arrays or so...).