add get_base and get_io_base helper function #1570

MrVan · 2017-05-31T02:17:04Z

Add get_base and get_io_base function helper function. These two functions are only to wrap phys_to_virt and phy_to_virt_io and add cpu_mmu_enabled checking.

vchong · 2017-05-31T02:59:10Z

+1

jenswi-linaro · 2017-05-31T05:50:58Z

Is the get_base() function needed by something?
Please add a proper prefix to the function names, like for instance core_mmu_

MrVan · 2017-05-31T06:49:55Z

@jenswi-linaro Currently not. In my local repo, I use get_base for suspend/resume case, it could make code looks clean. You mean change the function name to core_mmu_get_base and core_mmu_get_io_base?

jforissier

+1 with Jens' comment, core_mmu_ is a good prefix IMO.

I'm not sure about the word "base": what does it mean? Any address can be passed. It's not like you would return the base for some area given any address in the area (that's how it sounds to me). How about core_mmu_get_va(paddr_t pa) and core_mmu_get_io_va(paddr_t pa)?

Last thing: I am a bit reluctant to merge something that's not used yet. Because, sometimes we do some cleanup and we do remove unused code. So, I think these patches would be fine as part of a bigger series.

jforissier · 2017-05-31T07:38:49Z

core/arch/arm/include/mm/core_memprot.h

@@ -106,4 +106,9 @@ void *phys_to_virt_io(paddr_t pa);
 */
 paddr_t virt_to_phys(void *va);

+/*
+ * Return runtime usable address


"Return runtime usable address, irrespective of whether the MMU is enabled or not"

jforissier · 2017-05-31T07:42:44Z

core/arch/arm/include/mm/core_memprot.h

@@ -111,4 +111,9 @@ paddr_t virt_to_phys(void *va);
 */
 vaddr_t get_base(paddr_t base, enum teecore_memtypes type);

+/*
+ * Return runtime usable io address


"Return runtime usable io address, irrespective of whether the MMU is enabled or not. @base has to be in a memory area of type MEM_AREA_IO_SEC or MEM_AREA_IO_NSEC. Returns NULL on failure."

MrVan · 2017-05-31T08:28:24Z

@jforissier Thanks for comments. I'll drop the get_base patch. the get_io_base will be used in my following up patch.

MrVan · 2017-05-31T10:02:58Z

@jenswi-linaro @jforissier Updated, drop the get_base code, rename get_io_base to core_mmu_get_io_va and use core_mmu_get_io_va to cleanup the imx code.

jforissier · 2017-05-31T11:06:26Z

Thanks @MrVan , looks good.
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

jenswi-linaro · 2017-05-31T11:11:23Z

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

etienne-lms · 2017-05-31T12:38:03Z

core/arch/arm/include/mm/core_memprot.h

+/*
+ * Return runtime usable io address
+ */
+vaddr_t get_io_base(paddr_t base);


get_io_base seems ok for devices that may have a non secure status with any impact on the core behaviour. I.e uart-like console for debug.

I understand the get_base() as it provides the target memtype hence to expected attributes.
Some core_mmu_aware_phys_to_virt(). with some shorter name :)

etienne-lms · 2017-05-31T12:43:22Z

core/arch/arm/plat-imx/imx_pl310.c

-		return (vaddr_t)va;
-	}
-	return PL310_BASE;
+	return core_mmu_get_io_va(PL310_BASE);


I think we should assert pl310 is secure and use the memtype way.

I could not follow you about using memtype and assert pl310 is secure. the phys_to_virt_io will find the map in secure mapping, then non-secure if not found in secure.

Ok, I did not look back few lines above. This source file does register the pl310 physical iomem range as MEM_AREA_IO_SEC. So ok, not need to crosscheck this, as the previous implementation did by explicitly providing MEM_AREA_IO_SEC type as argument.

But... I feel it is more simple to call core_mmu_get_va(PL310_BASE, MEM_AREA_IO_SEC);. This is why I liked the previously proposed core_mmu_get_va(): caller explicitly specific to required state.

