Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Support ARM GICv3 mode #1465

Merged
merged 2 commits into from
Apr 24, 2017
Merged

Support ARM GICv3 mode #1465

merged 2 commits into from
Apr 24, 2017

Conversation

david-wang-2015
Copy link

@david-wang-2015 david-wang-2015 commented Apr 5, 2017 •
edited
Loading

These two patches enabled the ARM GICv3 support.

@david-wang-2015
Copy link
Author

Tested on Juno (aarch32 and aarch64).

@jenswi-linaro
Copy link
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@david-wang-2015 david-wang-2015 force-pushed the gicv3-handlers branch 2 times, most recently from 94e1ec4 to 25096c4 Compare April 7, 2017 10:26
@david-wang-2015
Copy link
Author

FIQ mode in aarch32 has banked register r8_fiq-r12_fiq. So thread_save_state should have a special version for fiq to save the r8-r12 in other mode.

@david-wang-2015 david-wang-2015 changed the title Refine the interrupt handlers Support ARM GICv3 mode Apr 7, 2017
@jenswi-linaro
Copy link
Contributor

Please don't change the subject/scope of a PR that is already reviewed and ready to merge. Doing so makes it harder and more time consuming to review it.

@david-wang-2015
Copy link
Author

Please don't change the subject/scope of a PR that is already reviewed and ready to merge. Doing so makes it harder and more time consuming to review it.

Oh...OK. Thanks for your reminder. :)

@jbech-linaro
Copy link
Contributor

@davwan01 , would you mind please add Jens RB tag and rebase this?

jenswi-linaro
jenswi-linaro reviewed Apr 12, 2017
View reviewed changes
core/include/drivers/gic.h Outdated

