IPMMU-VMSA updates #110
IPMMU-VMSA updates #110
Conversation
|
@iartemenko please merge |
| @@ -1944,6 +1950,9 @@ static int ipmmu_probe(struct platform_device *pdev) | |||
| else | |||
| mmu->num_ctx = 1; | |||
|
|
|||
| mmu->num_ctx = min_t(unsigned int, CONFIG_IPMMU_VMSA_CTX_NUM, | |||
| mmu->num_ctx); | |||
There was a problem hiding this comment.
I don't see how this change is related to M3N IPMMU
There was a problem hiding this comment.
This patch does sync with recent IPMMU-VMSA driver from Linux's rcar-3.5.9 branch. I have applied diff between two versions of driver.
There was a problem hiding this comment.
Commit message says "This patch enables IPMMU feature for M3N SoC.". Also, I can't see any benefit of this change.
According to code right above, mmu->num_ctx will be either 1 or 8.
There was a problem hiding this comment.
I can extend commit message to explain which Linux's commits it contains:
- iommu/ipmmu-vmsa: Add support for r8a77965 SoC
- iommu/ipmmu-vmsa: Add override support for the actual number of MMU contexts
Sounds good?
There was a problem hiding this comment.
But I still see no benefit in this change. Why it is needed in the first place?
There was a problem hiding this comment.
Just to be completely aligned with BSP's driver. That was the reason. You know that for IPMMU we try to retain everything to be as close as possible to Linux driver
There was a problem hiding this comment.
Yeah, but I can't imagine how are you going to upstream this this into XEN, providing that original driver is not upstreamed into Linux kernel.
There was a problem hiding this comment.
I believe that all required changes to the original driver will be reached upstream. So, BSP driver will be equal to the mainline driver
| static void __iomem *rcar_sysc_base = NULL; | ||
|
|
||
| /* SYSC MMIO range */ | ||
| #define RCAR_SYSC_BASE 0xe6180000 |
There was a problem hiding this comment.
Shouldn't this be read from DTB?
There was a problem hiding this comment.
In the whole it is possible, but requires "some" work to do for not so significant benefit. Xen should know about such device as Renesas SYSC the first to retrieve it's MMIO, I think.
| u32 syscier, syscimr; | ||
| int i; | ||
|
|
||
| if (rcar_sysc_base) |
There was a problem hiding this comment.
Do you expect that rcar_sysc_init will be called twice? Then you need comment there.
There was a problem hiding this comment.
Will add a comment.
There was a problem hiding this comment.
I think it is better to do call this once from ipmmu_preinit.
Also you forgot to unmap that region.ipmmu_preinit is also a good place to unmap it.
There was a problem hiding this comment.
I am afraid, no. ipmmu_preinit is called for "each" IPMMU IP.
There was a problem hiding this comment.
Okay. But at least it will not be called for each rcar_sysc_power_is_off and rcar_sysc_power_on
There was a problem hiding this comment.
Don't mind. I will minimize the places for it to be called from.
| for (i = 0; i < PWRER_RETRIES; i++) { | ||
|
|
||
| /* Wait until SYSC is ready to accept a power request */ | ||
| for (j = 0; j < SYSCSR_RETRIES; j++) { |
There was a problem hiding this comment.
I think such retry loops should be written as following:
retires_left = SYSCSR_RETRIES;
while (! (readl(rcar_sysc_base + SYSCSR) & BIT(SYSCSR_PONENB)) {
udelay(SYSCSR_DELAY_US);
if (retries_left-- <= 0)
return -EAGAIN;
}
or
num_retries = 0;
while (! (readl(rcar_sysc_base + SYSCSR) & BIT(SYSCSR_PONENB)) {
udelay(SYSCSR_DELAY_US);
if (num_retries++ > SYSCSR_RETRIES )
return -EAGAIN;
}
This is much shorter and this will ensure that you compare against SYSCSR_RETRIES exactly in one place.
There was a problem hiding this comment.
You know, I was trying the retain code as it was done in Linux for sync with the driver changes in the future easily.
There was a problem hiding this comment.
If you insist on changing I will change. But I would prefer to leave as is.
There was a problem hiding this comment.
I'm okay with this. Just showing how to do it more error-proof.
| */ | ||
| if (domain->mmus[0]->is_mmu_tlb_disabled) | ||
| ipmmu_ctx_write1(domain, IMSCTLR, | ||
| ipmmu_ctx_read(domain, IMSCTLR) | 0xE0000000); |
There was a problem hiding this comment.
0xE0000000 is the some magic value.
There was a problem hiding this comment.
Yes, the code of mmngr [1] I used as a reference, operated with the same magic value
[1] renesas-rcar/mmngr_drv@4aed49e
Do you want me to define it somehow?
There was a problem hiding this comment.
Yes, I think it should be defined. Renesas code is a bad excuse. I'm sure, it will not pass code review in LKML.
|
@iartemenko please postpone with merging this stuff till tomorrow, I will address comments |
|
|
|
@lorc I sent a follow-up patch to show what I had changed, if you don't have objections I will squash it with series. |
This patch contains following Linux's commits: 1. iommu/ipmmu-vmsa: Add support for r8a77965 SoC 2. iommu/ipmmu-vmsa: Add override support for the actual number of MMU contexts Signed-off-by: Oleksandr Tyshchenko <[email protected]> Reviewed-by: Andrii Anisov [email protected] Reviewed-by: Volodymyr Babchuk <[email protected]>
Some IPMMU-XX are not located in ALWAYS_ON power domain (IPMMU-VP0, IPMMU-VC0 belong to A3xx power domains) and as the result they are in power-off state during booting, therefore they must be explicitly powered on before initializing. Signed-off-by: Oleksandr Tyshchenko <[email protected]> Reviewed-by: Andrii Anisov [email protected] Reviewed-by: Volodymyr Babchuk <[email protected]>
Disable IPMMU TLB cache function of IPMMU caches that belong to non ALWAYS_ON power domain (IPMMU-VP0, IPMMU-VC0 belong to A3xx power domains) due to H/W restriction. According to the procedure being performed in MMNGR driver: https://github.com/renesas-rcar/mmngr_drv Signed-off-by: Oleksandr Tyshchenko <[email protected]> Reviewed-by: Andrii Anisov [email protected] Reviewed-by: Volodymyr Babchuk <[email protected]>
|
@iartemenko pls merge this one as well as others: |
In a nutshell this patch series does: