Page MenuHomeFreeBSD

Introduce ARM GICv3 support
ClosedPublic

Authored by zbb on Apr 27 2015, 1:48 PM.

Details

Reviewers
emaste
andrew
imp
ian
Group Reviewers
manpages
Summary

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

Obtained from: Semihalf
Sponsored by: The FreeBSD Foundation

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
zbb added inline comments.May 4 2015, 4:47 PM
sys/arm64/arm64/gic_v3.c
301

Yes. This is taken from Linux.

sys/arm64/arm64/gic_v3_reg.h
109

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 added inline comments.May 5 2015, 9:19 PM
sys/arm64/arm64/gic_v3.c
510

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 added inline comments.May 5 2015, 9:26 PM
sys/arm64/arm64/gic_v3.c
510

OK. I see.

andrew added inline comments.May 6 2015, 9:33 AM
sys/arm64/arm64/gic_v3.c
510

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

sys/arm64/arm64/gic_v3_reg.h
109

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 added inline comments.May 6 2015, 1:52 PM
sys/arm64/arm64/gic_v3.c
510

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

zbb updated this revision to Diff 5255.May 7 2015, 11:59 AM
zbb added inline comments.
sys/arm64/arm64/gic_v3_reg.h
109

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

zbb added inline comments.May 7 2015, 12:05 PM
sys/arm64/arm64/locore.S
37

ups. I forgot to remove this.

zbb updated this revision to Diff 5256.May 7 2015, 12:06 PM
zbb added a comment.May 11 2015, 12:00 PM

Does anyone have any further comments?

andrew added inline comments.May 11 2015, 12:19 PM
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

This should be ICC_CTLR_EL1_EOIMODE

112

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

113

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 added inline comments.May 11 2015, 1:19 PM
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

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 added inline comments.May 11 2015, 1:28 PM
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

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 added inline comments.May 11 2015, 2:46 PM
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

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 added inline comments.May 11 2015, 2:50 PM
sys/arm64/include/armreg.h
113

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 added inline comments.May 11 2015, 2:53 PM
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 added inline comments.May 11 2015, 3:01 PM
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 added inline comments.May 11 2015, 6:52 PM
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....

zbb updated this revision to Diff 5327.May 11 2015, 6:53 PM
emaste added inline comments.May 11 2015, 7:04 PM
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 added inline comments.May 12 2015, 3:35 PM
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 added inline comments.May 12 2015, 3:53 PM
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 added inline comments.May 12 2015, 3:58 PM
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.

zbb marked 40 inline comments as done.May 12 2015, 4:03 PM
emaste added inline comments.May 12 2015, 4:10 PM
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.

zbb updated this revision to Diff 5343.May 12 2015, 4:35 PM
zbb marked an inline comment as done.
zbb marked 4 inline comments as done.
zbb marked 32 inline comments as done.May 12 2015, 4:38 PM
zbb marked an inline comment as done.May 12 2015, 4:39 PM
zbb marked an inline comment as done.May 12 2015, 4:39 PM
emaste edited edge metadata.May 12 2015, 9:50 PM

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

andrew accepted this revision.May 13 2015, 2:28 PM
andrew edited edge metadata.
andrew added inline comments.
sys/arm64/arm64/gic_v3_fdt.c
45

I suspect some of these aren't needed.

This revision is now accepted and ready to land.May 13 2015, 2:28 PM
imp edited edge metadata.May 13 2015, 2:55 PM

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?

imp accepted this revision.May 13 2015, 3:11 PM
imp edited edge metadata.
zbb added inline comments.May 13 2015, 3:19 PM
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 updated this revision to Diff 5364.May 13 2015, 3:35 PM
zbb edited edge metadata.

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

This revision now requires review to proceed.May 13 2015, 3:35 PM
emaste accepted this revision.May 13 2015, 3:44 PM
emaste edited edge metadata.
This revision is now accepted and ready to land.May 13 2015, 3:44 PM
andrew accepted this revision.May 13 2015, 3:51 PM
andrew edited edge metadata.
andrew added inline comments.
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 updated this revision to Diff 5366.May 13 2015, 4:31 PM
zbb edited edge metadata.
This revision now requires review to proceed.May 13 2015, 4:31 PM
emaste accepted this revision.May 13 2015, 4:33 PM
emaste edited edge metadata.
This revision is now accepted and ready to land.May 13 2015, 4:33 PM
andrew accepted this revision.May 13 2015, 4:42 PM
andrew edited edge metadata.
zbb closed this revision.May 13 2015, 7:12 PM

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.