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.

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

Add i.MX PSCI CPU off and update i.MX6UL #1577

Merged
merged 7 commits into from
Jun 26, 2017
Merged

Conversation

MrVan
Copy link
Contributor

@MrVan MrVan commented Jun 2, 2017

This patch reset contains the two patches in #1570 which has not been merged. So post all the patches together to avoid function break and requesting comments. After comments addressed and #1570 merged, I'll drop the two patches and resend the pull request.

In this patchset, imx6ul-regs.h imx6-regs.h are replaced by one imx-regs.h. The 6UL TEE mapping is refined to 0x9E000000 and 32M used. The original code use 0x9C000000, and 48M used. Add psci helper function used for psci cpu off and add the support for i.MX6Q-SDB. Testing results shows works well with Linux in non-secure world.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 2, 2017

Updated with dropping the two patches merged by #1570

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Very nice change.
Looking a atf psci library integration in optee, i faced the same required sequence: here (edited: in my case,
cache is already off when entering arch_cpu_power_down).

#define IOMUXC_GPR10_OFFSET 0x28

#define IOMUXC_GPR10_OCRAM_TZ_ADDR_OFFSET 5
#define IOMUXC_GPR10_OCRAM_TZ_ADDR_MASK (0x3f << 5)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: prefer GENMASK_32(10, 5)

and same below, prefer use of BIT() and GENMASK_32().

FUNC psci_enable_smp, :
UNWIND( .fnstart)
read_actlr r0
orr r0, r0, #(1 << 6)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: can use ACTLR_SMP from arm32.h


bl psci_disable_smp

pop {lr}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor; suggest pop {pc}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. I use pop {lr}; bx {lr} to switch between ARM/THUMB. pop {pc} may cause issue if now is in ARM mode.

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it can cause an issue on pre-ARMv7 architectures, but in ARMv7 I haven't seen an issue related to this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I'll give a test on this. If ok, I'll switch to use pop {pc}


/* Disable Cache */
read_sctlr r0
bic r0, r0, #(1 << 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

suggest SCTLR_C from arm32.h

*
* flush dcache all to PoC
*/
FUNC psci_armv7_flush_dcache_all , :
Copy link
Contributor

Choose a reason for hiding this comment

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

may i claim a 2nd look at this?
against core native cache support.

This needs to flush the inner cache only. It so could be renamed cpu_armv7_flush_inner_dcache_all() ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this is ok for me. I would use psci_armv7_flush_inner_dcache_all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this function rather belong in core/arch/arm/kernel/ssvce_a32.S?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agree. I just checked core/arch/arm/kernel/ssvce_a32.S, seems arm_cl1_d_cleanbysetway is also for this, but arm_cl1_d_cleanbysetway is a simplied version. I may need to update ssvce_a32.S.


bl psci_armv7_flush_dcache_all

clrex
Copy link
Contributor

@etienne-lms etienne-lms Jun 2, 2017

Choose a reason for hiding this comment

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

clrex really needed ? ___ edited, actually, this was a question :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I have no good idea on this. From my experience of PSCI U-Boot interface, I think this is needed. I am not sure this is correct or not: This core may execute LDREX/STREX before, so when offline, need to execute CLREX to clear-exclusive.


imx_set_src_gpr(core_id, ~0);

