Introduce ITS support for ARM64
ClosedPublic
Actions

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

Details

Reviewers

emaste
andrew
imp
• ian

Group Reviewers

manpages

Summary

Add ARM ITS (Interrupt Translation Services) support required to bring-up
message signaled interrupts on some ARM64 platforms.

Obtained from: Semihalf
Sponsored by: The FreeBSD Foundation

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

zbb updated this revision to Diff 5025.Apr 27 2015, 1:52 PM

zbb retitled this revision from to Introduce ITS support for ARM64.

zbb updated this object.

zbb edited the test plan for this revision. (Show Details)

zbb added reviewers: emaste, imp, • ian, andrew.

zbb set the repository for this revision to rS FreeBSD src repository - subversion.

zbb added a subscriber: Unknown Object (MLST).

Herald added a reviewer: manpages. · View Herald TranscriptApr 27 2015, 1:52 PM

Herald added subscribers: imp, andrew, emaste. · View Herald Transcript

See also the basic GICv3 support in D2377

kib added a subscriber: kib.Apr 27 2015, 2:00 PM

emaste added inline comments.May 12 2015, 9:53 PM

sys/arm64/arm64/gic_v3_its.c
210–216	Indeed, this is unfortunate. Can you at least KASSERT that it's NULL prior to this assignment and set it to NULL in gic_v3_its_detach to make a failure completely obvious. How do we expect this to be done later?

General remarks:

This code needs to be updated against the new version of GICv3 patch.
I will try to apply necessary style changes by the way basing on the guidelines from GICv3 review

Please put off the style review until the code update appears.

emaste added a subscriber: gnn.May 13 2015, 8:45 PM

@zbb, do you have an ETA for the rebase and new review?

zbb added a parent revision: D2701: Add options to dmb() and dsb() macros.Jun 1 2015, 12:15 AM

In D2378#49645, @emaste wrote:

@zbb, do you have an ETA for the rebase and new review?

I rebased this commit but I need to check for style issues before updating diff here.
(Nevertheless I have it in the pipeline)

sys/arm64/arm64/gic_v3_its.c
210–216	In the ideal situation the caller (intr_machdep.c) should use the proper 'dev' structure of ITS instead of GIC. So this should be fixed in the upper layer and when done this WO can be simply removed.

emaste added inline comments.Jun 2 2015, 6:55 PM

sys/arm64/arm64/gic_v3_its.c
210–216	OK

What do we do with it?
BTW. Does anyone know why diffs are not visible :-) ?

In D2378#56588, @zbb wrote:

What do we do with it?
BTW. Does anyone know why diffs are not visible :-) ?

There are issues w/ phabric right now. If you have missing diffs, please let eadler know about them.

just for followup phabric-admin fixed the diff issue.

In D2378#57098, @eadler wrote:

just for followup phabric-admin fixed the diff issue.

Great thanks!

To the reviewers: please take a look at the code. I would really like to commit this soon.

Best regards
zbb

emaste added inline comments.Jul 2 2015, 9:38 PM

sys/arm64/arm64/gic_v3.c
297 ↗	(On Diff #5978)	Is something like GIC_LAST_LPI sensible? Or any interrupt number >= 8192 is LPI?

I will try to look over the newly added file tonight, but I have no objection to committing the current version and addressing any newly reported issues post-commit.

In D2378#58418, @emaste wrote:

I will try to look over the newly added file tonight, but I have no objection to committing the current version and addressing any newly reported issues post-commit.

Thank you Ed. That is a good idea.

sys/arm64/arm64/gic_v3.c
297 ↗	(On Diff #5978)	Yes, any interrupt number >= 8192 is LPI but the upper limit is (as far as I can remember) implementation defined (the maximum for the architecture should be something like 2^32 - 1).

jmg removed a subscriber: jmg.Jul 3 2015, 5:17 PM

Can you upload a patch with more context? If you're not using arc https://wiki.freebsd.org/CodeReview explains how to do it with svn and git.

sys/arm64/arm64/gic_v3_fdt.c
94		This is an odd place for the prototype.
140 ↗	(On Diff #5978)	You should be checking the return value, e.g. if (gic_v3_ofw_bus_attach(dev) != 0) {
199–200 ↗	(On Diff #5978)	Is this because it hasn't been added to the code, or because of that the fdt bindings say?
280 ↗	(On Diff #5978)	These should be collected at the start of the file.
299 ↗	(On Diff #5978)	Why is this here? Normally we put the probe function early on in the file.
sys/arm64/arm64/gic_v3_its.c
163		Extra brace
687		Why do we need a full barrier here? The comment doesn't explain why.
841		Why do we need a cache wb or dsb? A comment explaining why would be useful.
671 ↗	(On Diff #5978)	Why do we need a full barrier, the comment should explain why.
818 ↗	(On Diff #5978)	Why do we need to flush the cache?
821 ↗	(On Diff #5978)	Why do we need a memory barrier?
1077–1080 ↗	(On Diff #5978)	Can you find out why? Marc Zyngier did the Linux driver and might be able to tell you why they have such a timeout.

andrew added inline comments.Jul 6 2015, 9:17 AM

sys/arm64/arm64/gic_v3_its.c
163 ↗	(On Diff #5978)	Ignore this, it was an old comment I missed

zbb marked 3 inline comments as done.Jul 15 2015, 1:58 PM

zbb added inline comments.

sys/arm64/arm64/gic_v3_fdt.c
199–200 ↗	(On Diff #5978)	FDT bindings. Removed comment in the commit.
280 ↗	(On Diff #5978)	There are two sets of probe/attach pairs here.
299 ↗	(On Diff #5978)	There are two sets of probe/attach pairs here. I think that this is not an unusual thing when we have attachments for two different drivers in the same file: /* Driver A / stuff_A; probe_A(); attach_A(); / Driver B */ stuff_B; probe_B(); attach_B(); They could be in a separate files but this is more convenient.
sys/arm64/arm64/gic_v3_its.c
818 ↗	(On Diff #5978)	CPU writes to its cache while ITS has its own. We need this to stay coherent.
821 ↗	(On Diff #5978)	Everyone knows what barriers are for ... Is the necessity of detailed explanation of barriers usage limited only to ITS driver or other ARM64 code also has it?
1077–1080 ↗	(On Diff #5978)	I will ask him.

imp added inline comments.Jul 15 2015, 4:09 PM

sys/arm64/arm64/gic_v3_fdt.c
299 ↗	(On Diff #5978)	There are very few drivers in the tree that have multiple attachments in the same file. It complicates subsetting and modules a bit is the general reason why that's the case. In this case it might be OK, it might not (I haven't looked at the larger context). What makes it more convenient to do it this way? Since I didn't see it in a quick glance, chances are good that a comment or two about why they are co-located would be in order.

zbb marked an inline comment as done.Jul 15 2015, 4:34 PM

zbb added inline comments.

sys/arm64/arm64/gic_v3_fdt.c
299 ↗	(On Diff #5978)	This is because ITS is in fact part of GICv3. It will never be attached to any other "parent device". I can add a comment to clarify that if you think that keeping those together is still reasonable.