Introduce ARM GICv3 support

[zbb]

Summary

Support for GICv3 interrupt controller used by some ARM64 based chips.

Obtained from: Semihalf
Sponsored by: The FreeBSD Foundation

[zbb]

sys/arm64/arm64/gic_v3.c

300 ↗

(On Diff #5024)

Yes. This is taken from Linux.

sys/arm64/arm64/gic_v3_reg.h

108 ↗

(On Diff #5024)

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.

[emaste]

sys/arm64/arm64/gic_v3.c

509 ↗

(On Diff #5024)

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

[zbb]

sys/arm64/arm64/gic_v3.c

509

OK. I see.

[andrew]

sys/arm64/arm64/gic_v3.c

509 ↗

(On Diff #5024)

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

sys/arm64/arm64/gic_v3_reg.h

108 ↗

(On Diff #5024)

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.

[emaste]

sys/arm64/arm64/gic_v3.c

509

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

[zbb]

sys/arm64/arm64/gic_v3_reg.h

108 ↗

(On Diff #5024)

OK. So in brief you just want this to be moved to armreg.h.
Seems legitimate.

[zbb]

sys/arm64/arm64/locore.S

37

ups. I forgot to remove this.

[zbb]

Does anyone have any further comments?

[andrew]

sys/arm64/arm64/gic_v3.c

152

4 lines of magic numbers

164

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

192

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.

320

This magic value should come from armreg.h

342

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

392

Magic number, should be something like:

#define GICD_INTS_PER_foo 16

396

And here

405

And here

474

Magic number, should be something like:

#define GICR_TYPER_foo_SHIFT 32

541

if (err != 0)

556

Magic number

564

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

sys/arm64/arm64/gic_v3_fdt.c

82

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

136

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);

156

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.

sys/arm64/arm64/gic_v3_reg.h

32

Should be #define<tab>_GIC...

sys/arm64/arm64/gic_v3_var.h

80

No need for ({ and })

sys/arm64/arm64/locore.S

192

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.

sys/arm64/include/armreg.h

106 ↗

(On Diff #5256)

This should be ICC_CTLR_EL1_EOIMODE

112 ↗

(On Diff #5256)

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

113 ↗

(On Diff #5256)

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.

[zbb]

sys/arm64/arm64/gic_v3_fdt.c

82

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?

156

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

sys/arm64/include/armreg.h

112 ↗

(On Diff #5256)

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)

[andrew]

sys/arm64/arm64/gic_v3_fdt.c

82

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.

sys/arm64/include/armreg.h

112 ↗

(On Diff #5256)

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

[• ian]

sys/arm64/arm64/gic_v3.c

164

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);

sys/arm64/include/armreg.h

113 ↗

(On Diff #5256)

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

[andrew]

sys/arm64/include/armreg.h

113 ↗

(On Diff #5256)

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.

[zbb]

sys/arm64/arm64/gic_v3.c

192

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

[emaste]

sys/arm64/arm64/gic_v3.c

192

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

[zbb]

sys/arm64/arm64/gic_v3.c

164

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

192

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?

sys/arm64/arm64/locore.S

192

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

[emaste]

sys/arm64/arm64/gic_v3.c

192

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,

[• ian]

sys/arm64/arm64/gic_v3.c

164

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.

[zbb]

sys/arm64/arm64/gic_v3.c

164

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);
                     ~~~~~~~~~^

[• ian]

sys/arm64/arm64/gic_v3.c

164

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

[emaste]

sys/arm64/arm64/gic_v3.c

168–171

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.

[emaste]

I'm happy for this to be committed and we can address any further nits in subsequent commits.

[andrew]

sys/arm64/arm64/gic_v3_fdt.c

45

I suspect some of these aren't needed.

[imp]

Two minor comments. One is a suggestion we might want to say something. The second is the 1s timeout seems too long and is badly implemented (DELAY(1) isn't the highest precision thing). I'm wondering why you'd ever need more than several milliseconds.

Other than that, it looks OK. A couple of places could use some minor cleanup, but they are much better after the initial rounds of cleanup.

sys/arm64/arm64/gic_v3.c

206

You should at least printf or panic or something :)

322

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?

536

Same comments here. why 1 second?

[zbb]

sys/arm64/arm64/gic_v3.c

322

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.

[zbb]

Some more fixes from previous comments + fixes after final testing on HW.

[andrew]

sys/arm64/arm64/gic_v3_fdt.c

84

These don't need to be == 0 as they are boolean (even if they return an int)

117

bootverbose is a boolean

[zbb]

Thank you all very much.
Committed to: r282867
URL: https://svnweb.freebsd.org/changeset/base/282867

Any further comments will be applied on top of that.