asm volatile (
Copy link
Contributor

Choose a reason for hiding this comment

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

Provided that we had a wfi() function (not that hard to add) this could could be written as:

thread_mask_exceptions(THREAD_EXCP_ALL);
while (true)
         wfi();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fix in next version.

vaddr_t gpr5 = core_mmu_get_va(IOMUXC_BASE, MEM_AREA_IO_SEC) +
IOMUXC_GPR5_OFFSET;
uint32_t cpu, val;
uint32_t wfi;
Copy link
Contributor

Choose a reason for hiding this comment

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

Type should be bool


cpu = affinity;

wfi = !!(read32(gpr5) & ARM_WFI_STAT_MASK(cpu));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to normalize a bool as that's done by the compiler as required by the standard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix in next version


wfi = !!(read32(gpr5) & ARM_WFI_STAT_MASK(cpu));

if ((imx_get_src_gpr(cpu) == 0) || !wfi)
Copy link
Contributor

Choose a reason for hiding this comment

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

!imx_get_src_gpr(cpu) is the preferred style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is not to check false or true, I just check whether its value is 0 or non-zero, I prefer use == here.


bl psci_disable_smp

pop {lr}
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps it can cause an issue on pre-ARMv7 architectures, but in ARMv7 I haven't seen an issue related to this.

@jenswi-linaro
Copy link
Contributor

For information, I've started to (re-)import the cache maintenance routines from ARM-TF (lib/aarch32/cache_helpers.S and lib/aarch64/cache_helpers.S). I'll create a pull request in due time.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 7, 2017

@jenswi-linaro that's great. Then I'll directly use your new code in my psci code.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 8, 2017

@jenswi-linaro one more question, about the code importing from ARM-TF, will code be merged before 2.5.0? I would like to upstream more i.mx psci code before 2.5.0. If you do not plan to merge you patch before 2.5.0, I may still need to update ssvce_a32.S.

@jenswi-linaro
Copy link
Contributor

Please go ahead with this PR, waiting for my updates will only delay this.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 10, 2017

@etienne-lms @jenswi-linaro Updated. Use cpu_armv7_flush_inner_dcache_all and move to ssvce_a32.S. use macros but not number bit shift. wfi function is not added, still use asm code to do dead loop wfi. I added a new more patches in this patchset, switch to use REE time for 6UL, add platform early init code for A7 core, add 6ULL EVK.

During my test, I met failure: optee_test/host/xtest/regression_4000.c:2380: Expression "out_size == ciph_cases[n].in_incr" (0 == 13) is false regression_4003_NO_XTS.5 FAILED, using latest client/os/test.

@jforissier
Copy link
Contributor

During my test, I met failure: optee_test/host/xtest/regression_4000.c:2380: Expression "out_size == ciph_cases[n].in_incr" (0 == 13) is false regression_4003_NO_XTS.5 FAILED, using latest client/os/test.

This is likely a problem with the build, something not cleaned properly. Please make sure there is no libutee.a left from a previous build, and also make sure the crypt TA is properly rebuilt (re-linked against the new libutee.a). I sometimes do rm -rf optee_os/out optee_test/out when in doubt...

Copy link
Contributor

@etienne-lms etienne-lms left a comment

Choose a reason for hiding this comment

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

Acked-by: Etienne Carriere <[email protected]>

  • commit1 core: arm: mx6ulevk: refine the tee address map:
  • commit3 arm: ssvce_a32: introduce cpu_armv7_flush_inner_dcache_all
  • commit5 (minor comments) core: arm: imx: use one imx-regs.h file
  • commit7 core: imx6ul: switch to use CFG_SECURE_TIME_SOURCE_REE
  • commit8 (minor comments) core: arm: imx6ul: add platform early init code
  • commit9 core: arm: imx: add 6ULL EVK support

Reviewed-by: Etienne Carriere <[email protected]>

  • commit2: core: arm32_macros: add macros

Still open (to me)

  • commit4 core: arm: psci: add helper functions
    some comments. I am checking some dmb/isb. But looks ok.
  • commit6 core: arm: imx support psci off and affinity
    Some comments + minor comment on commit comment: s/Introuduce/Introduce/, s/is use/is used/

* ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE
* POSSIBILITY OF SUCH DAMAGE.
*/

Copy link
Contributor

Choose a reason for hiding this comment

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

#ifndef PLAT_IMX_IMX_H
#define PLAT_IMX_IMX_H
...
#endif

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fix in next version

#define DRAM0_BASE 0x80000000
#define DRAM0_SIZE 0x20000000
movw r0, #0x0040
movt r0, #0x0000
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: suggested mov_imm r0, 0x00000040
and below mov_imm r0, 0x00040C00
Macros for these magics would be welcome.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah. Thanks.

@@ -58,3 +58,5 @@ int psci_node_hw_state(uint32_t cpu_id, uint32_t power_level);
int psci_stat_residency(uint32_t cpu_id, uint32_t power_state);
int psci_stat_count(uint32_t cpu_id, uint32_t power_state);
void tee_psci_handler(struct thread_smc_args *args);

void psci_armv7_cpu_off(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

Declare also psci_enable/disable_smp(); as they are defined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They are now only used by psci_armv7_cpu_off. We could export them when we need in future, i think:)

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -90,14 +90,14 @@
#endif


#define CONSOLE_UART_BASE (UART0_BASE)
#define CONSOLE_UART_BASE (UART1_BASE)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parenthesis


#define IOMUXC_GPR4_OFFSET 0x10
#define IOMUXC_GPR5_OFFSET 0x14
#define ARM_WFI_STAT_MASK(n) (1 << (n))
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: BIT(n)

"wfi \r\n"
"b loop \r\n");