More generally, I think the phys_to_virt_io() api should be reserved to iomem for which the secure state does not really matter. @jforissier ?

Another point. previous code used a local static storage for the va. Hence conversion is done only once.
With this change, phys_to_virt_io() is called each time pl310 is used.

@etienne-lms +1 with your statement: "phys_to_virt_io() api should be reserved to iomem for which the secure state does not really matter", that's exactly the purpose of the function (otherwise use phys_to_virt()), and in my mind it was the meaning of the "_io" suffix: it means "any of the _IO_ memtyptes (secure or non-secure)".

So, for consistency with this naming scheme, we should have core_mmu_get_io_va() without a memtype parameter. And it's OK to use it here (imx_pl310.c) since as you said above the mem range is registered secure above.

Now, I don't mind also having core_mmu_get_va(pa, type) to complete the picture... but that can be added when needed.

etienne-lms · 2017-05-31T13:11:45Z

core/arch/arm/plat-imx/main.c

-	gicd_base = (vaddr_t)phys_to_virt(GIC_BASE + GICD_OFFSET,
-					  MEM_AREA_IO_SEC);
+	gicc_base = core_mmu_get_io_va(GIC_BASE + GICC_OFFSET);
+	gicd_base = core_mmu_get_io_va(GIC_BASE + GICD_OFFSET);


~~i feel we could insure gic is secure.~~
~~Some assert should check gic was mapped secure. may at gic inits.~~

(edited) from the discussions: Ok, here no need to assert GIC secure state registration. I will remove my comment.

etienne-lms · 2017-05-31T13:20:54Z

core/arch/arm/plat-imx/psci.c

 int psci_cpu_on(uint32_t core_idx, uint32_t entry,
 		uint32_t context_id __attribute__((unused)))
 {
 	uint32_t val;
-	vaddr_t va = src_base();
+	vaddr_t va = core_mmu_get_io_va(SRC_BASE);


Same comment. I think you should at least assert somewhere that va is MEM_AREA_IO_SEC. Or use the previously proposed core_mmu_get_va(paddr_t, enum memarea_type). Sound like a early_virt_to_phys().

Complementary to discussions above, here register_phys_mem(SRC_BASE, ...); is externally defined.

So if SRC_BASE is really expected secure, this generic psci.c should at least assert the secure state of the target ram. If the secure state does not matter, then fine.
And same remark, why not simply call a generic core_mmu_get_va(SRC_BASE, MEM_AREA_IO_SEC)?

@etienne-lms ha! yes maybe here core_mmu_get_va(SRC_BASE, MEM_AREA_IO_SEC) would be nice (if we want to check the secure state that is, I have no strong opinion on this).

@etienne-lms I thought core_mmu_get_va_io is ok, but not take secure state into consideration. Then I may need to add back core_mmu_get_va. And use core_mmu_get_va.

MrVan · 2017-06-01T09:49:51Z

@etienne-lms I updated the code with your comments addressed, using core_mmu_get_va.

etienne-lms · 2017-06-01T10:05:00Z

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Thanks.
Just note the pl310 that calls address conversion at each maintenance operation on pl310. May not be an issue.

jforissier · 2017-06-01T11:09:19Z

So we have core_mmu_get_io_va() that is defined but not used... but it's OK, it obviously goes with core_mmu_get_va(). Thanks @MrVan.
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

MrVan · 2017-06-01T12:18:52Z

@jforissier Updated with review tags applied. Thanks.

etienne-lms

Frankly i don't like core_mmu_get_io_va(). That's label buiness only but such sec-or-nonsec devices are, up to my experience, only supported on debug purpose. I would rather rename core_mmu_get_iodbg_va(). And same for phys_to_virt_io(). so that's a separate P-R maybe. And that is only but labels.

@MrVan if you need core_mmu_get_io_va() then include it in the P-R. If not, prefer don't include.

Fix needed in a comment.

etienne-lms · 2017-06-01T12:22:31Z

core/arch/arm/include/mm/core_memprot.h

+ * Return runtime usable io address, irrespective of whether
+ * the MMU is enabled or not.
+ * @base has to be in a memory area of type MEM_AREA_IO_SEC or
+ * MEM_AREA_IO_NSEC. Returns NULL on failure.


base should be pa.

Suggestion for the description, inspired from from phys_to_virt_io())

