LGTM

Thank you very much for your patience. Please don't forget the MFC for this whole series.

max77620 and as3722 are much more problematic. I think each function expects the other functions to have HW initialized before they can be consumed.

Strictly speaking, they are not entirely useless. A multi-function device can expose multiple different buses, so it can add another child when gpiobus_attach_bus() is called. In an ideal world, we should have an explicit bus_attach_children() function at the end of each respective driver (especially if it exposes multiple functions).
However, this is only a theoretical notice , nothing more.

One commit for moving gpiobus_attach_bus() after init (seems fine to me)
and another for changes to pl061 (the ACPI/WDT interrupt changes seems unrelated).
Removing bus_attach_children() is questionable ( and probably wrong)
and moving bus_setup_intr() before HW init is clearly wrong.

Thanks.

In D50868#1162636, @vexeduxr wrote:

Motivation? Why do we need gpio_pin_release to return errors?

It was to have it in line with gpio_pin_acquire. I found it a little weird that only one of them would report errors.

All *release* functions are usually very difficult to recover, especially if they are called throw multiple layers.

This creates the problem of freeing the gpio structure in case of an error.

At first I was freeing on both failure and success, but I wasn't sure that was the right thing to do..

This leading to a situation where the malloc cannot be released in any way. If gpiobus_release_pin() fails once, then it will always fail. But this is irrelevant if we replace errors with panic...

IMHO, I think all errors in gpiobus_release_pin() and gpiobus_acquire_pin() should be converted to panic().
Also passing NULL to gpio_pin_release() should be panic().

I agree for gpiobus_release_pin, since they're all programming errors. For gpiobus_acquire_pin, I think attempting to acquire a pin twice should just return an error to the caller, it's possible the caller can just recover.

Motivation? Why do we need gpio_pin_release to return errors?
This creates the problem of freeing the gpio structure in case of an error.
IMHO, I think all errors in gpiobus_release_pin() and gpiobus_acquire_pin() should be converted to panic().
Also passing NULL to gpio_pin_release() should be panic().

Thanks.
The most problematic variant is GPIO over I2C bus (or other serial bus), where sleepable(SX) locks are required for basic operation. So I just was to be very careful and warn others of the problematic place.

Why did you remove the comment? IMHO it is still a bit problematic (or not well defined) to hold the mutex across the GPIO infrastructure call.

Otherwise, it looks fine to me.

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.