return PSCI_RET_SUCCESS;
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: return value is not reliable.
This should return an error or eventually assert you don't return.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah. needs to return an error value. Thanks.

/* Kill cpu */
val = read32(va + SRC_SCR);
val &= ~BIT32(SRC_SCR_CORE1_ENABLE_OFFSET + (cpu - 1));
val |= BIT32(SRC_SCR_CORE1_RST_OFFSET + (cpu - 1));
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless parenthesis (cpu - 1).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fix in next version.

@MrVan MrVan force-pushed the psci-imx branch 2 times, most recently from 03111e0 to 63916d2 Compare June 18, 2017 10:08
@MrVan
Copy link
Contributor Author

MrVan commented Jun 18, 2017

@etienne-lms Updated with your comments addressed.

@etienne-lms
Copy link
Contributor

commit2: "core: arm32_macros: add macros"

commit3: "arm: ssvce_a32: introduce cpu_armv7_flush_inner_dcache_all "

  • See minor comment about inline comments.
  • This change looks ok but it flushes data to PoC whereas I think one could flush only the L1 cache when used in the psci sequence. Also, on cortex-A5/A9 with a pl310 (or some other external level2 cache), this routine will flush only from the L1 cache, hence not up to the PoC as stated in the comment.
    So I think it can be merge as is, and be reviewed once we will merge the 'cache helpers' from Jens (Update cache helpers #1606).

commit4: "core: arm: psci: add helper functions"

  • please use macro SCTLR_C as suggested in comment.
  • once fixed: Reviewed-by: Etienne Carriere <[email protected]>

All others commits Acked-by: Etienne Carriere <[email protected]> with some few minor comments that can be addressed... or not.


thread_mask_exceptions(THREAD_EXCP_ALL);

asm volatile(
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this preferable over:

while (true)
        wfi();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenswi-linaro if use "while (true)", compiler will complain errors, because this function expects a value returned.

Copy link
Contributor

Choose a reason for hiding this comment

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

This patch to enable usage of the suggested loop is in my opinion the lesser of two evils.

diff --git a/core/arch/arm/plat-imx/sub.mk b/core/arch/arm/plat-imx/sub.mk
index 41b4bbcd8b46..e42213caad9f 100644
--- a/core/arch/arm/plat-imx/sub.mk
+++ b/core/arch/arm/plat-imx/sub.mk
@@ -3,6 +3,7 @@ srcs-y += main.c imx-common.c
 
 srcs-$(CFG_PL310) += imx_pl310.c
 srcs-$(CFG_PSCI_ARM32) += psci.c
+cflags-psci.c-y += -Wno-suggest-attribute=noreturn
 
 ifneq (,$(filter y, $(CFG_MX6Q) $(CFG_MX6D) $(CFG_MX6DL)))
 srcs-y += a9_plat_init.S imx6.c

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jenswi-linaro Thanks. fix in next version.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 19, 2017

@etienne-lms Thanks. I could follow clearly,"So I think it can be merge as is, and be reviewed once we will merge the 'cache helpers' from Jens (#1606)." you mean this patch needs to be postponed until #1606 merged?

@etienne-lms
Copy link
Contributor

(...) you mean this patch needs to be postponed until #1606 merged?

@MrVan, no, rather let's merge your change as you propose them and we will update (if required) when merging #1606. As @jens pointed: "Please go ahead with this PR, waiting for my updates will only delay this."

Note: few of my review comments are still pending, i'll push them...

@@ -125,6 +125,84 @@ UNWIND( .fnend)
END_FUNC secure_mmu_unifiedtlbinv_byasid

/*
* From ARM DDI 0406C.c Page B2-1286
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor generic comment: the code snippet you reference provide few inline comments. Maybe you should preserve them to help understanding the sequence (although the reference you gave is already a good explanation).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. I'll add them.

@@ -58,3 +58,5 @@ int psci_node_hw_state(uint32_t cpu_id, uint32_t power_level);
int psci_stat_residency(uint32_t cpu_id, uint32_t power_state);
int psci_stat_count(uint32_t cpu_id, uint32_t power_state);
void tee_psci_handler(struct thread_smc_args *args);

void psci_armv7_cpu_off(void);
Copy link
Contributor

Choose a reason for hiding this comment

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

ok

FUNC plat_cpu_reset_early , :
UNWIND( .fnstart)

mov_imm r0, 0x00000040
Copy link
Contributor

Choose a reason for hiding this comment

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

Cross-check whether you would not prefer an init value of 0x41 : bit FW of ACTRL enables broadcasting of maintenance operations required for SMP.

minor: would look better using a macro (i.e ACTLR_INIT) from platform_config.h.
Same below with NSACR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@etienne-lms FW of ACTLR is in A9, not in A7. Let me keep it now, Later I could use a follow up patch to use marco.

@@ -76,4 +76,3 @@ void plat_cpu_reset_late(void)
write32(read32(addr) | CSU_SETTING_LOCK, addr);
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: useless change (are better to avoid)

Copy link
Contributor

Choose a reason for hiding this comment

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

+1


core_id = get_core_pos();

write32(~0, va + SRC_GPR1 + core_id * 8 + 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

this looks redundant with the imx_set_src_gpr(core_id, ~0); implemented below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. this needs to be removed.

* Wait secondary cpus ready to be killed
* TODO: Change to non dead loop
*/
while (read32(va + SRC_GPR1 + cpu * 8 + 4) != (uint32_t)~0)
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: suggested use of imx_get_src_gpr(cpu).
Can use UINT32_MAX instead of (uint32_t)~0.

write32(val, va + SRC_SCR);

/* Clean arg */
write32(0, va + SRC_GPR1 + cpu * 8 + 4);
Copy link
Contributor

Choose a reason for hiding this comment

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

redundant with the imx_set_src_gpr(cpu, 0); below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Remove.

@MrVan
Copy link
Contributor Author

MrVan commented Jun 20, 2017

@etienne-lms @jenswi-linaro Updated

  1. using "while (true) wfi()"
  2. add more comments in the cache flush function according ARM RM
  3. a few minor comments not addressed.
  4. tags applied. only patch "arm: ssvce_a32: introduce cpu_armv7_flush_inner_dcache_all" no review or ack tag.


core_id = get_core_pos();

DMSG("core_id: %d\n", core_id);
Copy link
Contributor

Choose a reason for hiding this comment

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

DMSG("core_id: %" PRIu32 , core_id);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix this globally.

{
vaddr_t va = core_mmu_get_va(SRC_BASE, MEM_AREA_IO_SEC);
vaddr_t gpr5 = core_mmu_get_va(IOMUXC_BASE, MEM_AREA_IO_SEC) +
IOMUXC_GPR5_OFFSET;
Copy link
Contributor

Choose a reason for hiding this comment

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

Align this with core_mmu_get_va( at the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

linux checkpatch does not report warning on this. To open (, need to align, but here ( is closed with ), no need to align?

Copy link
Contributor

Choose a reason for hiding this comment

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

Checkpatch is just a tool helping in some situations. You can probably enter the obfuscated C contest with something that checkpatch groks.
Please align it with the start of "core_mmu_get_va" on the line above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok. Fix in next version.

if ((imx_get_src_gpr(cpu) == 0) || !wfi)
return PSCI_AFFINITY_LEVEL_ON;

DMSG("cpu: %d GPR: %x\n", cpu, imx_get_src_gpr(cpu));
Copy link
Contributor

Choose a reason for hiding this comment

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

DMSG("cpu: %" PRIu32 "GPR: %" PRIx32, cpu, imx_get_src_gpr(cpu));


bl cpu_armv7_flush_inner_dcache_all

dsb
Copy link
Contributor

Choose a reason for hiding this comment

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

cpu_armv7_flush_inner_dcache_all() just finished with:

dsb st
isb

Isn't that enough? If an additional dsb is needed please add a comment explaining why.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The isb could be removed. dsb maybe needed, I may need to move it after disable cache and before the second cpu_armv7_flush_inner_dcache_all. dsb is to make all the load/store in order, the last inst of cpu_armv7_flush_inner_dcache_all read from memory and the first inst store to memory.

@MrVan MrVan force-pushed the psci-imx branch 2 times, most recently from 0fe8ffa to 5e31729 Compare June 22, 2017 03:18
@MrVan
Copy link
Contributor Author

MrVan commented Jun 22, 2017

Updated:
Drop the two patch. @jenswi-linaro introduced related code from ATF.
"core: arm32_macros: add macros"
"arm: ssvce_a32: introduce cpu_armv7_flush_inner_dcache_all "
Addressed comments form @jenswi-linaro

  1. Use "DMSG("core_id: %" PRIu32"
  2. Align code to (

mov r0, #DCACHE_OP_CLEAN_INV
b dcache_op_all

isb
Copy link
Contributor

Choose a reason for hiding this comment

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

This isb is probably redundant as dcache_op_all() (or do_dcsw_op()) does an isb just before returning.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will remove it.

push {lr}

mov r0, #DCACHE_OP_CLEAN_INV
b dcache_op_all
Copy link
Contributor

Choose a reason for hiding this comment

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

bl dcache_op_all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh. my bad. pushed wrong patch.

dsb

mov r0, #DCACHE_OP_CLEAN_INV
b dcache_op_all
Copy link
Contributor

Choose a reason for hiding this comment

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

bl dcache_op_all


FUNC psci_armv7_cpu_off, :
UNWIND( .fnstart)
push {lr}
Copy link
Contributor

Choose a reason for hiding this comment

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

When calling other functions it's more safe to keep the stack pointer 8-byte aligned as some function may expect that, it's a requirement in the calling convention.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not meet such requirement on i.MX platforms. From "http://infocenter.arm.com/help/topic/com.arm.doc.ihi0042f/IHI0042F_aapcs.pdf" 5.2.1.1 Universal stack constraints: SP mod 4 = 0. The stack must at all times be aligned to a word boundary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Then there's 5.2.1.2 Stack constraints at a public interface:

The stack must also conform to the following constraint at a public interface:

  • SP mod 8 = 0. The stack must be double-word aligned.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Then use "sub sp, sp, #4" after push lr and "add sp, sp, #4" before pop pc. Do you agree with this?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's less code to just push and pop a dummy register like:

push {r12, lr}

pop {r12, pc}

@MrVan
Copy link
Contributor Author

MrVan commented Jun 23, 2017

@jenswi-linaro Updated with your comments addressed. Aligned stack to 8 bytes, using "push {r12, lr}" and "pop {r12, pc}", correct "b dcache_op_all" to "bl dcache_op_all", remove the redundant "isb" after dcache_op_all

@MrVan
Copy link
Contributor Author

MrVan commented Jun 26, 2017

Is this patchset ok to be merged or any more comments on this patchset? I still have local patches queued to upstream that depends on this patchset.

@jenswi-linaro
Copy link
Contributor

Looks good to me:
Acked-by: Jens Wiklander <[email protected]>

Add helper function psci_armv7_cpu_off.
This function will be used when use psci to offline a cpu.

Signed-off-by: Peng Fan <[email protected]>
Reviewed-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
There is 512M DDR Memory on i.MX6UL-EVK board.
Reserve high 32M for TEE usage. The highest 2M for SHMEM.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Clean up to use one imx-regs.h for i.MX SoC family.
If there are different IP address, use CFG_MX6[Q,D,UL]
and etc to differentiate them.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Support psci off and affinity.
To i.MX6, CPU could not offline itself, so needs to use core0 to
offline other cores.

Introduce imx-common.c to include the common code for i.MX family,
SRC operation is used by i.MX6/7, so move them to imx-common.c

Use CFG_BOOT_SECONDARY_REQUEST to wrap the psci_cpu_on/off/affinity
functions, these functions are only needed by SMP systems.To i.MX6UL,
they are not needed.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Switch to use CFG_SECURE_TIME_SOURCE_REE.
Since we do not have RTC, and arm counter will lose power when suspend,
we use CFG_SECURE_TIME_SOURCE_REE now.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Add platform early init code.

Configure ACTLR to enable SMP.
Configure NSACR to let NS could access cp10/cp11 and NS_SMP.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
Add i.MX6 ULL EVK support.
i.MX6ULL is derivative from i.MX6UL, so reuse some code for i.MX6UL.

Signed-off-by: Peng Fan <[email protected]>
Acked-by: Etienne Carriere <[email protected]>
Acked-by: Jens Wiklander <[email protected]>
@MrVan
Copy link
Contributor Author

MrVan commented Jun 26, 2017

Rebased and Ack tags applied.

@jforissier jforissier merged commit 796b7a4 into OP-TEE:master Jun 26, 2017
@MrVan MrVan deleted the psci-imx branch June 26, 2017 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants