I apologize, I forgot about that. I thought I had already completed all phabricator tasks.

These are needed for cross-tree allocation of interrupts (and probably also pins). For example, in FDT, a device from another tree path, typically an I2C or SPI, may have its IRQ pin connected to GPIO.
A driver from another tree path can therefore use an GPIO based interrupt. These allocations are processed by the controller itself (due to FDT references), but by gpiobus.
@andrew is right, at least (de)activate_resource is necessary, but I'm not sure yet whether other resource list oriented methods are also needed.

I just found a similar problem with rockchip gpio. Implementing interrupt methods is not enough, we also need all the methods for resources. This led me to the idea that we could create a new class in which all these methods (interrupts and resources) would be implemented (by using bus_generic_<foo>), and then subclass all bus-like drivers from it. It's clean, simple, and saves a lot of lines...
@jhb, do you have any comments on this?

In D51932#1191682, @vexeduxr wrote:

In D51932#1191073, @mmel wrote:

We can introduce a new method, such as

METHOD  get_pin_list{
 device_t dev;
 device_t bus;
 uint32_t *pin_list;
};

which fills a pre-allocated array with the controller pin numbers, with a default implementation based on GPIO_PIN_MAX() (assuming it returns the number of controller pins - 1) and sequential numbering starting with pin number 0.
This way, we can obtain a list of pins for all 'standard' controllers without any changes to their code, but we retain the option to modify it for 'more creative' ones.

Consider this only as a first quick attempt to solve this puzzle.

That sounds good :)
Although I don't see why we would need the gpiobus dev.

Right, it's not necessary. Please ignore it.

We can introduce a new method, such as

METHOD  get_pin_list{
 device_t dev;
 device_t bus;
 uint32_t *pin_list;
};

which fills a pre-allocated array with the controller pin numbers, with a default implementation based on GPIO_PIN_MAX() (assuming it returns the number of controller pins - 1) and sequential numbering starting with pin number 0.
This way, we can obtain a list of pins for all 'standard' controllers without any changes to their code, but we retain the option to modify it for 'more creative' ones.

That seems fine to me. Unfortunately, I just found that config32 has num_pins as an argument, so it may not work on all 32 bits :( I didn't notice that before, sorry.

In D51931#1189574, @vexeduxr wrote:

In D51931#1189566, @mmel wrote:

Gpiobus should convert these virtual pin numbers to real ones (gpio controller) using IVARS and can assume absolutely nothing about the ordering/sequencing of these translated numbers.

Yeah I missed the ordering part, like the other review. I guess that means we can't check if they're all in the ivars before passing the call to the controller. All we can do in that case is make sure that devi->npins >= 32?

IMHO no, we should check if they are all in ivars, sequentially, starting with first_pin. See later.

The controller (i.e. using gpio ifc) access_32/configure_32 expects to work on 32/npins consecutive pin numbers. This means that the corresponding gpiobus methods must only work on slaves with 32/npins or more consecutive pin mappings.

Sorry, but this is making less and less sense to me.

Theoretically, every interrupt controller should implement all 3 of these functions.

Oups, my bad. I didn't realize that these are exposed by the ioct().
Only, Could you please check if the entire pin range
is within bound?

I like this step. But please give me one more day to re-analyze it.

pin_config_32 doesn't make sense to me at the moment. Ian created it at a time when we could only configure the direction of pins and only for specific hardware (if I remember correctly, iMX51). Now there is no chance that it could be implemented for current hardware and pin flags. Configuration settings are almost never atomic for a single pin, let alone for multiple pins.

Thanks for patience, I know how much frustrating can be these change requests based on "subjective" nuances .

I apologize. I should not write reviews when I am short on time.
I wanted to say that I fully agree with jhb. The gpiobus_attach_bus() function is too limited to be generally useful.
That is why I vote to replace it with the gpiobus_add_bus() function and explicit bus_attach_children().

Sorry for the delayed response, I've very busy in real life again.

Unfortunately, changing the memory to uncacheable does not ensure cache coherence. Both parties(firmware and kernel) must have the memory mapped with the same attribute, otherwise undefined behavior will occur.
Please also note that the current implementation of the pmap_change_attr() does not allow its use on memory pages that are mapped multiple times (such DMAP and kernel).

In D51330#1172388, @jrtc27 wrote:

but why can't the PIC expose a single isrc that internally happens to send multiple interrupts as far as the hardware is concerned?

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...