- * @base has to be in a memory area of type MEM_AREA_IO_SEC or - * MEM_AREA_IO_NSEC. Returns NULL on failure. + * Translate physical address to virtual address trying MEM_AREA_IO_SEC + * first then MEM_AREA_IO_NSEC if not found. + * Returns NULL on failure or a valid virtual address on success.

MrVan · 2017-06-01T12:55:34Z

@etienne-lms The phys_to_virt_io is used by the uart functions. I do not want to change that in this patch set. I could drop it.

Add core_mmu_get_va helper function. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

Use core_mmu_get_va to simplify the code. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org> Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>

MrVan · 2017-06-01T13:00:55Z

Updated with the get_io_va patch dropped, since not used by this patch set.

jforissier reviewed May 31, 2017

View reviewed changes

MrVan force-pushed the get_base branch from 932f8c0 to 222db71 Compare May 31, 2017 09:58

etienne-lms reviewed May 31, 2017

View reviewed changes

MrVan force-pushed the get_base branch from 222db71 to 223a652 Compare June 1, 2017 09:48

MrVan force-pushed the get_base branch from 223a652 to 97a840a Compare June 1, 2017 12:17

etienne-lms reviewed Jun 1, 2017

View reviewed changes

MrVan added 2 commits June 1, 2017 20:56

core: mmu: add core_mmu_get_va helper function

d1781f0

Add core_mmu_get_va helper function. Signed-off-by: Peng Fan <peng.fan@nxp.com> Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org> Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

MrVan force-pushed the get_base branch from 97a840a to d9baf16 Compare June 1, 2017 12:58

MrVan mentioned this pull request Jun 2, 2017

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

Merged

jforissier merged commit 351b242 into OP-TEE:master Jun 2, 2017

MrVan deleted the get_base branch September 29, 2017 02:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add get_base and get_io_base helper function #1570

add get_base and get_io_base helper function #1570

MrVan commented May 31, 2017

vchong commented May 31, 2017

jenswi-linaro commented May 31, 2017

MrVan commented May 31, 2017

jforissier left a comment

jforissier May 31, 2017

jforissier May 31, 2017

MrVan commented May 31, 2017

MrVan commented May 31, 2017

jforissier commented May 31, 2017

jenswi-linaro commented May 31, 2017

etienne-lms May 31, 2017

etienne-lms May 31, 2017

MrVan Jun 1, 2017

etienne-lms Jun 1, 2017

etienne-lms Jun 1, 2017

jforissier Jun 1, 2017

etienne-lms May 31, 2017 •

edited

Loading

etienne-lms May 31, 2017

etienne-lms Jun 1, 2017

jforissier Jun 1, 2017

MrVan Jun 1, 2017

MrVan commented Jun 1, 2017

etienne-lms commented Jun 1, 2017

jforissier commented Jun 1, 2017

MrVan commented Jun 1, 2017

etienne-lms left a comment

etienne-lms Jun 1, 2017

MrVan commented Jun 1, 2017

MrVan commented Jun 1, 2017

add get_base and get_io_base helper function #1570

add get_base and get_io_base helper function #1570

Conversation

MrVan commented May 31, 2017

vchong commented May 31, 2017

jenswi-linaro commented May 31, 2017

MrVan commented May 31, 2017

jforissier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented May 31, 2017

MrVan commented May 31, 2017

jforissier commented May 31, 2017

jenswi-linaro commented May 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

etienne-lms May 31, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Jun 1, 2017

etienne-lms commented Jun 1, 2017

jforissier commented Jun 1, 2017

MrVan commented Jun 1, 2017

etienne-lms left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrVan commented Jun 1, 2017

MrVan commented Jun 1, 2017

etienne-lms May 31, 2017 •

edited

Loading