Yes. This is taken from Linux.

This relates to ICC_SRE_EL2 register (if you didn't know).
I still don't get what is the problem here? Please write what do you mean exactly.

typically for style(9)

I don't see it mentioned in style(9), but I've always been told bitwise comparison of e.g. flags should be == 0 or != 0. This might just be tribal knowledge passed on via mailing lists / code review and should be added to style(9).

OK. I see.

I think it comes from Do not use ! for tests unless it is a boolean,....

It appears to be for the ICC_SRE register, however it's not in armreg.h, and there is no comment pointing this out to help find the details in the documentation.

Yes, indeed. I'll see about feeding all of these back into style(9).

ups. I forgot to remove this.

4 lines of magic numbers

Why the extra inderection? It's calling a list of 3 functions that don't change.

Why do we need i in this loop? And there could be a comment on what resources are being released, i.e. why sc->gic_redists.nregions + 1.

This magic value should come from armreg.h

if ((sre & ICC_SRE_EL1_SRE) != 0) {

Magic number, should be something like:

#define GICD_INTS_PER_foo 16

And here

And here

Magic number, should be something like:

#define GICR_TYPER_foo_SHIFT 32

if (err != 0)

Magic number

If there is no clean up you can just return above.

You should create a parent bus for these, then add them there.

err is not a bool, it should be if (err != 0), or change it to

if (err != 0) {
  /* The failure case below */
  if (bootverbose) {
    ...
  }
  gic_v3_fdt_detach(dev);
}

return (err);

You don't need this. The parent should set the detach functions to gic_v3_detach, then don't set one in the child. The child should only define a function if it needs to do something extra.

Should be #define<tab>_GIC...

No need for ({ and })

What is the GIC field? It's generally better to explain what is happening when writing asm so you still understand what is happening in 6 months time.

This should be ICC_CTLR_EL1_EOIMODE

I don't see this in the ARMv8 ARM, is it documented differently in the GICv3 docs?

In general I've tried to add all the used bits when adding new register values, and the registers should be alphabetically sorted by name.

I don't get it. What do you mean "parent bus for these"? When you did your first reviews on GitHub you asked for separate FDT-specific attachment for GICv3. Now we have some need for separate bus for GICv3? Can you point me some other example of such bus among other PIC controllers in FreeBSD?

I don't understand what is your intention here. Can you point me to the example in the other PIC driver?

To avoid another ping-pong:
1023 is a spurious interrupt ID that is described in GICv3 docs and is being read from ICC_IAR1_EL1 register.

Do you ask because:

1. you want it moved/redefined somewhere else?
2. for other reason (explain)

I meant parent class. The common parts of this should be in a parent class, with the bus specific code in either the fdt, or acpi when we add this. I've done this with the gic(v2) driver in D2463.

Ok, this is one of the problems with me not having GICv3 docs,I'm unsure what is in there that may be useful.

and why does everybody write function calls through pointers in that style? It's not required by style(9). IMO, the write way to spell that call would be

err = init_func(sc);

Eeek! No way -- the registers should be sorted ascending by address, not by name, so that they match the way they're documented. (I've yet to see a soc manual arrange register docs alphabetical as opposed to by address.)

These are special registers, they are listed by name in the docs. There is sort of an address (as much of one as the 32-bit coprocessor registers), but only the assembler needs to know about it.

i is used as a kosher way to iterate in loop - using size_t type, whereas rid has to be int.

Although this won't work if sc->gic_redists.nregions > INT_MAX anyway - we could probably just document that fact with a KASSERT.

Hmm? I don't think you can call init_func(sc); here. It would be an error.
Extra indirection seemed more convenient here since:

1. we will have at least one more function here
2. we have secondary cores initialization sequence (that andrew requested to be removed from this commit) and it is more convenient to see what are the differences between those thwo
3. the overhead of writing this loop is the same as writing 3-4 times:

err = func1();
if (err != 0)
      goto error;
err = func2();
if (err != 0)
      goto error;
....

OK. I will remove i from this loop. Regarding gic_refists.nregions + 1 then AFAIK we have proper comment when allocating resource:

	/*
	 * Allocate array of struct resource.
	 * One entry for Distributor and all remaining for Re-Distributor.
	 */

So now we have to comment on allocation and deallocation?

This is the explanation from the documentation ("GIC bits").
If you look at the following line you see more detailed description what field setting means what

/* 0001 - SR CPU IF supported */

I will make that line even more detailed in the next iteration....

So now we have to comment on allocation and deallocation?

Actually, you could just put a single kassert above at line 140 since we'll already break if rid > INT_MAX on
sc->gic_res[rid] = bus_alloc_resource_any(dev, SYS_RES_MEMORY,

This comment is now completely detached from the code it was commenting on.

My point was that these two lines are equivelent:

(*init_func)();
init_func();

When you've got a pointer to a function, the compiler knows that, there's no need to add parens and a star to make it call through the pointer.

You can comment to the previous diff and the comment will be in the right place.
But back to the topic, I've made a quick test:

error: called object type 'gic_v3_initseq_t *' (aka 'int (**)(struct gic_v3_softc *)') is not a function or function pointer
               err = init_func(sc);
                     ~~~~~~~~~^

Oh! I completely missed that the pointer was double-indirect through the typedef. My mistake, sorry about that.

Magic numbers are still here. In the interest of avoiding further round-trips I'm happy for you to either fix prior to commit or update in a subsequent commit.

You should at least printf or panic or something :)

Does delay 1 really delay 1us every time over 1M iterations? I guess it isn't critical, but if you only want to wait a second for something time sources are more reliable. Then again, why wait a full second here?

Same comments here. why 1 second?

DELAY is because we if fact expect to get immediate response from GIC. If we don't get any this most probably means malfunction. I would suggest to wait that second and panic instead of device_printf(). I don't have any good explanation why 1 second and not for example 0.5. There are no timing details in the GIC documentation related to that and the one below so this value was based on what Linux code does.