#if defined(CFG_ARM_GICV3)
#if defined(ARM64)
static inline uint32_t read_icc_ctlr_el1(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These inline functions belongs in core/arch/arm/include/arm64.h.
You can guard them with #if defined(CFG_ARM_GICV3) if you like.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

core/include/drivers/gic.h Outdated
__asm__ volatile ("msr S3_0_c12_c8_1, %0" : : "r" (v));
}
#elif defined(ARM32)
static inline uint32_t read_icc_ctlr_el1(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These inline functions belongs in core/arch/arm/include/arm32.h.
You can guard them with #if defined(CFG_ARM_GICV3) if you like.
The names of the functions should be read_<exact name of register> or write_<exact name of register> to minimize confusion regarding which register is accessed. Abstraction of hardware is usually done elsewhere.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got.
The register name should be icc_ctlr, so read_icc_ctrl should be good? Because just read_ctrl may cause confusion.

core/include/drivers/gic.h Outdated
{
uint32_t v;

__asm__ volatile ("mrs %0, S3_0_C12_C12_4" : "=r" (v));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/__asm__/asm/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

core/drivers/gic.c Outdated
@@ -189,16 +209,23 @@ void gic_init(struct gic_data *gd, vaddr_t gicc_base, vaddr_t gicd_base)
/* Set the priority mask to permit Non-secure interrupts, and to
* allow the Non-secure world to adjust the priority mask itself
*/
#if defined(CFG_ARM_GICV3)
write_icc_pmr_el1(0x80);
write_icc_ctlr_el1(GICC_CTLR_ENABLEGRP0 | GICC_CTLR_ENABLEGRP1
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please format as:

write_icc_ctlr_el1(GICC_CTLR_ENABLEGRP0 | GICC_CTLR_ENABLEGRP1 |
                   GICC_CTLR_FIQEN);

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

@jbech-linaro jbech-linaro mentioned this pull request Apr 12, 2017
Closed
@david-wang-2015
Copy link
Author

@davwan01 , would you mind please add Jens RB tag and rebase this?

@jbech-linaro, sure.

jenswi-linaro
jenswi-linaro reviewed Apr 13, 2017
View reviewed changes
core/arch/arm/include/arm64.h Outdated
@@ -304,6 +304,37 @@ DEFINE_U64_REG_WRITE_FUNC(mair_el1)

DEFINE_REG_READ_FUNC_(cntpct, uint64_t, cntpct_el0)

/* Register read/write functions for GICC registers by using system interface */
static inline uint32_t read_icc_ctlr(void)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the DEFINE_REG_READ_FUNC_() and DEFINE_REG_WRITE_FUNC_() macros above could be used to construct these functions.

Copy link
Author

@david-wang-2015 david-wang-2015 Apr 13, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked those macros, they use the register name as the parameter for mrs and msr. e.g. asm volatile("mrs %0, " esr_el1 : "=r" (val));
But it seems just support ARM core register name, not the GIC registers'.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't DEFINE_REG_READ_FUNC_(icc_ctlr, uint32_t, S3_0_C12_C12_4) generate read_icc_ctlr() correctly?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right. I just noticed DEFINE_U32_REG_XXX_FUNC.
Seems for aarch32, we don't have this kind of macros to define the read/write functions of system registers?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, we haven't done that yet.

core/drivers/gic.c Outdated
write_icc_pmr(0x80);
write_icc_ctlr(GICC_CTLR_ENABLEGRP0 | GICC_CTLR_ENABLEGRP1 |
GICC_CTLR_FIQEN);
write_icc_ctlr(GICD_CTLR_ENABLEGRP0 | GICD_CTLR_ENABLEGRP1);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this right? Shouldn't this target the distributor?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, that's a bug.

@david-wang-2015 david-wang-2015 force-pushed the gicv3-handlers branch 2 times, most recently from 7043b68 to a2de454 Compare April 14, 2017 05:14
@jenswi-linaro
Copy link
Contributor

Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

@jbech-linaro
Copy link
Contributor

Seems like this is ready to merge, right David @davwan01? If yes, would you mind please rebase the patch set?

@david-wang-2015
Copy link
Author

Seems like this is ready to merge, right David @davwan01? If yes, would you mind please rebase the patch set?

Sure. Seems it failed because of the CI issue. Will rebase.
BTW, @jbech-linaro , could you teach me how to retrigger the CI without rebase or close/open the PR?
Thanks.

David Wang added 2 commits April 24, 2017 13:10
Refine the interrupt handlers
3361bca 
The handlers of native and foreign interrupts are hardcoded in FIQ and
IRQ handlers.
This patch generalizes these handlers in macros.
For ARM GICv2 mode, FIQ handler calls native interrupt handler and IRQ
handler calls foreign interrupt handler.

Signed-off-by: David Wang <david.wang@arm.com>
Tested-by: David Wang <david.wang@arm.com> (juno arm32 and arm64)
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Support ARM GICv3 mode
1890132 
In ARM GICv3 mode, the interrupts are used as below for optee_os.
* FIQ - Foreign interrupts not handled by optee_os. This includes
the non-secure interrupts that should be handled by the REE and the
secure interrupts assigned to the monitor (aarch32 Monitor mode or
aarch64 EL3).
* IRQ - Native interrupts for optee_os.

And optee_os should use the system register interface to access the GICC
registers in GICv3 mode.

A new build flag `CFG_ARM_GICV3=y` should be set to support GICv3 mode.

Signed-off-by: David Wang <david.wang@arm.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
@jforissier
Copy link
Contributor

@davwan01 you can't re-trigger Travis unless you have admin rights on the project. You may however configure it to run on your own project. Simply go to https://www.travis-ci.org/ and log in with your GitHub account, then you can configure which projects and branches are checked.

@jforissier jforissier merged commit 1890132 into OP-TEE:master Apr 24, 2017
@jbech-linaro
Copy link
Contributor

Thanks for the contribution David!

@david-wang-2015
Copy link
Author

@jforissier , thanks.

@david-wang-2015
Copy link
Author

Hi @jbech-linaro ,
Just want to remind this patch is still in review - linaro-swg/linux#39.

@jforissier
Copy link
Contributor

@davwan01 merged -- thanks for the reminder.

@david-wang-2015 david-wang-2015 mentioned this pull request May 8, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jenswi-linaro jenswi-linaro jenswi-linaro left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@david-wang-2015 @jenswi-linaro @jbech-linaro @jforissier